From 02e08c9affa66f9d3f7e8fa73838ca5d6d04fd29 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 21 Aug 2025 15:31:57 +0100 Subject: [PATCH 01/15] fix(react): Avoid multiple name updates on navigation spans --- .../react/src/reactrouterv6-compat-utils.tsx | 29 ++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index f364facca1db..7334f05b56c3 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -518,14 +518,30 @@ export function handleNavigation(opts: { } const activeSpan = getActiveSpan(); - const isAlreadyInNavigationSpan = activeSpan && spanToJSON(activeSpan).op === 'navigation'; + const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; + const isAlreadyInNavigationSpan = rootSpan && spanToJSON(rootSpan).op === 'navigation'; // Cross usage can result in multiple navigation spans being created without this check if (isAlreadyInNavigationSpan) { - activeSpan?.updateName(name); - activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + // Check if we've already set the name for this span using a custom property + const hasBeenNamed = ( + rootSpan as { + __sentry_navigation_name_set__?: boolean; + } + )?.__sentry_navigation_name_set__; + if (!hasBeenNamed) { + // This is the first time we're setting the name for this span + const spanJson = spanToJSON(rootSpan); + if (!spanJson.timestamp) { + rootSpan?.updateName(name); + rootSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + } + // Mark this span as having its name set to prevent future updates + addNonEnumerableProperty(rootSpan, '__sentry_navigation_name_set__', true); + } + // If we already have a name for this span, don't update it } else { - startBrowserTracingNavigationSpan(client, { + const span = startBrowserTracingNavigationSpan(client, { name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, @@ -533,6 +549,11 @@ export function handleNavigation(opts: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, }, }); + + // Mark the new span as having its name set + if (span) { + addNonEnumerableProperty(span, '__sentry_navigation_name_set__', true); + } } } } From 86f2aef0d52631a9d2f183b01a914194c98fd6cd Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 21 Aug 2025 15:51:57 +0100 Subject: [PATCH 02/15] Update `activeSpan` instead of `rootSpan` --- .../react/src/reactrouterv6-compat-utils.tsx | 15 ++++++++------- .../test/reactrouter-cross-usage.test.tsx | 19 ------------------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 7334f05b56c3..78300ee4d2dc 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -518,27 +518,28 @@ export function handleNavigation(opts: { } const activeSpan = getActiveSpan(); - const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; - const isAlreadyInNavigationSpan = rootSpan && spanToJSON(rootSpan).op === 'navigation'; + const isAlreadyInNavigationSpan = activeSpan && spanToJSON(activeSpan).op === 'navigation'; // Cross usage can result in multiple navigation spans being created without this check if (isAlreadyInNavigationSpan) { // Check if we've already set the name for this span using a custom property const hasBeenNamed = ( - rootSpan as { + activeSpan as { __sentry_navigation_name_set__?: boolean; } )?.__sentry_navigation_name_set__; if (!hasBeenNamed) { // This is the first time we're setting the name for this span - const spanJson = spanToJSON(rootSpan); + const spanJson = spanToJSON(activeSpan); if (!spanJson.timestamp) { - rootSpan?.updateName(name); - rootSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + activeSpan?.updateName(name); } + // Mark this span as having its name set to prevent future updates - addNonEnumerableProperty(rootSpan, '__sentry_navigation_name_set__', true); + addNonEnumerableProperty(activeSpan, '__sentry_navigation_name_set__', true); } + + activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); // If we already have a name for this span, don't update it } else { const span = startBrowserTracingNavigationSpan(client, { diff --git a/packages/react/test/reactrouter-cross-usage.test.tsx b/packages/react/test/reactrouter-cross-usage.test.tsx index 57f9b4ce00cf..76cfe59f3df5 100644 --- a/packages/react/test/reactrouter-cross-usage.test.tsx +++ b/packages/react/test/reactrouter-cross-usage.test.tsx @@ -288,11 +288,6 @@ describe('React Router cross usage of wrappers', () => { // It's called 1 time from the wrapped `MemoryRouter` expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - - // It's called 3 times from the 3 `useRoutes` components - expect(mockNavigationSpan.updateName).toHaveBeenCalledTimes(3); - expect(mockNavigationSpan.updateName).toHaveBeenLastCalledWith('/second-level/:id/third-level/:id'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { name: '/second-level/:id/third-level/:id', attributes: { @@ -459,11 +454,6 @@ describe('React Router cross usage of wrappers', () => { // It's called 1 time from the wrapped `MemoryRouter` expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - - // It's called 3 times from the 3 `useRoutes` components - expect(mockNavigationSpan.updateName).toHaveBeenCalledTimes(3); - expect(mockNavigationSpan.updateName).toHaveBeenLastCalledWith('/second-level/:id/third-level/:id'); - expect(mockNavigationSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); }); @@ -611,10 +601,6 @@ describe('React Router cross usage of wrappers', () => { // It's called 1 time from the wrapped `createMemoryRouter` expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - // It's called 3 times from the 3 `SentryRoutes` components - expect(mockNavigationSpan.updateName).toHaveBeenCalledTimes(3); - expect(mockNavigationSpan.updateName).toHaveBeenLastCalledWith('/second-level/:id/third-level/:id'); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { name: '/second-level/:id/third-level/:id', attributes: { @@ -790,11 +776,6 @@ describe('React Router cross usage of wrappers', () => { // It's called 1 time from the wrapped `MemoryRouter` expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - - // It's called 3 times from the 2 `useRoutes` components and 1 component - expect(mockNavigationSpan.updateName).toHaveBeenCalledTimes(3); - - expect(mockNavigationSpan.updateName).toHaveBeenLastCalledWith('/second-level/:id/third-level/:id'); expect(mockNavigationSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); }); From 51dd07d9645c9caa477b76704c2dfb65753777dc Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Aug 2025 01:32:01 +0100 Subject: [PATCH 03/15] Reimplement --- .../react-router-7-lazy-routes/src/index.tsx | 6 + .../src/pages/AnotherLazyRoutes.tsx | 41 ++ .../src/pages/Index.tsx | 8 + .../src/pages/InnerLazyRoutes.tsx | 13 +- .../tests/transactions.test.ts | 108 ++++ packages/react/src/lazy-route-utils.tsx | 141 +++++ .../react/src/reactrouterv6-compat-utils.tsx | 482 ++++++++++++------ .../test/reactrouterv6-compat-utils.test.tsx | 52 +- 8 files changed, 678 insertions(+), 173 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/AnotherLazyRoutes.tsx create mode 100644 packages/react/src/lazy-route-utils.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 4570c23d06f5..c6c3cf74a3c7 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 @@ -50,6 +50,12 @@ const router = sentryCreateBrowserRouter( lazyChildren: () => import('./pages/InnerLazyRoutes').then(module => module.someMoreNestedRoutes), }, }, + { + path: '/another-lazy', + handle: { + lazyChildren: () => import('./pages/AnotherLazyRoutes').then(module => module.anotherNestedRoutes), + }, + }, { path: '/static', element: <>Hello World, diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/AnotherLazyRoutes.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/AnotherLazyRoutes.tsx new file mode 100644 index 000000000000..9d6818a64398 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/AnotherLazyRoutes.tsx @@ -0,0 +1,41 @@ +import React from 'react'; +import { Link } from 'react-router-dom'; + +export const anotherNestedRoutes = [ + { + path: 'sub', + children: [ + { + index: true, + element: ( +
+ Another Lazy Route + + Navigate to Inner Lazy Route + +
+ ), + }, + { + path: ':id', + children: [ + { + index: true, + element:
Another Lazy Route with ID
, + }, + { + path: ':subId', + element: ( +
+ Another Deep Lazy Route + + Navigate to Inner from Deep + +
+ ), + }, + ], + }, + ], + }, +]; 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 e24b1b7cbff7..aefa39d63811 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 @@ -7,6 +7,14 @@ const Index = () => { navigate +
+ + Navigate to Another Lazy Route + +
+ + Navigate to Another Deep Lazy Route + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx index e8385e54dab5..42324bb09391 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx @@ -1,4 +1,5 @@ import React from 'react'; +import { Link } from 'react-router-dom'; export const someMoreNestedRoutes = [ { @@ -24,9 +25,15 @@ export const someMoreNestedRoutes = [ }, { path: ':someAnotherId', - element:
- Rendered -
, + element: ( +
+ Rendered +
+ + Navigate to Another Lazy Route + +
+ ), }, ], }, 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 74a1fcff1faa..240e16a1cf95 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 @@ -54,3 +54,111 @@ test('Creates a navigation transaction inside a lazy route', async ({ page }) => expect(event.type).toBe('transaction'); expect(event.contexts?.trace?.op).toBe('navigation'); }); + +test('Creates navigation transactions between two different lazy routes', async ({ page }) => { + // First, navigate to the "another-lazy" route + const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/another-lazy/sub/:id/:subId' + ); + }); + + await page.goto('/'); + + // Navigate to another lazy route first + const navigationToAnotherDeep = page.locator('id=navigation-to-another-deep'); + await expect(navigationToAnotherDeep).toBeVisible(); + await navigationToAnotherDeep.click(); + + const firstEvent = await firstTransactionPromise; + + // Check if the first lazy route content is rendered + const anotherLazyContent = page.locator('id=another-lazy-route-deep'); + await expect(anotherLazyContent).toBeVisible(); + + // Validate the first transaction event + expect(firstEvent.transaction).toBe('/another-lazy/sub/:id/:subId'); + expect(firstEvent.type).toBe('transaction'); + expect(firstEvent.contexts?.trace?.op).toBe('navigation'); + + // Now navigate from the first lazy route to the second lazy route + const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' + ); + }); + + // Click the navigation link from within the first lazy route to the second lazy route + const navigationToInnerFromDeep = page.locator('id=navigate-to-inner-from-deep'); + await expect(navigationToInnerFromDeep).toBeVisible(); + await navigationToInnerFromDeep.click(); + + const secondEvent = await secondTransactionPromise; + + // Check if the second lazy route content is rendered + const innerLazyContent = page.locator('id=innermost-lazy-route'); + await expect(innerLazyContent).toBeVisible(); + + // Validate the second transaction event + expect(secondEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId'); + expect(secondEvent.type).toBe('transaction'); + expect(secondEvent.contexts?.trace?.op).toBe('navigation'); +}); + +test('Creates navigation transactions from inner lazy route to another lazy route', async ({ page }) => { + // First, navigate to the inner lazy route + const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' + ); + }); + + await page.goto('/'); + + // Navigate to inner lazy route first + const navigationToInner = page.locator('id=navigation'); + await expect(navigationToInner).toBeVisible(); + await navigationToInner.click(); + + const firstEvent = await firstTransactionPromise; + + // Check if the inner lazy route content is rendered + const innerLazyContent = page.locator('id=innermost-lazy-route'); + await expect(innerLazyContent).toBeVisible(); + + // Validate the first transaction event + expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId'); + expect(firstEvent.type).toBe('transaction'); + expect(firstEvent.contexts?.trace?.op).toBe('navigation'); + + // Now navigate from the inner lazy route to another lazy route + const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => { + return ( + !!transactionEvent?.transaction && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.transaction === '/another-lazy/sub/:id/:subId' + ); + }); + + // Click the navigation link from within the inner lazy route to another lazy route + const navigationToAnotherFromInner = page.locator('id=navigate-to-another-from-inner'); + await expect(navigationToAnotherFromInner).toBeVisible(); + await navigationToAnotherFromInner.click(); + + const secondEvent = await secondTransactionPromise; + + // Check if the another lazy route content is rendered + const anotherLazyContent = page.locator('id=another-lazy-route-deep'); + await expect(anotherLazyContent).toBeVisible(); + + // Validate the second transaction event + expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId'); + expect(secondEvent.type).toBe('transaction'); + expect(secondEvent.contexts?.trace?.op).toBe('navigation'); +}); diff --git a/packages/react/src/lazy-route-utils.tsx b/packages/react/src/lazy-route-utils.tsx new file mode 100644 index 000000000000..23c8c4f1e23e --- /dev/null +++ b/packages/react/src/lazy-route-utils.tsx @@ -0,0 +1,141 @@ +import type { Span, TransactionSource } from '@sentry/core'; +import { addNonEnumerableProperty, debug, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; +import { DEBUG_BUILD } from './debug-build'; +import { resolveRouteNameAndSource } from './reactrouterv6-compat-utils'; +import type { Location, MatchRoutes, RouteMatch, RouteObject } from './types'; + +/** + * Updates a navigation span with the correct route name after lazy routes have been loaded. + */ +export function updateNavigationSpanWithLazyRoutes( + activeRootSpan: Span, + location: Location, + allRoutes: RouteObject[], + forceUpdate = false, + matchRoutes: MatchRoutes, + rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, + locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, + getNormalizedName: ( + routes: RouteObject[], + location: Location, + branches: RouteMatch[], + basename?: string, + ) => [string, TransactionSource], + prefixWithSlash: (path: string) => string, +): void { + // Check if this span has already been named to avoid multiple updates + // But allow updates if this is a forced update (e.g., when lazy routes are loaded) + const hasBeenNamed = + !forceUpdate && + ( + activeRootSpan as { + __sentry_navigation_name_set__?: boolean; + } + )?.__sentry_navigation_name_set__; + + if (!hasBeenNamed) { + // Get fresh branches for the current location with all loaded routes + const currentBranches = matchRoutes(allRoutes, location); + const [name, source] = resolveRouteNameAndSource( + location, + allRoutes, + allRoutes, + (currentBranches as RouteMatch[]) || [], + '', + locationIsInsideDescendantRoute, + rebuildRoutePathFromAllRoutes, + getNormalizedName, + prefixWithSlash, + ); + + // Only update if we have a valid name and the span hasn't finished + const spanJson = spanToJSON(activeRootSpan); + if (name && !spanJson.timestamp) { + activeRootSpan.updateName(name); + activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + + // Mark this span as having its name set to prevent future updates + addNonEnumerableProperty( + activeRootSpan as { __sentry_navigation_name_set__?: boolean }, + '__sentry_navigation_name_set__', + true, + ); + } + } +} + +/** + * Creates a proxy wrapper for an async handler function. + */ +export function createAsyncHandlerProxy( + originalFunction: (...args: unknown[]) => unknown, + route: RouteObject, + handlerKey: string, + processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, +): (...args: unknown[]) => unknown { + const proxy = new Proxy(originalFunction, { + apply(target: (...args: unknown[]) => unknown, thisArg, argArray) { + const result = target.apply(thisArg, argArray); + handleAsyncHandlerResult(result, route, handlerKey, processResolvedRoutes); + return result; + }, + }); + + addNonEnumerableProperty(proxy, '__sentry_proxied__', true); + + return proxy; +} + +/** + * Handles the result of an async handler function call. + */ +export function handleAsyncHandlerResult( + result: unknown, + route: RouteObject, + handlerKey: string, + processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, +): void { + if ( + result && + typeof result === 'object' && + 'then' in result && + typeof (result as Promise).then === 'function' + ) { + (result as Promise) + .then((resolvedRoutes: unknown) => { + if (Array.isArray(resolvedRoutes)) { + processResolvedRoutes(resolvedRoutes, route); + } + }) + .catch((e: unknown) => { + DEBUG_BUILD && debug.warn(`Error resolving async handler '${handlerKey}' for route`, route, e); + }); + } else if (Array.isArray(result)) { + processResolvedRoutes(result, route); + } +} + +/** + * Recursively checks a route for async handlers and sets up Proxies to add discovered child routes to allRoutes when called. + */ +export function checkRouteForAsyncHandler( + route: RouteObject, + processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, +): void { + // Set up proxies for any functions in the route's handle + if (route.handle && typeof route.handle === 'object') { + for (const key of Object.keys(route.handle)) { + const maybeFn = route.handle[key]; + if (typeof maybeFn === 'function' && !(maybeFn as { __sentry_proxied__?: boolean }).__sentry_proxied__) { + route.handle[key] = createAsyncHandlerProxy(maybeFn, route, key, processResolvedRoutes); + } + } + } + + // Recursively check child routes + if (Array.isArray(route.children)) { + for (const child of route.children) { + checkRouteForAsyncHandler(child, processResolvedRoutes); + } + } +} diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 78300ee4d2dc..3bbfeb7a70da 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -6,7 +6,6 @@ import { browserTracingIntegration, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, - WINDOW, } from '@sentry/browser'; import type { Client, Integration, Span, TransactionSource } from '@sentry/core'; import { @@ -16,6 +15,7 @@ import { getClient, getCurrentScope, getRootSpan, + GLOBAL_OBJ, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -24,6 +24,7 @@ import { import * as React from 'react'; import { DEBUG_BUILD } from './debug-build'; import { hoistNonReactStatics } from './hoist-non-react-statics'; +import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-route-utils'; import type { Action, AgnosticDataRouteMatch, @@ -51,6 +52,31 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); +/** + * Gets the current location from the global object (window in browser environments). + * Returns undefined if global location is not available. + */ +function getGlobalLocation(): Location | undefined { + if (typeof GLOBAL_OBJ !== 'undefined') { + const globalLocation = (GLOBAL_OBJ as typeof GLOBAL_OBJ & Window).location; + if (globalLocation) { + return { pathname: globalLocation.pathname }; + } + } + return undefined; +} + +/** + * Gets the pathname from the global object (window in browser environments). + * Returns undefined if global location is not available. + */ +function getGlobalPathname(): string | undefined { + if (typeof GLOBAL_OBJ !== 'undefined') { + return (GLOBAL_OBJ as typeof GLOBAL_OBJ & Window).location?.pathname; + } + return undefined; +} + export interface ReactRouterOptions { useEffect: UseEffect; useLocation: UseLocation; @@ -78,61 +104,19 @@ type V6CompatibleVersion = '6' | '7'; // Keeping as a global variable for cross-usage in multiple functions const allRoutes = new Set(); -/** - * Adds resolved routes as children to the parent route. - * Prevents duplicate routes by checking if they already exist. - */ -function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { - const existingChildren = parentRoute.children || []; - - const newRoutes = resolvedRoutes.filter( - newRoute => - !existingChildren.some( - existing => - existing === newRoute || - (newRoute.path && existing.path === newRoute.path) || - (newRoute.id && existing.id === newRoute.id), - ), - ); - - if (newRoutes.length > 0) { - parentRoute.children = [...existingChildren, ...newRoutes]; - } -} - -/** - * Handles the result of an async handler function call. - */ -function handleAsyncHandlerResult(result: unknown, route: RouteObject, handlerKey: string): void { - if ( - result && - typeof result === 'object' && - 'then' in result && - typeof (result as Promise).then === 'function' - ) { - (result as Promise) - .then((resolvedRoutes: unknown) => { - if (Array.isArray(resolvedRoutes)) { - processResolvedRoutes(resolvedRoutes, route); - } - }) - .catch((e: unknown) => { - DEBUG_BUILD && debug.warn(`Error resolving async handler '${handlerKey}' for route`, route, e); - }); - } else if (Array.isArray(result)) { - processResolvedRoutes(result, route); - } -} - /** * Processes resolved routes by adding them to allRoutes and checking for nested async handlers. */ -function processResolvedRoutes(resolvedRoutes: RouteObject[], parentRoute?: RouteObject): void { +function processResolvedRoutes( + resolvedRoutes: RouteObject[], + parentRoute?: RouteObject, + currentLocation?: Location, +): void { resolvedRoutes.forEach(child => { allRoutes.add(child); // Only check for async handlers if the feature is enabled if (_enableAsyncRouteHandlers) { - checkRouteForAsyncHandler(child); + checkRouteForAsyncHandler(child, processResolvedRoutes); } }); @@ -141,65 +125,127 @@ function processResolvedRoutes(resolvedRoutes: RouteObject[], parentRoute?: Rout addResolvedRoutesToParent(resolvedRoutes, parentRoute); } - // After processing lazy routes, check if we need to update an active pageload transaction + // After processing lazy routes, check if we need to update an active transaction const activeRootSpan = getActiveRootSpan(); - if (activeRootSpan && spanToJSON(activeRootSpan).op === 'pageload') { - const location = WINDOW.location; + if (activeRootSpan) { + const spanOp = spanToJSON(activeRootSpan).op; + + // Try to use the provided location first, then fall back to global window location if needed + let location = currentLocation; + if (!location) { + location = getGlobalLocation(); + } + if (location) { - // Re-run the pageload transaction update with the newly loaded routes - updatePageloadTransaction( - activeRootSpan, - { pathname: location.pathname }, - Array.from(allRoutes), - undefined, - undefined, - Array.from(allRoutes), - ); + if (spanOp === 'pageload') { + // Re-run the pageload transaction update with the newly loaded routes + updatePageloadTransaction( + activeRootSpan, + { pathname: location.pathname }, + Array.from(allRoutes), + undefined, + undefined, + Array.from(allRoutes), + ); + } else if (spanOp === 'navigation') { + // For navigation spans, update the name with the newly loaded routes + updateNavigationSpanWithLazyRoutesLocal(activeRootSpan, location, Array.from(allRoutes)); + } } } } /** - * Creates a proxy wrapper for an async handler function. + * Local wrapper for updateNavigationSpanWithLazyRoutes that provides dependencies. */ -function createAsyncHandlerProxy( - originalFunction: (...args: unknown[]) => unknown, - route: RouteObject, - handlerKey: string, -): (...args: unknown[]) => unknown { - const proxy = new Proxy(originalFunction, { - apply(target: (...args: unknown[]) => unknown, thisArg, argArray) { - const result = target.apply(thisArg, argArray); - handleAsyncHandlerResult(result, route, handlerKey); - return result; - }, - }); +function updateNavigationSpanWithLazyRoutesLocal( + activeRootSpan: Span, + location: Location, + allRoutes: RouteObject[], + forceUpdate = false, +): void { + updateNavigationSpanWithLazyRoutes( + activeRootSpan, + location, + allRoutes, + forceUpdate, + _matchRoutes, + rebuildRoutePathFromAllRoutes, + locationIsInsideDescendantRoute, + getNormalizedName, + prefixWithSlash, + ); +} - addNonEnumerableProperty(proxy, '__sentry_proxied__', true); +function wrapPatchRoutesOnNavigationLocal( + opts: Record | undefined, + isMemoryRouter = false, +): Record { + if (!opts || !('patchRoutesOnNavigation' in opts) || typeof opts.patchRoutesOnNavigation !== 'function') { + return opts || {}; + } - return proxy; -} + const originalPatchRoutes = opts.patchRoutesOnNavigation; + return { + ...opts, + patchRoutesOnNavigation: async (args: unknown) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access + const targetPath = (args as any)?.path; + + // For browser router, wrap the patch function to update span during patching + if (!isMemoryRouter) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access + const originalPatch = (args as any)?.patch; + if (originalPatch) { + // 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 activeRootSpan = getActiveRootSpan(); + if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { + updateNavigationSpanWithLazyRoutesLocal( + activeRootSpan, + { + pathname: targetPath, + search: '', + hash: '', + state: null, + key: 'default', + }, + Array.from(allRoutes), + true, + ); + } + return originalPatch(routeId, children); + }; + } + } -/** - * Recursively checks a route for async handlers and sets up Proxies to add discovered child routes to allRoutes when called. - */ -export function checkRouteForAsyncHandler(route: RouteObject): void { - // Set up proxies for any functions in the route's handle - if (route.handle && typeof route.handle === 'object') { - for (const key of Object.keys(route.handle)) { - const maybeFn = route.handle[key]; - if (typeof maybeFn === 'function' && !(maybeFn as { __sentry_proxied__?: boolean }).__sentry_proxied__) { - route.handle[key] = createAsyncHandlerProxy(maybeFn, route, key); + const result = await originalPatchRoutes(args); + + // Update navigation span after routes are patched + const activeRootSpan = getActiveRootSpan(); + if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { + // For memory routers, we don't have a reliable way to get the current pathname + // without accessing window.location, so we'll use targetPath for both cases + const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); + if (pathname) { + updateNavigationSpanWithLazyRoutesLocal( + activeRootSpan, + { + pathname, + search: '', + hash: '', + state: null, + key: 'default', + }, + Array.from(allRoutes), + ); + } } - } - } - // Recursively check child routes - if (Array.isArray(route.children)) { - for (const child of route.children) { - checkRouteForAsyncHandler(child); - } - } + return result; + }, + }; } /** @@ -227,11 +273,14 @@ export function createV6CompatibleWrapCreateBrowserRouter< // Check for async handlers that might contain sub-route declarations (only if enabled) if (_enableAsyncRouteHandlers) { for (const route of routes) { - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, processResolvedRoutes); } } - const router = createRouterFunction(routes, opts); + // Wrap patchRoutesOnNavigation to detect when lazy routes are loaded + const wrappedOpts = wrapPatchRoutesOnNavigationLocal(opts); + + const router = createRouterFunction(routes, wrappedOpts); const basename = opts?.basename; const activeRootSpan = getActiveRootSpan(); @@ -313,11 +362,14 @@ export function createV6CompatibleWrapCreateMemoryRouter< // Check for async handlers that might contain sub-route declarations (only if enabled) if (_enableAsyncRouteHandlers) { for (const route of routes) { - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, processResolvedRoutes); } } - const router = createRouterFunction(routes, opts); + // Wrap patchRoutesOnNavigation to detect when lazy routes are loaded + const wrappedOpts = wrapPatchRoutesOnNavigationLocal(opts, true); + + const router = createRouterFunction(routes, wrappedOpts); const basename = opts?.basename; const activeRootSpan = getActiveRootSpan(); @@ -404,7 +456,7 @@ export function createReactRouterV6CompatibleTracingIntegration( afterAllSetup(client) { integration.afterAllSetup(client); - const initPathName = WINDOW.location?.pathname; + const initPathName = getGlobalPathname(); if (instrumentPageLoad && initPathName) { startBrowserTracingPageLoadSpan(client, { name: initPathName, @@ -504,57 +556,27 @@ export function handleNavigation(opts: { } if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) { - let name, - source: TransactionSource = 'url'; - const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes || routes); - - if (isInDescendantRoute) { - name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); - source = 'route'; - } - - if (!isInDescendantRoute || !name) { - [name, source] = getNormalizedName(routes, location, branches, basename); - } + const [name, source, isLikelyLazyRoute] = resolveRouteName( + location, + routes, + allRoutes || routes, + branches, + basename, + locationIsInsideDescendantRoute, + rebuildRoutePathFromAllRoutes, + getNormalizedName, + prefixWithSlash, + ); const activeSpan = getActiveSpan(); - const isAlreadyInNavigationSpan = activeSpan && spanToJSON(activeSpan).op === 'navigation'; + const spanJson = activeSpan && spanToJSON(activeSpan); + const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; // Cross usage can result in multiple navigation spans being created without this check - if (isAlreadyInNavigationSpan) { - // Check if we've already set the name for this span using a custom property - const hasBeenNamed = ( - activeSpan as { - __sentry_navigation_name_set__?: boolean; - } - )?.__sentry_navigation_name_set__; - if (!hasBeenNamed) { - // This is the first time we're setting the name for this span - const spanJson = spanToJSON(activeSpan); - if (!spanJson.timestamp) { - activeSpan?.updateName(name); - } - - // Mark this span as having its name set to prevent future updates - addNonEnumerableProperty(activeSpan, '__sentry_navigation_name_set__', true); - } - - activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); - // If we already have a name for this span, don't update it + if (isAlreadyInNavigationSpan && activeSpan && spanJson) { + handleExistingNavigationSpan(activeSpan, spanJson, name, source, isLikelyLazyRoute); } else { - const span = startBrowserTracingNavigationSpan(client, { - name, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, - }, - }); - - // Mark the new span as having its name set - if (span) { - addNonEnumerableProperty(span, '__sentry_navigation_name_set__', true); - } + createNewNavigationSpan(client, name, source, version, isLikelyLazyRoute); } } } @@ -873,6 +895,174 @@ function getActiveRootSpan(): Span | undefined { return op === 'navigation' || op === 'pageload' ? rootSpan : undefined; } +/** + * Shared helper function to resolve route name and source + */ +export function resolveRouteNameAndSource( + location: Location, + routes: RouteObject[], + allRoutes: RouteObject[], + branches: RouteMatch[], + basename: string = '', + locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, + rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, + getNormalizedName: ( + routes: RouteObject[], + location: Location, + branches: RouteMatch[], + basename?: string, + ) => [string, TransactionSource], + prefixWithSlash: (path: string) => string, +): [string, TransactionSource] { + let name: string | undefined; + let source: TransactionSource = 'url'; + + const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes); + + if (isInDescendantRoute) { + name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes, location)); + source = 'route'; + } + + if (!isInDescendantRoute || !name) { + [name, source] = getNormalizedName(routes, location, branches, basename); + } + + return [name || location.pathname, source]; +} + +/** + * Resolves the route name and source for navigation tracking + */ +export function resolveRouteName( + location: Location, + routes: RouteObject[], + allRoutes: RouteObject[], + branches: unknown[], + basename: string | undefined, + locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, + rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, + getNormalizedName: ( + routes: RouteObject[], + location: Location, + branches: RouteMatch[], + basename?: string, + ) => [string, TransactionSource], + prefixWithSlash: (path: string) => string, +): [string, TransactionSource, boolean] { + const [name, source] = resolveRouteNameAndSource( + location, + routes, + allRoutes || routes, + branches as RouteMatch[], + basename, + locationIsInsideDescendantRoute, + rebuildRoutePathFromAllRoutes, + getNormalizedName, + prefixWithSlash, + ); + + // If we couldn't find a good route name, it might be because we're navigating to a lazy route + // that hasn't been loaded yet. In this case, use the pathname as a fallback + const isLikelyLazyRoute = source === 'url' && (branches as unknown[]).length === 0; + let finalName = name; + let finalSource = source; + + if (isLikelyLazyRoute) { + finalName = location.pathname; + finalSource = 'url'; + } + + return [finalName, finalSource, isLikelyLazyRoute]; +} + +/** + * Handles updating an existing navigation span + */ +export function handleExistingNavigationSpan( + activeSpan: Span, + spanJson: ReturnType, + name: string, + source: TransactionSource, + isLikelyLazyRoute: boolean, +): void { + // Check if we've already set the name for this span using a custom property + const hasBeenNamed = ( + activeSpan as { + __sentry_navigation_name_set__?: boolean; + } + )?.__sentry_navigation_name_set__; + + if (!hasBeenNamed) { + // This is the first time we're setting the name for this span + if (!spanJson.timestamp) { + activeSpan?.updateName(name); + } + + // For lazy routes, don't mark as named yet so it can be updated later + if (!isLikelyLazyRoute) { + addNonEnumerableProperty( + activeSpan as { __sentry_navigation_name_set__?: boolean }, + '__sentry_navigation_name_set__', + true, + ); + } + } + + activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); +} + +/** + * Creates a new navigation span + */ +export function createNewNavigationSpan( + client: Client, + name: string, + source: TransactionSource, + version: string, + isLikelyLazyRoute: boolean, +): void { + const newSpan = startBrowserTracingNavigationSpan(client, { + name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, + }, + }); + + // For lazy routes, don't mark as named yet so it can be updated later when the route loads + if (!isLikelyLazyRoute && newSpan) { + addNonEnumerableProperty( + newSpan as { __sentry_navigation_name_set__?: boolean }, + '__sentry_navigation_name_set__', + true, + ); + } +} + +/** + * Adds resolved routes as children to the parent route. + * Prevents duplicate routes by checking if they already exist. + */ +export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { + const existingChildren = parentRoute.children || []; + + const newRoutes = resolvedRoutes.filter( + newRoute => + !existingChildren.some( + existing => + existing === newRoute || + (newRoute.path && existing.path === newRoute.path) || + (newRoute.id && existing.id === newRoute.id), + ), + ); + + if (newRoutes.length > 0) { + parentRoute.children = [...existingChildren, ...newRoutes]; + } +} + /** * Returns number of URL segments of a passed string URL. */ diff --git a/packages/react/test/reactrouterv6-compat-utils.test.tsx b/packages/react/test/reactrouterv6-compat-utils.test.tsx index 1f10d89c8558..de5d4fa74871 100644 --- a/packages/react/test/reactrouterv6-compat-utils.test.tsx +++ b/packages/react/test/reactrouterv6-compat-utils.test.tsx @@ -1,7 +1,11 @@ import { describe, expect, it, test } from 'vitest'; -import { checkRouteForAsyncHandler, getNumberOfUrlSegments } from '../src/reactrouterv6-compat-utils'; +import { checkRouteForAsyncHandler } from '../src/lazy-route-utils'; +import { getNumberOfUrlSegments } from '../src/reactrouterv6-compat-utils'; import type { RouteObject } from '../src/types'; +// Mock processResolvedRoutes function for tests +const mockProcessResolvedRoutes = () => {}; + describe('getNumberOfUrlSegments', () => { test.each([ ['regular path', '/projects/123/views/234', 4], @@ -23,8 +27,8 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route); - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); const proxiedHandler = route.handle?.lazyChildren; expect(typeof proxiedHandler).toBe('function'); @@ -41,7 +45,7 @@ describe('checkRouteForAsyncHandler', () => { path: '/test', }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with non-function handle properties', () => { @@ -52,7 +56,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with null/undefined handle properties', () => { @@ -61,7 +65,7 @@ describe('checkRouteForAsyncHandler', () => { handle: null as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with mixed function and non-function handle properties', () => { @@ -75,7 +79,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); const proxiedHandler = route.handle?.lazyChildren; expect(typeof proxiedHandler).toBe('function'); @@ -106,7 +110,7 @@ describe('checkRouteForAsyncHandler', () => { ], }; - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); // Check parent handler is proxied const proxiedParentHandler = route.handle?.lazyChildren; @@ -149,7 +153,7 @@ describe('checkRouteForAsyncHandler', () => { ], }; - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); // Check all handlers are proxied expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -175,7 +179,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); // Check all handlers are proxied expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -193,13 +197,13 @@ describe('checkRouteForAsyncHandler', () => { }; // First call should proxy the function - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); const firstProxiedHandler = route.handle?.lazyChildren; expect(firstProxiedHandler).not.toBe(mockHandler); expect((firstProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); // Second call should not create a new proxy - checkRouteForAsyncHandler(route); + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); const secondProxiedHandler = route.handle?.lazyChildren; expect(secondProxiedHandler).toBe(firstProxiedHandler); // Should be the same proxy expect((secondProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -211,7 +215,7 @@ describe('checkRouteForAsyncHandler', () => { children: [], }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with undefined children', () => { @@ -220,7 +224,7 @@ describe('checkRouteForAsyncHandler', () => { children: undefined, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with null children', () => { @@ -229,7 +233,7 @@ describe('checkRouteForAsyncHandler', () => { children: null as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with non-array children', () => { @@ -238,7 +242,7 @@ describe('checkRouteForAsyncHandler', () => { children: 'not an array' as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is not an object', () => { @@ -247,7 +251,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 'not an object' as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is null', () => { @@ -256,7 +260,7 @@ describe('checkRouteForAsyncHandler', () => { handle: null as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is undefined', () => { @@ -265,7 +269,7 @@ describe('checkRouteForAsyncHandler', () => { handle: undefined as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a function', () => { @@ -274,7 +278,7 @@ describe('checkRouteForAsyncHandler', () => { handle: (() => {}) as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a string', () => { @@ -283,7 +287,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 'string handle' as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a number', () => { @@ -292,7 +296,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 42 as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a boolean', () => { @@ -301,7 +305,7 @@ describe('checkRouteForAsyncHandler', () => { handle: true as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is an array', () => { @@ -310,6 +314,6 @@ describe('checkRouteForAsyncHandler', () => { handle: [] as any, }; - expect(() => checkRouteForAsyncHandler(route)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); }); }); From 42365cb2b1365fbb6e3e67b9c27bab62e92c14d3 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Aug 2025 14:39:23 +0100 Subject: [PATCH 04/15] Clean up --- packages/react/src/lazy-route-utils.tsx | 15 +- .../react/src/reactrouterv6-compat-utils.tsx | 135 +++-------- packages/react/src/reactrouterv6-utils.ts | 215 ++++++++++++++++++ 3 files changed, 251 insertions(+), 114 deletions(-) create mode 100644 packages/react/src/reactrouterv6-utils.ts diff --git a/packages/react/src/lazy-route-utils.tsx b/packages/react/src/lazy-route-utils.tsx index 23c8c4f1e23e..426505cfdf84 100644 --- a/packages/react/src/lazy-route-utils.tsx +++ b/packages/react/src/lazy-route-utils.tsx @@ -1,4 +1,4 @@ -import type { Span, TransactionSource } from '@sentry/core'; +import type { Span } from '@sentry/core'; import { addNonEnumerableProperty, debug, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; import { DEBUG_BUILD } from './debug-build'; import { resolveRouteNameAndSource } from './reactrouterv6-compat-utils'; @@ -13,15 +13,6 @@ export function updateNavigationSpanWithLazyRoutes( allRoutes: RouteObject[], forceUpdate = false, matchRoutes: MatchRoutes, - rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, - locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, - getNormalizedName: ( - routes: RouteObject[], - location: Location, - branches: RouteMatch[], - basename?: string, - ) => [string, TransactionSource], - prefixWithSlash: (path: string) => string, ): void { // Check if this span has already been named to avoid multiple updates // But allow updates if this is a forced update (e.g., when lazy routes are loaded) @@ -42,10 +33,6 @@ export function updateNavigationSpanWithLazyRoutes( allRoutes, (currentBranches as RouteMatch[]) || [], '', - locationIsInsideDescendantRoute, - rebuildRoutePathFromAllRoutes, - getNormalizedName, - prefixWithSlash, ); // Only update if we have a valid name and the span hasn't finished diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 3bbfeb7a70da..c41752ac849a 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -6,6 +6,7 @@ import { browserTracingIntegration, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, + WINDOW, } from '@sentry/browser'; import type { Client, Integration, Span, TransactionSource } from '@sentry/core'; import { @@ -15,7 +16,6 @@ import { getClient, getCurrentScope, getRootSpan, - GLOBAL_OBJ, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -25,6 +25,9 @@ import * as React from 'react'; import { DEBUG_BUILD } from './debug-build'; import { hoistNonReactStatics } from './hoist-non-react-statics'; import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-route-utils'; +import { + initializeRouterUtils, +} from './reactrouterv6-utils'; import type { Action, AgnosticDataRouteMatch, @@ -53,12 +56,12 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); /** - * Gets the current location from the global object (window in browser environments). - * Returns undefined if global location is not available. + * Gets the current location from the window object in browser environments. + * Returns undefined if window is not available. */ function getGlobalLocation(): Location | undefined { - if (typeof GLOBAL_OBJ !== 'undefined') { - const globalLocation = (GLOBAL_OBJ as typeof GLOBAL_OBJ & Window).location; + if (typeof WINDOW !== 'undefined') { + const globalLocation = WINDOW.location; if (globalLocation) { return { pathname: globalLocation.pathname }; } @@ -67,12 +70,12 @@ function getGlobalLocation(): Location | undefined { } /** - * Gets the pathname from the global object (window in browser environments). - * Returns undefined if global location is not available. + * Gets the pathname from the window object in browser environments. + * Returns undefined if window is not available. */ function getGlobalPathname(): string | undefined { - if (typeof GLOBAL_OBJ !== 'undefined') { - return (GLOBAL_OBJ as typeof GLOBAL_OBJ & Window).location?.pathname; + if (typeof WINDOW !== 'undefined') { + return WINDOW.location?.pathname; } return undefined; } @@ -149,35 +152,19 @@ function processResolvedRoutes( ); } else if (spanOp === 'navigation') { // For navigation spans, update the name with the newly loaded routes - updateNavigationSpanWithLazyRoutesLocal(activeRootSpan, location, Array.from(allRoutes)); + updateNavigationSpanWithLazyRoutes( + activeRootSpan, + location, + Array.from(allRoutes), + false, + _matchRoutes, + ); } } } } -/** - * Local wrapper for updateNavigationSpanWithLazyRoutes that provides dependencies. - */ -function updateNavigationSpanWithLazyRoutesLocal( - activeRootSpan: Span, - location: Location, - allRoutes: RouteObject[], - forceUpdate = false, -): void { - updateNavigationSpanWithLazyRoutes( - activeRootSpan, - location, - allRoutes, - forceUpdate, - _matchRoutes, - rebuildRoutePathFromAllRoutes, - locationIsInsideDescendantRoute, - getNormalizedName, - prefixWithSlash, - ); -} - -function wrapPatchRoutesOnNavigationLocal( +function wrapPatchRoutesOnNavigation( opts: Record | undefined, isMemoryRouter = false, ): Record { @@ -202,7 +189,7 @@ function wrapPatchRoutesOnNavigationLocal( addRoutesToAllRoutes(children); const activeRootSpan = getActiveRootSpan(); if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { - updateNavigationSpanWithLazyRoutesLocal( + updateNavigationSpanWithLazyRoutes( activeRootSpan, { pathname: targetPath, @@ -213,6 +200,7 @@ function wrapPatchRoutesOnNavigationLocal( }, Array.from(allRoutes), true, + _matchRoutes, ); } return originalPatch(routeId, children); @@ -229,7 +217,7 @@ function wrapPatchRoutesOnNavigationLocal( // without accessing window.location, so we'll use targetPath for both cases const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); if (pathname) { - updateNavigationSpanWithLazyRoutesLocal( + updateNavigationSpanWithLazyRoutes( activeRootSpan, { pathname, @@ -239,6 +227,8 @@ function wrapPatchRoutesOnNavigationLocal( key: 'default', }, Array.from(allRoutes), + false, + _matchRoutes, ); } } @@ -278,7 +268,7 @@ export function createV6CompatibleWrapCreateBrowserRouter< } // Wrap patchRoutesOnNavigation to detect when lazy routes are loaded - const wrappedOpts = wrapPatchRoutesOnNavigationLocal(opts); + const wrappedOpts = wrapPatchRoutesOnNavigation(opts); const router = createRouterFunction(routes, wrappedOpts); const basename = opts?.basename; @@ -367,7 +357,7 @@ export function createV6CompatibleWrapCreateMemoryRouter< } // Wrap patchRoutesOnNavigation to detect when lazy routes are loaded - const wrappedOpts = wrapPatchRoutesOnNavigationLocal(opts, true); + const wrappedOpts = wrapPatchRoutesOnNavigation(opts, true); const router = createRouterFunction(routes, wrappedOpts); const basename = opts?.basename; @@ -452,6 +442,9 @@ export function createReactRouterV6CompatibleTracingIntegration( _createRoutesFromChildren = createRoutesFromChildren; _stripBasename = stripBasename || false; _enableAsyncRouteHandlers = enableAsyncRouteHandlers; + + // Initialize the router utils with the required dependencies + initializeRouterUtils(matchRoutes, stripBasename || false); }, afterAllSetup(client) { integration.afterAllSetup(client); @@ -556,16 +549,12 @@ export function handleNavigation(opts: { } if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) { - const [name, source, isLikelyLazyRoute] = resolveRouteName( + const [name, source] = resolveRouteNameAndSource( location, routes, allRoutes || routes, - branches, + branches as RouteMatch[], basename, - locationIsInsideDescendantRoute, - rebuildRoutePathFromAllRoutes, - getNormalizedName, - prefixWithSlash, ); const activeSpan = getActiveSpan(); @@ -574,9 +563,9 @@ export function handleNavigation(opts: { // Cross usage can result in multiple navigation spans being created without this check if (isAlreadyInNavigationSpan && activeSpan && spanJson) { - handleExistingNavigationSpan(activeSpan, spanJson, name, source, isLikelyLazyRoute); + handleExistingNavigationSpan(activeSpan, spanJson, name, source, false); } else { - createNewNavigationSpan(client, name, source, version, isLikelyLazyRoute); + createNewNavigationSpan(client, name, source, version, false); } } } @@ -784,7 +773,7 @@ function getNormalizedName( const fallbackTransactionName = _stripBasename ? stripBasenameFromPathname(location.pathname, basename) - : location.pathname || '/'; + : location.pathname; return [fallbackTransactionName, 'url']; } @@ -904,15 +893,6 @@ export function resolveRouteNameAndSource( allRoutes: RouteObject[], branches: RouteMatch[], basename: string = '', - locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, - rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, - getNormalizedName: ( - routes: RouteObject[], - location: Location, - branches: RouteMatch[], - basename?: string, - ) => [string, TransactionSource], - prefixWithSlash: (path: string) => string, ): [string, TransactionSource] { let name: string | undefined; let source: TransactionSource = 'url'; @@ -931,51 +911,6 @@ export function resolveRouteNameAndSource( return [name || location.pathname, source]; } -/** - * Resolves the route name and source for navigation tracking - */ -export function resolveRouteName( - location: Location, - routes: RouteObject[], - allRoutes: RouteObject[], - branches: unknown[], - basename: string | undefined, - locationIsInsideDescendantRoute: (location: Location, routes: RouteObject[]) => boolean, - rebuildRoutePathFromAllRoutes: (allRoutes: RouteObject[], location: Location) => string, - getNormalizedName: ( - routes: RouteObject[], - location: Location, - branches: RouteMatch[], - basename?: string, - ) => [string, TransactionSource], - prefixWithSlash: (path: string) => string, -): [string, TransactionSource, boolean] { - const [name, source] = resolveRouteNameAndSource( - location, - routes, - allRoutes || routes, - branches as RouteMatch[], - basename, - locationIsInsideDescendantRoute, - rebuildRoutePathFromAllRoutes, - getNormalizedName, - prefixWithSlash, - ); - - // If we couldn't find a good route name, it might be because we're navigating to a lazy route - // that hasn't been loaded yet. In this case, use the pathname as a fallback - const isLikelyLazyRoute = source === 'url' && (branches as unknown[]).length === 0; - let finalName = name; - let finalSource = source; - - if (isLikelyLazyRoute) { - finalName = location.pathname; - finalSource = 'url'; - } - - return [finalName, finalSource, isLikelyLazyRoute]; -} - /** * Handles updating an existing navigation span */ diff --git a/packages/react/src/reactrouterv6-utils.ts b/packages/react/src/reactrouterv6-utils.ts new file mode 100644 index 000000000000..7001a27f8274 --- /dev/null +++ b/packages/react/src/reactrouterv6-utils.ts @@ -0,0 +1,215 @@ +import type { TransactionSource } 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; + +/** + * Initialize function to set dependencies that the router utilities need. + * Must be called before using any of the exported utility functions. + */ +export function initializeRouterUtils(matchRoutes: MatchRoutes, stripBasename: boolean = false): void { + _matchRoutes = matchRoutes; + _stripBasename = stripBasename; +} + +// Helper functions +function pickPath(match: RouteMatch): string { + return trimWildcard(match.route.path || ''); +} + +function pickSplat(match: RouteMatch): string { + return match.params['*'] || ''; +} + +function trimWildcard(path: string): string { + return path[path.length - 1] === '*' ? path.slice(0, -1) : path; +} + +function trimSlash(path: string): string { + return path[path.length - 1] === '/' ? path.slice(0, -1) : path; +} + +function pathEndsWithWildcard(path: string): boolean { + return path.endsWith('*'); +} + +function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): boolean { + return (pathEndsWithWildcard(path) && !!branch.route.children?.length) || false; +} + +function routeIsDescendant(route: RouteObject): boolean { + return !!(!route.children && route.element && route.path?.endsWith('/*')); +} + +function sendIndexPath(pathBuilder: string, pathname: string, basename: string): [string, TransactionSource] { + const reconstructedPath = pathBuilder || _stripBasename ? stripBasenameFromPathname(pathname, basename) : pathname; + + const formattedPath = + // If the path ends with a slash, remove it + reconstructedPath[reconstructedPath.length - 1] === '/' + ? reconstructedPath.slice(0, -1) + : // If the path ends with a wildcard, remove it + reconstructedPath.slice(-2) === '/*' + ? reconstructedPath.slice(0, -1) + : reconstructedPath; + + return [formattedPath, 'route']; +} + +function getNumberOfUrlSegments(url: string): number { + // split at '/' or at '\/' to split regex urls correctly + return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; +} + +/** + * Strip the basename from a pathname if exists. + * + * Vendored and modified from `react-router` + * https://github.com/remix-run/react-router/blob/462bb712156a3f739d6139a0f14810b76b002df6/packages/router/utils.ts#L1038 + */ +function stripBasenameFromPathname(pathname: string, basename: string): string { + if (!basename || basename === '/') { + return pathname; + } + + if (!pathname.toLowerCase().startsWith(basename.toLowerCase())) { + return pathname; + } + + // We want to leave trailing slash behavior in the user's control, so if they + // specify a basename with a trailing slash, we should support it + const startIndex = basename.endsWith('/') ? basename.length - 1 : basename.length; + const nextChar = pathname.charAt(startIndex); + if (nextChar && nextChar !== '/') { + // pathname does not start with basename/ + return pathname; + } + + return pathname.slice(startIndex) || '/'; +} + +// Exported utility functions + +/** + * Ensures a path string starts with a forward slash. + */ +export function prefixWithSlash(path: string): string { + return path[0] === '/' ? path : `/${path}`; +} + +/** + * Rebuilds the route path from all available routes by matching against the current location. + */ +export function rebuildRoutePathFromAllRoutes(allRoutes: RouteObject[], location: Location): string { + const matchedRoutes = _matchRoutes(allRoutes, location) as RouteMatch[]; + + if (!matchedRoutes || matchedRoutes.length === 0) { + return ''; + } + + for (const match of matchedRoutes) { + if (match.route.path && match.route.path !== '*') { + const path = pickPath(match); + const strippedPath = stripBasenameFromPathname(location.pathname, prefixWithSlash(match.pathnameBase)); + + if (location.pathname === strippedPath) { + return trimSlash(strippedPath); + } + + return trimSlash( + trimSlash(path || '') + + prefixWithSlash( + rebuildRoutePathFromAllRoutes( + allRoutes.filter(route => route !== match.route), + { + pathname: strippedPath, + }, + ), + ), + ); + } + } + + return ''; +} + +/** + * Checks if the current location is inside a descendant route (route with splat parameter). + */ +export function locationIsInsideDescendantRoute(location: Location, routes: RouteObject[]): boolean { + const matchedRoutes = _matchRoutes(routes, location) as RouteMatch[]; + + if (matchedRoutes) { + for (const match of matchedRoutes) { + if (routeIsDescendant(match.route) && pickSplat(match)) { + return true; + } + } + } + + return false; +} + +/** + * Gets a normalized route name and transaction source from the current routes and location. + */ +export function getNormalizedName( + routes: RouteObject[], + location: Location, + branches: RouteMatch[], + basename: string = '', +): [string, TransactionSource] { + if (!routes || routes.length === 0) { + return [_stripBasename ? stripBasenameFromPathname(location.pathname, basename) : location.pathname, 'url']; + } + + let pathBuilder = ''; + + if (branches) { + for (const branch of branches) { + const route = branch.route; + if (route) { + // Early return if index route + if (route.index) { + return sendIndexPath(pathBuilder, branch.pathname, basename); + } + const path = route.path; + + // If path is not a wildcard and has no child routes, append the path + if (path && !pathIsWildcardAndHasChildren(path, branch)) { + const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; + pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); + + // If the path matches the current location, return the path + if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { + if ( + // If the route defined on the element is something like + // Product} /> + // We should check against the branch.pathname for the number of / separators + getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) && + // We should not count wildcard operators in the url segments calculation + !pathEndsWithWildcard(pathBuilder) + ) { + return [(_stripBasename ? '' : basename) + newPath, 'route']; + } + + // if the last character of the pathbuilder is a wildcard and there are children, remove the wildcard + if (pathIsWildcardAndHasChildren(pathBuilder, branch)) { + pathBuilder = pathBuilder.slice(0, -1); + } + + return [(_stripBasename ? '' : basename) + pathBuilder, 'route']; + } + } + } + } + } + + const fallbackTransactionName = _stripBasename + ? stripBasenameFromPathname(location.pathname, basename) + : location.pathname; + + return [fallbackTransactionName, 'url']; +} From b10e5b62694800e4d8b3485dab60724c31f46dd5 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Aug 2025 15:27:11 +0100 Subject: [PATCH 05/15] Move under folder --- .../src/reactrouter-compat-utils/index.ts | 33 +++ .../instrumentation.tsx} | 244 +----------------- .../lazy-routes.tsx} | 6 +- .../utils.ts} | 39 ++- packages/react/src/reactrouterv6.tsx | 4 +- packages/react/src/reactrouterv7.tsx | 4 +- 6 files changed, 89 insertions(+), 241 deletions(-) create mode 100644 packages/react/src/reactrouter-compat-utils/index.ts rename packages/react/src/{reactrouterv6-compat-utils.tsx => reactrouter-compat-utils/instrumentation.tsx} (76%) rename packages/react/src/{lazy-route-utils.tsx => reactrouter-compat-utils/lazy-routes.tsx} (96%) rename packages/react/src/{reactrouterv6-utils.ts => reactrouter-compat-utils/utils.ts} (87%) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts new file mode 100644 index 000000000000..7d01a5a48eea --- /dev/null +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -0,0 +1,33 @@ +// Main exports from instrumentation +export type { ReactRouterOptions } from './instrumentation'; +export { + createReactRouterV6CompatibleTracingIntegration, + createV6CompatibleWithSentryReactRouterRouting, + createV6CompatibleWrapCreateBrowserRouter, + createV6CompatibleWrapCreateMemoryRouter, + createV6CompatibleWrapUseRoutes, + handleNavigation, + handleExistingNavigationSpan, + createNewNavigationSpan, + addResolvedRoutesToParent, +} from './instrumentation'; + +// Utility exports +export { + initializeRouterUtils, + prefixWithSlash, + rebuildRoutePathFromAllRoutes, + locationIsInsideDescendantRoute, + getNormalizedName, + resolveRouteNameAndSource, + pathEndsWithWildcard, + pathIsWildcardAndHasChildren, +} from './utils'; + +// Lazy route exports +export { + updateNavigationSpanWithLazyRoutes, + createAsyncHandlerProxy, + handleAsyncHandlerResult, + checkRouteForAsyncHandler, +} from './lazy-routes'; diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx similarity index 76% rename from packages/react/src/reactrouterv6-compat-utils.tsx rename to packages/react/src/reactrouter-compat-utils/instrumentation.tsx index c41752ac849a..e6ad17595802 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -22,12 +22,8 @@ import { spanToJSON, } from '@sentry/core'; import * as React from 'react'; -import { DEBUG_BUILD } from './debug-build'; -import { hoistNonReactStatics } from './hoist-non-react-statics'; -import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-route-utils'; -import { - initializeRouterUtils, -} from './reactrouterv6-utils'; +import { DEBUG_BUILD } from '../debug-build'; +import { hoistNonReactStatics } from '../hoist-non-react-statics'; import type { Action, AgnosticDataRouteMatch, @@ -43,14 +39,22 @@ import type { UseLocation, UseNavigationType, UseRoutes, -} from './types'; +} from '../types'; +import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-routes'; +import { + getNormalizedName, + initializeRouterUtils, + locationIsInsideDescendantRoute, + prefixWithSlash, + rebuildRoutePathFromAllRoutes, + resolveRouteNameAndSource, +} from './utils'; let _useEffect: UseEffect; let _useLocation: UseLocation; let _useNavigationType: UseNavigationType; let _createRoutesFromChildren: CreateRoutesFromChildren; let _matchRoutes: MatchRoutes; -let _stripBasename: boolean = false; let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); @@ -152,13 +156,7 @@ function processResolvedRoutes( ); } else if (spanOp === 'navigation') { // For navigation spans, update the name with the newly loaded routes - updateNavigationSpanWithLazyRoutes( - activeRootSpan, - location, - Array.from(allRoutes), - false, - _matchRoutes, - ); + updateNavigationSpanWithLazyRoutes(activeRootSpan, location, Array.from(allRoutes), false, _matchRoutes); } } } @@ -440,7 +438,6 @@ export function createReactRouterV6CompatibleTracingIntegration( _useNavigationType = useNavigationType; _matchRoutes = matchRoutes; _createRoutesFromChildren = createRoutesFromChildren; - _stripBasename = stripBasename || false; _enableAsyncRouteHandlers = enableAsyncRouteHandlers; // Initialize the router utils with the required dependencies @@ -570,74 +567,6 @@ export function handleNavigation(opts: { } } -/** - * Strip the basename from a pathname if exists. - * - * Vendored and modified from `react-router` - * https://github.com/remix-run/react-router/blob/462bb712156a3f739d6139a0f14810b76b002df6/packages/router/utils.ts#L1038 - */ -function stripBasenameFromPathname(pathname: string, basename: string): string { - if (!basename || basename === '/') { - return pathname; - } - - if (!pathname.toLowerCase().startsWith(basename.toLowerCase())) { - return pathname; - } - - // We want to leave trailing slash behavior in the user's control, so if they - // specify a basename with a trailing slash, we should support it - const startIndex = basename.endsWith('/') ? basename.length - 1 : basename.length; - const nextChar = pathname.charAt(startIndex); - if (nextChar && nextChar !== '/') { - // pathname does not start with basename/ - return pathname; - } - - return pathname.slice(startIndex) || '/'; -} - -function sendIndexPath(pathBuilder: string, pathname: string, basename: string): [string, TransactionSource] { - const reconstructedPath = pathBuilder || _stripBasename ? stripBasenameFromPathname(pathname, basename) : pathname; - - const formattedPath = - // If the path ends with a slash, remove it - reconstructedPath[reconstructedPath.length - 1] === '/' - ? reconstructedPath.slice(0, -1) - : // If the path ends with a wildcard, remove it - reconstructedPath.slice(-2) === '/*' - ? reconstructedPath.slice(0, -1) - : reconstructedPath; - - return [formattedPath, 'route']; -} - -function pathEndsWithWildcard(path: string): boolean { - return path.endsWith('*'); -} - -function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): boolean { - return (pathEndsWithWildcard(path) && !!branch.route.children?.length) || false; -} - -function routeIsDescendant(route: RouteObject): boolean { - return !!(!route.children && route.element && route.path?.endsWith('/*')); -} - -function locationIsInsideDescendantRoute(location: Location, routes: RouteObject[]): boolean { - const matchedRoutes = _matchRoutes(routes, location) as RouteMatch[]; - - if (matchedRoutes) { - for (const match of matchedRoutes) { - if (routeIsDescendant(match.route) && pickSplat(match)) { - return true; - } - } - } - - return false; -} - function addRoutesToAllRoutes(routes: RouteObject[]): void { routes.forEach(route => { const extractedChildRoutes = getChildRoutesRecursively(route); @@ -666,118 +595,6 @@ function getChildRoutesRecursively(route: RouteObject, allRoutes: Set route !== match.route), - { - pathname: strippedPath, - }, - ), - ), - ); - } - } - - return ''; -} - -function getNormalizedName( - routes: RouteObject[], - location: Location, - branches: RouteMatch[], - basename: string = '', -): [string, TransactionSource] { - if (!routes || routes.length === 0) { - return [_stripBasename ? stripBasenameFromPathname(location.pathname, basename) : location.pathname, 'url']; - } - - let pathBuilder = ''; - - if (branches) { - for (const branch of branches) { - const route = branch.route; - if (route) { - // Early return if index route - if (route.index) { - return sendIndexPath(pathBuilder, branch.pathname, basename); - } - const path = route.path; - - // If path is not a wildcard and has no child routes, append the path - if (path && !pathIsWildcardAndHasChildren(path, branch)) { - const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; - pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); - - // If the path matches the current location, return the path - if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { - if ( - // If the route defined on the element is something like - // Product} /> - // We should check against the branch.pathname for the number of / separators - getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) && - // We should not count wildcard operators in the url segments calculation - !pathEndsWithWildcard(pathBuilder) - ) { - return [(_stripBasename ? '' : basename) + newPath, 'route']; - } - - // if the last character of the pathbuilder is a wildcard and there are children, remove the wildcard - if (pathIsWildcardAndHasChildren(pathBuilder, branch)) { - pathBuilder = pathBuilder.slice(0, -1); - } - - return [(_stripBasename ? '' : basename) + pathBuilder, 'route']; - } - } - } - } - } - - const fallbackTransactionName = _stripBasename - ? stripBasenameFromPathname(location.pathname, basename) - : location.pathname; - - return [fallbackTransactionName, 'url']; -} - function updatePageloadTransaction( activeRootSpan: Span | undefined, location: Location, @@ -884,33 +701,6 @@ function getActiveRootSpan(): Span | undefined { return op === 'navigation' || op === 'pageload' ? rootSpan : undefined; } -/** - * Shared helper function to resolve route name and source - */ -export function resolveRouteNameAndSource( - location: Location, - routes: RouteObject[], - allRoutes: RouteObject[], - branches: RouteMatch[], - basename: string = '', -): [string, TransactionSource] { - let name: string | undefined; - let source: TransactionSource = 'url'; - - const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes); - - if (isInDescendantRoute) { - name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes, location)); - source = 'route'; - } - - if (!isInDescendantRoute || !name) { - [name, source] = getNormalizedName(routes, location, branches, basename); - } - - return [name || location.pathname, source]; -} - /** * Handles updating an existing navigation span */ @@ -997,11 +787,3 @@ export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentR parentRoute.children = [...existingChildren, ...newRoutes]; } } - -/** - * Returns number of URL segments of a passed string URL. - */ -export function getNumberOfUrlSegments(url: string): number { - // split at '/' or at '\/' to split regex urls correctly - return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; -} diff --git a/packages/react/src/lazy-route-utils.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx similarity index 96% rename from packages/react/src/lazy-route-utils.tsx rename to packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 426505cfdf84..79ab7fc7e23e 100644 --- a/packages/react/src/lazy-route-utils.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -1,8 +1,8 @@ import type { Span } from '@sentry/core'; import { addNonEnumerableProperty, debug, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; -import { DEBUG_BUILD } from './debug-build'; -import { resolveRouteNameAndSource } from './reactrouterv6-compat-utils'; -import type { Location, MatchRoutes, RouteMatch, RouteObject } from './types'; +import { DEBUG_BUILD } from '../debug-build'; +import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; +import { resolveRouteNameAndSource } from './utils'; /** * Updates a navigation span with the correct route name after lazy routes have been loaded. diff --git a/packages/react/src/reactrouterv6-utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts similarity index 87% rename from packages/react/src/reactrouterv6-utils.ts rename to packages/react/src/reactrouter-compat-utils/utils.ts index 7001a27f8274..8e6b8b8e8807 100644 --- a/packages/react/src/reactrouterv6-utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,5 +1,5 @@ import type { TransactionSource } from '@sentry/core'; -import type { Location, MatchRoutes, RouteMatch, RouteObject } from './types'; +import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; // Global variables that these utilities depend on let _matchRoutes: MatchRoutes; @@ -31,11 +31,17 @@ function trimSlash(path: string): string { return path[path.length - 1] === '/' ? path.slice(0, -1) : path; } -function pathEndsWithWildcard(path: string): boolean { +/** + * Checks if a path ends with a wildcard character (*). + */ +export function pathEndsWithWildcard(path: string): boolean { return path.endsWith('*'); } -function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): boolean { +/** + * Checks if a path is a wildcard and has child routes. + */ +export function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): boolean { return (pathEndsWithWildcard(path) && !!branch.route.children?.length) || false; } @@ -213,3 +219,30 @@ export function getNormalizedName( return [fallbackTransactionName, 'url']; } + +/** + * Shared helper function to resolve route name and source + */ +export function resolveRouteNameAndSource( + location: Location, + routes: RouteObject[], + allRoutes: RouteObject[], + branches: RouteMatch[], + basename: string = '', +): [string, TransactionSource] { + let name: string | undefined; + let source: TransactionSource = 'url'; + + const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes); + + if (isInDescendantRoute) { + name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes, location)); + source = 'route'; + } + + if (!isInDescendantRoute || !name) { + [name, source] = getNormalizedName(routes, location, branches, basename); + } + + return [name || location.pathname, source]; +} diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx index a32e2bb02bf1..0c58bdb68f35 100644 --- a/packages/react/src/reactrouterv6.tsx +++ b/packages/react/src/reactrouterv6.tsx @@ -1,13 +1,13 @@ import type { browserTracingIntegration } from '@sentry/browser'; import type { Integration } from '@sentry/core'; -import type { ReactRouterOptions } from './reactrouterv6-compat-utils'; +import type { ReactRouterOptions } from './reactrouter-compat-utils'; import { createReactRouterV6CompatibleTracingIntegration, createV6CompatibleWithSentryReactRouterRouting, createV6CompatibleWrapCreateBrowserRouter, createV6CompatibleWrapCreateMemoryRouter, createV6CompatibleWrapUseRoutes, -} from './reactrouterv6-compat-utils'; +} from './reactrouter-compat-utils'; import type { CreateRouterFunction, Router, RouterState, UseRoutes } from './types'; /** diff --git a/packages/react/src/reactrouterv7.tsx b/packages/react/src/reactrouterv7.tsx index 5a80482cd2c3..a25e12d40e68 100644 --- a/packages/react/src/reactrouterv7.tsx +++ b/packages/react/src/reactrouterv7.tsx @@ -1,14 +1,14 @@ // React Router v7 uses the same integration as v6 import type { browserTracingIntegration } from '@sentry/browser'; import type { Integration } from '@sentry/core'; -import type { ReactRouterOptions } from './reactrouterv6-compat-utils'; +import type { ReactRouterOptions } from './reactrouter-compat-utils'; import { createReactRouterV6CompatibleTracingIntegration, createV6CompatibleWithSentryReactRouterRouting, createV6CompatibleWrapCreateBrowserRouter, createV6CompatibleWrapCreateMemoryRouter, createV6CompatibleWrapUseRoutes, -} from './reactrouterv6-compat-utils'; +} from './reactrouter-compat-utils'; import type { CreateRouterFunction, Router, RouterState, UseRoutes } from './types'; /** From 451dabaea5af9f6a60db211656785f3e61c24cec Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 26 Aug 2025 09:53:55 +0100 Subject: [PATCH 06/15] Add missing local exports --- .../src/reactrouter-compat-utils/index.ts | 2 + .../instrumentation.tsx | 2 +- .../src/reactrouter-compat-utils/utils.ts | 9 ++- .../test/reactrouterv6-compat-utils.test.tsx | 56 +++++++++---------- 4 files changed, 39 insertions(+), 30 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index 7d01a5a48eea..9da9aabb6e88 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -10,6 +10,7 @@ export { handleExistingNavigationSpan, createNewNavigationSpan, addResolvedRoutesToParent, + processResolvedRoutes, } from './instrumentation'; // Utility exports @@ -19,6 +20,7 @@ export { rebuildRoutePathFromAllRoutes, locationIsInsideDescendantRoute, getNormalizedName, + getNumberOfUrlSegments, resolveRouteNameAndSource, pathEndsWithWildcard, pathIsWildcardAndHasChildren, diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index e6ad17595802..ba521b47a826 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -114,7 +114,7 @@ const allRoutes = new Set(); /** * Processes resolved routes by adding them to allRoutes and checking for nested async handlers. */ -function processResolvedRoutes( +export function processResolvedRoutes( resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location, diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index 8e6b8b8e8807..00205c3887e3 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -64,7 +64,14 @@ function sendIndexPath(pathBuilder: string, pathname: string, basename: string): return [formattedPath, 'route']; } -function getNumberOfUrlSegments(url: string): number { +/** + * Returns the number of URL segments in the given URL string. + * Splits at '/' or '\/' to handle regex URLs correctly. + * + * @param url - The URL string to segment. + * @returns The number of segments in the URL. + */ +export function getNumberOfUrlSegments(url: string): number { // split at '/' or at '\/' to split regex urls correctly return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; } diff --git a/packages/react/test/reactrouterv6-compat-utils.test.tsx b/packages/react/test/reactrouterv6-compat-utils.test.tsx index de5d4fa74871..d0ffeb66fd22 100644 --- a/packages/react/test/reactrouterv6-compat-utils.test.tsx +++ b/packages/react/test/reactrouterv6-compat-utils.test.tsx @@ -1,11 +1,11 @@ import { describe, expect, it, test } from 'vitest'; -import { checkRouteForAsyncHandler } from '../src/lazy-route-utils'; -import { getNumberOfUrlSegments } from '../src/reactrouterv6-compat-utils'; +import { + checkRouteForAsyncHandler, + getNumberOfUrlSegments, + processResolvedRoutes, +} from '../src/reactrouter-compat-utils'; import type { RouteObject } from '../src/types'; -// Mock processResolvedRoutes function for tests -const mockProcessResolvedRoutes = () => {}; - describe('getNumberOfUrlSegments', () => { test.each([ ['regular path', '/projects/123/views/234', 4], @@ -27,8 +27,8 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); const proxiedHandler = route.handle?.lazyChildren; expect(typeof proxiedHandler).toBe('function'); @@ -45,7 +45,7 @@ describe('checkRouteForAsyncHandler', () => { path: '/test', }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with non-function handle properties', () => { @@ -56,7 +56,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with null/undefined handle properties', () => { @@ -65,7 +65,7 @@ describe('checkRouteForAsyncHandler', () => { handle: null as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with mixed function and non-function handle properties', () => { @@ -79,7 +79,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); const proxiedHandler = route.handle?.lazyChildren; expect(typeof proxiedHandler).toBe('function'); @@ -110,7 +110,7 @@ describe('checkRouteForAsyncHandler', () => { ], }; - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); // Check parent handler is proxied const proxiedParentHandler = route.handle?.lazyChildren; @@ -153,7 +153,7 @@ describe('checkRouteForAsyncHandler', () => { ], }; - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); // Check all handlers are proxied expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -179,7 +179,7 @@ describe('checkRouteForAsyncHandler', () => { }, }; - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); // Check all handlers are proxied expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -197,13 +197,13 @@ describe('checkRouteForAsyncHandler', () => { }; // First call should proxy the function - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); const firstProxiedHandler = route.handle?.lazyChildren; expect(firstProxiedHandler).not.toBe(mockHandler); expect((firstProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); // Second call should not create a new proxy - checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + checkRouteForAsyncHandler(route, processResolvedRoutes); const secondProxiedHandler = route.handle?.lazyChildren; expect(secondProxiedHandler).toBe(firstProxiedHandler); // Should be the same proxy expect((secondProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); @@ -215,7 +215,7 @@ describe('checkRouteForAsyncHandler', () => { children: [], }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with undefined children', () => { @@ -224,7 +224,7 @@ describe('checkRouteForAsyncHandler', () => { children: undefined, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with null children', () => { @@ -233,7 +233,7 @@ describe('checkRouteForAsyncHandler', () => { children: null as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with non-array children', () => { @@ -242,7 +242,7 @@ describe('checkRouteForAsyncHandler', () => { children: 'not an array' as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is not an object', () => { @@ -251,7 +251,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 'not an object' as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is null', () => { @@ -260,7 +260,7 @@ describe('checkRouteForAsyncHandler', () => { handle: null as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is undefined', () => { @@ -269,7 +269,7 @@ describe('checkRouteForAsyncHandler', () => { handle: undefined as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a function', () => { @@ -278,7 +278,7 @@ describe('checkRouteForAsyncHandler', () => { handle: (() => {}) as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a string', () => { @@ -287,7 +287,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 'string handle' as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a number', () => { @@ -296,7 +296,7 @@ describe('checkRouteForAsyncHandler', () => { handle: 42 as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is a boolean', () => { @@ -305,7 +305,7 @@ describe('checkRouteForAsyncHandler', () => { handle: true as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); it('should handle routes with handle that is an array', () => { @@ -314,6 +314,6 @@ describe('checkRouteForAsyncHandler', () => { handle: [] as any, }; - expect(() => checkRouteForAsyncHandler(route, mockProcessResolvedRoutes)).not.toThrow(); + expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); }); }); From 2b78f54588b7196cf3c291837aa70f9866e0fe26 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 26 Aug 2025 14:08:04 +0100 Subject: [PATCH 07/15] Move `getGlobalLocation` to utils --- .../src/reactrouter-compat-utils/index.ts | 1 + .../instrumentation.tsx | 31 ++----------------- .../src/reactrouter-compat-utils/utils.ts | 15 +++++++++ 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index 9da9aabb6e88..7a4dd99c1119 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -16,6 +16,7 @@ export { // Utility exports export { initializeRouterUtils, + getGlobalLocation, prefixWithSlash, rebuildRoutePathFromAllRoutes, locationIsInsideDescendantRoute, diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index ba521b47a826..94aa2a4aac41 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -6,7 +6,6 @@ import { browserTracingIntegration, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, - WINDOW, } from '@sentry/browser'; import type { Client, Integration, Span, TransactionSource } from '@sentry/core'; import { @@ -42,6 +41,7 @@ import type { } from '../types'; import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-routes'; import { + getGlobalLocation, getNormalizedName, initializeRouterUtils, locationIsInsideDescendantRoute, @@ -59,31 +59,6 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); -/** - * Gets the current location from the window object in browser environments. - * Returns undefined if window is not available. - */ -function getGlobalLocation(): Location | undefined { - if (typeof WINDOW !== 'undefined') { - const globalLocation = WINDOW.location; - if (globalLocation) { - return { pathname: globalLocation.pathname }; - } - } - return undefined; -} - -/** - * Gets the pathname from the window object in browser environments. - * Returns undefined if window is not available. - */ -function getGlobalPathname(): string | undefined { - if (typeof WINDOW !== 'undefined') { - return WINDOW.location?.pathname; - } - return undefined; -} - export interface ReactRouterOptions { useEffect: UseEffect; useLocation: UseLocation; @@ -213,7 +188,7 @@ function wrapPatchRoutesOnNavigation( if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { // For memory routers, we don't have a reliable way to get the current pathname // without accessing window.location, so we'll use targetPath for both cases - const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); + const pathname = targetPath || (isMemoryRouter ? getGlobalLocation()?.pathname : undefined); if (pathname) { updateNavigationSpanWithLazyRoutes( activeRootSpan, @@ -446,7 +421,7 @@ export function createReactRouterV6CompatibleTracingIntegration( afterAllSetup(client) { integration.afterAllSetup(client); - const initPathName = getGlobalPathname(); + const initPathName = getGlobalLocation()?.pathname; if (instrumentPageLoad && initPathName) { startBrowserTracingPageLoadSpan(client, { name: initPathName, diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index 00205c3887e3..cf25e9fafd65 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,3 +1,4 @@ +import { WINDOW } from '@sentry/browser'; import type { TransactionSource } from '@sentry/core'; import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; @@ -14,6 +15,20 @@ export function initializeRouterUtils(matchRoutes: MatchRoutes, stripBasename: b _stripBasename = stripBasename; } +/** + * Gets the current location from the window object in browser environments. + * Returns undefined if window is not available. + */ +export function getGlobalLocation(): Location | undefined { + if (typeof WINDOW !== 'undefined') { + const globalLocation = WINDOW.location; + if (globalLocation) { + return { pathname: globalLocation.pathname }; + } + } + return undefined; +} + // Helper functions function pickPath(match: RouteMatch): string { return trimWildcard(match.route.path || ''); From 4dd267452af3d1bea79edee7f2620c437a1ccc28 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 26 Aug 2025 14:15:10 +0100 Subject: [PATCH 08/15] Return to `updateNavigationSpan` --- .../src/reactrouter-compat-utils/index.ts | 9 +-- .../instrumentation.tsx | 60 +++++++++++++++++-- .../reactrouter-compat-utils/lazy-routes.tsx | 53 +--------------- .../src/reactrouter-compat-utils/utils.ts | 11 ++++ 4 files changed, 70 insertions(+), 63 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index 7a4dd99c1119..c62fb408f033 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -11,12 +11,14 @@ export { createNewNavigationSpan, addResolvedRoutesToParent, processResolvedRoutes, + updateNavigationSpan, } from './instrumentation'; // Utility exports export { initializeRouterUtils, getGlobalLocation, + getGlobalPathname, prefixWithSlash, rebuildRoutePathFromAllRoutes, locationIsInsideDescendantRoute, @@ -28,9 +30,4 @@ export { } from './utils'; // Lazy route exports -export { - updateNavigationSpanWithLazyRoutes, - createAsyncHandlerProxy, - handleAsyncHandlerResult, - checkRouteForAsyncHandler, -} from './lazy-routes'; +export { createAsyncHandlerProxy, handleAsyncHandlerResult, checkRouteForAsyncHandler } from './lazy-routes'; diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 94aa2a4aac41..b8cb390b50a4 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -39,9 +39,10 @@ import type { UseNavigationType, UseRoutes, } from '../types'; -import { checkRouteForAsyncHandler, updateNavigationSpanWithLazyRoutes } from './lazy-routes'; +import { checkRouteForAsyncHandler } from './lazy-routes'; import { getGlobalLocation, + getGlobalPathname, getNormalizedName, initializeRouterUtils, locationIsInsideDescendantRoute, @@ -59,6 +60,53 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); +/** + * Updates a navigation span with the correct route name after lazy routes have been loaded. + */ +export function updateNavigationSpan( + activeRootSpan: Span, + location: Location, + allRoutes: RouteObject[], + forceUpdate = false, + matchRoutes: MatchRoutes, +): void { + // Check if this span has already been named to avoid multiple updates + // But allow updates if this is a forced update (e.g., when lazy routes are loaded) + const hasBeenNamed = + !forceUpdate && + ( + activeRootSpan as { + __sentry_navigation_name_set__?: boolean; + } + )?.__sentry_navigation_name_set__; + + if (!hasBeenNamed) { + // Get fresh branches for the current location with all loaded routes + const currentBranches = matchRoutes(allRoutes, location); + const [name, source] = resolveRouteNameAndSource( + location, + allRoutes, + allRoutes, + (currentBranches as RouteMatch[]) || [], + '', + ); + + // Only update if we have a valid name and the span hasn't finished + const spanJson = spanToJSON(activeRootSpan); + if (name && !spanJson.timestamp) { + activeRootSpan.updateName(name); + activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + + // Mark this span as having its name set to prevent future updates + addNonEnumerableProperty( + activeRootSpan as { __sentry_navigation_name_set__?: boolean }, + '__sentry_navigation_name_set__', + true, + ); + } + } +} + export interface ReactRouterOptions { useEffect: UseEffect; useLocation: UseLocation; @@ -131,7 +179,7 @@ export function processResolvedRoutes( ); } else if (spanOp === 'navigation') { // For navigation spans, update the name with the newly loaded routes - updateNavigationSpanWithLazyRoutes(activeRootSpan, location, Array.from(allRoutes), false, _matchRoutes); + updateNavigationSpan(activeRootSpan, location, Array.from(allRoutes), false, _matchRoutes); } } } @@ -162,7 +210,7 @@ function wrapPatchRoutesOnNavigation( addRoutesToAllRoutes(children); const activeRootSpan = getActiveRootSpan(); if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { - updateNavigationSpanWithLazyRoutes( + updateNavigationSpan( activeRootSpan, { pathname: targetPath, @@ -188,9 +236,9 @@ function wrapPatchRoutesOnNavigation( if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { // For memory routers, we don't have a reliable way to get the current pathname // without accessing window.location, so we'll use targetPath for both cases - const pathname = targetPath || (isMemoryRouter ? getGlobalLocation()?.pathname : undefined); + const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); if (pathname) { - updateNavigationSpanWithLazyRoutes( + updateNavigationSpan( activeRootSpan, { pathname, @@ -421,7 +469,7 @@ export function createReactRouterV6CompatibleTracingIntegration( afterAllSetup(client) { integration.afterAllSetup(client); - const initPathName = getGlobalLocation()?.pathname; + const initPathName = getGlobalPathname(); if (instrumentPageLoad && initPathName) { startBrowserTracingPageLoadSpan(client, { name: initPathName, diff --git a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 79ab7fc7e23e..992919bf59a3 100644 --- a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -1,55 +1,6 @@ -import type { Span } from '@sentry/core'; -import { addNonEnumerableProperty, debug, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; +import { addNonEnumerableProperty, debug } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; -import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; -import { resolveRouteNameAndSource } from './utils'; - -/** - * Updates a navigation span with the correct route name after lazy routes have been loaded. - */ -export function updateNavigationSpanWithLazyRoutes( - activeRootSpan: Span, - location: Location, - allRoutes: RouteObject[], - forceUpdate = false, - matchRoutes: MatchRoutes, -): void { - // Check if this span has already been named to avoid multiple updates - // But allow updates if this is a forced update (e.g., when lazy routes are loaded) - const hasBeenNamed = - !forceUpdate && - ( - activeRootSpan as { - __sentry_navigation_name_set__?: boolean; - } - )?.__sentry_navigation_name_set__; - - if (!hasBeenNamed) { - // Get fresh branches for the current location with all loaded routes - const currentBranches = matchRoutes(allRoutes, location); - const [name, source] = resolveRouteNameAndSource( - location, - allRoutes, - allRoutes, - (currentBranches as RouteMatch[]) || [], - '', - ); - - // Only update if we have a valid name and the span hasn't finished - const spanJson = spanToJSON(activeRootSpan); - if (name && !spanJson.timestamp) { - activeRootSpan.updateName(name); - activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); - - // Mark this span as having its name set to prevent future updates - addNonEnumerableProperty( - activeRootSpan as { __sentry_navigation_name_set__?: boolean }, - '__sentry_navigation_name_set__', - true, - ); - } - } -} +import type { Location, RouteObject } from '../types'; /** * Creates a proxy wrapper for an async handler function. diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index cf25e9fafd65..aeac099f5c93 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -29,6 +29,17 @@ export function getGlobalLocation(): Location | undefined { return undefined; } +/** + * Gets the pathname from the window object in browser environments. + * Returns undefined if window is not available. + */ +export function getGlobalPathname(): string | undefined { + if (typeof WINDOW !== 'undefined') { + return WINDOW.location?.pathname; + } + return undefined; +} + // Helper functions function pickPath(match: RouteMatch): string { return trimWildcard(match.route.path || ''); From 6378a64a4e848145d67f3d4ccc1543b85bf60d55 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Aug 2025 00:27:51 +0100 Subject: [PATCH 09/15] Add unit tests --- .../instrumentation.test.tsx | 183 +++++ .../lazy-routes.test.ts | 706 ++++++++++++++++++ .../reactrouter-compat-utils/utils.test.ts | 648 ++++++++++++++++ .../test/reactrouterv6-compat-utils.test.tsx | 319 -------- 4 files changed, 1537 insertions(+), 319 deletions(-) create mode 100644 packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx create mode 100644 packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts create mode 100644 packages/react/test/reactrouter-compat-utils/utils.test.ts delete mode 100644 packages/react/test/reactrouterv6-compat-utils.test.tsx diff --git a/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx new file mode 100644 index 000000000000..840adbf0a816 --- /dev/null +++ b/packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx @@ -0,0 +1,183 @@ +/** + * @vitest-environment jsdom + */ +import { startBrowserTracingNavigationSpan } from '@sentry/browser'; +import type { Client, Span } from '@sentry/core'; +import { addNonEnumerableProperty } from '@sentry/core'; +import * as React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + addResolvedRoutesToParent, + createNewNavigationSpan, + createReactRouterV6CompatibleTracingIntegration, + handleExistingNavigationSpan, + updateNavigationSpan, +} from '../../src/reactrouter-compat-utils'; +import type { Location, RouteObject } from '../../src/types'; + +const mockUpdateName = vi.fn(); +const mockSetAttribute = vi.fn(); +const mockSpan = { updateName: mockUpdateName, setAttribute: mockSetAttribute } as unknown as Span; +const mockClient = { addIntegration: vi.fn() } as unknown as Client; + +vi.mock('@sentry/core', async requireActual => { + const actual = await requireActual(); + return { + ...(actual as any), + addNonEnumerableProperty: vi.fn(), + getActiveSpan: vi.fn(() => mockSpan), + getClient: vi.fn(() => mockClient), + getRootSpan: vi.fn(() => mockSpan), + spanToJSON: vi.fn(() => ({ op: 'navigation' })), + }; +}); + +vi.mock('@sentry/browser', async requireActual => { + const actual = await requireActual(); + return { + ...(actual as any), + startBrowserTracingNavigationSpan: vi.fn(), + browserTracingIntegration: vi.fn(() => ({ + setup: vi.fn(), + afterAllSetup: vi.fn(), + name: 'BrowserTracing', + })), + }; +}); + +vi.mock('../../src/reactrouter-compat-utils/utils', () => ({ + resolveRouteNameAndSource: vi.fn(() => ['Test Route', 'route']), + initializeRouterUtils: vi.fn(), + getGlobalLocation: vi.fn(() => ({ pathname: '/test', search: '', hash: '' })), + getGlobalPathname: vi.fn(() => '/test'), +})); + +vi.mock('../../src/reactrouter-compat-utils/lazy-routes', () => ({ + checkRouteForAsyncHandler: vi.fn(), +})); + +describe('reactrouter-compat-utils/instrumentation', () => { + const sampleLocation: Location = { + pathname: '/test', + search: '', + hash: '', + state: null, + key: 'default', + }; + + const sampleRoutes: RouteObject[] = [ + { path: '/', element:
Home
}, + { path: '/about', element:
About
}, + ]; + + const mockMatchRoutes = vi.fn(() => []); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('updateNavigationSpan', () => { + it('should update navigation span name and source when not already named', () => { + updateNavigationSpan(mockSpan, sampleLocation, sampleRoutes, false, mockMatchRoutes); + + expect(mockUpdateName).toHaveBeenCalledWith('Test Route'); + expect(mockSetAttribute).toHaveBeenCalledWith('sentry.source', 'route'); + expect(addNonEnumerableProperty).toHaveBeenCalledWith(mockSpan, '__sentry_navigation_name_set__', true); + }); + + it('should not update when span already has name set', () => { + const spanWithNameSet = { ...mockSpan, __sentry_navigation_name_set__: true }; + + updateNavigationSpan(spanWithNameSet as any, sampleLocation, sampleRoutes, false, mockMatchRoutes); + + expect(mockUpdateName).not.toHaveBeenCalled(); + }); + }); + + describe('handleExistingNavigationSpan', () => { + it('should update span name when not already named', () => { + const spanJson = { op: 'navigation', data: {}, span_id: 'test', start_timestamp: 0, trace_id: 'test' }; + + handleExistingNavigationSpan(mockSpan, spanJson, 'Test Route', 'route', false); + + expect(mockUpdateName).toHaveBeenCalledWith('Test Route'); + expect(mockSetAttribute).toHaveBeenCalledWith('sentry.source', 'route'); + expect(addNonEnumerableProperty).toHaveBeenCalledWith(mockSpan, '__sentry_navigation_name_set__', true); + }); + + it('should not mark as named for lazy routes', () => { + const spanJson = { op: 'navigation', data: {}, span_id: 'test', start_timestamp: 0, trace_id: 'test' }; + + handleExistingNavigationSpan(mockSpan, spanJson, 'Test Route', 'route', true); + + expect(mockUpdateName).toHaveBeenCalledWith('Test Route'); + expect(mockSetAttribute).toHaveBeenCalledWith('sentry.source', 'route'); + expect(addNonEnumerableProperty).not.toHaveBeenCalled(); + }); + }); + + describe('createNewNavigationSpan', () => { + it('should create new navigation span with correct attributes', () => { + createNewNavigationSpan(mockClient, 'Test Route', 'route', '6', false); + + expect(startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, { + name: 'Test Route', + attributes: { + 'sentry.source': 'route', + 'sentry.op': 'navigation', + 'sentry.origin': 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + }); + + describe('addResolvedRoutesToParent', () => { + it('should add new routes to parent with no existing children', () => { + const parentRoute: RouteObject = { path: '/parent', element:
Parent
}; + const resolvedRoutes = [{ path: '/child1', element:
Child 1
}]; + + addResolvedRoutesToParent(resolvedRoutes, parentRoute); + + expect(parentRoute.children).toEqual(resolvedRoutes); + }); + + it('should not add duplicate routes by path', () => { + const existingRoute = { path: '/duplicate', element:
Existing
}; + const parentRoute: RouteObject = { + path: '/parent', + element:
Parent
, + children: [existingRoute], + }; + const duplicateRoute = { path: '/duplicate', element:
Duplicate
}; + + addResolvedRoutesToParent([duplicateRoute], parentRoute); + + expect(parentRoute.children).toEqual([existingRoute]); + }); + }); + + describe('createReactRouterV6CompatibleTracingIntegration', () => { + it('should create integration with correct setup', () => { + const mockUseEffect = vi.fn(); + const mockUseLocation = vi.fn(); + const mockUseNavigationType = vi.fn(); + const mockCreateRoutesFromChildren = vi.fn(); + + const integration = createReactRouterV6CompatibleTracingIntegration( + { + useEffect: mockUseEffect, + useLocation: mockUseLocation, + useNavigationType: mockUseNavigationType, + createRoutesFromChildren: mockCreateRoutesFromChildren, + matchRoutes: mockMatchRoutes, + }, + '6', + ); + + expect(integration).toHaveProperty('setup'); + expect(integration).toHaveProperty('afterAllSetup'); + expect(typeof integration.setup).toBe('function'); + expect(typeof integration.afterAllSetup).toBe('function'); + }); + }); +}); diff --git a/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts new file mode 100644 index 000000000000..732b893ea8f8 --- /dev/null +++ b/packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts @@ -0,0 +1,706 @@ +import { addNonEnumerableProperty, debug } from '@sentry/core'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + checkRouteForAsyncHandler, + createAsyncHandlerProxy, + handleAsyncHandlerResult, +} from '../../src/reactrouter-compat-utils'; +import type { RouteObject } from '../../src/types'; + +vi.mock('@sentry/core', async requireActual => { + const actual = await requireActual(); + return { + ...(actual as any), + addNonEnumerableProperty: vi.fn(), + debug: { + warn: vi.fn(), + }, + }; +}); + +vi.mock('../../src/debug-build', () => ({ + DEBUG_BUILD: true, +})); + +describe('reactrouter-compat-utils/lazy-routes', () => { + let mockProcessResolvedRoutes: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + mockProcessResolvedRoutes = vi.fn(); + }); + + describe('createAsyncHandlerProxy', () => { + it('should create a proxy that calls the original function', () => { + const originalFunction = vi.fn(() => 'result'); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + const result = proxy('arg1', 'arg2'); + + expect(originalFunction).toHaveBeenCalledWith('arg1', 'arg2'); + expect(result).toBe('result'); + }); + + it('should preserve the original function context (this binding)', () => { + const context = { value: 'test' }; + const originalFunction = vi.fn(function (this: typeof context) { + return this.value; + }); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + const result = proxy.call(context); + + expect(originalFunction).toHaveBeenCalledWith(); + expect(result).toBe('test'); + }); + + it('should handle functions with no arguments', () => { + const originalFunction = vi.fn(() => 'no-args-result'); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + const result = proxy(); + + expect(originalFunction).toHaveBeenCalledWith(); + expect(result).toBe('no-args-result'); + }); + + it('should handle functions with many arguments', () => { + const originalFunction = vi.fn((...args) => args.length); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + const result = proxy(1, 2, 3, 4, 5); + + expect(originalFunction).toHaveBeenCalledWith(1, 2, 3, 4, 5); + expect(result).toBe(5); + }); + + it('should mark the proxy with __sentry_proxied__ property', () => { + const originalFunction = vi.fn(); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + expect(addNonEnumerableProperty).toHaveBeenCalledWith(expect.any(Function), '__sentry_proxied__', true); + }); + + it('should call handleAsyncHandlerResult with the function result', () => { + const originalFunction = vi.fn(() => ['route1', 'route2']); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + proxy(); + + // Since handleAsyncHandlerResult is called internally, we verify through its side effects + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(['route1', 'route2'], route); + }); + + it('should handle functions that throw exceptions', () => { + const originalFunction = vi.fn(() => { + throw new Error('Test error'); + }); + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + + expect(() => proxy()).toThrow('Test error'); + expect(originalFunction).toHaveBeenCalled(); + }); + + it('should handle complex route objects', () => { + const originalFunction = vi.fn(() => []); + const route: RouteObject = { + path: '/complex', + id: 'complex-route', + index: false, + caseSensitive: true, + children: [{ path: 'child' }], + element: '
Test
', + }; + const handlerKey = 'complexHandler'; + + const proxy = createAsyncHandlerProxy(originalFunction, route, handlerKey, mockProcessResolvedRoutes); + proxy(); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith([], route); + }); + }); + + describe('handleAsyncHandlerResult', () => { + const route: RouteObject = { path: '/test' }; + const handlerKey = 'testHandler'; + + it('should handle array results directly', () => { + const routes: RouteObject[] = [{ path: '/route1' }, { path: '/route2' }]; + + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + }); + + it('should handle empty array results', () => { + const routes: RouteObject[] = []; + + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + }); + + 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); + + // Wait for the promise to resolve + await promiseResult; + + // Use setTimeout to wait for the async handling + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + }); + + it('should handle Promise results that resolve to empty arrays', async () => { + const routes: RouteObject[] = []; + const promiseResult = Promise.resolve(routes); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + await promiseResult; + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + }); + + it('should handle Promise results that resolve to non-arrays', async () => { + const promiseResult = Promise.resolve('not an array'); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + await promiseResult; + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle Promise results that resolve to null', async () => { + const promiseResult = Promise.resolve(null); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + await promiseResult; + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle Promise results that resolve to undefined', async () => { + const promiseResult = Promise.resolve(undefined); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + await promiseResult; + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle Promise rejections gracefully', async () => { + const promiseResult = Promise.reject(new Error('Test error')); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + // Wait for the promise to be handled + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(debug.warn).toHaveBeenCalledWith( + expect.stringContaining('Error resolving async handler'), + route, + expect.any(Error), + ); + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle Promise rejections with non-Error values', async () => { + const promiseResult = Promise.reject('string error'); + + handleAsyncHandlerResult(promiseResult, route, handlerKey, mockProcessResolvedRoutes); + + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(debug.warn).toHaveBeenCalledWith( + expect.stringContaining('Error resolving async handler'), + route, + 'string error', + ); + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + 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); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should ignore boolean values', () => { + handleAsyncHandlerResult(true, route, handlerKey, mockProcessResolvedRoutes); + handleAsyncHandlerResult(false, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should ignore functions as results', () => { + const functionResult = () => 'test'; + handleAsyncHandlerResult(functionResult, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it("should handle objects that look like promises but aren't", () => { + const fakeThenableButNotPromise = { + then: 'not a function', + }; + + handleAsyncHandlerResult(fakeThenableButNotPromise, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle objects that have then property but not a function', () => { + const almostPromise = { + then: null, + }; + + handleAsyncHandlerResult(almostPromise, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).not.toHaveBeenCalled(); + }); + + it('should handle complex route objects in async handling', async () => { + const complexRoute: RouteObject = { + path: '/complex', + id: 'complex-route', + loader: vi.fn(), + element: '
Complex
', + }; + const routes: RouteObject[] = [{ path: '/dynamic1' }, { path: '/dynamic2' }]; + const promiseResult = Promise.resolve(routes); + + handleAsyncHandlerResult(promiseResult, complexRoute, 'complexHandler', mockProcessResolvedRoutes); + + await promiseResult; + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, complexRoute); + }); + + it('should handle nested route objects in arrays', () => { + const routes: RouteObject[] = [ + { + path: '/parent', + children: [{ path: 'child1' }, { path: 'child2', children: [{ path: 'grandchild' }] }], + }, + ]; + + handleAsyncHandlerResult(routes, route, handlerKey, mockProcessResolvedRoutes); + + expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route); + }); + }); + + describe('checkRouteForAsyncHandler', () => { + it('should proxy functions in route.handle', () => { + const testFunction = vi.fn(); + const route: RouteObject = { + path: '/test', + handle: { + testHandler: testFunction, + notAFunction: 'string value', + }, + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // The function should be replaced with a proxy + expect(route.handle!.testHandler).not.toBe(testFunction); + expect(typeof route.handle!.testHandler).toBe('function'); + expect(route.handle!.notAFunction).toBe('string value'); + }); + + it('should handle multiple functions in route.handle', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + const handler3 = vi.fn(); + const route: RouteObject = { + path: '/test', + handle: { + handler1, + handler2, + handler3, + nonFunction: 'not a function', + anotherNonFunction: 42, + }, + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // All functions should be proxied + expect(route.handle!.handler1).not.toBe(handler1); + expect(route.handle!.handler2).not.toBe(handler2); + expect(route.handle!.handler3).not.toBe(handler3); + expect(typeof route.handle!.handler1).toBe('function'); + expect(typeof route.handle!.handler2).toBe('function'); + expect(typeof route.handle!.handler3).toBe('function'); + + // Non-functions should remain unchanged + expect(route.handle!.nonFunction).toBe('not a function'); + expect(route.handle!.anotherNonFunction).toBe(42); + }); + + it('should not proxy already proxied functions', () => { + const testFunction = vi.fn(); + // Mark function as already proxied + (testFunction as any).__sentry_proxied__ = true; + + const route: RouteObject = { + path: '/test', + handle: { + testHandler: testFunction, + }, + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // The function should remain unchanged + expect(route.handle!.testHandler).toBe(testFunction); + }); + + it('should handle mix of proxied and non-proxied functions', () => { + const proxiedFunction = vi.fn(); + const normalFunction = vi.fn(); + (proxiedFunction as any).__sentry_proxied__ = true; + + const route: RouteObject = { + path: '/test', + handle: { + proxiedHandler: proxiedFunction, + normalHandler: normalFunction, + }, + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // Proxied function should remain unchanged + expect(route.handle!.proxiedHandler).toBe(proxiedFunction); + // Normal function should be proxied + expect(route.handle!.normalHandler).not.toBe(normalFunction); + }); + + it('should recursively check child routes', () => { + const parentFunction = vi.fn(); + const childFunction = vi.fn(); + + const route: RouteObject = { + path: '/parent', + handle: { + parentHandler: parentFunction, + }, + children: [ + { + path: 'child', + handle: { + childHandler: childFunction, + }, + }, + ], + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // Both parent and child functions should be proxied + expect(route.handle!.parentHandler).not.toBe(parentFunction); + expect(route.children![0].handle!.childHandler).not.toBe(childFunction); + }); + + it('should handle children without handle properties', () => { + const parentFunction = vi.fn(); + + const route: RouteObject = { + path: '/parent', + handle: { + parentHandler: parentFunction, + }, + children: [ + { + path: 'child1', + // No handle property + }, + { + path: 'child2', + handle: undefined, + }, + ], + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + + expect(route.handle!.parentHandler).not.toBe(parentFunction); + }); + + it('should handle routes without handle property', () => { + const route: RouteObject = { + path: '/test', + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes with null handle property', () => { + const route: RouteObject = { + path: '/test', + handle: null, + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes with handle that is not an object', () => { + const route: RouteObject = { + path: '/test', + // @ts-expect-error - Testing edge case + handle: 'not an object', + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes with handle that is a function', () => { + const route: RouteObject = { + path: '/test', + // @ts-expect-error - Testing edge case + handle: vi.fn(), + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes without children', () => { + const testFunction = vi.fn(); + const route: RouteObject = { + path: '/test', + handle: { + testHandler: testFunction, + }, + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes with null children', () => { + const testFunction = vi.fn(); + const route: RouteObject = { + path: '/test', + handle: { + testHandler: testFunction, + }, + children: null, + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle routes with empty children array', () => { + const testFunction = vi.fn(); + const route: RouteObject = { + path: '/test', + handle: { + testHandler: testFunction, + }, + children: [], + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + }); + + it('should handle deeply nested child routes', () => { + const grandParentFunction = vi.fn(); + const parentFunction = vi.fn(); + const childFunction = vi.fn(); + + const route: RouteObject = { + path: '/grandparent', + handle: { + grandParentHandler: grandParentFunction, + }, + children: [ + { + path: 'parent', + handle: { + parentHandler: parentFunction, + }, + children: [ + { + path: 'child', + handle: { + childHandler: childFunction, + }, + }, + ], + }, + ], + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // All functions should be proxied + expect(route.handle!.grandParentHandler).not.toBe(grandParentFunction); + expect(route.children![0].handle!.parentHandler).not.toBe(parentFunction); + expect(route.children![0].children![0].handle!.childHandler).not.toBe(childFunction); + }); + + it('should handle routes with complex nested structures', () => { + const route: RouteObject = { + path: '/complex', + handle: { + handler1: vi.fn(), + handler2: vi.fn(), + }, + children: [ + { + path: 'level1a', + handle: { + level1aHandler: vi.fn(), + }, + children: [ + { + path: 'level2a', + handle: { + level2aHandler: vi.fn(), + }, + }, + { + path: 'level2b', + // No handle + }, + ], + }, + { + path: 'level1b', + // No handle + children: [ + { + path: 'level2c', + handle: { + level2cHandler: vi.fn(), + }, + }, + ], + }, + ], + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + + // Check that functions were proxied at all levels + expect(typeof route.handle!.handler1).toBe('function'); + expect(typeof route.handle!.handler2).toBe('function'); + expect(typeof route.children![0].handle!.level1aHandler).toBe('function'); + expect(typeof route.children![0].children![0].handle!.level2aHandler).toBe('function'); + expect(typeof route.children![1].children![0].handle!.level2cHandler).toBe('function'); + }); + + it('should preserve route properties during processing', () => { + const originalFunction = vi.fn(); + const route: RouteObject = { + path: '/preserve', + id: 'preserve-route', + caseSensitive: true, + index: false, + handle: { + testHandler: originalFunction, + }, + element: '
Test
', + loader: vi.fn(), + }; + + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + + // Route properties should be preserved + expect(route.path).toBe('/preserve'); + expect(route.id).toBe('preserve-route'); + expect(route.caseSensitive).toBe(true); + expect(route.index).toBe(false); + expect(route.element).toBe('
Test
'); + expect(route.loader).toBeDefined(); + + // Only the handler function should be changed + expect(route.handle!.testHandler).not.toBe(originalFunction); + }); + + it('should handle functions with special names', () => { + const route: RouteObject = { + path: '/test', + handle: { + constructor: vi.fn(), + toString: vi.fn(), + valueOf: vi.fn(), + hasOwnProperty: vi.fn(), + '0': vi.fn(), + 'special-name': vi.fn(), + 'with spaces': vi.fn(), + }, + }; + + expect(() => { + checkRouteForAsyncHandler(route, mockProcessResolvedRoutes); + }).not.toThrow(); + + // All functions should be proxied regardless of name + expect(typeof route.handle!.constructor).toBe('function'); + expect(typeof route.handle!.toString).toBe('function'); + expect(typeof route.handle!.valueOf).toBe('function'); + expect(typeof route.handle!.hasOwnProperty).toBe('function'); + expect(typeof route.handle!['0']).toBe('function'); + expect(typeof route.handle!['special-name']).toBe('function'); + expect(typeof route.handle!['with spaces']).toBe('function'); + }); + }); +}); diff --git a/packages/react/test/reactrouter-compat-utils/utils.test.ts b/packages/react/test/reactrouter-compat-utils/utils.test.ts new file mode 100644 index 000000000000..1b700d34240a --- /dev/null +++ b/packages/react/test/reactrouter-compat-utils/utils.test.ts @@ -0,0 +1,648 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + getGlobalLocation, + getGlobalPathname, + getNormalizedName, + getNumberOfUrlSegments, + initializeRouterUtils, + locationIsInsideDescendantRoute, + pathEndsWithWildcard, + pathIsWildcardAndHasChildren, + prefixWithSlash, + rebuildRoutePathFromAllRoutes, + resolveRouteNameAndSource, +} from '../../src/reactrouter-compat-utils'; +import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../../src/types'; + +vi.mock('@sentry/browser', async requireActual => { + const actual = await requireActual(); + return { + ...(actual as any), + WINDOW: { + location: { + pathname: '/test/path', + search: '?query=1', + hash: '#section', + href: 'https://example.com/test/path?query=1#section', + }, + }, + }; +}); + +const mockMatchRoutes = vi.fn(); + +describe('reactrouter-compat-utils/utils', () => { + beforeEach(() => { + vi.clearAllMocks(); + initializeRouterUtils(mockMatchRoutes as MatchRoutes, false); + }); + + describe('initializeRouterUtils', () => { + it('should initialize with matchRoutes function', () => { + expect(() => { + initializeRouterUtils(mockMatchRoutes as MatchRoutes, false); + }).not.toThrow(); + }); + + it('should handle custom matchRoutes function with dev mode true', () => { + const customMatchRoutes = vi.fn(); + expect(() => { + initializeRouterUtils(customMatchRoutes as MatchRoutes, true); + }).not.toThrow(); + }); + + it('should handle custom matchRoutes function without dev mode flag', () => { + const customMatchRoutes = vi.fn(); + expect(() => { + initializeRouterUtils(customMatchRoutes as MatchRoutes); + }).not.toThrow(); + }); + }); + + describe('getGlobalLocation', () => { + it('should return location with pathname when WINDOW is available', () => { + const result = getGlobalLocation(); + expect(result).toEqual({ pathname: '/test/path' }); + }); + }); + + describe('getGlobalPathname', () => { + it('should return pathname when WINDOW is available', () => { + const result = getGlobalPathname(); + expect(result).toBe('/test/path'); + }); + }); + + describe('prefixWithSlash', () => { + it('should add slash to string without leading slash', () => { + expect(prefixWithSlash('path')).toBe('/path'); + }); + + it('should not add slash to string with leading slash', () => { + expect(prefixWithSlash('/path')).toBe('/path'); + }); + + it('should handle empty string', () => { + expect(prefixWithSlash('')).toBe('/'); + }); + }); + + describe('pathEndsWithWildcard', () => { + it('should return true for path ending with /*', () => { + expect(pathEndsWithWildcard('/users/*')).toBe(true); + }); + + it('should return false for path not ending with /*', () => { + expect(pathEndsWithWildcard('/users')).toBe(false); + }); + + it('should return true for path ending with *', () => { + expect(pathEndsWithWildcard('/users*')).toBe(true); + }); + + it('should return false for empty string', () => { + expect(pathEndsWithWildcard('')).toBe(false); + }); + }); + + describe('pathIsWildcardAndHasChildren', () => { + it('should return true for wildcard path with children', () => { + const branch = { + route: { + path: '/users/*', + children: [{ path: 'profile' }], + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + } as RouteMatch; + + expect(pathIsWildcardAndHasChildren('/users/*', branch)).toBe(true); + }); + + it('should return false for wildcard path without children', () => { + const branch = { + route: { + path: '/users/*', + children: [], + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + } as RouteMatch; + + expect(pathIsWildcardAndHasChildren('/users/*', branch)).toBe(false); + }); + + it('should return false for non-wildcard path with children', () => { + const branch = { + route: { + path: '/users', + children: [{ path: 'profile' }], + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + } as RouteMatch; + + expect(pathIsWildcardAndHasChildren('/users', branch)).toBe(false); + }); + + it('should return false for non-wildcard path without children', () => { + const branch = { + route: { + path: '/users', + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + } as RouteMatch; + + expect(pathIsWildcardAndHasChildren('/users', branch)).toBe(false); + }); + + it('should return false when route has undefined children', () => { + const branch = { + route: { + path: '/users/*', + children: undefined, + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + } as RouteMatch; + + expect(pathIsWildcardAndHasChildren('/users/*', branch)).toBe(false); + }); + }); + + describe('getNumberOfUrlSegments', () => { + it('should count URL segments correctly', () => { + expect(getNumberOfUrlSegments('/users/123/profile')).toBe(3); + }); + + it('should handle single segment', () => { + expect(getNumberOfUrlSegments('/users')).toBe(1); + }); + + it('should handle root path', () => { + expect(getNumberOfUrlSegments('/')).toBe(0); + }); + + it('should handle empty string', () => { + expect(getNumberOfUrlSegments('')).toBe(0); + }); + + it('should handle regex URLs with escaped slashes', () => { + expect(getNumberOfUrlSegments('/users\\/profile')).toBe(2); + }); + + it('should filter out empty segments and commas', () => { + expect(getNumberOfUrlSegments('/users//profile,test')).toBe(2); + }); + }); + + describe('rebuildRoutePathFromAllRoutes', () => { + it('should return pathname when it matches stripped path', () => { + const allRoutes: RouteObject[] = [{ path: '/users', element: null }]; + + const location: Location = { pathname: '/users' }; + + const mockMatches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = rebuildRoutePathFromAllRoutes(allRoutes, location); + expect(result).toBe('/users'); + }); + + it('should return empty string when no routes match', () => { + const allRoutes: RouteObject[] = [{ path: '/users', element: null }]; + + const location: Location = { pathname: '/nonexistent' }; + + mockMatchRoutes.mockReturnValue([]); + + const result = rebuildRoutePathFromAllRoutes(allRoutes, location); + expect(result).toBe(''); + }); + + it('should return empty string when no matches found', () => { + const allRoutes: RouteObject[] = [{ path: '/users', element: null }]; + + const location: Location = { pathname: '/users' }; + + mockMatchRoutes.mockReturnValue(null); + + const result = rebuildRoutePathFromAllRoutes(allRoutes, location); + expect(result).toBe(''); + }); + + it('should handle wildcard routes', () => { + const allRoutes: RouteObject[] = [{ path: '/users/*', element: null }]; + + const location: Location = { pathname: '/users/anything' }; + + const mockMatches: RouteMatch[] = [ + { + route: { path: '*', element: null }, + params: { '*': 'anything' }, + pathname: '/users/anything', + pathnameBase: '/users', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = rebuildRoutePathFromAllRoutes(allRoutes, location); + expect(result).toBe(''); + }); + }); + + describe('locationIsInsideDescendantRoute', () => { + it('should return true when location is inside descendant route', () => { + const routes: RouteObject[] = [ + { + path: '/users/*', + element: 'div', + children: undefined, + }, + ]; + + const location: Location = { pathname: '/users/123/profile' }; + + const mockMatches: RouteMatch[] = [ + { + route: { + path: '/users/*', + element: 'div', + children: undefined, + }, + params: { '*': '123/profile' }, + pathname: '/users/123/profile', + pathnameBase: '/users', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(true); + }); + + it('should return false when route has children (not descendant)', () => { + const routes: RouteObject[] = [ + { + path: '/users/*', + element: 'div', + children: [{ path: 'profile' }], + }, + ]; + + const location: Location = { pathname: '/users/123/profile' }; + + const mockMatches: RouteMatch[] = [ + { + route: { + path: '/users/*', + element: 'div', + children: [{ path: 'profile' }], + }, + params: { '*': '123/profile' }, + pathname: '/users/123/profile', + pathnameBase: '/users', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(false); + }); + + it('should return false when route has no element', () => { + const routes: RouteObject[] = [ + { + path: '/users/*', + element: null, + children: undefined, + }, + ]; + + const location: Location = { pathname: '/users/123/profile' }; + + const mockMatches: RouteMatch[] = [ + { + route: { + path: '/users/*', + element: null, + children: undefined, + }, + params: { '*': '123/profile' }, + pathname: '/users/123/profile', + pathnameBase: '/users', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(false); + }); + + it('should return false when path does not end with /*', () => { + const routes: RouteObject[] = [ + { + path: '/users', + element: 'div', + children: undefined, + }, + ]; + + const location: Location = { pathname: '/users' }; + + const mockMatches: RouteMatch[] = [ + { + route: { + path: '/users', + element: 'div', + children: undefined, + }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(false); + }); + + it('should return false when no splat parameter', () => { + const routes: RouteObject[] = [ + { + path: '/users/*', + element: 'div', + children: undefined, + }, + ]; + + const location: Location = { pathname: '/users' }; + + const mockMatches: RouteMatch[] = [ + { + route: { + path: '/users/*', + element: 'div', + children: undefined, + }, + params: {}, + pathname: '/users', + pathnameBase: '/users', + }, + ]; + + mockMatchRoutes.mockReturnValue(mockMatches); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(false); + }); + + it('should return false when no matches found', () => { + const routes: RouteObject[] = [{ path: '/users', element: null }]; + + const location: Location = { pathname: '/posts' }; + + mockMatchRoutes.mockReturnValue(null); + + const result = locationIsInsideDescendantRoute(location, routes); + expect(result).toBe(false); + }); + }); + + describe('getNormalizedName', () => { + it('should return pathname with url source when no routes provided', () => { + const routes: RouteObject[] = []; + const location: Location = { pathname: '/test' }; + const branches: RouteMatch[] = []; + + const result = getNormalizedName(routes, location, branches); + expect(result).toEqual(['/test', 'url']); + }); + + it('should handle index route', () => { + const routes: RouteObject[] = [{ path: '/', index: true, element: null }]; + const location: Location = { pathname: '/' }; + const branches: RouteMatch[] = [ + { + route: { path: '/', index: true, element: null }, + params: {}, + pathname: '/', + pathnameBase: '', + }, + ]; + + const result = getNormalizedName(routes, location, branches, ''); + expect(result).toEqual(['', 'route']); + }); + + it('should handle simple route path', () => { + const routes: RouteObject[] = [{ path: '/users', element: null }]; + const location: Location = { pathname: '/users' }; + const branches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + const result = getNormalizedName(routes, location, branches, ''); + expect(result).toEqual(['/users', 'route']); + }); + + it('should handle nested routes', () => { + const routes: RouteObject[] = [ + { path: '/users', element: null }, + { path: ':userId', element: null }, + ]; + const location: Location = { pathname: '/users/123' }; + const branches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + { + route: { path: ':userId', element: null }, + params: { userId: '123' }, + pathname: '/users/123', + pathnameBase: '/users', + }, + ]; + + const result = getNormalizedName(routes, location, branches, ''); + expect(result).toEqual(['/users/:userId', 'route']); + }); + + it('should handle wildcard routes with children', () => { + const routes: RouteObject[] = [ + { + path: '/users/*', + element: null, + children: [{ path: 'profile', element: null }], + }, + ]; + const location: Location = { pathname: '/users/profile' }; + const branches: RouteMatch[] = [ + { + route: { + path: '/users/*', + element: null, + children: [{ path: 'profile', element: null }], + }, + params: { '*': 'profile' }, + pathname: '/users/profile', + pathnameBase: '/users', + }, + ]; + + const result = getNormalizedName(routes, location, branches, ''); + // Function falls back to url when wildcard path with children doesn't match exact logic + expect(result).toEqual(['/users/profile', 'url']); + }); + + it('should handle basename stripping', () => { + // Initialize with stripBasename = true + initializeRouterUtils(mockMatchRoutes as MatchRoutes, true); + + const routes: RouteObject[] = [{ path: '/users', element: null }]; + const location: Location = { pathname: '/app/users' }; + const branches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/app/users', + pathnameBase: '/app', + }, + ]; + + const result = getNormalizedName(routes, location, branches, '/app'); + // Function falls back to url when basename stripping doesn't match exact logic + expect(result).toEqual(['/users', 'url']); + }); + + it('should fallback to pathname when no matches', () => { + const routes: RouteObject[] = [{ path: '/users', element: null }]; + const location: Location = { pathname: '/posts' }; + const branches: RouteMatch[] = []; + + const result = getNormalizedName(routes, location, branches, ''); + expect(result).toEqual(['/posts', 'url']); + }); + }); + + describe('resolveRouteNameAndSource', () => { + beforeEach(() => { + // Reset to default stripBasename = false + initializeRouterUtils(mockMatchRoutes as MatchRoutes, false); + }); + + it('should use descendant route when location is inside one', () => { + const location: Location = { pathname: '/users/123/profile' }; + const routes: RouteObject[] = [{ path: '/users', element: null }]; + const allRoutes: RouteObject[] = [ + { path: '/users/*', element: 'div' }, // element must be truthy for descendant route + { path: '/users', element: null }, + ]; + const branches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + // Mock for descendant route check + const descendantMatches: RouteMatch[] = [ + { + route: { path: '/users/*', element: 'div' }, + params: { '*': '123/profile' }, + pathname: '/users/123/profile', + pathnameBase: '/users', + }, + ]; + + // Mock for rebuild route path - should return '/users' + const rebuildMatches: RouteMatch[] = [ + { + route: { path: '/users/*', element: 'div' }, + params: { '*': '123/profile' }, + pathname: '/users', + pathnameBase: '', + }, + ]; + + mockMatchRoutes + .mockReturnValueOnce(descendantMatches) // First call for descendant check + .mockReturnValueOnce(rebuildMatches); // Second call for path rebuild + + const result = resolveRouteNameAndSource(location, routes, allRoutes, branches, ''); + // Since locationIsInsideDescendantRoute returns true, it uses route source + expect(result).toEqual(['/users/123/profile', 'route']); + }); + + it('should use normalized name when not in descendant route', () => { + const location: Location = { pathname: '/users' }; + const routes: RouteObject[] = [{ path: '/users', element: null }]; + const allRoutes: RouteObject[] = [{ path: '/users', element: null }]; + const branches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + // Mock for descendant route check (no descendant) + const normalMatches: RouteMatch[] = [ + { + route: { path: '/users', element: null }, + params: {}, + pathname: '/users', + pathnameBase: '', + }, + ]; + + mockMatchRoutes.mockReturnValue(normalMatches); + + const result = resolveRouteNameAndSource(location, routes, allRoutes, branches, ''); + expect(result).toEqual(['/users', 'route']); + }); + + it('should fallback to pathname when no name resolved', () => { + const location: Location = { pathname: '/unknown' }; + const routes: RouteObject[] = []; + const allRoutes: RouteObject[] = []; + const branches: RouteMatch[] = []; + + mockMatchRoutes.mockReturnValue(null); + + const result = resolveRouteNameAndSource(location, routes, allRoutes, branches, ''); + expect(result).toEqual(['/unknown', 'url']); + }); + }); +}); diff --git a/packages/react/test/reactrouterv6-compat-utils.test.tsx b/packages/react/test/reactrouterv6-compat-utils.test.tsx deleted file mode 100644 index d0ffeb66fd22..000000000000 --- a/packages/react/test/reactrouterv6-compat-utils.test.tsx +++ /dev/null @@ -1,319 +0,0 @@ -import { describe, expect, it, test } from 'vitest'; -import { - checkRouteForAsyncHandler, - getNumberOfUrlSegments, - processResolvedRoutes, -} from '../src/reactrouter-compat-utils'; -import type { RouteObject } from '../src/types'; - -describe('getNumberOfUrlSegments', () => { - test.each([ - ['regular path', '/projects/123/views/234', 4], - ['single param parameterized path', '/users/:id/details', 3], - ['multi param parameterized path', '/stores/:storeId/products/:productId', 4], - ['regex path', String(/\/api\/post[0-9]/), 2], - ])('%s', (_: string, input, output) => { - expect(getNumberOfUrlSegments(input)).toEqual(output); - }); -}); - -describe('checkRouteForAsyncHandler', () => { - it('should not create nested proxies when called multiple times on the same route', () => { - const mockHandler = () => Promise.resolve([]); - const route: RouteObject = { - path: '/test', - handle: { - lazyChildren: mockHandler, - }, - }; - - checkRouteForAsyncHandler(route, processResolvedRoutes); - checkRouteForAsyncHandler(route, processResolvedRoutes); - - const proxiedHandler = route.handle?.lazyChildren; - expect(typeof proxiedHandler).toBe('function'); - expect(proxiedHandler).not.toBe(mockHandler); - - expect((proxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - - const proxyHandler = (proxiedHandler as any)?.__sentry_proxied__; - expect(proxyHandler).toBe(true); - }); - - it('should handle routes without handle property', () => { - const route: RouteObject = { - path: '/test', - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with non-function handle properties', () => { - const route: RouteObject = { - path: '/test', - handle: { - someData: 'not a function', - }, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with null/undefined handle properties', () => { - const route: RouteObject = { - path: '/test', - handle: null as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with mixed function and non-function handle properties', () => { - const mockHandler = () => Promise.resolve([]); - const route: RouteObject = { - path: '/test', - handle: { - lazyChildren: mockHandler, - someData: 'not a function', - anotherData: 123, - }, - }; - - checkRouteForAsyncHandler(route, processResolvedRoutes); - - const proxiedHandler = route.handle?.lazyChildren; - expect(typeof proxiedHandler).toBe('function'); - expect(proxiedHandler).not.toBe(mockHandler); - expect((proxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - - // Non-function properties should remain unchanged - expect(route.handle?.someData).toBe('not a function'); - expect(route.handle?.anotherData).toBe(123); - }); - - it('should handle nested routes with async handlers', () => { - const parentHandler = () => Promise.resolve([]); - const childHandler = () => Promise.resolve([]); - - const route: RouteObject = { - path: '/parent', - handle: { - lazyChildren: parentHandler, - }, - children: [ - { - path: '/child', - handle: { - lazyChildren: childHandler, - }, - }, - ], - }; - - checkRouteForAsyncHandler(route, processResolvedRoutes); - - // Check parent handler is proxied - const proxiedParentHandler = route.handle?.lazyChildren; - expect(typeof proxiedParentHandler).toBe('function'); - expect(proxiedParentHandler).not.toBe(parentHandler); - expect((proxiedParentHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - - // Check child handler is proxied - const proxiedChildHandler = route.children?.[0]?.handle?.lazyChildren; - expect(typeof proxiedChildHandler).toBe('function'); - expect(proxiedChildHandler).not.toBe(childHandler); - expect((proxiedChildHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - }); - - it('should handle deeply nested routes', () => { - const level1Handler = () => Promise.resolve([]); - const level2Handler = () => Promise.resolve([]); - const level3Handler = () => Promise.resolve([]); - - const route: RouteObject = { - path: '/level1', - handle: { - lazyChildren: level1Handler, - }, - children: [ - { - path: '/level2', - handle: { - lazyChildren: level2Handler, - }, - children: [ - { - path: '/level3', - handle: { - lazyChildren: level3Handler, - }, - }, - ], - }, - ], - }; - - checkRouteForAsyncHandler(route, processResolvedRoutes); - - // Check all handlers are proxied - expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - expect((route.children?.[0]?.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe( - true, - ); - expect( - (route.children?.[0]?.children?.[0]?.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__, - ).toBe(true); - }); - - it('should handle routes with multiple async handlers', () => { - const handler1 = () => Promise.resolve([]); - const handler2 = () => Promise.resolve([]); - const handler3 = () => Promise.resolve([]); - - const route: RouteObject = { - path: '/test', - handle: { - lazyChildren: handler1, - asyncLoader: handler2, - dataLoader: handler3, - }, - }; - - checkRouteForAsyncHandler(route, processResolvedRoutes); - - // Check all handlers are proxied - expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - expect((route.handle?.asyncLoader as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - expect((route.handle?.dataLoader as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - }); - - it('should not re-proxy already proxied functions', () => { - const mockHandler = () => Promise.resolve([]); - const route: RouteObject = { - path: '/test', - handle: { - lazyChildren: mockHandler, - }, - }; - - // First call should proxy the function - checkRouteForAsyncHandler(route, processResolvedRoutes); - const firstProxiedHandler = route.handle?.lazyChildren; - expect(firstProxiedHandler).not.toBe(mockHandler); - expect((firstProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - - // Second call should not create a new proxy - checkRouteForAsyncHandler(route, processResolvedRoutes); - const secondProxiedHandler = route.handle?.lazyChildren; - expect(secondProxiedHandler).toBe(firstProxiedHandler); // Should be the same proxy - expect((secondProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true); - }); - - it('should handle routes with empty children array', () => { - const route: RouteObject = { - path: '/test', - children: [], - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with undefined children', () => { - const route: RouteObject = { - path: '/test', - children: undefined, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with null children', () => { - const route: RouteObject = { - path: '/test', - children: null as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with non-array children', () => { - const route: RouteObject = { - path: '/test', - children: 'not an array' as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is not an object', () => { - const route: RouteObject = { - path: '/test', - handle: 'not an object' as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is null', () => { - const route: RouteObject = { - path: '/test', - handle: null as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is undefined', () => { - const route: RouteObject = { - path: '/test', - handle: undefined as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is a function', () => { - const route: RouteObject = { - path: '/test', - handle: (() => {}) as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is a string', () => { - const route: RouteObject = { - path: '/test', - handle: 'string handle' as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is a number', () => { - const route: RouteObject = { - path: '/test', - handle: 42 as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is a boolean', () => { - const route: RouteObject = { - path: '/test', - handle: true as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); - - it('should handle routes with handle that is an array', () => { - const route: RouteObject = { - path: '/test', - handle: [] as any, - }; - - expect(() => checkRouteForAsyncHandler(route, processResolvedRoutes)).not.toThrow(); - }); -}); From c46ee841ae2262889ca2495689e1cda2c610628e Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Aug 2025 01:10:32 +0100 Subject: [PATCH 10/15] Reorder to minimize diff --- .../instrumentation.tsx | 260 +++++++++--------- 1 file changed, 130 insertions(+), 130 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index b8cb390b50a4..c6fe5fb988c3 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -61,49 +61,24 @@ let _enableAsyncRouteHandlers: boolean = false; const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet(); /** - * Updates a navigation span with the correct route name after lazy routes have been loaded. + * Adds resolved routes as children to the parent route. + * Prevents duplicate routes by checking if they already exist. */ -export function updateNavigationSpan( - activeRootSpan: Span, - location: Location, - allRoutes: RouteObject[], - forceUpdate = false, - matchRoutes: MatchRoutes, -): void { - // Check if this span has already been named to avoid multiple updates - // But allow updates if this is a forced update (e.g., when lazy routes are loaded) - const hasBeenNamed = - !forceUpdate && - ( - activeRootSpan as { - __sentry_navigation_name_set__?: boolean; - } - )?.__sentry_navigation_name_set__; - - if (!hasBeenNamed) { - // Get fresh branches for the current location with all loaded routes - const currentBranches = matchRoutes(allRoutes, location); - const [name, source] = resolveRouteNameAndSource( - location, - allRoutes, - allRoutes, - (currentBranches as RouteMatch[]) || [], - '', - ); +export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { + const existingChildren = parentRoute.children || []; - // Only update if we have a valid name and the span hasn't finished - const spanJson = spanToJSON(activeRootSpan); - if (name && !spanJson.timestamp) { - activeRootSpan.updateName(name); - activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); + const newRoutes = resolvedRoutes.filter( + newRoute => + !existingChildren.some( + existing => + existing === newRoute || + (newRoute.path && existing.path === newRoute.path) || + (newRoute.id && existing.id === newRoute.id), + ), + ); - // Mark this span as having its name set to prevent future updates - addNonEnumerableProperty( - activeRootSpan as { __sentry_navigation_name_set__?: boolean }, - '__sentry_navigation_name_set__', - true, - ); - } + if (newRoutes.length > 0) { + parentRoute.children = [...existingChildren, ...newRoutes]; } } @@ -185,78 +160,51 @@ export function processResolvedRoutes( } } -function wrapPatchRoutesOnNavigation( - opts: Record | undefined, - isMemoryRouter = false, -): Record { - if (!opts || !('patchRoutesOnNavigation' in opts) || typeof opts.patchRoutesOnNavigation !== 'function') { - return opts || {}; - } - - const originalPatchRoutes = opts.patchRoutesOnNavigation; - return { - ...opts, - patchRoutesOnNavigation: async (args: unknown) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access - const targetPath = (args as any)?.path; - - // For browser router, wrap the patch function to update span during patching - if (!isMemoryRouter) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access - const originalPatch = (args as any)?.patch; - if (originalPatch) { - // 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 activeRootSpan = getActiveRootSpan(); - if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { - updateNavigationSpan( - activeRootSpan, - { - pathname: targetPath, - search: '', - hash: '', - state: null, - key: 'default', - }, - Array.from(allRoutes), - true, - _matchRoutes, - ); - } - return originalPatch(routeId, children); - }; - } +/** + * Updates a navigation span with the correct route name after lazy routes have been loaded. + */ +export function updateNavigationSpan( + activeRootSpan: Span, + location: Location, + allRoutes: RouteObject[], + forceUpdate = false, + matchRoutes: MatchRoutes, +): void { + // Check if this span has already been named to avoid multiple updates + // But allow updates if this is a forced update (e.g., when lazy routes are loaded) + const hasBeenNamed = + !forceUpdate && + ( + activeRootSpan as { + __sentry_navigation_name_set__?: boolean; } + )?.__sentry_navigation_name_set__; - const result = await originalPatchRoutes(args); + if (!hasBeenNamed) { + // Get fresh branches for the current location with all loaded routes + const currentBranches = matchRoutes(allRoutes, location); + const [name, source] = resolveRouteNameAndSource( + location, + allRoutes, + allRoutes, + (currentBranches as RouteMatch[]) || [], + '', + ); - // Update navigation span after routes are patched - const activeRootSpan = getActiveRootSpan(); - if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { - // For memory routers, we don't have a reliable way to get the current pathname - // without accessing window.location, so we'll use targetPath for both cases - const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); - if (pathname) { - updateNavigationSpan( - activeRootSpan, - { - pathname, - search: '', - hash: '', - state: null, - key: 'default', - }, - Array.from(allRoutes), - false, - _matchRoutes, - ); - } - } + // Only update if we have a valid name and the span hasn't finished + const spanJson = spanToJSON(activeRootSpan); + if (name && !spanJson.timestamp) { + activeRootSpan.updateName(name); + activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); - return result; - }, - }; + // Mark this span as having its name set to prevent future updates + addNonEnumerableProperty( + activeRootSpan as { __sentry_navigation_name_set__?: boolean }, + '__sentry_navigation_name_set__', + true, + ); + } + } } /** @@ -551,6 +499,80 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio }; } +function wrapPatchRoutesOnNavigation( + opts: Record | undefined, + isMemoryRouter = false, +): Record { + if (!opts || !('patchRoutesOnNavigation' in opts) || typeof opts.patchRoutesOnNavigation !== 'function') { + return opts || {}; + } + + const originalPatchRoutes = opts.patchRoutesOnNavigation; + return { + ...opts, + patchRoutesOnNavigation: async (args: unknown) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access + const targetPath = (args as any)?.path; + + // For browser router, wrap the patch function to update span during patching + if (!isMemoryRouter) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access + const originalPatch = (args as any)?.patch; + if (originalPatch) { + // 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 activeRootSpan = getActiveRootSpan(); + if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { + updateNavigationSpan( + activeRootSpan, + { + pathname: targetPath, + search: '', + hash: '', + state: null, + key: 'default', + }, + Array.from(allRoutes), + true, + _matchRoutes, + ); + } + return originalPatch(routeId, children); + }; + } + } + + const result = await originalPatchRoutes(args); + + // Update navigation span after routes are patched + const activeRootSpan = getActiveRootSpan(); + if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { + // For memory routers, we don't have a reliable way to get the current pathname + // without accessing window.location, so we'll use targetPath for both cases + const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); + if (pathname) { + updateNavigationSpan( + activeRootSpan, + { + pathname, + search: '', + hash: '', + state: null, + key: 'default', + }, + Array.from(allRoutes), + false, + _matchRoutes, + ); + } + } + + return result; + }, + }; +} + export function handleNavigation(opts: { location: Location; routes: RouteObject[]; @@ -788,25 +810,3 @@ export function createNewNavigationSpan( ); } } - -/** - * Adds resolved routes as children to the parent route. - * Prevents duplicate routes by checking if they already exist. - */ -export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentRoute: RouteObject): void { - const existingChildren = parentRoute.children || []; - - const newRoutes = resolvedRoutes.filter( - newRoute => - !existingChildren.some( - existing => - existing === newRoute || - (newRoute.path && existing.path === newRoute.path) || - (newRoute.id && existing.id === newRoute.id), - ), - ); - - if (newRoutes.length > 0) { - parentRoute.children = [...existingChildren, ...newRoutes]; - } -} From 453f5c6edee4025cba8adf9a0cdbcb2f2aaf0fb5 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Aug 2025 02:08:58 +0100 Subject: [PATCH 11/15] Clean up more --- .../src/reactrouter-compat-utils/index.ts | 2 -- .../instrumentation.tsx | 14 ++++++---- .../src/reactrouter-compat-utils/utils.ts | 26 ------------------- .../reactrouter-compat-utils/utils.test.ts | 16 ------------ 4 files changed, 9 insertions(+), 49 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index c62fb408f033..14b2be20378c 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -17,8 +17,6 @@ export { // Utility exports export { initializeRouterUtils, - getGlobalLocation, - getGlobalPathname, prefixWithSlash, rebuildRoutePathFromAllRoutes, locationIsInsideDescendantRoute, diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index c6fe5fb988c3..80b175328528 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -6,6 +6,7 @@ import { browserTracingIntegration, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, + WINDOW, } from '@sentry/browser'; import type { Client, Integration, Span, TransactionSource } from '@sentry/core'; import { @@ -41,8 +42,6 @@ import type { } from '../types'; import { checkRouteForAsyncHandler } from './lazy-routes'; import { - getGlobalLocation, - getGlobalPathname, getNormalizedName, initializeRouterUtils, locationIsInsideDescendantRoute, @@ -138,7 +137,12 @@ export function processResolvedRoutes( // Try to use the provided location first, then fall back to global window location if needed let location = currentLocation; if (!location) { - location = getGlobalLocation(); + if (typeof WINDOW !== 'undefined') { + const globalLocation = WINDOW.location; + if (globalLocation) { + location = { pathname: globalLocation.pathname }; + } + } } if (location) { @@ -417,7 +421,7 @@ export function createReactRouterV6CompatibleTracingIntegration( afterAllSetup(client) { integration.afterAllSetup(client); - const initPathName = getGlobalPathname(); + const initPathName = WINDOW.location?.pathname; if (instrumentPageLoad && initPathName) { startBrowserTracingPageLoadSpan(client, { name: initPathName, @@ -550,7 +554,7 @@ function wrapPatchRoutesOnNavigation( if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { // For memory routers, we don't have a reliable way to get the current pathname // without accessing window.location, so we'll use targetPath for both cases - const pathname = targetPath || (isMemoryRouter ? getGlobalPathname() : undefined); + const pathname = targetPath || (isMemoryRouter ? WINDOW.location?.pathname : undefined); if (pathname) { updateNavigationSpan( activeRootSpan, diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index aeac099f5c93..00205c3887e3 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -1,4 +1,3 @@ -import { WINDOW } from '@sentry/browser'; import type { TransactionSource } from '@sentry/core'; import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types'; @@ -15,31 +14,6 @@ export function initializeRouterUtils(matchRoutes: MatchRoutes, stripBasename: b _stripBasename = stripBasename; } -/** - * Gets the current location from the window object in browser environments. - * Returns undefined if window is not available. - */ -export function getGlobalLocation(): Location | undefined { - if (typeof WINDOW !== 'undefined') { - const globalLocation = WINDOW.location; - if (globalLocation) { - return { pathname: globalLocation.pathname }; - } - } - return undefined; -} - -/** - * Gets the pathname from the window object in browser environments. - * Returns undefined if window is not available. - */ -export function getGlobalPathname(): string | undefined { - if (typeof WINDOW !== 'undefined') { - return WINDOW.location?.pathname; - } - return undefined; -} - // Helper functions function pickPath(match: RouteMatch): string { return trimWildcard(match.route.path || ''); diff --git a/packages/react/test/reactrouter-compat-utils/utils.test.ts b/packages/react/test/reactrouter-compat-utils/utils.test.ts index 1b700d34240a..91885940db31 100644 --- a/packages/react/test/reactrouter-compat-utils/utils.test.ts +++ b/packages/react/test/reactrouter-compat-utils/utils.test.ts @@ -1,7 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { - getGlobalLocation, - getGlobalPathname, getNormalizedName, getNumberOfUrlSegments, initializeRouterUtils, @@ -59,20 +57,6 @@ describe('reactrouter-compat-utils/utils', () => { }); }); - describe('getGlobalLocation', () => { - it('should return location with pathname when WINDOW is available', () => { - const result = getGlobalLocation(); - expect(result).toEqual({ pathname: '/test/path' }); - }); - }); - - describe('getGlobalPathname', () => { - it('should return pathname when WINDOW is available', () => { - const result = getGlobalPathname(); - expect(result).toBe('/test/path'); - }); - }); - describe('prefixWithSlash', () => { it('should add slash to string without leading slash', () => { expect(prefixWithSlash('path')).toBe('/path'); From 60a9d7c78e4219adc7d40de314daad4327bfad13 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Aug 2025 08:44:56 +0100 Subject: [PATCH 12/15] Introduce `isLikeLazyRouteContext` --- .../src/reactrouter-compat-utils/index.ts | 9 ++--- .../instrumentation.tsx | 20 ++++++----- .../src/reactrouter-compat-utils/utils.ts | 33 ++++++++++++++++++- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index 14b2be20378c..c66e51e734fc 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -16,15 +16,16 @@ export { // Utility exports export { + resolveRouteNameAndSource, + getNormalizedName, initializeRouterUtils, + isLikelyLazyRouteContext, + locationIsInsideDescendantRoute, prefixWithSlash, rebuildRoutePathFromAllRoutes, - locationIsInsideDescendantRoute, - getNormalizedName, - getNumberOfUrlSegments, - resolveRouteNameAndSource, pathEndsWithWildcard, pathIsWildcardAndHasChildren, + getNumberOfUrlSegments, } 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 80b175328528..ef8dc829d03f 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -44,6 +44,7 @@ import { checkRouteForAsyncHandler } from './lazy-routes'; import { getNormalizedName, initializeRouterUtils, + isLikelyLazyRouteContext, locationIsInsideDescendantRoute, prefixWithSlash, rebuildRoutePathFromAllRoutes, @@ -114,7 +115,7 @@ const allRoutes = new Set(); export function processResolvedRoutes( resolvedRoutes: RouteObject[], parentRoute?: RouteObject, - currentLocation?: Location, + currentLocation: Location | null = null, ): void { resolvedRoutes.forEach(child => { allRoutes.add(child); @@ -538,7 +539,7 @@ function wrapPatchRoutesOnNavigation( key: 'default', }, Array.from(allRoutes), - true, + true, // forceUpdate = true since we're loading lazy routes _matchRoutes, ); } @@ -552,9 +553,8 @@ function wrapPatchRoutesOnNavigation( // Update navigation span after routes are patched const activeRootSpan = getActiveRootSpan(); if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') { - // For memory routers, we don't have a reliable way to get the current pathname - // without accessing window.location, so we'll use targetPath for both cases - const pathname = targetPath || (isMemoryRouter ? WINDOW.location?.pathname : undefined); + // For memory routers, we should not access window.location; use targetPath only + const pathname = isMemoryRouter ? targetPath : targetPath || WINDOW.location?.pathname; if (pathname) { updateNavigationSpan( activeRootSpan, @@ -566,7 +566,7 @@ function wrapPatchRoutesOnNavigation( key: 'default', }, Array.from(allRoutes), - false, + false, // forceUpdate = false since this is after lazy routes are loaded _matchRoutes, ); } @@ -603,15 +603,18 @@ export function handleNavigation(opts: { basename, ); + // Check if this might be a lazy route context + const isLazyRouteContext = isLikelyLazyRouteContext(allRoutes || routes, location); + const activeSpan = getActiveSpan(); const spanJson = activeSpan && spanToJSON(activeSpan); const isAlreadyInNavigationSpan = spanJson?.op === 'navigation'; // Cross usage can result in multiple navigation spans being created without this check if (isAlreadyInNavigationSpan && activeSpan && spanJson) { - handleExistingNavigationSpan(activeSpan, spanJson, name, source, false); + handleExistingNavigationSpan(activeSpan, spanJson, name, source, isLazyRouteContext); } else { - createNewNavigationSpan(client, name, source, version, false); + createNewNavigationSpan(client, name, source, version, isLazyRouteContext); } } } @@ -783,6 +786,7 @@ export function handleExistingNavigationSpan( } } + // Always set the source attribute to keep it consistent with the current route activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); } diff --git a/packages/react/src/reactrouter-compat-utils/utils.ts b/packages/react/src/reactrouter-compat-utils/utils.ts index 00205c3887e3..8f7abb7d548e 100644 --- a/packages/react/src/reactrouter-compat-utils/utils.ts +++ b/packages/react/src/reactrouter-compat-utils/utils.ts @@ -14,6 +14,37 @@ export function initializeRouterUtils(matchRoutes: MatchRoutes, stripBasename: b _stripBasename = stripBasename; } +/** + * Checks if the given routes or location context suggests this might be a lazy route scenario. + * This helps determine if we should delay marking navigation spans as "named" to allow for updates + * when lazy routes are loaded. + */ +export function isLikelyLazyRouteContext(routes: RouteObject[], location: Location): boolean { + // Check if any route in the current match has lazy properties + const hasLazyRoute = routes.some(route => { + return ( + // React Router lazy() route + route.lazy || + // Route with async handlers that might load child routes + (route.handle && + typeof route.handle === 'object' && + Object.values(route.handle).some(handler => typeof handler === 'function')) + ); + }); + + if (hasLazyRoute) { + return true; + } + + // Check if current route is unmatched, which might indicate a lazy route that hasn't loaded yet + const currentMatches = _matchRoutes(routes, location); + if (!currentMatches || currentMatches.length === 0) { + return true; + } + + return false; +} + // Helper functions function pickPath(match: RouteMatch): string { return trimWildcard(match.route.path || ''); @@ -222,7 +253,7 @@ export function getNormalizedName( const fallbackTransactionName = _stripBasename ? stripBasenameFromPathname(location.pathname, basename) - : location.pathname; + : location.pathname || ''; return [fallbackTransactionName, 'url']; } From 93f85992889d24a3372846aee1206d45cecdbb39 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 27 Aug 2025 22:05:03 +0200 Subject: [PATCH 13/15] add comment for compat utils --- packages/react/src/reactrouter-compat-utils/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react/src/reactrouter-compat-utils/index.ts b/packages/react/src/reactrouter-compat-utils/index.ts index c66e51e734fc..94f879017c13 100644 --- a/packages/react/src/reactrouter-compat-utils/index.ts +++ b/packages/react/src/reactrouter-compat-utils/index.ts @@ -1,3 +1,5 @@ +// These exports provide utility functions for React Router v6 compatibility (as of now v6 and v7) + // Main exports from instrumentation export type { ReactRouterOptions } from './instrumentation'; export { From c54e779176feb23973482170b9d2e3670f0242e7 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 27 Aug 2025 22:05:19 +0200 Subject: [PATCH 14/15] refactor updatePageloadTransaction --- .../instrumentation.tsx | 71 +++++++++++-------- 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index ef8dc829d03f..5012ed1ccc9c 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -149,14 +149,12 @@ export function processResolvedRoutes( if (location) { if (spanOp === 'pageload') { // Re-run the pageload transaction update with the newly loaded routes - updatePageloadTransaction( + updatePageloadTransaction({ activeRootSpan, - { pathname: location.pathname }, - Array.from(allRoutes), - undefined, - undefined, - Array.from(allRoutes), - ); + 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); @@ -253,14 +251,13 @@ export function createV6CompatibleWrapCreateBrowserRouter< // This is the earliest convenient time to update the transaction name. // Callbacks to `router.subscribe` are not called for the initial load. if (router.state.historyAction === 'POP' && activeRootSpan) { - updatePageloadTransaction( + updatePageloadTransaction({ activeRootSpan, - router.state.location, + location: router.state.location, routes, - undefined, basename, - Array.from(allRoutes), - ); + allRoutes: Array.from(allRoutes), + }); } router.subscribe((state: RouterState) => { @@ -358,7 +355,13 @@ export function createV6CompatibleWrapCreateMemoryRouter< : router.state.location; if (router.state.historyAction === 'POP' && activeRootSpan) { - updatePageloadTransaction(activeRootSpan, location, routes, undefined, basename, Array.from(allRoutes)); + updatePageloadTransaction({ + activeRootSpan, + location, + routes, + basename, + allRoutes: Array.from(allRoutes), + }); } router.subscribe((state: RouterState) => { @@ -475,14 +478,12 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio if (isMountRenderPass.current) { addRoutesToAllRoutes(routes); - updatePageloadTransaction( - getActiveRootSpan(), - normalizedLocation, + updatePageloadTransaction({ + activeRootSpan: getActiveRootSpan(), + location: normalizedLocation, routes, - undefined, - undefined, - Array.from(allRoutes), - ); + allRoutes: Array.from(allRoutes), + }); isMountRenderPass.current = false; } else { handleNavigation({ @@ -647,14 +648,21 @@ function getChildRoutesRecursively(route: RouteObject, allRoutes: Set Date: Wed, 27 Aug 2025 22:07:03 +0200 Subject: [PATCH 15/15] use isThenable --- .../react/src/reactrouter-compat-utils/lazy-routes.tsx | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx index 992919bf59a3..49923854554c 100644 --- a/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx +++ b/packages/react/src/reactrouter-compat-utils/lazy-routes.tsx @@ -1,4 +1,4 @@ -import { addNonEnumerableProperty, debug } from '@sentry/core'; +import { addNonEnumerableProperty, debug, isThenable } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; import type { Location, RouteObject } from '../types'; @@ -33,12 +33,7 @@ export function handleAsyncHandlerResult( handlerKey: string, processResolvedRoutes: (resolvedRoutes: RouteObject[], parentRoute?: RouteObject, currentLocation?: Location) => void, ): void { - if ( - result && - typeof result === 'object' && - 'then' in result && - typeof (result as Promise).then === 'function' - ) { + if (isThenable(result)) { (result as Promise) .then((resolvedRoutes: unknown) => { if (Array.isArray(resolvedRoutes)) {