310/972 ios create native bridge for onboarding workflow #2022
Conversation
PR SummaryImplemented a comprehensive native bridge for the onboarding workflow in iOS, enabling React Native to interact with native iOS functionality. Added new TurboModule methods for seed phrase generation, account registration, wallet initialization, notification permissions, and screen security. Introduced Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (2)
Files Processed (4)
- FRW.xcodeproj/project.pbxproj (13 hunks)
- FRW/Foundation/Bridge/NativeScreenName.swift (1 hunk)
- FRW/Foundation/Bridge/RCTNativeFRWBridge.mm (1 hunk)
- FRW/Foundation/Bridge/TurboModuleSwift.swift (1 hunk)
Actionable Comments (1)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [385-385]
possible bug: "Possible typo in method name."
Skipped Comments (2)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [368-370]
readability: "Variable shadowing may cause confusion in error logging."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [376-376]
performance: "Unnecessary object instantiation for some code paths."
| case .keyStoreRestore: | ||
| restoreModel.restoreWithKeyStore() | ||
| case .privateKeyRestore: | ||
| restoreModel.resteroWithPrivateKey() |
There was a problem hiding this comment.
There appears to be a typo in the method name resteroWithPrivateKey(). It should likely be restoreWithPrivateKey(). Please verify this is the correct method name in RestoreWalletViewModel.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 411f0d3: Merge branch 'dev' into 310/972-ios-create-native-bridge-for-onboarding-workflow
Files Processed (3)
- FRW.xcodeproj/project.pbxproj (13 hunks)
- FRW/Foundation/Bridge/NativeScreenName.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModuleSwift.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [369-369]
readability: "Misleading variable shadowing in error log message."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [376-376]
performance: "Unnecessary object instantiation for some screen types."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- d28c43b: feat: onboarding
Files Processed (15)
- FRW.xcodeproj/project.pbxproj (9 hunks)
- FRW/Foundation/Bridge/BridgeModels.swift (1 hunk)
- FRW/Foundation/Bridge/NativeScreenName.swift (1 hunk)
- FRW/Foundation/Bridge/RCTNativeFRWBridge.mm (1 hunk)
- FRW/Foundation/Bridge/RNBridgeError.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModule+Onboard.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModuleSwift.swift (2 hunks)
- FRW/Modules/MultiBackup/View/BackupListView.swift (2 hunks)
- FRW/Modules/ReactNative/ReactNativeViewController.swift (3 hunks)
- FRW/Modules/Wallet/AccountSwitch/AccountSwitchViewModel.swift (1 hunk)
- FRW/Modules/Wallet/SideMenu/SideContainerView.swift (1 hunk)
- FRW/Services/Manager/UserManager.swift (3 hunks)
- FRW/Services/Manager/Wallet/WalletManager.swift (1 hunk)
- FRW/Services/Router/RouteMap+React.swift (2 hunks)
- main.jsbundle (0 hunks)
Actionable Comments (1)
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [107-110]
possible issue: "Unused parameters in saveMnemonic method."
Skipped Comments (7)
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [52-52]
best practice: "Debug print statements should be removed or use proper logging."
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [107-107]
security: "Debug print statement in sensitive mnemonic handling method."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [370-372]
readability: "Variable shadowing causes confusing error message."
-
FRW/Services/Manager/UserManager.swift [82-82]
possible issue: "Dictionary may grow indefinitely without cleanup."
-
FRW/Modules/Wallet/AccountSwitch/AccountSwitchViewModel.swift [37-39]
maintainability: "Commented-out code should be removed."
-
FRW/Modules/Wallet/SideMenu/SideContainerView.swift [40-44]
maintainability: "Empty HStack used as a workaround for onAppear."
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [21-24]
best practice: "UI feedback mixed with error throwing logic."
| static func saveMnemonic(mnemonic: String, customToken: String, txId: String, username: String) async throws { | ||
| print("TurboModuleSwift: saveMnemonic called") | ||
| try await UserManager.shared.restoreLogin(withMnemonic: mnemonic) | ||
|
|
There was a problem hiding this comment.
The parameters customToken, txId, and username are declared but never used in the method body. Either remove these unused parameters or implement the logic that requires them.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Files Processed (2)
- FRW/Modules/ReactNative/ReactNativeViewController.swift (4 hunks)
- FRW/Services/Router/RouteMap+React.swift (2 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Modules/ReactNative/ReactNativeViewController.swift [178-178]
possible issue: "Default route fallback may be inappropriate for new onboarding routes."
-
FRW/Services/Router/RouteMap+React.swift [29-32]
maintainability: "Inconsistent initialization pattern between route cases."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- ee5a37b: Merge branch 'dev' into 310/972-ios-create-native-bridge-for-onboarding-workflow
Files Processed (3)
- FRW.xcodeproj/project.pbxproj (9 hunks)
- FRW/Services/Manager/UserManager.swift (3 hunks)
- FRW/Services/Manager/Wallet/WalletManager.swift (1 hunk)
Actionable Comments (1)
-
FRW/Services/Manager/UserManager.swift [83-83]
possible bug: "Dictionary may have thread-safety issues when accessed from multiple threads."
Skipped Comments (3)
-
FRW/Services/Manager/UserManager.swift [83-83]
best practice: "Variable name does not follow Swift naming conventions."
-
FRW/Services/Manager/UserManager.swift [218-218]
possible issue: "No error handling when wallet address creation fails."
-
FRW/Services/Manager/UserManager.swift [227-229]
maintainability: "Dictionary entries are never cleaned up, potentially causing memory growth."
| } | ||
|
|
||
| // It is only used when the bridge is called on page of onboard | ||
| var RNRegisterInfo:[String: String] = [:] |
There was a problem hiding this comment.
The dictionary RNRegisterInfo is declared as a var without any synchronization mechanism. If this dictionary is accessed from multiple threads (e.g., from React Native bridge calls and native code), it could lead to race conditions and data corruption. Consider using a thread-safe approach such as a concurrent queue, NSLock, or an actor.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a3d7777: feat: update RN
Files Processed (3)
- FRW/Modules/Profile/ProfileViewModel.swift (1 hunk)
- FRW/Services/Manager/Transaction/TransactionHolder.swift (1 hunk)
- main.jsbundle (0 hunks)
Actionable Comments (0)
Skipped Comments (1)
-
FRW/Services/Manager/Transaction/TransactionHolder.swift [95-98]
possible issue: "Verify that completion handler should also run on main queue."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 169fbc1: feat: eoa add parent for RN
Files Processed (1)
- FRW/Modules/Wallet/WalletAccount/WalletAccount+FlowWalletKit.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
FRW/Modules/Wallet/WalletAccount/WalletAccount+FlowWalletKit.swift [133-139]
possible issue: "Empty string handling for optional parent address."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (5)
- FRW/Foundation/Bridge/RCTNativeFRWBridge.mm (2 hunks)
- FRW/Foundation/Bridge/TurboModule+Onboard.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModuleSwift.swift (4 hunks)
- FRW/Modules/ReactNative/ReactNativeViewController.swift (5 hunks)
- FRW/Services/Router/Coordinator.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (5)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [251-253]
possible issue: "Empty method implementation will silently do nothing."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [392-394]
possible bug: "Variable shadowing causes incorrect error message."
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [111-119]
possible issue: "Screen security method has no actual implementation."
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [137-137]
best practice: "Debug print statement should use proper logging."
-
FRW/Services/Router/Coordinator.swift [101-101]
enhancement: "Animation parameter may cause visual issues during initial setup."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 98f43e4: feat: add animation and setup flag
Files Processed (1)
- FRW/Services/Router/Coordinator.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Services/Router/Coordinator.swift [77-83]
possible issue: "Subscription may not be properly cleaned up on coordinator reuse."
-
FRW/Services/Router/Coordinator.swift [127-131]
enhancement: "Animation approach may cause visual artifacts during root view transition."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- c5dd810: feat: add restore from icloud
Files Processed (2)
- FRW/Foundation/Bridge/NativeScreenName.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModuleSwift.swift (4 hunks)
Actionable Comments (1)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [392-394]
possible bug: "Variable shadowing causes incorrect error message."
Skipped Comments (3)
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [251-253]
possible issue: "Empty method implementation may cause silent failures."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [240-243]
possible issue: "Method returns all profiles instead of filtering for recoverable ones."
-
FRW/Foundation/Bridge/TurboModuleSwift.swift [7-8]
maintainability: "Unused import statement."
| guard let screen = NativeScreenName(rawValue: screen) else { | ||
| log.error("don't support route \(screen)") | ||
| HUD.error(title: "don't support route \(screen)") |
There was a problem hiding this comment.
The error message logs and displays the screen variable after it has been shadowed by the failed NativeScreenName initialization, which will always be nil at that point. The original screen: String parameter should be used in the error message instead.
Related Issue
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)