feat(tracing): Wrap Expo Router push, replace, navigate, back, dismiss in addition to prefetch
#6221
6 issues
Medium
Object hrefs with concrete pathnames bypass `sendDefaultPii` gate - `packages/core/src/js/tracing/expoRouter.ts:152-153`
Any object href with a concrete pathname (e.g. router.push({ pathname: '/users/42' })) is always assigned concretePathname: false in parseHref, so safePathname and safeRouteName at lines 152โ153 always resolve to the raw pathname, exposing it in spans and breadcrumbs even when sendDefaultPii is false.
No test covers `reactnavigation.ts` consuming `pendingExpoRouterNavigation` to tag idle spans - `packages/core/test/tracing/expoRouter.test.ts:8`
The PR's core feature โ attributing idle navigation spans back to the Expo Router call via navigation.method โ is exercised in expoRouter.test.ts only for the setter side; no test in reactnavigation.test.ts (or anywhere else) verifies that reactnavigation.ts actually reads and applies the pending value to the resulting span.
Also found at:
packages/core/src/js/tracing/reactnavigation.ts:33packages/core/src/js/tracing/reactnavigation.ts:455
Object-href pathnames bypass `sendDefaultPii` gating - `packages/core/src/js/tracing/expoRouter.ts:36`
In parseHref, every object href unconditionally sets concretePathname: false, so { pathname: '/users/42' } is treated as a structural template โ its pathname is included in breadcrumbs and spans (data.pathname, span name) even when sendDefaultPii is false. Only string hrefs are gated.
Low
Test incorrectly asserts router is unchanged when only `push` is present - `packages/core/src/js/tracing/expoRouter.ts:72-74`
The test 'returns the router unchanged if prefetch method does not exist' passes only because wrapped === router is a same-reference check โ but with this change router.push is now replaced with a wrapped function and __sentryWrapped is set, so the router is mutated. The test neither verifies that push was wrapped nor catches any regression in that path.
Stale `pendingExpoRouterNavigation` if `startInactiveSpan` throws - `packages/core/src/js/tracing/expoRouter.ts:177`
Consider moving startInactiveSpan (line 177) inside the try block so that a thrown exception clears the pending navigation state and prevents it from incorrectly attributing the next idle navigation span.
Stale `pendingExpoRouterNavigation` misattributes the next navigation's idle span - `packages/core/src/js/tracing/expoRouter.ts:4`
setPendingExpoRouterNavigation is set on every successful navigation call but is only consumed when React Navigation fires a state-change event; if the navigation is a no-op (e.g. already at the target route), the stale value persists and the next unrelated navigation's idle span will inherit the wrong navigation.method attribute.
Also found at:
packages/core/src/js/tracing/reactnavigation.ts:455-458
4 skills analyzed
| Skill | Findings | Duration | Cost |
|---|---|---|---|
| security-review | 0 | 2m | $0.39 |
| code-review | 4 | 8m 14s | $2.16 |
| find-bugs | 2 | 18m 21s | $3.60 |
| gha-security-review | 0 | 8m 32s | $0.10 |
โฑ 37m 7s ยท 2.3M in / 208.1k out ยท $6.26