Skip to content

fix(ui): preserve viewport position when switching layouts #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 2 commits into from
Apr 8, 2026
Merged

fix(ui): preserve viewport position when switching layouts #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
755a7ff
fix(ui): preserve viewport position when switching layouts
benvinegar Apr 8, 2026
532e51a
fix(ui): address viewport anchor review feedback
benvinegar Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/ui/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export function App({
const sidebarScrollRef = useRef<ScrollBoxRenderable | null>(null);
const diffScrollRef = useRef<ScrollBoxRenderable | null>(null);
const wrapToggleScrollTopRef = useRef<number | null>(null);
const layoutToggleScrollTopRef = useRef<number | null>(null);
const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0);
const [layoutMode, setLayoutMode] = useState<LayoutMode>(bootstrap.initialMode);
const [themeId, setThemeId] = useState(
() => resolveTheme(bootstrap.initialTheme, renderer.themeMode).id,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -472,7 +481,7 @@ export function App({
moveToHunk: review.moveToHunk,
refreshCurrentInput: triggerRefreshCurrentInput,
requestQuit,
selectLayoutMode: setLayoutMode,
selectLayoutMode,
selectThemeId: setThemeId,
showAgentNotes,
showHelp,
Expand All @@ -497,6 +506,7 @@ export function App({
moveToAnnotatedHunk,
requestQuit,
review.moveToHunk,
selectLayoutMode,
triggerRefreshCurrentInput,
showAgentNotes,
showHelp,
Expand Down Expand Up @@ -547,7 +557,7 @@ export function App({
requestQuit,
scrollCodeHorizontally,
scrollDiff,
selectLayoutMode: setLayoutMode,
selectLayoutMode,
showHelp,
switchMenu,
toggleAgentNotes,
Expand Down Expand Up @@ -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}
Expand Down
65 changes: 65 additions & 0 deletions src/ui/AppHost.interactions.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -1244,6 +1248,67 @@ describe("App interactions", () => {
}
});

test("layout toggles preserve the current viewport anchor across split and stack", async () => {
const setup = await testRender(<AppHost bootstrap={createLineScrollBootstrap()} />, {
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 =
Expand Down
148 changes: 50 additions & 98 deletions src/ui/components/panes/DiffPane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { computeHunkRevealScrollTop } from "../../lib/hunkScroll";
import {
measureDiffSectionGeometry,
type DiffSectionGeometry,
type DiffSectionRowBounds,
} from "../../lib/diffSectionGeometry";
import {
buildFileSectionLayouts,
Expand All @@ -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";
Expand All @@ -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) {
Expand Down Expand Up @@ -217,6 +128,8 @@ export function DiffPane({
showHunkHeaders,
wrapLines,
wrapToggleScrollTop,
layoutToggleScrollTop = null,
layoutToggleRequestId = 0,
selectedFileTopAlignRequestId = 0,
selectedHunkRevealRequestId,
theme,
Expand All @@ -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;
Expand Down Expand Up @@ -333,8 +248,10 @@ export function DiffPane({
const prevScrollTopRef = useRef(0);
const previousSectionGeometryRef = useRef<DiffSectionGeometry[] | null>(null);
const previousFilesRef = useRef<DiffFile[]>(files);
const previousLayoutRef = useRef(layout);
const previousWrapLinesRef = useRef(wrapLines);
const previousSelectedFileTopAlignRequestIdRef = useRef(selectedFileTopAlignRequestId);
const previousLayoutToggleRequestIdRef = useRef(layoutToggleRequestId);
const previousSelectedHunkRevealRequestIdRef = useRef(selectedHunkRevealRequestId);
const pendingFileTopAlignFileIdRef = useRef<string | null>(null);
const suppressViewportSelectionSyncRef = useRef(false);
Expand All @@ -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<number | null>(null);
const lastViewportRowAnchorRef = useRef<ViewportRowAnchor | null>(null);

/**
* Ignore viewport-follow selection updates while the pane is scrolling to an explicit selection.
Expand Down Expand Up @@ -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 previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles);
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));
Comment on lines +661 to +671
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 previousSectionHeaderHeights is recomputed on every render

buildInStreamFileHeaderHeights(previousFiles) is called unconditionally inside the effect even when neither layoutChanged nor wrapChanged is true. The value is only read inside the if block at line 662, so the allocation happens on every steady-state render without benefit. Hoisting the call inside the guard avoids unnecessary work:

Suggested change
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));
if ((layoutChanged || wrapChanged) && previousSectionMetrics && previousFiles.length > 0) {
const previousSectionHeaderHeights = buildInStreamFileHeaderHeights(previousFiles);
const previousScrollTop =

(The const previousSectionHeaderHeights = … declaration currently on line 659 would be removed.)

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved buildInStreamFileHeaderHeights(previousFiles) inside the relayout guard so we only compute it when layout or wrap restoration actually runs.

This comment was generated by Pi using OpenAI o3

const anchor = findViewportRowAnchor(
previousFiles,
previousSectionMetrics,
previousScrollTop,
previousSectionHeaderHeights,
lastViewportRowAnchorRef.current?.stableKey,
);
if (anchor) {
const nextTop = resolveViewportRowAnchorTop(
Expand All @@ -763,13 +687,16 @@ 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
// after wrapped row heights and viewport culling settle.
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;
Expand All @@ -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,
Expand All @@ -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;
Expand Down
Loading
Loading