fix(v8/react): Use Set as the allRoutes container. (#14878) (#14884)

backport of #14878 Co-authored-by: Onur Temizkan <[email protected]>
getsentry · Jan 7, 2025 · 77cabfb · 77cabfb
1 parent 6fa3797
commit 77cabfb
Show file tree

Hide file tree

Showing 6 changed files with 545 additions and 212 deletions.
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
@@ -3,6 +3,7 @@ import React from 'react';
 import ReactDOM from 'react-dom/client';
 import {
   BrowserRouter,
+  Outlet,
   Route,
   Routes,
   createRoutesFromChildren,
@@ -48,17 +49,28 @@ const DetailsRoutes = () => (
   </SentryRoutes>
 );
 
+const DetailsRoutesAlternative = () => (
+  <SentryRoutes>
+    <Route path=":detailId" element={<div id="details">Details</div>} />
+  </SentryRoutes>
+);
+
 const ViewsRoutes = () => (
   <SentryRoutes>
     <Route index element={<div id="views">Views</div>} />
     <Route path="views/:viewId/*" element={<DetailsRoutes />} />
+    <Route path="old-views/:viewId/*" element={<DetailsRoutesAlternative />} />
   </SentryRoutes>
 );
 
 const ProjectsRoutes = () => (
   <SentryRoutes>
-    <Route path="projects/:projectId/*" element={<ViewsRoutes />}></Route>
-    <Route path="*" element={<div>No Match Page</div>} />
+    <Route path="projects" element={<Outlet />}>
+      <Route index element={<div>Project Page Root</div>} />
+      <Route path="*" element={<Outlet />}>
+        <Route path=":projectId/*" element={<ViewsRoutes />} />
+      </Route>
+    </Route>
   </SentryRoutes>
 );
 
@@ -67,7 +79,7 @@ root.render(
   <BrowserRouter>
     <SentryRoutes>
       <Route path="/" element={<Index />} />
-      <Route path="/*" element={<ProjectsRoutes />}></Route>
+      <Route path="/*" element={<ProjectsRoutes />} />
     </SentryRoutes>
   </BrowserRouter>,
 );
diff --git a/...packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx b/...packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx
@@ -8,6 +8,9 @@ const Index = () => {
       <Link to="/projects/123/views/456/789" id="navigation">
         navigate
       </Link>
+      <Link to="/projects/123/old-views/345/654" id="old-navigation">
+        navigate old
+      </Link>
     </>
   );
 };

diff --git a/...s/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts b/...s/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts
@@ -10,6 +10,7 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) =
 
   const rootSpan = await transactionPromise;
 
+  expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
   expect(rootSpan).toMatchObject({
     contexts: {
       trace: {
@@ -24,6 +25,30 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) =
   });
 });
 
+test('sends a pageload transaction with a parameterized URL - alternative route', async ({ page }) => {
+  const transactionPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
+  });
+
+  await page.goto(`/projects/234/old-views/234/567`);
+
+  const rootSpan = await transactionPromise;
+
+  expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
+  expect(rootSpan).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'pageload',
+        origin: 'auto.pageload.react.reactrouter_v6',
+      },
+    },
+    transaction: '/projects/:projectId/old-views/:viewId/:detailId',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+});
+
 test('sends a navigation transaction with a parameterized URL', async ({ page }) => {
   const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
     return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
@@ -52,6 +77,8 @@ test('sends a navigation transaction with a parameterized URL', async ({ page })
   const linkElement = page.locator('id=navigation');
 
   const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]);
+
+  expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
   expect(navigationTxn).toMatchObject({
     contexts: {
       trace: {
@@ -65,3 +92,47 @@ test('sends a navigation transaction with a parameterized URL', async ({ page })
     },
   });
 });
+
+test('sends a navigation transaction with a parameterized URL - alternative route', async ({ page }) => {
+  const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
+  });
+
+  const navigationTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation';
+  });
+
+  await page.goto(`/`);
+  const pageloadTxn = await pageloadTxnPromise;
+
+  expect(pageloadTxn).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'pageload',
+        origin: 'auto.pageload.react.reactrouter_v6',
+      },
+    },
+    transaction: '/',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+
+  const linkElement = page.locator('id=old-navigation');
+
+  const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]);
+
+  expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
+  expect(navigationTxn).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'navigation',
+        origin: 'auto.navigation.react.reactrouter_v6',
+      },
+    },
+    transaction: '/projects/:projectId/old-views/:viewId/:detailId',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+});
diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx
@@ -180,7 +180,7 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio
     return origUseRoutes;
   }
 
-  const allRoutes: RouteObject[] = [];
+  const allRoutes: Set<RouteObject> = new Set();
 
   const SentryRoutes: React.FC<{
     children?: React.ReactNode;
@@ -207,18 +207,29 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio
 
       if (isMountRenderPass.current) {
         routes.forEach(route => {
-          allRoutes.push(...getChildRoutesRecursively(route));
+          const extractedChildRoutes = getChildRoutesRecursively(route);
+
+          extractedChildRoutes.forEach(r => {
+            allRoutes.add(r);
+          });
         });
 
-        updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes, undefined, undefined, allRoutes);
+        updatePageloadTransaction(
+          getActiveRootSpan(),
+          normalizedLocation,
+          routes,
+          undefined,
+          undefined,
+          Array.from(allRoutes),
+        );
         isMountRenderPass.current = false;
       } else {
         handleNavigation({
           location: normalizedLocation,
           routes,
           navigationType,
           version,
-          allRoutes,
+          allRoutes: Array.from(allRoutes),
         });
       }
     }, [navigationType, stableLocationParam]);
@@ -343,14 +354,18 @@ function locationIsInsideDescendantRoute(location: Location, routes: RouteObject
   return false;
 }
 
-function getChildRoutesRecursively(route: RouteObject, allRoutes: RouteObject[] = []): RouteObject[] {
-  if (route.children && !route.index) {
-    route.children.forEach(child => {
-      allRoutes.push(...getChildRoutesRecursively(child, allRoutes));
-    });
-  }
+function getChildRoutesRecursively(route: RouteObject, allRoutes: Set<RouteObject> = new Set()): Set<RouteObject> {
+  if (!allRoutes.has(route)) {
+    allRoutes.add(route);
 
-  allRoutes.push(route);
+    if (route.children && !route.index) {
+      route.children.forEach(child => {
+        const childRoutes = getChildRoutesRecursively(child, allRoutes);
+
+        childRoutes.forEach(r => allRoutes.add(r));
+      });
+    }
+  }
 
   return allRoutes;
 }
@@ -513,7 +528,7 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
     return Routes;
   }
 
-  const allRoutes: RouteObject[] = [];
+  const allRoutes: Set<RouteObject> = new Set();
 
   const SentryRoutes: React.FC<P> = (props: P) => {
     const isMountRenderPass = React.useRef(true);
@@ -527,18 +542,22 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
 
         if (isMountRenderPass.current) {
           routes.forEach(route => {
-            allRoutes.push(...getChildRoutesRecursively(route));
+            const extractedChildRoutes = getChildRoutesRecursively(route);
+
+            extractedChildRoutes.forEach(r => {
+              allRoutes.add(r);
+            });
           });
 
-          updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, allRoutes);
+          updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, Array.from(allRoutes));
           isMountRenderPass.current = false;
         } else {
           handleNavigation({
             location,
             routes,
             navigationType,
             version,
-            allRoutes,
+            allRoutes: Array.from(allRoutes),
           });
         }
       },