Skip to content

Commit a2c3a6a

Browse files
committed
fix(tracing): Address PR review on deep-link / navigation correlation
Addresses three findings from Cursor Bugbot and one from Sentry's review bot on #6264: * **Late deep link never tags span** (Cursor, medium). When Expo Router auto-handles `Linking.getInitialURL()` and finishes the initial navigation *before* our integration's own `getInitialURL().then(...)` chain resolves, the URL was never attributed to the resulting span. Fix: `pendingDeepLink` now supports a synchronous listener that the navigation integration registers in `afterAllSetup`. When a link arrives, the listener tags the in-flight (`latestNavigationSpan`) or most-recent still-recording (`lastIdleNavSpan`) span directly. If nothing live exists, the link falls through to the slot for the next dispatched navigation. A WeakSet guards against double-tagging. * **Same-route path skips deeplink retry** (Cursor, low). The early return in `updateLatestNavigationSpanWithCurrentRoute` for matching route keys (legitimate when deep-linking to the screen you're already on) bypassed the attribution call. Fix: consume + tag in that branch too, before the span reference is dropped. * **Early consume loses pending link** (Cursor, medium). Consuming the pending value inside `startIdleNavigationSpan` meant a later discard (noop / timeout / empty route) silently wasted the link — the real link-driven navigation that followed would not be attributed. Fix: removed the eager consume on dispatch. Pending values are now only consumed in `updateLatestNavigationSpanWithCurrentRoute` (post route mount) or by the listener (live span). Discards never touch the slot. * **`deeplink.received_at` semantically misleading** (Sentry, medium). The attribute stored a duration but its `_at` suffix conventionally denotes a timestamp. Renamed to `deeplink.dispatch_delay_ms`, which accurately reflects the measured gap between URL receipt and span annotation. Plus: changelog entry now references PR #6264 (Danger gate).
1 parent ff90614 commit a2c3a6a

6 files changed

Lines changed: 263 additions & 39 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
### Features
1212

13-
- Correlate deep links with the navigation transaction they trigger. The next idle navigation span started within `routeChangeTimeoutMs` of a deep link arrival is tagged with `navigation.trigger: 'deeplink'`, `deeplink.url` (sanitized, respects `sendDefaultPii`), and `deeplink.received_at` (ms gap between URL received and navigation dispatched). Covers both cold start (`Linking.getInitialURL()`) and warm open (`'url'` event) paths ([#6159](https://github.com/getsentry/sentry-react-native/issues/6159))
13+
- Correlate deep links with the navigation transaction they trigger. The next idle navigation span started within `routeChangeTimeoutMs` of a deep link arrival is tagged with `navigation.trigger: 'deeplink'`, `deeplink.url` (sanitized, respects `sendDefaultPii`), and `deeplink.dispatch_delay_ms` (ms gap between URL received and navigation dispatched). Covers both cold start (`Linking.getInitialURL()`) and warm open (`'url'` event) paths, including the late-arrival case where Expo Router auto-handles the link before our `getInitialURL()` chain resolves ([#6264](https://github.com/getsentry/sentry-react-native/pull/6264))
1414
- Add memory, CPU, and frame measurements to Android profiling ([#6250](https://github.com/getsentry/sentry-react-native/pull/6250))
1515
- Add `enableAutoConsoleLogs` option to opt out of automatic `console.*` capture while keeping `enableLogs: true` for manual `Sentry.logger.*` calls ([#6235](https://github.com/getsentry/sentry-react-native/pull/6235))
1616
- Instrument Expo Router `push`, `replace`, `navigate`, `back`, and `dismiss` (in addition to `prefetch`) with breadcrumbs and spans, and tag the resulting idle navigation span with the initiating `navigation.method` ([#6221](https://github.com/getsentry/sentry-react-native/pull/6221))

packages/core/etc/sentry-react-native.api.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -857,7 +857,7 @@ export function wrapExpoRouter<T extends ExpoRouter>(router: T): T;
857857
// src/js/feedback/integration.ts:21:5 - (ae-forgotten-export) The symbol "ScreenshotButtonProps" needs to be exported by the entry point index.d.ts
858858
// src/js/feedback/integration.ts:23:5 - (ae-forgotten-export) The symbol "FeedbackFormTheme" needs to be exported by the entry point index.d.ts
859859
// src/js/tracing/reactnativetracing.ts:90:3 - (ae-forgotten-export) The symbol "ReactNativeTracingState" needs to be exported by the entry point index.d.ts
860-
// src/js/tracing/reactnavigation.ts:222:3 - (ae-forgotten-export) The symbol "RouteOverrideProvider" needs to be exported by the entry point index.d.ts
860+
// src/js/tracing/reactnavigation.ts:224:3 - (ae-forgotten-export) The symbol "RouteOverrideProvider" needs to be exported by the entry point index.d.ts
861861

862862
// (No @packageDocumentation comment for this package)
863863

packages/core/src/js/tracing/pendingDeepLink.ts

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,20 @@
22
* Cross-module hand-off between the {@link deeplinkIntegration} and the
33
* {@link reactNavigationIntegration} idle navigation span.
44
*
5-
* When a deep link is received (either via `Linking.getInitialURL()` on cold
6-
* start or via the `'url'` event on warm open), the integration stores the
7-
* raw URL together with a receive timestamp here. The navigation integration
8-
* then attaches it to the next idle navigation span started within
9-
* `routeChangeTimeoutMs` (default 1000ms), so traces can correlate
10-
* "deep link → navigation" timing.
5+
* Two delivery modes are supported, both of which need to work in practice:
6+
*
7+
* 1. **Pre-navigation (warm open / normal cold start):** the deep link is
8+
* received before any navigation has been dispatched. The URL is stored in
9+
* a single slot here; the next idle navigation span consumes it inside
10+
* `updateLatestNavigationSpanWithCurrentRoute` (within `routeChangeTimeoutMs`).
11+
*
12+
* 2. **Late arrival (Expo Router auto-handled cold start):** Expo Router reads
13+
* `Linking.getInitialURL()` independently and may finish the initial
14+
* navigation *before* our integration's own `getInitialURL().then(...)`
15+
* chain resolves. To still attribute that span, a synchronous listener may
16+
* be registered (by the navigation integration) and receives every link as
17+
* it arrives. If it tags a still-recording span, it returns `true` and the
18+
* slot is left empty — otherwise the link falls through to the slot.
1119
*/
1220

1321
export interface PendingDeepLink {
@@ -17,20 +25,36 @@ export interface PendingDeepLink {
1725
receivedAtMs: number;
1826
}
1927

28+
/**
29+
* Synchronously notified for every deep link as it arrives. A `true` return
30+
* value indicates the listener has already attributed the link to a live span,
31+
* and the value should NOT be stored for a future navigation.
32+
*/
33+
export type PendingDeepLinkListener = (link: PendingDeepLink) => boolean;
34+
2035
let pending: PendingDeepLink | undefined;
36+
let listener: PendingDeepLinkListener | undefined;
2137

2238
/**
2339
* Stores the most recently received deep link URL together with the current
24-
* timestamp. Overwrites any previous pending value — only the latest link
40+
* timestamp. If a listener is registered and consumes the link synchronously,
41+
* the slot is left empty.
42+
*
43+
* Overwrites any previous unconsumed pending value — only the latest link
2544
* matters for correlation with the next navigation.
2645
*/
2746
export function setPendingDeepLink(url: string): void {
28-
pending = { url, receivedAtMs: Date.now() };
47+
const value: PendingDeepLink = { url, receivedAtMs: Date.now() };
48+
if (listener?.(value)) {
49+
return;
50+
}
51+
pending = value;
2952
}
3053

3154
/**
3255
* Returns and clears the pending deep link, but only if it was received
33-
* within `maxAgeMs` of "now". Stale entries are discarded.
56+
* within `maxAgeMs` of "now". Stale entries are discarded and the slot is
57+
* cleared in all cases.
3458
*/
3559
export function consumePendingDeepLink(maxAgeMs: number): PendingDeepLink | undefined {
3660
const value = pending;
@@ -44,7 +68,17 @@ export function consumePendingDeepLink(maxAgeMs: number): PendingDeepLink | unde
4468
return value;
4569
}
4670

47-
/** Test helper — clears the pending value without consuming it. */
71+
/**
72+
* Registers a synchronous listener that is invoked on every {@link setPendingDeepLink}
73+
* call. Pass `undefined` to unregister. Only a single listener is supported —
74+
* a new registration replaces the previous one.
75+
*/
76+
export function setPendingDeepLinkListener(fn: PendingDeepLinkListener | undefined): void {
77+
listener = fn;
78+
}
79+
80+
/** Test helper — clears the pending value and listener without consuming them. */
4881
export function clearPendingDeepLink(): void {
4982
pending = undefined;
83+
listener = undefined;
5084
}

packages/core/src/js/tracing/reactnavigation.ts

Lines changed: 82 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
} from '@sentry/core';
1616

1717
import type { UnsafeAction } from '../vendor/react-navigation/types';
18+
import type { PendingDeepLink } from './pendingDeepLink';
1819
import type { ReactNativeTracingIntegration } from './reactnativetracing';
1920

2021
import { getAppRegistryIntegration } from '../integrations/appRegistry';
@@ -28,7 +29,7 @@ import {
2829
markRootSpanForDiscard,
2930
} from './onSpanEndUtils';
3031
import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NAVIGATION } from './origin';
31-
import { consumePendingDeepLink } from './pendingDeepLink';
32+
import { consumePendingDeepLink, setPendingDeepLinkListener } from './pendingDeepLink';
3233
import { consumePendingExpoRouterNavigation } from './pendingExpoRouterNavigation';
3334
import { getReactNativeTracingIntegration } from './reactnativetracing';
3435
import { SEMANTIC_ATTRIBUTE_NAVIGATION_ACTION_TYPE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from './semanticAttributes';
@@ -232,7 +233,28 @@ export const reactNavigationIntegration = ({
232233
let latestNavigationSpan: Span | undefined;
233234
let latestNavigationSpanNameCustomized: boolean = false;
234235
let navigationProcessingSpan: Span | undefined;
235-
let deepLinkAppliedToLatestSpan: boolean = false;
236+
/**
237+
* Most recently created idle navigation span, retained even after
238+
* `latestNavigationSpan` is cleared on state change. Used by the deep-link
239+
* listener to attribute a link that arrives between "route mounted" and
240+
* "idle span ended". Cleared when superseded by the next nav span.
241+
*/
242+
let lastIdleNavSpan: Span | undefined;
243+
244+
/**
245+
* Synchronous listener invoked the moment a deep link is recorded. If a live
246+
* idle navigation span exists, tag it directly and tell the pendingDeepLink
247+
* module that the link has been consumed — otherwise let the link fall through
248+
* to the slot so the next dispatched navigation picks it up.
249+
*/
250+
const handleLateDeepLink = (link: PendingDeepLink): boolean => {
251+
const span = latestNavigationSpan ?? lastIdleNavSpan;
252+
if (!span || !isSpanRecording(span)) {
253+
return false;
254+
}
255+
tagSpanWithDeepLink(span, link);
256+
return true;
257+
};
236258

237259
let initialStateHandled: boolean = false;
238260
let isSetupComplete: boolean = false;
@@ -257,6 +279,14 @@ export const reactNavigationIntegration = ({
257279
};
258280
}
259281

282+
// Listen for deep links as they arrive so we can attribute a span that has
283+
// already mounted its route but not yet ended (e.g. Expo Router auto-handled
284+
// the link before our integration's `getInitialURL()` chain resolved).
285+
setPendingDeepLinkListener(handleLateDeepLink);
286+
client.on('close', () => {
287+
setPendingDeepLinkListener(undefined);
288+
});
289+
260290
if (initialStateHandled) {
261291
// We create an initial state here to ensure a transaction gets created before the first route mounts.
262292
// This assumes that the Sentry.init() call is made before the first route mounts.
@@ -467,14 +497,12 @@ export const reactNavigationIntegration = ({
467497
latestNavigationSpan.setAttribute('navigation.method', pendingExpoRouter.method);
468498
}
469499

470-
// Try to attribute this span to a recently received deep link. Covers the
471-
// warm-open case (link arrives → navigation dispatches). The cold-start
472-
// case is handled again in `updateLatestNavigationSpanWithCurrentRoute`
473-
// because `Linking.getInitialURL()` may resolve after this point.
474-
deepLinkAppliedToLatestSpan = false;
475-
if (latestNavigationSpan) {
476-
deepLinkAppliedToLatestSpan = applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs);
477-
}
500+
// Hold a reference to the freshly-created idle span so the deep-link
501+
// listener can still annotate it after `latestNavigationSpan` is cleared
502+
// on state change. We deliberately do NOT consume the pending deep link
503+
// here — if this span is later discarded (noop / timeout / empty route),
504+
// a still-fresh pending value must remain available for the next nav.
505+
lastIdleNavSpan = latestNavigationSpan;
478506
if (ignoreEmptyBackNavigationTransactions) {
479507
ignoreEmptyBackNavigation(getClient(), latestNavigationSpan);
480508
}
@@ -531,6 +559,11 @@ export const reactNavigationIntegration = ({
531559

532560
if (previousRoute?.key === route.key) {
533561
debug.log(`[${INTEGRATION_NAME}] Navigation state changed, but route is the same as previous.`);
562+
// Even a same-route state change is a legitimate destination for a
563+
// deep link (e.g. deep-linking to the screen you're already on). Make
564+
// sure the pending link still gets attributed before we drop the span
565+
// reference.
566+
applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs);
534567
pushRecentRouteKey(route.key);
535568
latestRoute = route;
536569

@@ -567,12 +600,10 @@ export const reactNavigationIntegration = ({
567600
routeName = getPathFromState(navigationState) || route.name;
568601
}
569602

570-
// Cold-start fallback: if the deep link arrived *after* the idle navigation
571-
// span was started (e.g. `getInitialURL()` resolved post-`afterAllSetup`),
572-
// try again now that the route has mounted.
573-
if (!deepLinkAppliedToLatestSpan) {
574-
deepLinkAppliedToLatestSpan = applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs);
575-
}
603+
// Consume any pending deep link and attach it to this span. Done here
604+
// (after route info is known) so the link is only attributed to a span
605+
// that actually mounted a route — not one that was later discarded.
606+
applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs);
576607

577608
navigationProcessingSpan?.updateName(`Navigation dispatch to screen ${routeName} mounted`);
578609
navigationProcessingSpan?.setStatus({ code: SPAN_STATUS_OK });
@@ -619,7 +650,6 @@ export const reactNavigationIntegration = ({
619650
}
620651
// Clear the latest transaction as it has been handled.
621652
latestNavigationSpan = undefined;
622-
deepLinkAppliedToLatestSpan = false;
623653
};
624654

625655
/** Pushes a recent route key, and removes earlier routes when there is greater than the max length */
@@ -644,7 +674,6 @@ export const reactNavigationIntegration = ({
644674
if (navigationProcessingSpan) {
645675
navigationProcessingSpan = undefined;
646676
}
647-
deepLinkAppliedToLatestSpan = false;
648677
};
649678

650679
const clearStateChangeTimeout = (): void => {
@@ -701,20 +730,49 @@ interface NavigationContainer {
701730
* `false` otherwise. Callers may invoke this multiple times against the same
702731
* span — once the pending value has been consumed it will not be re-applied.
703732
*/
704-
function applyPendingDeepLinkToSpan(span: Span, maxAgeMs: number): boolean {
705-
const pending = consumePendingDeepLink(maxAgeMs);
706-
if (!pending) {
707-
return false;
733+
/**
734+
* Per-span guard against double-tagging deep-link attributes. Shared between
735+
* the synchronous listener path (late arrival) and the post-state-change path.
736+
*/
737+
const taggedDeepLinkSpans = new WeakSet<Span>();
738+
739+
/**
740+
* Annotates the given span with deep-link attributes if it has not already
741+
* been annotated. Safe to call multiple times — a span is tagged at most once.
742+
*/
743+
function tagSpanWithDeepLink(span: Span, link: PendingDeepLink): void {
744+
if (taggedDeepLinkSpans.has(span)) {
745+
return;
708746
}
747+
taggedDeepLinkSpans.add(span);
709748

710749
const sendDefaultPii = getClient()?.getOptions()?.sendDefaultPii ?? false;
711-
const url = sendDefaultPii ? pending.url : sanitizeDeepLinkUrl(pending.url);
750+
const url = sendDefaultPii ? link.url : sanitizeDeepLinkUrl(link.url);
712751

713752
span.setAttributes({
714753
'navigation.trigger': 'deeplink',
715754
'deeplink.url': url,
716-
'deeplink.received_at': Math.max(0, Date.now() - pending.receivedAtMs),
755+
// Duration between URL receipt and the moment the span is annotated —
756+
// approximates the gap between "link received" and "navigation dispatched
757+
// / handled".
758+
'deeplink.dispatch_delay_ms': Math.max(0, Date.now() - link.receivedAtMs),
717759
});
760+
}
761+
762+
/** Returns true if the span is still recording (has not been ended). */
763+
function isSpanRecording(span: Span): boolean {
764+
return spanToJSON(span).timestamp === undefined;
765+
}
766+
767+
function applyPendingDeepLinkToSpan(span: Span, maxAgeMs: number): boolean {
768+
if (taggedDeepLinkSpans.has(span)) {
769+
return true;
770+
}
771+
const pending = consumePendingDeepLink(maxAgeMs);
772+
if (!pending) {
773+
return false;
774+
}
775+
tagSpanWithDeepLink(span, pending);
718776
return true;
719777
}
720778

packages/core/test/tracing/pendingDeepLink.test.ts

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1-
import { clearPendingDeepLink, consumePendingDeepLink, setPendingDeepLink } from '../../src/js/tracing/pendingDeepLink';
1+
import {
2+
clearPendingDeepLink,
3+
consumePendingDeepLink,
4+
setPendingDeepLink,
5+
setPendingDeepLinkListener,
6+
} from '../../src/js/tracing/pendingDeepLink';
27

38
describe('pendingDeepLink', () => {
49
afterEach(() => {
@@ -54,4 +59,53 @@ describe('pendingDeepLink', () => {
5459
clearPendingDeepLink();
5560
expect(consumePendingDeepLink(1_000)).toBeUndefined();
5661
});
62+
63+
describe('listener', () => {
64+
it('is invoked synchronously on every set, with url + timestamp', () => {
65+
const received: Array<{ url: string; receivedAtMs: number }> = [];
66+
setPendingDeepLinkListener(link => {
67+
received.push({ url: link.url, receivedAtMs: link.receivedAtMs });
68+
return false;
69+
});
70+
71+
setPendingDeepLink('myapp://a');
72+
setPendingDeepLink('myapp://b');
73+
74+
expect(received.map(r => r.url)).toEqual(['myapp://a', 'myapp://b']);
75+
expect(received[0]?.receivedAtMs).toBeGreaterThan(0);
76+
});
77+
78+
it('skips storage when the listener returns true (already consumed)', () => {
79+
setPendingDeepLinkListener(() => true);
80+
setPendingDeepLink('myapp://consumed-by-listener');
81+
82+
expect(consumePendingDeepLink(1_000)).toBeUndefined();
83+
});
84+
85+
it('falls through to storage when the listener returns false', () => {
86+
setPendingDeepLinkListener(() => false);
87+
setPendingDeepLink('myapp://stored');
88+
89+
expect(consumePendingDeepLink(1_000)?.url).toBe('myapp://stored');
90+
});
91+
92+
it('can be unregistered with undefined', () => {
93+
const fn = jest.fn().mockReturnValue(true);
94+
setPendingDeepLinkListener(fn);
95+
setPendingDeepLinkListener(undefined);
96+
97+
setPendingDeepLink('myapp://x');
98+
expect(fn).not.toHaveBeenCalled();
99+
expect(consumePendingDeepLink(1_000)?.url).toBe('myapp://x');
100+
});
101+
102+
it('clearPendingDeepLink also removes the listener', () => {
103+
const fn = jest.fn().mockReturnValue(true);
104+
setPendingDeepLinkListener(fn);
105+
clearPendingDeepLink();
106+
107+
setPendingDeepLink('myapp://x');
108+
expect(fn).not.toHaveBeenCalled();
109+
});
110+
});
57111
});

0 commit comments

Comments
 (0)