From c0756919f24f5e293f86f98105a4398e8b8fa9ff Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 27 Nov 2025 13:12:51 +0000 Subject: [PATCH 1/6] fix(react): Add transaction name guards for rapid lazy-route navigations. --- .../react-router-7-lazy-routes/src/index.tsx | 8 + .../src/pages/DelayedLazyRoute.tsx | 8 + .../src/pages/Index.tsx | 4 + .../src/pages/SlowFetchLazyRoutes.tsx | 42 +++ .../tests/transactions.test.ts | 347 +++++++++++++++++- .../instrumentation.tsx | 18 + .../reactrouter-compat-utils/lazy-routes.tsx | 32 +- .../lazy-routes.test.ts | 70 ++-- 8 files changed, 496 insertions(+), 33 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/SlowFetchLazyRoutes.tsx 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/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 6e19b9021ba5..abbfedc9d113 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -265,6 +265,24 @@ export function processResolvedRoutes( } } + // If the user navigated away before this lazy handler resolved, skip updating the span. + if (currentLocation && typeof WINDOW !== 'undefined') { + try { + const windowLocation = WINDOW.location; + if (windowLocation) { + const capturedKey = computeLocationKey(currentLocation); + const currentKey = `${windowLocation.pathname}${windowLocation.search || ''}${windowLocation.hash || ''}`; + + if (currentKey !== capturedKey) { + return; + } + } + } catch { + DEBUG_BUILD && debug.warn('[React Router] Could not access window.location'); + return; + } + } + if (location) { if (spanOp === 'pageload') { // Re-run the pageload transaction update with the newly loaded routes diff --git a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 49923854554c..895580ecd178 100644 --- a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -1,7 +1,31 @@ +import { WINDOW } from '@sentry/browser'; import { addNonEnumerableProperty, debug, isThenable } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; import type { Location, RouteObject } from '../types'; +/** + * Captures the current location at the time of invocation. + */ +function captureCurrentLocation(): Location | null { + if (typeof WINDOW !== 'undefined') { + try { + const windowLocation = WINDOW.location; + if (windowLocation) { + return { + pathname: windowLocation.pathname, + search: windowLocation.search || '', + hash: windowLocation.hash || '', + state: null, + key: 'default', + }; + } + } catch { + DEBUG_BUILD && debug.warn('[React Router] Could not access window.location'); + } + } + return null; +} + /** * Creates a proxy wrapper for an async handler function. */ @@ -13,8 +37,9 @@ export function createAsyncHandlerProxy( ): (...args: unknown[]) => unknown { const proxy = new Proxy(originalFunction, { apply(target: (...args: unknown[]) => unknown, thisArg, argArray) { + const locationAtInvocation = captureCurrentLocation(); const result = target.apply(thisArg, argArray); - handleAsyncHandlerResult(result, route, handlerKey, processResolvedRoutes); + handleAsyncHandlerResult(result, route, handlerKey, processResolvedRoutes, locationAtInvocation); return result; }, }); @@ -32,19 +57,20 @@ export function handleAsyncHandlerResult( route: RouteObject, handlerKey: string, processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, + currentLocation: Location | null, ): void { if (isThenable(result)) { (result as Promise) .then((resolvedRoutes: unknown) => { if (Array.isArray(resolvedRoutes)) { - processResolvedRoutes(resolvedRoutes, route); + processResolvedRoutes(resolvedRoutes, route, currentLocation ?? undefined); } }) .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); } } 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..ceb07fbb3bd4 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,8 @@ 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) + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(['route1', 'route2'], route, undefined); }); it('should handle functions that throw exceptions', () => { @@ -137,35 +138,37 @@ 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) + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route, 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); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); }); it('should handle empty array results', () => { const routes: RouteObject[] = []; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); }); 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); // Wait for the promise to resolve await promiseResult; @@ -173,25 +176,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); }); 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); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); }); 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); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -202,7 +205,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); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -213,7 +216,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); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -224,7 +227,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); // Wait for the promise to be handled await new Promise(resolve => setTimeout(resolve, 0)); @@ -240,7 +243,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); await new Promise(resolve => setTimeout(resolve, 0)); @@ -253,25 +256,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); + handleAsyncHandlerResult(123, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult({ not: 'array' }, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(null, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(undefined, route, handlerKey, mockProcessResolvedRoutes, mockLocation); 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); + handleAsyncHandlerResult(false, route, handlerKey, mockProcessResolvedRoutes, mockLocation); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); it('should ignore functions as results', () => { const functionResult = () => 'test'; - handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -281,7 +284,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: 'not a function', }; - handleAsyncHandlerResult(fakeThenableButNotPromise, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(fakeThenableButNotPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -291,7 +294,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: null, }; - handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -306,12 +309,12 @@ 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); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute, mockLocation); }); it('should handle nested route objects in arrays', () => { @@ -322,9 +325,18 @@ describe('reactrouter-compat-utils/lazy-routes', () => { }, ]; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); + }); + + it('should pass null location when currentLocation is null', () => { + const routes: RouteObject[] = [{ path: '/route1' }]; + + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, null); + + // When null is passed, it should convert to undefined for processResolvedRoutes + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, undefined); }); }); From 62536640dcebf9db07dac4638f6f127783f7bb97 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 27 Nov 2025 15:17:39 +0000 Subject: [PATCH 2/6] Address Copilot comments --- .../react/src/reactrouter-compat-utils/instrumentation.tsx | 6 +++++- .../react/test/reactrouter-compat-utils/lazy-routes.test.ts | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index abbfedc9d113..454e2e4530d2 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -271,7 +271,11 @@ export function processResolvedRoutes( const windowLocation = WINDOW.location; if (windowLocation) { const capturedKey = computeLocationKey(currentLocation); - const currentKey = `${windowLocation.pathname}${windowLocation.search || ''}${windowLocation.hash || ''}`; + const currentKey = computeLocationKey({ + pathname: windowLocation.pathname, + search: windowLocation.search || '', + hash: windowLocation.hash || '', + }); if (currentKey !== capturedKey) { return; 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 ceb07fbb3bd4..8c9fd0476c26 100644 --- a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts +++ b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts @@ -330,7 +330,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); }); - it('should pass null location when currentLocation is null', () => { + it('should convert null location to undefined for processResolvedRoutes', () => { const routes: RouteObject[] = [{ path: '/route1' }]; handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, null); From 33c2c81dd22a6ad10db554cd6db61278812a49cf Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 27 Nov 2025 15:58:46 +0000 Subject: [PATCH 3/6] Do not skip updating the previous span --- .../instrumentation.tsx | 87 ++++++++----------- .../reactrouter-compat-utils/lazy-routes.tsx | 67 ++++++++++++-- .../src/reactrouter-compat-utils/utils.ts | 46 +++++++++- .../instrumentation.test.tsx | 1 + .../lazy-routes.test.ts | 78 ++++++++++------- 5 files changed, 191 insertions(+), 88 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 454e2e4530d2..b5978831697d 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,56 +257,44 @@ 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); + + // 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; - // Try to use the provided location first, then fall back to global window location if needed + // 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 }; } } } - // If the user navigated away before this lazy handler resolved, skip updating the span. - if (currentLocation && typeof WINDOW !== 'undefined') { - try { - const windowLocation = WINDOW.location; - if (windowLocation) { - const capturedKey = computeLocationKey(currentLocation); - const currentKey = computeLocationKey({ - pathname: windowLocation.pathname, - search: windowLocation.search || '', - hash: windowLocation.hash || '', - }); - - if (currentKey !== capturedKey) { - return; - } - } - } catch { - DEBUG_BUILD && debug.warn('[React Router] Could not access window.location'); - return; - } - } - if (location) { 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); } } } @@ -750,7 +746,14 @@ function wrapPatchRoutesOnNavigation( } const lazyLoadPromise = (async () => { - const result = await originalPatchRoutes(args); + // Set context so async handlers can access correct targetPath and span + setNavigationContext(targetPath, activeRootSpan); + let result; + try { + result = await originalPatchRoutes(args); + } finally { + clearNavigationContext(); + } const currentActiveRootSpan = getActiveRootSpan(); if (currentActiveRootSpan && (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation') { @@ -1206,17 +1209,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, locationAtInvocation); + handleAsyncHandlerResult( + result, + route, + handlerKey, + processResolvedRoutes, + locationAtInvocation, + spanAtInvocation, + ); return result; }, }); @@ -51,26 +92,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, currentLocation ?? undefined); + 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, currentLocation ?? undefined); + processResolvedRoutes(result, route, currentLocation ?? undefined, capturedSpan); } } @@ -79,7 +127,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..0c3c501bf214 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,10 +1,37 @@ -import type { TransactionSource } from '@sentry/core'; +import type { Span, TransactionSource } from '@sentry/core'; +import { getActiveSpan, getRootSpan, spanToJSON } from '@sentry/core'; 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. +// Uses a stack to handle overlapping navigations correctly (LIFO semantics). +interface NavigationContext { + targetPath: string; + span: Span | undefined; +} + +const _navigationContextStack: NavigationContext[] = []; + +/** Pushes a navigation context before invoking patchRoutesOnNavigation. */ +export function setNavigationContext(targetPath: string, span: Span | undefined): void { + _navigationContextStack.push({ targetPath, span }); +} + +/** Pops the most recent navigation context after patchRoutesOnNavigation completes. */ +export function clearNavigationContext(): void { + _navigationContextStack.pop(); +} + +/** Gets the current (most recent) navigation context if inside a patchRoutesOnNavigation call. */ +export function getNavigationContext(): NavigationContext | null { + const length = _navigationContextStack.length; + 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 +300,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 8c9fd0476c26..0d1a493e08f2 100644 --- a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts +++ b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts @@ -107,7 +107,8 @@ describe('reactrouter-compat-utils/lazy-routes', () => { // Since handleAsyncHandlerResult is called internally, we verify through its side effects // The third parameter is the captured location (undefined in jsdom test environment) - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(['route1', 'route2'], route, undefined); + // 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', () => { @@ -139,7 +140,8 @@ describe('reactrouter-compat-utils/lazy-routes', () => { proxy(); // The third parameter is the captured location (undefined in jsdom test environment) - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route, undefined); + // The fourth parameter is the captured span (undefined since no active span in test) + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route, undefined, undefined); }); }); @@ -151,24 +153,24 @@ describe('reactrouter-compat-utils/lazy-routes', () => { it('should handle array results directly', () => { const routes: RouteObject[] = [{ path: '/route1' }, { path: '/route2' }]; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation, undefined); }); it('should handle empty array results', () => { const routes: RouteObject[] = []; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); + 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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); // Wait for the promise to resolve await promiseResult; @@ -176,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, mockLocation); + 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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); + 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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -205,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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -216,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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); @@ -227,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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); // Wait for the promise to be handled await new Promise(resolve => setTimeout(resolve, 0)); @@ -243,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, mockLocation); + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); await new Promise(resolve => setTimeout(resolve, 0)); @@ -256,25 +258,25 @@ describe('reactrouter-compat-utils/lazy-routes', () => { }); it('should ignore non-promise, non-array results', () => { - handleAsyncHandlerResult('string result', route, handlerKey, mockProcessResolvedRoutes, mockLocation); - handleAsyncHandlerResult(123, route, handlerKey, mockProcessResolvedRoutes, mockLocation); - handleAsyncHandlerResult({ not: 'array' }, route, handlerKey, mockProcessResolvedRoutes, mockLocation); - handleAsyncHandlerResult(null, route, handlerKey, mockProcessResolvedRoutes, mockLocation); - handleAsyncHandlerResult(undefined, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + 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, mockLocation); - handleAsyncHandlerResult(false, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + 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, mockLocation); + handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -284,7 +286,14 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: 'not a function', }; - handleAsyncHandlerResult(fakeThenableButNotPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult( + fakeThenableButNotPromise, + route, + handlerKey, + mockProcessResolvedRoutes, + mockLocation, + undefined, + ); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -294,7 +303,7 @@ describe('reactrouter-compat-utils/lazy-routes', () => { then: null, }; - handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); }); @@ -309,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, mockLocation); + handleAsyncHandlerResult( + promiseResult, + complexRoute, + 'complexHandler', + mockProcessResolvedRoutes, + mockLocation, + undefined, + ); await promiseResult; await new Promise(resolve => setTimeout(resolve, 0)); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute, mockLocation); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute, mockLocation, undefined); }); it('should handle nested route objects in arrays', () => { @@ -325,18 +341,18 @@ describe('reactrouter-compat-utils/lazy-routes', () => { }, ]; - handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, mockLocation, undefined); - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); + 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); + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes, null, undefined); // When null is passed, it should convert to undefined for processResolvedRoutes - expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, undefined); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, undefined, undefined); }); }); From c7f9647c146ab0f3ec386a14a1de3a5c57b75ff9 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 28 Nov 2025 01:10:59 +0000 Subject: [PATCH 4/6] Add safety guards to navigation context stack --- .../instrumentation.tsx | 2 +- .../reactrouter-compat-utils/lazy-routes.tsx | 4 +++- .../src/reactrouter-compat-utils/utils.ts | 19 ++++++++++++++----- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index b5978831697d..daf518a5ea4d 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -752,7 +752,7 @@ function wrapPatchRoutesOnNavigation( try { result = await originalPatchRoutes(args); } finally { - clearNavigationContext(); + clearNavigationContext(activeRootSpan); } const currentActiveRootSpan = getActiveRootSpan(); diff --git a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 780c2d0ef4a1..0a4f838dfd17 100644 --- a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -11,7 +11,9 @@ import { getActiveRootSpan, getNavigationContext } from './utils'; */ function captureCurrentLocation(): Location | null { const navContext = getNavigationContext(); - if (navContext) { + // Only use navigation context if targetPath is defined (it can be undefined + // if patchRoutesOnNavigation was invoked without a path argument) + if (navContext?.targetPath) { return { pathname: navContext.targetPath, search: '', diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index 0c3c501bf214..eca921cd40e8 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -10,20 +10,29 @@ let _stripBasename: boolean = false; // Required because window.location hasn't updated yet when handlers are invoked. // Uses a stack to handle overlapping navigations correctly (LIFO semantics). interface NavigationContext { - targetPath: string; + targetPath: string | undefined; span: Span | undefined; } const _navigationContextStack: NavigationContext[] = []; +const MAX_CONTEXT_STACK_SIZE = 10; /** Pushes a navigation context before invoking patchRoutesOnNavigation. */ -export function setNavigationContext(targetPath: string, span: Span | undefined): void { +export function setNavigationContext(targetPath: string | undefined, span: Span | undefined): void { + // Prevent unbounded stack growth from cleanup failures or rapid navigations + if (_navigationContextStack.length >= MAX_CONTEXT_STACK_SIZE) { + _navigationContextStack.shift(); // Remove oldest + } _navigationContextStack.push({ targetPath, span }); } -/** Pops the most recent navigation context after patchRoutesOnNavigation completes. */ -export function clearNavigationContext(): void { - _navigationContextStack.pop(); +/** Pops the navigation context for the given span after patchRoutesOnNavigation completes. */ +export function clearNavigationContext(span: Span | undefined): void { + // Only pop if top of stack matches this span to prevent corruption from mismatched calls + const top = _navigationContextStack[_navigationContextStack.length - 1]; + if (top?.span === span) { + _navigationContextStack.pop(); + } } /** Gets the current (most recent) navigation context if inside a patchRoutesOnNavigation call. */ From 853d062a19a5218c6e375888df9e0b5413e378af Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 28 Nov 2025 11:30:50 +0000 Subject: [PATCH 5/6] Use token-based LIFO for navigation context cleanup --- .../src/reactrouter-compat-utils/index.ts | 5 + .../instrumentation.tsx | 4 +- .../src/reactrouter-compat-utils/utils.ts | 32 ++-- .../reactrouter-compat-utils/utils.test.ts | 138 +++++++++++++++++- 4 files changed, 165 insertions(+), 14 deletions(-) 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 daf518a5ea4d..234904b84771 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -747,12 +747,12 @@ function wrapPatchRoutesOnNavigation( const lazyLoadPromise = (async () => { // Set context so async handlers can access correct targetPath and span - setNavigationContext(targetPath, activeRootSpan); + const contextToken = setNavigationContext(targetPath, activeRootSpan); let result; try { result = await originalPatchRoutes(args); } finally { - clearNavigationContext(activeRootSpan); + clearNavigationContext(contextToken); } const currentActiveRootSpan = getActiveRootSpan(); diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index eca921cd40e8..caefa7e5f935 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,5 +1,6 @@ import type { Span, TransactionSource } from '@sentry/core'; -import { getActiveSpan, getRootSpan, spanToJSON } 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 @@ -8,8 +9,8 @@ let _stripBasename: boolean = false; // Navigation context stack for nested/concurrent patchRoutesOnNavigation calls. // Required because window.location hasn't updated yet when handlers are invoked. -// Uses a stack to handle overlapping navigations correctly (LIFO semantics). interface NavigationContext { + token: object; targetPath: string | undefined; span: Span | undefined; } @@ -17,20 +18,29 @@ interface NavigationContext { const _navigationContextStack: NavigationContext[] = []; const MAX_CONTEXT_STACK_SIZE = 10; -/** Pushes a navigation context before invoking patchRoutesOnNavigation. */ -export function setNavigationContext(targetPath: string | undefined, span: Span | undefined): void { - // Prevent unbounded stack growth from cleanup failures or rapid navigations +/** + * 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) { - _navigationContextStack.shift(); // Remove oldest + DEBUG_BUILD && debug.warn('[React Router] Navigation context stack overflow - removing oldest context'); + _navigationContextStack.shift(); } - _navigationContextStack.push({ targetPath, span }); + _navigationContextStack.push({ token, targetPath, span }); + return token; } -/** Pops the navigation context for the given span after patchRoutesOnNavigation completes. */ -export function clearNavigationContext(span: Span | undefined): void { - // Only pop if top of stack matches this span to prevent corruption from mismatched calls +/** + * 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?.span === span) { + if (top?.token === token) { _navigationContextStack.pop(); } } 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'); + }); + }); + }); }); From 8ae6a96d1afa03d2f272e1b0fc3c68d46d9dc589 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 28 Nov 2025 11:48:11 +0000 Subject: [PATCH 6/6] Address remaining PR review comments --- .../react/src/reactrouter-compat-utils/instrumentation.tsx | 7 ++++++- packages/react/src/reactrouter-compat-utils/utils.ts | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 234904b84771..d646624618f9 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -731,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' }, diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index caefa7e5f935..96c178b64c14 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -48,6 +48,7 @@ export function clearNavigationContext(token: object): void { /** 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; }