[codex] Fix usePathname hydration snapshot

[southpolesteve]

Fixes #871.

Summary

• Share the browser hydration navigation context through a Symbol.for global so split next/navigation module instances read the same initial snapshot.
• Keep server-side ALS access unchanged while preventing client usePathname(), useSearchParams(), and useParams() from falling back to / during hydration.
• Add a regression test that loads separate setter/hook module instances and verifies the hydrated pathname, query, and params stay in sync.

Validation

• pnpm test tests/shims.test.ts -t "shares the hydrated navigation snapshot"
• pnpm test tests/nextjs-compat/use-client-page-pathname.test.ts tests/nextjs-compat/rsc-context-lazy-stream.test.ts

[southpolesteve]

/bigbonk review please

[pkg-pr-new]

Open in StackBlitz

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

commit: d96404d

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[ask-bonk]

Review — usePathname hydration snapshot fix

Reviewed the change in packages/vinext/src/shims/navigation.ts plus the regression test in tests/shims.test.ts. Verified behavior by running the new test against both main (fails with <span>/||</span>) and the PR branch (passes with <span>/split-hydration|hello|hello</span>). Also re-ran tests/nextjs-compat/use-client-page-pathname.test.ts + tests/nextjs-compat/rsc-context-lazy-stream.test.ts — all green.

Summary — looks correct

The root cause is real and this fix addresses it. On the client, usePathname() / useSearchParams() / useParams() call useSyncExternalStore with _getServerContext() as the getServerSnapshot. React uses that function during the hydration render to match the SSR-rendered HTML.

Before this PR, that getServerSnapshot path on the client had no globalThis bridge:

