fix(table-core): avoid parent lookup re-entrancy#6441
Conversation
📝 WalkthroughWalkthrough
ChangesStructural Parent Resolution
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
|
View your CI Pipeline Execution ↗ for commit 67547ea
☁️ Nx Cloud last updated this comment at |
![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
🤖 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 `@packages/table-core/src/core/rows/coreRowsFeature.utils.ts`:
- Around line 203-205: Update the parent check in the row-parent resolution
logic to test specifically for an undefined parentId, preserving empty-string
IDs as valid values. Keep the existing undefined return behavior when no parent
ID is provided.
In
`@packages/table-core/tests/implementation/features/column-filtering/createFilteredRowModel.test.ts`:
- Around line 107-129: Add a grandchild to the nested-row fixture in the
relevant filtering test, then add a filter case whose value matches only that
deeper descendant. Assert the filtered flat rows retain the matching descendant
and all of its ancestor rows, while preserving the existing parent-match
assertions to cover recursive parent lookup.
🪄 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: 5a9ee65a-52d8-4be6-aa67-1acf9cd782be
📒 Files selected for processing (2)
packages/table-core/src/core/rows/coreRowsFeature.utils.tspackages/table-core/tests/implementation/features/column-filtering/createFilteredRowModel.test.ts
| if (!row.parentId) { | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve empty-string row IDs.
Line 203 treats '' as no parent, so custom getRowId implementations using an empty-string ID cannot resolve their children’s parent. Check specifically for undefined.
Proposed fix
- if (!row.parentId) {
+ if (row.parentId === undefined) {
return undefined
}📝 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.
| if (!row.parentId) { | |
| return undefined | |
| } | |
| if (row.parentId === undefined) { | |
| return undefined | |
| } |
🤖 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/core/rows/coreRowsFeature.utils.ts` around lines 203
- 205, Update the parent check in the row-parent resolution logic to test
specifically for an undefined parentId, preserving empty-string IDs as valid
values. Keep the existing undefined return behavior when no parent ID is
provided.
| data: [ | ||
| { name: 'parent', subRows: [{ name: 'child' }] }, | ||
| { name: 'other', subRows: [{ name: 'nested' }] }, | ||
| ], | ||
| getSubRows: (row) => row.subRows, | ||
| initialState: { | ||
| columnFilters: [{ id: 'name', value: 'parent' }], | ||
| }, | ||
| }) | ||
|
|
||
| expect(() => table.getFilteredRowModel()).not.toThrow() | ||
| expect(rowNames(table.getFilteredRowModel().flatRows).sort()).toEqual([ | ||
| 'child', | ||
| 'parent', | ||
| ]) | ||
|
|
||
| table.setColumnFilters([{ id: 'name', value: 'other' }]) | ||
|
|
||
| expect(() => table.getFilteredRowModel()).not.toThrow() | ||
| expect(rowNames(table.getFilteredRowModel().flatRows).sort()).toEqual([ | ||
| 'nested', | ||
| 'other', | ||
| ]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover descendant matches across multiple nesting levels.
The fixture only has one child level, and both filter values match parents. Add a grandchild plus a descendant-matching filter case, asserting its ancestor family is retained; this validates recursive parent lookup and the linked issue’s multi-level requirement.
🤖 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/tests/implementation/features/column-filtering/createFilteredRowModel.test.ts`
around lines 107 - 129, Add a grandchild to the nested-row fixture in the
relevant filtering test, then add a filter case whose value matches only that
deeper descendant. Assert the filtered flat rows retain the matching descendant
and all of its ancestor rows, while preserving the existing parent-match
assertions to cover recursive parent lookup.
1 participant
Closes #5630
What changed
Why
During filtered-row-model construction, a custom filter function can call row.getParentRow(). The previous lookup entered the pre-pagination pipeline, which requests the filtered model currently being built and reads rowsById from an incomplete result. Looking up structural parents from the already-built core row model avoids this re-entrancy without changing support for generated parents.
Validation
Summary by CodeRabbit