diff --git a/apps/flipcash/shared/chat/src/commonMain/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt b/apps/flipcash/shared/chat/src/commonMain/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt index c1f80c08f3..9c75fe7908 100644 --- a/apps/flipcash/shared/chat/src/commonMain/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt +++ b/apps/flipcash/shared/chat/src/commonMain/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt @@ -675,9 +675,10 @@ class MessagingDelegate @Inject constructor( } override suspend fun markAsRead(chatId: ChatId): Result { - // The stored newest id, tombstones included — the feed's own `lastMessage` skips them so + // The highest stored id, tombstones included — the feed's own `lastMessage` skips them so // the list can preview the last message with content, and reading it here would park the - // pointer below a deleted message and leave the chat permanently unread. + // pointer below a deleted message and leave the chat permanently unread. Highest by id, not + // newest by timestamp: the unread count compares ids, so a timestamp pick lands short. val messageId = messageDataSource.getLatestMessageId(chatId) ?: stateHolder.current.feed ?.firstOrNull { it.chatId == chatId } diff --git a/apps/flipcash/shared/persistence/db/src/commonMain/kotlin/com/flipcash/app/persistence/dao/ChatMessageDao.kt b/apps/flipcash/shared/persistence/db/src/commonMain/kotlin/com/flipcash/app/persistence/dao/ChatMessageDao.kt index 8b624c1a8a..521ba2a761 100644 --- a/apps/flipcash/shared/persistence/db/src/commonMain/kotlin/com/flipcash/app/persistence/dao/ChatMessageDao.kt +++ b/apps/flipcash/shared/persistence/db/src/commonMain/kotlin/com/flipcash/app/persistence/dao/ChatMessageDao.kt @@ -59,6 +59,18 @@ interface ChatMessageDao { @Query("SELECT * FROM chat_messages WHERE chat_id_hex = :chatIdHex ORDER BY timestamp_epoch_ms DESC LIMIT 1") suspend fun getLatest(chatIdHex: String): ChatMessageEntity? + /** + * The highest server-assigned message id stored for [chatIdHex], tombstones and empty rows + * included, or `null` when there is none. Mark as Read moves READ here. + * + * By id, not by timestamp: the unread count compares the READ pointer by id against the newest + * visible message, so an anchor picked by timestamp lands short whenever the two orders disagree + * (two messages in one millisecond, or an unsent placeholder stamped with a device clock that + * runs ahead). Placeholders carry negative ids and are excluded; the server never saw them. + */ + @Query("SELECT MAX(message_id) FROM chat_messages WHERE chat_id_hex = :chatIdHex AND message_id > 0") + suspend fun getNewestMessageId(chatIdHex: String): Long? + /** The lowest message id stored for [chatIdHex], where paging back toward older history resumes. */ @Query("SELECT MIN(message_id) FROM chat_messages WHERE chat_id_hex = :chatIdHex") suspend fun getOldestMessageId(chatIdHex: String): Long? @@ -70,9 +82,9 @@ interface ChatMessageDao { * * This is what the conversation list previews and what its unread check reads, so deleting the * newest message falls the row back to the one before it instead of reading "Message deleted". - * Deliberately separate from [getLatest]: identity-keyed anchors (mark-read, receive buzz) need - * the newest id including tombstones, or a delete would regress the read pointer and leave the - * chat unread forever. + * Deliberately separate from the identity-keyed anchors ([getNewestMessageId] for mark-read, + * [getLatest] for the receive buzz): they need the newest id including tombstones, or a delete + * would regress the read pointer and leave the chat unread forever. */ @Query("SELECT * FROM chat_messages WHERE chat_id_hex = :chatIdHex AND is_deleted = 0 AND content_json IS NOT NULL AND content_json != '[]' AND (encryption_state IS NULL OR encryption_state != 'KEY_PENDING') " + NOT_REQUEST_SHARE + "ORDER BY timestamp_epoch_ms DESC, message_id DESC LIMIT 1") suspend fun getLatestVisible(chatIdHex: String): ChatMessageEntity? diff --git a/apps/flipcash/shared/persistence/sources/src/androidHostTest/kotlin/com/flipcash/app/persistence/sources/MarkReadAnchorTest.kt b/apps/flipcash/shared/persistence/sources/src/androidHostTest/kotlin/com/flipcash/app/persistence/sources/MarkReadAnchorTest.kt new file mode 100644 index 0000000000..6e76d0521d --- /dev/null +++ b/apps/flipcash/shared/persistence/sources/src/androidHostTest/kotlin/com/flipcash/app/persistence/sources/MarkReadAnchorTest.kt @@ -0,0 +1,108 @@ +package com.flipcash.app.persistence.sources + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.flipcash.app.persistence.FlipcashDatabase +import com.flipcash.app.persistence.converters.MessageContentSerialized +import com.flipcash.app.persistence.entities.ChatMessageEntity +import com.flipcash.app.persistence.init +import com.flipcash.app.persistence.sources.mapper.chat.ChatEntityMapper +import com.flipcash.services.models.chat.ChatId +import com.getcode.utils.hexEncodedString +import io.mockk.mockk +import kotlinx.coroutines.test.runTest +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 + +/** + * Mark as Read from the chat list moves READ to [ChatMessageDataSource.getLatestMessageId]. The + * list's unread count compares the pointer by message id against the newest visible message, so + * the anchor has to be the highest stored id. Picking the newest row by timestamp landed short + * whenever timestamps and ids disagreed, and the count dropped instead of clearing. + */ +@RunWith(RobolectricTestRunner::class) +class MarkReadAnchorTest { + + private val mapper = ChatEntityMapper() + private val messages = ChatMessageDataSource(mapper, mockk(relaxed = true), mockk(relaxed = true)) + + private val chatId = ChatId(listOf(0xAB.toByte())) + private val otherId = listOf(4, 5, 6) + + @Before + fun setUp() { + FlipcashDatabase.init(ApplicationProvider.getApplicationContext(), "entropy-a") + } + + @After + fun tearDown() { + FlipcashDatabase.closeDb() + } + + private fun inbound(messageId: Long, timestampEpochMs: Long) = ChatMessageEntity( + chatIdHex = mapper.chatIdHex(chatId), + messageId = messageId, + senderIdHex = otherId.hexEncodedString(), + contentJson = listOf(MessageContentSerialized.Text("m$messageId")), + timestampEpochMs = timestampEpochMs, + unreadSeq = messageId, + ) + + private suspend fun store(vararg rows: ChatMessageEntity) { + FlipcashDatabase.requireInstance().chatMessageDao().upsert(rows.toList()) + } + + @Test + fun `the anchor is the newest message when it shares a millisecond with an older one`() = runTest { + // The feed writes the chat's newest message first; the history load fills in older ones + // afterwards, so the older row is the later insert. + store(inbound(messageId = 5, timestampEpochMs = 4_000)) + store( + inbound(messageId = 1, timestampEpochMs = 1_000), + inbound(messageId = 2, timestampEpochMs = 2_000), + inbound(messageId = 3, timestampEpochMs = 3_000), + inbound(messageId = 4, timestampEpochMs = 4_000), + ) + + assertEquals(5L, messages.getLatestVisible(mapper.chatIdHex(chatId))?.messageId) + assertEquals(5L, messages.getLatestMessageId(chatId)) + } + + @Test + fun `the anchor is the highest id when an older message carries a later timestamp`() = runTest { + store( + inbound(messageId = 1, timestampEpochMs = 1_000), + inbound(messageId = 2, timestampEpochMs = 9_000).copy(isDeleted = true), + inbound(messageId = 3, timestampEpochMs = 3_000), + ) + + assertEquals(3L, messages.getLatestVisible(mapper.chatIdHex(chatId))?.messageId) + assertEquals(3L, messages.getLatestMessageId(chatId)) + } + + @Test + fun `an unsent placeholder is not an anchor`() = runTest { + store( + inbound(messageId = 1, timestampEpochMs = 1_000), + inbound(messageId = 2, timestampEpochMs = 2_000), + // A pending send is stamped with the device clock, which can run ahead of the server's. + inbound(messageId = -3_000, timestampEpochMs = 3_000).copy(senderIdHex = "010203"), + ) + + assertEquals(2L, messages.getLatestMessageId(chatId)) + } + + @Test + fun `a newest tombstone is still the anchor`() = runTest { + store( + inbound(messageId = 1, timestampEpochMs = 1_000), + inbound(messageId = 2, timestampEpochMs = 2_000).copy(isDeleted = true), + ) + + assertEquals(2L, messages.getLatestMessageId(chatId)) + } +} diff --git a/apps/flipcash/shared/persistence/sources/src/commonMain/kotlin/com/flipcash/app/persistence/sources/ChatMessageDataSource.kt b/apps/flipcash/shared/persistence/sources/src/commonMain/kotlin/com/flipcash/app/persistence/sources/ChatMessageDataSource.kt index 730e13bb14..af2310279f 100644 --- a/apps/flipcash/shared/persistence/sources/src/commonMain/kotlin/com/flipcash/app/persistence/sources/ChatMessageDataSource.kt +++ b/apps/flipcash/shared/persistence/sources/src/commonMain/kotlin/com/flipcash/app/persistence/sources/ChatMessageDataSource.kt @@ -158,8 +158,9 @@ class ChatMessageDataSource @Inject constructor( suspend fun hasMessages(chatId: ChatId): Boolean = db?.chatMessageDao()?.getLatest(mapper.chatIdHex(chatId)) != null + /** The highest stored server message id in [chatId]; see [ChatMessageDao.getNewestMessageId]. */ suspend fun getLatestMessageId(chatId: ChatId): Long? = - db?.chatMessageDao()?.getLatest(mapper.chatIdHex(chatId))?.messageId + db?.chatMessageDao()?.getNewestMessageId(mapper.chatIdHex(chatId)) /** The locally-stored copy of a single message, or `null` if this device has never seen it. */ suspend fun getMessage(chatId: ChatId, messageId: Long): ChatMessage? =