From b0ee4ec75d754a7a81a44f77394403f2c165ee7e Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Mon, 28 Sep 2026 13:13:19 -0400 Subject: [PATCH] fix(chat): refetch the reactors sheet's rows when reactions change ReactorsPrefetchCache.start returned early for any message it had fetched before, and nothing called clear(), so a message's reactor rows were fetched once per ChatViewModel. The title and summary pills read the live reaction overlay, so the sheet could show 4 reactions over 2 rows. An emoji added after the first open was never fetched at all. start now takes the pills and keeps the emoji to count set it fetched for. The same set reuses the cached rows; a different one refetches from the first page, keeping the previous rows up until the new round lands. A round superseded while in flight no longer publishes, and an empty set clears the message's rows. The open sheet collects the message's pills and calls syncReactors on each change, so reactions added or removed while it's up reach the rows too. --- .../flipcash/app/messenger/ChatFlowScreen.kt | 8 ++ .../app/messenger/internal/ChatViewModel.kt | 11 +- .../chat/reactions/ReactorsPrefetchCache.kt | 40 ++++-- .../reactions/ReactorsPrefetchCacheTest.kt | 121 +++++++++++++++++- 4 files changed, 162 insertions(+), 18 deletions(-) diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/ChatFlowScreen.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/ChatFlowScreen.kt index 0af571aa84..856629e16e 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/ChatFlowScreen.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/ChatFlowScreen.kt @@ -391,6 +391,14 @@ private fun FlowReactorsScreen(messageId: Long) { val loading by remember(viewModel, messageId) { viewModel.reactorsLoading(messageId) } .collectAsStateWithLifecycle(initialValue = true) + // OpenReactors already started the fetch; this catches reactions that change while the sheet + // is up. Collected here rather than off `pills` above, whose empty initial value would read as + // "every reaction removed". The first emission is a no-op unless the pills moved since the + // long-press. + LaunchedEffect(viewModel, messageId) { + viewModel.reactionPills(messageId).collect { viewModel.syncReactors(messageId, it) } + } + ReactorsSheet( pills = pills, rows = rows, diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt index 25632759e3..1039309eec 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt @@ -793,6 +793,14 @@ internal class ChatViewModel @Inject constructor( fun loadMoreReactors(messageId: Long) = reactorsPrefetchCache.loadMoreIfNeeded(messageId) + /** + * Refetches [messageId]'s reactors when [pills] no longer match what the cache fetched for — + * called by the open reactors sheet each time its live pills change, so a reaction added or + * removed while it's up (or since it was last opened) shows in the rows, not just the title. + */ + fun syncReactors(messageId: Long, pills: List) = + reactorsPrefetchCache.start(messageId, pills) + /** [messageId]'s pills, live — the reactors sheet's summary row (decision 4: title + pills). */ fun reactionPills(messageId: Long): Flow> = reactionOverlay.map { it[messageId]?.pills.orEmpty() }.distinctUntilChanged() @@ -1952,8 +1960,7 @@ internal class ChatViewModel @Inject constructor( // rather than on the sheet's own composition. eventFlow.filterIsInstance() .onEach { event -> - val emojis = reactionOverlay.value[event.messageId]?.pills?.map { it.emoji }.orEmpty() - reactorsPrefetchCache.start(event.messageId, emojis) + syncReactors(event.messageId, reactionOverlay.value[event.messageId]?.pills.orEmpty()) } .launchIn(viewModelScope) diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCache.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCache.kt index 3490a0c8de..93a8547a5b 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCache.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCache.kt @@ -3,6 +3,7 @@ package com.flipcash.shared.chat.reactions import com.flipcash.services.models.PagingToken import com.flipcash.services.repository.ReactorsPage import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.distinctUntilChanged @@ -30,6 +31,10 @@ class ReactorsPrefetchCache( ) { private val models = mutableMapOf() + + /** The emoji → count each model was started for, so a changed reaction set refetches. */ + private val signatures = mutableMapOf>() + private val jobs = mutableMapOf() private val allRows = MutableStateFlow>>(emptyMap()) private val allLoading = MutableStateFlow>(emptyMap()) @@ -45,14 +50,27 @@ class ReactorsPrefetchCache( fun hasMore(messageId: Long): Boolean = models[messageId]?.hasMore ?: false /** - * Starts the first round of pages for [messageId]'s [emojis] if nothing has started yet. - * No-ops for an empty [emojis] list (nothing reacted, so nothing to fetch) and for a messageId - * already in the cache — a second `OpenReactors` for the same message re-opens the existing - * fetch/rows rather than starting over. + * Starts the first round of pages for [messageId]'s [pills] unless the cache already holds a + * fetch for exactly these pills. An empty [pills] list (nothing reacted, or the last reaction + * was just removed) drops whatever was cached instead of fetching. No-ops for an unchanged + * emoji → count set — a second `OpenReactors` for the same + * reactions re-opens the existing fetch/rows rather than starting over. A changed set (someone + * reacted, un-reacted, or used a new emoji) refetches from the first page: the pages already + * held can't be patched, since the pills say how many reacted but not who. The previous rows + * stay published until the new round lands, so an open sheet doesn't flash placeholders. */ - fun start(messageId: Long, emojis: List) { - if (emojis.isEmpty() || models.containsKey(messageId)) return - models[messageId] = ReactorsListModel(emojis) { emoji, token -> fetchPage(messageId, emoji, token) } + fun start(messageId: Long, pills: List) { + if (pills.isEmpty()) { + clear(messageId) + return + } + val signature = pills.associate { it.emoji to it.count } + if (signatures[messageId] == signature) return + signatures[messageId] = signature + jobs.remove(messageId)?.cancel() + models[messageId] = ReactorsListModel(pills.map { it.emoji }) { emoji, token -> + fetchPage(messageId, emoji, token) + } loadMore(messageId) } @@ -64,10 +82,12 @@ class ReactorsPrefetchCache( } private fun loadMore(messageId: Long) { + val model = models.getValue(messageId) allLoading.update { it + (messageId to true) } - scope.launch { - val model = models.getValue(messageId) + jobs[messageId] = scope.launch { model.loadMoreIfNeeded() + // A start() for newer pills replaced this model while the round was in flight. + if (models[messageId] !== model) return@launch allRows.update { it + (messageId to model.rows) } allLoading.update { it + (messageId to false) } } @@ -76,6 +96,8 @@ class ReactorsPrefetchCache( /** Drops [messageId]'s cached fetch/rows — for a message that leaves the transcript (deleted, etc). */ fun clear(messageId: Long) { models.remove(messageId) + signatures.remove(messageId) + jobs.remove(messageId)?.cancel() allRows.update { it - messageId } allLoading.update { it - messageId } } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCacheTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCacheTest.kt index 9b0ef49832..45a791a93d 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCacheTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/reactions/ReactorsPrefetchCacheTest.kt @@ -4,6 +4,7 @@ import com.flipcash.services.models.PagingToken import com.flipcash.services.models.chat.Reactor import com.flipcash.services.repository.ReactorsPage import com.getcode.opencode.model.core.ID +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest @@ -15,6 +16,9 @@ private fun id(byte: Int): ID = listOf(byte.toByte()) private fun reactor(userId: ID, reactedAt: Instant) = Reactor(userId = userId, reactedAt = reactedAt) +private fun pill(emoji: String, count: Long = 1) = + ReactionPill(emoji = emoji, count = count, selfReacted = false, pending = false) + class ReactorsPrefetchCacheTest { @Test @@ -28,7 +32,7 @@ class ReactorsPrefetchCacheTest { // OpenReactors calls start() synchronously, one step before the navigator pushes the sheet // (see ChatViewModel's OpenReactors handler) — so by the time a sheet would compose and // read rows(), the fetch is already in flight. - cache.start(messageId = 1L, emojis = listOf("👍", "🔥")) + cache.start(messageId = 1L, pills = listOf(pill("👍"), pill("🔥"))) advanceUntilIdle() assertEquals(setOf(1L to "👍", 1L to "🔥"), calls.toSet()) @@ -42,7 +46,7 @@ class ReactorsPrefetchCacheTest { Result.success(ReactorsPage(reactors = listOf(reactor(alice, reactedAt)), hasMore = false)) } - cache.start(messageId = 1L, emojis = listOf("👍")) + cache.start(messageId = 1L, pills = listOf(pill("👍"))) advanceUntilIdle() assertEquals(false, cache.loading(1L).first()) @@ -62,23 +66,23 @@ class ReactorsPrefetchCacheTest { // now still reflects the fetch once it lands, rather than being a stale placeholder. val rowsFlow = cache.rows(1L) - cache.start(messageId = 1L, emojis = listOf("👍")) + cache.start(messageId = 1L, pills = listOf(pill("👍"))) advanceUntilIdle() assertEquals(listOf(alice), rowsFlow.first().map { it.userId }) } @Test - fun `start is idempotent per messageId`() = runTest { + fun `start is idempotent while the pills are unchanged`() = runTest { var callCount = 0 val cache = ReactorsPrefetchCache(scope = this) { _, _, _ -> callCount++ Result.success(ReactorsPage(reactors = emptyList(), hasMore = false)) } - cache.start(messageId = 1L, emojis = listOf("👍")) + cache.start(messageId = 1L, pills = listOf(pill("👍"))) advanceUntilIdle() - cache.start(messageId = 1L, emojis = listOf("👍")) + cache.start(messageId = 1L, pills = listOf(pill("👍"))) advanceUntilIdle() assertEquals(1, callCount) @@ -92,10 +96,113 @@ class ReactorsPrefetchCacheTest { Result.success(ReactorsPage(reactors = emptyList(), hasMore = false)) } - cache.start(messageId = 1L, emojis = emptyList()) + cache.start(messageId = 1L, pills = emptyList()) advanceUntilIdle() assertEquals(0, callCount) assertEquals(emptyList(), cache.rows(1L).first()) } + + @Test + fun `start refetches when a pill's count changes and republishes the new reactors`() = runTest { + val alice = id(2) + val bob = id(3) + var reactors = listOf(reactor(alice, Instant.fromEpochSeconds(1_000))) + val cache = ReactorsPrefetchCache(scope = this) { _, _, _ -> + Result.success(ReactorsPage(reactors = reactors, hasMore = false)) + } + + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 1))) + advanceUntilIdle() + + reactors = reactors + reactor(bob, Instant.fromEpochSeconds(2_000)) + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 2))) + advanceUntilIdle() + + assertEquals(listOf(bob, alice), cache.rows(1L).first().map { it.userId }) + } + + @Test + fun `start fetches an emoji added since the first open`() = runTest { + val calls = mutableListOf() + val cache = ReactorsPrefetchCache(scope = this) { _, emoji, _ -> + calls += emoji + Result.success(ReactorsPage(reactors = emptyList(), hasMore = false)) + } + + cache.start(messageId = 1L, pills = listOf(pill("👍"))) + advanceUntilIdle() + cache.start(messageId = 1L, pills = listOf(pill("👍"), pill("🔥"))) + advanceUntilIdle() + + assertEquals(listOf("👍", "👍", "🔥"), calls) + } + + @Test + fun `a refetch keeps the previous rows until its own round lands`() = runTest { + val alice = id(2) + val gate = CompletableDeferred() + var gated = false + val cache = ReactorsPrefetchCache(scope = this) { _, _, _ -> + if (gated) gate.await() + Result.success(ReactorsPage(reactors = listOf(reactor(alice, Instant.fromEpochSeconds(1_000))), hasMore = false)) + } + + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 1))) + advanceUntilIdle() + + gated = true + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 2))) + advanceUntilIdle() + + // Old rows stay up (no skeleton flash) while the refetch is in flight. + assertEquals(listOf(alice), cache.rows(1L).first().map { it.userId }) + assertEquals(true, cache.loading(1L).first()) + + gate.complete(Unit) + advanceUntilIdle() + assertEquals(false, cache.loading(1L).first()) + } + + @Test + fun `a superseded round does not overwrite the newer one`() = runTest { + val alice = id(2) + val bob = id(3) + val firstGate = CompletableDeferred() + var round = 0 + val cache = ReactorsPrefetchCache(scope = this) { _, _, _ -> + round++ + if (round == 1) { + firstGate.await() + Result.success(ReactorsPage(reactors = listOf(reactor(alice, Instant.fromEpochSeconds(1_000))), hasMore = false)) + } else { + Result.success(ReactorsPage(reactors = listOf(reactor(bob, Instant.fromEpochSeconds(2_000))), hasMore = false)) + } + } + + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 1))) + advanceUntilIdle() + cache.start(messageId = 1L, pills = listOf(pill("👍", count = 2))) + advanceUntilIdle() + firstGate.complete(Unit) + advanceUntilIdle() + + assertEquals(listOf(bob), cache.rows(1L).first().map { it.userId }) + assertEquals(false, cache.loading(1L).first()) + } + + @Test + fun `start with no pills drops the rows of a message whose reactions were all removed`() = runTest { + val alice = id(2) + val cache = ReactorsPrefetchCache(scope = this) { _, _, _ -> + Result.success(ReactorsPage(reactors = listOf(reactor(alice, Instant.fromEpochSeconds(1_000))), hasMore = false)) + } + + cache.start(messageId = 1L, pills = listOf(pill("👍"))) + advanceUntilIdle() + cache.start(messageId = 1L, pills = emptyList()) + advanceUntilIdle() + + assertEquals(emptyList(), cache.rows(1L).first()) + } }