From 61b54d9d41b63d4f7ac391f611843610c843c333 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 11:49:20 +0200 Subject: [PATCH 1/7] fix(react-router): Do not re-write origin on router state changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #20784 The router state-change subscription wrote `sentry.origin: auto.navigation.react_router` to whatever root span happened to be active. When the route was static (so the parameterized path equals the URL pathname), the `pathname === rootSpanName` guard did not filter out the still-active pageload span — so the pageload got tagged with the navigation origin, producing 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. The origin override here was also incorrect for the instrumentation API path: that creates the navigation span with `auto.navigation.react_router.instrumentation_api`, and the subscribe callback was stripping the `.instrumentation_api` suffix on its way through. The origin is correctly set once at span creation (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's origin is preserved when the subscribe callback fires while it's still active, and tightened the existing navigation assertion to check the attribute payload rather than just that `setAttributes` was called. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../react-router/src/client/hydratedRouter.ts | 28 +++++++++------- .../test/client/hydratedRouter.test.ts | 32 ++++++++++++++++--- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/packages/react-router/src/client/hydratedRouter.ts b/packages/react-router/src/client/hydratedRouter.ts index f63a60d4a234..d09637bc40c4 100644 --- a/packages/react-router/src/client/hydratedRouter.ts +++ b/packages/react-router/src/client/hydratedRouter.ts @@ -66,27 +66,33 @@ export function instrumentHydratedRouter(): void { }; } - // Subscribe to router state changes to update navigation transactions with parameterized routes + // Subscribe to router state changes to update transactions (navigation, or a pageload + // whose route info wasn't yet available at `trySubscribe`) with the parameterized route. + // We deliberately do NOT touch `sentry.origin` here: the navigation span sets it at + // creation time (legacy: `auto.navigation.react_router`; instrumentation API: + // `auto.navigation.react_router.instrumentation_api`) and the pageload's origin is set + // by `trySubscribe`. Re-writing it caused a race where the subscribe callback fired + // while the pageload was still active (static routes where pathname == rootSpanName) + // and clobbered the pageload origin with the navigation origin. 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({ + rootSpan.updateName(parameterizedRoute); + rootSpan.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react_router', }); } }); diff --git a/packages/react-router/test/client/hydratedRouter.test.ts b/packages/react-router/test/client/hydratedRouter.test.ts index b5a5fec1f84d..9b66fb931746 100644 --- a/packages/react-router/test/client/hydratedRouter.test.ts +++ b/packages/react-router/test/client/hydratedRouter.test.ts @@ -47,9 +47,10 @@ describe('instrumentHydratedRouter', () => { (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 +92,31 @@ 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'); + // The subscribe callback only updates source; the origin is set at navigation-span + // creation time and must be left alone here (otherwise we'd clobber the pageload origin + // when the pageload is still the active root, and we'd strip the `.instrumentation_api` + // suffix on spans created via the instrumentation API). + expect(mockNavigationSpan.setAttributes).toHaveBeenCalledWith({ 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); + expect(mockNavigationSpan.setAttributes).not.toHaveBeenCalled(); + // No `origin` key — only `source`. The pageload origin was already set by trySubscribe. + expect(mockPageloadSpan.setAttributes).toHaveBeenLastCalledWith({ source: 'route' }); }); it('does not update navigation transaction on state change to loading', () => { From 1dc18fe630a8e916c267a6f233e585ddde70be8e Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 11:51:16 +0200 Subject: [PATCH 2/7] fix comment --- packages/react-router/src/client/hydratedRouter.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/react-router/src/client/hydratedRouter.ts b/packages/react-router/src/client/hydratedRouter.ts index d09637bc40c4..4bf37a109ba5 100644 --- a/packages/react-router/src/client/hydratedRouter.ts +++ b/packages/react-router/src/client/hydratedRouter.ts @@ -66,14 +66,9 @@ export function instrumentHydratedRouter(): void { }; } - // Subscribe to router state changes to update transactions (navigation, or a pageload - // whose route info wasn't yet available at `trySubscribe`) with the parameterized route. - // We deliberately do NOT touch `sentry.origin` here: the navigation span sets it at - // creation time (legacy: `auto.navigation.react_router`; instrumentation API: - // `auto.navigation.react_router.instrumentation_api`) and the pageload's origin is set - // by `trySubscribe`. Re-writing it caused a race where the subscribe callback fired - // while the pageload was still active (static routes where pathname == rootSpanName) - // and clobbered the pageload origin with the navigation origin. + // 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 rootSpan = getActiveRootSpan(); From de12513ba86bd93ecd6f32be6657a80f55bb7309 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 11:51:43 +0200 Subject: [PATCH 3/7] streamline attr setting --- packages/react-router/src/client/hydratedRouter.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react-router/src/client/hydratedRouter.ts b/packages/react-router/src/client/hydratedRouter.ts index 4bf37a109ba5..adce54afc2d2 100644 --- a/packages/react-router/src/client/hydratedRouter.ts +++ b/packages/react-router/src/client/hydratedRouter.ts @@ -86,9 +86,7 @@ export function instrumentHydratedRouter(): void { normalizePathname(newState.location.pathname) === normalizePathname(rootSpanName) ) { rootSpan.updateName(parameterizedRoute); - rootSpan.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - }); + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); } }); return true; From 2dc82c4a8938b85feb76ccee63fa1cb2b3c37c1a Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 11:54:06 +0200 Subject: [PATCH 4/7] fix test --- packages/react-router/test/client/hydratedRouter.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/react-router/test/client/hydratedRouter.test.ts b/packages/react-router/test/client/hydratedRouter.test.ts index 9b66fb931746..8a5b3e961b37 100644 --- a/packages/react-router/test/client/hydratedRouter.test.ts +++ b/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('@sentry/core'); @@ -93,11 +94,7 @@ describe('instrumentHydratedRouter', () => { (core.getActiveSpan as any).mockReturnValue(mockNavigationSpan); callback(newState); expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/foo/:id'); - // The subscribe callback only updates source; the origin is set at navigation-span - // creation time and must be left alone here (otherwise we'd clobber the pageload origin - // when the pageload is still the active root, and we'd strip the `.instrumentation_api` - // suffix on spans created via the instrumentation API). - expect(mockNavigationSpan.setAttributes).toHaveBeenCalledWith({ source: 'route' }); + expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); it('does not overwrite pageload origin when the pageload is still active', () => { From 7f6a3245bdd1b9a5f3459af12c1e2904d68c7602 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 11:55:01 +0200 Subject: [PATCH 5/7] test(react-router): Adapt hydratedRouter test to setAttribute(SOURCE, 'route') Source now uses the single-attribute Span#setAttribute API since only `source` is updated by the router.subscribe callback. The test mocks need a setAttribute spy, and the regression assertion has to match the new call shape. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../react-router/test/client/hydratedRouter.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-router/test/client/hydratedRouter.test.ts b/packages/react-router/test/client/hydratedRouter.test.ts index 8a5b3e961b37..e02af8e9f55d 100644 --- a/packages/react-router/test/client/hydratedRouter.test.ts +++ b/packages/react-router/test/client/hydratedRouter.test.ts @@ -43,8 +43,8 @@ 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); @@ -111,9 +111,12 @@ describe('instrumentHydratedRouter', () => { // 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(); - // No `origin` key — only `source`. The pageload origin was already set by trySubscribe. - expect(mockPageloadSpan.setAttributes).toHaveBeenLastCalledWith({ source: 'route' }); + expect(mockPageloadSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); it('does not update navigation transaction on state change to loading', () => { From 6e0874ec82b7c706c7f4065b16628f447efaa00f Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 12:30:24 +0200 Subject: [PATCH 6/7] test(react-router): Update framework-instrumentation navigation tests for true origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These tests were asserting `origin: auto.navigation.react_router`, but that legacy origin was only being produced because of a bug in the router state-change subscription that unconditionally overwrote the origin attribute. With `useInstrumentationAPI: true` and the instrumentation array passed to HydratedRouter, React Router invokes the navigate hook on the client and the navigation span is genuinely created via the instrumentation API — so the correct origin is `auto.navigation.react_router.instrumentation_api`. The subscribe callback still updates the span name to its parameterized form so `sentry.source` ends up as `route`. Renamed the describe block and updated the top-of-file comment to reflect the new understanding. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../performance/navigation.client.test.ts | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts index ed5bafad79fc..d0db68dc4e06 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts +++ b/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`). +// +// Previously these tests asserted the legacy origin `auto.navigation.react_router`, but that was +// only true because of a bug where the subscribe callback unconditionally overwrote the origin. // 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 - navigation via instrumentation API', () => { + test('should send navigation transaction with instrumentation API origin', 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,11 @@ 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', }, }, transaction: '/performance/ssr', @@ -58,7 +60,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 +91,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 +111,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', From f50d850ae4fd67996aa8d6b71261b53f0cf64617 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 20 May 2026 12:37:43 +0200 Subject: [PATCH 7/7] fixes --- .../tests/performance/navigation.client.test.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts index d0db68dc4e06..6afd6b16fb48 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-instrumentation/tests/performance/navigation.client.test.ts @@ -8,12 +8,12 @@ import { APP_NAME } from '../constants'; // The legacy `instrumentHydratedRouter()` subscribe callback still runs and updates the span // name to its parameterized form (so `sentry.source` ends up as `route`). // -// Previously these tests asserted the legacy origin `auto.navigation.react_router`, but that was -// only true because of a bug where the subscribe callback unconditionally overwrote the origin. // See: https://github.com/remix-run/react-router/discussions/13749 -test.describe('client - navigation via instrumentation API', () => { - test('should send navigation transaction with instrumentation API origin', 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); @@ -34,6 +34,12 @@ test.describe('client - navigation via instrumentation API', () => { trace: { op: 'navigation', 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',