Skip to content

Commit f65ebba

Browse files
committed
Do not downgrade from wildcard route to unparameterized route
1 parent c055e02 commit f65ebba

File tree

2 files changed

+59
-7
lines changed

2 files changed

+59
-7
lines changed

packages/react/src/reactrouter-compat-utils/instrumentation.tsx

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,14 @@ export function updateNavigationSpan(
222222
);
223223

224224
// Only update if we have a valid name and it's better than what we have
225-
const isImprovement = name && (!currentName || !name.includes('*'));
225+
// Never downgrade from route source to url source
226+
const currentSource = spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
227+
const isImprovement =
228+
name &&
229+
(!hasBeenNamed || // Span not finalized - accept any name
230+
!currentName || // No current name - always set
231+
(currentNameHasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source)
232+
(currentSource !== 'route' && source === 'route')); // URL → route upgrade
226233
if (isImprovement) {
227234
activeRootSpan.updateName(name);
228235
activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
@@ -864,9 +871,13 @@ function tryUpdateSpanNameBeforeEnd(
864871
// Only update if we have a valid name and it's better than current
865872
// Upgrade conditions:
866873
// 1. No current name exists
867-
// 2. Current name has wildcards (less specific)
874+
// 2. Current name has wildcards and new source is also 'route' (never downgrade route→url)
868875
// 3. Upgrading from non-route source to route source (e.g., URL -> parameterized route)
869-
const isImprovement = name && (!currentName || hasWildcard || (currentSource !== 'route' && source === 'route'));
876+
const isImprovement =
877+
name &&
878+
(!currentName || // No current name - always set
879+
(hasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source)
880+
(currentSource !== 'route' && source === 'route')); // URL → route upgrade
870881
const spanNotEnded = spanType === 'pageload' || !spanJson.timestamp;
871882

872883
if (isImprovement && spanNotEnded) {

packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ describe('tryUpdateSpanNameBeforeEnd - source upgrade logic', () => {
451451
expect(mockSetAttribute).toHaveBeenCalledWith('sentry.source', 'route');
452452
});
453453

454-
it('should NOT downgrade from route source to URL source', async () => {
454+
it('should not downgrade from route source to URL source', async () => {
455455
// Setup: Current span has route source with parameterized name (no wildcard)
456456
vi.mocked(spanToJSON).mockReturnValue({
457457
op: 'navigation',
@@ -479,7 +479,7 @@ describe('tryUpdateSpanNameBeforeEnd - source upgrade logic', () => {
479479
vi.fn(() => [{ route: { path: '/users/:id' } }]),
480480
);
481481

482-
// Should NOT update because span is already named
482+
// Should not update because span is already named
483483
// The early return in tryUpdateSpanNameBeforeEnd (line 815) protects against downgrades
484484
// This test verifies that route->url downgrades are blocked
485485
expect(mockUpdateName).not.toHaveBeenCalled();
@@ -494,8 +494,10 @@ describe('tryUpdateSpanNameBeforeEnd - source upgrade logic', () => {
494494
data: { 'sentry.source': 'route' },
495495
} as any);
496496

497-
// Mock wildcard detection
498-
vi.mocked(transactionNameHasWildcard).mockReturnValue(true);
497+
// Mock wildcard detection: current name has wildcard, new name doesn't
498+
vi.mocked(transactionNameHasWildcard).mockImplementation((name: string) => {
499+
return name === '/users/*'; // Only the current name has wildcard
500+
});
499501

500502
// Target: Resolves to specific parameterized route
501503
vi.mocked(resolveRouteNameAndSource).mockReturnValue(['/users/:id', 'route']);
@@ -521,6 +523,45 @@ describe('tryUpdateSpanNameBeforeEnd - source upgrade logic', () => {
521523
expect(mockSetAttribute).toHaveBeenCalledWith('sentry.source', 'route');
522524
});
523525

526+
it('should not downgrade from wildcard route to URL', async () => {
527+
// Setup: Current span has route source with wildcard
528+
vi.mocked(spanToJSON).mockReturnValue({
529+
op: 'navigation',
530+
description: '/users/*',
531+
data: { 'sentry.source': 'route' },
532+
} as any);
533+
534+
// Mock wildcard detection: current name has wildcard, new name doesn't
535+
vi.mocked(transactionNameHasWildcard).mockImplementation((name: string) => {
536+
return name === '/users/*'; // Only the current wildcard name returns true
537+
});
538+
539+
// Target: After timeout, resolves to URL (lazy route didn't finish loading)
540+
vi.mocked(resolveRouteNameAndSource).mockReturnValue(['/users/123', 'url']);
541+
542+
const mockUpdateName = vi.fn();
543+
const mockSetAttribute = vi.fn();
544+
const testSpan = {
545+
updateName: mockUpdateName,
546+
setAttribute: mockSetAttribute,
547+
end: vi.fn(),
548+
__sentry_navigation_name_set__: true, // Mark span as already named/finalized
549+
} as unknown as Span;
550+
551+
updateNavigationSpan(
552+
testSpan,
553+
{ pathname: '/users/123', search: '', hash: '', state: null, key: 'test' },
554+
[{ path: '/users/*', element: <div /> }],
555+
false,
556+
vi.fn(() => [{ route: { path: '/users/*' } }]),
557+
);
558+
559+
// Should not update - keep wildcard route instead of downgrading to URL
560+
// Wildcard routes are better than URLs for aggregation in performance monitoring
561+
expect(mockUpdateName).not.toHaveBeenCalled();
562+
expect(mockSetAttribute).not.toHaveBeenCalled();
563+
});
564+
524565
it('should set name when no current name exists', async () => {
525566
// Setup: Current span has no name (undefined)
526567
vi.mocked(spanToJSON).mockReturnValue({

0 commit comments

Comments
 (0)