From 8bd10ffa9dd2925203bd0fac3e0e3cf043306c4c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 20 Jan 2026 10:27:35 +0000 Subject: [PATCH 1/2] fix(react): Prevent lazy route handlers from updating wrong navigation span --- .../tests/transactions.test.ts | 131 ++++++++++++++++++ .../instrumentation.tsx | 32 +++-- .../reactrouter-compat-utils/lazy-routes.tsx | 29 ++-- .../instrumentation.test.tsx | 52 +++++++ .../lazy-routes.test.ts | 92 ++++++++++-- 5 files changed, 309 insertions(+), 27 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts index f7a3ec4a5519..b45aabcc0678 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 @@ -1228,3 +1228,134 @@ test('Query/hash navigation does not corrupt transaction name', async ({ page }) const corruptedToRoot = navigationTransactions.filter(t => t.name === '/'); expect(corruptedToRoot.length).toBe(0); }); + +test('Captured navigation context is used instead of stale window.location during rapid navigation', async ({ + page, +}) => { + // Validates fix for race condition where captureCurrentLocation would use stale WINDOW.location. + // Navigate to slow route, then quickly to another route before lazy handler resolves. + await page.goto('/'); + + const allNavigationTransactions: Array<{ name: string; traceId: string }> = []; + + const collectorPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation') { + allNavigationTransactions.push({ + name: transactionEvent.transaction, + traceId: transactionEvent.contexts.trace.trace_id || '', + }); + } + return allNavigationTransactions.length >= 2; + }); + + const slowFetchLink = page.locator('id=navigation-to-slow-fetch'); + await expect(slowFetchLink).toBeVisible(); + await slowFetchLink.click(); + + // Navigate away quickly before slow-fetch's async handler resolves + await page.waitForTimeout(50); + + 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(2000); + + await Promise.race([ + collectorPromise, + new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 3000)), + ]).catch(() => {}); + + expect(allNavigationTransactions.length).toBeGreaterThanOrEqual(1); + + // /another-lazy transaction must have correct name (not corrupted by slow-fetch handler) + const anotherLazyTransaction = allNavigationTransactions.find( + t => + t.name.startsWith('/another-lazy/sub'), + ); + expect(anotherLazyTransaction).toBeDefined(); + + const corruptedToRoot = allNavigationTransactions.filter(t => t.name === '/'); + expect(corruptedToRoot.length).toBe(0); + + if (allNavigationTransactions.length >= 2) { + const uniqueNames = new Set(allNavigationTransactions.map(t => t.name)); + expect(uniqueNames.size).toBe(allNavigationTransactions.length); + } +}); + +test('Second navigation span is not corrupted by first slow lazy handler completing late', async ({ page }) => { + // Validates fix for race condition where slow lazy handler would update the wrong span. + // Navigate to slow route (which fetches /api/slow-data), then quickly to fast route. + // Without fix: second transaction gets wrong name and/or contains leaked spans. + + await page.goto('/'); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allNavigationTransactions: Array<{ name: string; traceId: string; spans: any[] }> = []; + + const collectorPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation') { + allNavigationTransactions.push({ + name: transactionEvent.transaction, + traceId: transactionEvent.contexts.trace.trace_id || '', + spans: transactionEvent.spans || [], + }); + } + return false; + }); + + // Navigate to slow-fetch (500ms lazy delay, fetches /api/slow-data) + const slowFetchLink = page.locator('id=navigation-to-slow-fetch'); + await expect(slowFetchLink).toBeVisible(); + await slowFetchLink.click(); + + // Wait 150ms (before 500ms lazy loading completes), then navigate away + await page.waitForTimeout(150); + + const anotherLink = page.locator('id=navigation-to-another'); + await anotherLink.click(); + + await expect(page.locator('id=another-lazy-route')).toBeVisible({ timeout: 10000 }); + + // Wait for slow-fetch lazy handler to complete and transactions to be sent + await page.waitForTimeout(2000); + + await Promise.race([ + collectorPromise, + new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 3000)), + ]).catch(() => {}); + + expect(allNavigationTransactions.length).toBeGreaterThanOrEqual(1); + + // /another-lazy transaction must have correct name, not "/slow-fetch/:id" + const anotherLazyTransaction = allNavigationTransactions.find( + t => + t.name.startsWith('/another-lazy/sub'), + ); + expect(anotherLazyTransaction).toBeDefined(); + + // Key assertion 2: /another-lazy transaction must NOT contain spans from /slow-fetch route + // The /api/slow-data fetch is triggered by the slow-fetch route's lazy loading + if (anotherLazyTransaction) { + const leakedSpans = anotherLazyTransaction.spans.filter( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (span: any) => span.description?.includes('slow-data') || span.data?.url?.includes('slow-data'), + ); + expect(leakedSpans.length).toBe(0); + } + + // Key assertion 3: If slow-fetch transaction exists, verify it has the correct name + // (not corrupted to /another-lazy) + const slowFetchTransaction = allNavigationTransactions.find(t => t.name.includes('slow-fetch')); + if (slowFetchTransaction) { + expect(slowFetchTransaction.name).toMatch(/\/slow-fetch/); + // Verify slow-fetch transaction doesn't contain spans that belong to /another-lazy + const wrongSpans = slowFetchTransaction.spans.filter( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (span: any) => span.description?.includes('another-lazy') || span.data?.url?.includes('another-lazy'), + ); + expect(wrongSpans.length).toBe(0); + } +}); diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index d646624618f9..a67d75a2bcf1 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -730,15 +730,20 @@ function wrapPatchRoutesOnNavigation( // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access (args as any).patch = (routeId: string, children: RouteObject[]) => { addRoutesToAllRoutes(children); - const currentActiveRootSpan = getActiveRootSpan(); - // Only update if we have a valid targetPath (patchRoutesOnNavigation can be called without path) + // Use the captured activeRootSpan instead of getActiveRootSpan() to avoid race conditions + // where user navigates away during lazy route loading and we'd update the wrong span + const spanJson = activeRootSpan ? spanToJSON(activeRootSpan) : undefined; + // Only update if we have a valid targetPath (patchRoutesOnNavigation can be called without path), + // the captured span exists, hasn't ended, and is a navigation span if ( targetPath && - currentActiveRootSpan && - (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation' + activeRootSpan && + spanJson && + !spanJson.timestamp && // Span hasn't ended yet + spanJson.op === 'navigation' ) { updateNavigationSpan( - currentActiveRootSpan, + activeRootSpan, { pathname: targetPath, search: '', hash: '', state: null, key: 'default' }, Array.from(allRoutes), true, @@ -760,13 +765,22 @@ function wrapPatchRoutesOnNavigation( clearNavigationContext(contextToken); } - const currentActiveRootSpan = getActiveRootSpan(); - if (currentActiveRootSpan && (spanToJSON(currentActiveRootSpan) as { op?: string }).op === 'navigation') { - const pathname = isMemoryRouter ? targetPath : targetPath || WINDOW.location?.pathname; + // Use the captured activeRootSpan instead of getActiveRootSpan() to avoid race conditions + // where user navigates away during lazy route loading and we'd update the wrong span + const spanJson = activeRootSpan ? spanToJSON(activeRootSpan) : undefined; + if ( + activeRootSpan && + spanJson && + !spanJson.timestamp && // Span hasn't ended yet + spanJson.op === 'navigation' + ) { + // Use targetPath consistently - don't fall back to WINDOW.location which may have changed + // if the user navigated away during async loading + const pathname = targetPath; if (pathname) { updateNavigationSpan( - currentActiveRootSpan, + activeRootSpan, { pathname, search: '', hash: '', state: null, key: 'default' }, Array.from(allRoutes), false, diff --git a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 0a4f838dfd17..f828b324ebed 100644 --- a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -8,19 +8,28 @@ import { getActiveRootSpan, getNavigationContext } from './utils'; /** * Captures location at invocation time. Prefers navigation context over window.location * since window.location hasn't updated yet when async handlers are invoked. + * + * When inside a patchRoutesOnNavigation call, uses the captured targetPath. If targetPath + * is undefined (patchRoutesOnNavigation can be invoked without a path argument), returns + * null rather than falling back to WINDOW.location which could be stale/wrong after the + * user navigated away during async loading. Returning null causes the span name update + * to be skipped, which is safer than using incorrect location data. */ function captureCurrentLocation(): Location | null { const navContext = getNavigationContext(); - // 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: '', - hash: '', - state: null, - key: 'default', - }; + + if (navContext) { + if (navContext.targetPath) { + return { + pathname: navContext.targetPath, + search: '', + hash: '', + state: null, + key: 'default', + }; + } + // Don't fall back to potentially stale WINDOW.location + return null; } if (typeof WINDOW !== 'undefined') { diff --git a/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx index 3d2b4f198cf5..956627be00c2 100644 --- a/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx +++ b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx @@ -1309,4 +1309,56 @@ describe('tryUpdateSpanNameBeforeEnd - source upgrade logic', () => { } }); }); + + describe('wrapPatchRoutesOnNavigation race condition fix', () => { + it('should use captured span instead of current active span in args.patch callback', () => { + const endedSpanJson = { + op: 'navigation', + timestamp: 1234567890, // Span has ended + }; + + vi.mocked(spanToJSON).mockReturnValue(endedSpanJson as any); + + const endedSpan = { + updateName: vi.fn(), + setAttribute: vi.fn(), + } as unknown as Span; + + updateNavigationSpan( + endedSpan, + { pathname: '/test', search: '', hash: '', state: null, key: 'test' }, + [], + false, + vi.fn(() => []), + ); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(endedSpan.updateName).not.toHaveBeenCalled(); + }); + + it('should not fall back to WINDOW.location.pathname after async operations', () => { + const validSpanJson = { + op: 'navigation', + timestamp: undefined, // Span hasn't ended + }; + + vi.mocked(spanToJSON).mockReturnValue(validSpanJson as any); + + const validSpan = { + updateName: vi.fn(), + setAttribute: vi.fn(), + } as unknown as Span; + + updateNavigationSpan( + validSpan, + { pathname: '/captured/path', search: '', hash: '', state: null, key: 'test' }, + [], + false, + vi.fn(() => []), + ); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(validSpan.updateName).toHaveBeenCalled(); + }); + }); }); 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 0d1a493e08f2..2122f64303bb 100644 --- a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts +++ b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts @@ -1,9 +1,13 @@ +import { WINDOW } from '@sentry/browser'; import { addNonEnumerableProperty, debug } from '@sentry/core'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { checkRouteForAsyncHandler, + clearNavigationContext, createAsyncHandlerProxy, + getNavigationContext, handleAsyncHandlerResult, + setNavigationContext, } from '../../src/reactrouter-compat-utils'; import type { RouteObject } from '../../src/types'; @@ -22,6 +26,21 @@ vi.mock('../../src/debug-build', () => ({ DEBUG_BUILD: true, })); +// Create a mutable mock for WINDOW.location that we can modify in tests +vi.mock('@sentry/browser', async requireActual => { + const actual = await requireActual(); + return { + ...(actual as any), + WINDOW: { + location: { + pathname: '/default', + search: '', + hash: '', + }, + }, + }; +}); + describe('reactrouter-compat-utils/lazy-routes', () => { let mockProcessResolvedRoutes: ReturnType; @@ -105,10 +124,12 @@ describe('reactrouter-compat-utils/lazy-routes', () => { proxy(); - // Since handleAsyncHandlerResult is called internally, we verify through its side effects - // 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); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith( + ['route1', 'route2'], + route, + expect.objectContaining({ pathname: '/default' }), // Falls back to WINDOW.location + undefined, + ); }); it('should handle functions that throw exceptions', () => { @@ -139,9 +160,12 @@ describe('reactrouter-compat-utils/lazy-routes', () => { const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); proxy(); - // 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); + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith( + [], + route, + expect.objectContaining({ pathname: '/default' }), // Falls back to WINDOW.location + undefined, + ); }); }); @@ -731,4 +755,56 @@ describe('reactrouter-compat-utils/lazy-routes', () => { expect(typeof route.handle!['with spaces']).toBe('function'); }); }); + + describe('captureCurrentLocation edge cases', () => { + afterEach(() => { + (WINDOW as any).location = { pathname: '/default', search: '', hash: '' }; + // Clean up any leaked navigation contexts + let ctx; + while ((ctx = getNavigationContext()) !== null) { + clearNavigationContext((ctx as any).token); + } + }); + + it('should use navigation context targetPath when defined', () => { + const token = setNavigationContext('/original-route', undefined); + (WINDOW as any).location = { pathname: '/different-route', search: '', hash: '' }; + + const originalFunction = vi.fn(() => [{ path: '/child' }]); + const route: RouteObject = { path: '/test' }; + + const proxy = createAsyncHandlerProxy(originalFunction, route, 'handler', mockProcessResolvedRoutes); + proxy(); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith( + [{ path: '/child' }], + route, + expect.objectContaining({ pathname: '/original-route' }), + undefined, + ); + + clearNavigationContext(token); + }); + + it('should not fall back to WINDOW.location when targetPath is undefined', () => { + // targetPath can be undefined when patchRoutesOnNavigation is called with args.path = undefined + const token = setNavigationContext(undefined, undefined); + (WINDOW as any).location = { pathname: '/wrong-route-from-window', search: '', hash: '' }; + + const originalFunction = vi.fn(() => [{ path: '/child' }]); + const route: RouteObject = { path: '/test' }; + + const proxy = createAsyncHandlerProxy(originalFunction, route, 'handler', mockProcessResolvedRoutes); + proxy(); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith( + [{ path: '/child' }], + route, + undefined, // Does not fall back to WINDOW.location + undefined, + ); + + clearNavigationContext(token); + }); + }); }); From 97659e9a478677349d7032d1e8f65c6612482afe Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 20 Jan 2026 11:23:23 +0000 Subject: [PATCH 2/2] Lint --- .../tests/transactions.test.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts index b45aabcc0678..e3a844bf814c 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 @@ -1270,10 +1270,7 @@ test('Captured navigation context is used instead of stale window.location durin expect(allNavigationTransactions.length).toBeGreaterThanOrEqual(1); // /another-lazy transaction must have correct name (not corrupted by slow-fetch handler) - const anotherLazyTransaction = allNavigationTransactions.find( - t => - t.name.startsWith('/another-lazy/sub'), - ); + const anotherLazyTransaction = allNavigationTransactions.find(t => t.name.startsWith('/another-lazy/sub')); expect(anotherLazyTransaction).toBeDefined(); const corruptedToRoot = allNavigationTransactions.filter(t => t.name === '/'); @@ -1330,10 +1327,7 @@ test('Second navigation span is not corrupted by first slow lazy handler complet expect(allNavigationTransactions.length).toBeGreaterThanOrEqual(1); // /another-lazy transaction must have correct name, not "/slow-fetch/:id" - const anotherLazyTransaction = allNavigationTransactions.find( - t => - t.name.startsWith('/another-lazy/sub'), - ); + const anotherLazyTransaction = allNavigationTransactions.find(t => t.name.startsWith('/another-lazy/sub')); expect(anotherLazyTransaction).toBeDefined(); // Key assertion 2: /another-lazy transaction must NOT contain spans from /slow-fetch route