Skip to content
Open
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 @@ -55,6 +55,12 @@ const router = sentryCreateBrowserRouter(
lazyChildren: () => import('./pages/AnotherLazyRoutes').then(module => module.anotherNestedRoutes),
},
},
{
path: '/long-running',
handle: {
lazyChildren: () => import('./pages/LongRunningLazyRoutes').then(module => module.longRunningNestedRoutes),
},
},
{
path: '/static',
element: <>Hello World</>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ const Index = () => {
<Link to="/another-lazy/sub/555/666" id="navigation-to-another-deep">
Navigate to Another Deep Lazy Route
</Link>
<br />
<Link to="/long-running/slow/12345" id="navigation-to-long-running">
Navigate to Long Running Lazy Route
</Link>
</>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React, { useEffect, useState } from 'react';
import { Link, useParams } from 'react-router-dom';

// Component that simulates a long-running component load
// This is used to test the POP guard during long-running pageloads
const SlowLoadingComponent = () => {
const { id } = useParams<{ id: string }>();
const [data, setData] = useState<string | null>(null);
const [isLoading, setIsLoading] = useState(true);

useEffect(() => {
// Simulate a component that takes time to initialize
// This extends the pageload duration to create a window where POP events might occur
setTimeout(() => {
setData(`Data loaded for ID: ${id}`);
setIsLoading(false);
}, 1000);
}, [id]);

if (isLoading) {
return <div id="loading-indicator">Loading...</div>;
}

return (
<div id="slow-loading-content">
<div>{data}</div>
<Link to="/" id="navigate-home">
Go Home
</Link>
</div>
);
};

export const longRunningNestedRoutes = [
{
path: 'slow',
children: [
{
path: ':id',
element: <SlowLoadingComponent />,
loader: async () => {
// Simulate slow data fetching in the loader
await new Promise(resolve => setTimeout(resolve, 2000));
return null;
},
},
],
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -294,3 +294,86 @@ test('Does not send any duplicate navigation transaction names browsing between
'/lazy/inner/:id/:anotherId',
]);
});

test('Does not create premature navigation transaction during long-running lazy route pageload', async ({ page }) => {
const navigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction.includes('long-running')
);
});

const pageloadPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'pageload' &&
transactionEvent.transaction === '/long-running/slow/:id'
);
});

await page.goto('/long-running/slow/12345');

const pageloadEvent = await pageloadPromise;

expect(pageloadEvent.transaction).toBe('/long-running/slow/:id');
expect(pageloadEvent.contexts?.trace?.op).toBe('pageload');

const slowLoadingContent = page.locator('id=slow-loading-content');
await expect(slowLoadingContent).toBeVisible({ timeout: 5000 });

const result = await Promise.race([
navigationPromise.then(() => 'navigation'),
new Promise<'timeout'>(resolve => setTimeout(() => resolve('timeout'), 2000)),
]);

// Should timeout, meaning no unwanted navigation transaction was created
expect(result).toBe('timeout');
});

test('Allows legitimate POP navigation (back/forward) after pageload completes', async ({ page }) => {
await page.goto('/');

const navigationToLongRunning = page.locator('id=navigation-to-long-running');
await expect(navigationToLongRunning).toBeVisible();

const firstNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction === '/long-running/slow/:id'
);
});

await navigationToLongRunning.click();

const slowLoadingContent = page.locator('id=slow-loading-content');
await expect(slowLoadingContent).toBeVisible({ timeout: 5000 });

const firstNavigationEvent = await firstNavigationPromise;

expect(firstNavigationEvent.transaction).toBe('/long-running/slow/:id');
expect(firstNavigationEvent.contexts?.trace?.op).toBe('navigation');

// Now navigate back using browser back button (POP event)
// This should create a navigation transaction since pageload is complete
const backNavigationPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
return (
!!transactionEvent?.transaction &&
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.transaction === '/'
);
});

await page.goBack();

// Verify we're back at home
const homeLink = page.locator('id=navigation');
await expect(homeLink).toBeVisible();

const backNavigationEvent = await backNavigationPromise;

// Validate that the back navigation (POP) was properly tracked
expect(backNavigationEvent.transaction).toBe('/');
expect(backNavigationEvent.contexts?.trace?.op).toBe('navigation');
});
92 changes: 69 additions & 23 deletions packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ export function createV6CompatibleWrapCreateBrowserRouter<

const activeRootSpan = getActiveRootSpan();

// Track whether we've completed the initial pageload to properly distinguish
// between POPs that occur during pageload vs. legitimate back/forward navigation.
let isInitialPageloadComplete = false;

// The initial load ends when `createBrowserRouter` is called.
// This is the earliest convenient time to update the transaction name.
// Callbacks to `router.subscribe` are not called for the initial load.
Expand All @@ -255,20 +259,25 @@ export function createV6CompatibleWrapCreateBrowserRouter<
}

router.subscribe((state: RouterState) => {
if (state.historyAction === 'PUSH' || state.historyAction === 'POP') {
// Wait for the next render if loading an unsettled route
if (state.navigation.state !== 'idle') {
requestAnimationFrame(() => {
handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
});
} else {
// Check if pageload span has ended to mark completion
if (!isInitialPageloadComplete) {
const currentRootSpan = getActiveRootSpan();
const isStillInPageload = currentRootSpan && spanToJSON(currentRootSpan).op === 'pageload';

if (!isStillInPageload) {
isInitialPageloadComplete = true;
// Don't handle this specific callback if it's a POP that marks completion
if (state.historyAction === 'POP') {
return;
}
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Race Condition in Pageload Completion Logic

The pageload completion logic has a race condition. It can prematurely mark pageload as complete if the root span is missing, not necessarily because pageload finished. This causes legitimate POP navigation events immediately following pageload to be incorrectly ignored.

Fix in Cursor Fix in Web


const shouldHandleNavigation =
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);

if (shouldHandleNavigation) {
const navigationHandler = (): void => {
handleNavigation({
location: state.location,
routes,
Expand All @@ -277,6 +286,13 @@ export function createV6CompatibleWrapCreateBrowserRouter<
basename,
allRoutes: Array.from(allRoutes),
});
};

// Wait for the next render if loading an unsettled route
if (state.navigation.state !== 'idle') {
requestAnimationFrame(navigationHandler);
} else {
navigationHandler();
}
}
});
Expand Down Expand Up @@ -352,17 +368,47 @@ export function createV6CompatibleWrapCreateMemoryRouter<
updatePageloadTransaction({ activeRootSpan, location, routes, basename, allRoutes: Array.from(allRoutes) });
}

// Track whether we've completed the initial pageload to properly distinguish
// between POPs that occur during pageload vs. legitimate back/forward navigation.
let isInitialPageloadComplete = false;

router.subscribe((state: RouterState) => {
// Check if pageload span has ended to mark completion
if (!isInitialPageloadComplete) {
const currentRootSpan = getActiveRootSpan();
const isStillInPageload = currentRootSpan && spanToJSON(currentRootSpan).op === 'pageload';

if (!isStillInPageload) {
isInitialPageloadComplete = true;
// Don't handle this specific callback if it's a POP that marks completion
if (state.historyAction === 'POP') {
return;
}
}
}

const location = state.location;
if (state.historyAction === 'PUSH' || state.historyAction === 'POP') {
handleNavigation({
location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
const shouldHandleNavigation =
state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete);

if (shouldHandleNavigation) {
const navigationHandler = (): void => {
handleNavigation({
location,
routes,
navigationType: state.historyAction,
version,
basename,
allRoutes: Array.from(allRoutes),
});
};

// Wait for the next render if loading an unsettled route
if (state.navigation.state !== 'idle') {
requestAnimationFrame(navigationHandler);
} else {
navigationHandler();
}
}
});

Expand Down
Loading