From 01ccdbb53df3bad6b33bd447a4968d78bc2b3d27 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 30 Aug 2026 16:58:25 -0400 Subject: [PATCH 1/3] fix(ui): fill the review stream on first paint File windowing treated an unmeasured scrollbox height as 0, so only the leading file plus one overscan neighbor mounted until the user scrolled. Use the estimated viewport height for that first paint and re-read once after layout.\n\n(cherry picked from commit d570d2f23d22cfd1f0fb19c5194555e750f0c3f3) --- .changeset/fix-startup-file-window.md | 5 ++++ src/ui/components/panes/DiffPane.tsx | 38 ++++++++++++++++-------- src/ui/components/ui-components.test.tsx | 28 +++++++++++++++++ src/ui/lib/fileRenderWindow.test.ts | 14 +++++++++ src/ui/lib/viewportTiming.test.ts | 22 ++++++++++++++ src/ui/lib/viewportTiming.ts | 18 +++++++++++ test/pty/harness.ts | 11 +++++++ test/pty/layout.test.ts | 29 ++++++++++++++++++ 8 files changed, 153 insertions(+), 12 deletions(-) create mode 100644 .changeset/fix-startup-file-window.md create mode 100644 src/ui/lib/viewportTiming.test.ts diff --git a/.changeset/fix-startup-file-window.md b/.changeset/fix-startup-file-window.md new file mode 100644 index 000000000..c6e6c14e0 --- /dev/null +++ b/.changeset/fix-startup-file-window.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Fill the review stream on first paint instead of leaving it blank until the user scrolls. diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 1cc7725bd..c90c297a9 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -70,7 +70,11 @@ import { } from "../../lib/fileSectionLayout"; import { diffHunkId, diffSectionId } from "../../lib/ids"; import { findViewportCenteredHunkTarget } from "../../lib/viewportSelection"; -import { VIEWPORT_READ_COALESCE_MS } from "../../lib/viewportTiming"; +import { + estimateInitialRenderViewportHeight, + resolveRenderViewportHeight, + VIEWPORT_READ_COALESCE_MS, +} from "../../lib/viewportTiming"; import { findViewportRowAnchor, resolveViewportRowAnchorTop, @@ -150,11 +154,6 @@ function clampVerticalScrollTop(scrollTop: number, contentHeight: number, viewpo return Math.min(Math.max(0, scrollTop), maxScrollTop); } -/** Estimate render-only viewport bounds before OpenTUI publishes exact scrollbox geometry. */ -function estimateInitialRenderViewportHeight(rendererHeight: number, screenTop: number) { - return Math.max(1, rendererHeight - Math.max(0, screenTop)); -} - /** Resolve one file-relative measured row through its precomputed whole-stream section layout. */ function streamRowBoundsAt( layouts: FileSectionLayout[], @@ -802,12 +801,21 @@ export function DiffPane({ }; readViewport(); + // OpenTUI can finish the first yoga layout after this effect without emitting resized or + // layout-changed. One follow-up read picks up that height so file windowing and the scrollbar + // do not wait for the user to scroll. + const warmupViewportRead = setTimeout(() => { + if (!cancelled) { + readViewport(); + } + }, VIEWPORT_READ_COALESCE_MS); scrollBox.verticalScrollBar.on("change", handleViewportChange); scrollBox.viewport.on("layout-changed", handleViewportChange); scrollBox.viewport.on("resized", handleViewportChange); return () => { cancelled = true; + clearTimeout(warmupViewportRead); if (scheduledViewportRead) { clearTimeout(scheduledViewportRead); } @@ -1553,6 +1561,16 @@ export function DiffPane({ files.map((file, sectionIndex) => ({ kind: "file", fileId: file.id, sectionIndex })), [files], ); + const initialRenderViewportHeight = estimateInitialRenderViewportHeight( + renderer.height, + screenTop, + ); + // File windowing must not see height 0: that range is only the first file plus one overscan + // neighbor, which leaves a tall first paint blank until the scrollbox later publishes geometry. + const fileWindowViewportHeight = resolveRenderViewportHeight( + scrollViewport.height, + initialRenderViewportHeight, + ); const fileRenderWindow = useMemo( () => windowingEnabled @@ -1562,13 +1580,13 @@ export function DiffPane({ overscanFiles: 1, scrollTop: scrollViewport.top, selectedFileId, - viewportHeight: scrollViewport.height, + viewportHeight: fileWindowViewportHeight, }) : null, [ fileSectionIndexById, fileSectionLayouts, - scrollViewport.height, + fileWindowViewportHeight, scrollViewport.top, selectedFileId, windowingEnabled, @@ -1607,10 +1625,6 @@ export function DiffPane({ // back the prior object when top/height are numerically unchanged lets mounted sections skip // re-rendering even though the Map itself is rebuilt every snapshot. const previousVisibleBodyBoundsRef = useRef>(new Map()); - const initialRenderViewportHeight = estimateInitialRenderViewportHeight( - renderer.height, - screenTop, - ); const visibleBodyBoundsByFile = useMemo(() => { const previous = previousVisibleBodyBoundsRef.current; const next = new Map(); diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 2b7412505..d9fc650e5 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -1220,6 +1220,34 @@ describe("UI components", () => { } }); + test("DiffPane first nowrap paint fills a tall viewport past the overscan neighbor", async () => { + const files = createWindowingFiles(8); + const theme = resolveTheme("github-dark-default", null); + const props = createDiffPaneProps(files, theme, { + diffContentWidth: 88, + separatorWidth: 84, + width: 92, + }); + const setup = await testRender(, { + width: 96, + height: 40, + }); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const frame = setup.captureCharFrame(); + + expect(frame).toContain("window-3.ts"); + expect(frame).toContain("file3Extra = true"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("DiffPane scrolls a later selected file into view in the windowed path", async () => { const files = createWindowingFiles(6); const theme = resolveTheme("github-dark-default", null); diff --git a/src/ui/lib/fileRenderWindow.test.ts b/src/ui/lib/fileRenderWindow.test.ts index edf18c5dd..b58dee453 100644 --- a/src/ui/lib/fileRenderWindow.test.ts +++ b/src/ui/lib/fileRenderWindow.test.ts @@ -56,6 +56,20 @@ describe("buildFileRenderWindow", () => { expect(plan.bottomSpacerHeight).toBe(0); }); + test("a zero-height viewport at the top only mounts the first file plus overscan", () => { + const layouts = createLayouts(6, 8); + const plan = buildFileRenderWindow({ + fileSectionLayouts: layouts, + overscanFiles: 1, + scrollTop: 0, + viewportHeight: 0, + }); + + expect(plan.mountedFileIndices).toEqual([0, 1]); + expect(plan.visibleStartIndex).toBe(0); + expect(plan.visibleEndIndex).toBe(0); + }); + test("mounts the visible first file and reserves the rest in one bottom spacer", () => { const layouts = createLayouts(4); const plan = buildFileRenderWindow({ diff --git a/src/ui/lib/viewportTiming.test.ts b/src/ui/lib/viewportTiming.test.ts new file mode 100644 index 000000000..29cce55be --- /dev/null +++ b/src/ui/lib/viewportTiming.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, test } from "bun:test"; +import { estimateInitialRenderViewportHeight, resolveRenderViewportHeight } from "./viewportTiming"; + +describe("estimateInitialRenderViewportHeight", () => { + test("subtracts the pane's screen top from the renderer height", () => { + expect(estimateInitialRenderViewportHeight(80, 2)).toBe(78); + }); + + test("never returns an empty window while geometry is still unknown", () => { + expect(estimateInitialRenderViewportHeight(0, 0)).toBe(1); + }); +}); + +describe("resolveRenderViewportHeight", () => { + test("falls back to the estimate while the scrollbox height is still 0", () => { + expect(resolveRenderViewportHeight(0, 48)).toBe(48); + }); + + test("keeps the measured scrollbox height once it is available", () => { + expect(resolveRenderViewportHeight(36, 48)).toBe(36); + }); +}); diff --git a/src/ui/lib/viewportTiming.ts b/src/ui/lib/viewportTiming.ts index b42433305..d3cbc595a 100644 --- a/src/ui/lib/viewportTiming.ts +++ b/src/ui/lib/viewportTiming.ts @@ -1,2 +1,20 @@ /** Delay used to coalesce imperative ScrollBox viewport reads to roughly one frame. */ export const VIEWPORT_READ_COALESCE_MS = 16; + +/** + * Estimate render-only viewport bounds before OpenTUI publishes exact scrollbox geometry. + * Subtracts the review pane's screen-top offset from the renderer height so the first paint + * can window files without waiting for the scrollbox to report its laid-out height. + */ +export function estimateInitialRenderViewportHeight(rendererHeight: number, screenTop: number) { + return Math.max(1, rendererHeight - Math.max(0, screenTop)); +} + +/** + * Prefer the measured scrollbox height once it exists; otherwise keep the first-paint estimate. + * Passing a measured 0 into file windowing only mounts the leading file plus overscan, which + * leaves a tall first frame blank until the user scrolls. + */ +export function resolveRenderViewportHeight(measuredHeight: number, estimatedHeight: number) { + return measuredHeight > 0 ? measuredHeight : Math.max(1, estimatedHeight); +} diff --git a/test/pty/harness.ts b/test/pty/harness.ts index 9874dfb4d..60ce1ace9 100644 --- a/test/pty/harness.ts +++ b/test/pty/harness.ts @@ -726,6 +726,16 @@ end ]); } + /** Build many short files so a tall first paint must mount past the first-file overscan neighbor. */ + function createManyShortFileRepoFixture() { + return createGitRepoFixture( + Array.from({ length: 8 }, (_, index) => ({ + path: `short-${index}.ts`, + before: `export const short${index} = ${index};\n`, + after: `export const short${index} = ${index + 10};\n`, + })), + ); + } function createPinnedHeaderRepoFixture() { return createGitRepoFixture([ { @@ -1093,6 +1103,7 @@ end createMultiHunkFilePair, createNarrowHeaderTestRepoFixture, createPagerPatchFixture, + createManyShortFileRepoFixture, createPinnedHeaderRepoFixture, createRapidThemePreviewTestRepoFixture, createScrollableFilePair, diff --git a/test/pty/layout.test.ts b/test/pty/layout.test.ts index a3ac1c3d2..dd005b753 100644 --- a/test/pty/layout.test.ts +++ b/test/pty/layout.test.ts @@ -43,6 +43,35 @@ describe("PTY layout", () => { } }); + test("the first nowrap frame fills a tall viewport past the first-file overscan neighbor", async () => { + // File windowing with a still-unmeasured (0) viewport only mounts the leading file plus one + // overscan neighbor. A tall first paint must use the estimated height so later short files + // appear without any scroll input. + const fixture = harness.createManyShortFileRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "stack", "--no-sidebar"], + cwd: fixture.dir, + cols: 100, + rows: 40, + }); + + try { + await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { + timeout: 15_000, + }); + + const firstFrame = await harness.waitForSnapshot( + session, + (text) => text.includes("short-3.ts") && text.includes("short3 = 13"), + 8_000, + ); + + expect(firstFrame).toContain("short-3.ts"); + expect(firstFrame).toContain("short3 = 13"); + } finally { + session.close(); + } + }); test("split rows keep the center separator aligned after wide characters", async () => { const fixture = harness.createWideCharacterFilePair(); const session = await harness.launchHunk({ From f9722b20740d5eff2d52cc5badd3300fc0175ba4 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 30 Aug 2026 16:02:56 -0400 Subject: [PATCH 2/3] fix(ui): stabilize viewport startup timing (cherry picked from commit 408a1a32e37d715372d5de64f630d78dc099c74d) --- src/ui/App.tsx | 14 +++ src/ui/AppHost.interactions.test.tsx | 10 ++ src/ui/components/panes/DiffPane.tsx | 139 +++++++++++++++++++++------ src/ui/lib/lineCursors.test.ts | 21 ++++ src/ui/lib/lineCursors.ts | 21 ++++ src/ui/lib/viewportTiming.test.ts | 4 + src/ui/lib/viewportTiming.ts | 17 +++- 7 files changed, 192 insertions(+), 34 deletions(-) diff --git a/src/ui/App.tsx b/src/ui/App.tsx index d53a9f600..804f32241 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -296,6 +296,10 @@ export function App({ const layoutToggleScrollTopRef = useRef(null); const cancelCopySelectionRef = useRef<(() => void) | null>(null); const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0); + const [scrollEdgeRequest, setScrollEdgeRequest] = useState<{ + id: number; + edge: "top" | "bottom"; + }>({ id: 0, edge: "top" }); const [transientNoticeText, setTransientNoticeText] = useState(null); const [layoutMode, setLayoutMode] = useState(bootstrap.initialMode); const [themeId, setThemeId] = useState( @@ -1395,6 +1399,15 @@ export function App({ delta: number, unit: "step" | "viewport" | "content" | "half" = "viewport", ) => { + if (unit === "content") { + if (delta !== 0) { + setScrollEdgeRequest((current) => ({ + id: current.id + 1, + edge: delta > 0 ? "bottom" : "top", + })); + } + return; + } if (unit === "half") { const scrollBox = diffScrollRef.current; if (!scrollBox) return; @@ -2407,6 +2420,7 @@ export function App({ wrapToggleScrollTop={wrapToggleScrollTopRef.current} layoutToggleScrollTop={layoutToggleScrollTopRef.current} layoutToggleRequestId={layoutToggleRequestId} + scrollEdgeRequest={scrollEdgeRequest} selectedFileTopAlignRequestId={review.selectedFileTopAlignRequestId} selectedHunkRevealRequestId={review.selectedHunkRevealRequestId} cursorLine={cursorLine} diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index cf44eb62f..343061778 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -433,6 +433,14 @@ async function flush(setup: Awaited>) { }); } +/** Let initial viewport measurement enable row windowing before testing imperative scroll jumps. */ +async function settleViewportMeasurement(setup: Awaited>) { + await act(async () => { + await Bun.sleep(32); + await setup.renderOnce(); + }); +} + /** Let wrap-toggle renders and follow-up layout retries settle before asserting on the frame. */ async function settleWrapToggle(setup: Awaited>) { await flush(setup); @@ -2462,6 +2470,7 @@ describe("App interactions", () => { try { await flush(setup); + await settleViewportMeasurement(setup); let frame = setup.captureCharFrame(); expect(frame).toContain("line01 = 1001"); @@ -2525,6 +2534,7 @@ describe("App interactions", () => { try { await flush(setup); + await settleViewportMeasurement(setup); let frame = setup.captureCharFrame(); expect(frame).toContain("line01 = 1001"); diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index c90c297a9..53eabe020 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -51,6 +51,7 @@ import { clampLineCursorToViewport, EMPTY_LINE_CURSORS, firstLineCursorInHunk, + reuseEquivalentLineCursors, type LineCursor, type LineCursorBoundsLookup, } from "../../lib/lineCursors"; @@ -292,6 +293,7 @@ export function DiffPane({ wrapToggleScrollTop, layoutToggleScrollTop = null, layoutToggleRequestId = 0, + scrollEdgeRequest, selectedFileTopAlignRequestId = 0, selectedHunkRevealRequestId, theme, @@ -355,6 +357,7 @@ export function DiffPane({ wrapToggleScrollTop: number | null; layoutToggleScrollTop?: number | null; layoutToggleRequestId?: number; + scrollEdgeRequest?: { id: number; edge: "top" | "bottom" }; selectedFileTopAlignRequestId?: number; selectedHunkRevealRequestId?: number; theme: AppTheme; @@ -649,6 +652,7 @@ export function DiffPane({ const previousFilesRef = useRef(files); const previousLayoutRef = useRef(layout); const previousWrapLinesRef = useRef(wrapLines); + const previousViewportPaneHeightRef = useRef(height); const draftNoteId = draftNote?.id ?? null; const draftNoteFileId = draftNote?.fileId ?? null; const previousDraftNoteIdRef = useRef(draftNoteId); @@ -732,14 +736,20 @@ export function DiffPane({ if (!scrollBox) { return; } + const paneHeightChanged = previousViewportPaneHeightRef.current !== height; + previousViewportPaneHeightRef.current = height; let cancelled = false; let scheduled = false; let scheduledViewportRead: ReturnType | null = null; + let lastReadTop = scrollBox.scrollTop ?? 0; + let lastReadHeight = scrollBox.viewport.height ?? 0; const readViewport = () => { const nextTop = scrollBox.scrollTop ?? 0; const nextHeight = scrollBox.viewport.height ?? 0; + lastReadTop = nextTop; + lastReadHeight = nextHeight; // The first viewport read is a baseline snapshot, not scroll input. The scroll box may retain // a non-zero top across remounts, so do not treat that retained position as a rapid burst. @@ -769,15 +779,15 @@ export function DiffPane({ ); }; - // OpenTUI emits `change` synchronously from inside its own slider sync, and other - // useLayoutEffects in this pane scroll the box from inside React's commit phase. - // Calling setScrollViewport directly from the listener can run setState while React - // is already committing — which downstream layout effects can amplify into a render - // loop and trip React's max-update-depth guard. Coalesce listener events into one - // timer-deferred read so rapid wheel/key bursts collapse into bounded React updates instead of - // turning every native scroll delta into a full review-stream render. Wrapped views use a - // half-frame interval to reduce blank-band latency; nowrap retains one frame. + // OpenTUI emits viewport events from its own layout and slider work. Keep React state updates + // timer-deferred so wheel/key bursts collapse into bounded review-stream renders. const handleViewportChange = () => { + if ( + (scrollBox.scrollTop ?? 0) === lastReadTop && + (scrollBox.viewport.height ?? 0) === lastReadHeight + ) { + return; + } if (scheduled) { return; } @@ -800,30 +810,43 @@ export function DiffPane({ ); }; - readViewport(); - // OpenTUI can finish the first yoga layout after this effect without emitting resized or - // layout-changed. One follow-up read picks up that height so file windowing and the scrollbar - // do not wait for the user to scroll. - const warmupViewportRead = setTimeout(() => { - if (!cancelled) { - readViewport(); + // Wait for one real Yoga height change only when geometry is still unknown or the planned pane + // height changed. Leaving this armed after a successful read feeds later content relayouts back + // into React even though the viewport height is already authoritative. + const handleViewportResize = () => { + if ((scrollBox.viewport.height ?? 0) === lastReadHeight) { + return; } - }, VIEWPORT_READ_COALESCE_MS); + scrollBox.viewport.off("resize", handleViewportResize); + queueMicrotask(() => { + if (!cancelled) { + readViewport(); + } + }); + }; + + readViewport(); scrollBox.verticalScrollBar.on("change", handleViewportChange); - scrollBox.viewport.on("layout-changed", handleViewportChange); - scrollBox.viewport.on("resized", handleViewportChange); + if (lastReadHeight <= 0 || paneHeightChanged) { + scrollBox.viewport.on("resize", handleViewportResize); + } return () => { cancelled = true; - clearTimeout(warmupViewportRead); if (scheduledViewportRead) { clearTimeout(scheduledViewportRead); } scrollBox.verticalScrollBar.off("change", handleViewportChange); - scrollBox.viewport.off("layout-changed", handleViewportChange); - scrollBox.viewport.off("resized", handleViewportChange); + scrollBox.viewport.off("resize", handleViewportResize); }; - }, [activateRapidScrollOverscan, clearAddNoteHoverForScroll, files.length, scrollRef, wrapLines]); + }, [ + activateRapidScrollOverscan, + clearAddNoteHoverForScroll, + files.length, + height, + scrollRef, + wrapLines, + ]); const sectionHeaderHeights = useMemo(() => buildInStreamFileHeaderHeights(files), [files]); const reserveAddNoteColumn = Boolean(onStartUserNoteAtHunk); @@ -926,17 +949,68 @@ export function DiffPane({ [estimatedBodyHeights, files, sectionHeaderHeights], ); const totalContentHeight = fileSectionLayouts[fileSectionLayouts.length - 1]?.sectionBottom ?? 0; + const previousScrollEdgeRequestIdRef = useRef(scrollEdgeRequest?.id ?? 0); + const pendingScrollEdgeRequest = + scrollEdgeRequest && scrollEdgeRequest.id !== previousScrollEdgeRequestIdRef.current + ? scrollEdgeRequest + : null; + const scrollEdgeViewportHeight = Math.max( + scrollViewport.height, + scrollRef.current?.viewport.height ?? 0, + ); + const requestedScrollEdgeTop = pendingScrollEdgeRequest + ? clampVerticalScrollTop( + pendingScrollEdgeRequest.edge === "bottom" ? totalContentHeight : 0, + totalContentHeight, + scrollEdgeViewportHeight, + ) + : null; + const renderScrollTop = requestedScrollEdgeTop ?? scrollViewport.top; + + // Edge jumps render their destination rows before moving OpenTUI's native viewport, avoiding a + // frame where viewport culling points at rows that React has not mounted yet. + useLayoutEffect(() => { + if (!pendingScrollEdgeRequest || requestedScrollEdgeTop === null) { + return; + } + const scrollBox = scrollRef.current; + if (!scrollBox) { + return; + } + + previousScrollEdgeRequestIdRef.current = pendingScrollEdgeRequest.id; + const viewportHeight = scrollBox.viewport.height || scrollEdgeViewportHeight; + const nextTop = clampVerticalScrollTop( + requestedScrollEdgeTop, + totalContentHeight, + viewportHeight, + ); + setScrollViewport({ top: nextTop, height: viewportHeight }); + scrollBox.scrollTo(nextTop); + }, [ + pendingScrollEdgeRequest, + requestedScrollEdgeTop, + scrollEdgeViewportHeight, + scrollRef, + totalContentHeight, + ]); const fileSectionIndexById = useMemo( () => buildFileSectionIndexById(fileSectionLayouts), [fileSectionLayouts], ); - const lineCursors = useMemo( + const measuredLineCursors = useMemo( // Nothing reads the stops while the marker is off, and enumerating them costs one object per // rendered row of the whole changeset every time geometry is remeasured. () => (cursorLine === "off" ? EMPTY_LINE_CURSORS : buildLineCursors(files, sectionGeometry)), [cursorLine, files, sectionGeometry], ); + const previousLineCursorsRef = useRef(EMPTY_LINE_CURSORS); + const lineCursors = reuseEquivalentLineCursors( + previousLineCursorsRef.current, + measuredLineCursors, + ); + previousLineCursorsRef.current = lineCursors; /** Locate one measured row in whole-stream rows, addressed by its file and plan anchor. */ const rowBoundsInStream = useCallback( (fileId: string, stableKey: string) => { @@ -960,7 +1034,9 @@ export function DiffPane({ // Read the live scroll box position during render so pinned-header ownership flips // immediately after imperative scrolls instead of waiting for the polled viewport snapshot. - const effectiveScrollTop = scrollRef.current?.scrollTop ?? scrollViewport.top; + const effectiveScrollTop = pendingScrollEdgeRequest + ? renderScrollTop + : (scrollRef.current?.scrollTop ?? scrollViewport.top); const pinnedHeaderFile = useMemo(() => { if (files.length === 0) { return null; @@ -1442,7 +1518,7 @@ export function DiffPane({ adjacentPrefetchFileIds, fileSectionLayouts, rapidScrollOverscanRows, - scrollTop: scrollViewport.top, + scrollTop: renderScrollTop, viewportHeight: scrollViewport.height, selectedFileId, }), @@ -1451,7 +1527,7 @@ export function DiffPane({ fileSectionLayouts, rapidScrollOverscanRows, scrollViewport.height, - scrollViewport.top, + renderScrollTop, selectedFileId, ], ); @@ -1564,6 +1640,7 @@ export function DiffPane({ const initialRenderViewportHeight = estimateInitialRenderViewportHeight( renderer.height, screenTop, + height, ); // File windowing must not see height 0: that range is only the first file plus one overscan // neighbor, which leaves a tall first paint blank until the scrollbox later publishes geometry. @@ -1578,7 +1655,7 @@ export function DiffPane({ fileSectionLayouts, indexByFileId: fileSectionIndexById, overscanFiles: 1, - scrollTop: scrollViewport.top, + scrollTop: renderScrollTop, selectedFileId, viewportHeight: fileWindowViewportHeight, }) @@ -1587,7 +1664,7 @@ export function DiffPane({ fileSectionIndexById, fileSectionLayouts, fileWindowViewportHeight, - scrollViewport.top, + renderScrollTop, selectedFileId, windowingEnabled, ], @@ -1663,9 +1740,9 @@ export function DiffPane({ // Convert the absolute review-stream viewport into file-body-local coordinates. // Example: if the viewport starts at row 2_000 globally and this file body starts at row // 1_940, then the file-local visible top is 60 rows into this file. - let minTop = scrollViewport.top - sectionLayout.bodyTop - overscanTerminalRows; + let minTop = renderScrollTop - sectionLayout.bodyTop - overscanTerminalRows; let maxBottom = - scrollViewport.top + renderViewportHeight - sectionLayout.bodyTop + overscanTerminalRows; + renderScrollTop + renderViewportHeight - sectionLayout.bodyTop + overscanTerminalRows; // A fitting selected hunk must remain fully mounted even during the zero-halo first paint. // Oversized hunks keep ordinary viewport windowing so one selection cannot defeat startup. @@ -1703,7 +1780,7 @@ export function DiffPane({ initialWrappedRenderWindowWarmed, rapidScrollOverscanRows, scrollViewport.height, - scrollViewport.top, + renderScrollTop, initialRenderViewportHeight, sectionGeometry, mountedFileIndices, diff --git a/src/ui/lib/lineCursors.test.ts b/src/ui/lib/lineCursors.test.ts index f88eda01e..6ddfb3596 100644 --- a/src/ui/lib/lineCursors.test.ts +++ b/src/ui/lib/lineCursors.test.ts @@ -15,6 +15,7 @@ import { findLineCursorAt, findNextLineCursor, firstLineCursorInHunk, + reuseEquivalentLineCursors, resolveLineCursor, type LineCursor, } from "./lineCursors"; @@ -196,6 +197,26 @@ describe("buildLineCursors", () => { }); }); +describe("reuseEquivalentLineCursors", () => { + test("keeps list identity when remeasurement preserves every cursor", () => { + const previous = cursorsFor([createContextWrappedFile("alpha", "alpha.ts")], "stack"); + const next = previous.map((cursor) => ({ ...cursor, target: { ...cursor.target } })); + + expect(reuseEquivalentLineCursors(previous, next)).toBe(previous); + }); + + test("keeps a changed cursor list", () => { + const previous = cursorsFor([createContextWrappedFile("alpha", "alpha.ts")], "stack"); + const next = previous.map((cursor, index) => + index === 0 + ? { ...cursor, target: { ...cursor.target, line: cursor.target.line + 1 } } + : cursor, + ); + + expect(reuseEquivalentLineCursors(previous, next)).toBe(next); + }); +}); + describe("findLineCursorAt", () => { /** Build a file whose inserted line pushes the trailing context onto different side numbers. */ function createShiftedContextFile() { diff --git a/src/ui/lib/lineCursors.ts b/src/ui/lib/lineCursors.ts index 510c576c9..603781495 100644 --- a/src/ui/lib/lineCursors.ts +++ b/src/ui/lib/lineCursors.ts @@ -106,6 +106,27 @@ export function buildLineCursors( }); } +/** Reuse the previous ordered cursor list when remeasurement preserved every navigation target. */ +export function reuseEquivalentLineCursors(previous: LineCursor[], next: LineCursor[]) { + if ( + previous.length === next.length && + next.every((cursor, index) => { + const prior = previous[index]; + return ( + prior?.fileId === cursor.fileId && + prior.hunkIndex === cursor.hunkIndex && + prior.stableKey === cursor.stableKey && + prior.expandedGapKey === cursor.expandedGapKey && + prior.target.side === cursor.target.side && + prior.target.line === cursor.target.line + ); + }) + ) { + return previous; + } + return next; +} + /** Find the first cursor in one hunk, then anywhere in its file. */ function nearestCursorInFile(cursors: LineCursor[], fileId: string, hunkIndex: number) { return ( diff --git a/src/ui/lib/viewportTiming.test.ts b/src/ui/lib/viewportTiming.test.ts index 29cce55be..1c14287ec 100644 --- a/src/ui/lib/viewportTiming.test.ts +++ b/src/ui/lib/viewportTiming.test.ts @@ -9,6 +9,10 @@ describe("estimateInitialRenderViewportHeight", () => { test("never returns an empty window while geometry is still unknown", () => { expect(estimateInitialRenderViewportHeight(0, 0)).toBe(1); }); + + test("does not include a bottom pane in the review viewport estimate", () => { + expect(estimateInitialRenderViewportHeight(100, 1, 5)).toBe(5); + }); }); describe("resolveRenderViewportHeight", () => { diff --git a/src/ui/lib/viewportTiming.ts b/src/ui/lib/viewportTiming.ts index d3cbc595a..83fb2103d 100644 --- a/src/ui/lib/viewportTiming.ts +++ b/src/ui/lib/viewportTiming.ts @@ -4,10 +4,21 @@ export const VIEWPORT_READ_COALESCE_MS = 16; /** * Estimate render-only viewport bounds before OpenTUI publishes exact scrollbox geometry. * Subtracts the review pane's screen-top offset from the renderer height so the first paint - * can window files without waiting for the scrollbox to report its laid-out height. + * can window files without waiting for the scrollbox to report its laid-out height. A planned + * pane height excludes any extension pane below the review. */ -export function estimateInitialRenderViewportHeight(rendererHeight: number, screenTop: number) { - return Math.max(1, rendererHeight - Math.max(0, screenTop)); +export function estimateInitialRenderViewportHeight( + rendererHeight: number, + screenTop: number, + paneHeight?: number, +) { + const availableRendererHeight = rendererHeight - Math.max(0, screenTop); + return Math.max( + 1, + paneHeight === undefined + ? availableRendererHeight + : Math.min(availableRendererHeight, paneHeight), + ); } /** From 8fd791fd31cc8f3b45660c198f4d3b7c5986908c Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 30 Aug 2026 17:30:52 -0400 Subject: [PATCH 3/3] fix(ui): let edge jumps supersede pending reveals --- src/ui/AppHost.interactions.test.tsx | 33 +++++++++++++++++ src/ui/components/panes/DiffPane.tsx | 53 +++++++++++++++------------- 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 343061778..7450abd7f 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -2494,6 +2494,39 @@ describe("App interactions", () => { } }); + test("G supersedes a pending selected-hunk reveal", async () => { + const setup = await testRender( + , + { + width: 120, + height: 16, + }, + ); + + try { + await flush(setup); + await settleViewportMeasurement(setup); + await pressHunkNavigationKey(setup, "]", 1); + + await act(async () => { + await setup.mockInput.pressKey("g", { shift: true }); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(160); + await setup.renderOnce(); + }); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("export const mid = 4;"); + expect(frame).not.toContain("line 021 changed"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("pager mode also supports G and g top/bottom jumps", async () => { const before = Array.from( diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 53eabe020..728dea0c5 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -669,6 +669,30 @@ export function DiffPane({ // is required before passive viewport-follow selection can trigger. const lastViewportSelectionTopRef = useRef(null); const lastViewportRowAnchorRef = useRef(null); + // Track the previous selected anchor to detect actual selection changes. + const prevSelectedAnchorIdRef = useRef(null); + const prevPinnedHeaderFileIdRef = useRef(null); + const pendingSelectionSettleRef = useRef(false); + const pendingSelectionRevealTimeoutsRef = useRef[]>([]); + + /** Clear scheduled selection-reveal retries without changing the resettle policy. */ + const clearPendingSelectionRevealTimers = useCallback(() => { + for (const timeout of pendingSelectionRevealTimeoutsRef.current) { + clearTimeout(timeout); + } + pendingSelectionRevealTimeoutsRef.current = []; + }, []); + + /** Retire selection reveal work once another explicit scroll policy becomes authoritative. */ + const supersedePendingSelectionReveal = useCallback(() => { + clearPendingSelectionRevealTimers(); + pendingSelectionSettleRef.current = false; + }, [clearPendingSelectionRevealTimers]); + + /** Clear any pending "selected file to top" follow-up. */ + const clearPendingFileTopAlign = useCallback(() => { + pendingFileTopAlignFileIdRef.current = null; + }, []); /** Track the currently hover-owned file without making scroll handlers depend on render state. */ const setHoveredFileForRowActions = useCallback((fileId: string) => { @@ -978,6 +1002,8 @@ export function DiffPane({ return; } + supersedePendingSelectionReveal(); + clearPendingFileTopAlign(); previousScrollEdgeRequestIdRef.current = pendingScrollEdgeRequest.id; const viewportHeight = scrollBox.viewport.height || scrollEdgeViewportHeight; const nextTop = clampVerticalScrollTop( @@ -988,10 +1014,12 @@ export function DiffPane({ setScrollViewport({ top: nextTop, height: viewportHeight }); scrollBox.scrollTo(nextTop); }, [ + clearPendingFileTopAlign, pendingScrollEdgeRequest, requestedScrollEdgeTop, scrollEdgeViewportHeight, scrollRef, + supersedePendingSelectionReveal, totalContentHeight, ]); const fileSectionIndexById = useMemo( @@ -1885,31 +1913,6 @@ export function DiffPane({ const selectedFileBodyTop = selectedFileIndex >= 0 ? (fileSectionLayouts[selectedFileIndex]?.bodyTop ?? 0) : 0; - // Track the previous selected anchor to detect actual selection changes. - const prevSelectedAnchorIdRef = useRef(null); - const prevPinnedHeaderFileIdRef = useRef(null); - const pendingSelectionSettleRef = useRef(false); - const pendingSelectionRevealTimeoutsRef = useRef[]>([]); - - /** Clear scheduled selection-reveal retries without changing the resettle policy. */ - const clearPendingSelectionRevealTimers = useCallback(() => { - for (const timeout of pendingSelectionRevealTimeoutsRef.current) { - clearTimeout(timeout); - } - pendingSelectionRevealTimeoutsRef.current = []; - }, []); - - /** Retire selection reveal work once an explicit line alignment becomes authoritative. */ - const supersedePendingSelectionReveal = useCallback(() => { - clearPendingSelectionRevealTimers(); - pendingSelectionSettleRef.current = false; - }, [clearPendingSelectionRevealTimers]); - - /** Clear any pending "selected file to top" follow-up. */ - const clearPendingFileTopAlign = useCallback(() => { - pendingFileTopAlignFileIdRef.current = null; - }, []); - /** * Report whether the align has landed as far as the rest of this pane can observe it. *