From 8d14c3f1bc7e41b35d86be42ffd364cf13ce5e81 Mon Sep 17 00:00:00 2001 From: Matt Quinn Date: Fri, 20 Dec 2024 16:05:35 -0500 Subject: [PATCH 1/3] 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 root}> issues group}> index} /> ``` 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 3473447a 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. --- .../react/src/reactrouterv6-compat-utils.tsx | 6 +- packages/react/test/reactrouterv6.test.tsx | 78 +++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 08f20354f870..db92e70418c2 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -426,11 +426,11 @@ 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; + const newPath = prefixWithSlash(path); + pathBuilder = trimSlash(pathBuilder) + 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..6f4d80f7ebd0 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); From 055b614c620087f7e4f2827823e74554174a0f6d Mon Sep 17 00:00:00 2001 From: Matt Quinn Date: Fri, 20 Dec 2024 16:40:39 -0500 Subject: [PATCH 2/3] less intrusive fix for pathBuilder double slash --- packages/react/src/reactrouterv6-compat-utils.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index db92e70418c2..9cb8d3cd8fe5 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -426,8 +426,8 @@ function getNormalizedName( // If path is not a wildcard and has no child routes, append the path if (path && !pathIsWildcardAndHasChildren(path, branch)) { - const newPath = prefixWithSlash(path); - pathBuilder = trimSlash(pathBuilder) + newPath; + const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; + pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); // If the path matches the current location, return the path if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { From 7c064d0ca295c29904294be85c4f97188e1dc396 Mon Sep 17 00:00:00 2001 From: Matt Quinn Date: Mon, 23 Dec 2024 09:30:36 -0500 Subject: [PATCH 3/3] duplicate test under describe wrapUseRoutesV6 --- packages/react/test/reactrouterv6.test.tsx | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 6f4d80f7ebd0..33e330c2e232 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -1475,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);