fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations#5515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstaley merged 6 commits into from
Apr 3, 2025
Merged

fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97a0a0b
fix(clerk-js): Use __internal_setActiveInProgress to prevent navigations
dstaley Apr 2, 2025
319465e
fix(clerk-js): Use ssoCallbackUrl if present in context
dstaley Apr 2, 2025
f07bdeb
fix(react): Add __internal_setActiveInProgress to list of excluded
dstaley Apr 2, 2025
230c91e
fix(clerk-js): Bump bundle size limit
dstaley Apr 2, 2025
92cf50c
fix(clerk-js): typo
dstaley Apr 2, 2025
e03f9a2
Merge branch 'main' into ds.fix/setactive-in-progress
dstaley Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-lamps-sneeze.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
---
'@clerk/clerk-js': patch
'@clerk/clerk-react': patch
'@clerk/types': patch
---

Fixes an issue where a race condition was caused by triggering navigations during a call to `setActive`.
5 changes: 5 additions & 0 deletions .changeset/nice-clubs-lead.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where the SSO callback URL was incorrectly set for combined sign-in/sign-up.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "584.7kB" },
{ "path": "./dist/clerk.js", "maxSize": "590kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "81KB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
Expand Down
201 changes: 104 additions & 97 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -231,6 +231,8 @@ export class Clerk implements ClerkInterface {
public __internal_isWebAuthnAutofillSupported: (() => Promise<boolean>) | undefined;
public __internal_isWebAuthnPlatformAuthenticatorSupported: (() => Promise<boolean>) | undefined;

public __internal_setActiveInProgress = false;

get publishableKey(): string {
return this.#publishableKey;
}
Expand DownExpand Up@@ -964,122 +966,127 @@ export class Clerk implements ClerkInterface {
* `setActive` can be used to set the active session and/or organization.
*/
public setActive = async ({ session, organization, beforeEmit, redirectUrl }: SetActiveParams): Promise<void> => {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}
this.__internal_setActiveInProgress = true;
try {
if (!this.client) {
throw new Error('setActive is being called before the client is loaded. Wait for init.');
}

if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}
if (session === undefined && !this.session) {
throw new Error(
'setActive should either be called with a session param or there should be already an active session.',
);
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;
const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}
if (typeof session === 'string') {
session = (this.client.sessions.find(x => x.id === session) as SignedInSessionResource) || null;
}

let newSession = session === undefined ? this.session : session;
let newSession = session === undefined ? this.session : session;

// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;
// At this point, the `session` variable should contain either an `SignedInSessionResource`
// ,`null` or `undefined`.
// We now want to set the last active organization id on that session (if it exists).
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;
if (newSession && shouldSwitchOrganization) {
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}
}

if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}
if (newSession?.status === 'pending') {
await this.#handlePendingSession(newSession);
return;
}

if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}
if (session?.lastActiveToken) {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}

//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
// id.
if (inActiveBrowserTab() || !this.#options.standardBrowser) {
await this.#touchCurrentSession(newSession);
// reload session from updated client
newSession = this.#getSessionFromClient(newSession?.id);
}
// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}

// getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event.
const token = await newSession?.getToken();
if (!token) {
eventBus.dispatch(events.TokenUpdate, { token: null });
}
//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

//2. If there's a beforeEmit, typically we're navigating. Emit the session as
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);
if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}
if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

if (redirectUrl && !beforeEmit) {
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
} finally {
this.__internal_setActiveInProgress = false;
}

this.#setAccessors(newSession);
this.#emit();
await onAfterSetActive();
};

#handlePendingSession = async (session: PendingSessionResource) => {
Expand Down
12 changes: 7 additions & 5 deletions packages/clerk-js/src/ui/common/SSOCallback.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,16 +17,18 @@ export const SSOCallback = withCardStateProvider<HandleOAuthCallbackParams | Han
});

export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCallbackParams) => {
const { handleRedirectCallback } = useClerk();
const { handleRedirectCallback, __internal_setActiveInProgress } = useClerk();
const { navigate } = useRouter();
const card = useCardState();

React.useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout>;
handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
if (__internal_setActiveInProgress !== true) {

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why don't we do this in our router instead?

@dstaleydstaleyApr 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely explore something like that, which I feel is more intelligent than this blunt approach. It'd likely use the same flag too, so we're good there. The main reason I went with this instead of scoping this purely to preventing navigations is I'm concerned we have more functionality that needs to be prevented from executing a second time. For example, the PR as is prevents the execution of handleRedirectCallback, rather than simply preventing a navigation from it. handleRedirectCallback has several non-navigation actions that it performs that it would be best if they were prevented completely (stuff like signIn.reload, signIn.create, signUp.create, setActive, etc).

handleRedirectCallback({ ...props }, navigate).catch(e => {
handleError(e, [], card.setError);
timeoutId = setTimeout(() => void navigate('../'), 4000);
});
}

return () => clearTimeout(timeoutId);
}, [handleError, handleRedirectCallback]);
Expand Down
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/redirects.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -207,5 +207,10 @@ describe('buildSSOCallbackURL(ctx, baseUrl)', () => {
expect(
buildSSOCallbackURL({ routing: 'virtual', authQueryString: 'redirect_url=%2Ffoo' }, 'http://test.host'),
).toBe('http://test.host/#/sso-callback?redirect_url=%2Ffoo');

// Custom SSO callback URL in the context
expect(buildSSOCallbackURL({ ssoCallbackUrl: 'http://test.host/ctx-sso-callback' })).toBe(
'http://test.host/ctx-sso-callback',
);
});
});
5 changes: 5 additions & 0 deletions packages/clerk-js/src/ui/common/redirects.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,11 @@ export function buildSSOCallbackURL(
ctx: Partial<SignInContextType | SignUpContextType>,
baseUrl: string | undefined = '',
): string {
// If the context contains an SSO callback URL, use it instead of building a new one, as it likely contains the
// combined flow path.
if ('ssoCallbackUrl' in ctx && ctx.ssoCallbackUrl) {
return ctx.ssoCallbackUrl;
}
Comment on lines +57 to +61

@brkalowbrkalowApr 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch

const { routing, authQueryString, path } = ctx;
return buildRedirectUrl({
routing,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/components/SignUp/SignUpContinue.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ function SignUpContinueInternal() {

useEffect(() => {
// Redirect to sign-up if there is no persisted sign-up
if (!signUp.id) {
if (!signUp.id && clerk.__internal_setActiveInProgress !== true) {
void navigate(displayConfig.signUpUrl);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/isomorphicClerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,7 @@ type IsomorphicLoadedClerk = Without<
| '__internal_reloadInitialResources'
| '__experimental_commerce'
| '__internal_setComponentNavigationContext'
| '__internal_setActiveInProgress'
> & {
client: ClientResource | undefined;
__experimental_commerce: __experimental_CommerceNamespace | undefined;
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/clerk.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -660,6 +660,12 @@ export interface Clerk {
* @internal
*/
__internal_reloadInitialResources: () => Promise<void>;

/**
* Internal flag indicating whether a `setActive` call is in progress. Used to prevent navigations from being
* initiated outside of the Clerk class.
*/
__internal_setActiveInProgress: boolean;
}

export type HandleOAuthCallbackParams = TransferableOption &
Expand Down