fix: bugs with pre-row models#6376
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit d6526f0
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR fixes a duplication bug in ChangesRow Model Bug Fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
🧹 Nitpick comments (3)
packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts (1)
600-664: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a fallback test for
getFilteredSelectedRowModelwithoutcolumnFilteringFeature.The suite tests the grouped/sorted fallback path (Lines 645-663) but has no equivalent case verifying
getFilteredSelectedRowModel()(which now callstable.getFilteredRowModel()) behaves safely on a table lackingcolumnFilteringFeature. Given the sourcing change discussed forrowSelectionFeature.utils.ts, a symmetric test would close the verification gap.🤖 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/row-selection/rowSelectionFeature.test.ts` around lines 600 - 664, Add a fallback test for getFilteredSelectedRowModel in rowSelectionFeature.test.ts that mirrors the grouped/sorted fallback coverage: build a table from makePipelineTable without columnFilteringFeature, call table.getFilteredSelectedRowModel(), and assert it still returns the selected rows safely instead of failing. Use getFilteredSelectedRowModel, makePipelineTable, and the pipelineFeatures override pattern to locate and verify the fallback behavior introduced in rowSelectionFeature.utils.ts.packages/table-core/src/features/column-grouping/createGroupedRowModel.ts (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor style inconsistency: manual for-loop vs. forEach.
Rest of the function uses
.forEach()for identical push patterns (lines 117, 185-188, 199-202). Using a plainforloop here is inconsistent but not incorrect.♻️ Optional consistency tweak
if (row.subRows.length) { row.subRows = groupUpRecursively(row.subRows, depth + 1, row.id) - for (let i = 0; i < row.subRows.length; i++) { - const subRow = row.subRows[i]! + row.subRows.forEach((subRow) => { groupedFlatRows.push(subRow) groupedRowsById[subRow.id] = subRow - } + }) }🤖 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/column-grouping/createGroupedRowModel.ts` around lines 90 - 96, The grouped row traversal in createGroupedRowModel is using a manual for-loop for the same push pattern that the rest of the function handles with forEach, so make this block consistent with the surrounding style. Update the row.subRows handling inside groupUpRecursively to use the same array iteration approach as the other groupedRowsById/groupedFlatRows population sites in createGroupedRowModel, keeping the behavior unchanged while matching the existing forEach style.packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts (1)
250-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the fallback assumption here too.
Line 286 has a comment explaining the sorted-model fallback chain, but Line 250 has none. If
table.getFilteredRowModel()also relies on a core fallback whencolumnFilteringFeatureisn't registered, a short comment here would aid future readers and match the pattern at Line 284-285.🤖 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` at line 250, The row-selection fallback logic in rowSelectionFeature.utils.ts should also document the assumption behind table.getFilteredRowModel(), since it may fall back to the core row model when columnFilteringFeature is not registered. Add a short explanatory comment near the rowModel assignment in the rowSelection-related helper so the fallback chain is clear and consistent with the existing sorted-model fallback comment, and reference the same selection flow used by getFilteredRowModel and the nearby sorted-model logic.
🤖 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.
Nitpick comments:
In `@packages/table-core/src/features/column-grouping/createGroupedRowModel.ts`:
- Around line 90-96: The grouped row traversal in createGroupedRowModel is using
a manual for-loop for the same push pattern that the rest of the function
handles with forEach, so make this block consistent with the surrounding style.
Update the row.subRows handling inside groupUpRecursively to use the same array
iteration approach as the other groupedRowsById/groupedFlatRows population sites
in createGroupedRowModel, keeping the behavior unchanged while matching the
existing forEach style.
In `@packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts`:
- Line 250: The row-selection fallback logic in rowSelectionFeature.utils.ts
should also document the assumption behind table.getFilteredRowModel(), since it
may fall back to the core row model when columnFilteringFeature is not
registered. Add a short explanatory comment near the rowModel assignment in the
rowSelection-related helper so the fallback chain is clear and consistent with
the existing sorted-model fallback comment, and reference the same selection
flow used by getFilteredRowModel and the nearby sorted-model logic.
In
`@packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts`:
- Around line 600-664: Add a fallback test for getFilteredSelectedRowModel in
rowSelectionFeature.test.ts that mirrors the grouped/sorted fallback coverage:
build a table from makePipelineTable without columnFilteringFeature, call
table.getFilteredSelectedRowModel(), and assert it still returns the selected
rows safely instead of failing. Use getFilteredSelectedRowModel,
makePipelineTable, and the pipelineFeatures override pattern to locate and
verify the fallback behavior introduced in rowSelectionFeature.utils.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e644cc99-532e-4779-974c-42505a58918e
📒 Files selected for processing (6)
packages/table-core/src/features/column-grouping/createGroupedRowModel.tspackages/table-core/src/features/row-selection/rowSelectionFeature.utils.tspackages/table-core/tests/implementation/features/column-grouping/createGroupedRowModel.test.tspackages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.tsperf-done.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
Tests
Documentation