fix: table-core's getIsAllSubRowsSelected returns false when subrows are unselectable#6177
Open
kwengontra wants to merge 4 commits intoTanStack:mainfrom
Open
fix: table-core's getIsAllSubRowsSelected returns false when subrows are unselectable#6177kwengontra wants to merge 4 commits intoTanStack:mainfrom
kwengontra wants to merge 4 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: c4bd664 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughRefactors row-selection aggregation to count only selectable sub-rows via a new BFS helper ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/table-core/tests/RowSelection.test.ts (1)
464-597:⚠️ Potential issue | 🟡 MinorMissing test:
getIsAllSubRowsSelectedwhen some sub-rows are non-selectable but all selectable ones are selected.This is the exact scenario from issue
#5173— the current tests cover "all non-selectable → false" and "all selectable & selected → true", but not the mixed case where the selectable subset is fully selected while non-selectable rows exist. Consider adding:it('should return true when all selectable sub-rows are selected and some are non-selectable', () => { const data = makeData(1, 3) const columns = generateColumns(data) const table = createTable<Person>({ enableRowSelection: (row) => row.id !== '0.1', // middle sub-row non-selectable onStateChange() {}, renderFallbackValue: '', data, getSubRows: (row) => row.subRows, state: { rowSelection: { '0.0': true, '0.2': true } }, columns, getCoreRowModel: getCoreRowModel(), }) const firstRow = table.getCoreRowModel().rows[0] expect(firstRow.getIsAllSubRowsSelected()).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/tests/RowSelection.test.ts` around lines 464 - 597, Add a test covering the mixed selectable/non-selectable sub-rows case: create data with three sub-rows via makeData(1, 3), build a table using createTable with getSubRows and enableRowSelection configured so one sub-row (e.g., id '0.1') is non-selectable, set state.rowSelection to mark only the selectable sub-rows (e.g., '0.0' and '0.2') as true, then call table.getCoreRowModel().rows[0].getIsAllSubRowsSelected() and assert it returns true; this uses the same helpers from the file (makeData, generateColumns, createTable, getSubRows, enableRowSelection and getIsAllSubRowsSelected) and mirrors the provided example in the review comment.
🧹 Nitpick comments (2)
packages/table-core/src/features/RowSelection.ts (2)
647-659: Minor: trailing semicolons after function declarations are inconsistent with the rest of the file.Lines 649 and 659 have trailing
;after the closing}ofexport functiondeclarations, which doesn't match the style used elsewhere in this file.🧹 Suggested fix
export function getSelectableSubRowCount<TData extends RowData>(row: Row<TData>): number { return countSubRows(row, (node) => node.getCanSelect()); -}; +} /** * ... */ export function getSelectedSubRowCount<TData extends RowData>(row: Row<TData>): number { return countSubRows(row, (node) => node.getCanSelect() && node.getIsSelected()); -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/src/features/RowSelection.ts` around lines 647 - 659, Remove the stray trailing semicolons after the two exported function declarations to match file style: delete the semicolons following the closing braces of getSelectableSubRowCount and getSelectedSubRowCount so the export function declarations end with just "}" rather than "};".
505-514: Double traversal: each call togetIsSomeSelected/getIsAllSubRowsSelectedperforms two full BFS traversals.Both methods call
getSelectableSubRowCountandgetSelectedSubRowCountindependently, each traversing the entire sub-tree. A single pass returning both counts would cut the work in half.Additionally,
Array.shift()is O(n) due to reindexing — using an index pointer avoids this.♻️ Suggested combined traversal
+function countSubRowSelections<TData extends RowData>( + row: Row<TData>, +): { selectable: number; selected: number } { + const q = [...row.subRows] + let selectable = 0 + let selected = 0 + let i = 0 + while (i < q.length) { + const node = q[i++]! + if (node.getCanSelect()) { + selectable++ + if (node.getIsSelected()) { + selected++ + } + } + for (const child of node.subRows) { + q.push(child) + } + } + return { selectable, selected } +}Then in the call sites:
row.getIsSomeSelected = () => { - const selectable = getSelectableSubRowCount(row) - const selected = getSelectedSubRowCount(row) + const { selectable, selected } = countSubRowSelections(row) return selected > 0 && selected < selectable; } row.getIsAllSubRowsSelected = () => { - const selectable = getSelectableSubRowCount(row) - const selected = getSelectedSubRowCount(row) + const { selectable, selected } = countSubRowSelections(row) return selected > 0 && selected === selectable; }Also applies to: 668-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/src/features/RowSelection.ts` around lines 505 - 514, The two methods row.getIsSomeSelected and row.getIsAllSubRowsSelected currently call getSelectableSubRowCount and getSelectedSubRowCount separately causing two full BFS traversals; replace them with a single combined traversal helper that returns both selectable and selected counts in one pass and have both methods use that result, and in the traversal avoid Array.shift() by using a numeric index pointer over the queue/array to iterate (this pattern should also be applied to the similar logic later in the file that mirrors lines 668-679).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/table-core/tests/RowSelection.test.ts`:
- Around line 150-153: The inline comment claiming getSelectableSubRowCount and
getSelectedSubRowCount are not part of the public interface is wrong; update or
remove that comment in the test file so it accurately reflects that both
functions are exported from RowSelection.ts and are part of the public API
(referencing getSelectableSubRowCount and getSelectedSubRowCount); if you need
to keep explanatory text, reword it to acknowledge these helpers are exported
and clarify why the two RowSelectionRow.* test suites are more useful rather
than asserting they are not exposed.
- Around line 298-299: The inline comment on the createTable<Person> call is
misleading: the predicate enableRowSelection: (row) => row.id !== '0.1' actually
makes the row with id '0.1' non-selectable, not selectable; update the inline
comment near the enableRowSelection usage (next to row.id !== '0.1') to state
that '0.1' is the non-selectable sub-row (or invert the predicate if you
intended the opposite).
---
Outside diff comments:
In `@packages/table-core/tests/RowSelection.test.ts`:
- Around line 464-597: Add a test covering the mixed selectable/non-selectable
sub-rows case: create data with three sub-rows via makeData(1, 3), build a table
using createTable with getSubRows and enableRowSelection configured so one
sub-row (e.g., id '0.1') is non-selectable, set state.rowSelection to mark only
the selectable sub-rows (e.g., '0.0' and '0.2') as true, then call
table.getCoreRowModel().rows[0].getIsAllSubRowsSelected() and assert it returns
true; this uses the same helpers from the file (makeData, generateColumns,
createTable, getSubRows, enableRowSelection and getIsAllSubRowsSelected) and
mirrors the provided example in the review comment.
---
Nitpick comments:
In `@packages/table-core/src/features/RowSelection.ts`:
- Around line 647-659: Remove the stray trailing semicolons after the two
exported function declarations to match file style: delete the semicolons
following the closing braces of getSelectableSubRowCount and
getSelectedSubRowCount so the export function declarations end with just "}"
rather than "};".
- Around line 505-514: The two methods row.getIsSomeSelected and
row.getIsAllSubRowsSelected currently call getSelectableSubRowCount and
getSelectedSubRowCount separately causing two full BFS traversals; replace them
with a single combined traversal helper that returns both selectable and
selected counts in one pass and have both methods use that result, and in the
traversal avoid Array.shift() by using a numeric index pointer over the
queue/array to iterate (this pattern should also be applied to the similar logic
later in the file that mirrors lines 668-679).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/table-core/tests/RowSelection.test.ts`:
- Around line 724-769: Add a positive regression test that verifies
getIsAllSubRowsSelected returns true when some sub-rows are non-selectable but
all selectable sub-rows are selected: create hierarchical data with
makeData(1,2,2) (or similar), configure createTable with
enableRowSelection:true, getSubRows:(r)=>r.subRows, and a state.rowSelection
that marks only the selectable sub-rows (e.g., '0.0', '0.0.0', '0.0.1', '0.1',
'0.1.0', '0.1.1') as true; then call table.getCoreRowModel().rows[0] and assert
firstRow.getIsAllSubRowsSelected() === true to exercise getSubRowSelectionsCount
path and cover the regression.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/table-core/tests/RowSelection.test.ts (1)
326-343: Optional: Extract a table-creation helper to reduce boilerplate.Many tests repeat the same
createTablecall differing only inenableRowSelection,data/makeDataargs,getSubRows, andstate.rowSelection. A small factory helper (e.g.,createTestTable(opts)with sensible defaults) would cut ~10 lines per test and make the distinguishing parameters more prominent.This is purely a readability nit — the existing style in the file is consistent, so feel free to defer.
Also applies to: 365-383, 407-424, 446-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/tests/RowSelection.test.ts` around lines 326 - 343, Extract a small helper (e.g., createTestTable) to reduce repeated createTable boilerplate across tests that call createTable with similar defaults (createTable, enableRowSelection, onStateChange, renderFallbackValue, getCoreRowModel) and varying inputs (data from makeData, columns from generateColumns, getSubRows, and state.rowSelection); update the tests that currently call createTable and then use table.getCoreRowModel() / RowSelection.getSubRowSelectionsCount(firstRow) to call the helper instead, passing only the differing options so each test only specifies its unique args (data/getSubRows/state) while the helper supplies the common defaults and returns the created table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/table-core/tests/RowSelection.test.ts`:
- Around line 326-343: Extract a small helper (e.g., createTestTable) to reduce
repeated createTable boilerplate across tests that call createTable with similar
defaults (createTable, enableRowSelection, onStateChange, renderFallbackValue,
getCoreRowModel) and varying inputs (data from makeData, columns from
generateColumns, getSubRows, and state.rowSelection); update the tests that
currently call createTable and then use table.getCoreRowModel() /
RowSelection.getSubRowSelectionsCount(firstRow) to call the helper instead,
passing only the differing options so each test only specifies its unique args
(data/getSubRows/state) while the helper supplies the common defaults and
returns the created table.
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
This PR fixes the root cause for #5173.
The bug was in table-core's
getIsAllSubRowsSelectedwhich returned true when all sub-rows are unselectable.This PR fixes the bug by re-implementing the method using counts:
selectedCount == selectableCount && selectedCount > 0The count is computed with a breadth-first-search traversal rather than the existing recursive approach.
✅ Checklist
pnpm test:pr.NX Successfully ran targets test:sherif, test:knip, test:docs, test:lib, test:types, build for 99 projects and 1 task they depend on (2m)🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
New Features
Tests