Open in StackBlitz

npm i https://pkg.pr.new/vinext@1582

commit: 4f4bbbd

@codex review

/bigbonk review

Review submitted. Here's a summary of the feedback I posted:

Overall assessment: Strong PR with good design, thorough test coverage, and clear upstream references. The core approach of rendering redirect targets inline during the action POST is sound.

Key feedback points:

1. Cookie value round-trip corruption risk (app-server-action-execution.ts:312) — The decodeURIComponent/encodeURIComponent round-trip in applySetCookieMutationsToRequestCookieHeader can corrupt cookie values containing raw % characters that aren't valid percent-escape sequences. Low practical risk since vinext's own cookies().set() uses standard encoding, but worth acknowledging.

2. Unexplained args.length branching (app-browser-entry.ts:1331) — The distinction between programmatic actions (throw redirect error) and form actions (suspend with pending promise) is subtle and critical. Suggested adding a comment explaining the React-specific reasoning.

3. Subtle actionStatus === 200 guard change (app-server-action-execution.ts:998) — The skip-rerender condition now requires actionStatus === 200 in addition to no-revalidation. This is likely correct (HTTP fallback actions need the rerender for boundary display), but it's an undocumented behavioral change.

4. ACTION_FORWARDED_HEADER guard removal from progressive path (app-server-action-execution.ts:646) — Previously rejected forwarded progressive actions; now processes them fully. The header isn't referenced in the progressive path after removal.

5. Minor items: indexOf redundancy in rootRouteBoundaryPath, legacy cookie date format edge case, suggestion comments on createPendingServerActionRedirectPromise and redirect header initialization ordering.

6. Positive callouts: Good response cloning before error detection, nice createServerActionRscResponse pattern for deferred context cleanup.

github run

Addressed the review feedback from @ask-bonk:

1. Cookie value round-trip corruption — Removed the unnecessary encodeURIComponent call in applySetCookieMutationsToRequestCookieHeader. Cookie header values are raw, not URL-encoded, and readSetCookieNameValue already decodes from Set-Cookie format. The encode/decode round-trip is now eliminated.

2. args.length branching — Added a comment documenting the React-specific distinction: programmatic actions (no args) throw a redirect error, while form actions (args present) suspend via a pending promise.

3. actionStatus === 200 guard — Added a comment explaining that non-200 HTTP fallback statuses (e.g., 404 from notFound()) need the page rerender to display error boundaries, so only 200 status actions skip the rerender.

4. ACTION_FORWARDED_HEADER in progressive path — Added a comment documenting that progressive form submissions (multipart without actionId) don't carry a forwarded-action header. They route to the visible page directly and can't be redirected cross-runtime.

5. Minor items:

   • Replaced indexOf with findIndex in rootRouteBoundaryPath for clarity
   • Relaxed expired-cookie date regex to accept Thu, 1 Jan 1970 (without leading zero)
   • Added explanatory comment to createPendingServerActionRedirectPromise
   • Removed unused navigateClientSide import in app-browser-entry.ts
   • Updated edge-runtime redirect test status expectation to 303 (same-runtime, non-forwarded)

All 262 targeted tests pass cleanly. Rebased on current upstream/main.

/bigbonk review

Review submitted. Here's a summary of the feedback I posted:

Overall assessment: Well-designed PR that closes a meaningful Next.js parity gap. The core approach of rendering redirect targets inline during the action POST and using wrapper status semantics is sound, well-referenced to upstream Next.js, and has strong test coverage.

Key feedback:

1. Implicit Node → Edge runtime comparison is still broken (app-server-action-execution.ts:568) — The author claimed to have fixed null runtime normalization per the previous review, but the fix doesn't appear in the final code. When currentRuntime is null (implicit Node, no runtime export) and targetRuntime is "edge", shouldUseForwardedActionRedirectStatus returns false instead of true. The existing cross-runtime test only covers Edge → Node (where currentRuntime="edge" is not null). Suggested normalizing null to "nodejs" before comparing and adding a unit test for the reverse direction.

2. Comment/code mismatch on args.length branching (app-browser-entry.ts:1338) — The comment says "programmatic actions (args.length === 0) throw" and "form actions (args.length !== 0) suspend", but the code does the opposite: if (args.length !== 0) throws. The parenthetical mapping in the comment is swapped.

3. Duplicate runtime assignment (app-segment-config.ts:113) — config.runtime is set on both line 113 and line 163 within the same loop iteration, both guarded by isRouteSegmentRuntime. The second always overwrites the first with the same value.

4. rootRouteBoundaryPath findIndex always returns 0 — rootPosition is layoutTreePositions[0], so findIndex always finds it at index 0. Not blocking, just noting.

5. Minor cookie value encoding asymmetry — readSetCookieNameValue decodes values while original Cookie header values stay raw, creating a subtle mixed-encoding state in the redirect render request.

6. Positive callouts — Good stale meta clearing in syncServerActionHttpFallbackHead, nice createServerActionRscResponse pattern for deferred context cleanup.

