fix(app-router): expose active source page on window.next - #1995
Conversation
App Router pages did not update window.next.__internal_src_page, so Next.js-compatible tooling and deploy tests could not observe the active source page on initial render or client transitions. The payload only exposed URL route IDs, which drop route groups and bracketed dynamic segment names. The fix carries a source-page metadata field from the app route's filesystem segments, reads it from committed browser router state, and mirrors it onto window.next like Next.js does. Adds regression coverage for route-group and dynamic source-page encoding plus the window.next writer contract ported from the upstream app-dir test.
commit: |
|
/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 active source page on window.next
Verified the core logic against the Next.js reference and ran the targeted suites locally.
Correctness against Next.js — looks good. The key invariant (route groups like (newroot) and bracketed dynamic segment names like [id] must be preserved, not derived from the URL) matches Next.js's extractSourcePageSegmentsFromFlightRouterState / segmentToSourcePagePathname in compute-changed-path.ts. Group segments are intentionally not stripped from the source page (only the visible-path normalizeSegments strips them), and catch-all routes keep their [...slug]/[[...slug]] filesystem form — which is exactly what route.routeSegments carries here. The committed-state effect in app-browser-entry.ts also mirrors Next.js's useEffect(..., [state.tree]) that assigns the value or deletes it when undefined.
Validation locally:
vp test run tests/app-page-route-wiring.test.ts tests/shims.test.ts tests/app-page-element-builder.test.ts→ 1142 passedvp checkon the changed source files → clean (format, lint, types)
One consistency nit (non-blocking) noted inline: readSourcePageMetadata throws on a malformed value, which diverges from the established "degrade like a missing field instead of crashing render paths" convention used by readArtifactCompatibilityMetadata right above it. Since readMetadata now runs in the browser commit effect on every navigation and in several server hot paths, a throw here is stricter than the rest of the reader. In practice vinext always emits a valid value, so risk is low — flagging for parity with the surrounding code.
Nice, tightly-scoped change with good test coverage ported from the upstream contract.
| throw new Error("[vinext] Invalid __sourcePage in App Router payload: expected absolute path"); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Minor consistency note: this reader throws on a malformed __sourcePage, whereas readArtifactCompatibilityMetadata directly above (lines 698-707) deliberately degrades to a default rather than crashing, with a comment explaining that scaffolding metadata should "degrade like missing __layoutFlags instead of crashing render paths that do not read it."
readAppElementsMetadata is now called on every navigation in the browser commit effect (app-browser-entry.ts:1047) and in several server hot paths (app-ssr-entry.ts, app-optimistic-routing.ts, app-browser-client-reuse-manifest.ts), most of which don't read sourcePage. A throw here would break those paths for a payload that's otherwise valid. vinext always emits a valid value today so the risk is low, but consider degrading to null to match the surrounding convention:
| } | |
| function readSourcePageMetadata(value: unknown): string | null { | |
| if (value === undefined || value === null) return null; | |
| if (typeof value !== "string" || !value.startsWith("/")) return null; | |
| return value; | |
| } |
|
Review posted successfully. Summary of my analysis of PR #1995: Review outcome: Approve with one minor nitVerified correct against Next.js. The PR's central design decision — that the source page must preserve route groups ( Validation (local):
One non-blocking inline nit: The change is tightly scoped, well-layered (segments flow correctly through the route graph → matching → element builder → wire), and has good test coverage ported from the upstream contract. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: expose active source page on window.next
Verified the implementation locally and against the Next.js reference. The change is tightly scoped and correct.
Correctness — looks good.
- The core invariant (source page preserves route groups like
(newroot)and bracketed dynamic/catch-all segment names like[id]/[...slug], rather than being derived from the visible URL) is satisfied becauserouteSegmentsis built from raw filesystem segments (path.relative(appDir, routeDir)inapp-route-graph.ts:2490). This matches Next.js'ssegmentToSourcePagePathname/extractSourcePageSegmentsFromFlightRouterStateincompute-changed-path.ts, where group segments are intentionally not stripped from the source page. - The browser commit effect (
app-browser-entry.ts:1046) mirrors Next.js'suseEffect(..., [state.tree])assign-or-delete semantics: it writes the value when present and deletes the field whennull. __sourcePageis correctly optional on the wire —createMetadataEntriesomits it fornull/undefined, so older payloads round-trip cleanly and theroute: nullboundary path emits no field (verified byapp-page-boundary-render.test.ts:474).
On the prior nit (readSourcePageMetadata throwing): the earlier review flagged this reader as throwing on malformed input, diverging from readArtifactCompatibilityMetadata's graceful-degrade convention. The current code at app-elements-wire.ts:709 already degrades to null (validated by the degrades malformed optional source-page metadata to null test), so that concern is resolved — no change needed.
Validation (local):
vp test runon the 7 affected test files → 1309 passedvp checkon all 11 changed source files → clean (format, lint, types)
Minor observation (non-blocking): In app-page-boundary-render.ts:258, options.route.routeSegments is typed optional, so a route with undefined segments would yield /page rather than being omitted. In practice the metadata reader and createAppPageSourcePage's ?? [] fallback keep this benign, and null route already omits the field. Not worth changing.
LGTM.
|
Review posted successfully. Summary of my analysis of PR #1995: Review outcome: Approve (LGTM)Verified correct against Next.js. The PR's central invariant — that the source page preserves route groups ( The browser commit effect mirrors Next.js's On the prior reviewer's nit about Validation (local):
I noted one non-blocking minor observation (a route with |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: expose active source page on window.next
Verified the implementation against the Next.js reference and ran the targeted suites locally. The change is correct, tightly scoped, and well-layered.
Correctness vs Next.js — confirmed. Fetched compute-changed-path.ts from Next.js canary. The PR's central invariant is exactly right:
segmentToSourcePagePathnamepreserves route groups (group segments are not stripped here — onlynormalizeSegmentsstrips them for the visible path) and emits bracketed dynamic/catch-all forms ([id],[...slug],[[...slug]], plus interception markers like(..)(..)[id]).- The terminal segment is always
page, and the result is/${segments.join('/')}.
vinext's createAppPageSourcePage builds /${[...routeSegments, "page"].join("/")} from raw filesystem segments (path.relative(appDir, routeDir) at app-route-graph.ts:2490, and path.relative(appDir, path.dirname(page)) for sibling intercepts at app-route-graph.ts:2409), which carry groups and brackets verbatim. This produces the same value Next.js does without needing a Flight router-state extractor.
Browser commit effect — correct. app-browser-entry.ts:1046 runs on [treeState.elements] and mirrors Next.js's useEffect(..., [state.tree]) assign-or-delete: writes the value when present, deletes the field when the metadata reads null (e.g. null-route boundary payloads that omit __sourcePage).
Wire compatibility — correct. __sourcePage is omitted for null/undefined, so older payloads round-trip cleanly. readSourcePageMetadata degrades malformed input to null (matching the surrounding readArtifactCompatibilityMetadata convention) rather than throwing — the prior reviewer's nit about throwing was based on a stale revision and is already resolved in the current code.
Validation (local):
vp test runon all 8 touched test files → 1327 passed (1188 + 139)vp checkon the changed source files → clean (format, lint, types)
One non-blocking observation below. LGTM.
| rootLayoutTreePath: layoutEntries[0]?.treePath ?? null, | ||
| routeId, | ||
| sourcePage: options.route | ||
| ? createAppPageSourcePage(options.sourcePageSegments ?? options.route.routeSegments) |
There was a problem hiding this comment.
Minor (non-blocking): AppPageBoundaryRoute.routeSegments is typed optional (readonly string[] | undefined), so when options.route is truthy but its routeSegments is undefined, this calls createAppPageSourcePage(undefined) which returns "/page" and emits a __sourcePage field rather than omitting it.
In practice this is benign — real matched AppRoute values always populate routeSegments, and the route: null path already omits the field (covered by app-page-boundary-render.test.ts:474). But if you want strict parity with Next.js (which returns undefined when no source page can be derived), you could guard on the segments being present:
| ? createAppPageSourcePage(options.sourcePageSegments ?? options.route.routeSegments) | |
| sourcePage: | |
| options.route && (options.sourcePageSegments ?? options.route.routeSegments) | |
| ? createAppPageSourcePage(options.sourcePageSegments ?? options.route.routeSegments) | |
| : null, |
|
Review posted successfully. Summary of my analysis of PR #1995: Review outcome: Approve (LGTM)Verified correct against Next.js. I fetched
vinext's Browser commit effect correct. Wire compatibility correct. Validation (local):
One non-blocking inline observation: a boundary route with |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: expose active source page on window.next
Verified the implementation against the Next.js canary reference (compute-changed-path.ts and app-router.tsx) and ran the targeted suites locally.
Correctness vs Next.js — confirmed.
- The central invariant is right:
segmentToSourcePagePathnamepreserves route groups (group segments fall through toreturn segmentand are not stripped — only the visible-pathnormalizeSegmentsstrips them) and emits bracketed dynamic/catch-all forms ([id],[...slug],[[...slug]], interception markers like(..)(..)[id]). The terminal segment is alwayspage, and the result is/${segments.join('/')}. vinext'screateAppPageSourcePagebuilds/${[...routeSegments, "page"].join("/")}from raw filesystem segments (path.relative(appDir, routeDir)atapp-route-graph.ts:2490, andpath.dirname(page)for sibling intercepts atapp-route-graph.ts:2409), which carry groups and brackets verbatim — same value, no Flight router-state extractor needed. - The root-page case matches: Next.js returns
['page']→/page;createAppPageSourcePage([])→/page([]is truthy, so the boundary path correctly emits it rather than omitting). - The browser commit effect (
app-browser-entry.ts:1046) runs on[treeState.elements], parallel to the adjacentsetMountedSlotsHeadereffect, mirroring Next.js'suseEffect(..., [state.tree])assign-or-delete: writes the value when present, deletes the field when the metadata readsnull.
Wire compatibility — correct. __sourcePage is omitted for null/undefined (createMetadataEntries), so older payloads round-trip cleanly, and readSourcePageMetadata degrades malformed input (missing leading slash, non-string) to null rather than throwing — matching the surrounding readArtifactCompatibilityMetadata graceful-degrade convention. The prior reviewer's "throwing" nit is already resolved in this revision (validated by the degrades malformed optional source-page metadata to null test).
Validation (local):
vp test run tests/app-elements.test.ts tests/app-page-route-wiring.test.ts tests/shims.test.ts→ 1188 passed
Tightly scoped, well-layered (segments flow correctly from route graph → matching → element builder → wire → browser commit), with good test coverage ported from the upstream contract. LGTM.
|
Review posted. Summary of my analysis of PR #1995: Review outcome: Approve (LGTM)Verified correct against Next.js canary. I fetched both
Notes on prior review threads: The earlier "throwing reader" nit is already resolved in the current code, and the Validation: Ran the 3 targeted test files locally → 1188 passed. The change is tightly scoped and well-layered. No blocking issues found. |
2 participants
Overview
window.next.__internal_src_pagebehaviour.window.next.app-page-route-wiring.tsx,app-elements-wire.ts,app-browser-entry.ts,window-next.tsWhy
Next.js exposes the active app source page as an internal App Router diagnostic field. The invariant is that this value describes the matched source file path, not the visible URL. Vinext already had the
window.nextshape, but it only carried URL-style route identity through the app payload, which loses route groups like(newroot)and dynamic source segments like[id].__sourcePageto AppElements metadata, omitted for older payloads and validated when present.window.next.__internal_src_pageshould reflect committed App Router state.treeState.elementsin the browser root effect, matching Next.js' committed-tree effect./dashboard,/dynamic/[category]/[id], and/(newroot)/dashboard/anotherexpectations into focused Vinext tests.What changed
window.next.__internal_src_pagestayed unset./dashboard/page./dynamic/[category]/[id]/page.(newroot)./(newroot)/dashboard/another/page.__sourcePagereads asnulland deletes the window field.Maintainer review path
packages/vinext/src/server/app-page-route-wiring.tsxbuilds source-page strings from route filesystem segments.packages/vinext/src/server/app-elements-wire.tscarries and validates the optional metadata field.packages/vinext/src/server/app-browser-entry.tsmirrors committed router state towindow.next.packages/vinext/src/client/window-next.tsowns the field write/delete semantics.tests/app-page-route-wiring.test.tsandtests/shims.test.tscover the ported upstream contract at the lowest useful boundaries.Validation
vp test run tests/app-elements.test.ts tests/app-page-route-wiring.test.ts tests/shims.test.tsvp check packages/vinext/src/client/window-next.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-elements-wire.ts packages/vinext/src/server/app-elements.ts packages/vinext/src/server/app-page-element-builder.ts packages/vinext/src/server/app-page-route-wiring.tsx tests/app-elements.test.ts tests/app-page-route-wiring.test.ts tests/shims.test.tsvp run vinext#buildvp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/app/index.test.tsThe upstream deploy-suite run is file-scoped. The four
window.next.__internal_src_pagecases are green after this change. The file still exits non-zero on unrelated existing behaviours in redirects, rewrites, layout reuse, back/forward soft navigation, loading fallbacks, searchParams-on-rewrite, template components, and bootstrap script placement.Risk / compatibility
window.nextfield already modeled by Vinext.__sourcePageis optional. Missing metadata reads asnulland does not break older app payloads.Non-goals
test/e2e/app-dir/app/index.test.ts.References
window.next.__internal_src_page.pageas the terminal segment.