From c983b23a5cba1741ee97cd1356dc1eb103f09724 Mon Sep 17 00:00:00 2001 From: Kevin Van Cott Date: Wed, 10 Jun 2026 22:23:34 -0500 Subject: [PATCH] fix: row selection method cloning bug --- .../rowSelectionFeature.utils.ts | 19 +++--- .../row-selection/rowSelectionFeature.test.ts | 59 +++++++++++++++++++ perf.md | 8 +-- 3 files changed, 75 insertions(+), 11 deletions(-) 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 4062d2ea4e..a04e84510e 100644 --- a/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts +++ b/packages/table-core/src/features/row-selection/rowSelectionFeature.utils.ts @@ -649,7 +649,7 @@ export function selectRowsFn< ): Array> => { const result: Array> = [] for (let i = 0; i < rows.length; i++) { - let row = rows[i]! + const row = rows[i]! const isSelected = isRowSelected(row) if (isSelected) { @@ -658,13 +658,18 @@ export function selectRowsFn< } if (row.subRows.length) { - row = { - ...row, - subRows: recurseRows(row.subRows, depth + 1), + // Always recurse — selected descendants of unselected parents must + // still be collected into flatRows/rowsById. + const newSubRows = recurseRows(row.subRows, depth + 1) + + if (isSelected) { + // Preserve prototype chain so methods like getValue() remain accessible + const cloned = Object.create(Object.getPrototypeOf(row)) + Object.assign(cloned, row) + cloned.subRows = newSubRows + result.push(cloned) } - } - - if (isSelected) { + } else if (isSelected) { result.push(row) } } 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 3759903555..435c4e31f5 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 @@ -96,6 +96,65 @@ describe('rowSelectionFeature', () => { expect(result.rowsById).toHaveProperty('0.0') }) + it('should collect selected descendants of unselected parents', () => { + const data = generateTestData(3, 2) + const columns = generateColumnDefs(data) + + const table = constructTable({ + features, + rowModels: {}, + enableRowSelection: true, + renderFallbackValue: '', + data, + getSubRows: (originalRow: Person, _idx: number) => originalRow.subRows, + initialState: { + rowSelection: { + '0.0': true, // child selected, parent '0' is not + }, + }, + columns, + }) + const rowModel = table.getCoreRowModel() + + const result = RowSelectionUtils.selectRowsFn(rowModel) + + expect(result.rows.length).toBe(0) + expect(result.flatRows.length).toBe(1) + expect(result.flatRows[0]?.id).toBe('0.0') + expect(result.rowsById).toHaveProperty('0.0') + }) + + it('should preserve row prototype methods on cloned parent rows', () => { + const data = generateTestData(3, 2) + const columns = generateColumnDefs(data) + + const table = constructTable({ + features, + rowModels: {}, + enableRowSelection: true, + renderFallbackValue: '', + data, + getSubRows: (originalRow: Person, _idx: number) => originalRow.subRows, + initialState: { + rowSelection: { + '0': true, + '0.0': true, + }, + }, + columns, + }) + const rowModel = table.getCoreRowModel() + + const result = RowSelectionUtils.selectRowsFn(rowModel) + + // Selected parents with subRows are cloned; the clone must keep the + // shared row prototype so APIs like getValue() still work + const clonedParent = result.rows[0]! + expect(clonedParent).not.toBe(rowModel.rows[0]) + expect(typeof clonedParent.getValue).toBe('function') + expect(clonedParent.getValue('id')).toBe(rowModel.rows[0]!.getValue('id')) + }) + it('should return an empty list if no rows are selected', () => { const data = generateTestData(5) const columns = generateColumnDefs(data) diff --git a/perf.md b/perf.md index a7c96565e8..834c00deb7 100644 --- a/perf.md +++ b/perf.md @@ -94,10 +94,10 @@ Typecheck verified clean after the sweep (`pnpm tsc --noEmit` passes). ## Progress - **Total findings:** 61 -- **Done `[x]`:** 19 +- **Done `[x]`:** 20 - **Partial `[~]`:** 2 - **Skipped `[-]`:** 5 -- **Not started `[ ]`:** 35 +- **Not started `[ ]`:** 34 _(Update these counters as you go.)_ @@ -1734,8 +1734,8 @@ Replace `let isAll = …; if (cond) isAll = false; return isAll` with `return !p ## 48. `selectRowsFn` spreads row object even when subRows did not change — Score: 4 -**Status:** `[ ]` not started -**Implementation note:** _(none)_ +**Status:** `[x]` done +**Implementation note:** Implemented a **reframed fix** — the proposed `newSubRows !== row.subRows` check is dead on arrival: `recurseRows` allocates a fresh `result` array on every call and returns it unconditionally, so the reference is _always_ different and the check could never skip a clone. (The scale table's premise is also inverted: an unmatched subtree returns `[]`, never the original reference.) The real waste in the same lines: the clone ran for **every** row with subRows, but unselected rows are never pushed to `result` — so their clone was allocated and immediately discarded. Fix: recurse unconditionally (required — selected descendants of unselected parents must still be collected into `flatRows`/`rowsById`), then build the clone only when `isSelected`. Under sparse selection on hierarchical data this eliminates nearly all clones. **Bug fix included:** rows are `Object.create(rowPrototype)` instances, but the old clone was a plain spread `{ ...row, subRows }` — which drops the prototype, so cloned parent rows in the selected row models lost all their prototype APIs (`getValue()`, etc.). The clone now uses the #49 precedent: `Object.assign(Object.create(Object.getPrototypeOf(row)), row)` + `subRows` assignment. Added regression tests (selected child under unselected parent; prototype-method survival on cloned parents) in `tests/implementation/features/row-selection/rowSelectionFeature.test.ts` — the existing suite only covered selected-child-under-selected-parent and would not have caught a recursion-skipping mistake. **Location:** `src/features/row-selection/rowSelectionFeature.utils.ts:618–658` **Category:** `micro`