Skip to content

fix(table-core): avoid parent lookup re-entrancy #6441

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.

Sign up for GitHub

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

Merged
KevinVandy merged 1 commit into from
Jul 24, 2026
+59 −3
Merged

fix(table-core): avoid parent lookup re-entrancy #6441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions packages/table-core/src/core/rows/coreRowsFeature.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ export function table_getMaxSubRowDepth<
/**
* Looks up this row's direct parent, if it has one.
*
* Parent lookup searches the pre-pagination row model so parent relationships
* are available even when the parent is not on the current page.
* Parent lookup prefers the core row model for structural parents, then falls
* back to the pre-pagination row model for generated parent rows.
*
* @example
* ```ts
Expand All @@ -200,7 +200,14 @@ export function row_getParentRow<
TFeatures extends TableFeatures,
TData extends RowData,
>(row: Row<TFeatures, TData>) {
return row.parentId ? row.table.getRow(row.parentId, true) : undefined
if (!row.parentId) {
return undefined
}
Comment on lines +203 to +205

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve empty-string row IDs.

Line 203 treats '' as no parent, so custom getRowId implementations using an empty-string ID cannot resolve their children’s parent. Check specifically for undefined.

Proposed fix 
-  if (!row.parentId) {
+  if (row.parentId === undefined) {
     return undefined
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!row.parentId) {
return undefined
}
if (row.parentId === undefined) {
return undefined
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/table-core/src/core/rows/coreRowsFeature.utils.ts` around lines 203
- 205, Update the parent check in the row-parent resolution logic to test
specifically for an undefined parentId, preserving empty-string IDs as valid
values. Keep the existing undefined return behavior when no parent ID is
provided.


return (
row.table.getCoreRowModel().rowsById[row.parentId] ??
row.table.getRow(row.parentId, true)
)
}

/**
Expand Down
49 changes: 49 additions & 0 deletions .../table-core/tests/implementation/features/column-filtering/createFilteredRowModel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,55 @@ const rowNames = (rows: Array<{ original: { name: string } }>) =>
rows.map((row) => row.original.name)

describe('createFilteredRowModel', () => {
it('allows a filter function to inspect structural parent rows', () => {
const parentAwareFilter: FilterFn<typeof features, NestedRow> = (
row,
columnId,
filterValue,
) => {
const parent = row.getParentRow()
return [row, parent]
.filter((candidate) => candidate !== undefined)
.some((candidate) =>
String(candidate.getValue(columnId))
.toLowerCase()
.includes(String(filterValue).toLowerCase()),
)
}
const table = constructTable<typeof features, NestedRow>({
features,
columns: [
{
accessorKey: 'name',
id: 'name',
filterFn: parentAwareFilter,
},
],
data: [
{ name: 'parent', subRows: [{ name: 'child' }] },
{ name: 'other', subRows: [{ name: 'nested' }] },
],
getSubRows: (row) => row.subRows,
initialState: {
columnFilters: [{ id: 'name', value: 'parent' }],
},
})

expect(() => table.getFilteredRowModel()).not.toThrow()
expect(rowNames(table.getFilteredRowModel().flatRows).sort()).toEqual([
'child',
'parent',
])

table.setColumnFilters([{ id: 'name', value: 'other' }])

expect(() => table.getFilteredRowModel()).not.toThrow()
expect(rowNames(table.getFilteredRowModel().flatRows).sort()).toEqual([
'nested',
'other',
])
Comment on lines +107 to +129

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cover descendant matches across multiple nesting levels.

The fixture only has one child level, and both filter values match parents. Add a grandchild plus a descendant-matching filter case, asserting its ancestor family is retained; this validates recursive parent lookup and the linked issue’s multi-level requirement.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/table-core/tests/implementation/features/column-filtering/createFilteredRowModel.test.ts`
around lines 107 - 129, Add a grandchild to the nested-row fixture in the
relevant filtering test, then add a filter case whose value matches only that
deeper descendant. Assert the filtered flat rows retain the matching descendant
and all of its ancestor rows, while preserving the existing parent-match
assertions to cover recursive parent lookup.

})

it('should assign display indexes in filtered row order', () => {
const table = constructTable<typeof features, TestRow>({
features,
Expand Down
Loading