From 9f17affb9b842e8d4e49bf5bc8ddc0d0f2eeeab9 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Tue, 3 Nov 2026 19:35:41 -0500 Subject: [PATCH] fix(chat-list): mark read moves READ to the highest stored message id Mark as Read from the chat list left the unread badge up, e.g. 4 dropped to 3. markAsRead took its target from ChatMessageDao.getLatest, which orders by timestamp only. The badge compares the READ pointer by id against the newest visible message, ordered by timestamp then id. When two messages share a millisecond and the older row is the later insert (the feed writes a chat's newest message before the history load fills in older ones), getLatest returns the older one and READ lands short. Both sides read the same local table, so the store is not behind the badge; the two queries disagree on order. Opening the chat works because it advances to the id of a message on screen. The list path now uses MAX(message_id) over positive ids, tombstones included. Excluding negative ids also stops an unsent placeholder from being picked, which used to send the server a negative READ. --- .../internal/delegates/MessagingDelegate.kt | 5 +- .../app/persistence/dao/ChatMessageDao.kt | 18 ++- .../persistence/sources/MarkReadAnchorTest.kt | 108 ++++++++++++++++++ .../sources/ChatMessageDataSource.kt | 3 +- 4 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 apps/flipcash/shared/persistence/sources/src/androidHostTest/kotlin/com/flipcash/app/persistence/sources/MarkReadAnchorTest.kt 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? =