From 10073cff0805966ed0c5d083ac6cad81ee476329 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 5 Nov 2025 12:10:15 +0000 Subject: [PATCH 1/4] fix(react): Prevent navigation span leaks for consecutive navigations --- .../tests/transactions.test.ts | 84 ++++++ .../instrumentation.tsx | 34 ++- .../test/reactrouter-cross-usage.test.tsx | 274 +++++++++++++++++- 3 files changed, 377 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 3901b0938ca5..b6a97054312a 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 @@ -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); +}); 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 e77282a61d691cf7fc8e807b087d16bfbca60700 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 13:01:05 +0000 Subject: [PATCH 2/4] 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 a48ecaf767a02444d0bbca0dbf03265987d49f2a Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 13:18:31 +0000 Subject: [PATCH 3/4] 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 f703ae92ce9d011a148bf3b274fb2f159af1b3d2 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Nov 2025 21:56:29 +0000 Subject: [PATCH 4/4] 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"