Refactor/native/split device and app onboarding modules #17605
Conversation
WalkthroughThe pull request introduces a dedicated device onboarding flow into the application. A new dependency ( ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-device-onboarding/src/screens/ConfirmFirmwareUpdateScreen.tsx (1)
19-19: Component name updated but import path might need revision.The component has been renamed from
OnboardingScreenWithExitButtontoDeviceOnboardingScreenWithExitButton, but the import path still references the file asOnboardingScreenWithExitButton. This could lead to confusion in the codebase.Consider updating the file name in the import path to match the new component name:
-import { DeviceOnboardingScreenWithExitButton } from '../components/OnboardingScreenWithExitButton'; +import { DeviceOnboardingScreenWithExitButton } from '../components/DeviceOnboardingScreenWithExitButton';suite-native/module-device-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (1)
17-17: Component name updated but import path might need revision.The component has been renamed from
OnboardingScreenWithExitButtontoDeviceOnboardingScreenWithExitButton, but the import path still references the file asOnboardingScreenWithExitButton. This could lead to confusion in the codebase.Consider updating the file name in the import path to match the new component name:
-import { DeviceOnboardingScreenWithExitButton } from '../components/OnboardingScreenWithExitButton'; +import { DeviceOnboardingScreenWithExitButton } from '../components/DeviceOnboardingScreenWithExitButton';suite-native/device/src/hooks/useHandleDeviceConnection.ts (1)
215-218: Dependency array contains unused variableThe
isOnboardingStackFocusedvariable is still included in the dependency array (line 215) even though it no longer appears to be used in this effect. Consider removing it to avoid unnecessary re-renders.}, [ isNoPhysicalDeviceConnected, isOnboardingFinished, navigation, shouldBlockSendReviewRedirect, isFirmwareInstallationRunning, isDeviceCompromisedModalFocused, isSuspiciousDeviceScreenFocused, - isOnboardingStackFocused, handleOnboardingDeviceDisconnection, isConnectAndUnlockDeviceScreenFocused, isDeviceOnboardingStackFocused, ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
suite-native/module-device-onboarding/src/assets/t3b1.pngis excluded by!**/*.pngsuite-native/module-device-onboarding/src/assets/t3b1Seal1.pngis excluded by!**/*.pngsuite-native/module-device-onboarding/src/assets/t3b1Seal2.pngis excluded by!**/*.pngsuite-native/module-device-onboarding/src/assets/t3t1.pngis excluded by!**/*.pngsuite-native/module-device-onboarding/src/assets/t3t1Seal.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
suite-native/app/package.json(1 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/device/src/hooks/useHandleDeviceConnection.ts(7 hunks)suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx(1 hunks)suite-native/device/src/index.ts(1 hunks)suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx(3 hunks)suite-native/firmware/src/hooks/useFirmware.tsx(1 hunks)suite-native/firmware/src/index.ts(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-device-onboarding/package.json(1 hunks)suite-native/module-device-onboarding/src/components/OnboardingScreenWithExitButton.tsx(2 hunks)suite-native/module-device-onboarding/src/components/SecurityCheckStepCard.tsx(4 hunks)suite-native/module-device-onboarding/src/components/SecuritySealDescription.tsx(2 hunks)suite-native/module-device-onboarding/src/components/SecuritySealImages.tsx(1 hunks)suite-native/module-device-onboarding/src/index.ts(1 hunks)suite-native/module-device-onboarding/src/navigation/DeviceOnboardingStackNavigator.tsx(1 hunks)suite-native/module-device-onboarding/src/redux.d.ts(1 hunks)suite-native/module-device-onboarding/src/screens/ConfirmFirmwareUpdateScreen.tsx(4 hunks)suite-native/module-device-onboarding/src/screens/FirmwareInstallationScreen.tsx(2 hunks)suite-native/module-device-onboarding/src/screens/SecurityCheckScreen.tsx(5 hunks)suite-native/module-device-onboarding/src/screens/SuspiciousDeviceScreen.tsx(5 hunks)suite-native/module-device-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx(4 hunks)suite-native/module-device-onboarding/tsconfig.json(1 hunks)suite-native/module-onboarding/package.json(0 hunks)suite-native/module-onboarding/src/components/E2ESkipOnboardingButton.e2e.tsx(1 hunks)suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx(0 hunks)suite-native/module-onboarding/src/screens/BiometricsScreen.tsx(1 hunks)suite-native/module-onboarding/tsconfig.json(0 hunks)suite-native/navigation/src/navigators.ts(3 hunks)suite-native/navigation/src/routes.ts(2 hunks)
💤 Files with no reviewable changes (3)
- suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx
- suite-native/module-onboarding/package.json
- suite-native/module-onboarding/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (73)
suite-native/module-device-onboarding/src/components/SecuritySealImages.tsx (1)
53-56: Translation ID Updated to Reflect Module SeparationThe updated translation ID now follows the new module naming scheme ("moduleDeviceOnboarding...") which aligns well with the PR objectives aimed at splitting the onboarding process. Please ensure that this new key exists in your internationalization resource files to avoid missing translations at runtime.
suite-native/firmware/src/hooks/useFirmware.tsx (1)
18-18: Changed parameter from required to optional which improves flexibilityThe
paramsparameter is now optional, allowing the hook to be called without arguments. This is a good enhancement that enables more flexible usage of the hook, especially in scenarios where you don't need to pass any parameters initially.suite-native/navigation/src/navigators.ts (3)
22-22: LGTM: DeviceOnboardingStackRoutes import addedThis import is correctly added to support the new device onboarding navigation structure.
122-132: Good separation of device onboarding from general onboarding flowThe separation of device-specific onboarding routes into their own parameter list enhances modularity and separation of concerns. This change aligns well with the PR objective of splitting device and app onboarding modules.
214-214: LGTM: RootStackParamList updated with new DeviceOnboardingStackThe root stack has been correctly updated to include the new device onboarding navigation stack, which is necessary for navigating to the device onboarding flow.
suite-native/module-device-onboarding/src/components/OnboardingScreenWithExitButton.tsx (4)
10-11: LGTM: Added import for useFirmware hookThe import is correctly added to support the firmware installation state management feature.
14-14: Consistent renaming for device-specific componentsThe component has been appropriately renamed from
OnboardingExitButtonScreenHeadertoDeviceOnboardingExitButtonScreenHeaderand the exported component fromOnboardingScreenWithExitButtontoDeviceOnboardingScreenWithExitButton, maintaining consistency with the PR's objective of separating device onboarding.Also applies to: 65-66
21-21: Added firmware installation state managementThe component now properly manages firmware installation state with
setIsFirmwareInstallationRunning(false)when exiting, which helps prevent issues with hanging firmware installation processes if the user cancels. The dependencies array has been correctly updated to include the new function.Also applies to: 35-36, 41-48
25-26: Updated translation keys for device-specific contextTranslation keys have been updated from
moduleOnboardingtomoduleDeviceOnboardingprefix, ensuring proper localization context for the device onboarding flow.Also applies to: 30-31
suite-native/firmware/src/index.ts (1)
7-7: LGTM: Exported useFirmware hookThe
useFirmwarehook is now properly exported from the index file, making it accessible to other modules that need firmware installation functionality without having to import it from the specific hooks directory.suite-native/module-device-onboarding/src/index.ts (1)
1-2: Looks good! Appropriate barrel file export.This simple export statement follows best practices for re-exporting components from a module, making the
DeviceOnboardingStackNavigatoraccessible to consumers of this package.suite-native/device/src/index.ts (1)
8-8: Hook export adds clear device disconnection handling for onboarding.This export makes the new
useHandleOnboardingDeviceDisconnectionhook publicly available, which aligns with the PR objective of improving device onboarding stability by providing specialized handling for device disconnection events during onboarding.suite-native/app/tsconfig.json (1)
57-57: Correctly configured TypeScript reference path.The reference path to the new module has been properly added to the TypeScript configuration, maintaining alphabetical ordering. This is necessary for proper type checking and IDE support.
suite-native/app/package.json (1)
66-66: Correctly added module dependency.The new
@suite-native/module-device-onboardingdependency has been properly added with the workspace resolution syntax, consistent with other internal dependencies. This addition supports the PR's goal of splitting device onboarding into a separate module.suite-native/app/src/navigation/RootStackNavigator.tsx (2)
16-16: LGTM! Properly importing the new device onboarding navigator.The import statement correctly references the new DeviceOnboardingStackNavigator from the module-device-onboarding package.
94-97: LGTM! Well-integrated device onboarding screen in the navigation stack.The new DeviceOnboardingStack screen is properly added to the appropriate navigation group with the slide_from_bottom animation, consistent with other similar flows in the application. The placement with other bottom-sliding navigation flows is logical and maintains UI consistency.
suite-native/module-onboarding/src/components/E2ESkipOnboardingButton.e2e.tsx (1)
14-15: Good improvement for test stabilityRemoving the setTimeout and directly dispatching the action before navigation eliminates a potential source of flaky end-to-end tests. Timeouts in tests often lead to race conditions and intermittent failures.
suite-native/module-device-onboarding/src/screens/FirmwareInstallationScreen.tsx (3)
4-4: Updated import path to reflect new module structureThe import path now correctly uses the dedicated device onboarding component, which aligns with the PR objective of splitting onboarding into device-specific and general app modules.
16-16: Updated component name to match the imported componentThe component name has been updated to use
DeviceOnboardingScreenWithExitButtonwhich matches the imported component, maintaining consistency across the codebase.Also applies to: 22-22
20-20: Added retry control for firmware installationThe new
isRetryAllowedprop provides explicit control over whether retry functionality is available during firmware installation. Setting it tofalseprevents users from retrying a failed firmware installation, which is appropriate in the context of initial device onboarding.suite-native/intl/src/en.ts (2)
780-781: New device onboarding module structure in translationsThe introduction of a dedicated
moduleDeviceOnboardingsection in the translations file supports the separation of device onboarding from general app onboarding, which aligns with the PR's main objective.
782-843: Comprehensive device onboarding translationsThe translations cover all necessary UI elements for the device onboarding flow, including:
- Uninitialized device screens with different firmware states
- Security check guidance
- Error handling for device disconnection
- Cancellation confirmations
This thorough coverage ensures a consistent user experience throughout the device onboarding process.
suite-native/module-device-onboarding/src/navigation/DeviceOnboardingStackNavigator.tsx (4)
1-8: Well-structured navigator importsThe imports are organized logically, with React Navigation's core functionality first, followed by parameter types and route definitions, and then configuration options. This organization makes the code easier to maintain.
9-14: Organized screen importsAll required screen components are imported alphabetically, which improves readability and makes it easier to identify which screens are included in this navigation stack.
15-16: Type-safe navigation stackThe navigator is created with proper TypeScript typing (
DeviceOnboardingStackParamList), ensuring type safety when navigating between screens and passing parameters.
17-43: Well-implemented device onboarding navigation flowThe
DeviceOnboardingStackNavigatorcomponent implements a clear, sequential flow for device onboarding:
- Starting with the uninitialized device landing screen
- Handling suspicious device detection
- Performing security checks
- Managing firmware installation
- Confirming firmware updates
All screens use consistent navigation options through the shared
stackNavigationOptionsConfig. This structure aligns perfectly with the PR objective of creating a dedicated device onboarding flow.suite-native/navigation/src/routes.ts (2)
5-5: Good addition to the routing structure!Adding
DeviceOnboardingStackto theRootStackRoutesenum supports the separation of device-specific onboarding flows from the general app onboarding, which aligns well with the PR objective of stabilizing the device onboarding process.
36-42: Well-structured device onboarding routesThe new
DeviceOnboardingStackRoutesenum properly captures the various screens needed for the device onboarding flow. This clear separation of concerns will make the codebase more maintainable and isolate device-specific initialization flows from the general app onboarding.suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx (3)
48-48: Good addition of control parameterAdding the optional
isRetryAllowedproperty enhances component flexibility by allowing parent components to control retry button visibility.
57-57: Smart choice for backward compatibilitySetting the default value to
truemaintains backward compatibility with existing implementations while allowing new implementations to disable the retry option when needed.
233-237: Properly implemented conditional renderingThe conditional rendering of the retry button is well implemented, ensuring it only appears when explicitly allowed. This change helps address the firmware installation issues mentioned in the PR objectives.
suite-native/module-onboarding/src/screens/BiometricsScreen.tsx (1)
49-50: Fixed state transition timingGood simplification by removing the timeout-based state change. Dispatching
setIsOnboardingFinished()immediately before navigation ensures proper state sequencing and helps prevent the device disconnected error mentioned in the PR objectives. This approach avoids race conditions that could occur with the previous timeout implementation.suite-native/module-device-onboarding/src/components/SecuritySealDescription.tsx (2)
36-37: Correct namespace update for translationsUpdating the translation prefix from
moduleOnboardingtomoduleDeviceOnboardingproperly aligns with the new module structure. This ensures correct translations are used in the device-specific context.
64-71: Consistent translation key updatesThe translation IDs have been consistently updated across all instances in this component, ensuring proper localization within the new module structure.
suite-native/module-device-onboarding/src/screens/ConfirmFirmwareUpdateScreen.tsx (4)
13-15: Import names updated to align with new module structure.The import names have been properly updated from
OnboardingStackParamListandOnboardingStackRoutestoDeviceOnboardingStackParamListandDeviceOnboardingStackRoutesto reflect the new module structure.
21-24: Type definition updated for navigation prop.The type definition for
NavigationProphas been correctly updated to useDeviceOnboardingStackParamListandDeviceOnboardingStackRoutes.ConfirmFirmwareUpdate.
43-45: Navigation route updated to new device onboarding structure.The navigation has been correctly updated to use
DeviceOnboardingStackRoutes.FirmwareInstallationto match the new navigation structure.
54-65: Component usage updated to reflect new naming.The component usage has been updated from
OnboardingScreenWithExitButtontoDeviceOnboardingScreenWithExitButtonto maintain consistency with the new module structure.suite-native/module-device-onboarding/src/screens/SuspiciousDeviceScreen.tsx (4)
9-10: Import names updated to align with new module structure.The import names have been properly updated from
OnboardingStackParamListandOnboardingStackRoutestoDeviceOnboardingStackParamListandDeviceOnboardingStackRoutesto reflect the new module structure.
38-40: Screen props type updated to use new stack parameter list.The screen props type has been correctly updated to use
DeviceOnboardingStackParamListandDeviceOnboardingStackRoutes.SuspiciousDeviceinstead of theirOnboardingcounterparts.
74-79: Translation ID prefix updated to new module.The translation ID has been correctly updated to use
moduleDeviceOnboarding.suspiciousDeviceScreen.titleinstead ofmoduleOnboarding.suspiciousDeviceScreen.title.
88-89: Translation IDs consistently updated across component.All translation IDs in the component have been consistently updated to use the
moduleDeviceOnboardingprefix instead ofmoduleOnboarding, which aligns with the new module structure.Also applies to: 96-97, 104-105, 113-114
suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx (2)
72-79: Alert translation IDs updated to new module.The translation IDs for the device disconnected alert have been correctly updated to use the
moduleDeviceOnboardingprefix instead ofmoduleOnboarding.
85-88: Secondary button behavior enhanced with navigation.The secondary button handler has been enhanced to navigate back after hiding the alert, which improves the user experience by returning them to the previous screen when they cancel the reconnection.
This change maintains the original functionality of hiding the alert while adding the improved UX of navigating back when the user cancels.
suite-native/module-device-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (8)
8-10: Import names updated to align with new module structure.The import names have been properly updated from
OnboardingStackParamListandOnboardingStackRoutestoDeviceOnboardingStackParamListandDeviceOnboardingStackRoutesto reflect the new module structure.
48-49: Translation IDs updated for firmware-related content.The translation IDs for firmware-related content have been correctly updated to use the
moduleDeviceOnboardingprefix instead ofmoduleOnboarding.Also applies to: 52-53, 58-59
76-79: Screen props type updated to use new stack parameter list.The screen props type has been correctly updated to use
DeviceOnboardingStackParamListandDeviceOnboardingStackRoutes.UninitializedDeviceLandinginstead of theirOnboardingcounterparts.
83-87: Navigation routes updated to new device onboarding structure.The navigation routes in the confirmation button handler have been correctly updated to use
DeviceOnboardingStackRoutes.ConfirmFirmwareUpdateandDeviceOnboardingStackRoutes.SecurityCheckto match the new navigation structure.
90-94: Navigation route updated for suspicious device flow.The navigation to the suspicious device screen has been correctly updated to use
DeviceOnboardingStackRoutes.SuspiciousDeviceto match the new navigation structure.
96-100: Navigation route consistently updated across handlers.The navigation route in the device looks different handler has been correctly updated to use
DeviceOnboardingStackRoutes.SuspiciousDeviceto match the new navigation structure, maintaining consistency across all navigation handlers.
103-133: Component usage updated to reflect new naming.The component usage has been updated from
OnboardingScreenWithExitButtontoDeviceOnboardingScreenWithExitButtonto maintain consistency with the new module structure.
112-113: Translation IDs consistently updated for UI buttons and labels.All translation IDs for buttons and labels have been consistently updated to use the
moduleDeviceOnboardingprefix instead ofmoduleOnboarding, which ensures consistency across the UI.Also applies to: 120-121, 128-129
suite-native/module-device-onboarding/src/screens/SecurityCheckScreen.tsx (6)
8-10: Good refactoring to separate device onboarding from general onboardingThe navigation routes and params have been correctly updated to use the device-specific types, which aligns well with the PR objective of splitting the onboarding modules.
15-15: Import path updated for consistencyThe component import has been properly updated to use
DeviceOnboardingScreenWithExitButtonfrom the correct path, maintaining consistency with the module separation.
21-21: Translation IDs correctly updated to match the new moduleThe translation keys have been properly updated from
moduleOnboardingtomoduleDeviceOnboardingacross all components, which ensures consistent localization within the new module structure.Also applies to: 25-25, 42-43, 48-51, 88-89, 91-91
67-68: Props type correctly updated to use device-specific navigationThe component props have been properly updated to use
DeviceOnboardingStackParamListandDeviceOnboardingStackRoutes.SecurityCheck, ensuring type safety with the new navigation structure.
78-78: Navigation route updated to reflect new moduleNavigation is now correctly targeting
DeviceOnboardingStackRoutes.FirmwareInstallation, maintaining the flow through the device-specific navigation stack.
82-82: Component usage updated to match new moduleThe component now properly uses
DeviceOnboardingScreenWithExitButtoninstead ofOnboardingScreenWithExitButton, consistent with the module separation.Also applies to: 110-110
suite-native/module-device-onboarding/src/components/SecurityCheckStepCard.tsx (4)
24-26: Updated imports for device-specific navigationThe component correctly imports the new device-specific navigation types, aligning with the module separation approach.
42-45: Navigation props type updated for device-specific contextThe
NavigationPropstype is now correctly usingDeviceOnboardingStackParamListandDeviceOnboardingStackRoutes.SecurityCheck, ensuring type safety with the new navigation structure.
74-74: Navigation route updated for suspicious device screenThe navigation call now correctly targets
DeviceOnboardingStackRoutes.SuspiciousDevice, maintaining consistency with the new module structure.
131-131: Translation ID updated for decline buttonThe translation key has been properly updated to use
moduleDeviceOnboardingnamespace, ensuring consistency with the module restructuring.suite-native/device/src/hooks/useHandleDeviceConnection.ts (8)
25-25: Import added for device-specific navigation routesThe component now correctly imports
DeviceOnboardingStackRoutes, which is necessary for the device-specific navigation flow.
76-76: Updated route reference for suspicious device screenThe
isSuspiciousDeviceScreenFocusedcheck now correctly referencesDeviceOnboardingStackRoutes.SuspiciousDevice, aligning with the new navigation structure.
86-86: Added tracking for device onboarding stack focusA new variable
isDeviceOnboardingStackFocusedhas been added to track when the device onboarding stack is focused, which is essential for the split navigation flows.
97-105: Updated conditions for device onboarding navigationThe navigation logic now includes additional checks for
isOnboardingFinishedand prevents navigation during firmware installation, which helps address the bug mentioned in the PR description where a device disconnected error was displayed during firmware installation.
107-109: Navigation updated to use device-specific stackNavigation now correctly targets the dedicated device onboarding stack, implementing the core of the module separation described in the PR objectives.
114-114: Dependencies updated in useEffectThe dependency array has been properly updated to include the new conditions and variables, ensuring the effect runs when relevant state changes.
Also applies to: 120-121
176-176: Added check for firmware installationThis additional check prevents disrupting the device disconnection handling during firmware installation, addressing part of the issue mentioned in the PR description.
194-194: Updated check for device onboarding stackThe device disconnection handling now correctly checks for
isDeviceOnboardingStackFocusedinstead of the general onboarding stack, aligning with the split module approach.suite-native/module-device-onboarding/package.json (1)
1-33: Package structure looks good for the new device onboarding module.The package configuration is well-structured with appropriate dependencies for a device onboarding module. I see all the necessary React Navigation components, Redux toolkit, and workspace dependencies that would be needed for implementing device connection flows and firmware installation.
suite-native/module-device-onboarding/src/redux.d.ts (1)
1-7: Redux type declaration for async thunks is properly implemented.This declaration file correctly extends the Redux Dispatch interface to handle async thunk actions from Redux Toolkit. This is a standard pattern that will ensure proper typing when dispatching async actions throughout the module.
suite-native/module-device-onboarding/tsconfig.json (1)
1-14: TypeScript configuration is properly set up with all necessary references.The TypeScript configuration extends the base project configuration and correctly references all the required dependencies. The references match the workspace dependencies declared in the package.json file, which is good practice for maintaining consistency across the project.
|
🚀 Expo preview is ready!
|
There were multiple bugs in device onboarding flow introduced recently. This PR is attempt to get the flow to somehow stable state again.
Description
@suite-native/module-onboarding- contains welcome flow that is unrelated to the device connection@suite-native/module-device-onboarding- contains device init flow (security check, fw install etc.)