perf(#230): remove O(N^2) per-cell row-index lookup in the data grid#235
Open
husamql3 wants to merge 1 commit into
Open
perf(#230): remove O(N^2) per-cell row-index lookup in the data grid#235husamql3 wants to merge 1 commit into
husamql3 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe row index computation in ChangesRow Index Memoization
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bundle Size
File breakdown
File breakdown |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/features/tables/components/table-cell.tsx`:
- Around line 22-30: The display-index lookup in TableCell is still happening
per cell via useMemo, so every row-model update re-runs rows.findIndex for each
visible cell and keeps rendering quadratic. Move this row-position calculation
out of TableCell into a shared parent or row renderer that already iterates the
table rows, and pass the computed display index down to TableCell using the
existing row/cell rendering path so it is computed once per row-model change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e39cf95-f553-494f-a503-ba3c714c8cdf
📒 Files selected for processing (1)
packages/web/src/features/tables/components/table-cell.tsx
Comment on lines
22
to
+30
|
|
||
| // Display position within the current (possibly sorted/filtered) row model. | ||
| // Memoized on the row-model identity + this row's data so the O(N) scan runs | ||
| // at most once per render pass per row model rather than on every cell render. | ||
| const rows = table.getRowModel().rows; | ||
| const displayRowIndex = rows.findIndex((row) => row.original === cell.row.original); | ||
| const rowIndex = displayRowIndex >= 0 ? displayRowIndex : originalRowIndex; | ||
| const rowIndex = useMemo(() => { | ||
| const displayRowIndex = rows.findIndex((row) => row.original === cell.row.original); | ||
| return displayRowIndex >= 0 ? displayRowIndex : cell.row.index; | ||
| }, [rows, cell.row.original, cell.row.index]); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first, then inspect the smallest useful slices.
ast-grep outline packages/web/src/features/tables/components/table-cell.tsx --view expanded || true
wc -l packages/web/src/features/tables/components/table-cell.tsx
echo
echo "---- table-cell.tsx (selected lines) ----"
sed -n '1,220p' packages/web/src/features/tables/components/table-cell.tsx
echo
echo "---- search for row rendering / table cell usage ----"
rg -n "TableCell|table.getRowModel\(\)\.rows|findIndex\(|row.original|row.index" packages/web/src/features/tables -SRepository: husamql3/db-studio
Length of output: 8556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- table-body-row.tsx ----"
wc -l packages/web/src/features/tables/components/table-body-row.tsx
sed -n '1,220p' packages/web/src/features/tables/components/table-body-row.tsx
echo
echo "---- table-body.tsx ----"
wc -l packages/web/src/features/tables/components/table-body.tsx
sed -n '1,220p' packages/web/src/features/tables/components/table-body.tsx
echo
echo "---- use-table-model.ts (relevant slice) ----"
wc -l packages/web/src/features/tables/hooks/use-table-model.ts
sed -n '1,220p' packages/web/src/features/tables/hooks/use-table-model.tsRepository: husamql3/db-studio
Length of output: 8541
Hoist the display-index lookup out of TableCell. useMemo is still per cell, so every row-model change re-runs rows.findIndex(...) in every visible cell. That keeps the render pass at O(rows² × cols). Compute the index once per row-model change in a shared parent, or pass the row position down from the row renderer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/web/src/features/tables/components/table-cell.tsx` around lines 22 -
30, The display-index lookup in TableCell is still happening per cell via
useMemo, so every row-model update re-runs rows.findIndex for each visible cell
and keeps rendering quadratic. Move this row-position calculation out of
TableCell into a shared parent or row renderer that already iterates the table
rows, and pass the computed display index down to TableCell using the existing
row/cell rendering path so it is computed once per row-model change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TableCell— the component ranrows.findIndex(...)(a linear scan of the whole row model) on every cell render, so a full grid render was O(rows² × cols) and re-ran on every focus/edit/selection toggle. The scan is now wrapped inuseMemokeyed on the row-model identity and this row's data, so it recomputes only when an input that could change the result changes.rowIndexstill resolves to the same value (display position within the current, possibly sorted, row model), so focus/edit/selection targeting is identical; only the redundant per-render recomputation is removed.Testing
bun run typecheck→ passesbun run format→ passesbun run build→ passesCloses #230
Glossary
getRowModel().rows), which can differ from pre-sort data order.useMemoSummary by CodeRabbit