From efe4989c80e7623cbe5224df125ba7fb0e4eed29 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Wed, 23 Sep 2026 13:58:37 +0200 Subject: [PATCH 1/3] feat(chat): mark message prefetch requests as such The client fetches a room's messages before the user asks for them, from a push notification and after a conversation list sync, and on the wire those requests are indistinguishable from the ones a user waiting on a chat screen is making. A server operator looking at request volume cannot tell what was speculative and what someone was waiting for. Send prefetch=1 on the chat requests the catch-up makes. The server ignores the parameter - the response is byte for byte the one it sends without it - so this only makes the traffic legible in an access log, where a query parameter needs no configuration to be recorded. The flag travels the same route as markNotificationsAsRead, which already separates these two cases, and defaults to false: the helpers that build the request are shared with the chat the user has open, and labelling those as prefetched would invert the very distinction this is for. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../chat/data/network/ChatMessageSyncer.kt | 30 +++++++++--- .../data/network/ChatMessageSyncerTest.kt | 46 +++++++++++++++++++ 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt index bb3bc03ad67..e57e3a227bd 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt @@ -132,10 +132,15 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, threadId: Long? = null, lastCommonRead: Int? = null, - markNotificationsAsRead: Boolean = true + markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false ): HashMap { val fieldMap = HashMap() + if (prefetch) { + fieldMap["prefetch"] = 1 + } + fieldMap["includeLastKnown"] = if (includeLastKnown) 1 else 0 if (lastKnown != null) { @@ -350,7 +355,8 @@ class ChatMessageSyncer @Inject constructor( target = target, fromMessageId = newestMessageIdFromDb, limit = limit, - markNotificationsAsRead = false + markNotificationsAsRead = false, + prefetch = true ) } else { initialCatchUp( @@ -358,7 +364,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, lastReadMessage = lastReadMessage, unreadMessages = unreadMessages, - markNotificationsAsRead = false + markNotificationsAsRead = false, + prefetch = true ) } @@ -399,6 +406,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages: Int = 0, lastCommonRead: Int? = null, markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false, events: Events = NO_EVENTS ): SyncOutcome { val closableBacklogAnchor = lastReadMessage?.takeIf { @@ -413,6 +421,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages = unreadMessages, lastCommonRead = lastCommonRead, markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch, events = events ) } else { @@ -426,7 +435,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) @@ -445,6 +455,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages: Int, lastCommonRead: Int?, markNotificationsAsRead: Boolean, + prefetch: Boolean, events: Events ): SyncOutcome { Log.d( @@ -462,7 +473,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) @@ -478,6 +490,7 @@ class ChatMessageSyncer @Inject constructor( limit = limit, lastCommonRead = lastCommonRead, markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch, events = events ) return SyncOutcome( @@ -510,6 +523,7 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, lastCommonRead: Int? = null, markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false, events: Events = NO_EVENTS ): SyncOutcome { var anchor = fromMessageId @@ -527,7 +541,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ) val roundOutcome = pullAndPersistMessages(target, fieldMap, events) @@ -569,7 +584,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt index 4503d582f9b..5df2da2b6c4 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt @@ -95,11 +95,57 @@ class ChatMessageSyncerTest { ) assertFalse(fieldMap.containsKey("markNotificationsAsRead")) + assertFalse(fieldMap.containsKey("prefetch")) assertFalse(fieldMap.containsKey("lastKnownMessageId")) assertEquals(0, fieldMap["setReadMarker"]) assertEquals(1, fieldMap["includeLastKnown"]) } + @Test + fun `buildFieldMap marks the request as a prefetch when asked`() { + val fieldMap = syncer.buildFieldMap( + lookIntoFuture = true, + timeout = 0, + includeLastKnown = false, + lastKnown = 42, + prefetch = true + ) + + assertEquals(1, fieldMap["prefetch"]) + } + + @Test + fun `catchUpRoom marks its request as a prefetch`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + syncer.catchUpRoom(target()) + + val fieldMapCaptor = argumentCaptor>() + verifyBlocking(network) { pullChatMessages(eq(CREDENTIALS), eq(CHAT_URL), fieldMapCaptor.capture()) } + assertEquals(1, fieldMapCaptor.firstValue["prefetch"]) + } + + @Test + fun `a fetch for a chat the user is reading is not marked as a prefetch`() = + runTest { + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + syncer.tryCloseBacklog(target(), fromMessageId = 42L) + + val fieldMapCaptor = argumentCaptor>() + verifyBlocking(network) { pullChatMessages(eq(CREDENTIALS), eq(CHAT_URL), fieldMapCaptor.capture()) } + assertFalse(fieldMapCaptor.firstValue.containsKey("prefetch")) + } + @Test fun `catchUpRoom skips without chat-keep-notifications capability`() = runTest { From 52092b0b70004f61f2c3cfc792871ce5d3b5bae9 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Wed, 23 Sep 2026 14:04:08 +0200 Subject: [PATCH 2/3] feat(chat): let the server turn message preloading off Preloading a conversation's messages before the user opens it spends the server's bandwidth on traffic nobody asked for yet. An operator who would rather not pay that can now say so: the mobile-preload-chat setting turns it off for mobile clients, and the server reports it in the chat config capabilities. Skip the catch-up when the server reports it as false. Anything else preloads - a server that reports it true, and a server too old to know the setting at all, which cannot mean the operator declined something they were never offered. Both entry points check it. The conversation list skips the whole catch-up rather than asking room by room, and the syncer checks it too, because push notifications reach it without passing the list. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../chat/data/network/ChatMessageSyncer.kt | 38 ++++++----- .../OfflineFirstConversationsRepository.kt | 6 ++ .../nextcloud/talk/utils/CapabilitiesUtil.kt | 12 ++++ .../data/network/ChatMessageSyncerTest.kt | 63 ++++++++++++++++++- 4 files changed, 101 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt index e57e3a227bd..5a9b74eeb4f 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt @@ -21,7 +21,11 @@ import com.nextcloud.talk.data.database.model.ChatMessageEntity import com.nextcloud.talk.data.network.NetworkMonitor import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.json.chat.ChatMessageDto +import com.nextcloud.talk.utils.CapabilitiesUtil import com.nextcloud.talk.utils.SpreedFeatures +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicBoolean +import javax.inject.Inject import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow @@ -30,9 +34,6 @@ import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.sync.Mutex import retrofit2.HttpException -import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.atomic.AtomicBoolean -import javax.inject.Inject /** * The chat message fetch-and-persist core, shared between the chat screen @@ -230,7 +231,8 @@ class ChatMessageSyncer @Inject constructor( * * Requires the chat-keep-notifications capability: without it, a background fetch would * dismiss the user's push notifications for the fetched messages, so the catch-up is skipped - * entirely (same guard as on iOS). + * entirely (same guard as on iOS). It is also skipped when the server turns preloading off + * for mobile clients. * * Reachability is deliberately not pre-checked. Callers are gated by a WorkManager * `NetworkType.CONNECTED` constraint, and a request that fails because the device is offline @@ -246,18 +248,24 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, lastReadMessage: Int? = null, unreadMessages: Int = 0 - ): SyncOutcome { - if (!target.user.hasSpreedFeatureCapability(SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value)) { - Log.d( - TAG, - "Server lacks ${SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value}, " + - "skipping catch-up for ${target.internalConversationId}" - ) - return NOTHING_SYNCED - } + ): SyncOutcome = + when { + !target.user.hasSpreedFeatureCapability(SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value) -> { + Log.d( + TAG, + "Server lacks ${SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value}, " + + "skipping catch-up for ${target.internalConversationId}" + ) + NOTHING_SYNCED + } - return coalescedRoomCatchUp(target, limit, lastReadMessage, unreadMessages) - } + !CapabilitiesUtil.isChatPreloadAllowed(target.user.capabilities?.spreedCapability) -> { + Log.d(TAG, "Server turned off preloading, skipping catch-up for ${target.internalConversationId}") + NOTHING_SYNCED + } + + else -> coalescedRoomCatchUp(target, limit, lastReadMessage, unreadMessages) + } /** * Runs at most one catch-up per room at a time. A request arriving while one is running only 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 dd477782797..b919a17190a 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 @@ -26,6 +26,7 @@ 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 +import com.nextcloud.talk.utils.CapabilitiesUtil import com.nextcloud.talk.utils.CapabilitiesUtil.isUserStatusAvailable import com.nextcloud.talk.utils.SpreedFeatures import com.nextcloud.talk.utils.withRetry @@ -431,6 +432,11 @@ class OfflineFirstConversationsRepository @Inject constructor( false } + !CapabilitiesUtil.isChatPreloadAllowed(user.capabilities?.spreedCapability) -> { + Log.d(TAG, "Server turned off preloading, skipping message catch-up") + false + } + context.isPowerSaveMode() -> { Log.d(TAG, "Battery saver is active, skipping message catch-up") false diff --git a/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt b/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt index e013e828d27..628e4fa9406 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt @@ -301,6 +301,17 @@ object CapabilitiesUtil { return false } + /** + * Whether the server lets mobile clients preload chat messages before the user opens a + * conversation. + * + * True unless the server says `mobile-preload-chat` is false, so a server that does not know + * the setting, does not report it, or has not been asked for its capabilities yet, allows + * preloading. + */ + fun isChatPreloadAllowed(spreedCapabilities: SpreedCapabilityDto?): Boolean = + spreedCapabilities?.config?.get("chat")?.get(MOBILE_PRELOAD_CHAT)?.toString()?.lowercase() != "false" + fun isTranslationsSupported(spreedCapabilities: SpreedCapabilityDto): Boolean = spreedCapabilities.config?.containsKey("chat") == true && spreedCapabilities.config!!["chat"] != null && @@ -418,4 +429,5 @@ object CapabilitiesUtil { private const val SERVER_VERSION_MIN_SUPPORTED = 17 private const val SERVER_VERSION_SUPPORT_WARNING = 26 private const val CONVERSATION_DESCRIPTION_LENGTH_FOR_OLD_SERVER = 500 + private const val MOBILE_PRELOAD_CHAT = "mobile-preload-chat" } diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt index 5df2da2b6c4..7df743cdcce 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt @@ -21,6 +21,8 @@ import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOCS import com.nextcloud.talk.models.json.chat.ChatOverall +import com.nextcloud.talk.utils.CapabilitiesUtil +import java.io.IOException import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf @@ -48,7 +50,6 @@ import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever import org.mockito.kotlin.wheneverBlocking import retrofit2.Response -import java.io.IOException @Suppress("TooManyFunctions", "LargeClass") class ChatMessageSyncerTest { @@ -146,6 +147,58 @@ class ChatMessageSyncerTest { assertFalse(fieldMapCaptor.firstValue.containsKey("prefetch")) } + @Test + fun `catchUpRoom skips when the server turned preloading off`() = + runTest { + val outcome = syncer.catchUpRoom(target(user(preloadChat = false))) + + assertFalse(outcome.persistedNewMessages) + verifyNoInteractions(network) + } + + @Test + fun `catchUpRoom preloads when the server does not report the setting`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + val outcome = syncer.catchUpRoom(target(user(preloadChat = null))) + + assertTrue(outcome.persistedNewMessages) + } + + /** + * Only an explicit false turns preloading off, so a server that says nothing about it allows + * it. Asserted on the capability itself rather than through [ChatMessageSyncer.catchUpRoom], + * which skips a user with no capabilities at the earlier `chat-keep-notifications` guard and + * so never reaches this one. + */ + @Test + fun `a server that says nothing about preloading still allows it`() { + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(null)) + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(SpreedCapabilityDto())) + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(SpreedCapabilityDto().apply { config = hashMapOf() })) + } + + @Test + fun `catchUpRoom preloads when the server allows it explicitly`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + val outcome = syncer.catchUpRoom(target(user(preloadChat = true))) + + assertTrue(outcome.persistedNewMessages) + } + @Test fun `catchUpRoom skips without chat-keep-notifications capability`() = runTest { @@ -824,19 +877,23 @@ class ChatMessageSyncerTest { verifyNoInteractions(chatBlocksDao) } - private fun user(withKeepNotificationsCapability: Boolean = true): User { + private fun user(withKeepNotificationsCapability: Boolean = true, preloadChat: Boolean? = null): User { val features = if (withKeepNotificationsCapability) { listOf("chat-keep-notifications") } else { emptyList() } + val chatConfig = preloadChat?.let { hashMapOf("mobile-preload-chat" to it) } return User( id = ACCOUNT_ID, userId = "me", username = "me", baseUrl = "https://server.example.com", capabilities = CapabilitiesDto().apply { - spreedCapability = SpreedCapabilityDto().apply { this.features = features } + spreedCapability = SpreedCapabilityDto().apply { + this.features = features + this.config = chatConfig?.let { hashMapOf("chat" to it) } + } } ) } From 0930de4e529b5153a4f0a0111111363e04c75aac Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 10 Oct 2026 18:31:08 +0200 Subject: [PATCH 3/3] docs(agents): say how to spot a test that asserts nothing Three tests in recent work passed while never reaching the code they named: an earlier guard returned first, so the assertion was never evaluated. Running the suite cannot distinguish that from a test that works, and review caught all three rather than CI. Write down the two habits that do catch it: check a new test fails without its fix, and take the fixture from the code path while the assertion still comes from the requirement. Drop the maxIssues figure from the detekt entry rather than correcting it. It claimed 80 against a real 114, and two attempts at describing the budget in prose were both wrong about when it fails. The other entries in that section only say where the config lives; this one now does the same and leaves the numbers to detekt.yml, which cannot go stale against itself. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- AGENTS.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 0b88963d90b..cfa2bca394a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,7 +194,7 @@ After finishing code changes, run `./gradlew detekt ktlintCheck` and fix any new ## Static Analysis -- **detekt**: config in `detekt.yml` (maxIssues: 80) +- **detekt**: config in `detekt.yml` - **ktlint**: via `org.jlleitschuh.gradle.ktlint` plugin - **SpotBugs**: filter in `spotbugs-filter.xml`; FindSecBugs and fb-contrib active - **lint**: HTML report at `app/build/reports/lint/lint.html` @@ -204,6 +204,8 @@ After finishing code changes, run `./gradlew detekt ktlintCheck` and fix any new - **Unit tests**: `app/src/test/` — JUnit 4/5, Mockito, Robolectric, MockWebServer. Uses `useJUnitPlatform()`. - **Instrumented tests**: `app/src/androidTest/` — Espresso. Integration tests need real server credentials in `gradle.properties` (`NC_TEST_SERVER_BASEURL`, etc.). - **Room migrations**: if you change the schema, add or update migration tests under `androidTest/data/`. See `data/source/local/TalkDatabase.kt` for migration declarations. +- **A new test must fail without its fix**: a passing test is not evidence that it reached the code it names. Revert the change under test, confirm the test fails, then restore it. Tests have been merged here asserting behaviour they never exercised, because an earlier guard returned before the code under test ran — and running the suite cannot tell you that. +- **Assertions come from the requirement, fixtures come from the code**: derive what the behaviour *should* be from the spec, but derive the setup from the code path, since only the code says which preconditions reach the branch. A fixture written from the description alone often puts the system in a state where the behaviour under test cannot occur at all. - **App startup workers**: `NextcloudTalkApplication.kt` schedules periodic workers (`CapabilitiesWorker`, signaling/WebSocket workers) at startup. Worker scheduling changes can cause subtle startup regressions. ## Commits