diff --git a/CHANGELOG.md b/CHANGELOG.md index a9e708fddb..1c4f3a8c6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ > make sure you follow our [migration guide](https://docs.sentry.io/platforms/react-native/migration/) first. +## Unreleased + +### Fixes + +- Fix spurious duplicate navigation transaction created by Expo Router's `withAnchor` bookkeeping `POP_TO` dispatch when `useDispatchedActionData` is enabled ([#6472](https://github.com/getsentry/sentry-react-native/pull/6472)) + ## 8.19.0 ### Features diff --git a/packages/core/src/js/tracing/reactnavigation.ts b/packages/core/src/js/tracing/reactnavigation.ts index 98f858fcc9..0673d33b83 100644 --- a/packages/core/src/js/tracing/reactnavigation.ts +++ b/packages/core/src/js/tracing/reactnavigation.ts @@ -261,7 +261,8 @@ export const reactNavigationIntegration = ({ const spanDispatchSeq = new WeakMap(); /** - * Attempts to attach the pending deep link to the given span. + * Attempts to attach the pending deep link to the given span. Returns `true` + * when the link was attached. * * Warm-open links only attach to spans dispatched *after* the link was * received. This prevents an unrelated, already-in-flight navigation from @@ -274,10 +275,10 @@ export const reactNavigationIntegration = ({ * Rejected warm-open links are left in the slot to be picked up by the next * eligible span. */ - const applyPendingDeepLinkToSpan = (span: Span, maxAgeMs: number): void => { + const applyPendingDeepLinkToSpan = (span: Span, maxAgeMs: number): boolean => { const pending = peekPendingDeepLink(maxAgeMs); if (!pending) { - return; + return false; } if (pending.source === 'warm-open') { const spanSeq = spanDispatchSeq.get(span); @@ -285,11 +286,11 @@ export const reactNavigationIntegration = ({ // Span was dispatched before (or at the same tick as) the link arrived // — it cannot be the navigation the link triggered. Leave the link in // the slot for the next eligible span. - return; + return false; } } consumePendingDeepLink(maxAgeMs); - tagSpanWithDeepLink(span, pending); + return tagSpanWithDeepLink(span, pending); }; /** @@ -442,6 +443,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 +564,23 @@ export const reactNavigationIntegration = ({ return; } + // A POP_TO whose target route is already focused is not a user-facing + // navigation — it's a params-only bookkeeping dispatch. Expo Router emits + // exactly this 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 processed in a deferred microtask, see #6436) or + // ship a spurious duplicate transaction that steals the real navigation's + // child spans. A genuine `popTo` is safe from this filter: its target is + // a route *behind* the focused one, and `__unsafe_action__` fires before + // the state change is applied, so the target is never focused here. + if (navigationActionType === 'POP_TO' && targetRouteName && 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) { @@ -661,10 +705,27 @@ export const reactNavigationIntegration = ({ // deep link (e.g. deep-linking to the screen you're already on). Make // sure the pending link still gets attributed before we drop the span // reference. - applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs); + const deepLinkAttached = applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs); pushRecentRouteKey(route.key); latestRoute = route; + // A POP_TO that landed on the route we were already on was a + // params-only bookkeeping dispatch (e.g. Expo Router's `withAnchor` + // anchor stamping) that slipped past the dispatch-time filter — for + // example 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 a deep link claimed it above. + if ( + !deepLinkAttached && + spanToJSON(latestNavigationSpan).data?.[SEMANTIC_ATTRIBUTE_NAVIGATION_ACTION_TYPE] === '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; diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index d9079a980e..6e6096154f 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -38,7 +38,11 @@ import { mockAppRegistryIntegration } from '../mocks/appRegistryIntegrationMock' import { getDefaultTestClientOptions, TestClient } from '../mocks/client'; import { NATIVE } from '../mockWrapper'; import { getDevServer } from './../../src/js/integrations/debugsymbolicatorutils'; -import { createMockNavigationAndAttachTo, createMockNavigationWithNestedState } from './reactnavigationutils'; +import { + createMockNavigationAndAttachTo, + createMockNavigationWithNestedState, + MockNavigationContainerWithState, +} from './reactnavigationutils'; const dummyRoute = { name: 'Route', @@ -1436,6 +1440,141 @@ describe('ReactNavigationInstrumentation', () => { expect(client.event?.transaction).toBe('TabScreen'); }); + test('POP_TO action to a different route creates a named span', async () => { + mockNavigation.emitWithStateChange( + { + data: { + action: { + type: 'POP_TO', + payload: { name: 'ScreenB' }, + }, + noop: false, + stack: undefined, + }, + }, + { key: 'screen_b', name: 'ScreenB' }, + ); + jest.runOnlyPendingTimers(); + + await client.flush(); + + expect(client.event?.transaction).toBe('ScreenB'); + }); + + test('POP_TO action targeting the focused route does not create a navigation span', async () => { + // Real navigation to New Screen. + mockNavigation.emitWithStateChange( + { + data: { + action: { + type: 'NAVIGATE', + payload: { name: 'New Screen' }, + }, + noop: false, + stack: undefined, + }, + }, + { key: 'new_screen', name: 'New Screen' }, + ); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + expect(client.event?.transaction).toBe('New Screen'); + client.event = undefined; + + // Expo Router `withAnchor` bookkeeping dispatch: POP_TO stamping + // `initial: false` onto the route that is already focused. + mockNavigation.emitWithStateChange({ + data: { + action: { + type: 'POP_TO', + payload: { name: 'New Screen', params: { initial: false } }, + }, + noop: false, + stack: undefined, + }, + }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.event).toBeUndefined(); + }); + + test('POP_TO action targeting the focused route does not remove the in-flight navigation span', async () => { + mockNavigation.emitNavigationWithoutStateChange(); + const activeSpan = getActiveSpan(); + + mockNavigation.emitWithoutStateChange({ + data: { + action: { + type: 'POP_TO', + payload: { name: 'Initial Screen', params: { initial: false } }, + }, + noop: false, + stack: undefined, + }, + }); + + expect(getActiveSpan()).toBe(activeSpan); + }); + + test('POP_TO span that did not change the route is discarded', async () => { + // Payload name matches no focused route (e.g. a nested navigator name), + // so the dispatch-time filter misses, but the state change lands on the + // same route key — the span must be discarded, not left running. + 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).toBeUndefined(); + }); + + test('POP_TO span that did not change the route is kept when it carries a deep link', async () => { + 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('NAVIGATE landing on the same route still creates a span', async () => { + mockNavigation.emitWithStateChange({ + data: { + action: { + type: 'NAVIGATE', + payload: { name: 'Initial Screen' }, + }, + noop: false, + stack: undefined, + }, + }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.event?.transaction).toBe('Initial Screen'); + }); + test('cancelled navigation with dispatched route name is discarded', async () => { mockNavigation.emitWithoutStateChange({ data: { @@ -1483,6 +1622,66 @@ describe('ReactNavigationInstrumentation', () => { }); }); + test('POP_TO targeting a focused parent navigator route does not create a navigation span', async () => { + // Expo Router's `withAnchor` bookkeeping POP_TO carries the route name of + // the divergent navigator level, which is often a parent of the leaf + // route — the focused-route check must walk the whole focused chain. + const rNavigation = reactNavigationIntegration({ + routeChangeTimeoutMs: 200, + useDispatchedActionData: true, + }); + + const container = new MockNavigationContainerWithState(); + container.currentRoute = { key: 'screen_b', name: 'screenB' }; + container.currentState = { + index: 0, + routes: [ + { + name: '(app)', + key: 'app', + state: { + index: 1, + routes: [ + { name: 'screenA', key: 'screen_a' }, + { name: 'screenB', key: 'screen_b' }, + ], + }, + }, + ], + }; + + const options = getDefaultTestClientOptions({ + enableNativeFramesTracking: false, + enableStallTracking: false, + tracesSampleRate: 1.0, + integrations: [rNavigation, reactNativeTracingIntegration()], + enableAppStartTracking: false, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + rNavigation.registerNavigationContainer(container); + + await jest.runOnlyPendingTimersAsync(); // Flush the initial navigation span + client.event = undefined; + + container.listeners['__unsafe_action__']({ + data: { + action: { + type: 'POP_TO', + payload: { name: '(app)', params: { initial: false } }, + }, + noop: false, + stack: undefined, + }, + }); + container.listeners['state']({}); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(client.event).toBeUndefined(); + }); + describe('useDispatchedActionData disabled (default) still uses generic span name', () => { beforeEach(async () => { setupTestClient({ useDispatchedActionData: false });