refactor(ui): centralize diff section row planning#359
Conversation
benvinegar
force-pushed
the
refactor/diff-section-row-plan
branch
from
3228212 to
e71c4c1
Compare
Greptile SummaryThis PR centralizes the diff-section row-planning pipeline into a new
Confidence Score: 4/5Safe to merge; the refactoring correctly preserves existing rendering and geometry behavior, with import updates verified across all call sites. The consolidation of four useMemo calls into one in PierreDiffView means that when visibleAgentNotes changes (e.g., during scroll), the comparatively expensive buildSplitRows/buildStackRows base-row build now runs unnecessarily alongside the cheap buildReviewRenderPlan update. For large diffs this could cause extra work on every note-visibility update, though the output values remain correct. src/ui/diff/PierreDiffView.tsx — the memo consolidation warrants a quick check on scroll performance with agent notes active on large diffs. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Rendering["PierreDiffView (rendering)"]
A["useMemo: buildDiffSectionRowPlan(...)"] --> B["plannedRows + lineNumberDigits"]
end
subgraph Geometry["measureDiffSectionGeometry (geometry + caching)"]
C["buildDiffSectionRowPlan(...)"] --> D["plannedRows + lineNumberDigits"]
D --> E["measurePlannedDiffSectionRowHeight x N"]
E --> F["DiffSectionGeometry"]
end
subgraph Plan["diffSectionRowPlan.ts (NEW shared layer)"]
G["buildBaseRows (buildSplitRows / buildStackRows)"] --> H["expandCollapsedRows"]
H --> I["findMaxLineNumberInRows -> lineNumberDigits"]
H --> J["buildReviewRenderPlan -> plannedRows"]
end
A -->|delegates| Plan
C -->|delegates| Plan
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/diff/PierreDiffView.tsx:192-216
**Unnecessary base-row rebuild when only `visibleAgentNotes` changes**
The single consolidated `useMemo` now reruns `buildBaseRows` (which calls `buildSplitRows`/`buildStackRows` over every hunk) and `expandCollapsedRows` whenever `visibleAgentNotes` changes. In the old code those two stages were in a separate memo keyed on `[file, layout, resolvedHighlighted, theme]`, so an agent-note visibility change only triggered the cheap `buildReviewRenderPlan` stage.
For large diffs the base-row build is non-trivial, and `visibleAgentNotes` can change during scroll. The refactoring loses that optimization by flattening all four memos into one.
### Issue 2 of 2
src/ui/diff/diffSectionGeometry.ts:17
**Duplicated `EMPTY_EXPANDED_GAP_KEYS` sentinel**
This constant is already defined in `diffSectionRowPlan.ts` (line 14) for the same purpose. Since `measureDiffSectionGeometry` passes the value down to `buildDiffSectionRowPlan`, the sentinel in this file could be replaced by importing (or relying on) the default in `buildDiffSectionRowPlan`'s parameter list, keeping one source of truth.
Reviews (1): Last reviewed commit: "refactor(ui): centralize diff section ro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const sectionRowPlan = useMemo( | ||
| () => | ||
| file | ||
| ? layout === "split" | ||
| ? buildSplitRows(file, resolvedHighlighted, theme) | ||
| : buildStackRows(file, resolvedHighlighted, theme) | ||
| : [], | ||
| [file, layout, resolvedHighlighted, theme], | ||
| ); | ||
| const expansionSide = file?.metadata.type === "deleted" ? "old" : "new"; | ||
| const fileHasSourceFetcher = Boolean(file?.sourceFetcher); | ||
| const gapToggleHandler = useMemo( | ||
| () => (fileHasSourceFetcher ? onToggleGap : undefined), | ||
| [fileHasSourceFetcher, onToggleGap], | ||
| ); | ||
| const expandedRows = useMemo( | ||
| () => | ||
| expandCollapsedRows(rows, { | ||
| layout, | ||
| buildDiffSectionRowPlan({ | ||
| expandedKeys: expandedGapKeys, | ||
| file, | ||
| highlightedDiff: resolvedHighlighted, | ||
| layout, | ||
| showHunkHeaders, | ||
| sourceLineSpans, | ||
| sourceStatus, | ||
| side: expansionSide, | ||
| theme, | ||
| visibleAgentNotes, | ||
| }), | ||
| [rows, layout, expandedGapKeys, sourceLineSpans, sourceStatus, expansionSide], | ||
| ); | ||
| const plannedRows = useMemo( | ||
| () => | ||
| file | ||
| ? buildReviewRenderPlan({ | ||
| fileId: file.id, | ||
| rows: expandedRows, | ||
| showHunkHeaders, | ||
| visibleAgentNotes, | ||
| }) | ||
| : [], | ||
| [file, expandedRows, showHunkHeaders, visibleAgentNotes], | ||
| [ | ||
| expandedGapKeys, | ||
| file, | ||
| layout, | ||
| resolvedHighlighted, | ||
| showHunkHeaders, | ||
| sourceLineSpans, | ||
| sourceStatus, | ||
| theme, | ||
| visibleAgentNotes, | ||
| ], | ||
| ); |
There was a problem hiding this comment.
visibleAgentNotes changes
The single consolidated useMemo now reruns buildBaseRows (which calls buildSplitRows/buildStackRows over every hunk) and expandCollapsedRows whenever visibleAgentNotes changes. In the old code those two stages were in a separate memo keyed on [file, layout, resolvedHighlighted, theme], so an agent-note visibility change only triggered the cheap buildReviewRenderPlan stage.
For large diffs the base-row build is non-trivial, and visibleAgentNotes can change during scroll. The refactoring loses that optimization by flattening all four memos into one.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/PierreDiffView.tsx
Line: 192-216
Comment:
**Unnecessary base-row rebuild when only `visibleAgentNotes` changes**
The single consolidated `useMemo` now reruns `buildBaseRows` (which calls `buildSplitRows`/`buildStackRows` over every hunk) and `expandCollapsedRows` whenever `visibleAgentNotes` changes. In the old code those two stages were in a separate memo keyed on `[file, layout, resolvedHighlighted, theme]`, so an agent-note visibility change only triggered the cheap `buildReviewRenderPlan` stage.
For large diffs the base-row build is non-trivial, and `visibleAgentNotes` can change during scroll. The refactoring loses that optimization by flattening all four memos into one.
How can I resolve this? If you propose a fix, please make it concise.| import type { PlannedReviewRow } from "./reviewRenderPlan"; | ||
| import { measureRenderedRowHeight } from "./renderRows"; | ||
|
|
||
| const EMPTY_EXPANDED_GAP_KEYS: ReadonlySet<string> = new Set(); |
There was a problem hiding this comment.
EMPTY_EXPANDED_GAP_KEYS sentinel
This constant is already defined in diffSectionRowPlan.ts (line 14) for the same purpose. Since measureDiffSectionGeometry passes the value down to buildDiffSectionRowPlan, the sentinel in this file could be replaced by importing (or relying on) the default in buildDiffSectionRowPlan's parameter list, keeping one source of truth.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/diffSectionGeometry.ts
Line: 17
Comment:
**Duplicated `EMPTY_EXPANDED_GAP_KEYS` sentinel**
This constant is already defined in `diffSectionRowPlan.ts` (line 14) for the same purpose. Since `measureDiffSectionGeometry` passes the value down to `buildDiffSectionRowPlan`, the sentinel in this file could be replaced by importing (or relying on) the default in `buildDiffSectionRowPlan`'s parameter list, keeping one source of truth.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
diffSectionRowPlanas the shared file-level row planning layer for diff sections.src/ui/diff/so row planning, geometry, and windowing live together.Testing
bun run typecheckbun test src/ui/diff/diffSectionGeometry.test.ts src/ui/diff/rowWindowing.test.ts src/ui/components/panes/copySelection.test.ts src/ui/AppHost.scroll-regression.test.tsxbun run lintbun run format:checkThis PR description was generated by Pi using OpenAI GPT-5 Codex