CP-13334: Reuse app connection screen for add ledger account#3571
CP-13334: Reuse app connection screen for add ledger account#3571ruijialin-avalabs wants to merge 37 commits intomainfrom
Conversation
# Conflicts: # packages/core-mobile/app/new/features/ledger/hooks/useLedgerWallet.ts # Conflicts: # packages/core-mobile/app/new/common/components/WalletCard.tsx
|
@copilot review |
|
@ruijialin-avalabs I've opened a new pull request, #3572, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR refactors Ledger integration to reuse the existing app connection screen for adding new accounts to Ledger wallets, rather than creating separate flows.
Changes:
- Refactored
AppConnectionScreento be reusable for both onboarding and adding accounts - Updated Ledger wallet storage to include device metadata and derivation path type
- Created new hook
useSetLedgerAddressto consolidate address setting logic
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core-mobile/app/services/ledger/types.ts | Updated wallet creation/update option interfaces |
| packages/core-mobile/app/new/routes/(signedIn)/_layout.tsx | Added new route for add account flow |
| packages/core-mobile/app/new/routes/(signedIn)/(modals)/addAccountAppConnection/*.tsx | New route components for adding Ledger accounts |
| packages/core-mobile/app/new/routes/(signedIn)/(modals)/accountSettings/ledger/appConnection.tsx | Updated to use renamed onboarding screen |
| packages/core-mobile/app/new/features/ledger/utils/index.ts | Added getOppositeKeys utility and updated schema |
| packages/core-mobile/app/new/features/ledger/store.ts | Enhanced Ledger wallet mapping with device info and derivation path |
| packages/core-mobile/app/new/features/ledger/screens/*.tsx | Refactored screens to support both onboarding and account addition |
| packages/core-mobile/app/new/features/ledger/hooks/*.ts | New and updated hooks for Ledger wallet management |
| packages/core-mobile/app/new/features/ledger/contexts/LedgerSetupContext.tsx | Simplified context by moving wallet creation logic to hooks |
| packages/core-mobile/app/new/features/ledger/components/LedgerAppConnection.tsx | Made component reusable with configurable title and loading states |
| packages/core-mobile/app/new/common/hooks/useManageWallet.ts | Added navigation to Ledger account addition flow |
| packages/core-mobile/app/new/common/components/WalletCard.tsx | Removed Ledger-specific app connection logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/hooks/useLedgerWallet.ts
Outdated
Show resolved
Hide resolved
|
@ruijialin-avalabs I've opened a new pull request, #3575, to work on those changes. Once the pull request is ready, I'll request review from you. |
| isUpdatingWallet={isUpdatingWallet} | ||
| resetSetup={resetSetup} | ||
| disconnectDevice={disconnectDevice} | ||
| accountIndex={0} |
There was a problem hiding this comment.
mind adding an inline comment here next to accountIndex specifying that we're intentionally setting it to zero here as this screen is used for importing the wallet for the first time
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/core-mobile/app/new/features/ledger/store.ts:59
- In the
removeLedgerWalletfunction, spreading the ledgerWalletMap before deleting creates a shallow copy, but then thedeleteoperator still mutates this copy. While this works, it's cleaner to use object destructuring or filtering to create a new object without the key. For example:const { [walletId]: _, ...newLedgerWalletMap } = get().ledgerWalletMapwould be more functional and clear.
removeLedgerWallet: (walletId: walletId) => {
const newLedgerWalletMap = { ...get().ledgerWalletMap }
delete newLedgerWalletMap[walletId]
set({
ledgerWalletMap: newLedgerWalletMap
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/screens/SolanaConnectionScreen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/screens/SolanaConnectionScreen.tsx
Outdated
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/screens/AppConnectionScreen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/screens/SolanaConnectionScreen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/hooks/useLedgerWallet.ts
Outdated
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/hooks/useLedgerWallet.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
packages/core-mobile/app/new/features/ledger/hooks/useLedgerWallet.ts:102
- The
connectToDevicefunction inuseLedgerWallethas been changed to only accept adeviceIdparameter, but inLedgerSetupContextit's wrapped to also acceptdeviceName. However, the actual connection to the device viaLedgerService.ensureConnectiononly uses thedeviceId. ThedeviceNameparameter is not passed to the service, which means the device name is only being stored in the context state but not used during the actual connection. Verify that this is intentional and that device name is not needed for the connection process.
const connectToDevice = useCallback(async (deviceId: string) => {
setIsConnecting(true)
try {
await LedgerService.ensureConnection(deviceId)
Logger.info('Connected to Ledger device')
} catch (error) {
Logger.error('Failed to connect to device', error)
throw error
} finally {
setIsConnecting(false)
}
}, [])
packages/core-mobile/app/new/features/ledger/store.ts:59
- In the
removeLedgerWalletfunction, the object is being spread before deletion (const newLedgerWalletMap = { ...get().ledgerWalletMap }). While this creates a shallow copy, the mutation happens on line 56 withdelete newLedgerWalletMap[walletId]. This is correct, but consider using a more functional approach like destructuring to avoid the mutation pattern:const { [walletId]: _, ...newLedgerWalletMap } = get().ledgerWalletMap.
removeLedgerWallet: (walletId: walletId) => {
const newLedgerWalletMap = { ...get().ledgerWalletMap }
delete newLedgerWalletMap[walletId]
set({
ledgerWalletMap: newLedgerWalletMap
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/components/LedgerAppConnection.tsx
Outdated
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/components/LedgerAppConnection.tsx
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/screens/AppConnectionScreen.tsx
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/screens/AppConnectionAddAccountScreen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
packages/core-mobile/app/new/features/ledger/screens/SolanaConnectionScreen.tsx:54
- The
isUpdatingWalletvariable is included in the dependency array but it's also checked inside the cleanup function. This creates a potential issue where the cleanup runs based on the captured value ofisUpdatingWalletat render time, but the actual check happens when the component unmounts. IfisUpdatingWalletchanges after render but before unmount, the cleanup logic may not behave as intended. Consider removingisUpdatingWalletfrom the dependency array or restructuring the cleanup logic to use a ref to track the actual updating state.
// Cleanup: Stop polling when component unmounts (unless wallet update is in progress)
useEffect(() => {
return () => {
// Only stop polling if we're not in the middle of wallet update
// If wallet update succeeded, the connection should remain for the wallet to use
if (!isUpdatingWallet) {
Logger.info('AppConnectionScreen unmounting, stopping app polling')
LedgerService.stopAppPolling()
}
}
}, [isUpdatingWallet])
packages/core-mobile/app/new/features/ledger/store.ts:59
- The
removeLedgerWalletfunction correctly creates a new object with the spread operator{ ...get().ledgerWalletMap }before mutating it. This is good practice for immutability. However, consider whether this should also disconnect the device if it's currently connected, similar to how wallet deletion works elsewhere in the codebase.
removeLedgerWallet: (walletId: walletId) => {
const newLedgerWalletMap = { ...get().ledgerWalletMap }
delete newLedgerWalletMap[walletId]
set({
ledgerWalletMap: newLedgerWalletMap
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core-mobile/app/new/features/ledger/screens/AppConnectionAddAccountScreen.tsx
Show resolved
Hide resolved
packages/core-mobile/app/new/features/ledger/screens/AppConnectionAddAccountScreen.tsx
Show resolved
Hide resolved
| useEffect(() => { | ||
| return () => { | ||
| // Only stop polling if we're not in the middle of wallet creation | ||
| // If wallet creation succeeded, the connection should remain for the wallet to use | ||
| if (!isCreatingWallet) { | ||
| if (!isUpdatingWallet) { | ||
| Logger.info('AppConnectionScreen unmounting, stopping app polling') | ||
| LedgerService.stopAppPolling() | ||
| } | ||
| } | ||
| }, [isCreatingWallet]) | ||
| }, [isUpdatingWallet]) |
There was a problem hiding this comment.
The isUpdatingWallet variable is included in the dependency array but it's also checked inside the cleanup function. This creates a potential issue where the cleanup runs based on the captured value of isUpdatingWallet at render time, but the actual check happens when the component unmounts. If isUpdatingWallet changes after render but before unmount, the cleanup logic may not behave as intended. Consider removing isUpdatingWallet from the dependency array or restructuring the cleanup logic to use a ref to track the actual updating state.
| const handleConnectAvalanche = useCallback(async () => { | ||
| try { | ||
| if (!deviceId) { | ||
| throw new Error('No device ID found') |
There was a problem hiding this comment.
The code checks if deviceId is falsy but doesn't provide any recovery mechanism or user feedback when this occurs. When deviceId is undefined or null at the start of the flow (before device connection), the buttons are disabled but the user sees no indication of what's wrong or what they need to do. Consider adding a loading state indicator or error message when device connection is pending, especially for the add account flow where the device info should already be available from the store.
| throw new Error('No device ID found') | |
| Alert.alert( | |
| 'No Ledger Device', | |
| 'No Ledger device is connected. Please connect your Ledger device and try again.', | |
| [{ text: 'OK' }] | |
| ) | |
| return |
Description
iOS: 7482
Android: 7483
Ticket: CP-13334
AppConnectionScreenfor both onboarding and adding accounts.Key Changes
New Zustand store (
packages/core-mobile/app/new/features/ledger/store.ts)Removed hooks
useObserveLedgerState- Eliminated app polling from WalletCard (no longer gates "Add Account")useLedgerDeviceInfo- Replaced withuseLedgerWalletMap().getLedgerInfoByWalletId()Unified AppConnectionScreen
AppConnectionOnboardingScreen) and adding accounts (AppConnectionAddAccountScreen)Simplified LedgerSetupContext
Migration
Store automatically persists to MMKV. Existing wallets will populate the store on next device connection.
Screenshots/Videos
IMG_0822.mov
Testing
Dev Testing (if applicable)
Checklist
Please check all that apply (if applicable)