diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt index a8ebcefbdd..c745734705 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/MessageRow.kt @@ -269,7 +269,9 @@ internal fun MessageRow( // name line. It carries the gutter's width as a leading inset to stay on the // edge it sat on when the column held it. Column(modifier = Modifier.fillMaxWidth()) { - if (sender != null && runStart) { + // A profile can arrive with no display name; drawing it would leave an empty + // line above the run. The avatar below still marks whose run it is. + if (sender != null && runStart && sender.displayName.isNotBlank()) { Text( modifier = Modifier.padding( start = senderNameInset(showsSenderGutter && !item.isFromSelf), diff --git a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/GroupTranscriptScreenshotTest.kt b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/GroupTranscriptScreenshotTest.kt index b532297a90..d675a82540 100644 --- a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/GroupTranscriptScreenshotTest.kt +++ b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/GroupTranscriptScreenshotTest.kt @@ -12,7 +12,12 @@ import androidx.compose.foundation.layout.width import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.draw.alpha +import androidx.compose.ui.semantics.SemanticsProperties +import androidx.compose.ui.semantics.getOrNull +import androidx.compose.ui.test.SemanticsMatcher +import androidx.compose.ui.test.assertCountEquals import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.unit.dp import androidx.paging.PagingData import androidx.paging.compose.LazyPagingItems @@ -105,6 +110,34 @@ class GroupTranscriptScreenshotTest { capture("group_transcript.png") } + /** + * A member whose profile has no display name still gets their face in the gutter, but no name + * line: an empty one would leave a blank gap above the run. + */ + @Test + fun omitsNameLineForBlankDisplayName() { + val nameless = SenderIdentity(userId = listOf(3), displayName = "", picture = null) + val items = listOf( + bubble(3, "Not even close", noah, secondsIn = 20), + bubble(2, "Who's in for Saturday?", nameless, secondsIn = 10), + ChatListItem.DateSeparator(start), + ) + + composeRule.setContent { + FlipcashThemeWrapper().Wrap { + Transcript(flowOf(PagingData.from(items)).collectAsLazyPagingItems()) + } + } + repeat(20) { composeRule.mainClock.advanceTimeByFrame() } + + composeRule.onAllNodesWithText("Noah Parker").assertCountEquals(1) + composeRule.onAllNodes( + SemanticsMatcher("has blank text") { node -> + node.config.getOrNull(SemanticsProperties.Text).orEmpty().any { it.isBlank() } + }, + ).assertCountEquals(0) + } + /** * The unread divider between read and unread messages, first on its own and then sharing its * gap with a day change, where the date sits above it. diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt index 06dd020cb3..2cd77cd633 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt @@ -61,8 +61,11 @@ private fun ChatSummary.formatPreview( // Only a group attributes, and only someone else's message: "You:" already covers the viewer's, // and a DM's counterparty is the row's own name. Null when the roster subset does not have the // sender — the list does not fetch profiles, so an unattributed body is the honest fallback. + // A blank name counts as missing: a cached member with no profile row maps to an empty one, + // which would otherwise read as ": gm". val senderName = if (isGroup && !sentBySelf) { - metadata.members.firstOrNull { it.userId == lastMsg.senderId }?.userProfile?.displayName + metadata.members.firstOrNull { it.userId == lastMsg.senderId } + ?.userProfile?.displayName?.takeIf { it.isNotBlank() } } else { null } diff --git a/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ChatSummaryPreviewTest.kt b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ChatSummaryPreviewTest.kt index 70892c27d2..1ba006deb6 100644 --- a/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ChatSummaryPreviewTest.kt +++ b/apps/flipcash/shared/chat-ui/src/test/kotlin/com/flipcash/shared/chat/ui/ChatSummaryPreviewTest.kt @@ -114,13 +114,14 @@ class ChatSummaryPreviewTest { private fun groupSummary( content: List, senderId: ID?, + otherName: String = "Alice", ) = ChatSummary( metadata = ChatMetadata( chatId = ChatId(byteArrayOf(9)), type = ChatType.GROUP, members = listOf( ChatMember(userId = self, userProfile = profile("Me"), pointers = emptyList()), - ChatMember(userId = other, userProfile = profile("Alice"), pointers = emptyList()), + ChatMember(userId = other, userProfile = profile(otherName), pointers = emptyList()), ), lastMessage = ChatMessage( messageId = 42, @@ -135,8 +136,11 @@ class ChatSummaryPreviewTest { unreadCount = 0, ) - private fun groupReference(content: List, senderId: ID? = other) = - groupSummary(content, senderId) + private fun groupReference( + content: List, + senderId: ID? = other, + otherName: String = "Alice", + ) = groupSummary(content, senderId, otherName) .toConversationReference(selfId = self, tokensByMint = emptyMap(), resources = resources) @Test @@ -208,6 +212,26 @@ class ChatSummaryPreviewTest { ) } + @Test + fun `a member cached without a profile is not prefixed with an empty name`() { + // A member row with no profile row maps to UserProfile.Empty, whose name is "". + assertEquals( + "gm", + groupReference(listOf(MessageContent.Text("gm")), otherName = "").lastMessagePreview, + ) + } + + @Test + fun `group cash from a member cached without a profile previews as the amount alone`() { + val cash = MessageContent.Cash( + intentId = listOf(3), + amount = 25.toFiat(), + mint = Mint.usdf, + ) + + assertEquals("$25.00", groupReference(listOf(cash), otherName = " ").lastMessagePreview) + } + @Test fun `a DM is not prefixed with the counterparty's name`() { assertEquals("gm", preview(listOf(MessageContent.Text("gm")))) diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt index 2cb0403d0a..ec31862713 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt @@ -10,6 +10,7 @@ import com.flipcash.services.controllers.ChatMessagingController import com.flipcash.services.controllers.EventStreamingController import com.flipcash.services.models.chat.ChatId import com.flipcash.services.models.chat.ChatMessage +import com.flipcash.services.models.chat.ChatType import com.flipcash.services.models.chat.ChatUpdate import com.flipcash.services.models.chat.Emoji import com.flipcash.services.models.chat.EmojiReaction @@ -383,10 +384,21 @@ class EventStreamDelegate @Inject constructor( when (metaUpdate) { is MetadataUpdate.FullRefresh -> { metadataDataSource.upsert(metaUpdate.metadata) - memberDataSource.replaceMembers( - metaUpdate.metadata.chatId, - metaUpdate.metadata.members, - ) + // A group's members here are a page of its roster, so replacing would drop + // members the device holds from roster changes and message loads — and with + // them the names its previews and transcript show. Merge, as + // RosterStateHolder.refetch does; departures come through MemberLeft. + if (metaUpdate.metadata.type == ChatType.GROUP) { + memberDataSource.upsert( + metaUpdate.metadata.chatId, + metaUpdate.metadata.members, + ) + } else { + memberDataSource.replaceMembers( + metaUpdate.metadata.chatId, + metaUpdate.metadata.members, + ) + } metaUpdate.metadata.lastMessage?.let { msg -> messageDataSource.upsert(metaUpdate.metadata.chatId, listOf(msg)) } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FullRefreshMembersTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FullRefreshMembersTest.kt new file mode 100644 index 0000000000..44a674844b --- /dev/null +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FullRefreshMembersTest.kt @@ -0,0 +1,147 @@ +package com.flipcash.shared.chat + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.flipcash.app.persistence.FlipcashDatabase +import com.flipcash.app.persistence.sources.ChatMemberDataSource +import com.flipcash.app.persistence.sources.mapper.chat.ChatEntityMapper +import com.flipcash.services.controllers.EventStreamingController +import com.flipcash.services.models.UserProfile +import com.flipcash.services.models.chat.ChatId +import com.flipcash.services.models.chat.ChatMember +import com.flipcash.services.models.chat.ChatMetadata +import com.flipcash.services.models.chat.ChatType +import com.flipcash.services.models.chat.ChatUpdate +import com.flipcash.services.models.chat.MetadataUpdate +import com.flipcash.shared.chat.internal.ChatStateHolder +import com.flipcash.shared.chat.internal.delegates.EventStreamDelegate +import com.getcode.opencode.model.core.ID +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertEquals +import kotlin.time.Instant + +/** + * A `FullRefresh` carries the chat's metadata, and for a group its `members` are only a page of + * the roster. Replacing the cached members with that page deleted everyone else the device held, + * so a group whose last message came from one of them lost the sender's name in the Chats list + * preview. Runs against the real member table, because the bug is in what the write keeps. + */ +@RunWith(RobolectricTestRunner::class) +class FullRefreshMembersTest { + + private val context = ApplicationProvider.getApplicationContext() + private val chatId = ChatId("aabbccdd") + + private val a: ID = listOf(1) + private val b: ID = listOf(2) + private val c: ID = listOf(3) + + private val memberDataSource = ChatMemberDataSource(ChatEntityMapper()) + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + + @Before + fun setUp() { + FlipcashDatabase.init(context, "full-refresh-members-test") + runBlocking { memberDataSource.clear() } + } + + @After + fun tearDown() { + scope.cancel() + FlipcashDatabase.closeDb() + } + + private fun member(id: ID, name: String) = ChatMember( + userId = id, + userProfile = UserProfile( + displayName = name, + socialAccounts = emptyList(), + phoneNumber = null, + email = null, + ), + pointers = emptyList(), + ) + + private fun fullRefresh(type: ChatType, members: List) = ChatUpdate( + chatId = chatId, + metadataUpdates = listOf( + MetadataUpdate.FullRefresh( + ChatMetadata( + chatId = chatId, + type = type, + members = members, + lastMessage = null, + lastActivity = Instant.fromEpochSeconds(1_000), + ), + ), + ), + ) + + /** Streams [update] through the delegate, then waits for [renamedA] to land. */ + private suspend fun apply(update: ChatUpdate, renamedA: String) { + val streaming = mockk(relaxed = true) + every { streaming.isConnected } returns true + every { streaming.chatUpdates } returns flowOf(update) + + EventStreamDelegate( + eventStreamingController = streaming, + messagingController = mockk(relaxed = true), + metadataDataSource = mockk(relaxed = true), + messageDataSource = mockk(relaxed = true), + memberDataSource = memberDataSource, + tokenCoordinator = mockk(relaxed = true), + userManager = mockk(relaxed = true), + stateHolder = ChatStateHolder(), + analytics = mockk(relaxed = true), + exchange = mockk(relaxed = true), + ).apply { + initialize(scope) + open() + } + + // The refresh renames A, and both writes commit A with the rest of the change in one + // transaction — so once A's new name is visible, whatever the write did to B and C is too. + withTimeout(5_000) { + memberDataSource.observeMembers(chatId).first { members -> + members.any { it.userId == a && it.userProfile.displayName == renamedA } + } + } + } + + private suspend fun storedNames(): Map = + memberDataSource.getMembersForChat(chatId).associate { it.userId to it.userProfile.displayName } + + @Test + fun `a group refresh keeps the members its page leaves out`() = runBlocking { + memberDataSource.upsert(chatId, listOf(member(a, "A"), member(b, "B"), member(c, "C"))) + + apply(fullRefresh(ChatType.GROUP, listOf(member(a, "A2"))), renamedA = "A2") + + // C is who the Chats list names when C sent the last message; the row reads the members + // it is handed off this table. + assertEquals(mapOf(a to "A2", b to "B", c to "C"), storedNames()) + } + + @Test + fun `a DM refresh still replaces its members`() = runBlocking { + memberDataSource.upsert(chatId, listOf(member(a, "A"), member(b, "B"))) + + apply(fullRefresh(ChatType.CONTACT_DM, listOf(member(a, "A2"))), renamedA = "A2") + + assertEquals(mapOf(a to "A2"), storedNames()) + } +}