fix(app-router): isolate page CSS chunks in production by NathanDrake2406 · Pull Request #1738 · cloudflare/vinext

NathanDrake2406 · 2026-06-04T14:40:56Z

Overview

Area	Detail
Goal	Match Next.js production App Router CSS isolation for sibling hash routes
Core change	Page modules in the generated RSC manifest load through cached dynamic `import()` loaders instead of static imports
Boundary	Production App Router page CSS chunks, prerender `generateStaticParams`, and intercepted/source-route renders
Primary files	`packages/vinext/src/entries/app-rsc-manifest.ts`, `packages/vinext/src/entries/app-rsc-entry.ts`, `packages/vinext/src/server/app-rsc-handler.ts`, `tests/app-router.test.ts`
Impact	Sibling page-level global CSS no longer leaks `scroll-padding-top` into unrelated hash pages

Why

Native hash scrolling uses the browser's layout state. Next.js calls scrollIntoView() for hash fragments, so a 20px-short scroll offset means unrelated CSS is present, not that hash math is wrong.

Area	Principle / invariant	What this PR changes
Route CSS	A matched page should only receive CSS for that page's route tree	Page modules are not statically imported into the shared RSC manifest chunk
Runtime loading	Route matching stays synchronous, rendering gets loaded modules	The handler loads the matched route once before page dispatch and action rerenders await matched/source routes
Prerender	Missing `generateStaticParams` must remain distinguishable from malformed return values	Lazy page sources return an internal missing-export sentinel consumed by the existing resolver

What changed

Scenario	Before	After
`/hash` with sibling `/hash-with-scroll-offset`	Linked CSS could include `scroll-padding-top:20px` from the sibling page	The no-offset route's linked CSS excludes the sibling page rule
Page `generateStaticParams`	Dynamic page modules were statically imported just to inspect an optional export	Page sources load lazily and report missing exports explicitly
Intercepted/action rerenders	Source route lookup assumed modules were already static imports	Source route lookups can await the same cached route-module loader

Validation

vp test run tests/app-router.test.ts -t "keeps production route-level global CSS isolated"
vp test run tests/app-router.test.ts tests/app-prerender-endpoints.test.ts tests/app-page-request.test.ts tests/app-page-dispatch.test.ts tests/app-server-action-execution.test.ts tests/app-rsc-handler.test.ts tests/entry-templates.test.ts
vp check
Commit hook: entry-template snapshots, vp check, and knip
Upstream deploy suite: vp 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/navigation/navigation.test.ts
- The original /hash-link-back-to-same-page scroll-offset failure now passes.
- /hash-with-scroll-offset passes.
- The suite still fails on separate pending navigation parity bugs: Pages useParams log capture, hash with-query-param RSC request detection, redirect-with-loading duplicate render, nested navigation, Promise proxy navigation, and scroll restoration.

Risk / compatibility

Page modules now load at dispatch time rather than RSC manifest module initialization time.
Static route matching remains synchronous because route metadata stays in the manifest.
Root fallback boundaries and global-not-found behavior keep their existing loading model.
Layout/template/error/loading modules are still static imports in this PR; this PR targets the page-level CSS leak observed in the upstream navigation fixture.

References

Reference	Why it matters
Next.js navigation hash tests	Source of the deploy-suite failure and expected hash-scroll behavior
Next.js layout-router hash scrolling	Shows hash navigation delegates to native `scrollIntoView()`
Next.js segment-cache navigation	Shows same-path hash navigations are treated as hash-only scroll updates

pkg-pr-new · 2026-06-04T14:41:55Z

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1738

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

commit: 669433e

Production App Router builds statically imported every page module into the generated RSC manifest. That let Vite/Rolldown concatenate page-level global CSS from sibling routes into shared CSS assets, so a sibling hash page with scroll-padding-top changed native hash scroll offsets on unrelated pages. The manifest now emits cached dynamic loaders for page modules and loads the matched page module at the route dispatch boundary. Page generateStaticParams sources use the same loader with an explicit missing-export sentinel so prerender still distinguishes absent exports from malformed results.

james-elicx · 2026-06-04T22:16:35Z

/bigbonk review

ask-bonk

Review: isolate page CSS chunks in production

Nice, well-structured change. The core idea — moving page modules from static manifest imports to cached dynamic import() loaders so a route only pulls in its own CSS chunk — is sound and matches Next.js's per-route CSS scoping. I traced the request, action, intercept, and prerender paths and the lazy-load gating is consistent.

Correctness — verified

Request path: app-rsc-handler.ts:664-666 loads the matched route via loadRouteModules before dispatch, so handlePageRequest (route.page?.default, segment config, generateStaticParams) reads a populated route.page. ✓
Action path: handleServerActionRequest is now async and awaits __loadRouteMatch(matchRoute(...)) before reading __actionMatch.route.page; the inner matchRoute option is also wrapped, so action rerenders/redirect targets load first. ✓
Intercept path: findIntercept returns a per-request shallow copy ({ ...entry, matchedParams } in app-rsc-route-matching.ts:129), so the intercept.page = await intercept.__pageLoader() mutation in app-page-request.ts:336 does not mutate the shared lookup entry. ✓
Prerender parity: the switch from mod?.generateStaticParams (filtered out when absent) to an always-present __createLazyGenerateStaticParamsSource(...) that returns the MISSING_GENERATE_STATIC_PARAMS sentinel is handled equivalently downstream. The build prerender path already treats a function-returning-null the same as a missing function (build/prerender.ts:1096 vs :1139), so the output: 'export' error and no-static-params skip behaviors are preserved. ✓

Notes (non-blocking)

