fix(attachment-preview): do not ask the content provider for the type during composition

[ToteMeiSter]

Problem

FileAttachmentPreviewContent computed hasCompressibleMedia in the body of the composable with FileUtils.resolveMimeType(context, uri) for every file, i.e. ContentResolver.getType on the main thread on every recomposition. For a file shared from the Nextcloud Files app this is a binder call into another process that answers slowly. Any recomposition of the preview (for example switching the file permission) blocked the main thread, and the screen froze into an ANR.

Change

• hasCompressibleMedia is derived from viewModel.descriptionsByUri (FileDescription.mimeType), which describeFiles fills on Dispatchers.IO. No second mechanism, no provider call during composition.
• compressImages starts from initialCompressImages; onSend passes compressImages && hasCompressibleMedia, so the value sent is the same as before.
• describeFiles publishes with putAll + retainAll instead of clear + putAll. Otherwise the quality option, which now depends on the descriptions, would vanish for a moment every time the descriptions are re-read (for example when the quality is toggled).
• Send is enabled once every file has a description, so the compression flag is never read before the type is known.
• The quality button appears when the descriptions have been read (a fraction of a second after opening) instead of immediately.

How to test

• ./gradlew :app:compileGplayDebugKotlin :app:ktlintCheck :app:detekt pass locally.
• Manual: share a file from the Nextcloud Files app into a chat, open the preview and switch the file permission several times; the preview stays responsive. Images and videos from the gallery still show the quality option and send as before.
• Verified on a device (Android 12) with the same change on top of a larger branch: sharing a file from the Nextcloud Files app and switching the permission repeatedly no longer freezes the preview.
• No unit test: the code is inside a composable. A test that reproduces the freeze needs a slow ContentProvider and a real composition, and that needs unitTests.isIncludeAndroidResources = true, which breaks the other Robolectric tests of the module (manifest receivers). The main-thread provider calls were counted with such a test run locally, with a provider that sleeps 300 ms: 10 main-thread calls before the change, 0 after.

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not needed
• 🔖 Capability is checked or not needed
• 🔙 Backport requests are created or not needed: /backport to stable-xx.x
• 📅 Milestone is set
• 🌸 PR title is meaningful

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI

The change was prepared with the help of Claude Code (Anthropic, claude-sonnet-5-5). The author reviewed the code, built it and ran the static checks.

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

The attachment preview requests MIME-type resolution from the view model in an effect keyed by the current file set. The view model resolves MIME types and file descriptions asynchronously and retains results only for files in the relevant snapshot. The preview uses cached MIME types to check compression eligibility. The compression option initializes from initialCompressImages. The send callback behavior is unchanged.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 73d1c

The change moves MIME-type lookups off the main thread and guards against stale results. No concrete merge-blocking risk remains; the sleep-based test could be made more deterministic as a follow-up.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: preventing content-provider MIME-type lookups during composition.
Description check		The description explains the problem, implementation, testing, manual verification, checklist status, and AI usage. It omits the template's screenshots and TODO sections, but the required change conte…
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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4f47a0a5-2365-496d-9e12-a1f58cf64993

📥 Commits

Reviewing files that changed from the base of the PR and between 976034c and 087f23b.

📒 Files selected for processing (2)

• app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt
• app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.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.

[rapterjet2004]

Thanks, getting getType out of composition is the right fix. One concern with how it's done, though.

Send now waits for the full file copy. sendEnabled waits for every FileDescription, and describeFile calls FileUtils.getFileFromUri, which copies the whole file into the cache for content:// URIs (plus a video frame extraction and size estimate). For a large video shared from Nextcloud Files, Send stays disabled until the copy finishes, with no indication why. Before this change it could be sent immediately.

The type alone is a single ContentResolver.getType call. It is only slow because it runs on the main thread, so I'd resolve just the type off the main thread, separately from the full description. For example:

val context = LocalContext.current
val currentFileSet = currentFiles.toSet()
val hasCompressibleMedia by produceState(initialValue = false, currentFileSet) {
    value = withContext(Dispatchers.IO) {
        currentFileSet.any { isCompressible(FileUtils.resolveMimeType(context, it.toUri())) }
    }
}

(or a small mimeTypesByUri state in the ViewModel filled on Dispatchers.IO, so it survives rotation).

