diff --git a/CHANGELOG.md b/CHANGELOG.md index f7866be9aa..f44eb82131 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ ## Unreleased +### Fixes + +- Fix duplicate navigation transaction on Expo Router `withAnchor` navigations ([#6439](https://github.com/getsentry/sentry-react-native/pull/6439)) + ### Dependencies - Bump Android SDK from v8.49.0 to v8.50.1 ([#6503](https://github.com/getsentry/sentry-react-native/pull/6503)) diff --git a/packages/core/src/js/tracing/reactnavigation.ts b/packages/core/src/js/tracing/reactnavigation.ts index 98f858fcc9..550aad6236 100644 --- a/packages/core/src/js/tracing/reactnavigation.ts +++ b/packages/core/src/js/tracing/reactnavigation.ts @@ -237,6 +237,10 @@ export const reactNavigationIntegration = ({ let latestNavigationSpan: Span | undefined; let latestNavigationSpanNameCustomized: boolean = false; + // Action type of the dispatch that started `latestNavigationSpan`. Tracked + // independently of the (flag-gated) span attribute so the same-route discard + // works even when `useDispatchedActionData` is off. + let latestNavigationActionType: string | undefined; let navigationProcessingSpan: Span | undefined; /** * The first nav span that successfully completed a state change — i.e. the @@ -442,6 +446,32 @@ export const reactNavigationIntegration = ({ initialStateHandled = true; }; + /** + * Returns `true` when the given route name is focused at some level of the + * current navigation state — i.e. it is on the chain of active routes from + * the root navigator down to the leaf. Falls back to comparing against the + * leaf route only when the full state is not available. + */ + const isRouteFocused = (routeName: string): boolean => { + try { + const rootState = navigationContainer?.getState(); + let currentState: NavigationState | undefined = rootState; + while (currentState) { + const route: NavigationRoute | undefined = currentState.routes[currentState.index ?? 0]; + if (route?.name === routeName) { + return true; + } + currentState = route?.state; + } + if (!rootState) { + return navigationContainer?.getCurrentRoute()?.name === routeName; + } + } catch (e) { + debug.warn(`${INTEGRATION_NAME} Failed to read navigation state to check focused route.`, e); + } + return false; + }; + /** * To be called on every React-Navigation action dispatch. * It does not name the transaction or populate it with route information. Instead, it waits for the state to fully change @@ -537,6 +567,37 @@ export const reactNavigationIntegration = ({ return; } + // A `POP_TO` that both targets an already-focused route AND carries the + // `withAnchor` marker (`params.initial === false`) is not a user-facing + // navigation — it's Expo Router's bookkeeping dispatch, emitted right after + // a `withAnchor` navigation purely to stamp `initial: false` onto the + // destination it just navigated to. Starting a span here would either + // discard the real navigation's in-flight span (state changes are applied + // in a deferred microtask, see #6436) or ship a spurious duplicate + // transaction that steals the real navigation's child spans (#6434). + // + // Requiring the `initial === false` marker (only ever set by `withAnchor`, + // see expo-router's `getNavigationAction`) is what keeps a genuine `popTo` + // to an earlier *same-named* route in the stack (e.g. `[id]` → `[id]`) from + // being wrongly skipped: those carry no `initial` param. The `route.key` + // check in `updateLatestNavigationSpanWithCurrentRoute` backstops any + // bookkeeping dispatch that slips past this filter. + // Driven off `actionType` (parsed unconditionally), not `navigationActionType`, + // so it also applies to the default `expoRouterIntegration` setup where + // `useDispatchedActionData` is off — that is where this bug was reported. + const popToParams = (event?.data?.action?.payload as { params?: { initial?: boolean } } | undefined)?.params; + if ( + actionType === 'POP_TO' && + targetRouteName && + popToParams?.initial === false && + isRouteFocused(targetRouteName) + ) { + debug.log( + `${INTEGRATION_NAME} POP_TO targets the already focused route ${targetRouteName}, not starting navigation span.`, + ); + return; + } + // Extract route name from dispatch action payload when available const dispatchedRouteName = useDispatchedActionData ? targetRouteName : undefined; if (useDispatchedActionData && event && !dispatchedRouteName && !isAppRestart) { @@ -562,6 +623,7 @@ export const reactNavigationIntegration = ({ latestNavigationSpanNameCustomized = finalSpanOptions.name !== originalName; latestNavigationSpan = startGenericIdleNavigationSpan(finalSpanOptions, { ...idleSpanOptions, isAppRestart }); + latestNavigationActionType = actionType; latestNavigationSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NAVIGATION); latestNavigationSpan?.setAttribute(SEMANTIC_ATTRIBUTE_NAVIGATION_ACTION_TYPE, navigationActionType); @@ -665,6 +727,24 @@ export const reactNavigationIntegration = ({ pushRecentRouteKey(route.key); latestRoute = route; + // A `POP_TO` span that landed on the route we were already on (same + // `route.key`) was a params-only bookkeeping dispatch — e.g. Expo + // Router's `withAnchor` anchor stamping — that slipped past the + // dispatch-time filter (for instance a nested destination whose payload + // name matches none of the focused route names). Letting the span run + // would ship a spurious duplicate transaction that collects the real + // navigation's in-flight child spans, so discard it unless it carries a + // deep link (#6434). We check `taggedDeepLinkSpans` rather than the + // just-attached result so a span tagged earlier via the synchronous + // late-arrival listener is also preserved. Keyed on `route.key`, this + // never affects a genuine navigation that actually changed the route. + if (!taggedDeepLinkSpans.has(latestNavigationSpan) && latestNavigationActionType === 'POP_TO') { + debug.log(`[${INTEGRATION_NAME}] Discarding POP_TO navigation span that did not change the route.`); + clearStateChangeTimeout(); + _discardLatestTransaction(); + return undefined; + } + // Clear the latest transaction as it has been handled. latestNavigationSpan = undefined; return undefined; @@ -777,6 +857,10 @@ export const reactNavigationIntegration = ({ latestNavigationSpan = undefined; } if (navigationProcessingSpan) { + // End before dropping the reference so we don't leave an unfinished span + // dangling. It is a child of the (now discarded) navigation span, so it + // is dropped with the transaction rather than sent on its own. + navigationProcessingSpan.end(); navigationProcessingSpan = undefined; } }; diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index d9079a980e..5e1c2e93d0 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -11,6 +11,7 @@ import { } from '@sentry/core'; import type { NavigationRoute } from '../../src/js/tracing/reactnavigation'; +import type { UnsafeAction } from '../../src/js/vendor/react-navigation/types'; import { nativeFramesIntegration, reactNativeTracingIntegration } from '../../src/js'; import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NAVIGATION } from '../../src/js/tracing/origin'; @@ -1277,6 +1278,179 @@ describe('ReactNavigationInstrumentation', () => { }); }); + describe('withAnchor POP_TO bookkeeping dispatch (#6434)', () => { + // `router.dismissTo('/ScreenB', { withAnchor: true })` navigates to ScreenB + // (a real POP_TO) and, on some expo-router versions, immediately issues a + // second POP_TO to the same route purely to stamp `initial: false`. That + // bookkeeping dispatch carries no state change of its own and must not + // produce a second, spurious navigation transaction. + const realPopTo: UnsafeAction = { + data: { + action: { type: 'POP_TO', payload: { name: 'ScreenB', params: {} } }, + noop: false, + stack: undefined, + }, + }; + const bookkeepingPopTo: UnsafeAction = { + data: { + action: { type: 'POP_TO', payload: { name: 'ScreenB', params: { initial: false } } }, + noop: false, + stack: undefined, + }, + }; + + function countTransactionsNamed(name: string): number { + return client.eventQueue.filter(e => e.type === 'transaction' && e.transaction === name).length; + } + + test('does not create a second transaction for the bookkeeping dispatch', async () => { + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); // Flush the initial navigation span + client.eventQueue = []; + + // Real navigation to ScreenB — resolves via a state change. + mockNavigation.emitWithStateChange(realPopTo, { key: 'screen_b', name: 'ScreenB' }); + await jest.runOnlyPendingTimersAsync(); + + // withAnchor bookkeeping POP_TO to the same route. It only flips + // `params.initial`, which React Navigation reports as a state change to + // the same route key. + mockNavigation.emitWithStateChange(bookkeepingPopTo, { key: 'screen_b', name: 'ScreenB' }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(countTransactionsNamed('ScreenB')).toBe(1); + }); + + test('does not create a second transaction with useDispatchedActionData disabled (default Expo setup)', async () => { + // The guards are driven off `actionType`, not the opt-in flag, so the + // default `expoRouterIntegration` setup (flag off) is fixed too. + setupTestClient({ useDispatchedActionData: false }); + await jest.runOnlyPendingTimers(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange(realPopTo, { key: 'screen_b', name: 'ScreenB' }); + await jest.runOnlyPendingTimersAsync(); + + mockNavigation.emitWithStateChange(bookkeepingPopTo, { key: 'screen_b', name: 'ScreenB' }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.eventQueue.filter(e => e.type === 'transaction').length).toBe(1); + }); + + test('still creates a transaction for a genuine popTo navigation', async () => { + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); // Flush the initial navigation span + client.eventQueue = []; + + // A real dismissTo/popTo to a different route must still be traced. + mockNavigation.emitWithStateChange(realPopTo, { key: 'screen_b', name: 'ScreenB' }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(countTransactionsNamed('ScreenB')).toBe(1); + }); + + test('still creates a span for a popTo to an earlier same-named route in the stack', async () => { + // Stack with a repeated route name (e.g. Expo Router `[id]`): on Detail(#2), + // popTo the earlier Detail(#1). The target name matches the focused leaf + // name, but the bookkeeping marker (`initial: false`) is absent, so this + // real, key-changing navigation must NOT be filtered. + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange( + { data: { action: { type: 'PUSH', payload: { name: 'Detail' } }, noop: false, stack: undefined } }, + { key: 'detail_2', name: 'Detail' }, + ); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange( + { data: { action: { type: 'POP_TO', payload: { name: 'Detail' } }, noop: false, stack: undefined } }, + { key: 'detail_1', name: 'Detail' }, + ); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(countTransactionsNamed('Detail')).toBe(1); + }); + + test('discards a bookkeeping POP_TO that slips past the dispatch-time filter (same route key)', async () => { + // Payload name matches no focused route (e.g. a parent navigator name), + // so the dispatch-time filter misses, but the state change lands on the + // same route key — the state-change guard must discard the span. + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange({ + data: { + action: { type: 'POP_TO', payload: { name: '(app)', params: { initial: false } } }, + noop: false, + stack: undefined, + }, + }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.eventQueue.filter(e => e.type === 'transaction').length).toBe(0); + }); + + test('keeps a same-route POP_TO span that carries a deep link', async () => { + // A same-route POP_TO that is actually a deep link to the screen you are + // already on must NOT be discarded by the bookkeeping guard. + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); + client.eventQueue = []; + + setPendingDeepLink('myapp://initial-screen', 'warm-open'); + mockNavigation.emitWithStateChange({ + data: { + action: { type: 'POP_TO', payload: { name: '(app)', params: { initial: false } } }, + noop: false, + stack: undefined, + }, + }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.event?.contexts?.trace?.data?.['navigation.trigger']).toBe('deeplink'); + clearPendingDeepLink(); + }); + + test('a focused-route bookkeeping POP_TO does not tear down an in-flight navigation span', async () => { + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimers(); + + // Start a real, still-in-flight navigation (dispatch with a route name in + // the payload, but no state change yet). + mockNavigation.emitWithoutStateChange({ + data: { + action: { type: 'NAVIGATE', payload: { name: 'New Screen' } }, + noop: false, + stack: undefined, + }, + }); + const activeSpan = getActiveSpan(); + expect(activeSpan).toBeDefined(); + + // The withAnchor bookkeeping POP_TO to the already-focused route must not + // discard that in-flight span via the "noop transaction" branch. + mockNavigation.emitWithoutStateChange({ + data: { + action: { type: 'POP_TO', payload: { name: 'Initial Screen', params: { initial: false } } }, + noop: false, + stack: undefined, + }, + }); + + expect(getActiveSpan()).toBe(activeSpan); + }); + }); + test('noop does not remove the previous navigation span from scope', async () => { setupTestClient({ useDispatchedActionData: true }); await jest.runOnlyPendingTimers(); // Flushes the initial navigation span