feat(router): promote intercepted preservation through planner#1249
feat(router): promote intercepted preservation through planner#1249NathanDrake2406 wants to merge 4 commits into
Conversation
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 👍 / 👎.
|
/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
2 times, most recently
from
7ad5832 to
24221dc
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: Core-15 intercepted preservation through planner
I reviewed this against the #726 roadmap. The architectural direction is correct: promoting interception preservation from implicit wire-shape inference to explicit planner authority with proof validation. The "no proof → no reuse → no visible commit" invariant is consistently enforced across planner, wire codec, state management, and lifecycle boundaries.
The test coverage is strong across all layers — hostile planner tests (missing proof, stale source, inactive slot, incompatible root), URL normalization round-trips, the findIntercept source-required behavioral tightening, and the E2E refresh/direct-target scenarios.
A few observations below, ranging from minor naming nits to one medium-severity concern about the interception entry path.
| // hard navigation. This fences legacy cached RSC payloads from before this | ||
| // deploy. The cache is busted by the deploy (different build), so no stale | ||
| // artifact should persist across deploys. | ||
| const hasInterceptedPayload = |
There was a problem hiding this comment.
Medium: interceptionContext !== null alone is too broad of a trigger for the interception validation path.
The comment explains that legacy cached RSC payloads are expected to be cache-busted by deploy, but the condition targetSnapshot.interceptionContext !== null enters the interception validation path for any payload that has a non-null interceptionContext — including payloads for non-intercepted routes that merely passed through a request with an interception context header.
Consider: a normal refresh on /feed while an intercepted modal was open sends X-Vinext-Interception-Context: /feed. The server renders the /feed route (not an intercepted route), but the response carries interceptionContext: "/feed" in its metadata because the header was present. This payload has no __interception proof (correct — it's not an intercepted render), but it also has interceptionContext !== null. Under this logic, it enters validateInterceptedPreservation, gets interceptedRejectedMissingProof, and triggers a hard navigation.
Is this scenario prevented upstream (e.g., the server only sets interceptionContext on actually-intercepted responses)? If so, a comment here clarifying that invariant would help. If not, this could cause unexpected hard navigations on refresh/action while an intercepted modal is open.
The previous bonk review flagged this as a cache-migration concern, but I think the more immediate risk is the runtime scenario above.
| if (proof.targetMatchedUrl !== options.targetSnapshot.matchedUrl) { | ||
| return { | ||
| kind: "rejected", | ||
| reasonCode: NavigationTraceReasonCodes.interceptedRejectedTargetMismatch, |
There was a problem hiding this comment.
Minor: The previous review noted this already — the interceptedRejectedTargetMismatch reason code was added in navigation-trace.ts (line 11: NC_INTERCEPT_REJECT_TARGET) but this line uses interceptedRejectedTargetMismatch correctly now. Good fix from the earlier feedback.
| 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 invariant: metadata.interception === null → previousNextUrl = null. Clean and well-placed. The behavioral change (refresh/action payloads for non-intercepted routes clear stale interception history) is correctly tested at line 1102 of the test file where the expectation changed from "/feed" to null.
One subtlety worth noting: this means any code path that creates a PendingNavigationCommit with previousNextUrl set will have it silently cleared if the server response doesn't carry __interception. That's the right behavior, but it's a non-obvious contract — the caller's requested previousNextUrl is advisory, not authoritative. Consider adding a brief doc comment on the previousNextUrl option in createPendingNavigationCommit noting this.
| return trieMatch(routeTrie, appRscPathnameParts(url)); | ||
| }, | ||
| findIntercept(pathname, sourcePathname = null) { | ||
| if (sourcePathname === null) return null; |
There was a problem hiding this comment.
Good tightening. Making findIntercept return null when sourcePathname === null is correct — an interception without a proven source route is not an interception. The test at app-rsc-route-matching.test.ts:141 (findIntercept("/photos/42", null)) covers this.
The previous behavior (proceeding with empty source params when source was null) could silently produce intercept matches with incomplete param merges. This is a correctness improvement.
| } | ||
|
|
||
| if (!wroteHistoryState) { | ||
| syncCurrentHistoryStatePreviousNextUrl(previousNextUrl); |
There was a problem hiding this comment.
Subtle edge case: when neither replace nor push wrote history state (i.e., the URL didn't change — same-URL navigation), the code falls through to syncCurrentHistoryStatePreviousNextUrl followed by stageClientParams. This correctly handles the case where an intercepted navigation resolves to the same URL but with different interception state (e.g., opening a modal from the same page).
The ordering matters: syncCurrentHistoryStatePreviousNextUrl must run before stageClientParams because stageClientParams may trigger React renders that read history state. Good.
| // Empty slot binding metadata is intentionally omitted. Missing | ||
| // __slotBindings round-trips as [] and means "no route-state proof", so | ||
| // default/unmatched slot preservation is not promoted for that payload. | ||
| const interceptionEntry = input.interception |
There was a problem hiding this comment.
The previous review noted the asymmetric __interception attachment across the two branches. I agree it's slightly fragile but also understand the reasoning: the interceptionEntry spread is computed once and applied in both branches, which is actually reasonably clean.
One potential improvement: the empty-object spread ...interceptionEntry when input.interception is null/undefined produces ...{}, which is a no-op but allocates an empty object per call. Since createMetadataEntries is called on every render, you could avoid this by conditionally spreading only when non-empty:
| const interceptionEntry = input.interception | |
| const interceptionEntry = input.interception | |
| ? { [APP_INTERCEPTION_KEY]: input.interception } | |
| : null; |
Then ...(interceptionEntry ?? {}) or conditional assignment. Not blocking — this is a micro-optimization.
| ); | ||
| const targetMatchedUrl = normalizeInterceptionProofMatchedUrl(routePath); | ||
| const slotId = opts?.interceptSlotId ?? null; | ||
| if (sourceMatchedUrl === null || targetMatchedUrl === null || slotId === null) return null; |
There was a problem hiding this comment.
Good defensive design: if any of the three required proof inputs (sourceMatchedUrl, targetMatchedUrl, slotId) is null, the proof is simply omitted rather than producing a partial/invalid proof object. This ensures the planner never sees malformed proof — it either gets a complete proof or null.
|
|
||
| if (latestApproval.approvedCommit) { | ||
| dispatchSynchronousVisibleCommit(latestApproval.approvedCommit); | ||
| syncHistoryStatePreviousNextUrl(latestApproval.approvedCommit.previousNextUrl); |
There was a problem hiding this comment.
This syncHistoryStatePreviousNextUrl after server-action commit is important for maintaining the invariant that history state and committed router state agree on previousNextUrl. Without it, a server action that revalidates an intercepted page could leave stale previousNextUrl in history while the router state has already been updated.
Question: should the sync also happen on the hard-navigate branch (line 623)? If performHardNavigation is called, the page is about to reload, so syncing history is probably moot. But if performHardNavigation fails or is deferred (e.g., loop guard), the history state could be inconsistent. Low risk, but worth considering.
| return Array.isArray(value) && value.length > 0 && value.every(isSlotBindingValue); | ||
| } | ||
|
|
||
| function isInterceptionMetadataValue(value: unknown): value is AppElementsInterception { |
There was a problem hiding this comment.
The previous review suggested adding value-type checks beyond in presence checks. I see this version already checks typeof value.sourceMatchedUrl === 'string' etc., which addresses that feedback. The guard is solid for the transport boundary.
The false-positive risk from user-created objects with all five field names as strings is extremely low given these are internal wire keys (sourceMatchedUrl, sourceRouteId, slotId, targetMatchedUrl, targetRouteId). Acceptable.
| interceptLayouts: intercept.interceptLayouts, | ||
| interceptSlotId: intercept.slotId, | ||
| interceptSlotKey: intercept.slotKey, | ||
| interceptSourceMatchedUrl: interceptionContext, |
There was a problem hiding this comment.
Note: interceptSourceMatchedUrl: interceptionContext means the source URL for proof comes from the raw X-Vinext-Interception-Context header value. The normalization happens downstream in createAppPageInterceptionProof via normalizeInterceptionProofMatchedUrl. This is the same pattern the Codex reviewer flagged and the test at app-page-element-builder.test.ts covers. Correct as-is.
|
Review posted successfully. Here's a summary: Review SummaryOverall: The PR is architecturally sound. Promoting interception preservation from implicit wire-shape inference to explicit planner authority with proof validation is the right move for Core-15 of the #726 roadmap. 11 inline comments posted:
|
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.
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.
- 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
Context-only AppElements payloads could enter intercepted preservation planning or leave context-suffixed route identity as later proof authority. That made transport metadata influence visible-world preservation when a normal response was rendered from an intercepted request context. Only explicit interception proof now enters the preservation branch. Planner snapshots also canonicalize context-suffixed route IDs when no proof is present, keeping render keys partitioned while preventing stale context from poisoning future explicit interception proof. Tests cover the direct planner decision and the two-step browser lifecycle regression.
NathanDrake2406
force-pushed
the
nathan/core-15-intercepted-preservation
branch
from
24221dc to
b84597e
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