feat(suite-native): Mobile Trade: Legal notice and safety #17011
Conversation
WalkthroughThe pull request implements several updates across the trading module. A new legal section is added to the translations, providing detailed legal and security information related to cryptocurrency transactions. Two new UI components, Confirmation and LegalSheet, have been introduced to manage user confirmation and display legal details through a bottom sheet interface. To support these components, new hooks such as useBottomSheetControls and useTradeConfirmationControls have been added, and the existing useTradeSheetControls hook has been refactored to delegate visibility management. A test suite has also been included to ensure the proper behavior of the bottom sheet controls. Additionally, the TradingScreen component has been updated to replace a placeholder button with the new Confirmation component, adjusting the user interaction flow in the trading environment. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
suite-native/module-trading/src/hooks/useBottomSheetControls.ts (1)
3-19: LGTM! Consider adding animation state.The implementation is clean and follows React best practices. Consider enhancing it with animation state management for smoother transitions.
Add animation state management:
export const useBottomSheetControls = () => { const [isSheetVisible, setIsSheetVisible] = useState(false); + const [isAnimating, setIsAnimating] = useState(false); const showSheet = useCallback(() => { + setIsAnimating(true); setIsSheetVisible(true); }, []); const hideSheet = useCallback(() => { + setIsAnimating(true); setIsSheetVisible(false); }, []); + const onAnimationComplete = useCallback(() => { + setIsAnimating(false); + }, []); return { isSheetVisible, + isAnimating, showSheet, hideSheet, + onAnimationComplete, }; };suite-native/module-trading/src/components/buy/Confirmation.tsx (2)
13-15: Add accessibility attributes to the button.Enhance the button's accessibility by adding appropriate ARIA attributes and role.
- <Button onPress={showSheet} disabled={!isConfirmationEnabled}> + <Button + onPress={showSheet} + disabled={!isConfirmationEnabled} + accessibilityRole="button" + accessibilityLabel={<Translation id="moduleTrading.tradingScreen.continueButtonA11yLabel" />} + accessibilityHint={<Translation id="moduleTrading.tradingScreen.continueButtonA11yHint" />} + >
7-23: Add error handling for edge cases.Consider handling edge cases such as:
- Network errors when loading provider data
- State transitions during sheet visibility changes
- Disabled state feedback
export const Confirmation = () => { const { isConfirmationEnabled, showSheet, hideSheet, isSheetVisible, tradeProviderName } = useTradeConfirmationControls(); + const [error, setError] = useState<Error | null>(null); + + const handleShowSheet = async () => { + try { + await showSheet(); + } catch (err) { + setError(err instanceof Error ? err : new Error('Failed to show sheet')); + } + }; return ( <> + {error && ( + <Text variant="error"> + <Translation + id="moduleTrading.tradingScreen.error" + values={{ message: error.message }} + /> + </Text> + )} <Button - onPress={showSheet} + onPress={handleShowSheet} disabled={!isConfirmationEnabled} > <Translation id="moduleTrading.tradingScreen.continueButton" /> </Button>suite-native/module-trading/src/hooks/__tests__/useBottomSheetControls.test.ts (1)
5-34: Test coverage looks good but could be enhanced.The tests effectively cover the basic functionality of the bottom sheet controls:
- Default state
- Show sheet
- Hide sheet after show
Consider adding tests for:
- Error handling scenarios
- Edge cases like multiple show/hide calls
- State persistence between renders
suite-native/module-trading/src/components/buy/LegalSheet.tsx (2)
12-16: Consider enhancing accessibility for helper components.The Subheader and Info components should include proper accessibility attributes for screen readers.
const Subheader = ({ children }: { children: ReactNode }) => ( - <Text variant="highlight" color="textDefault"> + <Text variant="highlight" color="textDefault" accessibilityRole="header"> {children} </Text> ); const Info = ({ children }: { children: ReactNode }) => ( - <HStack> + <HStack accessibilityRole="text"> <Box flex={0}> <Text>{'\u2022'}</Text> </Box>Also applies to: 18-29
31-82: Well-structured legal notice implementation.The component effectively organizes legal information into distinct sections with proper spacing and visual hierarchy. The use of translation keys ensures proper internationalization.
A few suggestions for improvement:
- Consider adding a loading state for the sheet
- Add error boundaries for translation key failures
- Consider adding analytics for user interactions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/src/components/buy/Confirmation.tsx(1 hunks)suite-native/module-trading/src/components/buy/LegalSheet.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useBottomSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/useBottomSheetControls.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeConfirmationControls.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
suite-native/module-trading/src/screens/TradingScreen.tsx (1)
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:31-43
Timestamp: 2025-02-04T19:21:55.211Z
Learning: In React Native components, when implementing visual stubs or UI mockups, it's acceptable to use inline hardcoded values that will be replaced with real data in future commits. These temporary values don't need to be extracted into constants.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
🔇 Additional comments (4)
suite-native/module-trading/src/hooks/useTradeSheetControls.ts (1)
5-14: LGTM! Well-structured refactoring.The refactoring improves code organization by delegating visibility management to
useBottomSheetControlswhile maintaining type safety through generics.suite-native/module-trading/src/screens/TradingScreen.tsx (1)
1-1: LGTM! Good improvement to the trading screen.The changes enhance the UI by replacing the placeholder button with a proper legal confirmation component, which aligns well with the PR's objectives of improving legal compliance and safety features.
Also applies to: 7-7, 19-19
suite-native/module-trading/src/components/buy/LegalSheet.tsx (1)
6-10: Props interface is well-defined.The props interface clearly defines the required properties and includes proper typing for the optional tradeProviderName.
suite-native/intl/src/en.ts (1)
1359-1376: Legal translations are well-structured but require legal review.The translations effectively cover important legal aspects:
- Security warnings
- Partner relationships
- Legal notices
- Transaction finality
- Regulatory compliance
However, these translations contain important legal statements that should be reviewed by legal counsel to ensure accuracy and compliance across jurisdictions.
Please ensure:
- Legal team has reviewed and approved all legal statements
- Translations maintain legal meaning when localized to other languages
- Compliance with financial regulations in target markets
|
🚀 Expo preview is ready!
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/module-trading/src/components/buy/LegalSheet.tsx (2)
12-22: Consider enhancing helper components with explicit prop types.While the components are well-structured, consider these improvements:
- Define explicit prop types interfaces for better maintainability
- Consider moving to separate files if these components are reused elsewhere
+interface SubheaderProps { + children: ReactNode; +} -const Subheader = ({ children }: { children: ReactNode }) => ( +const Subheader = ({ children }: SubheaderProps) => ( +interface InfoProps { + children: ReactNode; +} -const Info = ({ children }: { children: ReactNode }) => ( +const Info = ({ children }: InfoProps) => (
31-73: Consider enhancing accessibility and user experience.The legal information is well-structured, but consider these improvements:
- Add aria-labels for better screen reader support
- Consider adding a "Read more" or expand/collapse functionality for long text
- <VStack spacing="sp12" paddingHorizontal="sp12"> + <VStack + spacing="sp12" + paddingHorizontal="sp12" + role="region" + aria-label={<Translation id="moduleTrading.legalSheet.ariaLabel" />} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/module-trading/src/components/buy/LegalSheet.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
suite-native/module-trading/src/components/buy/LegalSheet.tsx (2)
1-10: LGTM! Well-structured imports and type definitions.The imports are appropriate, and the
LegalSheetPropstype is well-defined with clear purpose.
24-75:❓ Verification inconclusive
Verify the translation keys and button behavior.
The component structure is clean and well-organized. However, there are a few points to verify:
- The continue button uses a translation key from
tradingScreeninstead oflegalSheet- The
collapsableprop's behavior should be documentedLet's verify the translation keys are properly defined:
🏁 Script executed:
#!/bin/bash # Check if all translation keys are defined rg -A 1 "moduleTrading\.legalSheet\." suite-native/intl/src/ rg "moduleTrading\.tradingScreen\.continueButton" suite-native/intl/src/Length of output: 137
Action Required: Confirm Translation Keys and Document
collapsableBehaviorThe
LegalSheetcomponent is cleanly structured; however, two issues need your attention:
- Translation Key Verification: The continue button uses the key
moduleTrading.tradingScreen.continueButtonwhile other keys come frommoduleTrading.legalSheet.*. Please manually verify that this discrepancy is intentional and that the corresponding key is correctly defined in your internationalization files.collapsableProp Documentation: The behavior of thecollapsableprop on theBottomSheetcomponent isn’t immediately clear. Consider adding documentation (or verifying existing documentation) to clarify its intended functionality.Please review these points to ensure consistency and clarity in the component behavior.
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13333513654 |
trezor-ci
force-pushed
the
16993-Mobile-Trade-Legal-notice-and-safety
branch
from
dd23d05 to
9e29731
Compare
Description
Related Issue
Resolve #16993
Screenshots: