Skip to content

Commit 7d783fa

Browse files
brkalowclaudejacekradko
authored
fix(ui): sync BaseRouter state on pushState/replaceState (#7840)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Jacek Radko <jacek@clerk.dev>
1 parent 21aafe3 commit 7d783fa

10 files changed

Lines changed: 242 additions & 29 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/ui': patch
3+
---
4+
5+
Fix BaseRouter state not syncing after popup OAuth by observing `pushState`/`replaceState` changes in addition to `popstate`

integration/presets/longRunningApps.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export const createLongRunningApps = () => {
6363
{ id: 'react.vite.withEmailCodes', config: react.vite, env: envs.withEmailCodes },
6464
{ id: 'react.vite.withEmailCodes_persist_client', config: react.vite, env: envs.withEmailCodes_destroy_client },
6565
{ id: 'react.vite.withEmailLinks', config: react.vite, env: envs.withEmailLinks },
66+
{ id: 'react.vite.withLegalConsent', config: react.vite, env: envs.withLegalConsent },
6667
{ id: 'vue.vite', config: vue.vite, env: envs.withCustomRoles },
6768

6869
/**

integration/templates/react-vite/src/buttons/index.tsx

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ export default function Home() {
1111
Sign in button (force)
1212
</SignInButton>
1313

14+
<SignInButton
15+
mode='modal'
16+
oauthFlow='popup'
17+
forceRedirectUrl='/protected'
18+
signUpForceRedirectUrl='/protected'
19+
>
20+
Sign in button (force, popup)
21+
</SignInButton>
22+
1423
<SignInButton
1524
mode='modal'
1625
fallbackRedirectUrl='/protected'

integration/templates/react-vite/src/main.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { createBrowserRouter, Outlet, RouterProvider, useNavigate } from 'react-
66
import App from './App.tsx';
77
import Protected from './protected';
88
import SignIn from './sign-in';
9+
import SignInPopup from './sign-in-popup';
910
import SignUp from './sign-up';
1011
import UserProfile from './user';
1112
import UserProfileCustom from './custom-user-profile';
@@ -61,6 +62,10 @@ const router = createBrowserRouter([
6162
path: '/sign-in/*',
6263
element: <SignIn />,
6364
},
65+
{
66+
path: '/sign-in-popup/*',
67+
element: <SignInPopup />,
68+
},
6469
{
6570
path: '/sign-up/*',
6671
element: <SignUp />,
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import { SignIn } from '@clerk/react';
2+
3+
export default function Page() {
4+
return (
5+
<div>
6+
<SignIn
7+
path={'/sign-in-popup'}
8+
signUpUrl={'/sign-up'}
9+
oauthFlow='popup'
10+
fallbackRedirectUrl='/protected'
11+
fallback={<>Loading sign in</>}
12+
/>
13+
</div>
14+
);
15+
}

integration/tests/oauth-flows.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,63 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('oauth flo
181181
});
182182
});
183183

184+
testAgainstRunningApps({ withPattern: ['react.vite.withLegalConsent'] })(
185+
'oauth popup with path-based routing @react',
186+
({ app }) => {
187+
test.describe.configure({ mode: 'serial' });
188+
189+
let fakeUser: FakeUser;
190+
191+
test.beforeAll(async () => {
192+
const client = createClerkClient({
193+
secretKey: instanceKeys.get('oauth-provider').sk,
194+
publishableKey: instanceKeys.get('oauth-provider').pk,
195+
});
196+
const users = createUserService(client);
197+
fakeUser = users.createFakeUser({
198+
withUsername: true,
199+
});
200+
await users.createBapiUser(fakeUser);
201+
});
202+
203+
test.afterAll(async () => {
204+
const u = createTestUtils({ app });
205+
await fakeUser.deleteIfExists();
206+
await u.services.users.deleteIfExists({ email: fakeUser.email });
207+
await app.teardown();
208+
});
209+
210+
test('popup OAuth navigates through sso-callback with path-based routing', async ({ page, context }) => {
211+
const u = createTestUtils({ app, page, context });
212+
213+
await u.page.goToRelative('/sign-in-popup');
214+
await u.page.waitForClerkJsLoaded();
215+
await u.po.signIn.waitForMounted();
216+
217+
const popupPromise = context.waitForEvent('page');
218+
await u.page.getByRole('button', { name: 'E2E OAuth Provider' }).click();
219+
const popup = await popupPromise;
220+
const popupUtils = createTestUtils({ app, page: popup, context });
221+
await popupUtils.page.getByText('Sign in to oauth-provider').waitFor();
222+
223+
// Complete OAuth in the popup
224+
await popupUtils.po.signIn.setIdentifier(fakeUser.email);
225+
await popupUtils.po.signIn.continue();
226+
await popupUtils.po.signIn.enterTestOtpCode();
227+
228+
// Because the user is new to the app and legal consent is required,
229+
// the sign-up can't complete in the popup. The popup sends return_url
230+
// back to the parent, which navigates to /sso-callback via pushState.
231+
await u.page.getByRole('heading', { name: 'Legal consent' }).waitFor();
232+
await u.page.getByLabel(/I agree to the/).check();
233+
await u.po.signIn.continue();
234+
235+
await u.page.waitForAppUrl('/protected');
236+
await u.po.expect.toBeSignedIn();
237+
});
238+
},
239+
);
240+
184241
testAgainstRunningApps({ withEnv: [appConfigs.envs.withLegalConsent] })(
185242
'oauth flows with legal consent @nextjs',
186243
({ app }) => {

packages/ui/src/hooks/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,3 @@ export * from './useSafeState';
1717
export * from './useScrollLock';
1818
export * from './useSearchInput';
1919
export * from './useTotalEnabledAuthMethods';
20-
export * from './useWindowEventListener';

packages/ui/src/hooks/useWindowEventListener.ts

Lines changed: 0 additions & 15 deletions
This file was deleted.

packages/ui/src/router/BaseRouter.tsx

Lines changed: 149 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,135 @@ import type { NavigateOptions } from '@clerk/shared/types';
55
import React from 'react';
66
import { flushSync } from 'react-dom';
77

8-
import { useWindowEventListener } from '../hooks';
98
import { newPaths } from './newPaths';
109
import { match } from './pathToRegexp';
1110
import { Route } from './Route';
1211
import { RouteContext } from './RouteContext';
1312

13+
// Custom events that don't exist on WindowEventMap but are handled
14+
// by wrapping history.pushState/replaceState in the fallback path.
15+
type HistoryEvent = 'pushstate' | 'replacestate';
16+
type RefreshEvent = keyof WindowEventMap | HistoryEvent;
17+
type NavigationType = 'push' | 'replace' | 'traverse';
18+
19+
const isWindowRefreshEvent = (event: RefreshEvent): event is keyof WindowEventMap => {
20+
return event !== 'pushstate' && event !== 'replacestate';
21+
};
22+
23+
// Maps refresh events to Navigation API navigationType values.
24+
const eventToNavigationType: Partial<Record<RefreshEvent, NavigationType>> = {
25+
popstate: 'traverse',
26+
pushstate: 'push',
27+
replacestate: 'replace',
28+
};
29+
30+
// Global subscription sets for the history monkey-patching fallback.
31+
// Using a single patch with subscriber sets avoids conflicts when
32+
// multiple BaseRouter instances mount simultaneously.
33+
const pushStateSubscribers = new Set<() => void>();
34+
const replaceStateSubscribers = new Set<() => void>();
35+
let originalPushState: History['pushState'] | null = null;
36+
let originalReplaceState: History['replaceState'] | null = null;
37+
38+
function ensurePushStatePatched(): void {
39+
if (originalPushState) {
40+
return;
41+
}
42+
originalPushState = history.pushState.bind(history);
43+
history.pushState = (...args: Parameters<History['pushState']>) => {
44+
originalPushState!(...args);
45+
pushStateSubscribers.forEach(fn => fn());
46+
};
47+
}
48+
49+
function ensureReplaceStatePatched(): void {
50+
if (originalReplaceState) {
51+
return;
52+
}
53+
originalReplaceState = history.replaceState.bind(history);
54+
history.replaceState = (...args: Parameters<History['replaceState']>) => {
55+
originalReplaceState!(...args);
56+
replaceStateSubscribers.forEach(fn => fn());
57+
};
58+
}
59+
60+
/**
61+
* Observes history changes so the router's internal state stays in sync
62+
* with the URL. Uses the Navigation API when available, falling back to
63+
* monkey-patching history.pushState/replaceState plus native window events.
64+
*
65+
* Note: `events` should be a stable array reference to avoid
66+
* re-subscribing on every render.
67+
*/
68+
function useHistoryChangeObserver(events: Array<RefreshEvent> | undefined, callback: () => void): void {
69+
const callbackRef = React.useRef(callback);
70+
callbackRef.current = callback;
71+
72+
React.useEffect(() => {
73+
if (!events) {
74+
return;
75+
}
76+
77+
const notify = () => callbackRef.current();
78+
const windowEvents = events.filter(isWindowRefreshEvent);
79+
const navigationTypes = events
80+
.map(e => eventToNavigationType[e])
81+
.filter((type): type is NavigationType => Boolean(type));
82+
83+
const hasNavigationAPI =
84+
typeof window !== 'undefined' &&
85+
'navigation' in window &&
86+
typeof (window as any).navigation?.addEventListener === 'function';
87+
88+
if (hasNavigationAPI) {
89+
const nav = (window as any).navigation;
90+
const allowedTypes = new Set(navigationTypes);
91+
const handler = (e: { navigationType: NavigationType }) => {
92+
if (allowedTypes.has(e.navigationType)) {
93+
Promise.resolve().then(notify);
94+
}
95+
};
96+
nav.addEventListener('currententrychange', handler);
97+
98+
// Events without a navigationType mapping (e.g. hashchange) still
99+
// need native listeners even when the Navigation API is available.
100+
const unmappedEvents = windowEvents.filter(e => !eventToNavigationType[e]);
101+
unmappedEvents.forEach(e => window.addEventListener(e, notify));
102+
103+
return () => {
104+
nav.removeEventListener('currententrychange', handler);
105+
unmappedEvents.forEach(e => window.removeEventListener(e, notify));
106+
};
107+
}
108+
109+
// Fallback: use global subscriber sets for pushState/replaceState
110+
// so that multiple BaseRouter instances don't conflict.
111+
if (events.includes('pushstate')) {
112+
ensurePushStatePatched();
113+
pushStateSubscribers.add(notify);
114+
}
115+
if (events.includes('replacestate')) {
116+
ensureReplaceStatePatched();
117+
replaceStateSubscribers.add(notify);
118+
}
119+
120+
windowEvents.forEach(e => window.addEventListener(e, notify));
121+
122+
return () => {
123+
pushStateSubscribers.delete(notify);
124+
replaceStateSubscribers.delete(notify);
125+
windowEvents.forEach(e => window.removeEventListener(e, notify));
126+
};
127+
}, [events]);
128+
}
129+
14130
interface BaseRouterProps {
15131
basePath: string;
16132
startPath: string;
17133
getPath: () => string;
18134
getQueryString: () => string;
19135
internalNavigate: (toURL: URL, options?: NavigateOptions) => Promise<any> | any;
20-
refreshEvents?: Array<keyof WindowEventMap>;
136+
refreshEvents?: Array<RefreshEvent>;
21137
preservedParams?: string[];
22138
urlStateParam?: {
23139
startPath: string;
@@ -88,7 +204,23 @@ export const BaseRouter = ({
88204
}
89205
}, [currentPath, currentQueryString, getPath, getQueryString]);
90206

91-
useWindowEventListener(refreshEvents, refresh);
207+
// Suppresses the history observer during baseNavigate's internal navigation.
208+
// Without this, the observer's microtask triggers a render before setActive's
209+
// #updateAccessors sets clerk.session, causing task guards to see stale state.
210+
const isNavigatingRef = React.useRef(false);
211+
212+
const observerRefresh = React.useCallback((): void => {
213+
if (isNavigatingRef.current) {
214+
return;
215+
}
216+
const newPath = getPath();
217+
if (basePath && !newPath.startsWith('/' + basePath)) {
218+
return;
219+
}
220+
refresh();
221+
}, [basePath, getPath, refresh]);
222+
223+
useHistoryChangeObserver(refreshEvents, observerRefresh);
92224

93225
// TODO: Look into the real possible types of globalNavigate
94226
const baseNavigate = async (toURL: URL | undefined): Promise<unknown> => {
@@ -118,15 +250,20 @@ export const BaseRouter = ({
118250

119251
toURL.search = stringifyQueryParams(toQueryParams);
120252
}
121-
const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } });
122-
// We need to flushSync to guarantee the re-render happens before handing things back to the caller,
123-
// otherwise setActive might emit, and children re-render with the old navigation state.
124-
// An alternative solution here could be to return a deferred promise, set that to state together
125-
// with the routeParts and resolve it in an effect. That way we could avoid the flushSync performance penalty.
126-
flushSync(() => {
127-
setRouteParts({ path: toURL.pathname, queryString: toURL.search });
128-
});
129-
return internalNavRes;
253+
isNavigatingRef.current = true;
254+
try {
255+
const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } });
256+
// We need to flushSync to guarantee the re-render happens before handing things back to the caller,
257+
// otherwise setActive might emit, and children re-render with the old navigation state.
258+
// An alternative solution here could be to return a deferred promise, set that to state together
259+
// with the routeParts and resolve it in an effect. That way we could avoid the flushSync performance penalty.
260+
flushSync(() => {
261+
setRouteParts({ path: toURL.pathname, queryString: toURL.search });
262+
});
263+
return internalNavRes;
264+
} finally {
265+
isNavigatingRef.current = false;
266+
}
130267
};
131268

132269
return (

packages/ui/src/router/PathRouter.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export const PathRouter = ({ basePath, preservedParams, children }: PathRouterPr
5959
getPath={getPath}
6060
getQueryString={getQueryString}
6161
internalNavigate={internalNavigate}
62-
refreshEvents={['popstate']}
62+
refreshEvents={['pushstate', 'replacestate', 'popstate']}
6363
preservedParams={preservedParams}
6464
>
6565
{children}

0 commit comments

Comments
 (0)