No direct unit test for the new helper module. Per AGENTS.md ("If you move behavior out of a template string, add a focused unit test for the new helper module"), app-route-module-loader.ts has meaningful logic (WeakMap dedup, shared-promise reuse, page mutation) but is only covered indirectly via app-router.test.ts. A small focused test would lock in the dedup/caching contract.
Rejected loads are cached permanently. See inline comment — both createRouteModuleLoader and loadRouteModules cache the in-flight promise, so a transient import() failure poisons the route for the lifetime of the worker instance with no retry. Likely acceptable for built chunks, but worth a comment documenting the intent.
Minor inefficiency: intercept route modules are keyed in the loadRouteStates WeakMap by the per-request { ...entry } copy, so the route-level load cache is not reused across requests for intercepts (the underlying createRouteModuleLoader promise cache still dedups the actual import(), so this is harmless).

Nothing blocking from my read.

ask-bonk · 2026-06-04T22:21:13Z

+  await promise;
+  state.loaded = true;
+  return route;
+}


Rejected loads are cached forever. If loadLazyModules(route) rejects, state.promise holds the rejected promise and state.loaded stays false, so every later loadRouteModules(route) call falls into the state.promise branch (line 47-50) and re-awaits the same rejection — the route is permanently broken within this worker instance with no retry. createRouteModuleLoader (line 34-40) has the same behavior. This is probably fine for built page chunks (a failure means a genuinely missing/broken chunk), but it's a deliberate-looking tradeoff that isn't documented. Consider a short comment noting the no-retry caching is intentional, or clearing state.promise on rejection if transient retries are desired.

ask-bonk · 2026-06-04T22:21:13Z

+      return MISSING_GENERATE_STATIC_PARAMS;
+    }
+    return generateStaticParams(input);
+  };


Behavioral note (not a bug): because lazy sources are now always functions, a dynamic route with a page that has no generateStaticParams now eagerly import()s the page module during prerender enumeration just to discover the export is absent (returning the sentinel). Previously the absent export was filtered out at resolver-build time without loading the module. This is fine since prerender runs at build time, but it does mean more module loads during generateStaticParams discovery. Worth confirming this doesn't measurably slow large App Router prerender builds.

ask-bonk · 2026-06-04T22:21:22Z

Review posted successfully to PR #1738.

Summary

I reviewed the PR, which isolates page CSS chunks in production by moving App Router page modules from static manifest imports to cached dynamic import() loaders.

Verdict: well-structured, nothing blocking.

I traced all four affected paths and confirmed the lazy-load gating is consistent:

Request path — app-rsc-handler.ts:664-666 loads the route before dispatch, so route.page is populated when read.
Action path — handleServerActionRequest correctly became async and awaits the loaded match before reading route.page.
Intercept path — findIntercept returns a per-request shallow copy, so the intercept.page = await ... mutation doesn't corrupt the shared manifest lookup entry.
Prerender parity — verified the new MISSING_GENERATE_STATIC_PARAMS sentinel design preserves the old behavior in both the runtime resolver and the build prerender path (output: 'export' errors and no-static-params skips still work, since the code already handled function-returns-null identically for the CF Workers Proxy case).

Non-blocking notes I flagged:

No direct unit test for the new app-route-module-loader.ts helper (AGENTS.md asks for one when extracting logic from template strings).
Rejected import()s are cached permanently with no retry (inline comment) — likely fine for built chunks but undocumented.
Minor: intercept WeakMap caching keyed on per-request copies (harmless, since the underlying loader promise still dedups).

github run

Address the non-blocking review note from PR cloudflare#1738: per AGENTS.md, behavior moved out of a template string should get a focused helper test. Cover createRouteModuleLoader (promise dedup, rejection propagation), loadRouteModules (page assignment, concurrent dedup, no-op cases, missing `page` key) and loadRouteMatch (null short-circuit, match load). Also cover createLazyGenerateStaticParamsSource (delegation, missing sentinel for null/empty/non-function exports). Document the rejected-load cache intent and the per-request intercept copy in the WeakMap so future readers don't try to "fix" them as bugs.

NathanDrake2406 · 2026-06-05T04:39:45Z

Thanks for the review! Addressed all three non-blocking notes in 1e33ba5:

Unit test for the helper module — added tests/app-route-module-loader.test.ts (14 tests) covering createRouteModuleLoader (promise dedup, rejection propagation), loadRouteModules (page assignment, concurrent dedup, no-op cases, missing page key), loadRouteMatch (null short-circuit, match load), and createLazyGenerateStaticParamsSource (delegation, missing sentinel for null / non-function exports).
Rejected-load cache — added a comment in createRouteModuleLoader documenting that the loaders point at built chunks whose URL is stable for a deploy, so a rejection means the chunk is genuinely missing and rolling forward (rebuild/redeploy) is the recovery path. Rejecting within the same worker would just race a known-failing chunk and confuse error responses.
Per-request intercept copy in the WeakMap — added a comment above lazyRouteStates explaining that the WeakMap keys on the per-request shallow copy returned by findIntercept, so the loaded flag is per-request. The actual import() dedup still happens in the createRouteModuleLoader closure promise, which is keyed on the shared manifest entry, so concurrent loads for the same intercept still share a single in-flight import.

CI is green on a9852d2 — let me know if you want any further tweaks.

NathanDrake2406 · 2026-06-05T05:39:18Z

Agreed — a positive case proving the modal CSS is emitted when the intercept is active is a good follow-up, but not blocking. The runtime already loads the intercept page before render (the __pageLoader resolution in resolveAppPageInterceptState), and the lazy page-CSS path is covered by the regular page tests, so the gap is purely a regression guard for the "intercept active" direction. Happy to add it in a follow-up if you'd like — filing nothing right now since you said it shouldn't block this PR.

james-elicx · 2026-06-05T09:39:47Z

/bigbonk review

ask-bonk

Re-review: isolate page CSS chunks in production

I independently traced all four request paths and re-ran the targeted suites. The change is sound and the three follow-ups from the prior review are addressed. Nothing blocking.

Verified

