fix(app-router): expose not-found fallback flight payload#2349
fix(app-router): expose not-found fallback flight payload#2349james-elicx wants to merge 6 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/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: expose not-found fallback flight payload
Reviewed the 3 commits (10a14eb, 70b0dc2, be6fc0c). The implementation is clean, well-typed, the backward-compatible optionsOrNonce overload is handled correctly, and the chunk-tag mapping ([1, string] for text, [3, base64] for binary) matches Next.js's FlightSegment union in app-index.tsx. vp check is clean on all four runtime files and the new/changed tests pass locally.
One correctness concern worth resolving before merge, plus a couple of minor notes.
1. Per-flush self.__next_f.length=0 can wipe a multi-flush payload (functional)
be6fc0c moved the cleanup script out of finalize() into the end of every flush() batch:
// app-ssr-stream.ts flush()
if (options.mirrorNextFlight) {
scripts += createInlineScriptTag(createNextFlightCleanupScript(), options.scriptNonce);
}rscEmbed.flush() is called on every Fizz flush tick in the streaming transform (app-ssr-stream.ts:606), so a fallback render that flushes more than once produces:
- flush 1:
push([0]),push([1, A]),length=0← drops the[0]bootstrap and chunk A - flush 2:
push([1, B])(no new[0], sincemirroredNextFlightBootstrapstays true),length=0← drops chunk B
The net self.__next_f buffer ends empty, and the bootstrap [0] is gone for every batch after the first. Next.js's own consumer (nextServerDataCallback) throws "Unexpected server data: missing bootstrap script" when a [1,...] arrives with no preceding [0], so this is a genuine divergence from the transport this PR is trying to emulate.
Why the current tests still pass:
- The dev-server / ported parity assertions scrape
push([1,"..."])from the HTML text and ignorelength=0, so they're insensitive to runtime wipe. - The e2e test only asserts
__next_f.length === 0after hydration — which the per-flush cleanup trivially satisfies. - The targeted
notFound()boundary renders as a small synchronous tree, so in practice it flushes once and the bug doesn't surface.
So it works for the fixture, but the mechanism is incorrect for any multi-flush fallback. Two options:
- Preferred: emit the cleanup once in
finalize()(the pre-be6fc0cbehavior), and if the e2e "length must be 0" goal is the real requirement, keeplength=0only at the very end after all chunks. That preserves a decodable buffer for any consumer while still ending at length 0. - If per-batch clearing is truly intentional, document why the buffer is meant to be transient (i.e. that vinext never runs Next.js's
app-index.tsxconsumer) and add a multi-flush test that pins the expected behavior, so a future reader doesn't "fix" it back.
2. Add a multi-flush regression test
The new app-ssr-stream.test.ts cases all use a single batch (or a single chunk before an error). Regardless of which direction you take #1, please add a test that flushes twice with mirrorNextFlight: true and asserts the resulting script sequence — that's the case that actually distinguishes correct from incorrect cleanup placement and would have caught this.
3. Minor: not-found.tsx metadata export is not a Next.js convention
The fixture not-found.tsx exports a static metadata object. Next.js does not read a metadata export from not-found files (only layout/page/template participate in metadata resolution). vinext apparently honors it here, but if the intent is parity, the title/description should come from the route's generateMetadata/layout, not from not-found.tsx. Not blocking, but flag it so the fixture isn't mistaken for documented Next.js behavior.
Otherwise the change is focused and the gating (mirror only enabled for renderAppPageHttpAccessFallback, off for ordinary documents) is correct.
|
Review posted to PR #2349. Summary of findings: Review summaryThe PR is well-scoped and clean — correct typing, backward-compatible overload for One functional concern (should resolve before merge):
Minor notes:
|
|
/bigbonk review for issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: expose not-found fallback flight payload
Reviewed all 4 commits (10a14eb, 70b0dc2, be6fc0c, 2f01bf2). The latest commit 2f01bf2 ("preserve deferred flight consumers") directly resolves the multi-flush concern raised in my earlier review on be6fc0c. The implementation is now correct and well-tested. Verified locally: vp check clean on all 5 changed runtime/test files, tests/app-ssr-stream.test.ts (43 tests) and the targeted tests/app-router-dev-server.test.ts metadata cases pass.
Multi-flush cleanup is now correct ✅
2f01bf2 fixes the per-flush length=0 wipe I flagged before. The cleanup script is now:
- Emitted once, gated by
mirroredNextFlightBootstrap, alongside the[0]bootstrap on the first chunk (not at the end of everyflush()batch). - Deferred to
DOMContentLoadedinstead of running synchronously. - Guarded by
self.__next_f?.push === Array.prototype.push, so it only clears the buffer when no real consumer has taken overpush.
This is the right shape and matches the Next.js transport contract. In app-index.tsx, Next.js replaces self.__next_f.push with nextServerDataCallback (nextServerDataLoadingGlobal.push = nextServerDataCallback), and that callback throws "missing bootstrap script" if a [1,…] arrives without a preceding [0]. The new code emits exactly one [0], all chunks in order, then a single deferred cleanup — so the buffer stays decodable for the entire stream regardless of flush count, and the guard means vinext never stomps a Next.js consumer if one is installed. The new app-ssr-stream.test.ts "preserves progressive chunks until a deferred Next.js consumer claims the buffer" test pins exactly this ([[0],[1,"first"],[1,"second"]] across two flushes, with push reassignment). Good regression coverage — this is the test I asked for.
For the vinext-only browser case, self.__next_f is never consumed by vinext's own browser entry (grep confirms no client-side reader), so push stays Array.prototype.push, the DOMContentLoaded listener fires and clears to length=0, satisfying the e2e poll assertion. Consistent end-to-end.
Correction to my earlier minor note #3 (not-found.tsx metadata) ✅
My previous review claimed the fixture's static metadata export in not-found.tsx "is not a Next.js convention." That was wrong. The upstream fixture this is ported from does exactly the same thing:
test/e2e/app-dir/metadata-navigation/app/async/not-found/not-found.tsx exports metadata = { title: 'Local not found', description: 'Local not found description' }, and the test (metadata-navigation.test.ts:30 "should support notFound in generateMetadata") asserts getTitle(browser) is 'Local not found' and the description/keywords/robots come from exactly the sources this PR's fixture uses. So the vinext fixture is a faithful port and the static metadata on not-found.tsx is documented parity behavior. Disregard my earlier flag.
Other checks
- Chunk-tag mapping is correct:
[1, string]for text and[3, base64]for binary match Next.js'sFlightSegmentunion (app-index.tsx:60-64—[1, responsePartial],[3, responseBase64Partial]). The new "non-UTF-8" test assertsself.__next_f.push([3,"/wABAgM="]). - Gating is correct and dev/prod-parity-safe:
mirrorNextFlight: trueis set only inrenderAppPageHttpAccessFallback(app-page-boundary-render.ts:415), flows through the singleapp-page-stream→app-ssr-entrypath, and App Router prod inherits it via the built RSC entry. Ordinary documents leave itundefined(off). - Backward-compatible
optionsOrNonce: RscEmbedTransformOptions | stringoverload is handled correctly; no empty[0]-only mirror is emitted becauseflush()returns early on zero pending chunks and the bootstrap is emitted inside the per-chunk loop.
No blocking issues
The earlier functional concern is resolved and the requested multi-flush regression test was added. LGTM.
|
Review posted to PR #2349. SummaryI reviewed all 4 commits on PR #2349 (the prior review only saw 3). The newest commit Findings — no blocking issues:
LGTM. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: expose not-found fallback flight payload
Reviewed all 5 commits (10a14eb, 70b0dc2, be6fc0c, 2f01bf2, a021d3c). The two open concerns from the prior review rounds are both resolved, and the newest commit a021d3c ("avoid parsing script tags with regex") addresses the CodeQL/github-advanced-security alert. Verified locally: vp check clean on all changed runtime + test files, tests/app-ssr-stream.test.ts (43 tests) green, and the targeted tests/app-router-dev-server.test.ts metadata cases (renders dynamic metadata, generateMetadata() calls notFound) pass.
CodeQL "bad HTML filtering regexp" is resolved ✅
The advanced-security alert flagged a regex in tests/app-ssr-stream.test.ts that wouldn't match upper-case <SCRIPT> tags. a021d3c removes the regex-based script extraction in the multi-flush test and replaces it with node:vm (createContext/runInContext) plus a literal </script><script> String.split. The test no longer attempts to filter/parse HTML with a regex, so the alert is addressed without weakening coverage — it now actually executes the emitted scripts in a VM context and asserts the resulting self.__next_f buffer is [[0],[1,"first"],[1,"second"]], which is stronger than the prior text scrape. The only remaining regex in the file (/<script>once<\/script>/g) is a fixed-literal occurrence count, not an HTML sanitizer, so it is not in scope for the alert.
Multi-flush cleanup remains correct ✅
2f01bf2's shape is preserved: a single [0] bootstrap + a single deferred DOMContentLoaded cleanup, both gated by mirroredNextFlightBootstrap, with the cleanup guarded by self.__next_f?.push === Array.prototype.push. Per-chunk [1,…]/[3,…] pushes are emitted in order across every flush, and the buffer is only zeroed once at DOMContentLoaded if no real consumer claimed push. This keeps the buffer decodable for the whole stream and never stomps a Next.js consumer. The new VM-based regression test pins exactly this behavior.
Other checks
</script>breakout safety: mirrored chunks go throughsafeJsonStringify(html.ts:21), which escapes</>/&/U+2028/U+2029 to unicode escapes, matching the existing navigation-runtime chunk path. No injection risk from RSC text containing</script>.- Chunk-tag mapping is correct:
[1, string]for text,[3, base64]for binary, matching Next.js'sFlightSegmentunion; the non-UTF-8 test assertsself.__next_f.push([3,"/wABAgM="]). - Dev/prod parity:
mirrorNextFlight: trueis set only inrenderAppPageHttpAccessFallback(app-page-boundary-render.ts:412) and flows through the singleapp-fallback-renderer→app-page-stream→app-ssr-entrypath. App Router prod inherits this via the built RSC entry (app-rsc-entry.tsimportsapp-fallback-renderer.js), so there is no separate prod path to update. Ordinary documents leave the flagundefined(off) — confirmed by thedoes not mirror … by defaulttest and therenders dynamic metadataassertion that ordinary pages contain noself.__next_f. - No vinext client-side
__next_freader: grep confirms the__next_fmentions inapp-ssr-entry.ts/app-page-stream.tsare doc comments only, so vinext's own browser never consumes the buffer — theDOMContentLoadedclear-to-zero satisfies the e2e poll while the guard protects a hypothetical Next.js consumer. - Backward-compatible overload (
optionsOrNonce: RscEmbedTransformOptions | string) is handled correctly;flush()returns early on zero pending chunks so no empty[0]-only mirror is emitted. - The
not-found.tsxstaticmetadataexport is a faithful port of the upstream Next.js fixture (documented parity behavior, per the earlier correction).
No blocking issues
All prior functional concerns are resolved, the requested multi-flush regression test is present, and the CodeQL alert is addressed. LGTM.
|
Review posted to PR #2349. SummaryI reviewed all 5 commits (the prior reviews only covered 4). The newest commit Findings — no blocking issues:
LGTM. |
…t-found-flight # Conflicts: # packages/vinext/src/server/app-ssr-entry.ts # packages/vinext/src/server/app-ssr-stream.ts # tests/app-ssr-stream.test.ts
|
Worker B note: I checked this against GitHub Actions run 28478866791 / job 84413308650. This PR appears to cover the metadata-navigation notFound-in-generateMetadata failure in that run: it targets the fallback Flight payload for that exact metadata-navigation case and adds focused local coverage. I am treating that item as covered here and will skip duplicating this fix. |
|
Deploy-suite run 28478866791 / job 84413308650 triage: I think this PR already covers the metadata navigation notFound flight failure: test/e2e/app-dir/metadata-navigation/metadata-navigation.test.ts (1 failure for notFound in generateMetadata). I am skipping a duplicate backlog fix for that item and leaving this note so the overlap is visible. |
2 participants
Summary
self.__next_ftransportnotFound()thrown bygenerateMetadata()with local boundary metadataParity context
Run 28143992598 failed:
test/e2e/app-dir/metadata-navigation/metadata-navigation.test.tsshould support notFound in generateMetadataLocal found boundary, but decoded an empty payloadThe initially requested
segment-cache/metadatasuite was excluded because itsnext.config.jssetscacheComponents: true.Next.js v16.2.6 emits inline Flight instructions through
self.__next_finpackages/next/src/server/app-render/use-flight-response.tsx. Vinext already produced the correct local not-found boundary in its private navigation-runtime Flight stream, but did not expose that payload through the Next.js-compatible inline transport inspected by the parity test.Validation
vp test run tests/app-ssr-stream.test.ts -t "Next.js inline Flight|non-UTF-8"vp test run tests/app-router-dev-server.test.ts -t "renders dynamic metadata|generateMetadata.*calls notFound"vp checkon the six changed runtime/test filesvp run vinext#buildThe exact deploy-suite wrapper was attempted with one file and concurrency 1, but the retained Next.js checkout triggered an unexpected
pnpm install; the run was stopped without changing the lockfile. CI should perform the full exact-head deploy verification.