Skip to content

fix: clear stale compare sync offsets on version switch - #238

Open
vandit-bera wants to merge 1 commit into
mainfrom
fix/182-compare-offset-reset
Open

fix: clear stale compare sync offsets on version switch#238
vandit-bera wants to merge 1 commit into
mainfrom
fix/182-compare-offset-reset

Conversation

@vandit-bera

Copy link
Copy Markdown

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.
  • Both onChange handlers on CompareVersionSelect now clear offA/offB whenever either side's version changes.

Test plan

  • Added two tests in compare-overlay.test.tsx asserting offA/offB are stripped from the URL after switching either pane's version.
  • npx vitest run components/review/compare/__tests__/compare-overlay.test.tsx — 26/26 pass
  • npx tsc --noEmit — clean

Fixes #182

🤖 Generated with Claude Code

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 ravirajsinh45 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compare overlay: switching a version does not reset per-side sync offsets (offA/offB), silently mis-syncing the new pair

2 participants