Skip to content

Commit fc64c47

Browse files
authored
fix(react): Add POP guard for long-running pageload spans (#17867)
This resolves the issue that occurs when an extra `navigation` transaction is created after a prematurely ended `pageload` transaction in React Router lazy routes. This apparently occurs when there's a long-running pageload with lazy-routes (after fetching assets, there are multiple potentially long-running API calls happening). This causes the `pageload` transaction to prematurely end, even before the fully parameterized transaction name is resolved. The reason is that there can be a `POP` event emitted, which we subscribe to create a `navigation` transaction. This ends the ongoing `pageload` transaction before its name is updated with a resolved parameterized route path, and starts a `navigation` transaction, which contains the remaining spans that were supposed to be a part of the `pageload` transaction. This fix makes sure the initial `POP` events are not necessarily treated as `navigation` pointers, which should fix both: - Duplicate / extra `navigation` transactions having a part of `pageload` spans. - Remaining wildcards in the `pageload` transaction names
1 parent 05122e0 commit fc64c47

File tree

5 files changed

+248
-28
lines changed

5 files changed

+248
-28
lines changed

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,12 @@ const router = sentryCreateBrowserRouter(
5555
lazyChildren: () => import('./pages/AnotherLazyRoutes').then(module => module.anotherNestedRoutes),
5656
},
5757
},
58+
{
59+
path: '/long-running',
60+
handle: {
61+
lazyChildren: () => import('./pages/LongRunningLazyRoutes').then(module => module.longRunningNestedRoutes),
62+
},
63+
},
5864
{
5965
path: '/static',
6066
element: <>Hello World</>,

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ const Index = () => {
1515
<Link to="/another-lazy/sub/555/666" id="navigation-to-another-deep">
1616
Navigate to Another Deep Lazy Route
1717
</Link>
18+
<br />
19+
<Link to="/long-running/slow/12345" id="navigation-to-long-running">
20+
Navigate to Long Running Lazy Route
21+
</Link>
1822
</>
1923
);
2024
};
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import React, { useEffect, useState } from 'react';
2+
import { Link, useParams } from 'react-router-dom';
3+
4+
// Component that simulates a long-running component load
5+
// This is used to test the POP guard during long-running pageloads
6+
const SlowLoadingComponent = () => {
7+
const { id } = useParams<{ id: string }>();
8+
const [data, setData] = useState<string | null>(null);
9+
const [isLoading, setIsLoading] = useState(true);
10+
11+
useEffect(() => {
12+
// Simulate a component that takes time to initialize
13+
// This extends the pageload duration to create a window where POP events might occur
14+
setTimeout(() => {
15+
setData(`Data loaded for ID: ${id}`);
16+
setIsLoading(false);
17+
}, 1000);
18+
}, [id]);
19+
20+
if (isLoading) {
21+
return <div id="loading-indicator">Loading...</div>;
22+
}
23+
24+
return (
25+
<div id="slow-loading-content">
26+
<div>{data}</div>
27+
<Link to="/" id="navigate-home">
28+
Go Home
29+
</Link>
30+
</div>
31+
);
32+
};
33+
34+
export const longRunningNestedRoutes = [
35+
{
36+
path: 'slow',
37+
children: [
38+
{
39+
path: ':id',
40+
element: <SlowLoadingComponent />,
41+
loader: async () => {
42+
// Simulate slow data fetching in the loader
43+
await new Promise(resolve => setTimeout(resolve, 2000));
44+
return null;
45+
},
46+
},
47+
],
48+
},
49+
];

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

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,86 @@ test('Does not send any duplicate navigation transaction names browsing between
294294
'/lazy/inner/:id/:anotherId',
295295
]);
296296
});
297+
298+
test('Does not create premature navigation transaction during long-running lazy route pageload', async ({ page }) => {
299+
const navigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
300+
return (
301+
!!transactionEvent?.transaction &&
302+
transactionEvent.contexts?.trace?.op === 'navigation' &&
303+
transactionEvent.transaction.includes('long-running')
304+
);
305+
});
306+
307+
const pageloadPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
308+
return (
309+
!!transactionEvent?.transaction &&
310+
transactionEvent.contexts?.trace?.op === 'pageload' &&
311+
transactionEvent.transaction === '/long-running/slow/:id'
312+
);
313+
});
314+
315+
await page.goto('/long-running/slow/12345');
316+
317+
const pageloadEvent = await pageloadPromise;
318+
319+
expect(pageloadEvent.transaction).toBe('/long-running/slow/:id');
320+
expect(pageloadEvent.contexts?.trace?.op).toBe('pageload');
321+
322+
const slowLoadingContent = page.locator('id=slow-loading-content');
323+
await expect(slowLoadingContent).toBeVisible({ timeout: 5000 });
324+
325+
const result = await Promise.race([
326+
navigationPromise.then(() => 'navigation'),
327+
new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 2000)),
328+
]);
329+
330+
// Should timeout, meaning no unwanted navigation transaction was created
331+
expect(result).toBe('timeout');
332+
});
333+
334+
test('Allows legitimate POP navigation (back/forward) after pageload completes', async ({ page }) => {
335+
await page.goto('/');
336+
337+
const navigationToLongRunning = page.locator('id=navigation-to-long-running');
338+
await expect(navigationToLongRunning).toBeVisible();
339+
340+
const firstNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
341+
return (
342+
!!transactionEvent?.transaction &&
343+
transactionEvent.contexts?.trace?.op === 'navigation' &&
344+
transactionEvent.transaction === '/long-running/slow/:id'
345+
);
346+
});
347+
348+
await navigationToLongRunning.click();
349+
350+
const slowLoadingContent = page.locator('id=slow-loading-content');
351+
await expect(slowLoadingContent).toBeVisible({ timeout: 5000 });
352+
353+
const firstNavigationEvent = await firstNavigationPromise;
354+
355+
expect(firstNavigationEvent.transaction).toBe('/long-running/slow/:id');
356+
expect(firstNavigationEvent.contexts?.trace?.op).toBe('navigation');
357+
358+
// Now navigate back using browser back button (POP event)
359+
// This should create a navigation transaction since pageload is complete
360+
const backNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
361+
return (
362+
!!transactionEvent?.transaction &&
363+
transactionEvent.contexts?.trace?.op === 'navigation' &&
364+
transactionEvent.transaction === '/'
365+
);
366+
});
367+
368+
await page.goBack();
369+
370+
// Verify we're back at home
371+
const homeLink = page.locator('id=navigation');
372+
await expect(homeLink).toBeVisible();
373+
374+
const backNavigationEvent = await backNavigationPromise;
375+
376+
// Validate that the back navigation (POP) was properly tracked
377+
expect(backNavigationEvent.transaction).toBe('/');
378+
expect(backNavigationEvent.contexts?.trace?.op).toBe('navigation');
379+
});

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

