Improve iOS attachment previews and file delivery - #9907
azooz2003-bit wants to merge 74 commits into
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:
📝 WalkthroughWalkthroughThe PR replaces in-memory image attachments with file-backed image and file attachments. iOS stages and previews local files, uploads them in chunks, and sends validated attachment references through mobile terminal RPC methods. Cleanup, retry identity, authorization, and end-to-end coverage were added. ChangesFile-backed attachment contracts
Composer staging and UI
Chunked upload and delivery
Host resolution and terminal routing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Composer
participant MobileAttachmentRPCUploader
participant MobileTaskAttachmentStore
participant TerminalController
Composer->>MobileAttachmentRPCUploader: Upload staged file chunks
MobileAttachmentRPCUploader->>MobileTaskAttachmentStore: Store upload chunks
Composer->>TerminalController: Send attachment references
TerminalController->>MobileTaskAttachmentStore: Resolve completed attachment
MobileTaskAttachmentStore-->>TerminalController: Return validated file URL
TerminalController->>TerminalController: Paste shell-escaped path
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstrings`:
- Around line 2588-2683: Add "extractionState": "manual" to every new
mobile.attachment.* catalog entry, matching the metadata shape of the existing
entries. Apply this consistently to all 16 entries, including
mobile.attachment.add, mobile.attachment.done, mobile.attachment.files, and the
attachment error and preview keys, while preserving their existing
localizations.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileAttachmentRPCUploader.swift`:
- Around line 33-60: Update the upload loop in upload() to resume from the
host’s received_bytes offset when retrying an existing uploadID, seeking the
local file handle to that offset before reading the next chunk. Keep the local
offset and host acknowledgement synchronized after each response; if the staging
identity cannot be reused safely, reset it explicitly before restarting at
offset zero.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swift`:
- Around line 257-261: Remove the stagedFile deletion loop from
MobileChatEventSource, leaving temporaryURLs cleanup intact because the
transport owns those files. Ensure staged-file removal occurs only in
ChatConversationStore.discard(pendingID:) after delivery is confirmed,
preserving the pending attachment URLs until transcript reconciliation.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTestSupport.swift`:
- Around line 385-399: Update the upload handling logic around uploadFileNames
and uploadOperationIDs to validate any existing filename and operation ID before
accepting a chunk, not only when offset is zero. Reject metadata changes for
contiguous later chunks with the existing upload identity error, and only assign
the metadata when no prior value exists.
In
`@Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobilePendingAttachment.swift`:
- Around line 28-31: Update MobilePendingAttachment.data to return optional Data
and preserve read failures as nil instead of converting them to empty data. Find
every caller of MobilePendingAttachment.data, require each to handle the
optional result explicitly, and verify no reads occur from view bodies or other
main-actor paths; move affected file reads off the main actor if necessary.
- Around line 38-50: Make MobilePendingAttachment.init throw or return nil when
data.write(to:options:) fails instead of using try?, so no invalid staged
attachment is created. Update
CMUXMobileShellStore.addPendingAttachment(_:format:forTerminalID:) to propagate
that initializer failure through its existing optional result.
- Around line 53-61: Update MobilePendingAttachment.init(_:) and the
addPendingAttachment(_:forTerminalID:) retry flow so re-adding the same staged
attachment preserves its original operationID, either by storing that identity
on MobileStagedAttachment or reusing the existing MobilePendingAttachment.
Derive format from attachment.kind using the validated clipboard-format mapping,
including a safe fallback for unsupported or extensionless files, instead of
directly using the filename extension.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerSheet.swift`:
- Line 69: Document the ownership contract for initialAttachments in the
TaskComposerSheet initializer: callers must provide attachment files whose
staged copies are owned by the sheet and may be deleted by
removeStagedAttachmentFiles() during dismiss/reset. If that ownership cannot be
guaranteed, fail closed by preventing cleanup of caller-owned or shared staged
files.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerSheet`+Attachments.swift:
- Around line 246-250: Update the mobile.taskComposer.attachments.fileTooLarge
entries in ios/cmux/Resources/Localizable.xcstrings for every locale, including
en and ja, so their localized value matches “Choose a file 32 MB or smaller.”
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift`:
- Around line 582-629: Extract the duplicated attachment-staging algorithm from
stagePickedItems and stagePickedFiles into one shared helper near
MobileAttachmentStager, parameterized by the source list and per-item staging
operation. Update stagePickedItems, stagePickedFiles, stageSelectedPhotos, and
stageSelectedFiles to use this helper while preserving generation/session
guards, cancellation, capacity limits, rejected-file cleanup, error mapping,
selection cleanup, overflow reporting, and final remeasurement consistently
across both surfaces.
- Around line 692-700: Unify attachment alert localization under the
mobile.attachment.error.* namespace: update TerminalComposerView’s
staging-failure messages and OK action to use that namespace, matching the alert
title. In
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstrings
lines 2648-2683, retain those entries only if CmuxAgentChatUI resolves them;
otherwise move them to the CmuxMobileShellUI catalog so each string has one
owning package.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swift`:
- Around line 255-261: Update the attachment path validation around candidate
and parent URL construction to resolve symlinks on both the candidate path and
operationURL before comparing containment. Keep the regular-file validation and
reject any path whose resolved parent is not the resolved operation directory,
while returning the resolved candidate for downstream use.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Model/ChatOutboundAttachment.swift`:
- Around line 28-29: Update the attachment construction path that assigns
`thumbnailData` so it never stores the complete encoded image payload as preview
data. Either generate a bounded downsampled preview or set `thumbnailData` to
nil and preserve the existing view-side payload access; keep the
`ChatOutboundAttachment` contract consistent with its bounded-preview
documentation.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swift`:
- Around line 346-351: Centralize staged-file deletion in a private helper in
the store, such as releaseStagedFiles(of:), and replace the inline cleanup in
the current pending-item removal method with that helper. Update every
pending-row removal site in resetTranscriptAnchorForSourceReplacement() and the
.reset branch to invoke the helper for each removed delivered item before
removing it, preserving existing removal behavior.
In `@Sources/TerminalController`+MobileTaskAttachments.swift:
- Around line 77-83: Extract the repeated MobileTaskAttachmentStore
initialization into one shared factory helper, preserving the existing default
root URL, current date, and default file manager configuration. Update
v2MobileTaskAttachmentUpload, this handler, and v2MobileChatSend to call the
helper so all attachment operations use the same store construction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa768bd2-e0ca-4bbb-b673-8880d2005379
📒 Files selected for processing (42)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Model/ChatOutboundAttachment.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatConversationStore.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatConversationStoreTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/ChatComposerView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Composer/MobileAttachmentComponents.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstringsPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatPendingBubbleView.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileAttachmentRPCUploader.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TaskAttachments.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTests.swiftPackages/iOS/CmuxMobileShellModel/Package.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobilePendingAttachment.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/TaskComposerAttachment.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileTaskSubmissionSnapshotTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Debug/TaskComposer/TaskComposerAccessibilityPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerAttachmentPickerMenu.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerAttachmentPickerModifier.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerAttachmentStager.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerAttachmentStrip.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerMinimalLayout.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerPromptCard.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerSheet+Attachments.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TaskComposer/TaskComposerSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileAttachmentStager.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/MobileStagedAttachment.swiftPackages/iOS/CmuxMobileSupport/Tests/CmuxMobileSupportTests/MobileAttachmentStagerTests.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/MobileTaskAttachmentStoreTests.swiftSources/Mobile/MobileHostOrderedRequestQueue.swiftSources/Mobile/MobileHostService+TicketAuthorization.swiftSources/TerminalController+MobileChat.swiftSources/TerminalController+MobileTaskAttachments.swiftSources/TerminalController.swiftcmuxTests/MobileHostAuthorizationTests.swiftios/cmuxUITests/cmuxUITests.swift
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 (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift (2)
656-672: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winLimit the Files batch before staging.
urlshas no selection bound. This loop stages every selected file, even after the draft reaches its attachment cap. A large selection can cause unnecessary disk I/O and parsing before the store rejects each excess file.Snapshot the remaining capacity before starting. Stage only that prefix. Show one count-limit error for omitted selections.
Proposed fix
+ let remainingCapacity = max( + Self.maxAttachmentCount - pendingAttachments.count, + 0 + ) + let urlsToStage = urls.prefix(remainingCapacity) + if urls.count > urlsToStage.count { + attachmentError = attachmentAdmissionErrorMessage(.perTerminalCountLimit) + } let sessionGeneration = store.currentSessionGeneration stagingTask.task?.cancel() @@ - for url in urls { + for url in urlsToStage {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift` around lines 656 - 672, Update the Files batch flow around the urls staging loop to snapshot the remaining attachment capacity before staging, process only the permitted prefix, and avoid staging omitted selections. When URLs exceed capacity, show a single count-limit error for the omitted files while preserving the existing cancellation and generation checks for staged attachments.
596-630: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent stale staging tasks from mutating the active composer.
Cancellation does not guarantee that an awaited import or staging call throws
CancellationError. A stale task can still setattachmentError; the photo path can also clear a newerpickerSelection. Recheck the generation and session token before every UI-state write after an await.
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift#L596-L630: guardattachmentError,pickerSelection, and the final remeasure against the current generation and session.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift#L656-L680: guardattachmentErrorand the final remeasure against the current generation and session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift` around lines 596 - 630, Prevent stale staging tasks from updating the active composer by rechecking the staging generation and session generation before every post-await UI mutation. In TerminalComposerView.swift lines 596-630, guard attachmentError assignments, pickerSelection clearing, and the final requestHeightRemeasure call; in lines 656-680, guard attachmentError assignments and the final requestHeightRemeasure call. Preserve cancellation and cleanup behavior while ensuring stale tasks exit without mutating current state.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
7412-7476: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse the backing file as the byte-count source of truth.
Admission trusts caller-provided
MobileStagedAttachment.byteCount. The test helper writes one byte for every attachment while declaring arbitrary sizes. A replaced or malformed staged file can therefore bypass draft quotas or later upload a different byte count than the admitted attachment.
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L7412-L7476: validate thatlocalFileURLis a regular file and derive the accepted byte count from its current file metadata before applying limits and storing the attachment.Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swift#L29-L44: create files with the declared size and add coverage that mismatched or missing backing files are rejected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 7412 - 7476, Update admitPendingAttachment to require stagedAttachment.localFileURL to reference a regular file, read its current file size, and use that value as the accepted attachment byteCount before applying per-item, per-terminal, and global limits or storing the attachment. In Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swift:29-44, make helper files match declared sizes and add coverage rejecting mismatched and missing backing files.Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swift (1)
183-200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPut the attachment store under upload-id serialization.
MobileTaskAttachmentStore.uploadis shared filesystem state without an actor, queue, or documented serialized owner. Two concurrent valid requests for the sameuploadIDcan read the same staged offset, truncate, and append conflicting chunks. Use one actor per upload identity for the size check/truncate/write/finalize path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swift` around lines 183 - 200, Serialize the full upload mutation path in MobileTaskAttachmentStore.upload by routing each uploadID through a dedicated per-upload actor, including offset validation, truncation, append, and finalization. Reuse the same actor for all requests with the same uploadID, while allowing different upload IDs to proceed independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 7412-7476: Update admitPendingAttachment to require
stagedAttachment.localFileURL to reference a regular file, read its current file
size, and use that value as the accepted attachment byteCount before applying
per-item, per-terminal, and global limits or storing the attachment. In
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swift:29-44,
make helper files match declared sizes and add coverage rejecting mismatched and
missing backing files.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift`:
- Around line 656-672: Update the Files batch flow around the urls staging loop
to snapshot the remaining attachment capacity before staging, process only the
permitted prefix, and avoid staging omitted selections. When URLs exceed
capacity, show a single count-limit error for the omitted files while preserving
the existing cancellation and generation checks for staged attachments.
- Around line 596-630: Prevent stale staging tasks from updating the active
composer by rechecking the staging generation and session generation before
every post-await UI mutation. In TerminalComposerView.swift lines 596-630, guard
attachmentError assignments, pickerSelection clearing, and the final
requestHeightRemeasure call; in lines 656-680, guard attachmentError assignments
and the final requestHeightRemeasure call. Preserve cancellation and cleanup
behavior while ensuring stale tasks exit without mutating current state.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swift`:
- Around line 183-200: Serialize the full upload mutation path in
MobileTaskAttachmentStore.upload by routing each uploadID through a dedicated
per-upload actor, including offset validation, truncation, append, and
finalization. Reuse the same actor for all requests with the same uploadID,
while allowing different upload IDs to proceed independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54df5ec6-e031-4e8b-9707-95f421f5f0c2
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/MobileTaskAttachments/MobileTaskAttachmentStore.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/MobileTaskAttachmentStoreTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ab3159d. Configure here.

Summary
Verification
Closes #6643
Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Touches composer send routing, large-file I/O, and new RPC delivery paths across chat and terminal; mistakes could leak temp files or drop attachments mid-retry, but auth is not redesigned and errors are sanitized for display.
Overview
iOS composers (terminal, agent chat, new task) now stage Photos and Files as app-owned on-disk payloads with shared card UI (preview, remove, preparing state, localized limits/errors), instead of holding full image bytes in composer state.
Delivery moves from inline base64 to
mobile.task.attachment.uploadchunking with stableupload_id/operation_id(retries reuse identities; completed uploads can short-circuit re-upload), then host references viamobile.chat.send,mobile.terminal.paste_attachment, or the existing task-create path.CmuxMobileAttachmentTransfercentralizes upload + privacy-safe transfer errors.Chat store assigns a shared operation per prompt, releases staged files when pending rows reconcile, discard, reset, or change event source. Terminal shell adds typed admission results, raises per-terminal total staging to 64 MB, and deletes staged files on sign-out, topology prune, and clear/remove.
Reviewed by Cursor Bugbot for commit c8a4320. Bugbot is set up for automated code reviews on this repo. Configure here.