[TEL-313] Establish WebSocket connection between Grafana and backend #265
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a native WebSocket broadcaster and types, wires it into BackendController and LapController to emit telemetry and lap data to Grafana, updates server dependencies (remove sqlite*, add ws/@types/ws and rimraf), adds Grafana docs and a server .env example with GRAFANA_WS_PATH. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Telemetry Source
participant Backend as BackendController
participant LapCtrl as LapController
participant WS as NativeWebSocket
participant Grafana as Grafana Client
Source->>Backend: send packet
Backend->>Backend: handlePacketReceive
Backend->>WS: (optional) broadcastPacket(packet)
Source->>LapCtrl: stream packets
LapCtrl->>LapCtrl: detect lap (lastLapPackets.length > 5)
LapCtrl->>WS: broadcastLapData(lapData)
WS-->>Grafana: {type:"lapData", data:ILapData}
LapCtrl->>Backend: persist lap
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
packages/server/src/controllers/BackendController/BackendController.types.ts (1)
4-19: Consider depending on the GrafanaWebSocket interface type instead of the concrete classRight now
BackendControllerTypes.grafanaWebSocketis typed as the concreteGrafanaWebSocketclass. If you want easier testing or alternative implementations later, you might instead type this asGrafanaWebSocketType(fromGrafanaWebSocket.types.ts) and keep the class detail local toBackendController.packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.types.ts (1)
1-9: Interface matches implementation; consider whetherwssshould remain part of the public surface
GrafanaWebSocketTypecleanly describes the two broadcast methods and underlyingWebSocketServer. If consumers don’t need direct access towss, you could drop that property (or mark it readonly) to decouple callers fromwsinternals and keep the abstraction narrower.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/server/package.json(2 hunks)packages/server/src/controllers/BackendController/BackendController.ts(3 hunks)packages/server/src/controllers/BackendController/BackendController.types.ts(2 hunks)packages/server/src/controllers/LapController/LapController.ts(1 hunks)packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts(1 hunks)packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.types.ts (1)
packages/shared/src/types.ts (2)
ILapData(421-437)ITelemetryData(399-399)
packages/server/src/controllers/BackendController/BackendController.ts (2)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (1)
GrafanaWebSocket(14-81)packages/server/src/datasources/SocketIO/SocketIO.ts (1)
SocketIO(20-86)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (4)
packages/server/src/utils/logger.ts (1)
createLightweightApplicationLogger(98-102)packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.types.ts (1)
GrafanaWebSocketType(5-9)packages/server/src/controllers/BackendController/BackendController.ts (1)
BackendController(16-73)packages/shared/src/types.ts (2)
ITelemetryData(399-399)ILapData(421-437)
packages/server/src/controllers/BackendController/BackendController.types.ts (1)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (1)
GrafanaWebSocket(14-81)
🔇 Additional comments (7)
packages/server/src/controllers/LapController/LapController.ts (2)
96-98: Verify BackendController initializes grafanaWebSocket property.The constructor injects
backendControllerand stores it as an instance variable. Ensure thatBackendControllerproperly initializes thegrafanaWebSocketproperty during construction or before this method is invoked. IfgrafanaWebSocketis lazy-initialized or conditionally created, this could lead to runtime errors.
159-163: Remove unnecessaryawaitand reconsider the review comment's premise.The original review comment's core concern is invalid. The
broadcastLapDatamethod returnsvoid, not a Promise, so it cannot reject or throw an unhandled error. Theawaitat line 161 is semantically unnecessary but harmless.Additionally, there is no null safety concern—
grafanaWebSocketis properly initialized in BackendController's constructor at line 28.Minor issue: Line 161 uses
awaiton a void-returning function. This should be removed:public async handleLapData(lapData: ILapData) { await this.backendController.socketIO.broadcastLapData(lapData); - await this.backendController.grafanaWebSocket.broadcastLapData(lapData); + this.backendController.grafanaWebSocket.broadcastLapData(lapData); await this.backendController.dynamoDB.insertLapData(lapData); }Likely an incorrect or invalid review comment.
packages/server/package.json (1)
19-35: ws / @types/ws dependency additions look appropriate; confirm runtime environment compatibilityThe new
wsand@types/wsentries match howGrafanaWebSocketis imported and typed elsewhere. Just ensure the deployed Node.js version is compatible withws@^8.18.3and that your lockfile is updated so CI/prod run the same versions.Please double‑check the
wscompatibility matrix / release notes for your Node version and update the lockfile (package-lock.json/pnpm-lock.yaml/yarn.lock) accordingly if not already done.packages/server/src/controllers/BackendController/BackendController.ts (2)
7-31: GrafanaWebSocket wiring into BackendController looks soundImporting
GrafanaWebSocket, storing it as a public field, and instantiating it alongsideSocketIOandSolarMQTTClientkeeps all datasources consistently managed byBackendController. PassinghttpsServerandthismirrors the existing Socket.IO pattern and avoids runtime cycles because the reverse dependency is type‑only.
54-58: Extending packet broadcast to Grafana WebSocket aligns with existing flowForwarding every telemetry packet to
grafanaWebSocket.broadcastPacketright after the Socket.IO broadcast keeps Grafana clients in lockstep with the frontend without altering database or lap‑handling logic. The change is minimal and keepshandlePacketReceiveresponsibilities clear.packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (2)
1-39: Connection setup and logging for Grafana WebSocket are consistent with existing patternsBinding
WebSocketServerto the shared HTTP(S) server on/grafana-ws, logging the client’s remote address, registering per‑socket listeners, and sending an initial JSON “connection” message all mirror typicalwsusage and fit well with the existing datasource style (e.g.,SocketIO).
72-80: Per-socket close/error listeners are wired correctlyThe
initializeWebSocketListenershelper cleanly centralizes close and error handling with appropriate logging, matching the style used elsewhere in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)docs/GRAFANA.md(1 hunks)packages/server/package.json(2 hunks)packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/GRAFANA.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
docs/GRAFANA.md (1)
75-89: WebSocket setup section is clear and well-documented.The quick setup instructions correctly reference the
GRAFANA_WS_PATHenvironment variable and guide users through the Grafana UI steps. The default path/grafana-wsaligns with typical WebSocket datasource conventions. Instructions are actionable and match the integration approach described in the PR objectives.packages/server/package.json (2)
31-31: Dependency placement is correct."ws" correctly belongs in dependencies (runtime requirement) and "@types/ws" correctly in devDependencies (TypeScript types only). This aligns with the GrafanaWebSocket integration described in the PR objectives.
Also applies to: 40-40
31-31: I have confirmation that [email protected] has no known critical security vulnerabilities. Now I need to verify @types/[email protected] and confirm Node 18+ compatibility for both packages.I've confirmed both packages' security status. Now let me verify the actual dependencies in the package.json file:
Based on my verification, both packages have clean security records and are compatible with Node 18+. The original review comment's concerns have been addressed.
Security verification complete: [email protected] and @types/[email protected] are both secure for Node 18+.
@types/ws has a consistent security record with zero vulnerabilities across all versions including 8.18.1, and [email protected] has no direct vulnerabilities in Snyk's database. The high-severity CVE-2024-37890 (DoS via excessive headers) was fixed in [email protected] and is not present in 8.18.3. Both versions are compatible with Node 18+.
packages/server/src/controllers/BackendController/BackendController.ts
Outdated
Show resolved
Hide resolved
packages/server/src/controllers/BackendController/BackendController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/controllers/LapController/LapController.ts (1)
161-161: Remove unnecessaryawaiton synchronous method.The
broadcastLapDatamethod is synchronous (returnsvoid, notPromise<void>), so usingawaithere is misleading. While not breaking, it suggests the method is async when it's not.Apply this diff to remove the unnecessary await:
await this.backendController.socketIO.broadcastLapData(lapData); - await this.backendController.grafanaWebSocket.broadcastLapData(lapData); + this.backendController.grafanaWebSocket.broadcastLapData(lapData); await this.backendController.dynamoDB.insertLapData(lapData);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/controllers/BackendController/BackendController.ts(2 hunks)packages/server/src/controllers/LapController/LapController.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (1)
GrafanaWebSocket(16-83)
🔇 Additional comments (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
7-7: LGTM! GrafanaWebSocket integration looks good.The import, field declaration, and instantiation are correctly implemented. The initialization order is appropriate since
grafanaWebSocketis instantiated beforelapController(which uses it).Note: There's a past review suggestion about creating an abstract WebSocket class for extensibility if other services will use WebSocket. Consider this for future refactoring.
Also applies to: 21-21, 28-28
| if ( | ||
| // packet.B3.LapDigital && //TEST: commented out for broadcasting lap data to grafana | ||
| this.lastLapPackets.length > 5 | ||
| ) { |
There was a problem hiding this comment.
Restore the lap completion condition - current logic is broken.
The commented-out packet.B3.LapDigital check was the primary lap completion trigger. Without it, lap data will be broadcast on every packet after the first 5 packets accumulate, not just when a lap is actually completed. This will:
- Generate spurious lap records in the database
- Spam the Grafana dashboard with incorrect lap data
- Make the lap tracking system unusable
The comment indicates this is a test change, but it breaks production functionality and should not be merged.
Apply this diff to restore the proper lap detection logic:
- if (
- // packet.B3.LapDigital && //TEST: commented out for broadcasting lap data to grafana
- this.lastLapPackets.length > 5
- ) {
+ if (packet.B3.LapDigital && this.lastLapPackets.length > 5) {If you need to test Grafana lap broadcasting without actual lap completion events, consider adding a separate test endpoint or feature flag rather than breaking the production lap detection logic.
📝 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 ( | |
| // packet.B3.LapDigital && //TEST: commented out for broadcasting lap data to grafana | |
| this.lastLapPackets.length > 5 | |
| ) { | |
| if (packet.B3.LapDigital && this.lastLapPackets.length > 5) { |
🤖 Prompt for AI Agents
In packages/server/src/controllers/LapController/LapController.ts around lines
194-197, the lap completion guard was weakened by commenting out the
packet.B3.LapDigital check, causing lap data to be broadcast after five packets
instead of only on actual lap completions; restore the original condition by
re-enabling the packet.B3.LapDigital (or equivalent lap-completed flag) in the
if statement so broadcasting only occurs when a lap completion event is present,
and if needed add a separate test flag or endpoint to allow forced/test
broadcasts without modifying production lap-detection logic.
dastine0308
force-pushed
the
TEL-313-Setup-Websocket-connection-between-Grafana-and-Backend-Helios-Telemetry
branch
from
5fe01b1 to
ecdd67f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
7-29: GrafanaWebSocket wiring is consistent with existing SocketIO patternThe new
GrafanaWebSocketimport, field, and constructor wiring mirror howSocketIOis integrated (server +BackendControllerpassed in), which keeps the data-source patterns consistent. I don’t see correctness issues with this setup.As a non-blocking design thought (echoing the earlier review), if more WS-based integrations appear later, an abstract WS layer or common interface/factory could help decouple
BackendControllerfrom concrete implementations, but this can be deferred until there’s a second consumer.
🧹 Nitpick comments (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
56-58: Commented-out Grafana packet broadcast is fine for current scopeLeaving
grafanaWebSocket.broadcastPacket(message)commented out with the note that onlyILapDatashould go over WS matches the current product decision and avoids double-sending telemetry that already reaches Grafana via MQTT.Once/if you decide to stream full telemetry via WS, consider either:
- Removing the comment and enabling this line, or
- Guarding WS telemetry behind a config flag instead of a hard-coded comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/controllers/BackendController/BackendController.ts(3 hunks)packages/server/src/controllers/LapController/LapController.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/controllers/LapController/LapController.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/src/controllers/BackendController/BackendController.ts (2)
packages/server/src/datasources/GrafanaWebSocket/GrafanaWebSocket.ts (1)
GrafanaWebSocket(16-83)packages/server/src/datasources/SocketIO/SocketIO.ts (1)
SocketIO(20-86)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/server/src/datasources/WebSocket/WebSocket.ts (1)
28-39: Optionally guard the initialws.sendin the connection handler with try/catch for consistencyYou already wrap
ws.sendinbroadcastPacket/broadcastLapDatawith try/catch, which is great. The initial connection handshakews.sendin the"connection"handler is still unguarded. While failures here are unlikely, wrapping it similarly would make the behaviour consistent and ensure a sudden state change on the socket can’t throw synchronously:- this.wss.on("connection", (ws: WebSocket, req: IncomingMessage) => { + this.wss.on("connection", (ws: WebSocket, req: IncomingMessage) => { logger.info( `WebSocket client connected from ${req.socket.remoteAddress}`, ); this.initializeWebSocketListeners(ws); - ws.send( - JSON.stringify({ - message: "Connected to Helios Telemetry WebSocket", - type: "connection", - }), - ); + try { + ws.send( + JSON.stringify({ + message: "Connected to Helios Telemetry WebSocket", + type: "connection", + }), + ); + } catch (error) { + logger.error("Error sending initial WebSocket connection message", error); + } });Not mandatory, but it aligns this path with your broadcast error handling.
🧹 Nitpick comments (1)
packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
1-9: Interface surface is clear; consider whetherwssneeds to be exposed
WebSocketTypecleanly describes the broadcasting API. If no callers outside the datasource need direct access to the underlyingWebSocketServer, you might consider omittingwssfrom the interface and keeping it as an implementation detail for easier future swapping/mocking. If it is needed, this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/server/src/controllers/BackendController/BackendController.ts(2 hunks)packages/server/src/controllers/BackendController/BackendController.types.ts(2 hunks)packages/server/src/controllers/LapController/LapController.ts(2 hunks)packages/server/src/datasources/WebSocket/WebSocket.ts(1 hunks)packages/server/src/datasources/WebSocket/WebSocket.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/src/controllers/BackendController/BackendController.types.ts
- packages/server/src/controllers/LapController/LapController.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
packages/shared/src/types.ts (2)
ILapData(421-437)ITelemetryData(399-399)
packages/server/src/controllers/BackendController/BackendController.ts (3)
packages/server/src/controllers/BackendController/BackendController.types.ts (1)
BackendControllerTypes(10-19)packages/server/src/controllers/LapController/LapController.ts (1)
LapController(40-506)packages/server/src/datasources/WebSocket/WebSocket.ts (1)
NativeWebSocket(14-88)
packages/server/src/datasources/WebSocket/WebSocket.ts (4)
packages/server/src/utils/logger.ts (1)
createLightweightApplicationLogger(98-102)packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
WebSocketType(5-9)packages/server/src/controllers/BackendController/BackendController.ts (1)
BackendController(18-75)packages/shared/src/types.ts (2)
ITelemetryData(399-399)ILapData(421-437)
🔇 Additional comments (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
10-31: Grafana WebSocket wiring and conditional usage look consistentThe NativeWebSocket integration (env-based
grafanaWsPath, ctor wiring, andwebSocketfield) is straightforward and matches how SocketIO is attached to the HTTPS server. LeavingbroadcastPacketcommented out with the inline note keeps the intent clear that only lap data is sent over WS for now. No issues from this change set.Also applies to: 58-60
dastine0308
force-pushed
the
TEL-313-Setup-Websocket-connection-between-Grafana-and-Backend-Helios-Telemetry
branch
from
83cd549 to
456d971
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
docs/GRAFANA.md (2)
69-71: Specify a language for the fenced code blockThe fenced block showing
http://localhost:3000has no language specifier. To satisfy markdownlint and improve rendering, change the opening fence to something like:-``` +```text http://localhost:3000--- `85-88`: **Replace hard tabs with spaces in the nested list** Lines 86–88 are indented with hard tab characters. Replace each leading tab with spaces (2 or 4, consistent with the repo style) so markdownlint passes and rendering is consistent: ```diff - - Click **Explore** in the left-hand menu. - - For **Data source**, choose **WebSocket API** (the WebSocket data source in the Telemetry-Visualizer setup). - - In the **Field** input enter: `$.data` + - Click **Explore** in the left-hand menu. + - For **Data source**, choose **WebSocket API** (the WebSocket data source in the Telemetry-Visualizer setup). + - In the **Field** input enter: `$.data`packages/server/src/controllers/LapController/LapController.ts (1)
194-236: Restore LapDigital guard so laps are only finalized on real lap completionsWith
packet.B3.LapDigitalcommented out, the condition now fires wheneverlastLapPackets.length > 5, so every run of 6+ packets will be treated as a completed lap. That will spam lap records and Grafana with bogus laps and makes lap tracking unreliable.Restore the original LapDigital check:
- if ( - //TEST: commented out this condition for broadcasting lap data to grafana - // packet.B3.LapDigital && - this.lastLapPackets.length > 5 - ) { + if (packet.B3.LapDigital && this.lastLapPackets.length > 5) {If you need a way to test Grafana broadcasting without actual lap events, consider a dedicated test hook/flag instead of weakening production lap detection.
packages/server/src/datasources/WebSocket/WebSocket.ts (2)
28-39: Guard the initialws.sendin the connection handler with try/catchYou’ve added robust try/catch handling around broadcast sends, but the welcome message in the
"connection"handler still callsws.send(...)without protection. If a client disconnects immediately after connecting (or the socket isn’t in OPEN state),sendcan throw synchronously and bubble out of the listener.Mirror the broadcast pattern here:
- this.initializeWebSocketListeners(ws); - ws.send( - JSON.stringify({ - message: "Connected to Helios Telemetry WebSocket", - type: "connection", - }), - ); + this.initializeWebSocketListeners(ws); + try { + ws.send( + JSON.stringify({ + message: "Connected to Helios Telemetry WebSocket", + type: "connection", + }), + ); + } catch (error) { + logger.error("Error sending initial WebSocket handshake message", error); + }This keeps a single misbehaving Grafana/WebSocket client from impacting the server.
1-3: Fix invalidexport default WebSocket(type‑only import used as a value and wrong default)
WebSocketis imported withtypeonly, but at the bottom of the file you doexport default WebSocket;. That will fail type‑checking (type‑only import used as a value) and also exports the ws library’s class instead of yourNativeWebSocketwrapper.Either remove the default export or point it at
NativeWebSocket:-export default WebSocket; +// Either remove the default export entirely: +// (recommended if everything uses the named export) +// export default removed + +// or, if you really want a default: +export default NativeWebSocket;Then ensure consumers import this module consistently (either named or default, but not both).
Also applies to: 88-90
🧹 Nitpick comments (3)
packages/server/.env.example (1)
5-56: AddGRAFANA_WS_PATHto the server env example for discoverabilitySince
GRAFANA_WS_PATHis now used by the backend and referenced indocs/GRAFANA.md, consider adding a placeholder here (e.g.GRAFANA_WS_PATH="/grafana-ws"under a WebSocket/Grafana section) so new developers see all required server env vars in one place. The existing dotenv-linter warnings about quotes/key ordering are cosmetic and can be ignored or cleaned up later if you want a zero‑warning run.packages/server/src/controllers/LapController/LapController.ts (1)
158-163: WebSocket lap broadcast wiring looks correct; consider error‑isolation from DB writesAdding
backendController.webSocket.broadcastLapData(lapData)alongside the existing Socket.IO broadcast keeps Grafana clients in sync with the frontend and happens before the DynamoDB insert, which is a sensible order. If you ever want DB writes to proceed even when broadcasting fails, consider wrapping each broadcast in its own try/catch (or running them independently) so a transient WS/socket error can’t blockinsertLapData.packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
5-9: AlignWebSocketTypemethod return types with call‑site usage
broadcastLapDataandbroadcastPacketare typed as returningvoid, but call sites likeLapController.handleLapDatacurrentlyawaitthem. That works at runtime (the calls are synchronous), but it’s a bit misleading for readers and type‑checking.Consider either:
- Changing these signatures to
Promise<void>and marking implementationsasync, or- Keeping them synchronous and dropping the unnecessary
awaitat the call sites.Either option keeps behavior the same but makes intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
README.md(1 hunks)docs/GRAFANA.md(1 hunks)packages/server/.env.example(1 hunks)packages/server/package.json(2 hunks)packages/server/src/controllers/BackendController/BackendController.ts(2 hunks)packages/server/src/controllers/BackendController/BackendController.types.ts(2 hunks)packages/server/src/controllers/LapController/LapController.ts(2 hunks)packages/server/src/datasources/WebSocket/WebSocket.ts(1 hunks)packages/server/src/datasources/WebSocket/WebSocket.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/server/src/controllers/BackendController/BackendController.ts
- packages/server/src/controllers/BackendController/BackendController.types.ts
- README.md
- packages/server/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
packages/shared/src/types.ts (2)
ILapData(421-437)ITelemetryData(399-399)
packages/server/src/datasources/WebSocket/WebSocket.ts (4)
packages/server/src/utils/logger.ts (1)
createLightweightApplicationLogger(98-102)packages/server/src/datasources/WebSocket/WebSocket.types.ts (1)
WebSocketType(5-9)packages/server/src/controllers/BackendController/BackendController.ts (1)
BackendController(19-100)packages/shared/src/types.ts (2)
ITelemetryData(399-399)ILapData(421-437)
🪛 dotenv-linter (4.0.0)
packages/server/.env.example
[warning] 8-8: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 9-9: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [UnorderedKey] The DRIVER_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 11-11: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 11-11: [UnorderedKey] The GPS_CALCULATED_LAP_DATA_TABLE key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 12-12: [UnorderedKey] The GPS_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [UnorderedKey] The LOKI_PASSWORD key should go before the LOKI_USERNAME key
(UnorderedKey)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 35-35: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [UnorderedKey] The MQTT_PASSWORD key should go before the MQTT_USERNAME key
(UnorderedKey)
[warning] 41-41: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 42-42: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 47-47: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 48-48: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 50-50: [UnorderedKey] The LOG_ERROR_STACK key should go before the LOG_FILE key
(UnorderedKey)
[warning] 56-56: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 56-56: [UnorderedKey] The ENV key should go before the LOG_DIR key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
docs/GRAFANA.md
3-3: Bare URL used
(MD034, no-bare-urls)
70-70: Link fragments should be valid
(MD051, link-fragments)
71-71: Link fragments should be valid
(MD051, link-fragments)
72-72: Link fragments should be valid
(MD051, link-fragments)
73-73: Link fragments should be valid
(MD051, link-fragments)
74-74: Link fragments should be valid
(MD051, link-fragments)
75-75: Link fragments should be valid
(MD051, link-fragments)
76-76: Link fragments should be valid
(MD051, link-fragments)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/.env.example (1)
58-61: Add a trailing newline at the end of the file.Standard practice for text files is to end with a newline character. Also, consider wrapping the GRAFANA_WS_PATH value in quotes for consistency with other environment variables in the file, e.g.,
GRAFANA_WS_PATH="/grafana-ws".Apply this diff:
######################## # Grafana WebSocket # ######################## -GRAFANA_WS_PATH=/grafana-ws \ No newline at end of file +GRAFANA_WS_PATH="/grafana-ws"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/server/.env.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
packages/server/.env.example
[warning] 8-8: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 9-9: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [UnorderedKey] The DRIVER_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 11-11: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 11-11: [UnorderedKey] The GPS_CALCULATED_LAP_DATA_TABLE key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 12-12: [UnorderedKey] The GPS_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [UnorderedKey] The LOKI_PASSWORD key should go before the LOKI_USERNAME key
(UnorderedKey)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 35-35: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [UnorderedKey] The MQTT_PASSWORD key should go before the MQTT_USERNAME key
(UnorderedKey)
[warning] 41-41: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 42-42: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 47-47: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 48-48: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 50-50: [UnorderedKey] The LOG_ERROR_STACK key should go before the LOG_FILE key
(UnorderedKey)
[warning] 56-56: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 56-56: [UnorderedKey] The ENV key should go before the LOG_DIR key
(UnorderedKey)
[warning] 61-61: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (5)
packages/server/.env.example (5)
1-12: File structure and Database section look good.The header comments clearly explain the file's purpose, and the Database section is well-organized. The
QuoteCharacterwarnings from dotenv-linter are false positives—quotes around placeholder values are intentional and standard practice in .env.example files to indicate string values.
14-30: Loki section is well-documented.The authentication options are clearly explained, and the LOKI_LABELS format is helpful. This section provides good guidance for developers configuring logging.
32-42: MQTT and App secrets sections are appropriate.The MQTT section provides default credentials (intended for local development), while App secrets use placeholders. This mixed approach is reasonable for development convenience.
44-50: Local logging section uses sensible defaults.The logging configuration is clear and developer-friendly, with helpful comments for the LOG_LEVEL option.
52-56: Node/Environment section is correct.Both NODE_ENV and ENV are appropriately set to "development" for this example.
…e/ws into devDependencies
…r and adjust condition in LapController
dastine0308
force-pushed
the
TEL-313-Setup-Websocket-connection-between-Grafana-and-Backend-Helios-Telemetry
branch
from
6d2c984 to
55b763f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/controllers/BackendController/BackendController.ts (1)
92-99: Add WebSocket server cleanup.The cleanup method should close the WebSocket server to ensure all connections are properly terminated and resources are released during shutdown.
public async cleanup() { try { await this.databaseManager.close(); logger.info("Database connection closed successfully"); } catch (error) { logger.error("Error closing database connection:", error); } + + try { + this.webSocket.wss.close(() => { + logger.info("WebSocket server closed successfully"); + }); + } catch (error) { + logger.error("Error closing WebSocket server:", error); + } }
♻️ Duplicate comments (2)
docs/GRAFANA.md (2)
69-71: Add language specifier to fenced code block.This issue was previously flagged but remains unresolved. The fenced code block is missing a language identifier for proper syntax highlighting in rendered markdown.
Apply this diff:
-``` +```text http://localhost:3000--- `86-88`: **Replace hard tabs with spaces for markdown compatibility.** This issue was previously flagged but remains unresolved. Lines 86–88 use hard tabs for indentation. Markdown linters and renderers work best with spaces. Convert tabs to spaces (2 or 4 spaces per indent level). Apply this diff: ```diff - - Click **Explore** in the left-hand menu. - - For **Data source**, choose **WebSocket API** (the WebSocket data source in the Telemetry-Visualizer setup). - - In the **Field** input enter: `$.data` + - Click **Explore** in the left-hand menu. + - For **Data source**, choose **WebSocket API** (the WebSocket data source in the Telemetry-Visualizer setup). + - In the **Field** input enter: `$.data`
🧹 Nitpick comments (3)
packages/server/.env.example (2)
58-61: Add documentation comment for GRAFANA_WS_PATH.The new GRAFANA_WS_PATH variable lacks an explanatory comment describing its purpose and how to configure it. Following the pattern used for other environment variables (e.g., LOKI_URL at line 18), add a comment explaining what this WebSocket path is used for.
Additionally, consider whether the default value
/grafana-wsshould be provided, or whether this should be an empty string placeholder (like LOKI_URL) to encourage explicit configuration.######################## # Grafana WebSocket # ######################## +# The WebSocket path used to broadcast telemetry and lap data to Grafana. +# This must match the path configured in your Grafana instance. -GRAFANA_WS_PATH=/grafana-ws +GRAFANA_WS_PATH="" # e.g. "/grafana-ws"Alternatively, if
/grafana-wsis the recommended/standard default, keep the current value and just add the comment above it.
61-61: Add trailing newline at end of file.The file is missing a blank line at the end, which is a common style convention. Add a trailing newline after line 61.
GRAFANA_WS_PATH=/grafana-ws +packages/server/src/controllers/BackendController/BackendController.ts (1)
32-32: Add error handling for WebSocket initialization.If the WebSocket server fails to start (e.g., port conflict, invalid configuration), the constructor will throw and crash the server. Consider wrapping this in try-catch similar to the database initialization pattern used in
initializeDatabase().this.dynamoDB = new DynamoDB(this); this.socketIO = new SocketIO(httpsServer, this); - this.webSocket = new NativeWebSocket(grafanaWsPath, httpsServer, this); + try { + this.webSocket = new NativeWebSocket(grafanaWsPath, httpsServer, this); + } catch (error) { + logger.error("Failed to initialize WebSocket:", error); + throw error; // or handle gracefully depending on criticality + } this.mqtt = new SolarMQTTClient(options, this);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
README.md(1 hunks)docs/GRAFANA.md(1 hunks)packages/server/.env.example(1 hunks)packages/server/package.json(2 hunks)packages/server/src/controllers/BackendController/BackendController.ts(2 hunks)packages/server/src/controllers/BackendController/BackendController.types.ts(2 hunks)packages/server/src/controllers/LapController/LapController.ts(2 hunks)packages/server/src/datasources/WebSocket/WebSocket.ts(1 hunks)packages/server/src/datasources/WebSocket/WebSocket.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- packages/server/src/controllers/LapController/LapController.ts
- packages/server/src/datasources/WebSocket/WebSocket.types.ts
- packages/server/package.json
- packages/server/src/datasources/WebSocket/WebSocket.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/src/controllers/BackendController/BackendController.types.ts (1)
packages/server/src/datasources/WebSocket/WebSocket.ts (1)
NativeWebSocket(14-88)
🪛 dotenv-linter (4.0.0)
packages/server/.env.example
[warning] 8-8: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 9-9: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [UnorderedKey] The DRIVER_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 11-11: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 11-11: [UnorderedKey] The GPS_CALCULATED_LAP_DATA_TABLE key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 12-12: [UnorderedKey] The GPS_TABLE_NAME key should go before the LAP_TABLE_NAME key
(UnorderedKey)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [UnorderedKey] The LOKI_PASSWORD key should go before the LOKI_USERNAME key
(UnorderedKey)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 35-35: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 36-36: [UnorderedKey] The MQTT_PASSWORD key should go before the MQTT_USERNAME key
(UnorderedKey)
[warning] 41-41: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 42-42: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 47-47: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 48-48: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 50-50: [UnorderedKey] The LOG_ERROR_STACK key should go before the LOG_FILE key
(UnorderedKey)
[warning] 56-56: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 56-56: [UnorderedKey] The ENV key should go before the LOG_DIR key
(UnorderedKey)
[warning] 61-61: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 markdownlint-cli2 (0.18.1)
docs/GRAFANA.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
packages/server/src/controllers/BackendController/BackendController.types.ts (1)
6-6: LGTM!The type additions correctly reflect the new WebSocket integration and follow the established pattern for other datasources.
Also applies to: 18-18
packages/server/src/controllers/BackendController/BackendController.ts (2)
10-10: LGTM!The import and environment configuration for the Grafana WebSocket path are correctly implemented with a sensible default.
Also applies to: 16-16
74-76: Correctly commented out per team decision.The commented-out broadcast is intentional, as the team decided to only send
ILapDatathrough WebSocket (handled inLapController), while Grafana receivesITelemetryDatapackets via MQTT.Based on past review comments.
PR Description
Screenshot
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.