feat(clerk-js): Update OTP to use a single transparent field #6551
Conversation
🦋 Changeset detectedLatest commit: 800b72e 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 |
|
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/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: |
WalkthroughReworks the OTP input to a single transparent input driven by input-otp with visual slot segments; adds DivInput and OTPInputSegment, updates CodeControl to slot-based rendering, adjusts focus selectors, adds document.elementFromPoint mocks in test setups, updates tests to use label/test-id queries, and updates Playwright helpers/timeouts and changesets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input as input-otp (transparent)
participant Control as OTPCodeControl
participant Slot as OTPInputSegment
participant Parent as Consumer (onChange/onFinish)
Note over User,Input: User types or autofill triggers
User->>Input: enter digits / autofill
Input->>Control: slotsUpdated(values[])
Control->>Slot: render slot(n) with digit
Control->>Parent: onChange(values.join(''))
alt completed
Control->>Parent: onCodeEntryFinished(code)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related issues
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. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tmilewski
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx (2)
37-54: Be explicit with property descriptors when stubbing window.location/historyThe restore sets configurable: true, which is fine. To avoid surprises if tests override these properties, ensure those overrides also set configurable: true. In this file, the ticket flow tests (Line 532 and Line 570) use Object.defineProperty without configurable. Consider updating them for symmetry and robustness.
Example for those overrides:
Object.defineProperty(window, 'location', { value: { href: 'http://localhost/sign-in?__clerk_ticket=test_ticket' }, writable: true, configurable: true, }); Object.defineProperty(window, 'history', { value: { replaceState: jest.fn() }, writable: true, configurable: true, });Alternatively, you can avoid property descriptor friction by stubbing via spies where possible (e.g., jest.spyOn for methods) and only using defineProperty when necessary for non-function properties.
209-230: React type usage: import React types or switch to ReactNode/PropsWithChildrenThe wrapper annotates children as React.ReactNode but this file does not import the React type namespace. Depending on your tsconfig, this can cause “Cannot find name 'React'” during type-checking. Two safe options:
- Minimal change: add a type-only import at the top of the file:
import type React from 'react';
- Or use named types:
import type { ReactNode } from 'react';and update the signature to:
-const wrapperBefore = ({ children }: { children: React.ReactNode }) => ( +const wrapperBefore = ({ children }: { children: ReactNode }) => (Either keeps runtime unchanged and satisfies TS.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
integration/tests/next-build.test.ts(2 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration/tests/next-build.test.ts
🧰 Additional context used
📓 Path-based instructions (14)
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.{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/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.{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/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.{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/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.{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/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*.test.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.test.{jsx,tsx}: Use React Testing Library
Test component behavior, not implementation
Use proper test queries
Implement proper test isolation
Use proper test coverage
Test component interactions
Use proper test data
Implement proper test setup
Use proper test cleanup
Implement proper test assertions
Use proper test structure
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
**/*
⚙️ 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/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx
⏰ 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). (3)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx (1)
17-35: Resolved getComputedStyle leak; scoped mock and restoration look solidCapturing originals and restoring them per test prevents cross-test pollution and addresses the prior review concern. The mock shape also matches expected CSSStyleDeclaration usage.
| @@ -103,7 +103,10 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('component | |||
| await u.page.goToRelative(component.path, { waitUntil: 'commit' }); | |||
| await expect(u.page.getByText(component.fallback)).toBeVisible(); | |||
|
|
|||
| await signOut({ app, page, context }); | |||
| // eslint-disable-next-line playwright/no-conditional-in-test | |||
There was a problem hiding this comment.
No need to run if it's never signed in, per a similar if statement.
| maxFailures: process.env.CI ? 5 : undefined, | ||
| workers: process.env.CI ? '50%' : '70%', | ||
| use: { | ||
| actionTimeout: 10_000, | ||
| navigationTimeout: 30_000, |
There was a problem hiding this comment.
Relatively safe generic timeouts. More-specific ones are assigned (at the existing 90s) in various beforeAll calls and tests containing similar logic.
| await page.waitForTimeout(2000); | ||
| // TODO: In original test the input has autoSubmit and this step is not needed. Not used right now because it didn't work. | ||
| await page.getByRole('textbox', { name: 'Enter email verification code' }).click(); | ||
| await page.keyboard.type('424242', { delay: 100 }); |
There was a problem hiding this comment.
These particular actions don't apply to our generic OTP component and, as such, are handled differently... and aren't common thus shouldn't go in @clerk/testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
integration/tests/middleware-placement.test.ts (3)
47-54: Scope the 90s timeout via test.describe.configure instead of inside beforeAll.For consistency and predictability, configure the suite timeout at the describe level. This avoids ambiguity about whether the current beforeAll is covered and mirrors the global config shift away from a blanket 90s per-test timeout.
Within the selected range:
- test.setTimeout(90_000);Update the describe configuration (outside the selected range):
test.describe.configure({ mode: 'parallel', timeout: 90_000 });
78-85: Prefer describe-scoped timeout to cover beforeAll/afterAll and tests.Centralize the timeout with test.describe.configure({ timeout: 90_000 }) for this suite. It makes intent explicit and avoids relying on hook-local calls.
Within the selected range:
- test.setTimeout(90_000);Then adjust the describe’s configure (outside the selected range):
test.describe.configure({ mode: 'parallel', timeout: 90_000 });
22-29: Move timeout configuration to the suite level in integration/tests/middleware-placement.test.tsConfigure the timeout on each test.describe so it covers its beforeAll hook, then remove the in-hook test.setTimeout calls.
Apply these changes in each of the three describe blocks:
• In the “missing middleware” suite
- test.describe.configure({ mode: 'parallel' }); + test.describe.configure({ mode: 'parallel', timeout: 90_000 }); test.beforeAll(async () => { - test.setTimeout(90_000); app = await commonSetup.commit(); … });• In the “invalid middleware at root” suite
- test.describe.configure({ mode: 'parallel' }); + test.describe.configure({ mode: 'parallel', timeout: 90_000 }); test.beforeAll(async () => { - test.setTimeout(90_000); app = await commonSetup.addFile('middleware.ts', …).commit(); … });• In the “invalid middleware inside app” suite
- test.describe.configure({ mode: 'parallel' }); + test.describe.configure({ mode: 'parallel', timeout: 90_000 }); test.beforeAll(async () => { - test.setTimeout(90_000); app = await commonSetup.addFile('src/app/middleware.ts', …).commit(); … });Optional: this pattern (in-hook test.setTimeout) appears in many other integration tests—consider aligning them to use suite-level timeouts as well.
packages/clerk-js/bundlewatch.config.json (2)
1-35: Add explicit compression setting to bundlewatch configWe verified that packages/clerk-js/bundlewatch.config.json is the only Bundlewatch config in the repo, and it doesn’t declare a compression mode. To keep your size measurements stable and transparent, explicitly add a
"compression"field at the top of this file:• packages/clerk-js/bundlewatch.config.json
{ + "compression": "gzip", "files": [ { "path": "./dist/clerk.js", "maxSize": "628.65KB" }, … ] }(Use
"brotli"instead if you prefer.)
3-3: Tight clerk.js budget; consider rounding up to reduce CI flakesFile: packages/clerk-js/bundlewatch.config.json
Lines: 3-3The maxSize is currently set to 628.65KB:
{ "path": "./dist/clerk.js", "maxSize": "628.65KB" },That level of precision can lead to flaky CI failures on trivial, non-functional changes (e.g., minor tree-shaking differences). I recommend rounding up the threshold—either to a neat value like 630KB or by adding ~2% headroom over the actual gzipped size.
Because
dist/clerk.jsisn’t checked in, the verification script warns that it can’t find the file. Please:
Build the package so that
packages/clerk-js/dist/clerk.jsexists.Run this script to compute your current gzipped size and a +2% recommended threshold:
#!/usr/bin/env bash set -euo pipefail python - <<'PY' import json, glob, gzip, math cfg = json.load(open("packages/clerk-js/bundlewatch.config.json")) base = "packages/clerk-js" def gz_size(p): return len(gzip.compress(open(p,"rb").read(), compresslevel=6)) def fmt(b): return f"{math.ceil(b/1024*10)/10:.1f}KB" for e in cfg["files"]: if e["path"].endswith("clerk.js"): p = f"{base}/{e['path'][2:]}" b = gz_size(p) rec = math.ceil(b * 1.02) print("Current gzipped:", fmt(b), "→ recommended threshold:", fmt(rec)) PYUpdate
maxSizein the config to that rounded-up value.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
integration/tests/middleware-placement.test.ts(3 hunks)packages/clerk-js/bundlewatch.config.json(1 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:
integration/tests/middleware-placement.test.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:
integration/tests/middleware-placement.test.tspackages/clerk-js/bundlewatch.config.json
**/*.{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:
integration/tests/middleware-placement.test.ts
integration/**
📄 CodeRabbit Inference Engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/middleware-placement.test.ts
integration/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/middleware-placement.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/middleware-placement.test.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:
integration/tests/middleware-placement.test.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:
integration/tests/middleware-placement.test.tspackages/clerk-js/bundlewatch.config.json
⏰ 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). (26)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/clerk-js/bundlewatch.config.json (2)
9-9: Ensurepackages/clerk-js/dist/vendors*.jsis built and threshold is verified
- The
./dist/vendors*.jspattern didn’t match any files—please build the package so we can measure the actual gzipped size.- Once built, run the provided script (or equivalent) to find the largest vendor chunk and update
maxSize. Round to a clean value (e.g., 45 KB) and include ~3% headroom to reduce noise from minor dependency shifts.- Confirm whether the new
input-otpdependency is bundled intovendors*.jsor split into its own chunk; adjust your bundlewatch budgets accordingly.
13-33: Bundlewatch patterns require manual verification against built assets
The sandbox build had nodistfiles, so the automated check flagged every glob as unmatched yet found no “uncovered” chunks. To ensure your patterns actually match real output on case-sensitive filesystems:
- Run a local build of
packages/clerk-js(e.g.,npm run buildor your standard build command).- Rerun the helper script from the original comment to:
- Report any patterns that match nothing (typos, casing or hyphen vs camelCase mismatches).
- List any emitted
.jschunks not covered by your globs.- Compare current gzipped sizes against configured
maxSizeand suggest +2% headroom at 0.1 KB granularity.- Adjust your glob patterns to reflect actual filenames (consider using all-lowercase with hyphens if your bundler emits kebab-case).
- Round
maxSizeentries to one decimal place (e.g.,8.9KBinstead of8.86KB) to reduce noise over time.
tmilewski
force-pushed
the
tm/user-2571-otp
branch
from
cc98744 to
0beb254
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integration/tests/localhost/localhost-different-port-same-instance.test.ts (2)
24-24: Replace magic number with a shared constant or env-driven value.Multiple files in this PR use 90_000; consider centralizing to avoid drift and allow CI/local tuning without code changes.
Apply this diff within the selected line(s):
- test.setTimeout(90_000); // Wait for apps to be ready + test.setTimeout(SETUP_TIMEOUT_MS); // Wait for apps to be readyAnd add this near the imports (outside the selected range):
// test setup timeout (overridable via env) const SETUP_TIMEOUT_MS = parseInt(process.env.CLERK_TEST_SETUP_TIMEOUT ?? '90000', 10);
29-32: Remove inner awaits inside Promise.all to keep the operations concurrent.Using await inside the array serializes the two calls and defeats the point of Promise.all.
- await Promise.all([ - await u[0].services.users.createBapiUser(fakeUsers[0]), - await u[1].services.users.createBapiUser(fakeUsers[1]), - ]); + await Promise.all([ + u[0].services.users.createBapiUser(fakeUsers[0]), + u[1].services.users.createBapiUser(fakeUsers[1]), + ]);integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts (1)
15-15: Avoid magic number: hoist timeout to a named constant (optionally env-tunable).This keeps the value self-documenting and easy to tweak across environments (e.g., CI vs. local).
Apply this diff within this file:
- test.setTimeout(90_000); // Wait for app to be ready + test.setTimeout(STARTUP_TIMEOUT_MS); // Wait for app to be readyAdd this near the top of the file (or centralize in a shared test helper):
const STARTUP_TIMEOUT_MS = Number(process.env.APP_STARTUP_TIMEOUT_MS ?? '90000');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
.changeset/chilly-cities-write.md(1 hunks).changeset/rare-books-jam.md(1 hunks)integration/playwright.config.ts(1 hunks)integration/tests/components.test.ts(1 hunks)integration/tests/dynamic-keys.test.ts(1 hunks)integration/tests/elements/next-sign-in.test.ts(4 hunks)integration/tests/elements/next-sign-up.test.ts(5 hunks)integration/tests/global.setup.ts(1 hunks)integration/tests/global.teardown.ts(1 hunks)integration/tests/handshake.test.ts(3 hunks)integration/tests/legal-consent.test.ts(1 hunks)integration/tests/localhost/localhost-different-port-different-instance.test.ts(1 hunks)integration/tests/localhost/localhost-different-port-same-instance.test.ts(1 hunks)integration/tests/localhost/localhost-switch-instance.test.ts(1 hunks)integration/tests/machine-auth/api-keys.test.ts(2 hunks)integration/tests/machine-auth/m2m.test.ts(1 hunks)integration/tests/middleware-placement.test.ts(3 hunks)integration/tests/next-account-portal/clerk-v4-ap-core-1.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v5-ap-core-1.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts(1 hunks)integration/tests/next-account-portal/common.ts(1 hunks)integration/tests/next-build.test.ts(2 hunks)integration/tests/session-tasks-eject-flow.test.ts(1 hunks)integration/tests/sign-in-or-up-component.test.ts(1 hunks)integration/tests/sign-in-or-up-restricted-mode.test.ts(1 hunks)packages/clerk-js/bundlewatch.config.json(1 hunks)packages/clerk-js/jest.setup.ts(1 hunks)packages/clerk-js/package.json(1 hunks)packages/clerk-js/src/ui/baseTheme.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorOneCodeForm.spec.tsx(2 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorTwo.test.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)packages/clerk-js/src/ui/components/UserVerification/__tests__/UVFactorTwo.test.tsx(1 hunks)packages/clerk-js/src/ui/customizables/index.ts(1 hunks)packages/clerk-js/src/ui/elements/CodeControl.tsx(4 hunks)packages/clerk-js/src/ui/elements/Form.tsx(2 hunks)packages/clerk-js/src/ui/elements/InputGroup.tsx(1 hunks)packages/clerk-js/src/ui/elements/PhoneInput/index.tsx(1 hunks)packages/clerk-js/src/ui/elements/__tests__/CodeControl.spec.tsx(4 hunks)packages/clerk-js/src/ui/primitives/Input.tsx(1 hunks)packages/clerk-js/vitest.setup.mts(1 hunks)packages/testing/src/playwright/unstable/page-objects/common.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (40)
- packages/clerk-js/src/ui/elements/InputGroup.tsx
- packages/clerk-js/jest.setup.ts
- integration/tests/next-account-portal/clerk-v5-ap-core-1.test.ts
- integration/tests/sign-in-or-up-restricted-mode.test.ts
- packages/clerk-js/package.json
- packages/clerk-js/src/ui/elements/PhoneInput/index.tsx
- integration/tests/global.teardown.ts
- integration/tests/localhost/localhost-different-port-different-instance.test.ts
- integration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts
- integration/tests/localhost/localhost-switch-instance.test.ts
- integration/tests/machine-auth/api-keys.test.ts
- packages/clerk-js/vitest.setup.mts
- packages/clerk-js/src/ui/components/SignIn/tests/SignInFactorOneCodeForm.spec.tsx
- packages/clerk-js/src/ui/baseTheme.ts
- .changeset/rare-books-jam.md
- integration/tests/elements/next-sign-in.test.ts
- integration/tests/legal-consent.test.ts
- integration/tests/components.test.ts
- integration/tests/elements/next-sign-up.test.ts
- integration/tests/machine-auth/m2m.test.ts
- packages/clerk-js/src/ui/components/SignIn/tests/SignInFactorTwo.test.tsx
- integration/tests/next-build.test.ts
- integration/playwright.config.ts
- integration/tests/next-account-portal/clerk-v4-ap-core-1.test.ts
- packages/clerk-js/src/ui/customizables/index.ts
- .changeset/chilly-cities-write.md
- integration/tests/middleware-placement.test.ts
- integration/tests/sign-in-or-up-component.test.ts
- integration/tests/next-account-portal/common.ts
- integration/tests/dynamic-keys.test.ts
- integration/tests/handshake.test.ts
- packages/clerk-js/src/ui/components/UserVerification/tests/UVFactorTwo.test.tsx
- integration/tests/session-tasks-eject-flow.test.ts
- packages/clerk-js/src/ui/components/SignIn/tests/SignInStart.test.tsx
- packages/clerk-js/src/ui/primitives/Input.tsx
- packages/clerk-js/src/ui/elements/CodeControl.tsx
- packages/testing/src/playwright/unstable/page-objects/common.ts
- integration/tests/global.setup.ts
- packages/clerk-js/src/ui/elements/Form.tsx
- packages/clerk-js/src/ui/elements/tests/CodeControl.spec.tsx
🧰 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:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.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:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.tspackages/clerk-js/bundlewatch.config.json
**/*.{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:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
integration/**
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
integration/**/*
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.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:
integration/tests/localhost/localhost-different-port-same-instance.test.tsintegration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
integration/tests/localhost/localhost-different-port-same-instance.test.ts (1)
24-24: Setup-only timeout is appropriate for multi-app boot; LGTM.Scoping the 90s timeout to beforeAll avoids slowing individual tests and helps absorb occasional app-start variability. Change looks good.
integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts (2)
15-15: Good use of a scoped timeout in beforeAll for app bootstrap.Calling
test.setTimeout(90_000)insidebeforeAlllimits the longer timeout to the setup hook only and matches the updated Playwright config (shorter action/navigation timeouts, no global test timeout). This should improve stability without masking slow tests.
15-15: Verify scoped timeouts for allapp.dev()-based suitesPlease ensure that every test suite which spins up the dev server via
app.dev()in itsbeforeAllblock also declares its owntest.setTimeout(...)immediately around that call. Relying solely on a global timeout risks flakiness now that it’s been removed.• File: integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
– Line 15 already hastest.setTimeout(90_000)scoped to itsbeforeAll.
• Please manually audit the remaining integration tests under integration/tests/** for any otherbeforeAll(async () => await app.dev())and add a nearbytest.setTimeout(...)if missing.packages/clerk-js/bundlewatch.config.json (2)
9-9: Manual verification required: vendor chunk files not foundIt looks like the
packages/clerk-js/distdirectory isn’t present (the size-listing script failed with “not a directory”), so we can’t verify thevendors*.jsbudgets until the build artifacts exist. Please:
- Build the Clerk JS bundle (e.g. run
npm run buildor equivalent inpackages/clerk-js) to generatedist/vendors*.js.- Rerun the size check on those files, for example:
fd 'vendors*.js' packages/clerk-js/dist \ -x bash -lc 'for f; do printf "%-55s %8.2fKB\n" "$f" "$(wc -c <"$f" | awk "{print \$1/1024}")" done'- Use your source maps or a bundle analyzer to inspect which modules are contributing. For a quick grep:
rg -n --no-heading -S "input-otp" packages/clerk-js/dist/vendors*.js || true- Ensure that
input-otp(and any OTP-specific polyfills) end up in the app chunk if they’re not shared broadly.- Consider simplifying the budget to a round number (e.g.
44KB) or setting it to the current gzipped size plus ~1% headroom to avoid flakiness from minor build variance.
3-3: Slight budget bump for clerk.js: measure locally and tighten headroomWe can’t verify gzip sizes until the dist artifacts are generated, so please:
- Run your standard build (e.g.
npm run buildorpnpm build) inpackages/clerk-jsto producedist/*.js.- Re-run the bundlewatch script (or the provided shell snippet) against the freshly generated
dist/clerk.js:# from repo root cd packages/clerk-js # build step npm run build # verify sizes node ../bundle-check.mjs --config bundlewatch.config.json- Note the gzip size of
dist/clerk.js, then update its maxSize inbundlewatch.config.jsonto measured-gzip + ~1% headroom. For example:{ "path": "./dist/clerk.js",- "maxSize": "631KB"
- "maxSize": "<MEASURED_GZIP_KB * 1.01 rounded>KB"
}- Confirm that the only runtime import of `input-otp` is in `src/ui/elements/CodeControl.tsx`, and that it’s tree-shaken into the appropriate bundles (not duplicated across other entries). Once you’ve updated the config to the precise measured size, this will ensure we catch any unintended bundle growth. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tmilewski
force-pushed
the
tm/user-2571-otp
branch
from
0beb254 to
9e57414
Compare
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/ui/primitives/Input.tsx (1)
98-112: Bug: disabled attribute ignores form-field disabled stateYou compute
_disabled = isDisabled || fieldControlProps.isDisabled, but passdisabled={isDisabled}to the input. This desynchronizesdisabledandaria-disabled, and ignores the form context’s disabled state.Apply this diff:
return ( <input {...rest} {...typeProps} {...passwordManagerProps} ref={ref} onChange={onChange} - disabled={isDisabled} + disabled={_disabled} required={_required} id={props.id || fieldControlProps.id} aria-invalid={_hasError} aria-describedby={errorMessageId ? errorMessageId : undefined} aria-required={_required} aria-disabled={_disabled} data-feedback={feedbackType} data-variant={props.variant || 'default'} css={applyVariants(props)} /> );
♻️ Duplicate comments (8)
integration/tests/components.test.ts (1)
106-109: Conditional sign-out mirrors conditional sign-in; resolves earlier feedback — LGTMOnly sign out when a protected component actually signed the user in. This prevents unnecessary calls to
window.Clerk.signOut()and aligns with the prior comment about skipping sign-out when never signed in.packages/clerk-js/src/ui/primitives/Input.tsx (1)
125-146: DivInput: fix sanitizeInputProps usage, remove stale comment, and derive focus/error consistently
- The ts-expect-error is stale here.
- Requesting 'data-active' from sanitizeInputProps is invalid (not in form-field context).
- Derive hasError from form-field context when not provided via props.
- Compute data-focus-within from explicit focusRing or field isFocused for consistent theming.
Apply this diff:
export const DivInput = React.forwardRef<HTMLDivElement, DivInputProps>((props, ref) => { const fieldControl = useFormField() || {}; - // @ts-expect-error Typescript is complaining that `errorMessageId` does not exist. We are clearly passing them from above. - const { feedbackType } = sanitizeInputProps(fieldControl, ['feedbackType', 'data-active']); + const { feedbackType, isFocused, ...fieldControlProps } = sanitizeInputProps(fieldControl, [ + 'feedbackType', + 'isFocused', + ]); const propsWithoutVariants = filterProps({ ...props, - hasError: props.hasError, + hasError: props.hasError || fieldControlProps.hasError, }); - const { isDisabled, hasError, focusRing, isRequired, ...rest } = propsWithoutVariants; + const { hasError, focusRing, ...rest } = propsWithoutVariants; return ( <div {...rest} ref={ref} id={props.id} aria-invalid={hasError} data-feedback={feedbackType} data-variant={props.variant || 'default'} - data-focus-within={focusRing} + data-focus-within={Boolean(focusRing || isFocused)} css={applyVariants(props)} /> ); });Optional: If segments are visual-only, consider allowing aria-hidden as an opt-in prop and documenting it in DivInput’s JSDoc.
packages/clerk-js/src/ui/elements/CodeControl.tsx (3)
110-113: Reset should also clear feedback to fully recover the controlAfter an error, feedback persists;
resetonly clears values, so the control can remain disabled or styled as error. Clear feedback here.Apply this diff:
- const reset = React.useCallback(() => { - setValues(Array.from({ length }, () => '')); - }, [length]); + const reset = React.useCallback(() => { + setValues(Array.from({ length }, () => '')); + clearFeedback?.(); + }, [length, clearFeedback]);
166-176: Derive disabled directly from feedback (and drop sticky local state)The transient
disabledstate is set to true when feedback appears, but never reset on feedback clearing. Derive it fromfeedbackso the control re-enables automatically.Apply this diff:
export const OTPCodeControl = () => { - const [disabled, setDisabled] = React.useState(false); const { otpControl, isLoading, isDisabled } = useOTPInputContext(); const { feedback, values, setValues, feedbackType, length } = otpControl.otpInputProps; - - React.useEffect(() => { - if (feedback) { - setDisabled(true); - } - }, [feedback]); + const disabled = Boolean(feedback);No other logic changes required, as each Slot already ORs
isDisabled || isLoading || disabled || feedbackType === 'success'. The deriveddisabledwill now flip back to false when feedback clears.
8-11: Add autoComplete='one-time-code' and localize the aria-label (essential for Chrome iOS autofill and a11y)Hard-coded 'Enter verification code' violates localization guidelines and often prevents iOS Chrome OTP autofill from triggering reliably without
autoComplete='one-time-code'.Apply this diff:
-import { Box, descriptors, Flex, OTPInputSegment } from '../customizables'; +import { Box, descriptors, Flex, OTPInputSegment, localizationKeys } from '../customizables';-import { useLoadingStatus } from '../hooks'; +import { useLoadingStatus, useLocalizations } from '../hooks';export const OTPCodeControl = () => { + const { t } = useLocalizations(); const { otpControl, isLoading, isDisabled } = useOTPInputContext(); const { feedback, values, setValues, feedbackType, length } = otpControl.otpInputProps;<OTPInput - aria-label='Enter verification code' + aria-label={t(localizationKeys('formFieldOTPInput__label'))} + autoComplete='one-time-code' aria-required maxLength={length} inputMode='numeric' pattern={REGEXP_ONLY_DIGITS} textAlign='center'If a more specific localization key exists for this control, prefer it. Ensure tests updated to not rely on the literal string.
Also applies to: 166-173, 182-189
packages/testing/src/playwright/unstable/page-objects/common.ts (3)
24-31: Defaulting awaitPrepare to true risks indefinite hangs; prefer false with a soft-wait.If prepare_* already fired before calling enterOtpCode, an unconditional wait can hang. Make the default false and let tests opt into prepare-wait when needed.
Apply this diff:
const { name = 'Enter verification code', - awaitAttempt = true, - awaitPrepare = true, + awaitAttempt = true, + awaitPrepare = false, awaitRequests = true, } = opts ?? {};
32-39: Indefinite wait on prepare_*; add timeout and broaden endpoint coverage.Use a soft-wait to avoid hangs and include prepare_second_factor to cover MFA flows.
Apply this diff:
- if (awaitRequests && awaitPrepare) { - const prepareVerificationPromise = page.waitForResponse( - response => - response.request().method() === 'POST' && - (response.url().includes('prepare_verification') || response.url().includes('prepare_first_factor')), - ); - await prepareVerificationPromise; - } + if (awaitRequests && awaitPrepare) { + await page + .waitForResponse( + response => + response.request().method() === 'POST' && + (response.url().includes('prepare_verification') || + response.url().includes('prepare_first_factor') || + response.url().includes('prepare_second_factor')), + { timeout: 2000 }, + ) + .catch(() => {}); + }
51-59: Race on attempt_ response; start the wait before fill (Promise.all) and add a timeout.*Auto-submit can fire immediately on fill and complete attempt_* before the wait starts, causing hangs. Start waiting first and run both concurrently; include attempt_second_factor and add a timeout as a safety net.
Minimal safety patch (timeout + endpoint coverage):
- if (awaitRequests && awaitAttempt) { - const attemptVerificationPromise = page.waitForResponse( - response => - response.request().method() === 'POST' && - (response.url().includes('attempt_verification') || response.url().includes('attempt_first_factor')), - ); - await attemptVerificationPromise; - } + if (awaitRequests && awaitAttempt) { + await page + .waitForResponse( + response => + response.request().method() === 'POST' && + (response.url().includes('attempt_verification') || + response.url().includes('attempt_first_factor') || + response.url().includes('attempt_second_factor')), + { timeout: 5000 }, + ) + .catch(() => {}); + }Robust refactor (recommended) to avoid the race entirely:
// Before entering the code above (after selecting which input to use), compute the fill action: let fillAction: Promise<void>; if (await otpInput.isVisible()) { fillAction = otpInput.fill(code); } else { fillAction = (async () => { await originalInput.click(); await page.keyboard.type(code, { delay: 100 }); })(); } if (awaitRequests && awaitAttempt) { await Promise.all([ page.waitForResponse( r => r.request().method() === 'POST' && (r.url().includes('attempt_verification') || r.url().includes('attempt_first_factor') || r.url().includes('attempt_second_factor')), ), fillAction, ]); } else { await fillAction; }
🧹 Nitpick comments (15)
integration/tests/components.test.ts (1)
95-110: Consider try/finally to guarantee cleanup even on assertion failuresIf
expectthrows, the sign-out won’t run and may leave server-side sessions around. Wrapping the body intry/finallykeeps the current conditional behavior while ensuring cleanup for protected components.Apply this diff within the test body:
// eslint-disable-next-line playwright/no-conditional-in-test if (component.protected) { await signIn({ app, page, context }); } - - const u = createTestUtils({ app, page, context }); - await u.page.goToRelative(component.path, { waitUntil: 'commit' }); - await expect(u.page.getByText(component.fallback)).toBeVisible(); - - // eslint-disable-next-line playwright/no-conditional-in-test - if (component.protected) { - await signOut({ app, page, context }); - } + try { + const u = createTestUtils({ app, page, context }); + await u.page.goToRelative(component.path, { waitUntil: 'commit' }); + await expect(u.page.getByText(component.fallback)).toBeVisible(); + } finally { + // eslint-disable-next-line playwright/no-conditional-in-test + if (component.protected) { + await signOut({ app, page, context }); + } + }integration/tests/next-account-portal/clerk-v5-ap-core-1.test.ts (2)
25-28: Guard cleanup in afterAll to avoid crashes if setup fails.If
beforeAllfails before assigningapporfakeUser,afterAllwould attempt to call methods onundefined. Add simple guards to keep teardown robust.test.afterAll(async () => { - await fakeUser.deleteIfExists(); - await app.teardown(); + if (fakeUser) { + await fakeUser.deleteIfExists(); + } + if (app) { + await app.teardown(); + } });
15-15: Centralize the 90s Test Setup Timeout Across Integration TestsWe’ve identified that the 90 000 ms hardcoded timeout appears in over 20 integration test files (e.g., dynamic-keys.test.ts, sign-in-or-up-.test.ts, session-tasks-eject-flow.test.ts, middleware-placement.test.ts, handshake.test.ts, next-build.test.ts, next-account-portal/.test.ts, localhost/.test.ts, machine-auth/.test.ts) after scanning the
integration/tests/directory. To improve consistency and make local vs CI behavior clearer, consider extracting a shared helper or configuration value rather than repeating90_000in each file.• Files with
test.setTimeout(90_000)occurrences:
- integration/tests/dynamic-keys.test.ts
- integration/tests/sign-in-or-up-restricted-mode.test.ts
- integration/tests/session-tasks-eject-flow.test.ts
- integration/tests/middleware-placement.test.ts
- integration/tests/sign-in-or-up-component.test.ts
- integration/tests/next-build.test.ts
- integration/tests/handshake.test.ts
- integration/tests/next-account-portal/clerk-v4-ap-core-*.test.ts
- integration/tests/next-account-portal/clerk-v5-ap-core-*.test.ts
- integration/tests/localhost/*.test.ts
- integration/tests/machine-auth/*.test.ts
• Suggested helper (e.g., in
test/setupTimeout.ts):// test/setupTimeout.ts export const setupTimeoutMs = process.env.CI ? 90_000 : 45_000;• Then in each test beforeAll:
- test.setTimeout(90_000); // Wait for app to be ready + import { setupTimeoutMs } from '../test/setupTimeout'; + test.setTimeout(setupTimeoutMs); // CI tends to be slower; stricter locallyIf you’d rather apply a localized tweak first, update this file as:
- test.setTimeout(90_000); // Wait for app to be ready + const setupTimeoutMs = process.env.CI ? 90_000 : 45_000; + test.setTimeout(setupTimeoutMs); // CI tends to be slower; be stricter locallyFinally, you can re-run:
rg -nP -C1 'test\.setTimeout\(\s*90_000\s*\)' -g '*.ts' -g '*.tsx' integration/to confirm all instances are updated.
integration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts (2)
25-28: Defensive teardown: guard against partial initializationIf beforeAll throws before assigning app or fakeUser, afterAll could throw. Optional guard prevents noisy teardown failures.
Apply this diff:
test.afterAll(async () => { - await fakeUser.deleteIfExists(); - await app.teardown(); + await fakeUser?.deleteIfExists(); + await app?.teardown(); });
15-15: Prefer describe-level timeout for suite testsIf your intention is to extend the timeout for all tests in this suite (not just the setup hook), you can configure the suite-level timeout on the
describeblock rather than callingtest.setTimeoutinside each hook:test.describe('Next with ClerkJS V4 <-> Account Portal Core 2 @ap-flows', () => { - test.describe.configure({ mode: 'serial' }); + test.describe.configure({ mode: 'serial', timeout: 90_000 }); let app: Application; let fakeUser: FakeUser; test.beforeAll(async () => { - test.setTimeout(90_000); // Wait for app to be ready + // The suite-level timeout above covers setup and individual tests. app = await appConfigs.next.appRouterAPWithClerkNextV4.clone().commit();Verification results:
- The grep run shows dozens of tests under
integration/testsstill usingtest.setTimeout(90_000)in theirbeforeAllhooks (e.g.localhost-different-port-*.test.ts,sign-in-or-up-component.test.ts,handshake.test.ts, etc.).- No global
timeout: 90_000is configured inintegration/playwright.config.ts.- There is precedent for suite-level timeouts (e.g.
timeout: 5 * 60 * 1000inlocalhost-switch-instance.test.ts).Recommendation:
- This refactor is optional—if you prefer consistency and want the entire suite (setup + tests) to share the same extended timeout, apply the above diff.
- Otherwise, leaving the per-hook
test.setTimeoutcalls is fine, as long as you’re aware of the difference in scope.packages/clerk-js/src/ui/primitives/Input.tsx (1)
134-135: Remove unused destructured props to satisfy ESLint (no-unused-vars)
isDisabledandisRequiredare destructured but unused in DivInput. This will fail the project’s ESLint rules.Apply this diff (already included in the previous change, but calling it out explicitly):
- const { isDisabled, hasError, focusRing, isRequired, ...rest } = propsWithoutVariants; + const { hasError, focusRing, ...rest } = propsWithoutVariants;packages/clerk-js/src/ui/elements/CodeControl.tsx (2)
178-181: Preserve the container descriptor to avoid theming regressionsPrevious feedback requested preserving
elementDescriptor={descriptors.otpCodeFieldInputs}on the container. Current code usesotpCodeFieldInputContainer. If theming/appearance depends on the former, this is a breaking change.Proposed change (verify the descriptor exists in elementDescriptors and baseTheme):
- <Box - elementDescriptor={descriptors.otpCodeFieldInputContainer} + <Box + elementDescriptor={descriptors.otpCodeFieldInputs} sx={{ position: 'relative' }} >If both are valid and intentionally different, document the migration in the changeset to guide theme authors.
256-289: Hoist keyframes out of FakeCaret to avoid repeated style injectionEmbedding the @Keyframes block inside the component injects a <style> tag on every mount. Prefer a single global registration (theme keyframes helper or a top-level CSS module) and reference it via className or sx.
Example (conceptual):
- <style>{`@keyframes caret-blink-animation { ... }`}</style> + {/* register caret-blink-animation globally (e.g., baseTheme or CSS) and reference it here */} <Flex sx={() => ({ animation: 'caret-blink-animation 1s infinite', ... })} >If the styled system exposes a keyframes helper, use it to register once and reference by name.
packages/testing/src/playwright/unstable/page-objects/common.ts (7)
3-8: Export and document EnterOtpCodeOptions (public API surface).This type shapes the public signature of returned helpers (enterOtpCode/enterTestOtpCode/fillTestOtpCode). Per repo guidelines, public API types should be exported and documented with JSDoc.
Apply this diff:
-type EnterOtpCodeOptions = { +/** Options for OTP entry helpers. */ +export type EnterOtpCodeOptions = { name?: string; awaitRequests?: boolean; awaitPrepare?: boolean; awaitAttempt?: boolean; };
44-45: Use a debug logger or test step annotation instead of console.warn.console.warn adds noise to test output. Prefer test.step/debug or a scoped logger (e.g., using
debug) gated by an env flag.
60-66: Use @deprecated JSDoc for typed deprecation and steer consumers to enterTestOtpCode.The inline comment won’t show up in editor tooltips. Prefer JSDoc deprecation.
Apply this diff:
- // @deprecated Use .enterTestOtpCode({ name: '...' }) instead - fillTestOtpCode: async (name: string, opts?: Omit<EnterOtpCodeOptions, 'name'>) => { + /** @deprecated Use .enterTestOtpCode({ name: '...' }) instead */ + fillTestOtpCode: async (name: string, opts?: Omit<EnterOtpCodeOptions, 'name'>) => { return self.enterOtpCode('424242', { name, ...opts }); },
24-59: Optional: factor URL matching into helpers to reduce duplication and drift.Centralize predicates for prepare_* and attempt_* so new factors (e.g., second_factor) are added once.
Example helper (place near top of file):
const isPrepareUrl = (u: string) => u.includes('prepare_verification') || u.includes('prepare_first_factor') || u.includes('prepare_second_factor'); const isAttemptUrl = (u: string) => u.includes('attempt_verification') || u.includes('attempt_first_factor') || u.includes('attempt_second_factor');Then:
await page.waitForResponse(r => r.request().method() === 'POST' && isPrepareUrl(r.url()), { timeout: 2000 }).catch(() => {}); await page.waitForResponse(r => r.request().method() === 'POST' && isAttemptUrl(r.url()), { timeout: 5000 }).catch(() => {});
10-99: Add an explicit return type for common and a public interface for the page object.Per guidelines, public APIs should have explicit return types. Define a
CommonPageObjectinterface and annotatecommon.Example:
export interface CommonPageObject { continue: () => Promise<void>; setEmailAddress: (val: string) => Promise<void>; setPassword: (val: string) => Promise<void>; setPasswordConfirmation: (val: string) => Promise<void>; enterOtpCode: (code: string, opts?: EnterOtpCodeOptions) => Promise<void>; enterTestOtpCode: (opts?: EnterOtpCodeOptions) => Promise<void>; /** @deprecated Use enterTestOtpCode */ fillTestOtpCode: (name: string, opts?: Omit<EnterOtpCodeOptions, 'name'>) => Promise<void>; // ...rest of getters } export const common = ({ page }: { page: EnhancedPage }): CommonPageObject => { /* ... */ };
41-49: Consider configurable typing delay for the legacy path to speed up local runs.100ms per digit is slow (600ms per code). Make it an option with a sane default (20–40ms), e.g.,
legacyTypingDelayMs?: numberin EnterOtpCodeOptions.
24-59: Verification Results & Next Steps
- Found
prepare_second_factorandattempt_second_factorendpoints in the API resources:
packages/clerk-js/src/core/resources/Session.ts(lines 268, 282)packages/clerk-js/src/core/resources/SignIn.ts(actionsprepare_second_factor,attempt_second_factor)- The Playwright page-object (
packages/testing/src/playwright/unstable/page-objects/common.ts) currently only waits forprepare_verification/prepare_first_factorandattempt_verification/attempt_first_factorresponses. It does not includesecond_factorvariants.- All existing call sites of
enterOtpCode(in the samecommon.tshelpers and in the integration tests underintegration/tests/andintegration/deployments/) rely on the defaultawaitPrepare = true(none explicitly override it).
→ Changing the default tofalsewould disable waiting for the “prepare” request in every test and likely introduce race conditions.Action items:
- If you intend to support second-factor OTP flows in these page objects, update the response-waiting predicates to also match
prepare_second_factorandattempt_second_factor.- If you flip the default
awaitPrepare, add explicit overrides (awaitPrepare: true) at any call site that still needs to wait for the prepare request, or adjust the tests to work with the new behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
.changeset/chilly-cities-write.md(1 hunks).changeset/rare-books-jam.md(1 hunks)integration/playwright.config.ts(1 hunks)integration/tests/components.test.ts(1 hunks)integration/tests/dynamic-keys.test.ts(1 hunks)integration/tests/elements/next-sign-in.test.ts(4 hunks)integration/tests/elements/next-sign-up.test.ts(5 hunks)integration/tests/global.setup.ts(1 hunks)integration/tests/global.teardown.ts(1 hunks)integration/tests/handshake.test.ts(3 hunks)integration/tests/legal-consent.test.ts(1 hunks)integration/tests/localhost/localhost-different-port-different-instance.test.ts(1 hunks)integration/tests/localhost/localhost-different-port-same-instance.test.ts(1 hunks)integration/tests/localhost/localhost-switch-instance.test.ts(1 hunks)integration/tests/machine-auth/api-keys.test.ts(2 hunks)integration/tests/machine-auth/m2m.test.ts(1 hunks)integration/tests/middleware-placement.test.ts(3 hunks)integration/tests/next-account-portal/clerk-v4-ap-core-1.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v5-ap-core-1.test.ts(1 hunks)integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts(1 hunks)integration/tests/next-account-portal/common.ts(1 hunks)integration/tests/next-build.test.ts(2 hunks)integration/tests/session-tasks-eject-flow.test.ts(1 hunks)integration/tests/sign-in-or-up-component.test.ts(1 hunks)integration/tests/sign-in-or-up-restricted-mode.test.ts(1 hunks)packages/clerk-js/bundlewatch.config.json(1 hunks)packages/clerk-js/jest.setup.ts(1 hunks)packages/clerk-js/package.json(1 hunks)packages/clerk-js/src/ui/baseTheme.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorOneCodeForm.spec.tsx(2 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorTwo.test.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)packages/clerk-js/src/ui/components/UserVerification/__tests__/UVFactorTwo.test.tsx(1 hunks)packages/clerk-js/src/ui/customizables/index.ts(1 hunks)packages/clerk-js/src/ui/elements/CodeControl.tsx(4 hunks)packages/clerk-js/src/ui/elements/Form.tsx(2 hunks)packages/clerk-js/src/ui/elements/InputGroup.tsx(1 hunks)packages/clerk-js/src/ui/elements/PhoneInput/index.tsx(1 hunks)packages/clerk-js/src/ui/elements/__tests__/CodeControl.spec.tsx(4 hunks)packages/clerk-js/src/ui/primitives/Input.tsx(1 hunks)packages/clerk-js/vitest.setup.mts(1 hunks)packages/testing/src/playwright/unstable/page-objects/common.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (37)
- integration/tests/localhost/localhost-different-port-different-instance.test.ts
- packages/clerk-js/src/ui/components/UserVerification/tests/UVFactorTwo.test.tsx
- packages/clerk-js/src/ui/components/SignIn/tests/SignInFactorTwo.test.tsx
- packages/clerk-js/src/ui/elements/InputGroup.tsx
- integration/tests/legal-consent.test.ts
- .changeset/chilly-cities-write.md
- integration/tests/session-tasks-eject-flow.test.ts
- packages/clerk-js/package.json
- .changeset/rare-books-jam.md
- integration/tests/middleware-placement.test.ts
- integration/tests/sign-in-or-up-restricted-mode.test.ts
- integration/tests/dynamic-keys.test.ts
- integration/tests/next-account-portal/clerk-v4-ap-core-1.test.ts
- packages/clerk-js/vitest.setup.mts
- integration/tests/sign-in-or-up-component.test.ts
- integration/tests/localhost/localhost-different-port-same-instance.test.ts
- integration/tests/global.teardown.ts
- integration/tests/next-account-portal/common.ts
- integration/tests/elements/next-sign-up.test.ts
- integration/tests/machine-auth/m2m.test.ts
- integration/tests/next-account-portal/clerk-v5-ap-core-2.test.ts
- integration/tests/localhost/localhost-switch-instance.test.ts
- packages/clerk-js/src/ui/components/SignIn/tests/SignInStart.test.tsx
- packages/clerk-js/src/ui/baseTheme.ts
- integration/playwright.config.ts
- integration/tests/elements/next-sign-in.test.ts
- packages/clerk-js/src/ui/components/SignIn/tests/SignInFactorOneCodeForm.spec.tsx
- integration/tests/machine-auth/api-keys.test.ts
- integration/tests/next-build.test.ts
- integration/tests/handshake.test.ts
- packages/clerk-js/src/ui/customizables/index.ts
- packages/clerk-js/src/ui/elements/PhoneInput/index.tsx
- packages/clerk-js/jest.setup.ts
- integration/tests/global.setup.ts
- packages/clerk-js/src/ui/elements/Form.tsx
- packages/clerk-js/bundlewatch.config.json
- packages/clerk-js/src/ui/elements/tests/CodeControl.spec.tsx
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.tspackages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.tspackages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{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:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.tspackages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
integration/**
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts
integration/**/*
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.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:
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.tsintegration/tests/components.test.tsintegration/tests/next-account-portal/clerk-v4-ap-core-2.test.tspackages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/testing/src/playwright/unstable/page-objects/common.tspackages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/primitives/Input.tsxpackages/clerk-js/src/ui/elements/CodeControl.tsx
🧬 Code graph analysis (2)
packages/clerk-js/src/ui/primitives/Input.tsx (2)
packages/clerk-js/src/ui/styledSystem/types.ts (2)
PrimitiveProps(58-58)RequiredProp(58-58)packages/clerk-js/src/ui/primitives/hooks/useFormField.tsx (1)
sanitizeInputProps(115-151)
packages/clerk-js/src/ui/elements/CodeControl.tsx (2)
packages/clerk-js/src/ui/customizables/index.ts (4)
Box(15-15)descriptors(10-10)Flex(16-16)OTPInputSegment(35-37)packages/clerk-js/src/ui/customizables/elementDescriptors.ts (1)
descriptors(565-565)
⏰ 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). (6)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
integration/tests/next-account-portal/clerk-v5-ap-core-1.test.ts (1)
15-15: Timeout scoped to setup-only is appropriate (LGTM).Placing
test.setTimeout(90_000)insidebeforeAllisolates the longer allowance to the app boot process without impacting individual test cases. Matches the suite’s serial mode and sharedappfixture usage.integration/tests/next-account-portal/clerk-v4-ap-core-2.test.ts (1)
15-15: Correct scope: test.setTimeout in beforeAll only extends the hook timeout (good for app readiness)Line 15 correctly increases only the beforeAll hook’s timeout to 90s, leaving individual test timeouts to the config/defaults. This matches the intent “Wait for app to be ready” without broadly slowing tests. See Playwright docs on test.setTimeout behavior in hooks. (playwright.dev)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/clerk-js/src/ui/elements/CodeControl.tsx (4)
179-181: Verify elementDescriptor placement to preserve theming hooksThe container uses elementDescriptor={descriptors.otpCodeFieldInputContainer} while the inner Flex uses elementDescriptor={descriptors.otpCodeFieldInputs}. Past guidance asked to “preserve the descriptor on the container” as otpCodeFieldInputs. If dashboards/themes target otpCodeFieldInputs on the outer wrapper today, this change may break appearance overrides.
Questions:
- Which level (outer Box vs inner Flex) is the contract for otpCodeFieldInputs in our theming system?
- If the contract is the container, consider moving otpCodeFieldInputs back to the Box, or duplicating a more specific descriptor for the Flex (e.g., otpCodeFieldInputsGroup) to avoid selector churn.
Also applies to: 194-201
110-113: Reset should also clear feedback to fully recover the controlWithout clearing feedback, error/success state persists after reset, which can keep the control disabled and affect resend UX. Clear it here and fix the deps.
- const reset = React.useCallback(() => { - setValues(Array.from({ length }, () => '')); - }, [length]); + const reset = React.useCallback(() => { + setValues(Array.from({ length }, () => '')); + clearFeedback?.(); + }, [length, clearFeedback]);
166-176: Derived disabled state avoids “stuck disabled” bugThe transient state never flips back to false when feedback clears. Derive disabled directly from props/context so it automatically re-enables.
-export const OTPCodeControl = () => { - const [disabled, setDisabled] = React.useState(false); - const { otpControl, isLoading, isDisabled } = useOTPInputContext(); - const { feedback, values, setValues, feedbackType, length } = otpControl.otpInputProps; - - React.useEffect(() => { - if (feedback) { - setDisabled(true); - } - }, [feedback]); +export const OTPCodeControl = () => { + const { otpControl, isLoading, isDisabled } = useOTPInputContext(); + const { feedback, values, setValues, feedbackType, length } = otpControl.otpInputProps; + const disabled = Boolean(feedback) || isDisabled || isLoading || feedbackType === 'success';And update the slot usage below:
- isDisabled={isDisabled || isLoading || disabled || feedbackType === 'success'} + isDisabled={disabled}
182-193: Add autoComplete='one-time-code' and localize the accessible label (Chrome iOS autofill + i18n)Critical for USER-2571: the single transparent input must declare autoComplete='one-time-code'. Also replace the hard-coded aria-label with a localized string per guidelines.
Apply this diff here:
<OTPInput - aria-label='Enter verification code' + aria-label={t(localizationKeys('formFieldOTPInput__label'))} + autoComplete='one-time-code' + aria-invalid={feedbackType === 'error'} aria-required maxLength={length} inputMode='numeric' pattern={REGEXP_ONLY_DIGITS} - textAlign='center' value={values.join('')} onChange={newValue => { setValues(newValue.split('')); }}And add the necessary imports and hook usage:
- import { Box, descriptors, Flex, OTPInputSegment } from '../customizables'; + import { Box, descriptors, Flex, OTPInputSegment, localizationKeys } from '../customizables';- import { useLoadingStatus } from '../hooks'; + import { useLoadingStatus, useLocalizations } from '../hooks';Inside OTPCodeControl (top of function body):
- const { otpControl, isLoading, isDisabled } = useOTPInputContext(); + const { otpControl, isLoading, isDisabled } = useOTPInputContext(); + const { t } = useLocalizations();Optional (nice-to-have): add name='one-time-code' to help some autofill heuristics.
I can also update the tests to assert autocomplete/aria attributes are present. Do you want a patch for the spec files?
🧹 Nitpick comments (4)
packages/clerk-js/src/ui/elements/CodeControl.tsx (4)
190-192: Keep values array length stable to reduce downstream shape churnsetValues(newValue.split('')) shrinks/grows the array. Keeping a fixed-length array (padded with '') improves predictability for effects and any consumers relying on length.
- onChange={newValue => { - setValues(newValue.split('')); - }} + onChange={newValue => { + const chars = newValue.split('').slice(0, length); + if (chars.length < length) { + chars.length = length; + chars.fill('', newValue.length); + } + setValues(chars); + }}
123-124: Avoid values.toString() dependency hack; use proper depsRelying on toString() sidesteps ESLint and can mask bugs. Use the array and length as deps; onChange should be stable or wrapped.
- }, [values.toString()]); + }, [values, length]);If onChange identity changes, wrap it with useCallback at the source or read it from a ref.
220-223: Remove unused placeholderChar destructureplaceholderChar is unused and may trigger lint errors.
- const { char, hasFakeCaret, isActive, placeholderChar, ...rest } = otpProps; + const { char, hasFakeCaret, isActive, ...rest } = otpProps;
257-290: Move keyframes out of the component and respect reduced motionInlining <style> per render can duplicate keyframes and ignores prefers-reduced-motion. Define the animation once (global or theme) and gate it for motion-sensitive users.
Example approach:
- Hoist keyframes into a shared CSS/JS theme layer.
- Add a media query to disable the animation:
@media (prefers-reduced-motion: reduce) { .cl-otp-caret { animation: none; } }- Apply className='cl-otp-caret' on the animated Flex.
Minimal in-place change (if keeping inline styles):
- <Flex + <Flex + className='cl-otp-caret' align='center' justify='center' sx={() => ({ animation: 'caret-blink-animation 1s infinite',And include a reduced-motion rule in the <style> block.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/ui/elements/CodeControl.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{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/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{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/clerk-js/src/ui/elements/CodeControl.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/ui/elements/CodeControl.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{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/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/elements/CodeControl.tsx
**/*.{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/clerk-js/src/ui/elements/CodeControl.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/elements/CodeControl.tsx
🧬 Code graph analysis (1)
packages/clerk-js/src/ui/elements/CodeControl.tsx (2)
packages/clerk-js/src/ui/customizables/index.ts (4)
Box(15-15)descriptors(10-10)Flex(16-16)OTPInputSegment(35-37)packages/clerk-js/src/ui/customizables/elementDescriptors.ts (1)
descriptors(565-565)
⏰ 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). (6)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/clerk-js/src/ui/elements/CodeControl.tsx (1)
2-3: Good call on adopting input-otp primitivesTyped SlotProps with input-otp and using REGEXP_ONLY_DIGITS ensures correctness and keeps the value handling simple for the transparent input approach.
| pattern={`[0-9]{${length}}`} | ||
| minLength={length} | ||
| <OTPInput | ||
| aria-label='Enter verification code' |
There was a problem hiding this comment.
Should this be localized?
There was a problem hiding this comment.
Some of the existing ones weren't. I can follow-up with this.
Description
Our OTP woes largely come down to one thing: multiple individual inputs.
The interim fixes included adding a bi-directional sync between these fields and a single hidden input and while this helped a lot with password managers, it didn't solve the underlying issue.
This updates OTP to use a single transparent field via
input-otp.Other required updates:
@clerk/testingcommon utilities, in a non-breaking way, to support this new element as well as the old one.prepareandattemptrequests over adding timeouts and delays.Known (intentional) differences from our existing approach:
Fixes USER-2571
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores
Tests