Repository navigation
Conversation
useTableSortableState passed a new options object to useCollator on every render, so it got a new collator each time. Its sorted data depends on the collator, so every render of its owner re-sorted all rows, even with the same data and sort. The collator options are now one module constant. Sorting runs only when the data, the sort or the locale changes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TableBody | 3.5 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
| TableFooter | 3.5 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
| TableHeader | 3.5 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
5 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TableRow | 7.1 kB (0) · 4.0 kB CSS (0) | — | ✓ · ✓ · ✗ |
| Table | 130.4 kB (0) · 18.5 kB CSS (0) | — | ✓ · ✓ · — |
| TableCell | 114.5 kB (0) · 18.0 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TableHeaderCell | 114.0 kB (0) · 17.7 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TableSelectionToolbar | 73.7 kB (0) · 13.2 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 7.36 MB packed (+463 B); no new dependencies.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
74c4e0e to
02fd3be
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides on component:Table/DEC-6 and the new architecture:react-component-runtime INV4 exception. Those records change a current runtime rule. Any engineering owner, the author included, records the decision by commenting /approve-spec <full head SHA>, and DEC-6's Decider line should then name that owner. These two fixes are needed whatever the decision:
- The new INV4 sentence (
docs/architecture/react-component-runtime.md:135-137) and DEC-6 (docs/specs/components/Table/Table.spec.md:564-566) say a config change re-renders only the rows it changes, but changing stickystartKeyspins the header cell and leaves its body cells unpinned, and a newcomparatorsfunction leaves the rows in their old order, so the records, the eight new code comments and thecomparatorsdocs should state that real consequence, or those two refs should follow real value changes. - With the collator fix (
useTableSortableState.tsx:238, 273), the rendered order can come from an earlier render'scomparators, which current INV1 ("Rendered output MUST derive from current props, state, and context") still forbids, so the recorded exception should name INV1 too, orcomparatorsshould stay reactive.
Non-blocking: the new comments could each be split into short active sentences, for example "This ref changes during render. A new config object with the same values then re-renders no row."
[Automated review]
|
/approve-spec 02fd3be |
github-actions
Bot
removed
the
needs:spec-owner-review
02fd3be to
f14ace6
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval is on hold until an engineering owner approves component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions at this head. The earlier /approve-spec named a previous head, and this push rewrote DEC-6's consequence paragraph and the INV4 exception and added the INV1 exception, so that approval no longer applies. Any engineering owner, the author included, records the decision by commenting /approve-spec <full head SHA>; DEC-6's Decider line names cixzhang, so the approval should be hers or the line should name the owner who approves.
The two fixes the last review asked for are in, and no blocking finding remains. Two small wording points:
- DEC-6, the INV1 exception, the
comparatorsdocs and the code comment say a new comparator applies on the next data or sort change, but a locale change applies it too, as the changeset already says. - In
useTableStickyColumns.tsx, the older comment above the ref still says it never reads stale config, which the new comment below it contradicts.
[Automated review]
Table's plugin hooks keep their latest config in refs written during render: filtering, pagination, selection, sortable and tree data, the sticky columns' keys, and the sort state's comparators and change callback. They keep each plugin object stable, so a new config object with the same values re-renders no row. A changed value reaches a memoized row only when that row renders for another reason, and a new comparator applies on the next data, sort or locale change. DEC-6 in the Table spec records them as an exception to the runtime's ref rule. The runtime record states it in INV4, and in INV1 for the comparators. A note at each write says so. Five already had a test that fails when the ref moves into the memo's dependencies. Two new tests cover the sticky keys and the sort callback; the comparators' test comes with the sort fix underneath.
f14ace6 to
f3d8f40
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner approves component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions at this head. The earlier /approve-spec named a previous head, and DEC-6, the INV4 exception and the INV1 exception have changed since, so that approval no longer applies. Any engineering owner, the author included, records the decision by commenting /approve-spec <full head SHA>; DEC-6's Decider line names cixzhang, so the approval should be hers or the line should name the owner who approves.
The locale wording and the sticky comment are fixed and no blocking finding remains, with two non-blocking points:
- Miss: DEC-6 does not say that a header press runs the
onSortChangefrom the render of the last sort change, because the sort config copiesonSortChangeRef.current(useTableSortableState.tsx:314-322); say so in DEC-6, or passuseTableSortablea stablenext => onSortChangeRef.current(next). - Five new plugin-hook comments open with a passive fragment and join two topics; one shorter form is "Render writes this ref on purpose. A new config object with the same values then re-renders no row."
[Automated review]
| // Render writes this ref on purpose, an accepted exception to the ref rule. | ||
| // An inline callback then keeps the sort config stable. |
There was a problem hiding this comment.
Non-blocking (Miss): the sort config below copies onSortChangeRef.current when it is computed (lines 314-322), and it recomputes only when sort, allowUnsortedState or isMultiSortEnabled changes. A header press then runs the callback from the last sort change: the first press after a parent passes a new onSortChange runs the old one, and a controlled table whose onSortChange goes from undefined to a setter does not sort. Main does the same, but DEC-6 now decides this ref. Either add "A new change callback runs from the next sort change" to DEC-6, or hand useTableSortable onSortChange: next => onSortChangeRef.current(next) so the latest callback always runs.
| // Written during render on purpose, an accepted exception to the ref rule: | ||
| // the plugin object stays stable, so a config change re-renders no row. |
There was a problem hiding this comment.
Non-blocking (INV29): this opens with a passive fragment and joins two topics in one sentence; the same comment is in useTablePagination.tsx, useTableSelection.tsx, useTableSortable.tsx and useTableTreeData.tsx. One shorter form: "Render writes this ref on purpose. A new config object with the same values then re-renders no row."
|
/approve-spec f3d8f40 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so CI runs once it is marked ready for review and a maintainer adds the ci:run label.
component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions carry an engineering owner's approval for this head, and the new render-cost tests, the comparators test that fails on main and the Table suite back them.
Optional follow-ups, none blocking:
- DEC-6 could say that a header press runs the
onSortChangefrom the last sort change, so the first press after a parent passes a new callback runs the old one (main does the same). - The five identical plugin-hook comments could read "Render writes this ref on purpose. A new config object with the same values then re-renders no row."
- The INV4 exception could open with "
component:Table/DEC-6makes an exception" instead of the dated owner decision.
[Automated review]
cixzhang
added
the
ci:run
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 8ee7a81 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so CI runs once it is marked ready for review and a maintainer adds the ci:run label.
component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions carry an engineering owner's approval for this head. The comparators test fails without the collator fix and passes here, each new render-cost test fails once its ref joins the memo's dependencies, and the Table suite passes.
Optional follow-ups, none blocking:
- DEC-6 could say that a header press runs the
onSortChangefrom the last sort change, so the first press after a parent passes a new callback runs the old one (this predates the pull request). - The five identical plugin-hook comments could read "Render writes this ref on purpose. A new config object with the same values then re-renders no row."
- The INV4 exception could open with "
component:Table/DEC-6makes an exception" instead of the dated owner decision.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec c2a9a1e |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits for CI: CI has not run on this head yet. The pull request is still a draft, so marking it ready for review starts CI; the fork branch already carries ci:run.
component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions carry an engineering owner's approval for this head, and no blocking issue remains.
Optional follow-ups, none blocking:
- INV4 now states the Table and Markdown exceptions in two styles and two places; one closing passage that names both, and limits Table's part to "the eight config refs
component:Table/DEC-6lists", would read as one rule. - The Deciding specs entry for
component:Table/DEC-6could cite INV1 as well as INV4.
Claims and files checked
Claims:
- Changeset: Table's plugin hooks record their render-time config refs as an exception (DEC-6), with tests that pin what they keep fast. Verified: Table.spec.md:555-573, react-component-runtime.md:118-121 and :139-143, and the tests at useTableStickyColumns.test.tsx:363 and useTableSortableState.test.tsx:353.
- Eight refs hold the latest config, and rendered output reads them. Verified at the eight writes: useTableFiltering.tsx:1169, useTablePagination.tsx:265, useTableSelection.tsx:368, useTableSortable.tsx:371, useTableTreeData.tsx:532, useTableStickyColumns.tsx:498, useTableSortableState.tsx:293 and :298.
- DEC-6's consequences: same values re-render no row, a changed value reaches a row only when it renders, sticky keys pin the header first, and a comparator applies at the next data, sort or locale change. Verified in code: BaseTable.tsx:521, useTableStickyColumns.tsx:654 and :695, useTableSortableState.tsx:301-304.
- Rejected alternative: config in memo dependencies re-creates the plugin object. Verified from the memo dependencies (useTableSortable.tsx:457, useTableStickyColumns.tsx:760).
- Three spots follow the rule. Verified for column settings (useTableColumnSettingsState.tsx:182-184); the column and plugin caches are left to #10940, an open draft, as stated.
- Each of the eight has a test that fails when its ref joins its memo's dependencies. The named tests exist; the mutation runs could not be repeated here.
- A note at each write says it stays written during render on purpose. Verified: eight comments.
- Validation runs (check:knowledge, Prettier, the 771-test Table suite, typecheck, ESLint) describe an earlier commit and could not be repeated here.
- The second base merge keeps both pull requests' cases in useTableSortableState.test.tsx, and the change's own diff is unchanged. Verified: tests at :320 and :353; the record, comment and test hunks match the earlier patch.
- The owner records the decision on this pull request. Verified: an engineering owner's
/approve-specnames this head.
Files:
- .changeset/table-plugin-refs-exception.md: clear
- docs/architecture/react-component-runtime.md: non-blocking notes (two exception styles in INV4, Table wording wider than DEC-6, Deciding specs cites only INV4)
- docs/specs/components/Table/Table.spec.md: non-blocking note (DEC-6 omits the change callback's timing)
- packages/core/src/Table/plugins/filtering/useTableFiltering.tsx: non-blocking INV29 style note
- packages/core/src/Table/plugins/pagination/useTablePagination.tsx: non-blocking INV29 style note
- packages/core/src/Table/plugins/selection/useTableSelection.tsx: non-blocking INV29 style note
- packages/core/src/Table/plugins/sortable/useTableSortable.tsx: non-blocking INV29 style note
- packages/core/src/Table/plugins/sortable/useTableSortableState.doc.mjs: clear
- packages/core/src/Table/plugins/sortable/useTableSortableState.test.tsx: clear
- packages/core/src/Table/plugins/sortable/useTableSortableState.tsx: clear
- packages/core/src/Table/plugins/stickyColumns/useTableStickyColumns.test.tsx: clear
- packages/core/src/Table/plugins/stickyColumns/useTableStickyColumns.tsx: clear
- packages/core/src/Table/plugins/tree/useTableTreeData.tsx: non-blocking INV29 style note
| The owner's decision of 2026-11-09 (`component:Table/DEC-6`) makes an | ||
| exception for Table's plugin hooks: they MAY keep their latest config in refs | ||
| written during render. A new config object with the same values re-renders no | ||
| row. A changed value reaches a memoized row only when that row renders for | ||
| another reason. |
There was a problem hiding this comment.
Non-blocking: after the base merge, INV4 states two exceptions in two styles. This one sits mid-paragraph; Markdown's comes at the end as "One exception is…". One closing passage that names both decisions would read as one rule. Naming "the eight config refs component:Table/DEC-6 lists" here would also keep the exception from covering the column-resize and column-settings hooks, which sync their config in layout effects.
| - `component:Table/DEC-6` — Table's plugin hooks may keep their latest config | ||
| in refs written during render (INV4). |
There was a problem hiding this comment.
Non-blocking: DEC-6 also adds the INV1 exception for the sort comparators (line 118), so this entry could end "(INV1, INV4)".
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is still a draft, so marking it ready for review starts CI; the fork branch already carries ci:run.
component:Table/DEC-6 and the architecture:react-component-runtime INV4 and INV1 exceptions carry an engineering owner's approval for this head, and no blocking issue remains.
Optional follow-ups, none blocking:
- DEC-6 and the INV4 exception could say that a render React discards, such as a suspended transition, leaves its config in these refs until the next committed render; Markdown's exception states its equivalent.
- INV4 now states the Table and Markdown exceptions in two styles and places; one closing passage that names both, and limits Table's part to the eight refs DEC-6 lists, would read as one rule.
Claims and files checked
Claims:
- Changeset: the plugin hooks record their render-time config refs as an exception, with tests that pin what they keep fast. Verified against the new decision, both runtime exceptions and the two new tests.
- Eight refs hold the latest config and rendered output reads them; the ref invariant forbids that without a recorded exception. Verified at the eight writes. The description also names the Effects invariant, which does not govern refs.
- The earlier Table decisions do not cover plugin config, and no module record or open pull request settles it. Verified.
- Three other spots follow the rule, two of them through a separate open pull request. Verified for column settings; the caches still write during render at this head, as the description implies.
- Paging and sorting stay as fast as today. Verified: outside tests the diff is comments, JSDoc and one doc string.
- The decision's list and consequences, and the rejected alternative. Verified in the row memo, the header rendering and each hook's memo dependencies.
- Each ref has a test that fails when the ref moves into its memo's dependencies. Test names verified; the new tests failed under that mutation at an earlier commit with the same patch.
- A note at each write says it stays written during render on purpose. Verified: eight comments.
- The validation runs listed in the description. They describe an earlier commit with the same patch and are corroborated there; they could not be repeated on this head.
- The base merges keep both pull requests' test cases and leave the change's own diff unchanged. Verified.
- An engineering owner records the decision on this pull request. Verified for this head.
Files:
.changeset/table-plugin-refs-exception.md: cleardocs/architecture/react-component-runtime.md: non-blocking notesdocs/specs/components/Table/Table.spec.md: non-blocking notespackages/core/src/Table/plugins/filtering/useTableFiltering.tsx: non-blocking comment-style notepackages/core/src/Table/plugins/pagination/useTablePagination.tsx: non-blocking comment-style notepackages/core/src/Table/plugins/selection/useTableSelection.tsx: non-blocking comment-style notepackages/core/src/Table/plugins/sortable/useTableSortable.tsx: non-blocking comment-style notepackages/core/src/Table/plugins/sortable/useTableSortableState.doc.mjs: clearpackages/core/src/Table/plugins/sortable/useTableSortableState.test.tsx: clearpackages/core/src/Table/plugins/sortable/useTableSortableState.tsx: clearpackages/core/src/Table/plugins/stickyColumns/useTableStickyColumns.test.tsx: clearpackages/core/src/Table/plugins/stickyColumns/useTableStickyColumns.tsx: clearpackages/core/src/Table/plugins/tree/useTableTreeData.tsx: non-blocking comment-style note
| The owner's decision of 2026-11-09 (`component:Table/DEC-6`) makes an | ||
| exception for Table's plugin hooks: they MAY keep their latest config in refs | ||
| written during render. A new config object with the same values re-renders no | ||
| row. A changed value reaches a memoized row only when that row renders for | ||
| another reason. |
There was a problem hiding this comment.
Non-blocking: a render React discards, as a suspended transition does, still writes these refs, and nothing rolls them back. Until the next committed render, a selection click calls onSelectItem from that config (useTableSelection.tsx:323); useTableColumnSettingsState.tsx:180-184 syncs its config in a layout effect for exactly this reason. Markdown's exception below states its own discarded-render effect. One sentence here and in DEC-6 would do the same, for example: "A render React discards leaves its config in the refs until the next committed render."
Exact decision needed
Whether eight refs in Table's plugin hooks may stay written during render. Each holds the hook's latest config, and rendered output reads it.
architecture:react-component-runtimeINV4 says a ref mutation must not be the sole source for a value later read to produce rendered output.Existing authority searched
architecture:react-component-runtimeINV3 and INV4, which forbid this without a recorded exception.#4690,#8403and#8861on these files. None settles this.Decision
The spec owner's decision for Table: three spots follow the rule, and eight stay as documented exceptions. Paging and sorting stay as fast as today. The owner records it on this pull request.
component:Table/DEC-6records the eight.architecture:react-component-runtimestates the exception in INV4, and in INV1 for the comparators. DEC-6 covers the filtering, pagination, selection, sortable and tree data configs, the sticky columns' keys, and the sort state's comparators and change callback. They keep each plugin object, the sorted data and the sort config stable. A new config object with the same values re-renders no row. A changed value reaches a memoized row only when that row renders for another reason. Changed sticky keys pin the header first. A new comparator applies on the next data, sort or locale change.Rejected alternatives
Moving the config into state or memo dependencies. It re-creates the plugin object on each change, and every row re-renders.
Claim scope and non-goals
DEC-6 covers these eight refs only. The three spots that follow the rule are column settings' active keys, merged before this PR, and the column and plugin caches in #10940. Other refs keep the rule.
Evidence and implementation relationship
Each of the eight has a test that fails when its ref moves into its memo's dependencies. The table under Validation names them. Five existed. This PR adds the sticky keys' and the sort callback's. The comparators' test comes with #10942, the sort fix this PR stacks on.
A note at each write says it stays written during render on purpose.
Writing rubric results
verified_by, and DEC-6 keeps only the conclusion.Scope
Components: Table
verified_bynames.Validation
All runs below are at f3d8f40, stacked on the sort fix at 60e22db.
useTableFiltering.tsxuseTablePagination.tsxuseTableSelection.tsxuseTableSortable.tsxuseTableTreeData.tsxuseTableStickyColumns.tsxuseTableSortableState.tsxuseTableSortableState.tsxpnpm check:knowledge: knowledge templates and records are aligned.git diff --check: clean.pnpm exec vitest run packages/core/src/Table --project ui: 41 files, 771 tests pass.Table.perf.test.tsx. Each failed as the table says.@astryxdesign/coretypecheck: 0 errors. ESLint on the changed files: clean.useTableSortableState.test.tsx, keeps both PRs' cases. The change's own diff is unchanged.