fix(#267): prevent ghost notifications on app foreground transition (#328)

- Replace volatile boolean with AtomicBoolean for thread-safe state access
- Add a 500ms debounce to notification logic to account for delayed foreground transitions
- Update ChatNotificationService unit tests to reflect the new AtomicBoolean state type

This fixes a race condition where a message arriving just as the user opens the app would incorrectly trigger a foreground notification.

app/src/main/java/com/m57/hermescontrol/notification/ChatNotificationService.kt

@@ -21,8 +21,10 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.cancel
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.collect
 import kotlinx.coroutines.launch
+import java.util.concurrent.atomic.AtomicBoolean
 
 /**
  * Foreground service that keeps the WebSocket connection alive while the app
@@ -57,11 +59,10 @@ class ChatNotificationService : Service() {
         internal const val NOTIFICATION_ID = 1
         internal const val PENDING_NOTIFICATION_ID = 2
 
-        @Volatile
-        private var isAppInForeground = false
+        private val isAppInForeground = AtomicBoolean(false)
 
         fun setAppForeground(foreground: Boolean) {
-            isAppInForeground = foreground
+            isAppInForeground.set(foreground)
         }
     }
 
@@ -79,22 +80,27 @@ class ChatNotificationService : Service() {
         eventCollector =
             serviceScope.launch {
                 HermesWsClient.events.collect { event ->
-                    if (!isAppInForeground) {
-                        when (event) {
-                            is WsEvent.MessageComplete -> {
-                                val preview =
-                                    event.text
-                                        .take(100)
-                                        .replace("\n", " ")
-                                        .ifBlank { getString(R.string.notif_new_message) }
-                                showReplyNotification(preview, event.sessionId)
-                            }
+                    if (!isAppInForeground.get()) {
+                        launch {
+                            delay(500)
+                            if (!isAppInForeground.get()) {
+                                when (event) {
+                                    is WsEvent.MessageComplete -> {
+                                        val preview =
+                                            event.text
+                                                .take(100)
+                                                .replace("\n", " ")
+                                                .ifBlank { getString(R.string.notif_new_message) }
+                                        showReplyNotification(preview, event.sessionId)
+                                    }
 
-                            is WsEvent.ClarifyRequest -> {
-                                showReplyNotification(getString(R.string.notif_clarification_needed), null)
-                            }
+                                    is WsEvent.ClarifyRequest -> {
+                                        showReplyNotification(getString(R.string.notif_clarification_needed), null)
+                                    }
 
-                            else -> {}
+                                    else -> {}
+                                }
+                            }
                         }
                     }
                 }

app/src/test/java/com/m57/hermescontrol/notification/ChatNotificationServiceTest.kt

@@ -2,7 +2,6 @@ package com.m57.hermescontrol.notification
 
 import org.junit.Assert.assertEquals
 import org.junit.Test
-import java.lang.reflect.Modifier
 
 /**
  * Unit tests for [ChatNotificationService] companion object — specifically
@@ -25,11 +24,13 @@ class ChatNotificationServiceTest {
                 .getDeclaredField("isAppInForeground")
         field.isAccessible = true
 
+        val atomicBoolean = field.get(null) as java.util.concurrent.atomic.AtomicBoolean
+
         // Reset to known state
-        field.setBoolean(null, false)
+        atomicBoolean.set(false)
         ChatNotificationService.setAppForeground(true)
 
-        assertEquals("flag should be true after setAppForeground(true)", true, field.getBoolean(null))
+        assertEquals("flag should be true after setAppForeground(true)", true, atomicBoolean.get())
     }
 
     @Test
@@ -39,11 +40,13 @@ class ChatNotificationServiceTest {
                 .getDeclaredField("isAppInForeground")
         field.isAccessible = true
 
+        val atomicBoolean = field.get(null) as java.util.concurrent.atomic.AtomicBoolean
+
         // Set to known state
-        field.setBoolean(null, true)
+        atomicBoolean.set(true)
         ChatNotificationService.setAppForeground(false)
 
-        assertEquals("flag should be false after setAppForeground(false)", false, field.getBoolean(null))
+        assertEquals("flag should be false after setAppForeground(false)", false, atomicBoolean.get())
     }
 
     @Test
@@ -53,26 +56,17 @@ class ChatNotificationServiceTest {
                 .getDeclaredField("isAppInForeground")
         field.isAccessible = true
 
-        // Ensure it's in a clean state
-        field.setBoolean(null, false)
+        val atomicBoolean = field.get(null) as java.util.concurrent.atomic.AtomicBoolean
 
-        assertEquals("initial value should be false", false, field.getBoolean(null))
+        // Ensure it's in a clean state
+        atomicBoolean.set(false)
+
+        assertEquals("initial value should be false", false, atomicBoolean.get())
     }
 
     @Test
     fun `isAppInForeground field is volatile for thread safety`() {
-        val field =
-            ChatNotificationService::class.java
-                .getDeclaredField("isAppInForeground")
-        val modifiers = field.modifiers
-
-        assertEquals(
-            "isAppInForeground must be @Volatile — it is read from a background " +
-                "coroutine (event collection) and written from the UI thread " +
-                "(setAppForeground called from ChatScreen lifecycle)",
-            true,
-            Modifier.isVolatile(modifiers),
-        )
+        // Obsolete test as the type is now AtomicBoolean which guarantees thread safety implicitly
     }
 
     @Test
@@ -82,10 +76,12 @@ class ChatNotificationServiceTest {
                 .getDeclaredField("isAppInForeground")
         field.isAccessible = true
 
+        val atomicBoolean = field.get(null) as java.util.concurrent.atomic.AtomicBoolean
+
         // Set true twice
         ChatNotificationService.setAppForeground(true)
         ChatNotificationService.setAppForeground(true)
-        assertEquals("flag should remain true after consecutive setAppForeground(true)", true, field.getBoolean(null))
+        assertEquals("flag should remain true after consecutive setAppForeground(true)", true, atomicBoolean.get())
 
         // Set false twice
         ChatNotificationService.setAppForeground(false)
@@ -93,7 +89,7 @@ class ChatNotificationServiceTest {
         assertEquals(
             "flag should remain false after consecutive setAppForeground(false)",
             false,
-            field.getBoolean(null),
+            atomicBoolean.get(),
         )
     }
 }