chore(suite): rework DeviceWarning component #16854
Conversation
WalkthroughThe pull request revises device management within the UI components. In the 🪧 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 (
|
mroz22
force-pushed
the
device-needs-attention-refactor
branch
from
0d18264 to
5a1a841
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite/src/views/suite/SwitchDevice/CardWithDevice.tsx(3 hunks)packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceItem.tsx(2 hunks)packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceWarning.tsx(0 hunks)suite-common/wallet-core/src/device/deviceThunks.ts(10 hunks)
💤 Files with no reviewable changes (1)
- packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceWarning.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
🔇 Additional comments (3)
packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceItem.tsx (1)
1-74: LGTM! Good separation of concerns.The changes improve code organization by moving device management logic to
CardWithDevice, allowingDeviceItemto focus solely on rendering device information.packages/suite/src/views/suite/SwitchDevice/CardWithDevice.tsx (1)
92-115: LGTM! Good UX improvements.The new banner component with interactive solutions improves user experience by:
- Displaying clear device status messages
- Providing immediate actions to resolve device issues
- Handling different device states appropriately
suite-common/wallet-core/src/device/deviceThunks.ts (1)
1-675: LGTM! Good standardization of selectors.The changes improve code consistency by standardizing the use of
selectSelectedDeviceacross all functions while maintaining existing functionality.
|
🚀 Expo preview is ready!
|
mroz22
force-pushed
the
device-needs-attention-refactor
branch
2 times, most recently
from
f2f4ec2 to
ceb62d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/views/suite/SwitchDevice/CardWithDevice.tsx (1)
100-115: Consider adding error handling for action failures.The banner implementation effectively displays device attention states and provides user interaction. However, consider adding error handling for cases where the solve issue actions fail.
Add error handling to catch and display action failures:
const onSolveIssueClick = () => { + try { const needsAcquire = device.type === 'unacquired' || deviceStatus === 'used-in-other-window' || deviceStatus === 'was-used-in-other-window'; if (needsAcquire) { - dispatch(acquireDevice(device)); + await dispatch(acquireDevice(device)); } else { - dispatch(selectDeviceThunk({ device })); - dispatch(redirectAfterWalletSelectedThunk()); + await dispatch(selectDeviceThunk({ device })); + await dispatch(redirectAfterWalletSelectedThunk()); onCancel?.(false); } + } catch (error) { + // Handle error appropriately (e.g., show error message in banner) + console.error('Failed to solve device issue:', error); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite/src/views/suite/SwitchDevice/CardWithDevice.tsx(3 hunks)packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceItem.tsx(2 hunks)packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceWarning.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceWarning.tsx
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (5)
packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceItem.tsx (2)
4-4: LGTM! Import cleanup aligns with component's simplified responsibilities.The removal of device management-related imports and the Redux dispatch hook aligns with moving this functionality to the
CardWithDevicecomponent.Also applies to: 8-8
40-40: LGTM! Filter condition simplified.The filter condition has been simplified while maintaining its core functionality of filtering instances with valid states.
packages/suite/src/views/suite/SwitchDevice/CardWithDevice.tsx (3)
7-8: LGTM! Imports added for new device management functionality.The new imports support the component's enhanced responsibilities for device management and UI feedback.
Also applies to: 11-13
69-71: LGTM! Enhanced device state management.The addition of device status message and lock state tracking improves the component's ability to handle device states effectively.
73-85: LGTM! Clear and effective issue resolution handling.The function properly handles both device acquisition and selection scenarios, with appropriate action dispatching based on the device's state.
|
note for future refactoring, I was able to get into this state on develop, after ejecting device |
Description
Related Issue
Resolve
Screenshots: