fix: ui-bugs-eidwallet #446
Conversation
WalkthroughRefactors onboarding control flow into explicit branches, converts pre-verification provisioning into an entropy → provision two-step with centralized try/catch/finally error handling, disables Next/Continue until required inputs are non-empty, conditions Settings drawers rendering, and adds backdrop-to-close handling to the Drawer. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Onboard as Onboarding Page
participant Keys as Key Manager
participant Verif as Verification Service
User->>Onboard: Open onboarding
Onboard->>Keys: Check hardware / initialize keys
alt checkingHardware
Onboard->>User: "Checking device capabilities..."
Keys-->>Onboard: hardware result
else showHardwareError
Onboard->>User: Show hardware unavailable
User->>Onboard: Choose pre-verification
else preVerified
Onboard->>User: Show verification input (Next disabled if empty)
User->>Onboard: Submit code
Onboard->>Verif: Fetch entropy
Onboard->>Verif: Provision request (with entropy)
Verif-->>Onboard: provision response (validate success)
Onboard->>User: Show verificationId / errors
else final
Onboard->>User: Show demo name input (Continue disabled if empty)
User->>Onboard: Submit
Onboard->>Keys: Finalize wallet provisioning
Keys-->>Onboard: success
end
sequenceDiagram
participant User
participant Drawer as Drawer (CupertinoPane)
User->>Drawer: Tap backdrop
Drawer->>Drawer: onBackdropTap handler runs
Drawer-->>User: Destroy pane, set isPaneOpen = false (pane closed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-14T17:54:47.719ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(3 hunks)
⏰ 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)/onboarding/+page.svelte
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+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/routes/(auth)/onboarding/+page.svelte (1)
232-261: Loading state is never cleared inhandleFinalSubmit.The function sets
loading = trueat line 233 but never resets it tofalse. The loading spinner will remain visible for 10 seconds minimum (until navigation), and if navigation fails, the spinner persists indefinitely with no user feedback. Additionally, if any of the operations throw (e.g., setting vault data), there's no error handling.Apply this diff to properly manage the loading state and add error handling:
handleFinalSubmit = async () => { - loading = true; - - const tenYearsLater = new Date(); - tenYearsLater.setFullYear(tenYearsLater.getFullYear() + 10); - globalState.userController.user = { - name: - demoName || - capitalize(`${falso.randFirstName()} ${falso.randLastName()}`), - "Date of Birth": new Date().toDateString(), - "ID submitted": `Passport - ${falso.randCountryCode()}`, - "Passport Number": generatePassportNumber(), - }; - globalState.userController.isFake = true; - globalState.userController.document = { - "Valid From": new Date(Date.now()).toDateString(), - "Valid Until": tenYearsLater.toDateString(), - "Verified On": new Date().toDateString(), - }; - - // Set vault in controller - this will trigger profile creation with retry logic - globalState.vaultController.vault = { - uri, - ename, - }; - - setTimeout(() => { - goto("/register"); - }, 10_000); + try { + loading = true; + + const tenYearsLater = new Date(); + tenYearsLater.setFullYear(tenYearsLater.getFullYear() + 10); + globalState.userController.user = { + name: + demoName || + capitalize(`${falso.randFirstName()} ${falso.randLastName()}`), + "Date of Birth": new Date().toDateString(), + "ID submitted": `Passport - ${falso.randCountryCode()}`, + "Passport Number": generatePassportNumber(), + }; + globalState.userController.isFake = true; + globalState.userController.document = { + "Valid From": new Date(Date.now()).toDateString(), + "Valid Until": tenYearsLater.toDateString(), + "Verified On": new Date().toDateString(), + }; + + // Set vault in controller - this will trigger profile creation with retry logic + globalState.vaultController.vault = { + uri, + ename, + }; + + setTimeout(() => { + loading = false; + goto("/register"); + }, 10_000); + } catch (err) { + console.error("Failed to finalize onboarding:", err); + error = "Failed to complete onboarding. Please try again."; + loading = false; + setTimeout(() => { + error = null; + }, 5000); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T17:54:47.711Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 437
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:138-157
Timestamp: 2025-11-14T17:54:47.711Z
Learning: The `setOnboardingPin` method in `infrastructure/eid-wallet/src/lib/global/controllers/security.ts` is intentionally designed to allow overwriting existing PINs without checking if a PIN already exists. This is a workaround for Android keeping stale files around during app reinstallation or onboarding, which causes errors on some devices. Do not suggest adding guards to prevent PIN overwrites in this method.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte
🔇 Additional comments (2)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte (2)
305-311: Button state correctly reflects hardware checking.The disabled state and dynamic text properly prevent interaction during hardware capability checks, providing clear user feedback.
368-373: Perfect implementation of the PR objective!The button is now correctly disabled when the verification code field is empty, preventing the error reported in issue #445. Both the
disabledattribute and thevariantprop provide clear visual feedback to users that the button is not actionable until they enter a code.
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+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/routes/(app)/settings/+page.svelte (1)
1-209: Major inconsistency: File doesn't match PR objective.The PR objective states this fixes issue #445, which requires disabling the "generate ename button" when the verification code field is empty on the onboarding page. However, this file is the settings page and contains no code related to verification code inputs or the generate ename button.
Either the onboarding page changes (
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.sveltementioned in the AI summary) are missing from this review, or this PR contains unrelated changes that should be in a separate PR.
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte (2)
150-177: Potential state synchronization issue with conditional rendering and bind.The pattern
{#if isDeleteConfirmationOpen}+bind:isPaneOpen={isDeleteConfirmationOpen}is redundant and may cause issues:
- When the drawer closes, it tries to set
isPaneOpentofalsevia the bind- But the
{#if}condition removes the component from the DOM when the state becomesfalse- This race condition could prevent the bind from updating properly or cause the component to be destroyed mid-transition
Either:
- Remove the
{#if}wrapper and let the Drawer component handle visibility internally (if it supports this), OR- Remove the
bind:isPaneOpenand handle closing purely through thecancelDelete/confirmDeletefunctionsExample of option 2:
-{#if isDeleteConfirmationOpen} - <Drawer bind:isPaneOpen={isDeleteConfirmationOpen}> +<Drawer isPaneOpen={isDeleteConfirmationOpen} onClose={() => isDeleteConfirmationOpen = false}>This same issue applies to the final confirmation drawer (lines 180-209).
135-145: Fragile color logic based on emoji string matching.The color determination uses
retryMessage.includes('✅')which is fragile. If the message text changes or the emoji is different, the coloring breaks.Consider using explicit state for message type:
let isRetrying = $state(false); let retryMessage = $state(""); +let retryStatus = $state<"info" | "success" | "error" | null>(null);Then update the class binding:
-class="mt-2 text-sm {isRetrying - ? 'text-blue-600' - : retryMessage.includes('✅') - ? 'text-green-600' - : 'text-red-600'}" +class="mt-2 text-sm {retryStatus === 'info' + ? 'text-blue-600' + : retryStatus === 'success' + ? 'text-green-600' + : 'text-red-600'}"And set
retryStatusappropriately inhandleVersionTap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.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 (2)
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte (2)
39-44: LGTM: Proper async state reset and navigation.The function correctly awaits the state reset, updates both the context and local state reference, then navigates to the onboarding page. The flow is clean and follows proper async patterns.
98-100: The review comment is incorrect.Your search revealed that
runtime.header.titleis referenced only within the settings folder—specifically in the settings pages (which set it) and the settings layout (which reads it). Other app routes likescan-qrandePassportpass hardcoded title values directly toAppNav, they do not read fromruntime.header.title.When navigating away from settings, the router switches layout contexts, and those pages use their own title management—there is no persistence issue. Cleanup is unnecessary.
Likely an incorrect or invalid review comment.
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/blabsy/src/components/modal/mobile-sidebar-modal.tsx (1)
86-114: Remove unused Display modal setup.Since the Display button is commented out (lines 245-256), the display modal setup and rendering are now dead code. Remove the unused
useModalhook and theDisplayModalcomponent to avoid rendering unnecessary components and improve maintainability.Apply this diff:
- const { - open: displayOpen, - openModal: displayOpenModal, - closeModal: displayCloseModal - } = useModal(); - const { open: logOutOpen, openModal: logOutOpenModal, closeModal: logOutCloseModal } = useModal(); const allStats: Readonly<Stats[]> = [ ['following', 'Following', following.length], ['followers', 'Followers', followers.length] ]; const userLink = username ? `/user/${username}` : null; return ( <> - <Modal - className='items-center justify-center xs:flex' - modalClassName='max-w-xl bg-main-background w-full p-8 rounded-2xl hover-animation' - open={displayOpen} - closeModal={displayCloseModal} - > - <DisplayModal closeModal={displayCloseModal} /> - </Modal> <Modal
🧹 Nitpick comments (1)
platforms/blabsy/src/components/modal/mobile-sidebar-modal.tsx (1)
245-256: Consider fully removing instead of commenting out.If the Display feature is being permanently removed, delete the commented code rather than leaving it in place. If it's temporarily disabled, add a TODO or FIXME comment explaining why and when it should be re-enabled.
Apply this diff if permanently removing:
- {/* <Button - className='accent-tab accent-bg-tab flex items-center gap-2 rounded-md p-1.5 font-bold transition - hover:bg-light-primary/10 focus-visible:ring-2 first:focus-visible:ring-[#878a8c] - dark:hover:bg-dark-primary/10 dark:focus-visible:ring-white' - onClick={displayOpenModal} - > - <HeroIcon - className='h-5 w-5' - iconName='PaintBrushIcon' - /> - Display - </Button> */} <ButtonOr add a comment if temporarily disabled:
+ {/* TODO: Re-enable Display feature after [reason] */} {/* <Button
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/blabsy/src/components/modal/mobile-sidebar-modal.tsx(2 hunks)
⏰ 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
grv-saini-20
force-pushed
the
fix/ui-bugs-eidwallet
branch
from
568f74d to
1e22980
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte (1)
44-49: Honordismissibleflag for backdrop-tap close and align close semanticsRight now the drawer closes on backdrop tap even when
dismissible === false, which is inconsistent with howdismissiblealready controlsbottomCloseandshowDraggable. It also uses baredestroy()while the state effect usesdestroy({ animate: true }).Consider guarding the handler with
dismissibleand aligning the destroy call:- events: { - onBackdropTap: () => { - pane?.destroy(); - isPaneOpen = false; - }, - }, + events: { + onBackdropTap: () => { + if (!dismissible) return; + pane?.destroy({ animate: true }); + isPaneOpen = false; + }, + },This keeps non-dismissible drawers truly non-dismissible via backdrop and makes the close path visually consistent with the other
destroy({ animate: true })usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte(1 hunks)
⏰ 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: lint
There was a problem hiding this comment.
Actionable comments posted: 0
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)/onboarding/+page.svelte (1)
235-264: Add error handling to prevent stuck loading state.Unlike
handleNextandhandleContinue,handleFinalSubmithas no error handling. If any operation fails (user data assignment, vault setup, or navigation),loadingremainstrueindefinitely and the user receives no feedback. This leaves the app in an inconsistent state with no recovery path.Apply this diff to add comprehensive error handling:
handleFinalSubmit = async () => { loading = true; + error = null; - const tenYearsLater = new Date(); - tenYearsLater.setFullYear(tenYearsLater.getFullYear() + 10); - globalState.userController.user = { - name: - demoName || - capitalize(`${falso.randFirstName()} ${falso.randLastName()}`), - "Date of Birth": new Date().toDateString(), - "ID submitted": `Passport - ${falso.randCountryCode()}`, - "Passport Number": generatePassportNumber(), - }; - globalState.userController.isFake = true; - globalState.userController.document = { - "Valid From": new Date(Date.now()).toDateString(), - "Valid Until": tenYearsLater.toDateString(), - "Verified On": new Date().toDateString(), - }; - - // Set vault in controller - this will trigger profile creation with retry logic - globalState.vaultController.vault = { - uri, - ename, - }; - - setTimeout(() => { - goto("/register"); - }, 10_000); + try { + const tenYearsLater = new Date(); + tenYearsLater.setFullYear(tenYearsLater.getFullYear() + 10); + globalState.userController.user = { + name: + demoName || + capitalize(`${falso.randFirstName()} ${falso.randLastName()}`), + "Date of Birth": new Date().toDateString(), + "ID submitted": `Passport - ${falso.randCountryCode()}`, + "Passport Number": generatePassportNumber(), + }; + globalState.userController.isFake = true; + globalState.userController.document = { + "Valid From": new Date(Date.now()).toDateString(), + "Valid Until": tenYearsLater.toDateString(), + "Verified On": new Date().toDateString(), + }; + + // Set vault in controller - this will trigger profile creation with retry logic + globalState.vaultController.vault = { + uri, + ename, + }; + + setTimeout(() => { + goto("/register"); + }, 10_000); + } catch (err) { + console.error("Final submission failed:", err); + error = "Failed to complete registration. Please try again."; + setTimeout(() => { + error = null; + }, 6000); + } finally { + loading = false; + } };
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte (1)
43-53: Consider using a constant test key ID to prevent keystore slot accumulation.Each call to
handleGetStartedgenerates a new timestamp-based key ID, which accumulates keys in the hardware keystore without cleanup. Since the KeyManager API doesn't provide a delete method, consider using a constant ID like"hardware-capability-test"instead ofhardware-test-${Date.now()}. This way, repeated capability checks will overwrite the same slot rather than exhausting available hardware keystore slots over time.Apply this diff to use a constant test key ID:
- const testKeyId = `hardware-test-${Date.now()}`; + const testKeyId = "hardware-capability-test"; console.log( "Testing hardware key generation with test key:", testKeyId, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T17:54:47.719Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 437
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:138-157
Timestamp: 2025-11-14T17:54:47.719Z
Learning: The `setOnboardingPin` method in `infrastructure/eid-wallet/src/lib/global/controllers/security.ts` is intentionally designed to allow overwriting existing PINs without checking if a PIN already exists. This is a workaround for Android keeping stale files around during app reinstallation or onboarding, which causes errors on some devices. Do not suggest adding guards to prevent PIN overwrites in this method.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+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/routes/(auth)/onboarding/+page.svelte (4)
192-231: Excellent error handling implementation!The try/catch/finally structure properly handles all failure scenarios in the pre-verification flow. The finally block ensures
loadingis always reset, error messages are user-friendly, and state is properly cleaned up (resettingpreVerifiedandverificationId) on failure. This addresses the previous review feedback effectively.
355-360: Button disabled states correctly implement the requirement.The dynamic
disabledandvariantproperties based on input length successfully address issue #445. Both the verification code button (lines 374-379) and demo name button (lines 355-360) are disabled when their respective fields are empty, providing clear visual feedback to users.Also applies to: 374-379
391-413: Excellent fallback UX for hardware limitations.The hardware error branch provides a clear explanation and smooth transition to the pre-verification path. Setting
isPaneOpen = falsebefore callinghandlePreVerified()creates a seamless mode switch, improving the user experience when hardware keys aren't available.
326-326: Drawer backdrop-click-to-close is properly implemented.The Drawer component at
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.sveltecorrectly implements backdrop click handling via theonBackdropTapevent handler (lines 45-48), which destroys the pane and updatesisPaneOpen = false. The backdrop is enabled with blur and configured opacity. This properly addresses issue #450 and allows thebind:isPaneOpenbinding in this file to respond to backdrop interactions.
Description of change
Issue Number
closes #445
closes #442
closes #450
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores