fix(router): minimize client rewrite manifests - #2734
Conversation
Browser entries currently serialize resolved rewrite objects verbatim even when the client cannot execute a rule locally. Project rewrites into a client-specific discriminated shape, retaining internal destinations that support SPA routing while handing external and missing-condition rules to the server. Keep the full matcher lazy on Pages and cover generated manifests and both client navigation ownership paths.
|
@codex review |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50582478a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Cookie and header rewrite conditions can depend on values that browser JavaScript cannot observe. Evaluating them client-side can both publish those values and skip rewrites that the server would match. Project only query and host conditions, fail closed for every other condition type, and use the same projection for the public build manifest so all client outputs preserve the same boundary.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # tests/entry-templates.test.ts
|
/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.
Reviewed the client rewrite projection change. This is a well-designed, correctly fail-closed change and the tests are thorough (canary strings + full manifest equality + both client consumers). No blocking issues found.
Correctness reasoning I verified:
- The client
hasprefilter is provably safe.checkHasConditionsuses AND semantics (config-matchers.ts:565— "every condition must match"), so retaining only the host/query subset ofhason arequiresServerEvaluationrule can only ever make the client more likely to hand off to the server, never less. If the client-safe subset fails to match, the full rule cannot match, so skipping is safe. matchClientRewritestill checksisExternalUrl(destination)after substitution, so a config-internal destination that becomes protocol-relative via decoded catch-all params still forces a document navigation. Parity with the old inlineisExternalUrl(rewritten)check is preserved.- All consumers of
__VINEXT_CLIENT_REWRITES__were updated: the sync path inrouter.ts:1824correctly folds the oldrewrite.missingcheck intorequiresServerEvaluation, and the length-only consumers (router.ts:1611,router.ts:3231) need no field changes. _buildManifest.jsnow shares the sametoClientRewritesprojection, resolving the earlier Codex P2 finding about protocol-relative destinations leaking into the publicly served manifest.
A couple of minor, non-blocking observations below.
| function toClientRewrite(rewrite: NextRewrite): ClientRewrite { | ||
| const clientHas = rewrite.has?.filter(isClientHasCondition); | ||
| const hasServerOnlyCondition = | ||
| rewrite.has?.some((condition) => !isClientHasCondition(condition)) ?? false; |
There was a problem hiding this comment.
Minor: isClientHasCondition is called twice per condition here — once in the clientHas filter (line 54) and again in this some(...) predicate. Functionally fine, but you could compute both in a single pass to avoid the double classification, e.g. partition into clientHas / server-only in one reduce. Not worth blocking on given rewrite counts are tiny.
| }; | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
Nit: the client-resolvable branch re-spells every field (source, has, locale, basePath) instead of spreading common like the server-eval branch above. Spreading ...common and adding destination would keep the two branches symmetric and avoid a future field being added to common but forgotten here:
| return { | |
| return { | |
| ...common, | |
| destination: rewrite.destination, | |
| }; |
| basePathState, | ||
| ); | ||
| if (destination === null) return null; | ||
| if (rewrite.requiresServerEvaluation || isExternalUrl(destination)) { |
There was a problem hiding this comment.
Just confirming the intent: for a requiresServerEvaluation rule with a client-safe has prefilter, matchRewrite runs the placeholder destination / through param substitution, but since the discriminator short-circuits to { kind: "server" } the substituted value is discarded. That's correct — the destination here is only ever used to detect a match, never surfaced. Worth a one-line comment noting the substituted / is intentionally thrown away for the server-eval case, since the current comment only explains why / is used, not that its output is unused.
|
Review posted successfully to PR #2734. SummaryI reviewed PR #2734 (fix(router): minimize client rewrite manifests) and posted a batch review with no blocking issues found. This is a well-designed, correctly fail-closed change. Key correctness properties I verified:
I ran the two most relevant test files locally — all 52 tests pass. Three minor, non-blocking suggestions posted inline:
|
2 participants
Overview
Why
Client routing is correct only when every input to a rewrite decision is authoritative in the browser. Query and host conditions meet that requirement. Cookie conditions may depend on HttpOnly values, arbitrary headers may be added by the server or an intermediary, and
missingconditions require complete request context.The client projection therefore fails closed. Rules with external destinations,
missingconditions, cookie or headerhasconditions, or unknown future condition types omit both the destination and server-only condition data. The source and any safe query or host preconditions remain available so the router can conservatively hand matching navigations back to the server.What changed
missingrewrite_buildManifest.jsMaintainer review path
packages/vinext/src/client/client-rewrites.tsdefines the allow-list and discriminated client manifest.packages/vinext/src/build/next-client-runtime-manifests.tsapplies that projection to_buildManifest.js.packages/vinext/src/client/client-rewrite-matcher.tsperforms client resolution or server handoff.tests/client-build-manifest.test.tsverifies the emitted file and serialized projection.tests/hybrid-client-route-owner.test.tsandtests/shims.test.tsverify App/Link and Pages Router behavior from raw rewrite config.Validation
_buildManifest.jsuses the same projection and omits protocol-relative destinations.Commands and results
vp check: passed across 1,164 checked files.vp test run tests/client-build-manifest.test.ts tests/entry-templates.test.ts tests/hybrid-client-route-owner.test.ts tests/shims.test.ts: 1,372 tests passed.vp run vinext#build: passed.Risk / compatibility
requiresServerEvaluationdiscriminator; destination-less entries remain compatible with the existing server-handoff behavior.