fix erroneous RSC calls on hash changes (#66434)

When the router encounters a `stale` cache entry, it clears the `rsc`
data so that it can be fetched in render. All navigations (even just for
hash fragments) flow through the navigation reducer, which has logic to
discard any existing cache entries when the cache is stale.

This bug has become more obvious after removing the default 30s cache,
which would previously have masked it.

This updates the existing handling that clears flight data to not do so
if only the hash changes as there would be no server changes in this
case.

<!-- 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

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating 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 #

-->

packages/next/src/client/components/router-reducer/handle-mutable.ts

@@ -60,10 +60,7 @@ export function handleMutable(
           : state.focusAndScrollRef.apply
         : // If shouldScroll is false then we should not apply scroll and focus management.
           false,
-      onlyHashChange:
-        !!mutable.hashFragment &&
-        state.canonicalUrl.split('#', 1)[0] ===
-          mutable.canonicalUrl?.split('#', 1)[0],
+      onlyHashChange: mutable.onlyHashChange || false,
       hashFragment: shouldScroll
         ? // Empty hash should trigger default behavior of scrolling layout into view.
           // #top is handled in layout-router.

packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts

@@ -156,6 +156,16 @@ function navigateReducer_noPPR(
         return handleExternalUrl(state, mutable, href, pendingPush)
       }
 
+      const updatedCanonicalUrl = canonicalUrlOverride
+        ? createHrefFromUrl(canonicalUrlOverride)
+        : href
+
+      // Track if the navigation was only an update to the hash fragment
+      mutable.onlyHashChange =
+        !!hash &&
+        state.canonicalUrl.split('#', 1)[0] ===
+          updatedCanonicalUrl.split('#', 1)[0]
+
       let currentTree = state.tree
       const currentCache = state.cache
       let scrollableSegments: FlightSegmentPath[] = []
@@ -201,12 +211,15 @@ function navigateReducer_noPPR(
 
           if (
             prefetchValues.status === PrefetchCacheEntryStatus.stale &&
+            !mutable.onlyHashChange &&
             !isFirstRead
           ) {
             // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations
             // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data,
             // while copying over the `loading` for the segment that contains the page data.
             // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use.
+
+            // We skip this branch if only the hash fragment has changed, as we don't want to trigger a lazy fetch in that case
             applied = triggerLazyFetchForLeafSegments(
               cache,
               currentCache,
@@ -263,9 +276,7 @@ function navigateReducer_noPPR(
       }
 
       mutable.patchedTree = currentTree
-      mutable.canonicalUrl = canonicalUrlOverride
-        ? createHrefFromUrl(canonicalUrlOverride)
-        : href
+      mutable.canonicalUrl = updatedCanonicalUrl
       mutable.pendingPush = pendingPush
       mutable.scrollableSegments = scrollableSegments
       mutable.hashFragment = hash
@@ -330,6 +341,16 @@ function navigateReducer_PPR(
         return handleExternalUrl(state, mutable, href, pendingPush)
       }
 
+      const updatedCanonicalUrl = canonicalUrlOverride
+        ? createHrefFromUrl(canonicalUrlOverride)
+        : href
+
+      // Track if the navigation was only an update to the hash fragment
+      mutable.onlyHashChange =
+        !!hash &&
+        state.canonicalUrl.split('#', 1)[0] ===
+          updatedCanonicalUrl.split('#', 1)[0]
+
       let currentTree = state.tree
       const currentCache = state.cache
       let scrollableSegments: FlightSegmentPath[] = []
@@ -452,12 +473,15 @@ function navigateReducer_PPR(
 
             if (
               prefetchValues.status === PrefetchCacheEntryStatus.stale &&
+              !mutable.onlyHashChange &&
               !isFirstRead
             ) {
               // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations
               // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data,
               // while copying over the `loading` for the segment that contains the page data.
               // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use.
+
+              // We skip this branch if only the hash fragment has changed, as we don't want to trigger a lazy fetch in that case
               applied = triggerLazyFetchForLeafSegments(
                 cache,
                 currentCache,
@@ -515,9 +539,7 @@ function navigateReducer_PPR(
       }
 
       mutable.patchedTree = currentTree
-      mutable.canonicalUrl = canonicalUrlOverride
-        ? createHrefFromUrl(canonicalUrlOverride)
-        : href
+      mutable.canonicalUrl = updatedCanonicalUrl
       mutable.pendingPush = pendingPush
       mutable.scrollableSegments = scrollableSegments
       mutable.hashFragment = hash

packages/next/src/client/components/router-reducer/router-reducer-types.ts

@@ -38,6 +38,7 @@ export interface Mutable {
   hashFragment?: string
   shouldScroll?: boolean
   preserveCustomHistoryState?: boolean
+  onlyHashChange?: boolean
 }
 
 export interface ServerActionMutable extends Mutable {

test/e2e/app-dir/navigation/app/hash/page.js

@@ -29,6 +29,11 @@ export default function HashPage() {
       <Link href="/hash#non-existent" id="link-to-non-existent">
         To non-existent
       </Link>
+      <div>
+        <Link href="?with-query-param#hash-160" id="link-to-query-param">
+          To 160 (with query param)
+        </Link>
+      </div>
       <div>
         {items.map((item) => (
           <div key={item.id}>

test/e2e/app-dir/navigation/navigation.test.ts

@@ -3,7 +3,7 @@ import { retry, waitFor } from 'next-test-utils'
 import type { Response } from 'playwright'
 
 describe('app dir - navigation', () => {
-  const { next, isNextDev, isNextDeploy } = nextTestSetup({
+  const { next, isNextDev, isNextStart, isNextDeploy } = nextTestSetup({
     files: __dirname,
   })
 
@@ -151,7 +151,17 @@ describe('app dir - navigation', () => {
 
   describe('hash', () => {
     it('should scroll to the specified hash', async () => {
-      const browser = await next.browser('/hash')
+      let hasRscRequest = false
+      const browser = await next.browser('/hash', {
+        beforePageLoad(page) {
+          page.on('request', async (req) => {
+            const headers = await req.allHeaders()
+            if (headers['rsc']) {
+              hasRscRequest = true
+            }
+          })
+        },
+      })
 
       const checkLink = async (
         val: number | string,
@@ -166,13 +176,31 @@ describe('app dir - navigation', () => {
         )
       }
 
-      await checkLink(6, 114)
-      await checkLink(50, 730)
-      await checkLink(160, 2270)
-      await checkLink(300, 4230)
-      await checkLink(500, 7030) // this one is hash only (`href="#hash-500"`)
+      if (isNextStart) {
+        await browser.waitForIdleNetwork()
+        // there should be an RSC call for the prefetch
+        expect(hasRscRequest).toBe(true)
+      }
+
+      // Wait for all network requests to finish, and then initialize the flag
+      // used to determine if any RSC requests are made
+      hasRscRequest = false
+
+      await checkLink(6, 128)
+      await checkLink(50, 744)
+      await checkLink(160, 2284)
+      await checkLink(300, 4244)
+      await checkLink(500, 7044) // this one is hash only (`href="#hash-500"`)
       await checkLink('top', 0)
       await checkLink('non-existent', 0)
+
+      // there should have been no RSC calls to fetch data
+      expect(hasRscRequest).toBe(false)
+
+      // There should be an RSC request if the query param is changed
+      await checkLink('query-param', 2284)
+      await browser.waitForIdleNetwork()
+      expect(hasRscRequest).toBe(true)
     })
 
     it('should not scroll to hash when scroll={false} is set', async () => {
@@ -201,11 +229,11 @@ describe('app dir - navigation', () => {
         )
       }
 
-      await checkLink(6, 94)
-      await checkLink(50, 710)
-      await checkLink(160, 2250)
-      await checkLink(300, 4210)
-      await checkLink(500, 7010) // this one is hash only (`href="#hash-500"`)
+      await checkLink(6, 108)
+      await checkLink(50, 724)
+      await checkLink(160, 2264)
+      await checkLink(300, 4224)
+      await checkLink(500, 7024) // this one is hash only (`href="#hash-500"`)
       await checkLink('top', 0)
       await checkLink('non-existent', 0)
     })