diff --git a/CHANGELOG.md b/CHANGELOG.md index 0affc909ce..183aa8b307 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ ### Fixes - Fix Android fragment memory leak in `RNSentryReactFragmentLifecycleTracer` ([#6599](https://github.com/getsentry/sentry-react-native/pull/6599)) +- Ship a named navigation transaction when re-navigating to the focused route with new params, instead of an unnamed "Route Change" ([#6606](https://github.com/getsentry/sentry-react-native/pull/6606)) ### Dependencies diff --git a/packages/core/src/js/tracing/reactnavigation.ts b/packages/core/src/js/tracing/reactnavigation.ts index 550aad6236..507773a914 100644 --- a/packages/core/src/js/tracing/reactnavigation.ts +++ b/packages/core/src/js/tracing/reactnavigation.ts @@ -719,13 +719,9 @@ export const reactNavigationIntegration = ({ if (previousRoute?.key === route.key) { debug.log(`[${INTEGRATION_NAME}] Navigation state changed, but route is the same as previous.`); - // Even a same-route state change is a legitimate destination for a - // 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. + // Attribute a pending deep link even to a same-route change (e.g. a deep + // link to the screen you're already on) before deciding on the span. applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs); - 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 @@ -740,14 +736,33 @@ export const reactNavigationIntegration = ({ // 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.`); + pushRecentRouteKey(route.key); + latestRoute = route; clearStateChangeTimeout(); _discardLatestTransaction(); return undefined; } - // Clear the latest transaction as it has been handled. - latestNavigationSpan = undefined; - return undefined; + // Same key and unchanged params is a no-op (drawer toggle, empty + // SET_PARAMS, deep link to the current screen): keep the idle-path + // behaviour and let the span end via the idle timeout. Clear the pending + // discard timeout and end the processing span here so a leftover timer + // cannot fire later and discard the next navigation's span. + if (!haveRouteParamsChanged(previousRoute?.params, route.params)) { + pushRecentRouteKey(route.key); + latestRoute = route; + clearStateChangeTimeout(); + navigationProcessingSpan?.setStatus({ code: SPAN_STATUS_OK }); + navigationProcessingSpan?.end(stateChangedTimestamp); + navigationProcessingSpan = undefined; + latestNavigationSpan = undefined; + return undefined; + } + + // Same key but changed params is a genuine navigation onto the focused + // route (e.g. navigate('Chat', { id: 2 }) from Chat/id:1). Fall through + // to name and finalize the transaction instead of leaking an unnamed + // "Route Change" (#6593). } const routeHasBeenSeen = recentRouteKeys.includes(route.key); @@ -963,6 +978,28 @@ function getRouteNameFromAction(event: UnsafeAction | undefined): string | undef return undefined; } +/** + * Shallow-compares two route param bags to tell a genuine re-navigation onto + * the already-focused route (same `route.key`, changed params) apart from a + * no-op same-route state change (#6593). Missing and empty bags are equal. + */ +function haveRouteParamsChanged( + a: Record | undefined, + b: Record | undefined, +): boolean { + const aKeys = a ? Object.keys(a) : []; + const bKeys = b ? Object.keys(b) : []; + if (aKeys.length !== bKeys.length) { + return true; + } + // Counts are equal, so `a` non-empty implies `b` non-empty. Require every key + // to exist on `b` too — otherwise disjoint bags with matching `undefined` + // values (e.g. `{ x: undefined }` vs `{ y: undefined }`) would compare equal. + const aBag = a ?? {}; + const bBag = b ?? {}; + return aKeys.some(key => !Object.prototype.hasOwnProperty.call(bBag, key) || !Object.is(aBag[key], bBag[key])); +} + /** * Returns React Navigation integration of the given client. */ diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index 5e1c2e93d0..c04743968d 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -1451,6 +1451,199 @@ describe('ReactNavigationInstrumentation', () => { }); }); + describe('NAVIGATE onto the already-focused route with new params (#6593)', () => { + // navigate('Chat', { id: 2 }) while Chat/id:1 is focused keeps the same + // route.key (only params change), but is a genuine screen entry and must + // ship a named transaction, not an unnamed "Route Change". + const navigateToChat: UnsafeAction = { + data: { + action: { type: 'NAVIGATE', payload: { name: 'Chat' } }, + noop: false, + stack: undefined, + }, + }; + + function countTransactionsNamed(name: string): number { + return client.eventQueue.filter(e => e.type === 'transaction' && e.transaction === name).length; + } + + test('ships a named transaction instead of an unnamed "Route Change"', async () => { + // ignoreEmptyBackNavigationTransactions: false isolates the naming + // behaviour from the empty-back-navigation discard. + const instrumentation = reactNavigationIntegration({ + routeChangeTimeoutMs: 200, + ignoreEmptyBackNavigationTransactions: false, + useDispatchedActionData: true, + }); + const options = getDefaultTestClientOptions({ + enableNativeFramesTracking: false, + enableStallTracking: false, + tracesSampleRate: 1.0, + integrations: [instrumentation], + enableAppStartTracking: false, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + mockNavigation = createMockNavigationAndAttachTo(instrumentation); + await jest.runOnlyPendingTimersAsync(); // Flush the initial span + + // Enter Chat for conversation 1. + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 1 } }); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + // Tap a notification for conversation 2 — same route key, new params. + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 2 } }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(countTransactionsNamed('Chat')).toBe(1); + expect(countTransactionsNamed(DEFAULT_NAVIGATION_SPAN_NAME)).toBe(0); + const chat = client.eventQueue.find(e => e.type === 'transaction' && e.transaction === 'Chat'); + expect(chat?.contexts?.trace?.op).toBe('navigation'); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_NAME]).toBe('Chat'); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_KEY]).toBe('chat'); + }); + + test('detects a param change when only disjoint undefined-valued keys differ', async () => { + // { a: undefined } -> { b: undefined }: same key count, disjoint keys. + // haveRouteParamsChanged must still report a change, so the span goes + // through the naming path (which sets route.key/route.name) rather than + // the no-op idle path (which does not). + const instrumentation = reactNavigationIntegration({ + routeChangeTimeoutMs: 200, + ignoreEmptyBackNavigationTransactions: false, + useDispatchedActionData: true, + }); + const options = getDefaultTestClientOptions({ + enableNativeFramesTracking: false, + enableStallTracking: false, + tracesSampleRate: 1.0, + integrations: [instrumentation], + enableAppStartTracking: false, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + mockNavigation = createMockNavigationAndAttachTo(instrumentation); + await jest.runOnlyPendingTimersAsync(); // Flush the initial span + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { a: undefined } }); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { b: undefined } }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + const chat = client.eventQueue.find(e => e.type === 'transaction' && e.transaction === 'Chat'); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_KEY]).toBe('chat'); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_NAME]).toBe('Chat'); + }); + + test('does not leak an unnamed transaction when the re-navigation is empty (default config)', async () => { + // Under the default config an empty same-key re-nav is discarded as an + // empty back navigation, not leaked as an unnamed "Route Change". + setupTestClient({ useDispatchedActionData: true }); + await jest.runOnlyPendingTimersAsync(); // Flush the initial span + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 1 } }); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + // Re-navigate onto the already-focused "Chat" with new params (same key). + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 2 } }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + // Under the default config the re-nav is intentionally discarded as an + // empty back navigation, so neither an unnamed nor a named "Chat" + // transaction ships. + expect(countTransactionsNamed(DEFAULT_NAVIGATION_SPAN_NAME)).toBe(0); + expect(countTransactionsNamed('Chat')).toBe(0); + }); + + test('names the transaction with useDispatchedActionData off (true #6593 repro)', async () => { + // With useDispatchedActionData off the idle span starts unnamed + // (DEFAULT_NAVIGATION_SPAN_NAME). Before the fix, the same-key/changed- + // params branch dropped the span reference without naming it, so it + // self-finalized as an unnamed "Route Change". This is the actual bug + // mode; the fix must derive the name from the mounted route instead. + const instrumentation = reactNavigationIntegration({ + routeChangeTimeoutMs: 200, + ignoreEmptyBackNavigationTransactions: false, + useDispatchedActionData: false, + }); + const options = getDefaultTestClientOptions({ + enableNativeFramesTracking: false, + enableStallTracking: false, + tracesSampleRate: 1.0, + integrations: [instrumentation], + enableAppStartTracking: false, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + mockNavigation = createMockNavigationAndAttachTo(instrumentation); + await jest.runOnlyPendingTimersAsync(); // Flush the initial span + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 1 } }); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 2 } }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + expect(countTransactionsNamed('Chat')).toBe(1); + expect(countTransactionsNamed(DEFAULT_NAVIGATION_SPAN_NAME)).toBe(0); + }); + + test('takes the no-op idle path when the same-key re-nav does not change params', async () => { + // Same key and unchanged params ({ id: 1 } -> { id: 1 }) is a no-op: the + // span must end via the idle timeout without going through the naming + // path. useDispatchedActionData keeps it named "Chat" (from dispatch), so + // the discriminator is the absence of the route.key/route.name attributes + // that only the naming path sets. + const instrumentation = reactNavigationIntegration({ + routeChangeTimeoutMs: 200, + ignoreEmptyBackNavigationTransactions: false, + useDispatchedActionData: true, + }); + const options = getDefaultTestClientOptions({ + enableNativeFramesTracking: false, + enableStallTracking: false, + tracesSampleRate: 1.0, + integrations: [instrumentation], + enableAppStartTracking: false, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + mockNavigation = createMockNavigationAndAttachTo(instrumentation); + await jest.runOnlyPendingTimersAsync(); // Flush the initial span + + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 1 } }); + await jest.runOnlyPendingTimersAsync(); + client.eventQueue = []; + + // Re-dispatch onto the focused route with identical params. + mockNavigation.emitWithStateChange(navigateToChat, { key: 'chat', name: 'Chat', params: { id: 1 } }); + await jest.runOnlyPendingTimersAsync(); + await client.flush(); + + const chat = client.eventQueue.find(e => e.type === 'transaction' && e.transaction === 'Chat'); + expect(chat).toBeDefined(); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_KEY]).toBeUndefined(); + expect(chat?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_ROUTE_NAME]).toBeUndefined(); + }); + }); + test('noop does not remove the previous navigation span from scope', async () => { setupTestClient({ useDispatchedActionData: true }); await jest.runOnlyPendingTimers(); // Flushes the initial navigation span