From 781950df8b9f88066b352054634f1fa206152337 Mon Sep 17 00:00:00 2001 From: MP Date: Mon, 27 Jul 2026 16:21:26 +0800 Subject: [PATCH] perf: keep wrapped large diffs responsive --- .changeset/calm-diffs-window.md | 5 + src/ui/components/panes/DiffPane.tsx | 134 +++++++++++------ src/ui/components/ui-components.test.tsx | 174 +++++++++++++++++++++- src/ui/diff/PierreDiffView.tsx | 11 +- src/ui/diff/diffSectionGeometry.test.ts | 179 ++++++++++++++++++++++- src/ui/diff/diffSectionGeometry.ts | 18 +-- src/ui/diff/renderRows.tsx | 155 ++++++++++++++++---- src/ui/lib/fileRenderWindow.test.ts | 30 +++- test/pty/harness.ts | 44 ++++++ test/pty/nav.test.ts | 26 ++++ test/pty/scroll.test.ts | 41 ++++++ 11 files changed, 726 insertions(+), 91 deletions(-) create mode 100644 .changeset/calm-diffs-window.md diff --git a/.changeset/calm-diffs-window.md b/.changeset/calm-diffs-window.md new file mode 100644 index 000000000..6ad5e107d --- /dev/null +++ b/.changeset/calm-diffs-window.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep large wrapped reviews responsive by rendering nearby files and deferring offscreen highlighting. diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 9461b0cd8..e4edc38f4 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -60,11 +60,7 @@ import { DiffFileHeaderRow } from "./DiffFileHeaderRow"; import { VerticalScrollbar, type VerticalScrollbarHandle } from "../scrollbar/VerticalScrollbar"; import type { VisibleBodyBounds } from "../../diff/rowWindowing"; import { prefetchHighlightedDiff } from "../../diff/useHighlightedDiff"; -import { - buildFileRenderWindow, - buildFileSectionIndexById, - type FileRenderWindowItem, -} from "../../lib/fileRenderWindow"; +import { buildFileRenderWindow, buildFileSectionIndexById } from "../../lib/fileRenderWindow"; import { buildCopySelectedRowKeys, clampCopyColumn, @@ -172,6 +168,35 @@ function buildHighlightPrefetchFileIds({ return next; } +/** Keep selected and on-screen files on the immediate highlight path. */ +function buildImmediateHighlightFileIds({ + fileSectionLayouts, + scrollTop, + viewportHeight, + selectedFileId, +}: { + fileSectionLayouts: FileSectionLayout[]; + scrollTop: number; + viewportHeight: number; + selectedFileId?: string; +}) { + const next = new Set(); + if (selectedFileId) { + next.add(selectedFileId); + } + + for (const fileId of collectIntersectingFileSectionIds( + fileSectionLayouts, + Math.max(0, scrollTop), + scrollTop + Math.max(1, viewportHeight), + )) { + next.add(fileId); + } + + return next; +} + +const HIGHLIGHT_PREFETCH_IDLE_MS = 300; const EMPTY_EXPANDED_GAP_KEYS: ReadonlySet = new Set(); const EMPTY_EXPANDED_GAPS_BY_FILE_ID: Record> = {}; const EMPTY_SOURCE_STATUS_BY_FILE_ID: Record = {}; @@ -482,9 +507,6 @@ export function DiffPane({ showAgentNotes, ]); - // Keep the full file-section path for wrapped lines, where exact wrapped heights depend on - // mounting each section; nowrap reviews can window offscreen files behind exact spacers. - const windowingEnabled = !wrapLines; const [scrollViewport, setScrollViewport] = useState({ top: 0, height: 0 }); const [initialWrappedRenderWindowWarmed, setInitialWrappedRenderWindowWarmed] = useState( () => !wrapLines, @@ -1078,7 +1100,17 @@ export function DiffPane({ [totalContentHeight], ); - const highlightPrefetchFileIds = useMemo( + const immediateHighlightFileIds = useMemo( + () => + buildImmediateHighlightFileIds({ + fileSectionLayouts, + scrollTop: scrollViewport.top, + viewportHeight: scrollViewport.height, + selectedFileId, + }), + [fileSectionLayouts, scrollViewport.height, scrollViewport.top, selectedFileId], + ); + const speculativeHighlightFileIds = useMemo( () => buildHighlightPrefetchFileIds({ adjacentPrefetchFileIds, @@ -1098,15 +1130,14 @@ export function DiffPane({ ], ); - // Kick off highlight work from viewport planning rather than waiting for the section to mount. - // That avoids the "plain rows first, color later" stutter when a file is about to scroll onscreen. + // Selected and visible files must be ready for the next frame, so they bypass speculative work. useEffect(() => { if (files.length === 0 || (wrapLines && !initialWrappedRenderWindowWarmed)) { return; } for (const file of files) { - if (!highlightPrefetchFileIds.has(file.id)) { + if (!immediateHighlightFileIds.has(file.id)) { continue; } @@ -1115,7 +1146,37 @@ export function DiffPane({ theme, }); } - }, [files, highlightPrefetchFileIds, initialWrappedRenderWindowWarmed, theme, wrapLines]); + }, [files, immediateHighlightFileIds, initialWrappedRenderWindowWarmed, theme, wrapLines]); + + useEffect(() => { + if (files.length === 0 || (wrapLines && !initialWrappedRenderWindowWarmed)) { + return; + } + + // Highlighting is non-preemptible main-thread work. Delay speculative neighbors until review + // navigation and scrolling have been idle long enough to keep the next interaction frame free. + const timer = setTimeout(() => { + for (const file of files) { + if (immediateHighlightFileIds.has(file.id) || !speculativeHighlightFileIds.has(file.id)) { + continue; + } + + void prefetchHighlightedDiff({ + file, + theme, + }); + } + }, HIGHLIGHT_PREFETCH_IDLE_MS); + + return () => clearTimeout(timer); + }, [ + files, + immediateHighlightFileIds, + initialWrappedRenderWindowWarmed, + speculativeHighlightFileIds, + theme, + wrapLines, + ]); // Keep the selected file/hunk derived from the visible viewport for actual scroll-driven // movement, while leaving the initial mount and non-scroll relayouts alone. @@ -1168,28 +1229,23 @@ export function DiffPane({ renderer.intermediateRender(); }, [renderer, pinnedHeaderFileId]); - const fullFileRenderItems = useMemo( - (): FileRenderWindowItem[] => - files.map((file, sectionIndex) => ({ kind: "file", fileId: file.id, sectionIndex })), - [files], - ); const fileSectionIndexById = useMemo( () => buildFileSectionIndexById(fileSectionLayouts), [fileSectionLayouts], ); + // Exact full-stream geometry is available before mount in both modes, so wrapped files can use + // the same file-level spacers as nowrap files without changing scrollbar or navigation math. const fileRenderWindow = useMemo( () => - windowingEnabled - ? buildFileRenderWindow({ - fileSectionLayouts, - includeFileIds: adjacentPrefetchFileIds, - indexByFileId: fileSectionIndexById, - overscanFiles: 1, - scrollTop: scrollViewport.top, - selectedFileId, - viewportHeight: scrollViewport.height, - }) - : null, + buildFileRenderWindow({ + fileSectionLayouts, + includeFileIds: adjacentPrefetchFileIds, + indexByFileId: fileSectionIndexById, + overscanFiles: 1, + scrollTop: scrollViewport.top, + selectedFileId, + viewportHeight: scrollViewport.height, + }), [ adjacentPrefetchFileIds, fileSectionIndexById, @@ -1197,12 +1253,11 @@ export function DiffPane({ scrollViewport.height, scrollViewport.top, selectedFileId, - windowingEnabled, ], ); - const fileRenderItems = fileRenderWindow?.items ?? fullFileRenderItems; - const mountedFileIndices = fileRenderWindow?.mountedFileIndices ?? null; - // Render note rows for exactly the mounted sections (all sections when windowing is off). + const fileRenderItems = fileRenderWindow.items; + const mountedFileIndices = fileRenderWindow.mountedFileIndices; + // Render note rows for exactly the mounted sections. // Section layouts and spacer heights are measured with the full note set, so a mounted section // that skipped its notes would paint shorter than its layout height and transiently shrink the // scrollbox content height, clamping bottom-edge scrolls. A viewport-proximity set cannot be @@ -1211,11 +1266,8 @@ export function DiffPane({ const visibleAgentNotesByFile = useMemo(() => { const next = new Map(); - const mountedFileIds = mountedFileIndices - ? mountedFileIndices.map((index) => files[index]?.id) - : files.map((file) => file.id); - - for (const fileId of mountedFileIds) { + for (const index of mountedFileIndices) { + const fileId = files[index]?.id; if (!fileId) { continue; } @@ -1262,9 +1314,7 @@ export function DiffPane({ rapidScrollOverscanRows, ); - const indicesToMeasure = mountedFileIndices ?? files.map((_, index) => index); - - for (const index of indicesToMeasure) { + for (const index of mountedFileIndices) { const file = files[index]; const sectionLayout = fileSectionLayouts[index]; const geometry = sectionGeometry[index]; @@ -1875,7 +1925,7 @@ export function DiffPane({ copySelectedSide={copySelectionSide} shouldLoadHighlight={ (!wrapLines || initialWrappedRenderWindowWarmed) && - highlightPrefetchFileIds.has(file.id) + immediateHighlightFileIds.has(file.id) } sectionGeometry={sectionGeometry[index]} separatorWidth={separatorWidth} diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 409e3bf3a..3ada6d574 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -14,6 +14,7 @@ import { hexColorDistance } from "../lib/color"; import { RAPID_SCROLL_OVERSCAN_IDLE_MS } from "../lib/adaptiveScrollOverscan"; import { resolveTheme } from "../themes"; import { measureDiffSectionGeometry } from "../diff/diffSectionGeometry"; +import type { DiffRow } from "../diff/pierre"; import { buildFileSectionLayouts, buildInStreamFileHeaderHeights } from "../lib/fileSectionLayout"; const { AppHost } = await import("../AppHost"); @@ -791,7 +792,14 @@ describe("UI components", () => { spans: [{ text: "ำำ" }], }, }; - const measuredHeight = measureRenderedRowHeight(row, 4, 1, false, true, true, theme); + const measuredHeight = measureRenderedRowHeight(row, { + width: 4, + lineNumberDigits: 1, + showLineNumbers: false, + showHunkHeaders: true, + wrapLines: true, + theme, + }); const setup = await testRender( { spans, }, }; - const measuredHeight = measureRenderedRowHeight(row, 4, 1, false, true, true, theme); + const measuredHeight = measureRenderedRowHeight(row, { + width: 4, + lineNumberDigits: 1, + showLineNumbers: false, + showHunkHeaders: true, + wrapLines: true, + theme, + }); const setup = await testRender( { } }); + test("DiffRowView rendering matches wrapped width reservations at exact boundaries", async () => { + const theme = resolveTheme("github-dark-default", null); + const width = 24; + + for (const layout of ["split", "stack"] as const) { + const rowForLength = (length: number): DiffRow => + layout === "split" + ? { + type: "split-line", + key: `alpha:split:${length}`, + fileId: "alpha", + hunkIndex: 0, + left: { + kind: "context", + sign: " ", + lineNumber: 1, + spans: [{ text: "old" }], + }, + right: { + kind: "addition", + sign: "+", + lineNumber: 1, + spans: [{ text: "x".repeat(length) }], + }, + } + : { + type: "stack-line", + key: `alpha:stack:${length}`, + fileId: "alpha", + hunkIndex: 0, + cell: { + kind: "addition", + sign: "+", + newLineNumber: 1, + spans: [{ text: "x".repeat(length) }], + }, + }; + const measure = (length: number) => + measureRenderedRowHeight(rowForLength(length), { + width, + lineNumberDigits: 1, + showLineNumbers: false, + showHunkHeaders: true, + wrapLines: true, + theme, + noteGuideSide: "new", + reserveAddNoteColumn: true, + }); + let boundary = 1; + while (measure(boundary + 1) === 1) { + boundary += 1; + } + + for (const length of [boundary, boundary + 1]) { + const row = rowForLength(length); + const measuredHeight = measure(length); + const setup = await testRender( + {}} + />, + { width: width + 4, height: 4 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const renderedHeight = setup + .captureSpans() + .lines.filter((line) => + line.spans.some( + (span) => + capturedTestColorToHex(span.bg)?.toLowerCase() === theme.addedBg.toLowerCase(), + ), + ).length; + expect(renderedHeight).toBe(measuredHeight); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + } + } + }); + test("DiffPane geometry memo depends on add-note presence instead of callback identity", async () => { const source = await Bun.file(new URL("./panes/DiffPane.tsx", import.meta.url)).text(); const baseMemo = source.slice( @@ -1138,6 +1248,39 @@ describe("UI components", () => { } }); + test("DiffPane keeps wrapped reviews bounded to nearby file sections", async () => { + const files = createWindowingFiles(12); + const theme = resolveTheme("github-dark-default", null); + const scrollRef = createRef(); + const props = createDiffPaneProps(files, theme, { + diffContentWidth: 48, + scrollRef, + separatorWidth: 44, + width: 52, + wrapLines: true, + }); + const setup = await testRender(, { + width: 56, + height: 12, + }); + + try { + await settleDiffPane(setup); + + const mountedFileIds = files + .filter((file) => scrollRef.current?.content.findDescendantById(`diff-section:${file.id}`)) + .map((file) => file.id); + + expect(mountedFileIds).toContain(files[0]!.id); + expect(mountedFileIds.length).toBeLessThan(files.length); + expect(mountedFileIds).not.toContain(files.at(-1)!.id); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("DiffPane scrolls to the selected later hunk when hunk headers are hidden", async () => { const theme = resolveTheme("github-dark-default", null); const files = [ @@ -2994,6 +3137,33 @@ describe("UI components", () => { expect(binaryFileFrame).toContain("Binary file skipped"); }); + test("PierreDiffView reserves two rendered rows for a no-hunk message", async () => { + const theme = resolveTheme("github-dark-default", null); + const frame = await captureFrame( + + + after-placeholder + , + 76, + 6, + ); + const frameLines = frame.split("\n"); + const messageLine = frameLines.findIndex((line) => + line.includes("This change only renames the file."), + ); + const sentinelLine = frameLines.findIndex((line) => line.includes("after-placeholder")); + + expect(messageLine).toBeGreaterThanOrEqual(0); + expect(sentinelLine - messageLine).toBe(2); + }); + test("PierreDiffView shows the expand chevron only when a source fetcher is attached", async () => { const { file: baseFile } = createExpandableContextDiffFile("expand-affordance", "expand.ts"); const theme = resolveTheme("github-dark-default", null); diff --git a/src/ui/diff/PierreDiffView.tsx b/src/ui/diff/PierreDiffView.tsx index ec7df7d7c..9bf5ad56b 100644 --- a/src/ui/diff/PierreDiffView.tsx +++ b/src/ui/diff/PierreDiffView.tsx @@ -13,7 +13,7 @@ import { spansForHighlightedSourceLine, type DiffRow } from "./pierre"; import { plannedReviewRowVisible } from "./plannedReviewRows"; import { buildDiffSectionRowPlan } from "./diffSectionRowPlan"; import { resolveVisiblePlannedRowWindow, type VisibleBodyBounds } from "./rowWindowing"; -import { diffMessage, DiffRowView, fitText } from "./renderRows"; +import { DIFF_MESSAGE_BODY_HEIGHT, diffMessage, DiffRowView, fitText } from "./renderRows"; import { useHighlightedDiff } from "./useHighlightedDiff"; import { useHighlightedSource } from "./useHighlightedSource"; @@ -314,7 +314,14 @@ export function PierreDiffView({ if (file.metadata.hunks.length === 0) { return ( - + {fitText(diffMessage(file), Math.max(1, width - 2))} ); diff --git a/src/ui/diff/diffSectionGeometry.test.ts b/src/ui/diff/diffSectionGeometry.test.ts index 75c594883..ea139973e 100644 --- a/src/ui/diff/diffSectionGeometry.test.ts +++ b/src/ui/diff/diffSectionGeometry.test.ts @@ -1,6 +1,8 @@ import { describe, expect, test } from "bun:test"; +import type { DiffFile } from "../../core/types"; import type { VisibleAgentNote } from "../lib/agentAnnotations"; import { measureDiffSectionGeometry } from "./diffSectionGeometry"; +import { DIFF_MESSAGE_BODY_HEIGHT } from "./renderRows"; import { resolveTheme } from "../themes"; import { createTestDiffFile, @@ -275,7 +277,7 @@ describe("measureDiffSectionGeometry", () => { ); }); - test("returns a one-row placeholder for files with no visible hunks", () => { + test("returns a two-row placeholder for files with no visible hunks", () => { const file = createTestDiffFile({ after: "const stable = true;\n", before: "const stable = true;\n", @@ -286,11 +288,42 @@ describe("measureDiffSectionGeometry", () => { const metrics = measureDiffSectionGeometry(file, "split", true, theme); expect(file.metadata.hunks).toHaveLength(0); - expect(metrics.bodyHeight).toBe(1); + expect(metrics.bodyHeight).toBe(2); + expect(DIFF_MESSAGE_BODY_HEIGHT).toBe(2); expect(metrics.hunkBounds.size).toBe(0); + expect(metrics.plannedRows).toEqual([]); expect(metrics.rowBounds).toEqual([]); }); + test("gives every no-hunk message variant the same placeholder geometry", () => { + const base = createTestDiffFile({ + after: "const stable = true;\n", + before: "const stable = true;\n", + id: "empty-variants", + path: "empty.ts", + }); + const variants: DiffFile[] = [ + base, + { + ...base, + metadata: { ...base.metadata, type: "rename-pure" }, + previousPath: "old.ts", + }, + { ...base, isBinary: true }, + { ...base, isTooLarge: true }, + { ...base, metadata: { ...base.metadata, type: "new" } }, + { ...base, metadata: { ...base.metadata, type: "deleted" } }, + ]; + + for (const file of variants) { + for (const layout of ["split", "stack"] as const) { + expect( + measureDiffSectionGeometry(file, layout, true, theme, [], 24, true, true).bodyHeight, + ).toBe(2); + } + } + }); + test("can measure a header-only hunk stream without line rows", () => { const file = createTestHeaderOnlyDiffFile(); @@ -505,3 +538,145 @@ describe("measureDiffSectionGeometry", () => { expect(longGeometry.bodyHeight).toBeGreaterThan(shortGeometry.bodyHeight); }); }); + +describe("measureDiffSectionGeometry note-guide width", () => { + const theme = resolveTheme("github-dark-default", null); + const width = 120; + const addNoteBadgeWidth = 3; + + function createGuideFixture(side: "old" | "new", guidedLineLength: number) { + const annotatedLines = lines( + "const a = 1;", + "const anchor = 2;", + `const guided = "${"x".repeat(guidedLineLength)}";`, + "const tail = 4;", + ); + const unannotatedLines = lines("const a = 1;"); + + return createTestDiffFile({ + after: side === "new" ? annotatedLines : unannotatedLines, + before: side === "new" ? unannotatedLines : annotatedLines, + id: `guide-${side}-${guidedLineLength}`, + path: "guide.ts", + }); + } + + function guideNotes(side: "old" | "new"): VisibleAgentNote[] { + return [ + { + id: "annotation:guide:0", + annotation: { + ...(side === "new" ? { newRange: [2, 3] } : { oldRange: [2, 3] }), + rationale: "Guide every row the note covers.", + summary: "Guide", + }, + }, + ]; + } + + function guidedRowHeight({ + layout, + side, + guidedLineLength, + withGuide, + reserveAddNoteColumn = false, + }: { + layout: "split" | "stack"; + side: "old" | "new"; + guidedLineLength: number; + withGuide: boolean; + reserveAddNoteColumn?: boolean; + }) { + const file = createGuideFixture(side, guidedLineLength); + const measure = (notes: VisibleAgentNote[]) => + measureDiffSectionGeometry( + file, + layout, + true, + theme, + notes, + width, + true, + true, + undefined, + undefined, + reserveAddNoteColumn, + ); + const guided = measure(guideNotes(side)); + const guidedRow = guided.plannedRows.find( + (row) => row.kind === "diff-row" && row.noteGuideSide === side, + ); + expect(guidedRow).toBeDefined(); + + if (withGuide) { + return guided.rowBoundsByKey.get(guidedRow!.key)!.height; + } + + return measure([]).rowBoundsByStableKey.get(guidedRow!.stableKey)!.height; + } + + function exactSingleRowBoundary(measure: (guidedLineLength: number) => number) { + let low = 1; + let high = 400; + expect(measure(low)).toBe(1); + expect(measure(high)).toBeGreaterThan(1); + + while (low < high) { + const middle = Math.ceil((low + high) / 2); + if (measure(middle) === 1) { + low = middle; + } else { + high = middle - 1; + } + } + return low; + } + + for (const layout of ["split", "stack"] as const) { + test(`${layout} measurement reserves exactly one column for a new-side guide`, () => { + const boundary = exactSingleRowBoundary((guidedLineLength) => + guidedRowHeight({ + layout, + side: "new", + guidedLineLength, + withGuide: false, + }), + ); + + expect( + guidedRowHeight({ + layout, + side: "new", + guidedLineLength: boundary, + withGuide: true, + }), + ).toBe(2); + expect( + guidedRowHeight({ + layout, + side: "new", + guidedLineLength: boundary - 1, + withGuide: true, + }), + ).toBe(1); + }); + + test(`${layout} measurement combines guide and add-note reservations`, () => { + const boundaryFor = (withGuide: boolean, reserveAddNoteColumn: boolean) => + exactSingleRowBoundary((guidedLineLength) => + guidedRowHeight({ + layout, + side: "new", + guidedLineLength, + withGuide, + reserveAddNoteColumn, + }), + ); + const plain = boundaryFor(false, false); + + expect(boundaryFor(true, false)).toBe(plain - 1); + expect(boundaryFor(false, true)).toBe(plain - addNoteBadgeWidth); + expect(boundaryFor(true, true)).toBe(plain - addNoteBadgeWidth - 1); + }); + } +}); diff --git a/src/ui/diff/diffSectionGeometry.ts b/src/ui/diff/diffSectionGeometry.ts index 278f78729..638b1bef7 100644 --- a/src/ui/diff/diffSectionGeometry.ts +++ b/src/ui/diff/diffSectionGeometry.ts @@ -13,7 +13,7 @@ import { type PlannedHunkBounds, } from "./plannedReviewRows"; import type { PlannedReviewRow } from "./reviewRenderPlan"; -import { measureRenderedRowHeight } from "./renderRows"; +import { DIFF_MESSAGE_BODY_HEIGHT, measureRenderedRowHeight } from "./renderRows"; const EMPTY_EXPANDED_GAP_KEYS: ReadonlySet = new Set(); const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; @@ -242,16 +242,16 @@ function measurePlannedDiffSectionRowHeight( }); } - return measureRenderedRowHeight( - row.row, - width, + return measureRenderedRowHeight(row.row, { lineNumberDigits, - showLineNumbers, + noteGuideSide: row.noteGuideSide, + reserveAddNoteColumn, showHunkHeaders, - wrapLines, + showLineNumbers, theme, - reserveAddNoteColumn, - ); + width, + wrapLines, + }); } /** Measure one file section from the same render plan used by PierreDiffView. */ @@ -271,7 +271,7 @@ export function measureDiffSectionGeometry( ): DiffSectionGeometry { if (file.metadata.hunks.length === 0) { return { - bodyHeight: 1, + bodyHeight: DIFF_MESSAGE_BODY_HEIGHT, hunkAnchorRows: new Map(), hunkBounds: new Map(), lineNumberDigits: String(findMaxLineNumber(file)).length, diff --git a/src/ui/diff/renderRows.tsx b/src/ui/diff/renderRows.tsx index 3ef4debe6..451d3ac38 100644 --- a/src/ui/diff/renderRows.tsx +++ b/src/ui/diff/renderRows.tsx @@ -191,6 +191,68 @@ const addNoteBadgeText = "[+]"; const styledTextColorCache = new Map>(); const addNoteSpacerContentCache = new Map(); +interface RenderedRowWidthBudgetInput { + width: number; + wrapLines: boolean; + reserveAddNoteColumn?: boolean; + showAddNoteBadge?: boolean; + noteGuideSide?: "old" | "new"; +} + +interface RenderedSplitRowWidthBudget { + addNoteWidth: number; + leftWidth: number; + rightRenderWidth: number; +} + +interface RenderedStackRowWidthBudget { + addNoteWidth: number; + contentWidth: number; +} + +function resolveRenderedRowWidthBudget( + rowType: "split-line", + input: RenderedRowWidthBudgetInput, +): RenderedSplitRowWidthBudget; +function resolveRenderedRowWidthBudget( + rowType: "stack-line", + input: RenderedRowWidthBudgetInput, +): RenderedStackRowWidthBudget; +/** + * Resolve the exact widths used to render one diff row. + * + * Wrapped height depends on these widths, so measurement and rendering must reserve the note guide + * and add-note columns through the same path. + */ +function resolveRenderedRowWidthBudget( + rowType: "split-line" | "stack-line", + { + width, + wrapLines, + reserveAddNoteColumn = false, + showAddNoteBadge = false, + noteGuideSide, + }: RenderedRowWidthBudgetInput, +): RenderedSplitRowWidthBudget | RenderedStackRowWidthBudget { + const addNoteWidth = + showAddNoteBadge || (wrapLines && reserveAddNoteColumn) ? addNoteBadgeText.length : 0; + const newSideGuideWidth = noteGuideSide === "new" ? 1 : 0; + + if (rowType === "stack-line") { + return { + addNoteWidth, + contentWidth: Math.max(0, width - newSideGuideWidth - addNoteWidth), + }; + } + + const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); + return { + addNoteWidth, + leftWidth, + rightRenderWidth: Math.max(0, rightWidth - newSideGuideWidth - addNoteWidth), + }; +} + /** Resolve one OpenTUI color while reusing immutable parsed theme values. */ function styledTextColor(value: string | undefined) { if (!value) { @@ -314,11 +376,6 @@ function appendWrappedCellChunks( ); } -/** Reserve the hover affordance column in wrapped rows so hover cannot reflow code. */ -function wrappedAddNoteReserveWidth(wrapLines: boolean, reserveAddNoteColumn = false) { - return wrapLines && reserveAddNoteColumn ? addNoteBadgeText.length : 0; -} - /** Render a fixed-width inline span sequence for one diff cell. */ function renderInlineSpans( spans: RenderSpan[], @@ -1401,6 +1458,14 @@ function renderWrappedStackCellLine( ); } +/** + * Rows occupied by a no-hunk file body: one message row and one row of inter-section spacing. + * + * Keeping this aggregate height shared prevents an unmounted placeholder from changing the review + * stream extent when file-level windowing replaces it with a spacer. + */ +export const DIFF_MESSAGE_BODY_HEIGHT = 2; + /** Explain why a file still appears in the review stream even when it has no textual hunks. */ export function diffMessage(file: DiffFile) { if (file.metadata.type === "rename-pure") { @@ -1591,17 +1656,29 @@ function renderAddNoteSpacer(key: string, width: number, bg: string) { ); } +interface RenderedRowHeightOptions { + width: number; + lineNumberDigits: number; + showLineNumbers: boolean; + showHunkHeaders: boolean; + wrapLines: boolean; + theme: AppTheme; + reserveAddNoteColumn?: boolean; + noteGuideSide?: "old" | "new"; +} + /** Measure how many terminal rows one rendered diff row occupies. */ -export function measureRenderedRowHeight( - row: DiffRow, - width: number, - lineNumberDigits: number, - showLineNumbers: boolean, - showHunkHeaders: boolean, - wrapLines: boolean, - _theme: AppTheme, - reserveAddNoteColumn = false, -) { +export function measureRenderedRowHeight(row: DiffRow, options: RenderedRowHeightOptions) { + const { + lineNumberDigits, + noteGuideSide, + reserveAddNoteColumn, + showHunkHeaders, + showLineNumbers, + width, + wrapLines, + } = options; + if (row.type === "hunk-header") { return showHunkHeaders ? 1 : 0; } @@ -1616,8 +1693,12 @@ export function measureRenderedRowHeight( } const markerWidth = 1; - const hoverReserveWidth = wrappedAddNoteReserveWidth(wrapLines, reserveAddNoteColumn); - const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); + const { leftWidth, rightRenderWidth } = resolveRenderedRowWidthBudget("split-line", { + noteGuideSide, + reserveAddNoteColumn, + width, + wrapLines, + }); const leftGeometry = resolveSplitCellGeometry( leftWidth, lineNumberDigits, @@ -1625,7 +1706,7 @@ export function measureRenderedRowHeight( markerWidth, ); const rightGeometry = resolveSplitCellGeometry( - Math.max(0, rightWidth - hoverReserveWidth), + rightRenderWidth, lineNumberDigits, showLineNumbers, markerWidth, @@ -1645,8 +1726,14 @@ export function measureRenderedRowHeight( return 1; } + const { contentWidth } = resolveRenderedRowWidthBudget("stack-line", { + noteGuideSide, + reserveAddNoteColumn, + width, + wrapLines, + }); const cellGeometry = resolveStackCellGeometry( - Math.max(0, width - wrappedAddNoteReserveWidth(wrapLines, reserveAddNoteColumn)), + contentWidth, lineNumberDigits, showLineNumbers, marker().length, @@ -1719,11 +1806,18 @@ function renderRow( ? { side: "old", line: row.left.lineNumber } : undefined; - // Reserve fixed columns for the diff rails, center separator slot, and hover affordance. - const addBadgeWidth = - showAddNoteBadge || (wrapLines && reserveAddNoteColumn) ? addNoteBadgeText.length : 0; - const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); - const rightRenderWidth = Math.max(0, rightWidth - (guideOnNewSide ? 1 : 0) - addBadgeWidth); + // Measurement reads this same budget, so a wrapped row cannot disagree by one reserved column. + const { + addNoteWidth: addBadgeWidth, + leftWidth, + rightRenderWidth, + } = resolveRenderedRowWidthBudget("split-line", { + noteGuideSide, + reserveAddNoteColumn, + showAddNoteBadge, + width, + wrapLines, + }); const leftPrefix = { text: guideOnOldSide ? "│" : marker(), fg: guideOnOldSide @@ -1941,9 +2035,16 @@ function renderRow( : row.cell.oldLineNumber !== undefined ? { side: "old", line: row.cell.oldLineNumber } : undefined; - const addBadgeWidth = - showAddNoteBadge || (wrapLines && reserveAddNoteColumn) ? addNoteBadgeText.length : 0; - const contentWidth = Math.max(0, width - (guideOnNewSide ? 1 : 0) - addBadgeWidth); + const { addNoteWidth: addBadgeWidth, contentWidth } = resolveRenderedRowWidthBudget( + "stack-line", + { + noteGuideSide, + reserveAddNoteColumn, + showAddNoteBadge, + width, + wrapLines, + }, + ); const prefix = { text: guideOnOldSide ? "│" : marker(), fg: guideOnOldSide diff --git a/src/ui/lib/fileRenderWindow.test.ts b/src/ui/lib/fileRenderWindow.test.ts index da0a9a167..dd9d77aee 100644 --- a/src/ui/lib/fileRenderWindow.test.ts +++ b/src/ui/lib/fileRenderWindow.test.ts @@ -5,13 +5,13 @@ import { buildFileRenderWindow, type FileRenderWindowItem } from "./fileRenderWi /** Build simple file-section layouts with realistic separator/header rows after the first file. */ function createLayouts(count: number, bodyHeight = 10) { - const files = Array.from({ length: count }, (_, index) => ({ - id: `file-${index}`, - })) as DiffFile[]; - return buildFileSectionLayouts( - files, - Array.from({ length: count }, () => bodyHeight), - ); + return createVariableLayouts(Array.from({ length: count }, () => bodyHeight)); +} + +/** Build layouts from explicit per-file body heights, as wrapped files are never uniform. */ +function createVariableLayouts(bodyHeights: number[]) { + const files = bodyHeights.map((_, index) => ({ id: `file-${index}` })) as DiffFile[]; + return buildFileSectionLayouts(files, bodyHeights); } /** Sum exact rendered item heights using the source section layouts for file items. */ @@ -174,4 +174,20 @@ describe("buildFileRenderWindow", () => { expect(plan.bottomSpacerHeight).toBe(22); expect(renderedHeight(plan.items, layouts)).toBe(layouts.at(-1)!.sectionBottom); }); + + test("preserves exact stream height across variable wrapped file bodies", () => { + const layouts = createVariableLayouts([3, 17, 5, 29, 2, 13]); + const scrollPositions = [0, 4, layouts[2]!.bodyTop, layouts[4]!.bodyTop, 999]; + + for (const scrollTop of scrollPositions) { + const plan = buildFileRenderWindow({ + fileSectionLayouts: layouts, + overscanFiles: 1, + scrollTop, + viewportHeight: 7, + }); + + expect(renderedHeight(plan.items, layouts)).toBe(layouts.at(-1)!.sectionBottom); + } + }); }); diff --git a/test/pty/harness.ts b/test/pty/harness.ts index cacd8852c..5465079f7 100644 --- a/test/pty/harness.ts +++ b/test/pty/harness.ts @@ -527,6 +527,33 @@ export function createPtyHarness() { ]); } + /** Build a many-file stream whose changed lines wrap into tall sections at test widths. */ + function createWrappedStreamRepoFixture(fileCount = 8) { + function markerLine(fileNumber: number, suffix: string) { + const filler = Array.from( + { length: 3 }, + (_, part) => `segment${part}OfFile${fileNumber}PaddedWideEnoughToForceContinuationRows`, + ).join(" "); + + return `export const marker${fileNumber} = "${filler} ${suffix}";`; + } + + return createGitRepoFixture( + Array.from({ length: fileCount }, (_, index) => { + const fileNumber = index + 1; + return { + path: `wrapped-${String(fileNumber).padStart(2, "0")}.ts`, + before: `${markerLine(fileNumber, "before")}\n`, + after: [ + markerLine(fileNumber, "after"), + `export const marker${fileNumber}Extra = ${fileNumber * 100};`, + "", + ].join("\n"), + }; + }), + ); + } + function createPinnedHeaderRepoFixture() { return createGitRepoFixture([ { @@ -809,6 +836,21 @@ export function createPtyHarness() { return (text.match(pattern) ?? []).length; } + /** Repeat a navigation key until its destination is visible despite occasional PTY input loss. */ + async function pressUntilVisible(session: Session, key: string, needle: string, maxPresses = 16) { + for (let press = 0; press < maxPresses; press += 1) { + const text = await session.text({ immediate: true }); + if (text.includes(needle)) { + return text; + } + + await session.press(key as Parameters[0]); + await session.waitIdle({ timeout: 400 }); + } + + return waitForSnapshot(session, (text) => text.includes(needle), 5_000); + } + return { cleanup, countMatches, @@ -832,11 +874,13 @@ export function createPtyHarness() { createTwoFileRepoFixture, createWatchFilePair, createWideCharacterFilePair, + createWrappedStreamRepoFixture, launchHunk, launchHunkWithFileBackedStdin, launchShellCommand, buildHunkCommand, shellQuote, + pressUntilVisible, waitForSnapshot, }; } diff --git a/test/pty/nav.test.ts b/test/pty/nav.test.ts index 1cdabae78..a8b6a2f89 100644 --- a/test/pty/nav.test.ts +++ b/test/pty/nav.test.ts @@ -251,4 +251,30 @@ describe("PTY navigation", () => { session.close(); } }); + + test("wrapped hunk navigation crosses unmounted file ranges in both directions", async () => { + const fixture = harness.createWrappedStreamRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split", "--wrap"], + cwd: fixture.dir, + cols: 160, + rows: 14, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { + timeout: 15_000, + }); + expect(initial).toContain("marker1Extra = 100"); + expect(initial).not.toContain("marker8Extra = 800"); + + const forward = await harness.pressUntilVisible(session, "]", "marker8Extra = 800"); + expect(forward).not.toContain("marker1Extra = 100"); + + const backward = await harness.pressUntilVisible(session, "[", "marker1Extra = 100"); + expect(backward).not.toContain("marker8Extra = 800"); + } finally { + session.close(); + } + }); }); diff --git a/test/pty/scroll.test.ts b/test/pty/scroll.test.ts index 08a031c29..e4376cace 100644 --- a/test/pty/scroll.test.ts +++ b/test/pty/scroll.test.ts @@ -340,4 +340,45 @@ describe("PTY scrolling", () => { session.close(); } }); + + test("rapid wrapped scrolling returns to stable stream edges", async () => { + const fixture = harness.createWrappedStreamRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split", "--wrap"], + cwd: fixture.dir, + cols: 160, + rows: 14, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { + timeout: 15_000, + }); + expect(initial).toContain("marker1Extra = 100"); + + for (let burst = 0; burst < 6; burst += 1) { + await session.scrollDown(20); + } + + const bottom = await harness.waitForSnapshot( + session, + (text) => text.includes("marker8Extra = 800"), + 8_000, + ); + expect(bottom).not.toContain("marker1Extra = 100"); + + for (let burst = 0; burst < 8; burst += 1) { + await session.scrollUp(20); + } + + const top = await harness.waitForSnapshot( + session, + (text) => text.includes("marker1Extra = 100"), + 8_000, + ); + expect(top).not.toContain("marker8Extra = 800"); + } finally { + session.close(); + } + }); });