Conversation
This would take you back to re-enter the password, which never works so it was leading to a confusing dead end.
🦋 Changeset detectedLatest commit: 18269d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds a Changeset for a patch release and updates the sign-in flow in the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/SignIn/SignInFactorOne.tsx (1)
158-173:⚠️ Potential issue | 🟠 MajorUpdate/remove the test case that expects back navigation during pwned/compromised password flows.
The code change prevents back navigation when
passwordErrorCodeis present (line 159:const canGoBack = factorHasLocalStrategy(currentFactor) && !passwordErrorCode;). However, the test at line 306 inSignInFactorOne.test.tsx("entering a pwned password, then going back and clicking forgot password should result in the correct title") explicitly clicks the Back button during a pwned password scenario wherepasswordErrorCodewill be set to'pwned'. This test will fail because the Back button will no longer be clickable. Either remove this test or update it to validate that back navigation is intentionally disabled in error states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/SignIn/SignInFactorOne.tsx` around lines 158 - 173, The test expecting back navigation during a pwned/compromised password flow must be updated because SignInFactorOne now sets canGoBack = factorHasLocalStrategy(currentFactor) && !passwordErrorCode, which disables the Back button when passwordErrorCode (e.g., 'pwned') is present; modify the test "entering a pwned password, then going back and clicking forgot password should result in the correct title" in SignInFactorOne.test.tsx to either remove the Back click or assert that the backHandler is not invoked / the Back button is not rendered/clickable (inspect canGoBack/AlternativeMethods props or presence of onBackLinkClick) so the test reflects the new behavior. Ensure references to canGoBack, passwordErrorCode, backHandler, and the AlternativeMethods onBackLinkClick prop are used to drive the updated assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ui/src/components/SignIn/SignInFactorOne.tsx`:
- Around line 158-173: The test expecting back navigation during a
pwned/compromised password flow must be updated because SignInFactorOne now sets
canGoBack = factorHasLocalStrategy(currentFactor) && !passwordErrorCode, which
disables the Back button when passwordErrorCode (e.g., 'pwned') is present;
modify the test "entering a pwned password, then going back and clicking forgot
password should result in the correct title" in SignInFactorOne.test.tsx to
either remove the Back click or assert that the backHandler is not invoked / the
Back button is not rendered/clickable (inspect canGoBack/AlternativeMethods
props or presence of onBackLinkClick) so the test reflects the new behavior.
Ensure references to canGoBack, passwordErrorCode, backHandler, and the
AlternativeMethods onBackLinkClick prop are used to drive the updated
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d132934-db90-44cb-b82d-11c32b6ab187
📒 Files selected for processing (2)
.changeset/pink-taxes-do.mdpackages/ui/src/components/SignIn/SignInFactorOne.tsx
| if (showAllStrategies || showForgotPasswordStrategies) { | ||
| const canGoBack = factorHasLocalStrategy(currentFactor); | ||
| // Password errors are not recoverable by re-entering the password, so we hide the back button | ||
| const canGoBack = factorHasLocalStrategy(currentFactor) && !passwordErrorCode; |
There was a problem hiding this comment.
passwordErrorCode is to me not a very clear name for this flag (it sounds more general than it is) but I confirmed that it is just a union of 'compromised' | 'pwned' so it's exactly what we want to check.
3 participants
Description
These errors are not recoverable by re-entering the password, so the back button led to a confusing dead end that would always take you back to the same error.
Before:
After:
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change