feat: new custom plugins support and massive typescript simplificatoins#6305
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR rewrites table feature typing around feature maps, removes several plugin-merging type layers, converts many core features to direct ChangesFeature-map typing refactor
Sequence Diagram(s)sequenceDiagram
participant CustomFeatureDocs
participant ExamplePlugins
participant CoreTableTypes
participant FeatureWiring
participant DevtoolsAndTests
CustomFeatureDocs->>CoreTableTypes: describe new TableFeature and feature-map contracts
ExamplePlugins->>FeatureWiring: register plugin APIs with assignTableAPIs
FeatureWiring->>CoreTableTypes: consume TableFeature, TableState_All, and feature-map types
DevtoolsAndTests->>CoreTableTypes: use updated table, row, column, and devtools types
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
View your CI Pipeline Execution ↗ for commit a1a2e20
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
KevinVandy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/table-core/src/fns/filterFns.ts (1)
293-302:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
filterFn_arrHasis missingautoRemove, unlike other array filters.All other array filters (
arrIncludes,arrIncludesAll,arrIncludesSome) haveautoRemove: (val) => testFalsy(val) || !val?.lengthto remove the filter when the array is empty.filterFn_arrHaslacks this, which could cause unexpected behavior when users clear the filter array.🔧 Suggested fix for consistency
export const filterFn_arrHas = Object.assign( + <TFeatures extends TableFeatures, TData extends RowData>( -export const filterFn_arrHas = < - TFeatures extends TableFeatures, - TData extends RowData, ->( row: Row<TFeatures, TData>, columnId: string, filterValue: Array<unknown>, -) => { + ) => { return filterValue.some((val) => row.getValue<unknown>(columnId) === val) -} + }, + { autoRemove: (val: any) => testFalsy(val) || !val?.length }, +)🤖 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/fns/filterFns.ts` around lines 293 - 302, The filterFn_arrHas export is missing the autoRemove behavior used by other array filters; update the exported filter (filterFn_arrHas) to include an autoRemove property set to (val) => testFalsy(val) || !val?.length (matching arrIncludes/arrIncludesAll/arrIncludesSome) so the filter is auto-removed when the value is falsy or an empty array; import or reuse the testFalsy helper if needed and ensure the exported shape matches the other array filter fns.
🧹 Nitpick comments (6)
packages/table-core/tests/implementation/features/row-pinning/rowPinningFeature.test.ts (1)
42-42: 💤 Low value
as anycast bypasses column accessor typing.The cast silences type checking on the accessor options. If the typing issue is real (e.g.,
typedKeydoesn't satisfy the accessor's id constraint), this could mask a problem.♻️ Alternative: Remove cast if types align
- return columnHelper.accessor(typedKey, { id: typedKey } as any) + return columnHelper.accessor(typedKey, { id: typedKey })If the cast is necessary, add a comment explaining why the type system requires it.
🤖 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/row-pinning/rowPinningFeature.test.ts` at line 42, The test is silencing TypeScript by casting the accessor options to any in columnHelper.accessor(typedKey, { id: typedKey } as any); either remove the as any cast and make typedKey satisfy the accessor's id type (adjust the typedKey declaration or the generic/type parameter used by columnHelper.accessor) or if the cast is truly unavoidable add a brief comment explaining why the accessor's options typing cannot be satisfied here and reference the specific constraint being bypassed; ensure the change is applied at the call site using columnHelper.accessor and/or the typedKey definition so the typing intent is preserved.docs/guide/custom-features.md (1)
242-261: 💤 Low valueConsider clarifying the
table_prefix convention.The API keys use a
table_prefix (table_setDensity,table_toggleDensity), but users call the methods without this prefix (table.setDensity()). While this internal convention is consistent with the codebase, a brief inline comment explaining thatassignTableAPIsstrips the prefix would help users understand the mapping.For example:
// assignTableAPIs strips the 'table_' prefix when registering methods assignTableAPIs('densityPlugin', table, { table_setDensity: { ... } // → becomes table.setDensity()🤖 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 `@docs/guide/custom-features.md` around lines 242 - 261, Add a brief inline comment above the assignTableAPIs call explaining the internal naming convention: that keys prefixed with "table_" (e.g., table_setDensity, table_toggleDensity) are registered as instance methods without the prefix (e.g., table.setDensity, table.toggleDensity) by assignTableAPIs; reference the assignTableAPIs invocation and the specific keys (table_setDensity, table_toggleDensity) and mention that these map to table.options handlers like table.options.onDensityChange so readers understand the mapping.examples/angular/custom-plugin/src/app/density/density-feature.ts (1)
81-89: 💤 Low valueRedundant wrapper in
table_setDensity.The
safeUpdaterwrapping intable_setDensitydoesn't add any transformation—it just callsfunctionalUpdate(updater, old)and returns the result unchanged. You can passupdaterdirectly toonDensityChange.♻️ Suggested simplification
table_setDensity: { fn: (updater: Updater<DensityState>) => { - const safeUpdater: Updater<DensityState> = (old) => { - const newState = functionalUpdate(updater, old) - return newState - } - return table.options.onDensityChange?.(safeUpdater) + return table.options.onDensityChange?.(updater) }, },🤖 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 `@examples/angular/custom-plugin/src/app/density/density-feature.ts` around lines 81 - 89, The table_setDensity implementation contains a redundant safeUpdater wrapper that merely calls functionalUpdate(updater, old) and returns it; simplify by passing the original updater directly to table.options.onDensityChange instead of creating safeUpdater (update the table_setDensity fn to call table.options.onDensityChange?.(updater)), removing the unnecessary safeUpdater and its use of functionalUpdate in this path.examples/preact/custom-plugin/src/main.tsx (1)
77-118: 💤 Low valueConsider simplifying the redundant
safeUpdaterwrapper intable_setDensity.The
safeUpdaterfunction on lines 101-104 adds no transformation—it just forwards the result offunctionalUpdate. This could be simplified to pass the updater directly.♻️ Optional simplification
table_setDensity: { fn: (updater: Updater<DensityState>) => { - const safeUpdater: Updater<DensityState> = (old) => { - const newState = functionalUpdate(updater, old) - return newState - } - return table.options.onDensityChange?.(safeUpdater) + return table.options.onDensityChange?.(updater) }, },🤖 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 `@examples/preact/custom-plugin/src/main.tsx` around lines 77 - 118, The safeUpdater wrapper in densityPlugin -> constructTableAPIs for table_setDensity is redundant: instead of creating const safeUpdater that just computes functionalUpdate(updater, old) and returns it, pass the provided updater directly into the table options handler; replace the block in table_setDensity.fn so it calls table.options.onDensityChange?.(updater) (preserving the Updater<DensityState> type) rather than wrapping it in safeUpdater. This removes unnecessary indirection while keeping behavior identical.packages/table-core/src/core/table/constructTable.ts (1)
118-125: 💤 Low valueNon-null assertion relies on initialization invariant.
Line 125 uses
table.baseAtoms[key]!.get()with a non-null assertion. This assumes every key instateKeyshas a corresponding base atom. Since base atoms are created for each key in lines 106-114, this is safe—but the invariant depends onstateKeysbeing derived fromtable.initialStateimmediately before the atom creation loop.Consider adding a runtime assertion in development mode to catch violations of this invariant if the code is refactored:
Optional defensive check
const externalAtom = externalAtoms?.[key] if (externalAtom) { return externalAtom.get() } + if (process.env.NODE_ENV === 'development' && !table.baseAtoms[key]) { + throw new Error(`Missing base atom for state key: ${String(key)}`) + } return table.baseAtoms[key]!.get()🤖 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/table/constructTable.ts` around lines 118 - 125, The non-null assertion on table.baseAtoms[key]!.get() in constructTable assumes base atoms were created for every key in stateKeys; add a runtime defensive check (only in development) before calling .get(): verify table.baseAtoms[key] exists and throw a clear error or use console.assert/invariant naming the missing key and referencing stateKeys and the atom-creation loop (the code that builds baseAtoms) so refactors will surface the invariant violation early; update the code path that reads externalAtoms/externalAtom to use this check and keep behavior unchanged in production.packages/angular-table/src/helpers/createTableHook.ts (1)
470-477:injectAppTablecasts: avoidunknownby tightening thedefaultTableOptions/TDatatypingThe object passed into
injectTable<TFeatures, TData>is constructed fromdefaultTableOptions(coming fromCreateTableContextOptions, which is based onTableOptions<TFeatures, any>) plustableOptions()(which omitsfeatures/rowModels). ThatTData=anysource is the real reason for theas unknown as TableOptions<TFeatures, TData>bridge at lines 470-470-ish—so the cast isn’t obviously masking a runtime shape mismatch, but it does bypass type-checking across theTDatageneric boundary.The final cast to
AppAngularTable<...>is supported by runtime:appTableFeatures.constructTableAPIsassignsappCell/appHeader/appFooteronto the created table.No
angular-tabletests/specs cover thisinjectAppTabletyping path, so to reduce risk, tighten the intermediate typing to eliminate theunknowncasts (e.g., makedefaultTableOptionspreserveTDatainstead ofany).🤖 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/angular-table/src/helpers/createTableHook.ts` around lines 470 - 477, The current casts around injectTable/injectAppTable hide a TData=any leak from defaultTableOptions/CreateTableContextOptions; fix by making defaultTableOptions and CreateTableContextOptions generic over TData (i.e., preserve TableOptions<TFeatures, TData> instead of defaulting to any) so the constructed object already matches TableOptions<TFeatures, TData>, then remove the intermediate "as unknown as TableOptions..." and final cast to AppAngularTable; adjust the injectTable<>/injectAppTable<> call sites and the signature of defaultTableOptions (and any helper that builds it) so appTableFeatures.constructTableAPIs can still assign appCell/appHeader/appFooter without needing unsafe casts.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/table-core/src/features/column-sizing/columnSizingFeature.ts`:
- Around line 93-103: The memoization dependencies for the registered selector
header_getStart include an unnecessary position argument; update the
registration in columnSizingFeature so memoDeps for header_getStart only depends
on header-related atoms (remove position from memoDeps array) OR alternatively,
if header offsets should vary by position/pinning, update the header_getStart
utility to accept and propagate position and then keep position in
memoDeps—refer to the header_getStart registration and the header_getStart
function to implement the chosen fix.
In `@packages/table-core/src/fns/filterFns.ts`:
- Around line 127-147: The comparison filter fns are using resolveFilterValue
incorrectly; replace the object passed to Object.assign in filterFn_greaterThan,
filterFn_greaterThanOrEqualTo, filterFn_lessThan, and filterFn_lessThanOrEqualTo
so they use the autoRemove hook instead of resolveFilterValue — e.g. change {
resolveFilterValue: (val:any) => testFalsy(val) } to { autoRemove: testFalsy }
(refer to the symbols filterFn_greaterThan, filterFn_greaterThanOrEqualTo,
filterFn_lessThan, filterFn_lessThanOrEqualTo to locate each assignment).
In `@packages/table-core/tests/helpers/generateTestTable.ts`:
- Line 37: The test helper is using a double-cast ("as any) as unknown as
Table_Internal<TFeatures, Person>") because constructTable returns a
Table<TFeatures, Person> that is not assignable to Table_Internal<TFeatures,
Person>; fix by making the types compatible instead of escaping the checker:
either change constructTable (or its factory) to declare/return
Table_Internal<TFeatures, Person> if the internals are required in tests, or
change generateTestTable to return the public Table<TFeatures, Person> and
update tests to use only the public API; locate constructTable and
generateTestTable and reconcile their signatures/members (ensure required
internal methods/properties exist on the chosen type), remove the double-cast,
and re-run type checks to confirm no further mismatches.
- Line 86: The code unsafely casts table.options to any to set state; instead
make the update type-safe by updating the test typings or API: either change the
generateTestTable/createTestTable helper to accept a typed state parameter and
assign it through the helper, or declare a specific mutable options type (e.g.,
MutableTableOptions) and cast to that when assigning (use as unknown as
MutableTableOptions) so you don't disable TS checks entirely; if you must bypass
types, add a clear comment documenting the reason and the expected shape of
state and reference the symbols table.options and state in the helper to ensure
future reviewers know this is intentional.
---
Outside diff comments:
In `@packages/table-core/src/fns/filterFns.ts`:
- Around line 293-302: The filterFn_arrHas export is missing the autoRemove
behavior used by other array filters; update the exported filter
(filterFn_arrHas) to include an autoRemove property set to (val) =>
testFalsy(val) || !val?.length (matching
arrIncludes/arrIncludesAll/arrIncludesSome) so the filter is auto-removed when
the value is falsy or an empty array; import or reuse the testFalsy helper if
needed and ensure the exported shape matches the other array filter fns.
---
Nitpick comments:
In `@docs/guide/custom-features.md`:
- Around line 242-261: Add a brief inline comment above the assignTableAPIs call
explaining the internal naming convention: that keys prefixed with "table_"
(e.g., table_setDensity, table_toggleDensity) are registered as instance methods
without the prefix (e.g., table.setDensity, table.toggleDensity) by
assignTableAPIs; reference the assignTableAPIs invocation and the specific keys
(table_setDensity, table_toggleDensity) and mention that these map to
table.options handlers like table.options.onDensityChange so readers understand
the mapping.
In `@examples/angular/custom-plugin/src/app/density/density-feature.ts`:
- Around line 81-89: The table_setDensity implementation contains a redundant
safeUpdater wrapper that merely calls functionalUpdate(updater, old) and returns
it; simplify by passing the original updater directly to
table.options.onDensityChange instead of creating safeUpdater (update the
table_setDensity fn to call table.options.onDensityChange?.(updater)), removing
the unnecessary safeUpdater and its use of functionalUpdate in this path.
In `@examples/preact/custom-plugin/src/main.tsx`:
- Around line 77-118: The safeUpdater wrapper in densityPlugin ->
constructTableAPIs for table_setDensity is redundant: instead of creating const
safeUpdater that just computes functionalUpdate(updater, old) and returns it,
pass the provided updater directly into the table options handler; replace the
block in table_setDensity.fn so it calls
table.options.onDensityChange?.(updater) (preserving the Updater<DensityState>
type) rather than wrapping it in safeUpdater. This removes unnecessary
indirection while keeping behavior identical.
In `@packages/angular-table/src/helpers/createTableHook.ts`:
- Around line 470-477: The current casts around injectTable/injectAppTable hide
a TData=any leak from defaultTableOptions/CreateTableContextOptions; fix by
making defaultTableOptions and CreateTableContextOptions generic over TData
(i.e., preserve TableOptions<TFeatures, TData> instead of defaulting to any) so
the constructed object already matches TableOptions<TFeatures, TData>, then
remove the intermediate "as unknown as TableOptions..." and final cast to
AppAngularTable; adjust the injectTable<>/injectAppTable<> call sites and the
signature of defaultTableOptions (and any helper that builds it) so
appTableFeatures.constructTableAPIs can still assign appCell/appHeader/appFooter
without needing unsafe casts.
In `@packages/table-core/src/core/table/constructTable.ts`:
- Around line 118-125: The non-null assertion on table.baseAtoms[key]!.get() in
constructTable assumes base atoms were created for every key in stateKeys; add a
runtime defensive check (only in development) before calling .get(): verify
table.baseAtoms[key] exists and throw a clear error or use
console.assert/invariant naming the missing key and referencing stateKeys and
the atom-creation loop (the code that builds baseAtoms) so refactors will
surface the invariant violation early; update the code path that reads
externalAtoms/externalAtom to use this check and keep behavior unchanged in
production.
In
`@packages/table-core/tests/implementation/features/row-pinning/rowPinningFeature.test.ts`:
- Line 42: The test is silencing TypeScript by casting the accessor options to
any in columnHelper.accessor(typedKey, { id: typedKey } as any); either remove
the as any cast and make typedKey satisfy the accessor's id type (adjust the
typedKey declaration or the generic/type parameter used by
columnHelper.accessor) or if the cast is truly unavoidable add a brief comment
explaining why the accessor's options typing cannot be satisfied here and
reference the specific constraint being bypassed; ensure the change is applied
at the call site using columnHelper.accessor and/or the typedKey definition so
the typing intent is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b99d985-1813-42dc-9a48-ce877cfad5d8
📒 Files selected for processing (101)
docs/guide/custom-features.mdexamples/angular/custom-plugin/src/app/density/density-feature.tsexamples/preact/custom-plugin/src/main.tsxexamples/react/custom-plugin/src/main.tsxexamples/react/kitchen-sink-shadcn-base/src/components/data-table/data-table-column-header.tsxexamples/react/kitchen-sink-shadcn-base/src/components/data-table/data-table-filter-list.tsxexamples/react/kitchen-sink-shadcn-base/src/components/data-table/data-table-pagination.tsxexamples/react/kitchen-sink-shadcn-base/src/components/data-table/data-table-sort-list.tsxexamples/react/kitchen-sink-shadcn-base/src/components/data-table/data-table-view-options.tsxexamples/react/kitchen-sink-shadcn-radix/src/components/data-table/data-table-column-header.tsxexamples/react/kitchen-sink-shadcn-radix/src/components/data-table/data-table-filter-list.tsxexamples/react/kitchen-sink-shadcn-radix/src/components/data-table/data-table-pagination.tsxexamples/react/kitchen-sink-shadcn-radix/src/components/data-table/data-table-sort-list.tsxexamples/react/kitchen-sink-shadcn-radix/src/components/data-table/data-table-view-options.tsxexamples/svelte/sorting/src/App.svelteexamples/svelte/sorting/src/Header.sveltepackages/angular-table-devtools/src/index.tspackages/angular-table-devtools/src/injectTanStackTableDevtools.tspackages/angular-table/src/helpers/cell.tspackages/angular-table/src/helpers/createTableHook.tspackages/angular-table/src/helpers/header.tspackages/angular-table/src/helpers/table.tspackages/angular-table/tests/injectTable.test.tspackages/react-table-devtools/src/table-devtools.d.tspackages/react-table/src/useLegacyTable.tspackages/table-core/src/core/cells/coreCellsFeature.tspackages/table-core/src/core/columns/coreColumnsFeature.tspackages/table-core/src/core/headers/buildHeaderGroups.tspackages/table-core/src/core/headers/coreHeadersFeature.tspackages/table-core/src/core/headers/coreHeadersFeature.utils.tspackages/table-core/src/core/row-models/coreRowModelsFeature.tspackages/table-core/src/core/row-models/coreRowModelsFeature.types.tspackages/table-core/src/core/row-models/createCoreRowModel.tspackages/table-core/src/core/rows/coreRowsFeature.tspackages/table-core/src/core/table/constructTable.tspackages/table-core/src/core/table/coreTablesFeature.tspackages/table-core/src/core/table/coreTablesFeature.utils.tspackages/table-core/src/features/column-faceting/columnFacetingFeature.tspackages/table-core/src/features/column-faceting/createFacetedMinMaxValues.tspackages/table-core/src/features/column-faceting/createFacetedRowModel.tspackages/table-core/src/features/column-faceting/createFacetedUniqueValues.tspackages/table-core/src/features/column-filtering/columnFilteringFeature.tspackages/table-core/src/features/column-filtering/columnFilteringFeature.utils.tspackages/table-core/src/features/column-filtering/createFilteredRowModel.tspackages/table-core/src/features/column-grouping/columnGroupingFeature.tspackages/table-core/src/features/column-grouping/createGroupedRowModel.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.tspackages/table-core/src/features/column-pinning/columnPinningFeature.tspackages/table-core/src/features/column-pinning/columnPinningFeature.utils.tspackages/table-core/src/features/column-resizing/columnResizingFeature.tspackages/table-core/src/features/column-sizing/columnSizingFeature.tspackages/table-core/src/features/column-visibility/columnVisibilityFeature.tspackages/table-core/src/features/global-filtering/globalFilteringFeature.tspackages/table-core/src/features/row-expanding/createExpandedRowModel.tspackages/table-core/src/features/row-expanding/rowExpandingFeature.tspackages/table-core/src/features/row-pagination/createPaginatedRowModel.tspackages/table-core/src/features/row-pagination/rowPaginationFeature.tspackages/table-core/src/features/row-pinning/rowPinningFeature.tspackages/table-core/src/features/row-selection/rowSelectionFeature.tspackages/table-core/src/features/row-sorting/createSortedRowModel.tspackages/table-core/src/features/row-sorting/rowSortingFeature.tspackages/table-core/src/fns/aggregationFns.tspackages/table-core/src/fns/filterFns.tspackages/table-core/src/fns/sortFns.tspackages/table-core/src/types/Cell.tspackages/table-core/src/types/Column.tspackages/table-core/src/types/ColumnDef.tspackages/table-core/src/types/Header.tspackages/table-core/src/types/HeaderGroup.tspackages/table-core/src/types/Row.tspackages/table-core/src/types/RowModel.tspackages/table-core/src/types/RowModelFns.tspackages/table-core/src/types/Table.tspackages/table-core/src/types/TableFeatures.tspackages/table-core/src/types/TableOptions.tspackages/table-core/src/types/TableState.tspackages/table-core/src/utils.tspackages/table-core/tests/helpers/generateTestTable.tspackages/table-core/tests/implementation/features/row-pinning/rowPinningFeature.test.tspackages/table-core/tests/implementation/features/row-selection/rowSelectionFeature.test.tspackages/table-core/tests/performance/features/column-grouping/columnGroupingFeature.test.tspackages/table-core/tests/unit/core/cells/constructCell.test.tspackages/table-core/tests/unit/core/columns/constructColumn.test.tspackages/table-core/tests/unit/core/headers/constructHeader.test.tspackages/table-core/tests/unit/core/rows/constructRow.test.tspackages/table-core/tests/unit/core/table/stockFeaturesInitialState.test.tspackages/table-core/tests/unit/core/tableAtoms.test.tspackages/table-core/tests/unit/features/column-ordering/columnOrderingFeature.utils.test.tspackages/table-core/tests/unit/features/column-pinning/columnPinningFeature.utils.test.tspackages/table-core/tests/unit/features/column-resizing/columnResizingFeature.utils.test.tspackages/table-core/tests/unit/features/column-visibility/columnVisibilityFeature.utils.test.tspackages/table-core/tests/unit/features/row-pinning/rowPinningFeature.utils.test.tspackages/table-devtools/src/TableContextProvider.tsxpackages/table-devtools/src/components/ColumnsPanel.tsxpackages/table-devtools/src/components/FeaturesPanel.tsxpackages/table-devtools/src/components/OptionsPanel.tsxpackages/table-devtools/src/index.tspackages/table-devtools/src/production.tspackages/table-devtools/src/tableTarget.tspackages/vue-table/src/FlexRender.tspackages/vue-table/src/createTableHook.ts
Comment on lines
+93
to
+103
| header_getStart: { | ||
| fn: (header) => header_getStart(header), | ||
| memoDeps: (header, position) => [ | ||
| position, | ||
| table.options.columns, | ||
| table.atoms.columnSizing?.get(), | ||
| table.atoms.columnOrder?.get(), | ||
| table.atoms.columnPinning?.get(), | ||
| table.atoms.columnVisibility?.get(), | ||
| ], | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the signature of header_getStart to determine if it accepts a position parameter
ast-grep --pattern 'function header_getStart($_) { $$$ }'
ast-grep --pattern 'export function header_getStart($_) { $$$ }'
rg -nA5 'export function header_getStart' --type tsRepository: TanStack/table
Length of output: 749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/table-core/src/features/column-sizing/columnSizingFeature.ts"
# Show the header_getStart registration block with a bit more context
sed -n '70,130p' "$FILE" | nl -ba
# Show the column_getStart registration block for comparison (as referenced in the original comment)
sed -n '35,80p' "$FILE" | nl -ba
# Show the types/definition of the expected shape (where memoDeps/ fn signatures are declared), if present
rg -n "memoDeps|fn: \\(" "$FILE" --context 2
# Confirm whether any other header_getStart overload exists (position-less only, vs additional signatures)
rg -n "header_getStart\\b" packages/table-core/src/features/column-sizing --type tsRepository: TanStack/table
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/table-core/src/features/column-sizing/columnSizingFeature.ts"
echo "=== columnSizingFeature.ts (lines ~85-115) ==="
awk 'NR>=85 && NR<=115 {printf "%5d:%s\n", NR, $0}' "$FILE"
echo "=== columnSizingFeature.ts (lines ~40-85) ==="
awk 'NR>=40 && NR<=85 {printf "%5d:%s\n", NR, $0}' "$FILE"
echo "=== Find header_getStart registration occurrences ==="
rg -n "header_getStart" "$FILE"
echo "=== Locate type(s) that define memoDeps/registration function signatures ==="
# Search nearby for where the object shape is typed (best-effort)
rg -n "memoDeps" "$FILE" --context 3
rg -n "fn:" "$FILE" --context 3Repository: TanStack/table
Length of output: 8593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where assignPrototypeAPIs is implemented/typed
rg -n "assignPrototypeAPIs\\(" packages/table-core/src --type ts
# 2) Show the definition (first match) of assignPrototypeAPIs
DEF_FILE="$(rg -n "function assignPrototypeAPIs\\b|const assignPrototypeAPIs\\b|export function assignPrototypeAPIs\\b" packages/table-core/src --type ts -l | head -n 1 || true)"
echo "DEF_FILE=$DEF_FILE"
if [ -n "${DEF_FILE}" ]; then
awk 'NR>=1 && NR<=220 {print NR":"$0}' "$DEF_FILE" | head -n 220
fi
# 3) Inspect header_getStart implementation details
UTIL_FILE="packages/table-core/src/features/column-sizing/columnSizingFeature.utils.ts"
echo "=== header_getStart implementation (around line 215-270) ==="
awk 'NR>=210 && NR<=280 {printf "%5d:%s\n", NR, $0}' "$UTIL_FILE"Repository: TanStack/table
Length of output: 12419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find definition of assignPrototypeAPIs
rg -n "function assignPrototypeAPIs\\b|const assignPrototypeAPIs\\b|export function assignPrototypeAPIs\\b" packages/table-core/src --type ts
# Show surrounding implementation for first match
FILE="$(rg -n "function assignPrototypeAPIs\\b|const assignPrototypeAPIs\\b|export function assignPrototypeAPIs\\b" packages/table-core/src --type ts -l | head -n 1 || true)"
echo "DEF: $FILE"
if [ -n "${FILE}" ]; then
awk 'NR>=1 && NR<=260 {print NR":"$0}' "$FILE" | head -n 260
fi
# Locate callMemoOrStaticFn which is used by header_getStart
rg -n "callMemoOrStaticFn\\b" packages/table-core/src --type ts -S | head -n 20
CALL_FILE="$(rg -n "callMemoOrStaticFn\\b" packages/table-core/src --type ts -l | head -n 1 || true)"
echo "CALL_FILE=$CALL_FILE"
if [ -n "${CALL_FILE}" ]; then
awk 'NR>=1 && NR<=260 {print NR":"$0}' "$CALL_FILE" | head -n 260
fi
# Find usage of header_getStart/getStart with a position arg to confirm runtime deps
rg -n "header_getStart\\(" packages/table-core/src --type ts
rg -n "getStart\\b" packages/table-core/src/features packages/table-core/src/core --type ts | head -n 50Repository: TanStack/table
Length of output: 20843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
UTIL="packages/table-core/src/utils.ts"
# Print assignPrototypeAPIs section around where it was found (line ~382)
awk 'NR>=360 && NR<=520 {printf "%5d:%s\n", NR, $0}' "$UTIL"
# Print callMemoOrStaticFn around where it was found (~428)
awk 'NR>=410 && NR<=500 {printf "%5d:%s\n", NR, $0}' "$UTIL"
TYPES_FILE="packages/table-core/src/features/column-sizing/columnSizingFeature.types.ts"
echo "=== columnSizingFeature.types.ts (relevant getStart typing) ==="
rg -n "header_getStart|getStart\\b" "$TYPES_FILE" --context 2
awk 'NR>=60 && NR<=120 {printf "%5d:%s\n", NR, $0}' "$TYPES_FILE"
FEATURE_TS="packages/table-core/src/features/column-sizing/columnSizingFeature.ts"
echo "=== columnSizingFeature.ts (header_getStart registration) ==="
awk 'NR>=80 && NR<=110 {printf "%5d:%s\n", NR, $0}' "$FEATURE_TS"Repository: TanStack/table
Length of output: 7554
Remove redundant position from header_getStart memo deps
In packages/table-core/src/features/column-sizing/columnSizingFeature.ts (lines 93-103), header_getStart is registered with fn: (header) => header_getStart(header) but memoDeps includes position (memoDeps: (header, position) => [position, ...]). The header_getStart utility only accepts header (and doesn’t propagate position to sibling headers), so position doesn’t affect the computed result—only memo cache invalidation. Drop position from memoDeps or update header_getStart to accept/propagate position if header offsets should vary with pinning.
🤖 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/features/column-sizing/columnSizingFeature.ts` around
lines 93 - 103, The memoization dependencies for the registered selector
header_getStart include an unnecessary position argument; update the
registration in columnSizingFeature so memoDeps for header_getStart only depends
on header-related atoms (remove position from memoDeps array) OR alternatively,
if header offsets should vary by position/pinning, update the header_getStart
utility to accept and propagate position and then keep position in
memoDeps—refer to the header_getStart registration and the header_getStart
function to implement the chosen fix.
Comment on lines
+127
to
+147
| export const filterFn_greaterThan = Object.assign( | ||
| <TFeatures extends TableFeatures, TData extends RowData>( | ||
| row: Row<TFeatures, TData>, | ||
| columnId: string, | ||
| filterValue: unknown, | ||
| ) => { | ||
| const rowValue = row.getValue(columnId) | ||
| const numericRowValue = | ||
| rowValue === null || rowValue === undefined ? 0 : +rowValue | ||
| const numericFilterValue = Number(filterValue) | ||
|
|
||
| if (!isNaN(numericFilterValue) && !isNaN(numericRowValue)) { | ||
| return numericRowValue > numericFilterValue | ||
| } | ||
|
|
||
| const stringValue = (rowValue ?? '').toString().toLowerCase().trim() | ||
| const stringFilterValue = String(filterValue).toLowerCase().trim() | ||
| return stringValue > stringFilterValue | ||
| }, | ||
| { resolveFilterValue: (val: any) => testFalsy(val) }, | ||
| ) |
Contributor
There was a problem hiding this comment.
resolveFilterValue should be autoRemove for comparison filters.
The resolveFilterValue hook is meant to transform the filter value before comparison (see filterFn_inNumberRange which returns [min, max]). Here it's set to testFalsy which returns a boolean, but comparison filters don't need value transformation—they need auto-removal of empty values.
The same issue appears in filterFn_greaterThanOrEqualTo (line 165), filterFn_lessThan (line 181), and filterFn_lessThanOrEqualTo (line 197).
🐛 Proposed fix
export const filterFn_greaterThan = Object.assign(
<TFeatures extends TableFeatures, TData extends RowData>(
row: Row<TFeatures, TData>,
columnId: string,
filterValue: unknown,
) => {
const rowValue = row.getValue(columnId)
const numericRowValue =
rowValue === null || rowValue === undefined ? 0 : +rowValue
const numericFilterValue = Number(filterValue)
if (!isNaN(numericFilterValue) && !isNaN(numericRowValue)) {
return numericRowValue > numericFilterValue
}
const stringValue = (rowValue ?? '').toString().toLowerCase().trim()
const stringFilterValue = String(filterValue).toLowerCase().trim()
return stringValue > stringFilterValue
},
- { resolveFilterValue: (val: any) => testFalsy(val) },
+ { autoRemove: (val: any) => testFalsy(val) },
)Apply the same change to filterFn_greaterThanOrEqualTo, filterFn_lessThan, and filterFn_lessThanOrEqualTo.
📝 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
| export const filterFn_greaterThan = Object.assign( | |
| <TFeatures extends TableFeatures, TData extends RowData>( | |
| row: Row<TFeatures, TData>, | |
| columnId: string, | |
| filterValue: unknown, | |
| ) => { | |
| const rowValue = row.getValue(columnId) | |
| const numericRowValue = | |
| rowValue === null || rowValue === undefined ? 0 : +rowValue | |
| const numericFilterValue = Number(filterValue) | |
| if (!isNaN(numericFilterValue) && !isNaN(numericRowValue)) { | |
| return numericRowValue > numericFilterValue | |
| } | |
| const stringValue = (rowValue ?? '').toString().toLowerCase().trim() | |
| const stringFilterValue = String(filterValue).toLowerCase().trim() | |
| return stringValue > stringFilterValue | |
| }, | |
| { resolveFilterValue: (val: any) => testFalsy(val) }, | |
| ) | |
| export const filterFn_greaterThan = Object.assign( | |
| <TFeatures extends TableFeatures, TData extends RowData>( | |
| row: Row<TFeatures, TData>, | |
| columnId: string, | |
| filterValue: unknown, | |
| ) => { | |
| const rowValue = row.getValue(columnId) | |
| const numericRowValue = | |
| rowValue === null || rowValue === undefined ? 0 : +rowValue | |
| const numericFilterValue = Number(filterValue) | |
| if (!isNaN(numericFilterValue) && !isNaN(numericRowValue)) { | |
| return numericRowValue > numericFilterValue | |
| } | |
| const stringValue = (rowValue ?? '').toString().toLowerCase().trim() | |
| const stringFilterValue = String(filterValue).toLowerCase().trim() | |
| return stringValue > stringFilterValue | |
| }, | |
| { autoRemove: (val: any) => testFalsy(val) }, | |
| ) |
🤖 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/fns/filterFns.ts` around lines 127 - 147, The
comparison filter fns are using resolveFilterValue incorrectly; replace the
object passed to Object.assign in filterFn_greaterThan,
filterFn_greaterThanOrEqualTo, filterFn_lessThan, and filterFn_lessThanOrEqualTo
so they use the autoRemove hook instead of resolveFilterValue — e.g. change {
resolveFilterValue: (val:any) => testFalsy(val) } to { autoRemove: testFalsy }
(refer to the symbols filterFn_greaterThan, filterFn_greaterThanOrEqualTo,
filterFn_lessThan, filterFn_lessThanOrEqualTo to locate each assignment).
| coreReativityFeature: storeReactivityBindings(), | ||
| }, | ||
| } as any) | ||
| } as any) as unknown as Table_Internal<TFeatures, Person> |
Contributor
There was a problem hiding this comment.
Double casting suggests type misalignment between Table and Table_Internal.
The pattern as any) as unknown as Table_Internal<TFeatures, Person> is a complete type-system escape. This indicates that Table<TFeatures, Person> returned by constructTable is not compatible with Table_Internal<TFeatures, Person>, forcing the cast chain. While this unblocks the tests, it masks the underlying type incompatibility and could hide real type errors.
Consider whether Table_Internal should be the direct return type from constructTable, or whether test helpers should work with the public Table type instead.
Also applies to: 54-54
🤖 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/helpers/generateTestTable.ts` at line 37, The test
helper is using a double-cast ("as any) as unknown as Table_Internal<TFeatures,
Person>") because constructTable returns a Table<TFeatures, Person> that is not
assignable to Table_Internal<TFeatures, Person>; fix by making the types
compatible instead of escaping the checker: either change constructTable (or its
factory) to declare/return Table_Internal<TFeatures, Person> if the internals
are required in tests, or change generateTestTable to return the public
Table<TFeatures, Person> and update tests to use only the public API; locate
constructTable and generateTestTable and reconcile their signatures/members
(ensure required internal methods/properties exist on the chosen type), remove
the double-cast, and re-run type checks to confirm no further mismatches.
| } | ||
|
|
||
| table.options.state = state | ||
| ;(table.options as any).state = state |
Contributor
There was a problem hiding this comment.
Unsafe state mutation bypasses type safety.
Casting table.options to any before assigning state completely disables TypeScript checking. If options.state is readonly or has a different shape, this will silently break at runtime.
🔒 Consider using a type-safe state update approach
If table.options.state is intentionally mutable for tests, declare it as such in the test typing. If it's meant to be readonly, use proper reactivity APIs instead of bypassing the type system:
- ;(table.options as any).state = state
+ // If options.state is meant to be mutable for test purposes:
+ table.options.state = state
+ // Or use proper table API if availableIf the assignment legitimately requires an any cast, document why with a comment explaining the type constraint being worked around.
🤖 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/helpers/generateTestTable.ts` at line 86, The code
unsafely casts table.options to any to set state; instead make the update
type-safe by updating the test typings or API: either change the
generateTestTable/createTestTable helper to accept a typed state parameter and
assign it through the helper, or declare a specific mutable options type (e.g.,
MutableTableOptions) and cast to that when assigning (use as unknown as
MutableTableOptions) so you don't disable TS checks entirely; if you must bypass
types, add a clear comment documenting the reason and the expected shape of
state and reference the symbols table.options and state in the helper to ensure
future reviewers know this is intentional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
This PR adds first-class custom table plugin support while simplifying and tightening the TypeScript model across core features, adapters, devtools, and examples. It updates custom-feature docs and React/Preact/Angular plugin examples, improves feature-map typing for table/core APIs, fixes built-in sort/filter/aggregation function typing, makes devtools registration work with concrete table instances, restores proper contextual typing for legacy column helpers, and updates affected React/Svelte/Vue/Angular example types to use the real TanStack Table feature types instead of workaround shapes.
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Release Notes
New Features
Documentation
Examples
Refactor