Add dynamic parameter marker to router cache key (vercel#47957)

### What?

Took a bit to investigate this one, eventually found that the case where
it broke is this one:

```
app
├── [slug] // This matches `/blog`
│   └── page.js
└── blog
    └── [name] // This matches `/blog/a-post`
        └── page.js
```

The router cache key is based on the "static key" / "dynamic parameter
value" in the tree. This means that the cache key for `/blog` that
matches `/[slug]` would be the same as the static segment `blog`. This
caused the cache to become intertwined between those paths, it's
accidental that the router got stuck in that case, main reason it got
stuck is that the fetch for the RSC payload returned a deeper value than
expected. In `walkAddRefetch` we bailed because that walked the
`segmentPath` didn't match up.

The underlying problem with this was that the render would override the
cache nodes incorrectly. This would also cause wrong behavior, even
though that wasn't reported. E.g. `app/[slug]/layout.js` would apply on
`app/blog/[name]/page.js` because they'd share the `blog` cache node.

### How?

This PR changes the cache key to include the dynamic parameter name and
type, e.g. the dynamic segment `['slug', 'blog', 'd']` previously turned
into `'blog'` as the cache key, with these changes it turns into
`'slug|blog|d'`. For static segments like `blog` in `app/blog/[name]`
the key is still just `'blog'`.

I've also refactored the cases where we read the segment as the code was
duplicated in a few places.


Closes NEXT-877
Fixes vercel#47297

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation or adding/fixing Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md



## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->

fix NEXT-877

packages/next/src/client/components/layout-router.tsx

@@ -5,6 +5,7 @@ import type {
   FlightRouterState,
   FlightSegmentPath,
   ChildProp,
+  Segment,
 } from '../../server/app-render/types'
 import type { ErrorComponent } from './error-boundary'
 import { FocusAndScrollRef } from './router-reducer/router-reducer-types'
@@ -24,6 +25,8 @@ import { matchSegment } from './match-segments'
 import { handleSmoothScroll } from '../../shared/lib/router/utils/handle-smooth-scroll'
 import { RedirectBoundary } from './redirect-boundary'
 import { NotFoundBoundary } from './not-found-boundary'
+import { getSegmentValue } from './router-reducer/reducers/get-segment-value'
+import { createRouterCacheKey } from './router-reducer/create-router-cache-key'
 
 /**
  * Add refetch marker to router state at the point of the current layout segment.
@@ -230,7 +233,7 @@ function InnerLayoutRouter({
   tree,
   // TODO-APP: implement `<Offscreen>` when available.
   // isActive,
-  path,
+  cacheKey,
 }: {
   parallelRouterKey: string
   url: string
@@ -239,7 +242,7 @@ function InnerLayoutRouter({
   segmentPath: FlightSegmentPath
   tree: FlightRouterState
   isActive: boolean
-  path: string
+  cacheKey: ReturnType<typeof createRouterCacheKey>
 }) {
   const context = useContext(GlobalLayoutRouterContext)
   if (!context) {
@@ -249,7 +252,7 @@ function InnerLayoutRouter({
   const { changeByServerResponse, tree: fullTree, focusAndScrollRef } = context
 
   // Read segment path from the parallel router cache node.
-  let childNode = childNodes.get(path)
+  let childNode = childNodes.get(cacheKey)
 
   // If childProp is available this means it's the Flight / SSR case.
   if (
@@ -269,7 +272,7 @@ function InnerLayoutRouter({
     } else {
       // Add the segment's subTreeData to the cache.
       // This writes to the cache when there is no item in the cache yet. It never *overwrites* existing cache items which is why it's safe in concurrent mode.
-      childNodes.set(path, {
+      childNodes.set(cacheKey, {
         status: CacheStates.READY,
         data: null,
         subTreeData: childProp.current,
@@ -278,7 +281,7 @@ function InnerLayoutRouter({
       // Mutates the prop in order to clean up the memory associated with the subTreeData as it is now part of the cache.
       childProp.current = null
       // In the above case childNode was set on childNodes, so we have to get it from the cacheNodes again.
-      childNode = childNodes.get(path)
+      childNode = childNodes.get(cacheKey)
     }
   }
 
@@ -293,7 +296,7 @@ function InnerLayoutRouter({
     /**
      * Flight data fetch kicked off during render and put into the cache.
      */
-    childNodes.set(path, {
+    childNodes.set(cacheKey, {
       status: CacheStates.DATA_FETCH,
       data: fetchServerResponse(new URL(url, location.origin), refetchTree),
       subTreeData: null,
@@ -307,7 +310,7 @@ function InnerLayoutRouter({
           : new Map(),
     })
     // In the above case childNode was set on childNodes, so we have to get it from the cacheNodes again.
-    childNode = childNodes.get(path)
+    childNode = childNodes.get(cacheKey)
   }
 
   // This case should never happen so it throws an error. It indicates there's a bug in the Next.js.
@@ -461,24 +464,27 @@ export default function OuterLayoutRouter({
   // The reason arrays are used in the data format is that these are transferred from the server to the browser so it's optimized to save bytes.
   const treeSegment = tree[1][parallelRouterKey][0]
 
-  const childPropSegment = Array.isArray(childProp.segment)
-    ? childProp.segment[1]
-    : childProp.segment
+  const childPropSegment = childProp.segment
 
   // If segment is an array it's a dynamic route and we want to read the dynamic route value as the segment to get from the cache.
-  const currentChildSegment = Array.isArray(treeSegment)
-    ? treeSegment[1]
-    : treeSegment
+  const currentChildSegmentValue = getSegmentValue(treeSegment)
 
   /**
    * Decides which segments to keep rendering, all segments that are not active will be wrapped in `<Offscreen>`.
    */
   // TODO-APP: Add handling of `<Offscreen>` when it's available.
-  const preservedSegments: string[] = [currentChildSegment]
+  const preservedSegments: Segment[] = [treeSegment]
 
   return (
     <>
       {preservedSegments.map((preservedSegment) => {
+        const isChildPropSegment = matchSegment(
+          preservedSegment,
+          childPropSegment
+        )
+        const preservedSegmentValue = getSegmentValue(preservedSegment)
+        const cacheKey = createRouterCacheKey(preservedSegment)
+
         return (
           /*
             - Error boundary
@@ -490,7 +496,7 @@ export default function OuterLayoutRouter({
               - Passed to the router during rendering to ensure it can be immediately rendered when suspending on a Flight fetch.
           */
           <TemplateContext.Provider
-            key={preservedSegment}
+            key={cacheKey}
             value={
               <ErrorBoundary errorComponent={error} errorStyles={errorStyles}>
                 <LoadingBoundary
@@ -509,14 +515,12 @@ export default function OuterLayoutRouter({
                         url={url}
                         tree={tree}
                         childNodes={childNodesForParallelRouter!}
-                        childProp={
-                          childPropSegment === preservedSegment
-                            ? childProp
-                            : null
-                        }
+                        childProp={isChildPropSegment ? childProp : null}
                         segmentPath={segmentPath}
-                        path={preservedSegment}
-                        isActive={currentChildSegment === preservedSegment}
+                        cacheKey={cacheKey}
+                        isActive={
+                          currentChildSegmentValue === preservedSegmentValue
+                        }
                       />
                     </RedirectBoundary>
                   </NotFoundBoundary>

packages/next/src/client/components/navigation.ts

@@ -14,6 +14,7 @@ import {
   // LayoutSegmentsContext,
 } from '../../shared/lib/hooks-client-context'
 import { clientHookInServerComponentError } from './client-hook-in-server-component-error'
+import { getSegmentValue } from './router-reducer/reducers/get-segment-value'
 
 const INTERNAL_URLSEARCHPARAMS_INSTANCE = Symbol(
   'internal for urlsearchparams readonly'
@@ -188,7 +189,8 @@ function getSelectedLayoutSegmentPath(
 
   if (!node) return segmentPath
   const segment = node[0]
-  const segmentValue = Array.isArray(segment) ? segment[1] : segment
+
+  const segmentValue = getSegmentValue(segment)
   if (!segmentValue || segmentValue.startsWith('__PAGE__')) return segmentPath
 
   segmentPath.push(segmentValue)

packages/next/src/client/components/router-reducer/create-router-cache-key.test.ts

@@ -0,0 +1,19 @@
+import { createRouterCacheKey } from './create-router-cache-key'
+
+describe('createRouterCacheKey', () => {
+  it('should support string segment', () => {
+    expect(createRouterCacheKey('foo')).toEqual('foo')
+  })
+
+  it('should support dynamic segment', () => {
+    expect(createRouterCacheKey(['slug', 'hello-world', 'd'])).toEqual(
+      'slug|hello-world|d'
+    )
+  })
+
+  it('should support catch all segment', () => {
+    expect(createRouterCacheKey(['slug', 'blog/hello-world', 'c'])).toEqual(
+      'slug|blog/hello-world|c'
+    )
+  })
+})

packages/next/src/client/components/router-reducer/create-router-cache-key.ts

@@ -0,0 +1,7 @@
+import { Segment } from '../../../server/app-render/types'
+
+export function createRouterCacheKey(segment: Segment) {
+  return Array.isArray(segment)
+    ? `${segment[0]}|${segment[1]}|${segment[2]}`
+    : segment
+}

packages/next/src/client/components/router-reducer/fill-cache-with-new-subtree-data.ts

@@ -2,6 +2,7 @@ import { CacheNode, CacheStates } from '../../../shared/lib/app-router-context'
 import type { FlightDataPath } from '../../../server/app-render/types'
 import { invalidateCacheByRouterState } from './invalidate-cache-by-router-state'
 import { fillLazyItemsTillLeafWithHead } from './fill-lazy-items-till-leaf-with-head'
+import { createRouterCacheKey } from './create-router-cache-key'
 
 /**
  * Fill cache with subTreeData based on flightDataPath
@@ -14,7 +15,7 @@ export function fillCacheWithNewSubTreeData(
   const isLastEntry = flightDataPath.length <= 5
   const [parallelRouteKey, segment] = flightDataPath
 
-  const segmentForCache = Array.isArray(segment) ? segment[1] : segment
+  const cacheKey = createRouterCacheKey(segment)
 
   const existingChildSegmentMap =
     existingCache.parallelRoutes.get(parallelRouteKey)
@@ -31,8 +32,8 @@ export function fillCacheWithNewSubTreeData(
     newCache.parallelRoutes.set(parallelRouteKey, childSegmentMap)
   }
 
-  const existingChildCacheNode = existingChildSegmentMap.get(segmentForCache)
-  let childCacheNode = childSegmentMap.get(segmentForCache)
+  const existingChildCacheNode = existingChildSegmentMap.get(cacheKey)
+  let childCacheNode = childSegmentMap.get(cacheKey)
 
   if (isLastEntry) {
     if (
@@ -65,7 +66,7 @@ export function fillCacheWithNewSubTreeData(
         flightDataPath[4]
       )
 
-      childSegmentMap.set(segmentForCache, childCacheNode)
+      childSegmentMap.set(cacheKey, childCacheNode)
     }
     return
   }
@@ -83,7 +84,7 @@ export function fillCacheWithNewSubTreeData(
       subTreeData: childCacheNode.subTreeData,
       parallelRoutes: new Map(childCacheNode.parallelRoutes),
     } as CacheNode
-    childSegmentMap.set(segmentForCache, childCacheNode)
+    childSegmentMap.set(cacheKey, childCacheNode)
   }
 
   fillCacheWithNewSubTreeData(

packages/next/src/client/components/router-reducer/fill-lazy-items-till-leaf-with-head.ts

@@ -1,5 +1,6 @@
 import { CacheNode, CacheStates } from '../../../shared/lib/app-router-context'
 import type { FlightRouterState } from '../../../server/app-render/types'
+import { createRouterCacheKey } from './create-router-cache-key'
 
 export function fillLazyItemsTillLeafWithHead(
   newCache: CacheNode,
@@ -17,9 +18,7 @@ export function fillLazyItemsTillLeafWithHead(
   for (const key in routerState[1]) {
     const parallelRouteState = routerState[1][key]
     const segmentForParallelRoute = parallelRouteState[0]
-    const cacheKey = Array.isArray(segmentForParallelRoute)
-      ? segmentForParallelRoute[1]
-      : segmentForParallelRoute
+    const cacheKey = createRouterCacheKey(segmentForParallelRoute)
 
     if (existingCache) {
       if (cacheKey === '__DEFAULT__') {

packages/next/src/client/components/router-reducer/get-segment-value.test.ts

@@ -0,0 +1,17 @@
+import { getSegmentValue } from './reducers/get-segment-value'
+
+describe('getSegmentValue', () => {
+  it('should support string segment', () => {
+    expect(getSegmentValue('foo')).toEqual('foo')
+  })
+
+  it('should support dynamic segment', () => {
+    expect(getSegmentValue(['slug', 'hello-world', 'd'])).toEqual('hello-world')
+  })
+
+  it('should support catch all segment', () => {
+    expect(getSegmentValue(['slug', 'blog/hello-world', 'c'])).toEqual(
+      'blog/hello-world'
+    )
+  })
+})

packages/next/src/client/components/router-reducer/invalidate-cache-below-flight-segmentpath.ts

@@ -1,5 +1,6 @@
 import type { CacheNode } from '../../../shared/lib/app-router-context'
 import type { FlightSegmentPath } from '../../../server/app-render/types'
+import { createRouterCacheKey } from './create-router-cache-key'
 
 /**
  * Fill cache up to the end of the flightSegmentPath, invalidating anything below it.
@@ -12,7 +13,7 @@ export function invalidateCacheBelowFlightSegmentPath(
   const isLastEntry = flightSegmentPath.length <= 2
   const [parallelRouteKey, segment] = flightSegmentPath
 
-  const segmentForCache = Array.isArray(segment) ? segment[1] : segment
+  const cacheKey = createRouterCacheKey(segment)
 
   const existingChildSegmentMap =
     existingCache.parallelRoutes.get(parallelRouteKey)
@@ -31,12 +32,12 @@ export function invalidateCacheBelowFlightSegmentPath(
 
   // In case of last entry don't copy further down.
   if (isLastEntry) {
-    childSegmentMap.delete(segmentForCache)
+    childSegmentMap.delete(cacheKey)
     return
   }
 
-  const existingChildCacheNode = existingChildSegmentMap.get(segmentForCache)
-  let childCacheNode = childSegmentMap.get(segmentForCache)
+  const existingChildCacheNode = existingChildSegmentMap.get(cacheKey)
+  let childCacheNode = childSegmentMap.get(cacheKey)
 
   if (!childCacheNode || !existingChildCacheNode) {
     // Bailout because the existing cache does not have the path to the leaf node
@@ -51,7 +52,7 @@ export function invalidateCacheBelowFlightSegmentPath(
       subTreeData: childCacheNode.subTreeData,
       parallelRoutes: new Map(childCacheNode.parallelRoutes),
     } as CacheNode
-    childSegmentMap.set(segmentForCache, childCacheNode)
+    childSegmentMap.set(cacheKey, childCacheNode)
   }
 
   invalidateCacheBelowFlightSegmentPath(

packages/next/src/client/components/router-reducer/invalidate-cache-by-router-state.ts

@@ -1,5 +1,6 @@
 import type { CacheNode } from '../../../shared/lib/app-router-context'
 import type { FlightRouterState } from '../../../server/app-render/types'
+import { createRouterCacheKey } from './create-router-cache-key'
 
 /**
  * Invalidate cache one level down from the router state.
@@ -12,9 +13,7 @@ export function invalidateCacheByRouterState(
   // Remove segment that we got data for so that it is filled in during rendering of subTreeData.
   for (const key in routerState[1]) {
     const segmentForParallelRoute = routerState[1][key][0]
-    const cacheKey = Array.isArray(segmentForParallelRoute)
-      ? segmentForParallelRoute[1]
-      : segmentForParallelRoute
+    const cacheKey = createRouterCacheKey(segmentForParallelRoute)
     const existingParallelRoutesCacheNode =
       existingCache.parallelRoutes.get(key)
     if (existingParallelRoutesCacheNode) {

packages/next/src/client/components/router-reducer/reducers/find-head-in-cache.ts

@@ -1,5 +1,6 @@
 import type { FlightRouterState } from '../../../../server/app-render/types'
 import type { CacheNode } from '../../../../shared/lib/app-router-context'
+import { createRouterCacheKey } from '../create-router-cache-key'
 
 export function findHeadInCache(
   cache: CacheNode,
@@ -16,7 +17,7 @@ export function findHeadInCache(
       continue
     }
 
-    const cacheKey = Array.isArray(segment) ? segment[1] : segment
+    const cacheKey = createRouterCacheKey(segment)
 
     const cacheNode = childSegmentMap.get(cacheKey)
     if (!cacheNode) {

packages/next/src/client/components/router-reducer/reducers/get-segment-value.ts

@@ -0,0 +1,5 @@
+import { Segment } from '../../../../server/app-render/types'
+
+export function getSegmentValue(segment: Segment) {
+  return Array.isArray(segment) ? segment[1] : segment
+}

test/e2e/app-dir/app-routes/app/conflicting-dynamic-static-segments/[slug]/page.tsx

@@ -0,0 +1,3 @@
+export default function Page({ params }) {
+  return <h1>Page {params.slug}</h1>
+}

test/e2e/app-dir/app-routes/app/conflicting-dynamic-static-segments/blog/[slug]/page.tsx

@@ -0,0 +1,3 @@
+export default function Page({ params }) {
+  return <h1>Blog Sub Page {params.slug}</h1>
+}

test/e2e/app-dir/app-routes/app/layout.tsx

@@ -0,0 +1,16 @@
+export const metadata = {
+  title: 'Next.js',
+  description: 'Generated by Next.js',
+}
+
+export default function RootLayout({
+  children,
+}: {
+  children: React.ReactNode
+}) {
+  return (
+    <html lang="en">
+      <body>{children}</body>
+    </html>
+  )
+}

test/e2e/app-dir/router-stuck-dynamic-static-segment/app/[slug]/page.tsx

@@ -0,0 +1,15 @@
+import Link from 'next/link'
+
+export default function Page({ params }) {
+  return (
+    <div>
+      <h1 id="slug-page">Visiting page {params.slug}</h1>
+      <Link href="/blog/a-post" style={{ display: 'block' }} id="to-blog-post">
+        Go to a post
+      </Link>
+      <Link href="/" style={{ display: 'block' }}>
+        Go home
+      </Link>
+    </div>
+  )
+}