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..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 @@ -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', diff --git a/packages/react-router/src/client/hydratedRouter.ts b/packages/react-router/src/client/hydratedRouter.ts index f63a60d4a234..adce54afc2d2 100644 --- a/packages/react-router/src/client/hydratedRouter.ts +++ b/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; diff --git a/packages/react-router/test/client/hydratedRouter.test.ts b/packages/react-router/test/client/hydratedRouter.test.ts index b5a5fec1f84d..e02af8e9f55d 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'); @@ -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', () => {