Repository navigation
fix(chat): read contacts, the shared file and voice duration off the main thread - #6860
ToteMeiSter wants to merge 5 commits into
Conversation
…main thread Picking a contact (query, display name, vCard copy via the contacts provider), the readability probe of share-to-notes and the voice message duration were read in UI callbacks. Move them to Dispatchers.IO. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 17.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 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 main change: moving contact, shared-file, and voice-duration work off the main thread. Description check The description explains the problem, implementation, testing, manual-test status, and checklist items. It follows the repository template and provides sufficient context. 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.
- Fix all pre-merge checks with AI
✨ 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: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a56b7af9-522a-4079-aa63-7eb6b30520fe
📒 Files selected for processing (1)
app/src/main/java/com/nextcloud/talk/chat/ChatActivity.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.
…e missing-file path prepareVoiceMessage resumed after the off-main duration read and started the old message even when another one was selected meanwhile. Compare with the current voice message first. shareToNotes caught Exception around the new IO call, so a cancelled coroutine fell through to downloadFileToCache. Rethrow CancellationException. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks, the contact and share-to-notes parts look good (cursor.use {} and rethrowing CancellationException are nice touches). Two things in the voice message part need changes:
1. The duration is still read on the main thread when the file isn't cached yet. That is the first play of any voice message, so the most common case. The downloadFileToCache callback in prepareVoiceMessage still does:
downloadFileToCache(message, true) {
chatViewModel.syncVoiceMessageUiState(
message.apply { voiceMessageDuration = getAudioDuration(file.absolutePath).toInt() }
)
finishPreparing()
}This needs the same withContext(Dispatchers.IO) treatment, e.g. by launching a coroutine in the callback. Related: the new IO read runs before the file.exists() check, so for an uncached file it reads a file that isn't there yet. Moving it into the "file exists" branch avoids the useless read.
2. A fast double tap can now prepare the same voice message twice. Before, prepareVoiceMessage ran without suspending, so a second tap found the media item already loaded and only paused or resumed it. Now there is a window while the duration is read: a second tap on the same message passes the new currentVoiceMessage guard (same id), and setupAndPlay and the waveform computation run twice. Ignoring taps while a message is being prepared (e.g. keeping the preparing Job or message id and returning early), or re-checking controller.currentMediaItem?.mediaId after the read, would fix it.
Suggestions (non-blocking):
writeContactToVcfFilestill usesopenAssetFileDescriptor(...)!!, and a provider exception (missing vCard,SecurityException) is uncaught, so it now crashes from the coroutine instead of the main thread. WrappingsaveContactAsVcfFilein atry/catchthat logs and shows the existingnc_common_error_sorrysnackbar would be cheap.- Please use the PR template (how to test, checklist, AI disclosure section) as in your other PRs; the description here is a one-liner.
…ad and ignore repeated taps while it is read The duration of a voice message that is not cached yet was still read on the main thread in the download callback, and the read ran before the file exists check. Read it only for a file that exists and move the callback read to IO. While the duration of a message is read, a repeated tap on the same message is ignored, so it is not prepared twice. A failing contacts provider shows the error snackbar instead of crashing the coroutine. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
79836dd6-7438-4c33-9196-5fe2f49bf661
📒 Files selected for processing: 5
app/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/ProviderAccess.ktapp/src/main/java/com/nextcloud/talk/chat/VoiceMessageDurationLoader.ktapp/src/test/java/com/nextcloud/talk/chat/ProviderAccessTest.ktapp/src/test/java/com/nextcloud/talk/chat/VoiceMessageDurationLoaderTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
Thanks for the review, both points are fixed in 5f6e4f5.
Also done: |
…her message finishes its read The duration loader remembered only one message being read. When message B finished while A was still read, B cleared the mark, and a repeated tap on A read and prepared it a second time. Keep the set of messages being read. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
…n a live media controller Updating how a voice message is shown also selected it as the message to play. A finished download, a waveform or the download of any other file selected that message, so the check for another selected message could never see a change, and the playing message reported its progress into the wrong one. Only a tap selects the message now. A finished download always clears its indicator and stores the duration; the message is played only if it is still selected. Its duration is read through the same guard as a tap, so a tap during that read does not prepare it twice. The media controller lives from onStart to onStop. It was kept across the duration read and could be released by then. The current controller is taken after the read, and nothing is played without one. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks, both points fixed, and good catch on syncVoiceMessageUiState() reassigning currentVoiceMessage on every UI update; only the tap selecting the message is the right fix, and taking the current mediaController after the read avoids using a released one. The download indicator staying after a failed download is pre-existing and fine to leave for later.
Problem
Three places in
ChatActivityran provider and media work in UI callbacks, on the main thread:ContentResolver.query, the display name lookup and the vCard copy through the contacts provider (another process);MediaMetadataRetrieverreading the duration of the file, including the first play of a message that is not cached yet (thedownloadFileToCachecallback).Each is a blocking call into another process or a file read; a slow provider or a large file stalls the UI.
Change
Dispatchers.IO; results are published on the main thread.prepareVoiceMessage: the duration is read only for a file that exists. For a file that is not cached, thedownloadFileToCachecallback launches a coroutine that reads the duration on IO.VoiceMessageDurationLoader(new, small): reads on the injected IO dispatcher and ignores a repeated tap on the same message while its duration is being read. Before this changeprepareVoiceMessagedid not suspend, so the guardcurrentVoiceMessageandsetMediaItemran in one main-thread step. The IO read adds a suspension point between them, so a second tap could pass the guard and prepare the message again.runProviderCall(new, small): a failing contacts provider (no vCard,SecurityException) is logged and shows the existingnc_common_error_sorrysnackbar instead of crashing the coroutine.CancellationExceptionis rethrown.How to test
./gradlew :app:testGenericDebugUnitTest --tests "com.nextcloud.talk.chat.*",:app:ktlintCheckand:app:detektpass locally.VoiceMessageDurationLoaderTest: the read runs on the IO dispatcher thread, not on the caller; a secondreadOncefor the same message during the read returns null and the reader runs once; another message is not blocked. Without thewithContextand without the guard the first two tests fail.ProviderAccessTest: a provider exception is reported and not thrown; cancellation still propagates.🏁 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.🤖 Generated with Claude Code