fix(table-core): correct expanded/paginated state contents and sorting toggle defaults - #6501
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 9b40f22
☁️ 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 (32)
🚧 Files skipped from review as they are similar to previous changes (32)
📝 WalkthroughWalkthroughThe PR updates row expansion and pagination behavior, sorting contracts and direction inference, sorting documentation across frameworks, and Mantine and Material React example behavior and smoke tests. ChangesExpansion and pagination semantics
Sorting semantics
React example integration
Estimated code review effort: 4 (Complex) | ~45 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 |
![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: 4
🧹 Nitpick comments (1)
examples/react/mantine-react-table/src/mantine-react-table/components/head/MRT_TableHeadCellSortLabel.tsx (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
table.Subscribeover the standaloneSubscribeimport.This component imports the standalone
Subscribecomponent. The TanStack Table v9 migration guide states that "For table.Subscribe from useTable, prefer that API — it uses overloads so JSX contextual typing works. This standalone component uses a union props type." The Material counterpart of this component usestable.Subscribedirectly. Usetable.Subscribehere too for better type inference and consistency across the Mantine and Material examples.♻️ Proposed refactor
-import { Subscribe } from '`@tanstack/react-table`' +return ( - <Subscribe source={table.atoms.sorting}> + <table.Subscribe source={table.atoms.sorting}> {(sorting) => {}} - </Subscribe> + </table.Subscribe> )Also applies to: 31-32, 96-96
🤖 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/mantine-react-table/src/mantine-react-table/components/head/MRT_TableHeadCellSortLabel.tsx` at line 4, Remove the standalone Subscribe import and update the component’s Subscribe usages to call table.Subscribe instead, matching the Material counterpart and preserving the existing subscription behavior and JSX content.
🤖 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 `@docs/framework/alpine/guide/expanding.md`:
- Around line 279-280: Correct the getCanExpand() wording to state that rows are
expanded only when row.getCanExpand() returns true, not when it “accepts” rows.
Apply this wording change in docs/framework/alpine/guide/expanding.md lines
279-280, docs/framework/angular/guide/expanding.md lines 254-255,
docs/framework/ember/guide/expanding.md lines 270-271,
docs/framework/lit/guide/expanding.md lines 273-274,
docs/framework/octane/guide/expanding.md lines 258-259, and
docs/framework/preact/guide/expanding.md lines 258-259.
In `@docs/reference/index/interfaces/Column_RowSorting.md`:
- Around line 142-144: Update the documentation for column_getNextSortingOrder
to state that enableSortingRemoval is the global gate for removing sorting,
while enableMultiRemove applies the additional restriction for multi-sort
toggles.
In `@packages/table-core/src/features/row-expanding/rowExpandingFeature.utils.ts`:
- Around line 80-81: The TSDoc for table_toggleAllRowsExpanded must clarify that
having no expandable rows is a no-op only for expansion requests; collapsing
must still clear stale expanded IDs. Update the no-op description near
table_toggleAllRowsExpanded without changing the implementation.
- Around line 94-97: Update the guard in the all-rows expansion logic to treat
any state where table_getIsAllRowsExpanded(table) is true as already expanded,
not only currentExpanded === true. Preserve the existing partial-expansion and
non-expandable checks, and add a regression test covering
table_toggleAllRowsExpanded(table, true) with an explicit map containing every
expandable row ID to verify onExpandedChange is not called.
---
Nitpick comments:
In
`@examples/react/mantine-react-table/src/mantine-react-table/components/head/MRT_TableHeadCellSortLabel.tsx`:
- Line 4: Remove the standalone Subscribe import and update the component’s
Subscribe usages to call table.Subscribe instead, matching the Material
counterpart and preserving the existing subscription behavior and JSX content.
🪄 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: bbbc38de-d807-4682-874f-986352f71ef5
📒 Files selected for processing (42)
beta-window-triage.mddocs/framework/alpine/guide/expanding.mddocs/framework/alpine/guide/sorting.mddocs/framework/angular/guide/expanding.mddocs/framework/angular/guide/sorting.mddocs/framework/ember/guide/expanding.mddocs/framework/ember/guide/sorting.mddocs/framework/lit/guide/expanding.mddocs/framework/lit/guide/sorting.mddocs/framework/octane/guide/expanding.mddocs/framework/octane/guide/sorting.mddocs/framework/preact/guide/expanding.mddocs/framework/preact/guide/sorting.mddocs/framework/react/guide/expanding.mddocs/framework/react/guide/sorting.mddocs/framework/solid/guide/expanding.mddocs/framework/solid/guide/sorting.mddocs/framework/svelte/guide/expanding.mddocs/framework/svelte/guide/sorting.mddocs/framework/vue/guide/expanding.mddocs/framework/vue/guide/sorting.mddocs/reference/index/interfaces/ColumnDef_RowSorting.mddocs/reference/index/interfaces/Column_RowSorting.mddocs/reference/static-functions/functions/column_getAutoSortDir.mddocs/reference/static-functions/functions/column_toggleSorting.mdexamples/react/mantine-react-table/src/mantine-react-table/components/buttons/MRT_ToggleRowActionMenuButton.tsxexamples/react/mantine-react-table/src/mantine-react-table/components/head/MRT_TableHeadCellSortLabel.tsxexamples/react/mantine-react-table/tests/e2e/smoke.spec.tsexamples/react/material-react-table/src/main.tsxexamples/react/material-react-table/src/material-react-table/components/buttons/MRT_ToggleRowActionMenuButton.tsxexamples/react/material-react-table/src/material-react-table/components/head/MRT_TableHeadCellSortLabel.tsxexamples/react/material-react-table/src/material-react-table/features/mrtRefsFeature.tsexamples/react/material-react-table/tests/e2e/smoke.spec.tspackages/angular-table/tests/flex-render/flex-render-table.test.tspackages/ember-table/tests/integration/flex-render.test.gtspackages/table-core/src/features/row-expanding/rowExpandingFeature.utils.tspackages/table-core/src/features/row-pagination/createPaginatedRowModel.tspackages/table-core/src/features/row-sorting/rowSortingFeature.types.tspackages/table-core/src/features/row-sorting/rowSortingFeature.utils.tspackages/table-core/tests/implementation/features/row-pagination/createPaginatedRowModel.test.tspackages/table-core/tests/unit/features/row-expanding/rowExpandingFeature.utils.test.tspackages/table-core/tests/unit/features/row-sorting/rowSortingFeature.utils.test.ts
Comment on lines
+279
to
+280
| Toggling is guarded and de-duplicated. `row.toggleExpanded()` and `table.toggleAllRowsExpanded()` only expand rows that `row.getCanExpand()` accepts, and they skip `onExpandedChange` entirely when the requested state already matches the current state. Collapsing is always allowed, so stale ids left in a controlled `expanded` map can still be cleared. | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the getCanExpand() wording.
getCanExpand() returns a boolean. It does not accept rows. Replace “rows that row.getCanExpand() accepts” with “rows for which row.getCanExpand() returns true”.
docs/framework/alpine/guide/expanding.md#L279-L280: Correct the predicate wording.docs/framework/angular/guide/expanding.md#L254-L255: Apply the same wording correction.docs/framework/ember/guide/expanding.md#L270-L271: Apply the same wording correction.docs/framework/lit/guide/expanding.md#L273-L274: Apply the same wording correction.docs/framework/octane/guide/expanding.md#L258-L259: Apply the same wording correction.docs/framework/preact/guide/expanding.md#L258-L259: Apply the same wording correction.
📍 Affects 6 files
docs/framework/alpine/guide/expanding.md#L279-L280(this comment)docs/framework/angular/guide/expanding.md#L254-L255docs/framework/ember/guide/expanding.md#L270-L271docs/framework/lit/guide/expanding.md#L273-L274docs/framework/octane/guide/expanding.md#L258-L259docs/framework/preact/guide/expanding.md#L258-L259
🤖 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/expanding.md` around lines 279 - 280, Correct the
getCanExpand() wording to state that rows are expanded only when
row.getCanExpand() returns true, not when it “accepts” rows. Apply this wording
change in docs/framework/alpine/guide/expanding.md lines 279-280,
docs/framework/angular/guide/expanding.md lines 254-255,
docs/framework/ember/guide/expanding.md lines 270-271,
docs/framework/lit/guide/expanding.md lines 273-274,
docs/framework/octane/guide/expanding.md lines 258-259, and
docs/framework/preact/guide/expanding.md lines 258-259.
Comment on lines
+142
to
+144
| Returns the next sorting order. Pass `multi` to resolve the order for a | ||
| multi-sort toggle, where `enableMultiRemove` governs whether the cycle can | ||
| remove the sort. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the global removal setting as well.
enableSortingRemoval remains the global removal gate in column_getNextSortingOrder; enableMultiRemove adds a multi-sort-specific restriction. Update this text so readers do not assume enableMultiRemove alone controls removal.
🤖 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/reference/index/interfaces/Column_RowSorting.md` around lines 142 - 144,
Update the documentation for column_getNextSortingOrder to state that
enableSortingRemoval is the global gate for removing sorting, while
enableMultiRemove applies the additional restriction for multi-sort toggles.
…g toggle defaults
Two independent clusters of default-behavior bugs from the beta triage.
Expanded and paginated state contents:
Expand-all materialized every id in `rowsById`, including leaf rows that
can never expand, so a serialized `ExpandedState` was polluted with dead
keys and `getExpandedDepth` was skewed by them. Materialization now writes
only ids where `row.getCanExpand()` is true, chosen over `subRows.length`
so `getRowCanExpand` lazy-load overrides stay expandable.
`getExpandedDepth` filters the same way, and `getIsAllRowsExpanded` now
only considers expandable rows so the materialized map still round-trips
as "all expanded" (a map of stale ids with no expandable rows is false).
Paginated `flatRows` pushed each row and then recursed into its subRows,
but page rows already contain expanded descendants inline, so those rows
appeared twice. The rebuild now dedupes by row id. Collapsed descendants
stay included, consistent with every other row model, where `flatRows`
ignores expansion state.
`row.toggleExpanded(bool)` and `table.toggleAllRowsExpanded(bool)` fired
`onExpandedChange` even when the requested state already matched, so
controlled consumers got spurious callbacks. Both now early-return.
Neither toggle consulted `row.getCanExpand()`, unlike their row-selection
equivalents, so a non-expandable row could be written into expanded state
imperatively. The expand direction is now guarded; collapsing is always
allowed so stale expanded ids can still be cleaned up.
Sorting toggle defaults:
`column.toggleSorting()` called `column.getNextSortingOrder(column)` with
no `multi` argument, so `enableMultiRemove` was dead on every path. The
argument is now forwarded as `multi && column.getCanMultiSort()`, matching
the multi-mode condition the updater itself uses. The public
`getNextSortingOrder` type now accepts the `multi` argument it always
supported at runtime.
`column.getAutoSortDir()` sampled only `flatRows[0]`, so a leading null or
a manual-sorting data swap flipped the inferred first direction mid-cycle
and silently dropped a state from the toggle cycle. It now samples the
first 10 rows for a non-nullish value, matching `getAutoSortFn`.
Docs: `sortUndefined: false` was documented as "considered tied" in all
framework sorting guides; undefined values are actually passed straight to
the sorting function with no special handling. Corrected, and the missing
`'first'`/`'last'` bullets were added to the generated reference.
Also fixes three crashes in the MRT examples surfaced while testing the
sorting changes. The MUI and Mantine sort labels read `sorting` off
`state`, which does not carry it, and crashed on `sorting.length`; they now
read it through `table.Subscribe` on `table.atoms.sorting`, which also
makes the badge reactive. The row-action edit handler passed a shallow row
copy to `setEditingRow`, breaking the row identity the edit modal needs.
`mrtRefsFeature` seeded the keyed `editInputRefs`/`filterInputRefs` bags
with `null` instead of `{}`, crashing on first keyed write. New e2e
coverage pins all three plus the sort-direction indicator.
Closes #6115
Closes #5833
Closes #6136
Closes #4939
Closes #4946
Closes #5147
Closes #5832
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KevinVandy
force-pushed
the
fix/expanding-sorting-state-contents
branch
from
August 3, 2026 03:12
9b40f22 to
c35d70d
Compare
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.
Clears clusters 3 and 4 from the beta triage: expanded/paginated state contents, and sorting toggle defaults. Both are default-behavior changes, which is why they belong in the beta window rather than a later 9.x.
Expanded and paginated state contents
#6115 — expand-all materializes non-expandable ids. Toggling a row while
expanded === truematerialized every id inrowsById, including leaf rows that can never expand. That polluted serializedExpandedStateand skewedgetExpandedDepth. Materialization now writes only ids whererow.getCanExpand()is true.getCanExpandwas chosen oversubRows.lengthso thatgetRowCanExpandlazy-load overrides stay expandable.Companion change:
table.getIsAllRowsExpanded()now only considers expandable rows, so the materialized map still round-trips as "all expanded". A side effect worth calling out in review: a state of stale ids with no expandable rows in the current model now returnsfalserather thantrue.#5833 — paginated
flatRowsduplicates expanded sub-rows. The rebuild pushed each page row and then recursed into itssubRows, but page rows already contain expanded descendants inline, so those rows landed inflatRowstwice. Now deduped by row id. Semantic decision: collapsed descendants stay included, consistent with every other row model, whereflatRowsignores expansion state. Only the duplication is removed.#6136 — spurious
onExpandedChange.row.toggleExpanded(bool)andtable.toggleAllRowsExpanded(bool)fired the change handler even when the requested state already matched. Both now early-return.#4939 — imperative toggles ignore
getCanExpand. Neither toggle consultedrow.getCanExpand(), unlike their row-selection equivalents. The expand direction is now guarded. Collapsing is always allowed so stale expanded ids can still be cleaned up.This last one has fallout for consumers who drove expansion on flat rows without setting
getRowCanExpand: those calls become no-ops. The angular and ember flex-render tests did exactly that and are updated withgetRowCanExpand: () => true.Sorting toggle defaults
#4946 —
enableMultiRemovewas dead on every path.column.toggleSorting()calledcolumn.getNextSortingOrder(column)without themultiargument, so the option could never take effect. Now forwarded asmulti && column.getCanMultiSort(), which matches the multi-mode condition the updater itself computes, so a multi toggle on a column that cannot multi-sort correctly falls back to single-sort removal semantics. The publicgetNextSortingOrdertype now accepts themultiargument it always supported at runtime.#5147 / #5832 — auto sort direction flips the toggle cycle.
column.getAutoSortDir()sampled onlyflatRows[0], so a leading null or a manual-sorting data swap changed the inferred first direction mid-cycle, and the cycle went asc → removed without ever reaching desc. It now samples the first 10 filtered rows for the first non-nullish value, matchinggetAutoSortFn's existing N=10 convention. #5832 is the same root cause and closes as a duplicate.#5653 (docs half only).
sortUndefined: falsewas documented as "undefined values will be considered tied" in all ten framework sorting guides and the reference. That is wrong: undefined values are passed straight to the sorting function with no special handling, and the built-in text fns coerce them to''. Corrected, and the missing'first'/'last'bullets were added to the generated reference page.The semantic half of #5653 — whether the default
sortUndefined: 1should short-circuit before a user's customsortFn— is deliberately not addressed here. Current behavior is defensible and now accurately documented; users who want their fn to see undefined can setsortUndefined: false.MRT example crashes
Surfaced while exercising the sorting changes against the MUI and Mantine example ports, not caused by them.
sortingoffstate, which does not carry it, and crashed onsorting.length. They now read it throughtable.Subscribeontable.atoms.sorting, which also makes the multi-sort badge reactive.setEditingRow({ ...row })) instead of the row, breaking the row identity the edit modal depends on.mrtRefsFeatureseeded the keyededitInputRefs/filterInputRefsbags withnullinstead of{}, crashing on the first keyed write when filters or edit inputs mounted.keyin the MUI example's row-action menu.New e2e coverage pins all of these plus the sort-direction indicator, per the existing per-example self-contained spec convention.
Verification
tscand declaration-emit clean.packages/table-core/testsare untouched by this PR.Closes #6115, closes #5833, closes #6136, closes #4939, closes #4946, closes #5147, closes #5832.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests