fix(js): preserve SignUpFuture reference across SSO-to-sign-up transition#9060
fix(js): preserve SignUpFuture reference across SSO-to-sign-up transition#9060FrancoKaddour wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 9cd68a6 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR patches ChangesStale SignUp Reference Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: Resolves 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
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/Client.ts (2)
146-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix logic correctly addresses the stale
SignUpreference bug.The broadened condition
if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || !this.signUp.id))correctly preserves the existingSignUpinstance (and thus theSignUpFuturereference) when the current instance has no id yet, matching the fix described in the PR objectives. The downstreampath()/isNew()mechanics inBase.tsmean this instance will now correctly route PATCH requests to/client/sign_ups/{id}once the id arrives.One minor readability nit:
BaseResourcealready exposes a publicisNew()helper (return !this.id) that is used elsewhere for the same semantic check (e.g.path()). Usingthis.signUp.isNew()instead of!this.signUp.idhere would be more idiomatic and self-documenting.♻️ Optional refactor for consistency
- if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || !this.signUp.id)) { + if (data.sign_up && this.signUp instanceof SignUp && (this.signUp.id === data.sign_up.id || this.signUp.isNew())) {🤖 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` around lines 146 - 154, The SignUp update branch in Client should use the existing resource helper for the “no id yet” check instead of reading the id directly. In the conditional around this.signUp.__internal_updateFromJSON, replace the raw !this.signUp.id part with this.signUp.isNew() so the logic matches BaseResource semantics and stays consistent with path() and other resource checks.
141-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the no-id → id sign-up transition. Existing coverage checks matching ids, but not the case where
this.signUp.idis unset andfromJSON()receives asign_uppayload. A unit test asserting the sameSignUpinstance is reused and itsidis populated would lock in the SSO→sign-up flow behavior.🤖 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` around lines 141 - 171, Add a regression test for Client.fromJSON covering the no-id to id sign-up transition: exercise the branch where this.signUp is an existing SignUp with no id and data.sign_up has an id, and verify the same SignUp instance is reused rather than replaced. Assert that __internal_updateFromJSON is effectively applied by checking the original SignUp reference is preserved and its id is populated after the call, alongside the existing matching-id coverage.
🤖 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/shared/src/internal/clerk-js/url.ts`:
- Around line 449-455: The allowlist check in the URL validation logic is too
permissive because `portlessOrigin` is being matched for every pattern, causing
exact entries in the `isAllowed` flow to accept any port on the same host.
Update the logic in `url.ts` so only explicit wildcard or dev-only allowlist
entries can use the portless fallback, while exact allowlist strings remain
port-sensitive. Keep the fix localized to the `isAllowed` computation and the
`patterns.some(...)` matching path so `https://www.clerk.com` does not
implicitly allow `https://www.clerk.com:3000`.
---
Nitpick comments:
In `@packages/clerk-js/src/core/resources/Client.ts`:
- Around line 146-154: The SignUp update branch in Client should use the
existing resource helper for the “no id yet” check instead of reading the id
directly. In the conditional around this.signUp.__internal_updateFromJSON,
replace the raw !this.signUp.id part with this.signUp.isNew() so the logic
matches BaseResource semantics and stays consistent with path() and other
resource checks.
- Around line 141-171: Add a regression test for Client.fromJSON covering the
no-id to id sign-up transition: exercise the branch where this.signUp is an
existing SignUp with no id and data.sign_up has an id, and verify the same
SignUp instance is reused rather than replaced. Assert that
__internal_updateFromJSON is effectively applied by checking the original SignUp
reference is preserved and its id is populated after the call, alongside the
existing matching-id coverage.
🪄 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: e0af06f2-d013-4b37-b8e8-8e4a7c8c9fc7
📒 Files selected for processing (5)
.changeset/fix-allowed-redirect-origins-port.md.changeset/fix-signup-future-sso-transfer-stale-ref.mdpackages/clerk-js/src/core/resources/Client.tspackages/shared/src/internal/clerk-js/__tests__/url.test.tspackages/shared/src/internal/clerk-js/url.ts
…tion
When an SSO sign-in transitions to a sign-up flow (e.g. account does not
exist, legal acceptance required), Client.fromJSON created a brand-new
SignUp instance because the existing one had no id and the incoming data
did. The old SignUpFuture held by useSignUp() hooks pointed at the stale
id-less instance, so subsequent update() calls sent PATCH to
/client/sign_ups instead of /client/sign_ups/{id} and received a 405.
Fix: update the existing SignUp in-place (via __internal_updateFromJSON)
when it has no id yet, which preserves the SignUpFuture reference and
ensures hooks see the correct id after the transition.
Fixes clerk#8338
FrancoKaddour
force-pushed
the
fix/signup-future-sso-transfer-stale-ref
branch
from
167447f to
9cd68a6
Compare
1 participant
Fixes #8338
When an SSO sign-in transitions to a sign-up flow (e.g. the account does not exist and legal acceptance is required), Client.fromJSON created a brand-new SignUp instance because the existing one had no id while the incoming server data did. The SignUpFuture held by useSignUp() hooks pointed at the stale, id-less instance. Calling signUp.update({ legalAccepted: true }) then sent PATCH /client/sign_ups (no id segment) instead of PATCH /client/sign_ups/{id}, resulting in a 405 error.
Root cause: Client.fromJSON only reuses the existing SignUp instance when this.signUp.id === data.sign_up.id. During SSO transfer the old instance has no id, so the condition was false and a new instance was created — discarding the SignUpFuture reference held by hooks.
Fix: Also update in-place when !this.signUp.id (the instance is still uninitialized). __internal_updateFromJSON populates all fields including id, so the existing SignUpFuture transparently sees the correct id after the transition.
Summary by CodeRabbit