Skip to content

Implement segment-scoped useRouter bfcacheId semantics #1197

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
manNomi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Implement segment-scoped useRouter bfcacheId semantics #1197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02136a5
feat(app-router): implement useRouter bfcacheId semantics
manNomi May 14, 2026
361e6e6
fix(app-router): refine bfcacheId navigation behavior
manNomi May 14, 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
33 changes: 28 additions & 5 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 @@ -16,6 +16,7 @@ import {
commitClientNavigationState,
consumePrefetchResponse,
createClientNavigationRenderSnapshot,
getBfcacheIdMapContext,
getCurrentNextUrl,
getCurrentInterceptionContext,
getClientNavigationRenderContext,
Expand Down Expand Up @@ -62,6 +63,8 @@ import {
} from "./app-elements.js";
import {
createHistoryStateWithPreviousNextUrl,
createInitialBfcacheIdMap,
readHistoryStateBfcacheIds,
readHistoryStatePreviousNextUrl,
resolveInterceptionContextFromPreviousNextUrl,
resolveServerActionRequestState,
Expand Down Expand Up @@ -217,13 +220,14 @@ function clearClientNavigationCaches(): void {
}

function createNavigationCommitEffect(options: {
bfcacheIds: Readonly<Record<string, string>>;
href: string;
historyUpdateMode: HistoryUpdateMode | undefined;
navId: number;
params: Record<string, string | string[]>;
previousNextUrl: string | null;
}): () => void {
const { href, historyUpdateMode, navId, params, previousNextUrl } = options;
const { bfcacheIds, href, historyUpdateMode, navId, params, previousNextUrl } = options;

return () => {
// Only update URL if this is still the active navigation.
Expand All @@ -241,12 +245,15 @@ function createNavigationCommitEffect(options: {
const historyState = createHistoryStateWithPreviousNextUrl(
preserveExistingState ? window.history.state : null,
previousNextUrl,
bfcacheIds,
);

if (historyUpdateMode === "replace" && window.location.href !== targetHref) {
replaceHistoryStateWithoutNotify(historyState, "", href);
} else if (historyUpdateMode === "push" && window.location.href !== targetHref) {
pushHistoryStateWithoutNotify(historyState, "", href);
} else if (historyUpdateMode === undefined) {
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

This new else if arm fires for traversals (historyUpdateMode === undefined) to persist the updated bfcacheIds into history.state even when the URL hasn't changed. Makes sense — but it also fires for refresh navigations (which also pass undefined for historyUpdateMode).

For refresh, this replaceState is harmless (same URL, same-ish state), but it's a behavioral change from before this PR where refresh commits didn't touch history state at all. Worth a comment clarifying that this is intentional for both traverse and refresh cases.

replaceHistoryStateWithoutNotify(historyState, "", window.location.href);
}

// URL has been updated; the recovery hard-nav target is no longer needed.
Expand All @@ -266,6 +273,7 @@ async function renderNavigationPayload(
pendingRouterState: PendingBrowserRouterState | null,
actionType: "navigate" | "replace" | "traverse" = "navigate",
operationLane: OperationLane = "navigation",
restoredBfcacheIds?: Readonly<Record<string, string>> | null,
): Promise<NavigationPayloadOutcome> {
try {
return await browserNavigationController.renderNavigationPayload({
Expand All @@ -281,6 +289,7 @@ async function renderNavigationPayload(
params,
pendingRouterState,
previousNextUrl,
restoredBfcacheIds,
targetHref,
navId,
});
Expand Down Expand Up @@ -461,6 +470,7 @@ function BrowserRoot({
const initialMetadata = AppElementsWire.readMetadata(resolvedElements);
const [treeStateValue, setTreeStateValue] = useState<AppRouterState | Promise<AppRouterState>>({
activeOperation: null,
bfcacheIds: createInitialBfcacheIdMap(resolvedElements),
elements: resolvedElements,
interceptionContext: initialMetadata.interceptionContext,
layoutIds: initialMetadata.layoutIds,
Expand Down Expand Up @@ -510,13 +520,17 @@ function BrowserRoot({
}

replaceHistoryStateWithoutNotify(
createHistoryStateWithPreviousNextUrl(window.history.state, treeState.previousNextUrl),
createHistoryStateWithPreviousNextUrl(
window.history.state,
treeState.previousNextUrl,
treeState.bfcacheIds,
),
"",
window.location.href,
);
}, [treeState.previousNextUrl, treeState.renderId]);
}, [treeState.bfcacheIds, treeState.previousNextUrl, treeState.renderId]);

const innerTree = createElement(
const routeTree = createElement(
RedirectBoundary,
null,
createElement(
Expand All @@ -530,6 +544,11 @@ function BrowserRoot({
),
);

const BfcacheIdMapContext = getBfcacheIdMapContext();
const innerTree = BfcacheIdMapContext
? createElement(BfcacheIdMapContext.Provider, { value: treeState.bfcacheIds }, routeTree)
: routeTree;

// In dev, wrap the route tree in a top-level recovery boundary. A render
// error (e.g. a slot's RSC reference rejects) is caught here instead of
// tearing down BrowserRoot, so HMR can dispatch the next payload —
Expand Down Expand Up @@ -913,7 +932,7 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
latestClientParams,
);
replaceHistoryStateWithoutNotify(
createHistoryStateWithPreviousNextUrl(window.history.state, null),
createHistoryStateWithPreviousNextUrl(window.history.state, null, null),
"",
window.location.href,
);
Expand Down Expand Up @@ -999,6 +1018,8 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
const requestState = getRequestState(navigationKind, currentPrevNextUrl);
const requestInterceptionContext = requestState.interceptionContext;
const requestPreviousNextUrl = requestState.previousNextUrl;
const restoredBfcacheIds =
navigationKind === "traverse" ? readHistoryStateBfcacheIds(window.history.state) : null;

// 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 Expand Up @@ -1056,6 +1077,7 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
pendingRouterState,
toActionType(navigationKind),
toOperationLane(navigationKind),
restoredBfcacheIds,
);
return;
}
Expand Down Expand Up @@ -1210,6 +1232,7 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
pendingRouterState,
toActionType(navigationKind),
toOperationLane(navigationKind),
restoredBfcacheIds,
);
if (renderOutcome !== "committed") return;
// Don't cache the response if this navigation was superseded during
Expand Down
5 changes: 5 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 @@ -30,6 +30,7 @@ export type NavigationPayloadOutcome = "committed" | "no-commit" | "hard-navigat
type HardNavigationMode = "assign" | "replace";

type BrowserNavigationCommitEffectFactory = (options: {
bfcacheIds: Readonly<Record<string, string>>;
href: string;
historyUpdateMode: HistoryUpdateMode | undefined;
navId: number;
Expand Down Expand Up @@ -68,6 +69,7 @@ type BrowserNavigationController = {
params: Record<string, string | string[]>;
pendingRouterState: PendingBrowserRouterState | null;
previousNextUrl: string | null;
restoredBfcacheIds?: Readonly<Record<string, string>> | null;
targetHref: string;
navId: number;
}): Promise<NavigationPayloadOutcome>;
Expand Down Expand Up @@ -462,6 +464,7 @@ export function createAppBrowserNavigationController(
params: Record<string, string | string[]>;
pendingRouterState: PendingBrowserRouterState | null;
previousNextUrl: string | null;
restoredBfcacheIds?: Readonly<Record<string, string>> | null;
targetHref: string;
navId: number;
}): Promise<NavigationPayloadOutcome> {
Expand All @@ -482,6 +485,7 @@ export function createAppBrowserNavigationController(
operationLane: options.operationLane,
previousNextUrl: options.previousNextUrl,
renderId,
restoredBfcacheIds: options.restoredBfcacheIds,
type: options.actionType,
});

Expand Down Expand Up @@ -515,6 +519,7 @@ export function createAppBrowserNavigationController(
renderId,
options.createNavigationCommitEffect({
href: options.targetHref,
bfcacheIds: approvedCommit.action.bfcacheIds,
historyUpdateMode: options.historyUpdateMode,
navId: options.navId,
params: options.params,
Expand Down
142 changes: 142 additions & 0 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 @@ -30,13 +30,16 @@ import {
import type { ClientNavigationRenderSnapshot } from "vinext/shims/navigation";

const VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY = "__vinext_previousNextUrl";
const VINEXT_BFCACHE_IDS_HISTORY_STATE_KEY = "__vinext_bfcacheIds";

type HistoryStateRecord = {
[key: string]: unknown;
};

export type { OperationLane } from "./navigation-planner.js";

export type BfcacheIdMap = Readonly<Record<string, string>>;

type OperationRecordBase = {
id: number;
lane: OperationLane;
Expand All @@ -56,6 +59,7 @@ export type OperationRecord = PendingOperationRecord | CommittedOperationRecord;

export type AppRouterState = {
activeOperation: OperationRecord | null;
bfcacheIds: BfcacheIdMap;
elements: AppElements;
interceptionContext: string | null;
layoutFlags: LayoutFlags;
Expand All @@ -69,6 +73,7 @@ export type AppRouterState = {
};

export type AppRouterAction = {
bfcacheIds: BfcacheIdMap;
elements: AppElements;
interceptionContext: string | null;
layoutFlags: LayoutFlags;
Expand Down Expand Up @@ -106,6 +111,8 @@ type PendingNavigationCommitDispositionDecision =
| DispatchPendingNavigationCommitDispositionDecision
| NonDispatchPendingNavigationCommitDispositionDecision;

let nextBfcacheId = 0;
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Module-level mutable counter is shared across test runs. nextBfcacheId is a module-level let that only ever increases (via rememberBfcacheId / mintBfcacheId). Because Vitest reuses the module between tests in the same file, the counter carries forward. This means tests can't assert exact id values — they must use regex patterns like /^_b_\d+_$/.

The tests already do this, so it works today. But it's fragile: a test that calls mintBfcacheId (indirectly through createNextBfcacheIdMap) will bump the counter, affecting subsequent tests' id values. If a future test needs to assert exact ids (e.g. for snapshot testing), it'll be surprised.

Consider exporting a resetBfcacheIdCounter() test helper (guarded behind process.env.NODE_ENV !== 'production' or only exported from the test barrel), or restructuring the counter to be injectable.


function cloneHistoryState(state: unknown): HistoryStateRecord {
if (!state || typeof state !== "object") {
return {};
Expand All @@ -121,6 +128,7 @@ function cloneHistoryState(state: unknown): HistoryStateRecord {
export function createHistoryStateWithPreviousNextUrl(
state: unknown,
previousNextUrl: string | null,
bfcacheIds?: BfcacheIdMap | null,
): HistoryStateRecord | null {
const nextState = cloneHistoryState(state);

Expand All @@ -130,6 +138,14 @@ export function createHistoryStateWithPreviousNextUrl(
nextState[VINEXT_PREVIOUS_NEXT_URL_HISTORY_STATE_KEY] = previousNextUrl;
}

if (bfcacheIds !== undefined) {
if (bfcacheIds === null || Object.keys(bfcacheIds).length === 0) {
delete nextState[VINEXT_BFCACHE_IDS_HISTORY_STATE_KEY];
} else {
nextState[VINEXT_BFCACHE_IDS_HISTORY_STATE_KEY] = { ...bfcacheIds };
}
}

return Object.keys(nextState).length > 0 ? nextState : null;
}

Expand All @@ -138,6 +154,124 @@ export function readHistoryStatePreviousNextUrl(state: unknown): string | null {
return typeof value === "string" ? value : null;
}

function rememberBfcacheId(value: string): void {
const match = /^_b_(\d+)_$/.exec(value);
if (!match) return;
nextBfcacheId = Math.max(nextBfcacheId, Number(match[1]));
}

function mintBfcacheId(): string {
nextBfcacheId += 1;
return `_b_${nextBfcacheId}_`;
}

function isBfcacheSegmentId(id: string): boolean {
const parsed = AppElementsWire.parseElementKey(id);
return (
parsed?.kind === "layout" ||
parsed?.kind === "page" ||
parsed?.kind === "slot" ||
parsed?.kind === "template"
);
}

function getPathSegments(pathname: string): string[] {
return pathname.split("/").filter(Boolean);
}

function getVisibleTreePathSegments(treePath: string): string[] {
return treePath
.split("/")
.filter(Boolean)
.filter((segment) => !(segment.startsWith("(") && segment.endsWith(")")));
}

function getPathPrefix(pathname: string, segmentCount: number): string {
if (segmentCount === 0) return "/";
const segments = getPathSegments(pathname).slice(0, segmentCount);
return `/${segments.join("/")}`;
}

function createBfcacheSegmentIdentity(id: string, pathname: string): string | null {
const parsed = AppElementsWire.parseElementKey(id);
if (!parsed) return null;

if (parsed.kind === "page") {
return `${id}@${pathname}`;
}

if (parsed.kind === "layout" || parsed.kind === "slot" || parsed.kind === "template") {
const segmentCount = getVisibleTreePathSegments(parsed.treePath).length;
return `${id}@${getPathPrefix(pathname, segmentCount)}`;
}

return null;
}

function collectBfcacheSegmentIds(elements: AppElements): string[] {
const ids = new Set(Object.keys(elements));
try {
for (const layoutId of AppElementsWire.readMetadata(elements).layoutIds) {
ids.add(layoutId);
}
} catch {
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Silencing the catch with an empty block is fine for test robustness, but consider at least a // ignore or more specific comment about which tests hit this path, so future readers don't wonder if this swallows real errors in production.

// Some low-level tests pass partial element maps without metadata.
}

return Array.from(ids).filter(isBfcacheSegmentId);
}

export function createInitialBfcacheIdMap(elements: AppElements): BfcacheIdMap {
const ids: Record<string, string> = {};
for (const id of collectBfcacheSegmentIds(elements)) {
ids[id] = "0";
}
return ids;
}

export function readHistoryStateBfcacheIds(state: unknown): BfcacheIdMap | null {
const value = cloneHistoryState(state)[VINEXT_BFCACHE_IDS_HISTORY_STATE_KEY];
if (!value || typeof value !== "object" || Array.isArray(value)) {
return null;
}

const ids: Record<string, string> = {};
for (const [key, id] of Object.entries(value)) {
if (!isBfcacheSegmentId(key) || typeof id !== "string") {
return null;
}
ids[key] = id;
rememberBfcacheId(id);
}
return ids;
}

export function createNextBfcacheIdMap(options: {
current: BfcacheIdMap;
currentPathname: string;
elements: AppElements;
nextPathname: string;
restored?: BfcacheIdMap | null;
}): BfcacheIdMap {
for (const value of Object.values(options.current)) {
rememberBfcacheId(value);
}
for (const value of Object.values(options.restored ?? {})) {
rememberBfcacheId(value);
}

const ids: Record<string, string> = {};
for (const id of collectBfcacheSegmentIds(options.elements)) {
const currentIdentity = createBfcacheSegmentIdentity(id, options.currentPathname);
const nextIdentity = createBfcacheSegmentIdentity(id, options.nextPathname);
const currentValue = currentIdentity === nextIdentity ? options.current[id] : undefined;
const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId();
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Subtle ordering question: restored takes priority over currentValue, which makes sense for traversals (back/forward should restore the exact prior ids). But what happens if the restored map has stale segment ids that no longer exist in elements?

The for...of collectBfcacheSegmentIds(options.elements) loop ensures we only produce ids for segments present in the new elements, so stale keys from restored are silently dropped. That's correct behavior. Just confirming this is intentional — worth a brief inline comment explaining the precedence: restored > current-if-identity-matches > mint-fresh.

ids[id] = value;
rememberBfcacheId(value);
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Nit: this rememberBfcacheId(value) call is redundant. By this point value is one of:

  1. From options.restored — already remembered at line 260–261
  2. From options.current — already remembered at line 257–258
  3. From mintBfcacheId() — freshly minted with ++nextBfcacheId, so rememberBfcacheId is a no-op (the counter is already at or past this value)

Not harmful (it's a fast regex + Math.max no-op), but removing it would make the data flow clearer. Totally optional.

}
return ids;
}

function createOperationRecord(options: {
id: number;
lane: OperationLane;
Expand Down Expand Up @@ -398,6 +532,7 @@ export async function createPendingNavigationCommit(options: {
operationLane: OperationLane;
previousNextUrl?: string | null;
renderId: number;
restoredBfcacheIds?: BfcacheIdMap | null;
type: "navigate" | "replace" | "traverse";
}): Promise<PendingNavigationCommit> {
const elements = await options.nextElements;
Expand All @@ -409,6 +544,13 @@ export async function createPendingNavigationCommit(options: {

return {
action: {
bfcacheIds: createNextBfcacheIdMap({
current: options.currentState.bfcacheIds,
currentPathname: options.currentState.navigationSnapshot.pathname,
elements,
nextPathname: options.navigationSnapshot.pathname,
restored: options.restoredBfcacheIds,
}),
elements,
interceptionContext: metadata.interceptionContext,
layoutIds: metadata.layoutIds,
Expand Down
Loading
Loading