Lines changed: 106 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,12 @@ export function createV6CompatibleWrapCreateBrowserRouter<
241241

242242
const activeRootSpan = getActiveRootSpan();
243243

244+
// Track whether we've completed the initial pageload to properly distinguish
245+
// between POPs that occur during pageload vs. legitimate back/forward navigation.
246+
let isInitialPageloadComplete = false;
247+
let hasSeenPageloadSpan = !!activeRootSpan && spanToJSON(activeRootSpan).op === 'pageload';
248+
let hasSeenPopAfterPageload = false;
249+
244250
// The initial load ends when `createBrowserRouter` is called.
245251
// This is the earliest convenient time to update the transaction name.
246252
// Callbacks to `router.subscribe` are not called for the initial load.
@@ -255,20 +261,31 @@ export function createV6CompatibleWrapCreateBrowserRouter<
255261
}
256262

257263
router.subscribe((state: RouterState) => {
258-
if (state.historyAction === 'PUSH' || state.historyAction === 'POP') {
259-
// Wait for the next render if loading an unsettled route
260-
if (state.navigation.state !== 'idle') {
261-
requestAnimationFrame(() => {
262-
handleNavigation({
263-
location: state.location,
264-
routes,
265-
navigationType: state.historyAction,
266-
version,
267-
basename,
268-
allRoutes: Array.from(allRoutes),
269-
});
270-
});
271-
} else {
264+
// Track pageload completion to distinguish POPs during pageload from legitimate back/forward navigation
265+
if (!isInitialPageloadComplete) {
266+
const currentRootSpan = getActiveRootSpan();
267+
const isCurrentlyInPageload = currentRootSpan && spanToJSON(currentRootSpan).op === 'pageload';
268+
269+
if (isCurrentlyInPageload) {
270+
hasSeenPageloadSpan = true;
271+
} else if (hasSeenPageloadSpan) {
272+
// Pageload span was active but is now gone - pageload has completed
273+
if (state.historyAction === 'POP' && !hasSeenPopAfterPageload) {
274+
// Pageload ended: ignore the first POP after pageload
275+
hasSeenPopAfterPageload = true;
276+
} else {
277+
// Pageload ended: either non-POP action or subsequent POP
278+
isInitialPageloadComplete = true;
279+
}
280+
}
281+
// If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
282+
}
283+
284+
const shouldHandleNavigation =
285+
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);
286+
287+
if (shouldHandleNavigation) {
288+
const navigationHandler = (): void => {
272289
handleNavigation({
273290
location: state.location,
274291
routes,
@@ -277,6 +294,13 @@ export function createV6CompatibleWrapCreateBrowserRouter<
277294
basename,
278295
allRoutes: Array.from(allRoutes),
279296
});
297+
};
298+
299+
// Wait for the next render if loading an unsettled route
300+
if (state.navigation.state !== 'idle') {
301+
requestAnimationFrame(navigationHandler);
302+
} else {
303+
navigationHandler();
280304
}
281305
}
282306
});
@@ -327,7 +351,6 @@ export function createV6CompatibleWrapCreateMemoryRouter<
327351
const router = createRouterFunction(routes, wrappedOpts);
328352
const basename = opts?.basename;
329353

330-
const activeRootSpan = getActiveRootSpan();
331354
let initialEntry = undefined;
332355

333356
const initialEntries = opts?.initialEntries;
@@ -348,21 +371,68 @@ export function createV6CompatibleWrapCreateMemoryRouter<
348371
: initialEntry
349372
: router.state.location;
350373

351-
if (router.state.historyAction === 'POP' && activeRootSpan) {
352-
updatePageloadTransaction({ activeRootSpan, location, routes, basename, allRoutes: Array.from(allRoutes) });
374+
const memoryActiveRootSpan = getActiveRootSpan();
375+
376+
if (router.state.historyAction === 'POP' && memoryActiveRootSpan) {
377+
updatePageloadTransaction({
378+
activeRootSpan: memoryActiveRootSpan,
379+
location,
380+
routes,
381+
basename,
382+
allRoutes: Array.from(allRoutes),
383+
});
353384
}
354385

386+
// Track whether we've completed the initial pageload to properly distinguish
387+
// between POPs that occur during pageload vs. legitimate back/forward navigation.
388+
let isInitialPageloadComplete = false;
389+
let hasSeenPageloadSpan = !!memoryActiveRootSpan && spanToJSON(memoryActiveRootSpan).op === 'pageload';
390+
let hasSeenPopAfterPageload = false;
391+
355392
router.subscribe((state: RouterState) => {
393+
// Track pageload completion to distinguish POPs during pageload from legitimate back/forward navigation
394+
if (!isInitialPageloadComplete) {
395+
const currentRootSpan = getActiveRootSpan();
396+
const isCurrentlyInPageload = currentRootSpan && spanToJSON(currentRootSpan).op === 'pageload';
397+
398+
if (isCurrentlyInPageload) {
399+
hasSeenPageloadSpan = true;
400+
} else if (hasSeenPageloadSpan) {
401+
// Pageload span was active but is now gone - pageload has completed
402+
if (state.historyAction === 'POP' && !hasSeenPopAfterPageload) {
403+
// Pageload ended: ignore the first POP after pageload
404+
hasSeenPopAfterPageload = true;
405+
} else {
406+
// Pageload ended: either non-POP action or subsequent POP
407+
isInitialPageloadComplete = true;
408+
}
409+
}
410+
// If we haven't seen a pageload span yet, keep waiting (don't mark as complete)
411+
}
412+
356413
const location = state.location;
357-
if (state.historyAction === 'PUSH' || state.historyAction === 'POP') {
358-
handleNavigation({
359-
location,
360-
routes,
361-
navigationType: state.historyAction,
362-
version,
363-
basename,
364-
allRoutes: Array.from(allRoutes),
365-
});
414+
415+
const shouldHandleNavigation =
416+
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);
417+
418+
if (shouldHandleNavigation) {
419+
const navigationHandler = (): void => {
420+
handleNavigation({
421+
location,
422+
routes,
423+
navigationType: state.historyAction,
424+
version,
425+
basename,
426+
allRoutes: Array.from(allRoutes),
427+
});
428+
};
429+
430+
// Wait for the next render if loading an unsettled route
431+
if (state.navigation.state !== 'idle') {
432+
requestAnimationFrame(navigationHandler);
433+
} else {
434+
navigationHandler();
435+
}
366436
}
367437
});
368438

@@ -532,8 +602,16 @@ function wrapPatchRoutesOnNavigation(
532602
// Update navigation span after routes are patched
533603
const activeRootSpan = getActiveRootSpan();
534604
if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') {
535-
// For memory routers, we should not access window.location; use targetPath only
536-
const pathname = isMemoryRouter ? targetPath : targetPath || WINDOW.location?.pathname;
605+
// Determine pathname based on router type
606+
let pathname: string | undefined;
607+
if (isMemoryRouter) {
608+
// For memory routers, only use targetPath
609+
pathname = targetPath;
610+
} else {
611+
// For browser routers, use targetPath or fall back to window.location
612+
pathname = targetPath || WINDOW.location?.pathname;
613+
}
614+
537615
if (pathname) {
538616
updateNavigationSpan(
539617
activeRootSpan,

0 commit comments

Comments
 (0)