feat(suite-native): FW install in onboarding flow #17111
Conversation
WalkthroughThis update replaces the previous firmware update component with a new firmware installation workflow. A new component, ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
70-78: Consider simplifying the handlePressConfirmButton function.The current implementation can be more concise by combining the conditions.
const handlePressConfirmButton = () => { - if (currentStep < 3) { - setCurrentStep(currentStep + 1); - - return; - } - - navigation.navigate(OnboardingStackRoutes.FirmwareInstallationScreen); + currentStep < 3 + ? setCurrentStep(currentStep + 1) + : navigation.navigate(OnboardingStackRoutes.FirmwareInstallationScreen); };suite-native/navigation/src/routes.ts (1)
36-36: Consider consistent naming between route enums.There's a naming inconsistency between
OnboardingStackRoutes.FirmwareInstallationScreenandDeviceStackRoutes.FirmwareInstallation. Consider using consistent naming across route enums:Option 1: Use full names
export enum OnboardingStackRoutes { // ... FirmwareInstallationScreen = 'FirmwareInstallationScreen', } export enum DeviceStackRoutes { // ... - FirmwareInstallation = 'FirmwareInstallation', + FirmwareInstallationScreen = 'FirmwareInstallationScreen', }Option 2: Use concise names
export enum OnboardingStackRoutes { // ... - FirmwareInstallationScreen = 'FirmwareInstallationScreen', + FirmwareInstallation = 'FirmwareInstallation', } export enum DeviceStackRoutes { // ... FirmwareInstallation = 'FirmwareInstallation', }Also applies to: 51-51
suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx (2)
95-102: Consider handling edge cases in the success callback.The success callback is called immediately after device authorization, but there's no error handling if the authorization fails.
Consider wrapping the callback in a try-catch:
const handleFirmwareUpdateFinished = useCallback(async () => { - await requestPrioritizedDeviceAccess({ - deviceCallback: () => dispatch(authorizeDeviceThunk()), - }); + try { + await requestPrioritizedDeviceAccess({ + deviceCallback: () => dispatch(authorizeDeviceThunk()), + }); - setIsFirmwareInstallationRunning(false); - onFirmwareInstallationSuccess(); + setIsFirmwareInstallationRunning(false); + onFirmwareInstallationSuccess(); + } catch (error) { + setIsFirmwareInstallationRunning(false); + onFirmwareInstallationFailure(); + } }, [dispatch, onFirmwareInstallationSuccess, setIsFirmwareInstallationRunning]);
108-141: Consider adding a loading state during firmware update.The
startFirmwareUpdatefunction setsisFirmwareInstallationRunningbut doesn't handle the loading state during the update process.Consider adding a loading state to prevent multiple update attempts:
const startFirmwareUpdate = useCallback(async () => { + if (isFirmwareInstallationRunning) return; setIsFirmwareInstallationRunning(true); const result = await firmwareUpdate();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx(9 hunks)suite-native/firmware/src/index.ts(1 hunks)suite-native/module-device-settings/src/navigation/DeviceSettingsStackNavigator.tsx(2 hunks)suite-native/module-device-settings/src/screens/FirmwareInstallationScreen.tsx(1 hunks)suite-native/module-device-settings/src/screens/FirmwareUpdateScreen/FirmwareUpdateScreen.tsx(1 hunks)suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx(2 hunks)suite-native/module-onboarding/src/screens/FirmwareInstallationScreen.tsx(1 hunks)suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx(3 hunks)suite-native/navigation/src/navigators.ts(2 hunks)suite-native/navigation/src/routes.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
suite-native/firmware/src/index.ts (1)
3-3: LGTM! Export changes align with the new firmware installation workflow.The export change correctly reflects the transition from
FirmwareUpdateInProgressScreentoFirmwareInstallationScreenContent.suite-native/module-device-settings/src/screens/FirmwareInstallationScreen.tsx (1)
19-27: Review success handler's fallback behavior.The success handler's fallback to
popToTop()might leave the user in an unexpected state if the initial route is not available.Consider:
- Adding logging to track this edge case
- Defining a specific fallback route instead of
popToTop()const handleFirmwareInstallationSuccess = () => { const initialRoute = navigation.getState().routes.at(0)?.name; if (initialRoute) { navigation.navigate(initialRoute); } else { - // This cause should not happen, but just to be safe - navigation.popToTop(); + // Log the edge case for monitoring + console.warn('No initial route found in navigation state'); + // Navigate to a specific fallback route + navigation.navigate(DeviceStackRoutes.DeviceSettings); } };suite-native/module-device-settings/src/navigation/DeviceSettingsStackNavigator.tsx (1)
42-45: LGTM! Navigation changes align with the new firmware installation workflow.The navigator correctly integrates the new
FirmwareInstallationScreencomponent.suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx (1)
11-11: LGTM! The firmware installation screen is properly integrated into the onboarding flow.The changes correctly implement the firmware installation feature in the onboarding navigation stack, aligning with the PR's objective to enhance the onboarding flow for firmwareless devices.
Also applies to: 45-48
suite-native/module-device-settings/src/screens/FirmwareUpdateScreen/FirmwareUpdateScreen.tsx (1)
58-58: LGTM! Navigation route correctly updated to match the new firmware installation workflow.The change maintains the existing functionality while properly integrating with the new firmware installation process.
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
7-14: LGTM! Navigation implementation properly integrates firmware installation into the security check flow.The changes correctly implement type-safe navigation to the firmware installation screen after completing the security checks.
Also applies to: 65-67, 77-77
suite-native/navigation/src/navigators.ts (1)
126-126:✅ Verification successful
LGTM! Route changes align with the new firmware installation workflow.
The addition of
FirmwareInstallationScreento onboarding and renaming ofFirmwareUpdateInProgresstoFirmwareInstallationin device settings maintains consistency across both flows.Let's verify that these routes are properly used in the navigation components:
Also applies to: 171-171
🏁 Script executed:
#!/bin/bash # Search for usage of the new routes ast-grep --pattern 'FirmwareInstallationScreen' ast-grep --pattern 'FirmwareInstallation'Length of output: 1670
Verified Navigation Routes and Consistent Firmware Workflow
The new routes have been confirmed in both the onboarding and device settings navigation components. Our search results show that:
FirmwareInstallationScreenis correctly imported and used in bothDeviceSettingsStackNavigator.tsxandOnboardingStackNavigator.tsx.- The updated
FirmwareInstallationcomponent is properly referenced in the suite components (e.g., in firmware-related views and steps).LGTM! The changes maintain consistency across both flows.
suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx (2)
44-54: LGTM! Well-structured component with clear props interface.The component is properly documented as shared between modules, with a clear warning against module-specific logic.
250-263: LGTM! Clean implementation of the success state UI.The success state UI is properly animated and includes a continue button that triggers the success callback.
| // TODO: will be implemented as part of follow up issue: https://github.com/trezor/trezor-suite/issues/16291 | ||
| onFirmwareInstallationFailure={() => null} | ||
| /> |
There was a problem hiding this comment.
Implement failure handler.
The failure handler is currently a no-op. While issue #16291 is tracking this, consider implementing a basic error handling flow for better user experience until the full implementation is ready.
Consider implementing basic error handling:
-// TODO: will be implemented as part of follow up issue: https://github.com/trezor/trezor-suite/issues/16291
-onFirmwareInstallationFailure={() => null}
+onFirmwareInstallationFailure={() => {
+ showToast({
+ variant: 'error',
+ message: 'Firmware installation failed. Please try again.',
+ });
+}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: will be implemented as part of follow up issue: https://github.com/trezor/trezor-suite/issues/16291 | |
| onFirmwareInstallationFailure={() => null} | |
| /> | |
| onFirmwareInstallationFailure={() => { | |
| showToast({ | |
| variant: 'error', | |
| message: 'Firmware installation failed. Please try again.', | |
| }); | |
| }} | |
| /> |
| const handleFirmwareInstallationSuccess = () => { | ||
| showToast({ | ||
| variant: 'warning', | ||
| message: 'Firmware installation successful! TODO: redirect to the device AC screen.', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Implement actual redirection in success handler.
The success handler only shows a toast without implementing the actual redirection to the device AC screen.
Consider implementing the redirection logic:
const handleFirmwareInstallationSuccess = () => {
showToast({
variant: 'warning',
message: 'Firmware installation successful! TODO: redirect to the device AC screen.',
});
+ // TODO: Add navigation to device AC screen
+ // navigation.navigate(OnboardingStackRoutes.DeviceAC);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleFirmwareInstallationSuccess = () => { | |
| showToast({ | |
| variant: 'warning', | |
| message: 'Firmware installation successful! TODO: redirect to the device AC screen.', | |
| }); | |
| }; | |
| const handleFirmwareInstallationSuccess = () => { | |
| showToast({ | |
| variant: 'warning', | |
| message: 'Firmware installation successful! TODO: redirect to the device AC screen.', | |
| }); | |
| // TODO: Add navigation to device AC screen | |
| // navigation.navigate(OnboardingStackRoutes.DeviceAC); | |
| }; |
|
🚀 Expo preview is ready!
|
PeKne
changed the title
Description
FirmwareInstallationScreenContentcomponentHow to test it:
Related Issue
Resolve #16289
Screenshots:
@coderabbitai ignore 🐰 ☠️