fix(clerk-js,shared,types): Support calling password without identifier #6534
Conversation
🦋 Changeset detectedLatest commit: 8437b3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 Git ↗︎
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughUpdates across packages add optional-identifier support and finalization/state to the sign-in future. In clerk-js, SignInFuture gains fetchStatus ('idle' | 'fetching'), an availableStrategies getter, password({ identifier?, password }) which falls back to the previous identifier, and a finalize() method that validates createdSessionId and calls clerk.setActive({ session }). In types, SignInFutureResource mirrors fetchStatus, availableStrategies, optional identifier in password, an expanded create params shape, and finalize(). In shared, isClerkAPIResponseError is made null-safe. Changeset bumps @clerk/clerk-js, @clerk/shared, and @clerk/types and adds an experimental note about signIn.password() without an identifier. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
1-714: Add missing tests for SignInFuture experimental APIsThe new
SignInFuturemethods and properties aren’t covered by any existing unit tests inpackages/clerk-js/src/core/resources. Please add test cases to verify:• The
password({ identifier?, password })flow both when an identifier is provided and when it isn’t
• Thefinalize()method correctly callssetActiveand handles missing session errors
• TheavailableStrategiesgetter returnssupportedFirstFactorsas expected
• ThefetchStatusproperty transitions through"idle" | "fetching" | "error"as operations start, succeed, or failSuggested locations:
- Create a new test file alongside
SignIn.ts, e.g.packages/clerk-js/src/core/resources/__tests__/SignInFuture.spec.ts- Or extend
Client.spec.ts/addSignIn.spec.tsunderpackages/clerk-js/src/core/resources/__tests__to include these scenarios
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
506-713: Consider updating fetchStatus during async operations.The
fetchStatusproperty is initialized but never updated during async operations. Consider setting it to 'fetching' at the start of async methods and back to 'idle' when they complete to provide accurate loading states to consumers.For example, in the
passwordmethod:async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> { + this.fetchStatus = 'fetching'; eventBus.emit('resource:error', { resource: this.resource, error: null }); const previousIdentifier = this.resource.identifier; try { await this.resource.__internal_basePost({ path: this.resource.pathRoot, body: { identifier: identifier || previousIdentifier, password }, }); } catch (err: unknown) { eventBus.emit('resource:error', { resource: this.resource, error: err }); + this.fetchStatus = 'idle'; return { error: err }; } + this.fetchStatus = 'idle'; return { error: null }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/young-cats-retire.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(3 hunks)packages/shared/src/error.ts(1 hunks)packages/types/src/signIn.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
.changeset/**
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/young-cats-retire.md
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
packages/clerk-js/src/core/events.ts (1)
eventBus(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
.changeset/young-cats-retire.md (1)
1-8: LGTM! Changeset follows proper format.The changeset correctly bumps minor versions for all affected packages and includes a clear experimental feature description. The changes align with the PR objectives for supporting optional identifier in
signIn.password().packages/shared/src/error.ts (1)
85-85: LGTM! Null-safe error checking implemented correctly.The addition of the null check (
err &&) prevents runtime errors whenerris null or undefined while preserving the original behavior for valid objects. This is a defensive programming improvement that enhances robustness.packages/types/src/signIn.ts (4)
129-129: LGTM! Fetch status tracking added appropriately.The
fetchStatusproperty provides necessary state tracking for the SignIn future resource, enabling UI components to show loading states during authentication flows.
130-130: LGTM! Available strategies property enhances API discoverability.The
availableStrategiesproperty allows consumers to programmatically determine which authentication methods are available, improving the developer experience for custom flows.
133-133: LGTM! Optional identifier parameter enables the intended workflow.Making the
identifierparameter optional supports the use case described in the PR objectives - callingsignIn.password()without an identifier whensignIn.create()was previously called with one.
149-149: LGTM! Finalize method completes the authentication flow.The
finalize()method provides a clear way to complete the sign-in process and activate the session, which is essential for the custom flows API.packages/clerk-js/src/core/resources/SignIn.ts (4)
506-506: LGTM! Fetch status properly initialized.The
fetchStatusis correctly initialized to 'idle' and matches the TypeScript interface. This provides the foundation for tracking async operation states.
514-516: LGTM! Available strategies getter implemented correctly.The getter properly returns the supported first factors or an empty array as fallback, providing safe access to available authentication strategies.
602-608: LGTM! Optional identifier implementation with proper fallback.The implementation correctly handles the optional identifier by falling back to the previously stored identifier from
resource.identifier. This enables the workflow wherecreate()is called first with an identifier, thenpassword()can be called without one.
699-713: LGTM! Finalize method properly completes the sign-in flow.The implementation correctly:
- Validates that a session was created
- Activates the session using the Clerk client
- Handles errors appropriately with event emission
- Returns consistent error format
This provides a clean way to complete the authentication process.
📝 WalkthroughWalkthroughThis change updates SignInFuture in clerk-js to add a fetchStatus observable, expose availableStrategies from supportedFirstFactors, allow password() to omit the identifier by defaulting to the resource’s identifier, and introduce finalize() to activate the created session via setActive with error handling and events. The types package mirrors these API changes in SignInFutureResource. The shared package adjusts isClerkAPIResponseError to guard against falsy inputs. A changeset bumps minor versions for @clerk/clerk-js, @clerk/shared, and @clerk/types and notes experimental support for calling signIn.password() without an identifier. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
.changeset/young-cats-retire.md (1)
7-8: Remove stray character at EOF to avoid noisy release notes.There's a dangling "8" at the end of the file that will leak into changelogs.
Apply this diff:
[Experimental] Signals: Add support for calling `signIn.password()` without an identifier. -8 +
🧹 Nitpick comments (2)
.changeset/young-cats-retire.md (1)
1-7: Expand the Changeset to include all public API changes.Finalize and availableStrategies are also user-facing (even if experimental). Consider mentioning:
- SignInFuture.finalize() to complete sign-in by activating the session
- SignInFuture.availableStrategies exposing supported first factors
I can draft the revised text if helpful.
packages/clerk-js/src/core/resources/SignIn.ts (1)
699-713: Set fetchStatus during finalize and consider updating local state post-activation.Set 'fetching'/'idle' to make the observable useful. Optionally refresh or set status to 'complete' after activation.
Apply this diff:
async finalize(): Promise<{ error: unknown }> { - eventBus.emit('resource:error', { resource: this.resource, error: null }); - try { + eventBus.emit('resource:error', { resource: this.resource, error: null }); + this.fetchStatus = 'fetching'; + try { if (!this.resource.createdSessionId) { throw new Error('Cannot finalize sign in without a created session.'); } await SignIn.clerk.setActive({ session: this.resource.createdSessionId }); } catch (err: unknown) { eventBus.emit('resource:error', { resource: this.resource, error: err }); return { error: err }; - } + } finally { + this.fetchStatus = 'idle'; + } return { error: null }; }Please add tests for finalize() happy-path and missing createdSessionId error case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/young-cats-retire.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(3 hunks)packages/shared/src/error.ts(1 hunks)packages/types/src/signIn.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/error.tspackages/types/src/signIn.tspackages/clerk-js/src/core/resources/SignIn.ts
.changeset/**
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/young-cats-retire.md
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/resources/SignIn.ts (2)
packages/react/src/components/uiComponents.tsx (1)
SignIn(130-156)packages/clerk-js/src/core/events.ts (1)
eventBus(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (generic, chrome)
🔇 Additional comments (2)
packages/types/src/signIn.ts (1)
128-150: Tests are missing for type-level API changes.Please add tests to assert:
- password accepts optional identifier (type-level compile checks)
- finalize is present and correctly typed
- create accepts the extended parameter shape
I can open a follow-up PR scaffold for these tests in packages/types if you want.
packages/clerk-js/src/core/resources/SignIn.ts (1)
514-517: LGTM: availableStrategies mirrors supportedFirstFactors safely.Returning an empty array when null keeps consumers from having to null-check.
| fetchStatus: 'idle' | 'fetching' = 'idle'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
fetchStatus is never updated; wire it to network calls.
The property is added but never transitions to 'fetching'/'idle', limiting its usefulness as an observable.
As a pattern, set to 'fetching' before a request and back to 'idle' in finally blocks. See the password/finalize suggestions below for concrete diffs; apply similarly across other methods in SignInFuture.
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/resources/SignIn.ts around lines 506-507, the
fetchStatus property is initialized but never updated; update SignInFuture
methods that perform network requests to set this.fetchStatus = 'fetching'
immediately before the async call and reset this.fetchStatus = 'idle' in a
finally block after the call (and also in error branches where applicable).
Apply this pattern to all SignInFuture methods that call the network (e.g.,
start, attempt, finalize, password/finalize variants), ensuring the status is
set to 'fetching' before awaiting the request and always cleared to 'idle' in
finally so observers reliably see transitions.
| async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> { | ||
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | ||
| const previousIdentifier = this.resource.identifier; | ||
| try { | ||
| await this.resource.__internal_basePost({ | ||
| path: this.resource.pathRoot, | ||
| body: { identifier, password }, | ||
| body: { identifier: identifier || previousIdentifier, password }, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: password() misses strategy and wrong endpoint; add identifier guard and use attempt_first_factor when applicable.
Posting to /client/sign_ins without action and without strategy: 'password' will likely be treated as a create-with-identifier request and ignore the password. Also, when neither an explicit nor prior identifier exists, fail fast with a clear error. Finally, use fetchStatus to reflect request lifecycle.
Apply this diff:
- async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> {
- eventBus.emit('resource:error', { resource: this.resource, error: null });
- const previousIdentifier = this.resource.identifier;
- try {
- await this.resource.__internal_basePost({
- path: this.resource.pathRoot,
- body: { identifier: identifier || previousIdentifier, password },
- });
- } catch (err: unknown) {
- eventBus.emit('resource:error', { resource: this.resource, error: err });
- return { error: err };
- }
-
- return { error: null };
- }
+ async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> {
+ eventBus.emit('resource:error', { resource: this.resource, error: null });
+ this.fetchStatus = 'fetching';
+ const previousIdentifier = this.resource.identifier;
+ const idToUse = identifier ?? previousIdentifier ?? undefined;
+ try {
+ if (!this.resource.id) {
+ if (!idToUse) {
+ throw new Error('Cannot attempt password without an identifier. Call create() first or provide one.');
+ }
+ // Create + password in one call
+ await this.resource.__internal_basePost({
+ path: this.resource.pathRoot,
+ body: { strategy: 'password', identifier: idToUse, password },
+ });
+ } else {
+ // Attempt first factor on existing sign-in
+ await this.resource.__internal_basePost({
+ body: { strategy: 'password', password },
+ action: 'attempt_first_factor',
+ });
+ }
+ } catch (err: unknown) {
+ eventBus.emit('resource:error', { resource: this.resource, error: err });
+ return { error: err };
+ } finally {
+ this.fetchStatus = 'idle';
+ }
+
+ return { error: null };
+ }Additionally, consider a unit/integration test to cover:
- password() with explicit identifier (no prior create)
- password() without identifier after a prior create()
- password() without identifier and without prior create() should return
{ error }
🏁 Script executed:
#!/bin/bash
set -e
# Show the current password() implementation with surrounding context
rg -n 'async password' -C10 packages/clerk-js/src/core/resources/SignIn.tsLength of output: 952
🏁 Script executed:
#!/bin/bash
set -e
# Search for other uses of `strategy` in SignIn.ts
echo "=== strategy occurrences ==="
rg -n 'strategy' -C3 packages/clerk-js/src/core/resources/SignIn.ts || true
# Search for `action` parameter usages in SignIn.ts
echo "=== action occurrences ==="
rg -n 'action:' -C3 packages/clerk-js/src/core/resources/SignIn.ts || trueLength of output: 9787
🏁 Script executed:
#!/bin/bash
set -e
# Find definition of __internal_basePost across the repo
rg -n 'function __internal_basePost' -C3
rg -n '__internal_basePost' -C3Length of output: 51
🏁 Script executed:
#!/bin/bash
set -e
# Search for fetchStatus in SignIn.ts
rg -n 'fetchStatus' -C3 packages/clerk-js/src/core/resources/SignIn.ts || true
# Search for fetchStatus in the entire repo
rg -n 'fetchStatus' -C3 .Length of output: 23302
🏁 Script executed:
#!/bin/bash
set -e
# Show first 50 lines of SignIn.ts to inspect class context
sed -n '1,50p' packages/clerk-js/src/core/resources/SignIn.tsLength of output: 1430
Implement proper strategy, action, identifier guard, and fetchStatus in password()
- Set
fetchStatusto"fetching"at the start and reset to"idle"in afinallyblock. - Throw a clear error if neither an explicit
identifiernor an existing session ID is available. - Include
strategy: 'password'in the request body. - Use
action: 'attempt_first_factor'when a session ID already exists. - (Optional) specify
path: this.resource.pathRootfor the initial create. - Add tests for:
• Explicit identifier without priorcreate()
• Implicit identifier after a priorcreate()
• Missing identifier before anycreate()
Diff:
--- a/packages/clerk-js/src/core/resources/SignIn.ts
+++ b/packages/clerk-js/src/core/resources/SignIn.ts
@@ -602,14 +602,38 @@
async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> {
eventBus.emit('resource:error', { resource: this.resource, error: null });
+ this.fetchStatus = 'fetching';
const previousIdentifier = this.resource.identifier;
- try {
- await this.resource.__internal_basePost({
- path: this.resource.pathRoot,
- body: { identifier: identifier || previousIdentifier, password },
- });
- } catch (err: unknown) {
- eventBus.emit('resource:error', { resource: this.resource, error: err });
- return { error: err };
- }
-
- return { error: null };
+ const idToUse = identifier ?? previousIdentifier;
+ try {
+ if (!this.resource.id) {
+ if (!idToUse) {
+ throw new Error('Cannot attempt password without an identifier. Call create() first or provide one.');
+ }
+ // Create + password in one call
+ await this.resource.__internal_basePost({
+ path: this.resource.pathRoot,
+ body: { strategy: 'password', identifier: idToUse, password },
+ });
+ } else {
+ // Attempt first factor on existing sign-in
+ await this.resource.__internal_basePost({
+ body: { strategy: 'password', password },
+ action: 'attempt_first_factor',
+ });
+ }
+ } catch (err: unknown) {
+ eventBus.emit('resource:error', { resource: this.resource, error: err });
+ return { error: err };
+ } finally {
+ this.fetchStatus = 'idle';
+ }
+
+ return { error: null };
+ }Add corresponding unit/integration tests to cover all scenarios.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> { | |
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | |
| const previousIdentifier = this.resource.identifier; | |
| try { | |
| await this.resource.__internal_basePost({ | |
| path: this.resource.pathRoot, | |
| body: { identifier, password }, | |
| body: { identifier: identifier || previousIdentifier, password }, | |
| }); | |
| async password({ identifier, password }: { identifier?: string; password: string }): Promise<{ error: unknown }> { | |
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | |
| this.fetchStatus = 'fetching'; | |
| const previousIdentifier = this.resource.identifier; | |
| const idToUse = identifier ?? previousIdentifier; | |
| try { | |
| if (!this.resource.id) { | |
| if (!idToUse) { | |
| throw new Error( | |
| 'Cannot attempt password without an identifier. Call create() first or provide one.' | |
| ); | |
| } | |
| // Create + password in one call | |
| await this.resource.__internal_basePost({ | |
| path: this.resource.pathRoot, | |
| body: { strategy: 'password', identifier: idToUse, password }, | |
| }); | |
| } else { | |
| // Attempt first factor on existing sign-in | |
| await this.resource.__internal_basePost({ | |
| body: { strategy: 'password', password }, | |
| action: 'attempt_first_factor', | |
| }); | |
| } | |
| } catch (err: unknown) { | |
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | |
| return { error: err }; | |
| } finally { | |
| this.fetchStatus = 'idle'; | |
| } | |
| return { error: null }; | |
| } |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/resources/SignIn.ts around lines 602 to 609, set
this.resource.fetchStatus = "fetching" at the start of password() and ensure you
reset it to "idle" in a finally block; validate that either the explicit
identifier param or this.resource.identifier (session id/previous identifier)
exists and throw a clear error if neither is available; include strategy:
"password" in the request body and, when a previous identifier/session id
exists, add action: "attempt_first_factor" to the body (otherwise omit action
for initial create), and keep path: this.resource.pathRoot for the post; update
tests to cover explicit identifier without prior create(), implicit identifier
after prior create(), and missing identifier before any create().
| export function isClerkAPIResponseError(err: any): err is ClerkAPIResponseError { | ||
| return 'clerkError' in err; | ||
| return err && 'clerkError' in err; | ||
| } |
There was a problem hiding this comment.
Guard all type-narrowing helpers against falsy inputs to prevent runtime TypeErrors.
Good fix on isClerkAPIResponseError. However, isKnownError can still throw via isMetamaskError / isClerkRuntimeError when passed null/undefined, because the in operator on a non-object throws. Harden these as well.
Apply this diff:
export function isKnownError(error: any): error is ClerkAPIResponseError | ClerkRuntimeError | MetamaskError {
- return isClerkAPIResponseError(error) || isMetamaskError(error) || isClerkRuntimeError(error);
+ // Guard against falsy input to avoid TypeError from `in` operator in downstream checks
+ if (!error) {
+ return false;
+ }
+ return isClerkAPIResponseError(error) || isMetamaskError(error) || isClerkRuntimeError(error);
}
export function isClerkAPIResponseError(err: any): err is ClerkAPIResponseError {
- return err && 'clerkError' in err;
+ return !!err && typeof err === 'object' && 'clerkError' in err;
}
export function isClerkRuntimeError(err: any): err is ClerkRuntimeError {
- return 'clerkRuntimeError' in err;
+ return !!err && typeof err === 'object' && 'clerkRuntimeError' in err;
}
export function isMetamaskError(err: any): err is MetamaskError {
- return 'code' in err && [4001, 32602, 32603].includes(err.code) && 'message' in err;
+ return (
+ !!err &&
+ typeof err === 'object' &&
+ 'code' in err &&
+ [4001, 32602, 32603].includes((err as any).code) &&
+ 'message' in err
+ );
}Also applies to: 105-106, 123-124, 75-77
🤖 Prompt for AI Agents
In packages/shared/src/error.ts around lines 75-77, 84-86, 105-106 and 123-124,
the type-narrowing helpers (isMetamaskError, isClerkAPIResponseError,
isClerkRuntimeError and isKnownError) use the `in` operator without first
checking for falsy/non-object inputs which can throw; update each helper to
first confirm the value is truthy and typeof value === 'object' (or use value !=
null) before using `in` or other object checks, and adjust isKnownError to call
the hardened helpers safely so it never calls `in` on null/undefined.
| fetchStatus: 'idle' | 'fetching'; | ||
| availableStrategies: SignInFirstFactor[]; | ||
| status: SignInStatus | null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc for new public APIs (fetchStatus, availableStrategies, password identifier fallback, finalize).
Ensure discoverability and clarity for consumers.
Apply this diff to document the additions:
export interface SignInFutureResource {
- fetchStatus: 'idle' | 'fetching';
- availableStrategies: SignInFirstFactor[];
+ /**
+ * Reflects ongoing network activity for the future resource.
+ * 'fetching' while an API call is in flight, otherwise 'idle'.
+ * @experimental
+ */
+ fetchStatus: 'idle' | 'fetching';
+ /**
+ * Convenience mirror of SignIn.supportedFirstFactors. Always an array.
+ * @experimental
+ */
+ availableStrategies: SignInFirstFactor[];
status: SignInStatus | null;
- create: (params: { identifier: string }) => Promise<{ error: unknown }>;
- password: (params: { identifier?: string; password: string }) => Promise<{ error: unknown }>;
+ /**
+ * Attempts password auth. When identifier is omitted, falls back to the identifier
+ * set during a previous create() call.
+ * @experimental
+ */
+ password: (params: { identifier?: string; password: string }) => Promise<{ error: unknown }>;
...
- finalize: () => Promise<{ error: unknown }>;
+ /**
+ * Activates the created session (SignIn.createdSessionId) via Clerk.setActive.
+ * Returns `{ error: null }` on success or `{ error }` on failure.
+ * @experimental
+ */
+ finalize: () => Promise<{ error: unknown }>;
}Also applies to: 149-150, 133-134
🤖 Prompt for AI Agents
In packages/types/src/signIn.ts around lines 129-131 (also apply to lines
133-134 and 149-150), the new public API surface (fetchStatus,
availableStrategies, the password identifier fallback, and finalize) lacks JSDoc
comments; add clear JSDoc blocks above each exported property/function
describing its purpose, type/possible values (e.g., fetchStatus:
'idle'|'fetching'; availableStrategies: SignInFirstFactor[]), when it is
set/used, any fallback behavior for the password identifier, expected return
values/errors for finalize, and example usage/notes on stability so consumers
can discover and understand these members. Ensure each JSDoc includes a one-line
summary, @returns or @type where appropriate, and any relevant @default or
@remarks details.
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | ||
| try { | ||
| if (!this.resource.createdSessionId) { | ||
| throw new Error('Cannot finalize sign in without a created session.'); |
There was a problem hiding this comment.
| throw new Error('Cannot finalize sign in without a created session.'); | |
| throw new Error('Cannot finalize sign-in without a created session.'); |
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | ||
| try { | ||
| if (!this.resource.createdSessionId) { | ||
| throw new Error('Cannot finalize sign in without a created session.'); |
There was a problem hiding this comment.
Consider using ClerkRuntimeError here 🤔
There was a problem hiding this comment.
I'll do a full error pass in a future PR
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/types/src/signIn.ts (2)
129-130: Add JSDoc for newly added public properties (fetchStatus, availableStrategies).These are public APIs in the types package; they should be documented for discoverability and consistency with the coding guidelines. Also, please confirm that availableStrategies is guaranteed to be a non-null array at runtime to avoid type/runtime mismatches.
Apply this diff to document the properties:
export interface SignInFutureResource { + /** + * Reflects ongoing network activity for the future resource. + * 'fetching' while an API call is in flight, otherwise 'idle'. + * @experimental + */ fetchStatus: 'idle' | 'fetching'; + /** + * Convenience mirror of SignIn.supportedFirstFactors. Always an array. + * @experimental + */ availableStrategies: SignInFirstFactor[];Run this to verify the non-null runtime contract for availableStrategies:
#!/bin/bash # Inspect usages and initialization of `availableStrategies` and `fetchStatus`. rg -n -A3 -B3 'availableStrategies|fetchStatus'
138-138: Add JSDoc to password() to clarify identifier fallback behavior.This new behavior is a key developer affordance and should be documented.
Apply this diff:
- password: (params: { identifier?: string; password: string }) => Promise<{ error: unknown }>; + /** + * Attempts password authentication. + * If `identifier` is omitted, falls back to the identifier provided during a previous create() call. + * @experimental + */ + password: (params: { identifier?: string; password: string }) => Promise<{ error: unknown }>;
🧹 Nitpick comments (3)
packages/types/src/signIn.ts (3)
154-154: Add JSDoc for finalize().Document what finalize does (activates createdSessionId via Clerk.setActive) and the expected result shape so consumers know how to handle success/failure.
Apply this diff:
- finalize: () => Promise<{ error: unknown }>; + /** + * Activates the created session (`SignIn.createdSessionId`) via Clerk.setActive. + * Returns `{ error: null }` on success, or `{ error }` on failure. + * @experimental + */ + finalize: () => Promise<{ error: unknown }>;
129-139: Optional: Introduce a shared Result type for Future APIs to improve error typing.Right now, all Future methods return
{ error: unknown }. To align with “Use proper TypeScript error types,” consider adopting a common result type (e.g.,{ error: null } | { error: ClerkAPIResponseError }) and reusing it acrosscreate,password,emailCode,resetPasswordEmailCode,sso, andfinalize. This improves DX and narrows error handling.If you decide to proceed, you could introduce a reusable type (outside this interface):
// in this file (top-level, exported if needed) export type ActionResult<E = unknown> = { error: null } | { error: E };Then update Future methods’ return types to
Promise<ActionResult<YourErrorType>>. If a canonical error type exists (e.g., ClerkAPIResponseError in @clerk/shared), prefer that; otherwise annotate with a more specific error shape that matches runtime.
138-138: Tests: Cover password-without-identifier and finalize flows.No tests are visible in this PR. Please add runtime tests validating:
- password() succeeds without identifier when create() was previously called with one
- finalize() activates the session and returns
{ error: null }on success, and returns{ error }with a typed error on failureI can help scaffold these tests if you point me to the preferred test framework and target package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/core/resources/SignIn.ts(3 hunks)packages/types/src/signIn.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/resources/SignIn.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/types/src/signIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/types/src/signIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/types/src/signIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/types/src/signIn.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/types/src/signIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/types/src/signIn.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/types/src/signIn.ts
🧬 Code Graph Analysis (1)
packages/types/src/signIn.ts (1)
packages/types/src/strategies.ts (1)
OAuthStrategy(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: semgrep-cloud-platform/scan
| create: (params: { | ||
| identifier?: string; | ||
| strategy?: OAuthStrategy | 'saml' | 'enterprise_sso'; | ||
| redirectUrl?: string; | ||
| actionCompleteRedirectUrl?: string; | ||
| }) => Promise<{ error: unknown }>; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the expanded create() params and verify redirect param naming consistency.
The widened signature looks correct; please add JSDoc describing each param and its behavior. Additionally, the naming differs from sso() which uses redirectUrlComplete while create() uses actionCompleteRedirectUrl. If this is intentional, document the distinction; otherwise consider aligning or supporting both (runtime-dependent).
Apply this diff to document the params:
status: SignInStatus | null;
- create: (params: {
+ /**
+ * Starts a sign-in with optional identifier and/or federated SSO strategy.
+ * When `strategy` is provided (OAuth/SAML/enterprise SSO), `redirectUrl` and
+ * optionally `actionCompleteRedirectUrl` control the return locations.
+ * `identifier` is optional here to support flows that defer it to a later step.
+ * @experimental
+ */
+ create: (params: {
identifier?: string;
strategy?: OAuthStrategy | 'saml' | 'enterprise_sso';
redirectUrl?: string;
actionCompleteRedirectUrl?: string;
}) => Promise<{ error: unknown }>;Run this to check for naming consistency across the repo:
🏁 Script executed:
#!/bin/bash
# Find occurrences of these redirect params to confirm intent and consistency.
rg -n -S 'redirectUrlComplete|actionCompleteRedirectUrl' -A2 -B2Length of output: 50926
Document create() params and unify redirect param naming
Please add JSDoc to the new create() signature in packages/types/src/signIn.ts and resolve the discrepancy between its actionCompleteRedirectUrl and the rest of the APIs’ redirectUrlComplete. Either rename actionCompleteRedirectUrl to redirectUrlComplete (or alias it), or clearly document why this method diverges.
• File: packages/types/src/signIn.ts (lines 132–137)
• Compare with sso() at lines 150–152 (uses redirectUrlComplete)
Proposed diff for JSDoc and aliasing:
status: SignInStatus | null;
+ /**
+ * Starts a sign-in flow, optionally with an identifier or federated SSO strategy.
+ * @param params.identifier Optional user identifier for credentialed sign-in.
+ * @param params.strategy OAuth/SAML/enterprise SSO strategy.
+ * @param params.redirectUrl URL to return to after initiating the flow.
+ * @param params.redirectUrlComplete
+ * Full URL or path to navigate to once sign-in completes. Alias for
+ * `actionCompleteRedirectUrl` to align with other APIs.
+ * @experimental
+ */
create: (params: {
identifier?: string;
strategy?: OAuthStrategy | 'saml' | 'enterprise_sso';
redirectUrl?: string;
- actionCompleteRedirectUrl?: string;
+ actionCompleteRedirectUrl?: string;
+ redirectUrlComplete?: string;
}) => Promise<{ error: unknown }>;Additionally, review and update the same pattern in:
packages/types/src/signUp.tspackages/types/src/factors.ts- Any other user-facing methods that accept completion URLs
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| create: (params: { | |
| identifier?: string; | |
| strategy?: OAuthStrategy | 'saml' | 'enterprise_sso'; | |
| redirectUrl?: string; | |
| actionCompleteRedirectUrl?: string; | |
| }) => Promise<{ error: unknown }>; | |
| status: SignInStatus | null; | |
| /** | |
| * Starts a sign-in flow, optionally with an identifier or federated SSO strategy. | |
| * @param params.identifier Optional user identifier for credentialed sign-in. | |
| * @param params.strategy OAuth/SAML/enterprise SSO strategy. | |
| * @param params.redirectUrl URL to return to after initiating the flow. | |
| * @param params.redirectUrlComplete Full URL or path to navigate to once sign-in completes. | |
| * Alias for `actionCompleteRedirectUrl` to align with other APIs. | |
| * @experimental | |
| */ | |
| create: (params: { | |
| identifier?: string; | |
| strategy?: OAuthStrategy | 'saml' | 'enterprise_sso'; | |
| redirectUrl?: string; | |
| actionCompleteRedirectUrl?: string; | |
| redirectUrlComplete?: string; | |
| }) => Promise<{ error: unknown }>; |
🤖 Prompt for AI Agents
In packages/types/src/signIn.ts around lines 132 to 137, the create() signature
lacks JSDoc and uses actionCompleteRedirectUrl which is inconsistent with other
APIs that use redirectUrlComplete (see sso() at lines 150–152); add a concise
JSDoc block describing each param and the returned Promise, and rename the param
to redirectUrlComplete (or add redirectUrlComplete as an alias that maps to the
existing actionCompleteRedirectUrl) so naming is consistent; apply the same
JSDoc + rename/alias pattern to packages/types/src/signUp.ts,
packages/types/src/factors.ts, and any other user-facing methods that accept
completion/redirect URLs to keep the public API consistent.
Description
This PR updates our new custom flows APIs to allow for calling
signIn.password()without an identifier ifsignIn.create()was previously called with one. It also addssignIn.finalize(), andsignIn.availableStrategies. This PR pertains to our experimental Signals implementation, which is not yet intended for public use.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Chores