Skip to content

fix(react): Prevent navigation span leaks for consecutive navigations #18098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
+536 −122
Open

fix(react): Prevent navigation span leaks for consecutive navigations #18098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a849910
fix(react): Prevent navigation span leaks for consecutive navigations
onurtemizkan Nov 5, 2025
7ae1711
Reduce complexity in getNormalizedName function
onurtemizkan Nov 10, 2025
3c859bc
Fix tests
onurtemizkan Nov 10, 2025
d9e3c53
Use 100ms window for skipping cross-usage duplicates
onurtemizkan Nov 10, 2025
ade1b00
Fix lockfile
onurtemizkan Nov 11, 2025
121bb5e
Move idle check to `shouldHandleNavigation`
onurtemizkan Nov 11, 2025
11d1a6a
Address review comments from bots
onurtemizkan Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions ...ackages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,3 +498,87 @@ test('Updates navigation transaction name correctly when span is cancelled early
expect(['externalFinish', 'cancelled']).toContain(idleSpanFinishReason);
}
});

test('Creates separate transactions for rapid consecutive navigations', async ({ page }) => {
await page.goto('/');

// First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId
const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
);
});

const navigationToInner = page.locator('id=navigation');
await expect(navigationToInner).toBeVisible();
await navigationToInner.click();

const firstEvent = await firstTransactionPromise;

// Verify first transaction
expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
expect(firstEvent.contexts?.trace?.op).toBe('navigation');
const firstTraceId = firstEvent.contexts?.trace?.trace_id;
const firstSpanId = firstEvent.contexts?.trace?.span_id;

// Second navigation: /lazy/inner -> /another-lazy/sub/:id/:subId
const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction === '/another-lazy/sub/:id/:subId'
);
});

const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
await expect(navigationToAnother).toBeVisible();
await navigationToAnother.click();

const secondEvent = await secondTransactionPromise;

// Verify second transaction
expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId');
expect(secondEvent.contexts?.trace?.op).toBe('navigation');
const secondTraceId = secondEvent.contexts?.trace?.trace_id;
const secondSpanId = secondEvent.contexts?.trace?.span_id;

// Third navigation: /another-lazy -> /lazy/inner/:id/:anotherId/:someAnotherId (back to same route as first)
const thirdTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' &&
// Ensure we're not matching the first transaction again
transactionEvent.contexts?.trace?.trace_id !== firstTraceId
);
});

const navigationBackToInner = page.locator('id=navigate-to-inner-from-deep');
await expect(navigationBackToInner).toBeVisible();
await navigationBackToInner.click();

const thirdEvent = await thirdTransactionPromise;

// Verify third transaction
expect(thirdEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
expect(thirdEvent.contexts?.trace?.op).toBe('navigation');
const thirdTraceId = thirdEvent.contexts?.trace?.trace_id;
const thirdSpanId = thirdEvent.contexts?.trace?.span_id;

// Verify each navigation created a separate transaction with unique trace and span IDs
expect(firstTraceId).toBeDefined();
expect(secondTraceId).toBeDefined();
expect(thirdTraceId).toBeDefined();

// All trace IDs should be unique
expect(firstTraceId).not.toBe(secondTraceId);
expect(secondTraceId).not.toBe(thirdTraceId);
expect(firstTraceId).not.toBe(thirdTraceId);

// All span IDs should be unique
expect(firstSpanId).not.toBe(secondSpanId);
expect(secondSpanId).not.toBe(thirdSpanId);
expect(firstSpanId).not.toBe(thirdSpanId);
});
178 changes: 119 additions & 59 deletions packages/react/src/reactrouter-compat-utils/instrumentation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,11 @@ let _enableAsyncRouteHandlers: boolean = false;
const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet<Client>();

/**
* Adds resolved routes as children to the parent route.
* Prevents duplicate routes by checking if they already exist.
* Tracks last navigation per client to prevent duplicate spans in cross-usage scenarios.
* Entry persists until next different navigation, handling delayed wrapper execution.
*/
const LAST_NAVIGATION_PER_CLIENT = new WeakMap<Client, string>();

