fix: smooth track map marker movement - #688
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds reusable driver-progress interpolation for curved and flat track maps. It replaces effect-driven animation loops with ChangesTrack map animation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TrackMapCanvas
participant useProgressAnimation
participant perfMetrics.measure
participant rendererPerfBridge
participant recordRendererMeasure
TrackMapCanvas->>useProgressAnimation: Provide driver progress targets
useProgressAnimation->>perfMetrics.measure: Measure animation-frame drawing
perfMetrics.measure->>TrackMapCanvas: Execute drawing callback
perfMetrics.measure->>rendererPerfBridge: Record frame duration
rendererPerfBridge->>recordRendererMeasure: Store valid track-map timing
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58cc663230
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const frame = (now: number) => { | ||
| const active = interpolator.advance(now); | ||
| drawRef.current(interpolator.getValues(), interpolator.getCount()); | ||
| frameRef.current = active ? requestAnimationFrame(frame) : 0; |
There was a problem hiding this comment.
The new requestAnimationFrame callback runs interpolation and full canvas drawing at display refresh rate without a perfMetrics.measure('<name>', fn) wrapper. Architecture rule R13.1 requires new high-frequency paths to appear in the existing performance overlay; without this instrumentation, regressions from full-grid per-frame drawing cannot be observed through the project's metrics. Add a stable, literal measurement around the frame work.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| @@ -0,0 +1,199 @@ | |||
| import { useLayoutEffect, useRef } from 'react'; | |||
|
|
|||
| export const TRACK_POSITION_INTERVAL_MS = 1000 / 25; | |||
There was a problem hiding this comment.
With the documented 60 Hz curated telemetry input, ProcessorHost.isDue only processes the nominal 25 Hz channel on the first frame at least 40 ms after the previous one, which is every third input frame—approximately 50 ms. Hard-coding interpolation to 40 ms therefore makes markers reach each target early and pause or visibly decelerate before the next snapshot, especially on high-refresh displays, partially retaining the stutter this change is intended to remove. Derive the duration from consecutive snapshot timestamps or otherwise account for the effective cadence.
Useful? React with 👍 / 👎.
![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 (4)
src/app/bridge/rendererExposeBridge.ts (1)
34-40: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider skipping measurement work when renderer perf metrics are disabled.
rendererPerfBridgeis always exposed.perfMetrics.measuretherefore always takes twoperformance.now()samples and crosses the context bridge, once per animation frame per track-map widget, even when metrics collection is off.recordRendererMeasurethen discards the value because the sample buffer is undefined. The telemetry path already gates onisRendererPerfMetricsEnabled()at line 92.An early return keeps the same behaviour and avoids the per-frame bridge call once the frontend checks the bridge presence. If you prefer to keep the bridge always defined, the gate inside
recordMeasureis still cheaper than the buffer lookup.♻️ Optional gate
defineBridge<RendererPerfBridge>('rendererPerfBridge', { recordMeasure: (name, durationMs) => { + if (!isRendererPerfMetricsEnabled()) return; if (name !== 'trackMapAnimationFrame') return; if (!Number.isFinite(durationMs) || durationMs < 0) return; recordRendererMeasure(name, durationMs); }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/bridge/rendererExposeBridge.ts` around lines 34 - 40, Update the rendererPerfBridge exposure in rendererExposeBridge.ts so recordMeasure first checks isRendererPerfMetricsEnabled() and returns when metrics are disabled, avoiding recordRendererMeasure and unnecessary bridge work while preserving the existing name and duration validation.src/frontend/components/TrackMap/useProgressAnimation.ts (1)
213-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant unmount cleanup effect.
The main effect at lines 172-203 returns a cleanup that cancels
frameRef.currentand resets it to0. React runs that cleanup on unmount. This third effect therefore always seesframeRef.current === 0and cancels nothing.♻️ Proposed removal
- useLayoutEffect( - () => () => { - if (frameRef.current !== 0) cancelAnimationFrame(frameRef.current); - }, - [] - ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/TrackMap/useProgressAnimation.ts` around lines 213 - 218, Remove the redundant third useLayoutEffect cleanup block after the main progress animation effect; the main effect’s cleanup already cancels frameRef.current and resets it to 0 on unmount.src/frontend/components/TrackMap/trackDrawingUtils.ts (1)
149-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the ordering contract and consider sharing the comparator.
The function now sorts only record inputs. Array inputs are drawn in the supplied order. A caller that passes an array without pre-sorting silently loses the pit-road-first, player-last, leader-on-top layering.
TrackCanvas.tsxlines 228-238 duplicate this exact comparator to satisfy the new contract.Two small improvements:
- Export the comparator from this file and reuse it in
TrackCanvas.tsx, so the ordering rule has one definition.- Rename
calculatePositions. The parameter now holds data, not a function, and the name predates the union type.♻️ Suggested shape
+/** Draw order: pit-road cars first, then leader-on-top, then the player. */ +export const compareDrawOrder = ( + a: PositionedTrackDriver, + b: PositionedTrackDriver, + carIdxIsOnPitRoad?: readonly boolean[] +): number => { + const safePosition = (pos: number | undefined): number => + pos !== undefined && isFinite(pos) ? pos : 0; + const aOnPit = !!carIdxIsOnPitRoad?.[a.driver.CarIdx]; + const bOnPit = !!carIdxIsOnPitRoad?.[b.driver.CarIdx]; + if (aOnPit !== bOnPit) return aOnPit ? -1 : 1; + if (a.isPlayer !== b.isPlayer) return Number(a.isPlayer) - Number(b.isPlayer); + return safePosition(b.sessionPosition) - safePosition(a.sessionPosition); +}; + export const drawDrivers = ( ctx: CanvasRenderingContext2D, - calculatePositions: - PositionedTrackDriver[] | Record<number, PositionedTrackDriver>, + /** Arrays are drawn in the supplied order. Records are sorted by `compareDrawOrder`. */ + positionedDrivers: + | PositionedTrackDriver[] + | Record<number, PositionedTrackDriver>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/TrackMap/trackDrawingUtils.ts` around lines 149 - 181, Update drawDrivers to apply one shared ordering comparator to both array and record inputs, preserving pit-road-first, player-last, and leader-on-top layering; export that comparator from this module and reuse it in TrackCanvas.tsx instead of duplicating the logic. Rename the calculatePositions parameter and its references to reflect that it contains positioned driver data rather than a callable.src/frontend/components/TrackMap/useProgressAnimation.spec.tsx (1)
11-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a changing driver roster.
ProgressInterpolator.setTargetsmatches each new entry against the previous snapshot bydriver.CarIdxand falls back to the target value when no match exists. That matching loop is the most error-prone part of the class, and no test covers it. All current tests pass entries without adriverfield, sodriverIdalways falls back to the array index.A test that reorders drivers, removes one, and adds one would lock in the behaviour that a re-ordered driver keeps its interpolated value and a new driver snaps to its target.
💚 Suggested test
+ it('keeps interpolated values when drivers are reordered', () => { + const interpolator = new ProgressInterpolator(40); + interpolator.setTargets( + [ + { progress: 0.1, driver: { CarIdx: 7 } }, + { progress: 0.5, driver: { CarIdx: 3 } }, + ], + 0 + ); + interpolator.setTargets( + [ + { progress: 0.7, driver: { CarIdx: 3 } }, + { progress: 0.9, driver: { CarIdx: 9 } }, + ], + 0 + ); + + // CarIdx 3 continues from 0.5; CarIdx 9 is new and snaps to 0.9. + expect(interpolator.getValues()[0]).toBeCloseTo(0.5); + expect(interpolator.getValues()[1]).toBeCloseTo(0.9); + expect(interpolator.getCount()).toBe(2); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/TrackMap/useProgressAnimation.spec.tsx` around lines 11 - 62, Add a ProgressInterpolator test covering changing driver rosters: initialize targets with distinct driver.CarIdx values, then reorder the existing drivers while removing one and adding another, and verify the reordered driver retains its interpolated prior value while the new driver uses its target value. Exercise setTargets and advance sufficiently to distinguish matching by driver.CarIdx from array position.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/components/TrackMap/useProgressAnimation.ts`:
- Around line 164-166: Move the drawRef.current assignment out of the render
body and into the existing per-render layout effect near the RAF setup. Keep
drawRef initialization unchanged, and ensure the effect updates drawRef.current
on every committed render before repainting with the current draw callback.
---
Nitpick comments:
In `@src/app/bridge/rendererExposeBridge.ts`:
- Around line 34-40: Update the rendererPerfBridge exposure in
rendererExposeBridge.ts so recordMeasure first checks
isRendererPerfMetricsEnabled() and returns when metrics are disabled, avoiding
recordRendererMeasure and unnecessary bridge work while preserving the existing
name and duration validation.
In `@src/frontend/components/TrackMap/trackDrawingUtils.ts`:
- Around line 149-181: Update drawDrivers to apply one shared ordering
comparator to both array and record inputs, preserving pit-road-first,
player-last, and leader-on-top layering; export that comparator from this module
and reuse it in TrackCanvas.tsx instead of duplicating the logic. Rename the
calculatePositions parameter and its references to reflect that it contains
positioned driver data rather than a callable.
In `@src/frontend/components/TrackMap/useProgressAnimation.spec.tsx`:
- Around line 11-62: Add a ProgressInterpolator test covering changing driver
rosters: initialize targets with distinct driver.CarIdx values, then reorder the
existing drivers while removing one and adding another, and verify the reordered
driver retains its interpolated prior value while the new driver uses its target
value. Exercise setTargets and advance sufficiently to distinguish matching by
driver.CarIdx from array position.
In `@src/frontend/components/TrackMap/useProgressAnimation.ts`:
- Around line 213-218: Remove the redundant third useLayoutEffect cleanup block
after the main progress animation effect; the main effect’s cleanup already
cancels frameRef.current and resets it to 0 on unmount.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b369e18c-fa90-4fbc-8969-1b36c064caaf
📒 Files selected for processing (10)
src/app/bridge/rendererExposeBridge.tssrc/app/rendererPerfMetrics.tssrc/frontend/components/TrackMap/FlatTrackMapCanvas.tsxsrc/frontend/components/TrackMap/TrackCanvas.tsxsrc/frontend/components/TrackMap/trackDrawingUtils.tssrc/frontend/components/TrackMap/useProgressAnimation.spec.tsxsrc/frontend/components/TrackMap/useProgressAnimation.tssrc/frontend/utils/perfMetrics.tssrc/interface.d.tssrc/types/performance.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4fb9bbc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useLayoutEffect(() => { | ||
| const interpolator = interpolatorRef.current; | ||
| if (!interpolator) return; | ||
| drawRef.current(interpolator.getValues(), interpolator.getCount()); |
There was a problem hiding this comment.
Whenever drivers changes, the dependency-scoped layout effect already calls drawRef.current at line 192, after which this unconditional layout effect immediately invokes the same full-canvas paint again with identical interpolated values. At the normal 25 Hz snapshot cadence this adds 25 redundant unmeasured canvas redraws per second per map widget, while the RAF loop adds further paints; skip this second call when the target effect has already drawn, and ensure all remaining high-frequency paints are included in the R13.1 measurement.
AGENTS.md reference: AGENTS.md:L12-L18
Useful? React with 👍 / 👎.
| frameTimeMs: stats, | ||
| telemetryCallbackMs: callbackTimes.summarize(), | ||
| channelCallbackMs: channelTimes.summarize(), | ||
| trackMapAnimationFrameMs: trackMapFrameTimes.summarize(), |
There was a problem hiding this comment.
Although the RAF is now wrapped and this field is emitted into the raw renderer sample, tools/perf/analyze.ts never consumes trackMapAnimationFrameMs: its renderer summary and generated table still only report frame time and telemetry callbacks. Consequently normal perf:analyze output cannot display or compare the newly instrumented hot path, so the R13.1 observability requirement remains incomplete. This unchanged analyzer path is fresh evidence beyond the earlier instrumentation comment; add the metric to the analyzer summary/report and its tests.
AGENTS.md reference: AGENTS.md:L12-L18
Useful? React with 👍 / 👎.
| const onPitRoad = !!carIdxIsOnPitRoad?.[driver.CarIdx]; | ||
| if (onPitRoad) { | ||
| color = { fill: '#999999', text: 'white' }; |
There was a problem hiding this comment.
When the flat map contains any car on pit road, this branch constructs a new color object for that car on every animation frame; the inverted-leader branch above does the same when enabled. Moving this drawing code from telemetry updates into a display-refresh RAF turns those into per-frame allocations, contrary to R13.2 and likely to create avoidable garbage-collection pressure in full grids. Cache these colors or select scalar fill/text values without allocating objects inside the paint loop.
AGENTS.md reference: AGENTS.md:L12-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f36e01f3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Track-map animation frames / second | ${format(summary.renderer.trackMapAnimationFrameRateHz)} | | ||
| | Track-map animation-frame p99, mean / worst interval | ${format(summary.renderer.trackMapAnimationFrameP99MeanMs, 3)} / ${format(summary.renderer.trackMapAnimationFrameP99WorstMs, 3)} ms | |
There was a problem hiding this comment.
When perf:analyze is invoked with the documented --baseline option, writeAnalysis emits comparisonMarkdown, whose delta table never includes either of these new track-map metrics; these rows therefore appear only for single-run analysis and cannot show the before/after hot-path numbers needed to assess this change. The still-unchanged baseline comparison path is fresh evidence beyond the earlier analyzer comment; add the track-map timing values or deltas to PerfComparison and comparisonMarkdown.
AGENTS.md reference: AGENTS.md:L12-L18
Useful? React with 👍 / 👎.
| const m = ctx.measureText(displayText); | ||
| const visualOffset = | ||
| (m.actualBoundingBoxAscent - m.actualBoundingBoxDescent) / 2; |
There was a problem hiding this comment.
With the default showCarNumbers setting, moving this drawing callback onto RAF makes measureText allocate a TextMetrics object for every visible driver on every paint (and the curved map does the same in drawDrivers), producing thousands of short-lived objects per second for a full grid. This leaves a substantial per-frame allocation source in the newly introduced hot path despite R13.2; cache the vertical offset by font/display text and refresh it only when snapshot or appearance inputs change.
AGENTS.md reference: AGENTS.md:L12-L18
Useful? React with 👍 / 👎.
1 participant
Description
Smooths TrackMap and FlatTrackMap driver marker movement between the existing 25 Hz, 3-decimal track-state position snapshots.
The root cause was that both canvases painted only when React received a new positional snapshot, so the 60 Hz curated tape was presented as visible 40 ms steps. A widget-local imperative interpolator now follows the shortest wrapped lap-distance path, mutates reusable typed buffers, draws directly to each canvas, stops its requestAnimationFrame loop when settled, and cancels it on unmount. Driver colors, draw ordering, labels, pit state, player icons, settings, and the underlying positional channel remain unchanged. BlindSpotMonitor is untouched.
This is presentation-side follow-up to the Phase 4 architecture work in
docs/ARCHITECTURE_REVIEW.md: derived positional data remains on its declared 25 Hz channel, while renderer animation bridges snapshots without increasing channel or React update frequency.Performance evidence: the focused interpolation test advances 41 frames while asserting the same
Float64Arrayoutput identity on every frame. Driver collections and positioned driver objects are created only on incoming React snapshots; neither canvas creates driver arrays, maps, or cloned driver objects in its RAF draw path. The render-count test also proves RAF advancement does not trigger React renders.Validation:
npm run lint: passednpm run test:replay:curated: passednpm run irsdk:replay:app:curated: app launched, replay connected, all 40 drivers loaded, and Track Map rendered in the overlay editorArchitecture pre-PR checklist:
Screenshots
Before
Driver markers visibly advanced in discrete 25 Hz steps during the curated telemetry replay.
After
Driver markers move continuously between positional snapshots on both curved and flat maps.
Type of Change
Checklist
npm testnpm run lintand fixed any issuesSummary by CodeRabbit
New Features
Bug Fixes
Tests