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 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a shared ChangesShared live-query observer migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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.6 kB (+1.28%) Total Size: 127 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.
92f19fa to
84895f6
Compare
4c86c7f to
a7e3c5e
Compare
84895f6 to
84f4e93
Compare
3b3c92e to
cf92efd
Compare
84f4e93 to
4cf25ae
Compare
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>
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>
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.
A listener that synchronously mutates the collection used to trigger a nested, reentrant dispatch: later subscribers could observe the nested event (e.g. a delete) before the outer one (the insert) it reacted to. Publications are now queued and dispatched FIFO. Each publication is delivered over a snapshot of subscription records taken when it is dispatched: a subscription removed mid-delivery still receives the in-flight publication, one added mid-delivery does not. Records — not raw callbacks — identify subscriptions, so subscribing the same function twice no longer collapses into one Set entry whose first unsubscribe tore down both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l replay subscribeChanges delivers the initial state synchronously, so a listener could dispose the observer before the subscription handle was stored — detach() then had nothing to release and the collection subscription leaked past disposal. The release hook is now registered before the subscription is created, making attachment transactional: if detach() fired mid-replay, the subscription is undone as soon as subscribeChanges returns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The initial-state replay only happened on the first attach, so a second concurrent subscriber started with no rows and could never converge — its keyed map silently stayed empty. A subscriber arriving while the observer is already attached is now seeded with the collection's current rows as inserts, delivered to that subscription alone without advancing the observer revision. subscribe() after dispose() used to register a listener that could never fire; it now throws LiveQueryObserverDisposedError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The observer counted every delivery — including per-attach bootstrap replays and empty ready flushes — as a semantic revision. One readiness transition published three times ([], undefined, []), a plain unsubscribe/resubscribe manufactured a new snapshot identity with unchanged data, and rows committed while nothing was attached left the cached snapshot stale. The semantic clock now lives on the collection: emitEvents advances a monotonic stateRevision once per committed batch, whether or not anyone is subscribed. getSnapshot keys its cache on (stateRevision, status), so detached snapshots stay fresh and attachment replay can no longer advance the clock. Empty change batches are dropped from publication — only real deltas and the synthetic ready notify go out — so a readiness transition publishes exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…contract
The hand-rolled mock notified subscribers with empty change batches as a
wake-up signal — something real collections never do — and lacked the
state revision and status event channel the observer relies on. It now
advances _stateRevision on committed changes, emits real delete/insert
deltas from __replaceAll, and publishes status transitions through
on('status:change') instead of an empty notify.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The observer consumed row changes and onFirstReady but not the
collection's status events: a mounted consumer could sit on a stale
loading/ready status after an error or cleaned-up transition until an
unrelated row event happened to arrive. Status changes now publish a
synthetic notify through the same canonical path as data changes.
This also retires the onFirstReady registration, whose callbacks could
not be unsubscribed and accumulated across attach/detach cycles while
loading — collection.on('status:change') returns a real unsubscribe that
detach releases.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion
Forcing includeInitialState on every attach was a behavior change for
the wholesale adapters: React and Angular never requested an initial
snapshot before the observer, and the forced request issued an
unfiltered loadSubset({ where: undefined }) against on-demand
collections. The observer now takes a mode option: granular (default —
Vue/Svelte/Solid) keeps the initial-state subscription and late-
subscriber seeding; wholesale (React/Angular) subscribes with
includeInitialState: false, restoring the pre-observer loading policy
while deletes still flow through as notifies.
getSnapshot() now materializes rows lazily on first state/data access,
so a consumer that only reads status never enumerates the collection.
The React already-ready microtask notify is gone with the bootstrap
replay; it existed because the pre-observer per-subscription version
could miss a ready transition between render and subscribe, which the
collection-owned revision plus useSyncExternalStore's post-subscribe
re-read now cover.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ction The deferred initial notify could be overtaken by a same-tick delta: the bootstrap batch waited in a microtask while later changes emitted synchronously, so a granular consumer could see v2 before v1. The mechanism existed solely so React's useSyncExternalStore was not notified during its own subscribe call. With React on wholesale mode there is no bootstrap replay to defer — nothing is delivered synchronously during a wholesale subscribe — so the deferral, its attach-generation guard, and the reordering hazard are all removed. Every publication is now delivered synchronously in commit order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ubscribe Constructing an observer called startSyncImmediate(), so building one in a render that is later abandoned (React concurrent rendering) activated sync with no committed consumer. Construction is now side-effect-free: activation happens through the first subscription's own addSubscriber path — the identical startSync call — after the status listener is wired, so the loading/ready transitions of a synchronously-starting collection are observed and published instead of happening silently before anyone listens. The adapters' behavior is unchanged: React's input-resolution paths start sync in render themselves (pre-existing, unchanged here), and the effect-based adapters subscribe in the same tick they construct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Solid discards a superseded fetch's return value, but the fetcher's post-await writes are side effects into hook-scoped state: switching collections while toArrayWhenReady() was pending let the old continuation resurrect the replaced collection's rows and status over the new one's. Both the success and error continuations now check a generation counter and no-op when superseded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The observer is a contract for TanStack DB's official adapters, not a public extension point — the exported factory and interface now say so (@internal, may change in any release). The changeset drops the false "No behavior change" claim and describes the lifecycle fixes and the per-adapter loading-policy preservation instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Hi @KyleAMathews, i addressed your review. Here's the full breakdown of what was addressed and what was deliberately left out. Fixed in this PR1 + 3 — Exactly-once semantic publication; snapshot identity and freshness. The semantic clock is now collection-owned: 2a — Status delivery. Status transitions now publish through 4 — Dispatch. Publications are queued and dispatched FIFO — a listener that synchronously mutates the collection can no longer cause a nested event to overtake the outer one. Each publication is delivered over a snapshot of subscription records taken at dispatch: removed-mid-delivery still receives the in-flight publication, added-mid-delivery does not. Subscriptions are identified by record, not callback, so the duplicate-callback teardown break is fixed. 5 — Attachment is transactional. The release hook is registered before 6 — Event ordering. 7 — Subscriber semantics. A subscriber arriving while already attached is seeded with the current rows as inserts, delivered to that subscription alone without advancing the clock. 8 — Render-inert activation (observer half). Observer construction is now fully side-effect-free — your 9 — Initial-loading policy and materialization. The observer takes a 10 — Solid. Both the success and error continuations after 11 — Surface and changeset. Each finding has regression tests ( Deliberately not done, and why2b — Carrying the actual error as 8 — React's render-path activation. The observer half is fixed (above), but React's input resolution still calls Non-blocking cleanups (the copied insert/update/delete switch across granular adapters, redundant teardown, unused |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/angular-db/tests/inject-live-query.test.ts (2)
84-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
anytypes withunknown.As per coding guidelines, avoid using
anytypes; useunknowninstead when the type is truly unknown.
packages/angular-db/tests/inject-live-query.test.ts#L84-L86: replaceArray<any>andanywithArray<unknown>andunknowninsubsandstatusSubstype parameters.packages/angular-db/tests/inject-live-query.test.ts#L91-L91: replaceArray<any>withArray<unknown>in thenotifyparameter type.packages/angular-db/tests/inject-live-query.test.ts#L114-L117: replaceanywithunknownin thecbparameter type ofon.packages/angular-db/tests/inject-live-query.test.ts#L123-L126: replaceArray<any>withArray<unknown>in thecbparameter type ofsubscribeChanges.🤖 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-db/tests/inject-live-query.test.ts` around lines 84 - 86, Replace all any usages in the live-query test callbacks with unknown: update subs and statusSubs, notify, on, and subscribeChanges. Apply the changes at packages/angular-db/tests/inject-live-query.test.ts lines 84-86, 91, 114-117, and 123-126, using Array<unknown> for collections and unknown for callback event parameters.Source: Coding guidelines
157-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer array methods and spread operator over manual loops with push.
As per coding guidelines, use array methods for transformations and the spread operator for combining arrays instead of manual loops with
push.♻️ Proposed refactor
- __replaceAll: (rows: Array<T & Record<`id`, K>>) => { - const changes: Array<any> = [] - for (const [key, value] of map.entries()) { - changes.push({ type: `delete`, key, value }) - } - map.clear() - for (const r of rows) { - map.set(r.id, r) - changes.push({ type: `insert`, key: r.id, value: r }) - } - notify(changes) - }, + __replaceAll: (rows: Array<T & Record<`id`, K>>) => { + const deletes = Array.from(map.entries()).map(([key, value]) => ({ type: `delete`, key, value })) + map.clear() + const inserts = rows.map((r) => { + map.set(r.id, r) + return { type: `insert`, key: r.id, value: r } + }) + notify([...deletes, ...inserts]) + },🤖 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-db/tests/inject-live-query.test.ts` around lines 157 - 167, Refactor __replaceAll to eliminate the manual changes loop and push calls: derive delete changes from map entries with an array method, clear the map, then combine those changes with insert changes derived from rows using spread and array methods before calling notify(changes). Preserve the existing deletion, replacement, and insertion order.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/angular-db/tests/inject-live-query.test.ts`:
- Around line 84-86: Replace all any usages in the live-query test callbacks
with unknown: update subs and statusSubs, notify, on, and subscribeChanges.
Apply the changes at packages/angular-db/tests/inject-live-query.test.ts lines
84-86, 91, 114-117, and 123-126, using Array<unknown> for collections and
unknown for callback event parameters.
- Around line 157-167: Refactor __replaceAll to eliminate the manual changes
loop and push calls: derive delete changes from map entries with an array
method, clear the map, then combine those changes with insert changes derived
from rows using spread and array methods before calling notify(changes).
Preserve the existing deletion, replacement, and insertion order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e9f806c-7f69-4aa4-88fe-8b49954eac15
📒 Files selected for processing (12)
.changeset/live-query-observer.mdpackages/angular-db/src/index.tspackages/angular-db/tests/inject-live-query.test.tspackages/db/src/collection/changes.tspackages/db/src/collection/index.tspackages/db/src/errors.tspackages/db/src/live-query-observer.tspackages/db/tests/live-query-observer.test.tspackages/react-db/src/useLiveQuery.tspackages/react-db/tests/useLiveQuery.eager-onstorechange.test.tsxpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/useLiveQuery.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/live-query-observer.md
- packages/angular-db/src/index.ts
- packages/react-db/src/useLiveQuery.ts
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
New Features
createLiveQueryObserver) available from the main DB package.statein adapter conformance results/handles (React, Vue, Svelte, Solid, Angular) and added a new conformance scenario.Bug Fixes
Tests