Clicking a file to see the diff never lands on the file itself #6673

Activity

1. Nostromos

   added

   bugSomething isn't workingSomething isn't working

   on Aug 19, 2026
2. github-actions commented on Aug 19, 2026

   

   github-actionsBot

   on Aug 19, 2026 – with GitHub Actions

   Contributor

   More actions

   Clicking a changed file in the sidebar sends the right target — the wrongness happens after the scroll. The diff pane scrolls to the requested file exactly once, then permanently latches; anything that changes the item list above the target after that scroll (a second getDiffBulk group resolving, a bulk refetch whose query key rotated, PR comment threads arriving) shifts the content under a viewport that is never re-aligned. That fits the "hard to repro" character precisely: with a warm query cache and a quiet worktree everything resolves in one pass and the scroll is correct, which is what happened once you started recording.

   Affected code

   • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/hooks/useDiffCodeViewScroll/useDiffCodeViewScroll.ts — the scroll effect
   • .../DiffPane/hooks/useDiffCodeViewItems/useDiffCodeViewItems.ts — builds the CodeView item list
   • .../DiffPane/DiffPane.tsx — wires the two together, renders CodeView
   • apps/desktop/src/renderer/hooks/host-service/useGitStatus/useGitStatus.ts — invalidates diffs on git:changed
   • .../WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/FileRow/FileRow.tsx — the click source

   Suspected root cause

   The scroll is fire-and-forget and self-disables. useDiffCodeViewScroll.ts:92 bails when lastScrollTargetRef.current === scrollKey, and scrollKey is [targetItemId, focusLine, focusSide, focusTick] (:59-64) — constant for a given click. After the first successful scrollTo (:112-113) no further scroll happens for that click, no matter how the list changes. The effect also depends on items.length, not items (:120), so a same-length replacement (heights changed) doesn't even re-run it.

   Three things routinely change content above the target after that one scroll:

   1. Items appear late, in list order. useDiffCodeViewItems.ts:241-242 skips any file whose diff content hasn't loaded (if (!content) continue). A pane's files can span several getDiffBulk groups (staged / unstaged / against-base / commit — see the DiffGroup comment at :50-54). If a group listed above the target resolves second, its items are inserted above the target and push it down by their full height. The scroller has [overflow-anchor:none] (DiffPane.tsx:340), so the browser doesn't compensate.
   2. The bulk query key rotates on any worktree change. getDiffBulk is keyed on the whole paths array (useDiffCodeViewItems.ts:119-129), and git:changed invalidates every bulk query for the workspace (useGitStatus.ts:83). When an agent adds or removes a changed file, the path list changes → new query key → query.data is undefined → that group's items vanish → items.length === 0 makes DiffPane.tsx:305-315 return the "Loading…" placeholder and unmount CodeView entirely, losing scroll position. When items come back, the latch at useDiffCodeViewScroll.ts:92 refuses to re-scroll. This is the mechanism that best matches step 2 of your repro ("do some work").
   3. Comment threads arrive async and grow items. useDiffAnnotations.ts:60-77 polls getPullRequest/getPullRequestThreads (10s/30s intervals); each thread renders a variable-height CommentThread into the flow. Any thread on a file above the target grows the content after the scroll.

   Separate, deterministic wrong-file path: the Review tab's "open in diff" passes no changeKey (WorkspaceSidebar.tsx:114-120), so targetItemId falls back to "first item whose file path matches" (useDiffCodeViewScroll.ts:52-53). When the same path appears in two source groups (staged and unstaged, or against-base and a commit), that resolves to the wrong group's item. Same for diff panes persisted before change-key tracking.

   Ruled out: the sidebar identifies the file correctly. FileRow.tsx:81 computes getChangesetFileKey(file) and passes it through on every intent (:117-118), getChangesetFileKey is unambiguous across groups (changesetFileKey.ts:3-12), and useWorkspacePaneOpeners.ts:74-95 stores it on the pane. So this is a landing bug, not a targeting bug — except via the changeKey-less path above.

   I could not inspect @pierre/diffs (node_modules isn't present in this checkout), so I can't confirm whether scrollTo({type: "item"}) resolves offsets from measured or estimated row heights. If it estimates, that's a fourth contributor with the same symptom and the same fix.

   Reproduction

   Widen the window rather than chasing it:

   1. Open a workspace with many changed files spanning at least two source groups (some staged, some unstaged) so useDiffCodeViewItems builds more than one DiffGroup.
   2. Reload the renderer (⌘R) to empty the query cache, or switch the base branch to invalidate everything.
   3. Immediately click a file in the lower half of the Changes list. You should land above it, off by roughly the height of the group that resolved second.
   4. For mechanism 2: with an agent actively writing files, click a mid-list file repeatedly — each git:changed that alters the file set rotates the getDiffBulk key and drops you to the top.

   Regression test for the latch, no Electron needed:

   // useDiffCodeViewScroll.test.ts
   const scrollTo = mock();
   const codeViewRef = { current: { scrollTo, getInstance: () => ({}) } };
   const target = { id: "diff:unstaged:b.ts", type: "diff" };
   const earlier = { id: "diff:unstaged:a.ts", type: "diff" };
   
   // First pass: only the target's group has resolved.
   const { rerender } = renderHook(() => useDiffCodeViewScroll({ ...base, items: [target] }));
   expect(scrollTo).toHaveBeenCalledTimes(1);
   
   // Second group resolves and inserts an item ABOVE the target — same click,
   // same focusTick. Today this asserts 1; it should re-assert the target.
   rerender(() => useDiffCodeViewScroll({ ...base, items: [earlier, target] }));
   expect(scrollTo).toHaveBeenCalledTimes(2);

   Suggested fix direction

   Make the scroll a sticky intent instead of a one-shot:

   • Hold the target in a pendingTargetRef and re-assert scrollTo whenever items (identity, not .length) or measured heights change, until either the target's measured top matches the scroll position or the user scrolls by hand. Clearing on a real user gesture is the important part — handleScroll (DiffPane.tsx:173-179) currently can't distinguish user scrolls from programmatic ones, so it needs a "programmatic scroll in flight" flag.
   • Stop unmounting CodeView on transient empties: give the bulk queries placeholderData: keepPreviousData, or hold the last non-empty items in DiffPane.tsx:305.
   • Consider decoupling the getDiffBulk query key from the full paths array so ordinary worktree churn refetches in place instead of rotating the key.
   • Independently, thread changeKey through the Review tab's onOpenInDiff (WorkspaceSidebar.tsx:118), and make the path fallback at useDiffCodeViewScroll.ts:52-53 prefer the item whose source group matches the pane's current DiffRef rather than the first match.
3. Kitenite commented on Aug 19, 2026

   

   Kitenite

   on Aug 19, 2026

   Member

   More actions

   @AviPeltz for the change
4. AviPeltz

   added a commit that references this issue on Aug 20, 2026

   fix(desktop): keep the diff pane scrolled to the clicked file as cont…

   ...

   bffe46b
5. AviPeltz commented on Aug 20, 2026

   

   AviPeltz

   on Aug 20, 2026

   Collaborator

   More actions

   Fixed in #6679.

   Root cause: the diff pane's scroll was a one-shot — it scrolled to the clicked file once, then never re-corrected as more content resolved asynchronously (a later getDiffBulk group, a comment thread) and shifted the target out of view. The Review tab's "open in diff" also lacked a changeKey, so it could open the wrong source group for a path present in more than one (e.g. staged + against-base).

   Verified live against a real dev build with an actual before/after reproduction: with the old code, clicking a file could land correctly at first and then silently jump to an unrelated file once other content loaded; with the fix, it stays anchored on the clicked file. Details and testing evidence are in the PR.
6. AviPeltz

   closed this as completedon Aug 20, 2026