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()) + } }