Skip to content

Commit 269fb34

Browse files
brkalowclaudejacekradko
committed
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 35bcbd1 commit 269fb34

10 files changed

Lines changed: 243 additions & 23 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
@@ -59,6 +59,7 @@ export const createLongRunningApps = () => {
5959
{ id: 'react.vite.withEmailCodes', config: react.vite, env: envs.withEmailCodes },
6060
{ id: 'react.vite.withEmailCodes_persist_client', config: react.vite, env: envs.withEmailCodes_destroy_client },
6161
{ id: 'react.vite.withEmailLinks', config: react.vite, env: envs.withEmailLinks },
62+
{ id: 'react.vite.withLegalConsent', config: react.vite, env: envs.withLegalConsent },
6263
{ id: 'vue.vite', config: vue.vite, env: envs.withCustomRoles },
6364

6465
/**

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';
@@ -57,6 +58,10 @@ const router = createBrowserRouter([
5758
path: '/sign-in/*',
5859
element: <SignIn />,
5960
},
61+
{
62+
path: '/sign-in-popup/*',
63+
element: <SignInPopup />,
64+
},
6065
{
6166
path: '/sign-up/*',
6267
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/clerk-js/src/ui/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/clerk-js/src/ui/hooks/useWindowEventListener.ts

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

packages/clerk-js/src/ui/router/BaseRouter.tsx

Lines changed: 150 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,138 @@
11
import { useClerk } from '@clerk/shared/react';
22
import type { NavigateOptions } from '@clerk/shared/types';
33
import React from 'react';
4+
import { flushSync } from 'react-dom';
45

56
import { getQueryParams, stringifyQueryParams, trimTrailingSlash } from '../../utils';
6-
import { useWindowEventListener } from '../hooks';
77
import { newPaths } from './newPaths';
88
import { match } from './pathToRegexp';
99
import { Route } from './Route';
1010
import { RouteContext } from './RouteContext';
1111

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

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

91224
// TODO: Look into the real possible types of globalNavigate
92225
const baseNavigate = async (toURL: URL | undefined): Promise<unknown> => {
@@ -116,9 +249,20 @@ export const BaseRouter = ({
116249

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

124268
return (

packages/clerk-js/src/ui/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)