Repository navigation
fix(react-db): reject same-ID source replacement in mounted queries - #2003
Merged
Merged
Conversation
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe React query hook now tracks source Collection objects by ID and client scope for derived-identity queries. It throws when a different object reuses an ID in that scope. Suspense cache keys include prepared source identities for scoped and unscoped queries. Documentation and tests describe the rules and supported reuse cases. ChangesReact source identity
Svelte import cleanup
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed source-ID behavior has no identified merge-blocking issue; normal checks remain appropriate before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens source identity isolation and prevents one startup failure from blocking other pending sources. No introduced security issue was established in the reviewed paths. Residual uncertainty concerns failed-source cleanup and overlapping or interrupted renders. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Most changes support Issue Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (2 skipped: 2 unsupported.)
✨ 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/react-router-with-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: 0 B Total Size: 180 kB ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 8.66 kB ℹ️ View Unchanged
|
🦋 Changeset detectedLatest commit: dd95c37 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
KyleAMathews
added a commit
that referenced
this pull request
Oct 5, 2026
…iate }) (#2030) * fix(db): drop optimistic state at settlement and queue direct writes Remove begin({ immediate }) and post-completion retention. A sync transaction committed while an optimistic transaction persists is accepted, queued, and applied when that transaction settles. Query Collection direct writes and persisted internal applies queue the same way. A Query refetch no longer cancels an accepted result (#1990). Work in progress: oracle and dependent tests are not yet aligned. Co-authored-by: Isaac <no-reply@databricks.com> * wip: align oracles and fixtures with settlement-time drop Co-authored-by: Isaac <no-reply@databricks.com> * fix(db): track accepted and visible moments of a sync transaction Commit receipts resolve when a transaction is visible again. Handler-facing writes (Query Collection direct writes, persisted mutation confirmation) wait for acceptance through whenSyncAccepted, so a handler can await its own write. Local origin attribution covers only confirmations held at the completion boundary. Co-authored-by: Isaac <no-reply@databricks.com> * wip: align db tests with settlement-time drop and two-stage receipts Co-authored-by: Isaac <no-reply@databricks.com> * wip: align metadata, replay, and load-subset oracles with acceptance boundary Co-authored-by: Isaac <no-reply@databricks.com> * wip: readiness waits for every accepted transaction Co-authored-by: Isaac <no-reply@databricks.com> * fix: hold eager fetches older than a direct write; PowerSync handler waits for acceptance Also align Query Collection tests, glossary, docs, and add the changeset. Co-authored-by: Isaac <no-reply@databricks.com> * test(node-db-sqlite-persistence): handler awaits acceptance behind a pending source write Co-authored-by: Isaac <no-reply@databricks.com> * test: remove remaining immediate arguments; align TrailBase abort test; refresh mangle cache Co-authored-by: Isaac <no-reply@databricks.com> * test(query-db): keep an accepted superseded result's ownership A refetch that supersedes an accepted result held by a persisting mutation must diff against that result's ownership. Earlier supersession tests held durable storage, where the core transaction had already applied, so the ownership rollback guard was never observed. Co-authored-by: Isaac <no-reply@databricks.com> * fix: reject an aborted subset load while its accepted rows apply Electric and TrailBase check the signal before commit to discard a stale page, and reject the load with AbortError when the caller aborted after acceptance. The load-subset oracles state the law. Co-authored-by: Isaac <no-reply@databricks.com> * test(query-db): hold the first durable write in the #1990 reproduction The refetch variant now holds either the result's own row write or the first write after the refetch starts. The second interleaving fails on origin/main with the collection in error. Co-authored-by: Isaac <no-reply@databricks.com> * docs: add the settlement-drop oracle review record Co-authored-by: Isaac <no-reply@databricks.com> * test(db): type the subset error matrix sources; format two oracle files The matrix assigned static mock-sync sources to a type that admits only failing sources, which vitest reported as four unhandled type errors on main. Co-authored-by: Isaac <no-reply@databricks.com> * fix(db): only accepted sync transactions hold rows or attribute origin A sync transaction still open when an optimistic transaction settled counted as queued, so a later remote write could be marked local. The optimistic-history grammar now opens a sync transaction across settlement and commits or aborts it later, with pinned replays of both review probes. Co-authored-by: Isaac <no-reply@databricks.com> * fix(db): accept a DbClient chunk ahead of an open source transaction A hydration chunk was pushed raw after any open source transaction, so accepted-row readers missed it and the source's commit() targeted the chunk. The chunk now goes through the accepted path ahead of open transactions. Co-authored-by: Isaac <no-reply@databricks.com> * fix: Collection readiness counts accepted rows A mutation handler that awaited its own Collection's readiness during startup waited for rows its own persisting transaction held. Readiness now resolves once the startup rows are accepted; they still publish with the drop. Core drops its readiness deferral, Query and Electric mark ready at acceptance, and PowerSync's startup workaround is gone. Subset loads still wait for visible rows. Co-authored-by: Isaac <no-reply@databricks.com> * fix: reject an aborted subset load at every cut Electric and TrailBase now reject with AbortError when the caller aborts before the fetch, when the fetch fails because of the abort, after the fetch but before commit, and between pages. Accepted pages still apply, and a TrailBase load waits for them to be visible; cleanup stays quiet. The main-pinned Electric and TrailBase abort tests now expect the rejection. Co-authored-by: Isaac <no-reply@databricks.com> * fix(query-db): merge an older fetch with the direct writes made since A fetch that started before a direct write was discarded whole, losing server rows for keys the write never touched. It now keeps the server's rows and takes the accepted row only for keys written after the fetch began. The per-key write generations are cleared when no fetch is in flight. The ownership oracle states the law at four cuts. Co-authored-by: Isaac <no-reply@databricks.com> * docs(db): state the settlement-drop law in the live-query architecture Replace the retired retention and insert-dependency paragraph. Co-authored-by: Isaac <no-reply@databricks.com> * refactor(db): delete unreachable dependent-invalidation code commit(signal) is the only public path to cancellation, and it always targets the open last transaction, so no committed transaction can depend on a canceled one. Delete the invalid-committed replay branch, rejectInvalidCommittedTransactions, invalidationError, duplicateKeyError, and admittedAgainstExistingRow. A cancel of any other transaction, or a replay that invalidates one, now throws SyncQueueInvariantError; two tests witness both throws. Co-authored-by: Isaac <no-reply@databricks.com> * docs: cover readiness, aborted loads, older fetches, and DbClient chunks in the changeset Co-authored-by: Isaac <no-reply@databricks.com> * fix(db): keep open-transaction invalidation; hold the newest completed row The R3 deletion removed invalidation of an open sync transaction, which a later transaction begun inside it can reach by committing first; the R3 review probe showed the accepted receipt then never resolved. Restore it for open transactions only; a committed one still throws the invariant error. The extended optimistic-history campaign found two more counterexamples. A rollback while a sync transaction on its key was still open lost its delete event, because pre-sync capture counted the open transaction. When two completed transactions were held on one key, the older one's row showed after the newer one left the transaction map. Capture and the drain filter now count only accepted transactions, and the hold keeps the newest owner's row. All three are pinned replays. Co-authored-by: Isaac <no-reply@databricks.com> * fix(svelte-db): restore the import-duplicates comments The pre-commit eslint --fix in the last merge treated `svelte` and `svelte/reactivity` as one module. It merged the imports into `import { SvelteMap, untrack } from 'svelte'`, and `svelte` does not export `SvelteMap`, so 65 svelte-db tests failed with `SvelteMap is not a constructor`. #2003 had dropped the comments that disable that rule here. Restore them, as before #2003, so a local hook cannot merge the imports again. Co-authored-by: Isaac <no-reply@databricks.com> * chore: describe the rollback and held-row fixes in the changeset Co-authored-by: Isaac <no-reply@databricks.com> * test(react-db): drop begin({ immediate }) from tests merged from main Co-authored-by: Isaac <no-reply@databricks.com> * test: remove begin({ immediate }) from merged tests and examples; load one @tanstack/db in the DO fixture The persisted oracle tests merged from main still pinned immediate source writes. They now state the queued-source-write law under settlement drop. The SSR examples drop the option, and the on-demand fixture returns its commit receipt so the load settles when its rows are visible. The Cloudflare Durable Object E2E fixture imported createCollection from db's dist while wrangler loaded the persistence code's @tanstack/db from source, so the worker ran two copies of @tanstack/db. Import it from one module graph, as AGENTS.md requires. Co-authored-by: Isaac <no-reply@databricks.com> --------- Co-authored-by: Isaac <isaac@example.com> Co-authored-by: Isaac <no-reply@databricks.com>
Merged
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.
What changes
A mounted
useLiveQuery({ query })hook could keep reading an old source Collection after a new Collection reused its ID. The old source could then fail during cleanup. This PR rejects the replacement during render, before the hook returns stale rows.The error tells the application to unmount the hook and clean up the old source and client scope before it reuses the ID. The hook remembers source IDs across intervening queries and client switches.
Separate hooks can use different source Collections with the same ID. Suspense cache keys include each source object's identity for client-scoped and unscoped hooks. The merge with main keeps its pooled-query path, whose partitions use source object identity.
Deferred source startup
The rejection path can prepare shared
DbClientsources before it detects the ID collision. A failing sync start could prevent later sources from starting and leave their readers atidle. The hook now attempts each pending start once and then throws the first startup error. A later reader can start a healthy source and observe its row.Scope
Issue #1991 requested automatic replacement or migration guidance. This PR uses the maintainer's selected fail-fast rule. Explicit
queryKey, legacy dependency arrays, and direct Collection input keep their existing identity rules.Verification
idleversusready.Checklist
pnpm test.Release impact
Fixes #1991.
Summary by CodeRabbit