From 755a7fff1747e848ad589d576b1bbfa1fe690a18 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Wed, 8 Apr 2026 12:24:35 -0400 Subject: [PATCH 1/2] fix(ui): preserve viewport position when switching layouts --- src/ui/App.tsx | 16 ++- src/ui/AppHost.interactions.test.tsx | 65 ++++++++++++ src/ui/components/panes/DiffPane.tsx | 146 +++++++++----------------- src/ui/diff/reviewRenderPlan.ts | 123 ++++++++++++++++++++++ src/ui/lib/diffSectionGeometry.ts | 17 ++++ src/ui/lib/viewportAnchor.test.ts | 147 +++++++++++++++++++++++++++ src/ui/lib/viewportAnchor.ts | 116 +++++++++++++++++++++ test/pty/ui-integration.test.ts | 54 ++++++++++ 8 files changed, 585 insertions(+), 99 deletions(-) create mode 100644 src/ui/lib/viewportAnchor.test.ts create mode 100644 src/ui/lib/viewportAnchor.ts diff --git a/src/ui/App.tsx b/src/ui/App.tsx index a2ea18b..54c1b33 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -99,6 +99,8 @@ export function App({ const sidebarScrollRef = useRef(null); const diffScrollRef = useRef(null); const wrapToggleScrollTopRef = useRef(null); + const layoutToggleScrollTopRef = useRef(null); + const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0); const [layoutMode, setLayoutMode] = useState(bootstrap.initialMode); const [themeId, setThemeId] = useState( () => resolveTheme(bootstrap.initialTheme, renderer.themeMode).id, @@ -275,6 +277,13 @@ export function App({ [maxCodeHorizontalOffset, wrapLines], ); + /** Preserve the current review position before changing the active diff layout. */ + const selectLayoutMode = useCallback((mode: LayoutMode) => { + layoutToggleScrollTopRef.current = diffScrollRef.current?.scrollTop ?? 0; + setLayoutToggleRequestId((current) => current + 1); + setLayoutMode(mode); + }, []); + /** Toggle the global agent note layer on or off. */ const toggleAgentNotes = () => { setShowAgentNotes((current) => !current); @@ -472,7 +481,7 @@ export function App({ moveToHunk: review.moveToHunk, refreshCurrentInput: triggerRefreshCurrentInput, requestQuit, - selectLayoutMode: setLayoutMode, + selectLayoutMode, selectThemeId: setThemeId, showAgentNotes, showHelp, @@ -497,6 +506,7 @@ export function App({ moveToAnnotatedHunk, requestQuit, review.moveToHunk, + selectLayoutMode, triggerRefreshCurrentInput, showAgentNotes, showHelp, @@ -547,7 +557,7 @@ export function App({ requestQuit, scrollCodeHorizontally, scrollDiff, - selectLayoutMode: setLayoutMode, + selectLayoutMode, showHelp, switchMenu, toggleAgentNotes, @@ -707,6 +717,8 @@ export function App({ showHunkHeaders={showHunkHeaders} wrapLines={wrapLines} wrapToggleScrollTop={wrapToggleScrollTopRef.current} + layoutToggleScrollTop={layoutToggleScrollTopRef.current} + layoutToggleRequestId={layoutToggleRequestId} selectedFileTopAlignRequestId={review.selectedFileTopAlignRequestId} selectedHunkRevealRequestId={review.selectedHunkRevealRequestId} theme={activeTheme} diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 166197c..f9cbb21 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -364,6 +364,10 @@ function firstVisibleAddedLine(frame: string) { return frame.match(/line\d{2} = 1\d{2}/)?.[0] ?? null; } +function firstVisibleSourceLineNumber(frame: string) { + return frame.match(/line(\d{2}) =/)?.[1] ?? null; +} + function firstVisibleAddedLineNumber(frame: string) { return frame.match(/▌\s*(\d+)\s+\+/)?.[1] ?? null; } @@ -1244,6 +1248,67 @@ describe("App interactions", () => { } }); + test("layout toggles preserve the current viewport anchor across split and stack", async () => { + const setup = await testRender(, { + width: 220, + height: 12, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("line01 = 101"); + expect(frame).not.toContain("line08 = 108"); + + for (let index = 0; index < 24; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("down"); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("line08 = 108") && !frame.includes("line01 = 101")) { + break; + } + } + + expect(frame).toContain("line08 = 108"); + expect(frame).not.toContain("line01 = 101"); + const anchoredLineNumber = firstVisibleSourceLineNumber(frame); + expect(anchoredLineNumber).not.toBeNull(); + + await act(async () => { + await setup.mockInput.typeText("2"); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(80); + await setup.renderOnce(); + }); + + frame = setup.captureCharFrame(); + expect(frame).toContain(`line${anchoredLineNumber} =`); + expect(firstVisibleSourceLineNumber(frame)).toBe(anchoredLineNumber); + + await act(async () => { + await setup.mockInput.typeText("1"); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(80); + await setup.renderOnce(); + }); + + frame = setup.captureCharFrame(); + expect(frame).toContain(`line${anchoredLineNumber} =`); + expect(firstVisibleSourceLineNumber(frame)).toBe(anchoredLineNumber); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("Space scrolls down by viewport", async () => { // Create a file with many lines so Space has room to scroll const before = diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index d665cd8..e8e1572 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -15,7 +15,6 @@ import { computeHunkRevealScrollTop } from "../../lib/hunkScroll"; import { measureDiffSectionGeometry, type DiffSectionGeometry, - type DiffSectionRowBounds, } from "../../lib/diffSectionGeometry"; import { buildFileSectionLayouts, @@ -27,6 +26,11 @@ import { } from "../../lib/fileSectionLayout"; import { diffHunkId, diffSectionId } from "../../lib/ids"; import { findViewportCenteredHunkTarget } from "../../lib/viewportSelection"; +import { + findViewportRowAnchor, + resolveViewportRowAnchorTop, + type ViewportRowAnchor, +} from "../../lib/viewportAnchor"; import type { AppTheme } from "../../themes"; import { DiffSection } from "./DiffSection"; import { DiffFileHeaderRow } from "./DiffFileHeaderRow"; @@ -36,99 +40,6 @@ import { prefetchHighlightedDiff } from "../../diff/useHighlightedDiff"; const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; -/** Identify the rendered diff row that currently owns the top of the viewport. */ -interface ViewportRowAnchor { - fileId: string; - rowKey: string; - rowOffsetWithin: number; -} - -/** Find the rendered row bounds covering a vertical offset within one file body. */ -function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativeTop: number) { - let low = 0; - let high = sectionRowBounds.length - 1; - - while (low <= high) { - const mid = (low + high) >>> 1; - const rowBounds = sectionRowBounds[mid]!; - - if (relativeTop < rowBounds.top) { - high = mid - 1; - } else if (relativeTop >= rowBounds.top + rowBounds.height) { - low = mid + 1; - } else { - return rowBounds; - } - } - - return undefined; -} - -/** Capture a stable top-row anchor from the pre-toggle layout so it can be restored later. */ -function findViewportRowAnchor( - files: DiffFile[], - sectionGeometry: DiffSectionGeometry[], - scrollTop: number, - headerHeights: number[], -) { - const fileSectionLayouts = buildFileSectionLayouts( - files, - sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), - headerHeights, - ); - - for (let index = 0; index < files.length; index += 1) { - const sectionLayout = fileSectionLayouts[index]; - const bodyTop = sectionLayout?.bodyTop ?? 0; - const geometry = sectionGeometry[index]; - const bodyHeight = geometry?.bodyHeight ?? 0; - const relativeTop = scrollTop - bodyTop; - - if (relativeTop >= 0 && relativeTop < bodyHeight && geometry) { - const rowBounds = binarySearchRowBounds(geometry.rowBounds, relativeTop); - if (rowBounds) { - return { - fileId: files[index]!.id, - rowKey: rowBounds.key, - rowOffsetWithin: relativeTop - rowBounds.top, - } satisfies ViewportRowAnchor; - } - } - } - - return null; -} - -/** Resolve a captured row anchor into its new scrollTop after wrapping or layout changes. */ -function resolveViewportRowAnchorTop( - files: DiffFile[], - sectionGeometry: DiffSectionGeometry[], - anchor: ViewportRowAnchor, - headerHeights: number[], -) { - const fileSectionLayouts = buildFileSectionLayouts( - files, - sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), - headerHeights, - ); - - for (let index = 0; index < files.length; index += 1) { - const sectionLayout = fileSectionLayouts[index]; - const bodyTop = sectionLayout?.bodyTop ?? 0; - const file = files[index]; - const geometry = sectionGeometry[index]; - if (file?.id === anchor.fileId && geometry) { - const rowBounds = geometry.rowBoundsByKey.get(anchor.rowKey); - if (rowBounds) { - return bodyTop + rowBounds.top + Math.min(anchor.rowOffsetWithin, rowBounds.height - 1); - } - return bodyTop; - } - } - - return 0; -} - /** Keep syntax-highlight warm for the files immediately adjacent to the current selection. */ function buildAdjacentPrefetchFileIds(files: DiffFile[], selectedFileId?: string) { if (!selectedFileId) { @@ -217,6 +128,8 @@ export function DiffPane({ showHunkHeaders, wrapLines, wrapToggleScrollTop, + layoutToggleScrollTop = null, + layoutToggleRequestId = 0, selectedFileTopAlignRequestId = 0, selectedHunkRevealRequestId, theme, @@ -243,6 +156,8 @@ export function DiffPane({ showHunkHeaders: boolean; wrapLines: boolean; wrapToggleScrollTop: number | null; + layoutToggleScrollTop?: number | null; + layoutToggleRequestId?: number; selectedFileTopAlignRequestId?: number; selectedHunkRevealRequestId?: number; theme: AppTheme; @@ -333,8 +248,10 @@ export function DiffPane({ const prevScrollTopRef = useRef(0); const previousSectionGeometryRef = useRef(null); const previousFilesRef = useRef(files); + const previousLayoutRef = useRef(layout); const previousWrapLinesRef = useRef(wrapLines); const previousSelectedFileTopAlignRequestIdRef = useRef(selectedFileTopAlignRequestId); + const previousLayoutToggleRequestIdRef = useRef(layoutToggleRequestId); const previousSelectedHunkRevealRequestIdRef = useRef(selectedHunkRevealRequestId); const pendingFileTopAlignFileIdRef = useRef(null); const suppressViewportSelectionSyncRef = useRef(false); @@ -344,6 +261,7 @@ export function DiffPane({ // Initialized to null so the first render never fires a selection change; a real scroll // is required before passive viewport-follow selection can trigger. const lastViewportSelectionTopRef = useRef(null); + const lastViewportRowAnchorRef = useRef(null); /** * Ignore viewport-follow selection updates while the pane is scrolling to an explicit selection. @@ -734,23 +652,29 @@ export function DiffPane({ ); useLayoutEffect(() => { + const layoutChanged = previousLayoutRef.current !== layout; + const explicitLayoutToggle = previousLayoutToggleRequestIdRef.current !== layoutToggleRequestId; const wrapChanged = previousWrapLinesRef.current !== wrapLines; const previousSectionMetrics = previousSectionGeometryRef.current; const previousFiles = previousFilesRef.current; const previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles); - if (wrapChanged && previousSectionMetrics && previousFiles.length > 0) { + if ((layoutChanged || wrapChanged) && previousSectionMetrics && previousFiles.length > 0) { const previousScrollTop = // Prefer the synchronously captured pre-toggle position so anchor restoration does not // race the polling-based viewport snapshot. - wrapToggleScrollTop != null + wrapChanged && wrapToggleScrollTop != null ? wrapToggleScrollTop - : Math.max(prevScrollTopRef.current, scrollViewport.top); + : layoutChanged && explicitLayoutToggle && layoutToggleScrollTop != null + ? layoutToggleScrollTop + : (scrollRef.current?.scrollTop ?? + Math.max(prevScrollTopRef.current, scrollViewport.top)); const anchor = findViewportRowAnchor( previousFiles, previousSectionMetrics, previousScrollTop, previousSectionHeaderHeights, + lastViewportRowAnchorRef.current?.stableKey, ); if (anchor) { const nextTop = resolveViewportRowAnchorTop( @@ -763,6 +687,7 @@ export function DiffPane({ scrollRef.current?.scrollTo(nextTop); }; + lastViewportRowAnchorRef.current = anchor; suppressViewportSelectionSync(); restoreViewportAnchor(); // Retry across a couple of repaint cycles so the restored top-row anchor sticks @@ -770,6 +695,8 @@ export function DiffPane({ const retryDelays = [0, 16, 48]; const timeouts = retryDelays.map((delay) => setTimeout(restoreViewportAnchor, delay)); + previousLayoutRef.current = layout; + previousLayoutToggleRequestIdRef.current = layoutToggleRequestId; previousWrapLinesRef.current = wrapLines; previousSectionGeometryRef.current = sectionGeometry; previousFilesRef.current = files; @@ -780,11 +707,16 @@ export function DiffPane({ } } + previousLayoutRef.current = layout; + previousLayoutToggleRequestIdRef.current = layoutToggleRequestId; previousWrapLinesRef.current = wrapLines; previousSectionGeometryRef.current = sectionGeometry; previousFilesRef.current = files; }, [ files, + layout, + layoutToggleRequestId, + layoutToggleScrollTop, scrollRef, scrollViewport.top, sectionGeometry, @@ -794,6 +726,26 @@ export function DiffPane({ wrapToggleScrollTop, ]); + useLayoutEffect(() => { + if (files.length === 0) { + lastViewportRowAnchorRef.current = null; + return; + } + + const currentScrollTop = scrollRef.current?.scrollTop ?? scrollViewport.top; + const nextAnchor = findViewportRowAnchor( + files, + sectionGeometry, + currentScrollTop, + sectionHeaderHeights, + lastViewportRowAnchorRef.current?.stableKey, + ); + + if (nextAnchor) { + lastViewportRowAnchorRef.current = nextAnchor; + } + }, [files, scrollRef, scrollViewport.top, sectionGeometry, sectionHeaderHeights]); + useLayoutEffect(() => { if (previousSelectedFileTopAlignRequestIdRef.current === selectedFileTopAlignRequestId) { return; diff --git a/src/ui/diff/reviewRenderPlan.ts b/src/ui/diff/reviewRenderPlan.ts index eebeaa4..ba2e89e 100644 --- a/src/ui/diff/reviewRenderPlan.ts +++ b/src/ui/diff/reviewRenderPlan.ts @@ -23,6 +23,8 @@ export type PlannedReviewRow = | { kind: "diff-row"; key: string; + stableKey: string; + stableAliasKeys?: string[]; fileId: string; hunkIndex: number; row: DiffRow; @@ -32,6 +34,7 @@ export type PlannedReviewRow = | { kind: "inline-note"; key: string; + stableKey: string; fileId: string; hunkIndex: number; annotationId: string; @@ -43,6 +46,7 @@ export type PlannedReviewRow = | { kind: "note-guide-cap"; key: string; + stableKey: string; fileId: string; hunkIndex: number; side: "old" | "new"; @@ -54,6 +58,118 @@ function lineRows(rows: DiffRow[]) { ); } +/** Deduplicate stable row anchors while preserving the preferred resolution order. */ +function uniqueStableKeys(keys: Array) { + const next: string[] = []; + const seen = new Set(); + + for (const key of keys) { + if (!key || seen.has(key)) { + continue; + } + + seen.add(key); + next.push(key); + } + + return next; +} + +/** Build the file-scoped stable anchor for one old-side source line. */ +function oldLineStableKey(hunkIndex: number, lineNumber?: number) { + return lineNumber === undefined ? undefined : `line:${hunkIndex}:old:${lineNumber}`; +} + +/** Build the file-scoped stable anchor for one new-side source line. */ +function newLineStableKey(hunkIndex: number, lineNumber?: number) { + return lineNumber === undefined ? undefined : `line:${hunkIndex}:new:${lineNumber}`; +} + +/** Build the file-scoped stable anchor for one context row shared by both sides. */ +function contextLineStableKey(hunkIndex: number, oldLineNumber?: number, newLineNumber?: number) { + return oldLineNumber === undefined || newLineNumber === undefined + ? undefined + : `line:${hunkIndex}:context:${oldLineNumber}:${newLineNumber}`; +} + +/** Resolve the stable anchor keys for one rendered diff row across split and stack layouts. */ +function diffRowStableKeys(row: DiffRow) { + if (row.type === "collapsed") { + return [ + row.key.endsWith(":trailing") + ? `meta:collapsed:trailing:${row.hunkIndex}` + : `meta:collapsed:before:${row.hunkIndex}`, + ]; + } + + if (row.type === "hunk-header") { + return [`meta:hunk-header:${row.hunkIndex}`]; + } + + if (row.type === "split-line") { + const contextKey = contextLineStableKey( + row.hunkIndex, + row.left.lineNumber, + row.right.lineNumber, + ); + + if (row.left.kind === "context" && row.right.kind === "context") { + return uniqueStableKeys([ + contextKey, + newLineStableKey(row.hunkIndex, row.right.lineNumber), + oldLineStableKey(row.hunkIndex, row.left.lineNumber), + ]); + } + + // Prefer the old-side line so split→stack toggles stay near the same vertical position even + // when one large change block expands into many deletions followed by many additions. + return uniqueStableKeys([ + oldLineStableKey(row.hunkIndex, row.left.lineNumber), + newLineStableKey(row.hunkIndex, row.right.lineNumber), + ]); + } + + if (row.type !== "stack-line") { + return [`row:${row.key}`]; + } + + const contextKey = contextLineStableKey( + row.hunkIndex, + row.cell.oldLineNumber, + row.cell.newLineNumber, + ); + + if (row.cell.kind === "context") { + return uniqueStableKeys([ + contextKey, + newLineStableKey(row.hunkIndex, row.cell.newLineNumber), + oldLineStableKey(row.hunkIndex, row.cell.oldLineNumber), + ]); + } + + return uniqueStableKeys([ + newLineStableKey(row.hunkIndex, row.cell.newLineNumber), + oldLineStableKey(row.hunkIndex, row.cell.oldLineNumber), + ]); +} + +/** Pick the stable anchor that best matches one old/new-side guide row. */ +function diffRowStableKeyForSide(row: DiffRow, side: "old" | "new") { + if (row.type === "split-line") { + return side === "new" + ? newLineStableKey(row.hunkIndex, row.right.lineNumber) + : oldLineStableKey(row.hunkIndex, row.left.lineNumber); + } + + if (row.type === "stack-line") { + return side === "new" + ? newLineStableKey(row.hunkIndex, row.cell.newLineNumber) + : oldLineStableKey(row.hunkIndex, row.cell.oldLineNumber); + } + + return diffRowStableKeys(row)[0]; +} + /** Check whether a rendered diff row visually covers the note anchor line. */ function rowMatchesNote(row: DiffLineRow, annotation: AgentAnnotation) { const anchor = annotationAnchor(annotation); @@ -229,6 +345,9 @@ export function buildReviewRenderPlan({ const shouldAnchorHunk = rowCanAnchorHunk(row, showHunkHeaders) && !anchoredHunks.has(row.hunkIndex); const anchorId = shouldAnchorHunk ? diffHunkId(fileId, row.hunkIndex) : undefined; + const diffStableKeys = diffRowStableKeys(row); + const diffStableKey = diffStableKeys[0] ?? `row:${row.key}`; + const diffStableAliasKeys = diffStableKeys.slice(1); if (shouldAnchorHunk) { anchoredHunks.add(row.hunkIndex); @@ -239,6 +358,7 @@ export function buildReviewRenderPlan({ plannedRows.push({ kind: "inline-note", key: `inline-note:${placement.note.id}:${row.key}:${placement.noteIndex}`, + stableKey: `inline-note:${placement.note.id}`, fileId, hunkIndex: placement.hunkIndex, annotationId: placement.note.id, @@ -252,6 +372,8 @@ export function buildReviewRenderPlan({ plannedRows.push({ kind: "diff-row", key: `diff-row:${row.key}`, + stableKey: diffStableKey, + stableAliasKeys: diffStableAliasKeys, fileId: row.fileId, hunkIndex: row.hunkIndex, row, @@ -265,6 +387,7 @@ export function buildReviewRenderPlan({ plannedRows.push({ kind: "note-guide-cap", key: `note-guide-cap:${row.key}:${side}`, + stableKey: `note-guide-cap:${side}:${diffRowStableKeyForSide(row, side) ?? diffStableKey}`, fileId, hunkIndex: row.hunkIndex, side, diff --git a/src/ui/lib/diffSectionGeometry.ts b/src/ui/lib/diffSectionGeometry.ts index ecc42ef..f97ad63 100644 --- a/src/ui/lib/diffSectionGeometry.ts +++ b/src/ui/lib/diffSectionGeometry.ts @@ -12,12 +12,15 @@ import type { AppTheme } from "../themes"; export interface DiffSectionRowBounds extends VerticalBounds { key: string; + stableKey: string; + stableKeys: string[]; } /** Cached placeholder sizing and hunk navigation geometry for one file section. */ export interface DiffSectionGeometry extends SectionGeometry { rowBounds: DiffSectionRowBounds[]; rowBoundsByKey: Map; + rowBoundsByStableKey: Map; } const NOTE_AWARE_SECTION_GEOMETRY_CACHE = new WeakMap< @@ -105,6 +108,7 @@ export function measureDiffSectionGeometry( hunkBounds: new Map(), rowBounds: [], rowBoundsByKey: new Map(), + rowBoundsByStableKey: new Map(), }; } @@ -124,6 +128,7 @@ export function measureDiffSectionGeometry( const hunkBounds = new Map(); const rowBounds: DiffSectionRowBounds[] = []; const rowBoundsByKey = new Map(); + const rowBoundsByStableKey = new Map(); const lineNumberDigits = String(findMaxLineNumber(file)).length; let bodyHeight = 0; @@ -142,8 +147,14 @@ export function measureDiffSectionGeometry( wrapLines, theme, ); + const stableKeys = [ + row.stableKey, + ...(row.kind === "diff-row" ? (row.stableAliasKeys ?? []) : []), + ]; const rowBoundsEntry = { key: row.key, + stableKey: row.stableKey, + stableKeys, // Record both the starting top and the measured height so callers can translate between // scroll positions and stable review-row identities across wrap/layout changes. top: bodyHeight, @@ -151,6 +162,11 @@ export function measureDiffSectionGeometry( }; rowBounds.push(rowBoundsEntry); rowBoundsByKey.set(row.key, rowBoundsEntry); + for (const stableKey of stableKeys) { + if (!rowBoundsByStableKey.has(stableKey)) { + rowBoundsByStableKey.set(stableKey, rowBoundsEntry); + } + } if (height > 0 && rowContributesToHunkBounds(row)) { const rowId = reviewRowId(row.key); @@ -178,6 +194,7 @@ export function measureDiffSectionGeometry( hunkBounds, rowBounds, rowBoundsByKey, + rowBoundsByStableKey, }; if (visibleAgentNotes.length > 0) { diff --git a/src/ui/lib/viewportAnchor.test.ts b/src/ui/lib/viewportAnchor.test.ts new file mode 100644 index 0000000..a0132be --- /dev/null +++ b/src/ui/lib/viewportAnchor.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, test } from "bun:test"; +import { resolveTheme } from "../themes"; +import { buildInStreamFileHeaderHeights } from "./fileSectionLayout"; +import { measureDiffSectionGeometry } from "./diffSectionGeometry"; +import { findViewportRowAnchor, resolveViewportRowAnchorTop } from "./viewportAnchor"; +import { createTestDiffFile, lines } from "../../../test/helpers/diff-helpers"; + +describe("viewport row anchors", () => { + const theme = resolveTheme("midnight", null); + + function createChangedFile() { + return createTestDiffFile({ + after: lines("const alpha = 2;"), + before: lines("const alpha = 1;"), + id: "viewport-anchor", + path: "viewport-anchor.ts", + }); + } + + test("honors a preferred stable key when a split change row can map to multiple stacked rows", () => { + const file = createChangedFile(); + const headerHeights = buildInStreamFileHeaderHeights([file]); + const splitGeometry = measureDiffSectionGeometry( + file, + "split", + false, + theme, + [], + 120, + true, + false, + ); + const stackGeometry = measureDiffSectionGeometry( + file, + "stack", + false, + theme, + [], + 120, + true, + false, + ); + const splitChangeTop = splitGeometry.rowBounds.find((row) => row.key.includes(":change:"))?.top; + const stackDeletionTop = stackGeometry.rowBounds.find((row) => + row.key.includes(":deletion:"), + )?.top; + const stackAdditionTop = stackGeometry.rowBounds.find((row) => + row.key.includes(":addition:"), + )?.top; + + expect(splitChangeTop).toBeDefined(); + expect(stackDeletionTop).toBeDefined(); + expect(stackAdditionTop).toBeDefined(); + + const deletionAnchor = findViewportRowAnchor( + [file], + [stackGeometry], + stackDeletionTop!, + headerHeights, + ); + const additionAnchor = findViewportRowAnchor( + [file], + [stackGeometry], + stackAdditionTop!, + headerHeights, + ); + + const splitAsDeletion = findViewportRowAnchor( + [file], + [splitGeometry], + splitChangeTop!, + headerHeights, + deletionAnchor?.stableKey, + ); + const splitAsAddition = findViewportRowAnchor( + [file], + [splitGeometry], + splitChangeTop!, + headerHeights, + additionAnchor?.stableKey, + ); + + expect(splitAsDeletion?.stableKey).toBe(deletionAnchor?.stableKey); + expect(splitAsAddition?.stableKey).toBe(additionAnchor?.stableKey); + }); + + test("round-trips a stacked deletion row through split view without changing the viewport anchor", () => { + const file = createChangedFile(); + const headerHeights = buildInStreamFileHeaderHeights([file]); + const splitGeometry = measureDiffSectionGeometry( + file, + "split", + false, + theme, + [], + 120, + true, + false, + ); + const stackGeometry = measureDiffSectionGeometry( + file, + "stack", + false, + theme, + [], + 120, + true, + false, + ); + const stackDeletionTop = stackGeometry.rowBounds.find((row) => + row.key.includes(":deletion:"), + )?.top; + + expect(stackDeletionTop).toBeDefined(); + + const stackDeletionAnchor = findViewportRowAnchor( + [file], + [stackGeometry], + stackDeletionTop!, + headerHeights, + ); + + expect(stackDeletionAnchor).not.toBeNull(); + + const splitTop = resolveViewportRowAnchorTop( + [file], + [splitGeometry], + stackDeletionAnchor!, + headerHeights, + ); + const splitAnchor = findViewportRowAnchor( + [file], + [splitGeometry], + splitTop, + headerHeights, + stackDeletionAnchor?.stableKey, + ); + const roundTripTop = resolveViewportRowAnchorTop( + [file], + [stackGeometry], + splitAnchor!, + headerHeights, + ); + + expect(roundTripTop).toBe(stackDeletionTop!); + }); +}); diff --git a/src/ui/lib/viewportAnchor.ts b/src/ui/lib/viewportAnchor.ts new file mode 100644 index 0000000..ef65838 --- /dev/null +++ b/src/ui/lib/viewportAnchor.ts @@ -0,0 +1,116 @@ +import type { DiffFile } from "../../core/types"; +import type { DiffSectionGeometry, DiffSectionRowBounds } from "./diffSectionGeometry"; +import { buildFileSectionLayouts } from "./fileSectionLayout"; + +/** Identify the rendered review row that currently owns the viewport top. */ +export interface ViewportRowAnchor { + fileId: string; + rowKey: string; + stableKey: string; + rowOffsetWithin: number; +} + +/** Find the measured row bounds that cover one file-relative vertical offset. */ +function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativeTop: number) { + let low = 0; + let high = sectionRowBounds.length - 1; + + while (low <= high) { + const mid = (low + high) >>> 1; + const rowBounds = sectionRowBounds[mid]!; + + if (relativeTop < rowBounds.top) { + high = mid - 1; + } else if (relativeTop >= rowBounds.top + rowBounds.height) { + low = mid + 1; + } else { + return rowBounds; + } + } + + return undefined; +} + +/** + * Capture a stable top-row anchor from the current review stream. + * + * `preferredStableKey` lets callers preserve the exact logical side they were already following + * when a split row can map to multiple stacked rows and vice versa. + */ +export function findViewportRowAnchor( + files: DiffFile[], + sectionGeometry: DiffSectionGeometry[], + scrollTop: number, + headerHeights: number[], + preferredStableKey?: string | null, +) { + const fileSectionLayouts = buildFileSectionLayouts( + files, + sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), + headerHeights, + ); + + for (let index = 0; index < files.length; index += 1) { + const sectionLayout = fileSectionLayouts[index]; + const bodyTop = sectionLayout?.bodyTop ?? 0; + const geometry = sectionGeometry[index]; + const bodyHeight = geometry?.bodyHeight ?? 0; + const relativeTop = scrollTop - bodyTop; + + if (relativeTop >= 0 && relativeTop < bodyHeight && geometry) { + const rowBounds = binarySearchRowBounds(geometry.rowBounds, relativeTop); + if (!rowBounds) { + continue; + } + + const stableKey = + preferredStableKey && rowBounds.stableKeys.includes(preferredStableKey) + ? preferredStableKey + : rowBounds.stableKey; + + return { + fileId: files[index]!.id, + rowKey: rowBounds.key, + stableKey, + rowOffsetWithin: relativeTop - rowBounds.top, + } satisfies ViewportRowAnchor; + } + } + + return null; +} + +/** Resolve one captured row anchor into its next absolute scrollTop after a relayout. */ +export function resolveViewportRowAnchorTop( + files: DiffFile[], + sectionGeometry: DiffSectionGeometry[], + anchor: ViewportRowAnchor, + headerHeights: number[], +) { + const fileSectionLayouts = buildFileSectionLayouts( + files, + sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), + headerHeights, + ); + + for (let index = 0; index < files.length; index += 1) { + const sectionLayout = fileSectionLayouts[index]; + const bodyTop = sectionLayout?.bodyTop ?? 0; + const file = files[index]; + const geometry = sectionGeometry[index]; + if (file?.id !== anchor.fileId || !geometry) { + continue; + } + + const rowBounds = + geometry.rowBoundsByStableKey.get(anchor.stableKey) ?? + geometry.rowBoundsByKey.get(anchor.rowKey); + if (rowBounds) { + return bodyTop + rowBounds.top + Math.min(anchor.rowOffsetWithin, rowBounds.height - 1); + } + + return bodyTop; + } + + return 0; +} diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index e5c986e..d0f5c70 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -719,6 +719,60 @@ describe("live UI integration", () => { } }); + test("layout hotkeys preserve the current review position in a real PTY", async () => { + const fixture = harness.createScrollableFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "split"], + cols: 220, + rows: 12, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + expect(initial).toContain("line01 = 101"); + expect(initial).not.toContain("line08 = 108"); + + let anchored = initial; + for (let index = 0; index < 24; index += 1) { + await session.press("down"); + await session.waitIdle({ timeout: 200 }); + anchored = await session.text({ immediate: true }); + if (anchored.includes("line08 = 108") && !anchored.includes("line01 = 101")) { + break; + } + } + + const anchoredLineNumber = anchored.match(/line(\d{2}) =/)?.[1]; + + expect(anchored).toContain("line08 = 108"); + expect(anchored).not.toContain("line01 = 101"); + expect(anchoredLineNumber).toBeDefined(); + + await session.press("2"); + const stacked = await harness.waitForSnapshot( + session, + (text) => !/▌.*▌/.test(text) && text.includes(`line${anchoredLineNumber} =`), + 5_000, + ); + + expect(stacked).toContain(`line${anchoredLineNumber} =`); + + await session.press("1"); + const split = await harness.waitForSnapshot( + session, + (text) => /▌.*▌/.test(text) && text.includes(`line${anchoredLineNumber} =`), + 5_000, + ); + + expect(split).toContain(`line${anchoredLineNumber} =`); + } finally { + session.close(); + } + }); + test("mouse wheel scrolling moves the review pane", async () => { const fixture = harness.createScrollableFilePair(); const session = await harness.launchHunk({ From 532e51ac9aa4233fcb84feca5a396772c94bbb3c Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Wed, 8 Apr 2026 12:38:17 -0400 Subject: [PATCH 2/2] fix(ui): address viewport anchor review feedback --- src/ui/components/panes/DiffPane.tsx | 2 +- src/ui/lib/viewportAnchor.ts | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index e8e1572..c1e80d2 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -657,9 +657,9 @@ export function DiffPane({ const wrapChanged = previousWrapLinesRef.current !== wrapLines; const previousSectionMetrics = previousSectionGeometryRef.current; const previousFiles = previousFilesRef.current; - const previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles); if ((layoutChanged || wrapChanged) && previousSectionMetrics && previousFiles.length > 0) { + const previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles); const previousScrollTop = // Prefer the synchronously captured pre-toggle position so anchor restoration does not // race the polling-based viewport snapshot. diff --git a/src/ui/lib/viewportAnchor.ts b/src/ui/lib/viewportAnchor.ts index ef65838..5778de7 100644 --- a/src/ui/lib/viewportAnchor.ts +++ b/src/ui/lib/viewportAnchor.ts @@ -106,7 +106,11 @@ export function resolveViewportRowAnchorTop( geometry.rowBoundsByStableKey.get(anchor.stableKey) ?? geometry.rowBoundsByKey.get(anchor.rowKey); if (rowBounds) { - return bodyTop + rowBounds.top + Math.min(anchor.rowOffsetWithin, rowBounds.height - 1); + return ( + bodyTop + + rowBounds.top + + Math.min(anchor.rowOffsetWithin, Math.max(0, rowBounds.height - 1)) + ); } return bodyTop;