Skip to content

Commit fd26569

Browse files
authored
fix(react): Patch spanEnd for potentially cancelled lazy-route transactions (#17962)
Fixes an issue where `pageload` and `navigation` transactions have incorrect (URL-based or wildcard-based) names when the span is cancelled early before lazy routes finish loading. This occurs when `document.hidden` triggers early span cancellation (e.g., user switches tabs during page load). In React Router applications with lazy routes, the parameterized route information may not be available yet when the span ends, resulting in transaction names like `/user/123/edit` (URL-based) or `/projects/*/views/*` (wildcard-based) instead of the correct parameterized route like `/user/:id/edit` or `/projects/:projectId/views/:viewId`. This fix patches `span.end()` to perform a final route resolution check before the span is sent, using the live global `allRoutes` Set to capture any lazy routes that loaded after the span was created but before it ended.
1 parent 9c0397c commit fd26569

File tree

3 files changed

+254
-23
lines changed

3 files changed

+254
-23
lines changed

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,3 +377,124 @@ test('Allows legitimate POP navigation (back/forward) after pageload completes',
377377
expect(backNavigationEvent.transaction).toBe('/');
378378
expect(backNavigationEvent.contexts?.trace?.op).toBe('navigation');
379379
});
380+
381+
test('Updates pageload transaction name correctly when span is cancelled early (document.hidden simulation)', async ({
382+
page,
383+
}) => {
384+
const transactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
385+
return (
386+
!!transactionEvent?.transaction &&
387+
transactionEvent.contexts?.trace?.op === 'pageload' &&
388+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
389+
);
390+
});
391+
392+
// Set up the page to simulate document.hidden before navigation
393+
await page.addInitScript(() => {
394+
// Wait a bit for Sentry to initialize and start the pageload span
395+
setTimeout(() => {
396+
// Override document.hidden to simulate tab switching
397+
Object.defineProperty(document, 'hidden', {
398+
configurable: true,
399+
get: function () {
400+
return true;
401+
},
402+
});
403+
404+
// Dispatch visibilitychange event to trigger the idle span cancellation logic
405+
document.dispatchEvent(new Event('visibilitychange'));
406+
}, 100); // Small delay to ensure the span has started
407+
});
408+
409+
// Navigate to the lazy route URL
410+
await page.goto('/lazy/inner/1/2/3');
411+
412+
const event = await transactionPromise;
413+
414+
// Verify the lazy route content eventually loads (even though span was cancelled early)
415+
const lazyRouteContent = page.locator('id=innermost-lazy-route');
416+
await expect(lazyRouteContent).toBeVisible();
417+
418+
// Validate that the transaction event has the correct parameterized route name
419+
// even though the span was cancelled early due to document.hidden
420+
expect(event.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
421+
expect(event.type).toBe('transaction');
422+
expect(event.contexts?.trace?.op).toBe('pageload');
423+
424+
// Check if the span was indeed cancelled (should have idle_span_finish_reason attribute)
425+
const idleSpanFinishReason = event.contexts?.trace?.data?.['sentry.idle_span_finish_reason'];
426+
if (idleSpanFinishReason) {
427+
// If the span was cancelled due to visibility change, verify it still got the right name
428+
expect(['externalFinish', 'cancelled']).toContain(idleSpanFinishReason);
429+
}
430+
});
431+
432+
test('Updates navigation transaction name correctly when span is cancelled early (document.hidden simulation)', async ({
433+
page,
434+
}) => {
435+
// First go to home page
436+
await page.goto('/');
437+
438+
const navigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
439+
return (
440+
!!transactionEvent?.transaction &&
441+
transactionEvent.contexts?.trace?.op === 'navigation' &&
442+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
443+
);
444+
});
445+
446+
// Set up a listener to simulate document.hidden after clicking the navigation link
447+
await page.evaluate(() => {
448+
// Override document.hidden to simulate tab switching
449+
let hiddenValue = false;
450+
Object.defineProperty(document, 'hidden', {
451+
configurable: true,
452+
get: function () {
453+
return hiddenValue;
454+
},
455+
});
456+
457+
// Listen for clicks on the navigation link and simulate document.hidden shortly after
458+
document.addEventListener(
459+
'click',
460+
() => {
461+
setTimeout(() => {
462+
hiddenValue = true;
463+
// Dispatch visibilitychange event to trigger the idle span cancellation logic
464+
document.dispatchEvent(new Event('visibilitychange'));
465+
}, 50); // Small delay to ensure the navigation span has started
466+
},
467+
{ once: true },
468+
);
469+
});
470+
471+
// Click the navigation link to navigate to the lazy route
472+
const navigationLink = page.locator('id=navigation');
473+
await expect(navigationLink).toBeVisible();
474+
await navigationLink.click();
475+
476+
const event = await navigationPromise;
477+
478+
// Verify the lazy route content eventually loads (even though span was cancelled early)
479+
const lazyRouteContent = page.locator('id=innermost-lazy-route');
480+
await expect(lazyRouteContent).toBeVisible();
481+
482+
// Validate that the transaction event has the correct parameterized route name
483+
// even though the span was cancelled early due to document.hidden
484+
expect(event.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
485+
expect(event.type).toBe('transaction');
486+
expect(event.contexts?.trace?.op).toBe('navigation');
487+
488+
// Check if the span was indeed cancelled (should have cancellation_reason attribute or idle_span_finish_reason)
489+
const cancellationReason = event.contexts?.trace?.data?.['sentry.cancellation_reason'];
490+
const idleSpanFinishReason = event.contexts?.trace?.data?.['sentry.idle_span_finish_reason'];
491+
492+
// Verify that the span was cancelled due to document.hidden
493+
if (cancellationReason) {
494+
expect(cancellationReason).toBe('document.hidden');
495+
}
496+
497+
if (idleSpanFinishReason) {
498+
expect(['externalFinish', 'cancelled']).toContain(idleSpanFinishReason);
499+
}
500+
});

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

