feat(core): Capture Expo Router ErrorBoundary errors#6318
Open
alwx wants to merge 5 commits into
Open
Conversation
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
alwx
force-pushed
the
feat/expo-router-error-boundary
branch
from
e252cc5 to
f313648
Compare
Contributor
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sentry capture fires during render phase, risking duplicate events or missed deduplication on remount
Calling reportRouterBoundaryError in the render body rather than in a useEffect means the capture can fire during an abandoned Concurrent Mode render (error reported but boundary never committed) and will re-fire after any genuine unmount+remount because useRef resets on remount — the ref-based dedup only prevents duplicates within the same component lifetime. Consider moving the logic to useEffect(() => { if (props.error && reportedErrorRef.current !== props.error) { reportedErrorRef.current = props.error; reportRouterBoundaryError(props.error); } }, [props.error]).
Evidence
expoRouterErrorBoundary.tsxlines 56-58: the guard andcaptureExceptioncall are in the function body, not inside anyuseEffect.useRefis initialized tonullon every fresh mount; when Expo Router or a parent unmounts and remounts the boundary (e.g., navigation stack pop+push with the same error prop), a new ref starts atnull, bypassing the dedup check and emitting a second event for the same error.- In React 18 Concurrent Mode, renders that are interrupted and abandoned will still have executed lines 57-58, reporting to Sentry before the fallback UI ever commits.
- The test suite (
expoRouterErrorBoundary.test.tsx) only covers same-instance rerenders and does not exercise the unmount→remount path, so this scenario is untested.
Identified by Warden code-review
Contributor
Author
|
it's still a draft, don't review yet |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Really needed this, looking forward for it been merged |
Add `Sentry.wrapRouterErrorBoundary` to wrap Expo Router's per-route `ErrorBoundary` so render-phase errors that hit the fallback are captured with route context, the active navigation transaction is marked errored, and a breadcrumb is emitted. Closes #6160
Move the Sentry reporting side-effects out of the wrapped boundary's render body and into a `useEffect` keyed on the error instance. Running `captureException`, `addBreadcrumb`, and the navigation span status mutation during render violated React's purity rule and could fire from abandoned Concurrent Mode renders before the fallback ever committed, while also leaving a window where a discarded render would set the dedup ref and suppress the real report. Replace the per-instance `useRef` dedup with a module-scoped `WeakSet` of reported errors so an unmount → remount cycle with the same error instance (e.g. parent caches the error and re-renders the boundary) no longer produces duplicate events. `WeakSet` keeps the dedup table from growing unbounded. Wrap the reporting call in try/catch and log via `logger.error` so a failure inside Sentry instrumentation can never abort the wrapped boundary's render and break Expo Router's fallback UI. Move the changelog entry from the already-released 8.15.0 section to Unreleased and point the link at PR #6318 instead of the issue, per DangerJS guidance. Adds tests covering the unmount/remount dedup path and the Sentry-throws-still-renders-fallback path.
alwx
force-pushed
the
feat/expo-router-error-boundary
branch
from
f313648 to
a3d2be3
Compare
`logger.error` is typed to accept a single string message; passing the caught `unknown` value as a second argument failed `tsc` with TS2345. Stringify the caught value into the message instead — matches the existing `logger.error` usage convention elsewhere in the SDK.
Adding the error to the `reportedErrors` WeakSet before `reportRouterBoundaryError` ran meant a transient failure inside Sentry instrumentation permanently suppressed capture for that error instance: the try/catch swallowed the failure, the error stayed in the set, and later renders/remounts of the boundary with the same error short-circuit on the `has(error)` guard and never retry. Move the `.add(error)` call inside the try block, after the capture succeeds. A transient failure no longer poisons the dedup table — the next render of the boundary gets another shot at reporting the error. Adds a test `retries capture on the next render after a transient reporting failure` to cover the regression.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e3fccdd. Configure here.
`markActiveNavigationSpanErrored` was reading the span origin from a
non-standard `root.attributes['sentry.origin']` shape, which is not the
public API exposed by Sentry spans in this SDK. The convention used
elsewhere (see `tracing/span.ts`) is `spanToJSON(span).origin`. In
production the cast-based lookup almost always returned `undefined`, so
the `startsWith('auto.navigation.')` guard short-circuited and open
navigation transactions were never tagged with `SPAN_STATUS_ERROR` —
defeating one of the three contracts the ErrorBoundary wrapper promises.
Switch to `spanToJSON(root).origin` and update the test mocks to expose
the origin through a mocked `spanToJSON` instead of a fabricated
`attributes` map, so the tests now exercise the real read path.
Contributor
Author
|
@antonis can be reviewed again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Adds
Sentry.wrapRouterErrorBoundary— wraps Expo Router's per-routeErrorBoundaryso render-phase errors that hit the fallback are captured with route context (route.name,route.path,route.params), the active navigation transaction is marked errored, and a breadcrumb is emitted. Concrete path/params are gated behindsendDefaultPii.💡 Motivation and Context
Closes #6160.
💚 How did you test it?
New unit tests in
expoRouterErrorBoundary.test.tsx. Full suite green locally.📝 Checklist
sendDefaultPIIis enabled🔮 Next steps
Investigate Metro/Babel auto-instrumentation of
export { ErrorBoundary } from 'expo-router're-exports as a follow-up.