export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void {
const existingChildren = parentRoute.children || [];

Expand Down Expand Up @@ -275,27 +277,22 @@ export function createV6CompatibleWrapCreateBrowserRouter<
// If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
}

// Only handle navigation when it's complete (state is idle).
// During 'loading' or 'submitting', state.location may still have the old pathname,
// which would cause us to create a span for the wrong route.
const shouldHandleNavigation =
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);
(state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) &&
state.navigation.state === 'idle';

if (shouldHandleNavigation) {
const navigationHandler = (): void => {
handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
};

// Wait for the next render if loading an unsettled route
if (state.navigation.state !== 'idle') {
requestAnimationFrame(navigationHandler);
} else {
navigationHandler();
}
handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
}
});

Expand Down Expand Up @@ -404,29 +401,22 @@ export function createV6CompatibleWrapCreateMemoryRouter<
// If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
}

const location = state.location;

// Only handle navigation when it's complete (state is idle).
// During 'loading' or 'submitting', state.location may still have the old pathname,
// which would cause us to create a span for the wrong route.
const shouldHandleNavigation =
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);
(state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) &&
state.navigation.state === 'idle';

if (shouldHandleNavigation) {
const navigationHandler = (): void => {
handleNavigation({
location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
};

// Wait for the next render if loading an unsettled route
if (state.navigation.state !== 'idle') {
requestAnimationFrame(navigationHandler);
} else {
navigationHandler();
}
handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
}
});

Expand Down Expand Up @@ -622,6 +612,69 @@ function wrapPatchRoutesOnNavigation(
};
}

function getNavigationKey(location: Location): string {
return `${location.pathname}${location.search}${location.hash}`;
}

function tryUpdateSpanName(
activeSpan: Span,
currentSpanName: string | undefined,
newName: string,
newSource: string,
): void {
const isNewNameBetter = newName !== currentSpanName && newName.includes(':');
if (isNewNameBetter) {
activeSpan.updateName(newName);
activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, newSource as 'route' | 'url' | 'custom');
}
}

function isDuplicateNavigation(client: Client, navigationKey: string): boolean {
const lastKey = LAST_NAVIGATION_PER_CLIENT.get(client);
return lastKey === navigationKey;
}

function createNavigationSpan(opts: {
client: Client;
name: string;
source: string;
version: string;
location: Location;
routes: RouteObject[];
basename?: string;
allRoutes?: RouteObject[];
navigationKey: string;
}): Span | undefined {
const { client, name, source, version, location, routes, basename, allRoutes, navigationKey } = opts;

const navigationSpan = startBrowserTracingNavigationSpan(client, {
name,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source as 'route' | 'url' | 'custom',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
},
});

if (navigationSpan) {
LAST_NAVIGATION_PER_CLIENT.set(client, navigationKey);
patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes);

const unsubscribe = client.on('spanEnd', endedSpan => {
if (endedSpan === navigationSpan) {
// Clear key only if it's still our key (handles overlapping navigations)
const lastKey = LAST_NAVIGATION_PER_CLIENT.get(client);
if (lastKey === navigationKey) {
LAST_NAVIGATION_PER_CLIENT.delete(client);
}
unsubscribe(); // Prevent memory leak
}
});
}

return navigationSpan;
}

