feat(table): support external reactivity binding#6237
feat(table): support external reactivity binding#6237riccardoperra wants to merge 11 commits intoalphafrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors TanStack Table's reactivity system from a framework-specific feature-based approach to a framework-agnostic ChangesReactivity Abstraction & Core Migration
Framework Adapters: React, Vue, Solid, Svelte, Angular, Preact, Lit
Dependency Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
View your CI Pipeline Execution ↗ for commit 4984c5c
☁️ 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}
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 add the missing import { solidReactivity } from './signals' to packages/solid-table/src/createTable.ts, which fixes the Cannot find name 'solidReactivity' TypeScript error introduced when the PR refactored native reactivity bindings into a separate signals.ts file. This also resolves the knip "unused file" warning for signals.ts, as the file is now reachable from createTable.ts.
Tip
✅ We verified this fix by re-running @tanstack/solid-table:test:types, table:test:knip.
diff --git a/packages/solid-table/src/createTable.ts b/packages/solid-table/src/createTable.ts
index 09790fc79..fe62fde30 100644
--- a/packages/solid-table/src/createTable.ts
+++ b/packages/solid-table/src/createTable.ts
@@ -2,6 +2,7 @@ import { constructTable } from '@tanstack/table-core'
import { createComputed, getOwner, mergeProps, untrack } from 'solid-js'
import { shallow, useSelector } from '@tanstack/solid-store'
import { FlexRender } from './FlexRender'
+import { solidReactivity } from './signals'
import type { Atom, ReadonlyAtom } from '@tanstack/solid-store'
import type { Accessor, JSX } from 'solid-js'
import type {
Or Apply changes locally with:
npx nx-cloud apply-locally lZFc-mprV
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
|
@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.
| /** | ||
| * Table custom reactibity bindings. | ||
| */ | ||
| readonly reactivity?: TableReactivityBindings |
There was a problem hiding this comment.
Typo in JSDoc: "reactibity" → "reactivity".
- * Table custom reactibity bindings.
+ * Table custom reactivity bindings.📝 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.
| /** | |
| * Table custom reactibity bindings. | |
| */ | |
| readonly reactivity?: TableReactivityBindings | |
| /** | |
| * Table custom reactivity bindings. | |
| */ | |
| readonly reactivity?: TableReactivityBindings |
🤖 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
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.
| 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.
2 participants
Allows
constructTableto support a newreactivityfields used by adapters to pass custom signals implementationSummary by CodeRabbit
Dependencies Updated
Improvements