Send doesn't need to be gated either: UploadAndShareFilesWorker.compressMediaIfPossible already checks the type and only compresses images and videos, so passing compressImages as is stays correct. sendEnabled = currentFiles.isNotEmpty() can stay.

The putAll + retainAll change is a good one on its own, please keep it.

Minor (partly pre-existing): describeFiles launches a new job each time without cancelling the previous one. If an older job finishes last, its retainAll drops the description of a file that was just added. With clear() before, this only hid the preview; with the Send gate it would also leave Send disabled until the next change. Keeping a Job reference and cancelling it before launching a new one fixes it.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt (1)

73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test cancellation after the first job starts.

The test scheduler does not run the first describeFiles job before the second call cancels it. This test therefore passes without checking whether an in-progress, cancelled job can publish stale results. Start the first job, hold its describer, issue the second request, then release the first job and assert that the newer snapshot remains.

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 370d6305-bfde-4a84-b42e-61761579f010

📥 Commits

Reviewing files that changed from the base of the PR and between 0169d98 and 3e6dcee.

📒 Files selected for processing (3)

• app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt
• app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt
• app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.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.

[ToteMeiSter]

@coderabbitai regarding the nitpick on FileAttachmentPreviewViewModelTest (the cancel test only covers the trivial case): agreed. a9d9fd6 adds a test where the first describeFiles call blocks inside the describer on a real IO thread until the second call has finished; after releasing it, both descriptions must still be present. I checked that it fails without the withContext(mainDispatcher) change.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd '^FileAttachmentPreview(ViewModelTest|ViewModel)\.kt$' app
ast-grep outline app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt --items all
ast-grep outline app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt --items all
sed -n '1,280p' app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt
rg -n -C 18 'fun describeFiles|withContext\(mainDispatcher\)|describeFilesJob' app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt

Length of output: 9496

---

@ToteMeiSter Thanks for the update. I inspected the new test. It covers cancellation while the first describer is blocked on a real IO thread and checks that both descriptions remain after release.

The withContext(mainDispatcher) cancellation check prevents the cancelled job from publishing stale results. This addresses the original test-coverage nitpick. I did not run the test in this environment.

---

If 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.

[ToteMeiSter]

Good catch, thanks. Pushed 3e6dcee and a9d9fd6:

• The MIME type is now resolved separately on Dispatchers.IO into a mimeTypesByUri state in the ViewModel (survives rotation), independent of the full description. hasCompressibleMedia reads it from there.
• sendEnabled = currentFiles.isNotEmpty(), and compressImages is passed on as is, since the worker only compresses images and videos.
• putAll + retainAll is kept.
• describeFiles cancels the previous job before launching a new one. The blocking copy cannot be interrupted, so a cancelled job could still finish and retainAll away a newer description; results of describeFiles and resolveMimeTypes are now published via withContext(main), which throws for a cancelled job (a9d9fd6).
• Tests in FileAttachmentPreviewViewModelTest, including the race with a cancelled job that is still copying.

[rapterjet2004]

Thanks, all points addressed. Resolving only the MIME type into its own state, not gating Send, and publishing both results through withContext(mainDispatcher) so a cancelled job that was still copying can't write anymore look good, and the test for the cancelled-but-still-running job is a nice touch.

Tiny optional nit: the test hooks (ioDispatcher, mainDispatcher, describer, mimeTypeResolver) could be constructor parameters with defaults instead of internal vars.

[coderabbitai]

🧹 Nitpick comments: 1

app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt: 1

108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace Thread.sleep with deterministic synchronization.

The test uses Thread.sleep(300) to wait for real IO work. On a slow CI machine, the second describeFiles job may not finish within 300 ms. The test then asserts too early and fails intermittently. Wait on a CountDownLatch that the describer counts down for the second file. Also use a latch after release.countDown() to confirm the first job finished.

This is a test-quality improvement. The current logic is correct.

Also applies to: 116-116

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8a706c8f-233a-4208-8622-91f5ccb5bd17

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6dcee and 73d1c9f.

📒 Files selected for processing: 2

• app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt
• app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.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.

[ToteMeiSter]

Thanks! Done in 73d1c9f: ioDispatcher, mainDispatcher, describer and mimeTypeResolver are now constructor parameters with the production defaults, and the test passes them in instead of reassigning vars. Dagger can't use Kotlin default values, so the class keeps an @Inject constructor(context: Context) that delegates to the primary one. No behaviour change.