Skip to content

[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.

Sign up for GitHub

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

Merged
andrebispo5 merged 23 commits into from
Nov 18, 2025
Merged

[PM-27150] React to device changes on device screen unlock method #6103

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e55b8b7
[PM-27150] React to device changes on device screen unlock method.
andrebispo5 Oct 30, 2025
3533af5
[PM-27150] Add BiometricUnlockKey flow to AuthDiskSouce
andrebispo5 Nov 6, 2025
46c1cb3
[PM-27150] Add isUnlockWithBiometricsEnabledFlow to SettingsRepository
andrebispo5 Nov 6, 2025
436bcc1
[PM-27150] Remove saved biometric key if device no longer supports it.
andrebispo5 Nov 6, 2025
793e798
[PM-27150] Bypass UnlockScreen if device no longer supports it.
andrebispo5 Nov 6, 2025
3ce913d
[PM-27150] Update AppLock row visibility and toggle based on device b…
andrebispo5 Nov 6, 2025
e7ac063
[PM-27150] Add PR suggestions.
andrebispo5 Nov 7, 2025
85ff4e7
[PM-27150] Fix typo
andrebispo5 Nov 7, 2025
de0947a
[PM-27150] Add replay 1
andrebispo5 Nov 7, 2025
12c28c2
Merge branch 'main' into pm-27150/authenticator-unlock-enabled-bug
andrebispo5 Nov 7, 2025
3afe1b3
[PM-27150] Fix pre existing typo
andrebispo5 Nov 12, 2025
790ccd0
[PM-27150] Make isUnlockWithBiometricsEnabledFlow state flow
andrebispo5 Nov 12, 2025
385caae
[PM-27150] Extract BiometricChanges composable to be reused.
andrebispo5 Nov 12, 2025
9e28ed5
[PM-27150] Change BiometricSupportChanged to be external.
andrebispo5 Nov 12, 2025
8492014
[PM-27150] Track device biometric changes in Settings UI and ViewModel.
andrebispo5 Nov 12, 2025
4556e1a
[PM-27150] Add Kdoc
andrebispo5 Nov 12, 2025
9180c52
[PM-27150] Pass callback value
andrebispo5 Nov 12, 2025
2de0197
[PM-27150] Add missing test classes for RootNav in Authenticator.
andrebispo5 Nov 13, 2025
df27f32
[PM-27150] Init biometric support as true
andrebispo5 Nov 13, 2025
25f028c
[PM-27150] Move unlock screen navigation to RootNavViewModel
andrebispo5 Nov 13, 2025
d09486f
[PM-27150] Add launch effect to BiometricChanges composable
andrebispo5 Nov 13, 2025
3018a6c
[PM-27150] Add remember viewmodel to avoid recreation on recomposition.
andrebispo5 Nov 13, 2025
3c01f60
[PM-27150] Change assert to assertTrue or assertFalse
andrebispo5 Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/main/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ interface AuthDiskSource : AppIdProvider {
/**
* Gets the flow for the biometrics key for the given [userId].
*/
fun getUserBiometicUnlockKeyFlow(userId: String): Flow<String?>
fun getUserBiometricUnlockKeyFlow(userId: String): Flow<String?>

/**
* Retrieves a pin-protected user key for the given [userId].
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ class AuthDiskSourceImpl(
getMutableBiometricUnlockKeyFlow(userId).tryEmit(biometricsKey)
}

override fun getUserBiometicUnlockKeyFlow(userId: String): Flow<String?> =
override fun getUserBiometricUnlockKeyFlow(userId: String): Flow<String?> =
getMutableBiometricUnlockKeyFlow(userId)
.onSubscription { emit(getUserBiometricUnlockKey(userId = userId)) }

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/kotlin/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ class SettingsRepositoryImpl(
get() = activeUserId
?.let { userId ->
authDiskSource
.getUserBiometicUnlockKeyFlow(userId)
.getUserBiometricUnlockKeyFlow(userId)
.map { it != null }
}
?: flowOf(false)
Expand Down
23 changes: 21 additions & 2 deletions app/src/test/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,25 @@ class AuthDiskSourceTest {
assertEquals(biometricsKey, actual)
}

@Test
fun `getUserBiometricUnlockKeyFlow should react to changes in getUserBiometricUnlockKey`() =
runTest {
val mockUserId = "mockUserId"
val biometricsKey = "1234"
authDiskSource.getUserBiometricUnlockKeyFlow(userId = mockUserId).test {
// The initial values of the Flow and the property are in sync
assertNull(authDiskSource.getUserBiometricUnlockKey(userId = mockUserId))
assertNull(awaitItem())

// Updating the disk source updates shared preferences
authDiskSource.storeUserBiometricUnlockKey(
userId = mockUserId,
biometricsKey = biometricsKey,
)
assertEquals(biometricsKey, awaitItem())
}
}

@Test
fun `storeUserBiometricInitVector for non-null values should update SharedPreferences`() {
val biometricsInitVectorBaseKey = "bwSecureStorage:biometricInitializationVector"
Expand Down Expand Up @@ -773,11 +792,11 @@ class AuthDiskSourceTest {

@Suppress("MaxLineLength")
@Test
fun `storeUserBiometricUnlockKey should update the resulting flow from getUserBiometicUnlockKeyFlow`() =
fun `storeUserBiometricUnlockKey should update the resulting flow from getUserBiometricUnlockKeyFlow`() =
runTest {
val topSecretKey = "topsecret"
val mockUserId = "mockUserId"
authDiskSource.getUserBiometicUnlockKeyFlow(mockUserId).test {
authDiskSource.getUserBiometricUnlockKeyFlow(mockUserId).test {
assertNull(awaitItem())
authDiskSource.storeUserBiometricUnlockKey(
userId = mockUserId,
Expand Down
2 changes: 1 addition & 1 deletion app/src/test/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/util/FakeAuthDiskSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ class FakeAuthDiskSource : AuthDiskSource {
getMutableBiometricUnlockKeyFlow(userId).tryEmit(biometricsKey)
}

override fun getUserBiometicUnlockKeyFlow(userId: String): Flow<String?> =
override fun getUserBiometricUnlockKeyFlow(userId: String): Flow<String?> =
getMutableBiometricUnlockKeyFlow(userId)
.onSubscription { emit(getUserBiometricUnlockKey(userId)) }

Expand Down
6 changes: 6 additions & 0 deletions ...r/src/main/kotlin/com/bitwarden/authenticator/data/auth/datasource/disk/AuthDiskSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
package com.bitwarden.authenticator.data.auth.datasource.disk

import com.bitwarden.network.provider.AppIdProvider
import kotlinx.coroutines.flow.Flow

/**
* Primary access point for disk information.
*/
interface AuthDiskSource : AppIdProvider {

/**
* Tracks the biometrics key.
*/
val userBiometricUnlockKeyFlow: Flow<String?>

/**
* Retrieves the "last active time".
*
Expand Down
12 changes: 12 additions & 0 deletions ...c/main/kotlin/com/bitwarden/authenticator/data/auth/datasource/disk/AuthDiskSourceImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package com.bitwarden.authenticator.data.auth.datasource.disk

import android.content.SharedPreferences
import com.bitwarden.core.data.repository.util.bufferedMutableSharedFlow
import com.bitwarden.data.datasource.disk.BaseEncryptedDiskSource
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.onSubscription
import java.util.UUID

private const val AUTHENTICATOR_SYNC_SYMMETRIC_KEY = "authenticatorSyncSymmetricKey"
Expand All @@ -20,6 +23,7 @@ class AuthDiskSourceImpl(
sharedPreferences = sharedPreferences,
),
AuthDiskSource {
private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1)
Copy link
Contributor

@claude claude bot Nov 7, 2025

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 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:

  1. New subscribers immediately receive the current biometric key state
  2. The flow behaves consistently with other state flows in the codebase
  3. The isUnlockWithBiometricsEnabledFlow properly reflects the current state

This is the appropriate pattern for this use case.

Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Finding 6: Correct use of replay buffer in MutableSharedFlow

Details

The mutableUserBiometricUnlockKeyFlow correctly uses replay = 1:

private val mutableUserBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>(replay = 1)

Why this is correct:

  • Replay buffer ensures state availability: New subscribers immediately receive the most recent biometric key value
  • Matches password manager implementation: Consistent with the pattern in app/src/main/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt
  • Proper state semantics: The biometric key is stateful data that should be immediately available to observers

Flow behavior:

  1. onSubscription emits current value from disk
  2. replay = 1 ensures late subscribers get the most recent emission
  3. tryEmit in storeUserBiometricUnlockKey broadcasts changes

This implementation correctly maintains a reactive, state-like flow for biometric key changes.

Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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:

  1. onSubscription emits current value from disk
  2. replay = 1 ensures late subscribers get the most recent emission
  3. tryEmit in storeUserBiometricUnlockKey broadcasts changes to all active collectors

This implementation correctly maintains a reactive, state-like flow for biometric key changes.

Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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:

  1. New collectors get the current value from disk immediately
  2. Late subscribers also receive the most recent emission
  3. Changes propagate to all active collectors via tryEmit

This correctly maintains a reactive, state-like flow for biometric key changes.


override val uniqueAppId: String
get() = getString(key = UNIQUE_APP_ID_KEY) ?: generateAndStoreUniqueAppId()
Expand All @@ -39,13 +43,21 @@ class AuthDiskSourceImpl(
override fun getUserBiometricUnlockKey(): String? =
getEncryptedString(key = BIOMETRICS_UNLOCK_KEY)

override val userBiometricUnlockKeyFlow: Flow<String?>
get() =
mutableUserBiometricUnlockKeyFlow
.onSubscription {
emit(getUserBiometricUnlockKey())
}

override fun storeUserBiometricUnlockKey(
biometricsKey: String?,
) {
putEncryptedString(
key = BIOMETRICS_UNLOCK_KEY,
value = biometricsKey,
)
mutableUserBiometricUnlockKeyFlow.tryEmit(biometricsKey)
}

override var authenticatorBridgeSymmetricSyncKey: ByteArray?
Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/kotlin/com/bitwarden/authenticator/data/platform/repository/SettingsRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ interface SettingsRepository {
*/
val isUnlockWithBiometricsEnabled: Boolean

/**
* Tracks whether or not biometric unlocking is enabled for the current user.
*/
val isUnlockWithBiometricsEnabledFlow: StateFlow<Boolean>

/**
* Tracks changes to the expiration alert threshold.
*/
Expand Down
11 changes: 11 additions & 0 deletions ...ain/kotlin/com/bitwarden/authenticator/data/platform/repository/SettingsRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ class SettingsRepositoryImpl(
override val isUnlockWithBiometricsEnabled: Boolean
get() = authDiskSource.getUserBiometricUnlockKey() != null

override val isUnlockWithBiometricsEnabledFlow: StateFlow<Boolean>
get() =
authDiskSource
.userBiometricUnlockKeyFlow
.map { it != null }
.stateIn(
scope = unconfinedScope,
started = SharingStarted.Eagerly,
initialValue = isUnlockWithBiometricsEnabled,
)

override val appThemeStateFlow: StateFlow<AppTheme>
get() = settingsDiskSource
.appThemeFlow
Expand Down
7 changes: 7 additions & 0 deletions authenticator/src/main/kotlin/com/bitwarden/authenticator/ui/auth/unlock/UnlockScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.width
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
Expand Down Expand Up @@ -53,6 +54,12 @@ fun UnlockScreen(
}
}

LaunchedEffect(biometricsManager.isBiometricsSupported) {
Copy link
Collaborator

@david-livefront david-livefront Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, isn't the UnlockScreen a top-level screen? I believe this should be state driven in the RootNavViewModel, right?

if (!biometricsManager.isBiometricsSupported) {
onUnlocked()
}
}
Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Navigation bypass violates MVVM architecture

Details

This LaunchedEffect directly calls onUnlocked() when biometrics are unsupported, bypassing the ViewModel and creating screen-level navigation logic.

Issue: Per docs/ARCHITECTURE.md:298:

Any state that will eventually need to be used by the VM should be hoisted to the VM... These state changes are communicated via the "actions" that the Compose layer sends.

Navigation decisions should be state-driven from RootNavViewModel, not made locally in screens.

Why this is problematic:

  1. Inconsistent navigation logic: Some navigation happens via ViewModel state, some happens directly in the screen
  2. Harder to test: This navigation logic is not unit-testable through the ViewModel
  3. Violates separation of concerns: The screen should only render state, not make navigation decisions

Recommendation: Remove this LaunchedEffect entirely. The correct flow already exists:

  1. BiometricChanges in RootNavScreen detects biometric support loss on resume
  2. Sends BiometricSupportChanged(false) to RootNavViewModel
  3. RootNavViewModel.handleBiometricSupportChanged clears the biometric key
  4. State flow updates cause RootNavViewModel.handleHasSeenWelcomeTutorialChange to navigate to Unlocked state (lines 79-85 already check isUnlockWithBiometricsEnabled)

This ensures all navigation logic is centralized, state-driven, and testable.


when (val dialog = state.dialog) {
is UnlockState.Dialog.Error -> BitwardenBasicDialog(
title = stringResource(id = BitwardenString.an_error_has_occurred),
Expand Down
33 changes: 33 additions & 0 deletions .../kotlin/com/bitwarden/authenticator/ui/platform/components/biometrics/BiometricChanges.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package com.bitwarden.authenticator.ui.platform.components.biometrics

import androidx.compose.runtime.Composable
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 (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.
*/
@Composable
fun BiometricChanges(
Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Finding 4: Consider adding KDoc for public composable

Details

Per 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,
) {
LifecycleEventEffect { _, event ->
Copy link
Collaborator

@david-livefront david-livefront Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Consider adding LaunchedEffect for recomposition detection

Details

david-livefront suggested adding a LaunchedEffect to detect biometric changes on recomposition, not just lifecycle events:

LaunchedEffect(biometricsManager.isBiometricsSupported) {
    onBiometricSupportChange(biometricsManager.isBiometricsSupported)
}

Current behavior: Only checks on ON_RESUME lifecycle events

With LaunchedEffect: Would also trigger when isBiometricsSupported changes during recomposition

Analysis:

  • Biometric support typically only changes when the app is backgrounded (user disables in Settings), so ON_RESUME detection should be sufficient
  • Adding LaunchedEffect would make it more comprehensive but may be unnecessary overhead
  • Current implementation is simpler and covers the primary use case

Recommendation: Either add the LaunchedEffect per david-livefront's suggestion for completeness, or document why lifecycle-only monitoring is sufficient for this use case.

}
13 changes: 12 additions & 1 deletion .../src/main/kotlin/com/bitwarden/authenticator/ui/platform/feature/rootnav/RootNavScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@ import com.bitwarden.authenticator.ui.auth.unlock.unlockDestination
import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.AuthenticatorGraphRoute
import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.authenticatorGraph
import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.navigateToAuthenticatorGraph
import com.bitwarden.authenticator.ui.platform.components.biometrics.BiometricChanges
import com.bitwarden.authenticator.ui.platform.composition.LocalBiometricsManager
import com.bitwarden.authenticator.ui.platform.feature.splash.SplashRoute
import com.bitwarden.authenticator.ui.platform.feature.splash.navigateToSplash
import com.bitwarden.authenticator.ui.platform.feature.splash.splashDestination
import com.bitwarden.authenticator.ui.platform.feature.tutorial.TutorialRoute
import com.bitwarden.authenticator.ui.platform.feature.tutorial.navigateToTutorial
import com.bitwarden.authenticator.ui.platform.feature.tutorial.tutorialDestination
import com.bitwarden.authenticator.ui.platform.manager.biometrics.BiometricsManager
import com.bitwarden.ui.platform.theme.NonNullEnterTransitionProvider
import com.bitwarden.ui.platform.theme.NonNullExitTransitionProvider
import com.bitwarden.ui.platform.theme.RootTransitionProviders
Expand All @@ -41,7 +44,8 @@ import java.util.concurrent.atomic.AtomicReference
fun RootNavScreen(
viewModel: RootNavViewModel = hiltViewModel(),
navController: NavHostController = rememberNavController(),
onSplashScreenRemoved: () -> Unit,
biometricsManager: BiometricsManager = LocalBiometricsManager.current,
onSplashScreenRemoved: () -> Unit = {},
) {
val state by viewModel.stateFlow.collectAsState()
val previousStateReference = remember { AtomicReference(state) }
Expand All @@ -61,6 +65,13 @@ fun RootNavScreen(
.launchIn(this)
}

BiometricChanges(
Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Finding 5: Well-structured biometric monitoring implementation

Details

The BiometricChanges composable is properly integrated at the root navigation level:

✓ Separated into a reusable composable function with comprehensive KDoc
✓ Uses LifecycleEventEffect to monitor app lifecycle
✓ Clear callback interface that sends actions to ViewModel
✓ Checks biometric support on ON_RESUME to detect changes

Architectural correctness: Placing this at the RootNav level is appropriate since:

  1. Biometric state affects navigation decisions (whether to show unlock screen)
  2. Centralized monitoring ensures consistent behavior across the app
  3. Both RootNavViewModel and SettingsViewModel receive updates about biometric support changes

This approach follows MVVM principles and maintains a single source of truth for biometric support status.

biometricsManager = biometricsManager,
onBiometricSupportChange = {
viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it))
},
Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 1: Callback creates unnecessary lambda allocation on each recomposition

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).

)
Comment on lines 68 to 75
Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 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 viewModel from hiltViewModel()), explicit remember makes 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)


NavHost(
navController = navController,
startDestination = SplashRoute,
Expand Down
30 changes: 25 additions & 5 deletions ...c/main/kotlin/com/bitwarden/authenticator/ui/platform/feature/rootnav/RootNavViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class RootNavViewModel @Inject constructor(
}

is RootNavAction.Internal.HasSeenWelcomeTutorialChange -> {
handleHasSeenWelcomeTutorialChange(action.hasSeenWelcomeGuide)
handleHasSeenWelcomeTutorialChange(action)
Copy link
Collaborator

@david-livefront david-livefront Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

RootNavAction.Internal.TutorialFinished -> {
Expand All @@ -60,18 +60,25 @@ class RootNavViewModel @Inject constructor(
RootNavAction.Internal.AppUnlocked -> {
handleAppUnlocked()
}

is RootNavAction.BiometricSupportChanged -> {
handleBiometricSupportChanged(action)
}
}
}

private fun handleBackStackUpdate() {
authRepository.updateLastActiveTime()
}

private fun handleHasSeenWelcomeTutorialChange(hasSeenWelcomeGuide: Boolean) {
settingsRepository.hasSeenWelcomeTutorial = hasSeenWelcomeGuide
if (hasSeenWelcomeGuide) {
private fun handleHasSeenWelcomeTutorialChange(
action: RootNavAction.Internal.HasSeenWelcomeTutorialChange,
Copy link
Collaborator

@david-livefront david-livefront Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

) {
settingsRepository.hasSeenWelcomeTutorial = action.hasSeenWelcomeGuide
if (action.hasSeenWelcomeGuide) {
if (settingsRepository.isUnlockWithBiometricsEnabled &&
biometricsEncryptionManager.isBiometricIntegrityValid()) {
biometricsEncryptionManager.isBiometricIntegrityValid()
) {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Locked) }
} else {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Unlocked) }
Expand Down Expand Up @@ -99,6 +106,14 @@ class RootNavViewModel @Inject constructor(
it.copy(navState = RootNavState.NavState.Unlocked)
}
}

private fun handleBiometricSupportChanged(
Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 2: Missing test coverage for handleBiometricSupportChanged

Details

The handleBiometricSupportChanged function has no test coverage. The code coverage report shows 9 missing lines in RootNavViewModel.kt.

Required test:

  • Verify that settingsRepository.clearBiometricsKey() is called when isBiometricsSupported is false
  • Verify that nothing happens when isBiometricsSupported is true

Test should be added to authenticator/src/test/kotlin/com/bitwarden/authenticator/ui/platform/feature/rootnav/RootNavViewModelTest.kt (if it exists, otherwise create it following the pattern in the app module).

Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 3: Missing test coverage for handleBiometricSupportChanged

Details

The handleBiometricSupportChanged function lacks test coverage, contributing to the 23 missing lines reported by codecov.

Required tests:

  1. Verify settingsRepository.clearBiometricsKey() is called when isBiometricsSupported is false
  2. Verify no action is taken when isBiometricsSupported is true

Example test structure:

@Test
fun `handleBiometricSupportChanged should clear biometrics key when not supported`() = runTest {
    val action = RootNavAction.BiometricSupportChanged(isBiometricsSupported = false)
    
    viewModel.trySendAction(action)
    
    verify { settingsRepository.clearBiometricsKey() }
}

@Test
fun `handleBiometricSupportChanged should do nothing when biometrics supported`() = runTest {
    val action = RootNavAction.BiometricSupportChanged(isBiometricsSupported = true)
    
    viewModel.trySendAction(action)
    
    verify(exactly = 0) { settingsRepository.clearBiometricsKey() }
}

Add these tests to RootNavViewModelTest.kt to ensure proper behavior and improve coverage.

Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 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.

Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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:

  1. BiometricChanges detects change on ON_RESUME
  2. RootNavViewModel receives BiometricSupportChanged(false)
  3. Biometric key cleared via settingsRepository.clearBiometricsKey()
  4. If locked, immediately navigates to unlocked state
  5. Flow propagates to SettingsViewModel to update UI

action: RootNavAction.BiometricSupportChanged,
) {
if (!action.isBiometricsSupported) {
settingsRepository.clearBiometricsKey()
Copy link
Contributor

@claude claude bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 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.

}
}
}

/**
Expand Down Expand Up @@ -153,6 +168,11 @@ sealed class RootNavAction {
*/
data object BackStackUpdate : RootNavAction()

/**
* Indicates an update on device biometrics support.
*/
data class BiometricSupportChanged(val isBiometricsSupported: Boolean) : RootNavAction()

/**
* Models actions the [RootNavViewModel] itself may send.
*/
Expand Down
15 changes: 12 additions & 3 deletions ...rc/main/kotlin/com/bitwarden/authenticator/ui/platform/feature/settings/SettingsScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import androidx.compose.ui.unit.dp
import androidx.core.net.toUri
import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.bitwarden.authenticator.ui.platform.components.biometrics.BiometricChanges
import com.bitwarden.authenticator.ui.platform.composition.LocalBiometricsManager
import com.bitwarden.authenticator.ui.platform.feature.settings.appearance.model.AppLanguage
import com.bitwarden.authenticator.ui.platform.feature.settings.data.model.DefaultSaveOption
Expand Down Expand Up @@ -134,6 +135,15 @@ fun SettingsScreen(
}
}

BiometricChanges(
biometricsManager = biometricsManager,
onBiometricSupportChange = { isSupported ->
Copy link
Contributor

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Finding 2: Callback parameter naming could be more explicit

Details

The callback uses a lambda parameter isSupported but the name could be more explicit:

Current:

onBiometricSupportChange = { isSupported ->
    viewModel.trySendAction(
        SettingsAction.BiometricSupportChanged(isSupported),
    )
},

Minor improvement: While functional, consider using the parameter name explicitly for clarity:

onBiometricSupportChange = { isBiometricsSupported ->
    viewModel.trySendAction(
        SettingsAction.BiometricSupportChanged(isBiometricsSupported),
    )
},

This makes it clearer what boolean value is being passed and matches the action parameter name.

viewModel.trySendAction(
SettingsAction.BiometricSupportChanged(isSupported),
)
},
)

BitwardenScaffold(
modifier = Modifier
.fillMaxSize()
Expand Down Expand Up @@ -287,8 +297,7 @@ private fun SecuritySettings(
)

Spacer(modifier = Modifier.height(8.dp))
val hasBiometrics = biometricsManager.isBiometricsSupported
if (hasBiometrics) {
if (state.hasBiometricsSupport) {
UnlockWithBiometricsRow(
modifier = Modifier
.testTag("UnlockWithBiometricsSwitch")
Expand All @@ -302,7 +311,7 @@ private fun SecuritySettings(

ScreenCaptureRow(
currentValue = state.allowScreenCapture,
cardStyle = if (hasBiometrics) {
cardStyle = if (state.hasBiometricsSupport) {
CardStyle.Bottom
} else {
CardStyle.Full
Expand Down
Loading
Loading