fix: server action redirects use soft RSC navigation instead of hard reload (#654)#698
Conversation
…reload (cloudflare#654) - Pre-render redirect target's RSC payload in app-rsc-entry.ts - Client detects RSC payload and performs soft navigation in app-browser-entry.ts - Falls back to hard redirect for external URLs or pre-render failures - Add E2E test verifying no hard navigation events on same-origin redirects This fixes the parity gap where server action redirects caused full page reloads instead of SPA-style soft navigation like Next.js does. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The glob function from node:fs/promises is only available in Node.js 22.14+. This replaces it with a manual recursive directory scan that supports glob patterns like **/page for matching files at any directory depth. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Update client-side to properly detect RSC payload via content-type header - Fix test to correctly detect soft vs hard navigation using page load events - All 11 server actions tests now pass This completes the fix for issue cloudflare#654. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
commit: |
- Handle **/* pattern for matching all files with given extensions - Handle **/page pattern for matching specific files at any depth - Properly exclude api directories and _ prefixed files - Update entry-templates snapshots to reflect soft navigation changes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@southpolesteve @james-elicx review the PR. |
|
You don't need to tag us to ask for a review - we'll take a look when we get a chance :) |
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for working on this — the feature direction is right (pre-rendering the redirect target for soft RSC navigation is how Next.js does it). However, there are several correctness issues that need to be fixed before this can be merged.
Critical bugs
-
Duplicate Set-Cookie headers — cookies are appended to
redirectHeadersat lines 1896-1899, then the same cookies are appended again to theredirectResponseat lines 1946-1951. SinceredirectResponsewas constructed withredirectHeaders(which already contain the cookies), every cookie will appear twice in the response. This will cause duplicate cookie writes on the client. -
Missing
setNavigationContext(null)cleanup — the navigation context is set for the redirect target at line 1915 but never cleaned up. If pre-rendering fails (catch block at 1956) or the fallback path is taken, the navigation context is left pointing at the redirect target instead of being reset. The original code callssetNavigationContext(null)at line 1888 and the new code overwrites it at 1915 without restoring it on failure. -
Client-side
setNavigationContext/setClientParamsnot called — whennavigateRsc()does a normal soft navigation (lines 285-298 inapp-browser-entry.ts), it callssetClientParams()to update the client-side navigation shims. The new soft-redirect code path (lines 155-190) skips this entirely. After a server action redirect,usePathname(),useSearchParams(), anduseParams()will return stale values from the previous page, not the redirect target.
Significant concerns
-
Middleware is bypassed for the redirect target — the pre-render calls
matchRoute()+buildPageElement()directly, skipping the entire middleware pipeline. If a user's middleware sets auth headers, rewrites the path, or injects cookies for the target route, none of that will happen. Next.js does run middleware for the redirect target. This is a correctness gap that should at minimum be documented as a known limitation. -
file-matcher.tsrewrite is unrelated and risky — replacingnode:fs/promisesglob()with a hand-rolled recursivereaddirimplementation is a large behavioral change bundled into a feature PR. The original code used Node's built-inglob()(which handles brace expansion,**, and edge cases correctly). The replacement has subtle issues:- The
excludecallback receivesentry.name(just the filename), but the originalglobAPI's function-form exclude receives the full relative path. Callers pass(name) => name === "api"which happens to work for directory names but would break for file-name exclusions at nested paths. - Non-glob stems (e.g., a literal path like
pages/index) go through a convoluted matching path withrelativeBasethat may not match correctly whenstemcontains path separators. - The
isGlobcheck only looks for*and**, missing?,[...], and{...}patterns thatbuildExtensionGlobgenerates. - There are no new tests for
scanWithExtensionsdespite rewriting its internals completely.
This should either be a separate PR with dedicated tests, or the Node.js version requirement should be documented instead.
- The
Minor issues
-
Test is in the wrong
describeblock — the new soft-navigation test is insidetest.describe("useActionState")but it tests basic server action redirect behavior, notuseActionState. It belongs in the"Server Actions"describe block. -
content-typedetection is fragile — the client checkscontentType.includes("text/x-component")to decide if there's an RSC payload, but the fallback path also sendsContent-Type: text/x-component(set inredirectHeaders) with anullbody. The check works because of the&& fetchResponse.bodyguard, butnew Response(null)can still have a non-null.bodyin some environments. A more robust signal (e.g., a dedicated header likex-action-rsc-prerender: 1) would be safer.
| // 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.
Bug: duplicate Set-Cookie headers. redirectHeaders already has the cookies appended at lines 1896-1899 above. Since redirectResponse was constructed from redirectHeaders, appending again here doubles every cookie.
This entire block should be removed:
| // 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); | |
| } |
| // Set navigation context for the redirect target | ||
| setNavigationContext({ | ||
| pathname: redirectUrl.pathname, | ||
| searchParams: redirectUrl.searchParams, | ||
| params: redirectParams, | ||
| }); |
There was a problem hiding this comment.
Bug: navigation context is not cleaned up on failure. If the try block throws after this point, setNavigationContext is left pointing at the redirect target. The catch block at line 1956 doesn't reset it.
This should be wrapped in a try/finally, or the catch block should call setNavigationContext(null).
| if (isServerActionResult(result)) { | ||
| // Update the React tree with the redirect target's RSC payload | ||
| startTransition(() => { | ||
| 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); | ||
| } |
There was a problem hiding this comment.
Bug: client-side navigation context is not updated. After soft-navigating to the redirect target, usePathname(), useSearchParams(), and useParams() will return stale values from the previous page.
Compare with the navigateRsc function (lines 285-294) which calls setClientParams() after navigation. This code path needs equivalent updates:
// After startTransition + history update:
setNavigationContext({
pathname: new URL(actionRedirect, window.location.origin).pathname,
searchParams: new URL(actionRedirect, window.location.origin).searchParams,
params: {}, // or parse from X-Vinext-Params header
});| * Scans for files matching stem with extensions recursively under cwd. | ||
| * Supports glob patterns in stem. | ||
| */ | ||
| export async function* scanWithExtensions( |
There was a problem hiding this comment.
This is a complete rewrite of scanWithExtensions from a 6-line glob() wrapper to a 70-line hand-rolled recursive directory walker. This is unrelated to the server action redirect feature and should be in a separate PR with dedicated unit tests.
The original used Node's built-in glob() which correctly handles brace expansion, ** recursion, and other edge cases. The replacement has several issues:
excludereceivesentry.name(bare filename) instead of the relative path thatglob's function-form exclude receivesisGlobonly checks for*/**, missing?,[...],{...}whichbuildExtensionGlobgenerates- No tests were added for the new implementation
If the goal is Node < 22.14 compatibility, that's worth doing — but as a focused, well-tested change.
| await expect(page.locator("h1")).toHaveText("useActionState Test"); | ||
| }); | ||
|
|
||
| test("server action redirect performs soft RSC navigation (issue #654)", async ({ page }) => { |
There was a problem hiding this comment.
Nit: this test is inside test.describe("useActionState") but it doesn't test useActionState. It should be in the "Server Actions" describe block above (after line 122).
| // 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"); |
There was a problem hiding this comment.
The content-type check is fragile: the fallback Response(null) path also sends Content-Type: text/x-component because it uses the same redirectHeaders. The && fetchResponse.body guard saves you today, but new Response(null).body is non-null in some environments (it's an empty ReadableStream).
Consider adding an explicit signal header on the server side (e.g., x-action-rsc-prerender: 1) to distinguish pre-rendered responses from empty fallbacks, rather than relying on content-type + body presence.
|
Review posted. Here's a summary of the findings: 3 critical bugs:
2 significant concerns: 2 minor issues: |
- Fix duplicate Set-Cookie headers (collect cookies after rendering, not before) - Add setNavigationContext(null) cleanup on pre-render failure and fallback - Update client-side navigation context (setNavigationContext/setClientParams) so usePathname(), useSearchParams(), useParams() return correct values - Add x-action-rsc-prerender header for robust RSC payload detection - Document middleware bypass as known limitation in code comment - Move soft navigation test to correct describe block (Server Actions) - Remove file-matcher.ts changes (will be separate PR) Fixes review comments from ask-bonk on PR cloudflare#654 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Fixes #654
What changed
This PR fixes the RSC parity gap where server action redirects caused full page reloads instead of SPA-style soft navigation like Next.js does.
Server-side (app-rsc-entry.ts)
Client-side (app-browser-entry.ts)
Tests
Additional fix
Testing