Mobile: Native tokens on L2s have Ethereum icon with network #16862
Conversation
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
71d4ce0 to
3178934
Compare
WalkthroughThe pull request introduces new configurations and components to support network icons alongside existing cryptocurrency icons. New constants and configuration arrays are added to generate network icon files, and corresponding TypeScript modules and exports are created. Several components across different modules—including account management, transaction details, send, and receive flows—are updated to replace older icon components with new ones (e.g., replacing ✨ 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 (
|
|
🚀 Expo preview is ready!
|
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
3178934 to
2f9fe10
Compare
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
b685167 to
bafe76c
Compare
vytick
force-pushed
the
icons-tokens-with-networks
branch
4 times, most recently
from
baded94 to
f8f9b32
Compare
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
f8f9b32 to
41310f5
Compare
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
17f439a to
f075925
Compare
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
17df206 to
f0c5180
Compare
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-accounts-management/src/components/TransactionListHeader.tsx
Outdated
Show resolved
Hide resolved
suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx
Show resolved
Hide resolved
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
f0c5180 to
b841001
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: 0
🧹 Nitpick comments (8)
suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx (1)
8-10: Consider handling edge cases and adding prop validation.While the implementation is clean and focused, consider:
- Adding prop validation for required values
- Handling undefined/null cases
- Adding error boundaries for invalid network symbols
export const NetworkDisplaySymbolNameFormatter = ({ value, -}: NetworkDisplaySymbolNameFormatterProps) => getNetworkDisplaySymbolName(value); +}: NetworkDisplaySymbolNameFormatterProps) => { + if (!value) return null; + try { + return getNetworkDisplaySymbolName(value); + } catch (error) { + console.warn(`Invalid network symbol: ${value}`); + return null; + } +};suite-native/icons/src/NetworkIcon.tsx (1)
16-21: Consider using a TypeScript enum for size constants.The
networkIconSizesobject could benefit from being defined as an enum for better type safety and to prevent magic numbers.-export const networkIconSizes = { - extraSmall: 9, - small: 12, - large: 18, - extraLarge: 24, -} as const; +export enum NetworkIconSize { + ExtraSmall = 9, + Small = 12, + Large = 18, + ExtraLarge = 24, +}suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx (1)
12-18: Add test for invalid network symbol.The test suite should include a case for handling invalid network symbols to ensure graceful failure.
+ it('should handle invalid network symbol gracefully', () => { + const { queryByHintText } = render( + <CryptoIconWithNetwork symbol="invalid_network" as TokenAddress />, + ); + + expect(queryByHintText(cryptoIconHint)).toBeNull(); + });suite-native/icons/src/CryptoIconWithNetwork.tsx (2)
19-26: Consider using theme tokens for border radius.The border radius calculation
networkIconSizes[size] / 3might lead to inconsistent values. Consider using predefined theme tokens for better design consistency.- borderRadius: networkIconSizes[size] / 3, + borderRadius: utils.borders.radii.small,
30-52: Add accessibility props for better screen reader support.The component should provide proper accessibility information for both the main icon and the network indicator.
<Box style={{ width: cryptoIconSizes[size], height: cryptoIconSizes[size] }}> - <CryptoIcon symbol={iconSymbol} contractAddress={contractAddress} size={size} /> + <CryptoIcon + symbol={iconSymbol} + contractAddress={contractAddress} + size={size} + accessibilityLabel={`${iconSymbol} with ${symbol} network`} + /> {shouldShowNetwork && ( <Box style={applyStyle(networkWrapperStyle, { size })}> - <NetworkIcon symbol={symbol} size={size} /> + <NetworkIcon + symbol={symbol} + size={size} + accessibilityLabel={`${symbol} network indicator`} + /> </Box> )} </Box>suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (1)
54-60: Consider simplifying the icon symbol logic.The conditional logic for determining
iconSymbolcould be more concise.- let iconSymbol: NetworkSymbol | NetworkDisplaySymbol | undefined; - - if (contractAddress) { - iconSymbol = symbol; - } else if (symbol) { - iconSymbol = getNetworkDisplaySymbol(symbol) as NetworkDisplaySymbol; - } + const iconSymbol = contractAddress + ? symbol + : symbol + ? (getNetworkDisplaySymbol(symbol) as NetworkDisplaySymbol) + : undefined;suite-native/accounts/src/components/AccountSelectBottomSheet.tsx (1)
42-42: Consider making isNativeCoinOnly prop type-safe.The addition of
isNativeCoinOnlyprop enhances the icon display logic for native coins. However, consider making it type-safe by adding it to the component's props interface.type AccountSelectBottomSheetProps = { data: AccountSelectBottomSheetSection[]; onSelectAccount: OnSelectAccount; isStakingPressable?: boolean; + isNativeCoinOnly?: boolean; onClose: () => void; };suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx (1)
88-96: Consider removing unnecessary useMemo.The useMemo hook might be overkill here as it's wrapping simple JSX that's unlikely to cause performance issues. The overhead of creating a new function on every render might outweigh any benefits.
- const icon = useMemo( - () => - isNativeCoinOnly ? ( - <CryptoIconWithNetwork symbol={account.symbol} /> - ) : ( - <RoundedIcon symbol={account.symbol} /> - ), - [account.symbol, isNativeCoinOnly], - ); + const icon = isNativeCoinOnly ? ( + <CryptoIconWithNetwork symbol={account.symbol} /> + ) : ( + <RoundedIcon symbol={account.symbol} /> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
suite-common/icons/cryptoAssets/cryptoIcons/bnb.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/ada.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/ada_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/bsc.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/bsc_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/btc.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/btc_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/eth.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/eth_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/op.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/op_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/pol.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/pol_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol_inverse.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
suite-common/icons/generateIcons.js(3 hunks)suite-common/icons/src/cryptoIcons.ts(1 hunks)suite-common/icons/src/index.ts(1 hunks)suite-common/icons/src/networkIcons.ts(1 hunks)suite-common/wallet-config/src/networksConfig.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)suite-native/accounts/src/components/AccountDetailsCard.tsx(3 hunks)suite-native/accounts/src/components/AccountSelectBottomSheet.tsx(1 hunks)suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx(5 hunks)suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx(2 hunks)suite-native/accounts/src/components/TokenReceiveCard.tsx(3 hunks)suite-native/accounts/src/index.ts(1 hunks)suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx(1 hunks)suite-native/formatters/src/components/TokenAmountFormatter.tsx(1 hunks)suite-native/formatters/src/index.ts(1 hunks)suite-native/icons/jest.config.js(1 hunks)suite-native/icons/package.json(1 hunks)suite-native/icons/src/CryptoIcon.tsx(3 hunks)suite-native/icons/src/CryptoIconWithNetwork.tsx(1 hunks)suite-native/icons/src/NetworkIcon.tsx(1 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/icons/src/redux.d.ts(1 hunks)suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx(1 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-accounts-management/package.json(0 hunks)suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx(3 hunks)suite-native/module-accounts-management/src/components/CoinPriceCard.tsx(2 hunks)suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx(2 hunks)suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx(1 hunks)suite-native/module-accounts-management/tsconfig.json(0 hunks)suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx(2 hunks)suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx(2 hunks)suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(2 hunks)suite-native/receive/package.json(0 hunks)suite-native/receive/src/components/ReceiveScreenHeader.tsx(0 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(2 hunks)suite-native/receive/tsconfig.json(0 hunks)suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx(2 hunks)suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx(3 hunks)
💤 Files with no reviewable changes (5)
- suite-native/receive/src/components/ReceiveScreenHeader.tsx
- suite-native/module-accounts-management/package.json
- suite-native/receive/package.json
- suite-native/module-accounts-management/tsconfig.json
- suite-native/receive/tsconfig.json
✅ Files skipped from review due to trivial changes (3)
- suite-native/icons/jest.config.js
- suite-native/icons/src/index.ts
- suite-common/icons/src/networkIcons.ts
⏰ Context from checks skipped due to timeout of 90000ms (20)
- 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: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-web
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (41)
suite-native/formatters/src/components/TokenAmountFormatter.tsx (2)
28-28: LGTM! Safe null handling with proper case formatting.The optional chaining operator ensures safe handling of null tokenSymbol values while the uppercase conversion aligns with common cryptocurrency display conventions.
28-28: Consider adding tests for TokenAmountFormatter.Tests would help verify the formatting behavior, especially for edge cases like null token symbols and various decimal values.
suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx (4)
1-22: LGTM! Well-organized imports and type definitions.The imports are properly organized and the type definitions are clear and well-structured.
24-50: LGTM! Proper use of hooks and selectors.The component correctly uses hooks for data fetching and analytics reporting. The dependencies array in useEffect is properly configured.
52-55: LGTM! Effective memoization implementation.The list header component is properly memoized with correct dependencies to optimize performance.
60-63: LGTM! Improved conditional rendering logic.The switch to using
tokenContractfor conditional rendering is a cleaner approach compared to checkingtoken?.name. The props passed toTokenAccountDetailScreenHeaderare consistent with the component's updated interface.suite-common/icons/src/index.ts (1)
9-9: LGTM!The new export for networkIcons follows the established pattern and respects the warning about not re-exporting icons.ts. This change aligns with the PR objectives of supporting network icons.
suite-native/icons/src/redux.d.ts (1)
1-11: LGTM! Well-structured type declarations.The Redux type declarations are properly defined with:
- AsyncThunkAction support for Redux Toolkit
- Generic ThunkAction support with flexible return types
- Proper type parameters for state and extra arguments
suite-native/accounts/src/index.ts (1)
12-13: LGTM!The new exports follow the established pattern and support the PR objectives by exposing the necessary components for network icon display.
suite-native/formatters/src/index.ts (1)
10-10: LGTM!The addition of
NetworkDisplaySymbolNameFormatterexport aligns with the PR objective and follows the established pattern of formatter exports.suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx (1)
4-4: LGTM!The removal of
tokenAddressand simplification of props improves the component's maintainability while maintaining type safety.Also applies to: 16-16
suite-native/accounts/src/components/AccountDetailsCard.tsx (1)
13-16: LGTM!The changes improve component reusability and enhance native token support:
- Component renamed to be more generic
- Error message moved to appropriate module
- Added
isNativeCoinOnlyprop for better token handlingAlso applies to: 27-27, 37-37
suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx (1)
3-4: LGTM!The changes improve code quality through:
- Simplified state management using consolidated selectors
- Enhanced network symbol display using
getNetworkDisplaySymbol- Proper internationalization with dynamic values
Also applies to: 9-9, 20-22, 24-26, 32-32, 37-39
suite-native/icons/src/NetworkIcon.tsx (2)
11-14: LGTM! Well-defined props interface.The
NetworkIconPropsinterface is clear and properly typed withNetworkSymboland optional size.
28-50: Verify network icon accessibility.The component handles accessibility well with
accessibilityHint, but consider addingaccessibilityRoleandaccessibilityLabelfor better screen reader support.<Image accessibilityHint={translate('icons.networkIconHint')} + accessibilityRole="image" + accessibilityLabel={translate('icons.networkIconLabel', { symbol })} source={sourceUrl} recyclingKey={symbol} style={applyStyle(iconStyle, { width: sizeNumber, height: sizeNumber })} cachePolicy="memory-disk" />suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx (1)
28-37: Consider testing contract address validation.The test for contract addresses should include validation of malformed addresses.
+ it('should handle malformed contract addresses', () => { + const invalidContract = 'invalid_address' as TokenAddress; + const { queryByHintText } = render( + <CryptoIconWithNetwork symbol="op" contractAddress={invalidContract} />, + ); + + expect(queryByHintText(cryptoIconHint)).toBeDefined(); + expect(queryByA11yHint('ETH' + invalidContract)).toBeDefined(); + });suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx (1)
39-41: LGTM! Proper implementation of CryptoIconWithNetwork.The component correctly uses the new
CryptoIconWithNetworkwith appropriate props from the account and token data.suite-common/icons/src/cryptoIcons.ts (1)
9-9: LGTM! BNB icon added correctly.The BNB icon is added in alphabetical order, maintaining the file's structure.
suite-native/icons/src/CryptoIconWithNetwork.tsx (1)
13-17: LGTM! Well-defined props interface.The props interface is clear and properly typed with NetworkSymbol and optional TokenAddress.
suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx (1)
33-34: LGTM! Proper icon replacement and alignment.The NetworkIcon is more appropriate here as it represents the network, and the alignment is properly handled.
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (1)
28-44: Consider handling potential null network symbol.The non-null assertion operator on the network symbol selector might be unsafe. Consider handling the null case explicitly.
- )!; + ); + + if (!symbol) { + return null; + }suite-native/icons/src/CryptoIcon.tsx (2)
70-77: LGTM! Improved accessibility support.Good addition of accessibility props with proper translation support.
22-27: Verify the impact of increased icon sizes.The icon sizes have been significantly increased. Please ensure this doesn't affect the layout in existing usages.
suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (1)
59-59: Consider updating getNetworkDisplaySymbol return type.Type casting to NetworkDisplaySymbol suggests a potential type mismatch. Based on the past review comments, this was intentionally kept as string to maintain compatibility with desktop. Consider creating a separate mobile-specific version that returns NetworkDisplaySymbol if desktop compatibility is no longer a concern.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if getNetworkDisplaySymbol is used in desktop rg "getNetworkDisplaySymbol" --type tsLength of output: 14158
Action: Confirmed No Change Is Needed for getNetworkDisplaySymbol Return Type
The function still returns a string in suite‑common/wallet‑config (see packages/suite-common/wallet-config/src/utils.ts) and its use on mobile (e.g. in suite‑native/transactions/src/components/TransactionsList/TransactionIcon.tsx) leverages an explicit cast to NetworkDisplaySymbol specifically to maintain desktop compatibility. This matches previous decisions, so no update is required at this time.
suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx (1)
48-69: LGTM! Good improvements to the header layout.The changes improve the header by:
- Using CryptoIconWithNetwork for better network visualization
- Organizing content in VStack for better hierarchy
- Using proper text truncation for both token name and account label
suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx (1)
44-48: LGTM! Good simplification of the icon implementation.The changes improve the code by:
- Removing unnecessary Box wrapper
- Using the new CryptoIconWithNetwork component
- Using standardized size prop
suite-native/accounts/src/components/TokenReceiveCard.tsx (1)
47-47: LGTM! Good improvements to error handling and icon display.The changes improve the component by:
- Using consistent translation keys (moduleAccounts instead of moduleReceive)
- Using CryptoIconWithNetwork for better network visualization
Also applies to: 58-59
suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx (1)
37-54: LGTM! Clean prop removal.The removal of the
tokenAddressprop fromCryptoBalancecomponent aligns with the standardization of token representation across the app.suite-native/module-accounts-management/src/components/CoinPriceCard.tsx (1)
74-74: LGTM! Enhanced icon representation.The replacement of
RoundedIconwithCryptoIconWithNetworksuccessfully implements the desired L2 token visualization, showing Ethereum icon with network.Let's verify consistent usage of CryptoIconWithNetwork across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining RoundedIcon usage that might need updating rg "RoundedIcon" # Search for CryptoIconWithNetwork usage patterns rg "CryptoIconWithNetwork"Length of output: 7040
Verified: L2 Token Visualization Changes are Consistent
The usage of CryptoIconWithNetwork in CoinPriceCard (and similar L2 token components) has been correctly implemented to support the enhanced Ethereum icon with network. The remaining instances of RoundedIcon are in contexts where no such change was intended.
- Verified replacement in suite-native/module-accounts-management/src/components/CoinPriceCard.tsx.
- Other CryptoIconWithNetwork usages are aligned with the token/ethereum-specific visualization.
- RoundedIcon remains in modules where the original behavior is maintained.
suite-native/receive/src/screens/ReceiveAddressScreen.tsx (1)
91-91: LGTM! Component replacement maintains functionality.The replacement of
ReceiveAccountDetailsCardwithAccountDetailsCardmaintains the same props while aligning with the new standardized component structure.Let's verify the component export and usage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify AccountDetailsCard export rg "export.*AccountDetailsCard" "suite-native/accounts/src/" # Check for any remaining ReceiveAccountDetailsCard usage rg "ReceiveAccountDetailsCard"Length of output: 331
Verification Successful: Component export and usage confirmed.
- Found "AccountDetailsCard" being exported in "suite-native/accounts/src/components/AccountDetailsCard.tsx" and re-exported in "suite-native/accounts/src/index.ts".
- No residual usage of "ReceiveAccountDetailsCard" was found.
suite-common/icons/generateIcons.js (1)
11-11: LGTM! Well-structured network icons support.The implementation follows the established patterns consistently, making it easy to maintain and extend.
Also applies to: 29-31, 135-137
suite-common/wallet-config/src/utils.ts (1)
72-75: Add test coverage for the new type guard function.The
isNetworkDisplaySymbolfunction follows good practices but lacks test coverage, which was previously requested for similar functions.suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx (1)
28-37: LGTM! Improved prop naming for better semantics.The change from
isInModaltoisNativeCoinOnlybetter describes the prop's purpose.suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx (1)
111-117: LGTM! Clean icon component update.The replacement of CryptoIcon with CryptoIconWithNetwork is implemented cleanly with appropriate size adjustments.
suite-native/module-send/src/screens/SendOutputsScreen.tsx (1)
25-25: LGTM! UI structure improvements.The changes enhance the UI by:
- Adding the AccountDetailsCard for better account visualization
- Using proper fragment syntax for multiple children
- Maintaining consistent spacing with marginVertical
Also applies to: 290-297
suite-common/wallet-config/src/networksConfig.ts (1)
541-541: LGTM! Type-safe network display symbols.The new NetworkDisplaySymbol type enhances type safety when working with network display symbols across the application.
suite-native/intl/src/en.ts (3)
31-34: Great addition of accessibility hints!The new icon hint translations improve accessibility by providing clear descriptions for network and crypto icons.
883-887: LGTM! Enhanced error handling and token context.The new translation keys provide:
- Clear error messages for missing accounts/tokens
- Better context for tokens with their parent account
1102-1102: LGTM! Improved send screen title.The dynamic asset name in the send screen title provides better context for users.
suite-native/icons/package.json (2)
11-12: LGTM! Added essential scripts.The addition of generate-icons and test:unit scripts improves the development workflow.
15-15: Verify Redux Toolkit version compatibility.The fixed version 1.9.5 of @reduxjs/toolkit might not be the latest. Please verify if this version is intentionally pinned.
|
On desktop, we keep network icon there |
suite-native/formatters/src/components/TokenAmountFormatter.tsx
Outdated
Show resolved
Hide resolved
On mobile we dont: https://www.figma.com/design/bETHMSdkxMyt2sOyQORjXe/Trends-hotfixes?node-id=38-2688&t=DBTVmpBVxAhU2sfk-0 |
vytick
dismissed
tomasklim’s stale review
We agreed to resolve this in another PR
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx (2)
44-46: Consider adding a null check for token data.While there's a good null check for the symbol, consider adding a similar check for the token data to prevent potential runtime errors.
if (!symbol) { return null; } +if (!token) { + return null; +}
51-73: Consider adding accessibility attributes to text components.While the UI structure is well-organized with proper layout components, consider enhancing accessibility for screen readers.
-<Text ellipsizeMode="tail" numberOfLines={1}> +<Text + ellipsizeMode="tail" + numberOfLines={1} + accessibilityRole="header" + accessibilityLabel={`Token name: ${token?.name}`} +> {token?.name} </Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx(3 hunks)suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx(1 hunks)
⏰ 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 / 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: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (7)
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (4)
1-26: LGTM!The imports are well-organized and the type definitions are clear. The addition of
CryptoIconWithNetworkaligns with the PR objectives for implementing network icons.
28-48: Add missing Text component props.Based on past review comments, it's recommended to explicitly set variant and color props on Text components when not using the default preset.
Apply this diff to add the missing props:
- <Text variant="highlight" adjustsFontSizeToFit numberOfLines={1}> + <Text + variant="highlight" + color="textDefault" + adjustsFontSizeToFit + numberOfLines={1} + >
50-84: LGTM!The component is well-structured and correctly implements the new
AccountDetailScreenHeaderContentcomponent. The navigation and route handling are properly implemented.
1-85: Implementation aligns with PR objectives.The changes correctly implement the mobile-specific design for native tokens on L2s, using the
CryptoIconWithNetworkcomponent as intended. This aligns with the PR objectives and the mobile-specific design requirements discussed in the PR comments.suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx (3)
10-12: LGTM! Import changes and type definitions are well-structured.The changes properly introduce the necessary types and components for network icon support, aligning with the PR objectives.
Also applies to: 20-20, 22-25
27-40: LGTM! Props and selector usage are well-implemented.The component properly uses the token contract address and Redux selectors for data fetching.
53-57: Implementation aligns with PR objectives.The use of
CryptoIconWithNetworkwith bothsymbolandcontractAddressprops correctly implements the network icon requirements for mobile, as discussed in the PR comments.✅ Verification successful
🌐 Web query:
What is the current best practice for displaying network icons in React Native applications?💡 Result:
The current best practices for displaying network icons in React Native applications combine reliable network detection with optimized icon implementation. Here's a structured approach based on industry standards:
Core Implementation Strategy
- Network Detection
Use@react-native-community/netinfofor comprehensive network monitoring:import NetInfo from '@react-native-community/netinfo'; // Usage in component const [isConnected, setIsConnected] = useState(true); useEffect(() => { const unsubscribe = NetInfo.addEventListener(state => { setIsConnected(state.isConnected); }); return () => unsubscribe(); }, []);This provides real-time updates on connection status (
isConnected) and type (state.type) [5][8]
- Icon Libraries
Preferred solutions include:
- React Native Vector Icons (6k+ icons across 30+ sets)
import Icon from 'react-native-vector-icons/MaterialIcons'; <Icon name={isConnected ? 'wifi' : 'signal-wifi-off'} size={24} color={isConnected ? 'green' : 'red'} />
- React Native Paper (Material Design integration)
- Custom SVG Implementation ([4] shows automated SVG conversion)
Best Practice Table
Aspect Recommendation Source Visual Consistency Use same icon family throughout app [1][3] Conditional Rendering Pair icons with network state checks [5][8] Performance Use vector icons over bitmap formats [7][10] Accessibility Add ARIA labels for screen readers [4][9] Platform Testing Verify rendering on iOS/Android [1][6] Advanced Pattern
Create a reusable
NetworkStatusIndicatorcomponent:import {View, Text} from 'react-native'; import Icon from 'react-native-vector-icons/MaterialIcons'; export default function NetworkStatus({isConnected}) { return ( <View style={{flexDirection: 'row', alignItems: 'center'}}> <Icon name={isConnected ? 'cloud-done' : 'cloud-off'} size={20} color={isConnected ? '#4CAF50' : '#F44336'} /> <Text style={{marginLeft: 8}}> {isConnected ? 'Online' : 'Offline'} </Text> </View> ); }Key benefits:
- Centralized styling logic
- Consistent behavior across app
- Easy maintenance [2][5][7]
Optimization Tips
- Cache frequently used icons with
react-native-fast-image- Use icon subsets to reduce bundle size
- Implement dynamic loading for rare icons
- Follow platform-specific guidelines:
- iOS: SF Symbols integration
- Android: Material You theming [3][6][9]
For production apps, combine network status detection with animated transitions between icon states to enhance user perception of connectivity changes. Always test icon visibility under various lighting conditions and screen types [1][9][10].
Citations:
- 1: https://30dayscoding.com/blog/building-react-native-apps-with-react-native-icon
- 2: https://dev.to/thelamina/handling-network-status-in-your-react-app-4222
- 3: https://dev.to/aneeqakhan/top-icon-libraries-to-enhance-your-react-native-apps-ui-4na9
- 4: https://dev.to/seanyasno/handling-icons-in-react-best-practices-22c5
- 5: https://blog.openreplay.com/real-time-network-status-detection-with-react-native/
- 6: https://jscrambler.com/blog/how-to-add-a-splash-screen-and-app-icon-in-react-native
- 7: https://dev.to/ajmal_hasan/integrating-react-native-vector-icons-in-your-project-28k9
- 8: https://www.npmjs.com/package/@react-native-community/netinfo
- 9: https://theonetechnologies.com/blog/post/optimizing-performance-in-react-native-apps-7-best-practices-you-need-to-know
- 10: https://reactnative.dev/docs/images
Implementation aligns with PR objectives.
The use ofCryptoIconWithNetworkwith the appropriatesymbolandcontractAddressprops correctly integrates network icon display on mobile, in line with best practices for React Native icon implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/icons/src/CryptoIconWithNetwork.tsx (2)
19-26: Consider documenting the border radius calculation.The border radius calculation (
networkIconSizes[size] / 3) would benefit from a brief comment explaining why this specific ratio was chosen for visual consistency.const networkWrapperStyle = prepareNativeStyle<{ size: CryptoIconSize }>((utils, { size }) => ({ position: 'absolute', right: 0, bottom: 0, borderWidth: utils.borders.widths.small, borderColor: utils.colors.borderElevation1, + // Divide by 3 to maintain visual balance with the icon size borderRadius: networkIconSizes[size] / 3, }));
37-38: Enhance documentation for network icon display logic.The condition for showing the network icon could benefit from more detailed documentation explaining the specific use cases.
+ // Show network icon in two cases: + // 1. For non-Ethereum networks that use ETH as their native token (e.g., Base, Arbitrum, Optimism) + // 2. For token contracts on any network const showForNativeToken = displaySymbol === 'ETH' && symbol !== 'eth'; const shouldShowNetwork = showForNativeToken || contractAddress;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/icons/src/CryptoIconWithNetwork.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- 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: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
suite-native/icons/src/CryptoIconWithNetwork.tsx (3)
1-17: LGTM! Well-organized imports and clean type definitions.The imports are properly organized and the props interface is well-typed with appropriate types from the wallet configuration and types packages.
28-30: Consider renaming the component as suggested.As per the previous review suggestion, consider renaming to
TokenWithNetworkIconas it better describes the component's purpose.-export const CryptoIconWithNetwork = ({ +export const TokenWithNetworkIcon = ({
42-51: LGTM! Clean and well-structured implementation.The component is well-implemented with proper conditional rendering and styling. The box layout ensures correct positioning of both the crypto icon and the network indicator.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
9cd5d10 to
6bc67e3
Compare
|
LGTM. @tomasklim please approve, if you are good with it (I probably still do not have rights for that.) |
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
6bc67e3 to
39c7218
Compare
|
/rebase |
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
39c7218 to
6f6ceab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx (1)
32-32: Consider adding fallback for asset name.While the current implementation works, consider adding a fallback value for better robustness:
- const assetName = (tokenInfo?.symbol ?? getNetworkDisplaySymbol(account.symbol)).toUpperCase(); + const assetName = (tokenInfo?.symbol ?? getNetworkDisplaySymbol(account.symbol) ?? 'Unknown').toUpperCase();suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (1)
43-45: Consider adding accessibility label for screen readers.While the text adjusts nicely for visual display, consider adding an
accessibilityLabelprop to improve screen reader support.- <Text variant="highlight" adjustsFontSizeToFit numberOfLines={1}> + <Text + variant="highlight" + adjustsFontSizeToFit + numberOfLines={1} + accessibilityLabel={`Account ${accountLabel}`} + > {accountLabel} </Text>suite-native/icons/src/CryptoIconWithNetwork.tsx (1)
29-29: Enhance component documentation.The current comment could be more descriptive about when and why network icons are shown.
-// This component shows network icon for tokens -// and for non Ethereum networks with native coin being eth (base, arbitrum, optimism) +/** + * Displays a crypto icon with an optional network icon overlay. + * Network icon is shown in two cases: + * 1. For tokens on any network (when contractAddress is provided) + * 2. For native ETH on L2 networks (base, arbitrum, optimism) + */suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (1)
54-60: Consider using type guards instead of type assertion.The type assertion
as NetworkDisplaySymbolmight be unsafe. Consider using type guards to ensure type safety.-iconSymbol = getNetworkDisplaySymbol(symbol) as NetworkDisplaySymbol; +const displaySymbol = getNetworkDisplaySymbol(symbol); +if (isNetworkDisplaySymbol(displaySymbol)) { + iconSymbol = displaySymbol; +}suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx (1)
88-96: Consider extracting icon logic to a separate component.The icon rendering logic could be extracted to a separate component to improve reusability and maintainability.
+const AccountIcon = ({ symbol, isNativeCoinOnly }: { symbol: NetworkSymbol, isNativeCoinOnly: boolean }) => { + return isNativeCoinOnly ? ( + <CryptoIconWithNetwork symbol={symbol} /> + ) : ( + <RoundedIcon symbol={symbol} /> + ); +}; -const icon = useMemo( - () => - isNativeCoinOnly ? ( - <CryptoIconWithNetwork symbol={account.symbol} /> - ) : ( - <RoundedIcon symbol={account.symbol} /> - ), - [account.symbol, isNativeCoinOnly], -); +const icon = useMemo( + () => <AccountIcon symbol={account.symbol} isNativeCoinOnly={isNativeCoinOnly} />, + [account.symbol, isNativeCoinOnly], +);suite-native/icons/src/CryptoIcon.tsx (1)
50-63: Add error handling for invalid coingeckoId and contract address.While the network symbol handling logic is well-structured, consider adding error handling for cases where:
- coingeckoId is not found
- contract address formatting fails
if (isNetworkSymbol(symbol)) { const coingeckoId = getCoingeckoId(symbol); - if (coingeckoId && contractAddress) { + if (!coingeckoId) { + console.warn(`No coingeckoId found for network symbol: ${symbol}`); + return url; + } + if (contractAddress) { const formattedAddress = getContractAddressForNetworkSymbol( symbol, contractAddress, ); + if (!formattedAddress) { + console.warn(`Failed to format contract address: ${contractAddress} for network: ${symbol}`); + return url; + } url = getAssetLogoUrl({ coingeckoId, contractAddress: formattedAddress, quality: '@2x', }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
suite-common/icons/cryptoAssets/cryptoIcons/bnb.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/ada.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/ada_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/arb_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/base_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/bsc.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/bsc_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/btc.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/btc_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/eth.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/eth_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/op.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/op_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/pol.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/pol_inverse.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol.svgis excluded by!**/*.svgsuite-common/icons/cryptoAssets/networkIcons/sol_inverse.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
suite-common/icons/generateIcons.js(3 hunks)suite-common/icons/src/cryptoIcons.ts(1 hunks)suite-common/icons/src/index.ts(1 hunks)suite-common/icons/src/networkIcons.ts(1 hunks)suite-common/wallet-config/src/networksConfig.ts(1 hunks)suite-native/accounts/src/components/AccountDetailsCard.tsx(3 hunks)suite-native/accounts/src/components/AccountSelectBottomSheet.tsx(1 hunks)suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx(5 hunks)suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx(2 hunks)suite-native/accounts/src/components/TokenReceiveCard.tsx(3 hunks)suite-native/accounts/src/index.ts(1 hunks)suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx(1 hunks)suite-native/formatters/src/index.ts(1 hunks)suite-native/icons/jest.config.js(1 hunks)suite-native/icons/package.json(1 hunks)suite-native/icons/src/CryptoIcon.tsx(3 hunks)suite-native/icons/src/CryptoIconWithNetwork.tsx(1 hunks)suite-native/icons/src/NetworkIcon.tsx(1 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/icons/src/redux.d.ts(1 hunks)suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx(1 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-accounts-management/package.json(0 hunks)suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx(3 hunks)suite-native/module-accounts-management/src/components/CoinPriceCard.tsx(2 hunks)suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx(1 hunks)suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx(1 hunks)suite-native/module-accounts-management/tsconfig.json(0 hunks)suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx(2 hunks)suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx(2 hunks)suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(2 hunks)suite-native/receive/package.json(0 hunks)suite-native/receive/src/components/ReceiveScreenHeader.tsx(0 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(2 hunks)suite-native/receive/tsconfig.json(0 hunks)suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx(2 hunks)suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx(3 hunks)
💤 Files with no reviewable changes (5)
- suite-native/receive/package.json
- suite-native/receive/tsconfig.json
- suite-native/module-accounts-management/package.json
- suite-native/module-accounts-management/tsconfig.json
- suite-native/receive/src/components/ReceiveScreenHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (24)
- suite-native/icons/jest.config.js
- suite-common/icons/src/index.ts
- suite-native/formatters/src/index.ts
- suite-native/accounts/src/components/TokenReceiveCard.tsx
- suite-native/icons/src/index.ts
- suite-common/wallet-config/src/networksConfig.ts
- suite-native/module-send/src/screens/SendOutputsScreen.tsx
- suite-common/icons/src/networkIcons.ts
- suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx
- suite-common/icons/src/cryptoIcons.ts
- suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx
- suite-native/icons/package.json
- suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx
- suite-native/module-accounts-management/src/components/CoinPriceCard.tsx
- suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx
- suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx
- suite-native/receive/src/screens/ReceiveAddressScreen.tsx
- suite-native/icons/src/redux.d.ts
- suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx
- suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx
- suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx
- suite-native/accounts/src/components/AccountSelectBottomSheet.tsx
- suite-native/accounts/src/index.ts
- suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: build
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
- 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 (19)
suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx (3)
3-9: LGTM! Well-organized imports.The imports are properly organized by domain and align with the component's requirements.
20-30: LGTM! Improved data retrieval logic.The refactoring simplifies the data retrieval process by:
- Consolidating multiple selectors into
selectAccountByKeyandselectAccountTokenInfo- Adding proper null checks for type safety
34-43: LGTM! Clean rendering implementation.The rendering logic is well-structured:
- Uses Translation component for i18n support
- Follows mobile-specific design requirements as discussed in PR comments
- Maintains consistent header layout with navigation
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (3)
1-1: LGTM!The new imports are well-organized and properly support the network icon functionality.
Also applies to: 5-7
28-48: LGTM! Component extraction improves modularity.The new component is well-structured, follows React best practices, and addresses the previous feedback about moving to a standalone component.
64-71: LGTM!The integration of
AccountDetailScreenHeaderContentis clean and maintains the component's readability.suite-native/accounts/src/components/AccountDetailsCard.tsx (1)
10-10: LGTM! Clean refactoring of the AccountDetailsCard component.The changes improve component organization and add proper support for native coin display.
Also applies to: 13-16, 18-18, 27-27, 37-37
suite-native/icons/src/NetworkIcon.tsx (1)
1-50: Well-implemented NetworkIcon component with good practices!The component demonstrates:
- Proper type safety with NetworkIconProps interface
- Flexible size handling
- Accessibility support
- Efficient caching strategy
- Theme-aware rendering
suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx (1)
1-38: Comprehensive test coverage with clear test cases!The test suite effectively covers:
- Non-L2 networks behavior
- L2 networks with ETH icon
- Contract address scenarios
suite-native/icons/src/CryptoIconWithNetwork.tsx (1)
1-52: Well-structured component with clean implementation!The component demonstrates:
- Clear prop interface
- Proper style encapsulation
- Correct handling of different network scenarios
- Flexible size support
suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (1)
77-85: LGTM! The icon rendering logic is clear and concise.The conditional rendering of the icon with proper props looks good.
suite-common/icons/generateIcons.js (2)
29-31: LGTM! Network icons configuration follows the established pattern.The network icons configuration is consistent with the existing crypto icons configuration.
135-137: LGTM! Network icons generation is properly integrated.The network icons generation is correctly added to the generation process with appropriate logging.
suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx (1)
99-101: LGTM! Clear conditional logic for UI elements.The conditions for showing account label, token badge, and staking badge are clear and well-organized.
suite-native/intl/src/en.ts (2)
31-34: LGTM! Clear and descriptive icon hint messages.The icon hint messages are clear and follow i18n best practices.
883-887: LGTM! Account-related messages are properly structured.The account and token-related messages are clear and follow the established pattern.
suite-native/icons/src/CryptoIcon.tsx (3)
6-13: LGTM! Enhanced type safety with network symbol support.The addition of network-related types and translation utilities improves type safety and internationalization support.
Also applies to: 17-17
71-73: LGTM! Good accessibility improvements.The addition of translated accessibility hint and descriptive label enhances the component's accessibility. The key is effectively reused across multiple props.
23-26: Verify icon size changes with design system.The icon sizes have been significantly increased:
- extraSmall: 16 → 24
- small: 24 → 32
- large: 42 → 48
- Added extraLarge: 68
Please confirm these changes align with the design system specifications.
✅ Verification successful
🌐 Web query:
What are the recommended icon sizes for mobile applications in Material Design and Apple's Human Interface Guidelines?💡 Result:
Material Design and Apple's Human Interface Guidelines provide distinct recommendations for icon sizing across platforms. Here's a comprehensive breakdown:
Material Design Icon Guidelines
Recommended sizes
- Default: 24px (optimized for readability)
- Standard options: 18px, 24px, 36px, 48px[1][4][9]
- System icons: 24dp (density-independent pixels)
- Product icons: 48dp[9]
Implementation
/* Standard sizing classes */ .material-icons.md-18 { font-size: 18px; } .material-icons.md-24 { font-size: 24px; } .material-icons.md-36 { font-size: 36px; } .material-icons.md-48 { font-size: 48px; }[1][4]
Platform-specific assets
Asset Type Android Resolution iOS Resolution PNG 1x (48x48), 2x (96x96),
3x (144x144), etc.[4]Not typically used Vector Black 24dp SVG[4] N/A Color rules
/* Dark icons on light background */ .material-icons.md-dark { color: rgba(0,0,0,0.54); } /* Light icons on dark background */ .material-icons.md-light { color: rgba(255,255,255,1); }[1][4]
Apple's Human Interface Guidelines
iOS Icon Requirements
Device/Context Icon Size (Pixels) App Store 1024×1024[2][5][8] iPhone Home Screen 180×180 (@3x)
120×120 (@2x)[2][5]iPad Pro Home Screen 167×167[2][5] Spotlight Search 120×120[5] Settings 87×87[5] New Xcode 16 Sizes 128×128 (64pt@2x)
136×136 (68pt@2x)[10]Platform Comparison Table
Aspect Material Design Apple HIG Primary format SVG/Web Font PNG Color customization CSS-controlled Pre-rendered assets Size granularity Continuous scaling Fixed breakpoints Dark mode support CSS classes[1][4] Automatic adaptation[8] App store requirement N/A 1024×1024 master[8] Key iOS requirements
- All icons must be square with no transparency[8]
- Submit single 1024×1024 PNG that Apple resizes[7]
- Avoid text unless essential to branding[8]
- Test visibility at smallest size (40×40 notifications)[5]
For Android implementations using Material Design, focus on vector assets and density-aware PNGs. iOS developers should prioritize creating crisp high-resolution assets that maintain clarity when scaled down to small sizes like Settings icons (58×58)[2]. Both platforms emphasize simplicity and recognizability across device contexts.
Citations:
- 1: https://mdi.invite-comm.jp
- 2: https://codershigh.github.io/guidelines/ios/human-interface-guidelines/graphics/app-icon/index.html
- 3: https://www.mobileaction.co/guide/app-icon-guide/
- 4: https://developers.google.com/fonts/docs/material_icons
- 5: https://appradar.com/academy/ios-app-icon
- 6: https://www.learnui.design/blog/ios-design-guidelines-templates.html
- 7: https://www.appsflyer.com/blog/tips-strategy/app-icon-design/
- 8: https://median.co/blog/what-are-apples-ui-guidelines-for-app-icons
- 9: https://m1.material.io/style/icons.html
- 10: https://forums.developer.apple.com/forums/thread/731274?answerId=800798022
Icon Size Update Verification
Based on the design guidelines from Material Design and Apple’s Human Interface Guidelines:
- extraSmall (24px): Matches Material Design’s default 24px icon.
- small (32px): Although Material Design typically uses 24px or 36px, a 32px size serves as a reasonable intermediate value that can improve visual consistency.
- large (48px): Aligns perfectly with the standard 48px recommended by Material Design.
- extraLarge (68px): Corresponds with newer iOS guidelines (e.g., 68pt in Xcode 16) for high-resolution assets.
These adjustments appear intentional and consistent with modern design practices. Please confirm that our internal design system supports this scale.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (1)
14-22: 🛠️ Refactor suggestionUpdate TransactionIconProps to reflect actual usage.
The type definition should be updated to match the component's implementation. The
symbolprop is used to deriveiconSymbol, which can be eitherNetworkSymbolorNetworkDisplaySymbol.Apply this diff to improve type safety:
type TransactionIconProps = { transactionType: TransactionType; - symbol?: NetworkSymbol; + symbol?: NetworkSymbol | NetworkDisplaySymbol; contractAddress?: TokenAddress; isAnimated?: boolean; backgroundColor?: Color; containerSize?: number; iconSize?: IconSize; };
🧹 Nitpick comments (8)
suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx (1)
32-40: Consider improving readability of asset name computation.While the logic is correct, the asset name computation could be more readable and easier to debug.
Consider breaking down the computation into separate variables:
- const assetName = (tokenInfo?.symbol ?? getNetworkDisplaySymbol(account.symbol)).toUpperCase(); + const networkSymbol = getNetworkDisplaySymbol(account.symbol); + const assetName = (tokenInfo?.symbol ?? networkSymbol).toUpperCase();suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (1)
36-38: Consider adding a fallback UI for the null case.Instead of returning null when the symbol is not found, consider displaying a fallback UI to improve user experience.
- if (!symbol) { - return null; - } + if (!symbol) { + return ( + <HStack alignItems="center"> + <Text variant="highlight" color="textSubdued"> + {accountLabel ?? 'Unknown Account'} + </Text> + </HStack> + ); + }suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx (2)
38-40: Consider memoizing the token selector result.While the null check for symbol is good for safety, consider memoizing the token selector result to prevent unnecessary recalculations:
- const token = useSelector((state: TokensRootState) => - selectAccountTokenInfo(state, accountKey, tokenContract), - ); + const token = useSelector( + (state: TokensRootState) => selectAccountTokenInfo(state, accountKey, tokenContract), + (prev, next) => prev?.name === next?.name + );Also applies to: 44-46
51-73: Consider adding a fallback for token name.The UI layout improvements look good, but consider adding a fallback for when
token?.nameis undefined:- <Text ellipsizeMode="tail" numberOfLines={1}> - {token?.name} - </Text> + <Text ellipsizeMode="tail" numberOfLines={1}> + {token?.name ?? token?.symbol ?? translate('tokens.unknownToken')} + </Text>suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx (4)
8-9: Consider extracting L2 networks into a constant.The L2 networks (op, arb, base) are mentioned in test descriptions but not centralized. Consider extracting them into a constant for better maintainability:
+const L2_NETWORKS = ['op', 'arb', 'base'] as const; const cryptoIconHint = 'Crypto Icon'; const networkIconHint = 'Network Icon';
12-18: Enhance test coverage for non-L2 networks.The test only verifies BTC. Consider adding test cases for:
- Other non-L2 networks
- Empty or invalid symbols
- Edge cases like undefined/null
20-26: Enhance test coverage and clarity for L2 networks.The test could be improved by:
- Testing all L2 networks (op, arb, base)
- Clarifying in the test description why ETH is the default icon
- Adding size prop variations to verify icon scaling
29-29: Consider using a more realistic contract address format.The current contract address doesn't follow typical Ethereum contract address format (0x-prefixed, 40 hex chars). Consider using a realistic example:
-const contract = '2b1kV6DkPAnxd5ixfnxCpjxmKwqjjaYmCZfHsFu24GXo' as TokenAddress; +const contract = '0x1234567890123456789012345678901234567890' as TokenAddress;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
suite-common/wallet-config/src/networksConfig.ts(1 hunks)suite-native/accounts/src/components/AccountDetailsCard.tsx(3 hunks)suite-native/accounts/src/components/AccountSelectBottomSheet.tsx(1 hunks)suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx(5 hunks)suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx(2 hunks)suite-native/accounts/src/components/TokenReceiveCard.tsx(3 hunks)suite-native/accounts/src/index.ts(1 hunks)suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx(1 hunks)suite-native/formatters/src/index.ts(1 hunks)suite-native/icons/jest.config.js(1 hunks)suite-native/icons/package.json(1 hunks)suite-native/icons/src/CryptoIcon.tsx(3 hunks)suite-native/icons/src/CryptoIconWithNetwork.tsx(1 hunks)suite-native/icons/src/NetworkIcon.tsx(1 hunks)suite-native/icons/src/index.ts(1 hunks)suite-native/icons/src/redux.d.ts(1 hunks)suite-native/icons/src/tests/CryptoIconWithNetwork.comp.test.tsx(1 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-accounts-management/package.json(0 hunks)suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx(1 hunks)suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx(3 hunks)suite-native/module-accounts-management/src/components/CoinPriceCard.tsx(2 hunks)suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx(1 hunks)suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx(1 hunks)suite-native/module-accounts-management/tsconfig.json(0 hunks)suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx(2 hunks)suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx(2 hunks)suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(2 hunks)suite-native/receive/package.json(0 hunks)suite-native/receive/src/components/ReceiveScreenHeader.tsx(0 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(2 hunks)suite-native/receive/tsconfig.json(0 hunks)suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx(2 hunks)suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx(3 hunks)
💤 Files with no reviewable changes (5)
- suite-native/module-accounts-management/tsconfig.json
- suite-native/receive/package.json
- suite-native/receive/src/components/ReceiveScreenHeader.tsx
- suite-native/receive/tsconfig.json
- suite-native/module-accounts-management/package.json
🚧 Files skipped from review as they are similar to previous changes (25)
- suite-native/icons/jest.config.js
- suite-native/formatters/src/index.ts
- suite-native/icons/package.json
- suite-native/icons/src/index.ts
- suite-native/accounts/src/index.ts
- suite-native/accounts/src/components/TokenReceiveCard.tsx
- suite-native/module-accounts-management/src/screens/AccountDetailContentScreen.tsx
- suite-native/module-accounts-management/src/components/AccountDetailCryptoValue.tsx
- suite-native/icons/src/redux.d.ts
- suite-native/module-send/src/screens/SendOutputsScreen.tsx
- suite-native/module-send/src/components/CorrectNetworkMessageCard.tsx
- suite-native/accounts/src/components/AccountDetailsCard.tsx
- suite-native/module-send/src/components/TokenOfNetworkAlertContent.tsx
- suite-native/icons/src/NetworkIcon.tsx
- suite-native/module-accounts-management/src/components/CoinPriceCard.tsx
- suite-native/receive/src/screens/ReceiveAddressScreen.tsx
- suite-native/accounts/src/components/AccountsList/AccountsListTokenItem.tsx
- suite-native/icons/src/CryptoIconWithNetwork.tsx
- suite-native/transactions/src/components/TransactionDetail/TransactionDetailAddressesSection.tsx
- suite-native/icons/src/CryptoIcon.tsx
- suite-native/accounts/src/components/AccountSelectBottomSheet.tsx
- suite-native/formatters/src/components/NetworkDisplaySymbolNameFormatter.tsx
- suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx
- suite-native/module-accounts-management/src/components/AccountDetailHeader.tsx
- suite-common/wallet-config/src/networksConfig.ts
⏰ 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: run-desktop-tests (@group=other, trezor-user-env-unix)
- 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: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (13)
suite-native/module-send/src/components/AccountBalanceScreenHeader.tsx (2)
3-9: LGTM! Import changes are well-organized.The imports have been logically reorganized and consolidated to support the new selector-based approach.
20-30: LGTM! Selector consolidation improves code maintainability.The consolidation of multiple selectors into
selectAccountByKeyandselectAccountTokenInforeduces complexity while maintaining type safety. The null check for account existence is appropriate.suite-native/intl/src/en.ts (3)
31-34: LGTM! Clear accessibility hints for icons.The new accessibility hints for network and crypto icons are well-defined and follow best practices.
883-887: LGTM! Logical restructuring of account-related messages.Moving these messages from
moduleReceivetomoduleAccountsimproves organization by grouping related messages together.
1102-1102: LGTM! Clear and dynamic send screen title.The title format with dynamic asset name provides good context for users.
suite-native/module-accounts-management/src/components/AccountDetailScreenHeader.tsx (3)
1-26: LGTM!The imports are well-organized and the type definitions are properly structured.
28-48: LGTM! Component extraction improves code organization.The component extraction aligns with the previous review suggestion and improves code modularity.
50-84: LGTM! Clean integration of the new component.The integration of
AccountDetailScreenHeaderContentis clean and maintains the existing functionality.suite-native/module-accounts-management/src/components/TokenAccountDetailScreenHeader.tsx (2)
10-13: LGTM! Import changes align with network icon support.The new imports appropriately support the network icon functionality and type safety improvements.
Also applies to: 20-20
22-25: LGTM! Props type safety improvement.The change from
tokenName: stringtotokenContract: TokenAddressenhances type safety and better represents the data model.Also applies to: 27-30
suite-native/transactions/src/components/TransactionsList/TransactionIcon.tsx (3)
1-5: LGTM: Import changes are well-organized.The new imports from '@suite-common/wallet-config' are properly structured and necessary for the network display symbol functionality.
54-60: Consider improving type safety of getNetworkDisplaySymbol usage.The type casting of
getNetworkDisplaySymbol(symbol) as NetworkDisplaySymbolsuggests a potential type safety issue.Let's verify the implementation of
getNetworkDisplaySymbol:#!/bin/bash # Search for getNetworkDisplaySymbol implementation ast-grep --pattern 'function getNetworkDisplaySymbol($_) { $$$ }'
77-81: LGTM: Rendering logic is clean and aligned with PR objectives.The changes to use
iconSymbolin the rendering condition andCryptoIconcomponent are well-implemented and achieve the goal of correctly handling network icons.
| ); | ||
|
|
||
| expect(queryByHintText(cryptoIconHint)).toBeDefined(); | ||
| expect(queryByA11yHint('ETH' + contract)).toBeDefined(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct string concatenation for accessibility hints.
Direct string concatenation could lead to poor screen reader experience. Consider using a proper separator:
-expect(queryByA11yHint('ETH' + contract)).toBeDefined();
+expect(queryByA11yHint(`ETH contract ${contract}`)).toBeDefined();📝 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.
| expect(queryByA11yHint('ETH' + contract)).toBeDefined(); | |
| expect(queryByA11yHint(`ETH contract ${contract}`)).toBeDefined(); |
Related Issue
Resolve #16635
Screenshots: