-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(table-core): ensure getFilteredRowModel().flatRows uses pre-order traversal #6568
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@tanstack/table-core': patch | ||
| --- | ||
|
|
||
| Fix hierarchical filtered row models so `flatRows` lists parents before descendants in both filtering modes, preserves filter metadata on cloned rows, and round-trips nested data correctly through worker-backed row models. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import type { RowModel } from '../core/row-models/coreRowModelsFeature.types' | ||
| import type { | ||
| TableWorkerFilterData, | ||
| TableWorkerRowNode, | ||
| TableWorkerStage, | ||
| TableWorkerStagePayload, | ||
| } from './tableWorkerProtocol' | ||
|
|
||
|
|
@@ -14,14 +16,36 @@ function isCloneSafe(value: unknown): boolean { | |
| function serializeRows( | ||
| rows: Array<any>, | ||
| coreIndexById: Record<string, number>, | ||
| coreFlatRows: Array<any>, | ||
| aggregateColumnIds: Array<string>, | ||
| stage: TableWorkerStage, | ||
| ): Array<TableWorkerRowNode> { | ||
| const nodes = new Array<TableWorkerRowNode>(rows.length) | ||
| for (let i = 0; i < rows.length; i++) { | ||
| const row = rows[i] | ||
| if (row.groupingColumnId == null) { | ||
| // Data row: its position in `options.data` is all the main thread needs. | ||
| nodes[i] = coreIndexById[row.id]! | ||
| const index = coreIndexById[row.id]! | ||
| const coreRow = coreFlatRows[index]! | ||
| // A true leaf needs only its core-row position. Branch rows must carry | ||
| // their row-model children because filtering and sorting can replace or | ||
| // reorder (or remove all of) the core subtree. Filtered rows also carry | ||
| // the flags and metadata computed by the worker. | ||
| nodes[i] = | ||
| stage === 'filtered' || row.subRows.length || coreRow.subRows.length | ||
| ? { | ||
| index, | ||
| children: serializeRows( | ||
| row.subRows, | ||
| coreIndexById, | ||
| coreFlatRows, | ||
| aggregateColumnIds, | ||
| stage, | ||
| ), | ||
| ...(stage === 'filtered' | ||
| ? { filterData: serializeFilterData(row) } | ||
| : {}), | ||
|
Comment on lines
+44
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ποΈ Data Integrity & Integration | π Major | β‘ Quick win Preserve filter data in every downstream worker stage. Line 44 restricts
Serialize filter data for data rows in all stages after filtering. Add round-trip coverage for a filtered-and-grouped or filtered-and-sorted model that asserts Also applies to: 108-115 π€ Prompt for AI Agents |
||
| } | ||
| : index | ||
| continue | ||
| } | ||
| // Synthetic group row: compute aggregates eagerly (the expensive per-group | ||
|
|
@@ -50,7 +74,13 @@ function serializeRows( | |
| groupingValue: row.groupingValue, | ||
| index: row.index, | ||
| aggregates, | ||
| children: serializeRows(row.subRows, coreIndexById, aggregateColumnIds), | ||
| children: serializeRows( | ||
| row.subRows, | ||
| coreIndexById, | ||
| coreFlatRows, | ||
| aggregateColumnIds, | ||
| stage, | ||
| ), | ||
| } | ||
| } | ||
| return nodes | ||
|
|
@@ -59,22 +89,51 @@ function serializeRows( | |
| export function serializeRowModel( | ||
| model: RowModel<any, any>, | ||
| coreIndexById: Record<string, number>, | ||
| coreFlatRows: Array<any>, | ||
| aggregateColumnIds: Array<string>, | ||
| transfer: Array<Transferable>, | ||
| stage: TableWorkerStage, | ||
| ): TableWorkerStagePayload { | ||
| // Flat fast path: no synthetic rows anywhere (flatRows === rows for flat | ||
| // data). A Uint32Array permutation transfers at zero-copy cost. | ||
| if (model.flatRows.length === model.rows.length) { | ||
| const canUseFlatPayload = | ||
| model.flatRows.length === model.rows.length && | ||
| model.rows.every((row) => { | ||
| const coreRow = coreFlatRows[coreIndexById[row.id]!]! | ||
| return row.groupingColumnId == null && !coreRow.subRows.length | ||
| }) | ||
|
|
||
| if (canUseFlatPayload) { | ||
| const indices = new Uint32Array(model.rows.length) | ||
| const filterData = | ||
| stage === 'filtered' | ||
| ? new Array<TableWorkerFilterData>(model.rows.length) | ||
| : undefined | ||
| for (let i = 0; i < indices.length; i++) { | ||
| indices[i] = coreIndexById[model.rows[i]!.id]! | ||
| if (filterData) { | ||
| filterData[i] = serializeFilterData(model.rows[i]!) | ||
| } | ||
| } | ||
| transfer.push(indices.buffer) | ||
| return { kind: 'flat', indices } | ||
| return { kind: 'flat', indices, filterData } | ||
| } | ||
|
|
||
| return { | ||
| kind: 'tree', | ||
| children: serializeRows(model.rows, coreIndexById, aggregateColumnIds), | ||
| children: serializeRows( | ||
| model.rows, | ||
| coreIndexById, | ||
| coreFlatRows, | ||
| aggregateColumnIds, | ||
| stage, | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| function serializeFilterData(row: any): TableWorkerFilterData { | ||
| return { | ||
| columnFilters: row.columnFilters, | ||
| columnFiltersMeta: row.columnFiltersMeta, | ||
| } | ||
| } | ||
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.
π― Functional Correctness | π Major | β‘ Quick win
Preserve descendants at the leaf-filter depth limit.
When
depth === maxDepth, Line 68 skips recursion and the retainednewRowhas emptysubRows. A matching row at the cutoff then loses its original descendants from bothrowsandflatRows. The root-first path preserves that subtree at Lines 151-156. Assignrow.subRowsto the retained clone at the cutoff, and add a leaf-first regression test withmaxLeafRowFilterDepth: 0.constructRowinitializes omittedsubRowsas an empty array, while the depth option must leave deeper rows unfiltered. (tanstack.com)Proposed fix
} else { if (filterRow(newRow)) { + if (row.subRows.length) { + newRow.subRows = row.subRows + } filteredRows.push(newRow) } }π Committable suggestion
π€ Prompt for AI Agents