Mark message prefetch requests and let the server turn preloading off

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

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
@@ -132,10 +133,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<String, Int> {
         val fieldMap = HashMap<String, Int>()
 
+        if (prefetch) {
+            fieldMap["prefetch"] = 1
+        }
+
         fieldMap["includeLastKnown"] = if (includeLastKnown) 1 else 0
 
         if (lastKnown != null) {
@@ -225,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
@@ -241,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
@@ -350,15 +363,17 @@ class ChatMessageSyncer @Inject constructor(
                 target = target,
                 fromMessageId = newestMessageIdFromDb,
                 limit = limit,
-                markNotificationsAsRead = false
+                markNotificationsAsRead = false,
+                prefetch = true
             )
         } else {
             initialCatchUp(
                 target = target,
                 limit = limit,
                 lastReadMessage = lastReadMessage,
                 unreadMessages = unreadMessages,
-                markNotificationsAsRead = false
+                markNotificationsAsRead = false,
+                prefetch = true
             )
         }
 
@@ -399,6 +414,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 +429,7 @@ class ChatMessageSyncer @Inject constructor(
                 unreadMessages = unreadMessages,
                 lastCommonRead = lastCommonRead,
                 markNotificationsAsRead = markNotificationsAsRead,
+                prefetch = prefetch,
                 events = events
             )
         } else {
@@ -426,7 +443,8 @@ class ChatMessageSyncer @Inject constructor(
                     limit = limit,
                     threadId = target.threadId,
                     lastCommonRead = lastCommonRead,
-                    markNotificationsAsRead = markNotificationsAsRead
+                    markNotificationsAsRead = markNotificationsAsRead,
+                    prefetch = prefetch
                 ),
                 events
             )
@@ -445,6 +463,7 @@ class ChatMessageSyncer @Inject constructor(
         unreadMessages: Int,
         lastCommonRead: Int?,
         markNotificationsAsRead: Boolean,
+        prefetch: Boolean,
         events: Events
     ): SyncOutcome {
         Log.d(
@@ -462,7 +481,8 @@ class ChatMessageSyncer @Inject constructor(
                 limit = limit,
                 threadId = target.threadId,
                 lastCommonRead = lastCommonRead,
-                markNotificationsAsRead = markNotificationsAsRead
+                markNotificationsAsRead = markNotificationsAsRead,
+                prefetch = prefetch
             ),
             events
         )
@@ -478,6 +498,7 @@ class ChatMessageSyncer @Inject constructor(
             limit = limit,
             lastCommonRead = lastCommonRead,
             markNotificationsAsRead = markNotificationsAsRead,
+            prefetch = prefetch,
             events = events
         )
         return SyncOutcome(
@@ -510,6 +531,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 +549,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 +592,8 @@ class ChatMessageSyncer @Inject constructor(
                 limit = limit,
                 threadId = target.threadId,
                 lastCommonRead = lastCommonRead,
-                markNotificationsAsRead = markNotificationsAsRead
+                markNotificationsAsRead = markNotificationsAsRead,
+                prefetch = prefetch
             ),
             events
         )

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

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"
 }

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 {
@@ -95,11 +96,109 @@ 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<HashMap<String, Int>>()
+            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<HashMap<String, Int>>()
+            verifyBlocking(network) { pullChatMessages(eq(CREDENTIALS), eq(CHAT_URL), fieldMapCaptor.capture()) }
+            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 {
@@ -778,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<String, Any>("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) }
+                }
             }
         )
     }