Skip to content

Commit 519c08e

Browse files
committed
fix(chat): don't skip the initial backlog fetch on a stale lastMessage
loadInitialMessages skipped the network fetch entirely whenever the local chat block already reached conversationModel.lastMessage.id, trusting that value as proof we were caught up with the server. That field is only as fresh as the last room list sync, though: a message sent while the app had the conversation list open but the chat closed (and no push-triggered catch-up ran, e.g. on flavors without FCM) never updates it. Opening the chat then wrongly concluded there was nothing to fetch, leaving the newest message missing until whatever live-update mechanism happened to be active caught up on its own. Always close the backlog from the newest locally known message instead of gating on the conversation's cached lastMessage. closeBacklog is a single cheap request when there is genuinely nothing new, so there is no upside to trusting a value that isn't guaranteed current. Drop ChatMessageSyncer.seedHttpSyncedMessageId with it: it existed only to seed the insurance anchor from that same stale field for the now-removed branch, and has no other caller. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
1 parent 9c9db41 commit 519c08e

3 files changed

Lines changed: 7 additions & 43 deletions

File tree

app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -291,20 +291,11 @@ class ChatMessageSyncer @Inject constructor(
291291

292292
/**
293293
* The newest message id of the conversation/thread confirmed via HTTP sync, or null when no
294-
* sync happened yet (and [seedHttpSyncedMessageId] was never called) since app start.
294+
* sync happened yet since app start.
295295
*/
296296
fun lastHttpSyncedMessageId(internalConversationId: String, threadId: Long?): Long? =
297297
lastHttpSyncedMessageIds[syncStateKey(internalConversationId, threadId)]
298298

299-
/**
300-
* Seeds the HTTP-synced anchor without a fetch. Only call with ids whose coverage is proven by
301-
* HTTP-derived data — e.g. the conversation's lastMessage from the room list sync when the
302-
* local chat block already reaches it. The anchor only ever moves forward.
303-
*/
304-
fun seedHttpSyncedMessageId(internalConversationId: String, threadId: Long?, messageId: Long) {
305-
lastHttpSyncedMessageIds.merge(syncStateKey(internalConversationId, threadId), messageId, ::maxOf)
306-
}
307-
308299
private fun recordHttpSyncedMessageId(target: SyncTarget, messageId: Long) {
309300
lastHttpSyncedMessageIds.merge(syncStateKey(target.internalConversationId, target.threadId), messageId, ::maxOf)
310301
}

app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -188,33 +188,10 @@ class OfflineFirstChatRepository @Inject constructor(
188188
val weAlreadyHaveSomeOfflineMessages = newestMessageIdFromDb > 0
189189

190190
val weHaveAtLeastTheLastReadMessage = newestMessageIdFromDb >= conversationModel.lastReadMessage.toLong()
191-
val lastMessageIdFromServer = conversationModel.lastMessage?.id ?: 0
192-
val weHaveTheLastMessage = newestMessageIdFromDb >= lastMessageIdFromServer
193191
Log.d(TAG, "weAlreadyHaveSomeOfflineMessages:$weAlreadyHaveSomeOfflineMessages")
194192
Log.d(TAG, "weHaveAtLeastTheLastReadMessage:$weHaveAtLeastTheLastReadMessage")
195-
Log.d(TAG, "weHaveTheLastMessage:$weHaveTheLastMessage (lastMessageIdFromServer:$lastMessageIdFromServer)")
196193

197194
when {
198-
weAlreadyHaveSomeOfflineMessages && weHaveTheLastMessage -> {
199-
// The offline messages already reach the conversation's last message (e.g. because
200-
// the room list sync prefetched them), so no initial request is needed at all —
201-
// regardless of the live-update mode. Anything newer is handled by long polling,
202-
// the chat relay or the insurance requests.
203-
Log.d(
204-
TAG,
205-
"Initial online request is skipped because offline messages are up to date" +
206-
" until the conversation's last message"
207-
)
208-
209-
// No HTTP fetch happens in this branch, so seed the insurance anchor explicitly:
210-
// lastMessage came from the room list sync (HTTP) and the local chat block reaches
211-
// it, so it is a valid HTTP-synced anchor. Without the seed, the first insurance
212-
// request of this session would query with lastKnownMessageId=0.
213-
conversationModel.lastMessage?.id?.let {
214-
syncer.seedHttpSyncedMessageId(internalConversationId, threadId, it)
215-
}
216-
}
217-
218195
weAlreadyHaveSomeOfflineMessages && weHaveAtLeastTheLastReadMessage -> {
219196
// Close the backlog since the newest offline message. This is required on
220197
// chat-relay servers (the relay cannot deliver messages that arrived while the
@@ -223,6 +200,12 @@ class OfflineFirstChatRepository @Inject constructor(
223200
// initial load never has to know the live-update mode, i.e. it must not wait for
224201
// the websocket. closeBacklog loops until the backlog is fully closed — a single
225202
// capped fetch could leave a permanent gap on chat-relay servers.
203+
//
204+
// This request is always made, even if the conversation's cached lastMessage
205+
// suggests we are already caught up: that value is only as fresh as the last room
206+
// list sync and can already be behind the server by the time the chat is opened.
207+
// closeBacklog is cheap when there is truly nothing new (a single request coming
208+
// back empty), so there is no good reason to trust the stale value instead.
226209
Log.d(TAG, "Closing the backlog from the newest offline message for initial loading")
227210

228211
syncer.closeBacklog(

app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -402,16 +402,6 @@ class ChatMessageSyncerTest {
402402
assertEquals(42L, syncer.lastHttpSyncedMessageId(INTERNAL_CONVERSATION_ID, null))
403403
}
404404

405-
@Test
406-
fun `seeded insurance anchor only moves forward`() {
407-
assertNull(syncer.lastHttpSyncedMessageId(INTERNAL_CONVERSATION_ID, null))
408-
409-
syncer.seedHttpSyncedMessageId(INTERNAL_CONVERSATION_ID, null, 44L)
410-
syncer.seedHttpSyncedMessageId(INTERNAL_CONVERSATION_ID, null, 10L)
411-
412-
assertEquals(44L, syncer.lastHttpSyncedMessageId(INTERNAL_CONVERSATION_ID, null))
413-
}
414-
415405
@Test
416406
fun `cleanupExpiredMessages trims block boundaries and deletes empty blocks`() =
417407
runTest {

0 commit comments

Comments
 (0)