-
Notifications
You must be signed in to change notification settings - Fork 6
feat: update “Import account” ui #1316
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.
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
base: develop
Are you sure you want to change the base?
Conversation
PR SummaryEnhanced the import account flow with a new UI and improved multi-account management functionality. Added support for importing multiple accounts simultaneously, improved account filtering, and updated the profile management interface. Introduced new components for handling account information display and synchronization. Changes
autogenerated by presubmit.ai |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5277d15: feat: update “Import account” ui
Files Processed (28)
- FRW.xcodeproj/project.pbxproj (6 hunks)
- FRW/Modules/ImportAccount/ImportAccountView.swift (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.device.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.device.imageset/restore.icon.device.png (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.device.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.device.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.device.imageset/smartphone.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.multi.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.multi.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.multi.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.multi.imageset/upload-cloud.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.phrase.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.phrase.imageset/file-text.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.phrase.imageset/restore.icon.phrase.png (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.phrase.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/backup/restore/restore.icon.phrase.imageset/[email protected] (0 hunks)
- FRW/Resource/Assets.xcassets/wallet/restore.profile.icon.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/wallet/restore.profile.icon.imageset/restore.profile.icon.svg (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Background/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Background/Nav.colorset/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Cards.colorset/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Text/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Text/Primary.colorset/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/Text/Secondary.colorset/Contents.json (1 hunk)
- FRW/Resource/Colors.xcassets/Summer/icons.colorset/Contents.json (1 hunk)
- FRW/Resource/en.lproj/Localizable.strings (1 hunk)
- FRW/UI/Extension/Color.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
FRW/Modules/ImportAccount/ImportAccountView.swift [73-73]
possible issue: "Incomplete implementation of profile import functionality"
-
FRW/Modules/ImportAccount/ImportAccountView.swift [13-15]
best practice: "Empty view title could affect accessibility and navigation"
-
FRW/Modules/ImportAccount/ImportAccountView.swift [34-38]
maintainability: "Potential navigation issue with network check"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- afa0025: feat: udpate multi-backup UI
Files Processed (15)
- FRW.xcodeproj/project.pbxproj (14 hunks)
- FRW/Modules/MultiRestore/Views/MultiBackupItemView.swift (1 hunk)
- FRW/Modules/MultiRestore/Views/RestoreMultiBackupOptionView.swift (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/check_box_normal.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/check_box_normal.imageset/check-circle-fill 1.svg (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/check_box_normal.imageset/check-circle-fill.svg (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/check_box_selected.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/check_box_selected.imageset/check-circle-fill.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/icon.google.drive.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/backup/icon.google.drive.imageset/Group 48323.pdf (0 hunks)
- FRW/Resource/Assets.xcassets/backup/icon.google.drive.imageset/Group 48483.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/icon.recovery.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/backup/icon.recovery.imageset/Group 47614.svg (1 hunk)
- FRW/Resource/Assets.xcassets/backup/icon.recovery.imageset/file-text.svg (1 hunk)
- FRW/Resource/Assets.xcassets/logo/Icloud.imageset/Contents.json (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Modules/MultiRestore/Views/MultiBackupItemView.swift [19-19]
best practice: "Avoid duplicate state management"
-
FRW/Modules/MultiRestore/Views/MultiBackupItemView.swift [49-49]
enhancement: "Remove unnecessary view modifier"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- c02bc93: feat: rename ImportAccountListView
Files Processed (2)
- FRW.xcodeproj/project.pbxproj (14 hunks)
- FRW/Modules/ImportAccount/ImportAccountListView.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Modules/ImportAccount/ImportAccountListView.swift [73-73]
maintainability: "Incomplete implementation marked with TODO"
-
FRW/Modules/ImportAccount/ImportAccountListView.swift [34-38]
maintainability: "Repeated network validation logic"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 97e1316: feat: avatar
Files Processed (2)
- .gitignore (1 hunk)
- FRW.xcodeproj/project.pbxproj (18 hunks)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 47da5c2: feat: Account Info View
Files Processed (15)
- FRW.xcodeproj/project.pbxproj (27 hunks)
- FRW/Foundation/Model/AccountInfoProtocol.swift (1 hunk)
- FRW/Foundation/Model/MockAccountInfo.swift (1 hunk)
- FRW/Modules/EVM/View/EVMTagView.swift (3 hunks)
- FRW/Modules/Profile/WalletSetting/WalletListView.swift (3 hunks)
- FRW/Modules/Wallet/MoveAsset/MoveAccountsView.swift (1 hunk)
- FRW/Modules/Wallet/MoveAsset/MoveNFTsView.swift (1 hunk)
- FRW/Modules/Wallet/MoveAsset/MoveTokenView.swift (1 hunk)
- FRW/Modules/Wallet/SideMenu/AccountInfoView.swift (1 hunk)
- FRW/Modules/Wallet/SideMenu/AccountSideCell.swift (3 hunks)
- FRW/Resource/Assets.xcassets/Icons/icon-link.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/icon-link.imageset/icon-link.svg (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/icon_button_copy.imageset/Contents.json (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/icon_button_copy.imageset/Icon holder.svg (1 hunk)
- FRW/Resource/Assets.xcassets/Icons/icon_button_copy.imageset/icon_button_copy.svg (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
FRW/Foundation/Model/AccountInfoProtocol.swift [37-38]
best practice: "Potentially misleading hardcoded token count value"
-
FRW/Foundation/Model/MockAccountInfo.swift [26-26]
best practice: "Invalid mock address format"
-
FRW/Modules/Wallet/SideMenu/AccountInfoView.swift [70-74]
best practice: "Missing clipboard operation safeguards"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (3)
Files Processed (7)
- FRW.xcodeproj/project.pbxproj (35 hunks)
- FRW/Foundation/Model/AccountInfoProtocol.swift (1 hunk)
- FRW/Foundation/Model/AccountModel.swift (1 hunk)
- FRW/Foundation/Model/MockAccountInfo.swift (1 hunk)
- FRW/Modules/Wallet/SideMenu/AccountInfoView.swift (1 hunk)
- FRW/Modules/Wallet/SideMenu/SideMenuView.swift (7 hunks)
- FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift (5 hunks)
Actionable Comments (1)
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [165-168]
possible issue: "Potential race condition in account refresh"
Skipped Comments (2)
-
FRW/Foundation/Model/AccountModel.swift [26-30]
possible issue: "Potential issue with flow amount formatting"
-
FRW/Modules/Wallet/SideMenu/AccountInfoView.swift [63-67]
enhancement: "Enhance user feedback for copy action"
guard let current else { | ||
return | ||
} | ||
activeAccount = AccountModel(account: current, mainAccount: WalletManager.shared.isSelectedFlowAccount ? nil : WalletManager.shared.mainAccount, flowCount: "0", nftCount: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refreshActiveAccount()
method accesses multiple shared wallet manager properties without synchronization. Consider using a lock or synchronization mechanism to prevent potential race conditions when accessing these shared resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- fbccde9: feat: enable EVM
Files Processed (4)
- FRW/Foundation/Model/AccountModel.swift (1 hunk)
- FRW/Modules/EVM/View/EVMTagView.swift (2 hunks)
- FRW/Modules/Wallet/SideMenu/SideMenuView.swift (7 hunks)
- FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift (5 hunks)
Actionable Comments (1)
-
FRW/Foundation/Model/AccountModel.swift [31-32]
possible bug: "Potential number parsing issue in balance display"
Skipped Comments (3)
-
FRW/Foundation/Model/AccountModel.swift [36-36]
enhancement: "Incorrect grammar in UI text"
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [179-180]
best practice: "Improve error handling in async operations"
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [142-145]
maintainability: "Complex optional chaining could be simplified"
let flowCount = Double(flowCount) ?? 0 | ||
if flowCount >= 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Double(flowCount)
conversion could fail silently if flowCount
contains an invalid number format. Consider using a more robust number parsing approach or handling the failure case explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 0a97ced: feat: [SideMenu] sorted account by flow balance and nft count
Files Processed (1)
- FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift (5 hunks)
Actionable Comments (1)
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [173-174]
possible bug: "Potential crash when sorting empty arrays"
Skipped Comments (3)
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [206-212]
maintainability: "Duplicated sorting logic"
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [229-231]
possible issue: "Error handling returns empty result"
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [146-146]
best practice: "Hardcoded default values in account model creation"
|
||
guard let l = lhs.first, let r = rhs.first else { return false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array sorting logic could crash if results
contains empty arrays since it unconditionally accesses the first element with lhs.first
and rhs.first
. Consider adding a guard to filter out empty arrays before sorting or handle the nil case differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 92f40a7: feat: remove useless code
Files Processed (1)
- FRW/Modules/Wallet/SideMenu/SideMenuView.swift (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Modules/Wallet/SideMenu/SideMenuView.swift [179-182]
enhancement: "Add error handling and loading state for account switching"
-
FRW/Modules/Wallet/SideMenu/SideMenuView.swift [128-135]
maintainability: "Improve text localization structure"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Files Processed (7)
- FRW.xcodeproj/project.pbxproj (37 hunks)
- FRW/Modules/MultiBackup/Manager/MultiBackupManager.swift (1 hunk)
- FRW/Modules/Wallet/AccountSwitch/AccountSwitchView.swift (4 hunks)
- FRW/Modules/Wallet/AccountSwitch/AccountSwitchViewModel.swift (1 hunk)
- FRW/Modules/Wallet/FlowWalletKit/SecureEnclaveMigration.swift (7 hunks)
- FRW/Modules/Wallet/SideMenu/SideMenuView.swift (2 hunks)
- FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift (5 hunks)
Actionable Comments (2)
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [171-181]
possible bug: "Potential crash when sorting arrays with empty elements"
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [204-211]
possible bug: "Potential crash when parsing flow count strings"
Skipped Comments (1)
-
FRW/Modules/Wallet/SideMenu/SideMenuViewModel.swift [141-147]
possible issue: "Potential race condition when accessing shared wallet manager"
let sortedResult = results.sorted { lhs, rhs in | ||
|
||
guard let l = lhs.first, let r = rhs.first else { return false } | ||
|
||
let lFlow = Double(l.flowCount) ?? 0 | ||
let rFlow = Double(r.flowCount) ?? 0 | ||
if lFlow != rFlow { | ||
return lFlow > rFlow | ||
} | ||
return l.nftCount > r.nftCount | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array sorting logic could crash if results
contains empty arrays since it unconditionally accesses the first element with lhs.first
and rhs.first
. Consider adding a guard to handle empty arrays or filter them out before sorting. For example:
let sortedResult = results.filter { !$0.isEmpty }.sorted { lhs, rhs in
guard let l = lhs.first, let r = rhs.first else { return false }
// ... rest of sorting logic
}
let sortedResult = result.sorted { lhs, rhs in | ||
let lhsFlow = Double(lhs.flowCount) ?? 0 | ||
let rhsFlow = Double(rhs.flowCount) ?? 0 | ||
if lhsFlow != rhsFlow { | ||
return lhsFlow > rhsFlow | ||
} | ||
return lhs.nftCount > rhs.nftCount | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous sorting issue, this sorting logic could crash if any of the flow count strings cannot be converted to Double. Consider adding additional validation or using a more robust number parsing approach. For example:
let lhsFlow = (Double(lhs.flowCount) ?? 0).rounded()
let rhsFlow = (Double(rhs.flowCount) ?? 0).rounded()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- eca50d0: feat: update avatar to square
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (17)
- FRW.xcodeproj/project.pbxproj (59 hunks)
- FRW.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1 hunk)
- FRW.xcodeproj/xcshareddata/xcschemes/FlowWallet-dev.xcscheme (2 hunks)
- FRW/App/AppDelegate.swift (1 hunk)
- FRW/Modules/Profile/AccountSetting/AccountSettingView.swift (1 hunk)
- FRW/Modules/Profile/AccountSetting/Models/LinkedAccountView.swift (1 hunk)
- FRW/Modules/Profile/ProfileSecure/ProfileSecureView.swift (4 hunks)
- FRW/Modules/Profile/ProfileView.swift (9 hunks)
- FRW/Modules/Profile/ProfileViewModel.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/AccountListView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/LinkedAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/View/AccountDetailElement.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/ViewModel/LinkedAccountDetailViewModel.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/WalletListView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/WalletSettingView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/WalletSettingViewModel.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift [179-182]
possible issue: "Empty toggle action for gas fee setting"
-
FRW/Modules/Profile/WalletSetting/WalletSettingViewModel.swift [27-28]
best practice: "Empty catch block swallows errors"
-
FRW/App/AppDelegate.swift [189-190]
best practice: "Missing error handling for UserManager start"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- f9a24ba: feat: update Account Detail
Files Processed (7)
- FRW.xcodeproj/project.pbxproj (61 hunks)
- FRW/Modules/Profile/AccountSetting/AccountSettingView.swift (2 hunks)
- FRW/Modules/Profile/WalletSetting/COAAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/LinkedAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/View/AccountDetailElement.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/ViewModel/LinkedAccountDetailViewModel.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift [76-76]
enhancement: "Add error handling for network changes"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (8)
- FRW.xcodeproj/project.pbxproj (67 hunks)
- FRW.xcodeproj/xcshareddata/xcschemes/FlowWallet-dev.xcscheme (2 hunks)
- FRW/Modules/MultiBackup/View/BackupUploadView.swift (1 hunk)
- FRW/Modules/Profile/AccountSetting/ChildAccountDetailView.swift (3 hunks)
- FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/LinkedAccountDetailView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/View/UnlinkConfirmView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/ViewModel/LinkedAccountDetailViewModel.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
FRW/Modules/Profile/WalletSetting/LinkedAccountDetailView.swift [127-128]
enhancement: "Unimplemented TODO comment"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (2)
Files Processed (4)
- FRW/Modules/Profile/AccountSetting/ChildAccountDetailView.swift (4 hunks)
- FRW/Modules/Profile/ProfileView.swift (9 hunks)
- FRW/Modules/Profile/WalletSetting/AccountListView.swift (1 hunk)
- FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift (1 hunk)
Actionable Comments (2)
-
FRW/Modules/Profile/WalletSetting/AccountListView.swift [49-51]
possible bug: "Potential nil access in guard statement"
-
FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift [219-221]
possible bug: "Ineffective self-assignment in reload function"
Skipped Comments (1)
-
FRW/Modules/Profile/WalletSetting/FlowAccountDetailView.swift [154-157]
possible issue: "Empty toggle action handler"
guard let mainAccount = accounts.first?.account as? FlowWalletKit.Account else { | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guard statement could potentially crash if accounts.first
is nil. Consider adding a default return value or handling the nil case explicitly.
let user = WalletManager.shared.walletAccount.readInfo(at: account.hexAddr) | ||
account = account | ||
WalletManager.shared.changeNetwork(currentNetwork) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reload()
function assigns account = account
which is a self-assignment and has no effect. This line should probably be removed or replaced with the actual updated account value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 96b7121: feat: import profile
Files Processed (9)
- FRW.xcodeproj/project.pbxproj (71 hunks)
- FRW/Modules/Alert/EmptyKeychainAlert.swift (1 hunk)
- FRW/Modules/ImportAccount/ImportAccountListView.swift (1 hunk)
- FRW/Modules/Login/ImportProfileView.swift (1 hunk)
- FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift (1 hunk)
- FRW/Modules/Login/ViewModel/KeyStoreLoginViewModel.swift (4 hunks)
- FRW/Modules/Login/ViewModel/PrivateKeyLoginViewModel.swift (4 hunks)
- FRW/Modules/Login/ViewModel/SeedPhraseLoginViewModel.swift (2 hunks)
- FRW/Modules/Profile/WalletSetting/AccountListView.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [78-78]
enhancement: "Add error handling for empty account list"
-
FRW/Modules/Login/ViewModel/SeedPhraseLoginViewModel.swift [100-101]
maintainability: "Clarify TODO comment about testnet functionality"
-
FRW/Modules/ImportAccount/ImportAccountListView.swift [37-46]
maintainability: "Clean up commented navigation bar code"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- fb6fef5: feat: sync multi-account by device
Files Processed (2)
- FRW.xcodeproj/project.pbxproj (77 hunks)
- FRW/Modules/Login/ViewModel/SyncAccountViewModel.swift (2 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Modules/Login/ViewModel/SyncAccountViewModel.swift [27-31]
best practice: "Consider adding error handling for namespace validation"
-
FRW/Modules/Login/ViewModel/SyncAccountViewModel.swift [13-13]
readability: "Documentation comment could be more descriptive"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- be5ddc1: feat: update status of hide on account
Files Processed (2)
- FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift (1 hunk)
- FRW/Modules/Profile/ProfileView.swift (9 hunks)
Actionable Comments (2)
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [22-22]
possible bug: "Redundant assignment in initialization"
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [88-92]
possible issue: "Potential memory leak in closure"
Skipped Comments (2)
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [52-52]
best practice: "Use isEmpty instead of count comparison"
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [141-144]
enhancement: "Missing error handling for empty username"
self.list = list | ||
accounts = list.map { AccountFetcher.regroup(account: $0, with: [:]) } | ||
self.keyProvider = keyProvider | ||
hideAccount = hideAccount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line hideAccount = hideAccount
is redundant as it's assigning the property to itself. This appears to be a mistake and should be removed since the property is already initialized as an empty array.
createUserName { [weak self] name in | ||
guard let self = self else { | ||
HUD.dismissLoading() | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closure captures self
strongly while also using [weak self]
. This is redundant and could lead to memory leaks. Remove the explicit self
usage inside the closure since you're already handling the weak reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5f6e2e4: feat: update wallet user profile
Files Processed (7)
- FRW.xcodeproj/project.pbxproj (84 hunks)
- FRW/Foundation/Model/AccountInfoProtocol.swift (1 hunk)
- FRW/Foundation/Model/AddressBookInfoModel.swift (1 hunk)
- FRW/Foundation/Model/MockAccountInfo.swift (1 hunk)
- FRW/Foundation/Model/UserModel.swift (1 hunk)
- FRW/Modules/Browser/View/BrowserAuthnView.swift (1 hunk)
- FRW/Modules/Profile/ProfileSecure/ProfileSecureView.swift (4 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Foundation/Model/AccountInfoProtocol.swift [74-75]
enhancement: "Improve fallback name for unnamed accounts"
-
FRW/Foundation/Model/MockAccountInfo.swift [40-41]
best practice: "Use English comments for better code maintainability"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 633f389: feat: remove testdata
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- b4151b1: feat: update struct for account
Files Processed (4)
- FRW/Foundation/Model/AccountModel.swift (1 hunk)
- FRW/Foundation/Model/MockAccountInfo.swift (1 hunk)
- FRW/Modules/Login/ImportProfileView.swift (1 hunk)
- FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift (1 hunk)
Actionable Comments (1)
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [113-113]
security: "Sensitive information exposure in error logs"
Skipped Comments (3)
-
FRW/Foundation/Model/AccountModel.swift [36-36]
best practice: "Potential integer overflow in NFT count display"
-
FRW/Modules/Login/ViewModel/ImportProfileViewModel.swift [75-76]
possible bug: "Potential null pointer exception in error logging"
-
FRW/Modules/Login/ImportProfileView.swift [60-60]
possible issue: "Potential race condition in button state calculation"
} | ||
} else { | ||
HUD.dismissLoading() | ||
log.error("import account failed. \(response.httpCode):\(response.message)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error logging includes HTTP response codes and messages which could contain sensitive information. Consider logging only necessary information and avoid exposing detailed API responses in logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (3)
Files Processed (2)
- FRW.xcodeproj/project.pbxproj (88 hunks)
- FRW.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
FRW.xcodeproj/project.pbxproj [9983-9983]
best practice: "Document dependency version update rationale"
Related Issue
Summary of Changes
⭐⭐⭐ Test Case
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)