Implement useRouter bfcacheId semantics

[manNomi]

Summary

• Implements segment-scoped useRouter().bfcacheId semantics for the App Router shim.
• Persists Vinext-owned bfcache id maps in history state so back/forward traversal can restore previous ids.
• Provides bfcache contexts through the browser/SSR route tree and wraps the public router with a bounded cache keyed by the active segment id.
• Preserves bfcache metadata during hash-only navigation while avoiding stale scroll restoration/custom history state.

Feedback addressed

This is a fresh follow-up based on the review feedback from the earlier bfcacheId PR. In particular, this version:

• bounds _appRouterByBfcacheId with a small LRU cache instead of leaving it unbounded
• removes the React private-internals dispatcher check and relies on the existing render-stack fallback
• documents why unchanged-URL commits still persist bfcache ids for traversal/refresh cases
• preserves bfcache ids after reducer-level element merges, including planner-approved parallel route state
• adds programmatic router.push() / router.replace() coverage in the E2E fixture

Tests

• ./node_modules/.bin/vp check packages/vinext/src/server/app-history-state.ts packages/vinext/src/server/app-browser-state.ts packages/vinext/src/server/app-browser-visible-commit.ts packages/vinext/src/server/app-browser-navigation-controller.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-ssr-entry.ts packages/vinext/src/shims/navigation.ts packages/vinext/src/shims/slot.tsx tests/app-browser-entry.test.ts tests/shims.test.ts tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts tests/fixtures/app-basic/app/nextjs-compat/use-router-bfcache-id/[group]/[page]/leaf-content.tsx
• ./node_modules/.bin/vp test run tests/shims.test.ts -t "useRouter|bfcache|hash-only"
• ./node_modules/.bin/vp test run tests/app-browser-entry.test.ts -t "bfcache|parallel"
• PLAYWRIGHT_PROJECT=app-router ./node_modules/.bin/playwright test tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts
• PLAYWRIGHT_PROJECT=app-router ./node_modules/.bin/playwright test tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.ts

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1588

commit: 9616888

[NathanDrake2406]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e66ec5c91c

ℹ️ 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".

[chatgpt-codex-connector]

Handle catch-all segments when deriving bfcache identities

createBfcacheSegmentIdentity() derives layout/slot/template identity from a fixed path-prefix length (getVisibleTreePathSegments(...).length), which treats each dynamic segment as consuming exactly one URL segment. For catch-all routes like [...slug]/[[...slug]], the segment value spans the remaining path, so navigating /docs/a/b → /docs/a/c keeps the same computed prefix (/docs/a) and incorrectly reuses the previous bfcache id. That preserves client state for a segment whose params actually changed, so useRouter().bfcacheId semantics diverge for catch-all layouts/templates/slots.

Useful? React with 👍 / 👎.

[manNomi]

I’ve updated it accordingly.

[NathanDrake2406]

Important PR bro, I think it’ll get merged when maintainer is online. That said, there are a few improvements to be made.
Request changes: architecture is good, but I think we should tighten two seams before merging.

Blocking

1. useRouter() should preserve the actual AppRouterContext router

Right now useRouter() reads AppRouterContext, checks that it exists, then ignores the context value and returns a clone of the module-level _appRouter.
That breaks context ownership. The provider should remain the authority for router methods; useRouter() should only add the contextual bfcacheId.
This also better matches Next’s shape: read the router from context, read the nearest segment/cache identity, then return the router methods plus the contextual bfcacheId.
Suggested shape:

export function useRouter() {
  if (!AppRouterContext || typeof React.useContext !== "function") {
    throw new Error("invariant expected app router to be mounted");
  }
  const router = React.useContext(AppRouterContext);
  if (router === null) {
    throw new Error("invariant expected app router to be mounted");
  }
  const bfcacheId = readBfcacheIdFromContext();
  return React.useMemo(
    () => ({
      ...router,
      bfcacheId,
    }),
    [router, bfcacheId],
  );
}

That removes the need for the global _appRouterByBfcacheId LRU and avoids silently dropping custom/instrumented router methods later.

2. Public bfcacheId format should match the observable Next shape

The PR currently treats initial hook-visible IDs as "0". Next’s public hook materialises the zero cache node as "b_0".

Even though the value is opaque, this PR is Next-compat, and the b${n}_ format exists to make the value safe to concatenate into user keys. I’d prefer either storing b_0 in the map or storing internal numeric IDs and formatting at useRouter().

Architecture hardening worth doing now

I would not block on a full Route Manifest/semantic graph rewrite in this PR. That would make this slice too big.

But I do think we should create the seam now:

• Put all bfcache segment identity derivation behind one helper/module, e.g. createBfcacheSegmentIdentity(...).
• Document that the current AppElementsWire.parseElementKey(...) + pathname prefix logic is a legacy bridge, not the final semantic authority.
• Do not let wire-key parsing spread beyond that helper.
• Make history state strictly typed read/write transport for restoring prior IDs; it should not become route meaning.
• Add a test that a custom AppRouterContext provider keeps its custom methods after useRouter() adds bfcacheId.

This keeps the PR small while preventing the architecture from drifting back into “flat wire keys are the router’s brain”.

Test gaps

The new tests are useful, especially around push/replace and reducer preservation. I’d still add:

1. Browser back preserves actual keyed client state, not just the restored ID.
2. Server-action refresh preserves state, since this PR touches the same-URL/server-action commit path.
3. useRouter() with a non-singleton context router preserves the context router’s methods.

After those changes, I think the architecture is solid and aligned with the navigation direction.