Uh oh!
There was an error while loading. Please reload this page.
fix(query-core): reconnect MutationObserver on resubscribe (StrictMode fix) - #11120
Conversation
…e fix) Add onSubscribe() to MutationObserver to reconnect the observer to an in-progress mutation when the component resubscribes (e.g. React's StrictMode double-mount cycle). Without this, the observer is removed from the mutation's observers list on cleanup and never reconnects, causing isPending to stay true indefinitely. QueryObserver already has the same pattern — this makes MutationObserver consistent with it.
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesMutation observer lifecycle
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk:⚪ Minimal · up to This localized fix reconnects mutation observers during resubscription and includes targeted tests and release metadata; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…e===1 Apply Miller Marru's review suggestion: only reattach the observer to the current mutation when the first listener subscribes (listeners.size === 1), matching QueryObserver.onSubscribe's pattern exactly. Add regression test proving the observer reconnects to an in-flight mutation on resubscribe (StrictMode double-mount cycle). Verified RED on main (test fails without onSubscribe) and GREEN with the fix.
There was a problem hiding this comment.
That's right, we need a guard with listeners.size === 1. onSubscribe is called on every subscribe(), so it's not just the first time. Without this, an observer already connected would have to reconnect, which would be a waste of time. Pushed b849bce with the guard. Now MutationObserver behaves in this respect the same way as QueryObserver.
I also added regression testing to StrictMode. It unsubscribes while the mutation is still in progress, and then resubscribes to see if the observer finally succeeds. First, I tried to run it on the main branch, made sure it really failed, and after making the corrections, it passed. So it's not just a happy pass, it actually catches the bug.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/mutationObserver.test.tsx (1)
59-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the resubscribe state notification.
The test checks only the final success callback. It does not verify that resubscribe immediately emits the current pending result or that the final result clears
isPending. Add both assertions to cover theonSubscribe()refresh and notification behavior.Proposed test assertions
mutation.subscribe(subscriptionHandler) + expect(subscriptionHandler).toHaveBeenLastCalledWith(+ expect.objectContaining({+ status: 'pending',+ isPending: true,+ }),+ ) await vi.advanceTimersByTimeAsync(20) expect.objectContaining({ status: 'success', data: 'input', + isPending: false, })🤖 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/query-core/src/__tests__/mutationObserver.test.tsx` around lines 59 - 73, Update the mutationObserver test around mutation.subscribe and subscriptionHandler to assert that resubscribing immediately notifies the handler with the current pending result, including isPending: true, before advancing timers. After the mutation completes, retain the success assertions and additionally verify the final result has isPending: false.
🤖 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/query-core/src/__tests__/mutationObserver.test.tsx`:
- Around line 59-73: Update the mutationObserver test around mutation.subscribe
and subscriptionHandler to assert that resubscribing immediately notifies the
handler with the current pending result, including isPending: true, before
advancing timers. After the mutation completes, retain the success assertions
and additionally verify the final result has isPending: false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7b6516-4255-43ae-a1dc-124749767e7d
📒 Files selected for processing (3)
.changeset/mutation-observer-resubscribe.mdpackages/query-core/src/__tests__/mutationObserver.test.tsxpackages/query-core/src/mutationObserver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/mutationObserver.ts
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... | ❌ Failed | 3m 43s | View ↗ |
nx run-many --target=build --exclude=examples/*... | ❌ Failed | 3s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-08-20 14:56:58 UTC
Look, this is a pr related to this thread #5341.
In the arguments, I saw "that mutations shouldn't have side-effects", "Strict mode tell us we shouldnt use mutate inside use effects", I do understand that "Only on dev happens, but on production it works", I do understand that "useMutationState solves the business" and I also get that "You don't want to waste your time having a workaround to trick StrictMode".
but hey, listen to me. In regards of you not wanting to waste your time, its fine, because this pr solves the business. In regard to using useMutationState, I argue that on a huge codebase, in my opinion it's pretty messy to call useMutationState, because first, isPending should just work regardless of whether you used useEffect or not. isPending should still work reliably on react, without this useMutationState, which in my opinion its only useful for tricking the StrictMode.
I know, that only on dev mode, this issue happens, but listen to me broyos, if i'm developing, i'm gonna test on develop mode, i'm not gonna build to prod everytime I need to make it work.
In regards to "mutations shouldn't have side effects", I argue that Tanstack Query Started as React Query, Tanstack is react's bitch, Bow to it (hey if you didn't laugh I meant no harm, just trying to be funny guy, you dont need to bow if you think the code gets messy).
Now about where the fix lives: onSubscribe() is on query-core, called by Subscribable.subscribe(). It fires for ALL frameworks (React, Solid, Vue, Svelte...). But the scenario that breaks (subscribe → unsubscribe → subscribe on the same tick) is exclusive to React's StrictMode. For every other framework:
So the fix on query-core is safe for everyone. Moving it to react-query wouldn't work because MutationObserver lives on query-core and onSubscribe is a protected method called from inside the class.
But if you still think the change should live only on react-query, I can figure out a way. At least react query would be react's bitch and bow to it, right?
Changes
Adds
onSubscribe()toMutationObserverinquery-core. When a new listener subscribes and there is an existing#currentMutation, the observer is reconnected to it viaaddObserver(this), its result is refreshed via#updateResult(), and subscribers are notified via#notify(). This mirrors the existing pattern inQueryObserver.onSubscribe().Checklist
#currentMutationis undefinedRelease Impact
Summary by CodeRabbit
Bug Fixes
Tests