-
Notifications
You must be signed in to change notification settings - Fork 901
[PM-27150] React to device changes on device screen unlock method #6103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6103 +/- ##
==========================================
- Coverage 84.81% 84.30% -0.51%
==========================================
Files 721 837 +116
Lines 52811 55159 +2348
Branches 7668 7717 +49
==========================================
+ Hits 44791 46502 +1711
- Misses 5328 5953 +625
- Partials 2692 2704 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
| navController.navigateToTutorial(rootNavOptions) | ||
| } | ||
|
|
||
| RootNavState.NavState.Locked -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be happening at this level. Can we move this logic into the UnlockScreen?
| private fun handleClearBiometricsKey() { | ||
| settingsRepository.clearBiometricsKey() | ||
| mutableStateFlow.update { | ||
| it.copy(navState = RootNavState.NavState.Unlocked) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be needed. Just observe the isUnlockWithBiometricsEnabled state.
|
|
||
| // Recheck biometrics support when app comes to foreground | ||
| LifecycleEventEffect { _, event -> | ||
| if (event == Lifecycle.Event.ON_RESUME) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a composable that is called from the top level of the screen.
Additionally, can we send the state to the VM to handle this logic.
@Composable
private fun BiometricChanges(
onBiometricSupportChange: (isSupported: Boolean) -> Unit,
) {
LifecycleEventEffect { _, event ->
when (event) {
Lifecycle.Event.ON_RESUME -> {
onBiometricSupportChange(biometricsManager.isBiometricsSupported)
}
else -> Unit
}
}
}
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27150
📔 Objective
This pull request improves how the Authenticator app handles biometric authentication, especially in scenarios where the device's biometric setup changes (e.g., biometrics are disabled or removed). The main focus is on ensuring that the app responds appropriately if biometrics are no longer available, both in navigation logic and in the settings UI.
Biometric Authentication Handling Improvements:
ClearBiometricsKeyaction toRootNavAction.Internal, with corresponding handling inRootNavViewModelto clear the key and update navigation state.Settings Screen Enhancements:
Dependency Injection and Imports:
BiometricsManageris now injected intoRootNavScreenusingLocalBiometricsManager, and necessary imports have been added to support these changes.📸 Screenshots
Screen.Recording.2025-10-31.at.17.15.20.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes