Skip to content

Commit 7968cd8

Browse files
authored
fix(react): Don't trim index route / when getting pathname (#17985)
Currently, the test in this PR fail: #17789 Root routes can yield an empty transaction name, causing `<unlabeled transaction>` instead of `/` as the transaction name for the root. This happens when the router includes children routes with `index: true`. The route matching is also depending on the `allRoutes` Set. The `allRoutes` Set include the children routes twice (once as children of the route and once as a root element of the Set). When only including them once, it works but parametrization does not work anymore. --> First I thought, this duplication is the cause but probably it isn't ## What’s broken Root cause is in `sendIndexPath(...)`: - Mis-parenthesized ternary picks `stripBasenameFromPathname` instead of `pathBuilder`. - Trimming turns `/` into an empty string.
1 parent 027ab90 commit 7968cd8

File tree

5 files changed

+222
-14
lines changed

5 files changed

+222
-14
lines changed

dev-packages/e2e-tests/test-applications/react-router-7-cross-usage/src/index.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@ const ProjectsRoutes = () => (
8989
);
9090

9191
const router = sentryCreateBrowserRouter([
92+
{
93+
path: '/post/:post',
94+
element: <div>Post</div>,
95+
children: [
96+
{ index: true, element: <div>Post Index</div> },
97+
{ path: '/post/:post/related', element: <div>Related Posts</div> },
98+
],
99+
},
92100
{
93101
children: [
94102
{

packages/react/src/reactrouter-compat-utils/instrumentation.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,8 @@ export interface ReactRouterOptions {
106106
type V6CompatibleVersion = '6' | '7';
107107

108108
// Keeping as a global variable for cross-usage in multiple functions
109-
const allRoutes = new Set<RouteObject>();
109+
// only exported for testing purposes
110+
export const allRoutes = new Set<RouteObject>();
110111

111112
/**
112113
* Processes resolved routes by adding them to allRoutes and checking for nested async handlers.
@@ -679,7 +680,8 @@ export function handleNavigation(opts: {
679680
}
680681
}
681682

682-
function addRoutesToAllRoutes(routes: RouteObject[]): void {
683+
/* Only exported for testing purposes */
684+
export function addRoutesToAllRoutes(routes: RouteObject[]): void {
683685
routes.forEach(route => {
684686
const extractedChildRoutes = getChildRoutesRecursively(route);
685687

packages/react/src/reactrouter-compat-utils/utils.ts

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,21 +45,27 @@ export function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch<st
4545
return (pathEndsWithWildcard(path) && !!branch.route.children?.length) || false;
4646
}
4747

48-
function routeIsDescendant(route: RouteObject): boolean {
48+
/** Check if route is in descendant route (<Routes> within <Routes>) */
49+
export function routeIsDescendant(route: RouteObject): boolean {
4950
return !!(!route.children && route.element && route.path?.endsWith('/*'));
5051
}
5152

5253
function sendIndexPath(pathBuilder: string, pathname: string, basename: string): [string, TransactionSource] {
53-
const reconstructedPath = pathBuilder || _stripBasename ? stripBasenameFromPathname(pathname, basename) : pathname;
54-
55-
const formattedPath =
56-
// If the path ends with a slash, remove it
57-
reconstructedPath[reconstructedPath.length - 1] === '/'
58-
? reconstructedPath.slice(0, -1)
59-
: // If the path ends with a wildcard, remove it
60-
reconstructedPath.slice(-2) === '/*'
61-
? reconstructedPath.slice(0, -1)
62-
: reconstructedPath;
54+
const reconstructedPath =
55+
pathBuilder && pathBuilder.length > 0
56+
? pathBuilder
57+
: _stripBasename
58+
? stripBasenameFromPathname(pathname, basename)
59+
: pathname;
60+
61+
let formattedPath =
62+
// If the path ends with a wildcard suffix, remove both the slash and the asterisk
63+
reconstructedPath.slice(-2) === '/*' ? reconstructedPath.slice(0, -2) : reconstructedPath;
64+
65+
// If the path ends with a slash, remove it (but keep single '/')
66+
if (formattedPath.length > 1 && formattedPath[formattedPath.length - 1] === '/') {
67+
formattedPath = formattedPath.slice(0, -1);
68+
}
6369

6470
return [formattedPath, 'route'];
6571
}

packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
createReactRouterV6CompatibleTracingIntegration,
1111
updateNavigationSpan,
1212
} from '../../src/reactrouter-compat-utils';
13+
import { addRoutesToAllRoutes, allRoutes } from '../../src/reactrouter-compat-utils/instrumentation';
1314
import type { Location, RouteObject } from '../../src/types';
1415

1516
const mockUpdateName = vi.fn();
@@ -47,6 +48,7 @@ vi.mock('../../src/reactrouter-compat-utils/utils', () => ({
4748
initializeRouterUtils: vi.fn(),
4849
getGlobalLocation: vi.fn(() => ({ pathname: '/test', search: '', hash: '' })),
4950
getGlobalPathname: vi.fn(() => '/test'),
51+
routeIsDescendant: vi.fn(() => false),
5052
}));
5153

5254
vi.mock('../../src/reactrouter-compat-utils/lazy-routes', () => ({
@@ -141,3 +143,193 @@ describe('reactrouter-compat-utils/instrumentation', () => {
141143
});
142144
});
143145
});
146+
147+
describe('addRoutesToAllRoutes', () => {
148+
beforeEach(() => {
149+
vi.clearAllMocks();
150+
vi.resetModules();
151+
allRoutes.clear();
152+
});
153+
154+
it('should add simple routes without nesting', () => {
155+
const routes = [
156+
{ path: '/', element: <div /> },
157+
{ path: '/user/:id', element: <div /> },
158+
{ path: '/group/:group/:user?', element: <div /> },
159+
];
160+
161+
addRoutesToAllRoutes(routes);
162+
const allRoutesArr = Array.from(allRoutes);
163+
164+
expect(allRoutesArr).toHaveLength(3);
165+
expect(allRoutesArr).toEqual(
166+
expect.arrayContaining([
167+
expect.objectContaining({ path: '/' }),
168+
expect.objectContaining({ path: '/user/:id' }),
169+
expect.objectContaining({ path: '/group/:group/:user?' }),
170+
]),
171+
);
172+
173+
// Verify exact structure matches manual testing results
174+
allRoutesArr.forEach(route => {
175+
expect(route).toHaveProperty('element');
176+
expect(route.element).toHaveProperty('props');
177+
});
178+
});
179+
180+
it('should handle complex nested routes with multiple levels', () => {
181+
const routes = [
182+
{ path: '/', element: <div /> },
183+
{ path: '/user/:id', element: <div /> },
184+
{ path: '/group/:group/:user?', element: <div /> },
185+
{
186+
path: '/v1/post/:post',
187+
element: <div />,
188+
children: [
189+
{ path: 'featured', element: <div /> },
190+
{ path: '/v1/post/:post/related', element: <div /> },
191+
{
192+
element: <div>More Nested Children</div>,
193+
children: [{ path: 'edit', element: <div>Edit Post</div> }],
194+
},
195+
],
196+
},
197+
{
198+
path: '/v2/post/:post',
199+
element: <div />,
200+
children: [
201+
{ index: true, element: <div /> },
202+
{ path: 'featured', element: <div /> },
203+
{ path: '/v2/post/:post/related', element: <div /> },
204+
],
205+
},
206+
];
207+
208+
addRoutesToAllRoutes(routes);
209+
const allRoutesArr = Array.from(allRoutes);
210+
211+
expect(allRoutesArr).toEqual([
212+
{ path: '/', element: <div /> },
213+
{ path: '/user/:id', element: <div /> },
214+
{ path: '/group/:group/:user?', element: <div /> },
215+
// v1 routes ----
216+
{
217+
path: '/v1/post/:post',
218+
element: <div />,
219+
children: [
220+
{ element: <div />, path: 'featured' },
221+
{ element: <div />, path: '/v1/post/:post/related' },
222+
{ children: [{ element: <div>Edit Post</div>, path: 'edit' }], element: <div>More Nested Children</div> },
223+
],
224+
},
225+
{ element: <div />, path: 'featured' },
226+
{ element: <div />, path: '/v1/post/:post/related' },
227+
{ children: [{ element: <div>Edit Post</div>, path: 'edit' }], element: <div>More Nested Children</div> },
228+
{ element: <div>Edit Post</div>, path: 'edit' },
229+
// v2 routes ---
230+
{
231+
path: '/v2/post/:post',
232+
element: expect.objectContaining({ type: 'div', props: {} }),
233+
children: [
234+
{ element: <div />, index: true },
235+
{ element: <div />, path: 'featured' },
236+
{ element: <div />, path: '/v2/post/:post/related' },
237+
],
238+
},
239+
{ element: <div />, index: true },
240+
{ element: <div />, path: 'featured' },
241+
{ element: <div />, path: '/v2/post/:post/related' },
242+
]);
243+
});
244+
245+
it('should handle routes with nested index routes', () => {
246+
const routes = [
247+
{
248+
path: '/dashboard',
249+
element: <div />,
250+
children: [
251+
{ index: true, element: <div>Dashboard Index</div> },
252+
{ path: 'settings', element: <div>Settings</div> },
253+
],
254+
},
255+
];
256+
257+
addRoutesToAllRoutes(routes);
258+
const allRoutesArr = Array.from(allRoutes);
259+
260+
expect(allRoutesArr).toEqual([
261+
{
262+
path: '/dashboard',
263+
element: expect.objectContaining({ type: 'div' }),
264+
children: [
265+
{ element: <div>Dashboard Index</div>, index: true },
266+
{ element: <div>Settings</div>, path: 'settings' },
267+
],
268+
},
269+
{ element: <div>Dashboard Index</div>, index: true },
270+
{ element: <div>Settings</div>, path: 'settings' },
271+
]);
272+
});
273+
274+
it('should handle deeply nested routes with layout wrappers', () => {
275+
const routes = [
276+
{
277+
path: '/',
278+
element: <div>Root</div>,
279+
children: [
280+
{ path: 'dashboard', element: <div>Dashboard</div> },
281+
{
282+
element: <div>AuthLayout</div>,
283+
children: [{ path: 'login', element: <div>Login</div> }],
284+
},
285+
],
286+
},
287+
];
288+
289+
addRoutesToAllRoutes(routes);
290+
const allRoutesArr = Array.from(allRoutes);
291+
292+
expect(allRoutesArr).toEqual([
293+
{
294+
path: '/',
295+
element: expect.objectContaining({ type: 'div', props: { children: 'Root' } }),
296+
children: [
297+
{
298+
path: 'dashboard',
299+
element: expect.objectContaining({ type: 'div', props: { children: 'Dashboard' } }),
300+
},
301+
{
302+
element: expect.objectContaining({ type: 'div', props: { children: 'AuthLayout' } }),
303+
children: [
304+
{
305+
path: 'login',
306+
element: expect.objectContaining({ type: 'div', props: { children: 'Login' } }),
307+
},
308+
],
309+
},
310+
],
311+
},
312+
{ element: <div>Dashboard</div>, path: 'dashboard' },
313+
{
314+
children: [{ element: <div>Login</div>, path: 'login' }],
315+
element: <div>AuthLayout</div>,
316+
},
317+
{ element: <div>Login</div>, path: 'login' },
318+
]);
319+
});
320+
321+
it('should not duplicate routes when called multiple times', () => {
322+
const routes = [
323+
{ path: '/', element: <div /> },
324+
{ path: '/about', element: <div /> },
325+
];
326+
327+
addRoutesToAllRoutes(routes);
328+
const firstCount = allRoutes.size;
329+
330+
addRoutesToAllRoutes(routes);
331+
const secondCount = allRoutes.size;
332+
333+
expect(firstCount).toBe(secondCount);
334+
});
335+
});

packages/react/test/reactrouter-compat-utils/utils.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ describe('reactrouter-compat-utils/utils', () => {
436436
];
437437

438438
const result = getNormalizedName(routes, location, branches, '');
439-
expect(result).toEqual(['', 'route']);
439+
expect(result).toEqual(['/', 'route']);
440440
});
441441

442442
it('should handle simple route path', () => {

0 commit comments

Comments
 (0)