Request path — app-rsc-handler.ts:663-666 awaits loadRouteModules(matchedRoute) before dispatchMatchedPage reads route.page?.default (app-rsc-entry.ts:595). ✓
Action path — handleServerActionRscRequest is async; matchRoute is wired to __loadRouteMatch(matchRoute(...)) and getSourceRoute to __loadRouteModules(routes[i]), so route.page is populated before reads at app-server-action-execution.ts:645 (canRenderActionRedirectTarget) and app-rsc-entry.ts:797. ✓
Intercept path — findIntercept returns a fresh shallow copy per call (app-rsc-route-matching.ts:129), so the intercept.page = await intercept.__pageLoader() mutation (app-page-request.ts:335-336) never touches the shared interceptLookup entry. The shared __pageLoader closure still dedups the underlying import(). ✓
Prerender parity — the always-present lazy source returns MISSING_GENERATE_STATIC_PARAMS, and the new resolvedThisSource/resolvedAnySource tracking makes a layout-only pattern (page absent) behave identically to the old length === 1 branch. The sentinel is converted to null at the resolver boundary (lines 37, 78), which the build prerender path already treats the same as a missing function (build/prerender.ts:1096, :1117, :1135-1149) — so output: 'export' errors and no-static-params skips are preserved. ✓

Checks I ran locally

vp test run tests/app-route-module-loader.test.ts tests/app-prerender-endpoints.test.ts tests/app-page-request.test.ts tests/entry-templates.test.ts → 75 passed
vp test run tests/app-router.test.ts -t "global CSS isolated" → both isolation tests pass (sibling-route and intercepted-modal)
vp check on the changed server modules + new test → clean (format, lint, types)

Notes (non-blocking, already acknowledged)

Rejected import() results are cached for the worker lifetime in both createRouteModuleLoader and loadRouteModules (a rejection poisons the route with no retry). The added comment documents this as intentional for stable built chunks; fine.
The intercept-modal test only asserts the negative direction (modal CSS absent on direct /feed visit). A positive guard (modal CSS present when the intercept is active) would lock in the lazy-load-on-intercept path as a regression guard — good follow-up, not blocking.

LGTM.

ask-bonk · 2026-06-05T09:43:38Z

Review posted successfully to PR #1738.

Summary

I independently re-reviewed the PR, which isolates App Router page CSS chunks in production by moving page modules from static manifest imports to cached dynamic import() loaders.

Verdict: well-structured, nothing blocking.

I traced all four affected paths against the actual code (not just the diff) and confirmed lazy-load gating is consistent:

Request path — route loaded via loadRouteModules before dispatch reads route.page.
Action path — handler became async; both matchRoute and getSourceRoute are wrapped with loaders, so route.page is populated before all reads.
Intercept path — findIntercept returns a per-request shallow copy (app-rsc-route-matching.ts:129), so the intercept.page mutation never corrupts the shared lookup entry; the __pageLoader closure still dedups the underlying import().
Prerender parity — the new MISSING_GENERATE_STATIC_PARAMS sentinel + resolvedThisSource/resolvedAnySource tracking makes a layout-only pattern behave identically to the old length === 1 branch. The sentinel→null conversion is handled equivalently by the build prerender path (verified at build/prerender.ts:1096/1117/1135-1149).

Verification I ran locally: 75 unit tests pass, both CSS-isolation integration tests pass, and vp check is clean on the changed modules.

The two non-blocking notes (permanent rejection caching, missing positive intercept-CSS assertion) were already acknowledged by the author and prior reviewer.

github run

…-isolation # Conflicts: # tests/app-router.test.ts

james-elicx · 2026-06-05T15:42:43Z

/bigbonk review for issues

ask-bonk · 2026-06-05T15:43:48Z

UnknownError: "Overloaded"

github run

ask-bonk · 2026-06-05T15:43:51Z

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

james-elicx · 2026-06-05T15:44:32Z

/bigbonk review for issues

ask-bonk · 2026-06-05T15:45:32Z

UnknownError: "Overloaded"

github run

james-elicx · 2026-06-05T20:45:13Z

/bigbonk review for issues

ask-bonk

Review for issues: isolate page CSS chunks in production

I traced all four lazy-load paths against the actual code (not just the diff) and ran the targeted suites locally. No blocking issues. The change is correct, the dedup/cache contract is sound, and prerender parity holds.

Verified correct

Request path — app-rsc-handler.ts:670-673 awaits loadRouteModules(matchedRoute) before dispatchMatchedPage reads route.page?.default (app-rsc-entry.ts:597). ✓
Action path — handleServerActionRscRequest is async; matchRoute is wrapped with __loadRouteMatch and getSourceRoute with __loadRouteModules, so route.page is populated before canRenderActionRedirectTarget reads it (app-server-action-execution.ts:645) and before the segment-config read at app-rsc-entry.ts:799. ✓
Intercept path — findIntercept returns a per-request shallow copy (app-rsc-route-matching.ts:129) carrying the shared __pageLoader, so the intercept.page = await intercept.__pageLoader() mutation (app-page-request.ts:335-336) never corrupts the shared lookup entry, and the shared loader closure still dedups the import(). ✓
Prerender parity — the always-present lazy source returning MISSING_GENERATE_STATIC_PARAMS, combined with resolvedThisSource/resolvedAnySource, makes a layout-GSP + missing-page-GSP route behave identically to the old filtered single-source path. The resolver's null return is treated equivalently to a missing function by the build path (build/prerender.ts:1096, :1117, :1139), so output: 'export' errors and no-static-params skips are preserved. ✓

Checks I ran

vp test run tests/app-route-module-loader.test.ts tests/app-prerender-endpoints.test.ts → 28 passed
vp test run tests/entry-templates.test.ts tests/app-page-request.test.ts → 48 passed
vp test run tests/app-router-production-server.test.ts → 42 passed (both CSS-isolation tests green)

One new observation (non-blocking, scope note)

