fix(broadcast-client): handle unhandled postMessage rejections#10771
Open
n-satoshi061 wants to merge 8 commits into
Open
fix(broadcast-client): handle unhandled postMessage rejections#10771n-satoshi061 wants to merge 8 commits into
n-satoshi061 wants to merge 8 commits into
Conversation
Wrap channel.postMessage() calls in safePost() to catch DataCloneError and similar failures that occur when query data contains non-cloneable values (ReadableStream, Response, Vue reactive proxies, etc.). Adds an optional onBroadcastError callback to BroadcastQueryClientOptions so consumers can pipe errors to Sentry/Datadog. Falls back to console.warn in development when the callback is not provided. Fixes TanStack#10543 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 (1)
📝 WalkthroughWalkthroughbroadcastQueryClient wraps outbound BroadcastChannel.postMessage in safePost and accepts an optional onBroadcastError(error, event); failures invoke the callback (handling throws/rejections) or emit a development-only console.warn. updated/added include state; removed omits state. Tests, docs, and a changeset were added. ChangesBroadcast error handling and recovery
sequenceDiagram
participant BroadcastQueryClient
participant BroadcastChannel
participant onBroadcastError
participant Console
BroadcastQueryClient->>BroadcastChannel: safePost(message) -> postMessage(message)
BroadcastChannel--xBroadcastQueryClient: rejection (DataCloneError)
BroadcastQueryClient->>onBroadcastError: onBroadcastError(error, event)
onBroadcastError-->>BroadcastQueryClient: may throw / reject
BroadcastQueryClient->>Console: console.warn(type, queryHash, error) (dev-only fallback)
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
60-78: ⚡ Quick winAdd a regression test for
onBroadcastErrorthrowing.Please add a case where
onBroadcastErrorthrows, and assert no unhandled rejection escapes. That protects the error-handling contract end-to-end.🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 60 - 78, Add a test that simulates postMessage failing and the supplied onBroadcastError itself throwing: mockPostMessage to reject with the DOMException as before, pass an onBroadcastError mock that throws (e.g. throw new Error('boom')), register a temporary process.on('unhandledRejection') handler that fails the test if invoked, call broadcastQueryClient({ queryClient, broadcastChannel: 'test_channel', onBroadcastError }), trigger the broadcast via queryClient.setQueryData(['test'], { value: 1 }), await a microtask tick (e.g. setTimeout 0 or next tick), then remove the unhandledRejection handler and assert that onBroadcastError was called with the cloneError and that no unhandled rejection handler fired.
🤖 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.
Inline comments:
In `@packages/query-broadcast-client-experimental/src/index.ts`:
- Around line 41-43: The postMessage rejection handler currently calls
onBroadcastError(error, message) directly which can itself throw and convert the
rejection handler into an unhandled rejection; update the catch path that
contains the onBroadcastError invocation (in
packages/query-broadcast-client-experimental/src/index.ts — the block that
checks if (onBroadcastError) { onBroadcastError(error, message) } else if
(process.env.NODE_ENV !== 'production') { ... }) to invoke onBroadcastError
inside a local try/catch: call onBroadcastError(error, message) in the try, and
if it throws swallow or log that secondary error (fallback to the existing
console.warn behavior) so the original postMessage rejection handling remains
safe and cannot be escalated by a throwing callback.
---
Nitpick comments:
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 60-78: Add a test that simulates postMessage failing and the
supplied onBroadcastError itself throwing: mockPostMessage to reject with the
DOMException as before, pass an onBroadcastError mock that throws (e.g. throw
new Error('boom')), register a temporary process.on('unhandledRejection')
handler that fails the test if invoked, call broadcastQueryClient({ queryClient,
broadcastChannel: 'test_channel', onBroadcastError }), trigger the broadcast via
queryClient.setQueryData(['test'], { value: 1 }), await a microtask tick (e.g.
setTimeout 0 or next tick), then remove the unhandledRejection handler and
assert that onBroadcastError was called with the cloneError and that no
unhandled rejection handler fired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52ee48f6-f465-4bac-8951-d52f60b62ab6
📒 Files selected for processing (3)
.changeset/fix-broadcast-client-unhandled-rejection.mdpackages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
Wrap onBroadcastError() in a try/catch so that if the callback itself throws, the error does not escape as an unhandled rejection. Falls back to console.warn in development when the callback throws. Add a test asserting no unhandledRejection fires when onBroadcastError throws. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
60-91: ⚡ Quick winAdd a regression test for rejected async handlers.
This test covers sync throw, but a Promise-rejecting
onBroadcastErrorpath is different and worth locking down with a dedicated case.Suggested additional test
+ it('should not cause an unhandled rejection when onBroadcastError returns a rejected promise', async () => { + const cloneError = new DOMException('DataCloneError', 'DataCloneError') + mockPostMessage.mockRejectedValueOnce(cloneError) + + const unhandledRejections: Array<unknown> = [] + const onUnhandledRejection = (reason: unknown) => { + unhandledRejections.push(reason) + } + process.on('unhandledRejection', onUnhandledRejection) + + const onBroadcastError = vi.fn().mockRejectedValue(new Error('boom-async')) + + try { + broadcastQueryClient({ + queryClient, + broadcastChannel: 'test_channel', + onBroadcastError, + }) + + queryClient.setQueryData(['test-async'], { value: 1 }) + await new Promise((r) => setTimeout(r, 0)) + + expect(onBroadcastError).toHaveBeenCalledWith( + cloneError, + expect.objectContaining({ type: 'added' }), + ) + expect(unhandledRejections).toHaveLength(0) + } finally { + process.off('unhandledRejection', onUnhandledRejection) + } + })🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 60 - 91, Add a regression test that mirrors the existing "onBroadcastError throws" case but instead uses an async/rejecting handler: create an onBroadcastError stub that returns a rejected Promise (e.g. vi.fn().mockRejectedValue(new Error('boom'))), call broadcastQueryClient({ queryClient, broadcastChannel: 'test_channel', onBroadcastError }), trigger a broadcast via queryClient.setQueryData(['test'], { value: 1 }), capture process 'unhandledRejection' events into an array, wait a tick (setTimeout 0 or await Promise.resolve()) then assert the mock was called with the cloneError and expected message and that no unhandledRejections were recorded; reference onBroadcastError, broadcastQueryClient, and queryClient.setQueryData when locating where to add the test.
🤖 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.
Inline comments:
In `@packages/query-broadcast-client-experimental/src/index.ts`:
- Around line 42-51: safePost currently calls onBroadcastError(...) inside a
synchronous try/catch which misses async rejections; update safePost to capture
the return value of onBroadcastError (e.g., const result =
onBroadcastError(error, message)) and handle both sync and async errors by
calling Promise.resolve(result).catch(err => { if (process.env.NODE_ENV !==
'production') console.warn(`[broadcastQueryClient] failed to broadcast
"${message.type}" for queryHash "${message.queryHash}":`, err); }); so async
callbacks are properly caught and logged without causing unhandledRejection.
---
Nitpick comments:
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 60-91: Add a regression test that mirrors the existing
"onBroadcastError throws" case but instead uses an async/rejecting handler:
create an onBroadcastError stub that returns a rejected Promise (e.g.
vi.fn().mockRejectedValue(new Error('boom'))), call broadcastQueryClient({
queryClient, broadcastChannel: 'test_channel', onBroadcastError }), trigger a
broadcast via queryClient.setQueryData(['test'], { value: 1 }), capture process
'unhandledRejection' events into an array, wait a tick (setTimeout 0 or await
Promise.resolve()) then assert the mock was called with the cloneError and
expected message and that no unhandledRejections were recorded; reference
onBroadcastError, broadcastQueryClient, and queryClient.setQueryData when
locating where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2357c389-bb7f-4815-9fcc-0db5fcf17887
📒 Files selected for processing (2)
packages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
Support async onBroadcastError callbacks by calling .catch() on the returned Promise, preventing unhandled rejections when the callback rejects asynchronously. Also extracts the console.warn fallback into warnBroadcastError to deduplicate the warning logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
101-118: ⚡ Quick winEnsure the
unhandledRejectionlistener is always cleaned up.If this test throws before Line 117, the listener can leak into later tests. Wrap the body in
try/finallyand moveprocess.off(...)tofinally.Proposed fix
process.on('unhandledRejection', onUnhandledRejection) const onBroadcastError = vi .fn() .mockRejectedValueOnce(new Error('async boom')) - broadcastQueryClient({ - queryClient, - broadcastChannel: 'test_channel', - onBroadcastError, - }) - - queryClient.setQueryData(['test'], { value: 1 }) - - await new Promise((r) => setTimeout(r, 10)) - - process.off('unhandledRejection', onUnhandledRejection) - - expect(onBroadcastError).toHaveBeenCalledWith( - cloneError, - expect.objectContaining({ type: 'added' }), - ) - expect(unhandledRejections).toHaveLength(0) + try { + broadcastQueryClient({ + queryClient, + broadcastChannel: 'test_channel', + onBroadcastError, + }) + + queryClient.setQueryData(['test'], { value: 1 }) + + await new Promise((r) => setTimeout(r, 10)) + + expect(onBroadcastError).toHaveBeenCalledWith( + cloneError, + expect.objectContaining({ type: 'added' }), + ) + expect(unhandledRejections).toHaveLength(0) + } finally { + process.off('unhandledRejection', onUnhandledRejection) + }🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 101 - 118, The test registers an unhandledRejection listener (onUnhandledRejection) but only removes it after awaited work, which can leak if the test throws; wrap the test body that calls broadcastQueryClient({ queryClient, broadcastChannel: 'test_channel', onBroadcastError }) and the subsequent queryClient.setQueryData/await in a try/finally and move process.off('unhandledRejection', onUnhandledRejection) into the finally block so the listener is always cleaned up even on failures; keep the same onBroadcastError mock and onUnhandledRejection setup/teardown structure.
🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 101-118: The test registers an unhandledRejection listener
(onUnhandledRejection) but only removes it after awaited work, which can leak if
the test throws; wrap the test body that calls broadcastQueryClient({
queryClient, broadcastChannel: 'test_channel', onBroadcastError }) and the
subsequent queryClient.setQueryData/await in a try/finally and move
process.off('unhandledRejection', onUnhandledRejection) into the finally block
so the listener is always cleaned up even on failures; keep the same
onBroadcastError mock and onUnhandledRejection setup/teardown structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f614ddbd-4a69-46da-979b-42cb682551ca
📒 Files selected for processing (2)
packages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
…listener cleanup Prevents listener leaks into subsequent tests if an assertion throws before process.off() is reached. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
Fixed in the latest commit: wrapped both |
- Export BroadcastErrorEvent interface with QueryKey typing so consumers can import the type directly - Use discriminated union for internal BroadcastMessage (state only on updated) for more precise typing - onBroadcastError now receives BroadcastErrorEvent (no internal state field) instead of the raw BroadcastMessage - Differentiate dev warning messages: broadcast failures explain the structured-clone cause and consequence; onBroadcastError failures explicitly identify the hook as the source - Remove warnBroadcastError helper in favour of context-specific inlined warn calls - Fix duplicate queryClient.getQueryCache() call (use queryCache directly) - Add JSDoc to all BroadcastQueryClientOptions fields - Add onBroadcastError section to broadcastQueryClient docs with Sentry example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ows or rejects Add two tests that verify the '[broadcastQueryClient] onBroadcastError threw while handling...' console.warn message is emitted in development when the onBroadcastError callback itself throws synchronously or rejects asynchronously. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
163-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
console.warnwithtry/finallyto avoid spy leakage.If an assertion fails before
mockRestore(), the spy can leak into later tests and cause cascading failures.Proposed fix
it('should warn in dev when postMessage fails and onBroadcastError is not provided', async () => { process.env['NODE_ENV'] = 'development' const cloneError = new DOMException('DataCloneError', 'DataCloneError') mockPostMessage.mockRejectedValueOnce(cloneError) const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - broadcastQueryClient({ - queryClient, - broadcastChannel: 'test_channel', - }) - - queryClient.setQueryData(['test'], { value: 1 }) - - await new Promise((r) => setTimeout(r, 0)) - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('cross-tab sync for this query was skipped'), - cloneError, - ) - - warnSpy.mockRestore() + try { + broadcastQueryClient({ + queryClient, + broadcastChannel: 'test_channel', + }) + + queryClient.setQueryData(['test'], { value: 1 }) + + await new Promise((r) => setTimeout(r, 0)) + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('cross-tab sync for this query was skipped'), + cloneError, + ) + } finally { + warnSpy.mockRestore() + } }) it('should not warn in production when postMessage fails', async () => { process.env['NODE_ENV'] = 'production' const cloneError = new DOMException('DataCloneError', 'DataCloneError') mockPostMessage.mockRejectedValueOnce(cloneError) const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - broadcastQueryClient({ - queryClient, - broadcastChannel: 'test_channel', - }) - - queryClient.setQueryData(['test'], { value: 1 }) - - await new Promise((r) => setTimeout(r, 0)) - expect(warnSpy).not.toHaveBeenCalled() - - warnSpy.mockRestore() + try { + broadcastQueryClient({ + queryClient, + broadcastChannel: 'test_channel', + }) + + queryClient.setQueryData(['test'], { value: 1 }) + + await new Promise((r) => setTimeout(r, 0)) + expect(warnSpy).not.toHaveBeenCalled() + } finally { + warnSpy.mockRestore() + } })🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 163 - 204, Wrap the console.warn spy setup and assertions in a try/finally so the spy is always restored even if an assertion fails; specifically, in the two tests ("should warn in dev when postMessage fails and onBroadcastError is not provided" and "should not warn in production when postMessage fails") move the creation of warnSpy (vi.spyOn(console, 'warn').mockImplementation(() => {})) before the action, perform the broadcastQueryClient(...) and queryClient.setQueryData(...) and the expect(...) inside a try block, and call warnSpy.mockRestore() in the finally block to ensure the spy is cleaned up; keep references to mockPostMessage, broadcastQueryClient, queryClient, and warnSpy to locate the changes.
🧹 Nitpick comments (2)
docs/framework/react/plugins/broadcastQueryClient.md (1)
35-37: ⚡ Quick winUpdate the API call snippet to reflect the new option.
The snippet still shows only
queryClientandbroadcastChannel; consider includingonBroadcastError(or...options) so the top-level API example matches the expanded options surface.Suggested doc tweak
-broadcastQueryClient({ queryClient, broadcastChannel }) +broadcastQueryClient({ queryClient, broadcastChannel, onBroadcastError })🤖 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 `@docs/framework/react/plugins/broadcastQueryClient.md` around lines 35 - 37, Update the top-level API snippet for broadcastQueryClient to show the new options surface by including the onBroadcastError option (or a spread like ...options) in the call signature; change the example invocation of broadcastQueryClient({ queryClient, broadcastChannel }) to include onBroadcastError (or ...options) so it matches the expanded options in the implementation and docs.packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
139-161: ⚡ Quick winAdd rejection-path coverage for
updatedandremovedevents.The new failure-path assertions currently validate only the
'added'event shape. SincesafePostis used across outbound message types, add one rejection test each for'updated'and'removed'to lock the event contract and prevent branch-specific regressions.🤖 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-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 139 - 161, Add rejection-path tests for the other message types: duplicate the existing test that mocks mockPostMessage to reject (using the same cloneError and mockRejectedValueOnce) and assert onBroadcastError is called with that error and an event object whose type is 'updated' (triggered by updating an existing query via queryClient.setQueryData on the same key) and another test where type is 'removed' (triggered by removing the query via queryClient.removeQueries or setting data to undefined). Use the same helpers used in the current test: mockPostMessage, broadcastQueryClient({... , onBroadcastError }), and expect.objectContaining<BroadcastErrorEvent> to validate queryHash and queryKey shape so the failure path of safePost is covered for 'updated' and 'removed'.
🤖 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.
Inline comments:
In `@packages/query-broadcast-client-experimental/src/index.ts`:
- Line 19: Outbound "'added'" broadcast messages omit the source query state;
update the message shape to include a state field and ensure creators populate
it so receivers (the inbound 'added' handler that reads state) get the correct
data. Specifically, extend the 'added' payload type to include state (alongside
queryHash and queryKey) and modify the places that construct/send the 'added'
message (the two sites flagged around the existing message constructors) to pass
the source query's state (e.g., use the query's dehydrated/state value when
building the message) so the emitter and receiver contracts match.
---
Outside diff comments:
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 163-204: Wrap the console.warn spy setup and assertions in a
try/finally so the spy is always restored even if an assertion fails;
specifically, in the two tests ("should warn in dev when postMessage fails and
onBroadcastError is not provided" and "should not warn in production when
postMessage fails") move the creation of warnSpy (vi.spyOn(console,
'warn').mockImplementation(() => {})) before the action, perform the
broadcastQueryClient(...) and queryClient.setQueryData(...) and the expect(...)
inside a try block, and call warnSpy.mockRestore() in the finally block to
ensure the spy is cleaned up; keep references to mockPostMessage,
broadcastQueryClient, queryClient, and warnSpy to locate the changes.
---
Nitpick comments:
In `@docs/framework/react/plugins/broadcastQueryClient.md`:
- Around line 35-37: Update the top-level API snippet for broadcastQueryClient
to show the new options surface by including the onBroadcastError option (or a
spread like ...options) in the call signature; change the example invocation of
broadcastQueryClient({ queryClient, broadcastChannel }) to include
onBroadcastError (or ...options) so it matches the expanded options in the
implementation and docs.
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 139-161: Add rejection-path tests for the other message types:
duplicate the existing test that mocks mockPostMessage to reject (using the same
cloneError and mockRejectedValueOnce) and assert onBroadcastError is called with
that error and an event object whose type is 'updated' (triggered by updating an
existing query via queryClient.setQueryData on the same key) and another test
where type is 'removed' (triggered by removing the query via
queryClient.removeQueries or setting data to undefined). Use the same helpers
used in the current test: mockPostMessage, broadcastQueryClient({... ,
onBroadcastError }), and expect.objectContaining<BroadcastErrorEvent> to
validate queryHash and queryKey shape so the failure path of safePost is covered
for 'updated' and 'removed'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac035baa-9615-4fe3-b1ba-af6c95bf3b11
📒 Files selected for processing (3)
docs/framework/react/plugins/broadcastQueryClient.mdpackages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
The receiver's onmessage handler reads state for 'added' events to call query.setState(state) and queryCache.build(..., state), but the sender was not including state in the outbound message. This caused receiving tabs to apply undefined state for newly added queries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
Update summaryAfter reviewing other PRs addressing the same issue (#10569, #10770, #10697) and the CodeRabbit feedback, I've made additional improvements beyond the initial unhandled-rejection fix. Here's a full summary of what's in the branch now. Changes from the original PRTypes
Error handling in
Warning messages Two distinct messages to make debugging easier:
Bug fix: The receiver's Other
CodeRabbit review responses
Test coverage (9 tests)
|
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
…akage If an assertion fails before mockRestore(), the console.warn spy would leak into subsequent tests. Wrapping each spy block in try/finally ensures restore always runs regardless of test outcome. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
Addressed in the latest commit (52d1762). All four tests that call const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
try {
// ...test body and assertions...
} finally {
warnSpy.mockRestore()
}Applied to:
|
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
1 participant
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.
Summary
Fixes #10543
broadcastQueryClientcallschannel.postMessage()three times (onupdated,removed,added) without.catch()-ing the returned promise. When query data contains a non-cloneable value (ReadableStream,Response,File, functions, Vue/MobX reactive proxies, etc.), the rejection becomes an unhandledDataCloneErrorwith a stack pointing intonode_modules— effectively unactionable in Sentry/Datadog.Changes
channel.postMessage()calls in asafePost()helper that catches rejectionsonBroadcastErrorcallback toBroadcastQueryClientOptionsso consumers can route errors to their own error tracker with the offendingqueryHashconsole.warnin development whenonBroadcastErroris not providedUsage
Test plan
onBroadcastErroris called with the error and message whenpostMessagerejectsconsole.warnis emitted in development when noonBroadcastErroris providedpostMessagefails🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests