android: avoid staging raw media bytes#539
Open
jodobear wants to merge 1 commit intosledtools:masterfrom
Open
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a metadata-first, capacity-limited media staging system to ChatScreen with persistable URI permission management and just-in-time Base64 encoding at send time, updates UI preview rendering by MIME type, and introduces instrumented tests for selectUrisForStaging. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatScreen
participant ContentResolver
participant SystemPermissions as "System Permissions"
participant RemoteServer as "Remote Server"
User->>ChatScreen: select media files
ChatScreen->>ChatScreen: selectUrisForStaging(existingCount, uris)
ChatScreen->>SystemPermissions: takePersistableReadPermission(uri)
SystemPermissions-->>ChatScreen: permission granted
ChatScreen->>ContentResolver: readMediaMetadata(uri)
ContentResolver-->>ChatScreen: mimeType, filename
ChatScreen->>ChatScreen: add StagedMedia & show preview
User->>ChatScreen: send staged media
ChatScreen->>ChatScreen: encodeMediaUploadPayload(StagedMedia)
ChatScreen->>ContentResolver: openInputStream(uri) / read bytes
ContentResolver-->>ChatScreen: raw bytes
ChatScreen->>ChatScreen: Base64 encode -> dataBase64
ChatScreen->>RemoteServer: upload MediaUploadPayload(dataBase64, mimeType, filename)
RemoteServer-->>ChatScreen: success/error
ChatScreen->>SystemPermissions: releasePersistableReadPermission(uri)
SystemPermissions-->>ChatScreen: permission released
ChatScreen->>ChatScreen: update stagedMedia state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jodobear
force-pushed
the
android-issue-433-media-staging
branch
from
b913545 to
d79ecca
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt (3)
433-492:⚠️ Potential issue | 🟠 MajorMake staged sends non-reentrant.
sendStagedMedia()snapshots the list and only updates UI state after the IO work finishes. Double-tapping Send dispatches the same batch twice, and removing an item during that window can still send it because it's already instagedSnapshot. Add an in-flight guard insendStagedMedia()and disable the send/remove controls while it runs.Also applies to: 1038-1043, 1133-1137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt` around lines 433 - 492, The sendStagedMedia function is reentrant: it snapshots stagedMedia, does IO, then updates state, allowing double-taps or concurrent calls to dispatch duplicate sends; add an in-flight guard (e.g., a Boolean property like isSending or sendInProgress) checked at the top of sendStagedMedia and set true before starting the coroutine and false in all terminal paths (success, failure, early returns) to prevent reentry, and also use that flag to disable the send/remove UI controls while the operation runs (references: sendStagedMedia, stagedMedia, stagedSnapshot, draft, replyDraft).
509-520:⚠️ Potential issue | 🟠 MajorSingle picks should join an existing staged batch instead of sending immediately.
If
stagedMediaalready contains items, these callbacks still route a one-item selection throughsendMediaFromUri(). That sends a separate message, clears the caption, and leaves the existing staged items behind.🔀 Suggested routing fix
val pickPhotoOrVideoLauncher = rememberLauncherForActivityResult(ActivityResultContracts.OpenMultipleDocuments()) { uris -> - if (uris.size == 1) { + if (uris.size == 1 && currentStagedMedia.isEmpty()) { sendMediaFromUri(uris.first()) } else if (uris.isNotEmpty()) { stageMediaFromUris(uris) } } val pickFileLauncher = rememberLauncherForActivityResult(ActivityResultContracts.OpenDocument()) { uri -> - sendMediaFromUri(uri) + uri?.let { + if (currentStagedMedia.isEmpty()) sendMediaFromUri(it) else stageMediaFromUris(listOf(it)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt` around lines 509 - 520, The single-item selection handlers (pickPhotoOrVideoLauncher and pickFileLauncher) currently call sendMediaFromUri() which creates a separate message instead of joining an existing staged batch; change both callbacks to check stagedMedia (e.g., stagedMedia.isNotEmpty()) and, when non-empty, add the selected URI to the staging flow (call stageMediaFromUris(listOf(uri)) or the existing staging helper) instead of calling sendMediaFromUri(), otherwise keep the current behavior (use sendMediaFromUri() when stagedMedia is empty); update both locations referencing sendMediaFromUri, pickPhotoOrVideoLauncher, pickFileLauncher, stageMediaFromUris and stagedMedia accordingly.
210-233:⚠️ Potential issue | 🔴 CriticalWrap helper functions with exception handling to prevent provider failures from crashing coroutines.
readMediaMetadata()andencodeMediaUploadPayload()callContentResolver.getType(),query(),openInputStream(), andreadBytes(), which can all throw uncaught exceptions (e.g.,SecurityException,IllegalArgumentException,FileNotFoundException,IOException). These helpers are used insendMediaFromUri()andsendStagedMedia()without try-catch, so revoked document URIs or flaky providers will crash the coroutine instead of falling through to your existing null/toast handling.🩹 Suggested hardening
private fun readMediaMetadata(ctx: Context, uri: Uri): StagedMedia? { - val mimeType = ctx.contentResolver.getType(uri).orEmpty() - val filename = - ctx.contentResolver.query(uri, arrayOf(OpenableColumns.DISPLAY_NAME), null, null, null) - ?.use { cursor -> - val idx = cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) - if (idx >= 0 && cursor.moveToFirst()) cursor.getString(idx) else null - } - ?.trim() - .takeUnless { it.isNullOrEmpty() } - ?: "attachment.bin" - return StagedMedia(uri = uri, mimeType = mimeType, filename = filename) + return runCatching { + val mimeType = ctx.contentResolver.getType(uri).orEmpty() + val filename = + ctx.contentResolver.query(uri, arrayOf(OpenableColumns.DISPLAY_NAME), null, null, null) + ?.use { cursor -> + val idx = cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) + if (idx >= 0 && cursor.moveToFirst()) cursor.getString(idx) else null + } + ?.trim() + .takeUnless { it.isNullOrEmpty() } + ?: "attachment.bin" + StagedMedia(uri = uri, mimeType = mimeType, filename = filename) + }.getOrNull() } private fun encodeMediaUploadPayload(ctx: Context, staged: StagedMedia): MediaUploadPayload? { - val bytes = ctx.contentResolver.openInputStream(staged.uri)?.use { it.readBytes() } ?: return null + val bytes = + runCatching { + ctx.contentResolver.openInputStream(staged.uri)?.use { it.readBytes() } + }.getOrNull() ?: return null if (bytes.isEmpty()) return null return MediaUploadPayload(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt` around lines 210 - 233, Wrap the body of readMediaMetadata(...) and encodeMediaUploadPayload(...) in try-catch blocks that catch provider-related exceptions (e.g., SecurityException, IllegalArgumentException, FileNotFoundException, IOException or a general Exception fallback) so any thrown from ContentResolver.getType, query, openInputStream, or readBytes are handled; on exception return null (so callers like sendMediaFromUri/sendStagedMedia fall through to existing null/toast handling) and optionally log the exception via Android Log or your existing logger for debugging; keep the functions' signatures and returned types unchanged (StagedMedia? and MediaUploadPayload?) and ensure resources are still closed by using existing use { } inside the try.
♻️ Duplicate comments (1)
android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt (1)
371-398:⚠️ Potential issue | 🟠 MajorDon't let the first invalid picks consume the last free slots.
Capacity is still decided before validation. With 30 staged items and
[bad1, bad2, good1, good2], only the first two URIs are examined, so later valid URIs never get a chance to fill the freed slots. Iterate until you collectremainingCapacitysuccessful items instead of truncating up front.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt` around lines 371 - 398, stageMediaFromUris currently decides capacity by truncating acceptedUris up front, causing early invalid items to consume slots; change the logic to compute remainingCapacity from stagedMedia.size and stagingReservationCount, then iterate over the incoming uris (not just the truncated acceptedUris) and process each (takePersistableReadPermission, readMediaMetadata) inside the withContext block until you have collected up to remainingCapacity successful StagedMedia items or run out of uris; only increment stagingReservationCount by the number of actually reserved/retained items, and update grantedUris/retainedUris accordingly so later valid URIs can fill freed slots (keep referenced symbols: stageMediaFromUris, selectUrisForStaging, stagingReservationCount, acceptedUris, readMediaMetadata, takePersistableReadPermission).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt`:
- Around line 433-492: The sendStagedMedia function is reentrant: it snapshots
stagedMedia, does IO, then updates state, allowing double-taps or concurrent
calls to dispatch duplicate sends; add an in-flight guard (e.g., a Boolean
property like isSending or sendInProgress) checked at the top of sendStagedMedia
and set true before starting the coroutine and false in all terminal paths
(success, failure, early returns) to prevent reentry, and also use that flag to
disable the send/remove UI controls while the operation runs (references:
sendStagedMedia, stagedMedia, stagedSnapshot, draft, replyDraft).
- Around line 509-520: The single-item selection handlers
(pickPhotoOrVideoLauncher and pickFileLauncher) currently call
sendMediaFromUri() which creates a separate message instead of joining an
existing staged batch; change both callbacks to check stagedMedia (e.g.,
stagedMedia.isNotEmpty()) and, when non-empty, add the selected URI to the
staging flow (call stageMediaFromUris(listOf(uri)) or the existing staging
helper) instead of calling sendMediaFromUri(), otherwise keep the current
behavior (use sendMediaFromUri() when stagedMedia is empty); update both
locations referencing sendMediaFromUri, pickPhotoOrVideoLauncher,
pickFileLauncher, stageMediaFromUris and stagedMedia accordingly.
- Around line 210-233: Wrap the body of readMediaMetadata(...) and
encodeMediaUploadPayload(...) in try-catch blocks that catch provider-related
exceptions (e.g., SecurityException, IllegalArgumentException,
FileNotFoundException, IOException or a general Exception fallback) so any
thrown from ContentResolver.getType, query, openInputStream, or readBytes are
handled; on exception return null (so callers like
sendMediaFromUri/sendStagedMedia fall through to existing null/toast handling)
and optionally log the exception via Android Log or your existing logger for
debugging; keep the functions' signatures and returned types unchanged
(StagedMedia? and MediaUploadPayload?) and ensure resources are still closed by
using existing use { } inside the try.
---
Duplicate comments:
In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt`:
- Around line 371-398: stageMediaFromUris currently decides capacity by
truncating acceptedUris up front, causing early invalid items to consume slots;
change the logic to compute remainingCapacity from stagedMedia.size and
stagingReservationCount, then iterate over the incoming uris (not just the
truncated acceptedUris) and process each (takePersistableReadPermission,
readMediaMetadata) inside the withContext block until you have collected up to
remainingCapacity successful StagedMedia items or run out of uris; only
increment stagingReservationCount by the number of actually reserved/retained
items, and update grantedUris/retainedUris accordingly so later valid URIs can
fill freed slots (keep referenced symbols: stageMediaFromUris,
selectUrisForStaging, stagingReservationCount, acceptedUris, readMediaMetadata,
takePersistableReadPermission).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1cef75-a343-4c37-9340-737dc3173daf
📒 Files selected for processing (2)
android/app/src/androidTest/java/com/pika/app/ui/screens/ChatScreenMediaStagingTest.ktandroid/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt
jodobear
force-pushed
the
android-issue-433-media-staging
branch
from
d79ecca to
9988a31
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt (1)
515-527:⚠️ Potential issue | 🟠 MajorBlock compose actions while attachment staging is still in flight.
stagedMediastays empty untilstageMediaFromUris()finishes its IO pass, but these guards only look atstagedSendInFlight. In that gap, tapping Send can dispatch a plainSendMessagefor the caption, and a follow-up single pick can go throughsendMediaFromUri()instead of joining the staged batch. Please treatstagingReservationCount > 0as busy state anywhere we enable send/attach actions or choose immediate-send vs staging.💡 Suggested guard pattern
+ val isStagingMedia = stagingReservationCount > 0 + fun sendDraftMessage() { + if (isStagingMedia) return if (stagedMedia.isNotEmpty()) { sendStagedMedia() return } val text = draft.trim() @@ rememberLauncherForActivityResult(ActivityResultContracts.OpenMultipleDocuments()) { uris -> - if (uris.size == 1 && currentStagedMedia.isEmpty()) { + if (uris.size == 1 && currentStagedMedia.isEmpty() && !isStagingMedia) { sendMediaFromUri(uris.first()) } else if (uris.isNotEmpty()) { stageMediaFromUris(uris) } } @@ rememberLauncherForActivityResult(ActivityResultContracts.OpenDocument()) { uri -> uri?.let { - if (currentStagedMedia.isEmpty()) sendMediaFromUri(it) else stageMediaFromUris(listOf(it)) + if (currentStagedMedia.isEmpty() && !isStagingMedia) { + sendMediaFromUri(it) + } else { + stageMediaFromUris(listOf(it)) + } } } @@ - enabled = !stagedSendInFlight, + enabled = !stagedSendInFlight && !isStagingMedia, @@ - enabled = (draft.isNotBlank() || stagedMedia.isNotEmpty()) && !stagedSendInFlight, + enabled = (draft.isNotBlank() || stagedMedia.isNotEmpty()) && + !stagedSendInFlight && + !isStagingMedia,Also applies to: 529-543, 1098-1161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt` around lines 515 - 527, The sendDraftMessage flow is allowing a plain SendMessage to go through while stageMediaFromUris() is still performing IO because it only checks stagedSendInFlight; update send/attach guard logic to treat stagingReservationCount > 0 as a busy state as well so actions join the staging flow instead of sending immediately: in sendDraftMessage (and the other send/attach decision points around sendStagedMedia, sendMediaFromUri and the send/attach UI handlers referenced at lines ~529-543 and ~1098-1161) add a pre-check that if stagingReservationCount > 0 then either return/disable send or route the caption into the staged batch (i.e., defer dispatch of AppAction.SendMessage(chat.chatId, text, null, replyDraft?.id) until stagedMedia is available), and ensure replyDraft is preserved until staging completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt`:
- Around line 515-527: The sendDraftMessage flow is allowing a plain SendMessage
to go through while stageMediaFromUris() is still performing IO because it only
checks stagedSendInFlight; update send/attach guard logic to treat
stagingReservationCount > 0 as a busy state as well so actions join the staging
flow instead of sending immediately: in sendDraftMessage (and the other
send/attach decision points around sendStagedMedia, sendMediaFromUri and the
send/attach UI handlers referenced at lines ~529-543 and ~1098-1161) add a
pre-check that if stagingReservationCount > 0 then either return/disable send or
route the caption into the staged batch (i.e., defer dispatch of
AppAction.SendMessage(chat.chatId, text, null, replyDraft?.id) until stagedMedia
is available), and ensure replyDraft is preserved until staging completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a673840b-bcde-41c6-ae10-cdb196f22628
📒 Files selected for processing (2)
android/app/src/androidTest/java/com/pika/app/ui/screens/ChatScreenMediaStagingTest.ktandroid/app/src/main/java/com/pika/app/ui/screens/ChatScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/androidTest/java/com/pika/app/ui/screens/ChatScreenMediaStagingTest.kt
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jodobear
force-pushed
the
android-issue-433-media-staging
branch
from
9988a31 to
173d4ef
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jodobear
force-pushed
the
android-issue-433-media-staging
branch
from
173d4ef to
a099f69
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jodobear
force-pushed
the
android-issue-433-media-staging
branch
from
a099f69 to
3ead9a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dispatchers.IOand treatContentResolverfailures as soft failuresTesting
app:compileDebugKotlinapp:compileDebugAndroidTestKotlinSummary by CodeRabbit
New Features
Tests