added express error handling middleware and fixed cases for columns i…#287
Open
added express error handling middleware and fixed cases for columns i…#287
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR renames telemetry fields from PascalCase to camelCase (Rfid→rfid, Timestamp/TimeStamp→timestamp) across shared types, DB entities, repositories, and services; disables AppDataSource logging; adds centralized Express error handling (asyncHandler + errorHandler) and updates route/controller error propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressRoute as "Express Route\n(asyncHandler)"
participant Controller
participant DB as "Database (TypeORM)"
participant ErrorHandler as "errorHandler"
Client->>ExpressRoute: HTTP request
ExpressRoute->>Controller: invoke handler
Controller->>DB: insert/query (uses rfid/timestamp)
DB-->>Controller: success / error
alt success
Controller-->>Client: 200/response
else error
Controller-->>ErrorHandler: throw / next(err)
ErrorHandler-->>Client: 5xx with logged details
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces centralized Express error handling for async route handlers and aligns TimescaleDB entity/query field casing (notably timestamp/rfid) to prevent column-name/case mismatches.
Changes:
- Added
asyncHandler+errorHandlermiddleware and wired it into the server and selected routes/controllers. - Renamed lap/driver timestamp+rfid fields to lowercase across shared types, DB entities, and DB queries.
- Wrapped DB insertions in try/catch to prevent unhandled rejections from crashing long-running services.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/types.ts | Updates ILapData field names/types to timestamp/rfid. |
| packages/server/src/routes/playback.route.ts | Wraps playback endpoints with asyncHandler. |
| packages/server/src/routes/lap.route.ts | Wraps laps endpoint with asyncHandler. |
| packages/server/src/routes/driver.route.ts | Wraps driver endpoints with asyncHandler. |
| packages/server/src/middleware/errorHandler.ts | Adds async wrapper + centralized Express error middleware. |
| packages/server/src/index.ts | Registers the global errorHandler after routes. |
| packages/server/src/controllers/routeControllers/playback.controller.ts | Switches controller error handling to next(err). |
| packages/server/src/controllers/routeControllers/lap.controller.ts | Switches controller error handling to next(err). |
| packages/server/src/controllers/routeControllers/driver.controller.ts | Switches controller error handling to next(err). |
| packages/server/src/controllers/LapController/LapController.ts | Updates lap payload fields and wraps DB insert with try/catch. |
| packages/server/src/controllers/BackendController/BackendController.ts | Awaits + catches packet DB insertion errors before continuing. |
| packages/db/src/services/DatabaseService.ts | Updates queries to rfid/timestamp, updates lap insert signature, changes disconnected behavior. |
| packages/db/src/repositories/BaseRepository.ts | Aligns base entity PK fields to timestamp/rfid. |
| packages/db/src/entities/Lap.entity.ts | Renames columns/index to timestamp/rfid. |
| packages/db/src/entities/Driver.entity.ts | Renames primary key field to rfid. |
| packages/db/src/data-source.ts | Disables TypeORM logging unconditionally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
421
to
+433
| export interface ILapData { | ||
| AmpHours: number; | ||
| AveragePackCurrent: number; | ||
| AverageSpeed: number; | ||
| BatterySecondsRemaining: number; | ||
| Distance: number; | ||
| EnergyConsumed: number; | ||
| LapTime: number; | ||
| NetPowerOut: number; | ||
| TimeStamp: number; | ||
| timestamp: Date; | ||
| TotalPowerIn: number; | ||
| TotalPowerOut: number; | ||
| Rfid: string; | ||
| rfid: string; |
There was a problem hiding this comment.
ILapData now uses lowercase timestamp/rfid, but related exported types in the same module still use uppercase (IDriverData.Rfid, IFormattedLapData.TimeStamp/Rfid). This inconsistency will cause avoidable friction and type errors across packages; align the naming across the shared types (or introduce separate DTO types for DB vs API/client).
| "/firstAndLastPacket", | ||
| asyncHandler(controllers.getFirstAndLastPacket), | ||
| ); | ||
| playbackRouter.get("playback/health", controllers.getHealthPlayback); |
There was a problem hiding this comment.
Route path is missing a leading / ("playback/health"), so this handler may not be reachable at the intended URL (typically /playback/health). Update the path string to start with / for consistent routing.
Suggested change
| playbackRouter.get("playback/health", controllers.getHealthPlayback); | |
| playbackRouter.get("/playback/health", controllers.getHealthPlayback); |
Comment on lines
59
to
+65
| public async handlePacketReceive(message: ITelemetryData) { | ||
| // Insert the packet into the database | ||
| this.timescaleDB.insertPacketData(message); | ||
| try { | ||
| await this.timescaleDB.insertPacketData(message); | ||
| } catch (error) { | ||
| logger.error("Failed to insert packet data:", error); | ||
| } |
There was a problem hiding this comment.
handlePacketReceive now awaits the DB insert before broadcasting the packet and processing laps. This can significantly increase end-to-end latency and reduce throughput if the DB is slow. If ordering isn’t required, consider making the insert non-blocking (e.g., fire-and-forget with an attached .catch(...)) so packet broadcast/lap processing aren’t gated on DB write latency.
Comment on lines
23
to
+27
| export const AppDataSource = new DataSource({ | ||
| database: "tsdb", | ||
| entities: [TelemetryPacket, Driver, Lap], | ||
| host: process.env.DATABASE_HOST, | ||
| logging: !isProd, | ||
| logging: false, |
There was a problem hiding this comment.
logging is now hard-coded to false, removing the previous environment-based logging behavior. This reduces DB/ORM observability during development and troubleshooting. Consider restoring env-driven logging (or making it configurable) unless always-off logging is a deliberate requirement.
Comment on lines
+430
to
+433
| timestamp: Date; | ||
| TotalPowerIn: number; | ||
| TotalPowerOut: number; | ||
| Rfid: string; | ||
| rfid: string; |
There was a problem hiding this comment.
ILapData was changed to timestamp: Date and rfid: string, but this type is consumed by the client (and API responses / Socket.IO payloads) where Date will be serialized as a string. This will break current client code that expects TimeStamp/Rfid and will also mis-type the timestamp field. Consider keeping the public/shared shape stable (e.g., TimeStamp: number / Rfid: string) or updating all consumers + using a JSON-safe type like timestamp: string | number in the shared contract.
Comment on lines
+13
to
+31
| export const errorHandler = ( | ||
| err: Error, | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction, | ||
| ) => { | ||
| const logger = createApplicationLogger("errorHandler", req, res); | ||
|
|
||
| logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`); | ||
| if (err.stack) { | ||
| logger.error(`Stack trace: ${err.stack}`); | ||
| } | ||
|
|
||
| const statusCode = res.statusCode !== 200 ? res.statusCode : 500; | ||
|
|
||
| res.status(statusCode).json({ | ||
| message: err.message || "Internal Server Error", | ||
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | ||
| }); |
There was a problem hiding this comment.
errorHandler always attempts to write a JSON response. If headers have already been sent, this will trigger ERR_HTTP_HEADERS_SENT and can mask the original error. Add a if (res.headersSent) return next(err); guard (and consider typing err as unknown/any or ErrorRequestHandler), and rename next to _next if intentionally unused to satisfy the repo’s unused-args convention.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/server/src/index.ts (1)
52-53: Consider a dedicated 404 middleware beforeerrorHandler.This will ensure unknown routes return the same JSON error envelope instead of default Express behavior.
Optional follow-up
app.use("/", machineLearningRouter); +app.use((req, res, next) => { + res.status(404); + next(new Error("Route not found")); +}); + app.use(errorHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/index.ts` around lines 52 - 53, Add a dedicated 404 middleware placed immediately before the existing app.use(errorHandler) so unknown routes return the same JSON error envelope; implement a middleware (e.g., notFoundHandler or inline function) that captures unmatched requests, sets status 404 and responds with the same JSON shape your errorHandler expects (message, status/code, and any envelope keys), then call next() or end the response—register this notFoundHandler before the errorHandler registration to ensure consistent JSON responses for unknown routes.packages/server/src/controllers/LapController/LapController.ts (1)
185-188: Avoid a no-op geofence completion branch with a success log.Line 187 comments out the only action in this branch, but Line 186 still logs completion. This is misleading operationally and can drift behavior from intent. Either remove this branch or guard it with an explicit feature flag until geofence handling is reintroduced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/controllers/LapController/LapController.ts` around lines 185 - 188, The branch that logs "lap completed for geofence" currently does nothing because handleGeofenceLap is commented out; update the check in LapController so it either removes the branch entirely or re-enables it behind an explicit feature flag (e.g., ENABLE_GEOFENCE_HANDLING): if you choose the flag, add a boolean config (or env) and change the block around checkLap(packet) && this.lastLapPackets.length > 5 to only call logger.info and invoke this.handleGeofenceLap(packet.Pi.Rfid, packet.TimeStamp) when the flag is true; otherwise remove the log or change it to a debug statement to avoid misleading operational logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db/src/entities/Lap.entity.ts`:
- Line 9: The change in Lap.entity.ts reverts column names to lowercase ("rfid",
"timestamp") but if the production DB was already deployed with PascalCase
columns (Rfid, Timestamp) this will break queries because data-source.ts uses
synchronize: false and migrations are empty; confirm whether commit 431c1dfe was
applied to prod and if so generate and run a migration that renames the columns
Rfid->rfid and Timestamp->timestamp (or vice versa if prod has lowercase) before
deploying, using your migration tooling (migrate:generate / migrate:run) and
reference the Lap entity/@Index change when creating the migration to ensure
column names match the entity.
In `@packages/db/src/services/DatabaseService.ts`:
- Around line 329-330: Replace the silent console.warn + return [] in
DatabaseService with an explicit error path: remove console.warn("Database not
connected") (fixing the no-console lint) and instead log via the service logger
(e.g., this.logger.error(...)) and throw an Error (e.g., throw new
Error("Database not connected")) so callers receive a failure rather than an
empty result; update the method in DatabaseService that contains the
console.warn/return [] to propagate the error to callers.
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 159-166: The handleLapData method currently awaits
broadcastLapData and publishLapData outside the try/catch, allowing socket/MQTT
rejections to become unhandled; wrap the three awaited
calls—this.backendController.socketIO.broadcastLapData(lapData),
this.backendController.mqtt.publishLapData(lapData), and
this.backendController.timescaleDB.insertLapData(lapData)—inside a single
try/catch so all failures are caught, await each call (no fire-and-forget), and
replace console.error with the shared logger (e.g., this.logger.error or
backendController.logger) to log the error consistently.
In `@packages/server/src/middleware/errorHandler.ts`:
- Around line 13-31: In errorHandler, first check res.headersSent and if true
call next(err) immediately to avoid writing headers again; then compute
statusCode as before and log via createApplicationLogger, but when building the
JSON response do not return raw err.message in non-development
environments—return a generic "Internal Server Error" (only include err.message
and stack when process.env.NODE_ENV === "development"); ensure logger still
records the full err.message and stack for diagnostics while the client sees the
sanitized message.
---
Nitpick comments:
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 185-188: The branch that logs "lap completed for geofence"
currently does nothing because handleGeofenceLap is commented out; update the
check in LapController so it either removes the branch entirely or re-enables it
behind an explicit feature flag (e.g., ENABLE_GEOFENCE_HANDLING): if you choose
the flag, add a boolean config (or env) and change the block around
checkLap(packet) && this.lastLapPackets.length > 5 to only call logger.info and
invoke this.handleGeofenceLap(packet.Pi.Rfid, packet.TimeStamp) when the flag is
true; otherwise remove the log or change it to a debug statement to avoid
misleading operational logs.
In `@packages/server/src/index.ts`:
- Around line 52-53: Add a dedicated 404 middleware placed immediately before
the existing app.use(errorHandler) so unknown routes return the same JSON error
envelope; implement a middleware (e.g., notFoundHandler or inline function) that
captures unmatched requests, sets status 404 and responds with the same JSON
shape your errorHandler expects (message, status/code, and any envelope keys),
then call next() or end the response—register this notFoundHandler before the
errorHandler registration to ensure consistent JSON responses for unknown
routes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/db/src/data-source.tspackages/db/src/entities/Driver.entity.tspackages/db/src/entities/Lap.entity.tspackages/db/src/repositories/BaseRepository.tspackages/db/src/services/DatabaseService.tspackages/server/src/controllers/BackendController/BackendController.tspackages/server/src/controllers/LapController/LapController.tspackages/server/src/controllers/routeControllers/driver.controller.tspackages/server/src/controllers/routeControllers/lap.controller.tspackages/server/src/controllers/routeControllers/playback.controller.tspackages/server/src/index.tspackages/server/src/middleware/errorHandler.tspackages/server/src/routes/driver.route.tspackages/server/src/routes/lap.route.tspackages/server/src/routes/playback.route.tspackages/shared/src/types.ts
Comment on lines
+13
to
+31
| export const errorHandler = ( | ||
| err: Error, | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction, | ||
| ) => { | ||
| const logger = createApplicationLogger("errorHandler", req, res); | ||
|
|
||
| logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`); | ||
| if (err.stack) { | ||
| logger.error(`Stack trace: ${err.stack}`); | ||
| } | ||
|
|
||
| const statusCode = res.statusCode !== 200 ? res.statusCode : 500; | ||
|
|
||
| res.status(statusCode).json({ | ||
| message: err.message || "Internal Server Error", | ||
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the error handler file
find . -name "errorHandler.ts" -type f | head -20Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 123
🏁 Script executed:
# Check if file exists and read its content
cat -n packages/server/src/middleware/errorHandler.tsRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 1250
🏁 Script executed:
# Search for error handler imports/usage in the codebase
rg "errorHandler" --type ts --type js -B 2 -A 2 | head -100Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 2585
Harden the central error middleware for production safety.
Two fixes are needed: (1) guard res.headersSent before writing a response to prevent crashes when the error handler is invoked after headers have already been sent, and (2) avoid exposing raw internal err.message in production responses.
Suggested fix
export const errorHandler = (
err: Error,
req: Request,
res: Response,
next: NextFunction,
) => {
+ if (res.headersSent) {
+ return next(err);
+ }
+
const logger = createApplicationLogger("errorHandler", req, res);
logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`);
if (err.stack) {
logger.error(`Stack trace: ${err.stack}`);
}
const statusCode = res.statusCode !== 200 ? res.statusCode : 500;
res.status(statusCode).json({
- message: err.message || "Internal Server Error",
+ message:
+ process.env.NODE_ENV === "development"
+ ? err.message || "Internal Server Error"
+ : "Internal Server Error",
...(process.env.NODE_ENV === "development" && { stack: err.stack }),
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const errorHandler = ( | |
| err: Error, | |
| req: Request, | |
| res: Response, | |
| next: NextFunction, | |
| ) => { | |
| const logger = createApplicationLogger("errorHandler", req, res); | |
| logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`); | |
| if (err.stack) { | |
| logger.error(`Stack trace: ${err.stack}`); | |
| } | |
| const statusCode = res.statusCode !== 200 ? res.statusCode : 500; | |
| res.status(statusCode).json({ | |
| message: err.message || "Internal Server Error", | |
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | |
| }); | |
| export const errorHandler = ( | |
| err: Error, | |
| req: Request, | |
| res: Response, | |
| next: NextFunction, | |
| ) => { | |
| if (res.headersSent) { | |
| return next(err); | |
| } | |
| const logger = createApplicationLogger("errorHandler", req, res); | |
| logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`); | |
| if (err.stack) { | |
| logger.error(`Stack trace: ${err.stack}`); | |
| } | |
| const statusCode = res.statusCode !== 200 ? res.statusCode : 500; | |
| res.status(statusCode).json({ | |
| message: | |
| process.env.NODE_ENV === "development" | |
| ? err.message || "Internal Server Error" | |
| : "Internal Server Error", | |
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/middleware/errorHandler.ts` around lines 13 - 31, In
errorHandler, first check res.headersSent and if true call next(err) immediately
to avoid writing headers again; then compute statusCode as before and log via
createApplicationLogger, but when building the JSON response do not return raw
err.message in non-development environments—return a generic "Internal Server
Error" (only include err.message and stack when process.env.NODE_ENV ===
"development"); ensure logger still records the full err.message and stack for
diagnostics while the client sees the sanitized message.
skand088
force-pushed
the
TEL-328-implement-redo-for-database-connection-and-sender
branch
from
ca50af7 to
e0e5f3b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/middleware/errorHandler.ts (1)
13-32:⚠️ Potential issue | 🟠 MajorHarden error middleware for production safety.
Two issues need to be addressed:
Missing
res.headersSentguard: If error handler is invoked after headers are sent, callingres.status().json()will crash. Checkres.headersSentfirst and delegate to Express's default error handler if true.Raw error message exposed in production: Line 29 exposes
err.messageregardless of environment, potentially leaking internal details to clients. Only expose detailed messages in development.Recommended fix
export const errorHandler = ( err: Error, req: Request, res: Response, next: NextFunction, ) => { + if (res.headersSent) { + return next(err); + } + const logger = createApplicationLogger("errorHandler", req, res); logger.error(`ERROR - ${req.method} ${req.url} - ${err.message}`); if (err.stack) { logger.error(`Stack trace: ${err.stack}`); } const statusCode = res.statusCode !== 200 ? res.statusCode : 500; res.status(statusCode).json({ - message: err.message || "Internal Server Error", + message: + process.env.NODE_ENV === "development" + ? err.message || "Internal Server Error" + : "Internal Server Error", ...(process.env.NODE_ENV === "development" && { stack: err.stack }), }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/middleware/errorHandler.ts` around lines 13 - 32, The errorHandler middleware currently calls res.status(...).json(...) even when headers are already sent and always returns err.message to clients; update the errorHandler function to first check res.headersSent and if true, call next(err) to let Express handle it; then, when sending a JSON response, avoid exposing raw err.message in non-development environments by sending a generic message (e.g., "Internal Server Error") unless process.env.NODE_ENV === "development", and only include stack when in development; ensure you still set statusCode from res.statusCode !== 200 ? res.statusCode : 500 before responding.
🧹 Nitpick comments (4)
packages/server/src/controllers/LapController/LapController.ts (1)
223-223:handleLapDatais not awaited, creating a fire-and-forget pattern.While
handleLapDatahas internal error handling, not awaiting it means:
- The caller cannot know when lap data handling completes
- Errors are silently logged but not propagated
- If
handlePacketis called rapidly, concurrent lap data operations may overlapConsider awaiting the call or documenting why fire-and-forget is acceptable here.
Option: Await the call
- this.handleLapData(lapData); + await this.handleLapData(lapData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/controllers/LapController/LapController.ts` at line 223, The call to this.handleLapData(lapData) in handlePacket is currently fire-and-forget; change handlePacket to await this.handleLapData(lapData) (and make handlePacket async if it isn't) so the caller can observe completion and errors; if you need to keep internal error handling ensure handleLapData still throws or returns rejected promises when appropriate, and update callers of handlePacket to handle the returned promise (or explicitly document why a non-awaited fire-and-forget is acceptable).packages/db/src/services/DatabaseService.ts (1)
141-151: Consider replacingconsole.errorwith the application logger.Multiple
console.errorcalls throughout this file (lines 149, 160, 166, 179, 208, 216) may triggerno-consolelint errors and create inconsistent logging. The codebase uses structured loggers elsewhere (e.g.,createApplicationLoggerin the server package).Example for getDrivers
+import { createApplicationLogger } from "@/utils/logger"; // or appropriate import + +const logger = createApplicationLogger("DatabaseService"); + public async getDrivers() { try { const drivers = await this.driverRepo.find(); return drivers.map((driver) => ({ rfid: driver.rfid, driver: driver.Name, })); } catch (error: unknown) { - console.error("Error getting drivers"); + logger.error("Error getting drivers", error as Error); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/DatabaseService.ts` around lines 141 - 151, Replace direct console.error usages in DatabaseService methods (e.g., getDrivers) with the application's structured logger: import or accept the existing logger instance used by this service, then change error handling blocks to call logger.error with a descriptive message and the caught error object (preserve context like method name and relevant identifiers). Specifically update getDrivers (and the other methods flagged) to log via the service's logger instead of console.error, ensuring the catch signature captures the error (error: unknown) and you pass the error when logging for structured output and to satisfy lint rules.packages/db/src/entities/Lap.entity.ts (1)
9-18: Partial camelCase migration creates field naming inconsistency.The
rfidandtimestampfields are now camelCase, but other fields likeLapTime,TotalPowerIn,NetPowerOut, etc. remain PascalCase. This creates an inconsistent naming convention within the entity.Consider either:
- Completing the camelCase migration for all fields in a follow-up PR
- Adding a TODO comment documenting the intended naming convention
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/entities/Lap.entity.ts` around lines 9 - 18, The Lap entity shows mixed naming (camelCase: rfid, timestamp vs PascalCase: LapTime, TotalPowerIn, NetPowerOut); update this by either completing the camelCase migration for all properties in class Lap (rename PascalCase fields to camelCase and update any references/usages) or add a clear TODO comment above the Lap class indicating the intended naming convention (camelCase) and that a follow-up PR will complete the renames; reference the class Lap and the fields rfid, timestamp, LapTime, TotalPowerIn, NetPowerOut when making the change so reviewers can find and enforce consistency.packages/shared/src/types.ts (1)
430-433: Naming inconsistency with other interfaces in this file.The
ILapDatainterface now uses lowercasetimestampandrfid, but other interfaces in this file still use PascalCase:
IDriverNameUpdate(line 23):RfidIPiType(line 342):RfidIDriverData(line 418):RfidITelemetryDataType(line 392):TimeStampIFormattedLapData(lines 502-505):TimeStamp,RfidIf the intent is to standardize on camelCase for database fields, consider updating the other interfaces for consistency. If
IFormattedLapDataintentionally uses PascalCase for client display purposes, document this distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/types.ts` around lines 430 - 433, The interfaces have inconsistent casing: ILapData uses timestamp and rfid (camelCase) while IDriverNameUpdate, IPiType, IDriverData, ITelemetryDataType, and IFormattedLapData use PascalCase (Rfid, TimeStamp). Standardize to camelCase for database/domain models: rename TimeStamp -> timeStamp (or timestamp) and Rfid -> rfid in IDriverNameUpdate, IPiType, IDriverData, and ITelemetryDataType to match ILapData (update all references). Keep IFormattedLapData as PascalCase if it’s strictly a client/display DTO, and add a comment above IFormattedLapData documenting that its PascalCase shape is intentional for UI consumption; otherwise convert it to camelCase too for full consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server/src/middleware/errorHandler.ts`:
- Around line 13-32: The errorHandler middleware currently calls
res.status(...).json(...) even when headers are already sent and always returns
err.message to clients; update the errorHandler function to first check
res.headersSent and if true, call next(err) to let Express handle it; then, when
sending a JSON response, avoid exposing raw err.message in non-development
environments by sending a generic message (e.g., "Internal Server Error") unless
process.env.NODE_ENV === "development", and only include stack when in
development; ensure you still set statusCode from res.statusCode !== 200 ?
res.statusCode : 500 before responding.
---
Nitpick comments:
In `@packages/db/src/entities/Lap.entity.ts`:
- Around line 9-18: The Lap entity shows mixed naming (camelCase: rfid,
timestamp vs PascalCase: LapTime, TotalPowerIn, NetPowerOut); update this by
either completing the camelCase migration for all properties in class Lap
(rename PascalCase fields to camelCase and update any references/usages) or add
a clear TODO comment above the Lap class indicating the intended naming
convention (camelCase) and that a follow-up PR will complete the renames;
reference the class Lap and the fields rfid, timestamp, LapTime, TotalPowerIn,
NetPowerOut when making the change so reviewers can find and enforce
consistency.
In `@packages/db/src/services/DatabaseService.ts`:
- Around line 141-151: Replace direct console.error usages in DatabaseService
methods (e.g., getDrivers) with the application's structured logger: import or
accept the existing logger instance used by this service, then change error
handling blocks to call logger.error with a descriptive message and the caught
error object (preserve context like method name and relevant identifiers).
Specifically update getDrivers (and the other methods flagged) to log via the
service's logger instead of console.error, ensuring the catch signature captures
the error (error: unknown) and you pass the error when logging for structured
output and to satisfy lint rules.
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Line 223: The call to this.handleLapData(lapData) in handlePacket is currently
fire-and-forget; change handlePacket to await this.handleLapData(lapData) (and
make handlePacket async if it isn't) so the caller can observe completion and
errors; if you need to keep internal error handling ensure handleLapData still
throws or returns rejected promises when appropriate, and update callers of
handlePacket to handle the returned promise (or explicitly document why a
non-awaited fire-and-forget is acceptable).
In `@packages/shared/src/types.ts`:
- Around line 430-433: The interfaces have inconsistent casing: ILapData uses
timestamp and rfid (camelCase) while IDriverNameUpdate, IPiType, IDriverData,
ITelemetryDataType, and IFormattedLapData use PascalCase (Rfid, TimeStamp).
Standardize to camelCase for database/domain models: rename TimeStamp ->
timeStamp (or timestamp) and Rfid -> rfid in IDriverNameUpdate, IPiType,
IDriverData, and ITelemetryDataType to match ILapData (update all references).
Keep IFormattedLapData as PascalCase if it’s strictly a client/display DTO, and
add a comment above IFormattedLapData documenting that its PascalCase shape is
intentional for UI consumption; otherwise convert it to camelCase too for full
consistency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/client/src/stores/useLapData.tspackages/db/src/data-source.tspackages/db/src/entities/Driver.entity.tspackages/db/src/entities/Lap.entity.tspackages/db/src/repositories/BaseRepository.tspackages/db/src/services/DatabaseService.tspackages/server/src/controllers/BackendController/BackendController.tspackages/server/src/controllers/LapController/LapController.tspackages/server/src/controllers/routeControllers/driver.controller.tspackages/server/src/controllers/routeControllers/lap.controller.tspackages/server/src/controllers/routeControllers/playback.controller.tspackages/server/src/index.tspackages/server/src/middleware/errorHandler.tspackages/server/src/routes/driver.route.tspackages/server/src/routes/lap.route.tspackages/server/src/routes/playback.route.tspackages/shared/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/db/src/repositories/BaseRepository.ts
- packages/db/src/data-source.ts
- packages/server/src/controllers/routeControllers/lap.controller.ts
- packages/server/src/index.ts
- packages/db/src/entities/Driver.entity.ts
- packages/server/src/routes/lap.route.ts
- packages/server/src/controllers/routeControllers/driver.controller.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
added express middleware for error handling without crashing backend services
fixed cases for columns in hypertables that were causing errors (timestamp and rfid)
Summary by CodeRabbit
Bug Fixes
Chores