Parallel-slot page modules (slot.pagePath, app-rsc-manifest.ts:163) are still statically imported (imports.getImportVar), unlike the matched route page and the intercepting-route page on line 169 (which now use getLazyImportVar). A non-intercepting parallel-route page that imports route-global CSS would therefore still contribute its CSS to the shared manifest chunk — the same class of leak this PR fixes for the main page. This is consistent with the PR's stated scope, and the current fixtures don't exercise it (the @modal slot only has an intercepting page), so it's not a regression. Worth tracking as a follow-up so the remaining gap is explicit.

The other two known notes (permanent rejection caching, missing positive intercept-CSS assertion) were already raised and acknowledged. LGTM.

ask-bonk · 2026-06-05T20:49:30Z

      if (slot.errorPath) imports.getImportVar(slot.errorPath);
      for (const ir of slot.interceptingRoutes) {
-        imports.getImportVar(ir.pagePath);
+        imports.getLazyImportVar(ir.pagePath);


This intercepting-route page is now lazy (getLazyImportVar), but the parallel-slot page on line 163 (if (slot.pagePath) imports.getImportVar(slot.pagePath);) is still a static import. A non-intercepting parallel-route page that imports route-global CSS would still leak its CSS into the shared manifest chunk — the same class of leak this PR fixes for the main page. Not a regression and out of this PR's scope (current fixtures only have an intercepting modal page), but worth a follow-up so the remaining gap is tracked.

ask-bonk · 2026-06-05T20:49:39Z

Review posted successfully to PR #1738.

Summary

I reviewed the PR for issues, tracing all four lazy-load paths against the actual source and running the targeted test suites locally.

Verdict: no blocking issues. The change is correct.

What I verified

Request path — modules loaded before route.page is read in dispatch.
Action path — handler is async, both matchRoute and getSourceRoute wrapped with loaders, so route.page is populated before all reads (including canRenderActionRedirectTarget).
Intercept path — findIntercept returns a per-request shallow copy carrying the shared __pageLoader, so the intercept.page mutation never corrupts the shared lookup entry; the loader closure still dedups the import().
Prerender parity — the MISSING_GENERATE_STATIC_PARAMS sentinel + resolvedThisSource/resolvedAnySource tracking preserves the old filtered-source behavior; the resolver's null return is treated identically to a missing function by build/prerender.ts.

Tests run locally (all green)

app-route-module-loader + app-prerender-endpoints → 28 passed
entry-templates + app-page-request → 48 passed
app-router-production-server → 42 passed (both CSS-isolation tests)

One new observation (non-blocking)

Parallel-slot page modules (slot.pagePath) are still statically imported, unlike the main route page and intercepting-route pages. A non-intercepting parallel-route page importing route-global CSS would still leak into the shared chunk — same class of leak, but out of this PR's scope and not exercised by the fixtures. I flagged it inline as a follow-up.

The two previously-known notes (permanent rejection caching, missing positive intercept-CSS assertion) were already acknowledged by the author and prior reviews.

github run

…-isolation

james-elicx · 2026-06-08T09:02:56Z

/bigbonk review for issues

ask-bonk

Review: isolate page CSS chunks in production

Reviewed against the actual source (not just the diff), and ran the targeted suites locally. No blocking issues. The change is correct, the lazy-load gating is complete, and the new helper is well tested.

Verified correct

Manifest: intercepting-route pages now emit page: null + __pageLoader: load_N (app-rsc-manifest.ts:226-227, registered as a lazy loader at :191), keeping their CSS out of the shared RSC chunk. Matches the existing model for static/dynamic page modules.
Request path: app-rsc-handler.ts:712 awaits ensureRouteLoaded(route) before both the route-handler branch (:730 reads route.routeHandler) and dispatchMatchedPage (:748, which reads route.page?.default at app-rsc-entry.ts:599). ✓
Action path: handleServerActionRscRequest awaits resolveAppPageActionRerenderTarget(...) and then ensureRouteLoaded(actionRerenderTarget.route) at app-server-action-execution.ts:1171 before reading the page module. ✓
Intercept path: findIntercept returns a per-request shallow copy (app-rsc-route-matching.ts:129 → { ...entry, ... }), so the intercept.page = await intercept.__pageLoader() mutation at app-page-request.ts:335-336 never corrupts the shared lookup entry. The source route's page is loaded via buildPageElements (await __ensureRouteLoaded(route) at app-rsc-entry.ts:499) and the revalidation/action paths both hydrate the target route first (app-page-dispatch.ts:577). ✓
Prerender parity: createAppPrerenderStaticParamsResolver loads lazy { load } page sources on demand and filters fn !== null (app-prerender-static-params.ts:80, 83). A page with no generateStaticParams yields a null fn that's dropped → fns.length === 0 → resolver returns null, identical to the old eager-filtered behavior, so output: 'export' errors and no-static-params skips are preserved. ✓

Tests run locally (all green)

app-route-module-loader + app-prerender-endpoints + app-page-request + entry-templates → 70 passed
app-router-production-server -t "global CSS isolated" → both isolation tests pass

Notes for the record (non-blocking)

Rejection caching — actually correct here (clarifying prior reviews). The earlier bonk reviews stated rejected import()s are "cached forever." That's not what the code does: ensureAppRouteModulesLoaded clears __loading and leaves __loaded false on rejection (app-route-module-loader.ts:78), so the next request retries. The unit test at app-route-module-loader.test.ts:93 locks this in. No change needed — just flagging that the prior review's premise was inaccurate.
Parallel-slot page leak still open. slot.pagePath is still a static getImportVar (app-rsc-manifest.ts:185), unlike the intercepting page on :191. A non-intercepting parallel-route page importing route-global CSS would still leak into the shared chunk. Consistent with this PR's stated scope and not exercised by fixtures; worth a tracking follow-up.
Intercept-modal test asserts only the negative direction. It proves modal CSS is absent on a direct /feed visit but not present when the intercept is active, so a regression where the lazy modal page fails to load its CSS could pass silently. Author already acknowledged this as a follow-up.
Bundled e2e flakiness fix. The dev-error-overlay.spec.ts serial-mode + waitForCleanRsc change is unrelated to CSS isolation. Benign, but tangential to the PR's stated scope.

LGTM.

ask-bonk · 2026-06-08T09:07:37Z

+    const feedInlineCss = getInlineStyleText(feedHtml);
+    expect(feedInlineCss).not.toContain("scroll-padding-top");
+  });
+