github run

/bigbonk review

Review submitted. Here's a summary of the feedback I posted:

Overall assessment: Well-executed PR that closes a meaningful Next.js parity gap. The core approach of rendering redirect targets inline during the action POST, wrapper status semantics for forwarded/stale/cross-runtime cases, and deferred context cleanup via createServerActionRscResponse are all sound. Previous review feedback has been addressed — runtime normalization, duplicate segment config, and cookie encoding fixes are all confirmed in the final code.

Key feedback:

1. Stale comment on cookie value encoding (app-server-action-execution.ts:315-317) — After removing decodeURIComponent, the comment still says the value "has already been decoded from the Set-Cookie header's URL-encoded form." readSetCookieNameValue does no decoding — it slices verbatim. Suggested rewording.

2. readInvalidServerActionResponseError rejects all non-RSC responses (app-browser-action-result.ts:137) — Including 200 OK responses with non-RSC content types. This matches Next.js behavior but is a strict gate worth being aware of.

3. Forwarded action + notFound() skip-rerender interaction (app-server-action-execution.ts:1021) — A forwarded action that throws notFound() skips the page rerender (since actionWasForwarded takes priority). Confirmed this works because the client-side normalizeServerActionThrownValue converts the thrown value into a NEXT_HTTP_ERROR_FALLBACK digest that React's error boundary catches.

4. Positive callouts: Good defensive ordering in the browser entry (non-RSC redirect → hard nav check runs after invalid response error check), and well-designed deferred cleanup pattern where setHeadersContext for the redirect target survives until RSC streaming completes.

github run

/bigbonk review

Review submitted. Here's a summary of what I found:

Overall assessment: Approved. This is a well-executed PR that closes a meaningful Next.js parity gap. All feedback from three previous review cycles has been addressed and verified in the current code.

What the PR does:

• Renders same-origin action redirect targets inline during the action POST (instead of returning an empty redirect response)
• Uses wrapper status semantics (200 vs 303) for forwarded, stale child-route, and cross-runtime redirect cases
• Defers clearRequestContext until RSC stream consumption via createServerActionRscResponse
• Fixes the browser action dispatch to POST to the visible route URL and handle inline Flight redirect payloads

What I verified:

• All 7 previously flagged issues (runtime normalization, duplicate segment config, cookie encoding, args.length branching, stale comments, trailing semicolons, default redirect type) are confirmed fixed
• The isStaleChildSiblingRouteRedirect and shouldUseForwardedActionRedirectStatus logic is correct
• URL resolution is consistent across server and browser implementations
• Cookie mutation replay for redirect render requests is sound
• Test coverage is strong (455+ new unit test lines, 183 E2E lines, good Next.js test provenance)

One minor suggestion posted: Expanding the shouldSkipPageRendering comment to also mention the actionWasForwarded branch for completeness.

github run

/bigbonk review

Review submitted. Here's a summary of the feedback I posted:

Overall assessment: Approved. This is a well-executed PR that closes a meaningful Next.js parity gap for server-action redirect behavior. The core architectural shift — rendering the redirect target inline during the action POST instead of returning an empty redirect — is sound, well-referenced to upstream Next.js, and has thorough test coverage.

What I verified:

• Server-side redirect rendering pipeline with correct fallback paths (external URLs → 303, non-page routes → 303)
• Deferred clearRequestContext via deferUntilStreamConsumed prevents AsyncLocalStorage race
• Cookie replay, runtime normalization (null → "nodejs"), and forwarded action side-effect preservation
• Browser-side visible-route POST URL, RSC compatibility gating, 303→200 normalization, and ServerActionRedirectError integration
• Segment config runtime type narrowing, root boundary fallback for apps without root page
• 455+ new unit test lines and 183 new E2E lines with good provenance annotations

Two minor inline comments posted (neither blocking):

1. isStaleChildSiblingRouteRedirect — the depth inequality (currentSegments.length > targetSegments.length) means it only detects deeper-to-shallower sibling redirects, not same-depth siblings. This is likely intentional but worth a clarifying comment.
2. shouldSkipPageRendering comment — the existing comment documents the actionStatus === 200 guard but doesn't mention the actionWasForwarded branch that unconditionally skips regardless of status. Suggested expanding the comment to cover both branches.

github run

/bigbonk review

Review submitted. Here's a summary of what I posted:

Overall assessment: Well-designed PR that closes a meaningful Next.js parity gap. The core architecture — rendering redirect targets inline during the action POST and using wrapper status semantics — is sound. All prior review feedback (5 cycles) has been addressed. No blocking issues found.

What I verified:

• Redirect rendering pipeline with correct fallbacks to 303 for external/unmatched/route-handler targets
• Deferred clearRequestContext via deferUntilStreamConsumed with proper once() guard
• Cookie replay merging Set-Cookie mutations verbatim (decode/encode asymmetry fixed)
• Runtime normalization (null → "nodejs", "experimental-edge" → "edge") across all four wrapper status cases
• Browser-side visible-route POST URLs, next-action header, 303→200 rewrite, response cloning, and ServerActionRedirectError
• Manifest changes for root boundary discovery without a root page

