diff --git a/.changeset/fix-router-sync-popup-oauth.md b/.changeset/fix-router-sync-popup-oauth.md new file mode 100644 index 00000000000..639411475d0 --- /dev/null +++ b/.changeset/fix-router-sync-popup-oauth.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-js': patch +--- + +Fix BaseRouter state not syncing after popup OAuth by observing `pushState`/`replaceState` changes in addition to `popstate` diff --git a/integration/presets/longRunningApps.ts b/integration/presets/longRunningApps.ts index f3d9a8739a5..bb4f485540b 100644 --- a/integration/presets/longRunningApps.ts +++ b/integration/presets/longRunningApps.ts @@ -60,6 +60,7 @@ export const createLongRunningApps = () => { { id: 'react.vite.withEmailCodes', config: react.vite, env: envs.withEmailCodes }, { id: 'react.vite.withEmailCodes_persist_client', config: react.vite, env: envs.withEmailCodes_destroy_client }, { id: 'react.vite.withEmailLinks', config: react.vite, env: envs.withEmailLinks }, + { id: 'react.vite.withLegalConsent', config: react.vite, env: envs.withLegalConsent }, { id: 'vue.vite', config: vue.vite, env: envs.withCustomRoles }, /** diff --git a/integration/templates/react-vite/src/buttons/index.tsx b/integration/templates/react-vite/src/buttons/index.tsx index 5aa32d433cf..cb5e6de7be1 100644 --- a/integration/templates/react-vite/src/buttons/index.tsx +++ b/integration/templates/react-vite/src/buttons/index.tsx @@ -11,6 +11,15 @@ export default function Home() { Sign in button (force) + + Sign in button (force, popup) + + , }, + { + path: '/sign-in-popup/*', + element: , + }, { path: '/sign-up/*', element: , diff --git a/integration/templates/react-vite/src/sign-in-popup/index.tsx b/integration/templates/react-vite/src/sign-in-popup/index.tsx new file mode 100644 index 00000000000..b97b5841563 --- /dev/null +++ b/integration/templates/react-vite/src/sign-in-popup/index.tsx @@ -0,0 +1,15 @@ +import { SignIn } from '@clerk/clerk-react'; + +export default function Page() { + return ( +
+ Loading sign in} + /> +
+ ); +} diff --git a/integration/tests/oauth-flows.test.ts b/integration/tests/oauth-flows.test.ts index 6ff96a3cea2..77e97d8f8e5 100644 --- a/integration/tests/oauth-flows.test.ts +++ b/integration/tests/oauth-flows.test.ts @@ -181,6 +181,63 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('oauth flo }); }); +testAgainstRunningApps({ withPattern: ['react.vite.withLegalConsent'] })( + 'oauth popup with path-based routing @react', + ({ app }) => { + test.describe.configure({ mode: 'serial' }); + + let fakeUser: FakeUser; + + test.beforeAll(async () => { + const client = createClerkClient({ + secretKey: instanceKeys.get('oauth-provider').sk, + publishableKey: instanceKeys.get('oauth-provider').pk, + }); + const users = createUserService(client); + fakeUser = users.createFakeUser({ + withUsername: true, + }); + await users.createBapiUser(fakeUser); + }); + + test.afterAll(async () => { + const u = createTestUtils({ app }); + await fakeUser.deleteIfExists(); + await u.services.users.deleteIfExists({ email: fakeUser.email }); + await app.teardown(); + }); + + test('popup OAuth navigates through sso-callback with path-based routing', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.page.goToRelative('/sign-in-popup'); + await u.page.waitForClerkJsLoaded(); + await u.po.signIn.waitForMounted(); + + const popupPromise = context.waitForEvent('page'); + await u.page.getByRole('button', { name: 'E2E OAuth Provider' }).click(); + const popup = await popupPromise; + const popupUtils = createTestUtils({ app, page: popup, context }); + await popupUtils.page.getByText('Sign in to oauth-provider').waitFor(); + + // Complete OAuth in the popup + await popupUtils.po.signIn.setIdentifier(fakeUser.email); + await popupUtils.po.signIn.continue(); + await popupUtils.po.signIn.enterTestOtpCode(); + + // Because the user is new to the app and legal consent is required, + // the sign-up can't complete in the popup. The popup sends return_url + // back to the parent, which navigates to /sso-callback via pushState. + await u.page.getByRole('heading', { name: 'Legal consent' }).waitFor(); + await u.page.getByLabel(/I agree to the/).check(); + await u.po.signIn.continue(); + + await u.page.waitForAppUrl('/protected'); + await u.po.expect.toBeSignedIn(); + }); + }, +); + testAgainstRunningApps({ withEnv: [appConfigs.envs.withLegalConsent] })( 'oauth flows with legal consent @nextjs', ({ app }) => { diff --git a/packages/clerk-js/src/ui/hooks/index.ts b/packages/clerk-js/src/ui/hooks/index.ts index 7b531d210e4..796773dd1d4 100644 --- a/packages/clerk-js/src/ui/hooks/index.ts +++ b/packages/clerk-js/src/ui/hooks/index.ts @@ -17,4 +17,3 @@ export * from './useSafeState'; export * from './useScrollLock'; export * from './useSearchInput'; export * from './useTotalEnabledAuthMethods'; -export * from './useWindowEventListener'; diff --git a/packages/clerk-js/src/ui/hooks/useWindowEventListener.ts b/packages/clerk-js/src/ui/hooks/useWindowEventListener.ts deleted file mode 100644 index 8426b1b0175..00000000000 --- a/packages/clerk-js/src/ui/hooks/useWindowEventListener.ts +++ /dev/null @@ -1,15 +0,0 @@ -import React from 'react'; -type EventType = keyof WindowEventMap; - -export const useWindowEventListener = (eventOrEvents: EventType | EventType[] | undefined, cb: () => void): void => { - React.useEffect(() => { - const events = [eventOrEvents].flat().filter(x => !!x); - if (!events.length) { - return; - } - events.forEach(e => window.addEventListener(e, cb)); - return () => { - events.forEach(e => window.removeEventListener(e, cb)); - }; - }, [eventOrEvents, cb]); -}; diff --git a/packages/clerk-js/src/ui/router/BaseRouter.tsx b/packages/clerk-js/src/ui/router/BaseRouter.tsx index 734fc138937..4dc15538478 100644 --- a/packages/clerk-js/src/ui/router/BaseRouter.tsx +++ b/packages/clerk-js/src/ui/router/BaseRouter.tsx @@ -1,21 +1,140 @@ import { useClerk } from '@clerk/shared/react'; import type { NavigateOptions } from '@clerk/shared/types'; import React from 'react'; +import { flushSync } from 'react-dom'; import { getQueryParams, stringifyQueryParams, trimTrailingSlash } from '../../utils'; -import { useWindowEventListener } from '../hooks'; import { newPaths } from './newPaths'; import { match } from './pathToRegexp'; import { Route } from './Route'; import { RouteContext } from './RouteContext'; +// Custom events that don't exist on WindowEventMap but are handled +// by wrapping history.pushState/replaceState in the fallback path. +type HistoryEvent = 'pushstate' | 'replacestate'; +type RefreshEvent = keyof WindowEventMap | HistoryEvent; +type NavigationType = 'push' | 'replace' | 'traverse'; + +const isWindowRefreshEvent = (event: RefreshEvent): event is keyof WindowEventMap => { + return event !== 'pushstate' && event !== 'replacestate'; +}; + +// Maps refresh events to Navigation API navigationType values. +const eventToNavigationType: Partial> = { + popstate: 'traverse', + pushstate: 'push', + replacestate: 'replace', +}; + +// Global subscription sets for the history monkey-patching fallback. +// Using a single patch with subscriber sets avoids conflicts when +// multiple BaseRouter instances mount simultaneously. +const pushStateSubscribers = new Set<() => void>(); +const replaceStateSubscribers = new Set<() => void>(); +let originalPushState: History['pushState'] | null = null; +let originalReplaceState: History['replaceState'] | null = null; + +function ensurePushStatePatched(): void { + if (originalPushState) { + return; + } + const original = history.pushState.bind(history); + originalPushState = original; + history.pushState = (...args: Parameters) => { + original(...args); + pushStateSubscribers.forEach(fn => fn()); + }; +} + +function ensureReplaceStatePatched(): void { + if (originalReplaceState) { + return; + } + const original = history.replaceState.bind(history); + originalReplaceState = original; + history.replaceState = (...args: Parameters) => { + original(...args); + replaceStateSubscribers.forEach(fn => fn()); + }; +} + +/** + * Observes history changes so the router's internal state stays in sync + * with the URL. Uses the Navigation API when available, falling back to + * monkey-patching history.pushState/replaceState plus native window events. + * + * Note: `events` should be a stable array reference to avoid + * re-subscribing on every render. + */ +function useHistoryChangeObserver(events: Array | undefined, callback: () => void): void { + const callbackRef = React.useRef(callback); + callbackRef.current = callback; + + React.useEffect(() => { + if (!events) { + return; + } + + const notify = () => callbackRef.current(); + const windowEvents = events.filter(isWindowRefreshEvent); + const navigationTypes = events + .map(e => eventToNavigationType[e]) + .filter((type): type is NavigationType => Boolean(type)); + + const hasNavigationAPI = + typeof window !== 'undefined' && + 'navigation' in window && + typeof (window as any).navigation?.addEventListener === 'function'; + + if (hasNavigationAPI) { + const nav = (window as any).navigation; + const allowedTypes = new Set(navigationTypes); + const handler = (e: { navigationType: NavigationType }) => { + if (allowedTypes.has(e.navigationType)) { + Promise.resolve().then(notify); + } + }; + nav.addEventListener('currententrychange', handler); + + // Events without a navigationType mapping (e.g. hashchange) still + // need native listeners even when the Navigation API is available. + const unmappedEvents = windowEvents.filter(e => !eventToNavigationType[e]); + unmappedEvents.forEach(e => window.addEventListener(e, notify)); + + return () => { + nav.removeEventListener('currententrychange', handler); + unmappedEvents.forEach(e => window.removeEventListener(e, notify)); + }; + } + + // Fallback: use global subscriber sets for pushState/replaceState + // so that multiple BaseRouter instances don't conflict. + if (events.includes('pushstate')) { + ensurePushStatePatched(); + pushStateSubscribers.add(notify); + } + if (events.includes('replacestate')) { + ensureReplaceStatePatched(); + replaceStateSubscribers.add(notify); + } + + windowEvents.forEach(e => window.addEventListener(e, notify)); + + return () => { + pushStateSubscribers.delete(notify); + replaceStateSubscribers.delete(notify); + windowEvents.forEach(e => window.removeEventListener(e, notify)); + }; + }, [events]); +} + interface BaseRouterProps { basePath: string; startPath: string; getPath: () => string; getQueryString: () => string; internalNavigate: (toURL: URL, options?: NavigateOptions) => Promise | any; - refreshEvents?: Array; + refreshEvents?: Array; preservedParams?: string[]; urlStateParam?: { startPath: string; @@ -86,7 +205,23 @@ export const BaseRouter = ({ } }, [currentPath, currentQueryString, getPath, getQueryString]); - useWindowEventListener(refreshEvents, refresh); + // 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. + const isNavigatingRef = React.useRef(false); + + const observerRefresh = React.useCallback((): void => { + if (isNavigatingRef.current) { + return; + } + const newPath = getPath(); + if (basePath && !newPath.startsWith('/' + basePath)) { + return; + } + refresh(); + }, [basePath, getPath, refresh]); + + useHistoryChangeObserver(refreshEvents, observerRefresh); // TODO: Look into the real possible types of globalNavigate const baseNavigate = async (toURL: URL | undefined): Promise => { @@ -116,9 +251,20 @@ export const BaseRouter = ({ toURL.search = stringifyQueryParams(toQueryParams); } - const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } }); - setRouteParts({ path: toURL.pathname, queryString: toURL.search }); - return internalNavRes; + isNavigatingRef.current = true; + try { + const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } }); + // We need to flushSync to guarantee the re-render happens before handing things back to the caller, + // otherwise setActive might emit, and children re-render with the old navigation state. + // An alternative solution here could be to return a deferred promise, set that to state together + // with the routeParts and resolve it in an effect. That way we could avoid the flushSync performance penalty. + flushSync(() => { + setRouteParts({ path: toURL.pathname, queryString: toURL.search }); + }); + return internalNavRes; + } finally { + isNavigatingRef.current = false; + } }; return ( diff --git a/packages/clerk-js/src/ui/router/PathRouter.tsx b/packages/clerk-js/src/ui/router/PathRouter.tsx index c34f5a404b0..b1c48e48b57 100644 --- a/packages/clerk-js/src/ui/router/PathRouter.tsx +++ b/packages/clerk-js/src/ui/router/PathRouter.tsx @@ -59,7 +59,7 @@ export const PathRouter = ({ basePath, preservedParams, children }: PathRouterPr getPath={getPath} getQueryString={getQueryString} internalNavigate={internalNavigate} - refreshEvents={['popstate']} + refreshEvents={['pushstate', 'replacestate', 'popstate']} preservedParams={preservedParams} > {children}