Address copilot feedbacks

Author: TimoPtr

app/src/main/kotlin/io/homeassistant/companion/android/frontend/improv/FrontendImprovOrchestrator.kt

@@ -43,11 +43,11 @@ import timber.log.Timber
  *   reload.
  *
  * **Concurrency contract.** Entry points are safe to call from any coroutine. Var-backed internal
- * state ([provisioningJob], [discoveredDevices]) is guarded by [mutex]; StateFlow-backed state
- * ([scanRequested], [uiState]) is atomic by construction and not lock-guarded. The orchestrator
- * does not own a long-lived scope: the caller passes its scope to [onConnectDevice] for
- * provisioning, and the UI layer hosts the scan collect through [processImprovScanRequests] so
- * navigation away from the frontend route naturally pauses BLE work.
+ * state ([provisioningJob], [discoveredDevices], [sentDeviceNames]) is guarded by [mutex];
+ * StateFlow-backed state ([scanRequested], [uiState]) is atomic by construction and not
+ * lock-guarded. The orchestrator does not own a long-lived scope: the caller passes its scope to
+ * [onConnectDevice] for provisioning, and the UI layer hosts the scan collect through
+ * [processImprovScanRequests] so navigation away from the frontend route naturally pauses BLE work.
  */
 @ViewModelScoped
 internal class FrontendImprovOrchestrator @Inject constructor(
@@ -83,6 +83,17 @@ internal class FrontendImprovOrchestrator @Inject constructor(
      */
     private var discoveredDevices: List<ImprovDevice> = emptyList()
 
+    /**
+     * Device names already forwarded to the frontend during the current session (from
+     * [onStartImprovScan] until [onDismissed]). Survives `processImprovScanRequests` restarts
+     * caused by the UI's lifecycle-bound collect tearing down on PAUSE and re-running on RESUME —
+     * the scan flow replays its current device list to the new collector, and re-sending those
+     * names would surface duplicates in the frontend's device list. Cleared on [onDismissed].
+     *
+     * Reads and writes go through [mutex].
+     */
+    private val sentDeviceNames: MutableSet<String> = mutableSetOf()
+
     private val _uiState = MutableStateFlow<ImprovUIState?>(null)
 
     /**
@@ -230,6 +241,7 @@ internal class FrontendImprovOrchestrator @Inject constructor(
             provisioningJob = null
             _scanRequested.value = false
             discoveredDevices = emptyList()
+            sentDeviceNames.clear()
             _uiState.value = null
         }
         if (domain != null) {
@@ -275,16 +287,24 @@ internal class FrontendImprovOrchestrator @Inject constructor(
                     }
                 }
                 is ProvisioningEvent.Provisioned -> {
+                    var transitioned = false
                     _uiState.update { current ->
-                        // Guard against a Provisioned event arriving after the user dismissed the
-                        // sheet — overwriting `null` with `Provisioned` would briefly resurrect it.
+                        // Guard against a Provisioned event arriving after the user dismissed,
+                        // restarted, or after an Errored event overwriting any of those would
+                        // resurrect the UI or misrepresent the outcome.
                         if (current is ImprovUIState.Provisioning) {
+                            transitioned = true
                             ImprovUIState.Provisioned(domain = event.domain)
                         } else {
+                            transitioned = false
                             current
                         }
                     }
-                    externalBusRepository.send(ImprovDeviceSetupDoneMessage)
+                    // Gate the frontend signal on the same condition telling the frontend setup
+                    // is done while the UI is errored/closed would be inconsistent.
+                    if (transitioned) {
+                        externalBusRepository.send(ImprovDeviceSetupDoneMessage)
+                    }
                 }
             }
         }
@@ -295,13 +315,17 @@ internal class FrontendImprovOrchestrator @Inject constructor(
      * emission: mirrors the list into [discoveredDevices], promotes
      * [ImprovUIState.SearchingDevice] to [ImprovUIState.ConfiguringDevice] when the target
      * appears, and forwards each newly-seen device name to the frontend via
-     * [ImprovDiscoveredDeviceMessage] — deduped per call so a single scan session never sends
-     * duplicates.
+     * [ImprovDiscoveredDeviceMessage] — deduped against [sentDeviceNames] so the frontend sees
+     * each name exactly once per session (across collector restarts).
      */
     private suspend fun forwardDiscoveredDevices() {
-        val sentNames = mutableSetOf<String>()
         improvRepository.scanDevices().collect { devices ->
-            mutex.withLock { discoveredDevices = devices }
+            val namesToForward = mutex.withLock {
+                discoveredDevices = devices
+                // Compute the diff inside the lock so it's atomic with the [sentDeviceNames]
+                // update; emit outside the lock so the suspending [send] doesn't hold the mutex.
+                devices.mapNotNull { it.name }.filter { sentDeviceNames.add(it) }
+            }
             // Promote SearchingDevice → ConfiguringDevice as soon as the scan resolves the BLE
             // address. Once we leave SearchingDevice, late scan emissions must not overwrite the
             // user-driven transitions (Provisioning, Errored, …).
@@ -317,12 +341,8 @@ internal class FrontendImprovOrchestrator @Inject constructor(
                     current
                 }
             }
-            // Forward each newly-seen device name to the frontend exactly once per session.
-            devices.forEach { device ->
-                val name = device.name ?: return@forEach
-                if (sentNames.add(name)) {
-                    externalBusRepository.send(ImprovDiscoveredDeviceMessage(name = name))
-                }
+            namesToForward.forEach { name ->
+                externalBusRepository.send(ImprovDiscoveredDeviceMessage(name = name))
             }
         }
     }

app/src/test/kotlin/io/homeassistant/companion/android/frontend/improv/FrontendImprovOrchestratorTest.kt

@@ -194,6 +194,32 @@ class FrontendImprovOrchestratorTest {
         job.cancel()
     }
 
+    @Test
+    fun `Given scan collector cancelled and resubscribed while session still active then previously sent device names are not re-sent`() = runTest {
+        val orchestrator = createOrchestrator()
+        orchestrator.onStartImprovScan()
+
+        // First lifecycle-bound collect — simulates the foreground RESUMED effect.
+        val firstJob = launch { orchestrator.processImprovScanRequests() }
+        advanceUntilIdle()
+        scanFlow.emit(listOf(ImprovDevice("Smart Plug", "AA:BB")))
+        advanceUntilIdle()
+        coVerify(exactly = 1) { externalBusRepository.send(any<OutgoingExternalBusMessage>()) }
+
+        // Simulate app backgrounding — lifecycle cancels processImprovScanRequests. The session
+        // is still active (no onDismissed, _scanRequested stays true).
+        firstJob.cancel()
+
+        // Foreground again: the lifecycle effect re-runs processImprovScanRequests. The repository's
+        // scanFlow still holds the previously discovered device (replay semantics), so without
+        // session-level dedup the orchestrator re-sends it to the frontend.
+        val secondJob = launch { orchestrator.processImprovScanRequests() }
+        advanceUntilIdle()
+
+        coVerify(exactly = 1) { externalBusRepository.send(any<OutgoingExternalBusMessage>()) }
+        secondJob.cancel()
+    }
+
     @Test
     fun `Given onConfigureImprovDevice when handled then uiState is initialised to SearchingDevice`() = runTest {
         val orchestrator = createOrchestrator()
@@ -265,6 +291,31 @@ class FrontendImprovOrchestratorTest {
         provisioningScope.cancel()
     }
 
+    @Test
+    fun `Given Errored state when late Provisioned event arrives then device_setup_done is not sent`() = runTest {
+        val orchestrator = createOrchestrator()
+        val scanJob = configureWithResolvedAddress(orchestrator)
+        val provisioningScope = CoroutineScope(coroutineContext + Job())
+        orchestrator.onConnectDevice(scope = provisioningScope, ssid = "wifi", password = "pwd")
+        advanceUntilIdle()
+
+        // First an error transitions UI to Errored.
+        provisionFlow.emit(ProvisioningEvent.ErrorOccurred(ErrorState.UNABLE_TO_CONNECT))
+        advanceUntilIdle()
+        assertInstanceOf(ImprovUIState.Errored::class.java, orchestrator.uiState.value)
+
+        // Then a late Provisioned event from the BLE stack. The UI must stay Errored AND no
+        // setup-done message should be forwarded — otherwise the frontend would think setup
+        // succeeded while the sheet still shows the error.
+        provisionFlow.emit(ProvisioningEvent.Provisioned(domain = "acme"))
+        advanceUntilIdle()
+
+        assertInstanceOf(ImprovUIState.Errored::class.java, orchestrator.uiState.value)
+        coVerify(exactly = 0) { externalBusRepository.send(ImprovDeviceSetupDoneMessage) }
+        scanJob.cancel()
+        provisioningScope.cancel()
+    }
+
     @Test
     fun `Given onConnectDevice while still SearchingDevice then provisionDevice is not called`() = runTest {
         val orchestrator = createOrchestrator()