docs: improve spreadsheet selection interactions - #6484
Conversation
📝 WalkthroughWalkthroughAdded complete Solid and Svelte spreadsheet examples with virtualized grids, editing, selection, clipboard operations, history, workbook sheets, filtering, sorting, menus, and end-to-end tests. Extended the React spreadsheet with scoped hotkeys and drag-based row and column selection. ChangesReact spreadsheet interaction updates
Solid spreadsheet example
Svelte spreadsheet example
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SpreadsheetGrid
participant GridInteractions
participant History
participant Spreadsheet
SpreadsheetGrid->>GridInteractions: Commit edit, paste, fill, or selection action
GridInteractions->>History: Execute cell patches
History-->>GridInteractions: Updated rows and undo/redo state
GridInteractions-->>SpreadsheetGrid: Updated selection and editing state
SpreadsheetGrid-->>Spreadsheet: Updated workbook view
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 77567a9
☁️ Nx Cloud last updated this comment at |
KevinVandy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (19)
examples/solid/spreadsheet/vite.config.ts (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
allowedHosts: trueto the server options.The declared Vite version is 8.2.0, which enables DNS rebinding protection. Without
allowedHosts, the example fails to load in CodeSandbox previews.♻️ Proposed change
- server: { port: 7777 }, + server: { port: 7777, allowedHosts: true },Based on learnings: in any Vite config file, add
server: { allowedHosts: true }to work with DNS rebinding protection in CodeSandbox environments when using newer Vite versions.🤖 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/solid/spreadsheet/vite.config.ts` at line 5, Update the Vite server options in the configuration to include allowedHosts: true alongside the existing port setting, preserving the current port behavior for CodeSandbox previews.Source: Learnings
examples/solid/spreadsheet/src/createGridInteractions.ts (1)
555-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated no-op
useCallbackshim in two files. Both files define the same localuseCallbackthat ignores its dependency array and returns the callback unchanged. The shared root cause is the React port: the shim exists only to keep the React call shape. Solid components run once, so no memoization is needed, and the dependency arrays mislead readers of an official Solid example.
examples/solid/spreadsheet/src/createGridInteractions.ts#L555-L560: delete the shim and convert eachuseCallback(fn, [...])call site to a plainconst fn = ...declaration.examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L1203-L1208: delete the shim and convertgetDisplayColumns,resolveCoordinate,applyHeaderSelectionDrag,updateDragTarget,runEdgeScroll,ensureEdgeScroll,startHeaderSelection,extendHeaderSelection, andstartFillDragto plain function declarations. Note that several dependency arrays in this file call signals, for example[centerColumns(), endColumns(), startColumns()]at Line 243, which reads them outside any reactive scope for no purpose.🤖 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/solid/spreadsheet/src/createGridInteractions.ts` around lines 555 - 560, Remove the local useCallback shim in examples/solid/spreadsheet/src/createGridInteractions.ts (lines 555-560) and replace every useCallback(fn, [...]) call with a plain const fn declaration. Also remove the shim in examples/solid/spreadsheet/src/SpreadsheetGrid.tsx (lines 1203-1208) and convert getDisplayColumns, resolveCoordinate, applyHeaderSelectionDrag, updateDragTarget, runEdgeScroll, ensureEdgeScroll, startHeaderSelection, extendHeaderSelection, and startFillDrag to plain function declarations, removing their dependency arrays including signal calls such as centerColumns(), endColumns(), and startColumns().examples/solid/spreadsheet/src/ColumnMenu.tsx (2)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClamp the top position and derive the width offset from one constant.
left()clamps withMath.max(8, ...), buttopuses onlyMath.min(...). If the viewport height is below 250px,topbecomes negative and the menu moves off screen. The literals286and250also duplicate the.column-menuwidth defined inindex.css.♻️ Proposed fix for the top clamp
const filterValue = () => String(props.column.getFilterValue() ?? '') - const left = () => Math.min(props.anchorRect.left, window.innerWidth - 286) + const MENU_WIDTH = 264 + const MENU_MARGIN = 8 + const MENU_MIN_HEIGHT = 250 + const left = () => + Math.min(props.anchorRect.left, window.innerWidth - MENU_WIDTH - MENU_MARGIN) return ( <Portal> <div ref={menuRef} class="column-menu" role="dialog" aria-label={`Column ${props.column.columnDef.meta?.letter ?? props.column.id} options`} style={{ - left: `${Math.max(8, left())}px`, - top: `${Math.min(props.anchorRect.bottom + 4, window.innerHeight - 250)}px`, + left: `${Math.max(MENU_MARGIN, left())}px`, + top: `${Math.max(MENU_MARGIN, Math.min(props.anchorRect.bottom + 4, window.innerHeight - MENU_MIN_HEIGHT))}px`, }} >🤖 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/solid/spreadsheet/src/ColumnMenu.tsx` around lines 32 - 43, Update the ColumnMenu position calculations to clamp the computed top to the same minimum inset used for the left position, preventing negative values on short viewports. Define or reuse a single menu dimension constant for the CSS width and use it consistently for the horizontal and vertical viewport offsets instead of the duplicated 286 and 250 literals.
80-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet focus on the filter input in
onMountinstead of usingautofocus.
ColumnMenurenders this input insidesolid-js/web'sPortal, soautofocusmay not work reliably for keyboard users after the component is mounted. Use a ref inonMountand callfocus()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/solid/spreadsheet/src/ColumnMenu.tsx` around lines 80 - 88, Update the filter input in ColumnMenu to remove the autofocus attribute, capture the element with a ref, and call its focus method from onMount after the Portal-mounted component initializes. Preserve the existing value binding, input handler, and placeholder.examples/solid/spreadsheet/src/Spreadsheet.tsx (1)
496-512: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
<For>for the reactive sheet list.
sheets().map(...)returns a static array of elements. Solid then re-creates every tab element when thesheetssignal changes, because there is no keyed reconciliation. Use<For>so Solid updates only the changed tabs.The static arrays at Line 273 and Line 397 do not need this change, because those values never change.
♻️ Proposed refactor
- {sheets().map((sheet) => ( - <button - type="button" - role="tab" - class={ - sheet.id === activeSheetId() - ? 'sheet-tab sheet-tab-active' - : 'sheet-tab' - } - aria-selected={sheet.id === activeSheetId()} - onClick={() => switchSheet(sheet.id)} - > - {sheet.name} - </button> - ))} + <For each={sheets()}> + {(sheet) => ( + <button + type="button" + role="tab" + class={ + sheet.id === activeSheetId() + ? 'sheet-tab sheet-tab-active' + : 'sheet-tab' + } + aria-selected={sheet.id === activeSheetId()} + onClick={() => switchSheet(sheet.id)} + > + {sheet.name} + </button> + )} + </For>Add
Forto thesolid-jsimport.🤖 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/solid/spreadsheet/src/Spreadsheet.tsx` around lines 496 - 512, Update the reactive sheet tab rendering in the sheet-tabs tablist to use Solid’s For component instead of sheets().map(...), and add For to the solid-js imports. Keep the existing tab attributes, active-state logic, click handler, and sheet name rendering unchanged; do not modify the static arrays elsewhere.examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts (1)
125-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for Shift-extend and Ctrl/Cmd include/exclude header-drag selection.
This test only exercises a plain drag across column and row headers. The PR objective states this change "Preserves Shift extension and Ctrl/Cmd include/exclude behavior for header selections," but no test here exercises Shift-click-extend or Ctrl/Cmd-click toggle for header drags. Add test cases for both modifier behaviors to validate this stated feature.
🤖 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 125 - 200, Add coverage alongside the existing “selects ranges by dragging across column and row headers” test for header-drag selection with Shift extension and Ctrl/Cmd include/exclude toggling. Reuse the column and row header locators and selection assertions to verify Shift extends an existing range, while the platform-appropriate Ctrl/Cmd modifier adds and removes selected headers; retain the existing cleanup and error assertions.examples/svelte/spreadsheet/src/SpreadsheetGrid.svelte (3)
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe column resizer is mouse-only.
Line 193 declares
role="separator"and wiresonmousedown,ontouchstart, andondblclick. The element has notabindex, so keyboard users cannot resize a column or trigger auto-fit.Add
tabindex="0"and arrow-key handling that callstable.setColumnSizing, and bindEntertoautoFit. A focusableseparatoralso needsaria-valuenow,aria-valuemin, andaria-valuemax.🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` at line 193, Add keyboard accessibility to the column resizer element in SpreadsheetGrid: add tabindex="0", expose aria-valuenow, aria-valuemin, and aria-valuemax, and handle arrow keys by calling table.setColumnSizing for column resizing. Bind Enter to autoFit while preserving the existing mouse, touch, and double-click behavior.
156-156: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the auto-fit measurement sample.
getAutoFitColumnWidthmeasures every row intable.options.data. WithSTRESS_ROW_COUNTat 10,000 rows, one double-click on a column resizer runs 10,000 synchronousmeasureTextcalls on the main thread.Measure a bounded sample instead. Spreadsheet applications commonly sample the visible rows plus a fixed cap.
♻️ Proposed bounded sample
- function getAutoFitColumnWidth(table: SpreadsheetTable, column: SpreadsheetTableColumn) { const index = column.columnDef.meta?.index; if (index == null) return column.getSize(); let widest = 0; for (const row of table.options.data) widest = Math.max(widest, measure(formatted(row.cells[index]), row.kind === 'field-header')); return Math.max(column.columnDef.minSize ?? 0, Math.ceil(widest + CELL_HORIZONTAL_PADDING + 2)) } + const AUTO_FIT_SAMPLE_LIMIT = 500 + function getAutoFitColumnWidth(table: SpreadsheetTable, column: SpreadsheetTableColumn) { const index = column.columnDef.meta?.index; if (index == null) return column.getSize(); let widest = 0; const data = table.options.data; const limit = Math.min(data.length, AUTO_FIT_SAMPLE_LIMIT); for (let i = 0; i < limit; i++) { const row = data[i]!; widest = Math.max(widest, measure(formatted(row.cells[index]), row.kind === 'field-header')) } return Math.max(column.columnDef.minSize ?? 0, Math.ceil(widest + CELL_HORIZONTAL_PADDING + 2)) }🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` at line 156, Update getAutoFitColumnWidth to measure only a bounded sample of table.options.data, combining visible rows with a fixed maximum cap rather than iterating every row. Preserve the existing width calculation, minimum-size handling, header distinction, and fallback behavior for columns without an index.
163-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the header height between the script and the stylesheet.
Line 15 defines
HEADER_HEIGHT = 26, and line 163 applies it inline.examples/svelte/spreadsheet/src/index.cssline 352 hardcodestop: 26pxfor.frozen-row-region, and line 388 hardcodesheight: 26pxfor.corner-header. A change toHEADER_HEIGHTsilently misaligns the frozen row region.Expose the value as a CSS custom property on
.spreadsheet-canvasand consume it in the stylesheet.Also applies to: 172-172
🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` at line 163, Share HEADER_HEIGHT between SpreadsheetGrid.svelte and index.css by exposing it as a CSS custom property on the spreadsheet-canvas element. Replace the hardcoded 26px values for top in .frozen-row-region and height in .corner-header with that custom property, while preserving the existing header sizing behavior.examples/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts (4)
125-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the header Shift and Ctrl modifiers.
The PR objectives state that header selection preserves Shift extension and Ctrl/Cmd include and exclude behavior.
SpreadsheetGrid.sveltelines 143 and 144 implement both branches. This test covers only the plain drag path, so neither modifier branch has coverage.Add a case that Shift-clicks a second column header and asserts the extended range, and a case that Ctrl-clicks a selected header and asserts it becomes unselected.
🤖 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/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts` around lines 125 - 199, The header-selection test currently covers only plain dragging; add coverage for both modifier behaviors implemented by SpreadsheetGrid.svelte. In the existing test around the column-header selection flow, Shift-click a second column header and assert the intervening headers are selected, then Ctrl/Cmd-click a selected header and assert that header becomes unselected while preserving the relevant selection.
541-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
originalcapture, or assert against it.Line 541 stores the cell text before editing. Line 575 only asserts that the value is not null. The test never compares a later state against
original, so the capture and the assertion add no coverage.Either drop both lines, or restore the cell with undo at the end and assert
toHaveText(original ?? '').Also applies to: 575-575
🤖 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/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts` at line 541, Remove the unused original text capture and its corresponding assertion from the spreadsheet test, unless restoring the cell with undo and asserting against original is required; keep the remaining edit behavior and meaningful assertions unchanged.
355-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the descending sort assertion.
Line 368 asserts
revenueAfter >= revenueBefore. That comparison also passes when the sort does nothing, because the two values are then identical. The test cannot fail ifSort Z → Astops working.Assert the ordering of two adjacent displayed rows instead, or assert that the first displayed revenue equals the maximum of the filtered set.
💚 Proposed stronger assertion
- expect(revenueAfter).toBeGreaterThanOrEqual(revenueBefore) + const revenueNext = Number( + ( + await page + .locator('[data-row-index="2"] [data-column-id="column-4"]') + .textContent() + )?.replaceAll(',', ''), + ) + expect(revenueAfter).toBeGreaterThanOrEqual(revenueNext) + expect(revenueAfter).toBeGreaterThan(revenueBefore)🤖 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/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts` around lines 355 - 368, Strengthen the assertion in the descending-sort test around the “Sort Z → A” action so it verifies the displayed ordering rather than comparing the post-sort value with the pre-sort value. Use two adjacent displayed revenue cells (or the filtered set’s maximum) and assert that the first sorted value is greater than or equal to the next value, while preserving the existing sort interaction.
247-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a dataset load followed by a sheet round trip.
This test covers sheet switching with the default dataset only. It never loads a different dataset before switching sheets. That path is the one that fails because of the missing persistence in
loadDataset, which I flagged atexamples/svelte/spreadsheet/src/App.svelteline 49.Add a case that clicks 'Stress data', adds a sheet, switches back to Sheet1, and asserts the dataset size still reads '10,000 × 250'.
🤖 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/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts` around lines 247 - 275, Add coverage to the spreadsheet e2e test around the existing sheet navigation flow: click the “Stress data” dataset control, add a sheet, switch back to “Sheet1”, and assert the displayed dataset size remains “10,000 × 250”. Keep the existing sheet persistence assertions intact and reuse the test’s established role-based locators and sheet navigation controls.examples/svelte/spreadsheet/src/App.svelte (2)
72-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle rejections from the clipboard promises.
Line 72 uses
void interactions.pasteFromClipboard(),void interactions.cutToClipboard(), andvoid interactions.copyToClipboard().voiddiscards the promise without a rejection handler. The async clipboard API rejects when the user denies permission or when the document is not focused. Each rejection becomes an unhandled promise rejection.
CellContextMenu.svelteline 27 has the same pattern inrun.Attach a
catchhandler, or wrap the calls in a helper that reports the failure.♻️ Proposed helper
function formatNumber(value: number) { return new Intl.NumberFormat(undefined, { maximumFractionDigits: 2 }).format(value) } + function runClipboard(action: () => Promise<void>) { action().catch((error: unknown) => console.warn('Clipboard action failed', error)) }Then replace each
void interactions.xToClipboard()call withrunClipboard(interactions.xToClipboard).🤖 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/svelte/spreadsheet/src/App.svelte` at line 72, Handle rejected clipboard promises in the ribbon handlers in App.svelte and the run handler in CellContextMenu.svelte. Replace the bare void calls to pasteFromClipboard, cutToClipboard, and copyToClipboard with a shared or local helper that attaches a catch handler and reports failures, then use that helper for each clipboard operation.
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe live region announces on every selection change.
.selection-summaryusesaria-live="polite".summaryrecomputes on every cell-selection change, including each step of a drag selection. A screen reader queues an announcement for each step.Announce the summary only after the selection settles. Debounce the value that feeds the live region, or move the live region update to the drag end.
🤖 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/svelte/spreadsheet/src/App.svelte` at line 89, Update the selection summary around the .selection-summary live region so it does not react to every intermediate drag-selection change. Debounce the summary value used by aria-live="polite", or update a separate live-region value only when the selection drag ends, while preserving the existing count, numeric count, sum, and average display.examples/svelte/spreadsheet/src/ColumnMenu.svelte (2)
6-6: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebounce the filter input.
Line 22 calls
column.setFilterValueon everyinputevent. Each call re-runs the filtered row model, and the sorted row model after it. The default dataset has 2,000 rows and the stress dataset has 10,000 rows, so every keystroke filters the whole dataset synchronously.Debounce the write, and keep a local draft value so the input stays responsive.
♻️ Proposed debounce
let menuRef: HTMLDivElement const filterValue = $derived(String(column.getFilterValue() ?? '')) + let draft = $state<string | null>(null) + let timer: ReturnType<typeof setTimeout> | undefined + function queueFilter(next: string) { + draft = next + clearTimeout(timer) + timer = setTimeout(() => { column.setFilterValue(next || undefined); draft = null }, 150) + }- <label>Filter values containing<input autofocus value={filterValue} oninput={(event) => column.setFilterValue(event.currentTarget.value)} placeholder="Type to filter…" /></label> + <label>Filter values containing<input autofocus value={draft ?? filterValue} oninput={(event) => queueFilter(event.currentTarget.value)} placeholder="Type to filter…" /></label>Clear the pending timer in the
onMountcleanup so a queued write cannot run after the menu closes.Also applies to: 22-22
🤖 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/svelte/spreadsheet/src/ColumnMenu.svelte` at line 6, Update ColumnMenu.svelte’s filter input flow to maintain a local draft value for immediate UI updates and debounce calls to column.setFilterValue instead of writing on every input event. Create the debounce timer within the component’s existing lifecycle context and clear it in onMount cleanup so pending writes cannot execute after the menu closes; initialize the draft from the current filterValue and preserve synchronization with external filter changes.
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth menus clamp their position with bare
innerWidthandinnerHeightglobals. The shared root cause is a direct global lookup instead of a reactive viewport size. Svelte resolves the bare identifiers towindow.innerWidthandwindow.innerHeight, so the values are read once during render and never update. A viewport resize while a menu is open leaves the menu clipped. The bare reference also breaks if the component is ever rendered outside a browser.Bind the viewport size once with
<svelte:window bind:innerWidth bind:innerHeight />and use the bound values, so the clamp stays correct and the dependency is explicit.
examples/svelte/spreadsheet/src/ColumnMenu.svelte#L16-L16: replace the bareinnerWidthandinnerHeightreads in theleftandtopclamps with values bound through<svelte:window>.examples/svelte/spreadsheet/src/CellContextMenu.svelte#L30-L30: apply the same binding. Also replace the hardcoded218and286literals, which duplicate the.cell-context-menuwidth of210pxset atexamples/svelte/spreadsheet/src/index.cssline 654 and an estimate of the current item count. MeasuremenuRefafter mount and clamp against the measured size, and add a lower clamp matching theMath.max(8, ...)already used inColumnMenu.svelte.🤖 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/svelte/spreadsheet/src/ColumnMenu.svelte` at line 16, Update examples/svelte/spreadsheet/src/ColumnMenu.svelte lines 16-16 by adding a <svelte:window> binding for reactive innerWidth and innerHeight, then use those bound values in the left and top clamps. Update examples/svelte/spreadsheet/src/CellContextMenu.svelte lines 30-30 with the same viewport binding; measure menuRef after mount, replace the hardcoded 218 and 286 dimensions with the measured menu size, and add the lower horizontal clamp matching ColumnMenu.svelte’s minimum of 8.examples/svelte/spreadsheet/src/CellContextMenu.svelte (1)
15-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe two menu components duplicate the dismissal logic. Both components register the same
pointerdownandkeydowndocument listeners inonMount, apply the samemenuRef?.contains(...)outside test, and return the same cleanup. The shared root cause is a missing reusable primitive for outside-dismiss behavior.Extract one Svelte attachment or action, for example
dismissOnOutside(node, onClose), and use it in both components. A single implementation also gives one place to add the focus restoration that the context menu needs.
examples/svelte/spreadsheet/src/CellContextMenu.svelte#L15-L26: replace theonMountblock with the shared attachment applied to the menu element.examples/svelte/spreadsheet/src/ColumnMenu.svelte#L7-L13: replace the identicalonMountblock with the same shared attachment.🤖 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/svelte/spreadsheet/src/CellContextMenu.svelte` around lines 15 - 26, Extract the duplicated outside-dismiss behavior into a shared Svelte attachment or action such as dismissOnOutside, including document pointerdown and Escape-key listeners, the menuRef containment check, cleanup, and context-menu focus restoration. In examples/svelte/spreadsheet/src/CellContextMenu.svelte lines 15-26 and examples/svelte/spreadsheet/src/ColumnMenu.svelte lines 7-13, remove the local onMount listener blocks and apply the shared primitive to each menu element.examples/svelte/spreadsheet/src/createGridInteractions.svelte.ts (1)
554-575: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the Solid/React-style shim layer.
createSignal,createMemo, anduseCallbackreimplement Solid/React primitives on top of$state. Svelte 5 has no render-cycle identity concern, souseCallbackhere is a plain passthrough that adds indirection without benefit. Writing this file with$state,$derived/$derived.by, and plain functions directly would be more idiomatic and easier to reason about for readers familiar with Svelte 5 runes.This is optional; it does not change behavior once the
createMemofix above is applied.🤖 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/svelte/spreadsheet/src/createGridInteractions.svelte.ts` around lines 554 - 575, Remove the Solid/React-style shim functions createSignal, createMemo, and useCallback, and update their call sites to use Svelte 5 runes directly: $state for mutable state, $derived or $derived.by for computed values, and plain functions for callbacks. Preserve the existing behavior and apply this only as an idiomatic simplification.
🤖 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/solid/spreadsheet/src/index.css`:
- Around line 523-536: Insert an empty line between the final custom property
`--edge-left` and the `display: flex` declaration in the `.spreadsheet-cell`
rule to satisfy the `declaration-empty-line-before` Stylelint rule.
In `@examples/solid/spreadsheet/src/Spreadsheet.tsx`:
- Around line 647-652: Remove the local useCallback shim and its
dependency-array arguments throughout the Spreadsheet component. Convert each
affected callback at the identified call sites near lines 186, 201, and 225 to
plain function declarations or equivalent stable functions, preserving their
existing callback behavior while preventing setup-time evaluation of reactive
expressions such as activeSheetId(), history.rows(), spreadsheetData(),
sheets(), and sheets().length.
- Around line 574-575: Remove the createEffect that resets draft from
props.initialValue() near the draft signal declaration, leaving draft
initialized from initialValue only once. Preserve draft independently while
editing, and update it only through the existing commit or cancel flows.
In `@examples/solid/spreadsheet/src/SpreadsheetGrid.tsx`:
- Around line 611-615: Guard the virtual-index lookups before rendering their
children: at examples/solid/spreadsheet/src/SpreadsheetGrid.tsx lines 611-615,
only render SubscribedRow when centerRows()[virtualRow.index] exists; at lines
710-712, only render HeaderCell when centerHeaders[virtualColumn.index] exists;
and at lines 951-953, only render SpreadsheetCell when
centerCells[virtualColumn.index] exists. Use a Show wrapper or early null return
at each site so inconsistent reactive sources cannot pass undefined to child
components.
- Around line 385-389: Update the cell-selection branch in updateDragTarget to
guard the row, column, and cell lookups before calling
getSelectionExtendHandler. Return early when any lookup is undefined, matching
the existing header-drag guard, so stale virtualized indices cannot cause a
TypeError during runEdgeScroll.
- Around line 871-887: Replace the affected parent list renders at the
referenced `.map` sites with Solid’s keyed `<For>`, preserving each item’s key
and rendered child structure. Update `SpreadsheetRowView`, `HeaderRow`,
`HeaderCell`, and `SpreadsheetCell` to accept a `props` object and access values
through `props.x` so prop getters remain reactive. Remove the redundant
`SubscribedRow` pass-through wrapper and render `SpreadsheetRowView` directly.
In `@examples/svelte/spreadsheet/src/App.svelte`:
- Around line 43-44: Update the value-bar editing flow around formulaDraft and
commitFormula to capture the active row and column when the input receives
focus, store them in formulaTarget, and have commitFormula use that captured
pair instead of reading active at commit time. Clear formulaTarget after
committing, and wire the value-bar input’s focus and blur handlers so drafts
always apply to the cell originally edited.
- Line 49: Update loadDataset to write the newly generated dataset into the
sheets entry matching activeSheetId, in addition to assigning spreadsheetData
and resetting history/view state. Preserve the existing persist behavior used by
switchSheet and addSheet so the active sheet retains the loaded dataset when
navigation occurs.
In `@examples/svelte/spreadsheet/src/CellContextMenu.svelte`:
- Line 27: Update the run function so closing the menu also restores focus to
the spreadsheet grid element, using the grid’s existing focusable symbol or
reference from SpreadsheetGrid.svelte. Ensure focus is restored before or after
invoking action, while preserving the current onClose-then-action behavior.
In `@examples/svelte/spreadsheet/src/ColumnMenu.svelte`:
- Around line 22-23: Update the filter input handler in ColumnMenu to normalize
an empty or whitespace-free value to undefined before calling
column.setFilterValue; preserve non-empty input values unchanged so clearing the
field removes the active filter entry and filtered indicators.
In `@examples/svelte/spreadsheet/src/createGridInteractions.svelte.ts`:
- Around line 566-568: Replace the no-op createMemo implementation with a Svelte
reactive derivation using $derived.by, preserving the read callback and its
return type so rowById and columnIndexById cache their computed Maps until
dependencies change.
In `@examples/svelte/spreadsheet/src/index.css`:
- Around line 524-528: Insert a blank line between the custom property
declarations (--edge-top through --edge-left) and the display: flex declaration
in the affected style rule to satisfy the declaration-empty-line-before lint
rule.
In `@examples/svelte/spreadsheet/src/SpreadsheetGrid.svelte`:
- Line 164: Add role="columnheader" to the corner-header button and
role="rowheader" to each row-header button in SpreadsheetGrid.svelte, preserving
their labels and focusability. Update the affected end-to-end selectors in
spreadsheet.spec.ts to query the new ARIA roles while retaining the existing
selection behavior.
- Around line 79-80: Update the keyboard bindings in SpreadsheetGrid so Tab and
Shift+Tab do not trap focus within the grid. Preserve in-grid movement where
appropriate, but allow focus to leave at the grid boundary or provide an Escape
path that moves focus out when the selection is already empty; adjust the
preventDefault behavior accordingly.
- Line 218: Update the fill-handle span rendered by the showFill branch to
reflect its mouse-only behavior: remove its aria-label and add
aria-hidden="true" so assistive technology ignores it, preserving the existing
startFill mouse interaction.
- Around line 96-107: Update the coordinate fallback around localX/localY so
visual-pixel offsets are converted to unzoomed content units using the canvas
zoom scale before row/column size comparisons and virtualizer offsets. Apply the
same scale conversion to the rect width/end-column boundary comparison and the
rect edges used by edgeDelta. Preserve elementFromPoint hit detection while
ensuring all fallback geometry uses consistent content-space coordinates at any
zoom.
In `@examples/svelte/spreadsheet/src/spreadsheetTable.ts`:
- Around line 40-64: Memoize the reordered model in
createSpreadsheetSortedRowModel for every spreadsheet example wrapper. Cache the
generated { rows, flatRows } model by the unchanged getBaseRowModel() result
(using reference or model id), return the cached model on repeated evaluations,
and rebuild the reordered arrays only when the base model changes.
In `@examples/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 131-132: Replace the ambiguous accessible-name columnheader
locators throughout the spreadsheet test, including the declarations near lines
61 and 131, the loop near line 150, and the D-header lookup near line 156, with
locators targeting each header’s unique data-column-id attribute. Preserve the
existing column IDs and assertions while ensuring every locator resolves to
exactly one header.
---
Nitpick comments:
In `@examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 125-200: Add coverage alongside the existing “selects ranges by
dragging across column and row headers” test for header-drag selection with
Shift extension and Ctrl/Cmd include/exclude toggling. Reuse the column and row
header locators and selection assertions to verify Shift extends an existing
range, while the platform-appropriate Ctrl/Cmd modifier adds and removes
selected headers; retain the existing cleanup and error assertions.
In `@examples/solid/spreadsheet/src/ColumnMenu.tsx`:
- Around line 32-43: Update the ColumnMenu position calculations to clamp the
computed top to the same minimum inset used for the left position, preventing
negative values on short viewports. Define or reuse a single menu dimension
constant for the CSS width and use it consistently for the horizontal and
vertical viewport offsets instead of the duplicated 286 and 250 literals.
- Around line 80-88: Update the filter input in ColumnMenu to remove the
autofocus attribute, capture the element with a ref, and call its focus method
from onMount after the Portal-mounted component initializes. Preserve the
existing value binding, input handler, and placeholder.
In `@examples/solid/spreadsheet/src/createGridInteractions.ts`:
- Around line 555-560: Remove the local useCallback shim in
examples/solid/spreadsheet/src/createGridInteractions.ts (lines 555-560) and
replace every useCallback(fn, [...]) call with a plain const fn declaration.
Also remove the shim in examples/solid/spreadsheet/src/SpreadsheetGrid.tsx
(lines 1203-1208) and convert getDisplayColumns, resolveCoordinate,
applyHeaderSelectionDrag, updateDragTarget, runEdgeScroll, ensureEdgeScroll,
startHeaderSelection, extendHeaderSelection, and startFillDrag to plain function
declarations, removing their dependency arrays including signal calls such as
centerColumns(), endColumns(), and startColumns().
In `@examples/solid/spreadsheet/src/Spreadsheet.tsx`:
- Around line 496-512: Update the reactive sheet tab rendering in the sheet-tabs
tablist to use Solid’s For component instead of sheets().map(...), and add For
to the solid-js imports. Keep the existing tab attributes, active-state logic,
click handler, and sheet name rendering unchanged; do not modify the static
arrays elsewhere.
In `@examples/solid/spreadsheet/vite.config.ts`:
- Line 5: Update the Vite server options in the configuration to include
allowedHosts: true alongside the existing port setting, preserving the current
port behavior for CodeSandbox previews.
In `@examples/svelte/spreadsheet/src/App.svelte`:
- Line 72: Handle rejected clipboard promises in the ribbon handlers in
App.svelte and the run handler in CellContextMenu.svelte. Replace the bare void
calls to pasteFromClipboard, cutToClipboard, and copyToClipboard with a shared
or local helper that attaches a catch handler and reports failures, then use
that helper for each clipboard operation.
- Line 89: Update the selection summary around the .selection-summary live
region so it does not react to every intermediate drag-selection change.
Debounce the summary value used by aria-live="polite", or update a separate
live-region value only when the selection drag ends, while preserving the
existing count, numeric count, sum, and average display.
In `@examples/svelte/spreadsheet/src/CellContextMenu.svelte`:
- Around line 15-26: Extract the duplicated outside-dismiss behavior into a
shared Svelte attachment or action such as dismissOnOutside, including document
pointerdown and Escape-key listeners, the menuRef containment check, cleanup,
and context-menu focus restoration. In
examples/svelte/spreadsheet/src/CellContextMenu.svelte lines 15-26 and
examples/svelte/spreadsheet/src/ColumnMenu.svelte lines 7-13, remove the local
onMount listener blocks and apply the shared primitive to each menu element.
In `@examples/svelte/spreadsheet/src/ColumnMenu.svelte`:
- Line 6: Update ColumnMenu.svelte’s filter input flow to maintain a local draft
value for immediate UI updates and debounce calls to column.setFilterValue
instead of writing on every input event. Create the debounce timer within the
component’s existing lifecycle context and clear it in onMount cleanup so
pending writes cannot execute after the menu closes; initialize the draft from
the current filterValue and preserve synchronization with external filter
changes.
- Line 16: Update examples/svelte/spreadsheet/src/ColumnMenu.svelte lines 16-16
by adding a <svelte:window> binding for reactive innerWidth and innerHeight,
then use those bound values in the left and top clamps. Update
examples/svelte/spreadsheet/src/CellContextMenu.svelte lines 30-30 with the same
viewport binding; measure menuRef after mount, replace the hardcoded 218 and 286
dimensions with the measured menu size, and add the lower horizontal clamp
matching ColumnMenu.svelte’s minimum of 8.
In `@examples/svelte/spreadsheet/src/createGridInteractions.svelte.ts`:
- Around line 554-575: Remove the Solid/React-style shim functions createSignal,
createMemo, and useCallback, and update their call sites to use Svelte 5 runes
directly: $state for mutable state, $derived or $derived.by for computed values,
and plain functions for callbacks. Preserve the existing behavior and apply this
only as an idiomatic simplification.
In `@examples/svelte/spreadsheet/src/SpreadsheetGrid.svelte`:
- Line 193: Add keyboard accessibility to the column resizer element in
SpreadsheetGrid: add tabindex="0", expose aria-valuenow, aria-valuemin, and
aria-valuemax, and handle arrow keys by calling table.setColumnSizing for column
resizing. Bind Enter to autoFit while preserving the existing mouse, touch, and
double-click behavior.
- Line 156: Update getAutoFitColumnWidth to measure only a bounded sample of
table.options.data, combining visible rows with a fixed maximum cap rather than
iterating every row. Preserve the existing width calculation, minimum-size
handling, header distinction, and fallback behavior for columns without an
index.
- Line 163: Share HEADER_HEIGHT between SpreadsheetGrid.svelte and index.css by
exposing it as a CSS custom property on the spreadsheet-canvas element. Replace
the hardcoded 26px values for top in .frozen-row-region and height in
.corner-header with that custom property, while preserving the existing header
sizing behavior.
In `@examples/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 125-199: The header-selection test currently covers only plain
dragging; add coverage for both modifier behaviors implemented by
SpreadsheetGrid.svelte. In the existing test around the column-header selection
flow, Shift-click a second column header and assert the intervening headers are
selected, then Ctrl/Cmd-click a selected header and assert that header becomes
unselected while preserving the relevant selection.
- Line 541: Remove the unused original text capture and its corresponding
assertion from the spreadsheet test, unless restoring the cell with undo and
asserting against original is required; keep the remaining edit behavior and
meaningful assertions unchanged.
- Around line 355-368: Strengthen the assertion in the descending-sort test
around the “Sort Z → A” action so it verifies the displayed ordering rather than
comparing the post-sort value with the pre-sort value. Use two adjacent
displayed revenue cells (or the filtered set’s maximum) and assert that the
first sorted value is greater than or equal to the next value, while preserving
the existing sort interaction.
- Around line 247-275: Add coverage to the spreadsheet e2e test around the
existing sheet navigation flow: click the “Stress data” dataset control, add a
sheet, switch back to “Sheet1”, and assert the displayed dataset size remains
“10,000 × 250”. Keep the existing sheet persistence assertions intact and reuse
the test’s established role-based locators and sheet navigation controls.
🪄 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: e156087e-5ed0-458d-80ea-55f24ecd72bd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
docs/config.jsonexamples/react/spreadsheet/package.jsonexamples/react/spreadsheet/src/SpreadsheetGrid.tsxexamples/react/spreadsheet/src/useGridInteractions.tsexamples/react/spreadsheet/tests/e2e/spreadsheet.spec.tsexamples/solid/spreadsheet/index.htmlexamples/solid/spreadsheet/package.jsonexamples/solid/spreadsheet/src/CellContextMenu.tsxexamples/solid/spreadsheet/src/ColumnMenu.tsxexamples/solid/spreadsheet/src/Spreadsheet.tsxexamples/solid/spreadsheet/src/SpreadsheetGrid.tsxexamples/solid/spreadsheet/src/createGridInteractions.tsexamples/solid/spreadsheet/src/createSpreadsheetHistory.tsexamples/solid/spreadsheet/src/index.cssexamples/solid/spreadsheet/src/index.tsxexamples/solid/spreadsheet/src/spreadsheetModel.tsexamples/solid/spreadsheet/src/spreadsheetTable.tsexamples/solid/spreadsheet/src/vite-env.d.tsexamples/solid/spreadsheet/tests/e2e/spreadsheet.spec.tsexamples/solid/spreadsheet/tsconfig.jsonexamples/solid/spreadsheet/vite.config.tsexamples/svelte/spreadsheet/index.htmlexamples/svelte/spreadsheet/package.jsonexamples/svelte/spreadsheet/src/App.svelteexamples/svelte/spreadsheet/src/CellContextMenu.svelteexamples/svelte/spreadsheet/src/ColumnMenu.svelteexamples/svelte/spreadsheet/src/SpreadsheetGrid.svelteexamples/svelte/spreadsheet/src/createGridInteractions.svelte.tsexamples/svelte/spreadsheet/src/index.cssexamples/svelte/spreadsheet/src/main.tsexamples/svelte/spreadsheet/src/spreadsheetHistory.svelte.tsexamples/svelte/spreadsheet/src/spreadsheetModel.tsexamples/svelte/spreadsheet/src/spreadsheetTable.tsexamples/svelte/spreadsheet/src/vite-env.d.tsexamples/svelte/spreadsheet/svelte.config.jsexamples/svelte/spreadsheet/tests/e2e/spreadsheet.spec.tsexamples/svelte/spreadsheet/tsconfig.jsonexamples/svelte/spreadsheet/vite.config.js
| .spreadsheet-cell { | ||
| --edge-top: 0; | ||
| --edge-right: 0; | ||
| --edge-bottom: 0; | ||
| --edge-left: 0; | ||
| display: flex; | ||
| align-items: center; | ||
| height: 24px; | ||
| padding: 0 5px; | ||
| outline: none; | ||
| cursor: cell; | ||
| font-size: 11px; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint failure at Line 528.
Stylelint reports declaration-empty-line-before for display: flex;. The rule requires an empty line after the custom property block. Add the empty line so the lint check passes.
🐛 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.
| .spreadsheet-cell { | |
| --edge-top: 0; | |
| --edge-right: 0; | |
| --edge-bottom: 0; | |
| --edge-left: 0; | |
| display: flex; | |
| align-items: center; | |
| height: 24px; | |
| padding: 0 5px; | |
| outline: none; | |
| cursor: cell; | |
| font-size: 11px; | |
| white-space: nowrap; | |
| } | |
| .spreadsheet-cell { | |
| --edge-top: 0; | |
| --edge-right: 0; | |
| --edge-bottom: 0; | |
| --edge-left: 0; | |
| display: flex; | |
| align-items: center; | |
| height: 24px; | |
| padding: 0 5px; | |
| outline: none; | |
| cursor: cell; | |
| font-size: 11px; | |
| white-space: nowrap; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.1)
[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/solid/spreadsheet/src/index.css` around lines 523 - 536, Insert an
empty line between the final custom property `--edge-left` and the `display:
flex` declaration in the `.spreadsheet-cell` rule to satisfy the
`declaration-empty-line-before` Stylelint rule.
Source: Linters/SAST tools
| const [draft, setDraft] = createSignal(props.initialValue()) | ||
| createEffect(() => setDraft(props.initialValue())) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how the formula bar draft interacts with interaction-driven value changes.
set -euo pipefail
fd -t f 'Spreadsheet.tsx|createGridInteractions.ts' examples/solid/spreadsheet/src
# Show the formula bar and the commit/edit paths that can change the active cell value.
rg -n -C 6 'initialValue|commitCellValue|setFocusedCell|cancelEditing' examples/solid/spreadsheet/srcRepository: TanStack/table
Length of output: 12091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the cell value/source-of-truth helpers and interaction paths.
rg -n "function formatCellValue|const formatCellValue|formatCellValue|undo|pasteSelection|cutSelection|copySelection|setValue|setFocusedCell|focusedCell|initialValue|activeValue|getCellSelectionBounds" examples/solid/spreadsheet/src
# Show formula bar and focused cell derivation slices.
sed -n '400,460p' examples/solid/spreadsheet/src/Spreadsheet.tsx
sed -n '90,135p' examples/solid/spreadsheet/src/createGridInteractions.ts
sed -n '569,615p' examples/solid/spreadsheet/src/Spreadsheet.tsx
# Programmatic read-only source-shape probe for the relevant draft refresh and input mutation order.
python3 - <<'PY'
from pathlib import Path
p=Path('examples/solid/spreadsheet/src/Spreadsheet.tsx')
s=p.read_text()
checks={
'draft_effect_exists': "createEffect(() => setDraft(props.initialValue()))" in s,
'input_mutates_draft': "onInput={(event) => setDraft(event.currentTarget.value)}" in s,
'escape_computes_initial_before_cancel': "setDraft(props.initialValue())" in s and "props.interactions.cancelEditing()" in s,
'formula_bar_props_derive_active_value': "<SpreadsheetFormulaBar" in s and "initialValue={activeValue}" in s,
}
for k,v in checks.items():
print(f"{k}={v}")
# Minimal SolidEffect-with-signal semantics probe: only dependency reads trigger recomputation.
# This models the current code shape: active changes inputValue, which triggers setDraft, without reading draft.
active = 1
initial_value = 1
draft = initial_value
deps=[active, initial_value]
def set_active(x):
global active, initial_value
active=x
initial_value=x
deps[0]=x
deps[1]=x
draft=deps[1] # createEffect only reads props.initialValue
return { 'active': active, 'initialValue': initial_value, 'draft': draft}
print("initial=", draft, set_active(2)) # effect-only assignment
print("input_then_external=", draft)
PYRepository: TanStack/table
Length of output: 8397
Stop resetting the formula bar draft from initialValue.
activeValue() re-reads the active cell value, so undoes, pastes, cuts, or any other change to that cell triggers createEffect(() => setDraft(props.initialValue())) while the user is typing. Keep the draft independent from initialValue until commit/cancel.
🤖 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/solid/spreadsheet/src/Spreadsheet.tsx` around lines 574 - 575,
Remove the createEffect that resets draft from props.initialValue() near the
draft signal declaration, leaving draft initialized from initialValue only once.
Preserve draft independently while editing, and update it only through the
existing commit or cancel flows.
| function useCallback<TArgs extends Array<unknown>, TResult>( | ||
| callback: (...args: TArgs) => TResult, | ||
| _dependencies?: Array<unknown>, | ||
| ): (...args: TArgs) => TResult { | ||
| return callback | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the useCallback shim and the dependency arrays.
This shim returns the callback unchanged. It provides no memoization in Solid. It also causes real side effects at component setup: the dependency arrays are ordinary expressions, so activeSheetId(), history.rows(), spreadsheetData(), sheets(), and sheets().length are all evaluated once during setup and then discarded. See Line 186, Line 201, and Line 225.
The shim is misleading in a Solid example, because it copies a React idiom that Solid does not need. Solid component bodies run once, so plain function declarations are already stable.
♻️ Proposed refactor
-function useCallback<TArgs extends Array<unknown>, TResult>(
- callback: (...args: TArgs) => TResult,
- _dependencies?: Array<unknown>,
-): (...args: TArgs) => TResult {
- return callback
-}Then convert each call site to a plain function. For example:
- const persistActiveSheet = useCallback(
- (currentSheets: Array<WorkbookSheet>) =>
- currentSheets.map((sheet) =>
- sheet.id === activeSheetId()
- ? {
- ...sheet,
- data: { ...spreadsheetData(), rows: history.rows() },
- }
- : sheet,
- ),
- [activeSheetId(), history.rows(), spreadsheetData()],
- )
+ const persistActiveSheet = (currentSheets: Array<WorkbookSheet>) =>
+ currentSheets.map((sheet) =>
+ sheet.id === activeSheetId()
+ ? {
+ ...sheet,
+ data: { ...spreadsheetData(), rows: history.rows() },
+ }
+ : sheet,
+ )🤖 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/solid/spreadsheet/src/Spreadsheet.tsx` around lines 647 - 652,
Remove the local useCallback shim and its dependency-array arguments throughout
the Spreadsheet component. Convert each affected callback at the identified call
sites near lines 186, 201, and 225 to plain function declarations or equivalent
stable functions, preserving their existing callback behavior while preventing
setup-time evaluation of reactive expressions such as activeSheetId(),
history.rows(), spreadsheetData(), sheets(), and sheets().length.
| 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 | 🟠 Major | ⚡ Quick win
Guard the row and column lookups before extending the selection.
Line 386 and Line 387 index into arrays without a guard, and Line 388 dereferences the results twice. Three lookups can return undefined:
getRowsInDisplayOrder()[coordinate.rowIndex]getDisplayColumns()[coordinate.columnIndex]row.getAllCellsByColumnId()[column.id]
runEdgeScroll calls updateDragTarget on every animation frame with a synthetic event while scrollTop and scrollLeft change (Line 420 to Line 425). During that window the virtual item set and the display order change, so a stale index can fall out of range. The result is a TypeError on mousemove, which aborts the drag.
The header-drag path already guards its lookup at Line 381. Apply the same guard here.
🛡️ 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/solid/spreadsheet/src/SpreadsheetGrid.tsx` around lines 385 - 389,
Update the cell-selection branch in updateDragTarget to guard the row, column,
and cell lookups before calling getSelectionExtendHandler. Return early when any
lookup is undefined, matching the existing header-drag guard, so stale
virtualized indices cannot cause a TypeError during runEdgeScroll.
| {virtualRows().map((virtualRow) => { | ||
| const row = centerRows()[virtualRow.index] | ||
| return ( | ||
| <SubscribedRow | ||
| row={row} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded virtual-index lookups in three render paths. Each site indexes a separately derived array with an index from a virtualizer, then passes the result to a child that dereferences it immediately. The shared root cause is that the virtual item list and the row, header, or cell array are separate reactive sources, so they can be inconsistent for one render. That happens when the data set changes size, for example when the user switches to the 10,000-row stress grid. The result is a TypeError during render.
examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L611-L615: guardcenterRows()[virtualRow.index]before renderingSubscribedRow, becauseSpreadsheetRowViewcallsrow.getDisplayIndex()at Line 888.examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L710-L712: guardcenterHeaders[virtualColumn.index]before renderingHeaderCell, because it destructuresheader.columnat Line 759.examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L951-L953: guardcenterCells[virtualColumn.index]before renderingSpreadsheetCell, because it readscell.column.idat Line 1010.
A <Show when={...}> wrapper, or an early return null, covers all three sites.
📍 Affects 1 file
examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L611-L615(this comment)examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L710-L712examples/solid/spreadsheet/src/SpreadsheetGrid.tsx#L951-L953
🤖 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/solid/spreadsheet/src/SpreadsheetGrid.tsx` around lines 611 - 615,
Guard the virtual-index lookups before rendering their children: at
examples/solid/spreadsheet/src/SpreadsheetGrid.tsx lines 611-615, only render
SubscribedRow when centerRows()[virtualRow.index] exists; at lines 710-712, only
render HeaderCell when centerHeaders[virtualColumn.index] exists; and at lines
951-953, only render SpreadsheetCell when centerCells[virtualColumn.index]
exists. Use a Show wrapper or early null return at each site so inconsistent
reactive sources cannot pass undefined to child components.
| const localX = Math.min(Math.max(clientX - rect.left, ROW_HEADER_WIDTH + 1), rect.width - 1) | ||
| const localY = Math.min(Math.max(clientY - rect.top, HEADER_HEIGHT + 1), rect.height - 1) | ||
| const hit = document.elementFromPoint(clientX, clientY) | ||
| const hitRowIndex = Number(hit?.closest<HTMLElement>('[data-row-index]')?.dataset.rowIndex) | ||
| let row: SpreadsheetTableRow | undefined = Number.isFinite(hitRowIndex) ? table.getRowsInDisplayOrder()[hitRowIndex] : undefined | ||
| if (!row && topRows.length && localY < HEADER_HEIGHT + frozenRowsHeight) row = topRows[Math.min(topRows.length - 1, Math.max(0, Math.floor((localY - HEADER_HEIGHT) / ROW_HEIGHT)))] | ||
| if (!row) { const item = get(rowVirtualizer).getVirtualItemForOffset(scrollRef.scrollTop + localY); row = item ? centerRows[item.index] : undefined } | ||
| const hitColumnId = hit?.closest<HTMLElement>('[data-column-id]')?.dataset.columnId | ||
| let column = hitColumnId ? displayColumns.find((candidate) => candidate.id === hitColumnId) : undefined | ||
| if (!column && startColumns.length && localX < ROW_HEADER_WIDTH + startWidth) { let offset = ROW_HEADER_WIDTH; column = startColumns.find((candidate) => { const next = offset + candidate.getSize(); const match = localX >= offset && localX < next; offset = next; return match }) } | ||
| if (!column && endColumns.length && localX > rect.width - endWidth) { let offset = rect.width - endWidth; column = endColumns.find((candidate) => { const next = offset + candidate.getSize(); const match = localX >= offset && localX < next; offset = next; return match }) } | ||
| if (!column) { const item = get(columnVirtualizer).getVirtualItemForOffset(scrollRef.scrollLeft + localX); column = item ? centerColumns[item.index] : undefined } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The coordinate fallback ignores the CSS zoom scale.
Line 162 applies style:zoom={zoom / 100} to .spreadsheet-canvas. Lines 96 and 97 compute localX and localY from getBoundingClientRect() on scrollRef, so both are visual pixels. Lines 102 and 107 add those visual pixels to scrollRef.scrollTop and scrollRef.scrollLeft and pass the sum to getVirtualItemForOffset, which expects unzoomed content units. Lines 105 and 106 compare localX against column.getSize(), which is also an unzoomed value.
At any zoom other than 100 the fallback resolves the wrong row or column. The elementFromPoint path at line 98 hides this in most cases, so the zoom end-to-end test still passes. The fallback runs whenever the hit element carries no data-row-index or data-column-id, for example during edge scroll.
Divide the local offsets by the zoom scale before using them for content-space math.
🐛 Sketch of the scale correction
function resolveCoordinate(clientX: number, clientY: number): GridCoordinate | null {
if (!scrollRef) return null
const rect = scrollRef.getBoundingClientRect()
- const localX = Math.min(Math.max(clientX - rect.left, ROW_HEADER_WIDTH + 1), rect.width - 1)
- const localY = Math.min(Math.max(clientY - rect.top, HEADER_HEIGHT + 1), rect.height - 1)
+ const scale = zoom / 100
+ const localX = Math.min(Math.max((clientX - rect.left) / scale, ROW_HEADER_WIDTH + 1), rect.width / scale - 1)
+ const localY = Math.min(Math.max((clientY - rect.top) / scale, HEADER_HEIGHT + 1), rect.height / scale - 1)Apply the same scale correction to the rect.width - endWidth comparison at line 106 and to the rect edges used by edgeDelta at lines 132 and 133.
🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` around lines 96 -
107, Update the coordinate fallback around localX/localY so visual-pixel offsets
are converted to unzoomed content units using the canvas zoom scale before
row/column size comparisons and virtualizer offsets. Apply the same scale
conversion to the rect width/end-column boundary comparison and the rect edges
used by edgeDelta. Preserve elementFromPoint hit detection while ensuring all
fallback geometry uses consistent content-space coordinates at any zoom.
| <div bind:this={scrollRef} class="spreadsheet-grid" data-testid="spreadsheet-grid" role="grid" tabindex="0" aria-rowcount={table.getRowsInDisplayOrder().length} aria-colcount={displayColumns.length} onkeydown={interactions.handleGridTextEntry} oncopy={interactions.copySelection} oncut={interactions.cutSelection} onpaste={interactions.pasteSelection} onmousedown={(event) => { if (event.target === event.currentTarget) event.currentTarget.focus({ preventScroll: true }) }} {@attach gridKeys}> | ||
| <div class="spreadsheet-canvas" data-zoom={zoom} style:width={`${Math.max($columnVirtualizer.getTotalSize(), 720)}px`} style:height={`${Math.max($rowVirtualizer.getTotalSize(), 320)}px`} style:zoom={zoom / 100}> | ||
| <div class="spreadsheet-row spreadsheet-header-row" role="row" style:height={`${HEADER_HEIGHT}px`}> | ||
| <button type="button" class="corner-header" aria-label="Select all cells" onclick={() => table.selectAllCells()}><span></span></button> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The header buttons break the role="grid" child structure.
Line 163 and line 201 declare role="row". Line 164 places a <button class="corner-header"> inside the header row, and line 202 places a <button class="row-header"> inside each data row. The ARIA grid pattern allows only columnheader, rowheader, and gridcell as row children. A plain button child makes assistive technology report an inconsistent column count for those rows.
Add role="rowheader" to the row header button and role="columnheader" to the corner header button. The elements stay focusable and keep their accessible names, so the existing end-to-end selectors that query by role need updating in examples/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts lines 159-194 and 632.
Also applies to: 202-202
🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` at line 164, Add
role="columnheader" to the corner-header button and role="rowheader" to each
row-header button in SpreadsheetGrid.svelte, preserving their labels and
focusability. Update the affected end-to-end selectors in spreadsheet.spec.ts to
query the new ARIA roles while retaining the existing selection behavior.
| {@const fillTarget = fillPreview && rowIndex >= fillPreview.destination.minRowIndex && rowIndex <= fillPreview.destination.maxRowIndex && columnIndex >= fillPreview.destination.minColumnIndex && columnIndex <= fillPreview.destination.maxColumnIndex} | ||
| {@const showFill = bound && rowIndex === bound.maxRowIndex && columnIndex === bound.maxColumnIndex} | ||
| <div class="spreadsheet-cell" class:cell-pinned={pinned} class:cell-selected={cell.getIsSelected()} class:cell-focused={cell.getIsFocused()} class:cell-edge-top={edges.top} class:cell-edge-right={edges.right} class:cell-edge-bottom={edges.bottom} class:cell-edge-left={edges.left} class:cell-fill-preview={fillTarget} class:cell-field-header={cell.row.original.kind === 'field-header'} role="gridcell" aria-colindex={columnIndex + 1} aria-selected={cell.getIsSelected()} data-sheet-cell data-row-id={cell.row.id} data-column-id={cell.column.id} tabindex={isEditing ? -1 : cell.getTabIndex()} style={columnStyle(cell.column, left, pinned)} onmousedown={(event) => { if (isEditing || event.button !== 0) return; event.currentTarget.closest<HTMLElement>('.spreadsheet-grid')?.focus({ preventScroll: true }); cell.getSelectionStartHandler(document)(event) }} onmouseenter={cell.getSelectionExtendHandler()} ondblclick={() => interactions.startEditing(cell.row.id, cell.column.id)} oncontextmenu={(event) => { event.preventDefault(); event.currentTarget.closest<HTMLElement>('.spreadsheet-grid')?.focus({ preventScroll: true }); openMenu = null; openCellMenu = { x: event.clientX, y: event.clientY, column: cell.column } }}> | ||
| {#if isEditing}<input autofocus class="cell-editor" aria-label={`Edit ${cell.column.columnDef.meta?.letter}${rowIndex + 1}`} value={editing?.draft ?? ''} onfocus={(event) => event.currentTarget.select()} onmousedown={(event) => event.stopPropagation()} oninput={(event) => interactions.setEditingDraft(event.currentTarget.value)} onkeydown={interactions.handleEditorKeyDown} onblur={() => interactions.commitEditing()} />{:else}<span class="cell-value">{formatted(cell.getValue())}</span>{/if}{#if showFill}<span class="fill-handle" data-testid="fill-handle" aria-label="Drag to fill" onmousedown={(event) => startFill(event, { minRowIndex: bound.minRowIndex, maxRowIndex: bound.maxRowIndex, minColumnIndex: bound.minColumnIndex, maxColumnIndex: bound.maxColumnIndex })}></span>{/if} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Give the fill handle an interactive role, or hide it from assistive technology.
The fill-handle span carries aria-label="Drag to fill" but has no role and no tabindex. Assistive technology announces a label on a non-interactive element, and keyboard users cannot activate it.
If fill stays mouse-only, set aria-hidden="true" and drop the label. If fill must be operable, add role="button" and tabindex="0".
🛡️ Minimal fix for the mouse-only case
-{`#if` showFill}<span class="fill-handle" data-testid="fill-handle" aria-label="Drag to fill" onmousedown={(event) => startFill(event, { minRowIndex: bound.minRowIndex, maxRowIndex: bound.maxRowIndex, minColumnIndex: bound.minColumnIndex, maxColumnIndex: bound.maxColumnIndex })}></span>{/if}
+{`#if` showFill}<span class="fill-handle" data-testid="fill-handle" aria-hidden="true" onmousedown={(event) => startFill(event, { minRowIndex: bound.minRowIndex, maxRowIndex: bound.maxRowIndex, minColumnIndex: bound.minColumnIndex, maxColumnIndex: bound.maxColumnIndex })}></span>{/if}📝 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 isEditing}<input autofocus class="cell-editor" aria-label={`Edit ${cell.column.columnDef.meta?.letter}${rowIndex + 1}`} value={editing?.draft ?? ''} onfocus={(event) => event.currentTarget.select()} onmousedown={(event) => event.stopPropagation()} oninput={(event) => interactions.setEditingDraft(event.currentTarget.value)} onkeydown={interactions.handleEditorKeyDown} onblur={() => interactions.commitEditing()} />{:else}<span class="cell-value">{formatted(cell.getValue())}</span>{/if}{#if showFill}<span class="fill-handle" data-testid="fill-handle" aria-label="Drag to fill" onmousedown={(event) => startFill(event, { minRowIndex: bound.minRowIndex, maxRowIndex: bound.maxRowIndex, minColumnIndex: bound.minColumnIndex, maxColumnIndex: bound.maxColumnIndex })}></span>{/if} | |
| {`#if` isEditing}<input autofocus class="cell-editor" aria-label={`Edit ${cell.column.columnDef.meta?.letter}${rowIndex + 1}`} value={editing?.draft ?? ''} onfocus={(event) => event.currentTarget.select()} onmousedown={(event) => event.stopPropagation()} oninput={(event) => interactions.setEditingDraft(event.currentTarget.value)} onkeydown={interactions.handleEditorKeyDown} onblur={() => interactions.commitEditing()} />{:else}<span class="cell-value">{formatted(cell.getValue())}</span>{/if}{`#if` showFill}<span class="fill-handle" data-testid="fill-handle" aria-hidden="true" onmousedown={(event) => startFill(event, { minRowIndex: bound.minRowIndex, maxRowIndex: bound.maxRowIndex, minColumnIndex: bound.minColumnIndex, maxColumnIndex: bound.maxColumnIndex })}></span>{/if} |
🤖 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/svelte/spreadsheet/src/SpreadsheetGrid.svelte` at line 218, Update
the fill-handle span rendered by the showFill branch to reflect its mouse-only
behavior: remove its aria-label and add aria-hidden="true" so assistive
technology ignores it, preserving the existing startFill mouse interaction.
| return (): RowModel<TFeatures, TData> => { | ||
| const model = getBaseRowModel() | ||
| const headerIndex = model.rows.findIndex( | ||
| (row) => row.original.kind === 'field-header', | ||
| ) | ||
| if (headerIndex <= 0) return model | ||
|
|
||
| const header = model.rows[headerIndex] | ||
| const rows = [ | ||
| header, | ||
| ...model.rows.slice(0, headerIndex), | ||
| ...model.rows.slice(headerIndex + 1), | ||
| ] | ||
| const flatHeaderIndex = model.flatRows.indexOf(header) | ||
| const flatRows = | ||
| flatHeaderIndex <= 0 | ||
| ? model.flatRows | ||
| : [ | ||
| header, | ||
| ...model.flatRows.slice(0, flatHeaderIndex), | ||
| ...model.flatRows.slice(flatHeaderIndex + 1), | ||
| ] | ||
|
|
||
| return { ...model, rows, flatRows } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the Svelte and Solid spreadsheet sorted row model wrappers and inspect base memoization.
set -euo pipefail
fd -t f 'spreadsheetTable.ts' examples | while IFS= read -r f; do
echo "===== $f"
cat -n "$f"
done
echo "===== createSortedRowModel definition"
rg -n -C 10 'export function createSortedRowModel|const createSortedRowModel' --glob '!**/node_modules/**' packages || trueRepository: TanStack/table
Length of output: 13608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== spreadsheetTable.ts files"
for f in examples/spreadsheet/src/spreadsheetTable.ts examples/spreadsheet/*/src/spreadsheetTable.ts examples/react/spreadsheet/src/spreadsheetTable.ts examples/solid/spreadsheet/src/spreadsheetTable.ts examples/svelte/spreadsheet/src/spreadsheetTable.ts; do
[ -f "$f" ] || continue
echo "--- $f"
cat -n "$f"
done
echo "===== createSortedRowModel implementation"
cat -n packages/table-core/src/features/row-sorting/createSortedRowModel.ts
echo "===== getBaseRowModel call sites / row model pipeline"
rg -n -C 3 'getBaseRowModel|getSortedRowModel|getFilteredRowModel|getPinnedRowModel|createMemo|tableMemo|getRowsInDisplayOrder' packages examples --glob '!**/node_modules/**' || trueRepository: TanStack/table
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== target spreadsheetTable.ts files"
for f in examples/spreadsheet/src/spreadsheetTable.ts examples/react/spreadsheet/src/spreadsheetTable.ts examples/solid/spreadsheet/src/spreadsheetTable.ts examples/svelte/spreadsheet/src/spreadsheetTable.ts; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f"
fi
done
echo "===== tableMemo and createSortedRowModel implementations"
sed -n '200,260p' packages/table-core/src/utils.ts
sed -n '1,90p' packages/table-core/src/features/row-sorting/createSortedRowModel.ts
echo "===== spreadsheet display-order call count"
python3 - <<'PY'
import pathlib, re
for p in pathlib.Path('examples').rglob('*'):
if p.name not in {'SpreadsheetGrid.svelte','SpreadsheetGrid.tsx','Spreadsheet.tsx','Table.tsx','App.vue'}:
continue
text = p.read_text(errors='ignore')
if 'getRowsInDisplayOrder' in text:
line_count = text.count('getRowsInDisplayOrder')
# count call syntax occurrences without imports/comments context
occurrences = len(re.findall(r'\.getRowsInDisplayOrder\s*\(', text))
print(f'{p}: getRowsInDisplayOrder text count={line_count} call occurrences={occurrences}')
for i, line in enumerate(text.splitlines(), 1):
if 'getRowsInDisplayOrder' in line:
print(f' line {i}: {line.strip()}')
PY
echo "===== STRESS_ROW_COUNT"
rg -n 'STRESS_ROW_COUNT|spreadsheetFeatures|createSpreadsheetSortedRowModel' examples/spreadsheet examples/react/spreadsheet examples/solid/spreadsheet examples/svelte/spreadsheet --glob '!**/node_modules/**'Repository: TanStack/table
Length of output: 21773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== getRowsInDisplayOrder definition"
rg -n -C 20 'function table_getRowsInDisplayOrder|table_getRowsInDisplayOrder|getRowsInDisplayOrder' packages/table-core/src --glob '!**/node_modules/**' || true
echo "===== base row memo tests for sorting-related cases"
rg -n -C 4 'manualSorting|getPreSortedRowModel|getSortedRowModel|sortedRowModelsFeature|rowSortingFeature' packages/table-core/tests/unit/core/row-models packages/table-core/tests/unit/core/table --glob '!**/node_modules/**' || trueRepository: TanStack/table
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== specific getRowsInDisplayOrder definitions"
rg -n -C 25 'export function table_getRowsInDisplayOrder|table_getRowsInDisplayOrder|function table_getRowsInDisplayOrder' packages/table-core/src/core/rows/coreRowsFeature.utils.ts || true
echo "===== rowModelSlots relevant tests"
rg -n -C 6 'getRowsInDisplayOrder\s*\(' packages/table-core/tests/unit/core/table/rowModelSlots.test.ts || true
echo "===== row memo tests around display order"
rg -n -C 5 'getRowsInDisplayOrder' packages/table-core/tests/unit/core/row-models --glob '!**/node_modules/**' || trueRepository: TanStack/table
Length of output: 3291
Memoize the reordered spreadsheet row model.
createSpreadsheetSortedRowModel() creates a new array for rows and/or flatRows on every evaluation and returns { ...model, rows, flatRows }. getRowsInDisplayOrder() then resolves through this model, so repeated calls from the spreadsheet grid copy the current row set and update display indexes without referential stability. Cache the selected model against the unchanged base model by id or reference before cloning/copying. Apply this fix in each spreadsheet example wrapper.
🤖 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/svelte/spreadsheet/src/spreadsheetTable.ts` around lines 40 - 64,
Memoize the reordered model in createSpreadsheetSortedRowModel for every
spreadsheet example wrapper. Cache the generated { rows, flatRows } model by the
unchanged getBaseRowModel() result (using reference or model id), return the
cached model on repeated evaluations, and rebuild the reordered arrays only when
the base model changes.
| const columnA = page.getByRole('columnheader', { name: /^A/ }) | ||
| const columnC = page.getByRole('columnheader', { name: /^C/ }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the column header locators unambiguous.
getByRole('columnheader', { name: /^A/ }) matches any header whose accessible name starts with 'A'. The default dataset has 100 columns, so columns AA through AZ also exist and also start with 'A'. The same applies to the loop at line 150 and to /^D/ at line 156.
The locators work today only because virtualization does not render column A and column AA at the same time. A change to the overscan or the initial column widths makes the locator resolve two elements, and Playwright fails with a strict mode violation.
Target the headers by data-column-id, which SpreadsheetGrid.svelte line 190 sets on every column header.
💚 Proposed fix
+function columnHeader(page: Page, columnIndex: number) {
+ return page.locator(`[role="columnheader"][data-column-id="column-${columnIndex}"]`)
+}- const columnA = page.getByRole('columnheader', { name: /^A/ })
- const columnC = page.getByRole('columnheader', { name: /^C/ })
+ const columnA = columnHeader(page, 0)
+ const columnC = columnHeader(page, 2)- for (const letter of ['A', 'B', 'C']) {
- await expect(
- page.getByRole('columnheader', { name: new RegExp(`^${letter}`) }),
- ).toHaveAttribute('aria-selected', 'true')
- }
- await expect(
- page.getByRole('columnheader', { name: /^D/ }),
- ).toHaveAttribute('aria-selected', 'false')
+ for (const columnIndex of [0, 1, 2]) {
+ await expect(columnHeader(page, columnIndex)).toHaveAttribute(
+ 'aria-selected',
+ 'true',
+ )
+ }
+ await expect(columnHeader(page, 3)).toHaveAttribute(
+ 'aria-selected',
+ 'false',
+ )Line 61 and line 423 use the same name-based pattern and benefit from the same change.
Also applies to: 150-157
🤖 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/svelte/spreadsheet/tests/e2e/spreadsheet.spec.ts` around lines 131 -
132, Replace the ambiguous accessible-name columnheader locators throughout the
spreadsheet test, including the declarations near lines 61 and 131, the loop
near line 150, and the D-header lookup near line 156, with locators targeting
each header’s unique data-column-id attribute. Preserve the existing column IDs
and assertions while ensuring every locator resolves to exactly one header.
1 participant
What
API decision
This stays in userland. The example can capture the selection state at drag start, update the active header range through
setCellSelection, and derive header highlighting fromgetCellSelectionBounds. Row/column header interaction policy is renderer-specific, so no additional TanStack Table API is needed.Verification
pnpm test— all 839 tasks passedgit diff --check— passedSummary by CodeRabbit