From 0e6848d886f7eba95cb2207d6a4693b26f1be750 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn Date: Thu, 7 May 2026 13:46:11 +0200 Subject: [PATCH] fix: Address Codex review on auto-monitor-mode - Seed connectedDevices flow with onStart so ALWAYS autolaunch fires before HEADSET profile binds - Gate UnavailableMissingPermission classification on Android 12+ (BLUETOOTH_CONNECT only exists from API 31) - Treat blank/empty profile.address as unpaired (matches AutoConnect) - Subscribe DeviceSettingsViewModel to nudgeCapabilityStore.availability so UI updates immediately on verdict change - Seed availability StateFlow with the persisted value via valueBlocking to close the cold-start race --- .../common/bluetooth/BluetoothManager2.kt | 9 ++++++++- .../common/bluetooth/NudgeCapabilityStore.kt | 7 ++++++- .../devicesettings/DeviceSettingsViewModel.kt | 4 +++- .../main/ui/overview/OverviewViewModel.kt | 18 ++++++++++++++---- .../capod/monitor/core/MonitorModeResolver.kt | 3 ++- .../monitor/core/MonitorModeResolverTest.kt | 14 ++++++++++++++ 6 files changed, 47 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/eu/darken/capod/common/bluetooth/BluetoothManager2.kt b/app/src/main/java/eu/darken/capod/common/bluetooth/BluetoothManager2.kt index 04e75ffe..9ff7ccd1 100644 --- a/app/src/main/java/eu/darken/capod/common/bluetooth/BluetoothManager2.kt +++ b/app/src/main/java/eu/darken/capod/common/bluetooth/BluetoothManager2.kt @@ -10,6 +10,7 @@ import android.content.BroadcastReceiver import android.content.Context import android.content.Intent import android.content.IntentFilter +import android.os.Build import android.os.Handler import android.os.HandlerThread import dagger.hilt.android.qualifiers.ApplicationContext @@ -443,7 +444,13 @@ class BluetoothManager2 @Inject constructor( Bugs.report(tag = TAG, "BluetoothHeadset.connect(device) is unavailable", exception = e) return@map NudgeAttemptResult.Rejected } - if (!Permission.BLUETOOTH_CONNECT.isGranted(context)) { + // BLUETOOTH_CONNECT is only a runtime permission on Android 12+. On older + // versions a SecurityException can't be a missing-permission failure for that + // permission — it's hidden-API enforcement (e.g. MODIFY_PHONE_STATE) and we + // should persist BROKEN. + val missingBtConnect = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && + !Permission.BLUETOOTH_CONNECT.isGranted(context) + if (missingBtConnect) { log(TAG, WARN) { "nudgeConnection failed because BLUETOOTH_CONNECT is not granted" } NudgeAttemptResult.UnavailableMissingPermission } else { diff --git a/app/src/main/java/eu/darken/capod/common/bluetooth/NudgeCapabilityStore.kt b/app/src/main/java/eu/darken/capod/common/bluetooth/NudgeCapabilityStore.kt index cb3f24b8..9151d537 100644 --- a/app/src/main/java/eu/darken/capod/common/bluetooth/NudgeCapabilityStore.kt +++ b/app/src/main/java/eu/darken/capod/common/bluetooth/NudgeCapabilityStore.kt @@ -4,6 +4,7 @@ import eu.darken.capod.common.coroutine.AppScope import eu.darken.capod.common.coroutine.DispatcherProvider import eu.darken.capod.common.datastore.DataStoreValue import eu.darken.capod.common.datastore.value +import eu.darken.capod.common.datastore.valueBlocking import eu.darken.capod.common.debug.logging.Logging.Priority.INFO import eu.darken.capod.common.debug.logging.log import eu.darken.capod.common.debug.logging.logTag @@ -23,11 +24,15 @@ class NudgeCapabilityStore @Inject constructor( private val dispatcherProvider: DispatcherProvider, ) { + // Resolve the persisted value synchronously at construction so consumers that read + // `availability.value` (resolver, AutoConnect precondition, UI state) never see the + // synthetic UNKNOWN seed before the first DataStore emission arrives. On a known-broken + // device this prevents one bonus ALWAYS-mode + nudge-attempt cycle per cold start. val availability: StateFlow = persistedValue.flow .stateIn( scope = appScope + dispatcherProvider.IO, started = SharingStarted.Eagerly, - initialValue = NudgeAvailability.UNKNOWN, + initialValue = persistedValue.valueBlocking, ) fun record(result: NudgeAttemptResult) { diff --git a/app/src/main/java/eu/darken/capod/main/ui/devicesettings/DeviceSettingsViewModel.kt b/app/src/main/java/eu/darken/capod/main/ui/devicesettings/DeviceSettingsViewModel.kt index a5f74938..898ac1b9 100644 --- a/app/src/main/java/eu/darken/capod/main/ui/devicesettings/DeviceSettingsViewModel.kt +++ b/app/src/main/java/eu/darken/capod/main/ui/devicesettings/DeviceSettingsViewModel.kt @@ -119,6 +119,7 @@ class DeviceSettingsViewModel @Inject constructor( bluetoothManager.connectedDevices.onStart { emit(emptyList()) }, monitorModeResolver.effectiveMode, profilesRepo.profiles, + nudgeCapabilityStore.availability, ) { args -> val device = args[1] as PodDevice? val upgrade = args[2] as UpgradeRepo.Info @@ -130,6 +131,7 @@ class DeviceSettingsViewModel @Inject constructor( @Suppress("UNCHECKED_CAST") val profiles = args[6] as List + val nudgeAvailability = args[7] as NudgeAvailability val stemActions = profiles.filterIsInstance() .firstOrNull { it.id == profileId } ?.stemActions @@ -145,7 +147,7 @@ class DeviceSettingsViewModel @Inject constructor( device = device, now = timeSource.now(), isPro = upgrade.isPro, - isNudgeAvailable = nudgeCapabilityStore.availability.value != NudgeAvailability.BROKEN, + isNudgeAvailable = nudgeAvailability != NudgeAvailability.BROKEN, isForceConnecting = forcing, isClassicallyConnected = device?.address?.let { it in connectedAddresses } == true, monitorMode = monitorMode, diff --git a/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt b/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt index 0b0324ae..9b63b8b5 100644 --- a/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt +++ b/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt @@ -34,9 +34,12 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.channelFlow +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flatMapLatest +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.isActive import java.time.Instant import javax.inject.Inject @@ -79,16 +82,23 @@ class OverviewViewModel @Inject constructor( val workerAutolaunch = combine( permissionTool.missingScanPermissions, monitorModeResolver.effectiveMode, - bluetoothManager.connectedDevices, - ) { missing, mode, connected -> Triple(missing, mode, connected) } - .onEach { (missing, mode, connected) -> + // BluetoothManager2.connectedDevices is gated by a slow HEADSET-profile lookup. Without + // an onStart seed the combine wouldn't fire until that profile is bound, so ALWAYS mode + // would silently delay autolaunch until the first connected-device emission. + bluetoothManager.connectedDevices + .onStart { emit(emptyList()) } + .map { it.isNotEmpty() } + .distinctUntilChanged(), + ) { missing, mode, anyConnected -> Triple(missing, mode, anyConnected) } + .distinctUntilChanged() + .onEach { (missing, mode, anyConnected) -> if (missing.isNotEmpty()) { log(TAG) { "Missing scan permissions: $missing" } return@onEach } val shouldStart = when (mode) { MonitorMode.MANUAL -> false - MonitorMode.AUTOMATIC -> connected.isNotEmpty() + MonitorMode.AUTOMATIC -> anyConnected MonitorMode.ALWAYS -> true } if (shouldStart) { diff --git a/app/src/main/java/eu/darken/capod/monitor/core/MonitorModeResolver.kt b/app/src/main/java/eu/darken/capod/monitor/core/MonitorModeResolver.kt index 3e6e1fcd..d8b0d5d9 100644 --- a/app/src/main/java/eu/darken/capod/monitor/core/MonitorModeResolver.kt +++ b/app/src/main/java/eu/darken/capod/monitor/core/MonitorModeResolver.kt @@ -33,7 +33,8 @@ class MonitorModeResolver @Inject constructor( .onEach { log(TAG, VERBOSE) { "effectiveMode = $it" } } private fun DeviceProfile.requiredMode(nudge: NudgeAvailability): MonitorMode = when { - address == null -> MonitorMode.MANUAL + // Match AutoConnect.kt's isNullOrEmpty check — a blank/legacy "" address is no address. + address.isNullOrBlank() -> MonitorMode.MANUAL toReactionConfig().autoConnect && nudge != NudgeAvailability.BROKEN -> MonitorMode.ALWAYS else -> MonitorMode.AUTOMATIC } diff --git a/app/src/test/java/eu/darken/capod/monitor/core/MonitorModeResolverTest.kt b/app/src/test/java/eu/darken/capod/monitor/core/MonitorModeResolverTest.kt index 450e069b..f4c970ad 100644 --- a/app/src/test/java/eu/darken/capod/monitor/core/MonitorModeResolverTest.kt +++ b/app/src/test/java/eu/darken/capod/monitor/core/MonitorModeResolverTest.kt @@ -85,6 +85,20 @@ class MonitorModeResolverTest : BaseTest() { resolver.effectiveMode.first() shouldBe MonitorMode.MANUAL } + @Test + fun `case 4 variant - profile with blank address is treated as unpaired`() = runTest { + profilesFlow.value = listOf(profile(address = "", autoConnect = true)) + + resolver.effectiveMode.first() shouldBe MonitorMode.MANUAL + } + + @Test + fun `case 4 variant - profile with whitespace address is treated as unpaired`() = runTest { + profilesFlow.value = listOf(profile(address = " ", autoConnect = true)) + + resolver.effectiveMode.first() shouldBe MonitorMode.MANUAL + } + @Test fun `multi-profile primary-only - primary case 2 + secondary case 3 - AUTOMATIC`() = runTest { profilesFlow.value = listOf(