feat(suite-native): enable and discover account via trade #17266
Conversation
|
🚀 Expo preview is ready!
|
WalkthroughThe changes span several modules and include a substantial refactoring of internationalization, navigation, hooks, and state management. In the internationalization files, messages formerly grouped under “accountSheet” are now restructured under “accountScreen” with revised keys and nested objects. The add accounts module updates the hook logic by introducing new type definitions, additional functions for confirmation and navigation, and changes to function signatures. In the trading module, Redux is now used for managing the selected receive account, accessibility labels have been updated, obsolete components and tests have been removed, and navigation routes have been expanded to include a new receive accounts screen. Additionally, new dependencies were added and a custom hook for managing sectioned lists was introduced. Type declarations and parameters were also updated in various screens and test files to reflect these structural and navigation modifications. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
suite-native/module-trading/src/hooks/useSectionListData.tsx (2)
8-12: Rename or clarify theItemRenderConfigtype name for broader usage.
WhileItemRenderConfigis descriptive, consider adding more context or a doc comment about its purpose and usage, as it’s a core interface for the item rendering logic.
137-187: Excellent custom hook architecture for section lists.
The combined usage ofuseMemofor item counts and list size calculations is beneficial for performance. Ensure that upstream changes (e.g., dynamic item heights) are tracked if future design requirements evolve.suite-native/module-trading/src/components/general/AccountSheet/NoAccountsComponent.tsx (1)
34-59: Good implementation of conditional UI based on device mode.The component correctly implements different messages based on the device's view-only mode status, which aligns with the PR requirement that account discovery should be restricted when the device is not connected.
Consider extracting the translation IDs into constants to avoid hardcoding strings in the component:
+const TRANSLATION_IDS = { + VIEW_ONLY_TITLE: 'moduleTrading.accountSheet.accountEmpty.viewOnly.title', + VIEW_ONLY_DESCRIPTION: 'moduleTrading.accountSheet.accountEmpty.viewOnly.description', + NETWORK_NOT_ENABLED_TITLE: 'moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title', + NETWORK_NOT_ENABLED_DESCRIPTION: 'moduleTrading.accountSheet.accountEmpty.networkNotEnabled.description' +}; export const NoAccountsComponent = ({ isBottomRounded }: NoAccountsComponentProps) => { const { applyStyle } = useNativeStyles(); const isDeviceInViewOnlyMode = useSelector(selectIsDeviceInViewOnlyMode); const title = isDeviceInViewOnlyMode ? ( - <Translation id="moduleTrading.accountSheet.accountEmpty.viewOnly.title" /> + <Translation id={TRANSLATION_IDS.VIEW_ONLY_TITLE} /> ) : ( - <Translation id="moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title" /> + <Translation id={TRANSLATION_IDS.NETWORK_NOT_ENABLED_TITLE} /> ); const description = isDeviceInViewOnlyMode ? ( - <Translation id="moduleTrading.accountSheet.accountEmpty.viewOnly.description" /> + <Translation id={TRANSLATION_IDS.VIEW_ONLY_DESCRIPTION} /> ) : ( - <Translation id="moduleTrading.accountSheet.accountEmpty.networkNotEnabled.description" /> + <Translation id={TRANSLATION_IDS.NETWORK_NOT_ENABLED_DESCRIPTION} /> );suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx (1)
154-159: Ensure empty state handling is comprehensive.The filter variable is defined but appears to not be used anywhere else in the component. Consider removing it if unused or implement filtering functionality.
-const filter = ''; -const emptyComponent = - filter.length > 0 ? ( - <AddressListEmptyComponent /> - ) : ( - <NoAccountsComponent isBottomRounded={isDeviceInViewOnlyMode} /> - ); +const emptyComponent = pickerMode === 'address' + ? <AddressListEmptyComponent /> + : <NoAccountsComponent isBottomRounded={isDeviceInViewOnlyMode} />;suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts (1)
377-389: Added account type confirmation handler with timeout.The new
handleAccountTypeConfirmationfunction improves the user experience by streamlining the confirmation process and includes a timeout to prevent UI crashes during transitions.Consider extracting the timeout value to a named constant to make the code more maintainable:
+const ACCOUNT_TYPE_CONFIRMATION_TIMEOUT = 100; // ms + const handleAccountTypeConfirmation = (flowType: AddCoinFlowType) => { if (networkSymbolWithTypeToBeAdded) { // Timeout is needed so AccountTypeDecisionBottomSheet has time to hide otherwise app crashes setTimeout(() => { addCoinAccount({ symbol: networkSymbolWithTypeToBeAdded[0], accountType: networkSymbolWithTypeToBeAdded[1], flowType, }); - }, 100); + }, ACCOUNT_TYPE_CONFIRMATION_TIMEOUT); clearNetworkWithTypeToBeAdded(); } };suite-native/module-add-accounts/src/screens/AddCoinDiscoveryFinishedScreen.tsx (1)
115-115: Confirm coin name usage.
PassingnetworkSymbolascoinNamemight differ from the human-readable coin name. If needed, consider retrieving the full coin name fromgetNetwork(networkSymbol).namefor consistency.suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (1)
92-96: Conditional navigation inopenAccountPicker.
Selecting the route only whenselectedSymbolis defined prevents erroneous navigation. Consider logging or providing user feedback ifselectedSymbolis absent.
📜 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 (25)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts(11 hunks)suite-native/module-add-accounts/src/index.ts(1 hunks)suite-native/module-add-accounts/src/screens/AddCoinAccountScreen.tsx(1 hunks)suite-native/module-add-accounts/src/screens/AddCoinDiscoveryFinishedScreen.tsx(4 hunks)suite-native/module-add-accounts/src/screens/AddCoinDiscoveryRunningScreen.tsx(2 hunks)suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx(5 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/src/components/buy/BuyCard.tsx(4 hunks)suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(3 hunks)suite-native/module-trading/src/components/buy/__tests__/ReceiveAccountPicker.comp.test.tsx(0 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx(0 hunks)suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/AccountSheet/NoAccountsComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx(2 hunks)suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx(10 hunks)suite-native/module-trading/src/hooks/__tests__/useSelectedAccount.test.ts(0 hunks)suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts(3 hunks)suite-native/module-trading/src/hooks/useSectionListData.tsx(1 hunks)suite-native/module-trading/src/hooks/useSelectedAccount.ts(0 hunks)suite-native/module-trading/src/navigation/TradingStackNavigator.tsx(2 hunks)suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx(1 hunks)suite-native/navigation/src/navigators.ts(2 hunks)suite-native/navigation/src/routes.ts(1 hunks)
💤 Files with no reviewable changes (4)
- suite-native/module-trading/src/components/buy/tests/ReceiveAccountPicker.comp.test.tsx
- suite-native/module-trading/src/components/general/AccountSheet/AccountSheet.tsx
- suite-native/module-trading/src/hooks/useSelectedAccount.ts
- suite-native/module-trading/src/hooks/tests/useSelectedAccount.test.ts
✅ Files skipped from review due to trivial changes (1)
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
🔇 Additional comments (70)
suite-native/module-trading/src/hooks/useSectionListData.tsx (6)
1-3: Nice use of React Native Reanimated imports.
The fade-in/out animations bring a smooth visual transition for section headers and items.
14-25: Thorough type definitions for sectioned data and internal item shapes.
These definitions effectively separate concerns for section headers versus items. This enhances readability and helps ensure correct usage in the rendering logic.
36-59: Conditional border styling is well-structured.
Using theextendarray to conditionally apply corner radii based onisFirst/isLastprovides clean and maintainable styling for card-like sections.
60-88: Verify the single-section header logic.
(!noSingletonSectionHeader || inputData.length > 1)ensures headers are rendered for multiple sections or ifnoSingletonSectionHeaderis false, but consider edge cases where a single section might contain many items. If the design requires a header even for one large section, this logic might need revisiting.
90-104: Great usage ofUnreachableCaseError.
This pattern enforces exhaustive handling of union types, catching changes in the data structure at compile time. Keep it up!
106-135: Clear separation of section header vs. item rendering.
The switch statement is straightforward and leveragesAnimatedBoxfor subtle transitions, resulting in well-organized item rendering code.suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx (4)
1-3: Imports are concise and relevant.
Removing unnecessary imports keeps the file cleaner.
5-10: Defer to new hook definitions for data structures.
CentralizingItemRenderConfig,SectionListData, andListInternalItemShapein the hook promotes a more modular design.
39-50: Seamless integration withuseSectionList.
The component effectively delegates data transformation and rendering concerns to the new hook, simplifying local logic.
53-55: UsingBottomSheetFlashListwith internal data.
Passing the derived props (internalData,internalKeyExtractor,internalRenderItem) keeps the component minimal and focused on its role.suite-native/navigation/src/routes.ts (1)
129-132: Nice addition to the trading stack routes!Adding
ReceiveAccountsto theTradingStackRoutesenum aligns well with the PR objective of enabling account discovery within the trade section. This route will be used for the new account selection screen where users can choose accounts for receiving assets.suite-native/module-add-accounts/src/index.ts (1)
2-3: Good modularity improvementExposing these hooks and components makes them available for the trading module to utilize, which is essential for the account discovery feature described in the PR objectives. This approach promotes code reuse across modules.
suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx (1)
7-8:❓ Verification inconclusive
Translation keys restructured for better organization
The translation keys have been updated to use a more hierarchical structure, changing from flat keys to nested ones. This is a good practice for better organization of internationalization resources.
Make sure these new translation keys are properly added to all language files to prevent any missing translations.
🏁 Script executed:
#!/bin/bash # Search for the new translation keys in language files echo "Checking for new translation keys in language files" grep -r "moduleTrading.accountSheet.addressEmpty" --include="*.ts" suite-native/intl/src/Length of output: 202
Translation Keys Update Verification Needed
The AddressListEmptyComponent now uses the updated translation keys ("moduleTrading.accountSheet.addressEmpty.title" and "moduleTrading.accountSheet.addressEmpty.description") to enforce a more hierarchical structure. However, our search in the language files (under suite-native/intl/src) did not return any matches for these keys. Please manually verify that these keys have been added to all relevant language files (including both TS and, if applicable, JSON files) to prevent missing translations at runtime.
suite-native/module-trading/src/navigation/TradingStackNavigator.tsx (2)
9-9: Clean import addition.Import for the new ReceiveAccountsPickerScreen component is correctly added to support the new navigation route.
24-28: Good implementation of the new screen route.The ReceiveAccounts screen is properly added to the TradingStack navigator with appropriate title and component configuration, which enables the account discovery functionality mentioned in the PR objectives.
suite-native/module-add-accounts/src/screens/AddCoinAccountScreen.tsx (2)
28-28: Function rename looks good.The change from
addCoinAccounttohandleAccountTypeConfirmationimproves readability by clearly describing the function's purpose.
36-36:❓ Verification inconclusive
Simplified implementation.
The handleConfirmTap function has been simplified to directly call handleAccountTypeConfirmation with the flowType parameter, removing any previous timeout logic.
Verify that the timeout logic is properly implemented in the handleAccountTypeConfirmation function to prevent potential UI freezes during confirmation:
🏁 Script executed:
#!/bin/bash # Search for the implementation of handleAccountTypeConfirmation rg -A 10 "handleAccountTypeConfirmation.*=" --type typescriptLength of output: 98
I'll re-run the verification without specifying the file type to ensure we capture all possible locations for the
handleAccountTypeConfirmationimplementation. Please run the following script:#!/bin/bash # Searching for the implementation of handleAccountTypeConfirmation without specifying file type rg -A 10 "handleAccountTypeConfirmation" .Once you obtain the output, we can verify if the timeout logic is integrated as expected in the
handleAccountTypeConfirmationfunction. Please share the results.suite-native/module-trading/src/components/general/AccountSheet/NoAccountsComponent.tsx (2)
8-10: Good props definition.The props interface is clear and properly typed with a boolean flag for styling control.
12-32: Well-structured styling.The styling approach using prepareNativeStyle with conditional styling for bottom corners is clean and maintainable.
suite-native/navigation/src/navigators.ts (2)
146-146: Good type extension.Adding 'trade' to AddCoinFlowType properly extends the navigation capabilities to support the account discovery functionality within the trading context.
236-236: Well-defined route parameter.The new ReceiveAccounts route is correctly defined with a required symbol parameter of type NetworkSymbol, which enables proper type checking for this route.
suite-native/module-trading/src/components/buy/BuyCard.tsx (5)
2-2: Moving to Redux for state management.You've added Redux imports here, which is consistent with the architectural changes needed to support account discovery within the trading flow.
14-15: Appropriate imports for Redux integration.The imports for Redux selectors and actions are properly added, along with the required type import.
27-28: Redux setup for account management.Good implementation of the Redux hooks to manage the selected receive account state centrally, which will make it easier to share this state with other components that need to access it.
35-37: State management moved to Redux.The useEffect hook now properly dispatches a Redux action instead of using local state. This is a good change for centralizing state management, particularly for features that need to be accessed across different components.
64-64: Simplified ReceiveAccountPicker props.You've simplified the component by only passing the selectedSymbol and removing the local state management props, which is appropriate since this state is now managed in Redux.
suite-native/module-add-accounts/src/screens/AddCoinDiscoveryRunningScreen.tsx (3)
21-26: Import structure improvements.Good organization of imports with explicit type imports from navigation, which improves code readability and maintainability.
30-32: Enhanced type safety with StackProps.The component now properly types its props using StackProps with specific route parameters, which improves type safety and developer experience.
62-66: Special handling for trade flow.This new conditional block implements the special handling for the 'trade' flow type, allowing users to return to the trade section after account discovery. This is a key part of the new feature to enable account discovery from the trade section.
suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx (5)
28-32: Improved import structure.The destructured imports are now more explicit, which improves code readability.
89-95: New helper function for safer translation access.The
getAccountTypeTranslationsfunction adds robustness by safely checking if a translation exists before attempting to access it, which prevents potential runtime errors.
115-115: Safer access to translation keys.Now using the helper function to safely retrieve the translation key, which is more robust against potential missing translations.
145-149: Null check for missing translations.Good defensive programming by checking if translation data exists before attempting to use it, which prevents potential errors if account types are added without corresponding translations.
187-189: Conditional translation for undefined keys.The translation function now safely handles the case where accountTypeKey might be undefined, preventing potential rendering issues.
suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx (6)
10-10: Updated import for new mode parameter.The import now correctly includes the ReceiveAccountsListMode type which is used in the updated hook interface.
62-62: Added mode parameter to test helper function.The test helper function signature has been updated to include the new mode parameter, which matches the changes in the hook API.
66-67: Updated hook call with mode parameter.The hook call now correctly passes the mode parameter along with the other arguments, matching the updated hook interface.
77-77: Default mode in initial props.Setting a default mode in the initial props ensures backward compatibility in test cases that might not explicitly specify a mode.
91-95: Updated all test cases with mode parameter.All test cases now correctly include the mode parameter, ensuring complete test coverage for both 'account' and 'address' modes.
Also applies to: 110-113, 128-136, 145-157, 189-192, 220-224
116-116: Updated rerender call with mode parameter.The rerender call now correctly includes the mode parameter to maintain consistency with the initial render.
suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts (4)
16-22: Type definitions look good and improve code clarity.The introduction of clear type definitions for
ReceiveAccountsListModeandUseReceiveAccountsListDataPropsprovides better structure and makes the code more maintainable.
36-49: Improved conditional logic with the new mode parameter.The logic now relies on the explicit mode parameter rather than inferring it from whether selectedAccount exists. This makes the code's intention clearer and more maintainable.
78-78: Correctly updated useMemo dependencies.The
modeparameter has been properly added to the dependency array, ensuring the memoized value recalculates when the mode changes.
24-28:✅ Verification successful
API change improves flexibility but requires updates to all consumers.
The hook signature change from individual parameters to an object parameter follows React best practices and makes it easier to add or modify parameters in the future.
Note that this is a breaking change that requires updating all places where this hook is called.
🏁 Script executed:
#!/bin/bash # Find all usages of useReceiveAccountsListData to verify they've been updated rg "useReceiveAccountsListData\(" --type tsx --type tsLength of output: 84
Refactor hook API usage across the codebase
The updated hook signature now accepts an object parameter, which follows React best practices and allows for easier modifications in the future. However, since this is a breaking change, please ensure that every consumer of
useReceiveAccountsListDatahas been updated accordingly.
- Verify that there are no lingering calls using the old parameter format.
- Re-run the above script or perform a manual code review to confirm that all invocations match the updated hook signature.
🏁 Script executed:
#!/bin/bash # Re-run the search for all usages of useReceiveAccountsListData in .ts and .tsx files rg "useReceiveAccountsListData\(" -g "*.ts" -g "*.tsx"Length of output: 328
Hook API change verified in consumer usage
The updated hook signature (taking an object parameter) is now properly propagated. The search confirms that both the main consumer in suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx and the test in suite-native/module-trading/src/hooks/tests/useReceiveAccountsListData.test.tsx are updated to use the new object parameter.
No further changes are required.
suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx (6)
76-76: State management looks good.Proper initialization of picker mode state using the
ReceiveAccountsListModetype from the hooks file.
78-83: Good integration with the modified hook.The component correctly uses the updated
useReceiveAccountsListDatahook with the new parameter structure.
94-103: Account selection logic is well-implemented.The
onItemSelectfunction handles account selection, correctly updating Redux state and managing navigation based on whether the selected account has addresses.
123-129: Well-structured handler methods.The handler methods are concise and each have a single responsibility, following good functional programming principles.
133-151: Conditional footer rendering improves UX.The footer component is conditionally rendered based on device mode and picker mode, providing a good user experience with appropriate actions available in different states.
181-188: Good integration with AccountTypeDecisionBottomSheet.The component properly integrates with the
AccountTypeDecisionBottomSheetand passes all required props for account type selection.suite-native/intl/src/en.ts (1)
1294-1309: Improved i18n structure with more specific error messages.The restructuring of the accountSheet messages into nested objects provides better context-specific error messages for different empty states:
- View-only mode without accounts
- Network not enabled without accounts
- Address not found (preserved from previous structure)
This change aligns well with the new account selection functionality introduced in the ReceiveAccountsPickerScreen.
suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts (5)
53-53: Improved type safety with AddCoinEnabledAccountType.The new type properly excludes unsupported account types, making the code more type-safe.
189-196: Added support for 'trade' flow type.The navigation logic correctly handles the 'trade' flow type, checking the current route and using appropriate navigation actions.
233-233: Enhanced type safety with explicit parameter typing.The explicit type annotation for the flowType parameter improves type safety and code clarity.
350-363: Conditional navigation based on flow type.The function now handles navigation differently for 'trade' flow versus other flows, using the appropriate navigation method based on context.
401-401: Export for the new confirmation handler function.The function is properly added to the hook's return object, making it available to consumers.
suite-native/module-add-accounts/src/screens/AddCoinDiscoveryFinishedScreen.tsx (5)
15-23: Good use of typed imports for navigation.
These new imports from@suite-native/navigationenhance type safety and maintain consistency across files.
29-33: Navigation type aligns well with stack structure.
IntroducingAddCoinDiscoveryFinishedScreenNavigationPropskeeps the route parameters strongly typed and helps avoid runtime navigation errors.
37-42: Screen parameters strongly defined.
Wrapping the screen component with typedStackPropsensures therouteprop matches the expected structure forAddCoinDiscoveryFinishedScreen.
51-51: Ensure usage consistency ofhandleAccountTypeConfirmation.
This function replaces the older account confirmation logic. Double-check that all references are updated and tested.Would you like me to generate a script that scans for any remaining calls to deprecated handlers, so you can verify everything points to
handleAccountTypeConfirmation?
71-71: Simple direct call to handle confirmations.
This approach is more streamlined, but verify thatflowTypeis always defined to avoid unexpected behavior if navigated incorrectly.suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (9)
2-2: ReduxuseSelectorimport is consistent with the pattern.
This standard approach integrates component state with the global Redux store effectively.
4-4: Navigation import is appropriate.
UsinguseNavigationhelps decouple the component from higher-level navigators while maintaining typed route support.
9-14: Extended navigation types.
Bringing inRootStackParamList,TradingStackParamList, andTradingStackRoutesclarifies route transitions and ensures type checking across route boundaries.
17-17: Select function from Redux store.
selectBuySelectedReceiveAccountis well named. Confirm that the state slice is tested so the component always gets correct account data.
32-36: DedicatedNavigationPropsis clear.
Defining a specialized type for navigation props improves maintainability and avoids duplication in multiple components.
38-40: StreamlinedReceiveAccountPickerProps.
Removing unneeded dependencies and exposing onlyselectedSymbolmakes the component’s interface more focused.
87-90: Hook-based navigation and Redux integration.
This pattern is concise and consistent. Make sure that edge cases (e.g.,selectedSymbolisundefined) are handled upstream.
98-104: Fetch address text from account object.
The fallback toaccount.descriptorifaddressesare missing is a logical approach. Ensure that other parts of the UI handledescriptorgracefully if no addresses exist.
107-119: Enhanced layout withTradingOverviewRow.
This approach organizes the UI clearly, placing the account information in the right column. The refactor away from inline sheet rendering simplifies the component structure.
suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts
Outdated
Show resolved
Hide resolved
| const handleAccountTypeConfirmation = (flowType: AddCoinFlowType) => { | ||
| if (networkSymbolWithTypeToBeAdded) { | ||
| // Timeout is needed so AccountTypeDecisionBottomSheet has time to hide otherwise app crashes | ||
| setTimeout(() => { | ||
| addCoinAccount({ | ||
| symbol: networkSymbolWithTypeToBeAdded[0], | ||
| accountType: networkSymbolWithTypeToBeAdded[1], | ||
| flowType, | ||
| }); | ||
| }, 100); | ||
| clearNetworkWithTypeToBeAdded(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
question: I do not like this solution. Is 0.1s enough in all cases? Is it too much? Is there a property we can wait for?
Also WDYT about using async approach? Something like (this snippet does not address the question):
const handleAccountTypeConfirmation = async (flowType: AddCoinFlowType) => {
if (networkSymbolWithTypeToBeAdded) {
clearNetworkWithTypeToBeAdded();
// Timeout is needed so AccountTypeDecisionBottomSheet has time to hide otherwise app crashes
await new Promise(resolve => setTimeout(resolve, 100));
await addCoinAccount({
symbol: networkSymbolWithTypeToBeAdded[0],
accountType: networkSymbolWithTypeToBeAdded[1],
flowType,
});
}
};This way we can actually wait for handleAccountTypeConfirmation to finish. (However I am not sure we want to.)
Also clearNetworkWithTypeToBeAdded is first thing that addCoinAccount calls. Do we need to call it 2 times with 0.1s delay?
There was a problem hiding this comment.
I tested that clear and delay is definitely needed there to work.
suite-native/module-add-accounts/src/screens/AddCoinDiscoveryRunningScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-trading/src/hooks/__tests__/useReceiveAccountsListData.test.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| export const useSectionList = <T, U = undefined>({ |
There was a problem hiding this comment.
suggestion: You know when is the best time to add tests? When you want to touch the code. Would it be possible to add tests for these returned values?
- data (correct transformation, correct headers, correct number of sections)
- sectionsCount
- itemsCount
- estimatedListSize
- (bonus) keyExtractor
There was a problem hiding this comment.
As we agreed on slack I will add tests for this in following PR.
| <Screen | ||
| header={ | ||
| <ScreenHeader | ||
| content={translate('moduleTrading.tradingScreen.buyTitle')} |
There was a problem hiding this comment.
the title should say Pick account according to design file.
| header={ | ||
| <ScreenHeader | ||
| content={translate('moduleTrading.tradingScreen.buyTitle')} | ||
| closeActionType="back" |
There was a problem hiding this comment.
| closeActionType="back" | |
| closeActionType="close" |
| }; | ||
|
|
||
| const renderItem = (item: ReceiveAccount) => ( | ||
| <AccountListItem receiveAccount={item} onPress={() => onItemSelect(item)} symbol={symbol} /> |
There was a problem hiding this comment.
What about wrapping it in some TouchableOpacity or something like that? There is no on tap animation. Wdyt?
| addCoinAccount({ | ||
| symbol: networkSymbolWithTypeToBeAdded[0], | ||
| accountType: networkSymbolWithTypeToBeAdded[1], | ||
| flowType, | ||
| }); |
There was a problem hiding this comment.
nit: readability improvement. Feel free to close this.
| addCoinAccount({ | |
| symbol: networkSymbolWithTypeToBeAdded[0], | |
| accountType: networkSymbolWithTypeToBeAdded[1], | |
| flowType, | |
| }); | |
| const [symbol, accountType] = networkSymbolWithTypeToBeAdded; | |
| addCoinAccount({ | |
| symbol, | |
| accountType, | |
| flowType, | |
| }); |
| const title = isDeviceInViewOnlyMode ? ( | ||
| <Translation id="moduleTrading.accountSheet.accountEmpty.viewOnly.title" /> | ||
| ) : ( | ||
| <Translation id="moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title" /> | ||
| ); |
There was a problem hiding this comment.
nit
| const title = isDeviceInViewOnlyMode ? ( | |
| <Translation id="moduleTrading.accountSheet.accountEmpty.viewOnly.title" /> | |
| ) : ( | |
| <Translation id="moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title" /> | |
| ); | |
| const title = <Translation id={isDeviceInViewOnlyMode ? "moduleTrading.accountSheet.accountEmpty.viewOnly.title": "moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title"} /> |
| ) : ( | ||
| <Translation id="moduleTrading.accountSheet.accountEmpty.networkNotEnabled.title" /> | ||
| ); | ||
| const description = isDeviceInViewOnlyMode ? ( |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (1)
98-101: Use useMemo for address text computationThis computation for
addressTextshould be memoized withuseMemoto prevent unnecessary recalculations on re-renders.+ import { useCallback, useMemo } from 'react'; - import { ReactNode } from 'react'; + import { ReactNode } from 'react'; // ... export const ReceiveAccountPicker = ({ selectedSymbol }: ReceiveAccountPickerProps) => { // ... - const addressText = - (selectedReceiveAccount?.account.addresses - ? selectedReceiveAccount?.address?.address - : selectedReceiveAccount?.account.descriptor) ?? ''; + const addressText = useMemo(() => + (selectedReceiveAccount?.account.addresses + ? selectedReceiveAccount?.address?.address + : selectedReceiveAccount?.account.descriptor) ?? '', + [selectedReceiveAccount] + );suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts (1)
397-407: Consider refactoring the timeout approach inhandleAccountTypeConfirmation.While the timeout may be necessary to prevent UI crashes, this approach can be brittle as it depends on arbitrary timing that may vary across devices and conditions.
A more robust approach might be to leverage React Navigation's events or transition callbacks if available:
- const handleAccountTypeConfirmation = async (flowType: AddCoinFlowType) => { - if (networkSymbolWithTypeToBeAdded) { - clearNetworkWithTypeToBeAdded(); - await new Promise(resolve => setTimeout(resolve, 100)); - await addCoinAccount({ - symbol: networkSymbolWithTypeToBeAdded[0], - accountType: networkSymbolWithTypeToBeAdded[1], - flowType, - }); - } - }; + const handleAccountTypeConfirmation = (flowType: AddCoinFlowType) => { + if (networkSymbolWithTypeToBeAdded) { + const [symbol, accountType] = networkSymbolWithTypeToBeAdded; + clearNetworkWithTypeToBeAdded(); + + // Use navigation event listener or find another UI-based signal + // that the sheet has finished closing instead of arbitrary timeout + navigation.addListener('transitionEnd', () => { + addCoinAccount({ + symbol, + accountType, + flowType, + }); + }); + } + };Additionally, note that
clearNetworkWithTypeToBeAddedis also called at the beginning ofaddCoinAccount, so it's being called twice in this flow.suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx (1)
133-151: Enhance clarity by explaining the add-account CTA.A short descriptive note for users about adding a new account could make the UI more intuitive. Currently, the button label relies predominantly on the subtitle. A brief helper text might be a nice touch.
suite-native/module-trading/src/hooks/useSectionList.tsx (2)
21-25: Consider using a discriminated union instead of tuples for better type safety.The
ListInternalItemShapetype is defined as a tuple with string literals, which works but can be error-prone. Using a discriminated union would provide better type safety and code readability.-export type ListInternalItemShape<T, U> = - // [type, text, key] - | ['sectionHeader', ReactNode, string] - // [type, data, config] - | ['item', T, ItemRenderConfig<U>]; +export type ListInternalItemShape<T, U> = + | { type: 'sectionHeader'; text: ReactNode; key: string } + | { type: 'item'; data: T; config: ItemRenderConfig<U> };This would require updating the switch statements and other references to tuple indexes in the code.
107-136: Consider adding error boundaries around item rendering.While the render function is well-structured, an error in the user-provided
renderItemfunction could crash the entire list. Consider adding try/catch blocks to handle potential rendering errors gracefully.case 'item': return ( <AnimatedBox entering={FadeInUp} exiting={FadeOutUp} style={applyStyle(itemStyle, item[2])} > - {renderItem(item[1], item[2])} + {(() => { + try { + return renderItem(item[1], item[2]); + } catch (error) { + console.error('Error rendering item:', error); + return <Text color="textError">Error rendering item</Text>; + } + })()} </AnimatedBox> );suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts (1)
51-53: Add null check forselectedAccountto prevent potential errors.When
modeis 'address', there's a potential issue ifselectedAccountis undefined. Consider adding a more robust null check.-if (!selectedAccount?.addresses) { +if (!selectedAccount || !selectedAccount.addresses) { return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
suite-native/intl/src/en.ts(1 hunks)suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts(11 hunks)suite-native/module-add-accounts/src/screens/AddCoinDiscoveryRunningScreen.tsx(2 hunks)suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx(5 hunks)suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx(3 hunks)suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx(3 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/NoAccountsComponent.tsx(1 hunks)suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx(1 hunks)suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx(2 hunks)suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts(3 hunks)suite-native/module-trading/src/hooks/useSectionList.tsx(1 hunks)suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- suite-native/module-trading/src/components/general/AccountSheet/AddressListEmptyComponent.tsx
- suite-native/module-trading/src/components/general/TradeableAssetsSheet/TradeableAssetsSheet.tsx
- suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx
- suite-native/module-trading/src/components/general/AccountSheet/NoAccountsComponent.tsx
- suite-native/module-add-accounts/src/screens/AddCoinDiscoveryRunningScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: build
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (36)
suite-native/module-trading/src/components/buy/ReceiveAccountPicker.tsx (6)
2-4: Import structure and type definitions look goodThe changes in the imports section reflect the shift from a direct component approach to using Redux for state management and React Navigation for handling navigation. The navigation types are well-defined with appropriate generics.
Also applies to: 9-14, 32-36
17-17: Selective Redux selector looks goodClean implementation of the Redux selector for accessing the selected receive account from the global state.
38-40: Props simplification is a good improvementThe simplification of
ReceiveAccountPickerPropsby removing the dependency on the return type ofuseTradeSheetControlsmakes the component more focused and easier to understand.
87-91: Component initialization and state management looks goodClean implementation of component hooks and state selection. The component correctly uses
useSelectorto access the Redux store anduseNavigationwith proper typing.
92-96: MoveopenAccountPickeroutside of component or use useMemoFunctions defined inside a component will be recreated on each render, which can impact performance.
Consider one of these alternatives:
+ const openAccountPicker = ( + navigation: NavigationProps, + selectedSymbol?: NetworkSymbol + ) => + selectedSymbol && + navigation.navigate(TradingStackRoutes.ReceiveAccounts, { symbol: selectedSymbol }); export const ReceiveAccountPicker = ({ selectedSymbol }: ReceiveAccountPickerProps) => { const { translate } = useTranslate(); const navigation = useNavigation<NavigationProps>(); const selectedReceiveAccount = useSelector(selectBuySelectedReceiveAccount); - const openAccountPicker = () => - selectedSymbol && - navigation.navigate(TradingStackRoutes.ReceiveAccounts, { symbol: selectedSymbol }); + const handleAccountPicker = () => openAccountPicker(navigation, selectedSymbol); - const onPress = selectedSymbol ? openAccountPicker : undefined; + const onPress = selectedSymbol ? handleAccountPicker : undefined;Or use
useCallback:export const ReceiveAccountPicker = ({ selectedSymbol }: ReceiveAccountPickerProps) => { const { translate } = useTranslate(); const navigation = useNavigation<NavigationProps>(); const selectedReceiveAccount = useSelector(selectBuySelectedReceiveAccount); - const openAccountPicker = () => - selectedSymbol && - navigation.navigate(TradingStackRoutes.ReceiveAccounts, { symbol: selectedSymbol }); + const openAccountPicker = useCallback( + () => + selectedSymbol && + navigation.navigate(TradingStackRoutes.ReceiveAccounts, { symbol: selectedSymbol }), + [navigation, selectedSymbol] + ); const onPress = selectedSymbol ? openAccountPicker : undefined;
104-116: Component rendering structure looks goodThe updated component structure with
TradingOverviewRowand nested components improves readability and maintainability.suite-native/module-add-accounts/src/hooks/useAddCoinAccount.ts (8)
53-53: Good enhancement of type safety with the new type definition.Creating a specialized
AddCoinEnabledAccountTypethat explicitly excludes unsupported account types improves type safety and makes the code more self-documenting.
140-144: Improved type safety insetDefaultAccountToBeAdded.The explicit type casting and null check before setting state help prevent potential type errors and ensure we only set valid account types.
189-199: Proper handling for the new 'trade' flow type.The addition of the 'trade' case with specific logic for navigating back to the top of the stack completes the switch statement. Using
UnreachableCaseErrorfor the default case is excellent for type safety and will catch any future flow types that aren't explicitly handled.
203-245: Well-structured error handling with dedicated navigation logic.The new
navigateToFailureScreenfunction centralizes error handling navigation, making the code more maintainable. The function appropriately handles different flow types, displays relevant alerts based on error conditions, and resets the navigation stack in a clean way.
370-383: Conditional navigation behavior based on flow type.The separate handling for 'trade' flow using
navigateversusreplacefor other flows is a good approach. This prevents navigation stack issues when coming from different parts of the app.
350-350: Good use of the new error handling function.Replacing the inline error handling with a call to the centralized
navigateToFailureScreenfunction improves code maintainability and consistency.
277-289: Type annotation improvement inhandleAccountTypeSelection.Adding the explicit type annotation for the
flowTypeparameter enhances code readability and type safety. The nested navigation parameter structure is also more maintainable.
397-407: This solution warrants verification across devices.The timeout-based approach in
handleAccountTypeConfirmationmight behave differently across various devices with different processing capabilities. Please ensure this has been tested on both high-end and low-end devices to confirm the 100ms delay is sufficient in all cases.Can we also verify if this is the only solution, or if there's a property or event we can wait for instead of using an arbitrary timeout?
suite-native/module-trading/src/components/general/AccountSheet/AccountSheetHeader.tsx (1)
46-46: Translation key rename is consistent with the refactoring.Switching to
moduleTrading.accountScreen.titleStep1aligns with the newaccountScreencontext used throughout the codebase.suite-native/module-trading/src/components/general/AccountSheet/AccountListItem.tsx (3)
92-92: Use of the updated crypto balance translation key looks good.Changing the translation ID to
moduleTrading.accountScreen.balanceCryptois consistent with the shift fromaccountSheettoaccountScreen.
113-113: Correct transition to the new fiat balance translation key.This update to
moduleTrading.accountScreen.balanceFiataligns with the restructured translation keys.
124-124: Renamed accessibility hint is consistent with new context.Using
moduleTrading.accountScreen.step2Hintis appropriate under the newaccountScreenscope.suite-native/module-trading/src/screens/ReceiveAccountsPickerScreen.tsx (5)
65-77: Consider fallback logic for invalid or empty symbols.Although the route must supply a valid
symbol, it might be prudent to handle unexpected values or missing parameters gracefully to avoid potential runtime errors.
94-103: Validate device connection and address logic in onItemSelect.The selection flow is sound, but consider whether additional checks (e.g., ensuring the device is connected if needed) are required before switching the picker mode or popping the navigation stack.
153-159: Add separate testing to verify empty states.The conditional displaying of
AddressListEmptyComponentvs.NoAccountsComponentis clear, but consider unit tests or UI tests to ensure these states behave correctly, especially with different filter values.
161-165: Handle missing account label when pickerMode is 'address'.If
selectedReceiveAccountis somehow undefined, the screen title could become undefined. Providing a default fallback (e.g., "Address") might improve user experience in corner cases.
167-189: Overall screen structure successfully integrates the new list flow.Using
FlashListfor performance, a well-definedListEmptyComponent, and theAccountTypeDecisionBottomSheetis a coherent approach. Looks maintainable and consistent with the design pattern used elsewhere.suite-native/module-trading/src/hooks/useSectionList.tsx (4)
38-59: LGTM! Good responsive styling approach.The styling implementation uses a clean pattern with conditional styles based on item position. The use of
prepareNativeStylewithextendarray for conditional styling is a good practice.
61-89: Well-structured data transformation function.The
transformToInternalFlatListDatafunction effectively flattens the sectioned data while maintaining section information. The conditional section header rendering withnoSingletonSectionHeaderis a nice touch for flexibility.
91-105: Good error handling with UnreachableCaseError.The use of
UnreachableCaseErrorensures type safety at runtime and helps catch potential issues during development if new item types are added.
138-188: Good hook implementation with proper dependency tracking.The hook effectively uses
useMemowith appropriate dependencies for performance optimization. The clear return structure makes it easy to integrate with components.suite-native/module-trading/src/hooks/useReceiveAccountsListData.ts (5)
16-22: Good API design with clear type definitions.Introducing a dedicated type for the mode and a structured props interface makes the hook API more understandable and maintainable.
24-28: Improved function signature with object destructuring.The change from positional parameters to an object parameter with destructuring improves code readability and makes the hook more flexible for future additions.
35-49: LGTM! Clear mode-based conditional logic.The condition based on the
modeparameter is clear and provides good separation of concerns between account and address modes.
64-77: Updated translation keys are well-structured.The translation keys have been updated to use the new
accountScreennamespace, which aligns with the changes in the translation file.
78-78: Addedmodeto useMemo dependencies.Good addition of
modeto the dependency array ensures that the data is recalculated when the mode changes.suite-native/intl/src/en.ts (1)
1293-1310: Improved message structure with more specific contexts.The restructuring of messages under
accountScreenwith specific sub-objects for different scenarios (viewOnly,networkNotEnabled) provides better organization and more targeted messaging.suite-native/module-trading/src/components/general/TradingBottomSheetSectionList.tsx (3)
1-10: Good refactoring to use the new useSectionList hook.The imports have been updated to use the new
useSectionListhook, which centralizes list rendering logic and improves code organization.
31-50: Effective simplification through hook delegation.The component has been simplified by delegating the complex list transformation logic to the
useSectionListhook, improving maintainability.
52-62: Clean component structure with proper prop forwarding.The component uses the hook's outputs effectively and forwards additional props to
BottomSheetFlashList, making it flexible and reusable.
vytick
force-pushed
the
feat/enable-network-via-trade-account-selection
branch
from
d7b8437 to
3071dd2
Compare
|
|
||
| const handleAddAccountConfirmTap = () => handleAccountTypeConfirmation(flowType); | ||
|
|
||
| const handleSetPickerMode = (mode: ReceiveAccountsListMode) => setPickerMode(mode); |
There was a problem hiding this comment.
question: This is just setPickerMode with extra step, isn't it?
|
/rebase |
trezor-ci
force-pushed
the
feat/enable-network-via-trade-account-selection
branch
from
d10dd89 to
cccf840
Compare
Related Issue
Resolve #16990
Screenshots:
Simulator.Screen.Recording.-.iPhone.16.-.2025-02-26.at.23.03.31.mp4
Simulator.Screen.Recording.-.iPhone.16.-.2025-02-26.at.23.05.14.mp4