-
Notifications
You must be signed in to change notification settings - Fork 9
373 - add entry point for testing onboarding flow #1898
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: dev
Are you sure you want to change the base?
Conversation
|
⏳ Analyzing changes in this PR... ⏳ This might take a few minutes, please wait 📥 CommitsAnalyzing changes from base (
📁 Files being considered (30)🔄 app/build.gradle (1 hunk) 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.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- f8ccfdb: test: add entry point for testing onboarding flow
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (2 hunks)
Actionable Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt [79-84]
best practice: "Temporary test code should not be committed"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt [169-173]
possible bug: "Missing null check in screen type handling"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt [206-210]
possible bug: "Missing null check in screen type handling"
Skipped Comments (0)
| cvReceive.setOnClickListener { | ||
| // TEMPORARY: Launch onboarding flow for testing | ||
| // TODO: Remove this and restore original receive functionality | ||
| ReactNativeActivity.launch(view.context, RNBridge.ScreenType.ONBOARDING) | ||
| // Original code: ReceiveActivity.launch(view.context) | ||
| } |
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 code contains a temporary modification of the receive button functionality for testing purposes. This should not be committed to the codebase as it overrides the actual receive functionality. Consider using feature flags or test configurations instead of modifying production code for testing.
| val screenString = when (it) { | ||
| RNBridge.ScreenType.SEND_ASSET -> "send-asset" | ||
| RNBridge.ScreenType.TOKEN_DETAIL -> "token-detail" | ||
| RNBridge.ScreenType.ONBOARDING -> "onboarding" | ||
| } |
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 when expression doesn't handle the case where screenType might be null. Consider adding an else branch or making the when expression exhaustive by handling all possible cases to prevent potential runtime errors.
| val screenString = when (screenType) { | ||
| RNBridge.ScreenType.SEND_ASSET -> "send-asset" | ||
| RNBridge.ScreenType.TOKEN_DETAIL -> "token-detail" | ||
| RNBridge.ScreenType.ONBOARDING -> "onboarding" | ||
| } |
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 issue, this when expression doesn't handle the case where screenType might be null. Add an else branch or make the expression exhaustive to prevent potential runtime errors.
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)
- e9f8b25: feat: extending bridge for existing onboarding flow
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [742-767]
security: "Remove debug logging of sensitive information"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [779-780]
possible issue: "Potential username collision in account creation"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [868-874]
enhancement: "Missing activity state handling"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
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)
- 27b7188: pull from dev
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [775-775]
possible bug: "Potential null pointer exception in string operation"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [803-803]
possible issue: "Username generation could cause collisions"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [900-900]
maintainability: "Hard-coded Android version check"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
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)
- 8a7649d: feat: onboarding ui
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (2 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [850-850]
security: "Potential security issue with username generation"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [1019-1038]
possible issue: "Missing default case handling in screen security level switch"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [877-877]
possible bug: "Potential null pointer exception in mnemonic handling"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
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)
- f821df9: feat: onboarding ui
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (3 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [798-802]
security: "Unimplemented critical security feature marked as TODO"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [867-872]
security: "Error handling could expose sensitive data"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [847-847]
possible issue: "Potential username collision risk"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
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)
- 0e4cb3f: feat: onboarding ui entrypoint
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/profile/subpage/wallet/WalletListActivity.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt [47-48]
maintainability: "Inconsistent onboarding launch implementation"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/profile/subpage/wallet/WalletListActivity.kt [174-178]
best practice: "Intent flags should be set after all extras"
app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt
Outdated
Show resolved
Hide resolved
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)
- 991dea6: feat: onboarding ui entrypoint
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (3 hunks)
Actionable Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [772-772]
security: "Missing validation for recovery phrase word count"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [847-847]
security: "Weak username generation mechanism"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [1015-1036]
security: "Improper handling of unknown screen security levels"
Skipped Comments (0)
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
| currentActivity.runOnUiThread { | ||
| when (level) { | ||
| "secure" -> { | ||
| // Prevent screenshots and screen recording | ||
| currentActivity.window.setFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE, | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE enabled") | ||
| } | ||
| "normal" -> { | ||
| // Allow screenshots again | ||
| currentActivity.window.clearFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE disabled") | ||
| } | ||
| else -> { | ||
| android.util.Log.w(TAG, "setScreenSecurityLevel() - unknown level: $level") | ||
| } | ||
| } | ||
| } |
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 screen security level handling should throw an exception for unknown levels rather than just logging a warning. This ensures the app maintains a known security state:
else -> {
throw IllegalArgumentException("Unknown security level: $level")
}- Saves mnemonic to secure storage using Wallet.store() - Creates EOA Account in AccountManager (no Firebase UID) - Initializes WalletManager with new account - Returns success/error response
- Remove generateRecoveryPhrase() (using TypeScript implementation) - Remove createEOAAccount() stub (replaced by saveMnemonic) - Clean up unused code per code review feedback
- Resolved import conflicts in NativeFRWBridge.kt - Updated build.gradle to read GitHub credentials from local.properties
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/handlers/AuthBridgeHandler.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/handlers/AuthBridgeHandler.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/handlers/AuthBridgeHandler.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/handlers/UIBridgeHandler.kt
Outdated
Show resolved
Hide resolved
…Backend Renamed method to match PlatformSpec interface naming convention. Updated all log messages to use new method name. This resolves Android build error: - 'linkCOAAccountOnChain' overrides nothing - Class 'NativeFRWBridge' missing registerAccountWithBackend implementation
Changed generateSeedPhrase() to use InMemoryStorage instead of FileSystemStorage. The mnemonic should NOT be persisted to disk until the user confirms it. Security issue: Previously, the mnemonic was written to disk immediately upon generation, before user confirmation. This creates a security risk if the user abandons the flow or the app crashes before confirmation. The mnemonic will be properly saved to secure storage when saveMnemonic() is called after the user confirms the recovery phrase.
…arding-entrypoint
Update Android entry points to launch React Native onboarding flow instead of native activities: - ProfileSwitchDialog: "Create a new profile" now launches RN onboarding (GetStartedScreen) via ReactNativeActivity - DrawerLayoutContent: "Add account" now launches RN onboarding instead of WalletRestoreActivity Both entry points use RNBridge.ScreenType.ONBOARDING which routes to the GetStartedScreen as the initial route.
Remove WalletUnregisteredFragment and its layout file as they are no longer used. All onboarding entry points now route through: - ProfileSwitchDialog (Create new profile) - DrawerLayoutContent (Add account) - Both launch ReactNativeActivity with RN onboarding flow The WalletUnregisteredFragment was legacy code with no references in the current codebase.
Resolved merge conflicts in DrawerLayoutContent.kt and BridgeModels.kt: - Kept dev's two-button drawer layout (import wallet + add profile) - Preserved onboarding-entrypoint's InitialRoute and NativeScreenName enums - Updated imports to use WalletCreateActivity and WalletRestoreActivity - Added isTestnet check for profile creation This merge combines the improved drawer UX from dev with the bridge models needed for React Native onboarding integration.
Resolved merge conflicts in DrawerLayoutContent.kt and BridgeModels.kt: - Kept dev's two-button drawer layout (import wallet + add profile) - Preserved onboarding-entrypoint's InitialRoute and NativeScreenName enums - Kept BOTH sets of imports for future flexibility: * Dev's: WalletCreateActivity, WalletRestoreActivity (currently used) * Onboarding: ReactNativeActivity, RNBridge (for future RN integration) - Added isTestnet check for profile creation This merge combines the improved drawer UX from dev with the bridge models and imports needed for future React Native onboarding integration.
- Plus icon button now launches ReactNativeActivity with ONBOARDING screen - Removed unused imports (WalletCreateActivity, isTestnet, DialogType, etc.) - Simplified to use React Native onboarding flow for profile creation
Resolved conflict between merge commit and RN onboarding feature: - Keep ReactNativeActivity.launch for add profile button - Remove unused imports (isTestnet, WalletCreateActivity, DialogType) - Maintain clean integration with React Native onboarding flow
Changed onboarding entry points to go directly to ProfileTypeSelection screen instead of GetStarted screen. This applies to: - Drawer "Add profile" button - WalletRestoreActivity import wallet flow Also removed duplicate ONBOARDING case in when statement. Closes #373
- Revert ONBOARDING default route back to GET_STARTED for new users - Update drawer "Add profile" button to use launchWithRoute with PROFILE_TYPE_SELECTION - Update wallet import flow to use launchWithRoute with PROFILE_TYPE_SELECTION This ensures: - New users see GetStarted screen first - Existing users skip GetStarted when adding profiles or importing wallets
Removed 58 auto-generated image files from version control: - 51 React Navigation icon assets (__node_modules_*) - 7 workspace package assets (__packages_*) These files are automatically generated during React Native builds and should not be committed. Updated .gitignore to prevent future commits of these auto-generated resources. The optimized/resized native Android assets (ic_launcher, ic_crown, etc.) remain in version control as they are intentionally optimized native resources.
Resolved merge conflicts: - DrawerLayoutContent.kt: Kept React Native onboarding flow for Add Profile, merged both sets of imports - BridgeModels.kt: Kept new InitialRoute and NativeScreenName enums from onboarding work - build.gradle: Kept comprehensive credentials configuration with local.properties support - Removed auto-generated React Navigation drawable assets (covered by .gitignore) Changes from dev: - WalletConnect utility updates - ReactNativeActivity enhancements - String resource additions
The custom Firebase auth changes are no longer needed as registration is working correctly with the original implementation.
Related Issue
Closes onflow/FRW-monorepo#373
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)