Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add backup page #594

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat: add backup page #594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/extension/src/ui/onboard/App.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const isShowBackButton = () => {
route.name != 'user-analytics' &&
route.name != 'create-wallet-wallet-ready' &&
route.name != 'restore-wallet-wallet-ready' &&
route.name != 'restore-wallet-backup-detected' &&
!(route.name as string).includes('hardware-wallet')
);
};
Expand All @@ -54,7 +55,8 @@ const wrapClassObject = () => {
return {
'onboard__wrap--ready':
route.name == 'create-wallet-wallet-ready' ||
route.name == 'restore-wallet-wallet-ready',
route.name == 'restore-wallet-wallet-ready' ||
route.name == 'restore-wallet-backup-detected',
Comment on lines +58 to +59
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

'onboard__wrap--auto-height': route.path.match(/hardware-wallet/),
};
};
Expand Down
1 change: 1 addition & 0 deletions packages/extension/src/ui/onboard/create-wallet/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import CheckPhrase from './double-check-phrase.vue';
import WalletReady from './wallet-ready.vue';
import UserAnalytics from '../user-analytics.vue';
import { RouteRecordRaw } from 'vue-router';

export const routes = {
pickPassword: {
path: 'pick-password',
Expand Down
125 changes: 125 additions & 0 deletions packages/extension/src/ui/onboard/restore-wallet/backup-detected.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
<template>
<div class="backup-detected">
<h3>Found backup</h3>

<div class="backup-detected__backups">
<h4>Please choose a backup to use:</h4>
<div class="backup-detected__backup-items-container">
<a
v-for="backup in backups"
:key="backup"
@click="selectBackup(backup)"
:class="[
{ selected: selectedBackup === backup },
'backup-detected__backup-item',
]"
>
{{ backup }}
</a>
</div>
Comment on lines +8 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve accessibility for backup selection

The backup selection UI needs accessibility improvements:

  1. Missing proper ARIA attributes
  2. Using anchor tags (<a>) for clickable elements without href
  3. 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.

Suggested change
<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>

</div>
<base-button title="Use backup" :disabled="disabled" :click="useBackup" />
<base-button
style="margin-top: 10px"
no-background
title="Skip"
:click="skip"
/>
</div>
</template>
<script setup lang="ts">
import BaseButton from '@action/components/base-button/index.vue';
import { computed, ref } from 'vue';

const selectedBackup = ref('');
const disabled = computed(() => !selectedBackup.value);
const backups = ['Backup 1', 'Backup 2', 'Backup 3'];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


const selectBackup = (backup: string) => {
if (selectedBackup.value === backup) {
selectedBackup.value = '';
} else {
selectedBackup.value = backup;
}
};
const useBackup = () => {
// replace with actual functionality
window.close();
};

const skip = () => {
window.close();
};
Comment on lines +45 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

</script>

<style lang="less">
@import '@action/styles/theme.less';

.selected {
background: @default;
border-radius: 10px;
}
.backup-detected {
width: 100%;

&__logo {
margin-bottom: 24px;
}

h3 {
font-style: normal;
font-weight: 700;
font-size: 34px;
line-height: 40px;
letter-spacing: 0.25px;
color: @primaryLabel;
margin: 0 0 24px 0;
}
h4 {
font-style: normal;
font-weight: 400;
font-size: 18px;
line-height: 24px;
color: @secondaryLabel;
margin: 0 0 8px 0;
}

&__backup-items-container {
height: 150px;
}

&__backup-item {
height: 50px;
padding: 0 16px;
display: flex;
font-size: 16px;
align-items: center;
justify-content: center;
}

&__backups {
margin-bottom: 24px;

h4 {
font-style: normal;
font-weight: 400;
font-size: 16px;
line-height: 24px;
color: @secondaryLabel;
margin: 0 0 8px 0;
}

&-wrap {
display: flex;
flex-direction: row;
align-items: center;
justify-content: flex-start;

a {
display: block;
margin-right: 24px;
}
}
}
}
</style>
6 changes: 6 additions & 0 deletions packages/extension/src/ui/onboard/restore-wallet/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import PickPassword from './pick-password.vue';
import TypePassword from './type-password.vue';
import WalletReady from '../create-wallet/wallet-ready.vue';
import UserAnalytics from '../user-analytics.vue';
import BackupDetected from './backup-detected.vue';
import { RouteRecordRaw } from 'vue-router';
export const routes = {
start: {
Expand Down Expand Up @@ -37,6 +38,11 @@ export const routes = {
name: 'user-analytics',
component: UserAnalytics,
},
backupDetected: {
path: 'backup-detected',
name: 'backup-detected',
component: BackupDetected,
},
walletReady: {
path: 'wallet-ready',
name: 'wallet-ready',
Expand Down
2 changes: 1 addition & 1 deletion packages/extension/src/ui/onboard/restore-wallet/type-password.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const nextAction = () => {
onboardInitializeWallets(store.mnemonic, store.password).then(() => {
isInitializing.value = false;
router.push({
name: routes.walletReady.name,
name: routes.backupDetected.name,
});
});
Comment on lines 46 to 50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
});
}
};

}
Expand Down
Loading