fix(query-broadcast-client-experimental): stop leaking postMessage rejections as unhandled errors#10569
Open
senutpal wants to merge 2 commits intoTanStack:mainfrom
Open
fix(query-broadcast-client-experimental): stop leaking postMessage rejections as unhandled errors#10569senutpal wants to merge 2 commits intoTanStack:mainfrom
senutpal wants to merge 2 commits intoTanStack:mainfrom
Conversation
…jections as unhandled errors
github-actions
Bot
added
documentation
Contributor
📝 WalkthroughWalkthroughAdds guarded broadcasting: BroadcastChannel.postMessage calls are wrapped in a safePost helper that catches structured‑clone/postMessage rejections and forwards them to an optional onBroadcastError hook or emits a development-time console.warn, preventing unhandled rejections and skipping sync for the failing query. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client (QueryClient)
participant BC as BroadcastChannel
participant Hook as onBroadcastError / Logger
App->>BC: send broadcast message (safePost)
alt postMessage resolves
BC-->>App: resolved
else postMessage rejects (DataCloneError)
BC-->>App: rejection (error)
App->>Hook: call onBroadcastError(error, BroadcastErrorEvent)
alt hook throws or absent
App->>Hook: console.warn (dev only) rgba(255,165,0,0.5)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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)
docs/framework/react/plugins/broadcastQueryClient.md (1)
52-65: Optional: reference the exportedBroadcastErrorEventto avoid drift.The inlined event shape duplicates the exported
BroadcastErrorEventinterface frompackages/query-broadcast-client-experimental/src/index.ts(lines 10–14). If the event type grows a field, this docs snippet will silently fall out of sync. Since the surrounding snippet already usesQueryKeywithout importing it, it's fine to keep it type-level and point at the exported name.📚 Proposed tweak
- /** Called when a query event cannot be broadcast to other tabs — - * most commonly because the query's `state.data`, `state.error`, or - * `queryKey` contains a value the structured-clone algorithm cannot - * serialize (e.g. `ReadableStream`, `File`, functions, framework - * proxies). Useful for routing failures to an error tracker. */ - onBroadcastError?: ( - error: unknown, - event: { - type: 'added' | 'removed' | 'updated' - queryHash: string - queryKey: QueryKey - }, - ) => void + /** Called when a query event cannot be broadcast to other tabs — + * most commonly because the query's `state.data`, `state.error`, or + * `queryKey` contains a value the structured-clone algorithm cannot + * serialize (e.g. `ReadableStream`, `File`, functions, framework + * proxies). Useful for routing failures to an error tracker. + * See `BroadcastErrorEvent`. */ + onBroadcastError?: (error: unknown, event: BroadcastErrorEvent) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/react/plugins/broadcastQueryClient.md` around lines 52 - 65, The docs inline event type for onBroadcastError duplicates the exported BroadcastErrorEvent interface and may drift; update the onBroadcastError signature to reference the exported BroadcastErrorEvent (instead of the inline object type) so it stays in sync with packages/query-broadcast-client-experimental's BroadcastErrorEvent, keeping QueryKey as-is and preserving the parameter order (error: unknown, event: BroadcastErrorEvent) in the onBroadcastError declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/query-broadcast-client-experimental/src/index.ts`:
- Around line 69-90: The .catch in safePost may reintroduce unhandled rejections
if onBroadcastError throws; modify the catch block in safePost to invoke
onBroadcastError inside a try/catch and swallow or locally log any error thrown
by the user handler so the promise chain never rejects due to a user callback
(keep the existing process.env.NODE_ENV console.warn behavior for
non-production); reference safePost, onBroadcastError, and the constructed event
object when locating the change. Also add a test variant to the existing "does
not surface broadcast failures as unhandled rejections" test that passes an
onBroadcastError which throws to assert no unhandled rejection occurs.
---
Nitpick comments:
In `@docs/framework/react/plugins/broadcastQueryClient.md`:
- Around line 52-65: The docs inline event type for onBroadcastError duplicates
the exported BroadcastErrorEvent interface and may drift; update the
onBroadcastError signature to reference the exported BroadcastErrorEvent
(instead of the inline object type) so it stays in sync with
packages/query-broadcast-client-experimental's BroadcastErrorEvent, keeping
QueryKey as-is and preserving the parameter order (error: unknown, event:
BroadcastErrorEvent) in the onBroadcastError declaration.
🪄 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: ed021583-1d1a-4711-b89f-212c355bdeb9
📒 Files selected for processing (4)
.changeset/fix-broadcast-client-postmessage.mddocs/framework/react/plugins/broadcastQueryClient.mdpackages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/query-broadcast-client-experimental/src/index.ts (1)
40-43: Optional: deriveBroadcastMessagefromBroadcastErrorEventto avoid drift.The three
type/queryHash/queryKeyfields are now duplicated betweenBroadcastMessage(internal wire shape) and the exportedBroadcastErrorEvent. They must stay in sync becausesafePostconstructs the latter from the former at lines 71–75. A small shared base would make future shape changes safer, but this is purely a nit on the current diff.♻️ Possible shape
-type BroadcastMessage = - | { type: 'added'; queryHash: string; queryKey: QueryKey } - | { type: 'removed'; queryHash: string; queryKey: QueryKey } - | { type: 'updated'; queryHash: string; queryKey: QueryKey; state: unknown } +type BroadcastMessage = + | ({ type: 'added' } & Omit<BroadcastErrorEvent, 'type'>) + | ({ type: 'removed' } & Omit<BroadcastErrorEvent, 'type'>) + | ({ type: 'updated'; state: unknown } & Omit<BroadcastErrorEvent, 'type'>)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-broadcast-client-experimental/src/index.ts` around lines 40 - 43, BroadcastMessage duplicates fields with the exported BroadcastErrorEvent; unify them to avoid drift by extracting a shared base type (e.g., BroadcastBase or reuse BroadcastErrorEvent's common fields) and have BroadcastMessage extend that base (or derive BroadcastMessage from BroadcastErrorEvent) so the common properties type/queryHash/queryKey are defined once; update the union variants (types 'added'|'removed'|'updated') to extend the shared base and ensure safePost (which constructs BroadcastErrorEvent from BroadcastMessage) still type-checks against the new shared type.packages/query-broadcast-client-experimental/src/__tests__/index.test.ts (1)
148-178: Tighten the assertion so the test distinguishes the two warn paths.Today this test only asserts
warn.toHaveBeenCalled(). Both theonBroadcastError threwbranch (lines 86–89 inindex.ts) and the no-hook fallback branch (lines 96–100) log throughconsole.warn, so a regression that made the fallback fire while the hook was also wired would still pass this test. Assert on the specific "onBroadcastError threw" message so the test actually proves the guarded branch ran.Also,
await new Promise(r => setTimeout(r, 20))here and at lines 140–141 is a timing gamble —vi.waitFor(() => expect(warn).toHaveBeenCalled())(for this test) and waiting onpostMessagecall count (for the previous one) would be deterministic; then a single trailing microtask flush is enough before theonUnhandledassertion.🧪 Proposed tweak
- await new Promise((resolve) => setTimeout(resolve, 20)) - - expect(onUnhandled).not.toHaveBeenCalled() - expect(warn).toHaveBeenCalled() + await vi.waitFor(() => { + expect(warn).toHaveBeenCalled() + }) + // Flush one more turn so any would-be unhandledRejection has fired. + await new Promise((resolve) => setImmediate(resolve)) + + expect(onUnhandled).not.toHaveBeenCalled() + expect(warn.mock.calls[0]?.[0]).toEqual( + expect.stringContaining('onBroadcastError threw'), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts` around lines 148 - 178, Tighten the test to assert the specific "onBroadcastError threw" warning and make the wait deterministic: replace the loose await new Promise(setTimeout) with vi.waitFor(() => expect(warn).toHaveBeenCalledWith(expect.stringContaining('onBroadcastError threw'))) to ensure the guarded branch logged the specific message, then await Promise.resolve() (a single microtask flush) before asserting on onUnhandled; keep the existing console.warn spy and process.on/off setup and reference this change in the test named 'does not surface failures even when onBroadcastError itself throws' and the warn spy variable `warn`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-broadcast-client-experimental/src/__tests__/index.test.ts`:
- Around line 148-178: Tighten the test to assert the specific "onBroadcastError
threw" warning and make the wait deterministic: replace the loose await new
Promise(setTimeout) with vi.waitFor(() =>
expect(warn).toHaveBeenCalledWith(expect.stringContaining('onBroadcastError
threw'))) to ensure the guarded branch logged the specific message, then await
Promise.resolve() (a single microtask flush) before asserting on onUnhandled;
keep the existing console.warn spy and process.on/off setup and reference this
change in the test named 'does not surface failures even when onBroadcastError
itself throws' and the warn spy variable `warn`.
In `@packages/query-broadcast-client-experimental/src/index.ts`:
- Around line 40-43: BroadcastMessage duplicates fields with the exported
BroadcastErrorEvent; unify them to avoid drift by extracting a shared base type
(e.g., BroadcastBase or reuse BroadcastErrorEvent's common fields) and have
BroadcastMessage extend that base (or derive BroadcastMessage from
BroadcastErrorEvent) so the common properties type/queryHash/queryKey are
defined once; update the union variants (types 'added'|'removed'|'updated') to
extend the shared base and ensure safePost (which constructs BroadcastErrorEvent
from BroadcastMessage) still type-checks against the new shared type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eefd5dfc-a53a-4d1a-a8d2-d99555e36598
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/framework/react/plugins/broadcastQueryClient.md
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.
changes
fixes #10543 (also closes duplicate #10542).
BroadcastChannel.postMessagereturns aPromise<void>that rejects when the payload cannot be structured-cloned - for example when a query'sstate.data,state.error, orqueryKeycontains aReadableStream,File,Request/Response, function, or framework proxy (vuereactive, mobx observables). the threechannel.postMessage(...)call sites inside thequeryCache.subscribecallback never attached a.catchhandler, so every non-cloneable update escaped as anunhandledrejectionwith a stack trace pointing only intonode_modules/broadcast-channel/...and noqueryHashcontext.the fix wraps all three call sites in a single
safePosthelper that:.catchso the rejection never bubbles as anunhandledrejection, even when a user-suppliedonBroadcastErroritself throws (the hook call is wrapped in try/catch and the handler error is logged in dev).{ type, queryHash, queryKey }metadata to a new optionalonBroadcastErrorhook onBroadcastQueryClientOptions, for consumers who want to forward to sentry/datadog.console.warnwhen no hook is provided, so cross-tab sync failures are never entirely silent.the wire format of the broadcast messages is unchanged, so tabs running old and new code stay compatible during a roll-out. related: #8356 was closed with a user-side
toRaw()workaround that does not generalize toReadableStream/File/ function payloads; this change supersedes that guidance at the library level.checklist
release impact
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests