fix: table-core's getIsAllSubRowsSelected returns false when subrows are unselectable#6177
Open
kwengontra wants to merge 2 commits intoTanStack:mainfrom
Open
fix: table-core's getIsAllSubRowsSelected returns false when subrows are unselectable#6177kwengontra wants to merge 2 commits intoTanStack:mainfrom
kwengontra wants to merge 2 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 519e40d 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 |
📝 WalkthroughWalkthroughA patch version bump for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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).
Comment on lines
+150
to
+153
|
|
||
| // IMO the following 2 tests suites are not useful as they are not exposed to the public interface | ||
| // Instead the 2 RowSelectionRow.* test suites are more useful. | ||
| describe('getSelectableSubRowCount', () => { |
There was a problem hiding this comment.
Incorrect comment: getSelectableSubRowCount and getSelectedSubRowCount are part of the public interface.
Both functions are export function in RowSelection.ts, so they are exposed to consumers. The comment on lines 151–152 claims otherwise. Consider removing or correcting it.
🤖 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 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.
Comment on lines
+298
to
+299
| const table = createTable<Person>({ | ||
| enableRowSelection: (row) => row.id !== '0.1', // second sub-row is selectable |
There was a problem hiding this comment.
Misleading comment: '0.1' is the non-selectable sub-row, not the selectable one.
The predicate row.id !== '0.1' makes '0.1' non-selectable. The inline comment says the opposite.
🧹 Suggested fix
- enableRowSelection: (row) => row.id !== '0.1', // second sub-row is selectable
+ enableRowSelection: (row) => row.id !== '0.1', // second sub-row is NOT selectable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const table = createTable<Person>({ | |
| enableRowSelection: (row) => row.id !== '0.1', // second sub-row is selectable | |
| const table = createTable<Person>({ | |
| enableRowSelection: (row) => row.id !== '0.1', // second sub-row is NOT selectable |
🤖 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 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).
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
Release Notes
Bug Fixes
New Features