From 9f509b44a500d59948d83f0b957c4a6f38605530 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Tue, 10 Sep 2024 16:25:17 -0700 Subject: [PATCH] bugfix: actions that self-redirect should be handled gracefully --- .../next/src/client/components/app-router.tsx | 31 ++++++++++ .../internal/helpers/use-error-handler.ts | 5 ++ .../create-initial-router-state.ts | 7 ++- .../router-reducer/prefetch-cache-utils.ts | 5 +- .../reducers/server-action-reducer.ts | 51 ++++++++++++++-- .../src/shared/lib/router/action-queue.ts | 20 ------- test/e2e/app-dir/actions/app-action.test.ts | 58 +++++++++++++++++++ .../app-dir/actions/app/redirect/actions.ts | 17 ++++++ .../app-dir/actions/app/redirect/layout.tsx | 21 +++++++ .../actions/app/redirect/other/page.tsx | 3 + .../e2e/app-dir/actions/app/redirect/page.tsx | 3 + .../actions/app/self-redirect/actions.ts | 17 ++++++ .../actions/app/self-redirect/layout.tsx | 21 +++++++ .../actions/app/self-redirect/other/page.tsx | 3 + .../actions/app/self-redirect/page.tsx | 3 + 15 files changed, 234 insertions(+), 31 deletions(-) create mode 100644 test/e2e/app-dir/actions/app/redirect/actions.ts create mode 100644 test/e2e/app-dir/actions/app/redirect/layout.tsx create mode 100644 test/e2e/app-dir/actions/app/redirect/other/page.tsx create mode 100644 test/e2e/app-dir/actions/app/redirect/page.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/actions.ts create mode 100644 test/e2e/app-dir/actions/app/self-redirect/layout.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/other/page.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/page.tsx diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 78a66a3741ca6f..74f392fca45830 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -58,6 +58,12 @@ import type { FlightRouterState } from '../../server/app-render/types' import { useNavFailureHandler } from './nav-failure-handler' import { useServerActionDispatcher } from '../app-call-server' import type { AppRouterActionQueue } from '../../shared/lib/router/action-queue' +import { + getRedirectTypeFromError, + getURLFromRedirectError, + isRedirectError, + RedirectType, +} from './redirect' const globalMutable: { pendingMpaPath?: string @@ -369,6 +375,31 @@ function Router({ } }, [dispatch]) + useEffect(() => { + // Ensure that any redirect errors that bubble up outside of the RedirectBoundary + // are caught and handled by the router. + function handle(event: ErrorEvent | PromiseRejectionEvent) { + const error = 'reason' in event ? event.reason : event.error + if (isRedirectError(error)) { + event.preventDefault() + const url = getURLFromRedirectError(error) + const redirectType = getRedirectTypeFromError(error) + if (redirectType === RedirectType.push) { + appRouter.push(url, {}) + } else { + appRouter.replace(url, {}) + } + } + } + window.addEventListener('error', handle) + window.addEventListener('unhandledrejection', handle) + + return () => { + window.removeEventListener('error', handle) + window.removeEventListener('unhandledrejection', handle) + } + }, [appRouter]) + // When mpaNavigation flag is set do a hard navigation to the new url. // Infinitely suspend because we don't actually want to rerender any child // components with the new URL and any entangled state updates shouldn't diff --git a/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts b/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts index d8006f6cf00628..a84402337f4973 100644 --- a/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts +++ b/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts @@ -58,6 +58,11 @@ if (typeof window !== 'undefined') { 'unhandledrejection', (ev: WindowEventMap['unhandledrejection']): void => { const reason = ev?.reason + if (isNextRouterError(reason)) { + ev.preventDefault() + return + } + if ( !reason || !(reason instanceof Error) || diff --git a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts index 465349fdffdc27..ebfd4bc33dbddc 100644 --- a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts +++ b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts @@ -4,8 +4,8 @@ import type { FlightDataPath } from '../../../server/app-render/types' import { createHrefFromUrl } from './create-href-from-url' import { fillLazyItemsTillLeafWithHead } from './fill-lazy-items-till-leaf-with-head' import { extractPathFromFlightRouterState } from './compute-changed-path' -import { createPrefetchCacheEntryForInitialLoad } from './prefetch-cache-utils' -import type { PrefetchCacheEntry } from './router-reducer-types' +import { createSeededPrefetchCacheEntry } from './prefetch-cache-utils' +import { PrefetchKind, type PrefetchCacheEntry } from './router-reducer-types' import { addRefreshMarkerToActiveParallelSegments } from './refetch-inactive-parallel-segments' import { getFlightDataPartsFromPath } from '../../flight-data-helpers' @@ -112,7 +112,7 @@ export function createInitialRouterState({ location.origin ) - createPrefetchCacheEntryForInitialLoad({ + createSeededPrefetchCacheEntry({ url, data: { flightData: [normalizedFlightData], @@ -125,6 +125,7 @@ export function createInitialRouterState({ tree: initialState.tree, prefetchCache: initialState.prefetchCache, nextUrl: initialState.nextUrl, + kind: PrefetchKind.AUTO, }) } diff --git a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts index 5767ea5e914a99..a2133966c90a05 100644 --- a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts +++ b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts @@ -249,19 +249,20 @@ function prefixExistingPrefetchCacheEntry({ /** * Use to seed the prefetch cache with data that has already been fetched. */ -export function createPrefetchCacheEntryForInitialLoad({ +export function createSeededPrefetchCacheEntry({ nextUrl, tree, prefetchCache, url, data, + kind, }: Pick & { url: URL data: FetchServerResponseResult + kind: PrefetchKind }) { // The initial cache entry technically includes full data, but it isn't explicitly prefetched -- we just seed the // prefetch cache so that we can skip an extra prefetch request later, since we already have the data. - const kind = PrefetchKind.AUTO // if the prefetch corresponds with an interception route, we use the nextUrl to prefix the cache key const prefetchCacheKey = data.couldBeIntercepted ? createPrefetchCacheKey(url, kind, nextUrl) diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts index 9b49a5f6ee51f6..426248fabd3469 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts @@ -21,11 +21,12 @@ const { createFromFetch, encodeReply } = ( require('react-server-dom-webpack/client') ) as typeof import('react-server-dom-webpack/client') -import type { - ReadonlyReducerState, - ReducerState, - ServerActionAction, - ServerActionMutable, +import { + PrefetchKind, + type ReadonlyReducerState, + type ReducerState, + type ServerActionAction, + type ServerActionMutable, } from '../router-reducer-types' import { addBasePath } from '../../../add-base-path' import { createHrefFromUrl } from '../create-href-from-url' @@ -43,6 +44,10 @@ import { normalizeFlightData, type NormalizedFlightData, } from '../../../flight-data-helpers' +import { getRedirectError, RedirectType } from '../../redirect' +import { createSeededPrefetchCacheEntry } from '../prefetch-cache-utils' +import { removeBasePath } from '../../../remove-base-path' +import { hasBasePath } from '../../../has-base-path' type FetchServerActionResult = { redirectLocation: URL | undefined @@ -221,7 +226,41 @@ export function serverActionReducer( if (redirectLocation) { const newHref = createHrefFromUrl(redirectLocation, false) - mutable.canonicalUrl = newHref + + // Because the RedirectBoundary will trigger a navigation, we need to seed the prefetch cache + // with the FlightData that we got from the server action for the target page, so that it's + // available when the page is navigated to and doesn't need to be re-fetched. + createSeededPrefetchCacheEntry({ + url: redirectLocation, + data: { + flightData, + canonicalUrl: undefined, + couldBeIntercepted: false, + isPrerender: false, + postponed: false, + }, + tree: state.tree, + prefetchCache: state.prefetchCache, + nextUrl: state.nextUrl, + kind: PrefetchKind.FULL, + }) + + mutable.prefetchCache = state.prefetchCache + // If the action triggered a redirect, we reject the action promise with a redirect + // so that it's handled by RedirectBoundary as we won't have a valid + // action result to resolve the promise with. This will effectively reset the state of + // the component that called the action as the error boundary will remount the tree. + // The status code doesn't matter here as the action handler will have already sent + // a response with the correct status code. + + reject( + getRedirectError( + hasBasePath(newHref) ? removeBasePath(newHref) : newHref, + RedirectType.push + ) + ) + + return handleMutable(state, mutable) } for (const normalizedFlightData of flightData) { diff --git a/packages/next/src/shared/lib/router/action-queue.ts b/packages/next/src/shared/lib/router/action-queue.ts index c970d1634cafba..4134802a893fd9 100644 --- a/packages/next/src/shared/lib/router/action-queue.ts +++ b/packages/next/src/shared/lib/router/action-queue.ts @@ -3,8 +3,6 @@ import { type AppRouterState, type ReducerActions, type ReducerState, - ACTION_REFRESH, - ACTION_SERVER_ACTION, ACTION_NAVIGATE, ACTION_RESTORE, } from '../../../client/components/router-reducer/router-reducer-types' @@ -20,7 +18,6 @@ export type AppRouterActionQueue = { dispatch: (payload: ReducerActions, setState: DispatchStatePromise) => void action: (state: AppRouterState, action: ReducerActions) => ReducerState pending: ActionQueueNode | null - needsRefresh?: boolean last: ActionQueueNode | null } @@ -45,18 +42,6 @@ function runRemainingActions( action: actionQueue.pending, setState, }) - } else { - // No more actions are pending, check if a refresh is needed - if (actionQueue.needsRefresh) { - actionQueue.needsRefresh = false - actionQueue.dispatch( - { - type: ACTION_REFRESH, - origin: window.location.origin, - }, - setState - ) - } } } } @@ -160,11 +145,6 @@ function dispatchAction( // Mark this action as the last in the queue actionQueue.last = newAction - // if the pending action was a server action, mark the queue as needing a refresh once events are processed - if (actionQueue.pending.payload.type === ACTION_SERVER_ACTION) { - actionQueue.needsRefresh = true - } - runAction({ actionQueue, action: newAction, diff --git a/test/e2e/app-dir/actions/app-action.test.ts b/test/e2e/app-dir/actions/app-action.test.ts index 376e10fab86ed5..4fa01ffbaa10c8 100644 --- a/test/e2e/app-dir/actions/app-action.test.ts +++ b/test/e2e/app-dir/actions/app-action.test.ts @@ -564,6 +564,64 @@ describe('app-dir action handling', () => { await check(() => browser.elementByCss('h1').text(), 'Transition is: idle') }) + it('should reset the form state when the action redirects to a page that contains the same form', async () => { + const browser = await next.browser('/redirect') + const input = await browser.elementByCss('input[name="name"]') + const submit = await browser.elementByCss('button') + + expect(await browser.hasElementByCssSelector('#error')).toBe(false) + + await input.fill('foo') + await submit.click() + + // The server action will fail validation and will return error state + // verify that the error state is displayed + await retry(async () => { + expect(await browser.hasElementByCssSelector('#error')).toBe(true) + expect(await browser.elementByCss('#error').text()).toBe( + "Only 'justputit' is accepted." + ) + }) + + // The server action won't return an error state, it will just call redirect to itself + // Validate that the form state is reset + await input.fill('justputit') + await submit.click() + + await retry(async () => { + expect(await browser.hasElementByCssSelector('#error')).toBe(false) + }) + }) + + it('should reset the form state when the action redirects to itself', async () => { + const browser = await next.browser('/self-redirect') + const input = await browser.elementByCss('input[name="name"]') + const submit = await browser.elementByCss('button') + + expect(await browser.hasElementByCssSelector('#error')).toBe(false) + + await input.fill('foo') + await submit.click() + + // The server action will fail validation and will return error state + // verify that the error state is displayed + await retry(async () => { + expect(await browser.hasElementByCssSelector('#error')).toBe(true) + expect(await browser.elementByCss('#error').text()).toBe( + "Only 'justputit' is accepted." + ) + }) + + // The server action won't return an error state, it will just call redirect to itself + // Validate that the form state is reset + await input.fill('justputit') + await submit.click() + + await retry(async () => { + expect(await browser.hasElementByCssSelector('#error')).toBe(false) + }) + }) + // This is disabled when deployed because the 404 page will be served as a static route // which will not support POST requests, and will return a 405 instead. if (!isNextDeploy) { diff --git a/test/e2e/app-dir/actions/app/redirect/actions.ts b/test/e2e/app-dir/actions/app/redirect/actions.ts new file mode 100644 index 00000000000000..f47a8d45da1ac3 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/actions.ts @@ -0,0 +1,17 @@ +'use server' + +import { redirect } from 'next/navigation' + +type State = { + errors: Record +} + +export async function action(previousState: State, formData: FormData) { + const name = formData.get('name') + + if (name !== 'justputit') { + return { errors: { name: "Only 'justputit' is accepted." } } + } + + redirect('/redirect/other') +} diff --git a/test/e2e/app-dir/actions/app/redirect/layout.tsx b/test/e2e/app-dir/actions/app/redirect/layout.tsx new file mode 100644 index 00000000000000..ad419941eebc7b --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/layout.tsx @@ -0,0 +1,21 @@ +'use client' + +import React, { useActionState } from 'react' +import { action } from './actions' + +export default function Page({ children }) { + const [{ errors }, dispatch] = useActionState(action, { + errors: { name: '' }, + }) + + return ( +
+
+ + + {errors.name &&

{errors.name}

} +
+ {children} +
+ ) +} diff --git a/test/e2e/app-dir/actions/app/redirect/other/page.tsx b/test/e2e/app-dir/actions/app/redirect/other/page.tsx new file mode 100644 index 00000000000000..187b8651110662 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/other/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Other Page
+} diff --git a/test/e2e/app-dir/actions/app/redirect/page.tsx b/test/e2e/app-dir/actions/app/redirect/page.tsx new file mode 100644 index 00000000000000..b2bb7da42de654 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Main Page
+} diff --git a/test/e2e/app-dir/actions/app/self-redirect/actions.ts b/test/e2e/app-dir/actions/app/self-redirect/actions.ts new file mode 100644 index 00000000000000..0ea200e2570771 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/actions.ts @@ -0,0 +1,17 @@ +'use server' + +import { redirect } from 'next/navigation' + +type State = { + errors: Record +} + +export async function action(previousState: State, formData: FormData) { + const name = formData.get('name') + + if (name !== 'justputit') { + return { errors: { name: "Only 'justputit' is accepted." } } + } + + redirect('/self-redirect') +} diff --git a/test/e2e/app-dir/actions/app/self-redirect/layout.tsx b/test/e2e/app-dir/actions/app/self-redirect/layout.tsx new file mode 100644 index 00000000000000..ad419941eebc7b --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/layout.tsx @@ -0,0 +1,21 @@ +'use client' + +import React, { useActionState } from 'react' +import { action } from './actions' + +export default function Page({ children }) { + const [{ errors }, dispatch] = useActionState(action, { + errors: { name: '' }, + }) + + return ( +
+
+ + + {errors.name &&

{errors.name}

} +
+ {children} +
+ ) +} diff --git a/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx b/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx new file mode 100644 index 00000000000000..187b8651110662 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Other Page
+} diff --git a/test/e2e/app-dir/actions/app/self-redirect/page.tsx b/test/e2e/app-dir/actions/app/self-redirect/page.tsx new file mode 100644 index 00000000000000..b2bb7da42de654 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Main Page
+}