feat: add backup page #594
Conversation
WalkthroughThis pull request introduces changes to the wallet restoration and onboarding flow in the extension's user interface. The modifications include adding a new backup detection route and component, updating routing logic in the password typing component, and adjusting the main App.vue to handle the new route. The changes primarily focus on enhancing the user experience during wallet restoration by introducing a step to detect and select existing backups before wallet initialization. Changes
Sequence DiagramsequenceDiagram
participant User
participant TypePasswordComponent
participant BackupDetectedComponent
participant Router
User->>TypePasswordComponent: Enter and validate password
TypePasswordComponent->>Router: Navigate to backup detection route
Router->>BackupDetectedComponent: Load backup selection view
BackupDetectedComponent->>User: Display available backups
User->>BackupDetectedComponent: Select or skip backup
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
💼 Build Files |
![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
🧹 Nitpick comments (1)
packages/extension/src/ui/onboard/restore-wallet/backup-detected.vue (1)
87-89: Avoid fixed height containersUsing a fixed height for the backup items container might cause issues with overflow when there are many backups.
&__backup-items-container { - height: 150px; + max-height: 150px; + overflow-y: auto; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/extension/src/ui/onboard/App.vue(2 hunks)packages/extension/src/ui/onboard/create-wallet/routes.ts(1 hunks)packages/extension/src/ui/onboard/restore-wallet/backup-detected.vue(1 hunks)packages/extension/src/ui/onboard/restore-wallet/routes.ts(2 hunks)packages/extension/src/ui/onboard/restore-wallet/type-password.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (3)
packages/extension/src/ui/onboard/create-wallet/routes.ts (1)
Line range hint
1-8: LGTM!The route configuration is well-structured and follows Vue Router conventions.
packages/extension/src/ui/onboard/restore-wallet/routes.ts (1)
41-45: Verify navigation flow for backup detectionThe backup detection route is correctly added, but let's verify the navigation flow:
- Should this route be accessible directly?
- Should users be able to navigate back from this route?
Run this script to analyze the navigation flow:
✅ Verification successful
Backup detection route follows secure navigation pattern ✅
The route is correctly implemented with proper navigation guards:
- Only accessible through the password verification flow
- Cannot be directly accessed, maintaining security
- Follows the established wallet restoration pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for route guards and navigation patterns rg -A 3 "router\.(push|replace)" packages/extension/src/ui/onboard/Length of output: 8072
packages/extension/src/ui/onboard/App.vue (1)
49-49: LGTM! Back button visibility logic is consistent.The addition of
restore-wallet-backup-detectedto the list of routes where the back button should be hidden follows the existing pattern for terminal states in the workflow.
| isInitializing.value = false; | ||
| router.push({ | ||
| name: routes.walletReady.name, | ||
| name: routes.backupDetected.name, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add error handling for wallet initialization
The wallet initialization process lacks error handling. If initialization fails, the user might be stuck in a broken state.
Consider this implementation:
const nextAction = () => {
if (!isDisabled.value) {
isInitializing.value = true;
- onboardInitializeWallets(store.mnemonic, store.password).then(() => {
- isInitializing.value = false;
- router.push({
- name: routes.backupDetected.name,
- });
- });
+ onboardInitializeWallets(store.mnemonic, store.password)
+ .then(() => {
+ isInitializing.value = false;
+ router.push({
+ name: routes.backupDetected.name,
+ });
+ })
+ .catch((error) => {
+ isInitializing.value = false;
+ // Handle initialization error (e.g., show error message)
+ console.error('Wallet initialization failed:', error);
+ });
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isInitializing.value = false; | |
| router.push({ | |
| name: routes.walletReady.name, | |
| name: routes.backupDetected.name, | |
| }); | |
| }); | |
| const nextAction = () => { | |
| if (!isDisabled.value) { | |
| isInitializing.value = true; | |
| onboardInitializeWallets(store.mnemonic, store.password) | |
| .then(() => { | |
| isInitializing.value = false; | |
| router.push({ | |
| name: routes.backupDetected.name, | |
| }); | |
| }) | |
| .catch((error) => { | |
| isInitializing.value = false; | |
| // Handle initialization error (e.g., show error message) | |
| console.error('Wallet initialization failed:', error); | |
| }); | |
| } | |
| }; |
| <a | ||
| v-for="backup in backups" | ||
| :key="backup" | ||
| @click="selectBackup(backup)" | ||
| :class="[ | ||
| { selected: selectedBackup === backup }, | ||
| 'backup-detected__backup-item', | ||
| ]" | ||
| > | ||
| {{ backup }} | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for backup selection
The backup selection UI needs accessibility improvements:
- Missing proper ARIA attributes
- Using anchor tags (
<a>) for clickable elements without href - No keyboard navigation support
- <a
+ <button
+ type="button"
+ role="option"
+ :aria-selected="selectedBackup === backup"
v-for="backup in backups"
:key="backup"
@click="selectBackup(backup)"
:class="[
{ selected: selectedBackup === backup },
'backup-detected__backup-item',
]"
>
{{ backup }}
- </a>
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a | |
| v-for="backup in backups" | |
| :key="backup" | |
| @click="selectBackup(backup)" | |
| :class="[ | |
| { selected: selectedBackup === backup }, | |
| 'backup-detected__backup-item', | |
| ]" | |
| > | |
| {{ backup }} | |
| </a> | |
| </div> | |
| <button | |
| type="button" | |
| role="option" | |
| :aria-selected="selectedBackup === backup" | |
| v-for="backup in backups" | |
| :key="backup" | |
| @click="selectBackup(backup)" | |
| :class="[ | |
| { selected: selectedBackup === backup }, | |
| 'backup-detected__backup-item', | |
| ]" | |
| > | |
| {{ backup }} | |
| </button> | |
| </div> |
| const useBackup = () => { | ||
| // replace with actual functionality | ||
| window.close(); | ||
| }; | ||
|
|
||
| const skip = () => { | ||
| window.close(); | ||
| }; |
There was a problem hiding this comment.
Implement proper navigation instead of window.close()
Using window.close() directly is not a recommended practice in a Vue application. This should be handled through proper navigation.
-const useBackup = () => {
- // replace with actual functionality
- window.close();
-};
-
-const skip = () => {
- window.close();
-};
+const router = useRouter();
+const useBackup = async () => {
+ try {
+ // Add actual backup restoration logic
+ await restoreFromBackup(selectedBackup.value);
+ router.push({ name: routes.walletReady.name });
+ } catch (error) {
+ // Handle backup restoration error
+ console.error('Failed to restore backup:', error);
+ }
+};
+
+const skip = () => {
+ router.push({ name: routes.walletReady.name });
+};Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| const selectedBackup = ref(''); | ||
| const disabled = computed(() => !selectedBackup.value); | ||
| const backups = ['Backup 1', 'Backup 2', 'Backup 3']; |
There was a problem hiding this comment.
Replace hardcoded backup list
The backups array is hardcoded with test values. This needs to be replaced with actual backup data.
-const backups = ['Backup 1', 'Backup 2', 'Backup 3'];
+// TODO: Implement actual backup retrieval
+const backups = ref<string[]>([]);
+onMounted(async () => {
+ // Add actual backup retrieval logic
+ backups.value = await getAvailableBackups();
+});Committable suggestion skipped: line range outside the PR's diff.
| route.name == 'restore-wallet-wallet-ready' || | ||
| route.name == 'restore-wallet-backup-detected', |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed: Backup detection should not have the 'ready' visual treatment
The backup detection screen is an interactive selection step where users choose from available backups, not a completion state. The 'ready' styling (no background/shadow) should be reserved for actual completion states like 'wallet-ready' which also show the "Pin the extension" instructions.
- Remove 'restore-wallet-backup-detected' from the wrapClassObject conditions to maintain consistent visual patterns throughout the onboarding flow.
🔗 Analysis chain
Verify if backup detection should have the same visual treatment as completion states.
The onboard__wrap--ready class is typically used for completion states (it removes background and box-shadow). However, based on the PR description, the backup detection route is for selecting and using a backup, which seems like an intermediate step rather than a completion state.
This might lead to inconsistent user experience as users might perceive it as a final step when it's actually a selection step.
Consider keeping the standard visual treatment for the backup detection step to maintain consistent UX patterns throughout the onboarding flow.
- route.name == 'restore-wallet-wallet-ready' ||
- route.name == 'restore-wallet-backup-detected',
+ route.name == 'restore-wallet-wallet-ready',🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find route definitions
rg -A 3 "restore-wallet-backup-detected|restore-wallet-wallet-ready" --type vue
# Find other usages of the ready class
rg "onboard__wrap--ready" --type vue
# Look for route configuration files
fd "router|routes" --type file --extension vue,js,ts
Length of output: 229
Script:
#!/bin/bash
# Find route definitions and usages
rg -l "restore-wallet-backup-detected|restore-wallet-wallet-ready"
# Find class usage
rg "onboard__wrap--ready"
# Look for router configuration
fd -e js -e ts -e vue . | rg -i "router|routes"
Length of output: 1499
Script:
#!/bin/bash
# Check restore wallet routes
cat packages/extension/src/ui/onboard/restore-wallet/routes.ts
# Look for backup detection component
rg -A 5 "backup-detected" packages/extension/src/ui/onboard/restore-wallet/
# Check App.vue for context
cat packages/extension/src/ui/onboard/App.vue
Length of output: 8763
Summary by CodeRabbit
New Features
Bug Fixes
Chores