Fix/eid app onboarding UX issues #419
Conversation
WalkthroughRegistration, login, PIN input, and settings PIN pages updated: dynamic confirm-button variant and stricter repeat-PIN validation in registration; login clearPin behavior simplified; InputPin auto-resets when cleared; settings PIN update parameter order and navigation changed; package.json newline normalized. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Input as InputPin
participant Reg as Register Page
participant Sec as SecurityController
participant Nav as Router
rect rgba(200,230,255,0.25)
Note over U,Reg: Registration flow (btnVariant controls Confirm)
U->>Input: enter digits
Input->>Reg: update pin
Reg->>Reg: when pin length == 4 -> firstStep = false, btnVariant = "solid"
U->>Reg: enter repeatPin
Reg->>Reg: effect sets btnVariant="solid" only if repeatPin length==4 && match
U->>Reg: click Confirm
Reg->>Reg: validate repeat length and match
alt match
Reg->>Sec: updatePin(pin, repeatPin, ...)
Reg->>Reg: open Drawer / biometric setup
Reg->>Nav: navigate on completion
else mismatch
Reg->>U: show error, reset to firstStep
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
107-112: Incorrect error message for first PIN entry.The error message "Your PIN does not match, try again." on line 111 doesn't make sense on the first step where the user hasn't entered a confirmation PIN yet. The error condition checks
isError && pin.length === 0, which could occur if the user returns from the confirmation step with an error.Consider separate error states for each step, or adjust the error message to be context-appropriate:
- Your PIN does not match, try again. + Please enter a 4-digit PIN.Or better yet, handle error states separately for each step so the confirmation step error doesn't leak back to the first step.
🧹 Nitpick comments (4)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (4)
23-27: Avoid comma operator for side effects.The comma operator on line 25 reduces readability. Use separate statements or a block.
Apply this diff:
const handleFirstStep = async () => { if (pin.length === 4) { - (firstStep = false), (btnVariant = "solid"); + firstStep = false; + btnVariant = "solid"; } };
48-51: Consider bidirectional error state management.The effect clears the error when PINs match, but doesn't set it back to
trueif the user subsequently modifies one PIN to break the match. This may be intentional for UX, but consider whether you want real-time validation feedback.
57-58: Remove debug console.log.The console.log on line 58 should be removed before production deployment.
Apply this diff:
isBiometricsAvailable = (await checkStatus()).isAvailable; - console.log("isBiometricsAvailable", isBiometricsAvailable);
31-46: Remove redundant placeholder comments.The comments on lines 32, 37, and 44 suggest placeholder code, but the actual logic is already implemented. These comments should be removed.
Apply this diff:
const handleNext = async () => { - //handle next logic goes here isBiometricScreen = true; }; const handleSkip = async () => { - // handle skip biometrics logic goes here goto("/review"); }; let handleSetupBiometrics = $state(async () => {}); const handleBiometricsAdded = async () => { - //handle logic when biometrics added successfully goto("/review"); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
⏰ 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: build
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
Outdated
Show resolved
Hide resolved
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)
infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte (1)
132-149: Fix duplicate IDs to restorefocusOnMount.Every
<input>gets the sameid={uniqueId}, butonMountstill looks fordocument.getElementById("pin0"). The result is an invalid DOM (duplicate IDs) and the initial focus never lands on the first field, sofocusOnMountno longer works. Please give each slot a deterministic ID (e.g.pin${i}or${uniqueId}-${i}) and update the focus logic to match.- id={uniqueId} + id={`pin${i}`} ... - document.getElementById("pin0")?.focus(); + document.getElementById("pin0")?.focus();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/control-panel/package.json(1 hunks)infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/control-panel/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/eid-wallet/src/routes/(auth)/register/+page.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: lint
- GitHub Check: build
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)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
127-132: Confusing error condition related to the firstStep bug.The error display condition
isError && pin.length === 0(lines 127 and 129) seems problematic:
- When a PIN mismatch occurs,
isErroris set totrueandfirstStepis reset totrue(line 83)- The user returns to the first step, but their
pinis not cleared, sopin.lengthis still 4- The error message won't display because
pin.length === 0is falseThis creates a confusing state where the user is sent back to the first step due to an error, but no error message is shown.
This issue will be resolved when line 83's
firstStep = trueis removed (as flagged in the previous comment). The user will stay on the second step where the error message is properly displayed.
♻️ Duplicate comments (3)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (3)
36-39: Fix typo in comment.Line 37: "biometics" should be "biometrics".
Apply this diff:
const handleSkip = async () => { - // handle skip biometics logic goes here + // handle skip biometrics logic goes here goto("/review"); };
75-95: Critical UX issue: Don't reset to first step on PIN mismatch (past issue still present).Line 83 sets
firstStep = truewhen PINs don't match, forcing the user to re-enter the initial PIN from scratch. This is frustrating UX—the user should remain on the confirmation screen and only re-enter therepeatPin.Apply this diff:
if (pin !== repeatPin) { isError = true; - firstStep = true; + repeatPin = ""; return; }Note: The error handling with try-catch (lines 88-94) correctly addresses a previous review concern.
97-112: Critical: Logic bug sets biometrics flag when unavailable (past issue still present).Line 111 sets
isBiometricsAdded = trueoutside theif (isBiometricsAvailable)block. This means even when biometrics are unavailable or setup fails, the flag is set totrue, causing the UI on line 208-212 to incorrectly display "You're all set! Your biometrics have been successfully added."Apply this diff:
handleSetupBiometrics = async () => { if (!globalState) throw new Error( "Cannot set biometric support, Global state is not defined", ); if (isBiometricsAvailable) { try { globalState.securityController.biometricSupport = true; + isBiometricsAdded = true; } catch (error) { console.error("Failed to enable biometric support:", error); // Consider showing an error message to the user return; } } - isBiometricsAdded = true; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/control-panel/package.json(1 hunks)infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/control-panel/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T05:54:12.858Z
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
🔇 Additional comments (9)
infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte (3)
1-10: LGTM! Good use of constants for keyboard keys.The imports are appropriate and the
KEYBOARDconstant is a good practice for avoiding magic strings in keyboard event handling.
46-50: LGTM! PIN reset effect supports the fix for issue #399.This effect correctly resets all PIN input fields when the PIN is cleared externally (e.g., via the "Clear PIN" button). The effect chain is safe from infinite loops since the condition guards against repeated triggers.
64-104: LGTM! Comprehensive keyboard navigation implementation.The keyboard handling correctly implements:
- Backspace/delete behavior (clear current, then move back)
- Auto-advance on numeric input
- Arrow key navigation between fields
All actions properly use
preventDefault()and focus management is sound.infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (3)
35-48: LGTM! Good defensive initialization.The globalState validation with early return and error logging is a good practice. The pending deep link detection provides useful context for the user.
99-103: Verify effect placement and timing concerns.Defining a
$effectinsideonMountis unconventional. The comment suggests a timing dependency with biometric initialization, but this approach may cause unexpected behavior:
- The effect won't exist until after mount completes
- If PIN changes during mount (before effect is registered), those changes won't trigger the handler
- The dependency on initialization order suggests fragile coupling
Consider:
- Moving the effect outside
onMountif it should always be active- Adding explicit guards in
handlePinInputto handle the biometric initialization state- Documenting why this specific ordering is required
Can you verify this implementation doesn't cause race conditions during initialization?
105-148: LGTM! Biometric authentication flow is well-structured.The biometric check correctly verifies support and availability before attempting authentication. Error handling is present, and the fallback to PIN entry works as expected. The duplicated deep link handling was already flagged in a previous comment.
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (3)
23-27: LGTM! Button variant state is now properly used.The
btnVariantstate is correctly integrated into the template (lines 136 and 155), resolving the previous issue about unused state.
48-66: LGTM! Button state management correctly fixes issue #399.These effects properly implement the requirement from issue #399:
- The "Confirm" button starts in the "soft" (disabled) state
- It becomes "solid" (enabled) only when a valid 4-digit PIN is entered
- On the re-enter screen, the button is enabled only when the PINs match
This ensures the button is disabled on initial navigation and while no valid data is entered.
154-158: LGTM! Confirmation button properly uses variant state.The confirm button on the re-enter step correctly uses
btnVariantto control its enabled state, ensuring it's only enabled when the PINs match. This implements the fix for issue #399.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (3)
37-37: Fix typo in comment."biometics" should be "biometrics".
Apply this diff:
- // handle skip biometics logic goes here + // handle skip biometrics logic goes here
97-112: Critical: Logic bug sets biometrics flag when unavailable.Line 111 sets
isBiometricsAdded = trueoutside theif (isBiometricsAvailable)block. If biometrics are unavailable, the flag is still set totrue, causing the UI on line 208 to incorrectly display "You're all set! Your biometrics have been successfully added."Apply this diff:
handleSetupBiometrics = async () => { if (!globalState) throw new Error( "Cannot set biometric support, Global state is not defined", ); if (isBiometricsAvailable) { try { globalState.securityController.biometricSupport = true; + isBiometricsAdded = true; } catch (error) { console.error("Failed to enable biometric support:", error); // Consider showing an error message to the user return; } } - isBiometricsAdded = true; };
81-85: Don't reset to first step on PIN mismatch.When the confirmation PIN doesn't match, line 83 sets
firstStep = true, sending the user back to the initial PIN entry screen. The user should remain on the confirmation screen and re-enter only therepeatPin.Apply this diff:
if (pin !== repeatPin) { isError = true; - firstStep = true; + repeatPin = ""; return; }
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
23-27: Simplify: Remove redundant assignment and comma operator.The comma operator on line 25 is valid but reduces readability. Additionally, setting
btnVariant = "solid"is redundant since the$effectblock on lines 53-66 already managesbtnVariantreactively.Apply this diff:
const handleFirstStep = async () => { if (pin.length === 4) { - (firstStep = false), (btnVariant = "solid"); + firstStep = false; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T05:54:12.858Z
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/register/+page.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). (1)
- GitHub Check: build
🔇 Additional comments (3)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (2)
19-19: Good: Previously unused state is now wired.The
btnVariantstate is now properly used in the template on lines 136 and 155, addressing the previous review feedback.
88-94: Good: Error handling added for PIN update.The try-catch block properly handles failures from
updatePin, prevents the success drawer from showing on error, and logs the error for debugging. This addresses previous review feedback.infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte (1)
46-50: No reactive loop issue found—code design is intentional.After examining the InputPin component and parent usages:
The first effect (
pin = calcPin(pins)) and second effect (resetpinswhenpin === "") don't create an infinite loop. Svelte 5's $effect dependency system only triggers effects when dependencies change. When the second effect resetspins, the first effect runs and recalculatespinas""(since allpins[key]are now""), but this doesn't retrigger the second effect becausepinis already"".The reset logic is intentionally used by parent components—the login page explicitly sets
pin = ""to clear the input, which cascades to reset individual pin slots. This is the expected behavior.Initial state handling is correct: the component initializes via
onMount, and the boundpinvalue defaults to"".The implementation is sound.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (4)
37-39: Fix typo in comment."biometics" should be "biometrics".
Apply this diff:
const handleSkip = async () => { - // handle skip biometics logic goes here + // handle skip biometrics logic goes here goto("/review"); };
98-113: Critical: Biometrics flag incorrectly set when unavailable.Line 112 sets
isBiometricsAdded = trueunconditionally, even whenisBiometricsAvailableis false. This causes the UI to incorrectly display "You're all set! Your biometrics have been successfully added" when biometrics were never enabled.Apply this diff:
handleSetupBiometrics = async () => { if (!globalState) throw new Error( "Cannot set biometric support, Global state is not defined", ); if (isBiometricsAvailable) { try { globalState.securityController.biometricSupport = true; + isBiometricsAdded = true; } catch (error) { console.error("Failed to enable biometric support:", error); // Consider showing an error message to the user return; } } - isBiometricsAdded = true; };
82-86: Major: Don't reset to first step on PIN mismatch.Setting
firstStep = trueon line 84 forces the user to re-enter both PINs from scratch when confirmation fails. Standard UX keeps the user on the confirmation screen to re-enter onlyrepeatPin.Apply this diff:
if (pin !== repeatPin) { isError = true; - firstStep = true; + repeatPin = ""; return; }
135-141: Major: Buttons must be disabled, not just styled differently.PR objective #399 requires: "Ensure the 'Confirm' button is disabled when navigating to the 'Re-enter your PIN' screen."
The current implementation only changes visual appearance via
variant(soft/solid) but doesn't actually disable the buttons. Users can still click the "soft" variant button, triggering validation errors. TheButtonActioncomponent supports adisabledprop (line 223).Apply this diff to properly disable the buttons:
<ButtonAction class="w-full" variant={btnVariant} + disabled={pin.length !== 4} callback={handleFirstStep} > Confirm </ButtonAction>And for the second step:
<ButtonAction variant={btnVariant} class="w-full" + disabled={!(repeatPin.length === 4 && pin === repeatPin)} callback={handleConfirm}>Confirm</ButtonAction >Also applies to: 155-159
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (2)
23-28: Redundant btnVariant assignment.Setting
btnVariant = "solid"on line 26 duplicates the logic in the$effect(lines 54-67), which already managesbtnVariantreactively based onfirstStepandpin.length. Remove this redundant assignment to avoid multiple sources of truth.Apply this diff:
const handleFirstStep = async () => { if (pin.length === 4) { firstStep = false; - btnVariant = "solid"; } };
54-67: Approve reactive variant management, but note duplication.The
$effectcorrectly synchronizesbtnVariantwith PIN state. However, line 62 (isError = false) duplicates the logic in the earlier$effect(lines 49-52). Consider consolidating into a single reactive statement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte(3 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T05:54:12.858Z
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/register/+page.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: lint
- GitHub Check: build
🔇 Additional comments (4)
infrastructure/eid-wallet/src/lib/ui/InputPin/InputPin.svelte (1)
46-52: LGTM! Auto-reset enhances UX.The reactive effect correctly clears all PIN entries when the computed PIN becomes empty, supporting seamless reset flows across registration, login, and settings screens.
infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte (2)
17-21: LGTM! Navigation improvement.Adding
goto("/settings")appropriately returns the user to the settings page after closing the drawer.
35-39: No action required—signature supports both call patterns correctly.The
updatePinmethod signature at line 116 ofsecurity.tsdefinesoldPinas an optional parameter:async updatePin(newPin: string, confirmPin: string, oldPin?: string). This design correctly supports both use cases: registration without an existing PIN (2 parameters) and settings with PIN update (3 parameters). The implementation validates the old PIN only when needed, making the inconsistency in call patterns expected and correct.infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
88-95: LGTM! Error handling added.The try-catch block properly handles
updatePinfailures, preventing the success drawer from displaying on errors.
Description of change
Fixed all the issues from 399 to 402
Issue Number
Closes #399, #400, #401, #402
Type of change
How the change has been tested
Manual
Change checklist
Summary by CodeRabbit
New Features
Improvements