-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: retain columnFiltersMeta when leaf row filtering #6075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2a98ea1
to
9333d0f
Compare
WalkthroughPropagates columnFiltersMeta to newly created leaf rows in filterRowModelFromLeafs, aligning it with columnFilters during leaf-based filtering. No public API or control-flow changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Table as Table Core
participant Filter as filterRowModelFromLeafs
participant Leaf as New Leaf Row(s)
User->>Table: Trigger filtering
Table->>Filter: Build row model from leafs
rect rgb(240,248,255)
note right of Filter: For each matched row
Filter->>Leaf: Create new leaf row
Filter->>Leaf: Copy columnFilters
Filter->>Leaf: Copy columnFiltersMeta (new)
end
Filter-->>Table: Return filtered leaf row model
Table-->>User: Render filtered rows
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/table-core/src/utils/filterRowsUtils.ts (3)
104-115
: Mirror filter-state copy in root-path clone for consistencyWhen cloning a row in filterRowModelFromRoot, also copy columnFilters and columnFiltersMeta to avoid surprises if downstream consumers read these on cloned parents.
Apply:
const newRow = createRow( table, row.id, row.original, row.index, row.depth, undefined, row.parentId ) + newRow.columnFilters = row.columnFilters + newRow.columnFiltersMeta = row.columnFiltersMeta newRow.subRows = recurseFilterRows(row.subRows, depth + 1) row = newRow
44-68
: Avoid repeated filterRow(row) calls; compute onceMinor perf/readability: filterRow(row) is evaluated multiple times per row. Cache the result.
if (row.subRows?.length && depth < maxDepth) { newRow.subRows = recurseFilterRows(row.subRows, depth + 1) row = newRow - - if (filterRow(row) && !newRow.subRows.length) { + const pass = filterRow(row) + if (pass && !newRow.subRows.length) { rows.push(row) newFilteredRowsById[row.id] = row newFilteredFlatRows.push(row) continue } - if (filterRow(row) || newRow.subRows.length) { + if (pass || newRow.subRows.length) { rows.push(row) newFilteredRowsById[row.id] = row newFilteredFlatRows.push(row) continue } } else { row = newRow - if (filterRow(row)) { + const pass = filterRow(row) + if (pass) { rows.push(row) newFilteredRowsById[row.id] = row newFilteredFlatRows.push(row) } }
41-42
: Confirm immutability; add a small regression testThese assignments copy references. If columnFilters/columnFiltersMeta can be mutated per-row downstream, consider shallow-cloning to avoid aliasing; otherwise current approach is fine. Also worth a regression test that asserts columnFiltersMeta is preserved when filterFromLeafRows=true.
I can draft a focused test that builds a nested row model, sets columnFilters/columnFiltersMeta, runs both leaf and root filtering, and asserts preservation. Want me to add it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/table-core/src/utils/filterRowsUtils.ts
(1 hunks)
🔇 Additional comments (1)
packages/table-core/src/utils/filterRowsUtils.ts (1)
41-43
: LGTM: Preserve columnFiltersMeta with leaf-row cloningCopying columnFiltersMeta alongside columnFilters fixes the metadata loss when filterFromLeafRows is enabled. Looks correct.
9333d0f
to
32acc97
Compare
Summary by CodeRabbit