Skip to content

Commit da06b4f

Browse files
Lms24Lms24
committed
fix tests and incorrect trailing slash behaviour
1 parent e40b701 commit da06b4f

6 files changed

Lines changed: 120 additions & 66 deletions

File tree

‎packages/browser/src/tracing/browserTracingIntegration.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -430,12 +430,9 @@ export const browserTracingIntegration = ((options: Partial<BrowserTracingOption
430430
// For pageloads (and manual navigation spans without a URL) we fall back to the current location.
431431
const urlObject = parseStringToURLObject(url || getLocationHref());
432432

433-
const pathName = urlObject?.pathname?.replace(/\/$/, '');
434-
const fullUrl = urlObject && !isURLObjectRelative(urlObject) && urlObject.href.replace(/\/$/, '');
435-
436433
const attributes = {
437-
...(pathName && { [URL_PATH]: pathName }),
438-
...(fullUrl && { [URL_FULL]: fullUrl }),
434+
...(urlObject?.pathname && { [URL_PATH]: urlObject.pathname }),
435+
...(urlObject && !isURLObjectRelative(urlObject) && { [URL_FULL]: urlObject.href }),
439436
...finalStartSpanOptions.attributes,
440437
};
441438

‎packages/react-router/src/client/createClientInstrumentation.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
import { startBrowserTracingNavigationSpan } from '@sentry/browser';
2-
import { getAbsoluteUrl } from '@sentry/react';
1+
import { getAbsoluteUrl, startBrowserTracingNavigationSpan } from '@sentry/browser';
32
import type { Span } from '@sentry/core';
43
import {
54
debug,
@@ -18,7 +17,7 @@ import {
1817
import { DEBUG_BUILD } from '../common/debug-build';
1918
import type { ClientInstrumentation, InstrumentableRoute, InstrumentableRouter } from '../common/types';
2019
import { captureInstrumentationError, getPathFromRequest, getPattern, normalizeRoutePath } from '../common/utils';
21-
import { resolveNavigateArg } from './utils';
20+
import { resolveNavigateArg, resolveNavigateUrl } from './utils';
2221
import { URL_TEMPLATE } from '@sentry/conventions/attributes';
2322

2423
const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;
@@ -195,7 +194,7 @@ export function createSentryClientInstrumentation(
195194
'navigation.type': 'router.navigate',
196195
},
197196
},
198-
{ url: getAbsoluteUrl(info.to) },
197+
{ url: getAbsoluteUrl(resolveNavigateUrl(info.to)) },
199198
);
200199
}
201200

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
import { startBrowserTracingNavigationSpan } from '@sentry/browser';
2-
import { getAbsoluteUrl } from '@sentry/react';
1+
import { getAbsoluteUrl, startBrowserTracingNavigationSpan } from '@sentry/browser';
32
import type { Span } from '@sentry/core';
43
import {
54
debug,
@@ -15,7 +14,7 @@ import {
1514
import type { DataRouter, RouterState } from 'react-router';
1615
import { DEBUG_BUILD } from '../common/debug-build';
1716
import { isClientInstrumentationApiUsed } from './createClientInstrumentation';
18-
import { resolveNavigateArg } from './utils';
17+
import { resolveNavigateArg, resolveNavigateUrl } from './utils';
1918
import { URL_TEMPLATE } from '@sentry/conventions/attributes';
2019

2120
const GLOBAL_OBJ_WITH_DATA_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & {
@@ -63,7 +62,11 @@ export function instrumentHydratedRouter(): void {
6362
router.navigate = function sentryPatchedNavigate(...args) {
6463
// Skip if instrumentation API is enabled (it handles navigation spans itself)
6564
if (!isClientInstrumentationApiUsed()) {
66-
maybeCreateNavigationTransaction(resolveNavigateArg(args[0]) || '<unknown route>', 'url');
65+
maybeCreateNavigationTransaction(
66+
resolveNavigateArg(args[0]) || '<unknown route>',
67+
resolveNavigateUrl(args[0]),
68+
'url',
69+
);
6770
}
6871
return originalNav(...args);
6972
};
@@ -125,7 +128,7 @@ export function instrumentHydratedRouter(): void {
125128
}
126129
}
127130

128-
function maybeCreateNavigationTransaction(name: string, source: 'url' | 'route'): Span | undefined {
131+
function maybeCreateNavigationTransaction(name: string, url: string, source: 'url' | 'route'): Span | undefined {
129132
const client = getClient();
130133

131134
if (!client) {
@@ -143,7 +146,7 @@ function maybeCreateNavigationTransaction(name: string, source: 'url' | 'route')
143146
...(source === 'route' ? { [URL_TEMPLATE]: name } : {}),
144147
},
145148
},
146-
{ url: getAbsoluteUrl(name) },
149+
{ url: getAbsoluteUrl(url) },
147150
);
148151
}
149152

‎packages/react-router/src/client/utils.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { GLOBAL_OBJ } from '@sentry/core';
22

3+
const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;
4+
35
/**
46
* Resolves a navigate argument to a pathname string.
57
*
@@ -20,5 +22,23 @@ export function resolveNavigateArg(target: unknown): string {
2022
}
2123

2224
// Object `to` without pathname - navigation stays on current path
23-
return (GLOBAL_OBJ as typeof GLOBAL_OBJ & Window).location?.pathname || '/';
25+
return WINDOW.location?.pathname || '/';
26+
}
27+
28+
/**
29+
* Resolves a navigate argument to the full destination path, preserving `search`/`hash` from a
30+
* To object. Unlike `resolveNavigateArg` (used for span/route naming, which should stay a bare
31+
* path), this is used to derive `url.full`/`url.path`, which should reflect the actual
32+
* destination the user is navigating to, including any query string.
33+
*/
34+
export function resolveNavigateUrl(target: unknown): string {
35+
if (typeof target !== 'object' || target === null) {
36+
// string or number
37+
return String(target);
38+
}
39+
40+
const { pathname, search, hash } = target as Record<string, unknown>;
41+
const path = typeof pathname === 'string' && pathname !== '' ? pathname : WINDOW.location?.pathname || '/';
42+
43+
return `${path}${typeof search === 'string' ? search : ''}${typeof hash === 'string' ? hash : ''}`;
2444
}

‎packages/react-router/test/client/createClientInstrumentation.test.ts‎

Lines changed: 67 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ vi.mock('@sentry/core', async () => {
2727

2828
vi.mock('@sentry/browser', () => ({
2929
startBrowserTracingNavigationSpan: vi.fn().mockReturnValue({ setStatus: vi.fn() }),
30+
getAbsoluteUrl: vi.fn((urlOrPath: string) => urlOrPath),
3031
}));
3132

3233
describe('createSentryClientInstrumentation', () => {
@@ -92,15 +93,19 @@ describe('createSentryClientInstrumentation', () => {
9293
to: '/about',
9394
});
9495

95-
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, {
96-
name: '/about',
97-
attributes: expect.objectContaining({
98-
'sentry.source': 'url',
99-
'sentry.op': 'navigation',
100-
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
101-
'navigation.type': 'router.navigate',
102-
}),
103-
});
96+
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(
97+
mockClient,
98+
{
99+
name: '/about',
100+
attributes: expect.objectContaining({
101+
'sentry.source': 'url',
102+
'sentry.op': 'navigation',
103+
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
104+
'navigation.type': 'router.navigate',
105+
}),
106+
},
107+
{ url: '/about' },
108+
);
104109
expect(mockCallNavigate).toHaveBeenCalled();
105110
});
106111

@@ -123,15 +128,20 @@ describe('createSentryClientInstrumentation', () => {
123128
to: { pathname: '/items/123', search: '?foo=bar' },
124129
});
125130

126-
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, {
127-
name: '/items/123',
128-
attributes: expect.objectContaining({
129-
'sentry.source': 'url',
130-
'sentry.op': 'navigation',
131-
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
132-
'navigation.type': 'router.navigate',
133-
}),
134-
});
131+
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(
132+
mockClient,
133+
{
134+
name: '/items/123',
135+
attributes: expect.objectContaining({
136+
'sentry.source': 'url',
137+
'sentry.op': 'navigation',
138+
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
139+
'navigation.type': 'router.navigate',
140+
}),
141+
},
142+
// the destination URL keeps the query string, even though the span name doesn't
143+
{ url: '/items/123?foo=bar' },
144+
);
135145
expect(mockCallNavigate).toHaveBeenCalled();
136146
});
137147

@@ -374,15 +384,19 @@ describe('createSentryClientInstrumentation', () => {
374384

375385
await hooks.navigate(mockCallNavigate, { currentUrl: '/current-page', to });
376386

377-
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, {
378-
name: '/current-page',
379-
attributes: expect.objectContaining({
380-
'sentry.source': 'url',
381-
'sentry.op': 'navigation',
382-
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
383-
'navigation.type': expectedType,
384-
}),
385-
});
387+
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(
388+
mockClient,
389+
{
390+
name: '/current-page',
391+
attributes: expect.objectContaining({
392+
'sentry.source': 'url',
393+
'sentry.op': 'navigation',
394+
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
395+
'navigation.type': expectedType,
396+
}),
397+
},
398+
{ url: '/current-page' },
399+
);
386400
expect(mockNavigationSpan.updateName).toHaveBeenCalledWith(destination);
387401
},
388402
);
@@ -608,15 +622,19 @@ describe('createSentryClientInstrumentation', () => {
608622

609623
popstateHandler!();
610624

611-
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, {
612-
name: '/current-page',
613-
attributes: expect.objectContaining({
614-
'sentry.source': 'url',
615-
'sentry.op': 'navigation',
616-
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
617-
'navigation.type': 'browser.popstate',
618-
}),
619-
});
625+
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(
626+
mockClient,
627+
{
628+
name: '/current-page',
629+
attributes: expect.objectContaining({
630+
'sentry.source': 'url',
631+
'sentry.op': 'navigation',
632+
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
633+
'navigation.type': 'browser.popstate',
634+
}),
635+
},
636+
{ url: '/current-page' },
637+
);
620638
});
621639

