17301 welcome screen improvements #17573
Conversation
|
🚀 Expo preview is ready!
|
|
/rebase |
246fac1 to
7612068
Compare
WalkthroughThe changes introduce a new utility function, Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (6)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/views/settings/SettingsGeneral/EnableViewOnly.tsx (1)
1-2: Consider using the same utility function as Labeling.tsx.You're importing
getStatushere, but in Labeling.tsx you're usingisDevicePerceivedAsNew. For consistency and maintainability, consider using the same utility function across both files.-import { getStatus } from '@suite-common/suite-utils'; +import { isDevicePerceivedAsNew } from '@suite-common/suite-utils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/suite-data/files/translations/cs.json(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsGeneral/EnableViewOnly.tsx(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsGeneral/EnableViewOnly.tsx(2 hunks)packages/suite/src/views/settings/SettingsGeneral/Labeling.tsx(3 hunks)suite-common/suite-utils/src/device.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_android_e2e_tests
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
packages/suite-data/files/translations/cs.json (1)
1580-1580: New translation entry for view-only mode tooltip addedThe new entry
"TR_SETTINGS_DEVICE_VIEW_ONLY_DISABLED_TOOLTIP": "Nedostupné, pokud se zařízení teprve nastavuje."
clearly communicates that the view-only mode is unavailable during device setup. The phrasing is concise and consistent with similar tooltip messages in the file.suite-common/suite-utils/src/device.ts (1)
73-81: Well-designed utility function that encapsulates device status checksThe
isDevicePerceivedAsNewfunction is a well-structured addition that properly encapsulates the logic for determining if a device should be considered "new" based on its status. It handles null/undefined inputs appropriately and reuses the existinggetStatusfunction, following the coding patterns established in other utility functions within this file.This abstraction will help maintain consistency across the codebase by providing a single source of truth for this check, reducing duplication in components that need to determine if a device is in a new/setup state.
packages/suite/src/support/messages.ts (1)
5898-5901: New tooltip message for device setup state approved.This new translation entry provides a clear message for users when certain features are disabled during device initialization. The message is concise and follows the same pattern as other tooltip messages in the file.
packages/suite/src/views/settings/SettingsGeneral/Labeling.tsx (4)
1-1: Well-organized import of utility function.Good job adding the import for
isDevicePerceivedAsNew. This separation of concerns makes the code more maintainable by centralizing device status logic in a common utility.
45-50: Clean implementation of device status check.Good refactoring to use the utility function
isDevicePerceivedAsNewinstead of directly checking device status in the component. This change improves maintainability by centralizing the logic for determining if a device is new.The
isDisabledconditions are now more comprehensive and properly handle the case of a newly connected device.
52-62: Well-structured tooltip content function.The new
getTooltipContentfunction is a good addition that provides clear, contextual feedback to users based on the device state. The conditional logic is easy to follow and maintains good separation of concerns.
77-82: Improved tooltip behavior.The updated tooltip configuration with conditional activation and dynamic content improves the user experience by providing relevant guidance based on the current state of the device and discovery process.
packages/suite/src/views/settings/SettingsGeneral/EnableViewOnly.tsx (2)
40-48: Improved button state handling and feedback.The updates to the button state and tooltip content provide better user feedback based on device status. This is a good improvement to the user experience.
However, as mentioned above, consider using the
isDevicePerceivedAsNewutility function for consistency with other files.
27-28: 🛠️ Refactor suggestionUse shared utility for device status check.
Currently, you're manually checking for specific device statuses, while the Labeling.tsx file uses the
isDevicePerceivedAsNewutility function. For consistency and to avoid duplication of logic, consider using the same utility across both files.-const deviceStatus = device && getStatus(device); -const newlyConnectedDevice = deviceStatus === 'bootloader' || deviceStatus === 'initialize'; +const newlyConnectedDevice = isDevicePerceivedAsNew(device);Likely an incorrect or invalid review comment.
packages/suite/src/views/settings/SettingsGeneral/EnableViewOnly.tsx
Outdated
Show resolved
Hide resolved
…device is connected
7612068 to
0dde8fd
Compare
Description
Related Issue
Addresses: #17301
I addressed issues outlined in the ticket except these: I didn't address (1) and (2) and (5) in this PR: #17573
Screenshots: