fix(react): Remove handleExistingNavigation (#17534)

Removes recently introduced special-casing lazy-route -> lazy-route
transaction name updates.
This completes the fix in #17438.
E2E tests are updated to replicate a similar case in the reproduction
here: #17417

Also manually tested on the reproduction.

---------

Co-authored-by: Sigrid Huemer <[email protected]>

Author: onurtemizkan

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx

@@ -32,6 +32,9 @@ export const someMoreNestedRoutes = [
                     <Link to="/another-lazy/sub/888/999" id="navigate-to-another-from-inner">
                       Navigate to Another Lazy Route
                     </Link>
+                    <Link to="/lazy/inner/1/2/" id="navigate-to-upper">
+                      Navigate to Upper Lazy Route
+                    </Link>
                   </div>
                 ),
               },

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts

@@ -107,7 +107,15 @@ test('Creates navigation transactions between two different lazy routes', async
   expect(secondEvent.contexts?.trace?.op).toBe('navigation');
 });
 
-test('Creates navigation transactions from inner lazy route to another lazy route', async ({ page }) => {
+test('Creates navigation transactions from inner lazy route to another lazy route with history navigation', async ({
+  page,
+}) => {
+  await page.goto('/');
+
+  // Navigate to inner lazy route first
+  const navigationToInner = page.locator('id=navigation');
+  await expect(navigationToInner).toBeVisible();
+
   // First, navigate to the inner lazy route
   const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
     return (
@@ -117,11 +125,6 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
     );
   });
 
-  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;
@@ -135,6 +138,10 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
   expect(firstEvent.type).toBe('transaction');
   expect(firstEvent.contexts?.trace?.op).toBe('navigation');
 
+  // 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();
+
   // Now navigate from the inner lazy route to another lazy route
   const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
     return (
@@ -144,9 +151,6 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
     );
   });
 
-  // 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;
@@ -159,4 +163,103 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
   expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId');
   expect(secondEvent.type).toBe('transaction');
   expect(secondEvent.contexts?.trace?.op).toBe('navigation');
+
+  // Go back to the previous page to ensure history navigation works as expected
+  const goBackTransactionPromise = 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.goBack();
+
+  const goBackEvent = await goBackTransactionPromise;
+
+  // Validate the second go back transaction event
+  expect(goBackEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
+  expect(goBackEvent.type).toBe('transaction');
+  expect(goBackEvent.contexts?.trace?.op).toBe('navigation');
+
+  // Navigate to the upper route
+  const goUpperRouteTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
+    return (
+      !!transactionEvent?.transaction &&
+      transactionEvent.contexts?.trace?.op === 'navigation' &&
+      transactionEvent.transaction === '/lazy/inner/:id/:anotherId'
+    );
+  });
+
+  const navigationToUpper = page.locator('id=navigate-to-upper');
+
+  await navigationToUpper.click();
+
+  const goUpperRouteEvent = await goUpperRouteTransactionPromise;
+
+  // Validate the go upper route transaction event
+  expect(goUpperRouteEvent.transaction).toBe('/lazy/inner/:id/:anotherId');
+  expect(goUpperRouteEvent.type).toBe('transaction');
+  expect(goUpperRouteEvent.contexts?.trace?.op).toBe('navigation');
+});
+
+test('Does not send any duplicate navigation transaction names browsing between different routes', async ({ page }) => {
+  const transactionNamesList: string[] = [];
+
+  // Monitor and add all transaction names sent to Sentry for the navigations
+  const allTransactionsPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
+    if (transactionEvent?.transaction) {
+      transactionNamesList.push(transactionEvent.transaction);
+    }
+
+    if (transactionNamesList.length >= 5) {
+      // Stop monitoring once we have enough transaction names
+      return true;
+    }
+
+    return false;
+  });
+
+  // Go to root page
+  await page.goto('/');
+  page.waitForTimeout(1000);
+
+  // Navigate to inner lazy route
+  const navigationToInner = page.locator('id=navigation');
+  await expect(navigationToInner).toBeVisible();
+  await navigationToInner.click();
+
+  // Navigate to another lazy route
+  const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
+  await expect(navigationToAnother).toBeVisible();
+  await page.waitForTimeout(1000);
+
+  // Click to navigate to another lazy route
+  await navigationToAnother.click();
+  const anotherLazyRouteContent = page.locator('id=another-lazy-route-deep');
+  await expect(anotherLazyRouteContent).toBeVisible();
+  await page.waitForTimeout(1000);
+
+  // Navigate back to inner lazy route
+  await page.goBack();
+  await expect(page.locator('id=innermost-lazy-route')).toBeVisible();
+  await page.waitForTimeout(1000);
+
+  // Navigate to upper inner lazy route
+  const navigationToUpper = page.locator('id=navigate-to-upper');
+  await expect(navigationToUpper).toBeVisible();
+  await navigationToUpper.click();
+
+  await page.waitForTimeout(1000);
+
+  await allTransactionsPromise;
+
+  expect(transactionNamesList.length).toBe(5);
+  expect(transactionNamesList).toEqual([
+    '/',
+    '/lazy/inner/:id/:anotherId/:someAnotherId',
+    '/another-lazy/sub/:id/:subId',
+    '/lazy/inner/:id/:anotherId/:someAnotherId',
+    '/lazy/inner/:id/:anotherId',
+  ]);
 });

