perf: widen diff overscan during rapid scrolling#391
Conversation
Greptile SummaryThis PR adds an adaptive overscan halo for rapid scrolling in
Confidence Score: 4/5Safe to merge. The changes are well-scoped and self-contained; the worst-case outcome of any issue is a brief 160ms over-rendering at remount, which is invisible to users. The core logic is straightforward and the timeout lifecycle is handled correctly. The one behavioral gap — prevScrollTopRef starting at 0 so a remount with a non-zero scroll position briefly fires an unneeded overscan burst — is transient and harmless. The threshold boundary at deltaRows === 4 is also untested, leaving a minor gap in confidence that the strict < 4 guard is intentional. src/ui/components/panes/DiffPane.tsx around the initial readViewport() call; src/ui/lib/adaptiveScrollOverscan.test.ts is missing a boundary-value case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Scroll event fires] --> B[readViewport via microtask]
B --> C{nextTop changed?}
C -- No --> G[Update scrollViewport state]
C -- Yes --> D[computeRapidScrollOverscanRows\ndeltaRows and viewportHeight]
D --> E{absDelta less than 4 rows?}
E -- Yes --> F[return 0]
E -- No --> H[min 240, max absDelta x2 vs viewportRows x3]
F --> I[activateRapidScrollOverscan\nearly-exit: no change]
H --> J[activateRapidScrollOverscan overscanRows]
J --> K[setRapidScrollOverscanRows to max of current and new]
K --> L[Reset idle timer 160ms]
L --> M[rapidScrollOverscanRows drives windowing]
M --> N1[visibleViewportFileIds: max 8 or rows]
M --> N2[Mounted diff window: max 24 or height x2 or rows]
M --> N3[Highlight prefetch: max 24 or height x3 or rows]
L --> O[On idle timeout: reset rows to 0]
G --> P[Render]
I --> G
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/ui/components/panes/DiffPane.tsx:504-513
**False overscan burst on remount with non-zero scroll position**
`prevScrollTopRef` is always initialized to `0`. When `readViewport()` is called immediately on effect setup (line 550), any non-zero `scrollBox.scrollTop` — which can happen if `DiffPane` unmounts and remounts while the underlying scroll box retains its position — will compute `deltaRows = nextTop - 0`. A value ≥ 4 rows triggers `activateRapidScrollOverscan`, adding a 160 ms overscan burst even though no real rapid scrolling occurred. The impact is minor (a brief over-render and then reset), but could be avoided by initializing `prevScrollTopRef` lazily from `scrollBox.scrollTop` before the first scroll-delta check.
### Issue 2 of 2
src/ui/lib/adaptiveScrollOverscan.test.ts:5-8
The exact threshold boundary `deltaRows === RAPID_SCROLL_MIN_DELTA_ROWS` (4) is untested. Since the guard is `< 4` (strictly less than), `deltaRows: 4` should trigger the calculation — but there's no assertion confirming that. Adding a case at `4` would pin the inclusive boundary explicitly.
```suggestion
test("leaves slow row-by-row movement on the default window", () => {
expect(computeRapidScrollOverscanRows({ deltaRows: 1, viewportHeight: 30 })).toBe(0);
expect(computeRapidScrollOverscanRows({ deltaRows: -3, viewportHeight: 30 })).toBe(0);
// Exactly at the threshold (4) is the first value that should trigger expansion.
expect(computeRapidScrollOverscanRows({ deltaRows: 4, viewportHeight: 30 })).toBe(90);
});
```
Reviews (1): Last reviewed commit: "perf: widen diff overscan during rapid s..." | Re-trigger Greptile |
| if (nextTop !== prevScrollTopRef.current) { | ||
| const previousTop = prevScrollTopRef.current; | ||
| scrollbarRef.current?.show(); | ||
| clearAddNoteHoverForScroll(); | ||
| activateRapidScrollOverscan( | ||
| computeRapidScrollOverscanRows({ | ||
| deltaRows: nextTop - previousTop, | ||
| viewportHeight: nextHeight, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
False overscan burst on remount with non-zero scroll position
prevScrollTopRef is always initialized to 0. When readViewport() is called immediately on effect setup (line 550), any non-zero scrollBox.scrollTop — which can happen if DiffPane unmounts and remounts while the underlying scroll box retains its position — will compute deltaRows = nextTop - 0. A value ≥ 4 rows triggers activateRapidScrollOverscan, adding a 160 ms overscan burst even though no real rapid scrolling occurred. The impact is minor (a brief over-render and then reset), but could be avoided by initializing prevScrollTopRef lazily from scrollBox.scrollTop before the first scroll-delta check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 504-513
Comment:
**False overscan burst on remount with non-zero scroll position**
`prevScrollTopRef` is always initialized to `0`. When `readViewport()` is called immediately on effect setup (line 550), any non-zero `scrollBox.scrollTop` — which can happen if `DiffPane` unmounts and remounts while the underlying scroll box retains its position — will compute `deltaRows = nextTop - 0`. A value ≥ 4 rows triggers `activateRapidScrollOverscan`, adding a 160 ms overscan burst even though no real rapid scrolling occurred. The impact is minor (a brief over-render and then reset), but could be avoided by initializing `prevScrollTopRef` lazily from `scrollBox.scrollTop` before the first scroll-delta check.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed by making the first viewport read a baseline snapshot. A retained non-zero scrollTop on remount no longer counts as scroll input or activates rapid overscan.
This comment was generated by Pi using OpenAI GPT-5
| test("leaves slow row-by-row movement on the default window", () => { | ||
| expect(computeRapidScrollOverscanRows({ deltaRows: 1, viewportHeight: 30 })).toBe(0); | ||
| expect(computeRapidScrollOverscanRows({ deltaRows: -3, viewportHeight: 30 })).toBe(0); | ||
| }); |
There was a problem hiding this comment.
The exact threshold boundary
deltaRows === RAPID_SCROLL_MIN_DELTA_ROWS (4) is untested. Since the guard is < 4 (strictly less than), deltaRows: 4 should trigger the calculation — but there's no assertion confirming that. Adding a case at 4 would pin the inclusive boundary explicitly.
| test("leaves slow row-by-row movement on the default window", () => { | |
| expect(computeRapidScrollOverscanRows({ deltaRows: 1, viewportHeight: 30 })).toBe(0); | |
| expect(computeRapidScrollOverscanRows({ deltaRows: -3, viewportHeight: 30 })).toBe(0); | |
| }); | |
| test("leaves slow row-by-row movement on the default window", () => { | |
| expect(computeRapidScrollOverscanRows({ deltaRows: 1, viewportHeight: 30 })).toBe(0); | |
| expect(computeRapidScrollOverscanRows({ deltaRows: -3, viewportHeight: 30 })).toBe(0); | |
| // Exactly at the threshold (4) is the first value that should trigger expansion. | |
| expect(computeRapidScrollOverscanRows({ deltaRows: 4, viewportHeight: 30 })).toBe(90); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/lib/adaptiveScrollOverscan.test.ts
Line: 5-8
Comment:
The exact threshold boundary `deltaRows === RAPID_SCROLL_MIN_DELTA_ROWS` (4) is untested. Since the guard is `< 4` (strictly less than), `deltaRows: 4` should trigger the calculation — but there's no assertion confirming that. Adding a case at `4` would pin the inclusive boundary explicitly.
```suggestion
test("leaves slow row-by-row movement on the default window", () => {
expect(computeRapidScrollOverscanRows({ deltaRows: 1, viewportHeight: 30 })).toBe(0);
expect(computeRapidScrollOverscanRows({ deltaRows: -3, viewportHeight: 30 })).toBe(0);
// Exactly at the threshold (4) is the first value that should trigger expansion.
expect(computeRapidScrollOverscanRows({ deltaRows: 4, viewportHeight: 30 })).toBe(90);
});
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added a boundary test for deltaRows === 4 so the inclusive burst threshold is explicit.
This comment was generated by Pi using OpenAI GPT-5
48d20e4 to
169a849
Compare
Summary
Validation
bun test src/ui/lib/adaptiveScrollOverscan.test.ts src/ui/components/ui-components.test.tsx src/ui/diff/rowWindowing.test.tsbun test ./test/pty/scroll.test.tsbun run format:checkbun run typecheckbun run lintbun run bench:large-streamThis PR description was generated by Pi using OpenAI GPT-5