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..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 @@ -504,7 +505,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,20 +514,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 && @@ -535,40 +522,54 @@ 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'); + 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'); 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..235b207ed9a0 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. + * Entry persists until the navigation span ends, allowing cross-usage detection during delayed wrapper execution. */ +const LAST_NAVIGATION_PER_CLIENT = new WeakMap(); + export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { const existingChildren = parentRoute.children || []; @@ -74,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; @@ -275,27 +297,15 @@ export function createV6CompatibleWrapCreateBrowserRouter< // If we haven't seen a pageload span yet, keep waiting (don't mark as complete) } - const shouldHandleNavigation = - state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete); - - if (shouldHandleNavigation) { - const navigationHandler = (): void => { - handleNavigation({ - location: state.location, - routes, - navigationType: state.historyAction, - version, - basename, - allRoutes: Array.from(allRoutes), - }); - }; - - // Wait for the next render if loading an unsettled route - if (state.navigation.state !== 'idle') { - requestAnimationFrame(navigationHandler); - } else { - navigationHandler(); - } + if (shouldHandleNavigation(state, isInitialPageloadComplete)) { + handleNavigation({ + location: state.location, + routes, + navigationType: state.historyAction, + version, + basename, + allRoutes: Array.from(allRoutes), + }); } }); @@ -404,29 +414,15 @@ export function createV6CompatibleWrapCreateMemoryRouter< // If we haven't seen a pageload span yet, keep waiting (don't mark as complete) } - const location = state.location; - - const shouldHandleNavigation = - state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete); - - 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(); - } + if (shouldHandleNavigation(state, isInitialPageloadComplete)) { + handleNavigation({ + location: state.location, + routes, + navigationType: state.historyAction, + version, + basename, + allRoutes: Array.from(allRoutes), + }); } }); @@ -622,6 +618,71 @@ 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 { + // 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'); + } +} + +function isDuplicateNavigation(client: Client, navigationKey: string): boolean { + const lastKey = LAST_NAVIGATION_PER_CLIENT.get(client); + return lastKey === navigationKey; +} + +function createNavigationSpan(opts: { + client: Client; + name: string; + source: string; + version: string; + location: Location; + routes: RouteObject[]; + basename?: string; + allRoutes?: RouteObject[]; + navigationKey: string; +}): Span | undefined { + const { client, name, source, version, location, routes, basename, allRoutes, navigationKey } = opts; + + const navigationSpan = startBrowserTracingNavigationSpan(client, { + name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source as 'route' | 'url' | 'custom', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, + }, + }); + + if (navigationSpan) { + LAST_NAVIGATION_PER_CLIENT.set(client, navigationKey); + patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); + + const unsubscribe = client.on('spanEnd', endedSpan => { + if (endedSpan === navigationSpan) { + // Clear key only if it's still our key (handles overlapping navigations) + const lastKey = LAST_NAVIGATION_PER_CLIENT.get(client); + if (lastKey === navigationKey) { + LAST_NAVIGATION_PER_CLIENT.delete(client); + } + unsubscribe(); // Prevent memory leak + } + }); + } + + return navigationSpan; +} + export function handleNavigation(opts: { location: Location; routes: RouteObject[]; @@ -632,15 +693,13 @@ export function handleNavigation(opts: { allRoutes?: RouteObject[]; }): void { const { location, routes, navigationType, version, matches, basename, allRoutes } = opts; - const branches = Array.isArray(matches) ? matches : _matchRoutes(routes, location, basename); + const branches = Array.isArray(matches) ? matches : _matchRoutes(allRoutes || routes, location, basename); const client = getClient(); if (!client || !CLIENTS_WITH_INSTRUMENT_NAVIGATION.has(client)) { return; } - // Avoid starting a navigation span on initial load when a pageload root span is active. - // This commonly happens when lazy routes resolve during the first render and React Router emits a POP. const activeRootSpan = getActiveRootSpan(); if (activeRootSpan && spanToJSON(activeRootSpan).op === 'pageload' && navigationType === 'POP') { return; @@ -649,31 +708,39 @@ export function handleNavigation(opts: { if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) { const [name, source] = resolveRouteNameAndSource( location, - routes, + allRoutes || routes, allRoutes || routes, branches as RouteMatch[], basename, ); - const activeSpan = getActiveSpan(); - const spanJson = activeSpan && spanToJSON(activeSpan); - const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; + const currentNavigationKey = getNavigationKey(location); + const isNavDuplicate = isDuplicateNavigation(client, currentNavigationKey); - // Cross usage can result in multiple navigation spans being created without this check - if (!isAlreadyInNavigationSpan) { - const navigationSpan = startBrowserTracingNavigationSpan(client, { - name, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, - }, - }); + if (isNavDuplicate) { + // Cross-usage duplicate - update existing span name if better + const activeSpan = getActiveSpan(); + const spanJson = activeSpan && spanToJSON(activeSpan); + const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; - // Patch navigation span to handle early cancellation (e.g., document.hidden) - if (navigationSpan) { - patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); + 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, + source, + version, + location, + routes, + basename, + allRoutes, + navigationKey: currentNavigationKey, + }); } } } @@ -727,7 +794,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 +853,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/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']; } /** diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index 77d8e3d95b2e..be71f4b838c5 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, @@ -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,16 +220,10 @@ 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).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', - }, - }); + // 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'); }); }); @@ -339,7 +335,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,17 +460,12 @@ 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).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'); }); }); @@ -597,14 +587,290 @@ describe('React Router cross usage of wrappers', () => { expect(container.innerHTML).toContain('Details'); 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'); + }); + }); + + 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: '/second-level/:id/third-level/:id', + 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 create separate spans for same route 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 waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2)); + }); + + // 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 () => { + 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', + }), + }); }); }); }); 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', () => {