fix: usePathname() returns "/" during SSR of "use client" page components#689
Conversation
…ents (cloudflare#688) The navigation shim uses a registration pattern where navigation-state.ts calls _registerStateAccessors() to upgrade navigation.ts from module-level state to ALS-backed state. This only updates the specific module instance that imported navigation-state.ts. In Vite multi-environment dev mode, "use client" components can end up with a separate module instance of navigation.ts (due to pre-bundling or different resolver chains). That instance never receives the ALS-backed accessors and falls back to _serverContext (null), causing usePathname() to return "/". Fix: navigation-state.ts now also stores the accessor functions on globalThis via Symbol.for("vinext.navigation.globalAccessors"). The default accessors in navigation.ts check this global before falling back to module-level state. This ensures all module instances can reach the ALS-backed state regardless of which instance _registerStateAccessors was called on. On the browser (where navigation-state.ts is never imported), the global key does not exist, so the fallback to module-level _serverContext is preserved — which is the correct behavior for client-side hydration. Closes cloudflare#688
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes a Vite multi-environment SSR edge case where usePathname() (and related navigation hooks) can return "/" during SSR for "use client" page components, leading to React hydration mismatches.
Changes:
- Share ALS-backed navigation state accessors across module instances by publishing them on
globalThis(viaSymbol.for) and consulting them fromnavigation.ts. - Add a Next.js-compat regression fixture + Vitest coverage for
"use client"page components usingusePathname(),useSearchParams(), anduseParams()(static + dynamic routes). - Add Playwright E2E coverage to assert correct rendering and absence of hydration errors for the new
"use client"pages.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/nextjs-compat/use-client-page-pathname.test.ts | New regression tests asserting correct SSR output + embedded __VINEXT_RSC_NAV__/__VINEXT_RSC_PARAMS__ for "use client" page components. |
| tests/fixtures/app-basic/app/use-client-page-pathname/page.tsx | New "use client" static page fixture rendering usePathname() + useSearchParams() values. |
| tests/fixtures/app-basic/app/use-client-page-pathname/[slug]/page.tsx | New "use client" dynamic page fixture rendering usePathname() + useParams() (and search string). |
| tests/e2e/app-router/routes.spec.ts | New E2E coverage ensuring correct hook behavior and no hydration errors on "use client" pages. |
| packages/vinext/src/shims/navigation.ts | Adds global accessor lookup fallback so separate module instances can still reach ALS-backed navigation state. |
| packages/vinext/src/shims/navigation-state.ts | Registers ALS-backed accessors both via _registerStateAccessors() and on globalThis for cross-module-instance discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const _GLOBAL_ACCESSORS_KEY = Symbol.for("vinext.navigation.globalAccessors"); | ||
| type _GlobalWithAccessors = typeof globalThis & { [_GLOBAL_ACCESSORS_KEY]?: _StateAccessors }; | ||
|
|
||
| function _getGlobalAccessors(): _StateAccessors | undefined { | ||
| return (globalThis as _GlobalWithAccessors)[_GLOBAL_ACCESSORS_KEY]; | ||
| } |
There was a problem hiding this comment.
The global accessor key (Symbol.for("vinext.navigation.globalAccessors")) is duplicated here and in navigation-state.ts. To avoid the two modules drifting out of sync, consider centralizing this key in a shared module/constant that both files import (or exporting it from one place) so a future rename can’t silently break the cross-instance lookup.
| _getState().serverInsertedHTMLCallbacks = []; | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Consider typing _accessors (e.g., with satisfies the _registerStateAccessors parameter type) before registering / writing it to globalThis. Right now it’s inferred, so a future change could accidentally diverge from what navigation.ts expects without a type error.
| }; | |
| } satisfies Parameters<typeof _registerStateAccessors>[0]; |
| const _GLOBAL_ACCESSORS_KEY = Symbol.for("vinext.navigation.globalAccessors"); | ||
|
|
There was a problem hiding this comment.
The _GLOBAL_ACCESSORS_KEY Symbol.for string is duplicated with navigation.ts. If either side changes, the cross-module-instance fallback will silently stop working. Consider importing a shared constant instead of re-declaring it in both modules.
|
/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
This is a well-diagnosed and cleanly implemented fix for a real Vite multi-environment edge case. The root cause analysis (separate module instances for the SSR entry vs "use client" page components) is correct, and the globalThis + Symbol.for pattern is the right approach — it's the same strategy already used elsewhere in the codebase for contexts (layoutSegmentContext, serverInsertedHTMLContext) and ALS instances.
The test coverage is thorough: Vitest for SSR HTML correctness + hydration data, and Playwright E2E for no hydration errors after client-side hydration.
A few observations below, none blocking.
Not in diff, but worth noting:
-
router.tsandhead.tsuse the same_register*StateAccessorspattern without the global fallback. If a Pages Router page component is ever marked"use client", they'd hit the same multi-instance bug. Probably rare in practice since Pages Router pages aren't typically client components, but worth tracking as a potential follow-up. -
In
flushServerInsertedHTML()(navigation.ts:781),callbacks.length = 0mutates the ALS-backed array in-place when the global accessor path is active. This is correct — the ALS store owns that array — but it's a subtle interaction worth calling out for future readers.
| clearInsertedHTMLCallbacks: () => void; | ||
| } | ||
|
|
||
| const _GLOBAL_ACCESSORS_KEY = Symbol.for("vinext.navigation.globalAccessors"); |
There was a problem hiding this comment.
The Symbol.for("vinext.navigation.globalAccessors") string is duplicated between this file and navigation-state.ts:102. Copilot flagged this too.
Since navigation-state.ts already imports from navigation.ts (_registerStateAccessors + NavigationContext), the simplest fix would be to export the key (or just the string constant) from navigation.ts and import it in navigation-state.ts. That way a typo or rename can't silently break the cross-instance lookup.
Not blocking since the two files are tightly coupled and the test suite would catch a mismatch, but it's a nice one-line cleanup.
| // These are overridden by navigation-state.ts on the server to use ALS. | ||
| let _getServerContext = (): NavigationContext | null => _serverContext; | ||
| // The defaults check globalThis for cross-module-instance access (issue #688). | ||
| let _getServerContext = (): NavigationContext | null => { |
There was a problem hiding this comment.
Nit for future readers: after _registerStateAccessors runs on the SSR entry's module instance, these let variables are overwritten to point directly at the ALS-backed functions. The _getGlobalAccessors() fallback in these defaults only fires from the other module instance that never had _registerStateAccessors called. The cost of the extra property lookup on globalThis per hook invocation in the unregistered instance is negligible.
| })); | ||
|
|
||
| // Warm up so first test doesn't pay cold-start cost. | ||
| await fetch(`${_baseUrl}${ROUTE}`); |
There was a problem hiding this comment.
Minor: the warm-up fetch result is discarded. If the fixture is misconfigured and 500s, the first real test gives a confusing failure instead of a clear "warm-up failed" message. Could add expect((await fetch(...)).ok).toBe(true) here, though the first test checks status anyway so not critical.
|
Review submitted successfully. Here's a summary of what I reviewed: Verdict: Approved The fix correctly addresses the Vite multi-environment module instance problem where Key observations in the review:
|
- Export GLOBAL_ACCESSORS_KEY from navigation.ts and import in navigation-state.ts to prevent silent drift if the Symbol string ever changes - Add satisfies constraint on the accessor object so shape mismatches are caught at compile time - Assert warm-up fetch status in test to surface fixture setup failures
Summary
Fixes #688.
usePathname()returns"/"during SSR when the page component itself is a"use client"component, causing a React hydration mismatchnavigation.tsfor the SSR entry vs"use client"components. The_registerStateAccessors()pattern only updates the SSR entry's instance — the"use client"instance falls back to_serverContext(null ->"/")navigation-state.tsnow also stores the ALS-backed accessor functions onglobalThisviaSymbol.for("vinext.navigation.globalAccessors"). The default accessors innavigation.tscheck this global before falling back to module-level state, ensuring all module instances reach the ALS-backed state regardless of which instance was registeredOn the browser (where
navigation-state.tsis never imported), the global key doesn't exist, so the fallback to module-level_serverContextis preserved — correct behavior for client-side hydration.Test plan
tests/nextjs-compat/use-client-page-pathname.test.ts— 8 tests coveringusePathname(),useSearchParams(),useParams()for"use client"page components (static + dynamic routes)tests/e2e/app-router/routes.spec.ts— 3 tests checking no hydration errors and correct rendering for"use client"pages