[PM-27150] React to device changes on device screen unlock method #6103
Conversation
andrebispo5
requested review from
a team,
brian-livefront and
david-livefront
as code owners
andrebispo5
changed the title
|
Claude finished @andrebispo5's task —— View job Code Review CompleteOverall Assessment: APPROVE This PR successfully implements biometric authentication handling improvements for the Authenticator app. The implementation properly reacts to device biometric changes, follows MVVM architecture, and includes comprehensive test coverage. SummaryThe changes introduce reactive biometric state monitoring that:
Key Strengths✓ Architecture compliance: Follows MVVM patterns with proper state hoisting to ViewModels Minor ConsiderationsI've added inline comments with:
These are all minor observations and don't block approval. The current implementation is solid and production-ready. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6103 +/- ##
==========================================
+ Coverage 84.97% 85.22% +0.25%
==========================================
Files 723 723
Lines 52746 52901 +155
Branches 7649 7683 +34
==========================================
+ Hits 44822 45087 +265
+ Misses 5250 5126 -124
- Partials 2674 2688 +14 ☔ 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.
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.
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.
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
}
}
}…iometric support.
| ), | ||
| AuthDiskSource { | ||
| private val mutableUserBiometricUnlockKeyFlow = | ||
| bufferedMutableSharedFlow<String?>() |
There was a problem hiding this comment.
Shouldn't this have a relay of 1?
| override fun getUserBiometricUnlockKey(): String? = | ||
| getEncryptedString(key = BIOMETRICS_UNLOCK_KEY) | ||
|
|
||
| override fun getUserBiometricUnlockKeyFlow(): Flow<String?> = mutableUserBiometricUnlockKeyFlow |
There was a problem hiding this comment.
Can we make this a val with a get() here?
| get() = | ||
| authDiskSource | ||
| .getUserBiometricUnlockKeyFlow() | ||
| .map { it != null } |
There was a problem hiding this comment.
Can you run the foirmatter here, this looks overly indented. You can also move the authDiskSource up a line
|
|
||
| private var lastActiveTimeMillis: Long? = null | ||
| private var userBiometricUnlockKey: String? = null | ||
| private val userBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>() |
There was a problem hiding this comment.
Same thing here, shouldn't this have a replay of 1?
| ) { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| isUnlockWithBiometricsEnabled = action.isEnabled, |
| BiometricChanges( | ||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = { | ||
| viewModel.trySendAction(RootNavAction.Internal.BiometricSupportChanged(it)) |
There was a problem hiding this comment.
This is an internal event but you are sending it externally?
There was a problem hiding this comment.
Nice catch, I was moving things around and didn't noticed. ty
| } | ||
|
|
||
| is RootNavAction.Internal.BiometricSupportChanged -> { | ||
| handleBiometricSupportChanged(action.isBiometricsSupported) |
There was a problem hiding this comment.
You should be passing the entire action through
# Conflicts: # authenticator/src/main/kotlin/com/bitwarden/authenticator/ui/platform/feature/rootnav/RootNavScreen.kt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Indicates an update on device biometrics support. | ||
| */ | ||
| data class BiometricSupportChanged(val isBiometricsSupported: Boolean) : Internal() |
There was a problem hiding this comment.
Details
The BiometricSupportChanged action is currently nested under Internal but is sent externally from RootNavScreen:
// In RootNavScreen.kt:72
viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it))Issue: Actions marked as Internal should only be sent by the ViewModel itself, not from external UI components.
Recommendation: Move BiometricSupportChanged outside the Internal sealed class to be a direct subclass of RootNavAction, similar to BackStackUpdate:
sealed class RootNavAction {
data object BackStackUpdate : RootNavAction()
data class BiometricSupportChanged(val isBiometricsSupported: Boolean) : RootNavAction()
sealed class Internal : RootNavAction() {
// ... existing internal actions
}
}This follows the established pattern where external UI events are root-level actions while async/internal events are nested under Internal.
| sharedPreferences = sharedPreferences, | ||
| ), | ||
| AuthDiskSource { | ||
| private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1) |
There was a problem hiding this comment.
🎨 Finding 2: Consider adding replay buffer to MutableSharedFlow
Details
The mutableUserBiometricUnlockKeyFlow uses replay = 1 which ensures new collectors receive the most recent value. However, this should be verified against the app's password manager module for consistency.
Looking at the password manager module in app/src/main/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt, similar flows use replay = 1 for state that should be immediately available to new subscribers.
Current implementation is correct - the replay = 1 parameter ensures that:
- New subscribers immediately receive the current biometric key state
- The flow behaves consistently with other state flows in the codebase
- The
isUnlockWithBiometricsEnabledFlowproperly reflects the current state
This is the appropriate pattern for this use case.
| .onEach(::sendAction) | ||
| .launchIn(viewModelScope) | ||
| settingsRepository | ||
| .isUnlockWithBiometricsEnabledFlow |
There was a problem hiding this comment.
♻️ Finding 3: New Flow observation creates potential for state inconsistency
Details
The ViewModel now observes isUnlockWithBiometricsEnabledFlow to update state, but this creates a dual path for state updates:
- Direct path: User toggles biometrics →
handleBiometricsSetupClick→ updates state directly - Flow path: Disk source changes → Flow emits →
handleUnlockWithBiometricsUpdated→ updates state
Concern: When RootNavViewModel clears the biometric key (when biometrics become unsupported), the flow will emit and update SettingsViewModel's state. However, this happens asynchronously and could race with user interactions.
Current implementation appears safe because:
- The biometric toggle in settings already checks
biometricsManager.isBiometricsSupportedbefore showing (SettingsScreen.kt:302) - The repository clears the key immediately, and the flow propagates the change
- State updates are synchronized through the action queue
However, this architectural pattern is more complex than necessary. Consider whether the Settings screen needs this flow observation or if it could rely on the repository's synchronous isUnlockWithBiometricsEnabled property.
| BiometricChanges( | ||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = { | ||
| viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🎨 Add remember(viewModel) for callback stability
Details
Per david-livefront's comment: "Can you make sure to remember the viewModel here"
Current code:
onBiometricSupportChange = {
viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it))
},Recommended:
onBiometricSupportChange = remember(viewModel) {
{ viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) }
},Why this matters:
- While the lambda is already stable (captures
viewModelfromhiltViewModel()), explicitremembermakes the intent clearer - Follows Compose best practices for callback stability
- Prevents potential unnecessary recompositions of
BiometricChanges - Consistent with pattern used elsewhere in the file (lines 166-172)
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = { | ||
| viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) | ||
| }, |
There was a problem hiding this comment.
Details
Per david-livefront's comment, the callback should be wrapped in remember(viewModel) for stability:
onBiometricSupportChange = remember(viewModel) {
{ viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) }
},This prevents unnecessary recompositions of BiometricChanges and follows the pattern established elsewhere in this file (lines 166-172).
| showSyncWithBitwarden = shouldShowSyncWithBitwarden, | ||
| showDefaultSaveOptionRow = shouldShowDefaultSaveOption, | ||
| allowScreenCapture = isScreenCaptureAllowed, | ||
| hasBiometricsSupport = true, |
There was a problem hiding this comment.
💭 Finding 2: Hardcoded initialization may not reflect actual device capability
Details
Initializing hasBiometricsSupport = true assumes all devices support biometrics. While BiometricChanges updates this on first app resume, there's a brief window where the UI may incorrectly show the biometric toggle.
Current behavior:
- First launch: Toggle appears (even on devices without biometrics)
- After resume: Toggle correctly hidden if unsupported
Considerations:
- This approach optimizes for the common case (devices with biometrics)
- The delay is minimal (only until first
ON_RESUME) - Alternative (init to
false) would hide the toggle even on supported devices until first resume
Question: Is this intentional, or should we initialize to false to be more conservative?
| biometricsManager: BiometricsManager, | ||
| onBiometricSupportChange: (isSupported: Boolean) -> Unit, | ||
| ) { | ||
| LaunchedEffect(biometricsManager.isBiometricsSupported) { |
There was a problem hiding this comment.
👍 Well-structured implementation
Details
The combination of LaunchedEffect + LifecycleEventEffect provides comprehensive biometric change detection:
- LaunchedEffect: Detects changes during recomposition (when
isBiometricsSupportedchanges) - LifecycleEventEffect: Detects changes when app returns from background
This dual approach, as suggested by david-livefront, ensures biometric support status is checked in all relevant scenarios.
| } | ||
| } | ||
|
|
||
| private fun handleBiometricSupportChanged( |
There was a problem hiding this comment.
🎨 Good navigation logic centralization
Details
The handleBiometricSupportChanged function correctly:
✓ Clears the biometric key when biometrics become unsupported
✓ Navigates from Locked → Unlocked state when currently locked
✓ Centralizes navigation decisions in the ViewModel per MVVM architecture
This ensures the app gracefully handles biometric removal while maintaining a single source of truth for navigation state.
Note: Test coverage for this function was added in RootNavViewModelTest.kt, addressing the codecov gap.
| showSyncWithBitwarden = shouldShowSyncWithBitwarden, | ||
| showDefaultSaveOptionRow = shouldShowDefaultSaveOption, | ||
| allowScreenCapture = isScreenCaptureAllowed, | ||
| hasBiometricsSupport = true, |
There was a problem hiding this comment.
💭 Finding 1: Hardcoded biometric support initialization
Details
The hasBiometricsSupport field is hardcoded to true during initialization:
hasBiometricsSupport = true,Impact: On devices without biometric hardware, the toggle will briefly appear until BiometricChanges triggers on first ON_RESUME.
Consideration: This optimizes for the common case (devices with biometrics) but creates a brief UX inconsistency on devices without biometric support.
Alternative approaches:
- Initialize to
false(more conservative, but hides toggle even on supported devices until first resume) - Accept current behavior as a reasonable tradeoff given that most devices support biometrics
Question: Is this intentional behavior? If so, consider adding a comment explaining the tradeoff.
| biometricsManager: BiometricsManager, | ||
| onBiometricSupportChange: (isSupported: Boolean) -> Unit, | ||
| ) { | ||
| LaunchedEffect(biometricsManager.isBiometricsSupported) { |
There was a problem hiding this comment.
👍 Finding 2: Comprehensive biometric monitoring implementation
Details
The combination of LaunchedEffect + LifecycleEventEffect provides thorough biometric change detection:
- LaunchedEffect: Detects changes during recomposition when
isBiometricsSupportedchanges - LifecycleEventEffect: Detects changes when app returns from background
This dual approach ensures biometric support status is checked in all relevant scenarios, addressing david-livefront's suggestion from the review.
Well-structured aspects:
✓ Comprehensive KDoc documentation
✓ Clear separation of concerns
✓ Proper use of lifecycle-aware effects
✓ Reusable composable design
| } | ||
| } | ||
|
|
||
| private fun handleBiometricSupportChanged( |
There was a problem hiding this comment.
👍 Finding 3: Proper navigation logic centralization
Details
The handleBiometricSupportChanged function correctly implements MVVM architecture:
✓ Clears the biometric key when biometrics become unsupported
✓ Navigates from Locked → Unlocked state when currently locked
✓ Centralizes navigation decisions in the ViewModel
✓ Has comprehensive test coverage in RootNavViewModelTest.kt
This ensures the app gracefully handles biometric removal while maintaining a single source of truth for navigation state, which is exactly what was requested in earlier review feedback.
Navigation flow when biometrics are disabled:
BiometricChangesdetects change onON_RESUMERootNavViewModelreceivesBiometricSupportChanged(false)- Biometric key cleared via
settingsRepository.clearBiometricsKey() - If locked, immediately navigates to unlocked state
- Flow propagates to
SettingsViewModelto update UI
|
|
||
| BiometricChanges( | ||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = remember(viewModel) { |
There was a problem hiding this comment.
👍 Finding 4: Proper callback memoization
Details
The callback is correctly wrapped with remember(viewModel) for stability:
onBiometricSupportChange = remember(viewModel) {
{ viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) }
},This prevents unnecessary recompositions of BiometricChanges and follows Compose best practices, addressing david-livefront's feedback from the review.
| .onEach(::sendAction) | ||
| .launchIn(viewModelScope) | ||
| settingsRepository | ||
| .isUnlockWithBiometricsEnabledFlow |
There was a problem hiding this comment.
👍 Finding 5: Appropriate flow observation pattern
Details
The ViewModel observes isUnlockWithBiometricsEnabledFlow to reactively update state when the biometric key is cleared externally. This creates a proper reactive architecture:
Update paths:
- Direct path: User toggles →
handleBiometricsSetupClick→ updates state - Flow path: External clear (from
RootNavViewModel) → Disk source emits → Flow observer → updates state
Why this is correct:
- The biometric key can be cleared from multiple locations (
RootNavViewModelwhen biometrics become unsupported,SettingsViewModelwhen user disables) - Flow observation ensures Settings UI stays in sync with external changes
- Maintains consistency across the app without tight coupling
This pattern follows the repository pattern correctly and ensures single source of truth from the data layer.
| sharedPreferences = sharedPreferences, | ||
| ), | ||
| AuthDiskSource { | ||
| private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1) |
There was a problem hiding this comment.
👍 Finding 6: Correct use of replay buffer
Details
The mutableUserBiometricUnlockKeyFlow correctly uses replay = 1:
private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1)Why this is correct:
- State availability: New subscribers immediately receive the most recent biometric key value
- Consistent with app module: Matches the pattern in the password manager's
AuthDiskSourceImpl - Proper state semantics: The biometric key is stateful data that should be immediately available to observers
Flow behavior:
onSubscriptionemits current value from diskreplay = 1ensures late subscribers get the most recent emissiontryEmitinstoreUserBiometricUnlockKeybroadcasts changes to all active collectors
This implementation correctly maintains a reactive, state-like flow for biometric key changes.
| navOptions = expectedNavOptions, | ||
| ) | ||
| } | ||
| assert(!onSplashScreenRemovedCalled) |
There was a problem hiding this comment.
These should use assertTrue or assertFalse we never use the regular assert
| onBiometricSupportChange: (isSupported: Boolean) -> Unit, | ||
| ) { | ||
| LaunchedEffect(biometricsManager.isBiometricsSupported) { | ||
| onBiometricSupportChange(biometricsManager.isBiometricsSupported) |
There was a problem hiding this comment.
💭 LaunchedEffect may cause unnecessary emissions
Details
The LaunchedEffect(biometricsManager.isBiometricsSupported) will re-trigger whenever isBiometricsSupported changes. However, BiometricsManager.isBiometricsSupported is typically a property that reads system state synchronously, not a state variable that changes.
Current behavior:
LaunchedEffectfires on initial compositionLifecycleEventEffectfires on ON_RESUME- Both may fire the callback with the same value
Question: Does isBiometricsSupported change during recomposition without a lifecycle event? If not, the LaunchedEffect may be redundant since LifecycleEventEffect on ON_RESUME already handles the primary use case (detecting changes when app returns from Settings).
If keeping both: Consider whether the initial LaunchedEffect emission on first composition is necessary, or if you want to wait for the first ON_RESUME to check biometric support.
| showSyncWithBitwarden = shouldShowSyncWithBitwarden, | ||
| showDefaultSaveOptionRow = shouldShowDefaultSaveOption, | ||
| allowScreenCapture = isScreenCaptureAllowed, | ||
| hasBiometricsSupport = true, |
There was a problem hiding this comment.
💭 Consider the initialization strategy for hasBiometricsSupport
Details
The field is hardcoded to true during initialization:
hasBiometricsSupport = true,Current behavior:
- On first launch, biometric toggle shows even on devices without biometric hardware
BiometricChangesupdates this to correct value on first ON_RESUME- Brief window where UI may incorrectly show the toggle
Trade-offs:
-
Current approach (init to
true): Optimizes for common case (devices with biometrics), but creates brief UX inconsistency on devices without biometric support -
Alternative (init to
false): More conservative, hides toggle initially even on supported devices until first resume validates support
Recommendation: Consider adding a comment explaining this design choice, or evaluate whether initializing to false would be a better default to avoid showing UI that shouldn't be there.
// Initialize to true as most devices support biometrics.
// BiometricChanges will update to false on first resume if unsupported.
hasBiometricsSupport = true,
| action: RootNavAction.BiometricSupportChanged, | ||
| ) { | ||
| if (!action.isBiometricsSupported) { | ||
| settingsRepository.clearBiometricsKey() |
There was a problem hiding this comment.
🎨 Consider extracting navigation state check to improve readability
Details
The current implementation checks the navigation state inline:
if (mutableStateFlow.value.navState is RootNavState.NavState.Locked) {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Unlocked) }
}Minor improvement: Extract to a local variable for clarity:
private fun handleBiometricSupportChanged(
action: RootNavAction.BiometricSupportChanged,
) {
if (!action.isBiometricsSupported) {
settingsRepository.clearBiometricsKey()
// If currently locked, navigate to unlocked since biometrics are no longer available
val isCurrentlyLocked = mutableStateFlow.value.navState is RootNavState.NavState.Locked
if (isCurrentlyLocked) {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Unlocked) }
}
}
}This makes the intent clearer and slightly improves readability, though the current implementation is acceptable.
| sharedPreferences = sharedPreferences, | ||
| ), | ||
| AuthDiskSource { | ||
| private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1) |
There was a problem hiding this comment.
👍 Correct use of replay buffer
Details
The replay = 1 parameter is correctly used here:
private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1)Why this is correct:
- New subscribers immediately receive the most recent biometric key value
- Matches the pattern in the password manager's
AuthDiskSourceImpl - Proper state semantics for stateful data that should be immediately available
The onSubscription emission combined with replay = 1 ensures:
- New collectors get the current value from disk immediately
- Late subscribers also receive the most recent emission
- Changes propagate to all active collectors via
tryEmit
This correctly maintains a reactive, state-like flow for biometric key changes.
|
|
||
| BiometricChanges( | ||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = remember(viewModel) { |
There was a problem hiding this comment.
👍 Proper callback memoization
Details
The callback is correctly wrapped with remember(viewModel):
onBiometricSupportChange = remember(viewModel) {
{
viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it))
}
},This prevents unnecessary recompositions of BiometricChanges and follows Compose best practices for callback stability. The remember(viewModel) key ensures the lambda is only recreated if the ViewModel instance changes, which shouldn't happen during the lifecycle of this screen.
| } | ||
|
|
||
| @Test | ||
| fun `on BiometricSupportChanged with false should clear biometrics key`() { |
There was a problem hiding this comment.
👍 Comprehensive test coverage for biometric changes
Details
Excellent test coverage for the BiometricSupportChanged action:
✓ Tests clearing key when biometrics become unsupported
✓ Tests NOT clearing key when biometrics remain supported
✓ Tests navigation from Locked → Unlocked when biometrics disabled
✓ Tests no navigation change when already Unlocked
These tests properly validate all code paths in handleBiometricSupportChanged and cover both the repository interaction and the navigation state logic. This addresses the test coverage gaps that were identified in earlier reviews.
|
|
||
| BiometricChanges( | ||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = remember(viewModel) { |
There was a problem hiding this comment.
👍 Clean integration of BiometricChanges composable
Details
The BiometricChanges composable is properly integrated into the Settings screen:
✓ Uses remember(viewModel) for callback stability
✓ Sends actions to ViewModel following MVVM architecture
✓ Placed at the top level of the screen composable
This ensures biometric support changes are monitored whenever the Settings screen is active, and the ViewModel maintains the single source of truth for the biometric support state.
The same pattern is correctly used in RootNavScreen, showing good architectural consistency across the codebase.
🎟️ 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