[perf] Add throttled telemetry values hook and prevent driver standings memo from recalculation on each re-render - #603
Draft
pohy wants to merge 3 commits into
Draft
[perf] Add throttled telemetry values hook and prevent driver standings memo from recalculation on each re-render#603pohy wants to merge 3 commits into
pohy wants to merge 3 commits into
Conversation
CarIdxLapDistPct rounded to 3-4dp still recomputed Standings ~every 60Hz tick — with 24 cars on track, some car crosses any rounding threshold almost every frame, so value-based rounding never throttled the cadence. Added useTelemetryValuesThrottled (time-gated sampling, event-driven via setTimeout, no idle polling) and switched the Standings/Relative hot paths to it. Also lifted CarIdxLapDistPct/ CarIdxTrackSurface subscriptions to the top of each hook tree instead of each child hook (useDriverPositions, useCarState, useDriverLivePositions) resubscribing independently. Added a deterministic render-count regression test and a vitest bench comparing wall-clock cost: throttled is 75% fewer renders and ~1.5x faster under a simulated 24-car race.
Document the perf.spec.ts/bench.ts split established for the Standings throttle fix so future perf claims get a render-count test or a wall-clock bench instead of Task Manager eyeballing. CI runs npm run bench after tests, non-blocking since wall-clock numbers are noisy on shared runners. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Every call site passed the same 66ms, so make it the default and drop the redundant argument.
Contributor
📝 WalkthroughWalkthroughIntroduces ChangesThrottled Telemetry for Standings
Sequence Diagram(s)sequenceDiagram
participant TelemetryFeed as Telemetry Feed (60Hz)
participant Store as useTelemetryStore (Zustand)
participant Throttle as useTelemetryValuesThrottled
participant Standings as useDriverStandings
participant ChildHooks as useDriverPositions / useCarState / useDriverLivePositions
TelemetryFeed->>Store: update CarIdxLapDistPct + CarIdxTrackSurface each tick
Store-->>Throttle: notify subscriber
Throttle->>Throttle: schedule timeout if none pending (intervalMs=66)
Note over Throttle: intermediate ticks ignored
Throttle->>Store: read latest on timeout fire
Throttle-->>Standings: updated carIdxLapDistPct array
Standings->>ChildHooks: pass carIdxLapDistPct + carIdxTrackSurface as overrides
ChildHooks-->>Standings: computed positions / car state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Standings/hooks/useDriverLivePositions.tsx`:
- Around line 39-46: The useDriverLivePositions hook opens subscriptions for
carIdxLapDistPct and carIdxTrackSurface on lines 84 and 88 regardless of whether
override arrays are supplied, defeating the purpose of the optional parameters.
Modify the subscription logic to conditionally subscribe only when the
corresponding override is not provided, so that when a caller passes
carIdxLapDistPct or carIdxTrackSurface overrides, those local subscriptions are
skipped and the caller's already-subscribed arrays are used instead.
In `@src/frontend/components/Standings/hooks/useDriverPositions.tsx`:
- Around line 45-49: The useDriverPositions hook has unconditional telemetry
subscriptions that create duplicate listeners even when the carIdxLapDstPct
override is provided. To fix this, wrap the store subscriptions at the locations
mentioned in the comment (around lines 63 and 129) with conditional checks that
only execute these subscriptions if the corresponding override parameter
(carIdxLapDstPct) was not provided. When the override is provided, use that
value directly instead of subscribing, thereby eliminating the duplicate
listener issue.
In `@src/frontend/context/TelemetryStore/standingsThrottle.perf.spec.ts`:
- Around line 91-95: Replace the `console.log` call in the performance test with
`logger.info()` to align with established test patterns. Import the logger
utility from `src/frontend/utils/logger.ts` at the top of the
standingsThrottle.perf.spec.ts file, then replace the console.log statement with
logger.info() using the same message content, and remove the
eslint-disable-next-line comment since it will no longer be needed.
🪄 Autofix (Beta)
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: f0aa3b0c-254d-496f-851d-c9f2714f6083
📒 Files selected for processing (12)
.github/workflows/ci.ymlAGENTS.mdpackage.jsonsrc/frontend/components/Standings/hooks/useDriverLivePositions.tsxsrc/frontend/components/Standings/hooks/useDriverPositions.tsxsrc/frontend/components/Standings/hooks/useDriverRelatives.spec.tsxsrc/frontend/components/Standings/hooks/useDriverRelatives.tsxsrc/frontend/components/Standings/hooks/useDriverStandings.tsxsrc/frontend/context/TelemetryStore/TelemetryStore.spec.tsxsrc/frontend/context/TelemetryStore/TelemetryStore.tsxsrc/frontend/context/TelemetryStore/standingsThrottle.bench.tssrc/frontend/context/TelemetryStore/standingsThrottle.perf.spec.ts
Comment on lines
+39
to
+46
| carIdxLapDistPct: carIdxLapDistPctOverride, | ||
| carIdxTrackSurface: carIdxTrackSurfaceOverride, | ||
| }: { | ||
| enabled: boolean; | ||
| /** Pass an already-subscribed array to avoid a duplicate store subscription | ||
| * when the caller already holds this telemetry key. */ | ||
| carIdxLapDistPct?: number[]; | ||
| carIdxTrackSurface?: number[]; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
useDriverLivePositions still subscribes even when override arrays are supplied.
Line [84] and Line [88] always open local subscriptions, so the new override inputs do not actually eliminate duplicate listeners.
Also applies to: 84-90
🤖 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/Standings/hooks/useDriverLivePositions.tsx` around
lines 39 - 46, The useDriverLivePositions hook opens subscriptions for
carIdxLapDistPct and carIdxTrackSurface on lines 84 and 88 regardless of whether
override arrays are supplied, defeating the purpose of the optional parameters.
Modify the subscription logic to conditionally subscribe only when the
corresponding override is not provided, so that when a caller passes
carIdxLapDistPct or carIdxTrackSurface overrides, those local subscriptions are
skipped and the caller's already-subscribed arrays are used instead.
Comment on lines
+45
to
+49
| export const useDriverPositions = (overrides?: { | ||
| /** Pass an already-subscribed array to avoid a duplicate store subscription | ||
| * when the caller already holds this telemetry key. */ | ||
| carIdxLapDstPct?: number[]; | ||
| }) => { |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Override paths still create duplicate telemetry subscriptions.
Line [63] and Line [129] subscribe unconditionally, so passing overrides does not actually remove the duplicate listeners that these comments describe.
Refactor pattern to make deduplication real
- export const useDriverPositions = (overrides?: { carIdxLapDstPct?: number[] }) => {
- const ownCarIdxLapDstPct = useTelemetryValuesThrottled('CarIdxLapDistPct');
- const carIdxLapDstPct = overrides?.carIdxLapDstPct ?? ownCarIdxLapDstPct;
+ const useDriverPositionsBase = (carIdxLapDstPct: number[]) => {
// existing body
- }
+ };
+
+ export const useDriverPositions = () => {
+ const carIdxLapDstPct = useTelemetryValuesThrottled('CarIdxLapDistPct');
+ return useDriverPositionsBase(carIdxLapDstPct);
+ };
+
+ export const useDriverPositionsWithOverrides = (carIdxLapDstPct: number[]) =>
+ useDriverPositionsBase(carIdxLapDstPct);- export const useCarState = (overrides?: { carIdxTrackSurface?: ReturnType<typeof useTelemetry<number[]>> }) => {
- const ownCarIdxTrackSurface = useTelemetry('CarIdxTrackSurface');
- const carIdxTrackSurface = overrides?.carIdxTrackSurface ?? ownCarIdxTrackSurface;
+ const useCarStateBase = (
+ carIdxTrackSurface: ReturnType<typeof useTelemetry<number[]>>
+ ) => {
// existing body
- }
+ };Also applies to: 60-64, 124-131
🤖 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/Standings/hooks/useDriverPositions.tsx` around lines
45 - 49, The useDriverPositions hook has unconditional telemetry subscriptions
that create duplicate listeners even when the carIdxLapDstPct override is
provided. To fix this, wrap the store subscriptions at the locations mentioned
in the comment (around lines 63 and 129) with conditional checks that only
execute these subscriptions if the corresponding override parameter
(carIdxLapDstPct) was not provided. When the override is provided, use that
value directly instead of subscribing, thereby eliminating the duplicate
listener issue.
Comment on lines
+91
to
+95
| // eslint-disable-next-line no-console | ||
| console.log( | ||
| `[perf] ${TICK_COUNT} ticks @ 60Hz, ${CAR_COUNT} cars: ` + | ||
| `rounded(3dp)=${roundedTickRenders} renders, throttled(66ms)=${throttledTickRenders} renders` | ||
| ); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for test logger utilities or patterns
# Check for logger exports from utils
rg -n 'export.*logger' --type=ts src/frontend/utils/
# Check how other test files handle logging
rg -n 'console\.(log|warn|error)' --type=ts -g '*.spec.ts' -C2Repository: tariknz/irdashies
Length of output: 762
🏁 Script executed:
cat -n src/frontend/utils/logger.ts | head -50Repository: tariknz/irdashies
Length of output: 1341
🏁 Script executed:
rg -l '\.spec\.ts' src/frontend --type=ts | head -10 | xargs -I {} sh -c 'echo "File: {}" && rg "logger|console\.(log|warn|error)" {} -A1'Repository: tariknz/irdashies
Length of output: 132
🏁 Script executed:
rg 'import.*logger' --type=ts -g '*.spec.ts'Repository: tariknz/irdashies
Length of output: 235
🏁 Script executed:
head -40 src/frontend/context/TelemetryStore/standingsThrottle.perf.spec.tsRepository: tariknz/irdashies
Length of output: 1758
Use the frontend logger instead of console.log to align with established test patterns.
The logger utility at src/frontend/utils/logger.ts explicitly supports test environments by falling back to console when window.logBridge is unavailable—no IPC overhead in tests. Replace console.log with logger.info() and remove the eslint-disable comment. This pattern is already established in src/frontend/context/ReferenceLapStore/ReferenceLapStore.bench.spec.ts.
🤖 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/context/TelemetryStore/standingsThrottle.perf.spec.ts` around
lines 91 - 95, Replace the `console.log` call in the performance test with
`logger.info()` to align with established test patterns. Import the logger
utility from `src/frontend/utils/logger.ts` at the top of the
standingsThrottle.perf.spec.ts file, then replace the console.log statement with
logger.info() using the same message content, and remove the
eslint-disable-next-line comment since it will no longer be needed.
Source: Coding guidelines
pohy
marked this pull request as draft
June 22, 2026 22:25
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
1 participant
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.
Description
Plus add benchmark testing, not sure about integration into CI, tho.
Screenshots
Before
After
Type of Change
Checklist
npm testnpm run lintand fixed any issuesSummary by CodeRabbit
Release Notes
Tests
Chores
Documentation