Repository navigation
Report global errors once with several Providers or StrictMode (#108) - #171
devtools-agent[bot] wants to merge 8 commits into
Conversation
Every Provider built its own Rollbar instance in its constructor, and every instance with captureUncaught / captureUnhandledRejections adds its own window.onerror and unhandledrejection handler. rollbar.js only filters duplicates within one instance, so one error was reported once per Provider (#108). Under StrictMode a single Provider reported twice, because React constructs class components twice in development and the discarded instance's handlers stay installed. - Create the instance on first render instead of in the constructor, so the constructor StrictMode discards never builds one. - Coordinate global capture between Provider-created instances: the first one to render owns each kind of capture; later ones are built with it off and take over when the owner unmounts. The handoff flips the options through configure(), which rollbar.js checks on every call. Instances passed via `instance` aren't touched. Fixes #108 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Coordinates global error capture across Providers so that only one Provider-created Rollbar instance handles window.onerror / unhandledrejection at a time. It also moves instance creation from the constructor into the first render, so StrictMode's thrown-away constructor call doesn't build a second instance.
What I traced against the installed rollbar 3.1.0 and React 17.0.2 source:
configure()merges options (utility.js:791-801) and re-runssetupUnhandledCapture()(core.js:111-135).- A disabled instance's handlers stay on
windowbut return early (core.js:234-240,338-344), so turning off the previous owner stops duplicate reports. - The takeover, remount, nested and StrictMode paths behave as the new tests expect. I traced the tests by hand; I have not seen them run.
One issue: the instance and its claim are created during render, and a claim is only released when its Provider unmounts. If React throws away a Provider's first render, the claim stays pending forever. That instance keeps capturing global errors, and the Provider that actually mounts never takes over. Details are in the finding.
Minor, not flagged: Provider no longer sets this.state, so ref.current.state.rollbar, which the old ProviderState typing exposed, is now undefined at runtime. It was never documented API, but it is an observable change.
A Provider mounting next to a pending owner only waited for it, since a pending owner could be an ancestor that mounts later in the same commit. But if React threw that owner's render away (for example a concurrent root re-rendering after a caught render error), it never mounts or unmounts: its instance kept reporting uncaught errors without the person or context set on the provided instance, and no later Provider on the page ever took over. Each claim now records the enclosing Provider's claim, passed down through context. On mount, a pending owner keeps capture only if it is an ancestor; any other pending owner is replaced like an unmounted one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Reviewed the new src/global-capture.js coordination, the Provider changes, the README text and the new tests against the checkout and the installed rollbar@3.1.0 / react@17.0.2.
What holds up
- Creating the instance in
render()instead of the constructor fixes the StrictMode double instance on React 17. The throwaway instance never renders, and the doublerender()reuses the cachedthis.rollbar. - The PR switches capture on and off with
rollbar.configure(...). In rollbar 3.1.0 afalseflag overridestrue(utility.js:858-866), a switched-off instance's handler returns immediately (core.js:234-240,338-344), and switching capture back on installs the handlers (core.js:133,208-232). - One Provider at a time owns capture, both across siblings (earlier renders mount first) and when nested (children mount before parents).
Issue (low)
- If nested Providers are both inside a render React throws away (an error boundary that remounts its children, like the test's
Retry), the inner Provider ends up owning global capture and the outer one waits. That contradicts the nested test, the comment inmountGlobalCaptureand the README. Errors are still reported only once, but under the inner Provider's config.
Minor notes (not raised as findings)
- If a custom
Rollbarconstructor throws right after its claim switched off an unmounted owner, nothing captures global errors until another Provider claims. ProviderStateis gone fromindex.d.ts; nothing in the repo references it.
The tests were not run as part of this review.
When a render holding nested Providers was thrown away, the inner Provider that mounted took over global capture, because it mounts before its parent and the stale owner wasn't its ancestor. It now hands capture to its outermost enclosing Provider that captures the same kind, which mounts later in the same commit. The same thing happened with nested Providers under StrictMode on React 18 and 19 (createRoot), with no thrown-away render: StrictMode unmounts and remounts every component after the first mount, and the owner's unmount handed capture to its own child. An unmounting owner now skips Providers nested inside it, since they unmount or disappear with it, and hands capture to the outermost waiting Provider around the one it picks. If a custom Rollbar constructor throws after its claim switched off an unmounted owner, that owner is now switched back on instead of leaving nothing capturing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Report global errors once with several Providers or StrictMode (#108)
The coordination is correct in every scenario I traced. I read src/global-capture.js and src/provider.js in full and walked through React 17 legacy order, React 18 StrictMode (children unmount first, then remount) and React 19 StrictMode (parents disappear first, children reappear first). I also covered siblings, nesting (including through an instance Provider), renders thrown away by an error boundary or Suspense, a constructor that throws (including React 17's DEV replay), and interleaved commits across multiple roots. Two things held throughout:
- At most one managed instance has each kind of capture enabled at a time.
- Every path that disables an owner also enables a replacement (
takeOver, or the unmounted-owner branch ofclaimGlobalCapture), andabandonGlobalCaptureputs the previous owner back.
Switching capture through configure works in the pinned Rollbar (3.1.0). configure re-runs setupUnhandledCapture (node_modules/rollbar/src/browser/core.js:111-135, :208-232). handleUncaughtException and handleUnhandledRejection check the options each time they run (core.js:234-240, :338-344). The window.onerror chaining (node_modules/rollbar/src/browser/globalSetup.js:1-27) explains the original duplicate reports. The peer range also allows rollbar ^2.26.4, which isn't installed here, so I couldn't check 2.x.
Behaviour changes worth noting (none of them defects):
- Several Providers with different access tokens no longer all get uncaught errors. Only the owner reports them. This is what #108 asks for, and the README documents it.
Providerno longer hasthis.state, at runtime or inindex.d.ts. Code that readref.current.state.rollbarwill break. That was undocumented, and nothing in this repo uses it.- The ownership state is module-level, so it only coordinates Providers that share one copy of the package.
Outside the changed files: the experimental, unexported useScopedConfiguration (src/hooks/use-scoped-rollbar-config.js:18) builds a new instance from base.options. Used under the owner, that instance would capture global errors too.
One finding (low): the new static contextType makes nested Providers re-render more often (see below).
I did not run the tests; nothing here shows whether they pass.
Provider read the enclosing Provider's claim from `Context`, whose value is a new object on every render. That subscribed each nested Provider to its parent, so a parent re-render forced every inner Provider, and the `useRollbar` consumers under it, to re-render as well. The claim now travels in a private context whose value is the claim itself, which doesn't change after a Provider's first render. `Context` carries what it did before this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AI Agent Review (openai, openai-astra)Review: global capture coordination across Providers (#108)No defects found in the changed lines. I checked the mechanism against the actual code and the pinned
I did not run the test suite or lint. Nothing here claims they pass. Worth deciding deliberately (not defects in this diff)
|
|
[P2] Preserve runtime capture settings during ownership handoff
Reproduction:
The coordinator should distinguish the application’s current requested settings from its own temporary suppression of capture, and preserve those requested settings during a handoff. |
waltjones
left a comment
There was a problem hiding this comment.
Blocking on the issue @aborek-rollbar raised.
What I ran: I used the PR's built package on React 18.3.1 and 19.2.0, without StrictMode, which matches production. The main column comes from reading the code, not from running it.
┌────────────────────────────────────────────────────────────────┬───────────────────────────────────────────────────────────────────────────────┬─────────────────────────────────────┐
│ Case │ PR 171 │ main │
├────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────┤
│ Reviewer's repro: B opts out with useRollbarConfiguration, │ B reports both errors, despite the opt-out, until the component that opted │ B stays silent; A's instance keeps │
│ then A unmounts │ out happens to re-render │ reporting │
├────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────┤
│ The capturing Provider (A) opts out while B is mounted │ Nothing reports. B never takes over │ B reports │
├────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────┤
│ B, not capturing, opts in at runtime │ The uncaught error is reported twice, by A and B │ Twice (the existing bug) │
└────────────────────────────────────────────────────────────────┴───────────────────────────────────────────────────────────────────────────────┴─────────────────────────────────────┘
With StrictMode in development it gets more confusing. StrictMode unmounts and remounts everything once after mounting, and that runs the handoff code. The handoff puts every Provider back to its original capture settings, which undoes any opt-in or opt-out made during the first render. So development and production behave differently.
Why it happens: the coordinator saves the capture settings when the Provider is created (claim.captures) and switches them through configure(). The app changes the same two options through the same configure(), using the public useRollbarConfiguration (typed at index.d.ts:79) or useRollbar().configure(...). Neither side sees the other's changes, so whichever writes last wins.
Severity: P2 is fair. Apps that set the capture flags once in config are fine. Still, an opt-out that's silently undone matters, since an app may turn capture off on purpose. In the second row, global errors stop being reported at all. The fix only touches global-capture.js and provider.js, so I'd do it in this PR rather than as a follow-up.
Suggested fix: let the coordinator own the capture flags on the instances Providers create, by wrapping each instance's configure:
- Record requests: capture keys in an app's configure call become that Provider's requested settings, replacing what was saved at creation.
- Apply only when owning: apply them only if that Provider currently captures that kind of error; otherwise keep them off.
- Opting out: a capturing Provider that opts out hands capture to the next waiting Provider, the same as on unmount. A waiting Provider that has opted out is skipped at handoff.
- Internal switching: enable and disable call the original configure, not the wrapped one.
- Providers that start without capture: they don't get a claim today (claimGlobalCapture returns null), so a later opt-in on one would still double-report. They need a claim too.
Fixing only the hook isn't enough, because useRollbarConfiguration is just a two-line wrapper around rollbar.configure. A direct call would hit the same bug.
The three rows above would make good regression tests in src/tests/components/global-capture.test.tsx. React 17 can reproduce all of them without StrictMode. They're also in my harness at /private/tmp/claude-501/-Users-waltjones-projects-rollbar-rollbar-react/797d7152-fa2f-447d-898e-57a72dbe421f/scratchpad/harness/run.cjs, as runtimeOptOut, ownerOptOut and runtimeOptIn.
The coordinator saved each Provider's capture options when it built the instance and restored them on every handoff, while the app changes the same options through configure() or useRollbarConfiguration(). Whichever wrote last won: an opt-out was undone when the owner unmounted, an owner that opted out left nothing capturing, and a Provider that opted in reported every error a second time. Each managed instance's configure is now wrapped. Capture options passed to it become what that Provider wants, and are applied only while it owns that kind of capture. An owner that opts out hands capture to the next mounted Provider that wants it, Providers that opted out are skipped at handoff, and a Provider that opts in captures only once nobody else does. Internal switching calls the original configure. Providers that start without capture now get a claim too, so a later opt-in is coordinated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressing the CHANGES_REQUESTED review from waltjones and the P2 comment from aborek-rollbar (runtime capture settings undone at handoff). Fixed in f798951, following the suggested design. What changed (
Regression tests. The three rows of the table are now in Built
All four modes give the same numbers. The earlier scenarios are unchanged: three StrictMode siblings (A reports, then B after A and C unmount), nested Providers, and a nested render thrown away by a boundary above them (the outer instance reports). Not covered: capture options assigned to |
AI Agent Review (openai, openai-astra)No defects found; I did not run the tests. Checked against the pinned rollbar 3.1.0 (
Coordinator logic ( Scenarios traced by hand, all with exactly one instance capturing: sibling Providers; nested Providers, including through an
Context (not findings):
|
Addresses the review on #171. Instead of switching capture on and off through configure, each instance a Provider creates now checks, when its uncaught-error or rejection handler runs, whether it is the one to report: of the instances with the same access token that capture that kind, the first-created mounted one; with none mounted, the newest that has mounted, else the newest. The choice is made from current options each time. - Scoped per accessToken, so Providers for different projects each report. - configure and the app's options are untouched; state.rollbar is kept (instance is created in the constructor again; ProviderState restored). - A shared instance from a custom constructor is managed once. - Same owner with or without StrictMode; nothing is enabled at teardown. - Chrome anonymous errors reach the reporting instance. - Component tests use a token per test so each starts from a clean page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
AI Agent Review (anthropic, CLI default model; requested openai/openai-astra)
Scrutiny: high (failure path)
Before this change, an app with more than one <Provider config={…}> that captures uncaught errors or unhandled promise rejections reported each crash once per Provider, so one crash showed up as two, three or more occurrences. In development, <React.StrictMode> caused even a single Provider to report twice, because React builds a throwaway extra copy of it.
After this change, Providers that report to the same project (the same access token) pick one of themselves to report each error. That is the outermost or first-rendered Provider still on the page. If none is on the page any more, the most recent one that was takes over, so errors are still reported after the app unmounts. Turning capture on or off while the app runs takes effect from the next error.
| Situation | Before | After |
|---|---|---|
| Several Providers, same project | one report per Provider | one report |
| One Provider under StrictMode (development) | two reports | one report |
| Providers for different projects (e.g. micro-frontends) | one report each | one report each (unchanged) |
Provider given a ready-made instance, or Rollbar.init |
not coordinated | not coordinated (unchanged) |
| Server rendering | no capture | no capture (unchanged) |
This also changes which settings apply. For global errors, only the chosen Provider's own settings decide whether and how the error is sent: its environment, context, checkIgnore filter and so on. An inner Provider configured with a more specific context or a looser filter no longer sends its own copy. That is the intent of the change, but a sentence about it in the README would save users some surprise.
Conclusion: I found no blocking problem. There is one low-severity gap. In a mixed setup, a single error can still be reported twice once the Providers involved have all left the page. The setup is one Rollbar instance shared by several Providers through a custom constructor, plus another Provider for the same project that builds its own instance. The inline comment walks through it.
How I reached this. I read the new coordination module, the Provider changes, and the global-error code of rollbar.js 3.1.0, which is the version pinned in package-lock.json. Rollbar installs a separate handler for each instance and chains them, so every instance sees every error. Each handler is looked up on the instance at the moment the error happens, which is why wrapping it per instance works. I traced these situations by hand through the code:
- sibling and nested Providers
- StrictMode's extra copy, and the unmount-then-remount order of React 18 and 19
- a render thrown away by an error boundary
- disabled instances
- turning capture on or off at runtime
- different access tokens
- one shared instance
- everything unmounting
- Chrome's anonymous cross-origin errors
Apart from the mixed case above, each gave exactly one report per project. The common case of a single Provider in production behaves as before.
The description lists some known limitations, with no pull request or ticket behind them. The code confirms each one matches how things worked before this change, so none is a regression. They are: Providers given an instance, separate copies of the package in different bundles, and Chrome anonymous errors going to only one project.
One remaining risk, which I did not raise as a finding because it needs a misbehaving third-party script: this approach assumes every Provider's handler is still being called. Suppose a script replaces window.onerror without passing errors on. Providers created after that would defer to an earlier Provider that no longer receives errors, and the error would be lost. Before this change, those later Providers reported it themselves.
Limits: I read the new tests but did not run them. I could not check the React 18/19 browser results in the description beyond reading the unit tests that simulate those mount orders.
A shared instance (custom Rollbar constructor) and a separate instance for the same token could both report an error once all their Providers had unmounted. Each instance added its own newest claim to the candidates, and unmounting dropped the newer claims of a shared instance in favour of the one just unmounted, so the two instances disagreed. - reporter() chooses only from the registered claims, so every instance reaches the same answer; an instance with none registered reports only when no other instance for its token does. - On unmount, an instance keeps its newest claim that has mounted, so the result no longer depends on unmount order and StrictMode's extra construction can't stand in for it. - README: the reporting Provider's options decide how the error is sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #108.
What was wrong
Each
<Provider config={…}>builds its own Rollbar instance. Every instance withcaptureUncaught/captureUnhandledRejectionsadds its ownwindow.onerror/unhandledrejectionhandler, and rollbar.js only filters duplicates within one instance. So one uncaught error or unhandled rejection was reported once per Provider. Under<React.StrictMode>a single Provider reported twice, because React constructs class components twice in development, throws one away, and the discarded instance's handlers stay installed.What changed
Each instance a Provider creates now decides, at the moment an uncaught error or rejection arrives, whether it is the one that reports it. The app's options and
configureare left alone; nothing is switched on or off.accessTokenare deduplicated, so Providers for different Rollbar projects (micro-frontends, say) each still get every error. Among the instances for one token that currently capture that kind of error (and aren'tenabled: false), the one whose Provider was created first among those mounted reports. That Provider's own options (environment,checkIgnore, …) decide how the error is sent. Parents are created before children and earlier siblings before later ones, so the outer or first Provider reports. While none is mounted (all unmounted, or before the first commit), the newest one that has mounted reports; failing that, the newest one, which covers StrictMode's extra construction.configure/useRollbarConfigurationopt-ins and opt-outs are respected immediately. The result doesn't depend on the order Providers mounted or unmounted in, so StrictMode and production pick the same instance.src/global-capture.js(new, ~160 lines): wraps each managed instance'shandleUncaughtExceptionandhandleUnhandledRejection(the methods rollbar.js's global handlers call) with that check. An instance shared by several Providers (a customRollbarconstructor returning a singleton) is wrapped once. Every instance chooses from the same registered Providers, so they always agree on which one reports. Chrome's anonymous errors are counted by every instance as before, and the one instance whoseError.prepareStackTracereceives the error hands it to the reporting instance.src/provider.js: still creates the instance in the constructor and keepsstate.rollbar/state.options, so nothing changes for code that reads them. It registers the instance with the coordinator and reports mount and unmount.configis resolved withvalue()fromutils.index.d.ts: unchanged frommain.README.md: short note on the behaviour, including whose options apply.Skipped during SSR (no
window) and for instances passed with theinstanceprop.Validation
src/tests/components/global-capture.test.tsx, 21 cases (React 17): sibling Providers, StrictMode, handoff on unmount, keyed remount, nesting (with StrictMode, through aninstanceProvider), renders thrown away by an error boundary (single and nested), a throwing constructor, no extra re-renders of nested Providers, and the runtime opt-in/opt-out cases. New in this revision: one report per project with different tokens, a shared instance from a custom constructor,state.rollbarkept,configureand options left untouched, an outer Provider that opts in, one report after every Provider unmounts, and anonymous errors in Chrome. Each test uses its own access token and resetswindow.onerror, so it starts from an empty page. Onmain, 13 of the 21 fail; on the previous head of this PR (f499118), 7 fail.src/tests/global-capture.test.js, 13 cases driving the coordinator in the React 18 and 19 StrictMode mount/unmount orders, plus per-project scoping, disabled instances, shared instances, teardown, and StrictMode's extra construction not winning the fallback. New in this revision: a shared instance plus a separate instance for the same token, unmounted in either order and with a never-mounted StrictMode construction, and an instance that turns capture back on after every Provider unmounted. Each reports once; all 4 reported twice or picked the wrong instance on the previous head (d83c9ab).npm run lint,npm run typecheck,npm run buildandprettier --checkon the changed files are clean.lib/on React 18.3.1 and 19.2.0createRootwith the rollbar 3.1.0 browser build. Each step fires one uncaught error plus one rejection, and every scenario ran with StrictMode on and off, with identical results:mainroot.unmount()with 3 ProvidersKnown limitations
instanceprop, or created withRollbar.init, aren't coordinated; create them once and share them.Error.prepareStackTracelast, as before this change.🤖 Generated with Claude Code