feat(suite-native): onboarding unitialized device landing screen #16616
Conversation
|
🚀 Expo preview is ready!
|
|
Beware, the "seedless" term can be confusing. Check https://trezor.io/learn/a/seedless-setup and try to search for "seedless" in our codebase... I believe it would be better to rename it to "uninitialized". |
|
@matejkriz Good point 👀, I will rename it |
PeKne
changed the title
|
|
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
Show resolved
Hide resolved
PeKne
force-pushed
the
feat/native/seedless-device-landing-screen
branch
2 times, most recently
from
94fe6b8 to
c2a76de
Compare
WalkthroughThe pull request introduces changes across navigation, state hooks, utilities, internationalization, and the onboarding module. The onboarding route in the root navigator is renamed to "OnboardingStack" across multiple files, and a new route for the Uninitialized Device Landing screen is added. Several hooks, including those for detecting device errors and handling device connections, now incorporate new selectors (e.g., Assessment against linked issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
|
@vytick FYI I had to squash the commits to be able to rename it (seedless ---> unitialized). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
suite-native/module-onboarding/redux.d.ts (1)
1-7: LGTM! Consider improving type safety.The module augmentation correctly extends Redux's Dispatch interface to support async thunk actions. However, consider making the type parameters more specific instead of using 'any'.
Here's a suggested improvement with stricter typing:
-<TThunk extends AsyncThunkAction<any, any, any>>(thunk: TThunk): ReturnType<TThunk>; +<ReturnType = unknown, State = unknown, ExtraThunkArg = unknown>( + thunk: AsyncThunkAction<ReturnType, State, { rejectValue: Error; extra: ExtraThunkArg }> +): Promise<ReturnType>;Also consider adding JSDoc comments to document the purpose of this extension:
/** * Extends Redux Dispatch to support async thunk actions in the onboarding module. * This enables proper typing for async operations during device initialization. */suite-native/module-onboarding/src/components/HeaderUnderlineSvg.tsx (1)
6-17: LGTM! Well-structured SVG component.The component follows React best practices with proper TypeScript integration and custom hook usage.
Consider memoizing the component with
React.memo()since it only depends on props and thelineColorfrom the hook:-export const HeaderUnderlineSvg = (props: SvgProps) => { +export const HeaderUnderlineSvg = React.memo((props: SvgProps) => { const { lineColor } = useIllustrationColors(); return ( <Svg width={222} height={10} fill="none" {...props}> <Path fill={lineColor} d="M1.995 2.525C53.744-.098 107.902.93 160.472 1.268c18.682.175 37.417 1.564 56.049 3.262l2.801.247.682.06.324.02.126.006c-.111.06.3-.144-.415.159.184 3.4.258 1.022.216 1.75-39.372-4.117-79.695-.91-118.773 2.284-8.64-2.438 79.033-8.419 118.414-4.347.25.043.453.065.775.137.005.744.163-1.63.364 1.815-.77.32-.425.107-.59.171-.138-.005-.462-.019-.599-.037-4.596-.445-10.069-1.018-14.659-1.306-33.506-2.283-67.268-3.043-100.873-2.636-40.776.651-116.234 2.03-102.32-.328Z" /> </Svg> ); -}; +});suite-native/device/src/hooks/useHandleDeviceConnection.ts (1)
48-48: Consider combining related selectors.The new selectors
isDeviceInitializedandisDeviceSetupSupportedare closely related. Consider combining them into a single selector to reduce re-renders and improve performance.-const isDeviceInitialized = useSelector(selectIsDeviceInitialized); -const isDeviceSetupSupported = useSelector(selectIsDeviceSetupSupported); +const { isDeviceInitialized, isDeviceSetupSupported } = useSelector(state => ({ + isDeviceInitialized: selectIsDeviceInitialized(state), + isDeviceSetupSupported: selectIsDeviceSetupSupported(state), +}));Also applies to: 51-51
suite-native/intl/src/en.ts (1)
828-842: LGTM! The strings align well with the PR objectives.The internationalization strings for the uninitialized device landing screen are well-structured and clear. The terminology change from "seedless" to "uninitialized" has been correctly implemented as discussed in the PR comments.
Consider adding a period at the end of the subtitle for consistency with other similar messages in the file:
- 'Firmware is already installed on this Trezor. Continue only if you have used this Trezor before.', + 'Firmware is already installed on this Trezor. Continue only if you have used this Trezor before.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
suite-native/module-onboarding/src/assets/trezorSafe3.pngis excluded by!**/*.pngsuite-native/module-onboarding/src/assets/trezorSafe5.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/device/src/hooks/useDetectDeviceError.tsx(5 hunks)suite-native/device/src/hooks/useHandleDeviceConnection.ts(6 hunks)suite-native/device/src/index.ts(1 hunks)suite-native/device/src/selectors.ts(2 hunks)suite-native/device/src/types.ts(1 hunks)suite-native/device/src/utils.ts(2 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-onboarding/package.json(1 hunks)suite-native/module-onboarding/redux.d.ts(1 hunks)suite-native/module-onboarding/src/components/HeaderUnderlineSvg.tsx(1 hunks)suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx(2 hunks)suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx(1 hunks)suite-native/module-onboarding/tsconfig.json(1 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 (17)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: build
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (18)
suite-native/module-onboarding/package.json (3)
16-16: New Dependency: @reduxjs/toolkit Added
The addition of@reduxjs/toolkitat version1.9.5supports the enhanced Redux-based state management required for the new onboarding functionality. Please ensure any newly created Redux slices or middleware conform to Toolkit best practices.
21-21: New Dependency: @trezor/connect Added
The inclusion of@trezor/connect(using the workspace version) aligns with the monorepo strategy and is expected to facilitate device connectivity features. Verify that its integration with the onboarding module works seamlessly with the rest of the system.
27-27: New Dependency: react-native-svg Added
Addingreact-native-svgwith version^15.9.0enables support for SVG rendering (e.g., the newHeaderUnderlineSvgcomponent). Please check that this version integrates well within the existing React Native setup and that any required native linking or configuration is addressed as needed.suite-native/device/src/types.ts (1)
3-9: LGTM! Well-defined type alias for setup-supporting devices.The type definition is clear, explicit, and follows TypeScript best practices by using the
Excludeutility type to define supported device models.suite-native/device/src/index.ts (1)
15-15: LGTM! Export statement follows consistent pattern.The export statement maintains consistency with other exports in the file.
suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx (1)
11-11: LGTM! Navigation changes follow consistent patterns.The new screen is properly integrated into the navigation stack following React Navigation best practices.
Also applies to: 30-33
suite-native/device/src/utils.ts (1)
42-57: LGTM! Well-structured device support check.The implementation is robust with:
- Exhaustive pattern matching for device models
- Clear separation between supported/unsupported models
- Type safety using UnreachableCaseError
suite-native/navigation/src/routes.ts (1)
4-4: LGTM! Clean navigation route updates.The changes maintain consistency in navigation structure and properly integrate the new uninitialized device flow.
Also applies to: 32-32
suite-native/app/src/navigation/RootStackNavigator.tsx (1)
44-44: LGTM! Consistent route name updates.The changes maintain consistency with the route renaming in routes.ts.
Also applies to: 53-53
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (3)
74-82: Implement navigation logic for TODO comments.Both button handlers currently show toast messages instead of implementing navigation. Please implement the navigation logic for:
- Next onboarding screen based on firmware status
- Screen for tampered device information
85-86: Add close button handler.The close button event handling is missing. Implement the close action to ensure proper navigation flow.
15-22: Validate image height calculation.The image height calculation uses a fixed ratio of screen height. Consider:
- Testing with different screen sizes
- Ensuring the image remains visible with different system font sizes
- Validating the behavior when content overflows
suite-native/device/src/hooks/useHandleDeviceConnection.ts (1)
62-93: LGTM! Well-structured device initialization handling.The new effect correctly handles uninitialized devices by checking multiple conditions before navigating to the onboarding screen. The conditions are comprehensive and include:
- Device setup support check
- Device initialization status
- Device connection status
- Onboarding completion status
- Portfolio tracker check
- Biometrics overlay visibility
suite-native/device/src/selectors.ts (1)
189-192: LGTM! Well-implemented memoized selector.The new selector
selectIsDeviceSetupSupportedis correctly implemented usingcreateMemoizedSelectorfor performance optimization. The null check on the model parameter is a good defensive programming practice.suite-native/navigation/src/navigators.ts (1)
115-115: LGTM! Type-safe route addition.The new route
UninitializedDeviceLandingis correctly added to theOnboardingStackParamListwith proper TypeScript type safety.suite-native/device/src/hooks/useDetectDeviceError.tsx (2)
106-140: LGTM! Improved error handling for unsupported firmware.The condition for showing the unsupported firmware alert now correctly considers device setup support, preventing unnecessary alerts for devices that support setup but have unsupported firmware.
142-205: LGTM! Enhanced uninitialized device handling.The early return condition for uninitialized device alerts now includes
isDeviceSetupSupported, ensuring that alerts are only shown for devices that don't support setup. This aligns with the PR's objective of improving the onboarding experience.suite-native/module-onboarding/tsconfig.json (1)
9-9: LGTM! The dependency addition is appropriate.Adding the reference to the
connectpackage is necessary for the uninitialized device landing screen to interact with the device.
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
Show resolved
Hide resolved
PeKne
force-pushed
the
feat/native/seedless-device-landing-screen
branch
from
f79e01f to
6cb12b4
Compare
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
Outdated
Show resolved
Hide resolved
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13114712466 |
trezor-ci
force-pushed
the
feat/native/seedless-device-landing-screen
branch
from
68cc994 to
4ac48a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/device/src/utils.ts (1)
42-57: LGTM with a minor suggestion!The implementation is well-structured with proper type safety and exhaustive pattern matching. The function clearly separates supported and unsupported device models for setup.
Consider enhancing the comment to better explain the business logic:
- // Exhaustive check for case that new model is introduced later it won't be forgotten. + // Exhaustive check ensures handling of all device models, including future additions. + // Only T2B1, T3B1, and T3T1 models are currently supported for device setup.suite-native/app/src/navigation/RootStackNavigator.tsx (1)
108-111: Document why gesture navigation is disabled for AuthorizeDeviceStack.Consider adding a comment explaining why
gestureEnabled: falseis necessary for this specific stack. This helps other developers understand the security or UX implications.options={{ ...stackNavigationOptionsConfig, + // Disable gesture navigation to prevent accidental exits during device authorization gestureEnabled: false, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
suite-native/module-onboarding/src/assets/t3b1.pngis excluded by!**/*.pngsuite-native/module-onboarding/src/assets/t3t1.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/device/src/hooks/useDetectDeviceError.tsx(5 hunks)suite-native/device/src/hooks/useHandleDeviceConnection.ts(6 hunks)suite-native/device/src/index.ts(1 hunks)suite-native/device/src/selectors.ts(2 hunks)suite-native/device/src/types.ts(1 hunks)suite-native/device/src/utils.ts(2 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-onboarding/package.json(1 hunks)suite-native/module-onboarding/redux.d.ts(1 hunks)suite-native/module-onboarding/src/components/HeaderUnderlineSvg.tsx(1 hunks)suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx(2 hunks)suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx(1 hunks)suite-native/module-onboarding/tsconfig.json(1 hunks)suite-native/navigation/src/navigators.ts(2 hunks)suite-native/navigation/src/routes.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- suite-native/device/src/index.ts
- suite-native/module-onboarding/tsconfig.json
- suite-native/module-onboarding/redux.d.ts
- suite-native/device/src/selectors.ts
- suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx
- suite-native/module-onboarding/src/components/HeaderUnderlineSvg.tsx
- suite-native/device/src/types.ts
- suite-native/module-onboarding/package.json
- suite-native/navigation/src/navigators.ts
- suite-native/device/src/hooks/useDetectDeviceError.tsx
- suite-native/intl/src/en.ts
- suite-native/navigation/src/routes.ts
- suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
- suite-native/device/src/hooks/useHandleDeviceConnection.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (3)
suite-native/device/src/utils.ts (1)
5-5: LGTM!The import of
UnreachableCaseErrorfollows the suggestion from the past review comments.suite-native/app/src/navigation/RootStackNavigator.tsx (2)
44-44: LGTM! Route name change is consistent with the new navigation structure.The renaming from
OnboardingtoOnboardingStackaligns with the new navigation hierarchy and the implementation of the uninitialized device landing screen.
83-84: LGTM! Well-structured navigation group with clear documentation.Good organization of related navigation flows with consistent bottom slide animation. The comment clearly explains the purpose of this grouping.
Description
Related Issue
Resolve #16278
Screenshots:
Firmware-less device
Device with firmware installed