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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 { const r = cb?.(...args) if (r && typeof (r as any).then === 'function') { void (r as Promise<unknown>).catch((e) => Promise.reject(e)) } } catch (e) { void Promise.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.
|
lachlancollins
force-pushed
the
main
branch
from
5d7869e to
18a12ca
Compare
|
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. |
3 participants
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