feat(db): ordered snapshot / layout-revision contract (RFC #1623 phase 4) - #1669
Conversation
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>
- 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>
…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>
…1623 phase 4) An `orderBy` live query that reorders its rows without changing any projected row value (an "order-only move") was swallowed by the collection's value-diff: `.values()`/`.entries()` re-sorted, but no change event fired, so subscribers kept the stale order. This is the last universal expected-fail in the cross-adapter conformance suite (issue #1601). Phase 4 of the live-query platform RFC calls for an explicit layout-revision contract rather than a forged row `update`. This does that: - The live-query flush captures the retracted side of each change and, after commit, detects an order-only move (value deep-equal, `orderByIndex` moved) and publishes a first-class empty layout-change notification via a new `CollectionChangesManager.emitLayoutChangeEvent()`. - The shared observer snapshot gains `layoutRevision`, which increments on any visible membership, ordering, or order-only-move change. All five adapters pick this up through their existing wholesale re-read, so the `order-only-move` conformance scenario is removed from UNIVERSAL_EXPECTED_FAIL and now passes on React, Vue, Svelte, Solid, and Angular. Distinct from PR #1601 (v-anton), which fixes the same bug via a forced row `update`; this uses the RFC's layout-revision approach instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughLive-query collections now track state and layout revisions, publish order-only changes, and expose layout-aware notifications. A framework-agnostic observer provides stable snapshots, subscription modes, lazy activation, queued delivery, preload, and disposal. Tests cover observer lifecycle, nested reordering, and stale-key removal. ChangesLive-query layout event propagation
Shared live-query observer
Order-move regression and release validation
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 💡 1📝 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: +692 B (+0.54%) Total Size: 128 kB 📦 View Changed
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.81 kB ℹ️ View Unchanged
|
Addresses independent review of the layoutRevision contract: - The join-with-separator signature could collide: a key value equal to the concatenation of neighboring keys around the separator produces the same string as two separate keys, so a real layout change (a membership change whose combined key spans the separator) was missed. Compare the ordered key sequence directly instead - collision-free, and it avoids materializing a large string on every snapshot rebuild (a new key array is only allocated when the layout actually moved). Adds a regression test. - Correct the layoutRevision doc comment: it is NOT in lockstep with snapshot identity (a value-only update yields a new snapshot but the same layoutRevision). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Code review
Found 2 issues, both reproduced with failing regression tests against d7628551f219cab27ddd542bffda40c8ee7f5aad:
- A graph flush containing both an ordinary projected-value update and an order-only move publishes twice.
commit()synchronously emits the ordinary row batch, thenemitLayoutChangeEvent()emits a second empty batch even though listeners already observe the final values and order. My regression expected one observer callback and received two. Please coalesce layout dirtiness into the commit publication (or otherwise suppress the separate layout event when that commit already publishes), and cover this with an exact-one-callback mixed-batch test.
db/packages/db/src/query/live/collection-config-builder.ts
Lines 815 to 830 in d762855
- The layout fix does not cover ordered child collections produced by
includes. On retract-then-insert, the insertion side replacesexisting.valuebut leaves the retractedorderByIndex; later the child collection commits without any layout-only signal. I reproduced this with a child projection that omits its sort field: after movingc1behindc2, the child collection remained ordered[c1, c2]instead of[c2, c1]. Please preserve the insertion-side order metadata, publish child layout-only changes through the same atomic mechanism, and add an ordered-includes regression.
db/packages/db/src/query/live/collection-config-builder.ts
Lines 886 to 918 in d762855
db/packages/db/src/query/live/collection-config-builder.ts
Lines 1977 to 2005 in d762855
Generated with Claude Code
Reproductions for the requested changesI verified both findings against head 1. Mixed value update + order-only move must publish exactly onceUsing the existing ordered query whose projection omits it(`publishes a mixed value update and order-only move exactly once`, async () => {
const source = makeSource()
const lq = await makeOrderedByAge(source)
const observer = createLiveQueryObserver<
{ id: string; name: string },
string
>(lq as any)
let notifications = 0
observer.subscribe(() => notifications++)
notifications = 0 // exclude subscribeChanges' initial-state publication
source.utils.begin()
source.utils.write({
type: `update`,
value: { id: `1`, name: `Alicia`, age: 30 },
})
source.utils.write({
type: `update`,
value: { id: `2`, name: `Bob`, age: 99 },
})
source.utils.commit()
const after = observer.getSnapshot()
expect((after.data as Array<Person>).map(({ id, name }) => [id, name])).toEqual([
[`1`, `Alicia`],
[`3`, `Carol`],
[`2`, `Bob`],
])
expect(notifications).toBe(1)
})Result on the PR head: The first callback comes from Relevant code: db/packages/db/src/query/live/collection-config-builder.ts Lines 815 to 830 in d762855 2. Ordered included child must consume the new order metadata and publish its moveit(`publishes an ordered included child move exactly once`, async () => {
const parents = createCollection(
mockSyncCollectionOptions<{ id: string }>({
id: `parents`,
getKey: ({ id }) => id,
initialData: [{ id: `p1` }],
}),
)
const children = createCollection(
mockSyncCollectionOptions<{
id: string
parentId: string
name: string
position: number
}>({
id: `children`,
getKey: ({ id }) => id,
initialData: [
{ id: `c1`, parentId: `p1`, name: `One`, position: 1 },
{ id: `c2`, parentId: `p1`, name: `Two`, position: 2 },
],
}),
)
const lq = createLiveQueryCollection((q) =>
q.from({ parent: parents }).select(({ parent }) => ({
id: parent.id,
children: q
.from({ child: children })
.where(({ child }) => eq(child.parentId, parent.id))
.orderBy(({ child }) => child.position)
.select(({ child }) => ({ id: child.id, name: child.name })),
})),
)
await lq.preload()
const childCollection = lq.get(`p1`)!.children
let notifications = 0
const subscription = childCollection.subscribeChanges(
() => notifications++,
{ includeInitialState: false },
)
children.utils.begin()
children.utils.write({
type: `update`,
value: { id: `c1`, parentId: `p1`, name: `One`, position: 3 },
})
children.utils.commit()
expect([...childCollection.values()].map(({ id }) => id)).toEqual([
`c2`,
`c1`,
])
expect(notifications).toBe(1)
subscription.unsubscribe()
})Result on the PR head: There are two gaps in this path:
Relevant code:
I also tested the stable-rank case ( |
Two gaps in the order-only-move handling, reproduced as failing tests (to be fixed in a follow-up commit): 1. A commit containing both an ordinary value update and an order-only move publishes twice (commit's row batch + the separate empty layout event), where exactly one publication is expected. 2. Ordered child collections produced by `includes` don't consume the insertion-side order metadata or publish a layout-only move, so an ordered child stays in its old order after a child order-only move. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dren
Addresses Kyle's review of the order-only-move handling:
1. A commit containing both an ordinary value update and an order-only move
published twice: commit() emitted the row batch and then the separate layout
event fired redundantly. Replace hasOrderOnlyMove with
needsLayoutOnlyPublication, which fires the layout event only when the commit
published nothing else (any real insert/delete/value-changed update already
notifies subscribers, who re-read the re-sorted collection).
2. Ordered child collections produced by includes did not reorder on an
order-only child move:
- The child accumulate replaced value on the insert side but left the
retracted orderByIndex, so the child collection re-sorted against a stale
index. Update orderByIndex on insert and capture the retract side (both the
single-level and nested-includes accumulate blocks).
- The child flush committed without a layout-only publication when the
projected child value was unchanged. Publish one through the same
mechanism (emitLayoutChange) when the child commit published nothing else.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The includes flush is recursive, so the order-only-move handling must hold beyond one level. Adds a two-level ordered-includes regression (org -> teams -> members): moving a grandchild whose projected value is unchanged must re-sort its collection and publish exactly once. Verified red when the child-flush layout publication is removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough review, Kyle — both findings were spot on. Fixed in c9ec751, with the two regressions you specified added first (43f1a7d) so the failure is on record. 1. Mixed value update + order-only move now publishes exactly once. Replaced 2. Ordered
Covered by the ordered-includes regression. Depth. Since Stable-rank case (sort field changes but position doesn't, value unchanged) needs no change, as you noted — it produces zero layout events; there's a test asserting that too. Full |
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>
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
[P1] Tie the layout revision to the sync transaction that changes the layout
This is reproducible on the current head, df9403a.
markLayoutChange() currently increments the collection-wide layoutRevision and sets pendingLayoutChange before the live-query result collection calls commit(). A normal sync commit is not always applied at that point: commitPendingTransactions() parks committed sync transactions while a user transaction on the same collection is persisting.
That lets the layout clock describe state that is not visible yet.
Deterministic reproduction
- Create a live query ordered by
age, but project only{ id, name }. - Read a detached observer snapshot. Its initial order is
[2, 1, 3]. - Start a non-optimistic mutation on the result collection and keep its
onUpdatehandler unresolved. The user transaction is nowpersisting. - Update source row
2so its age moves it to the end. The projected row value remains deep-equal. - The query pipeline calls
markLayoutChange(), advancinglayoutRevision, then commits its result-collection sync transaction. - The sync transaction is parked behind the persisting mutation, so the visible order is still
[2, 1, 3]. - A detached
getSnapshot()sees the new layout revision and caches the old entries against it. - Resolve the mutation. The parked sync transaction applies and the collection order becomes
[1, 3, 2]. - Because the projected values are deep-equal, no row event or state-revision increment occurs. The layout revision also does not advance again.
- The next detached snapshot sees neither revision change and remains stuck on
[2, 1, 3].
I added that regression locally. Before the fix it fails at the final assertion:
expected [ '2', '1', '3' ] to deeply equal [ '1', '3', '2' ]
There is a related attached-observer hazard: while the sync transaction is parked, an unrelated forced publication can consume the collection-wide pendingLayoutChange flag. The eventual order-only commit then has neither row events nor a layout marker, so it cannot send the corrective publication.
Requested fix
Please store layout dirtiness on the exact pending sync transaction that produced it, rather than on the collection:
interface PendingSyncedTransaction<...> {
// existing fields
layoutChanged: boolean
}_markLayoutChange() should mark the active, uncommitted sync transaction. When commitPendingTransactions() actually applies committed transactions, it should OR their layoutChanged flags and advance/publish the collection layout revision after applying state and immediately before emitting the final batch.
This preserves the required properties:
- The revision advances only when the matching reordered state becomes visible.
- The layout signal and row-event batch publish atomically.
- Multiple transactions applied in one flush coalesce into one layout publication.
- Unrelated optimistic recomputations cannot steal the marker.
- Cleanup naturally discards the marker with the pending transaction.
I verified this design locally: the new regression turns green, all 10 order-only layout tests pass, and the broader collection transaction suites pass (149 tests total, with no type errors). ESLint, Prettier, and git diff --check also pass.
The mixed membership/order batching and nested ordered-includes fixes on this head look correct. This transaction-boundary race is the remaining blocker.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/db/src/collection/sync.ts (1)
74-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
getActivePendingSyncTransaction()here.Lines 76-85 repeat the lookup and both guards already implemented by the private helper
getActivePendingSyncTransaction()at lines 292-306 of this file. The behavior is identical, including both error types. Reuse the helper so future changes to the guard logic apply to every caller.As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places".
♻️ Proposed refactor
/** Mark the active sync transaction as changing collection layout. */ public markLayoutChange(): void { - const pendingTransaction = - this.state.pendingSyncedTransactions[ - this.state.pendingSyncedTransactions.length - 1 - ] - if (!pendingTransaction) { - throw new NoPendingSyncTransactionWriteError() - } - if (pendingTransaction.committed) { - throw new SyncTransactionAlreadyCommittedWriteError() - } - - pendingTransaction.layoutChanged = true + this.getActivePendingSyncTransaction().layoutChanged = true }Note:
getActivePendingSyncTransactionis declared belowmarkLayoutChange. Method hoisting inside a class makes this call order safe.🤖 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/collection/sync.ts` around lines 74 - 88, Update markLayoutChange() to obtain the transaction through the existing private getActivePendingSyncTransaction() helper instead of duplicating the pending transaction lookup and committed-state guards, then set layoutChanged on the returned transaction while preserving the helper’s existing error behavior.Source: Coding guidelines
packages/db/src/query/live/collection-config-builder.ts (1)
905-912: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne multiplicity-accumulation rule is implemented three times in this file. Each site applies the same four rules to a
Changes<T>entry: adddeletes, recordpreviousValue/previousOrderByIndexon retract, addinsertsand replacevalueon insert, and overwriteorderByIndexonly when it is defined. This PR added the same twoprevious*lines to all three sites, so a future change to the order-only-move contract must land in three places; a missed site produces a silent layout-notification gap. Extract oneapplyMultiplicity<T>(existing, value, orderByIndex, multiplicity)helper and call it from every site.
packages/db/src/query/live/collection-config-builder.ts#L905-L912: replace the inline branches insetupIncludesOutputwithapplyMultiplicity(existing, childResult, _orderByIndex, multiplicity).packages/db/src/query/live/collection-config-builder.ts#L1337-L1344: replace the inline branches insetupNestedPipelineswith the same call.packages/db/src/query/live/collection-config-builder.ts#L2340-L2343: replace the retract and insert branches inaccumulateChangeswithapplyMultiplicity(changes, value, orderByIndex, multiplicity).As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places".
🤖 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/query/live/collection-config-builder.ts` around lines 905 - 912, The multiplicity accumulation logic is duplicated across three sites; extract a shared generic applyMultiplicity<T> helper that handles deletes, previousValue/previousOrderByIndex on retracts, inserts, value replacement, and defined orderByIndex updates. In packages/db/src/query/live/collection-config-builder.ts at lines 905-912 and 1337-1344, replace the inline branches with applyMultiplicity(existing, childResult, _orderByIndex, multiplicity); at lines 2340-2343, replace the retract/insert branches with applyMultiplicity(changes, value, orderByIndex, multiplicity), preserving all existing behavior.Source: Coding guidelines
packages/db/tests/live-query-observer.test.ts (1)
593-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd empty-source no-op coverage.
The order-only move and stable-rank assertions already exist. Add an empty-source test that asserts
datais[]andlayoutRevisionremains unchanged after a no-op commit.🤖 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` around lines 593 - 617, Add a test near the existing layoutRevision observer cases that creates an empty source, subscribes a live query observer, records its initial snapshot, performs a begin/commit cycle without writes, and asserts the snapshot data remains [] while layoutRevision is unchanged; dispose the observer afterward.Source: Coding guidelines
packages/db/tests/conformance/suite.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale section comment for
order-only-move.
UNIVERSAL_EXPECTED_FAILis now empty, but line 686 still labels the following section---- tail: universal expected-fail ---------------------------. Theorder-only-movescenario at line 688 is now an ordinary passing scenario. Update or remove that heading so readers do not treat the scenario as expected-fail.As per coding guidelines: "Keep comments that explain non-obvious behavior... remove outdated comments when refactoring".
🤖 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/suite.ts` at line 42, Update or remove the stale “universal expected-fail” section heading near the order-only-move scenario, since UNIVERSAL_EXPECTED_FAIL is empty and order-only-move is a normal passing scenario. Ensure the surrounding comment accurately describes the scenario’s current status.Source: Coding guidelines
packages/angular-db/src/index.ts (1)
261-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAngular reads the collection directly; other adapters read the observer snapshot.
syncDataFromCollectionreadscurrentCollection.entries(),values(), andstatuson every notify. Vue, Svelte, and Solid readobserver.getSnapshot().statusthroughsyncFromObserver(seepackages/vue-db/src/useLiveQuery.ts:371-377). The observer maintainsvisibleStatusand a captured entries view, so the two sources can report different statuses for the same publication. Consider readingobserver.getSnapshot()forstatus(and optionallystate/data) so all adapters share one status contract.🤖 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/src/index.ts` around lines 261 - 266, The Angular sync path currently reads collection state directly, unlike other adapters that use the observer snapshot. Update syncDataFromCollection and its call sites in the subscription flow to consume observer.getSnapshot() for status, and reuse snapshot state/data where applicable, preserving the post-subscribe synchronization behavior.packages/db/src/live-query-observer.ts (1)
495-500: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA throwing listener stops delivery for the whole batch.
subRecord.listener(publication.changes)runs unguarded. If one listener throws, the loop exits, the remaining targets never receive this publication, and publications still inpublicationQueuestay queued until the nextemit(). Framework listeners run user code (Vue/Svelte/Solid state updates), so a throw is reachable. Consider isolating each listener call and rethrowing after the queue drains.♻️ Suggested isolation
if (deliver) { + let deliveryError: unknown + let hasError = false for (const subRecord of publication.targets) { if (this.disposed) return - subRecord.listener(publication.changes) + try { + subRecord.listener(publication.changes) + } catch (error) { + if (!hasError) { + hasError = true + deliveryError = error + } + } } + if (hasError) throw deliveryError }Note: rethrowing after the inner loop still aborts the outer
while. If you want the queue to always drain, collect the error and rethrow after the loop ends.🤖 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` around lines 495 - 500, Update the publication delivery logic around the listener invocation in the observer’s emit flow to isolate each subRecord.listener call, allowing all targets and queued publications to be processed even when a listener throws. Capture the first thrown error, continue draining the inner and outer publication queues, then rethrow after queue processing completes while preserving the disposed checks and existing delivery behavior.packages/solid-db/tests/useLiveQuery.test.tsx (1)
586-586: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed setup delays with
waitFor.Lines 586, 641, 646, and 665 gate assertions on fixed 10ms, 20ms, and 50ms sleeps. Under a loaded CI runner these windows can expire before the collection settles, which makes the tests flaky. The first new test at Line 109 already uses
waitForcorrectly.Keep the synchronous assertions that are the subject of the tests (Lines 592-594 after
setMinAge, Lines 113-114 aftersetCurrent). Change only the setup waits.♻️ Proposed change for the narrowing test setup
- await new Promise((resolve) => setTimeout(resolve, 50)) - expect(rendered.result.state.size).toBe(3) // all three ages > 10 + // all three ages > 10 + await waitFor(() => expect(rendered.result.state.size).toBe(3))♻️ Proposed change for the superseded-continuation test setup
- await new Promise((resolve) => setTimeout(resolve, 10)) - expect(rendered.result.isLoading).toBe(true) + await waitFor(() => expect(rendered.result.isLoading).toBe(true)) // Switch collections while the slow fetch is still awaiting readiness. setUseSlow(false) - await new Promise((resolve) => setTimeout(resolve, 10)) - expect(rendered.result.state.has(`1`)).toBe(true) + await waitFor(() => expect(rendered.result.state.has(`1`)).toBe(true))For the final wait at Line 665, await the observable effect instead of a fixed delay:
markReadyA!() - await new Promise((resolve) => setTimeout(resolve, 20)) - - expect(rendered.result.state.has(`stale`)).toBe(false) + await waitFor(() => expect(rendered.result.status).toBe(`ready`)) + expect(rendered.result.state.has(`stale`)).toBe(false)Also applies to: 641-646, 665-665
🤖 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` at line 586, Replace the fixed setup sleeps at the identified waits in useLiveQuery tests with waitFor-based conditions that observe the collection state or effect completion. Preserve the synchronous assertions after setMinAge and setCurrent unchanged, and for the final wait await the observable effect rather than using a timeout; modify only the setup synchronization.packages/solid-db/src/useLiveQuery.ts (1)
412-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree adapters re-implement the same observer-delta materialization. Each granular adapter applies
insert/update/deleteto a keyed map, then rebuilds that map fromobserver.getSnapshot().statewhenchangesisundefined. Only the map type and the batching wrapper differ. A future change to the status-only rebuild rule must then be made three times, and a missed site silently diverges the keyed view from the ordered data.Extract one helper into
@tanstack/db, for exampleapplyObserverChanges(map, changes, snapshotState), and call it from each adapter inside that adapter's own batching primitive.
packages/solid-db/src/useLiveQuery.ts#L412-L439: call the shared helper inside the existingbatch(...), keeping theReactiveMapas the target.packages/svelte-db/src/useLiveQuery.svelte.ts#L424-L450: call the shared helper inside the existinguntrack(...), keeping theSvelteMapas the target.packages/vue-db/src/useLiveQuery.ts#L402-L426: call the shared helper directly, keeping thereactive(new Map())as the target.As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places".
🤖 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/src/useLiveQuery.ts` around lines 412 - 439, Extract the shared observer-delta materialization into an `@tanstack/db` helper such as applyObserverChanges(map, changes, snapshotState), including insert/update/delete handling and rebuilding from snapshotState when changes is undefined. In packages/solid-db/src/useLiveQuery.ts#L412-L439, call it inside batch(...) with the ReactiveMap; in packages/svelte-db/src/useLiveQuery.svelte.ts#L424-L450, call it inside untrack(...) with the SvelteMap; and in packages/vue-db/src/useLiveQuery.ts#L402-L426, call it directly with the reactive Map. Keep each adapter’s existing batching behavior and subsequent synchronization/status updates unchanged.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 @.changeset/live-query-observer.md:
- Around line 12-16: Update the changeset description around
createLiveQueryObserver and the lifecycle summary to accurately reflect that
useLiveQuery starts sync via startSyncImmediate before observer creation for
directly supplied and callback-returned collections. Either document this
direct-collection exception explicitly, or remove that pre-activation in
useLiveQuery so the observer owns activation on its first committed subscription
consistently.
---
Nitpick comments:
In `@packages/angular-db/src/index.ts`:
- Around line 261-266: The Angular sync path currently reads collection state
directly, unlike other adapters that use the observer snapshot. Update
syncDataFromCollection and its call sites in the subscription flow to consume
observer.getSnapshot() for status, and reuse snapshot state/data where
applicable, preserving the post-subscribe synchronization behavior.
In `@packages/db/src/collection/sync.ts`:
- Around line 74-88: Update markLayoutChange() to obtain the transaction through
the existing private getActivePendingSyncTransaction() helper instead of
duplicating the pending transaction lookup and committed-state guards, then set
layoutChanged on the returned transaction while preserving the helper’s existing
error behavior.
In `@packages/db/src/live-query-observer.ts`:
- Around line 495-500: Update the publication delivery logic around the listener
invocation in the observer’s emit flow to isolate each subRecord.listener call,
allowing all targets and queued publications to be processed even when a
listener throws. Capture the first thrown error, continue draining the inner and
outer publication queues, then rethrow after queue processing completes while
preserving the disposed checks and existing delivery behavior.
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 905-912: The multiplicity accumulation logic is duplicated across
three sites; extract a shared generic applyMultiplicity<T> helper that handles
deletes, previousValue/previousOrderByIndex on retracts, inserts, value
replacement, and defined orderByIndex updates. In
packages/db/src/query/live/collection-config-builder.ts at lines 905-912 and
1337-1344, replace the inline branches with applyMultiplicity(existing,
childResult, _orderByIndex, multiplicity); at lines 2340-2343, replace the
retract/insert branches with applyMultiplicity(changes, value, orderByIndex,
multiplicity), preserving all existing behavior.
In `@packages/db/tests/conformance/suite.ts`:
- Line 42: Update or remove the stale “universal expected-fail” section heading
near the order-only-move scenario, since UNIVERSAL_EXPECTED_FAIL is empty and
order-only-move is a normal passing scenario. Ensure the surrounding comment
accurately describes the scenario’s current status.
In `@packages/db/tests/live-query-observer.test.ts`:
- Around line 593-617: Add a test near the existing layoutRevision observer
cases that creates an empty source, subscribes a live query observer, records
its initial snapshot, performs a begin/commit cycle without writes, and asserts
the snapshot data remains [] while layoutRevision is unchanged; dispose the
observer afterward.
In `@packages/solid-db/src/useLiveQuery.ts`:
- Around line 412-439: Extract the shared observer-delta materialization into an
`@tanstack/db` helper such as applyObserverChanges(map, changes, snapshotState),
including insert/update/delete handling and rebuilding from snapshotState when
changes is undefined. In packages/solid-db/src/useLiveQuery.ts#L412-L439, call
it inside batch(...) with the ReactiveMap; in
packages/svelte-db/src/useLiveQuery.svelte.ts#L424-L450, call it inside
untrack(...) with the SvelteMap; and in
packages/vue-db/src/useLiveQuery.ts#L402-L426, call it directly with the
reactive Map. Keep each adapter’s existing batching behavior and subsequent
synchronization/status updates unchanged.
In `@packages/solid-db/tests/useLiveQuery.test.tsx`:
- Line 586: Replace the fixed setup sleeps at the identified waits in
useLiveQuery tests with waitFor-based conditions that observe the collection
state or effect completion. Preserve the synchronous assertions after setMinAge
and setCurrent unchanged, and for the final wait await the observable effect
rather than using a timeout; modify only the setup synchronization.
🪄 Autofix
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 Plus
Run ID: 7e53d52d-57eb-461b-a2b5-05314344178b
📒 Files selected for processing (34)
.changeset/live-query-observer.md.changeset/live-query-order-only-move.mdpackages/angular-db/src/index.tspackages/angular-db/tests/conformance.test.tspackages/angular-db/tests/inject-live-query.test.tspackages/db/src/collection/change-events.tspackages/db/src/collection/changes.tspackages/db/src/collection/index.tspackages/db/src/collection/lifecycle.tspackages/db/src/collection/state.tspackages/db/src/collection/subscription.tspackages/db/src/collection/sync.tspackages/db/src/errors.tspackages/db/src/index.tspackages/db/src/live-query-observer.tspackages/db/src/query/live/collection-config-builder.tspackages/db/src/query/live/types.tspackages/db/tests/conformance/contract.tspackages/db/tests/conformance/suite.tspackages/db/tests/live-query-observer.test.tspackages/db/tests/live-query-order-only-move.test.tspackages/react-db/src/useLiveQuery.tspackages/react-db/tests/conformance.test.tsxpackages/react-db/tests/useLiveQuery.eager-onstorechange.test.tsxpackages/react-db/tests/useLiveQuery.strictmode.test.tsxpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/conformance.test.tsxpackages/solid-db/tests/useLiveQuery.test.tsxpackages/svelte-db/src/useLiveQuery.svelte.tspackages/svelte-db/tests/conformance.svelte.test.tspackages/svelte-db/tests/useLiveQuery.svelte.test.tspackages/vue-db/src/useLiveQuery.tspackages/vue-db/tests/conformance.test.tspackages/vue-db/tests/useLiveQuery.test.ts
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Verified the transaction-scoped layout revision fix on 619c3a3. The parked detached-observer regression now passes, the layout revision remains unchanged until the sync transaction is applied, and the broader collection/order-only suites pass (149 tests, no type errors). The implementation also removes the collection-wide pending marker, so unrelated publications cannot consume it. Ready from my review.
# Conflicts: # packages/db/src/collection/changes.ts # packages/db/src/collection/index.ts # packages/db/src/live-query-observer.ts # packages/db/tests/live-query-observer.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/query/includes-oracle.property.test.ts (1)
129-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe generated histories cannot produce an order-only move.
normalizePutpins each row position at the firstputfor that key:positions.get(action.id) ?? action.position. Every laterputfor the same key reuses the stored position. Therefore the fuzz property at Line 984 can changevalueandgroup, but neverposition. Order-only moves are only covered by the hardcodedconfirmedChildReorderSeedand the#1444seed, which both bypassensureActionsTargetRows.This PR targets order-only moves, so the property test provides no random coverage of them. Consider letting a repeat
putchange the position while keeping the correlation keys stable.♻️ Proposed change to allow position moves in generated histories
const normalizePut = (): HistoryAction => { keys.add(action.id) - const position = positions.get(action.id) ?? action.position - positions.set(action.id, position) - return { ...action, type: `put`, position } + positions.set(action.id, action.position) + return { ...action, type: `put`, position: action.position } }Note:
positionsthen only tracks the latest position for non-putactions, which already readcurrent?.positioninapplyAction.Also applies to: 1201-1207
🤖 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/query/includes-oracle.property.test.ts` around lines 129 - 154, Update normalizePut in the history generator so repeated put actions for an existing key can use a newly generated action.position, while preserving the key’s correlation identity and initial-position behavior as appropriate. Adjust positionsByLevel tracking so it stores the latest position needed by subsequent non-put actions, allowing generated histories to include order-only moves and keeping the corresponding logic around the other normalizePut occurrence consistent.
🤖 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/query/includes-oracle.property.test.ts`:
- Around line 129-154: Update normalizePut in the history generator so repeated
put actions for an existing key can use a newly generated action.position, while
preserving the key’s correlation identity and initial-position behavior as
appropriate. Adjust positionsByLevel tracking so it stores the latest position
needed by subsequent non-put actions, allowing generated histories to include
order-only moves and keeping the corresponding logic around the other
normalizePut occurrence consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f2d4cf4-06ad-4ddb-9471-82cc04abae01
📒 Files selected for processing (3)
.changeset/live-query-observer.mdpackages/db/src/collection/sync.tspackages/db/tests/query/includes-oracle.property.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/live-query-observer.md
- packages/db/src/collection/sync.ts
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Re-reviewed current head c27d572 after the main merge. The follow-up cleanly promotes the two includes-oracle cases to positive regressions, reuses the existing active-sync-transaction guard, and corrects the observer changeset wording. The equivalent merged tree passes the full @tanstack/db suite (2,534 tests, 5 skipped, no type errors). Ready.
|
@KyleAMathews Thanks for re-reviewing. #1669 is green and mergeable now—please merge when ready: #1669 #1675 is clean and stacked behind it. |
3 participants
Phase 4 of the live-query platform RFC (#1623): the ordered snapshot / layout contract. Stacked on #1642 (observer migration) — review/merge that first; this PR's base is
refactor/live-query-observerso the diff is Phase 4 only.Problem
An
orderBylive query that reorders its rows without changing any projected row value (an "order-only move") is swallowed by the collection's value-diff:.values()/.entries()re-sort internally, but no change event fires, souseLiveQuerykeeps rendering the stale order. This is the last universal expected-fail in the cross-adapter conformance suite (tracked as #1601).Approach
The RFC is explicit that this should be "an explicit layout-revision requirement, not a hidden forced-update path". So instead of forging a row
update:commit(), detects an order-only move (projected value deep-equal,orderByIndexmoved) and publishes a first-class empty layout-change notification via a newCollectionChangesManager.emitLayoutChangeEvent(). It reuses the existing empty-batch delivery already used for the ready signal — subscribers re-read, nothing is faked.layoutRevision, which increments on any visible membership, ordering, or order-only-move change. This is the canonical contract the RFC wants for future fine-grained materializers; adapters currently pick up the reorder through their existing wholesale re-read.Result
order-only-moveis removed fromUNIVERSAL_EXPECTED_FAILand now passes on all five adapters (React, Vue, Svelte, Solid, Angular) — 26/26 conformance each. Full@tanstack/dbsuite green (2464 tests), all five adapter suites green.Coverage
packages/db/tests/live-query-order-only-move.test.ts— core mechanism: republish +layoutRevisionbump on an order-only move; no bump when order is unchanged (no spurious notification); bump on membership change.order-only-movescenario now a real pass across all adapters.Relationship to #1601
@v-anton's #1601 fixes the same bug via a forced row
update. This PR takes the RFC-sanctioned layout-revision approach instead (a distinct, first-class notification +layoutRevision), so it supersedes rather than duplicates that path. Happy to coordinate on which lands — flagging for maintainer decision.🤖 Generated with Claude Code
Summary by CodeRabbit