feat(suite-native): onboarding security check #16883
Conversation
![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 (9)
suite-native/module-onboarding/package.json (1)
27-27: New Dependency Addition and Configuration Reminder
The addition of "react-native-reanimated": "^3.16.7" is appropriate to support the new security check animations. Please ensure that the Babel configuration (typically in babel.config.js) is updated to include the react-native-reanimated plugin if required by the version used.suite-native/module-onboarding/src/components/SecuritySealImages.tsx (1)
14-61: Consider performance optimizations.
- Memoize the component to prevent unnecessary re-renders when parent components update.
- Consider using static image imports for better performance and type safety.
Apply this diff to optimize the component:
-export const SecuritySealImages = () => { +import { memo } from 'react'; + +const t3t1SealImage = require('../assets/t3t1Seal.png'); +const t3b1Seal1Image = require('../assets/t3b1Seal1.png'); +const t3b1Seal2Image = require('../assets/t3b1Seal2.png'); + +export const SecuritySealImages = memo(() => { const { applyStyle } = useNativeStyles(); const deviceModel = useSelector(selectDeviceModel); const isTrezorSafe5 = deviceModel === DeviceModelInternal.T3T1; if (isTrezorSafe5) { return ( <Box alignItems="center"> <Image - source={require('../assets/t3t1Seal.png')} + source={t3t1SealImage} contentFit="contain" style={applyStyle(trezorImageStyle, { isTrezorSafe5: true, })} /> </Box> ); } return ( <VStack spacing="sp24" alignItems="center"> <HStack justifyContent="space-between" flex={1}> <Image - source={require('../assets/t3b1Seal1.png')} + source={t3b1Seal1Image} contentFit="contain" style={applyStyle(trezorImageStyle, { isTrezorSafe5: false, })} /> <Image - source={require('../assets/t3b1Seal2.png')} + source={t3b1Seal2Image} contentFit="contain" style={applyStyle(trezorImageStyle, { isTrezorSafe5: false, })} /> </HStack> <AlertBox variant="info" textVariant="hint" contentColor="textDefault" title={ <Translation id="moduleOnboarding.securityCheckScreen.step2.modal.alertBox" /> } /> </VStack> ); -}; +});suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
14-57: Consider using enum for step numbers.The step numbers are currently hardcoded. Using an enum would make the code more maintainable and type-safe.
+enum SecurityCheckStep { + ResellersCheck = 1, + SealCheck = 2, + PackagingCheck = 3, +} + const stepToContentMap = { - 1: { + [SecurityCheckStep.ResellersCheck]: { header: <Translation id="moduleOnboarding.securityCheckScreen.step1.header" />, // ... }, - 2: { + [SecurityCheckStep.SealCheck]: { header: <Translation id="moduleOnboarding.securityCheckScreen.step2.header" />, // ... }, - 3: { + [SecurityCheckStep.PackagingCheck]: { header: <Translation id="moduleOnboarding.securityCheckScreen.step3.header" />, // ... }, } as const satisfies Record< - 1 | 2 | 3, + SecurityCheckStep, { header: ReactNode; // ... } >;suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx (3)
26-34: Props type looks good, but consider adding JSDoc comments.The
SecurityCheckStepCardPropstype is well-defined with clear property names. Consider adding JSDoc comments to document the purpose of each prop for better maintainability.+/** + * Props for the SecurityCheckStepCard component + * @property header - The header content to display + * @property description - The description content to display + * @property isChecked - Whether the step is checked/completed + * @property isOpened - Whether the step card is expanded + * @property icon - The icon to display + * @property onPressConfirmButton - Callback when confirm button is pressed + * @property suspicionCause - The cause of suspicion if user declines + */ type SecurityCheckStepCardProps = { header: ReactNode; description: ReactNode; isChecked: boolean; isOpened: boolean; icon: IconName; onPressConfirmButton: () => void; suspicionCause: DeviceSuspicionCause; };
71-72: Consider extracting this logic to a constant.The condition for determining if it's the first step card is used for animation control. Consider extracting it to a named constant at the top of the component for better readability and reusability.
+const FIRST_STEP_SUSPICION_CAUSE = 'untrustedReseller'; + export const SecurityCheckStepCard = ({ // ...props }) => { - const isFirstStepCard = suspicionCause === 'untrustedReseller'; + const isFirstStepCard = suspicionCause === FIRST_STEP_SUSPICION_CAUSE;
89-93: Consider extracting animation configuration.The animation configuration is embedded within the JSX. Consider extracting it to a constant for better maintainability and reusability.
+const ANIMATION_CONFIG = { + entering: FadeIn.delay(150), + exiting: FadeOutUp, +}; + export const SecurityCheckStepCard = ({ // ...props }) => { // ... return ( // ... <AnimatedVStack spacing="sp16" - entering={isFirstStepCard ? undefined : FadeIn.delay(150)} - exiting={FadeOutUp} + entering={isFirstStepCard ? undefined : ANIMATION_CONFIG.entering} + exiting={ANIMATION_CONFIG.exiting} >suite-native/module-onboarding/src/components/SecuritySealDescription.tsx (2)
23-27: Consider using a constant for device model comparison.The device model comparison could be more maintainable using a constant. Also, consider handling the case where
deviceModelis undefined.+const TREZOR_SAFE_5_MODEL = DeviceModelInternal.T3T1; + export const SecuritySealDescription = () => { // ... - const isTrezorSafe5 = deviceModel === DeviceModelInternal.T3T1; + const isTrezorSafe5 = deviceModel === TREZOR_SAFE_5_MODEL;
51-93: Consider extracting bottom sheet content to a separate component.The bottom sheet content is quite complex and could be extracted to improve readability and maintainability.
+const SecuritySealBottomSheetContent = ({ + onClose, + onLearnMore, +}: { + onClose: () => void; + onLearnMore: () => void; +}) => ( + <VStack spacing="sp32" justifyContent="flex-end"> + {/* ... existing content ... */} + </VStack> +); export const SecuritySealDescription = () => { // ... return ( <> <Text variant="highlight"> {/* ... */} </Text> <Box flex={0}> <BottomSheet isVisible={isBottomSheetVisible} onClose={closeBottomSheet} isCloseDisplayed={false} > - <VStack spacing="sp32" justifyContent="flex-end"> - {/* ... existing content ... */} - </VStack> + <SecuritySealBottomSheetContent + onClose={closeBottomSheet} + onLearnMore={handleLearnMoreButtonPress} + /> </BottomSheet> </Box> </> ); };suite-native/intl/src/en.ts (1)
857-884: LGTM! Well-structured security check translations with clear user guidance.The translations effectively guide users through the security verification process with clear, actionable steps. The use of rich text formatting for links and the inclusion of a modal for detailed holographic seal verification shows good attention to user education.
Consider adding tooltips or help text for the "I'm not sure" button to set user expectations about what happens when they click it.
Add a tooltip translation:
declineButton: 'I'm not sure', + declineButtonTooltip: 'Clicking this will take you to a support page for assistance',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfsuite-native/module-onboarding/src/assets/t3b1Seal1.pngis excluded by!**/*.pngsuite-native/module-onboarding/src/assets/t3b1Seal2.pngis excluded by!**/*.pngsuite-native/module-onboarding/src/assets/t3t1Seal.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
suite-common/icons/generateIconFont.ts(2 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/atoms/src/AlertBox.tsx(4 hunks)suite-native/intl/src/en.ts(2 hunks)suite-native/module-onboarding/package.json(1 hunks)suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx(1 hunks)suite-native/module-onboarding/src/components/SecuritySealDescription.tsx(1 hunks)suite-native/module-onboarding/src/components/SecuritySealImages.tsx(1 hunks)suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx(2 hunks)suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx(1 hunks)suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx(1 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- 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: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
🔇 Additional comments (11)
suite-common/icons/generateIconFont.ts (1)
77-77: LGTM! New icons align with security check feature.The addition of
package,seal, andselectionSlashicons supports the new onboarding security check screen functionality.Also applies to: 92-93
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (2)
28-29: LGTM! Icon mappings are correctly configured.The new security-related icons (
selectionSlash,seal,package) are properly mapped to unique unicode points, maintaining sequential ordering.Also applies to: 44-44
28-29: Verify icon usage in security check components.Let's ensure the new icons are properly imported and used in the security check screen components.
Also applies to: 44-44
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usage of new icons in security check related components echo "Searching for icon usage in components..." rg -l "package|seal|selectionSlash" --type ts --type tsxLength of output: 179
🏁 Script executed:
#!/bin/bash echo "Searching for usage of 'selectionSlash' or 'seal' in TS and TSX files..." rg -l -e "(selectionSlash|seal)" -g "*.ts" -g "*.tsx" echo "Searching for references to 'TrezorSuiteIcons.json' in TS and TSX files..." rg -l -e "TrezorSuiteIcons.json" -g "*.ts" -g "*.tsx"Length of output: 678
🏁 Script executed:
#!/bin/bash echo "Displaying icon usage snippet in SecurityCheckScreen.tsx..." rg -C 5 "(selectionSlash|seal)" suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsxLength of output: 774
Security Check Icons Used Correctly
The new icons ("seal" and "selectionSlash") are properly imported and utilized within the SecurityCheckScreen component in suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx. The code snippet confirms that the icons are referenced as intended.
suite-native/atoms/src/AlertBox.tsx (1)
87-87: LGTM! Well-structured component enhancement.The addition of the
textVariantprop with proper typing and default value improves component flexibility while maintaining type safety.Also applies to: 104-104, 128-128
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
69-70: TODO: Implement navigation to Firmware install screen.The TODO comment indicates missing navigation implementation. This should be addressed before merging.
suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx (1)
11-11: LGTM! Clean navigation integration.The SecurityCheckScreen is properly integrated into the navigation stack, following the established patterns.
Also applies to: 40-43
suite-native/navigation/src/routes.ts (1)
35-35: LGTM! Route addition is clean and follows existing pattern.The new
SecurityCheckroute is properly added to theOnboardingStackRoutesenum, maintaining consistency with the existing route naming pattern.suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (2)
84-91: Track TODO comment and consider error handling.The TODO comment about adding the firmware update screen should be tracked. Also, consider adding error handling for the navigation.
Would you like me to:
- Create an issue to track the TODO for adding the firmware update screen?
- Generate error handling code for the navigation?
107-108: Address TODO comment about close button handling.There's an unrelated TODO comment about handling the close button event that should also be tracked.
Would you like me to create an issue to track the TODO for handling the close button event?
suite-native/navigation/src/navigators.ts (1)
125-125: LGTM! The new route is well-placed and properly typed.The addition of the
SecurityCheckroute to theOnboardingStackParamListis appropriate for implementing the security check screen during the onboarding flow.suite-native/intl/src/en.ts (1)
24-25: LGTM! The new button translations follow the established pattern.The additions are consistent with the existing button translations and provide reusable text for common actions.
| export const SecurityCheckScreen = () => { | ||
| const { showToast } = useToast(); | ||
| const [currentStep, setCurrentStep] = useState<number>(1); | ||
|
|
||
| const handlePressConfirmButton = () => { | ||
| if (currentStep < 3) { | ||
| setCurrentStep(currentStep + 1); | ||
|
|
||
| return; | ||
| } | ||
| // TODO: navigate to Firmware install | ||
| showToast({ variant: 'warning', message: 'TODO: implement next screen' }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for edge cases.
The current implementation assumes happy path. Consider adding error handling for:
- Invalid step transitions
- Toast failures
export const SecurityCheckScreen = () => {
const { showToast } = useToast();
- const [currentStep, setCurrentStep] = useState<number>(1);
+ const [currentStep, setCurrentStep] = useState<SecurityCheckStep>(SecurityCheckStep.ResellersCheck);
+ const [error, setError] = useState<Error | null>(null);
const handlePressConfirmButton = () => {
- if (currentStep < 3) {
- setCurrentStep(currentStep + 1);
+ try {
+ if (currentStep < SecurityCheckStep.PackagingCheck) {
+ setCurrentStep(currentStep + 1);
+ return;
+ }
- return;
- }
- // TODO: navigate to Firmware install
- showToast({ variant: 'warning', message: 'TODO: implement next screen' });
+ // TODO: navigate to Firmware install
+ showToast({ variant: 'warning', message: 'TODO: implement next screen' });
+ } catch (err) {
+ setError(err instanceof Error ? err : new Error('Unknown error'));
+ showToast({ variant: 'error', message: 'Failed to proceed to next step' });
+ }
};
| exiting={FadeOutUp} | ||
| > | ||
| <Text variant="highlight">{description}</Text> | ||
| <HStack flex={1} justifyContent="space-between"> |
There was a problem hiding this comment.
Invalid gap between buttons, design says 12px.
| import { DeviceModelInternal } from '@trezor/connect'; | ||
| import { prepareNativeStyle, useNativeStyles } from '@trezor/styles'; | ||
|
|
||
| const trezorImageStyle = prepareNativeStyle<{ isTrezorSafe5: boolean }>((_, { isTrezorSafe5 }) => ({ |
There was a problem hiding this comment.
What about creating two styles instead? You need to pass params, have conditions, you cannot just use deviceModel === DeviceModelInternal.T3T1 on one place.
|
|
||
| const isTrezorSafe5 = deviceModel === DeviceModelInternal.T3T1; | ||
|
|
||
| const knowledgeBaseLink = isTrezorSafe5 |
There was a problem hiding this comment.
Does make sense to just inline this?
| const knowledgeBaseLink = isTrezorSafe5 | |
| const knowledgeBaseLink = deviceModel === DeviceModelInternal.T3T1 |
|
|
||
| const deviceModel = useSelector(selectDeviceModel); | ||
|
|
||
| const isTrezorSafe5 = deviceModel === DeviceModelInternal.T3T1; |
There was a problem hiding this comment.
I'd rename the constant or inline it as shown below.
| const isTrezorSafe5 = deviceModel === DeviceModelInternal.T3T1; | |
| const isModelT3T1 = deviceModel === DeviceModelInternal.T3T1; |
Description
I'm not surebutton and he is redirected to theSuspiciousDeviceScreen, theContact Supportbutton takes him to a specific web link according to the stepRelated Issue
Resolve #16287
Screenshots:
Uploading Screen Recording 2025-02-06 at 3.38.23 PM.mov…