diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.stories.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.stories.tsx index 6fa504f967..586b44160e 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.stories.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.stories.tsx @@ -436,7 +436,9 @@ export const ImmersiveNotesSidebarActionFooter: Story = { await waitFor( () => { canvas.getByTestId("immersive-review-view"); - canvas.getByText(/Keep the formatter instance shared/i); + if (canvas.queryAllByText(/Keep the formatter instance shared/i).length === 0) { + throw new Error("Expected the immersive review formatter note to render."); + } }, { timeout: 10_000 } ); diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 9a2eb6b801..1b99068951 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 { cleanup, fireEvent, render } 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"; @@ -83,6 +83,10 @@ function createFileTreeForPaths(filePaths: string[]): FileTreeNode { return root; } +function encodeFileReadOutput(content: string): string { + return `${Buffer.byteLength(content, "utf8")}\n${Buffer.from(content, "utf8").toString("base64")}`; +} + function renderImmersiveReview( overrides: Partial> = {} ) { @@ -163,6 +167,150 @@ 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("skips full-file reads when the selected hunk starts beyond the render budget", () => { + const farHunk = createHunk({ + id: "hunk-far", + oldStart: 5000, + newStart: 5000, + header: "@@ -5000 +5000 @@", + content: "-old far line\n+new far line", + }); + + const view = renderImmersiveReview({ + fileTree: createFileTree(farHunk.filePath), + hunks: [farHunk], + allHunks: [farHunk], + selectedHunkId: farHunk.id, + }); + + expect(view.container.textContent ?? "").toContain("new far line"); + expect(mockApi.workspace.executeBash).not.toHaveBeenCalled(); + }); + + 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", + 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", + }); + + renderImmersiveReview({ + fileTree: createFileTree(nearHunk.filePath), + hunks: [nearHunk, farHunk], + allHunks: [nearHunk, farHunk], + selectedHunkId: nearHunk.id, + }); + + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); + }); + + test("accepts full-file context at the line budget when the file ends with a newline", async () => { + const lineBudget = 1500; + const fileContent = `${[ + "new line", + "context after selected hunk", + ...Array.from({ length: lineBudget - 2 }, (_, index) => `filler ${index}`), + ].join("\n")}\n`; + mockApi.workspace.executeBash = mock(() => + Promise.resolve({ + success: true as const, + data: { + success: true, + output: encodeFileReadOutput(fileContent), + exitCode: 0, + }, + }) + ); + + const view = renderImmersiveReview(); + + await waitFor(() => + expect(view.container.textContent ?? "").toContain("context after selected hunk") + ); + }); + + test("retries full-file context after a transient read failure", async () => { + const firstHunk = createHunk({ id: "hunk-first", filePath: "src/first.ts" }); + const secondHunk = createHunk({ id: "hunk-second", filePath: "src/second.ts" }); + const allHunks = [firstHunk, secondHunk]; + const fileTree = createFileTreeForPaths(allHunks.map((hunk) => hunk.filePath)); + const onSelectHunk = mock((_hunkId: string | null) => undefined); + mockApi.workspace.executeBash = mock(() => + Promise.resolve({ + success: true as const, + data: { + success: false, + output: "", + exitCode: 1, + }, + }) + ); + + const renderView = (selectedHunkId: string) => ( + + false} + onToggleRead={mock(() => undefined)} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={selectedHunkId} + onSelectHunk={onSelectHunk} + onExit={mock(() => undefined)} + isTouchImmersive={true} + reviewsByFilePath={new Map()} + firstSeenMap={{}} + /> + + ); + + const view = render(renderView(firstHunk.id)); + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(1)); + + view.rerender(renderView(secondHunk.id)); + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(2)); + + view.rerender(renderView(firstHunk.id)); + await waitFor(() => expect(mockApi.workspace.executeBash).toHaveBeenCalledTimes(3)); + }); + test("weights completion by changed lines instead of hunk count", () => { const smallHunk = createHunk({ id: "hunk-small", diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx index 5bf1bad1d7..81be637836 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.tsx @@ -42,7 +42,12 @@ import { matchesKeybind, } from "@/browser/utils/ui/keybinds"; import { stopKeyboardPropagation } from "@/browser/utils/events"; -import { buildReadFileScript, processFileContents } from "@/browser/utils/fileRead"; +import { + buildReadFileScript, + EXIT_CODE_TOO_LARGE, + EXIT_CODE_TOO_MANY_LINES, + processFileContents, +} from "@/browser/utils/fileRead"; import { TooltipIfPresent } from "@/browser/components/Tooltip/Tooltip"; import { parseReviewLineRange, @@ -128,6 +133,9 @@ interface ImmersiveOverlayData { hunkLineRanges: Map; } +const MAX_FULL_FILE_CONTEXT_LINES = 1500; +const MAX_FULL_FILE_CONTEXT_BYTES = 256 * 1024; + 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. @@ -196,6 +204,28 @@ function normalizeFileLines(content: string): string[] { 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[] @@ -541,93 +571,123 @@ export const ImmersiveReviewView: React.FC = (props) = 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 during file switches until loading + initial scroll are complete. + // Hold diff reveal during file switches until the initial scroll is complete. const [pendingRevealFilePath, setPendingRevealFilePath] = useState(null); const revealAnimationFrameRef = useRef(null); - // Load full file content so immersive mode can render one coherent file with hunk overlays. - // Keep a per-file loading state so switches can show a splash until loading settles, - // which avoids a visible fallback-overlay -> full-content jump. + // 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. useEffect(() => { const apiClient = api; const filePath = activeFilePath; + const cacheKey = activeFileContentCacheKey; - if (!filePath || !apiClient) { + const settleContent = (content: string | null) => { setActiveFileContentState({ filePath: filePath ?? null, - content: 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; - let cancelled = false; + setActiveFileContentState({ filePath: resolvedFilePath, content: null, isSettled: false, }); - async function loadActiveFileContent() { - try { - // 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), + const settleLoadedContent = (content: string | null, shouldCache: boolean) => { + if (shouldCache) { + fileContentCacheRef.current.set(cacheKey, content); + } + if (!cancelled) { + setActiveFileContentState({ + filePath: resolvedFilePath, + content, + isSettled: true, }); + } + }; - if (cancelled) { - return; - } + 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, + }), + }); - if (!fileResult.success) { - setActiveFileContentState({ - filePath: resolvedFilePath, - content: null, - isSettled: true, - }); - return; - } + if (cancelled) { + return; + } - const bashResult = fileResult.data; + if (!fileResult.success) { + settleLoadedContent(null, false); + return; + } - if (!bashResult.success && !bashResult.output) { - setActiveFileContentState({ - filePath: resolvedFilePath, - content: null, - isSettled: true, - }); - return; - } + const bashResult = fileResult.data; + const isDeterministicBudgetMiss = + bashResult.exitCode === EXIT_CODE_TOO_LARGE || + bashResult.exitCode === EXIT_CODE_TOO_MANY_LINES; - const data = processFileContents(bashResult.output ?? "", bashResult.exitCode); - setActiveFileContentState({ - filePath: resolvedFilePath, - content: data.type === "text" ? data.content : null, - isSettled: true, - }); - } catch { - if (!cancelled) { - setActiveFileContentState({ - filePath: resolvedFilePath, - content: null, - isSettled: true, - }); - } + 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; + settleLoadedContent(content, content != null || isDeterministicBudgetMiss); } - void loadActiveFileContent(); + loadActiveFileContent().catch(() => { + settleLoadedContent(null, false); + }); return () => { cancelled = true; }; - }, [api, props.workspaceId, activeFilePath]); + }, [ + activeFileContentCacheKey, + activeFilePath, + api, + props.workspaceId, + shouldLoadFullFileContext, + ]); const isActiveFileContentSettled = !activeFilePath || @@ -637,10 +697,6 @@ export const ImmersiveReviewView: React.FC = (props) = ? activeFileContentState.content : null; - const isActiveFileContentLoading = Boolean( - activeFilePath && currentFileHunks.length > 0 && !isActiveFileContentSettled - ); - const overlayData = useMemo(() => { if (currentFileHunks.length === 0) { return { @@ -782,7 +838,7 @@ export const ImmersiveReviewView: React.FC = (props) = selectedHunkId !== null && currentFileHunks.some((hunk) => hunk.id === selectedHunkId); useEffect(() => { - if (!isActiveFileRevealPending || !isActiveFileContentSettled) { + if (!isActiveFileRevealPending) { return; } @@ -805,7 +861,6 @@ export const ImmersiveReviewView: React.FC = (props) = currentFileHunks.length, hasResolvedSelectedHunkForReveal, isActiveFileRevealPending, - isActiveFileContentSettled, selectedHunkRevealTargetLineIndex, ]); @@ -1579,10 +1634,6 @@ export const ImmersiveReviewView: React.FC = (props) = } } - if (isActiveFileRevealPending && !isActiveFileContentSettled) { - return; - } - const lineIndexForScroll = isActiveFileRevealPending ? revealTargetLineIndex : activeLineIndex; if (lineIndexForScroll === null) { return; @@ -1641,7 +1692,6 @@ export const ImmersiveReviewView: React.FC = (props) = }, [ activeFilePath, activeLineIndex, - isActiveFileContentSettled, isActiveFileRevealPending, overlayData.content, revealTargetLineIndex, @@ -1941,44 +1991,36 @@ export const ImmersiveReviewView: React.FC = (props) = ) : (
- {isActiveFileContentLoading ? ( -
+ {isActiveFileRevealPending && ( +
Loading file...
- ) : ( - <> - {isActiveFileRevealPending && ( -
- Loading file... -
- )} -
- -
- )} +
+ +
)}
- {!isReviewComplete && overlayData && !isTouchExperience && !isActiveFileContentLoading && ( + {!isReviewComplete && !isTouchExperience && ( = React.memo( }) => { const { showOld, showNew } = getLineNumberModeFlags(lineNumberMode); const textareaRef = React.useRef(null); - const resizeFrameRef = React.useRef(null); - - const resizeTextarea = React.useCallback(() => { - const textarea = textareaRef.current; - if (!textarea) { - return; - } - - textarea.style.height = "auto"; - textarea.style.height = `${textarea.scrollHeight}px`; - }, []); - - const scheduleTextareaResize = React.useCallback(() => { - if (resizeFrameRef.current !== null) { - cancelAnimationFrame(resizeFrameRef.current); - } - - resizeFrameRef.current = window.requestAnimationFrame(() => { - resizeFrameRef.current = null; - resizeTextarea(); - }); - }, [resizeTextarea]); // Keep the composer uncontrolled so typing does not trigger per-key React re-renders // through immersive diff overlays. Parent-initiated prefill changes are synced here. @@ -749,21 +727,11 @@ const ReviewNoteInput: React.FC = React.memo( } textarea.value = initialNoteText ?? ""; - scheduleTextareaResize(); - }, [initialNoteText, scheduleTextareaResize]); + }, [initialNoteText]); // Auto-focus on mount. React.useEffect(() => { textareaRef.current?.focus(); - scheduleTextareaResize(); - }, [scheduleTextareaResize]); - - React.useEffect(() => { - return () => { - if (resizeFrameRef.current !== null) { - cancelAnimationFrame(resizeFrameRef.current); - } - }; }, []); const handleSubmit = () => { @@ -881,15 +849,18 @@ const ReviewNoteInput: React.FC = React.memo( className="w-[3px] shrink-0" style={{ background: "var(--color-review-accent)" }} /> + {/* Avoid JS autosize here. Reading scrollHeight on every input forces layout across + large immersive diffs, which can make each typed character feel delayed. */}