diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx index 7787b60be398..1bcad5eaf4ce 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx @@ -126,6 +126,14 @@ const router = sentryCreateBrowserRouter( lazyChildren: () => import('./pages/deep/Level1Routes').then(module => module.level2Routes), }, }, + { + path: '/slow-fetch', + handle: { + // This lazy handler takes 500ms due to the top-level await in SlowFetchLazyRoutes.tsx + // It also makes a fetch request during loading which creates a span + lazyChildren: () => import('./pages/SlowFetchLazyRoutes').then(module => module.slowFetchRoutes), + }, + }, ], { async patchRoutesOnNavigation({ matches, patch }: Parameters[0]) { diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/DelayedLazyRoute.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/DelayedLazyRoute.tsx index 41e5ba5463be..53bfe048ca4e 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/DelayedLazyRoute.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/DelayedLazyRoute.tsx @@ -23,6 +23,14 @@ const DelayedLazyRoute = () => { Back Home
+ + Go to Slow Fetch Route (500ms) + +
+ + Go to Another Lazy Route + +
View: Detailed (query param) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx index 21b965f571f3..cf80af402b96 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx @@ -31,6 +31,10 @@ const Index = () => { Navigate to Deep Nested Route (3 levels, 900ms total) +
+ + Navigate to Slow Fetch Route (500ms delay with fetch) + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/SlowFetchLazyRoutes.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/SlowFetchLazyRoutes.tsx new file mode 100644 index 000000000000..f24a8c56f416 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/SlowFetchLazyRoutes.tsx @@ -0,0 +1,42 @@ +import React from 'react'; +import { Link, useParams } from 'react-router-dom'; + +// Simulate a slow async fetch during lazy route loading +// This delay happens before the module exports, simulating network latency +const fetchPromise = fetch('/api/slow-data') + .then(res => res.json()) + .catch(() => ({ message: 'fallback data' })); + +// Add a 500ms delay to simulate slow lazy loading +await new Promise(resolve => setTimeout(resolve, 500)); + +// Component that displays the lazy-loaded data +const SlowFetchComponent = () => { + const { id } = useParams<{ id: string }>(); + const [data, setData] = React.useState<{ message: string } | null>(null); + + React.useEffect(() => { + fetchPromise.then(setData); + }, []); + + return ( +
+

Slow Fetch Route

+

ID: {id}

+

Data: {data?.message || 'loading...'}

+ + Go Home + + + Go to Another Lazy Route + +
+ ); +}; + +export const slowFetchRoutes = [ + { + path: ':id', + element: , + }, +]; 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 ce8137d7f686..f7a3ec4a5519 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 @@ -666,7 +666,7 @@ test('Creates navigation transaction when navigating with query parameters from await expect(page.locator('id=delayed-lazy-source')).toHaveText('Source: homepage'); // Verify the navigation transaction has the correct parameterized route name - // Query parameters should NOT affect the transaction name (still /delayed-lazy/:id) + // Query parameters don't affect the transaction name (still /delayed-lazy/:id) expect(navigationEvent.transaction).toBe('/delayed-lazy/:id'); expect(navigationEvent.contexts?.trace?.op).toBe('navigation'); expect(navigationEvent.contexts?.trace?.data?.['sentry.source']).toBe('route'); @@ -883,3 +883,348 @@ test('Creates navigation transaction when changing both query and hash on same r expect(navigationEvent.contexts?.trace?.data?.['sentry.source']).toBe('route'); expect(navigationEvent.contexts?.trace?.status).toBe('ok'); }); + +test('Creates navigation transaction with correct name for slow lazy route', async ({ page }) => { + // This test verifies that navigating to a slow lazy route (with top-level await) + // creates a correctly named navigation transaction. + // The route uses handle.lazyChildren with a 500ms delay. + + await page.goto('/'); + + const navigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/slow-fetch/:id' + ); + }); + + // Navigate to slow-fetch route (500ms delay) + const navigationToSlowFetch = page.locator('id=navigation-to-slow-fetch'); + await expect(navigationToSlowFetch).toBeVisible(); + await navigationToSlowFetch.click(); + + const navigationEvent = await navigationPromise; + + // Wait for the component to render (after the 500ms delay) + const slowFetchContent = page.locator('id=slow-fetch-content'); + await expect(slowFetchContent).toBeVisible({ timeout: 5000 }); + await expect(page.locator('id=slow-fetch-id')).toHaveText('ID: 123'); + + // Verify the transaction has the correct parameterized route name + expect(navigationEvent.transaction).toBe('/slow-fetch/:id'); + expect(navigationEvent.contexts?.trace?.op).toBe('navigation'); + expect(navigationEvent.contexts?.trace?.data?.['sentry.source']).toBe('route'); +}); + +test('Rapid navigation does not corrupt transaction names when lazy handlers resolve late', async ({ page }) => { + await page.goto('/'); + + const allTransactions: Array<{ name: string; op: string }> = []; + + const collectorPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op) { + allTransactions.push({ + name: transactionEvent.transaction, + op: transactionEvent.contexts.trace.op, + }); + } + return allTransactions.length >= 2; + }); + + // Navigate to slow-fetch route (500ms delay) + const slowFetchLink = page.locator('id=navigation-to-slow-fetch'); + await expect(slowFetchLink).toBeVisible(); + await slowFetchLink.click(); + + // Navigate away before lazy handler resolves + await page.waitForTimeout(200); + const anotherLink = page.locator('id=navigation-to-another'); + await anotherLink.click(); + + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + await page.waitForTimeout(3000); + + await Promise.race([ + collectorPromise, + new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 15000)), + ]); + + const navigationTransactions = allTransactions.filter(t => t.op === 'navigation'); + + expect(navigationTransactions.length).toBeGreaterThanOrEqual(1); + + // No "/" corruption + const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); + + // At least one valid route name + const validRoutePatterns = [ + '/slow-fetch/:id', + '/another-lazy/sub', + '/another-lazy/sub/:id', + '/another-lazy/sub/:id/:subId', + ]; + const hasValidRouteName = navigationTransactions.some(t => validRoutePatterns.includes(t.name)); + expect(hasValidRouteName).toBe(true); +}); + +test('Correctly names pageload transaction for slow lazy route with fetch', async ({ page }) => { + // This test verifies that a slow lazy route (with top-level await and fetch) + // creates a correctly named pageload transaction + + const pageloadPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'pageload' && + transactionEvent.transaction === '/slow-fetch/:id' + ); + }); + + await page.goto('/slow-fetch/123'); + + const pageloadEvent = await pageloadPromise; + + // Wait for the component to render (after the 500ms delay) + const slowFetchContent = page.locator('id=slow-fetch-content'); + await expect(slowFetchContent).toBeVisible({ timeout: 5000 }); + await expect(page.locator('id=slow-fetch-id')).toHaveText('ID: 123'); + + // Verify the transaction has the correct parameterized route name + expect(pageloadEvent.transaction).toBe('/slow-fetch/:id'); + expect(pageloadEvent.contexts?.trace?.op).toBe('pageload'); + expect(pageloadEvent.contexts?.trace?.data?.['sentry.source']).toBe('route'); + + // Verify the transaction contains a fetch span + const spans = pageloadEvent.spans || []; + const fetchSpan = spans.find( + (span: { op?: string; description?: string }) => + span.op === 'http.client' && span.description?.includes('/api/slow-data'), + ); + + // The fetch span should exist (even if the fetch failed, the span is created) + expect(fetchSpan).toBeDefined(); +}); + +test('Three-route rapid navigation preserves distinct transaction names', async ({ page }) => { + const navigationTransactions: Array<{ name: string }> = []; + + const navigationCollector = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent.contexts?.trace?.op === 'navigation') { + navigationTransactions.push({ name: transactionEvent.transaction || '' }); + } + return false; + }); + + const pageloadPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'pageload' && + transactionEvent.transaction === '/delayed-lazy/:id' + ); + }); + + // Pageload to delayed-lazy route + await page.goto('/delayed-lazy/111'); + await pageloadPromise; + await expect(page.locator('id=delayed-lazy-ready')).toBeVisible({ timeout: 5000 }); + + // Navigate to slow-fetch (500ms delay) + const slowFetchLink = page.locator('id=delayed-lazy-to-slow-fetch'); + await slowFetchLink.click(); + await page.waitForTimeout(150); + + // Navigate to another-lazy before slow-fetch resolves + const anotherLazyLink = page.locator('id=delayed-lazy-to-another-lazy'); + await anotherLazyLink.click(); + + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + await page.waitForTimeout(2000); + + await Promise.race([ + navigationCollector, + new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 5000)), + ]).catch(() => {}); + + expect(navigationTransactions.length).toBe(2); + + // Distinct names (corruption causes both to have same name) + const uniqueNames = new Set(navigationTransactions.map(t => t.name)); + expect(uniqueNames.size).toBe(2); + + // No "/" corruption + const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); +}); + +test('Zero-wait rapid navigation does not corrupt transaction names', async ({ page }) => { + const navigationTransactions: Array<{ name: string }> = []; + + const collector = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent.contexts?.trace?.op === 'navigation') { + navigationTransactions.push({ name: transactionEvent.transaction || '' }); + } + return false; + }); + + await page.goto('/'); + + const slowFetchLink = page.locator('id=navigation-to-slow-fetch'); + const anotherLink = page.locator('id=navigation-to-another'); + await expect(slowFetchLink).toBeVisible(); + await expect(anotherLink).toBeVisible(); + + // Click first then immediately second (no wait) + await slowFetchLink.click(); + await anotherLink.click(); + + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + await page.waitForTimeout(3000); + + await Promise.race([collector, new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 5000))]).catch( + () => {}, + ); + + expect(navigationTransactions.length).toBeGreaterThanOrEqual(1); + + // No "/" corruption + const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); +}); + +test('Browser back during lazy handler resolution does not corrupt', async ({ page }) => { + const allTransactions: Array<{ name: string; op: string }> = []; + + const collector = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op) { + allTransactions.push({ + name: transactionEvent.transaction, + op: transactionEvent.contexts.trace.op, + }); + } + return false; + }); + + await page.goto('/'); + await expect(page.locator('id=navigation')).toBeVisible(); + + // Navigate to another-lazy to establish history + const anotherLink = page.locator('id=navigation-to-another'); + await anotherLink.click(); + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + + // Navigate to slow-fetch route + await page.goto('/slow-fetch/123'); + await page.waitForTimeout(150); + + // Press browser back before handler resolves + await page.goBack(); + await page.waitForTimeout(3000); + + await Promise.race([collector, new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 10000))]).catch( + () => {}, + ); + + expect(allTransactions.length).toBeGreaterThanOrEqual(1); + expect(allTransactions.every(t => t.name.length > 0)).toBe(true); +}); + +test('Multiple overlapping lazy handlers do not corrupt each other', async ({ page }) => { + const navigationTransactions: Array<{ name: string }> = []; + + const collector = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent.contexts?.trace?.op === 'navigation') { + navigationTransactions.push({ name: transactionEvent.transaction || '' }); + } + return false; + }); + + await page.goto('/'); + + // Navigation 1: To delayed-lazy (400ms delay) + const delayedLazyLink = page.locator('id=navigation-to-delayed-lazy'); + await expect(delayedLazyLink).toBeVisible(); + await delayedLazyLink.click(); + await page.waitForTimeout(50); + + // Navigation 2: To slow-fetch (500ms delay) + const slowFetchLink = page.locator('id=navigation-to-slow-fetch'); + await slowFetchLink.click(); + await page.waitForTimeout(50); + + // Navigation 3: To another-lazy (fast) + const anotherLink = page.locator('id=navigation-to-another'); + await anotherLink.click(); + + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + await page.waitForTimeout(3000); + + await Promise.race([collector, new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 5000))]).catch( + () => {}, + ); + + expect(navigationTransactions.length).toBeGreaterThanOrEqual(1); + + // No "/" corruption + const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); + + // If multiple navigations, they should have distinct names + if (navigationTransactions.length >= 2) { + const allSameName = navigationTransactions.every(t => t.name === navigationTransactions[0].name); + expect(allSameName).toBe(false); + } +}); + +test('Query/hash navigation does not corrupt transaction name', async ({ page }) => { + const navigationTransactions: Array<{ name: string }> = []; + + const collectorPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation') { + navigationTransactions.push({ name: transactionEvent.transaction }); + } + return navigationTransactions.length >= 1; + }); + + await page.goto('/'); + + // Navigate to delayed-lazy route + const delayedLazyLink = page.locator('id=navigation-to-delayed-lazy'); + await expect(delayedLazyLink).toBeVisible(); + await delayedLazyLink.click(); + await expect(page.locator('id=delayed-lazy-ready')).toBeVisible({ timeout: 10000 }); + + // Trigger query-only navigation + const queryLink = page.locator('id=link-to-query-view-detailed'); + await expect(queryLink).toBeVisible(); + await queryLink.click(); + await page.waitForURL('**/delayed-lazy/**?view=detailed'); + + // Trigger hash-only navigation + const hashLink = page.locator('id=link-to-hash-section1'); + await expect(hashLink).toBeVisible(); + await hashLink.click(); + await page.waitForTimeout(500); + expect(page.url()).toContain('#section1'); + + // Trigger combined query+hash navigation + const combinedLink = page.locator('id=link-to-query-and-hash'); + await expect(combinedLink).toBeVisible(); + await combinedLink.click(); + await page.waitForTimeout(500); + expect(page.url()).toContain('view=grid'); + expect(page.url()).toContain('#results'); + + await page.waitForTimeout(2000); + await Promise.race([ + collectorPromise, + new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 5000)), + ]).catch(() => {}); + + expect(navigationTransactions.length).toBeGreaterThanOrEqual(1); + expect(navigationTransactions[0].name).toBe('/delayed-lazy/:id'); + + // No "/" corruption from query/hash navigations + const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); +}); diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index bb91ba8d3072..968abd9ecae6 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -26,6 +26,11 @@ export { pathIsWildcardAndHasChildren, getNumberOfUrlSegments, transactionNameHasWildcard, + getActiveRootSpan, + // Navigation context functions (internal use and testing) + setNavigationContext, + clearNavigationContext, + getNavigationContext, } from './utils'; // Lazy route exports diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 6e19b9021ba5..d646624618f9 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -12,10 +12,8 @@ import type { Client, Integration, Span } from '@sentry/core'; import { addNonEnumerableProperty, debug, - getActiveSpan, getClient, getCurrentScope, - getRootSpan, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -41,7 +39,14 @@ import type { UseRoutes, } from '../types'; import { checkRouteForAsyncHandler } from './lazy-routes'; -import { initializeRouterUtils, resolveRouteNameAndSource, transactionNameHasWildcard } from './utils'; +import { + clearNavigationContext, + getActiveRootSpan, + initializeRouterUtils, + resolveRouteNameAndSource, + setNavigationContext, + transactionNameHasWildcard, +} from './utils'; let _useEffect: UseEffect; let _useLocation: UseLocation; @@ -230,11 +235,14 @@ function trackLazyRouteLoad(span: Span, promise: Promise): void { /** * Processes resolved routes by adding them to allRoutes and checking for nested async handlers. + * When capturedSpan is provided, updates that specific span instead of the current active span. + * This prevents race conditions where a lazy handler resolves after the user has navigated away. */ export function processResolvedRoutes( resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation: Location | null = null, + capturedSpan?: Span, ): void { resolvedRoutes.forEach(child => { allRoutes.add(child); @@ -249,17 +257,27 @@ export function processResolvedRoutes( addResolvedRoutesToParent(resolvedRoutes, parentRoute); } - // After processing lazy routes, check if we need to update an active transaction - const activeRootSpan = getActiveRootSpan(); - if (activeRootSpan) { - const spanOp = spanToJSON(activeRootSpan).op; + // Use captured span if provided, otherwise fall back to current active span + const targetSpan = capturedSpan ?? getActiveRootSpan(); + if (targetSpan) { + const spanJson = spanToJSON(targetSpan); - // Try to use the provided location first, then fall back to global window location if needed + // Skip update if span has already ended (timestamp is set when span.end() is called) + if (spanJson.timestamp) { + DEBUG_BUILD && debug.warn('[React Router] Lazy handler resolved after span ended - skipping update'); + return; + } + + const spanOp = spanJson.op; + + // Use captured location for route matching (ensures we match against the correct route) + // Fall back to window.location only if no captured location and no captured span + // (i.e., this is not from an async handler) let location = currentLocation; - if (!location) { + if (!location && !capturedSpan) { if (typeof WINDOW !== 'undefined') { const globalLocation = WINDOW.location; - if (globalLocation) { + if (globalLocation?.pathname) { location = { pathname: globalLocation.pathname }; } } @@ -269,14 +287,14 @@ export function processResolvedRoutes( if (spanOp === 'pageload') { // Re-run the pageload transaction update with the newly loaded routes updatePageloadTransaction({ - activeRootSpan, + activeRootSpan: targetSpan, location: { pathname: location.pathname }, routes: Array.from(allRoutes), allRoutes: Array.from(allRoutes), }); } else if (spanOp === 'navigation') { // For navigation spans, update the name with the newly loaded routes - updateNavigationSpan(activeRootSpan, location, Array.from(allRoutes), false, _matchRoutes); + updateNavigationSpan(targetSpan, location, Array.from(allRoutes), false, _matchRoutes); } } } @@ -713,7 +731,12 @@ function wrapPatchRoutesOnNavigation( (args as any).patch = (routeId: string, children: RouteObject[]) => { addRoutesToAllRoutes(children); const currentActiveRootSpan = getActiveRootSpan(); - if (currentActiveRootSpan && (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation') { + // Only update if we have a valid targetPath (patchRoutesOnNavigation can be called without path) + if ( + targetPath && + currentActiveRootSpan && + (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation' + ) { updateNavigationSpan( currentActiveRootSpan, { pathname: targetPath, search: '', hash: '', state: null, key: 'default' }, @@ -728,7 +751,14 @@ function wrapPatchRoutesOnNavigation( } const lazyLoadPromise = (async () => { - const result = await originalPatchRoutes(args); + // Set context so async handlers can access correct targetPath and span + const contextToken = setNavigationContext(targetPath, activeRootSpan); + let result; + try { + result = await originalPatchRoutes(args); + } finally { + clearNavigationContext(contextToken); + } const currentActiveRootSpan = getActiveRootSpan(); if (currentActiveRootSpan && (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation') { @@ -1184,17 +1214,3 @@ export function createV6CompatibleWithSentryReactRouterRouting

