feat(suite-native): onboarding flow exits #17224
Conversation
|
🚀 Expo preview is ready!
|
WalkthroughThe pull request consolidates updates across multiple modules to enhance device disconnection management, onboarding flow, and firmware update handling. A new dependency on the Jotai library is added with associated updates in package.json files. A new atom is introduced to track user-initiated device disconnections, and hooks are updated to integrate onboarding-specific disconnection logic through a newly added custom hook. Firmware update screens are restructured by separating content and footer components, with the footer logic now encapsulated in a new component that handles user interactions. Onboarding screens are updated to use a component that manages exit functionality, replacing previous Screen and header components. Additionally, navigation hooks and type definitions are refined for improved type safety, and new localization alerts are added to support device disconnection and onboarding cancellation scenarios. Package dependencies and project references are updated accordingly across the affected modules. ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx (1)
19-19: Consider when to set the disconnection flag.Setting
wasDeviceDisconnectedByUserActionto true before showing the alert might cause state inconsistencies if the user doesn't confirm the cancellation.Consider moving this line inside the
onPressPrimaryButtoncallback to ensure it's only set when the user actually confirms the cancellation:- setWasDeviceDisconnectedByUserAction(true); showAlert({ title: translate('moduleOnboarding.cancelOnboardingAlert.title'), description: translate('moduleOnboarding.cancelOnboardingAlert.description'), primaryButtonTitle: translate('generic.buttons.cancel'), primaryButtonVariant: 'redBold', secondaryButtonTitle: translate( 'moduleOnboarding.cancelOnboardingAlert.continueButton', ), secondaryButtonVariant: 'redElevation0', onPressPrimaryButton: () => { + setWasDeviceDisconnectedByUserAction(true); if (selectedDevice) { dispatch(deviceActions.deviceDisconnect(selectedDevice)); } }, });suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx (1)
43-91: Effective handling of device disconnection during onboarding.The implementation properly manages navigation reset and alert display when a device is disconnected, fulfilling the PR objective for better user feedback during disconnection.
However, there are a few areas that could be improved:
- The debounce function is used without a specified timeout, which could lead to unpredictable behavior:
- debounce(() => { + // Use a specific debounce timeout (e.g., 500ms) to ensure consistent behavior + debounce(() => { // Navigation and alert logic - }); + }, 500);
- The hardcoded timeout for alert display might not be sufficient on all devices:
- setTimeout(() => { + // Use a longer timeout or consider using a navigation callback if available + setTimeout(() => { showAlert({ // Alert configuration }); - }, 300); + }, 500);
- The atom state reset should be conditional on the alert being shown:
- setWasDeviceDisconnectedByUserAction(false); + // Only reset the state if we're showing the alert, otherwise keep the current value + if (!wasDeviceDisconnectedByUserAction) { + setWasDeviceDisconnectedByUserAction(false); + }suite-native/firmware/src/components/ConfirmFirmwareUpdateScreenFooter.tsx (1)
16-22: Consider simplifying type definitions.The current approach defines
ConfirmFirmwareUpdateScreenPropswith aheaderprop and then usesExcludeto createConfirmFirmwareUpdateScreenFooterPropswithout it. A more direct approach would be to define a base type and extend it for specific components.-type ConfirmFirmwareUpdateScreenProps = { +type BaseConfirmFirmwareUpdateProps = { onUpdateConfirmation: () => void; onSkipUpdate?: () => void; - header?: ReactNode; }; -type ConfirmFirmwareUpdateScreenFooterProps = Exclude<ConfirmFirmwareUpdateScreenProps, 'header'>; +type ConfirmFirmwareUpdateScreenFooterProps = BaseConfirmFirmwareUpdateProps; +type ConfirmFirmwareUpdateScreenProps = BaseConfirmFirmwareUpdateProps & { + header?: ReactNode; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
suite-native/device/package.json(1 hunks)suite-native/device/src/deviceAtoms.ts(1 hunks)suite-native/device/src/hooks/useHandleDeviceConnection.ts(9 hunks)suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx(1 hunks)suite-native/device/src/index.ts(1 hunks)suite-native/firmware/src/components/ConfirmFirmwareUpdateScreenContent.tsx(1 hunks)suite-native/firmware/src/components/ConfirmFirmwareUpdateScreenFooter.tsx(1 hunks)suite-native/firmware/src/index.ts(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-device-settings/src/screens/ConfirmFirmwareUpdateScreen.tsx(2 hunks)suite-native/module-onboarding/package.json(1 hunks)suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx(1 hunks)suite-native/module-onboarding/src/screens/ConfirmFirmwareUpdateScreen.tsx(2 hunks)suite-native/module-onboarding/src/screens/FirmwareInstallationScreen.tsx(2 hunks)suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx(3 hunks)suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx(3 hunks)suite-native/module-onboarding/tsconfig.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(1 hunks)suite-native/navigation/src/hooks/useNavigationRoute.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-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: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (34)
suite-native/firmware/src/index.ts (1)
5-5: LGTM: Clean export addition.The new export for the ConfirmFirmwareUpdateScreenFooter component follows the established pattern and aligns with the PR objective of enhancing the onboarding flow.
suite-native/device/src/deviceAtoms.ts (1)
1-3: LGTM: Well-defined atom for tracking user-initiated disconnections.This atom is well-designed to track whether device disconnection was initiated by user action, which directly supports the PR objective of displaying alerts when a device is disconnected during onboarding.
suite-native/module-onboarding/tsconfig.json (1)
6-6: LGTM: Proper TypeScript reference configuration.Correctly added reference to the device module, which is necessary for TypeScript to resolve the newly added dependency used for device disconnection tracking.
suite-native/device/package.json (1)
37-37: Added Jotai dependency to support the new atom.The addition of Jotai is appropriate for implementing the device disconnection tracking atom.
Consider checking if version 1.9.1 is the most appropriate or if a more recent version could be used, while ensuring consistency with other modules using Jotai.
suite-native/device/src/index.ts (1)
16-16: Export addition looks goodThe addition of
export * from './deviceAtoms';correctly exposes the new device atom state management for device disconnection tracking.suite-native/module-onboarding/package.json (2)
18-18: Appropriate dependency additionAdding
@suite-native/deviceas a dependency is necessary to access the device atoms that track disconnection states.
26-26: Jotai dependency looks goodAdding Jotai as a dependency is appropriate since it's needed for the atom-based state management that handles device disconnection tracking.
suite-native/module-onboarding/src/screens/FirmwareInstallationScreen.tsx (2)
4-5: Import of new component looks goodThe import of
OnboardingScreenWithExitButtonis properly structured.
16-22: Appropriate component replacementReplacing
ScreenwithOnboardingScreenWithExitButtonimplements the required confirmation when exiting the onboarding flow, as specified in the PR objectives.suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (2)
15-15: Import of new component looks goodThe import of
OnboardingScreenWithExitButtonis properly structured.
80-106: Appropriate component replacementReplacing
ScreenwithOnboardingScreenWithExitButtonmaintains the same UI structure while adding the required exit confirmation functionality, aligning with the PR objectives.suite-native/navigation/src/components/Screen.tsx (1)
18-18: Good enhancement to type visibility!Exporting the
ScreenPropstype is a good practice, as it enables other modules to leverage this type for better type checking and prop validation when using the Screen component.suite-native/intl/src/en.ts (2)
831-835: Well-structured alert for device disconnection.This internationalization entry provides clear guidance to users when their device disconnects during onboarding, which aligns well with the PR objective to improve user awareness in this scenario.
836-840: Good confirmation flow for cancelling onboarding.This alert text helps prevent accidental cancellations by requiring explicit confirmation, directly implementing the second key requirement from the PR objectives.
suite-native/module-device-settings/src/screens/ConfirmFirmwareUpdateScreen.tsx (2)
3-6: Good component restructuring.The imports now clearly separate content from interaction components, following a better component architecture pattern.
26-36: Improved component composition.The restructuring separates content from interaction controls by using the Screen component with a dedicated footer prop. This is a good architectural pattern that:
- Improves component organization
- Enhances reusability
- Creates clearer separation of concerns
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (2)
17-17: Good import of centralized exit button component.Importing the specialized OnboardingScreenWithExitButton component enables consistent exit functionality across the onboarding flow.
100-100: Improved onboarding exit handling.Replacing the generic Screen component with OnboardingScreenWithExitButton centralizes the exit functionality logic, which:
- Creates consistency across multiple onboarding screens
- Reduces code duplication
- Simplifies the implementation of the exit confirmation flow required by the PR objectives
Also applies to: 130-130
suite-native/module-onboarding/src/screens/ConfirmFirmwareUpdateScreen.tsx (2)
8-11: Proper implementation of footer component extraction.The import changes appropriately extract the
ConfirmFirmwareUpdateScreenFootercomponent from the firmware module, improving the separation of concerns.
55-65: Well-implemented integration with exit button functionality.The screen has been refactored to use the new
OnboardingScreenWithExitButtoncomponent while maintaining the original logic for firmware update confirmation. This change aligns with the PR objectives to add confirmation when exiting the onboarding flow.suite-native/device/src/hooks/useHandleDeviceConnection.ts (5)
38-38: Good integration of onboarding disconnection hook.The new hook import aligns with the PR objective to handle device disconnection during onboarding.
66-67: Clean destructuring pattern for hook values.Using destructuring to access both the handler function and state in one statement improves readability.
101-102: Correctly added check for alert display state.Adding the condition to check for
isOnboardingDeviceDisconnectedAlertDisplayedprevents navigation to device onboarding when an alert is already displayed.
189-193: Effective device disconnection handling during onboarding.This implementation properly calls
handleOnboardingDeviceDisconnectionwhen a device is disconnected during the onboarding flow, fulfilling the PR objective to alert users of device disconnection.
211-212: Updated dependencies to match function usage.The dependency array is properly updated to include the new values used in the effect.
suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx (3)
3-3: New dependency on Jotai for state management.Using Jotai for atom-based state management is a good choice for cross-component state like device disconnection tracking.
11-38: Well-implemented onboarding cancellation flow with confirmation.The header component correctly implements the PR objective to require confirmation when canceling the onboarding flow. The alert provides clear options for the user.
40-44: Clean component composition pattern.The component follows a good pattern by using composition to wrap children with the header while passing through any additional props with the spread operator.
suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx (1)
71-87:❓ Verification inconclusive
Alert displayed after navigation might cause race conditions.
The setTimeout approach for showing the alert after navigation completes could be unreliable if navigation takes longer than expected, particularly on slower devices.
Consider using a navigation callback or event listener if available in the navigation library to ensure the alert is shown after navigation is fully complete:
🏁 Script executed:
#!/bin/bash # Check if the navigation library provides completion callbacks or events # Look for navigation completion events or callbacks in the @react-navigation library documentation rg -A 3 "navigation completion|navigation events|onComplete" node_modules/@react-navigationLength of output: 164
Attention: Potential Race Condition in Alert Timing Post-Navigation
The current implementation uses a fixed 300ms delay (via setTimeout) to show the alert, which might trigger before navigation has fully completed—especially on slower devices. Our initial investigation into navigation completion callbacks (e.g., an
onCompleteevent) in the navigation library did not yield any results, which means it's unclear if an alternative event-driven approach is available.
- Review Location:
suite-native/device/src/hooks/useHandleOnboardingDeviceDisconnection.tsx, Lines 71-87- Concern: A setTimeout delay may be unreliable for ensuring that navigation has completely finished before displaying the alert.
- Recommendation:
- Verify manually if the navigation library in use supports completion callbacks or events (such as
onComplete,navigationTransitionEnd, etc.) that can be leveraged here.- If such callbacks exist, refactor the alert display logic to trigger only once navigation is confirmed complete.
- If not, consider alternative mechanisms or additional safeguards to handle potential delays in navigation.
suite-native/firmware/src/components/ConfirmFirmwareUpdateScreenFooter.tsx (2)
38-46: LGTM - Good use of useCallback and alert structure.The alert implementation for confirming firmware updates is well structured with proper translations and action handling.
48-67: LGTM - Button states are properly managed.The component correctly handles button states based on the discovery process and firmware update availability. The skip button is conditionally rendered only when the handler is provided.
suite-native/firmware/src/components/ConfirmFirmwareUpdateScreenContent.tsx (1)
7-22: LGTM - Clean component extraction.The component has been nicely simplified to focus solely on content display, following good separation of concerns principles. The interaction logic has been appropriately moved elsewhere.
suite-native/navigation/src/hooks/useNavigationRoute.ts (2)
6-18: Enhanced type safety and route name resolution.The updated code improves type safety with the new
AppNavigationStatetype and enhances route name resolution by checkingroute.params?.screenbefore falling back toroute.name.
26-27: LGTM - Consistent type assertion.The use of type assertion for the navigation state in
useNavigationRoutealigns with the changes made togetActiveRouteName.
|
Have we also considered the option to stay on the current screen when disconnecting the Trezor in case it disconnects accidentally? Maybe it's overkill now, I just wonder if we've considered that option. |
|
The behaviour when clicking the system back button is funny 😉 Screenrecorder-2025-02-25-17-52-05-977.mp4 |
|
@matejkriz back button behavior and duplicated close button on firmware installation screen fixed in 688199c |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx (1)
21-38: Consider handling the case when no device is selected.
Currently, the device disconnection operation only occurs ifselectedDeviceis available. If the user enters onboarding without a device, pressing "Cancel" might still require a confirmation flow. Consider clarifying or handling the no-device scenario for consistency.suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx (2)
137-143: Consider conditionally including optional callback in dependencies array.While functionally correct, including an optional callback in the dependencies array might cause unnecessary re-renders if the callback is not provided.
}, [ setIsFirmwareInstallationRunning, - onFirmwareInstallationFailure, + ...(onFirmwareInstallationFailure ? [onFirmwareInstallationFailure] : []), firmwareUpdate, handleAnalyticsReportFinished, handleAnalyticsReportCancelled, ]);
50-52: Consider documenting the purpose of the new prop.Since this is a shared component between modules, adding documentation for the new
isCancellationAllowedprop would help future developers understand its purpose.// This component is shared between `module-onboarding` and `module-device-settings`. // Avoid doing anything module specific in this file!!! +// `isCancellationAllowed` controls whether the user can cancel the firmware installation process +// when an error occurs. Set to `false` to prevent cancellation in critical flows. export const FirmwareInstallationScreenContent = ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx(3 hunks)suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx(1 hunks)suite-native/module-onboarding/src/screens/FirmwareInstallationScreen.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-onboarding/src/screens/FirmwareInstallationScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-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: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
suite-native/module-onboarding/src/components/OnboardingScreenWithExitButton.tsx (5)
1-6: Imports look good.
These imports correctly cover React, Redux, and Jotai usage without introducing unnecessary dependencies.
13-19: Clear separation of concerns with dedicated header component.
Defining a separateOnboardingExitButtonScreenHeaderenhances clarity and maintainability by isolating the exit handling logic from the screen container.
40-50: Override back navigation for consistency in user flow.
OverridingGO_BACKis an effective way to ensure consistent exit actions. If you need to handle hardware back-button events (e.g., on Android) more explicitly, confirm that React Navigation’s handling covers this use case or add similar checks to avoid unexpected behavior.
52-53: Good approach to unify exit logic on the header.
PassinghandleExitButtonPressas thecloseActionensures that the entire onboarding exit workflow remains consistent.
55-59: Well-structured composition with the custom header.
Wrapping child components in<OnboardingScreenWithExitButton>centralizes exit handling, reducing repetitive alert/disconnect code in each onboarding screen. This design improves consistency across the onboarding flow.suite-native/firmware/src/components/FirmwareInstallationScreenContent.tsx (5)
44-48: LGTM: Good API enhancement for more flexible usage patterns.The changes to make
onFirmwareInstallationFailureoptional and addingisCancellationAllowedare well-designed. This provides more flexibility for different usage scenarios across modules.
55-56: LGTM: Default value preserves backward compatibility.Setting
isCancellationAllowedto default totruemaintains backward compatibility with existing implementations.
126-127: LGTM: Properly handling optional callback.The optional chaining operator (
?.) is correctly used to safely call the function only if it's provided.
192-193: LGTM: Enhanced control over cancel button visibility.The updated condition provides better control over when the cancel button is displayed, which aligns with the PR objectives for improved onboarding flow control.
197-198: LGTM: Consistent implementation of the conditional display.This change properly implements the updated condition for displaying the cancel button using the new
isCancelButtonDisplayedvariable.
|
/rebase |
688199c to
8090176
Compare
|
/rebase |
8090176 to
37161d6
Compare
|
/rebase |
37161d6 to
5aa5cf1
Compare
Description
Related Issue
Resolve #16291
Screenshots:
IMG_6025.1.mov