Uh oh!
There was an error while loading. Please reload this page.
fix(chore): report errors of mutation callbacks asynchronously - #9675
Conversation
WalkthroughWraps mutation lifecycle callbacks (onSuccess/onError/onSettled) in try/catch and converts thrown errors into rejected Promises via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant MO as MutationObserver
participant MF as mutationFn
participant CB as Callbacks (onSuccess/onError/onSettled)
participant LS as Subscribers
participant PR as Promise.reject
U->>MO: mutate(variables, { onSuccess/onError, onSettled })
MO->>MF: execute mutationFn
alt mutation succeeds
MF-->>MO: result
MO->>CB: try { onSuccess(result) }
alt onSuccess throws
CB-->>MO: throw e
MO->>PR: void Promise.reject(e)
end
MO->>CB: try { onSettled(result, null) }
alt onSettled throws
CB-->>MO: throw e2
MO->>PR: void Promise.reject(e2)
end
else mutation errors
MF-->>MO: throw error
MO->>CB: try { onError(error) }
alt onError throws
CB-->>MO: throw e
MO->>PR: void Promise.reject(e)
end
MO->>CB: try { onSettled(undefined, error) }
alt onSettled throws
CB-->>MO: throw e2
MO->>PR: void Promise.reject(e2)
end
end
MO->>LS: notify subscribers (state updates)
MO->>LS: notify subscribers (settled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
View your CI Pipeline Execution ↗ for commit 9faafef
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #9675 +/- ##
===========================================
+ Coverage 45.70% 59.52% +13.82%
===========================================
Files 200 129 -71 Lines 8505 5727 -2778 Branches 1976 1572 -404 ===========================================
- Hits 3887 3409 -478 + Misses 4158 2001 -2157 + Partials 460 317 -143
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/query-core/src/mutationObserver.ts (1)
174-196: DRY up callback handling and also surface async rejections from Promise-returning callbacks.You can remove duplication and ensure callbacks that return a Promise and reject are also reported as unhandled rejections without awaiting them.
Apply this diff within
#notify:- try {- this.#mutateOptions.onSuccess?.(- action.data,- variables,- onMutateResult,- context,- )- } catch (e) {- void Promise.reject(e)- }- try {- this.#mutateOptions.onSettled?.(- action.data,- null,- variables,- onMutateResult,- context,- )- } catch (e) {- void Promise.reject(e)- }+ this.#callSafely(+ this.#mutateOptions.onSuccess,+ action.data,+ variables,+ onMutateResult,+ context,+ )+ this.#callSafely(+ this.#mutateOptions.onSettled,+ action.data,+ null,+ variables,+ onMutateResult,+ context,+ )- try {- this.#mutateOptions.onError?.(- action.error,- variables,- onMutateResult,- context,- )- } catch (e) {- void Promise.reject(e)- }- try {- this.#mutateOptions.onSettled?.(- undefined,- action.error,- variables,- onMutateResult,- context,- )- } catch (e) {- void Promise.reject(e)- }+ this.#callSafely(+ this.#mutateOptions.onError,+ action.error,+ variables,+ onMutateResult,+ context,+ )+ this.#callSafely(+ this.#mutateOptions.onSettled,+ undefined,+ action.error,+ variables,+ onMutateResult,+ context,+ )Add this private helper inside the class:
private #callSafely(cb: ((...args: any[])=>any)|undefined, ...args: any[]): void{try{constr=cb?.(...args)if(r&&typeof(rasany).then==='function'){void(rasPromise<unknown>).catch((e)=>Promise.reject(e))}}catch(e){voidPromise.reject(e)}}Also applies to: 197-217
packages/query-core/src/__tests__/mutationObserver.test.tsx (3)
388-390: Don’t remove all global listeners.
process.removeAllListeners('unhandledRejection')may interfere with the test runner or other suites. Prefer registering a per-test handler and removing just that handler.Apply this diff:
- describe('erroneous mutation callback', () => {- afterEach(() => {- process.removeAllListeners('unhandledRejection')- })+ describe('erroneous mutation callback', () => {And update each test to register and cleanup a dedicated handler (see next comment).
392-429: Register a dedicated handler and clean it up.Avoid leaking listeners and keep tests isolated.
Apply this diff in the first test:
- const unhandledRejectionFn = vi.fn()- process.on('unhandledRejection', (error) => unhandledRejectionFn(error))+ const unhandledRejectionFn = vi.fn()+ const handler = (error: unknown) => unhandledRejectionFn(error)+ process.on('unhandledRejection', handler) @@ - unsubscribe()+ unsubscribe()+ process.off('unhandledRejection', handler)Optionally, after advancing timers, also flush microtasks for robustness:
- await vi.advanceTimersByTimeAsync(0)+ await vi.advanceTimersByTimeAsync(0)+ await Promise.resolve()
431-471: Mirror the handler pattern in the error-path test.Same isolation as above.
Apply this diff:
- const unhandledRejectionFn = vi.fn()- process.on('unhandledRejection', (error) => unhandledRejectionFn(error))+ const unhandledRejectionFn = vi.fn()+ const handler = (error: unknown) => unhandledRejectionFn(error)+ process.on('unhandledRejection', handler) @@ - await vi.advanceTimersByTimeAsync(0)+ await vi.advanceTimersByTimeAsync(0)+ await Promise.resolve() @@ - unsubscribe()+ unsubscribe()+ process.off('unhandledRejection', handler)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/query-core/src/__tests__/mutationObserver.test.tsx(1 hunks)packages/query-core/src/mutationObserver.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/query-core/src/mutationObserver.ts (1)
packages/query-core/src/mutation.ts (1)
action(315-383)
packages/query-core/src/__tests__/mutationObserver.test.tsx (1)
packages/query-core/src/mutationObserver.ts (1)
MutationObserver(23-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/query-core/src/mutationObserver.ts (1)
174-196: Bug fix is correct and aligns with PR objective.Catching user-callback exceptions and deferring them via
void Promise.reject(e)prevents aborting the batch and fixes the stuckisPendingissue while still surfacing the error asynchronously. 👍packages/query-core/src/__tests__/mutationObserver.test.tsx (2)
387-472: Nice coverage of thrown callback errors.The tests validate both success and error paths and confirm listener notification continues. 👍
5-17: Minor: consider adding a jsdom/browser variant forunhandledrejection.Optional: a test that uses
window.addEventListener('unhandledrejection', ...)would exercise the browser path too.
|
SimonSimCity
commented
Dec 9, 2025
If there's anything that would need a discussion or if you want further insight on why I wrote something the way I did, please ping me. I just want to help having this merged, because I see not having this in as a way of disguising errors in my codebase that I want to be aware of. |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9664, by catching the error of
onSuccess,onErrorandonSettledcallbacks passed to themutatefunction and reporting it on a separate execution context.This change will catch all errors and, by passing it to
Promise.reject(e), move it to a new execution context, which we explicitly want to ignore (hence thevoidkeyword).By ignoring the error on the newly created execution context, this error is reported by https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event, where tools like Sentry can pick it up.
Raising of an
unhandledRejectionevent is crucial to help the developer to be informed about their function misbehaving, and the surrounding try-catch will help securing this libraries code, despite of the fact that something failed in the users codebase.Not to be confused with #9676, which handles a slightly different problem.
Summary by CodeRabbit
Bug Fixes
Tests