Repository navigation
Replace iterateOverViewportColumnsForRow with mapViewportColumns, and fix cells hidden by colSpan - #4225
Merged
Conversation
nstepien
marked this pull request as ready for review
October 23, 2026 03:31
… by index The viewport helpers build arrays with plain loops: iterating over generator objects is much slower, and React Compiler doesn't support `yield`. `iterateOverViewportColumnsForRow` is part of `RenderRowProps`, so it still returns an `IteratorObject`, the array's `.values()`. Cells covered by `colSpan` were skipped by their position in the list of viewport columns, so a cell spanning columns that aren't rendered, like an active cell outside of the overscan range or the last cell of that range, hid the next rendered columns. They're now skipped by index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
colSpan
…` callback `RenderRowProps.iterateOverViewportColumnsForRow` returned an iterator of `[column, isCellActive, colSpan]` tuples. It's replaced by `mapViewportColumns(activeIdx, colSpanArgs, callback)`, which calls `callback(column, isCellActive, colSpan, index)` for each cell and returns the results, so no tuple, iterator, or iterator result is allocated per cell. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
colSpaniterateOverViewportColumnsForRow with mapViewportColumns, and fix cells hidden by colSpan
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4225 +/- ##
==========================================
+ Coverage 98.56% 98.58% +0.01%
==========================================
Files 45 46 +1
Lines 1957 1984 +27
Branches 728 750 +22
==========================================
+ Hits 1929 1956 +27
Misses 28 28
🚀 New features to boost your workflow:
|
`getViewportColSpanArgs` and `getViewportRowIndexes` each had one caller. `getViewportRows` now pushes the rendered rows directly, without an array of row indexes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nstepien
added a commit
that referenced
this pull request
Oct 23, 2026
…test header and summary rows (#4226) Follow-ups to #4225 from a review of it. ### Docs The `mapViewportColumns` JSDoc, mirrored in the README, said the active cell is always rendered. Two cases weren't covered: - When virtualization would leave out the active row, `DataGrid` still renders it, but its `mapViewportColumns` only maps the active cell, or nothing when the whole row is active. - A cell's `colSpan` can cover the active column, which is then skipped like any other spanned column. ### Fewer allocations in the `startIdx` memo The memo built the `colSpan` args of every rendered row whenever `colOverscanStartIdx > 0` and any column had `colSpan`, even when no column could span into the overscan range, e.g., when every `colSpan` column is frozen or after the range. It now looks for candidate columns first, and only builds the args when there are some. ### Tests The regression tests for cells hidden by `colSpan` only covered data rows, but the bug also affected header and summary rows. The three tests now run on all three row types. All 9 fail on the parent of #4225 and pass now. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
nstepien
added a commit
that referenced
this pull request
Oct 25, 2026
`vp pack` compiles `src/` with React Compiler, which skips every component or hook it can't compile. On `main`, it skips `DataGrid`, `EditCell`, `GroupedRow`, `useCalculatedColumns`, `useColumnWidths`, and `useViewportRows`. After this PR, all of them are compiled, and if a bail-out comes back, oxlint reports it and `vp pack` fails in CI. ### Unsupported syntax - **`useColumnWidths`**: replace `||=` with an `if` statement. `hasChanges = hasChanges || …` would trip `eslint/logical-assignment-operators`. - **`useCalculatedColumns`** / **`useViewportRows`**: replace `x++` on variables captured by nested functions with `x += 1`. - Enable `react/todo`, which reports both. It was turned off in #4155, before React Compiler was set up, while the generators couldn't be compiled either. #4225 removed them. ### Immutability - **`GroupedRow`**: `let idx` was incremented in the `mapViewportColumns` callback when the first rendered column was the select column, and `GroupCell` compared it with `column.idx`. The group-by columns follow the select column in `groupBy` order, so `GroupedRow` now compares each column's key with `groupBy[row.level]` and passes the result to `GroupCell` as `isLevelMatching`. ### Hoisted functions - **`DataGrid`**: the `useImperativeHandle` callback read `setPosition`, which is declared further down. The handle now exposes `setPositionLatest`, which also keeps its `setActivePosition` stable across renders, and the `react/immutability` suppression is gone. - **`EditCell`**: the `useLayoutEffect` cleanup called `cancelTask`, which was declared after the effect. It's now declared before the effect, and the cleanup calls it through `useEffectEvent`, like `commitOnOutsideMouseDown`. A plain call would make it an effect dependency, and the effect must not clean up between re-renders, as the comment there explains. `onClose` also moves above the `useEffectEvent` that calls it. The `TODO: report React Compiler bug` note and its `react/invariant` suppression are gone. ### Build - `compiler: { reportDiagnostics: true }` turns the code React Compiler skips into plugin warnings. `vp pack` already fails on warnings in CI and agent sessions (`failOnWarn: isAgent || 'ci-only'`), and `vp dev`, `vp build`, and `vp test` log them. Unlike lint, a suppression comment doesn't hide them. `panicThreshold: 'all_errors'` would fail every build, including the dev server and tests while code is being edited, and `'critical_errors'` misses unsupported syntax like `x++`. - One `react()` instance now serves both Vite and `vp pack`. It produces identical `lib/` and website builds, and the same dev-server modules. `vp pack` can't use `lazyPlugins`, so its plugins were already created on every config load. The plugin keeps some per-run state, like whether to emit development JSX, which is fine because Vite and `vp pack` never run in the same process. - `.d.ts` files stay excluded: `vp pack` passes the declaration files it generates through the plugin, and without the exclusion, `lib/index.d.ts` becomes `export {}`, which publint doesn't catch. `node_modules` stays excluded too, as setting `exclude` replaces the plugin's default. - AGENTS.md mentions that `vp pack` fails on code React Compiler can't compile. ### Performance The differences are small, and only a few hold in at least 4 of the 5 rounds: - Compared to `main`, this PR makes typing in an editor about 10% faster (4 of 5 rounds) and collapsing or expanding a group about 8% faster (5 of 5). `EditCell` and `GroupedRow` are among the components it now compiles. Moving the active cell is about 9% slower (5 of 5). - Without React Compiler, mounting `DataGrid` is about 7% faster (5 of 5) and re-rendering it with the same props about 11% faster (4 of 5). Mounting `TreeDataGrid` is about 8% slower (5 of 5), and typing in an editor about 11% slower (4 of 5). - The other differences go both ways between rounds. | Update (ms) | `main` | This PR | This PR, no compiler | | ------------------------------------------ | ------------------ | ------------------ | -------------------- | | `DataGrid`: mount | 2.26 (2.16–2.32) | 2.26 (2.16–2.34) | 2.09 (1.96–2.18) | | `DataGrid`: parent re-render, same props | 0.102 (0.095–0.110) | 0.103 (0.094–0.118) | 0.091 (0.084–0.101) | | `DataGrid`: toggle row selection | 0.216 (0.191–0.251) | 0.224 (0.203–0.235) | 0.222 (0.206–0.242) | | `DataGrid`: move the active cell | 0.325 (0.314–0.344) | 0.354 (0.334–0.376) | 0.346 (0.307–0.405) | | `DataGrid`: type in an editor | 0.223 (0.209–0.236) | 0.201 (0.182–0.218) | 0.223 (0.208–0.243) | | `DataGrid`: scroll down by 4px | 0.171 (0.161–0.182) | 0.160 (0.146–0.176) | 0.170 (0.143–0.192) | | `DataGrid`: scroll down by one row | 0.329 (0.301–0.362) | 0.327 (0.283–0.414) | 0.319 (0.290–0.364) | | `DataGrid`: scroll right by 4px | 0.149 (0.140–0.162) | 0.144 (0.127–0.178) | 0.144 (0.124–0.159) | | `DataGrid`: scroll right by one column | 0.752 (0.672–0.821) | 0.700 (0.601–0.798) | 0.686 (0.623–0.725) | | `TreeDataGrid`: mount | 3.62 (3.39–3.74) | 3.46 (3.26–3.75) | 3.74 (3.45–4.03) | | `TreeDataGrid`: scroll down by one row | 0.367 (0.314–0.411) | 0.337 (0.304–0.387) | 0.345 (0.286–0.403) | | `TreeDataGrid`: collapse or expand a group | 2.40 (2.29–2.52) | 2.21 (1.91–2.39) | 2.17 (1.79–2.47) | - The grid has 10,000 rows and 41 columns: `SelectColumn`, then 40 columns with `renderTextEditor`, the first of which is frozen. `TreeDataGrid` also has two columns that group the rows, with all 110 groups expanded. The app around the grid isn't compiled, so each of its re-renders passes a new `style` object. - Chromium, React's production build (`NODE_ENV=production`, `react-dom/client`). - Each value is the mean of 5 runs, alternating between the three builds, and the range of the 5 run means is in parentheses. Each run times 200 updates, one per frame, and 10 mounts. - Each update is timed with `performance.now()` around a `flushSync` call. That covers React's render and commit, including layout effects. It doesn't cover the browser's layout and paint after the commit, and the scroll scenarios lay out before starting the timer. Chromium rounds `performance.now()` to 0.1 ms, hence the means. - Without React Compiler, the grid already memoizes by hand (`memo`, `useMemo`, `useLatestFunc`). React Compiler adds a memo cache to each component, which a mount allocates and fills. In `DataGrid`, most memoized values still change on every render, so its cache mostly adds comparisons. #4236 fixes that. `vp run ci` passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Follow-up to #4222.
Breaking change
RenderRowProps.iterateOverViewportColumnsForRow, which returned an iterator of[column, isCellActive, colSpan]tuples, is replaced bymapViewportColumns, which calls a callback for each cell and returns the results:This only affects custom row renderers that render their own cells. Row renderers that render
<Row {...props} />keep working.Why
useViewportColumnscreated its generators withuseCallback(function* …), andDataGriddeclaredfunction* iterateOverViewportRowIdx()in its body.DataGridrender foriterateOverViewportRowIdx, and whenever the column range changed for the column generators, the.map(),.toArray(), andfor…ofcalls on its objects saw a new hidden class. Onmain, in Chromium,getViewportRows'.map()missed V8's megamorphic cache 473 times in the CommonFeatures vertical scroll, once per render, three sites missed it 228–237 times each in the MillionCells horizontal scroll, and library code deoptimized 4 times withwrong map.yield(Handle YieldExpression expressions), so it skippeduseViewportColumns, with 11 bail-outs, andDataGrid, with 3.The first commit moved the generators to module scope. The next commits replace them with plain loops, and the iterator with a callback, so building a row's cells allocates nothing per cell besides what the callback returns. The two helpers used in more than one place are in
src/utils/viewportUtils.ts, and the others are inlined.Results
Building a row's cells, with 30 columns and a callback that creates
Row's props for each cell, takes per row about 1.0 µs with module-level generators, 0.67 µs with arrays returned as.values(), and 0.16 µs with the callback. In cold code, before V8 optimizes it, the callback takes about 2.5 µs per row, against 6.6 µs with.values().Sampled allocations in Chromium, on an unminified build of the demo site:
The library's CPU time in the MillionCells horizontal scroll dropped from about 86 ms to 72 ms, as medians of 3 alternating runs against the
.values()version. In the CommonFeatures vertical scroll, the difference was within the noise between runs: the library's JavaScript is only a few percent of busy time.No generators are left, no iterator call site goes megamorphic, and the library's remaining deopts are one-time warm-up ones.
useViewportColumnsis now compiled by React Compiler, and its tworeact/memo-dependenciessuppressions are gone.DataGridstill isn't compiled: its remaining bail-out issetPositionbeing read before its declaration is initialized, inuseLatestFunc(setPosition).AGENTS.mdnow says to avoid generators in rendering code.Fix: cells hidden by
colSpanAfter a cell with
colSpanN, the generator skipped the next N − 1 columns in its list, but the list has gaps: it only contains the rendered columns. So a cell spanning columns that aren't rendered hid the next rendered columns:The list is ordered by index, so spanned columns are now skipped by index. The three new tests failed in Chromium before the fix.
Verification
vp run cipasses. A new test renders a row's cells through a custom row renderer withmapViewportColumns.🤖 Generated with Claude Code