fix(react-db): defer eager onStoreChange to a microtask in useLiveQuery + regression test (closes #1587)#1594
Open
kevin-dp wants to merge 5 commits into
Open
Conversation
Closes TanStack#1587. `useLiveQuery`'s `subscribeRef` calls `onStoreChange()` synchronously inside the `useSyncExternalStore` subscribe function when the underlying collection is already `ready`. That synchronous notification lands during the render-to-commit window when subscribe runs under StrictMode double-render or cold/throttled loads, which React surfaces as: Can't perform a React state update on a component that hasn't mounted yet. This indicates that you have a side-effect in your render function that asynchronously tries to update the component. Move this work to useEffect instead. The fix is to defer the eager notification to a microtask so it lands after the current commit. While doing so, also guard the late notify path against an in-flight `subscribeChanges` callback firing after React unsubscribes — track a local `unsubscribed` flag and drop both the eager microtask and any in-flight subscription event after teardown, so React never sees a state update post-unsubscribe. No public API change; the contract of `useLiveQuery` is preserved (an already-ready collection still notifies React once after mount, just asynchronously instead of mid-commit). Verified `pnpm test` in packages/react-db — 94/94 pass, no type errors. Existing tests don't cover the race directly (it's a StrictMode-double-render / cold-load condition observed via Lighthouse in the issue), so the existing suite is the regression guard for existing behavior and the issue's repro is the behavioral validation.
…nge (TanStack#1587) Captures the subscribe callback that useLiveQuery passes to React.useSyncExternalStore and asserts that onStoreChange is not invoked synchronously when the collection is already in the 'ready' state — it is instead deferred to a microtask. Without the fix, the eager notify lands during the render-to-commit window and React surfaces: Can't perform a React state update on a component that hasn't mounted yet. ... Move this work to useEffect instead.
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 (1)
📝 WalkthroughWalkthrough
ChangesDeferred ready notification and unsubscribe guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-db/tests/useLiveQuery.issue-1587.test.tsx (1)
62-65: 💤 Low valueConsider adding a test for the unsubscribe guard.
The implementation guards against late notifications when
unsub()is called before the microtask fires. A complementary test could verify this:it(`does not fire onStoreChange if unsubscribed before microtask`, async () => { // ... same setup as above ... const onStoreChange = vi.fn() const unsub = capturedSubscribe!(onStoreChange) // Unsubscribe immediately, before microtask fires unsub() await Promise.resolve() expect(onStoreChange).not.toHaveBeenCalled() })🤖 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.issue-1587.test.tsx` around lines 62 - 65, Add a new test case that verifies the unsubscribe guard functionality works correctly. Create a test (using the `it` function) that sets up the same initial conditions as the existing test (creating a capturedSubscribe call with an onStoreChange mock), immediately calls `unsub()` before the microtask fires, then awaits the Promise to resolve, and finally asserts that onStoreChange was never called. This complementary test verifies that the guard prevents late notifications when unsubscribe is called before the microtask executes.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/react-db/tests/useLiveQuery.issue-1587.test.tsx`:
- Around line 62-65: Add a new test case that verifies the unsubscribe guard
functionality works correctly. Create a test (using the `it` function) that sets
up the same initial conditions as the existing test (creating a
capturedSubscribe call with an onStoreChange mock), immediately calls `unsub()`
before the microtask fires, then awaits the Promise to resolve, and finally
asserts that onStoreChange was never called. This complementary test verifies
that the guard prevents late notifications when unsubscribe is called before the
microtask executes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23d56529-ef8b-4b75-8634-5de216001b56
📒 Files selected for processing (2)
packages/react-db/src/useLiveQuery.tspackages/react-db/tests/useLiveQuery.issue-1587.test.tsx
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
2 participants
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.
Closes #1587. Supersedes / re-applies #1588.
What
Two commits:
fix(react-db): defer eager onStoreChange to a microtask in useLiveQuery— cherry-picked from fix(react-db): defer eager onStoreChange to a microtask in useLiveQuery #1588 by @tsushanth (authorship preserved). WhenuseLiveQuery'ssubscribecallback runs against a collection whosestatusis alreadyready, it used to callonStoreChange()synchronously. Under React 19'suseSyncExternalStore(and especially in StrictMode double-render or on cold/throttled loads), that notify lands during the render-to-commit window and React surfaces:The fix wraps the eager notify in
queueMicrotask(...)so it lands after commit, and adds anunsubscribedguard to drop late notifies if React tears down between scheduling and firing.test(react-db): add regression test for useLiveQuery eager onStoreChange (#1587)— new. Mocksreact'suseSyncExternalStoreto capture thesubscribecallbackuseLiveQueryregisters, invokes it directly against an already-readyLiveQueryCollection, and asserts:onStoreChangeis NOT called synchronously insidesubscribe(),onStoreChangeIS called exactly once after one microtask.I verified that test fails on
main(synchronous call, 1 invocation observed) and passes after the fix.Verification
packages/react-dbsuite: 95/95 passing (94 existing + 1 new).Credit
Fix commit is cherry-picked with original authorship from @tsushanth (PR #1588). Thanks!
Summary by CodeRabbit
Bug Fixes
Tests