fix(js): preserve SignInFuture reference after resetSignIn#9071
fix(js): preserve SignInFuture reference after resetSignIn#9071FrancoKaddour wants to merge 1 commit into
Conversation
After resetSignIn(), fromJSON was creating a new SignIn instance when the next attempt arrived (ids didn't match), breaking the __internal_future reference held by useSignIn() hooks. Add `|| !this.signIn.id` so an empty-id SignIn is updated in-place instead, same pattern as SignUp. Fixes clerk#9006
🦋 Changeset detectedLatest commit: 9bbd637 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@FrancoKaddour is attempting to deploy a commit to the Clerk Production Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR modifies ChangesSignIn Instance Reuse Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Hook as useSignIn()
participant Client as Client
participant SignIn as SignIn instance
Hook->>Client: resetSignIn()
Client->>SignIn: clear id (id becomes undefined)
Hook->>Client: fromJSON(data with sign_in)
alt id matches or this.signIn has no id
Client->>SignIn: __internal_updateFromJSON(data.sign_in)
SignIn-->>Hook: same instance updated (id, identifier)
else
Client->>Client: replace this.signIn with new SignIn
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/clerk-js/src/core/resources/Client.ts`:
- Line 152: The SignUp reuse logic in Client.fromJSON is still too strict and
replaces the existing SignUp instance after resetSignUp(), which drops the
__internal_future used by useSignUp(). Update the sign_up branch to mirror the
SignIn handling by reusing this.signUp when the ids match or when this.signUp.id
is falsy, using the existing SignUp and fromJSON paths. Add a test covering
resetSignUp() followed by a new attempt id to ensure the same SignUp instance is
preserved and its internal future remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 91a92164-23cb-4afb-9463-f5b2fc0eeb2e
📒 Files selected for processing (3)
.changeset/fix-signin-future-stale-ref-after-reset.mdpackages/clerk-js/src/core/resources/Client.tspackages/clerk-js/src/core/resources/__tests__/Client.test.ts
| } | ||
|
|
||
| if (data.sign_in && this.signIn instanceof SignIn && this.signIn.id === data.sign_in.id) { | ||
| if (data.sign_in && this.signIn instanceof SignIn && (this.signIn.id === data.sign_in.id || !this.signIn.id)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if resetSignUp + fromJSON exhibits the same stale-instance issue as resetSignIn did
rg -n -A5 'resetSignUp' packages/clerk-js/src/core/resources/Client.ts
rg -n 'this.signUp.id ===' packages/clerk-js/src/core/resources/Client.tsRepository: clerk/javascript
Length of output: 552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/clerk-js/src/core/resources/Client.tsRepository: clerk/javascript
Length of output: 6391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B4 'class SignUp|class SignIn|__internal_updateFromJSON|useSignUp|useSignIn' packages/clerk-js/src/core/resources packages/clerk-js/src -g '!**/dist/**'Repository: clerk/javascript
Length of output: 28599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/clerk-js/src/core/resources/SignUp.ts
sed -n '500,760p' packages/clerk-js/src/core/resources/SignUp.ts
sed -n '200,280p' packages/clerk-js/src/core/resources/__tests__/Client.test.tsRepository: clerk/javascript
Length of output: 19406
Align SignUp reuse with the SignIn fix
sign_up still only reuses the existing instance when ids match. After resetSignUp(), this.signUp.id is falsy, so the next fromJSON() with a new attempt id replaces the instance and drops the __internal_future reference that useSignUp() depends on. Add the same || !this.signUp.id guard here, plus a reset→new-attempt test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/clerk-js/src/core/resources/Client.ts` at line 152, The SignUp reuse
logic in Client.fromJSON is still too strict and replaces the existing SignUp
instance after resetSignUp(), which drops the __internal_future used by
useSignUp(). Update the sign_up branch to mirror the SignIn handling by reusing
this.signUp when the ids match or when this.signUp.id is falsy, using the
existing SignUp and fromJSON paths. Add a test covering resetSignUp() followed
by a new attempt id to ensure the same SignUp instance is preserved and its
internal future remains intact.
There was a problem hiding this comment.
Good catch. The signUp side is already covered by #9060 which adds the same || !this.signUp.id guard for that path — once that lands both flows will be protected.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh pr view 9060 --repo clerk/javascript --json title,body,state,url 2>&1 | head -100Length of output: 4839
@FrancoKaddour Confirmed — I checked #9060 and it does add the identical || !this.signUp.id guard to the sign_up branch in Client.fromJSON, addressing the same stale SignUpFuture issue for useSignUp(). Once that PR merges, both the signIn and signUp reuse paths will be consistent. No action needed here; I'll leave this as a non-blocking note for tracking purposes.
(っ˘̩╭╮˘̩)っ
🧬 Code Graph Analysis Results
packages/clerk-js/src/core/resources/Client.ts
Lines 31-33 (sign-up/sign-in attempt instances live on Client)
sessions: Session[] = [];
signUp: SignUpResource = new SignUp();
signIn: SignInResource = new SignIn();Lines 141-156 (fromJSON in-place update logic; sign-in already uses the || !this.signIn.id guard, sign-up does not in this file)
fromJSON(data: ClientJSON | ClientJSONSnapshot | null): this {
if (data) {
this.id = data.id;
this.sessions = (data.sessions || []).map(s => new Session(s));
if (data.sign_up && this.signUp instanceof SignUp && this.signUp.id === data.sign_up.id) {
this.signUp.__internal_updateFromJSON(data.sign_up);
} else {
this.signUp = new SignUp(data.sign_up);
}
if (data.sign_in && this.signIn instanceof SignIn && (this.signIn.id === data.sign_in.id || !this.signIn.id)) {
this.signIn.__internal_updateFromJSON(data.sign_in);
} else {
this.signIn = new SignIn(data.sign_in);
}
this.lastActiveSessionId = data.last_active_session_id;
this.captchaBypass = data.captcha_bypass || false;
this.cookieExpiresAt = data.cookie_expires_at ? unixEpochToDate(data.cookie_expires_at) : null;
this.lastAuthenticationStrategy = data.last_authentication_strategy || null;
this.createdAt = unixEpochToDate(data.created_at || undefined);
this.updatedAt = unixEpochToDate(data.updated_at || undefined);
}
return this;
}packages/clerk-js/src/core/resources/SignUp.ts
Line-range: 77-610 (relevant behavior: SignUp tracks id and populates it from API responses; this is what the Client.fromJSON check relies on)
export class SignUp extends BaseResource implements SignUpResource {
pathRoot = '/client/sign_ups';
id: string | undefined;
...
constructor(data: SignUpJSON | SignUpJSONSnapshot | null = null) {
super();
this.fromJSON(data);
}
...
protected fromJSON(data: SignUpJSON | SignUpJSONSnapshot | null): this {
if (data) {
this.id = data.id;
...
}
eventBus.emit('resource:update', { resource: this });
return this;
}
...
}Docstring-style summary of the relevant parts (from the same snippet range):
/**
* SignUpResource identity:
* - `id: string | undefined` (set from API payloads).
* - `constructor(data)` calls `fromJSON(data)`.
* - `fromJSON(...)` sets `this.id = data.id` when `data` is present and emits `resource:update`.
*
* Parameters:
* - data: SignUpJSON | SignUpJSONSnapshot | null
*
* Returns:
* - this (SignUp)
*
* Exceptions:
* - none shown in the provided excerpt.
*/🧠 Learnings used
Learnt from: wobsoriano
Repo: clerk/javascript PR: 7883
File: packages/backend/src/api/__tests__/M2MTokenApi.test.ts:414-419
Timestamp: 2026-02-24T18:03:27.067Z
Learning: In TypeScript files within the clerk/javascript repository, enforce explicit return type annotations for exported functions and public APIs, but allow internal test helper functions without explicit return types. This helps maintain API clarity and type safety for consumers while not burdening test helpers that are not part of the public surface.
1 participant
After resetSignIn(), the next call to fromJSON with fresh sign-in data was creating a brand-new SignIn instance because the id comparison this.signIn.id === data.sign_in.id) failed — the current sign-in had no id after the reset. This caused useSignIn() hooks to hold a stale __internal_future reference pointing at the discarded instance.
Fix: add || !this.signIn.id to the condition so an empty-id SignIn is updated in-place instead of replaced, preserving the __internal_future reference across the reset → new attempt transition. Same pattern as the existing SignUp fix.
Closes #9006
Summary by CodeRabbit