From 0902ea12056fa0a8b88bd8e052b59ede39350345 Mon Sep 17 00:00:00 2001 From: Ammar Date: Fri, 5 Jun 2026 15:19:52 -0500 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20bench:=20profile=20immersive?= =?UTF-8?q?=20hunk=20iteration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a perf fixture and Electron profiling scenario that reproduces large-file immersive review hunk iteration across 60 J-key steps after the full-file diff is syntax highlighted. Optimize immersive hunk navigation to keep selection local while iterating, avoid per-step localStorage writes, and keep diff renderer callbacks stable so the highlighted diff tree is not re-rendered on each jump.\n\nOptimized highlighted baseline captured locally: 1,500-line file, 150 hunks, 1,650 rendered diff rows syntax-highlighted, 60 iterations, 300 ms wall time, 0.505 s TaskDuration, median 4.70 ms, p95 5.50 ms, max 12.60 ms.\n\n---\n\n_Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `unknown`_\n\n --- .../ImmersiveReviewAgentStatusBar.tsx | 21 +- .../CodeReview/ImmersiveReviewView.test.tsx | 258 ++++---- .../CodeReview/ImmersiveReviewView.tsx | 612 +++++++++++++++--- .../RightSidebar/CodeReview/ReviewPanel.tsx | 71 +- src/browser/features/Shared/DiffRenderer.tsx | 159 ++++- .../highlighting/highlightDiffChunk.test.ts | 70 +- .../utils/highlighting/highlightDiffChunk.ts | 203 ++++-- src/common/constants/storage.ts | 9 + .../perf.reviewHunkIteration.spec.ts | 246 +++++++ .../scenarios/perf.reviewMarkHunkRead.spec.ts | 18 +- tests/e2e/scenarios/reviewHydration.spec.ts | 383 +++++++++++ tests/e2e/utils/reviewPerfFixture.ts | 259 ++++++++ 12 files changed, 2005 insertions(+), 304 deletions(-) create mode 100644 tests/e2e/scenarios/perf.reviewHunkIteration.spec.ts create mode 100644 tests/e2e/scenarios/reviewHydration.spec.ts diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewAgentStatusBar.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewAgentStatusBar.tsx index 2ffc1c3308..4c734cce83 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewAgentStatusBar.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewAgentStatusBar.tsx @@ -55,28 +55,27 @@ export const ImmersiveReviewAgentStatusBar: React.FC + // Some unit tests mock only the store selectors they exercise; keep the + // immersive status bar crash-safe in those partial-store environments too. + typeof workspaceStore.hasRegisteredWorkspace === "function" && + workspaceStore.hasRegisteredWorkspace(workspaceId); const subscribe = (callback: () => void) => - workspaceStore.hasRegisteredWorkspace(workspaceId) - ? workspaceStore.subscribeKey(workspaceId, callback) - : () => undefined; + hasRegisteredWorkspace() ? workspaceStore.subscribeKey(workspaceId, callback) : () => undefined; const todos = useSyncExternalStore(subscribe, () => - workspaceStore.hasRegisteredWorkspace(workspaceId) - ? workspaceStore.getWorkspaceState(workspaceId).todos - : EMPTY_TODOS + hasRegisteredWorkspace() ? workspaceStore.getWorkspaceState(workspaceId).todos : EMPTY_TODOS ); const canInterrupt = useSyncExternalStore(subscribe, () => - workspaceStore.hasRegisteredWorkspace(workspaceId) - ? workspaceStore.getWorkspaceState(workspaceId).canInterrupt - : false + hasRegisteredWorkspace() ? workspaceStore.getWorkspaceState(workspaceId).canInterrupt : false ); // Sidebar derives `isStarting` directly from `isStreamStarting`. const isStarting = useSyncExternalStore(subscribe, () => - workspaceStore.hasRegisteredWorkspace(workspaceId) + hasRegisteredWorkspace() ? workspaceStore.getWorkspaceState(workspaceId).isStreamStarting : false ); const awaitingUserQuestion = useSyncExternalStore(subscribe, () => - workspaceStore.hasRegisteredWorkspace(workspaceId) + hasRegisteredWorkspace() ? workspaceStore.getWorkspaceState(workspaceId).awaitingUserQuestion : false ); diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 5090cbe243..03cc68c7ca 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; -import { act, cleanup, fireEvent, render, waitFor } from "@testing-library/react"; +import { cleanup, fireEvent, render, waitFor } from "@testing-library/react"; import { GlobalWindow } from "happy-dom"; import { useEffect, useState, type ComponentProps } from "react"; @@ -114,36 +114,6 @@ function renderImmersiveReview( ); } -function installManualAnimationFrame() { - let nextFrameId = 1; - const callbacks = new Map(); - - const requestAnimationFrameMock = mock((callback: FrameRequestCallback) => { - const frameId = nextFrameId; - nextFrameId += 1; - callbacks.set(frameId, callback); - return frameId; - }) as unknown as typeof globalThis.requestAnimationFrame; - const cancelAnimationFrameMock = mock((frameId: number) => { - callbacks.delete(frameId); - }) as unknown as typeof globalThis.cancelAnimationFrame; - - globalThis.requestAnimationFrame = requestAnimationFrameMock; - globalThis.cancelAnimationFrame = cancelAnimationFrameMock; - globalThis.window.requestAnimationFrame = requestAnimationFrameMock; - globalThis.window.cancelAnimationFrame = cancelAnimationFrameMock; - - return { - flush() { - const pendingCallbacks = Array.from(callbacks.values()); - callbacks.clear(); - for (const callback of pendingCallbacks) { - callback(performance.now()); - } - }, - }; -} - describe("ImmersiveReviewView", () => { let originalWindow: typeof globalThis.window; let originalDocument: typeof globalThis.document; @@ -197,102 +167,6 @@ describe("ImmersiveReviewView", () => { globalThis.cancelAnimationFrame = originalCancelAnimationFrame; }); - test("renders the hunk overlay while full-file context is still pending", async () => { - type ExecuteBashValue = Awaited>; - let resolveRead: ((value: ExecuteBashValue) => void) | undefined; - const pendingRead = new Promise((resolve) => { - resolveRead = resolve; - }); - mockApi.workspace.executeBash = mock(() => pendingRead); - - const view = renderImmersiveReview(); - - expect(view.container.textContent ?? "").toContain("new line"); - await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); - - if (!resolveRead) { - throw new Error("Read promise resolver was not captured"); - } - resolveRead({ - success: true, - data: { - success: true, - output: "0", - exitCode: 0, - }, - }); - }); - - test("keeps hydrated full-file context behind the reveal gate until positioned", async () => { - type ExecuteBashValue = Awaited>; - let resolveRead: ((value: ExecuteBashValue) => void) | undefined; - const pendingRead = new Promise((resolve) => { - resolveRead = resolve; - }); - mockApi.workspace.executeBash = mock(() => pendingRead); - const animationFrame = installManualAnimationFrame(); - - const hunk = createHunk({ - newStart: 3, - oldStart: 3, - header: "@@ -3 +3 @@", - content: "-old selected line\n+new selected line", - }); - - const view = renderImmersiveReview({ - fileTree: createFileTree(hunk.filePath), - hunks: [hunk], - allHunks: [hunk], - selectedHunkId: hunk.id, - isTouchImmersive: false, - }); - - await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); - expect(view.getByTestId("immersive-diff-reveal-overlay").textContent ?? "").toContain( - "Loading file" - ); - expect(view.getByTestId("immersive-diff-reveal-stage").className).toContain("invisible"); - - act(() => animationFrame.flush()); - await waitFor(() => expect(view.queryByTestId("immersive-diff-reveal-overlay")).toBeNull()); - expect(view.getByTestId("immersive-diff-reveal-stage").className).not.toContain("invisible"); - - const resolveLoadedContent = resolveRead; - if (!resolveLoadedContent) { - throw new Error("Read promise resolver was not captured"); - } - - act(() => { - resolveLoadedContent({ - success: true, - data: { - success: true, - output: encodeFileReadOutput( - ["context before selected 1", "context before selected 2", "new selected line"].join( - "\n" - ) - ), - exitCode: 0, - }, - }); - }); - - await waitFor(() => - expect(view.getByTestId("immersive-diff-reveal-overlay").textContent ?? "").toContain( - "Preparing diff" - ) - ); - expect(view.container.textContent ?? "").toContain("context before selected 1"); - expect(view.getByTestId("immersive-diff-reveal-stage").className).toContain("invisible"); - expect(view.getByTestId("immersive-minimap-reveal-stage").className).toContain("h-full"); - expect(view.getByTestId("immersive-minimap-reveal-stage").className).toContain("invisible"); - - act(() => animationFrame.flush()); - await waitFor(() => expect(view.queryByTestId("immersive-diff-reveal-overlay")).toBeNull()); - expect(view.getByTestId("immersive-diff-reveal-stage").className).not.toContain("invisible"); - expect(view.getByTestId("immersive-minimap-reveal-stage").className).not.toContain("invisible"); - }); - test("only preserves context cursors while overlay content is unchanged", () => { const previousRange = { startIndex: 0, endIndex: 1 }; @@ -604,6 +478,136 @@ describe("ImmersiveReviewView", () => { expect(view.getByText("No hunks for this file")).toBeTruthy(); }); + test("marking an unread hunk as read advances to the next hunk even when read hunks stay visible", async () => { + const firstHunk = createHunk({ + id: "hunk-first", + filePath: "src/example.ts", + newStart: 1, + newLines: 1, + oldStart: 1, + oldLines: 1, + header: "@@ -1 +1 @@", + content: "-old first\n+new first", + }); + const secondHunk = createHunk({ + id: "hunk-second", + filePath: "src/example.ts", + newStart: 3, + newLines: 1, + oldStart: 3, + oldLines: 1, + header: "@@ -3 +3 @@", + content: "-old second\n+new second", + }); + const onToggleRead = mock(() => undefined); + + const view = renderImmersiveReview({ + fileTree: createFileTree(firstHunk.filePath), + hunks: [firstHunk, secondHunk], + allHunks: [firstHunk, secondHunk], + selectedHunkId: firstHunk.id, + onToggleRead, + isTouchImmersive: false, + }); + + expect( + view.getByTestId("immersive-review-view").getAttribute("data-selected-hunk-position") + ).toBe("1"); + + const markReadButton = view.container.querySelector( + 'button[aria-label="Mark hunk as read"]' + ); + expect(markReadButton).toBeTruthy(); + fireEvent.click(markReadButton!); + + await waitFor(() => + expect( + view.getByTestId("immersive-review-view").getAttribute("data-selected-hunk-position") + ).toBe("2") + ); + expect(onToggleRead).toHaveBeenCalledWith(firstHunk.id); + }); + + test("marking a locally navigated hunk as read ignores the parent selection echo", async () => { + const firstHunk = createHunk({ + id: "hunk-first", + filePath: "src/example.ts", + newStart: 1, + oldStart: 1, + header: "@@ -1 +1 @@", + content: "-old first\n+new first", + }); + const secondHunk = createHunk({ + id: "hunk-second", + filePath: "src/example.ts", + newStart: 3, + oldStart: 3, + header: "@@ -3 +3 @@", + content: "-old second\n+new second", + }); + const thirdHunk = createHunk({ + id: "hunk-third", + filePath: "src/example.ts", + newStart: 5, + oldStart: 5, + header: "@@ -5 +5 @@", + content: "-old third\n+new third", + }); + const onToggleRead = mock(() => undefined); + + function ParentEchoHarness() { + const [parentSelectedHunkId, setParentSelectedHunkId] = useState(firstHunk.id); + + return ( + + false} + onToggleRead={onToggleRead} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={parentSelectedHunkId} + onSelectHunk={setParentSelectedHunkId} + onExit={mock(() => undefined)} + isTouchImmersive={false} + reviewsByFilePath={new Map()} + firstSeenMap={{}} + /> + + ); + } + + const view = render(); + + const findMarkReadButton = () => + view.container.querySelector('button[aria-label="Mark hunk as read"]'); + + const firstMarkReadButton = findMarkReadButton(); + expect(firstMarkReadButton).toBeTruthy(); + fireEvent.click(firstMarkReadButton!); + await waitFor(() => + expect( + view.getByTestId("immersive-review-view").getAttribute("data-selected-hunk-position") + ).toBe("2") + ); + + const secondMarkReadButton = findMarkReadButton(); + expect(secondMarkReadButton).toBeTruthy(); + fireEvent.click(secondMarkReadButton!); + + // The parent commits the just-read second hunk after the click. Keep the + // immersive-local work queue on the third hunk instead of replaying that echo. + await waitFor(() => + expect( + view.getByTestId("immersive-review-view").getAttribute("data-selected-hunk-position") + ).toBe("3") + ); + expect(onToggleRead).toHaveBeenCalledWith(firstHunk.id); + expect(onToggleRead).toHaveBeenCalledWith(secondHunk.id); + }); + test("clicking a sidebar review selects its hunk even when hidden by the active filter", () => { // Repro for: clicking a pending review in the immersive sidebar should // jump back to the hunk the review was attached to. Previously, when diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx index 6c14c6087a..23d86a1f14 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx @@ -5,6 +5,7 @@ */ import React, { useState, useCallback, useEffect, useLayoutEffect, useMemo, useRef } from "react"; +import { flushSync } from "react-dom"; import { ArrowLeft, Check, @@ -19,7 +20,8 @@ import { Trash2, } from "lucide-react"; import { cn } from "@/common/lib/utils"; -import { SelectableDiffRenderer } from "../../Shared/DiffRenderer"; +import { Skeleton } from "@/browser/components/Skeleton/Skeleton"; +import { preloadHighlightedDiff, SelectableDiffRenderer } from "../../Shared/DiffRenderer"; import { ImmersiveMinimap } from "./ImmersiveMinimap"; import { ImmersiveReviewAgentStatusBar } from "./ImmersiveReviewAgentStatusBar"; import { @@ -28,10 +30,13 @@ import { parseDiffLines, } from "./immersiveMinimapMath"; import { KeycapGroup } from "@/browser/components/Keycap/Keycap"; +import { useTheme } from "@/browser/contexts/ThemeContext"; import { useAPI } from "@/browser/contexts/API"; import { formatLineRangeCompact } from "@/browser/utils/review/lineRange"; import { findAdjacentFileHunkId, + findNextHunkId, + findNextHunkIdAfterFileRemoval, flattenFileTreeLeaves, getFileHunks, sortHunksInFileOrder, @@ -49,7 +54,9 @@ import { EXIT_CODE_TOO_MANY_LINES, processFileContents, } from "@/browser/utils/fileRead"; +import { updatePersistedState } from "@/browser/hooks/usePersistedState"; import { TooltipIfPresent } from "@/browser/components/Tooltip/Tooltip"; +import { getReviewSelectedHunkKey } from "@/common/constants/storage"; import { parseReviewLineRange, type DiffHunk, @@ -154,16 +161,97 @@ interface OverlayRevealIdentity { const MAX_FULL_FILE_CONTEXT_LINES = 1500; const MAX_FULL_FILE_CONTEXT_BYTES = 256 * 1024; +const FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS = 5_000; const LINE_JUMP_SIZE = 10; // Keep syntax highlighting on for larger review files now that per-line tooltip overhead is gone, // but still cap it to avoid pathological DOM costs on extremely large diffs. const MAX_HIGHLIGHTED_DIFF_LINES = 4000; const ACTIVE_LINE_OUTLINE = "1px solid hsl(from var(--color-review-accent) h s l / 0.45)"; +const HUNK_RANGE_OUTLINE_COLOR = "hsl(from var(--color-review-accent) h s l / 0.45)"; const LIKE_NOTE_PREFIX = "I like this change"; const DISLIKE_NOTE_PREFIX = "I don't like this change"; const EMPTY_REVIEWS: Review[] = []; const EMPTY_COMMENT_LINE_INDICES = new Set(); +const REVEAL_SKELETON_LINE_WIDTHS: readonly string[] = [ + "w-[72%]", + "w-[46%]", + "w-[88%]", + "w-[61%]", + "w-[80%]", + "w-[52%]", + "w-[70%]", + "w-[92%]", + "w-[44%]", + "w-[83%]", + "w-[58%]", + "w-[76%]", + "w-[49%]", + "w-[87%]", + "w-[65%]", + "w-[79%]", + "w-[54%]", + "w-[91%]", + "w-[68%]", + "w-[42%]", + "w-[74%]", + "w-[85%]", + "w-[57%]", + "w-[78%]", +]; + +function ImmersiveDiffRevealLoadingState(props: { label: string }) { + return ( +
+ {/* Match the transcript hydration shimmer instead of a centered spinner: the + diff-shaped placeholder keeps the hidden geometry swap feeling like code + is hydrating in place rather than a blocking modal flash. */} + +
+ {REVEAL_SKELETON_LINE_WIDTHS.map((width, rowIndex) => ( +
+ + +
+ ))} +
+
+ ); +} + +const FULL_FILE_CONTEXT_TIMEOUT = Symbol("full-file-context-timeout"); + +async function withFullFileContextTimeout( + promise: Promise +): Promise { + let timeoutId: ReturnType | null = null; + try { + return await Promise.race([ + promise, + new Promise((resolve) => { + timeoutId = setTimeout( + () => resolve(FULL_FILE_CONTEXT_TIMEOUT), + FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS + ); + }), + ]); + } finally { + if (timeoutId !== null) { + clearTimeout(timeoutId); + } + } +} + function getFileBaseName(filePath: string): string { const segments = filePath.split(/[\\/]/); return segments[segments.length - 1] || filePath; @@ -471,25 +559,48 @@ export const ImmersiveReviewView: React.FC = (props) = const containerRef = useRef(null); const scrollContainerRef = useRef(null); const notesSidebarRef = useRef(null); + const hunkJumpScrollBlockRef = useRef("center"); const hunkJumpRef = useRef(false); const pendingJumpSelectAllHunkIdRef = useRef(null); const { api } = useAPI(); + const { theme } = useTheme(); const { fileTree, hunks, allHunks, - selectedHunkId, - onSelectHunk, + selectedHunkId: externalSelectedHunkId, + onSelectHunk: commitSelectedHunk, onToggleRead, onMarkFileAsRead, - onExit, + onExit: commitExit, onReviewNote, isRead, isTouchImmersive, assistedHunkIds, assistedCommentByHunkId, } = props; + const selectedHunkStorageKey = getReviewSelectedHunkKey(props.workspaceId); + const [selectedHunkId, setSelectedHunkId] = useState(externalSelectedHunkId); + const externalSelectedHunkIdRef = useRef(externalSelectedHunkId); + const ignoredExternalSelectionEchoRef = useRef(null); + useEffect(() => { + externalSelectedHunkIdRef.current = externalSelectedHunkId; + if (ignoredExternalSelectionEchoRef.current === externalSelectedHunkId) { + ignoredExternalSelectionEchoRef.current = null; + return; + } + + setSelectedHunkId(externalSelectedHunkId); + }, [externalSelectedHunkId]); + const onSelectHunk = useCallback((hunkId: string | null) => { + setSelectedHunkId(hunkId); + }, []); + const onExit = useCallback(() => { + commitSelectedHunk(selectedHunkId); + commitExit(); + }, [commitExit, commitSelectedHunk, selectedHunkId]); + const selectedAssistedComment = selectedHunkId !== null ? (assistedCommentByHunkId?.get(selectedHunkId) ?? null) : null; const isSelectedAssisted = @@ -601,6 +712,11 @@ export const ImmersiveReviewView: React.FC = (props) = return currentFileHunks[0] ?? null; }, [selectedHunkId, currentFileHunks]); + const selectedHunkRef = useRef(selectedHunk); + useEffect(() => { + selectedHunkRef.current = selectedHunk; + }, [selectedHunk]); + const shouldReserveAssistedBannerSlot = assistedHunkIds != null && activeFilePath != null && @@ -640,15 +756,15 @@ export const ImmersiveReviewView: React.FC = (props) = [activeFilePath, currentFileHunks, props.workspaceId] ); - // Hold diff reveal until geometry-changing overlay swaps have been positioned. - // This covers file switches and same-file hydration from compact hunk overlays - // into full-file context, so the browser never paints an unanchored diff tree. + // Hold diff reveal until overlay geometry swaps have been positioned. File + // switches and same-file content swaps are covered by a scrollport-sized + // shimmer overlay while hidden layout effects scroll the target hunk into place. const [revealedOverlayIdentity, setRevealedOverlayIdentity] = useState(null); const revealAnimationFrameRef = useRef(null); - // Load full file context only when it is cheap. The hunk overlay remains visible - // while this request is pending so file switches do not block on bash/base64 I/O. + // Load full file context only when it is cheap. If disk I/O or highlighting stalls, + // fail open to the compact hunk overlay instead of trapping the review behind loading. useEffect(() => { const apiClient = api; const filePath = activeFilePath; @@ -674,6 +790,8 @@ export const ImmersiveReviewView: React.FC = (props) = const resolvedApi: NonNullable = apiClient; const resolvedFilePath: string = filePath; + const resolvedFileHunks = currentFileHunks; + const resolvedTheme = theme; let cancelled = false; setActiveFileContentState({ @@ -698,18 +816,25 @@ export const ImmersiveReviewView: React.FC = (props) = async function loadActiveFileContent() { // Keep plain file reads on the shared container root so immersive review can open // sibling-project files without forcing the primary repo checkout. - const fileResult = await resolvedApi.workspace.executeBash({ - workspaceId: props.workspaceId, - script: buildReadFileScript(resolvedFilePath, { - maxSizeBytes: MAX_FULL_FILE_CONTEXT_BYTES, - maxLineCount: MAX_FULL_FILE_CONTEXT_LINES, - }), - }); + const fileResult = await withFullFileContextTimeout( + resolvedApi.workspace.executeBash({ + workspaceId: props.workspaceId, + script: buildReadFileScript(resolvedFilePath, { + maxSizeBytes: MAX_FULL_FILE_CONTEXT_BYTES, + maxLineCount: MAX_FULL_FILE_CONTEXT_LINES, + }), + }) + ); if (cancelled) { return; } + if (fileResult === FULL_FILE_CONTEXT_TIMEOUT) { + settleLoadedContent(null, false); + return; + } + if (!fileResult.success) { settleLoadedContent(null, false); return; @@ -730,6 +855,29 @@ export const ImmersiveReviewView: React.FC = (props) = data.type === "text" && isWithinFullFileContextLineBudget(data.content) ? data.content : null; + + if (content != null) { + const hydratedOverlay = buildOverlayFromFileContent(content, resolvedFileHunks); + if (hydratedOverlay.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES) { + // Preload syntax tokens before swapping compact hunks to full-file context so + // users do not see plain fallback rows flash into colored Shiki spans. + const preloadResult = await withFullFileContextTimeout( + preloadHighlightedDiff({ + content: hydratedOverlay.content, + filePath: resolvedFilePath, + themeMode: resolvedTheme, + }) + ); + if (cancelled) { + return; + } + if (preloadResult === FULL_FILE_CONTEXT_TIMEOUT) { + settleLoadedContent(null, false); + return; + } + } + } + settleLoadedContent(content, content != null || isDeterministicBudgetMiss); } @@ -744,8 +892,10 @@ export const ImmersiveReviewView: React.FC = (props) = activeFileContentCacheKey, activeFilePath, api, + currentFileHunks, props.workspaceId, shouldLoadFullFileContext, + theme, ]); const isActiveFileContentSettled = @@ -772,16 +922,74 @@ export const ImmersiveReviewView: React.FC = (props) = return buildOverlayFromHunks(currentFileHunks); }, [resolvedActiveFileContent, currentFileHunks]); + const shouldEnableHighlighting = overlayData.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES; const activeOverlayRevealIdentity = useMemo( () => (activeFilePath ? { filePath: activeFilePath, content: overlayData.content } : null), [activeFilePath, overlayData.content] ); + const activeOverlayHighlightKey = activeOverlayRevealIdentity + ? `${activeOverlayRevealIdentity.filePath}\u0000${activeOverlayRevealIdentity.content}` + : null; + const [settledOverlayHighlightKey, setSettledOverlayHighlightKey] = useState(null); + const isActiveFileRevealPending = + activeFilePath !== null && revealedOverlayIdentity?.filePath !== activeFilePath; const isActiveOverlayRevealPending = activeOverlayRevealIdentity !== null && !isSameOverlayRevealIdentity(revealedOverlayIdentity, activeOverlayRevealIdentity); - const isActiveFileRevealPending = - activeFilePath !== null && revealedOverlayIdentity?.filePath !== activeFilePath; - const revealLoadingLabel = isActiveFileRevealPending ? "Loading file..." : "Preparing diff..."; + const isHappyDomEnvironment = typeof window !== "undefined" && "happyDOM" in window; + // Only worker-backed highlighting is safe to use as a reveal prerequisite; tests and + // other non-worker shells fall back to slower main-thread highlighting. + const canWaitForOverlayHighlight = typeof Worker !== "undefined" && !isHappyDomEnvironment; + const shouldWaitForFullFileContextReveal = Boolean( + isActiveFileRevealPending && shouldLoadFullFileContext && !isActiveFileContentSettled + ); + const shouldWaitForOverlayHighlight = Boolean( + canWaitForOverlayHighlight && + isActiveOverlayRevealPending && + shouldEnableHighlighting && + activeOverlayHighlightKey + ); + const isActiveOverlayHighlightReadyForReveal = + !shouldWaitForOverlayHighlight || settledOverlayHighlightKey === activeOverlayHighlightKey; + const isActiveOverlayReadyForReveal = + !shouldWaitForFullFileContextReveal && isActiveOverlayHighlightReadyForReveal; + // Gate every overlay geometry swap, not just file switches. Same-file hydration + // inserts context rows above the current hunk, so reveal only after the hidden + // layout pass scrolls the selected full-file row into place. + const isActiveOverlayReadyForRevealRef = useRef(isActiveOverlayReadyForReveal); + useLayoutEffect(() => { + isActiveOverlayReadyForRevealRef.current = isActiveOverlayReadyForReveal; + }, [isActiveOverlayReadyForReveal]); + const [revealOverlayHeight, setRevealOverlayHeight] = useState(null); + useLayoutEffect(() => { + if (!isActiveOverlayRevealPending) { + return; + } + + const scrollContainer = scrollContainerRef.current; + if (!scrollContainer) { + return; + } + + const updateRevealOverlayHeight = () => { + const nextHeight = Math.round(scrollContainer.clientHeight); + setRevealOverlayHeight((currentHeight) => + currentHeight === nextHeight ? currentHeight : nextHeight + ); + }; + + updateRevealOverlayHeight(); + const resizeObserver = + typeof ResizeObserver === "undefined" ? null : new ResizeObserver(updateRevealOverlayHeight); + resizeObserver?.observe(scrollContainer); + window.addEventListener("resize", updateRevealOverlayHeight); + + return () => { + resizeObserver?.disconnect(); + window.removeEventListener("resize", updateRevealOverlayHeight); + }; + }, [isActiveOverlayRevealPending]); + const revealLoadingLabel = "Loading file..."; const scheduleOverlayReveal = useCallback((overlayIdentity: OverlayRevealIdentity) => { if (revealAnimationFrameRef.current !== null) { @@ -789,15 +997,33 @@ export const ImmersiveReviewView: React.FC = (props) = } revealAnimationFrameRef.current = window.requestAnimationFrame(() => { - setRevealedOverlayIdentity((currentRevealedIdentity) => - isSameOverlayRevealIdentity(activeOverlayRevealIdentityRef.current, overlayIdentity) + setRevealedOverlayIdentity((currentRevealedIdentity) => { + // A stale rAF from an earlier compact overlay must not reveal while a + // newer full-file hydration is still loading/highlighting. + if (!isActiveOverlayReadyForRevealRef.current) { + return currentRevealedIdentity; + } + + return isSameOverlayRevealIdentity(activeOverlayRevealIdentityRef.current, overlayIdentity) ? overlayIdentity - : currentRevealedIdentity - ); + : currentRevealedIdentity; + }); revealAnimationFrameRef.current = null; }); }, []); + const handleDiffHighlightSettledChange = useCallback( + (isSettled: boolean) => { + setSettledOverlayHighlightKey((currentKey) => { + if (isSettled) { + return activeOverlayHighlightKey; + } + return currentKey === activeOverlayHighlightKey ? null : currentKey; + }); + }, + [activeOverlayHighlightKey] + ); + const selectedHunkRange = useMemo( () => (selectedHunk ? (overlayData.hunkLineRanges.get(selectedHunk.id) ?? null) : null), [selectedHunk, overlayData] @@ -890,6 +1116,11 @@ export const ImmersiveReviewView: React.FC = (props) = // hide-read auto-advance without changing the main review panel's unread shortcut semantics. const readUndoStackRef = useRef([]); + const setHunkJumpScroll = useCallback((block: ScrollLogicalPosition) => { + hunkJumpRef.current = true; + hunkJumpScrollBlockRef.current = block; + }, []); + useLayoutEffect(() => { if (revealAnimationFrameRef.current !== null) { cancelAnimationFrame(revealAnimationFrameRef.current); @@ -905,11 +1136,11 @@ export const ImmersiveReviewView: React.FC = (props) = if (isActiveOverlayRevealPending) { // The pending state is derived during render, not set from an effect, so - // file switches and same-file hydration swaps are hidden on their first - // paint until the scroll effect reveals the positioned overlay. - hunkJumpRef.current = true; + // file switches are hidden on their first paint until the scroll effect + // reveals the positioned overlay. + setHunkJumpScroll("center"); } - }, [activeOverlayRevealIdentity, isActiveOverlayRevealPending]); + }, [activeOverlayRevealIdentity, isActiveOverlayRevealPending, setHunkJumpScroll]); useEffect(() => { return () => { @@ -943,6 +1174,10 @@ export const ImmersiveReviewView: React.FC = (props) = return; } + if (!isActiveOverlayReadyForReveal) { + return; + } + // Fail open once selection is stable if we still cannot resolve a reveal target. if (selectedHunkRevealTargetLineIndex === null) { setRevealedOverlayIdentity(activeOverlayRevealIdentity); @@ -951,6 +1186,7 @@ export const ImmersiveReviewView: React.FC = (props) = activeOverlayRevealIdentity, currentFileHunks.length, hasResolvedSelectedHunkForReveal, + isActiveOverlayReadyForReveal, isActiveOverlayRevealPending, selectedHunkRevealTargetLineIndex, ]); @@ -1011,9 +1247,11 @@ export const ImmersiveReviewView: React.FC = (props) = // Refs keep hot-path callbacks stable so cursor movement doesn't trigger expensive re-renders. const activeLineIndexRef = useRef(null); + const hunkJumpLineRangeRef = useRef(null); const selectedLineRangeRef = useRef(null); const selectedHunkIdRef = useRef(selectedHunkId); const isReadRef = useRef(isRead); + const commitSelectedHunkRef = useRef(commitSelectedHunk); const onToggleReadRef = useRef(onToggleRead); const onSelectHunkRef = useRef(onSelectHunk); const allHunksRef = useRef(allHunks); @@ -1022,8 +1260,49 @@ export const ImmersiveReviewView: React.FC = (props) = const previousSelectedHunkIdRef = useRef(null); const previousSelectedHunkRangeRef = useRef(null); const skipScrollUntilCursorSettlesRef = useRef(false); + const hunkRangeLineElementsRef = useRef([]); const highlightedLineElementRef = useRef(null); + const clearHunkJumpRangeHighlight = useCallback(() => { + for (const lineElement of hunkRangeLineElementsRef.current) { + lineElement.dataset.selected = "false"; + lineElement.style.boxShadow = ""; + } + hunkRangeLineElementsRef.current = []; + }, []); + + const applyHunkJumpRangeHighlight = useCallback( + (range: SelectedLineRange) => { + clearHunkJumpRangeHighlight(); + const startIndex = Math.min(range.startIndex, range.endIndex); + const endIndex = Math.max(range.startIndex, range.endIndex); + const highlightedElements: HTMLElement[] = []; + + for (let lineIndex = startIndex; lineIndex <= endIndex; lineIndex += 1) { + const lineElement = containerRef.current?.querySelector( + `[data-line-index="${lineIndex}"]` + ); + if (!lineElement) { + continue; + } + + const edgeShadows = [ + `inset 1px 0 0 ${HUNK_RANGE_OUTLINE_COLOR}`, + `inset -1px 0 0 ${HUNK_RANGE_OUTLINE_COLOR}`, + lineIndex === startIndex ? `inset 0 1px 0 ${HUNK_RANGE_OUTLINE_COLOR}` : null, + lineIndex === endIndex ? `inset 0 -1px 0 ${HUNK_RANGE_OUTLINE_COLOR}` : null, + ].filter((shadow): shadow is string => Boolean(shadow)); + + lineElement.dataset.selected = "true"; + lineElement.style.boxShadow = edgeShadows.join(", "); + highlightedElements.push(lineElement); + } + + hunkRangeLineElementsRef.current = highlightedElements; + }, + [clearHunkJumpRangeHighlight] + ); + useEffect(() => { activeLineIndexRef.current = activeLineIndex; }, [activeLineIndex]); @@ -1036,10 +1315,23 @@ export const ImmersiveReviewView: React.FC = (props) = selectedHunkIdRef.current = selectedHunkId; }, [selectedHunkId]); + useEffect(() => { + return () => { + // Global immersive toggles or page teardown can unmount this view without + // calling onExit; persist directly instead of relying on parent cleanup. + updatePersistedState(selectedHunkStorageKey, selectedHunkIdRef.current); + commitSelectedHunkRef.current(selectedHunkIdRef.current); + }; + }, [selectedHunkStorageKey]); + useEffect(() => { isReadRef.current = isRead; }, [isRead]); + useEffect(() => { + commitSelectedHunkRef.current = commitSelectedHunk; + }, [commitSelectedHunk]); + useEffect(() => { onToggleReadRef.current = onToggleRead; }, [onToggleRead]); @@ -1070,6 +1362,8 @@ export const ImmersiveReviewView: React.FC = (props) = if (!selectedHunkRange || !resolvedSelectedHunkId) { pendingJumpSelectAllHunkIdRef.current = null; + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; skipScrollUntilCursorSettlesRef.current = false; setActiveLineIndex(null); setSelectedLineRange(null); @@ -1083,14 +1377,24 @@ export const ImmersiveReviewView: React.FC = (props) = const modifiedStart = selectedHunkRange.firstModifiedIndex ?? selectedHunkRange.startIndex; const modifiedEnd = selectedHunkRange.lastModifiedIndex ?? selectedHunkRange.endIndex; skipScrollUntilCursorSettlesRef.current = activeLineIndexRef.current !== modifiedEnd; - setActiveLineIndex(modifiedEnd); - setSelectedLineRange({ + hunkJumpLineRangeRef.current = { startIndex: modifiedStart, endIndex: modifiedEnd, - }); + }; + applyHunkJumpRangeHighlight(hunkJumpLineRangeRef.current); + setActiveLineIndex(modifiedEnd); + setSelectedLineRange(null); return; } + if ( + hunkJumpLineRangeRef.current && + !isSelectionInsideRange(hunkJumpLineRangeRef.current, selectedHunkRange) + ) { + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; + } + const cursorLineIndex = activeLineIndexRef.current; const shouldPreserveContextCursor = shouldPreserveImmersiveContextCursor({ cursorLineIndex, @@ -1146,6 +1450,8 @@ export const ImmersiveReviewView: React.FC = (props) = return null; }); }, [ + applyHunkJumpRangeHighlight, + clearHunkJumpRangeHighlight, overlayData.content, selectedHunk?.id, selectedHunkRange?.startIndex, @@ -1179,10 +1485,10 @@ export const ImmersiveReviewView: React.FC = (props) = } pendingJumpSelectAllHunkIdRef.current = null; - hunkJumpRef.current = true; + setHunkJumpScroll("center"); onSelectHunk(targetHunkId); }, - [activeFilePath, fileList, hunks, onSelectHunk] + [activeFilePath, fileList, hunks, onSelectHunk, setHunkJumpScroll] ); const navigateHunk = useCallback( @@ -1224,9 +1530,16 @@ export const ImmersiveReviewView: React.FC = (props) = } } + const targetHunk = (selectedHunkIsFilteredOut ? allHunks : hunks).find( + (hunk) => hunk.id === targetHunkId + ); pendingJumpSelectAllHunkIdRef.current = targetHunkId; - hunkJumpRef.current = true; - onSelectHunk(targetHunkId); + // Same-file J/K iteration should avoid re-centering every nearby hunk; + // nearest keeps the viewport anchored unless the target actually leaves view. + setHunkJumpScroll(targetHunk?.filePath === activeFilePath ? "nearest" : "center"); + // Keyboard hunk iteration should commit before the next key event so the + // browser can paint each step without waiting for React's default batching. + flushSync(() => onSelectHunk(targetHunkId)); }, [ activeFilePath, @@ -1236,6 +1549,7 @@ export const ImmersiveReviewView: React.FC = (props) = hunks, onSelectHunk, selectedHunkId, + setHunkJumpScroll, selectedHunkIsFilteredOut, ] ); @@ -1249,7 +1563,7 @@ export const ImmersiveReviewView: React.FC = (props) = const targetHunkId = findReviewHunkId(review, fileHunks) ?? fileHunks[0].id; pendingJumpSelectAllHunkIdRef.current = null; - hunkJumpRef.current = true; + setHunkJumpScroll("center"); const targetRange = activeFilePath === review.data.filePath ? (overlayData.hunkLineRanges.get(targetHunkId) ?? null) @@ -1264,6 +1578,7 @@ export const ImmersiveReviewView: React.FC = (props) = } onSelectHunk(targetHunkId); + commitSelectedHunk(targetHunkId); // Force scroll effect to re-fire even when activeLineIndex is unchanged // (for example when the cursor is already inside the selected hunk). setScrollNonce((previousNonce) => previousNonce + 1); @@ -1279,8 +1594,10 @@ export const ImmersiveReviewView: React.FC = (props) = [ activeFilePath, allHunks, + commitSelectedHunk, onSelectHunk, overlayData.hunkLineRanges, + setHunkJumpScroll, props.reviewActions?.onEditComment, ] ); @@ -1308,6 +1625,10 @@ export const ImmersiveReviewView: React.FC = (props) = return selectedLineRange; } + if (hunkJumpLineRangeRef.current) { + return hunkJumpLineRangeRef.current; + } + if (activeLineIndex === null) { return null; } @@ -1338,7 +1659,8 @@ export const ImmersiveReviewView: React.FC = (props) = Math.max(0, Math.min(lineCount - 1, lineIndex)); const selection = selectionOverride ?? - getCurrentLineSelection() ?? { + selectedLineRangeRef.current ?? + hunkJumpLineRangeRef.current ?? { startIndex: activeLineIndexRef.current ?? 0, endIndex: activeLineIndexRef.current ?? 0, }; @@ -1354,7 +1676,7 @@ export const ImmersiveReviewView: React.FC = (props) = const resolvedTarget = findHunkAtLine(cursorIndex, overlayData, currentFileHunks) ?? findHunkAtLine(effectiveSelection.startIndex, overlayData, currentFileHunks); - const targetHunk = resolvedTarget?.hunk ?? selectedHunk; + const targetHunk = resolvedTarget?.hunk ?? selectedHunkRef.current; if (!targetHunk) { return; } @@ -1385,7 +1707,7 @@ export const ImmersiveReviewView: React.FC = (props) = cursorIndex, }); }, - [getCurrentLineSelection, selectedHunk, overlayData, currentFileHunks, onSelectHunk] + [overlayData, currentFileHunks, onSelectHunk] ); const handleReviewNoteSubmit = useCallback( @@ -1397,19 +1719,23 @@ export const ImmersiveReviewView: React.FC = (props) = setInlineComposerRequest(null); // Clear the line selection so the next Shift+C targets the current keyboard // cursor (activeLineIndex) rather than the stale range from this comment. + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; setSelectedLineRange(null); containerRef.current?.focus(); }, - [onReviewNote] + [clearHunkJumpRangeHighlight, onReviewNote] ); const handleInlineComposerCancel = useCallback(() => { // Keep immersive parent state aligned with child composer teardown so canceled // keyboard-initiated requests do not linger or steal focus. setInlineComposerRequest(null); + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; setSelectedLineRange(null); containerRef.current?.focus(); - }, []); + }, [clearHunkJumpRangeHighlight]); const moveLineCursor = useCallback( (delta: number, extendRange: boolean) => { @@ -1421,6 +1747,8 @@ export const ImmersiveReviewView: React.FC = (props) = const currentIndex = activeLineIndexRef.current ?? selectedHunkRange?.startIndex ?? 0; const nextIndex = Math.max(0, Math.min(lineCount - 1, currentIndex + delta)); + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; setActiveLineIndex(nextIndex); if (extendRange) { @@ -1436,33 +1764,137 @@ export const ImmersiveReviewView: React.FC = (props) = onSelectHunk(lineHunkId); } }, - [overlayData.lineHunkIds, selectedHunkRange, onSelectHunk] + [clearHunkJumpRangeHighlight, overlayData.lineHunkIds, selectedHunkRange, onSelectHunk] ); - const resetViewCursorForHunk = useCallback((hunkId: string) => { - pendingJumpSelectAllHunkIdRef.current = null; - hunkJumpRef.current = true; - setSelectedLineRange(null); + const resetViewCursorForHunk = useCallback( + (hunkId: string) => { + pendingJumpSelectAllHunkIdRef.current = null; + setHunkJumpScroll("center"); + setSelectedLineRange(null); - if (selectedHunkIdRef.current === hunkId) { - const hunkRange = hunkLineRangesRef.current.get(hunkId) ?? null; - setActiveLineIndex(hunkRange?.firstModifiedIndex ?? hunkRange?.startIndex ?? null); - setScrollNonce((previousNonce) => previousNonce + 1); - } else { - setActiveLineIndex(null); - } + if (selectedHunkIdRef.current === hunkId) { + const hunkRange = hunkLineRangesRef.current.get(hunkId) ?? null; + setActiveLineIndex(hunkRange?.firstModifiedIndex ?? hunkRange?.startIndex ?? null); + setScrollNonce((previousNonce) => previousNonce + 1); + } else { + setActiveLineIndex(null); + } - onSelectHunkRef.current(hunkId); - }, []); + onSelectHunkRef.current(hunkId); + }, + [setHunkJumpScroll] + ); + + const getNextHunkAfterMarkRead = useCallback( + (hunkId: string) => { + const navigationHunks = selectedHunkIsFilteredOut ? allHunks : hunks; + const targetHunkId = findNextHunkId(navigationHunks, hunkId); + if (!targetHunkId) { + return null; + } - const handleToggleReadWithUndo = useCallback((hunkId: string) => { - const wasRead = isReadRef.current(hunkId); - readUndoStackRef.current = wasRead - ? readUndoStackRef.current.filter((trackedHunkId) => trackedHunkId !== hunkId) - : [...readUndoStackRef.current.filter((trackedHunkId) => trackedHunkId !== hunkId), hunkId]; - onToggleReadRef.current(hunkId); + return { + targetHunkId, + targetHunk: navigationHunks.find((hunk) => hunk.id === targetHunkId), + }; + }, + [allHunks, hunks, selectedHunkIsFilteredOut] + ); + + const selectNextHunkAfterMarkRead = useCallback( + (nextHunk: { targetHunkId: string; targetHunk: DiffHunk | undefined }) => { + pendingJumpSelectAllHunkIdRef.current = nextHunk.targetHunkId; + setHunkJumpScroll(nextHunk.targetHunk?.filePath === activeFilePath ? "nearest" : "center"); + flushSync(() => onSelectHunkRef.current(nextHunk.targetHunkId)); + }, + [activeFilePath, setHunkJumpScroll] + ); + + const commitSelectionForParentAction = useCallback((hunkId: string) => { + flushSync(() => commitSelectedHunkRef.current(hunkId)); }, []); + const getNextHunkAfterMarkFileRead = useCallback( + (hunkId: string) => { + const navigationHunks = selectedHunkIsFilteredOut ? allHunks : hunks; + const currentHunk = navigationHunks.find((hunk) => hunk.id === hunkId); + if (!currentHunk) { + return null; + } + + const targetHunkId = findNextHunkIdAfterFileRemoval( + navigationHunks, + hunkId, + currentHunk.filePath + ); + if (!targetHunkId) { + return null; + } + + return { + targetHunkId, + targetHunk: navigationHunks.find((hunk) => hunk.id === targetHunkId), + }; + }, + [allHunks, hunks, selectedHunkIsFilteredOut] + ); + + const selectNextHunkAfterMarkFileRead = useCallback( + (nextHunk: { targetHunkId: string; targetHunk: DiffHunk | undefined }) => { + pendingJumpSelectAllHunkIdRef.current = nextHunk.targetHunkId; + setHunkJumpScroll(nextHunk.targetHunk?.filePath === activeFilePath ? "nearest" : "center"); + flushSync(() => onSelectHunkRef.current(nextHunk.targetHunkId)); + }, + [activeFilePath, setHunkJumpScroll] + ); + + const handleMarkFileAsRead = useCallback( + (hunkId: string) => { + const nextHunkAfterFileRead = getNextHunkAfterMarkFileRead(hunkId); + if (nextHunkAfterFileRead && externalSelectedHunkIdRef.current !== hunkId) { + ignoredExternalSelectionEchoRef.current = hunkId; + } + + commitSelectionForParentAction(hunkId); + onMarkFileAsRead(hunkId); + if (nextHunkAfterFileRead) { + selectNextHunkAfterMarkFileRead(nextHunkAfterFileRead); + } + }, + [ + commitSelectionForParentAction, + getNextHunkAfterMarkFileRead, + onMarkFileAsRead, + selectNextHunkAfterMarkFileRead, + ] + ); + + const handleToggleReadWithUndo = useCallback( + (hunkId: string) => { + const wasRead = isReadRef.current(hunkId); + readUndoStackRef.current = wasRead + ? readUndoStackRef.current.filter((trackedHunkId) => trackedHunkId !== hunkId) + : [...readUndoStackRef.current.filter((trackedHunkId) => trackedHunkId !== hunkId), hunkId]; + const nextHunkAfterRead = wasRead ? null : getNextHunkAfterMarkRead(hunkId); + if (nextHunkAfterRead && externalSelectedHunkIdRef.current !== hunkId) { + // Parent selection is intentionally stale during hot immersive navigation. + // Ignore the parent echo for this committed read action so it cannot replay + // over the local work-queue advance to the next hunk. + ignoredExternalSelectionEchoRef.current = hunkId; + } + + commitSelectionForParentAction(hunkId); + onToggleReadRef.current(hunkId); + if (nextHunkAfterRead) { + // Immersive review is a keyboard-first work queue: marking a hunk read + // should advance even when the main panel is configured to keep read hunks visible. + selectNextHunkAfterMarkRead(nextHunkAfterRead); + } + }, + [commitSelectionForParentAction, getNextHunkAfterMarkRead, selectNextHunkAfterMarkRead] + ); + const handleUndoLastRead = useCallback(() => { while (readUndoStackRef.current.length > 0) { const targetHunkId = readUndoStackRef.current[readUndoStackRef.current.length - 1]; @@ -1489,6 +1921,8 @@ export const ImmersiveReviewView: React.FC = (props) = onSelectHunk(resolvedHunk.hunk.id); } + clearHunkJumpRangeHighlight(); + hunkJumpLineRangeRef.current = null; const anchorIndex = shiftKey ? (selectedLineRangeRef.current?.startIndex ?? activeLineIndexRef.current ?? lineIndex) : lineIndex; @@ -1513,7 +1947,14 @@ export const ImmersiveReviewView: React.FC = (props) = openComposer("", { startIndex: lineIndex, endIndex: lineIndex }); } }, - [overlayData, currentFileHunks, isTouchExperience, onSelectHunk, openComposer] + [ + clearHunkJumpRangeHighlight, + overlayData, + currentFileHunks, + isTouchExperience, + onSelectHunk, + openComposer, + ] ); const handleMinimapSelectLine = useCallback( @@ -1523,10 +1964,11 @@ export const ImmersiveReviewView: React.FC = (props) = onSelectHunk(hunkId); } + clearHunkJumpRangeHighlight(); setActiveLineIndex(lineIndex); setSelectedLineRange(null); }, - [overlayData.lineHunkIds, onSelectHunk] + [clearHunkJumpRangeHighlight, overlayData.lineHunkIds, onSelectHunk] ); // Auto-focus only for keyboard-first immersive mode. @@ -1717,7 +2159,9 @@ export const ImmersiveReviewView: React.FC = (props) = // since matchesKeybind for 'm' could match if shift isn't checked first if (matchesKeybind(e, KEYBINDS.MARK_FILE_READ)) { e.preventDefault(); - if (selectedHunkId) onMarkFileAsRead(selectedHunkId); + if (selectedHunkId) { + handleMarkFileAsRead(selectedHunkId); + } return; } @@ -1752,9 +2196,10 @@ export const ImmersiveReviewView: React.FC = (props) = moveLineCursor, openComposer, selectedHunkId, + commitSelectionForParentAction, handleToggleReadWithUndo, + handleMarkFileAsRead, handleUndoLastRead, - onMarkFileAsRead, isTouchExperience, ]); @@ -1847,14 +2292,19 @@ export const ImmersiveReviewView: React.FC = (props) = highlightedLineElementRef.current = lineElement; } - const block = hunkJumpRef.current ? "center" : "nearest"; + const block = hunkJumpRef.current ? hunkJumpScrollBlockRef.current : "nearest"; hunkJumpRef.current = false; + hunkJumpScrollBlockRef.current = "center"; lineElement.scrollIntoView({ behavior: "auto", block }); if (!isActiveOverlayRevealPending || !activeOverlayRevealIdentity) { return; } + if (!isActiveOverlayReadyForReveal) { + return; + } + if (!isTouchExperience) { // The minimap redraws from scrollTop; after a hidden hydration/file-swap // scroll, force one hidden redraw before the shared reveal gate opens. @@ -1864,6 +2314,7 @@ export const ImmersiveReviewView: React.FC = (props) = }, [ activeLineIndex, activeOverlayRevealIdentity, + isActiveOverlayReadyForReveal, isActiveOverlayRevealPending, isTouchExperience, overlayData.content, @@ -1931,13 +2382,13 @@ export const ImmersiveReviewView: React.FC = (props) = }; }, [inlineComposerRequest, overlayData.lineHunkIds.length, selectedHunk]); - const shouldEnableHighlighting = overlayData.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES; - return (
= 0 ? currentHunkIdx + 1 : undefined} + data-current-file-hunk-count={currentFileHunks.length} data-testid="immersive-review-view" > {/* Header */} @@ -2170,8 +2621,19 @@ export const ImmersiveReviewView: React.FC = (props) = {/* Avoid top padding here; it reads as a blank block between the controls and diff. */}
+ {isActiveOverlayRevealPending && currentFileHunks.length > 0 && !isReviewComplete && ( +
+
+ +
+
+ )} {props.isLoading && currentFileHunks.length === 0 ? (
Loading diff... @@ -2207,15 +2669,12 @@ export const ImmersiveReviewView: React.FC = (props) = {activeFilePath ? "No hunks for this file" : "No files to review"}
) : ( -
- {isActiveOverlayRevealPending && ( -
- {revealLoadingLabel} -
+
= (props) = fontSize="11px" maxHeight="none" className="rounded-none border-0 [&>div]:overflow-x-visible" + onHighlightSettledChange={handleDiffHighlightSettledChange} onReviewNote={handleReviewNoteSubmit} onComposerCancel={handleInlineComposerCancel} reviewActions={diffReviewActions} diff --git a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx index aa34b49510..1aa2302845 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx @@ -43,6 +43,7 @@ import { createPortal } from "react-dom"; import { HunkViewer } from "./HunkViewer"; import { InlineReviewNote, type ReviewActionCallbacks } from "../../Shared/InlineReviewNote"; import { ReviewControls } from "./ReviewControls"; +import { preloadHighlightedDiff } from "../../Shared/DiffRenderer"; import { ImmersiveReviewView } from "./ImmersiveReviewView"; import { FileTree } from "./FileTree"; import { UntrackedStatus } from "./UntrackedStatus"; @@ -53,7 +54,12 @@ import { repoRootBashOptions, resolveRepoRootProjectPath, } from "@/browser/utils/executeBash"; -import { readPersistedString, usePersistedState } from "@/browser/hooks/usePersistedState"; +import { + readPersistedState, + readPersistedString, + updatePersistedState, + usePersistedState, +} from "@/browser/hooks/usePersistedState"; import { STORAGE_KEYS, WORKSPACE_DEFAULTS } from "@/constants/workspaceDefaults"; import { useReviewState } from "@/browser/hooks/useReviewState"; import { useReviews } from "@/browser/hooks/useReviews"; @@ -67,6 +73,7 @@ import { parseDiff, extractAllHunks, buildGitDiffCommand } from "@/common/utils/ import { getReviewImmersiveKey, getReviewDefaultBaseKey, + getReviewSelectedHunkKey, getReviewSearchStateKey, REVIEW_INCLUDE_UNCOMMITTED_KEY, REVIEW_SORT_ORDER_KEY, @@ -93,6 +100,7 @@ import { import { applyFrontendFilters } from "@/browser/utils/review/filterHunks"; import { findNextHunkId, findNextHunkIdAfterFileRemoval } from "@/browser/utils/review/navigation"; import { cn } from "@/common/lib/utils"; +import { useTheme } from "@/browser/contexts/ThemeContext"; import { useAPI, type APIClient } from "@/browser/contexts/API"; import { useWorkspaceMetadata } from "@/browser/contexts/WorkspaceContext"; import { workspaceStore, useWorkspaceStoreRaw } from "@/browser/stores/WorkspaceStore"; @@ -784,19 +792,31 @@ export const ReviewPanel: React.FC = ({ }) => { const originFetchRef = useRef(null); const { api } = useAPI(); + const { theme } = useTheme(); const { workspaceMetadata } = useWorkspaceMetadata(); const panelRef = useRef(null); const scrollContainerRef = useRef(null); const searchInputRef = useRef(null); + useEffect(() => { + // Review is a code-heavy surface; warm the worker/highlighter during diff loading + // so immersive mode does not reveal plain text before Shiki is ready. + preloadHighlightedDiff({ + content: "+const muxReviewSyntaxWarmup = true;", + filePath: "review-warmup.ts", + themeMode: theme, + }).catch(() => undefined); + }, [theme]); + // Unified diff state - discriminated union makes invalid states unrepresentable // Note: Parent renders with key={workspaceId}, so component remounts on workspace change. const [diffState, setDiffState] = useState({ status: "loading" }); - // Persist selected hunk per workspace so navigation survives tab switches - const [selectedHunkId, setSelectedHunkId] = usePersistedState( - `review-selected-hunk:${workspaceId}`, - null + const selectedHunkStorageKey = getReviewSelectedHunkKey(workspaceId); + // Keep hunk selection local during navigation; persisting every J/K step writes + // localStorage synchronously and dominates large immersive-review iteration. + const [selectedHunkId, setSelectedHunkId] = useState(() => + readPersistedState(selectedHunkStorageKey, null) ); const [isLoadingTree, setIsLoadingTree] = useState(true); const [diagnosticInfo, setDiagnosticInfo] = useState(null); @@ -958,6 +978,16 @@ export const ReviewPanel: React.FC = ({ isReadRef.current = isRead; const selectedHunkIdRef = useRef(selectedHunkId); selectedHunkIdRef.current = selectedHunkId; + + useEffect(() => { + updatePersistedState(selectedHunkStorageKey, selectedHunkId); + }, [selectedHunkStorageKey, selectedHunkId]); + + useEffect(() => { + return () => { + updatePersistedState(selectedHunkStorageKey, selectedHunkIdRef.current); + }; + }, [selectedHunkStorageKey]); const showReadHunksRef = useRef(false); // Will be updated after filters state is declared // Track hunk first-seen timestamps for LIFO sorting @@ -1016,6 +1046,33 @@ export const ReviewPanel: React.FC = ({ [diffState] ); + const syntaxPrewarmHunk = useMemo(() => { + if (hunks.length === 0) { + return null; + } + + return ( + (selectedHunkId ? (hunks.find((hunk) => hunk.id === selectedHunkId) ?? null) : null) ?? + hunks[0] + ); + }, [hunks, selectedHunkId]); + + useEffect(() => { + if (!syntaxPrewarmHunk) { + return; + } + + // Start Shiki/language loading while the user is still in the review list so + // entering immersive review does not visibly repaint plain rows as colored tokens. + preloadHighlightedDiff({ + content: syntaxPrewarmHunk.content, + filePath: syntaxPrewarmHunk.filePath, + oldStart: syntaxPrewarmHunk.oldStart, + newStart: syntaxPrewarmHunk.newStart, + themeMode: theme, + }).catch(() => undefined); + }, [syntaxPrewarmHunk, theme]); + const orphanReviews = useMemo(() => { const diffFilePaths = new Set(); for (const hunk of hunks) { @@ -2033,14 +2090,14 @@ export const ReviewPanel: React.FC = ({ // with Assisted mode (which uses `assistedShowReadHunks`). Reading `filters.showReadHunks` // directly meant that marking a file read in Assisted mode left the selection on a now- // hidden hunk, breaking subsequent keyboard navigation when `currentIndex` became -1. - if (hunkId === selectedHunkId && !showReadHunksRef.current) { + if (hunkId === selectedHunkIdRef.current && !showReadHunksRef.current) { // Use ref to get current filtered/sorted list, then find next hunk not in same file setSelectedHunkId( findNextHunkIdAfterFileRemoval(filteredHunksRef.current, hunkId, hunk.filePath) ); } }, - [hunks, markAsRead, selectedHunkId, setSelectedHunkId] + [hunks, markAsRead, setSelectedHunkId] ); // Count agent-flagged hunks the user hasn't acked. The panel still reports diff --git a/src/browser/features/Shared/DiffRenderer.tsx b/src/browser/features/Shared/DiffRenderer.tsx index 58cfa26d56..54d5a647e2 100644 --- a/src/browser/features/Shared/DiffRenderer.tsx +++ b/src/browser/features/Shared/DiffRenderer.tsx @@ -17,7 +17,7 @@ import { groupDiffLines } from "@/browser/utils/highlighting/diffChunking"; import { useTheme, type ThemeMode } from "@/browser/contexts/ThemeContext"; import { escapeHtml, - highlightDiffChunk, + highlightDiffChunks, type HighlightedChunk, } from "@/browser/utils/highlighting/highlightDiffChunk"; import { LRUCache } from "lru-cache"; @@ -421,12 +421,10 @@ interface DiffRendererProps { /** * Module-level cache for fully-highlighted diff results. - * Key: `${content.length}:${oldStart}:${newStart}:${language}:${themeMode}` - * (Using content.length instead of full content as a fast differentiator - collisions are rare - * and just cause re-highlighting, not incorrect rendering) + * Key: `${contentHash}:${content.length}:${oldStart}:${newStart}:${language}:${themeMode}` * - * This allows synchronous cache hits, eliminating the "Processing" flash when - * re-rendering the same diff content (e.g., scrolling back to a previously-viewed message). + * This allows synchronous cache hits, avoiding visible plain-text-to-token-color + * swaps when immersive review preloads a full-file overlay before rendering it. */ const highlightedDiffCache = new LRUCache({ max: 10000, // High limit - rely on maxSize for eviction @@ -439,6 +437,8 @@ const highlightedDiffCache = new LRUCache({ ), }); +const highlightedDiffInFlight = new Map>(); + // Fast string hash (djb2 algorithm) - O(n) but very low constant factor function hashString(str: string): number { let hash = 5381; @@ -480,6 +480,77 @@ function createPlainTextChunks( })); } +interface HighlightedDiffRequest { + content: string; + language: string; + oldStart: number; + newStart: number; + themeMode: ThemeMode; +} + +async function loadHighlightedDiff(request: HighlightedDiffRequest): Promise { + const cacheKey = getDiffCacheKey( + request.content, + request.language, + request.oldStart, + request.newStart, + request.themeMode + ); + const cached = highlightedDiffCache.get(cacheKey); + if (cached) { + return cached; + } + + const inFlight = highlightedDiffInFlight.get(cacheKey); + if (inFlight) { + return inFlight; + } + + const highlightPromise = (async () => { + const lines = splitDiffLines(request.content); + const diffChunks = groupDiffLines(lines, request.oldStart, request.newStart); + const highlighted = await highlightDiffChunks(diffChunks, request.language, request.themeMode); + highlightedDiffCache.set(cacheKey, highlighted); + return highlighted; + })(); + + highlightedDiffInFlight.set(cacheKey, highlightPromise); + try { + return await highlightPromise; + } finally { + if (highlightedDiffInFlight.get(cacheKey) === highlightPromise) { + highlightedDiffInFlight.delete(cacheKey); + } + } +} + +export async function preloadHighlightedDiff(options: { + content: string; + filePath: string | null; + oldStart?: number; + newStart?: number; + themeMode: ThemeMode; +}): Promise { + const language = options.filePath ? getLanguageFromPath(options.filePath) : "text"; + await loadHighlightedDiff({ + content: options.content, + language, + oldStart: options.oldStart ?? 1, + newStart: options.newStart ?? 1, + themeMode: options.themeMode, + }); +} + +interface HighlightedDiffState { + cacheKey: string; + chunks: HighlightedChunk[]; + isSettled: boolean; +} + +function isPlainTextLanguage(language: string): boolean { + return language === "text" || language === "plaintext"; +} + /** * Hook to highlight diff content. Returns plain-text immediately, then upgrades * to syntax-highlighted when ready. Never returns null (no loading flash). @@ -490,52 +561,65 @@ function useHighlightedDiff( oldStart: number, newStart: number, themeMode: ThemeMode -): HighlightedChunk[] { +): HighlightedDiffState { const cacheKey = getDiffCacheKey(content, language, oldStart, newStart, themeMode); const cachedResult = highlightedDiffCache.get(cacheKey); + const isPlainText = isPlainTextLanguage(language); - // Sync fallback: plain-text chunks for instant render + // Sync fallback: plain-text chunks for instant render. const plainText = useMemo( () => createPlainTextChunks(content, oldStart, newStart), [content, oldStart, newStart] ); - const [chunks, setChunks] = useState(cachedResult ?? plainText); - const hasRealHighlightRef = React.useRef(false); + const [state, setState] = useState({ + cacheKey, + chunks: cachedResult ?? plainText, + isSettled: cachedResult != null || isPlainText, + }); useEffect(() => { const cached = highlightedDiffCache.get(cacheKey); if (cached) { - setChunks(cached); - if (language !== "text") hasRealHighlightRef.current = true; + setState({ cacheKey, chunks: cached, isSettled: true }); return; } - // Keep syntax-highlighted version when toggling to language="text" - if (language === "text" && hasRealHighlightRef.current) return; + if (isPlainText) { + setState({ cacheKey, chunks: plainText, isSettled: true }); + return; + } - // Show plain-text immediately, then upgrade async - setChunks(plainText); + // Show plain text immediately unless a caller preloaded this diff into the cache. + setState({ cacheKey, chunks: plainText, isSettled: false }); let cancelled = false; - void (async () => { - const lines = splitDiffLines(content); - const diffChunks = groupDiffLines(lines, oldStart, newStart); - const highlighted = await Promise.all( - diffChunks.map((chunk) => highlightDiffChunk(chunk, language, themeMode)) - ); - if (!cancelled) { - highlightedDiffCache.set(cacheKey, highlighted); - setChunks(highlighted); - if (language !== "text") hasRealHighlightRef.current = true; - } - })(); + loadHighlightedDiff({ content, language, oldStart, newStart, themeMode }) + .then((highlighted) => { + if (!cancelled) { + setState({ cacheKey, chunks: highlighted, isSettled: true }); + } + }) + .catch(() => { + if (!cancelled) { + setState({ cacheKey, chunks: plainText, isSettled: true }); + } + }); + return () => { cancelled = true; }; - }, [cacheKey, content, language, oldStart, newStart, themeMode, plainText]); + }, [cacheKey, content, isPlainText, language, oldStart, newStart, themeMode, plainText]); + + if (cachedResult) { + return { cacheKey, chunks: cachedResult, isSettled: true }; + } - return cachedResult ?? chunks; + if (state.cacheKey === cacheKey) { + return state; + } + + return { cacheKey, chunks: plainText, isSettled: isPlainText }; } /** @@ -565,10 +649,11 @@ export const DiffRenderer: React.FC = ({ [filePath] ); - const highlightedChunks = useHighlightedDiff(content, language, oldStart, newStart, theme); + const highlightedDiff = useHighlightedDiff(content, language, oldStart, newStart, theme); + const highlightedChunks = highlightedDiff.chunks; const lineNumberWidths = React.useMemo(() => { - if (!showLineNumbers || !highlightedChunks) { + if (!showLineNumbers) { return { oldWidthCh: 2, newWidthCh: 2 }; } // Flatten chunks and map HighlightedLine property names to common interface @@ -640,6 +725,8 @@ interface SelectableDiffRendererProps extends Omit void; /** Callback when review note composition state changes (selection active/inactive) */ onComposingChange?: (isComposing: boolean) => void; /** Action callbacks for inline review notes (edit, check, delete, etc.) */ @@ -984,6 +1071,7 @@ export const SelectableDiffRenderer = React.memo( onLineClick, searchConfig, enableHighlighting = true, + onHighlightSettledChange, onComposingChange, reviewActions, activeLineIndex, @@ -1215,13 +1303,18 @@ export const SelectableDiffRenderer = React.memo( ); // Only highlight if enabled (for viewport optimization) - const highlightedChunks = useHighlightedDiff( + const highlightedDiff = useHighlightedDiff( content, enableHighlighting ? language : "text", oldStart, newStart, theme ); + const highlightedChunks = highlightedDiff.chunks; + + React.useEffect(() => { + onHighlightSettledChange?.(highlightedDiff.isSettled); + }, [highlightedDiff.isSettled, onHighlightSettledChange]); // Parse raw lines once for use in lineData const rawLines = React.useMemo(() => splitDiffLines(content), [content]); diff --git a/src/browser/utils/highlighting/highlightDiffChunk.test.ts b/src/browser/utils/highlighting/highlightDiffChunk.test.ts index bbd027781a..46572fd71b 100644 --- a/src/browser/utils/highlighting/highlightDiffChunk.test.ts +++ b/src/browser/utils/highlighting/highlightDiffChunk.test.ts @@ -1,4 +1,8 @@ -import { highlightDiffChunk, isWithinDiffHighlightSyncBudget } from "./highlightDiffChunk"; +import { + highlightDiffChunk, + highlightDiffChunks, + isWithinDiffHighlightSyncBudget, +} from "./highlightDiffChunk"; import type { DiffChunk } from "./diffChunking"; /** @@ -198,6 +202,70 @@ describe("highlightDiffChunk", () => { expect(result.usedFallback).toBe(false); }); + it("should preserve chunk metadata when highlighting multiple chunks together", async () => { + const chunks: DiffChunk[] = [ + { + type: "context", + lines: ["export function value() {"], + startIndex: 0, + oldLineNumbers: [1], + newLineNumbers: [1], + }, + { + type: "remove", + lines: [' return "pending";'], + startIndex: 1, + oldLineNumbers: [2], + newLineNumbers: [null], + }, + { + type: "add", + lines: [' return "ready";', "}"], + startIndex: 2, + oldLineNumbers: [null, 3], + newLineNumbers: [2, 3], + }, + ]; + + const result = await highlightDiffChunks(chunks, "typescript"); + + expect(result.map((chunk) => chunk.type)).toEqual(["context", "remove", "add"]); + expect(result.every((chunk) => chunk.usedFallback === false)).toBe(true); + expect(result[0].lines[0].oldLineNumber).toBe(1); + expect(result[1].lines[0].newLineNumber).toBeNull(); + expect(result[2].lines[0].originalIndex).toBe(2); + expect(result[2].lines[1].html).toContain("}"); + expect( + result.flatMap((chunk) => chunk.lines).every((line) => line.html.includes(" { + const removedChunk: DiffChunk = { + type: "remove", + lines: ["/* removed version starts a comment"], + startIndex: 0, + oldLineNumbers: [1], + newLineNumbers: [null], + }; + const addedChunk: DiffChunk = { + type: "add", + lines: ["const added = 1;"], + startIndex: 1, + oldLineNumbers: [null], + newLineNumbers: [1], + }; + + const highlightedTogether = await highlightDiffChunks( + [removedChunk, addedChunk], + "typescript" + ); + const highlightedAddedAlone = await highlightDiffChunk(addedChunk, "typescript"); + + expect(highlightedTogether[1].lines[0].html).toBe(highlightedAddedAlone.lines[0].html); + expect(highlightedTogether[1].lines[0].html).toContain("const"); + }); + it("should handle incomplete syntax (unclosed string)", async () => { const chunk: DiffChunk = { type: "add", diff --git a/src/browser/utils/highlighting/highlightDiffChunk.ts b/src/browser/utils/highlighting/highlightDiffChunk.ts index 435a7ab302..3ff6b1c38e 100644 --- a/src/browser/utils/highlighting/highlightDiffChunk.ts +++ b/src/browser/utils/highlighting/highlightDiffChunk.ts @@ -1,5 +1,6 @@ import { highlightCode } from "./highlightWorkerClient"; import { isLightThemeMode } from "./shiki-shared"; +import type { ThemeMode } from "@/browser/contexts/ThemeContext"; import type { DiffChunk } from "./diffChunking"; /** @@ -45,8 +46,6 @@ export interface HighlightedLine { originalIndex: number; // Index in original diff } -import type { ThemeMode } from "@/browser/contexts/ThemeContext"; - export interface HighlightedChunk { type: DiffChunk["type"]; lines: HighlightedLine[]; @@ -54,77 +53,191 @@ export interface HighlightedChunk { } /** - * Highlight a chunk of code using Shiki - * Falls back to plain text on error + * Highlight a chunk of code using Shiki. + * Falls back to plain text on error. */ export async function highlightDiffChunk( chunk: DiffChunk, language: string, themeMode: ThemeMode = "dark" ): Promise { - // Fast path: no highlighting for text files - if (language === "text" || language === "plaintext") { - return { - type: chunk.type, - lines: chunk.lines.map((line, i) => ({ - html: escapeHtml(line), - oldLineNumber: chunk.oldLineNumbers[i], - newLineNumber: chunk.newLineNumbers[i], - originalIndex: chunk.startIndex + i, - })), - usedFallback: false, - }; - } + const [highlighted] = await highlightDiffChunks([chunk], language, themeMode); + return highlighted ?? createFallbackChunk(chunk); +} + +interface DiffLineRef { + chunkIndex: number; + lineIndex: number; +} - if (!isWithinDiffHighlightSyncBudget(chunk.lines)) { - return createFallbackChunk(chunk); +interface DiffHighlightSegment { + lines: string[]; + refs: DiffLineRef[]; + lastLineNumber: number | null; +} + +function appendSegmentLine( + segments: DiffHighlightSegment[], + lineNumber: number, + line: string, + ref: DiffLineRef +): void { + const currentSegment = segments[segments.length - 1]; + if (currentSegment?.lastLineNumber == null || lineNumber !== currentSegment.lastLineNumber + 1) { + segments.push({ lines: [], refs: [], lastLineNumber: null }); } - const code = chunk.lines.join("\n"); - const workerTheme = isLightThemeMode(themeMode) ? "light" : "dark"; + const targetSegment = segments[segments.length - 1]; + targetSegment.lines.push(line); + targetSegment.refs.push(ref); + targetSegment.lastLineNumber = lineNumber; +} - try { - // Highlight via worker (cached, off main thread) - const html = await highlightCode(code, language, workerTheme); +function buildVersionedHighlightSegments(chunks: readonly DiffChunk[]): { + oldSegments: DiffHighlightSegment[]; + newSegments: DiffHighlightSegment[]; +} { + const oldSegments: DiffHighlightSegment[] = []; + const newSegments: DiffHighlightSegment[] = []; - // Parse HTML to extract line contents + chunks.forEach((chunk, chunkIndex) => { + chunk.lines.forEach((line, lineIndex) => { + const ref = { chunkIndex, lineIndex }; + const oldLineNumber = chunk.oldLineNumbers[lineIndex]; + const newLineNumber = chunk.newLineNumbers[lineIndex]; + + if (oldLineNumber !== null) { + appendSegmentLine(oldSegments, oldLineNumber, line, ref); + } + if (newLineNumber !== null) { + appendSegmentLine(newSegments, newLineNumber, line, ref); + } + }); + }); + + return { oldSegments, newSegments }; +} + +async function highlightSegments( + segments: readonly DiffHighlightSegment[], + language: string, + workerTheme: "dark" | "light", + lineHtmlByChunk: Array> +): Promise { + for (const segment of segments) { + const html = await highlightCode(segment.lines.join("\n"), language, workerTheme); const lines = extractLinesFromHtml(html); - // Validate output (detect broken highlighting) - if (lines.length !== chunk.lines.length) { - // Mismatch - highlighting broke the structure - return createFallbackChunk(chunk); + if (lines.length !== segment.lines.length) { + return false; } - // Check if any non-empty line became empty after extraction (indicates malformed HTML) - // This prevents rendering empty spans when original line had content (especially whitespace) const hasEmptyExtraction = lines.some( - (extractedHtml, i) => extractedHtml.length === 0 && chunk.lines[i].length > 0 + (extractedHtml, i) => extractedHtml.length === 0 && segment.lines[i].length > 0 ); if (hasEmptyExtraction) { - return createFallbackChunk(chunk); + return false; + } + + segment.refs.forEach((ref, index) => { + lineHtmlByChunk[ref.chunkIndex][ref.lineIndex] = lines[index]; + }); + } + + return true; +} + +/** + * Highlight all rendered diff chunks with Shiki while preserving old/new file versions. + * + * Immersive review can hydrate a full-file overlay into hundreds of tiny + * add/remove/context chunks. Sending each chunk through Comlink separately makes + * syntax coloring visibly replace the plain fallback after first paint. We still + * keep old and new line streams separate so syntax state from removed code never + * bleeds into added rows from the other version of the file. + */ +export async function highlightDiffChunks( + chunks: readonly DiffChunk[], + language: string, + themeMode: ThemeMode = "dark" +): Promise { + if (chunks.length === 0) { + return []; + } + + // Fast path: no highlighting for text files, but still escape attacker-controlled text. + if (language === "text" || language === "plaintext") { + return chunks.map((chunk) => createPlainTextChunk(chunk, false)); + } + + const { oldSegments, newSegments } = buildVersionedHighlightSegments(chunks); + const sourceLines = [...oldSegments, ...newSegments].flatMap((segment) => segment.lines); + if (sourceLines.length === 0 || !isWithinDiffHighlightSyncBudget(sourceLines)) { + return chunks.map(createFallbackChunk); + } + + const workerTheme = isLightThemeMode(themeMode) ? "light" : "dark"; + + try { + const lineHtmlByChunk = chunks.map( + (chunk) => new Array(chunk.lines.length) + ); + const highlightedOld = await highlightSegments( + oldSegments, + language, + workerTheme, + lineHtmlByChunk + ); + const highlightedNew = await highlightSegments( + newSegments, + language, + workerTheme, + lineHtmlByChunk + ); + + if (!highlightedOld || !highlightedNew) { + return chunks.map(createFallbackChunk); } - return { + return chunks.map((chunk, chunkIndex) => ({ type: chunk.type, - lines: lines.map((html, i) => ({ - html, - oldLineNumber: chunk.oldLineNumbers[i], - newLineNumber: chunk.newLineNumbers[i], - originalIndex: chunk.startIndex + i, - })), + lines: chunk.lines.map((_, lineIndex) => { + const html = lineHtmlByChunk[chunkIndex][lineIndex]; + if (html === undefined) { + return { + html: escapeHtml(chunk.lines[lineIndex]), + oldLineNumber: chunk.oldLineNumbers[lineIndex], + newLineNumber: chunk.newLineNumbers[lineIndex], + originalIndex: chunk.startIndex + lineIndex, + }; + } + + return { + html, + oldLineNumber: chunk.oldLineNumbers[lineIndex], + newLineNumber: chunk.newLineNumbers[lineIndex], + originalIndex: chunk.startIndex + lineIndex, + }; + }), usedFallback: false, - }; + })); } catch (error) { - console.warn(`Syntax highlighting failed for language ${language}:`, error); - return createFallbackChunk(chunk); + console.warn( + `Syntax highlighting failed for language ${language} (${sourceLines.length} lines):`, + error + ); + return chunks.map(createFallbackChunk); } } /** - * Create plain text fallback for a chunk + * Create plain text fallback for a chunk. */ function createFallbackChunk(chunk: DiffChunk): HighlightedChunk { + return createPlainTextChunk(chunk, true); +} + +function createPlainTextChunk(chunk: DiffChunk, usedFallback: boolean): HighlightedChunk { return { type: chunk.type, lines: chunk.lines.map((line, i) => ({ @@ -133,7 +246,7 @@ function createFallbackChunk(chunk: DiffChunk): HighlightedChunk { newLineNumber: chunk.newLineNumbers[i], originalIndex: chunk.startIndex + i, })), - usedFallback: true, + usedFallback, }; } diff --git a/src/common/constants/storage.ts b/src/common/constants/storage.ts index 53d9c0eefc..f0cdea8435 100644 --- a/src/common/constants/storage.ts +++ b/src/common/constants/storage.ts @@ -534,6 +534,14 @@ export function getReviewStateKey(workspaceId: string): string { return `review-state:${workspaceId}`; } +/** + * Get the localStorage key for selected review hunk per workspace. + * Format: "review-selected-hunk:{workspaceId}" + */ +export function getReviewSelectedHunkKey(workspaceId: string): string { + return `review-selected-hunk:${workspaceId}`; +} + /** * Get the localStorage key for hunk first-seen timestamps per workspace * Tracks when each hunk content address was first observed (for LIFO sorting) @@ -748,6 +756,7 @@ const PERSISTENT_WORKSPACE_KEY_FUNCTIONS: Array<(workspaceId: string) => string> getAgentIdKey, getPinnedAgentIdKey, getThinkingLevelKey, + getReviewSelectedHunkKey, getReviewStateKey, getHunkFirstSeenKey, getReviewExpandStateKey, diff --git a/tests/e2e/scenarios/perf.reviewHunkIteration.spec.ts b/tests/e2e/scenarios/perf.reviewHunkIteration.spec.ts new file mode 100644 index 0000000000..49d89a3e8d --- /dev/null +++ b/tests/e2e/scenarios/perf.reviewHunkIteration.spec.ts @@ -0,0 +1,246 @@ +import { type Locator, type Page } from "@playwright/test"; +import { electronTest as test, electronExpect as expect } from "../electronTest"; +import { REVIEW_SORT_ORDER_KEY } from "../../../src/common/constants/storage"; +import { STORAGE_KEYS } from "../../../src/constants/workspaceDefaults"; +import { + readReactProfileSnapshot, + resetReactProfileSamples, + withChromeProfiles, + writePerfArtifacts, +} from "../utils/perfProfile"; +import { disableReviewTutorial, seedLargeReviewSingleFileDiff } from "../utils/reviewPerfFixture"; + +const shouldRunPerfScenarios = process.env.MUX_E2E_RUN_PERF === "1"; +const HUNK_ITERATION_COUNT = 60; + +test.skip( + ({ browserName }) => browserName !== "chromium", + "Electron scenario runs on chromium only" +); + +interface DurationSummary { + sampleCount: number; + totalMs: number; + meanMs: number; + medianMs: number; + p95Ms: number; + maxMs: number; +} + +interface SyntaxHighlightSummary { + lineCount: number; + syntaxHighlightedLineCount: number; + fullContextReadyAfterInitialRevealMs?: number; + fullContextReadyMs?: number; + initialRevealReadyMs?: number; +} + +function summarizeDurations(samples: number[]): DurationSummary { + if (samples.length === 0) { + return { + sampleCount: 0, + totalMs: 0, + meanMs: 0, + medianMs: 0, + p95Ms: 0, + maxMs: 0, + }; + } + + const sorted = [...samples].sort((a, b) => a - b); + const percentile = (percent: number) => { + const index = Math.min(sorted.length - 1, Math.ceil((percent / 100) * sorted.length) - 1); + return sorted[Math.max(0, index)]; + }; + const totalMs = samples.reduce((total, sample) => total + sample, 0); + + return { + sampleCount: samples.length, + totalMs, + meanMs: totalMs / samples.length, + medianMs: percentile(50), + p95Ms: percentile(95), + maxMs: sorted[sorted.length - 1], + }; +} + +async function readSyntaxHighlightSummary( + lineContainers: Locator +): Promise { + return lineContainers.evaluateAll((lineElements) => { + const getCodeCell = (lineElement: Element): Element | null => { + const spanCells = Array.from(lineElement.children).filter( + (child) => child.tagName === "SPAN" + ); + return spanCells[spanCells.length - 1] ?? null; + }; + + return { + lineCount: lineElements.length, + syntaxHighlightedLineCount: lineElements.filter((lineElement) => { + const codeCell = getCodeCell(lineElement); + return codeCell?.querySelector('span[style*="color"]') != null; + }).length, + }; + }); +} + +async function waitForAllSyntaxHighlighted( + lineContainers: Locator, + expectedLineCount: number +): Promise { + await expect(lineContainers).toHaveCount(expectedLineCount, { timeout: 20_000 }); + await expect + .poll(() => readSyntaxHighlightSummary(lineContainers), { timeout: 20_000 }) + .toEqual({ + lineCount: expectedLineCount, + syntaxHighlightedLineCount: expectedLineCount, + }); + + return readSyntaxHighlightSummary(lineContainers); +} + +async function primeReviewForHeadDiff(page: Page, workspaceId: string): Promise { + const reviewDiffBaseKey = STORAGE_KEYS.reviewDiffBase(workspaceId); + + await page.evaluate( + ({ diffBaseKey, sortOrderKey }) => { + window.localStorage.setItem(diffBaseKey, JSON.stringify("HEAD")); + window.localStorage.setItem("review-show-read", JSON.stringify(true)); + window.localStorage.setItem(sortOrderKey, JSON.stringify("file-order")); + }, + { + diffBaseKey: reviewDiffBaseKey, + sortOrderKey: REVIEW_SORT_ORDER_KEY, + } + ); +} + +test.describe("immersive review hunk iteration performance profiling", () => { + test.skip(!shouldRunPerfScenarios, "Set MUX_E2E_RUN_PERF=1 to run perf profiling scenarios"); + + test("perf: iterate hunks in immersive review for a large file", async ({ + page, + ui, + workspace, + }, testInfo) => { + await disableReviewTutorial(page); + + const diffSummary = seedLargeReviewSingleFileDiff(workspace.demoProject.workspacePath); + const expectedOverlayLineCount = diffSummary.lineCount + diffSummary.deletedLines; + expect(diffSummary.hunkCount).toBeGreaterThan(HUNK_ITERATION_COUNT); + + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText(`0/${diffSummary.hunkCount}`, { exact: true })).toBeVisible({ + timeout: 20_000, + }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); + + const syntaxHighlightStartedAt = Date.now(); + // Wait for the full-file overlay, not just compact diff hunks, so this profile + // reproduces the sluggish path users hit when reviewing many hunks in one large file. + const diffLineContainers = immersiveReview.locator( + '[data-testid="immersive-diff-reveal-stage"] div[data-line-index]' + ); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + const initialRevealReadyMs = Date.now() - syntaxHighlightStartedAt; + const fullContextReadyStartedAt = Date.now(); + // Plain fallback lines render as text-only cells. Shiki-highlighted lines add + // nested colorized token spans; wait for every generated line before profiling. + const syntaxHighlightSummary = { + ...(await waitForAllSyntaxHighlighted(diffLineContainers, expectedOverlayLineCount)), + fullContextReadyAfterInitialRevealMs: Date.now() - fullContextReadyStartedAt, + fullContextReadyMs: Date.now() - syntaxHighlightStartedAt, + initialRevealReadyMs, + }; + await immersiveReview.focus(); + + await resetReactProfileSamples(page); + + let iterationDurationsMs: number[] = []; + const runLabel = `review-immersive-hunk-iteration-${diffSummary.lineCount}-lines-${diffSummary.hunkCount}-hunks`; + const chromeProfile = await withChromeProfiles(page, { label: runLabel }, async () => { + iterationDurationsMs = await page.evaluate(async (iterationCount) => { + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); + if (!immersiveView) { + throw new Error("Immersive review view was not found"); + } + + const waitForFrame = () => + new Promise((resolve) => window.requestAnimationFrame(() => resolve())); + const durations: number[] = []; + immersiveView.focus(); + + for (let stepIndex = 0; stepIndex < iterationCount; stepIndex += 1) { + const expectedPosition = String(stepIndex + 2); + const startedAt = window.performance.now(); + window.dispatchEvent( + new KeyboardEvent("keydown", { + key: "j", + bubbles: true, + cancelable: true, + }) + ); + + while (immersiveView.getAttribute("data-selected-hunk-position") !== expectedPosition) { + await waitForFrame(); + } + durations.push(window.performance.now() - startedAt); + } + + return durations; + }, HUNK_ITERATION_COUNT); + }); + + const reactProfileSnapshot = await readReactProfileSnapshot(page); + if (!reactProfileSnapshot) { + throw new Error("React profile snapshot was not captured"); + } + + const iterationSummary = summarizeDurations(iterationDurationsMs); + const artifactDirectory = await writePerfArtifacts({ + testInfo, + runLabel, + chromeProfile, + reactProfile: reactProfileSnapshot, + historyProfile: { + kind: "immersive-review-hunk-iteration", + ...diffSummary, + syntaxHighlighting: syntaxHighlightSummary, + iterationCount: HUNK_ITERATION_COUNT, + iterationDurationsMs, + iterationSummary, + }, + }); + + expect(iterationSummary.sampleCount).toBe(HUNK_ITERATION_COUNT); + expect(chromeProfile.wallTimeMs).toBeGreaterThan(0); + expect(chromeProfile.cpuProfile).not.toBeNull(); + expect(reactProfileSnapshot.enabled).toBe(true); + + testInfo.annotations.push({ + type: "perf-artifact", + description: artifactDirectory, + }); + }); +}); diff --git a/tests/e2e/scenarios/perf.reviewMarkHunkRead.spec.ts b/tests/e2e/scenarios/perf.reviewMarkHunkRead.spec.ts index 97504e8871..b5e0bbdd14 100644 --- a/tests/e2e/scenarios/perf.reviewMarkHunkRead.spec.ts +++ b/tests/e2e/scenarios/perf.reviewMarkHunkRead.spec.ts @@ -7,7 +7,7 @@ import { withChromeProfiles, writePerfArtifacts, } from "../utils/perfProfile"; -import { seedLargeReviewDiff } from "../utils/reviewPerfFixture"; +import { LARGE_CHANGE_ROOT, seedLargeReviewDiff } from "../utils/reviewPerfFixture"; const shouldRunPerfScenarios = process.env.MUX_E2E_RUN_PERF === "1"; @@ -60,6 +60,12 @@ test.describe("immersive review performance profiling", () => { const immersiveReview = page.getByTestId("immersive-review-view"); await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + const firstFilePath = `${LARGE_CHANGE_ROOT}/group-01/bucket-01/probe-001.ts`; + const secondFilePath = `${LARGE_CHANGE_ROOT}/group-01/bucket-01/probe-002.ts`; + await expect(immersiveReview.getByText(firstFilePath)).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); const markReadButton = immersiveReview.getByRole("button", { name: "Mark hunk as read" }); await expect(markReadButton).toBeVisible({ timeout: 20_000 }); @@ -68,9 +74,13 @@ test.describe("immersive review performance profiling", () => { const runLabel = `review-immersive-mark-read-${diffSummary.fileCount}-files-${diffSummary.hunkCount}-hunks`; const chromeProfile = await withChromeProfiles(page, { label: runLabel }, async () => { await markReadButton.dispatchEvent("click"); - await expect( - immersiveReview.getByRole("button", { name: "Mark hunk as unread" }) - ).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview.getByText(secondFilePath)).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); + await expect(immersiveReview.getByRole("button", { name: "Mark hunk as read" })).toBeVisible({ + timeout: 20_000, + }); await expect .poll( () => diff --git a/tests/e2e/scenarios/reviewHydration.spec.ts b/tests/e2e/scenarios/reviewHydration.spec.ts new file mode 100644 index 0000000000..e493b62c1e --- /dev/null +++ b/tests/e2e/scenarios/reviewHydration.spec.ts @@ -0,0 +1,383 @@ +import { type Page } from "@playwright/test"; +import { electronExpect as expect, electronTest as test } from "../electronTest"; +import { REVIEW_SORT_ORDER_KEY } from "../../../src/common/constants/storage"; +import { STORAGE_KEYS } from "../../../src/constants/workspaceDefaults"; +import { + disableReviewTutorial, + seedReviewHydrationJumpDiff, + seedReviewMarkReadIterationDiff, +} from "../utils/reviewPerfFixture"; + +test.skip( + ({ browserName }) => browserName !== "chromium", + "Electron scenario runs on chromium only" +); + +interface HydrationSample { + t: number; + overlayVisible: boolean; + stageVisible: boolean; + lineCount: number; + scrollTop: number | null; + overlayTop: number | null; + overlayHeight: number | null; + scrollContainerTop: number | null; + scrollContainerHeight: number | null; + selectedTop: number | null; + stageText: string; +} + +async function primeReviewForHeadDiff( + page: Page, + workspaceId: string, + options: { showReadHunks?: boolean } = {} +): Promise { + const reviewDiffBaseKey = STORAGE_KEYS.reviewDiffBase(workspaceId); + + await page.evaluate( + ({ diffBaseKey, sortOrderKey, showReadHunks }) => { + window.localStorage.setItem(diffBaseKey, JSON.stringify("HEAD")); + window.localStorage.setItem("review-show-read", JSON.stringify(showReadHunks)); + window.localStorage.setItem(sortOrderKey, JSON.stringify("file-order")); + }, + { + diffBaseKey: reviewDiffBaseKey, + sortOrderKey: REVIEW_SORT_ORDER_KEY, + showReadHunks: options.showReadHunks ?? true, + } + ); +} + +async function startHydrationSampler( + page: Page, + selectedFullLineIndex: number | null +): Promise { + await page.evaluate((targetLineIndex) => { + const samples: HydrationSample[] = []; + let frameId: number | null = null; + let running = true; + const startedAt = performance.now(); + + const isVisible = (element: Element | null): boolean => { + if (!element) return false; + const style = window.getComputedStyle(element); + const rect = element.getBoundingClientRect(); + return ( + style.display !== "none" && + style.visibility !== "hidden" && + Number(style.opacity) !== 0 && + rect.width > 0 && + rect.height > 0 + ); + }; + + const sample = () => { + const stage = document.querySelector( + '[data-testid="immersive-diff-reveal-stage"]' + ); + const overlay = document.querySelector( + '[data-testid="immersive-diff-reveal-overlay"]' + ); + const lines = stage + ? Array.from(stage.querySelectorAll("div[data-line-index]")) + : []; + const selectedLine = + targetLineIndex == null + ? null + : (stage?.querySelector(`div[data-line-index="${targetLineIndex}"]`) ?? + null); + const scrollContainer = stage?.closest(".overflow-y-auto") ?? null; + const overlayRect = overlay?.getBoundingClientRect() ?? null; + const scrollContainerRect = scrollContainer?.getBoundingClientRect() ?? null; + + samples.push({ + t: performance.now() - startedAt, + overlayVisible: isVisible(overlay), + overlayTop: overlayRect?.top ?? null, + overlayHeight: overlayRect?.height ?? null, + scrollContainerTop: scrollContainerRect?.top ?? null, + scrollContainerHeight: scrollContainerRect?.height ?? null, + stageVisible: isVisible(stage), + lineCount: lines.length, + scrollTop: scrollContainer?.scrollTop ?? null, + selectedTop: selectedLine?.getBoundingClientRect().top ?? null, + stageText: stage?.textContent ?? "", + }); + + if (running) { + frameId = window.requestAnimationFrame(sample); + } + }; + + window.__muxHydrationJumpSampler = { + samples, + stop: () => { + running = false; + if (frameId !== null) { + window.cancelAnimationFrame(frameId); + frameId = null; + } + sample(); + return samples; + }, + }; + + frameId = window.requestAnimationFrame(sample); + }, selectedFullLineIndex); +} + +async function stopHydrationSampler(page: Page): Promise { + return page.evaluate(() => window.__muxHydrationJumpSampler?.stop() ?? []); +} + +declare global { + interface Window { + __muxHydrationJumpSampler?: { + samples: HydrationSample[]; + stop: () => HydrationSample[]; + }; + } +} + +test.describe("immersive review hydration stability", () => { + test("keeps m-key mark-read loading overlay sized to the scrollport", async ({ + page, + ui, + workspace, + }) => { + await disableReviewTutorial(page); + const diffSummary = seedReviewMarkReadIterationDiff(workspace.demoProject.workspacePath); + + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText(`0/${diffSummary.hunkCount}`, { exact: true })).toBeVisible({ + timeout: 20_000, + }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview.getByText(diffSummary.filePaths[0])).toBeVisible({ + timeout: 20_000, + }); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + + await startHydrationSampler(page, null); + await immersiveReview.focus(); + await page.evaluate(async (filePaths) => { + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); + if (!immersiveView) { + throw new Error("Immersive review view was not found"); + } + + const waitForFrame = () => + new Promise((resolve) => window.requestAnimationFrame(() => resolve())); + + for (const filePath of filePaths.slice(1)) { + window.dispatchEvent( + new KeyboardEvent("keydown", { + key: "m", + bubbles: true, + cancelable: true, + }) + ); + + while (!immersiveView.textContent?.includes(filePath)) { + await waitForFrame(); + } + await waitForFrame(); + } + }, diffSummary.filePaths); + + const samples = await stopHydrationSampler(page); + const overlaySamples = samples.filter( + (sample) => + sample.overlayVisible && + sample.overlayTop !== null && + sample.overlayHeight !== null && + sample.scrollContainerTop !== null && + sample.scrollContainerHeight !== null + ); + expect(overlaySamples.length).toBeGreaterThan(0); + + for (const sample of overlaySamples) { + expect( + Math.abs((sample.overlayTop ?? 0) - (sample.scrollContainerTop ?? 0)) + ).toBeLessThanOrEqual(2); + expect( + Math.abs((sample.overlayHeight ?? 0) - (sample.scrollContainerHeight ?? 0)) + ).toBeLessThanOrEqual(2); + } + + const hiddenStageSamples = samples.filter( + (sample) => !sample.stageVisible && sample.lineCount > 0 + ); + expect(hiddenStageSamples.length).toBeGreaterThan(0); + expect(hiddenStageSamples.every((sample) => sample.overlayVisible)).toBe(true); + }); + + test("keeps Shift+M file-read advancement covered while read hunks are hidden", async ({ + page, + ui, + workspace, + }) => { + await disableReviewTutorial(page); + const diffSummary = seedReviewMarkReadIterationDiff(workspace.demoProject.workspacePath); + + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId, { showReadHunks: false }); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText(`0/${diffSummary.hunkCount}`, { exact: true })).toBeVisible({ + timeout: 20_000, + }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview.getByText(diffSummary.filePaths[0])).toBeVisible({ + timeout: 20_000, + }); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + + await startHydrationSampler(page, null); + await immersiveReview.focus(); + await page.evaluate(async (nextFilePath) => { + const sampler = window.__muxHydrationJumpSampler; + if (sampler) { + sampler.samples.length = 0; + } + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); + if (!immersiveView) { + throw new Error("Immersive review view was not found"); + } + + const waitForFrame = () => + new Promise((resolve) => window.requestAnimationFrame(() => resolve())); + window.dispatchEvent( + new KeyboardEvent("keydown", { + key: "M", + shiftKey: true, + bubbles: true, + cancelable: true, + }) + ); + + while (!immersiveView.textContent?.includes(nextFilePath)) { + await waitForFrame(); + } + await waitForFrame(); + }, diffSummary.filePaths[1]); + + await expect(immersiveReview.getByText(diffSummary.filePaths[1])).toBeVisible({ + timeout: 20_000, + }); + const samples = await stopHydrationSampler(page); + const staleVisibleSamples = samples.filter( + (sample) => + sample.stageVisible && !sample.overlayVisible && sample.stageText.includes("ready-001-001") + ); + expect(staleVisibleSamples).toEqual([]); + + const hiddenStageSamples = samples.filter( + (sample) => !sample.stageVisible && sample.lineCount > 0 + ); + expect(hiddenStageSamples.every((sample) => sample.overlayVisible)).toBe(true); + }); + + test("keeps compact-to-full file hydration hidden until selected hunk geometry is stable", async ({ + page, + ui, + workspace, + }) => { + await disableReviewTutorial(page); + const diffSummary = seedReviewHydrationJumpDiff(workspace.demoProject.workspacePath); + const expectedFullOverlayLineCount = diffSummary.lineCount + diffSummary.deletedLines; + + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText("0/1", { exact: true })).toBeVisible({ timeout: 20_000 }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await startHydrationSampler(page, diffSummary.changedLineNumber); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); + + const diffLineContainers = immersiveReview.locator( + '[data-testid="immersive-diff-reveal-stage"] div[data-line-index]' + ); + await expect(diffLineContainers).toHaveCount(expectedFullOverlayLineCount, { timeout: 20_000 }); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + + const samples = await stopHydrationSampler(page); + expect(samples.length).toBeGreaterThan(0); + + const visibleCompactSamples = samples.filter( + (sample) => + sample.stageVisible && + !sample.overlayVisible && + sample.lineCount > 0 && + sample.lineCount < expectedFullOverlayLineCount + ); + expect(visibleCompactSamples).toEqual([]); + + const visibleOverlaySamples = samples.filter( + (sample) => sample.overlayVisible && sample.overlayTop !== null + ); + expect(visibleOverlaySamples.length).toBeGreaterThan(0); + const overlayTops = visibleOverlaySamples.map((sample) => sample.overlayTop ?? 0); + const overlayTopRange = Math.max(...overlayTops) - Math.min(...overlayTops); + expect(overlayTopRange).toBeLessThanOrEqual(2); + + const visibleSelectedSamples = samples.filter( + (sample) => + sample.stageVisible && + !sample.overlayVisible && + sample.lineCount === expectedFullOverlayLineCount && + sample.selectedTop !== null + ); + expect(visibleSelectedSamples.length).toBeGreaterThan(0); + + const selectedTops = visibleSelectedSamples.map((sample) => sample.selectedTop ?? 0); + const selectedTopRange = Math.max(...selectedTops) - Math.min(...selectedTops); + expect(selectedTopRange).toBeLessThanOrEqual(2); + }); +}); diff --git a/tests/e2e/utils/reviewPerfFixture.ts b/tests/e2e/utils/reviewPerfFixture.ts index 09ed5d06b5..a13b4a32ab 100644 --- a/tests/e2e/utils/reviewPerfFixture.ts +++ b/tests/e2e/utils/reviewPerfFixture.ts @@ -11,6 +11,14 @@ const LARGE_CHANGE_FILES_PER_BUCKET = 5; const LARGE_CHANGE_FILE_COUNT = LARGE_CHANGE_GROUP_COUNT * LARGE_CHANGE_BUCKETS_PER_GROUP * LARGE_CHANGE_FILES_PER_BUCKET; +const LARGE_FILE_CHANGE_ROOT = "src/review/perf-large-file"; +const LARGE_FILE_CHANGE_PATH = `${LARGE_FILE_CHANGE_ROOT}/hunk-iteration.ts`; +const LARGE_FILE_LINE_COUNT = 1_500; +const LARGE_FILE_HUNK_COUNT = 150; +const LARGE_FILE_HUNK_SPACING = 10; +const MARK_READ_ITERATION_ROOT = "src/review/mark-read-iteration"; +const MARK_READ_ITERATION_FILE_COUNT = 4; + export interface LargeReviewDiffSummary { rootPath: string; fileCount: number; @@ -21,10 +29,22 @@ export interface LargeReviewDiffSummary { changedLinesPerFile: number; } +export interface LargeReviewSingleFileDiffSummary extends LargeReviewDiffSummary { + filePath: string; + lineCount: number; + hunkSpacing: number; +} + interface LargeReviewDiffOptions { changedLinesPerFile?: number; } +interface LargeReviewSingleFileDiffOptions { + hunkCount?: number; + hunkSpacing?: number; + lineCount?: number; +} + function runGitCommand(cwd: string, args: string[]): string { const result = spawnSync("git", args, { cwd, encoding: "utf-8" }); if (result.status === 0) { @@ -61,6 +81,48 @@ function buildLargeReviewFixtureSource( ].join("\n"); } +function normalizeLargeReviewSingleFileOptions( + options: LargeReviewSingleFileDiffOptions +): Required { + const hunkCount = Math.max(1, Math.trunc(options.hunkCount ?? LARGE_FILE_HUNK_COUNT)); + // Keep hunks far enough apart that Git's default diff context does not merge + // neighboring changes; otherwise the perf fixture stops exercising hunk iteration. + const hunkSpacing = Math.max(10, Math.trunc(options.hunkSpacing ?? LARGE_FILE_HUNK_SPACING)); + const minimumLineCount = 1 + (hunkCount - 1) * hunkSpacing; + const lineCount = Math.max( + minimumLineCount, + Math.trunc(options.lineCount ?? LARGE_FILE_LINE_COUNT) + ); + + return { hunkCount, hunkSpacing, lineCount }; +} + +function buildLargeReviewSingleFileSource( + variant: "base" | "modified", + options: Required +): string { + const status = variant === "base" ? "pending" : "ready"; + const changedLineNumbers = new Set(); + for (let hunkIndex = 0; hunkIndex < options.hunkCount; hunkIndex += 1) { + changedLineNumbers.add(1 + hunkIndex * options.hunkSpacing); + } + + const lines = Array.from({ length: options.lineCount }, (_, lineIndex) => { + const lineNumber = lineIndex + 1; + const lineId = String(lineNumber).padStart(4, "0"); + if (changedLineNumbers.has(lineNumber)) { + return `export const reviewLargeChangedLine${lineId} = "${status}-${lineId}";`; + } + return `export const reviewLargeContextLine${lineId} = "stable-${lineId}";`; + }); + + return `${lines.join("\n")}\n`; +} + +function countGitDiffHunks(diffOutput: string): number { + return diffOutput.split("\n").filter((line) => line.startsWith("@@ ")).length; +} + export async function disableReviewTutorial(page: Page): Promise { await page.evaluate((tutorialStateKey) => { const raw = window.localStorage.getItem(tutorialStateKey); @@ -165,3 +227,200 @@ export function seedLargeReviewDiff( changedLinesPerFile, }; } + +export interface ReviewMarkReadIterationDiffSummary extends LargeReviewDiffSummary { + filePaths: string[]; +} + +export function seedReviewMarkReadIterationDiff( + workspacePath: string +): ReviewMarkReadIterationDiffSummary { + const filePaths = Array.from({ length: MARK_READ_ITERATION_FILE_COUNT }, (_, fileIndex) => { + const fileId = String(fileIndex + 1).padStart(2, "0"); + return `${MARK_READ_ITERATION_ROOT}/probe-${fileId}.ts`; + }); + + for (const [fileIndex, relativePath] of filePaths.entries()) { + const filePath = path.join(workspacePath, ...relativePath.split("/")); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, buildLargeReviewFixtureSource(fileIndex, "base", 1), "utf-8"); + } + + runGitCommand(workspacePath, ["add", MARK_READ_ITERATION_ROOT]); + runGitCommand(workspacePath, ["commit", "-q", "-m", "Seed mark-read review fixture"]); + + for (const [fileIndex, relativePath] of filePaths.entries()) { + const filePath = path.join(workspacePath, ...relativePath.split("/")); + fs.writeFileSync(filePath, buildLargeReviewFixtureSource(fileIndex, "modified", 1), "utf-8"); + } + + const diffOutput = runGitCommand(workspacePath, ["diff", "HEAD", "--", MARK_READ_ITERATION_ROOT]); + const hunkCount = countGitDiffHunks(diffOutput); + if (hunkCount !== filePaths.length) { + throw new Error(`Expected ${filePaths.length} mark-read hunks, received ${hunkCount}`); + } + + const numstatOutput = runGitCommand(workspacePath, [ + "diff", + "HEAD", + "--numstat", + "--", + MARK_READ_ITERATION_ROOT, + ]).trim(); + let addedLines = 0; + let deletedLines = 0; + for (const line of numstatOutput.split("\n").filter(Boolean)) { + const [addedText = "0", deletedText = "0"] = line.split("\t"); + addedLines += Number.parseInt(addedText, 10) || 0; + deletedLines += Number.parseInt(deletedText, 10) || 0; + } + + return { + rootPath: MARK_READ_ITERATION_ROOT, + filePaths, + fileCount: filePaths.length, + directoryCount: 3, + hunkCount, + addedLines, + deletedLines, + changedLinesPerFile: 1, + }; +} + +const HYDRATION_JUMP_CHANGE_ROOT = "src/review/hydration-jump"; +const HYDRATION_JUMP_CHANGE_PATH = `${HYDRATION_JUMP_CHANGE_ROOT}/hydration-jump.ts`; +const HYDRATION_JUMP_LINE_COUNT = 1_000; +const HYDRATION_JUMP_CHANGED_LINE = 500; + +export interface ReviewHydrationJumpDiffSummary extends LargeReviewDiffSummary { + filePath: string; + lineCount: number; + changedLineNumber: number; + selectedAddedText: string; +} + +function buildReviewHydrationJumpSource( + variant: "base" | "modified", + lineCount: number, + changedLineNumber: number +): string { + const status = variant === "base" ? "pending" : "ready"; + const lines = Array.from({ length: lineCount }, (_, lineIndex) => { + const lineNumber = lineIndex + 1; + const lineId = String(lineNumber).padStart(4, "0"); + if (lineNumber === changedLineNumber) { + return `export const reviewHydrationJumpChangedLine${lineId} = "${status}-${lineId}";`; + } + return `export const reviewHydrationJumpContextLine${lineId} = "stable-${lineId}";`; + }); + + return `${lines.join("\n")}\n`; +} + +export function seedReviewHydrationJumpDiff(workspacePath: string): ReviewHydrationJumpDiffSummary { + const filePath = path.join(workspacePath, ...HYDRATION_JUMP_CHANGE_PATH.split("/")); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync( + filePath, + buildReviewHydrationJumpSource("base", HYDRATION_JUMP_LINE_COUNT, HYDRATION_JUMP_CHANGED_LINE), + "utf-8" + ); + + runGitCommand(workspacePath, ["add", HYDRATION_JUMP_CHANGE_PATH]); + runGitCommand(workspacePath, ["commit", "-q", "-m", "Seed hydration jump review fixture"]); + + fs.writeFileSync( + filePath, + buildReviewHydrationJumpSource( + "modified", + HYDRATION_JUMP_LINE_COUNT, + HYDRATION_JUMP_CHANGED_LINE + ), + "utf-8" + ); + + const diffOutput = runGitCommand(workspacePath, [ + "diff", + "HEAD", + "--", + HYDRATION_JUMP_CHANGE_PATH, + ]); + const hunkCount = countGitDiffHunks(diffOutput); + if (hunkCount !== 1) { + throw new Error(`Expected one hydration-jump hunk, received ${hunkCount}`); + } + + const numstatOutput = runGitCommand(workspacePath, [ + "diff", + "HEAD", + "--numstat", + "--", + HYDRATION_JUMP_CHANGE_PATH, + ]).trim(); + const [addedText = "0", deletedText = "0"] = numstatOutput.split("\t"); + const lineId = String(HYDRATION_JUMP_CHANGED_LINE).padStart(4, "0"); + + return { + rootPath: HYDRATION_JUMP_CHANGE_ROOT, + filePath: HYDRATION_JUMP_CHANGE_PATH, + fileCount: 1, + directoryCount: 3, + hunkCount, + addedLines: Number.parseInt(addedText, 10) || 0, + deletedLines: Number.parseInt(deletedText, 10) || 0, + changedLinesPerFile: 1, + lineCount: HYDRATION_JUMP_LINE_COUNT, + changedLineNumber: HYDRATION_JUMP_CHANGED_LINE, + selectedAddedText: `ready-${lineId}`, + }; +} + +export function seedLargeReviewSingleFileDiff( + workspacePath: string, + options: LargeReviewSingleFileDiffOptions = {} +): LargeReviewSingleFileDiffSummary { + const normalizedOptions = normalizeLargeReviewSingleFileOptions(options); + const filePath = path.join(workspacePath, ...LARGE_FILE_CHANGE_PATH.split("/")); + + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, buildLargeReviewSingleFileSource("base", normalizedOptions), "utf-8"); + + runGitCommand(workspacePath, ["add", LARGE_FILE_CHANGE_PATH]); + runGitCommand(workspacePath, ["commit", "-q", "-m", "Seed large review file perf fixture"]); + + fs.writeFileSync( + filePath, + buildLargeReviewSingleFileSource("modified", normalizedOptions), + "utf-8" + ); + + const diffOutput = runGitCommand(workspacePath, ["diff", "HEAD", "--", LARGE_FILE_CHANGE_PATH]); + const hunkCount = countGitDiffHunks(diffOutput); + if (hunkCount !== normalizedOptions.hunkCount) { + throw new Error( + `Expected ${normalizedOptions.hunkCount} hunks in large-file fixture, received ${hunkCount}` + ); + } + + const numstatOutput = runGitCommand(workspacePath, [ + "diff", + "HEAD", + "--numstat", + "--", + LARGE_FILE_CHANGE_PATH, + ]).trim(); + const [addedText = "0", deletedText = "0"] = numstatOutput.split("\t"); + + return { + rootPath: LARGE_FILE_CHANGE_ROOT, + filePath: LARGE_FILE_CHANGE_PATH, + fileCount: 1, + directoryCount: 3, + hunkCount, + addedLines: Number.parseInt(addedText, 10) || 0, + deletedLines: Number.parseInt(deletedText, 10) || 0, + changedLinesPerFile: hunkCount, + lineCount: normalizedOptions.lineCount, + hunkSpacing: normalizedOptions.hunkSpacing, + }; +} From 7a88206ded647c3e03dfc61d06a3c8dd51b23da6 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 7 Jun 2026 15:52:46 -0500 Subject: [PATCH 2/3] refactor immersive review reveal pipeline --- .../ImmersiveDiffRevealLoadingState.tsx | 53 ++ .../CodeReview/ImmersiveReviewView.test.tsx | 125 +++ .../CodeReview/ImmersiveReviewView.tsx | 803 +++--------------- .../CodeReview/useImmersiveOverlay.ts | 643 ++++++++++++++ src/browser/features/Shared/DiffRenderer.tsx | 40 +- tests/e2e/scenarios/reviewHydration.spec.ts | 191 ++++- 6 files changed, 1150 insertions(+), 705 deletions(-) create mode 100644 src/browser/features/RightSidebar/CodeReview/ImmersiveDiffRevealLoadingState.tsx create mode 100644 src/browser/features/RightSidebar/CodeReview/useImmersiveOverlay.ts diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveDiffRevealLoadingState.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveDiffRevealLoadingState.tsx new file mode 100644 index 0000000000..b22a52dd85 --- /dev/null +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveDiffRevealLoadingState.tsx @@ -0,0 +1,53 @@ +import { Skeleton } from "@/browser/components/Skeleton/Skeleton"; + +const REVEAL_SKELETON_LINE_WIDTHS: readonly string[] = [ + "w-[72%]", + "w-[46%]", + "w-[88%]", + "w-[61%]", + "w-[80%]", + "w-[52%]", + "w-[70%]", + "w-[92%]", + "w-[44%]", + "w-[83%]", + "w-[58%]", + "w-[76%]", + "w-[49%]", + "w-[87%]", + "w-[65%]", + "w-[79%]", + "w-[54%]", + "w-[91%]", + "w-[68%]", + "w-[42%]", + "w-[74%]", + "w-[85%]", + "w-[57%]", + "w-[78%]", +]; + +export function ImmersiveDiffRevealLoadingState(props: { label: string }) { + return ( +
+ {/* Match the transcript hydration shimmer instead of a centered spinner: the + diff-shaped placeholder keeps the hidden geometry swap feeling like code + is hydrating in place rather than a blocking modal flash. */} + +
+ {REVEAL_SKELETON_LINE_WIDTHS.map((width, rowIndex) => ( +
+ + +
+ ))} +
+
+ ); +} diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 03cc68c7ca..387323a07b 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx @@ -315,6 +315,131 @@ describe("ImmersiveReviewView", () => { expect(view.queryByTestId("immersive-assisted-banner")).toBeNull(); }); + test("normalizes CRLF hunk rows in compact overlays", () => { + const crlfHunk = createHunk({ + id: "hunk-crlf", + oldStart: 5000, + newStart: 5000, + header: "@@ -5000 +5000 @@", + content: "-old crlf\r\n+new crlf\r\n context crlf\r", + }); + + const view = renderImmersiveReview({ + fileTree: createFileTree(crlfHunk.filePath), + hunks: [crlfHunk], + allHunks: [crlfHunk], + selectedHunkId: crlfHunk.id, + }); + + expect(view.container.textContent ?? "").toContain("new crlf"); + expect(view.container.textContent ?? "").not.toContain("\r"); + }); + + test("defers the compact diff renderer while full-file context is pending", async () => { + type ExecuteBashResult = Awaited>; + let resolveRead: (result: ExecuteBashResult) => void = () => { + throw new Error("executeBash was not called"); + }; + mockApi.workspace.executeBash = mock( + () => + new Promise((resolve) => { + resolveRead = resolve; + }) + ); + + const view = renderImmersiveReview(); + + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); + expect(view.getByTestId("immersive-diff-reveal-skeleton")).toBeTruthy(); + // Full-file hydration is the expected end state for this hunk, so do not spend + // a hidden render/highlight pass on the compact hunk rows that would be thrown away. + expect(view.container.textContent ?? "").not.toContain("new line"); + + resolveRead({ + success: true as const, + data: { + success: true, + output: encodeFileReadOutput("new line\ncontext after selected hunk\n"), + exitCode: 0, + }, + }); + + await waitFor(() => + expect(view.container.textContent ?? "").toContain("context after selected hunk") + ); + }); + + test("keeps same-file compact-to-full hydration behind the reveal gate", async () => { + const nearHunk = createHunk({ + id: "hunk-near", + newStart: 40, + newLines: 1, + header: "@@ -40 +40 @@", + content: "-old near line\n+new near line", + }); + const farHunk = createHunk({ + id: "hunk-far", + newStart: 5000, + newLines: 1, + header: "@@ -5000 +5000 @@", + content: "-old far line\n+new far line", + }); + type ExecuteBashResult = Awaited>; + let resolveRead: (result: ExecuteBashResult) => void = () => { + throw new Error("executeBash was not called"); + }; + mockApi.workspace.executeBash = mock( + () => + new Promise((resolve) => { + resolveRead = resolve; + }) + ); + + const renderView = (selectedHunkId: string) => ( + + false} + onToggleRead={mock(() => undefined)} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={selectedHunkId} + onSelectHunk={mock(() => undefined)} + onExit={mock(() => undefined)} + isTouchImmersive={true} + reviewsByFilePath={new Map()} + firstSeenMap={{}} + /> + + ); + + const view = render(renderView(farHunk.id)); + expect(view.container.textContent ?? "").toContain("new far line"); + expect(mockApi.workspace.executeBash).not.toHaveBeenCalled(); + + view.rerender(renderView(nearHunk.id)); + + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); + expect(view.getByTestId("immersive-diff-reveal-skeleton")).toBeTruthy(); + expect(view.container.textContent ?? "").not.toContain("new near line"); + expect(view.container.textContent ?? "").not.toContain("new far line"); + + resolveRead({ + success: true as const, + data: { + success: true, + output: encodeFileReadOutput("new near line\ncontext after selected hunk\n"), + exitCode: 0, + }, + }); + + await waitFor(() => + expect(view.container.textContent ?? "").toContain("context after selected hunk") + ); + }); + 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 23d86a1f14..1fac3ebedc 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx @@ -20,10 +20,15 @@ import { Trash2, } from "lucide-react"; import { cn } from "@/common/lib/utils"; -import { Skeleton } from "@/browser/components/Skeleton/Skeleton"; -import { preloadHighlightedDiff, SelectableDiffRenderer } from "../../Shared/DiffRenderer"; +import { SelectableDiffRenderer } from "../../Shared/DiffRenderer"; +import { ImmersiveDiffRevealLoadingState } from "./ImmersiveDiffRevealLoadingState"; import { ImmersiveMinimap } from "./ImmersiveMinimap"; import { ImmersiveReviewAgentStatusBar } from "./ImmersiveReviewAgentStatusBar"; +import { + useImmersiveOverlay, + type HunkLineRange, + type ImmersiveOverlayData, +} from "./useImmersiveOverlay"; import { buildNewLineNumberToIndexMap, buildOldLineNumberToIndexMap, @@ -48,12 +53,6 @@ import { matchesKeybind, } from "@/browser/utils/ui/keybinds"; import { stopKeyboardPropagation } from "@/browser/utils/events"; -import { - buildReadFileScript, - EXIT_CODE_TOO_LARGE, - EXIT_CODE_TOO_MANY_LINES, - processFileContents, -} from "@/browser/utils/fileRead"; import { updatePersistedState } from "@/browser/hooks/usePersistedState"; import { TooltipIfPresent } from "@/browser/components/Tooltip/Tooltip"; import { getReviewSelectedHunkKey } from "@/common/constants/storage"; @@ -140,32 +139,7 @@ interface PendingComposerHunkSwitch { toHunkId: string; } -interface HunkLineRange { - startIndex: number; - endIndex: number; - firstModifiedIndex: number | null; - lastModifiedIndex: number | null; -} - -interface ImmersiveOverlayData { - content: string; - lineHunkIds: Array; - hunkLineRanges: Map; -} - -interface OverlayRevealIdentity { - filePath: string; - content: string; -} - -const MAX_FULL_FILE_CONTEXT_LINES = 1500; -const MAX_FULL_FILE_CONTEXT_BYTES = 256 * 1024; - -const FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS = 5_000; const LINE_JUMP_SIZE = 10; -// Keep syntax highlighting on for larger review files now that per-line tooltip overhead is gone, -// but still cap it to avoid pathological DOM costs on extremely large diffs. -const MAX_HIGHLIGHTED_DIFF_LINES = 4000; const ACTIVE_LINE_OUTLINE = "1px solid hsl(from var(--color-review-accent) h s l / 0.45)"; const HUNK_RANGE_OUTLINE_COLOR = "hsl(from var(--color-review-accent) h s l / 0.45)"; const LIKE_NOTE_PREFIX = "I like this change"; @@ -173,85 +147,6 @@ const DISLIKE_NOTE_PREFIX = "I don't like this change"; const EMPTY_REVIEWS: Review[] = []; const EMPTY_COMMENT_LINE_INDICES = new Set(); -const REVEAL_SKELETON_LINE_WIDTHS: readonly string[] = [ - "w-[72%]", - "w-[46%]", - "w-[88%]", - "w-[61%]", - "w-[80%]", - "w-[52%]", - "w-[70%]", - "w-[92%]", - "w-[44%]", - "w-[83%]", - "w-[58%]", - "w-[76%]", - "w-[49%]", - "w-[87%]", - "w-[65%]", - "w-[79%]", - "w-[54%]", - "w-[91%]", - "w-[68%]", - "w-[42%]", - "w-[74%]", - "w-[85%]", - "w-[57%]", - "w-[78%]", -]; - -function ImmersiveDiffRevealLoadingState(props: { label: string }) { - return ( -
- {/* Match the transcript hydration shimmer instead of a centered spinner: the - diff-shaped placeholder keeps the hidden geometry swap feeling like code - is hydrating in place rather than a blocking modal flash. */} - -
- {REVEAL_SKELETON_LINE_WIDTHS.map((width, rowIndex) => ( -
- - -
- ))} -
-
- ); -} - -const FULL_FILE_CONTEXT_TIMEOUT = Symbol("full-file-context-timeout"); - -async function withFullFileContextTimeout( - promise: Promise -): Promise { - let timeoutId: ReturnType | null = null; - try { - return await Promise.race([ - promise, - new Promise((resolve) => { - timeoutId = setTimeout( - () => resolve(FULL_FILE_CONTEXT_TIMEOUT), - FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS - ); - }), - ]); - } finally { - if (timeoutId !== null) { - clearTimeout(timeoutId); - } - } -} - function getFileBaseName(filePath: string): string { const segments = filePath.split(/[\\/]/); return segments[segments.length - 1] || filePath; @@ -293,177 +188,6 @@ function getReviewStatusSidebarClasses(status: Review["status"]): { }; } -function splitDiffLines(content: string): string[] { - const lines = content.split(/\r?\n/); - if (lines.length > 0 && lines[lines.length - 1] === "") { - lines.pop(); - } - return lines; -} - -function normalizeFileLines(content: string): string[] { - // Normalize Windows CRLF to LF-equivalent lines so rows stay single-height in - // whitespace-preserving diff cells (embedded "\r" can render as extra breaks). - const lines = content - .split(/\r?\n/) - .map((line) => (line.endsWith("\r") ? line.slice(0, Math.max(0, line.length - 1)) : line)); - return lines.filter((line, idx) => idx < lines.length - 1 || line !== ""); -} - -function isWithinFullFileContextLineBudget(content: string): boolean { - return normalizeFileLines(content).length <= MAX_FULL_FILE_CONTEXT_LINES; -} - -function shouldAttemptFullFileContext(selectedHunk: DiffHunk | null): boolean { - if (!selectedHunk) { - return false; - } - - const lastDisplayLine = selectedHunk.newStart + Math.max(selectedHunk.newLines, 1) - 1; - return lastDisplayLine <= MAX_FULL_FILE_CONTEXT_LINES; -} - -function buildFileContentCacheKey( - workspaceId: string, - filePath: string, - sortedHunks: readonly DiffHunk[] -): string { - const hunkSignature = sortedHunks.map((hunk) => hunk.id).join("|"); - return [workspaceId, filePath, hunkSignature].join("\u0000"); -} - -function buildOverlayFromFileContent( - fileContent: string, - sortedHunks: DiffHunk[] -): ImmersiveOverlayData { - const fileLines = normalizeFileLines(fileContent); - const contentLines: string[] = []; - const lineHunkIds: Array = []; - const hunkLineRanges = new Map(); - - let newLineIdx = 0; - - const pushDisplayLine = (line: string, hunkId: string | null) => { - contentLines.push(line); - lineHunkIds.push(hunkId); - }; - - for (const hunk of sortedHunks) { - const hunkStartInNew = Math.max(0, hunk.newStart - 1); - - while (newLineIdx < hunkStartInNew && newLineIdx < fileLines.length) { - pushDisplayLine(` ${fileLines[newLineIdx]}`, null); - newLineIdx += 1; - } - - const hunkStartIndex = lineHunkIds.length; - let firstModifiedIndex: number | null = null; - let lastModifiedIndex: number | null = null; - - for (const line of splitDiffLines(hunk.content)) { - const prefix = line[0] ?? " "; - if (prefix !== "+" && prefix !== "-" && prefix !== " ") { - continue; - } - - if (prefix === "+" || prefix === "-") { - firstModifiedIndex ??= lineHunkIds.length; - lastModifiedIndex = lineHunkIds.length; - } - - pushDisplayLine(`${prefix}${line.slice(1)}`, hunk.id); - if (prefix !== "-") { - newLineIdx += 1; - } - } - - if (lineHunkIds.length > hunkStartIndex) { - hunkLineRanges.set(hunk.id, { - startIndex: hunkStartIndex, - endIndex: lineHunkIds.length - 1, - firstModifiedIndex, - lastModifiedIndex, - }); - } - } - - while (newLineIdx < fileLines.length) { - pushDisplayLine(` ${fileLines[newLineIdx]}`, null); - newLineIdx += 1; - } - - return { - content: contentLines.join("\n"), - lineHunkIds, - hunkLineRanges, - }; -} - -function buildOverlayFromHunks(sortedHunks: DiffHunk[]): ImmersiveOverlayData { - const contentLines: string[] = []; - const lineHunkIds: Array = []; - const hunkLineRanges = new Map(); - - const pushDisplayLine = (line: string, hunkId: string | null) => { - contentLines.push(line); - lineHunkIds.push(hunkId); - }; - - const pushHeaderLine = (line: string) => { - // Header rows are intentionally excluded from lineHunkIds because DiffRenderer - // does not render @@ header lines in selectable output. - contentLines.push(line); - }; - - sortedHunks.forEach((hunk, index) => { - if (index > 0) { - pushDisplayLine(" ", null); - } - - pushHeaderLine(`@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`); - - const hunkStartIndex = lineHunkIds.length; - let firstModifiedIndex: number | null = null; - let lastModifiedIndex: number | null = null; - - for (const line of splitDiffLines(hunk.content)) { - const prefix = line[0] ?? " "; - if (prefix !== "+" && prefix !== "-" && prefix !== " ") { - continue; - } - - if (prefix === "+" || prefix === "-") { - firstModifiedIndex ??= lineHunkIds.length; - lastModifiedIndex = lineHunkIds.length; - } - - pushDisplayLine(`${prefix}${line.slice(1)}`, hunk.id); - } - - if (lineHunkIds.length > hunkStartIndex) { - hunkLineRanges.set(hunk.id, { - startIndex: hunkStartIndex, - endIndex: lineHunkIds.length - 1, - firstModifiedIndex, - lastModifiedIndex, - }); - } - }); - - return { - content: contentLines.join("\n"), - lineHunkIds, - hunkLineRanges, - }; -} - -function isSameOverlayRevealIdentity( - lhs: OverlayRevealIdentity | null, - rhs: OverlayRevealIdentity | null -): boolean { - return lhs?.filePath === rhs?.filePath && lhs?.content === rhs?.content; -} - function isSelectionInsideRange(selection: SelectedLineRange, range: HunkLineRange): boolean { const start = Math.min(selection.startIndex, selection.endIndex); const end = Math.max(selection.startIndex, selection.endIndex); @@ -679,8 +403,6 @@ export const ImmersiveReviewView: React.FC = (props) = return null; }, [selectedHunkId, hunks, allHunks, fileList, isReviewComplete]); - const activeOverlayRevealIdentityRef = useRef(null); - const selectedHunkFromAll = useMemo( () => (selectedHunkId ? (allHunks.find((item) => item.id === selectedHunkId) ?? null) : null), [selectedHunkId, allHunks] @@ -734,295 +456,30 @@ export const ImmersiveReviewView: React.FC = (props) = } }, [currentFileHunks, selectedHunkId, onSelectHunk]); - const [activeFileContentState, setActiveFileContentState] = useState<{ - filePath: string | null; - content: string | null; - isSettled: boolean; - }>({ - filePath: null, - content: null, - isSettled: true, - }); - const fileContentCacheRef = useRef>(new Map()); - const shouldLoadFullFileContext = useMemo( - () => shouldAttemptFullFileContext(selectedHunk), - [selectedHunk] - ); - const activeFileContentCacheKey = useMemo( - () => - activeFilePath - ? buildFileContentCacheKey(props.workspaceId, activeFilePath, currentFileHunks) - : null, - [activeFilePath, currentFileHunks, props.workspaceId] - ); - - // Hold diff reveal until overlay geometry swaps have been positioned. File - // switches and same-file content swaps are covered by a scrollport-sized - // shimmer overlay while hidden layout effects scroll the target hunk into place. - const [revealedOverlayIdentity, setRevealedOverlayIdentity] = - useState(null); - const revealAnimationFrameRef = useRef(null); - - // Load full file context only when it is cheap. If disk I/O or highlighting stalls, - // fail open to the compact hunk overlay instead of trapping the review behind loading. - useEffect(() => { - const apiClient = api; - const filePath = activeFilePath; - const cacheKey = activeFileContentCacheKey; - - const settleContent = (content: string | null) => { - setActiveFileContentState({ - filePath: filePath ?? null, - content, - isSettled: true, - }); - }; - - if (!filePath || !apiClient || !shouldLoadFullFileContext || !cacheKey) { - settleContent(null); - return; - } - - if (fileContentCacheRef.current.has(cacheKey)) { - settleContent(fileContentCacheRef.current.get(cacheKey) ?? null); - return; - } - - const resolvedApi: NonNullable = apiClient; - const resolvedFilePath: string = filePath; - const resolvedFileHunks = currentFileHunks; - const resolvedTheme = theme; - let cancelled = false; - - setActiveFileContentState({ - filePath: resolvedFilePath, - content: null, - isSettled: false, - }); - - const settleLoadedContent = (content: string | null, shouldCache: boolean) => { - if (shouldCache) { - fileContentCacheRef.current.set(cacheKey, content); - } - if (!cancelled) { - setActiveFileContentState({ - filePath: resolvedFilePath, - content, - isSettled: true, - }); - } - }; - - async function loadActiveFileContent() { - // Keep plain file reads on the shared container root so immersive review can open - // sibling-project files without forcing the primary repo checkout. - const fileResult = await withFullFileContextTimeout( - resolvedApi.workspace.executeBash({ - workspaceId: props.workspaceId, - script: buildReadFileScript(resolvedFilePath, { - maxSizeBytes: MAX_FULL_FILE_CONTEXT_BYTES, - maxLineCount: MAX_FULL_FILE_CONTEXT_LINES, - }), - }) - ); - - if (cancelled) { - return; - } - - if (fileResult === FULL_FILE_CONTEXT_TIMEOUT) { - settleLoadedContent(null, false); - return; - } - - if (!fileResult.success) { - settleLoadedContent(null, false); - return; - } - - const bashResult = fileResult.data; - const isDeterministicBudgetMiss = - bashResult.exitCode === EXIT_CODE_TOO_LARGE || - bashResult.exitCode === EXIT_CODE_TOO_MANY_LINES; - - if (!bashResult.success && !bashResult.output) { - settleLoadedContent(null, isDeterministicBudgetMiss); - return; - } - - const data = processFileContents(bashResult.output ?? "", bashResult.exitCode); - const content = - data.type === "text" && isWithinFullFileContextLineBudget(data.content) - ? data.content - : null; - - if (content != null) { - const hydratedOverlay = buildOverlayFromFileContent(content, resolvedFileHunks); - if (hydratedOverlay.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES) { - // Preload syntax tokens before swapping compact hunks to full-file context so - // users do not see plain fallback rows flash into colored Shiki spans. - const preloadResult = await withFullFileContextTimeout( - preloadHighlightedDiff({ - content: hydratedOverlay.content, - filePath: resolvedFilePath, - themeMode: resolvedTheme, - }) - ); - if (cancelled) { - return; - } - if (preloadResult === FULL_FILE_CONTEXT_TIMEOUT) { - settleLoadedContent(null, false); - return; - } - } - } - - settleLoadedContent(content, content != null || isDeterministicBudgetMiss); - } - - loadActiveFileContent().catch(() => { - settleLoadedContent(null, false); - }); + const setHunkJumpScroll = useCallback((block: ScrollLogicalPosition) => { + hunkJumpRef.current = true; + hunkJumpScrollBlockRef.current = block; + }, []); - return () => { - cancelled = true; - }; - }, [ - activeFileContentCacheKey, - activeFilePath, + const { + overlayData, + shouldEnableHighlighting, + isActiveOverlayRevealPending, + isActiveOverlayReadyForReveal, + activeOverlayRevealIdentity, + revealLoadingLabel, + revealActiveOverlayNow, + scheduleOverlayReveal, + handleDiffHighlightSettledChange, + } = useImmersiveOverlay({ api, + workspaceId: props.workspaceId, + activeFilePath, currentFileHunks, - props.workspaceId, - shouldLoadFullFileContext, + selectedHunk, theme, - ]); - - const isActiveFileContentSettled = - !activeFilePath || - (activeFileContentState.filePath === activeFilePath && activeFileContentState.isSettled); - - const resolvedActiveFileContent = isActiveFileContentSettled - ? activeFileContentState.content - : null; - - const overlayData = useMemo(() => { - if (currentFileHunks.length === 0) { - return { - content: "", - lineHunkIds: [], - hunkLineRanges: new Map(), - }; - } - - if (resolvedActiveFileContent != null) { - return buildOverlayFromFileContent(resolvedActiveFileContent, currentFileHunks); - } - - return buildOverlayFromHunks(currentFileHunks); - }, [resolvedActiveFileContent, currentFileHunks]); - - const shouldEnableHighlighting = overlayData.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES; - const activeOverlayRevealIdentity = useMemo( - () => (activeFilePath ? { filePath: activeFilePath, content: overlayData.content } : null), - [activeFilePath, overlayData.content] - ); - const activeOverlayHighlightKey = activeOverlayRevealIdentity - ? `${activeOverlayRevealIdentity.filePath}\u0000${activeOverlayRevealIdentity.content}` - : null; - const [settledOverlayHighlightKey, setSettledOverlayHighlightKey] = useState(null); - const isActiveFileRevealPending = - activeFilePath !== null && revealedOverlayIdentity?.filePath !== activeFilePath; - const isActiveOverlayRevealPending = - activeOverlayRevealIdentity !== null && - !isSameOverlayRevealIdentity(revealedOverlayIdentity, activeOverlayRevealIdentity); - const isHappyDomEnvironment = typeof window !== "undefined" && "happyDOM" in window; - // Only worker-backed highlighting is safe to use as a reveal prerequisite; tests and - // other non-worker shells fall back to slower main-thread highlighting. - const canWaitForOverlayHighlight = typeof Worker !== "undefined" && !isHappyDomEnvironment; - const shouldWaitForFullFileContextReveal = Boolean( - isActiveFileRevealPending && shouldLoadFullFileContext && !isActiveFileContentSettled - ); - const shouldWaitForOverlayHighlight = Boolean( - canWaitForOverlayHighlight && - isActiveOverlayRevealPending && - shouldEnableHighlighting && - activeOverlayHighlightKey - ); - const isActiveOverlayHighlightReadyForReveal = - !shouldWaitForOverlayHighlight || settledOverlayHighlightKey === activeOverlayHighlightKey; - const isActiveOverlayReadyForReveal = - !shouldWaitForFullFileContextReveal && isActiveOverlayHighlightReadyForReveal; - // Gate every overlay geometry swap, not just file switches. Same-file hydration - // inserts context rows above the current hunk, so reveal only after the hidden - // layout pass scrolls the selected full-file row into place. - const isActiveOverlayReadyForRevealRef = useRef(isActiveOverlayReadyForReveal); - useLayoutEffect(() => { - isActiveOverlayReadyForRevealRef.current = isActiveOverlayReadyForReveal; - }, [isActiveOverlayReadyForReveal]); - const [revealOverlayHeight, setRevealOverlayHeight] = useState(null); - useLayoutEffect(() => { - if (!isActiveOverlayRevealPending) { - return; - } - - const scrollContainer = scrollContainerRef.current; - if (!scrollContainer) { - return; - } - - const updateRevealOverlayHeight = () => { - const nextHeight = Math.round(scrollContainer.clientHeight); - setRevealOverlayHeight((currentHeight) => - currentHeight === nextHeight ? currentHeight : nextHeight - ); - }; - - updateRevealOverlayHeight(); - const resizeObserver = - typeof ResizeObserver === "undefined" ? null : new ResizeObserver(updateRevealOverlayHeight); - resizeObserver?.observe(scrollContainer); - window.addEventListener("resize", updateRevealOverlayHeight); - - return () => { - resizeObserver?.disconnect(); - window.removeEventListener("resize", updateRevealOverlayHeight); - }; - }, [isActiveOverlayRevealPending]); - const revealLoadingLabel = "Loading file..."; - - const scheduleOverlayReveal = useCallback((overlayIdentity: OverlayRevealIdentity) => { - if (revealAnimationFrameRef.current !== null) { - cancelAnimationFrame(revealAnimationFrameRef.current); - } - - revealAnimationFrameRef.current = window.requestAnimationFrame(() => { - setRevealedOverlayIdentity((currentRevealedIdentity) => { - // A stale rAF from an earlier compact overlay must not reveal while a - // newer full-file hydration is still loading/highlighting. - if (!isActiveOverlayReadyForRevealRef.current) { - return currentRevealedIdentity; - } - - return isSameOverlayRevealIdentity(activeOverlayRevealIdentityRef.current, overlayIdentity) - ? overlayIdentity - : currentRevealedIdentity; - }); - revealAnimationFrameRef.current = null; - }); - }, []); - - const handleDiffHighlightSettledChange = useCallback( - (isSettled: boolean) => { - setSettledOverlayHighlightKey((currentKey) => { - if (isSettled) { - return activeOverlayHighlightKey; - } - return currentKey === activeOverlayHighlightKey ? null : currentKey; - }); - }, - [activeOverlayHighlightKey] - ); + onRevealPending: setHunkJumpScroll, + }); const selectedHunkRange = useMemo( () => (selectedHunk ? (overlayData.hunkLineRanges.get(selectedHunk.id) ?? null) : null), @@ -1116,40 +573,6 @@ export const ImmersiveReviewView: React.FC = (props) = // hide-read auto-advance without changing the main review panel's unread shortcut semantics. const readUndoStackRef = useRef([]); - const setHunkJumpScroll = useCallback((block: ScrollLogicalPosition) => { - hunkJumpRef.current = true; - hunkJumpScrollBlockRef.current = block; - }, []); - - useLayoutEffect(() => { - if (revealAnimationFrameRef.current !== null) { - cancelAnimationFrame(revealAnimationFrameRef.current); - revealAnimationFrameRef.current = null; - } - - activeOverlayRevealIdentityRef.current = activeOverlayRevealIdentity; - - if (!activeOverlayRevealIdentity) { - setRevealedOverlayIdentity(null); - return; - } - - if (isActiveOverlayRevealPending) { - // The pending state is derived during render, not set from an effect, so - // file switches are hidden on their first paint until the scroll effect - // reveals the positioned overlay. - setHunkJumpScroll("center"); - } - }, [activeOverlayRevealIdentity, isActiveOverlayRevealPending, setHunkJumpScroll]); - - useEffect(() => { - return () => { - if (revealAnimationFrameRef.current !== null) { - cancelAnimationFrame(revealAnimationFrameRef.current); - } - }; - }, []); - const selectedHunkRevealTargetLineIndex = selectedHunkRange?.firstModifiedIndex ?? selectedHunkRange?.startIndex ?? null; const revealTargetLineIndex = isActiveOverlayRevealPending @@ -1165,7 +588,7 @@ export const ImmersiveReviewView: React.FC = (props) = // Fail open so the UI cannot get stuck if a file has no hunks. if (currentFileHunks.length === 0) { - setRevealedOverlayIdentity(activeOverlayRevealIdentity); + revealActiveOverlayNow(); return; } @@ -1180,7 +603,7 @@ export const ImmersiveReviewView: React.FC = (props) = // Fail open once selection is stable if we still cannot resolve a reveal target. if (selectedHunkRevealTargetLineIndex === null) { - setRevealedOverlayIdentity(activeOverlayRevealIdentity); + revealActiveOverlayNow(); } }, [ activeOverlayRevealIdentity, @@ -1188,6 +611,7 @@ export const ImmersiveReviewView: React.FC = (props) = hasResolvedSelectedHunkForReveal, isActiveOverlayReadyForReveal, isActiveOverlayRevealPending, + revealActiveOverlayNow, selectedHunkRevealTargetLineIndex, ]); @@ -2382,11 +1806,25 @@ export const ImmersiveReviewView: React.FC = (props) = }; }, [inlineComposerRequest, overlayData.lineHunkIds.length, selectedHunk]); + const immersiveOverlayState = isReviewComplete + ? "complete" + : props.isLoading && currentFileHunks.length === 0 + ? "loading" + : isActiveOverlayRevealPending + ? "pending" + : overlayData.content.length > 0 + ? "revealed" + : "empty"; + return (
= 0 ? currentHunkIdx + 1 : undefined} data-current-file-hunk-count={currentFileHunks.length} data-testid="immersive-review-view" @@ -2619,86 +2057,93 @@ export const ImmersiveReviewView: React.FC = (props) = /> ))} {/* Avoid top padding here; it reads as a blank block between the controls and diff. */} -
- {isActiveOverlayRevealPending && currentFileHunks.length > 0 && !isReviewComplete && ( -
-
- +
+
+ {props.isLoading && currentFileHunks.length === 0 ? ( +
+ Loading diff...
-
- )} - {props.isLoading && currentFileHunks.length === 0 ? ( -
- Loading diff... -
- ) : isReviewComplete ? ( -
+ ) : isReviewComplete ? ( +
+
+
+
+
+

Review complete

+

+ You have already reviewed all {reviewedHunkLabel} in this diff. Return to + chat to keep going, or reopen reviewed hunks from the review panel if you + want another pass. +

+
+ +
+
+ ) : currentFileHunks.length === 0 ? ( +
+ {activeFilePath ? "No hunks for this file" : "No files to review"} +
+ ) : (
-
-
-
-

Review complete

-

- You have already reviewed all {reviewedHunkLabel} in this diff. Return to chat - to keep going, or reopen reviewed hunks from the review panel if you want - another pass. -

-
- + {overlayData.content.length > 0 && ( + + )} +
-
- ) : currentFileHunks.length === 0 ? ( -
- {activeFilePath ? "No hunks for this file" : "No files to review"} -
- ) : ( + )} +
+ {isActiveOverlayRevealPending && currentFileHunks.length > 0 && !isReviewComplete && (
-
- -
+
)}
diff --git a/src/browser/features/RightSidebar/CodeReview/useImmersiveOverlay.ts b/src/browser/features/RightSidebar/CodeReview/useImmersiveOverlay.ts new file mode 100644 index 0000000000..2a7139d77e --- /dev/null +++ b/src/browser/features/RightSidebar/CodeReview/useImmersiveOverlay.ts @@ -0,0 +1,643 @@ +import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"; +import type { APIClient } from "@/browser/contexts/API"; +import type { ThemeMode } from "@/browser/contexts/ThemeContext"; +import { + buildReadFileScript, + EXIT_CODE_TOO_LARGE, + EXIT_CODE_TOO_MANY_LINES, + processFileContents, +} from "@/browser/utils/fileRead"; +import type { DiffHunk } from "@/common/types/review"; +import { preloadHighlightedDiff } from "../../Shared/DiffRenderer"; + +const MAX_FULL_FILE_CONTEXT_LINES = 1500; +const MAX_FULL_FILE_CONTEXT_BYTES = 256 * 1024; +const FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS = 5_000; + +// Keep syntax highlighting on for larger review files now that per-line tooltip overhead is gone, +// but still cap it to avoid pathological DOM costs on extremely large diffs. +const MAX_HIGHLIGHTED_DIFF_LINES = 4000; + +const FULL_FILE_CONTEXT_TIMEOUT = Symbol("full-file-context-timeout"); + +type FullFileContextTimeout = typeof FULL_FILE_CONTEXT_TIMEOUT; + +export interface HunkLineRange { + startIndex: number; + endIndex: number; + firstModifiedIndex: number | null; + lastModifiedIndex: number | null; +} + +export interface ImmersiveOverlayData { + content: string; + /** Small stable key used by reveal/highlight gates instead of re-comparing multi-KB content. */ + contentKey: string; + lineHunkIds: Array; + hunkLineRanges: Map; +} + +export interface OverlayRevealIdentity { + filePath: string; + contentKey: string; +} + +interface ActiveFileContentState { + filePath: string | null; + content: string | null; + isSettled: boolean; +} + +interface UseImmersiveOverlayDataInput { + api: APIClient | null; + workspaceId: string; + activeFilePath: string | null; + currentFileHunks: DiffHunk[]; + selectedHunk: DiffHunk | null; + theme: ThemeMode; +} + +interface UseImmersiveOverlayInput extends UseImmersiveOverlayDataInput { + onRevealPending: (scrollBlock: ScrollLogicalPosition) => void; +} + +interface UseImmersiveOverlayRevealInput { + activeFilePath: string | null; + overlayData: ImmersiveOverlayData; + isActiveFileContentSettled: boolean; + shouldLoadFullFileContext: boolean; + shouldEnableHighlighting: boolean; + onRevealPending: (scrollBlock: ScrollLogicalPosition) => void; +} + +export interface ImmersiveOverlayState { + overlayData: ImmersiveOverlayData; + shouldEnableHighlighting: boolean; + isActiveOverlayRevealPending: boolean; + isActiveOverlayReadyForReveal: boolean; + activeOverlayRevealIdentity: OverlayRevealIdentity | null; + revealLoadingLabel: string; + revealActiveOverlayNow: () => void; + scheduleOverlayReveal: (overlayIdentity: OverlayRevealIdentity) => void; + handleDiffHighlightSettledChange: (isSettled: boolean) => void; +} + +const EMPTY_OVERLAY_DATA: ImmersiveOverlayData = { + content: "", + contentKey: "empty:0:0", + lineHunkIds: [], + hunkLineRanges: new Map(), +}; + +async function withFullFileContextTimeout( + promise: Promise +): Promise { + let timeoutId: ReturnType | null = null; + try { + return await Promise.race([ + promise, + new Promise((resolve) => { + timeoutId = setTimeout( + () => resolve(FULL_FILE_CONTEXT_TIMEOUT), + FULL_FILE_CONTEXT_REVEAL_TIMEOUT_MS + ); + }), + ]); + } finally { + if (timeoutId !== null) { + clearTimeout(timeoutId); + } + } +} + +function splitDiffLines(content: string): string[] { + // Preserve intentionally blank trailing lines but avoid inventing an extra row + // for the final newline that git diffs commonly include. Normalize CRLF hunk + // rows too; full-file context does this separately, but compact overlays are + // built directly from hunk content and should not carry raw \r into diff cells. + const lines = content + .split(/\r?\n/) + .map((line) => (line.endsWith("\r") ? line.slice(0, Math.max(0, line.length - 1)) : line)); + if (lines.length > 1 && lines[lines.length - 1] === "") { + return lines.slice(0, -1); + } + return lines; +} + +function normalizeFileLines(content: string): string[] { + // Normalize Windows CRLF to LF-equivalent lines so rows stay single-height in + // whitespace-preserving diff cells (embedded "\r" can render as extra breaks). + const lines = content + .split(/\r?\n/) + .map((line) => (line.endsWith("\r") ? line.slice(0, Math.max(0, line.length - 1)) : line)); + return lines.filter((line, idx) => idx < lines.length - 1 || line !== ""); +} + +function isWithinFullFileContextLineBudget(content: string): boolean { + return normalizeFileLines(content).length <= MAX_FULL_FILE_CONTEXT_LINES; +} + +function shouldAttemptFullFileContext(selectedHunk: DiffHunk | null): boolean { + if (!selectedHunk) { + return false; + } + + const lastDisplayLine = selectedHunk.newStart + Math.max(selectedHunk.newLines, 1) - 1; + return lastDisplayLine <= MAX_FULL_FILE_CONTEXT_LINES; +} + +function buildFileContentCacheKey( + workspaceId: string, + filePath: string, + sortedHunks: readonly DiffHunk[] +): string { + const hunkSignature = sortedHunks.map((hunk) => hunk.id).join("|"); + return [workspaceId, filePath, hunkSignature].join("\u0000"); +} + +function buildContentKey(content: string, renderedLineCount: number): string { + // Hash once when overlay content is built; reveal/highlight checks run on every + // cursor render and should not allocate or compare the full diff body repeatedly. + let hash = 2166136261; + for (let index = 0; index < content.length; index += 1) { + hash ^= content.charCodeAt(index); + hash = Math.imul(hash, 16777619); + } + return `${content.length}:${renderedLineCount}:${hash >>> 0}`; +} + +function createOverlayData( + contentLines: string[], + lineHunkIds: Array, + hunkLineRanges: Map +): ImmersiveOverlayData { + const content = contentLines.join("\n"); + return { + content, + contentKey: buildContentKey(content, lineHunkIds.length), + lineHunkIds, + hunkLineRanges, + }; +} + +function buildOverlayFromFileContent( + fileContent: string, + sortedHunks: DiffHunk[] +): ImmersiveOverlayData { + const fileLines = normalizeFileLines(fileContent); + const contentLines: string[] = []; + const lineHunkIds: Array = []; + const hunkLineRanges = new Map(); + + let newLineIdx = 0; + + const pushDisplayLine = (line: string, hunkId: string | null) => { + contentLines.push(line); + lineHunkIds.push(hunkId); + }; + + for (const hunk of sortedHunks) { + const hunkStartInNew = Math.max(0, hunk.newStart - 1); + + while (newLineIdx < hunkStartInNew && newLineIdx < fileLines.length) { + pushDisplayLine(` ${fileLines[newLineIdx]}`, null); + newLineIdx += 1; + } + + const hunkStartIndex = lineHunkIds.length; + let firstModifiedIndex: number | null = null; + let lastModifiedIndex: number | null = null; + + for (const line of splitDiffLines(hunk.content)) { + const prefix = line[0] ?? " "; + if (prefix !== "+" && prefix !== "-" && prefix !== " ") { + continue; + } + + if (prefix === "+" || prefix === "-") { + firstModifiedIndex ??= lineHunkIds.length; + lastModifiedIndex = lineHunkIds.length; + } + + pushDisplayLine(`${prefix}${line.slice(1)}`, hunk.id); + if (prefix !== "-") { + newLineIdx += 1; + } + } + + if (lineHunkIds.length > hunkStartIndex) { + hunkLineRanges.set(hunk.id, { + startIndex: hunkStartIndex, + endIndex: lineHunkIds.length - 1, + firstModifiedIndex, + lastModifiedIndex, + }); + } + } + + while (newLineIdx < fileLines.length) { + pushDisplayLine(` ${fileLines[newLineIdx]}`, null); + newLineIdx += 1; + } + + return createOverlayData(contentLines, lineHunkIds, hunkLineRanges); +} + +function buildOverlayFromHunks(sortedHunks: DiffHunk[]): ImmersiveOverlayData { + const contentLines: string[] = []; + const lineHunkIds: Array = []; + const hunkLineRanges = new Map(); + + const pushDisplayLine = (line: string, hunkId: string | null) => { + contentLines.push(line); + lineHunkIds.push(hunkId); + }; + + const pushHeaderLine = (line: string) => { + // Header rows are intentionally excluded from lineHunkIds because DiffRenderer + // does not render @@ header lines in selectable output. + contentLines.push(line); + }; + + sortedHunks.forEach((hunk, index) => { + if (index > 0) { + pushDisplayLine(" ", null); + } + + pushHeaderLine(`@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`); + + const hunkStartIndex = lineHunkIds.length; + let firstModifiedIndex: number | null = null; + let lastModifiedIndex: number | null = null; + + for (const line of splitDiffLines(hunk.content)) { + const prefix = line[0] ?? " "; + if (prefix !== "+" && prefix !== "-" && prefix !== " ") { + continue; + } + + if (prefix === "+" || prefix === "-") { + firstModifiedIndex ??= lineHunkIds.length; + lastModifiedIndex = lineHunkIds.length; + } + + pushDisplayLine(`${prefix}${line.slice(1)}`, hunk.id); + } + + if (lineHunkIds.length > hunkStartIndex) { + hunkLineRanges.set(hunk.id, { + startIndex: hunkStartIndex, + endIndex: lineHunkIds.length - 1, + firstModifiedIndex, + lastModifiedIndex, + }); + } + }); + + return createOverlayData(contentLines, lineHunkIds, hunkLineRanges); +} + +export function isSameOverlayRevealIdentity( + lhs: OverlayRevealIdentity | null, + rhs: OverlayRevealIdentity | null +): boolean { + return lhs?.filePath === rhs?.filePath && lhs?.contentKey === rhs?.contentKey; +} + +function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { + const [activeFileContentState, setActiveFileContentState] = useState({ + filePath: null, + content: null, + isSettled: true, + }); + const fileContentCacheRef = useRef>(new Map()); + const shouldLoadFullFileContext = useMemo( + () => shouldAttemptFullFileContext(input.selectedHunk), + [input.selectedHunk] + ); + const activeFileContentCacheKey = useMemo( + () => + input.activeFilePath + ? buildFileContentCacheKey(input.workspaceId, input.activeFilePath, input.currentFileHunks) + : null, + [input.activeFilePath, input.currentFileHunks, input.workspaceId] + ); + + // Load full file context only when it is cheap. If disk I/O or highlighting stalls, + // fail open to the compact hunk overlay instead of trapping the review behind loading. + useEffect(() => { + const apiClient = input.api; + const filePath = input.activeFilePath; + const cacheKey = activeFileContentCacheKey; + + const settleContent = (content: string | null) => { + setActiveFileContentState({ + filePath: filePath ?? null, + content, + isSettled: true, + }); + }; + + if (!filePath || !apiClient || !shouldLoadFullFileContext || !cacheKey) { + settleContent(null); + return; + } + + if (fileContentCacheRef.current.has(cacheKey)) { + settleContent(fileContentCacheRef.current.get(cacheKey) ?? null); + return; + } + + const resolvedApi = apiClient; + const resolvedFilePath = filePath; + const resolvedFileHunks = input.currentFileHunks; + const resolvedTheme = input.theme; + let cancelled = false; + + setActiveFileContentState({ + filePath: resolvedFilePath, + content: null, + isSettled: false, + }); + + const settleLoadedContent = (content: string | null, shouldCache: boolean) => { + if (shouldCache) { + fileContentCacheRef.current.set(cacheKey, content); + } + if (!cancelled) { + setActiveFileContentState({ + filePath: resolvedFilePath, + content, + isSettled: true, + }); + } + }; + + async function loadActiveFileContent() { + // Keep plain file reads on the shared container root so immersive review can open + // sibling-project files without forcing the primary repo checkout. + const fileResult = await withFullFileContextTimeout( + resolvedApi.workspace.executeBash({ + workspaceId: input.workspaceId, + script: buildReadFileScript(resolvedFilePath, { + maxSizeBytes: MAX_FULL_FILE_CONTEXT_BYTES, + maxLineCount: MAX_FULL_FILE_CONTEXT_LINES, + }), + }) + ); + + if (cancelled) { + return; + } + + if (fileResult === FULL_FILE_CONTEXT_TIMEOUT) { + settleLoadedContent(null, false); + return; + } + + if (!fileResult.success) { + settleLoadedContent(null, false); + return; + } + + const bashResult = fileResult.data; + const isDeterministicBudgetMiss = + bashResult.exitCode === EXIT_CODE_TOO_LARGE || + bashResult.exitCode === EXIT_CODE_TOO_MANY_LINES; + + if (!bashResult.success && !bashResult.output) { + settleLoadedContent(null, isDeterministicBudgetMiss); + return; + } + + const data = processFileContents(bashResult.output ?? "", bashResult.exitCode); + const content = + data.type === "text" && isWithinFullFileContextLineBudget(data.content) + ? data.content + : null; + + if (content != null) { + const hydratedOverlay = buildOverlayFromFileContent(content, resolvedFileHunks); + if (hydratedOverlay.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES) { + // Preload syntax tokens before swapping compact hunks to full-file context so + // users do not see plain fallback rows flash into colored Shiki spans. + const preloadResult = await withFullFileContextTimeout( + preloadHighlightedDiff({ + content: hydratedOverlay.content, + filePath: resolvedFilePath, + themeMode: resolvedTheme, + }) + ); + if (cancelled) { + return; + } + if (preloadResult === FULL_FILE_CONTEXT_TIMEOUT) { + settleLoadedContent(null, false); + return; + } + } + } + + settleLoadedContent(content, content != null || isDeterministicBudgetMiss); + } + + loadActiveFileContent().catch(() => { + settleLoadedContent(null, false); + }); + + return () => { + cancelled = true; + }; + }, [ + activeFileContentCacheKey, + input.activeFilePath, + input.api, + input.currentFileHunks, + input.theme, + input.workspaceId, + shouldLoadFullFileContext, + ]); + + const isActiveFileContentSettled = + !input.activeFilePath || + (activeFileContentState.filePath === input.activeFilePath && activeFileContentState.isSettled); + + const resolvedActiveFileContent = + isActiveFileContentSettled && shouldLoadFullFileContext ? activeFileContentState.content : null; + const shouldDeferOverlayRender = Boolean( + shouldLoadFullFileContext && !isActiveFileContentSettled + ); + + const overlayData = useMemo(() => { + if (input.currentFileHunks.length === 0 || shouldDeferOverlayRender) { + return EMPTY_OVERLAY_DATA; + } + + if (resolvedActiveFileContent != null) { + return buildOverlayFromFileContent(resolvedActiveFileContent, input.currentFileHunks); + } + + return buildOverlayFromHunks(input.currentFileHunks); + }, [input.currentFileHunks, resolvedActiveFileContent, shouldDeferOverlayRender]); + + const shouldEnableHighlighting = overlayData.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES; + + return { + overlayData, + shouldEnableHighlighting, + shouldLoadFullFileContext, + isActiveFileContentSettled, + }; +} + +function useImmersiveOverlayReveal(input: UseImmersiveOverlayRevealInput) { + const { onRevealPending } = input; + const activeOverlayRevealIdentity = useMemo( + () => + input.activeFilePath + ? { filePath: input.activeFilePath, contentKey: input.overlayData.contentKey } + : null, + [input.activeFilePath, input.overlayData.contentKey] + ); + const activeOverlayHighlightKey = activeOverlayRevealIdentity + ? `${activeOverlayRevealIdentity.filePath}\u0000${activeOverlayRevealIdentity.contentKey}` + : null; + const [settledOverlayHighlightKey, setSettledOverlayHighlightKey] = useState(null); + + // Hold diff reveal until overlay geometry swaps have been positioned. File + // switches and same-file content swaps are covered by a scrollport-sized + // shimmer overlay while hidden layout effects scroll the target hunk into place. + const [revealedOverlayIdentity, setRevealedOverlayIdentity] = + useState(null); + const revealAnimationFrameRef = useRef(null); + const activeOverlayRevealIdentityRef = useRef(null); + + const isActiveOverlayRevealPending = + activeOverlayRevealIdentity !== null && + !isSameOverlayRevealIdentity(revealedOverlayIdentity, activeOverlayRevealIdentity); + const isHappyDomEnvironment = typeof window !== "undefined" && "happyDOM" in window; + // Only worker-backed highlighting is safe to use as a reveal prerequisite; tests and + // other non-worker shells fall back to slower main-thread highlighting. + const canWaitForOverlayHighlight = typeof Worker !== "undefined" && !isHappyDomEnvironment; + const shouldWaitForFullFileContextReveal = Boolean( + isActiveOverlayRevealPending && + input.shouldLoadFullFileContext && + !input.isActiveFileContentSettled + ); + const shouldWaitForOverlayHighlight = Boolean( + canWaitForOverlayHighlight && + isActiveOverlayRevealPending && + input.shouldEnableHighlighting && + input.overlayData.lineHunkIds.length > 0 && + activeOverlayHighlightKey + ); + const isActiveOverlayHighlightReadyForReveal = + !shouldWaitForOverlayHighlight || settledOverlayHighlightKey === activeOverlayHighlightKey; + const isActiveOverlayReadyForReveal = + !shouldWaitForFullFileContextReveal && isActiveOverlayHighlightReadyForReveal; + + // Gate every overlay geometry swap, not just file switches. Same-file hydration + // inserts context rows above the current hunk, so reveal only after the hidden + // layout pass scrolls the selected full-file row into place. + const isActiveOverlayReadyForRevealRef = useRef(isActiveOverlayReadyForReveal); + useLayoutEffect(() => { + isActiveOverlayReadyForRevealRef.current = isActiveOverlayReadyForReveal; + }, [isActiveOverlayReadyForReveal]); + + const revealActiveOverlayNow = useCallback(() => { + const activeIdentity = activeOverlayRevealIdentityRef.current; + if (activeIdentity) { + setRevealedOverlayIdentity(activeIdentity); + } + }, []); + + const scheduleOverlayReveal = useCallback((overlayIdentity: OverlayRevealIdentity) => { + if (revealAnimationFrameRef.current !== null) { + cancelAnimationFrame(revealAnimationFrameRef.current); + } + + revealAnimationFrameRef.current = window.requestAnimationFrame(() => { + setRevealedOverlayIdentity((currentRevealedIdentity) => { + // A stale rAF from an earlier compact overlay must not reveal while a + // newer full-file hydration is still loading/highlighting. + if (!isActiveOverlayReadyForRevealRef.current) { + return currentRevealedIdentity; + } + + return isSameOverlayRevealIdentity(activeOverlayRevealIdentityRef.current, overlayIdentity) + ? overlayIdentity + : currentRevealedIdentity; + }); + revealAnimationFrameRef.current = null; + }); + }, []); + + const handleDiffHighlightSettledChange = useCallback( + (isSettled: boolean) => { + setSettledOverlayHighlightKey((currentKey) => { + if (isSettled) { + return activeOverlayHighlightKey; + } + return currentKey === activeOverlayHighlightKey ? null : currentKey; + }); + }, + [activeOverlayHighlightKey] + ); + + useLayoutEffect(() => { + if (revealAnimationFrameRef.current !== null) { + cancelAnimationFrame(revealAnimationFrameRef.current); + revealAnimationFrameRef.current = null; + } + + activeOverlayRevealIdentityRef.current = activeOverlayRevealIdentity; + + if (!activeOverlayRevealIdentity) { + setRevealedOverlayIdentity(null); + return; + } + + if (isActiveOverlayRevealPending) { + // The pending state is derived during render, not set from an effect, so + // file switches are hidden on their first paint until the scroll effect + // reveals the positioned overlay. + onRevealPending("center"); + } + }, [activeOverlayRevealIdentity, isActiveOverlayRevealPending, onRevealPending]); + + useEffect(() => { + return () => { + if (revealAnimationFrameRef.current !== null) { + cancelAnimationFrame(revealAnimationFrameRef.current); + } + }; + }, []); + + return { + activeOverlayRevealIdentity, + isActiveOverlayRevealPending, + isActiveOverlayReadyForReveal, + revealLoadingLabel: "Loading file...", + revealActiveOverlayNow, + scheduleOverlayReveal, + handleDiffHighlightSettledChange, + }; +} + +export function useImmersiveOverlay(input: UseImmersiveOverlayInput): ImmersiveOverlayState { + const overlayDataState = useImmersiveOverlayData(input); + const revealState = useImmersiveOverlayReveal({ + activeFilePath: input.activeFilePath, + overlayData: overlayDataState.overlayData, + isActiveFileContentSettled: overlayDataState.isActiveFileContentSettled, + shouldLoadFullFileContext: overlayDataState.shouldLoadFullFileContext, + shouldEnableHighlighting: overlayDataState.shouldEnableHighlighting, + onRevealPending: input.onRevealPending, + }); + + return { + overlayData: overlayDataState.overlayData, + shouldEnableHighlighting: overlayDataState.shouldEnableHighlighting, + ...revealState, + }; +} diff --git a/src/browser/features/Shared/DiffRenderer.tsx b/src/browser/features/Shared/DiffRenderer.tsx index 54d5a647e2..e61dcdf46d 100644 --- a/src/browser/features/Shared/DiffRenderer.tsx +++ b/src/browser/features/Shared/DiffRenderer.tsx @@ -562,47 +562,65 @@ function useHighlightedDiff( newStart: number, themeMode: ThemeMode ): HighlightedDiffState { - const cacheKey = getDiffCacheKey(content, language, oldStart, newStart, themeMode); + const cacheKey = useMemo( + () => getDiffCacheKey(content, language, oldStart, newStart, themeMode), + [content, language, oldStart, newStart, themeMode] + ); const cachedResult = highlightedDiffCache.get(cacheKey); const isPlainText = isPlainTextLanguage(language); - // Sync fallback: plain-text chunks for instant render. + // Sync fallback: plain-text chunks for instant render. Cache hits return the + // preloaded highlighted chunks directly so tooltip/selection state inside the + // renderer does not rebuild a throwaway full-file fallback on every local render. const plainText = useMemo( - () => createPlainTextChunks(content, oldStart, newStart), - [content, oldStart, newStart] + () => cachedResult ?? createPlainTextChunks(content, oldStart, newStart), + [cachedResult, content, oldStart, newStart] ); - const [state, setState] = useState({ + const [state, setState] = useState(() => ({ cacheKey, chunks: cachedResult ?? plainText, isSettled: cachedResult != null || isPlainText, - }); + })); useEffect(() => { + const setHighlightedState = (nextState: HighlightedDiffState) => { + setState((currentState) => { + if ( + currentState.cacheKey === nextState.cacheKey && + currentState.chunks === nextState.chunks && + currentState.isSettled === nextState.isSettled + ) { + return currentState; + } + return nextState; + }); + }; + const cached = highlightedDiffCache.get(cacheKey); if (cached) { - setState({ cacheKey, chunks: cached, isSettled: true }); + setHighlightedState({ cacheKey, chunks: cached, isSettled: true }); return; } if (isPlainText) { - setState({ cacheKey, chunks: plainText, isSettled: true }); + setHighlightedState({ cacheKey, chunks: plainText, isSettled: true }); return; } // Show plain text immediately unless a caller preloaded this diff into the cache. - setState({ cacheKey, chunks: plainText, isSettled: false }); + setHighlightedState({ cacheKey, chunks: plainText, isSettled: false }); let cancelled = false; loadHighlightedDiff({ content, language, oldStart, newStart, themeMode }) .then((highlighted) => { if (!cancelled) { - setState({ cacheKey, chunks: highlighted, isSettled: true }); + setHighlightedState({ cacheKey, chunks: highlighted, isSettled: true }); } }) .catch(() => { if (!cancelled) { - setState({ cacheKey, chunks: plainText, isSettled: true }); + setHighlightedState({ cacheKey, chunks: plainText, isSettled: true }); } }); diff --git a/tests/e2e/scenarios/reviewHydration.spec.ts b/tests/e2e/scenarios/reviewHydration.spec.ts index e493b62c1e..a24b189b55 100644 --- a/tests/e2e/scenarios/reviewHydration.spec.ts +++ b/tests/e2e/scenarios/reviewHydration.spec.ts @@ -4,6 +4,7 @@ import { REVIEW_SORT_ORDER_KEY } from "../../../src/common/constants/storage"; import { STORAGE_KEYS } from "../../../src/constants/workspaceDefaults"; import { disableReviewTutorial, + seedLargeReviewSingleFileDiff, seedReviewHydrationJumpDiff, seedReviewMarkReadIterationDiff, } from "../utils/reviewPerfFixture"; @@ -17,14 +18,20 @@ interface HydrationSample { t: number; overlayVisible: boolean; stageVisible: boolean; + minimapStageVisible: boolean; lineCount: number; + overlayLineCount: number | null; + selectedLineIndex: number | null; scrollTop: number | null; overlayTop: number | null; overlayHeight: number | null; scrollContainerTop: number | null; scrollContainerHeight: number | null; + minimapCanvasTop: number | null; + minimapCanvasHeight: number | null; selectedTop: number | null; - stageText: string; + activeFilePath: string | null; + overlayState: string | null; } async function primeReviewForHeadDiff( @@ -72,23 +79,44 @@ async function startHydrationSampler( }; const sample = () => { + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); const stage = document.querySelector( '[data-testid="immersive-diff-reveal-stage"]' ); const overlay = document.querySelector( '[data-testid="immersive-diff-reveal-overlay"]' ); + const minimapStage = document.querySelector( + '[data-testid="immersive-minimap-reveal-stage"]' + ); + const minimapCanvas = document.querySelector( + '[data-testid="immersive-minimap-canvas"]' + ); const lines = stage ? Array.from(stage.querySelectorAll("div[data-line-index]")) : []; + const selectedLineIndexText = + immersiveView?.dataset.selectedLineIndex ?? stage?.dataset.selectedLineIndex ?? null; + const parsedSelectedLineIndex = + selectedLineIndexText == null ? Number.NaN : Number.parseInt(selectedLineIndexText, 10); + const sampledLineIndex = + targetLineIndex ?? + (Number.isFinite(parsedSelectedLineIndex) ? parsedSelectedLineIndex : null); const selectedLine = - targetLineIndex == null + sampledLineIndex == null ? null - : (stage?.querySelector(`div[data-line-index="${targetLineIndex}"]`) ?? + : (stage?.querySelector(`div[data-line-index="${sampledLineIndex}"]`) ?? null); const scrollContainer = stage?.closest(".overflow-y-auto") ?? null; const overlayRect = overlay?.getBoundingClientRect() ?? null; const scrollContainerRect = scrollContainer?.getBoundingClientRect() ?? null; + const minimapCanvasRect = minimapCanvas?.getBoundingClientRect() ?? null; + const overlayLineCountText = + immersiveView?.dataset.overlayLineCount ?? stage?.dataset.overlayLineCount ?? null; + const overlayLineCount = + overlayLineCountText == null ? Number.NaN : Number.parseInt(overlayLineCountText, 10); samples.push({ t: performance.now() - startedAt, @@ -97,11 +125,18 @@ async function startHydrationSampler( overlayHeight: overlayRect?.height ?? null, scrollContainerTop: scrollContainerRect?.top ?? null, scrollContainerHeight: scrollContainerRect?.height ?? null, + minimapStageVisible: isVisible(minimapStage), + minimapCanvasTop: minimapCanvasRect?.top ?? null, + minimapCanvasHeight: minimapCanvasRect?.height ?? null, stageVisible: isVisible(stage), lineCount: lines.length, + overlayLineCount: Number.isFinite(overlayLineCount) ? overlayLineCount : null, + selectedLineIndex: sampledLineIndex, scrollTop: scrollContainer?.scrollTop ?? null, selectedTop: selectedLine?.getBoundingClientRect().top ?? null, - stageText: stage?.textContent ?? "", + activeFilePath: + immersiveView?.dataset.activeFilePath ?? stage?.dataset.activeFilePath ?? null, + overlayState: immersiveView?.dataset.overlayState ?? stage?.dataset.overlayState ?? null, }); if (running) { @@ -195,7 +230,7 @@ test.describe("immersive review hydration stability", () => { }) ); - while (!immersiveView.textContent?.includes(filePath)) { + while (immersiveView.dataset.activeFilePath !== filePath) { await waitForFrame(); } await waitForFrame(); @@ -222,11 +257,15 @@ test.describe("immersive review hydration stability", () => { ).toBeLessThanOrEqual(2); } - const hiddenStageSamples = samples.filter( - (sample) => !sample.stageVisible && sample.lineCount > 0 + expect(overlaySamples.every((sample) => !sample.minimapStageVisible)).toBe(true); + + const hiddenRevealSamples = samples.filter( + (sample) => !sample.stageVisible && sample.overlayVisible ); - expect(hiddenStageSamples.length).toBeGreaterThan(0); - expect(hiddenStageSamples.every((sample) => sample.overlayVisible)).toBe(true); + expect(hiddenRevealSamples.length).toBeGreaterThan(0); + expect( + samples.filter((sample) => !sample.stageVisible).every((sample) => sample.overlayVisible) + ).toBe(true); }); test("keeps Shift+M file-read advancement covered while read hunks are hidden", async ({ @@ -287,7 +326,7 @@ test.describe("immersive review hydration stability", () => { }) ); - while (!immersiveView.textContent?.includes(nextFilePath)) { + while (immersiveView.dataset.activeFilePath !== nextFilePath) { await waitForFrame(); } await waitForFrame(); @@ -299,14 +338,21 @@ test.describe("immersive review hydration stability", () => { const samples = await stopHydrationSampler(page); const staleVisibleSamples = samples.filter( (sample) => - sample.stageVisible && !sample.overlayVisible && sample.stageText.includes("ready-001-001") + sample.stageVisible && + !sample.overlayVisible && + sample.activeFilePath !== null && + sample.activeFilePath !== diffSummary.filePaths[1] ); expect(staleVisibleSamples).toEqual([]); - const hiddenStageSamples = samples.filter( - (sample) => !sample.stageVisible && sample.lineCount > 0 - ); - expect(hiddenStageSamples.every((sample) => sample.overlayVisible)).toBe(true); + expect( + samples + .filter((sample) => sample.overlayVisible) + .every((sample) => !sample.minimapStageVisible) + ).toBe(true); + expect( + samples.filter((sample) => !sample.stageVisible).every((sample) => sample.overlayVisible) + ).toBe(true); }); test("keeps compact-to-full file hydration hidden until selected hunk geometry is stable", async ({ @@ -366,6 +412,7 @@ test.describe("immersive review hydration stability", () => { const overlayTops = visibleOverlaySamples.map((sample) => sample.overlayTop ?? 0); const overlayTopRange = Math.max(...overlayTops) - Math.min(...overlayTops); expect(overlayTopRange).toBeLessThanOrEqual(2); + expect(visibleOverlaySamples.every((sample) => !sample.minimapStageVisible)).toBe(true); const visibleSelectedSamples = samples.filter( (sample) => @@ -380,4 +427,118 @@ test.describe("immersive review hydration stability", () => { const selectedTopRange = Math.max(...selectedTops) - Math.min(...selectedTops); expect(selectedTopRange).toBeLessThanOrEqual(2); }); + + test("keeps same-file J/K hunk iteration visually stable after hydration", async ({ + page, + ui, + workspace, + }) => { + await disableReviewTutorial(page); + const diffSummary = seedLargeReviewSingleFileDiff(workspace.demoProject.workspacePath, { + hunkCount: 24, + hunkSpacing: 20, + lineCount: 600, + }); + const expectedFullOverlayLineCount = diffSummary.lineCount + diffSummary.deletedLines; + + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText(`0/${diffSummary.hunkCount}`, { exact: true })).toBeVisible({ + timeout: 20_000, + }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); + + const diffLineContainers = immersiveReview.locator( + '[data-testid="immersive-diff-reveal-stage"] div[data-line-index]' + ); + await expect(diffLineContainers).toHaveCount(expectedFullOverlayLineCount, { timeout: 20_000 }); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + + await startHydrationSampler(page, null); + await immersiveReview.focus(); + await page.evaluate(async () => { + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); + if (!immersiveView) { + throw new Error("Immersive review view was not found"); + } + + const waitForFrame = () => + new Promise((resolve) => window.requestAnimationFrame(() => resolve())); + + for (let step = 0; step < 10; step += 1) { + window.dispatchEvent( + new KeyboardEvent("keydown", { + key: "j", + bubbles: true, + cancelable: true, + }) + ); + + const expectedPosition = String(step + 2); + while (immersiveView.dataset.selectedHunkPosition !== expectedPosition) { + await waitForFrame(); + } + await waitForFrame(); + } + }); + + const samples = await stopHydrationSampler(page); + expect(samples.length).toBeGreaterThan(0); + + const visibleCompactSamples = samples.filter( + (sample) => + sample.stageVisible && + !sample.overlayVisible && + (sample.overlayLineCount ?? sample.lineCount) > 0 && + (sample.overlayLineCount ?? sample.lineCount) < expectedFullOverlayLineCount + ); + expect(visibleCompactSamples).toEqual([]); + + const visibleSelectedSamples = samples.filter( + (sample) => + sample.stageVisible && + !sample.overlayVisible && + sample.selectedTop !== null && + sample.scrollContainerTop !== null && + sample.scrollContainerHeight !== null + ); + expect(visibleSelectedSamples.length).toBeGreaterThan(0); + for (const sample of visibleSelectedSamples) { + const containerTop = sample.scrollContainerTop ?? 0; + const containerBottom = containerTop + (sample.scrollContainerHeight ?? 0); + expect(sample.selectedTop ?? 0).toBeGreaterThanOrEqual(containerTop - 4); + expect(sample.selectedTop ?? 0).toBeLessThanOrEqual(containerBottom + 4); + } + + const scrollSamples = samples + .filter( + (sample) => sample.stageVisible && !sample.overlayVisible && sample.scrollTop !== null + ) + .map((sample) => sample.scrollTop ?? 0); + expect(scrollSamples.length).toBeGreaterThan(0); + let furthestScrollTop = scrollSamples[0] ?? 0; + for (const scrollTop of scrollSamples) { + expect(scrollTop).toBeGreaterThanOrEqual(furthestScrollTop - 4); + furthestScrollTop = Math.max(furthestScrollTop, scrollTop); + } + }); }); From 7b55af3052c879388a1499a533a6c53db63045b4 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 7 Jun 2026 19:33:38 -0500 Subject: [PATCH 3/3] fix excess loading states during same-file hunk iteration Marking a hunk read (or any same-file hunk-set change) used to invalidate the full-file cache and replay the reveal skeleton. Key the file-content cache by (path, content-version), scope overlay deferral to genuine file switches, and gate the loading cover on file-path changes so same-file iteration reveals in place. The content-version is derived from the file's unfiltered diff content, so a real file change (tool edit / diff refresh) still busts the cache while mark-read filtering does not. --- .../CodeReview/ImmersiveReviewView.test.tsx | 150 ++++++++++----- .../CodeReview/ImmersiveReviewView.tsx | 24 ++- .../CodeReview/useImmersiveOverlay.ts | 172 ++++++++++++++---- tests/e2e/scenarios/reviewHydration.spec.ts | 86 +++++++++ 4 files changed, 352 insertions(+), 80 deletions(-) diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 387323a07b..2b64017fce 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx @@ -369,39 +369,45 @@ describe("ImmersiveReviewView", () => { ); }); - test("keeps same-file compact-to-full hydration behind the reveal gate", async () => { - const nearHunk = createHunk({ - id: "hunk-near", - newStart: 40, + test("keeps same-file hunk-set changes instant without re-reading or re-covering", async () => { + // Two in-budget hunks in one file. After the file hydrates, marking a hunk read while + // read hunks are hidden removes it from the visible set and rebuilds the overlay. That + // rebuild must stay an in-memory, instant operation: no second file read (the body is + // cached per file path) and no loading cover (the file is already on screen). + const hunkA = createHunk({ + id: "hunk-a", + newStart: 5, newLines: 1, - header: "@@ -40 +40 @@", - content: "-old near line\n+new near line", + header: "@@ -5 +5 @@", + content: "-old a\n+new a", }); - const farHunk = createHunk({ - id: "hunk-far", - newStart: 5000, + const hunkB = createHunk({ + id: "hunk-b", + newStart: 10, newLines: 1, - header: "@@ -5000 +5000 @@", - content: "-old far line\n+new far line", + header: "@@ -10 +10 @@", + content: "-old b\n+new b", }); - type ExecuteBashResult = Awaited>; - let resolveRead: (result: ExecuteBashResult) => void = () => { - throw new Error("executeBash was not called"); - }; - mockApi.workspace.executeBash = mock( - () => - new Promise((resolve) => { - resolveRead = resolve; - }) - ); - const renderView = (selectedHunkId: string) => ( + const fileBody = `${Array.from({ length: 20 }, (_, index) => `file line ${index + 1}`).join( + "\n" + )}\n`; + let readCount = 0; + mockApi.workspace.executeBash = mock(() => { + readCount += 1; + return Promise.resolve({ + success: true as const, + data: { success: true, output: encodeFileReadOutput(fileBody), exitCode: 0 }, + }); + }); + + const renderView = (hunks: DiffHunk[], selectedHunkId: string) => ( false} onToggleRead={mock(() => undefined)} onMarkFileAsRead={mock(() => undefined)} @@ -415,29 +421,89 @@ describe("ImmersiveReviewView", () => { ); - const view = render(renderView(farHunk.id)); - expect(view.container.textContent ?? "").toContain("new far line"); - expect(mockApi.workspace.executeBash).not.toHaveBeenCalled(); + const view = render(renderView([hunkA, hunkB], hunkA.id)); - view.rerender(renderView(nearHunk.id)); + // Full-file context hydrates and the loading cover clears (file is on screen). + await waitFor(() => expect(view.container.textContent ?? "").toContain("file line 12")); + await waitFor(() => expect(view.queryByTestId("immersive-diff-reveal-overlay")).toBeNull()); + const readsAfterHydration = readCount; + expect(readsAfterHydration).toBeGreaterThanOrEqual(1); - await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); - expect(view.getByTestId("immersive-diff-reveal-skeleton")).toBeTruthy(); - expect(view.container.textContent ?? "").not.toContain("new near line"); - expect(view.container.textContent ?? "").not.toContain("new far line"); + // Mark hunk A read (hidden): the visible hunk set shrinks to [hunkB] in the same file. + view.rerender(renderView([hunkB], hunkB.id)); - resolveRead({ - success: true as const, - data: { - success: true, - output: encodeFileReadOutput("new near line\ncontext after selected hunk\n"), - exitCode: 0, - }, + // Give any (incorrect) re-read / re-cover a chance to appear before asserting it did not. + await new Promise((resolve) => setTimeout(resolve, 80)); + expect(readCount).toBe(readsAfterHydration); + expect(view.queryByTestId("immersive-diff-reveal-overlay")).toBeNull(); + expect(view.queryByTestId("immersive-diff-reveal-skeleton")).toBeNull(); + expect(view.container.textContent ?? "").toContain("file line 12"); + }); + + test("re-reads the full-file body when the file's diff content changes", async () => { + const baseHunk = createHunk({ + id: "hunk-a", + newStart: 5, + newLines: 1, + header: "@@ -5 +5 @@", + content: "-old a\n+new a v1", + }); + // Same id (hash of path + line ranges) but different content, as if a tool edited the + // file in place and the diff was re-fetched. Content -- not the id -- must invalidate + // the cached full-file body so reviewers never see stale surrounding lines. + const editedHunk = createHunk({ + id: "hunk-a", + newStart: 5, + newLines: 1, + header: "@@ -5 +5 @@", + content: "-old a\n+new a v2", }); - await waitFor(() => - expect(view.container.textContent ?? "").toContain("context after selected hunk") + const fileBody = `${Array.from({ length: 20 }, (_, index) => `file line ${index + 1}`).join( + "\n" + )}\n`; + let readCount = 0; + mockApi.workspace.executeBash = mock(() => { + readCount += 1; + return Promise.resolve({ + success: true as const, + data: { success: true, output: encodeFileReadOutput(fileBody), exitCode: 0 }, + }); + }); + + const renderView = (hunk: DiffHunk) => ( + + false} + onToggleRead={mock(() => undefined)} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={hunk.id} + onSelectHunk={mock(() => undefined)} + onExit={mock(() => undefined)} + isTouchImmersive={true} + reviewsByFilePath={new Map()} + firstSeenMap={{}} + /> + ); + + const view = render(renderView(baseHunk)); + // Full-file context hydrates: a context-only line from the file body is visible. + await waitFor(() => expect(view.container.textContent ?? "").toContain("file line 12")); + const readsAfterFirstHydration = readCount; + + // The diff content changed in place. The cached body must be invalidated, and the stale + // full-file context must NOT remain on screen while the new body re-reads -- settled + // state is tracked by content version, so the overlay drops to the compact hunk (which + // does not include the file-body context line) until the fresh body loads. + view.rerender(renderView(editedHunk)); + expect(view.container.textContent ?? "").not.toContain("file line 12"); + await waitFor(() => expect(readCount).toBeGreaterThan(readsAfterFirstHydration)); + await waitFor(() => expect(view.container.textContent ?? "").toContain("file line 12")); }); test("loads full-file context for an in-budget selected hunk even when another hunk is far away", async () => { diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx index 1fac3ebedc..ae37a65716 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx @@ -25,6 +25,7 @@ import { ImmersiveDiffRevealLoadingState } from "./ImmersiveDiffRevealLoadingSta import { ImmersiveMinimap } from "./ImmersiveMinimap"; import { ImmersiveReviewAgentStatusBar } from "./ImmersiveReviewAgentStatusBar"; import { + buildFileHunksContentVersion, useImmersiveOverlay, type HunkLineRange, type ImmersiveOverlayData, @@ -423,6 +424,17 @@ export const ImmersiveReviewView: React.FC = (props) = [activeFileHunks, activeFilePath] ); + // Version the cached full-file body by the active file's UNFILTERED diff content so it is + // re-read when the file's diff actually changes (a tool edits it / the diff is refreshed) + // but reused when a hunk is only filtered out by mark-read. `allHunks` is the complete + // diff set for the workspace (read-state independent), so this stays stable across + // marking hunks read while still busting on a real content change. + const activeFileContentVersion = useMemo( + () => + activeFilePath ? buildFileHunksContentVersion(getFileHunks(allHunks, activeFilePath)) : "", + [allHunks, activeFilePath] + ); + const selectedHunk = useMemo(() => { if (selectedHunkId) { const matchingHunk = currentFileHunks.find((hunk) => hunk.id === selectedHunkId); @@ -465,6 +477,7 @@ export const ImmersiveReviewView: React.FC = (props) = overlayData, shouldEnableHighlighting, isActiveOverlayRevealPending, + isActiveFileRevealPending, isActiveOverlayReadyForReveal, activeOverlayRevealIdentity, revealLoadingLabel, @@ -478,6 +491,7 @@ export const ImmersiveReviewView: React.FC = (props) = currentFileHunks, selectedHunk, theme, + fileContentVersion: activeFileContentVersion, onRevealPending: setHunkJumpScroll, }); @@ -1810,7 +1824,7 @@ export const ImmersiveReviewView: React.FC = (props) = ? "complete" : props.isLoading && currentFileHunks.length === 0 ? "loading" - : isActiveOverlayRevealPending + : isActiveFileRevealPending ? "pending" : overlayData.content.length > 0 ? "revealed" @@ -2100,11 +2114,11 @@ export const ImmersiveReviewView: React.FC = (props) =
= (props) =
)}
- {isActiveOverlayRevealPending && currentFileHunks.length > 0 && !isReviewComplete && ( + {isActiveFileRevealPending && currentFileHunks.length > 0 && !isReviewComplete && (
= (props) = {!isReviewComplete && !isTouchExperience && (
hunk.id).join("|"); - return [workspaceId, filePath, hunkSignature].join("\u0000"); + // Cache raw file bodies per file path + content version. The on-disk content does not + // depend on which hunks are selected or marked read, so changing the visible hunk set + // (e.g. marking a hunk read while read hunks are hidden) must NOT invalidate this cache + // and force a re-read + loading flash. The version is derived from the file's UNFILTERED + // diff content, so it stays stable across read-state filtering but busts the cache when + // the file's diff actually changes (a tool edits the file and the diff is re-fetched), + // preventing reviewers from seeing a stale full-file body. + return [workspaceId, filePath, fileContentVersion].join("\u0000"); } function buildContentKey(content: string, renderedLineCount: number): string { @@ -166,6 +185,34 @@ function buildContentKey(content: string, renderedLineCount: number): string { return `${content.length}:${renderedLineCount}:${hash >>> 0}`; } +/** + * Version key for a file's cached full-file body. FNV-1a over the file's diff hunk + * content (and headers) so the cached body is invalidated when the file's diff actually + * changes — e.g. a tool edits the file and the diff is re-fetched — but NOT when a hunk is + * filtered out of the visible set (marking a hunk read while read hunks are hidden), which + * leaves the underlying file content unchanged. Pass the UNFILTERED hunks for the file so + * read-state filtering does not bust the cache. Hunk ids cannot be used here: they hash + * file path + line ranges, so an in-place edit that preserves line ranges would not change + * them even though the file content differs. + */ +export function buildFileHunksContentVersion(fileHunks: readonly DiffHunk[]): string { + let hash = 2166136261; + const mix = (text: string) => { + for (let index = 0; index < text.length; index += 1) { + hash ^= text.charCodeAt(index); + hash = Math.imul(hash, 16777619); + } + // Boundary marker so concatenation-adjacent hunks cannot collide. + hash ^= 0x1f; + hash = Math.imul(hash, 16777619); + }; + for (const hunk of fileHunks) { + mix(hunk.header); + mix(hunk.content); + } + return `${fileHunks.length}:${hash >>> 0}`; +} + function createOverlayData( contentLines: string[], lineHunkIds: Array, @@ -306,7 +353,7 @@ export function isSameOverlayRevealIdentity( function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { const [activeFileContentState, setActiveFileContentState] = useState({ - filePath: null, + cacheKey: null, content: null, isSettled: true, }); @@ -318,11 +365,21 @@ function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { const activeFileContentCacheKey = useMemo( () => input.activeFilePath - ? buildFileContentCacheKey(input.workspaceId, input.activeFilePath, input.currentFileHunks) + ? buildFileContentCacheKey( + input.workspaceId, + input.activeFilePath, + input.fileContentVersion + ) : null, - [input.activeFilePath, input.currentFileHunks, input.workspaceId] + [input.activeFilePath, input.fileContentVersion, input.workspaceId] ); + // The raw file read does not depend on the hunk set, so read the latest hunks through a + // ref instead of a dependency. This keeps marking a hunk read (which mutates the hunk + // list) from re-running the read effect and re-entering the unsettled/loading state. + const currentFileHunksRef = useRef(input.currentFileHunks); + currentFileHunksRef.current = input.currentFileHunks; + // Load full file context only when it is cheap. If disk I/O or highlighting stalls, // fail open to the compact hunk overlay instead of trapping the review behind loading. useEffect(() => { @@ -330,12 +387,20 @@ function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { const filePath = input.activeFilePath; const cacheKey = activeFileContentCacheKey; + // Short-circuit identical commits so cache-hit re-runs (e.g. after a hunk-set change) + // do not churn state and force a redundant render of the full-file overlay. + const commitContentState = (next: ActiveFileContentState) => { + setActiveFileContentState((current) => + current.cacheKey === next.cacheKey && + current.content === next.content && + current.isSettled === next.isSettled + ? current + : next + ); + }; + const settleContent = (content: string | null) => { - setActiveFileContentState({ - filePath: filePath ?? null, - content, - isSettled: true, - }); + commitContentState({ cacheKey, content, isSettled: true }); }; if (!filePath || !apiClient || !shouldLoadFullFileContext || !cacheKey) { @@ -348,28 +413,21 @@ function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { return; } + const resolvedCacheKey = cacheKey; const resolvedApi = apiClient; const resolvedFilePath = filePath; - const resolvedFileHunks = input.currentFileHunks; + const resolvedFileHunks = currentFileHunksRef.current; const resolvedTheme = input.theme; let cancelled = false; - setActiveFileContentState({ - filePath: resolvedFilePath, - content: null, - isSettled: false, - }); + commitContentState({ cacheKey: resolvedCacheKey, content: null, isSettled: false }); const settleLoadedContent = (content: string | null, shouldCache: boolean) => { if (shouldCache) { - fileContentCacheRef.current.set(cacheKey, content); + fileContentCacheRef.current.set(resolvedCacheKey, content); } if (!cancelled) { - setActiveFileContentState({ - filePath: resolvedFilePath, - content, - isSettled: true, - }); + commitContentState({ cacheKey: resolvedCacheKey, content, isSettled: true }); } }; @@ -452,20 +510,49 @@ function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { activeFileContentCacheKey, input.activeFilePath, input.api, - input.currentFileHunks, input.theme, input.workspaceId, shouldLoadFullFileContext, ]); + // Track settled state by cache key (path + content version), not just path, so an + // in-place edit (same path, new version) is unsettled until the new body loads -- we must + // never render the previous version's body for a same-path content change. const isActiveFileContentSettled = !input.activeFilePath || - (activeFileContentState.filePath === input.activeFilePath && activeFileContentState.isSettled); - - const resolvedActiveFileContent = - isActiveFileContentSettled && shouldLoadFullFileContext ? activeFileContentState.content : null; + (activeFileContentState.cacheKey === activeFileContentCacheKey && + activeFileContentState.isSettled); + + // Resolve an already-loaded body straight from the cache during render. Revisiting a + // file (or rebuilding the overlay after a hunk-set change) then reuses the cached body + // immediately instead of waiting for the read effect to re-commit it, so the stage never + // blanks behind the loading cover for content we already have. + const cachedFullFileContent = + shouldLoadFullFileContext && + activeFileContentCacheKey != null && + fileContentCacheRef.current.has(activeFileContentCacheKey) + ? (fileContentCacheRef.current.get(activeFileContentCacheKey) ?? null) + : undefined; + const hasCachedFullFileContent = cachedFullFileContent !== undefined; + + const resolvedActiveFileContent = !shouldLoadFullFileContext + ? null + : hasCachedFullFileContent + ? cachedFullFileContent + : isActiveFileContentSettled + ? activeFileContentState.content + : null; + + // Only blank the stage (hidden behind the file-switch cover) while a *different* file's + // body is still loading with nothing cached to show. Same-file overlay rebuilds keep + // rendering the overlay we already have, so they reveal in place without a skeleton. + const renderedFilePathRef = useRef(null); + const isSwitchingToNewFile = input.activeFilePath !== renderedFilePathRef.current; const shouldDeferOverlayRender = Boolean( - shouldLoadFullFileContext && !isActiveFileContentSettled + shouldLoadFullFileContext && + !hasCachedFullFileContent && + !isActiveFileContentSettled && + isSwitchingToNewFile ); const overlayData = useMemo(() => { @@ -480,6 +567,15 @@ function useImmersiveOverlayData(input: UseImmersiveOverlayDataInput) { return buildOverlayFromHunks(input.currentFileHunks); }, [input.currentFileHunks, resolvedActiveFileContent, shouldDeferOverlayRender]); + // Remember the file whose overlay we actually rendered so the next switch can tell a + // genuine file change (defer + cover) apart from a same-file overlay rebuild (reveal in + // place). Updated after render via layout effect so the comparison above stays stable. + useLayoutEffect(() => { + if (!shouldDeferOverlayRender && input.activeFilePath != null) { + renderedFilePathRef.current = input.activeFilePath; + } + }, [shouldDeferOverlayRender, input.activeFilePath]); + const shouldEnableHighlighting = overlayData.lineHunkIds.length <= MAX_HIGHLIGHTED_DIFF_LINES; return { @@ -515,6 +611,14 @@ function useImmersiveOverlayReveal(input: UseImmersiveOverlayRevealInput) { const isActiveOverlayRevealPending = activeOverlayRevealIdentity !== null && !isSameOverlayRevealIdentity(revealedOverlayIdentity, activeOverlayRevealIdentity); + // The loading cover only exists to hide the scroll jump when switching to a DIFFERENT + // file. Same-file overlay rebuilds (marking a hunk read while read hunks are hidden, + // compact->full hydration) keep the file on screen, so they update in place and must not + // replay the skeleton. We still track the content-key pending state above to re-run the + // hidden scroll pass that keeps the selected hunk in view across same-file rebuilds. + const isActiveFileRevealPending = + activeOverlayRevealIdentity !== null && + revealedOverlayIdentity?.filePath !== activeOverlayRevealIdentity.filePath; const isHappyDomEnvironment = typeof window !== "undefined" && "happyDOM" in window; // Only worker-backed highlighting is safe to use as a reveal prerequisite; tests and // other non-worker shells fall back to slower main-thread highlighting. @@ -597,13 +701,14 @@ function useImmersiveOverlayReveal(input: UseImmersiveOverlayRevealInput) { return; } - if (isActiveOverlayRevealPending) { + if (isActiveFileRevealPending) { // The pending state is derived during render, not set from an effect, so // file switches are hidden on their first paint until the scroll effect - // reveals the positioned overlay. + // reveals the positioned overlay. Only re-center for genuine file switches; + // same-file rebuilds keep the existing scroll block (e.g. "nearest"). onRevealPending("center"); } - }, [activeOverlayRevealIdentity, isActiveOverlayRevealPending, onRevealPending]); + }, [activeOverlayRevealIdentity, isActiveFileRevealPending, onRevealPending]); useEffect(() => { return () => { @@ -616,6 +721,7 @@ function useImmersiveOverlayReveal(input: UseImmersiveOverlayRevealInput) { return { activeOverlayRevealIdentity, isActiveOverlayRevealPending, + isActiveFileRevealPending, isActiveOverlayReadyForReveal, revealLoadingLabel: "Loading file...", revealActiveOverlayNow, diff --git a/tests/e2e/scenarios/reviewHydration.spec.ts b/tests/e2e/scenarios/reviewHydration.spec.ts index a24b189b55..522ca3611b 100644 --- a/tests/e2e/scenarios/reviewHydration.spec.ts +++ b/tests/e2e/scenarios/reviewHydration.spec.ts @@ -504,6 +504,10 @@ test.describe("immersive review hydration stability", () => { const samples = await stopHydrationSampler(page); expect(samples.length).toBeGreaterThan(0); + // Same-file J/K must never replay the loading cover: the file is already hydrated, so + // iterating hunks is an instant scroll, not a loading state. + expect(samples.filter((sample) => sample.overlayVisible)).toEqual([]); + const visibleCompactSamples = samples.filter( (sample) => sample.stageVisible && @@ -541,4 +545,86 @@ test.describe("immersive review hydration stability", () => { furthestScrollTop = Math.max(furthestScrollTop, scrollTop); } }); + + test("keeps same-file mark-read iteration instant while read hunks are hidden", async ({ + page, + ui, + workspace, + }) => { + await disableReviewTutorial(page); + const diffSummary = seedLargeReviewSingleFileDiff(workspace.demoProject.workspacePath, { + hunkCount: 24, + hunkSpacing: 20, + lineCount: 600, + }); + const expectedFullOverlayLineCount = diffSummary.lineCount + diffSummary.deletedLines; + + // Hide read hunks so marking a hunk read removes it from the visible set -- the + // overlay rebuild that previously re-read the file and replayed the loading cover. + await primeReviewForHeadDiff(page, workspace.demoProject.workspaceId, { showReadHunks: false }); + + await ui.projects.openFirstWorkspace(); + await ui.metaSidebar.expectVisible(); + await ui.metaSidebar.selectTab("Review"); + + const reviewPanel = page.getByTestId("review-panel"); + await expect(reviewPanel).toBeVisible(); + await expect(reviewPanel.getByText(`0/${diffSummary.hunkCount}`, { exact: true })).toBeVisible({ + timeout: 20_000, + }); + + const immersiveButton = reviewPanel.getByRole("button", { name: "Enter immersive review" }); + await expect(immersiveButton).toBeVisible(); + await immersiveButton.dispatchEvent("click"); + + const immersiveReview = page.getByTestId("immersive-review-view"); + await expect(immersiveReview).toBeVisible({ timeout: 20_000 }); + await expect(immersiveReview).toHaveAttribute("data-selected-hunk-position", "1", { + timeout: 20_000, + }); + // Wait for full-file hydration so the file is on screen before we start marking hunks. + const diffLineContainers = immersiveReview.locator( + '[data-testid="immersive-diff-reveal-stage"] div[data-line-index]' + ); + await expect(diffLineContainers).toHaveCount(expectedFullOverlayLineCount, { timeout: 20_000 }); + await expect(immersiveReview.getByTestId("immersive-diff-reveal-overlay")).not.toBeVisible({ + timeout: 20_000, + }); + + await startHydrationSampler(page, null); + await immersiveReview.focus(); + await page.evaluate(async () => { + const immersiveView = document.querySelector( + '[data-testid="immersive-review-view"]' + ); + if (!immersiveView) { + throw new Error("Immersive review view was not found"); + } + + const waitForFrame = () => + new Promise((resolve) => window.requestAnimationFrame(() => resolve())); + const startPath = immersiveView.dataset.activeFilePath; + // Mark several hunks read within the same file; each removes the selected hunk and + // advances to the next one without leaving the file. + for (let step = 0; step < 8; step += 1) { + window.dispatchEvent( + new KeyboardEvent("keydown", { key: "m", bubbles: true, cancelable: true }) + ); + for (let frame = 0; frame < 6; frame += 1) { + await waitForFrame(); + } + if (immersiveView.dataset.activeFilePath !== startPath) { + break; + } + } + }); + + const samples = await stopHydrationSampler(page); + expect(samples.length).toBeGreaterThan(0); + + // The whole iteration stays in one file, so the loading cover must never appear and + // the diff stage must never be hidden behind it. + expect(samples.filter((sample) => sample.overlayVisible)).toEqual([]); + expect(samples.every((sample) => sample.stageVisible)).toBe(true); + }); });