export function handleNavigation(opts: {
location: Location;
routes: RouteObject[];
Expand All @@ -632,15 +685,13 @@ export function handleNavigation(opts: {
allRoutes?: RouteObject[];
}): void {
const { location, routes, navigationType, version, matches, basename, allRoutes } = opts;
const branches = Array.isArray(matches) ? matches : _matchRoutes(routes, location, basename);
const branches = Array.isArray(matches) ? matches : _matchRoutes(allRoutes || routes, location, basename);

const client = getClient();
if (!client || !CLIENTS_WITH_INSTRUMENT_NAVIGATION.has(client)) {
return;
}

// Avoid starting a navigation span on initial load when a pageload root span is active.
// This commonly happens when lazy routes resolve during the first render and React Router emits a POP.
const activeRootSpan = getActiveRootSpan();
if (activeRootSpan && spanToJSON(activeRootSpan).op === 'pageload' && navigationType === 'POP') {
return;
Expand All @@ -649,7 +700,7 @@ export function handleNavigation(opts: {
if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) {
const [name, source] = resolveRouteNameAndSource(
location,
routes,
allRoutes || routes,
allRoutes || routes,
branches as RouteMatch[],
basename,
Expand All @@ -658,22 +709,25 @@ export function handleNavigation(opts: {
const activeSpan = getActiveSpan();
const spanJson = activeSpan && spanToJSON(activeSpan);
const isAlreadyInNavigationSpan = spanJson?.op === 'navigation';
const isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name;

// Cross usage can result in multiple navigation spans being created without this check
if (!isAlreadyInNavigationSpan) {
const navigationSpan = startBrowserTracingNavigationSpan(client, {
const currentNavigationKey = getNavigationKey(location);
const isNavDuplicate = isDuplicateNavigation(client, currentNavigationKey);

if (!isSpanForSameRoute && !isNavDuplicate) {
createNavigationSpan({
client,
name,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
},
source,
version,
location,
routes,
basename,
allRoutes,
navigationKey: currentNavigationKey,
});

// Patch navigation span to handle early cancellation (e.g., document.hidden)
if (navigationSpan) {
patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes);
}
} else if (isNavDuplicate && isAlreadyInNavigationSpan && activeSpan) {
tryUpdateSpanName(activeSpan, spanJson?.description, name, source);
}
Comment on lines +721 to 731
Copy link

@sentry sentry bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Stale LAST_NAVIGATION_PER_CLIENT entry can cause subsequent navigations to be silently ignored if span creation fails.
Severity: HIGH | Confidence: 0.90

🔍 Detailed Analysis

If startBrowserTracingNavigationSpan() returns undefined (e.g., if no idle span is created/set), the LAST_NAVIGATION_PER_CLIENT entry is still set, but the cleanup listener is not registered. This leaves a stale timestamp entry in LAST_NAVIGATION_PER_CLIENT. Consequently, a subsequent navigation to the same location within 100ms will be incorrectly identified as a duplicate by isDuplicateNavigation() and silently ignored, leading to missing navigation spans.

💡 Suggested Fix

Ensure LAST_NAVIGATION_PER_CLIENT entries are always cleaned up, even if startBrowserTracingNavigationSpan() returns undefined, or modify isDuplicateNavigation() to account for cases where no span was successfully created for the initial navigation.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/react/src/reactrouter-compat-utils/instrumentation.tsx#L717-L731

Potential issue: If `startBrowserTracingNavigationSpan()` returns `undefined` (e.g., if
no idle span is created/set), the `LAST_NAVIGATION_PER_CLIENT` entry is still set, but
the cleanup listener is not registered. This leaves a stale timestamp entry in
`LAST_NAVIGATION_PER_CLIENT`. Consequently, a subsequent navigation to the same location
within 100ms will be incorrectly identified as a duplicate by `isDuplicateNavigation()`
and silently ignored, leading to missing navigation spans.

Did we get this right? 👍 / 👎 to inform future reviews.

}
}
Expand Down Expand Up @@ -727,7 +781,13 @@ function updatePageloadTransaction({
: (_matchRoutes(allRoutes || routes, location, basename) as unknown as RouteMatch[]);

if (branches) {
const [name, source] = resolveRouteNameAndSource(location, routes, allRoutes || routes, branches, basename);
const [name, source] = resolveRouteNameAndSource(
location,
allRoutes || routes,
allRoutes || routes,
branches,
basename,
);

getCurrentScope().setTransactionName(name || '/');

Expand Down Expand Up @@ -780,7 +840,7 @@ function patchSpanEnd(
if (branches) {
const [name, source] = resolveRouteNameAndSource(
location,
routes,
currentAllRoutes.length > 0 ? currentAllRoutes : routes,
currentAllRoutes.length > 0 ? currentAllRoutes : routes,
branches,
basename,
Expand Down
Loading