fix(turbopack): correctly handle catchall specificity (#68800)

(generated by graphite) ### What? Introduce `get_specificity` method to determine the specificity of routes. This facilitates more accurate route matching for catchall routes by comparing route specificity. Modify existing route matching logic to incorporate specificity checks and avoid route conflicts. Add corresponding tests to validate the new specificity handling mechanism. Fixes #67045 Closes PACK-3154
vercel · Aug 20, 2024 · aa90fe9 · aa90fe9
1 parent 15aeb92
commit aa90fe9
Show file tree

Hide file tree

Showing 6 changed files with 62 additions and 10 deletions.
diff --git a/crates/next-core/src/app_structure.rs b/crates/next-core/src/app_structure.rs
@@ -474,6 +474,22 @@ impl LoaderTree {
 
         true
     }
+
+    /// Returns the specificity of the page (i.e. the number of segments
+    /// affecting the path)
+    pub fn get_specificity(&self) -> usize {
+        if &*self.segment == "__PAGE__" {
+            return AppPath::from(self.page.clone()).len();
+        }
+
+        let mut specificity = 0;
+
+        for (_, tree) in &self.parallel_routes {
+            specificity = specificity.max(tree.get_specificity());
+        }
+
+        specificity
+    }
 }
 
 #[derive(
@@ -525,10 +541,6 @@ fn is_group_route(name: &str) -> bool {
     name.starts_with('(') && name.ends_with(')')
 }
 
-fn is_catchall_route(name: &str) -> bool {
-    name.starts_with("[...") && name.ends_with(']')
-}
-
 fn match_parallel_route(name: &str) -> Option<&str> {
     name.strip_prefix('@')
 }
@@ -909,7 +921,6 @@ fn directory_tree_to_loader_tree_internal(
 
         let mut child_app_page = app_page.clone();
         let mut illegal_path_error = None;
-        let is_current_directory_catchall = is_catchall_route(subdir_name);
 
         // When constructing the app_page fails (e. g. due to limitations of the order),
         // we only want to emit the error when there are actual pages below that
@@ -947,11 +958,12 @@ fn directory_tree_to_loader_tree_internal(
             }
 
             if let Some(current_tree) = tree.parallel_routes.get("children") {
-                if is_current_directory_catchall && subtree.has_only_catchall() {
-                    // there's probably already a more specific page in the
-                    // slot.
-                } else if current_tree.has_only_catchall() {
-                    tree.parallel_routes.insert("children".into(), subtree);
+                if current_tree.has_only_catchall()
+                    && (!subtree.has_only_catchall()
+                        || current_tree.get_specificity() < subtree.get_specificity())
+                {
+                    tree.parallel_routes
+                        .insert("children".into(), subtree.clone());
                 }
             } else {
                 tree.parallel_routes.insert("children".into(), subtree);

diff --git a/test/e2e/app-dir/catchall-specificity/app/(group)/specific/[[...slug]]/page.tsx b/test/e2e/app-dir/catchall-specificity/app/(group)/specific/[[...slug]]/page.tsx
@@ -0,0 +1,3 @@
+export default function Page() {
+  return <div>Specific Page</div>
+}
diff --git a/test/e2e/app-dir/catchall-specificity/app/[[...slug]]/page.tsx b/test/e2e/app-dir/catchall-specificity/app/[[...slug]]/page.tsx
@@ -0,0 +1,3 @@
+export default function Page() {
+  return <div>Generic Page</div>
+}
diff --git a/test/e2e/app-dir/catchall-specificity/app/layout.tsx b/test/e2e/app-dir/catchall-specificity/app/layout.tsx
@@ -0,0 +1,9 @@
+import { ReactNode } from 'react'
+
+export default function Root({ children }: { children: ReactNode }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/e2e/app-dir/catchall-specificity/catchall-specificity.test.ts b/test/e2e/app-dir/catchall-specificity/catchall-specificity.test.ts
@@ -0,0 +1,19 @@
+import { nextTestSetup } from 'e2e-utils'
+
+describe('catchall-specificity', () => {
+  const { next } = nextTestSetup({
+    files: __dirname,
+  })
+
+  it('should match the generic catchall correctly', async () => {
+    const browser = await next.browser('/foo')
+
+    expect(await browser.elementByCss('body').text()).toContain('Generic Page')
+  })
+
+  it('should match the specific catchall correctly', async () => {
+    const browser = await next.browser('/specific')
+
+    expect(await browser.elementByCss('body').text()).toContain('Specific Page')
+  })
+})
diff --git a/test/e2e/app-dir/catchall-specificity/next.config.js b/test/e2e/app-dir/catchall-specificity/next.config.js
@@ -0,0 +1,6 @@
+/**
+ * @type {import('next').NextConfig}
+ */
+const nextConfig = {}
+
+module.exports = nextConfig