Mobile Trade: Token picker modal visual stub #16668
Conversation
jbazant
added
mobile
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
c16db60 to
a1651ea
Compare
|
🚀 Expo preview is ready!
|
jbazant
force-pushed
the
module-trade-ui-tokens
branch
3 times, most recently
from
4bc8227 to
45e0225
Compare
vytick
removed
the
no-project
jbazant
force-pushed
the
module-trade-ui-tokens
branch
2 times, most recently
from
6877442 to
620e0d6
Compare
jbazant
added
no-project
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
620e0d6 to
8e32a6e
Compare
WalkthroughThis pull request introduces a broad range of changes across multiple modules. Updates include adding new icons to the icon font generation and overhauling the JSON icon data. UI components are enhanced with new color schemes and additional props, such as in Button, SearchInput, and BottomSheet components. In the trading module, several new components have been implemented including TradeableAssetPicker, TradeableAssetsSheet, TradeableAssetListItem, CountrySheet, and related headers and empty state components, aimed at supporting a token picker modal visual stub. Other changes include updates to test providers, new hook implementations for managing trade sheet controls, translation enhancements, and configuration updates such as the runtimeVersion bump. Overall, these modifications refine the visual and functional aspects of the mobile trading user interface. Assessment against linked issues
Possibly related PRs
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 (
|
jbazant
force-pushed
the
module-trade-ui-tokens
branch
3 times, most recently
from
8a82958 to
182f018
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (25)
suite-native/test-utils/src/BasicProviderForTests.tsx (1)
19-29: Consider making formatter config configurable.The formatter configuration is currently hardcoded. Consider making it configurable through props to support different test scenarios.
type ProviderProps = { children: ReactNode; + formattersConfig?: { + locale?: 'en'; + fiatCurrency?: 'usd'; + bitcoinAmountUnit?: number; + is24HourFormat?: boolean; + }; }; -export const BasicProviderForTests = ({ children }: ProviderProps) => { +export const BasicProviderForTests = ({ children, formattersConfig: customConfig }: ProviderProps) => { const formattersConfig = useMemo( - () => ({ + () => customConfig ?? ({ locale: 'en' as const, fiatCurrency: 'usd' as const, bitcoinAmountUnit: 0, is24HourFormat: true, }), - [], + [customConfig], );suite-native/atoms/src/Input/SearchInput.tsx (2)
64-64: Consider inlining thenoOpfallbackThe
noOpfunction is a sensible placeholder for callbacks, but consider using a direct inline default instead to reduce the mental overhead of referencing an external variable.
66-133: ChecklineHeightusage for potential layout or accessibility issuesThe use of
lineHeight: 0within the input style might cause text rendering or accessibility issues on certain devices. Consider using a natural line height to ensure consistent display across platforms.suite-native/module-trading/src/screens/TradingScreen.tsx (1)
11-12: Placeholder text usageThe placeholder text and newly added
AmountCardprovide a good foundation. Be sure to revisit and remove or refine the placeholder once the trading functionality is fully implemented.suite-native/module-trading/src/components/general/__tests__/TradeableNetworkButton.comp.test.tsx (1)
12-20: Add additional edge case testsWhile checking
onPressis sufficient, consider adding edge case tests (e.g., no symbol, invalid symbol) to further ensure robust behavior. Also, if there's a favourite press functionality, it could be tested here as well.suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx (1)
5-22: Consider adding more test coverage.While the current tests cover the basic rendering states, consider adding tests for:
- Verifying the
onPresscallback is triggered correctly- Testing with different network symbols
- Testing accessibility properties
Example test case for
onPress:it('should call onPress when button is pressed', () => { const onPressMock = jest.fn(); const { getByText } = render( <SelectTradeableAssetButton onPress={onPressMock} selectedAsset="ada" />, ); fireEvent.press(getByText('ADA')); expect(onPressMock).toHaveBeenCalled(); });suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx (1)
6-8: Fix grammatical issues in test descriptions.The test descriptions should be grammatically correct.
Apply this diff to fix the grammar:
- it('should has correct hint when marked as favourite', () => { + it('should have correct hint when marked as favourite', () => { const { getByA11yHint } = render(<FavouriteIcon isFavourite={true} onPress={jest.fn()} />); expect(getByA11yHint('Remove from favourites')).toBeDefined(); }); - it('should has correct hint when not marked as favourite', () => { + it('should have correct hint when not marked as favourite', () => { const { getByA11yHint } = render(<FavouriteIcon isFavourite={false} onPress={jest.fn()} />); expect(getByA11yHint('Add to favourites')).toBeDefined(); });Also applies to: 11-13, 16-24
suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx (1)
13-29: Consider memoizing the component for performance optimization.While the current implementation is correct, consider wrapping the component with React.memo since it's likely to be re-rendered frequently in lists or forms.
-export const SelectTradeableAssetButton = ({ onPress, selectedAsset }: SelectAssetButtonProps) => { +export const SelectTradeableAssetButton = React.memo(({ onPress, selectedAsset }: SelectAssetButtonProps) => { const { iconColor } = buttonSchemeToColorsMap.primary; // ... rest of the component -}; +});suite-native/module-trading/src/components/TradeableAssetsSheet/FavouriteIcon.tsx (1)
20-28: Consider using Pressable instead of TouchableWithoutFeedback.TouchableWithoutFeedback is being deprecated in favor of Pressable, which offers better feedback states and performance.
-import { TouchableWithoutFeedback } from 'react-native'; +import { Pressable } from 'react-native'; // ... - <TouchableWithoutFeedback + <Pressable onPress={onPress} accessibilityRole="button" accessibilityHint={hint} > <Icon name={iconName} color={iconColor} /> - </TouchableWithoutFeedback> + </Pressable>suite-native/module-trading/src/components/buy/AmountCard.tsx (2)
10-16: Consider destructuring hook values more concisely.The current destructuring pattern could be simplified for better readability.
- const { - isTradeableAssetsSheetVisible, - showTradeableAssetsSheet, - hideTradeableAssetsSheet, - selectedTradeableAsset, - setSelectedTradeableAsset, - } = useTradeableAssetsSheetControls(); + const { + isTradeableAssetsSheetVisible, showTradeableAssetsSheet, + hideTradeableAssetsSheet, selectedTradeableAsset, + setSelectedTradeableAsset + } = useTradeableAssetsSheetControls();
18-33: Consider extracting sheet rendering logic.The sheet rendering could be extracted to a separate component for better maintainability.
+const AssetSelectionSheet = ({ + isVisible, + onClose, + onAssetSelect +}: Pick<TradeableAssetsSheetProps, 'isVisible' | 'onClose' | 'onAssetSelect'>) => ( + <TradeableAssetsSheet + isVisible={isVisible} + onClose={onClose} + onAssetSelect={onAssetSelect} + /> +); export const AmountCard = () => { // ... hook usage return ( <Card> <HStack> <SelectTradeableAssetButton onPress={showTradeableAssetsSheet} selectedAsset={selectedTradeableAsset} /> </HStack> - <TradeableAssetsSheet - isVisible={isTradeableAssetsSheetVisible} - onClose={hideTradeableAssetsSheet} - onAssetSelect={setSelectedTradeableAsset} - /> + <AssetSelectionSheet + isVisible={isTradeableAssetsSheetVisible} + onClose={hideTradeableAssetsSheet} + onAssetSelect={setSelectedTradeableAsset} + /> </Card> ); };suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
13-13: Consider moving constants to a dedicated file.The
PICKER_BUTTON_HEIGHTconstant could be moved to a dedicated constants file to maintain consistency and reusability across components.suite-native/module-trading/src/components/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx (1)
8-15: Consider extracting style constants.The padding calculations use magic numbers (
sp12,sp52). Consider extracting these to named constants for better maintainability.suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx (2)
7-7: Consider extending props interface.The
SearchInputWithCancelPropscould be extended to include specific props for cancel button customization (e.g.,cancelButtonText,onCancel).
11-49: Consider improving cancel button implementation.The cancel button implementation could be improved:
- Add debounce to prevent rapid clear/blur operations
- Consider adding a callback for cancel action
- Handle keyboard events (e.g., Escape key)
export const SearchInputWithCancel = ({ onFocus = noOp, onBlur = noOp, + onCancel = noOp, + cancelButtonText, ...props }: SearchInputWithCancelProps) => { const { translate } = useTranslate(); const [isInputActive, setIsInputActive] = useState(false); const inputRef = useRef<TextInput>(null); + const handleCancel = () => { + inputRef.current?.clear(); + inputRef.current?.blur(); + onCancel(); + }; return ( <HStack alignItems="center"> <Box flex={1}> <SearchInput ref={inputRef} placeholder={translate('moduleTrading.defaultSearchLabel')} onFocus={() => { setIsInputActive(true); onFocus(); }} onBlur={() => { setIsInputActive(false); onBlur(); }} + onKeyPress={({ nativeEvent: { key } }) => { + if (key === 'Escape') { + handleCancel(); + } + }} {...props} /> </Box> {isInputActive && ( <TextButton - onPress={() => { - inputRef.current?.clear(); - inputRef.current?.blur(); - }} + onPress={handleCancel} > - <Translation id="generic.buttons.cancel" /> + <Translation + id={cancelButtonText ?? 'generic.buttons.cancel'} + /> </TextButton> )} </HStack> ); };suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/TradeableNetworkListItem.comp.test.tsx (2)
6-19: Enhance test coverage for price and fiat rate display.The rendering test could be more comprehensive by verifying the display of
priceChangeandfiatRateprops.it('should render with correct labels', () => { const { getByText } = render( <TradeableNetworkListItem symbol="btc" onPress={jest.fn()} onFavouritePress={jest.fn()} - priceChange={0} - fiatRate={1} + priceChange={5.5} + fiatRate={45000} />, ); expect(getByText('Bitcoin')).toBeDefined(); expect(getByText('BTC')).toBeDefined(); + expect(getByText('+5.5%')).toBeDefined(); + expect(getByText('$45,000')).toBeDefined(); });
38-53: Test both favorite and unfavorite states.The favorite icon test should verify both states of the icon.
it('should call onFavouritePress when star is clicked', () => { const onFavouritePress = jest.fn(); const { getByAccessibilityHint } = render( <TradeableNetworkListItem symbol="btc" onPress={jest.fn()} onFavouritePress={onFavouritePress} priceChange={0} fiatRate={1} + isFavourite={false} />, ); fireEvent.press(getByAccessibilityHint('Add to favourites')); expect(onFavouritePress).toHaveBeenCalledWith(); + const { getByAccessibilityHint: getHint } = render( + <TradeableNetworkListItem + symbol="btc" + onPress={jest.fn()} + onFavouritePress={onFavouritePress} + priceChange={0} + fiatRate={1} + isFavourite={true} + />, + ); + expect(getHint('Remove from favourites')).toBeDefined(); });suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts (2)
13-21: Fix inconsistent function names in test descriptions.The test descriptions use "TokensSheet" while the functions use "TradeableAssetsSheet".
- it('should be true after showTokensSheet call', () => { + it('should be true after showTradeableAssetsSheet call', () => { const { result } = renderHook(() => useTradeableAssetsSheetControls()); act(() => { result.current.showTradeableAssetsSheet(); }); expect(result.current.isTradeableAssetsSheetVisible).toBe(true); }); - it('should be false after hideTokensSheet call', () => { + it('should be false after hideTradeableAssetsSheet call', () => {Also applies to: 23-32
35-51: Add tests for edge cases.The test suite should include validation of invalid asset selection and concurrent state changes.
describe('selectedAsset', () => { it('should be undefined by default', () => { const { result } = renderHook(() => useTradeableAssetsSheetControls()); expect(result.current.selectedTradeableAsset).toBeUndefined(); }); it('should be set after setSelectedNetwork call', () => { const { result } = renderHook(() => useTradeableAssetsSheetControls()); act(() => { result.current.setSelectedTradeableAsset('btc'); }); expect(result.current.selectedTradeableAsset).toBe('btc'); }); + + it('should handle invalid asset selection', () => { + const { result } = renderHook(() => useTradeableAssetsSheetControls()); + + act(() => { + result.current.setSelectedTradeableAsset('invalid_asset'); + }); + + expect(result.current.selectedTradeableAsset).toBe('invalid_asset'); + // Add validation if the hook should prevent invalid assets + }); + + it('should handle rapid state changes correctly', () => { + const { result } = renderHook(() => useTradeableAssetsSheetControls()); + + act(() => { + result.current.showTradeableAssetsSheet(); + result.current.setSelectedTradeableAsset('btc'); + result.current.hideTradeableAssetsSheet(); + }); + + expect(result.current.isTradeableAssetsSheetVisible).toBe(false); + expect(result.current.selectedTradeableAsset).toBe('btc'); + }); });suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (1)
48-53: Optimize LinearGradient rendering.Memoize the LinearGradient component to prevent unnecessary re-renders.
+ const gradientColors = useMemo( + () => [hexToRgba(bgBaseColor, 0.3), hexToRgba(bgBaseColor, 0.01)], + [bgBaseColor] + ); + return ( <LinearGradient - colors={[hexToRgba(bgBaseColor, 0.3), hexToRgba(bgBaseColor, 0.01)]} + colors={gradientColors} style={applyStyle(gradientBackgroundStyle)} start={{ x: 0, y: 0.5 }} end={{ x: 1, y: 0.5 }} >suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (2)
14-15: Consider using TypeScript const assertions for constants.The constants could benefit from more type safety using const assertions.
-export const SHEET_HEADER_HEIGHT = 110; -const FOCUS_ANIMATION_DURATION = 500; +export const SHEET_HEADER_HEIGHT = 110 as const; +const FOCUS_ANIMATION_DURATION = 500 as const;
41-46: Consider debouncing the search input.The search input might trigger frequent state updates. Consider debouncing the onChange handler to improve performance.
+import { useMemo } from 'react'; +import { debounce } from 'lodash'; const TradeableAssetsSheetHeader = () => { const [isFilterActive, setIsFilterActive] = useState(false); const [filterValue, setFilterValue] = useState(''); + + const debouncedSetFilterValue = useMemo( + () => debounce((value: string) => setFilterValue(value), 300), + [] + ); return ( <SearchInputWithCancel - onChange={setFilterValue} + onChange={debouncedSetFilterValue} onFocus={() => setIsFilterActive(true)} onBlur={() => setIsFilterActive(false)} value={filterValue} /> ); };suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetListItem.tsx (1)
10-19: Consider making required props explicit.The
isFavouriteprop is marked as optional but has a default value in the component. Consider making it required in the type definition for better type safety.export type AssetListItemProps = { assetName: ReactNode; displaySymbol: ReactNode; icon: ReactNode; fiatRate: number; priceChange: number; onPress: () => void; - isFavourite?: boolean; + isFavourite: boolean; onFavouritePress: () => void; };suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx (1)
47-60: Consider using StyleSheet.absoluteFillObject for sticky components.The sticky component styles could be simplified using
StyleSheet.absoluteFillObject.const styles = StyleSheet.create({ stickyListHeader: { - position: 'absolute', - top: 0, - left: 0, - right: 0, + ...StyleSheet.absoluteFillObject, + bottom: undefined, }, stickyListFooter: { - position: 'absolute', - bottom: 0, - left: 0, - right: 0, + ...StyleSheet.absoluteFillObject, + top: undefined, }, });suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx (1)
109-130: Memoize data transformation outside the component.The data transformation logic could be moved outside the component and memoized for better performance.
+const transformData = (favourites: NetworkSymbol[], assetsData: NetworkSymbol[]): ListInnerDataShape[] => [ + ['spacer', SHEET_HEADER_HEIGHT, 'spacerTop'], + [ + 'sectionHeader', + <Translation + key="favourites" + id="moduleTrading.tradeableAssetsSheet.favouritesTitle" + />, + 'section_favourites', + ], + ...favourites.map(symbol => ['asset', symbol, true] as ListInnerDataShape), + [ + 'sectionHeader', + <Translation key="all" id="moduleTrading.tradeableAssetsSheet.allTitle" />, + 'section_all', + ], + ...assetsData.map(symbol => ['asset', symbol, false] as ListInnerDataShape), + ['spacer', PICKER_BUTTON_HEIGHT, 'spacer_bottom'], +]; export const TradeableAssetsSheet = ({ /* ... */ }) => { - const data: ListInnerDataShape[] = useMemo( - () => [/* ... */], - [favourites, assetsData], - ); + const data = useMemo( + () => transformData(favourites, assetsData), + [favourites, assetsData] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/atoms/src/Button/Button.tsx(2 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/atoms/src/__tests__/PriceChangeBadge.comp.test.tsx(2 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-dev-utils/src/screens/DemoScreen.tsx(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableNetworkListItem.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/TradeableNetworkListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableNetworkButton.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableNetworkButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/BasicProvider.tsx(0 hunks)suite-native/test-utils/src/BasicProviderForTests.tsx(1 hunks)suite-native/test-utils/src/StoreProviderForTests.tsx(2 hunks)suite-native/test-utils/src/index.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
- suite-native/test-utils/src/BasicProvider.tsx
✅ Files skipped from review due to trivial changes (1)
- suite-native/module-trading/jest.config.js
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- 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: 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: test
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (25)
suite-native/module-trading/package.json (4)
11-11: Script Simplification: Updated Jest CommandThe test command has been simplified by removing the custom configuration file reference. Please verify that the default Jest configuration meets all requirements for testing this module.
18-18: New Dependency Added: expo-linear-gradientA new dependency
"expo-linear-gradient": "^14.0.1"has been introduced to support enhanced UI or animation features as required by the token picker modal and related components. Ensure that this version is compatible with the Expo and related packages in your project.
20-20: Dependency Confirmation: react-native VersionThe dependency for
"react-native": "0.76.1"is explicitly maintained. Confirm that this version remains consistent with your overall project requirements and that it works seamlessly with the newly added dependencies.
21-21: New Dependency Added: react-native-reanimatedThe addition of
"react-native-reanimated": "3.16.7"should provide enhanced animation capabilities, potentially supporting visual aspects of the token picker modal and other interactive components. Please verify compatibility with the existing React Native version and related modules.suite-common/icons/generateIconFont.ts (1)
96-97: LGTM! New icons added for token picker modal.The addition of
starandstarFilledicons aligns with the PR objectives for implementing the token picker modal visual stub.suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
23-24: LGTM! Icon mappings properly configured.The new
starandstarFilledicons are correctly mapped with unique codepoints (61718 and 61719), maintaining the sequential order of the icon font.suite-native/test-utils/src/index.tsx (1)
1-1: LGTM! Consistent provider replacement.The changes consistently replace
BasicProviderwithBasicProviderForTestsacross imports, render function creation, and exports.Also applies to: 17-17, 21-21
suite-native/test-utils/src/StoreProviderForTests.tsx (1)
8-8: LGTM! Consistent provider replacement.The changes consistently replace
BasicProviderwithBasicProviderForTestsacross imports and component usage.Also applies to: 35-35
suite-native/test-utils/src/BasicProviderForTests.tsx (2)
1-18: LGTM! Clean implementation of test provider.The imports are well-organized, and theme/renderer are correctly created outside the component for better performance.
30-43: LGTM! Logical provider nesting.The providers are nested in a logical order, ensuring proper context hierarchy.
suite-native/atoms/src/__tests__/PriceChangeBadge.comp.test.tsx (1)
1-1: LGTM! Consistent provider replacement.The changes consistently replace
BasicProviderwithBasicProviderForTestsacross imports and renderHook usage.Also applies to: 15-15
suite-native/atoms/src/Input/SearchInput.tsx (1)
10-19: Well-defined prop interfaceThe new
SearchInputPropsinterface adds clarity for focus, blur, and controlled value handling. This is a solid foundation for building a reusable search input component.suite-native/module-trading/src/screens/TradingScreen.tsx (1)
3-3: Imports look goodThe newly imported
TextandAmountCardcomponents are consistent with your module structure. No issues found here.Also applies to: 7-8
suite-native/module-trading/src/components/general/__tests__/TradeableNetworkButton.comp.test.tsx (1)
6-10: Clear test for symbol displayVerifying that the symbol's display name is correctly rendered helps prevent regression in the UI. Good test coverage.
suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts (1)
5-26: Well-structured hook implementation!The hook follows React best practices, provides a clean API, and maintains good type safety. The separation of concerns between visibility and asset selection is well-thought-out.
suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx (1)
8-11: LGTM! Props interface is well-defined.The TypeScript interface clearly defines the component's contract with required onPress handler and optional selectedAsset.
suite-native/module-trading/src/components/general/TradeableNetworkButton.tsx (1)
8-12: LGTM! Props interface is well-defined.The TypeScript interface clearly defines the required and optional props with appropriate types.
suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
15-36: LGTM! Well-structured component with enhanced visual hierarchy.The gradient background implementation effectively enhances the visual hierarchy while maintaining proper component composition. The use of
hexToRgbafor gradient opacity is a nice touch.suite-native/module-trading/src/components/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx (1)
17-30: LGTM! Well-structured empty state component.The component follows good practices for empty state UX with proper text hierarchy and spacing. The use of translation IDs ensures proper localization support.
suite-native/module-trading/src/components/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx (1)
5-37: Consider adding more test coverage.While the current tests cover basic interactions, consider adding tests for:
- Search functionality (input changes, filtering)
- Edge cases (empty search, special characters)
- Keyboard interactions (submit, escape)
Would you like me to help generate additional test cases?
suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx (1)
137-142: Consider handling component unmounting.The sticky components might need cleanup when unmounting to prevent memory leaks.
suite-native/atoms/src/Button/Button.tsx (1)
35-36: LGTM!The new button color scheme
backgroundSurfaceElevation0is well-implemented, following the established pattern and using existing theme colors.Also applies to: 185-191
suite-native/module-dev-utils/src/screens/DemoScreen.tsx (1)
82-82: LGTM!The new button color scheme is correctly added to the demo screen for testing and visualization purposes.
suite-native/intl/src/en.ts (1)
1233-1248: LGTM!The new translations for the trading module are well-organized and provide clear, user-friendly strings for the token picker modal functionality.
suite-native/test-utils/package.json (1)
13-13: Verify the bottom-sheet library version.The
@gorhom/bottom-sheetdependency is pinned to version 5.0.5. While this is a stable version, it's recommended to:
- Check for any known security vulnerabilities in this version.
- Consider using the caret (^) version range to allow for compatible updates.
Run the following script to check for security advisories and latest versions:
✅ Verification successful
Bottom-sheet dependency verified: no security issues found, but update recommended.
- The latest version on npm is now 5.0.6 (compared to the pinned 5.0.5 in the PR).
- No security vulnerabilities were reported for the package.
- It’s advisable to use a caret range (e.g.,
"^5.0.5") to allow automatic patch updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of @gorhom/bottom-sheet # Check npm for latest versions curl -s https://registry.npmjs.org/@gorhom/bottom-sheet | jq '.["dist-tags"].latest' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "@gorhom/bottom-sheet") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 448
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableNetworkListItem.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/TradeableNetworkButton.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetListItem.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx
Outdated
Show resolved
Hide resolved
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
182f018 to
161f07a
Compare
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (27)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx (3)
1-7: LGTM! Consider adding type imports.The imports are well-organized and follow the project's architecture by utilizing design system components.
Consider importing types for the constants to ensure type safety:
-import { SHEET_HEADER_HEIGHT } from './TradeableAssetsSheetHeader'; -import { PICKER_BUTTON_HEIGHT } from '../PickerCloseButton'; +import type { SHEET_HEADER_HEIGHT } from './TradeableAssetsSheetHeader'; +import type { PICKER_BUTTON_HEIGHT } from '../PickerCloseButton';
8-15: LGTM! Consider extracting magic numbers.The style definition is clean and properly uses design system tokens for consistency.
Consider extracting the magic number
52into a named constant for better maintainability:+const HORIZONTAL_PADDING = 52; const emptyComponentStyle = prepareNativeStyle(({ spacings }) => ({ paddingTop: SHEET_HEADER_HEIGHT + spacings.sp12, paddingBottom: PICKER_BUTTON_HEIGHT + spacings.sp12, - paddingHorizontal: spacings.sp52, + paddingHorizontal: spacings[`sp${HORIZONTAL_PADDING}`], alignContent: 'center', justifyContent: 'center', gap: spacings.sp12, }));
17-30: LGTM! Consider memoizing the component.The component implementation is clean, follows React best practices, and properly uses the design system and translation utilities.
Since this is a purely presentational component, consider memoizing it to prevent unnecessary re-renders:
-export const TradeAssetsListEmptyComponent = () => { +export const TradeAssetsListEmptyComponent = React.memo(() => { const { applyStyle } = useNativeStyles(); return ( <VStack style={applyStyle(emptyComponentStyle)}> <Text variant="body" color="textDefault" textAlign="center"> <Translation id="moduleTrading.tradeableAssetsSheet.emptyTitle" /> </Text> <Text variant="hint" color="textSubdued" textAlign="center"> <Translation id="moduleTrading.tradeableAssetsSheet.emptyDescription" /> </Text> </VStack> ); -}; +});Don't forget to add the React import:
+import React from 'react'; import { Text, VStack } from '@suite-native/atoms';suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx (2)
16-27: Consider using TypeScript's Pick type for better type safety.The helper function's props type could be more precise using TypeScript's utility types.
-({ symbol, contractAddress }: { symbol: NetworkSymbol; contractAddress?: string }) => +({ symbol, contractAddress }: Pick<{ symbol: NetworkSymbol; contractAddress?: string }, 'symbol' | 'contractAddress'>) =>
52-90: Consider adding color format validation test.While the test suite is comprehensive, it would be beneficial to add a test case that validates the returned color format (hex color code).
+ it('should return valid hex color format', async () => { + const { result } = renderTradeableAssetDominantColorHook('btc', MOCK_CONTRACT_ADDR); + await waitFor(() => { + expect(result.current).toMatch(/^#[0-9A-F]{6}$/i); + }); + });suite-native/test-utils/src/expoMock.js (1)
411-420: Consider making the mock more flexible for testing.The current mock implementation could be enhanced to support dynamic platform and color values for more comprehensive testing scenarios.
jest.mock('react-native-image-colors', () => ({ - getColors: () => + getColors: (options = { platform: 'ios', colors: { background: '#111111', primary: '#333333', secondary: '#555555', detail: '#777777' } }) => Promise.resolve({ - background: '#111111', - primary: '#333333', - secondary: '#555555', - detail: '#777777', - platform: 'ios', + background: options.colors?.background ?? '#111111', + primary: options.colors?.primary ?? '#333333', + secondary: options.colors?.secondary ?? '#555555', + detail: options.colors?.detail ?? '#777777', + platform: options.platform ?? 'ios', }), }));suite-native/atoms/src/Input/SearchInput.tsx (3)
64-64: Consider moving the noOp utility function.The
noOputility function could be moved to a shared utilities file if it's likely to be reused across components.
93-95: Simplify handleInputFocus function.The
handleInputFocusfunction can be simplified since it only callsfocus()on the ref.-const handleInputFocus = () => { - searchInputRef.current?.focus(); -}; +const handleInputFocus = () => searchInputRef.current?.focus();
81-82: Consider deriving clear button visibility from value prop.The
isClearButtonVisiblestate could be derived from thevalueprop to avoid potential synchronization issues.-const [isClearButtonVisible, setIsClearButtonVisible] = useState<boolean>(false); -const handleOnChangeText = (inputValue: string) => { - setIsClearButtonVisible(!!inputValue.length); - onChange(inputValue); -}; +const handleOnChangeText = (inputValue: string) => { + onChange(inputValue); +}; +const isClearButtonVisible = !!value?.length;Also applies to: 97-100
suite-native/icons/src/getCryptoIconUrl.ts (1)
6-20: Consider adding JSDoc comments for better documentation.The function logic is well-implemented, handling both regular and contract-based crypto icons. Consider adding JSDoc comments to document the parameters, return value, and behavior.
Add documentation:
+/** + * Gets the URL for a crypto icon based on the symbol and optional contract address. + * @param symbol - The network symbol of the cryptocurrency + * @param contractAddress - Optional contract address for tokens + * @returns The URL of the crypto icon + */ export const getCryptoIconUrl = (symbol: NetworkSymbol, contractAddress?: string) => {suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx (2)
6-9: Consider adding JSDoc comments for the props interface.The props interface is well-defined but could benefit from documentation.
Add documentation:
+/** + * Props for the FavouriteIcon component + * @property isFavourite - Whether the item is marked as favourite + * @property onPress - Callback function when the icon is pressed + */ export type FavouriteIconProps = {
11-25: Consider adding hit slop and feedback state.The component is well-implemented with proper accessibility support. Consider these improvements:
- Add hit slop to increase the touchable area
- Add visual feedback for pressed state
Apply these improvements:
return ( <Pressable onPress={onPress} accessibilityRole="button" accessibilityHint={hint} + hitSlop={8} + style={({ pressed }) => ({ + opacity: pressed ? 0.7 : 1, + })} > <Icon name={iconName} color={iconColor} /> </Pressable> );suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx (1)
5-25: Consider adding more test cases for comprehensive coverage.The current test suite covers basic functionality well. Consider adding these additional test cases:
- Test for disabled state behavior
- Test for loading state if applicable
- Test for error handling
suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx (2)
7-8: Consider using uppercase in mock data for consistency.The mock data uses lowercase 'btc' while the test expects uppercase 'BTC'. Consider using uppercase in the mock data for better readability and to make the transformation explicit.
- const btcAsset: TradeableAsset = { symbol: 'btc' }; + const btcAsset: TradeableAsset = { symbol: 'BTC' };
9-19: Verify accessibility label is properly set.The test should verify that the accessibility label is properly set since it's a required prop.
it('should render display name of given symbol', () => { - const { getByText } = render( + const { getByText, getByA11yLabel } = render( <TradeableAssetButton asset={btcAsset} onPress={jest.fn()} accessibilityLabel="a11yLabel" />, ); expect(getByText('BTC')).toBeDefined(); + expect(getByA11yLabel('a11yLabel')).toBeDefined(); });suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx (1)
8-11: Add prop types validation.Consider adding runtime validation for required props using PropTypes or similar.
suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx (1)
5-37: Enhance test coverage with additional scenarios.Consider adding these test cases:
- Test search input value changes and callbacks
- Test empty state handling
- Extract hardcoded text values to constants
Example implementation:
const TEST_STRINGS = { COINS: 'Coins', ALL: 'All', SEARCH: 'Search', CANCEL: 'Cancel' }; it('should handle search input changes', () => { const onSearchChange = jest.fn(); const { getByPlaceholderText } = render( <TradeableAssetsSheetHeader onSearchChange={onSearchChange} /> ); const input = getByPlaceholderText(TEST_STRINGS.SEARCH); fireEvent.changeText(input, 'test'); expect(onSearchChange).toHaveBeenCalledWith('test'); });suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts (2)
34-36: Improve error handling by providing fallback color.When an error occurs during color extraction, the color state isn't updated, which could lead to inconsistent UI. Consider setting the fallback color in the catch block.
} catch (error) { console.error('Error getting dominant color', error); + setColor(defaultColor); }
39-39: Remove unnecessary empty cleanup function.The empty cleanup function can be omitted as it doesn't perform any cleanup.
- return () => {};suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (1)
17-28: Move mock data to a separate file.The mock data should be moved to a separate file to improve maintainability and make it easier to replace with real data later.
Consider creating a new file
mockData.ts:export const mockTabNames = [ 'All', 'Ethereum', 'Solana', 'Base', 'Ethereum 2', 'Solana 2', 'Base 2', 'Ethereum 3', 'Solana 3', 'Base 3', ];suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
37-41: Add type for gradient coordinates.The gradient start and end coordinates should have proper typing.
+type GradientPoint = { x: number; y: number }; + <LinearGradient colors={gradientColors} - start={{ x: 0.5, y: 0.5 }} - end={{ x: 0.5, y: 0 }} + start={useMemo<GradientPoint>(() => ({ x: 0.5, y: 0.5 }), [])} + end={useMemo<GradientPoint>(() => ({ x: 0.5, y: 0 }), [])} >suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (1)
14-14: Type the SHEET_HEADER_HEIGHT constant as a number.The constant should be explicitly typed as a number instead of using
as const.-export const SHEET_HEADER_HEIGHT = 110 as const; +export const SHEET_HEADER_HEIGHT: number = 110;suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx (1)
6-79: Add test case for price change rendering.The test suite covers basic rendering and interactions but should also verify the price change badge rendering with different values (positive, negative, zero).
Add this test case:
it('should render price change badge correctly', () => { const { getByText } = render( <TradeableAssetListItem asset={btcAsset} onPress={jest.fn()} onFavouritePress={jest.fn()} priceChange={-5.5} fiatRate={100} />, ); expect(getByText('-5.5%')).toBeDefined(); });suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (1)
38-38: Use theme colors for border color.Replace the hardcoded rgba value with a theme color for consistency.
- borderColor: 'rgba(0, 0, 0, 0.06)', + borderColor: colors.borderSubtleSecondary,suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (3)
14-27: Add JSDoc comments to props and types.Consider adding JSDoc comments to document:
- Purpose of each prop in
TradeableAssetsSheetProps- Meaning of each tuple type in
ListInnerItemShape- Expected format of data
+/** + * Props for the TradeableAssetsSheet component + * @property isVisible - Controls the visibility of the sheet + * @property onClose - Callback when the sheet is closed + * @property onAssetSelect - Callback when an asset is selected + */ export type TradeableAssetsSheetProps = { isVisible: boolean; onClose: () => void; onAssetSelect: (symbol: TradeableAsset) => void; }; +/** + * Represents different types of items that can appear in the list + * - spacer: [type, height, key] + * - sectionHeader: [type, text, key] + * - asset: [type, data, metadata] + */ type ListInnerItemShape = | ['spacer', number, string] | ['sectionHeader', ReactNode, string] | ['asset', TradeableAsset, { isFavourite?: boolean; isFirst?: boolean; isLast?: boolean }];
143-147: Implement the toggleFavourite functionality.The
toggleFavouritefunction is currently a placeholder with a TODO comment. This needs to be implemented to enable users to add/remove assets from their favorites.Would you like me to help implement the
toggleFavouritefunctionality? I can suggest an implementation that integrates with a state management solution.
168-201: Consider performance optimizations for the list rendering.The component could benefit from these optimizations:
- Memoize the
onAssetSelectCallbackusinguseCallback- Add
getItemTypefunction to optimize list rendering- Consider implementing windowing for large lists
+import { useCallback } from 'react'; export const TradeableAssetsSheet = ({ isVisible, onClose, onAssetSelect, }: TradeableAssetsSheetProps) => { - const onAssetSelectCallback = (asset: TradeableAsset) => { + const onAssetSelectCallback = useCallback((asset: TradeableAsset) => { onAssetSelect(asset); onClose(); - }; + }, [onAssetSelect, onClose]); + const getItemType = (item: ListInnerItemShape) => item[0]; // ... rest of the component return ( <BottomSheetFlashList<ListInnerItemShape> // ... other props + getItemType={getItemType} estimatedListHeight={estimatedListHeight} renderItem={({ item }) => renderItem(item, onAssetSelectCallback)} /> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (43)
suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/atoms/src/Button/Button.tsx(2 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/atoms/src/__tests__/PriceChangeBadge.comp.test.tsx(2 hunks)suite-native/icons/src/CryptoIcon.tsx(2 hunks)suite-native/icons/src/getCryptoIconUrl.ts(1 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-dev-utils/src/screens/DemoScreen.tsx(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/BasicProvider.tsx(0 hunks)suite-native/test-utils/src/BasicProviderForTests.tsx(1 hunks)suite-native/test-utils/src/StoreProviderForTests.tsx(2 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)suite-native/test-utils/src/index.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- suite-native/module-trading/src/components/general/PickerHeader.tsx
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/test-utils/src/BasicProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (16)
- suite-native/module-trading/jest.config.js
- suite-native/module-dev-utils/src/screens/DemoScreen.tsx
- suite-native/test-utils/package.json
- suite-native/module-trading/src/screens/TradingScreen.tsx
- suite-native/atoms/src/tests/PriceChangeBadge.comp.test.tsx
- suite-native/test-utils/src/StoreProviderForTests.tsx
- suite-common/icons/generateIconFont.ts
- suite-native/test-utils/src/index.tsx
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/test-utils/src/BasicProviderForTests.tsx
- suite-native/module-trading/package.json
- suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- suite-native/module-trading/src/components/buy/AmountCard.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- 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: EAS Update
- 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: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (17)
suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx (2)
1-11: LGTM! Test setup and imports are well organized.The imports cover all necessary testing utilities and dependencies, and the mock contract address is properly defined as a constant.
38-50: LGTM! Test cases thoroughly cover undefined contract address scenarios.The test suite properly handles both valid network colors and fallback scenarios.
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
23-104: Verify Icon Re-mapping and ConsistencyThe changes introduce a comprehensive reordering and re-assignment of icon IDs. Notably, new entries such as
"starFilled"(61718) and"star"(61719) are now included, and several previous icons (e.g.,"stack","shuffle", etc.) have been re-assigned new numeric values. Please ensure that:
- These updated IDs align with the design specifications and the consuming components (e.g., the icon generation logic in
generateIconFont.ts).- Any references to the old ID values are updated to match the new assignments.
- The overall ordering remains consistent with any expectations in downstream processes (e.g., caching or lookup tables).
Follow-up: Consider reviewing the documentation or build scripts that depend on these mappings to avoid potential mismatches after this overhaul.
suite-native/atoms/src/Input/SearchInput.tsx (3)
1-1: LGTM! Good job on the type definitions.The renamed type
SearchInputPropsand its new properties align well with React Native'sTextInputpatterns. Exporting the type is a good practice for component reusability.Also applies to: 10-19
66-133: Well-structured component implementation!The refactoring to use
forwardRefand the addition of controlled input behavior through thevalueprop is well done. The component follows React Native best practices and maintains good separation of concerns.
85-85: Avoid non-null assertion in useImperativeHandle.The non-null assertion (
!) inuseImperativeHandlecould be unsafe if the ref is null.Consider adding a null check or using optional chaining:
-useImperativeHandle(ref, () => searchInputRef.current!, [searchInputRef]); +useImperativeHandle(ref, () => searchInputRef.current ?? null, [searchInputRef]);✅ Verification successful
Non-null assertion in useImperativeHandle can cause runtime issues
The current code directly asserts thatsearchInputRef.currentis non-null. If for any reason the ref is null whenuseImperativeHandleis called, this may lead to unexpected runtime errors. Replacing the non-null assertion with a nullish check—such assearchInputRef.current ?? null—ensures that the ref value is safely handled.
- Location:
suite-native/atoms/src/Input/SearchInput.tsx(line 85)- Recommendation: Change
-useImperativeHandle(ref, () => searchInputRef.current!, [searchInputRef]); +useImperativeHandle(ref, () => searchInputRef.current ?? null, [searchInputRef]);🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
suite-native/icons/src/index.ts (1)
6-6: LGTM!The export is consistent with other exports in the file and correctly exposes the new
getCryptoIconUrlfunction.suite-native/module-trading/src/types.ts (1)
4-9: Consider usingTokenInfoBrandedtype as suggested in the note.The
TradeableAssettype is well-structured, but as noted,TokenInfoBrandedmight be more appropriate for production code.Let's verify if
TokenInfoBrandedwould be a better fit:✅ Verification successful
TokenInfoBranded Adoption Confirmed
The repository already defines and widely uses the
TokenInfoBrandedtype (e.g. in selectors and account types). This supports the review suggestion that in production theTradeableAssettype insuite-native/module-trading/src/types.tsshould useTokenInfoBranded(or adjust to align with it) for consistency. Note that this change may require renaming properties – for example, replacingcontractAddresswith thecontractproperty (and ensuring symbol types align) as defined inTokenInfoBranded.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the TokenInfoBranded type definition and its usage # Search for TokenInfoBranded type definition ast-grep --pattern $'type TokenInfoBranded = { $$$ }' # Search for existing usage of TokenInfoBranded rg -A 5 'TokenInfoBranded'Length of output: 6943
suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx (1)
13-37: Consider error boundary for translation fallback.The component uses translation keys directly. Consider adding error boundaries or fallback text for missing translations.
suite-native/icons/src/CryptoIcon.tsx (1)
38-41: LGTM! Well-structured component with proper optimizations.The component is well-implemented with:
- Proper memoization of sourceUrl
- Good error handling with placeholder
- Efficient caching policy
suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
20-20: Remove or use PICKER_BUTTON_HEIGHT constant.The PICKER_BUTTON_HEIGHT constant is defined but never used in the component.
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (1)
31-53: LGTM! Well-structured component with proper animations.The component is well-organized with:
- Proper state management for filter activation and value
- Memoized gradient colors
- Smooth animations for title visibility
- Consistent spacing and layout
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (1)
55-77: LGTM! Well-implemented button with proper accessibility.The component has:
- Proper gradient implementation
- Good accessibility support
- Correct symbol formatting
- Proper icon integration
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx (2)
56-86: LGTM! Well-structured list item with proper layout.The component has:
- Good type definitions
- Proper accessibility support
- Clean layout structure
- Consistent styling
53-53: Add error handling for NetworkNameFormatter.The formatter could potentially throw an error if the symbol is invalid.
Add error handling:
- const assetName = name ?? <NetworkNameFormatter value={symbol} />; + const assetName = name ?? ( + try { + return <NetworkNameFormatter value={symbol} />; + } catch (error) { + console.warn('Failed to format network name:', error); + return symbol.toUpperCase(); + } + );suite-native/atoms/src/Button/Button.tsx (1)
35-36: LGTM! The new color scheme follows the existing pattern.The new
backgroundSurfaceElevation0color scheme is well-integrated with the existing button color schemes and follows the established pattern.Also applies to: 185-191
suite-native/intl/src/en.ts (1)
1248-1263: Verify consistency of trading-related translations.The new translations for the trading module are well-organized. However, let's ensure consistency:
- The term "coin" is used throughout - verify if this should be "asset" in some places
- Check if "favourites" spelling is consistent with the rest of the app (vs "favorites")
Let's check the terminology usage:
✅ Verification successful
Trading translations are consistent with the rest of the codebase.
- The term “coin” is used extensively throughout TypeScript files, with no evidence suggesting any necessity to use “asset” instead.
- The British spelling “favourites” appears uniformly in the translation snippet, and no conflicting usage (e.g. “favorites”) was found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for "coin" vs "asset" usage echo "Searching for 'coin' usage..." rg -i '\bcoin' --type ts echo "Searching for 'asset' usage..." rg -i '\basset' --type ts echo "Searching for 'favorites' vs 'favourites' usage..." rg -i 'favou?rites?' --type tsLength of output: 68734
...ive/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
Outdated
Show resolved
Hide resolved
...e-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
Show resolved
Hide resolved
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
161f07a to
6c7f6f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (2)
49-53: Add error handling for dominant color hook.Consider handling cases where the dominant color might not be available.
- const dominantAssetColor = useTradeableAssetDominantColor(symbol, contractAddress); + const dominantAssetColor = useTradeableAssetDominantColor(symbol, contractAddress) ?? '#000000';
50-53: Improve type safety for gradient colors.Consider using a more specific type for the gradient colors array.
- const gradientColors = useMemo<[string, string]>( + type GradientColor = `rgba(${number}, ${number}, ${number}, ${number})`; + const gradientColors = useMemo<[GradientColor, GradientColor]>( () => [hexToRgba(dominantAssetColor, 0.3), hexToRgba(dominantAssetColor, 0.01)], [dominantAssetColor], );suite-native/atoms/src/Input/SearchInput.tsx (2)
93-96: Remove redundant handleInputFocus function.The
handleInputFocusfunction only callsfocus()on the ref, which is redundant since thePressablecomponent'sonPresscould directly use the ref'sfocus()method.-const handleInputFocus = () => { - searchInputRef.current?.focus(); -}; return ( - <Pressable onPress={handleInputFocus}> + <Pressable onPress={() => searchInputRef.current?.focus()}>
66-133: Consider wrapping with React.memo for performance.Since this component might be used in lists (e.g., search results), consider wrapping it with
React.memoto prevent unnecessary re-renders when parent components update.-export const SearchInput = forwardRef<TextInput, SearchInputProps>( +export const SearchInput = React.memo(forwardRef<TextInput, SearchInputProps>( // ... existing code ... -); +));suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
35-51: Consider extracting gradient configuration for reusability.The gradient configuration could be reused across other components. Consider extracting it into a shared constant or utility.
+const SURFACE_FADE_GRADIENT_CONFIG = { + start: { x: 0.5, y: 0.5 }, + end: { x: 0.5, y: 0 }, +} as const; export const PickerCloseButton = (props: PickerCloseButtonProps) => { // ... return ( <VStack spacing={0}> <LinearGradient colors={gradientColors} - start={{ x: 0.5, y: 0.5 }} - end={{ x: 0.5, y: 0 }} + {...SURFACE_FADE_GRADIENT_CONFIG} >suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (1)
30-38: Add hover and focus states for better UX.The
FilterTabcomponent should handle hover and focus states for better user experience.const FilterTab = ({ active, onPress, children }: FilterTabProps) => { const colorScheme = active ? 'tertiaryElevation0' : 'backgroundSurfaceElevation0'; return ( <Button colorScheme={colorScheme} size="small" onPress={onPress} + hoverColorScheme={active ? 'tertiaryElevation1' : 'backgroundSurfaceElevation1'} + focusRing > {children} </Button> ); };suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (2)
143-147: Implement the TODO for toggling favorites.The
toggleFavouritefunction is currently a placeholder with a TODO comment.Would you like me to generate an implementation for the
toggleFavouritefunction that persists the favorite state?
182-185: Optimize useMemo dependencies.The
useMemohook's dependency array includes the entirefavouritesandassetsDataarrays, which could cause unnecessary re-renders.const data: ListInnerItemShape[] = useMemo( () => transformToInnerFlatListData(favourites, assetsData), - [favourites, assetsData], + [favourites.length, assetsData.length], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)
💤 Files with no reviewable changes (2)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- suite-common/icons/generateIconFont.ts
- suite-native/module-trading/src/screens/TradingScreen.tsx
- suite-native/module-trading/src/components/buy/AmountCard.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- suite-native/test-utils/src/expoMock.js
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-native/module-trading/src/types.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx
- suite-native/module-trading/src/hooks/tests/useTradeableAssetDominantColor.test.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetListItem.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/FavouriteIcon.comp.test.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/module-trading/package.json
- suite-native/module-trading/jest.config.js
- suite-native/module-trading/src/components/general/tests/TradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetsSheetHeader.comp.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- 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: 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: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (14)
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (2)
1-21: LGTM! Well-structured imports and props interface.The imports are organized logically, and the props interface is well-defined with proper types, including accessibility support.
23-39: Use theme constants for style values.Replace hardcoded numeric values with theme constants for consistency.
const styles = StyleSheet.create({ button: { - height: 36, + height: nativeSpacings.sp36, padding: nativeSpacings.sp4, paddingRight: nativeSpacings.sp12, flexDirection: 'row', justifyContent: 'center', alignItems: 'center', gap: nativeSpacings.sp8, }, }); const gradientBackgroundStyle = prepareNativeStyle(({ borders, colors }) => ({ borderRadius: borders.radii.round, borderWidth: borders.widths.small, - borderColor: 'rgba(0, 0, 0, 0.06)', + borderColor: colors.borderSubtleSecondary, }));suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (2)
1-22: Overall JSON Structure and Icon Integrity Check.The JSON structure is well-formed, and the earlier section (lines 1–22) correctly lists the icon keys that remain unchanged. Please ensure that the removal of outdated icons does not leave any lingering references elsewhere in the application. Cross-check that all changes are synchronized with the icon font generation tool and design documentation.
23-104: New Icon Additions and Sequential Code Points.The new icons—including
"starFilled","star","stack","shuffle","shieldWarning","shieldCheck", and others—are added with sequential code points (61718 to 61799), which demonstrates a consistent and error-free update. Verify that these icons have the intended visual representations in the token picker modal (and other relevant UI components) and that the corresponding generation script (insuite-common/icons/generateIconFont.ts) is updated accordingly.suite-native/atoms/src/Input/SearchInput.tsx (3)
1-1: LGTM! Well-structured type definitions and imports.The type renaming to
SearchInputPropsand the addition of new props (onFocus,onBlur,value) improve the component's API clarity and functionality.Also applies to: 10-19
21-62: LGTM! Well-organized styles with proper typing.The styles are well-structured, using utility-first approach and proper typing for style variants.
66-85: Consider adding controlled input validation.The component accepts a
valueprop but doesn't properly handle the controlled vs uncontrolled input state transition warnings. Consider adding a warning when the component switches between controlled and uncontrolled states.Add the following validation using
useEffect:+import { useEffect } from 'react'; export const SearchInput = forwardRef<TextInput, SearchInputProps>( ({ onChange, placeholder, maxLength, isDisabled = false, elevation = '0', onFocus = noOp, onBlur = noOp, value, }, ref, ) => { // ... existing code ... + useEffect(() => { + const isControlled = value !== undefined; + const hasValue = searchInputRef.current?.props.value !== undefined; + if (isControlled !== hasValue) { + console.warn( + `SearchInput is changing from ${ + hasValue ? 'controlled' : 'uncontrolled' + } to ${isControlled ? 'controlled' : 'uncontrolled'}` + ); + } + }, [value]);suite-native/module-trading/src/components/general/PickerCloseButton.tsx (4)
15-18: LGTM! Well-structured style definition.The style is properly defined using theme variables and the
prepareNativeStyleutility for consistent theming.
30-34: LGTM! Optimized gradient colors calculation.Good use of
useMemoto optimize gradient colors calculation based on the background color.
1-8: Verify expo-linear-gradient dependency.The component now uses
LinearGradientfromexpo-linear-gradient. Let's ensure we're using a compatible version.✅ Verification successful
Expo-linear-gradient dependency check complete.
The vulnerability query returned no issues, and while the package manifest wasn't found at the root (likely due to project structure), there's no indication of a problematic version being in use. If your project is organized as a monorepo, please verify that the correct package.json (in the relevant sub-project) specifies an appropriate version for expo-linear-gradient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for expo-linear-gradient version cat package.json | jq '.dependencies["expo-linear-gradient"]' # Check for any known issues or breaking changes gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "expo-linear-gradient") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 462
20-20: Document the purpose of PICKER_BUTTON_HEIGHT constant.The constant is exported but not used within the component. Consider adding a JSDoc comment explaining its purpose and intended usage.
✅ Verification successful
I'll re-run the search with an updated command to properly handle TSX files.
Clarify and Document Picker Button Height Constant Usage
The constant is actively imported and used in other modules (e.g., in
TradeAssetsListEmptyComponent.tsxandTradeableAssetsSheet.tsxfor layout spacing). While its usage is consistent, a JSDoc comment describing that 52 represents the picker button’s height (and its role in layout calculations) would improve code clarity and maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of PICKER_BUTTON_HEIGHT in the codebase rg "PICKER_BUTTON_HEIGHT" --type ts --type tsxLength of output: 74
Script:
#!/bin/bash # Adding TSX file type explicitly for proper search rg "PICKER_BUTTON_HEIGHT" --type-add "tsx:*.tsx" --type ts --type tsxLength of output: 974
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (1)
51-62: Add key prop and accessibility support.The FlatList is missing key extraction and accessibility props.
<Animated.FlatList entering={FadeIn.duration(animationDuration)} horizontal showsHorizontalScrollIndicator={false} data={mockTabNames} + keyExtractor={item => item} + accessible={true} + accessibilityRole="tablist" renderItem={({ item }) => ( <FilterTab active={activeTab === item} onPress={() => setActiveTab(item)} + accessibilityRole="tab" + accessibilityState={{ selected: activeTab === item }} > {item} </FilterTab> )} />suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (1)
30-55: Replace mock data in TradeableAssetsSheet with live data sources.The component currently hardcodes favorites, available assets, and even generates random fiat rates and price changes. However, our repository already provides several API and utility implementations for fetching real data (for example, in 'suite-common/fiat-services' and 'suite-common/trading/src/invityAPI.ts'). This confirms that relying on hardcoded mock data in production is risky and should be updated.
suite-native/intl/src/en.ts (1)
1248-1263: LGTM! The translations follow the established patterns.The new translations for the trading module are well-structured and consistent with the existing translation patterns in the file.
...ive/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
Show resolved
Hide resolved
...e-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
Outdated
Show resolved
Hide resolved
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
6c7f6f2 to
16673c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
suite-native/intl/src/en.ts (2)
1262-1262: Consider movingdefaultSearchLabelto a more generic section.The
defaultSearchLabeltranslation seems more generic and could be reused across different modules. Consider moving it to thegenericsection at the top of the file.- defaultSearchLabel: 'Search',Add to the
genericsection (around line 30):+ defaultSearchLabel: 'Search',
1248-1263: Consider adding translations for additional error states.The module currently only handles the empty search results error state. Consider adding translations for other potential error states such as:
- Network errors when loading coins
- Rate limiting errors
- General API errors
moduleTrading: { selectCoin: { buttonTitle: 'Select coin', }, tradeableAssetsSheet: { title: 'Coins', favouritesTitle: 'Favourites', allTitle: 'All coins', favouritesAdd: 'Add to favourites', favouritesRemove: 'Remove from favourites', emptyTitle: 'No coin found', emptyDescription: 'We couldn't find a coin matching your search. Try checking the spelling or exploring the list for the right option.', + errors: { + networkError: 'Unable to load coins. Please check your connection.', + rateLimitError: 'Too many requests. Please try again later.', + generalError: 'Something went wrong. Please try again.', + }, }, defaultSearchLabel: 'Search', },suite-native/atoms/src/Input/SearchInput.tsx (2)
93-95: Consider adding onFocus callback to handleInputFocus.The
handleInputFocusfunction only focuses the input but doesn't call theonFocuscallback. This could lead to inconsistent behavior with the direct focus event handler.Apply this diff to maintain consistency:
const handleInputFocus = () => { searchInputRef.current?.focus(); + onFocus(); };
81-82: Consider deriving clear button visibility from value prop.Since the component is now controlled with the
valueprop, theisClearButtonVisiblestate could be derived from thevalueprop instead of maintaining a separate state.Apply this diff to optimize the state management:
-const [isClearButtonVisible, setIsClearButtonVisible] = useState<boolean>(false); +const isClearButtonVisible = !!value?.length; const handleOnChangeText = (inputValue: string) => { - setIsClearButtonVisible(!!inputValue.length); onChange(inputValue); };Also applies to: 97-100
suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
37-41: Consider extracting gradient configuration.The gradient start and end points could be extracted as constants or memoized if they're used elsewhere.
+const GRADIENT_START = { x: 0.5, y: 0.5 } as const; +const GRADIENT_END = { x: 0.5, y: 0 } as const; <LinearGradient colors={gradientColors} - start={{ x: 0.5, y: 0.5 }} - end={{ x: 0.5, y: 0 }} + start={GRADIENT_START} + end={GRADIENT_END} >suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (2)
30-38: Enhance FilterTab component with spacing and accessibility.The FilterTab component needs improvements:
- Add horizontal margin/padding for better spacing between tabs
- Add accessibility support for screen readers
const FilterTab = ({ active, onPress, children }: FilterTabProps) => { const colorScheme = active ? 'tertiaryElevation0' : 'backgroundSurfaceElevation0'; return ( <Button colorScheme={colorScheme} size="small" onPress={onPress} + style={{ marginHorizontal: 4 }} + accessibilityRole="tab" + accessibilityState={{ selected: active }} > {children} </Button> ); };
40-63: Optimize performance and add test coverage support.Consider the following improvements:
- Memoize the onPress handler with useCallback
- Add testID for E2E testing
+import { useCallback } from 'react'; export const TradeableAssetsFilterTabs = ({ visible, animationDuration, }: TradeableAssetsFilterTabsProps) => { const [activeTab, setActiveTab] = useState(mockTabNames[0]); + + const handleTabPress = useCallback((item: string) => { + setActiveTab(item); + }, []); if (!visible) { return null; } return ( <Animated.FlatList + testID="tradeable-assets-filter-tabs" entering={FadeIn.duration(animationDuration)} horizontal showsHorizontalScrollIndicator={false} data={mockTabNames} renderItem={({ item }) => ( <FilterTab active={activeTab === item} - onPress={() => setActiveTab(item)} + onPress={() => handleTabPress(item)} + testID={`filter-tab-${item}`} > {item} </FilterTab> )} /> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)
💤 Files with no reviewable changes (2)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (24)
- suite-common/icons/generateIconFont.ts
- suite-native/module-trading/jest.config.js
- suite-native/module-trading/src/components/general/tests/TradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/screens/TradingScreen.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/FavouriteIcon.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetsSheetHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
- suite-native/test-utils/src/expoMock.js
- suite-native/module-trading/package.json
- suite-native/module-trading/src/components/buy/AmountCard.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetListItem.comp.test.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts
- suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
- suite-native/module-trading/src/hooks/tests/useTradeableAssetDominantColor.test.tsx
- suite-native/module-trading/src/types.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-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 / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- 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: build-web
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
suite-native/intl/src/en.ts (1)
1266-1267: LGTM!The type declaration is well-defined and follows TypeScript best practices.
suite-native/atoms/src/Input/SearchInput.tsx (2)
1-1: LGTM! Well-structured type definitions and imports.The renaming of
InputPropstoSearchInputPropsbetter reflects the component's purpose, and the new props enhance the component's flexibility.Also applies to: 10-19
66-85: LGTM! Well-implemented ref forwarding.The component correctly implements
forwardRefanduseImperativeHandleto expose theTextInputref to parent components.suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
23-104: Comprehensive Update to Icon DefinitionsThe new icon entries (starting with
"starFilled": 61718through to"arrowDown": 61799) are added in a sequential and consistent manner. The updated codepoints maintain proper ordering relative to the preceding icons (e.g.,"swap": 61717), ensuring there are no duplicate values and the numbering is coherent with the icon font generation process.Please verify that the newly added icons are correctly integrated into the mobile UI—particularly for elements like the token picker modal visual stub—as outlined in the PR objectives. It might also be beneficial to add inline comments or logical grouping (e.g., grouping icons by usage or design category) to improve long-term maintainability.
suite-native/module-trading/src/components/general/PickerCloseButton.tsx (3)
1-13: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the type definition clearly specifies the component's props interface.
30-33: LGTM! Good use of useMemo for gradient colors.The gradient colors are properly memoized with the correct dependency array.
20-20: Verify the usage ofPICKER_BUTTON_HEIGHTconstant.The constant is exported but not used within this component. Please verify if this constant is used elsewhere in the codebase.
✅ Verification successful
Constant usage verified: The
PICKER_BUTTON_HEIGHTconstant is actively used in the following files:
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(used in padding calculations)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(imported and applied in layout definitions)No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of PICKER_BUTTON_HEIGHT in the codebase rg "PICKER_BUTTON_HEIGHT" --type ts --type tsxLength of output: 74
Script:
#!/bin/bash # Search for usage of PICKER_BUTTON_HEIGHT in .ts and .tsx files using globs instead of file types. rg "PICKER_BUTTON_HEIGHT" --glob "*.ts" --glob "*.tsx"Length of output: 959
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (2)
17-28: Replace hardcoded mock data with dynamic data source.The mock data should be replaced with dynamic data passed through props.
51-62: Add key prop and accessibility support.The FlatList needs key extraction and accessibility props.
...ive/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
Show resolved
Hide resolved
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
16673c7 to
819d98e
Compare
trezor-ci
force-pushed
the
module-trade-ui-tokens
branch
from
c198b54 to
7198238
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
suite-native/intl/src/en.ts (1)
1259-1261: Consider improving the empty state message for better clarity.The empty state description could be more concise while maintaining its helpfulness.
- emptyDescription: - 'We couldn't find a coin matching your search. Try checking the spelling or exploring the list for the right option.', + emptyDescription: 'No matches found. Check spelling or browse the full list.',suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
25-50: Consider these improvements to the component implementation.
- The
PICKER_BUTTON_HEIGHTconstant is defined but not used in the component.- The gradient colors array could have a more descriptive name.
- The Button should use the defined height constant.
Consider applying these changes:
export const PickerCloseButton = (props: PickerCloseButtonProps) => { const { utils: { colors: { backgroundSurfaceElevation0 }, }, applyStyle, } = useNativeStyles(); - const gradientColors = useMemo<[string, string]>( + const fadeOutGradientColors = useMemo<[string, string]>( () => [backgroundSurfaceElevation0, hexToRgba(backgroundSurfaceElevation0, 0.1)], [backgroundSurfaceElevation0], ); return ( <VStack spacing={0}> - <LinearGradient colors={gradientColors} start={GRADIENT_START} end={GRADIENT_END}> + <LinearGradient colors={fadeOutGradientColors} start={GRADIENT_START} end={GRADIENT_END}> <Box paddingHorizontal="sp16"> - <Button colorScheme="tertiaryElevation0" {...props}> + <Button + colorScheme="tertiaryElevation0" + height={PICKER_BUTTON_HEIGHT} + {...props} + > <Translation id="generic.buttons.close" /> </Button> </Box> </LinearGradient> <Box style={applyStyle(spacerStyle)} /> </VStack> ); };suite-native/atoms/src/Input/SearchInput.tsx (3)
64-64: Consider moving noOp to shared utilities.The
noOpfunction is a common utility that could be reused across components.Consider moving it to a shared utilities file:
-const noOp = () => {}; +import { noOp } from '@suite-native/utils';
87-100: Consider memoizing event handlers.To prevent unnecessary re-renders, consider memoizing the event handlers using
useCallback.-const handleClear = () => { +const handleClear = useCallback(() => { setIsClearButtonVisible(false); searchInputRef.current?.clear(); onChange(''); -}; +}, [onChange]); -const handleInputFocus = () => { +const handleInputFocus = useCallback(() => { searchInputRef.current?.focus(); -}; +}, []); -const handleOnChangeText = (inputValue: string) => { +const handleOnChangeText = useCallback((inputValue: string) => { setIsClearButtonVisible(!!inputValue.length); onChange(inputValue); -}; +}, [onChange]);
102-131: Consider extracting ClearButton component.The clear button logic could be extracted into a separate component for better maintainability.
+const ClearButton = ({ onPress }: { onPress: () => void }) => ( + <TouchableOpacity onPress={onPress}> + <Icon name="xCircle" size="large" color="iconSubdued" /> + </TouchableOpacity> +); return ( <Pressable onPress={handleInputFocus}> <Box style={applyStyle(inputWrapperStyle, { isFocused, elevation })}> <Icon name="magnifyingGlass" color="iconSubdued" size="large" /> <TextInput ref={searchInputRef} onChangeText={handleOnChangeText} placeholder={placeholder} placeholderTextColor={utils.colors.textSubdued} editable={!isDisabled} onFocus={() => { setIsFocused(true); onFocus(); }} onBlur={() => { setIsFocused(false); onBlur(); }} style={applyStyle(inputStyle)} maxLength={maxLength} value={value} /> - {isClearButtonVisible && ( - <TouchableOpacity onPress={handleClear}> - <Icon name="xCircle" size="large" color="iconSubdued" /> - </TouchableOpacity> - )} + {isClearButtonVisible && <ClearButton onPress={handleClear} />} </Box> </Pressable> );suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx (2)
47-55: Consider extracting style constants.The styles for sticky components could be more maintainable with extracted constants.
+const STICKY_BASE_STYLE = { + ...StyleSheet.absoluteFillObject, +}; -const stickyListHeaderStyle = prepareNativeStyle(_ => ({ - ...StyleSheet.absoluteFillObject, - bottom: undefined, -})); +const stickyListHeaderStyle = prepareNativeStyle(_ => ({ + ...STICKY_BASE_STYLE, + bottom: undefined, +})); -const stickyListFooterStyle = prepareNativeStyle(_ => ({ - ...StyleSheet.absoluteFillObject, - top: undefined, -})); +const stickyListFooterStyle = prepareNativeStyle(_ => ({ + ...STICKY_BASE_STYLE, + top: undefined, +}));
132-137: Consider extracting sticky components.The sticky components could be extracted for better maintainability and reusability.
+const StickyHeader = ({ children }: { children: ReactNode }) => ( + <Box style={applyStyle(stickyListHeaderStyle)}>{children}</Box> +); +const StickyFooter = ({ children }: { children: ReactNode }) => ( + <Box style={applyStyle(stickyListFooterStyle)}>{children}</Box> +); -{stickyListHeaderComponent ? ( - <Box style={applyStyle(stickyListHeaderStyle)}>{stickyListHeaderComponent}</Box> -) : null} -{stickyListFooterComponent ? ( - <Box style={applyStyle(stickyListFooterStyle)}>{stickyListFooterComponent}</Box> -) : null} +{stickyListHeaderComponent && <StickyHeader>{stickyListHeaderComponent}</StickyHeader>} +{stickyListFooterComponent && <StickyFooter>{stickyListFooterComponent}</StickyFooter>}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
packages/theme/src/spacings.ts(2 hunks)suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-common/suite-utils/src/hexToRgba.ts(1 hunks)suite-native/app/app.config.ts(1 hunks)suite-native/atoms/src/Button/Button.tsx(2 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/atoms/src/__tests__/PriceChangeBadge.comp.test.tsx(2 hunks)suite-native/icons/src/CryptoIcon.tsx(2 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/icons/src/utils.ts(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-dev-utils/src/screens/DemoScreen.tsx(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/BasicProvider.tsx(0 hunks)suite-native/test-utils/src/BasicProviderForTests.tsx(1 hunks)suite-native/test-utils/src/StoreProviderForTests.tsx(2 hunks)suite-native/test-utils/src/index.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
- suite-native/test-utils/src/BasicProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (36)
- suite-common/icons/generateIconFont.ts
- suite-native/module-trading/jest.config.js
- suite-native/test-utils/package.json
- suite-native/module-trading/src/types.ts
- suite-native/icons/src/index.ts
- suite-native/module-trading/src/screens/TradingScreen.tsx
- suite-native/module-dev-utils/src/screens/DemoScreen.tsx
- suite-native/app/app.config.ts
- suite-native/module-trading/src/components/general/tests/TradeableAssetButton.comp.test.tsx
- suite-native/atoms/src/tests/PriceChangeBadge.comp.test.tsx
- suite-native/test-utils/src/StoreProviderForTests.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts
- suite-native/module-trading/src/components/buy/AmountCard.tsx
- suite-native/icons/src/utils.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/FavouriteIcon.comp.test.tsx
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-native/module-trading/src/hooks/tests/useTradeableAssetDominantColor.test.tsx
- suite-native/icons/src/CryptoIcon.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- suite-native/test-utils/src/BasicProviderForTests.tsx
- suite-native/test-utils/src/index.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx
- suite-common/suite-utils/src/hexToRgba.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx
- suite-native/module-trading/package.json
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
- packages/theme/src/spacings.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetsSheetHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
- suite-native/atoms/src/Button/Button.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetListItem.comp.test.tsx
🧰 Additional context used
📓 Learnings (1)
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (2)
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/general/TradeableAssetButton.tsx:0-0
Timestamp: 2025-02-04T13:17:47.436Z
Learning: React Native Pressable components should include accessibility props (accessible={true}, accessibilityRole="button", accessibilityLabel) to ensure proper screen reader support and user interaction.
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/general/TradeableNetworkButton.tsx:0-0
Timestamp: 2025-02-04T13:17:26.822Z
Learning: The `useTradeableAssetDominantColor` hook in the trading module uses `utils.colors.backgroundNeutralBold` as a fallback color when a network's color is not found in `coinsColors`. This ensures consistent UI even when specific coin colors are unavailable.
⏰ 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-override init-api-flaky
- GitHub Check: PR-check / node-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: 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: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-deploy
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (12)
suite-native/intl/src/en.ts (1)
1248-1263: LGTM! The translations for the trading module are well-structured and comprehensive.The new
moduleTradingmodule provides clear and user-friendly translations for the token picker modal, including:
- Button labels
- Section titles
- Favorites functionality
- Empty state messages
- Search functionality
The translations follow the established patterns in the codebase and provide a good user experience.
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
23-104: Sequential New Icon Definitions AddedThe new icon entries—from
"starFilled": 61718through"arrowDown": 61799—have been introduced with sequential codepoints that follow the existing numbering, ensuring consistency in the icon mapping. Please confirm that the icon font generation process (e.g., updates ingenerateIconFont.ts) properly incorporates these new icons for the mobile application.suite-native/module-trading/src/components/general/PickerCloseButton.tsx (3)
1-13: LGTM! Well-organized imports and clear type definition.The imports are logically organized and the type definition properly constrains the component props.
15-19: LGTM! Constants are well-defined and properly placed.Constants are placed at the top of the file as requested in the past review, with proper typing using 'as const'.
20-23: LGTM! Style follows theme system.The spacer style is well-defined using theme values for consistency.
suite-native/module-trading/src/components/general/TradeableAssetButton.tsx (4)
1-20: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the
TradeableAssetButtonPropstype is well-defined with all necessary props including accessibility support.
22-39: LGTM! Well-structured styles using theme constants.The styles are well-organized and consistently use theme constants for spacing, colors, and borders. The gradient positioning constants are properly typed with
as const.
41-82: LGTM! Well-implemented component with performance optimization.The component is well-structured with:
- Proper use of hooks
- Performance optimization through useMemo
- Comprehensive accessibility support
- Consistent styling with LinearGradient
56-58: Consider handling empty name edge case.The fallback for
displayNameassumes that eithernameorsymbolwill be available. Consider adding a default value to handle cases where both might be undefined.- const displayName = name ?? ( - <DisplaySymbolFormatter value={symbol} areAmountUnitsEnabled={false} /> - ); + const displayName = name ?? ( + symbol ? ( + <DisplaySymbolFormatter value={symbol} areAmountUnitsEnabled={false} /> + ) : ( + 'Unknown Asset' + ) + );suite-native/atoms/src/Input/SearchInput.tsx (2)
1-1: LGTM! Well-structured type definitions.The renamed type and additional props enhance the component's flexibility and type safety.
Also applies to: 10-19
66-85: LGTM! Proper ref handling implementation.The use of
forwardRefanduseImperativeHandlecorrectly exposes theTextInputref to parent components.suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx (1)
14-14: LGTM! Well-structured type definitions.The addition of sticky header and footer components enhances the component's flexibility.
Also applies to: 23-24
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
7198238 to
b9b7f2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx (1)
12-23: Consider adding type safety for translation keys.The helper functions are well-structured with good separation of concerns. Consider adding type safety for translation keys to prevent potential issues with missing translations.
+type TranslationKey = 'moduleTrading.tradeableAssetsSheet.favouritesRemove' | 'moduleTrading.tradeableAssetsSheet.favouritesAdd'; const useA11yButtonHint = (isFavourite: boolean): string => { const { translate } = useTranslate(); return isFavourite - ? translate('moduleTrading.tradeableAssetsSheet.favouritesRemove') - : translate('moduleTrading.tradeableAssetsSheet.favouritesAdd'); + ? translate<TranslationKey>('moduleTrading.tradeableAssetsSheet.favouritesRemove') + : translate<TranslationKey>('moduleTrading.tradeableAssetsSheet.favouritesAdd'); };suite-native/module-trading/src/components/general/PickerCloseButton.tsx (1)
38-50: Consider using the PICKER_BUTTON_HEIGHT constant.The PICKER_BUTTON_HEIGHT constant is defined but not used in the component. Consider applying it to the Button or its container for consistent sizing.
- <Button colorScheme="tertiaryElevation0" {...props}> + <Button + colorScheme="tertiaryElevation0" + height={PICKER_BUTTON_HEIGHT} + {...props} + >suite-native/atoms/src/Input/SearchInput.tsx (4)
66-85: Add displayName for better debugging.The forwardRef component should have a displayName for better debugging experience in React DevTools.
export const SearchInput = forwardRef<TextInput, SearchInputProps>( ({...}) => { // ... } ); +SearchInput.displayName = 'SearchInput';
85-85: Make ref handling more type-safe.The non-null assertion in useImperativeHandle could be made safer by checking for null.
-useImperativeHandle(ref, () => searchInputRef.current!, [searchInputRef]); +useImperativeHandle(ref, () => { + if (!searchInputRef.current) { + throw new Error('TextInput ref is not available'); + } + return searchInputRef.current; +}, [searchInputRef]);
93-95: Make focus handling more robust.The handleInputFocus function should check if the ref exists before calling focus.
const handleInputFocus = () => { - searchInputRef.current?.focus(); + if (searchInputRef.current) { + searchInputRef.current.focus(); + } };
102-131: Consider adding test ID for automated testing.Add testID prop to make the component easier to find in automated tests.
<Pressable onPress={handleInputFocus}> <Box style={applyStyle(inputWrapperStyle, { isFocused, elevation })}> <Icon name="magnifyingGlass" color="iconSubdued" size="large" /> <TextInput + testID="search-input" ref={searchInputRef} onChangeText={handleOnChangeText}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
packages/theme/src/spacings.ts(2 hunks)suite-common/icons/generateIconFont.ts(1 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-common/suite-utils/src/hexToRgba.ts(1 hunks)suite-native/app/app.config.ts(1 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/AmountCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)
💤 Files with no reviewable changes (2)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (27)
- suite-common/icons/generateIconFont.ts
- suite-native/app/app.config.ts
- suite-native/module-trading/src/screens/TradingScreen.tsx
- suite-native/module-trading/jest.config.js
- suite-native/module-trading/src/types.ts
- suite-native/module-trading/src/components/general/tests/TradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/components/buy/AmountCard.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-common/suite-utils/src/hexToRgba.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/FavouriteIcon.comp.test.tsx
- packages/theme/src/spacings.ts
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/module-trading/package.json
- suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx
- suite-native/module-trading/src/hooks/tests/useTradeableAssetDominantColor.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetsSheetHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx
- suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetListItem.comp.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Type Checking
- GitHub Check: Unit Tests
- GitHub Check: Linting and formatting
- 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: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: test
🔇 Additional comments (10)
suite-native/intl/src/en.ts (1)
1248-1263: Well-structured translations for the trading module.The translations are clear, concise, and follow the established patterns in the codebase. The error messages are user-friendly, and the structure is logically organized.
Some notable strengths:
- Clear hierarchy with
selectCoinandtradeableAssetsSheetsections- Helpful empty state message that guides users with actionable suggestions
- Consistent with the file's best practices (no dynamic keys, no split strings)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx (2)
1-10: LGTM! Clean imports and well-defined types.The imports are well-organized and the
FavouriteIconPropstype is clear and concise.
25-37: LGTM! Well-implemented component with good accessibility.The component is well-structured with:
- Enlarged press area using Box padding (addressing previous feedback)
- Proper accessibility implementation with role and hint
- Clean and focused implementation
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (2)
1-22: JSON Structure & Integrity
The JSON file is well-formed with a clear structure. The initial key-value pairs (lines 1–22) are consistent, and the structure uses proper JSON formatting.
23-104: Icon Mapping Consistency & ID Reassignment
In this segment, a series of icon definitions have been added (e.g.,"starFilled": 61718,"star": 61719, etc.). Notably, the previously removed icons are being reintroduced with a completely new numeric sequence. Please verify that these numeric ID reassignments are intentional and that any client-side code or style references tied to these IDs are updated accordingly. Additionally, if there exists any logic or caching mechanism that depends on these IDs remaining stable over time, it might be beneficial to document this change or provide a migration guide.suite-native/module-trading/src/components/general/PickerCloseButton.tsx (4)
1-13: LGTM! Well-organized imports and type definitions.The imports are properly organized and the type definition correctly omits props that will be internally handled by the component.
15-19: LGTM! Constants are properly placed and typed.The constants are now correctly placed at the top of the file as requested in the past review. Good use of 'as const' for type safety.
20-23: LGTM! Well-structured style definition.The style is properly typed and uses theme values for consistency.
33-36: Verify the gradient opacity value.The gradient opacity of 0.1 seems very low and might be barely visible. Consider testing with a higher opacity value for better visual feedback.
suite-native/atoms/src/Input/SearchInput.tsx (1)
1-19: LGTM! Type definitions and imports are well-structured.The renaming of
InputPropstoSearchInputPropsimproves clarity, and the new props enhance the component's interactivity.
| @@ -113,6 +129,12 @@ export const BottomSheetFlashList = <TItem,>({ | |||
| insetBottom, | |||
| })} | |||
| /> | |||
| {stickyListHeaderComponent ? ( | |||
There was a problem hiding this comment.
Did you considered using props handleComponent/footerComponent props? https://gorhom.dev/react-native-bottom-sheet/props#handlecomponent
Not sure if they are usable but they seems good fit.
There was a problem hiding this comment.
Thank you for the suggestion. In meantime design was changed. Only handleComponent is needed now. I also did some small refactoring. (Exported Grabber component, removed unused prop).
There was a problem hiding this comment.
Also I would suggest to change name of the commit from feat(suite-native): Introduce sticky header and footer to BottomSheetFlashList to feat(suite-native): Expose handleComponent from BottomSheetFlashList, refactor after the squash. WDYT?
|
|
||
| return ( | ||
| <HStack alignItems="center"> | ||
| <Box flex={1}> |
There was a problem hiding this comment.
Should be just few lines to add layout animations for showing cancel button, please add it
|
|
||
| return ( | ||
| <LinearGradient colors={gradientColors} start={GRADIENT_START} end={GRADIENT_END}> | ||
| <VStack spacing="sp16" padding="sp16"> |
There was a problem hiding this comment.
Please add layout animation here
There was a problem hiding this comment.
we have another issue for animation of this component #16747
There was a problem hiding this comment.
Why not do it now? It shoudn't be more like 5 lines of code
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
b9b7f2d to
623b02a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
suite-native/module-trading/src/components/buy/PaymentCard.tsx (1)
6-9: Consider using a more descriptive placeholder message.The current message is very generic. Consider adding more context about which feature is not implemented.
- console.log('Not implemented'); + console.log('Payment feature not implemented yet');suite-native/atoms/src/Input/SearchInput.tsx (1)
93-95: Consider calling onFocus in handleInputFocus.The
handleInputFocusfunction focuses the input but doesn't call theonFocusprop. This could lead to inconsistent behavior when the input is focused via the Pressable vs direct interaction.const handleInputFocus = () => { searchInputRef.current?.focus(); + onFocus(); };suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx (2)
12-15: Consider using TypeScript const assertions for better type safety.The icon props could be defined using const assertions to ensure type safety and autocompletion:
-const getIconProps = (isFavourite: boolean): IconProps => - isFavourite - ? { name: 'starFilled', color: 'backgroundAlertYellowBold' } - : { name: 'star', color: 'textSubdued' }; +const iconPresets = { + favourite: { name: 'starFilled', color: 'backgroundAlertYellowBold' }, + normal: { name: 'star', color: 'textSubdued' } +} as const; + +const getIconProps = (isFavourite: boolean): IconProps => + isFavourite ? iconPresets.favourite : iconPresets.normal;
32-34: Consider increasing padding for better touch targets.While the padding helps create a larger touch area, 8px might still be too small. Apple's Human Interface Guidelines recommend a minimum touch target size of 44x44 points.
- <Box padding="sp8"> + <Box padding="sp12"> <Icon {...iconProps} /> </Box>suite-native/module-trading/src/components/general/TradingFooter.tsx (3)
8-11: Consider removing unnecessary optimization and improving asset import.The
useMemofor a static image import might be unnecessary overhead since therequirestatement is already evaluated at build time and cached.Consider simplifying to:
- const imageSource = useMemo(() => require('../../../assets/InvityLogo.png'), []); + const imageSource = require('@suite-native/assets/InvityLogo.png');This also makes the import path more maintainable by using module resolution instead of relative paths.
12-12: Consider moving the URL to a constants file.Hardcoded URLs can be difficult to maintain and update. Consider moving it to a centralized constants file.
+import { EXTERNAL_URLS } from '@suite-native/constants'; + - const openLinkToInvity = () => openLink('https://invity.io'); + const openLinkToInvity = () => openLink(EXTERNAL_URLS.INVITY);
14-34: Add accessibility attributes to interactive elements.While the UI structure is clean and follows the design system, the interactive elements could benefit from improved accessibility.
Consider adding these accessibility attributes:
- <TouchableOpacity onPress={openLinkToInvity}> + <TouchableOpacity + onPress={openLinkToInvity} + accessibilityRole="link" + accessibilityLabel="Invity website" + > <Image source={imageSource} contentFit="contain" width={44} height={18} /> </TouchableOpacity> <Button colorScheme="tertiaryElevation0" size="extraSmall" onPress={openLinkToInvity} viewRight="arrowUpRight" + accessibilityRole="link" + accessibilityLabel="Learn more about Invity" >suite-native/intl/src/en.ts (2)
1271-1274: Consider refining the empty state and search label translations.
- The empty state description could be more concise while maintaining clarity.
- The search label could be more specific to the trading context.
Consider these alternatives:
- emptyDescription: - 'We couldn't find a coin matching your search. Try checking the spelling or exploring the list for the right option.', + emptyDescription: 'No matching coins found. Check spelling or browse the list.', - defaultSearchLabel: 'Search', + defaultSearchLabel: 'Search coins',
1249-1273: Consider grouping related translations for better maintainability.The translations could be better organized by grouping related items into sub-objects, similar to how other modules in the file are structured.
Consider this structure:
moduleTrading: { - tradingScreen: { - buyTitle: 'Buy', - footer: { - poweredBy: 'Powered by', - learnMore: 'Learn more', - }, - }, - selectCoin: { - buttonTitle: 'Select coin', - }, - tradeableAssetsSheet: { + screens: { + trading: { + buyTitle: 'Buy', + footer: { + poweredBy: 'Powered by', + learnMore: 'Learn more', + }, + }, + }, + components: { + selectCoin: { + buttonTitle: 'Select coin', + }, + tradeableAssetsSheet: { + title: 'Coins', + favouritesTitle: 'Favourites', + allTitle: 'All coins', + favouritesAdd: 'Add to favourites', + favouritesRemove: 'Remove from favourites', + emptyTitle: 'No coin found', + emptyDescription: 'No matching coins found. Check spelling or browse the list.', + }, + }, + common: { + searchLabel: 'Search coins', + },This organization:
- Separates screen-specific translations
- Groups reusable component translations
- Isolates common/shared translations
suite-native/module-trading/src/components/buy/BuyCard.tsx (3)
25-37: Add TypeScript interface for component props.Consider adding an interface to define the component's props, even if currently empty, to make future additions more maintainable.
+interface BuyCardProps { + // Add props as needed +} + -export const BuyCard = () => { +export const BuyCard: React.FC<BuyCardProps> = () => {
49-51: Extract hardcoded values to constants.Move hardcoded values ("0.0", "0") to named constants or configuration to improve maintainability and reusability.
+const DEFAULT_AMOUNT = "0.0"; +const DEFAULT_CRYPTO_AMOUNT = "0"; +const DEFAULT_FIAT_AMOUNT = 0; + <Text variant="titleMedium" color="textDisabled"> - 0.0 + {DEFAULT_AMOUNT} </Text> // ... <CryptoAmountFormatter - value="0" + value={DEFAULT_CRYPTO_AMOUNT} symbol={selectedTradeableAsset.symbol} /> // ... <FiatAmountFormatter - value={0} + value={DEFAULT_FIAT_AMOUNT} />Also applies to: 56-59, 66-67
78-84: Improve Bitcoin address display.The current implementation truncates the Bitcoin address with ellipsis. Consider:
- Adding a copy button
- Implementing a proper address truncation utility
- Adding a tooltip to show the full address
+const truncateAddress = (address: string) => + `${address.slice(0, 8)}...${address.slice(-8)}`; <VStack spacing={0} paddingLeft="sp20"> <Text color="textSubdued" variant="body" textAlign="right"> - Bitcoin Vault + {account.name} </Text> - <Text color="textSubdued" variant="hint" ellipsizeMode="tail" numberOfLines={1}> - 3QJmV3qfvL9SuYo34YihAf3sRCW3qSinyC + <HStack alignItems="center" spacing="sp8"> + <Text color="textSubdued" variant="hint"> + {truncateAddress('3QJmV3qfvL9SuYo34YihAf3sRCW3qSinyC')} + </Text> + <Icon name="copy" onPress={handleCopyAddress} /> + </HStack> </VStack>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfsuite-native/module-trading/assets/InvityLogo.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
packages/theme/src/spacings.ts(2 hunks)suite-common/icons/generateIconFont.ts(2 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-common/suite-utils/src/hexToRgba.ts(1 hunks)suite-native/app/app.config.ts(1 hunks)suite-native/atoms/src/Button/Button.tsx(2 hunks)suite-native/atoms/src/Input/SearchInput.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/atoms/src/__tests__/PriceChangeBadge.comp.test.tsx(2 hunks)suite-native/icons/src/CryptoIcon.tsx(2 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/icons/src/utils.ts(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-dev-utils/src/screens/DemoScreen.tsx(1 hunks)suite-native/module-trading/jest.config.js(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(1 hunks)suite-native/module-trading/src/components/buy/PaymentCard.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(1 hunks)suite-native/module-trading/src/components/general/PickerHeader.tsx(0 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetButton.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/FavouriteIcon.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingFooter.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/PickerHeader.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/__tests__/SelectTradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/__tests__/TradeableAssetButton.comp.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useAssetsSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeableAssetDominantColor.test.tsx(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/BasicProvider.tsx(0 hunks)suite-native/test-utils/src/BasicProviderForTests.tsx(1 hunks)suite-native/test-utils/src/StoreProviderForTests.tsx(2 hunks)suite-native/test-utils/src/index.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- suite-native/module-trading/src/components/general/tests/PickerHeader.comp.test.tsx
- suite-native/test-utils/src/BasicProvider.tsx
- suite-native/module-trading/src/components/general/PickerHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (34)
- suite-common/icons/generateIconFont.ts
- suite-native/icons/src/index.ts
- suite-native/module-trading/jest.config.js
- suite-native/test-utils/package.json
- suite-native/app/app.config.ts
- suite-native/module-trading/src/types.ts
- suite-common/suite-utils/src/hexToRgba.ts
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/FavouriteIcon.comp.test.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetDominantColor.ts
- suite-native/test-utils/src/StoreProviderForTests.tsx
- suite-native/icons/src/utils.ts
- suite-native/module-trading/src/hooks/tests/useAssetsSheetControls.test.ts
- suite-native/module-trading/src/components/general/tests/TradeableAssetButton.comp.test.tsx
- suite-native/module-trading/package.json
- suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx
- suite-native/module-trading/src/hooks/tests/useTradeableAssetDominantColor.test.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetsSheetHeader.comp.test.tsx
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx
- suite-native/module-trading/src/hooks/useTradeableAssetsSheetControls.ts
- packages/theme/src/spacings.ts
- suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
- suite-native/test-utils/src/BasicProviderForTests.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/tests/TradeableAssetListItem.comp.test.tsx
- suite-native/module-trading/src/components/general/tests/SelectTradeableAssetButton.comp.test.tsx
- suite-native/test-utils/src/index.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/atoms/src/tests/PriceChangeBadge.comp.test.tsx
- suite-native/module-dev-utils/src/screens/DemoScreen.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
- suite-native/atoms/src/Button/Button.tsx
🧰 Additional context used
📓 Learnings (1)
suite-native/module-trading/src/screens/TradingScreen.tsx (4)
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.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:16-18
Timestamp: 2025-02-04T19:20:55.214Z
Learning: Translation error handling should not be implemented in individual screen components like TradingScreen. This responsibility lies elsewhere in the component hierarchy.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:47-54
Timestamp: 2025-02-04T19:20:16.952Z
Learning: In visual stubs or prototypes, it's acceptable to use hardcoded placeholder values directly in the components, as they will be replaced with real data in subsequent implementations.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:11-14
Timestamp: 2025-02-04T19:21:19.263Z
Learning: When reviewing visual stubs or temporary implementations that will be replaced with real logic in future commits, skip suggesting improvements to logging, error handling, or other implementation details.
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Other Checks
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- 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 / 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: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- 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: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (35)
suite-native/module-trading/src/screens/TradingScreen.tsx (1)
10-21: LGTM! Clean and well-organized component structure.The component follows a clear hierarchy and good spacing practices. The Translation component is correctly used for internationalization.
suite-native/module-trading/src/components/buy/PaymentCard.tsx (2)
20-22: LGTM! Hardcoded values are acceptable for visual stub.Based on the retrieved learnings, using inline hardcoded values is acceptable in visual stubs that will be replaced with real data in future commits.
Also applies to: 28-30, 36-38
11-47: LGTM! Well-structured component with clear separation of concerns.The component follows good practices:
- Consistent use of translation keys
- Clear visual hierarchy with Card and TradingOverviewRow components
- Proper spacing with Box component
suite-native/atoms/src/Input/SearchInput.tsx (3)
1-1: LGTM! Type definitions and imports are well-structured.The renaming of
InputPropstoSearchInputPropsand the addition of new props improve the component's API clarity.Also applies to: 10-19
21-62: LGTM! Styles are well-organized and follow the design system.The styles properly handle focus states and elevation levels while maintaining consistency with the design system.
64-85: LGTM! Ref forwarding and default handlers are well-implemented.The use of
forwardRefanduseImperativeHandleis correct, and thenoOpfunction is a good practice for default event handlers.suite-native/module-trading/src/components/general/TradeableAssetsSheet/FavouriteIcon.tsx (2)
1-6: LGTM!The imports are well-organized and all are being used in the component.
7-10: LGTM!The type definition is clear and properly exported.
suite-native/module-trading/src/components/general/TradingFooter.tsx (1)
1-7: LGTM! Clean and well-organized imports.All imports are necessary and properly structured, following a clear pattern: React core, React Native core, and then internal modules.
suite-native/intl/src/en.ts (1)
1248-1275: LGTM! The new module structure follows established patterns.The new
moduleTradingmodule is well-organized and consistent with the existing translation structure.suite-native/module-trading/src/components/buy/BuyCard.tsx (3)
1-13: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are utilized in the component.
14-18: Track implementation of missing functionality.The
notImplementedCallbackis a placeholder that needs to be implemented with actual trading functionality.Would you like me to:
- Create an issue to track this implementation?
- Help implement the trading functionality?
Consider replacing the console.log with a proper error handling mechanism or user feedback in the meantime.
19-23: LGTM! Consistent styling approach.The styles follow the project's theming patterns and use appropriate theme variables.
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (16)
23-26: New Basic Icons Added
The icons"starFilled","star","stack", and"shuffle"have been added with sequential code points (61718–61721). Please verify that these codes do not conflict with any existing ones in the icon font generation workflow and that the visual assets match the design specifications.
27-31: New Warning and Network Icons
The additions"shieldWarning","shieldCheck","shareNetwork","question", and"qrCode"(61722–61726) follow the established pattern. Ensure these icons align with the updated design guidelines and are integrated properly in the consuming components.
32-36: Additional Action Icons
The new entries"prohibit","plusCircle","plus","plugs", and"piggyBankFilled"(61727–61731) are introduced consecutively. Confirm that the naming and numeric assignments are intentional and that the visual assets for these actions are current.
37-41: Banking and Editing Icons
The icons"piggyBank","pictureFrame","pencilSimpleLine","pencilSimple", and"pencil"(61732–61736) have been added. Their grouping suggests themes around financial and editing functionalities. Please double-check that these icons are used consistently throughout the UI.
42-46: Security and Design Elements
The signatures for"password","palette","magnifyingGlass","lock", and"link"(61737–61741) have been updated. Ensure that these icon names and code points accurately reflect the intended visual cues and security/design requirements.
47-51: Informational and Household Icons
With"lightbulb","lifebuoy","info","image", and"houseFilled"(61742–61746), the file now includes icons for conveying information and context. Verify that the styling for"houseFilled"versus any unfilled variant is clearly differentiated across the application.
52-56: Brand and Device-Related Icons
The new icons"house","handPalm","githubLogo","gearFilled", and"gear"(61747–61751) are now defined. It’s important to confirm that icons like"house"and"houseFilled"have distinct design representations and that platform logos such as"githubLogo"comply with branding guidelines.
57-62: Flag, Fingerprint & Document Icons
Introducing"flagCheckered","flag","fingerprintSimple","fingerprint","filePdf", and"facebookLogo"(61752–61757) extends the range to cover authentication, document types, and branding. Please ensure that there are no duplicate codes elsewhere and that these assets are up to date.
63-67: UI and Discovery Icons
The set"eyeSlash","eye","discoverFilled","discover", and"detective"(61758–61762) is added to enhance user interaction elements. Confirm that the visual differences between similarly named icons (e.g.,"eyeSlash"vs."eye") are clear and conform to the design system.
68-72: Technical and Commerce Icons
With"database","cpu","copy","coins", and"coinVerticalCheck"(61763–61767), the file now includes icons relevant to technical operations and financial transactions. Their sequential numeric assignment is correct; please verify integration with any related modules.
73-77: Code and Status Indicators
The icons"code","clockClockwise","circleDashed","checks", and"checkCircleFilled"(61768–61772) have been added. These icons likely serve for system status and debugging feedback. Confirm that they map correctly to the expected visual designs in both static and dynamic contexts.
78-82: Interaction and Notification Icons
New entries"checkCircle","check","chatCircle","change", and"caretUpFilled"(61773–61777) enrich the interaction vocabulary. It is advisable to ensure that icon variations such as"check"versus"checkCircle"remain distinguishable to end users.
83-88: Directional Caret Icons
The directional carets"caretUpDown","caretUp","caretRight","caretLeft","caretDownFilled", and"caretDown"(61778–61783) are now defined. Validate that the directional nuance and visual weights are consistent with UI expectations across different components.
89-94: Miscellaneous System Interaction Icons
The icons"caretCircleRight","calendar","bugBeetle","bookmarkSimple","backspace", and"arrowsLeftRight"(61784–61789) have been introduced. Their inclusion broadens system interaction options. Please verify their styling and that usage within the suite reflects the intended semantics.
95-100: Arrow Variation Icons (Part 1)
Icons such as"arrowsDownUp","arrowsCounterClockwise","arrowUpRight","arrowUp","arrowURightDown", and"arrowSquareOut"(61790–61795) extend directional indicators. Confirm that these variants are accurately documented and that the consuming modules have been updated to utilize them where necessary.
101-105: Arrow Variation Icons (Part 2)
Finally, the icons"arrowRight","arrowLineUpRight","arrowLineUp","arrowLineDown", and"arrowDown"(61796–61800) complete the arrow icon set. Ensure that no overlaps occur with similar variants and that these icons are integrated into the icon font generation process as expected.suite-native/module-trading/src/components/general/PickerCloseButton.tsx (4)
1-13: LGTM! Imports and type definitions are well organized.The imports are properly structured and the type definition correctly omits props that will be handled internally by the component.
15-19: LGTM! Constants are well-defined and properly placed.The constants are properly typed and placed at the top of the file as requested in the previous review.
25-50: Verify color scheme and consider accessibility.The implementation looks good, but please verify:
- Does the
tertiaryElevation0color scheme provide sufficient contrast with the gradient background?- Is the button height (
PICKER_BUTTON_HEIGHT) sufficient for touch targets on mobile devices?Would you like me to help implement accessibility testing for this component?
20-23: Verify the spacer height.The spacer height of
sp32seems quite large for a button component. Please verify if this spacing aligns with the design specifications.suite-native/icons/src/CryptoIcon.tsx (2)
5-6: LGTM! Clean import refactoring.The imports have been properly reorganized to support the new
getCryptoIconUrlimplementation while maintaining type safety.Also applies to: 9-9
38-41: Verify edge case handling in getCryptoIconUrl.The refactoring to use
getCryptoIconUrlimproves code maintainability. However, let's verify that it handles edge cases correctly.Let's check the implementation of
getCryptoIconUrl:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of getCryptoIconUrl and its edge case handling # Find and display the implementation ast-grep --pattern 'export function getCryptoIconUrl($_) { $$$ }' # Search for tests covering edge cases rg -A 5 'getCryptoIconUrl.*test'Length of output: 104
Let's gather more details on how
getCryptoIconUrlis implemented and tested. Please run the script below to search for its import, definition (both as a regular and arrow function), and any tests referencing it:
🏁 Script executed:
#!/bin/bash # Verify the implementation and testing of getCryptoIconUrl echo ">>> Searching for import statement in CryptoIcon.tsx:" rg -n "import.*getCryptoIconUrl" suite-native/icons/src/CryptoIcon.tsx echo "--------------------------------------------------" echo ">>> Searching for the regular function definition for getCryptoIconUrl:" rg -n "export function getCryptoIconUrl" echo "--------------------------------------------------" echo ">>> Searching for the arrow function definition for getCryptoIconUrl:" rg -n "export const getCryptoIconUrl" echo "--------------------------------------------------" echo ">>> Searching test files for getCryptoIconUrl references:" rg -n "getCryptoIconUrl" --type testLength of output: 1259
Let's gather additional details about the edge case handling by inspecting the implementation of
getCryptoIconUrlin its source file. Please run the script below to display the relevant implementation details and check for any conditional handling ofcontractAddressor similar edge cases:
🏁 Script executed:
#!/bin/bash # Display implementation details of getCryptoIconUrl and search for edge case handling in utils.ts echo ">>> Displaying first 150 lines of suite-native/icons/src/utils.ts:" sed -n '1,150p' suite-native/icons/src/utils.ts echo "--------------------------------------------------" echo ">>> Displaying the getCryptoIconUrl block (from definition until the first closing brace):" sed -n '/export const getCryptoIconUrl/,/}/p' suite-native/icons/src/utils.ts echo "--------------------------------------------------" echo ">>> Searching for conditional checks on contractAddress in utils.ts:" grep -R "if.*contractAddress" suite-native/icons/src/utils.tsLength of output: 2205
Edge case handling verified in getCryptoIconUrl.
The function checks for both a valid coingeckoId and a provided contractAddress before computing a new URL, otherwise it falls back to a default icon from the cryptoIcons mapping. This implementation correctly handles the edge cases.
| <TradeableAssetsSheet | ||
| isVisible={isTradeableAssetsSheetVisible} | ||
| onClose={hideTradeableAssetsSheet} | ||
| onAssetSelect={setSelectedTradeableAsset} | ||
| /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add error handling for asset selection.
Consider adding error handling for cases where asset selection fails or is cancelled.
🏁 Script executed:
#!/bin/bash
# Check if error handling exists in TradeableAssetsSheet
ast-grep --pattern 'onError($$$)' || ast-grep --pattern 'catch($$$)'Length of output: 1467
Asset selection error handling not implemented in BuyCard.tsx
The shell script confirms that while error handling (using constructs like onError or catch) is present elsewhere in the repository, this file’s use of the <TradeableAssetsSheet> component does not include any error handling for the onAssetSelect callback. Please add appropriate error handling to manage asset selection failures or cancellations.
jbazant
force-pushed
the
module-trade-ui-tokens
branch
from
623b02a to
aba62ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/intl/src/en.ts (1)
1274-1274: Consider moving the search label to a more generic location.The "Search" translation is generic and could be reused across different features. Consider moving it to the
genericsection to promote reusability and maintain DRY principles.- defaultSearchLabel: 'Search',Add to the
genericsection:generic: { + search: 'Search',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.ttfis excluded by!**/*.ttfsuite-native/module-trading/assets/InvityLogo.pngis excluded by!**/*.png
📒 Files selected for processing (8)
suite-common/icons/generateIconFont.ts(2 hunks)suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(1 hunks)suite-native/module-trading/src/components/buy/PaymentCard.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingFooter.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingRowDivider.tsx(0 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-trading/src/components/general/TradingRowDivider.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- suite-common/icons/generateIconFont.ts
- suite-native/module-trading/src/components/general/TradingFooter.tsx
- suite-native/module-trading/src/components/buy/PaymentCard.tsx
- suite-native/module-trading/src/components/buy/BuyCard.tsx
🧰 Additional context used
📓 Learnings (1)
suite-native/module-trading/src/screens/TradingScreen.tsx (4)
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.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:16-18
Timestamp: 2025-02-04T19:20:55.214Z
Learning: Translation error handling should not be implemented in individual screen components like TradingScreen. This responsibility lies elsewhere in the component hierarchy.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:47-54
Timestamp: 2025-02-04T19:20:16.952Z
Learning: In visual stubs or prototypes, it's acceptable to use hardcoded placeholder values directly in the components, as they will be replaced with real data in subsequent implementations.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:11-14
Timestamp: 2025-02-04T19:21:19.263Z
Learning: When reviewing visual stubs or temporary implementations that will be replaced with real logic in future commits, skip suggesting improvements to logging, error handling, or other implementation details.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
suite-native/intl/src/en.ts (3)
1250-1259: LGTM! Clear and concise trading screen translations.The translations are well-structured and follow common trading UI patterns.
1261-1263: LGTM! Clear button label for coin selection.The translation is concise and reusable.
1264-1273: LGTM! Comprehensive translations for the tradeable assets sheet.The translations effectively cover all UI states including favorites management and empty state handling. The empty state message is particularly well-crafted, providing clear guidance to users.
suite-native/module-trading/src/screens/TradingScreen.tsx (2)
1-8: LGTM!The imports are well-organized and all imported components are utilized in the implementation.
10-21: LGTM! Clean implementation of the visual stub.The component structure is clean and follows React Native best practices. The implementation appropriately uses the Screen component with proper layout and spacing.
Note: As per the PR objectives, this is a visual stub for the token picker modal that will be enhanced in future commits.
suite-common/icons/iconFontsMobile/TrezorSuiteIcons.json (1)
23-105: Verify Icon Naming & Codepoints ConsistencyThis block adds multiple new icon entries (e.g.,
"starFilled","star","stack","shuffle", etc.) with sequential codepoints. Please ensure that:
- The new icon names follow the established naming conventions throughout the project.
- Their assigned numeric codepoints are unique and do not conflict with any previously existing icons.
- All parts of the icon generation system (for example, updates in
generateIconFont.ts) properly reference these new icons.Additionally, consider updating documentation or inline comments to highlight any changes in the icon set that may affect design or usage in the mobile token picker modal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (2)
19-23: Define explicit type for the style object.The empty object type
{}should be replaced with an explicit interface or type definition.-const wrapperStyle = prepareNativeStyle<{}>(({ spacings }) => ({ +interface WrapperStyle { + height: number; + padding: number; + gap: number; +} +const wrapperStyle = prepareNativeStyle<WrapperStyle>(({ spacings }) => ({🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
16-17: Consider extracting animation configuration.Since the animation duration is used in multiple places, consider extracting it to a shared constants file for better maintainability and reusability across other components.
+// In a shared constants file (e.g., animationConstants.ts) +export const ANIMATION_DURATIONS = { + FOCUS: 300, + // Add other durations here +} as const; -const FOCUS_ANIMATION_DURATION = 300 as const; +const FOCUS_ANIMATION_DURATION = ANIMATION_DURATIONS.FOCUS;suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx (2)
8-13: Consider making the accessibility label configurable.The component could be used for different purposes, but the accessibility label is hardcoded to "Close". Consider adding an optional
accessibilityLabelprop to make it more reusable.export type SheetHeaderTitleProps = { children: ReactNode; + accessibilityLabel?: string; } & RequireAllOrNone<{ onLeftButtonPress: () => void; leftButtonIcon: IconName; }>;
22-33: Extract magic flex values into named constants.The flex values (1 and 3) are magic numbers. Consider extracting them into named constants at the top of the file to improve maintainability and make their purpose clear.
+const SIDE_FLEX = 1; +const TITLE_FLEX = 3; + export const SheetHeaderTitle = ({ onLeftButtonPress, leftButtonIcon, children, }: SheetHeaderTitleProps) => ( <HStack alignItems="center" justifyContent="space-between"> {leftButtonIcon && ( - <Box flex={1}> + <Box flex={SIDE_FLEX}> <IconButton iconName={leftButtonIcon} onPress={onLeftButtonPress} colorScheme="tertiaryElevation0" size="medium" accessibilityRole="button" accessibilityLabel="Close" /> </Box> )} - <Box flex={3}> + <Box flex={TITLE_FLEX}>suite-native/atoms/src/Sheet/BottomSheetGrabber.tsx (1)
12-20: Consider accessibility improvements.While the implementation is clean, consider adding accessibility props to improve user experience.
export const BottomSheetGrabber = () => { const { applyStyle } = useNativeStyles(); return ( - <Box flex={1} alignItems="center"> + <Box + flex={1} + alignItems="center" + accessibilityRole="button" + accessibilityLabel="Bottom sheet grabber" + accessibilityHint="Drag to resize bottom sheet" + > <Box style={applyStyle(bottomSheetGrabberStyle)} /> </Box> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx(4 hunks)suite-native/atoms/src/Sheet/BottomSheetGrabber.tsx(1 hunks)suite-native/atoms/src/Sheet/BottomSheetHeader.tsx(1 hunks)suite-native/atoms/src/index.ts(1 hunks)suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx(1 hunks)suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
- suite-native/atoms/src/Sheet/BottomSheetFlashList.tsx
- suite-native/module-trading/src/components/general/SearchInputWithCancel.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
🧰 Additional context used
📓 Learnings (1)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (2)
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx:17-28
Timestamp: 2025-02-04T13:18:46.084Z
Learning: The mock data in TradeableAssetsFilterTabs.tsx is part of a visual stub and will be replaced with dynamic data in future commits.
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx:0-0
Timestamp: 2025-02-04T13:18:21.530Z
Learning: The mock data arrays in `suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx` are intentionally used as visual stubs and will be replaced with real data fetching in future commits.
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (11)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (2)
1-15: LGTM!The imports are well-organized, and the type definition is clear and concise.
34-45: Animation implementation looks good!The layout animations are properly implemented using react-native-reanimated's LinearTransition with consistent duration across all animated views. This addresses the animation requirements discussed in the past review comments.
Also applies to: 46-53, 54-59
suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx (1)
15-40: LGTM! Well-structured component with good accessibility.The component is well-implemented with:
- Proper use of flex layout for responsive design
- Good handling of conditional rendering
- Appropriate accessibility attributes
- Symmetric layout with placeholder space
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsFilterTabs.tsx (4)
1-15: LGTM! Clean imports and well-defined types.The imports are appropriate, and the type definitions are clear and include necessary accessibility props.
17-28: Mock data acknowledged as visual stub.As confirmed in the learnings, this mock data will be replaced with dynamic data in future commits.
30-44: LGTM! Well-implemented tab component with proper accessibility.The component has clean styling logic and correctly implements accessibility features.
46-73: Add test coverage for the component.While the implementation looks good, test coverage would help ensure the component's reliability, especially for:
- Visibility toggle behavior
- Tab selection functionality
- Animation triggers
Would you like me to help generate test cases for this component?
suite-native/atoms/src/Sheet/BottomSheetGrabber.tsx (1)
5-10: LGTM! Well-defined style using theme utilities.The style definition is clean and follows best practices by:
- Using theme utilities for colors and borders
- Defining fixed dimensions appropriate for touch targets
- Using borderRadius from theme for consistency
suite-native/atoms/src/index.ts (1)
22-22: LGTM! Export follows package conventions.The new export is correctly placed with other Sheet-related exports and maintains alphabetical ordering.
suite-native/atoms/src/Sheet/BottomSheetHeader.tsx (2)
8-9: LGTM! Clean import organization.Imports are well-organized and follow the package's conventions.
60-64: Consider conditional rendering of BottomSheetGrabber.The grabber is always rendered, even when the header is not displayed. Consider making it conditional based on use case.
Would the grabber still be needed when
isHeaderDisplayedis false? Please clarify the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
suite-native/module-trading/src/navigation/__tests__/TradingStackNavigator.comp.test.tsx (1)
7-8: LGTM! Correct usage of async/await with waitFor.The test correctly uses async/await pattern with waitFor to handle asynchronous rendering.
However, consider making the test more resilient to implementation changes:
- await waitFor(() => expect(getAllByText('Buy').length).toBe(2)); + await waitFor(() => { + const buyElements = getAllByText('Buy'); + expect(buyElements.length).toBeGreaterThan(0); + // Optional: Verify the context/purpose of each 'Buy' text + buyElements.forEach(element => { + expect(element).toBeVisible(); + }); + });suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx (1)
9-47: Good test coverage, with room for improvements.The test cases are well-structured and cover key functionality. Consider these improvements:
- Make test descriptions more consistent (e.g., remove "should" prefix for consistency with BDD style)
- Add negative test cases (e.g., verify cancel button disappears on blur)
- Extract hardcoded strings into constants
Here's a suggested refactor:
+const TEST_IDS = { + SEARCH_INPUT: 'Search', + CANCEL_BUTTON: 'Cancel', + CLOSE_BUTTON: 'Close', +}; +const LABELS = { + COINS: 'Coins', + ALL: 'All', +}; describe('TradeableAssetsSheetHeader', () => { const renderComponent = (onClose = jest.fn()) => render(<TradeableAssetsSheetHeader onClose={onClose} />); - it('should display "Coins" and do not display tabs by default', () => { + it('displays Coins label and hides tabs by default', () => { const { getByText, queryByText } = renderComponent(); - expect(getByText('Coins')).toBeDefined(); - expect(queryByText('All')).toBeNull(); + expect(getByText(LABELS.COINS)).toBeDefined(); + expect(queryByText(LABELS.ALL)).toBeNull(); }); - it('should display tabs after focusing search input', () => { + it('displays tabs and hides Coins label when search is focused', () => { const { getByPlaceholderText, getByText, queryByText } = renderComponent(); - fireEvent(getByPlaceholderText('Search'), 'focus'); + fireEvent(getByPlaceholderText(TEST_IDS.SEARCH_INPUT), 'focus'); - expect(getByText('All')).toBeDefined(); - expect(queryByText('Coins')).toBeNull(); + expect(getByText(LABELS.ALL)).toBeDefined(); + expect(queryByText(LABELS.COINS)).toBeNull(); }); // ... similar changes for other tests ... + it('hides cancel button when search is blurred', () => { + const { getByPlaceholderText, queryByText } = renderComponent(); + + const searchInput = getByPlaceholderText(TEST_IDS.SEARCH_INPUT); + fireEvent(searchInput, 'focus'); + fireEvent(searchInput, 'blur'); + + expect(queryByText(TEST_IDS.CANCEL_BUTTON)).toBeNull(); + }); });suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx (1)
16-42: Consider using constants for flex values.The implementation looks good, but the flex values could be extracted into constants for better maintainability.
+const SIDE_FLEX = 1; +const TITLE_FLEX = 3; + export const SheetHeaderTitle = ({ onLeftButtonPress, leftButtonIcon, leftButtonA11yLabel, children, }: SheetHeaderTitleProps) => ( <HStack alignItems="center" justifyContent="space-between"> {leftButtonIcon && ( - <Box flex={1}> + <Box flex={SIDE_FLEX}> <IconButton iconName={leftButtonIcon} onPress={onLeftButtonPress} colorScheme="tertiaryElevation0" size="medium" accessibilityRole="button" accessibilityLabel={leftButtonA11yLabel} /> </Box> )} - <Box flex={3}> + <Box flex={TITLE_FLEX}> <Text variant="highlight" textAlign="center" ellipsizeMode="tail" numberOfLines={1}> {children} </Text> </Box> - {leftButtonIcon && <Box flex={1} />} + {leftButtonIcon && <Box flex={SIDE_FLEX} />} </HStack> );suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (3)
12-17: Consider moving animation duration to a shared constants file.The
FOCUS_ANIMATION_DURATIONcould be reused across other components that need similar animations.Consider moving it to a shared constants file:
// src/constants/animations.ts export const FOCUS_ANIMATION_DURATION = 300 as const;
19-23: Define explicit type for style object.Address the static analysis warning by explicitly defining the object shape.
-const wrapperStyle = prepareNativeStyle<{}>(({ spacings }) => ({ +type WrapperStyleType = { + height: number; + padding: number; + gap: number; +}; + +const wrapperStyle = prepareNativeStyle<WrapperStyleType>(({ spacings }) => ({ height: HEADER_HEIGHT, padding: spacings.sp16, gap: spacings.sp16, }));🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
25-67: Consider debouncing filter value changes.To optimize performance, consider debouncing the filter value changes to prevent excessive updates.
+import { useMemo } from 'react'; +import { debounce } from 'lodash'; + export const TradeableAssetsSheetHeader = ({ onClose }: TradeableAssetsSheetHeaderProps) => { const { applyStyle } = useNativeStyles(); const { translate } = useTranslate(); const [isFilterActive, setIsFilterActive] = useState(false); const [filterValue, setFilterValue] = useState(''); + const debouncedSetFilterValue = useMemo( + () => debounce((value: string) => setFilterValue(value), 300), + [] + ); + return ( // ... rest of the component <SearchInputWithCancel - onChange={setFilterValue} + onChange={debouncedSetFilterValue} onFocus={() => setIsFilterActive(true)} onBlur={() => setIsFilterActive(false)} value={filterValue} /> // ... rest of the component ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
suite-native/accounts/src/components/AccountSelectBottomSheet.tsx(0 hunks)suite-native/module-trading/src/components/general/PickerCloseButton.tsx(0 hunks)suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx(1 hunks)suite-native/module-trading/src/navigation/__tests__/TradingStackNavigator.comp.test.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- suite-native/module-trading/src/components/general/PickerCloseButton.tsx
- suite-native/accounts/src/components/AccountSelectBottomSheet.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- 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: 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: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
🔇 Additional comments (2)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetsSheetHeader.comp.test.tsx (1)
1-8: LGTM! Well-structured test setup.The test setup follows good practices with minimal imports and a clean helper function for component rendering.
suite-native/module-trading/src/components/general/SheetHeaderTitle.tsx (1)
8-14: LGTM! Props type definition is well-structured.The use of
RequireAllOrNoneensures proper type safety by requiring all or none of the left button props.
| describe('TradingStackNavigator', () => { | ||
| it('should render', async () => { | ||
| const { getByText } = renderWithStore(<TradingStackNavigator />); | ||
| await waitFor(() => expect(getByText('Trading placeholder')).toBeDefined()); | ||
| const { getAllByText } = renderWithStore(<TradingStackNavigator />); | ||
| await waitFor(() => expect(getAllByText('Buy').length).toBe(2)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more test cases for comprehensive coverage.
The navigator should be tested for:
- Navigation between screens
- Error states
- Loading states
- Deep linking scenarios
Example additional test:
it('should navigate to token picker when triggered', async () => {
const { getByText, findByTestId } = renderWithStore(<TradingStackNavigator />);
// Trigger navigation to token picker
await waitFor(() => {
const buyButton = getByText('Buy');
fireEvent.press(buyButton);
});
// Verify navigation occurred
const tokenPicker = await findByTestId('token-picker');
expect(tokenPicker).toBeVisible();
});
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
suite-native/module-trading/src/components/buy/TradeableAssetPicker.tsx (1)
1-6: LGTM! Consider adding JSDoc for better documentation.The imports are well-organized and the type definition elegantly leverages TypeScript's ReturnType utility type. This approach ensures type safety and reduces maintenance overhead.
Consider adding JSDoc documentation for the
TradeableAssetPickerPropstype:+/** + * Props for the TradeableAssetPicker component. + * Derived from the return type of useTradeSheetControls hook with TradeableAsset type parameter. + */ type TradeableAssetPickerProps = ReturnType<typeof useTradeSheetControls<TradeableAsset>>;suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (3)
10-12: Consider adding JSDoc comments to the type definition.Adding documentation to the props type would improve code maintainability and help other developers understand the component's API better.
+/** + * Props for the TradeableAssetsSheetHeader component + * @property {() => void} onClose - Callback function triggered when the header close action is initiated + */ type TradeableAssetsSheetHeaderProps = { onClose: () => void; };
14-14: Document the header height constant.Add a comment explaining why this specific height value was chosen to help future maintainers understand the design decision.
+// Fixed height for the header component including space for the search input and filter tabs const HEADER_HEIGHT = 160;
23-23: Consider adding error handling and debouncing.Two suggestions for improved robustness:
- Wrap the onClose callback in a try-catch block.
- Consider debouncing the filter focus changes to prevent rapid state updates.
+import { useCallback } from 'react'; +import debounce from 'lodash/debounce'; export const TradeableAssetsSheetHeader = ({ onClose }: TradeableAssetsSheetHeaderProps) => { const { applyStyle } = useNativeStyles(); const [isFilterActive, setIsFilterActive] = useState(false); + const handleClose = useCallback(() => { + try { + onClose(); + } catch (error) { + console.error('Failed to close sheet:', error); + } + }, [onClose]); + + const handleFilterFocusChange = useCallback( + debounce((isActive: boolean) => { + setIsFilterActive(isActive); + }, 150), + [] + ); return ( <SearchableSheetHeader - onClose={onClose} + onClose={handleClose} title={<Translation id="moduleTrading.tradeableAssetsSheet.title" />} - onFilterFocusChange={setIsFilterActive} + onFilterFocusChange={handleFilterFocusChange} style={applyStyle(wrapperStyle)} >Also applies to: 29-29
suite-native/module-trading/src/hooks/useTradeSheetControls.ts (1)
3-22: LGTM! Well-structured custom hook.The hook follows React best practices with clean state management and single responsibility.
Consider adding a reset function to clear both states at once:
export const useTradeSheetControls = <T>() => { const [isSheetVisible, setIsSheetVisible] = useState(false); const [selectedValue, setSelectedValue] = useState<undefined | T>(); const showSheet = () => { setIsSheetVisible(true); }; const hideSheet = () => { setIsSheetVisible(false); }; + const reset = () => { + setIsSheetVisible(false); + setSelectedValue(undefined); + }; return { isSheetVisible, showSheet, hideSheet, selectedValue, setSelectedValue, + reset, }; };suite-native/module-trading/src/components/buy/PaymentCard.tsx (3)
21-23: Move hardcoded text to translation keys.Consider moving the hardcoded "Credit card" text to a translation key for consistency with other translated strings.
<Text color="textSubdued" variant="body"> - Credit card + <Translation id="moduleTrading.tradingScreen.creditCard" /> </Text>
31-33: Move hardcoded text to translation keys.Consider moving the hardcoded "Anycoin" text to a translation key for consistency with other translated strings.
<Text color="textSubdued" variant="body"> - Anycoin + <Translation id="moduleTrading.tradingScreen.provider.anycoin" /> </Text>
7-10: Move notImplementedCallback to shared utilities.Consider moving the
notImplementedCallbackto a shared utility file since it's likely to be reused across components.-const notImplementedCallback = () => { - // eslint-disable-next-line no-console - console.log('Not implemented'); -};Create a new file
suite-native/module-trading/src/utils/callbacks.ts:export const notImplementedCallback = () => { // eslint-disable-next-line no-console console.log('Not implemented'); };suite-native/module-trading/src/hooks/__tests__/useTradeSheetControls.test.ts (1)
35-51: Consider adding edge case tests for selectedTradeableAsset.While the basic functionality is tested, consider adding tests for:
- Setting value to null
- Setting value to empty string
- Setting value to undefined explicitly
describe('selectedTradeableAsset', () => { it('should be undefined by default', () => { const { result } = renderHook(() => useTradeSheetControls()); expect(result.current.selectedValue).toBeUndefined(); }); it('should be set after setSelectedTradeableAsset call', () => { const { result } = renderHook(() => useTradeSheetControls()); act(() => { result.current.setSelectedValue('btc'); }); expect(result.current.selectedValue).toBe('btc'); }); + + it('should handle null value', () => { + const { result } = renderHook(() => useTradeSheetControls<string | null>()); + + act(() => { + result.current.setSelectedValue(null); + }); + + expect(result.current.selectedValue).toBeNull(); + }); + + it('should handle empty string', () => { + const { result } = renderHook(() => useTradeSheetControls<string>()); + + act(() => { + result.current.setSelectedValue(''); + }); + + expect(result.current.selectedValue).toBe(''); + }); + + it('should handle explicit undefined', () => { + const { result } = renderHook(() => useTradeSheetControls<string>()); + + act(() => { + result.current.setSelectedValue('btc'); + result.current.setSelectedValue(undefined); + }); + + expect(result.current.selectedValue).toBeUndefined(); + }); });suite-native/intl/src/en.ts (1)
1249-1281: LGTM! Well-structured translations for the trading module.The translations are organized logically and follow consistent naming patterns.
Consider adding JSDoc comments to document the structure:
moduleTrading: { + /** + * Main trading screen translations + */ tradingScreen: { buyTitle: 'Buy', // ... }, + /** + * Coin selection related translations + */ selectCoin: { buttonTitle: 'Select coin', }, + /** + * Tradeable assets sheet translations + */ tradeableAssetsSheet: { // ... }, + /** + * Country selection sheet translations + */ countrySheet: { // ... }, },suite-native/module-trading/src/components/general/TradingEmptyComponent.tsx (1)
18-31: Consider adding accessibility props.The component could benefit from accessibility improvements for screen readers.
export const TradingEmptyComponent = ({ title, description }: TradingEmptyComponentProps) => { const { applyStyle } = useNativeStyles(); return ( - <VStack style={applyStyle(emptyComponentStyle)}> + <VStack + style={applyStyle(emptyComponentStyle)} + accessible={true} + accessibilityRole="alert" + accessibilityLiveRegion="polite" + > <Text variant="body" color="textDefault" textAlign="center"> {title} </Text>suite-native/module-trading/src/components/general/CountrySheet/CountryListItem.tsx (1)
8-14: Consider improving type safety for flag icons.The
flagprop type could be more specific to ensure only valid flag icons are used.+type CountryFlagIconName = Extract<IconName, 'flag' | 'flagCheckered'>; + export type CountryListItemProps = { - flag: IconName; + flag: CountryFlagIconName; id: string; name: ReactNode; isSelected: boolean; onPress: () => void; };suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx (3)
23-26: Define explicit object shape for style type parameter.The empty object type
{}should be replaced with an explicit interface to improve type safety.-const wrapperStyle = prepareNativeStyle<{}> +interface WrapperStyleProps {} +const wrapperStyle = prepareNativeStyle<WrapperStyleProps>🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
19-19: Add documentation for the animation duration constant.Consider adding a comment explaining why 300ms was chosen as the animation duration and what visual effect it achieves.
+// Duration of 300ms provides a smooth transition that feels responsive while still being noticeable export const FOCUS_ANIMATION_DURATION = 300 as const;
69-74: Add customizable placeholder text for search input.Consider adding a prop to customize the search input's placeholder text for better reusability across different contexts.
export type SearchableSheetHeaderProps = { onClose: () => void; title: ReactNode; onFilterFocusChange?: (isFilterActive: boolean) => void; children?: ReactNode; style?: NativeStyleObject; + searchPlaceholder?: string; }; export const SearchableSheetHeader = ({ onClose, title, children, onFilterFocusChange = noOp, style, + searchPlaceholder, }: SearchableSheetHeaderProps) => { // ... <SearchInputWithCancel onChange={setFilterValue} onFocus={() => changeFilterFocus(true)} onBlur={() => changeFilterFocus(false)} value={filterValue} + placeholder={searchPlaceholder} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(1 hunks)suite-native/module-trading/src/components/buy/CountryOfResidencePicker.tsx(1 hunks)suite-native/module-trading/src/components/buy/PaymentCard.tsx(1 hunks)suite-native/module-trading/src/components/buy/TradeableAssetPicker.tsx(1 hunks)suite-native/module-trading/src/components/general/CountrySheet/CountryListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/CountrySheet/CountryListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/CountrySheet/CountrySheet.tsx(1 hunks)suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useTradeSheetControls.test.ts(1 hunks)suite-native/module-trading/src/hooks/useTradeSheetControls.ts(1 hunks)suite-native/module-trading/src/screens/TradingScreen.tsx(1 hunks)suite-native/module-trading/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-native/module-trading/src/components/buy/BuyCard.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeAssetsListEmptyComponent.tsx
🧰 Additional context used
📓 Learnings (2)
suite-native/module-trading/src/components/buy/TradeableAssetPicker.tsx (1)
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx:0-0
Timestamp: 2025-02-04T13:18:21.530Z
Learning: The mock data arrays in `suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx` are intentionally used as visual stubs and will be replaced with real data fetching in future commits.
suite-native/module-trading/src/screens/TradingScreen.tsx (4)
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.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:16-18
Timestamp: 2025-02-04T19:20:55.214Z
Learning: Translation error handling should not be implemented in individual screen components like TradingScreen. This responsibility lies elsewhere in the component hierarchy.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:47-54
Timestamp: 2025-02-04T19:20:16.952Z
Learning: In visual stubs or prototypes, it's acceptable to use hardcoded placeholder values directly in the components, as they will be replaced with real data in subsequent implementations.
Learnt from: jbazant
PR: trezor/trezor-suite#16829
File: suite-native/module-trading/src/screens/TradingScreen.tsx:11-14
Timestamp: 2025-02-04T19:21:19.263Z
Learning: When reviewing visual stubs or temporary implementations that will be replaced with real logic in future commits, skip suggesting improvements to logging, error handling, or other implementation details.
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx
[error] 23-23: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: PR-check / node-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: 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=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: connect-web-e2e
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (11)
suite-native/module-trading/src/components/buy/TradeableAssetPicker.tsx (1)
8-23: LGTM! Consider adding error handling for edge cases.The component implementation is clean, focused, and follows React best practices. Props are properly typed and the component has a clear single responsibility.
Consider handling these edge cases:
- What happens if
selectedValueis undefined?- Should we prevent opening the sheet in certain conditions?
You might want to add prop validation or default props:
export const TradeableAssetPicker = ({ isSheetVisible, showSheet, hideSheet, selectedValue, setSelectedValue, }: TradeableAssetPickerProps) => { + // Prevent opening sheet if no assets are available + const handleShowSheet = () => { + if (/* check condition */) { + showSheet(); + } + }; return ( <> - <SelectTradeableAssetButton onPress={showSheet} selectedAsset={selectedValue} /> + <SelectTradeableAssetButton + onPress={handleShowSheet} + selectedAsset={selectedValue} + disabled={/* condition */} + /> <TradeableAssetsSheet isVisible={isSheetVisible} onClose={hideSheet} onAssetSelect={setSelectedValue} /> </> ); };suite-native/module-trading/src/types.ts (3)
1-4: LGTM! Clean import structure.The imports are well-organized and properly scoped from their respective packages.
5-10: Consider implementing TokenInfoBranded now to avoid future refactoring.The comment suggests using
TokenInfoBrandedin production. If this is a temporary solution, it might be better to implement the production-ready type now to avoid future refactoring.Let's check if
TokenInfoBrandedis already defined:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for TokenInfoBranded definition rg "TokenInfoBranded" -A 5Length of output: 6886
Token Type Consistency in TradeableAsset Definition
The current implementation in the token picker stub uses a simplified TradeableAsset type intentionally for UI purposes, with a clear note indicating production code should likely use TokenInfoBranded. Since TokenInfoBranded is defined (e.g., in suite-common/wallet-types) and used in other parts of the codebase, deferring its integration to production is acceptable for this stub.
- TradeableAsset remains minimal for the modal's visual stub.
- A note exists to replace it with TokenInfoBranded when moving to production.
- TokenInfoBranded is available and in use in other modules, so its future adoption is feasible.
12-16: Consider using a more specific type for country IDs.The
idfield usingstringtype might be too permissive. Consider:
- Using ISO 3166-1 alpha-2 or alpha-3 country codes
- Creating a union type of valid country codes for better type safety
Also, verify that all required country flags are included in the IconName type.
Let's check the IconName type definition:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for IconName type definition and available country flags ast-grep --pattern 'type IconName = $$$'Length of output: 405
Consider narrowing the type for country IDs and ensuring that only valid flag icons are used for the flag field.
Our investigation shows that the IconName type is declared in several files as a union of keys taken from icon objects (e.g., using "keyof typeof icons" or "keyof typeof codepoints"). This dynamic approach means IconName encompasses all available icons rather than being limited strictly to country flag icons. If your intent is to restrict the Country type’s flag property to valid country flags only, you might need a dedicated type (or union type) listing only those flag icons. Also, to improve type safety the id field, consider using a specific union of ISO country codes rather than a general string.
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (1)
32-37: Great job implementing the layout animation!The animation implementation looks good and addresses the previous review comments. The use of
LinearTransitionwith a consistent duration provides a smooth user experience.suite-native/module-trading/src/screens/TradingScreen.tsx (1)
15-29: LGTM! Clean component structure.The component follows React Native best practices with proper spacing and internationalization.
suite-native/module-trading/src/components/buy/CountryOfResidencePicker.tsx (2)
10-43: LGTM! Well-structured component.The component follows React best practices with clean state management using the custom hook.
35-40: Ensure type safety for selectedCountryId prop.The
selectedValue?.idcould be undefined. Verify that theCountrySheetcomponent properly handles undefined values for theselectedCountryIdprop.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if CountrySheet component handles undefined selectedCountryId # Search for the CountrySheet component's prop types ast-grep --pattern 'interface $name { $$$ selectedCountryId: $_ $$$ }' # Search for type assertions or checks on selectedCountryId rg -A 5 'selectedCountryId'Length of output: 2653
Type safety for selectedCountryId is properly maintained.
The CountrySheet component defines the property as an optional string (selectedCountryId?: string) and uses it in a context (strict equality check: item.id === selectedCountryId) that safely handles undefined values. No changes are needed.suite-native/module-trading/src/hooks/__tests__/useTradeSheetControls.test.ts (1)
5-33: LGTM! Well-structured test suite for sheet visibility.The tests for
isSheetVisibleare comprehensive and follow the Arrange-Act-Assert pattern.suite-native/module-trading/src/components/general/CountrySheet/CountryListEmptyComponent.tsx (1)
1-10: LGTM! Well-structured empty state component.The component follows good practices:
- Reuses the generic TradingEmptyComponent
- Properly handles internationalization
- Follows single responsibility principle
suite-native/module-trading/src/components/general/TradingEmptyComponent.tsx (1)
6-9: LGTM! Well-defined props interface.Props are properly typed with ReactNode allowing for flexible content.
| const wrapperStyle = prepareNativeStyle<{}>(() => ({ | ||
| height: HEADER_HEIGHT, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the empty object type in prepareNativeStyle.
The static analysis tool correctly flags the use of {} as a type. Let's define a proper type for the style object.
-const wrapperStyle = prepareNativeStyle<{}>(() => ({
+type WrapperStyleProps = {
+ height: number;
+};
+
+const wrapperStyle = prepareNativeStyle<WrapperStyleProps>(() => ({
height: HEADER_HEIGHT,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wrapperStyle = prepareNativeStyle<{}>(() => ({ | |
| height: HEADER_HEIGHT, | |
| })); | |
| type WrapperStyleProps = { | |
| height: number; | |
| }; | |
| const wrapperStyle = prepareNativeStyle<WrapperStyleProps>(() => ({ | |
| height: HEADER_HEIGHT, | |
| })); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| export const CountryListItem = ({ flag, name, onPress, id, isSelected }: CountryListItemProps) => { | ||
| const { applyStyle } = useNativeStyles(); | ||
|
|
||
| return ( | ||
| <Pressable onPress={onPress} style={applyStyle(wrapperStyle)}> | ||
| <Card> | ||
| <HStack alignItems="center" justifyContent="space-between"> | ||
| <HStack> | ||
| <Icon name={flag} size="medium" /> | ||
| <Text variant="body" color="textDefault"> | ||
| {name} | ||
| </Text> | ||
| </HStack> | ||
| <Radio value={id} onPress={onPress} isChecked={isSelected} /> | ||
| </HStack> | ||
| </Card> | ||
| </Pressable> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility support for interactive elements.
The component should include proper accessibility props for better screen reader support.
export const CountryListItem = ({ flag, name, onPress, id, isSelected }: CountryListItemProps) => {
const { applyStyle } = useNativeStyles();
return (
- <Pressable onPress={onPress} style={applyStyle(wrapperStyle)}>
+ <Pressable
+ onPress={onPress}
+ style={applyStyle(wrapperStyle)}
+ accessible={true}
+ accessibilityRole="radio"
+ accessibilityState={{ checked: isSelected }}
+ accessibilityLabel={`Select ${name}`}
+ >
<Card>
<HStack alignItems="center" justifyContent="space-between">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const CountryListItem = ({ flag, name, onPress, id, isSelected }: CountryListItemProps) => { | |
| const { applyStyle } = useNativeStyles(); | |
| return ( | |
| <Pressable onPress={onPress} style={applyStyle(wrapperStyle)}> | |
| <Card> | |
| <HStack alignItems="center" justifyContent="space-between"> | |
| <HStack> | |
| <Icon name={flag} size="medium" /> | |
| <Text variant="body" color="textDefault"> | |
| {name} | |
| </Text> | |
| </HStack> | |
| <Radio value={id} onPress={onPress} isChecked={isSelected} /> | |
| </HStack> | |
| </Card> | |
| </Pressable> | |
| ); | |
| }; | |
| export const CountryListItem = ({ flag, name, onPress, id, isSelected }: CountryListItemProps) => { | |
| const { applyStyle } = useNativeStyles(); | |
| return ( | |
| <Pressable | |
| onPress={onPress} | |
| style={applyStyle(wrapperStyle)} | |
| accessible={true} | |
| accessibilityRole="radio" | |
| accessibilityState={{ checked: isSelected }} | |
| accessibilityLabel={`Select ${name}`} | |
| > | |
| <Card> | |
| <HStack alignItems="center" justifyContent="space-between"> | |
| <HStack> | |
| <Icon name={flag} size="medium" /> | |
| <Text variant="body" color="textDefault"> | |
| {name} | |
| </Text> | |
| </HStack> | |
| <Radio value={id} onPress={onPress} isChecked={isSelected} /> | |
| </HStack> | |
| </Card> | |
| </Pressable> | |
| ); | |
| }; |
| const onCountrySelectCallback = (country: Country) => { | ||
| onCountrySelect(country); | ||
| onClose(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for country selection.
The callback should include error handling in case the selection fails.
const onCountrySelectCallback = (country: Country) => {
+ try {
onCountrySelect(country);
onClose();
+ } catch (error) {
+ // Handle error appropriately
+ console.error('Failed to select country:', error);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onCountrySelectCallback = (country: Country) => { | |
| onCountrySelect(country); | |
| onClose(); | |
| }; | |
| const onCountrySelectCallback = (country: Country) => { | |
| try { | |
| onCountrySelect(country); | |
| onClose(); | |
| } catch (error) { | |
| // Handle error appropriately | |
| console.error('Failed to select country:', error); | |
| } | |
| }; |
| handleComponent={() => ( | ||
| <SearchableSheetHeader | ||
| onClose={onClose} | ||
| title={<Translation id="moduleTrading.countrySheet.title" />} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement search functionality.
The SearchableSheetHeader is used but search functionality is not implemented.
🏁 Script executed:
#!/bin/bash
# Check if search functionality exists in SearchableSheetHeader
rg -A 5 "SearchableSheetHeader.*onSearch.*" || echo "Search functionality not found"Length of output: 116
Search functionality is missing in CountrySheet.tsx usage of SearchableSheetHeader.
Our verification confirmed that there is no implementation of search functionality (e.g., no "onSearch" prop or related logic) in the SearchableSheetHeader component usage. Please implement the required search functionality to meet the intent of the code review.
| const mockCountries: Country[] = [ | ||
| { id: 'us', name: 'United States', flag: 'flag' }, | ||
| { id: 'cz', name: 'Czech Republic', flag: 'flagCheckered' }, | ||
| { id: 'sk', name: 'Slovakia', flag: 'flag' }, | ||
| { id: 'de', name: 'Germany', flag: 'flagCheckered' }, | ||
| { id: 'fr', name: 'France', flag: 'flag' }, | ||
| { id: 'es', name: 'Spain', flag: 'flagCheckered' }, | ||
| { id: 'it', name: 'Italy', flag: 'flag' }, | ||
| { id: 'pl', name: 'Poland', flag: 'flagCheckered' }, | ||
| { id: 'hu', name: 'Hungary', flag: 'flag' }, | ||
| { id: 'at', name: 'Austria', flag: 'flagCheckered' }, | ||
| { id: 'ch', name: 'Switzerland', flag: 'flag' }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move mock data to a separate file.
The mock data should be moved to a separate file for better maintainability and testing.
-const mockCountries: Country[] = [
- { id: 'us', name: 'United States', flag: 'flag' },
- // ... other countries
-];
+import { mockCountries } from '../../../mocks/countries';
Description
Related Issue
Resolve #16600 and #16604
Screenshots: