OK-34789: Device management #6667
Conversation
originalix
requested review from
ezailWang,
loatheb,
sidmorizon and
huhuanming
as code owners
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis pull request simplifies sidebar functionality and enhances device management features. It removes unnecessary elements from navigation components and introduces new hooks for displaying the address book. Multiple device management components and modal routes are added for showing device details, lists, and guides. Updates to shared routes, translations, and utility functions support this new functionality. Additionally, a new service API method is included to fetch hardware and QR wallets with their devices. Overall, the changes streamline UI components while integrating new device management capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant BackgroundAPI
participant Navigator
User->>UI: Click on Address Book button
UI->>BackgroundAPI: promptPasswordVerify()
BackgroundAPI-->>UI: Verification successful
UI->>Navigator: Push appropriate Address Book Modal
Navigator-->>UI: Render Address Book view
sequenceDiagram
participant User
participant DeviceMgmtBtn
participant Navigator
participant DeviceDetails
User->>DeviceMgmtBtn: Click Device Management
DeviceMgmtBtn->>Navigator: Navigate to DeviceManagementModal
Navigator-->>DeviceDetails: Load Device Details Modal
DeviceDetails->>DeviceDetails: Fetch and display device info
DeviceDetails-->>User: Show device management details
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (4)
packages/components/src/layouts/Navigation/Tab/TabBar/DesktopLeftSideBar.tsx (4)
46-53: 🧹 Nitpick (assertive)Optimize icon prefetching.
The icon prefetching logic runs on every render. Move it to a useEffect hook to run only when options change.
- useMemo(() => { + useEffect(() => { // @ts-expect-error const activeIcon = options?.tabBarIcon?.(true) as IKeyOfIcons; // @ts-expect-error const inActiveIcon = options?.tabBarIcon?.(false) as IKeyOfIcons; // Avoid icon jitter during lazy loading by prefetching icons. void Icon.prefetch(activeIcon, inActiveIcon); - }, [options]); + }, [options?.tabBarIcon]);
56-67: 🧹 Nitpick (assertive)Add missing ARIA labels for better accessibility.
The tab items need proper ARIA labels for screen readers.
<DesktopTabItem onPress={onPress} aria-current={isActive ? 'page' : undefined} + aria-label={`${options.tabBarLabel ?? route.name} tab`} selected={isActive} shortcutKey={options.shortcutKey} tabBarStyle={options.tabBarStyle} // @ts-expect-error icon={options?.tabBarIcon?.(isActive) as IKeyOfIcons} label={(options.tabBarLabel ?? route.name) as string} actionList={options.actionList} testID={route.name.toLowerCase()} />
31-73: 🧹 Nitpick (assertive)Fix TypeScript type assertions.
Replace
@ts-expect-errorwith proper type definitions for thetabBarIconoption.- // @ts-expect-error - const activeIcon = options?.tabBarIcon?.(true) as IKeyOfIcons; - // @ts-expect-error - const inActiveIcon = options?.tabBarIcon?.(false) as IKeyOfIcons; + const activeIcon = options?.tabBarIcon?.({ focused: true, color: '', size: 0 }) as IKeyOfIcons; + const inActiveIcon = options?.tabBarIcon?.({ focused: false, color: '', size: 0 }) as IKeyOfIcons;
104-157: 🧹 Nitpick (assertive)Optimize tab rendering performance.
Move the tab press handler outside the map function to prevent unnecessary recreations.
+ const createTabPressHandler = useCallback((route: NavigationState['routes'][0], focus: boolean) => () => { + const event = navigation.emit({ + type: 'tabPress', + target: route.key, + canPreventDefault: true, + }); + + if (!focus && !event.defaultPrevented) { + navigation.dispatch({ + ...CommonActions.navigate({ + name: route.name, + merge: true, + }), + target: state.key, + }); + } + }, [navigation, state.key]); const tabs = useMemo( () => routes.map((route, index) => { const focus = index === state.index; const { options } = descriptors[route.key]; - const onPress = () => { - const event = navigation.emit({ - type: 'tabPress', - target: route.key, - canPreventDefault: true, - }); - - if (!focus && !event.defaultPrevented) { - navigation.dispatch({ - ...CommonActions.navigate({ - name: route.name, - merge: true, - }), - target: state.key, - }); - } - }; + const onPress = createTabPressHandler(route, focus);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (20)
packages/kit/assets/device-management-guide.pngis excluded by!**/*.pngpackages/shared/src/locale/json/bn.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/de.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/en_US.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/es.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/fr_FR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/hi_IN.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/id.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/it_IT.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ja_JP.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ko_KR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/pt.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/pt_BR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ru.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/th_TH.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/uk_UA.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/vi.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_CN.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_HK.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_TW.jsonis excluded by!packages/shared/src/locale/json/*.json
📒 Files selected for processing (24)
packages/components/src/layouts/Navigation/Tab/TabBar/DesktopLeftSideBar.tsx(3 hunks)packages/kit-bg/src/services/ServiceAccount/ServiceAccount.ts(2 hunks)packages/kit/src/hooks/useShowAddressBook.ts(1 hunks)packages/kit/src/provider/Container/PortalBodyContainer/SideBanner.tsx(3 hunks)packages/kit/src/routes/Modal/router.tsx(2 hunks)packages/kit/src/views/AccountManagerStacks/pages/AccountSelectorStack/WalletDetails/WalletOptions/DeviceManagementButton.tsx(1 hunks)packages/kit/src/views/AccountManagerStacks/pages/AccountSelectorStack/WalletDetails/WalletOptions/index.tsx(3 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceAdvanceSection.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceBasicInfoSection.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceQrInfoSection.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceSpecsSection.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceGuideModal/index.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx(1 hunks)packages/kit/src/views/DeviceManagement/router/index.tsx(1 hunks)packages/kit/src/views/Onboarding/router/index.ts(2 hunks)packages/kit/src/views/Setting/pages/List/DefaultSection/index.tsx(2 hunks)packages/shared/src/locale/enum/translations.ts(2 hunks)packages/shared/src/routes/deviceManagement.ts(1 hunks)packages/shared/src/routes/index.ts(1 hunks)packages/shared/src/routes/modal.ts(3 hunks)packages/shared/src/routes/onboarding.ts(2 hunks)packages/shared/src/utils/deviceUtils.ts(2 hunks)packages/shared/types/account.ts(2 hunks)
🔇 Additional comments (30)
packages/kit/src/provider/Container/PortalBodyContainer/SideBanner.tsx (3)
4-34: Imports look fine.All added imports appear correct. No obvious issues or redundancies.
103-124: Hide the download button on non-web platforms.This logic is clear and concise. It matches the environment check. No issues here.
195-199: Structural change to SidebarBanner.
SidebarBannernow rendersBottomMenu. The approach is clean and modular.packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx (7)
1-34: Imports and route props.All imports look correct. The route prop typing is good. No issues.
82-91: Safe usage of navigation.These callback functions are well-structured. No major concerns.
93-105: Firmware verify flow.This is a good approach. The try/catch blocks are handled in
onPressAuthRequest.
107-113: Firmware update triggers.Clear call to open the changelog modal. Looks good.
150-180: Conditional rendering.
renderContentchecksisLoadingandisQrWallet. Good clarity. No issues.
182-203: Page layout and body.The structure is straightforward. Keep an eye on test coverage for these sections.
206-207: Default export.Clean export. No problems here.
packages/shared/src/routes/deviceManagement.ts (2)
1-5: New enum for device management routes.Enums are clear. The naming is good.
7-13: Param list typed well.Defines route parameters succinctly. No improvements needed.
packages/kit/src/views/DeviceManagement/router/index.tsx (1)
1-34: Well-structured router configuration!Great use of lazy loading to optimize performance. The code is clean and properly typed.
packages/shared/src/routes/index.ts (1)
39-39: Clean export addition!The new export maintains alphabetical order and follows the existing pattern.
packages/shared/types/account.ts (1)
50-53: Clean type definition!The
IHwQrWalletWithDevicetype is clear and properly handles optional device field.packages/kit/src/hooks/useShowAddressBook.ts (1)
26-37: LGTM! Good security practice with password verification.The implementation properly verifies the password before showing sensitive address book data.
packages/shared/src/routes/modal.ts (1)
62-62: LGTM!The new device management modal route is well-integrated and follows the established patterns.
Also applies to: 96-96
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceBasicInfoSection.tsx (1)
74-111: LGTM! Well-structured layoutThe layout structure using YStack and XStack is clean and maintainable.
packages/kit/src/views/Onboarding/router/index.ts (1)
79-81: LGTM! Clean route integrationThe DeviceManagementGuideModal is properly integrated into the routing system, following the established patterns.
Also applies to: 187-190
packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx (1)
45-53: LGTM! Proper event cleanupThe event listener cleanup in useEffect is well-implemented, preventing memory leaks.
packages/kit/src/views/Setting/pages/List/DefaultSection/index.tsx (1)
44-58: LGTM! Clean hook implementationThe refactoring to use useShowAddressBook simplifies the component and improves maintainability.
packages/components/src/layouts/Navigation/Tab/TabBar/DesktopLeftSideBar.tsx (4)
75-84: LGTM! Good platform-specific rendering.The OneKeyLogo component correctly handles platform-specific rendering with a clean conditional return.
1-30: LGTM! Clean and focused imports.The imports are well-organized and include only what's needed.
75-84: LGTM! Clean and focused component.The OneKeyLogo component is simple and follows best practices.
159-212: LGTM! Clean sidebar implementation.The simplified sidebar with animation support looks good. The removal of download and settings buttons aligns with the PR objectives.
packages/kit/src/routes/Modal/router.tsx (1)
167-170: LGTM! Clean route integration.The DeviceManagementModal route is properly integrated following the established pattern.
packages/shared/src/locale/enum/translations.ts (4)
125-126: LGTM! Clear and consistent backup enable keys.The keys follow the established naming pattern and are properly placed in alphabetical order.
131-132: LGTM! Well-structured Google backup sunsetting keys.The keys clearly indicate their purpose and include both the main message and its description.
138-139: LGTM! Well-organized iCloud backup sunsetting keys.The keys maintain consistency with the Google backup keys and follow the same pattern.
1610-1610: LGTM! Clear OneKey ID logout description key.The key follows the naming convention and is properly placed in the prime section.
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx
Outdated
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceQrInfoSection.tsx
Show resolved
Hide resolved
.../views/AccountManagerStacks/pages/AccountSelectorStack/WalletDetails/WalletOptions/index.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceBasicInfoSection.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx
Show resolved
Hide resolved
…repo into feat/device-management
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
packages/kit-bg/src/services/ServiceAccount/ServiceAccount.ts(2 hunks)packages/kit/src/views/DeviceManagement/hooks/useBuyOneKeyHeaderRightButton.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceGuideModal/index.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx(1 hunks)packages/kit/src/views/DeviceManagement/router/index.tsx(1 hunks)packages/kit/src/views/Onboarding/pages/ConnectHardwareWallet/ConnectYourDevice.tsx(2 hunks)packages/shared/src/routes/deviceManagement.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unittest (20.x)
- GitHub Check: lint (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/shared/src/routes/deviceManagement.ts (1)
1-15: Well-structured route definitions!The route definitions are type-safe and clearly organized. The parameter list properly maps undefined types for modals without params and includes the required walletId for DeviceDetailModal.
packages/kit/src/views/DeviceManagement/router/index.tsx (1)
6-23: Great use of lazy loading!The lazy loading implementation will improve initial load time and bundle size.
packages/kit/src/views/DeviceManagement/pages/DeviceGuideModal/index.tsx (2)
31-38: Add alt text translation for accessibility.The image alt text should be translated for international users.
<Image source={require('@onekeyhq/kit/assets/device-management-guide.png')} - alt="OneKey devices" + alt={intl.formatMessage({ + id: ETranslations.device_management_guide_alt, + })} w="100%" h={200} resizeMode="contain" />
58-62: Add loading state to connection button.The connection process might take time. Show loading state during navigation.
+const [isConnecting, setIsConnecting] = useState(false); <Button variant="primary" size="medium" - onPress={handleStartConnect} + onPress={async () => { + setIsConnecting(true); + await handleStartConnect(); + setIsConnecting(false); + }} + loading={isConnecting} > {intl.formatMessage({ id: ETranslations.global_start_connection, })} </Button>packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx (2)
1-33: Clean and well-organized imports!The imports are logically grouped and the hooks are properly used.
163-164: Move hardcoded URL to constants.The URL "https://bit.ly/3YsKilK" should be moved to a constants file.
packages/kit-bg/src/services/ServiceAccount/ServiceAccount.ts (1)
275-303: Add error handling and type safety.The method needs error handling and stronger type checking.
packages/kit/src/views/Onboarding/pages/ConnectHardwareWallet/ConnectYourDevice.tsx (1)
1185-1186: Clean hook implementation!The hook destructuring is well-organized and follows React best practices.
packages/kit/src/views/DeviceManagement/hooks/useBuyOneKeyHeaderRightButton.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceManagementListModal/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/kit/assets/device-management-guide.pngis excluded by!**/*.png
📒 Files selected for processing (3)
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceSpecsSection.tsx(1 hunks)packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx(1 hunks)packages/shared/src/utils/deviceUtils.ts(1 hunks)
🔇 Additional comments (5)
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx (4)
1-33: Overall implementation looks solid.
The structure follows good React patterns. No big issues in the setup and imports.
110-114: Expand the supported device types.
You only check'classic', 'mini', 'classic1s'. What abouttouchorpro? This might cause missed logic.
98-98: Remove debug logging in production.
Thatconsole.log(checked);may clutter real logs.
1-203: No major blockers found.
Everything else looks clean and follows consistent patterns.packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceSpecsSection.tsx (1)
1-76: Good data handling.
Graceful fallback to defaults prevents crashes if features are missing.
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/index.tsx
Show resolved
Hide resolved
packages/kit/src/views/DeviceManagement/pages/DeviceDetailsModal/DeviceSpecsSection.tsx
Show resolved
Hide resolved
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Summary by CodeRabbit
New Features
Refactor