Inline comments posted (6 total, none blocking):

1. Confirmed response clone control flow is safe in the browser action dispatch
2. Flagged that buildServerActionPageTags might want verification for intercepted routes' routeSegments
3. Suggested a clarifying comment on isStaleChildSiblingRouteRedirect's intentional depth inequality
4. Noted duplicated URL resolution logic between browser and server
5. Flagged potential E2E timing sensitivity in the stale child-route redirect test
6. Positive callout on the redirect render context setup and deferred cleanup

github run

All review feedback from @ask-bonk has been addressed. Summary of fixes across review cycles:

Code changes in this PR:

1. Cookie value encoding — Removed decodeURIComponent/encodeURIComponent round-trip in readSetCookieNameValue to prevent cookie value corruption. Values are now passed verbatim.
2. args.length branching — We always throw ServerActionRedirectError for both programmatic and form-based action redirects. SPA navigations run asynchronously via renderNavigationPayload; returning a pending promise would block state updates.
3. Skip-rerender condition — Added comment documenting that actionWasForwarded takes unconditional priority and that only actionStatus === 200 non-forwarded actions skip the page rerender (HTTP fallback actions like notFound() need the rerender for boundary display).
4. ACTION_FORWARDED_HEADER in progressive path — Progressive form submissions don't carry a forwarded-action header; they route to the visible page directly. This matches upstream Next.js behavior.
5. Runtime normalization — null route runtimes are normalized to "nodejs" before comparing, so implicit Node -> explicit Edge redirects correctly use the wrapper 200 response path.
6. Duplicate runtime assignment — Removed the duplicate config.runtime assignment from the end of the segment config reduction loop.
7. Expired cookie regex — Broadened isExpiredSetCookie regex to match both RFC 6265 space format and legacy RFC 2109 hyphen format (Thu, 01-Jan-1970).
8. Stale comment on cookie encoding — Updated comment to reflect that values are extracted verbatim without decoding.
9. Default redirect type — Changed from "replace" to "push" to match Next.js server-action-reducer.ts:370.
10. isStaleChildSiblingRouteRedirect depth guard — Added clarifying comment that currentSegments.length > targetSegments.length is intentional (same-depth siblings share the same page worker in the Next.js model, so only deeper-to-shallower redirects can be stale).
11. Scroll intent on action redirects — Captured beginAppRouterScrollIntent return value and passed it through renderNavigationPayload so scroll-to-top works correctly.
12. shouldSkipPageRendering comment — Expanded to document both the actionWasForwarded unconditional branch and the actionStatus === 200 guard.
13. E2E timing — Increased fixture delay from 1s to 3s to avoid race between client-side navigation and delayed action redirect in slow CI environments.

Observations acknowledged (no code change needed):

• Response clone control flow confirmed safe.
• readInvalidServerActionResponseError rejecting non-RSC responses matches Next.js behavior.
• Relative URL resolution including base search params is spec-correct.
• Duplicated URL resolution logic between browser and server noted for future refactoring.
• buildServerActionPageTags observation on intercepted routes noted (low risk, cache tags are advisory).

All 57 unit tests in app-server-action-execution.test.ts pass cleanly.

/bigbonk re-review for issues and parity

Review submitted. Here's a summary of the feedback I posted:

Overall assessment: Well-designed PR that closes a meaningful Next.js parity gap. The core architecture -- rendering redirect targets inline during the action POST, wrapper status semantics for forwarded/stale/cross-runtime cases, and deferred context cleanup via createServerActionRscResponse -- is sound. All prior review feedback (5 cycles) has been addressed.

Key feedback points:

1. Dead production code (app-browser-action-result.ts:106) -- resolveServerActionRedirectLocation is no longer called from production code paths after the switch to resolveActionRedirectTarget. Still tested and exported, but dead in production.

2. canRenderActionRedirectTarget defensive concern (app-server-action-execution.ts:640) -- When page is absent from the route object entirely (not null, just missing), the function falls through to return true, treating it as renderable. Not a current issue since generated routes always have page, but could surprise with manually constructed route objects.

3. E2E timing fragility (server-actions.spec.ts:246) -- The stale child-route test relies on a 3-second setTimeout delay that could be tight in slow CI with cold compilation. Suggested using page.waitForURL or DOM readiness signals instead of fixed delays.

4. Positive observations:

   • Clean partition between isAncestorRouteRedirect and isStaleChildSiblingRouteRedirect -- they cover complementary cases without overlap
   • Defensive syncServerActionHttpFallbackHead(null) at the top of the action callback correctly prevents stale noindex meta from persisting
   • The currentMatch ordering relative to context switches is safe because cleanPathname is a readonly option parameter
   • boundaryPaths?.[0] indexing in rootRouteBoundaryPath works correctly with current scanner ordering

github run