Skip to content

Commit 8d14c3f

Browse files
committed
fix(react): improve handling of routes nested under path="/"
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevent 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`, the recorded transaction name is `//issues/:groupId/` (notice the double slash). This is messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (note the 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 other routes with this structure (nested under a route with path `/`). This commit fixes both of these issues and passes both the new tests (which reproduce the above issues) and all existing tests, but I still don't trust the change. First, `newPath` now always has a leading slash, and I don't know how that interacts with the other path concatenations inside `getNormalizedName`. It feels like dealing with path concatenation should be handled in a more systematic way. Second, I revert a change from 3473447 where ```js if (basename + branch.pathname === location.pathname) { ``` became ``` if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that 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. This seems likely to be a wide regression in transaction names. But, no tests failed from this change, which makes me worried that there's some necessary behaviour that isn't covered.
1 parent 65531f3 commit 8d14c3f

File tree

2 files changed

+81
-3
lines changed

2 files changed

+81
-3
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -426,11 +426,11 @@ function getNormalizedName(
426426

427427
// If path is not a wildcard and has no child routes, append the path
428428
if (path && !pathIsWildcardAndHasChildren(path, branch)) {
429-
const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`;
430-
pathBuilder += newPath;
429+
const newPath = prefixWithSlash(path);
430+
pathBuilder = trimSlash(pathBuilder) + newPath;
431431

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

packages/react/test/reactrouterv6.test.tsx

Lines changed: 78 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);

0 commit comments

Comments
 (0)