622640
it('should not create span on popstate when no client is available', () => {
@@ -671,12 +689,16 @@ describe('createSentryClientInstrumentation', () => {
671689
// Direct popstate without navigate(-1) - simulates browser back button click
672690
popstateHandler!();
673691

674-
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(mockClient, {
675-
name: '/current-page',
676-
attributes: expect.objectContaining({
677-
'navigation.type': 'browser.popstate',
678-
}),
679-
});
692+
expect(browser.startBrowserTracingNavigationSpan).toHaveBeenCalledWith(
693+
mockClient,
694+
{
695+
name: '/current-page',
696+
attributes: expect.objectContaining({
697+
'navigation.type': 'browser.popstate',
698+
}),
699+
},
700+
{ url: '/current-page' },
701+
);
680702
});
681703
});
682704
});
@@ -762,7 +784,7 @@ describe('navigation root parameterization', () => {
762784
});
763785

764786
it('renames the active navigation/pageload root span with the route pattern from the loader hook', async () => {
765-
const mockRootSpan = { setAttribute: vi.fn() };
787+
const mockRootSpan = { setAttributes: vi.fn() };
766788
(core.getActiveSpan as any).mockReturnValue({});
767789
(core.getRootSpan as any).mockReturnValue(mockRootSpan);
768790
(core.spanToJSON as any).mockReturnValue({ op: 'navigation' });
@@ -780,11 +802,11 @@ describe('navigation root parameterization', () => {
780802
});
781803

782804
expect(core.updateSpanName).toHaveBeenCalledWith(mockRootSpan, '/users/:id');
783-
expect(mockRootSpan.setAttribute).toHaveBeenCalledWith('sentry.source', 'route');
805+
expect(mockRootSpan.setAttributes).toHaveBeenCalledWith({ 'sentry.source': 'route', 'url.template': '/users/:id' });
784806
});
785807

