feat: refactor reactivity bindings to work without tanstack-store#6289
feat: refactor reactivity bindings to work without tanstack-store#6289riccardoperra wants to merge 1 commit into
Conversation
|
View your CI Pipeline Execution ↗ for commit fb969ef
☁️ Nx Cloud last updated this comment at |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces external atom wrapping and lifecycle management for TanStack Table's reactivity system. The core interface expands to support wrapping external atoms and lifecycle hooks, core table logic implements two-way atom synchronization, Angular and Solid adapters are refactored to use Signal-based wrapping instead of Store atoms, and other adapters provide stub implementations. ChangesExternal Atom Wrapping and Adapter Lifecycle Management
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 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. Comment |
![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.
riccardoperra
force-pushed
the
fix/native-reactivity-with-external-atoms
branch
from
fb969ef to
ba8f961
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/angular-table/src/reactivity.ts (1)
62-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOutdated JSDoc comment.
The documentation states "Table state atoms are backed by TanStack Store atoms" but the implementation now uses Angular signals directly via
computed/signalandtoObservable.📝 Suggested fix
/** * Creates the table-core reactivity bindings used by the Angular adapter. * - * Table state atoms are backed by TanStack Store atoms. The options store stays - * framework-native because row-model APIs read `table.options` directly during - * render. Readonly table atoms bridge Store dependency tracking into Angular - * computed signals. + * Table state atoms are backed by Angular signals. The options store stays + * framework-native because row-model APIs read `table.options` directly during + * render. Readonly table atoms use `toObservable` to bridge Angular signal + * updates into atom subscription semantics. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/angular-table/src/reactivity.ts` around lines 62 - 68, Update the outdated JSDoc block that begins "Creates the table-core reactivity bindings used by the Angular adapter." to accurately describe the current implementation: state atoms are implemented with Angular signals via computed/signal and interop uses toObservable (not TanStack Store atoms), mention that options remain framework-native and readonly atoms bridge Angular computed signals where applicable; update any terminology and examples in that comment to reference computed, signal, and toObservable so readers understand the actual mechanism used.
🧹 Nitpick comments (1)
packages/solid-table/src/reactivity.ts (1)
23-33: 💤 Low valueClarify owner-context risk: atom creation is synchronous during
createTable, so the original concern mainly applies to out-of-contract async usage.
createReadonlyAtom/createWritableAtominpackages/solid-table/src/reactivity.tscallcreateMemo/createSignalwithoutrunWithOwner(owner, ...), but table atom creation happens synchronously insidecreateTable/constructTableaftercreateTablecapturesownerviagetOwner()!. The owner mismatch risk would only apply if someone usessolidReactivity(owner)to create atoms later from a different owner context (not the normal table flow).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-table/src/reactivity.ts` around lines 23 - 33, Update the inline comment/docs in packages/solid-table/src/reactivity.ts to clarify the owner-context risk: state that createReadonlyAtom/createWritableAtom (and signalToReadonlyAtom) are invoked synchronously inside createTable/constructTable after createTable captures the owner via getOwner(), so owner mismatch is only a concern when consumers call solidReactivity(owner) to create atoms later from a different owner (i.e., out-of-contract async usage); reference the functions signalToReadonlyAtom, createReadonlyAtom, createWritableAtom and the getOwner() capture to make the scope and safe synchronous assumption explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lit-table/src/reactivity.ts`:
- Around line 22-26: The current export in packages/lit-table/src/reactivity.ts
defines unmount as a function that throws, which can break callers expecting an
optional no-op; update the reactivity binding so unmount is omitted or
implemented as a harmless no-op instead of throwing (i.e., remove the throwing
implementation for unmount in the exported object returned from the Lit
reactivity adapter), leaving addSubscription unchanged; this keeps
TableController.hostDisconnected and constructTable (with wrapExternalAtoms
usage) safe while preserving the optional unmount contract.
In `@packages/solid-table/src/createTable.ts`:
- Around line 127-132: The merge currently allows user-supplied
coreReativityFeature to override the adapter's binding because in mergedOptions
you spread ...tableOptions._features after coreReativityFeature; change the
merge so the adapter's coreReativityFeature wins by placing
...tableOptions._features before coreReativityFeature (i.e., ensure the object
passed to mergeProps sets coreReativityFeature: reactivity last), updating the
creation of mergedOptions (in createTable.ts where mergeProps, tableOptions,
_features, coreReativityFeature, and reactivity are referenced) accordingly.
---
Outside diff comments:
In `@packages/angular-table/src/reactivity.ts`:
- Around line 62-68: Update the outdated JSDoc block that begins "Creates the
table-core reactivity bindings used by the Angular adapter." to accurately
describe the current implementation: state atoms are implemented with Angular
signals via computed/signal and interop uses toObservable (not TanStack Store
atoms), mention that options remain framework-native and readonly atoms bridge
Angular computed signals where applicable; update any terminology and examples
in that comment to reference computed, signal, and toObservable so readers
understand the actual mechanism used.
---
Nitpick comments:
In `@packages/solid-table/src/reactivity.ts`:
- Around line 23-33: Update the inline comment/docs in
packages/solid-table/src/reactivity.ts to clarify the owner-context risk: state
that createReadonlyAtom/createWritableAtom (and signalToReadonlyAtom) are
invoked synchronously inside createTable/constructTable after createTable
captures the owner via getOwner(), so owner mismatch is only a concern when
consumers call solidReactivity(owner) to create atoms later from a different
owner (i.e., out-of-contract async usage); reference the functions
signalToReadonlyAtom, createReadonlyAtom, createWritableAtom and the getOwner()
capture to make the scope and safe synchronous assumption explicit.
🪄 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: 4e054110-b6d4-4e15-b737-b3e3f97d111f
📒 Files selected for processing (12)
examples/angular/basic-external-atoms/src/app/app.tspackages/angular-table/src/injectTable.tspackages/angular-table/src/reactivity.tspackages/lit-table/src/reactivity.tspackages/preact-table/src/reactivity.tspackages/react-table/src/reactivity.tspackages/solid-table/src/createTable.tspackages/solid-table/src/reactivity.tspackages/table-core/src/core/reactivity/coreReactivityFeature.types.tspackages/table-core/src/core/table/constructTable.tspackages/table-core/src/store-reactivity-bindings.tspackages/vue-table/src/reactivity.ts
💤 Files with no reviewable changes (1)
- examples/angular/basic-external-atoms/src/app/app.ts
| unmount: () => { | ||
| throw new Error( | ||
| 'Feature not supported in current reactivity implementation', | ||
| ) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of reactivity.unmount and how presence is checked
rg -nP '\bunmount\b' --type=ts -C3 -g '!**/*.test.ts' packagesRepository: TanStack/table
Length of output: 8030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how lit-table calls the adapter unmount during teardown
rg -nP '\bunmount\s*[\?\)]' packages/lit-table -S
# Also check whether lit uses the shared core reactivity binding (table._reactivity.unmount?.)
rg -nP 'table\._reactivity\.unmount\?\.' packages/lit-table -S || true
# Check any generic/framework-agnostic unmount invocation that lit's adapter might be wired into
rg -nP '\.unmount\?\.' packages/table-core -S
rg -nP '\._reactivity\.unmount' packages -SRepository: TanStack/table
Length of output: 40
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== Call sites: table._reactivity.unmount?.() =="
rg -nP 'table\._reactivity\.unmount\?\s*\(' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: reactivity.unmount?.() =="
rg -nP 'reactivity\.unmount\?\s*\(' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: reactivity.unmount (direct) =="
rg -nP 'reactivity\.unmount\s*\(' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: .unmount?.() (generic) =="
rg -nP '\.unmount\?\s*\(' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: _reactivity.unmount (any form) =="
rg -nP '\._reactivity\.unmount\b' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"Repository: TanStack/table
Length of output: 395
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== Call sites: table._reactivity.unmount =="
rg -nP 'table\._reactivity\.unmount\b' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: ._reactivity.unmount( =="
rg -nP '\._reactivity\.unmount\s*\(' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"
echo "== Call sites: core.unmount =="
rg -nP '\.unmount\s*\(' --type=ts packages -S -g '!**/*.test.ts' packages/react-table packages/vue-table packages/preact-table packages/lit-table || echo "(none)"
echo "== Call sites: reactivity.unmount =="
rg -nP '\breactivity\.unmount\b' --type=ts packages -S -g '!**/*.test.ts' || echo "(none)"Repository: TanStack/table
Length of output: 643
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== table-core: any access to _reactivity.unmount / reactivity.unmount =="
rg -nP '\.(_reactivity|reactivity)\.unmount\b' packages/table-core -S || echo "(none)"
echo "== table-core: any call to unmount() (direct) =="
rg -nP '\bunmount\s*\(\s*\)' packages/table-core -S || echo "(none)"
echo "== table-core: any call to unmount?.() (optional) =="
rg -nP '\bunmount\s*\?\.' packages/table-core -S || echo "(none)"
echo "== table-core: core reactivity bindings destroy/unmount wiring =="
rg -nP 'store-reactivity-bindings|coreReactivityFeature|reactivityFeature|_reactivity' packages/table-core -SRepository: TanStack/table
Length of output: 5042
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== table-core: all unmount mentions (non-tests) =="
rg -nP '\bunmount\b' packages/table-core/src -S | head -n 200
echo "== table-core: any call to reactivity.unmount or _reactivity.unmount (broader) =="
rg -nP '\b(reactivity|_reactivity)\.unmount\b' packages/table-core/src -S || echo "(none)"
echo "== table-core: any call to unmount() on something called core / table =="
rg -nP '\.unmount\s*\(\s*\)' packages/table-core/src -S || echo "(none)"
echo "== lit-table: any reference to _reactivity.unmount / reactivity.unmount / core.unmount =="
rg -nP '\b(_reactivity|reactivity)\.unmount\b|\.unmount\s*\(' packages/lit-table/src -S || echo "(none)"
echo "== react-table: any reference to _reactivity.unmount / reactivity.unmount / core.unmount =="
rg -nP '\b(_reactivity|reactivity)\.unmount\b|\.unmount\s*\(' packages/react-table/src -S || echo "(none)"
echo "== preact-table: any reference to _reactivity.unmount / reactivity.unmount / core.unmount =="
rg -nP '\b(_reactivity|reactivity)\.unmount\b|\.unmount\s*\(' packages/preact-table/src -S || echo "(none)"
echo "== vue-table: any reference to _reactivity.unmount / reactivity.unmount / core.unmount =="
rg -nP '\b(_reactivity|reactivity)\.unmount\b|\.unmount\s*\(' packages/vue-table/src -S || echo "(none)"Repository: TanStack/table
Length of output: 845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the contents around the unmount stub in table-core
sed -n '1,120p' packages/table-core/src/store-reactivity-bindings.ts
# Find and inspect where core "destroy"/"unmount" lifecycle is implemented (coreTablesFeature etc.)
echo "---- coreTablesFeature for destroy/unmount ----"
rg -nP 'destroy\(|unmount\(|teardown|cleanup' packages/table-core/src/core -S
# Grab key files that likely implement table.destroy / lifecycle
ls packages/table-core/src/core/table 2>/dev/null || true
ls packages/table-core/src/core 2>/dev/null || true
# Inspect constructTable.ts where reactivity bindings are wired; then search within for unmount/destroy usage
sed -n '1,220p' packages/table-core/src/core/table/constructTable.ts
rg -nP 'destroy\(|unmount|cleanup' packages/table-core/src/core/table/constructTable.ts -S || trueRepository: TanStack/table
Length of output: 7411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lit-table: unmount invocations =="
rg -nP '\.(_reactivity\.)?unmount\s*\?\.' packages/lit-table/src -S || echo "(none)"
rg -nP '\.(_reactivity\.)?unmount\s*\(' packages/lit-table/src -S || echo "(none)"
rg -nP '\bunmount\b' packages/lit-table/src -S || echo "(none)"
echo "== lit-table: likely teardown hooks =="
rg -nP '(disconnectedCallback|disconnected|disposal|cleanup|willDestroy|dispose|onDisconnect|disconnectedCallback)' packages/lit-table/src -S || echo "(none)"
echo "== lit-table: where reactivity binding object is stored =="
rg -nP 'reactivity|_reactivity' packages/lit-table/src -SRepository: TanStack/table
Length of output: 1396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nS '(destroy|dispose|cleanup|disconnectedCallback|disconnected|onDisconnect|willDestroy|unmount)' packages/lit-table/src || echo "(none)"
echo "---- TableController.ts (relevant excerpts around lifecycle/cleanup) ----"
# Show portions where controllers are constructed and where any cleanup might be defined
sed -n '1,260p' packages/lit-table/src/TableController.ts
echo "---- lit-table/src/reactivity.ts ----"
sed -n '1,120p' packages/lit-table/src/reactivity.tsRepository: TanStack/table
Length of output: 9366
Avoid throwing unmount in Lit reactivity bindings
packages/lit-table/src/reactivity.ts sets unmount: () => { throw ... }. Lit’s teardown (TableController.hostDisconnected) only unsubscribes subscriptions and table-core doesn’t call reactivity.unmount, so this won’t crash in current Lit teardown. Still, unmount is optional (unmount?: () => void); if any teardown path later calls it with ?.() the throw defeats the intended no-op contract—prefer omitting unmount (or making it a no-op). addSubscription is fine to throw because constructTable only reaches it behind wrapExternalAtoms, which Lit sets to false.
♻️ Proposed change
addSubscription: () => {
throw new Error(
'Feature not supported in current reactivity implementation',
)
},
- unmount: () => {
- throw new Error(
- 'Feature not supported in current reactivity implementation',
- )
- },📝 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.
| unmount: () => { | |
| throw new Error( | |
| 'Feature not supported in current reactivity implementation', | |
| ) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/lit-table/src/reactivity.ts` around lines 22 - 26, The current
export in packages/lit-table/src/reactivity.ts defines unmount as a function
that throws, which can break callers expecting an optional no-op; update the
reactivity binding so unmount is omitted or implemented as a harmless no-op
instead of throwing (i.e., remove the throwing implementation for unmount in the
exported object returned from the Lit reactivity adapter), leaving
addSubscription unchanged; this keeps TableController.hostDisconnected and
constructTable (with wrapExternalAtoms usage) safe while preserving the optional
unmount contract.
| const mergedOptions = mergeProps(tableOptions, { | ||
| _features: { | ||
| coreReativityFeature: solidReactivity(owner), | ||
| coreReativityFeature: reactivity, | ||
| ...tableOptions._features, | ||
| }, | ||
| }) as any |
There was a problem hiding this comment.
Merge order allows user to override adapter reactivity.
The spread order places ...tableOptions._features after coreReativityFeature: reactivity, which means if a user passes their own coreReativityFeature in tableOptions._features, it will silently override the Solid adapter's bindings. This is likely unintentional since the adapter should enforce its framework-specific reactivity.
Consider reversing the order so the adapter's reactivity wins:
Proposed fix
const mergedOptions = mergeProps(tableOptions, {
_features: {
- coreReativityFeature: reactivity,
...tableOptions._features,
+ coreReativityFeature: reactivity,
},
}) as any🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-table/src/createTable.ts` around lines 127 - 132, The merge
currently allows user-supplied coreReativityFeature to override the adapter's
binding because in mergedOptions you spread ...tableOptions._features after
coreReativityFeature; change the merge so the adapter's coreReativityFeature
wins by placing ...tableOptions._features before coreReativityFeature (i.e.,
ensure the object passed to mergeProps sets coreReativityFeature: reactivity
last), updating the creation of mergedOptions (in createTable.ts where
mergeProps, tableOptions, _features, coreReativityFeature, and reactivity are
referenced) accordingly.
riccardoperra
force-pushed
the
fix/native-reactivity-with-external-atoms
branch
from
ba8f961 to
68b124e
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We add the two properties required by the updated TableReactivityBindings interface (wrapExternalAtoms and addSubscription) to the svelte adapter's svelteReactivity() function, which were introduced in @tanstack/table-core by this PR but not applied to the Svelte package. The fix follows the same subscription-tracking pattern used by the Angular and Solid adapters, resolving the svelte-check type error.
Tip
✅ We verified this fix by re-running @tanstack/svelte-table:test:types.
Suggested Fix changes
diff --git a/packages/svelte-table/src/reactivity.svelte.ts b/packages/svelte-table/src/reactivity.svelte.ts
index 8549be44..3078f961 100644
--- a/packages/svelte-table/src/reactivity.svelte.ts
+++ b/packages/svelte-table/src/reactivity.svelte.ts
@@ -4,7 +4,12 @@ import type {
TableAtomOptions,
TableReactivityBindings,
} from '@tanstack/table-core/reactivity'
-import type { Atom, Observer, ReadonlyAtom } from '@tanstack/svelte-store'
+import type {
+ Atom,
+ Observer,
+ ReadonlyAtom,
+ Subscription,
+} from '@tanstack/svelte-store'
const optionsStoreDebugName = 'table/optionsStore'
@@ -56,8 +61,18 @@ function createRuneWritableAtom<T>(initialValue: T): Atom<T> {
* render. Readonly table atoms bridge Store dependency tracking into `$derived.by`.
*/
export function svelteReactivity(): TableReactivityBindings {
+ const subscriptions = new Set<Subscription>()
+
return {
createOptionsStore: true,
+ wrapExternalAtoms: true,
+ addSubscription: (subscription) => {
+ subscriptions.add(subscription)
+ },
+ unmount: () => {
+ subscriptions.forEach((s) => s.unsubscribe())
+ subscriptions.clear()
+ },
schedule: (fn) => queueMicrotask(() => fn()),
createReadonlyAtom: <T>(fn: () => T, _options?: TableAtomOptions<T>) => {
const storeAtom = createAtom(() => fn(), {
Or Apply changes locally with:
npx nx-cloud apply-locally PRI9-SMyR
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
1 participant
Summary by CodeRabbit
New Features
Bug Fixes