feat(native-suite): handling discovery/disconnect state in Settings #17544
Conversation
WalkthroughThe pull request introduces a new dependency ( ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚀 Expo preview is ready!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/module-device-settings/src/hooks/useSettingsProtection.ts (1)
1-43: Good abstraction of device connection and discovery statesThis hook cleanly encapsulates the logic for checking device connection and discovery status, as well as handling device connection navigation. It appropriately uses Redux selectors to determine the states and provides a convenient interface for components to use.
One suggestion to consider - perhaps add a comment explaining the hook's purpose and when it should be used to make the code more maintainable for future developers.
+/** + * Hook that provides device connection status, discovery status, and connection handling + * for device settings screens that require an active device connection. + */ export const useSettingsProtection = () => {
📜 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 (6)
suite-native/module-device-settings/package.json(1 hunks)suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx(5 hunks)suite-native/module-device-settings/src/components/DeviceFirmwareCard.tsx(4 hunks)suite-native/module-device-settings/src/components/DevicePinActionButton.tsx(4 hunks)suite-native/module-device-settings/src/components/LoadingIndicator.tsx(1 hunks)suite-native/module-device-settings/src/hooks/useSettingsProtection.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- 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=passphrase, 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: EAS Update
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (19)
suite-native/module-device-settings/package.json (1)
37-37: LGTM: Added react-native-reanimated dependency for animation supportThe addition of this dependency is properly aligned with the implementation of the new animated
LoadingIndicatorcomponent in the module.suite-native/module-device-settings/src/components/LoadingIndicator.tsx (1)
1-11: LGTM: Clean implementation of animated loading indicatorThe component is well-structured with a clear single responsibility. It uses react-native-reanimated for the fade-in animation and displays a loader with appropriate styling for the disabled state.
suite-native/module-device-settings/src/components/DevicePinActionButton.tsx (6)
20-21: LGTM: Appropriate imports for new componentsThe imports for the LoadingIndicator and useSettingsProtection hook are correctly added.
65-65: LGTM: Integration of useSettingsProtection hookGood use of destructuring to get the needed values from the hook.
102-104: LGTM: Early return for disconnected deviceAppropriate early check for device connection status, redirecting to device connection flow when needed.
142-151: LGTM: Updated dependency arrayThe dependency array for the useCallback hook has been properly updated to include the new dependencies.
159-159: LGTM: Button disabled state during discoveryGood UX improvement to disable the button while discovery is running.
161-161: LGTM: Conditional rendering of LoadingIndicatorThe LoadingIndicator is correctly shown during discovery, providing clear visual feedback to the user.
suite-native/module-device-settings/src/components/DeviceFirmwareCard.tsx (5)
28-28: Clean hook addition for better state management.Adding the
useSettingsProtectionhook is a good abstraction that centralizes device connection and discovery state management.
68-68: Good use of custom hook for state management.The hook provides a clean way to access connection status and discovery state, removing the need for individual selectors and simplifying the component.
80-80: Simplified conditional logic.The condition has been refined to remove the direct dependency on device connection state. This allows the firmware update card to be visible even when a device isn't connected, improving discoverability of the feature.
101-101: Good implementation of device connection check.Instead of hiding the button, this approach allows the button to be visible but handles the connection flow when needed, improving the user experience.
104-105: Appropriate visual feedback during discovery.The button is correctly disabled and shows a loading state during discovery, providing users with clear feedback about the system state.
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx (6)
22-23: Good component and hook imports.The addition of the LoadingIndicator component and useSettingsProtection hook properly supports the new functionality for handling discovery and connection states.
33-33: Effective use of custom hook.Using the useSettingsProtection hook provides a consistent approach to handling device connection and discovery states across different components.
48-48: Early return for device connection handling.This implementation correctly checks if the device is connected before proceeding with the authenticity check, ensuring proper UX flow.
86-86: Updated dependency array.The dependency array has been properly updated to include the new dependencies (handleConnectDevice and isDeviceConnected), ensuring the useCallback works as expected.
126-126: Appropriate button state handling.The button is correctly disabled during discovery, preventing multiple requests or confusing user interactions.
128-133: Good conditional rendering for loading state.The component properly shows the LoadingIndicator during discovery, providing clear visual feedback to the user about the ongoing process.
Description
Enhancement of handling disconnect and discovery states in settings.
Related Issue
Resolve #16010
Screenshots:
Discovery State
Screenrecorder-2025-03-10-12-46-53-688.mp4
Disconnect State
Screenrecorder-2025-03-10-18-55-34-853.mp4