From 046216f9e8fb644171d4e4bd6ebff1305c5f1b06 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 5 Nov 2025 12:10:15 +0000 Subject: [PATCH 01/10] fix(react): Prevent navigation span leaks for consecutive navigations --- .../tests/transactions.test.ts | 3 + .../instrumentation.tsx | 34 ++- .../test/reactrouter-cross-usage.test.tsx | 274 +++++++++++++++++- 3 files changed, 296 insertions(+), 15 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts index a57199c52633..74e40ffc9191 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts @@ -523,6 +523,7 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId'); expect(firstEvent.contexts?.trace?.op).toBe('navigation'); expect(firstEvent.contexts?.trace?.status).toBe('ok'); + const firstTraceId = firstEvent.contexts?.trace?.trace_id; const firstSpanId = firstEvent.contexts?.trace?.span_id; @@ -545,6 +546,7 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId'); expect(secondEvent.contexts?.trace?.op).toBe('navigation'); expect(secondEvent.contexts?.trace?.status).toBe('ok'); + const secondTraceId = secondEvent.contexts?.trace?.trace_id; const secondSpanId = secondEvent.contexts?.trace?.span_id; @@ -569,6 +571,7 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ expect(thirdEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId'); expect(thirdEvent.contexts?.trace?.op).toBe('navigation'); expect(thirdEvent.contexts?.trace?.status).toBe('ok'); + const thirdTraceId = thirdEvent.contexts?.trace?.trace_id; const thirdSpanId = thirdEvent.contexts?.trace?.span_id; diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index a6e55f1a967c..2df54c7f1230 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -279,7 +279,10 @@ export function createV6CompatibleWrapCreateBrowserRouter< state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete); if (shouldHandleNavigation) { - const navigationHandler = (): void => { + // 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. + if (state.navigation.state === 'idle') { handleNavigation({ location: state.location, routes, @@ -288,13 +291,6 @@ export function createV6CompatibleWrapCreateBrowserRouter< basename, allRoutes: Array.from(allRoutes), }); - }; - - // Wait for the next render if loading an unsettled route - if (state.navigation.state !== 'idle') { - requestAnimationFrame(navigationHandler); - } else { - navigationHandler(); } } }); @@ -632,7 +628,8 @@ 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); + // Use allRoutes for matching to include lazy-loaded routes + const branches = Array.isArray(matches) ? matches : _matchRoutes(allRoutes || routes, location, basename); const client = getClient(); if (!client || !CLIENTS_WITH_INSTRUMENT_NAVIGATION.has(client)) { @@ -649,7 +646,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, @@ -659,8 +656,11 @@ export function handleNavigation(opts: { const spanJson = activeSpan && spanToJSON(activeSpan); const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; - // Cross usage can result in multiple navigation spans being created without this check - if (!isAlreadyInNavigationSpan) { + // Only skip creating a new span if we're already in a navigation span AND the route name matches. + // This handles cross-usage (multiple wrappers for same navigation) while allowing consecutive navigations. + const isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name; + + if (!isSpanForSameRoute) { const navigationSpan = startBrowserTracingNavigationSpan(client, { name, attributes: { @@ -727,7 +727,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 || '/'); @@ -780,7 +786,7 @@ function patchSpanEnd( if (branches) { const [name, source] = resolveRouteNameAndSource( location, - routes, + currentAllRoutes.length > 0 ? currentAllRoutes : routes, currentAllRoutes.length > 0 ? currentAllRoutes : routes, branches, basename, diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index 77d8e3d95b2e..a99760597dc0 100644 --- a/packages/react/test/reactrouter-cross-usage.test.tsx +++ b/packages/react/test/reactrouter-cross-usage.test.tsx @@ -9,7 +9,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, setCurrentClient, } from '@sentry/core'; -import { render } from '@testing-library/react'; +import { act, render, waitFor } from '@testing-library/react'; import * as React from 'react'; import { createMemoryRouter, @@ -607,4 +607,276 @@ describe('React Router cross usage of wrappers', () => { }); }); }); + + describe('consecutive navigations to different routes', () => { + it('should create separate transactions for consecutive navigations to different routes', async () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter); + + const router = createSentryMemoryRouter( + [ + { + children: [ + { path: '/users', element:
Users
}, + { path: '/settings', element:
Settings
}, + { path: '/profile', element:
Profile
}, + ], + }, + ], + { initialEntries: ['/users'] }, + ); + + render( + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).not.toHaveBeenCalled(); + + await act(async () => { + router.navigate('/settings'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/settings', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + + await act(async () => { + router.navigate('/profile'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); + + const calls = mockStartBrowserTracingNavigationSpan.mock.calls; + expect(calls[0]![1].name).toBe('/settings'); + expect(calls[1]![1].name).toBe('/profile'); + expect(calls[0]![1].attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('navigation'); + expect(calls[1]![1].attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('navigation'); + }); + + it('should create separate transactions for rapid consecutive navigations', async () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter); + + const router = createSentryMemoryRouter( + [ + { + children: [ + { path: '/a', element:
A
}, + { path: '/b', element:
B
}, + { path: '/c', element:
C
}, + ], + }, + ], + { initialEntries: ['/a'] }, + ); + + render( + + + , + ); + + await act(async () => { + router.navigate('/b'); + router.navigate('/c'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); + + const calls = mockStartBrowserTracingNavigationSpan.mock.calls; + expect(calls[0]![1].name).toBe('/b'); + expect(calls[1]![1].name).toBe('/c'); + }); + + it('should NOT create duplicate spans for same route name (even with different params)', async () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter); + + const router = createSentryMemoryRouter( + [ + { + children: [{ path: '/user/:id', element:
User
}], + }, + ], + { initialEntries: ['/user/1'] }, + ); + + render( + + + , + ); + + await act(async () => { + router.navigate('/user/2'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledWith(expect.any(BrowserClient), { + name: '/user/:id', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + + await act(async () => { + router.navigate('/user/3'); + await new Promise(resolve => setTimeout(resolve, 100)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + }); + + it('should handle mixed cross-usage and consecutive navigations correctly', async () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter); + const sentryUseRoutes = wrapUseRoutesV6(useRoutes); + + const UsersRoute: React.FC = () => sentryUseRoutes([{ path: '/', element:
Users
}]); + + const SettingsRoute: React.FC = () => sentryUseRoutes([{ path: '/', element:
Settings
}]); + + const router = createSentryMemoryRouter( + [ + { + children: [ + { path: '/users/*', element: }, + { path: '/settings/*', element: }, + ], + }, + ], + { initialEntries: ['/users'] }, + ); + + render( + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).not.toHaveBeenCalled(); + + await act(async () => { + router.navigate('/settings'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1)); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/settings/*', + attributes: expect.objectContaining({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + }), + }); + }); + + it('should not create duplicate spans for cross-usage on same route', async () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter); + const sentryUseRoutes = wrapUseRoutesV6(useRoutes); + + const NestedRoute: React.FC = () => sentryUseRoutes([{ path: '/', element:
Details
}]); + + const router = createSentryMemoryRouter( + [ + { + children: [{ path: '/details/*', element: }], + }, + ], + { initialEntries: ['/home'] }, + ); + + render( + + + , + ); + + await act(async () => { + router.navigate('/details'); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalled()); + }); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledWith(expect.any(BrowserClient), { + name: '/details/*', + attributes: expect.objectContaining({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + }), + }); + }); + }); }); From 14ce2fb703607c7e9d5f6ab356190d9fd43f9c37 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 13:01:05 +0000 Subject: [PATCH 02/10] Reduce complexity in getNormalizedName function --- .../src/reactrouter-compat-utils/utils.ts | 93 ++++++++++--------- 1 file changed, 51 insertions(+), 42 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index d6501d0e4dbf..4cec7bd98dcd 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -171,6 +171,13 @@ export function locationIsInsideDescendantRoute(location: Location, routes: Rout return false; } +/** + * Returns a fallback transaction name from location pathname. + */ +function getFallbackTransactionName(location: Location, basename: string): string { + return _stripBasename ? stripBasenameFromPathname(location.pathname, basename) : location.pathname || ''; +} + /** * Gets a normalized route name and transaction source from the current routes and location. */ @@ -184,53 +191,55 @@ export function getNormalizedName( return [_stripBasename ? stripBasenameFromPathname(location.pathname, basename) : location.pathname, 'url']; } + if (!branches) { + return [getFallbackTransactionName(location, basename), 'url']; + } + let pathBuilder = ''; - if (branches) { - for (const branch of branches) { - const route = branch.route; - if (route) { - // Early return if index route - if (route.index) { - return sendIndexPath(pathBuilder, branch.pathname, basename); - } - const path = route.path; - - // If path is not a wildcard and has no child routes, append the path - if (path && !pathIsWildcardAndHasChildren(path, branch)) { - const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; - pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); - - // If the path matches the current location, return the path - if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { - if ( - // If the route defined on the element is something like - // Product} /> - // We should check against the branch.pathname for the number of / separators - getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) && - // We should not count wildcard operators in the url segments calculation - !pathEndsWithWildcard(pathBuilder) - ) { - return [(_stripBasename ? '' : basename) + newPath, 'route']; - } - - // if the last character of the pathbuilder is a wildcard and there are children, remove the wildcard - if (pathIsWildcardAndHasChildren(pathBuilder, branch)) { - pathBuilder = pathBuilder.slice(0, -1); - } - - return [(_stripBasename ? '' : basename) + pathBuilder, 'route']; - } - } - } + for (const branch of branches) { + const route = branch.route; + if (!route) { + continue; + } + + // Early return for index routes + if (route.index) { + return sendIndexPath(pathBuilder, branch.pathname, basename); } - } - const fallbackTransactionName = _stripBasename - ? stripBasenameFromPathname(location.pathname, basename) - : location.pathname || ''; + const path = route.path; + if (!path || pathIsWildcardAndHasChildren(path, branch)) { + continue; + } + + // Build the route path + const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; + pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); + + // Check if this path matches the current location + if (trimSlash(location.pathname) !== trimSlash(basename + branch.pathname)) { + continue; + } + + // Check if this is a parameterized route like /stores/:storeId/products/:productId + if ( + getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) && + !pathEndsWithWildcard(pathBuilder) + ) { + return [(_stripBasename ? '' : basename) + newPath, 'route']; + } + + // Handle wildcard routes with children - strip trailing wildcard + if (pathIsWildcardAndHasChildren(pathBuilder, branch)) { + pathBuilder = pathBuilder.slice(0, -1); + } + + return [(_stripBasename ? '' : basename) + pathBuilder, 'route']; + } - return [fallbackTransactionName, 'url']; + // Fallback when no matching route found + return [getFallbackTransactionName(location, basename), 'url']; } /** From 5144fa30c28e84a389aeb7e3e3ab907a69346c33 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 13:18:31 +0000 Subject: [PATCH 03/10] Fix tests --- packages/react/test/reactrouter-cross-usage.test.tsx | 11 +++++------ .../react/test/reactrouter-descendant-routes.test.tsx | 2 ++ packages/react/test/reactrouterv6.test.tsx | 2 ++ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index a99760597dc0..bdc5fc02c889 100644 --- a/packages/react/test/reactrouter-cross-usage.test.tsx +++ b/packages/react/test/reactrouter-cross-usage.test.tsx @@ -26,6 +26,7 @@ import { } from 'react-router-6'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { BrowserClient } from '../src'; +import { allRoutes } from '../src/reactrouter-compat-utils/instrumentation'; import { reactRouterV6BrowserTracingIntegration, withSentryReactRouterV6Routing, @@ -101,6 +102,7 @@ describe('React Router cross usage of wrappers', () => { beforeEach(() => { vi.clearAllMocks(); getCurrentScope().setClient(undefined); + allRoutes.clear(); }); describe('wrapCreateBrowserRouter and wrapUseRoutes', () => { @@ -218,8 +220,7 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); - // It's called 1 time from the wrapped `MemoryRouter` - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { name: '/second-level/:id/third-level/:id', attributes: { @@ -339,7 +340,6 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); - // It's called 1 time from the wrapped `MemoryRouter` expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); }); }); @@ -465,8 +465,7 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); - // It's called 1 time from the wrapped `createMemoryRouter` - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { name: '/second-level/:id/third-level/:id', @@ -596,7 +595,7 @@ describe('React Router cross usage of wrappers', () => { ); expect(container.innerHTML).toContain('Details'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { name: '/second-level/:id/third-level/:id', attributes: { diff --git a/packages/react/test/reactrouter-descendant-routes.test.tsx b/packages/react/test/reactrouter-descendant-routes.test.tsx index fe75bc81e858..a08893694a30 100644 --- a/packages/react/test/reactrouter-descendant-routes.test.tsx +++ b/packages/react/test/reactrouter-descendant-routes.test.tsx @@ -25,6 +25,7 @@ import { } from 'react-router-6'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { BrowserClient } from '../src'; +import { allRoutes } from '../src/reactrouter-compat-utils/instrumentation'; import { reactRouterV6BrowserTracingIntegration, withSentryReactRouterV6Routing, @@ -79,6 +80,7 @@ describe('React Router Descendant Routes', () => { beforeEach(() => { vi.clearAllMocks(); getCurrentScope().setClient(undefined); + allRoutes.clear(); }); describe('withSentryReactRouterV6Routing', () => { diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 61fefdff9b63..fda5043d2e6a 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -28,6 +28,7 @@ import { } from 'react-router-6'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { BrowserClient } from '../src'; +import { allRoutes } from '../src/reactrouter-compat-utils/instrumentation'; import { reactRouterV6BrowserTracingIntegration, withSentryReactRouterV6Routing, @@ -83,6 +84,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { beforeEach(() => { vi.clearAllMocks(); getCurrentScope().setClient(undefined); + allRoutes.clear(); }); it('wrapCreateMemoryRouterV6 starts and updates a pageload transaction - single initialEntry', () => { From 7572a984574a73f958f90ce278188d5811310b0a Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 21:56:29 +0000 Subject: [PATCH 04/10] Use 100ms window for skipping cross-usage duplicates --- .../instrumentation.tsx | 102 ++++++++++++++---- .../test/reactrouter-cross-usage.test.tsx | 40 +++---- yarn.lock | 46 ++++++++ 3 files changed, 141 insertions(+), 47 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 2df54c7f1230..469d10692190 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -53,9 +53,11 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); /** - * 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. + * Uses 100ms window to deduplicate when multiple wrappers handle the same navigation. */ +const LAST_NAVIGATION_PER_CLIENT = new WeakMap(); + export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { const existingChildren = parentRoute.children || []; @@ -618,6 +620,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 lastNavigation = LAST_NAVIGATION_PER_CLIENT.get(client); + const now = Date.now(); + return !!(lastNavigation && lastNavigation.key === navigationKey && now - lastNavigation.timestamp < 100); +} + +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; + + LAST_NAVIGATION_PER_CLIENT.set(client, { + key: navigationKey, + timestamp: Date.now(), + }); + + 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) { + patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); + + client.on('spanEnd', endedSpan => { + if (endedSpan === navigationSpan) { + LAST_NAVIGATION_PER_CLIENT.delete(client); + } + }); + } + + return navigationSpan; +} + export function handleNavigation(opts: { location: Location; routes: RouteObject[]; @@ -628,7 +693,6 @@ export function handleNavigation(opts: { allRoutes?: RouteObject[]; }): void { const { location, routes, navigationType, version, matches, basename, allRoutes } = opts; - // Use allRoutes for matching to include lazy-loaded routes const branches = Array.isArray(matches) ? matches : _matchRoutes(allRoutes || routes, location, basename); const client = getClient(); @@ -636,8 +700,6 @@ export function handleNavigation(opts: { 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; @@ -655,25 +717,25 @@ export function handleNavigation(opts: { const activeSpan = getActiveSpan(); const spanJson = activeSpan && spanToJSON(activeSpan); const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; - - // Only skip creating a new span if we're already in a navigation span AND the route name matches. - // This handles cross-usage (multiple wrappers for same navigation) while allowing consecutive navigations. const isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name; - if (!isSpanForSameRoute) { - 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); } } } diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index bdc5fc02c889..c84a1b16af6a 100644 --- a/packages/react/test/reactrouter-cross-usage.test.tsx +++ b/packages/react/test/reactrouter-cross-usage.test.tsx @@ -220,15 +220,10 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/second-level/:id/third-level/:id', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + // In cross-usage scenarios, the first wrapper creates the span and the second updates it + expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/second-level/:id/third-level/:id'); + expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); }); @@ -465,16 +460,12 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/second-level/:id/third-level/:id', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); + // Cross-usage deduplication: Span created once with initial route name + // With nested lazy routes, initial name may be raw path, updated to parameterized by later wrapper + expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/second-level/:id/third-level/:id'); + expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); }); @@ -595,15 +586,10 @@ describe('React Router cross usage of wrappers', () => { ); expect(container.innerHTML).toContain('Details'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/second-level/:id/third-level/:id', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + // Cross-usage with all three wrappers: span created once, then updated + expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/second-level/:id/third-level/:id'); + expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); }); diff --git a/yarn.lock b/yarn.lock index 99e434f5efff..934e3f2dc2af 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6943,6 +6943,20 @@ "@angular-devkit/schematics" "14.2.13" jsonc-parser "3.1.0" +"@sentry-internal/browser-utils@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry-internal/browser-utils/-/browser-utils-10.23.0.tgz#738a07ed99168cdf69d0cdb5a152289ed049de81" + integrity sha512-FUak8FH51TnGrx2i31tgqun0VsbDCVQS7dxWnUZHdi+0hpnFoq9+wBHY+qrOQjaInZSz3crIifYv3z7SEzD0Jg== + dependencies: + "@sentry/core" "10.23.0" + +"@sentry-internal/feedback@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry-internal/feedback/-/feedback-10.23.0.tgz#4b9ade29f1d96309eea83cc513c4a73e3992c4d7" + integrity sha512-+HWC9VTPICsFX/lIPoBU9GxTaJZVXJcukP+qGxj+j/8q/Dy1w22JHDWcJbZiaW4kWWlz7VbA0KVKS3grD+e9aA== + dependencies: + "@sentry/core" "10.23.0" + "@sentry-internal/node-cpu-profiler@^2.2.0": version "2.2.0" resolved "https://registry.yarnpkg.com/@sentry-internal/node-cpu-profiler/-/node-cpu-profiler-2.2.0.tgz#0640d4aebb4d36031658ccff83dc22b76f437ede" @@ -6959,6 +6973,22 @@ detect-libc "^2.0.4" node-abi "^3.73.0" +"@sentry-internal/replay-canvas@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry-internal/replay-canvas/-/replay-canvas-10.23.0.tgz#236916fb9d40637d8c9f86c52b2b1619b1170854" + integrity sha512-GLNY8JPcMI6xhQ5FHiYO/W/3flrwZMt4CI/E3jDRNujYWbCrca60MRke6k7Zm1qi9rZ1FuhVWZ6BAFc4vwXnSg== + dependencies: + "@sentry-internal/replay" "10.23.0" + "@sentry/core" "10.23.0" + +"@sentry-internal/replay@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry-internal/replay/-/replay-10.23.0.tgz#7a6075e2c2e1d0a371764d7c2e5dad578bb7b1fe" + integrity sha512-5yPD7jVO2JY8+JEHXep0Bf/ugp4rmxv5BkHIcSAHQsKSPhziFks2x+KP+6M8hhbF1WydqAaDYlGjrkL2yspHqA== + dependencies: + "@sentry-internal/browser-utils" "10.23.0" + "@sentry/core" "10.23.0" + "@sentry-internal/rrdom@2.34.0": version "2.34.0" resolved "https://registry.yarnpkg.com/@sentry-internal/rrdom/-/rrdom-2.34.0.tgz#fccc9fe211c3995d4200abafbe8d75b671961ee9" @@ -7032,6 +7062,17 @@ resolved "https://registry.yarnpkg.com/@sentry/babel-plugin-component-annotate/-/babel-plugin-component-annotate-4.3.0.tgz#c5b6cbb986952596d3ad233540a90a1fd18bad80" integrity sha512-OuxqBprXRyhe8Pkfyz/4yHQJc5c3lm+TmYWSSx8u48g5yKewSQDOxkiLU5pAk3WnbLPy8XwU/PN+2BG0YFU9Nw== +"@sentry/browser@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-10.23.0.tgz#aa85f9c21c9a6c80b8952ee15307997fb34edbb3" + integrity sha512-9hViLfYONxRJykOhJQ3ZHQ758t1wQIsxEC7mTsydbDm+m12LgbBtXbfgcypWHlom5Yvb+wg6W+31bpdGnATglw== + dependencies: + "@sentry-internal/browser-utils" "10.23.0" + "@sentry-internal/feedback" "10.23.0" + "@sentry-internal/replay" "10.23.0" + "@sentry-internal/replay-canvas" "10.23.0" + "@sentry/core" "10.23.0" + "@sentry/bundler-plugin-core@4.3.0", "@sentry/bundler-plugin-core@^4.3.0": version "4.3.0" resolved "https://registry.yarnpkg.com/@sentry/bundler-plugin-core/-/bundler-plugin-core-4.3.0.tgz#cf302522a3e5b8a3bf727635d0c6a7bece981460" @@ -7106,6 +7147,11 @@ "@sentry/cli-win32-i686" "2.56.0" "@sentry/cli-win32-x64" "2.56.0" +"@sentry/core@10.23.0": + version "10.23.0" + resolved "https://registry.yarnpkg.com/@sentry/core/-/core-10.23.0.tgz#7d4eb4d2c7b9ecc88872975a916f44e0b9fec78a" + integrity sha512-4aZwu6VnSHWDplY5eFORcVymhfvS/P6BRfK81TPnG/ReELaeoykKjDwR+wC4lO7S0307Vib9JGpszjsEZw245g== + "@sentry/rollup-plugin@^4.3.0": version "4.3.0" resolved "https://registry.yarnpkg.com/@sentry/rollup-plugin/-/rollup-plugin-4.3.0.tgz#d23fe49e48fa68dafa2b0933a8efabcc964b1df9" From 979d6e3fdec329dc6eeb7a0b1871da9513ce3e0e Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 11 Nov 2025 10:38:34 +0000 Subject: [PATCH 05/10] Fix lockfile --- yarn.lock | 46 ---------------------------------------------- 1 file changed, 46 deletions(-) diff --git a/yarn.lock b/yarn.lock index 934e3f2dc2af..99e434f5efff 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6943,20 +6943,6 @@ "@angular-devkit/schematics" "14.2.13" jsonc-parser "3.1.0" -"@sentry-internal/browser-utils@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry-internal/browser-utils/-/browser-utils-10.23.0.tgz#738a07ed99168cdf69d0cdb5a152289ed049de81" - integrity sha512-FUak8FH51TnGrx2i31tgqun0VsbDCVQS7dxWnUZHdi+0hpnFoq9+wBHY+qrOQjaInZSz3crIifYv3z7SEzD0Jg== - dependencies: - "@sentry/core" "10.23.0" - -"@sentry-internal/feedback@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry-internal/feedback/-/feedback-10.23.0.tgz#4b9ade29f1d96309eea83cc513c4a73e3992c4d7" - integrity sha512-+HWC9VTPICsFX/lIPoBU9GxTaJZVXJcukP+qGxj+j/8q/Dy1w22JHDWcJbZiaW4kWWlz7VbA0KVKS3grD+e9aA== - dependencies: - "@sentry/core" "10.23.0" - "@sentry-internal/node-cpu-profiler@^2.2.0": version "2.2.0" resolved "https://registry.yarnpkg.com/@sentry-internal/node-cpu-profiler/-/node-cpu-profiler-2.2.0.tgz#0640d4aebb4d36031658ccff83dc22b76f437ede" @@ -6973,22 +6959,6 @@ detect-libc "^2.0.4" node-abi "^3.73.0" -"@sentry-internal/replay-canvas@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry-internal/replay-canvas/-/replay-canvas-10.23.0.tgz#236916fb9d40637d8c9f86c52b2b1619b1170854" - integrity sha512-GLNY8JPcMI6xhQ5FHiYO/W/3flrwZMt4CI/E3jDRNujYWbCrca60MRke6k7Zm1qi9rZ1FuhVWZ6BAFc4vwXnSg== - dependencies: - "@sentry-internal/replay" "10.23.0" - "@sentry/core" "10.23.0" - -"@sentry-internal/replay@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry-internal/replay/-/replay-10.23.0.tgz#7a6075e2c2e1d0a371764d7c2e5dad578bb7b1fe" - integrity sha512-5yPD7jVO2JY8+JEHXep0Bf/ugp4rmxv5BkHIcSAHQsKSPhziFks2x+KP+6M8hhbF1WydqAaDYlGjrkL2yspHqA== - dependencies: - "@sentry-internal/browser-utils" "10.23.0" - "@sentry/core" "10.23.0" - "@sentry-internal/rrdom@2.34.0": version "2.34.0" resolved "https://registry.yarnpkg.com/@sentry-internal/rrdom/-/rrdom-2.34.0.tgz#fccc9fe211c3995d4200abafbe8d75b671961ee9" @@ -7062,17 +7032,6 @@ resolved "https://registry.yarnpkg.com/@sentry/babel-plugin-component-annotate/-/babel-plugin-component-annotate-4.3.0.tgz#c5b6cbb986952596d3ad233540a90a1fd18bad80" integrity sha512-OuxqBprXRyhe8Pkfyz/4yHQJc5c3lm+TmYWSSx8u48g5yKewSQDOxkiLU5pAk3WnbLPy8XwU/PN+2BG0YFU9Nw== -"@sentry/browser@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-10.23.0.tgz#aa85f9c21c9a6c80b8952ee15307997fb34edbb3" - integrity sha512-9hViLfYONxRJykOhJQ3ZHQ758t1wQIsxEC7mTsydbDm+m12LgbBtXbfgcypWHlom5Yvb+wg6W+31bpdGnATglw== - dependencies: - "@sentry-internal/browser-utils" "10.23.0" - "@sentry-internal/feedback" "10.23.0" - "@sentry-internal/replay" "10.23.0" - "@sentry-internal/replay-canvas" "10.23.0" - "@sentry/core" "10.23.0" - "@sentry/bundler-plugin-core@4.3.0", "@sentry/bundler-plugin-core@^4.3.0": version "4.3.0" resolved "https://registry.yarnpkg.com/@sentry/bundler-plugin-core/-/bundler-plugin-core-4.3.0.tgz#cf302522a3e5b8a3bf727635d0c6a7bece981460" @@ -7147,11 +7106,6 @@ "@sentry/cli-win32-i686" "2.56.0" "@sentry/cli-win32-x64" "2.56.0" -"@sentry/core@10.23.0": - version "10.23.0" - resolved "https://registry.yarnpkg.com/@sentry/core/-/core-10.23.0.tgz#7d4eb4d2c7b9ecc88872975a916f44e0b9fec78a" - integrity sha512-4aZwu6VnSHWDplY5eFORcVymhfvS/P6BRfK81TPnG/ReELaeoykKjDwR+wC4lO7S0307Vib9JGpszjsEZw245g== - "@sentry/rollup-plugin@^4.3.0": version "4.3.0" resolved "https://registry.yarnpkg.com/@sentry/rollup-plugin/-/rollup-plugin-4.3.0.tgz#d23fe49e48fa68dafa2b0933a8efabcc964b1df9" From ed53a1df82a7b9f0e7af8fe8278d598884c0aaaa Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 11 Nov 2025 11:40:11 +0000 Subject: [PATCH 06/10] Move idle check to `shouldHandleNavigation` --- .../instrumentation.tsx | 60 ++++++++----------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 469d10692190..8aeecc5914b8 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -277,23 +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) { - // 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. - if (state.navigation.state === 'idle') { - handleNavigation({ - location: state.location, - routes, - navigationType: state.historyAction, - version, - basename, - allRoutes: Array.from(allRoutes), - }); - } + handleNavigation({ + location: state.location, + routes, + navigationType: state.historyAction, + version, + basename, + allRoutes: Array.from(allRoutes), + }); } }); @@ -402,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), + }); } }); From 0cfe2643057a168a53498ae432b17f4340ca8da1 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 11 Nov 2025 16:03:33 +0000 Subject: [PATCH 07/10] Address review comments from bots --- .../instrumentation.tsx | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 8aeecc5914b8..41f6de8dc014 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -54,9 +54,9 @@ const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); /** * Tracks last navigation per client to prevent duplicate spans in cross-usage scenarios. - * Uses 100ms window to deduplicate when multiple wrappers handle the same navigation. + * Entry persists until next different navigation, handling delayed wrapper execution. */ -const LAST_NAVIGATION_PER_CLIENT = new WeakMap(); +const LAST_NAVIGATION_PER_CLIENT = new WeakMap(); export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { const existingChildren = parentRoute.children || []; @@ -630,9 +630,8 @@ function tryUpdateSpanName( } function isDuplicateNavigation(client: Client, navigationKey: string): boolean { - const lastNavigation = LAST_NAVIGATION_PER_CLIENT.get(client); - const now = Date.now(); - return !!(lastNavigation && lastNavigation.key === navigationKey && now - lastNavigation.timestamp < 100); + const lastKey = LAST_NAVIGATION_PER_CLIENT.get(client); + return lastKey === navigationKey; } function createNavigationSpan(opts: { @@ -648,11 +647,6 @@ function createNavigationSpan(opts: { }): Span | undefined { const { client, name, source, version, location, routes, basename, allRoutes, navigationKey } = opts; - LAST_NAVIGATION_PER_CLIENT.set(client, { - key: navigationKey, - timestamp: Date.now(), - }); - const navigationSpan = startBrowserTracingNavigationSpan(client, { name, attributes: { @@ -663,11 +657,17 @@ function createNavigationSpan(opts: { }); if (navigationSpan) { + LAST_NAVIGATION_PER_CLIENT.set(client, navigationKey); patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); - client.on('spanEnd', endedSpan => { + const unsubscribe = client.on('spanEnd', endedSpan => { if (endedSpan === navigationSpan) { - LAST_NAVIGATION_PER_CLIENT.delete(client); + // 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 } }); } From e9cab06916fca469f76d363e341c8c333479d390 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 12 Nov 2025 09:30:01 +0000 Subject: [PATCH 08/10] Edge case fixes --- .../instrumentation.tsx | 21 ++++++++++++------- .../test/reactrouter-cross-usage.test.tsx | 15 ++++++++++--- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 41f6de8dc014..ea81fe3dcf0c 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -706,15 +706,22 @@ export function handleNavigation(opts: { basename, ); - const activeSpan = getActiveSpan(); - const spanJson = activeSpan && spanToJSON(activeSpan); - const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; - const isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name; - const currentNavigationKey = getNavigationKey(location); const isNavDuplicate = isDuplicateNavigation(client, currentNavigationKey); - if (!isSpanForSameRoute && !isNavDuplicate) { + if (isNavDuplicate) { + // Cross-usage duplicate - update existing span name if better + const activeSpan = getActiveSpan(); + const spanJson = activeSpan && spanToJSON(activeSpan); + const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; + + if (isAlreadyInNavigationSpan && activeSpan) { + tryUpdateSpanName(activeSpan, spanJson?.description, name, source); + } + } else { + // Not a cross-usage duplicate - create new span + // This handles: different routes, same route with different params (/user/2 → /user/3) + // startBrowserTracingNavigationSpan will end any active navigation span createNavigationSpan({ client, name, @@ -726,8 +733,6 @@ export function handleNavigation(opts: { allRoutes, navigationKey: currentNavigationKey, }); - } else if (isNavDuplicate && isAlreadyInNavigationSpan && activeSpan) { - tryUpdateSpanName(activeSpan, spanJson?.description, name, source); } } } diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index c84a1b16af6a..be71f4b838c5 100644 --- a/packages/react/test/reactrouter-cross-usage.test.tsx +++ b/packages/react/test/reactrouter-cross-usage.test.tsx @@ -707,7 +707,7 @@ describe('React Router cross usage of wrappers', () => { expect(calls[1]![1].name).toBe('/c'); }); - it('should NOT create duplicate spans for same route name (even with different params)', async () => { + it('should create separate spans for same route with different params', async () => { const client = createMockBrowserClient(); setCurrentClient(client); @@ -755,10 +755,19 @@ describe('React Router cross usage of wrappers', () => { await act(async () => { router.navigate('/user/3'); - await new Promise(resolve => setTimeout(resolve, 100)); + await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2)); }); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + // Should create 2 spans - different concrete paths are different user actions + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenNthCalledWith(2, expect.any(BrowserClient), { + name: '/user/:id', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); }); it('should handle mixed cross-usage and consecutive navigations correctly', async () => { From 3084371535f18fc7414ee29db2b47ad489971a1c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 13 Nov 2025 13:05:40 +0000 Subject: [PATCH 09/10] Address review comments --- .../tests/transactions.test.ts | 71 +++++++++---------- .../instrumentation.tsx | 42 ++++++----- 2 files changed, 58 insertions(+), 55 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts index 74e40ffc9191..ce356b2f1a24 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts @@ -504,7 +504,7 @@ test('Updates navigation transaction name correctly when span is cancelled early test('Creates separate transactions for rapid consecutive navigations', async ({ page }) => { await page.goto('/'); - // First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId + // Set up transaction listeners const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { return ( !!transactionEvent?.transaction && @@ -513,21 +513,6 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ ); }); - 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'); - expect(firstEvent.contexts?.trace?.status).toBe('ok'); - - 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 && @@ -536,13 +521,42 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ ); }); - const navigationToAnother = page.locator('id=navigate-to-another-from-inner'); - await expect(navigationToAnother).toBeVisible(); - await navigationToAnother.click(); + // Third navigation promise - using counter to match second occurrence of same route + let innerRouteMatchCount = 0; + const thirdTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if ( + transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' + ) { + innerRouteMatchCount++; + return innerRouteMatchCount === 2; // Match the second occurrence + } + return false; + }); + + // Perform navigations + // First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId + await page.locator('id=navigation').click(); + + const firstEvent = await firstTransactionPromise; + + // Second navigation: /lazy/inner -> /another-lazy/sub/:id/:subId + await page.locator('id=navigate-to-another-from-inner').click(); const secondEvent = await secondTransactionPromise; - // Verify second transaction + // Third navigation: /another-lazy -> /lazy/inner/:id/:anotherId/:someAnotherId (back to same route as first) + await page.locator('id=navigate-to-inner-from-deep').click(); + + const thirdEvent = await thirdTransactionPromise; + + // Verify transactions + 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; + expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId'); expect(secondEvent.contexts?.trace?.op).toBe('navigation'); expect(secondEvent.contexts?.trace?.status).toBe('ok'); @@ -550,23 +564,6 @@ test('Creates separate transactions for rapid consecutive navigations', async ({ 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'); diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index ea81fe3dcf0c..8acd571dfe8e 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -76,6 +76,26 @@ export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentR } } +/** + * Determines if a navigation should be handled based on router state. + * Only handles: + * - PUSH navigations (always) + * - POP navigations (only after initial pageload is complete) + * - When router state is 'idle' (not 'loading' or 'submitting') + * + * During 'loading' or 'submitting', state.location may still have the old pathname, + * which would cause us to create a span for the wrong route. + */ +function shouldHandleNavigation( + state: { historyAction: string; navigation: { state: string } }, + isInitialPageloadComplete: boolean, +): boolean { + return ( + (state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) && + state.navigation.state === 'idle' + ); +} + export interface ReactRouterOptions { useEffect: UseEffect; useLocation: UseLocation; @@ -277,14 +297,7 @@ 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.navigation.state === 'idle'; - - if (shouldHandleNavigation) { + if (shouldHandleNavigation(state, isInitialPageloadComplete)) { handleNavigation({ location: state.location, routes, @@ -401,14 +414,7 @@ export function createV6CompatibleWrapCreateMemoryRouter< // 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.navigation.state === 'idle'; - - if (shouldHandleNavigation) { + if (shouldHandleNavigation(state, isInitialPageloadComplete)) { handleNavigation({ location: state.location, routes, @@ -622,8 +628,8 @@ function tryUpdateSpanName( newName: string, newSource: string, ): void { - const isNewNameBetter = newName !== currentSpanName && newName.includes(':'); - if (isNewNameBetter) { + const isNewNameParameterized = newName !== currentSpanName && newName.includes(':'); + if (isNewNameParameterized) { activeSpan.updateName(newName); activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, newSource as 'route' | 'url' | 'custom'); } From 4ee07cdc40e5bc10d60f7e3d292052e400319d28 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 13 Nov 2025 14:16:24 +0000 Subject: [PATCH 10/10] Address reviews further --- .../tests/transactions.test.ts | 37 ++++++++++--------- .../instrumentation.tsx | 6 ++- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts index ce356b2f1a24..e5b9f35042ed 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts @@ -87,7 +87,7 @@ test('Creates a navigation transaction inside a lazy route', async ({ page }) => }); test('Creates navigation transactions between two different lazy routes', async ({ page }) => { - // First, navigate to the "another-lazy" route + // Set up transaction listeners for both navigations const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { return ( !!transactionEvent?.transaction && @@ -96,6 +96,14 @@ test('Creates navigation transactions between two different lazy routes', async ); }); + const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' + ); + }); + await page.goto('/'); // Navigate to another lazy route first @@ -115,14 +123,6 @@ test('Creates navigation transactions between two different lazy routes', async expect(firstEvent.contexts?.trace?.op).toBe('navigation'); // Now navigate from the first lazy route to the second lazy route - const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { - return ( - !!transactionEvent?.transaction && - transactionEvent.contexts?.trace?.op === 'navigation' && - transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' - ); - }); - // Click the navigation link from within the first lazy route to the second lazy route const navigationToInnerFromDeep = page.locator('id=navigate-to-inner-from-deep'); await expect(navigationToInnerFromDeep).toBeVisible(); @@ -255,7 +255,7 @@ test('Does not send any duplicate navigation transaction names browsing between // Go to root page await page.goto('/'); - page.waitForTimeout(1000); + await page.waitForTimeout(1000); // Navigate to inner lazy route const navigationToInner = page.locator('id=navigation'); @@ -339,6 +339,7 @@ test('Allows legitimate POP navigation (back/forward) after pageload completes', const navigationToLongRunning = page.locator('id=navigation-to-long-running'); await expect(navigationToLongRunning).toBeVisible(); + // Set up transaction listeners for both navigations const firstNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { return ( !!transactionEvent?.transaction && @@ -347,6 +348,14 @@ test('Allows legitimate POP navigation (back/forward) after pageload completes', ); }); + const backNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/' + ); + }); + await navigationToLongRunning.click(); const slowLoadingContent = page.locator('id=slow-loading-content'); @@ -359,14 +368,6 @@ test('Allows legitimate POP navigation (back/forward) after pageload completes', // Now navigate back using browser back button (POP event) // This should create a navigation transaction since pageload is complete - const backNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { - return ( - !!transactionEvent?.transaction && - transactionEvent.contexts?.trace?.op === 'navigation' && - transactionEvent.transaction === '/' - ); - }); - await page.goBack(); // Verify we're back at home diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 8acd571dfe8e..235b207ed9a0 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -54,7 +54,7 @@ const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); /** * Tracks last navigation per client to prevent duplicate spans in cross-usage scenarios. - * Entry persists until next different navigation, handling delayed wrapper execution. + * Entry persists until the navigation span ends, allowing cross-usage detection during delayed wrapper execution. */ const LAST_NAVIGATION_PER_CLIENT = new WeakMap(); @@ -628,7 +628,9 @@ function tryUpdateSpanName( newName: string, newSource: string, ): void { - const isNewNameParameterized = newName !== currentSpanName && newName.includes(':'); + // Check if the new name contains React Router parameter syntax (/:param/) + const isReactRouterParam = /\/:[a-zA-Z0-9_]+/.test(newName); + const isNewNameParameterized = newName !== currentSpanName && isReactRouterParam; if (isNewNameParameterized) { activeSpan.updateName(newName); activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, newSource as 'route' | 'url' | 'custom');