fix(native-suite): device setting actions refactor #17689
Conversation
|
🚀 Expo preview is ready!
|
BrantalikP
force-pushed
the
fix/native/device-setting-actions-trigger
branch
from
a840f77 to
3b8d7fd
Compare
BrantalikP
force-pushed
the
fix/native/device-setting-actions-trigger
branch
from
3b8d7fd to
b48f961
Compare
WalkthroughThe changes refactor device authentication and pin management across several modules. In the device authenticity component, Redux state references and complex logic for checking and reporting authenticity have been removed and replaced with a simpler navigation function to the authenticity route. The device pin action component similarly discards detailed pin handling—including state management, analytics, and error routines—in favor of a straightforward navigation to the pin protection screen. Two new custom hooks have been introduced: one manages device authenticity actions by monitoring connection status and invoking authentication via an external service, while the other manages pin protection actions with defined configurations and feedback for success or failure. Additionally, device change detection now calls a cancellation method from an external library upon real device mismatch, and navigation parameters have been updated with a new type definition to enforce strict handling of pin action routes. ✨ 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: 0
🧹 Nitpick comments (2)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (1)
36-74: Consider handling device disconnection more explicitly.The
checkAuthenticityfunction correctly handles various error scenarios, but there could be edge cases when the device disconnects during the authentication process. The error handling at lines 64-67 mentions device disconnection in a comment but doesn't explicitly handle it differently from other interruptions.Consider enhancing error handling with more specific handling for device disconnection:
} else if (errorCode === 'Method_Interrupted' || errorCode === undefined) { // navigation.goBack() already called via the X button (or the device was disconnected) reportCheckResult('cancelled'); +} else if (errorCode === 'Device_Disconnected') { + reportCheckResult('deviceDisconnected'); + // Optional: Show specific message about device being disconnected } else {suite-native/module-device-settings/src/hooks/usePinAction.tsx (1)
93-130: Consider adding a timeout handling mechanism.The
handlePinActionfunction handles various error cases well, but it might benefit from a timeout mechanism for cases where the device doesn't respond in a timely manner.Consider implementing a timeout handler:
const handlePinAction = useCallback(async () => { analytics.report({ type: EventType.DeviceSettingsPinProtectionChange, payload: { action: type }, }); const { param, successMessageKey, canceledMessageKey } = actionConfigMap[type]; + // Create a timeout promise + const timeoutPromise = new Promise((_, reject) => { + const id = setTimeout(() => { + clearTimeout(id); + reject(new Error('Operation timed out')); + }, 60000); // 60-second timeout + }); const result = await requestPrioritizedDeviceAccess({ deviceCallback: () => TrezorConnect.changePin({ device: { path: device?.path, }, remove: param, }), }); dispatch(removeButtonRequests({ device })); // Error handling logic... }, [device, dispatch, navigation, showError, showSuccess, type]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx(3 hunks)suite-native/module-device-settings/src/components/DevicePinActionButton.tsx(2 hunks)suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts(1 hunks)suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts(2 hunks)suite-native/module-device-settings/src/hooks/usePinAction.tsx(1 hunks)suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx(1 hunks)suite-native/module-device-settings/src/navigation/DevicePinProtectionStackNavigator.tsx(2 hunks)suite-native/navigation/src/navigators.ts(2 hunks)
🔇 Additional comments (19)
suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts (2)
16-16: Good addition of TrezorConnect importThe import of TrezorConnect is necessary for the new functionality.
39-39: Good implementation of device change handlingAdding
TrezorConnect.cancel()ensures that pending operations are properly terminated when a device mismatch is detected, preventing unexpected behaviors.suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx (2)
9-9: Good addition of useDeviceAuthenticityAction hookThe import of the new hook aligns with the refactoring goal of managing device authentication.
17-17: Good implementation of the hookAdding
useDeviceAuthenticityAction()before the device connection check is appropriate as it centralizes the device authenticity logic in a dedicated hook.suite-native/module-device-settings/src/navigation/DevicePinProtectionStackNavigator.tsx (2)
14-14: Good addition of usePinAction hookThe import of the new hook aligns with the refactoring goal of managing pin protection actions.
25-25: Good implementation of the hookAdding
usePinAction()before checking device connection status helps centralize pin-related logic in a dedicated hook, improving code organization and maintainability.suite-native/navigation/src/navigators.ts (2)
151-152: Good addition of PinActionTypeCreating a dedicated type for pin actions improves type safety and makes the codebase more maintainable by clearly defining the allowed values.
174-176: Good implementation of type enforcementUpdating the device pin protection route parameters to require a specific action type improves type safety and prevents unintentional triggering of device setting actions.
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (2)
23-36: Well-implemented hook with good analytics integration.The hook is well-structured with a clean separation of concerns. The
reportCheckResultfunction is properly memoized withuseCallbackto prevent unnecessary re-renders.
76-80: Good use of useEffect for triggering the authentication flow.The effect hook appropriately triggers the authentication process when the device is connected, which aligns with the PR objective of preventing unintentional triggering of device setting actions. The disabled eslint rule is justified in this case.
suite-native/module-device-settings/src/hooks/usePinAction.tsx (3)
35-51: Well-structured action configuration mapping.The
actionConfigMapcleanly maps pin action types to their respective configurations. The use ofas const satisfiesensures type safety while providing strong typing. This approach makes it easy to maintain and extend the configuration in the future.
64-91: Well-implemented UI feedback functions.Both
showSuccessandshowErrorfunctions are properly memoized withuseCallbackand have clear, well-defined purposes. The error function provides good user options with retry functionality, improving the user experience when errors occur.
132-136: Good use of useEffect for device connection monitoring.The effect hook appropriately triggers the pin action when the device is connected, which aligns with the PR objective of preventing unintentional triggering of device setting actions.
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx (3)
6-6: Streamlined import statement.The removal of
selectSelectedDeviceand addition ofselectIsDeviceDiscoveryActivealigns with the PR objective of simplifying device setting actions. This change moves the device state management responsibility to the dedicated hook.
31-33: Simplified navigation function.The
navigateToDeviceAuthenticityStackfunction elegantly replaces the complexcheckAuthenticityfunction with a simple navigation action. This change aligns with the PR's goal to prevent unintentional triggering of device setting actions.
55-57: Updated callback dependencies.The
onPressPrimaryButtonnow uses the simplified navigation function, and the dependency array has been correctly updated to include it. This change improves the component's maintainability.suite-native/module-device-settings/src/components/DevicePinActionButton.tsx (3)
2-13: Streamlined imports and updated types.The import statements have been simplified to include only what's necessary, and
ActionTypehas been replaced withPinActionType. This change aligns with the PR objective and improves code clarity.
35-39: Simplified navigation function.The
navigateToPinStackfunction replaces the more complexchangePinfunction with a simple navigation action. This refactoring moves the pin handling logic to the dedicated hook and prevents unintentional triggering of device setting actions.
42-44: Updated button callback.The button's
onPressevent now uses the simplified navigation function, which aligns with the PR's goal of refactoring device setting actions to prevent unintentional triggering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (1)
36-74: Comprehensive authentication flow with proper error handlingThe checkAuthenticity function is well-structured with clear navigation paths for different authentication outcomes. Error handling is comprehensive, covering various failure scenarios.
One minor suggestion - consider adding a comment explaining what happens if
payload.error === 'CA_PUBKEY_NOT_FOUND' && !payload.configExpired, as it's not immediately obvious how this case is handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx(3 hunks)suite-native/module-device-settings/src/components/DevicePinActionButton.tsx(2 hunks)suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts(1 hunks)suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts(2 hunks)suite-native/module-device-settings/src/hooks/usePinAction.tsx(1 hunks)suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx(1 hunks)suite-native/module-device-settings/src/navigation/DevicePinProtectionStackNavigator.tsx(2 hunks)suite-native/navigation/src/navigators.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx
- suite-native/module-device-settings/src/navigation/DevicePinProtectionStackNavigator.tsx
- suite-native/module-device-settings/src/hooks/usePinAction.tsx
- suite-native/navigation/src/navigators.ts
- suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
🔇 Additional comments (14)
suite-native/module-device-settings/src/components/DevicePinActionButton.tsx (4)
2-2: Simplified imports align with the new refactored approach.The import statements have been updated to only include necessary dependencies. The transition from
ActionTypetoPinActionTypesuggests a more specialized type interface for PIN-related actions, which improves type safety and better represents the component's purpose.Also applies to: 6-6, 11-11
22-22: Updated prop type to match import changes.The type definition is now more specific and aligns with the navigation parameters, improving type safety and clarity of the component's API.
35-39: Simplified navigation logic enhances maintainability.The navigation logic has been significantly simplified by removing complex error handling, analytics, and device access request logic from this component. This refactor aligns with the PR objective to prevent unintentional triggering of device settings actions by moving the action execution logic outside this component and focusing solely on navigation.
The
useCallbackoptimization is correctly implemented with proper dependencies.
43-43: Updated button handler to use the new navigation function.The button now uses the simplified navigation function, ensuring consistency with the component's new approach. This change helps prevent unintentional triggering of device settings by introducing a separate confirmation screen.
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (5)
1-17: Good organization of importsThe imports are well-structured, separated by type (React, navigation, application modules), which enhances readability.
18-22: Good use of TypeScript for navigation typingStrong typing for navigation props ensures type safety throughout the navigation flow.
23-35: Well-structured hook initialization and analytics reportingThe hook properly initializes navigation and selectors from Redux. The analytics reporting function is correctly memoized with useCallback.
76-81: Verify dependency array in useEffectThe useEffect only includes
isDeviceConnectedin its dependency array but usescheckAuthenticityinside. While the ESLint rule is disabled, this could potentially lead to stale closure issues if the dependencies ofcheckAuthenticitychange.Consider either:
- Including
checkAuthenticityin the dependency array, or- Adding a comment explaining why it's intentionally excluded
1-81: Overall implementation aligns well with PR objectivesThis hook successfully refactors device authenticity checks to prevent unintentional triggering, as stated in the PR objectives. The implementation provides a clear, controlled flow for authentication that navigates appropriately based on the device state and authentication results.
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx (5)
6-6: Import dependencies have been simplified.The code now imports only the
selectIsDeviceDiscoveryActiveselector, removing the previousselectSelectedDeviceimport. This aligns with the refactoring goal of simplifying this component's responsibilities.
31-33: Simplified navigation approach approved.The new
navigateToDeviceAuthenticityStackfunction provides a cleaner, more direct approach to navigation than the previous implementation. This aligns with the PR objective of preventing unintentional triggering of device setting actions by moving the authentication logic out of this component.
55-55: Alert action now uses simplified navigation.The alert's primary button now triggers simple navigation instead of the previously complex authentication logic, which aligns with the refactoring objectives.
57-57: Dependency array properly updated.The dependency array has been correctly updated to include the new navigation function, ensuring the callback is properly memoized.
25-81:❓ Verification inconclusive
Verify error handling in the new architecture.
The component has been simplified by removing device authentication logic, analytics tracking, and error handling. Please confirm that these responsibilities are properly handled in the new
useDeviceAuthenticityActionhook mentioned in the summary.
🏁 Script executed:
#!/bin/bash # Search for the new hook implementation to verify error handling echo "Searching for useDeviceAuthenticityAction hook implementation..." rg -A 10 "export const useDeviceAuthenticityAction" --type typescriptLength of output: 245
Manual Verification Required: Confirm Error Handling is Implemented via useDeviceAuthenticityAction
The review comment noted that the component has been simplified by removing direct error handling, with the expectation that the new
useDeviceAuthenticityActionhook now covers this responsibility. The initial automated search using the file type "typescript" failed due to an unrecognized file type. To further verify the hook’s implementation, please re-run the following command which restricts the search to common TypeScript file extensions:Once the hook’s code is located, confirm that it handles error cases (for example, by catching exceptions or managing error states) to ensure the responsibilities removed from the component are properly migrated.
🏁 Script executed:
#!/bin/bash echo "Searching for useDeviceAuthenticityAction hook implementation in TypeScript files..." rg -A 10 "export const useDeviceAuthenticityAction" -g "*.{ts,tsx}"Length of output: 1690
Manual Verification Required: Confirm Error Handling in useDeviceAuthenticityAction
The search located the
useDeviceAuthenticityActionhook in the codebase, but we still need to verify that it implements proper error handling (e.g., using try/catch or handling error states) rather than simply delegating actions. Please run additional verification to ensure error handling is implemented as intended. For example, search the hook for try/catch blocks or error state management.Once confirmed, please update or comment accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for try/catch error handling in useDeviceAuthenticityAction..." rg -A 10 -B 10 "try\s*{" suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.tsLength of output: 458
Action Required: Verify Error Handling in useDeviceAuthenticityAction
- The new
useDeviceAuthenticityActionhook is implemented insuite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts.- An initial inspection (via regex-based searches) did not reveal explicit error handling constructs (e.g.,
try/catchblocks or.catchcallbacks) within the hook.- Please confirm that error handling responsibilities removed from the component are either managed within this hook (possibly via alternate mechanisms) or are addressed by a higher-level error boundary in the new architecture.
BrantalikP
force-pushed
the
fix/native/device-setting-actions-trigger
branch
from
b48f961 to
e06dbdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (4)
47-49: Consider handling the unsuccessful result case.When
result.successis false, the function returns early without notifying the user or reporting to analytics.if (!result.success) { + reportCheckResult('failed'); + navigation.goBack(); return; }
60-73: Consider using constants for error codes.Error codes like 'Failure_ActionCancelled' are hardcoded as string literals. Consider defining them as constants for better maintainability.
+ const ERROR_ACTION_CANCELLED = 'Failure_ActionCancelled'; + const ERROR_PIN_CANCELLED = 'Failure_PinCancelled'; + const ERROR_INTERRUPTED = 'Method_Interrupted'; const errorCode = payload.code; - if (errorCode === 'Failure_ActionCancelled' || errorCode === 'Failure_PinCancelled') { + if (errorCode === ERROR_ACTION_CANCELLED || errorCode === ERROR_PIN_CANCELLED) { navigation.goBack(); reportCheckResult('cancelled'); - } else if (errorCode === 'Method_Interrupted' || errorCode === undefined) { + } else if (errorCode === ERROR_INTERRUPTED || errorCode === undefined) {
76-81: Consider adding missing dependencies to the useEffect.The ESLint disable comment suggests that the dependencies array might be incomplete. The effect uses
checkAuthenticitywhich depends ondevice,navigation, andreportCheckResult.useEffect(() => { if (isDeviceConnected) void checkAuthenticity(); - // eslint-disable-next-line react-hooks/exhaustive-deps -}, [isDeviceConnected]); +}, [isDeviceConnected, checkAuthenticity]);If you only want the effect to run when device connection changes (and not on other dependency changes), consider extracting the authenticity check logic to a separate function within the effect.
36-74: Properly handle unhandled promise rejections from checkAuthenticity.The checkAuthenticity function is called with void operator in the useEffect, but there's no error handling for potential rejections.
const checkAuthenticity = useCallback(async () => { try { navigation.navigate(DeviceStackRoutes.DeviceAuthenticity); const result = await requestPrioritizedDeviceAccess({ deviceCallback: () => TrezorConnect.authenticateDevice({ device: { path: device?.path, }, }), }); // Rest of the function remains the same + } catch (error) { + console.error('Device authenticity check failed:', error); + reportCheckResult('failed'); + navigation.goBack(); + } }, [device, navigation, reportCheckResult]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx(3 hunks)suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts(1 hunks)suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts(2 hunks)suite-native/module-device-settings/src/hooks/usePinAction.tsx(1 hunks)suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx(1 hunks)suite-native/module-device-settings/src/screens/FirmwareInstallationScreen.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- suite-native/module-device-settings/src/navigation/DeviceAuthenticityStackNavigator.tsx
- suite-native/module-device-settings/src/hooks/useDeviceChangedCheck.ts
- suite-native/module-device-settings/src/hooks/usePinAction.tsx
🔇 Additional comments (11)
suite-native/module-device-settings/src/screens/FirmwareInstallationScreen.tsx (1)
20-22: Type assertion improves navigation type safety.The addition of the type assertion
as DeviceStackRoutes.DeviceSettingsenhances type safety by explicitly specifying the expected route type. This change aligns with the broader navigation refactoring in the PR and ensures consistent type handling across the navigation system without changing the actual behavior of the function.suite-native/module-device-settings/src/components/DeviceAuthenticityCard.tsx (5)
6-6: Import change reflects simplified component responsibility.The removal of
selectSelectedDeviceand addition of onlyselectIsDeviceDiscoveryActiveimport shows this component no longer directly handles device selection, aligning with the PR's goal of preventing unintentional device setting actions.
31-33: Simplified navigation improves user flow.The introduction of
navigateToDeviceAuthenticityStacksimplifies the component by replacing the previous complexcheckAuthenticityfunction with a straightforward navigation action. This enhances maintainability and helps prevent accidental triggering of authenticity checks.
55-55: Clean integration with alert system.Using the new navigation function as the primary button handler maintains the same user flow while removing direct authenticity checking logic from this component.
57-57: Updated dependency array ensures proper memoization.The dependency array correctly includes both the alert function and the navigation callback, ensuring the memoized function is updated when either dependency changes.
25-81: Successful separation of concerns in component architecture.The refactored component now follows a cleaner separation of concerns - it's solely responsible for displaying information and navigating to the appropriate route, while the actual device authenticity checking logic has been moved elsewhere (likely to the new
useDeviceAuthenticityActionhook mentioned in the summary).This architectural change aligns with the PR's goal of preventing unintentional device setting actions by ensuring that more complex operations happen in dedicated screens rather than being triggered directly from this settings card.
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (5)
1-17: Clean and comprehensive imports for device authenticity functionality.The imports cover all necessary dependencies including React hooks, Redux, navigation, device selectors, analytics, and device communication.
18-22: Well-typed navigation props for stack-to-stack navigation.The type definition properly specifies the composite navigation properties needed for navigating between stacks.
23-35: Efficient setup for device authenticity action hook.The hook correctly uses selectors to get device connection status and the selected device. The analytics reporting function is properly memoized with useCallback.
36-46: Clean implementation of device authenticity check initiation.The navigation to the Device Authenticity screen and subsequent device authentication via TrezorConnect is implemented effectively.
51-59: Good handling of successful authenticity check results.The code correctly processes different success scenarios, including valid devices and expired configurations.
suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts
Outdated
Show resolved
Hide resolved
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (1)
36-74: Consider adding error logging for debugging purposesWhile the error handling is comprehensive, adding detailed logging for unexpected errors would help with debugging in production. Currently, errors are handled for navigation but not logged.
} else { const errorCode = payload.code; if (errorCode === 'Failure_ActionCancelled' || errorCode === 'Failure_PinCancelled') { navigation.goBack(); reportCheckResult('cancelled'); } else if (errorCode === 'Method_Interrupted' || errorCode === 'Method_PermissionsNotGranted') { // navigation.goBack() already called via the X button (or the device was disconnected) reportCheckResult('cancelled'); } else { + console.error('Device authenticity check failed with error:', payload); navigation.navigate(DeviceAuthenticityStackRoutes.AuthenticitySummary, { checkResult: 'compromised', }); reportCheckResult('failed'); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts(1 hunks)suite-native/module-device-settings/src/hooks/usePinAction.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-device-settings/src/hooks/usePinAction.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (4)
23-82: Well-structured hook implementation with clear separation of concernsThe hook effectively manages device authenticity checks through clean separation of navigation, state management, device interaction, and analytics. The error handling is comprehensive, covering various edge cases like cancellation scenarios and configuration expiry.
77-77: Use ofvoidoperator with async functionThe
voidoperator is used here to explicitly indicate that there's no need to await this async function's completion. This addresses the ESLint warning about unhandled promises.
79-81: Comment explaining dependency exclusionGood practice to include this comment explaining why
checkAuthenticityis excluded from dependencies, as it prevents unintended effect triggers when the device object changes.
51-73: Comprehensive success/error handlingThe implementation thoroughly handles various success and error cases, with appropriate navigation and analytics reporting for each scenario. The special handling for expired configurations is a nice touch.
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 (2)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts(1 hunks)suite-native/module-device-settings/src/hooks/usePinAction.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-device-settings/src/hooks/usePinAction.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
suite-native/module-device-settings/src/hooks/useDeviceAuthenticityAction.ts (4)
1-35: Well-structured hook initialization with proper separation of concerns.The hook is initialized with clean imports and appropriate type definitions. Using
useCallbackfor thereportCheckResultfunction ensures it's not recreated on each render.
36-74: Comprehensive error handling with appropriate navigation flows.The
checkAuthenticityfunction handles different scenarios well:
- Proper device authentication flow
- Handling of success and failure cases
- Special case handling for expired configurations
- Various cancellation scenarios with appropriate navigation responses
This implementation aligns well with the PR objective of preventing unintentional triggering of device setting actions.
76-81: Properly documented dependency exclusion.The comment explaining why
checkAuthenticityis excluded from the dependencies is helpful and aligns with the discussion in the past review comments. This prevents unintentional triggering of the effect when the device object changes.
76-77: Verify the initial load behavior.When the component mounts and
isDeviceConnectedis already true,checkAuthenticitywill run immediately. Confirm this is the intended behavior as it will navigate to the device authenticity screen on component mount if a device is connected.
| device: { | ||
| path: device?.path, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling undefined device path.
While the optional chaining with device?.path handles null devices, what happens if the device object exists but has no path property? Consider adding a guard to prevent authenticating a device without a valid path.
🏁 Script executed:
#!/bin/bash
# Check if there are any instances where device authentication is attempted with a potentially undefined path
rg -A 5 "TrezorConnect\.authenticateDevice" --type tsLength of output: 3553
Action: Add an explicit guard to ensure the device object has a valid path before calling TrezorConnect.authenticateDevice.
The current code:
device: {
path: device?.path,
},relies on optional chaining, which only protects against a null or undefined device but not against an existing device lacking a path property. Notably, in suite-common/device-authenticity/src/deviceAuthenticityThunks.ts the call passes device.path directly—suggesting that a valid path is required. To maintain consistency and prevent potential issues, add a guard (e.g., an early return or error handling) that confirms device?.path is defined and valid before proceeding with device authentication.
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13987566009 |
trezor-ci
force-pushed
the
fix/native/device-setting-actions-trigger
branch
from
29f47ee to
f109226
Compare
|
android E2E is failing on the same thing as on develop https://github.com/trezor/trezor-suite/actions/runs/13981866909/job/39148822012 |
Description
Prevent triggering device setting actions unintentionally.
Related Issue
Resolve #17648
Screenshots:
Before
IMG_6529.MOV
IMG_6535.MOV
After
IMG_6534.MOV
IMG_6530.MOV