From 60479ec454419809f5f1e60f25a2a5d838a04f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Lehmann?= Date: Tue, 1 Sep 2026 18:37:10 +0200 Subject: [PATCH] fix: stop centering the viewport on mouse line clicks in diffs Every line click re-centered the focused line, breaking double-click word selection because the first click moved the content under the cursor. Focus changes now carry their source; only keyboard and programmatic focus keep the centering scroll, while mouse clicks never move the viewport. The inline comment form now scrolls itself fully into view on open with the smallest possible scroll, so clicking a line near the viewport bottom still shows the whole form. --- src/browser/components/pr-review.tsx | 33 +++++++++++++++----- src/browser/contexts/pr-review/index.test.ts | 12 +++++++ src/browser/contexts/pr-review/index.tsx | 22 ++++++++++++- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/src/browser/components/pr-review.tsx b/src/browser/components/pr-review.tsx index 01f80ff..99c3189 100644 --- a/src/browser/components/pr-review.tsx +++ b/src/browser/components/pr-review.tsx @@ -2494,7 +2494,7 @@ const DiffViewer = memo(function DiffViewer({ if (state.selectionAnchor === null) { store.setSelectionAnchor(state.focusedLine, side); } - store.setFocusedLine(lineNum, side); + store.setFocusedLine(lineNum, side, "mouse"); const startLine = Math.min(anchor, lineNum); const endLine = Math.max(anchor, lineNum); if (startLine !== endLine) { @@ -2510,7 +2510,7 @@ const DiffViewer = memo(function DiffViewer({ isDraggingRef.current = true; dragAnchorRef.current = lineNum; dragSideRef.current = side; - store.setFocusedLine(lineNum, side); + store.setFocusedLine(lineNum, side, "mouse"); store.setSelectionAnchor(lineNum, side); setIsDraggingState(true); }, @@ -2667,7 +2667,7 @@ const DiffViewer = memo(function DiffViewer({ } if (closestLine !== null) { - store.setFocusedLine(closestLine, dragSide); + store.setFocusedLine(closestLine, dragSide, "mouse"); } }; @@ -2702,6 +2702,7 @@ const DiffViewer = memo(function DiffViewer({ // Selection state for CSS-based highlighting (no per-row subscriptions) const focusedLine = usePRReviewSelector((s) => s.focusedLine); const focusedLineSide = usePRReviewSelector((s) => s.focusedLineSide); + const focusSource = usePRReviewSelector((s) => s.focusSource); const selectionAnchor = usePRReviewSelector((s) => s.selectionAnchor); const selectionAnchorSide = usePRReviewSelector((s) => s.selectionAnchorSide); @@ -2756,8 +2757,10 @@ const DiffViewer = memo(function DiffViewer({ } } - // 2. Scroll to focused line (after selection update) - if (focusedLine && !isDraggingState) { + // 2. Scroll to focused line (after selection update). Only for + // keyboard/programmatic focus — re-centering on mouse clicks breaks + // double-click word selection. + if (focusedLine && !isDraggingState && focusSource !== "mouse") { const rowIndex = getRowIndexForLine(focusedLine, focusedLineSide); if (rowIndex !== undefined) { virtualizer.scrollToIndex(rowIndex, { @@ -2775,6 +2778,7 @@ const DiffViewer = memo(function DiffViewer({ }, [ focusedLine, focusedLineSide, + focusSource, selectionAnchor, selectionAnchorSide, isDraggingState, @@ -3125,7 +3129,7 @@ const DiffLineRow = memo(function DiffLineRow({ state.focusedLineSide ?? lineSide ); } - store.setFocusedLine(lineNum, lineSide); + store.setFocusedLine(lineNum, lineSide, "mouse"); const startLine = Math.min(anchor, lineNum); const endLine = Math.max(anchor, lineNum); if (startLine !== endLine) { @@ -3154,7 +3158,7 @@ const DiffLineRow = memo(function DiffLineRow({ } // Normal click: focus the line (clear any selection) - store.setFocusedLine(lineNum, lineSide); + store.setFocusedLine(lineNum, lineSide, "mouse"); store.setSelectionAnchor(null, null); }, [lineNum, lineSide, store] @@ -3693,6 +3697,20 @@ const InlineCommentForm = memo(function InlineCommentForm({ textRef.current = text; const [submitting, setSubmitting] = useState(false); + const formRef = useRef(null); + + // Make sure the freshly opened form is fully visible with the smallest + // possible scroll (no re-centering). + useEffect(() => { + requestAnimationFrame(() => { + formRef.current?.scrollIntoView({ + block: "nearest", + behavior: "instant", + }); + }); + // Only on mount, when the form is opened + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); // Persist draft text when the form unmounts without submitting useEffect(() => { @@ -3799,6 +3817,7 @@ const InlineCommentForm = memo(function InlineCommentForm({ return (
{ const state = store.getSnapshot(); expect(state.focusedLine).toBe(42); expect(state.focusedLineSide).toBe("new"); + expect(state.focusSource).toBe("keyboard"); +}); + +test("setFocusedLine records mouse source", () => { + const store = createStore(); + store.selectFile("src/index.ts"); + + store.setFocusedLine(42, "new", "mouse"); + expect(store.getSnapshot().focusSource).toBe("mouse"); + + store.setFocusedLine(43, "new"); + expect(store.getSnapshot().focusSource).toBe("keyboard"); }); test("setFocusedLine clears skip block focus", () => { diff --git a/src/browser/contexts/pr-review/index.tsx b/src/browser/contexts/pr-review/index.tsx index 4f942b4..8e79878 100644 --- a/src/browser/contexts/pr-review/index.tsx +++ b/src/browser/contexts/pr-review/index.tsx @@ -314,6 +314,9 @@ interface PRReviewState { // Line selection focusedLine: number | null; focusedLineSide: "old" | "new" | null; // 'old' for delete lines, 'new' for insert/context + // How the current focus was set, so the UI can react differently (e.g. only + // keyboard focus gets scrolled to center) + focusSource: "mouse" | "keyboard"; selectionAnchor: number | null; selectionAnchorSide: "old" | "new" | null; focusedSkipBlockIndex: number | null; // Index of focused skip block for keyboard navigation @@ -613,6 +616,7 @@ export class PRReviewStore { ...initialState, files: sortedFiles, viewerCanMergeAsAdmin: false, + focusSource: "keyboard", // PR data (loaded separately) reviews: [], @@ -1890,11 +1894,13 @@ export class PRReviewStore { setFocusedLine = ( line: number | null, - side: "old" | "new" | null = "new" + side: "old" | "new" | null = "new", + source: "mouse" | "keyboard" = "keyboard" ) => { this.set({ focusedLine: line, focusedLineSide: line !== null ? side : null, + focusSource: source, focusedSkipBlockIndex: null, // Clear skip block focus when focusing a line }); }; @@ -2184,6 +2190,7 @@ export class PRReviewStore { this.set({ focusedLine: nextItem.lineNum, focusedLineSide: nextItem.side, + focusSource: "keyboard", focusedSkipBlockIndex: null, selectionAnchor: null, selectionAnchorSide: null, @@ -2231,6 +2238,7 @@ export class PRReviewStore { this.set({ focusedLine: nextNav.lineNum, focusedLineSide: nextNav.side, + focusSource: "keyboard", focusedPendingCommentId: null, focusedCommentId: null, selectionAnchor: null, @@ -2248,6 +2256,7 @@ export class PRReviewStore { this.set({ focusedLine: pendingLine, focusedLineSide: "new", + focusSource: "keyboard", focusedPendingCommentId: null, selectionAnchor: null, selectionAnchorSide: null, @@ -2284,6 +2293,7 @@ export class PRReviewStore { this.set({ focusedLine: nextNav.lineNum, focusedLineSide: nextNav.side, + focusSource: "keyboard", focusedCommentId: null, selectionAnchor: null, selectionAnchorSide: null, @@ -2311,6 +2321,7 @@ export class PRReviewStore { this.set({ focusedLine: commentLine, focusedLineSide: "new", + focusSource: "keyboard", focusedCommentId: null, selectionAnchor: null, selectionAnchorSide: null, @@ -2458,6 +2469,7 @@ export class PRReviewStore { focusedSkipBlockIndex: nextPair.skipIndex, focusedLine: null, focusedLineSide: null, + focusSource: "keyboard", selectionAnchor: null, selectionAnchorSide: null, focusedCommentId: null, @@ -2522,6 +2534,7 @@ export class PRReviewStore { this.set({ focusedLine: nextLine, focusedLineSide: nextSide, + focusSource: "keyboard", selectionAnchor: selectionAnchor ?? focusedLine ?? nextLine, selectionAnchorSide: selectionAnchorSide ?? focusedLineSide ?? nextSide, @@ -2533,6 +2546,7 @@ export class PRReviewStore { this.set({ focusedLine: nextLine, focusedLineSide: nextSide, + focusSource: "keyboard", selectionAnchor: null, selectionAnchorSide: null, focusedSkipBlockIndex: null, @@ -2629,6 +2643,7 @@ export class PRReviewStore { this.set({ focusedLine: nextLine, focusedLineSide: nextSide, + focusSource: "keyboard", selectionAnchor: selectionAnchor ?? focusedLine ?? nextLine, selectionAnchorSide: selectionAnchorSide ?? focusedLineSide ?? nextSide, focusedSkipBlockIndex: null, @@ -2639,6 +2654,7 @@ export class PRReviewStore { this.set({ focusedLine: nextLine, focusedLineSide: nextSide, + focusSource: "keyboard", selectionAnchor: null, selectionAnchorSide: null, focusedSkipBlockIndex: null, @@ -3039,6 +3055,7 @@ export class PRReviewStore { // Focus the line the comment was on so user can continue with keyboard focusedLine: commentLine ?? null, focusedLineSide: commentLine ? "new" : null, + focusSource: "keyboard", }); this.recomputeCommentRangeLookup(); }; @@ -3109,6 +3126,7 @@ export class PRReviewStore { // Focus the line the comment was on so user can continue with keyboard focusedLine: commentLine ?? null, focusedLineSide: commentLine ? "new" : null, + focusSource: "keyboard", }); }; @@ -3369,6 +3387,7 @@ export class PRReviewStore { this.set({ focusedLine: end, focusedLineSide: "new", + focusSource: "keyboard", selectionAnchor: start, selectionAnchorSide: "new", focusedSkipBlockIndex: null, @@ -3381,6 +3400,7 @@ export class PRReviewStore { this.set({ focusedLine: line, focusedLineSide: "new", + focusSource: "keyboard", selectionAnchor: null, selectionAnchorSide: null, focusedSkipBlockIndex: null,