Repository navigation
Give calculated columns a single shape to avoid megamorphic property reads - #4222
Merged
Merged
Conversation
…reads
`useCalculatedColumns` spread each raw column into its calculated column,
so the hidden class of each column depended on which optional properties
were set, and in what order. Past 4 shapes, the column property reads that
run for every rendered cell go megamorphic.
Calculated columns now set every property in a fixed order, and copy
custom properties after the known ones. `setPosition` builds the active
position from `idx` and `rowIdx` instead of spreading its input, and all
position literals use the `{ idx, rowIdx }` order.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4222 +/- ##
=======================================
Coverage 98.56% 98.56%
=======================================
Files 45 45
Lines 1956 1957 +1
Branches 728 728
=======================================
+ Hits 1928 1929 +1
Misses 28 28
🚀 New features to boost your workflow:
|
nstepien
added a commit
that referenced
this pull request
Oct 22, 2026
Found while working on #4222. This fixes two bugs with PageUp and PageDown on header and summary rows. ### Crash without rows In a grid with no rows and a `rowHeight` function, PageUp and PageDown on header and summary rows could throw: ``` TypeError: Cannot read properties of undefined (reading 'top') ``` With a `rowHeight` function, `useViewportRows` reads row positions from its `rowPositions` array. `getRowTop` and `getRowHeight` clamp the row index to the first or last row, so header and summary rows use the position of the closest row. With no rows, the clamp returned index 0 of an empty array, and reading `.top` or `.height` from `undefined` threw. A number `rowHeight` computes positions from the index, so it wasn't affected. `getRowTop` and `getRowHeight` now return an empty position at the top (`top: 0`, `height: 0`) when there are no rows, so PageUp and PageDown move the active cell like they do with a number `rowHeight`. ### PageUp moving down On a top summary row, or on the main header row when column group header rows are above it, PageUp moved the active cell down to the first row. PageUp aimed above the first row, and `nextRowY > 0 ? findRowIdx(nextRowY) : 0` turned that into row 0. PageUp only did nothing on the first header row. PageUp now does nothing on any row above the rows (`rowIdx < 0`), like PageDown already does on rows below them (`rowIdx >= rows.length`). ### Verification - Each new test failed in Chromium before its fix: the crash test with the `TypeError` reported as an unhandled error, and the PageUp tests with the active cell moved to the first row. - With the PageUp fix but without the `useViewportRows` fix, the crash test still fails, since PageDown on the header row and PageUp on a bottom summary row still reach `getRowTop`. - The crash test also passes with `rowHeight: 35`, so both kinds of `rowHeight` navigate the same way. - `vp run ci` passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Columns don't support properties outside of `Column`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Calculated columns set every property, so the type now says so, and leaving a property out of the object literal in `useCalculatedColumns` is a type error, including properties added to `Column` later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nstepien
marked this pull request as ready for review
October 22, 2026 23:20
Apps type-check the published declarations with their own compiler options. Without `exactOptionalPropertyTypes`, which is off by default, `Required<Column>` removed `undefined` from the optional properties, so `renderEditCell` was typed as a function or `null` while it's `undefined` when not set. Redeclaring them as required with `Column`'s own property types keeps `undefined` with either setting. Co-Authored-By: Claude Opus 5.5 <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.
Applies What's up with monomorphism? and Elements kinds in V8 to the grid. Only the first one called for changes.
Cause
useCalculatedColumnscreated each calculated column with{ ...rawColumn, … }, so the column's hidden class (shape) depended on which optional properties the raw column set, and in what order. With the columns of the CommonFeatures demo, the 16 calculated columns had 9 different shapes. V8's inline caches handle up to 4 shapes per property access, so thecolumn.*reads that run for every rendered cell, inCell,getColSpan,getCellStyle,getCellClassname,isCellEditableUtil, and others, went megamorphic.Changes
undefinedwhen unset. To match,CalculatedColumnredeclares the optionalColumnproperties as required, withColumn's own property types, likeColumn<TRow, TSummaryRow>['cellClass'], so leaving one out is a type error. UnlikeRequired<Column>, this keepsundefinedin their types whether or not apps enableexactOptionalPropertyTypes.setPositionbuilds the active position fromidxandrowIdxinstead of spreading the position it receives, and the five position literals written as{ rowIdx, idx }now use{ idx, rowIdx }like the others.AGENTS.mddocuments the convention.Measurements
Measured on the packed
lib/with Node 26 (V8 14.6), using the CommonFeatures columns andenableVirtualization={false}:%HaveSameMap)--log-ic)renderToStringof 300 rows × 16 columns, medianThe render time improves by 2.7%, across 8 alternating pairs of processes. React's own work dominates rendering, and DOM work in the browser dilutes the gain further, so this is a small, steady improvement rather than a big one.
Reads of the raw columns stay megamorphic, since users create them, but they only run in
useMemowhencolumnschanges.Behavior and type changes
Columnproperty as an own property, soObject.keys(column)and'cellClass' in columnreturn different results than before.Columnare no longer copied to calculated columns, as they aren't supported.Columnproperty is required onCalculatedColumn, so code that createsCalculatedColumnobjects itself, like test mocks, has to set all of them.Elements kinds
No changes needed. The array patterns the grid uses all create packed arrays on V8 14.6, and no hot loop reads past the end of an array.
Auditing the out-of-bounds reads found a crash, which #4223 fixes.
Verification
vp run cipasses.🤖 Generated with Claude Code