fix(query-broadcast-client-experimental): catch postMessage rejections to prevent unhandled DataCloneErrors#10980
Open
tsushanth wants to merge 1 commit into
Conversation
…s to prevent unhandled DataCloneErrors When query state contains a value the structured-clone algorithm cannot serialize (ReadableStream, Response, File, functions, Vue reactive proxies, MobX observables, etc.), every postMessage call on the three subscription paths throws a DataCloneError. Because the returned promise was never caught, these became unhandled rejections that polluted error trackers with stack traces pointing into node_modules and no indication of which query was at fault. This commit introduces a `safePost` helper that wraps all three `channel.postMessage()` call sites with a `.catch()`. By default the caught error is logged as a `console.warn` in non-production builds so developers still get actionable feedback (including the offending `queryHash`). Callers can supply an `onBroadcastError` option to route errors wherever they want (Sentry, Datadog, etc.) without forking the package. The `onBroadcastError` callback receives the raw error and the attempted `BroadcastMessage` object (which includes `type` and `queryHash`), giving enough context to filter noise or attach it to a specific query in user-land monitoring. Tests added: - asserts no `unhandledrejection` event fires when postMessage rejects - asserts `onBroadcastError` is called with the correct error and queryHash Fixes TanStack#10542
Contributor
📝 WalkthroughWalkthroughAdds a ChangesBroadcastQueryClient postMessage error handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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)
78-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the prototype patch in a
finallyblock.If any assertion or
setQueryDatabetween lines 80–86 throws,BC.prototype.postMessageis left monkey-patched, leaking the rejection behavior into later tests. Wrap the patched section so restoration always runs.♻️ Proposed fix
const { BroadcastChannel: BC } = await import('broadcast-channel') const originalPost = BC.prototype.postMessage BC.prototype.postMessage = () => Promise.reject(cloneError) - - queryClient.setQueryData(['key-for-error-test'], 'value') - - await new Promise((resolve) => setTimeout(resolve, 50)) - - BC.prototype.postMessage = originalPost + try { + queryClient.setQueryData(['key-for-error-test'], 'value') + await new Promise((resolve) => setTimeout(resolve, 50)) + } finally { + BC.prototype.postMessage = originalPost + }🤖 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 78 - 86, The test in the BroadcastChannel patch section leaves BC.prototype.postMessage monkey-patched if setQueryData or an assertion throws. Wrap the temporary override in a try/finally so the original postMessage is always restored, using the BroadcastChannel prototype patch around queryClient.setQueryData and the subsequent wait.
🤖 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/__tests__/index.test.ts`:
- Around line 35-45: The unhandledrejection listener added in the test setup is
never removed because afterEach passes a different anonymous function to
removeEventListener than the one registered in beforeEach. Update the test in
index.test.ts to store the handler in a named or scoped variable alongside
unhandledRejections, use that same reference for both addEventListener and
removeEventListener, and keep the listener lifecycle tied to each test.
---
Nitpick comments:
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 78-86: The test in the BroadcastChannel patch section leaves
BC.prototype.postMessage monkey-patched if setQueryData or an assertion throws.
Wrap the temporary override in a try/finally so the original postMessage is
always restored, using the BroadcastChannel prototype patch around
queryClient.setQueryData and the subsequent wait.
🪄 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: a30b5e43-9477-4163-9f59-6884b09af5f5
📒 Files selected for processing (2)
packages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
Comment on lines
+35
to
+45
| beforeEach(() => { | ||
| unhandledRejections = [] | ||
| window.addEventListener('unhandledrejection', (e) => { | ||
| unhandledRejections.push(e) | ||
| e.preventDefault() | ||
| }) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| window.removeEventListener('unhandledrejection', () => {}) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
unhandledrejection listener is never removed — leaks across tests.
removeEventListener is called with a brand-new () => {}, which is a different reference from the anonymous handler registered in beforeEach. The original listener is never detached, so each test in this file (and any subsequent suite sharing the same window) accumulates a stale listener still pushing into a captured unhandledRejections array. Capture the handler in a variable and remove that same reference.
🔧 Proposed fix
describe('postMessage error handling', () => {
let unhandledRejections: Array<PromiseRejectionEvent>
+ let onUnhandledRejection: (e: PromiseRejectionEvent) => void
beforeEach(() => {
unhandledRejections = []
- window.addEventListener('unhandledrejection', (e) => {
+ onUnhandledRejection = (e) => {
unhandledRejections.push(e)
e.preventDefault()
- })
+ }
+ window.addEventListener('unhandledrejection', onUnhandledRejection)
})
afterEach(() => {
- window.removeEventListener('unhandledrejection', () => {})
+ window.removeEventListener('unhandledrejection', onUnhandledRejection)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| unhandledRejections = [] | |
| window.addEventListener('unhandledrejection', (e) => { | |
| unhandledRejections.push(e) | |
| e.preventDefault() | |
| }) | |
| }) | |
| afterEach(() => { | |
| window.removeEventListener('unhandledrejection', () => {}) | |
| }) | |
| let unhandledRejections: Array<PromiseRejectionEvent> | |
| let onUnhandledRejection: (e: PromiseRejectionEvent) => void | |
| beforeEach(() => { | |
| unhandledRejections = [] | |
| onUnhandledRejection = (e) => { | |
| unhandledRejections.push(e) | |
| e.preventDefault() | |
| } | |
| window.addEventListener('unhandledrejection', onUnhandledRejection) | |
| }) | |
| afterEach(() => { | |
| window.removeEventListener('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 35 - 45, The unhandledrejection listener added in the test setup is
never removed because afterEach passes a different anonymous function to
removeEventListener than the one registered in beforeEach. Update the test in
index.test.ts to store the handler in a named or scoped variable alongside
unhandledRejections, use that same reference for both addEventListener and
removeEventListener, and keep the listener lifecycle tied to each test.
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
2 participants
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.
Fixes #10542
Problem
broadcastQueryClientsubscribes to the query cache and callschannel.postMessage()on three paths (updated,removed,added). The returned promise was never caught, so any value the structured-clone algorithm cannot serialize —ReadableStream,Response,File, functions, Vue reactive proxies, MobX observables, etc. — causes aDataCloneErrorthat surfaces as an unhandled rejection:The stack points into
node_moduleswith no indication of which query triggered it, making the error effectively unactionable in Sentry / Datadog. Cross-tab sync for that update is silently dropped while the error tracker accumulates noise.Solution
Introduce a
safePosthelper that wraps all threechannel.postMessage()call sites in.catch(). The default behavior is aconsole.warnin non-production builds that includes the eventtypeand offendingqueryHash, making the failure locally actionable without polluting production error trackers.Callers who want to route errors to their own monitoring can supply an
onBroadcastErrorcallback:onBroadcastErrorreceives the raw error and the attemptedBroadcastMessage(includingtypeandqueryHash), giving enough context to filter noise or tie the failure back to a specific query.Changes
src/index.ts— addsBroadcastMessagetype,onBroadcastErroroption, andsafePosthelper; replaces the three barechannel.postMessage()callssrc/__tests__/index.test.ts— adds two new tests: one asserting nounhandledrejectionfires when postMessage rejects, and one assertingonBroadcastErroris called with the correct error andqueryHashAll four tests pass.
Summary by CodeRabbit