fix(chore): report errors of useMutation callbacks asynchronously #9676
Conversation
WalkthroughMutation error-path callbacks are now individually guarded so thrown errors from lifecycle callbacks are converted to rejected promises and do not break the outer mutation error flow; tests expanded for erroneous-callback scenarios and unhandledRejection listeners; Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User code
participant M as Mutation.execute
participant C as MutationCache (cache hooks)
participant O as Options (per-mutation hooks)
U->>M: execute() → run mutateFn()
alt mutateFn throws (E)
M->>C: cache.onError(E)
Note right of M: guarded — thrown errors -> void Promise.reject(e)
M->>O: options.onError(E)
Note right of M: guarded — thrown errors -> void Promise.reject(e)
M->>C: cache.onSettled(undefined, E)
Note right of M: guarded — thrown errors -> void Promise.reject(e)
M->>O: options.onSettled(undefined, E)
Note right of M: guarded — thrown errors -> void Promise.reject(e)
M->>M: dispatch(error, E)
M-->>U: rethrow original error E
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 362d187
☁️ Nx Cloud last updated this comment at |
![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 (2)
packages/query-core/src/__tests__/mutations.test.tsx (1)
846-849: Process listener cleanup should be more robust.The
afterEachcleanup only removes listeners but doesn't restore any previously registered listeners. This could interfere with other test suites if they also use unhandledRejection listeners.Consider saving and restoring existing listeners:
+ let originalListeners: Array<(...args: any[]) => void> + + beforeEach(() => { + originalListeners = process.listeners('unhandledRejection') + process.removeAllListeners('unhandledRejection') + }) + afterEach(() => { process.removeAllListeners('unhandledRejection') + originalListeners.forEach(listener => { + process.on('unhandledRejection', listener) + }) })packages/query-core/src/mutation.ts (1)
281-283: Consider adding error context for debugging.While
void Promise.reject(e)correctly reports errors asynchronously, the errors lack context about their origin, which could make debugging challenging in production.Consider wrapping the error with additional context:
} catch (e) { - void Promise.reject(e) + void Promise.reject(Object.assign( + e instanceof Error ? e : new Error(String(e)), + { source: 'mutationCache.onError', mutationKey: this.options.mutationKey } + )) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/query-core/src/__tests__/mutations.test.tsx(2 hunks)packages/query-core/src/mutation.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/mutations.test.tsx (4)
packages/query-core/src/queryClient.ts (1)
QueryClient(61-648)packages/query-core/src/mutationCache.ts (1)
MutationCache(93-240)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)packages/query-core/src/utils.ts (1)
sleep(363-367)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (9)
packages/query-core/src/__tests__/mutations.test.tsx (4)
3-3: Good addition of MutationCache import for test scenarios.The direct import of
MutationCacheenables the new test suite to exercise cache-level callbacks, which is essential for testing the enhanced error handling paths.
851-907: Well-structured test for global onSuccess error propagation.This test correctly validates that errors thrown by global
onSuccesscallbacks trigger theonErrorcallback path, which is a key improvement in the error handling logic.
959-1026: Excellent test coverage for the double-invocation of onSettled.The test properly validates the documented behavior where
onSettledcan be invoked twice when an error occurs - once after success and once after the error handling. The unhandled rejection verification at line 1022-1023 confirms that the second error is properly reported asynchronously.
1087-1161: Comprehensive test for multiple callback errors.This test thoroughly validates that all callback errors are properly captured as unhandled rejections (lines 1153-1160), while the original mutation error is still propagated correctly (line 1151). The verification of all four unhandled rejections ensures no errors are silently swallowed.
packages/query-core/src/mutation.ts (5)
271-283: Critical improvement: Isolated error handling for cache onError callback.The try-catch wrapper prevents the cache's
onErrorcallback from disrupting the error handling flow. Thevoid Promise.reject(e)pattern correctly schedules the error for asynchronous reporting without blocking execution.
285-294: Good isolation of options onError callback.Each callback is now independently wrapped, ensuring that failures in one don't prevent others from executing. This aligns with the PR objective of ensuring
onSettledis called even ifonErrorthrows.
296-320: Proper implementation of the onSettled double-invocation pattern.The separate try-catch blocks for cache and options
onSettledcallbacks ensure both are invoked even if either throws. This maintains the existing behavior whereonSettledmay be called twice (once after success, once after error) while preventing callback errors from disrupting the flow.
322-323: Correct preservation of original error propagation.The dispatch and throw statements correctly maintain the original mutation error flow, ensuring backward compatibility while the new try-catch blocks handle callback errors separately.
282-282: Verified: void Promise.reject(e) triggers Node unhandledRejection; no browser handler present — confirm desired behavior
- Node: tests register process.on('unhandledRejection', ...) and assert it is called; occurrences in packages/query-core/src/mutation.ts at lines 282, 293, 307, 319.
- Browser: no window.addEventListener('unhandledrejection') found in repo; browsers emit 'unhandledrejection' for unhandled Promise rejections — add a browser-level handler if you need to capture/report these errors.
SimonSimCity
force-pushed
the
report-use-mutation-callback-errors-async
branch
from
0657280 to
d3755da
Compare
SimonSimCity
force-pushed
the
report-use-mutation-callback-errors-async
branch
from
d3755da to
9e26948
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/react-query/src/__tests__/useMutation.test.tsx (2)
1067-1074: Avoid global listener teardown; remove only the handler you add.
removeAllListeners('unhandledRejection')wipes unrelated listeners and can cause cross-test flakiness. Store the handler andoffit inonTestFinished.Apply:
-it('should go to error state if onError callback errors', async ({ onTestFinished }) => { - onTestFinished(() => { - process.removeAllListeners('unhandledRejection') - }) - process.on('unhandledRejection', vi.fn()) +it('should go to error state if onError callback errors', async ({ onTestFinished }) => { + const handler = vi.fn() + process.on('unhandledRejection', handler) + onTestFinished(() => { + process.off('unhandledRejection', handler) + })
1111-1118: Same targeted cleanup here.Mirror the scoped listener removal pattern to avoid nuking global listeners.
Apply:
-it('should go to error state if onSettled callback errors', async ({ onTestFinished }) => { - onTestFinished(() => { - process.removeAllListeners('unhandledRejection') - }) - process.on('unhandledRejection', vi.fn()) +it('should go to error state if onSettled callback errors', async ({ onTestFinished }) => { + const handler = vi.fn() + process.on('unhandledRejection', handler) + onTestFinished(() => { + process.off('unhandledRejection', handler) + })packages/query-core/src/mutation.ts (1)
281-321: Asynchronous error reporting looks right; consider deduping the pattern.The
void Promise.reject(e)approach meets the goal (surfaces unhandledrejection without perturbing control flow). Repeating it four times hurts readability—extract a tiny helper.Apply:
+// Report callback-thrown errors on a separate microtask so they surface as unhandled rejections. +const reportUnhandled = (e: unknown) => { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + Promise.reject(e) +} ... -} catch (e) { - void Promise.reject(e) +} catch (e) { + reportUnhandled(e) } ... -} catch (e) { - void Promise.reject(e) +} catch (e) { + reportUnhandled(e) } ... -} catch (e) { - void Promise.reject(e) +} catch (e) { + reportUnhandled(e) } ... -} catch (e) { - void Promise.reject(e) +} catch (e) { + reportUnhandled(e) }Note: keep the helper file-local (not exported).
packages/query-core/src/__tests__/mutations.test.tsx (3)
846-849: Don’t nuke allunhandledRejectionlisteners.Use targeted removal instead of
removeAllListenersto prevent interference with other suites.Apply:
-describe('erroneous mutation callback', () => { - afterEach(() => { - process.removeAllListeners('unhandledRejection') - }) +describe('erroneous mutation callback', () => { + let addedHandlers: Array<(...args: any[]) => void> = [] + afterEach(() => { + for (const h of addedHandlers) process.off('unhandledRejection', h) + addedHandlers = [] + })And when adding listeners inside tests, push them to
addedHandlers.
962-979: Minor safety/readability: declareresultsbefore using in cache config.The current closure works because invocation happens later, but hoisting
resultsavoids TDZ pitfalls if this test ever changes.Apply:
- queryClient = new QueryClient({ + const results: Array<string> = [] + queryClient = new QueryClient({ mutationCache: new MutationCache({ onSettled: async () => { results.push('global-onSettled') await sleep(10) throw newMutationError }, }), }) queryClient.mount() - - const unhandledRejectionFn = vi.fn() + const unhandledRejectionFn = vi.fn() process.on('unhandledRejection', (error) => unhandledRejectionFn(error)) - - const key = queryKey() - const results: Array<string> = [] + const key = queryKey()
1087-1090: Test name nit: grammar.“it are reported” → “they are reported”.
Apply:
-test('errors by onError and consecutive onSettled callbacks are transferred to different execution context where it are reported', async () => { +test('errors by onError and consecutive onSettled callbacks are transferred to a different execution context where they are reported', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-core/src/__tests__/mutations.test.tsx(2 hunks)packages/query-core/src/mutation.ts(2 hunks)packages/react-query/src/__tests__/useMutation.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/mutations.test.tsx (4)
packages/query-core/src/queryClient.ts (1)
QueryClient(61-648)packages/query-core/src/mutationCache.ts (1)
MutationCache(93-240)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)packages/query-core/src/utils.ts (1)
sleep(363-367)
🔇 Additional comments (2)
packages/query-core/src/mutation.ts (1)
322-323: Preserves original error semantics.Dispatching the mutation error and rethrowing the original
erroraligns with prior behavior and the PR intent.If SSR environments in your ecosystem treat unhandled rejections as hard failures, call this out in release notes to avoid surprises.
packages/query-core/src/__tests__/mutations.test.tsx (1)
3-3: Import from package root is good.Aligns tests with the public API surface.
…rted asynchronously
SimonSimCity
force-pushed
the
report-use-mutation-callback-errors-async
branch
from
9e26948 to
66c5e21
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9676 +/- ##
===========================================
+ Coverage 46.38% 60.70% +14.31%
===========================================
Files 214 143 -71
Lines 8488 5733 -2755
Branches 1920 1536 -384
===========================================
- Hits 3937 3480 -457
+ Misses 4108 1953 -2155
+ Partials 443 300 -143 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/solid-query/src/__tests__/useMutation.test.tsx (1)
1121-1128: Prefer removing only the listener you add (avoid removeAllListeners).Using removeAllListeners risks nuking unrelated handlers added by other tests. Capture the listener and remove just that one.
Apply this diff:
- onTestFinished(() => { - process.removeAllListeners('unhandledRejection') - }) - process.on('unhandledRejection', vi.fn()) + const listener = vi.fn() + process.on('unhandledRejection', listener) + onTestFinished(() => { + process.off('unhandledRejection', listener) + })packages/query-core/src/mutation.ts (1)
281-324: Asynchronous reporting approach looks solid; consider de-duplicating the “void Promise.reject(e)” pattern.Current behavior correctly surfaces callback-thrown errors via unhandledrejection while preserving the original mutation error. For readability and future maintenance, extract a helper.
Apply this diff:
- } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandledRejection(e) + } @@ - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandledRejection(e) + } @@ - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandledRejection(e) + } @@ - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandledRejection(e) + }Add this helper in the module (top-level, near other utils):
function reportUnhandledRejection(e: unknown): void { // Surface callback errors asynchronously without breaking control flow // eslint-disable-next-line @typescript-eslint/no-floating-promises Promise.reject(e) }packages/query-core/src/__tests__/mutations.test.tsx (3)
847-849: Avoid removeAllListeners in test cleanup.Limit cleanup to the listener(s) added by each test to prevent side effects across tests in the same process.
Example per-test pattern:
const unhandledRejectionFn = vi.fn() process.on('unhandledRejection', unhandledRejectionFn) // ...test... process.off('unhandledRejection', unhandledRejectionFn)
962-972: TDZ footgun: “results” is referenced in the MutationCache callback before its declaration.While it works because the callback runs later, referencing a const declared later relies on timing and can confuse readers. Declare results before constructing QueryClient.
Apply this diff inside the test:
- queryClient = new QueryClient({ - mutationCache: new MutationCache({ - onSettled: async () => { - results.push('global-onSettled') - await sleep(10) - throw newMutationError - }, - }), - }) - queryClient.mount() - - const key = queryKey() - const results: Array<string> = [] + const results: Array<string> = [] + + queryClient = new QueryClient({ + mutationCache: new MutationCache({ + onSettled: async () => { + results.push('global-onSettled') + await sleep(10) + throw newMutationError + }, + }), + }) + queryClient.mount() + + const key = queryKey()Also applies to: 977-977
1087-1087: Test name grammar nit.Consider: “errors by onError and consecutive onSettled callbacks are transferred to a different execution context where they are reported”.
-it('errors by onError and consecutive onSettled callbacks are transferred to different execution context where it are reported', async () => { +it('errors by onError and consecutive onSettled callbacks are transferred to a different execution context where they are reported', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/query-core/src/__tests__/mutations.test.tsx(2 hunks)packages/query-core/src/mutation.ts(2 hunks)packages/react-query/src/__tests__/useMutation.test.tsx(2 hunks)packages/solid-query/src/__tests__/useMutation.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-query/src/tests/useMutation.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/mutations.test.tsx (4)
packages/query-core/src/queryClient.ts (1)
QueryClient(61-648)packages/query-core/src/mutationCache.ts (1)
MutationCache(93-240)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)packages/query-core/src/utils.ts (1)
sleep(363-367)
⏰ 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 (2)
packages/solid-query/src/__tests__/useMutation.test.tsx (1)
1169-1176: Same cleanup nit as above: remove the specific listener, not all.- onTestFinished(() => { - process.removeAllListeners('unhandledRejection') - }) - process.on('unhandledRejection', vi.fn()) + const listener = vi.fn() + process.on('unhandledRejection', listener) + onTestFinished(() => { + process.off('unhandledRejection', listener) + })packages/query-core/src/__tests__/mutations.test.tsx (1)
3-3: Import change LGTM.Importing MutationCache from the package root aligns tests with public API usage.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/query-core/src/mutation.ts (4)
281-283: Surface callback errors asynchronously — consider a small helper to deduplicate and document intent
void Promise.reject(e)correctly routes thrown callback errors to global unhandledrejection without breaking the control flow. To avoid repetition and make intent explicit, extract a helper and use it here.Apply within this range:
- } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandled(e) + }Add once in this file (outside the shown range), e.g. near the top-level utilities:
function reportUnhandled(e: unknown): void { // Surface callback errors to global `unhandledrejection` listeners without disrupting mutation flow void Promise.reject(e) }
285-294: Use the same helper for options.onError wrapperRepeat the helper to keep behavior consistent and reduce duplication.
- try { + try { await this.options.onError?.( error as TError, variables, this.state.context, mutationFnContext, ) - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandled(e) + }
296-308: Apply helper for cache onSettled wrapperSame rationale; keep all error-path wrappers uniform.
- try { + try { // Notify cache callback await this.#mutationCache.config.onSettled?.( undefined, error as any, this.state.variables, this.state.context, this as Mutation<unknown, unknown, unknown, unknown>, mutationFnContext, ) - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandled(e) + }
310-319: Apply helper for options.onSettled wrapperConsistent handling and clearer intent.
- try { + try { await this.options.onSettled?.( undefined, error as TError, variables, this.state.context, mutationFnContext, ) - } catch (e) { - void Promise.reject(e) - } + } catch (e) { + reportUnhandled(e) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/mutation.ts(2 hunks)
⏰ 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 (1)
packages/query-core/src/mutation.ts (1)
322-323: Confirm error dispatch ordering matches prior behavior
Dispatching the error after executing all error-path callbacks alters observable event timing; verify this matches previous versions to avoid regressions in consumers relying on specific ordering.
lachlancollins
force-pushed
the
main
branch
from
5d7869e to
18a12ca
Compare
Not to be confused with #9675, which handles a slightly different problem.
By catching the error of
onErrorandonSettledcallbacks passed to theuseMutationhook and reporting it on a separate execution context, we can ensure thatonSettledwill be called even ifonErrorthrows an error.For developers, by reporting the error in a separate context, we enable the ability to globally catch those callback errors by using the
unhandledRejectionevent fired onwindow.Unchanged remains the error handling of
onSuccess, which will (as before) callonErrorandonSettled.Just one thing might remain suspicious - and this is still how it has been before: If the mutation function was successful, and the following
onSettledthrows an error, theonErrorcallbacks are called, and alsoonSettled- means,onSettled, if it errors, is called twice. I didn't want to go into a deep refactoring here, so I left it this way - the same behaviour as before, just that its second error now raises anunhandledRejectionevent.Summary by CodeRabbit
Bug Fixes
Tests
Breaking Changes