fix: clear stale compare sync offsets on version switch - #238
Conversation
offA/offB were calibrated for the previously-selected version pair. Switching either pane's version left them in place, silently misapplying an unrelated sync offset to the new pair and re-thrashing the decoder to hold a wrong alignment. Fixes #182
ravirajsinh45
left a comment
ravirajsinh45
left a comment
There was a problem hiding this comment.
Hey @vandit-bera, ran a deep review against the actual branch (with tests runnable) and found a couple of correctness gaps worth fixing before merge.
Re-selecting the current version wipes calibrated offsets. CompareVersionSelect has no guard against clicking the already-selected version, so reopening a dropdown and re-clicking the same item still fires onChange and deletes offA/offB even though the compared pair never changed.
Right-side switch has a state-race window. offA/offB clear via router.replace (deferred through React's startTransition), while setCurrentVersion is a synchronous Zustand write right after. That mismatch means a render can briefly show the new version with the stale offset still applied, before the URL update lands. The left-side handler doesn't have this because it bundles both changes into one writeParams call.
Test coverage: the two new tests only assert the URL string from the mocked router.replace, never the resulting state. Confirmed by removing setCurrentVersion(v) from handleSwitchRight entirely, both tests still passed.
Happy to pair on fixes if useful.
2 participants
Summary
offA/offB(per-side sync offsets) were calibrated for whichever version pair was being compared. Switching either pane's version via the compare dropdown left the old offsets in place, silently misapplying a stale sync correction to the new pair — and, per the issue, re-thrashing the decoder trying to hold a wrong alignment on mixed-fps comparisons.onChangehandlers onCompareVersionSelectnow clearoffA/offBwhenever either side's version changes.Test plan
compare-overlay.test.tsxassertingoffA/offBare stripped from the URL after switching either pane's version.npx vitest run components/review/compare/__tests__/compare-overlay.test.tsx— 26/26 passnpx tsc --noEmit— cleanFixes #182
🤖 Generated with Claude Code