Repository navigation
fix(tracing): Support Expo Router 58 in expoRouterIntegration - #6923
Conversation
Replace the placeholder in the CHANGELOG entry with the PR number, #6923.
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. |
Replace the placeholder in the CHANGELOG entry with the PR number, #6923.
87f19fa to
c899efa
Compare
87f19fa to
c899efa
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 fc6a0b5. Configure here.
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Thanks for tackling this! The Expo Router 58 path looks right: actionDispatched fires before the container's state event, and v57 has no internalNavigationEvents, so no navigation gets two spans.
Two blockers for older versions, details inline:
- Expo Router ≤55 silently loses the integration (
getRouteInfoFromStateis missing). - v56/57 regress #6431 (
navigationRefis read only once).
I verified both against the published expo-router packages (4.x through 58.x).
Replace the placeholder in the CHANGELOG entry with the PR number, #6923.
0e409bf to
1246c96
Compare
Review of #6923 found two regressions for Expo Router versions before 58. Expo Router 55 and older do not ship getRouteInfoFromState, so the require threw and the integration stopped as if expo-router were not installed. The route info module is now optional. Without it, the integration still registers the navigation container and skips the route override, so spans use the React Navigation route names. In Expo Router 56 and 57, useNavigationContainerRef reads a store that stays empty until the Root Layout renders. The ref was read once at Sentry.init, so polling timed out and brought back #6431. The store now reads the ref on every access, and a throwing call returns undefined so it cannot escape the poll timer. The #6431 regression test is restored with the new mocks, and new tests cover a missing getRouteInfoFromState and a throwing useNavigationContainerRef. The CHANGELOG entry moves back to the Unreleased section after the 8.32.0 release.
|
We backported this PR (at 1246c96) onto 8.28.0 to run expo-router 58.0.25, and real route changes now get navigation spans with the templated route name. Thanks for this. One thing we hit on 58:
We worked around it by forwarding only navigation actions from the listener: const NAVIGATION_ACTION_TYPES = new Set([
'NAVIGATE',
'NAVIGATE_TO_HREF',
'PUSH',
'REPLACE',
'GO_BACK',
'POP',
'POP_TO',
'POP_TO_TOP',
'JUMP_TO',
'RESET',
]);
// in the actionDispatched listener, after the `!event.actionType` check
if (!NAVIGATION_ACTION_TYPES.has(event.actionType)) {
return;
}With that, a cold launch ships one A denylist ( |
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Thank you for the changes Alex 🙇 Overall LGTM 👍
Let's also handle the report from @invivek26 #6923 (comment) 🙏
Replace the placeholder in the CHANGELOG entry with the PR number, #6923.
Review of #6923 found two regressions for Expo Router versions before 58. Expo Router 55 and older do not ship getRouteInfoFromState, so the require threw and the integration stopped as if expo-router were not installed. The route info module is now optional. Without it, the integration still registers the navigation container and skips the route override, so spans use the React Navigation route names. In Expo Router 56 and 57, useNavigationContainerRef reads a store that stays empty until the Root Layout renders. The ref was read once at Sentry.init, so polling timed out and brought back #6431. The store now reads the ref on every access, and a throwing call returns undefined so it cannot escape the poll timer. The #6431 regression test is restored with the new mocks, and new tests cover a missing getRouteInfoFromState and a throwing useNavigationContainerRef. The CHANGELOG entry moves back to the Unreleased section after the 8.32.0 release.
1246c96 to
8f147a5
Compare
📲 Install BuildsAndroid
|
| ### Fixes | ||
|
|
||
| - Apply `screenshot` masking options to Android error and feedback screenshots ([#6930](https://github.com/getsentry/sentry-react-native/pull/6930)) | ||
| - Support Expo Router 58 in `expoRouterIntegration`. Metro no longer warns about the removed `expo-router/build/global-state/router-store` module, and the integration creates navigation spans again ([#6923](https://github.com/getsentry/sentry-react-native/pull/6923)) |
There was a problem hiding this comment.
On Expo Router ≤55 there's no getRouteInfoFromState, so the route override is skipped and transaction names change from the templated path to the React Navigation route name. Users with dashboards or alerts on those names will notice, so it's worth calling out:
| - Support Expo Router 58 in `expoRouterIntegration`. Metro no longer warns about the removed `expo-router/build/global-state/router-store` module, and the integration creates navigation spans again ([#6923](https://github.com/getsentry/sentry-react-native/pull/6923)) | |
| - Support Expo Router 58 in `expoRouterIntegration`. Metro no longer warns about the removed `expo-router/build/global-state/router-store` module, and the integration creates navigation spans again ([#6923](https://github.com/getsentry/sentry-react-native/pull/6923)) | |
| - On Expo Router 55 and older (Expo SDK 52–55), navigation transactions now use React Navigation route names (for example `[id]`) instead of templated paths (`/profile/[id]`). To keep full paths, set `useFullPathsForNavigationRoutes: true`. |
Could you check in the ≤55 test that useFullPathsForNavigationRoutes actually produces the old templated path? If it doesn't, drop that sentence. And if you'd rather restore templated names on ≤55 here (e.g. build segments from the focused route names and reuse buildExpoRouterTemplatedPath), the sub-bullet isn't needed.
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Added a comment on the changelog. Other than that LGTM 🎉
Expo Router 58 removed the internal router-store module and no longer emits React Navigation's __unsafe_action__ event. Because 58 declares package exports, Metro printed an invalid package.json warning for the router-store require on every bundle, and the integration could not start navigation spans. The navigation container ref now comes from the public useNavigationContainerRef export, which returns a module-level ref in Expo Router 57 and 58. Route info is computed from the root navigation state with getRouteInfoFromState, which both versions ship at the same path. On Expo Router 58 the integration listens to the internal actionDispatched event and starts the idle navigation span through a new internal _startIdleNavigationSpan method on reactNavigationIntegration. Expo Router 57 has no exports field, so the missing internalNavigationEvents path is a silent optional dependency there and spans still start from __unsafe_action__. Fixes #6901
Replace the placeholder in the CHANGELOG entry with the PR number, #6923.
Review of #6923 found two regressions for Expo Router versions before 58. Expo Router 55 and older do not ship getRouteInfoFromState, so the require threw and the integration stopped as if expo-router were not installed. The route info module is now optional. Without it, the integration still registers the navigation container and skips the route override, so spans use the React Navigation route names. In Expo Router 56 and 57, useNavigationContainerRef reads a store that stays empty until the Root Layout renders. The ref was read once at Sentry.init, so polling timed out and brought back #6431. The store now reads the ref on every access, and a throwing call returns undefined so it cannot escape the poll timer. The #6431 regression test is restored with the new mocks, and new tests cover a missing getRouteInfoFromState and a throwing useNavigationContainerRef. The CHANGELOG entry moves back to the Unreleased section after the 8.32.0 release.
Expo Router 58 emits actionDispatched for every reducer action, not only user navigations. On a cold launch with tabs it sends ROUTE_NAMES_CHANGED and one PRELOAD per tab, and router.prefetch() and param updates send more PRELOAD, ROUTE_NAMES_CHANGED, and SET_PARAMS actions. expoRouterIntegration does not enable useDispatchedActionData, so startIdleNavigationSpan did not filter these. Each one started an idle span that shipped as an empty "Route Change" transaction on the idle timeout. The listener now forwards only the known navigation action types (NAVIGATE, PUSH, REPLACE, GO_BACK, POP, POP_TO, POP_TO_TOP, JUMP_TO, RESET). An allowlist is safer than a denylist because the router 58 reducer also has internal actions such as REMOVE_ROUTES and NAVIGATOR_CHANGED.
8f147a5 to
2b5d588
Compare
📢 Type of change
📜 Description
expoRouterIntegrationnow gets the navigation container ref from the publicuseNavigationContainerRefexport. It computes route info withgetRouteInfoFromState, which Expo Router 57 and 58 both have at the same path. On Expo Router 58, the integration listens to the internalactionDispatchedevent and starts navigation spans through a new internal_startIdleNavigationSpanmethod onreactNavigationIntegration.💡 Motivation and Context
Expo Router 58 removed
expo-router/build/global-state/router-storeand stopped sending__unsafe_action__. Metro then warned on every bundle, and the integration could not start navigation spans. Fixes #6901.💚 How did you test it?
I ran
expo exportonsamples/expo(expo-router58.0.23). The warning appeared 10 times before this change and 0 times after it. I added unit tests for the new route info source, the action listener, and the listener cleanup.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
On Expo Router 58,
actionDispatchedfires after the new screen renders, so spans start a bit late. A follow-up can use theexpo-router:action-enqueuedperformance mark for an earlier start time.