unknown, route: RouteObject, handlerKey: string, - processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, + processResolvedRoutes: ( + resolvedRoutes: RouteObject[], + parentRoute?: RouteObject, + currentLocation?: Location, + capturedSpan?: Span, + ) => void, ): (...args: unknown[]) => unknown { const proxy = new Proxy(originalFunction, { apply(target: (...args: unknown[]) => unknown, thisArg, argArray) { + const locationAtInvocation = captureCurrentLocation(); + const spanAtInvocation = captureActiveSpan(); const result = target.apply(thisArg, argArray); - handleAsyncHandlerResult(result, route, handlerKey, processResolvedRoutes); + handleAsyncHandlerResult( + result, + route, + handlerKey, + processResolvedRoutes, + locationAtInvocation, + spanAtInvocation, + ); return result; }, }); @@ -26,25 +94,33 @@ export function createAsyncHandlerProxy( /** * Handles the result of an async handler function call. + * Passes the captured span through to ensure the correct span is updated. */ export function handleAsyncHandlerResult( result: unknown, route: RouteObject, handlerKey: string, - processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, + processResolvedRoutes: ( + resolvedRoutes: RouteObject[], + parentRoute?: RouteObject, + currentLocation?: Location, + capturedSpan?: Span, + ) => void, + currentLocation: Location | null, + capturedSpan: Span | undefined, ): void { if (isThenable(result)) { (result as Promise) .then((resolvedRoutes: unknown) => { if (Array.isArray(resolvedRoutes)) { - processResolvedRoutes(resolvedRoutes, route); + processResolvedRoutes(resolvedRoutes, route, currentLocation ?? undefined, capturedSpan); } }) .catch((e: unknown) => { DEBUG_BUILD && debug.warn(`Error resolving async handler '${handlerKey}' for route`, route, e); }); } else if (Array.isArray(result)) { - processResolvedRoutes(result, route); + processResolvedRoutes(result, route, currentLocation ?? undefined, capturedSpan); } } @@ -53,7 +129,12 @@ export function handleAsyncHandlerResult( */ export function checkRouteForAsyncHandler( route: RouteObject, - processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, + processResolvedRoutes: ( + resolvedRoutes: RouteObject[], + parentRoute?: RouteObject, + currentLocation?: Location, + capturedSpan?: Span, + ) => void, ): void { // Set up proxies for any functions in the route's handle if (route.handle && typeof route.handle === 'object') { diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index 8431e283108b..96c178b64c14 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,10 +1,57 @@ -import type { TransactionSource } from '@sentry/core'; +import type { Span, TransactionSource } from '@sentry/core'; +import { debug, getActiveSpan, getRootSpan, spanToJSON } from '@sentry/core'; +import { DEBUG_BUILD } from '../debug-build'; import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; // Global variables that these utilities depend on let _matchRoutes: MatchRoutes; let _stripBasename: boolean = false; +// Navigation context stack for nested/concurrent patchRoutesOnNavigation calls. +// Required because window.location hasn't updated yet when handlers are invoked. +interface NavigationContext { + token: object; + targetPath: string | undefined; + span: Span | undefined; +} + +const _navigationContextStack: NavigationContext[] = []; +const MAX_CONTEXT_STACK_SIZE = 10; + +/** + * Pushes a navigation context and returns a unique token for cleanup. + * The token uses object identity for uniqueness (no counter needed). + */ +export function setNavigationContext(targetPath: string | undefined, span: Span | undefined): object { + const token = {}; + // Prevent unbounded stack growth - oldest (likely stale) contexts are evicted first + if (_navigationContextStack.length >= MAX_CONTEXT_STACK_SIZE) { + DEBUG_BUILD && debug.warn('[React Router] Navigation context stack overflow - removing oldest context'); + _navigationContextStack.shift(); + } + _navigationContextStack.push({ token, targetPath, span }); + return token; +} + +/** + * Clears the navigation context if it's on top of the stack (LIFO). + * If our context is not on top (out-of-order completion), we leave it - + * it will be cleaned up by overflow protection when the stack fills up. + */ +export function clearNavigationContext(token: object): void { + const top = _navigationContextStack[_navigationContextStack.length - 1]; + if (top?.token === token) { + _navigationContextStack.pop(); + } +} + +/** Gets the current (most recent) navigation context if inside a patchRoutesOnNavigation call. */ +export function getNavigationContext(): NavigationContext | null { + const length = _navigationContextStack.length; + // The `?? null` converts undefined (from array access) to null to match return type + return length > 0 ? (_navigationContextStack[length - 1] ?? null) : null; +} + /** * Initialize function to set dependencies that the router utilities need. * Must be called before using any of the exported utility functions. @@ -273,3 +320,20 @@ export function resolveRouteNameAndSource( return [name || location.pathname, source]; } + +/** + * Gets the active root span if it's a pageload or navigation span. + */ +export function getActiveRootSpan(): Span | undefined { + const span = getActiveSpan(); + const rootSpan = span ? getRootSpan(span) : undefined; + + if (!rootSpan) { + return undefined; + } + + const op = spanToJSON(rootSpan).op; + + // Only use this root span if it is a pageload or navigation span + return op === 'navigation' || op === 'pageload' ? rootSpan : undefined; +} diff --git a/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx index 276a5b9950fc..3d2b4f198cf5 100644 --- a/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx +++ b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx @@ -59,6 +59,7 @@ vi.mock('../../src/reactrouter-compat-utils/utils', () => ({ transactionNameHasWildcard: vi.fn((name: string) => { return name.includes('/*') || name === '*' || name.endsWith('*'); }), + getActiveRootSpan: vi.fn(() => undefined), })); vi.mock('../../src/reactrouter-compat-utils/lazy-routes', () => ({ diff --git a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts index 732b893ea8f8..0d1a493e08f2 100644 --- a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts +++ b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts @@ -106,7 +106,9 @@ describe('reactrouter-compat-utils/lazy-routes', () => { proxy(); // Since handleAsyncHandlerResult is called internally, we verify through its side effects - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(['route1', 'route2'], route); + // The third parameter is the captured location (undefined in jsdom test environment) + // The fourth parameter is the captured span (undefined since no active span in test) + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(['route1', 'route2'], route, undefined, undefined); }); it('should handle functions that throw exceptions', () => { @@ -137,35 +139,38 @@ describe('reactrouter-compat-utils/lazy-routes', () => { const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); proxy(); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route); + // The third parameter is the captured location (undefined in jsdom test environment) + // The fourth parameter is the captured span (undefined since no active span in test) + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route, undefined, undefined); }); }); describe('handleAsyncHandlerResult', () => { const route: RouteObject = { path: '/test' }; const handlerKey = 'testHandler'; + const mockLocation = { pathname: '/test', search: '', hash: '', state: null, key: 'default' }; it('should handle array results directly', () => { const routes: RouteObject[] = [{ path: '/route1' }, { path: '/route2' }]; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); }); it('should handle empty array results', () => { const routes: RouteObject[] = []; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); }); it('should handle Promise results that resolve to arrays', async () => { const routes: RouteObject[] = [{ path: '/route1' }, { path: '/route2' }]; const promiseResult = Promise.resolve(routes); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); // Wait for the promise to resolve await promiseResult; @@ -173,25 +178,25 @@ describe('reactrouter-compat-utils/lazy-routes', () => { // Use setTimeout to wait for the async handling await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); }); it('should handle Promise results that resolve to empty arrays', async () => { const routes: RouteObject[] = []; const promiseResult = Promise.resolve(routes); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); }); it('should handle Promise results that resolve to non-arrays', async () => { const promiseResult = Promise.resolve('not an array'); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -202,7 +207,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { it('should handle Promise results that resolve to null', async () => { const promiseResult = Promise.resolve(null); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -213,7 +218,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { it('should handle Promise results that resolve to undefined', async () => { const promiseResult = Promise.resolve(undefined); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -224,7 +229,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { it('should handle Promise rejections gracefully', async () => { const promiseResult = Promise.reject(new Error('Test error')); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); // Wait for the promise to be handled await new Promise(resolve => setTimeout(resolve, 0)); @@ -240,7 +245,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { it('should handle Promise rejections with non-Error values', async () => { const promiseResult = Promise.reject('string error'); - handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await new Promise(resolve => setTimeout(resolve, 0)); @@ -253,25 +258,25 @@ describe('reactrouter-compat-utils/lazy-routes', () => { }); it('should ignore non-promise, non-array results', () => { - handleAsyncHandlerResult('string result', route, handlerKey, mockProcessResolvedRoutes); - handleAsyncHandlerResult(123, route, handlerKey, mockProcessResolvedRoutes); - handleAsyncHandlerResult({ not: 'array' }, route, handlerKey, mockProcessResolvedRoutes); - handleAsyncHandlerResult(null, route, handlerKey, mockProcessResolvedRoutes); - handleAsyncHandlerResult(undefined, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult('string result', route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + handleAsyncHandlerResult(123, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + handleAsyncHandlerResult({ not: 'array' }, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + handleAsyncHandlerResult(null, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + handleAsyncHandlerResult(undefined, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); it('should ignore boolean values', () => { - handleAsyncHandlerResult(true, route, handlerKey, mockProcessResolvedRoutes); - handleAsyncHandlerResult(false, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(true, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + handleAsyncHandlerResult(false, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); it('should ignore functions as results', () => { const functionResult = () => 'test'; - handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -281,7 +286,14 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: 'not a function', }; - handleAsyncHandlerResult(fakeThenableButNotPromise, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult( + fakeThenableButNotPromise, + route, + handlerKey, + mockProcessResolvedRoutes, + mockLocation, + undefined, + ); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -291,7 +303,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: null, }; - handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -306,12 +318,19 @@ describe('reactrouter-compat-utils/lazy-routes', () => { const routes: RouteObject[] = [{ path: '/dynamic1' }, { path: '/dynamic2' }]; const promiseResult = Promise.resolve(routes); - handleAsyncHandlerResult(promiseResult, complexRoute, 'complexHandler', mockProcessResolvedRoutes); + handleAsyncHandlerResult( + promiseResult, + complexRoute, + 'complexHandler', + mockProcessResolvedRoutes, + mockLocation, + undefined, + ); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute, mockLocation, undefined); }); it('should handle nested route objects in arrays', () => { @@ -322,9 +341,18 @@ describe('reactrouter-compat-utils/lazy-routes', () => { }, ]; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); + }); + + it('should convert null location to undefined for processResolvedRoutes', () => { + const routes: RouteObject[] = [{ path: '/route1' }]; + + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, null, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + // When null is passed, it should convert to undefined for processResolvedRoutes + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, undefined, undefined); }); }); diff --git a/packages/react/test/reactrouter-compat-utils/utils.test.ts b/packages/react/test/reactrouter-compat-utils/utils.test.ts index 438b026104bd..401ea648b0fc 100644 --- a/packages/react/test/reactrouter-compat-utils/utils.test.ts +++ b/packages/react/test/reactrouter-compat-utils/utils.test.ts @@ -1,5 +1,7 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { + clearNavigationContext, + getNavigationContext, getNormalizedName, getNumberOfUrlSegments, initializeRouterUtils, @@ -9,6 +11,7 @@ import { prefixWithSlash, rebuildRoutePathFromAllRoutes, resolveRouteNameAndSource, + setNavigationContext, transactionNameHasWildcard, } from '../../src/reactrouter-compat-utils'; import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../../src/types'; @@ -664,4 +667,137 @@ describe('reactrouter-compat-utils/utils', () => { expect(transactionNameHasWildcard('/path/to/asterisk')).toBe(false); // 'asterisk' contains 'isk' but not '*' }); }); + + describe('navigation context management', () => { + // Clean up navigation context after each test by popping until empty + afterEach(() => { + // Pop all remaining contexts + while (getNavigationContext() !== null) { + const ctx = getNavigationContext(); + if (ctx) { + clearNavigationContext((ctx as any).token); + } + } + }); + + describe('setNavigationContext', () => { + it('should return unique tokens (object identity)', () => { + const token1 = setNavigationContext('/path1', undefined); + const token2 = setNavigationContext('/path2', undefined); + const token3 = setNavigationContext('/path3', undefined); + + // Each token should be a unique object + expect(token1).not.toBe(token2); + expect(token2).not.toBe(token3); + expect(token1).not.toBe(token3); + }); + + it('should store targetPath and span in context', () => { + const mockSpan = { name: 'test-span' } as any; + setNavigationContext('/test-path', mockSpan); + + const context = getNavigationContext(); + expect(context).not.toBeNull(); + expect(context?.targetPath).toBe('/test-path'); + expect(context?.span).toBe(mockSpan); + }); + + it('should handle undefined targetPath', () => { + setNavigationContext(undefined, undefined); + + const context = getNavigationContext(); + expect(context).not.toBeNull(); + expect(context?.targetPath).toBeUndefined(); + }); + }); + + describe('clearNavigationContext', () => { + it('should remove context when token matches top of stack (LIFO)', () => { + const token = setNavigationContext('/test', undefined); + + expect(getNavigationContext()).not.toBeNull(); + + clearNavigationContext(token); + + expect(getNavigationContext()).toBeNull(); + }); + + it('should NOT remove context when token is not on top (out-of-order completion)', () => { + // Simulate: Nav1 starts, Nav2 starts, Nav1 tries to complete first + const token1 = setNavigationContext('/nav1', undefined); + const token2 = setNavigationContext('/nav2', undefined); + + // Most recent should be nav2 + expect(getNavigationContext()?.targetPath).toBe('/nav2'); + + // Nav1 tries to complete first (out of order) - should NOT pop because nav1 is not on top + clearNavigationContext(token1); + + // Nav2 should still be the current context (nav1's context is still buried) + expect(getNavigationContext()?.targetPath).toBe('/nav2'); + + // Nav2 completes - should pop because nav2 IS on top + clearNavigationContext(token2); + + // Now nav1's stale context is on top (will be cleaned by overflow protection) + expect(getNavigationContext()?.targetPath).toBe('/nav1'); + }); + + it('should not throw when clearing with unknown token', () => { + const unknownToken = {}; + expect(() => clearNavigationContext(unknownToken)).not.toThrow(); + }); + + it('should correctly handle LIFO cleanup order', () => { + const token1 = setNavigationContext('/path1', undefined); + const token2 = setNavigationContext('/path2', undefined); + const token3 = setNavigationContext('/path3', undefined); + + // Clear in LIFO order + clearNavigationContext(token3); + expect(getNavigationContext()?.targetPath).toBe('/path2'); + + clearNavigationContext(token2); + expect(getNavigationContext()?.targetPath).toBe('/path1'); + + clearNavigationContext(token1); + expect(getNavigationContext()).toBeNull(); + }); + }); + + describe('getNavigationContext', () => { + it('should return null when stack is empty', () => { + expect(getNavigationContext()).toBeNull(); + }); + + it('should return the most recent context', () => { + setNavigationContext('/first', undefined); + setNavigationContext('/second', undefined); + setNavigationContext('/third', undefined); + + expect(getNavigationContext()?.targetPath).toBe('/third'); + }); + }); + + describe('stack overflow protection', () => { + it('should remove oldest context when stack exceeds limit', () => { + // Push 12 contexts (limit is 10) + const tokens: object[] = []; + for (let i = 0; i < 12; i++) { + tokens.push(setNavigationContext(`/path${i}`, undefined)); + } + + // Most recent should be /path11 + expect(getNavigationContext()?.targetPath).toBe('/path11'); + + // The oldest contexts (path0, path1) were evicted due to overflow + // Trying to clear them does nothing (their tokens no longer match anything) + clearNavigationContext(tokens[0]!); + clearNavigationContext(tokens[1]!); + + // /path11 should still be current + expect(getNavigationContext()?.targetPath).toBe('/path11'); + }); + }); + }); });