Skip to content

Commit 63ebcd0

Browse files
onurtemizkanonurtemizkan
committed
fix(react): Prevent navigation span leaks for consecutive navigations
1 parent ea20d8d commit 63ebcd0

File tree

3 files changed

+329
-3
lines changed

3 files changed

+329
-3
lines changed

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,3 +498,54 @@ test('Updates navigation transaction name correctly when span is cancelled early
498498
expect(['externalFinish', 'cancelled']).toContain(idleSpanFinishReason);
499499
}
500500
});
501+
502+
test('Creates separate transactions for rapid consecutive navigations', async ({ page }) => {
503+
const transactionsList: Array<{ name: string; traceId: string; spanId: string }> = [];
504+
505+
const allTransactionsPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
506+
if (transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation') {
507+
transactionsList.push({
508+
name: transactionEvent.transaction,
509+
traceId: transactionEvent.contexts.trace.trace_id || '',
510+
spanId: transactionEvent.contexts.trace.span_id || '',
511+
});
512+
}
513+
514+
if (transactionsList.length >= 3) {
515+
return true;
516+
}
517+
518+
return false;
519+
});
520+
521+
await page.goto('/');
522+
523+
// Perform rapid consecutive navigations without delays between them
524+
const navigationToInner = page.locator('id=navigation');
525+
await expect(navigationToInner).toBeVisible();
526+
await navigationToInner.click();
527+
528+
const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
529+
await expect(navigationToAnother).toBeVisible();
530+
await navigationToAnother.click();
531+
532+
const navigationBackToInner = page.locator('id=navigate-to-inner-from-deep');
533+
await expect(navigationBackToInner).toBeVisible();
534+
await navigationBackToInner.click();
535+
536+
await allTransactionsPromise;
537+
538+
expect(transactionsList.length).toBe(3);
539+
expect(transactionsList[0].name).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
540+
expect(transactionsList[1].name).toBe('/another-lazy/sub/:id/:subId');
541+
expect(transactionsList[2].name).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
542+
543+
// Verify each navigation created a separate transaction with unique trace and span IDs
544+
const traceIds = transactionsList.map(t => t.traceId);
545+
const uniqueTraceIds = new Set(traceIds);
546+
expect(uniqueTraceIds.size).toBe(3);
547+
548+
const spanIds = transactionsList.map(t => t.spanId);
549+
const uniqueSpanIds = new Set(spanIds);
550+
expect(uniqueSpanIds.size).toBe(3);
551+
});

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -659,8 +659,11 @@ export function handleNavigation(opts: {
659659
const spanJson = activeSpan && spanToJSON(activeSpan);
660660
const isAlreadyInNavigationSpan = spanJson?.op === 'navigation';
661661

662-
// Cross usage can result in multiple navigation spans being created without this check
663-
if (!isAlreadyInNavigationSpan) {
662+
// Only skip creating a new span if we're already in a navigation span AND the route name matches.
663+
// This handles cross-usage (multiple wrappers for same navigation) while allowing consecutive navigations.
664+
const isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name;
665+
666+
if (!isSpanForSameRoute) {
664667
const navigationSpan = startBrowserTracingNavigationSpan(client, {
665668
name,
666669
attributes: {

‎packages/react/test/reactrouter-cross-usage.test.tsx‎

Lines changed: 273 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
1010
setCurrentClient,
1111
} from '@sentry/core';
12-
import { render } from '@testing-library/react';
12+
import { act, render, waitFor } from '@testing-library/react';
1313
import * as React from 'react';
1414
import {
1515
createMemoryRouter,
@@ -607,4 +607,276 @@ describe('React Router cross usage of wrappers', () => {
607607
});
608608
});
609609
});
610+
611+
describe('consecutive navigations to different routes', () => {
612+
it('should create separate transactions for consecutive navigations to different routes', async () => {
613+
const client = createMockBrowserClient();
614+
setCurrentClient(client);
615+
616+
client.addIntegration(
617+
reactRouterV6BrowserTracingIntegration({
618+
useEffect: React.useEffect,
619+
useLocation,
620+
useNavigationType,
621+
createRoutesFromChildren,
622+
matchRoutes,
623+
}),
624+
);
625+
626+
const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter);
627+
628+
const router = createSentryMemoryRouter(
629+
[
630+
{
631+
children: [
632+
{ path: '/users', element: <div>Users</div> },
633+
{ path: '/settings', element: <div>Settings</div> },
634+
{ path: '/profile', element: <div>Profile</div> },
635+
],
636+
},
637+
],
638+
{ initialEntries: ['/users'] },
639+
);
640+
641+
render(
642+
<React.StrictMode>
643+
<RouterProvider router={router} />
644+
</React.StrictMode>,
645+
);
646+
647+
expect(mockStartBrowserTracingNavigationSpan).not.toHaveBeenCalled();
648+
649+
await act(async () => {
650+
router.navigate('/settings');
651+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1));
652+
});
653+
654+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
655+
name: '/settings',
656+
attributes: {
657+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
658+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
659+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
660+
},
661+
});
662+
663+
await act(async () => {
664+
router.navigate('/profile');
665+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2));
666+
});
667+
668+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2);
669+
670+
const calls = mockStartBrowserTracingNavigationSpan.mock.calls;
671+
expect(calls[0]![1].name).toBe('/settings');
672+
expect(calls[1]![1].name).toBe('/profile');
673+
expect(calls[0]![1].attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('navigation');
674+
expect(calls[1]![1].attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('navigation');
675+
});
676+
677+
it('should create separate transactions for rapid consecutive navigations', async () => {
678+
const client = createMockBrowserClient();
679+
setCurrentClient(client);
680+
681+
client.addIntegration(
682+
reactRouterV6BrowserTracingIntegration({
683+
useEffect: React.useEffect,
684+
useLocation,
685+
useNavigationType,
686+
createRoutesFromChildren,
687+
matchRoutes,
688+
}),
689+
);
690+
691+
const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter);
692+
693+
const router = createSentryMemoryRouter(
694+
[
695+
{
696+
children: [
697+
{ path: '/a', element: <div>A</div> },
698+
{ path: '/b', element: <div>B</div> },
699+
{ path: '/c', element: <div>C</div> },
700+
],
701+
},
702+
],
703+
{ initialEntries: ['/a'] },
704+
);
705+
706+
render(
707+
<React.StrictMode>
708+
<RouterProvider router={router} />
709+
</React.StrictMode>,
710+
);
711+
712+
await act(async () => {
713+
router.navigate('/b');
714+
router.navigate('/c');
715+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2));
716+
});
717+
718+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(2);
719+
720+
const calls = mockStartBrowserTracingNavigationSpan.mock.calls;
721+
expect(calls[0]![1].name).toBe('/b');
722+
expect(calls[1]![1].name).toBe('/c');
723+
});
724+
725+
it('should NOT create duplicate spans for same route name (even with different params)', async () => {
726+
const client = createMockBrowserClient();
727+
setCurrentClient(client);
728+
729+
client.addIntegration(
730+
reactRouterV6BrowserTracingIntegration({
731+
useEffect: React.useEffect,
732+
useLocation,
733+
useNavigationType,
734+
createRoutesFromChildren,
735+
matchRoutes,
736+
}),
737+
);
738+
739+
const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter);
740+
741+
const router = createSentryMemoryRouter(
742+
[
743+
{
744+
children: [{ path: '/user/:id', element: <div>User</div> }],
745+
},
746+
],
747+
{ initialEntries: ['/user/1'] },
748+
);
749+
750+
render(
751+
<React.StrictMode>
752+
<RouterProvider router={router} />
753+
</React.StrictMode>,
754+
);
755+
756+
await act(async () => {
757+
router.navigate('/user/2');
758+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1));
759+
});
760+
761+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
762+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledWith(expect.any(BrowserClient), {
763+
name: '/user/:id',
764+
attributes: {
765+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
766+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
767+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
768+
},
769+
});
770+
771+
await act(async () => {
772+
router.navigate('/user/3');
773+
await new Promise(resolve => setTimeout(resolve, 100));
774+
});
775+
776+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
777+
});
778+
779+
it('should handle mixed cross-usage and consecutive navigations correctly', async () => {
780+
const client = createMockBrowserClient();
781+
setCurrentClient(client);
782+
783+
client.addIntegration(
784+
reactRouterV6BrowserTracingIntegration({
785+
useEffect: React.useEffect,
786+
useLocation,
787+
useNavigationType,
788+
createRoutesFromChildren,
789+
matchRoutes,
790+
}),
791+
);
792+
793+
const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter);
794+
const sentryUseRoutes = wrapUseRoutesV6(useRoutes);
795+
796+
const UsersRoute: React.FC = () => sentryUseRoutes([{ path: '/', element: <div>Users</div> }]);
797+
798+
const SettingsRoute: React.FC = () => sentryUseRoutes([{ path: '/', element: <div>Settings</div> }]);
799+
800+
const router = createSentryMemoryRouter(
801+
[
802+
{
803+
children: [
804+
{ path: '/users/*', element: <UsersRoute /> },
805+
{ path: '/settings/*', element: <SettingsRoute /> },
806+
],
807+
},
808+
],
809+
{ initialEntries: ['/users'] },
810+
);
811+
812+
render(
813+
<React.StrictMode>
814+
<RouterProvider router={router} />
815+
</React.StrictMode>,
816+
);
817+
818+
expect(mockStartBrowserTracingNavigationSpan).not.toHaveBeenCalled();
819+
820+
await act(async () => {
821+
router.navigate('/settings');
822+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1));
823+
});
824+
825+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
826+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
827+
name: '/settings/*',
828+
attributes: expect.objectContaining({
829+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
830+
}),
831+
});
832+
});
833+
834+
it('should not create duplicate spans for cross-usage on same route', async () => {
835+
const client = createMockBrowserClient();
836+
setCurrentClient(client);
837+
838+
client.addIntegration(
839+
reactRouterV6BrowserTracingIntegration({
840+
useEffect: React.useEffect,
841+
useLocation,
842+
useNavigationType,
843+
createRoutesFromChildren,
844+
matchRoutes,
845+
}),
846+
);
847+
848+
const createSentryMemoryRouter = wrapCreateMemoryRouterV6(createMemoryRouter);
849+
const sentryUseRoutes = wrapUseRoutesV6(useRoutes);
850+
851+
const NestedRoute: React.FC = () => sentryUseRoutes([{ path: '/', element: <div>Details</div> }]);
852+
853+
const router = createSentryMemoryRouter(
854+
[
855+
{
856+
children: [{ path: '/details/*', element: <NestedRoute /> }],
857+
},
858+
],
859+
{ initialEntries: ['/home'] },
860+
);
861+
862+
render(
863+
<React.StrictMode>
864+
<RouterProvider router={router} />
865+
</React.StrictMode>,
866+
);
867+
868+
await act(async () => {
869+
router.navigate('/details');
870+
await waitFor(() => expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalled());
871+
});
872+
873+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
874+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledWith(expect.any(BrowserClient), {
875+
name: '/details/*',
876+
attributes: expect.objectContaining({
877+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
878+
}),
879+
});
880+
});
881+
});
610882
});

0 commit comments

Comments
 (0)