feat: rowSpan and colSpan for cells - #6487
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 1160ab5
☁️ Nx Cloud last updated this comment at |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds ChangesCell spanning core
Framework examples and spreadsheet integration
Documentation and metadata
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts-817-827 (1)
817-827: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe stickiness poll is vacuous, and the real assertion does not wait.
The poll at Lines 822-824 asserts
x < before.x + 1.5. A merged cell that fails to stick scrolls left, so itsxdecreases and the poll still passes. The assertion that actually checks stickiness is Line 825, and it runs without retry, so it can flake before the scroll settles. Poll on the absolute difference instead.💚 Proposed fix
const before = (await merged.boundingBox())! await page.getByTestId('spreadsheet-grid').evaluate((element) => { element.scrollLeft = 400 element.dispatchEvent(new Event('scroll')) }) await expect - .poll(async () => (await merged.boundingBox())!.x) - .toBeLessThan(before.x + 1.5) - expect(Math.abs((await merged.boundingBox())!.x - before.x)).toBeLessThan( - 1.5, - ) + .poll(async () => Math.abs((await merged.boundingBox())!.x - before.x)) + .toBeLessThan(1.5)🤖 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 817 - 827, Update the scroll assertion around the merged cell in the spreadsheet stickiness test to poll on the absolute difference between its current x position and before.x, rather than only checking that x is less than a threshold. Move the LessThan(1.5) expectation into the poll callback so the actual stickiness condition waits for scrolling to settle, and remove the separate non-retrying assertion._artifacts/domain_map.yaml-27-27 (1)
27-27: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSynchronize the stock-feature count across metadata.
This line now documents 17 stock features, but
_artifacts/skill_tree.yamlLine 121 still documents all 16 feature registrations. Update the stale count so migration and routing metadata describe the same feature set.🤖 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 `@_artifacts/domain_map.yaml` at line 27, Update the stale stock-feature count in the corresponding metadata entry in skill_tree.yaml from 16 to 17, keeping the feature-registration description otherwise unchanged and synchronized with the domain_map.yaml entry.docs/framework/solid/guide/cell-spanning.md-81-81 (1)
81-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
getIsCovered()description.
getIsCovered()is true when either span is0. Use!cell.getIsCovered()to render the cell. The current sentence says thatgetIsCovered()checks that both spans are non-zero, which reverses the API semantics.Suggested wording
-`cell.getIsCovered()` is a convenience for checking that `cell.getRowSpan()` and `cell.getColSpan()` are both non-zero, so read the span numbers directly when you need them separately. +`cell.getIsCovered()` is true when either span is `0`. Use `!cell.getIsCovered()` to render the cell; read the span numbers directly when you need them separately.🤖 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 `@docs/framework/solid/guide/cell-spanning.md` at line 81, Correct the getIsCovered() description in the cell-spanning guide to state that it is true when either getRowSpan() or getColSpan() is 0. Instruct readers to use !cell.getIsCovered() when rendering the cell, while preserving the guidance to read span values directly when needed separately.docs/framework/alpine/guide/cell-spanning.md-13-13 (1)
13-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the hyphenated form
cell-spanningin all setup sentences.The same compound modifier is missing hyphens in three framework guides.
docs/framework/alpine/guide/cell-spanning.md#L13-L13: change “cell spanning features” and “cell spanning feature” to “cell-spanning features” and “cell-spanning feature”.docs/framework/angular/guide/cell-spanning.md#L13-L13: change “cell spanning features” and “cell spanning feature” to “cell-spanning features” and “cell-spanning feature”.docs/framework/ember/guide/cell-spanning.md#L13-L13: change “cell spanning features” and “cell spanning feature” to “cell-spanning features” and “cell-spanning feature”.Static analysis flagged the missing hyphens in these setup sentences.
🤖 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 `@docs/framework/alpine/guide/cell-spanning.md` at line 13, Update the setup sentence in docs/framework/alpine/guide/cell-spanning.md:13-13, docs/framework/angular/guide/cell-spanning.md:13-13, and docs/framework/ember/guide/cell-spanning.md:13-13 to use “cell-spanning features” and “cell-spanning feature” consistently.Source: Linters/SAST tools
docs/framework/react/guide/cell-spanning.md-64-78 (1)
64-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeclare
FlexRenderfor the Lit example.
docs/framework/lit/guide/cell-spanning.mdcallsFlexRender({ cell })inside a self-contained HTML block, but only importshtml,nothing, andrepeat. AddFlexRenderto the import from@tanstack/lit-tableso the snippet is copy-paste valid.🤖 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 `@docs/framework/react/guide/cell-spanning.md` around lines 64 - 78, Update the Lit example in docs/framework/lit/guide/cell-spanning.md around lines 72-95 to import FlexRender from `@tanstack/lit-table` alongside the existing imports, so the self-contained FlexRender({ cell }) usage is copy-paste valid. The React example in docs/framework/react/guide/cell-spanning.md around lines 64-78 requires no direct change.examples/alpine/cell-spanning/src/index.css-407-419 (1)
407-419: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stylelint errors in both copies of the example stylesheet. The two files are identical, so one shared template carries the same four violations into both. Stylelint 17.14.1 reports
declaration-empty-line-beforefor thebox-shadowdeclaration that follows the custom properties, andvalue-keyword-casefor threecurrentColorvalues.
examples/alpine/cell-spanning/src/index.css#L407-L419: add a blank line before thebox-shadowdeclaration in.cell-selectable, and lowercasecurrentColortocurrentcoloron lines 135, 216, and 321.examples/svelte/cell-spanning/src/index.css#L407-L419: apply the same two changes at the same lines.Confirm first that these example paths are inside the stylelint scope for CI.
🎨 Proposed fix for the reported declarations
.cell-selectable { user-select: none; cursor: cell; --cell-edge-top: 0 0 0 0 transparent; --cell-edge-right: 0 0 0 0 transparent; --cell-edge-bottom: 0 0 0 0 transparent; --cell-edge-left: 0 0 0 0 transparent; + box-shadow: inset var(--cell-edge-top),- border: 1px solid currentColor; + border: 1px solid currentcolor;🤖 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/alpine/cell-spanning/src/index.css` around lines 407 - 419, Confirm both example stylesheets are included in CI’s stylelint scope. In examples/alpine/cell-spanning/src/index.css (lines 407-419) and examples/svelte/cell-spanning/src/index.css (lines 407-419), add a blank line before the .cell-selectable box-shadow declaration and lowercase the three currentColor values at lines 135, 216, and 321 to currentcolor.Source: Linters/SAST tools
examples/vue/cell-spanning/index.html-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the scaffold title and confirm the favicon exists.
Line 7 keeps the Vite default
Vite App. The other examples name themselves, for exampleexamples/ember/cell-spanning/index.htmlLine 5 usesTanStack Ember Table - Cell Spanning. Line 5 also references/favicon.ico; if this example ships no favicon, the browser logs a 404.✏️ Proposed fix
- <link rel="icon" href="/favicon.ico" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" /> - <title>Vite App</title> + <title>TanStack Vue Table - Cell Spanning</title>🤖 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/vue/cell-spanning/index.html` around lines 5 - 7, Update the page title in the HTML head from the Vite scaffold value to the example’s descriptive TanStack Vue Table cell-spanning title, and verify that the referenced /favicon.ico asset exists for this example; add or correct the favicon asset/reference if it is missing.examples/angular/cell-spanning/src/styles.css-407-419 (1)
407-419: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStylelint fails on three copies of the same stylesheet. The three example stylesheets are identical copies of one template. The template carries two violations: uppercase
currentColor(value-keyword-case) and a missing blank line beforebox-shadow(declaration-empty-line-before). Fix the template, then re-copy it to each example.
examples/angular/cell-spanning/src/styles.css#L407-L419: add a blank line beforebox-shadow, and lowercasecurrentcoloron lines 135, 216, and 321.examples/ember/cell-spanning/app/app.css#L407-L419: apply the same two fixes at the same lines.examples/vue/cell-spanning/src/index.css#L407-L419: apply the same two fixes at the same lines.🤖 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/angular/cell-spanning/src/styles.css` around lines 407 - 419, Update the shared cell-spanning stylesheet template by inserting a blank line before the .cell-selectable box-shadow declaration and changing currentColor to lowercase currentcolor at the specified occurrences. Re-copy the corrected template so the same fixes are applied in examples/angular/cell-spanning/src/styles.css#L407-L419, examples/ember/cell-spanning/app/app.css#L407-L419, and examples/vue/cell-spanning/src/index.css#L407-L419.Source: Linters/SAST tools
examples/angular/cell-spanning/tests/e2e/smoke.spec.ts-456-470 (1)
456-470: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for the toggle to take effect before you read the report.
This test reads the DOM immediately after
uncheck(). The other tests in this file wait withexpect.pollbefore they read state. If the Angular change detection cycle has not flushed the span removal,readSpanReportcan observe the previous render and the assertion onrowSpanningCellsbecomes flaky.Add a stabilizing assertion first, for example poll on the rendered cell count or on a
data-testidthat reflects the spanning flag.🧪 Proposed stabilization
await page.getByLabel('Row spanning', { exact: true }).uncheck() + await expect + .poll(async () => (await readSpanReport(spanTable(page))).rowSpanningCells) + .toBe(0) + const columnCount = await visibleLeafCount(page)🤖 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/angular/cell-spanning/tests/e2e/smoke.spec.ts` around lines 456 - 470, In the test “renders one cell per column when spanning is turned off,” add an expect.poll stabilization after unchecking the “Row spanning” toggle and before calling readSpanReport, waiting for the rendered cell count or another spanning-state indicator to reflect the disabled state. Keep the existing span and grid assertions unchanged.examples/solid/cell-spanning/src/index.css-135-135 (1)
135-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
currentColorcasing to satisfy Stylelint.Stylelint flags
currentColorat Line 135, Line 216, and Line 321. Usecurrentcolorfor consistent keyword casing.🎨 Proposed fix for keyword casing
.demo-button, .pin-button, .compact-input, .filter-input, .filter-select, .page-size-input, .text-input, .number-input, .wide-action-button, .primary-action, .secondary-action, .success-action { - border: 1px solid currentColor; + border: 1px solid currentcolor; border-radius: 0.25rem; }.summary-panel { - border: 1px solid currentColor; + border: 1px solid currentcolor; box-shadow: 0 1px 3px rgb(0 0 0 / 0.2); padding: 0.5rem; }.column-size-input { width: 6rem; margin-left: 0.5rem; - border: 1px solid currentColor; + border: 1px solid currentcolor; border-radius: 0.25rem; padding: 0.25rem; }Also applies to: 216-216, 321-321
🤖 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/cell-spanning/src/index.css` at line 135, Update the border declarations at the affected styles in the cell-spanning stylesheet to use the lowercase CSS keyword currentcolor instead of currentColor, including all three flagged occurrences.Source: Linters/SAST tools
examples/solid/cell-spanning/src/index.css-407-419 (1)
407-419: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a blank line before the
box-shadowdeclaration.Stylelint flags the missing blank line before the
box-shadowdeclaration at Line 414 (declaration-empty-line-before). Add a blank line to separate the custom property declarations frombox-shadow.🎨 Proposed fix for the missing blank line
--cell-edge-left: 0 0 0 0 transparent; + box-shadow: inset var(--cell-edge-top),🤖 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/cell-spanning/src/index.css` around lines 407 - 419, In the .cell-selectable rule, add a blank line between the custom property declarations and the box-shadow declaration to satisfy the declaration-empty-line-before style rule.Source: Linters/SAST tools
examples/octane/cell-spanning/src/index.css-135-135 (1)
135-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the Stylelint errors.
Stylelint reports two rule violations in this file. Lowercase the keyword to
currentcolorat lines 135, 216, and 321. Add an empty line before thebox-shadowdeclaration at line 414.♻️ Proposed fixes
- border: 1px solid currentColor; + border: 1px solid currentcolor;--cell-edge-left: 0 0 0 0 transparent; + box-shadow: inset var(--cell-edge-top),Also applies to: 216-216, 321-321, 414-418
🤖 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/octane/cell-spanning/src/index.css` at line 135, Update the CSS declarations at the referenced borders to use the lowercase `currentcolor` keyword, and add an empty line immediately before the `box-shadow` declaration in the affected rule. Keep all other styling unchanged.Source: Linters/SAST tools
examples/octane/cell-spanning/index.html-7-7 (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the page title.
This is the Octane cell-spanning example, but the title names Preact.
📝 Proposed fix
- <title>Vite + Preact</title> + <title>Vite + Octane</title>🤖 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/octane/cell-spanning/index.html` at line 7, Update the document title in the page head from “Vite + Preact” to identify the Octane cell-spanning example, leaving the surrounding HTML unchanged.examples/preact/cell-spanning/src/index.css-135-135 (1)
135-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
currentColorcasing to satisfy Stylelint.Stylelint flags
currentColorat Line 135, Line 216, and Line 321. Usecurrentcolorinstead.🎨 Proposed fix
.demo-button, .pin-button, .compact-input, .filter-input, .filter-select, .page-size-input, .text-input, .number-input, .wide-action-button, .primary-action, .secondary-action, .success-action { - border: 1px solid currentColor; + border: 1px solid currentcolor; border-radius: 0.25rem; }Apply the same change at Line 216 (
.summary-panel) and Line 321 (.column-size-input).Also applies to: 216-216, 321-321
🤖 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/preact/cell-spanning/src/index.css` at line 135, Update the CSS border declarations in the relevant cell-spanning styles, including the rules for .summary-panel and .column-size-input, to use the lowercase currentcolor keyword instead of currentColor so Stylelint passes.Source: Linters/SAST tools
examples/preact/cell-spanning/src/index.css-407-419 (1)
407-419: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank line before the
box-shadowdeclaration.Stylelint flags
declaration-empty-line-beforeat Line 414. Add a blank line beforebox-shadowto satisfy the rule.🎨 Proposed fix
--cell-edge-left: 0 0 0 0 transparent; + box-shadow: inset var(--cell-edge-top),🤖 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/preact/cell-spanning/src/index.css` around lines 407 - 419, Add a blank line in the .cell-selectable rule between the --cell-edge-left custom property and the box-shadow declaration to satisfy the declaration-empty-line-before style rule.Source: Linters/SAST tools
examples/react/cell-spanning/src/index.css-135-135 (1)
135-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the reported Stylelint violations.
Use
currentcolorat Lines 135, 216, and 321. Add the required empty line beforebox-shadowat Lines 414-418. These locations fail the configured Stylelint rules.Proposed fix
- border: 1px solid currentColor; + border: 1px solid currentcolor; ... - border: 1px solid currentColor; + border: 1px solid currentcolor; ... - border: 1px solid currentColor; + border: 1px solid currentcolor; ... --cell-edge-left: 0 0 0 0 transparent; + box-shadow:Also applies to: 216-216, 321-321, 414-418
🤖 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/cell-spanning/src/index.css` at line 135, Update the affected CSS declarations in the stylesheet: change the `currentColor` values at the rules around lines 135, 216, and 321 to lowercase `currentcolor`, and add the required empty line before the `box-shadow` declaration in the rule around lines 414–418 to satisfy the configured Stylelint rules.Source: Linters/SAST tools
examples/react/cell-spanning/vite.config.js-12-18 (1)
12-18: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winDon’t force dev-time values into production builds.
examples/react/cell-spanning/vite.config.jsrunsrollupReplaceregardless of command, sovite buildkeeps__DEV__andprocess.env.NODE_ENVset to dev values.examples/react/cell-spanning/src/main.tsxalso setsdebugTable: truefor both tables, so production bundles retain debug logging.
examples/react/cell-spanning/vite.config.js#L12-L18: compute replacements fromcommand === 'serve'ormode.examples/react/cell-spanning/src/main.tsx#L149,L163: enabledebugTableonly in dev or remove it.🤖 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/cell-spanning/vite.config.js` around lines 12 - 18, Update examples/react/cell-spanning/vite.config.js lines 12-18 to derive __DEV__ and process.env.NODE_ENV replacement values from the Vite command or mode, using development values only for serve/dev builds and production values for builds. Update examples/react/cell-spanning/src/main.tsx lines 149-163 so both tables enable debugTable only in development, or remove the option entirely.packages/table-core/tests/unit/features/cell-spanning/cellSpanningFeature.utils.test.ts-296-312 (1)
296-312: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe test title contradicts its assertion.
The title says "reports spans of one". Line 310 asserts a row span of
3. The assertion is the correct expectation, becausecell_getRowSpanfalls back totable_getCellSpanIndexthroughcallMemoOrStaticFnand still resolves real spans without the feature registered. Rename the test so the intended contract is unambiguous.💚 Proposed rename
- it('reports spans of one and builds an index from the row model', () => { + it('still resolves spans by deriving the index on the fly', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/table-core/tests/unit/features/cell-spanning/cellSpanningFeature.utils.test.ts` around lines 296 - 312, Rename the test case around cell_getRowSpan and table_getCellSpanIndex so its title describes resolving the actual row span from the row model, not “spans of one.” Keep the existing span assertion and index-building behavior unchanged.packages/table-core/tests/implementation/features/cell-spanning/cellSpanningFeature.test.ts-314-328 (1)
314-328: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test does not reorder columns.
The test name claims coverage of row spans across reordering. The features are
cellSpanningFeatureandcolumnVisibilityFeatureonly, and no column order is ever set. The single assertion repeats the default-order expectation already asserted at Line 327's counterparts elsewhere in this file, so a regression in column-order handling would not fail here.columnOrderis a declared memo dep incellSpanningFeature.tsLine 66, so the property is worth exercising.Register
columnOrderingFeatureand reorder the columns before asserting.💚 Proposed test change
it('keeps row spans keyed by column id across reordering', () => { const orderable = testFeatures({ cellSpanningFeature, + columnOrderingFeature, columnVisibilityFeature, }) const table = constructTable<typeof orderable, TestRow>({ features: orderable, data: makeData(), columns: makeColumns(), getRowId: (row) => row.id, renderFallbackValue: '', }) expect(regionSpans(table)).toEqual([3, 0, 0, 3, 0, 0, 3, 0, 0]) + + // `region` now renders last; spans stay keyed by column id. + table.setColumnOrder(['team', 'amount', 'region']) + expect(regionSpans(table)).toEqual([3, 0, 0, 3, 0, 0, 3, 0, 0]) })Add
columnOrderingFeatureto the imports from'../../../../src'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/table-core/tests/implementation/features/cell-spanning/cellSpanningFeature.test.ts` around lines 314 - 328, Update the “keeps row spans keyed by column id across reordering” test to import and register columnOrderingFeature alongside the existing features, then apply a non-default column order to the table before asserting regionSpans(table). Keep the assertion focused on the reordered result and ensure the chosen order exercises column-order handling.packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts-979-997 (1)
979-997: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the snapped landing column against selectable columns.
Lines 971-977 verify that
columns[nextColumnIndex]is selectable. The merge snap at Lines 988-991 then replaces the column withlandingMerge.minColumnIndexand no further check runs. If a spanning column setsenableCellSelection: falseon its column definition, or the table predicate excludes it, navigation can focus a non-selectable cell.🐛 Proposed guard
if (landingMerge) { landingRowIndex = landingMerge.minRowIndex - landingColumnIndex = landingMerge.minColumnIndex + const anchorColumn = columns[landingMerge.minColumnIndex] + if (anchorColumn && selectableColumnIds.has(anchorColumn.id)) { + landingColumnIndex = landingMerge.minColumnIndex + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts` around lines 979 - 997, After merge snapping in the navigation logic, revalidate the resolved landing column represented by landingColumnIndex using the same selectable-column predicate already applied to nextColumnIndex. Return null when the snapped column is not selectable, before resolving or returning the landing row and column; preserve the existing behavior for selectable destinations.
🤖 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 @.changeset/cell-spanning-feature.md:
- Line 30: Update the changeset’s selection description to remove the stale
statement that cellSelectionFeature is not span-aware and that span-aware
selection is planned. Document the shipped merge-aware behavior instead,
including that getCellSelectionMergeBounds() ensures selections crossing merged
cells account for their merge bounds.
In `@docs/framework/alpine/guide/cell-spanning.md`:
- Around line 17-21: Update the Alpine setup around the table initialization to
import FlexRender from `@tanstack/alpine-table` and include FlexRender alongside
table in the returned Alpine data object, so the template call FlexRender({ cell
}) resolves correctly.
In `@examples/ember/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 460-469: After unchecking the “Row spanning” control, wait by
polling an observable report signal such as readSpanReport(spanTable(page))
until rowSpanningCells is zero before reading spans, reference, and performing
the grid assertions. Follow the existing wait pattern used by nearby
state-changing tests, keeping the final report snapshot and expectations
unchanged.
In `@examples/octane/cell-spanning/package.json`:
- Around line 13-22: Update the devDependencies entry for
`@tsrx/typescript-plugin` to the published TypeScript 6-compatible version
0.3.107, while leaving the existing `@tsrx/eslint-plugin` versioning and other
dependencies unchanged.
In `@examples/octane/cell-spanning/src/main.tsrx`:
- Line 141: Fix the App function declaration by removing the stray “@”, then
wrap the JSX block beginning around the main JSX expression and ending at the
closing </div> in a return expression so App renders it instead of returning
undefined; reindent the returned JSX consistently with the function body.
In `@examples/react/cell-spanning/index.html`:
- Line 7: Update the react-scan script tag in the cell-spanning example to use a
pinned versioned unpkg URL with a matching Subresource Integrity attribute, or
remove the tag if react-scan is not required; do not leave the mutable remote
URL in the published example.
In
`@packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts`:
- Around line 315-324: Update the horizontal-span logic in
cellSelectionFeature.utils.ts (lines 315-324) to skip only explicit vertical
runs by checking for values greater than 1, preserving 0 as a valid non-span
value. Add coverage in cellSelectionSpanAware.test.ts (lines 41-45) with a
column-spanning definition and assertions for merged-cell bounds, selection
expansion, edges, and count.
---
Minor comments:
In `@_artifacts/domain_map.yaml`:
- Line 27: Update the stale stock-feature count in the corresponding metadata
entry in skill_tree.yaml from 16 to 17, keeping the feature-registration
description otherwise unchanged and synchronized with the domain_map.yaml entry.
In `@docs/framework/alpine/guide/cell-spanning.md`:
- Line 13: Update the setup sentence in
docs/framework/alpine/guide/cell-spanning.md:13-13,
docs/framework/angular/guide/cell-spanning.md:13-13, and
docs/framework/ember/guide/cell-spanning.md:13-13 to use “cell-spanning
features” and “cell-spanning feature” consistently.
In `@docs/framework/react/guide/cell-spanning.md`:
- Around line 64-78: Update the Lit example in
docs/framework/lit/guide/cell-spanning.md around lines 72-95 to import
FlexRender from `@tanstack/lit-table` alongside the existing imports, so the
self-contained FlexRender({ cell }) usage is copy-paste valid. The React example
in docs/framework/react/guide/cell-spanning.md around lines 64-78 requires no
direct change.
In `@docs/framework/solid/guide/cell-spanning.md`:
- Line 81: Correct the getIsCovered() description in the cell-spanning guide to
state that it is true when either getRowSpan() or getColSpan() is 0. Instruct
readers to use !cell.getIsCovered() when rendering the cell, while preserving
the guidance to read span values directly when needed separately.
In `@examples/alpine/cell-spanning/src/index.css`:
- Around line 407-419: Confirm both example stylesheets are included in CI’s
stylelint scope. In examples/alpine/cell-spanning/src/index.css (lines 407-419)
and examples/svelte/cell-spanning/src/index.css (lines 407-419), add a blank
line before the .cell-selectable box-shadow declaration and lowercase the three
currentColor values at lines 135, 216, and 321 to currentcolor.
In `@examples/angular/cell-spanning/src/styles.css`:
- Around line 407-419: Update the shared cell-spanning stylesheet template by
inserting a blank line before the .cell-selectable box-shadow declaration and
changing currentColor to lowercase currentcolor at the specified occurrences.
Re-copy the corrected template so the same fixes are applied in
examples/angular/cell-spanning/src/styles.css#L407-L419,
examples/ember/cell-spanning/app/app.css#L407-L419, and
examples/vue/cell-spanning/src/index.css#L407-L419.
In `@examples/angular/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 456-470: In the test “renders one cell per column when spanning is
turned off,” add an expect.poll stabilization after unchecking the “Row
spanning” toggle and before calling readSpanReport, waiting for the rendered
cell count or another spanning-state indicator to reflect the disabled state.
Keep the existing span and grid assertions unchanged.
In `@examples/octane/cell-spanning/index.html`:
- Line 7: Update the document title in the page head from “Vite + Preact” to
identify the Octane cell-spanning example, leaving the surrounding HTML
unchanged.
In `@examples/octane/cell-spanning/src/index.css`:
- Line 135: Update the CSS declarations at the referenced borders to use the
lowercase `currentcolor` keyword, and add an empty line immediately before the
`box-shadow` declaration in the affected rule. Keep all other styling unchanged.
In `@examples/preact/cell-spanning/src/index.css`:
- Line 135: Update the CSS border declarations in the relevant cell-spanning
styles, including the rules for .summary-panel and .column-size-input, to use
the lowercase currentcolor keyword instead of currentColor so Stylelint passes.
- Around line 407-419: Add a blank line in the .cell-selectable rule between the
--cell-edge-left custom property and the box-shadow declaration to satisfy the
declaration-empty-line-before style rule.
In `@examples/react/cell-spanning/src/index.css`:
- Line 135: Update the affected CSS declarations in the stylesheet: change the
`currentColor` values at the rules around lines 135, 216, and 321 to lowercase
`currentcolor`, and add the required empty line before the `box-shadow`
declaration in the rule around lines 414–418 to satisfy the configured Stylelint
rules.
In `@examples/react/cell-spanning/vite.config.js`:
- Around line 12-18: Update examples/react/cell-spanning/vite.config.js lines
12-18 to derive __DEV__ and process.env.NODE_ENV replacement values from the
Vite command or mode, using development values only for serve/dev builds and
production values for builds. Update examples/react/cell-spanning/src/main.tsx
lines 149-163 so both tables enable debugTable only in development, or remove
the option entirely.
In `@examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 817-827: Update the scroll assertion around the merged cell in the
spreadsheet stickiness test to poll on the absolute difference between its
current x position and before.x, rather than only checking that x is less than a
threshold. Move the LessThan(1.5) expectation into the poll callback so the
actual stickiness condition waits for scrolling to settle, and remove the
separate non-retrying assertion.
In `@examples/solid/cell-spanning/src/index.css`:
- Line 135: Update the border declarations at the affected styles in the
cell-spanning stylesheet to use the lowercase CSS keyword currentcolor instead
of currentColor, including all three flagged occurrences.
- Around line 407-419: In the .cell-selectable rule, add a blank line between
the custom property declarations and the box-shadow declaration to satisfy the
declaration-empty-line-before style rule.
In `@examples/vue/cell-spanning/index.html`:
- Around line 5-7: Update the page title in the HTML head from the Vite scaffold
value to the example’s descriptive TanStack Vue Table cell-spanning title, and
verify that the referenced /favicon.ico asset exists for this example; add or
correct the favicon asset/reference if it is missing.
In
`@packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts`:
- Around line 979-997: After merge snapping in the navigation logic, revalidate
the resolved landing column represented by landingColumnIndex using the same
selectable-column predicate already applied to nextColumnIndex. Return null when
the snapped column is not selectable, before resolving or returning the landing
row and column; preserve the existing behavior for selectable destinations.
In
`@packages/table-core/tests/implementation/features/cell-spanning/cellSpanningFeature.test.ts`:
- Around line 314-328: Update the “keeps row spans keyed by column id across
reordering” test to import and register columnOrderingFeature alongside the
existing features, then apply a non-default column order to the table before
asserting regionSpans(table). Keep the assertion focused on the reordered result
and ensure the chosen order exercises column-order handling.
In
`@packages/table-core/tests/unit/features/cell-spanning/cellSpanningFeature.utils.test.ts`:
- Around line 296-312: Rename the test case around cell_getRowSpan and
table_getCellSpanIndex so its title describes resolving the actual row span from
the row model, not “spans of one.” Keep the existing span assertion and
index-building behavior unchanged.
---
Nitpick comments:
In `@examples/alpine/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 105-280: Extract SpanReport, readSpanReport, expectWellFormed,
expectMaximalSpans, and expectGridsMatch into a shared helper under
tests/e2e/helpers/ beside startExampleServer, with a configurable cell selector
defaulting to :scope > td. Update
examples/alpine/cell-spanning/tests/e2e/smoke.spec.ts#L105-L280 to import the
helper and pass :scope > td; update
examples/preact/cell-spanning/tests/e2e/smoke.spec.ts#L105-L276 and
examples/svelte/cell-spanning/tests/e2e/smoke.spec.ts#L105-L276 to import it and
use the default selector, removing the duplicated harness implementations.
- Around line 46-53: Remove the unused react-scan page.route stub from the smoke
tests in examples/alpine/cell-spanning/tests/e2e/smoke.spec.ts,
examples/preact/cell-spanning/tests/e2e/smoke.spec.ts, and
examples/svelte/cell-spanning/tests/e2e/smoke.spec.ts; leave the remaining
smoke-test setup unchanged.
In `@examples/ember/cell-spanning/package.json`:
- Around line 18-25: Add a lint script to the scripts section of the Ember
cell-spanning package.json, matching the established lint command used by the
Vue cell-spanning example and the repository’s other examples. Keep the existing
dev, build, serve, start, test:e2e, and test:types scripts unchanged.
In `@examples/ember/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 529-546: Extract the duplicated drag-selection setup from the
tests into a shared async helper, such as dragShiftOntoMergedRegion, near the
test definitions. Move the spanTable locators and mouse interaction into the
helper, return mergedRegionCell for assertions, and update both affected tests
to call it while preserving their existing behavior.
- Around line 46-53: Remove the react-scan page.route stub from the Ember smoke
test, including its empty fulfillment configuration. Do not replace it in this
suite; only relocate it to a shared helper if existing non-Ember suites require
the same route.
In `@examples/lit/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 446-460: Update the test after unchecking “Row spanning” to wait
for a spanning-derived DOM value to reflect the disabled state before calling
readSpanReport; use the existing polling/stable-signal pattern from the
neighboring tests, then keep the current span assertions unchanged.
In `@examples/octane/cell-spanning/package.json`:
- Line 9: Update the package.json lint script to include the .tsrx extension
alongside .ts, ensuring ESLint checks the example entry file src/main.tsrx.
In `@examples/octane/cell-spanning/vite.config.ts`:
- Around line 4-9: Add allowedHosts: true to the server configuration in the
Vite defineConfig export, alongside port, to support CodeSandbox compatibility
while preserving the existing plugin and server settings.
In `@examples/preact/cell-spanning/vite.config.ts`:
- Around line 5-10: Update the server configuration in defineConfig to include
allowedHosts: true alongside the existing port setting, enabling
CodeSandbox-compatible host handling while preserving the current port and
preact plugin configuration.
In `@examples/react/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 90-276: Extract SpanReport, readSpanReport, expectWellFormed,
expectMaximalSpans, and expectGridsMatch into a shared helper module alongside
tests/e2e/helpers/startExampleServer.ts. In
examples/react/cell-spanning/tests/e2e/smoke.spec.ts lines 90-276,
examples/octane/cell-spanning/tests/e2e/smoke.spec.ts lines 81-267,
examples/solid/cell-spanning/tests/e2e/smoke.spec.ts lines 90-276, and
examples/vue/cell-spanning/tests/e2e/smoke.spec.ts lines 90-276, remove the
duplicated harness and import the shared symbols, leaving only
framework-specific setup and expected counts.
In `@examples/react/spreadsheet/src/SpreadsheetGrid.tsx`:
- Around line 1271-1304: Restrict the anchor scan in MergedCellsOverlay to the
virtualized visible row window, expanding the bounds as needed to include merges
that straddle that window, and pass those row bounds from the cellSelection
subscription into the overlay. Apply the bounds to both the rowSpans and
colSpans passes while preserving anchors whose merge intersects the visible
range.
- Around line 1369-1379: Update MergedCellView and its merge-layer parent
structure so every role="gridcell" with aria-rowspan/aria-colspan is contained
by a role="row", either by wrapping each merged cell in a row or by adding row
elements that own the cells in every layer; preserve the existing merged-cell
rendering and positioning.
In `@examples/react/spreadsheet/tests/e2e/spreadsheet.spec.ts`:
- Around line 864-872: Update the spreadsheet scroll test around the existing
`.merge-layer` clipPath poll to capture the inset value before scrolling, then
assert after scrolling that the inset increases or matches the expected pixel
value based on scrollLeft, ROW_HEADER_WIDTH, and startWidth. Replace the current
presence-only toContain('inset') assertion while preserving the existing polling
behavior.
In `@examples/solid/cell-spanning/tests/e2e/smoke.spec.ts`:
- Around line 46-53: Remove the unused react-scan page.route stub from
openExample in both examples/solid/cell-spanning/tests/e2e/smoke.spec.ts (lines
46-53) and examples/vue/cell-spanning/tests/e2e/smoke.spec.ts (lines 46-53);
leave the React smoke spec route unchanged.
In `@examples/solid/cell-spanning/vite.config.ts`:
- Around line 4-8: Update the server configuration in defineConfig to include
allowedHosts: true alongside the existing port setting, preserving the current
Solid plugin configuration.
In `@examples/vue/cell-spanning/env.d.ts`:
- Line 1: Remove the duplicate env.d.ts declaration by checking the
cell-spanning example’s tsconfig.json include patterns and retaining only the
covered file between the root-level and src-level env.d.ts files. Delete the
other declaration without changing the remaining Vite reference.
In `@examples/vue/cell-spanning/package.json`:
- Line 9: Update the test:types script in the package configuration to invoke
vue-tsc with --noEmit, unless the referenced tsconfig already enables noEmit;
ensure running the type-check command does not write build artifacts.
- Around line 12-24: Remove the unused `@vitejs/plugin-vue-jsx` devDependency from
the cell-spanning example and remove its import and registration from
vite.config.ts; leave the existing Vue configuration unchanged.
- Around line 12-24: Remove the unused `@vitejs/plugin-vue-jsx` entry from
devDependencies, leaving the remaining example dependencies unchanged.
In `@packages/table-core/src/features/cell-selection/cellSelectionFeature.ts`:
- Around line 150-160: Export the existing probeCellSpanIndex helper from
cellSelectionFeature.utils.ts and replace the inline getCellSpanIndex optional
lookup and unknown cast in the memoDeps dependency list with a call to that
helper. Preserve the current memo dependency behavior while centralizing the
typed structural contract.
- Around line 205-209: Remove the redundant getCellSelectionMergeBounds
dependency from the memo configuration for table_getSelectedCellIds, since
table_getCellSelectionBounds already propagates merge changes. Keep the
corresponding dependency in table_getSelectedCellCount because it checks
merges.length directly.
In
`@packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts`:
- Around line 1255-1271: Narrow the enumeration branch in the cell-selection
count logic around getCellSelectionMergeBounds and forEachSelectedCell: first
test whether any final selection bound intersects a merge using
intersectCellSelectionBounds from cellSelectionGeometry. Enumerate selected
cells only when the enableCellSelection predicate is a function or a merge
intersects a final bound; otherwise retain the existing arithmetic path.
- Around line 723-736: Refactor isStripOutside to make its row-versus-column
meaning self-describing, preferably by adding isRowStripOutside and
isColumnStripOutside helpers or replacing the positional arguments with a named
object parameter. Update all four call sites to use the clearer API while
preserving the existing isWithinBounds behavior and results.
- Line 204: Protect the shared EMPTY_MERGE_BOUNDS result used by
table_getCellSelectionMergeBounds from consumer mutation. Either freeze the
module-level constant while preserving the declared return contract, or change
getCellSelectionMergeBounds to return a fresh empty array whenever spanning is
absent.
- Around line 342-359: Optimize findMergeBoundsAt hit-testing by deriving a
row-indexed lookup alongside the memoized merge list, using a Map<number,
Array<CellSelectionBounds>> keyed by each merge’s covered row indices. Update
cell_getSelectionEdges and stepCoordinate to reuse this index when probing
cells, scanning only merges covering the requested row while preserving the
existing bounds matching behavior.
- Line 943: Use fromColumnIndex consistently throughout the column fallback
logic, including the reads and assignments currently using columnIndex near the
next-column calculation. Keep nextColumnIndex initialized from fromColumnIndex
and preserve the existing behavior while removing the mixed coordinate variable
usage.
In `@packages/table-core/src/features/cell-selection/cellSelectionGeometry.ts`:
- Around line 164-187: Replace the restart-based while loop in
table_getCellSelectionBounds with a worklist and visited set for merges. Seed
the worklist with the initial candidate merges, absorb each merge at most once,
and enqueue only unvisited merges that may intersect the expanded bounds;
preserve the existing union logic and fixed-point result while avoiding full
rescans after every expansion.
In `@packages/table-core/src/features/cell-spanning/cellSpanningFeature.types.ts`:
- Around line 139-174: Declare a Row_CellSpanning interface in the cell-spanning
feature types with the internal _cellSpanRowIndex field, register it as
cellSpanningFeature in Row_FeatureMap, and import/use that typed row contract in
cellSpanningFeature.ts and cellSpanningFeature.utils.ts. Remove the three inline
structural casts so all accesses are checked against the shared interface.
In
`@packages/table-core/tests/implementation/features/cell-selection/cellSelectionSpanAware.test.ts`:
- Around line 41-45: Extend the cell-selection span-aware tests around the
shared columns definition and cases at lines 68-119 to include a column with
colSpan, exercising the horizontal-only path in
table_getCellSelectionMergeBounds. Add assertions covering the merged
selection’s expansion, boundary edges, and cell count, including the vertical[r]
!== 1 skip and columnIdBySpanIndex mapping behavior.
In
`@packages/table-core/tests/performance/features/cell-spanning/cellSpanningFeature.test.ts`:
- Around line 55-57: Make the structural assertions in the cell-spanning
performance tests unconditional, but gate the absolute build-duration checks
around the assertions near the rowSpans expectations with an environment flag so
shared CI runs do not depend on wall-clock timing. Update the rowSpans key
comparison near the first structural assertion to compare a sorted copy,
preserving validation of the expected keys without relying on insertion order.
🪄 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: fc7b1d3a-bb0d-4fb1-bb28-3765daf488b3
⛔ Files ignored due to path filters (3)
examples/angular/cell-spanning/public/favicon.icois excluded by!**/*.icoexamples/vue/cell-spanning/public/favicon.icois excluded by!**/*.icopnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (166)
.changeset/cell-spanning-feature.md.changeset/span-aware-cell-selection.md_artifacts/domain_map.yaml_artifacts/skill_tree.yamldocs/config.jsondocs/framework/alpine/guide/cell-spanning.mddocs/framework/angular/guide/cell-spanning.mddocs/framework/angular/guide/migrating.mddocs/framework/ember/guide/cell-spanning.mddocs/framework/lit/guide/cell-spanning.mddocs/framework/lit/guide/migrating.mddocs/framework/octane/guide/cell-spanning.mddocs/framework/preact/guide/cell-spanning.mddocs/framework/preact/guide/migrating.mddocs/framework/react/guide/cell-selection.mddocs/framework/react/guide/cell-spanning.mddocs/framework/react/guide/migrating.mddocs/framework/solid/guide/cell-spanning.mddocs/framework/solid/guide/migrating.mddocs/framework/svelte/guide/cell-spanning.mddocs/framework/svelte/guide/migrating.mddocs/framework/vue/guide/cell-spanning.mddocs/framework/vue/guide/migrating.mddocs/guide/cells.mddocs/guide/headers.mdexamples/alpine/cell-spanning/.devcontainer/devcontainer.jsonexamples/alpine/cell-spanning/.gitignoreexamples/alpine/cell-spanning/index.htmlexamples/alpine/cell-spanning/package.jsonexamples/alpine/cell-spanning/src/index.cssexamples/alpine/cell-spanning/src/main.tsexamples/alpine/cell-spanning/src/makeData.tsexamples/alpine/cell-spanning/src/vite-env.d.tsexamples/alpine/cell-spanning/tests/e2e/smoke.spec.tsexamples/alpine/cell-spanning/tsconfig.jsonexamples/alpine/cell-spanning/vite.config.jsexamples/angular/cell-spanning/.devcontainer/devcontainer.jsonexamples/angular/cell-spanning/.gitignoreexamples/angular/cell-spanning/angular.jsonexamples/angular/cell-spanning/package.jsonexamples/angular/cell-spanning/src/app/app.config.tsexamples/angular/cell-spanning/src/app/app.htmlexamples/angular/cell-spanning/src/app/app.routes.tsexamples/angular/cell-spanning/src/app/app.tsexamples/angular/cell-spanning/src/app/makeData.tsexamples/angular/cell-spanning/src/index.htmlexamples/angular/cell-spanning/src/main.tsexamples/angular/cell-spanning/src/styles.cssexamples/angular/cell-spanning/tests/e2e/smoke.spec.tsexamples/angular/cell-spanning/tsconfig.app.jsonexamples/angular/cell-spanning/tsconfig.jsonexamples/ember/cell-spanning/.devcontainer/devcontainer.jsonexamples/ember/cell-spanning/app/app.cssexamples/ember/cell-spanning/app/app.tsexamples/ember/cell-spanning/app/config.tsexamples/ember/cell-spanning/app/router.tsexamples/ember/cell-spanning/app/templates/application.gtsexamples/ember/cell-spanning/app/utils/make-data.tsexamples/ember/cell-spanning/babel.config.jsexamples/ember/cell-spanning/index.htmlexamples/ember/cell-spanning/package.jsonexamples/ember/cell-spanning/tests/e2e/smoke.spec.tsexamples/ember/cell-spanning/tsconfig.jsonexamples/ember/cell-spanning/vite.config.mjsexamples/lit/cell-spanning/.devcontainer/devcontainer.jsonexamples/lit/cell-spanning/.gitignoreexamples/lit/cell-spanning/index.htmlexamples/lit/cell-spanning/package.jsonexamples/lit/cell-spanning/src/main.tsexamples/lit/cell-spanning/src/makeData.tsexamples/lit/cell-spanning/tests/e2e/smoke.spec.tsexamples/lit/cell-spanning/tsconfig.jsonexamples/lit/cell-spanning/vite.config.jsexamples/octane/cell-spanning/index.htmlexamples/octane/cell-spanning/package.jsonexamples/octane/cell-spanning/src/index.cssexamples/octane/cell-spanning/src/main.tsrxexamples/octane/cell-spanning/src/makeData.tsexamples/octane/cell-spanning/src/vite-env.d.tsexamples/octane/cell-spanning/tests/e2e/smoke.spec.tsexamples/octane/cell-spanning/tsconfig.jsonexamples/octane/cell-spanning/vite.config.tsexamples/preact/cell-spanning/.devcontainer/devcontainer.jsonexamples/preact/cell-spanning/.gitignoreexamples/preact/cell-spanning/index.htmlexamples/preact/cell-spanning/package.jsonexamples/preact/cell-spanning/src/index.cssexamples/preact/cell-spanning/src/main.tsxexamples/preact/cell-spanning/src/makeData.tsexamples/preact/cell-spanning/src/vite-env.d.tsexamples/preact/cell-spanning/tests/e2e/smoke.spec.tsexamples/preact/cell-spanning/tsconfig.jsonexamples/preact/cell-spanning/vite.config.tsexamples/react/cell-spanning/.devcontainer/devcontainer.jsonexamples/react/cell-spanning/.gitignoreexamples/react/cell-spanning/index.htmlexamples/react/cell-spanning/package.jsonexamples/react/cell-spanning/src/index.cssexamples/react/cell-spanning/src/main.tsxexamples/react/cell-spanning/src/makeData.tsexamples/react/cell-spanning/src/vite-env.d.tsexamples/react/cell-spanning/tests/e2e/smoke.spec.tsexamples/react/cell-spanning/tsconfig.jsonexamples/react/cell-spanning/vite.config.jsexamples/react/spreadsheet/src/Spreadsheet.tsxexamples/react/spreadsheet/src/SpreadsheetGrid.tsxexamples/react/spreadsheet/src/index.cssexamples/react/spreadsheet/src/spreadsheetTable.tsexamples/react/spreadsheet/tests/e2e/spreadsheet.spec.tsexamples/solid/cell-spanning/.devcontainer/devcontainer.jsonexamples/solid/cell-spanning/.gitignoreexamples/solid/cell-spanning/index.htmlexamples/solid/cell-spanning/package.jsonexamples/solid/cell-spanning/src/App.tsxexamples/solid/cell-spanning/src/index.cssexamples/solid/cell-spanning/src/index.tsxexamples/solid/cell-spanning/src/makeData.tsexamples/solid/cell-spanning/src/vite-env.d.tsexamples/solid/cell-spanning/tests/e2e/smoke.spec.tsexamples/solid/cell-spanning/tsconfig.jsonexamples/solid/cell-spanning/vite.config.tsexamples/svelte/cell-spanning/.devcontainer/devcontainer.jsonexamples/svelte/cell-spanning/.gitignoreexamples/svelte/cell-spanning/index.htmlexamples/svelte/cell-spanning/package.jsonexamples/svelte/cell-spanning/src/App.svelteexamples/svelte/cell-spanning/src/index.cssexamples/svelte/cell-spanning/src/main.tsexamples/svelte/cell-spanning/src/makeData.tsexamples/svelte/cell-spanning/svelte.config.jsexamples/svelte/cell-spanning/tests/e2e/smoke.spec.tsexamples/svelte/cell-spanning/tsconfig.jsonexamples/svelte/cell-spanning/vite.config.jsexamples/vue/cell-spanning/.devcontainer/devcontainer.jsonexamples/vue/cell-spanning/.gitignoreexamples/vue/cell-spanning/env.d.tsexamples/vue/cell-spanning/index.htmlexamples/vue/cell-spanning/package.jsonexamples/vue/cell-spanning/src/App.vueexamples/vue/cell-spanning/src/env.d.tsexamples/vue/cell-spanning/src/index.cssexamples/vue/cell-spanning/src/main.tsexamples/vue/cell-spanning/src/makeData.tsexamples/vue/cell-spanning/tests/e2e/smoke.spec.tsexamples/vue/cell-spanning/tsconfig.jsonexamples/vue/cell-spanning/vite.config.tspackages/table-core/skills/cell-spanning/SKILL.mdpackages/table-core/src/features/cell-selection/cellSelectionFeature.tspackages/table-core/src/features/cell-selection/cellSelectionFeature.types.tspackages/table-core/src/features/cell-selection/cellSelectionFeature.utils.tspackages/table-core/src/features/cell-selection/cellSelectionGeometry.tspackages/table-core/src/features/cell-spanning/cellSpanningFeature.tspackages/table-core/src/features/cell-spanning/cellSpanningFeature.types.tspackages/table-core/src/features/cell-spanning/cellSpanningFeature.utils.tspackages/table-core/src/features/stockFeatures.tspackages/table-core/src/index.tspackages/table-core/src/static-functions.tspackages/table-core/src/types/Cell.tspackages/table-core/src/types/ColumnDef.tspackages/table-core/src/types/Table.tspackages/table-core/src/types/TableOptions.tspackages/table-core/tests/implementation/features/cell-selection/cellSelectionGeometry.test.tspackages/table-core/tests/implementation/features/cell-selection/cellSelectionSpanAware.test.tspackages/table-core/tests/implementation/features/cell-spanning/cellSpanningFeature.test.tspackages/table-core/tests/performance/features/cell-spanning/cellSpanningFeature.test.tspackages/table-core/tests/unit/features/cell-spanning/cellSpanningFeature.utils.test.ts
| } | ||
| ``` | ||
|
|
||
| Known limitation: `cellSelectionFeature` is not span-aware yet. Selection ranges address the underlying cell grid, so a selection crossing a merged cell selects the covered cells individually. Span-aware selection is planned as a follow-up. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the stale selection limitation.
This changeset says that cellSelectionFeature is not span-aware. The companion changeset and getCellSelectionMergeBounds() implementation show that merge-aware selection ships in this PR. Replace this limitation with the shipped behavior before publishing the release note.
🤖 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 @.changeset/cell-spanning-feature.md at line 30, Update the changeset’s
selection description to remove the stale statement that cellSelectionFeature is
not span-aware and that span-aware selection is planned. Document the shipped
merge-aware behavior instead, including that getCellSelectionMergeBounds()
ensures selections crossing merged cells account for their merge bounds.
Comment on lines
+315
to
+324
| for (let c = 0; c < rowColSpans.length; c++) { | ||
| const span = rowColSpans[c]! | ||
| if (span <= 1) continue | ||
|
|
||
| const columnId = columnIdBySpanIndex[c] | ||
| if (columnId === undefined) continue | ||
| // Vertical runs already emitted this anchor as a full rectangle, and | ||
| // their covered rows sit inside it. | ||
| const vertical = spanIndex.rowSpans[columnId] | ||
| if (vertical && vertical[r] !== 1) continue |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The horizontal-span branch mixes two span encodings and has no test. Line 281 treats rowSpans entries <= 1 as "no run", so 0 is a valid non-span value, and Int32Array initializes to 0. Line 324 instead skips whenever vertical[r] !== 1. packages/table-core/tests/performance/features/cell-spanning/cellSpanningFeature.test.ts Line 89 asserts a grand total of exactly ROW_COUNT * COLUMN_COUNT, which holds only when covered cells report 0. That confirms 0 is a real value in these arrays, so the !== 1 check can drop legitimate column spans. No test exercises this branch, so the defect would ship unnoticed.
packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts#L315-L324: change the skip to test for a vertical run explicitly, for exampleif (vertical && vertical[r]! > 1) continue, so entries of0no longer suppress a valid column span.packages/table-core/tests/implementation/features/cell-selection/cellSelectionSpanAware.test.ts#L41-L45: add a column definition that produces a column span, then assert merge bounds, selection expansion, edges, and count for a merged cell that spans columns.
📍 Affects 2 files
packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts#L315-L324(this comment)packages/table-core/tests/implementation/features/cell-selection/cellSelectionSpanAware.test.ts#L41-L45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/table-core/src/features/cell-selection/cellSelectionFeature.utils.ts`
around lines 315 - 324, Update the horizontal-span logic in
cellSelectionFeature.utils.ts (lines 315-324) to skip only explicit vertical
runs by checking for values greater than 1, preserving 0 as a valid non-span
value. Add coverage in cellSelectionSpanAware.test.ts (lines 41-45) with a
column-spanning definition and assertions for merged-cell bounds, selection
expansion, edges, and count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
New Features
Documentation
Tests