From 98a00c9c8592569cd39341d1040687d5e617bf3b Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 12 Oct 2026 10:49:54 +0000 Subject: [PATCH 1/7] fix(chat): resolve the name, type and size of a file to send off the main thread Sending a file read its display name, mime type and size from the content resolver in uploadFile, before the coroutine that adds the upload placeholder. uploadFile is called from the main thread (attachment preview result, camera, contact, keyboard content), so getType and query ran there. For a file of another app such as Nextcloud Files both are blocking binder calls into its process, and a slow provider stalls the UI per file. Resolve the file info on Dispatchers.IO inside the existing launch that adds the placeholder. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 3 +- .../viewmodels/ChatViewModelUploadFileTest.kt | 181 ++++++++++++++++++ 2 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 918cf62a682..79a90d28546 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2374,8 +2374,9 @@ class ChatViewModel @AssistedInject constructor( require(fileUri.isNotEmpty()) if (!isVoiceMessage) { - val (fileName, mimeType, fileSize) = resolveFileInfo(fileUri) viewModelScope.launch { + // getType/query are blocking binder calls into the provider's process (e.g. Nextcloud Files) + val (fileName, mimeType, fileSize) = withContext(Dispatchers.IO) { resolveFileInfo(fileUri) } chatRepository.addUploadPlaceholderMessage( localFileUri = fileUri, fileName = fileName, diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt new file mode 100644 index 00000000000..56af2ccc06b --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt @@ -0,0 +1,181 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.chat.viewmodels + +import android.content.ContentProvider +import android.content.ContentValues +import android.database.Cursor +import android.database.MatrixCursor +import android.net.Uri +import android.os.Looper +import android.provider.OpenableColumns +import androidx.work.testing.WorkManagerTestInitHelper +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.chat.data.ChatMessageRepository +import com.nextcloud.talk.chat.data.model.ChatMessage +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.utils.ParticipantPermissions +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito +import org.mockito.Mockito.RETURNS_DEEP_STUBS +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger + +/** + * The name, type and size of a file to send come from the provider of the app that owns it (Nextcloud Files): + * a blocking binder call into another process. Sending a file must not make it on the main thread. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class ChatViewModelUploadFileTest { + + private val chatRepository: ChatMessageRepository = mock() + private val placeholderAdded = CountDownLatch(1) + private var previousApplication: NextcloudTalkApplication? = null + + // the setter of the singleton is protected: only the real application sets it + private val sharedApplicationField = NextcloudTalkApplication::class.java + .getDeclaredField("sharedApplication") + .apply { isAccessible = true } + + @Before + fun setUp() { + val context = RuntimeEnvironment.getApplication() + WorkManagerTestInitHelper.initializeTestWorkManager(context) + previousApplication = sharedApplicationField.get(null) as NextcloudTalkApplication? + val application = mock() + whenever(application.contentResolver).thenReturn(context.contentResolver) + whenever(application.applicationContext).thenReturn(context) + sharedApplicationField.set(null, application) + + SlowProvider.reset() + Robolectric.buildContentProvider(SlowProvider::class.java).create(AUTHORITY) + runBlocking { + whenever(chatRepository.addUploadPlaceholderMessage(any(), any(), any(), any(), any(), any())) + .thenAnswer { + placeholderAdded.countDown() + flowOf>() + } + } + } + + @After + fun tearDown() { + sharedApplicationField.set(null, previousApplication) + } + + @Test + fun `sending a file does not ask its provider on the main thread`() { + val user = User().apply { id = 1L } + val viewModel = ChatViewModel( + logger = mock(), + appPreferences = mock(defaultAnswer = RETURNS_DEEP_STUBS), + chatNetworkDataSource = mock(defaultAnswer = RETURNS_DEEP_STUBS), + chatRepository = chatRepository, + threadsRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + conversationRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + reactionsRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + unifiedSearchRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + mediaRecorderManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + audioFocusRequestManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + userManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + appScope = CoroutineScope(Dispatchers.Unconfined), + initialUser = user, + chatRoomToken = "room", + conversationThreadId = null + ) + viewModel.participantPermissions = Mockito.mock(ParticipantPermissions::class.java).also { + whenever(it.hasChatPermission()).thenReturn(true) + } + + viewModel.uploadFile( + fileUri = "content://$AUTHORITY/document.pdf", + isVoiceMessage = false, + displayName = "Team" + ) + + val deadline = System.currentTimeMillis() + TIMEOUT_MS + while (placeholderAdded.count > 0 && System.currentTimeMillis() < deadline) { + shadowOf(Looper.getMainLooper()).idle() + placeholderAdded.await(POLL_MS, TimeUnit.MILLISECONDS) + } + + assertTrue("placeholder message is added", placeholderAdded.count == 0L) + assertEquals("provider calls on the main thread", 0, SlowProvider.mainThreadCalls.get()) + assertTrue("provider is asked for the type and the name", SlowProvider.otherCalls.get() >= 2) + } + + class SlowProvider : ContentProvider() { + override fun onCreate() = true + + override fun getType(uri: Uri): String { + count() + return "application/pdf" + } + + override fun query( + uri: Uri, + projection: Array?, + selection: String?, + selectionArgs: Array?, + sortOrder: String? + ): Cursor { + count() + return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { + addRow(arrayOf(uri.lastPathSegment, FILE_BYTES)) + } + } + + override fun insert(uri: Uri, values: ContentValues?): Uri? = null + + override fun delete(uri: Uri, selection: String?, selectionArgs: Array?) = 0 + + override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 + + private fun count() { + val onMain = Looper.myLooper() == Looper.getMainLooper() + (if (onMain) mainThreadCalls else otherCalls).incrementAndGet() + Thread.sleep(PROVIDER_DELAY_MS) + } + + companion object { + val mainThreadCalls = AtomicInteger() + val otherCalls = AtomicInteger() + + fun reset() { + mainThreadCalls.set(0) + otherCalls.set(0) + } + } + } + + companion object { + private const val AUTHORITY = "com.nextcloud.client.documentsfileprovider" + private const val PROVIDER_DELAY_MS = 300L + private const val FILE_BYTES = 2048L + private const val TIMEOUT_MS = 20_000L + private const val POLL_MS = 50L + } +} From c985918f118fee4422ba3b5d97b56654557c0685 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 12 Oct 2026 15:53:31 +0000 Subject: [PATCH 2/7] fix(chat): keep sending a file when its provider rejects the info query The file info is resolved inside the launch since the previous commit, so an IllegalArgumentException or SecurityException from the provider no longer reached the try around uploadFile and was uncaught in viewModelScope. Fall back to the last path segment so the placeholder is still added and the worker reports the failed upload. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 13 +++++++++- .../viewmodels/ChatViewModelUploadFileTest.kt | 26 ++++++++++++++----- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 79a90d28546..586f8585e66 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2376,7 +2376,18 @@ class ChatViewModel @AssistedInject constructor( if (!isVoiceMessage) { viewModelScope.launch { // getType/query are blocking binder calls into the provider's process (e.g. Nextcloud Files) - val (fileName, mimeType, fileSize) = withContext(Dispatchers.IO) { resolveFileInfo(fileUri) } + val (fileName, mimeType, fileSize) = withContext(Dispatchers.IO) { + try { + resolveFileInfo(fileUri) + } catch (e: IllegalArgumentException) { + // unknown or revoked uri: the worker reports the failed upload, the placeholder stays + Log.w(TAG, "Could not resolve file info of the file to send", e) + Triple(fileUri.toUri().lastPathSegment.orEmpty(), null, 0L) + } catch (e: SecurityException) { + Log.w(TAG, "No access to the file info of the file to send", e) + Triple(fileUri.toUri().lastPathSegment.orEmpty(), null, 0L) + } + } chatRepository.addUploadPlaceholderMessage( localFileUri = fileUri, fileName = fileName, diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt index 56af2ccc06b..ed756bc809f 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt @@ -32,6 +32,7 @@ import org.junit.runner.RunWith import org.mockito.Mockito import org.mockito.Mockito.RETURNS_DEEP_STUBS import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.robolectric.Robolectric @@ -73,7 +74,7 @@ class ChatViewModelUploadFileTest { SlowProvider.reset() Robolectric.buildContentProvider(SlowProvider::class.java).create(AUTHORITY) runBlocking { - whenever(chatRepository.addUploadPlaceholderMessage(any(), any(), any(), any(), any(), any())) + whenever(chatRepository.addUploadPlaceholderMessage(any(), any(), any(), anyOrNull(), any(), any())) .thenAnswer { placeholderAdded.countDown() flowOf>() @@ -88,6 +89,22 @@ class ChatViewModelUploadFileTest { @Test fun `sending a file does not ask its provider on the main thread`() { + sendAndAwaitPlaceholder("document.pdf") + + assertTrue("placeholder message is added", placeholderAdded.count == 0L) + assertEquals("provider calls on the main thread", 0, SlowProvider.mainThreadCalls.get()) + assertTrue("provider is asked for the type and the name", SlowProvider.otherCalls.get() >= 2) + } + + @Test + fun `a provider that rejects the uri still leaves a placeholder and does not crash`() { + sendAndAwaitPlaceholder("rejected.pdf") + + assertTrue("placeholder message is added", placeholderAdded.count == 0L) + assertEquals("provider calls on the main thread", 0, SlowProvider.mainThreadCalls.get()) + } + + private fun sendAndAwaitPlaceholder(name: String) { val user = User().apply { id = 1L } val viewModel = ChatViewModel( logger = mock(), @@ -111,7 +128,7 @@ class ChatViewModelUploadFileTest { } viewModel.uploadFile( - fileUri = "content://$AUTHORITY/document.pdf", + fileUri = "content://" + AUTHORITY + "/" + name, isVoiceMessage = false, displayName = "Team" ) @@ -121,10 +138,6 @@ class ChatViewModelUploadFileTest { shadowOf(Looper.getMainLooper()).idle() placeholderAdded.await(POLL_MS, TimeUnit.MILLISECONDS) } - - assertTrue("placeholder message is added", placeholderAdded.count == 0L) - assertEquals("provider calls on the main thread", 0, SlowProvider.mainThreadCalls.get()) - assertTrue("provider is asked for the type and the name", SlowProvider.otherCalls.get() >= 2) } class SlowProvider : ContentProvider() { @@ -132,6 +145,7 @@ class ChatViewModelUploadFileTest { override fun getType(uri: Uri): String { count() + require(uri.lastPathSegment != "rejected.pdf") { "Unknown URL " + uri } return "application/pdf" } From b2bb61a04dbf85f1116e5ae1b44e827967fed275 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 12 Oct 2026 16:09:18 +0000 Subject: [PATCH 3/7] fix(chat): enqueue the upload worker after the placeholder message exists The info lookup now runs asynchronously, so a fast worker failure could call updatePlaceholderStatus before the placeholder was inserted and the later placeholder stayed pending. Voice messages keep the immediate enqueue. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 44 ++++++++++--------- .../viewmodels/ChatViewModelUploadFileTest.kt | 23 ++++++++++ 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 586f8585e66..39c80cdc12b 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2373,7 +2373,24 @@ class ChatViewModel @AssistedInject constructor( try { require(fileUri.isNotEmpty()) - if (!isVoiceMessage) { + val internalConversationId = "${currentUser.id}@$chatRoomToken" + val enqueueUpload = { + UploadAndShareFilesWorker.upload( + userId = currentUser.id!!, + fileUri = fileUri, + roomToken = room, + conversationName = displayName, + metaData = metaData, + referenceId = referenceId, + internalConversationId = internalConversationId, + compressImages = compressImages, + allowUpdate = allowUpdate + ) + } + + if (isVoiceMessage) { + enqueueUpload() + } else { viewModelScope.launch { // getType/query are blocking binder calls into the provider's process (e.g. Nextcloud Files) val (fileName, mimeType, fileSize) = withContext(Dispatchers.IO) { @@ -2396,26 +2413,13 @@ class ChatViewModel @AssistedInject constructor( fileSize = fileSize, referenceId = referenceId ).collect {} - } - } - val internalConversationId = "${currentUser.id}@$chatRoomToken" - val workerId = UploadAndShareFilesWorker.upload( - userId = currentUser.id!!, - fileUri = fileUri, - roomToken = room, - conversationName = displayName, - metaData = metaData, - referenceId = referenceId, - internalConversationId = internalConversationId, - compressImages = compressImages, - allowUpdate = allowUpdate - ) - - if (!isVoiceMessage) { - uploadReferenceToWorkId[referenceId] = workerId - _uploadedLocalPreviewMap.update { it + (referenceId to fileUri) } - observeUploadProgress(workerId, referenceId) + // the placeholder has to exist before the worker can report a failure for it + val workerId = enqueueUpload() + uploadReferenceToWorkId[referenceId] = workerId + _uploadedLocalPreviewMap.update { it + (referenceId to fileUri) } + observeUploadProgress(workerId, referenceId) + } } } catch (e: IllegalArgumentException) { Log.e(javaClass.simpleName, "Something went wrong when trying to upload file", e) diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt index ed756bc809f..2644b1565b8 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt @@ -13,6 +13,7 @@ import android.database.MatrixCursor import android.net.Uri import android.os.Looper import android.provider.OpenableColumns +import androidx.work.WorkManager import androidx.work.testing.WorkManagerTestInitHelper import com.nextcloud.talk.application.NextcloudTalkApplication import com.nextcloud.talk.chat.data.ChatMessageRepository @@ -54,6 +55,7 @@ class ChatViewModelUploadFileTest { private val chatRepository: ChatMessageRepository = mock() private val placeholderAdded = CountDownLatch(1) + private var queuedWhenPlaceholderAdded = -1 private var previousApplication: NextcloudTalkApplication? = null // the setter of the singleton is protected: only the real application sets it @@ -76,6 +78,7 @@ class ChatViewModelUploadFileTest { runBlocking { whenever(chatRepository.addUploadPlaceholderMessage(any(), any(), any(), anyOrNull(), any(), any())) .thenAnswer { + queuedWhenPlaceholderAdded = queuedUploads() placeholderAdded.countDown() flowOf>() } @@ -104,6 +107,26 @@ class ChatViewModelUploadFileTest { assertEquals("provider calls on the main thread", 0, SlowProvider.mainThreadCalls.get()) } + @Test + fun `the upload worker is enqueued only after the placeholder message exists`() { + sendAndAwaitPlaceholder("document.pdf") + + val deadline = System.currentTimeMillis() + TIMEOUT_MS + while (queuedUploads() == 0 && System.currentTimeMillis() < deadline) { + shadowOf(Looper.getMainLooper()).idle() + Thread.sleep(POLL_MS) + } + + assertEquals("uploads queued when the placeholder was added", 0, queuedWhenPlaceholderAdded) + assertEquals("the upload was enqueued afterwards", 1, queuedUploads()) + } + + private fun queuedUploads(): Int = + WorkManager.getInstance(RuntimeEnvironment.getApplication()) + .getWorkInfosForUniqueWork("upload_queue_1@room") + .get() + .size + private fun sendAndAwaitPlaceholder(name: String) { val user = User().apply { id = 1L } val viewModel = ChatViewModel( From 21b1f55864eb82d4eeab7ad49fbf30864b41f7a1 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 12 Oct 2026 16:28:47 +0000 Subject: [PATCH 4/7] fix(chat): enqueue the upload worker at once and mark a failed placeholder afterwards Moving the worker behind the file info lookup lost the upload when the view model was cleared meanwhile, and let files with providers of different speed enter the chained upload queue out of order. The worker is enqueued synchronously again, in the order of the calls. The progress observer now starts after the placeholder exists and marks it failed when the worker has already failed, so a fast failure no longer leaves the placeholder pending. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/data/ChatMessageRepository.kt | 6 + .../network/OfflineFirstChatRepository.kt | 7 ++ .../talk/chat/viewmodels/ChatViewModel.kt | 20 ++-- .../viewmodels/ChatViewModelUploadFileTest.kt | 106 +++++++++++++++--- 4 files changed, 114 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt index fb9ab780313..ebe226929e9 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt @@ -169,6 +169,12 @@ interface ChatMessageRepository : LifecycleAwareManager { */ suspend fun deleteTempMessageByReferenceId(referenceId: String): Boolean + /** + * Marks the upload placeholder for [referenceId] as failed, but only while it's still PENDING. + * Does nothing if there is no such placeholder (yet). + */ + suspend fun markUploadPlaceholderFailed(referenceId: String) + /** * Writes the new text into the local database before the server is asked, so the chat renders the * edit right away, and restores the previous text if the request fails or the server rejects the diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 5b871c2ce3e..e0f15a1e8da 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -803,6 +803,13 @@ class OfflineFirstChatRepository @Inject constructor( override suspend fun deleteTempMessageByReferenceId(referenceId: String): Boolean = chatDao.deleteTempChatMessageIfPending(internalConversationId, referenceId) > 0 + override suspend fun markUploadPlaceholderFailed(referenceId: String) { + val entity = chatDao.getTempMessageForConversation(internalConversationId, referenceId, null).firstOrNull() + if (entity?.sendStatus == SendStatus.PENDING) { + chatDao.updateChatMessage(entity.copy(sendStatus = SendStatus.FAILED)) + } + } + override suspend fun editChatMessage( credentials: String, url: String, diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 39c80cdc12b..ebd0fa8f9ec 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -46,6 +46,7 @@ import com.nextcloud.talk.jobs.ReadMarkerSyncWorker import com.nextcloud.talk.jobs.SendMessageWorker import com.nextcloud.talk.jobs.ShareOperationWorker import androidx.lifecycle.asFlow +import androidx.work.WorkInfo import androidx.work.WorkManager import com.nextcloud.talk.jobs.UploadAndShareFilesWorker import com.nextcloud.talk.logger.Logger @@ -2388,9 +2389,13 @@ class ChatViewModel @AssistedInject constructor( ) } - if (isVoiceMessage) { - enqueueUpload() - } else { + // enqueued right here, in the order of the calls: the queue of a conversation is chained, and the + // slow lookup of the file info below must neither reorder the files nor die with the view model + val workerId = enqueueUpload() + if (!isVoiceMessage) { + uploadReferenceToWorkId[referenceId] = workerId + _uploadedLocalPreviewMap.update { it + (referenceId to fileUri) } + viewModelScope.launch { // getType/query are blocking binder calls into the provider's process (e.g. Nextcloud Files) val (fileName, mimeType, fileSize) = withContext(Dispatchers.IO) { @@ -2414,10 +2419,8 @@ class ChatViewModel @AssistedInject constructor( referenceId = referenceId ).collect {} - // the placeholder has to exist before the worker can report a failure for it - val workerId = enqueueUpload() - uploadReferenceToWorkId[referenceId] = workerId - _uploadedLocalPreviewMap.update { it + (referenceId to fileUri) } + // observed only now: a failure the worker reported before the placeholder existed is + // delivered with the current state and marks the placeholder that was just added observeUploadProgress(workerId, referenceId) } } @@ -2452,6 +2455,9 @@ class ChatViewModel @AssistedInject constructor( if (progress >= 0) { _uploadProgressMap.update { it + (referenceId to progress) } } + if (workInfo.state == WorkInfo.State.FAILED) { + viewModelScope.launch { chatRepository.markUploadPlaceholderFailed(referenceId) } + } if (workInfo.state.isFinished) { _uploadProgressMap.update { it - referenceId } uploadReferenceToWorkId.remove(referenceId) diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt index 2644b1565b8..ff0aea25318 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt @@ -13,15 +13,19 @@ import android.database.MatrixCursor import android.net.Uri import android.os.Looper import android.provider.OpenableColumns +import androidx.lifecycle.viewModelScope import androidx.work.WorkManager +import androidx.work.impl.WorkManagerImpl import androidx.work.testing.WorkManagerTestInitHelper import com.nextcloud.talk.application.NextcloudTalkApplication import com.nextcloud.talk.chat.data.ChatMessageRepository import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.utils.ParticipantPermissions +import com.nextcloud.talk.utils.message.SendMessageUtils import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.runBlocking import org.junit.After @@ -41,6 +45,7 @@ import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import org.robolectric.Shadows.shadowOf import org.robolectric.annotation.Config +import java.util.UUID import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger @@ -55,7 +60,7 @@ class ChatViewModelUploadFileTest { private val chatRepository: ChatMessageRepository = mock() private val placeholderAdded = CountDownLatch(1) - private var queuedWhenPlaceholderAdded = -1 + private val placeholderMarkedFailed = CountDownLatch(1) private var previousApplication: NextcloudTalkApplication? = null // the setter of the singleton is protected: only the real application sets it @@ -78,10 +83,12 @@ class ChatViewModelUploadFileTest { runBlocking { whenever(chatRepository.addUploadPlaceholderMessage(any(), any(), any(), anyOrNull(), any(), any())) .thenAnswer { - queuedWhenPlaceholderAdded = queuedUploads() placeholderAdded.countDown() flowOf>() } + whenever(chatRepository.markUploadPlaceholderFailed(any())).thenAnswer { + placeholderMarkedFailed.countDown() + } } } @@ -108,17 +115,61 @@ class ChatViewModelUploadFileTest { } @Test - fun `the upload worker is enqueued only after the placeholder message exists`() { - sendAndAwaitPlaceholder("document.pdf") + fun `the upload worker is enqueued at once, before the provider has answered`() { + val viewModel = newViewModel() + + viewModel.send("slow-a.pdf", order = 1) + + assertEquals("uploads queued while the provider is still asked", 1, queuedUploads()) + assertEquals("placeholder is not added yet", 1L, placeholderAdded.count) + } + + @Test + fun `a view model cleared while the provider answers still has its upload enqueued`() { + val viewModel = newViewModel() + + viewModel.send("slow-a.pdf", order = 1) + viewModel.viewModelScope.cancel() + + assertEquals("uploads queued", 1, queuedUploads()) + } + + @Test + fun `files are enqueued in the order they were sent whatever speed their providers answer with`() { + val viewModel = newViewModel() + + viewModel.send("slow-a.pdf", order = 1) + viewModel.send("b.pdf", order = 2) + + val workIds = viewModel.uploadWorkIds() + val first = workIds.getValue(referenceId(1)) + val second = workIds.getValue(referenceId(2)) + val dependents = (WorkManager.getInstance(RuntimeEnvironment.getApplication()) as WorkManagerImpl) + .workDatabase + .dependencyDao() + .getDependentWorkIds(first.toString()) + assertEquals("the second file waits for the first one", listOf(second.toString()), dependents) + } + + @Test + fun `a worker that fails before the placeholder exists does not leave the placeholder pending`() { + val viewModel = newViewModel() + + viewModel.send("slow-a.pdf", order = 1) + val workId = viewModel.uploadWorkIds().getValue(referenceId(1)) + WorkManagerTestInitHelper.getTestDriver(RuntimeEnvironment.getApplication())!!.setAllConstraintsMet(workId) val deadline = System.currentTimeMillis() + TIMEOUT_MS - while (queuedUploads() == 0 && System.currentTimeMillis() < deadline) { + while (placeholderAdded.count > 0 && System.currentTimeMillis() < deadline) { shadowOf(Looper.getMainLooper()).idle() - Thread.sleep(POLL_MS) + placeholderAdded.await(POLL_MS, TimeUnit.MILLISECONDS) + } + while (placeholderMarkedFailed.count > 0 && System.currentTimeMillis() < deadline) { + shadowOf(Looper.getMainLooper()).idle() + placeholderMarkedFailed.await(POLL_MS, TimeUnit.MILLISECONDS) } - assertEquals("uploads queued when the placeholder was added", 0, queuedWhenPlaceholderAdded) - assertEquals("the upload was enqueued afterwards", 1, queuedUploads()) + assertEquals("the placeholder is marked as failed", 0L, placeholderMarkedFailed.count) } private fun queuedUploads(): Int = @@ -127,7 +178,25 @@ class ChatViewModelUploadFileTest { .get() .size - private fun sendAndAwaitPlaceholder(name: String) { + private fun referenceId(order: Int) = SendMessageUtils().generateGroupedReferenceId(UPLOAD_ID, order) + + @Suppress("UNCHECKED_CAST") + private fun ChatViewModel.uploadWorkIds(): Map = + ChatViewModel::class.java.getDeclaredField("uploadReferenceToWorkId") + .apply { isAccessible = true } + .get(this) as Map + + private fun ChatViewModel.send(name: String, order: Int) { + uploadFile( + fileUri = "content://" + AUTHORITY + "/" + name, + isVoiceMessage = false, + displayName = "Team", + uploadId = UPLOAD_ID, + order = order + ) + } + + private fun newViewModel(): ChatViewModel { val user = User().apply { id = 1L } val viewModel = ChatViewModel( logger = mock(), @@ -149,12 +218,11 @@ class ChatViewModelUploadFileTest { viewModel.participantPermissions = Mockito.mock(ParticipantPermissions::class.java).also { whenever(it.hasChatPermission()).thenReturn(true) } + return viewModel + } - viewModel.uploadFile( - fileUri = "content://" + AUTHORITY + "/" + name, - isVoiceMessage = false, - displayName = "Team" - ) + private fun sendAndAwaitPlaceholder(name: String) { + newViewModel().send(name, order = 1) val deadline = System.currentTimeMillis() + TIMEOUT_MS while (placeholderAdded.count > 0 && System.currentTimeMillis() < deadline) { @@ -167,7 +235,7 @@ class ChatViewModelUploadFileTest { override fun onCreate() = true override fun getType(uri: Uri): String { - count() + count(uri) require(uri.lastPathSegment != "rejected.pdf") { "Unknown URL " + uri } return "application/pdf" } @@ -179,7 +247,7 @@ class ChatViewModelUploadFileTest { selectionArgs: Array?, sortOrder: String? ): Cursor { - count() + count(uri) return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { addRow(arrayOf(uri.lastPathSegment, FILE_BYTES)) } @@ -191,10 +259,10 @@ class ChatViewModelUploadFileTest { override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 - private fun count() { + private fun count(uri: Uri) { val onMain = Looper.myLooper() == Looper.getMainLooper() (if (onMain) mainThreadCalls else otherCalls).incrementAndGet() - Thread.sleep(PROVIDER_DELAY_MS) + Thread.sleep(if (uri.lastPathSegment.orEmpty().startsWith("slow")) SLOW_DELAY_MS else PROVIDER_DELAY_MS) } companion object { @@ -211,8 +279,10 @@ class ChatViewModelUploadFileTest { companion object { private const val AUTHORITY = "com.nextcloud.client.documentsfileprovider" private const val PROVIDER_DELAY_MS = 300L + private const val SLOW_DELAY_MS = 1500L private const val FILE_BYTES = 2048L private const val TIMEOUT_MS = 20_000L private const val POLL_MS = 50L + private const val UPLOAD_ID = "upload" } } From 11a7256b0eaa9bcd002b9c0b44c7316b9c2727e8 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Thu, 15 Oct 2026 06:21:04 +0000 Subject: [PATCH 5/7] fix(chat): skip the upload placeholder when its server copy is already stored The upload worker is enqueued before the placeholder is inserted. If a sync stores the server copy first, upsertChatMessagesAndDeleteTemp has nothing to delete, and the placeholder inserted afterwards stays PENDING next to the real message. Insert the placeholder through a DAO transaction that skips it when a non-temporary message with the same referenceId exists in the conversation. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-sonnet-5-5 --- .../network/OfflineFirstChatRepository.kt | 7 +- .../talk/data/database/dao/ChatMessagesDao.kt | 31 +++ .../utils/preview/ComposePreviewUtilsDaos.kt | 3 + ...irstChatRepositoryUploadPlaceholderTest.kt | 226 ++++++++++++++++++ 4 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index e0f15a1e8da..ebc55b3435b 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -793,7 +793,12 @@ class OfflineFirstChatRepository @Inject constructor( sendStatus = SendStatus.PENDING, silent = false ) - chatDao.upsertChatMessage(entity) + // The worker is enqueued before this runs, so a sync may already have stored the server copy. + // upsertChatMessagesAndDeleteTemp removed temp messages only at that time, so a placeholder + // stored now would stay pending next to the real message. + if (!chatDao.upsertTempChatMessageUnlessServerCopyExists(entity)) { + Log.d(TAG, "addUploadPlaceholderMessage: skipped, server copy of $referenceId already stored") + } } catch (e: Exception) { Log.e(TAG, "addUploadPlaceholderMessage failed for referenceId=$referenceId", e) emit(Result.failure(e)) diff --git a/app/src/main/java/com/nextcloud/talk/data/database/dao/ChatMessagesDao.kt b/app/src/main/java/com/nextcloud/talk/data/database/dao/ChatMessagesDao.kt index 3bdae24ee9f..eaeb3acac7b 100644 --- a/app/src/main/java/com/nextcloud/talk/data/database/dao/ChatMessagesDao.kt +++ b/app/src/main/java/com/nextcloud/talk/data/database/dao/ChatMessagesDao.kt @@ -112,6 +112,37 @@ interface ChatMessagesDao { @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsertChatMessage(chatMessage: ChatMessageEntity) + @Query( + """ + SELECT EXISTS( + SELECT 1 + FROM ChatMessages + WHERE internalConversationId = :internalConversationId + AND referenceId = :referenceId + AND isTemporary = 0 + ) + """ + ) + suspend fun hasServerCopyOfReferenceId(internalConversationId: String, referenceId: String): Boolean + + /** + * Stores a temporary message unless the server copy of it is already stored. The counterpart of + * [upsertChatMessagesAndDeleteTemp], which drops the temporary message once the server copy arrives: + * a temporary message stored after that would never be dropped again. Check and insert share one + * transaction, so a sync cannot store the server copy in between. + * + * @return true if the message was stored, false if the server copy already exists + */ + @Transaction + suspend fun upsertTempChatMessageUnlessServerCopyExists(chatMessage: ChatMessageEntity): Boolean { + val referenceId = chatMessage.referenceId + if (referenceId != null && hasServerCopyOfReferenceId(chatMessage.internalConversationId, referenceId)) { + return false + } + upsertChatMessage(chatMessage) + return true + } + @Deprecated("use getChatMessageEntity") @Query( """ diff --git a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt index f305eb5d7ab..6d6bb33e5c8 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt @@ -65,6 +65,9 @@ class DummyChatMessagesDaoImpl : ChatMessagesDao { /* */ } + override suspend fun hasServerCopyOfReferenceId(internalConversationId: String, referenceId: String): Boolean = + false + override fun getChatMessageForConversation( internalConversationId: String, messageId: Long diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt new file mode 100644 index 00000000000..83e0d470742 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt @@ -0,0 +1,226 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.chat.data.network + +import android.app.Application +import android.content.Context +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import com.nextcloud.talk.chat.data.model.ChatMessage +import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater +import com.nextcloud.talk.data.database.mappers.asEntity +import com.nextcloud.talk.data.database.model.ChatMessageEntity +import com.nextcloud.talk.data.database.model.SendStatus +import com.nextcloud.talk.data.network.NetworkMonitor +import com.nextcloud.talk.data.source.local.TalkDatabase +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.data.user.model.UserEntity +import com.nextcloud.talk.logger.Logger +import com.nextcloud.talk.models.domain.ConversationModel +import com.nextcloud.talk.models.json.conversations.ConversationDto +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Upload placeholders against a real in-memory database. The worker is enqueued before the placeholder + * exists, so a sync can store the server copy of the shared file first; the placeholder must then not + * be stored, because [com.nextcloud.talk.data.database.dao.ChatMessagesDao.upsertChatMessagesAndDeleteTemp] + * only drops temporary messages that exist at the time the server copy is written. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class OfflineFirstChatRepositoryUploadPlaceholderTest { + + private lateinit var db: TalkDatabase + private lateinit var repository: OfflineFirstChatRepository + + private val logger: Logger = mock() + private val network: ChatNetworkDataSource = mock() + private val networkMonitor: NetworkMonitor = mock() + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, TalkDatabase::class.java) + .allowMainThreadQueries() + .build() + runBlocking { + db.usersDao().saveUser(UserEntity(id = ACCOUNT_ID, userId = "me", username = "me", baseUrl = BASE_URL)) + db.conversationsDao().upsertConversations( + ACCOUNT_ID, + listOf(ConversationDto(token = ROOM_TOKEN).asEntity(ACCOUNT_ID)) + ) + } + whenever(networkMonitor.isOnline).thenReturn(MutableStateFlow(true)) + + val conversationListUpdater = + ConversationListUpdater(db.chatMessagesDao(), db.chatBlocksDao(), db.conversationsDao()) + repository = OfflineFirstChatRepository( + logger, + db.chatMessagesDao(), + db.chatBlocksDao(), + network, + networkMonitor, + ChatMessageSyncer( + db.chatMessagesDao(), + db.chatBlocksDao(), + network, + networkMonitor, + conversationListUpdater + ), + conversationListUpdater + ) + repository.initData(user(), "credentials", CHAT_URL, ROOM_TOKEN, null) + repository.updateConversation( + ConversationModel.mapToConversationModel(ConversationDto(token = ROOM_TOKEN), user()) + ) + } + + @After + fun tearDown() { + db.close() + } + + @Test + fun `placeholder is not stored when the server copy of the file message already exists`() = + runBlocking { + // the sync stored the shared file before the placeholder insert ran + db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( + INTERNAL_CONVERSATION_ID, + listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) + ) + + val results = addPlaceholder(REFERENCE_ID) + + assertTrue(results.isEmpty()) + assertNull(tempMessage(REFERENCE_ID)) + val stored = db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID) + assertNotNull(stored) + assertEquals(false, stored!!.isTemporary) + } + + @Test + fun `placeholder is stored pending when no server copy exists`() = + runBlocking { + val results = addPlaceholder(REFERENCE_ID) + + assertTrue(results.isEmpty()) + val placeholder = tempMessage(REFERENCE_ID) + assertNotNull(placeholder) + assertEquals(SendStatus.PENDING, placeholder!!.sendStatus) + } + + @Test + fun `server copy of another file does not block the placeholder`() = + runBlocking { + db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( + INTERNAL_CONVERSATION_ID, + listOf(serverMessage(SERVER_MESSAGE_ID, OTHER_REFERENCE_ID)) + ) + + addPlaceholder(REFERENCE_ID) + + assertNotNull(tempMessage(REFERENCE_ID)) + } + + @Test + fun `server copy arriving after the placeholder still removes it`() = + runBlocking { + addPlaceholder(REFERENCE_ID) + assertNotNull(tempMessage(REFERENCE_ID)) + + db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( + INTERNAL_CONVERSATION_ID, + listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) + ) + + assertNull(tempMessage(REFERENCE_ID)) + assertNotNull(db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID)) + } + + @Test + fun `marking a failed upload does nothing when no placeholder exists`() = + runBlocking { + db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( + INTERNAL_CONVERSATION_ID, + listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) + ) + addPlaceholder(REFERENCE_ID) + + repository.markUploadPlaceholderFailed(REFERENCE_ID) + + assertNull(tempMessage(REFERENCE_ID)) + val stored = db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID) + assertEquals(false, stored!!.isTemporary) + assertNull(stored.sendStatus) + } + + private suspend fun addPlaceholder(referenceId: String) = + repository.addUploadPlaceholderMessage( + localFileUri = "content://provider/file", + fileName = "photo.jpg", + caption = "", + mimeType = "image/jpeg", + fileSize = FILE_SIZE, + referenceId = referenceId + ).toList() + + private suspend fun tempMessage(referenceId: String): ChatMessageEntity? = + db.chatMessagesDao().getTempMessageForConversation(INTERNAL_CONVERSATION_ID, referenceId, null).first() + + private fun serverMessage(id: Long, referenceId: String): ChatMessageEntity = + ChatMessageEntity( + internalId = "$INTERNAL_CONVERSATION_ID@$id", + internalConversationId = INTERNAL_CONVERSATION_ID, + id = id, + accountId = ACCOUNT_ID, + token = ROOM_TOKEN, + actorDisplayName = "Me", + actorId = "me", + actorType = "users", + message = "{file}", + messageType = "comment", + systemMessageType = ChatMessage.SystemMessageType.DUMMY, + referenceId = referenceId + ) + + private fun user(): User = + User( + id = ACCOUNT_ID, + userId = "me", + username = "me", + displayName = "Me", + baseUrl = BASE_URL + ) + + companion object { + private const val ACCOUNT_ID = 1L + private const val ROOM_TOKEN = "room1" + private const val INTERNAL_CONVERSATION_ID = "$ACCOUNT_ID@$ROOM_TOKEN" + private const val BASE_URL = "https://server.example.com" + private const val CHAT_URL = "$BASE_URL/ocs/v2.php/apps/spreed/api/v1/chat/$ROOM_TOKEN" + private const val REFERENCE_ID = "ref-file-1" + private const val OTHER_REFERENCE_ID = "ref-file-2" + private const val SERVER_MESSAGE_ID = 100L + private const val FILE_SIZE = 1024L + } +} From d9627f1a75ad6431514d922f6849beacda16163a Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Thu, 15 Oct 2026 06:35:20 +0000 Subject: [PATCH 6/7] test(chat): build the server copy with the mapper in the placeholder race test Build the server copy from a ChatMessageDto through asEntity, so the test proves the mapper's conversation key matches the repository's. Compare the stored server row with a snapshot instead of checking one flag. Turn the duplicate "mark failed without placeholder" test into a check that markUploadPlaceholderFailed leaves the server copy unchanged. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-sonnet-5-5 --- ...irstChatRepositoryUploadPlaceholderTest.kt | 60 +++++++++---------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt index 83e0d470742..84dd55e621c 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt @@ -22,6 +22,7 @@ import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.data.user.model.UserEntity import com.nextcloud.talk.logger.Logger import com.nextcloud.talk.models.domain.ConversationModel +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.conversations.ConversationDto import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first @@ -104,18 +105,13 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { fun `placeholder is not stored when the server copy of the file message already exists`() = runBlocking { // the sync stored the shared file before the placeholder insert ran - db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( - INTERNAL_CONVERSATION_ID, - listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) - ) + val serverCopy = storeServerCopy(SERVER_MESSAGE_ID, REFERENCE_ID) val results = addPlaceholder(REFERENCE_ID) assertTrue(results.isEmpty()) assertNull(tempMessage(REFERENCE_ID)) - val stored = db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID) - assertNotNull(stored) - assertEquals(false, stored!!.isTemporary) + assertEquals(serverCopy, storedMessage(SERVER_MESSAGE_ID)) } @Test @@ -132,10 +128,7 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { @Test fun `server copy of another file does not block the placeholder`() = runBlocking { - db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( - INTERNAL_CONVERSATION_ID, - listOf(serverMessage(SERVER_MESSAGE_ID, OTHER_REFERENCE_ID)) - ) + storeServerCopy(SERVER_MESSAGE_ID, OTHER_REFERENCE_ID) addPlaceholder(REFERENCE_ID) @@ -148,30 +141,21 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { addPlaceholder(REFERENCE_ID) assertNotNull(tempMessage(REFERENCE_ID)) - db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( - INTERNAL_CONVERSATION_ID, - listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) - ) + storeServerCopy(SERVER_MESSAGE_ID, REFERENCE_ID) assertNull(tempMessage(REFERENCE_ID)) - assertNotNull(db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID)) + assertNotNull(storedMessage(SERVER_MESSAGE_ID)) } @Test - fun `marking a failed upload does nothing when no placeholder exists`() = + fun `marking a failed upload leaves the server copy unchanged`() = runBlocking { - db.chatMessagesDao().upsertChatMessagesAndDeleteTemp( - INTERNAL_CONVERSATION_ID, - listOf(serverMessage(SERVER_MESSAGE_ID, REFERENCE_ID)) - ) - addPlaceholder(REFERENCE_ID) + val serverCopy = storeServerCopy(SERVER_MESSAGE_ID, REFERENCE_ID) repository.markUploadPlaceholderFailed(REFERENCE_ID) assertNull(tempMessage(REFERENCE_ID)) - val stored = db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, SERVER_MESSAGE_ID) - assertEquals(false, stored!!.isTemporary) - assertNull(stored.sendStatus) + assertEquals(serverCopy, storedMessage(SERVER_MESSAGE_ID)) } private suspend fun addPlaceholder(referenceId: String) = @@ -187,21 +171,31 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { private suspend fun tempMessage(referenceId: String): ChatMessageEntity? = db.chatMessagesDao().getTempMessageForConversation(INTERNAL_CONVERSATION_ID, referenceId, null).first() - private fun serverMessage(id: Long, referenceId: String): ChatMessageEntity = - ChatMessageEntity( - internalId = "$INTERNAL_CONVERSATION_ID@$id", - internalConversationId = INTERNAL_CONVERSATION_ID, + /** + * Stores the server copy the way the sync does: DTO -> [asEntity] -> upsertChatMessagesAndDeleteTemp. The + * mapper builds the conversation key itself, so the tests also prove it matches the repository's key. + * + * @return the stored row + */ + private suspend fun storeServerCopy(id: Long, referenceId: String): ChatMessageEntity { + val entity = ChatMessageDto( id = id, - accountId = ACCOUNT_ID, token = ROOM_TOKEN, - actorDisplayName = "Me", - actorId = "me", actorType = "users", + actorId = "me", + actorDisplayName = "Me", message = "{file}", messageType = "comment", systemMessageType = ChatMessage.SystemMessageType.DUMMY, referenceId = referenceId - ) + ).asEntity(ACCOUNT_ID) + assertEquals(INTERNAL_CONVERSATION_ID, entity.internalConversationId) + db.chatMessagesDao().upsertChatMessagesAndDeleteTemp(entity.internalConversationId, listOf(entity)) + return storedMessage(id)!! + } + + private suspend fun storedMessage(id: Long): ChatMessageEntity? = + db.chatMessagesDao().getChatMessageEntity(INTERNAL_CONVERSATION_ID, id) private fun user(): User = User( From 4ad3e8ee68ee48d8b6d1bd00202bb64e2a667620 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Thu, 15 Oct 2026 07:01:30 +0000 Subject: [PATCH 7/7] fix(chat): mark a failed upload placeholder off the main thread markUploadPlaceholderFailed is called from viewModelScope and updateChatMessage is a blocking Room call, so the update ran on the main thread. The placeholder test database no longer allows main thread queries, which catches it. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-opus-5-5 --- .../data/network/OfflineFirstChatRepository.kt | 3 ++- ...FirstChatRepositoryUploadPlaceholderTest.kt | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index ebc55b3435b..bb15517f7f0 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -811,7 +811,8 @@ class OfflineFirstChatRepository @Inject constructor( override suspend fun markUploadPlaceholderFailed(referenceId: String) { val entity = chatDao.getTempMessageForConversation(internalConversationId, referenceId, null).firstOrNull() if (entity?.sendStatus == SendStatus.PENDING) { - chatDao.updateChatMessage(entity.copy(sendStatus = SendStatus.FAILED)) + // called from viewModelScope: updateChatMessage is a blocking Room call + withContext(Dispatchers.IO) { chatDao.updateChatMessage(entity.copy(sendStatus = SendStatus.FAILED)) } } } diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt index 84dd55e621c..8df331f4886 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt @@ -46,6 +46,9 @@ import org.robolectric.annotation.Config * exists, so a sync can store the server copy of the shared file first; the placeholder must then not * be stored, because [com.nextcloud.talk.data.database.dao.ChatMessagesDao.upsertChatMessagesAndDeleteTemp] * only drops temporary messages that exist at the time the server copy is written. + * + * The database does not allow main thread queries: the test thread is the main looper thread under + * Robolectric, so a blocking DAO call made by the repository on its caller's thread fails the test. */ @RunWith(RobolectricTestRunner::class) @Config(application = Application::class, sdk = [33]) @@ -61,9 +64,7 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { @Before fun setUp() { val context = ApplicationProvider.getApplicationContext() - db = Room.inMemoryDatabaseBuilder(context, TalkDatabase::class.java) - .allowMainThreadQueries() - .build() + db = Room.inMemoryDatabaseBuilder(context, TalkDatabase::class.java).build() runBlocking { db.usersDao().saveUser(UserEntity(id = ACCOUNT_ID, userId = "me", username = "me", baseUrl = BASE_URL)) db.conversationsDao().upsertConversations( @@ -158,6 +159,17 @@ class OfflineFirstChatRepositoryUploadPlaceholderTest { assertEquals(serverCopy, storedMessage(SERVER_MESSAGE_ID)) } + @Test + fun `marking a failed upload sets the pending placeholder to failed off the main thread`() = + runBlocking { + addPlaceholder(REFERENCE_ID) + + // called from viewModelScope, i.e. the main thread; the update must not run there + repository.markUploadPlaceholderFailed(REFERENCE_ID) + + assertEquals(SendStatus.FAILED, tempMessage(REFERENCE_ID)!!.sendStatus) + } + private suspend fun addPlaceholder(referenceId: String) = repository.addUploadPlaceholderMessage( localFileUri = "content://provider/file",