Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,31 @@ const DeepTeamRoutes = () => (
</SentryRoutes>
);

// Two independent descendant <SentryRoutes> trees that each contribute a single-segment param leaf
// (`:fooId` / `:barId`). Because `allRoutes` is a shared module-level set, once both have mounted a
// navigation into one can pick up the param name from the other, yielding a hybrid name like
// `/bar/:fooId` instead of `/bar/:barId` (see issue #22782).
const FooRoutes = () => (
<SentryRoutes>
<Route path=":fooId" element={<div id="foo">Foo</div>} />
</SentryRoutes>
);

const BarRoutes = () => (
<SentryRoutes>
<Route path=":barId" element={<div id="bar">Bar</div>} />
</SentryRoutes>
);

const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement);
root.render(
<BrowserRouter>
<SentryRoutes>
<Route path="/" element={<Index />} />
<Route path="child/*" element={<ChildRoutes />} />
<Route path="workspace/*" element={<DeepTeamRoutes />} />
<Route path="foo/*" element={<FooRoutes />} />
<Route path="bar/*" element={<BarRoutes />} />
<Route path="/*" element={<ProjectsRoutes />} />
</SentryRoutes>
</BrowserRouter>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ const Index = () => {
<Link to="/workspace/team/u123" id="deep-member-navigation">
navigate deep member
</Link>
<Link to="/foo/123" id="foo-navigation">
navigate foo
</Link>
<Link to="/bar/456" id="bar-navigation">
navigate bar
</Link>
</>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,66 @@ test('resolves deep wildcard chain with three levels of nesting - pageload', asy
});
});

test('does not mix param names across independent descendant routers', async ({ page }) => {
const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

const fooNavigationTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return (
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.contexts?.trace?.data?.['url.path'] === '/foo/123'
);
});

const barNavigationTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return (
transactionEvent.contexts?.trace?.op === 'navigation' &&
transactionEvent.contexts?.trace?.data?.['url.path'] === '/bar/456'
);
});

await page.goto(`/`);
await pageloadTxnPromise;

// Mount the first descendant router (`foo/*` -> `:fooId`), which populates the shared `allRoutes` set.
const [, fooNavigationTxn] = await Promise.all([page.locator('id=foo-navigation').click(), fooNavigationTxnPromise]);

expect((await page.innerHTML('#root')).includes('Foo')).toBe(true);
expect(fooNavigationTxn).toMatchObject({
transaction: '/foo/:fooId',
transaction_info: { source: 'route' },
});

// Return to the index so we can navigate into the second, unrelated descendant router client-side.
// A fresh page load would reset the module-level `allRoutes` and hide the bug.
await page.goBack();
await page.locator('id=bar-navigation').waitFor();

// Now mount the second descendant router (`bar/*` -> `:barId`). With the accumulation bug, the name
// comes out as the hybrid `/bar/:fooId`.
const [, barNavigationTxn] = await Promise.all([page.locator('id=bar-navigation').click(), barNavigationTxnPromise]);

expect((await page.innerHTML('#root')).includes('Bar')).toBe(true);
expect(barNavigationTxn).toMatchObject({
contexts: {
trace: {
op: 'navigation',
origin: 'auto.navigation.react.reactrouter_v6',
data: {
'sentry.source': 'route',
'url.template': '/bar/:barId',
'url.path': '/bar/456',
},
},
},
transaction: '/bar/:barId',
transaction_info: {
source: 'route',
},
});
});

test('resolves deep wildcard chain with three levels of nesting - navigation', async ({ page }) => {
const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
Expand Down
43 changes: 36 additions & 7 deletions packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -766,13 +766,20 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio
const stableLocationParam =
typeof locationArg === 'string' || locationArg?.pathname ? (locationArg as { pathname: string }) : location;

// Register this `<Routes>`'s routes in the shared set for as long as it is mounted, removing them on
// unmount so they don't leak into later unrelated navigations (#22782). Tying add and remove to the
// same effect lifecycle keeps it correct under StrictMode's mount/unmount/remount.
useIsomorphicLayoutEffect(() => {
const added = addRoutesToAllRoutes(routes);

return () => removeRoutesFromAllRoutes(added);
});

useIsomorphicLayoutEffect(() => {
const normalizedLocation =
typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam;

if (isMountRenderPass.current) {
addRoutesToAllRoutes(routes);

updatePageloadTransaction({
activeRootSpan: getActiveRootSpan(),
location: normalizedLocation,
Expand Down Expand Up @@ -1049,14 +1056,29 @@ export function handleNavigation(opts: {
}

/* Only exported for testing purposes */
export function addRoutesToAllRoutes(routes: RouteObject[]): void {
export function addRoutesToAllRoutes(routes: RouteObject[]): RouteObject[] {
const added: RouteObject[] = [];
routes.forEach(route => {
const extractedChildRoutes = getChildRoutesRecursively(route);

extractedChildRoutes.forEach(r => {
allRoutes.add(r);
added.push(r);
});
});

return added;
}

/**
* Removes routes previously added via `addRoutesToAllRoutes` from the shared set. Called when a
* `<Routes>` unmounts so its routes don't linger and get matched against later, unrelated navigations
* (which produced hybrid names like `/bar/:fooId` across independent routers - see #22782).
*/
function removeRoutesFromAllRoutes(routes: RouteObject[]): void {
routes.forEach(route => {
allRoutes.delete(route);
});
}

function getChildRoutesRecursively(route: RouteObject, allRoutes: Set<RouteObject> = new Set()): Set<RouteObject> {
Expand Down Expand Up @@ -1355,13 +1377,20 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
const location = _useLocation();
const navigationType = _useNavigationType();

const routes = _createRoutesFromChildren(props.children) as RouteObject[];

// Register this `<Routes>`'s routes in the shared set for as long as it is mounted, removing them on
// unmount so they don't leak into later unrelated navigations (#22782). Tying add and remove to the
// same effect lifecycle keeps it correct under StrictMode's mount/unmount/remount.
useIsomorphicLayoutEffect(() => {
const added = addRoutesToAllRoutes(routes);

return () => removeRoutesFromAllRoutes(added);
});

useIsomorphicLayoutEffect(
() => {
const routes = _createRoutesFromChildren(props.children) as RouteObject[];

if (isMountRenderPass.current) {
addRoutesToAllRoutes(routes);

updatePageloadTransaction({
activeRootSpan: getActiveRootSpan(),
location,
Expand Down
Loading