diff --git a/.changeset/great-pugs-sniff.md b/.changeset/great-pugs-sniff.md new file mode 100644 index 0000000000..d40c8b320f --- /dev/null +++ b/.changeset/great-pugs-sniff.md @@ -0,0 +1,5 @@ +--- +'@tanstack/table-core': patch +--- + +Fix `getSortedRowModel().flatRows` listing sub-rows before their parent. This restores the parent-first order used by the v8 sorted model and aligns the flattened result with the sorted `rows` tree, core row model, and paginated row model. diff --git a/packages/table-core/src/features/row-sorting/createSortedRowModel.ts b/packages/table-core/src/features/row-sorting/createSortedRowModel.ts index 09653b8e98..dee54c1932 100644 --- a/packages/table-core/src/features/row-sorting/createSortedRowModel.ts +++ b/packages/table-core/src/features/row-sorting/createSortedRowModel.ts @@ -163,6 +163,11 @@ function _createSortedRowModel< changed = true } + // Take this row's slot before descending, so a parent stays ahead of + // its own sub-rows in flatRows. + const flatIndex = sortedFlatRows.length + sortedFlatRows.push(row) + if (row.subRows.length) { const sortedSubRows = sortData(row.subRows) @@ -172,13 +177,9 @@ function _createSortedRowModel< copyInstancePropertiesWithoutMemos(cloned, row) cloned.subRows = sortedSubRows.rows sortedData[i] = cloned - sortedFlatRows.push(cloned) + sortedFlatRows[flatIndex] = cloned changed = true - } else { - sortedFlatRows.push(row) } - } else { - sortedFlatRows.push(row) } } diff --git a/packages/table-core/src/worker/createWorkerRowModel.ts b/packages/table-core/src/worker/createWorkerRowModel.ts index 7d2e7395de..3f34819b5b 100644 --- a/packages/table-core/src/worker/createWorkerRowModel.ts +++ b/packages/table-core/src/worker/createWorkerRowModel.ts @@ -109,7 +109,7 @@ export function createWorkerRowModel( } } } - return rebuildRowModel(table, payload, stage !== 'filtered') + return rebuildRowModel(table, payload, stage) }, }) diff --git a/packages/table-core/src/worker/rebuildRowModel.ts b/packages/table-core/src/worker/rebuildRowModel.ts index 62dbd34ba2..630d476669 100644 --- a/packages/table-core/src/worker/rebuildRowModel.ts +++ b/packages/table-core/src/worker/rebuildRowModel.ts @@ -6,6 +6,7 @@ import type { TableFeatures } from '../types/TableFeatures' import type { RowData } from '../types/type-utils' import type { TableWorkerRowNode, + TableWorkerStage, TableWorkerStagePayload, } from './tableWorkerProtocol' @@ -38,15 +39,14 @@ export function rebuildRowModel< >( table: Table_Internal, payload: TableWorkerDataPayload, - /** - * Whether a flat payload should rewrite row depth/parentId. Mirrors core: - * the grouped model's passthrough resets them, the filtered model never - * touches them. Without this distinction a filtered rebuild could zero the - * depths a grouped/sorted tree rebuild just assigned to shared row objects. - */ - resetDepths: boolean, + stage: TableWorkerStage, ): RowModel { const core = table.getCoreRowModel() + // The grouped model's flat passthrough resets row relationships, while the + // filtered model never touches them. Without this distinction a filtered + // rebuild could zero depths assigned by a grouped/sorted tree rebuild. + const resetDepths = stage !== 'filtered' + const flattenParentsFirst = stage === 'sorted' if (payload.kind === 'flat') { const { indices } = payload @@ -84,6 +84,14 @@ export function rebuildRowModel< continue } + // Sorted flatRows preserve the recursive rows order. Reserve the + // synthetic parent's position before rebuilding its descendants, then + // fill it once the row can be constructed from those descendants. + const flatIndex = flattenParentsFirst ? flatRows.length : -1 + if (flattenParentsFirst) { + flatRows.push(undefined) + } + const subRows = rebuildRows(node.children, depth + 1, node.id) const leafRows: Array = [] collectLeafRows(subRows, leafRows) @@ -107,7 +115,11 @@ export function rebuildRowModel< hasOwn(aggregates, columnId) ? aggregates[columnId] : undefined, }) - flatRows.push(row) + if (flattenParentsFirst) { + flatRows[flatIndex] = row + } else { + flatRows.push(row) + } rowsById[node.id] = row rows[i] = row } diff --git a/packages/table-core/tests/implementation/features/row-sorting/createSortedRowModel.test.ts b/packages/table-core/tests/implementation/features/row-sorting/createSortedRowModel.test.ts index fb1e6a2d0e..b2f6cbdc11 100644 --- a/packages/table-core/tests/implementation/features/row-sorting/createSortedRowModel.test.ts +++ b/packages/table-core/tests/implementation/features/row-sorting/createSortedRowModel.test.ts @@ -5,7 +5,7 @@ import { rowSortingFeature, } from '../../../../src' import { testFeatures } from '../../../fixtures/features' -import type { ColumnDef } from '../../../../src' +import type { ColumnDef, Row } from '../../../../src' type Person = { firstName: string @@ -42,6 +42,22 @@ function makeTable( }) } +function flattenRows(rows: Array>) { + const flatRows: Array> = [] + + const visit = (nestedRows: Array>) => { + for (const row of nestedRows) { + flatRows.push(row) + if (row.subRows.length) { + visit(row.subRows) + } + } + } + + visit(rows) + return flatRows +} + describe('createSortedRowModel', () => { it('does not crash when the sorting state references a column that no longer exists', () => { const table = makeTable([{ id: 'thisColumnDoesNotExist', desc: false }]) @@ -210,6 +226,84 @@ describe('createSortedRowModel', () => { ).toEqual(['child-a', 'child-b']) }) + it('flattens each parent ahead of its own sub-rows', () => { + const table = makeTable( + [{ id: 'age', desc: false }], + [ + { + firstName: 'older-parent', + age: 20, + subRows: [ + { + firstName: 'child-b', + age: 15, + subRows: [ + { firstName: 'grandchild-b', age: 4 }, + { firstName: 'grandchild-a', age: 2 }, + ], + }, + { firstName: 'child-a', age: 10 }, + ], + }, + { + firstName: 'younger-parent', + age: 5, + subRows: [{ firstName: 'child-c', age: 1 }], + }, + ], + ) + + expect( + table.getSortedRowModel().flatRows.map((row) => row.original.firstName), + ).toEqual([ + 'younger-parent', + 'child-c', + 'older-parent', + 'child-a', + 'child-b', + 'grandchild-a', + 'grandchild-b', + ]) + }) + + it('flattens nested branch clones rather than the rows they replaced', () => { + const table = makeTable( + [{ id: 'age', desc: false }], + [ + { + firstName: 'parent', + age: 20, + subRows: [ + { + firstName: 'child-b', + age: 15, + subRows: [ + { firstName: 'grandchild-b', age: 4 }, + { firstName: 'grandchild-a', age: 2 }, + ], + }, + { firstName: 'child-a', age: 10 }, + ], + }, + ], + ) + + const preSortedRowModel = table.getPreSortedRowModel() + const sortedRowModel = table.getSortedRowModel() + const recursivelyFlattenedRows = flattenRows(sortedRowModel.rows) + + expect(sortedRowModel.flatRows).toHaveLength( + recursivelyFlattenedRows.length, + ) + for (let i = 0; i < recursivelyFlattenedRows.length; i++) { + expect(sortedRowModel.flatRows[i]).toBe(recursivelyFlattenedRows[i]) + } + expect(sortedRowModel.rows[0]).not.toBe(preSortedRowModel.rows[0]) + expect(sortedRowModel.rows[0]!.subRows[1]).not.toBe( + preSortedRowModel.rows[0]!.subRows[0], + ) + }) + describe('sortUndefined', () => { type MaybePerson = { firstName: string diff --git a/packages/table-core/tests/unit/worker/serializeRebuild.test.ts b/packages/table-core/tests/unit/worker/serializeRebuild.test.ts index 51a7975b5f..47aae6ff57 100644 --- a/packages/table-core/tests/unit/worker/serializeRebuild.test.ts +++ b/packages/table-core/tests/unit/worker/serializeRebuild.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it } from 'vitest' import { aggregationFns, - rowAggregationFeature, columnFilteringFeature, columnGroupingFeature, constructTable, @@ -10,6 +9,7 @@ import { createSortedRowModel, filterFns, globalFilteringFeature, + rowAggregationFeature, rowSortingFeature, sortFns, } from '../../../src' @@ -18,6 +18,7 @@ import { serializeRowModel } from '../../../src/worker/serializeRowModel' import { rebuildRowModel } from '../../../src/worker/rebuildRowModel' import { makeObjectMap } from '../../../src/utils' import type { ColumnDef, RowModel, Table } from '../../../src' +import type { TableWorkerStage } from '../../../src/worker/tableWorkerProtocol' type Person = { firstName: string @@ -83,7 +84,7 @@ function roundTrip( workerTable: Table, mainTable: Table, model: RowModel, - resetDepths = true, + stage: TableWorkerStage, ) { const transfer: Array = [] const payload = serializeRowModel( @@ -97,7 +98,7 @@ function roundTrip( } return { payload, - rebuilt: rebuildRowModel(mainTable, payload, resetDepths), + rebuilt: rebuildRowModel(mainTable, payload, stage), } } @@ -111,10 +112,16 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { workerTable.baseAtoms.sorting.set([{ id: 'age', desc: true }]) const model = workerTable.getSortedRowModel() - const { payload, rebuilt } = roundTrip(workerTable, mainTable, model) + const { payload, rebuilt } = roundTrip( + workerTable, + mainTable, + model, + 'sorted', + ) expect(payload.kind).toBe('flat') expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) + expect(ids(rebuilt.flatRows)).toEqual(ids(model.flatRows)) // rows are the main table's own core rows, not clones expect(rebuilt.rows[0]!).toBe( mainTable.getCoreRowModel().flatRows[Number(model.rows[0]!.id)], @@ -129,12 +136,18 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { workerTable.baseAtoms.columnFilters.set([{ id: 'status', value: 'single' }]) const model = workerTable.getFilteredRowModel() - const { payload, rebuilt } = roundTrip(workerTable, mainTable, model) + const { payload, rebuilt } = roundTrip( + workerTable, + mainTable, + model, + 'filtered', + ) expect(payload.kind).toBe('flat') expect(model.rows.length).toBeGreaterThan(0) expect(model.rows.length).toBeLessThan(data.length) expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) + expect(ids(rebuilt.flatRows)).toEqual(ids(model.flatRows)) }) it('round-trips a grouped model as a tree with aggregates', () => { @@ -144,7 +157,12 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { workerTable.baseAtoms.grouping.set(['status']) const model = workerTable.getGroupedRowModel() - const { payload, rebuilt } = roundTrip(workerTable, mainTable, model) + const { payload, rebuilt } = roundTrip( + workerTable, + mainTable, + model, + 'grouped', + ) expect(payload.kind).toBe('tree') expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) @@ -190,10 +208,16 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { workerTable.baseAtoms.sorting.set([{ id: 'visits', desc: false }]) const model = workerTable.getSortedRowModel() - const { payload, rebuilt } = roundTrip(workerTable, mainTable, model) + const { payload, rebuilt } = roundTrip( + workerTable, + mainTable, + model, + 'sorted', + ) expect(payload.kind).toBe('tree') expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) + expect(ids(rebuilt.flatRows)).toEqual(ids(model.flatRows)) // leaves within each group are in the worker's sorted order for (let i = 0; i < model.rows.length; i++) { expect(ids(rebuilt.rows[i]!.subRows)).toEqual(ids(model.rows[i]!.subRows)) @@ -207,7 +231,7 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { workerTable.baseAtoms.grouping.set(['status', 'age']) const model = workerTable.getGroupedRowModel() - const { rebuilt } = roundTrip(workerTable, mainTable, model) + const { rebuilt } = roundTrip(workerTable, mainTable, model, 'grouped') expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) const firstGroup = rebuilt.rows[0]! @@ -219,6 +243,20 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { expect(firstLeaf.parentId).toBe(firstSubGroup.id) }) + it('round-trips parent-first sorted flatRows through nested groups', () => { + const data = makeData(18) + const workerTable = makeTable(data) + const mainTable = makeTable(data) + workerTable.baseAtoms.grouping.set(['status', 'age']) + workerTable.baseAtoms.sorting.set([{ id: 'visits', desc: false }]) + + const model = workerTable.getSortedRowModel() + const { rebuilt } = roundTrip(workerTable, mainTable, model, 'sorted') + + expect(ids(rebuilt.rows)).toEqual(ids(model.rows)) + expect(ids(rebuilt.flatRows)).toEqual(ids(model.flatRows)) + }) + it('does not reset depths when rebuilding a filtered payload (regression)', () => { const data = makeData(12) const workerTable = makeTable(data) @@ -227,17 +265,22 @@ describe('serializeRowModel -> rebuildRowModel round trip', () => { // Tree rebuild assigns leaf depths on the main table's shared row objects const grouped = workerTable.getGroupedRowModel() - const { rebuilt: rebuiltTree } = roundTrip(workerTable, mainTable, grouped) + const { rebuilt: rebuiltTree } = roundTrip( + workerTable, + mainTable, + grouped, + 'grouped', + ) const someLeaf = rebuiltTree.rows[0]!.subRows[0]! expect(someLeaf.depth).toBe(1) // A filtered (flat) rebuild afterward must not zero those depths const filtered = workerTable.getFilteredRowModel() - roundTrip(workerTable, mainTable, filtered, false) + roundTrip(workerTable, mainTable, filtered, 'filtered') expect(someLeaf.depth).toBe(1) // ...while a grouped-or-later flat passthrough does reset them - roundTrip(workerTable, mainTable, filtered, true) + roundTrip(workerTable, mainTable, filtered, 'grouped') expect(someLeaf.depth).toBe(0) }) }) diff --git a/perf-done.md b/perf-done.md index 163b371cb0..131d8aa0a1 100644 --- a/perf-done.md +++ b/perf-done.md @@ -1297,6 +1297,8 @@ table_getIsSomeRowsSelected: { **Status:** `[x]` done **Implementation note:** Investigated why the clone existed: the post-sort loop assigns `row.subRows = sortData(row.subRows)`, which would corrupt the source row model if `row` were the original. So the clone is genuinely necessary for **rows with subRows**, but pointless for leaf rows. Refactored: `rows.slice()` produces a sortable array copy (one allocation), the sort runs as before, and the post-sort loop clones only rows where `row.subRows.length > 0`. Leaf rows pass through as their original references. For a flat table (the common case) this drops from N heavy clones to **zero per-row clones** plus one `slice()`. For nested tables, only parent rows are cloned (typically a small fraction of total rows). The native `Array.prototype.sort` is stable since ES2019; the explicit `row.index` tiebreaker was preserved in the comparator for any caller that relied on it. +**2026-08-08 follow-up (#6529):** The branch-only clone rewrite moved the branch's `flatRows` insertion below recursive sorting, unintentionally changing sorted hierarchical models from parent-first to post-order. Restored the v8 parent-first contract by reserving the parent's flat-array slot before recursion and replacing that slot when a clone is required; worker-backed sorted tree reconstruction now mirrors it. The original optimization remains intact: leaf rows are still reused rather than cloned, and the flattening pass remains O(R). + **Location:** `src/features/row-sorting/createSortedRowModel.ts:81–89` **Category:** `big-o`, `micro` @@ -1312,39 +1314,49 @@ This allocates N row clones every time the sorted row model rebuilds. `Array.pro **After** ```ts +const sortedData = rows.slice() +sortedData.sort(compareRows) +let changed = false + // If there are sub-rows, sort them. Clone only rows that need mutation // (i.e. have subRows) so we don't corrupt the source row model. for (let i = 0; i < sortedData.length; i++) { const row = sortedData[i]! + if (row !== rows[i]) changed = true + + const flatIndex = sortedFlatRows.length + sortedFlatRows.push(row) + if (row.subRows.length) { - // Preserve prototype chain so methods like getValue() remain accessible - const cloned = Object.create(Object.getPrototypeOf(row)) - Object.assign(cloned, row) - cloned.subRows = sortData(row.subRows) - sortedData[i] = cloned - sortedFlatRows.push(cloned) - } else { - sortedFlatRows.push(row) + const sortedSubRows = sortData(row.subRows) + if (sortedSubRows.changed) { + const cloned = Object.create(Object.getPrototypeOf(row)) + copyInstancePropertiesWithoutMemos(cloned, row) + cloned.subRows = sortedSubRows.rows + sortedData[i] = cloned + sortedFlatRows[flatIndex] = cloned + changed = true + } } } -return sortedData +return { rows: sortedData, changed } ``` **Big-O:** Drops O(n) heavy object allocations per sort. -**Scale impact** (heavy row clones replaced with lightweight `{row, index}` wrappers — dimension: rows sorted per sort pass): +**Scale impact** (flat-table case — dimension: rows sorted per sort pass): -| Rows sorted | Before (full row clones via `Object.create` + `Object.assign`) | After (`{row, index}` wrappers) | Saved | -| ----------- | -------------------------------------------------------------- | ------------------------------- | ----------------------------- | -| 10 | 10 heavy clones | 10 small wrappers | ~10 wide → narrow allocations | -| 100 | 100 | 100 | ~100 | -| 1,000 | 1,000 | 1,000 | ~1,000 | -| 10,000 | 10,000 | 10,000 | ~10,000 | +| Rows sorted | Before (full row clones via `Object.create` + `Object.assign`) | After (`rows.slice()`) | Saved heavy clones | +| ----------- | -------------------------------------------------------------- | ---------------------- | ------------------ | +| 10 | 10 | 0 | 10 | +| 100 | 100 | 0 | 100 | +| 1,000 | 1,000 | 0 | 1,000 | +| 10,000 | 10,000 | 0 | 10,000 | -(Memory is the bigger win than count: each "heavy clone" copies _all_ enumerable fields on a constructed Row, vs `{row, index}` which is 2 fields.) +(Nested tables still clone only branches whose sorted descendants changed.) -**Risk:** Behavior depends on whether downstream code mutates the returned rows. The current clone is defensive against mutation. Verify nothing post-sort writes to row instances (the project uses prototype methods, so mutations should not occur). +**Risk:** Behavior depends on whether downstream code mutates the returned rows. The current clone is defensive against mutation. Verify nothing post-sort writes to row instances (the project uses prototype methods, so mutations should not occur). Recursive rewrites must also preserve parent-first `flatRows` order and replace reserved entries with any branch clones; #6529 adds direct and worker-backed nested-tree coverage for both invariants. --- @@ -2647,7 +2659,7 @@ if (Array.isArray(value)) { ## 116. B10: createSortedRowModel: no availableSorting.length guard; branch rows cloned even when subRows unchanged — Score: 4 **Status:** `[x]` done -**Implementation note:** Added the `availableSorting.length` guard after missing/unsortable sorting entries are filtered, so a sorting state containing only unavailable ids returns `preSortedRowModel` directly. Also changed recursive sorting to return a `changed` flag computed during the existing post-sort `flatRows` walk; branch rows are cloned only when their sorted `subRows` changed order or contain a cloned descendant. Added focused row-sorting tests for unknown-only sorting returning the pre-sorted model, unchanged branch-row identity preservation, and clone-on-changed-subRows behavior. +**Implementation note:** Added the `availableSorting.length` guard after missing/unsortable sorting entries are filtered, so a sorting state containing only unavailable ids returns `preSortedRowModel` directly. Also changed recursive sorting to return a `changed` flag computed during the existing post-sort `flatRows` walk; branch rows are cloned only when their sorted `subRows` changed order or contain a cloned descendant. Added focused row-sorting tests for unknown-only sorting returning the pre-sorted model, unchanged branch-row identity preservation, and clone-on-changed-subRows behavior. Follow-up #6529 found that this inherited #49's post-order flat-row regression and restored parent-first insertion without changing the clone-skipping logic. **Location:** `createSortedRowModel.ts:47–58, 130–147` **Category:** `micro`