fix(db): propagate changes through nested toArray includes at depth 3+#1457
fix(db): propagate changes through nested toArray includes at depth 3+#1457KyleAMathews merged 5 commits intomainfrom
Conversation
flushIncludesState only flushed entries dirty from direct child changes (Phase 2) or single-level buffer drain (Phase 3). When only the deepest level changed (e.g., textDeltas in runs→texts→textDeltas), intermediate levels had no changes, so Phase 4 never triggered recursive flushing and deep buffer changes were permanently stranded. Add a third pass in Phase 4 that scans child registry entries for pending deep nested buffer changes via hasPendingIncludesChanges(). Entries with stranded buffers are recursively flushed and their correlation keys added to the inline re-emit set. Also prevent toArray()/concat(toArray()) from being silently wrapped as Value nodes inside expressions like coalesce() — throw a clear error at query construction time instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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: +251 B (+0.22%) Total Size: 113 kB 📦 View Changed
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 4.24 kB ℹ️ View Unchanged
|
Remove explicit generic parameter from createLiveQueryCollection and use `any` casts for query/select callbacks in the chained darix pattern test, matching the style used in other test cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
GPT5.2 review: Looks good to me. What I reviewed
What I checked
Validation
Notes
LGTM |
|
Opus 4.6 Review: All 112 tests pass. Here's my review of PR #1457: PR Review: fix(db): propagate changes through nested toArray includes at depth 3+Overall assessment: This is a well-targeted, minimal fix for a real bug with excellent test coverage. A few suggestions below. What the PR doesTwo distinct fixes:
Correctness analysisThe core fix is correct. The key insight is sound: when only the deepest level changes (e.g., a textDelta insert at depth 3), intermediate levels have no direct or buffer-drained changes, so they're never marked dirty and never flushed by Phase 4. The new pass uses the existing recursive I traced through the shared buffer model carefully. In state.nestedSetups = setup.nestedSetupsThis means Suggestions1. Consider The wrapper classes are proper classes in the same package, so import { ToArrayWrapper, ConcatToArrayWrapper } from './functions.js'
if (value instanceof ToArrayWrapper || value instanceof ConcatToArrayWrapper) {
const name = value instanceof ToArrayWrapper ? `toArray()` : `concat(toArray())`
throw new Error(...)
}This avoids adding a runtime 2. Minor performance: early exit for the deep buffer pass The deep buffer pass iterates the entire if (state.nestedSetups && hasNestedBufferChanges(state.nestedSetups)) {
for (const [correlationKey, entry] of state.childRegistry) {
// ... existing logic
}
}This replaces the current However, this might not be fully correct because per-entry 3. PR description file table is inaccurate The description lists TestsThe test coverage is excellent — 10 new tests covering:
The spurious update test at line 4902 is particularly good — it verifies referential identity ( expect(data().runs[0].texts[0].text).toBe(``)
expect(data().runs[0].texts).toBe(run1TextsBefore)The tests use SummaryApprove with minor suggestions. The fix is minimal, correct, well-tested, and solves a real user-facing bug. The |
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
I think this looks good, I had a good read through too while the agents were looking. I think Opus is being overly pernickety.
2 participants
Summary
Nested
toArray()includes (depth 3+) silently drop changes. A query liketoArray(runs) → toArray(texts) → concat(toArray(textDeltas))never propagates textDelta inserts — the concatenated text stays empty forever. Also, wrappingtoArray()in expressions likecoalesce(toArray(...))silently produces incorrect results instead of erroring.Root Cause
flushIncludesStatehas five phases. Phase 3 (drainNestedBuffers) only drains one level of shared nested buffers — it processes the immediate children's buffer but not grandchildren's. Phase 4 then recursively flushes only entries that Phase 2 (direct child changes) or Phase 3 (buffer-drained changes) marked as dirty.When only the deepest level changes (e.g., a textDelta insert), intermediate levels have no changes, so neither Phase 2 nor Phase 3 produce dirty entries. Phase 4 does nothing, and the deep buffer changes are permanently stranded.
Approach
Added a third pass at the end of Phase 4 that scans all child registry entries for deep nested buffer changes via
hasPendingIncludesChanges(). This function already recursively checks buffers at all depths. Entries with stranded buffers are recursively flushed, and their correlation keys are added to the inline re-emit set so parent rows are re-materialized.The recursive
flushIncludesStatecall applies the same logic at each level, so this handles arbitrary nesting depth.For the
coalesce(toArray())issue, added brand-based runtime checks intoExpression()that throw a clear error at query construction time.Key Invariants
drainNestedBuffersonly routes entries matching the per-entry routing index — unroutable entries stay in the shared buffer for other entries to claimflushIncludesStatecall applies the same three-pass logic, enabling arbitrary depthNon-goals
drainNestedBuffersitself recursive (would change the existing single-level routing model)Verification
All 2382 tests pass (2060 db + 322 IVM), including 10 new regression tests.
Files changed
packages/db/src/query/live/collection-config-builder.tsflushIncludesState()packages/db/src/query/builder/ref-proxy.tsToArrayWrapper/ConcatToArrayWrapperintoExpression()packages/db/src/query/builder/functions.ts__brandproperty to wrapper classespackages/db/tests/query/repro-bug2.test.tspackages/db/tests/query/repro-many-siblings.test.ts🤖 Generated with Claude Code