Lines changed: 96 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
startBrowserTracingPageLoadSpan,
99
WINDOW,
1010
} from '@sentry/browser';
11-
import type { Client, Integration, Span, TransactionSource } from '@sentry/core';
11+
import type { Client, Integration, Span } from '@sentry/core';
1212
import {
1313
addNonEnumerableProperty,
1414
debug,
@@ -41,14 +41,7 @@ import type {
4141
UseRoutes,
4242
} from '../types';
4343
import { checkRouteForAsyncHandler } from './lazy-routes';
44-
import {
45-
getNormalizedName,
46-
initializeRouterUtils,
47-
locationIsInsideDescendantRoute,
48-
prefixWithSlash,
49-
rebuildRoutePathFromAllRoutes,
50-
resolveRouteNameAndSource,
51-
} from './utils';
44+
import { initializeRouterUtils, resolveRouteNameAndSource } from './utils';
5245

5346
let _useEffect: UseEffect;
5447
let _useLocation: UseLocation;
@@ -668,14 +661,19 @@ export function handleNavigation(opts: {
668661

669662
// Cross usage can result in multiple navigation spans being created without this check
670663
if (!isAlreadyInNavigationSpan) {
671-
startBrowserTracingNavigationSpan(client, {
664+
const navigationSpan = startBrowserTracingNavigationSpan(client, {
672665
name,
673666
attributes: {
674667
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
675668
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
676669
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
677670
},
678671
});
672+
673+
// Patch navigation span to handle early cancellation (e.g., document.hidden)
674+
if (navigationSpan) {
675+
patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes);
676+
}
679677
}
680678
}
681679
}
@@ -729,29 +727,104 @@ function updatePageloadTransaction({
729727
: (_matchRoutes(allRoutes || routes, location, basename) as unknown as RouteMatch[]);
730728

731729
if (branches) {
732-
let name,
733-
source: TransactionSource = 'url';
734-
735-
const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes || routes);
736-
737-
if (isInDescendantRoute) {
738-
name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location));
739-
source = 'route';
740-
}
741-
742-
if (!isInDescendantRoute || !name) {
743-
[name, source] = getNormalizedName(routes, location, branches, basename);
744-
}
730+
const [name, source] = resolveRouteNameAndSource(location, routes, allRoutes || routes, branches, basename);
745731

746732
getCurrentScope().setTransactionName(name || '/');
747733

748734
if (activeRootSpan) {
749735
activeRootSpan.updateName(name);
750736
activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
737+
738+
// Patch span.end() to ensure we update the name one last time before the span is sent
739+
patchPageloadSpanEnd(activeRootSpan, location, routes, basename, allRoutes);
751740
}
752741
}
753742
}
754743