• navigation-state.ts (the Symbol.for globalThis writer for issue #688) is server-only — it imports node:async_hooks, so it never runs in the browser.
• The browser entry’s restoreHydrationNavigationContext() → setNavigationContext(...) calls only updated _serverContext on whichever module instance of navigation.ts the browser entry resolved.
• "use client" components could get a different module instance of navigation.ts under Vite (different resolved ID / env), and that instance’s _serverContext was still null → fallback to "/" → hydration mismatch (exactly what #871 reports).

Moving the hydration snapshot onto globalThis[Symbol.for("vinext.navigation.clientHydrationContext")] gives every client module instance of the shim a shared view, mirroring the existing GLOBAL_ACCESSORS_KEY pattern that already solves the server side (navigation.ts:145-153, navigation-state.ts:128-129). Good fix, minimal surface area.

Correctness walk-through

Server behavior is unchanged. navigation-state.ts replaces _setServerContext / _getServerContext via _registerStateAccessors() at import time (navigation-state.ts:128). Once that’s done the function bodies with the new typeof window !== "undefined" branch are never executed on the server — ALS is the live code path. The defaults’ client-only fast path is therefore unreachable during SSR and cannot leak state across requests or break ALS isolation. Verified by re-running the pathname / lazy-stream compat tests.

Client behavior is updated as intended.

• _setServerContext on the client sets both the module-local _serverContext and the global snapshot (navigation.ts:204-216). Setting the local as well is nice belt-and-suspenders, and it also keeps hasOwnProperty-based reads from the same module instance working.
• _getServerContext prefers the global snapshot when it has been explicitly set (including null), otherwise falls back to the module-local _serverContext (navigation.ts:196-203). The Object.prototype.hasOwnProperty.call(...) check is the right choice to distinguish "never set" from "explicitly null" — without that, setNavigationContext(null) on the client followed by a hook read would silently hand back a stale context from _serverContext.

Staleness is not an issue. useSyncExternalStore only consults getServerSnapshot during the SSR / hydration initial render. Post-hydration renders use getSnapshot (the client store), and subsequent client navigations go through getPathnameSnapshot() / getSearchParamsSnapshot() / getClientParamsSnapshot() which read ClientNavigationState.cachedPathname etc. The hydration snapshot is therefore allowed to stay "stuck" at the initial URL indefinitely — React won’t read it again for that page.

getNavigationContext() consumers are all server-side. I grep’d — getNavigationContext is consumed by app-rsc-entry.ts, app-page-render.ts, app-page-boundary-render.ts only. None of them run in the browser, so changing the client fallback doesn’t alter any existing code path.

No cleanup concern. restoreHydrationNavigationContext in the browser entry only ever passes a populated NavigationContext; nothing in the browser calls setNavigationContext(null). So the hydrationContext !== undefined ? hydrationContext : _serverContext branch never ends up intentionally poisoning reads with an explicit null.

Test quality

The regression test genuinely exercises the split-module case:

• Loads the shim under two distinct query strings so ESM caches two separate module records (the setter instance and the hook instance).
• Clears both GLOBAL_ACCESSORS_KEY and the new hydration key beforehand to prevent interference from other tests, and restores prior values in finally.
• Mocks window so isServer evaluates false inside the shim and the default _setServerContext takes the new client branch.
• Uses renderToStaticMarkup, which drives useSyncExternalStore through its getServerSnapshot argument — the exact code path that returns "/" during browser hydration in #871.

Confirmed by inverting the diff locally: without the shim change the test renders <span>/||</span>; with it, <span>/split-hydration|hello|hello</span>.

Minor suggestions (non-blocking)

1. Comment clarifying the "why" of the client branch. The existing inline comment above GLOBAL_HYDRATION_CONTEXT_KEY is good, but the typeof window check inside _getServerContext / _setServerContext reads a little odd on a default that’s "supposed to be overridden on the server". A single-line reminder like // Hit only on the client — navigation-state.ts replaces both setters on the server. at navigation.ts:197 and :205 would help future readers not worry about the server branch leaking through this path.

2. Consider covering setNavigationContext(null) on the client explicitly. It’s not reachable today, but if a future refactor ever starts calling setNavigationContext(null) from browser code the fallback-to-stale-module-local vs explicit-null semantics become subtle. A tiny unit test asserting that after setNavigationContext(null) on module A the hook module B reads null (not a stale context from its own _serverContext) would lock in the hasOwnProperty contract.

3. _getClientHydrationContext return simplification. return globalState[_GLOBAL_HYDRATION_CONTEXT_KEY] ?? null coupled with an hasOwnProperty guard is fine, but the ?? null is a no-op when the property exists — stored values are NavigationContext | null, never undefined. You could just return globalState[_GLOBAL_HYDRATION_CONTEXT_KEY] as NavigationContext | null without changing behavior. Cosmetic only.

4. Next.js parity note. The PR comment in the test correctly points at app-router.tsx’s PathnameContext. Worth noting in the shim comment too that Next.js sidesteps this class of module-split bug by keeping hydration values inside a React context the provider tree carries across module instances; vinext’s shim uses useSyncExternalStore instead and so has to bridge the snapshot via globalThis. That context in navigation.ts:166-168 is the main place a future reader will look to understand why this workaround exists.

Verdict

Approve. Fix is minimal, targeted, correct on both client and server, and the regression test is a real regression test (I confirmed it fails without the shim change). No dev/prod parity concerns — the client-only branch is gated by typeof window !== "undefined" and server ALS overrides take precedence via _registerStateAccessors.

[ask-bonk]

Review posted on PR #878.

Summary of review:

• Verdict: Approve. The fix correctly addresses usePathname temporarily resolves to "/" on the client before navigating to the correct pathname #871.
• Root cause confirmed: On the client, useSyncExternalStore's getServerSnapshot argument (_getServerContext()?.pathname ?? "/") had no globalThis bridge because navigation-state.ts is server-only (imports node:async_hooks). When Vite loaded separate module instances of the shim for "use client" components, the browser entry’s setNavigationContext(...) only populated one instance, and hook callers read "/" from their own module-local _serverContext → hydration mismatch.
• Fix is well-scoped: Uses Symbol.for globalThis bridge, mirroring the existing GLOBAL_ACCESSORS_KEY pattern from usePathname returns invalid value on SSR #688. Server ALS is unchanged because _registerStateAccessors replaces the default functions before the typeof window branch can run.
• Verified by inversion: Ran the new regression test against main (fails: <span>/||</span>) vs PR branch (passes: <span>/split-hydration|hello|hello</span>). Related compat tests pass.
• Minor non-blocking nits: clarifying comments on the client-only branch, optional explicit-null unit test, cosmetic ?? null cleanup, and a note about Next.js using PathnameContext rather than useSyncExternalStore.

github run

[NathanDrake2406]

@james-elicx not approved here. I think it forgot how to approve PR

[james-elicx]

/bigbonk re-review for issues

[ask-bonk]

Approving on the basis of the detailed review above — verdict there was "Approve" but it was submitted as a plain comment rather than an approval. Re-submitting now as a formal approval.

Recap of that review:

• Root cause confirmed: on the client, useSyncExternalStore's getServerSnapshot (_getServerContext()?.pathname ?? "/") had no globalThis bridge because navigation-state.ts is server-only (imports node:async_hooks). Separate Vite module instances of the shim meant the browser entry's setNavigationContext(...) populated only one instance while hook callers read "/" from their own module-local _serverContext.
• Fix is well-scoped: uses Symbol.for globalThis bridge, mirroring the existing GLOBAL_ACCESSORS_KEY pattern from #688. Server ALS is unchanged because _registerStateAccessors replaces both default functions before the typeof window branch can run.
• Verified by inversion: new regression test fails on main (<span>/||</span>) and passes on this branch (<span>/split-hydration|hello|hello</span>). Related compat tests (use-client-page-pathname, rsc-context-lazy-stream) stay green.
• Nits listed in the prior review remain non-blocking.

[ask-bonk]

Formal approval posted on PR #878.

What I did:

• Checked PR review history — the prior ask-bonk review concluded "Approve" in its body but was submitted with state COMMENTED, which is exactly what NathanDrake2406 flagged.
• Submitted a proper APPROVED review referencing the earlier analysis (root cause, fix scope, verification by inversion, and the non-blocking nits).

Review state on PR #878 is now APPROVED.

github run