fix(db): emit change on order-only reorder in ordered live queries - #1601
Open
v-anton wants to merge 4 commits into
Open
fix(db): emit change on order-only reorder in ordered live queries#1601v-anton wants to merge 4 commits into
v-anton wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends ChangesorderBy reorder emission for live queries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
Relationship to other open
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/db/tests/live-query-orderby-reorder.test.ts (2)
128-153: ⚡ Quick winAdd explicit
limit/offsetboundary tests for reorder behavior.You already cover a standard
limitpath; please addlimit(0)and offset-beyond-length cases to lock down empty-window semantics during reorder updates.As per coding guidelines, "Handle limit and offset edge cases: consider what happens when limit is 0, undefined, or when offset exceeds data length" and test corner cases in
*.test.ts.🤖 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-orderby-reorder.test.ts` around lines 128 - 153, The current test covers a standard limit case with limit(3). Add two additional test cases following the same pattern: one testing the reorder behavior when limit(0) is used to create an empty result window, and another testing when offset exceeds the data length to verify empty-window semantics during reorder updates. Use the same manualCollection, createLiveQueryCollection, write, and subscribeChanges pattern to verify that reorder updates are handled correctly when the query window is empty or beyond available data boundaries.Source: Coding guidelines
9-9: ⚡ Quick winRemove
anyfrom test helper/projection paths and use explicit test-local types.Typed helper contracts and projected row types keep regressions catchable at compile-time and make assertions safer.
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: 54-55, 87-87, 95-95, 124-124, 140-140, 152-152
🤖 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-orderby-reorder.test.ts` at line 9, Replace all instances of the `any` type throughout the file (found at the variable declaration `let fns: any` and at the other locations noted in lines 54-55, 87, 95, 124, 140, 152) with explicit type annotations. For test helper functions and projection row types, define appropriate test-local types that clearly express what these variables and functions should return, rather than using `any` or `unknown`. This ensures better type safety and makes assertions more robust by allowing the compiler to catch type-related regressions.Source: Coding guidelines
packages/db/src/query/live/collection-config-builder.ts (1)
1024-1030: 🏗️ Heavy liftAvoid reaching into
_changesviaany; delegate through a typed internal method.This hard-couples
CollectionConfigBuildertoCollectioninternals and weakens compile-time safety. Prefer an internal delegate onCollection/sync methods for forced emits, then call that API here.♻️ Proposed direction
- const changesManager = (config.collection as any)._changes as { - emitEvents: ( - changes: Array<ChangeMessage<TResult>>, - forceEmit?: boolean, - ) => void - } - changesManager.emitEvents(moves, true) + config.collection.emitInternalChanges(moves, { forceEmit: true })As per coding guidelines, "Encapsulate implementation details within responsible classes; use delegation to maintain clean boundaries between components" and "Avoid exposing internal properties directly; instead add public methods that delegate to internal implementations".
🤖 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 1024 - 1030, The code directly accesses the private _changes property on the collection object through any type casting to call emitEvents, which breaks encapsulation and type safety. Instead of reaching into _changes via any, create an internal delegate method on the Collection class (such as emitChangesForced or similar) that encapsulates the _changes.emitEvents call, then replace the direct changesManager assignment and emitEvents invocation in the current code with a call to this new internal method on the collection object. This maintains clean boundaries between components while preserving the forced emit behavior.Source: Coding guidelines
packages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx (2)
37-37: ⚡ Quick winReplace
anyin result mapping with a concrete row type.This keeps hook assertions type-safe and avoids masking contract drift in
useLiveQuerydata shape.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: 44-44
🤖 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/tests/useLiveQuery-orderby-reorder.test.tsx` at line 37, The map function in the result.current.data transformation uses `any` type annotation for the row parameter, which reduces type safety. Identify the concrete row type that useLiveQuery returns (likely defined in your test setup or data models), and replace the `any` type annotation with that concrete type in both the mapping operation at line 37 and the additional occurrence at line 44. This ensures type-safe assertions and prevents masking any contract drift in the useLiveQuery data shape.Source: Coding guidelines
21-47: ⚡ Quick winAdd a rapid consecutive updates test to cover hook-level async race ordering.
A case with back-to-back writes that reorder in opposite directions would strengthen confidence that stale order does not render between emissions.
As per coding guidelines, test corner cases including "async race conditions where operations may resolve in unexpected order".
🤖 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/tests/useLiveQuery-orderby-reorder.test.tsx` around lines 21 - 47, Add a new test case within the existing describe block for useLiveQuery orderBy + select(id-only) that tests rapid consecutive updates with reordering in opposite directions. This test should perform multiple back-to-back writes using act() that cause the items to reorder in alternating directions (for example, one update that moves an item to the top, followed immediately by another that moves it to the bottom), and then verify using waitFor() that the final rendered order matches the expected result without any stale intermediate orders appearing. This will ensure the hook properly handles async race conditions where multiple reordering operations occur in quick succession.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/tests/live-query-orderby-reorder.test.ts`:
- Line 4: The `flush` function uses a hardcoded setTimeout with a fixed 20ms
delay, making tests timing-sensitive under CI load. Instead of relying on this
sleep-based approach, replace the `flush` function implementation to use
condition-based waits by leveraging testing utilities like `waitFor` to
explicitly wait on observable state changes. Identify the specific order or
event expectations that each test needs to verify after calling `flush`, and
replace the fixed sleep with explicit waits on those observable state conditions
to ensure reliable test execution regardless of system load.
---
Nitpick comments:
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 1024-1030: The code directly accesses the private _changes
property on the collection object through any type casting to call emitEvents,
which breaks encapsulation and type safety. Instead of reaching into _changes
via any, create an internal delegate method on the Collection class (such as
emitChangesForced or similar) that encapsulates the _changes.emitEvents call,
then replace the direct changesManager assignment and emitEvents invocation in
the current code with a call to this new internal method on the collection
object. This maintains clean boundaries between components while preserving the
forced emit behavior.
In `@packages/db/tests/live-query-orderby-reorder.test.ts`:
- Around line 128-153: The current test covers a standard limit case with
limit(3). Add two additional test cases following the same pattern: one testing
the reorder behavior when limit(0) is used to create an empty result window, and
another testing when offset exceeds the data length to verify empty-window
semantics during reorder updates. Use the same manualCollection,
createLiveQueryCollection, write, and subscribeChanges pattern to verify that
reorder updates are handled correctly when the query window is empty or beyond
available data boundaries.
- Line 9: Replace all instances of the `any` type throughout the file (found at
the variable declaration `let fns: any` and at the other locations noted in
lines 54-55, 87, 95, 124, 140, 152) with explicit type annotations. For test
helper functions and projection row types, define appropriate test-local types
that clearly express what these variables and functions should return, rather
than using `any` or `unknown`. This ensures better type safety and makes
assertions more robust by allowing the compiler to catch type-related
regressions.
In `@packages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx`:
- Line 37: The map function in the result.current.data transformation uses `any`
type annotation for the row parameter, which reduces type safety. Identify the
concrete row type that useLiveQuery returns (likely defined in your test setup
or data models), and replace the `any` type annotation with that concrete type
in both the mapping operation at line 37 and the additional occurrence at line
44. This ensures type-safe assertions and prevents masking any contract drift in
the useLiveQuery data shape.
- Around line 21-47: Add a new test case within the existing describe block for
useLiveQuery orderBy + select(id-only) that tests rapid consecutive updates with
reordering in opposite directions. This test should perform multiple
back-to-back writes using act() that cause the items to reorder in alternating
directions (for example, one update that moves an item to the top, followed
immediately by another that moves it to the bottom), and then verify using
waitFor() that the final rendered order matches the expected result without any
stale intermediate orders appearing. This will ensure the hook properly handles
async race conditions where multiple reordering operations occur in quick
succession.
🪄 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: 45978f9c-2f09-4ecd-b14c-7c28e43ad837
📒 Files selected for processing (5)
.changeset/orderby-reorder-emission.mdpackages/db/src/query/live/collection-config-builder.tspackages/db/src/query/live/types.tspackages/db/tests/live-query-orderby-reorder.test.tspackages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx
…ary cases - Drop fixed setTimeout flushes (live query is synchronous on commit) - Type the sync helper and projections instead of any - Add limit(0) and offset-beyond-length empty-window reorder cases
Author
|
Thanks @coderabbitai — addressed the test feedback in
On the |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
On the |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/live-query-orderby-reorder.test.ts (1)
49-203: ⚡ Quick winAdd missing corner-case tests required by test guidelines (single-element + async race).
This suite now covers the main repro and limit/offset edges, but it still misses guideline-required corner coverage for a single-element collection and an async race/ordering scenario (e.g., back-to-back updates that reorder in opposite directions and assert final order + emitted events are not stale).
As per coding guidelines, "
**/*.test.{ts,tsx,js}: Test corner cases including: ... single-element collections ... async race conditions ... and limit/offset edge cases".🤖 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-orderby-reorder.test.ts` around lines 49 - 203, Add two new test cases to the test suite that cover the required corner cases. First, add a single-element collection test using the same pattern as the existing tests (manualCollection with one item, createLiveQueryCollection with orderBy and select, write an update that triggers a reorder, and verify the collection remains correct and emits appropriate events). Second, add an async race condition test that writes two back-to-back updates causing opposite reorders (for example, first update moves an item to the front, then immediately updates another item to push the first item back), and verify that the final order is correct and that the emitted events reflect the actual state transitions without stale or duplicate events.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/live-query-orderby-reorder.test.ts`:
- Around line 49-203: Add two new test cases to the test suite that cover the
required corner cases. First, add a single-element collection test using the
same pattern as the existing tests (manualCollection with one item,
createLiveQueryCollection with orderBy and select, write an update that triggers
a reorder, and verify the collection remains correct and emits appropriate
events). Second, add an async race condition test that writes two back-to-back
updates causing opposite reorders (for example, first update moves an item to
the front, then immediately updates another item to push the first item back),
and verify that the final order is correct and that the emitted events reflect
the actual state transitions without stale or duplicate events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcc9819d-c473-496c-89f0-93da1040c68e
📒 Files selected for processing (1)
packages/db/tests/live-query-orderby-reorder.test.ts
Author
|
Added the two requested corner cases in |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/live-query-orderby-reorder.test.ts (1)
261-263: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTighten the emission-count assertion to catch duplicate-emission regressions.
At Line 262,
toBeGreaterThanOrEqual(2)still passes if each reorder emits more than once. Since this scenario is validating reorder emission behavior, assert exactly one callback per write.Suggested change
// move a to the front (2 -> 0) write(`update`, { id: `a`, name: `Alice`, value: 0 }) expect(idsOf(live)).toEqual([`a`, `b`]) + expect(emitted).toBe(1) // push a back behind b (0 -> 5) write(`update`, { id: `a`, name: `Alice`, value: 5 }) expect(idsOf(live)).toEqual([`b`, `a`]) // each reorder is observed; no stale/missing final state - expect(emitted).toBeGreaterThanOrEqual(2) + expect(emitted).toBe(2)🤖 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-orderby-reorder.test.ts` around lines 261 - 263, The assertion `expect(emitted).toBeGreaterThanOrEqual(2)` on line 262 is too permissive and will not catch duplicate emission regressions. Change the matcher from `toBeGreaterThanOrEqual(2)` to `toBe(2)` in the expect statement to verify that exactly 2 emissions occur (one per reorder), which will fail if any duplicate emissions occur during the reorder operations.
🤖 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/live-query-orderby-reorder.test.ts`:
- Around line 261-263: The assertion `expect(emitted).toBeGreaterThanOrEqual(2)`
on line 262 is too permissive and will not catch duplicate emission regressions.
Change the matcher from `toBeGreaterThanOrEqual(2)` to `toBe(2)` in the expect
statement to verify that exactly 2 emissions occur (one per reorder), which will
fail if any duplicate emissions occur during the reorder operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89844ed8-2fad-4903-a790-0ebc3f4601bf
📒 Files selected for processing (1)
packages/db/tests/live-query-orderby-reorder.test.ts
Author
|
Good call — tightened in the latest commit: the back-to-back test now asserts |
7 tasks
KyleAMathews
added a commit
that referenced
this pull request
Aug 12, 2026
…e 4) (#1669) * feat(db): shared live-query observer + migrate all five adapters 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> * fix(db): make observer onFirstReady detach-safe 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> * fix(db): address observer/react lifecycle review findings - 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> * fix(react-db): don't dispose the observer in an unmount effect (StrictMode) 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> * test(db): cover live-query keyed-state invariant on recompile 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> * fix(solid-db): clear keyed state before subscribing to a new collection 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> * feat(db): republish ordered live queries on an order-only move (RFC #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> * ci: apply automated fixes * refactor(db): compare key sequence directly for layoutRevision + fix doc 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> * ci: apply automated fixes * test(db): add failing regressions for Kyle's review findings 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> * fix(db): coalesce layout publications and cover ordered includes children 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> * test(db): guard order-only moves in deeply-nested ordered includes 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> * ci: apply automated fixes * fix(db): FIFO non-reentrant observer dispatch over subscription records 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> * fix(db): release the collection subscription on dispose during initial 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> * fix(db): seed late observer subscribers; reject subscribe after dispose 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> * fix(db): drive observer snapshots from a collection-owned state revision 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> * test(angular-db): align the mock collection with the real collection 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> * fix(db): publish collection status changes through the canonical path 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> * fix(db): per-consumer initial-state policy; lazy snapshot materialization 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> * fix(db): remove deferInitialNotify — event reordering gone by construction 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> * fix(db): make observer construction inert — sync activates on first subscribe 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> * fix(solid-db): generation-guard the resource's async continuations 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> * docs(db): mark the observer as internal/unstable; honest changeset 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> * fix(db): address live query observer review * fix(db): preserve wholesale consistency reads * fix(db): capture granular initial loads * fix(db): bind layout revisions to sync transactions * fix(db): address post-merge review feedback --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Kyle Mathews <mathews.kyle@gmail.com> Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com>
KyleAMathews
added a commit
that referenced
this pull request
Aug 12, 2026
) * feat(db): shared live-query observer + migrate all five adapters 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> * fix(db): make observer onFirstReady detach-safe 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> * fix(db): address observer/react lifecycle review findings - 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> * fix(react-db): don't dispose the observer in an unmount effect (StrictMode) 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> * test(db): cover live-query keyed-state invariant on recompile 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> * fix(solid-db): clear keyed state before subscribing to a new collection 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> * feat(db): republish ordered live queries on an order-only move (RFC #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> * ci: apply automated fixes * refactor(db): compare key sequence directly for layoutRevision + fix doc 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> * ci: apply automated fixes * test(db): add failing regressions for Kyle's review findings 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> * fix(db): coalesce layout publications and cover ordered includes children 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> * test(db): guard order-only moves in deeply-nested ordered includes 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> * ci: apply automated fixes * feat(db): shared live-query window controller (RFC #1623 phase 5) Extracts the forward-pagination state machine out of react-db's useLiveInfiniteQuery into a framework-agnostic createLiveQueryWindowController in @tanstack/db, composing the shared live-query observer. The controller owns loadedPageCount, the peek-ahead window (via collection.utils.setWindow), page slicing, and hasNextPage/isFetchingNextPage, and exposes a reactivity-free getSnapshot/subscribe/fetchNextPage/reset/dispose surface mirroring the observer. react-db's useLiveInfiniteQuery is now a thin binding over it with no public API change; its existing suite stays green. Vue/Svelte/Solid/Angular can build infinite queries on the same controller instead of re-porting React's logic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: apply automated fixes * fix(react-db): restore useLiveQuery type-only import for return type UseLiveInfiniteQueryReturn references ReturnType<typeof useLiveQuery>, but the import was dropped in the controller rewrite. vitest's typecheck missed it; the package build (strict tsc) caught it (TS2304). Re-add as a type-only import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(react-db): react to runtime pageSize changes + restore window-mismatch warn Addresses review of the window-controller extraction: - pageSize/initialPageParam are now part of the controller-recreation check, so changing them at runtime re-windows and re-slices (the old hook had them in its effect/memo deps; the first controller draft baked them in at creation). - Restore the one-time console.warn when a pre-created collection's existing window differs from the first page the hook enforces (dropped in the rewrite). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: apply automated fixes * fix(db): FIFO non-reentrant observer dispatch over subscription records 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> * fix(db): release the collection subscription on dispose during initial 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> * fix(db): seed late observer subscribers; reject subscribe after dispose 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> * fix(db): drive observer snapshots from a collection-owned state revision 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> * test(angular-db): align the mock collection with the real collection 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> * fix(db): publish collection status changes through the canonical path 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> * fix(db): per-consumer initial-state policy; lazy snapshot materialization 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> * fix(db): remove deferInitialNotify — event reordering gone by construction 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> * fix(db): make observer construction inert — sync activates on first subscribe 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> * fix(solid-db): generation-guard the resource's async continuations 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> * docs(db): mark the observer as internal/unstable; honest changeset 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> * fix(db): address live query observer review * fix(db): preserve wholesale consistency reads * fix(db): capture granular initial loads * fix(db): harden live query window subscriptions * fix(db): bind layout revisions to sync transactions * fix(db): address post-merge review feedback * fix(db): harden live query window ownership * fix(db): close window controller race gaps * fix(db): preserve live query observer invariants * fix(react-db): handle pagination load failures * fix(db): harden live query window coordination * docs: update window controller release notes --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Kyle Mathews <mathews.kyle@gmail.com> Co-authored-by: Tanner Linsley <tannerlinsley@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
A live query with
orderBydoes not emit a change when a row's position changes but its projected value does not. For example:When a row's
valuechanges so its position moves, the projected value ({ id }) is byte-identical, so the collection's value-diff (deepEqualsinstate.ts) suppresses the update. The live query'sSortedMapre-sorts internally, butsubscribeChangesemits nothing — souseLiveQuerykeeps rendering the stale order. Any projection that omits the changingorderByfield hits this (it is not specific to id-only).Fix
The
orderByoperator already streams the move as a retract (old value + oldorderByIndex) plus an insert (new value + new index). We capture the retracted side inaccumulateChangesand, after each flush, emit anupdatedirectly only when the index changed and the value is unchanged — the exact case the value-diff swallows. This mirrors the existing includes-materialization direct-emit pattern and leaves generic collection code untouched.orderBy(the gate short-circuits onundefinedindex).state.ts's emit (value changed).✅ Checklist
pnpm test.New tests:
packages/db/tests/live-query-orderby-reorder.test.ts(id-only reorder emits; exactly-one-emit when sort field projected; no spurious emit;limitwindow) andpackages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx(useLiveQueryrenders correct order). Full@tanstack/db(2391) and@tanstack/react-db(95) suites pass.🚀 Release Impact
@tanstack/dbpatch).Summary by CodeRabbit
Bug Fixes
orderByto correctly detect and publish “order-only” reorders, even when item projected values remain unchanged.Tests
orderByreorder coverage (projection behavior,limit/offsetedge cases, single-item, and consecutive reorders).useLiveQuerycoverage to verify UI ordering updates after reorder writes even when the sort field isn’t selected.