[PM-27150] React to device changes on device screen unlock method #6103
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package com.bitwarden.authenticator.ui.platform.components.biometrics | ||
|
|
||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.LaunchedEffect | ||
| import androidx.lifecycle.Lifecycle | ||
| import com.bitwarden.authenticator.ui.platform.manager.biometrics.BiometricsManager | ||
| import com.bitwarden.ui.platform.base.util.LifecycleEventEffect | ||
|
|
||
| /** | ||
| * Tracks changes in biometric support and notifies when the app resumes. | ||
| * | ||
| * This composable monitors lifecycle events and checks biometric support status | ||
| * whenever the app returns to the foreground ([Lifecycle.Event.ON_RESUME]) or | ||
| * biometric support status changes (via [LaunchedEffect]). | ||
| * | ||
| * @param biometricsManager Manager to check current biometric support status. | ||
| * @param onBiometricSupportChange Callback invoked with the current biometric | ||
| * support status. | ||
| */ | ||
| @Composable | ||
| fun BiometricChanges( | ||
|
Contributor
There was a problem hiding this comment. 🎨 Finding 4: Consider adding KDoc for public composable DetailsPer project guidelines (CLAUDE.md), all public APIs should have KDoc documentation. While there's a brief comment, consider expanding it to include parameter descriptions: /**
* Tracks changes in biometric support and notifies when the app resumes.
*
* This composable monitors lifecycle events and checks biometric support status
* whenever the app returns to the foreground (ON_RESUME), useful for detecting
* when biometric settings change while the app is backgrounded.
*
* @param biometricsManager Manager to check current biometric support status.
* @param onBiometricSupportChange Callback invoked with the current biometric
* support status when the app resumes.
*/This helps other developers understand when and why to use this composable. |
||
| biometricsManager: BiometricsManager, | ||
| onBiometricSupportChange: (isSupported: Boolean) -> Unit, | ||
| ) { | ||
| LaunchedEffect(biometricsManager.isBiometricsSupported) { | ||
|
Contributor
There was a problem hiding this comment. 👍 Well-structured implementation DetailsThe combination of
This dual approach, as suggested by david-livefront, ensures biometric support status is checked in all relevant scenarios.
Contributor
There was a problem hiding this comment. 👍 Finding 2: Comprehensive biometric monitoring implementation DetailsThe combination of
This dual approach ensures biometric support status is checked in all relevant scenarios, addressing david-livefront's suggestion from the review. Well-structured aspects: |
||
| onBiometricSupportChange(biometricsManager.isBiometricsSupported) | ||
|
Contributor
There was a problem hiding this comment. 💭 LaunchedEffect may cause unnecessary emissions DetailsThe Current behavior:
Question: Does If keeping both: Consider whether the initial |
||
| } | ||
|
|
||
| LifecycleEventEffect { _, event -> | ||
|
Collaborator
There was a problem hiding this comment. If this is going to monitor Biometric changes, what do you think about adding this as well? LaunchedEffect(biometricsManager.isBiometricsSupported) {
onBiometricSupportChange(biometricsManager.isBiometricsSupported)
}This will allow this to emit on resume and on recomposition all in one. |
||
| when (event) { | ||
| Lifecycle.Event.ON_RESUME -> { | ||
| onBiometricSupportChange(biometricsManager.isBiometricsSupported) | ||
| } | ||
|
|
||
| else -> Unit | ||
| } | ||
| } | ||
|
Comment on lines
+29
to
+37
Contributor
There was a problem hiding this comment. 💭 Consider adding LaunchedEffect for recomposition detection Detailsdavid-livefront suggested adding a LaunchedEffect(biometricsManager.isBiometricsSupported) {
onBiometricSupportChange(biometricsManager.isBiometricsSupported)
}Current behavior: Only checks on With LaunchedEffect: Would also trigger when Analysis:
Recommendation: Either add the |
||
| } | ||
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.
🎨 Finding 2: Consider adding replay buffer to MutableSharedFlow
Details
The
mutableUserBiometricUnlockKeyFlowusesreplay = 1which 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 usereplay = 1for state that should be immediately available to new subscribers.Current implementation is correct - the
replay = 1parameter ensures that:isUnlockWithBiometricsEnabledFlowproperly reflects the current stateThis is the appropriate pattern for this use case.