feat(db): shared live-query observer + migrate all five adapters (RFC #1623 step 3)#1642
feat(db): shared live-query observer + migrate all five adapters (RFC #1623 step 3)#1642kevin-dp wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds a shared ChangesShared live-query observer migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +1.06 kB (+0.85%) Total Size: 126 kB 📦 View Changed
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 4.22 kB ℹ️ View Unchanged
|
kevin-dp
changed the title
| knownGaps: [`config-object-input`], | ||
| // The config-object gap is now closed here: the observer starts sync on the | ||
| // resolved collection regardless of the config path, so a bare `{ query }` | ||
| // syncs. (PR #1638 also fixes the source path directly.) |
There was a problem hiding this comment.
This comment is not needed. It only makes sense with the scope of this PR but no longer makes sense when we will merge it into main.
There was a problem hiding this comment.
Removed in cf92efd — the comment was scoped to this PR and wouldn't make sense on main.
| knownGaps: [`config-object-input`], | ||
| // The config-object gap is now closed here: the observer starts sync on the | ||
| // resolved collection regardless of the config path, so a bare `{ query }` | ||
| // syncs. (PR #1638 also fixes the source path directly.) |
There was a problem hiding this comment.
let's remove this comment, it only makes sense within this PR but in main it won't make sense anymore.
There was a problem hiding this comment.
Removed in cf92efd — the comment was scoped to this PR and wouldn't make sense on main.
kevin-dp
force-pushed
the
refactor/extract-adapter-helpers
branch
from
92f19fa to
84895f6
Compare
kevin-dp
force-pushed
the
refactor/live-query-observer
branch
from
4c86c7f to
a7e3c5e
Compare
kevin-dp
force-pushed
the
refactor/extract-adapter-helpers
branch
from
84895f6 to
84f4e93
Compare
kevin-dp
force-pushed
the
refactor/live-query-observer
branch
2 times, most recently
from
3b3c92e to
cf92efd
Compare
kevin-dp
force-pushed
the
refactor/extract-adapter-helpers
branch
from
84f4e93 to
4cf25ae
Compare
kevin-dp
force-pushed
the
refactor/live-query-observer
branch
from
cf92efd to
68ea1ca
Compare
Code reviewFound 1 issue:
db/packages/db/src/live-query-observer.ts Lines 178 to 184 in 68ea1ca db/packages/db/src/collection/lifecycle.ts Lines 282 to 289 in 68ea1ca db/packages/db/src/collection/lifecycle.ts Lines 149 to 151 in 68ea1ca I red-tested this locally with a temporary observer test on this branch: after subscribing, unsubscribing before first ready, subscribing again, and then calling Suggested fix: either make 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
Good catch — reproduced it exactly (subscribe → unsubscribe-before-ready → subscribe → |
Add createLiveQueryObserver to @tanstack/db. Given a resolved collection (or null for disabled), it owns the shared lifecycle: start sync, subscribe with initial state, the loading→ready notify, a stable per-revision snapshot for wholesale consumers, and delivery of the raw ChangeMessage[] for granular consumers (deferInitialNotify defers the initial notify for useSyncExternalStore consumers like React). React, Vue, Svelte, Solid, and Angular all materialize from the observer, removing their duplicated subscribe/status/ready-race plumbing while keeping native reactivity: Vue/Svelte/Solid apply the change deltas granularly to their reactive maps; React/Angular consume the snapshot wholesale. Observer unit tests cover the wholesale and granular paths, disabled, deferred-notify, and dispose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
onFirstReady returns no unsubscribe and detach() couldn't remove it, so a subscribe → unsubscribe-before-ready → subscribe sequence left a stale ready callback that also fired on markReady — the current listener saw two synthetic ready notifications instead of one. Guard the callback with an attach-generation token so only the current attachment's callback notifies. Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kevin-dp
force-pushed
the
refactor/live-query-observer
branch
from
8ee8e8e to
e957121
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: 5
🧹 Nitpick comments (3)
packages/db/src/live-query-observer.ts (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the collection generic instead of exposing
any.The observer API can use
Collection<T, TKey>here; the default utility generic keeps the API precise without leakingany.Proposed typing cleanup
- collection: Collection<T, TKey, any> | undefined + collection: Collection<T, TKey> | undefined ... - private readonly collection: Collection<T, TKey, any> | null + private readonly collection: Collection<T, TKey> | null ... - collection: Collection<T, TKey, any> | null, + collection: Collection<T, TKey> | null, ... - collection: Collection<T, TKey, any> | null | undefined, + collection: Collection<T, TKey> | null | undefined,As per coding guidelines, “Avoid using
anytypes; useunknowninstead when the type is truly unknown, and provide proper type annotations for return values.”Also applies to: 86-100, 248-253
🤖 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/db/src/live-query-observer.ts` at line 24, The observer typings are leaking `any` through the collection generic, so tighten the `LiveQueryObserver` API to use `Collection<T, TKey>` (or the existing default utility generic) instead of `Collection<T, TKey, any>`. Update the `collection` field and the related observer methods/types in `live-query-observer` that reference this shape, including the other affected spots noted in the diff, so the generic stays precise without exposing `any`.Source: Coding guidelines
packages/db/tests/live-query-observer.test.ts (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
as anycasts from observer tests.These casts hide whether the new public factory accepts the collection types it is expected to support.
As per coding guidelines, “Avoid using
anytypes; useunknowninstead when the type is truly unknown.”Also applies to: 60-60, 92-92, 121-123, 138-138
🤖 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/db/tests/live-query-observer.test.ts` at line 45, The observer tests are masking the real type compatibility of createLiveQueryObserver by casting sources to any; update the affected test cases to pass properly typed collection/source values instead of any so the public factory is exercised against its supported collection types. Use the existing test helpers and types around createLiveQueryObserver, makeSource, and the other affected observer assertions to infer the correct typings, and replace any remaining any casts in the referenced test blocks with explicit, appropriate types.Source: Coding guidelines
packages/react-db/src/useLiveQuery.ts (1)
435-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid erasing the hook return type with
any.The new
as anybypasses the overload contracts at the return boundary. Please cast to the hook’s concrete result type or add a typed adapter fromLiveQuerySnapshotto the existing public return shape. As per coding guidelines, “Avoid usinganytypes” and “Always provide the most precise return type annotation.”🤖 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/react-db/src/useLiveQuery.ts` around lines 435 - 439, The return in useLiveQuery is erasing the hook’s type contract with as any, which bypasses the overload guarantees. Update the useSyncExternalStore result to preserve the concrete hook return type by casting to the existing public snapshot/result type or introducing a typed adapter from LiveQuerySnapshot, and keep the return aligned with the function’s declared overloads instead of using any.Source: Coding guidelines
🤖 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/db/src/live-query-observer.ts`:
- Around line 112-115: The snapshot cache in `LiveQueryObserver.getSnapshot()`
can stay stale when `preload()` or a status-only transition makes the collection
ready without bumping `version`. Update the cache invalidation logic so
`preload()` and any readiness/status changes also force a cached snapshot
refresh, not just version changes. Use the existing `cachedVersion`, `version`,
and `getSnapshot()` flow, and make sure the invalidation also covers the related
status transition path referenced in the observer.
- Around line 199-204: The deferred initial notification flush in
LiveQueryObserver can still emit stale changes after a
subscribe/unsubscribe/resubscribe cycle. Update the deferInitialNotify path in
live-query-observer’s attach logic to associate the queued microtask with the
specific attach instance (for example via an incrementing attach token or
generation) and skip flushing if a newer attach has superseded it or the
observer was detached. Ensure the guard is checked before emitting deferred
changes in the queueMicrotask callback.
In `@packages/db/tests/live-query-observer.test.ts`:
- Around line 120-155: Add regression coverage for the remaining lifecycle races
in createLiveQueryObserver: extend the existing observer tests to reproduce a
deferInitialNotify flow where a subscription is unsubscribed and then
resubscribed before the microtask flush, and verify only the latest subscription
receives the deferred notify. Also add a test around getSnapshot() and preload()
that calls getSnapshot() before preload(), then asserts a ready snapshot is
returned after preload() completes. Use the existing createLiveQueryObserver,
subscribe, getSnapshot, and preload behavior to keep the tests aligned with the
bug scenarios.
In `@packages/react-db/src/useLiveQuery.ts`:
- Around line 414-423: Move the observer teardown out of the render path in
useLiveQuery: the needsNewCollection branch should not call
observerRef.current?.dispose() during render, because that can tear down the
committed subscription before React finishes updating. Update useLiveQuery to
perform observer disposal/recreation in a commit-phase effect or cleanup, and
add separate state/flag tracking for initialization so disabled queries do not
recreate the observer on every render when collectionRef.current remains null.
In `@packages/solid-db/src/useLiveQuery.ts`:
- Around line 393-397: Clear the existing Solid state before wiring up the new
observer in useLiveQuery so stale rows from the previous collection are removed
when collection() changes. Add the reset immediately before
createLiveQueryObserver/currentCollection subscription setup, and keep the logic
localized around observer.subscribe so includeInitialState only repopulates the
new collection instead of leaving old keys behind.
---
Nitpick comments:
In `@packages/db/src/live-query-observer.ts`:
- Line 24: The observer typings are leaking `any` through the collection
generic, so tighten the `LiveQueryObserver` API to use `Collection<T, TKey>` (or
the existing default utility generic) instead of `Collection<T, TKey, any>`.
Update the `collection` field and the related observer methods/types in
`live-query-observer` that reference this shape, including the other affected
spots noted in the diff, so the generic stays precise without exposing `any`.
In `@packages/db/tests/live-query-observer.test.ts`:
- Line 45: The observer tests are masking the real type compatibility of
createLiveQueryObserver by casting sources to any; update the affected test
cases to pass properly typed collection/source values instead of any so the
public factory is exercised against its supported collection types. Use the
existing test helpers and types around createLiveQueryObserver, makeSource, and
the other affected observer assertions to infer the correct typings, and replace
any remaining any casts in the referenced test blocks with explicit, appropriate
types.
In `@packages/react-db/src/useLiveQuery.ts`:
- Around line 435-439: The return in useLiveQuery is erasing the hook’s type
contract with as any, which bypasses the overload guarantees. Update the
useSyncExternalStore result to preserve the concrete hook return type by casting
to the existing public snapshot/result type or introducing a typed adapter from
LiveQuerySnapshot, and keep the return aligned with the function’s declared
overloads instead of using any.
🪄 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: a2f170d5-edf2-466d-b219-bdc647b51052
📒 Files selected for processing (10)
.changeset/live-query-observer.mdpackages/angular-db/src/index.tspackages/angular-db/tests/conformance.test.tspackages/db/src/index.tspackages/db/src/live-query-observer.tspackages/db/tests/live-query-observer.test.tspackages/react-db/src/useLiveQuery.tspackages/solid-db/src/useLiveQuery.tspackages/svelte-db/src/useLiveQuery.svelte.tspackages/vue-db/src/useLiveQuery.ts
- observer: getSnapshot() rebuilds when collection.status changes without a version bump (status-only loading→ready / preload with no active subscription), so a cached snapshot can't go stale. - observer: guard the deferred initial-notify microtask with the attach generation + listener count, so a superseded attach can't flush a stale initial batch to a later listener. - react: don't dispose the previous observer during render (unsafe under concurrent rendering) — useSyncExternalStore detaches it when the subscribe changes; dispose the current observer in an unmount effect instead. - tests: regressions for the deferred-notify race and the status-only snapshot refresh (both verified red before the fixes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/react-db/src/useLiveQuery.ts`:
- Around line 428-434: The unmount cleanup in useLiveQuery’s observer lifecycle
is disposing the shared observer too early, which can leave observerRef.current
pointing to a disposed instance during StrictMode/offscreen replay. Update the
useEffect cleanup to avoid disposing the observer there, or ensure the observer
is recreated before reuse when attach() is called on a disposed instance. Keep
the fix localized to useLiveQuery and the observerRef/useSyncExternalStore
subscription path so the next subscribe always gets a live observer.
🪄 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: a7b1d806-2a37-4fb2-b0ff-7b3a757303ed
📒 Files selected for processing (3)
packages/db/src/live-query-observer.tspackages/db/tests/live-query-observer.test.tspackages/react-db/src/useLiveQuery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/live-query-observer.ts
…tMode) The unmount-effect dispose could run during StrictMode/offscreen effect replay (mount → cleanup → mount) without a re-render, leaving observerRef pointing at a disposed observer; the next subscribe hit attach()'s disposed guard and the store stopped resubscribing. Remove the explicit dispose — useSyncExternalStore already detaches the observer on unsubscribe/unmount, so the collection subscription is torn down and the observer is GC'd. Adds a StrictMode regression test (verified red before the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expose the keyed `state` map in the shared conformance harness (added to ConformanceResult and read by all five adapter drivers) and add a steady-state `recompile-drops-stale-keys` scenario asserting the map stays in sync with `data` across a narrowing recompile. Also add a solid-db regression (in useLiveQuery.test.tsx) that inspects `state` synchronously in the window after a recompile, where solid-db leaks the previous collection's keys until its async resource reconciles. This test fails until the follow-up fix (state.clear() before re-subscribing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When the query recompiles to a different collection, the observer re-seeds via `includeInitialState`, which only inserts current rows and never deletes keys from the previous collection. Without clearing first, the dropped keys lingered in `state` until the async resource reconciled — a transient window where `state` exposed stale rows (though `data`, rebuilt wholesale, stayed correct). Clear synchronously before re-subscribing, matching vue-db and svelte-db. Fixes the solid-db stale-keys regression added in the previous commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kevin-dp
force-pushed
the
refactor/live-query-observer
branch
from
557c4a0 to
2a0679e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/solid-db/tests/useLiveQuery.test.tsx (1)
526-568: 🧹 Nitpick | 🔵 TrivialLGTM!
The test correctly validates the synchronous state-clearing fix —
setMinAge(32)triggers the effect synchronously (outside any batch),state.clear()runs before the observer re-seeds viaincludeInitialState, and the assertions readstatewith no settle. Thedispose()call properly triggersonCleanupwhich disposes the observer.Optional nit: adding
expect(rendered.result.state.has('3')).toBe(true)alongside thehas('1')/has('2')checks would make the positive case explicit and self-documenting, rather than relying on elimination fromsize === 1.✨ Optional: assert the surviving key explicitly
expect(rendered.result.state.size).toBe(1) expect(rendered.result.state.has(`1`)).toBe(false) expect(rendered.result.state.has(`2`)).toBe(false) + expect(rendered.result.state.has(`3`)).toBe(true)🤖 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-db/tests/useLiveQuery.test.tsx` around lines 526 - 568, Add an explicit positive assertion in the stale-key narrowing test after setMinAge(32), verifying rendered.result.state.has(`3`) is true alongside the existing size and removed-key assertions.packages/db/tests/conformance/contract.ts (1)
77-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
unknowninstead ofanyin the new contract property.This new field represents intentionally unknown key/value types. Prefer
ReadonlyMap<unknown, unknown> | undefined; this preserves type safety without weakening the conformance contract.As per coding guidelines, TypeScript code should avoid
anyand useunknownwhen the type is truly unknown.🤖 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/db/tests/conformance/contract.ts` around lines 77 - 82, Update the new contract property state in the conformance contract from ReadonlyMap<any, any> | undefined to ReadonlyMap<unknown, unknown> | undefined, preserving its optional semantics and existing documentation.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/db/tests/conformance/contract.ts`:
- Around line 77-82: Update the new contract property state in the conformance
contract from ReadonlyMap<any, any> | undefined to ReadonlyMap<unknown, unknown>
| undefined, preserving its optional semantics and existing documentation.
In `@packages/solid-db/tests/useLiveQuery.test.tsx`:
- Around line 526-568: Add an explicit positive assertion in the stale-key
narrowing test after setMinAge(32), verifying rendered.result.state.has(`3`) is
true alongside the existing size and removed-key assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e153c660-9ffb-4f29-b1ec-34a25edff3ec
📒 Files selected for processing (9)
packages/angular-db/tests/conformance.test.tspackages/db/tests/conformance/contract.tspackages/db/tests/conformance/suite.tspackages/react-db/tests/conformance.test.tsxpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/conformance.test.tsxpackages/solid-db/tests/useLiveQuery.test.tsxpackages/svelte-db/tests/conformance.svelte.test.tspackages/vue-db/tests/conformance.test.ts
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
The shared observer is a good direction, but focused regression tests against 2a0679e0 uncovered several lifecycle and semantic-clock defects that the existing conformance suite does not cover. I’m requesting changes because every migrated adapter now depends on these foundations.
Blocking findings
1. Bootstrap delivery is being counted as semantic state revisions
A loading empty collection currently emits an initial []; markReady() then emits undefined followed by another []. One readiness transition therefore produces two post-bootstrap revisions. Unsubscribe/resubscribe also creates a new snapshot when data and status are unchanged.
Failing tests observed:
expect(events).toEqual([[], undefined, []]) // actual sequence
expect(postReadyNotifications).toBe(1) // failed: received 2
const before = observer.getSnapshot()
observer.subscribe(() => {})()
observer.subscribe(() => {})()
expect(observer.getSnapshot()).toBe(before) // failedPlease separate per-subscriber bootstrap delivery from observer-wide semantic revisions, coalesce readiness through one canonical path, and preserve snapshot identity when observable state has not changed.
2. Status-only transitions do not wake consumers
The observer consumes row changes and onFirstReady, but not the collection’s status event channel. A mounted consumer can therefore remain on loading or ready after error/cleaned-up until an unrelated row event occurs. Repeated attach/detach while loading also accumulates non-removable onFirstReady callbacks.
Failing tests observed:
collection._lifecycle.setStatus('error')
collection._lifecycle.setStatus('cleaned-up')
expect(statuses).toContain('error') // failed
expect(statuses).toContain('cleaned-up') // failed
expect(onFirstReadyCallbacks.size).toBe(0) // failed: 3 retained after 3 cyclesPlease make status events part of the canonical publication path and avoid permanent readiness callbacks. The RFC also requires carrying the actual error as unknown, rather than exposing only isError; that appears to require plumbing the originating error through collection lifecycle state.
3. Snapshot freshness and identity are incorrect while detached
Rows changed while detached can leave the cached snapshot stale, while reattachment manufactures revisions even without an observable change.
const unsubscribe = observer.subscribe(() => {})
const before = observer.getSnapshot()
unsubscribe()
mutateCollectionWithoutChangingStatus()
const after = observer.getSnapshot()
expect(after).not.toBe(before) // failed
expect(after.data).toEqual(expected) // failed: stale rowsThe semantic clock needs either a collection-owned observable revision or a canonical observer materialization retained for the observer lifetime. Attachment replay itself should not advance that clock.
4. Dispatch is reentrant and subscription mutation affects in-flight delivery
Direct Set.forEach dispatch allows nested events to overtake outer events. In a focused regression, listener A synchronously deleted a row while handling its insert; listener B received delete before insert and could finish inconsistent with the collection.
The following also reproduced: a listener added during dispatch received the event in progress; a removed listener missed the event in progress; and two logical subscriptions using the same callback broke teardown ownership.
expect(listenerBEvents).toEqual(['insert', 'delete'])
// failed: ['delete', 'insert']
expect(listenerAddedDuringDispatchCalls).toBe(0) // failed
expect(existingListenerCalls).toBe(1) // failed after in-flight removal
expect(source.subscriberCount).toBe(1) // failed for duplicate callbackPlease queue nested publications FIFO, dispatch each publication over a snapshot of subscription records, and identify logical subscriptions independently of callback identity.
5. Synchronous initial replay can leak the underlying subscription
subscribeChanges() invokes initial delivery before returning, so a listener can dispose the observer while collectionUnsub is still unset. The returned collection subscription is then retained after disposal.
observer.subscribe(() => observer.dispose())
expect(source.subscriberCount).toBe(0) // failed: 1Please make attachment transactional or immediately unsubscribe if disposal occurred before subscribeChanges() returned.
6. Deferred initial notification can reorder events
A same-tick update can be delivered before the older deferred initial batch.
expect(events).toEqual(['v1', 'v2'])
// failed: ['v2', 'v1']Please queue subsequent deltas until the deferred bootstrap delivery has flushed.
7. Granular subscriber semantics are incomplete
A second concurrent subscriber does not receive current rows, and subscribe() after dispose() silently registers a listener that can never be called.
expect(secondSubscriberKeys).toEqual(['1', '2']) // failed: []
expect(() => observer.subscribe(listener)).toThrow() // failed after disposePlease seed every new granular subscriber without advancing the semantic revision, and either reject subscriptions after disposal or explicitly support revival.
8. Observer and React collection resolution perform render-time activation
Construction calls startSyncImmediate(). React also starts direct and callback-returned collections during render, while generated collections use startSync: true. An abandoned concurrent render can therefore activate resources without a committed consumer.
const source = makeIdleSource()
createLiveQueryObserver(source)
expect(source.status).toBe('idle') // failed: loadingCode inspection confirms the same side effect in all three React resolution paths. This conflicts with the RFC requirement that render be inert. Please activate synchronization through committed retain/subscription ownership instead.
9. Forced initial state introduces unfiltered loading and unnecessary materialization
attach() unconditionally requests initial state, which reproduced an unfiltered load:
expect(loadSubset).not.toHaveBeenCalledWith({ where: undefined })
// failed: loadSubset({ where: undefined })This changes React/Angular behavior. Separately, a granular listener reading only getSnapshot().status caused a full collection entries() enumeration for a one-row delta:
expect(entriesCalls).toBe(0) // failed: 1Please preserve the adapters’ previous initial-loading policy (or make it explicit/configurable) and avoid materializing all entries when only status is requested.
10. Solid can resurrect superseded async state
After await currentCollection.toArrayWhenReady(), the Solid resource continuation writes keyed state/data/status without checking whether that collection has been superseded. The error continuation has the same issue.
An executable source assertion found unguarded post-await writes; adding a generation check to success and error continuations passed Solid typechecking. Please generation-guard both paths.
11. API and release-note presentation do not match the RFC
The RFC says the observer is for official adapters and may cross package boundaries only as an internal/unstable surface, not as a documented third-party extension point. The PR root-exports it and the changeset advertises createLiveQueryObserver as a minor feature. The changeset also says “No behavior change,” contradicted by the initial-load and Angular behavior changes above.
Please mark/use an internal or unstable surface and update the changeset accordingly. Removing the cross-package export entirely is not necessary if official adapters require it.
Non-blocking cleanup
I would not block this PR on these, but the migration also leaves redundant adapter synchronization, a copied insert/update/delete switch across three adapters, redundant Vue/Svelte teardown, and an unused LiveQueryObserver.preload().
Verification
These findings were tested in fresh worktrees at PR head 2a0679e0 after pnpm install.
- One regression group initially produced 9/9 failing behavioral assertions; minimal local fixes made the focused observer suite pass 17/17.
- The semantic-clock/reentrancy group initially produced 5/5 failing focused tests; minimal local fixes made that suite pass 13/13.
- Solid typechecking passed after locally generation-guarding its async continuations.
- Adapter builds and focused conformance tests passed after the verification-only fixes.
No verification changes were committed or pushed.
I would not block this PR on phase-four ordered snapshots, the final patch model, or the complete DbClient/SSR ownership architecture. I would block on exactly-once semantic publication, status/error delivery, snapshot freshness and identity, FIFO non-reentrant dispatch, balanced attachment/teardown, event ordering, and render-inert activation.
2 participants
RFC #1623 step 3 — the shared observer that unifies the live-query lifecycle, now adopted by all five adapters. This is where the duplicated lifecycle across adapters actually collapses.
Base: stacked on
refactor/extract-adapter-helpers(#1641, step 2) → conformance suite (#1636). Re-target down the stack as each lands.The observer
createLiveQueryObserver(collection | null, { deferInitialNotify? })in@tanstack/db. Given a resolved live-query collection (ornullfor disabled), it owns everything the five adapters used to each re-implement:includeInitialState, so consumers get initial rows + deltas through one aligned channel)onFirstReady, de-duplicated)getSnapshot()— reference-stable foruseSyncExternalStore)ChangeMessage[]to subscribers for granular consumersThe last two are the item-6 decision made concrete: the observer carries both a snapshot and the change set, so Vue/Svelte/Solid keep fine-grained keyed-map updates while React/Angular consume the snapshot.
Input resolution stays in each adapter (query fn / config / collection / disabled) — it's framework-reactive and fixed separately (#1637/#1638). The observer owns everything after the input resolves.
All five adapters migrated
Each keeps its native reactivity; the duplicated subscribe/
onFirstReady/status/ready-race plumbing is gone:useSyncExternalStore(observer.subscribe, observer.getSnapshot)— opts intodeferInitialNotifyChangeMessage[]deltas to itsreactivemapSvelteMap(runes)ReactiveMap; keepscreateResource/Suspense +reconcileTwo contract refinements the migrations forced (both interesting)
includeInitialStateis required, not optional. Seeding a granular adapter's map fromgetSnapshot()and subscribing without initial state desyncs the collection's per-subscriber change stream — deletes arrived as empty batches. The observer must subscribe with initial state so initial rows + deltas flow through one channel.deferInitialNotifyis per-consumer. React'suseSyncExternalStoremust not get a synchronous notify duringsubscribe, so React opts into deferring the initial notify to a microtask. Effect/watcher adapters (Svelte reads synchronously afterflushSync) want it synchronous, which is the default.Bonus
Angular's
config-object-inputconformance gap closes here as a side effect — the observer starts sync on the resolved collection regardless of the config path. (PR #1638 still fixes the source path directly.)Verification
All green:
@tanstack/db2461, react 119, vue 54, svelte 54, solid 62, angular 50 (+1 todo) — including the cross-adapter conformance suite that guards every migration. Observer unit tests cover both wholesale and granular paths, disabled, deferred-notify, and dispose. Minor changeset for db, patch for all five adapters.🤖 Generated with Claude Code
Summary by CodeRabbit
createLiveQueryObserver) exposed from the main DB package.stateand added a new “recompile drops stale keys” scenario.