Skip to content

Commit daef6ce

Browse files
mjqonurtemizkan
mjq
authored and
onurtemizkan
committed
fix(react): improve handling of routes nested under path="/" (#14821)
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123` (no trailing slash), the recorded transaction name is `//issues/:groupId/` (notice the double slash). This looks messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (with trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route with `path="/"`. (Thanks @JoshFerge for noticing the bad data in Sentry! 🙌) --- Note that this commit effectively reverts a change from #14304 where ```js if (basename + branch.pathname === location.pathname) { ``` became ```js if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that caused the difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind of `Route` nesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal of `endsWith`. @onurtemizkan, who wrote the change in question: > I'd expect this to break descendant routes. But in the final > implementation, descendant routes don't end up here. So, I > believe it's safe to revert.
1 parent dbd3296 commit daef6ce

File tree

2 files changed

+158
-2
lines changed

2 files changed

+158
-2
lines changed

‎packages/react/src/reactrouterv6-compat-utils.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -428,10 +428,10 @@ function getNormalizedName(
428428
// If path is not a wildcard and has no child routes, append the path
429429
if (path && !pathIsWildcardAndHasChildren(path, branch)) {
430430
const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`;
431-
pathBuilder += newPath;
431+
pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath);
432432

433433
// If the path matches the current location, return the path
434-
if (location.pathname.endsWith(basename + branch.pathname)) {
434+
if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) {
435435
if (
436436
// If the route defined on the element is something like
437437
// <Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} />

‎packages/react/test/reactrouterv6.test.tsx‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,84 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
594594
});
595595
});
596596

597+
it('works under a slash route with a trailing slash', () => {
598+
const client = createMockBrowserClient();
599+
setCurrentClient(client);
600+
601+
client.addIntegration(
602+
reactRouterV6BrowserTracingIntegration({
603+
useEffect: React.useEffect,
604+
useLocation,
605+
useNavigationType,
606+
createRoutesFromChildren,
607+
matchRoutes,
608+
}),
609+
);
610+
const SentryRoutes = withSentryReactRouterV6Routing(Routes);
611+
612+
render(
613+
<MemoryRouter initialEntries={['/']}>
614+
<SentryRoutes>
615+
<Route index element={<Navigate to="/issues/123/" />} />
616+
<Route path="/" element={<div>root</div>}>
617+
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
618+
<Route index element={<div>index</div>} />
619+
</Route>
620+
</Route>
621+
</SentryRoutes>
622+
</MemoryRouter>,
623+
);
624+
625+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
626+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
627+
name: '/issues/:groupId/',
628+
attributes: {
629+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
630+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
631+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
632+
},
633+
});
634+
});
635+
636+
it('works nested under a slash root without a trailing slash', () => {
637+
const client = createMockBrowserClient();
638+
setCurrentClient(client);
639+
640+
client.addIntegration(
641+
reactRouterV6BrowserTracingIntegration({
642+
useEffect: React.useEffect,
643+
useLocation,
644+
useNavigationType,
645+
createRoutesFromChildren,
646+
matchRoutes,
647+
}),
648+
);
649+
const SentryRoutes = withSentryReactRouterV6Routing(Routes);
650+
651+
render(
652+
<MemoryRouter initialEntries={['/']}>
653+
<SentryRoutes>
654+
<Route index element={<Navigate to="/issues/123" />} />
655+
<Route path="/" element={<div>root</div>}>
656+
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
657+
<Route index element={<div>index</div>} />
658+
</Route>
659+
</Route>
660+
</SentryRoutes>
661+
</MemoryRouter>,
662+
);
663+
664+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
665+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
666+
name: '/issues/:groupId/',
667+
attributes: {
668+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
669+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
670+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
671+
},
672+
});
673+
});
674+
597675
it("updates the scope's `transactionName` on a navigation", () => {
598676
const client = createMockBrowserClient();
599677
setCurrentClient(client);
@@ -1397,6 +1475,84 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
13971475
expect(mockRootSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
13981476
});
13991477

1478+
it('works under a slash route with a trailing slash', () => {
1479+
const client = createMockBrowserClient();
1480+
setCurrentClient(client);
1481+
1482+
client.addIntegration(
1483+
reactRouterV6BrowserTracingIntegration({
1484+
useEffect: React.useEffect,
1485+
useLocation,
1486+
useNavigationType,
1487+
createRoutesFromChildren,
1488+
matchRoutes,
1489+
}),
1490+
);
1491+
const SentryRoutes = withSentryReactRouterV6Routing(Routes);
1492+
1493+
render(
1494+
<MemoryRouter initialEntries={['/']}>
1495+
<SentryRoutes>
1496+
<Route index element={<Navigate to="/issues/123/" />} />
1497+
<Route path="/" element={<div>root</div>}>
1498+
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
1499+
<Route index element={<div>index</div>} />
1500+
</Route>
1501+
</Route>
1502+
</SentryRoutes>
1503+
</MemoryRouter>,
1504+
);
1505+
1506+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
1507+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
1508+
name: '/issues/:groupId/',
1509+
attributes: {
1510+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
1511+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
1512+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
1513+
},
1514+
});
1515+
});
1516+
1517+
it('works nested under a slash root without a trailing slash', () => {
1518+
const client = createMockBrowserClient();
1519+
setCurrentClient(client);
1520+
1521+
client.addIntegration(
1522+
reactRouterV6BrowserTracingIntegration({
1523+
useEffect: React.useEffect,
1524+
useLocation,
1525+
useNavigationType,
1526+
createRoutesFromChildren,
1527+
matchRoutes,
1528+
}),
1529+
);
1530+
const SentryRoutes = withSentryReactRouterV6Routing(Routes);
1531+
1532+
render(
1533+
<MemoryRouter initialEntries={['/']}>
1534+
<SentryRoutes>
1535+
<Route index element={<Navigate to="/issues/123" />} />
1536+
<Route path="/" element={<div>root</div>}>
1537+
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
1538+
<Route index element={<div>index</div>} />
1539+
</Route>
1540+
</Route>
1541+
</SentryRoutes>
1542+
</MemoryRouter>,
1543+
);
1544+
1545+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
1546+
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
1547+
name: '/issues/:groupId/',
1548+
attributes: {
1549+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
1550+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
1551+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6',
1552+
},
1553+
});
1554+
});
1555+
14001556
it("updates the scope's `transactionName` on a navigation", () => {
14011557
const client = createMockBrowserClient();
14021558
setCurrentClient(client);

0 commit comments

Comments
 (0)