Skip to content

fix(clerk-js): Ensure sessionId is set after sign in #7117

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

Closed
wants to merge 12 commits into from
Closed

fix(clerk-js): Ensure sessionId is set after sign in #7117

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f0d9553
fix(clerk-js): ensure sessionId is available immeditately after sign in
jacekradko Oct 31, 2025
7964a90
set transitive state when switching orgs
jacekradko Oct 31, 2025
6397235
Merge branch 'main' into fix/null-session-id-after-sign-in
jacekradko Nov 3, 2025
5112830
Merge branch 'main' into fix/null-session-id-after-sign-in
jacekradko Nov 3, 2025
74575b8
remove preliminary client optimization
jacekradko Nov 4, 2025
d29ec53
changeset
jacekradko Nov 4, 2025
a71bb0f
Merge branch 'main' into fix/null-session-id-after-sign-in
jacekradko Nov 4, 2025
fc11bb5
add explanation
jacekradko Nov 4, 2025
8b3df51
add test coverage
jacekradko Nov 4, 2025
89fc47e
more tests
jacekradko Nov 4, 2025
d7b3918
CR suggestion
jacekradko Nov 4, 2025
51b2f6a
Merge branch 'main' into fix/null-session-id-after-sign-in
jacekradko Nov 4, 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
17 changes: 15 additions & 2 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,7 +1412,7 @@ export class Clerk implements ClerkInterface {
return;
}

if (newSession?.status !== 'pending') {
if (newSession?.status !== 'pending' && (this.session?.id !== newSession?.id || shouldSwitchOrganization)) {
Copy link
Member

@brkalow brkalow Nov 3, 2025

Choose a reason for hiding this comment

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

what problem is this addressing? The transitive state sets undefined, not null. If we're not entirely sure, I would recommend that we remove this.

Copy link
Contributor

@panteliselef panteliselef Nov 3, 2025

Choose a reason for hiding this comment

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

This is unclear to me as well. Could this cause a chain of events that result to null eventually ?

Copy link
Member Author

@jacekradko jacekradko Nov 4, 2025

Choose a reason for hiding this comment

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

This check prevents calling #setTransitiveState() unnecessarily. It ensures we only clear the transitive state (which sets the session to undefined) when the session is changing or the org is changing.

Without it you get unnecessary session → undefined → session when nothing changed

Copy link

@Ephem Ephem Nov 4, 2025

Choose a reason for hiding this comment

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

Is this what triggered during the sign-in, causing session to be undefined when the updateClient call ran, causing it to not update the session?

So this PR kind of fixes the sign-in bug from two separate angles?

Copy link
Member Author

@jacekradko jacekradko Nov 4, 2025

Choose a reason for hiding this comment

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

@Ephem Exactly! Without this, every navigation with setActive() would clear the session unnecessarily, opening the window for a race condition where updateClient() would run before setActive() can set and emit the session

this.#setTransitiveState();
}

Expand Down Expand Up @@ -2351,7 +2351,9 @@ export class Clerk implements ClerkInterface {
}

updateClient = (newClient: ClientResource): void => {
if (!this.client) {
const isFirstClientSet = !this.client;

if (isFirstClientSet) {
// This is the first time client is being
// set, so we also need to set session
const session = this.#options.selectInitialSession
Expand Down Expand Up @@ -2381,6 +2383,11 @@ export class Clerk implements ClerkInterface {

// A client response contains its associated sessions, along with a fresh token, so we dispatch a token update event.
eventBus.emit(events.TokenUpdate, { token: this.session?.lastActiveToken });
} else if (!isFirstClientSet && newClient.sessions?.length > 0) {
const session = this.#options.selectInitialSession
? this.#options.selectInitialSession(newClient)
: this.#defaultSession(newClient);
this.#setAccessors(session);
Comment on lines 2424 to 2431
Copy link
Member

@brkalow brkalow Nov 3, 2025

Choose a reason for hiding this comment

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

do we know why this is new case is necessary? The client is being updated, which has a new session to be set, but the session accessor wasn't yet set elsewhere?

Copy link
Member Author

@jacekradko jacekradko Nov 4, 2025

Choose a reason for hiding this comment

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

Yes, it is. This handles the case where updateClient() is executed during touch(). In that call we get the piggybacked client data. The key here is that this is an else branch of if (this.session) { which we do not have at this point.

So without this else, we do nothing here and just this.#emit(); an empty session.

Copy link

@Ephem Ephem Nov 4, 2025

Choose a reason for hiding this comment

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

Oh! Had to dig through the code and think for a bit to get it, but:

  • First updateClient is guaranteed to set a session
  • But if session is later removed, for example because of transitive state..
  • ..and updateClient is called during that time (touch), we currently don't set the new session?

Very nice find. Adding a comment might be helpful here.

Copy link
Contributor

@panteliselef panteliselef Nov 4, 2025

Choose a reason for hiding this comment

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

Shouldn't this be covered by updateClient ?

Copy link

@Ephem Ephem Nov 4, 2025

Choose a reason for hiding this comment

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

That's what this PR adds. It might be easier to grok looking at it like this:

  updateClient = (newClient: ClientResource): void => {
    const isFirstClientSet = !this.client;

    if (isFirstClientSet) {
      // If first time setting client, also set session
      // Note: This does _not_ trigger during transitive state as we still have a client
    }
    this.client = newClient;

    if (this.session) {
      // Previously we only set a new session if we already had an active session,
      // which we don't during transitive state
    } else if (!isFirstClientSet && newClient.sessions?.length > 0) {
      // Now, if client is set, but not session, and there is a new session to set, set it
    }

    this.#emit();
  };

Copy link
Member Author

@jacekradko jacekradko Nov 4, 2025

Choose a reason for hiding this comment

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

@Ephem that's a pretty good summary. This is my mental model of this mechanism:

updateClient = (newClient: ClientResource): void => {
  const isFirstClientSet = !this.client;

  if (isFirstClientSet) {
    // First time setting client (during initialization)
    // Also set the initial session from the new client
  }

  this.client = newClient;

  if (this.session) {
    // We have an active session - refresh it with updated data from newClient
    // This is the normal path during regular operation
  } else if (!isFirstClientSet && newClient.sessions?.length > 0) {
    // Race condition recovery path:
    // - Client was already set (so NOT first initialization)
    // - Session is currently falsy (e.g., cleared by setTransitiveState during setActive)
    // - BUT newClient has session data (e.g., from touch() response that piggybacked)
    // → Restore the session immediately to prevent null flash
  }

  this.#emit();
};

Copy link

@Ephem Ephem Nov 5, 2025

Choose a reason for hiding this comment

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

Thinking about this some more, I realize I still don't understand how the this.session goes from undefined to null because of this? Shouldn't the this.#emit be emitting the existing session, which is undefined? 🤔

Copy link

@Ephem Ephem Nov 6, 2025

Choose a reason for hiding this comment

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

I think I found the reason here: #7157 (comment)

(Just crossposting here to keep a good history if someone stumbles across this in the future)

}

this.#emit();
Expand Down Expand Up @@ -2591,6 +2598,12 @@ export class Clerk implements ClerkInterface {
});

const initClient = async () => {
const jwtInCookie = this.#authService?.getSessionCookie();
if (jwtInCookie) {
const preliminaryClient = createClientFromJwt(jwtInCookie);
this.updateClient(preliminaryClient);
}

return Client.getOrCreateInstance()
.fetch()
.then(res => this.updateClient(res))
Expand Down
Loading