From d677b9ee9d11d7f27c1175bc06158c3851ab92d3 Mon Sep 17 00:00:00 2001 From: Ammar Date: Tue, 2 Jun 2026 13:00:43 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20prevent=20immersive=20rev?= =?UTF-8?q?iew=20hunk=20layout=20flashes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stabilize Immersive Review hunk navigation by moving visual scroll and selection synchronization to layout effects and reserving a fixed assisted-callout row for files containing agent-flagged hunks. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `3820105{MUX_COSTS_USD:-0}`_ --- .../CodeReview/ImmersiveReviewView.test.tsx | 69 ++++++ .../CodeReview/ImmersiveReviewView.tsx | 215 ++++++++++++++---- 2 files changed, 236 insertions(+), 48 deletions(-) diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 7f9903e2a51..ea3bc090dd7 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx @@ -232,6 +232,75 @@ describe("ImmersiveReviewView", () => { expect(badge.textContent ?? "").toContain("2/3"); }); + test("reserves the assisted banner slot while iterating through hunks in one file", () => { + const assistedHunk = createHunk({ + id: "hunk-assisted", + filePath: "src/example.ts", + oldStart: 1, + oldLines: 1, + newStart: 1, + newLines: 1, + header: "@@ -1 +1 @@", + content: "-old assisted\n+new assisted", + }); + const regularHunk = createHunk({ + id: "hunk-regular", + filePath: assistedHunk.filePath, + oldStart: 12, + oldLines: 1, + newStart: 12, + newLines: 1, + header: "@@ -12 +12 @@", + content: "-old regular\n+new regular", + }); + const hunks = [assistedHunk, regularHunk]; + const assistedComment = "Inspect this change\nThe full rationale should remain available."; + const assistedHunkIds = new Set([assistedHunk.id]); + const assistedCommentByHunkId = new Map([[assistedHunk.id, assistedComment]]); + + const renderView = (selectedHunkId: string, visibleHunks: DiffHunk[] = hunks) => ( + + false} + onToggleRead={mock(() => undefined)} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={selectedHunkId} + onSelectHunk={mock(() => undefined)} + onExit={mock(() => undefined)} + isTouchImmersive={true} + reviewsByFilePath={new Map()} + firstSeenMap={{}} + assistedHunkIds={assistedHunkIds} + assistedCommentByHunkId={assistedCommentByHunkId} + /> + + ); + + const view = render(renderView(assistedHunk.id)); + + expect(view.container.querySelector('[data-assisted-banner-slot="true"]')).toBeTruthy(); + const banner = view.getByTestId("immersive-assisted-banner"); + expect(banner.textContent ?? "").toContain("Inspect this change"); + expect(banner.getAttribute("title")).toBe(assistedComment); + + view.rerender(renderView(regularHunk.id)); + + // The fixed slot remains mounted for same-file hunk iteration, but the + // selected-hunk callout content disappears when the plain hunk is selected. + expect(view.getByTestId("immersive-assisted-banner-slot")).toBeTruthy(); + expect(view.queryByTestId("immersive-assisted-banner")).toBeNull(); + + // Filters can hide the assisted hunk while the file remains active; reserve + // from allHunks so hide-read/search does not collapse the layout mid-file. + view.rerender(renderView(regularHunk.id, [regularHunk])); + expect(view.getByTestId("immersive-assisted-banner-slot")).toBeTruthy(); + expect(view.queryByTestId("immersive-assisted-banner")).toBeNull(); + }); + test("loads full-file context for an in-budget selected hunk even when another hunk is far away", async () => { const nearHunk = createHunk({ id: "hunk-near", diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx index ffdca2d3efc..b88550ebf18 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx @@ -459,6 +459,7 @@ export const ImmersiveReviewView: React.FC = (props) = selectedHunkId !== null ? (assistedCommentByHunkId?.get(selectedHunkId) ?? null) : null; const isSelectedAssisted = selectedHunkId !== null && (assistedHunkIds?.has(selectedHunkId) ?? false); + const selectedAssistedLabel = selectedAssistedComment ?? "Flagged by agent for review"; const isTouchExperience = isTouchImmersive === true; // Flatten file tree into ordered file list @@ -532,6 +533,9 @@ export const ImmersiveReviewView: React.FC = (props) = return null; }, [selectedHunkId, hunks, allHunks, fileList, isReviewComplete]); + const activeFilePathRef = useRef(null); + activeFilePathRef.current = activeFilePath; + const selectedHunkFromAll = useMemo( () => (selectedHunkId ? (allHunks.find((item) => item.id === selectedHunkId) ?? null) : null), [selectedHunkId, allHunks] @@ -563,6 +567,11 @@ export const ImmersiveReviewView: React.FC = (props) = return currentFileHunks[0] ?? null; }, [selectedHunkId, currentFileHunks]); + const shouldReserveAssistedBannerSlot = + assistedHunkIds != null && + activeFilePath != null && + getFileHunks(allHunks, activeFilePath).some((hunk) => assistedHunkIds.has(hunk.id)); + // Ensure we always have a selected hunk when the active file has hunks. useEffect(() => { if (currentFileHunks.length === 0) { @@ -598,7 +607,9 @@ export const ImmersiveReviewView: React.FC = (props) = ); // Hold diff reveal during file switches until the initial scroll is complete. - const [pendingRevealFilePath, setPendingRevealFilePath] = useState(null); + // Track the last revealed file instead of setting "pending" from an effect so + // a newly selected file is hidden on its first render rather than for one frame after paint. + const [revealedFilePath, setRevealedFilePath] = useState(null); const revealAnimationFrameRef = useRef(null); // Load full file context only when it is cheap. The hunk overlay remains visible @@ -817,21 +828,24 @@ export const ImmersiveReviewView: React.FC = (props) = // hide-read auto-advance without changing the main review panel's unread shortcut semantics. const readUndoStackRef = useRef([]); - useEffect(() => { + useLayoutEffect(() => { if (revealAnimationFrameRef.current !== null) { cancelAnimationFrame(revealAnimationFrameRef.current); revealAnimationFrameRef.current = null; } if (!activeFilePath) { - setPendingRevealFilePath(null); + setRevealedFilePath(null); return; } - // Keep the splash visible for each file switch until we have scrolled to the target hunk. - setPendingRevealFilePath(activeFilePath); - hunkJumpRef.current = true; - }, [activeFilePath]); + if (revealedFilePath !== activeFilePath) { + // Keep the splash visible for each file switch until we have scrolled to the target hunk. + // The pending state is derived during render so cross-file hunk iteration never flashes + // the new file at the old scroll position before this effect runs. + hunkJumpRef.current = true; + } + }, [activeFilePath, revealedFilePath]); useEffect(() => { return () => { @@ -843,21 +857,21 @@ export const ImmersiveReviewView: React.FC = (props) = const selectedHunkRevealTargetLineIndex = selectedHunkRange?.firstModifiedIndex ?? selectedHunkRange?.startIndex ?? null; - const isActiveFileRevealPending = pendingRevealFilePath === activeFilePath; + const isActiveFileRevealPending = activeFilePath !== null && revealedFilePath !== activeFilePath; const revealTargetLineIndex = isActiveFileRevealPending ? selectedHunkRevealTargetLineIndex : (activeLineIndex ?? selectedHunkRevealTargetLineIndex); const hasResolvedSelectedHunkForReveal = selectedHunkId !== null && currentFileHunks.some((hunk) => hunk.id === selectedHunkId); - useEffect(() => { + useLayoutEffect(() => { if (!isActiveFileRevealPending) { return; } // Fail open so the UI cannot get stuck if a file has no hunks. if (currentFileHunks.length === 0) { - setPendingRevealFilePath(null); + setRevealedFilePath(activeFilePath); return; } @@ -868,9 +882,10 @@ export const ImmersiveReviewView: React.FC = (props) = // Fail open once selection is stable if we still cannot resolve a reveal target. if (selectedHunkRevealTargetLineIndex === null) { - setPendingRevealFilePath(null); + setRevealedFilePath(activeFilePath); } }, [ + activeFilePath, currentFileHunks.length, hasResolvedSelectedHunkForReveal, isActiveFileRevealPending, @@ -940,6 +955,9 @@ export const ImmersiveReviewView: React.FC = (props) = const onSelectHunkRef = useRef(onSelectHunk); const allHunksRef = useRef(allHunks); const hunkLineRangesRef = useRef(overlayData.hunkLineRanges); + const previousSelectedHunkIdRef = useRef(null); + const previousSelectedHunkRangeRef = useRef(null); + const skipScrollUntilCursorSettlesRef = useRef(false); const highlightedLineElementRef = useRef(null); useEffect(() => { @@ -974,12 +992,19 @@ export const ImmersiveReviewView: React.FC = (props) = hunkLineRangesRef.current = overlayData.hunkLineRanges; }, [overlayData.hunkLineRanges]); - // Keep cursor and selection aligned to the selected hunk when hunk navigation changes. - useEffect(() => { + // Keep cursor and selection aligned to the selected hunk before paint so J/K hunk + // iteration does not flash the previous cursor/selection for a frame. + useLayoutEffect(() => { const resolvedSelectedHunkId = selectedHunk?.id ?? null; + const previousSelectedHunkId = previousSelectedHunkIdRef.current; + const previousSelectedHunkRange = previousSelectedHunkRangeRef.current; + + previousSelectedHunkIdRef.current = resolvedSelectedHunkId; + previousSelectedHunkRangeRef.current = selectedHunkRange; if (!selectedHunkRange || !resolvedSelectedHunkId) { pendingJumpSelectAllHunkIdRef.current = null; + skipScrollUntilCursorSettlesRef.current = false; setActiveLineIndex(null); setSelectedLineRange(null); return; @@ -991,6 +1016,7 @@ export const ImmersiveReviewView: React.FC = (props) = // Use actual modified boundaries (without context padding) for the highlight const modifiedStart = selectedHunkRange.firstModifiedIndex ?? selectedHunkRange.startIndex; const modifiedEnd = selectedHunkRange.lastModifiedIndex ?? selectedHunkRange.endIndex; + skipScrollUntilCursorSettlesRef.current = activeLineIndexRef.current !== modifiedEnd; setActiveLineIndex(modifiedEnd); setSelectedLineRange({ startIndex: modifiedStart, @@ -999,6 +1025,34 @@ export const ImmersiveReviewView: React.FC = (props) = return; } + const cursorLineIndex = activeLineIndexRef.current; + const cursorWasInPreviousSelectedHunk = Boolean( + cursorLineIndex !== null && + previousSelectedHunkRange && + cursorLineIndex >= previousSelectedHunkRange.startIndex && + cursorLineIndex <= previousSelectedHunkRange.endIndex + ); + const shouldPreserveContextCursor = Boolean( + cursorLineIndex !== null && + previousSelectedHunkRange && + previousSelectedHunkId === resolvedSelectedHunkId && + !cursorWasInPreviousSelectedHunk + ); + + if (shouldPreserveContextCursor) { + // Full-file context can arrive after the user has intentionally moved the + // cursor onto an unchanged/context row. Preserve that cursor instead of + // snapping back to the selected hunk just because overlay indices changed. + skipScrollUntilCursorSettlesRef.current = false; + return; + } + + skipScrollUntilCursorSettlesRef.current = Boolean( + cursorLineIndex !== null && + (cursorLineIndex < selectedHunkRange.startIndex || + cursorLineIndex > selectedHunkRange.endIndex) + ); + setActiveLineIndex((previousLineIndex) => { if ( previousLineIndex !== null && @@ -1132,6 +1186,19 @@ export const ImmersiveReviewView: React.FC = (props) = const targetHunkId = findReviewHunkId(review, fileHunks) ?? fileHunks[0].id; pendingJumpSelectAllHunkIdRef.current = null; hunkJumpRef.current = true; + const targetRange = + activeFilePath === review.data.filePath + ? (overlayData.hunkLineRanges.get(targetHunkId) ?? null) + : null; + if (targetRange) { + // Note/sidebar jumps are explicit hunk navigation, even when the note maps + // to the already-selected hunk. Reset any context-row cursor first so the + // centered jump lands on the note's hunk instead of a stale context line. + skipScrollUntilCursorSettlesRef.current = false; + setSelectedLineRange(null); + setActiveLineIndex(targetRange.firstModifiedIndex ?? targetRange.startIndex); + } + onSelectHunk(targetHunkId); // Force scroll effect to re-fire even when activeLineIndex is unchanged // (for example when the cursor is already inside the selected hunk). @@ -1145,7 +1212,13 @@ export const ImmersiveReviewView: React.FC = (props) = }); } }, - [allHunks, onSelectHunk, props.reviewActions?.onEditComment] + [ + activeFilePath, + allHunks, + onSelectHunk, + overlayData.hunkLineRanges, + props.reviewActions?.onEditComment, + ] ); const diffReviewActions = useMemo(() => { @@ -1624,8 +1697,10 @@ export const ImmersiveReviewView: React.FC = (props) = const previousContentRef = useRef(overlayData.content); // Keep the active line visible while moving with keyboard shortcuts, without - // forcing the full diff tree to re-render on every cursor move. - useEffect(() => { + // forcing the full diff tree to re-render on every cursor move. This is a layout + // effect because scroll/outline writes must happen before paint to avoid hunk + // navigation flashing at the previous viewport position. + useLayoutEffect(() => { const contentChanged = previousContentRef.current !== overlayData.content; previousContentRef.current = overlayData.content; @@ -1638,10 +1713,23 @@ export const ImmersiveReviewView: React.FC = (props) = // When overlay content structure changes (fallback hunks -> full-file view), // defer regular scrolling until the selected-hunk effect has recalculated - // activeLineIndex. During a file-switch reveal gate we still need one initial - // scroll so the diff appears already positioned at the selected hunk. + // activeLineIndex. Preserve ordinary context-row cursor movement: if the user + // is already outside the selected hunk and no hunk sync is pending, do not arm + // a center jump for the next scroll. if (contentChanged) { - hunkJumpRef.current = true; + const cursorIsInsideSelectedHunk = Boolean( + activeLineIndex !== null && + selectedHunkRange && + activeLineIndex >= selectedHunkRange.startIndex && + activeLineIndex <= selectedHunkRange.endIndex + ); + hunkJumpRef.current = Boolean( + isActiveFileRevealPending || + skipScrollUntilCursorSettlesRef.current || + activeLineIndex === null || + !selectedHunkRange || + cursorIsInsideSelectedHunk + ); if (!isActiveFileRevealPending) { return; } @@ -1652,6 +1740,26 @@ export const ImmersiveReviewView: React.FC = (props) = return; } + if (skipScrollUntilCursorSettlesRef.current) { + const cursorHasSettled = + isActiveFileRevealPending || + activeLineIndex === null || + !selectedHunkRange || + (activeLineIndex >= selectedHunkRange.startIndex && + activeLineIndex <= selectedHunkRange.endIndex); + + if (!cursorHasSettled) { + // A hunk jump renders once with the previous hunk's activeLineIndex before + // the selected-hunk layout effect commits the new cursor. Do not issue a + // stale scrollIntoView in that intermediate commit; the next layout pass + // will scroll directly to the selected hunk. Plain line-cursor movement to + // full-file context lines never sets this ref, so it still scrolls normally. + return; + } + + skipScrollUntilCursorSettlesRef.current = false; + } + const lineElement = containerRef.current?.querySelector( `[data-line-index="${lineIndexForScroll}"]` ); @@ -1666,8 +1774,8 @@ export const ImmersiveReviewView: React.FC = (props) = const revealFilePath = activeFilePath; revealAnimationFrameRef.current = window.requestAnimationFrame(() => { - setPendingRevealFilePath((pendingFilePath) => - pendingFilePath === revealFilePath ? null : pendingFilePath + setRevealedFilePath((currentRevealedFilePath) => + activeFilePathRef.current === revealFilePath ? revealFilePath : currentRevealedFilePath ); revealAnimationFrameRef.current = null; }); @@ -1697,8 +1805,8 @@ export const ImmersiveReviewView: React.FC = (props) = const revealFilePath = activeFilePath; revealAnimationFrameRef.current = window.requestAnimationFrame(() => { - setPendingRevealFilePath((pendingFilePath) => - pendingFilePath === revealFilePath ? null : pendingFilePath + setRevealedFilePath((currentRevealedFilePath) => + activeFilePathRef.current === revealFilePath ? revealFilePath : currentRevealedFilePath ); revealAnimationFrameRef.current = null; }); @@ -1709,6 +1817,7 @@ export const ImmersiveReviewView: React.FC = (props) = overlayData.content, revealTargetLineIndex, scrollNonce, + selectedHunkRange, ]); useEffect(() => { @@ -1965,36 +2074,46 @@ export const ImmersiveReviewView: React.FC = (props) = {/* Unified whole-file diff with hunk overlays + notes sidebar */}
- {/* Diff column. The assisted-review banner lives INSIDE this column (not + {/* Diff column. The assisted-review callout lives INSIDE this column (not above the whole body) so the agent's per-hunk comment spans only the diff width and lines up with the code it refers to — rather than stretching across the minimap and notes sidebar. */}
- {/* Assisted-review banner — surfaces the agent's flag + comment when - the selected hunk is one the agent pinned for review. Pinned to the - top of the diff column so the focus signal is impossible to miss - after entering immersive mode, where the side-panel cues aren't - visible. */} - {isSelectedAssisted && ( -
-