feat(frontend): move 2FA management into dedicated tab with inline stepper#1649
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughExtracts two-factor authentication management into a dedicated ManageTwoFactor page and component, adds translations and a new account tab, and removes inline MFA/OTP workflows from the account settings index component. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Manage as "ManageTwoFactor<br/>Component"
participant Captcha as "Turnstile<br/>CAPTCHA"
participant EmailAPI as "Email OTP<br/>Endpoint"
participant MFAEnroll as "MFA Enroll<br/>Endpoint"
participant MFAVerify as "MFA Verify<br/>Endpoint"
User->>Manage: Open /settings/account/manage-2fa
Manage->>Manage: Load existing MFA factors, cleanup unverified
Manage->>Captcha: Render CAPTCHA (step 1)
User->>Captcha: Complete CAPTCHA
Captcha->>Manage: Return token
User->>Manage: Request OTP (step 2)
Manage->>EmailAPI: Send OTP (with CAPTCHA token)
EmailAPI->>User: Deliver OTP via email
User->>Manage: Submit OTP
Manage->>EmailAPI: Verify OTP
EmailAPI->>Manage: OTP valid
Manage->>MFAEnroll: Enroll TOTP factor (generate QR/secret)
MFAEnroll->>Manage: Return QR + secret
User->>Manage: Scan QR + enter TOTP code
Manage->>MFAVerify: Verify TOTP
MFAVerify->>Manage: TOTP verified
Manage->>Manage: Enable MFA, persist factor
Manage->>User: Show success notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/settings/account/ManageTwoFactor.vue (2)
244-247:as anycast on MFA factor — fragile property access.
created_atandupdated_atare accessed viaas anybecause they're not in the Supabase MFA factor type definition. If the API response changes, this silently returnsnull(which is handled), but it also hides potential type mismatches.Consider adding a brief comment explaining why the cast is necessary, or define a local interface extending the factor type with these optional fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/ManageTwoFactor.vue` around lines 244 - 247, The as any casts on verifiedFactor when setting mfaSetupDate are fragile; create a local interface (e.g., ExtendedMfaFactor) that extends the Supabase factor type with optional created_at and updated_at fields, then narrow/cast verifiedFactor to that interface when reading those properties, or add a concise TODO comment explaining why the cast is required and that these fields are optional and come from the API; update the code that sets mfaFactorId and mfaSetupDate to use the typed ExtendedMfaFactor instead of as any so property access is explicit and documented.
257-261:onBeforeUnmountwithasync— unenroll may not complete.Vue does not await async lifecycle hooks. The
supabase.auth.mfa.unenrollcall may be dropped if the component tears down before it completes. The risk is mitigated by thecleanupUnverifiedFactorscall inonMounted, which catches orphaned factors on next visit.No change strictly required since the safety net exists, but worth noting the intent vs. guarantee gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/ManageTwoFactor.vue` around lines 257 - 261, The onBeforeUnmount hook is declared async so the unenroll promise may be dropped; change it to a non-async handler that explicitly handles the promise (call supabase.auth.mfa.unenroll({ factorId: enrolledFactorId.value }).then(...).catch(...)) so errors are surfaced and the promise isn’t silently ignored, and if you need a guarantee the call completes before navigation use a route guard (e.g., beforeRouteLeave) to await supabase.auth.mfa.unenroll; reference the existing onBeforeUnmount and supabase.auth.mfa.unenroll calls (and keep cleanupUnverifiedFactors as the safety net).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/account/ManageTwoFactor.vue`:
- Around line 68-91: sendOtpVerification is reusing the single-use Cloudflare
Turnstile token (captchaToken.value) which causes Resend to fail; after a
successful OTP send (no error) clear the captcha token (set captchaToken.value
to '' or undefined) so subsequent calls don't resend an already-consumed token,
then continue to set otpVerificationCode and advance currentStep; keep using the
existing signInWithOtp call and the captchaToken.value || undefined guard so new
tokens can be supplied later.
---
Nitpick comments:
In `@src/pages/settings/account/ManageTwoFactor.vue`:
- Around line 244-247: The as any casts on verifiedFactor when setting
mfaSetupDate are fragile; create a local interface (e.g., ExtendedMfaFactor)
that extends the Supabase factor type with optional created_at and updated_at
fields, then narrow/cast verifiedFactor to that interface when reading those
properties, or add a concise TODO comment explaining why the cast is required
and that these fields are optional and come from the API; update the code that
sets mfaFactorId and mfaSetupDate to use the typed ExtendedMfaFactor instead of
as any so property access is explicit and documented.
- Around line 257-261: The onBeforeUnmount hook is declared async so the
unenroll promise may be dropped; change it to a non-async handler that
explicitly handles the promise (call supabase.auth.mfa.unenroll({ factorId:
enrolledFactorId.value }).then(...).catch(...)) so errors are surfaced and the
promise isn’t silently ignored, and if you need a guarantee the call completes
before navigation use a route guard (e.g., beforeRouteLeave) to await
supabase.auth.mfa.unenroll; reference the existing onBeforeUnmount and
supabase.auth.mfa.unenroll calls (and keep cleanupUnverifiedFactors as the
safety net).
…epper Replace the confusing inline 2FA sections on the General account page with a dedicated Manage 2FA tab that guides users through a clear 5-step wizard (CAPTCHA, send OTP, verify OTP, scan QR, verify TOTP) and shows a clean status view when 2FA is already enabled.
WcaleNieWolny
force-pushed
the
feat/manage-2fa-tab
branch
from
67f6244 to
f8eb9cd
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bc78d7ffc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }) | ||
| otpSending.value = false | ||
|
|
||
| savedCaptchaToken.value = '' |
There was a problem hiding this comment.
When signInWithOtp fails in step 2, the code clears savedCaptchaToken before checking error, and this step has no way to re-run CAPTCHA. In CAPTCHA-enforced environments, a transient send error (or an expired token) leaves the user stuck because every retry sends undefined as captchaToken until they reload the page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/settings/account/index.vue (1)
636-642:⚠️ Potential issue | 🟡 MinorHardcoded English strings — use
t()for i18n.Lines 638–641 contain raw English text that bypasses the translation system. These user-facing strings should use
t()keys for consistency with the rest of the component and to support localization.This action cannot be undone. Your account and all associated data will be permanently deleted. Your account will be deleted after 30 days🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 636 - 642, Replace the hardcoded English strings in the account deletion text with i18n calls using t(); specifically update the two <p> elements that currently contain "This action cannot be undone. Your account and all associated data will be permanently deleted." and "Your account will be deleted after 30 days" to use t('settings.account.deleteWarning') and t('settings.account.deleteAfter30Days') (or similar keys) in the template, and add corresponding keys to the locale files so translations are available; ensure you use the same i18n pattern used elsewhere in this component (t()) rather than raw strings.
🧹 Nitpick comments (2)
src/pages/settings/account/ManageTwoFactor.vue (1)
498-504: "Resend" restarts from CAPTCHA (step 1) — intentional but potentially frustrating UX.Since Turnstile tokens are single-use, going back to CAPTCHA is technically necessary. Consider labeling the button more descriptively (e.g.,
t('restart-verification')) so users understand why they're re-solving the CAPTCHA rather than just resending the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/ManageTwoFactor.vue` around lines 498 - 504, The current "Resend" button calls restartFromCaptcha which intentionally restarts the flow at the Turnstile CAPTCHA (step 1) but is misleading; change the button label to a clearer translation key (e.g., use t('restart-verification') instead of t('resend')) and add/update the corresponding i18n entry so the UI communicates that verification must be re-solved; keep the onClick handler (restartFromCaptcha) unchanged so the single-use Turnstile token flow still restarts correctly.src/pages/settings/account/index.vue (1)
450-455: Redirect does notawaitand lacks earlyreturn.
router.replace()returns a Promise. Although no code follows theifblock today, addingawait+returnfuture-proofs the hook and prevents any brief flash of the account page content before the navigation completes.Suggested fix
onMounted(async () => { // Auto-redirect to Manage 2FA page if setup2fa query param is present if (route.query.setup2fa === 'true') { - router.replace('/settings/account/manage-2fa?setup2fa=true') + await router.replace('/settings/account/manage-2fa?setup2fa=true') + return } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 450 - 455, The onMounted hook should await the navigation and stop further execution to avoid a flash; inside onMounted (the async function using route.query.setup2fa) change the redirect call to await router.replace('/settings/account/manage-2fa?setup2fa=true') and immediately return after that await so the hook exits early when setup2fa === 'true'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/account/ManageTwoFactor.vue`:
- Around line 239-244: The cleanupUnverifiedFactors function currently awaits
Promise.all of supabase.auth.mfa.unenroll calls which will reject if any call
fails and can abort onMounted; change it to a best-effort non-throwing cleanup
by using Promise.allSettled (or wrap each unenroll in try/catch) and swallow or
log individual errors instead of letting one rejection propagate, and ensure
callers (e.g., the onMounted that awaits cleanupUnverifiedFactors) do not rely
on it throwing so the page can finish initialization even if some unenrolls
fail.
- Around line 279-282: The code casts verifiedFactor to any to access
timestamps; remove the unnecessary "as any" casts and access created_at and
updated_at directly from verifiedFactor (the variables involved are
verifiedFactor, mfaFactorId, and mfaSetupDate). Update the block where
mfaFactorId.value is set and replace (verifiedFactor as any).created_at and
(verifiedFactor as any).updated_at with verifiedFactor.created_at and
verifiedFactor.updated_at so TypeScript uses the Supabase Factor type's fields
without casting.
---
Outside diff comments:
In `@src/pages/settings/account/index.vue`:
- Around line 636-642: Replace the hardcoded English strings in the account
deletion text with i18n calls using t(); specifically update the two <p>
elements that currently contain "This action cannot be undone. Your account and
all associated data will be permanently deleted." and "Your account will be
deleted after 30 days" to use t('settings.account.deleteWarning') and
t('settings.account.deleteAfter30Days') (or similar keys) in the template, and
add corresponding keys to the locale files so translations are available; ensure
you use the same i18n pattern used elsewhere in this component (t()) rather than
raw strings.
---
Duplicate comments:
In `@src/pages/settings/account/ManageTwoFactor.vue`:
- Around line 72-97: The review notes that the single-use CAPTCHA token handling
has been fixed: ensure sendOtpVerification clears savedCaptchaToken
(savedCaptchaToken.value = '') after each attempt and that restartFromCaptcha
(triggered by the "Resend" button) resets the flow back to step 1 (e.g., by
updating currentStep) so a fresh Turnstile token is obtained before the next
call to supabase.auth.signInWithOtp; keep these behaviors in sendOtpVerification
and restartFromCaptcha as implemented.
---
Nitpick comments:
In `@src/pages/settings/account/index.vue`:
- Around line 450-455: The onMounted hook should await the navigation and stop
further execution to avoid a flash; inside onMounted (the async function using
route.query.setup2fa) change the redirect call to await
router.replace('/settings/account/manage-2fa?setup2fa=true') and immediately
return after that await so the hook exits early when setup2fa === 'true'.
In `@src/pages/settings/account/ManageTwoFactor.vue`:
- Around line 498-504: The current "Resend" button calls restartFromCaptcha
which intentionally restarts the flow at the Turnstile CAPTCHA (step 1) but is
misleading; change the button label to a clearer translation key (e.g., use
t('restart-verification') instead of t('resend')) and add/update the
corresponding i18n entry so the UI communicates that verification must be
re-solved; keep the onClick handler (restartFromCaptcha) unchanged so the
single-use Turnstile token flow still restarts correctly.
| async function cleanupUnverifiedFactors(factors: { id: string, status: string }[]) { | ||
| const unverified = factors.filter(f => f.status === 'unverified') | ||
| if (unverified.length > 0) { | ||
| await Promise.all(unverified.map(f => supabase.auth.mfa.unenroll({ factorId: f.id }))) | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanupUnverifiedFactors can throw and block page initialization.
Promise.all on line 242 will reject if any single unenroll call fails, and the await on line 274 has no try/catch. This would abort onMounted, leaving the page in a permanent loading state. Cleanup of stale factors is best-effort and should not prevent the page from rendering.
Suggested fix
async function cleanupUnverifiedFactors(factors: { id: string, status: string }[]) {
const unverified = factors.filter(f => f.status === 'unverified')
if (unverified.length > 0) {
- await Promise.all(unverified.map(f => supabase.auth.mfa.unenroll({ factorId: f.id })))
+ await Promise.allSettled(unverified.map(f => supabase.auth.mfa.unenroll({ factorId: f.id })))
}
}Also applies to: 262-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/settings/account/ManageTwoFactor.vue` around lines 239 - 244, The
cleanupUnverifiedFactors function currently awaits Promise.all of
supabase.auth.mfa.unenroll calls which will reject if any call fails and can
abort onMounted; change it to a best-effort non-throwing cleanup by using
Promise.allSettled (or wrap each unenroll in try/catch) and swallow or log
individual errors instead of letting one rejection propagate, and ensure callers
(e.g., the onMounted that awaits cleanupUnverifiedFactors) do not rely on it
throwing so the page can finish initialization even if some unenrolls fail.
The cli-s3 test was failing with a duplicate key constraint violation when Vitest retried the test. This adds cleanup before inserting the version to handle test retries properly.
|
2 participants
Summary (AI generated)
?setup2fa=truequery param to the new Manage 2FA pageMotivation (AI generated)
The previous 2FA setup flow was scattered across the General account page with a confusing mix of inline email verification and popup dialogs for QR code scanning. Users had to understand a non-obvious two-phase process (verify email first, then enable 2FA). The new dedicated tab with a clear stepper makes the flow self-explanatory.
Business Impact (AI generated)
Improves 2FA adoption by making the setup process clearer and more guided. Organizations that enforce 2FA will see fewer support requests from users confused by the old flow.
Test Plan (AI generated)
?setup2fa=trueredirects to the Manage 2FA page{{ .Token }}for OTP codes to appearGenerated with AI
Summary by CodeRabbit