fix(react-router): Do not re-write origin on router state changes

dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts

@@ -2,18 +2,22 @@ import { expect, test } from '@playwright/test';
 import { waitForTransaction } from '@sentry-internal/test-utils';
 import { APP_NAME } from '../constants';
 
-// Known React Router limitation: HydratedRouter doesn't invoke instrumentation API
-// hooks on the client-side in Framework Mode. Server-side instrumentation works.
+// When `useInstrumentationAPI: true` is set and the instrumentations array is passed to
+// HydratedRouter, React Router invokes the navigate hook on the client and the navigation span
+// is created via the instrumentation API (origin: `auto.navigation.react_router.instrumentation_api`).
+// The legacy `instrumentHydratedRouter()` subscribe callback still runs and updates the span
+// name to its parameterized form (so `sentry.source` ends up as `route`).
+//
 // See: https://github.com/remix-run/react-router/discussions/13749
-// The legacy HydratedRouter instrumentation provides fallback navigation tracking.
 
-test.describe('client - navigation fallback to legacy instrumentation', () => {
-  test('should send navigation transaction via legacy HydratedRouter instrumentation', async ({ page }) => {
+test.describe('client - hybrid navigation (instrumentation API span + legacy parameterization)', () => {
+  test('should create navigation span via instrumentation API and parameterize via legacy subscribe', async ({
+    page,
+  }) => {
     // First load the performance page
     await page.goto(`/performance`);
     await page.waitForTimeout(1000);
 
-    // Wait for the navigation transaction (from legacy instrumentation)
     const navigationTxPromise = waitForTransaction(APP_NAME, async transactionEvent => {
       return (
         transactionEvent.transaction === '/performance/ssr' && transactionEvent.contexts?.trace?.op === 'navigation'
@@ -25,13 +29,17 @@ test.describe('client - navigation fallback to legacy instrumentation', () => {
 
     const transaction = await navigationTxPromise;
 
-    // Navigation should work via legacy HydratedRouter instrumentation
-    // (not instrumentation_api since that doesn't work in Framework Mode)
     expect(transaction).toMatchObject({
       contexts: {
         trace: {
           op: 'navigation',
-          origin: 'auto.navigation.react_router', // Legacy origin, not instrumentation_api
+          origin: 'auto.navigation.react_router.instrumentation_api',
+          data: {
+            'sentry.source': 'route',
+            'sentry.op': 'navigation',
+            'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
+            'navigation.type': 'router.navigate',
+          },
         },
       },
       transaction: '/performance/ssr',
@@ -58,7 +66,7 @@ test.describe('client - navigation fallback to legacy instrumentation', () => {
       contexts: {
         trace: {
           op: 'navigation',
-          origin: 'auto.navigation.react_router',
+          origin: 'auto.navigation.react_router.instrumentation_api',
           data: {
             'sentry.source': 'route',
           },
@@ -89,7 +97,7 @@ test.describe('client - navigation fallback to legacy instrumentation', () => {
       contexts: {
         trace: {
           op: 'navigation',
-          origin: 'auto.navigation.react_router',
+          origin: 'auto.navigation.react_router.instrumentation_api',
         },
       },
       transaction: '/performance/ssr',
@@ -109,7 +117,7 @@ test.describe('client - navigation fallback to legacy instrumentation', () => {
       contexts: {
         trace: {
           op: 'navigation',
-          origin: 'auto.navigation.react_router',
+          origin: 'auto.navigation.react_router.instrumentation_api',
         },
       },
       transaction: '/performance',

packages/react-router/src/client/hydratedRouter.ts

@@ -66,28 +66,27 @@ export function instrumentHydratedRouter(): void {
         };
       }
 
-      // Subscribe to router state changes to update navigation transactions with parameterized routes
+      // Subscribe to router state changes to update navigation transactions (and any pageload
+      // whose route info only became available after `trySubscribe`, e.g. lazy routes) with the
+      // parameterized route.
       router.subscribe(newState => {
-        const navigationSpan = getActiveRootSpan();
+        const rootSpan = getActiveRootSpan();
 
-        if (!navigationSpan) {
+        if (!rootSpan) {
           return;
         }
 
-        const navigationSpanName = spanToJSON(navigationSpan).description;
-        const parameterizedNavRoute = getParameterizedRoute(newState);
+        const rootSpanName = spanToJSON(rootSpan).description;
+        const parameterizedRoute = getParameterizedRoute(newState);
 
         if (
-          navigationSpanName &&
+          rootSpanName &&
           newState.navigation.state === 'idle' && // navigation has completed
-          // this event is for the currently active navigation
-          normalizePathname(newState.location.pathname) === normalizePathname(navigationSpanName)
+          // this event is for the currently active root span
+          normalizePathname(newState.location.pathname) === normalizePathname(rootSpanName)
         ) {
-          navigationSpan.updateName(parameterizedNavRoute);
-          navigationSpan.setAttributes({
-            [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
-            [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react_router',
-          });
+          rootSpan.updateName(parameterizedRoute);
+          rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
         }
       });
       return true;

packages/react-router/test/client/hydratedRouter.test.ts

@@ -2,6 +2,7 @@ import * as browser from '@sentry/browser';
 import * as core from '@sentry/core';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import { instrumentHydratedRouter } from '../../src/client/hydratedRouter';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
 
 vi.mock('@sentry/core', async () => {
   const actual = await vi.importActual<any>('@sentry/core');
@@ -42,14 +43,15 @@ describe('instrumentHydratedRouter', () => {
     };
     (globalThis as any).__reactRouterDataRouter = mockRouter;
 
-    mockPageloadSpan = { updateName: vi.fn(), setAttributes: vi.fn() };
-    mockNavigationSpan = { updateName: vi.fn(), setAttributes: vi.fn() };
+    mockPageloadSpan = { updateName: vi.fn(), setAttributes: vi.fn(), setAttribute: vi.fn() };
+    mockNavigationSpan = { updateName: vi.fn(), setAttributes: vi.fn(), setAttribute: vi.fn() };
 
     (core.getActiveSpan as any).mockReturnValue(mockPageloadSpan);
     (core.getRootSpan as any).mockImplementation((span: any) => span);
-    (core.spanToJSON as any).mockImplementation((_span: any) => ({
+    (core.spanToJSON as any).mockImplementation((span: any) => ({
       description: '/foo/bar',
-      op: 'pageload',
+      // Distinguish so the subscribe callback can branch on op (pageload vs. navigation).
+      op: span === mockNavigationSpan ? 'navigation' : 'pageload',
     }));
     (core.getClient as any).mockReturnValue({});
     (browser.startBrowserTracingNavigationSpan as any).mockReturnValue(mockNavigationSpan);
@@ -91,8 +93,30 @@ describe('instrumentHydratedRouter', () => {
     // After navigation, the active span should be the navigation span
     (core.getActiveSpan as any).mockReturnValue(mockNavigationSpan);
     callback(newState);
-    expect(mockNavigationSpan.updateName).toHaveBeenCalled();
-    expect(mockNavigationSpan.setAttributes).toHaveBeenCalled();
+    expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/foo/:id');
+    expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
+  });
+
+  it('does not overwrite pageload origin when the pageload is still active', () => {
+    // Regression test for #20784: a static-route pageload (where pathname == rootSpanName) was
+    // being tagged with `origin: auto.navigation.react_router` because the subscribe callback
+    // re-wrote origin unconditionally, even when the active root span was still the pageload.
+    instrumentHydratedRouter();
+    const callback = mockRouter.subscribe.mock.calls[0][0];
+    const newState = {
+      location: { pathname: '/foo/bar' },
+      matches: [{ route: { path: '/foo/:id' } }],
+      navigation: { state: 'idle' },
+    };
+    // Active root span is still the pageload (no navigation has happened yet).
+    (core.getActiveSpan as any).mockReturnValue(mockPageloadSpan);
+    callback(newState);
+    // Subscribe callback must not touch the navigation span, and must not write `origin` on the
+    // pageload — only `source` via the single-attribute setter. The pageload origin was already
+    // set by trySubscribe.
+    expect(mockNavigationSpan.setAttribute).not.toHaveBeenCalled();
+    expect(mockNavigationSpan.setAttributes).not.toHaveBeenCalled();
+    expect(mockPageloadSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
   });
 
   it('does not update navigation transaction on state change to loading', () => {