[FEAT] Virtualize Cell Rows with TanStack and React Callbacks #71
Conversation
- Remove opacity-based virtualization from CellOutputs - Replace <img> tags with text placeholders: ![Image: image/png] - Add CSS styling for image-placeholder class - Remove unused isVisible prop from CellOutputs component Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Add 'Courier New' as fallback font for better cross-platform support - Remove unrelated .error-output style to keep changes focused Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Extract ImagePlaceholder component with aria-label for screen readers - Add role="img" attribute for semantic HTML - Use helper function for consistent placeholder formatting Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Use <div> instead of <pre> for image placeholder to avoid semantic conflict with role="img" - Add white-space: pre-wrap and font-size to CSS for proper formatting - Use double quotes for font-family names containing spaces (CSS best practice) Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Convert MIME types to user-friendly format (e.g., 'PNG image output' instead of 'Image output of type image/png') - Add fallback for unknown image types Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Remove unreachable fallback case since only PNG and JPEG are supported - Simplify conditional to binary choice for clarity Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
- Document that only image/png and image/jpeg are currently supported - Makes the intentional constraint explicit for future maintainers Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
The previous fix removed the isVisible/opacity optimization entirely, causing all cells to render at full opacity. This made everything slow. Now: text placeholders (fixes flickering) + opacity optimization (fixes performance). Text placeholders have consistent dimensions, so opacity changes don't trigger ResizeObserver. Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
**1. `scrollTop` state is tracked but never used in JSX** — every scroll event calls `setScrollTop()`, which forces a full `ConflictResolver` re-render. This is pure waste. **2. No `React.memo` on `MergeRow`** — every state change in the parent (scroll, typing, ResizeObserver, drag) re-renders ALL `MergeRow` components. With 100+ rows, each keystroke triggers 100+ component renders. **3. No `React.memo` on `CellContent`** — even within a single MergeRow re-render, all 3 `CellContent` children fully re-render, including expensive diff computations and markdown rendering. **4. `getRefCallback(i)` creates a new closure on every render** — React sees a different `ref` function, triggers ResizeObserver unobserve/observe on every render for every row. **5. `DiffContent` runs `computeLineDiff` without `useMemo`** — expensive LCS computation runs on every render even when inputs haven't changed. **6. Inline drag handlers in MergeRow** — `onDragStart` closures are recreated each render, defeating any memoization on `CellContent`. The cascade on each keystroke: type → `setChoices` → ConflictResolver renders → ALL MergeRows render → ALL CellContent children render → ALL diffs recompute → ResizeObserver fires → `setRowHeights` → ANOTHER full render cycle.
[FIX] Perf improvements * **Refactor** * Optimized rendering performance for content cells and merge row operations through strategic memoization of expensive computations. * Improved callback efficiency by implementing caching strategies for frequently used event handlers. * Enhanced scroll performance in conflict resolution interface by streamlining position tracking mechanisms.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntegrates Changes
Sequence DiagramsequenceDiagram
participant ConflictResolver
participant useVirtualizer as useVirtualizer<br/>(`@tanstack/react-virtual`)
participant MergeRow
participant CellContent
participant ImagePlaceholder
ConflictResolver->>useVirtualizer: Initialize with rowSize & overscan
useVirtualizer-->>ConflictResolver: Return virtualRows (visible only)
ConflictResolver->>MergeRow: Render each virtualRow with<br/>drag-state primitives
Note over MergeRow: cellDragActive, cellDragSide,<br/>cellDragSourceRow, etc.
MergeRow->>CellContent: Render base/current/incoming<br/>cell content
CellContent->>CellContent: Check output type
alt Image Output
CellContent->>ImagePlaceholder: Render text placeholder
ImagePlaceholder-->>CellContent: Return placeholder HTML
else Non-Image Output
CellContent->>CellContent: Render markdown/diff/text
end
CellContent-->>MergeRow: Return rendered cell
MergeRow-->>ConflictResolver: Return positioned row element
ConflictResolver->>ConflictResolver: Apply absolute positioning<br/>& translateY transform
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/web/client/CellContent.tsx (1)
38-48:⚠️ Potential issue | 🔴 CriticalCritical:
useMemocalled after early return violates Rules of Hooks.The early return on line 38 means
useMemoon line 48 is conditionally skipped whencellis undefined. React requires all hooks to be called in the same order on every render. This is also flagged by the Biome linter.Move the hook above the early return (with a fallback for undefined
cell):Proposed fix
export function CellContentInner({ cell, cellIndex, side, isConflict = false, compareCell, baseCell, diffMode = 'base', showOutputs = true, onDragStart, onDragEnd, }: CellContentProps): React.ReactElement { + const encodedCell = useMemo(() => cell ? encodeURIComponent(JSON.stringify(cell)) : '', [cell]); + if (!cell) { return ( <div className="cell-placeholder"> <span className="placeholder-text">(not present)</span> </div> ); } const source = normalizeCellSource(cell.source); const cellType = cell.cell_type; - const encodedCell = useMemo(() => encodeURIComponent(JSON.stringify(cell)), [cell]);src/web/client/ConflictResolver.tsx (1)
265-287:⚠️ Potential issue | 🟠 MajorFix cell-columns grid to respect
showBaseColumnprop.The
.cell-columnsgrid always renders 3 columns (base/current/incoming) but thecolumn-labelsheader switches to a 2-column layout whenshowBaseColumnis false. This creates a visual misalignment where 2 header labels span over 3 content columns.Add the
two-columnCSS class conditionally to thecell-columnsdiv:<div className={`cell-columns${showBaseColumn ? '' : ' two-column'}`}>The CSS rule
.cell-columns.two-column { grid-template-columns: repeat(2, minmax(0, 1fr)); }already exists in styles.ts but is not being applied.
🤖 Fix all issues with AI agents
In `@src/web/client/CellContent.tsx`:
- Around line 108-109: The DiffContent component is missing the diffMode prop in
its parameter destructuring which causes the build error; update the DiffContent
signature to destructure diffMode from DiffContentProps (e.g., function
DiffContent({ source, compareSource, side, diffMode }: DiffContentProps)) and
also add diffMode to the useMemo dependency array (useMemo(() =>
computeLineDiff(compareSource, source, diffMode), [compareSource, source,
diffMode])) so the computeLineDiff call and memoization correctly receive and
react to diffMode changes.
🧹 Nitpick comments (1)
src/web/client/ConflictResolver.tsx (1)
84-86:buildMergeRowsFromSemanticruns on every render but its result is only used as initial state.
initialRowsis recomputed on each render even thoughuseState(line 91) and the history initializer (line 92) only consume it on mount. Wrap it inuseMemoor move it into the lazy state initializer to avoid unnecessary work during re-renders.Option: lazy useState initializer
- const initialRows = conflict.type === 'semantic' && conflict.semanticConflict - ? buildMergeRowsFromSemantic(conflict.semanticConflict) - : []; - const [choices, setChoices] = useState<Map<number, ResolutionState>>(new Map()); const [markAsResolved, setMarkAsResolved] = useState(INITIAL_MARK_AS_RESOLVED); const [renumberExecutionCounts, setRenumberExecutionCounts] = useState(INITIAL_RENUMBER_EXECUTION_COUNTS); - const [rows, setRows] = useState<MergeRowType[]>(initialRows); - const [history, setHistory] = useState<HistoryState>(() => ({ + const [rows, setRows] = useState<MergeRowType[]>(() => + conflict.type === 'semantic' && conflict.semanticConflict + ? buildMergeRowsFromSemantic(conflict.semanticConflict) + : [] + ); + const [history, setHistory] = useState<HistoryState>(() => { + const initRows = conflict.type === 'semantic' && conflict.semanticConflict + ? buildMergeRowsFromSemantic(conflict.semanticConflict) + : []; + return { entries: [{ label: 'Initial state', snapshot: { choices: cloneChoices(new Map()), - rows: cloneRows(initialRows), + rows: cloneRows(initRows), markAsResolved: INITIAL_MARK_AS_RESOLVED, renumberExecutionCounts: INITIAL_RENUMBER_EXECUTION_COUNTS, }, }], index: 0, - })); + }; + });Note: this calls
buildMergeRowsFromSemantictwice on mount. If you want to avoid that, compute it once in a ref or shared initializer.
src/web/client/CellContent.tsx
Outdated
| function DiffContent({ source, compareSource, side }: DiffContentProps): React.ReactElement { | ||
| const diff = useMemo(() => computeLineDiff(compareSource, source), [compareSource, source]); |
There was a problem hiding this comment.
Critical: diffMode not destructured — causes build failure.
diffMode is declared in DiffContentProps but omitted from the destructuring on line 108. This causes the pipeline error Cannot find name 'diffMode' at line 121 where it's referenced.
Proposed fix
-function DiffContent({ source, compareSource, side }: DiffContentProps): React.ReactElement {
+function DiffContent({ source, compareSource, side, diffMode }: DiffContentProps): React.ReactElement {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function DiffContent({ source, compareSource, side }: DiffContentProps): React.ReactElement { | |
| const diff = useMemo(() => computeLineDiff(compareSource, source), [compareSource, source]); | |
| function DiffContent({ source, compareSource, side, diffMode }: DiffContentProps): React.ReactElement { | |
| const diff = useMemo(() => computeLineDiff(compareSource, source), [compareSource, source]); |
🤖 Prompt for AI Agents
In `@src/web/client/CellContent.tsx` around lines 108 - 109, The DiffContent
component is missing the diffMode prop in its parameter destructuring which
causes the build error; update the DiffContent signature to destructure diffMode
from DiffContentProps (e.g., function DiffContent({ source, compareSource, side,
diffMode }: DiffContentProps)) and also add diffMode to the useMemo dependency
array (useMemo(() => computeLineDiff(compareSource, source, diffMode),
[compareSource, source, diffMode])) so the computeLineDiff call and memoization
correctly receive and react to diffMode changes.
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes
Style