This intercept-modal test only asserts the negative direction — modal CSS is absent on a direct /feed visit. It does not prove the modal CSS is emitted when the intercept is active, so a regression where the lazy modal page fails to load its CSS would pass this test silently. A positive guard (navigate /feed -> intercepted /photos/[id] and assert scroll-padding-top is present) would lock in the lazy-load-on-intercept path. Non-blocking follow-up.

ask-bonk · 2026-06-08T09:07:37Z

      if (slot.errorPath) imports.getImportVar(slot.errorPath);
      for (const ir of slot.interceptingRoutes) {
-        imports.getImportVar(ir.pagePath);
+        imports.getLazyLoaderVar(ir.pagePath);


Intercepting-route pages are now lazy here. Note the parallel-slot page module a few lines up (if (slot.pagePath) imports.getImportVar(slot.pagePath);) is still a static eager import. A non-intercepting parallel-route page that imports route-global CSS would still contribute its CSS to the shared manifest chunk — the same class of leak this PR fixes for the main and intercepting pages. Out of scope and not exercised by current fixtures, but worth tracking as a follow-up so the remaining gap is explicit.

ask-bonk · 2026-06-08T09:07:48Z

Review posted successfully to PR #1738.

Summary

I reviewed PR #1738 against the actual source (not just the diff) and ran the targeted suites locally. No blocking issues — the change is correct, the lazy-load gating is complete, and the new helper is well tested.

What I verified

Manifest: intercepting-route pages now emit page: null + __pageLoader (app-rsc-manifest.ts:226-227), matching the existing lazy model for static/dynamic pages.
Request path: app-rsc-handler.ts:712 awaits ensureRouteLoaded(route) before both the route-handler and page-dispatch branches read route.routeHandler / route.page.
Action path: target route hydrated at app-server-action-execution.ts:1171 before the page module read.
Intercept path: findIntercept returns a per-request shallow copy (app-rsc-route-matching.ts:129), so the intercept.page mutation never corrupts the shared lookup entry; source/revalidation routes are loaded via buildPageElements and app-page-dispatch.ts:577.
Prerender parity: lazy { load } sources are imported on demand and null fns filtered out, preserving the old output: 'export' / no-static-params behavior.
Ran 70 unit tests + both CSS-isolation production tests locally — all green.

Notable correction

The two prior bonk reviews claimed rejected import()s are "cached forever." That is wrong — the actual ensureAppRouteModulesLoaded clears __loading on rejection and retries, with a dedicated unit test locking that in. I flagged this in the review so the record is accurate.

Non-blocking follow-ups flagged inline

Parallel-slot page modules are still eagerly imported (same class of CSS leak, out of scope).
The intercept-modal test only asserts the negative direction (no positive guard that modal CSS appears when active).
The bundled dev-error-overlay.spec.ts flakiness fix is unrelated to CSS isolation but benign.

LGTM.

github run

… probe Adds the positive-direction guard the bonk review asked for: navigating /feed -> intercepted /photos/[id] must load the lazy modal page and emit its scroll-padding-top CSS chunk. The existing test only proved the negative (CSS absent on a direct /feed visit), so a regression where the lazy modal page failed to load its CSS would have passed silently. Also fixes the probePage() intercept branch, which read intercept.page without resolving the now-lazy __pageLoader. The render path awaits the loader (resolveAppPageInterceptState), but the probe path did not, so the dynamic-bailout probe silently inspected an undefined component and never observed the intercept page's searchParams/headers access. Resolve the lazy page before probing to match the render path.

james-elicx · 2026-06-08T10:47:14Z

Pushed 669433e addressing the latest bonk review:

1. Positive intercept-modal CSS guard (app-router-production-server.test.ts:178) — added emits intercepted modal CSS on RSC navigation from feed, which fires the /feed → /photos/[id] intercept and asserts the lazy modal page's scroll-padding-top CSS chunk is loaded and referenced in the flight payload. Together with the existing negative test this locks both directions of the lazy-load-on-intercept path.

Self-review finding — probe path didn't load the lazy intercept page. While verifying, I found that probePage() calls findIntercept(...) and buildAppPageProbes reads intercept.page?.default for the dynamic-bailout probe, but — unlike the render path (resolveAppPageInterceptState awaits __pageLoader) — it never resolved the now-lazy __pageLoader. So the intercept probe branch was silently inspecting undefined and never observing the intercept page's searchParams/headers access. Fixed by resolving the lazy page before probing.

Parallel-slot page leak (app-rsc-manifest.ts:185) — left as the tracked follow-up, per the scope agreed across the prior reviews; generalizing lazy-loading to non-intercepting slot pages touches the render-wiring/probe/segment-config/head read sites and is a larger, separate change.

Targeted suites green locally (production-server, page-request, dispatch, rsc-handler, prerender-endpoints, dev-server intercept, route-matching, page-wiring, probe) and vp check is clean.

james-elicx · 2026-06-08T11:02:52Z

/bigbonk review for issues

ask-bonk

Review: isolate page CSS chunks in production

Reviewed the full diff against the actual source and ran the targeted suites locally with vp. No blocking issues. The lazy-load gating for intercepting-route pages is correct and complete across every path that reads intercept.page, and the new probePage fix closes a real gap.

Verified correct

Manifest — intercepting-route pages now emit page: null + __pageLoader (app-rsc-manifest.ts:226-227), registered via getLazyLoaderVar (:191), matching the existing lazy model for static/dynamic pages. Parallel-slot pages (slot.pagePath, :185 / :234) remain eager getImportVar, consistent within the PR's stated scope.
Async propagation — resolveAppPageInterceptState is now async, and its three callers (resolveAppPageInterceptMatch, resolveAppPageInterceptionRerenderTarget, resolveAppPageIntercept) plus the public resolveAppPageActionRerenderTarget all return promises. Every call site awaits: app-server-action-execution.ts:1154, app-page-dispatch.ts:558 (the fixed await in this PR), and app-page-dispatch.ts:718. No un-awaited callers remain.
Mutation safety — findIntercept returns a per-request shallow copy (app-rsc-route-matching.ts:129, { ...entry, ... }), so the intercept.page = await intercept.__pageLoader() mutation (app-page-request.ts:335-336, app-rsc-entry.ts:695-696) never corrupts the shared interceptLookup entry. The generated () => import() loader (app-rsc-manifest.ts:130) is deduped by the ES module system, so even though probe and render each call findIntercept independently and reload, the module evaluates once.
Ordering — toInterceptOpts(intercept) reads intercept.page -> interceptPage (app-rsc-entry.ts:909). In every path the resolveAppPageInterceptState load runs before toInterceptOpts/buildPageElement, so interceptPage is always populated.
probePage fix — buildAppPageProbes reads intercept.page?.default (app-page-probe.ts:381). Before this PR the lazy intercept page was null at probe time, so the dynamic-bailout probe silently inspected undefined. The new resolve-before-probe (app-rsc-entry.ts:687-697) correctly mirrors the render path. Since app-rsc-entry.ts is the shared dev+prod App Router entry, this covers both — no separate dev-server probe path to keep in sync.
Prerender parity — createAppPrerenderStaticParamsResolver loads lazy { load } page sources on demand and drops null fns (app-prerender-static-params.ts:80,83); a missing generateStaticParams -> fns.length === 0 -> resolver returns null, identical to the old eager-filtered behavior.
Rejection caching — ensureAppRouteModulesLoaded clears __loading and leaves __loaded false on rejection (app-route-module-loader.ts:78), so a transient import failure retries on the next request. The earlier bonk reviews that called this "cached forever" were inaccurate; the unit test at app-route-module-loader.test.ts:93 locks the retry behavior in.

Tests run locally (vp), all green

app-page-request + app-prerender-endpoints + entry-templates -> 62 passed
app-page-dispatch + app-server-action-execution + app-rsc-handler -> 110 passed
app-route-module-loader -> 8 passed
app-router-production-server -t "CSS isolated" and -t "intercepted modal CSS" -> both isolation + positive intercept tests pass
vp check on all 7 changed server/entry modules -> clean

Non-blocking notes

Parallel-slot page modules (slot.pagePath, app-rsc-manifest.ts:185) are still eagerly imported, so a non-intercepting parallel-route page importing route-global CSS would still leak into the shared chunk. Out of scope and not exercised by fixtures; worth tracking as a follow-up so the remaining gap stays explicit.
The navigation-trace.ts export downgrade and the dev-error-overlay.spec.ts serial-mode flakiness fix are both unrelated to CSS isolation. Benign, but tangential scope creep.

LGTM.

ask-bonk · 2026-06-08T11:07:31Z

      if (slot.errorPath) imports.getImportVar(slot.errorPath);
      for (const ir of slot.interceptingRoutes) {
-        imports.getImportVar(ir.pagePath);
+        imports.getLazyLoaderVar(ir.pagePath);


Tracking note (non-blocking): this intercepting-route page is now lazy (getLazyLoaderVar), but the parallel-slot page a few lines up (if (slot.pagePath) imports.getImportVar(slot.pagePath);) is still an eager static import. A non-intercepting parallel-route page that imports route-global CSS would still contribute its CSS to the shared manifest chunk — the same class of leak this PR fixes for the main and intercepting pages. Out of scope and not exercised by current fixtures, but worth filing a follow-up so the remaining gap is explicit.

ask-bonk · 2026-06-08T11:07:42Z

Review posted successfully to PR #1738.

Summary

I reviewed PR #1738 against the actual source (not just the diff) and ran the targeted suites locally with vp. No blocking issues — the change is correct.

What I verified

Manifest: intercepting-route pages now emit page: null + __pageLoader (app-rsc-manifest.ts:226-227, registered at :191), matching the existing lazy model. Parallel-slot pages stay eager, consistent with scope.
Async propagation: resolveAppPageInterceptState became async; traced all callers (resolveAppPageInterceptMatch, ...RerenderTarget, resolveAppPageIntercept, resolveAppPageActionRerenderTarget) and every call site awaits. No un-awaited callers.
Mutation safety: findIntercept returns a per-request shallow copy, so intercept.page = await __pageLoader() never corrupts the shared lookup entry; the () => import() loader is deduped by the ES module system.
Ordering: toInterceptOpts reads intercept.page after the load completes in every path.
probePage fix: verified the new resolve-before-probe closes a real gap where the lazy intercept page was null at probe time and the dynamic-bailout probe silently inspected undefined. Since app-rsc-entry.ts is shared dev+prod, no separate path to sync.
Prerender parity and rejection retry both preserved/correct (corrected the prior bonk reviews' inaccurate "cached forever" claim).

Tests run locally (all green)

62 + 110 + 8 unit tests across the changed modules
Both CSS-isolation production tests + the new positive intercept-modal test
vp check clean on all 7 changed modules

Non-blocking notes flagged

Parallel-slot page modules still eagerly imported (same class of leak, out of scope) — flagged inline as a tracking follow-up.
The navigation-trace.ts export downgrade and dev-error-overlay.spec.ts flakiness fix are unrelated scope creep, but benign.

Verdict: LGTM.

github run

Integrate sibling interceptions with the lazy intercept-page loading introduced on main in cloudflare#1738 (CSS-chunk isolation). main made slot intercept pages lazy (manifest emits `page: null` + `__pageLoader`, awaited on demand by resolveAppPageInterceptState / probePage). The auto-merge added `__pageLoader` to the sibling lookup push without the matching type/manifest support, breaking `vp check`. Resolution: make sibling intercept pages lazy too, mirroring slots: - add `__pageLoader?` to AppRscSiblingInterceptForMatching - manifest emits `page: null` + `__pageLoader` (getLazyLoaderVar) for sibling intercepts and allocates a lazy loader instead of an eager import, so the intercepting page's CSS chunk stays isolated in prod

The main merge auto-resolved incorrectly: it moved the `__pageLoader` field that cloudflare#1738 added to the *slot* intercept push block onto the new *sibling* push block instead (the two `interceptLookup.push({...})` calls are textually similar). As a result slot intercept lookup entries lost `__pageLoader`, so resolveAppPageInterceptState never loaded the lazy intercepting page (`page` stayed null) and the modal slot fell back to default.tsx — breaking every slot interception. Restore `__pageLoader: intercept.__pageLoader` on the slot push block. The sibling block already carries it (see merge commit), and both intercept types declare the optional field. Fixes the app-router-dev-server intercept tests and the interception e2e suites that regressed after merging main.

@slot

…1804) * test(interception): add failing sibling-interception graph tests * feat(routing): discover sibling-style interception markers * feat(routing): emit sibling interception manifest facts * feat(app-router): serialize sibling intercepts in route manifest * feat(app-router): match sibling intercepts in route matcher * feat(app-router): render sibling intercept as full page response * test: add siblingIntercepts field to AppRoute test fixtures * test(interception): add sibling interception fixtures * fix(app-router): emit null proof for sibling intercepts to bypass slot-validation * test(interception): add sibling interception e2e specs * fix(interception): correct (...)→(..)(..) marker in interception-from-root fixture * fix(interception): resolve sourceRouteIndex to slot-owner route in intercept lookup When a route inherits a @slot from an ancestor via discoverInheritedParallelSlots, the inherited slot's interceptingRoutes are copied from the ancestor. Previously, createInterceptLookup used routeIndex (the inheriting descendant's index) as sourceRouteIndex, causing resolveAppPageInterceptState to see sourceRoute === currentRoute → kind='current-route' instead of kind='source-route'. Fix: build a patternToIndex map and resolve sourceRouteIndex via the intercept's sourceMatchPattern (which names the actual slot-owner route), falling back to routeIndex only when no match is found. * fix(interception): use locale-prefixed href so middleware interception fires correctly * fix(interception): address codex review issues in sibling intercept implementation - Pass effectiveParams to resolveAppPageHead so generateMetadata/generateViewport sees the intercepted route's params instead of the source route's params - Rename SIBLING_INTERCEPT_SLOT_NAME from __page to __vinext_sibling_intercept to prevent collision with a user @__page parallel route directory - Restore middleware-rewrite fixture link without locale prefix so the interception-dynamic-segment-middleware spec exercises the intended middleware rewrite path * fix(test): update slotId assertions and restore locale-prefixed fixture link - Update test assertions from slot:__page to slot:__vinext_sibling_intercept following the rename in commit 2248b8f - Restore locale-prefixed href in interception-mw fixture so the interception-dynamic-segment-middleware E2E tests pass (reverts the accidental revert from 2248b8f back to the working state from 02e80c2) * fix(interception): address review findings on sibling interception PR - Restore locale-less href in interception-mw fixture and fix the underlying middleware-rewrite interception gap: add a Referer-based fallback in app-rsc-handler so interception fires when middleware rewrites the URL before the server matches it (the client-side manifest check sees the pre-rewrite target and skips setting the interception context header; the Referer carries the source page the server needs) - Apply interceptLayouts for sibling intercepts in buildPageElements so a layout.tsx under the interception marker dir wraps the intercepting page, matching Next.js segment-layout wrapping behaviour; add fixture layout and E2E assertion - Add unit test proving findOwnerRouteForDir ancestor walk attaches a sibling intercept to the nearest ancestor route when the marker's parent dir has no page.tsx (not dropped silently) * fix(interception): send current pathname as fallback interception context The client-side manifest check compares the pre-middleware target URL against declared interception patterns. When middleware rewrites the URL (e.g. /foo/p/1 → /en/foo/p/1), the pre-rewrite URL has fewer segments and doesn't match the pattern, so no X-Vinext-Interception-Context header is sent and the server cannot fire interception. Fix: when the manifest check yields no match and there is no prior intercepted navigation in browser state, fall back to sending the current page pathname as the interception context. This mirrors Next.js sending Next-URL on every RSC navigation. The server's findIntercept is double-gated on both source and target matching, so false positives cannot occur. The previousNextUrl is also populated so back/forward traversal can restore intercepted state. Revert the Referer-based server-side fallback (unreliable across environments) in favour of this explicit browser-side fix. * fix(interception): gate middleware-rewrite fallback on declared source pattern The previous fix sent window.location.pathname as fallback interception context on every unmatched navigation, which broke prefetch cache reuse: a prefetch stored with null context wouldn't be found when the navigation used a non-null context key. Refine the fallback to only fire when the current page URL matches at least one sourcePatternParts declared in the route manifest — i.e., the current page is a known interception source. This preserves prefetch cache reuse for ordinary navigations while still enabling middleware- rewrite interception for pages that declare an interception relationship. * fix(interception): support middleware-rewrite targets in fallback context and planner The previous fallback-context gate only checked whether the current page is a declared interception source; it did not verify the target URL could plausibly be a middleware-rewritten version of the declared target pattern. This meant the client sent interception context when it shouldn't (pre-middleware target had no static anchors in common with the pattern) and the planner hard-navigated when the proof target matched only after the rewrite. Three coordinated fixes: 1. Client fallback gate (app-browser-entry / app-browser-interception-context): replace the broad source-only check with matchRoutePatternWithOptionalDynamicSegments on the target side — the pre-middleware URL must still share every static anchor of the declared target pattern, in order, while allowing dynamic segments to be absent. 2. Planner interception proof (navigation-planner): findRouteManifestInterceptionForProof now accepts a proof whose target matched URL equals the pre-rewrite canonical pathname (shorter than the full pattern) when matchRoutePatternWithOptionalDynamicSegments passes. Exact pattern match still skips the targetRouteId guard; the relaxed path skips it too so the middleware-added locale segment does not invalidate an otherwise valid proof. 3. displayPathname threading (app-rsc-handler / app-rsc-entry / app-page-element-builder): the handler now passes canonicalPathname as displayPathname separately from cleanPathname. buildPageElements uses cleanPathname for all internal routing work (slot param extraction, slot overrides) and displayPathname only for render identity — keeping the browser-visible URL in RSC metadata without affecting internal matching. * fix(interception): guard siblingInterceptElement construction and document reserved slot name * fix(interception): surface missing default export on sibling intercept pages A sibling-style interception whose intercepting page module is missing its `default` export previously fell back to the source route's page component (`effectivePageModule?.default ?? PageComponent`), silently rendering a *different* page than the one requested. Resolve the effective component from the intercepting page only and extend the existing no-default-export guard to cover sibling intercepts, so a missing export surfaces the same explicit "Page has no default export" error the source/slot paths already produce. For a normal request this is identical to the previous behavior since `effectivePageModule === pageModule`. Addresses bonk review feedback on #1804. * test(app-router): guard canonical pathname commit on soft-nav config rewrites The `displayPathname = canonicalPathname` change feeds the RSC payload identity the client planner uses to commit a navigation, so it affects every rewritten route, not just interceptions. Existing coverage (nextjs-compat/hooks.test.ts) only exercises the hard-nav (SSR) path. Add an e2e that soft-navigates (client-side) to an afterFiles-rewritten route and asserts the committed URL and `usePathname()` stay canonical (pre-rewrite), with a marker check proving no full page reload occurred. Addresses bonk review feedback on #1804. * fix(interception): restore __pageLoader on slot intercept lookup entries The main merge auto-resolved incorrectly: it moved the `__pageLoader` field that #1738 added to the *slot* intercept push block onto the new *sibling* push block instead (the two `interceptLookup.push({...})` calls are textually similar). As a result slot intercept lookup entries lost `__pageLoader`, so resolveAppPageInterceptState never loaded the lazy intercepting page (`page` stayed null) and the modal slot fell back to default.tsx — breaking every slot interception. Restore `__pageLoader: intercept.__pageLoader` on the slot push block. The sibling block already carries it (see merge commit), and both intercept types declare the optional field. Fixes the app-router-dev-server intercept tests and the interception e2e suites that regressed after merging main. --------- Co-authored-by: James Anderson <james@eli.cx>

NathanDrake2406 marked this pull request as draft June 4, 2026 14:41

NathanDrake2406 force-pushed the nathan/nav-hash-css-isolation branch from 16feaf7 to 14a7a5e Compare June 4, 2026 15:25

NathanDrake2406 force-pushed the nathan/nav-hash-css-isolation branch from 14a7a5e to a9852d2 Compare June 4, 2026 15:35

NathanDrake2406 marked this pull request as ready for review June 4, 2026 15:45

ask-bonk Bot reviewed Jun 4, 2026

View reviewed changes

ask-bonk Bot reviewed Jun 5, 2026

View reviewed changes

NathanDrake2406 added 2 commits June 6, 2026 01:06

Merge remote-tracking branch 'upstream/main' into nathan/nav-hash-css…

cee366c

…-isolation # Conflicts: # tests/app-router.test.ts

test: stabilize dev overlay HMR recovery

6f380d4

NathanDrake2406 mentioned this pull request Jun 5, 2026

Flaky E2E: dev overlay soft-nav reload canary assertion #1769

Closed

chore: retrigger ci

885fe60

ask-bonk Bot reviewed Jun 5, 2026

View reviewed changes

NathanDrake2406 mentioned this pull request Jun 8, 2026

Track parallel-route page CSS isolation in App Router manifest #1817

Open

NathanDrake2406 added 2 commits June 8, 2026 16:15

Merge remote-tracking branch 'upstream/main' into nathan/nav-hash-css…

7ec1bd6

…-isolation

chore: fix knip check config

f5bfdf3

ask-bonk Bot reviewed Jun 8, 2026

View reviewed changes

james-elicx merged commit 5dd7a34 into cloudflare:main Jun 8, 2026
41 checks passed

github-actions Bot mentioned this pull request Jun 8, 2026

chore: version packages #1823

Merged

NathanDrake2406 deleted the nathan/nav-hash-css-isolation branch June 13, 2026 13:25

NathanDrake2406 mentioned this pull request Jun 17, 2026

fix(app-router): isolate parallel slot page CSS #2095

Open

Conversation

NathanDrake2406 commented Jun 4, 2026

Overview

Why

What changed

References

Uh oh!

pkg-pr-new Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Jun 4, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review: isolate page CSS chunks in production

Correctness — verified

Notes (non-blocking)

Uh oh!

ask-bonk Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jun 4, 2026

Summary

Uh oh!

NathanDrake2406 commented Jun 5, 2026

Uh oh!

NathanDrake2406 commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Re-review: isolate page CSS chunks in production

Verified

Checks I ran locally

Notes (non-blocking, already acknowledged)

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Summary

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review for issues: isolate page CSS chunks in production

Verified correct

Checks I ran

One new observation (non-blocking, scope note)

Uh oh!

ask-bonk Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Summary

What I verified

Tests run locally (all green)

One new observation (non-blocking)

Uh oh!

james-elicx commented Jun 8, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review: isolate page CSS chunks in production

Verified correct

Tests run locally (all green)

Notes for the record (non-blocking)

pkg-pr-new Bot commented Jun 4, 2026 •

edited

Loading

ask-bonk Bot commented Jun 5, 2026 •

edited

Loading