From bf75bafc4bada1d9831d41c9b597758396e1c039 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 21 Sep 2026 17:36:48 +0200 Subject: [PATCH 1/5] feat(conversations): refresh the conversation list while it is open With the list on screen, a message sent from another device shows up only when a push arrives or the user leaves the screen and comes back. That gap matters most for the people who have no working push at all - no Play Services, no UnifiedPush distributor - for whom nothing else refreshes the list while they are looking at it. Re-sync it every thirty seconds instead, matching the iOS timer. The loop is bound to the resumed lifecycle, so it cannot run behind the screen or outlive it, and it waits before the first sync rather than after: onResume already fetches, and syncing on entering the loop would fetch twice every time the screen is resumed. A tick is dropped while a sync is already in flight, so the loop can never queue behind itself, and while a search is open, where swapping the list out would move what the user is reading. Being offline and battery saver are deliberately not checked here: the first is already handled where the sync would make its request, and the second already stops the expensive half, the message catch-up it triggers. Every tick asks only for what changed, which is what makes a timer this short affordable at all. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../ConversationsListActivity.kt | 29 +++++++++++-- .../data/OfflineConversationsRepository.kt | 15 +++++-- .../OfflineFirstConversationsRepository.kt | 42 ++++++++++--------- .../viewmodels/ConversationsListViewModel.kt | 36 ++++++++++++++++ 4 files changed, 94 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt index 1258f2b857b..368c1c7a2c4 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt @@ -36,8 +36,6 @@ import androidx.lifecycle.lifecycleScope import androidx.lifecycle.repeatOnLifecycle import androidx.work.Data import androidx.work.OneTimeWorkRequest -import com.nextcloud.talk.dagger.modules.assistedViewModels -import com.nextcloud.talk.utils.setExpeditedIfSupported import androidx.work.WorkInfo import androidx.work.WorkManager import autodagger.AutoInjector @@ -62,6 +60,7 @@ import com.nextcloud.talk.conversationlist.ui.ConversationsListScreenCallbacks import com.nextcloud.talk.conversationlist.ui.ConversationsListScreenState import com.nextcloud.talk.conversationlist.viewmodels.ConversationsListViewModel import com.nextcloud.talk.conversationtags.viewmodels.ConversationTagsViewModel +import com.nextcloud.talk.dagger.modules.assistedViewModels import com.nextcloud.talk.data.network.NetworkMonitor import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.events.ConversationsListFetchDataEvent @@ -115,17 +114,20 @@ import com.nextcloud.talk.utils.bundle.BundleKeys.KEY_SCROLL_TO_NOTIFICATION_CAT import com.nextcloud.talk.utils.bundle.BundleKeys.KEY_SHARED_TEXT import com.nextcloud.talk.utils.permissions.PlatformPermissionUtil import com.nextcloud.talk.utils.power.PowerManagerUtils +import com.nextcloud.talk.utils.setExpeditedIfSupported import com.nextcloud.talk.utils.singletons.ApplicationWideCurrentRoomHolder +import java.util.concurrent.TimeUnit +import javax.inject.Inject +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import retrofit2.HttpException -import java.util.concurrent.TimeUnit -import javax.inject.Inject @SuppressLint("StringFormatInvalid") @AutoInjector(NextcloudTalkApplication::class) @@ -215,6 +217,8 @@ class ConversationsListActivity : BaseActivity() { lifecycleScope.launch { userManager.setUserAsActive(currentUser) } } + startForegroundRefreshLoop() + setSupportActionBar(null) forwardMessageState.value = intent.getBooleanExtra(KEY_FORWARD_MSG_FLAG, false) if (savedInstanceState != null) { @@ -714,6 +718,22 @@ class ConversationsListActivity : BaseActivity() { lifecycleScope.launch { snackbarHostState.showSnackbar(text) } } + /** + * Starts a loop that refreshes the conversation list every + * [FOREGROUND_REFRESH_INTERVAL_MILLIS] milliseconds while this screen is resumed, waiting one + * interval before the first refresh. + */ + private fun startForegroundRefreshLoop() { + lifecycleScope.launch { + repeatOnLifecycle(Lifecycle.State.RESUMED) { + while (isActive) { + delay(FOREGROUND_REFRESH_INTERVAL_MILLIS) + currentUser?.let { conversationsListViewModel.refreshRoomsIfIdle(it) } + } + } + } + } + fun fetchRooms(forceFullSync: Boolean = false) { conversationsListViewModel.getRooms(currentUser, forceFullSync) } @@ -1630,6 +1650,7 @@ class ConversationsListActivity : BaseActivity() { addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP) } + private const val FOREGROUND_REFRESH_INTERVAL_MILLIS = 30_000L const val BOTTOM_SHEET_DELAY: Long = 2500 const val SEARCH_DEBOUNCE_INTERVAL_MS = 300 const val HTTP_UNAUTHORIZED = 401 diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index d5aa33dfe38..4f7f4af0003 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -43,10 +43,8 @@ interface OfflineConversationsRepository { * Synchronizes the conversations of [user] with the server (when online). The synced changes * surface through [roomListFlow], which observes the database. * - * The sync asks the server only for what changed since the last one where it can. Set - * [forceFullSync] for the cases where that is not good enough and the whole list is wanted - * back - a pull to refresh, where a conversation the user left elsewhere should be gone by the - * time the indicator stops spinning, rather than within the next five minutes. + * The sync asks the server only for the conversations changed since the last one where it + * can. [forceFullSync] makes it fetch the whole list instead. */ @Deprecated("use observeConversation") fun getRooms(user: User, forceFullSync: Boolean = false): Job @@ -86,6 +84,15 @@ interface OfflineConversationsRepository { */ suspend fun catchUpRooms(user: User, rooms: List) + /** + * Whether a sync for [user] right now would either ask only for the conversations that changed, + * or be the full refresh that falls due every five minutes. + * + * False means the next sync would fetch the whole conversation list without the cadence calling + * for it. + */ + suspend fun isPeriodicSyncDue(user: User): Boolean + /** * Called once onStart to emit a conversation to [conversationFlow] * to be handled asynchronously. diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 28261526d7d..602fc74c12f 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -11,7 +11,6 @@ package com.nextcloud.talk.conversationlist.data.network import android.content.Context import android.database.sqlite.SQLiteConstraintException import android.net.ConnectivityManager -import android.os.PowerManager import android.util.Log import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager import com.nextcloud.talk.chat.data.network.ChatMessageSyncer @@ -23,6 +22,7 @@ import com.nextcloud.talk.data.database.mappers.toDomainModel import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.network.NetworkMonitor import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.extensions.isPowerSaveMode import com.nextcloud.talk.logger.Logger import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.utils.ApiUtils @@ -274,31 +274,38 @@ class OfflineFirstConversationsRepository @Inject constructor( withContext(Dispatchers.IO) { readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) } /** - * The value to send as `modifiedSince`, or null when this sync has to be a full one. + * The stored timestamp to send as `modifiedSince`, or null when the sync has to fetch the whole + * list. * - * A filtered response cannot express a removal, so the server asks clients to refresh in full - * regularly (`docs/conversation.md`): at least every five minutes, and always when the internal - * signaling backend is in use, since there is no signaling server to announce a change out of - * band. On top of that a caller can demand one, for a pull to refresh or an account switch. + * Null is returned when [forceFullSync] is set, when the account uses the internal signaling + * backend, when the last full sync is older than [FULL_SYNC_INTERVAL_MILLIS], and when no + * timestamp is stored. */ private fun modifiedSinceFor(user: User, forceFullSync: Boolean): Long? { val accountId = user.id!! - val lastFullSyncAt = readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) - val fullSyncIsRecent = lastFullSyncAt != null && - System.currentTimeMillis() - lastFullSyncAt in 0 until FULL_SYNC_INTERVAL_MILLIS val usesExternalSignaling = !user.externalSignalingServer?.externalSignalingServer.isNullOrEmpty() - return if (!forceFullSync && usesExternalSignaling && fullSyncIsRecent) { + return if (!forceFullSync && usesExternalSignaling && fullSyncIsRecent(accountId)) { readTimestamp(accountId, KEY_MODIFIED_SINCE) } else { null } } + private fun fullSyncIsRecent(accountId: Long): Boolean { + val lastFullSyncAt = readTimestamp(accountId, KEY_LAST_FULL_SYNC_AT) ?: return false + return System.currentTimeMillis() - lastFullSyncAt in 0 until FULL_SYNC_INTERVAL_MILLIS + } + + override suspend fun isPeriodicSyncDue(user: User): Boolean = + withContext(Dispatchers.IO) { + modifiedSinceFor(user, forceFullSync = false) != null || !fullSyncIsRecent(user.id!!) + } + /** - * Stores what the next sync needs, and only once the response is safely in the database: a - * timestamp kept ahead of a write that then failed would permanently skip the conversations - * that write was carrying. + * Stores the timestamp [roomList] reported for the next request, and, when it was a full + * response, the time of this full sync. Called after the response has been written to the + * database. */ private fun rememberSyncedState(accountId: Long, roomList: RoomListResult) { storeTimestamp(accountId, KEY_MODIFIED_SINCE, roomList.modifiedBefore) @@ -307,7 +314,7 @@ class OfflineFirstConversationsRepository @Inject constructor( } } - /** Null for anything that is not a plausible timestamp, so a bad value asks for a full sync. */ + /** The timestamp stored under [key] for [accountId], or null when none is stored or it is not a positive number. */ private fun readTimestamp(accountId: Long, key: String): Long? = arbitraryStorageManager.getStorageSetting(accountId, key, "") .blockingGet() @@ -396,7 +403,7 @@ class OfflineFirstConversationsRepository @Inject constructor( false } - isPowerSaveMode() -> { + context.isPowerSaveMode() -> { Log.d(TAG, "Battery saver is active, skipping message catch-up") false } @@ -409,11 +416,6 @@ class OfflineFirstConversationsRepository @Inject constructor( else -> true } - private fun isPowerSaveMode(): Boolean { - val powerManager = context.getSystemService(Context.POWER_SERVICE) as PowerManager - return powerManager.isPowerSaveMode - } - private fun isBackgroundDataRestricted(): Boolean { val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager return connectivityManager.isActiveNetworkMetered && diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 8c894ab549e..af634069ddf 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -51,6 +51,7 @@ import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.disposables.Disposable import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -562,6 +563,41 @@ class ConversationsListViewModel @AssistedInject constructor( } } + /** + * Refreshes the conversation list, unless a room list sync is already running, a search is + * open, or the sync would fetch the whole list without the refresh cadence calling for it. + * + * The in-flight check covers the room list request only: [getRooms] completes once the list is + * stored and leaves the message catch-up running, so a tick can start while messages of the + * previous one are still being fetched. That is deliberate - the list is what this refreshes. + * + * Whether a sync is due is read from stored state, and a read that fails must not take the + * refresh loop with it: this runs every tick for as long as the screen is open, and an + * exception would escape the loop's scope rather than skip one refresh. + */ + suspend fun refreshRoomsIfIdle(user: User) { + when { + _isLoadingRooms.value -> + Log.d(TAG, "Foreground refresh skipped: a room list sync is already in flight") + + _isSearchActiveFlow.value -> + Log.d(TAG, "Foreground refresh skipped: a search is active") + + !isPeriodicSyncDue(user) -> + Log.d(TAG, "Foreground refresh skipped: no delta available and no full sync due") + + else -> getRooms(user) + } + } + + /** Whether the cadence calls for a sync, treating a failed read of the stored state as yes. */ + private suspend fun isPeriodicSyncDue(user: User): Boolean = + runCatching { repository.isPeriodicSyncDue(user) }.getOrElse { throwable -> + if (throwable is CancellationException) throw throwable + Log.w(TAG, "Could not read when the last sync was, refreshing anyway", throwable) + true + } + fun getRooms(user: User, forceFullSync: Boolean = false): Job { val startNanoTime = System.nanoTime() Log.d(TAG, "fetchData - getRooms - calling: $startNanoTime") From e1e1546d59b34da6ec94d16506de2a5a55e898f8 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 21 Sep 2026 17:36:49 +0200 Subject: [PATCH 2/5] test(conversations): cover the foreground refresh loop The loop fires whatever else is going on, so what decides whether the list stays usable is when a tick is dropped: one that is not dropped while a sync is running queues requests behind each other, and one that is not dropped during a search swaps the list out from under what the user is reading. The two cases pin each other down. A tick during an in-flight sync must leave the call count at one, and a tick after the previous sync finished must take it to two, so neither dropping every tick nor dropping none of them passes both. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- ...tionsListViewModelForegroundRefreshTest.kt | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt new file mode 100644 index 00000000000..d080852b88c --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt @@ -0,0 +1,178 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Andy Scherzinger + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.conversationlist.viewmodels + +import android.app.Application +import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager +import com.nextcloud.talk.contacts.ContactsRepository +import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.invitation.data.InvitationsRepository +import com.nextcloud.talk.logger.Logger +import com.nextcloud.talk.openconversations.data.OpenConversationsRepository +import com.nextcloud.talk.repositories.conversations.ConversationsRepository +import com.nextcloud.talk.repositories.unifiedsearch.UnifiedSearchRepository +import com.nextcloud.talk.threadsoverview.data.ThreadsRepository +import com.nextcloud.talk.users.UserManager +import kotlinx.coroutines.CompletableJob +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.any +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.mockito.kotlin.wheneverBlocking +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Tests for [ConversationsListViewModel.refreshRoomsIfIdle]: which refresh ticks reach the + * repository and which are dropped. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class ConversationsListViewModelForegroundRefreshTest { + + private val repository: OfflineConversationsRepository = mock() + + private lateinit var viewModel: ConversationsListViewModel + + @Before + fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + wheneverBlocking { repository.isPeriodicSyncDue(any()) }.thenReturn(true) + whenever(repository.roomListFlow(any())).thenReturn(emptyFlow()) + whenever(repository.syncErrorFlow).thenReturn(emptyFlow()) + + viewModel = ConversationsListViewModel( + repository, + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + USER + ) + } + + @After + fun tearDown() { + Dispatchers.resetMain() + } + + @Test + fun `a tick refreshes the list once the previous sync has finished`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(2)).getRooms(eq(USER), eq(false)) + } + + @Test + fun `a tick while a sync is still running is dropped rather than queued`() { + whenever(repository.getRooms(any(), any())).thenReturn(Job()) + viewModel.getRooms(USER) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + } + + @Test + fun `a tick during an open search leaves the list alone`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + viewModel.setIsSearchActive(true) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + } + + @Test + fun `the refresh resumes once the search is closed`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + viewModel.setIsSearchActive(true) + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + viewModel.setIsSearchActive(false) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(2)).getRooms(eq(USER), eq(false)) + } + + @Test + fun `a tick that would have to fetch the whole list is dropped until the cadence is due`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + wheneverBlocking { repository.isPeriodicSyncDue(any()) }.thenReturn(false) + viewModel.getRooms(USER) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + } + + @Test + fun `a tick never asks for a full sync, that is what the cadence and pull to refresh are for`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, never()).getRooms(any(), eq(true)) + } + + @Test + fun `a failed read of the sync state refreshes instead of killing the loop`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + wheneverBlocking { repository.isPeriodicSyncDue(any()) } + .thenThrow(IllegalStateException("storage is gone")) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(2)).getRooms(eq(USER), eq(false)) + } + + private fun completedJob(): CompletableJob = Job().apply { complete() } + + companion object { + private val USER = User( + id = 1, + userId = "me", + username = "me", + token = "app-password", + baseUrl = "https://server.example.com" + ) + } +} From 96ebdc13ae89ce12be5b39a5bb1bede62af7a70c Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Wed, 7 Oct 2026 13:48:31 +0200 Subject: [PATCH 3/5] fix(conversations): keep the loading state until the last room sync ends getRooms cleared the loading flag when its own job finished, even if another sync started meanwhile was still running. With resume, pull to refresh and the foreground refresh tick all starting syncs, they now overlap more often, and two things went wrong when they did: - pull to refresh stopped spinning when the older sync finished, while the forced full sync it started was still running, so a conversation left elsewhere could stay visible after the indicator was gone - the next refresh tick saw no sync in flight and started another one alongside the one still running Count the syncs in flight and clear the flag only when the last one completes. getRooms is also called from a background event subscriber, so the count and the flag update are guarded by a lock. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../viewmodels/ConversationsListViewModel.kt | 32 ++++++++++++++++--- ...tionsListViewModelForegroundRefreshTest.kt | 30 +++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index af634069ddf..4bab0d8f442 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -161,8 +161,17 @@ class ConversationsListViewModel @AssistedInject constructor( private val _isLoadingRooms = MutableStateFlow(true) /** - * True while a [getRooms] call is in progress (from start until the fetch-Job completes, - * i.e. after both the local-DB and the network-sync emissions have been produced). + * Number of [getRooms] jobs that have not completed yet. Guarded by [roomSyncsLock], together + * with the matching update of [_isLoadingRooms]: [getRooms] is also called off the main thread, + * from a background event subscriber. + */ + private var roomSyncsInFlight = 0 + private val roomSyncsLock = Any() + + /** + * True while any [getRooms] call is in progress (from start until the last overlapping + * fetch-Job completes, i.e. after both the local-DB and the network-sync emissions have been + * produced). */ val isLoadingRooms: StateFlow = _isLoadingRooms.asStateFlow() @@ -601,11 +610,24 @@ class ConversationsListViewModel @AssistedInject constructor( fun getRooms(user: User, forceFullSync: Boolean = false): Job { val startNanoTime = System.nanoTime() Log.d(TAG, "fetchData - getRooms - calling: $startNanoTime") - _isLoadingRooms.value = true val job = repository.getRooms(user, forceFullSync) + // overlapping syncs (resume, pull to refresh, a refresh tick) must not end the loading + // state while another one is still running, or pull to refresh stops spinning too early + synchronized(roomSyncsLock) { + roomSyncsInFlight++ + _isLoadingRooms.value = true + } viewModelScope.launch { - job.join() - _isLoadingRooms.value = false + try { + job.join() + } finally { + synchronized(roomSyncsLock) { + roomSyncsInFlight-- + if (roomSyncsInFlight == 0) { + _isLoadingRooms.value = false + } + } + } } return job } diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt index d080852b88c..6a8de9c2a25 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt @@ -30,6 +30,8 @@ import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -106,6 +108,34 @@ class ConversationsListViewModelForegroundRefreshTest { verify(repository, times(1)).getRooms(eq(USER), eq(false)) } + @Test + fun `a tick is dropped while an earlier sync still runs, even after a later one finished`() { + val earlierSync = Job() + whenever(repository.getRooms(any(), any())).thenReturn(earlierSync, completedJob()) + viewModel.getRooms(USER) + viewModel.getRooms(USER, forceFullSync = true) + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + verify(repository, times(1)).getRooms(eq(USER), eq(true)) + } + + @Test + fun `the loading state ends only once the last overlapping sync finishes`() { + val earlierSync = Job() + val laterSync = Job() + whenever(repository.getRooms(any(), any())).thenReturn(earlierSync, laterSync) + viewModel.getRooms(USER) + viewModel.getRooms(USER, forceFullSync = true) + + earlierSync.complete() + assertTrue(viewModel.isLoadingRooms.value) + + laterSync.complete() + assertFalse(viewModel.isLoadingRooms.value) + } + @Test fun `a tick during an open search leaves the list alone`() { whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) From 21e8a8d847f2a834c7795f72209cb55f56266b84 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Wed, 7 Oct 2026 15:37:06 +0200 Subject: [PATCH 4/5] fix(conversations): run room list syncs of one account one at a time Each room list sync reads the stored conversations, merges its response into them and writes the result, and a full sync deletes what its response no longer has. Nothing kept two syncs of the same account from running at once, and they are started from several places: the screen, pull to refresh, the foreground refresh tick and the background worker. Interleaved, they undo each other. A delta sync that is still running when a pull to refresh starts a full sync writes its response after the full sync has deleted a conversation the user left elsewhere, and puts it back until the next full sync. An older response can likewise overwrite newer state. The refresh tick only avoids starting a sync while another one runs; the other callers do not check. Hold a per-account lock for the whole sync, including the choice of modifiedSince, so a waiting sync builds on what the previous one stored. Accounts do not wait for each other. Waiting for the lock counts against the worker's time budget, and a failed read of the stored sync state is now handled as a failed sync instead of escaping the sync's coroutine. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../OfflineFirstConversationsRepository.kt | 28 +++++++- ...OfflineFirstConversationsRepositoryTest.kt | 67 +++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 602fc74c12f..6fec011d58f 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -43,10 +43,13 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.sync.withPermit import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout +import java.util.concurrent.ConcurrentHashMap import javax.inject.Inject import kotlin.collections.map import kotlin.time.Duration.Companion.milliseconds @@ -84,6 +87,9 @@ class OfflineFirstConversationsRepository @Inject constructor( private val scope = CoroutineScope(Dispatchers.IO) + /** One lock per account, held for the whole of a room list sync. See [syncRoomListExclusively]. */ + private val roomListSyncLocks = ConcurrentHashMap() + sealed interface ConversationResult { data class Found(val conversation: ConversationModel) : ConversationResult object NotFound : ConversationResult @@ -192,13 +198,13 @@ class OfflineFirstConversationsRepository @Inject constructor( reportSyncError: Boolean ): List? { val accountId = user.id!! - val modifiedSince = modifiedSinceFor(user, forceFullSync) return try { if (roomListTimeoutMillis == null) { - syncRoomList(user, modifiedSince) + syncRoomListExclusively(user, forceFullSync) } else { - withTimeout(roomListTimeoutMillis.milliseconds) { syncRoomList(user, modifiedSince) } + // waiting for another sync of the account counts against the budget too + withTimeout(roomListTimeoutMillis.milliseconds) { syncRoomListExclusively(user, forceFullSync) } } } catch (e: TimeoutCancellationException) { // the caller's run goes on, so this is a failed sync rather than a cancellation @@ -216,6 +222,22 @@ class OfflineFirstConversationsRepository @Inject constructor( } } + /** + * Runs [syncRoomList] for [user] once no other sync of the same account is running. + * + * Each sync reads the stored conversations, merges its response into them and writes the + * result, and a full sync deletes what its response no longer has. Syncs of one account are + * started from several places (screen, pull to refresh, refresh tick, background worker), so + * they are run one after the other: interleaved, a delta response written after a full sync + * would put back a conversation the full sync had just deleted, and an older response would + * overwrite newer state. The waiting sync also picks its `modifiedSince` from what the previous + * one stored. + */ + private suspend fun syncRoomListExclusively(user: User, forceFullSync: Boolean): List = + roomListSyncLocks.getOrPut(user.id!!) { Mutex() }.withLock { + syncRoomList(user, modifiedSinceFor(user, forceFullSync)) + } + /** * Fetches [user]'s room list, filtered by [modifiedSince] when set, and stores it, returning the * rooms whose messages should be caught up. diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index 5c1a9717bd9..f61f2522884 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -28,6 +28,7 @@ import io.reactivex.Maybe import io.reactivex.Observable import io.reactivex.android.plugins.RxAndroidPlugins import io.reactivex.schedulers.Schedulers +import java.util.concurrent.CountDownLatch import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.awaitCancellation import kotlinx.coroutines.delay @@ -53,6 +54,7 @@ import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.stub +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.verifyBlocking import org.mockito.kotlin.wheneverBlocking @@ -291,6 +293,70 @@ class OfflineFirstConversationsRepositoryTest { } } + @Test + fun `a full sync waits for a running delta sync, so the delta cannot bring back what it deletes`() = + runBlocking { + val staying = conversation(token = "roomA", lastActivity = 5, unreadMessages = 0) + val leaving = conversation(token = "roomB", lastActivity = 5, unreadMessages = 0) + whenever(dao.getConversationsForUser(ACCOUNT_ID)) + .thenReturn(flowOf(listOf(staying.asEntity(ACCOUNT_ID), leaving.asEntity(ACCOUNT_ID)))) + val deltaResponseHeld = CountDownLatch(1) + val deltaResponse = Observable.fromCallable { + deltaResponseHeld.await() + RoomListResult(listOf(staying, leaving), modifiedBefore = null, wasDelta = true) + } + whenever(network.getRooms(any(), any(), any(), anyOrNull())) + .thenReturn(deltaResponse, roomList(listOf(staying))) + + try { + val deltaSync = repository.getRooms(user()) + verify(network, timeout(AWAIT_TIMEOUT_MILLIS).times(1)).getRooms(any(), any(), any(), anyOrNull()) + val fullSync = repository.getRooms(user(), forceFullSync = true) + + verify(network, after(AFTER_DELAY_MILLIS).times(1)).getRooms(any(), any(), any(), anyOrNull()) + + deltaResponseHeld.countDown() + deltaSync.join() + fullSync.join() + } finally { + deltaResponseHeld.countDown() + } + + val deletedIds = argumentCaptor>() + verifyBlocking(dao, times(2)) { + syncConversationsForUser(eq(ACCOUNT_ID), any(), deletedIds.capture()) + } + assertEquals( + listOf(emptyList(), listOf(leaving.asEntity(ACCOUNT_ID).internalId)), + deletedIds.allValues + ) + } + + @Test + fun `a sync of one account does not wait for a running sync of another`() = + runBlocking { + val otherUser = user().copy(id = OTHER_ACCOUNT_ID) + whenever(dao.getConversationsForUser(OTHER_ACCOUNT_ID)).thenReturn(flowOf(emptyList())) + val firstResponseHeld = CountDownLatch(1) + val heldResponse = Observable.fromCallable { + firstResponseHeld.await() + RoomListResult(emptyList(), modifiedBefore = null, wasDelta = false) + } + whenever(network.getRooms(eq(user()), any(), any(), anyOrNull())).thenReturn(heldResponse) + whenever(network.getRooms(eq(otherUser), any(), any(), anyOrNull())).thenReturn(roomList(emptyList())) + + try { + repository.getRooms(user()) + verify(network, timeout(AWAIT_TIMEOUT_MILLIS)).getRooms(eq(user()), any(), any(), anyOrNull()) + + repository.getRooms(otherUser).join() + + verify(network).getRooms(eq(otherUser), any(), any(), anyOrNull()) + } finally { + firstResponseHeld.countDown() + } + } + @Test fun `getRooms merges the server response through the conversation list updater`() = runBlocking { @@ -569,6 +635,7 @@ class OfflineFirstConversationsRepositoryTest { companion object { private const val ACCOUNT_ID = 1L + private const val OTHER_ACCOUNT_ID = 2L private const val BASE_URL = "https://server.example.com" private const val ROOM_TOKEN = "room1" private const val AFTER_DELAY_MILLIS = 300L From 621cac582cb68b24d418f4114977c916dc4a0e4c Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Wed, 7 Oct 2026 15:58:56 +0200 Subject: [PATCH 5/5] fix(conversations): read the refresh cadence before checking for a running sync refreshRoomsIfIdle checked for a running room list sync and an open search first, and only then read from storage whether a sync was due. That read suspends, so a sync or a search started while it ran went unnoticed and the tick refreshed anyway: a redundant request, or a list refreshed under a search the skip was meant to protect. Read the cadence first, so nothing suspends between the checks and the call to getRooms. The read now also runs on ticks that are then skipped, which is one small storage read every thirty seconds. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../viewmodels/ConversationsListViewModel.kt | 6 ++-- ...tionsListViewModelForegroundRefreshTest.kt | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 4bab0d8f442..f8d4303821b 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -582,9 +582,11 @@ class ConversationsListViewModel @AssistedInject constructor( * * Whether a sync is due is read from stored state, and a read that fails must not take the * refresh loop with it: this runs every tick for as long as the screen is open, and an - * exception would escape the loop's scope rather than skip one refresh. + * exception would escape the loop's scope rather than skip one refresh. That read suspends, so + * it is done before the other checks: a sync or a search started while it runs is still seen. */ suspend fun refreshRoomsIfIdle(user: User) { + val syncDue = isPeriodicSyncDue(user) when { _isLoadingRooms.value -> Log.d(TAG, "Foreground refresh skipped: a room list sync is already in flight") @@ -592,7 +594,7 @@ class ConversationsListViewModel @AssistedInject constructor( _isSearchActiveFlow.value -> Log.d(TAG, "Foreground refresh skipped: a search is active") - !isPeriodicSyncDue(user) -> + !syncDue -> Log.d(TAG, "Foreground refresh skipped: no delta available and no full sync due") else -> getRooms(user) diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt index 6a8de9c2a25..5e16bc7341b 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.kt @@ -36,9 +36,11 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.kotlin.any +import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never +import org.mockito.kotlin.stub import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -136,6 +138,39 @@ class ConversationsListViewModelForegroundRefreshTest { assertFalse(viewModel.isLoadingRooms.value) } + @Test + fun `a sync started while the cadence is read still drops the tick`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob(), Job()) + viewModel.getRooms(USER) + repository.stub { + onBlocking { isPeriodicSyncDue(any()) } doSuspendableAnswer { + viewModel.getRooms(USER, forceFullSync = true) + true + } + } + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + verify(repository, times(1)).getRooms(eq(USER), eq(true)) + } + + @Test + fun `a search opened while the cadence is read still drops the tick`() { + whenever(repository.getRooms(any(), any())).thenReturn(completedJob()) + viewModel.getRooms(USER) + repository.stub { + onBlocking { isPeriodicSyncDue(any()) } doSuspendableAnswer { + viewModel.setIsSearchActive(true) + true + } + } + + runBlocking { viewModel.refreshRoomsIfIdle(USER) } + + verify(repository, times(1)).getRooms(eq(USER), eq(false)) + } + @Test fun `a tick during an open search leaves the list alone`() { whenever(repository.getRooms(any(), any())).thenReturn(completedJob())