fix(clerk-js,shared): Improve ticket and SSO types for Future APIs#8267
fix(clerk-js,shared): Improve ticket and SSO types for Future APIs#8267
Conversation
🦋 Changeset detectedLatest commit: 9b0f086 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request updates SignIn and SignUp resource implementations and their type definitions. In SignIn, creation is now conditional: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
1128-1154:⚠️ Potential issue | 🔴 Critical
identifieris dropped whensso()reuses an existing sign-inLine 1128 reads
identifier, but Lines 1148-1154 only send it through_create()when noidexists. Ifidalready exists,identifieris never applied, which can break enterprise SSO targeting and reuse stale state.Suggested fix
- if (!this.#resource.id) { + if (!this.#resource.id || Boolean(identifier)) { await this._create({ strategy, ...routes, identifier, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clerk-js/src/core/resources/SignIn.ts` around lines 1128 - 1154, The current flow only sends identifier to the server when creating a new SignIn via this._create, so when this.#resource.id exists the identifier is dropped; modify the logic after computing routes (and after popup wrapping) to ensure identifier is applied for existing sign-ins by calling the existing update/patch method (e.g., this._update or the internal update helper) with { identifier } when this.#resource.id is present, so identifier is always sent whether creating (this._create) or reusing an existing sign-in; keep the popup/wrapped routes logic unchanged and only add the identifier update for the existing resource case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clerk-js/src/core/resources/SignUp.ts`:
- Around line 1029-1045: Add unit/integration tests that cover both SSO request
branches introduced in SignUp.ts: one test ensuring when this.#resource.id is
present the code calls __internal_basePatch with the expected path and body
(existing-resource SSO flow), and another ensuring when this.#resource.id is
absent it calls __internal_basePost with the expected path and body
(new-resource SSO flow); locate the behavior around the id-dependent branching
and assert the correct method (__internal_basePatch vs __internal_basePost),
request path (this.#resource.pathRoot), and body contents (strategy, routes,
unsafeMetadata, oidcPrompt, enterpriseConnectionId, emailAddress, captchaToken,
captchaWidgetType, captchaError) are sent.
---
Outside diff comments:
In `@packages/clerk-js/src/core/resources/SignIn.ts`:
- Around line 1128-1154: The current flow only sends identifier to the server
when creating a new SignIn via this._create, so when this.#resource.id exists
the identifier is dropped; modify the logic after computing routes (and after
popup wrapping) to ensure identifier is applied for existing sign-ins by calling
the existing update/patch method (e.g., this._update or the internal update
helper) with { identifier } when this.#resource.id is present, so identifier is
always sent whether creating (this._create) or reusing an existing sign-in; keep
the popup/wrapped routes logic unchanged and only add the identifier update for
the existing resource case.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a203b9ab-0078-40a0-a968-1c8c503b9e06
📒 Files selected for processing (4)
packages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/resources/SignUp.tspackages/shared/src/types/signInFuture.tspackages/shared/src/types/signUpFuture.ts
| const body: Record<string, unknown> = { | ||
| strategy, | ||
| ...routes, | ||
| unsafeMetadata, | ||
| legalAccepted, | ||
| oidcPrompt, | ||
| enterpriseConnectionId, | ||
| emailAddress, | ||
| captchaToken, | ||
| captchaWidgetType, | ||
| captchaError, | ||
| }; | ||
| if (this.#resource.id) { | ||
| return this.#resource.__internal_basePatch({ path: this.#resource.pathRoot, body }); | ||
| } | ||
| return this.#resource.__internal_basePost({ path: this.#resource.pathRoot, body }); | ||
| }; |
There was a problem hiding this comment.
Add regression tests for the new SSO id-dependent request branching
Lines 1041-1045 changed request behavior (PATCH vs POST) based on existing id, but no tests are included in this PR context for these auth-path branches. Please add coverage for both existing-resource and new-resource SSO flows before merge.
As per coding guidelines "If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/clerk-js/src/core/resources/SignUp.ts` around lines 1029 - 1045, Add
unit/integration tests that cover both SSO request branches introduced in
SignUp.ts: one test ensuring when this.#resource.id is present the code calls
__internal_basePatch with the expected path and body (existing-resource SSO
flow), and another ensuring when this.#resource.id is absent it calls
__internal_basePost with the expected path and body (new-resource SSO flow);
locate the behavior around the id-dependent branching and assert the correct
method (__internal_basePatch vs __internal_basePost), request path
(this.#resource.pathRoot), and body contents (strategy, routes, unsafeMetadata,
oidcPrompt, enterpriseConnectionId, emailAddress, captchaToken,
captchaWidgetType, captchaError) are sent.
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
looks good, left a question!
| if (!this.#resource.id) { | ||
| await this._create({ | ||
| strategy, | ||
| ...routes, | ||
| identifier, | ||
| }); | ||
| } |
There was a problem hiding this comment.
So before this change, we always created a fresh SSO attempt here, even when a SignIn already existed?
There was a problem hiding this comment.
yes, in the new hooks (the previous hooks had this same "if existing, update, otherwise create" logic). I think the ticket flow is the only instance in which you'd want to SSO into an existing sign-in attempt.
2 participants
Description
This PR improves compatibility with tickets in the new SignInFuture and SignUpFuture APIs. It adds the following:
passwordparameter when usingsignIn.create()strategy: 'ticket'when usingsignIn.create()strategyparameter when usingsignUp.create()passwordparameter when usingsignUp.create()signIn.sso()Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change