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' });
+ }
};
suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-onboarding/src/components/SecuritySealImages.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-onboarding/src/components/SecuritySealDescription.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-onboarding/src/components/SecuritySealDescription.tsx
Outdated
Show resolved
Hide resolved
PeKne
force-pushed
the
feat/native/security-check-onboarding
branch
from
5594e18 to
9cb8378
Compare
|
🚀 Expo preview is ready!
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
suite-native/module-onboarding/src/components/SecuritySealImages.tsx (1)
23-33: Consider using TypeScript enum for device model comparison.Instead of directly comparing with
DeviceModelInternal.T3T1, consider creating a type-safe enum for supported device models.+enum SupportedDeviceModel { + T3T1 = DeviceModelInternal.T3T1, + T3B1 = DeviceModelInternal.T3B1, +} -if (deviceModel === DeviceModelInternal.T3T1) { +if (deviceModel === SupportedDeviceModel.T3T1) {suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
69-70: Implement navigation to Firmware install screen.The TODO comment indicates missing navigation implementation.
Would you like me to help implement the navigation to the Firmware install screen?
suite-native/module-onboarding/src/components/SecuritySealDescription.tsx (1)
28-30: Add error handling for external link opening.The
openLinkcall should include error handling to manage cases where the URL cannot be opened.const handleLearnMoreButtonPress = () => { - openLink(knowledgeBaseLink); + openLink(knowledgeBaseLink).catch(error => { + console.error('Failed to open knowledge base link:', error); + // Consider showing a user-friendly error message + }); };suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx (2)
47-47: Extract magic numbers to named constants.The animation duration and card height values should be extracted to named constants for better maintainability.
const ANIMATION_DURATION = 300; +const CARD_HEIGHT = { + COLLAPSED: 44, + EXPANDED: 180, +}; const cardStyle = prepareNativeStyle<{ wasOpened: boolean }>((utils, { wasOpened }) => ({ backgroundColor: wasOpened ? utils.colors.backgroundSurfaceElevationNegative : utils.colors.backgroundSurfaceElevation1, })); // Later in the code: - withTiming(isOpened ? 180 : 44, { + withTiming(isOpened ? CARD_HEIGHT.EXPANDED : CARD_HEIGHT.COLLAPSED, {Also applies to: 85-85
81-91: Improve type safety for animation values.Consider using
SharedValuetype from react-native-reanimated for better type safety in the animation style.+import type { SharedValue } from 'react-native-reanimated'; const animatedCardStyle = useAnimatedStyle( - () => ({ + (): { minHeight: number } => ({ minHeight: withDelay( isOpened ? ANIMATION_DURATION : 0, withTiming(isOpened ? 180 : 44, { duration: ANIMATION_DURATION, }), ), }), [isOpened], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
suite-common/icons/cryptoAssets/networkIcons/ada_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol_inverse.svgis excluded by!**/*.svgsuite-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 (18)
- 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 / web-override init-api-flaky
- 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=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (11)
suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx (1)
11-11: LGTM! Security check screen properly integrated.The SecurityCheckScreen is correctly imported and integrated into the navigation stack following the established pattern.
Also applies to: 40-43
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
59-71: Add error handling for edge cases.The current implementation assumes happy path. Consider adding error handling for:
- Invalid step transitions
- Toast failures
suite-native/atoms/src/AlertBox.tsx (1)
87-87: LGTM! Text variant customization properly implemented.The new
textVariantprop is well-implemented with proper typing and default value, maintaining backward compatibility.Also applies to: 104-104, 128-128
suite-native/navigation/src/routes.ts (1)
35-35: LGTM!The addition of
SecurityCheckroute toOnboardingStackRoutesenum is well-placed and aligns with the PR objectives for implementing the onboarding security check screen.suite-native/module-onboarding/src/components/SecuritySealDescription.tsx (1)
23-26: Consider simplifying the device model check.The conditional check can be simplified for better readability.
- const knowledgeBaseLink = - deviceModel === DeviceModelInternal.T3T1 - ? HELP_CENTER_PACKAGING_T3T1_URL - : HELP_CENTER_PACKAGING_T3B1_URL; + const knowledgeBaseLink = deviceModel === DeviceModelInternal.T3T1 + ? HELP_CENTER_PACKAGING_T3T1_URL + : HELP_CENTER_PACKAGING_T3B1_URL;suite-common/icons/generateIconFont.ts (1)
78-78: LGTM!The addition of
package,seal, andselectionSlashicons is appropriate for the security check screen implementation.Also applies to: 93-94
suite-native/navigation/src/navigators.ts (1)
125-125: LGTM! Route definition follows existing patterns.The SecurityCheck route is properly integrated into the OnboardingStackParamList.
suite-native/intl/src/en.ts (2)
24-25: LGTM! Generic button translations added.The new button translations are clear and reusable across the application.
858-885: LGTM! Security check translations are comprehensive.The translations for the security check screen are well-structured with:
- Clear step headers and descriptions
- Proper link placeholders for dynamic content
- Informative modal content for the holographic seal
suite-native/module-onboarding/package.json (1)
27-27: Verify the react-native-reanimated version.Let's ensure we're using the latest stable version of react-native-reanimated that's compatible with the current React Native version (0.76.1).
✅ Verification successful
🌐 Web query:
What is the latest stable version of react-native-reanimated that's compatible with React Native 0.76.1?💡 Result:
The latest stable version of React Native Reanimated compatible with React Native 0.76.x is 3.16.x, specifically 3.16.7 or newer[2][10]. This version supports both React Native's legacy ("Paper") and New Architecture ("Fabric") modes[10].
Key details:
- Reanimated ≥3.16.x maintains full compatibility with React Native 0.75-0.77 when using Fabric[2]
- Version 3.16.x introduced critical fixes for:
- NativeWind support restoration[1]
- Web compatibility improvements[1]
- Warning message cleanup[1]
- For projects using Expo SDK 52 with React Native 0.76, explicit installation of
react-native-reanimated@~3.16.xis required[3]Installation command:
npm install react-native-reanimated@~3.16.x # or yarn add react-native-reanimated@~3.16.xAvoid versions below 3.16.x due to known compatibility issues with React Native 0.76[5][8]. The next major release (4.x) only supports React Native's New Architecture and requires dropping legacy architecture support[2][9].
Citations:
- 1: https://github.com/software-mansion/react-native-reanimated/releases
- 2: https://docs.swmansion.com/react-native-reanimated/docs/next/guides/compatibility/
- 3: https://expo.dev/changelog/2025/01-21-react-native-0.77
- 4: https://snyk.io/advisor/npm-package/react-native-reanimated
- 5: React Native 0.76 is not supported software-mansion/react-native-reanimated#6574
- 6: https://reactnative.dev/versions
- 7: https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/getting-started/
- 8: React Native Reanimated is not compatible with React Native 0.76 software-mansion/react-native-reanimated#6475
- 9: https://reactnative.dev/blog/2024/10/23/the-new-architecture-is-here
- 10: https://docs.swmansion.com/react-native-reanimated/docs/guides/compatibility/
React Native Reanimated version verified as up-to-date.
The dependency"react-native-reanimated": "^3.16.7"is in line with the latest stable version (3.16.x) that is compatible with React Native 0.76.1.suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
1-109: LGTM! The icon mappings are well-structured.The file maintains consistent naming conventions and sequential code points, which is good for maintainability.
suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx
Show resolved
Hide resolved
|
@yanascz the animation was fixed for iOS. Now it should look identical on both platforms. |
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13281870816 |
trezor-ci
force-pushed
the
feat/native/security-check-onboarding
branch
from
9cb8378 to
af85952
Compare
WalkthroughThe pull request introduces several changes across multiple modules. In the icon generation files, three new icon names are appended to the list for generating the mobile icon font, and the JSON file is updated by removing certain icon entries and inserting new ones with reassigned identifiers. In the React Native atoms, the AlertBox component now accepts a new property to customize the text variant, with the necessary type imported. The English translations have been extended with two new button labels and a complete security check screen definition, including steps and descriptions. A new dependency for react-native-reanimated is added to the module-onboarding package file. Several new UI components are introduced, including SecurityCheckStepCard, SecuritySealDescription, and SecuritySealImages, along with a new security check screen. Additionally, the navigation structure is updated by adding a new route to the onboarding stack, and conditional logic in the device landing screen is modified to integrate the security check process. ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
69-71: Implement navigation to firmware install screen.The TODO comment indicates missing navigation implementation. Consider implementing the navigation to the firmware install screen or creating a tracking issue.
Would you like me to help implement the navigation logic or create an issue to track this task?
suite-native/module-onboarding/src/components/SecuritySealDescription.tsx (1)
23-26: Consider using a type-safe mapping for knowledge base links.The conditional assignment of
knowledgeBaseLinkcould be made more maintainable using a type-safe mapping object.- const knowledgeBaseLink = - deviceModel === DeviceModelInternal.T3T1 - ? HELP_CENTER_PACKAGING_T3T1_URL - : HELP_CENTER_PACKAGING_T3B1_URL; + const KNOWLEDGE_BASE_LINKS: Record<DeviceModelInternal, string> = { + [DeviceModelInternal.T3T1]: HELP_CENTER_PACKAGING_T3T1_URL, + [DeviceModelInternal.T3B1]: HELP_CENTER_PACKAGING_T3B1_URL, + }; + const knowledgeBaseLink = KNOWLEDGE_BASE_LINKS[deviceModel];suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx (1)
73-77: Consider adding error handling for navigation.The navigation function should handle potential navigation failures gracefully.
const navigateToSuspiciousDeviceScreen = () => { + try { navigation.navigate(OnboardingStackRoutes.SuspiciousDevice, { suspicionCause, }); + } catch (error) { + console.error('Navigation failed:', error); + // Consider showing a fallback UI or error message + } };suite-native/module-onboarding/src/screens/UninitializedDeviceLandingScreen.tsx (2)
83-92: Address the firmware update screen TODO before release.The TODO comment indicates a missing feature in the onboarding flow. This should be addressed before releasing to production.
Would you like me to help create a new issue to track the implementation of the firmware update screen in the onboarding flow?
107-108: Address the TODO for close button handling.The TODO comment about handling the close button event needs to be addressed for proper navigation flow.
Would you like me to help implement the close button event handler?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
suite-common/icons/cryptoAssets/networkIcons/ada_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol_inverse.svgis excluded by!**/*.svgsuite-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 (16)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (14)
suite-native/module-onboarding/src/navigation/OnboardingStackNavigator.tsx (1)
11-11: LGTM! Clean integration of the security check screen.The SecurityCheckScreen is properly integrated into the navigation stack, following the established patterns.
Also applies to: 40-43
suite-native/module-onboarding/src/components/SecuritySealImages.tsx (2)
26-30: Add accessibility labels to images.For better accessibility, add descriptive alt text to the images.
Also applies to: 38-42, 43-47
9-17: LGTM! Clean implementation of device-specific UI.The component correctly handles different device models with separate styles and appropriate conditional rendering.
Also applies to: 19-59
suite-native/module-onboarding/src/screens/SecurityCheckScreen.tsx (1)
14-57: Use TypeScript enum for step management.Replace magic numbers with a type-safe enum for better maintainability.
suite-native/atoms/src/AlertBox.tsx (1)
87-87: LGTM! Clean implementation of text variant customization.The textVariant prop is properly typed and integrated, enhancing the component's flexibility while maintaining type safety.
Also applies to: 104-104, 128-128
suite-native/navigation/src/routes.ts (1)
35-35: LGTM!The new
SecurityCheckroute is properly added to theOnboardingStackRoutesenum, maintaining consistency with the existing route naming pattern.suite-native/module-onboarding/src/components/SecuritySealDescription.tsx (1)
50-92: LGTM! Well-structured bottom sheet implementation.The bottom sheet implementation is clean and follows good practices:
- Proper use of nested VStack components for layout
- Clear separation of content sections
- Consistent button styling and positioning
suite-native/module-onboarding/src/components/SecurityCheckStepCard.tsx (1)
81-91: LGTM! Clean animation implementation.The animation logic is well-implemented:
- Uses
useAnimatedStylefor performance- Has appropriate delay and duration settings
- Smooth height transition between states
suite-native/navigation/src/navigators.ts (1)
125-125: LGTM!The route definition for the security check screen is correctly added to the
OnboardingStackParamListtype.suite-native/intl/src/en.ts (2)
24-25: LGTM!The new generic button translations are properly added and follow the existing pattern.
858-885: LGTM!The security check screen translations are well-structured and comprehensive:
- Clear hierarchy with title, subtitle, and steps
- Proper use of rich text formatting for links
- Consistent with the PR objectives for security check implementation
suite-native/module-onboarding/package.json (1)
27-27: Verify the react-native-reanimated version.The dependency is added with a caret version range. Let's verify if this is the latest stable version compatible with the project's React Native version (0.76.1).
✅ Verification successful
🌐 Web query:
What is the latest stable version of react-native-reanimated that is compatible with React Native 0.76.1?💡 Result:
The latest stable version of
react-native-reanimatedcompatible with React Native 0.76.x isreact-native-reanimated@~3.16.x, specifically version3.16.7or later[2][9].Key Details:
- Compatibility Confirmation:
The official compatibility table shows[email protected]supports React Native 0.76 under both the old ("Paper") and new ("Fabric") architectures[2][9].- Stability:
- Version
3.xis actively maintained[4].- Version
3.xincludes critical fixes for React Native 0.x compatibility (e.g., memory management improvements and layout animation stability)[1][9].Installation Instructions:
yarn add react-native-reanimated@~3.x # or npm install react-native-reanimated@~3.xFor Expo projects using React Native 0.x, ensure
react-native-reanimated@~xaligns with your SDK’s recommended dependencies[3][7].
Note: If upgrading from older versions of Reanimated (e.g.,
2.x), follow migration guides to address breaking changes[9].Citations:
- 1: https://github.com/software-mansion/react-native-reanimated/releases
- 2: https://docs.swmansion.com/react-native-reanimated/docs/next/guides/compatibility/
- 3: https://expo.dev/changelog/2025/01-21-react-native-0.77
- 4: https://snyk.io/advisor/npm-package/react-native-reanimated
- 5: React Native 0.76 is not supported software-mansion/react-native-reanimated#6574
- 6: https://reactnavigation.org/docs/5.x/supported-react-native-versions/
- 7: https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/getting-started/
- 8: React Native Reanimated is not compatible with React Native 0.76 software-mansion/react-native-reanimated#6475
- 9: https://docs.swmansion.com/react-native-reanimated/docs/guides/compatibility/
- 10: https://www.npmjs.com/package/react-native-reanimated/v/1.2.0?activeTab=versions
React Native Reanimated Version is Correct
- The latest stable version for React Native 0.76.x is confirmed to be
react-native-reanimated@~3.16.x, specifically version 3.16.7.- The project’s dependency
"react-native-reanimated": "^3.16.7"aligns with this, ensuring compatibility with React Native 0.76.1.suite-common/icons/generateIconFont.ts (1)
78-94: LGTM! The new icons align with the PR objectives.The addition of
package,seal, andselectionSlashicons is consistent with the security check screen feature being implemented.suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
30-108:⚠️ Potential issue
⚠️ Icon ID reassignment may cause breaking changes.The extensive reordering and reassignment of icon IDs could break existing references in the codebase. For example, icons like "question", "qrCode", and "prohibit" have been assigned new IDs.
Run the following script to verify the impact of icon ID changes:
Additionally, based on past review comments, these icons are still actively used in several files:
suite-common/icons-deprecated/src/icons.tssuite-native/module-device-settings/src/screens/FirmwareUpdateScreen/FirmwareChangelogButton.tsxsuite-native/module-send/src/components/AddressReviewHelpSheet.tsxsuite-native/module-send/src/components/QrCodeBottomSheetIcon.tsxPlease ensure all icon references are updated to match the new IDs or consider preserving the original IDs to maintain backward compatibility.
❌ Incorrect review comment
🏁 Script executed:
#!/bin/bash # Description: Search for direct references to icon IDs in the codebase. echo "Searching for hardcoded icon IDs..." rg -i "61725|61726|61727" --type ts --type tsxLength of output: 157
Let's try a revised search—using glob patterns for both
.tsand.tsxfiles—to see if there are hardcoded references to these icon IDs:Once we have the output from this search, we can verify whether these icons' IDs are still hardcoded elsewhere and assess the risk of breaking changes due to the reassignment.
🏁 Script executed:
#!/bin/bash echo "Searching for hardcoded icon IDs in .ts and .tsx files..." rg -i "61725|61726|61727" --glob '*.ts' --glob '*.tsx'Length of output: 941
⚠️ No evidence of breakage from icon ID reassignments detected.Our updated search for the numeric IDs (61725, 61726, 61727) in TypeScript and TSX files did not locate any hardcoded references. Although previous comments mentioned potential issues in files like:
suite-common/icons-deprecated/src/icons.tssuite-native/module-device-settings/src/screens/FirmwareUpdateScreen/FirmwareChangelogButton.tsxsuite-native/module-send/src/components/AddressReviewHelpSheet.tsxsuite-native/module-send/src/components/QrCodeBottomSheetIcon.tsxthe current grep results show no direct usage of those numeric IDs that would signal a breakage. This suggests that the icon references are most likely made via their symbolic keys or via a mapping that has been updated consistently.
Based on this verification, the reviewed concern about potential breaking changes from icon ID reordering appears to be unfounded in the current codebase.
Likely an incorrect or invalid review comment.
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…