Repository navigation
fix(chat): resolve the name, type and size of a file to send off the main thread - #6858
ToteMeiSter wants to merge 7 commits into
Conversation
…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 <krajnov.g@kontentplus.ru>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 8.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 7 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: resolving file metadata off the main thread. Description check The description explains the problem, implementation, testing, manual verification, checklist, and AI usage. The Screenshots and TODO sections are omitted, but the description is otherwise complete an… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0cd856c8-77b9-4de5-9563-e19230ee7b63
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
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 <krajnov.g@kontentplus.ru>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Insert the placeholder before enqueueing non-voice uploads. · ChatViewModel.kt:2377-2390
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt:2377-2390
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInsert the placeholder before enqueueing non-voice uploads.
UploadAndShareFilesWorker.upload(...)runs before the asynchronous placeholder insertion completes. A fast worker failure can callupdatePlaceholderStatus(SendStatus.FAILED)while no placeholder exists. That method then skips the update, and the later placeholder is inserted asSendStatus.PENDING, so the failed upload can remain stuck as pending. Move the non-voice enqueue and progress registration aftercollect {}. Keep the voice-message path unchanged.Suggested fix
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) { 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, caption = caption, mimeType = mimeType, fileSize = fileSize, referenceId = referenceId - ).collect {} + ).collect {} + + val workerId = UploadAndShareFilesWorker.upload( + userId = currentUser.id!!, + fileUri = fileUri, + roomToken = room, + conversationName = displayName, + metaData = metaData, + referenceId = referenceId, + internalConversationId = internalConversationId, + compressImages = compressImages, + allowUpdate = allowUpdate + ) + uploadReferenceToWorkId[referenceId] = workerId + _uploadedLocalPreviewMap.update { it + (referenceId to fileUri) } + observeUploadProgress(workerId, referenceId) } } - 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) + val internalConversationId = "${currentUser.id}@$chatRoomToken" + if (isVoiceMessage) { + UploadAndShareFilesWorker.upload( + userId = currentUser.id!!, + fileUri = fileUri, + roomToken = room, + conversationName = displayName, + metaData = metaData, + referenceId = referenceId, + internalConversationId = internalConversationId, + compressImages = compressImages, + allowUpdate = allowUpdate + ) }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e43cb0e9-b0ad-415c-9718-28a5ed97cfab
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt
- app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
…ists 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 <krajnov.g@kontentplus.ru>
|
@coderabbitai Thanks, the outside-diff finding is valid (the lookup became asynchronous with this PR, so the worker could fail before the placeholder existed). Fixed in b2bb61a: for non-voice uploads the worker is now enqueued, and the progress observer registered, after the placeholder insert completes. Voice messages keep the immediate enqueue. A test checks that no upload is queued when the placeholder is added and that it is queued afterwards. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt (1)
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftVerify placeholder insertion, not only the repository call.
This mock records queue size when
addUploadPlaceholderMessageis called, then returns an empty flow. It never inserts a placeholder. The ordering test can therefore pass even if work is enqueued before a placeholder exists. Record the assertion after a real or test-backed insert, and check that the placeholder is present when work is queued.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
737efc95-9631-4824-8f3a-41ff01049ee9
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
…older 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 <krajnov.g@kontentplus.ru>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Reconcile an existing server message before inserting the… · ChatViewModel.kt:2421-2425
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt:2421-2425
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReconcile an existing server message before inserting the placeholder.
The worker is enqueued before the asynchronous file lookup and placeholder insertion. If sync persists the successful server message first, its temporary-row deletion finds no row. The later placeholder uses a different primary key, so both rows can remain.
Changing only
sendStatustoSENT_PENDING_ACKdoes not remove the duplicate. Perform the server-message lookup and placeholder insert in one transaction. Skip the insert when a non-temporary message with the samereferenceIdalready exists.Suggested fix
+ @Query( + """ + SELECT * + FROM ChatMessages + WHERE internalConversationId = :internalConversationId + AND referenceId = :referenceId + AND isTemporary = 0 + ORDER BY timestamp DESC, id DESC + LIMIT 1 + """ + ) + suspend fun getServerMessageForConversation( + internalConversationId: String, + referenceId: String + ): ChatMessageEntity? + + @Transaction + suspend fun upsertUploadPlaceholderIfServerMessageAbsent( + internalConversationId: String, + referenceId: String, + placeholder: ChatMessageEntity + ): ChatMessageEntity? { + val serverMessage = getServerMessageForConversation(internalConversationId, referenceId) + if (serverMessage != null) return serverMessage + + upsertChatMessage(placeholder) + return null + }- chatDao.upsertChatMessage(entity) + val serverMessage = chatDao.upsertUploadPlaceholderIfServerMessageAbsent( + internalConversationId = internalConversationId, + referenceId = referenceId, + placeholder = entity + ) + if (serverMessage != null) { + emit(Result.success(serverMessage.toDomainModel())) + }
🧹 Nitpick comments (1)
app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt (1)
158-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSynchronize the early-failure test with placeholder creation.
setAllConstraintsMet(workId)only allows the worker to run. The test then waits forplaceholderAddedbefore it waits forplaceholderMarkedFailed, so it also passes when the worker fails after the placeholder is created. Block theSlowProviderresponse, assert thatplaceholderAddedhas not fired when the work reachesFAILED, then release the provider and assert that the failure update marks the placeholder failed.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4ec11626-6ef7-4cde-af5d-9d84dc3f8fe8
📒 Files selected for processing (4)
app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.ktapp/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelUploadFileTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks, the change looks good overall: enqueueing first keeps the order of the chained queue, and observing the work only after the placeholder exists makes sure an early failure still marks it. One thing to add before this goes in:
Please guard against a placeholder that is added after its server copy. The upload is now enqueued before the placeholder exists. Placeholders are removed when the server copy is persisted (ChatMessagesDao deletes temp messages with the same referenceId on upsert). If the server message arrives before addUploadPlaceholderMessage runs, nothing removes the placeholder anymore, and a stuck "uploading" bubble stays next to the real message.
It's unlikely in practice (the worker reads the same slow provider, copies the file and uploads it), but the result is permanent, and the guard is cheap: in addUploadPlaceholderMessage, skip the insert if a non-temporary message with that referenceId is already stored for the conversation (ideally in the same DAO transaction as the upsert, so the check and the insert can't interleave with the sync).
…y 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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
…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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Run the placeholder failure update on Dispatchers.IO. · OfflineFirstChatRepository.kt:814
app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt:814
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRun the placeholder failure update on
Dispatchers.IO.
observeUploadProgresslaunches this suspend function fromviewModelScope. The function performs a synchronous@UpdatethroughchatDao.updateChatMessage, which can block the main thread.allowMainThreadQueries()prevents Room from rejecting the call, but it does not move the database work off the main thread.Suggested fix
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)) + withContext(Dispatchers.IO) { + val entity = chatDao.getTempMessageForConversation(internalConversationId, referenceId, null).firstOrNull() + if (entity?.sendStatus == SendStatus.PENDING) { + chatDao.updateChatMessage(entity.copy(sendStatus = SendStatus.FAILED)) + } } }Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e2d0c1bb-52a8-4dfa-8279-e1dde8c9e086
📒 Files selected for processing: 4
app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.ktapp/src/main/java/com/nextcloud/talk/data/database/dao/ChatMessagesDao.ktapp/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.ktapp/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
|
@coderabbitai Confirmed and fixed in 4ad3e8e, thanks. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f '^(OfflineFirstChatRepository|OfflineFirstChatRepositoryUploadPlaceholderTest)\.kt$' .
fd -t f '^(OfflineFirstChatRepository|OfflineFirstChatRepositoryUploadPlaceholderTest)\.kt$' . --exec ast-grep outline {} --items all
rg -n -C 18 'override suspend fun markUploadPlaceholderFailed|marking a failed upload sets the pending placeholder to failed off the main thread|allowMainThreadQueries|inMemoryDatabaseBuilder' app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryUploadPlaceholderTest.ktLength of output: 24218
The test database no longer allows main-thread queries. The new regression test checks that a pending placeholder becomes ✏️ Learnings added
You are interacting with an AI system. |
|
Thanks for the review, good catch. Added in 11a7256 (test tightened in d9627f1):
Also fixed the outside-diff remark from CodeRabbit in 4ad3e8e: Text messages don't have this race: This change was prepared with AI assistance (Claude Code). |
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks, upsertTempChatMessageUnlessServerCopyExists() as a transaction alongside upsertChatMessagesAndDeleteTemp() closes the race from both sides, and moving markUploadPlaceholderFailed() off the main thread is a good catch. Note for later: the FAILED-state handling here relies on WorkInfo.State.FAILED, which #6828 changes for failed uploads, so whichever lands second needs to adapt (also noted on #6828).
2 participants
Problem
ChatViewModel.uploadFilecalledresolveFileInfo(ContentResolver.getType+queryfor display name and size) synchronously, before the coroutine that adds the upload placeholder.uploadFileis called from the main thread: the attachment preview result listener, the camera, the contact picker, the keyboard content listener and share to notes (ChatActivity.uploadFiles/uploadFile).viewModelScopeisDispatchers.Main.immediate, so nothing moved the call off the main thread.For a file from another app's provider (for example Nextcloud Files) both calls are blocking binder calls into that process. A slow provider stalls the UI once per file when sending, and several files multiply it.
Change
resolveFileInfonow runs onDispatchers.IO(withContext) inside the existingviewModelScope.launchthat adds the placeholder. No new mechanism; the placeholder is added right after the info is read. The worker is still enqueued synchronously, as before.How to test
./gradlew :app:testGenericDebugUnitTest --tests "com.nextcloud.talk.chat.*"and:app:ktlintCheckpass locally.detektreports no new finding for the changed lines.ChatViewModelUploadFileTest: a realChatViewModelwith mocked collaborators and a content provider that sleeps 300 ms ingetType/queryand counts calls on the main thread. Before the change: 2 main-thread calls, test fails. After: 0, the placeholder is still added.🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The change was prepared with Claude Code (Anthropic,
claude-sonnet-5-5). The author reviewed the code, built it and ran the tests and static checks. Not tested on a device.🤖 Generated with Claude Code