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

## Summary

Closes #20784. Closes #20772.

The router state-change subscription in `hydratedRouter.ts` was
unconditionally re-writing `sentry.origin` on whatever root span was
active at the time. Two side effects:

1. **The flaky CI failure** — for static routes (where the parameterized
path equals the URL pathname), the `pathname === rootSpanName` guard
didn't filter out the still-active pageload span, so the pageload got
tagged with `auto.navigation.react_router` and produced transactions
with `op=pageload` but `origin=auto.navigation.react_router`. Dynamic
routes hid the bug because the parameterized name no longer equalled the
pathname, short-circuiting the block.
2. **An instrumentation-API regression** — when the navigation span was
created via `createClientInstrumentation`, its origin is
`auto.navigation.react_router.instrumentation_api`. The subscribe
callback's re-write was stripping the `.instrumentation_api` suffix.

The origin is correctly set once at span creation (in
`maybeCreateNavigationTransaction` / instrumentation API) or once in
`trySubscribe` for the pageload. The subscribe callback only needs to
update `name` + `source`. Drop the origin write entirely.

Added a regression test asserting the pageload origin is preserved when
the subscribe callback fires while the pageload is still the active
root, and tightened the existing navigation assertion to check the
attribute payload rather than just that `setAttributes` was called.

Also corrected the `react-router-7-framework-instrumentation` navigation
e2e tests, which were asserting the legacy origin only because of the
same bug — React Router 7 actually invokes the instrumentation API
navigate hook on the client, so the navigation spans genuinely carry the
`.instrumentation_api` origin (with the legacy subscribe callback still
doing parameterization).

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: mydea

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', () => {