Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refreshing an intercepted parallel route (modal) breaks router #51714

Closed
1 task done
pedro757 opened this issue Jun 23, 2023 · 13 comments · Fixed by #63607
Closed
1 task done

Refreshing an intercepted parallel route (modal) breaks router #51714

pedro757 opened this issue Jun 23, 2023 · 13 comments · Fixed by #63607
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@pedro757
Copy link
Contributor

pedro757 commented Jun 23, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #1 SMP PREEMPT_DYNAMIC Sun Jun  4 12:31:46 UTC 2023
    Binaries:
      Node: 18.16.0
      npm: 9.6.7
      Yarn: N/A
      pnpm: 8.6.1
    Relevant packages:
      next: 13.4.7
      eslint-config-next: 13.4.7
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.3

Which area(s) of Next.js are affected? (leave empty if unsure)

App directory (appDir: true), Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue or a replay of the bug

https://codesandbox.io/p/sandbox/goofy-williamson-myrykt

To Reproduce

Steps to reproduce:

  1. Click Go to Login Page Button
  2. Click Refresh Button
  3. Click Go Back Button

Describe the Bug

When refreshing an intercepted parallel route the router stops working and then you can't go to any other route because the intercepted route doesn't disappear.

Expected Behavior

I expect the intercepted parallel route to disappear and take me to the correct route

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-2030

@pedro757 pedro757 added the bug Issue was opened via the bug report template. label Jun 23, 2023
@github-actions github-actions bot added area: app App directory (appDir: true) Navigation Related to Next.js linking (e.g., <Link>) and navigation. labels Jun 23, 2023
@jmarbutt
Copy link

I am having a very similar issue.

@Fredkiss3
Copy link
Contributor

Tested with the latest canary version of nextjs, it's still present.

@joeleonard212
Copy link

joeleonard212 commented Aug 15, 2023

I'm having the same issue as described from @pedro757

When calling router.refresh() from within a @modal/intercept/page.tsx rendered component, the underlying server component doesn't fetch new data.

Also, if I call router.back() right after, the component persist rendered on the back url and can only be removed manually refreshing the browser.

When the component is called on its own normal route (so when I refresh the browser from the modal view) is working fine

@joeleonard212
Copy link

Hey guys, it seems they've released new docs and an extensive explanation of how the cache works. Everything is here!
#54075

@Fredkiss3
Copy link
Contributor

@joeleonard212 This is a total different discussion from this issue.

@joeleonard212
Copy link

joeleonard212 commented Aug 17, 2023

@joeleonard212 This is a total different discussion from this issue.

You sure ?

The Component Cache keeps track of each rendered segment separately, in doing so it unlocks a bunch of features:

  • Navigating between pages reusing segments (Partial Navigation).
  • Instant back/forward navigation that preserves the browser scroll position
  • Parallel Routes / Interception, as you can keep the segment cache nodes of multiple pages at the same time. The Router Tree is the other piece in making Parallel Routes work as that decides what is being shown on screen._
  • Purging individual segments of the router cache (i.e. router.refresh() purges the entire cache, but revalidatePath('/dashboard') will only purge the client-side router cache below the specified dashboard path in the near future).

I found it usefull since personally I fixed changing caching strategy. If a component requires interaction with the user, I just don't rely on router.refresh() but I manage the cache using tags and server actions. Also, to correctly use intercepted routes, you should check if you are taking care of router tree

@Fredkiss3
Copy link
Contributor

@joeleonard212
Can you share the fix then ?

@joeleonard212
Copy link

joeleonard212 commented Aug 17, 2023

@joeleonard212 Can you share the fix then ?

Sure, hope this can be helpfull https://github.com/joeleonard212/Intercept_and_Revalidate/tree/main

this is just an example of how I'm using the following to let things work:

server_actions
unstable_cache
revalidate_tag (or revalidate_path, depending on your needing)

I think what a lot of people is missing is that the use of server_actions and tags to manage stored data is the direction Next.js will likely follow in the future

@joeleonard212
Copy link

joeleonard212 commented Aug 17, 2023

sample-2023-08-17_18.24.53.mp4

@joeleonard212
Copy link

joeleonard212 commented Aug 17, 2023

Just want to point out that I'm using the unstable_cache cause relying on a third party libraries to fetch data.

image

If you actually use await fetch yourself, it's even simpler cause you do not need to use unstable_cache to tag fetched data.
My next step is adopting useOptimistic() hook to let the update be instantaneous

I use arch btw

@gragland
Copy link

gragland commented Aug 24, 2023

Also having this issue on 13.4.20-canary.4. The problem persists even after navigating away from the intercepted route with <Link> or router.push.

@akomm
Copy link

akomm commented Aug 29, 2023

I think it has nothing to do with specifically intercepting routes. It just happens with parallel routes:

#54723

Also: soft navigating does not unmount the page in the parallel route slot. So modals won't disappear that easy. Its not a bug. But using refresh is bugged and routing stops working.

@samcx samcx added the linear: next Confirmed issue that is tracked by the Next.js team. label Jan 9, 2024
timneutkens pushed a commit that referenced this issue Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.

### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.

### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.

**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030
ztanner added a commit that referenced this issue Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.

### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.

### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.

**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants