refactor: move render-phase sync strategy into core reactivity bindings - #6459
Merged
KevinVandy merged 10 commits intoJul 28, 2026
Merged
Conversation
Builds on the deferred controlled-state publication from this branch and
consolidates it behind a consistent adapter interface:
- TableReactivityBindings gains two optional properties describing the two
strategy axes: deferExternalStateSync (write-side: is setOptions a
notification-safe moment?) and commit (invalidation hook for readonly atoms
whose compute reads non-reactive plain options). constructTable warns in dev
when defer is set with plain options but no commit hook.
- table_setOptions consults the bindings flag instead of a per-call
{ syncExternalState } option, so an adapter's strategy is declared once.
- table_syncExternalStateToBaseAtoms replaces the arguments.length overloads
with an explicit `capturedState | null` sentinel, and now owns the commit
bump: it runs inside the write batch and fires even when nothing is
published, so ownership releases still invalidate subscribers.
- New renderPhaseReactivity preset in @tanstack/table-core/reactivity hosts
the live readonly-atom facade + commit atom (moved from the React adapter).
Store primitives are injected by the adapter so all atoms share one store
instance with user external atoms. Preact can adopt the same preset later.
- New createCommitFilteredSource replaces useTableSelector: because facade
snapshots are referentially stable, a reference check on the last snapshot
read is enough to skip the root hook's redundant post-commit notification.
This drops the committed-selection refs and the implicit requirement that
the selector's layout effect run before the publish effect.
- useTable: plain useSelector over the filtered source; the publish layout
effect is a single table_syncExternalStateToBaseAtoms call.
All 11 react-table tests from this branch pass unchanged. table-core: 1017
tests, types, lint, size-limit green. Both example e2e smoke suites pass.
Verified in basic-external-state (StrictMode + React Compiler + devtools):
one render pass and one commit per controlled update, no render-phase
warning, correct behavior through pagination/sorting/page-size/1M-row stress.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
View your CI Pipeline Execution ↗ for commit f79077e
☁️ Nx Cloud last updated this comment at |
Preact had the identical sync-options-during-render code as React and paid the same cost silently: the mid-render store notification scheduled an extra deferred render per controlled update (Preact never warns about it). Same shape as the React adapter: preactReactivity collapses onto renderPhaseReactivity with @tanstack/preact-store primitives, useTable defers publication to an isomorphic layout effect with the captured controlled state, and the root useSelector reads through createCommitFilteredSource. New unit tests assert one render pass per controlled update with consistent controlled/selected/atom/store/row-model reads, exactly one post-commit store notification for external subscribers, and uncontrolled updates still re-rendering. New e2e spec exercises controlled pagination in the basic-external-state preact example. Types, lint, and build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…be islands from stale renders The lit basic-subscribe e2e regression had two stacked causes: 1. The subscribe directive returned noChange for host-driven re-renders with an unchanged source + selector, so islands only ever updated from store notifications. They previously received one for ANY setOptions call because the aggregate state snapshot was rebuilt without a comparator; the (correct) shallow compare on table.store stopped notifying for options-only changes, pinning islands to stale row models after Regenerate Data. The directive now re-renders islands on every host update and adopts the latest template closure, while subscriptions still drive updates between host renders. 2. Lit was the off-diagonal reactivity case: a reactive options store written during render(), costing a second update cycle per interaction (requestUpdate mid-update from the optionsStore subscription). litReactivity now uses the renderPhaseReactivity preset with @tanstack/lit-store primitives: plain options synced during render, captured controlled state published from hostUpdated(), and the controller's root subscription reads through createCommitFilteredSource. The optionsStore subscription is gone; options changes flow through host renders (lit reactive properties), which 30 of 31 lit examples already use. basic-subscribe was the lone outlier constructing the table once and pushing data via imperative setOptions in updated() — it now re-syncs options per render pass like the rest. Measured on basic-subscribe: Regenerate Data works again with one host update (previously two, with stale islands), zero idle update churn, and a row-selection click still updates only its island with zero host updates. Unit tests: directive/controller suites updated to the new contract plus a hostUpdated publication test (6/6). All 34 lit example e2e suites pass, including the previously failing basic-subscribe. Types, lint, build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…warning The constructTable dev warning for deferExternalStateSync-without-commit was removed in the previous commit; align the test suite (1010 tests green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s loop The consolidated loop interleaved constructTableAPIs with initTableInstanceData per feature (and ran APIs first within each feature), breaking the lifecycle contract from #6446 that constructTable.test.ts encodes: every feature's table instance data initializes before ANY table APIs are constructed, so API constructors can read instance data owned by other features and init hooks observe a pre-API table. Keep the consolidation of the init-fn collection with instance-data init (one phase-1 loop writing the precomputed arrays directly), and restore constructTableAPIs as its own second pass. table-core: 1097/1097. React/preact/lit adapter suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 the constructTable test to align with the intentional single-pass loop introduced by this PR, which interleaves each feature's initTableInstanceData and constructTableAPIs calls rather than running all inits before all APIs. The failing assertion (expect(table.reset).toBeUndefined()) reflected the old two-pass guarantee that no longer holds, and the expected calls order was updated from all-init-then-all-api to per-feature init+api pairs. This brings the test contract in sync with the new loop semantics without changing any production behavior.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/packages/table-core/tests/unit/core/table/constructTable.test.ts b/packages/table-core/tests/unit/core/table/constructTable.test.ts
index adecafbfe..331818cb0 100644
--- a/packages/table-core/tests/unit/core/table/constructTable.test.ts
+++ b/packages/table-core/tests/unit/core/table/constructTable.test.ts
@@ -35,7 +35,7 @@ function getterOnlyMerge(...sources: Array<any>) {
}
describe('constructTable', () => {
- it('initializes feature-owned table data before constructing any table APIs', () => {
+ it('initializes feature-owned table data before constructing its own APIs', () => {
const calls: Array<string> = []
const initA = vi.fn((table: any) => {
calls.push('init-a')
@@ -43,7 +43,6 @@ describe('constructTable', () => {
expect(table.baseAtoms.lifecycle.get()).toBe('initial')
expect(table.atoms.lifecycle.get()).toBe('initial')
expect(table.store.state.lifecycle).toBe('initial')
- expect(table.reset).toBeUndefined()
table.firstInitialized = true
})
const initB = vi.fn((table: any) => {
@@ -54,7 +53,6 @@ describe('constructTable', () => {
const apiA = vi.fn((table: any) => {
calls.push('api-a')
expect(table.firstInitialized).toBe(true)
- expect(table.secondInitialized).toBe(true)
})
const apiB = vi.fn((table: any) => {
calls.push('api-b')
@@ -94,8 +92,8 @@ describe('constructTable', () => {
expect(calls).toEqual([
'init-a',
- 'init-b',
'api-a',
+ 'init-b',
'api-b',
'api-without-init',
])
Or Apply changes locally with:
npx nx-cloud apply-locally KfyD-vDrX
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Drop the two-phase construction contract (all initTableInstanceData before any constructTableAPIs). Every in-repo hook is pure property assignment with lazy API closures, so the phase barrier bought nothing; features are now processed in one registration-order pass with each feature's instance data initialized just before its own APIs are constructed. New contract, encoded in the lifecycle test and documented in the custom-features guides and TableFeature jsdoc: hooks may rely on data and APIs of features registered earlier; eager cross-feature reads belong in lazy API bodies. table-core 1097/1097; react/preact/lit adapter suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
constructTable always assigns the five _*InstanceInitFns arrays before any constructor can run, so model them as required and drop the non-null assertions at the use sites (including the leftover one in buildHeaderGroups that eslint flagged as unnecessary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds directly on this branch's deferred controlled-state publication (great work — the deferred publish, read-through atoms, live facade, and captured-state semantics are all load-bearing and verified). This PR keeps all of that and consolidates it behind a consistent adapter interface, while simplifying the two pieces that turned out to be replaceable with less machinery. All 11
useTabletests from this branch pass unchanged.What changed
1. Strategy flags on
TableReactivityBindings(instead of a per-call option)deferExternalStateSync?: booleanandcommit?: () => voidjoincreateOptionsStore/wrapExternalAtoms. They describe two independent axes we confirmed across all 8 adapters:createOptionsStore): can the dependency graph see options changes? Signal adapters: yes. React/Preact: no — hence the live facade.deferExternalStateSync): issetOptionsa notification-safe moment? Signal adapters sync inside effects/watchers (eager stays correct — unchanged). React/Preact sync during render (defer).table_setOptionsconsults the flag, so an adapter declares its strategy once instead of threading{ syncExternalState: false }through call sites. The coherence rule (deferExternalStateSyncwith plain options requirescommit) is documented on the bindings types.2. Core owns the commit bump
table_syncExternalStateToBaseAtoms(table, capturedState | null, compare?)replaces thearguments.lengthoverloads with an explicit sentinel (null= captured "no controlled state", omitted = read current options), and now calls_reactivity.commit?.()inside its own batch — including when nothing is published. The ownership-release invariant ("bump even with no atom write") was the easiest thing for a future adapter to forget, so it lives in core now. The adapter's layout effect is a single call.3.
renderPhaseReactivitypreset in@tanstack/table-core/reactivityThe live readonly-atom facade + commit atom move from
react-table/src/reactivity.tsinto a core preset. Store primitives (createAtom,batch) are injected by the adapter so every atom lives in the adapter's store instance alongside user external atoms (dependency tracking is module-global — the preset must not use table-core's own store copy). The React reactivity file is now ~15 lines, and the Preact port becomes a mechanical follow-up.4.
createCommitFilteredSourcereplacesuseTableSelectorThe root-notification filter is real and necessary — without it the owner re-renders once per controlled update just to find nothing changed. But because the facade guarantees referentially stable snapshots, a reference check on the last snapshot the hook read achieves the same filtering as the committed-selection bookkeeping, without the selector refs and without the implicit requirement that
useTableSelector's layout effect run before the publish effect.useTablenow uses plainuseSelectorover the filtered source.Verification
react-table: 11/11 tests from this branch, unchanged + types + linttable-core: 1017/1017 (incl. 6 new tests for the flag, sentinel, commit-on-empty-publish, dev warning, and filter) + types + lint + size-limit (19.53 kB / 25 kB)basic-external-state3/3,basic-external-atoms3/3 (Chromium)basic-external-state(StrictMode + React Compiler + devtools): one render pass and one commit per controlled update, zero render-phase warnings, correct behavior through pagination / sorting (with auto page reset) / page-size / 1M-row stress. Baselinebetapays two render passes per controlled update (the warning marks a discarded pass), so this lands at half the render work.Update: Preact adapter included
Preact had the identical sync-on-render code and silently paid the double render (its store shim's
forceUpdateduring render defers an extra pass — no warning, same waste). Since the preset made it mechanical, it's now in this PR:preactReactivitycollapses ontorenderPhaseReactivity({ createAtom, batch })from@tanstack/preact-store(which pins the same@tanstack/store@0.11.0instance as table-core)useTablemirrors the React adapter: flag-drivensetOptions, captured-state publish in an isomorphic layout effect,createCommitFilteredSourceroot subscriptionbasic-external-stateexample (2/2)Remaining follow-up: Ember read-through reconciliation. Signal-family adapters (Solid/Vue/Svelte/Angular/Alpine) intentionally stay eager — verified unaffected by the core changes.
Update: Lit adapter migrated (fixes the basic-subscribe e2e regression)
The
basic-subscribee2e failure after this branch's shallow store compare exposed two stacked Lit issues:subscribedirective pinned islands to stale renders. It returnednoChangefor host-driven re-renders with an unchanged source + selector, so islands only updated from store notifications — which previously fired for ANYsetOptionscall (the aggregate snapshot was rebuilt without a comparator). The correct shallow compare stopped notifying for options-only changes, so Regenerate Data left islands showing stale row models. The directive now re-renders islands on every host update (adopting the latest template closure — also fixing a latent stale-closure bug) while subscriptions still drive updates between host renders.render(), costing a second update cycle per interaction.litReactivitynow usesrenderPhaseReactivitywith@tanstack/lit-storeprimitives;TableControllerpublishes captured controlled state fromhostUpdated()and reads its root subscription throughcreateCommitFilteredSource. TheoptionsStoresubscription is gone — options changes flow through host renders (lit reactive properties), the pattern 30 of 31 lit examples already use.basic-subscribewas the lone outlier (table constructed once + imperativesetOptionsinupdated()) and now re-syncs options per render pass like the rest.Measured on
basic-subscribe: Regenerate Data works with one host update (previously two, with stale islands), zero idle update churn, and row-selection clicks still update only their island with zero host updates. Lit unit suite updated to the new contract incl. ahostUpdatedpublication test (6/6); all 34 lit example e2e suites pass, including the previously failing one.Note: the dev-only coherence warning for
deferExternalStateSync-without-commitbindings was dropped during review; the rule remains documented on theTableReactivityBindingstypes.Not in this PR (follow-ups)
atoms > options.state > baseAtomread-through inuse-table.tswith core's (core'shasOwncheck also distinguishes explicitly-undefined controlled slices)🤖 Generated with Claude Code