786808
it('does not rename the root span when the route has no pattern', async () => {
787-
const mockRootSpan = { setAttribute: vi.fn() };
809+
const mockRootSpan = { setAttributes: vi.fn() };
788810
(core.getActiveSpan as any).mockReturnValue({});
789811
(core.getRootSpan as any).mockReturnValue(mockRootSpan);
790812
(core.spanToJSON as any).mockReturnValue({ op: 'navigation' });

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ vi.mock('@sentry/core', async () => {
2323
});
2424
vi.mock('@sentry/browser', () => ({
2525
startBrowserTracingNavigationSpan: vi.fn(),
26+
getAbsoluteUrl: vi.fn((urlOrPath: string) => urlOrPath),
2627
}));
2728

2829
describe('instrumentHydratedRouter', () => {
@@ -94,7 +95,10 @@ describe('instrumentHydratedRouter', () => {
9495
(core.getActiveSpan as any).mockReturnValue(mockNavigationSpan);
9596
callback(newState);
9697
expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/foo/:id');
97-
expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
98+
expect(mockNavigationSpan.setAttributes).toHaveBeenCalledWith({
99+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
100+
'url.template': '/foo/:id',
101+
});
98102
});
99103

100104
it('does not overwrite pageload origin when the pageload is still active', () => {
@@ -112,11 +116,14 @@ describe('instrumentHydratedRouter', () => {
112116
(core.getActiveSpan as any).mockReturnValue(mockPageloadSpan);
113117
callback(newState);
114118
// Subscribe callback must not touch the navigation span, and must not write `origin` on the
115-
// pageload — only `source` via the single-attribute setter. The pageload origin was already
116-
// set by trySubscribe.
119+
// pageload — only `source`/`url.template` via the attribute setter. The pageload origin was
120+
// already set by trySubscribe.
117121
expect(mockNavigationSpan.setAttribute).not.toHaveBeenCalled();
118122
expect(mockNavigationSpan.setAttributes).not.toHaveBeenCalled();
119-
expect(mockPageloadSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
123+
expect(mockPageloadSpan.setAttributes).toHaveBeenLastCalledWith({
124+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
125+
'url.template': '/foo/:id',
126+
});
120127
});
121128

122129
it('skips the subscribe heuristic when the API is active and a route hook already set source:route', () => {
@@ -165,7 +172,10 @@ describe('instrumentHydratedRouter', () => {
165172
callback(newState);
166173

167174
expect(mockNavigationSpan.updateName).toHaveBeenCalledWith('/foo/:id');
168-
expect(mockNavigationSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
175+
expect(mockNavigationSpan.setAttributes).toHaveBeenCalledWith({
176+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
177+
'url.template': '/foo/:id',
178+
});
169179

170180
delete (globalThis as any).__sentryReactRouterClientInstrumentationUsed;
171181
});
@@ -210,6 +220,8 @@ describe('instrumentHydratedRouter', () => {
210220
expect.objectContaining({
211221
name: '/items/123',
212222
}),
223+
// the destination URL keeps the query string, even though the span name doesn't
224+
{ url: '/items/123?foo=bar' },
213225
);
214226
});
215227

@@ -221,6 +233,7 @@ describe('instrumentHydratedRouter', () => {
221233
expect.objectContaining({
222234
name: '-1',
223235
}),
236+
{ url: '-1' },
224237
);
225238
});
226239

0 commit comments

Comments
 (0)