feat(router): promote intercepted preservation through planner#1249
feat(router): promote intercepted preservation through planner#1249NathanDrake2406 wants to merge 3 commits into
Conversation
Intercepted navigation could still preserve source UI through context-bearing payload shape and same-URL history state. That made modal preservation depend on transport metadata instead of the visible route world that had actually been committed. The planner now requires explicit interception proof before preserving source layouts or unrelated slots, and the browser lifecycle clears stale previous-next history state when normal target payloads commit. Wire metadata carries proof, while missing, stale, or incompatible proof falls back to hard navigation. Tests cover approved source-slot preservation, missing, stale, and malformed proof rejection, current-context refresh and action handling, traverse restoration, and direct-target refresh clearing stale interception state.
commit: |
|
@codex review |
![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: 1eac9588db
ℹ️ 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".
|
|
||
| return { | ||
| sourceMatchedUrl, | ||
| sourceRouteId: AppElementsWire.encodeRouteId(sourceMatchedUrl, null), |
There was a problem hiding this comment.
createAppPageInterceptionProof builds sourceRouteId/targetRouteId directly from interceptSourceMatchedUrl and routePath, but those two inputs are not canonicalized to the same representation used by the committed route snapshot IDs. In particular, browser-derived paths can remain percent-encoded while request normalization decodes segments, so intercepted navigations on encoded/non-ASCII URLs (for example /caf%C3%A9) can produce proof IDs that fail validateInterceptedPreservation’s source/target ID checks and get downgraded to a hard navigation instead of preserving the intercepted UI.
Useful? React with 👍 / 👎.
Interception proof could be built from browser-derived percent-encoded paths while committed route snapshots used the server-normalized route-state form. Non-ASCII intercepted source or target paths could then fail proof validation and fall back to a hard navigation. Normalize proof matched URLs before encoding source and target route IDs, and normalize client planner snapshot matched URLs to the same route-state representation without changing the user-facing navigation snapshot. Adds regressions for encoded non-ASCII proof generation and planner validation.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Core-15 intercepted preservation through planner
I read #726 and reviewed this PR in the context of that roadmap. The core architectural move — promoting interception preservation decisions from implicit wire-shape inference to explicit planner authority with proof validation — is sound and well-executed. The invariant that "no proof means no reuse, no skip, no visible commit" is clearly enforced across the planner, wire codec, and lifecycle boundaries.
The test coverage is thorough: hostile planner tests for missing proof, stale source, inactive slot; E2E tests for chained intercept refresh and stale history clearing; and the URL-normalization round-trip test addressing the Codex reviewer's encoding concern. The findIntercept behavioral tightening (requiring source match) is a correctness improvement.
A few observations below, mostly minor, one medium-priority.
| if (proof.targetMatchedUrl !== options.targetSnapshot.matchedUrl) { | ||
| return { | ||
| kind: "rejected", | ||
| reasonCode: NavigationTraceReasonCodes.interceptedRejectedUnknownSource, |
There was a problem hiding this comment.
Nit: when proof.targetMatchedUrl !== targetSnapshot.matchedUrl, the rejection reason is interceptedRejectedUnknownSource. This is slightly misleading — the target didn't match, not the source. Consider a dedicated interceptedRejectedTargetMismatch reason code, or at minimum a comment explaining why the source-rejection code is reused here.
Not blocking, but it would make planner traces easier to debug when the target URL diverges (e.g., from an encoding mismatch).
| function syncCurrentHistoryStatePreviousNextUrl(previousNextUrl: string | null): void { | ||
| if (readHistoryStatePreviousNextUrl(window.history.state) === previousNextUrl) { | ||
| return; | ||
| } | ||
|
|
||
| const nextHistoryState = createHistoryStateWithPreviousNextUrl( | ||
| window.history.state, | ||
| previousNextUrl, | ||
| ); | ||
| replaceHistoryStateWithoutNotify(nextHistoryState, "", window.location.href); |
There was a problem hiding this comment.
The double-attempt pattern here (try replaceHistoryStateWithoutNotify, re-check, then fall back to raw window.history.replaceState) is defensive but undocumented. Under what circumstances does replaceHistoryStateWithoutNotify succeed but readHistoryStatePreviousNextUrl still return the wrong value? A brief comment explaining when the fallback fires would help future readers understand whether this is guarding against a browser quirk, a race with popstate listeners, or something else.
| case "navigate": { | ||
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | ||
| if (currentPreviousNextUrl !== null) { | ||
| return { | ||
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | ||
| currentPreviousNextUrl, | ||
| __basePath, | ||
| ), | ||
| previousNextUrl: currentPreviousNextUrl, | ||
| }; |
There was a problem hiding this comment.
In the "navigate" case, when currentPreviousNextUrl !== null, the code takes the new early-return path using resolveInterceptionContextFromPreviousNextUrl. When currentPreviousNextUrl === null, it falls through to the legacy getCurrentInterceptionContext() / getCurrentNextUrl() path.
Is the legacy fallback still reachable in any production scenario after this PR? If the only way to reach previousNextUrl !== null is through a committed intercepted navigation (which now requires proof), and all non-intercepted commits clear previousNextUrl to null, then the fallback always fires for non-intercepted navigations. If that's the case, a comment clarifying the two branches (proven interception state vs. legacy DOM-derived context) would make the intent clearer.
|
|
||
| const targetSnapshot = options.event.result.targetSnapshot; | ||
| const hasInterceptedPayload = | ||
| targetSnapshot.interception !== null || targetSnapshot.interceptionContext !== null; |
There was a problem hiding this comment.
The hasInterceptedPayload check is targetSnapshot.interception !== null || targetSnapshot.interceptionContext !== null. This means a payload with only legacy interceptionContext (no proof interception) still enters the interception validation path, where it immediately gets rejected with interceptedRejectedMissingProof → hard navigation.
This is intentional (fencing legacy context-only payloads), but it's a behavioral change for any existing payloads that carried interceptionContext without the new __interception metadata — for example, cached RSC responses from before this deploy. Is there a cache invalidation story for this? If a stale cached RSC response has interceptionContext but no __interception, it will trigger a hard navigation instead of falling through to the normal root-boundary path.
If artifact compatibility or cache busting already covers this, a comment here explaining the migration safety would help.
| interceptParams: intercept.matchedParams, | ||
| interceptSlotId: intercept.slotId ?? null, | ||
| interceptSlotKey: intercept.slotKey, | ||
| interceptSourceMatchedUrl: interceptionContext, |
There was a problem hiding this comment.
Both here and in the generated entry (app-rsc-entry.ts:737), interceptSourceMatchedUrl is set to interceptionContext, which is the raw header value from X-Vinext-Interception-Context. The normalization happens downstream in createAppPageInterceptionProof via normalizeInterceptionProofMatchedUrl.
This is fine because the normalization is defensive, but it's worth noting that interceptionContext is the browser-reported source pathname (possibly percent-encoded), while the proof's sourceMatchedUrl is the normalized form. The Codex reviewer flagged exactly this concern, and the test at app-page-element-builder.test.ts:"normalizes encoded interception proof paths" covers it. Just confirming this is addressed.
| options.previousNextUrl !== undefined | ||
| ? options.previousNextUrl | ||
| : options.currentState.previousNextUrl; | ||
| const previousNextUrl = metadata.interception === null ? null : requestedPreviousNextUrl; |
There was a problem hiding this comment.
This is the key line that implements the clearing invariant: when metadata.interception === null (non-intercepted payload), previousNextUrl is forced to null regardless of the requested value. This ensures direct-load and refresh-to-normal-target commits clear stale interception history context.
Clean and correct. The behavioral change is well-tested (the existing test expectation changed from "/feed" to null at line 1102 of the test file).
| function isInterceptionMetadataValue(value: unknown): value is AppElementsInterception { | ||
| if (typeof value !== "object" || value === null || Array.isArray(value)) return false; | ||
| return ( | ||
| "sourceMatchedUrl" in value && | ||
| "sourceRouteId" in value && | ||
| "slotId" in value && | ||
| "targetMatchedUrl" in value && | ||
| "targetRouteId" in value |
There was a problem hiding this comment.
The isInterceptionMetadataValue type guard uses duck-typing (checking for presence of 5 known fields). This is fine for the transport boundary, but it could false-positive on a user-created React element props object that happens to have these field names. The risk is low since these are internal wire keys, but you might consider adding a discriminant field (e.g., __kind: "interception") or at minimum checking that the values are strings, not just present. The wire-side parser (parseInterceptionMetadata) already does strict type checking, so this is a defense-in-depth suggestion for the slot rendering path.
| const slotBindings = normalizeAppElementsSlotBindings(input.slotBindings, { layoutIds }); | ||
| return { | ||
| ...entries, | ||
| ...(input.interception ? { [APP_INTERCEPTION_KEY]: input.interception } : {}), |
There was a problem hiding this comment.
Minor: the __interception metadata is only included when slotBindings are present (inside the if block) or when there are no slot bindings (the else branch). Both branches handle it, but the pattern is slightly asymmetric — the slot-bindings branch spreads interception conditionally, while the no-slot-bindings branch returns a ternary.
This works but is a bit fragile under future edits. Consider extracting the interception attachment into a shared step after the slot-bindings branch, so there's one place to reason about when __interception appears in the metadata entries.
|
Review posted. Here's a summary of the feedback: Overall: The PR is architecturally sound. Promoting interception preservation from implicit wire-shape inference to explicit planner authority with proof validation is the right move. Test coverage is thorough. 8 inline comments posted:
|
NathanDrake2406
force-pushed
the
nathan/core-15-intercepted-preservation
branch
from
ad91766 to
7ad5832
Compare
- Add interceptedRejectedTargetMismatch reason code for target URL mismatch in planner traces (was misusing UnknownSource) - Document double-attempt replaceState fallback for browser quirks - Clarify navigate branches: proven interception vs legacy fallback - Document cache migration safety for legacy interceptionContext - Tighten isInterceptionMetadataValue type guard with string checks - Extract interception attachment into shared step in wire metadata entries to remove asymmetric branching
NathanDrake2406
force-pushed
the
nathan/core-15-intercepted-preservation
branch
from
7ad5832 to
24221dc
Compare
2 participants
Overview
#726-CORE-15from #726 by promoting intercepted route preservation into the planner and lifecycle path.navigation-planner.ts,app-browser-state.ts,app-browser-entry.ts,app-elements-wire.ts, app page dispatch/build wiring, and the intercepted route tests.Bonk reviewer note: please read #726 before reviewing this PR so the Core-15 slice is evaluated in the architectural big picture.
Why
This implements
#726-CORE-15by promoting intercepted route preservation through the planner/lifecycle path. The invariant is that route state and lifecycle authority decide what previous UI may remain visible. Context suffixes, wire-key suffixes, mounted-slot headers, cache partitioning, and missing payload entries are evidence only.What Changed
Maintainer review path
packages/vinext/src/server/navigation-planner.tsfor the new Core-15 authority boundary and rejection reasons.packages/vinext/src/server/app-elements-wire.tsfor the proof metadata boundary and malformed proof fencing.packages/vinext/src/server/app-page-element-builder.ts,app-page-dispatch.ts, andapp-rsc-route-matching.tsfor how server route facts become proof evidence.packages/vinext/src/server/app-browser-state.ts,app-browser-visible-commit.ts,app-browser-entry.ts, andapp-browser-navigation-controller.tsfor lifecycle and same-URL current-context handling.tests/navigation-planner.test.ts,tests/app-browser-entry.test.ts,tests/app-elements.test.ts, andtests/e2e/app-router/advanced.spec.tsfor the hostile cases.Validation
Ran:
The Playwright command selected the app-router advanced project and completed with
29 passed, 1 skipped.Risk / compatibility
__interceptionmetadata and validates it strictly when present. Legacy context-only payloads no longer authorize preservation.Non-goals
References
#726-CORE-15