Skip to content

feat(router): promote intercepted preservation through planner #1249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NathanDrake2406 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(router): promote intercepted preservation through planner #1249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1eac958
feat(router): promote intercepted preservation through planner
NathanDrake2406 May 16, 2026
14fdef5
fix(router): normalize intercepted proof paths
NathanDrake2406 May 16, 2026
24221dc
review: address ask-bonk PR review comments
NathanDrake2406 May 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/vinext/src/entries/app-rsc-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,9 @@ export default __createAppRscHandler({
return {
interceptionContext,
interceptLayouts: intercept.interceptLayouts,
interceptSlotId: intercept.slotId,
interceptSlotKey: intercept.slotKey,
interceptSourceMatchedUrl: interceptionContext,
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

interceptPage: intercept.page,
interceptParams: intercept.matchedParams,
};
Expand Down
62 changes: 59 additions & 3 deletions packages/vinext/src/server/app-browser-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ const MAX_VISITED_RESPONSE_CACHE_SIZE = 50;
const VISITED_RESPONSE_CACHE_TTL = 5 * 60_000;
const MAX_TRAVERSAL_CACHE_TTL = 30 * 60_000;
const CLIENT_RSC_COMPATIBILITY_ID = getVinextRscCompatibilityId();
const browserNavigationController = createAppBrowserNavigationController();
const browserNavigationController = createAppBrowserNavigationController({
syncHistoryStatePreviousNextUrl: syncCurrentHistoryStatePreviousNextUrl,
});
const discardedServerActionRefreshScheduler = createDiscardedServerActionRefreshScheduler({
runRefresh() {
clearClientNavigationCaches();
Expand Down Expand Up @@ -234,6 +236,28 @@ function clearClientNavigationCaches(): void {
clearPrefetchState();
}

function syncCurrentHistoryStatePreviousNextUrl(previousNextUrl: string | null): void {
if (readHistoryStatePreviousNextUrl(window.history.state) === previousNextUrl) {
return;
}

const nextHistoryState = createHistoryStateWithPreviousNextUrl(
window.history.state,
previousNextUrl,
);
// First attempt: use replaceHistoryStateWithoutNotify which fires no popstate
// or hashchange events. If the browser accepted the state update (checked via
// readHistoryStatePreviousNextUrl), we're done. The double-read is needed
// because some browsers (notably Safari) can silently coalesce or ignore
// replaceState calls when called in rapid succession (e.g. back-to-back
// navigation commits). The fallback fires only when the state didn't stick.
replaceHistoryStateWithoutNotify(nextHistoryState, "", window.location.href);
Comment on lines +239 to +254
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (readHistoryStatePreviousNextUrl(window.history.state) === previousNextUrl) {
return;
}
window.history.replaceState(nextHistoryState, "", window.location.href);
}

function createActionInitiationSnapshot() {
const routerState = getBrowserRouterState();
return createServerActionInitiationSnapshot({
Expand Down Expand Up @@ -265,17 +289,26 @@ function createNavigationCommitEffect(options: {
}

const targetHref = new URL(href, window.location.origin).href;
stageClientParams(params);
const preserveExistingState = historyUpdateMode === "replace";
const historyState = createHistoryStateWithPreviousNextUrl(
preserveExistingState ? window.history.state : null,
previousNextUrl,
);

let wroteHistoryState = false;
if (historyUpdateMode === "replace" && window.location.href !== targetHref) {
stageClientParams(params);
replaceHistoryStateWithoutNotify(historyState, "", href);
wroteHistoryState = true;
} else if (historyUpdateMode === "push" && window.location.href !== targetHref) {
stageClientParams(params);
pushHistoryStateWithoutNotify(historyState, "", href);
wroteHistoryState = true;
}

if (!wroteHistoryState) {
syncCurrentHistoryStatePreviousNextUrl(previousNextUrl);
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stageClientParams(params);
}

// URL has been updated; the recovery hard-nav target is no longer needed.
Expand Down Expand Up @@ -435,12 +468,30 @@ function getRequestState(
};
}

// Two branches for "navigate":
// 1. previousNextUrl !== null → a committed intercepted navigation set this
// in browser state (requires proof). This is the proven interception path.
// 2. previousNextUrl === null → fall through to legacy DOM-derived context.
// This fires for non-intercepted navigations (direct loads, normal client
// navs) where no proven interception state exists. The legacy path returns
// whatever the current DOM/history context reflects.
switch (navigationKind) {
case "navigate":
case "navigate": {
const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl;
if (currentPreviousNextUrl !== null) {
return {
interceptionContext: resolveInterceptionContextFromPreviousNextUrl(
currentPreviousNextUrl,
__basePath,
),
previousNextUrl: currentPreviousNextUrl,
};
Comment on lines +479 to +488
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return {
interceptionContext: getCurrentInterceptionContext(),
previousNextUrl: getCurrentNextUrl(),
};
}
case "traverse": {
const previousNextUrl = readHistoryStatePreviousNextUrl(window.history.state);
return {
Expand Down Expand Up @@ -500,6 +551,7 @@ function BrowserRoot({
const [treeStateValue, setTreeStateValue] = useState<AppRouterState | Promise<AppRouterState>>({
activeOperation: null,
elements: resolvedElements,
interception: initialMetadata.interception,
interceptionContext: initialMetadata.interceptionContext,
layoutIds: initialMetadata.layoutIds,
layoutFlags: initialMetadata.layoutFlags,
Expand Down Expand Up @@ -842,6 +894,7 @@ function registerServerActionCallback(): void {
// which sends `Next-URL` on action POSTs when the current tree contains
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syncCurrentHistoryStatePreviousNextUrl call before the action POST ensures the server sees the correct interception context in X-Vinext-Interception-Context. This is important because resolveServerActionRequestState reads previousNextUrl to derive the header value.

But there's a timing question: createActionInitiationSnapshot() on line 893 captures routerState.previousNextUrl before this sync call. If the sync changes history state, does the captured snapshot diverge from what's now in history? The snapshot is used for the action POST headers (via resolveServerActionRequestState), and those headers read from previousNextUrl — which came from the snapshot, not from history state directly. So this should be fine since the snapshot and the sync target are derived from the same routerState.previousNextUrl. But a brief comment explaining the ordering invariant (snapshot captures, then history syncs to match) would help.

// an interception route.
const actionInitiation = createActionInitiationSnapshot();
syncCurrentHistoryStatePreviousNextUrl(actionInitiation.routerState.previousNextUrl);
const body = await encodeReply(args, { temporaryReferences });
const { headers } = resolveServerActionRequestState({
actionId: id,
Expand Down Expand Up @@ -1061,6 +1114,9 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
const requestState = getRequestState(navigationKind, currentPrevNextUrl);
const requestInterceptionContext = requestState.interceptionContext;
const requestPreviousNextUrl = requestState.previousNextUrl;
if (navigationKind === "refresh") {
syncCurrentHistoryStatePreviousNextUrl(requestPreviousNextUrl);
}

// Set this navigation as the pending pathname, overwriting any previous.
// Pass navId so only this navigation (or a newer one) can clear it later.
Expand Down
3 changes: 3 additions & 0 deletions packages/vinext/src/server/app-browser-navigation-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type SameUrlServerActionLifecycleOptions = {
type BrowserNavigationControllerDeps = {
commitClientNavigationState?: typeof commitClientNavigationState;
performHardNavigation?: (href: string, mode?: HardNavigationMode) => boolean;
syncHistoryStatePreviousNextUrl?: (previousNextUrl: string | null) => void;
};

type BrowserNavigationController = {
Expand Down Expand Up @@ -190,6 +191,7 @@ export function createAppBrowserNavigationController(
const commitClientNavigationStateImpl =
deps.commitClientNavigationState ?? commitClientNavigationState;
const performHardNavigation = deps.performHardNavigation ?? performHardNavigationWithLoopGuard;
const syncHistoryStatePreviousNextUrl = deps.syncHistoryStatePreviousNextUrl ?? (() => {});

// These are plain module-level variables (inside the controller closure),
// unlike ClientNavigationState which uses Symbol.for to survive multiple
Expand Down Expand Up @@ -624,6 +626,7 @@ export function createAppBrowserNavigationController(

if (latestApproval.approvedCommit) {
dispatchSynchronousVisibleCommit(latestApproval.approvedCommit);
syncHistoryStatePreviousNextUrl(latestApproval.approvedCommit.previousNextUrl);
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
notifyDiscardedServerActionRevalidation(lifecycleOptions);
}
Expand Down
34 changes: 28 additions & 6 deletions packages/vinext/src/server/app-browser-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getMountedSlotIds,
getMountedSlotIdsHeader,
type AppElements,
type AppElementsInterception,
type AppElementsSlotBinding,
type LayoutFlags,
} from "./app-elements.js";
Expand All @@ -29,6 +30,8 @@ import {
type RouteSnapshotV0,
} from "./navigation-planner.js";
import type { ClientNavigationRenderSnapshot } from "vinext/shims/navigation";
import { normalizePathnameForRouteMatch } from "../routing/utils.js";
import { normalizePath } from "./normalize-path.js";

const VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY = "__vinext_previousNextUrl";

Expand Down Expand Up @@ -58,6 +61,7 @@ export type OperationRecord = PendingOperationRecord | CommittedOperationRecord;
export type AppRouterState = {
activeOperation: OperationRecord | null;
elements: AppElements;
interception: AppElementsInterception | null;
interceptionContext: string | null;
layoutFlags: LayoutFlags;
layoutIds: readonly string[];
Expand All @@ -72,6 +76,7 @@ export type AppRouterState = {

export type AppRouterAction = {
elements: AppElements;
interception: AppElementsInterception | null;
interceptionContext: string | null;
layoutFlags: LayoutFlags;
layoutIds: readonly string[];
Expand All @@ -87,6 +92,7 @@ export type AppRouterAction = {

export type PendingNavigationCommit = {
action: AppRouterAction;
interception: AppElementsInterception | null;
interceptionContext: string | null;
previousNextUrl: string | null;
rootLayoutTreePath: string | null;
Expand Down Expand Up @@ -155,6 +161,10 @@ function createOperationRecord(options: {
};
}

function normalizeNavigationSnapshotMatchedUrl(pathname: string): string {
return normalizePath(normalizePathnameForRouteMatch(pathname));
}

export function resolveInterceptionContextFromPreviousNextUrl(
previousNextUrl: string | null,
basePath: string = "",
Expand Down Expand Up @@ -284,13 +294,17 @@ function createMountedParallelSlotSnapshots(

function createVisibleRouteSnapshot(state: AppRouterState): RouteSnapshotV0 {
const displayUrl = createNavigationSnapshotUrl(state.navigationSnapshot);
const matchedUrl = normalizeNavigationSnapshotMatchedUrl(state.navigationSnapshot.pathname);
return {
displayUrl,
interception: state.interception,
interceptionContext: state.interceptionContext,
layoutIds: state.layoutIds,
// `displayUrl` preserves the browser-visible query string for decisions and
// traces. `matchedUrl` stays path-only because route matching has already
// consumed query params before AppElements metadata reaches this boundary.
matchedUrl: state.navigationSnapshot.pathname,
// `displayUrl` preserves the browser-visible URL for decisions and traces.
// `matchedUrl` uses the route-state canonical pathname, matching the
// server's segment-decoded representation without changing user-facing
// navigation state such as usePathname().
matchedUrl,
mountedParallelSlots: createMountedParallelSlotSnapshots(state.elements),
rootBoundaryId: state.rootLayoutTreePath,
routeId: state.routeId,
Expand All @@ -300,12 +314,17 @@ function createVisibleRouteSnapshot(state: AppRouterState): RouteSnapshotV0 {

function createPendingRouteSnapshot(pending: PendingNavigationCommit): RouteSnapshotV0 {
const displayUrl = createNavigationSnapshotUrl(pending.action.navigationSnapshot);
const matchedUrl = normalizeNavigationSnapshotMatchedUrl(
pending.action.navigationSnapshot.pathname,
);
return {
displayUrl,
interception: pending.action.interception,
interceptionContext: pending.action.interceptionContext,
layoutIds: pending.action.layoutIds,
// See createVisibleRouteSnapshot: matchedUrl intentionally models the route
// identity, not the address bar URL.
matchedUrl: pending.action.navigationSnapshot.pathname,
matchedUrl,
mountedParallelSlots: createMountedParallelSlotSnapshots(pending.action.elements),
rootBoundaryId: pending.rootLayoutTreePath,
routeId: pending.routeId,
Expand Down Expand Up @@ -409,14 +428,16 @@ export async function createPendingNavigationCommit(options: {
}): Promise<PendingNavigationCommit> {
const elements = await options.nextElements;
const metadata = AppElementsWire.readMetadata(elements);
const previousNextUrl =
const requestedPreviousNextUrl =
options.previousNextUrl !== undefined
? options.previousNextUrl
: options.currentState.previousNextUrl;
const previousNextUrl = metadata.interception === null ? null : requestedPreviousNextUrl;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
action: {
elements,
interception: metadata.interception,
interceptionContext: metadata.interceptionContext,
layoutIds: metadata.layoutIds,
layoutFlags: metadata.layoutFlags,
Expand All @@ -434,6 +455,7 @@ export async function createPendingNavigationCommit(options: {
type: options.type,
},
// Convenience aliases — always equal action.interceptionContext / action.rootLayoutTreePath / action.routeId.
interception: metadata.interception,
interceptionContext: metadata.interceptionContext,
previousNextUrl,
rootLayoutTreePath: metadata.rootLayoutTreePath,
Expand Down
4 changes: 4 additions & 0 deletions packages/vinext/src/server/app-browser-visible-commit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export type ApprovedVisibleCommit = {
readonly [approvedVisibleCommitBrand]: true;
readonly action: AppRouterAction;
readonly decision: VisibleCommitDecision;
readonly interception: AppRouterAction["interception"];
readonly interceptionContext: string | null;
readonly previousNextUrl: string | null;
readonly rootLayoutTreePath: string | null;
Expand Down Expand Up @@ -156,6 +157,7 @@ function reduceApprovedVisibleCommitState(
preserveElementIds: commit.decision.preserveElementIds,
preservePreviousSlotIds: commit.decision.preservePreviousSlotIds,
}),
interception: action.interception,
interceptionContext: action.interceptionContext,
layoutFlags: mergeLayoutFlags(
state.layoutFlags,
Expand All @@ -182,6 +184,7 @@ function reduceApprovedVisibleCommitState(
state,
{
elements: action.elements,
interception: action.interception,
interceptionContext: action.interceptionContext,
layoutFlags: action.layoutFlags,
layoutIds: action.layoutIds,
Expand Down Expand Up @@ -266,6 +269,7 @@ function createApprovedVisibleCommit(options: {
[approvedVisibleCommitBrand]: true,
action: options.pending.action,
decision: options.decision,
interception: options.pending.interception,
interceptionContext: options.pending.interceptionContext,
previousNextUrl: options.pending.previousNextUrl,
rootLayoutTreePath: options.pending.rootLayoutTreePath,
Expand Down
Loading
Loading