feat: encode interception context in App Router payload IDs and caches#753
Draft
NathanDrake2406 wants to merge 24 commits intocloudflare:mainfrom
Draft
Conversation
- Fix stale closure on readBrowserRouterState by using a useRef updated synchronously during render instead of a closure captured in useLayoutEffect. External callers (navigate, server actions, HMR) now always read the current router state. - Restore GlobalErrorBoundary wrapping that was dropped when switching from buildPageElement to buildAppPageElements. Apps with app/global-error.tsx now get their global error boundary back. - Add exhaustive default case to routerReducer so new action types produce a compile error and a runtime throw instead of silent undefined. - Remove dead code: createRouteNodeSnapshot, AppRouteNodeSnapshot, AppRouteNodeValue were defined but never imported. - Remove deprecated buildAppPageRouteElement and its test — no production callers remain after the flat payload cutover. - Short-circuit normalizeAppElements when no slot keys need rewriting to avoid unnecessary allocation on every payload. - Align test data in error boundary RSC payload test (matchedParams slug: "post" -> "missing" to match requestUrl /posts/missing).
createFromReadableStream() returns a React thenable whose .then() returns undefined (not a Promise). Chaining .then(normalizeAppElements) broke SSR by assigning undefined to flightRoot. Fix: call use() on the raw thenable, then normalize synchronously after resolution. Also widen renderAppPageLifecycle element type to accept flat map payloads.
The SSR entry always expects a flat Record<string, ReactNode> with __route and __rootLayout metadata from the RSC stream. Three paths were still producing bare ReactNode payloads: 1. renderAppPageBoundaryElementResponse only created the flat map for isRscRequest=true, but HTML requests also flow through RSC→SSR 2. buildPageElements "no default export" early return 3. Server action "Page not found" fallback All three now produce the flat keyed element map, fixing 17 test failures across 404/not-found, forbidden/unauthorized, error boundary, production build, rewrite, and encoded-slash paths.
- Update renderElementToStream mock to extract the route element from the flat map before rendering to HTML (mirrors real SSR entry flow) - Update entry template snapshots for the buildPageElements changes
createFromReadableStream() returns a React Flight thenable whose .then() returns undefined instead of a new Promise. The browser entry's normalizeAppElementsPromise chained .then() on this raw thenable, producing undefined — which crashed use() during hydration with "An unsupported type was passed to use(): undefined". Wrapping in Promise.resolve() first converts the Flight thenable into a real Promise, making .then() chains work correctly. The same fix was already applied to the SSR entry in 5395efc but was missed in the browser entry.
React 19.2.4's use(Promise) during hydration triggers "async Client Component" because native Promises lack React's internal .status property (set only by Flight thenables). When use() encounters a Promise without .status, it suspends — which React interprets as the component being async, causing a fatal error. Fix: store resolved AppElements directly in ElementsContext and router state instead of Promise<AppElements>. The navigation async flow (createPendingNavigationCommit) awaits the Promise before dispatching, so React state never holds a Promise. - ElementsContext: Promise<AppElements> → AppElements - AppRouterState.elements: Promise<AppElements> → AppElements - mergeElementsPromise → mergeElements (sync object spread) - Slot: useContext only, no use(Promise) - SSR entry: pass resolved elements to context - dispatchBrowserTree: simplified, no async error handler Also fix flaky instrumentation E2E test that read the last error entry instead of finding by path.
- Remove Promise wrappers from ElementsContext test values - mergeElementsPromise → mergeElements (sync) - Replace Suspense streaming test with direct render test - Remove unused createDeferred helper and Suspense import - Update browser state test assertions (no longer async)
P1a: mergeElements preserves previous slot content when the new payload marks a parallel slot as unmatched. On soft navigation, unmatched slots keep their previous subtree instead of triggering notFound(). P1b: renderNavigationPayload now receives navId and checks for superseded navigations after its await. Stale payloads are discarded instead of being dispatched into the React tree. P2: The catch block in renderNavigationPayload only calls commitClientNavigationState() when activateNavigationSnapshot() was actually reached, preventing counter underflow. P3: The no-default-export fallback in buildPageElements now derives the root layout tree path from route.layoutTreePositions and route.routeSegments instead of hardcoding "/".
Prove the flat keyed map architecture works end-to-end: - Layout state persists across sibling navigation (counter survives) - Template remounts on segment boundary change, persists within segment - Error boundary clears on navigate-away-and-back - Back/forward preserves layout state through history - Parallel slots persist on soft nav, show default.tsx on hard nav Zero production code changes — test fixtures and Playwright specs only.
commit: |
![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: e1f1be6cbc
ℹ️ 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".
Comment on lines
+320
to
+321
| case "navigate": | ||
| return getCurrentInterceptionContext(); |
There was a problem hiding this comment.
When navigationKind === "navigate", this always derives the context from window.location.pathname, which drops the original source context after the first intercepted transition. For example, after opening /photos/42 as a modal from /feed, subsequent in-app navigations from that modal send X-Vinext-Interception-Context: /photos/42 instead of /feed; the server then resolves the direct route and the modal flow breaks into a full-page render. The code already stores interception context in history state during commit, so using pathname here causes a real regression for chained intercepted navigations.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of #726 (PR 4). This adds the interception-context identity that PR 2c intentionally deferred. Intercepted App Router navigations now carry their source route through the payload IDs, prefetch cache, visited-response cache, and history state, so
/feed -> /photos/42modal navigations stop aliasing direct/photos/42page loads.The practical effect is that intercepted and direct renders of the same visible pathname no longer stomp each other. Repeated feed/gallery modal navigations reuse the right cached payload, back/forward restores the right source context, and the ordinary visited-response cache still works for non-intercepted navigations.
What changed
__interceptionContextwhen an intercepted render is active, and route/page IDs are encoded with that context instead of keying only by pathnameapp-browser-entryandapp-browser-statenow carry committed interception context alongsideelements,routeId, androotLayoutTreePath, and write it into history state so traversal can recover the original source route/photos/42.rscfrom/feedand/gallerydo not alias each other. Direct-route payloads still omit interception metadata, so visited-response storage preserves the request-side cache partition when the payload does not provide onenext/linkprefetch and App Router navigations sendX-Vinext-Interception-Contextwhen the current render is inside an intercepted flow, and the server reads that header when building intercepted payloadsgallerysource route plus targeted integration/E2E coverage for direct-vs-intercepted cache separation, repeated intercepted navigations, refresh behavior, and interception-aware prefetch entriesWhat this does NOT do
Server action / refresh parity for intercepted routes is still deferred. This PR intentionally leaves action re-renders on the direct-route path and keeps the scope to payload encoding + browser-side cache/history identity.
Test plan
vp test run tests/app-elements.test.ts tests/app-browser-entry.test.ts tests/prefetch-cache.test.tsvp test run tests/app-router.test.ts -t "renders intercepted photo modal on RSC navigation from feed"vp run test:e2e -- tests/e2e/app-router/advanced.spec.ts --project app-routergit commitpre-commit hook (vp check --fix)