packages/react/src/reactrouter-compat-utils/index.ts

@@ -9,8 +9,6 @@ export {
   createV6CompatibleWrapCreateMemoryRouter,
   createV6CompatibleWrapUseRoutes,
   handleNavigation,
-  handleExistingNavigationSpan,
-  createNewNavigationSpan,
   addResolvedRoutesToParent,
   processResolvedRoutes,
   updateNavigationSpan,
@@ -21,7 +19,6 @@ export {
   resolveRouteNameAndSource,
   getNormalizedName,
   initializeRouterUtils,
-  isLikelyLazyRouteContext,
   locationIsInsideDescendantRoute,
   prefixWithSlash,
   rebuildRoutePathFromAllRoutes,

packages/react/src/reactrouter-compat-utils/instrumentation.tsx

@@ -44,7 +44,6 @@ import { checkRouteForAsyncHandler } from './lazy-routes';
 import {
   getNormalizedName,
   initializeRouterUtils,
-  isLikelyLazyRouteContext,
   locationIsInsideDescendantRoute,
   prefixWithSlash,
   rebuildRoutePathFromAllRoutes,
@@ -176,12 +175,7 @@ export function updateNavigationSpan(
   // 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__;
+    !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
@@ -355,13 +349,7 @@ export function createV6CompatibleWrapCreateMemoryRouter<
       : router.state.location;
 
     if (router.state.historyAction === 'POP' && activeRootSpan) {
-      updatePageloadTransaction({
-        activeRootSpan,
-        location,
-        routes,
-        basename,
-        allRoutes: Array.from(allRoutes),
-      });
+      updatePageloadTransaction({ activeRootSpan, location, routes, basename, allRoutes: Array.from(allRoutes) });
     }
 
     router.subscribe((state: RouterState) => {
@@ -389,11 +377,7 @@ export function createReactRouterV6CompatibleTracingIntegration(
   options: Parameters<typeof browserTracingIntegration>[0] & ReactRouterOptions,
   version: V6CompatibleVersion,
 ): Integration {
-  const integration = browserTracingIntegration({
-    ...options,
-    instrumentPageLoad: false,
-    instrumentNavigation: false,
-  });
+  const integration = browserTracingIntegration({ ...options, instrumentPageLoad: false, instrumentNavigation: false });
 
   const {
     useEffect,
@@ -532,13 +516,7 @@ function wrapPatchRoutesOnNavigation(
             if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') {
               updateNavigationSpan(
                 activeRootSpan,
-                {
-                  pathname: targetPath,
-                  search: '',
-                  hash: '',
-                  state: null,
-                  key: 'default',
-                },
+                { pathname: targetPath, search: '', hash: '', state: null, key: 'default' },
                 Array.from(allRoutes),
                 true, // forceUpdate = true since we're loading lazy routes
                 _matchRoutes,
@@ -559,13 +537,7 @@ function wrapPatchRoutesOnNavigation(
         if (pathname) {
           updateNavigationSpan(
             activeRootSpan,
-            {
-              pathname,
-              search: '',
-              hash: '',
-              state: null,
-              key: 'default',
-            },
+            { pathname, search: '', hash: '', state: null, key: 'default' },
             Array.from(allRoutes),
             false, // forceUpdate = false since this is after lazy routes are loaded
             _matchRoutes,
@@ -604,18 +576,20 @@ 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, isLazyRouteContext);
-    } else {
-      createNewNavigationSpan(client, name, source, version, isLazyRouteContext);
+    if (!isAlreadyInNavigationSpan) {
+      startBrowserTracingNavigationSpan(client, {
+        name,
+        attributes: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
+        },
+      });
     }
   }
 }
@@ -726,13 +700,7 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
           });
           isMountRenderPass.current = false;
         } else {
-          handleNavigation({
-            location,
-            routes,
-            navigationType,
-            version,
-            allRoutes: Array.from(allRoutes),
-          });
+          handleNavigation({ location, routes, navigationType, version, allRoutes: Array.from(allRoutes) });
         }
       },
       // `props.children` is purposely not included in the dependency array, because we do not want to re-run this effect
@@ -765,69 +733,3 @@ function getActiveRootSpan(): Span | undefined {
   // Only use this root span if it is a pageload or navigation span
   return op === 'navigation' || op === 'pageload' ? rootSpan : undefined;
 }
-
-/**
- * Handles updating an existing navigation span
- */
-export function handleExistingNavigationSpan(
-  activeSpan: Span,
-  spanJson: ReturnType<typeof spanToJSON>,
-  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,
-      );
-    }
-  }
-
-  // Always set the source attribute to keep it consistent with the current route
-  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,
-    );
-  }
-}