Updated LapController with button triggered lap logic.#298
Open
alexwhelan12 wants to merge 3 commits intomainfrom
Open
Updated LapController with button triggered lap logic.#298alexwhelan12 wants to merge 3 commits intomainfrom
alexwhelan12 wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughLapController now always buffers incoming packets, uses a LapDigital false→true transition (with a previousLapDigital state) to gate lap emission, requires at least 5 buffered packets before emitting a lap, increments lapNumber on emit, and fixes argument order for battery seconds-remaining calculation. Changes
Sequence Diagram(s)sequenceDiagram
participant Telemetry as TelemetryPacketSource
participant LapCtrl as LapController
participant Broad as Broadcaster
participant DB as Database
Telemetry->>LapCtrl: send packet (includes B3.LapDigital)
activate LapCtrl
LapCtrl->>LapCtrl: push packet to lastLapPackets
LapCtrl->>LapCtrl: compute lapDigital from packet.B3.LapDigital
alt previousLapDigital false -> true
LapCtrl->>LapCtrl: if lastLapPackets.length < MIN_PACKETS_FOR_LAP then warn & broadcast race info, return
LapCtrl->>LapCtrl: construct lapData (correct amphours/avgCurrent arg order)
LapCtrl->>LapCtrl: validate lap calculations (isFinite / hasValidLapCalculations)
alt valid
LapCtrl->>Broad: broadcast lap and race info
LapCtrl->>DB: insert lap record
LapCtrl->>LapCtrl: increment raceInfo.lapNumber and clear lastLapPackets
else invalid
LapCtrl->>Broad: broadcast warning/info
end
else no lap transition
LapCtrl-->>Telemetry: continue buffering
end
deactivate LapCtrl
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/server/src/controllers/LapController/LapController.ts`:
- Around line 214-215: The broadcastLapComplete() call and any
raceInfo.lapNumber mutation must occur only after the lap has been
validated/accepted by checkLap(); update the digital flow (the block gated by
canInsertLapFromDigital and the guard around checkLap()) so that you call
checkLap() first, only broadcast via broadcastLapComplete() if checkLap()
indicates acceptance, and then increment raceInfo.lapNumber (or otherwise
finalize raceInfo) as part of the accepted-path; ensure no early broadcasts or
increments occur before checkLap() returns success so rejected laps never emit
completion nor leave raceInfo advanced.
- Around line 193-203: The code currently treats a discarded boundary
(early-return branches around the LapDigital edge handling) as leaving old
packets in this.lastLapPackets so the next accepted false->true edge includes
stale history; update the early-return branches that reject/discard boundary
packets (the branches that check lapDigital / canInsertLapFromDigital and the
other discard paths) to reset/reseed the lap buffer by clearing
this.lastLapPackets and, where appropriate, reinitializing
this.previousLapDigital (or setting it to the current lapDigital) so the next
accepted edge starts with a fresh buffer and no leftover packets.
- Around line 175-177: The unconditional push to lastLapPackets in LapController
causes an unbounded buffer growth; modify the logic in the method where
this.push occurs (the packet handling path that appends to lastLapPackets) to
either (a) only append when a race/lap is active (use your race state or
LapDigital flag to gate pushes) or (b) replace the array with a fixed-size
ring/bounded buffer (cap size, drop oldest entries) and ensure you clear/reset
lastLapPackets when the race becomes inactive or when LapDigital indicates no
lap is in progress; update references to lastLapPackets and any packet-handling
method accordingly so the buffer cannot grow unbounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db11daa1-a7ca-4e48-bb49-a9b74da428e3
📒 Files selected for processing (1)
packages/server/src/controllers/LapController/LapController.ts
alexwhelan12
force-pushed
the
end-to-end-testing
branch
from
6918ffc to
82e9072
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/controllers/LapController/LapController.ts (1)
253-259:⚠️ Potential issue | 🔴 CriticalMissing
hasValidLapCalculationsmethod causes build failure.The pipeline fails with
TS2339: Property 'hasValidLapCalculations' does not exist on type 'LapController'.The method is called but never defined in this file.Additionally,
raceInfo.lapNumberis never incremented in the digital lap path—it only increments incheckLap()(the geofence approach).🐛 Proposed fix: Add the missing validation method and increment lap number
+ private isFiniteNumber(value: number): boolean { + return typeof value === "number" && Number.isFinite(value); + } + + private hasValidLapCalculations(lapData: ILapData): boolean { + return ( + this.isFiniteNumber(lapData.AmpHours) && + this.isFiniteNumber(lapData.AveragePackCurrent) && + this.isFiniteNumber(lapData.AverageSpeed) && + this.isFiniteNumber(lapData.BatterySecondsRemaining) && + this.isFiniteNumber(lapData.Distance) && + this.isFiniteNumber(lapData.EnergyConsumed) && + this.isFiniteNumber(lapData.LapTime) && + this.isFiniteNumber(lapData.NetPowerOut) && + this.isFiniteNumber(lapData.TotalPowerIn) && + this.isFiniteNumber(lapData.TotalPowerOut) + ); + }Also increment the lap number after validation succeeds (near line 260):
if (!this.hasValidLapCalculations(lapData)) { logger.warn( "Ignoring lap boundary: lap calculations are not valid yet.", ); this.backendController.socketIO.broadcastRaceInfo(this.raceInfo); + this.lastLapPackets = [packet]; return; } + this.raceInfo.lapNumber += 1; 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` around lines 253 - 259, Add a missing validation method and increment the lap counter in the digital-lap path: implement a private hasValidLapCalculations(lapData: LapData) method on LapController that performs the same sanity checks used elsewhere (e.g., verifies lapData.distance/time/required fields and returns boolean) and call it from the existing digital-lap branch; after the validation succeeds, increment this.raceInfo.lapNumber (the same place checkLap() increments lapNumber for the geofence path) and then broadcast updated race info via backendController.socketIO.broadcastRaceInfo(this.raceInfo). Ensure the new method name matches hasValidLapCalculations exactly and is used where currently referenced.
🤖 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/controllers/LapController/LapController.ts`:
- Around line 253-259: Add a missing validation method and increment the lap
counter in the digital-lap path: implement a private
hasValidLapCalculations(lapData: LapData) method on LapController that performs
the same sanity checks used elsewhere (e.g., verifies
lapData.distance/time/required fields and returns boolean) and call it from the
existing digital-lap branch; after the validation succeeds, increment
this.raceInfo.lapNumber (the same place checkLap() increments lapNumber for the
geofence path) and then broadcast updated race info via
backendController.socketIO.broadcastRaceInfo(this.raceInfo). Ensure the new
method name matches hasValidLapCalculations exactly and is used where currently
referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 672f2991-107f-4925-8735-b1244949194d
📒 Files selected for processing (1)
packages/server/src/controllers/LapController/LapController.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/controllers/LapController/LapController.ts (1)
253-254: Consider awaitinghandleLapDatafor consistent error handling.
handleLapDatais an async method that performs database inserts and broadcasts, but it's called withoutawait. While internal errors are caught, unhandled promise rejections could occur. Addingawaitwould ensure lap data persistence completes before clearing the buffer.- this.handleLapData(lapData); + await this.handleLapData(lapData); this.lastLapPackets = [];🤖 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 253 - 254, Call and await the async handler so lap persistence completes before clearing the buffer: replace the fire-and-forget invocation of handleLapData with an awaited call (await this.handleLapData(lapData)) and only set this.lastLapPackets = [] after that await completes; if the enclosing function is not async, make it async or wrap the await in an async IIFE, and preserve existing error handling (or add a try/catch around the await to surface/log rejections).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 253-254: Call and await the async handler so lap persistence
completes before clearing the buffer: replace the fire-and-forget invocation of
handleLapData with an awaited call (await this.handleLapData(lapData)) and only
set this.lastLapPackets = [] after that await completes; if the enclosing
function is not async, make it async or wrap the await in an async IIFE, and
preserve existing error handling (or add a try/catch around the await to
surface/log rejections).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9b42e0f-370e-403f-8bc1-88d43ad2c42e
📒 Files selected for processing (1)
packages/server/src/controllers/LapController/LapController.ts
Contributor
There was a problem hiding this comment.
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/LapController/LapController.ts (1)
225-256:⚠️ Potential issue | 🟡 Minor
broadcastLapCompletefires beforelapNumberis incremented.At Line 225,
broadcastLapComplete()is called whileraceInfo.lapNumberstill reflects the previous count. The increment happens at Line 255, after lap data handling. If clients expect the updated lap number when receiving the completion event, they'll see stale data.Proposed fix: increment before broadcasting
+ this.raceInfo.lapNumber += 1; await this.backendController.socketIO.broadcastLapComplete(); // mark lap, calculate lap, and add to lap table in database // send lap over socket @@ this.handleLapData(lapData); - this.raceInfo.lapNumber += 1; this.lastLapPackets = [];🤖 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 225 - 256, The broadcastLapComplete() call currently occurs before raceInfo.lapNumber is incremented, causing clients to receive a stale lap number; move the increment of this.raceInfo.lapNumber to happen before calling this.backendController.socketIO.broadcastLapComplete(), ensuring you still build and handle lapData (this.handleLapData) and clear this.lastLapPackets afterward so the broadcast reflects the updated lap count.
♻️ Duplicate comments (1)
packages/server/src/controllers/LapController/LapController.ts (1)
217-223:⚠️ Potential issue | 🟠 MajorEarly return leaves stale packets in the buffer.
When the boundary is rejected due to insufficient packets,
lastLapPacketsretains all buffered data. The next validfalse → trueedge will include these old packets, skewing calculations.Proposed fix: reseed buffer with current packet
if (this.lastLapPackets.length < MIN_PACKETS_FOR_LAP) { logger.warn( `Ignoring lap boundary: only ${this.lastLapPackets.length} packet(s) buffered, need at least ${MIN_PACKETS_FOR_LAP}.`, ); this.backendController.socketIO.broadcastRaceInfo(this.raceInfo); + this.lastLapPackets = [packet]; return; }🤖 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 217 - 223, The early return in the LapController boundary check leaves stale packets in this.lastLapPackets so a future valid false→true edge will include old data; inside the same method in LapController where you check if (this.lastLapPackets.length < MIN_PACKETS_FOR_LAP) and then broadcastRaceInfo and return, replace the pure early return with reseeding the buffer: clear this.lastLapPackets and push the current packet (the packet being processed in that handler) so the buffer starts fresh, then call this.backendController.socketIO.broadcastRaceInfo(this.raceInfo) and return; this ensures subsequent boundary detection only sees packets after the rejected edge.
🧹 Nitpick comments (1)
packages/server/src/controllers/LapController/LapController.ts (1)
43-43: Consider makingpreviousLapDigitalprivate.This field appears to be internal state for detecting LapDigital transitions. Unless it's needed for testing or external access,
privatevisibility would better encapsulate the state machine logic.🤖 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 43, The field previousLapDigital on LapController is internal state and should be made private to encapsulate transition logic; change the visibility of previousLapDigital from public to private within the LapController class, and if any external code or tests access it, either provide a controlled accessor (e.g., a private getter used only internally) or update those callers to use the public API instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 225-256: The broadcastLapComplete() call currently occurs before
raceInfo.lapNumber is incremented, causing clients to receive a stale lap
number; move the increment of this.raceInfo.lapNumber to happen before calling
this.backendController.socketIO.broadcastLapComplete(), ensuring you still build
and handle lapData (this.handleLapData) and clear this.lastLapPackets afterward
so the broadcast reflects the updated lap count.
---
Duplicate comments:
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Around line 217-223: The early return in the LapController boundary check
leaves stale packets in this.lastLapPackets so a future valid false→true edge
will include old data; inside the same method in LapController where you check
if (this.lastLapPackets.length < MIN_PACKETS_FOR_LAP) and then broadcastRaceInfo
and return, replace the pure early return with reseeding the buffer: clear
this.lastLapPackets and push the current packet (the packet being processed in
that handler) so the buffer starts fresh, then call
this.backendController.socketIO.broadcastRaceInfo(this.raceInfo) and return;
this ensures subsequent boundary detection only sees packets after the rejected
edge.
---
Nitpick comments:
In `@packages/server/src/controllers/LapController/LapController.ts`:
- Line 43: The field previousLapDigital on LapController is internal state and
should be made private to encapsulate transition logic; change the visibility of
previousLapDigital from public to private within the LapController class, and if
any external code or tests access it, either provide a controlled accessor
(e.g., a private getter used only internally) or update those callers to use the
public API instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f2721b2-89b7-477a-b768-0c12e4362275
📒 Files selected for processing (1)
packages/server/src/controllers/LapController/LapController.ts
burtonjong
approved these changes
Mar 14, 2026
justin-phxm
requested changes
Mar 14, 2026
|
|
||
| public async handlePacket(packet: ITelemetryData) { | ||
| // Always buffer packets so a lap boundary has data available for calculations. | ||
| // NOTE: The lastLapPackets will grow indefinitely if LapDigital is never triggered. |
Collaborator
There was a problem hiding this comment.
2 * 60 * 60 = 7200.
Set a maximum array size of 7200 to avoid memory overflows.
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
3 participants
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.
Summary by CodeRabbit
Bug Fixes
Improvements