Update backend controllers with timescale#277
Conversation
…Telemetry into move-to-timescale
…Telemetry into move-to-timescale
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request consolidates the telemetry data model by replacing individual sensor entity tables with a single TelemetryPacket hypertable, migrating the backend from DynamoDB to TimescaleDB-backed storage. It updates client-side PIS components to track field paths and removes isFault flags, while refactoring server layers to use a new DatabaseService. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/amplify/amplify/backend.ts (2)
479-479:⚠️ Potential issue | 🟡 MinorFix typo in comment.
"hte" → "the"
📝 Proposed fix
-// Give DynamoDB Permissions to hte packet data and lap data +// Give DynamoDB Permissions to the packet data and lap data
122-163:⚠️ Potential issue | 🟠 MajorRemove unused DynamoDB tables to eliminate unnecessary AWS costs.
These DynamoDB tables (
packetDataTable,lapDataTable,driverDataTable,gpsCalculatedLapDataTable) are provisioned at lines 122-163 and environment variables are exported at lines 323-327, but the server package does not reference them anywhere. The application now uses TimescaleDB (connection string active at lines 355, 359, 363, 367, 384, 397). These tables should be removed to avoid unnecessary infrastructure costs, unless they're being retained for archived data migration—in which case, add a comment documenting the retention strategy and timeline for deletion.packages/db/src/repositories/BaseRepository.ts (1)
10-19:⚠️ Potential issue | 🟠 MajorPrimary key default value has casing mismatch with interface.
The
BaseEntityinterface now uses PascalCase (Timestamp,Rfid), but the constructor's defaultprimaryKeyvalue on line 18 is still lowercase"timestamp". This mismatch could cause issues if a subclass relies on the default value.🐛 Proposed fix
constructor( protected repository: Repository<T>, - protected primaryKey: keyof T = "timestamp" as keyof T, + protected primaryKey: keyof T = "Timestamp" as keyof T, ) {}packages/server/src/controllers/BackendController/BackendController.ts (1)
59-68:⚠️ Potential issue | 🟠 MajorMissing
awaiton async database insert operation.
insertPacketDatareturns aPromise<GenericResponse>but is not awaited. This fire-and-forget pattern means:
- Database errors will be silently swallowed
- Under high packet rates, unresolved promises could accumulate
- No guarantee data is persisted before broadcasting to frontend
Consider awaiting the insert and handling errors appropriately, or at minimum adding a
.catch()handler.🐛 Proposed fix - Option 1: Await with error handling
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); + } // Broadcast the packet to the frontend this.socketIO.broadcastPacket(message);🐛 Proposed fix - Option 2: Fire-and-forget with error logging
public async handlePacketReceive(message: ITelemetryData) { // Insert the packet into the database - this.timescaleDB.insertPacketData(message); + this.timescaleDB.insertPacketData(message).catch((error) => { + logger.error("Failed to insert packet data:", error); + }); // Broadcast the packet to the frontend this.socketIO.broadcastPacket(message);
🤖 Fix all issues with AI agents
In `@packages/amplify/amplify/backend.ts`:
- Around line 55-59: The stack references a pre-existing Secrets Manager secret
via secretsmanager.Secret.fromSecretNameV2(TelemetryBackendStack,
"TimescaleConnectionString", "TimescaleConnectionString"), so create a Secrets
Manager secret named exactly "TimescaleConnectionString" before deployment and
populate it with JSON keys DATABASE_HOST, DATABASE_PASSWORD, DATABASE_PORT, and
DATABASE_USERNAME (with appropriate values); verify the secret name matches the
string passed to fromSecretNameV2 and that the JSON structure is correct so the
ECS task can read each key at runtime.
In `@packages/db/src/data-source.ts`:
- Around line 38-42: The production DB config currently spreads ...(isProd && {
ssl: { rejectUnauthorized: false } }) which disables certificate validation;
update the isProd SSL branch to set rejectUnauthorized: true and load/attach CA
certificates (e.g., from an environment-provided PEM) into the ssl.ca option
instead of turning validation off; ensure the code that builds the data-source
config (the isProd/ssl block) reads a CA env var or file and assigns ssl.ca when
present and falls back to proper system CA validation.
- Around line 12-19: The current env-validation block incorrectly requires
DATABASE_PORT even though later code falls back to 5432; remove DATABASE_PORT
from the required-check so the throw condition only tests DATABASE_HOST,
DATABASE_USERNAME, and DATABASE_PASSWORD, keeping the fallback to 5432 intact
(update the if conditional that throws the Error and retain the existing
fallback logic that reads process.env.DATABASE_PORT || "5432").
In `@packages/db/src/services/DatabaseService.ts`:
- Around line 140-150: getDrivers currently swallows errors and can return
undefined; add the same isConnected check used in other DatabaseService methods
at the start of getDrivers (call the isConnected guard and throw or return an
empty array if not connected), and update the catch block to either rethrow the
caught error or return an empty array so callers never receive undefined;
specifically, ensure the method around driverRepo.find uses try/catch to log the
error and then return [] (or throw) and include the isConnected check before
calling driverRepo.find to match the service's behavior.
- Around line 152-183: Add the same database connection guard used in
getPacketData/insertPacketData to both getDriverNameUsingRfid and getDriverLaps:
at the start of each method check this.isConnected (or the class's connection
boolean) and throw or return a clear error if not connected, so these methods
fail consistently when DB isn’t initialized; update getDriverNameUsingRfid and
getDriverLaps to perform that check before calling this.driverRepo/find or
this.lapRepo.find and keep existing try/catch behavior afterward.
- Around line 16-34: The constructor of DatabaseService currently calls
AppDataSource.getRepository(...) (telemetryPacketRepo, driverRepo, lapRepo)
before the DataSource is initialized; change to lazy initialize
repositories—remove repository calls from the constructor and instead obtain
them after AppDataSource.initialize() (e.g., inside initialize() or via private
getter helpers that call AppDataSource.getRepository(...) on first access).
Ensure DatabaseService.initialize() sets isConnected and then assigns
telemetryPacketRepo, driverRepo, and lapRepo by calling
AppDataSource.getRepository for TelemetryPacket, Driver, and Lap so repository
access never happens before AppDataSource.initialize().
- Around line 349-354: The Timestamp being saved via lapRepo.save (Timestamp) is
using timestamp ?? new Date().getTime() (a number) — convert the incoming
timestamp into a Date object (e.g., new Date(timestamp) or new Date()) so
Timestamp is a Date; also stop defaulting Rfid to the string "unknown driver"
which masks bad input — either require/validate rfid and throw or persist
null/undefined instead (e.g., Rfid: rfid ?? null) and let upstream validation
handle missing RFIDs; update the object passed to lapRepo.save (fields: Rfid,
Timestamp, Type: "gps-lap") accordingly.
In `@packages/server/.env.example`:
- Line 23: Remove the unnecessary surrounding quotes from the environment value
for LAP_POSITION_PASSWORD in the .env example (change
LAP_POSITION_PASSWORD="changeme" to an unquoted LAP_POSITION_PASSWORD=changeme)
so dotenv-linter's QuoteCharacter warning is resolved; alternatively, if quoting
is required for some reason, update the dotenv-linter config to allow quoted
values for that variable name or pattern instead of leaving the quoted example
value.
In `@packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts`:
- Around line 72-78: The try/catch in publishLapData won't catch MQTT publish
failures because MQTT.js reports errors via the publish callback; update
publishLapData to call this.client.publish(lapdataTopic,
JSON.stringify(lapdata), callback) and handle the callback error (log via
logger.error with context and error details) or wrap the callback in a Promise
and await it so callers can observe failures; reference the publishLapData
method, this.client.publish, lapdataTopic, and logger.error when making the
change.
🧹 Nitpick comments (7)
packages/db/src/repositories/BaseRepository.ts (1)
40-47: Minor typo in comment."entitiy" should be "entity".
✏️ Proposed fix
- // no nested updates possible with current any entitiy so Partial<T> is okay + // no nested updates possible with current entity so Partial<T> is okaypackages/db/src/entities/Lap.entity.ts (1)
4-9: Outdated comment references hypertable but decorator was removed.The docstring still mentions "Lap data hypertable" but the
@Hypertabledecorator has been removed (unlikeTelemetryPacketwhich retains it). Either update the comment to reflect it's now a regular table, or verify if the hypertable configuration should be restored.✏️ Proposed fix if intentionally a regular table
/** - * Lap data hypertable + * Lap data table * Stores computed lap metrics for each lap */packages/db/src/data-source.ts (1)
23-37: Consider parameterizing the database name.The database name
"tsdb"is hardcoded. For flexibility across environments (dev, staging, prod), consider making it configurable via environment variable.♻️ Proposed enhancement
export const AppDataSource = new DataSource({ - database: "tsdb", + database: process.env.DATABASE_NAME || "tsdb", entities: [TelemetryPacket, Driver, Lap],packages/db/src/interfaces/repositories.interface.ts (1)
8-16: IBaseRepository assumes timestamp-based identification, but not all entities use timestamp as primary key.The
findById,update, anddeletemethods all usetimestamp: Dateas the identifier. However,IDriverRepositoryextends this interface, andDriverentities are identified byRfid(as seen inDatabaseService.getDriverNameUsingRfid). This creates a contract mismatch.Consider either:
- Making
IBaseRepositorygeneric over the identifier type- Defining entity-specific methods in each repository interface
Option 1: Generic identifier type
-export interface IBaseRepository<T> { +export interface IBaseRepository<T, ID = Date> { create(entity: DeepPartial<T>): Promise<T>; - findById(timestamp: Date): Promise<T | null>; + findById(id: ID): Promise<T | null>; findOne(options: FindOneOptions<T>): Promise<T | null>; findMany(options?: FindManyOptions<T>): Promise<T[]>; - update(timestamp: Date, updates: DeepPartial<T>): Promise<T | null>; - delete(timestamp: Date): Promise<boolean>; + update(id: ID, updates: DeepPartial<T>): Promise<T | null>; + delete(id: ID): Promise<boolean>; count(options?: FindManyOptions<T>): Promise<number>; } -export interface IDriverRepository extends IBaseRepository<Driver> {} +export interface IDriverRepository extends IBaseRepository<Driver, string> {}packages/db/src/services/DatabaseService.ts (3)
187-189: Redundant runtime type check for TypeScript-typed parameter.The
typeof Rfid !== "string"check is unnecessary sinceRfidis already typed asstringin the function signature. TypeScript enforces this at compile time.Remove redundant check
public async updateDriverInfo(Rfid: string, name: string) { + if (!this.isConnected) { + throw new Error("Database not connected"); + } try { - if (typeof Rfid !== "string") { - throw new Error("Rfid must be a string"); - } - const existingDriver = await this.driverRepo.findOne({
148-148: Replace console statements with a logging framework.Multiple
console.errorandconsole.logstatements are flagged by ESLint. For production code, consider using a structured logging library (e.g.,winston,pino) that supports log levels, formatting, and output configuration.If console logging is intentional for this service, you may want to disable the ESLint rule selectively or configure an allowed logging utility.
Also applies to: 159-159, 165-165, 178-178, 207-207, 215-215
287-294: Consider using aggregate queries for better performance.Two separate queries with
ORDER BYandLIMIT 1are less efficient than usingMIN/MAXaggregates, especially on large TimescaleDB hypertables.Optimized approach using query builder
const result = await this.telemetryPacketRepo .createQueryBuilder("packet") .select("MIN(packet.Timestamp)", "firstDate") .addSelect("MAX(packet.Timestamp)", "lastDate") .getRawOne(); return { firstDateUTC: result?.firstDate ? new Date(result.firstDate).getTime() : null, lastDateUTC: result?.lastDate ? new Date(result.lastDate).getTime() : null, };
| const TimescaleConnectionString = secretsmanager.Secret.fromSecretNameV2( | ||
| TelemetryBackendStack, | ||
| "HeliosTelemetryDBCredentials", | ||
| { | ||
| secretName: "HeliosTelemetryDBCredentials" + backend.stack.stackName, | ||
| secretObjectValue: { | ||
| POSTGRES_PASSWORD: cdk.SecretValue.unsafePlainText(""), | ||
| POSTGRES_USERNAME: cdk.SecretValue.unsafePlainText(""), | ||
| }, | ||
| }, | ||
| "TimescaleConnectionString", | ||
| "TimescaleConnectionString", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure to understand the repository layout
git ls-files | head -20Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 443
🏁 Script executed:
# Search for references to the TimescaleConnectionString secret
rg "TimescaleConnectionString" -A 5 -B 2Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 2747
🏁 Script executed:
# Search for the specific database environment variable keys mentioned
rg "DATABASE_HOST|DATABASE_PASSWORD|DATABASE_PORT|DATABASE_USERNAME" -A 2 -B 2Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 2760
🏁 Script executed:
# Look for any setup or deployment documentation
fd -e md -e txt | xargs grep -l -i "secret\|timescale\|database" 2>/dev/null | head -10Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 168
🏁 Script executed:
# Check the ECS task definition around lines 354-369
sed -n '350,375p' packages/amplify/amplify/backend.tsRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 956
🏁 Script executed:
# Search for any AWS secrets configuration or deployment scripts
fd -e sh -e py -e ts -e js | xargs grep -l "Secrets Manager\|secrets\|DATABASE" 2>/dev/null | grep -E "(setup|deploy|init|config)" | head -5Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 57
🏁 Script executed:
# Check the AMPLIFY documentation for secret setup instructions
cat docs/AMPLIFY.mdRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 7620
🏁 Script executed:
# Check the database package documentation
cat packages/db/README.mdRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 243
🏁 Script executed:
# Check for any other deployment or setup documentation
cat docs/SERVER.md | head -100Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 5131
🏁 Script executed:
# Search for any CloudFormation templates or IaC that might provision the secret
find . -name "*.yml" -o -name "*.yaml" -o -name "*.json" | grep -E "(cloudformation|template|infrastructure|secrets)" | head -10Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 57
🏁 Script executed:
# Check if there's any reference to secret provisioning in the codebase
rg "TimescaleConnectionString" -B 5 -A 5 --type-add 'config:*.yml' --type config --type-add 'template:*.json' --type templateRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 57
🏁 Script executed:
# Look for any environment setup or deployment instructions in README files
find . -maxdepth 2 -name "README*" -type f | xargs cat 2>/dev/nullRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 6068
Ensure the TimescaleConnectionString secret is pre-provisioned in AWS Secrets Manager before deployment.
The code references a TimescaleConnectionString secret and expects it to contain JSON keys: DATABASE_HOST, DATABASE_PASSWORD, DATABASE_PORT, and DATABASE_USERNAME. The ECS task will fail to start if this secret does not exist or if these keys are missing. Create this secret in Secrets Manager with the correct structure before deploying.
🤖 Prompt for AI Agents
In `@packages/amplify/amplify/backend.ts` around lines 55 - 59, The stack
references a pre-existing Secrets Manager secret via
secretsmanager.Secret.fromSecretNameV2(TelemetryBackendStack,
"TimescaleConnectionString", "TimescaleConnectionString"), so create a Secrets
Manager secret named exactly "TimescaleConnectionString" before deployment and
populate it with JSON keys DATABASE_HOST, DATABASE_PASSWORD, DATABASE_PORT, and
DATABASE_USERNAME (with appropriate values); verify the secret name matches the
string passed to fromSecretNameV2 and that the JSON structure is correct so the
ECS task can read each key at runtime.
| if ( | ||
| !process.env.DATABASE_HOST || | ||
| !process.env.DATABASE_PORT || | ||
| !process.env.DATABASE_USERNAME || | ||
| !process.env.DATABASE_PASSWORD | ||
| ) { | ||
| throw new Error("Database configuration environment variables are not set."); | ||
| } |
There was a problem hiding this comment.
DATABASE_PORT validation is inconsistent with fallback logic.
The validation throws if DATABASE_PORT is not set, but lines 31-33 provide a fallback to 5432. Either remove DATABASE_PORT from the required validation (since there's a default), or remove the fallback to enforce the requirement.
🐛 Option 1: Remove from required validation (allow default)
if (
!process.env.DATABASE_HOST ||
- !process.env.DATABASE_PORT ||
!process.env.DATABASE_USERNAME ||
!process.env.DATABASE_PASSWORD
) {
throw new Error("Database configuration environment variables are not set.");
}🐛 Option 2: Remove fallback (enforce requirement)
- port: process.env.DATABASE_PORT
- ? parseInt(process.env.DATABASE_PORT, 10)
- : 5432,
+ port: parseInt(process.env.DATABASE_PORT, 10),📝 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.
| if ( | |
| !process.env.DATABASE_HOST || | |
| !process.env.DATABASE_PORT || | |
| !process.env.DATABASE_USERNAME || | |
| !process.env.DATABASE_PASSWORD | |
| ) { | |
| throw new Error("Database configuration environment variables are not set."); | |
| } | |
| if ( | |
| !process.env.DATABASE_HOST || | |
| !process.env.DATABASE_USERNAME || | |
| !process.env.DATABASE_PASSWORD | |
| ) { | |
| throw new Error("Database configuration environment variables are not set."); | |
| } |
🤖 Prompt for AI Agents
In `@packages/db/src/data-source.ts` around lines 12 - 19, The current
env-validation block incorrectly requires DATABASE_PORT even though later code
falls back to 5432; remove DATABASE_PORT from the required-check so the throw
condition only tests DATABASE_HOST, DATABASE_USERNAME, and DATABASE_PASSWORD,
keeping the fallback to 5432 intact (update the if conditional that throws the
Error and retain the existing fallback logic that reads
process.env.DATABASE_PORT || "5432").
| ...(isProd && { | ||
| ssl: { | ||
| rejectUnauthorized: false, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Production SSL with rejectUnauthorized: false is insecure.
Setting rejectUnauthorized: false disables certificate validation, making the connection vulnerable to man-in-the-middle attacks. For production, consider:
- Setting to
truewith proper CA certificates - Using
caoption if using self-signed certs
🛡️ Proposed fix
...(isProd && {
ssl: {
- rejectUnauthorized: false,
+ rejectUnauthorized: true,
+ // If using self-signed certs, add: ca: process.env.DATABASE_CA_CERT
},
}),📝 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.
| ...(isProd && { | |
| ssl: { | |
| rejectUnauthorized: false, | |
| }, | |
| }), | |
| ...(isProd && { | |
| ssl: { | |
| rejectUnauthorized: true, | |
| // If using self-signed certs, add: ca: process.env.DATABASE_CA_CERT | |
| }, | |
| }), |
🤖 Prompt for AI Agents
In `@packages/db/src/data-source.ts` around lines 38 - 42, The production DB
config currently spreads ...(isProd && { ssl: { rejectUnauthorized: false } })
which disables certificate validation; update the isProd SSL branch to set
rejectUnauthorized: true and load/attach CA certificates (e.g., from an
environment-provided PEM) into the ssl.ca option instead of turning validation
off; ensure the code that builds the data-source config (the isProd/ssl block)
reads a CA env var or file and assigns ssl.ca when present and falls back to
proper system CA validation.
| constructor() { | ||
| this.dataSource = AppDataSource; | ||
| this.telemetryPacketRepo = AppDataSource.getRepository(TelemetryPacket); | ||
| this.driverRepo = AppDataSource.getRepository(Driver); | ||
| this.lapRepo = AppDataSource.getRepository(Lap); | ||
| } | ||
|
|
||
| async initialize(): Promise<void> { | ||
| async initialize() { | ||
| if (!this.isConnected) { | ||
| await AppDataSource.initialize(); | ||
| this.isConnected = true; | ||
| } | ||
| } | ||
|
|
||
| static getInstance(): DatabaseService { | ||
| if (!DatabaseService.instance) { | ||
| DatabaseService.instance = new DatabaseService(); | ||
| } | ||
| return DatabaseService.instance; | ||
| } |
There was a problem hiding this comment.
Repository access in constructor will fail if AppDataSource is not initialized.
The constructor calls AppDataSource.getRepository() immediately, but AppDataSource.initialize() is only called later in the initialize() method. TypeORM requires the DataSource to be initialized before accessing repositories.
This will throw an error like: "Cannot access repository before data source is initialized".
Proposed fix: Lazy repository initialization
export class DatabaseService {
private isConnected = false;
private static instance: DatabaseService;
- private telemetryPacketRepo: Repository<TelemetryPacket>;
- private driverRepo: Repository<Driver>;
- private lapRepo: Repository<Lap>;
+ private telemetryPacketRepo!: Repository<TelemetryPacket>;
+ private driverRepo!: Repository<Driver>;
+ private lapRepo!: Repository<Lap>;
- constructor() {
- this.telemetryPacketRepo = AppDataSource.getRepository(TelemetryPacket);
- this.driverRepo = AppDataSource.getRepository(Driver);
- this.lapRepo = AppDataSource.getRepository(Lap);
- }
+ constructor() {}
async initialize() {
if (!this.isConnected) {
await AppDataSource.initialize();
+ this.telemetryPacketRepo = AppDataSource.getRepository(TelemetryPacket);
+ this.driverRepo = AppDataSource.getRepository(Driver);
+ this.lapRepo = AppDataSource.getRepository(Lap);
this.isConnected = true;
}
}🤖 Prompt for AI Agents
In `@packages/db/src/services/DatabaseService.ts` around lines 16 - 34, The
constructor of DatabaseService currently calls AppDataSource.getRepository(...)
(telemetryPacketRepo, driverRepo, lapRepo) before the DataSource is initialized;
change to lazy initialize repositories—remove repository calls from the
constructor and instead obtain them after AppDataSource.initialize() (e.g.,
inside initialize() or via private getter helpers that call
AppDataSource.getRepository(...) on first access). Ensure
DatabaseService.initialize() sets isConnected and then assigns
telemetryPacketRepo, driverRepo, and lapRepo by calling
AppDataSource.getRepository for TelemetryPacket, Driver, and Lap so repository
access never happens before AppDataSource.initialize().
| public async getDrivers() { | ||
| try { | ||
| if (!this.dataSource.isInitialized) { | ||
| await this.dataSource.initialize(); | ||
| console.log("Database connection initialized successfully"); | ||
| const drivers = await this.driverRepo.find(); | ||
| return drivers.map((driver) => ({ | ||
| Rfid: driver.Rfid, | ||
| driver: driver.Name, | ||
| })); | ||
| } catch (error: unknown) { | ||
| console.error("Error getting drivers"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Method returns undefined on error instead of throwing or returning an empty array.
The catch block logs an error but doesn't return a value, causing the method to implicitly return undefined. Callers iterating over the result (e.g., getDrivers().map(...)) will fail.
Also missing isConnected check unlike other methods.
Proposed fix
public async getDrivers() {
+ if (!this.isConnected) {
+ throw new Error("Database not connected");
+ }
+
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");
+ throw new Error("Failed to get drivers: " + (error as Error).message);
}
}🧰 Tools
🪛 ESLint
[error] 148-148: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
In `@packages/db/src/services/DatabaseService.ts` around lines 140 - 150,
getDrivers currently swallows errors and can return undefined; add the same
isConnected check used in other DatabaseService methods at the start of
getDrivers (call the isConnected guard and throw or return an empty array if not
connected), and update the catch block to either rethrow the caught error or
return an empty array so callers never receive undefined; specifically, ensure
the method around driverRepo.find uses try/catch to log the error and then
return [] (or throw) and include the isConnected check before calling
driverRepo.find to match the service's behavior.
| public async getDriverNameUsingRfid(Rfid: string) { | ||
| try { | ||
| const driver = await this.driverRepo.findOne({ | ||
| where: { Rfid }, | ||
| }); | ||
|
|
||
| if (!driver) { | ||
| console.error(`No driver found for Rfid: ${Rfid}`); | ||
| return "Driver not found"; | ||
| } | ||
|
|
||
| return driver.Name; | ||
| } catch (error: unknown) { | ||
| console.error("Error getting driver name using the given Rfid"); | ||
| throw new Error((error as Error).message); | ||
| } | ||
| } | ||
|
|
||
| public async getDriverLaps(Rfid: string) { | ||
| try { | ||
| const laps = await this.lapRepo.find({ | ||
| order: { Timestamp: "DESC" }, | ||
| where: { Rfid }, | ||
| }); | ||
| return laps; | ||
| } catch (error: unknown) { | ||
| console.error("Error getting lap data for driver", error); | ||
| throw new Error( | ||
| (error as Error).message || "Failed to fetch driver laps", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing isConnected checks in driver-related methods.
getDriverNameUsingRfid and getDriverLaps lack the isConnected guard that other methods (e.g., getPacketData, insertPacketData) have. This inconsistency could lead to confusing errors when the database isn't initialized.
Add consistent guards
public async getDriverNameUsingRfid(Rfid: string) {
+ if (!this.isConnected) {
+ throw new Error("Database not connected");
+ }
try {
// ...
public async getDriverLaps(Rfid: string) {
+ if (!this.isConnected) {
+ throw new Error("Database not connected");
+ }
try {
// ...📝 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.
| public async getDriverNameUsingRfid(Rfid: string) { | |
| try { | |
| const driver = await this.driverRepo.findOne({ | |
| where: { Rfid }, | |
| }); | |
| if (!driver) { | |
| console.error(`No driver found for Rfid: ${Rfid}`); | |
| return "Driver not found"; | |
| } | |
| return driver.Name; | |
| } catch (error: unknown) { | |
| console.error("Error getting driver name using the given Rfid"); | |
| throw new Error((error as Error).message); | |
| } | |
| } | |
| public async getDriverLaps(Rfid: string) { | |
| try { | |
| const laps = await this.lapRepo.find({ | |
| order: { Timestamp: "DESC" }, | |
| where: { Rfid }, | |
| }); | |
| return laps; | |
| } catch (error: unknown) { | |
| console.error("Error getting lap data for driver", error); | |
| throw new Error( | |
| (error as Error).message || "Failed to fetch driver laps", | |
| ); | |
| } | |
| } | |
| public async getDriverNameUsingRfid(Rfid: string) { | |
| if (!this.isConnected) { | |
| throw new Error("Database not connected"); | |
| } | |
| try { | |
| const driver = await this.driverRepo.findOne({ | |
| where: { Rfid }, | |
| }); | |
| if (!driver) { | |
| console.error(`No driver found for Rfid: ${Rfid}`); | |
| return "Driver not found"; | |
| } | |
| return driver.Name; | |
| } catch (error: unknown) { | |
| console.error("Error getting driver name using the given Rfid"); | |
| throw new Error((error as Error).message); | |
| } | |
| } | |
| public async getDriverLaps(Rfid: string) { | |
| if (!this.isConnected) { | |
| throw new Error("Database not connected"); | |
| } | |
| try { | |
| const laps = await this.lapRepo.find({ | |
| order: { Timestamp: "DESC" }, | |
| where: { Rfid }, | |
| }); | |
| return laps; | |
| } catch (error: unknown) { | |
| console.error("Error getting lap data for driver", error); | |
| throw new Error( | |
| (error as Error).message || "Failed to fetch driver laps", | |
| ); | |
| } | |
| } |
🧰 Tools
🪛 ESLint
[error] 159-159: Unexpected console statement.
(no-console)
[error] 165-165: Unexpected console statement.
(no-console)
[error] 178-178: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
In `@packages/db/src/services/DatabaseService.ts` around lines 152 - 183, Add the
same database connection guard used in getPacketData/insertPacketData to both
getDriverNameUsingRfid and getDriverLaps: at the start of each method check
this.isConnected (or the class's connection boolean) and throw or return a clear
error if not connected, so these methods fail consistently when DB isn’t
initialized; update getDriverNameUsingRfid and getDriverLaps to perform that
check before calling this.driverRepo/find or this.lapRepo.find and keep existing
try/catch behavior afterward.
| try { | ||
| await this.lapRepo.save({ | ||
| Rfid: rfid ?? "unknown driver", | ||
| Timestamp: timestamp ?? new Date().getTime(), | ||
| Type: "gps-lap", | ||
| }); |
There was a problem hiding this comment.
Type mismatch: Timestamp should be a Date, not a number.
Line 352 assigns timestamp ?? new Date().getTime() (a number) to Timestamp, but the Lap entity likely expects a Date type based on patterns elsewhere in this file (e.g., new Date(timestamp) on line 227).
Additionally, falling back to "unknown driver" silently masks invalid input data.
Proposed fix
public async insertIntoGpsLapCountTable(
rfid: string,
timestamp: number,
): Promise<GenericResponse> {
if (!this.isConnected) {
throw new Error("Database not connected");
}
+ if (!rfid) {
+ throw new Error("rfid is required");
+ }
+
try {
await this.lapRepo.save({
- Rfid: rfid ?? "unknown driver",
- Timestamp: timestamp ?? new Date().getTime(),
+ Rfid: rfid,
+ Timestamp: new Date(timestamp),
Type: "gps-lap",
});🤖 Prompt for AI Agents
In `@packages/db/src/services/DatabaseService.ts` around lines 349 - 354, The
Timestamp being saved via lapRepo.save (Timestamp) is using timestamp ?? new
Date().getTime() (a number) — convert the incoming timestamp into a Date object
(e.g., new Date(timestamp) or new Date()) so Timestamp is a Date; also stop
defaulting Rfid to the string "unknown driver" which masks bad input — either
require/validate rfid and throw or persist null/undefined instead (e.g., Rfid:
rfid ?? null) and let upstream validation handle missing RFIDs; update the
object passed to lapRepo.save (fields: Rfid, Timestamp, Type: "gps-lap")
accordingly.
| # App secrets / other # | ||
| ######################## | ||
| LAP_POSITION_PASSWORD="changeme" | ||
| LAP_POSITION_PASSWORD="changeme" |
There was a problem hiding this comment.
Dotenv-linter QuoteCharacter warning on quoted value.
If dotenv-linter runs in CI, quoted values can fail lint; consider removing quotes or adjusting linter config.
💡 Suggested change (if you want to satisfy dotenv-linter)
-LAP_POSITION_PASSWORD="changeme"
+LAP_POSITION_PASSWORD=changeme📝 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.
| LAP_POSITION_PASSWORD="changeme" | |
| LAP_POSITION_PASSWORD=changeme |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 23-23: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
In `@packages/server/.env.example` at line 23, Remove the unnecessary surrounding
quotes from the environment value for LAP_POSITION_PASSWORD in the .env example
(change LAP_POSITION_PASSWORD="changeme" to an unquoted
LAP_POSITION_PASSWORD=changeme) so dotenv-linter's QuoteCharacter warning is
resolved; alternatively, if quoting is required for some reason, update the
dotenv-linter config to allow quoted values for that variable name or pattern
instead of leaving the quoted example value.
| public async publishLapData(lapdata: ILapData) { | ||
| try { | ||
| this.client.publish(lapdataTopic, JSON.stringify(lapdata)); | ||
| } catch (error) { | ||
| logger.error("Error publishing lap data: ", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MQTT v5 client.publish signature error handling callback
💡 Result:
It depends on the client library you’re using—MQTT v5 adds reason codes / properties, but each library exposes them differently.
Node.js (MQTT.js)
Signature
client.publish(topic, message, options, callback)Callback
function (err, packet) { ... }The callback fires when QoS handling completes (or next tick for QoS 0). Handle errors via err; the packet lets you inspect the ack flow (useful for MQTT v5). You can also use publishAsync() which returns a Promise. [1]
Python (Eclipse Paho paho-mqtt)
Publish completion is handled via on_publish, not an inline publish(..., callback=...).
on_publish signatures
- Callback API v2 (includes MQTT v5 info):
on_publish(client, userdata, mid, reason_code, properties)- Callback API v1:
on_publish(client, userdata, mid)Use reason_code / properties (API v2) for MQTT v5-level error reporting; otherwise you only get mid. [2]
Java (Eclipse Paho)
Use the async publish overload that takes an IMqttActionListener:
publish(topic, payload, qos, retained, userContext, callback)Error handling is via:
callback.onSuccess(IMqttToken token)callback.onFailure(IMqttToken token, Throwable exception)[3]
You can also track completion via MqttCallback.deliveryComplete(IMqttDeliveryToken) instead of per-call listeners. [3]
Sources: [1] MQTT.js README (publish callback signature) • [2] Eclipse Paho Python docs (on_publish signatures incl. v5 reason_code/properties) • [3] Eclipse Paho Java docs (publish(..., IMqttActionListener) and failure callback)
🏁 Script executed:
# Find MQTT library configuration and imports
find . -name "package.json" -type f | head -5 | xargs -I {} grep -l mqtt "{}" 2>/dev/nullRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 59
🏁 Script executed:
# Look at the imports in the SolarMQTTClient file to identify the MQTT library
head -30 packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.tsRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 869
🏁 Script executed:
cd packages/server && cat package.json | grep -A 2 -B 2 '"mqtt"'Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 201
🏁 Script executed:
# Also check if publishAsync is mentioned anywhere in the codebase
rg "publishAsync" packages/server/Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 57
Publish error handling won't trigger as written.
client.publish() (MQTT.js v5.8.0) reports errors via callback, not synchronous exceptions. The current try/catch will never catch publish failures, making message drops difficult to diagnose.
💡 Suggested fix using publish callback
- public async publishLapData(lapdata: ILapData) {
- try {
- this.client.publish(lapdataTopic, JSON.stringify(lapdata));
- } catch (error) {
- logger.error("Error publishing lap data: ", error);
- }
- }
+ public publishLapData(lapdata: ILapData): Promise<void> {
+ return new Promise((resolve, reject) => {
+ this.client.publish(lapdataTopic, JSON.stringify(lapdata), (err) => {
+ if (err) {
+ logger.error("Error publishing lap data: ", err);
+ reject(err);
+ } else {
+ resolve();
+ }
+ });
+ });
+ }📝 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.
| public async publishLapData(lapdata: ILapData) { | |
| try { | |
| this.client.publish(lapdataTopic, JSON.stringify(lapdata)); | |
| } catch (error) { | |
| logger.error("Error publishing lap data: ", error); | |
| } | |
| } | |
| public publishLapData(lapdata: ILapData): Promise<void> { | |
| return new Promise((resolve, reject) => { | |
| this.client.publish(lapdataTopic, JSON.stringify(lapdata), (err) => { | |
| if (err) { | |
| logger.error("Error publishing lap data: ", err); | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts` around
lines 72 - 78, The try/catch in publishLapData won't catch MQTT publish failures
because MQTT.js reports errors via the publish callback; update publishLapData
to call this.client.publish(lapdataTopic, JSON.stringify(lapdata), callback) and
handle the callback error (log via logger.error with context and error details)
or wrap the callback in a Promise and await it so callers can observe failures;
reference the publishLapData method, this.client.publish, lapdataTopic, and
logger.error when making the change.
alexwhelan12
force-pushed
the
update-backend-controllers-with-timescale
branch
from
af8e7fd to
bacd818
Compare
4 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.