Pin the current file header while scrolling the review pane#141
Pin the current file header while scrolling the review pane#141benvinegar merged 4 commits intomodem-dev:mainfrom
Conversation
Greptile SummaryThis PR implements sticky file-header pinning in the review pane: as you scroll past a file header it stays anchored at the top of the diff pane until the next file's header scrolls into view. Sidebar clicks now also align the selected file's header to exactly the top of the viewport. The implementation is well-structured — layout geometry is extracted into a new Key changes:
Minor notes:
Confidence Score: 5/5Safe to merge — all findings are P2 style/robustness concerns with no functional impact on the primary review-pane scrolling path. The sticky-header ownership logic is correct (binary search correctly attributes separator rows to the previous section, first-section edge case handled, single-file suppression is intentional). The counter-increment pattern for top-align requests safely handles repeated sidebar clicks on the same file. The addition of prevSelectedAnchorIdRef update when consuming the suppress flag is a genuine correctness improvement that also fixes a latent race in the wrap-toggle path. The two remaining P2 notes (pending-ref linger on deleted file, hard-coded sleep in one test) do not affect normal usage. src/ui/components/panes/DiffPane.tsx (pendingFileTopAlignFileIdRef retry loop) and test/app-interactions.test.tsx (Bun.sleep timing) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Sidebar
participant App
participant DiffPane
participant ScrollBox
User->>Sidebar: click file row
Sidebar->>App: onSelectFile(fileId)
App->>App: setSelectedFileId(fileId)
App->>App: setSelectedFileTopAlignRequestId(id + 1)
App->>DiffPane: re-render with new selectedFileTopAlignRequestId
DiffPane->>DiffPane: useLayoutEffect [selectedFileTopAlignRequestId changed]
DiffPane->>DiffPane: suppressNextSelectionAutoScroll = true
DiffPane->>DiffPane: pendingFileTopAlignFileId = fileId
DiffPane->>ScrollBox: scrollTo(headerTop)
DiffPane->>DiffPane: useLayoutEffect [sectionLayoutMetrics] (retry)
alt scrollFileHeaderToTop succeeds
DiffPane->>ScrollBox: scrollTo(headerTop)
DiffPane->>DiffPane: clearPendingFileTopAlign()
end
DiffPane->>DiffPane: useLayoutEffect selection auto-scroll
DiffPane->>DiffPane: suppress flag set → skip auto-scroll
DiffPane->>DiffPane: prevSelectedAnchorId = selectedAnchorId
User->>ScrollBox: scroll (any direction)
ScrollBox-->>DiffPane: scrollViewport state update (50ms poll)
DiffPane->>DiffPane: effectiveScrollTop = scrollRef.current.scrollTop
DiffPane->>DiffPane: stickyHeaderFile = findHeaderOwningSection(...)
alt scrollTop > owner.headerTop
DiffPane->>DiffPane: render sticky DiffFileHeaderRow overlay
else scrollTop <= owner.headerTop
DiffPane->>DiffPane: hide sticky overlay
end
Reviews (1): Last reviewed commit: "Pin file headers in the review pane" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const effectiveScrollTop = scrollRef.current?.scrollTop ?? scrollViewport.top; | ||
|
|
||
| // Calculate total content height including separators and headers | ||
| const totalContentHeight = useMemo(() => { | ||
| let total = 0; | ||
| for (let index = 0; index < files.length; index += 1) { | ||
| // Separator between files (except first) | ||
| if (index > 0) { | ||
| total += 1; | ||
| } | ||
| // File header | ||
| total += 1; | ||
| // File body | ||
| total += estimatedBodyHeights[index] ?? 0; | ||
| const totalContentHeight = | ||
| sectionLayoutMetrics[sectionLayoutMetrics.length - 1]?.sectionBottom ?? 0; | ||
|
|
||
| const stickyHeaderFile = useMemo(() => { | ||
| if (files.length < 2) { | ||
| return null; | ||
| } | ||
| return total; | ||
| }, [files.length, estimatedBodyHeights]); | ||
|
|
||
| const owner = findHeaderOwningSection(sectionLayoutMetrics, effectiveScrollTop); | ||
| if (!owner || effectiveScrollTop <= owner.headerTop) { | ||
| return null; | ||
| } | ||
|
|
||
| return files[owner.sectionIndex] ?? null; | ||
| }, [effectiveScrollTop, files, sectionLayoutMetrics]); |
There was a problem hiding this comment.
effectiveScrollTop is computed inline from scrollRef.current?.scrollTop during the render phase. This works correctly in practice because scroll events drive setScrollViewport state updates (via the 50 ms polling interval), which trigger re-renders that re-read the freshest DOM value before the stickyHeaderFile memo re-evaluates. However, the coupling between the polled state update and the ref read is implicit. A brief clarifying comment here would help future readers understand why scrollRef.current?.scrollTop is preferred over scrollViewport.top (it avoids up-to-50 ms lag between a programmatic scroll and the sticky-header handoff).
There was a problem hiding this comment.
Added an inline comment explaining why the sticky header reads the live scrollbox value during render: it keeps header ownership in sync with imperative scroll-to-header jumps instead of waiting for the polled viewport snapshot.
This comment was generated by Pi using gpt-5
src/ui/components/panes/DiffPane.tsx
Outdated
| useLayoutEffect(() => { | ||
| const pendingFileId = pendingFileTopAlignFileIdRef.current; | ||
| if (!pendingFileId) { | ||
| return; | ||
| } | ||
|
|
||
| if (scrollFileHeaderToTop(pendingFileId)) { | ||
| clearPendingFileTopAlign(); | ||
| } | ||
| }, [clearPendingFileTopAlign, scrollFileHeaderToTop, sectionLayoutMetrics]); |
There was a problem hiding this comment.
pendingFileTopAlignFileIdRef can linger if scrollFileHeaderToTop never returns true
scrollFileHeaderToTop returns false when either resolveFileHeaderTop finds no matching metric or scrollRef.current is null. In either case clearPendingFileTopAlign is never called, so pendingFileTopAlignFileIdRef.current stays set indefinitely. Every subsequent sectionLayoutMetrics change (e.g. as windowed sections mount and report measured heights) would re-trigger the effect and keep retrying — harmless scrolls if the file eventually appears, but a permanent no-op loop if it never does (e.g. a file deleted from the changeset between click and metrics settlement).
A simple guard — clearing the pending when files no longer contains pendingFileId — would bound the retry lifetime:
useLayoutEffect(() => {
const pendingFileId = pendingFileTopAlignFileIdRef.current;
if (!pendingFileId) {
return;
}
// Stop retrying if the file was removed from the changeset.
const fileStillPresent = sectionLayoutMetrics.some((m) => m.fileId === pendingFileId);
if (!fileStillPresent || scrollFileHeaderToTop(pendingFileId)) {
clearPendingFileTopAlign();
}
}, [clearPendingFileTopAlign, scrollFileHeaderToTop, sectionLayoutMetrics]);There was a problem hiding this comment.
Good catch. I now clear the pending top-align request if the target file disappears before the retry can land, so we stop retrying a stale sidebar selection.
This comment was generated by Pi using gpt-5
| // Move partway into the first file so ownership can visibly change on sidebar selection. | ||
| for (let index = 0; index < 8; index += 1) { | ||
| await act(async () => { | ||
| await setup.mockInput.pressArrow("down"); | ||
| }); | ||
| await flush(setup); | ||
| } | ||
|
|
||
| await act(async () => { | ||
| // Click inside the second file row in the left sidebar. | ||
| await setup.mockMouse.click(6, 4); |
There was a problem hiding this comment.
The surrounding ui-components.test.tsx defines a waitForFrame predicate-polling helper (added in this same PR) that retries up to 8 times with 50 ms gaps. Using a hard-coded Bun.sleep(80) here gives only one chance and assumes the scroll always settles within that window. Under load or in a slow CI runner this can fail spuriously. Replicating (or importing) the waitForFrame pattern used in ui-components.test.tsx would make this assertion as robust as the component-level sticky-header test.
There was a problem hiding this comment.
Replaced the fixed sleep in this interaction test with the existing frame-polling helper, so the assertion waits for the pinned-header handoff instead of assuming a single 80 ms settle window.
This comment was generated by Pi using gpt-5
benvinegar
force-pushed
the
tanvesh01/issue-112-plan
branch
from
ba27c6d to
2caa253
Compare
|
@tanvesh01 - wanted this, thanks! |
2 participants
Closes #112
Summary
Testing
bun run typecheckbun test test/ui-components.test.tsx test/app-interactions.test.tsxbun test, but Bun 1.3.5 segfaulted on this machine after running a large portion of the suitebun run test:tty-smoke, but the local macOSscriptbinary here rejects-f(script: illegal option -- f)Notes