feat(db): ordered snapshot / layout-revision contract (RFC #1623 phase 4)#1669
feat(db): ordered snapshot / layout-revision contract (RFC #1623 phase 4)#1669kevin-dp wants to merge 8 commits into
Conversation
…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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: +474 B (+0.38%) Total Size: 127 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 |
2 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