fix(db): materialize includes into parent rows before commit#1684
Closed
juliuslipp wants to merge 2 commits into
Closed
fix(db): materialize includes into parent rows before commit#1684juliuslipp wants to merge 2 commits into
juliuslipp wants to merge 2 commits into
Conversation
The includes flush patched parent rows in place only after the live query collection committed, without emitting an event. Consumers that copy rows at emit time (e.g. a live query layered on the collection, the shape framework adapters produce for component-level queries) captured the pre-patch row with its included fields unset, and a parent-only update never triggered the follow-up re-emit, so those consumers kept empty includes until an unrelated child change recomputed the row. Materialize the current includes state into the new parent change values before begin/commit; the post-commit flush re-applies the same values idempotently and the child-change re-emit path is unchanged. Fixes TanStack#1635
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe live query flush now materializes inline includes into parent changes before commit and emission. A regression test verifies layered queries retain included child rows after parent-only updates, and the changeset documents the fix for issue 1635. ChangesLive query include materialization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 1257-1260: Update patchParentChangesFromIncludes to use a generic
result type and type parentChanges as Map<unknown, Changes<TResult>> instead of
any. Preserve the caller’s Changes<TResult> type through the function contract,
using narrowly scoped internal casts only where symbol-based indexing or
setIncludedValue requires them.
In `@packages/db/tests/query/includes.test.ts`:
- Around line 755-757: In the test flow around layered.get(1), replace the fixed
10ms timeout with a deterministic synchronization mechanism, such as awaiting
the collection’s change event or using the project’s retry-polling helper, so
the assertion runs only after the async update completes. Replace the any cast
on renamed with the appropriate concrete type inferred from the collection or
existing test types.
🪄 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: 6ec1e02b-c316-4ea5-9673-dc1fae95c75e
📒 Files selected for processing (3)
.changeset/heavy-pandas-tell.mdpackages/db/src/query/live/collection-config-builder.tspackages/db/tests/query/includes.test.ts
- Type patchParentChangesFromIncludes over the caller's Changes<TResult> instead of Changes<any>, with one narrow cast for symbol-indexed routing - Replace the fixed test sleep with vi.waitFor and drop the as-any casts (the layered query's inferred row type carries name/issues) - Trim comments to the function contract Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
Fixes #1635.
Live query includes are materialized into parent rows only after the parent collection commits:
flushIncludesStatepatches the stored row in place (setIncludedValue) without emitting an event, and the follow-up re-emit (inlineReEmitKeys) only fires for correlation keys with pending child changes.Any consumer that copies rows at emit time never observes that patch. The most common shape is another live query layered on the collection — which is what every framework adapter produces for component-level queries (
useLiveQuery((q) => q.from({ row: myLiveQuery }))). For those consumers, a parent-only update (e.g. a status column change synced from the server, no child rows touched) re-emits the parent row with every included field unset, and since no child change follows, they keep the empty includes until an unrelated child change recomputes the row. This is the "nested includes are undefined on next render after collection.update, a forced re-render brings them back" behavior reported in #1635.The fix: materialize the current includes state into the new parent change values before
begin()/commit(), so the emitted events already carry the included values. The post-commit flush then re-applies the same values idempotently (it still owns child-collection bookkeeping and registry maintenance), and the child-change re-emit path is unchanged. Only inline materializations (array/singleton/concat) are patched pre-commit;collectionmaterialization still attaches in the post-commit flush.New regression test:
includes subqueries > parent-only updates > keeps materialized includes on rows observed through a layered live query— fails onmain, passes with this change.✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit