diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx index 09751f0f920d..31f7765f5374 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -103,6 +103,22 @@ const DeepTeamRoutes = () => ( ); +// Two independent descendant 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 = () => ( + + Foo} /> + +); + +const BarRoutes = () => ( + + Bar} /> + +); + const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement); root.render( @@ -110,6 +126,8 @@ root.render( } /> } /> } /> + } /> + } /> } /> , diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx index e0b372a51965..ff6b75a0a536 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx @@ -16,6 +16,12 @@ const Index = () => { navigate deep member + + navigate foo + + + navigate bar + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts index 3c4598be922a..9e25f8d65d9d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts @@ -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'; diff --git a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx index 61e11e46c7ac..62bf4521c2b4 100644 --- a/packages/react/src/reactrouter-compat-utils/instrumentation.tsx +++ b/packages/react/src/reactrouter-compat-utils/instrumentation.tsx @@ -766,13 +766,20 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio const stableLocationParam = typeof locationArg === 'string' || locationArg?.pathname ? (locationArg as { pathname: string }) : location; + // Register this ``'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, @@ -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 + * `` 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 = new Set()): Set { @@ -1355,13 +1377,20 @@ export function createV6CompatibleWithSentryReactRouterRouting

`'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,