From 17f7bcd2e858b912aa704dd5dc0e40e4e16e71bc Mon Sep 17 00:00:00 2001 From: Matt Quinn Date: Mon, 23 Dec 2024 14:35:31 -0500 Subject: [PATCH] fix(react): improve handling of routes nested under path="/" (#14821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 root}> issues group}> index} /> ``` 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. --- .../react/src/reactrouterv6-compat-utils.tsx | 4 +- packages/react/test/reactrouterv6.test.tsx | 156 ++++++++++++++++++ 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 08f20354f870..9cb8d3cd8fe5 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -427,10 +427,10 @@ function getNormalizedName( // If path is not a wildcard and has no child routes, append the path if (path && !pathIsWildcardAndHasChildren(path, branch)) { const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; - pathBuilder += newPath; + pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); // If the path matches the current location, return the path - if (location.pathname.endsWith(basename + branch.pathname)) { + if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { if ( // If the route defined on the element is something like // Product} /> diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 815b562f08f7..33e330c2e232 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -594,6 +594,84 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }); }); + it('works under a slash route with a trailing slash', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + render( + + + } /> + root}> + issues group}> + index} /> + + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/issues/:groupId/', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + + it('works nested under a slash root without a trailing slash', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + render( + + + } /> + root}> + issues group}> + index} /> + + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/issues/:groupId/', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + it("updates the scope's `transactionName` on a navigation", () => { const client = createMockBrowserClient(); setCurrentClient(client); @@ -1397,6 +1475,84 @@ describe('reactRouterV6BrowserTracingIntegration', () => { expect(mockRootSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); + it('works under a slash route with a trailing slash', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + render( + + + } /> + root}> + issues group}> + index} /> + + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/issues/:groupId/', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + + it('works nested under a slash root without a trailing slash', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + render( + + + } /> + root}> + issues group}> + index} /> + + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/issues/:groupId/', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + it("updates the scope's `transactionName` on a navigation", () => { const client = createMockBrowserClient(); setCurrentClient(client);