16638 mobile trade account and address picker stub #16935
Conversation
|
🚀 Expo preview is ready!
|
jbazant
force-pushed
the
16638-Mobile-Trade-Account-and-address-picker-stub
branch
2 times, most recently
from
e17c3c4 to
1bccaf0
Compare
suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx
Outdated
Show resolved
Hide resolved
jbazant
force-pushed
the
16638-Mobile-Trade-Account-and-address-picker-stub
branch
from
1bccaf0 to
51a0287
Compare
WalkthroughThe pull request introduces multiple enhancements across the trading module. It adds a new Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (19)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (2)
26-51: Verify the mock data replacement plan.Based on previous learnings, these mock arrays and helper functions are intentionally used as visual stubs. Please ensure there's a tracking issue or task for replacing them with real data fetching.
Would you like me to help create a new issue to track the implementation of real data fetching?
57-78: Implement the favorite toggle functionality.The
toggleFavouritefunction is currently a stub with a TODO comment. Consider implementing this functionality to enable users to manage their favorite assets.Would you like me to help create a new issue to track the implementation of the favorite toggle functionality?
suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx (1)
88-92: Consider refactoring to avoid O(n^2) complexity with array spreads.Using the spread operator on the accumulator can degrade performance for larger datasets. Instead, push the new items into the accumulator array.
if (noSingletonSectionHeader && inputData.length === 1) { - return [...acc, ...itemsData]; + acc.push(...itemsData); + return acc; } - return [...acc, ['sectionHeader', label, key], ...itemsData]; + acc.push(['sectionHeader', label, key], ...itemsData); + return acc;🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 92-92: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
suite-native/module-trading/src/hooks/useSelectedAccount.ts (1)
13-14: Add JSDoc documentation for better maintainability.Consider adding JSDoc documentation to describe the hook's purpose, parameters, and return value.
+/** + * Hook for managing account selection state in the account picker. + * @param {AccountSheetState} props - The account sheet state props + * @returns {Object} The selected account state and handlers + */ export const useSelectedAccount = ({ onAccountSelect, onClose, isVisible }: AccountSheetState) => {suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (1)
28-36: Add accessibility label for back navigation.The back navigation button in SearchableSheetHeader should have an accessibility label.
<SearchableSheetHeader onClose={clearSelectedAccount} title={selectedAccount.accountLabel} leftButtonIcon="caretLeft" + leftButtonA11yLabel={translate('generic.buttons.back')} />suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx (2)
32-33: Address TODO comment regarding loading state.Consider implementing a loading state to improve user experience while data is being fetched.
Would you like me to help implement a loading state for the account list?
18-18: Add type safety to keyExtractor.The keyExtractor should handle cases where address is undefined.
-const keyExtractor = (item: ReceiveAccount) => `${item.account}_${item.address?.address}`; +const keyExtractor = (item: ReceiveAccount) => + `${item.account.path}_${item.address?.address ?? 'no-address'}`;suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts (1)
56-56: Consider memoizing address mapping functions.The address mapping operations could be memoized to optimize performance for large lists.
+ const mapAddressToReceiveAccount = useCallback( + (address: Address) => ({ account: selectedAccount, address }), + [selectedAccount], + ); + return [ { key: 'unused', label: translate('moduleTrading.accountSheet.newAddress'), - data: unused.slice(0, 1).map(address => ({ account: selectedAccount, address })), + data: unused.slice(0, 1).map(mapAddressToReceiveAccount), sectionData: undefined, }, { key: 'used', label: translate('moduleTrading.accountSheet.usedAddresses'), - data: used.map(address => ({ account: selectedAccount, address })), + data: used.map(mapAddressToReceiveAccount), sectionData: undefined, }, ]Also applies to: 62-62
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (2)
67-67: Consider making the default symbol configurable.The default symbol 'btc' is hardcoded. Consider making it configurable through a constant or configuration to improve maintainability.
- symbol={selectedAsset?.symbol ?? 'btc'} + symbol={selectedAsset?.symbol ?? DEFAULT_TRADING_SYMBOL}
59-65: Enhance accessibility for account selection.The account selection row could benefit from improved accessibility labels to better describe the current selection state.
<TradingOverviewRow title={translate('moduleTrading.tradingScreen.receiveAccount')} onPress={showSheet} noBottomBorder + accessibilityRole="button" + accessibilityLabel={ + selectedValue + ? `${translate('moduleTrading.tradingScreen.receiveAccount')}: ${selectedValue.account.accountLabel}` + : translate('moduleTrading.tradingScreen.receiveAccount') + } + accessibilityHint={translate('moduleTrading.tradingScreen.selectAccountHint')} >suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx (1)
43-43: Enhance accessibility for asset selection.While the basic accessibility is good, consider adding more ARIA attributes for better screen reader support.
- <Pressable onPress={onPress} accessibilityRole="radio" accessibilityLabel={name ?? symbol}> + <Pressable + onPress={onPress} + accessibilityRole="radio" + accessibilityLabel={name ?? symbol} + accessibilityState={{ checked: false }} + accessibilityHint={translate('moduleTrading.tradingScreen.selectAssetHint')} + >suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx (1)
23-23: Improve type safety for noOp function.Consider adding proper type annotation for the noOp function to match the callback type it replaces.
-const noOp = () => {}; +const noOp: (isFilterActive: boolean) => void = () => {};suite-native/module-trading/src/hooks/__tests__/useSelectedAccount.test.ts (1)
1-131: LGTM with suggestions for additional test cases.The test coverage is comprehensive but could be enhanced with the following scenarios:
- Test error handling when invalid account data is provided
- Test behavior when
onCloseoronAccountSelectcallbacks are undefined- Test behavior with other cryptocurrency types besides BTC and ETH
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (2)
43-123: Consider performance optimizations and accessibility improvements.
- Memoize expensive computations and callbacks:
-export const AccountListItem = ({ +export const AccountListItem = React.memo(({ symbol, receiveAccount: { account, address }, onPress, }: AccountListItemProps) => { const { applyStyle } = useNativeStyles(); const { translate } = useTranslate(); const { DisplaySymbolFormatter, FiatAmountFormatter, CryptoAmountFormatter } = useFormatters(); + const handlePress = useCallback(() => { + onPress(); + }, [onPress]); const cryptoValue = address?.balance ?? account.availableBalance; const fiatValue = useFiatFromCryptoValue({ symbol, cryptoValue }); // ... rest of the component -}; +});
- Enhance accessibility by adding more descriptive labels:
<Pressable - accessibilityRole="radio" + accessibilityRole="button" + accessibilityState={{ selected: true }} accessibilityLabel={account.accountLabel} + accessibilityHint={translate('moduleTrading.accountSheet.selectAccountHint')} onPress={onPress} >
13-17: Improve type safety with stricter prop types.Consider making the callback prop more specific:
- onPress: () => void; + onPress: (account: ReceiveAccount) => void;suite-native/module-trading/src/components/general/AccountSheet/__tests__/AccountListItem.comp.test.tsx (2)
8-10: Consider using a more realistic mock for useFiatFromCryptoValue.The current mock simply returns the input value. A more realistic mock would simulate actual currency conversion:
jest.mock('@suite-native/formatters', () => ({ - useFiatFromCryptoValue: ({ cryptoValue }: { cryptoValue: string }) => cryptoValue, + useFiatFromCryptoValue: ({ cryptoValue, symbol }: { cryptoValue: string, symbol: string }) => { + const mockRates = { btc: 50000, eth: 3000 }; + return (Number(cryptoValue) * (mockRates[symbol] || 1)).toString(); + }, }));
12-145: Add test cases for error scenarios and edge cases.Consider adding tests for:
- Invalid balance values
- Missing account label
- Network errors in formatters
- Extremely long addresses that need truncation
- RTL layout support
suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx (1)
12-204: Add error handling test cases.Consider adding tests for:
- Network errors when fetching account data
- Invalid account data in the store
- Race conditions when switching symbols rapidly
- Memory leaks from unsubscribed listeners
suite-native/module-trading/package.json (1)
18-18: Dependencies properly configured.The new dependencies are correctly added. For consistency with react-redux, consider pinning the version of react-native-reanimated.
- "react-native-reanimated": "3.16.7", + "react-native-reanimated": "^3.16.7",Also applies to: 22-23
📜 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 (19)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(2 hunks)suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/__tests__/AccountListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx(4 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(4 hunks)suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useSelectedAccount.test.ts(1 hunks)suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts(1 hunks)suite-native/module-trading/src/hooks/useSelectedAccount.ts(1 hunks)suite-native/module-trading/src/types.ts(2 hunks)suite-native/module-trading/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (2)
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.
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.
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
[error] 15-15: 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/TradingBottomSheetSectionList.tsx
[error] 89-89: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 92-92: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ 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 / 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=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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (4)
1-24: LGTM! Clean and focused type definitions.The import statements and type definitions are well-organized, and the
ListItemExtraDatatype is appropriately focused on the essentialisFavouriteproperty.
52-56: LGTM! Robust key generation.The key extractor function generates unique keys by combining all relevant properties (symbol, contract address, and favorite status).
93-110: LGTM! Well-structured section data.The section data is properly memoized and organized with clear keys and labels. The dependency array correctly includes both
favouritesandassetsData.
112-124: Verify the noSingletonSectionHeader prop usage.The
noSingletonSectionHeaderprop is used to disable section headers. Please verify if this is the intended behavior, as section headers could help users distinguish between favorites and all assets.Would you like me to help investigate alternative approaches to section header rendering?
suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx (1)
5-10: Add unit test coverage to ensure correct rendering.Testing that the appropriate translation IDs appear and that the component displays the correct text can help prevent regressions when translation keys change.
[nitpick, offer_assistance]
Would you like me to provide a basic test snippet or open a new issue to track the addition of test coverage?
suite-native/module-trading/src/types.ts (1)
19-22: LGTM: Type definition looks solid.The
ReceiveAccounttype is clearly defined and aligns well with the underlyingAccountand optionalAddressstructures.suite-native/module-trading/src/hooks/useSelectedAccount.ts (1)
20-24: Verify the account.addresses check.The condition
account.addresses && !addressmight need additional validation to ensure type safety and handle edge cases.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for Account type definition and usage of addresses property ast-grep --pattern 'type Account = { $$$ addresses?: $_ $$$ }'Length of output: 70
Action: Manually Verify the
account.addressesValidationOur initial search didn’t reveal an explicit definition for the
Accounttype containing theaddressesproperty. This lack of explicit output suggests one or more of the following:
- The type definition for
Account(with itsaddressesproperty) might be defined elsewhere, imported from an external source, or set up differently than the searched pattern.- Depending on how
addressesis implemented (likely as an array), the current conditionaccount.addresses && !addresswill pass even whenaddressesis an empty array—which might not be the intended behavior if you expect a non-empty list.To ensure type safety and properly handle edge cases, please:
- Manually verify the definition of the
Accounttype: Locate the actual type or interface forAccountin your repository. Ensure that theaddressesproperty is defined the way you expect.- Reevaluate the check on
account.addresses: Ifaddressesis an array, consider updating the condition (for example, using a length check withaccount.addresses?.length) to ensure it handles empty arrays appropriately.suite-native/module-trading/src/components/buy/BuyCard.tsx (1)
4-4: LGTM! Good refactoring.The changes improve separation of concerns by moving account selection logic to a dedicated
ReceiveAccountPickercomponent.Also applies to: 7-7, 52-52
suite-native/intl/src/en.ts (1)
1348-1358: Well-structured localization strings!The new
accountSheetstrings are well-organized, follow the established patterns, and provide clear, user-friendly messages for the account selection interface.suite-native/module-trading/tsconfig.json (1)
6-9: LGTM! Proper TypeScript project reference added.The reference to blockchain-link-types is correctly configured and follows the project's TypeScript setup pattern.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (1)
28-37: Enhance accessibility for back navigation.The back navigation using
SearchableSheetHeadershould provide more context about where the user is navigating back to.<SearchableSheetHeader onClose={clearSelectedAccount} title={selectedAccount.accountLabel} leftButtonIcon="caretLeft" - leftButtonA11yLabel={translate('generic.buttons.back')} + leftButtonA11yLabel={translate('generic.buttons.back') + ' to account selection'} />suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (1)
21-41: Consolidate similar style definitions.Multiple style definitions share common properties. Consider extracting shared styles to reduce duplication.
+const baseTextStyle = prepareNativeStyle(({ colors }) => ({ + color: colors.textDefault, +})); + +const baseAlignedTextStyle = prepareNativeStyle( + { extend: baseTextStyle }, + { + textAlign: 'right', + flex: 0, + }, +); + -const labelTextStyle = prepareNativeStyle(({ colors }) => ({ - color: colors.textDefault, +const labelTextStyle = prepareNativeStyle( + { extend: baseTextStyle }, + { flex: 1, -})); + }, +);suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx (2)
70-98: Optimize section data transformation.The
transformToInternalFlatListDatafunction creates new arrays on each render. Consider usinguseMemofor the item data transformation.const transformToInternalFlatListData = <T, U = undefined>( inputData: SectionListData<T, U>, noSingletonSectionHeader: boolean | undefined, ): ListInternalItemShape<T, U>[] => inputData.reduce( (acc, { key, label, data, sectionData }) => { - const itemsData = data.map( + const itemsData = useMemo(() => data.map( (item, index): ListInternalItemShape<T, U> => [ 'item', item, { isFirst: index === 0, isLast: index === data.length - 1, sectionData, }, ], - ); + ), [data, sectionData]);
150-158: Optimize items count calculation.The
itemsCountcalculation could be more efficient by using a single reduce operation that also tracks the sections count.- const sectionsCount = data.length; - - const itemsCount = useMemo( - () => - data.reduce( - (intermediateDataLength, { data: sectionData }) => - intermediateDataLength + sectionData.length, - 0, - ), - [data], - ); + const { sectionsCount, itemsCount } = useMemo( + () => data.reduce( + (acc, { data: sectionData }) => ({ + sectionsCount: acc.sectionsCount + 1, + itemsCount: acc.itemsCount + sectionData.length, + }), + { sectionsCount: 0, itemsCount: 0 }, + ), + [data], + );
📜 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 (7)
suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx(4 hunks)suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx(1 hunks)suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-native/module-trading/package.json
- suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
[error] 15-15: 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: 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: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-web
🔇 Additional comments (4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (1)
15-18: Fix empty object type in style definition.The empty object type
{}inprepareNativeStyleshould be replaced with a proper style type definition.-const wrapperStyle = prepareNativeStyle<{}>(({ spacings }) => ({ +const wrapperStyle = prepareNativeStyle<{ padding: number; gap: number }>(({ spacings }) => ({🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: 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/SearchableSheetHeader.tsx (3)
5-5: LGTM!The import of
IconNametype is correctly added to support the new icon prop.
15-16: LGTM!The new prop types are well-defined and follow React naming conventions.
64-68: LGTM!The props are correctly passed to
SheetHeaderTitlewith a good fallback pattern for accessibility labels.
suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (3)
25-34: Consider adjusting flex properties for better text handling.While the styling is generally good, the current flex setup might not handle very long text optimally. Consider using
flexShrink: 1instead offlex: 1for the label style to prevent text overflow issues while maintaining layout flexibility.const labelTextStyle = prepareNativeStyle<TextStyle>(({ colors }, { textColor }) => ({ color: colors[textColor], - flex: 1, + flexShrink: 1, }));
55-59: Enhance accessibility with role description.While the component includes good accessibility features, adding a role description would improve the user experience for screen readers.
<Pressable onPress={onPress} accessibilityRole="button" accessibilityLabel={account.accountLabel} + accessibilityRoleDescription={translate('moduleTrading.accountSheet.accountItemRole')} >
50-52: Simplify conditional logic.The conditional checks could be simplified for better readability and maintenance.
- const shouldDisplayCaret = !isAddressDetail && !!account.addresses; - const shouldDisplayBalance = !isAddressDetail || address?.balance != null; - const label = isAddressDetail ? address.address : account.accountLabel; + const shouldDisplayCaret = !isAddressDetail && Boolean(account.addresses); + const shouldDisplayBalance = Boolean(!isAddressDetail || address?.balance); + const label = address?.address ?? account.accountLabel;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.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: build
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (2)
13-23: LGTM! Well-structured type definitions.The type definitions are clear, properly exported, and follow TypeScript best practices. The constant for height helps maintain consistency across the UI.
47-48: Add error handling for undefined values.The crypto and fiat value calculations don't handle the case where both values could be undefined.
- const cryptoValue = isAddressDetail ? address.balance : account.availableBalance; + const cryptoValue = isAddressDetail ? address?.balance ?? 0n : account.availableBalance ?? 0n; const fiatValue = useFiatFromCryptoValue({ symbol, cryptoValue });
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx (1)
33-33: Add null check for data.The nullish coalescing operator is used but the type of data is not clear. Consider adding type annotation for better type safety.
-const data = useReceiveAccountsListData(symbol, selectedAccount) ?? []; +const data: ReceiveAccount[] = useReceiveAccountsListData(symbol, selectedAccount) ?? [];suite-native/module-trading/src/components/buy/BuyCard.tsx (1)
4-4: Remove unused import.The
Translationimport is only used once for the buy title. Consider moving it closer to where it's used.suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (1)
81-81: Avoid hardcoded fallback value.The hardcoded fallback of 'btc' might not be suitable for all use cases. Consider making it configurable or using a more generic default.
Apply this diff:
-symbol={selectedSymbol ?? 'btc'} +symbol={selectedSymbol ?? DEFAULT_NETWORK_SYMBOL}Add the constant at the top of the file:
const DEFAULT_NETWORK_SYMBOL: NetworkSymbol = 'btc';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
suite-native/module-trading/src/components/buy/BuyCard.tsx(2 hunks)suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
[error] 15-15: 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: 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: build
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (2)
9-13: LGTM!The type definition is clear and follows React's naming conventions.
20-51: LGTM!The component implementation is well-structured with:
- Clear conditional rendering
- Proper use of hooks
- Good internationalization support
suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx (3)
11-16: LGTM!The type definition is clear and follows React's naming conventions.
18-19: Improve key extractor robustness.The key extractor could be more robust by using better fallback values.
-const keyExtractor = (item: ReceiveAccount) => - `${item.account.key}_${item.address?.address ?? 'address_undefined'}`; +const keyExtractor = (item: ReceiveAccount) => + `${item.account.key}_${item.address?.address ?? item.account.accountIndex}`;
21-60: Consider implementing loading state.The component should handle loading state from
useReceiveAccountsListDatahook.Would you like me to help implement a loading state component that shows a skeleton UI while the data is being fetched?
suite-native/module-trading/src/components/buy/BuyCard.tsx (1)
52-52: LGTM!The integration of
ReceiveAccountPickerwithselectedSymbolprop looks good and aligns with the PR objectives for mobile trade account functionality.suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (4)
28-32: LGTM!The component is well-structured and reusable, with consistent text styling.
34-58: LGTM!The component follows the coding style by accepting specific props instead of whole objects, and handles different states appropriately.
74-77: Pass specific props instead of whole objects.Following the coding style mentioned in past reviews, pass only the required properties instead of accessing them through nested objects.
Apply this diff:
-<ReceiveAccountPickerRight - selectedAccountLabel={selectedValue?.account.accountLabel} - selectedAddress={selectedValue?.address?.address} -/> +<ReceiveAccountPickerRight + selectedAccountLabel={selectedValue?.accountLabel} + selectedAddress={selectedValue?.address} +/>
60-88: Consider renaming toAccountPicker.As suggested in past reviews, this component might be used for selling or swapping as well. Consider renaming it to just
AccountPickerfor broader use.
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (2)
34-58: Add explicit return type annotation for better type safety.Consider adding an explicit return type annotation to the component for better type safety and documentation.
-const ReceiveAccountPickerRight = ({ +const ReceiveAccountPickerRight = ({ selectedAccountLabel, selectedAddress, -}: ReceiveAccountPickerRightProps) => { +}: ReceiveAccountPickerRightProps): JSX.Element => {
81-81: Make the fallback symbol more explicit.Consider defining the fallback symbol as a constant to make the code more maintainable and self-documenting.
+const DEFAULT_NETWORK_SYMBOL: NetworkSymbol = 'btc'; + export const ReceiveAccountPicker = ({ selectedSymbol }: ReceiveAccountPickerProps) => { // ... - symbol={selectedSymbol ?? 'btc'} + symbol={selectedSymbol ?? DEFAULT_NETWORK_SYMBOL}suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (1)
19-50: Consider memoizing the component for better performance.Since this component is likely to be rendered within a list or frequently updated parent component, consider wrapping it with React.memo to prevent unnecessary re-renders.
-export const AccountSheetHeader = ({ +export const AccountSheetHeader = React.memo(({ selectedAccountLabel, clearSelectedAccount, onClose, }: AccountSheetHeaderProps) => { const { applyStyle } = useNativeStyles(); const { translate } = useTranslate(); // ... rest of the component implementation -}; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
[error] 14-14: 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: 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: build
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (3)
1-27: LGTM! Well-structured imports and type definitions.The imports are well-organized, and the type definitions are clear and focused, following TypeScript best practices.
28-32: LGTM! Clean and focused component implementation.The component is well-designed with proper text overflow handling and clear prop types.
60-88: Consider future component naming.Based on past review comments, this component might be used for selling or swapping in the future. While the current name is accurate for its current use, keep in mind that it might need to be renamed to
AccountPickerwhen the functionality expands.Would you like me to create an issue to track this potential future refactoring?
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (3)
1-6: LGTM!The imports are well-organized and all dependencies are used in the component.
8-12: LGTM!The type definition is clear and well-structured, with appropriate types for each prop.
14-17: Fix the empty object type.The empty object type
{}should be replaced with a more specific type.-const wrapperStyle = prepareNativeStyle<{}> +const wrapperStyle = prepareNativeStyle<Record<string, never>>🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
|
/rebase |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (2)
10-12: Add JSDoc comments to improve props documentation.Consider adding JSDoc comments to describe the purpose of the
onClosecallback and any specific behavior expectations.type TradeableAssetsSheetHeaderProps = { + /** Callback invoked when the sheet should be closed */ onClose: () => void; };
32-37: Consider memoizing the Animated.View content.The
Animated.Viewcontent could be memoized to prevent unnecessary re-renders when parent component props change.+const MemoizedFilterTabs = React.memo(({ visible, animationDuration }: { + visible: boolean; + animationDuration: number; +}) => ( + <TradeableAssetsFilterTabs + visible={visible} + animationDuration={animationDuration} + /> +)); <Animated.View layout={LinearTransition.duration(FOCUS_ANIMATION_DURATION)}> - <TradeableAssetsFilterTabs - visible={isFilterActive} - animationDuration={FOCUS_ANIMATION_DURATION} - /> + <MemoizedFilterTabs + visible={isFilterActive} + animationDuration={FOCUS_ANIMATION_DURATION} + /> </Animated.View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx(5 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx
- suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-deploy
- GitHub Check: build
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- 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: prepare_android_test_app
- 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: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx (1)
16-16: LGTM! Type parameter removal improves code clarity.The removal of the empty object type parameter
<{}>fromprepareNativeStyleis a good change as it was redundant. Type inference will work correctly for the simple style object being returned.
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13311609986 |
trezor-ci
force-pushed
the
16638-Mobile-Trade-Account-and-address-picker-stub
branch
from
1c4515b to
b66e7b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (1)
47-48: 🛠️ Refactor suggestionAdd fallback for undefined cryptoValue.
The cryptoValue calculation should handle cases where both values are undefined.
- const cryptoValue = isAddressDetail ? address.balance : account.availableBalance; + const cryptoValue = isAddressDetail ? address?.balance ?? '0' : account.availableBalance ?? '0';
🧹 Nitpick comments (17)
suite-native/module-trading/src/components/buy/BuyCard.tsx (3)
32-34: Consider replacing hardcoded values with constants or props.The hardcoded "0.0" value appears to be a placeholder. Consider:
- Moving it to a constant
- Making it configurable via props
- Adding a comment explaining its purpose if it's intentionally static
38-43: Enhance error state handling.The current ternary operator for
selectedValue?.symbolcould be improved for better maintainability.Consider extracting this logic into a separate function for better readability:
-{selectedValue?.symbol ? ( - <CryptoAmountFormatter value="0" symbol={selectedValue.symbol} /> -) : ( - '-' -)} +{renderCryptoAmount(selectedValue)} +const renderCryptoAmount = (selectedValue?: TradeableAsset) => { + if (!selectedValue?.symbol) return '-'; + return <CryptoAmountFormatter value="0" symbol={selectedValue.symbol} />; +};
18-55: Add TypeScript interface for component props.While the component currently doesn't accept props, it's good practice to explicitly define the interface for future maintainability.
Consider adding:
interface BuyCardProps { // Add props as needed } export const BuyCard: React.FC<BuyCardProps> = () => { // ... existing implementation };suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx (1)
43-43: Consider revising the accessibility role.The
accessibilityRole="radio"might not be the most appropriate choice for this context. A list item or button role would be more semantic for a selectable asset item.- <Pressable onPress={onPress} accessibilityRole="radio" accessibilityLabel={name ?? symbol}> + <Pressable onPress={onPress} accessibilityRole="button" accessibilityLabel={name ?? symbol}>suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (1)
28-32: Consider enhancing type safety and reusability.The component could benefit from:
- Using a union type for the variant prop instead of string literals
- Making it more reusable by accepting additional text props
-type RightTextProps = { +type TextVariant = 'body' | 'hint'; +type RightTextProps = { children: ReactNode; color: Color; - variant?: 'body' | 'hint'; + variant?: TextVariant; + numberOfLines?: number; }; const RightText = ({ color, variant = 'body', children }: RightTextProps) => ( <Text color={color} variant={variant} textAlign="right" ellipsizeMode="tail" numberOfLines={1}> {children} </Text> );suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx (1)
18-19: Improve keyExtractor robustness.The keyExtractor falls back to a static string 'address_undefined' which could potentially cause key conflicts. Consider using a more unique identifier.
- `${item.account.key}_${item.address?.address ?? 'address_undefined'}`; + `${item.account.key}_${item.address?.address ?? item.account.accountIndex ?? 'no_address'}`;suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (1)
62-66: Implement toggleFavourite functionality.The TODO comment indicates missing implementation for the favourite toggle feature.
Would you like me to help implement the toggleFavourite functionality or create an issue to track this task?
suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (1)
96-106: Simplify conditional rendering logic.The nested conditional rendering could be simplified by combining conditions.
- {shouldDisplayBalance && fiatValue && ( + {shouldDisplayBalance && !!fiatValue && (suite-native/module-trading/src/components/general/AccountSheet/__tests__/AccountListItem.comp.test.tsx (2)
8-10: Enhance mock implementation.The formatter mock could be more robust by handling edge cases and invalid inputs.
jest.mock('@suite-native/formatters', () => ({ - useFiatFromCryptoValue: ({ cryptoValue }: { cryptoValue: string }) => cryptoValue, + useFiatFromCryptoValue: ({ cryptoValue }: { cryptoValue: string | undefined }) => + cryptoValue ? cryptoValue : '0', }));
33-40: Reduce test data duplication.Consider extracting common test data setup to reduce duplication across test cases.
const createTestAccount = (overrides = {}) => ({ key: 'btc1', symbol: 'btc', accountLabel: 'My BTC account', availableBalance: '10000000', ...overrides } as Account); const createTestAddress = (overrides = {}) => ({ address: 'BTC_address', balance: '5000000', ...overrides } as Address);Also applies to: 49-56, 67-79, 87-98, 110-121, 129-139
suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx (5)
7-11: Consider constraining the type parameter U.The type parameter U in ItemRenderConfig is unconstrained. Consider adding a constraint if there are specific requirements for the section data type.
-export type ItemRenderConfig<U> = { +export type ItemRenderConfig<U extends Record<string, unknown>> = {
13-30: Improve documentation for omitted props.The comment "not supported" could be more descriptive to explain why these props are not supported.
- // not supported + // These props are not supported as they would conflict with the section list's internal implementation
45-68: Consider using design system tokens for SECTION_HEADER_HEIGHT.The magic number 48 for SECTION_HEADER_HEIGHT could be derived from the design system's spacing tokens for consistency.
-const SECTION_HEADER_HEIGHT = 48 as const; +const SECTION_HEADER_HEIGHT = spacings.sp48 as const;
70-97: Consider performance optimizations for data transformation.The itemsData mapping could be memoized to prevent unnecessary recalculations, and the reduce accumulator type could be more explicit.
const transformToInternalFlatListData = <T, U = undefined>( inputData: SectionListData<T, U>, noSingletonSectionHeader: boolean | undefined, ): ListInternalItemShape<T, U>[] => inputData.reduce( (acc, { key, label, data, sectionData }) => { - const itemsData = data.map( + const itemsData = useMemo(() => data.map( (item, index): ListInternalItemShape<T, U> => [ 'item', item, { isFirst: index === 0, isLast: index === data.length - 1, sectionData, }, ], - ); + ), [data, sectionData]); if (!noSingletonSectionHeader || inputData.length > 1) { acc.push(['sectionHeader', label, key]); } acc.push(...itemsData); return acc; }, [] as ListInternalItemShape<T, U>[], );
115-136: Consider accessibility and component extraction.The render function could be improved by:
- Extracting the section header into a reusable component
- Adding accessibility props for better screen reader support
+const SectionHeader = ({ label }: { label: ReactNode }) => ( + <Box paddingVertical="sp12"> + <Text + variant="hint" + color="textSubdued" + accessibilityRole="header" + accessibilityLabel={`Section ${label}`} + > + {label} + </Text> + </Box> +); const renderInternalItem = <T, U>( item: ListInternalItemShape<T, U>, renderItem: (item: T, config: ItemRenderConfig<U>) => ReactElement, applyStyle: ReturnType<typeof useNativeStyles>['applyStyle'], ): ReactElement => { switch (item[0]) { case 'sectionHeader': - return ( - <Box paddingVertical="sp12"> - <Text variant="hint" color="textSubdued"> - {item[1]} - </Text> - </Box> - ); + return <SectionHeader label={item[1]} />; case 'item': - return <Box style={applyStyle(itemStyle, item[2])}>{renderItem(item[1], item[2])}</Box>; + return ( + <Box + style={applyStyle(itemStyle, item[2])} + accessibilityRole="listitem" + > + {renderItem(item[1], item[2])} + </Box> + ); default: throw new UnreachableCaseError(item[0]); } };suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx (2)
12-85: Consider adding test cases for error handling and edge cases.The test setup is good, but consider adding tests for:
- Invalid network symbols
- Empty or malformed account data
- Error cases when the store is in an invalid state
it('should handle invalid network symbols gracefully', async () => { const { result } = await renderUseReceiveAccountsListDataHook( 'invalid' as NetworkSymbol, undefined ); expect(result.current).toEqual([]); }); it('should handle malformed account data', async () => { const malformedState = { ...defaultPreloadedState, wallet: { accounts: [{ symbol: 'btc' }] as unknown as Account[], }, }; const { result } = await renderUseReceiveAccountsListDataHook( 'btc', undefined, malformedState ); expect(result.current).toEqual([]); });
86-118: Consider testing symbol validation more thoroughly.Add test cases to verify that the hook properly validates and handles different network symbols.
it('should handle case-sensitive network symbols', async () => { const { result } = await renderUseReceiveAccountsListDataHook( 'BTC' as NetworkSymbol, undefined ); expect(result.current).toEqual([]); }); it('should handle network symbol changes between BTC-like assets', async () => { const { result, rerender } = await renderUseReceiveAccountsListDataHook( 'btc', undefined ); rerender({ symbol: 'ltc' as NetworkSymbol, selectedAccount: undefined }); expect(result.current).toEqual([]); });
📜 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 (22)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(2 hunks)suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/__tests__/AccountListItem.comp.test.tsx(1 hunks)suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx(5 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/TradeableAssetListItem.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(4 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx(1 hunks)suite-native/module-trading/src/hooks/__tests__/useSelectedAccount.test.ts(1 hunks)suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts(1 hunks)suite-native/module-trading/src/hooks/useSelectedAccount.ts(1 hunks)suite-native/module-trading/src/types.ts(2 hunks)suite-native/module-trading/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- suite-native/module-trading/src/types.ts
- suite-native/module-trading/tsconfig.json
- suite-native/module-trading/src/components/general/TradeableAssetButton.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheetHeader.tsx
- suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx
- suite-native/module-trading/package.json
- suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx
- suite-native/module-trading/src/hooks/useSelectedAccount.ts
- suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts
- suite-native/module-trading/src/components/general/SelectTradeableAssetButton.tsx
- suite-native/module-trading/src/components/general/SearchableSheetHeader.tsx
- suite-native/intl/src/en.ts
- suite-native/module-trading/src/hooks/tests/useSelectedAccount.test.ts
🧰 Additional context used
🧠 Learnings (1)
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (2)
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Analyze with CodeQL (javascript)
- 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: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- 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: build-deploy
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
🔇 Additional comments (12)
suite-native/module-trading/src/components/buy/BuyCard.tsx (1)
1-10: LGTM! Well-organized imports.The imports are logically grouped and all appear to be used within the component.
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetListItem.tsx (3)
10-17: LGTM! Clean and well-structured props interface.The
AssetListItemPropstype provides a comprehensive and self-documenting interface for the component. The removal of positional props (isFirst,isLast) aligns with modern list rendering practices.
19-27: LGTM! Well-structured and maintainable styling.The styling implementation follows best practices:
- Uses constant for consistent item height
- Leverages flexbox for robust layouts
- Applies proper spacing and alignment
29-70: LGTM! Clean and robust component implementation.The component demonstrates good practices:
- Proper use of formatters for consistent data presentation
- Graceful handling of null values
- Clear separation of concerns in the layout structure
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (3)
1-27: LGTM! Well-structured type definitions and imports.The type definitions are clear, properly typed, and follow TypeScript best practices. The imports are minimal and logically grouped.
34-58: LGTM! Clean implementation with proper state handling.The component effectively handles different states of account selection and follows the team's coding style by accepting only the required props.
60-88: Consider making the component more flexible and generic.
- The component name could be more generic as it might be used for selling or swapping in the future.
- The default symbol 'btc' could be made configurable through props.
-type ReceiveAccountPickerProps = { +type AccountPickerProps = { selectedSymbol?: NetworkSymbol; + defaultSymbol?: NetworkSymbol; }; -export const ReceiveAccountPicker = ({ selectedSymbol }: ReceiveAccountPickerProps) => { +export const AccountPicker = ({ selectedSymbol, defaultSymbol = 'btc' }: AccountPickerProps) => { // ... return ( <> <AccountSheet - symbol={selectedSymbol ?? 'btc'} + symbol={selectedSymbol ?? defaultSymbol} // ... /> </> ); };suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx (1)
33-33: Add loading state handling.The hook result is used with nullish coalescing but there's no handling of the loading state.
Consider implementing a loading state to improve user experience. Would you like me to help implement a loading state component?
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx (1)
73-74: 💡 Verification agent🧩 Analysis chain
Replace mock data functions.
The mock functions for price data should be replaced with actual implementations.
🏁 Script executed:
#!/bin/bash # Search for other mock data usage rg "getMockFiatRate|getMockPriceChange" -A 2Length of output: 1106
Action Required: Replace Mock Data Functions with Real Implementations
The file
suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsxstill defines and uses mock functions for pricing data:
getMockPriceChange()returns a random value computed asMath.random() * 3 - 1.getMockFiatRate()returns a random value computed asMath.random() * 1000.Please update these functions with actual implementations that fetch real market price data and fiat rates, ensuring consistency with the component's expected data contracts.
⛔ Skipped due to learnings
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/components/general/TradingBottomSheetSectionList.tsx (2)
99-113: LGTM! Well-implemented key extractor.The key extractor function is well-implemented with proper type safety and exhaustive type checking using UnreachableCaseError.
138-184: LGTM! Well-implemented component with proper optimizations.The component is well-implemented with:
- Proper use of generics
- Performance optimizations using useMemo
- Accurate list size estimation
suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx (1)
120-204: LGTM! Comprehensive test coverage for account selection scenarios.The test cases for account selection scenarios are well-written and cover important use cases:
- Non-BTC assets
- BTC-like assets with addresses
- Empty sections
- Account visibility
jbazant
deleted the
16638-Mobile-Trade-Account-and-address-picker-stub
branch
Description
Related Issue
Resolve #16638
Screenshots: