Skip to content

Fix(integrations): Fix support for wildcard paths in react-router-dom v6 integration (#5997) #14076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix(integrations): Fix support for wildcard paths in react-router-dom v6 integration (#5997) #14076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9afda8
fix(integrations): Fix support for wildcard paths in react-router-dom…
grahamhency Oct 22, 2024
f491698
Merge branch 'getsentry:develop' into react_router_6_wildcard_handling
grahamhency Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions packages/react/src/reactrouterv6.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
spanToJSON,
} from '@sentry/core';
import type { Client, Integration, Span, TransactionSource } from '@sentry/types';
import { getNumberOfUrlSegments, logger } from '@sentry/utils';
import { logger } from '@sentry/utils';
import hoistNonReactStatics from 'hoist-non-react-statics';
import * as React from 'react';

Expand Down Expand Up @@ -141,6 +141,14 @@ function stripBasenameFromPathname(pathname: string, basename: string): string {
return pathname.slice(startIndex) || '/';
}

function sendIndexPath(pathBuilder: string, pathname: string, basename: string): void | string[] {
const p1 = pathBuilder || stripBasenameFromPathname(pathname, basename);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little hard to track. Could you please add some comments here?

const p2 = pathBuilder || pathname;
const p3 = _stripBasename ? p1 : p2;
const formattedPath = p3[p3.length - 1] === '/' ? p3.slice(0, -1) : p3[p3.length - 1] === '/*' ? p3.slice(0, -1) : p3;
if (formattedPath) return [formattedPath, 'route'];
}

function getNormalizedName(
routes: RouteObject[],
location: Location,
Expand All @@ -157,27 +165,22 @@ function getNormalizedName(
const route = branch.route;
if (route) {
// Early return if index route
if (route.index) {
return [_stripBasename ? stripBasenameFromPathname(branch.pathname, basename) : branch.pathname, 'route'];
}
if (route.index) sendIndexPath(pathBuilder, branch.pathname, basename);

const path = route.path;
if (path) {
const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`;
pathBuilder += newPath;

if (basename + branch.pathname === location.pathname) {
if (
// If the route defined on the element is something like
// <Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} />
// We should check against the branch.pathname for the number of / seperators
getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) &&
// We should not count wildcard operators in the url segments calculation
pathBuilder.slice(-2) !== '/*'
) {
return [(_stripBasename ? '' : basename) + newPath, 'route'];
// If path is not a wildcard and has no child routes, append the path
if (!(path === '*' && branch.route.children && branch.route.children.length > 0)) {
const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`;
pathBuilder += newPath;
if (basename + branch.pathname === location.pathname) {
// if the last character of the pathbuilder is a wilcard and there are children, remove the wildcard
if (pathBuilder[pathBuilder.length - 1] === '*' && branch.route.children && branch.route.children.length > 0) {
pathBuilder = pathBuilder.slice(0, -1);
} else {
return [(_stripBasename ? '' : basename) + pathBuilder, 'route'];
}
}
return [(_stripBasename ? '' : basename) + pathBuilder, 'route'];
}
}
}
Expand Down
67 changes: 58 additions & 9 deletions packages/react/test/reactrouterv6.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="/about" element={<div>About</div>}>
<Route path="/about/us" element={<div>us</div>} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to cover this usage in tests to be sure we're not breaking anything. This is still a valid use. Could you please instead add a new test case as you update?

<Route path="us" element={<div>us</div>} />
</Route>
<Route path="/" element={<Navigate to="/about/us" />} />
</SentryRoutes>
Expand Down Expand Up @@ -287,7 +287,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="/about" element={<div>About</div>}>
<Route path="/about/:page" element={<div>page</div>} />
<Route path=":page" element={<div>page</div>} />
</Route>
<Route path="/" element={<Navigate to="/about/us" />} />
</SentryRoutes>
Expand Down Expand Up @@ -324,8 +324,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="/stores" element={<div>Stores</div>}>
<Route path="/stores/:storeId" element={<div>Store</div>}>
<Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} />
<Route path=":storeId" element={<div>Store</div>}>
<Route path="products/:productId" element={<div>Product</div>} />
</Route>
</Route>
<Route path="/" element={<Navigate to="/stores/foo/products/234" />} />
Expand Down Expand Up @@ -391,6 +391,55 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
});
});

it('works with wildcard routes', () => {
const client = createMockBrowserClient();
setCurrentClient(client);

client.addIntegration(
reactRouterV6BrowserTracingIntegration({
useEffect: React.useEffect,
useLocation,
useNavigationType,
createRoutesFromChildren,
matchRoutes,
}),
);
const SentryRoutes = withSentryReactRouterV6Routing(Routes);

render(
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="*" element={<Outlet />}>
<Route index element={<Navigate to="/projects/123/views/234" />} />
<Route path="account" element={<div>Account Page</div>} />
<Route path="projects">
<Route path="*" element={<Outlet />}>
<Route path=":projectId" element={<div>Project Page</div>}>
<Route index element={<div>Project Page Root</div>} />
<Route element={<div>Editor</div>}>
<Route path="views/:viewId" element={<div>View Canvas</div>} />
<Route path="spaces/:spaceId" element={<div>Space Canvas</div>} />
</Route>
</Route>
</Route>
</Route>
<Route path="*" element={<div>No Match Page</div>} />
</Route>
</SentryRoutes>
</MemoryRouter>,
);

expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1);
expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), {
name: '/projects/:projectId/views/:viewId',
Copy link
Collaborator

@onurtemizkan onurtemizkan Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test case where the wildcard exists in the transaction name (like: /param-page/*/details)? Something similar to the example app you provided.

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);
Expand All @@ -410,7 +459,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route path="/about" element={<div>About</div>}>
<Route path="/about/:page" element={<div>page</div>} />
<Route path=":page" element={<div>page</div>} />
</Route>
<Route path="/" element={<Navigate to="/about/us" />} />
</SentryRoutes>
Expand Down Expand Up @@ -639,7 +688,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
element: <div>About</div>,
children: [
{
path: '/about/us',
path: 'us',
element: <div>us</div>,
},
],
Expand Down Expand Up @@ -689,7 +738,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
element: <div>About</div>,
children: [
{
path: '/about/:page',
path: ':page',
element: <div>page</div>,
},
],
Expand Down Expand Up @@ -739,11 +788,11 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
element: <div>Stores</div>,
children: [
{
path: '/stores/:storeId',
path: ':storeId',
element: <div>Store</div>,
children: [
{
path: '/stores/:storeId/products/:productId',
path: 'products/:productId',
element: <div>Product</div>,
},
],
Expand Down
Loading