fix(clerk-js): Require email or phone when password field is visible #6259
Conversation
🦋 Changeset detectedLatest commit: b8c813d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 ↗︎
|
bratsos
changed the title
📝 Walkthrough""" WalkthroughA patch was applied to the '@clerk/clerk-js' package to address a bug in the sign-up process involving password authentication when used with email or phone number fields. The validation logic for the required status of these fields was updated to more accurately reflect when email or phone inputs should be marked as required or optional, especially in progressive sign-up scenarios. Associated tests were updated and expanded to verify these conditions, including renaming and enhancing existing tests and adding new scenarios to ensure correct behavior when password, email, and phone requirements interact. A new function, Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nikos Douvlis <[email protected]>
bratsos
force-pushed
the
alexbratsos/user-704-wrong-optional-field-in-ui-context
branch
from
99d0a35 to
ae9b8cd
Compare
@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: |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpStart.spec.tsx(2 hunks)packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts(2 hunks)packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/ui/components/SignUp/tests/SignUpStart.spec.tsx
🧰 Additional context used
📓 Path-based instructions (10)
`**/*.{js,jsx,ts,tsx}`: All code must pass ESLint checks with the project's conf...
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Use Prettier for consistent code formatting
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
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Validate all inputs and sanitize outputs
Implement proper logging with different levels
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`packages/**/*.ts`: TypeScript is required for all packages
packages/**/*.ts: TypeScript is required for all packages
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`packages/**/*.{ts,tsx,d.ts}`: Packages should export TypeScript types alongside runtime code
packages/**/*.{ts,tsx,d.ts}: Packages should export TypeScript types alongside runtime code
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/*.{ts,tsx}`: Use proper TypeScript error types
**/*.{ts,tsx}: Use proper TypeScript error types
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`packages/{clerk-js,elements,themes}/**/*`: Visual regression testing should be performed for UI components.
packages/{clerk-js,elements,themes}/**/*: Visual regression testing should be performed for UI components.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/*.{ts,tsx}`: Always define explicit return types for functions, especially p...
**/*.{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
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
Document public functions and APIs with JSDoc-style comments including @param, @returns, @throws, and @example
Define custom error classes for domain-specific errors
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining and nullish coalescing for safe property access
Let TypeScript infer types when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use readonly arrays and objects for immutability
Use immutable update patterns (spread, etc.) for objects and arrays
Use lazy loading for large types
Preferunknownoveranyfor performance
Use type-only imports:import type { ... }
Use branded types for domain safety
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints in TypeScript generics
No unused type parameters in generics
Proper use of utility types instead of manual type construction
Type-only imports where possible for performance
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/*.ts`: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
**/*.ts: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.tspackages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/{__tests__,**/__tests__}/**/*.{js,jsx,ts,tsx}`: Test files should be co-located with source files or in `__tests__` directories
**/{__tests__,**/__tests__}/**/*.{js,jsx,ts,tsx}: Test files should be co-located with source files or in__tests__directories
📄 Source: CodeRabbit Inference Engine (.cursor/rules/development.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/*.{test,spec}.{js,ts,tsx}`: Unit tests should use Jest or Vitest as the test runner.
**/*.{test,spec}.{js,ts,tsx}: Unit tests should use Jest or Vitest as the test runner.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
`**/__tests__/**/*.{ts,tsx}`: Use Vitest for type-safe testing in TypeScript Cre...
**/__tests__/**/*.{ts,tsx}: Use Vitest for type-safe testing in TypeScript
Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces in tests
📄 Source: CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
List of files the instruction was applied to:
packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: dstaley
PR: clerk/javascript#6116
File: .changeset/tangy-garlics-say.md:1-2
Timestamp: 2025-06-13T16:09:53.061Z
Learning: In the Clerk JavaScript repository, contributors create intentionally empty changeset files (containing only the YAML delimiters) when a PR touches only non-published parts of the codebase (e.g., sandbox assets). This signals that no package release is required, so such changesets should not be flagged as missing content.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/{clerk-js,elements,themes}/**/* : Visual regression testing should be performed for UI components.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/@clerk/*/package.json : Framework packages must depend on '@clerk/clerk-js' for core functionality.
Learnt from: wobsoriano
PR: clerk/javascript#6163
File: packages/backend/src/api/endpoints/APIKeysApi.ts:60-70
Timestamp: 2025-06-20T17:44:17.570Z
Learning: The Clerk codebase uses POST method for API key update operations instead of the typical PATCH method, as clarified by the maintainer wobsoriano.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/clerk-react/**/*.{test,spec}.{js,ts,tsx} : Component testing should use React Testing Library.
Learnt from: dstaley
PR: clerk/javascript#6100
File: packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx:121-124
Timestamp: 2025-06-16T17:08:58.414Z
Learning: The @clerk/clerk-js package only supports browsers released in the last two years (since May 8, 2023), so modern CSS features like color-mix() are fully supported across all target browsers without requiring fallbacks.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/@clerk/*/jest.config.{js,ts} : Each framework integration package must have its own test configuration.
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.ts (1)
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-06-30T10:32:37.848Z
Learning: Applies to **/*.{jsx,tsx} : Use proper form validation
packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts (11)
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/{clerk-js,elements,themes}/**/* : Visual regression testing should be performed for UI components.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/clerk-react/**/*.{test,spec}.{js,ts,tsx} : Component testing should use React Testing Library.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-06-30T10:32:37.848Z
Learning: Applies to **/*.test.{jsx,tsx} : Test component interactions
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/monorepo.mdc:0-0
Timestamp: 2025-06-30T10:30:56.197Z
Learning: Applies to packages/@clerk/*/jest.config.{js,ts} : Each framework integration package must have its own test configuration.
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/development.mdc:0-0
Timestamp: 2025-06-30T10:29:42.997Z
Learning: Include tests for all new features
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-06-30T10:33:45.961Z
Learning: Applies to **/__tests__/**/*.{ts,tsx} : Create type-safe test builders/factories
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-06-30T10:33:45.961Z
Learning: Applies to **/__tests__/**/*.{ts,tsx} : Use branded types for test isolation
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/development.mdc:0-0
Timestamp: 2025-06-30T10:29:42.997Z
Learning: Unit tests are required for all new functionality
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-06-30T10:32:37.848Z
Learning: Applies to **/*.test.{jsx,tsx} : Implement proper test setup
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-06-30T10:32:37.848Z
Learning: Applies to **/*.test.{jsx,tsx} : Implement proper test isolation
Learnt from: CR
PR: clerk/javascript#0
File: .cursor/rules/development.mdc:0-0
Timestamp: 2025-06-30T10:29:42.997Z
Learning: Test authentication flows end-to-end
🧬 Code Graph Analysis (1)
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.ts (1)
packages/types/src/userSettings.ts (1)
Attributes(103-105)
⏰ 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: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
packages/clerk-js/src/ui/components/SignUp/signUpFormHelpers.ts (7)
195-199: LGTM! Clean integration of the new validation logic.The refactoring to use
determineRequiredCommunicationMethodproperly encapsulates the complex requirement logic and makes the field determination cleaner.Also applies to: 202-202
243-247: LGTM! Consistent implementation with email field.The phone field correctly follows the same pattern as the email field, ensuring consistent behavior across both communication methods.
Also applies to: 250-250
316-322: Well-structured context type definition.The
Contexttype clearly defines all the data needed for the decision-making process, improving type safety and readability.
328-335: Excellent function signature and documentation.The function signature is clear with proper TypeScript types and the JSDoc comment explains the purpose effectively.
360-396: Excellent rule-based decision system.The priority-ordered rules provide a clear and maintainable way to handle the complex logic for determining communication method requirements. The rules cover all necessary scenarios from server overrides to fallback behaviors.
398-401: Good fallback handling.The fallback rule ensures the function always returns a deterministic result, defaulting to email when no specific rules match.
405-432: Comprehensive switch statement with proper error handling.The switch statement covers all possible winner cases and includes an appropriate error for unreachable code paths, demonstrating good defensive programming.
packages/clerk-js/src/ui/components/SignUp/__tests__/signUpFormHelpers.test.ts (13)
3-7: Good import structure.The import statement correctly includes the new function alongside existing ones, maintaining clean organization.
544-654: Excellent integration tests for password scenarios.These tests ensure that the new logic works correctly within the existing
determineActiveFieldsfunction, covering important real-world scenarios where password requirements interact with communication method requirements.
656-758: Comprehensive password security tests.These tests specifically verify the core requirement that password-enabled flows must have at least one communication method available for password recovery. The scenarios cover both single and multiple communication method configurations.
760-815: Excellent active field behavior tests.These tests verify the crucial behavior where the active communication method becomes required in email OR phone layouts when password is enabled, ensuring users can't proceed without providing a recovery method.
819-825: Well-structured test setup.The
createMinimalAttributeshelper function provides a clean foundation for test data creation, following the test factory pattern recommended in the coding guidelines.
827-857: Thorough Rule 1 coverage.Tests correctly verify that when password is not required, the function mirrors the server configuration, covering both disabled and enabled-but-not-required scenarios.
859-904: Complete Rule 2 verification.Tests systematically cover all cases where the server already requires communication methods, ensuring the function respects server configuration in all scenarios.
906-921: Proper edge case handling.Tests verify the function correctly handles the scenario where no communication methods are available, returning appropriate "none required" results.
923-968: Comprehensive email OR phone layout tests.These tests cover the critical progressive sign-up scenario where users can toggle between email and phone, ensuring the active field becomes required for password recovery.
970-1002: Thorough single-method coverage.Tests verify both email-only and phone-only configurations work correctly when password is required, ensuring the only available communication method becomes required.
1004-1051: Complete primary method precedence tests.Tests verify the function correctly prioritizes primary communication methods, with email taking precedence when both are primary, following expected business logic.
1070-1130: Excellent rule priority and edge case coverage.These tests verify that higher-priority rules correctly override lower-priority ones and that the function handles null/undefined active identifiers gracefully.
1132-1211: Outstanding real-world scenario coverage.These tests cover practical configurations that users would encounter, including typical email/phone primary setups and progressive sign-up toggles, ensuring the function works correctly in production scenarios.
Description
When password authentication is enabled alongside email/phone identifiers in progressive sign-up, the field requirement logic was not correctly determining which identifier should be marked as optional. This led to incorrect validation behavior where required fields might be incorrectly treated as optional.
Solution
For Email Address Fields:
For Phone Number Fields:
clerk-js-fix-email-required-before.mp4
clerk-js-fix-email-required-after.mp4
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests