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

Since v14.0.2, using revalidatePath in nested Parallel Routes prevents router.back from functioning #58715

Closed
1 task done
zaru opened this issue Nov 21, 2023 · 8 comments · Fixed by #63607
Closed
1 task done
Labels
bug Issue was opened via the bug report template. locked

Comments

@zaru
Copy link

zaru commented Nov 21, 2023

Link to the code that reproduces this issue

https://github.com/zaru/nested-parallel-routes

To Reproduce

  1. Click the "Open drawer" button.
  2. Click the "Open modal" button.
  3. Click the "Revalidate submit" button.
  4. The modal is expected to close but does not close.

Current vs. Expected behavior

Executing router.back() is expected to clear the Slot of Parallel Routes and close the modal, but in reality, the modal does not close. However, the URL and History do go back one step.

This unexpected behavior occurs when revalidatePath() is executed within ServerActions.

https://github.com/zaru/nested-parallel-routes/blob/c459d5d2b7425b5cc9b7c44cada8e001de02c5f4/src/app/%40modal/modal/action.ts#L7

I can close the modal when I don't execute revalidatePath(), but I'm having trouble updating the screen to the latest version without it.

Verify canary release

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

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 22.6.0: Wed Jul  5 22:22:05 PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6000
Binaries:
  Node: 18.12.1
  npm: 9.6.6
  Yarn: 3.6.3
  pnpm: N/A
Relevant Packages:
  next: 14.0.4-canary.6
  eslint-config-next: 14.0.3
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.2.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router

Additional context

This was working correctly in Next.js versions prior to v14.0.1.

@zaru zaru added the bug Issue was opened via the bug report template. label Nov 21, 2023
@zaru
Copy link
Author

zaru commented Nov 21, 2023

Unexpected behavior: The modal does not close.

2023-11-21.15.42.12.mov

Expected behavior: The modal closes.

2023-11-21.15.43.39.mov

@scastiel
Copy link

I’m having the same issue with 14.0.4 and 14.0.5-canary.14. The code is working fine with 14.0.1. 😢

@michaelcummings12
Copy link

Having this issue with 14.0.5-canary.16. It ** looked ** like it was supposed to be fixed in .15, but doesn't appear so.

#59585

14.0.1 is the last working version for me.

@sinafath
Copy link

same issue on 14.0.5-canary.18

@jasonbert
Copy link

I think this behaviour may be related to the issue described here, difference is I'm revalidating by tag.

I've got a modal displaying a blog post coming from a parallel/intercepted route, which has a "favourite" (heart) toggle button that calls a server action and revalidates the cache by tag. When "closing" the modal (router.back()), the route changes but doesn't remove the modal.

  1. Trigger modal/parallel/intercepted route by clicking on blog link (next/link).
    image

  2. Click on "favourite" toggle button, triggers server action and revalidates by tag.
    image

  3. Click on "close" (X) button to close the modal (router.back()). The route has changed, but the modal hasn't been removed.
    image

Modal Component

'use client'

import { useRouter } from "next/navigation";
import React, { MouseEventHandler, PropsWithChildren, useCallback, useEffect, useRef } from 'react';
import { Heading2 } from '../heading';
import styles from './index.module.scss';

interface ModalProps extends PropsWithChildren {
    header?: string | React.ReactNode
    footer?: React.ReactNode
    onClose?: () => void
}

export default function Modal({ children, header, footer, onClose }: ModalProps) {
    const overlay = useRef(null);

    const handleClose = useCallback(() => {
        if (onClose) onClose();
    }, [onClose]);

    const handleClick: MouseEventHandler = useCallback((e) => {
        if (onClose && e.target === overlay.current) {
            onClose();
        }
    }, [onClose]);

    const handleKeyDown = useCallback((e: KeyboardEvent) => {
        if (onClose && e.key === "Escape") onClose();
    }, [onClose]);

    useEffect(() => {
        document.addEventListener("keydown", handleKeyDown);
        return () => document.removeEventListener("keydown", handleKeyDown);
    }, [handleKeyDown]);

    return (
        <div ref={overlay} className={styles.Modal} onClick={handleClick}>
            <div className={styles.Modal__container}>
                <div className={styles.Modal__header}>
                    {header &&
                        <div className={styles.Modal__headerContent}>
                            {typeof header === 'string' ?
                                <Heading2>{header}</Heading2> :
                                header
                            }
                        </div>
                    }
                    <div className={styles.Modal__headerClose}>
                        <button onClick={handleClose} className={styles.Modal__headerCloseButton}>X</button>
                    </div>
                </div>
                <div className={styles.Modal__content}>
                    {children}
                </div>
                {footer &&
                    <div className={styles.Modal__footer}>
                        <div className={styles.Modal__footerContent}>
                            {footer}
                        </div>
                    </div>
                }
            </div>
        </div>
    )
}

export function RouterModal(props: ModalProps) {
    const router = useRouter();
    const handleClose = useCallback(() => {
        if (props.onClose) {
            props.onClose();
        }

        router.back();
    }, [router]);

    return <Modal {...props} onClose={handleClose} />
}

Action

'use server'

export async function toggleBlogFavoriteAction(blogId: string) {
    await toggleBlogFavorite(blogId);

    revalidateTag(BLOG_FAVORITES_CACHE_TAG);
}

Component

'use client';

import { startTransition, useOptimistic } from "react";
import { PrimaryButton } from "~f/framework/button";
import { toggleBlogFavoriteAction } from "./blogAction";

export default function BlogFavoriteToggleButton({ blogId, isFavorite }: { blogId: string, isFavorite: boolean }) {
    const [optimisticIsFavorite, setOptimisticIsFavorite] = useOptimistic(isFavorite);

    async function toggle() {
        startTransition(() => {
            setOptimisticIsFavorite(!optimisticIsFavorite);
        });

        await toggleBlogFavoriteAction(blogId);
    };

    return (
        <PrimaryButton onClick={toggle} style={{ minWidth: 50 }}>{optimisticIsFavorite ? '❤️' : '🖤'}</PrimaryButton>
    )
}

Source: https://github.com/scorado-ltd/scorado-examples-nextjs-cosmos
Next.js: 14.0.4

@aryzle
Copy link

aryzle commented Jan 29, 2024

experiencing the same problem in 14.1.0

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
@aryzle
Copy link

aryzle commented Mar 29, 2024

this is looking great on the latest canary version 14.2.0-canary.48, thank you!!

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 13, 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.

6 participants