Skip to content

Fix diff pane header popping while scrolling#159

Merged
benvinegar merged 9 commits intomainfrom
investigate/smooth-scroll-pin-bounds
Apr 4, 2026
Merged

Fix diff pane header popping while scrolling#159
benvinegar merged 9 commits intomainfrom
investigate/smooth-scroll-pin-bounds

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • stabilize pinned file-header ownership at file boundaries and remove the top-row scroll pop
  • always pin the current file header and hide the first in-stream header so the review pane keeps a stable top row
  • add regression coverage for collapsed-gap scrolling, header handoff, sidebar jumps, PTY wheel behavior, and clean up follow-up naming/helpers

Testing

  • bun test
  • bun run test:tty-smoke

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR stabilizes the pinned file-header behavior in the diff pane by replacing the old "pop-in sticky header" pattern with an always-pinned top row and removing the in-stream header only for the first file. Scroll math throughout (scrollFileHeaderToTop, findViewportRowAnchor, resolveViewportRowAnchorTop) is updated to use bodyTop instead of headerTop so the new layout model is consistent end-to-end. The viewport polling loop is also replaced with proper event listeners (verticalScrollBar change, viewport layout-changed, viewport resized), and an intermediateRender call is triggered on pinned-header ownership changes to eliminate the one-frame lag on ownership handoff.

  • fileSectionLayout.ts: Adds getInStreamFileHeaderHeight, shouldRenderInStreamFileHeader, and buildInStreamFileHeaderHeights to centralize the "first file has no in-stream header" policy; removes the now-unused getFileSectionHeaderTop helper.
  • DiffPane.tsx: Renames stickyHeaderFilepinnedHeaderFile, changes guard from files.length < 2 to files.length === 0 (single-file diffs now show the always-pinned header too), passes sectionHeaderHeights to all buildFileSectionLayouts calls, and adds pinnedHeaderChangedWhileSettling settle logic to handle post-jump header handoff.
  • DiffSection.tsx / DiffSectionPlaceholder.tsx: Add showHeader: boolean prop; the first file's in-stream header is hidden, all others keep theirs so placeholder geometry matches the real section geometry.
  • Tests: Adds regression coverage for collapsed-gap scrolling, boundary/handoff scenarios, sidebar jumps, and PTY wheel behavior; updates offset calculations in existing tests to reflect the new layout math.

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 style suggestions with no current bugs.

The implementation is logically correct: the effectiveScrollTop - 1 handoff boundary, the bodyTop-based scroll alignment, the event-based viewport tracking, and the settle logic all behave as intended and are backed by thorough unit and PTY integration tests. The only finding is a P2 inconsistency in the buildFileSectionLayouts default fallback that has no impact on current callers.

src/ui/lib/fileSectionLayout.ts — minor default fallback inconsistency at line 40.

Important Files Changed

Filename Overview
src/ui/lib/fileSectionLayout.ts New helpers centralise the 'first file has no in-stream header' policy; buildFileSectionLayouts default fallback of ?? 1 for missing headerHeights is inconsistent with the new first-file behaviour.
src/ui/components/panes/DiffPane.tsx Core fix implemented correctly: always-pinned header, event-based viewport tracking replacing polling, settle logic for post-jump handoff, and consistent bodyTop-based scroll alignment throughout.
src/ui/components/panes/DiffSection.tsx Adds showHeader prop and removes dead selected prop; memo comparator updated correctly.
src/ui/components/panes/DiffSectionPlaceholder.tsx Adds showHeader prop so placeholder geometry matches real section geometry, keeping scroll math stable.
test/integration/tuistory-hunk.integration.ts Adds PTY wheel-scroll tests for header handoff and collapsed-gap round-trip; uses new createCollapsedTopRepoFixture and createPinnedHeaderRepoFixture helpers.
test/ui-components.test.tsx Updates scroll offsets for the new layout (secondHeaderTop = firstBodyHeight+1), adds stable-viewport-height assertions, and adds two new collapsed-gap unit tests.
test/app-interactions.test.tsx Adds collapsed-gap regression tests and updates existing sticky-header test to cover the new handoff sequence and stable viewport height invariant.
test/integration/tuistoryHarness.ts Adds createCollapsedTopRepoFixture for integration tests; correctly exported from harness return object.
test/tty-render-smoke.test.ts Updates expected visible row from after_06 to after_05, reflecting the new layout where the first file body starts at row 0 instead of row 1.

Sequence Diagram

sequenceDiagram
    participant User as User scroll/key
    participant ScrollBox as ScrollBox
    participant DiffPane as DiffPane
    participant PinnedHeader as Pinned Header Row
    participant Stream as In-Stream Content

    User->>ScrollBox: scroll or scrollTo(bodyTop)
    ScrollBox->>DiffPane: verticalScrollBar change event
    DiffPane->>DiffPane: updateViewport sets scrollViewport state
    DiffPane->>DiffPane: pinnedHeaderFile = findHeaderOwningFileSection(scrollTop - 1)
    DiffPane->>DiffPane: useLayoutEffect calls renderer.intermediateRender()
    DiffPane->>PinnedHeader: render current file header
    DiffPane->>Stream: render file bodies, first file has no in-stream header
    Note over PinnedHeader,Stream: In-stream header visible while scrollTop equals headerTop. Pinned header hands off one row later via the scrollTop minus one policy.
Loading

Reviews (1): Last reviewed commit: "Remove unused DiffSection selected prop" | Re-trigger Greptile


files.forEach((file, index) => {
const separatorHeight = index > 0 ? 1 : 0;
const headerHeight = Math.max(0, headerHeights?.[index] ?? 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Inconsistent default for headerHeights fallback

The fallback ?? 1 applies to all indices, including index === 0, but getInStreamFileHeaderHeight(0) returns 0. Any caller that omits headerHeights (e.g. a future test or a new call site) will compute an off-by-one bodyTop for the first file and all subsequent positions.

All current callers pass buildInStreamFileHeaderHeights(files) explicitly, so there is no current bug — but the inconsistency is a latent footgun. Consider changing the fallback to use getInStreamFileHeaderHeight:

Suggested change
const headerHeight = Math.max(0, headerHeights?.[index] ?? 1);
const headerHeight = Math.max(0, headerHeights?.[index] ?? getInStreamFileHeaderHeight(index));

This keeps the function self-consistent and makes the default match the actual policy.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch — I changed the fallback to use getInStreamFileHeaderHeight(index) so buildFileSectionLayouts() stays consistent even if a caller omits headerHeights.

Responded by pi using unknown-model.

@benvinegar benvinegar merged commit 6d6a983 into main Apr 4, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant