fix(suite-native): fix device onboarding feature flag evaluation #16941
Conversation
PeKne
added
mobile
WalkthroughThe changes involve modifications to the device connection hook and the selectors managing device state. In the hook, a new condition is added to one of the ✨ 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 (1)
suite-native/device/src/hooks/useHandleDeviceConnection.ts (1)
181-183: Consider refactoring the navigation logic.The TODO comment suggests that the navigation logic is becoming complex and hard to understand. Consider implementing a single source of truth for navigation state.
Consider extracting the navigation logic into a separate function that returns the target route based on the current state. This would make the logic more maintainable and easier to test. Example:
+const getTargetRoute = (state: { + isDeviceSetupSupported: boolean; + isDeviceInitialized: boolean; + isDeviceConnected: boolean; + // ... other state +}): Route => { + if (state.isDeviceSetupSupported && !state.isDeviceInitialized) { + return { stack: RootStackRoutes.OnboardingStack, screen: OnboardingStackRoutes.UninitializedDeviceLanding }; + } + // ... other conditions + return { stack: RootStackRoutes.AppTabs, screen: AppTabsRoutes.HomeStack }; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/device/src/hooks/useHandleDeviceConnection.ts(2 hunks)suite-native/device/src/selectors.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (4)
suite-native/device/src/selectors.ts (2)
1-1: LGTM! Type definition updated correctly.The addition of
FeatureFlagsRootStatetoNativeDeviceRootStatetype is necessary to support the new feature flag functionality.Also applies to: 50-51
202-212: Verify the impact of feature flag on device setup.The updated
selectIsDeviceSetupSupportedselector now includes a feature flag check that could prevent device setup even for supported models. This is a significant change in behavior.Let's verify the usage of this selector to ensure we understand the impact:
Additionally, let's verify that the feature flag is properly defined:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for usages of selectIsDeviceSetupSupported rg "selectIsDeviceSetupSupported" -A 3Length of output: 1865
🏁 Script executed:
#!/bin/bash # Search for the IsDeviceOnboardingEnabled feature flag definition rg "IsDeviceOnboardingEnabled.*=|enum.*IsDeviceOnboardingEnabled" -A 3Length of output: 72
Impact of Feature Flag on Device Setup Behavior
- The
selectIsDeviceSetupSupportedselector now returnsfalseif the feature flag (FeatureFlag.IsDeviceOnboardingEnabled) is disabled—even when the device model is valid and supported byisDeviceSetupSupported(model).- Usages in files like
useDetectDeviceError.tsxanduseHandleDeviceConnection.tsimply that this change directly affects application behavior regarding device setup.- The initial search for the feature flag’s definition produced no output. Please verify that
FeatureFlag.IsDeviceOnboardingEnabledis correctly defined (or imported) in the codebase or configuration to ensure the intended behavior.suite-native/device/src/hooks/useHandleDeviceConnection.ts (2)
168-169: LGTM! Dependency array updated correctly.The addition of
isDeviceSetupSupportedto the dependency array is necessary as it's used in the effect's condition.
136-138: Verify the navigation logic with the new condition.The addition of
!isDeviceSetupSupportedto the condition means that devices that support setup will skip the authorization flow. This is a significant change in the navigation logic.Let's verify the navigation paths to ensure we understand all scenarios:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for navigation to AuthorizeDeviceStack rg "navigate.*AuthorizeDeviceStack" -A 3 # Search for navigation to OnboardingStack rg "navigate.*OnboardingStack" -A 3Length of output: 10384
Navigation Logic Verification: The new condition behaves as intended.
- In
suite-native/device/src/hooks/useHandleDeviceConnection.ts, the condition now includes!isDeviceSetupSupported, which ensures that if a device supports setup, it will bypass the authorization flow.- The code navigates to
RootStackRoutes.AuthorizeDeviceStack(e.g., with theConnectingDevicescreen) only when both!shouldNavigateToDeviceCompromisedModaland!isDeviceSetupSupportedare true.- Conversely, when
isDeviceSetupSupportedis true, the branch that navigates toRootStackRoutes.OnboardingStack(with theUninitializedDeviceLandingscreen) is taken, confirming that devices with supported setup skip the authorization flow.- The rg search results across the repository corroborate these distinct navigation paths.
|
QA OK Info |
Description
IsDeviceOnboardingEnabledso the device onboarding flow is hidden for the production builds