Repository navigation
Fix RollbarContext without a payload (#102), make onRender work and document it (#88) - #166
devtools-agent[bot] wants to merge 14 commits into
Conversation
…ocument it (#88) #102: rollbar.js has no default `payload` option, so RollbarContext and useRollbarContext threw "Cannot read properties of undefined (reading 'context')" unless the config set one. Read it with `?.`. Restoring the unset context on unmount now uses '' instead of undefined, which configure() ignores, so the context used to stay set after unmount. rollbar.js sends '' for an unset context anyway. #88: by default the context is set on mount, after the children have rendered, so errors an ErrorBoundary catches during the first render are reported with the previous context. `onRender` sets it first, but it was undocumented and had bugs: - it called setState during render, which React warns about - it ignored later changes to the `context` prop The previous context is now kept on the instance. componentDidUpdate applies a changed `context` under onRender. The README documents onRender, when to use it and why it isn't the default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tests were plain JS only because index.d.ts on main had no `onRender`. Stacked on #162 they type-check as TSX, like the other component tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6f02701 to
e5456cd
Compare
|
[P2] Preserve the innermost context when an outer context updates — With nested |
Review feedback: with nested onRender contexts, changing the outer `context` prop overwrote the inner one that was still mounted. React runs update lifecycles child-first, so the outer component applied its value last. Nesting was already broken without onRender and in the hook: children mount first, so the outer context won at mount, and unmounting restored in the wrong order, leaving the inner context set. The component and the hook now share a list of active contexts per Rollbar client (src/context-stack.js). Each entry takes an order number on first render, which ranks parents before children, and the innermost active entry is applied on every change. When the list empties, the context from before the first entry is restored. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Thanks, confirmed and fixed in d722ee7. This PR caused the
Fix: the component (with or without
Regression test:
🤖 Generated with Claude Code |
CI's examples lint (eslint-plugin-react-hooks 7) rejects the hook
mutating the entry object it kept in useState ("Cannot modify local
variables after render completes"). The stack now maps each order
number to its context, and callers pass the context in, so nothing
held by React is mutated.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Follow-up: CI failed on d722ee7. The examples lint (eslint-plugin-react-hooks 7) flagged the hook for mutating an object it kept in 🤖 Generated with Claude Code |
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Review of #166: one medium-severity bug. The payload crash fix, the context stack and the hook rewrite are correct. With onRender, however, an error that an ErrorBoundary catches on first render leaves an entry in the context stack that is never removed, and that entry blocks every context set before it. Tests were not run.
What checks out:
- Missing
payload(#102): rollbar.js 3.1.0merge()skipsundefinedvalues (node_modules/rollbar/src/utility.js:865). So the old restore toundefineddid leave the context in place, and restoring withstack.base ?? ''fixes it.options.payload?.contexthandles a config with nopayload. - Hook: splitting
useRollbarContextinto a set effect ([ctx]) and a remove-only cleanup ([]) is correct on update and unmount, and works with React 18 StrictMode's unmount/remount.useRollbar()andgetRollbarFromContextreturn the same instance, so the class and the hook share one stack. - Class component: keeping
order/activeon the instance avoids callingsetStateduring render. ResettingactiveincomponentWillUnmountmakes StrictMode's simulated remount apply the context again. - Ordering: order numbers are handed out during render, parents before children, so "innermost wins" holds even though children mount first.
Finding: with onRender, the stack entry is added inside render() and removed only in componentWillUnmount. When the ErrorBoundary catches an error from a child's first render, React throws the RollbarContext away without committing it. The entry is never removed and outranks every context set before it, for the life of the client (details inline).
Outside the findings: during server rendering componentWillUnmount never runs. If an app passes a shared, module-level Rollbar instance to Provider, each server render of an onRender RollbarContext would add another entry that is never removed. The examples in this repo use the per-Provider config prop, so this depends on how an app is set up.
Review feedback: with onRender, the stack entry was added in render() and only removed in componentWillUnmount. When an ErrorBoundary around the RollbarContext catches an error from a child's first render, React never mounts the RollbarContext, so the entry stayed in the stack for good and outranked every other context. The same happened on the server, where nothing mounts. onRender now sets the context directly in render(), and the component joins the stack on mount. A microtask queued from render() applies the context of whatever is mounted again. React reports the error during the commit, before the microtask runs, and rollbar.js captures its options when rollbar.error() is called, so the report keeps this context. This also fixes the leak on main when no other context is mounted. The README now recommends putting RollbarContext outside the ErrorBoundary: React removes everything inside the boundary before the error is reported, so a RollbarContext inside it can't apply to errors after the first render. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AI Agent Review (openai, openai-astra)This PR replaces the per-component save/restore of I traced the stack logic against React 17's commit order: nesting order, mount before unmount, key-change remounts, discarded renders, and One gap remains, reported inline and low severity. With Other notes, not findings:
|
AI Agent Review (openai, openai-astra)Review: RollbarContext stack, the no-payload fix (#102) and onRender (#88)No defects confirmed on the changed lines. I read What I checked and found correct
Observations, not findings (low impact, or outside the changed lines)
|
Restoring an unset context as '' is only equivalent to leaving it unset in the browser. On the server, rollbar.js derives the context from the Express route in addRequestData, and addPayloadOptions then merges payload.context, including '', over it. So after an onRender server render with a shared instance whose config has no payload.context, later request errors lose their route context. rollbar.js has no way to remove the key (configure() skips undefined and the notifier keeps its own merged copy), so correct the comment and document it in the README instead. 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)
Review of #166: RollbarContext context stack and onRender
The PR moves RollbarContext and useRollbarContext onto a per-client context stack in src/context-stack.js, keyed by render order. It fixes the #102 crash when the config has no payload, gives nesting a clear innermost-wins rule, removes the setState-during-render, and makes onRender set the context before children render (#88). I read all of this in the checkout and it holds up:
configure()ignoresundefined: rollbar 3.1.0mergeskips undefined values atnode_modules/rollbar/src/utility.js:865.- On the server,
payload.context(even'') overrides the route-derived context:addRequestDatasets it atnode_modules/rollbar/src/server/transforms.js:141, andaddPayloadOptionsruns later (node_modules/rollbar/src/server/rollbar.js:614-618) and merges the payload over it. - The README's advice to give the server
Provideraconfigdoesn't leak global wrappers per request, because server rollbar defaultsautoInstrument: false(node_modules/rollbar/src/server/rollbar.js:777).
Problems
The context that onRender sets during render is never added to the stack. It lasts until the component mounts, but only if no other stack operation runs first. In react-dom 17.0.2, the commit runs every componentWillUnmount (mutation phase, react-dom.development.js:23121) before any componentDidCatch (layout phase, :23151, :20159).
- Page changes (medium). Say the incoming page's
RollbarContext onRenderis new in this commit, and the outgoing page'sRollbarContextunmounts in the same commit. The outgoing page'sremoveContext→applyStackoverwrites the render-time context before the innerErrorBoundaryreports. A first-render error on the new page is then reported with the base (or outer) context. Only the initial page load gets it right. - Changed
contextprop (low). When React reuses anonRenderinstance with a newcontextprop,render()skips the pre-render set. The new children render, and any error they throw is reported, with the old context. The new value is only applied incomponentDidUpdate.
The new tests don't cover either case.
Notes
- Only React 17 is installed. The context-stack comment's claims about React 18/19 transitions and synchronous error retry are unverified here.
- Nothing was run. I make no claim about tests or lint passing.
The context onRender set during render wasn't recorded in the context stack, so any other stack change earlier in the same commit replaced it before an ErrorBoundary inside reported. That happens on a page change, when the old page's RollbarContext unmounts in the mutation phase, and when an earlier sibling RollbarContext re-applies in componentDidUpdate. A changed context prop on a mounted onRender instance also wasn't set until componentDidUpdate, after the boundary had reported. setRenderContext now adds a rendering entry for the component's order, which ranks with the mounted ones. The component's mount or update replaces it, and a microtask removes it if the render was thrown away. render() sets it on the first render and whenever the context prop differs from the one applied on mount or update. 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)
Review of #166: RollbarContext without a payload (#102) and onRender (#88)
This replaces the old save-and-restore approach with one shared stack of contexts per Rollbar client (src/context-stack.js). Contexts are ranked by when they first render. onRender now adds a temporary entry during render, and a microtask removes it again if React never commits that render.
What I checked against the pinned dependencies:
- Undefined is ignored: rollbar 3.1.0's
configure()merge skipsundefinedvalues (node_modules/rollbar/src/utility.js:865). So restoring an unset context needs'', and the #102 fix is sound. - No default payload: the browser defaults have no
payloadkey (node_modules/rollbar/src/browser/core.js:588-607), sooptions.payload?.contextis needed. - Server context overwrite: on the server,
addPayloadOptionsruns afteraddRequestData(node_modules/rollbar/src/server/rollbar.js:614-618). So an emptypayload.contextdoes replace the route context, as the README warns. - Build: Babel preset-env runs without
loose(babel.config.js:5), soMath.max(...map.keys())compiles to iterator-aware spread. WithpreserveModules(rollup.config.mjs:78),context-stack.jsstays a single shared module. - Tests: I traced the new tests (ErrorBoundary inside and outside, sibling updates, a page unmounting in the same commit, server render, nesting) against React 17 commit order, and they are consistent with the code. I could not run them.
One gap: by default, useRollbarContext leaves the stack in a passive-effect cleanup. React 17 runs that cleanup after the commit in which an ErrorBoundary reports. So in the README's recommended ErrorBoundary pattern, a page that used the hook can still decide the context when the next page throws. This is not a regression, but it leaves #88 partly open. Details are inline.
Not verified: anything that depends on React 18 or 19 behaviour, such as the synchronous retry after an error in a transition described in context-stack.js:84-87. Only React 17.0.2 is pinned here (package-lock.json:11842).
By default the hook removed its context in a passive effect cleanup. When React removes a component, it runs passive cleanups only after the commit, while an ErrorBoundary reports during it. So with the README's <RollbarContext onRender><ErrorBoundary> pattern, a page that used the hook still set the context of an error the next page threw on its first render, since the hook ranks inside the RollbarContext. The hook now always removes its context in a layout effect cleanup, which React runs during the commit, whatever isLayout is. React 18 and later also run layout cleanups when Suspense hides a tree that was showing, and only rerun layout effects when it shows it again, so that effect puts back the context the hook last set. On the server it uses useEffect, since useLayoutEffect warns there before React 19. 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/context-stack.js, the rewritten RollbarContext and useRollbarContext, the README changes and both new test files against the checkout. I did not run the tests (no shell), so this does not claim they pass.
What I checked
- #102 is fixed. rollbar.js 3.1.0's
mergeskipsundefinedvalues (node_modules/rollbar/src/utility.js:865), so the old restore toundefinedleft the context in place.applyStacknow restoresstack.base ?? '', and the new stack meanspayloadno longer has to exist. - Nesting works. Parents render before their children, so the order number taken on first render (
useState(nextContextOrder)/ the class field) ranks nested contexts from outermost to innermost.applyStackpicks the highest order, and a render-time entry takes precedence over a mounted one. - The
ErrorBoundarypattern works, traced through React 17 (react-dom 17.0.2 is the pinned version). When React removes a component, it runscomponentWillUnmountand layout-effect cleanups in the mutation phase.componentDidCatchruns later, in the layout phase, and child before parent. So the hook's switch to a layout cleanup, andsetRenderContextkeeping the render-time context until a microtask, give the orderings the new tests expect. I traced eachErrorBoundarytest by hand. - The microtask cleanup is safe. A stack is only deleted once both of its maps are empty, so a pending microtask can never act on a stale stack.
- The README's server warning is accurate.
addPayloadOptionsruns afteraddRequestData(node_modules/rollbar/src/server/rollbar.js:614-618), so on the server a restored''does replace the context taken from the request's route.
Worth knowing (not defects)
- With
onRender, the context is changed on the shared client during render. Anything else reported before the microtask runs gets it too. The README says so. - React 18 concurrent behaviour, like retrying synchronously after an error or delaying a commit, can't be checked from the installed React 17, so I made no claims about it.
Finding
- One low-severity inconsistency: the PR adds
useClientLayoutEffectto avoid the React 17 server warning, butuseRollbarContext(ctx, true)still callsuseLayoutEffecton the server. The existing code already did this, so it is not a regression.
…true) useEffectOfType still passed the raw useLayoutEffect when isLayout was true, so the hook logged "useLayoutEffect does nothing on the server" under react-dom 17's server renderer. Use useClientLayoutEffect there too, and run the server test with both isLayout values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AI Agent Review LGTM (openai, openai-astra)LGTM. No blocking findings were found. Review: Fix RollbarContext without a payload (#102), make onRender work (#88)No findings. I read the diff, all three changed source files and their callers and dependencies, and I think the design is sound. What changed
Claims I checked against the pinned code
TestsI traced each new test by hand through React 17.0.2 legacy-mode commit order. React Testing Library 12 uses Nits, not published as findings
Outside the diff
|
|
Finding 1 and 2 look worth addressing before merge. (2 fixes both.) 3 looks like a good follow up (React version matrix in CI.) Findings
I ran this against the PR's built lib/ on React 19.2.8 with rollbar 3.1.0. The tree was [stylesheet], going from home to about:
The author checked 18.3.1 and 19.x by hand, but nothing guards it, and the peer range is 16–19. A follow-up job that runs rollbar-context.test.tsx on React 18 and 19, with @testing-library/react 16, would cover it.
How these relate to previous reviewsThat LGTM comment also covers part of the gap I mentioned in my last answer:
The same LGTM comment has one nit I missed. The beforeEach console.error mock in rollbar-context.test.tsx replaces the prop-type failure check in src/tests/jest-setup.ts, so a prop-type failure in that describe block would pass without anyone noticing. It's still unfixed. If I post the review, I'll credit both unfixed nits to that comment rather than present them as new. |
waltjones
left a comment
There was a problem hiding this comment.
Finding 1 and 2 look worth addressing before merge. (2 fixes both.) 3 looks like a good follow up (React version matrix in CI.)
Findings
- The onRender design assumes React commits in the same task, which React 19 doesn't always do.
The comment at src/context-stack.js:79-87 says "React commits in the same task that it finishes rendering in". React 19 can hold the commit until a later task, for example while a from the same update loads. By the time it commits, the microtask at :93 has already removed the render-time entry, so componentDidCatch reports with the previous context.
I ran this against the PR's built lib/ on React 19.2.8 with rollbar 3.1.0. The tree was [stylesheet], going from home to about:
┌─────────────────────────────────────────────────────────┬──────────────────┐
│ Update │ Reported context │
├─────────────────────────────────────────────────────────┼──────────────────┤
│ sync / transition, no stylesheet │ about ✓ │
├─────────────────────────────────────────────────────────┼──────────────────┤
│ sync / transition, precedence stylesheet not yet loaded │ home ✗ │
└─────────────────────────────────────────────────────────┴──────────────────┘
- Impact: in this case onRender quietly falls back to the default behavior. Nothing crashes, nothing leaks, and the context ends up as about afterwards. It's not a regression from main, but it breaks the README's promise that "a page that React mounts inside an existing RollbarContext is covered too."
- Minimum fix: correct the comment and add this limitation to the README's onRender caveats. Finding 2 removes the problem entirely.
- Consider: apply the context when the ErrorBoundary reports, instead of setting it during render.
The rendering map, the microtask, and most of the comment block in context-stack.js exist only because onRender changes a global during render and then has to guess when React is done.
- Alternative: RollbarContext also passes its context down through a React context. ErrorBoundary reads the nearest value when it renders and applies it around the rollbarlevel call in componentDidCatch.
- How it would work: rollbar.js has no per-item context (addPayloadOptions merges options.payload over the item), so this would be configure, report, then re-apply the stack. That relies on rollbar.js taking a synchronous snapshot of options, which the PR thread already relies on.
- Why it's better: React resolves context during render, so when the commit happens stops mattering. It fixes finding 1, works without onRender, and makes the README's original claim true by default.
- Trade-offs:
- ErrorBoundary already uses contextType for the client, so it needs a Consumer or another way to read the value.
- useRollbarContext can't provide a React context, so hooks wouldn't feed this. That matches the README's current guidance.
- Why decide now: this PR moves onRender from undocumented to documented, recommended API, and that's hard to take back. Decide before merge, or merge and track this as a follow-up. It isn't a blocker.
- Consider: CI only tests React 17, but this code depends on how each React version orders a commit.
It depends on:
- layout cleanups running before componentDidCatch
- passive cleanups being deferred
- Suspense hide/show behavior in 18+
- the same-task commit from finding 1
The author checked 18.3.1 and 19.x by hand, but nothing guards it, and the peer range is 16–19. A follow-up job that runs rollbar-context.test.tsx on React 18 and 19, with @testing-library/react 16, would cover it.
- Optional: stop calling configure() on every update in the default mode.
At src/rollbar-context.js:46, !onRender || context !== prevProps.context re-applies the context on every re-render of every RollbarContext. Each re-apply merges the full options object and reconfigures the client. main already did this.
- Now that the stack is the source of truth, re-applying an unchanged value only overwrites outside configure calls, such as historyContext.
- if (context !== prevProps.context) for both modes is simpler and removes an asymmetry readers will wonder about.
- Behavior change: an outside configure() would then last until the next stack chae-render.
- Nit: at rollbar-context.server.test.tsx:31, mockRestore() comes after the assertion. If the test fails, the console.error spy stays in place for the next case. Move it to afterEach, as the main test file does.
How these relate to previous reviews
Two of my six findings were raised before, three were mentioned in passing without being raised as findings, and one is entirely new. brianr's two reviews, including the approval, have empty bodies, so all the earlier discussion comes from the AI reviewers and the replies to them.
┌───────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ My finding │ Covered before? │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ │ Not as a finding. Three of the rollbar-circleci-machine reviews said the comment's React 18/19 claims couldn't be checked with React 17 installed. The 05:55 │
│ 1. React 19 delayed │ review on 9/27 even named "delaying a commit" as something it couldn't verify. The reply to the first inline finding said componentDidCatch runs "in the same │
│ commit │ task as the render". That reply tested a React 19 transition that yields partway through and the synchronous retry after an error, but not a commit that React │
│ │ holds back. My repro is the first to show the claim failing. │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 2. Report with the │ │
│ context at │ No. Every review accepted the render-time approach and only reviewed how it was implemented. │
│ ErrorBoundary │ │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 3. CI runs only React │ Yes, as notes. The 9/27 LGTM comment listed it as a nit: the suite only runs React 17, so none of the 18+ code paths are tested. Two other reviews said the same │
│ 17 │ in their notes. Nobody proposed adding a CI job, and the author relied on checking 18 and 19 by hand. │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 4. configure() on │ Mentioned, not raised. The 9/25 22:44 review said: "Without onRender, componentDidUpdate still calls configure() on every re-render. That matches the old │
│ every update │ behaviour." Nobody pointed out that the stack makes it easy to drop. Two reviews also noted that historyContext bypasses the stack, which is the behavior change │
│ │ my finding mentions. │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 5. mockRestore after │ Yes, the same nit. It's in the 9/27 LGTM comment, under "Nits, not published as findings". It's still unfixed. │
│ the assertion │ │
├───────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 6. rollbar 4.x │ No. The reviews finished on 9/28, before main allowed rollbar 4. │
└───────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
That LGTM comment also covers part of the gap I mentioned in my last answer:
- It traced the code and found that _applyTransforms snapshots this.options when error() is called, and that configure() replaces the options object rather than changing it. So a report already in flight keeps the context it was logged with.
- The author also checked the context on real sent items, using rollbar.js's transform option, on React 17 with rollbar 3.1.0 and on React 19 with 2.26.4.
- That's verified for 2.26.4 and 3.1.0. It's still unchecked for 4.0.0.
The same LGTM comment has one nit I missed. The beforeEach console.error mock in rollbar-context.test.tsx replaces the prop-type failure check in src/tests/jest-setup.ts, so a prop-type failure in that describe block would pass without anyone noticing. It's still unfixed. If I post the review, I'll credit both unfixed nits to that comment rather than present them as new.
Addresses waltjones's review on #166. RollbarContext now also passes its context down through a React context, and ErrorBoundary applies it around the report in componentDidCatch, then puts the previous context back. React resolves the context during render, so this no longer depends on when React commits. It works without onRender, on the first render and after a change to the `context` prop. It also fixes React 19 holding a commit back for a precedence stylesheet, where the onRender microtask removed the context before the report. A useRollbarContext between the RollbarContext and the ErrorBoundary still wins, as the innermost context. Without a RollbarContext, ErrorBoundary doesn't touch the client, as before. - onRender stays, for anything else the children send while mounting. The README documents it with that use, and its caveats now include the React 19 delayed commit. The context-stack.js comment no longer claims React always commits in the same task. - componentDidUpdate only re-applies when the `context` prop changes, with or without onRender. - RollbarContext without children no longer throws on React 17. - jest-setup.ts formats console.error's arguments, so a prop-type failure fails the test again. React passes the message as a format string. The RollbarContext tests' console.error mock passes those failures on, and the server test restores its spy in afterEach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@waltjones thanks, addressed in dcac601, after merging 1. React 19 delayed commit: fixed (by 2). I reproduced it first. Your tree, going from home to about on React 19.3.0, with the stylesheet's 2. Apply the context when the ErrorBoundary reports: done.
3. React version matrix in CI: left as the follow-up you suggested. I ran the 4. 5. The other carried-over nit (the Also: a
🤖 Generated with Claude Code |
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Fixes #102: reads of payload.context now use ?., and the context is restored to '' rather than undefined. It also fixes RollbarContext/ErrorBoundary timing (#88) by adding a per-client stack of contexts (src/context-stack.js) and having ErrorBoundary read the nearest RollbarContext through an internal React context.
What I checked in the checkout:
- rollbar 3.1.0, the repo's dev dependency (
package.json:97): the PR's key assumption holds.Notifier._applyTransformscopiesthis.optionswhen an item is logged (node_modules/rollbar/src/notifier.js:104), andaddPayloadOptionsreadspayloadfrom that copy (node_modules/rollbar/src/transforms.js:14).configure()replaces the options object rather than changing it (node_modules/rollbar/src/rollbar.js:67,node_modules/rollbar/src/notifier.js:33,node_modules/rollbar/src/browser/core.js:122). So setting the context around the report inreportWithContextworks. I could not check the same for rollbar 2.26.4 or 4.0.0, because only 3.1.0 is installed. - Ordering: a parent always gets a lower order than its children.
RollbarContexttakes it in a class field (src/rollbar-context.js:32) and the hook in auseStateinitializer (src/hooks/use-rollbar-context.js:27). Removal happens incomponentWillUnmountor a layout cleanup (src/hooks/use-rollbar-context.js:34-41), so the old page's entries are gone before the new boundary'scomponentDidCatchruns. ErrorBoundary: it re-renders itsConsumeron the render that catches the error, sothis.reportContextcomes from the tree being committed. Without aRollbarContext, the client isn't touched (src/context-stack.js:122-125), which keeps working with mocked rollbar clients like the one inexamples/nextjs.- Tests: I traced the new tests against this behaviour and they are consistent. I did not run them, so I can't confirm they pass.
Declared dependencies:
- The typings half of #88 (#162) is delivered in this checkout:
index.d.ts:43-47hascontext: stringandonRender?: boolean. - The React 18/19 and rollbar 2.x/4.x results in the description come from the author's local runs. CI pins React 17 (
package.json:93-94) while the peer range is 16.x–19.x (package.json:109), and the description proposes a version matrix as a follow-up.
One low-severity gap: reportWithContext defers to any stack entry created later, not only one nested between the RollbarContext and the boundary. So a later-created sibling RollbarContext or hook overrides the boundary's own context, which contradicts the new README text. This is the same as main's last-mounted-wins behaviour, so it is not a regression.
reportWithContext kept the client's context whenever any stack entry ranked after the boundary's RollbarContext. Order is only creation order, so an unrelated sibling RollbarContext or hook created later (a footer, a sidebar) also won, and the result depended on mount history. RollbarContext and ErrorBoundary now provide a scope path, the orders of the RollbarContexts and ErrorBoundaries around a component, through an internal ScopeContext, and every stack entry records its path. The boundary only gives way to an entry inside its RollbarContext that is either inside the boundary (an onRender render React threw away) or a useRollbarContext that rendered before the boundary. A hook doesn't provide a scope, so one in an earlier sibling of the boundary inside the same RollbarContext still counts; the README says so. 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 full diff against the checkout: src/context-stack.js, src/rollbar-context.js, src/error-boundary.js, src/hooks/use-rollbar-context.js, the new tests, jest-setup.ts and the README sections. I also read src/provider.js, index.d.ts, babel.config.js, rollup.config.mjs and the existing ErrorBoundary/Provider tests.
Verified
-
The #102 fix holds. Every read now goes through
rollbar.options.payload?.context(src/context-stack.js:24,:174). Restores use?? '', so an unset context isn't silently skipped byconfigure(src/context-stack.js:69,:184). -
The approach
reportWithContextrelies on holds in rollbar 3.1.0, the pinned version (package-lock.json):Notifier._applyTransformstakesthis.optionsonce, before running the transforms (node_modules/rollbar/src/notifier.js:100-123).configurebuilds a new options object instead of changing the old one (node_modules/rollbar/src/browser/core.js:122,node_modules/rollbar/src/notifier.js:33)._logreachesnotifier.logsynchronously (node_modules/rollbar/src/rollbar.js:162-176).
So setting the context, reporting, then restoring it does affect the item that gets sent.
-
Hand traces of these tests reach the asserted values: the hook-between, sibling, footer and innermost-hook tests; the page change with the hook; and the nested-contexts cases.
-
Unmount timing checks out. The hook removes its entry in a layout cleanup (
src/hooks/use-rollbar-context.js:44-51) andRollbarContextincomponentWillUnmount(src/rollbar-context.js:67-70). Both run beforecomponentDidCatch, matching the comments. -
?.,??and??=go through@babel/preset-envwith no targets set (babel.config.js:5), so the build transpiles them. -
The declared dependency on #162 is confirmed in the checkout:
index.d.ts:43-46declaresRollbarContextwithonRender?: boolean.
Not a finding, worth a thought
ReportContextcarries only{ order, context }(src/rollbar-context.js:96). With nestedProviders (<Provider instance={a}><RollbarContext>…<Provider instance={b}><ErrorBoundary>), the boundary would applya'sRollbarContextcontext to clientbwhile reporting (src/error-boundary.js:53-54). Nested Providers aren't documented, so this is noted rather than filed.- Order numbers are taken at first render. If an
ErrorBoundaryis remounted (akeychange, or mounted conditionally later), a plain-component hook rendered after it in the tree but created earlier counts as "between". The README's "renders before" wording roughly covers this.
No tests were run. Test results are only the author's claims in the PR description.
…ntext reportWithContext treated any useRollbarContext inside the boundary's RollbarContext that took an earlier order number as between them. A hook records its scope path, so one inside a sibling ErrorBoundary or RollbarContext can be told apart: only count a hook whose scopes are all around the ErrorBoundary. 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)
Review: RollbarContext without a payload (#102), plus onRender and nested contexts (#88)
Overall this is a careful fix, and its core claims check out against the pinned dependencies:
- #102: rollbar's
mergeskipsundefined(node_modules/rollbar/src/utility.js:865), so restoring''is needed. Readingoptions.payload?.context(src/context-stack.js:24,:181) removes the crash. - Report-time context: applying the context around the report works because rollbar snapshots its options when an item is logged (
node_modules/rollbar/src/notifier.js:104), andconfigure()replaces the options object rather than changing it (node_modules/rollbar/src/rollbar.js:67,node_modules/rollbar/src/browser/core.js:122). - #162 dependency: the stated dependency on #162 is delivered in this checkout.
index.d.ts:45-46hascontext: string(required) andonRender?: boolean.
Findings
- Medium: removing a hook's entry in a layout cleanup loses its context for errors the hook's own component throws after it has mounted. The repo's TypeScript example reports with
''instead of/Home. - Low:
reportWithContextdecides whether a sibling hook sits "before" the boundary by when it was created, not where it renders. After a keyedErrorBoundaryremounts, the result no longer matches the README.
Behaviour change to be aware of (documented in the README, not a finding): an ErrorBoundary inside a RollbarContext now reports with that component's context. It overrides any context set directly through historyContext or useRollbarConfiguration (src/context-stack.js:162, :186; README.md:398).
Not verified: the React 18 and 19 claims. Only React 17.0.2 is installed (package-lock.json). I could not run the tests, so nothing here says they pass.
Removing a useRollbarContext entry in a layout cleanup keeps the old page's hook from tagging the next page's error, but React also runs that cleanup before componentDidCatch when the hook's own component throws after mounting, so the error lost the hook's context. The TypeScript example (ErrorBoundary around a page that calls the hook and throws on a state update) reported '' instead of '/Home'. The hook now records the context and scope path it renders with in a registry that only reportWithContext reads; it never changes the client's context. A layout effect clears it on commit, and a microtask clears renders React throws away. The ErrorBoundary copies it while it renders after catching, so a commit React 19 holds back (a precedence stylesheet) still has it. A hook in a component that rendered with the error (it threw, or a child rendering with it did) takes precedence like any other entry inside the boundary, also without a RollbarContext. One that didn't render with it, like the previous page or a child throwing on its own update, still doesn't. A sibling hook no longer has to have an earlier order than the ErrorBoundary to count as between it and its RollbarContext. Order is creation order, so a keyed ErrorBoundary that remounts ranked after a footer hook it used to rank before. A hook doesn't provide a scope, so any hook whose scopes are all around the boundary counts, before or after it, as it does for the client's context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #102. Covers the behaviour and docs half of #88; the typings half (
contextrequired,onRenderadded toindex.d.ts) is in #162, so #88 can be closed once both have merged.#162 has merged, and this PR now targets
main.Why
#102:
Cannot read properties of undefined (reading 'context'). Neither rollbar.js 2.26.4 nor 3.1.0 sets a default for thepayloadoption.RollbarContextanduseRollbarContextboth readrollbar.options.payload.context, so either one throws on mount unless the config happens to setpayload. While testing the fix I found a second problem with the same cause: on unmount they restore the previous context withconfigure({ payload: { context: undefined } }). rollbar.js'smergeskipsundefinedvalues, so the context stayed set after unmount.#88:
RollbarContextdoesn't match the docs. By default the context is set incomponentDidMount, and React only runs that after the children have rendered and mounted. So an error thrown while the children first render, which is exactly what the README'sErrorBoundarypairing is for, is reported with the previous context. The fix:ErrorBoundarynow reports with the context of the nearestRollbarContextaround it, taken from a React context (from waltjones's review; see below). The undocumentedonRenderprop sets the context during the first render instead. It also had two bugs:setStatefromrender, which triggers React's "Cannot update during an existing state transition" warningcontextpropNested contexts (from review). Making
onRenderfollow prop changes meant that changing an outer context overwrote an inner one that was still mounted, because React runs update lifecycles child-first. Nesting was already broken onmainwithoutonRenderand in the hook: children mount before parents, so the outer context won at mount, and unmounting restored in the wrong order, leaving the inner context set after everything unmounted.ErrorBoundaryreports with the nearestRollbarContext's context (waltjones's review). The first version relied ononRendersetting the context during render, and on a microtask putting it back after React committed. React 19 doesn't always commit in the same task: it can hold a commit back until a<link rel="stylesheet" precedence>has loaded, and by then the microtask had removed the context. NowRollbarContextpasses{ order, context }down through a React context.ErrorBoundaryreads it when it renders and applies it around the report incomponentDidCatch. React resolves the value during render, so when React commits no longer matters, andonRenderisn't needed for this.What changed
src/context-stack.js(new): the active contexts for each Rollbar client, shared by the component and the hook.previous ?? ''.onRender's render-time context is kept as a rendering entry with the same ranking. Another context unmounting or re-applying earlier in the same commit (a page change, a siblingRollbarContext) therefore can't replace it before anErrorBoundaryinside reports. The component's mount or update replaces the entry. If React throws the render away, a microtask removes it.rollbar.options.payload?.context, so a config withoutpayloadworks.reportWithContext(forErrorBoundary): with aRollbarContextaround the boundary, it configures that context, reports, and puts the previous context back (previous ?? ''). Without aRollbarContext, the same entries as below take precedence over the client's context, and if there aren't any it doesn't touch the client, as before.RollbarContext, the innermost one if there are several: auseRollbarContextbetween it and the boundary, a hook inside the boundary in a component that rendered with the error, or anonRendercontext inside the boundary whose render React threw away. It decides from each entry's scope path (below), not from order alone, so a siblingRollbarContextor a hook outside it doesn't override it. A hook counts as between them if every scope around it is also around the boundary, before or after it among the boundary's siblings, so one inside a siblingErrorBoundaryorRollbarContextdoesn't.setHookRender: the context and scope path eachuseRollbarContextrenders with. OnlyreportWithContextreads it; it never changes the client's context. A layout effect clears it on commit, and a microtask clears renders React throws away. This relies on rollbar.js reading the options an item is sent with when it's logged (_applyTransformssnapshotsthis.options), and onconfigure()replacing the options object rather than changing it. Both checked in rollbar 2.26.4, 3.1.0 and 4.0.0.src/error-boundary.js: reads the nearestRollbarContextthrough aConsumer, sincecontextTypeis taken by theProvider's context, and reports throughreportWithContext. It takes an order number and provides a scope path, likeRollbarContext. In the render after catching, it copiessetHookRender's entries, so a commit React holds back doesn't lose them.src/rollbar-context.js{ order, context }to theErrorBoundarythrough an internal React context (not exported from the package). As a side effect, aRollbarContextwith no children no longer throws "Nothing was returned from render" on React 17.ScopeContext: the orders of theRollbarContexts andErrorBoundarys around a component. It's stable per instance, so acontextprop change doesn't re-render components that use the hook. Stack entries record it.onRenderno longer callssetStateduring render.onRender, a rendering entry is set before the children render: on the first render, and whenever thecontextprop differs from the one last applied. The mount or update then applies it for good.onRender: added on mount and removed on unmount, as before.contextprop changed, with or withoutonRender. Before, it re-applied on every re-render withoutonRender. Behaviour change: a directconfigure()(for examplehistoryContext) now lasts until the next stack change rather than the next re-render of anyRollbarContext.src/hooks/use-rollbar-context.js: uses the same stack: adds its entry in an effect, updates it whenctxchanges, and removes it on unmount. The entry records the scope path around the hook.isLayoutis. React runs passive cleanups of a removed component only after the commit in which anErrorBoundaryreports, so otherwise the old page's hook would set the context of the next page's first-render error.setHookRenderwhile it renders. React runs that layout cleanup beforecomponentDidCatchtoo, so anErrorBoundaryaround a component that throws after mounting gets the hook's context from the copy it made.useEffect, sinceuseLayoutEffectwarns there before React 19. The same applies touseRollbarContext(ctx, true).README.mdErrorBoundary": put theRollbarContextoutside theErrorBoundary. The boundary reports with its context, including on the first render and after acontextprop change. AuseRollbarContextbetween them takes precedence, and aRollbarContextor hook elsewhere in the tree doesn't. A hook doesn't provide a scope, so one in another component inside the sameRollbarContext(a sidebar or a footer) counts as between them, as it does for the client's context, unless it's inside anotherErrorBoundaryorRollbarContextthere.onRenderfor anything else the children send while mounting, for example from their own effects. It explains whyonRenderisn't the default: it's a side effect inrender. Nothing restores the context if React throws that render away, except the microtask. React may also commit after that microtask (a transition yielding, or React 19 holding a commit for a stylesheet), and then the children mount with the previous context.useRollbarContexthas the same timing as the default, and is removed as soon as its component is. AnErrorBoundaryaround or inside the component reports with it if the component rendered with the error. An error it didn't render with, like a child's own state update or an effect, gets the context around it.src/tests/components/rollbar-context.test.tsxandrollbar-context.server.test.tsx(node environment,renderToStringwith the hook): new tests. They're TSX and use Fix historyContext and RollbarContext typings, type-check index.d.ts in CI (#69) #162'sRollbarContexttypings (onRender), so ts-jest andnpm run typechecktype-check them. TheErrorBoundarytests with theRollbarContextoutside the boundary run with and withoutonRender.src/tests/jest-setup.ts: formatsconsole.error's arguments before checking for a prop-type failure. React passes'Failed %s type: %s%s', 'prop', …, so the check never matched, and prop-type failures passed silently everywhere. TheRollbarContexttests'console.errormock passes prop-type failures on to it.Why
''rather thanundefinedornullon restoreconfigure()ignoresundefined.nullwould be sent as the string"null", becausebuildPayloadinapiUtility.jsstringifies any non-string context. For an unset context,buildPayloadalready sends''(contextResult.value || ''), so restoring''produces the same item as never having set one. I checked themergeandbuildPayloadbehaviour in both rollbar 2.26.4 and 3.1.0.Not changed: making
onRenderthe defaultErrorBoundaryno longer needs it.onRenderwould still move a global side effect intorenderfor every user, with the leak and timing caveats above.Follow-up: a React version matrix in CI
CI only runs React 17, while this code depends on how each React version orders a commit. A follow-up job could run the
RollbarContextandErrorBoundarytests on React 18 and 19 with@testing-library/react16. Locally that also neededMessageChannelandTextEncoderin the jsdom environment, for React 19'sreact-dom/server.Validation
Node 20.19, React 17, rollbar 3.1.0 (the repo's dev dependency):
npm run typecheck:12 files, no errorsnpx jest: 35/35 passing (5 suites, 22 new tests)Run against
main'ssrc/rollbar-context.jsandsrc/hooks/use-rollbar-context.js, 8 of the new tests fail:onRender, the hook, and the hook inside the componentThe ErrorBoundary tests pin down <RollbarContext /> implementation doesn't match docs #88's behaviour: a render error is reported with
rootby default and withhomeunderonRender.npm run lint -- --max-warnings 0: passnpm run lint:examplesandnpm run test:examples: pass, with the examples using this branch's buildPrettier: all changed files pass
npm run build: pass.?.and??are transpiled indist/andlib/.Review round 3 (page change, sibling update,
contextprop change underonRender): 4 new tests fail on abca1d9 and pass on ec425b0. I also ran the scenarios against the builtlib/on React 18.3.1 and 19.3.0 with rollbar 3.1.0, with and withoutStrictModeand including astartTransitionpage change. All report with the new context, and the pre-fix build reports the old one.Review round 4 (a page that used
useRollbarContextis replaced by one that throws on its first render): the new test fails on ec425b0 (reported withhome#index) and passes on 8bfdfc2. The server test fails if the hook usesuseLayoutEffectdirectly. On the builtlib/, on React 18.3.1 and 19.3.0 with rollbar 3.1.0, with and withoutStrictMode, the page change reports with the new page's context. A Suspense re-suspend drops to the outer context while hidden and comes back when shown.lint:examplesandtest:examplespass with npm 10 afterinstall:all -- ci.Review round 5 (
useRollbarContext(ctx, true)on the server): the server test now runs withisLayoutbothfalseandtrue. Thetruecase fails on 8bfdfc2 with theuseLayoutEffectwarning and passes on 9677a34.npm run typecheck, lint, Prettier andnpm run buildpass. With npm 10, afterinstall:all -- ci,lint:examplesandtest:examplespass.Review round 6 (waltjones). Merged
main, so rollbar 4.x is in the peer range.npx jest: 47/47 (12 new tests). 8 of them fail against the previous source: the six default-modeErrorBoundarytests (first render, page change, old page with the hook, sibling update,contextprop change, putting the previous context back), plus "does not re-apply an unchanged context" and "renders without children".examples/nextjs's test (a mocked rollbar with nooptions) caught in my first version.<RollbarContext context={page}>with aprecedencestylesheet, then<ErrorBoundary><Throw key={page}/>, going from home to about) on React 19.3.0 in jsdom, with the stylesheet'sloaddelayed. I checked the context on the item rollbar.js builds, using itstransformoption. On the previous build the item is sent withhome, in every case. On this build it's sent withabout: default andonRender, sync andstartTransition, on rollbar 2.26.4, 3.1.0 and 4.0.0.RollbarContext, server andErrorBoundarytest files: 36/36 on React 18.3.1 and on 19.3.0, with@testing-library/react16.3.3.npm run typecheck,npm run lint -- --max-warnings 0, Prettier on the changed files, andnpm run build:allpass. With npm 10, afterinstall:all -- ci,lint:examplesandtest:examplespass.Review round 7 (AI review: a later sibling overrode the boundary's context). Scope paths replace the order-only check.
npx jest: 55/55, with 8 new tests: 4 cases, each with and withoutonRender. The cases are a later siblingRollbarContext, hooks after the boundary or outside theRollbarContext, a lateronRendersibling still rendering in the same commit, and the innermost of two hooks between. All 8 fail on dcac601 and pass on bc8f28d.RollbarContext, server andErrorBoundarytest files: 44/44 on React 18.3.1 and on 19.3.0, with@testing-library/react16 and jsdom polyfills forTextEncoderandMessageChannel.npm run typecheck,npm run lint -- --max-warnings 0, Prettier on the changed files andnpm run buildpass. With npm 10, afterinstall:all -- ci,lint:examplesandtest:examplespass.Review round 8 (AI review: a hook in a sibling
ErrorBoundarycounted as between the boundary and itsRollbarContext).reportWithContextnow also requires every scope around the hook to be around theErrorBoundary.npx jest: 57/57, with 1 new test run with and withoutonRender: hooks inside a siblingErrorBoundaryand inside a siblingRollbarContext, before the boundary that reports. Both runs fail on bc8f28d (reported withnav#menu) and pass on e9cae5e (reported withdashboard).npm run typecheck,npm run lint -- --max-warnings 0, Prettier on the changed files andnpm run buildpass.Review round 9 (AI review: the hook's layout cleanup dropped its context when its own component threw after mounting;
order < boundarychanged meaning when a keyedErrorBoundaryremounted).npx jest: 67/67, with 10 new tests. 9 of them fail on e9cae5e and pass on 7d23894:RollbarContext(the TypeScript example's shape)ErrorBoundaryinside the hook's component, on first renderonRender: a child rendering with the hook's component throws; the new page calls the hook and throws on its first render; a footer hook after a keyedErrorBoundarythat remountsRollbarContext's context, not the hook's.RollbarContext, server andErrorBoundarytest files: 56/56 on React 18.3.1 and 19.3.0, with@testing-library/react16.3.3.precedencestylesheet: the report hashome#index. Reading the registry atcomponentDidCatchinstead of the render-time copy givesroot.npm run typecheck,npm run lint -- --max-warnings 0, Prettier andnpm run buildpass. With npm 10, afterinstall:all -- ci,lint:examplesandtest:examplespass.🤖 Generated with Claude Code