Uh oh!
There was an error while loading. Please reload this page.
fix(ui): sync BaseRouter state on pushState/replaceState - #7840
Conversation
…pup OAuth BaseRouter now uses a single useHistoryChangeObserver hook driven by the refreshEvents array to stay in sync with the URL. With the Navigation API it listens to currententrychange filtered by navigationType; without it, it wraps pushState/replaceState and listens for native window events. PathRouter opts in to pushstate, replacestate, and popstate. This fixes the popup OAuth flow where the SSO callback URL was pushed via history.pushState but the PathRouter never re-rendered because popstate does not fire on pushState calls. Adds an integration test that forces the popup OAuth flow with path-based routing to verify the fix.
…afety Use global subscription sets for history monkey-patching to prevent conflicts when multiple BaseRouter instances mount. Move Navigation API detection into the effect for SSR safety. Defer notify via microtask to avoid re-entrancy. Remove unused useWindowEventListener hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: cbab21e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a SignIn popup flow to the React-Vite template: a new SignInPopup page, a /sign-in-popup route, and an additional SignInButton in Home. Introduces path-based OAuth popup tests for the React/Vite app (duplicated test suite). Removes the exported useWindowEventListener hook and its implementation. Replaces its usage with a new useHistoryChangeObserver in BaseRouter, updates BaseRouter props to accept history events, and patches history.pushState/replaceState. PathRouter now refreshes on pushstate, replacestate, and popstate. Adds a changeset and a new long-running app preset entry. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui/src/router/BaseRouter.tsx`:
- Around line 43-46: The forEach callbacks currently use concise arrow bodies
that implicitly return the result (e.g., in the history.pushState override and
other locations referencing pushStateSubscribers.forEach and
replaceStateSubscribers.forEach); change those arrow callbacks to block bodies
with no return value (or replace with a for...of loop) so they do not return a
value — update the callbacks used in history.pushState override,
history.replaceState override, and any other pushStateSubscribers.forEach /
replaceStateSubscribers.forEach usages (the blocks around lines 43-46, 54-57,
100-106, 120-125) to use explicit block-bodied arrows that call the subscriber
and do not return anything.
| history.pushState = (...args: Parameters<History['pushState']>) => { | ||
| originalPushState!(...args); | ||
| pushStateSubscribers.forEach(fn => fn()); | ||
| }; |
There was a problem hiding this comment.
Fix lint errors: forEach callbacks must not return values.
Biome flags these callbacks as errors; CI will fail unless they’re changed to block bodies with no return value.
Suggested fix
- history.pushState = (...args: Parameters<History['pushState']>) => {- originalPushState!(...args);- pushStateSubscribers.forEach(fn => fn());- };+ history.pushState = (...args: Parameters<History['pushState']>) => {+ originalPushState!(...args);+ pushStateSubscribers.forEach(fn => {+ fn();+ });+ };- history.replaceState = (...args: Parameters<History['replaceState']>) => {- originalReplaceState!(...args);- replaceStateSubscribers.forEach(fn => fn());- };+ history.replaceState = (...args: Parameters<History['replaceState']>) => {+ originalReplaceState!(...args);+ replaceStateSubscribers.forEach(fn => {+ fn();+ });+ };- unmappedEvents.forEach(e => window.addEventListener(e, notify));+ unmappedEvents.forEach(e => {+ window.addEventListener(e, notify);+ });- unmappedEvents.forEach(e => window.removeEventListener(e, notify));+ unmappedEvents.forEach(e => {+ window.removeEventListener(e, notify);+ });- windowEvents.forEach(e => window.addEventListener(e, notify));+ windowEvents.forEach(e => {+ window.addEventListener(e, notify);+ });- windowEvents.forEach(e => window.removeEventListener(e, notify));+ windowEvents.forEach(e => {+ window.removeEventListener(e, notify);+ });As per coding guidelines, “All code must pass ESLint checks with the project's configuration.”
Also applies to: 54-57, 100-106, 120-125
🧰 Tools
🪛 Biome (2.3.14)
[error] 45-45: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/ui/src/router/BaseRouter.tsx` around lines 43 - 46, The forEach
callbacks currently use concise arrow bodies that implicitly return the result
(e.g., in the history.pushState override and other locations referencing
pushStateSubscribers.forEach and replaceStateSubscribers.forEach); change those
arrow callbacks to block bodies with no return value (or replace with a for...of
loop) so they do not return a value — update the callbacks used in
history.pushState override, history.replaceState override, and any other
pushStateSubscribers.forEach / replaceStateSubscribers.forEach usages (the
blocks around lines 43-46, 54-57, 100-106, 120-125) to use explicit block-bodied
arrows that call the subscriber and do not return anything.
@clerk/agent-toolkit@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/dev-cli@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
…n popup OAuth test The previous test used withEmailCodes where OAuth auto-completes the sign-up (popup returns session directly). This meant the parent page never navigated to sso-callback via pushState, so BaseRouter's history observation was never exercised. Switch to withLegalConsent where the sign-up requires an additional step. The popup returns return_url instead of session, forcing the parent to navigate to /sso-callback via pushState — which is the exact code path the BaseRouter fix addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ent stale renders The history observer's microtask can trigger a React render before setActive's #updateAccessors sets clerk.session, causing task guards to see stale state and redirect prematurely. Suppress the observer during baseNavigate since flushSync already handles the state update. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| Promise.resolve().then(notify); | ||
| } | ||
| }; | ||
| nav.addEventListener('currententrychange', handler); |
There was a problem hiding this comment.
Uses the new baseline currententrychange listener if available
| // Suppresses the history observer during baseNavigate's internal navigation. | ||
| // Without this, the observer's microtask triggers a render before setActive's | ||
| // #updateAccessors sets clerk.session, causing task guards to see stale state. |
| getQueryString={getQueryString} | ||
| internalNavigate={internalNavigate} | ||
| refreshEvents={['popstate']} | ||
| refreshEvents={['pushstate', 'replacestate', 'popstate']} |
There was a problem hiding this comment.
We already had the refresh mechanism included in the router, this expands it to support pushstate and repalcestate
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jacekradko
left a comment
There was a problem hiding this comment.
Looks really solid overall. Just a bit light on unit tests
brkalow
commented
Feb 13, 2026
@jacekradko fair point! I didn't feel like unit tests would provide much value here, as we'd end up with a lot of mocking and mostly ceremonious assertions. Existing integration tests and the new one cover what we need. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Jacek Radko <jacek@clerk.dev>
HashRouter wasn't detecting pushState/replaceState changes from popup OAuth flows because those calls don't fire hashchange events. This adds pushstate/replacestate to HashRouter's refreshEvents (matching the PathRouter fix from #7840) and suppresses the history observer during external navigation in BaseRouter to prevent stale state updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
clerk.navigate(), which ultimately calls the host application's router (e.g., to/sso-callbackthen the redirect URL), butBaseRouteronly listened forpopstateevents — which only fire on back/forward navigation, not programmaticpushState/replaceState. This left the router's internal state out of sync with the actual URL.useWindowEventListenerwith a newuseHistoryChangeObserverhook that uses the Navigation API (currententrychange) when available, falling back to monkey-patchinghistory.pushState/replaceState.PathRouternow listens forpushstate,replacestate, andpopstateevents.BaseRouterinstances don't conflict when patching history methods. Navigation API detection is evaluated at effect-time (not module-load) for SSR safety. Notifications are deferred via microtask to prevent re-entrancy.useWindowEventListenerhook.Test plan
oauth popup with path-based routingintegration test validates popup OAuth completes and parent navigates to redirect URL<SignIn oauthFlow="popup" />with path-based routing, complete OAuth in popup, verify parent page lands on the redirect URL🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores