fix(desktop): keep the diff pane scrolled to the clicked file as content loads

Merged
AV
3mo ago
+103-18

Links

Summary

  • Fix the diff pane silently landing on the wrong (or a completely unrelated) file after clicking, when other diff content finishes loading after the initial scroll.
  • Fix the Review tab's "open in diff" opening the wrong source group for a path that exists in more than one (e.g. staged + against-base).

Why / Context

#6673 reports clicking a file in the Changes/Review sidebar landing on the wrong file, and notes it's hard to reproduce — it worked as soon as the reporter started recording. That's consistent with a race: the pane's files can span 1-3 getDiffBulk groups (staged/unstaged/against-base/commit) that resolve independently, plus async PR comment threads. With a warm cache everything resolves in one pass and looks fine; under real conditions (cold cache, an agent actively changing files, a big changeset) it doesn't.

How It Works

Three contributing causes, all fixed:

  1. useDiffCodeViewScroll — one-shot scroll latch. The scroll effect bailed once lastScrollTargetRef.current === scrollKey, and depended on items.length rather than items itself. So the first group to resolve triggered a scroll, then a later-resolving group landing above the target (or an async comment thread growing content above it) silently pushed the target out of view with no re-scroll. Replaced the one-shot latch with a sticky re-scroll: it keeps re-asserting scrollTo for the current click whenever items changes, until either the target settles or the user manually takes over scrolling (detected via a timing window around our own programmatic scrollTo calls, since @pierre/diffs doesn't expose a scroll-settled callback).
  2. useDiffCodeViewItems — bulk query key rotation. Each getDiffBulk query is keyed on that group's full paths array. An ordinary worktree change (agent adds/removes a file) rotates the key, and query.data goes undefined until the refetch lands — vanishing that group's items mid-session and, if it was the only group with data, unmounting CodeView entirely and losing scroll position. Cache the last successful response per DiffGroup.key (category + base/commit, independent of array position) so a rotated query still finds its group's prior content while it refetches.
  3. WorkspaceSidebar — Review tab missing changeKey. The Changes tab already disambiguates same-path files across source groups via a changeKey (getChangesetFileKey), but the Review tab's "open in diff" never passed one, so it fell back to "first item whose path matches" — landing in the wrong group whenever a path exists in more than one (e.g. a file with both staged edits and a pending PR comment against the base branch). Now builds the same changeKey once the workspace's base branch is known (PR review comments are inherently against-base); while it's still loading, changeKey is omitted so this falls back to the old (safe) path-match behavior instead of guessing.

Manual QA Checklist

  • Click a file in the Changes tab — lands on the correct file and stays there
  • Live-reproduced the original race and confirmed the fix (see Testing below)
  • Group-cardinality edge case (a category's file count going 1→0 mid-session) — no crash, no console errors
  • bun run typecheck clean
  • bash scripts/lint.sh clean on the changed files

Testing

  • bun run typecheck (desktop) — clean
  • bash scripts/lint.sh on the changed files — clean
  • Live CDP verification against a real dev build, with the actual before/after evidence gate:
    • Built a throwaway workspace with a real 26-file changeset spanning two source groups (25 unstaged files, 1 staged file).
    • Temporarily staggered the "unstaged" group's resolution (~2.5s artificial delay, removed before commit) to reliably reproduce the async race the issue describes as "hard to repro."
    • Before fix (git stash of useDiffCodeViewScroll.ts/DiffPane.tsx, delay still active): clicked the staged file, landed correctly while only that group had resolved; once the delayed unstaged group resolved and inserted 25 files above it, the viewport silently jumped to an unrelated file (preset-icons/index.ts) with no trace of the originally clicked file — reproducing the exact reported symptom.
    • After fix (same steps, fix restored): the same sequence kept the originally clicked file correctly in view once the delayed group resolved.
    • Re-verified the happy path with the artificial delay removed (real network/processing timing) — no regression.
    • renderHook-based unit tests for the scroll hook were considered (matching a regression test suggested in the issue thread) but this repo's Bun test environment doesn't have a real DOM (test-setup.ts stubs document minimally), so @testing-library/react's renderHook throws (TypeError: style is not an Object, confirmed empirically) — live verification was used instead.
  • Follow-up round (addressing CodeRabbit review): fresh workspace, fresh changeset (5 unstaged + 1 staged). Clicked the staged file — landed correctly with zero console errors. Then unstaged that file mid-session (the staged group's file count goes 1→0, the exact scenario the DiffGroup.key cache fix targets) — no crash, no console errors, pane correctly re-rendered the file under Unstaged.

Known Limitations

  • The "user took over scrolling" detection is a timing heuristic (a window after our own scrollTo calls), not an exact signal — @pierre/diffs' CodeView doesn't expose a scroll-settled/animation-complete callback. A user who manually scrolls during our own smooth-scroll animation (rare) may have that scroll ignored.

Summary by cubic

Keeps the diff pane anchored on the clicked file as diff groups and comments load, and opens the correct source group from the Review tab. Previously the pane could jump to a different file as later content arrived, and Review could open the wrong group for paths present in multiple sources.

  • useDiffCodeViewScroll: replaces the one-shot latch with a sticky re-scroll keyed to the current click; re-asserts scrollTo on items changes; stops re-scrolling when the user takes over (timed window around programmatic scrolls aligned with @pierre/diffs smooth scroll). Exposes notifyScroll; DiffPane wires it to CodeView onScroll.
  • useDiffCodeViewItems: replaces placeholderData: keepPreviousData with a cache keyed by each diff group's key, preserving that group’s content while its query refetches even when group positions shift. Prunes entries for removed groups. Avoids mismatches caused by useQueries index-based observer matching in @tanstack/react-query.
  • WorkspaceSidebar (Review tab): builds an against-base changeKey only after workspaceTrpc.git.getBaseBranch resolves; otherwise falls back to path-match. Passes changeKey to onSelectDiffFile to disambiguate same-path files across groups.

Summary by CodeRabbit

  • Bug Fixes
    • Review comments now open the correct files in the diff, including when staged or unstaged changes are present.
    • Diff content remains visible while switching files instead of briefly disappearing during loading.
    • Improved smooth scrolling and scroll restoration in diffs, reducing unwanted jumps and preserving the selected location.
    • Manual scrolling now reliably takes control after programmatic navigation.
    • Diff navigation now maintains the correct content when switching between groups or revisiting recently viewed changes.