744+
/**
745+
* Patches the span.end() method to update the transaction name one last time before the span is sent.
746+
* This handles cases where the span is cancelled early (e.g., document.hidden) before lazy routes have finished loading.
747+
*/
748+
function patchSpanEnd(
749+
span: Span,
750+
location: Location,
751+
routes: RouteObject[],
752+
basename: string | undefined,
753+
_allRoutes: RouteObject[] | undefined,
754+
spanType: 'pageload' | 'navigation',
755+
): void {
756+
const patchedPropertyName = `__sentry_${spanType}_end_patched__` as const;
757+
const hasEndBeenPatched = (span as unknown as Record<string, boolean | undefined>)?.[patchedPropertyName];
758+
759+
if (hasEndBeenPatched || !span.end) {
760+
return;
761+
}
762+
763+
const originalEnd = span.end.bind(span);
764+
765+
span.end = function patchedEnd(...args) {
766+
try {
767+
// Only update if the span source is not already 'route' (i.e., it hasn't been parameterized yet)
768+
const spanJson = spanToJSON(span);
769+
const currentSource = spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
770+
if (currentSource !== 'route') {
771+
// Last chance to update the transaction name with the latest route info
772+
// Use the live global allRoutes Set to include any lazy routes loaded after patching
773+
const currentAllRoutes = Array.from(allRoutes);
774+
const branches = _matchRoutes(
775+
currentAllRoutes.length > 0 ? currentAllRoutes : routes,
776+
location,
777+
basename,
778+
) as unknown as RouteMatch[];
779+
780+
if (branches) {
781+
const [name, source] = resolveRouteNameAndSource(
782+
location,
783+
routes,
784+
currentAllRoutes.length > 0 ? currentAllRoutes : routes,
785+
branches,
786+
basename,
787+
);
788+
789+
// Only update if we have a valid name
790+
if (name && (spanType === 'pageload' || !spanJson.timestamp)) {
791+
span.updateName(name);
792+
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
793+
}
794+
}
795+
}
796+
} catch (error) {
797+
// Silently catch errors to ensure span.end() is always called
798+
DEBUG_BUILD && debug.warn(`Error updating span details before ending: ${error}`);
799+
}
800+
801+
return originalEnd(...args);
802+
};
803+
804+
// Mark this span as having its end() method patched to prevent duplicate patching
805+
addNonEnumerableProperty(span as unknown as Record<string, boolean>, patchedPropertyName, true);
806+
}
807+
808+
function patchPageloadSpanEnd(
809+
span: Span,
810+
location: Location,
811+
routes: RouteObject[],
812+
basename: string | undefined,
813+
_allRoutes: RouteObject[] | undefined,
814+
): void {
815+
patchSpanEnd(span, location, routes, basename, _allRoutes, 'pageload');
816+
}
817+
818+
function patchNavigationSpanEnd(
819+
span: Span,
820+
location: Location,
821+
routes: RouteObject[],
822+
basename: string | undefined,
823+
_allRoutes: RouteObject[] | undefined,
824+
): void {
825+
patchSpanEnd(span, location, routes, basename, _allRoutes, 'navigation');
826+
}
827+
755828
// eslint-disable-next-line @typescript-eslint/no-explicit-any
756829
export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<string, any>, R extends React.FC<P>>(
757830
Routes: R,

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,43 @@ describe('reactrouter-compat-utils/instrumentation', () => {
142142
expect(typeof integration.afterAllSetup).toBe('function');
143143
});
144144
});
145+
146+
describe('span.end() patching for early cancellation', () => {
147+
it('should update transaction name when span.end() is called during cancellation', () => {
148+
const mockEnd = vi.fn();
149+
let patchedEnd: ((...args: any[]) => any) | null = null;
150+
151+
const updateNameMock = vi.fn();
152+
const setAttributeMock = vi.fn();
153+
154+
const testSpan = {
155+
updateName: updateNameMock,
156+
setAttribute: setAttributeMock,
157+
get end() {
158+
return patchedEnd || mockEnd;
159+
},
160+
set end(fn: (...args: any[]) => any) {
161+
patchedEnd = fn;
162+
},
163+
} as unknown as Span;
164+
165+
// Simulate the patching behavior
166+
const originalEnd = testSpan.end.bind(testSpan);
167+
(testSpan as any).end = function patchedEndFn(...args: any[]) {
168+
// This simulates what happens in the actual implementation
169+
updateNameMock('Updated Route');
170+
setAttributeMock('sentry.source', 'route');
171+
return originalEnd(...args);
172+
};
173+
174+
// Call the patched end
175+
testSpan.end(12345);
176+
177+
expect(updateNameMock).toHaveBeenCalledWith('Updated Route');
178+
expect(setAttributeMock).toHaveBeenCalledWith('sentry.source', 'route');
179+
expect(mockEnd).toHaveBeenCalledWith(12345);
180+
});
181+
});
145182
});
146183

147184
describe('addRoutesToAllRoutes', () => {

0 commit comments

Comments
 (0)