feat: much better resizing performance#6367
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates multiple framework examples to use semantic table markup and direct column-sizing updates, adds interactive resizing to virtualized column demos, refactors table-core sizing and memoization behavior, adds Lit selector-gated updates, and introduces a new React TanStack Start SSR kitchen-sink example. ChangesColumn-resizing performant examples
Estimated code review effort: 4 (Complex) | ~60 minutes Virtualized-columns resize feature addition
Estimated code review effort: 3 (Moderate) | ~30 minutes table-core sizing, offset, and memoization refactor
Estimated code review effort: 4 (Complex) | ~45 minutes lit-table selector gate
Estimated code review effort: 3 (Moderate) | ~20 minutes React kitchen-sink-start SSR example
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ResizerUI as Header Resizer
participant Table as TanStack Table
participant columnSizingAtom as columnSizing atom
participant TableDOM as Table DOM
User->>ResizerUI: drag handle
ResizerUI->>Table: getResizeHandler()
Table->>columnSizingAtom: update sizing state
columnSizingAtom-->>TableDOM: write CSS variables and debug text
sequenceDiagram
participant User
participant HeaderResizer as Virtualized Header Resizer
participant Table as TanStack Table
participant columnSizingAtom as columnSizing atom
participant ColumnVirtualizer as Column Virtualizer
User->>HeaderResizer: drag resizer
HeaderResizer->>Table: update column sizing
Table->>columnSizingAtom: publish sizing change
columnSizingAtom-->>ColumnVirtualizer: measure()
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit dfc82c0
☁️ Nx Cloud last updated this comment at |
![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
🧹 Nitpick comments (8)
packages/table-core/src/features/column-sizing/columnSizingFeature.ts (2)
82-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate memoDeps arrays for
header_getStartandtable_getColumnOffsets.Both lists enumerate the exact same 7 dependencies. Since
header_getStartinternally reads offsets that are driven bytable_getColumnOffsets, keeping these in sync manually is fragile — a future dependency added to one but not the other reintroduces the stale-memo bug this change is fixing.♻️ Suggested refactor: extract a shared deps function
+function columnOffsetsMemoDeps<TFeatures extends TableFeatures, TData extends RowData>( + table: Table_Internal<TFeatures, TData>, +) { + return [ + table.options.columns, + table.atoms.columnSizing?.get(), + table.atoms.columnOrder?.get(), + table.atoms.columnPinning?.get(), + table.atoms.columnVisibility?.get(), + table.atoms.grouping?.get(), + table.options.groupedColumnMode, + ] +} + header_getStart: { fn: (header) => header_getStart(header), - memoDeps: () => [ - table.options.columns, - table.atoms.columnSizing?.get(), - table.atoms.columnOrder?.get(), - table.atoms.columnPinning?.get(), - table.atoms.columnVisibility?.get(), - table.atoms.grouping?.get(), - table.options.groupedColumnMode, - ], + memoDeps: () => columnOffsetsMemoDeps(table), }, ... table_getColumnOffsets: { fn: () => table_getColumnOffsets(table), - memoDeps: () => [ - table.options.columns, - table.atoms.columnSizing?.get(), - table.atoms.columnOrder?.get(), - table.atoms.columnPinning?.get(), - table.atoms.columnVisibility?.get(), - table.atoms.grouping?.get(), - table.options.groupedColumnMode, - ], + memoDeps: () => columnOffsetsMemoDeps(table), },🤖 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/column-sizing/columnSizingFeature.ts` around lines 82 - 108, The memoDeps arrays for header_getStart and table_getColumnOffsets are duplicated and must stay in sync manually. Extract the shared dependency list into a single helper or shared function in columnSizingFeature so both APIs reuse the same source of truth, and update the assignTableAPIs calls to reference that shared deps builder.
82-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
header.getStart()regression test for the grouping memoDeps fix.The new grouping tests in
columnSizingFeature.utils.test.tsvalidatecolumn.getStart()/getAfter()but notheader.getStart(), which is the function whose memoDeps were actually expanded here. A header-level assertion would directly protect against this cache going stale again.🤖 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/column-sizing/columnSizingFeature.ts` around lines 82 - 91, The grouping memoDeps fix in columnSizingFeature should be covered with a header-level regression test because the changed cache affects header.getStart(), not just column.getStart()/getAfter(). Update the existing grouping-related tests in columnSizingFeature.utils.test.ts to include an assertion on header.getStart() for a grouped header, using the relevant header/column setup so the memoized start position is verified after grouping changes.packages/table-core/src/features/column-sizing/columnSizingFeature.utils.ts (1)
131-157: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
table_getColumnOffsetsalways builds 4 offset maps, even when pinning is unused.For tables without column pinning,
centeris identical toall, andleft/rightare empty — yet all four regions are unconditionally recomputed on every dependency change, including every committed resize tick. Consider short-circuiting to skip left/right (or reuseallforcenter) whencolumnPinningis empty, to avoid extra work exactly on the hot path this PR is trying to speed up.🤖 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/column-sizing/columnSizingFeature.utils.ts` around lines 131 - 157, table_getColumnOffsets currently recomputes all four pinning buckets on every call, even when columnPinning is unused. Update table_getColumnOffsets to short-circuit based on the table state: when pinning is empty, reuse the all result for center and avoid building left/right offsets; otherwise keep the current per-position behavior. Use table_getPinnedVisibleLeafColumns and buildColumnOffsets as the key symbols to adjust, so the hot path no longer does redundant work.packages/table-core/src/features/column-pinning/columnPinningFeature.ts (1)
260-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect dependency additions; consider extracting a shared grouping-deps helper.
The added
table.atoms.grouping?.get()/table.options.groupedColumnModepairs are correctly placed on all six leaf-column getters. The same two-line pair is now repeated six times in this file; extracting a small helper (e.g.groupingMemoDeps(table)) returning[grouping, groupedColumnMode]and spreading it into each array would reduce duplication if these deps need to change again later.Also applies to: 264-283, 289-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 `@packages/table-core/src/features/column-pinning/columnPinningFeature.ts` around lines 260 - 262, The leaf-column getter dependency arrays now repeat the same grouping deps in several places, so refactor the duplicated `table.atoms.grouping?.get()` and `table.options.groupedColumnMode` pair into a shared helper in `columnPinningFeature.ts` (for example, a `groupingMemoDeps(table)` function) and spread that helper result into each of the six memo dependency arrays used by the leaf-column getters. Keep the existing dependency values unchanged, just centralize them so all getters use the same shared source.examples/react/virtualized-columns/src/main.tsx (1)
77-86: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftFull-state selector +
onChangeresize will re-render the whole virtualized table per drag frame.
columnResizeMode: 'onChange'now drives per-frame updates through a selector that subscribes to all table state ((state) => state), so every resize tick re-rendersTableContainerand its (potentially thousands of) virtualized header/body cells — the exact per-tick cost this PR's siblingcolumn-resizing-performantexamples specifically eliminate via CSS-variable/Subscribe-island patterns.Consider narrowing the selector (e.g. subscribe to nothing and drive width via CSS vars/
table.Subscribeislands) to keep this virtualization demo consistent with the perf goals of the PR.🤖 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/virtualized-columns/src/main.tsx` around lines 77 - 86, The virtualized table in useTable is subscribing to the entire table state via the default selector, which causes full re-renders on every onChange resize tick. Update the selector in main.tsx to avoid full-state subscription and move resize-driven width updates into a narrower subscription path, using the same CSS-variable or table.Subscribe island pattern used by the performant column-resizing examples. Keep the fix centered around useTable, columnResizeMode, and the selector callback so the virtualization demo stays aligned with the performance intent.examples/alpine/column-resizing-performant/index.html (1)
31-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSemantic
<table>overridden withdisplay: grid/flexmay lose implicit table a11y roles.Setting
display: gridon<thead>/<tbody>anddisplay: flexon<tr>is a common technique for CSS-variable-driven column widths, but several browsers drop the implicit ARIA table/row/cell roles once the table's CSSdisplayno longer matches its native display type. Since this pattern is reused across the whole "column-resizing-performant" cohort (Angular, Svelte, Vue, etc.), consider adding explicitrole="table",role="row",role="columnheader"/role="cell"to restore semantics for assistive technology.♿ Suggested roles for restoring table semantics
- <table :style="columnSizeVars()"> - <thead style="display: grid"> + <table :style="columnSizeVars()" role="table"> + <thead style="display: grid" role="rowgroup"> <template ...> - <tr style="display: flex; width: 100%; height: 30px"> + <tr style="display: flex; width: 100%; height: 30px" role="row">🤖 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/column-resizing-performant/index.html` around lines 31 - 85, The table in the Alpine column-resizing example loses native accessibility semantics because `thead`, `tbody`, and `tr` are restyled with `display: grid`/`flex`. Update the markup around `table`, `thead`, `tbody`, `tr`, `th`, and `td` to restore semantics with explicit ARIA roles. Add `role="table"` to the container, `role="row"` to each header/body row, `role="columnheader"` to `th`, and `role="cell"` to `td` so assistive tech still recognizes the structure.examples/react/kitchen-sink-start/vite.config.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
server.allowedHostsfor CodeSandbox compatibility.Based on learnings, Vite configs in this repo's examples add
server: { allowedHosts: true }to enable DNS rebinding protection bypass in CodeSandbox environments on newer Vite versions; this is forward-compatible and ignored by older Vite versions.🔧 Suggested addition
server: { port: 3000, + allowedHosts: true, },Based on learnings from a prior PR (technicallynick, TanStack/table PR
#6176): "add server: { allowedHosts: true } to enable 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/react/kitchen-sink-start/vite.config.ts` around lines 11 - 13, The Vite config’s server block only sets port and is missing the CodeSandbox compatibility setting. Update the server configuration in the react kitchen-sink Vite config to include allowedHosts set to true alongside the existing port setting; use the server object in vite.config.ts so newer Vite versions work in CodeSandbox while older versions ignore it.Source: Learnings
examples/react/kitchen-sink-start/package.json (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
pnpm dlxoverpnpx.The rest of the project uses pnpm (per README:
pnpm install,pnpm dev), but thestartscript invokespnpx, which is deprecated in favor ofpnpm dlx. Purely stylistic but worth aligning for consistency.🤖 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/kitchen-sink-start/package.json` at line 9, The package.json start script uses pnpx, which should be aligned with the project’s pnpm usage. Update the start command to use pnpm dlx instead of pnpx, keeping the existing srvx invocation and arguments intact. Use the start script entry in package.json as the place to make this consistency fix.
🤖 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/alpine/column-resizing-performant/src/index.css`:
- Around line 250-254: The stylesheet uses the mixed-case keyword currentColor,
which violates stylelint’s value-keyword-case rule. Update the border color
value in .summary-panel and the matching declaration in .column-size-input to
use lowercase currentcolor so both occurrences pass CI.
In `@examples/lit/column-resizing-performant/src/main.ts`:
- Around line 94-119: The resize listener setup in firstUpdated() is one-time
only, so after disconnectedCallback() unsubscribes, the element won’t
resubscribe when reattached and column sizes stop updating. Move the
subscription/setup logic into connectedCallback() in the same class, using the
existing writeColumnSizeVars helper and guarding on this._tableEl and
this._table so it can safely reinitialize after reconnect. Keep
disconnectedCallback() responsible for unsubscribing and clearing
this._sizingSub to avoid duplicate subscriptions.
- Around line 171-241: The table markup in the Lit example is using display:
grid/flex on native table elements, which can remove their implicit
accessibility semantics. Update the rendering in the main table template so the
<table>, <tr>, <th>, and <td> elements in the repeat-based rendering keep table
semantics by either restoring table display values or adding explicit roles such
as table, row, columnheader, and cell on the corresponding elements. Keep the
change scoped to the table-rendering logic around the table.getHeaderGroups()
and table.getRowModel().rows blocks.
In `@examples/react/kitchen-sink-start/package.json`:
- Around line 30-38: The kitchen-sink-start package is missing a required
devDependency for the React compiler setup. Add `@babel/core` to the
devDependencies in the package.json used by the React example so
reactCompilerPreset() can work with `@rolldown/plugin-babel` and
babel-plugin-react-compiler.
In `@examples/react/kitchen-sink-start/src/index.css`:
- Line 187: Update the CSS declarations in the kitchen-sink stylesheet so the
`currentColor` keyword uses lowercase `currentcolor` at each flagged border
rule. Make the same casing change in all affected declarations in the
`index.css` file, including the ones associated with the style blocks near the
repeated border definitions, to satisfy the `value-keyword-case` rule.
In `@examples/react/kitchen-sink-start/src/routes/index.tsx`:
- Around line 689-723: The resizing path in the App component is still
subscribing to the full table state through useTable(..., (state) => state),
which forces a full re-render on each column-resize tick. Update the
kitchen-sink route to follow the same non-rendering pattern used by the sibling
column-resizing-performant examples: move the column-size CSS variable updates
out of React render/memoization and into a side-effect or subscribed
atom/selector approach, so App does not re-render while resizing. Use the
existing useTable, useTanStackTableDevtools, and columnSizeVars setup as the
place to refactor.
In `@packages/lit-table/src/TableController.ts`:
- Line 2: The TableController import is pulling shallow from the wrong package,
since `@tanstack/lit-store` does not export it. Update the import in
TableController to use shallow from `@tanstack/store`, and ensure `@tanstack/store`
is listed as a direct dependency if it is not already.
---
Nitpick comments:
In `@examples/alpine/column-resizing-performant/index.html`:
- Around line 31-85: The table in the Alpine column-resizing example loses
native accessibility semantics because `thead`, `tbody`, and `tr` are restyled
with `display: grid`/`flex`. Update the markup around `table`, `thead`, `tbody`,
`tr`, `th`, and `td` to restore semantics with explicit ARIA roles. Add
`role="table"` to the container, `role="row"` to each header/body row,
`role="columnheader"` to `th`, and `role="cell"` to `td` so assistive tech still
recognizes the structure.
In `@examples/react/kitchen-sink-start/package.json`:
- Line 9: The package.json start script uses pnpx, which should be aligned with
the project’s pnpm usage. Update the start command to use pnpm dlx instead of
pnpx, keeping the existing srvx invocation and arguments intact. Use the start
script entry in package.json as the place to make this consistency fix.
In `@examples/react/kitchen-sink-start/vite.config.ts`:
- Around line 11-13: The Vite config’s server block only sets port and is
missing the CodeSandbox compatibility setting. Update the server configuration
in the react kitchen-sink Vite config to include allowedHosts set to true
alongside the existing port setting; use the server object in vite.config.ts so
newer Vite versions work in CodeSandbox while older versions ignore it.
In `@examples/react/virtualized-columns/src/main.tsx`:
- Around line 77-86: The virtualized table in useTable is subscribing to the
entire table state via the default selector, which causes full re-renders on
every onChange resize tick. Update the selector in main.tsx to avoid full-state
subscription and move resize-driven width updates into a narrower subscription
path, using the same CSS-variable or table.Subscribe island pattern used by the
performant column-resizing examples. Keep the fix centered around useTable,
columnResizeMode, and the selector callback so the virtualization demo stays
aligned with the performance intent.
In `@packages/table-core/src/features/column-pinning/columnPinningFeature.ts`:
- Around line 260-262: The leaf-column getter dependency arrays now repeat the
same grouping deps in several places, so refactor the duplicated
`table.atoms.grouping?.get()` and `table.options.groupedColumnMode` pair into a
shared helper in `columnPinningFeature.ts` (for example, a
`groupingMemoDeps(table)` function) and spread that helper result into each of
the six memo dependency arrays used by the leaf-column getters. Keep the
existing dependency values unchanged, just centralize them so all getters use
the same shared source.
In `@packages/table-core/src/features/column-sizing/columnSizingFeature.ts`:
- Around line 82-108: The memoDeps arrays for header_getStart and
table_getColumnOffsets are duplicated and must stay in sync manually. Extract
the shared dependency list into a single helper or shared function in
columnSizingFeature so both APIs reuse the same source of truth, and update the
assignTableAPIs calls to reference that shared deps builder.
- Around line 82-91: The grouping memoDeps fix in columnSizingFeature should be
covered with a header-level regression test because the changed cache affects
header.getStart(), not just column.getStart()/getAfter(). Update the existing
grouping-related tests in columnSizingFeature.utils.test.ts to include an
assertion on header.getStart() for a grouped header, using the relevant
header/column setup so the memoized start position is verified after grouping
changes.
In `@packages/table-core/src/features/column-sizing/columnSizingFeature.utils.ts`:
- Around line 131-157: table_getColumnOffsets currently recomputes all four
pinning buckets on every call, even when columnPinning is unused. Update
table_getColumnOffsets to short-circuit based on the table state: when pinning
is empty, reuse the all result for center and avoid building left/right offsets;
otherwise keep the current per-position behavior. Use
table_getPinnedVisibleLeafColumns and buildColumnOffsets as the key symbols to
adjust, so the hot path no longer does redundant work.
🪄 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
Run ID: 1452f248-14ea-495c-a7ea-effc43e2f528
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
examples/alpine/column-resizing-performant/index.htmlexamples/alpine/column-resizing-performant/src/index.cssexamples/alpine/column-resizing-performant/src/main.tsexamples/angular/column-resizing-performant/src/app/app.htmlexamples/angular/column-resizing-performant/src/app/app.tsexamples/angular/column-resizing-performant/src/app/resizable-cell/resizable-cell.tsexamples/angular/column-resizing-performant/src/styles.cssexamples/angular/virtualized-columns/src/app/app.htmlexamples/angular/virtualized-columns/src/app/app.tsexamples/angular/virtualized-columns/src/styles.cssexamples/lit/column-resizing-performant/src/main.tsexamples/lit/virtualized-columns/src/main.tsexamples/preact/column-resizing-performant/src/index.cssexamples/preact/column-resizing-performant/src/main.tsxexamples/react/column-pinning-sticky/src/main.tsxexamples/react/column-resizing-performant/src/index.cssexamples/react/column-resizing-performant/src/main.tsxexamples/react/column-resizing-performant/tests/e2e/smoke.spec.tsexamples/react/kitchen-sink-start/README.mdexamples/react/kitchen-sink-start/package.jsonexamples/react/kitchen-sink-start/src/index.cssexamples/react/kitchen-sink-start/src/makeData.tsexamples/react/kitchen-sink-start/src/routeTree.gen.tsexamples/react/kitchen-sink-start/src/router.tsxexamples/react/kitchen-sink-start/src/routes/__root.tsxexamples/react/kitchen-sink-start/src/routes/index.tsxexamples/react/kitchen-sink-start/tsconfig.jsonexamples/react/kitchen-sink-start/vite.config.tsexamples/react/virtualized-columns-experimental/src/index.cssexamples/react/virtualized-columns-experimental/src/main.tsxexamples/react/virtualized-columns-experimental/vite.config.jsexamples/react/virtualized-columns/src/index.cssexamples/react/virtualized-columns/src/main.tsxexamples/react/virtualized-columns/vite.config.jsexamples/solid/column-resizing-performant/src/App.tsxexamples/solid/column-resizing-performant/src/index.cssexamples/solid/virtualized-columns/src/App.tsxexamples/solid/virtualized-columns/src/index.cssexamples/svelte/column-resizing-performant/src/App.svelteexamples/svelte/column-resizing-performant/src/index.cssexamples/svelte/virtualized-columns/src/App.svelteexamples/svelte/virtualized-columns/src/index.cssexamples/vue/column-resizing-performant/src/App.vueexamples/vue/column-resizing-performant/src/index.cssexamples/vue/virtualized-columns/src/App.vueexamples/vue/virtualized-columns/src/index.csspackages/lit-table/src/TableController.tspackages/lit-table/tests/unit/selectorGate.test.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.tspackages/table-core/src/features/column-pinning/columnPinningFeature.tspackages/table-core/src/features/column-resizing/columnResizingFeature.utils.tspackages/table-core/src/features/column-sizing/columnSizingFeature.tspackages/table-core/src/features/column-sizing/columnSizingFeature.types.tspackages/table-core/src/features/column-sizing/columnSizingFeature.utils.tspackages/table-core/src/features/column-visibility/columnVisibilityFeature.tspackages/table-core/src/features/column-visibility/columnVisibilityFeature.utils.tspackages/table-core/tests/unit/features/column-resizing/columnResizingFeature.utils.test.tspackages/table-core/tests/unit/features/column-sizing/columnSizingFeature.utils.test.tspackages/table-core/tests/unit/features/column-visibility/columnVisibilityFeature.utils.test.ts
💤 Files with no reviewable changes (1)
- examples/angular/column-resizing-performant/src/app/resizable-cell/resizable-cell.ts
Comment on lines
+250
to
+254
| .summary-panel { | ||
| border: 1px solid currentColor; | ||
| box-shadow: 0 1px 3px rgb(0 0 0 / 0.2); | ||
| padding: 0.5rem; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Lowercase currentColor to satisfy stylelint (value-keyword-case).
Stylelint flags this occurrence (and the identical one on Line 356 in .column-size-input); both will fail CI.
🎨 Proposed fix
.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;
}🧰 Tools
🪛 Stylelint (17.14.0)
[error] 251-251: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/column-resizing-performant/src/index.css` around lines 250 -
254, The stylesheet uses the mixed-case keyword currentColor, which violates
stylelint’s value-keyword-case rule. Update the border color value in
.summary-panel and the matching declaration in .column-size-input to use
lowercase currentcolor so both occurrences pass CI.
Source: Linters/SAST tools
Comment on lines
+171
to
+241
| <table style="display: grid"> | ||
| <thead style="display: grid"> | ||
| ${repeat( | ||
| table.getHeaderGroups(), | ||
| (headerGroup) => headerGroup.id, | ||
| (headerGroup) => html` | ||
| <div class="tr"> | ||
| <tr style="display: flex; width: 100%; height: 30px"> | ||
| ${repeat( | ||
| headerGroup.headers, | ||
| (header) => header.id, | ||
| (header) => html` | ||
| <div | ||
| class="th" | ||
| style="${styleMap({ | ||
| width: `calc(var(--header-${header.id}-size) * 1px)`, | ||
| })}" | ||
| <th | ||
| colspan=${header.colSpan} | ||
| style="display: flex; flex-shrink: 0; width: calc(var(--header-${header.id}-size) * 1px)" | ||
| > | ||
| ${header.isPlaceholder | ||
| ? null | ||
| : FlexRender({ header })} | ||
| <div | ||
| @dblclick="${() => header.column.resetSize()}" | ||
| @mousedown="${header.getResizeHandler()}" | ||
| @touchstart="${header.getResizeHandler()}" | ||
| class="resizer ${header.column.getIsResizing() | ||
| ? 'isResizing' | ||
| : ''}" | ||
| ></div> | ||
| </div> | ||
| ${table.subscribe( | ||
| table.atoms.columnResizing, | ||
| // Each resizer subscribes to just its own "am I | ||
| // being resized?" boolean, so a drag re-renders | ||
| // exactly one of these islands at drag start/end | ||
| (columnResizing) => | ||
| columnResizing.isResizingColumn === | ||
| header.column.id, | ||
| (isResizing) => html` | ||
| <div | ||
| @dblclick="${() => header.column.resetSize()}" | ||
| @mousedown="${header.getResizeHandler()}" | ||
| @touchstart="${header.getResizeHandler()}" | ||
| class="resizer ${isResizing | ||
| ? 'isResizing' | ||
| : ''}" | ||
| ></div> | ||
| `, | ||
| )} | ||
| </th> | ||
| `, | ||
| )} | ||
| </div> | ||
| </tr> | ||
| `, | ||
| )} | ||
| </div> | ||
| <div class="tbody"> | ||
| </thead> | ||
| <!-- No memoization needed: the host subscribes to no table state, | ||
| so the body only re-renders when the data itself changes --> | ||
| <tbody style="display: grid"> | ||
| ${repeat( | ||
| table.getRowModel().rows, | ||
| (row) => row.id, | ||
| (row) => html` | ||
| <div class="tr"> | ||
| <tr | ||
| style="display: flex; width: 100%; height: 30px; content-visibility: auto; contain-intrinsic-height: auto 30px" | ||
| > | ||
| ${repeat( | ||
| row.getAllCells(), | ||
| (cell) => cell.id, | ||
| (cell) => html` | ||
| <div | ||
| class="td" | ||
| style="${styleMap({ | ||
| width: `calc(var(--col-${cell.column.id}-size) * 1px)`, | ||
| })}" | ||
| <td | ||
| style="display: flex; flex-shrink: 0; width: calc(var(--col-${cell | ||
| .column.id}-size) * 1px)" | ||
| > | ||
| ${cell.renderValue()} | ||
| </div> | ||
| </td> | ||
| `, | ||
| )} | ||
| </div> | ||
| </tr> | ||
| `, | ||
| )} | ||
| </div> | ||
| </div> | ||
| </tbody> | ||
| </table> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
does setting CSS display grid on table element remove ARIA table role accessibility tree
💡 Result:
Yes, setting CSS display: grid (or other non-table display values) on a native
element can cause browsers to remove its table semantics from the accessibility tree, which prevents screen readers from announcing it as a table and breaks table-specific navigation [1][2][3]. While the W3C specification explicitly states that the display property should not affect an element's document semantics [4][5], this remains a known implementation bug or limitation in many browsers [1][4][6]. Historically, changing the display property on a| element caused browsers to stop treating them as structural table elements in the accessibility API [1][7][3]. If you choose to use CSS grid on a table, you risk losing accessibility features unless you manually re-apply them [1][6]. To mitigate this, developers often use ARIA roles to explicitly define the semantics, such as applying role="table" to the table element, role="row" to rows, and role="cell" to cells [8][9][6]. However, this approach requires significant maintenance and may not restore all native table functionality, such as complex spanning or header associations, which can still be broken by the layout change [9][10]. For critical data tables, it is strongly recommended to avoid changing the default display property [10]. If a responsive design requires such a change, you must rigorously test across different browsers and screen readers to ensure that the necessary accessibility tree information is still being exposed correctly [11][10].
Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== Relevant file lines ==\n'
sed -n '150,260p' examples/lit/column-resizing-performant/src/main.ts
printf '\n== Search for role-based selectors in repo ==\n'
rg -n --hidden --glob '!**/node_modules/**' 'getByRole|role="' examples | head -n 200
printf '\n== Search for display:grid/flex on table elements in examples ==\n'
rg -n --hidden --glob '!**/node_modules/**' '<table[^>]*display:\s*grid|<tr[^>]*display:\s*flex|<th[^>]*display:\s*flex|<td[^>]*display:\s*flex' examplesRepository: TanStack/table Length of output: 32824 Preserve the table semantics here — 🤖 Prompt for AI Agents |
Comment on lines
+30
to
+38
| "devDependencies": { | ||
| "@rolldown/plugin-babel": "^0.2.3", | ||
| "@types/node": "^26.0.0", | ||
| "@types/react": "^19.2.17", | ||
| "@types/react-dom": "^19.2.3", | ||
| "@vitejs/plugin-react": "^6.0.3", | ||
| "babel-plugin-react-compiler": "^1.0.0", | ||
| "typescript": "6.0.3", | ||
| "vite": "^8.1.0" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the current peer dependency list and required version range for @babel/corewhen using reactCompilerPreset with@vitejs/plugin-react v6?
💡 Result:
When using reactCompilerPreset with @vitejs/plugin-react v6, @babel/core is required as a peer dependency [1][2]. While @vitejs/plugin-react v6 itself no longer includes Babel as a dependency, the reactCompilerPreset helper explicitly requires @babel/core, @rolldown/plugin-babel, and babel-plugin-react-compiler to be installed in your project [3][4]. Official documentation and package metadata specify that @babel/core must be installed alongside the other required packages [1][5]: npm install -D @rolldown/plugin-babel @babel/core babel-plugin-react-compiler There is no strict, single version range for @babel/core enforced by the plugin's peer dependency list in the same way as core dependencies; however, it is standard practice to use a version compatible with the requirements of babel-plugin-react-compiler. You may also need to install @types/babelcore if you are using TypeScript [1][2].
Citations:
- 1: https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react/README.md
- 2: https://npmx.dev/package/@vitejs/plugin-react/v/6.0.3
- 3: https://github.com/vitejs/vite-plugin-react/releases/tag/plugin-react@6.0.0
- 4: https://github.com/vitejs/vite-plugin-react/releases/tag/plugin-react%406.0.0
- 5: https://registry.npmjs.org/%40vitejs%2Fplugin-react
Add @babel/core to devDependencies. reactCompilerPreset() requires it alongside @rolldown/plugin-babel and babel-plugin-react-compiler, so this example can fail to install or build without 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/kitchen-sink-start/package.json` around lines 30 - 38, The
kitchen-sink-start package is missing a required devDependency for the React
compiler setup. Add `@babel/core` to the devDependencies in the package.json used
by the React example so reactCompilerPreset() can work with
`@rolldown/plugin-babel` and babel-plugin-react-compiler.
| .text-input, | ||
| .number-input, | ||
| .icon-button { | ||
| border: 1px solid currentColor; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint: currentColor casing violation (value-keyword-case).
Flagged by Stylelint at all three occurrences; CSS keyword casing should be lowercase.
🎨 Proposed fix
- border: 1px solid currentColor;
+ border: 1px solid currentcolor;(apply at Lines 187, 241, and 247)
Also applies to: 241-241, 247-247
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 187-187: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/kitchen-sink-start/src/index.css` at line 187, Update the CSS
declarations in the kitchen-sink stylesheet so the `currentColor` keyword uses
lowercase `currentcolor` at each flagged border rule. Make the same casing
change in all affected declarations in the `index.css` file, including the ones
associated with the style blocks near the repeated border definitions, to
satisfy the `value-keyword-case` rule.
Source: Linters/SAST tools
Comment on lines
+689
to
+723
| const table = useTable( | ||
| { | ||
| key: 'kitchen-sink', // needed for devtools | ||
| features, | ||
| columns, | ||
| data, | ||
| getSubRows: (row) => row.subRows, | ||
| globalFilterFn: 'fuzzy', | ||
| columnResizeMode: 'onChange', | ||
| defaultColumn: { minSize: 200, maxSize: 800 }, | ||
| initialState: { | ||
| columnOrder: columns.map((c) => c.id!), | ||
| columnPinning: { left: ['select'], right: [] }, | ||
| pagination: { pageIndex: 0, pageSize: 20 }, | ||
| }, | ||
| keepPinnedRows: true, | ||
| debugTable: true, | ||
| autoResetExpanded: false, // keep expanded rows during filtering changes | ||
| }, | ||
| (state) => state, // default selector | ||
| ) | ||
|
|
||
| useTanStackTableDevtools(table) | ||
|
|
||
| // memoized column-size css variables (column-resizing-performant pattern) | ||
| const columnSizeVars = React.useMemo(() => { | ||
| const headers = table.getFlatHeaders() | ||
| const colSizes: Record<string, number> = {} | ||
| for (let i = 0; i < headers.length; i++) { | ||
| const header = headers[i] | ||
| colSizes[`--header-${header.id}-size`] = header.getSize() | ||
| colSizes[`--col-${header.column.id}-size`] = header.column.getSize() | ||
| } | ||
| return colSizes | ||
| }, [table.state.columnResizing, table.state.columnSizing]) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
App still fully re-renders per resize tick — contradicts the PR's resizing-performance goal.
useTable(options, (state) => state) subscribes App to the entire table state, and columnSizeVars is recomputed via React.useMemo keyed on columnResizing/columnSizing. Every RAF-batched resize tick therefore re-renders the whole App tree (all visible rows, toolbar, state dump), which is exactly the per-tick re-render problem the sibling column-resizing-performant examples in this same PR eliminate by switching to CSS-variable writes via useLayoutEffect/Subscribe/atoms instead of a full-state-driven useMemo. The comment (column-resizing-performant pattern) is now stale — it references the pre-refactor approach.
💡 Suggested direction
- const table = useTable(
- {...},
- (state) => state, // default selector
- )
+ const table = useTable(
+ {...},
+ (state) => state, // TODO: replace with a narrow selector for App-level re-renders
+ )
- // memoized column-size css variables (column-resizing-performant pattern)
- const columnSizeVars = React.useMemo(() => {
- const headers = table.getFlatHeaders()
- const colSizes: Record<string, number> = {}
- for (let i = 0; i < headers.length; i++) {
- const header = headers[i]
- colSizes[`--header-${header.id}-size`] = header.getSize()
- colSizes[`--col-${header.column.id}-size`] = header.column.getSize()
- }
- return colSizes
- }, [table.state.columnResizing, table.state.columnSizing])
+ // write CSS vars imperatively on a table ref + subscribe only to
+ // columnSizing/columnResizing atoms, mirroring the CSS-variable
+ // pattern used by the other column-resizing-performant examples.🤖 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/kitchen-sink-start/src/routes/index.tsx` around lines 689 -
723, The resizing path in the App component is still subscribing to the full
table state through useTable(..., (state) => state), which forces a full
re-render on each column-resize tick. Update the kitchen-sink route to follow
the same non-rendering pattern used by the sibling column-resizing-performant
examples: move the column-size CSS variable updates out of React
render/memoization and into a side-effect or subscribed atom/selector approach,
so App does not re-render while resizing. Use the existing useTable,
useTanStackTableDevtools, and columnSizeVars setup as the place to refactor.
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
Bug Fixes
Documentation
Tests