chore: fix modal UX #428
chore: fix modal UX #428
Conversation
WalkthroughAdds observable error and loading stores (authError, signingError, authLoading) to the QR scan auth/sign flows, wires them into drawers, and adjusts drawer open/decline control flow to close drawers, conditionally restart scanning, or navigate to /main on errors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as Scan QR Page
participant Logic as Scan Logic
participant Drawer as Drawer (Auth/Signing/Reveal)
participant Router
User->>Page: Scan QR / trigger auth or sign
Page->>Logic: handleAuth() / handleSigning()
rect rgb(240,248,255)
Logic->>Logic: Clear previous errors\nSet authLoading = true (for auth)
end
alt Success
Logic->>Drawer: Close/transition drawers as sequence dictates
Logic->>Logic: Clear error stores\nSet authLoading = false
Drawer->>User: Show success UI
else Error
Logic->>Logic: Set authError or signingError\nSet authLoading = false
Logic->>Drawer: Open drawer with error props
Drawer->>User: Show error banner + "Okay"
User->>Page: Click "Okay"
Page->>Router: navigate to /main
end
alt Decline with no error
User->>Page: Click "Decline"
Page->>Drawer: Close drawer
Page->>Page: restart scanning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 0
🧹 Nitpick comments (3)
platforms/group-charter-manager-api/src/controllers/AuthController.ts (1)
104-108: Good UX improvement with correct status code.The 404 status code is now semantically correct for user-not-found errors, and the
detailsfield provides helpful guidance to users—a nice UX improvement aligned with the modal UX fixes mentioned in the PR.However, the error detection via
error.message.includes("not found")is fragile and relies on exact error message format from UserService. Consider using a custom error class or error code instead for more robust detection:// Example alternative: define a custom error type class UserNotFoundError extends Error { constructor(message: string) { super(message); this.name = "UserNotFoundError"; } } // Then catch it by type instead of message matching if (error instanceof UserNotFoundError) { return res.status(404).json({...}); }This approach is less brittle if UserService error messages change in the future.
infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/SigningDrawer.svelte (1)
255-279: Consider extracting the error banner into a reusable component.The error banner markup (lines 255-279) is duplicated from the blind voting section (lines 146-174). This duplication increases maintenance burden—any styling or structural changes must be applied in multiple places.
Consider creating a shared
ErrorBanner.sveltecomponent:<!-- ErrorBanner.svelte --> <script lang="ts"> export let message: string; </script> <div class="bg-red-50 border border-red-200 rounded-lg p-4 mt-4"> <div class="flex items-center"> <div class="flex-shrink-0"> <svg class="h-5 w-5 text-red-400" viewBox="0 0 20 20" fill="currentColor"> <path fill-rule="evenodd" d="M10 18a8 8 0 100-16 8 8 0 000 16zM8.707 7.293a1 1 0 00-1.414 1.414L8.586 10l-1.293 1.293a1 1 0 101.414 1.414L10 11.414l1.293 1.293a1 1 0 001.414-1.414L11.414 10l1.293-1.293a1 1 0 00-1.414-1.414L10 8.586 8.707 7.293z" clip-rule="evenodd" /> </svg> </div> <div class="ml-3"> <h3 class="text-sm font-medium text-red-800">Error</h3> <div class="mt-2 text-sm text-red-700">{message}</div> </div> </div> </div>Then use it in both places:
-{#if signingError} - <div class="bg-red-50 border border-red-200 rounded-lg p-4 mt-4"> - ... - </div> -{/if} +{#if signingError} + <ErrorBanner message={signingError} /> +{/if}infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
308-310: Consider consolidating drawer close operations.Lines 308-310, 319-321, and 365-367 all close the auth drawer before opening the logged-in drawer. This redundancy suggests that drawer state management might benefit from a helper function.
Consider creating a helper to ensure atomic drawer transitions:
function transitionDrawer(close: () => void, open: () => void) { close(); open(); } // Usage: transitionDrawer( () => codeScannedDrawerOpen.set(false), () => loggedInDrawerOpen.set(true) );This would make the intent clearer and reduce the chance of forgetting to close one drawer when opening another.
Also applies to: 319-321, 365-367
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte(5 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/AuthDrawer.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/SigningDrawer.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(18 hunks)platforms/group-charter-manager-api/src/controllers/AuthController.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
platforms/pictique/src/lib/stores/posts.ts (1)
error(7-7)
⏰ 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 (12)
platforms/group-charter-manager-api/src/controllers/AuthController.ts (2)
78-81: Formatting looks good.The version mismatch error response is now more concise and readable. Status code and content are appropriate for this validation failure.
100-100: Also applies to: 111-111infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/SigningDrawer.svelte (1)
284-313: LGTM! Error-aware button flow is well-structured.The conditional rendering provides clear user guidance: when an error occurs, a single "Okay" button allows dismissal; otherwise, standard "Decline"/"Sign" actions are available. The loading state is also properly reflected in the button label.
infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/AuthDrawer.svelte (1)
69-93: LGTM! Error and loading state handling is well-implemented.The component correctly:
- Displays an error banner when
authErroris set- Simplifies the action to a single "Okay" button on error
- Disables both buttons during authentication
- Shows appropriate loading feedback ("Authenticating...")
This provides clear visual feedback and prevents user actions during async operations.
Note: The error banner markup is also duplicated here. Consider the shared
ErrorBannercomponent suggestion from the SigningDrawer review.Also applies to: 96-125
infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte (2)
101-111: Verify that navigating to/mainon error is the desired UX.Both decline handlers now navigate to
/mainwhen an error exists, otherwise they restart scanning. This seems reasonable for unrecoverable errors, but consider whether users might want to:
- Retry the same operation immediately
- See more details about what went wrong
- Have a "Try Again" button instead of navigation
The current flow is:
- Error occurs → user sees error message in drawer
- User clicks "Okay" → navigates to
/main- User must navigate back to scan-qr and start over
Consider whether a "Try Again" option that restarts scanning without navigation might improve UX for transient errors (like network issues).
Also applies to: 128-138
40-42: LGTM! Error and loading states properly wired through the component tree.The new stores are correctly destructured and passed as props to the respective drawer components, ensuring error and loading states are consistently reflected in the UI.
Also applies to: 194-195, 218-218
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (6)
236-238: LGTM! Comprehensive error handling with user-friendly messages.The authentication flow properly:
- Clears previous errors and sets loading state at the start
- Categorizes errors by type (network, W3ID, redirect) and provides specific messages
- Ensures
authLoadingis reset in thefinallyblockThis provides good user feedback and prevents state leakage between operations.
Also applies to: 282-386
402-404: LGTM! Signing request error handling is thorough.The code properly:
- Clears previous signing errors at the start of
handleSigningRequest- Sets specific error messages for invalid parameters, decoding failures, and parsing errors
- Provides actionable guidance ("Please scan the QR code again")
Also applies to: 433-433, 467-467, 472-472
549-550: Excellent error categorization inhandleSignVote.The error handling distinguishes between:
- Vault/identity errors
- Network/connectivity errors
- Missing redirect URLs
- Server-side submission failures
Each category has a tailored message that helps users understand what went wrong.
Also applies to: 647-663
1045-1051: Good practice: clearing errors when drawers close.This prevents stale error messages from appearing when drawers are reopened, ensuring users always see current state.
Also applies to: 1057-1063
1100-1105: Modal coordination improved, but verify deep link behavior.The code now explicitly closes other drawers before opening the target drawer in
handleDeepLinkData. This prevents multiple drawers from being open simultaneously.Please verify that closing all other modals before handling a deep link doesn't interrupt ongoing operations or lose user state. For example:
- What happens if a user is mid-authentication when a signing deep link arrives?
- Should there be a confirmation before switching contexts?
Run manual testing with various deep link timing scenarios to ensure smooth UX.
Also applies to: 1122-1127, 1237-1242
78-80: LGTM! New error/loading stores properly integrated into the public API.The three new stores are:
- Correctly added to the
ScanStoresinterface- Initialized with appropriate default values
- Exported through the public stores object
This allows consumers to reactively display error and loading states throughout the UI.
Also applies to: 142-144, 1363-1365
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (2)
372-389: Consider using error codes instead of string matching.The error categorization relies on substring matching within error messages, which is fragile and prone to false positives. If the upstream code changes error messages, this logic will break.
Consider introducing custom error classes or error codes:
class NetworkError extends Error { constructor(message: string) { super(message); this.name = 'NetworkError'; } } class W3IDError extends Error { constructor(message: string) { super(message); this.name = 'W3IDError'; } } // Then in error handling: if (error instanceof NetworkError) { errorMessage = "Network error. Please check your connection and try again."; } else if (error instanceof W3IDError) { errorMessage = "Failed to retrieve your identity. Please try again."; }
662-688: Same string matching fragility as inhandleAuth.This error categorization has the same issues noted in the
handleAuthfunction. Consider applying the same custom error class solution here for consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (2)
platforms/pictique/src/lib/stores/posts.ts (1)
error(7-7)infrastructure/eid-wallet/src/lib/global/controllers/security.ts (1)
value(167-177)
⏰ 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
🔇 Additional comments (13)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (13)
78-80: LGTM! Clean interface extension.The new error and loading stores follow the existing pattern and are properly typed for their use cases.
142-144: LGTM! Proper store initialization.The stores are initialized with appropriate default values matching their types.
236-238: LGTM! Proper state initialization.Clearing previous errors and setting loading state at the beginning of the authentication flow ensures clean state for each attempt.
369-392: Verify that silent error handling is intentional.The catch block sets a user-friendly error message but doesn't re-throw or propagate the error. This means the authentication failure is handled silently (user sees the error in the UI but no error is logged to monitoring systems beyond the console.error). Confirm this is the intended behavior.
403-403: LGTM! Proper error state reset.Clearing previous auth errors when handling a new authentication request ensures users see fresh feedback for each attempt.
408-410: LGTM! Consistent error clearing.Clearing previous signing errors at the start of the signing request flow matches the pattern established in
handleAuth.
439-441: Good UX improvement with specific error messages.The different error messages for invalid parameters, decoding failures, and parsing failures help users understand what went wrong. This is a significant improvement over generic error messages.
Also applies to: 475-477, 482-484
561-562: LGTM! Consistent error state management.Clearing previous errors before attempting to sign ensures clean state for the operation.
1072-1076: LGTM! Prevents stale error messages.Clearing the auth error when the drawer closes ensures users don't see outdated error messages when the drawer is reopened.
1084-1088: LGTM! Consistent error clearing pattern.This mirrors the error clearing behavior in
setCodeScannedDrawerOpen, maintaining consistency across drawer state management.
1126-1130: LGTM! Improved drawer state management.Closing conflicting drawers before handling deep links ensures a consistent UI state and prevents multiple drawers from being open simultaneously. The early returns for in-progress operations (lines 1106-1121) correctly prevent unnecessary drawer manipulation.
Also applies to: 1148-1152, 1263-1267
1143-1143: LGTM! Proper error state initialization.Clearing previous errors when handling new deep link requests ensures fresh error feedback for each operation.
Also applies to: 1159-1159, 1273-1273
1388-1390: LGTM! Proper store exposure.The new stores are correctly exposed through the public API, allowing consuming components to observe and react to authentication/signing errors and loading states.
Description of change
Fixes Modal UX on eID Wallet
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes