From e32c112cabb81ea51d94bf31c6aa79c597a992e2 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Tue, 27 Oct 2026 11:46:51 -0400 Subject: [PATCH] fix(chat): no reaction alert pill for the open chat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pill showed for a reaction in the open chat when the message was scrolled off screen, labelled "This chat ↓". Drop every alert for the open chat, and close the pill and drop queued pills for a chat when it becomes the open chat. Removes ReactionAlertState.inActiveChat, ReactionAlertAction.ScrollTo, the MessengerScreen collector, the alert_thisChat string and its screenshot variant. --- .../core/src/main/res/values/strings.xml | 1 - .../internal/screens/MessengerScreen.kt | 16 ---- .../shared/chat/ui/ReactionAlertHost.kt | 1 - .../shared/chat/ui/ReactionAlertPill.kt | 7 +- .../ui/ReactionAlertPillScreenshotTest.kt | 6 +- .../chat/alerts/ReactionAlertController.kt | 43 ++++------- .../shared/chat/alerts/ReactionAlertQueue.kt | 7 ++ .../alerts/ReactionAlertControllerTest.kt | 74 +++++++++++++++---- 8 files changed, 88 insertions(+), 67 deletions(-) diff --git a/apps/flipcash/core/src/main/res/values/strings.xml b/apps/flipcash/core/src/main/res/values/strings.xml index 1eac9b3f3e..81b0741459 100644 --- a/apps/flipcash/core/src/main/res/values/strings.xml +++ b/apps/flipcash/core/src/main/res/values/strings.xml @@ -1852,5 +1852,4 @@ %1$s and %2$d others reacted Someone - This chat ↓ diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/MessengerScreen.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/MessengerScreen.kt index 26a5cea943..309c2d26be 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/MessengerScreen.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/MessengerScreen.kt @@ -31,9 +31,6 @@ import com.flipcash.features.messenger.R import androidx.compose.ui.res.stringResource import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.SideEffect -import androidx.compose.runtime.rememberUpdatedState -import com.flipcash.shared.chat.alerts.LocalReactionAlerts -import com.flipcash.shared.chat.alerts.ReactionAlertAction import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -167,19 +164,6 @@ internal fun MessengerScreen(viewModel: ChatViewModel) { if (privateOutsider) flowNavigator.replaceStack(listOf(ChatStep.GroupProfile)) } - // A reaction pill tapped while this chat is open asks to scroll to its message and pulse it. - val reactionAlerts = LocalReactionAlerts.current - val currentChatId by rememberUpdatedState(state.chatId) - LaunchedEffect(reactionAlerts, viewModel) { - reactionAlerts?.actions - ?.filterIsInstance() - ?.collect { action -> - if (action.chatId == currentChatId) { - viewModel.dispatchEvent(ChatViewModel.Event.JumpToMessage(action.messageId)) - } - } - } - // Here rather than in the action handler because the lookup comes first: the tap only knows a // handle, and where it leads is the view model's to decide once the handle has an answer. LaunchedEffect(viewModel) { diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertHost.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertHost.kt index a9534bed48..ba0f239080 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertHost.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertHost.kt @@ -147,7 +147,6 @@ fun ReactionAlertHost( ReactionAlertPill( content = current.content, - inActiveChat = current.inActiveChat, maxWidth = maxWidth - 24.dp, hazeState = hazeState, animate = animate, diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPill.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPill.kt index df09b364c8..6ba369eef8 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPill.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPill.kt @@ -86,7 +86,6 @@ object ReactionAlertPillSpec { @Composable fun ReactionAlertPill( content: ReactionAlertContent, - inActiveChat: Boolean, modifier: Modifier = Modifier, maxWidth: Dp = ReactionAlertPillSpec.MaxWidth, hazeState: HazeState? = null, @@ -111,13 +110,13 @@ fun ReactionAlertPill( animationSpec = tween(if (animate) 150 else 0), label = "alertContent", ) { shown -> - AlertRow(shown, inActiveChat) + AlertRow(shown) } } } @Composable -private fun AlertRow(content: ReactionAlertContent, inActiveChat: Boolean) { +private fun AlertRow(content: ReactionAlertContent) { Row( modifier = Modifier .fillMaxWidth() @@ -135,7 +134,7 @@ private fun AlertRow(content: ReactionAlertContent, inActiveChat: Boolean) { } Column(modifier = Modifier.weight(1f), verticalArrangement = Arrangement.spacedBy(2.dp)) { Text( - text = if (inActiveChat) stringResource(R.string.alert_thisChat) else content.chatName.orEmpty(), + text = content.chatName.orEmpty(), style = CodeTheme.typography.caption, color = CodeTheme.colors.textSecondary, maxLines = 1, diff --git a/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPillScreenshotTest.kt b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPillScreenshotTest.kt index 7c67e9a730..3400a0e80e 100644 --- a/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPillScreenshotTest.kt +++ b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ReactionAlertPillScreenshotTest.kt @@ -53,13 +53,11 @@ class ReactionAlertPillScreenshotTest { "alert_stacked.png", ) - @Test fun inActiveChat() = render(content(Kind.Reaction), "alert_this_chat.png", inActiveChat = true) - - private fun render(content: ReactionAlertContent, fileName: String, inActiveChat: Boolean = false) { + private fun render(content: ReactionAlertContent, fileName: String) { composeRule.mainClock.autoAdvance = false composeRule.setContent { Frame { - ReactionAlertPill(content, inActiveChat, animate = false) + ReactionAlertPill(content, animate = false) } } repeat(10) { composeRule.mainClock.advanceTimeByFrame() } diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertController.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertController.kt index bca5e3abf8..ae31894cfd 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertController.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertController.kt @@ -15,8 +15,6 @@ import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.update -import kotlinx.coroutines.launch import kotlinx.coroutines.launch import javax.inject.Inject import javax.inject.Singleton @@ -33,17 +31,12 @@ fun interface ReactionAlertSink { /** A pill on screen. [generation] bumps when content merges in, so the UI can restart its effects. */ data class ReactionAlertState( val content: ReactionAlertContent, - /** The pill's chat is the one the user has open, so its top row reads "This chat ↓". */ - val inActiveChat: Boolean, val generation: Int, ) /** What a tap on the pill asks for. */ sealed interface ReactionAlertAction { - /** The chat is open: scroll to the message and pulse it. */ - data class ScrollTo(val chatId: ChatId, val messageId: Long) : ReactionAlertAction - - /** Anywhere else: open the chat at the message. */ + /** Open the chat at the message. */ data class OpenChat(val chatId: ChatId, val messageId: Long) : ReactionAlertAction } @@ -52,8 +45,9 @@ sealed interface ReactionAlertAction { * timer. It has its own state rather than extending `SessionToastController`, whose queue writes a * `BillToast(amount, isDeposit)` into the bill state; a pill carries chat names, emoji and merges. * - * The chat screen reports which messages are on screen through [reportOnScreen]. A reaction on one - * of them draws nothing here: the chip on the bubble already pops, and a boost shows its amount. + * The chat screen reports itself as the open chat through [reportOnScreen]. A reaction in that chat + * draws no pill, on screen or not: the pill is for chats the user is not in. Opening a chat also + * closes its pill and drops its queued ones. */ @Singleton class ReactionAlertController internal constructor( @@ -80,7 +74,6 @@ class ReactionAlertController internal constructor( private var generation = 0 private var activeChat: ChatId? = null - private var onScreen: Set = emptySet() private val _state = MutableStateFlow(null) val state: StateFlow = _state.asStateFlow() @@ -88,26 +81,29 @@ class ReactionAlertController internal constructor( private val _actions = MutableSharedFlow(extraBufferCapacity = 4) val actions: SharedFlow = _actions.asSharedFlow() - /** The open chat's visible message ids. Replaces the earlier report for [chatId]. */ + /** + * [chatId] is the open chat. [messageIds], the ids visible in it, no longer affect alerts. When + * the open chat changes, its pill closes and its queued pills are dropped. + */ + @Suppress("UNUSED_PARAMETER") fun reportOnScreen(chatId: ChatId, messageIds: Set) = synchronized(lock) { + if (activeChat == chatId) return@synchronized activeChat = chatId - onScreen = messageIds - refreshActiveChatFlag() + queue.dropPending(chatId) + if (queue.current?.chatId == chatId) close() } /** The chat screen for [chatId] left. A later chat's report is not cleared by an earlier one's exit. */ fun clearOnScreen(chatId: ChatId) = synchronized(lock) { if (activeChat == chatId) { activeChat = null - onScreen = emptySet() - refreshActiveChatFlag() } } override fun offer(event: ReactionAlertEvent) { synchronized(lock) { if (!isForeground()) return - if (event.chatId == activeChat && event.messageId in onScreen) return + if (event.chatId == activeChat) return when (queue.offer(event)) { ReactionAlertQueue.Outcome.Started -> publishAndHold() ReactionAlertQueue.Outcome.Merged -> publishAndHold() @@ -127,10 +123,7 @@ class ReactionAlertController internal constructor( } val chatId = shown.content.chatId val messageId = shown.content.messageId - _actions.tryEmit( - if (shown.inActiveChat) ReactionAlertAction.ScrollTo(chatId, messageId) - else ReactionAlertAction.OpenChat(chatId, messageId), - ) + _actions.tryEmit(ReactionAlertAction.OpenChat(chatId, messageId)) } /** Drops everything, for sign-out. */ @@ -143,7 +136,7 @@ class ReactionAlertController internal constructor( private fun publishAndHold() { val content = queue.current ?: return generation++ - _state.value = ReactionAlertState(content, content.chatId == activeChat, generation) + _state.value = ReactionAlertState(content, generation) holdJob?.cancel() holdJob = scope.after(content.holdMillis) { synchronized(lock) { close() } } } @@ -163,12 +156,6 @@ class ReactionAlertController internal constructor( } } - private fun refreshActiveChatFlag() { - _state.update { shown -> - shown?.copy(inActiveChat = shown.content.chatId == activeChat) - } - } - private fun CoroutineScope.after(millis: Long, block: () -> Unit): Job = launch { delay(millis) block() diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertQueue.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertQueue.kt index e1081faf1e..131fffa94a 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertQueue.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertQueue.kt @@ -112,6 +112,13 @@ class ReactionAlertQueue { return current } + /** Drops the waiting pills for [chatId]. The current pill is left to its owner to close. */ + fun dropPending(chatId: ChatId) { + pending.removeAll { dropped -> + (dropped.chatId == chatId).also { if (it) actors.remove(key(dropped)) } + } + } + /** Drops the current pill and everything waiting. */ fun clear() { current = null diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertControllerTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertControllerTest.kt index 37c64e403d..980abe605c 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertControllerTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/alerts/ReactionAlertControllerTest.kt @@ -1,5 +1,6 @@ package com.flipcash.shared.chat.alerts +import com.flipcash.services.models.chat.ReactionUpdate import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestScope @@ -72,44 +73,91 @@ class ReactionAlertControllerTest { } @Test - fun `off screen message in the open chat shows with this chat label`() = runTest { + fun `off screen message in the open chat draws no pill`() = runTest { val c = controller() c.reportOnScreen(CHAT, setOf(11L)) c.offer(alertEvent(messageId = 10)) runCurrent() - assertEquals(true, c.state.value?.inActiveChat) - c.reset() + assertNull(c.state.value) } @Test - fun `on screen in another chat still shows`() = runTest { + fun `reaction in another chat shows with the chat name`() = runTest { val c = controller() c.reportOnScreen(OTHER_CHAT, setOf(10L)) c.offer(alertEvent(messageId = 10, chatId = CHAT)) runCurrent() assertNotNull(c.state.value) - assertEquals(false, c.state.value?.inActiveChat) + assertEquals(CHAT, c.state.value?.content?.chatId) c.reset() } @Test - fun `tap in the open chat scrolls, elsewhere opens`() = runTest { + fun `no chat open shows the pill`() = runTest { val c = controller() - val seen = mutableListOf() - backgroundScope.launch { c.actions.collect { a -> seen += a } } + c.offer(alertEvent()) runCurrent() + assertNotNull(c.state.value) + c.reset() + } + + @Test + fun `opening a chat closes its showing pill`() = runTest { + val c = controller() c.offer(alertEvent()) runCurrent() - c.onTap() + assertNotNull(c.state.value) + c.reportOnScreen(CHAT, setOf(10L)) + assertNull(c.state.value) + c.reset() + } + + @Test + fun `opening a chat drops its queued pills and keeps others`() = runTest { + val c = controller() + c.offer(alertEvent(messageId = 10, chatId = CHAT)) + c.offer(alertEvent(messageId = 11, chatId = CHAT)) + c.offer(alertEvent(messageId = 12, chatId = OTHER_CHAT)) runCurrent() - assertEquals(listOf(ReactionAlertAction.OpenChat(CHAT, 10)), seen) - c.reportOnScreen(CHAT, setOf(99L)) - c.offer(alertEvent(messageId = 12)) + c.reportOnScreen(CHAT, emptySet()) + assertNull(c.state.value) advanceTimeBy(ReactionAlertController.CLOSE_MILLIS + 1) + assertEquals(OTHER_CHAT, c.state.value?.content?.chatId) + assertEquals(12, c.state.value?.content?.messageId) + c.reset() + } + + @Test + fun `boost in the open chat draws no pill`() = runTest { + val c = controller() + c.reportOnScreen(CHAT, setOf(99L)) + c.offer(alertEvent(messageId = 10, action = ReactionUpdate.Action.BOOSTED, boostDeltaCents = 500)) + runCurrent() + assertNull(c.state.value) + } + + @Test + fun `boost in another chat shows with the amount`() = runTest { + val c = controller() + c.reportOnScreen(OTHER_CHAT, emptySet()) + c.offer(alertEvent(messageId = 10, action = ReactionUpdate.Action.BOOSTED, boostDeltaCents = 500)) + runCurrent() + assertEquals(ReactionAlertContent.Kind.Boost, c.state.value?.content?.kind) + assertEquals(500L, c.state.value?.content?.boostCents) + c.reset() + } + + @Test + fun `tap opens the chat at the message`() = runTest { + val c = controller() + val seen = mutableListOf() + backgroundScope.launch { c.actions.collect { a -> seen += a } } + runCurrent() + c.offer(alertEvent()) runCurrent() c.onTap() runCurrent() - assertEquals(ReactionAlertAction.ScrollTo(CHAT, 12), seen.last()) + assertEquals(listOf(ReactionAlertAction.OpenChat(CHAT, 10)), seen) c.reset() } }