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

router.push("/") broken when intercepting routes as modal with parallel routes #51711

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

router.push("/") broken when intercepting routes as modal with parallel routes #51711

pedro757 opened this issue Jun 23, 2023 · 12 comments · Fixed by #63607
Labels
bug Issue was opened via the bug report template. locked

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)

No response

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

https://codesandbox.io/p/sandbox/fast-bush-qlpdwm?file=%2Fapp%2Fhome%2Fpage.tsx

To Reproduce

use push("/") in a modal

Describe the Bug

This is what I get
image

Expected Behavior

I expect it to actually redirect me to the path I telling it to redirect

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

@pedro757 pedro757 added the bug Issue was opened via the bug report template. label Jun 23, 2023
@m10rten
Copy link

m10rten commented Jun 30, 2023

Relatable issue here:
When having a modal in @modal folder that is a intercepted version of another page eg: signin modal, intercepted from /signin.
Whenever I press a <Link href="terms" /> it does not remove the modal, even though I set this in my Dialog from @shadcn ui:

"use client";

import { useRouter } from "next/navigation";
import SigninContent from "@/app/(auth)/signin/content";
import { Dialog, DialogContent } from "@/components/ui/dialog";

export default function SigninModalContent() {
  const router = useRouter();
  const handleOnOpenChange = (open: boolean) => {
    if (!open) {
      router.back();
    }
  };
  return (
    <Dialog open={true} onOpenChange={handleOnOpenChange}>
      <DialogContent>
        <SigninContent />
      </DialogContent>
    </Dialog>
  );
}

@m10rten
Copy link

m10rten commented Jun 30, 2023

Relatable issue here: When having a modal in @modal folder that is a intercepted version...

I think I did something

  useEffect(() => {
    if (path !== "/signin") {
      setOpen(false);
    } else {
      setOpen(true);
    }
  }, [path]);

@m10rten
Copy link

m10rten commented Jun 30, 2023

To comment on main issue, I've seen a default.tsx with return null; in many projects, that could be the case.

@BrunoFerreira95
Copy link

Check the import
import { useRouter } from 'next/router'
or
import { useRouter } from 'next/navigation'

maybe be valide.
A new update in the docs about the defalut.tsx was release.

@TheLexoPlexx
Copy link

TheLexoPlexx commented Sep 10, 2023

Probably the same issue here: I fixed it through push to the parent directory first. I don't know why that works: https://github.com/TheLexoPlexx/senator/blob/ca7385cda942c2dced692bcf014783db6b8c9e86/app/einsatzplanung/%5Bid%5D/%40user/add/setUsersForm.tsx#L24C3-L25

next 13.4.19

@pedro757
Copy link
Contributor Author

This issue still persist in v13.5.2

@david0178418
Copy link

Seems related to #49662

As noted by others in the ticket, routing back is working. But pushing new state does not. Even using the native (navigation || history).back() will unmount the modal. But history.pushState(...) does not, which I'd imagine Link is using under the hood with simple linking fallback when js is not enabled.

@jchamale
Copy link

Happening to me as well, I'm intercepting a route like /member/menu/card and in this dialog I want to move to /member/menu/information. The intercept route never closes the dialog.

@samcx
Copy link
Member

samcx commented Jan 9, 2024

@pedro757 Thank you for submitting an issue!

I don't think this is an issue anymore with router.back() or router.push(), but I believe router.refresh() is still broken → #51714.

Let me know if you can confirm so we can close this issue in favor of the one I shared (the other one you created)!

@pedro757
Copy link
Contributor Author

pedro757 commented Jan 9, 2024

Hey @samcx ! Thanks for addressing this issue, I just updated the code sandbox to the latest canary v14.0.5-canary.45 and I see the background component (not the modal) disappears when refreshing, idk if that's the expected behavior. Other than that I believe router.back() androuter.refresh() are working good considering this sandbox. I haven't used this functionality in a real project though.

You can check out the code sandbox to make sure that's the expected behavior.

If everything is alright I guess you can close both issues in next release.

@samcx
Copy link
Member

samcx commented Jan 25, 2024

@pedro757 I can confirm this is a separate 🐛 with router.refresh()—the /home/add should not disappear after router.refresh(). I have shared this with the team to take a look!

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
bug Issue was opened via the bug report template. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants