From d6526f0f3b3c79a9754835312e4137101420ef95 Mon Sep 17 00:00:00 2001 From: Kevin Van Cott Date: Fri, 3 Jul 2026 12:44:17 -0500 Subject: [PATCH] fix: bugs with pre-row models --- .../column-grouping/createGroupedRowModel.ts | 12 +- .../rowSelectionFeature.utils.ts | 6 +- .../createGroupedRowModel.test.ts | 151 ++++++++++++++++++ .../row-selection/rowSelectionFeature.test.ts | 120 +++++++++++++- perf-done.md | 51 +++++- perf-todo.md | 52 +----- 6 files changed, 335 insertions(+), 57 deletions(-) create mode 100644 packages/table-core/tests/implementation/features/column-grouping/createGroupedRowModel.test.ts diff --git a/packages/table-core/src/features/column-grouping/createGroupedRowModel.ts b/packages/table-core/src/features/column-grouping/createGroupedRowModel.ts index e370593148..2293d43847 100644 --- a/packages/table-core/src/features/column-grouping/createGroupedRowModel.ts +++ b/packages/table-core/src/features/column-grouping/createGroupedRowModel.ts @@ -83,11 +83,17 @@ function _createGroupedRowModel< return rows.map((row) => { row.depth = depth - groupedFlatRows.push(row) - groupedRowsById[row.id] = row - + // Every row is pushed into flatRows/rowsById exactly once, by its + // parent frame: rows returned here are pushed by the caller (the + // parent group's loop or the root loop), so only descendants below + // the terminal depth are pushed here. 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]! + groupedFlatRows.push(subRow) + groupedRowsById[subRow.id] = subRow + } } return row diff --git a/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts b/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts index b21d771bc7..c29ef322bf 100644 --- a/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts +++ b/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts @@ -247,7 +247,7 @@ export function table_getFilteredSelectedRowModel< TFeatures extends TableFeatures, TData extends RowData, >(table: Table_Internal) { - const rowModel = table.getCoreRowModel() + const rowModel = table.getFilteredRowModel() if ( !callMemoOrStaticFn( @@ -281,7 +281,9 @@ export function table_getGroupedSelectedRowModel< TFeatures extends TableFeatures, TData extends RowData, >(table: Table_Internal) { - const rowModel = table.getCoreRowModel() + // The sorted model falls back grouped -> filtered -> core when those + // features are not registered, so selected group rows are always visible. + const rowModel = table.getSortedRowModel() if ( !callMemoOrStaticFn( diff --git a/packages/table-core/tests/implementation/features/column-grouping/createGroupedRowModel.test.ts b/packages/table-core/tests/implementation/features/column-grouping/createGroupedRowModel.test.ts new file mode 100644 index 0000000000..fffbf181f6 --- /dev/null +++ b/packages/table-core/tests/implementation/features/column-grouping/createGroupedRowModel.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it } from 'vitest' +import { + aggregationFns, + columnGroupingFeature, + constructTable, + coreFeatures, + createGroupedRowModel, +} from '../../../../src' +import { storeReactivityBindings } from '../../../../src/store-reactivity-bindings' +import { generateTestData } from '../../../fixtures/data/generateTestData' +import type { Person } from '../../../fixtures/data/types' +import type { ColumnDef, RowModel } from '../../../../src' + +const features = { + ...coreFeatures, + columnGroupingFeature, + coreReactivityFeature: storeReactivityBindings(), + groupedRowModel: createGroupedRowModel(), + aggregationFns, +} + +interface TestRow { + status: string | undefined + firstName: string +} + +const columns: Array> = [ + { accessorKey: 'status', id: 'status' }, + { accessorKey: 'firstName', id: 'firstName' }, +] + +function makeTable(data: Array, grouping: Array) { + return constructTable({ + features, + renderFallbackValue: '', + data, + columns, + initialState: { grouping }, + }) +} + +function expectUniqueFlatRowIds(rowModel: RowModel) { + const ids = rowModel.flatRows.map((row) => row.id) + expect(new Set(ids).size).toBe(ids.length) +} + +describe('createGroupedRowModel flatRows contain every row exactly once', () => { + it('single-level grouping over flat data', () => { + const data: Array = [ + { status: 'a', firstName: 'one' }, + { status: 'a', firstName: 'two' }, + { status: 'a', firstName: 'three' }, + { status: 'b', firstName: 'four' }, + { status: 'b', firstName: 'five' }, + ] + const table = makeTable(data, ['status']) + const rowModel = table.getGroupedRowModel() + + expect(rowModel.rows.length).toBe(2) + // 2 group rows + 5 leaf rows, each exactly once + expect(rowModel.flatRows.length).toBe(7) + expectUniqueFlatRowIds(rowModel) + expect(Object.keys(rowModel.rowsById).length).toBe(7) + }) + + it('two-level grouping over flat data', () => { + const data: Array = [ + { status: 'a', firstName: 'x' }, + { status: 'a', firstName: 'x' }, + { status: 'a', firstName: 'y' }, + { status: 'b', firstName: 'x' }, + ] + const table = makeTable(data, ['status', 'firstName']) + const rowModel = table.getGroupedRowModel() + + // 2 depth-0 groups + 3 depth-1 groups + 4 leaves + expect(rowModel.flatRows.length).toBe(9) + expectUniqueFlatRowIds(rowModel) + + const ids = new Set(rowModel.flatRows.map((row) => row.id)) + expect(ids.has('status:a')).toBe(true) + expect(ids.has('status:b')).toBe(true) + expect(ids.has('status:a>firstName:x')).toBe(true) + expect(ids.has('status:a>firstName:y')).toBe(true) + expect(ids.has('status:b>firstName:x')).toBe(true) + }) + + it('single-level grouping over tree data keeps descendants below the terminal depth exactly once', () => { + // 2 parents, each with 2 children, each with 2 grandchildren + const data = generateTestData(2, 2, 2) + data[0]!.status = 'single' + data[1]!.status = 'complicated' + + const table = constructTable({ + features, + renderFallbackValue: '', + data, + columns: [ + { accessorKey: 'status', id: 'status' }, + { accessorKey: 'firstName', id: 'firstName' }, + ] as Array>, + getSubRows: (originalRow) => originalRow.subRows, + initialState: { grouping: ['status'] }, + }) + const rowModel = table.getGroupedRowModel() + + // 2 group rows + 2 parents + 4 children + 8 grandchildren + expect(rowModel.flatRows.length).toBe(16) + expectUniqueFlatRowIds(rowModel) + + const ids = new Set(rowModel.flatRows.map((row) => row.id)) + for (const id of ['0', '1', '0.0', '0.1', '0.0.0', '0.0.1', '1.1.1']) { + expect(ids.has(id)).toBe(true) + } + + // Depths are rewritten under the grouping: groups 0, parents 1, children 2, grandchildren 3 + expect(rowModel.rows[0]!.depth).toBe(0) + expect(rowModel.rowsById['0']!.depth).toBe(1) + expect(rowModel.rowsById['0.0']!.depth).toBe(2) + expect(rowModel.rowsById['0.0.0']!.depth).toBe(3) + }) + + it('groups rows with undefined grouping values exactly once', () => { + const data: Array = [ + { status: 'a', firstName: 'one' }, + { status: 'a', firstName: 'two' }, + { status: undefined, firstName: 'three' }, + ] + const table = makeTable(data, ['status']) + const rowModel = table.getGroupedRowModel() + + expect(rowModel.rowsById).toHaveProperty('status:undefined') + // 2 group rows + 3 leaf rows + expect(rowModel.flatRows.length).toBe(5) + expectUniqueFlatRowIds(rowModel) + }) + + it('grouping on a nonexistent column keeps every top-level row exactly once', () => { + const data: Array = [ + { status: 'a', firstName: 'one' }, + { status: 'b', firstName: 'two' }, + { status: 'c', firstName: 'three' }, + ] + const table = makeTable(data, ['nope']) + const rowModel = table.getGroupedRowModel() + + expect(rowModel.rows.length).toBe(3) + expect(rowModel.flatRows.length).toBe(3) + expectUniqueFlatRowIds(rowModel) + }) +}) diff --git a/packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts b/packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts index 210ff58bc6..ec89349417 100644 --- a/packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts +++ b/packages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.ts @@ -1,12 +1,24 @@ import { describe, expect, it, vi } from 'vitest' import { + aggregationFns, + columnFilteringFeature, + columnGroupingFeature, constructTable, coreFeatures, createColumnHelper, + createFilteredRowModel, + createGroupedRowModel, + createSortedRowModel, + filterFns, rowSelectionFeature, + rowSortingFeature, + sortFns, } from '../../../../src' import * as RowSelectionUtils from '../../../../src/features/row-selection/rowSelectionFeature.utils' -import { generateTestData } from '../../../fixtures/data/generateTestData' +import { + generateTestData, + getStaticTestData, +} from '../../../fixtures/data/generateTestData' import { storeReactivityBindings } from '../../../../src/store-reactivity-bindings' import type { Person } from '../../../fixtures/data/types' import type { ColumnDef, Row, Table_Internal } from '../../../../src' @@ -544,4 +556,110 @@ describe('rowSelectionFeature', () => { ) }) }) + + describe('selected row models source the correct pipeline models', () => { + // Full pipeline: filtered -> grouped -> sorted, matching v8's sourcing of + // getFilteredSelectedRowModel (filtered) and getGroupedSelectedRowModel (sorted). + const pipelineFeatures = { + ...coreFeatures, + aggregationFns, + columnFilteringFeature, + columnGroupingFeature, + coreReactivityFeature: storeReactivityBindings(), + filterFns, + filteredRowModel: createFilteredRowModel(), + groupedRowModel: createGroupedRowModel(), + rowSelectionFeature, + rowSortingFeature, + sortFns, + sortedRowModel: createSortedRowModel(), + } + + // Static data: row ids default to index — '0' relationship, '1' complicated, '2' single + const pipelineColumns: Array< + ColumnDef + > = [ + { accessorKey: 'firstName', id: 'firstName' }, + { accessorKey: 'status', filterFn: 'equalsString', id: 'status' }, + ] + + function makePipelineTable( + initialState: Record, + features = pipelineFeatures, + ) { + return constructTable({ + features, + enableRowSelection: true, + renderFallbackValue: '', + data: getStaticTestData(), + columns: pipelineColumns, + initialState, + }) + } + + it('getFilteredSelectedRowModel excludes selected rows that are filtered out', () => { + const table = makePipelineTable({ + columnFilters: [{ id: 'status', value: 'single' }], + rowSelection: { '0': true, '2': true }, + }) + + // Row '0' (relationship) fails the filter; row '2' (single) passes + const filteredSelected = table.getFilteredSelectedRowModel() + expect(filteredSelected.rows.map((row) => row.id)).toEqual(['2']) + expect(filteredSelected.flatRows.length).toBe(1) + expect(filteredSelected.rowsById).toHaveProperty('2') + expect(filteredSelected.rowsById).not.toHaveProperty('0') + + // Sanity: the plain selected model stays core-sourced and keeps both rows + expect(table.getSelectedRowModel().flatRows.length).toBe(2) + }) + + it('getGroupedSelectedRowModel returns a selected group row', () => { + const table = makePipelineTable({ + grouping: ['status'], + rowSelection: { 'status:single': true }, + }) + + const groupedSelected = table.getGroupedSelectedRowModel() + expect(groupedSelected.rows.length).toBe(1) + expect(groupedSelected.rows[0]!.id).toBe('status:single') + // The group's only leaf ('2') is unselected, so it is not collected + expect(groupedSelected.flatRows.map((row) => row.id)).toEqual([ + 'status:single', + ]) + }) + + it('getGroupedSelectedRowModel collects a selected leaf under an unselected group', () => { + const table = makePipelineTable({ + grouping: ['status'], + rowSelection: { '2': true }, + }) + + const groupedSelected = table.getGroupedSelectedRowModel() + // No top-level group row is selected... + expect(groupedSelected.rows).toEqual([]) + // ...but the selected leaf is still collected from the grouped tree + expect(groupedSelected.flatRows.map((row) => row.id)).toEqual(['2']) + }) + + it('getGroupedSelectedRowModel falls back through the row-model chain when sorting is not registered', () => { + const { + rowSortingFeature: _sorting, + sortFns: _sortFns, + sortedRowModel: _sorted, + ...rest + } = pipelineFeatures + const table = makePipelineTable( + { + grouping: ['status'], + rowSelection: { 'status:single': true }, + }, + rest as typeof pipelineFeatures, + ) + + const groupedSelected = table.getGroupedSelectedRowModel() + expect(groupedSelected.rows.length).toBe(1) + expect(groupedSelected.rows[0]!.id).toBe('status:single') + }) + }) }) diff --git a/perf-done.md b/perf-done.md index 790915db90..35a9a4fb81 100644 --- a/perf-done.md +++ b/perf-done.md @@ -7,9 +7,10 @@ Entries are sorted by adjusted effectiveness score descending. ## Counts -- **Entries:** 48 -- **Source findings:** 46 +- **Entries:** 49 +- **Source findings:** 47 - **Cross-cutting sweeps:** 2 +- 2026-07-03: #102 (C9) moved here from perf-todo.md after the row-model benchmark confirmed and the fix landed. ## Score 9 @@ -1430,6 +1431,52 @@ Lowercasing an already-lowercased string hits the V8 no-change fast path (no all ## Score 6 +## 102. C9: getFilteredSelectedRowModel / getGroupedSelectedRowModel read the CORE row model while memoDeps declare filtered/sorted models (bug) — Score: 6 (bug) + +**Status:** `[x]` done +**Implementation note:** Fixed 2026-07-03 after the v8↔v9 row-model benchmark empirically confirmed the bug (`benchmark-examples/results/row-model-2026-07-03T15-30-46.270Z.json`): `selection:groupedSelected10Percent` returned an EMPTY row model at every row count (group-row ids like `group:group-0` do not exist in the core model — checksum DIFF vs v8), and `filteredSelected` was measurably slower at 20k (walking R core rows instead of R_filtered). Two-line fix in `rowSelectionFeature.utils.ts`: `table_getFilteredSelectedRowModel` now reads `table.getFilteredRowModel()` (:250) and `table_getGroupedSelectedRowModel` reads `table.getSortedRowModel()` (:284, v8 parity — the sorted model falls back grouped→filtered→core when those features are not registered, so the change is safe for all feature combinations). The registrations' memoDeps already declared the correct models, so no registration change was needed — this was a pure fn-body/deps drift. Regression coverage added in `tests/implementation/features/row-selection/rowSelectionFeature.test.ts` (filtered-selected excludes filtered-out rows; grouped-selected returns selected group rows; selected-leaf-under-unselected-group discriminates grouped vs core sourcing; fallback-chain variant without the sorting feature). The benchmark harness's `filteredSelected10Percent` scenario was also fixed to straddle the filter (`makeStraddledSelectionState`) — its previous selection was a subset of the filter and structurally could not detect this class of bug (verified: against published beta.29 the straddled scenario now flags checksum DIFF, 34 vs 68 output rows). + +**Location:** `packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts:246–267, 280–301` + registrations `packages/table-core/src/features/row-selection/rowSelectionFeature.ts:118–131` +**Category:** `bug` + +All three selected-row-model getters (`getSelectedRowModel`, `getFilteredSelectedRowModel`, `getGroupedSelectedRowModel`) call `selectRowsFn(table.getCoreRowModel(), ...)`, so they return IDENTICAL results: "filtered selected" includes selected rows that are filtered OUT, and "grouped selected" ignores grouping/sorting structure. Meanwhile the memos invalidate on models the fn never reads (spurious O(R) recomputes on filter change) while producing un-filtered output. + +**Before** + +```ts +export function table_getFilteredSelectedRowModel<...>(table: ...) { + const rowModel = table.getCoreRowModel() + // ... + return selectRowsFn(rowModel, table) +} +``` + +**After** + +```ts +export function table_getFilteredSelectedRowModel<...>(table: ...) { + const rowModel = table.getFilteredRowModel() + // ... + return selectRowsFn(rowModel, table) +} + +export function table_getGroupedSelectedRowModel<...>(table: ...) { + // The sorted model falls back grouped -> filtered -> core when those + // features are not registered, so selected group rows are always visible. + const rowModel = table.getSortedRowModel() + // ... + return selectRowsFn(rowModel, table) +} +``` + +**Big-O:** Correctness fix; also removes the perf side effect of walking R core rows where R_filtered would do (measured −50% at 20k rows for the filtered variant pre-fix). + +**Risk:** Behavior change for anyone relying on the broken identical-output behavior; restores documented v8 semantics. + +**Verification:** CONFIRMED-BUG, severity HIGH; the verifier pinned the grouped variant's intended input to `getSortedRowModel` and confirmed all three getters returned identical output. Empirically confirmed by the 2026-07-03 benchmark before fixing. Same failure-mode class as C16 (memoDeps not matching fn reads); a lint-style audit for deps/fn drift is worth considering. + +--- + ## 9. `cell_getContext()` re-allocates the context object on every call — Score: 6 **Status:** `[x]` done diff --git a/perf-todo.md b/perf-todo.md index 6b529c8473..8fcafbaa82 100644 --- a/perf-todo.md +++ b/perf-todo.md @@ -7,9 +7,10 @@ Entries are sorted by adjusted effectiveness score descending. ## Counts -- **Entries:** 91 -- **Source findings:** 91 +- **Entries:** 90 +- **Source findings:** 90 - **Cross-cutting sweeps:** 0 +- 2026-07-03: #102 (C9) completed and moved to perf-done.md. ## Score 8 @@ -1079,53 +1080,6 @@ const extendedCell = Object.assign(cell, boundCellComponents) --- -## 102. C9: getFilteredSelectedRowModel / getGroupedSelectedRowModel read the CORE row model while memoDeps declare filtered/sorted models (bug) — Score: 6 (bug) - -**Status:** `[ ]` not started -**Implementation note:** _(none)_ - -**Location:** `packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts:246–267, 280–301` + registrations `packages/table-core/src/features/row-selection/rowSelectionFeature.ts:118–131` -**Category:** `bug` - -All three selected-row-model getters (`getSelectedRowModel`, `getFilteredSelectedRowModel`, `getGroupedSelectedRowModel`) call `selectRowsFn(table.getCoreRowModel(), ...)`, so they return IDENTICAL results: "filtered selected" includes selected rows that are filtered OUT, and "grouped selected" ignores grouping/sorting structure. Meanwhile the memos invalidate on models the fn never reads (spurious O(R) recomputes on filter change) while producing un-filtered output. - -**Proposed direction (owner decision required; behavior-changing):** `getFilteredSelectedRowModel` should read `table.getFilteredRowModel()`; the grouped variant's intended input is almost certainly `table.getSortedRowModel()` (matching its declared dep and v8 semantics), not `getGroupedRowModel()`. If the duplication is intentional, dedupe all three to the core-model memo instead. Report-only. - -**Before** - -```ts -export function table_getFilteredSelectedRowModel<...>(table: ...) { - const rowModel = table.getCoreRowModel() - // ... - return selectRowsFn(rowModel, table) -} -``` - -with registration: - -```ts -table_getFilteredSelectedRowModel: { - fn: () => table_getFilteredSelectedRowModel(table), - memoDeps: () => [ - table.atoms.rowSelection?.get(), - table.getFilteredRowModel(), - ], -}, -table_getGroupedSelectedRowModel: { - fn: () => table_getGroupedSelectedRowModel(table), - memoDeps: () => [ - table.atoms.rowSelection?.get(), - table.getSortedRowModel(), - ], -}, -``` - -**Big-O:** Perf side effect: three separate O(R) `selectRowsFn` walks per selection change where one shared result would do if the duplication were intentional. - -**Verification:** CONFIRMED-BUG, severity HIGH; the verifier pinned the grouped variant's likely intended input to `getSortedRowModel` and confirmed all three getters currently return identical output. Same failure-mode class as C16 (memoDeps not matching fn reads); a lint-style audit for deps/fn drift is worth considering. - ---- - ## Score 5 ## 3. `memo()` debug timing locals always allocated (broadened by D3: tableMemo prod fast path) — Score: 5