Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ test.describe('client - hybrid navigation (instrumentation API span + legacy par
data: {
'sentry.source': 'route',
'url.template': '/performance/with/:param',
'navigation.route.id': 'routes/performance/dynamic-param',
'url.path': '/performance/with/sentry',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/with\/sentry$/),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ test.describe('client - instrumentation API pageload', () => {
data: {
'sentry.source': 'route',
'url.template': '/performance/with/:param',
'navigation.route.id': 'routes/performance/dynamic-param',
'url.path': '/performance/with/some-param',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/with\/some-param$/),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ test.describe('client - navigation performance', () => {
data: {
'sentry.source': 'route',
'url.template': '/performance/with/:param',
'navigation.route.id': 'routes/performance/dynamic-param',
'url.path': '/performance/with/object-nav',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/with\/object-nav\?foo=bar$/),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ test.describe('client - pageload performance', () => {
'sentry.op': 'pageload',
'sentry.source': 'route',
'url.template': '/performance/with/:param',
'navigation.route.id': 'routes/performance/dynamic-param',
'url.path': '/performance/with/sentry',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/with\/sentry$/),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
finalizeNavigationSpanFromHydratedRouter,
updateNavigationSpanUrlFromLocation,
} from './utils';
import { URL_TEMPLATE } from '@sentry/conventions/attributes';
import { NAVIGATION_ROUTE_ID, URL_TEMPLATE } from '@sentry/conventions/attributes';

const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;

Expand Down Expand Up @@ -249,7 +249,7 @@ export function createSentryClientInstrumentation(
const routePattern = pattern || urlPath;
// Parameterize the active navigation root span. (Route hooks don't fire on initial
// pageload, so this only affects navigations.)
updateRootSpanRoute(routePattern, !!pattern);
updateRootSpanRoute(routePattern, !!pattern, routeId);

await startSpan(
{
Expand All @@ -275,7 +275,7 @@ export function createSentryClientInstrumentation(
const urlPath = getPathFromRequest(info.request);
const pattern = normalizeRoutePath(getPattern(info));
const routePattern = pattern || urlPath;
updateRootSpanRoute(routePattern, !!pattern);
updateRootSpanRoute(routePattern, !!pattern, routeId);

await startSpan(
{
Expand Down Expand Up @@ -360,7 +360,7 @@ export function createSentryClientInstrumentation(
* Updates the active navigation/pageload root span name with the parameterized route, so the
* transaction reflects the parameterized route pattern (e.g. `/users/:id`).
*/
function updateRootSpanRoute(routeName: string, hasPattern: boolean): void {
function updateRootSpanRoute(routeName: string, hasPattern: boolean, routeId?: string): void {
if (!hasPattern) {
return;
}
Expand All @@ -377,7 +377,11 @@ function updateRootSpanRoute(routeName: string, hasPattern: boolean): void {
}

updateSpanName(rootSpan, routeName);
rootSpan.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [URL_TEMPLATE]: routeName });
rootSpan.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
[URL_TEMPLATE]: routeName,
...(routeId && { [NAVIGATION_ROUTE_ID]: routeId }),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-leaf route id can stick

Medium Severity

In the instrumentation-API path, navigation.route.id is taken from whichever route's loader/action hook runs, not from the leaf match. Once that sets sentry.source to route, the hydrated-router subscribe path early-returns and never corrects it via getRouteId. Nested apps where only a parent has a client loader can therefore keep a parent route id on the navigation span.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f597246. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is fine since we just follow the exisitng behavior of url.template here

}

/**
Expand Down
12 changes: 10 additions & 2 deletions packages/react-router/src/client/hydratedRouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import { isClientInstrumentationApiUsed } from './createClientInstrumentation';
import {
finalizeNavigationSpanFromRouterState,
getParameterizedRoute,
getRouteId,
normalizePathname,
resolveNavigateAbsoluteUrl,
resolveNavigateArg,
} from './utils';
import { URL_PATH, URL_TEMPLATE } from '@sentry/conventions/attributes';
import { NAVIGATION_ROUTE_ID, URL_PATH, URL_TEMPLATE } from '@sentry/conventions/attributes';

const GLOBAL_OBJ_WITH_DATA_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & {
__reactRouterDataRouter?: DataRouter;
Expand Down Expand Up @@ -56,11 +57,13 @@ export function instrumentHydratedRouter(): void {
// this event is for the currently active pageload
normalizePathname(router.state.location.pathname) === normalizePathname(pageloadName)
) {
const pageloadRouteId = getRouteId(router.state);
pageloadSpan.updateName(parameterizePageloadRoute);
pageloadSpan.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.react_router',
[URL_TEMPLATE]: parameterizePageloadRoute,
...(pageloadRouteId && { [NAVIGATION_ROUTE_ID]: pageloadRouteId }),
});
}
}
Expand Down Expand Up @@ -149,8 +152,13 @@ export function instrumentHydratedRouter(): void {
(destinationPathname === normalizePathname(rootSpanName) ||
(spanPathname && destinationPathname === normalizePathname(spanPathname)))
) {
const routeId = getRouteId(newState);
rootSpan.updateName(parameterizedRoute);
rootSpan.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [URL_TEMPLATE]: parameterizedRoute });
rootSpan.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
[URL_TEMPLATE]: parameterizedRoute,
...(routeId && { [NAVIGATION_ROUTE_ID]: routeId }),
});
}
});
return true;
Expand Down
17 changes: 15 additions & 2 deletions packages/react-router/src/client/utils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { getAbsoluteUrl } from '@sentry/browser';
import type { Span } from '@sentry/core';
import { GLOBAL_OBJ, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
import { URL_FULL, URL_PATH, URL_TEMPLATE } from '@sentry/conventions/attributes';
import { NAVIGATION_ROUTE_ID, URL_FULL, URL_PATH, URL_TEMPLATE } from '@sentry/conventions/attributes';
import type { DataRouter, RouterState } from 'react-router';

const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;
Expand Down Expand Up @@ -122,6 +122,14 @@ export function getParameterizedRoute(routerState: RouterState): string {
return normalizePathname(lastMatch?.route.path || routerState.location.pathname);
}

/**
* Returns the framework-assigned id of the leaf matched route (e.g. `routes/blog.$slug`), which
* is distinct from the parameterized path pattern used for `url.template`.
*/
export function getRouteId(routerState: RouterState): string | undefined {
return routerState.matches[routerState.matches.length - 1]?.route.id;
}

/**
* Updates a navigation span's URL attributes and parameterizes its name from the router state.
* Used after numeric navigations (`navigate(-1)` / `navigate(1)`) where route hooks may not
Expand All @@ -144,8 +152,13 @@ export function finalizeNavigationSpanFromRouterState(span: Span, routerState: R
normalizePathname(routerState.location.pathname) === normalizePathname(pathname)
) {
const parameterizedRoute = getParameterizedRoute(routerState);
const routeId = getRouteId(routerState);
span.updateName(parameterizedRoute);
span.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [URL_TEMPLATE]: parameterizedRoute });
span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
[URL_TEMPLATE]: parameterizedRoute,
...(routeId && { [NAVIGATION_ROUTE_ID]: routeId }),
});
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,11 @@ describe('navigation root parameterization', () => {
});

expect(core.updateSpanName).toHaveBeenCalledWith(mockRootSpan, '/users/:id');
expect(mockRootSpan.setAttributes).toHaveBeenCalledWith({ 'sentry.source': 'route', 'url.template': '/users/:id' });
expect(mockRootSpan.setAttributes).toHaveBeenCalledWith({
'sentry.source': 'route',
'url.template': '/users/:id',
'navigation.route.id': 'r',
});
});

it('does not rename the root span when the route has no pattern', async () => {
Expand Down
18 changes: 18 additions & 0 deletions packages/react-router/test/client/hydratedRouter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,24 @@ describe('instrumentHydratedRouter', () => {
});
});

it('sets navigation.route.id from the leaf matched route id on state change', () => {
instrumentHydratedRouter();
const callback = mockRouter.subscribe.mock.calls[0][0];
const newState = {
location: { pathname: '/foo/bar' },
matches: [{ route: { path: '/foo/:id', id: 'routes/foo.$id' } }],
navigation: { state: 'idle' },
};
mockRouter.navigate('/foo/bar');
(core.getActiveSpan as any).mockReturnValue(mockNavigationSpan);
callback(newState);
expect(mockNavigationSpan.setAttributes).toHaveBeenCalledWith({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
'url.template': '/foo/:id',
'navigation.route.id': 'routes/foo.$id',
});
});

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
Expand Down
16 changes: 16 additions & 0 deletions packages/react-router/test/client/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,22 @@ describe('finalizeNavigationSpanFromRouterState', () => {
});
});

it('sets navigation.route.id from the leaf matched route id', () => {
const span = { updateName: vi.fn(), setAttributes: vi.fn() } as any;

finalizeNavigationSpanFromRouterState(span, {
location: { pathname: '/performance/' },
matches: [{ route: { path: '', id: 'routes/performance/index' } }],
navigation: { state: 'idle' },
} as any);

expect(span.setAttributes).toHaveBeenLastCalledWith({
'sentry.source': 'route',
'url.template': '/performance',
'navigation.route.id': 'routes/performance/index',
});
});

it('sets url attributes but skips parameterization when router state is stale', () => {
const span = { updateName: vi.fn(), setAttributes: vi.fn() } as any;

Expand Down
Loading