Review triggers were lost when the user left the screen immediately

app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt

@@ -111,9 +111,7 @@ class PathActivity : ScreenLockActionBarActivity() {
             binding.pathRowsContainer.layoutParams = params
         }
 
-        lifecycleScope.launch {
-            inAppReviewManager.onEvent(InAppReviewManager.Event.PathScreenVisited)
-        }
+        inAppReviewManager.onEvent(InAppReviewManager.Event.PathScreenVisited)
     }
 
     private fun registerObservers() {

app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsViewModel.kt

@@ -639,9 +639,7 @@ class SettingsViewModel @Inject constructor(
             }
 
             is Commands.OnDonateClicked -> {
-                viewModelScope.launch {
-                    inAppReviewManager.onEvent(InAppReviewManager.Event.DonateButtonClicked)
-                }
+                inAppReviewManager.onEvent(InAppReviewManager.Event.DonateButtonClicked)
                 showUrlDialog(URL_DONATE)
             }
 

app/src/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt

@@ -2,11 +2,9 @@ package org.thoughtcrime.securesms.preferences.appearance
 
 import androidx.annotation.StyleRes
 import androidx.lifecycle.ViewModel
-import androidx.lifecycle.viewModelScope
 import dagger.hilt.android.lifecycle.HiltViewModel
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
-import kotlinx.coroutines.launch
 import org.session.libsession.utilities.TextSecurePreferences
 import org.thoughtcrime.securesms.reviews.InAppReviewManager
 import org.thoughtcrime.securesms.ui.theme.invalidateComposeThemeColors
@@ -30,9 +28,7 @@ class AppearanceSettingsViewModel @Inject constructor(
 
         invalidateComposeThemeColors()
 
-        viewModelScope.launch {
-            inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged)
-        }
+        inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged)
     }
 
     fun setNewStyle(newThemeStyle: String) {
@@ -42,9 +38,7 @@ class AppearanceSettingsViewModel @Inject constructor(
 
         invalidateComposeThemeColors()
 
-        viewModelScope.launch {
-            inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged)
-        }
+        inAppReviewManager.onEvent(InAppReviewManager.Event.ThemeChanged)
     }
 
     fun setNewFollowSystemSettings(followSystemSettings: Boolean) {

app/src/main/java/org/thoughtcrime/securesms/reviews/InAppReviewManager.kt

@@ -70,7 +70,11 @@ class InAppReviewManager @Inject constructor(
         .stateIn(scope, SharingStarted.Eagerly, false)
 
     init {
-        val channel = Channel<Event>()
+        // UNLIMITED, not the default rendezvous. On a rendezvous channel `send` does not complete when
+        // it is called — it suspends until this collector receives — so an event emitted from a screen's
+        // scope is lost if that screen goes away in between. Buffering makes the handoff finish at the
+        // call, which is what lets [onEvent] be non-suspending.
+        val channel = Channel<Event>(capacity = Channel.UNLIMITED)
         eventsChannel = channel
 
         scope.launch {
@@ -143,8 +147,24 @@ class InAppReviewManager @Inject constructor(
         }
     }
 
-    suspend fun onEvent(event: Event) {
-        eventsChannel.send(event)
+    /**
+     * Record something the user did that might earn a review prompt.
+     *
+     * Deliberately NOT suspending, and deliberately not requiring a coroutine at the call site. Every
+     * caller is a screen, and a screen's scope dies when the user leaves it — which is exactly when these
+     * events happen. Changing the theme and pressing back immediately used to lose the event entirely,
+     * because the emission had to outlive the screen that triggered it.
+     *
+     * The channel is UNLIMITED, so this hands off and returns rather than waiting for the collector.
+     */
+    fun onEvent(event: Event) {
+        val result = eventsChannel.trySend(event)
+
+        // Unreachable on an unlimited channel short of the manager being closed, but silence here would
+        // look exactly like the bug this replaced: a trigger that simply never arrives.
+        if (result.isFailure) {
+            Log.w(TAG, "Dropped review event $event: ${result.exceptionOrNull()}")
+        }
     }
 
     enum class Event {

app/src/test/java/org/thoughtcrime/securesms/reviews/InAppReviewManagerTest.kt

@@ -3,6 +3,7 @@ package org.thoughtcrime.securesms.reviews
 import android.content.pm.PackageInfo
 import android.content.pm.PackageManager
 import app.cash.turbine.test
+import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.advanceTimeBy
 import kotlinx.coroutines.test.runTest
@@ -216,6 +217,43 @@ class InAppReviewManagerAppUpdatedOverrideTest {
     }
 }
 
+/**
+ * That an event survives being fired from a caller that does not stick around.
+ *
+ * This is the regression: a theme change emitted from a screen's own scope was lost if the user left the
+ * screen immediately, because the channel was rendezvous and the send did not complete until the collector
+ * received it. The prompt then never appeared — for anyone, not only a test.
+ */
+@RunWith(JUnit4::class)
+class InAppReviewManagerEventDeliveryTest {
+    @get:Rule
+    val mockLoggingRule = MockLoggingRule()
+
+    @Test
+    fun `an event fired before the collector is ready is still delivered`() = runTest {
+        val manager = createManager(isFreshInstall = true)
+
+        // Deliberately no yield between construction and the event: the collector has not started, so this
+        // is the moment a rendezvous channel had nowhere to put it. `first { it }` waits rather than
+        // sampling, so a lost event fails by timing out instead of by reading a stale false.
+        manager.onEvent(InAppReviewManager.Event.ThemeChanged)
+
+        assertTrue(manager.shouldShowPrompt.first { it })
+    }
+
+    @Test
+    fun `onEvent needs no coroutine at the call site`() = runTest {
+        // Compilation is half the assertion: onEvent must stay non-suspending, because every caller is a
+        // screen and wrapping it in that screen's scope is what made the event losable.
+        val manager = createManager(isFreshInstall = true)
+        val fire: () -> Unit = { manager.onEvent(InAppReviewManager.Event.PathScreenVisited) }
+
+        fire()
+
+        assertTrue(manager.shouldShowPrompt.first { it })
+    }
+}
+
 fun TestScope.createManager(
     isFreshInstall: Boolean,
     supportInAppReviewFlow: Boolean = true,