Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ export function selectRowsFn<
): Array<Row<TFeatures, TData>> => {
const result: Array<Row<TFeatures, TData>> = []
for (let i = 0; i < rows.length; i++) {
let row = rows[i]!
const row = rows[i]!
const isSelected = isRowSelected(row)

if (isSelected) {
Expand All @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof features, Person>({
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)
Expand Down
8 changes: 4 additions & 4 deletions perf.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.)_

Expand Down Expand Up @@ -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`
Expand Down
Loading