refactor: improve MFA challenge form logic and fix account page issues #2365
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthroughverify in src/lib/components/mfaChallengeFormList.svelte was expanded to accept a challengeType (default AuthenticationFactor.Totp) and optional factors, and verifyCurrent() was added and exported. verify now validates that a requested factor is enabled when no challenge is provided, and dynamically creates a challenge using the requested factor (not always TOTP). Input handling moved to InputDigits/InputText; errors are tracked and rethrown without auto-clearing inputs. enabledMainFactors and onMount logic were updated to skip MFA if no factors or prefer Totp when available. In src/routes/(console)/account/updateMfa.svelte, Recovery Regenerate/View buttons are disabled when no other methods are enabled. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/account/updateMfa.svelte (1)
81-90: Fix inverted toast copy when toggling MFA.The notification uses the current value instead of the target state, so it can announce the opposite action.
Apply:
async function updateMfa() { try { - await sdk.forConsole.account.updateMFA({ mfa: !$user.mfa }); + const nextMfa = !$user.mfa; + await sdk.forConsole.account.updateMFA({ mfa: nextMfa }); await invalidate(Dependencies.ACCOUNT); addNotification({ - message: `Multi-factor authentication has been ${$user.mfa ? 'enabled' : 'disabled'}`, + message: `Multi-factor authentication has been ${nextMfa ? 'enabled' : 'disabled'}`, type: 'success' }); - trackEvent(Submit.AccountUpdateMfa, { mfa: !$user.mfa }); + trackEvent(Submit.AccountUpdateMfa, { mfa: nextMfa });
🧹 Nitpick comments (5)
src/routes/(console)/account/updateMfa.svelte (2)
260-264: Show “Recovery” only when a primary method exists.
enabledMethods.length >= 0is always true. Hiding the section when no main methods are enabled simplifies the UI and avoids dangling actions.-{#if enabledMethods.length >= 0} +{#if enabledMethods.length > 0}
175-183: Block deleting the last primary MFA method.To avoid “recovery-only” accounts (and potential login dead-ends), disable Delete when TOTP is the sole primary method.
If policy allows at least one primary factor at all times, apply:
-<Button - text - class="is-not-mobile" - on:click={() => (showDelete = true)}>Delete</Button> +<Button + text + class="is-not-mobile" + disabled={enabledMethods.length === 1} + on:click={() => (showDelete = true)}>Delete</Button>src/lib/components/mfaChallengeFormList.svelte (3)
15-19: Align challenge creation in verify() with createChallenge logic.
createChallenge()intentionally skips creating challenges for TOTP and Recovery Code, butverify()will still create one ifchallengeis falsy. Make creation conditional on factors that require it (Email/Phone) to avoid redundant server calls and inconsistencies.- if (!challenge) { + if ( + !challenge && + challengeType !== AuthenticationFactor.Totp && + challengeType !== AuthenticationFactor.Recoverycode + ) { challenge = await sdk.forConsole.account.createMFAChallenge({ factor: challengeType }); }Also applies to: 62-67
26-28: Improve analytics: include factor used.Capturing which factor was used helps analyze adoption and issues.
- trackEvent(Submit.AccountLogin, { mfa_used: true }); + trackEvent(Submit.AccountLogin, { mfa_used: true, factor: challengeType });
50-56: Minor: consistent naming for “primary/main” factors.Variable is named
enabledMainFactorswhile UI copy refers to “primary methods.” Consider harmonizing naming for maintainability.Also applies to: 129-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/mfaChallengeFormList.svelte(3 hunks)src/routes/(console)/account/updateMfa.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
PR: appwrite/console#2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/lib/components/mfaChallengeFormList.svelte
🔇 Additional comments (2)
src/routes/(console)/account/updateMfa.svelte (1)
283-286: Good guard: recovery actions disabled when no methods are enabled.The new disabled state prevents generating or viewing recovery codes without at least one primary factor. Solid UX safety net.
Also applies to: 289-293
src/lib/components/mfaChallengeFormList.svelte (1)
86-99: Edge case: “recovery-only” accounts bypass the form.When no primary factors are enabled but recovery codes exist, the template skips MFA entirely (
enabledMainFactors.length === 0). Confirm product policy; if recovery-only sign-in should be supported, default to Recovery Code instead of skipping.Proposed handling:
onMount(async () => { - if (enabledMainFactors.length === 0) { - return; - } + if (enabledMainFactors.length === 0) { + if (factors.recoveryCode) { + challengeType = AuthenticationFactor.Recoverycode; + } + return; + }-{#if enabledMainFactors.length === 0} - <!-- Skip MFA completely if no factors enabled --> +{#if enabledMainFactors.length === 0 && !factors.recoveryCode} + <!-- Skip MFA completely if no primary factors or recovery code -->-{#if showVerifyButton && enabledMainFactors.length > 0} +{#if showVerifyButton && (enabledMainFactors.length > 0 || challengeType === AuthenticationFactor.Recoverycode)}Also applies to: 101-107, 125-127
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MFA (Multi-Factor Authentication) challenge form logic to provide better UX and fixes issues in the account page. The changes improve factor selection, add smart initialization, and prevent potential user lockout scenarios.
- Enhanced MFA challenge form with smarter factor initialization and cleaner UI
- Added validation to prevent users from disabling all MFA methods when recovery codes exist
- Streamlined factor switching logic with improved messaging and automatic challenge creation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/routes/(console)/account/updateMfa.svelte | Disables recovery code actions when no other MFA methods are enabled to prevent lockout |
| src/lib/components/mfaChallengeFormList.svelte | Major refactor of MFA challenge logic with improved initialization, factor switching, and UI flow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <script context="module" lang="ts"> | ||
| let inputDigitFields: InputDigits; | ||
| import { sdk } from '$lib/stores/sdk'; | ||
| import { invalidate } from '$app/navigation'; | ||
| import { Dependencies } from '$lib/constants'; | ||
| import { Submit, trackEvent, trackError } from '$lib/actions/analytics'; | ||
| import { AuthenticationFactor, type Models } from '@appwrite.io/console'; |
There was a problem hiding this comment.
Module-level imports should be moved to the instance-level script block. The context='module' script should only contain code that needs to be shared across component instances.
Copilot uses AI. Check for mistakes.
| function switchToFactor(factor: AuthenticationFactor) { | ||
| createChallenge(factor); | ||
| } |
There was a problem hiding this comment.
The switchToFactor function is a simple wrapper around createChallenge. Consider removing this wrapper and calling createChallenge directly for better code clarity.
Copilot uses AI. Check for mistakes.
| await invalidate(Dependencies.ACCOUNT); | ||
| trackEvent(Submit.AccountLogin, { mfa_used: true }); | ||
| } catch (error) { | ||
| inputDigitFields?.clearInputsAndRefocus(); | ||
| trackError(error, Submit.AccountLogin); |
There was a problem hiding this comment.
The inputDigitFields?.clearInputsAndRefocus() call was removed from the error handling, but there's no corresponding functionality to clear the input fields on error. This could leave users with invalid codes still displayed.
| trackError(error, Submit.AccountLogin); | |
| trackError(error, Submit.AccountLogin); | |
| code = ''; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/components/mfaChallengeFormList.svelte (1)
2-6: Fix: MoveInputDigitsimport to the instance script.The
InputDigitscomponent is imported in the module script (line 7 is missing), but it's used in the template. Components used in templates must be imported in the instance script to be in scope.Move the import from the module script to the instance script:
<script context="module" lang="ts"> import { sdk } from '$lib/stores/sdk'; import { invalidate } from '$app/navigation'; import { Dependencies } from '$lib/constants'; import { Submit, trackEvent, trackError } from '$lib/actions/analytics'; import { AuthenticationFactor, type Models } from '@appwrite.io/console'; - import { InputDigits } from '$lib/elements/forms'; </script> <script lang="ts"> import { onMount } from 'svelte'; - import { Button, InputText } from '$lib/elements/forms'; + import { Button, InputText, InputDigits } from '$lib/elements/forms';Also applies to: 36-36
🧹 Nitpick comments (3)
src/lib/components/mfaChallengeFormList.svelte (3)
50-54: Improve type safety forenabledMainFactors.The current implementation creates an array with potential
falsevalues that are then filtered out. This can be made more type-safe and cleaner.-// Get enabled non-recovery factors for main factors -$: enabledMainFactors = [ - factors.totp && 'totp', - factors.email && 'email', - factors.phone && 'phone' -].filter(Boolean); +// Get enabled non-recovery factors for main factors +$: enabledMainFactors = (() => { + const factors_list: string[] = []; + if (factors.totp) factors_list.push('totp'); + if (factors.email) factors_list.push('email'); + if (factors.phone) factors_list.push('phone'); + return factors_list; +})();
77-79: Consider renamingswitchToFactorfor clarity.The function name
switchToFactordoesn't clearly indicate that it creates a new challenge. Consider a more descriptive name.-function switchToFactor(factor: AuthenticationFactor) { +function switchToFactorAndCreateChallenge(factor: AuthenticationFactor) { createChallenge(factor); }Also update all references to this function in lines 143, 154, 165, and 176.
128-182: Simplify thehasSecondaryOptionscomputation.The computed variable
hasSecondaryOptionshas redundant logic that can be simplified for better readability.-{@const hasSecondaryOptions = - enabledMainFactors.length > 1 || - (factors.recoveryCode && challengeType !== AuthenticationFactor.Recoverycode) || - (enabledMainFactors.length === 1 && factors.recoveryCode)} +{@const hasSecondaryOptions = + enabledMainFactors.length > 1 || + (factors.recoveryCode && challengeType !== AuthenticationFactor.Recoverycode)}The condition
(enabledMainFactors.length === 1 && factors.recoveryCode)is already covered by the second condition when there's at least one enabled factor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/mfaChallengeFormList.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
PR: appwrite/console#2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/lib/components/mfaChallengeFormList.svelte
🔇 Additional comments (3)
src/lib/components/mfaChallengeFormList.svelte (3)
100-122: LGTM! Well-structured conditional rendering.The template properly handles all MFA factor types with appropriate user instructions and input components. The use of
InputDigitsfor code-based factors andInputTextfor recovery codes is correct.
8-31: Verify public API change impact on consumers.
File: src/lib/components/mfaChallengeFormList.svelte (lines 8–31). The verify(...) signature now adds an optional challengeType parameter — confirm no internal or external callers break because of this change. Repo search failed with rg (unrecognized file type: svelte); run locally to find callers:rg -n -S 'from.*mfaChallengeFormList' || true rg -n -S 'verify\s*\(' || true git grep -n 'mfaChallengeFormList' || true
85-97: Don't auto-create email/phone challenges on mountonMount calls createChallenge(AuthenticationFactor.Email|Phone) which may send email/SMS immediately — confirm createChallenge's implementation and, if it triggers outgoing messages, defer challenge creation to an explicit user action (button/confirm) or add a visible opt‑in/guard.
Location: src/lib/components/mfaChallengeFormList.svelte (lines 85–97).
HarshMN2345
force-pushed
the
fix-mfa-flow
branch
from
afc1512 to
aeb1e1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/components/mfaChallengeFormList.svelte (4)
63-68: Type the reactive list as factors, not strings.Avoid magic strings; store
AuthenticationFactor[]for stronger invariants.- $: enabledMainFactors = [ - factors.totp && 'totp', - factors.email && 'email', - factors.phone && 'phone' - ].filter(Boolean); + $: enabledMainFactors = ([ + factors.totp && AuthenticationFactor.Totp, + factors.email && AuthenticationFactor.Email, + factors.phone && AuthenticationFactor.Phone + ].filter(Boolean)) as AuthenticationFactor[];
82-86: Harden notification error handling.
errormay not always have.message. Fallback toString(error)to avoid blank toasts.- addNotification({ - type: 'error', - message: error.message - }); + addNotification({ + type: 'error', + message: (error as { message?: string })?.message ?? String(error) + });
128-129: Improve auto-fill UX for OTP inputs.Enable OS/browser OTP auto-fill.
- <InputDigits bind:value={code} required autofocus /> + <InputDigits bind:value={code} required autofocus autocomplete="one-time-code" inputmode="numeric" />(Apply to TOTP, Email, and Phone sections.)
Also applies to: 133-134, 138-139
35-35: Enrich analytics with factor dimension.Capture which factor was used.
- trackEvent(Submit.AccountLogin, { mfa_used: true }); + trackEvent(Submit.AccountLogin, { mfa_used: true, mfa_factor: challengeType });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/mfaChallengeFormList.svelte(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
PR: appwrite/console#2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/lib/components/mfaChallengeFormList.svelte
🪛 GitHub Actions: Tests
src/lib/components/mfaChallengeFormList.svelte
[error] 1-1: Prettier formatting check failed (command: 'prettier --check .'). Run 'prettier --write' to fix code style issues.
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (2)
src/lib/components/mfaChallengeFormList.svelte (2)
1-200: Fix Prettier violations and commit formatted files.Ran
pnpm -s prettier --write .(Prettier v3.6.2). Reformatted: src/lib/components/mfaChallengeFormList.svelte — commit and push these changes to unblock CI.
92-100: Exported instance function is a valid Svelte pattern — no change required.Svelte allows exporting functions from the instance <script> to expose component methods on the component instance (callable via bind:this). (v4.svelte.dev)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/mfaChallengeFormList.svelte (1)
1-41: Fix: module-scoped verify() is inaccessible to the instance script and references undefined identifiers
- The instance script calls verify(...) (line 93) but exports from <script context="module"> are not visible to the component instance — this will fail at compile time.
- The module script references sdk, invalidate, Dependencies, Submit, trackEvent, trackError, AuthenticationFactor, and Models without importing them.
Action: move verify() to a shared helper (e.g., src/lib/mfa/verify.ts) and import it into the instance script, or add the missing imports to the module script and do not call the module export from the instance script.
File: src/lib/components/mfaChallengeFormList.svelte — lines 1–41 (also applies to lines 91–99)
♻️ Duplicate comments (1)
src/lib/components/mfaChallengeFormList.svelte (1)
50-50: LGTM: moved InputDigits to instance scriptThis fixes component scope and compilation.
🧹 Nitpick comments (2)
src/lib/components/mfaChallengeFormList.svelte (2)
58-61: Initialize props to safe defaults and reflect nullable challengeMake challenge nullable and initialize code to an empty string to avoid undefined access and ensure bindings have a value.
-export let challenge: Models.MfaChallenge; -export let code: string; +export let challenge: Models.MfaChallenge | null = null; +export let code: string = '';
91-99: Good UX: clear code on verification failureResetting code on error prevents stale/invalid values from lingering. Consider also re-focusing the input after clearing for faster retry.
export async function verifyCurrent() { try { return await verify(challenge, code, challengeType, factors); } catch (error) { // Clear input fields on verification failure code = ''; + // Optionally re-focus the active input + // await tick(); inputRef?.focus(); throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/mfaChallengeFormList.svelte(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
PR: appwrite/console#2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/lib/components/mfaChallengeFormList.svelte
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (1)
src/lib/components/mfaChallengeFormList.svelte (1)
63-68: LGTM: enabledMainFactors computationCompact and reactive; avoids truthiness pitfalls by filtering falsy values.
| async function createChallenge(factor: AuthenticationFactor) { | ||
| disabled = true; | ||
| challengeType = factor; | ||
| code = ''; | ||
| try { | ||
| challenge = await sdk.forConsole.account.createMFAChallenge({ factor }); | ||
| if ( | ||
| factor !== AuthenticationFactor.Totp && | ||
| factor !== AuthenticationFactor.Recoverycode | ||
| ) { | ||
| challenge = await sdk.forConsole.account.createMFAChallenge({ factor }); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align challenge creation semantics with verify()
createChallenge() avoids creating a challenge for TOTP and RecoveryCode, while verify() (in the original module script) always creates one when challenge is missing. Confirm API expectations and make both paths consistent (either gate by factor or always create). I can update both places once you confirm the API contract.
🌐 Web query:
Does Appwrite’s Account.createMFAChallenge require a challenge for TOTP and RecoveryCode factors, or only for Email/Phone?
💡 Result:
You must create a challenge for all supported factors (email, phone, totp and recoveryCode). Appwrite’s createMFAChallenge accepts totp and recoveryCode as valid factors (not just email/phone) and returns a challenge ID to complete with updateMfaChallenge. [1][2]
Always create an MFA challenge for all factors (include TOTP & RecoveryCode)
Appwrite requires a challenge for all supported factors (email, phone, totp, recoveryCode). In src/lib/components/mfaChallengeFormList.svelte (lines 70–81) remove the guard that skips createMFAChallenge for AuthenticationFactor.Totp and AuthenticationFactor.Recoverycode so createChallenge always calls sdk.forConsole.account.createMFAChallenge({ factor }); align verify() and createChallenge behavior.
🤖 Prompt for AI Agents
In src/lib/components/mfaChallengeFormList.svelte around lines 70 to 81, the
createChallenge function currently skips calling
sdk.forConsole.account.createMFAChallenge for AuthenticationFactor.Totp and
AuthenticationFactor.Recoverycode; remove that conditional guard so
createChallenge always calls sdk.forConsole.account.createMFAChallenge({ factor
}); (preserve existing disabled/challengeType/code handling and try/catch
behavior) to align createChallenge with verify() and ensure an MFA challenge is
created for all factor types.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores