refactor(clerk-js): Ensure last used badge does not show with a single strategy #7224
Conversation
🦋 Changeset detectedLatest commit: 7387aa2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
🔇 Additional comments (1)
Comment |
…hould-not-render-if-there-is-only-1
…ender-if-there-is-only-1' of github.com:clerk/javascript into alexcarpenter/user-3976-last-used-strategy-should-not-render-if-there-is-only-1
|
|
||
| const totalCount = firstFactorCount + oauthCount + web3Count + alternativePhoneCodeCount; | ||
|
|
||
| return { totalCount }; |
There was a problem hiding this comment.
if you return this value as totalEnabledAuthMethods, you won't have to rename it every time we use the hook.
const { totalEnabledAuthMethods } = useTotalEnabledAuthMethods();also since you're only returning one value you don't really need to wrap it in an object. However, if you prefer the object notation, I think this might be a little cleaner:
const totalEnabledAuthMethods = useTotalEnabledAuthMethods();
// ...
if (totalEnabledAuthMethods.count > 1) {
// ...
}
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/elements/__tests__/SocialButtons.test.tsx (2)
159-210: Clarify intent and reduce overlap between the two “email enabled” testsLines 159‑186 and 188‑210 both cover “single social provider + email enabled” and assert the badge is shown. This is good behavior coverage but a bit redundant, and the second test’s name (
email/username is also enabled) no longer matches the setup (onlywithEmailAddressis called).Consider either:
- Differentiating the second test by actually enabling username (and/or another identifier) so it exercises a distinct configuration, or
- Collapsing to a single test if you don’t need two variants of “google + email”.
This will keep the suite focused and avoid confusion from a misleading test description.
310-331: SAML scenarios are well covered; consider mirroring the hook assertion from the OAuth testThe updated SAML test (lines 310‑331) now runs with email enabled, and the new test (lines 333‑358) covers the “single total auth method + SAML” case where the badge should be suppressed. That combination matches the new rules and the PR objective.
Two small improvements you might consider:
- In the SAML single‑auth test, optionally assert
useTotalEnabledAuthMethods()returns1, mirroring the earlier OAuth single‑auth test for extra safety.- In the multi‑auth SAML test, you could also assert the presence of the “Last used” badge (not just “Continue with Google”) to confirm that SAML→OAuth conversion participates fully in the last‑auth logic.
Both are non‑blocking but would tighten the regression net around SAML behavior.
Also applies to: 333-358
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx(3 hunks)packages/clerk-js/src/ui/elements/SocialButtons.tsx(3 hunks)packages/clerk-js/src/ui/elements/__tests__/SocialButtons.test.tsx(4 hunks)packages/clerk-js/src/ui/hooks/useTotalEnabledAuthMethods.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/ui/hooks/useTotalEnabledAuthMethods.ts
- packages/clerk-js/src/ui/elements/SocialButtons.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/clerk-js/src/ui/elements/__tests__/SocialButtons.test.tsx (3)
packages/clerk-js/src/test/utils.ts (2)
renderHook(77-77)screen(72-72)packages/clerk-js/src/ui/hooks/useTotalEnabledAuthMethods.ts (1)
useTotalEnabledAuthMethods(6-22)packages/clerk-js/src/ui/elements/SocialButtons.tsx (1)
SocialButtons(56-228)
packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx (1)
packages/clerk-js/src/ui/hooks/useTotalEnabledAuthMethods.ts (1)
useTotalEnabledAuthMethods(6-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx (3)
40-40: LGTM!The import is correctly added to support the new functionality for conditionally displaying the "last used" badge.
91-91: LGTM!The hook is correctly invoked following React conventions, and the variable name clearly conveys its purpose.
529-532: Logic correctly implements the PR objective.The
useTotalEnabledAuthMethodshook accurately counts authentication methods available to users. The hook filters strategies to include only known/supported methods throughuseEnabledThirdPartyProviders, which returns the exact same filtered OAuth, Web3, and alternative phone code values rendered by SocialButtons. First-factor identifiers (displayed as form fields) are counted without filtering and remain fully displayable. The condition correctly ensures the "last used" badge displays only when multiple authentication methods are available, eliminating redundant UI indicators in single-method scenarios.packages/clerk-js/src/ui/elements/__tests__/SocialButtons.test.tsx (3)
2-7: Imports forrenderHookanduseTotalEnabledAuthMethodslook appropriateBringing in
renderHookanduseTotalEnabledAuthMethodsdirectly in this test file makes sense given the new hook‑level assertions; no issues here.
121-157: Nice coverage of the single total‑auth‑method caseThis test thoroughly validates the “only one enabled auth method” scenario by:
- Explicitly disabling email/username,
- Inspecting
userSettingsfor identifiers/social strategies, and- Verifying
useTotalEnabledAuthMethodsreturns1before asserting that the badge is hidden.This gives strong confidence that the new
totalEnabledAuthMethods > 1condition behaves as intended.
212-216: Explicitly enabling email here aligns the test with the “multiple auth methods” descriptionAdding
withEmailAddress({ enabled: true, used_for_first_factor: true })ensures that this scenario genuinely has more than one auth method, matching both the test name and thetotalEnabledAuthMethods‑based logic. The expectations that both “Continue with Google” and “Last used” appear are consistent with the updated behavior.
…hould-not-render-if-there-is-only-1
…ender-if-there-is-only-1' of github.com:clerk/javascript into alexcarpenter/user-3976-last-used-strategy-should-not-render-if-there-is-only-1
alexcarpenter
deleted the
alexcarpenter/user-3976-last-used-strategy-should-not-render-if-there-is-only-1
branch
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit