Spreadsheet example#6457
Conversation
📝 WalkthroughWalkthroughAdds an experimental React spreadsheet example with deterministic data, virtualized rendering, editing, selection, clipboard and fill operations, sorting/filtering, frozen panes, multiple sheets, zoom controls, styling, Vite configuration, and Playwright end-to-end tests. ChangesReact Spreadsheet Example
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
View your CI Pipeline Execution ↗ for commit c9c24ca
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
examples/react/spreadsheet/src/useSpreadsheetHistory.ts (2)
113-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUndo replays patches in forward order.
If a single command ever contains two patches for the same cell, applying
beforein forward order restores the first patch'sbeforelast — but the correct pre-command value is the first patch'sbefore, which happens to work here only because current producers (patchesForBoundsdedupes by cell key,buildFillPatchesemits one patch per cell) never duplicate. Iterating in reverse for'before'makes this robust against future command builders.🤖 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 `@examples/react/spreadsheet/src/useSpreadsheetHistory.ts` around lines 113 - 141, Update applyPatches to iterate patches in reverse order when value is 'before', while preserving forward order for 'after'. Keep the existing row and cell cloning logic unchanged so undo restores the earliest pre-command value when multiple patches target the same cell.
26-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
rowIndexByIdis rebuilt on every dispatch, includingreset.With the 10k-row stress dataset this allocates a 10k-entry Map for every keystroke commit, undo, or redo — even for
reset, which never uses it. Build it lazily inside the branches that need it, or derive it once fromstate.rowsvia a memo keyed on the rows array.♻️ Proposed change
- const rowIndexById = new Map( - state.rows.map((row, index) => [row.id, index]), - ) - if (action.type === 'reset') { return { rows: action.rows, past: [], future: [] } } + + const rowIndexById = new Map( + state.rows.map((row, index) => [row.id, index]), + )🤖 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 `@examples/react/spreadsheet/src/useSpreadsheetHistory.ts` around lines 26 - 34, Update the reducer callback around rowIndexById so the Map is not constructed before handling every action, especially reset. Create it lazily only within the action branches that read row indices, while preserving the existing reset result and behavior for other actions.examples/react/spreadsheet/src/spreadsheetModel.ts (2)
399-418: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sourceValuesis recomputed for every destination cell.For a vertical fill the source column values depend only on
destinationColumnId; for a horizontal fill they depend only onrowId. Recomputing inside the inner loop makes the fill O(destCells × sourceLen)getValuecalls — noticeable when dragging a fill over thousands of rows in the stress grid. Hoist/memoize per column (vertical) or per row (horizontal).♻️ Sketch: cache source values per axis key
+ const sourceValuesCache = new Map<string, Array<CellValue>>() + const getSourceValues = (key: string, compute: () => Array<CellValue>) => { + let cached = sourceValuesCache.get(key) + if (!cached) { + cached = compute() + sourceValuesCache.set(key, cached) + } + return cached + }Then key on
destinationColumnIdwhenvertical, otherwise onrowId.🤖 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 `@examples/react/spreadsheet/src/spreadsheetModel.ts` around lines 399 - 418, Cache the sourceValues computation in the fill logic instead of recomputing it for every destination cell. In the vertical path, memoize by destinationColumnId; in the horizontal path, memoize by rowId, while preserving the existing range and getValue behavior. Reuse the cached values when calling getFilledValue for each destination cell.
227-237: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFormula-injection guard makes internal copy/paste lossy.
escapeTsvValueprefixes'for strings starting with= + @ -, so copying such a cell and pasting back into the grid persists the apostrophe. SinceparseTsv/parseInputValuenever strip it, repeated copy/paste cycles accumulate prefixes. Consider stripping a single leading'on paste, or only applying the guard on export rather than in-grid clipboard payloads.🤖 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 `@examples/react/spreadsheet/src/spreadsheetModel.ts` around lines 227 - 237, Adjust the escapeTsvValue copy/paste flow so formula-injection apostrophes are applied only to exported TSV, not internal clipboard payloads; preserve the guard for external export and ensure parseTsv/parseInputValue round-trips values without accumulating prefixes.examples/react/spreadsheet/src/useGridInteractions.ts (1)
560-586: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLarge selections report
sum: 0indistinguishably from a real zero sum.The 10k cutoff is a sensible guard, but returning zeroed stats makes the status bar claim "Sum 0 / Avg 0" for a huge numeric selection. Return
null(or atruncated: trueflag) so the UI can render "—" instead.🤖 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 `@examples/react/spreadsheet/src/useGridInteractions.ts` around lines 560 - 586, The large-selection branch in getSelectionSummary should indicate that numeric statistics were not calculated instead of returning zero values that look like real results. Return null for the unavailable stats, or add a truncated indicator while preserving the exact count, and update the consuming status-bar rendering to display “—” for truncated summaries.examples/react/spreadsheet/vite.config.js (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReact Compiler devDependencies appear unwired.
package.jsonlistsbabel-plugin-react-compilerand@rolldown/plugin-babelas devDependencies, butreact()here is called with no options, so neither is actually enabled. If React Compiler support was intended for this example, wire it throughreact({ babel: { plugins: [['babel-plugin-react-compiler', {}]] } }); otherwise the dependencies are dead weight.🤖 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 `@examples/react/spreadsheet/vite.config.js` at line 18, Update the Vite React plugin invocation in the configuration to enable React Compiler by passing the Babel options with the babel-plugin-react-compiler plugin, matching the declared devDependencies. Use the existing react() call as the only change target and preserve the rest of the Vite configuration.examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the sheet’s row seed explicit in the e2e selectors.
Row IDs are built as
row-${seed}-${rowIndex}, so"row-7"bakes a specificseed: 7assumption into everycell()lookup. Extract it as a named constant with a comment, or derive it from a stable app-level source, so changes to the seed cannot flip the whole suite’s row locators silently.🤖 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 `@examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts` around lines 34 - 38, Update the cell() selector to avoid hardcoding the row seed in the row ID. Define a named constant for the sheet row seed with a clarifying comment, or derive it from a stable application-level source, and use it when constructing the row-${seed}-${rowIndex} selector so seed changes are explicit.examples/react/spreadsheet/src/SpreadsheetGrid.tsx (3)
574-587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winColumn selection is mouse-only.
The
columnheaderis a plaindivwithonClickand notabIndex/key handling, so keyboard users can select rows (row headers are real<button>s, Line 726) but never a whole column. AddtabIndex={0}plus Enter/Space handling, or nest a button as done for the menu trigger.🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 574 - 587, Update the column header element in the SpreadsheetGrid render flow to be keyboard accessible: add tabIndex={0} and handle Enter and Space key events by invoking interactions.selectColumn with the column id and event, while preserving the existing click behavior and columnheader semantics.
990-1012: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAuto-fit scans every row synchronously.
With the stress dataset this is a full
measureTextpass over the entire column on a single double-click, blocking the main thread. Consider capping the scan (e.g. first N rows plus the current viewport) or measuring the widest string by length first and only callingmeasureTexton candidates.Also note
measureTextWidthhardcodes11px Arial(Line 986) while the app font stack isAptos, 'Segoe UI', Arial— measured widths won't match rendered widths.🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 990 - 1012, Update getAutoFitColumnWidth to avoid measuring every row synchronously: cap candidates to a bounded sample such as the first N rows plus the current viewport, or prefilter by string length before calling measureTextWidth. Also update measureTextWidth to use the spreadsheet’s actual font stack instead of hardcoded 11px Arial so calculated widths match rendered text.
694-706: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the leaf-column count out of the
bounds.somecallback.Three arrays are spread and concatenated for every bound, for every rendered row, on every selection change. Compute the count once.
⚡ Proposed refactor
+ const totalColumnCount = + table.getStartVisibleLeafColumns().length + + table.getCenterVisibleLeafColumns().length + + table.getEndVisibleLeafColumns().length const fullySelected = bounds.some( (bound) => bound.minColumnIndex === 0 && - bound.maxColumnIndex === - [ - ...table.getStartVisibleLeafColumns(), - ...table.getCenterVisibleLeafColumns(), - ...table.getEndVisibleLeafColumns(), - ].length - - 1 && + bound.maxColumnIndex === totalColumnCount - 1 && rowIndex >= bound.minRowIndex && rowIndex <= bound.maxRowIndex, )🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 694 - 706, In the selection logic surrounding fullySelected, compute the total visible leaf-column count once before calling bounds.some by combining the start, center, and end visible leaf columns. Replace the callback’s repeated array construction and length calculation with that hoisted value, preserving the existing boundary comparison.examples/react/spreadsheet/src/ColumnMenu.tsx (1)
85-93: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFilter input re-runs the row model on every keystroke.
column.setFilterValuefires per character, re-evaluating the filtered + sorted row models over the whole dataset — noticeably janky on the stress dataset the example ships with. Keep the input in local state and debounce the commit (~150-250ms).🤖 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 `@examples/react/spreadsheet/src/ColumnMenu.tsx` around lines 85 - 93, Update the filter input in ColumnMenu to use local state for immediate typing, then debounce calls to column.setFilterValue by approximately 150–250ms. Keep the displayed value responsive, commit the latest value after the debounce, and clean up or reset the pending timer when the component or filter changes.examples/react/spreadsheet/src/CellContextMenu.tsx (1)
49-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
role="menu"without focus management or arrow-key navigation.Nothing receives focus when the menu opens, and the
menuitems are only reachable by tabbing from wherever focus currently is. Escape closes it (good), but keyboard users can't practically operate it. Focus the first item on mount and handle ArrowUp/ArrowDown, or drop the menu ARIA roles and treat it as a plain button group.Note also the
218/286offsets duplicate the.cell-context-menuwidth declared inindex.css(Line 654) — measuring the rendered menu would avoid the two drifting apart.🤖 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 `@examples/react/spreadsheet/src/CellContextMenu.tsx` around lines 49 - 59, The CellContextMenu menu declares role="menu" without usable keyboard focus management. Update the CellContextMenu focus and keyboard handling to focus the first menuitem when opened and support ArrowUp/ArrowDown navigation while preserving Escape closing; alternatively remove the menu/menuitem ARIA roles and use plain button-group semantics. Also replace the hard-coded 218/286 positioning offsets with measurements from the rendered menu element rather than duplicating CSS dimensions.examples/react/spreadsheet/src/Spreadsheet.tsx (1)
300-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRibbon
tablistsemantics are incomplete.The
<span className="file-tab">File</span>is a non-tabchild ofrole="tablist", and the tabs don't reference the panel they control (the ribbon isrole="toolbar", nottabpanel). Screen readers will announce "tab 1 of 3" with no associated panel.♿ Suggested wiring
- <div className="ribbon-tab-row" role="tablist" aria-label="Ribbon"> - <span className="file-tab">File</span> + <div className="ribbon-tab-row"> + <span className="file-tab">File</span> + <div role="tablist" aria-label="Ribbon" style={{ display: 'contents' }}> {(['home', 'data', 'view'] as const).map((tab) => ( <button key={tab} type="button" role="tab" + id={`ribbon-tab-${tab}`} + aria-controls="ribbon-panel" aria-selected={ribbonTab === tab} className={ribbonTab === tab ? 'ribbon-tab-active' : undefined} onClick={() => setRibbonTab(tab)} > {tab[0].toUpperCase() + tab.slice(1)} </button> ))} + </div> </div> <div + id="ribbon-panel" + role="tabpanel" + aria-labelledby={`ribbon-tab-${ribbonTab}`} className="spreadsheet-ribbon" - role="toolbar" aria-label={`${ribbonTab} tools`} >🤖 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 `@examples/react/spreadsheet/src/Spreadsheet.tsx` around lines 300 - 320, Update the ribbon tab markup around the `ribbon-tab-row` and `spreadsheet-ribbon` elements so the `role="tablist"` contains only tab elements, moving the non-tab `file-tab` outside that container or giving it an appropriate surrounding structure. Add stable `id`/`aria-controls` wiring from each mapped tab to the corresponding ribbon content, and expose the ribbon content as the associated `tabpanel` while preserving the existing selected-tab state and rendering behavior.
🤖 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 `@examples/react/spreadsheet/src/index.css`:
- Around line 524-528: Insert a blank line after the --edge-left custom property
and before the display declaration in the affected CSS rule to satisfy
Stylelint’s declaration-empty-line-before requirement.
In `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx`:
- Around line 366-374: Update resolveCoordinate to account for the canvas CSS
zoom factor when converting client coordinates into virtualizer offsets: divide
the local X/Y coordinates and any comparison constants by zoom / 100 before
resolving cells. Preserve correct coordinate mapping at 100% zoom and ensure
marquee selection and fill-drag remain accurate at other zoom levels.
- Around line 414-418: Guard all virtualized render lookups against stale
indices: in SpreadsheetGrid.tsx at lines 414-418, return null when the row
lookup after virtualRow.index is missing; at lines 507-512, return null when the
header lookup after virtualColumn.index is missing; and at lines 749-754, return
null when the cell lookup after virtualColumn.index is missing. Keep valid-item
rendering unchanged.
- Around line 316-322: Update the cleanup returned by the effect in
SpreadsheetGrid to set scrollFrameRef.current to null after canceling its
animation frame. Preserve the existing cancellation and event-listener removal,
ensuring ensureEdgeScroll can schedule a new frame after the effect re-runs
during a drag.
- Around line 236-240: Guard the row and column lookups in the cell-selection
extend handler before dereferencing them: validate the resolved row, column, and
corresponding cell from getAllCellsByColumnId(). If any is unavailable due to a
transient index mismatch, return without invoking getSelectionExtendHandler(),
while preserving the existing _isSelectingCells check and normal drag behavior.
In `@examples/react/spreadsheet/src/useGridInteractions.ts`:
- Around line 548-558: Update getRangeLabel to resolve column letters from the
display-ordered columns returned by getDisplayColumns(), rather than indexing
the model columns array with bound.minColumnIndex and bound.maxColumnIndex.
Preserve the existing fallback and range-formatting behavior, and include the
display-column source in the callback dependencies as needed.
- Around line 243-247: Update copyToClipboard to return whether the clipboard
write succeeded, including false when it catches a rejection, then update
cutToClipboard to clear and execute the cut patches only after a successful
copy. Preserve the existing copy behavior while preventing cell removal when
clipboard writing fails.
In `@examples/react/spreadsheet/vite.config.js`:
- Around line 6-20: The Vite configuration’s rollupReplace settings force
process.env.NODE_ENV to development for production builds. Update the
defineConfig setup to receive mode, apply the NODE_ENV replacement only for the
dev-server mode, and derive __DEV__ from mode so build and serve preserve
production optimization behavior.
---
Nitpick comments:
In `@examples/react/spreadsheet/src/CellContextMenu.tsx`:
- Around line 49-59: The CellContextMenu menu declares role="menu" without
usable keyboard focus management. Update the CellContextMenu focus and keyboard
handling to focus the first menuitem when opened and support ArrowUp/ArrowDown
navigation while preserving Escape closing; alternatively remove the
menu/menuitem ARIA roles and use plain button-group semantics. Also replace the
hard-coded 218/286 positioning offsets with measurements from the rendered menu
element rather than duplicating CSS dimensions.
In `@examples/react/spreadsheet/src/ColumnMenu.tsx`:
- Around line 85-93: Update the filter input in ColumnMenu to use local state
for immediate typing, then debounce calls to column.setFilterValue by
approximately 150–250ms. Keep the displayed value responsive, commit the latest
value after the debounce, and clean up or reset the pending timer when the
component or filter changes.
In `@examples/react/spreadsheet/src/Spreadsheet.tsx`:
- Around line 300-320: Update the ribbon tab markup around the `ribbon-tab-row`
and `spreadsheet-ribbon` elements so the `role="tablist"` contains only tab
elements, moving the non-tab `file-tab` outside that container or giving it an
appropriate surrounding structure. Add stable `id`/`aria-controls` wiring from
each mapped tab to the corresponding ribbon content, and expose the ribbon
content as the associated `tabpanel` while preserving the existing selected-tab
state and rendering behavior.
In `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx`:
- Around line 574-587: Update the column header element in the SpreadsheetGrid
render flow to be keyboard accessible: add tabIndex={0} and handle Enter and
Space key events by invoking interactions.selectColumn with the column id and
event, while preserving the existing click behavior and columnheader semantics.
- Around line 990-1012: Update getAutoFitColumnWidth to avoid measuring every
row synchronously: cap candidates to a bounded sample such as the first N rows
plus the current viewport, or prefilter by string length before calling
measureTextWidth. Also update measureTextWidth to use the spreadsheet’s actual
font stack instead of hardcoded 11px Arial so calculated widths match rendered
text.
- Around line 694-706: In the selection logic surrounding fullySelected, compute
the total visible leaf-column count once before calling bounds.some by combining
the start, center, and end visible leaf columns. Replace the callback’s repeated
array construction and length calculation with that hoisted value, preserving
the existing boundary comparison.
In `@examples/react/spreadsheet/src/spreadsheetModel.ts`:
- Around line 399-418: Cache the sourceValues computation in the fill logic
instead of recomputing it for every destination cell. In the vertical path,
memoize by destinationColumnId; in the horizontal path, memoize by rowId, while
preserving the existing range and getValue behavior. Reuse the cached values
when calling getFilledValue for each destination cell.
- Around line 227-237: Adjust the escapeTsvValue copy/paste flow so
formula-injection apostrophes are applied only to exported TSV, not internal
clipboard payloads; preserve the guard for external export and ensure
parseTsv/parseInputValue round-trips values without accumulating prefixes.
In `@examples/react/spreadsheet/src/useGridInteractions.ts`:
- Around line 560-586: The large-selection branch in getSelectionSummary should
indicate that numeric statistics were not calculated instead of returning zero
values that look like real results. Return null for the unavailable stats, or
add a truncated indicator while preserving the exact count, and update the
consuming status-bar rendering to display “—” for truncated summaries.
In `@examples/react/spreadsheet/src/useSpreadsheetHistory.ts`:
- Around line 113-141: Update applyPatches to iterate patches in reverse order
when value is 'before', while preserving forward order for 'after'. Keep the
existing row and cell cloning logic unchanged so undo restores the earliest
pre-command value when multiple patches target the same cell.
- Around line 26-34: Update the reducer callback around rowIndexById so the Map
is not constructed before handling every action, especially reset. Create it
lazily only within the action branches that read row indices, while preserving
the existing reset result and behavior for other actions.
In `@examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 34-38: Update the cell() selector to avoid hardcoding the row seed
in the row ID. Define a named constant for the sheet row seed with a clarifying
comment, or derive it from a stable application-level source, and use it when
constructing the row-${seed}-${rowIndex} selector so seed changes are explicit.
In `@examples/react/spreadsheet/vite.config.js`:
- Line 18: Update the Vite React plugin invocation in the configuration to
enable React Compiler by passing the Babel options with the
babel-plugin-react-compiler plugin, matching the declared devDependencies. Use
the existing react() call as the only change target and preserve the rest of the
Vite configuration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14c32f05-8cf3-459d-82dd-1749c866085b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
docs/config.jsonexamples/react/spreadsheet/index.htmlexamples/react/spreadsheet/package.jsonexamples/react/spreadsheet/src/CellContextMenu.tsxexamples/react/spreadsheet/src/ColumnMenu.tsxexamples/react/spreadsheet/src/Spreadsheet.tsxexamples/react/spreadsheet/src/SpreadsheetGrid.tsxexamples/react/spreadsheet/src/index.cssexamples/react/spreadsheet/src/main.tsxexamples/react/spreadsheet/src/spreadsheetModel.tsexamples/react/spreadsheet/src/spreadsheetTable.tsexamples/react/spreadsheet/src/useGridInteractions.tsexamples/react/spreadsheet/src/useSpreadsheetHistory.tsexamples/react/spreadsheet/src/vite-env.d.tsexamples/react/spreadsheet/tests/e2e/spreadsheet.spec.tsexamples/react/spreadsheet/tsconfig.jsonexamples/react/spreadsheet/vite.config.js
| --edge-top: 0; | ||
| --edge-right: 0; | ||
| --edge-bottom: 0; | ||
| --edge-left: 0; | ||
| display: flex; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint declaration-empty-line-before failure.
A blank line is required between the custom property block and the first standard declaration; this will fail lint in CI.
🎨 Proposed fix
--edge-bottom: 0;
--edge-left: 0;
+
display: flex;📝 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.
| --edge-top: 0; | |
| --edge-right: 0; | |
| --edge-bottom: 0; | |
| --edge-left: 0; | |
| display: flex; | |
| --edge-top: 0; | |
| --edge-right: 0; | |
| --edge-bottom: 0; | |
| --edge-left: 0; | |
| display: flex; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 528-528: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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 `@examples/react/spreadsheet/src/index.css` around lines 524 - 528, Insert a
blank line after the --edge-left custom property and before the display
declaration in the affected CSS rule to satisfy Stylelint’s
declaration-empty-line-before requirement.
Source: Linters/SAST tools
| if (!table._isSelectingCells) return | ||
| const row = table.getRowsInDisplayOrder()[coordinate.rowIndex] | ||
| const column = getDisplayColumns()[coordinate.columnIndex] | ||
| row.getAllCellsByColumnId()[column.id].getSelectionExtendHandler()(event) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the row/column lookups before invoking the extend handler.
coordinate indexes are resolved against the virtualizer/selection-index map, while these lookups go through getRowsInDisplayOrder() and getDisplayColumns(). Any transient desync (filtering/sorting/pinning applied between resolve and lookup) makes row, column, or the cell undefined and throws a TypeError mid-drag, which also leaves the drag state stuck.
🛡️ Proposed guard
if (!table._isSelectingCells) return
const row = table.getRowsInDisplayOrder()[coordinate.rowIndex]
const column = getDisplayColumns()[coordinate.columnIndex]
- row.getAllCellsByColumnId()[column.id].getSelectionExtendHandler()(event)
+ if (!row || !column) return
+ const cell = row.getAllCellsByColumnId()[column.id]
+ cell?.getSelectionExtendHandler()(event)📝 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.
| if (!table._isSelectingCells) return | |
| const row = table.getRowsInDisplayOrder()[coordinate.rowIndex] | |
| const column = getDisplayColumns()[coordinate.columnIndex] | |
| row.getAllCellsByColumnId()[column.id].getSelectionExtendHandler()(event) | |
| }, | |
| if (!table._isSelectingCells) return | |
| const row = table.getRowsInDisplayOrder()[coordinate.rowIndex] | |
| const column = getDisplayColumns()[coordinate.columnIndex] | |
| if (!row || !column) return | |
| const cell = row.getAllCellsByColumnId()[column.id] | |
| cell?.getSelectionExtendHandler()(event) | |
| }, |
🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 236 - 240,
Guard the row and column lookups in the cell-selection extend handler before
dereferencing them: validate the resolved row, column, and corresponding cell
from getAllCellsByColumnId(). If any is unavailable due to a transient index
mismatch, return without invoking getSelectionExtendHandler(), while preserving
the existing _isSelectingCells check and normal drag behavior.
| return () => { | ||
| document.removeEventListener('mousemove', handleMouseMove) | ||
| document.removeEventListener('mouseup', handleMouseUp) | ||
| if (scrollFrameRef.current != null) { | ||
| cancelAnimationFrame(scrollFrameRef.current) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup cancels the rAF without clearing scrollFrameRef, permanently disabling edge auto-scroll.
This effect re-runs whenever updateDragTarget/interactions identity changes (which happens on virtualization and data updates, including mid-drag). The cleanup cancels the frame but leaves scrollFrameRef.current non-null, so ensureEdgeScroll (Line 284) will never schedule another frame for the rest of the session.
🐛 Proposed fix
return () => {
document.removeEventListener('mousemove', handleMouseMove)
document.removeEventListener('mouseup', handleMouseUp)
if (scrollFrameRef.current != null) {
cancelAnimationFrame(scrollFrameRef.current)
+ scrollFrameRef.current = null
}
}📝 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.
| return () => { | |
| document.removeEventListener('mousemove', handleMouseMove) | |
| document.removeEventListener('mouseup', handleMouseUp) | |
| if (scrollFrameRef.current != null) { | |
| cancelAnimationFrame(scrollFrameRef.current) | |
| } | |
| } | |
| return () => { | |
| document.removeEventListener('mousemove', handleMouseMove) | |
| document.removeEventListener('mouseup', handleMouseUp) | |
| if (scrollFrameRef.current != null) { | |
| cancelAnimationFrame(scrollFrameRef.current) | |
| scrollFrameRef.current = null | |
| } | |
| } |
🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 316 - 322,
Update the cleanup returned by the effect in SpreadsheetGrid to set
scrollFrameRef.current to null after canceling its animation frame. Preserve the
existing cancellation and event-listener removal, ensuring ensureEdgeScroll can
schedule a new frame after the effect re-runs during a drag.
| <div | ||
| className="spreadsheet-canvas" | ||
| data-zoom={zoom} | ||
| style={{ | ||
| width: canvasWidth, | ||
| height: canvasHeight, | ||
| zoom: zoom / 100, | ||
| }} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Drag/fill coordinate resolution ignores the CSS zoom factor.
The canvas is scaled via zoom: zoom / 100, but resolveCoordinate (Lines 150-207) mixes unscaled viewport pixels (clientX/clientY, getBoundingClientRect() of the unscaled scroll container) with scaled canvas offsets from the virtualizers. At any zoom other than 100%, marquee selection extension and fill-drag will resolve to the wrong cells, with the error growing toward the far edge of the grid.
Divide the local coordinates (and the constants compared against them) by zoom / 100 before mapping to virtualizer offsets, or drive zoom via transform: scale() on the canvas and compensate explicitly.
🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 366 - 374,
Update resolveCoordinate to account for the canvas CSS zoom factor when
converting client coordinates into virtualizer offsets: divide the local X/Y
coordinates and any comparison constants by zoom / 100 before resolving cells.
Preserve correct coordinate mapping at 100% zoom and ensure marquee selection
and fill-drag remain accurate at other zoom levels.
| {virtualRows.map((virtualRow) => { | ||
| const row = centerRows[virtualRow.index] | ||
| return ( | ||
| <SubscribedRow | ||
| key={row.id} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Virtual items are indexed into row/column/cell arrays without a bounds guard. All three render loops assume virtualRow.index / virtualColumn.index are still valid for the current arrays. When the row or visible-column set shrinks (filter, sort, sheet switch, pinning change) the virtualizer can return a stale index for one render, yielding undefined and throwing on .id. getItemKey already defends with ?? index (Lines 81, 91); the render bodies do not.
examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L414-L418: addif (!row) return nullafterconst row = centerRows[virtualRow.index].examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L507-L512: addif (!header) return nullafterconst header = centerHeaders[virtualColumn.index].examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L749-L754: addif (!cell) return nullafterconst cell = centerCells[virtualColumn.index].
📍 Affects 1 file
examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L414-L418(this comment)examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L507-L512examples/react/spreadsheet/src/SpreadsheetGrid.tsx#L749-L754
🤖 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 `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx` around lines 414 - 418,
Guard all virtualized render lookups against stale indices: in
SpreadsheetGrid.tsx at lines 414-418, return null when the row lookup after
virtualRow.index is missing; at lines 507-512, return null when the header
lookup after virtualColumn.index is missing; and at lines 749-754, return null
when the cell lookup after virtualColumn.index is missing. Keep valid-item
rendering unchanged.
| const cutToClipboard = React.useCallback(async () => { | ||
| await copyToClipboard() | ||
| const patches = patchesForBounds(getSelectedBounds(), () => null) | ||
| execute('Cut cells', patches) | ||
| }, [copyToClipboard, execute, getSelectedBounds, patchesForBounds]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cut clears cells even when the clipboard write failed.
copyToClipboard swallows the rejection (Line 228), so a denied Clipboard API leaves the user with cleared cells and nothing on the clipboard. Have copyToClipboard report success and skip the clear on failure.
🐛 Proposed fix
const copyToClipboard = React.useCallback(async () => {
const text = serializeTsv(table.getSelectedCellRangesData())
- if (!text) return
+ if (!text) return false
try {
await navigator.clipboard.writeText(text)
+ return true
} catch {
// Clipboard access can be denied by an embedded docs preview. Keyboard
// copy remains available through the grid's native copy handler.
+ return false
}
}, [table])
const cutToClipboard = React.useCallback(async () => {
- await copyToClipboard()
+ if (!(await copyToClipboard())) return
const patches = patchesForBounds(getSelectedBounds(), () => null)
execute('Cut cells', patches)
}, [copyToClipboard, execute, getSelectedBounds, patchesForBounds])📝 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.
| const cutToClipboard = React.useCallback(async () => { | |
| await copyToClipboard() | |
| const patches = patchesForBounds(getSelectedBounds(), () => null) | |
| execute('Cut cells', patches) | |
| }, [copyToClipboard, execute, getSelectedBounds, patchesForBounds]) | |
| const cutToClipboard = React.useCallback(async () => { | |
| if (!(await copyToClipboard())) return | |
| const patches = patchesForBounds(getSelectedBounds(), () => null) | |
| execute('Cut cells', patches) | |
| }, [copyToClipboard, execute, getSelectedBounds, patchesForBounds]) |
🤖 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 `@examples/react/spreadsheet/src/useGridInteractions.ts` around lines 243 -
247, Update copyToClipboard to return whether the clipboard write succeeded,
including false when it catches a rejection, then update cutToClipboard to clear
and execute the cut patches only after a successful copy. Preserve the existing
copy behavior while preventing cell removal when clipboard writing fails.
| const getRangeLabel = React.useCallback(() => { | ||
| const bound = getSelectedBounds().at(-1) | ||
| if (!bound) return '' | ||
| const start = `${columns[bound.minColumnIndex]?.letter ?? '?'}${ | ||
| bound.minRowIndex + 1 | ||
| }` | ||
| const end = `${columns[bound.maxColumnIndex]?.letter ?? '?'}${ | ||
| bound.maxRowIndex + 1 | ||
| }` | ||
| return start === end ? start : `${start}:${end}` | ||
| }, [columns, getSelectedBounds]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Range label indexes the model columns array with a display index.
getSelectedBounds() returns indices into the display order produced by getDisplayColumns() (start-pinned + center + end-pinned), but columns[bound.minColumnIndex] indexes the unordered model metadata. With columnPinningFeature enabled — or any hidden column — the formula-bar label reports the wrong cell reference. Resolve the letter through the display columns instead.
🐛 Proposed fix
const getRangeLabel = React.useCallback(() => {
const bound = getSelectedBounds().at(-1)
if (!bound) return ''
- const start = `${columns[bound.minColumnIndex]?.letter ?? '?'}${
+ const displayColumns = getDisplayColumns()
+ const letterAt = (index: number) => {
+ const id = displayColumns.at(index)?.id
+ return (id ? columns.find((column) => column.id === id)?.letter : undefined) ?? '?'
+ }
+ const start = `${letterAt(bound.minColumnIndex)}${
bound.minRowIndex + 1
}`
- const end = `${columns[bound.maxColumnIndex]?.letter ?? '?'}${
+ const end = `${letterAt(bound.maxColumnIndex)}${
bound.maxRowIndex + 1
}`
return start === end ? start : `${start}:${end}`
- }, [columns, getSelectedBounds])
+ }, [columns, getDisplayColumns, getSelectedBounds])📝 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.
| const getRangeLabel = React.useCallback(() => { | |
| const bound = getSelectedBounds().at(-1) | |
| if (!bound) return '' | |
| const start = `${columns[bound.minColumnIndex]?.letter ?? '?'}${ | |
| bound.minRowIndex + 1 | |
| }` | |
| const end = `${columns[bound.maxColumnIndex]?.letter ?? '?'}${ | |
| bound.maxRowIndex + 1 | |
| }` | |
| return start === end ? start : `${start}:${end}` | |
| }, [columns, getSelectedBounds]) | |
| const getRangeLabel = React.useCallback(() => { | |
| const bound = getSelectedBounds().at(-1) | |
| if (!bound) return '' | |
| const displayColumns = getDisplayColumns() | |
| const letterAt = (index: number) => { | |
| const id = displayColumns.at(index)?.id | |
| return (id ? columns.find((column) => column.id === id)?.letter : undefined) ?? '?' | |
| } | |
| const start = `${letterAt(bound.minColumnIndex)}${ | |
| bound.minRowIndex + 1 | |
| }` | |
| const end = `${letterAt(bound.maxColumnIndex)}${ | |
| bound.maxRowIndex + 1 | |
| }` | |
| return start === end ? start : `${start}:${end}` | |
| }, [columns, getDisplayColumns, getSelectedBounds]) |
🤖 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 `@examples/react/spreadsheet/src/useGridInteractions.ts` around lines 548 -
558, Update getRangeLabel to resolve column letters from the display-ordered
columns returned by getDisplayColumns(), rather than indexing the model columns
array with bound.minColumnIndex and bound.maxColumnIndex. Preserve the existing
fallback and range-formatting behavior, and include the display-column source in
the callback dependencies as needed.
| export default defineConfig({ | ||
| server: { | ||
| port: 6565, | ||
| }, | ||
| plugins: [ | ||
| rollupReplace({ | ||
| preventAssignment: true, | ||
| values: { | ||
| __DEV__: JSON.stringify(true), | ||
| 'process.env.NODE_ENV': JSON.stringify('development'), | ||
| }, | ||
| }), | ||
| react(), | ||
| ], | ||
| }) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether __DEV__ is required by `@tanstack/react-store` at runtime.
rg -n '__DEV__' -g '*.ts' -g '*.tsx' node_modules/@tanstack/react-store 2>/dev/null || echo "package not vendored in sandbox"Repository: TanStack/table
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)vite\.config\.js$|(^|/)package\.json$' | sed -n '1,120p'
echo
echo "== target vite config =="
if [ -f examples/react/spreadsheet/vite.config.js ]; then
cat -n examples/react/spreadsheet/vite.config.js
fi
echo
echo "== target package scripts/deps =="
if [ -f examples/react/spreadsheet/package.json ]; then
cat -n examples/react/spreadsheet/package.json
fi
echo
echo "== process.env.NODE_ENV / __DEV__ usages =="
rg -n 'process\.env\.NODE_ENV|__DEV__|defineConfig|rollupReplace|react\(' examples/react/spreadsheet -S || trueRepository: TanStack/table
Length of output: 8041
🌐 Web query:
@rollup/plugin-replace executes during build dev command vite
💡 Result:
Yes, @rollup/plugin-replace executes during both the Vite dev command and the vite build command by default [1][2]. In Vite, Rollup plugins added to the plugins array in your vite.config.js are treated as compatible Vite plugins [1][2]. Because Vite’s development server uses a plugin container that invokes Rollup-compatible build hooks (such as transform) to process modules, @rollup/plugin-replace functions during development (the 'serve' command) just as it does during a production build [1][3][4]. If you wish to restrict the plugin to only run during the build process, you can explicitly set the apply property to 'build' in your configuration [1][2]: import replace from '@rollup/plugin-replace'; import { defineConfig } from 'vite'; export default defineConfig({ plugins: [ {...replace({ 'process.env.NODE_ENV': JSON.stringify('production'), }), apply: 'build', // Restricts execution to 'vite build' only }, ], }); Conversely, if you only want it to run during development, you can set apply to 'serve' [1][2]. By default, without the apply property specified, the plugin will remain active in both environments [1][2].
Citations:
- 1: https://vite.dev/guide/api-plugin
- 2: https://vite.dev/guide/using-plugins
- 3: https://github.com/vitejs/rolldown-vite/blob/82d9315d/docs/guide/api-plugin.md
- 4: https://frontend-build-tooling.com/vite-configuration-ecosystem/advanced-vite-plugin-configuration/
Don’t hard-code NODE_ENV to 'development' in the Vite build.
@rollup/plugin-replace runs during vite build, so this config makes npm run build/npm run serve bundle React and any process.env.NODE_ENV-gated code in dev mode, producing an unminified, unoptimized preview. Scope process.env.NODE_ENV to the dev server, and keep __DEV__ tied to mode if the replacement is still needed.
🤖 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 `@examples/react/spreadsheet/vite.config.js` around lines 6 - 20, The Vite
configuration’s rollupReplace settings force process.env.NODE_ENV to development
for production builds. Update the defineConfig setup to receive mode, apply the
NODE_ENV replacement only for the dev-server mode, and derive __DEV__ from mode
so build and serve preserve production optimization behavior.
1 participant
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit