Skip to content
Merged
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 @@ -11,6 +11,7 @@ import {
useNavigationType,
} from 'react-router-dom';
import Index from './pages/Index';
import Products from './pages/Products';
import SSE from './pages/SSE';
import User from './pages/User';

Expand Down Expand Up @@ -51,6 +52,7 @@ root.render(
<SentryRoutes>
<Route path="/" element={<Index />} />
<Route path="/user/:id" element={<User />} />
<Route path="/products" element={<Products />} />
<Route path="/sse" element={<SSE />} />
</SentryRoutes>
</BrowserRouter>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const Index = () => {
<Link to="/user/5" id="navigation">
navigate
</Link>
<Link to="/products" id="navigation-products">
products
</Link>
</>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as React from 'react';

const Products = () => {
// Fired on mount, i.e. while navigating to /products. This mirrors a typical
// route component that loads its data in an effect. The request is same-origin,
// so the SDK attaches `sentry-trace`/`baggage` headers by default.
React.useEffect(() => {
fetch('/api/products').catch(() => {
// ignore network errors in the test environment
});
}, []);

return <div id="products">Products</div>;
};

export default Products;
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/test-utils';

test('propagates the navigation trace (not the stale pageload trace) for a fetch in a route mount effect', async ({
page,
}) => {
// Intercept the /products data fetch and capture the tracing header the SDK attached.
let productsRequestSentryTrace: string | undefined;
await page.route('**/api/products', async route => {
productsRequestSentryTrace = route.request().headers()['sentry-trace'];
await route.fulfill({
status: 200,
contentType: 'application/json',
body: '[]',
});
});

const pageloadTxnPromise = waitForTransaction('react-router-6', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

const navigationTxnPromise = waitForTransaction('react-router-6', async transactionEvent => {
return transactionEvent.contexts?.trace?.op === 'navigation' && transactionEvent.transaction === '/products';
});

await page.goto('/');
const pageloadTxn = await pageloadTxnPromise;

await page.locator('id=navigation-products').click();
const navigationTxn = await navigationTxnPromise;

const pageloadTraceId = pageloadTxn.contexts?.trace?.trace_id;
const navigationTraceId = navigationTxn.contexts?.trace?.trace_id;
const propagatedTraceId = productsRequestSentryTrace?.split('-')[0];

expect(pageloadTraceId).toBeDefined();
expect(navigationTraceId).toBeDefined();
expect(propagatedTraceId).toBeDefined();
expect(navigationTraceId).not.toEqual(pageloadTraceId);

// The fetch fired on /products must carry the navigation trace, not the stale pageload trace.
expect(propagatedTraceId).toEqual(navigationTraceId);
expect(propagatedTraceId).not.toEqual(pageloadTraceId);
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
useNavigationType,
} from 'react-router';
import Index from './pages/Index';
import Products from './pages/Products';
import SSE from './pages/SSE';
import User from './pages/User';

Expand Down Expand Up @@ -50,6 +51,7 @@ root.render(
<SentryRoutes>
<Route path="/" element={<Index />} />
<Route path="/user/:id" element={<User />} />
<Route path="/products" element={<Products />} />
<Route path="/sse" element={<SSE />} />
</SentryRoutes>
</BrowserRouter>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const Index = () => {
<Link to="/user/5" id="navigation">
navigate
</Link>
<Link to="/products" id="navigation-products">
products
</Link>
</>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as React from 'react';

const Products = () => {
// Fired on mount, i.e. while navigating to /products. This mirrors a typical
// route component that loads its data in an effect. The request is same-origin,
// so the SDK attaches `sentry-trace`/`baggage` headers by default.
React.useEffect(() => {
fetch('/api/products').catch(() => {
// ignore network errors in the test environment
});
}, []);

return <div id="products">Products</div>;
};

export default Products;
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/test-utils';

test('propagates the navigation trace (not the stale pageload trace) for a fetch in a route mount effect', async ({
page,
}) => {
// Intercept the /products data fetch and capture the tracing header the SDK attached.
let productsRequestSentryTrace: string | undefined;
await page.route('**/api/products', async route => {
productsRequestSentryTrace = route.request().headers()['sentry-trace'];
await route.fulfill({
status: 200,
contentType: 'application/json',
body: '[]',
});
});

const pageloadTxnPromise = waitForTransaction('react-router-7-spa', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

const navigationTxnPromise = waitForTransaction('react-router-7-spa', async transactionEvent => {
return transactionEvent.contexts?.trace?.op === 'navigation' && transactionEvent.transaction === '/products';
});

await page.goto('/');
const pageloadTxn = await pageloadTxnPromise;

await page.locator('id=navigation-products').click();
const navigationTxn = await navigationTxnPromise;

const pageloadTraceId = pageloadTxn.contexts?.trace?.trace_id;
const navigationTraceId = navigationTxn.contexts?.trace?.trace_id;
const propagatedTraceId = productsRequestSentryTrace?.split('-')[0];

expect(pageloadTraceId).toBeDefined();
expect(navigationTraceId).toBeDefined();
expect(propagatedTraceId).toBeDefined();
expect(navigationTraceId).not.toEqual(pageloadTraceId);

// The fetch fired on /products must carry the navigation trace, not the stale pageload trace.
expect(propagatedTraceId).toEqual(navigationTraceId);
expect(propagatedTraceId).not.toEqual(pageloadTraceId);
});
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ let _basename: string = '';

const CLIENTS_WITH_INSTRUMENT_NAVIGATION = new WeakSet<Client>();

// Detect navigations in a layout effect so the navigation trace is set up before child route components'
// passive mount effects fire requests (else they propagate the stale pageload trace).
// Guard on `document` (present in real browsers and jsdom) so we only fall back to `useEffect` in true
// SSR, where `useLayoutEffect` warns and effects don't run anyway.
const useIsomorphicLayoutEffect = WINDOW?.document ? React.useLayoutEffect : React.useEffect;
Comment thread
chargome marked this conversation as resolved.

// Prevents duplicate spans when router.subscribe fires multiple times
const activeNavigationSpans = new WeakMap<
Client,
Expand Down Expand Up @@ -768,7 +774,7 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio
const stableLocationParam =
typeof locationArg === 'string' || locationArg?.pathname ? (locationArg as { pathname: string }) : location;

_useEffect(() => {
useIsomorphicLayoutEffect(() => {
const normalizedLocation =
typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam;

Expand Down Expand Up @@ -1351,7 +1357,7 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
const location = _useLocation();
const navigationType = _useNavigationType();

_useEffect(
useIsomorphicLayoutEffect(
() => {
const routes = _createRoutesFromChildren(props.children) as RouteObject[];

Expand Down
52 changes: 51 additions & 1 deletion packages/react/test/reactrouterv6.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
setCurrentClient,
} from '@sentry/core';
import { render } from '@testing-library/react';
import { fireEvent, render } from '@testing-library/react';
import * as React from 'react';
import type { RouteObject } from 'react-router-6';
import {
Expand All @@ -23,6 +23,7 @@ import {
RouterProvider,
Routes,
useLocation,
useNavigate,
useNavigationType,
useRoutes,
} from 'react-router-6';
Expand Down Expand Up @@ -335,6 +336,55 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
});
});

it('starts the navigation span before the navigated-to route component runs its mount effect', () => {
const client = createMockBrowserClient();
setCurrentClient(client);

client.addIntegration(
reactRouterV6BrowserTracingIntegration({
useEffect: React.useEffect,
useLocation,
useNavigationType,
createRoutesFromChildren,
matchRoutes,
}),
);
const SentryRoutes = withSentryReactRouterV6Routing(Routes);

let navigationSpanCallsAtChildMount: number | undefined;
function NavigatedRoute(): React.ReactElement {
React.useEffect(() => {
navigationSpanCallsAtChildMount = mockStartBrowserTracingNavigationSpan.mock.calls.length;
}, []);
return <div>Navigated</div>;
}

function Home(): React.ReactElement {
const navigate = useNavigate();
return (
<button type="button" onClick={() => navigate('/about')}>
to about
</button>
);
}

const { getByText } = render(
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="/" element={<Home />} />
<Route path="/about" element={<NavigatedRoute />} />
</SentryRoutes>
</MemoryRouter>,
);

// Navigate via a user click (matching real usage) - not `<Navigate>`, which would redirect
// from within a passive effect and not exercise the same commit ordering.
fireEvent.click(getByText('to about'));

expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
expect(navigationSpanCallsAtChildMount).toBe(1);
});

it('works with nested routes', () => {
const client = createMockBrowserClient();
setCurrentClient(client);
Expand Down
Loading