feat(diff): investigate row windowing for large single-file reviews#237
feat(diff): investigate row windowing for large single-file reviews#237benvinegar wants to merge 7 commits intomainfrom
Conversation
benvinegar
changed the title
Greptile SummaryThis PR introduces two performance improvements for large single-file diffs: a highlight policy that routes generated lockfiles to a plain-text or no-op highlight path, and an env-gated row-windowing proof-of-concept that slices the mounted diff-row tree down to a visible body window.
Confidence Score: 4/5The lockfile highlight change and the env-gated row-windowing POC both degrade gracefully to full rendering when geometry or bounds are absent, making this safe to merge. The two feature tracks are independent and both have targeted tests. The geometry/row-count alignment check in The Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DiffPane
scrollViewport + sectionGeometry] -->|computes per-file
visibleBodyBounds| B[visibleBodyBoundsByFile Map]
B --> C[DiffSection
visibleBodyBounds prop]
C --> D[PierreDiffView
visibleBodyBounds + sectionGeometry]
D -->|rowWindowingPocEnabled?| E{POC enabled?}
E -->|No / missing bounds| F[Render all plannedRows
topSpacer=0, bottomSpacer=0]
E -->|Yes| G[resolveVisiblePlannedRowWindow]
G --> H[Slice plannedRows to
visible index range]
H --> I[topSpacer box + sliced rows + bottomSpacer box]
J[DiffFile] --> K[resolveDiffHighlightMode]
K -->|full| L[prepareHighlighter language]
K -->|text| M[prepareHighlighter text]
K -->|none| N[Return empty highlighted arrays]
L --> O[renderDiffWithHighlighter]
M --> O
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return next; | ||
| } | ||
|
|
||
| const overscanRows = Math.max(24, scrollViewport.height * 2); |
There was a problem hiding this comment.
overscanRows is a coordinate offset, not a row count
The name overscanRows suggests it controls how many logical diff rows are kept in the render window, but it's actually a body-coordinate offset added to sectionLayout.bodyTop-relative values. Since each terminal row is 1 unit in this coordinate space the semantics aren't broken, but a future contributor tuning overscan might adjust this thinking they're working in logical diff-row units (where a wrapped line can be multiple terminal rows). A name like overscanPx or overscanTerminalRows would avoid the confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 599
Comment:
**Misleading variable name: `overscanRows` is a coordinate offset, not a row count**
The name `overscanRows` suggests it controls how many logical diff rows are kept in the render window, but it's actually a body-coordinate offset added to `sectionLayout.bodyTop`-relative values. Since each terminal row is 1 unit in this coordinate space the semantics aren't broken, but a future contributor tuning overscan might adjust this thinking they're working in logical diff-row units (where a wrapped line can be multiple terminal rows). A name like `overscanPx` or `overscanTerminalRows` would avoid the confusion.
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!
| firstVisibleIndex = index; | ||
| } | ||
| lastVisibleIndex = index; | ||
| } | ||
|
|
||
| if (firstVisibleIndex < 0 || lastVisibleIndex < 0) { |
There was a problem hiding this comment.
bottomSpacerHeight
When no row intersects the visible range (e.g. the file body is entirely scrolled past or not yet reached), the function returns topSpacerHeight: 0, bottomSpacerHeight: bodyHeight. Total height is preserved correctly, but the distribution is asymmetric: the rendered body is [spacer=0][rows=none][spacer=bodyHeight]. For a file entirely above the viewport it would be more semantically accurate to put the full height in topSpacerHeight. This is not a correctness bug today, but it could become one if downstream code assumes topSpacerHeight represents the distance from the body top to the first rendered row.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/rowWindowing.ts
Line: 71-76
Comment:
**Offscreen-file spacer layout always assigns all height to `bottomSpacerHeight`**
When no row intersects the visible range (e.g. the file body is entirely scrolled past or not yet reached), the function returns `topSpacerHeight: 0, bottomSpacerHeight: bodyHeight`. Total height is preserved correctly, but the distribution is asymmetric: the rendered body is `[spacer=0][rows=none][spacer=bodyHeight]`. For a file entirely *above* the viewport it would be more semantically accurate to put the full height in `topSpacerHeight`. This is not a correctness bug today, but it could become one if downstream code assumes `topSpacerHeight` represents the distance from the body top to the first rendered row.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Why
Large single-file diffs were still expensive even with file-level windowing, because mounting one visible file could still mount thousands of diff rows. This change virtualizes rows within a mounted file section so Hunk keeps the same review-stream geometry while rendering only the visible row slice for that file body.
What changed
DiffPanenow translates the global review viewport into file-local body bounds for mounted filesPierreDiffViewnow renders only the planned rows that overlap that visible body slicerowWindowing.tsowns the row-slice math and spacer-height preservationMeasurements
Using a synthetic large single-file diff with many distant hunks:
]avg:39.7ms]p95:50.1ms]avg:19.5ms]p95:23.2msUsing the isolated large-file renderer earlier in investigation:
4793140Notes
Validation
bun run typecheckbun testThis PR description was generated by Pi using OpenAI GPT-5