-
Notifications
You must be signed in to change notification settings - Fork 293
fix: server action redirects use soft RSC navigation instead of hard reload (#654) #698
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
de4bf6f
7a94313
ed29480
c714eb3
c16cae3
0fb28ce
7a249da
28750cd
0730add
f14713c
de691d7
6953ee9
3de9712
c81c2f5
a932a5e
b532645
c7c5bca
7cd9bac
6ce2636
612720f
29dde11
43b446a
b5e38fa
db1c8ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1877,6 +1877,10 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||||||||||||||||||||||||||||||
| // We can't use a real HTTP redirect (the fetch would follow it automatically | ||||||||||||||||||||||||||||||||||||||
| // and receive a page HTML instead of RSC stream). Instead, we return a 200 | ||||||||||||||||||||||||||||||||||||||
| // with x-action-redirect header that the client entry detects and handles. | ||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||
| // For same-origin routes, we pre-render the redirect target's RSC payload | ||||||||||||||||||||||||||||||||||||||
| // so the client can perform a soft RSC navigation (SPA-style) instead of | ||||||||||||||||||||||||||||||||||||||
| // a hard page reload. This matches Next.js behavior. | ||||||||||||||||||||||||||||||||||||||
| if (actionRedirect) { | ||||||||||||||||||||||||||||||||||||||
| const actionPendingCookies = getAndClearPendingCookies(); | ||||||||||||||||||||||||||||||||||||||
| const actionDraftCookie = getDraftModeCookieHeader(); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -1893,8 +1897,71 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||||||||||||||||||||||||||||||
| redirectHeaders.append("Set-Cookie", cookie); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (actionDraftCookie) redirectHeaders.append("Set-Cookie", actionDraftCookie); | ||||||||||||||||||||||||||||||||||||||
| // Send an empty RSC-like body (client will navigate instead of parsing) | ||||||||||||||||||||||||||||||||||||||
| return new Response("", { status: 200, headers: redirectHeaders }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Try to pre-render the redirect target for soft RSC navigation. | ||||||||||||||||||||||||||||||||||||||
| // This is the Next.js parity fix for issue #654. | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| const redirectUrl = new URL(actionRedirect.url, request.url); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Only pre-render same-origin URLs. External URLs fall through to | ||||||||||||||||||||||||||||||||||||||
| // the empty-body response, which triggers a hard redirect on the client. | ||||||||||||||||||||||||||||||||||||||
| if (redirectUrl.origin === new URL(request.url).origin) { | ||||||||||||||||||||||||||||||||||||||
| const redirectMatch = matchRoute(redirectUrl.pathname); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (redirectMatch) { | ||||||||||||||||||||||||||||||||||||||
| const { route: redirectRoute, params: redirectParams } = redirectMatch; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Set navigation context for the redirect target | ||||||||||||||||||||||||||||||||||||||
| setNavigationContext({ | ||||||||||||||||||||||||||||||||||||||
| pathname: redirectUrl.pathname, | ||||||||||||||||||||||||||||||||||||||
| searchParams: redirectUrl.searchParams, | ||||||||||||||||||||||||||||||||||||||
| params: redirectParams, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1946
to
+1951
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this This is the same pattern as the normal RSC render path (line 1995-1999), so it's not a regression — just worth noting as a pre-existing concern with streaming responses. |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Build and render the redirect target page | ||||||||||||||||||||||||||||||||||||||
| const redirectElement = buildPageElement( | ||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider extracting the redirect pre-render logic (lines 1918-2052) into a runtime helper in
This would also reduce the 4x snapshot duplication (~750 new lines in snapshots are this same block repeated for each test variant). |
||||||||||||||||||||||||||||||||||||||
| redirectRoute, | ||||||||||||||||||||||||||||||||||||||
| redirectParams, | ||||||||||||||||||||||||||||||||||||||
| undefined, | ||||||||||||||||||||||||||||||||||||||
| redirectUrl.searchParams, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const redirectOnError = createRscOnErrorHandler( | ||||||||||||||||||||||||||||||||||||||
| request, | ||||||||||||||||||||||||||||||||||||||
| redirectUrl.pathname, | ||||||||||||||||||||||||||||||||||||||
| redirectRoute.pattern, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const rscStream = renderToReadableStream( | ||||||||||||||||||||||||||||||||||||||
| { root: redirectElement, returnValue }, | ||||||||||||||||||||||||||||||||||||||
| { temporaryReferences, onError: redirectOnError }, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const redirectResponse = new Response(rscStream, { | ||||||||||||||||||||||||||||||||||||||
| status: 200, | ||||||||||||||||||||||||||||||||||||||
| headers: redirectHeaders, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Append cookies to the response | ||||||||||||||||||||||||||||||||||||||
| if (actionPendingCookies.length > 0 || actionDraftCookie) { | ||||||||||||||||||||||||||||||||||||||
| for (const cookie of actionPendingCookies) { | ||||||||||||||||||||||||||||||||||||||
| redirectResponse.headers.append("Set-Cookie", cookie); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (actionDraftCookie) redirectResponse.headers.append("Set-Cookie", actionDraftCookie); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| // Append cookies to the response | |
| if (actionPendingCookies.length > 0 || actionDraftCookie) { | |
| for (const cookie of actionPendingCookies) { | |
| redirectResponse.headers.append("Set-Cookie", cookie); | |
| } | |
| if (actionDraftCookie) redirectResponse.headers.append("Set-Cookie", actionDraftCookie); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug (from previous review, still present): actionPendingCookies are collected at line 1889 but never appended here. Only redirectPendingCookies are included. This drops cookies set by the action before redirect() was called (e.g., cookies().set('session', token)).
Both sets need to be included:
| } | |
| if (actionPendingCookies.length > 0 || actionDraftCookie || redirectPendingCookies.length > 0 || redirectDraftCookie) { | |
| for (const cookie of actionPendingCookies) { | |
| redirectResponse.headers.append("Set-Cookie", cookie); | |
| } | |
| if (actionDraftCookie) redirectResponse.headers.append("Set-Cookie", actionDraftCookie); | |
| for (const cookie of redirectPendingCookies) { | |
| redirectResponse.headers.append("Set-Cookie", cookie); | |
| } | |
| if (redirectDraftCookie) redirectResponse.headers.append("Set-Cookie", redirectDraftCookie); | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| import { glob } from "node:fs/promises"; | ||
| import { readdir } from "node:fs/promises"; | ||
| import { join } from "node:path"; | ||
| import type { Dirent } from "node:fs"; | ||
|
|
||
| export const DEFAULT_PAGE_EXTENSIONS = ["tsx", "ts", "jsx", "js"] as const; | ||
|
|
||
|
|
@@ -85,19 +87,80 @@ export function createValidFileMatcher( | |
| } | ||
|
|
||
| /** | ||
| * Use function-form exclude for Node < 22.14 compatibility. | ||
| * Use function-form exclude for Node 22.14+ compatibility. | ||
| * Scans for files matching stem with extensions recursively under cwd. | ||
| * Supports glob patterns in stem. | ||
| */ | ||
| export async function* scanWithExtensions( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a complete rewrite of The original used Node's built-in
If the goal is Node < 22.14 compatibility, that's worth doing — but as a focused, well-tested change.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still an unrelated change bundled into a feature PR. The rewrite replaces a 6-line Additional issues beyond what the previous review noted:
This doesn't affect the current callers (all use Please move this to a separate PR with dedicated tests. |
||
| stem: string, | ||
| cwd: string, | ||
| extensions: readonly string[], | ||
| exclude?: (name: string) => boolean, | ||
| ): AsyncGenerator<string> { | ||
| const pattern = buildExtensionGlob(stem, extensions); | ||
| for await (const file of glob(pattern, { | ||
| cwd, | ||
| ...(exclude ? { exclude } : {}), | ||
| })) { | ||
| yield file; | ||
| const dir = cwd; | ||
|
|
||
| // Check if stem contains glob patterns | ||
| const isGlob = stem.includes("**") || stem.includes("*"); | ||
|
|
||
| // Extract the base name from stem (e.g., "**/page" -> "page", "page" -> "page") | ||
| // For "**/*", baseName will be "*" which means match all files | ||
| const baseName = stem.split("/").pop() || stem; | ||
| const matchAllFiles = baseName === "*"; | ||
|
|
||
| async function* scanDir(currentDir: string, relativeBase: string): AsyncGenerator<string> { | ||
| let entries: Dirent[]; | ||
| try { | ||
| entries = (await readdir(currentDir, { withFileTypes: true })) as Dirent[]; | ||
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| for (const entry of entries) { | ||
| if (exclude && exclude(entry.name)) continue; | ||
| if (entry.name.startsWith(".")) continue; | ||
|
|
||
| const fullPath = join(currentDir, entry.name); | ||
| const relativePath = fullPath.startsWith(dir) ? fullPath.slice(dir.length + 1) : fullPath; | ||
|
|
||
| if (entry.isDirectory()) { | ||
| // Recurse into subdirectories | ||
| yield* scanDir(fullPath, relativePath); | ||
| } else if (entry.isFile()) { | ||
| if (matchAllFiles) { | ||
| // For "**/*" pattern, match any file with the given extensions | ||
| for (const ext of extensions) { | ||
| if (entry.name.endsWith(`.${ext}`)) { | ||
| yield relativePath; | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| // Check if file matches baseName.{extension} | ||
| for (const ext of extensions) { | ||
| const expectedName = `${baseName}.${ext}`; | ||
| if (entry.name === expectedName) { | ||
| // For glob patterns like **/page, match any path ending with page.tsx | ||
| if (isGlob) { | ||
| if (relativePath.endsWith(`${baseName}.${ext}`)) { | ||
| yield relativePath; | ||
| } | ||
| } else { | ||
| // For non-glob stems, the path should start with the stem | ||
| if ( | ||
| relativePath === `${relativeBase}.${ext}` || | ||
| relativePath.startsWith(`${relativeBase}/`) || | ||
| relativePath === `${baseName}.${ext}` | ||
| ) { | ||
| yield relativePath; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| yield* scanDir(dir, stem); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| /// <reference types="vite/client" /> | ||
|
|
||
| import type { ReactNode } from "react"; | ||
| import { startTransition } from "react"; | ||
| import type { Root } from "react-dom/client"; | ||
| import { | ||
| createFromFetch, | ||
|
|
@@ -144,11 +145,52 @@ function registerServerActionCallback(): void { | |
| // Fall through to hard redirect below if URL parsing fails. | ||
| } | ||
|
|
||
| // Use hard redirect for all action redirects because vinext's server | ||
| // currently returns an empty body for redirect responses. RSC navigation | ||
| // requires a valid RSC payload. This is a known parity gap with Next.js, | ||
| // which pre-renders the redirect target's RSC payload. | ||
| // Check if the server pre-rendered the redirect target's RSC payload. | ||
| // If so, we can perform a soft RSC navigation (SPA-style) instead of | ||
| // a hard page reload. This is the fix for issue #654. | ||
| const contentType = fetchResponse.headers.get("content-type") ?? ""; | ||
| const hasRscPayload = contentType.includes("text/x-component"); | ||
|
||
| const redirectType = fetchResponse.headers.get("x-action-redirect-type") ?? "replace"; | ||
|
|
||
| if (hasRscPayload && fetchResponse.body) { | ||
| // Server pre-rendered the redirect target — apply it as a soft SPA navigation. | ||
| // This matches how Next.js handles action redirects internally. | ||
| try { | ||
| const result = await createFromFetch(Promise.resolve(fetchResponse), { | ||
| temporaryReferences, | ||
| }); | ||
|
|
||
| if (isServerActionResult(result)) { | ||
| // Update the React tree with the redirect target's RSC payload | ||
| startTransition(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing code at lines 611-618 documents this exact scenario:
This new code triggers a URL change via RSC payload but bypasses For this PR this is acceptable (the full tree is replaced so most components remount), but consider leaving a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing code at lines 611-618 documents that server actions don't trigger URL changes via RSC payload. This PR introduces exactly that behavior. The comment has been partially updated (lines 611-614 now mention the redirect path), but line 617-618 still says:
This is accurate for the non-redirect path, but it's worth adding a note that the redirect path above does use |
||
| getReactRoot().render(result.root); | ||
| }); | ||
|
|
||
| // Update the browser URL without a reload | ||
| if (redirectType === "push") { | ||
| window.history.pushState(null, "", actionRedirect); | ||
| } else { | ||
| window.history.replaceState(null, "", actionRedirect); | ||
| } | ||
|
||
|
|
||
| // Handle return value if present | ||
| if (result.returnValue) { | ||
| if (!result.returnValue.ok) throw result.returnValue.data; | ||
| return result.returnValue.data; | ||
| } | ||
| return undefined; | ||
| } | ||
| } catch (rscParseErr) { | ||
| // RSC parse failed — fall through to hard redirect below. | ||
| console.error( | ||
| "[vinext] RSC navigation failed, falling back to hard redirect:", | ||
| rscParseErr, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Fallback: empty body (external URL, unmatched route, or parse error). | ||
| // Use hard redirect to ensure the navigation still completes. | ||
| if (redirectType === "push") { | ||
| window.location.assign(actionRedirect); | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: navigation context is not cleaned up on failure. If the
tryblock throws after this point,setNavigationContextis left pointing at the redirect target. Thecatchblock at line 1956 doesn't reset it.This should be wrapped in a try/finally, or the catch block should call
setNavigationContext(null).