Address review comments

Author: onurtemizkan

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

@@ -504,7 +504,7 @@ test('Updates navigation transaction name correctly when span is cancelled early
 test('Creates separate transactions for rapid consecutive navigations', async ({ page }) => {
   await page.goto('/');
 
-  // First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId
+  // Set up transaction listeners
   const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
     return (
       !!transactionEvent?.transaction &&
@@ -513,21 +513,6 @@ test('Creates separate transactions for rapid consecutive navigations', async ({
     );
   });
 
-  const navigationToInner = page.locator('id=navigation');
-  await expect(navigationToInner).toBeVisible();
-  await navigationToInner.click();
-
-  const firstEvent = await firstTransactionPromise;
-
-  // Verify first transaction
-  expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
-  expect(firstEvent.contexts?.trace?.op).toBe('navigation');
-  expect(firstEvent.contexts?.trace?.status).toBe('ok');
-
-  const firstTraceId = firstEvent.contexts?.trace?.trace_id;
-  const firstSpanId = firstEvent.contexts?.trace?.span_id;
-
-  // Second navigation: /lazy/inner -> /another-lazy/sub/:id/:subId
   const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
     return (
       !!transactionEvent?.transaction &&
@@ -536,37 +521,49 @@ test('Creates separate transactions for rapid consecutive navigations', async ({
     );
   });
 
-  const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
-  await expect(navigationToAnother).toBeVisible();
-  await navigationToAnother.click();
+  // Third navigation promise - using counter to match second occurrence of same route
+  let innerRouteMatchCount = 0;
+  const thirdTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
+    if (
+      transactionEvent?.transaction &&
+      transactionEvent.contexts?.trace?.op === 'navigation' &&
+      transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
+    ) {
+      innerRouteMatchCount++;
+      return innerRouteMatchCount === 2; // Match the second occurrence
+    }
+    return false;
+  });
+
+  // Perform navigations
+  // First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId
+  await page.locator('id=navigation').click();
+
+  const firstEvent = await firstTransactionPromise;
+
+  // Second navigation: /lazy/inner -> /another-lazy/sub/:id/:subId
+  await page.locator('id=navigate-to-another-from-inner').click();
 
   const secondEvent = await secondTransactionPromise;
 
-  // Verify second transaction
+  // Third navigation: /another-lazy -> /lazy/inner/:id/:anotherId/:someAnotherId (back to same route as first)
+  await page.locator('id=navigate-to-inner-from-deep').click();
+
+  const thirdEvent = await thirdTransactionPromise;
+
+  // Verify transactions
+  expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
+  expect(firstEvent.contexts?.trace?.op).toBe('navigation');
+  const firstTraceId = firstEvent.contexts?.trace?.trace_id;
+  const firstSpanId = firstEvent.contexts?.trace?.span_id;
+
   expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId');
   expect(secondEvent.contexts?.trace?.op).toBe('navigation');
   expect(secondEvent.contexts?.trace?.status).toBe('ok');
 
   const secondTraceId = secondEvent.contexts?.trace?.trace_id;
   const secondSpanId = secondEvent.contexts?.trace?.span_id;
 
-  // Third navigation: /another-lazy -> /lazy/inner/:id/:anotherId/:someAnotherId (back to same route as first)
-  const thirdTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
-    return (
-      !!transactionEvent?.transaction &&
-      transactionEvent.contexts?.trace?.op === 'navigation' &&
-      transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' &&
-      // Ensure we're not matching the first transaction again
-      transactionEvent.contexts?.trace?.trace_id !== firstTraceId
-    );
-  });
-
-  const navigationBackToInner = page.locator('id=navigate-to-inner-from-deep');
-  await expect(navigationBackToInner).toBeVisible();
-  await navigationBackToInner.click();
-
-  const thirdEvent = await thirdTransactionPromise;
-
   // Verify third transaction
   expect(thirdEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
   expect(thirdEvent.contexts?.trace?.op).toBe('navigation');

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

@@ -76,6 +76,26 @@ export function addResolvedRoutesToParent(resolvedRoutes: RouteObject[], parentR
   }
 }
 
+/**
+ * Determines if a navigation should be handled based on router state.
+ * Only handles:
+ * - PUSH navigations (always)
+ * - POP navigations (only after initial pageload is complete)
+ * - When router state is 'idle' (not 'loading' or 'submitting')
+ *
+ * During 'loading' or 'submitting', state.location may still have the old pathname,
+ * which would cause us to create a span for the wrong route.
+ */
+function shouldHandleNavigation(
+  state: { historyAction: string; navigation: { state: string } },
+  isInitialPageloadComplete: boolean,
+): boolean {
+  return (
+    (state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) &&
+    state.navigation.state === 'idle'
+  );
+}
+
 export interface ReactRouterOptions {
   useEffect: UseEffect;
   useLocation: UseLocation;
@@ -277,14 +297,7 @@ export function createV6CompatibleWrapCreateBrowserRouter<
         // If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
       }
 
-      // Only handle navigation when it's complete (state is idle).
-      // During 'loading' or 'submitting', state.location may still have the old pathname,
-      // which would cause us to create a span for the wrong route.
-      const shouldHandleNavigation =
-        (state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) &&
-        state.navigation.state === 'idle';
-
-      if (shouldHandleNavigation) {
+      if (shouldHandleNavigation(state, isInitialPageloadComplete)) {
         handleNavigation({
           location: state.location,
           routes,
@@ -401,14 +414,7 @@ export function createV6CompatibleWrapCreateMemoryRouter<
         // If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
       }
 
-      // Only handle navigation when it's complete (state is idle).
-      // During 'loading' or 'submitting', state.location may still have the old pathname,
-      // which would cause us to create a span for the wrong route.
-      const shouldHandleNavigation =
-        (state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) &&
-        state.navigation.state === 'idle';
-
-      if (shouldHandleNavigation) {
+      if (shouldHandleNavigation(state, isInitialPageloadComplete)) {
         handleNavigation({
           location: state.location,
           routes,
@@ -622,8 +628,8 @@ function tryUpdateSpanName(
   newName: string,
   newSource: string,
 ): void {
-  const isNewNameBetter = newName !== currentSpanName && newName.includes(':');
-  if (isNewNameBetter) {
+  const isNewNameParameterized = newName !== currentSpanName && newName.includes(':');
+  if (isNewNameParameterized) {
     activeSpan.updateName(newName);
     activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, newSource as 'route' | 'url' | 'custom');
   }