feat(table): support external reactivity binding#6237
feat(table): support external reactivity binding#6237riccardoperra wants to merge 17 commits intoalphafrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces notifier-based reactivity with a TableReactivityBindings abstraction, adds framework-specific reactivity adapters (Angular, Preact, React, Solid, Svelte, Vue), refactors core table internals to consume these bindings, updates many tests/helpers to inject the new bindings, and applies assorted example/UI and dependency tweaks. ChangesCore reactivity migration
Framework adapters & adapter surface
Angular tests, config, and benchmarks
Examples, UI handlers, and misc updates
Sequence Diagram(s)sequenceDiagram
participant App as App / Framework
participant Adapter as Framework Adapter (useTable / injectTable)
participant Core as constructTable / Table Core
participant Reactivity as TableReactivityBindings
participant Table as Table Instance
App->>Adapter: call useTable/injectTable(tableOptions)
Adapter->>Core: constructTable({...tableOptions, _features.coreReativityFeature})
Core->>Reactivity: create optionsStore/atoms via bindings
Adapter->>Table: observe state/options and call table.setOptions(merged) on changes
Reactivity->>Core: batch / untrack used during internal updates
Table->>Adapter: expose state/options via store/atoms -> propagate to App
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
View your CI Pipeline Execution ↗ for commit ae7eb1f
☁️ Nx Cloud last updated this comment at |
riccardoperra
force-pushed
the
feat/native-reactivity
branch
from
5d74b34 to
20e26ec
Compare
riccardoperra
force-pushed
the
feat/native-reactivity
branch
2 times, most recently
from
e54fed5 to
fc27ad3
Compare
riccardoperra
force-pushed
the
feat/native-reactivity
branch
from
fc27ad3 to
5a5be8b
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/table-core/src/core/table/coreTablesFeature.utils.ts (1)
11-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
table_reset()no longer resets externally bound state slices.After this PR,
constructTable()resolves each slice fromoptions.atoms?.[key]before falling back tobaseAtoms[key], buttable_reset()still only writesbaseAtoms. In the new native-reactivity mode,table.reset()becomes a no-op for any slice backed by an external atom because the visible source of truth never changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/src/core/table/coreTablesFeature.utils.ts` around lines 11 - 16, table_reset() currently writes only to baseAtoms so slices bound to external atoms via options.atoms are not reset; update the reset logic to resolve the actual atom for each key (the same resolution constructTable uses: options.atoms?.[key] fallback to baseAtoms[key]) and write the snap value into that resolved atom. Concretely, in the reset implementation that uses cloneState(table.initialState) and table.reactivity.batch(), iterate keys and call set on the resolved atom map (e.g., the per-slice atoms object created during constructTable or a helper that resolves options.atoms?.[key] || baseAtoms[key]) instead of always using (table.baseAtoms as any)[key]. Ensure you reference the same resolution logic used by constructTable so external atoms observe the reset.packages/svelte-table/src/createTable.svelte.ts (1)
72-80:⚠️ Potential issue | 🟠 MajorAdd
columnsto the reactive dependencies tracked by this effect.This effect only tracks changes to
stateanddata. In Svelte 5, an effect only reruns when properties that are synchronously read inside its function body change. Ifcolumnsis reactive (passed as a getter or signal), updating it will not rerun this effect, leaving the table with stale column definitions.Suggested fix
if (state) { for (const key in state) { void state[key] } } void mergedOptions.data + void mergedOptions.columns🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-table/src/createTable.svelte.ts` around lines 72 - 80, The effect currently only reads mergedOptions.state and mergedOptions.data so it won't retrigger when reactive columns change; update the $effect.pre callback to also synchronously read mergedOptions.columns (e.g., assign or void mergedOptions.columns or iterate over it if it's an object/array) so Svelte 5 will track columns as a dependency and rerun the effect when columns updates occur; locate the $effect.pre block in createTable.svelte.ts and add a synchronous read of mergedOptions.columns (use void mergedOptions.columns or a safe iteration if it may be undefined) alongside the existing reads of state and data.
🧹 Nitpick comments (6)
packages/vue-table/tests/unit/signals.test.ts (1)
1-22: ⚡ Quick winConsider adding a test that exercises the
subscribecallback path.The current test only verifies
get()on writable and readonly atoms. Thesubscribe/unsubscribecontract — including thatwatch({ flush: 'sync' })notifications fire synchronously and thatunsubscribe()actually stops further callbacks — is untested. Given that the subscription mechanism is the main reactivity bridge between Vue's scheduler andtable-core, a subscription-side test would meaningfully increase confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-table/tests/unit/signals.test.ts` around lines 1 - 22, Add a unit test that exercises the subscription path of the vueReactivity bridge: instantiate vueReactivity(), create a writable atom via createWritableAtom(...) and a readonly atom via createReadonlyAtom(...), then call subscribe(callback) on the readonly atom (or writable) and assert the callback is called synchronously when you update the writable atom (use Vue's watch with { flush: 'sync' } behavior expectation) and that unsubscribe() returned by subscribe stops further callbacks; reference createWritableAtom, createReadonlyAtom, subscribe/unsubscribe and ensure you await/drive a nextTick only where appropriate to distinguish sync vs async notifications.packages/angular-table/src/signals.ts (1)
11-42: ⚡ Quick winRedundant
computed(signal)wrapper insubscribe— usetoObservable(signal, { injector })directly.Both
signalToReadonlyAtomandsignalToWritableAtomwrap the already-reactivesignalin an extracomputed(signal)before callingtoObservable. Sincesignalis already aSignal<T>(andWritableSignal<T>extends it),toObservablecan accept it directly. The intermediate computed is created fresh on everysubscribe()call, allocating an unnecessary reactive node each time.♻️ Proposed cleanup
function signalToReadonlyAtom<T>( signal: Signal<T>, injector: Injector, ): ReadonlyAtom<T> { return Object.assign(signal, { get: () => signal(), subscribe: (observer: Observer<T>) => { - return toObservable(computed(signal), { injector: injector }).subscribe( - observer, - ) + return toObservable(signal, { injector }).subscribe(observer) }, }) } function signalToWritableAtom<T>( signal: WritableSignal<T>, injector: Injector, ): Atom<T> { return Object.assign(signal.asReadonly(), { set: ..., get: () => signal(), subscribe: (observer: Observer<T>) => { - return toObservable(computed(signal), { injector: injector }).subscribe( - observer, - ) + return toObservable(signal, { injector }).subscribe(observer) }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-table/src/signals.ts` around lines 11 - 42, The subscribe implementations in signalToReadonlyAtom and signalToWritableAtom create an extra computed(signal) reactive node per subscription; change both subscribe callbacks to call toObservable(signal, { injector }) directly (i.e., replace toObservable(computed(signal), { injector }) with toObservable(signal, { injector })) so the existing Signal/WritableSignal is used without allocating a new computed each subscribe.packages/preact-table/tests/unit/signals.test.ts (1)
1-20: ⚡ Quick winMissing
subscribetest — would have caught the infinite-recursion bug insignals.ts.The suite only exercises
.get()and.set(fn). Adding asubscribetest would directly expose the stack-overflow introduced byObject.assignmutatingsource.subscribebefore the wrapper closes over it.💡 Suggested additional test
+ test('subscribe emits current and future values', () => { + const reactivity = preactReactivity() + const count = reactivity.createWritableAtom(0) + const received: number[] = [] + const sub = count.subscribe((v) => received.push(v)) + + count.set(1) + count.set(2) + + sub.unsubscribe() + count.set(3) + + expect(received).toContain(1) + expect(received).toContain(2) + expect(received).not.toContain(3) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-table/tests/unit/signals.test.ts` around lines 1 - 20, Add a unit test that calls the atom's subscribe path to reproduce the infinite-recursion bug: in tests for preactReactivity(), create a writable atom via createWritableAtom and subscribe to it (and unsubscribe) asserting the callback is invoked on set; this will expose the stack overflow caused by mutating source.subscribe. Then fix signals.ts by stopping the in-place mutation via Object.assign of the original source.subscribe; instead return or attach a new wrapper subscribe function (do not overwrite source.subscribe) so the wrapper closes over the original subscribe reference without reassigning it (look for the Object.assign usage and the subscribe wrapper in the createWritableAtom/createReadonlyAtom implementation).packages/solid-table/src/signals.ts (1)
22-24: ⚡ Quick winNon-null assertion on
runWithOwnerresult hides a potentialundefined.
runWithOwner<T>(owner, fn)can returnT | undefined(e.g. iffnthrows). The!silently converts a potentialundefinedto a runtime crash at.subscribe(observer). Whileowneris guaranteed non-null at the call site, a throw insideobservable(signal)would still surface as an unhandled error rather than a clean message.💡 Safer alternative
- return runWithOwner(owner, () => observable(signal))!.subscribe(observer) + const obs = runWithOwner(owner, () => observable(signal)) + if (!obs) throw new Error('[solid-table] runWithOwner returned undefined — owner may be disposed') + return obs.subscribe(observer)Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-table/src/signals.ts` around lines 22 - 24, Replace the non-null assertion on runWithOwner(...)!.subscribe with a safe pattern: call const obs = runWithOwner(owner, () => observable(signal)) inside a try/catch, if obs is undefined throw or rethrow a clear Error like "runWithOwner returned undefined while creating observable" (or rethrow the caught error with added context), then call obs.subscribe(observer); do the same for the other occurrences (the subscribe implementation and the spots at lines ~40-42) so we never call .subscribe on a possibly undefined value.packages/table-core/tests/unit/core/table/constructTable.test.ts (1)
7-14: ⚡ Quick winAdd a regression test for atom-backed state, not just construction.
This still only proves that
constructTable(...)accepts a reactivity implementation. The new behavior is the externalatoms/statesourcing path, so a focused test that binds one slice through an external atom and assertstable.atoms[key]plustable.reset()behavior would catch regressions in this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/tests/unit/core/table/constructTable.test.ts` around lines 7 - 14, The test only verifies constructTable accepts a reactivity implementation; add a unit test that exercises the external atom-backed state path by creating an external atom or state slice, passing it into constructTable (using the same reactivity e.g. tanstackSignals) so the table uses that external source, then assert that table.atoms[<key>] (or table.state[<key>]) references the provided atom and that invoking table.reset() restores the atom-backed slice to its initial value; target helpers and symbols: constructTable, table.atoms (or table.state), and table.reset to locate the relevant API in the test file and assert both reference equality and reset behavior to catch regressions.packages/lit-table/src/TableController.ts (1)
136-136: 💤 Low valueUnused
_notifierfield.The
_notifierfield is incremented in the subscription callbacks but never read. If it's not needed for debugging or future use, consider removing it to reduce noise.♻️ Proposed removal
private _table: Table<TFeatures, TData> | null = null private _storeSubscription?: { unsubscribe: () => void } private _optionsSubscription?: { unsubscribe: () => void } - private _notifier = 0if (this._table && !this._storeSubscription) { this._storeSubscription = this._table.store.subscribe(() => { - this._notifier++ this.host.requestUpdate() }) this._optionsSubscription = this._table.optionsStore.subscribe(() => { - this._notifier++ this.host.requestUpdate() }) }Also applies to: 213-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-table/src/TableController.ts` at line 136, Remove the unused private field _notifier from the TableController class and delete all increments to it in the subscription callbacks (where ++this._notifier is used); if the increments were intended as a change-trigger, either replace them with a proper state/readable signal or simply remove both the field declaration and those increment statements to eliminate dead code and noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/angular-table/tests/angularReactivityFeature.test.ts`:
- Around line 40-41: The test "methods within effect will be re-trigger when
options/state changes" is flaky due to a race between state changes and
assertion timing; make it deterministic by synchronizing on the effect
completion (e.g., await the next microtask/tick or use a test utility like
waitFor/waitForNextUpdate) and stabilize call counting by resetting mocks before
the test and asserting after you run pending timers (jest.runAllTimers or await
act/nextTick) so the method-call count is consistently observed; update the test
to explicitly wait for the re-triggered effect and then assert the exact number
of calls rather than relying on timing-sensitive immediate assertions.
In `@packages/angular-table/tests/injectTable.test.ts`:
- Around line 127-128: The second duplicate assertion should check the second
invocation of the mock instead of repeating the first; update the duplicate
expect to reference coreRowModelFn.mock.calls[1] (the second call) and assert
its [0].rows.length equals 10 so the test verifies both calls to coreRowModelFn.
In `@packages/preact-table/src/signals.ts`:
- Around line 16-46: The subscribe wrapper currently shadows and replaces the
original Preact subscribe method causing infinite recursion; in both
signalToReadonlyAtom and signalToWritableAtom capture the original subscribe
into a local (e.g. const nativeSubscribe = source.subscribe) before calling
Object.assign, then implement the wrapper to call
nativeSubscribe(observerToCallback(...)) instead of source.subscribe(...); keep
the rest of the wrapper behavior (returning { unsubscribe }) and ensure get/set
remain unchanged.
In `@packages/react-table/src/useTable.ts`:
- Around line 142-147: The current useEffect block that calls
table.setOptions(({...prev, ...tableOptions})) causes post-commit timing and
one-render staleness for computed methods like getRowModel() and
getHeaderGroups(); change this to useLayoutEffect so options are synchronized
pre-paint (before the browser paints) and visible within the same render
pass—replace the useEffect that references table.setOptions, table, and
tableOptions with useLayoutEffect and keep the same merge logic to avoid
render-phase mutations.
In `@packages/solid-table/src/createTable.ts`:
- Around line 77-79: The merge order currently forces the default reactivity to
override caller-provided values; change the merge so the default comes first and
caller options last (swap the args to mergeProps) so caller-provided reactivity
wins: update the mergedOptions creation that calls mergeProps(tableOptions, {
reactivity: solidReactivity(owner) }) to call mergeProps({ reactivity:
solidReactivity(owner) }, tableOptions) instead, keeping the same symbols
mergedOptions, mergeProps, tableOptions, solidReactivity(owner), and the
reactivity property.
In `@packages/table-core/src/core/table/constructTable.ts`:
- Around line 21-32: The public API currently requires reactivity by changing
the ConstructTable type and constructTable parameter to include a mandatory
reactivity, which is a breaking change; revert this by making reactivity
optional on the public ConstructTable/constructTable signature (e.g., keep
TableOptions<TFeatures,TData> & { reactivity?: TableReactivityBindings }) and
inside constructTable (the function using tableOptions.reactivity) default to
the existing internal bindings when reactivity is undefined (create or assign
the default TableReactivityBindings before using signals). Ensure you reference
and update ConstructTable, constructTable, TableOptions, and
TableReactivityBindings so consumers without explicit reactivity continue to get
the previous behavior.
In `@packages/table-core/src/core/table/coreTablesFeature.types.ts`:
- Around line 112-115: Fix the JSDoc typo for the Table custom reactibity
bindings by changing "reactibity" to "reactivity" above the readonly
reactivity?: TableReactivityBindings property; update the comment text so it
reads "Table custom reactivity bindings." to match the property name and type
(TableReactivityBindings) and ensure consistency across any nearby docs/comments
referencing this symbol.
In `@packages/table-core/src/features/table-reactivity/table-reactivity.ts`:
- Around line 61-74: The function atomToStore mutates the incoming Atom (using
Object.defineProperty on atom and assigning store.setState), which can throw on
subsequent calls and corrupt the original Atom; change it to return a thin
wrapper object that inherits from the atom (e.g., wrapper = Object.create(atom))
so the original atom is not mutated, define the 'state' accessor on the wrapper
with configurable: true, and assign wrapper.setState (not atom or store
referencing the same object) to atom.set.bind(atom) when a setter exists; keep
all other atom methods available via the prototype chain so atomToStore remains
a pure adapter.
In `@packages/vue-table/src/signals.ts`:
- Around line 60-61: The current untrack implementation (untrack: (fn) => fn())
does not suppress Vue reactive tracking; replace it with a proper pause/reset
pair from Vue internals by importing pauseTracking and resetTracking from
'@vue/reactivity' and implement untrack to call pauseTracking(), invoke fn() in
a try block, and call resetTracking() in finally so tracking is always restored;
keep batch as-is or document its behavior if you choose not to change it.
---
Outside diff comments:
In `@packages/svelte-table/src/createTable.svelte.ts`:
- Around line 72-80: The effect currently only reads mergedOptions.state and
mergedOptions.data so it won't retrigger when reactive columns change; update
the $effect.pre callback to also synchronously read mergedOptions.columns (e.g.,
assign or void mergedOptions.columns or iterate over it if it's an object/array)
so Svelte 5 will track columns as a dependency and rerun the effect when columns
updates occur; locate the $effect.pre block in createTable.svelte.ts and add a
synchronous read of mergedOptions.columns (use void mergedOptions.columns or a
safe iteration if it may be undefined) alongside the existing reads of state and
data.
In `@packages/table-core/src/core/table/coreTablesFeature.utils.ts`:
- Around line 11-16: table_reset() currently writes only to baseAtoms so slices
bound to external atoms via options.atoms are not reset; update the reset logic
to resolve the actual atom for each key (the same resolution constructTable
uses: options.atoms?.[key] fallback to baseAtoms[key]) and write the snap value
into that resolved atom. Concretely, in the reset implementation that uses
cloneState(table.initialState) and table.reactivity.batch(), iterate keys and
call set on the resolved atom map (e.g., the per-slice atoms object created
during constructTable or a helper that resolves options.atoms?.[key] ||
baseAtoms[key]) instead of always using (table.baseAtoms as any)[key]. Ensure
you reference the same resolution logic used by constructTable so external atoms
observe the reset.
---
Nitpick comments:
In `@packages/angular-table/src/signals.ts`:
- Around line 11-42: The subscribe implementations in signalToReadonlyAtom and
signalToWritableAtom create an extra computed(signal) reactive node per
subscription; change both subscribe callbacks to call toObservable(signal, {
injector }) directly (i.e., replace toObservable(computed(signal), { injector })
with toObservable(signal, { injector })) so the existing Signal/WritableSignal
is used without allocating a new computed each subscribe.
In `@packages/lit-table/src/TableController.ts`:
- Line 136: Remove the unused private field _notifier from the TableController
class and delete all increments to it in the subscription callbacks (where
++this._notifier is used); if the increments were intended as a change-trigger,
either replace them with a proper state/readable signal or simply remove both
the field declaration and those increment statements to eliminate dead code and
noise.
In `@packages/preact-table/tests/unit/signals.test.ts`:
- Around line 1-20: Add a unit test that calls the atom's subscribe path to
reproduce the infinite-recursion bug: in tests for preactReactivity(), create a
writable atom via createWritableAtom and subscribe to it (and unsubscribe)
asserting the callback is invoked on set; this will expose the stack overflow
caused by mutating source.subscribe. Then fix signals.ts by stopping the
in-place mutation via Object.assign of the original source.subscribe; instead
return or attach a new wrapper subscribe function (do not overwrite
source.subscribe) so the wrapper closes over the original subscribe reference
without reassigning it (look for the Object.assign usage and the subscribe
wrapper in the createWritableAtom/createReadonlyAtom implementation).
In `@packages/solid-table/src/signals.ts`:
- Around line 22-24: Replace the non-null assertion on
runWithOwner(...)!.subscribe with a safe pattern: call const obs =
runWithOwner(owner, () => observable(signal)) inside a try/catch, if obs is
undefined throw or rethrow a clear Error like "runWithOwner returned undefined
while creating observable" (or rethrow the caught error with added context),
then call obs.subscribe(observer); do the same for the other occurrences (the
subscribe implementation and the spots at lines ~40-42) so we never call
.subscribe on a possibly undefined value.
In `@packages/table-core/tests/unit/core/table/constructTable.test.ts`:
- Around line 7-14: The test only verifies constructTable accepts a reactivity
implementation; add a unit test that exercises the external atom-backed state
path by creating an external atom or state slice, passing it into constructTable
(using the same reactivity e.g. tanstackSignals) so the table uses that external
source, then assert that table.atoms[<key>] (or table.state[<key>]) references
the provided atom and that invoking table.reset() restores the atom-backed slice
to its initial value; target helpers and symbols: constructTable, table.atoms
(or table.state), and table.reset to locate the relevant API in the test file
and assert both reference equality and reset behavior to catch regressions.
In `@packages/vue-table/tests/unit/signals.test.ts`:
- Around line 1-22: Add a unit test that exercises the subscription path of the
vueReactivity bridge: instantiate vueReactivity(), create a writable atom via
createWritableAtom(...) and a readonly atom via createReadonlyAtom(...), then
call subscribe(callback) on the readonly atom (or writable) and assert the
callback is called synchronously when you update the writable atom (use Vue's
watch with { flush: 'sync' } behavior expectation) and that unsubscribe()
returned by subscribe stops further callbacks; reference createWritableAtom,
createReadonlyAtom, subscribe/unsubscribe and ensure you await/drive a nextTick
only where appropriate to distinguish sync vs async notifications.
🪄 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: 4fb02d43-1779-4f51-8252-70d91fffdc82
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
examples/react/mantine-react-table/package.jsonexamples/react/with-tanstack-form/package.jsonexamples/react/with-tanstack-router/package.jsonexamples/solid/with-tanstack-form/package.jsonexamples/solid/with-tanstack-router/package.jsonexamples/svelte/with-tanstack-form/package.jsonexamples/vue/with-tanstack-form/package.jsonpackages/angular-table/package.jsonpackages/angular-table/src/injectTable.tspackages/angular-table/src/lazySignalInitializer.tspackages/angular-table/src/signals.tspackages/angular-table/tests/angularReactivityFeature.test.tspackages/angular-table/tests/benchmarks/injectTable.benchmark.tspackages/angular-table/tests/benchmarks/setup.tspackages/angular-table/tests/flex-render/flex-render-table.test.tspackages/angular-table/tests/injectTable.test.tspackages/angular-table/vite.config.tspackages/lit-table/src/TableController.tspackages/lit-table/tests/unit/defaultReactivity.test.tspackages/preact-table/package.jsonpackages/preact-table/src/signals.tspackages/preact-table/src/useTable.tspackages/preact-table/tests/unit/signals.test.tspackages/react-table-devtools/package.jsonpackages/react-table/package.jsonpackages/react-table/src/useTable.tspackages/solid-table/src/createTable.tspackages/solid-table/src/signals.tspackages/svelte-table/src/createTable.svelte.tspackages/svelte-table/src/signals.svelte.tspackages/table-core/package.jsonpackages/table-core/src/core/table/constructTable.tspackages/table-core/src/core/table/coreTablesFeature.types.tspackages/table-core/src/core/table/coreTablesFeature.utils.tspackages/table-core/src/features/table-reactivity/table-reactivity.tspackages/table-core/src/features/table-reactivity/tableReactivityFeature.tspackages/table-core/src/features/table-reactivity/tanstack-signals.tspackages/table-core/src/index.tspackages/table-core/src/types/Table.tspackages/table-core/tests/helpers/generateTestTable.tspackages/table-core/tests/helpers/rowPinningHelpers.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/columns/constructColumn.test.tspackages/table-core/tests/unit/core/table/constructTable.test.tspackages/table-core/tests/unit/core/table/stockFeaturesInitialState.test.tspackages/table-core/tests/unit/core/tableAtoms.test.tspackages/table-core/tsdown.config.tspackages/vue-table/src/signals.tspackages/vue-table/src/useTable.tspackages/vue-table/tests/unit/signals.test.ts
💤 Files with no reviewable changes (5)
- packages/angular-table/tests/benchmarks/injectTable.benchmark.ts
- packages/angular-table/package.json
- packages/angular-table/tests/benchmarks/setup.ts
- packages/angular-table/tests/flex-render/flex-render-table.test.ts
- packages/table-core/src/features/table-reactivity/tableReactivityFeature.ts
| // TODO this switches between 1 and 2 calls on every other run, so it's not a reliable test | ||
| test.skip('methods within effect will be re-trigger when options/state changes', () => { | ||
| test('methods within effect will be re-trigger when options/state changes', () => { |
There was a problem hiding this comment.
Address test flakiness before unskipping.
The TODO comment indicates this test "switches between 1 and 2 calls on every other run." Re-enabling a known flaky test may cause intermittent CI failures. Consider either resolving the underlying timing issue or adding a more robust synchronization mechanism before unskipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-table/tests/angularReactivityFeature.test.ts` around lines
40 - 41, The test "methods within effect will be re-trigger when options/state
changes" is flaky due to a race between state changes and assertion timing; make
it deterministic by synchronizing on the effect completion (e.g., await the next
microtask/tick or use a test utility like waitFor/waitForNextUpdate) and
stabilize call counting by resetting mocks before the test and asserting after
you run pending timers (jest.runAllTimers or await act/nextTick) so the
method-call count is consistently observed; update the test to explicitly wait
for the re-triggered effect and then assert the exact number of calls rather
than relying on timing-sensitive immediate assertions.
| expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10) | ||
| expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10) |
There was a problem hiding this comment.
Duplicate assertion — likely copy-paste error.
Line 128 repeats the same assertion as line 127. Given the test expects 2 calls to coreRowModelFn, the second assertion should probably verify the second call's row count.
🐛 Proposed fix
expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10)
- expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10)
+ expect(coreRowModelFn.mock.calls[1]![0].rows.length).toEqual(10)📝 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.
| expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10) | |
| expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10) | |
| expect(coreRowModelFn.mock.calls[0]![0].rows.length).toEqual(10) | |
| expect(coreRowModelFn.mock.calls[1]![0].rows.length).toEqual(10) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-table/tests/injectTable.test.ts` around lines 127 - 128, The
second duplicate assertion should check the second invocation of the mock
instead of repeating the first; update the duplicate expect to reference
coreRowModelFn.mock.calls[1] (the second call) and assert its [0].rows.length
equals 10 so the test verifies both calls to coreRowModelFn.
| function signalToReadonlyAtom<T>(source: { | ||
| value: T | ||
| subscribe: (observer: (value: T) => void) => () => void | ||
| }): ReadonlyAtom<T> { | ||
| return Object.assign(source, { | ||
| get: () => source.value, | ||
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | ||
| const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) | ||
| return { unsubscribe } | ||
| }) as ReadonlyAtom<T>['subscribe'], | ||
| }) | ||
| } | ||
|
|
||
| function signalToWritableAtom<T>(source: { | ||
| value: T | ||
| subscribe: (observer: (value: T) => void) => () => void | ||
| }): Atom<T> { | ||
| return Object.assign(source, { | ||
| set: (updater: T | ((prevVal: T) => T)) => { | ||
| source.value = | ||
| typeof updater === 'function' | ||
| ? (updater as (prevVal: T) => T)(source.value) | ||
| : updater | ||
| }, | ||
| get: () => source.value, | ||
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | ||
| const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) | ||
| return { unsubscribe } | ||
| }) as Atom<T>['subscribe'], | ||
| }) | ||
| } |
There was a problem hiding this comment.
Critical: subscribe wrapper infinitely recurses — stack overflow on first subscription.
Object.assign(source, { subscribe: wrapper }) replaces source.subscribe (the native Preact Signal.prototype.subscribe, accessed as a closure-captured property lookup) with wrapper before any caller can invoke it. When wrapper runs, the expression source.subscribe(...) resolves to wrapper itself (own-property shadows the prototype), not the original Preact method → infinite recursion → stack overflow.
The test only exercises .get() / .set(), so this is not caught today. Any framework subscriber (or internal table wiring) that calls .subscribe() on one of these atoms will crash immediately.
The fix is to capture the original native subscribe before the assignment in both signalToReadonlyAtom and signalToWritableAtom:
🐛 Proposed fix
function signalToReadonlyAtom<T>(source: {
value: T
subscribe: (observer: (value: T) => void) => () => void
}): ReadonlyAtom<T> {
+ const _nativeSubscribe = source.subscribe.bind(source)
return Object.assign(source, {
get: () => source.value,
subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => {
- const unsubscribe = source.subscribe(observerToCallback(observerOrNext))
+ const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext))
return { unsubscribe }
}) as ReadonlyAtom<T>['subscribe'],
})
}
function signalToWritableAtom<T>(source: {
value: T
subscribe: (observer: (value: T) => void) => () => void
}): Atom<T> {
+ const _nativeSubscribe = source.subscribe.bind(source)
return Object.assign(source, {
set: (updater: T | ((prevVal: T) => T)) => {
source.value =
typeof updater === 'function'
? (updater as (prevVal: T) => T)(source.value)
: updater
},
get: () => source.value,
subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => {
- const unsubscribe = source.subscribe(observerToCallback(observerOrNext))
+ const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext))
return { unsubscribe }
}) as Atom<T>['subscribe'],
})
}📝 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.
| function signalToReadonlyAtom<T>(source: { | |
| value: T | |
| subscribe: (observer: (value: T) => void) => () => void | |
| }): ReadonlyAtom<T> { | |
| return Object.assign(source, { | |
| get: () => source.value, | |
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | |
| const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) | |
| return { unsubscribe } | |
| }) as ReadonlyAtom<T>['subscribe'], | |
| }) | |
| } | |
| function signalToWritableAtom<T>(source: { | |
| value: T | |
| subscribe: (observer: (value: T) => void) => () => void | |
| }): Atom<T> { | |
| return Object.assign(source, { | |
| set: (updater: T | ((prevVal: T) => T)) => { | |
| source.value = | |
| typeof updater === 'function' | |
| ? (updater as (prevVal: T) => T)(source.value) | |
| : updater | |
| }, | |
| get: () => source.value, | |
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | |
| const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) | |
| return { unsubscribe } | |
| }) as Atom<T>['subscribe'], | |
| }) | |
| } | |
| function signalToReadonlyAtom<T>(source: { | |
| value: T | |
| subscribe: (observer: (value: T) => void) => () => void | |
| }): ReadonlyAtom<T> { | |
| const _nativeSubscribe = source.subscribe.bind(source) | |
| return Object.assign(source, { | |
| get: () => source.value, | |
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | |
| const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext)) | |
| return { unsubscribe } | |
| }) as ReadonlyAtom<T>['subscribe'], | |
| }) | |
| } | |
| function signalToWritableAtom<T>(source: { | |
| value: T | |
| subscribe: (observer: (value: T) => void) => () => void | |
| }): Atom<T> { | |
| const _nativeSubscribe = source.subscribe.bind(source) | |
| return Object.assign(source, { | |
| set: (updater: T | ((prevVal: T) => T)) => { | |
| source.value = | |
| typeof updater === 'function' | |
| ? (updater as (prevVal: T) => T)(source.value) | |
| : updater | |
| }, | |
| get: () => source.value, | |
| subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { | |
| const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext)) | |
| return { unsubscribe } | |
| }) as Atom<T>['subscribe'], | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/preact-table/src/signals.ts` around lines 16 - 46, The subscribe
wrapper currently shadows and replaces the original Preact subscribe method
causing infinite recursion; in both signalToReadonlyAtom and
signalToWritableAtom capture the original subscribe into a local (e.g. const
nativeSubscribe = source.subscribe) before calling Object.assign, then implement
the wrapper to call nativeSubscribe(observerToCallback(...)) instead of
source.subscribe(...); keep the rest of the wrapper behavior (returning {
unsubscribe }) and ensure get/set remain unchanged.
| useEffect(() => { | ||
| table.setOptions((prev) => ({ | ||
| ...prev, | ||
| ...tableOptions, | ||
| })) | ||
| }, [table, tableOptions]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In React, are updates made inside useEffect visible during the same render, or only after the commit phase? What does that imply for derived values that are read during render?
💡 Result:
In React, updates made inside useEffect are not visible during the same render. They only take effect after the commit phase, triggering a new render. useEffect runs after the render and commit phases complete—specifically, after the browser has painted the screen (for useEffect; useLayoutEffect runs before paint but still after render). Any state updates via setState inside useEffect schedule a re-render for the next cycle. The current render uses the state values from before the effect ran. This implies that derived values read during render cannot depend on state updated in useEffect from the same render cycle. They will see stale values. To ensure derived values reflect effect-updated state, compute them in a subsequent render (e.g., via another useEffect or directly in the component body using current state/props). For values purely derived from props/state without side effects, compute them during render instead of using effects. Example: function MyComponent({ prop }) { const [state, setState] = useState(initial); const derived = computeFrom(prop, state); // Uses current render's state/prop useEffect( => { const newState = computeNewState(prop, state); setState(newState); // Queues re-render; derived above doesn't see this yet }, [prop]); return
Citations:
- 1: https://beta.reactjs.org/learn/synchronizing-with-effects
- 2: https://react.dev/learn/render-and-commit
- 3: https://react.dev/learn/synchronizing-with-effects
- 4: https://react.dev/reference/react/useEffect
- 5: https://reactjs.org/docs/hooks-effect.html
🏁 Script executed:
# First, let's understand the structure
cd packages/react-table && find . -name "*.ts" -o -name "*.tsx" | head -20Repository: TanStack/table
Length of output: 280
🏁 Script executed:
# Search for getRowModel and getHeaderGroups usage
rg "getRowModel|getHeaderGroups" --type ts --type tsx -B 2 -A 2 | head -60Repository: TanStack/table
Length of output: 86
🏁 Script executed:
# Check the useTable hook implementation to see the full context
cat -n packages/react-table/src/useTable.ts | head -160Repository: TanStack/table
Length of output: 6693
🏁 Script executed:
# Look for where table instance is created and returned
rg "createTable|new Table" packages/react-table/src/useTable.ts -B 3 -A 3Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Search for getRowModel and getHeaderGroups to understand when they're called
rg "getRowModel|getHeaderGroups" --type ts -B 2 -A 2 | head -100Repository: TanStack/table
Length of output: 8845
🏁 Script executed:
# Check the table-core implementation to see if these are computed on-demand
fd -e ts -e tsx | grep -i "table-core" | head -5Repository: TanStack/table
Length of output: 312
🏁 Script executed:
# Search for setOptions implementation and how it affects computed values
rg "setOptions" packages/react-table/src/ -B 2 -A 5Repository: TanStack/table
Length of output: 484
🏁 Script executed:
# Check git history for setOptions changes in useTable
git log --oneline -20 -- packages/react-table/src/useTable.tsRepository: TanStack/table
Length of output: 88
🏁 Script executed:
# Check if useLayoutEffect is imported or used elsewhere in react-table
rg "useLayoutEffect" packages/react-table/src/Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look at the full context of the Svelte implementation to understand the pattern
rg -B 5 -A 10 "effect.pre" packages/svelte-table/src/Repository: TanStack/table
Length of output: 1929
🏁 Script executed:
# Check if there are any tests that verify the option synchronization timing
rg "setOptions" packages/table-core -B 3 -A 5 --type ts | head -80Repository: TanStack/table
Length of output: 4442
Use pre-commit timing for option synchronization, not post-commit.
Moving table.setOptions(...) into useEffect delays option updates until after commit, causing one-render staleness. If consumers call getRowModel(), getHeaderGroups(), or other computed methods during the same render cycle as a prop change, they will observe stale options. The Svelte adapter in this repository already acknowledges this issue and uses pre-commit timing ($effect.pre) with explicit documentation that post-commit timing causes "one-frame-behind data." Consider useLayoutEffect instead, which runs after render but before paint, ensuring option updates are visible to downstream render passes while avoiding render-phase mutations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-table/src/useTable.ts` around lines 142 - 147, The current
useEffect block that calls table.setOptions(({...prev, ...tableOptions})) causes
post-commit timing and one-render staleness for computed methods like
getRowModel() and getHeaderGroups(); change this to useLayoutEffect so options
are synchronized pre-paint (before the browser paints) and visible within the
same render pass—replace the useEffect that references table.setOptions, table,
and tableOptions with useLayoutEffect and keep the same merge logic to avoid
render-phase mutations.
| const mergedOptions = mergeProps(tableOptions, { | ||
| _features: mergeProps(tableOptions._features, { | ||
| solidReactivityFeature, | ||
| }), | ||
| reactivity: solidReactivity(owner), | ||
| }) as any |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In SolidJS, when the same key exists in multiple mergeProps(...) sources, which source takes precedence? Please confirm whether later sources override earlier ones.
💡 Result:
In SolidJS, when the same key exists in multiple mergeProps(...) sources, the last (later) source takes precedence. It merges prop sources from left to right, and the last source with a non-undefined value for a property wins. Later sources override earlier ones.
Citations:
Let caller-provided reactivity win by swapping the merge order.
In SolidJS, mergeProps gives later sources precedence. The current order mergeProps(tableOptions, { reactivity: solidReactivity(owner) }) always overwrites any caller-provided tableOptions.reactivity with the default, which breaks the contract this PR establishes for allowing external/native reactivity bindings.
Suggested fix
- const mergedOptions = mergeProps(tableOptions, {
- reactivity: solidReactivity(owner),
- }) as any
+ const mergedOptions = mergeProps(
+ { reactivity: solidReactivity(owner) },
+ tableOptions,
+ ) as any📝 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.
| const mergedOptions = mergeProps(tableOptions, { | |
| _features: mergeProps(tableOptions._features, { | |
| solidReactivityFeature, | |
| }), | |
| reactivity: solidReactivity(owner), | |
| }) as any | |
| const mergedOptions = mergeProps( | |
| { reactivity: solidReactivity(owner) }, | |
| tableOptions, | |
| ) as any |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-table/src/createTable.ts` around lines 77 - 79, The merge
order currently forces the default reactivity to override caller-provided
values; change the merge so the default comes first and caller options last
(swap the args to mergeProps) so caller-provided reactivity wins: update the
mergedOptions creation that calls mergeProps(tableOptions, { reactivity:
solidReactivity(owner) }) to call mergeProps({ reactivity:
solidReactivity(owner) }, tableOptions) instead, keeping the same symbols
mergedOptions, mergeProps, tableOptions, solidReactivity(owner), and the
reactivity property.
| export type ConstructTable< | ||
| TFeatures extends TableFeatures, | ||
| TData extends RowData, | ||
| > = Omit<TableOptions<TFeatures, TData>, 'reactivity'> & { | ||
| reactivity: TableReactivityBindings | ||
| } | ||
|
|
||
| export function constructTable< | ||
| TFeatures extends TableFeatures, | ||
| TData extends RowData, | ||
| >(tableOptions: TableOptions<TFeatures, TData>): Table<TFeatures, TData> { | ||
| >(tableOptions: ConstructTable<TFeatures, TData>): Table<TFeatures, TData> { | ||
| const signals = tableOptions.reactivity |
There was a problem hiding this comment.
Keep reactivity backward-compatible on the public constructTable API.
Making reactivity mandatory turns this into a source-compatible break for every direct @tanstack/table-core consumer, even when they just want the old default behavior. Since the PR goal is to add external bindings support, this should either keep a default binding here or be called out explicitly as a breaking migration surface before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/core/table/constructTable.ts` around lines 21 - 32,
The public API currently requires reactivity by changing the ConstructTable type
and constructTable parameter to include a mandatory reactivity, which is a
breaking change; revert this by making reactivity optional on the public
ConstructTable/constructTable signature (e.g., keep
TableOptions<TFeatures,TData> & { reactivity?: TableReactivityBindings }) and
inside constructTable (the function using tableOptions.reactivity) default to
the existing internal bindings when reactivity is undefined (create or assign
the default TableReactivityBindings before using signals). Ensure you reference
and update ConstructTable, constructTable, TableOptions, and
TableReactivityBindings so consumers without explicit reactivity continue to get
the previous behavior.
| export function atomToStore<T>( | ||
| atom: Atom<T> | ReadonlyAtom<T>, | ||
| ): Store<T> | ReadonlyStore<T> { | ||
| const store: Store<T> = atom as Store<T> | ||
| Object.defineProperty(atom, 'state', { | ||
| get() { | ||
| return atom.get() | ||
| }, | ||
| }) | ||
| if ('set' in atom) { | ||
| store.setState = atom.set.bind(atom) | ||
| } | ||
| return store | ||
| } |
There was a problem hiding this comment.
atomToStore mutates its input and will throw on a second call with the same atom.
Two concrete problems:
-
Object.definePropertydefaults toconfigurable: false, so ifatomToStoreis called with the sameAtominstance a second time (e.g., becauseconstructTableis somehow invoked again with a memoized atom, or in a test harness), the runtime throwsTypeError: Cannot redefine property: state. -
const store: Store<T> = atom as Store<T>—storeandatomare the same object. Every mutation (Object.defineProperty,store.setState = ...) permanently alters the originalAtom, making the function an impure side-effecting operation rather than a pure bridge/adapter.
Replace the in-place mutation with a thin wrapper that inherits the atom's prototype chain:
🛡️ Proposed fix
export function atomToStore<T>(
atom: Atom<T> | ReadonlyAtom<T>,
): Store<T> | ReadonlyStore<T> {
- const store: Store<T> = atom as Store<T>
- Object.defineProperty(atom, 'state', {
+ const store = Object.create(atom) as Store<T>
+ Object.defineProperty(store, 'state', {
get() {
return atom.get()
},
+ configurable: true,
+ enumerable: true,
})
if ('set' in atom) {
- store.setState = atom.set.bind(atom)
+ store.setState = (atom as Atom<T>).set.bind(atom as Atom<T>)
}
return store
}Using Object.create(atom) keeps atom unmodified, all atom methods remain accessible through the prototype chain, and the state/setState additions live only on the wrapper object. Adding configurable: true also makes repeated wrapping safe.
📝 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.
| export function atomToStore<T>( | |
| atom: Atom<T> | ReadonlyAtom<T>, | |
| ): Store<T> | ReadonlyStore<T> { | |
| const store: Store<T> = atom as Store<T> | |
| Object.defineProperty(atom, 'state', { | |
| get() { | |
| return atom.get() | |
| }, | |
| }) | |
| if ('set' in atom) { | |
| store.setState = atom.set.bind(atom) | |
| } | |
| return store | |
| } | |
| export function atomToStore<T>( | |
| atom: Atom<T> | ReadonlyAtom<T>, | |
| ): Store<T> | ReadonlyStore<T> { | |
| const store = Object.create(atom) as Store<T> | |
| Object.defineProperty(store, 'state', { | |
| get() { | |
| return atom.get() | |
| }, | |
| configurable: true, | |
| enumerable: true, | |
| }) | |
| if ('set' in atom) { | |
| store.setState = (atom as Atom<T>).set.bind(atom as Atom<T>) | |
| } | |
| return store | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/features/table-reactivity/table-reactivity.ts` around
lines 61 - 74, The function atomToStore mutates the incoming Atom (using
Object.defineProperty on atom and assigning store.setState), which can throw on
subsequent calls and corrupt the original Atom; change it to return a thin
wrapper object that inherits from the atom (e.g., wrapper = Object.create(atom))
so the original atom is not mutated, define the 'state' accessor on the wrapper
with configurable: true, and assign wrapper.setState (not atom or store
referencing the same object) to atom.set.bind(atom) when a setter exists; keep
all other atom methods available via the prototype chain so atomToStore remains
a pure adapter.
| untrack: (fn) => fn(), | ||
| batch: (fn) => fn(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all call-sites of `untrack` inside table-core to understand
# whether it is called in reactive contexts (computed/watchEffect) that would
# cause wrong behavior with a no-op implementation.
rg -n --type=ts "untrack" packages/table-core/src/ -A 3 -B 3Repository: TanStack/table
Length of output: 1500
🏁 Script executed:
# First, let's see the actual implementation in packages/vue-table/src/signals.ts
cat -n packages/vue-table/src/signals.ts | head -70Repository: TanStack/table
Length of output: 2411
🏁 Script executed:
# Search for actual usages/calls of untrack in the codebase
rg "\.untrack\(" --type=ts -B 3 -A 3Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check if there are other implementations of untrack in different bindings
rg -n "untrack.*=|untrack:" --type=ts -B 2 -A 2Repository: TanStack/table
Length of output: 5527
🏁 Script executed:
# Look for any imports of pauseTracking or resetTracking to see if they're already being used
rg "pauseTracking|resetTracking" --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check package.json for `@vue/reactivity`
cat packages/vue-table/package.json | grep -A 5 -B 5 "vue\|reactivity"Repository: TanStack/table
Length of output: 1032
🏁 Script executed:
# Look for any existing imports of `@vue/reactivity` in the codebase
rg "@vue/reactivity" --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check the root package.json to see Vue version
cat package.json | jq '.devDependencies."vue" // .dependencies."vue" // empty'Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Search for how other Vue files handle reactive context management
rg "computed|watch|watchEffect" packages/vue-table/src/ --type=ts -lRepository: TanStack/table
Length of output: 127
🏁 Script executed:
# Check the Vue type definitions to see if untrack is exported as public API
cat packages/vue-table/node_modules/vue/dist/vue.d.ts 2>/dev/null | grep -i "untrack" || echo "Vue type definitions not available in sandbox"Repository: TanStack/table
Length of output: 104
🏁 Script executed:
# Check Vue 3.5+ release notes or source - search for untrack in any documentation files
find . -name "*.md" -type f -exec grep -l "untrack" {} \; 2>/dev/null | head -5Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Let's check what actually gets exported from 'vue' in the current import statement
rg "export.*untrack|export.*from.*vue" packages/vue-table/src/ --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check if there's any Vue documentation or examples using untrack
rg "import.*untrack.*from.*['\"]vue['\"]" --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look at the actual Vue table store to understand the contract better
cat packages/vue-table/src/useTable.ts | head -50Repository: TanStack/table
Length of output: 1521
🏁 Script executed:
# Let's see how untrack is actually used in table-core to understand the context better
rg -B 5 -A 5 "\.untrack\(" packages/table-core/src/ --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check if there's a watch or watchEffect that might call setOptions
rg "watch|watchEffect" packages/table-core/src/ --type=ts | head -20Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look at Vue 3 official documentation or check if there's any version info about untrack
web_search
Vue 3 untrack API reactive tracking suppressionRepository: TanStack/table
Length of output: 151
🌐 Web query:
Vue 3 untrack pauseTracking resetTracking internal API
💡 Result:
Vue 3's reactivity system includes internal APIs for controlling dependency tracking: pauseTracking, resetTracking, and untrack. These are not part of the public API documented in official Vue docs but exist in the @vue/reactivity source code and are used internally or by advanced users. pauseTracking and resetTracking temporarily disable and restore dependency tracking during effect execution. They manipulate a global tracking stack to prevent track calls from collecting dependencies. - pauseTracking: Pushes current shouldTrack state (false) onto trackStack. - resetTracking: Pops the stack to restore previous tracking state. Usage example from community code: function untrack(fn) { pauseTracking try { return fn } finally { resetTracking } } untrack is a newer utility (PR #13286, open as of 2025) that executes a callback without tracking dependencies, likely built on similar internals. These are internal and exporting them publicly was proposed (PR #13072) but closed without merge. They appear in source files like packages/reactivity/src/effect.ts and are used in watch.ts for cleanups. Official docs recommend customRef for controlling tracking in refs or explicit watch sources instead. Use with caution as they affect global state and can lead to inconsistent reactivity graphs if misused. As of 2026-05-02, no public stable API; check latest @vue/reactivity types/source for availability.
Citations:
- 1: https://github.com/vuejs/vue-next/blob/0764c33d3da8c06d472893a4e451e33394726a42/packages/reactivity/src/effect.ts
- 2: https://gist.github.com/johnsoncodehk/1ecf6907644bd06c0650a0a24ee868e3
- 3: Possibility to untrack ref in `watchEffect` vuejs/rfcs#750
- 4: feat(runtime-core): expose pauseTracking and resetTracking vuejs/core#13072
- 5: https://github.com/vuejs/core/blob/main/packages/reactivity/src/watch.ts
- 6: https://github.com/vuejs/core/blob/main/packages/reactivity/src/baseHandlers.ts
- 7: feat(reactivity): untrack vuejs/core#13286
🏁 Script executed:
# Check the exact usage context in table-core's constructTable to see when untrack is called
rg -n "reactivity\\.untrack" packages/table-core/src/ -B 10 -A 5Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look for setOptions usage pattern to understand the reactive context
rg -n "setOptions" packages/table-core/src/core/table.ts -B 5 -A 8 | head -50Repository: TanStack/table
Length of output: 136
🏁 Script executed:
# Find where setOptions is defined
fd "table.ts" packages/table-core/src/ --type fRepository: TanStack/table
Length of output: 150
🏁 Script executed:
# Look for the actual file structure in table-core
ls -la packages/table-core/src/core/ 2>/dev/null || find packages/table-core/src/ -name "*.ts" -type f | head -20Repository: TanStack/table
Length of output: 563
🏁 Script executed:
# Search for how reactivity.untrack is called in table-core
rg "reactivity" packages/table-core/src/ --type=ts -A 3 -B 3 | grep -A 5 -B 5 "untrack"Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look at constructTable.ts to find untrack usage
cat packages/table-core/src/core/table/constructTable.ts | head -100Repository: TanStack/table
Length of output: 3504
🏁 Script executed:
# Search more broadly for where reactivity object methods are called
rg "options\\.untrack|reactivity\.untrack|state\.untrack" --type=ts -B 3 -A 3Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check if there are any watch or computed calls in the core that might need untrack
rg "watch|computed|effect" packages/table-core/src/core/table/constructTable.tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Continue reading constructTable.ts to find untrack usage
cat packages/table-core/src/core/table/constructTable.ts | tail -100Repository: TanStack/table
Length of output: 2962
🏁 Script executed:
# Search for setOptions implementation which likely uses untrack
rg -n "setOptions" packages/table-core/src/core/table/constructTable.ts -B 5 -A 10Repository: TanStack/table
Length of output: 697
🏁 Script executed:
# Look for where untrack is actually invoked in any of the features
rg "untrack\(" packages/table-core/src/ --type=ts -B 5 -A 5Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Search in features directory for untrack usage
rg "untrack" packages/table-core/src/features/ --type=ts -B 3 -A 3Repository: TanStack/table
Length of output: 1464
🏁 Script executed:
# Check if untrack is used anywhere in table-core at all
rg "untrack" packages/table-core/ --type=tsRepository: TanStack/table
Length of output: 258
🏁 Script executed:
# Look at what features actually use reactivity
rg "reactivity\." packages/table-core/src/features/ --type=ts | head -20Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Double-check - search across ALL frameworks' binding files for actual untrack calls
rg "\.untrack\(" --type=tsRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check the interface definition and see if there are any comments about usage
cat packages/table-core/src/features/table-reactivity/table-reactivity.tsRepository: TanStack/table
Length of output: 1935
untrack is a no-op that does not actually suppress Vue reactive tracking.
Vue 3 has no public untrack API. Calling fn() directly does not pause Vue's dependency-tracking mechanism. While this is not currently an issue since untrack() is not invoked anywhere in the codebase, the implementation violates the interface contract: "Evaluates a function without tracking reactive dependencies." Other frameworks (Solid, Svelte, Angular, Preact) all provide proper implementations.
Choose a remedy:
Option A: Use `@vue/reactivity` internals (undocumented but functional)
import { pauseTracking, resetTracking } from '@vue/reactivity'
untrack: (fn) => {
pauseTracking()
try {
return fn()
} finally {
resetTracking()
}
}Option B: Document the limitation
Add a comment explaining that Vue's untrack cannot suppress dependencies and table-core should avoid relying on it inside reactive contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-table/src/signals.ts` around lines 60 - 61, The current untrack
implementation (untrack: (fn) => fn()) does not suppress Vue reactive tracking;
replace it with a proper pause/reset pair from Vue internals by importing
pauseTracking and resetTracking from '@vue/reactivity' and implement untrack to
call pauseTracking(), invoke fn() in a try block, and call resetTracking() in
finally so tracking is always restored; keep batch as-is or document its
behavior if you choose not to change it.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated five areas introduced by this PR to resolve failing type checks, runtime import errors, and the knip unresolved-import failure. The fixes correct a reactivity → _reactivity typo, remove erroneous readonly modifiers from lazily-assigned prototype caches, add the missing reactivity option to TableOptions_Table, redirect constructTable to read from the new option, rename all test references from the non-existent defaultReactivityBindings module to the actual constructReactivityBindings, and update the vue-table signals test to import from the renamed reactivity.ts file.
Warning
- ❌ We could not verify this fix.
- The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Or Apply changes locally with:
npx nx-cloud apply-locally XWKB-YFhH
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/angular-table/src/helpers/createTableHook.ts (1)
445-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUser-provided
coreReativityFeatureindefaultTableOptions._featuressilently overrides Angular's injected reactivity.
injectAppTablenow forwards{ ...defaultTableOptions._features, appTableFeatures }asoptions()._features. InsideinjectTable.ts(line 136–143), the merge order is:_features: { coreReativityFeature: angularReactivity(injector), // injected first ...options()._features, // user features spread after → wins }Because
CreateTableContextOptionsdoes not exclude_features, a consumer ofcreateTableHookcan pass_features: { coreReativityFeature: someCustomBinding }indefaultTableOptions. That will propagate intooptions()._featuresand silently replaceangularReactivity(injector), defeating the framework adapter entirely.The fix belongs in
packages/angular-table/src/injectTable.ts— Angular's binding should be placed last so it always takes precedence:_features: { - coreReativityFeature: angularReactivity(injector), ...options()._features, + coreReativityFeature: angularReactivity(injector), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-table/src/helpers/createTableHook.ts` around lines 445 - 455, The merge in injectTable (injectTable.ts) currently spreads options()._features after inserting the framework reactivity, which allows a user-provided _features.coreReativityFeature to override Angular's angularReactivity(injector); change the merge so Angular's binding wins by applying the user features first and then setting coreReativityFeature: angularReactivity(injector) (i.e., merge ...options()._features before, and explicitly assign coreReativityFeature = angularReactivity(injector) afterwards) so angularReactivity always takes precedence; reference injectTable, coreReativityFeature, angularReactivity(injector) and options()._features when making the change.packages/svelte-table/src/createTable.svelte.ts (1)
67-83:⚠️ Potential issue | 🟠 MajorThe
$effect.preonly tracksstateanddatadependencies, leaving other reactive options likecolumnsunsynced.If
columnsor other option properties are passed as reactive getters (e.g.,$stateor derived), changes to them won't retrigger the effect, sotable.setOptions(...)remains stale. Since the effect merges all ofmergedOptionsviasetOptions, dependency reads should cover all reactive properties, not juststateanddata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-table/src/createTable.svelte.ts` around lines 67 - 83, The effect only reads mergedOptions.state and mergedOptions.data so reactive getters like mergedOptions.columns won't become dependencies; fix by, inside the existing $effect.pre callback that currently reads state/data (before calling table.setOptions/untrack), iterate over all keys of mergedOptions and access each value (e.g., for (const k in mergedOptions) void mergedOptions[k]) so any reactive getters on mergedOptions (columns, other option props) are tracked and will retrigger the effect that calls table.setOptions; keep the existing untrack/setOptions placement unchanged.
♻️ Duplicate comments (2)
packages/preact-table/src/reactivity.ts (1)
17-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
subscribewrapper causes infinite recursion — stack overflow on first subscription.
Object.assign(source, { subscribe: wrapper })replacessource.subscribewithwrapperbefore it is invoked. Whenwrapperexecutes,source.subscribe(...)on lines 24 and 43 now resolves towrapperitself (own-property shadows the prototype), causing infinite recursion.This was flagged in a previous review and remains unfixed.
🐛 Proposed fix — capture native subscribe before assignment
function signalToReadonlyAtom<T>(source: { value: T subscribe: (observer: (value: T) => void) => () => void }): ReadonlyAtom<T> { + const _nativeSubscribe = source.subscribe.bind(source) return Object.assign(source, { get: () => source.value, subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { - const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) + const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext)) return { unsubscribe } }) as ReadonlyAtom<T>['subscribe'], }) } function signalToWritableAtom<T>(source: { value: T subscribe: (observer: (value: T) => void) => () => void }): Atom<T> { + const _nativeSubscribe = source.subscribe.bind(source) return Object.assign(source, { set: (updater: T | ((prevVal: T) => T)) => { source.value = typeof updater === 'function' ? (updater as (prevVal: T) => T)(source.value) : updater }, get: () => source.value, subscribe: ((observerOrNext: Observer<T> | ((value: T) => void)) => { - const unsubscribe = source.subscribe(observerToCallback(observerOrNext)) + const unsubscribe = _nativeSubscribe(observerToCallback(observerOrNext)) return { unsubscribe } }) as Atom<T>['subscribe'], }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-table/src/reactivity.ts` around lines 17 - 47, The subscribe wrapper in signalToReadonlyAtom and signalToWritableAtom overwrites source.subscribe before calling it, causing infinite recursion; fix by capturing the original subscribe function into a local variable (e.g., const nativeSubscribe = source.subscribe) before calling Object.assign, then use nativeSubscribe(observerToCallback(...)) inside the new subscribe wrapper; keep the rest of the wrapper behavior (observerToCallback conversion and returning { unsubscribe }) unchanged so semantics match the original intent.packages/vue-table/src/reactivity.ts (1)
60-61:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff
untrackis a no-op that does not suppress Vue reactive tracking.Vue 3 has no public
untrackAPI. The current implementation(fn) => fn()does not pause dependency tracking, violating theTableReactivityBindingscontract. While this is not immediately broken sinceuntrackisn't called in reactive contexts currently, it's a latent correctness issue.This was flagged in a previous review. Consider using
@vue/reactivityinternals or documenting the limitation.Option A: Use `@vue/reactivity` internals
import { pauseTracking, resetTracking } from '@vue/reactivity' untrack: (fn) => { pauseTracking() try { return fn() } finally { resetTracking() } }Option B: Document the limitation
- untrack: (fn) => fn(), + // Vue 3 has no public untrack API. This is a no-op; table-core should + // avoid calling untrack() inside Vue reactive contexts (computed/watch). + untrack: (fn) => fn(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-table/src/reactivity.ts` around lines 60 - 61, The current untrack implementation in TableReactivityBindings (untrack: (fn) => fn()) is a no-op and does not suppress Vue reactive tracking; replace it with a proper tracking-suppression wrapper by importing pauseTracking and resetTracking from `@vue/reactivity` and invoking pauseTracking before calling the passed fn and resetTracking in a finally block, or if you prefer not to use internal APIs, explicitly document the limitation in the TableReactivityBindings comment and keep untrack as a documented noop; ensure the change references the untrack symbol so behavior matches the contract and leave batch unchanged.
🧹 Nitpick comments (2)
packages/table-core/src/core/reactivity/coreReactivityFeature.utils.ts (1)
16-29:atomToStorepermanently mutates the input atom — use a wrapper object instead.
storeis just a re-cast alias ofatom(same reference). TheObject.definePropertycall and thestore.setState = ...assignment both modify the original atom directly, causing callers' atoms to acquire unwantedstateandsetStateside effects. A wrapper avoids polluting the original object.Additionally,
Atom<T>.sethas a broader signature thanStore<T>.setState: it accepts((prevVal: T) => T) | T, whilesetStateonly accepts(prev: T) => T. Theas Store<T>cast suppresses this TypeScript incompatibility. A wrapper sidesteps both issues.♻️ Proposed refactor (wrapper approach)
export function atomToStore<T>( atom: Atom<T> | ReadonlyAtom<T>, ): Store<T> | ReadonlyStore<T> { - const store: Store<T> = atom as Store<T> - Object.defineProperty(atom, 'state', { - get() { - return atom.get() - }, - }) - if ('set' in atom) { - store.setState = atom.set.bind(atom) - } - return store + const base: ReadonlyStore<T> = { + get state() { + return atom.get() + }, + } + if ('set' in atom) { + ;(base as Store<T>).setState = (atom as Atom<T>).set.bind(atom) + } + return base as 'set' extends keyof typeof atom ? Store<T> : ReadonlyStore<T> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/table-core/src/core/reactivity/coreReactivityFeature.utils.ts` around lines 16 - 29, The atomToStore function currently mutates the input atom by casting it to Store<T> and adding a state getter and setState, which pollutes the original Atom and hides a signature mismatch between Atom.set and Store.setState; instead create and return a new wrapper object that implements Store<T>/ReadonlyStore<T> without modifying the original atom: implement a wrapper with a state getter that calls atom.get(), a setState method that adapts Atom.set's broader signature to Store.setState by detecting functions vs values and delegating to atom.set appropriately, and forward other needed methods/properties to the original atom (referencing atomToStore, Atom.set and Store.setState to locate the code to change).packages/lit-table/src/TableController.ts (1)
98-104: 💤 Low valueOutdated documentation comment references old function name.
The JSDoc mentions
constructReactivityFeaturebut the implementation now usesconstructReactivityBindings.📝 Proposed fix
/** * A Lit ReactiveController for TanStack Table integration. * - * Uses `constructReactivityFeature` from table-core to properly integrate + * Uses `constructReactivityBindings` from table-core to properly integrate * with the TanStack Store reactivity system, matching the pattern used by * all other framework adapters (React, Vue, Solid, Svelte, Angular).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-table/src/TableController.ts` around lines 98 - 104, The JSDoc in TableController.ts is out of date: it references constructReactivityFeature while the code now uses constructReactivityBindings; update the documentation comment to mention constructReactivityBindings (and any relevant behavior name) so the doc matches the implementation in the TableController class and surrounding reactivity integration code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/angular-table/src/injectTable.ts`:
- Around line 59-65: The exported AngularTableComputed type is out of sync with
the new table.computed implementation (which now only accepts a selector +
optional equal), so update or remove AngularTableComputed to match the narrowed
API: remove the old "source" overloads and ensure the type signature exactly
matches computed: <TSubSelected = {}>(props: { selector: (state:
TableState<TFeatures>) => TSubSelected; equal?: ValueEqualityFn<TSubSelected> })
=> Signal<Readonly<TSubSelected>> (also apply the same narrowing for the
duplicate/related type declared near lines 163-169), referencing the
AngularTableComputed identifier and any duplicate interface to keep public types
consistent with table.computed.
- Around line 137-143: Cache the result of options() into a local variable, then
pass that cached options to constructTable while merging _features so that the
Angular reactivity binding overrides any caller-provided feature: take the
cachedOptions._features (if any) and spread them first, then set
coreReativityFeature: angularReactivity(injector) last so it wins; update the
constructTable call (and the cast to AngularTable<TFeatures, TData, TSelected>)
to use cachedOptions and the reordered _features merge to ensure
table.state/table.computed/table.value remain backed by Angular signals.
In `@packages/preact-table/src/reactivity.ts`:
- Line 1: Fix the typo in the top-of-file comment: change the comment text "TOTO
- re-explore preact signals for reactivity" to "TODO - re-explore preact signals
for reactivity" so the intent is clear and searchable; locate the string "TOTO -
re-explore preact signals for reactivity" in the file and update it to "TODO..."
accordingly.
In `@packages/preact-table/src/useTable.ts`:
- Around line 124-135: The hook useTable now returns the raw selector result in
state which can omit required fields (columns, data) from the declared
PreactTable['state'], causing runtime undefined for table.state.columns; fix by
ensuring the returned state always contains the required shape: when building
the return object in useTable (the selector/result used by useSelector and the
returned state), normalize or merge missing fields so state.columns and
state.data are present (e.g., fall back to table.state.columns/table.state.data
or default empty arrays/objects) or update the public type to reflect the new
optional fields — locate useTable, the selector passed into useSelector, and the
returned object spreading table to implement the normalization or update the
type accordingly.
In `@packages/react-table/src/useTable.ts`:
- Around line 152-165: The returned object from useTable (in useTable.ts where
useSelector and useMemo are used to return {...table, options, state}) can
expose a runtime-missing `columns` and `data` despite the public
ReactTable.state type; fix this by normalizing/ensuring the returned state
includes the expected keys (e.g., guarantee state.columns and state.data exist
by merging defaults or falling back to table.initialState or empty arrays)
before returning from useMemo, or alternatively update the public API types and
migration notes if you intend to remove those fields (adjust the return in
useMemo and the selector usage accordingly).
In `@packages/table-core/src/core/coreFeatures.ts`:
- Line 10: The interface marks coreReativityFeature as optional but
constructTable accesses it with non-null assertion, causing silent runtime
failures; fix by either (A) making coreReativityFeature required in the
interface and adding a default coreReativityFeature implementation to the
exported coreFeatures object so callers can safely spread `{ ...coreFeatures }`,
or (B) keep it optional and add a runtime guard in constructTable: read
tableOptions._features.coreReativityFeature into _reactivity and if falsy throw
a clear Error like '[table-core] coreReativityFeature must be provided in
_features' before any use; update references to the symbols
coreReativityFeature, coreFeatures, constructTable, _features, and _reactivity
accordingly.
- Line 10: The public API field is misspelled: rename the symbol
coreReativityFeature to coreReactivityFeature across the codebase (start with
the declaration in coreFeatures.ts), update all imports/uses in adapters, test
helpers, feature interfaces and any exported types or re-exports to the
corrected name, and update related docs/tests to reference coreReactivityFeature
so the change is consistent; ensure you also update any type/interface names,
exported indexes, and usages in runtime code to avoid leftover references to
coreReativityFeature.
In `@packages/table-core/src/core/reactivity/coreReactivityFeature.utils.ts`:
- Around line 20-24: The descriptor for atom.state is being defined without
configurable: true which breaks idempotency of atomToStore; update the code in
the atomToStore path that calls Object.defineProperty(atom, 'state', ...) to
either add configurable: true to the property descriptor (e.g.,
Object.defineProperty(atom, 'state', { get() { return atom.get() },
configurable: true })) or, alternatively, guard by checking
Object.getOwnPropertyDescriptor(atom, 'state') and skip redefining if it already
exists; reference the atom object and the 'state' property in your change so
HMR/tests that re-run atomToStore won’t throw.
In `@packages/table-core/src/core/table/coreTablesFeature.types.ts`:
- Around line 77-83: The runtime selection in constructTable currently prefers
options.state over options.atoms, which contradicts the documented precedence;
update the constructTable logic so for each slice it chooses options.atoms[key]
if present, otherwise options.state[key], and only then falls back to the
internal base atom (keep the existing behavior that library-originating writes
go through the internal base atom and consumers mirror changes via the onXChange
callbacks). Adjust any selection code paths in constructTable (and related
helpers) to reference options.atoms before options.state, and ensure the
ExternalAtoms type, constructTable, and onXChange interactions remain consistent
with this precedence.
---
Outside diff comments:
In `@packages/angular-table/src/helpers/createTableHook.ts`:
- Around line 445-455: The merge in injectTable (injectTable.ts) currently
spreads options()._features after inserting the framework reactivity, which
allows a user-provided _features.coreReativityFeature to override Angular's
angularReactivity(injector); change the merge so Angular's binding wins by
applying the user features first and then setting coreReativityFeature:
angularReactivity(injector) (i.e., merge ...options()._features before, and
explicitly assign coreReativityFeature = angularReactivity(injector) afterwards)
so angularReactivity always takes precedence; reference injectTable,
coreReativityFeature, angularReactivity(injector) and options()._features when
making the change.
In `@packages/svelte-table/src/createTable.svelte.ts`:
- Around line 67-83: The effect only reads mergedOptions.state and
mergedOptions.data so reactive getters like mergedOptions.columns won't become
dependencies; fix by, inside the existing $effect.pre callback that currently
reads state/data (before calling table.setOptions/untrack), iterate over all
keys of mergedOptions and access each value (e.g., for (const k in
mergedOptions) void mergedOptions[k]) so any reactive getters on mergedOptions
(columns, other option props) are tracked and will retrigger the effect that
calls table.setOptions; keep the existing untrack/setOptions placement
unchanged.
---
Duplicate comments:
In `@packages/preact-table/src/reactivity.ts`:
- Around line 17-47: The subscribe wrapper in signalToReadonlyAtom and
signalToWritableAtom overwrites source.subscribe before calling it, causing
infinite recursion; fix by capturing the original subscribe function into a
local variable (e.g., const nativeSubscribe = source.subscribe) before calling
Object.assign, then use nativeSubscribe(observerToCallback(...)) inside the new
subscribe wrapper; keep the rest of the wrapper behavior (observerToCallback
conversion and returning { unsubscribe }) unchanged so semantics match the
original intent.
In `@packages/vue-table/src/reactivity.ts`:
- Around line 60-61: The current untrack implementation in
TableReactivityBindings (untrack: (fn) => fn()) is a no-op and does not suppress
Vue reactive tracking; replace it with a proper tracking-suppression wrapper by
importing pauseTracking and resetTracking from `@vue/reactivity` and invoking
pauseTracking before calling the passed fn and resetTracking in a finally block,
or if you prefer not to use internal APIs, explicitly document the limitation in
the TableReactivityBindings comment and keep untrack as a documented noop;
ensure the change references the untrack symbol so behavior matches the contract
and leave batch unchanged.
---
Nitpick comments:
In `@packages/lit-table/src/TableController.ts`:
- Around line 98-104: The JSDoc in TableController.ts is out of date: it
references constructReactivityFeature while the code now uses
constructReactivityBindings; update the documentation comment to mention
constructReactivityBindings (and any relevant behavior name) so the doc matches
the implementation in the TableController class and surrounding reactivity
integration code.
In `@packages/table-core/src/core/reactivity/coreReactivityFeature.utils.ts`:
- Around line 16-29: The atomToStore function currently mutates the input atom
by casting it to Store<T> and adding a state getter and setState, which pollutes
the original Atom and hides a signature mismatch between Atom.set and
Store.setState; instead create and return a new wrapper object that implements
Store<T>/ReadonlyStore<T> without modifying the original atom: implement a
wrapper with a state getter that calls atom.get(), a setState method that adapts
Atom.set's broader signature to Store.setState by detecting functions vs values
and delegating to atom.set appropriately, and forward other needed
methods/properties to the original atom (referencing atomToStore, Atom.set and
Store.setState to locate the code to change).
🪄 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: 5f3858a4-eace-4cef-9c0a-af69dd03ef34
📒 Files selected for processing (72)
examples/preact/basic-external-atoms/index.htmlexamples/preact/basic-external-state/index.htmlexamples/preact/basic-use-app-table/index.htmlexamples/preact/basic-use-table/index.htmlexamples/preact/column-groups/index.htmlexamples/preact/column-ordering/index.htmlexamples/preact/column-pinning-split/index.htmlexamples/preact/column-pinning-sticky/index.htmlexamples/preact/column-pinning/index.htmlexamples/preact/column-resizing-performant/index.htmlexamples/preact/column-resizing/index.htmlexamples/preact/column-sizing/index.htmlexamples/preact/column-visibility/index.htmlexamples/preact/composable-tables/index.htmlexamples/preact/composable-tables/src/components/header-components.tsxexamples/preact/custom-plugin/index.htmlexamples/preact/custom-plugin/src/main.tsxexamples/preact/expanding/index.htmlexamples/preact/expanding/src/main.tsxexamples/preact/filters-faceted/index.htmlexamples/preact/filters-faceted/src/main.tsxexamples/preact/filters-fuzzy/index.htmlexamples/preact/filters-fuzzy/src/main.tsxexamples/preact/filters/index.htmlexamples/preact/filters/src/main.tsxexamples/preact/grouping/index.htmlexamples/preact/pagination/index.htmlexamples/preact/pagination/src/main.tsxexamples/preact/row-pinning/index.htmlexamples/preact/row-pinning/src/main.tsxexamples/preact/row-selection/index.htmlexamples/preact/row-selection/src/main.tsxexamples/preact/sorting/index.htmlexamples/preact/sub-components/index.htmlexamples/preact/with-tanstack-query/index.htmlpackages/angular-table/src/helpers/createTableHook.tspackages/angular-table/src/injectTable.tspackages/angular-table/src/reactivity.tspackages/lit-table/src/TableController.tspackages/lit-table/tests/unit/defaultReactivity.test.tspackages/preact-table/src/reactivity.tspackages/preact-table/src/useTable.tspackages/preact-table/tests/unit/signals.test.tspackages/react-table/src/useTable.tspackages/solid-table/src/createTable.tspackages/solid-table/src/reactivity.tspackages/svelte-table/src/createTable.svelte.tspackages/svelte-table/src/reactivity.svelte.tspackages/table-core/package.jsonpackages/table-core/src/core/coreFeatures.tspackages/table-core/src/core/reactivity/constructReactivityBindings.tspackages/table-core/src/core/reactivity/coreReactivityFeature.types.tspackages/table-core/src/core/reactivity/coreReactivityFeature.utils.tspackages/table-core/src/core/table/constructTable.tspackages/table-core/src/core/table/coreTablesFeature.types.tspackages/table-core/src/core/table/coreTablesFeature.utils.tspackages/table-core/src/index.tspackages/table-core/src/reactivity.tspackages/table-core/src/types/TableFeatures.tspackages/table-core/tests/helpers/generateTestTable.tspackages/table-core/tests/helpers/rowPinningHelpers.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/columns/constructColumn.test.tspackages/table-core/tests/unit/core/table/constructTable.test.tspackages/table-core/tests/unit/core/table/stockFeaturesInitialState.test.tspackages/table-core/tests/unit/core/tableAtoms.test.tspackages/table-core/tsdown.config.tspackages/vue-table/src/reactivity.tspackages/vue-table/src/useTable.tspackages/vue-table/tests/unit/signals.test.ts
💤 Files with no reviewable changes (27)
- examples/preact/basic-use-table/index.html
- examples/preact/column-resizing-performant/index.html
- examples/preact/filters-fuzzy/index.html
- examples/preact/column-ordering/index.html
- examples/preact/basic-external-atoms/index.html
- examples/preact/expanding/index.html
- examples/preact/sorting/index.html
- examples/preact/basic-use-app-table/index.html
- examples/preact/filters/index.html
- examples/preact/row-selection/index.html
- examples/preact/custom-plugin/index.html
- examples/preact/filters-faceted/index.html
- examples/preact/basic-external-state/index.html
- examples/preact/column-groups/index.html
- examples/preact/column-pinning-sticky/index.html
- examples/preact/with-tanstack-query/index.html
- examples/preact/column-sizing/index.html
- examples/preact/column-visibility/index.html
- examples/preact/composable-tables/index.html
- examples/preact/row-pinning/index.html
- examples/preact/column-pinning/index.html
- examples/preact/sub-components/index.html
- examples/preact/pagination/index.html
- examples/preact/column-pinning-split/index.html
- examples/preact/column-resizing/index.html
- examples/preact/grouping/index.html
- packages/table-core/src/index.ts
✅ Files skipped from review due to trivial changes (7)
- packages/preact-table/tests/unit/signals.test.ts
- packages/table-core/tsdown.config.ts
- packages/table-core/src/reactivity.ts
- packages/table-core/src/core/table/coreTablesFeature.utils.ts
- packages/table-core/src/types/TableFeatures.ts
- packages/vue-table/tests/unit/signals.test.ts
- packages/table-core/tests/unit/core/columns/constructColumn.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lit-table/tests/unit/defaultReactivity.test.ts
| * Creates a computed that subscribe to changes in the table store with a custom selector. | ||
| * Default equality function is "shallow". | ||
| */ | ||
| computed: AngularTableComputed<TFeatures> | ||
| Subscribe: AngularTableComputed<TFeatures> | ||
| computed: <TSubSelected = {}>(props: { | ||
| selector: (state: TableState<TFeatures>) => TSubSelected | ||
| equal?: ValueEqualityFn<TSubSelected> | ||
| }) => Signal<Readonly<TSubSelected>> |
There was a problem hiding this comment.
Keep the exported AngularTableComputed type in sync with the narrowed API.
Lines 59-65 make table.computed selector-only, but the exported AngularTableComputed interface above still exposes the removed source overloads. That leaves a public type compiling against an API this implementation no longer supports. Please narrow or remove AngularTableComputed in the same change.
Also applies to: 163-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-table/src/injectTable.ts` around lines 59 - 65, The exported
AngularTableComputed type is out of sync with the new table.computed
implementation (which now only accepts a selector + optional equal), so update
or remove AngularTableComputed to match the narrowed API: remove the old
"source" overloads and ensure the type signature exactly matches computed:
<TSubSelected = {}>(props: { selector: (state: TableState<TFeatures>) =>
TSubSelected; equal?: ValueEqualityFn<TSubSelected> }) =>
Signal<Readonly<TSubSelected>> (also apply the same narrowing for the
duplicate/related type declared near lines 163-169), referencing the
AngularTableComputed identifier and any duplicate interface to keep public types
consistent with table.computed.
| const table = constructTable({ | ||
| ...options(), | ||
| _features: { | ||
| coreReativityFeature: angularReactivity(injector), | ||
| ...options()._features, | ||
| angularReactivityFeature, | ||
| }, | ||
| } | ||
|
|
||
| const table = constructTable(resolvedOptions) as AngularTable< | ||
| TFeatures, | ||
| TData, | ||
| TSelected | ||
| > | ||
| const tableState = injectSelector(table.store, (state) => state, { | ||
| injector, | ||
| }) | ||
| const tableOptions = injectSelector(table.optionsStore, (state) => state, { | ||
| injector, | ||
| }) | ||
|
|
||
| const updatedOptions = computed<TableOptions<TFeatures, TData>>(() => { | ||
| const tableOptionsValue = options() | ||
| const result: TableOptions<TFeatures, TData> = { | ||
| ...untracked(() => table.options), | ||
| ...tableOptionsValue, | ||
| _features: { ...tableOptionsValue._features, angularReactivityFeature }, | ||
| } | ||
| if (tableOptionsValue.state) { | ||
| result.state = tableOptionsValue.state | ||
| } | ||
| return result | ||
| }) | ||
|
|
||
| effect( | ||
| () => { | ||
| const newOptions = updatedOptions() | ||
| untracked(() => table.setOptions(newOptions)) | ||
| }, | ||
| { injector, debugName: 'tableOptionsUpdate' }, | ||
| ) | ||
| }) as AngularTable<TFeatures, TData, TSelected> |
There was a problem hiding this comment.
Don't let caller features override the Angular reactivity binding.
Because ...options()._features is spread after coreReativityFeature: angularReactivity(injector), any caller-provided coreReativityFeature wins. In that case the table can stop being backed by Angular signals, and table.state / table.computed / table.value will stop updating reactively. Cache options() once and write the Angular binding last.
Suggested fix
- const table = constructTable({
- ...options(),
- _features: {
- coreReativityFeature: angularReactivity(injector),
- ...options()._features,
- },
- }) as AngularTable<TFeatures, TData, TSelected>
+ const initialOptions = options()
+ const table = constructTable({
+ ...initialOptions,
+ _features: {
+ ...initialOptions._features,
+ coreReativityFeature: angularReactivity(injector),
+ },
+ }) as AngularTable<TFeatures, TData, TSelected>📝 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.
| const table = constructTable({ | |
| ...options(), | |
| _features: { | |
| coreReativityFeature: angularReactivity(injector), | |
| ...options()._features, | |
| angularReactivityFeature, | |
| }, | |
| } | |
| const table = constructTable(resolvedOptions) as AngularTable< | |
| TFeatures, | |
| TData, | |
| TSelected | |
| > | |
| const tableState = injectSelector(table.store, (state) => state, { | |
| injector, | |
| }) | |
| const tableOptions = injectSelector(table.optionsStore, (state) => state, { | |
| injector, | |
| }) | |
| const updatedOptions = computed<TableOptions<TFeatures, TData>>(() => { | |
| const tableOptionsValue = options() | |
| const result: TableOptions<TFeatures, TData> = { | |
| ...untracked(() => table.options), | |
| ...tableOptionsValue, | |
| _features: { ...tableOptionsValue._features, angularReactivityFeature }, | |
| } | |
| if (tableOptionsValue.state) { | |
| result.state = tableOptionsValue.state | |
| } | |
| return result | |
| }) | |
| effect( | |
| () => { | |
| const newOptions = updatedOptions() | |
| untracked(() => table.setOptions(newOptions)) | |
| }, | |
| { injector, debugName: 'tableOptionsUpdate' }, | |
| ) | |
| }) as AngularTable<TFeatures, TData, TSelected> | |
| const initialOptions = options() | |
| const table = constructTable({ | |
| ...initialOptions, | |
| _features: { | |
| ...initialOptions._features, | |
| coreReativityFeature: angularReactivity(injector), | |
| }, | |
| }) as AngularTable<TFeatures, TData, TSelected> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-table/src/injectTable.ts` around lines 137 - 143, Cache the
result of options() into a local variable, then pass that cached options to
constructTable while merging _features so that the Angular reactivity binding
overrides any caller-provided feature: take the cachedOptions._features (if any)
and spread them first, then set coreReativityFeature:
angularReactivity(injector) last so it wins; update the constructTable call (and
the cast to AngularTable<TFeatures, TData, TSelected>) to use cachedOptions and
the reordered _features merge to ensure table.state/table.computed/table.value
remain backed by Angular signals.
| @@ -0,0 +1,63 @@ | |||
| // TOTO - re-explore preact signals for reactivity | |||
There was a problem hiding this comment.
Typo: "TOTO" should be "TODO".
📝 Proposed fix
-// TOTO - re-explore preact signals for reactivity
+// TODO - re-explore preact signals for reactivity📝 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.
| // TOTO - re-explore preact signals for reactivity | |
| // TODO - re-explore preact signals for reactivity |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/preact-table/src/reactivity.ts` at line 1, Fix the typo in the
top-of-file comment: change the comment text "TOTO - re-explore preact signals
for reactivity" to "TODO - re-explore preact signals for reactivity" so the
intent is clear and searchable; locate the string "TOTO - re-explore preact
signals for reactivity" in the file and update it to "TODO..." accordingly.
| const state = useSelector(table.store, selector, { compare: shallow }) | ||
| const options = useSelector(table.optionsStore, (options) => options, { | ||
| compare: shallow, | ||
| }) | ||
|
|
||
| return useMemo( | ||
| () => ({ | ||
| ...table, | ||
| options, | ||
| state, | ||
| }), | ||
| [state, table], | ||
| [table, options, state], |
There was a problem hiding this comment.
The returned state no longer matches PreactTable['state'].
This hook now returns the raw selector result, but PreactTable.state is still declared to always include columns and data. That leaves existing consumers able to compile against table.state.columns while getting undefined at runtime. Either restore those fields on the returned state or update the public type/docs as a breaking change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/preact-table/src/useTable.ts` around lines 124 - 135, The hook
useTable now returns the raw selector result in state which can omit required
fields (columns, data) from the declared PreactTable['state'], causing runtime
undefined for table.state.columns; fix by ensuring the returned state always
contains the required shape: when building the return object in useTable (the
selector/result used by useSelector and the returned state), normalize or merge
missing fields so state.columns and state.data are present (e.g., fall back to
table.state.columns/table.state.data or default empty arrays/objects) or update
the public type to reflect the new optional fields — locate useTable, the
selector passed into useSelector, and the returned object spreading table to
implement the normalization or update the type accordingly.
| const state = useSelector(table.store, selector, { compare: shallow }) | ||
| const options = useSelector(table.optionsStore, (options) => options, { | ||
| compare: shallow, | ||
| }) | ||
|
|
||
| // we know this is not the most efficient way to return the table, | ||
| // but it is required for the react compiler to work | ||
| return useMemo( | ||
| () => ({ | ||
| ...table, | ||
| options: tableOptions, | ||
| options, | ||
| state, | ||
| }), | ||
| [table, tableOptions, state], | ||
| [table, options, state], |
There was a problem hiding this comment.
ReactTable['state'] is now unsound at runtime.
The hook returns the raw selector output, but the public ReactTable.state type still promises columns and data. Consumers can keep compiling against table.state.columns and now read undefined at runtime. Either re-add those fields to the returned state or update the public API/migration surface explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-table/src/useTable.ts` around lines 152 - 165, The returned
object from useTable (in useTable.ts where useSelector and useMemo are used to
return {...table, options, state}) can expose a runtime-missing `columns` and
`data` despite the public ReactTable.state type; fix this by
normalizing/ensuring the returned state includes the expected keys (e.g.,
guarantee state.columns and state.data exist by merging defaults or falling back
to table.initialState or empty arrays) before returning from useMemo, or
alternatively update the public API types and migration notes if you intend to
remove those fields (adjust the return in useMemo and the selector usage
accordingly).
| import type { TableReactivityBindings } from '../reactivity' | ||
|
|
||
| export interface CoreFeatures { | ||
| coreReativityFeature?: TableReactivityBindings |
There was a problem hiding this comment.
coreReativityFeature? is marked optional but constructTable accesses it with !, creating a silent runtime failure.
coreFeatures (the exported object) does not include coreReativityFeature, so any user who passes only { ...coreFeatures } to _features will have _reactivity = undefined inside constructTable, causing downstream failures with no type-system warning. The ? and the ! are contradictory: the interface promises "this is fine to omit" while the implementation assumes "this is always present".
Fix options:
- Make it required in the interface and include a default in
coreFeatures, or - Keep it optional and add a guard in
constructTable.ts:
// in constructTable.ts
const _reactivity = tableOptions._features.coreReativityFeature
if (!_reactivity) {
throw new Error('[table-core] coreReativityFeature must be provided in _features')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/core/coreFeatures.ts` at line 10, The interface marks
coreReativityFeature as optional but constructTable accesses it with non-null
assertion, causing silent runtime failures; fix by either (A) making
coreReativityFeature required in the interface and adding a default
coreReativityFeature implementation to the exported coreFeatures object so
callers can safely spread `{ ...coreFeatures }`, or (B) keep it optional and add
a runtime guard in constructTable: read
tableOptions._features.coreReativityFeature into _reactivity and if falsy throw
a clear Error like '[table-core] coreReativityFeature must be provided in
_features' before any use; update references to the symbols
coreReativityFeature, coreFeatures, constructTable, _features, and _reactivity
accordingly.
Typo in new public API: coreReativityFeature → coreReactivityFeature.
"Reativity" is missing the letter c. This name is propagated across every framework adapter, test helper, and feature interface in this PR. Fixing it now (alpha branch) is a one-shot rename; after a stable release it becomes a breaking change across all consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/core/coreFeatures.ts` at line 10, The public API
field is misspelled: rename the symbol coreReativityFeature to
coreReactivityFeature across the codebase (start with the declaration in
coreFeatures.ts), update all imports/uses in adapters, test helpers, feature
interfaces and any exported types or re-exports to the corrected name, and
update related docs/tests to reference coreReactivityFeature so the change is
consistent; ensure you also update any type/interface names, exported indexes,
and usages in runtime code to avoid leftover references to coreReativityFeature.
| Object.defineProperty(atom, 'state', { | ||
| get() { | ||
| return atom.get() | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Object.defineProperty without configurable: true makes atomToStore non-idempotent.
With configurable defaulting to false, calling atomToStore a second time on the same atom instance throws TypeError: Cannot redefine property: state. This can realistically occur in HMR scenarios, tests that reuse atom instances, or any framework that re-initializes the table without recreating the atom.
🛡️ Proposed fix
- Object.defineProperty(atom, 'state', {
+ Object.defineProperty(atom, 'state', {
+ configurable: true,
+ enumerable: true,
get() {
return atom.get()
},
})Alternatively, guard with a check first:
+ if (!Object.getOwnPropertyDescriptor(atom, 'state')) {
Object.defineProperty(atom, 'state', {
+ configurable: true,
get() { return atom.get() },
})
+ }📝 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.
| Object.defineProperty(atom, 'state', { | |
| get() { | |
| return atom.get() | |
| }, | |
| }) | |
| Object.defineProperty(atom, 'state', { | |
| configurable: true, | |
| enumerable: true, | |
| get() { | |
| return atom.get() | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/core/reactivity/coreReactivityFeature.utils.ts`
around lines 20 - 24, The descriptor for atom.state is being defined without
configurable: true which breaks idempotency of atomToStore; update the code in
the atomToStore path that calls Object.defineProperty(atom, 'state', ...) to
either add configurable: true to the property descriptor (e.g.,
Object.defineProperty(atom, 'state', { get() { return atom.get() },
configurable: true })) or, alternatively, guard by checking
Object.getOwnPropertyDescriptor(atom, 'state') and skip redefining if it already
exists; reference the atom object and the 'state' property in your change so
HMR/tests that re-run atomToStore won’t throw.
| * Optionally, provide your own external writable atoms for individual state slices. | ||
| * When an atom is provided for a given slice, it takes precedence over `options.state[key]` | ||
| * and the internal base atom for that slice. Writes originating from the library are | ||
| * still routed through the internal base atom; consumers are responsible for | ||
| * mirroring changes back to their external atom via the corresponding `onXChange` callback. | ||
| */ | ||
| atoms?: ExternalAtoms<TFeatures> | ||
| readonly atoms?: ExternalAtoms<TFeatures> |
There was a problem hiding this comment.
Documented atom precedence doesn't match the runtime order.
Lines 77-83 and Lines 151-155 say options.atoms[key] wins over options.state[key], but packages/table-core/src/core/table/constructTable.ts:61-87 currently reads state first and only falls back to the external atom afterward. If both are present, the external atom never owns that slice, which breaks the contract this PR is introducing. Please align the runtime order or these docs/types before merging.
Also applies to: 151-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/table-core/src/core/table/coreTablesFeature.types.ts` around lines
77 - 83, The runtime selection in constructTable currently prefers options.state
over options.atoms, which contradicts the documented precedence; update the
constructTable logic so for each slice it chooses options.atoms[key] if present,
otherwise options.state[key], and only then falls back to the internal base atom
(keep the existing behavior that library-originating writes go through the
internal base atom and consumers mirror changes via the onXChange callbacks).
Adjust any selection code paths in constructTable (and related helpers) to
reference options.atoms before options.state, and ensure the ExternalAtoms type,
constructTable, and onXChange interactions remain consistent with this
precedence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-table/src/useLegacyTable.ts`:
- Around line 462-465: The inline comment for getState is stale — it says "all
state except for columns and data" but getState (defined via useCallback
returning table.state) now returns the full table.state; update or remove that
comment to accurately reflect the implementation: either change it to something
like "return full table.state" or delete the comment near the getState
useCallback so the comment and the returned value (table.state) are consistent.
🪄 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: fa3ce0cc-ef46-48a0-bed5-1aee661e8414
📒 Files selected for processing (1)
packages/react-table/src/useLegacyTable.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-table/src/useLegacyTable.ts`:
- Around line 405-452: Add a migration caveat to
docs/framework/react/guide/use-legacy-table.md explaining that the lazy
initializer inside useLegacyTable (the const [_rowModels] = useState(() => { ...
}) block) captures row model options (e.g., getFilteredRowModel and custom
filterFns, sortFns, aggregationFns) only on first render and does not update on
rerenders; state that toggling row model features or replacing these custom
functions after mount will have no effect and the table must be remounted to
pick up changes, with a brief example or note advising users to remount to
change row model behavior.
🪄 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: 870519e2-2145-4538-9b21-629dea31ba4c
📒 Files selected for processing (1)
packages/react-table/src/useLegacyTable.ts
| const [_rowModels] = useState(() => { | ||
| const rowModels: CreateRowModels_All<StockFeatures, TData> = {} | ||
|
|
||
| // Legacy row model options are setup-only. Capture the first render's | ||
| // marker options to match the table instance lifecycle. | ||
|
|
||
| if (getFilteredRowModel) { | ||
| rowModels.filteredRowModel = createFilteredRowModel({ | ||
| ...filterFns, | ||
| ...options.filterFns, | ||
| }) | ||
| } | ||
|
|
||
| if (getSortedRowModel) { | ||
| rowModels.sortedRowModel = createSortedRowModel({ | ||
| ...sortFns, | ||
| ...options.sortFns, | ||
| }) | ||
| } | ||
|
|
||
| if (getPaginationRowModel) { | ||
| rowModels.paginatedRowModel = createPaginatedRowModel() | ||
| } | ||
|
|
||
| if (getExpandedRowModel) { | ||
| rowModels.expandedRowModel = createExpandedRowModel() | ||
| } | ||
|
|
||
| if (getGroupedRowModel) { | ||
| rowModels.groupedRowModel = createGroupedRowModel({ | ||
| ...aggregationFns, | ||
| ...options.aggregationFns, | ||
| }) | ||
| } | ||
|
|
||
| if (getFacetedRowModel) { | ||
| rowModels.facetedRowModel = createFacetedRowModel() | ||
| } | ||
|
|
||
| if (getFacetedMinMaxValues) { | ||
| rowModels.facetedMinMaxValues = createFacetedMinMaxValues() | ||
| } | ||
|
|
||
| if (getFacetedUniqueValues) { | ||
| rowModels.facetedUniqueValues = createFacetedUniqueValues() | ||
| } | ||
|
|
||
| return rowModels |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the file size and read the relevant section
wc -l packages/react-table/src/useLegacyTable.tsRepository: TanStack/table
Length of output: 105
🏁 Script executed:
# Read the file around the flagged lines and surrounding context
sed -n '390,480p' packages/react-table/src/useLegacyTable.tsRepository: TanStack/table
Length of output: 2344
🏁 Script executed:
# Search for all usages of _rowModels to understand how it's used
rg '_rowModels' packages/react-table/src/useLegacyTable.ts -n -B 2 -A 2Repository: TanStack/table
Length of output: 4085
🏁 Script executed:
# Check the full function to understand the lifecycle and dependencies
sed -n '1,50p' packages/react-table/src/useLegacyTable.tsRepository: TanStack/table
Length of output: 1379
🏁 Script executed:
# Look for any dependency arrays or effect hooks that might update row models
rg 'useEffect|useMemo|useCallback' packages/react-table/src/useLegacyTable.ts -n -A 3Repository: TanStack/table
Length of output: 642
🏁 Script executed:
# Search for test files related to useLegacyTable
fd -t f 'useLegacyTable' --type f -e 'test.ts' -e 'spec.ts' -e 'test.tsx' -e 'spec.tsx'Repository: TanStack/table
Length of output: 40
🏁 Script executed:
# Look for any tests or documentation about legacy table behavior
rg 'useLegacyTable' -t ts -t tsx -lRepository: TanStack/table
Length of output: 86
🏁 Script executed:
# Check if there are any other comments or documentation about the setup-only behavior
rg 'setup-only|non-reactive|reactive' packages/react-table/src/ -i -n -B 2 -A 2Repository: TanStack/table
Length of output: 499
🏁 Script executed:
# Look for any mechanism that might update _rowModels after initialization
rg 'setRowModels|updateRowModels|_rowModels.*=' packages/react-table/src/useLegacyTable.ts -nRepository: TanStack/table
Length of output: 102
🏁 Script executed:
# Check if filterFns, sortFns, aggregationFns are used elsewhere after the initializer
sed -n '1,488p' packages/react-table/src/useLegacyTable.ts | grep -n 'filterFns\|sortFns\|aggregationFns'Repository: TanStack/table
Length of output: 1418
🏁 Script executed:
# Search for migration guides or documentation about this limitation
rg 'useLegacyTable|setup-only|non-reactive' -t md -t txt -i --max-count=20Repository: TanStack/table
Length of output: 12607
🏁 Script executed:
# Look for CHANGELOG or migration guides
fd -t f '(CHANGELOG|MIGRATION|GUIDE)' --type fRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Check if there are any GitHub issues or discussions referenced
rg 'TODO|FIXME|XXX' packages/react-table/src/useLegacyTable.ts -nRepository: TanStack/table
Length of output: 40
🏁 Script executed:
# Get full context around the setUp-only comment and any surrounding documentation
sed -n '350,470p' packages/react-table/src/useLegacyTable.tsRepository: TanStack/table
Length of output: 3352
🏁 Script executed:
# Read the full migration guide for useLegacyTable
cat docs/framework/react/guide/use-legacy-table.mdRepository: TanStack/table
Length of output: 7288
Row model initialization is non-reactive—document this caveat for v8 migrations.
The lazy useState(() => ...) initializer captures row model options (getFilteredRowModel, custom filterFns/sortFns/aggregationFns, etc.) on first render and never updates them. While the code comment indicates this is intentional ("setup-only"), this is not documented in the migration guide as a caveat. Users upgrading from v8—where these options are reactive across rerenders—may silently encounter bugs if they attempt to swap custom functions or toggle row model features after mount. Add a caveats section to docs/framework/react/guide/use-legacy-table.md explicitly stating that row model options are frozen at initialization and cannot be changed without remounting the table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-table/src/useLegacyTable.ts` around lines 405 - 452, Add a
migration caveat to docs/framework/react/guide/use-legacy-table.md explaining
that the lazy initializer inside useLegacyTable (the const [_rowModels] =
useState(() => { ... }) block) captures row model options (e.g.,
getFilteredRowModel and custom filterFns, sortFns, aggregationFns) only on first
render and does not update on rerenders; state that toggling row model features
or replacing these custom functions after mount will have no effect and the
table must be remounted to pick up changes, with a brief example or note
advising users to remount to change row model behavior.
2 participants
Allows
constructTableto support a newreactivityfields used by adapters to pass custom signals implementationSummary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores
Tests