fix(ui): preserve viewport position when switching layouts

[benvinegar]

Summary

• preserve the visible review position when switching between split and stack layouts by capturing the pre-toggle viewport anchor and restoring the matching logical row after relayout
• add layout-independent stable row keys so split/stack transitions can map back to the same code, including ambiguous changed rows when the user toggles back and forth
• add unit, app interaction, PTY integration, and TTY smoke coverage for layout-toggle viewport preservation

Testing

• bun run typecheck
• bun run lint
• bun test
• bun run test:integration
• bun run test:tty-smoke
• manual PTY smoke run of bun run src/main.tsx -- diff /tmp/before.ts /tmp/after.ts --mode split

This PR description was generated by Pi using OpenAI o3

[greptile-apps]

Greptile Summary

This PR adds layout-toggle viewport preservation: when a user switches between split and stack diff layouts, the visible review position is maintained by capturing a stable row anchor before the relayout and restoring the equivalent scroll position after. It introduces:

• A new viewportAnchor.ts module with findViewportRowAnchor / resolveViewportRowAnchorTop that translate scroll offsets to stable row identities (keyed by file + source line numbers) and back
• Stable key infrastructure in reviewRenderPlan.ts (stableKey, stableAliasKeys) so split "change" rows and stack deletion/addition rows can cross-reference each other
• rowBoundsByStableKey lookup maps in diffSectionGeometry.ts for O(1) post-relayout anchor resolution
• A layoutToggleScrollTop / layoutToggleRequestId signal pair in App.tsx that captures the pre-toggle scroll position synchronously before React re-renders
• A useLayoutEffect in DiffPane.tsx that consumes the captured anchor, resolves the new scroll position, and retries across a few repaint cycles to let row heights settle
• Unit, interaction, PTY integration, and TTY smoke test coverage

The change fits naturally into the existing viewport-anchor pattern already used by the wrap-toggle path, reusing the same helper structure and retry logic. No previous systems are orphaned.

Confidence Score: 5/5

Safe to merge — only P2 style suggestions remain, no correctness or reliability issues.

The implementation is well-structured and directly reuses the existing wrap-toggle viewport-anchor pattern. Both findings are P2: a latent zero-height guard in resolveViewportRowAnchorTop (unreachable in practice) and a minor buildInStreamFileHeaderHeights call that runs on every steady-state render. All changed code paths are covered by unit tests, app interaction tests, PTY integration tests, and a TTY smoke test. No prior systems are orphaned.

src/ui/lib/viewportAnchor.ts — minor defensive hardening opportunity on line 109.

Vulnerabilities

No security concerns identified. The changes are purely UI/rendering logic — viewport scroll offset arithmetic, stable row key generation, and React effect coordination — with no network access, user-controlled data flowing into dangerous sinks, or secrets handling.

Important Files Changed

Filename	Overview
src/ui/lib/viewportAnchor.ts	New module exposing find/resolve viewport anchor functions; binary-search row lookup and stable-key preference logic are correct, but the offset clamp has a latent edge case when rowBounds.height === 0.
src/ui/diff/reviewRenderPlan.ts	Adds stable key generation helpers and stableAliasKeys to planned diff-row entries; the split-prefers-old-line ordering choice is well-documented and the deduplication logic is correct.
src/ui/lib/diffSectionGeometry.ts	Extends DiffSectionGeometry with rowBounds, rowBoundsByKey, and rowBoundsByStableKey; first-write-wins semantics for the stable key map are intentional and correct.
src/ui/components/panes/DiffPane.tsx	Adds lastViewportRowAnchorRef tracking and a useLayoutEffect that detects explicit vs auto layout changes and restores the viewport anchor with retry delays; cleanly extends the existing wrap-toggle pattern.
src/ui/App.tsx	selectLayoutMode now captures layoutToggleScrollTopRef and increments layoutToggleRequestId before triggering the mode change, correctly separating the explicit-toggle signal from auto-layout changes.
src/ui/lib/viewportAnchor.test.ts	Good unit coverage for preferred-stable-key disambiguation and the full round-trip (stack deletion → split → stack); correctly exercises the ambiguous changed-row case.
src/ui/AppHost.interactions.test.tsx	Adds a layout-toggle viewport anchor test using the existing line-scroll fixture; settles with an inline sleep+render pattern consistent with wrap toggle tests.
test/pty/ui-integration.test.ts	Adds PTY integration tests for mouse/keyboard layout switching; covers both split-to-stack and stack-to-split paths in a real terminal session.

Sequence Diagram

sequenceDiagram
    actor User
    participant App
    participant DiffPane
    participant viewportAnchor

    User->>App: press "1" or "2" (layout toggle)
    App->>App: capture layoutToggleScrollTopRef = scrollTop
    App->>App: setLayoutToggleRequestId(n+1)
    App->>App: setLayoutMode(newMode)
    App->>DiffPane: re-render with new layout + layoutToggleScrollTop + layoutToggleRequestId

    Note over DiffPane: useLayoutEffect fires (toggle path)
    DiffPane->>viewportAnchor: findViewportRowAnchor(previousFiles, previousGeometry, capturedScrollTop, preferredStableKey)
    viewportAnchor-->>DiffPane: ViewportRowAnchor { fileId, rowKey, stableKey, rowOffsetWithin }

    DiffPane->>viewportAnchor: resolveViewportRowAnchorTop(files, newGeometry, anchor, headerHeights)
    viewportAnchor-->>DiffPane: nextScrollTop

    DiffPane->>DiffPane: scrollRef.scrollTo(nextScrollTop)
    DiffPane->>DiffPane: schedule retries at 0ms / 16ms / 48ms

    Note over DiffPane: anchor-tracking useLayoutEffect fires
    DiffPane->>viewportAnchor: findViewportRowAnchor(files, newGeometry, currentScrollTop, prevStableKey)
    viewportAnchor-->>DiffPane: updated lastViewportRowAnchorRef

Loading

_{Reviews (1): Last reviewed commit: "fix(ui): preserve viewport position when..." | Re-trigger Greptile}

[greptile-apps]

Offset clamp is not guarded against zero-height rows

rowBounds.height - 1 is -1 when height === 0, so Math.min(anchor.rowOffsetWithin, -1) returns -1 and the resolved scroll position lands one unit above the row. In practice this is unreachable today (every planned row has height ≥ 1), but a defensive Math.max(0, ...) guard would make the contract explicit:

Suggested change

	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));

[benvinegar]

Added the defensive clamp so a zero-height row cannot resolve one line above its anchor.

This comment was generated by Pi using OpenAI o3

[greptile-apps]

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.)

[benvinegar]

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