fix(app-router): align server action routing#2520
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Review: server action routing alignmentReviewed the full diff, ran the targeted suites ( A few observations, none blocking: 1. Action-owner injection is gated behind the classification stub filter (fragile coupling)In
The action-owner stub is only replaced if the chunk also contains The 2. Dev vs prod ownership divergence through node_modules
3.
|
|
Posted the review to PR #2520. Summary of what I found: Review outcomeThe PR is well-implemented and the security model is sound. I verified:
Issues raised (none blocking)
|
|
/bigbonk review for issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ${ | ||
| hasServerActions | ||
| ? `const __loadAppServerActionExecution = () => import(${JSON.stringify(appServerActionExecutionPath)});` | ||
| ? `const __loadAppServerActionExecution = () => import(${JSON.stringify(appServerActionExecutionPath)}); |
| hasServerActions | ||
| ? `const __loadAppServerActionExecution = () => import(${JSON.stringify(appServerActionExecutionPath)});` | ||
| ? `const __loadAppServerActionExecution = () => import(${JSON.stringify(appServerActionExecutionPath)}); | ||
| const __loadAppActionForwarding = () => import(${JSON.stringify(appActionForwardingPath)});` |
|
/bigbonk review for issues |
Follow-up review — action-owner injection robustness + build costRe-reviewed the diff, re-ran
Two things worth addressing, only the first is more than cosmetic: 1. Reinforce the
|
|
Posted the review to PR #2520. Review summaryI re-reviewed the full diff, re-ran the targeted suites (
Issues raised (none blocking)
This complements the prior |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: server action routing alignment (follow-up)
Re-reviewed the current head (f698182, which is 4 commits past the first ask-bonk pass) and re-ran the targeted suites — tests/app-action-forwarding.test.ts, tests/action-owner-manifest.test.ts, and the production-build tests/action-runtime-security.test.ts all pass (42 tests green).
The security model still holds up and two prior review items are now resolved:
x-action-forwardedcannot be spoofed — it is inINTERNAL_HEADERS(headers.ts:211), sofilterInternalHeadersstrips any client copy (app-rsc-handler.ts:1316) and it is re-injected only via the trustedctx.actionForwardedpath (app-rsc-handler.ts:1317-1319). The re-entry loop guard fails closed to{}(app-action-forwarding.ts:149-152).- Prior review #1 (fragile
__VINEXT_CLASScoupling) is fixed.renderChunknow gates the action-owner injection on its own independenthasActionOwnerStubcondition (index.ts:3664-3666) rather than piggybacking the classification pre-filter, andgenerateBundle(index.ts:3744-3750) now fails the build loudly if__VINEXT_ACTION_OWNERS_STUB__survives un-injected. This converts the prior silent-runtime-404 risk into a hard build error. - Prior review #5 (cookie deletion untested) is fixed —
tests/app-action-forwarding.test.ts:133-183now coversMax-Age=0,Max-Age=-1, and expired-Expiresdeletions.
1. CodeQL "improper code sanitization" — the manifest is embedded into generated JS with raw JSON.stringify (should use safeJsonStringify)
The two CodeQL alerts on app-rsc-entry.ts are legitimate and both trace back to embedding the action-owner manifest into generated JavaScript source without HTML/JS-safe escaping:
- Dev path:
app-rsc-entry.ts:708—function __VINEXT_ACTION_OWNERS() { return ${JSON.stringify(actionOwners)}; } - Prod path:
injectActionOwnerManifestataction-owner-manifest.ts:375-378—`function ${match[1]}() { return ${JSON.stringify(manifest)}; }`
The manifest keys and values are developer-controlled strings, but they are not constants: in dev, keys are filesystem-derived reference paths (referenceKey → /@fs/${id} or /${relative}, action-owner-manifest.ts:90-96) and values are route patterns derived from directory names. JSON.stringify does not escape <, >, &, U+2028, or U+2029 — exactly the characters that break JS/HTML embedding. This repo already has the correct helper for this: safeJsonStringify in server/html.ts, which is used everywhere else data is serialized into emitted JS (app-ssr-stream.ts, dev-server.ts, pages-server-entry.ts, etc.). The classification injector avoided the alert only because it emits an integer switch dispatch, not embedded strings.
Recommend swapping both sites to safeJsonStringify. It produces valid JSON that is also a valid JS literal, so runtime JSON.parse/object-literal semantics are unchanged, and it clears the CodeQL alerts. generateRscEntry can import it the same way the other entry templates do; injectActionOwnerManifest is in build/ and can import from ../server/html.js.
2. Forwarded dispatch drops the query string (forwardUrl.search = "")
forwardServerActionIfNeeded clears the search params on the forward (app-action-forwarding.ts:157). For the action invocation itself this is fine (the action is keyed by header, not query), but the owner page re-render triggered by the forward loses the original request's query string. If an action's owner page reads searchParams during the post-action render, it will see an empty set on the forwarded path but the real params on the non-forwarded (same-route) path — a dev/prod-adjacent behavior split worth a comment or a test. Worth confirming against Next.js' forwarded-action behavior before merge; if intentional, a one-line comment explaining why the query is dropped would prevent a future "bug fix" that re-adds it.
3. (Carry-over, non-blocking) Eager static manifest cost on every prod build
buildStaticActionOwnerManifest still runs unconditionally in the load hook (index.ts:3509-3519) and AST-parses every route-reachable module, even though in production the authoritative source is the Rollup module graph and staticOwners is only consulted for the node_modules re-export edge case (action-owner-manifest.ts:357). This lines up with the +11.8% production-build-time regression the benchmark bot flagged. Narrowing the static pass to only node_modules re-export chains (or gating it on their presence) would keep the common case off that cost. Tracked from the prior review; not a merge blocker.
Nice work on the injection-guard hardening and the thin-entry delegation. Only #1 is more than cosmetic, and it has a clean mechanical fix.
|
Review posted to PR #2520. SummaryI reviewed the current head ( Security model confirmed sound, and two prior review items are now resolved:
Issues raised (only #1 is more than cosmetic):
|
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: server action routing alignment (head 2dcf18a, 5 commits)
Re-reviewed the current head, which is one commit (2dcf18a "escape action owner metadata") past the prior ask-bonk pass. Re-ran the targeted suites and vp check:
tests/app-action-forwarding.test.ts+tests/action-owner-manifest.test.ts— 34 passtests/action-runtime-security.test.ts+tests/app-server-action-execution.test.ts+tests/entry-templates.test.ts— 140 passtests/app-browser-server-action-client.test.ts— 3 passvp checkon the changed source — clean
All substantive items from prior reviews are now resolved. The latest commit and the earlier hardening close the loop:
-
Prior #1 (fragile
__VINEXT_CLASScoupling) — fixed.renderChunknow gates action-owner injection on its own independenthasActionOwnerStubcondition (index.ts:3664-3666), andgenerateBundle(index.ts:3744-3750) fails the build loudly if__VINEXT_ACTION_OWNERS_STUB__survives un-injected. Converts the silent-runtime-404 risk into a hard build error. -
CodeQL "improper code sanitization" — fixed. Both injection sites now use
safeJsonStringifyinstead of rawJSON.stringify: the dev path (app-rsc-entry.ts:709) and the prod injector (action-owner-manifest.ts:378).safeJsonStringifyescapes<,>,&, U+2028, U+2029 (server/html.ts:21). Covered by a new test (action-owner-manifest.test.ts"escapes action owner manifests embedded in generated JavaScript"). -
Prior #5 (cookie deletion untested) — fixed. Tests now cover
Max-Age=0,Max-Age=-1, and expiredExpires(app-action-forwarding.test.ts:133-183). -
Prior #3 (ExecutionContext not forwarded) — now addressed. The forward dispatch threads the real CF context through (
app-rsc-entry.ts:1064-1076): when__getRequestExecutionContext()resolves, it passeswaitUntil/cache/passThroughOnExceptionalongsideactionForwarded: true, soisExecutionContextLike(app-rsc-handler.ts:349) recognizes it andafter()/waitUntilinside a forwarded action attach to the request lifetime. -
Prior #2 (dropped query string) — verified against Next.js and now correct + documented.
forwardServerActionIfNeededclearsforwardUrl.search(app-action-forwarding.ts:159), matching Next.jscreateForwardedActionResponse()which buildsnew URL(${origin}${basePath}${workerPathname})with no query. The concern about the owner page losingsearchParamson re-render is moot: on the forwarded pathshouldSkipPageRenderingis true for non-HTTP-fallback actions (app-server-action-execution.ts:1455-1457), so the page isn't re-rendered — only the actionreturnValueis streamed. This mirrors Next.jsskipPageRendering = actionWasForwarded. The added comment documents the parity intent.
Security model re-confirmed sound:
x-action-forwardedis unspoofable — it's inINTERNAL_HEADERS, sofilterInternalHeadersstrips any client copy beforehandleServerActionRequestsees the request (app-rsc-handler.ts:1316), and it is re-injected only via the trustedctx.actionForwardedpath (:1317-1318). The re-entry loop guard fails closed to{}(app-action-forwarding.ts:149-152).- The cookie-merge divergence from Next.js is deliberate and correct: vinext's
ResponseCookies.getAll()returns deletions asvalue: ""(notundefined), so it can't use Next.js'scookie.value === undefinedcheck. The customdeletesCookie()heuristic (Max-Age<=0/ expiredExpires) is self-consistent with vinext's owndelete()which setsexpires: new Date(0).
Remaining (non-blocking, carry-over)
Eager static manifest cost on every prod build — buildStaticActionOwnerManifest still runs unconditionally in the load hook and AST-parses every route-reachable module, even though in production the Rollup module graph is authoritative and staticOwners is only consulted for the node_modules re-export edge case (action-owner-manifest.ts:358). This lines up with the build-time regression the benchmark bot flagged (+10–12% across rounds). Narrowing the static pass to node_modules re-export chains (or gating it on their presence) would keep the common case off that cost. Not a merge blocker.
Nice work — the injection-guard hardening, the escaping fix, and the ExecutionContext threading all landed cleanly, and the thin-entry delegation is holding up.
|
Review posted to PR #2520. SummaryI reviewed the current head ( All substantive prior-review items are now resolved:
Security model re-confirmed sound: Remaining (non-blocking): the eager No blocking issues found. |
|
/bigbonk review for issues |
Review: server action routing alignment (head
|
|
Review posted to PR #2520. SummaryI reviewed the current head ( All substantive prior-review items remain resolved, and the security model is sound: New commit Non-blocking observations:
No blocking issues found. |
|
/bigbonk review for issues |
Review: server action routing alignment (head
|
|
Review posted to PR #2520. SummaryI reviewed the current head ( Verification:
New commit
Non-blocking observations:
No blocking issues found. |
…ime-takeover # Conflicts: # packages/vinext/src/index.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: server action routing alignment (head e3630a8, 13 commits)
Reviewed the current head, which is 5 commits past the last ask-bonk pass (ba44206). Those commits (6423c6b, c9e6041, 09feead, 13f9f0c, e3630a8) are a substantial rewrite of how the action-owner manifest is built — the prior dev-only static AST pass and the RSC-scan createActionOwnerModuleCollector are gone, replaced by a patched @vitejs/plugin-rsc that exposes scanBuildObservers and hands vinext the es-module-lexer output during the scan build. Because the whole manifest pipeline changed, this needed a fresh look rather than a delta on prior reviews.
Verification on this head:
vp install(applies the newpatches/@vitejs__plugin-rsc@0.5.27.patch; confirmedscanBuildObserverspresent innode_modules/@vitejs/plugin-rsc/dist/plugin-BK29Va7z.js)tests/action-owner-manifest.test.ts+tests/app-action-forwarding.test.ts+tests/action-runtime-security.test.ts+tests/app-server-action-execution.test.ts— 125 passtests/entry-templates.test.ts+tests/route-sorting.test.ts— 91 passvp checkon the changed source — clean
Security model still sound and prior items remain resolved: x-action-forwarded is stripped by filterInternalHeaders and re-injected only via the trusted ctx path; the loop guard fails closed to {} (app-action-forwarding.ts:149-152); safeJsonStringify is used at both injection sites; the ExecutionContext threading (app-rsc-entry.ts:1064-1076) landed; cookie-deletion tests are present. The real prod build in action-runtime-security.test.ts:243-249 exercises the patched plugin end-to-end and confirms owner-based routing (401 for a middleware-gated owner, action executes only through its owner).
But the rewrite introduces a few new concerns, and one is a merge blocker.
1. (Blocking) Server actions become unbuildable for end users — the required plugin-rsc API only exists in this repo's local patch
The new ownership pipeline depends entirely on manager.scanBuildObservers / manager.scanBuildModules, which are added by the new patches/@vitejs__plugin-rsc@0.5.27.patch (this file does not exist on main). That patch is applied only inside this monorepo via pnpm-workspace.yaml patchedDependencies (:104-105).
@vitejs/plugin-rsc is an optional peer dependency of vinext (packages/vinext/package.json:193,203-205), and vinext init adds it as a plain devDependency (src/init.js:108). End users therefore install the unpatched, published @vitejs/plugin-rsc from npm, which has no scanBuildObservers. There is no mechanism that ships this patch to consumers, and the old fallback (buildStaticActionOwnerManifest) was deleted in this PR.
Result: for any end-user App Router app with server actions, renderChunk hits the guard at index.ts:3729-3733:
if (!hasActionOwnerScanObserver && serverReferences.length > 0) {
this.error("[vinext] The installed @vitejs/plugin-rsc does not expose scan-build observers ... Install a compatible release before building.");
}
…and the build fails hard. The message tells users to "install a compatible release," but no such release exists on npm — the API is a local patch. So this ships as a hard regression: apps with server actions cannot build outside this repo. This needs one of: (a) vendor the observer logic so it doesn't depend on a patched plugin, (b) get the API upstreamed and bump the peer range to a real published version, or (c) restore a fallback path when the observer is absent. Please confirm the intended distribution story before merge.
2. (Regression from prior head) The generateBundle build-fail safety net was removed
ba44206 had a generateBundle hook that failed the build if __VINEXT_ACTION_OWNERS_STUB__ survived in any RSC chunk — every prior ask-bonk review credited this as converting the silent-runtime-404 into a hard build error. That hook is deleted in this PR (confirmed: git show ba44206:.../index.ts has "server action owner manifest was not injected into the RSC entry", e3630a8 does not).
The runtime failure mode it guarded against is unchanged and still severe: if the stub ships un-replaced, __VINEXT_ACTION_OWNERS() returns the literal string "__VINEXT_ACTION_OWNERS_STUB__". That string is truthy, so forwardServerActionIfNeeded's if (!options.actionOwners) return null guard (app-action-forwarding.ts:129) does not short-circuit; actionOwnerPatterns then does Object.hasOwn(<string>, actionId) → undefined → ownerPath undefined → every server action 404s (:143-148). The only remaining guard is the renderChunk per-chunk logic, which does not catch the case where the stub-bearing chunk is processed while rscActionOwnerRoutes is null (e.g. a future codegen split, or the load hook not re-firing). Recommend restoring the generateBundle assertion — it's cheap insurance against a total-breakage, silent failure mode and was explicitly relied on in prior review sign-offs.
3. Dev now disables action ownership/forwarding entirely (dev/prod parity gap)
In dev (command === "serve"), rscActionOwnerRoutes is forced to null and actionOwners is passed as null (index.ts:3527-3528 and the command === "build" ? undefined : null at the actionOwners: field). At runtime forwardServerActionIfNeeded returns null immediately for a null manifest (app-action-forwarding.ts:129), so in dev every action executes on whatever route it was POSTed to — the ownership gating and middleware re-entry this PR adds are prod-only.
The prior head (ba44206) computed a dev static manifest so ownership worked in dev too. This is a deliberate-looking simplification, but it's a real parity regression for a security-relevant feature (the PR's own framing is "action runtime takeover"). At minimum this deserves a code comment + a tracked follow-up; ideally dev keeps some ownership enforcement. dev/prod parity is a stated project goal.
4. Scan observer silently drops modules on positional-alignment mismatch
createActionOwnerScanObserver maps lexer specifiers to resolved ids positionally: sourceOrder[index] → event.info.importedIds[index] (action-owner-manifest.ts:284-291), gated by if (importedIds.length !== sourceOrder.length) return;. When the counts differ, the entire module's edges are dropped silently, so any action reachable only through that module becomes unowned → runtime 404 with no diagnostic (the same silent-drop class the prior review flagged, now in the observer).
The alignment is fragile:
sourceOrderdedups by specifier string (:179-182), whileinfo.importedIdsis Rollup's list of resolved ids. Two specifiers resolving to the same id (./aand./a.js) shrinkimportedIdsbelowsourceOrder→ whole module dropped.- Dynamic imports are pushed into
sourceOrderbefore theitem.d >= 0check (:178-186), so a static-string dynamic import is treated positionally as a static import; static-analyzable dynamic imports land inimportedIds(the scan-strip plugin rewrites them to staticimport "x"at plugin:445), but non-static-string dynamic imports change the count and drop the module.
Consider matching by resolved specifier rather than by index, and emitting at least a debug log when a module is dropped so a silent ownership gap is diagnosable. Not a blocker on its own, but it compounds #2 (there's no longer a build-time backstop).
5. Action re-render mode changed PRESERVE_UI → NAVIGATION via the main merge (confirm intended)
app-server-action-execution.ts:1408 / :1555 now use APP_RSC_RENDER_MODE_NAVIGATION instead of APP_RSC_RENDER_MODE_ACTION_RERENDER_PRESERVE_UI. The PRESERVE_UI constant was removed on main (app-rsc-render-mode.ts no longer defines it), and the merge (206e84c) reconciled these call sites to NAVIGATION. This is a semantic change to post-action re-render behavior that rode in with the merge rather than being an explicit decision in this PR. One of these is the dead inline-redirect fallback path; the other (:1555) is the action-rerender target. Worth confirming the NAVIGATION mode preserves the same UI/state semantics that PRESERVE_UI did, or the post-action re-render could regress.
Summary: the security model and the happy-path build (with the patch) are correct and well-tested. But #1 is a blocker — the patched-plugin dependency means server actions can't build for end users on the published @vitejs/plugin-rsc, and the build fails with an error pointing at a nonexistent "compatible release." #2 (removed generateBundle guard) and #4 (silent module drop) together remove the safety nets that prior reviews relied on for the total-breakage failure mode. #3 is a dev/prod parity regression for a security feature. Please confirm the plugin-rsc distribution story before merge.
|
Review posted to PR #2520. SummaryThe head is now Issues raised
Only #1 is a merge blocker; #2/#4 remove safety nets prior reviews relied on. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: server action routing alignment (head 2dc5a3a, 14 commits)
Reviewed the current head, which is one commit past the last ask-bonk pass (e3630a8). The new commit 2dc5a3a "retain imported inline action ownership" fixes a real ownership gap and extends the plugin patch. I re-verified the security model and re-ran the targeted suites with the patch applied (vp install → scanBuildObservers present in node_modules/@vitejs/plugin-rsc/dist/plugin-BK29Va7z.js):
tests/action-owner-manifest.test.ts+tests/app-action-forwarding.test.ts+tests/action-runtime-security.test.ts— 39 pass
Security model still sound: x-action-forwarded is stripped by filterInternalHeaders and re-injected only via the trusted ctx path; the loop guard fails closed to {} (app-action-forwarding.ts:149-152); redirects re-enter through middleware; both manifest-injection sites use safeJsonStringify.
New commit 2dc5a3a — verified correct
buildActionOwnerManifest now grants ownership to inline ($$hoist_*) actions declared inside reachable imported Server Components (action-owner-manifest.ts:441-452), using a new inlineExportNames field from the patched serverReferenceMetaMap. The gating on inlineExportNames.has(exportName) correctly avoids inferring provenance for user-named exports (covered by the new "does not infer inline action provenance from user-defined export names" test) while owning compiler-hoisted inline actions reachable through a plain import (the IMPORTED_INLINE_ACTION_EXECUTED prod test at action-runtime-security.test.ts:322). Good fix. Note it adds another patch-only API surface (inlineExportNames), which compounds #1 below.
1. (Blocking, unchanged) Server actions are unbuildable for end users — the required plugin API only exists in this repo's local patch
The ownership pipeline still depends entirely on manager.scanBuildObservers / manager.scanBuildModules / serverReferenceMetaMap.inlineExportNames, all of which are added by patches/@vitejs__plugin-rsc@0.5.27.patch (this file does not exist on main). I confirmed the published plugin lacks the API:
$ npm pack @vitejs/plugin-rsc@0.5.27 && grep -c scanBuildObservers package/dist/*.js
scanBuildObservers NOT in published 0.5.27
@vitejs/plugin-rsc is an optional peer dep with range ^0.5.26 (packages/vinext/package.json:193), and vinext init installs it as a plain devDependency (init.ts:226). End users therefore get the unpatched, published plugin. There is no mechanism to ship this patch to consumers, and no fallback path remains. Result: for any end-user App Router app with server actions, configResolved leaves hasActionOwnerScanObserver = false (index.ts:3202-3205), and renderChunk hits:
if (!hasActionOwnerScanObserver && serverReferences.length > 0) {
this.error("[vinext] The installed @vitejs/plugin-rsc does not expose scan-build observers ... Install a compatible release before building.");
}
(index.ts:3729-3733) — the build fails hard, pointing users at a "compatible release" that does not exist on npm. This ships as a hard regression. Needs one of: (a) vendor the observer/lexer logic so it doesn't depend on a patched plugin, (b) get the API upstreamed and pin the peer range to a real published version, or (c) restore a fallback when the observer is absent. Please confirm the distribution story before merge.
2. (Regression, unchanged) The generateBundle build-fail safety net is still removed
ba44206 had a generateBundle hook that failed the build if __VINEXT_ACTION_OWNERS_STUB__ survived un-injected in any RSC chunk ("vinext: server action owner manifest was not injected into the RSC entry"). Every prior ask-bonk review credited this with converting the silent-runtime-404 into a hard build error. It is still gone (grep for the message returns nothing in packages/vinext/src).
The failure mode it guarded is unchanged and severe: if the stub ships un-replaced, __VINEXT_ACTION_OWNERS() returns the literal string "__VINEXT_ACTION_OWNERS_STUB__". That string is truthy, so forwardServerActionIfNeeded's if (!options.actionOwners) return null (app-action-forwarding.ts:129) does not short-circuit; actionOwnerPatterns then does Object.hasOwn(<string>, actionId) → undefined → ownerPath undefined → every server action 404s (:143-148). The only remaining backstop is the per-chunk if (!injected) this.error(...) at index.ts:3742, which fires only when the stub is seen but the regex misses — not when the whole hasActionOwnerStub && rscActionOwnerRoutes block is skipped (e.g. rscActionOwnerRoutes already null, load hook not re-fired, or a future codegen split). Recommend restoring the generateBundle assertion — cheap insurance against a total-breakage, silent failure mode.
3. (Parity gap, unchanged) Dev disables action ownership/forwarding entirely
actionOwners is passed as null in serve (index.ts:3556, command === "build" ? undefined : null) and rscActionOwnerRoutes is null in dev (:3527-3528). At runtime forwardServerActionIfNeeded returns null immediately for a null manifest (app-action-forwarding.ts:129), so in dev every action executes on whatever route it was POSTed to — the ownership gating and middleware re-entry this PR adds are prod-only. For a security-relevant feature ("action runtime takeover") this deserves at least a code comment + tracked follow-up; ideally dev keeps some enforcement. dev/prod parity is a stated project goal.
4. (Silent drop, unchanged) Scan observer drops modules on positional-alignment mismatch
createActionOwnerScanObserver maps lexer specifiers to resolved ids positionally (action-owner-manifest.ts:288-292), gated by if (event.info.importedIds.length !== pending.sourceOrder.length) return; (:285-287). When counts differ, the entire module's edges are dropped silently → any action reachable only through it becomes unowned → runtime 404, no diagnostic. The alignment is fragile: sourceOrder dedups by specifier string (:180-183) while info.importedIds is Rollup's list of resolved ids, so two specifiers resolving to the same id (./a and ./a.js) shrink importedIds below sourceOrder and drop the whole module. Consider matching by resolved specifier rather than by index, and emitting a debug log on drop. Compounds #2 (no build-time backstop remains).
5. (Confirm) Action re-render mode is NAVIGATION (was PRESERVE_UI before the main merge)
app-server-action-execution.ts:1408/:1555 now use APP_RSC_RENDER_MODE_NAVIGATION. PRESERVE_UI was removed on main; the merge reconciled these sites to NAVIGATION. This is a semantic change to post-action re-render that rode in with the merge rather than being an explicit decision here. Worth confirming NAVIGATION preserves the same UI/state semantics PRESERVE_UI did.
Summary: the security model and the patched happy-path build are correct and well-tested, and 2dc5a3a closes a genuine inline-ownership gap. But #1 is a merge blocker — server actions can't build for end users on the published @vitejs/plugin-rsc, and the build fails pointing at a nonexistent release; 2dc5a3a adds yet another patch-only API. #2 (removed generateBundle guard) and #4 (silent module drop) together remove the safety nets prior reviews relied on for the total-breakage failure mode. #3 is a dev/prod parity regression for a security feature. Please confirm the plugin-rsc distribution story before merge.
|
Review posted to PR #2520. SummaryThe head is now New commit Issues raised
Only #1 is a merge blocker; #2/#4 remove safety nets prior reviews relied on. |
…ime-takeover # Conflicts: # pnpm-lock.yaml
2 participants
Summary
Validation
vp test run tests/action-owner-manifest.test.ts tests/app-action-forwarding.test.ts tests/action-runtime-security.test.ts tests/app-server-action-execution.test.ts tests/entry-templates.test.ts tests/app-router-rsc-plugin.test.tsvp check packages/vinext/src/server/app-action-forwarding.ts packages/vinext/src/build/action-owner-manifest.ts packages/vinext/src/entries/app-rsc-entry.ts packages/vinext/src/index.ts packages/vinext/src/server/app-rsc-handler.ts packages/vinext/src/server/app-server-action-execution.ts tests/app-action-forwarding.test.ts tests/action-owner-manifest.test.ts tests/action-runtime-security.test.ts tests/app-server-action-execution.test.ts tests/entry-templates.test.tsvp run vinext#buildgit diff --check