fix: let parent rows unselect child rows when not itself selected#6364
Merged
Conversation
- Updated checkbox selection logic to account for sub-row selections in various examples (Alpine, Angular, React, Preact, Solid, Svelte, Vue). - Refactored core row selection feature to include memoization for `getIsAllSubRowsSelected` and `getCanSelectSubRows` functions. - Improved performance by optimizing selection checks in utility functions.
|
View your CI Pipeline Execution ↗ for commit 461622f
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR updates table-core row selection selectors to include explicit memoDeps, removes an early-return short-circuit in row_toggleSelected, and refactors isSubRowSelected to use a for loop with break. Framework example apps update checkbox checked logic to reflect sub-row selection. Two perf markdown files get formatting-only edits. ChangesRow selection core logic and examples
Perf notes formatting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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/src/features/row-selection/rowSelectionFeature.utils.ts (1)
786-819: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the zero-selectable-descendant case.
isSubRowSelected()returns'all'when every descendant is non-selectable, sochecked={row.getCanSelectSubRows() && row.getIsAllSubRowsSelected()}can mark a parent as checked even though nothing below it can be selected. Returnfalsefor that case, or track selectable descendants separately.🤖 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/row-selection/rowSelectionFeature.utils.ts` around lines 786 - 819, The `isSubRowSelected` logic in `rowSelectionFeature.utils.ts` currently treats a branch with only non-selectable descendants as `'all'`, which can make `getIsAllSubRowsSelected()` report true for a parent with nothing selectable below it. Update this helper to distinguish “all selectable descendants selected” from “no selectable descendants exist” by tracking whether any selectable descendant was found while walking `row.subRows`, and return `false` for the zero-selectable-descendant case. Keep the existing behavior for `someSelected`/`allChildrenSelected`, but ensure the final return only yields `'all'` when at least one selectable descendant exists and all of them are selected.
🤖 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.
Outside diff comments:
In `@packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts`:
- Around line 786-819: The `isSubRowSelected` logic in
`rowSelectionFeature.utils.ts` currently treats a branch with only
non-selectable descendants as `'all'`, which can make
`getIsAllSubRowsSelected()` report true for a parent with nothing selectable
below it. Update this helper to distinguish “all selectable descendants
selected” from “no selectable descendants exist” by tracking whether any
selectable descendant was found while walking `row.subRows`, and return `false`
for the zero-selectable-descendant case. Keep the existing behavior for
`someSelected`/`allChildrenSelected`, but ensure the final return only yields
`'all'` when at least one selectable descendant exists and all of them are
selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbe6b7a0-256a-42aa-a805-ffbb8d9d77b8
📒 Files selected for processing (12)
examples/alpine/expanding/index.htmlexamples/angular/expanding/src/app/expandable-cell/expandable-cell.tsexamples/lit/expanding/src/main.tsexamples/preact/expanding/src/main.tsxexamples/react/expanding/src/main.tsxexamples/solid/expanding/src/App.tsxexamples/svelte/expanding/src/App.svelteexamples/vue/expanding/src/App.tsxpackages/table-core/src/features/row-selection/rowSelectionFeature.tspackages/table-core/src/features/row-selection/rowSelectionFeature.utils.tsperf-done.mdperf-skipped.md
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.
getIsAllSubRowsSelectedandgetCanSelectSubRowsfunctions.🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
New Features
Bug Fixes