iOS composer: image attachments, attach button, autocorrect, padding - #6102
Conversation
Add per-terminal pending-attachment state to the mobile shell store so picked images can be staged as drafts (keyed by terminal id, like the text draft) and sent on the next composer submit, reusing the existing terminal.paste_image transport. - New MobilePendingAttachment value type (data + lowercase format + stable id), host-testable (no UIKit). - Store add/remove/clear/read methods plus composerCanSend (text non-empty OR attachments present, so an images-only send is allowed). - submitComposer() sends staged images in pick order (awaited) then the text, then clears the staged set for the submitted terminal. - Unit tests for add/remove/clear, per-terminal keying, send gating, and clear-after-send. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace the chevron.down (hide-composer) button to the left of the field with a paperclip attach button that opens a PhotosUI photo picker (multi-select). Composer dismissal still lives on the accessory toolbar's compose toggle. - Picked images are encoded the same way the clipboard paste path encodes them (PNG, JPEG fallback over ~8MB) and staged as pending attachments. - Render staged attachments as a horizontal row of removable thumbnail chips above the text field (iMessage style). - Send is enabled when text is non-empty OR attachments are staged; send routes through store.submitComposer() (images first, then text) and re-measures the band height. - Composer now uses normal text assistance (autocorrect on, sentence-case) since it is natural language to an agent; the raw terminal input is unchanged. - Reduce the top padding above the field (was 8pt vertical, now 2pt top / 8pt bottom) so the composer sits tighter; band measurement still driven by content + padding. - Add NSPhotoLibraryUsageDescription to Info.plist and en/ja strings for the attach and remove-attachment labels. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 end-to-end photo attachment support to the iOS mobile composer. A new ChangesPhoto Attachment Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalComposerView
participant MobileShellComposite
participant TerminalSession
rect rgba(173, 216, 230, 0.5)
Note over User,TerminalComposerView: submitComposer() captures terminal once
end
User->>TerminalComposerView: tap send
TerminalComposerView->>MobileShellComposite: submitComposer()
MobileShellComposite->>MobileShellComposite: snapshot currentWorkspace + selectedTerminal
loop Each staged MobilePendingAttachment (pick order)
MobileShellComposite->>TerminalSession: submitTerminalPasteImage(data, format)
TerminalSession-->>MobileShellComposite: true (acked)
MobileShellComposite->>MobileShellComposite: removePendingAttachment(id)
end
MobileShellComposite->>MobileShellComposite: submitComposerInput() for captured terminal
MobileShellComposite-->>TerminalComposerView: complete
TerminalComposerView->>TerminalComposerView: requestHeightRemeasure()
sequenceDiagram
participant User
participant TerminalComposerView
participant PhotosPicker
participant MobileShellComposite
rect rgba(255, 228, 196, 0.5)
Note over User,TerminalComposerView: User picks images for staging
end
User->>TerminalComposerView: tap paperclip
TerminalComposerView->>PhotosPicker: present (images only)
PhotosPicker-->>TerminalComposerView: pickerSelection changed
TerminalComposerView->>TerminalComposerView: stagePickedItems(_:)
par Async per-image
TerminalComposerView->>TerminalComposerView: Task.detached encode()<br/>(PNG ≤8MB else JPEG 0.8)
TerminalComposerView->>TerminalComposerView: downsampledThumbnailData(from:)<br/>via ImageIO
end
TerminalComposerView->>MobileShellComposite: addPendingAttachment(data, format)
MobileShellComposite-->>TerminalComposerView: attachment id
TerminalComposerView->>TerminalComposerView: cache thumbnail<br/>clear picker<br/>requestHeightRemeasure()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20a18767a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Clear what we sent (only this terminal's staged set) before the text | ||
| // submit so the chip row empties immediately. | ||
| if !attachments.isEmpty { | ||
| clearPendingAttachments(forTerminalID: submittedTerminalID?.rawValue) |
There was a problem hiding this comment.
When the Mac is disconnected or terminal.paste_image rejects a photo (for example because the JPEG still exceeds the Mac-side limit or the queue is full), submitTerminalPasteImage returns after recording the error but does not report failure. This block still clears every staged attachment, so the user loses the only retryable copy while nothing was delivered; mirror the text path by clearing only after acknowledged image sends.
Useful? React with 👍 / 👎.
| // Deliver each image first and await it, so the agent's terminal has the | ||
| // file paths before the text arrives. | ||
| for attachment in attachments { | ||
| await submitTerminalPasteImage(attachment.data, format: attachment.format) |
There was a problem hiding this comment.
If a user taps Send with multiple photos and switches terminals while the first await is in flight, the next iteration calls submitTerminalPasteImage, which re-reads selectedWorkspace/selectedTerminalID instead of using submittedTerminalID. That sends later photos to the newly selected terminal while clearing the original terminal’s staged chips, so a single composed message can be split across terminals.
Useful? React with 👍 / 👎.
| /// Allowed with empty text as long as at least one attachment is staged; an | ||
| /// images-only send skips the (no-op) text submit. Captures the submitted | ||
| /// terminal up front so a mid-flight terminal switch clears the right key. | ||
| public func submitComposer() async { |
There was a problem hiding this comment.
The existing re-entrancy guard is inside submitComposerInput, so it never protects the image loop, and images-only sends never enter it at all. With a photo staged, a second tap while the first image RPC is awaiting starts another submitComposer() that captures the same attachments before they are cleared, causing duplicate image paths to be injected into the terminal.
Useful? React with 👍 / 👎.
| <key>NSLocalNetworkUsageDescription</key> | ||
| <string>Connect to your Mac on the local network for cmux mobile pairing and terminal sync.</string> | ||
| <key>NSPhotoLibraryUsageDescription</key> | ||
| <string>Attach photos to send to your terminal agent.</string> |
There was a problem hiding this comment.
The repo’s /workspace/cmux/AGENTS.md requires all user-facing strings to be localized, and this target already localizes Info.plist privacy prompts in ios/cmux/Resources/InfoPlist.xcstrings. Adding NSPhotoLibraryUsageDescription only as an English plist value means any iOS photo-library permission prompt/settings text falls back to English for Japanese users; add the key to InfoPlist.xcstrings with both locales.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds iMessage-style photo attachment support to the iOS terminal composer: a paperclip button opens
Confidence Score: 5/5Safe to merge — all three previously flagged concerns (re-entrancy gap, main-actor encoding, missing xcstrings entry) are addressed in this diff, and no new correctness issues were found. The store-side logic is carefully designed: caps are enforced atomically on @mainactor, session and connection generations are both checked before each image send, the text snapshot is captured before any await, and the per-terminal/global dual-cap model is thoroughly exercised by the new tests. The view's ImageIO path correctly uses No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant View as TerminalComposerView
participant Store as MobileShellComposite
participant Host as Mac Host (RPC)
User->>View: tap Send
View->>View: guard canSend
View->>Store: submitComposer()
Store->>Store: "guard !isSubmittingComposer / isSubmittingComposer = true"
Store->>Store: snapshot workspaceID, terminalID, text, attachments, signInGeneration, connectionGeneration
loop For each staged attachment (in pick order)
Store->>Store: guard isComposerSubmitIdentityCurrent()
Store->>Store: guard attachment still staged
Store->>Host: await terminal.paste_image(data, format)
Host-->>Store: ack
Store->>Store: removePendingAttachment(id)
end
Store->>Store: guard isComposerSubmitIdentityCurrent()
Store->>Store: submitComposerInput(workspaceID, terminalID, capturedText)
Store->>Host: await terminal.paste(text)
Host-->>Store: ack
Store->>Store: reconcileComposerDraftAfterSend()
Store->>Store: "isSubmittingComposer = false"
Store-->>View: return
View->>View: thumbnailCache.retain(remaining ids)
View->>View: requestHeightRemeasure()
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant View as TerminalComposerView
participant Store as MobileShellComposite
participant Host as Mac Host (RPC)
User->>View: tap Send
View->>View: guard canSend
View->>Store: submitComposer()
Store->>Store: "guard !isSubmittingComposer / isSubmittingComposer = true"
Store->>Store: snapshot workspaceID, terminalID, text, attachments, signInGeneration, connectionGeneration
loop For each staged attachment (in pick order)
Store->>Store: guard isComposerSubmitIdentityCurrent()
Store->>Store: guard attachment still staged
Store->>Host: await terminal.paste_image(data, format)
Host-->>Store: ack
Store->>Store: removePendingAttachment(id)
end
Store->>Store: guard isComposerSubmitIdentityCurrent()
Store->>Store: submitComposerInput(workspaceID, terminalID, capturedText)
Store->>Host: await terminal.paste(text)
Host-->>Store: ack
Store->>Store: reconcileComposerDraftAfterSend()
Store->>Store: "isSubmittingComposer = false"
Store-->>View: return
View->>View: thumbnailCache.retain(remaining ids)
View->>View: requestHeightRemeasure()
Reviews (8): Last reviewed commit: "iOS composer: abort submit on session/co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <key>NSPhotoLibraryUsageDescription</key> | ||
| <string>Attach photos to send to your terminal agent.</string> |
There was a problem hiding this comment.
NSPhotoLibraryUsageDescription entry in InfoPlist.xcstrings
InfoPlist.xcstrings already holds NSCameraUsageDescription and NSLocalNetworkUsageDescription each with en + ja entries — that file is the authoritative source for localized Info.plist strings. Adding NSPhotoLibraryUsageDescription only to Info.plist as a hardcoded English string means iOS will always show English to Japanese users instead of pulling the catalog translation. The fix is to add this key (with both locales) to InfoPlist.xcstrings instead; the raw Info.plist entry should then also be removed, since the catalog value takes precedence when both exist.
Rule Used: Flag production user-facing text that is not fully... (source)
| public func submitComposer() async { | ||
| let submittedTerminalID = selectedTerminalID | ||
| let attachments = pendingAttachments(forTerminalID: submittedTerminalID?.rawValue) | ||
| // Deliver each image first and await it, so the agent's terminal has the | ||
| // file paths before the text arrives. | ||
| for attachment in attachments { | ||
| await submitTerminalPasteImage(attachment.data, format: attachment.format) | ||
| } | ||
| // Clear what we sent (only this terminal's staged set) before the text | ||
| // submit so the chip row empties immediately. | ||
| if !attachments.isEmpty { | ||
| clearPendingAttachments(forTerminalID: submittedTerminalID?.rawValue) | ||
| } | ||
| // Submit the text (a no-op when empty, e.g. an images-only send). | ||
| await submitComposerInput() | ||
| } |
There was a problem hiding this comment.
submitComposer() has no re-entrancy guard
submitComposerInput() uses isSubmittingComposerInput specifically to prevent a double-tap from sending the same text twice. submitComposer() has no equivalent guard. If the user double-taps Send while images are still being delivered (between await submitTerminalPasteImage calls), a second invocation captures the same attachments snapshot before clearPendingAttachments runs and sends every image a second time. The view's send() function creates a new Task { @MainActor in ... } each call and only checks canSend—which is still true until the clear finishes—so both tasks proceed. Extending the isSubmittingComposerInput flag (or an equivalent dedicated flag) to gate the full submitComposer() body would close this window.
| if let png = image.pngData(), png.count <= Self.maxImageBytes { | ||
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | ||
| } else if let jpeg = image.jpegData(compressionQuality: 0.8) { | ||
| store.addPendingAttachment(jpeg, format: "jpg", forTerminalID: terminalID) | ||
| } else if let png = image.pngData() { | ||
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | ||
| } |
There was a problem hiding this comment.
pngData() called twice in the over-size fallback path
When PNG exceeds the size cap and JPEG encoding also fails (rare but possible), image.pngData() is called again and the oversized PNG is staged anyway. This means the expensive PNG render runs twice in the worst case—first for the size check, then for the fallback—doubling the main-actor block time. Cache the first result to avoid the redundant encode.
| if let png = image.pngData(), png.count <= Self.maxImageBytes { | |
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | |
| } else if let jpeg = image.jpegData(compressionQuality: 0.8) { | |
| store.addPendingAttachment(jpeg, format: "jpg", forTerminalID: terminalID) | |
| } else if let png = image.pngData() { | |
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | |
| } | |
| let png = image.pngData() | |
| if let png, png.count <= Self.maxImageBytes { | |
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | |
| } else if let jpeg = image.jpegData(compressionQuality: 0.8) { | |
| store.addPendingAttachment(jpeg, format: "jpg", forTerminalID: terminalID) | |
| } else if let png { | |
| store.addPendingAttachment(png, format: "png", forTerminalID: terminalID) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@ios/Config/Info.plist`:
- Around line 58-59: Add the NSPhotoLibraryUsageDescription localization entry
to ios/cmux/Resources/InfoPlist.xcstrings to match the permission description
added to ios/Config/Info.plist. Follow the existing pattern used for
NSCameraUsageDescription, creating an entry with the key
NSPhotoLibraryUsageDescription that includes both English and Japanese
localizations. The English value should be "Attach photos to send to your
terminal agent." and you must provide the appropriate Japanese translation for
the "ja" localization. Ensure the entry uses the same structure with
extractionState set to "manual" and state set to "translated" for both language
variants.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2795-2810: The submitComposer() method captures
submittedTerminalID but then calls submitTerminalPasteImage(...) and
submitComposerInput() without passing this captured value, causing them to
resolve selectedTerminalID at execution time instead. If the user changes the
selected terminal during the awaits, the attachments and text will be sent to
the wrong terminal. Fix this by modifying both submitTerminalPasteImage(...) and
submitComposerInput() to accept the captured submittedTerminalID as a parameter
and use it directly, ensuring messages are always sent to the terminal that was
selected when the user clicked submit.
- Around line 313-317: The pendingAttachmentsByTerminalID dictionary stores
unsent user content but is not cleared during sign out in the signOut() method,
which can leak one user's staged images to the next session on a shared device.
Add a line in the signOut() method to clear this dictionary by setting it to an
empty dictionary, ensuring all pending attachments are removed when the user
signs out.
- Around line 2801-2807: The clearPendingAttachments call at line 2806 executes
unconditionally after submitTerminalPasteImage, which is fire-and-forget with a
Void return type. This means attachments are cleared even if the image RPC
fails, losing user data with no retry path. Modify submitTerminalPasteImage to
return a Result type or Bool indicating success/failure, or wrap the attachment
clearing logic with error handling so that clearPendingAttachments is only
called after confirming that all image sends completed successfully. If any
attachment fails to send, retain it in the staged set so the user can retry.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swift`:
- Around line 28-123: Add three new edge case test methods to the
ComposerPendingAttachmentTests class: First, add a test that calls
removePendingAttachment with a UUID that does not correspond to any attachment
and verify it is a no-op without crashing. Second, add a test that calls
clearPendingAttachments with a terminal ID that does not exist and verify it is
a no-op. Third, add a test that sets both terminalInputText to a non-empty value
and adds a pending attachment, then calls composerCanSend to verify it returns
true, explicitly testing the combined OR logic end-to-end. Each test should
follow the same setup pattern as the existing tests using Self.makeComposite()
and should verify the expected behavior using `#expect` assertions.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift`:
- Around line 325-341: The stagePickedItems function currently runs all photo
loading and transcoding operations on the main actor, which blocks the UI thread
while decoding and re-encoding large photos. Remove the `@MainActor` annotation
from the Task so the item.loadTransferable, UIImage initialization, and
pngData/jpegData calls execute off-main. Then wrap only the
store.addPendingAttachment, pickerSelection assignment, and
requestHeightRemeasure calls in a separate `@MainActor-isolated` block to ensure
those UI-related operations run on the main thread.
- Around line 267-272: The photosPicker configuration sets maxSelectionCount to
nil, allowing unlimited image selection in a single batch, and the code does not
validate the encoded byte size before calling addPendingAttachment, which can
cause memory spikes and stage payloads exceeding the transport limit. Set a
reasonable maxSelectionCount value in the photosPicker call (around lines
267-272), and add size validation logic in the image encoding/attachment staging
flow (also around lines 330-335) to check the encoded attachment size against
the transport limit before adding it as a pending attachment; reject or
downscale any attachment that exceeds this limit to prevent oversized payloads
from being staged.
🪄 Autofix (Beta)
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
Run ID: 2c1fce16-a566-4172-b80c-0ed831df7e51
📒 Files selected for processing (6)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobilePendingAttachment.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftios/Config/Info.plistios/cmux/Resources/Localizable.xcstrings
| @Test func addAppendsInPickOrder() { | ||
| let composite = Self.makeComposite() | ||
| composite.addPendingAttachment(Self.bytes("one"), format: "png", forTerminalID: "term-a") | ||
| composite.addPendingAttachment(Self.bytes("two"), format: "jpg", forTerminalID: "term-a") | ||
|
|
||
| let staged = composite.pendingAttachments(forTerminalID: "term-a") | ||
| #expect(staged.count == 2) | ||
| #expect(staged[0].data == Self.bytes("one")) | ||
| #expect(staged[0].format == "png") | ||
| #expect(staged[1].data == Self.bytes("two")) | ||
| #expect(staged[1].format == "jpg") | ||
| } | ||
|
|
||
| @Test func addIgnoresEmptyData() { | ||
| let composite = Self.makeComposite() | ||
| composite.addPendingAttachment(Data(), format: "png", forTerminalID: "term-a") | ||
| #expect(composite.pendingAttachments(forTerminalID: "term-a").isEmpty) | ||
| } | ||
|
|
||
| @Test func removeDropsOnlyTheTargetedAttachment() { | ||
| let composite = Self.makeComposite() | ||
| composite.addPendingAttachment(Self.bytes("one"), format: "png", forTerminalID: "term-a") | ||
| composite.addPendingAttachment(Self.bytes("two"), format: "png", forTerminalID: "term-a") | ||
| let toRemove = composite.pendingAttachments(forTerminalID: "term-a")[0].id | ||
|
|
||
| composite.removePendingAttachment(id: toRemove, forTerminalID: "term-a") | ||
|
|
||
| let staged = composite.pendingAttachments(forTerminalID: "term-a") | ||
| #expect(staged.count == 1) | ||
| #expect(staged[0].data == Self.bytes("two")) | ||
| } | ||
|
|
||
| @Test func clearEmptiesOnlyTheGivenTerminal() { | ||
| let composite = Self.makeComposite() | ||
| composite.addPendingAttachment(Self.bytes("a1"), format: "png", forTerminalID: "term-a") | ||
| composite.addPendingAttachment(Self.bytes("b1"), format: "png", forTerminalID: "term-b") | ||
|
|
||
| composite.clearPendingAttachments(forTerminalID: "term-a") | ||
|
|
||
| #expect(composite.pendingAttachments(forTerminalID: "term-a").isEmpty) | ||
| #expect(composite.pendingAttachments(forTerminalID: "term-b").count == 1) | ||
| } | ||
|
|
||
| @Test func attachmentsAreKeyedPerTerminal() { | ||
| let composite = Self.makeComposite() | ||
| composite.addPendingAttachment(Self.bytes("a1"), format: "png", forTerminalID: "term-a") | ||
| composite.addPendingAttachment(Self.bytes("b1"), format: "png", forTerminalID: "term-b") | ||
| composite.addPendingAttachment(Self.bytes("b2"), format: "png", forTerminalID: "term-b") | ||
|
|
||
| #expect(composite.pendingAttachments(forTerminalID: "term-a").count == 1) | ||
| #expect(composite.pendingAttachments(forTerminalID: "term-b").count == 2) | ||
| } | ||
|
|
||
| @Test func defaultsToSelectedTerminalWhenIDOmitted() { | ||
| let composite = Self.makeComposite() | ||
| // Selected terminal is term-a (set at init). | ||
| composite.addPendingAttachment(Self.bytes("sel"), format: "png") | ||
| #expect(composite.pendingAttachments().count == 1) | ||
| #expect(composite.pendingAttachments(forTerminalID: "term-a").count == 1) | ||
| #expect(composite.pendingAttachments(forTerminalID: "term-b").isEmpty) | ||
| } | ||
|
|
||
| @Test func canSendWhenTextEmptyButAttachmentPresent() { | ||
| let composite = Self.makeComposite() | ||
| composite.terminalInputText = "" | ||
| #expect(composite.composerCanSend(forTerminalID: "term-a") == false) | ||
|
|
||
| composite.addPendingAttachment(Self.bytes("img"), format: "png", forTerminalID: "term-a") | ||
| #expect(composite.composerCanSend(forTerminalID: "term-a") == true) | ||
| } | ||
|
|
||
| @Test func canSendWhenTextPresentButNoAttachment() { | ||
| let composite = Self.makeComposite() | ||
| composite.terminalInputText = "hello" | ||
| #expect(composite.composerCanSend(forTerminalID: "term-a") == true) | ||
| } | ||
|
|
||
| @Test func cannotSendWhenTextWhitespaceAndNoAttachment() { | ||
| let composite = Self.makeComposite() | ||
| composite.terminalInputText = " \n " | ||
| #expect(composite.composerCanSend(forTerminalID: "term-a") == false) | ||
| } | ||
|
|
||
| @Test func submitClearsAttachmentsForSubmittedTerminal() async { | ||
| let composite = Self.makeComposite() | ||
| // No remoteClient is wired, so the image/text RPCs are no-ops, but the | ||
| // clear-after-send of the staged set still runs (it does not depend on | ||
| // the wire). | ||
| composite.addPendingAttachment(Self.bytes("img"), format: "png", forTerminalID: "term-a") | ||
| composite.terminalInputText = "" | ||
|
|
||
| await composite.submitComposer() | ||
|
|
||
| #expect(composite.pendingAttachments(forTerminalID: "term-a").isEmpty) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: Consider adding edge case tests for defensive coverage.
The current test suite has solid coverage of the core behavior. For even more defensive coverage, consider adding:
- Remove with non-existent ID: Verify that
removePendingAttachment(id: UUID(), forTerminalID: "term-a")is a no-op and doesn't crash. - Clear with non-existent terminal: Verify that
clearPendingAttachments(forTerminalID: "non-existent")is a no-op. - Send gating with both text AND attachment: Explicitly test
composerCanSendreturnstruewhen both conditions are satisfied (to verify the OR logic end-to-end, though the individual branches are already covered).
These are nice-to-haves; the current coverage is comprehensive for the feature scope.
🤖 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/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swift`
around lines 28 - 123, Add three new edge case test methods to the
ComposerPendingAttachmentTests class: First, add a test that calls
removePendingAttachment with a UUID that does not correspond to any attachment
and verify it is a no-op without crashing. Second, add a test that calls
clearPendingAttachments with a terminal ID that does not exist and verify it is
a no-op. Third, add a test that sets both terminalInputText to a non-empty value
and adds a pending attachment, then calls composerCanSend to verify it returns
true, explicitly testing the combined OR logic end-to-end. Each test should
follow the same setup pattern as the existing tests using Self.makeComposite()
and should verify the expected behavior using `#expect` assertions.
The touched file is already well over any reasonable size; splitting it is a separate refactor and stored properties cannot move to an extension. Accept the incremental growth as known debt so the budget guard reflects reality.
Three P1 autoreview fixes for the iOS composer image attachments: 1. Capture target terminal once in submitComposer. The image and text sends now thread an explicit workspace + terminal id captured at submit time, so a terminal switch while an awaited image send is in flight can no longer reroute later images or the text to whatever is selected at that moment. Adds internal targeted variants: submitTerminalPasteImage(_:format:workspaceID:terminalID:) and submitComposerInput(workspaceID:terminalID:); the public selection-based overloads call them, preserving the clipboard-paste path. 2. Keep attachments on a failed send. The image send path now returns Bool (threaded through sendRemoteTerminalPasteImage). submitComposer removes each attachment only after its send is acknowledged; on a failure it stops, keeps the remaining and failed attachments staged, and does not submit the text, matching the text-keep-on-failure semantics. 3. Bound staging and stop per-render decodes. The picker is capped at 10 and a 32 MB total byte budget is enforced when staging. Each attachment gets a small downsampled thumbnail built once off the main thread (via ImageIO) and cached by id; the chip renders the cached thumbnail instead of decoding the full Data in the view body on every keystroke. Tests: ComposerPendingAttachmentTests updated for keep-on-failure; new ComposerSubmitRoutingTests drives submitComposer end to end over the real paste RPC frames (captured-terminal routing, mid-send switch, full and partial image-send failures). Budget TSV refreshed for the two grown files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the three P1 autoreview findings (commit 4ba9f3f): 1. Wrong-terminal routing after a mid-send switch. 2. Failed sends no longer lose photos. The image send path returns 3. Bounded staging, no per-render full-image decode. Picker capped at Final API shape (image/text send):
Tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ba9f3f9e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| // Submit the text to the captured terminal (a no-op when empty, e.g. an | ||
| // images-only send). All images acked by here, so the text follows. | ||
| await submitComposerInput(workspaceID: workspaceID, terminalID: submittedTerminalID) |
There was a problem hiding this comment.
When a send includes attachments, the text is not captured at tap time; after each awaited image upload this calls submitComposerInput, which re-reads the live terminalInputText. If the user starts typing the next message while a large photo is still uploading, that newer draft is sent with the old images and can then be cleared by the post-ack reconciliation. Capture the text before the image loop and pass that snapshot to the text-send path.
Useful? React with 👍 / 👎.
| if let jpeg = image.jpegData(compressionQuality: 0.8) { | ||
| return (jpeg, "jpg") |
There was a problem hiding this comment.
For picked photos whose PNG exceeds the cap, this returns the 0.8-quality JPEG without checking its size, even though the Mac-side saveImageData rejects payloads over the 10 MB image limit. A high-resolution photo whose JPEG is still larger than that limit can be staged under the 32 MB total budget, but every send attempt will fail and leave the attachment stuck until the user removes it; keep compressing or skip the pick when the encoded bytes are still too large.
Useful? React with 👍 / 👎.
# Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2836-2861: The submitComposer() function is vulnerable to
concurrent calls that can both snapshot and send the same staged attachments
before either removes them after acknowledgment, causing duplicate image sends
on double-tap. Add a new stored property isSubmittingComposer (similar to the
existing isSubmittingComposerInput flag) alongside other composer submit state,
then guard the entire submitComposer() function with this flag at the beginning
to prevent concurrent execution. This prevents both attachment snapshots and
sends from being duplicated, not just the text input portion.
- Around line 2787-2795: The submitComposerInput method reads the live
terminalInputText field, but since submitComposer awaits image uploads first,
the user could edit the input or switch terminals during that await, causing the
wrong text to be submitted. Modify the submitComposerInput method signature to
accept a submittedText parameter instead of reading terminalInputText directly
from the live field. Then in the caller (submitComposer method), capture the
text value by snapshotting terminalInputText before any awaits on image uploads
occur, and pass this captured text as the submittedText argument when calling
submitComposerInput.
🪄 Autofix (Beta)
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
Run ID: 6886c6f3-bb46-44c8-a38c-42f3a0619a7b
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b9f284e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ForEach(pendingAttachments) { attachment in | ||
| AttachmentChip(thumbnail: thumbnailCache.image(for: attachment.id)) { |
There was a problem hiding this comment.
When a user stages a photo, switches terminals, and then returns, WorkspaceDetailView keys the terminal surface by terminalID, so this composer view and its @State AttachmentThumbnailCache are recreated empty while the store still restores the per-terminal pendingAttachments. Every retained chip then gets nil here and renders only the generic placeholder, leaving the user unable to identify which photo will be sent; rebuild the cache from the stored attachment data on mount or keep the thumbnail with the attachment state.
Useful? React with 👍 / 👎.
1. Swift 6 Sendable: PreparedAttachment no longer carries a non-Sendable UIImage across the detached thumbnail-prep task. The off-main path keeps ImageIO downsampling but returns the thumbnail as PNG Data; the UIImage is built on the main actor when populating the chip cache. 2. Privacy: clear pendingAttachmentsByTerminalID in signOut(), alongside the text-draft wipe, so a previous account's staged photo bytes cannot resurface on a reused terminal id. 3. Re-entrancy: add isSubmittingComposer guard around the whole submitComposer() (images + text), so a double tap on Send cannot re-upload the still-staged attachments while the first RPC awaits. Failure still keeps attachments. 4. UI test: testComposerSurvivesRepeatedOpenCloseCycles closes the composer via the accessory compose toggle instead of the removed MobileComposerClose chevron; drop the now-unused bandClose constant. 5. Localize NSPhotoLibraryUsageDescription in InfoPlist.xcstrings (en + ja). Add host tests for sign-out clearing (finding 2) and double-submit guard (finding 3). Refresh the Swift file-length budget for the two touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Round-2 autoreview fixes (96dcff1):
Tests: added host tests for sign-out clearing and the double-submit guard; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96dcff1f6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| // Submit the text to the captured terminal (a no-op when empty, e.g. an | ||
| // images-only send). All images acked by here, so the text follows. | ||
| await submitComposerInput(workspaceID: workspaceID, terminalID: submittedTerminalID) |
There was a problem hiding this comment.
When the user taps Send with only attachments, the image RPCs only inject escaped file paths into the terminal (TerminalController.v2MobileTerminalPasteImage calls sendInputResult(escapedPath) and does not press Return), and this final text submit is a no-op because submitComposerInput returns success for empty text. The chips are then removed even though the agent prompt is left with unsubmitted paths, so an images-only composer send does not actually submit anything; send a submit key/Return after successful images when the text snapshot is empty.
Useful? React with 👍 / 👎.
| /// Encode a picked image the way the clipboard paste path does: PNG when it | ||
| /// fits the per-image cap, otherwise JPEG, falling back to PNG. | ||
| private static func encode(_ image: UIImage) -> (data: Data, format: String)? { | ||
| if let png = image.pngData(), png.count <= maxImageBytes { |
There was a problem hiding this comment.
This accepts PNG attachments up to 8 MiB, but the mobile RPC request wraps the bytes as base64 inside JSON before MobileCoreRPCSession calls MobileSyncFrameCodec.encodeFrame, whose default frame limit is also 8 MiB. A selected PNG around 6.5–8 MiB passes this check, then expands past the frame limit and fails locally on every send attempt while remaining staged; use a lower limit based on the framed/base64 payload or compress before staging.
Useful? React with 👍 / 👎.
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)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2769-2772:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the text half of
composerCanSendto the requested terminal.
terminalIDscopes the attachment lookup, butterminalInputTextis always the selected terminal’s draft. A caller checking a non-selected terminal can get Send enabled because another terminal has text.Suggested fix
public func composerCanSend(forTerminalID terminalID: String? = nil) -> Bool { - let textNonEmpty = !terminalInputText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - return textNonEmpty || !pendingAttachments(forTerminalID: terminalID).isEmpty + let key = terminalID ?? selectedTerminalID?.rawValue + let textNonEmpty = key == selectedTerminalID?.rawValue + && !terminalInputText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + return textNonEmpty || !pendingAttachments(forTerminalID: key).isEmpty }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2769 - 2772, The composerCanSend method accepts a terminalID parameter to scope checks to a specific terminal, but the textNonEmpty check uses terminalInputText which always refers to the selected terminal's draft, causing incorrect Send enablement for non-selected terminals. Modify the method to retrieve and check the input text for the specific terminal identified by the terminalID parameter, rather than always checking the selected terminal's terminalInputText. This ensures both the text and attachment checks are properly scoped to the requested terminal.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2769-2772: The composerCanSend method accepts a terminalID
parameter to scope checks to a specific terminal, but the textNonEmpty check
uses terminalInputText which always refers to the selected terminal's draft,
causing incorrect Send enablement for non-selected terminals. Modify the method
to retrieve and check the input text for the specific terminal identified by the
terminalID parameter, rather than always checking the selected terminal's
terminalInputText. This ensures both the text and attachment checks are properly
scoped to the requested terminal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecad115b-bfb0-451f-a56e-6d7fad1aa15b
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftios/cmux/Resources/InfoPlist.xcstringsios/cmuxUITests/cmuxUITests.swift
Two async/privacy bugs from the codex autoreview: 1. submitComposer captured the target terminal but re-read the live terminalInputText after the awaited image paste_image RPCs. A terminal switch (draft swap) or a field edit during those awaits could skip the composed text or paste a different terminal's draft. Snapshot the text at the very start of submitComposer (before any await) and thread it through submitComposerInput via a new capturedText param; the post-send reconcile already keys on that same sent text, so a mid-send edit is preserved (cleared only when the field still equals the snapshot). Text-only entry points keep nil (no prior await, no drift). Images-only sends snapshot empty text and no-op the text submit. 2. The photo picker started an unstructured Task that awaited load+encode then unconditionally re-staged bytes via addPendingAttachment. A sign-out in flight cleared pending attachments but the continuation re-added the previous user's photo under an explicit terminal id. Add a signInGeneration token bumped by signOut, captured before staging, and re-checked in a new guarded addPendingAttachment(...ifSessionGeneration:) store path that drops the result when the token moved or the target terminal no longer exists. Tests: ComposerSubmitRoutingTests gains text-snapshot-survives-edit and text-snapshot-survives-switch; ComposerPendingAttachmentTests gains guarded-add dropped-after-signout, dropped-when-terminal-gone, and succeeds-when-unchanged. Budget actuals refreshed for both touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Round-3 autoreview fixes (7853a83): Finding 1 (text not captured): Finding 2 (re-stage after sign-out): Added a Tests ( |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
321-334: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDon’t grow the 5k-line composite with another store responsibility.
MobileShellComposite.swiftis already 5386 lines and mixes connection lifecycle, workspace/terminal state, drafts, transport, recovery, and UI-facing composer state. Please move the attachment queue/session-generation state and CRUD/send orchestration behind a small composer/attachment collaborator, with this facade delegating to it. As per coding guidelines, production Swift files over 800 lines must be flagged, especially when mixed responsibilities keep expanding.🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 321 - 334, The MobileShellComposite class is exceeding recommended size and mixing too many responsibilities. Extract the attachment-related state (pendingAttachmentsByTerminalID and signInGeneration) and their associated CRUD/send orchestration logic into a separate small collaborator class dedicated to composer and attachment concerns. Have MobileShellComposite delegate attachment operations to this new facade rather than managing them directly, keeping the main composite focused on connection lifecycle, workspace/terminal state, and core transport concerns.Source: Coding guidelines
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2784-2786: The nested workspaces.contains and terminals.contains
check is being executed repeatedly for each staged photo in the batch operation
called by TerminalComposerView.stagePickedItems, causing unnecessary topology
scans. Instead of validating the terminalID existence on each item individually,
maintain a terminal-id index in the store or perform the terminal validation
once before processing the entire batch of attachments, then proceed with
appending them without repeating the contains checks for each item.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 321-334: The MobileShellComposite class is exceeding recommended
size and mixing too many responsibilities. Extract the attachment-related state
(pendingAttachmentsByTerminalID and signInGeneration) and their associated
CRUD/send orchestration logic into a separate small collaborator class dedicated
to composer and attachment concerns. Have MobileShellComposite delegate
attachment operations to this new facade rather than managing them directly,
keeping the main composite focused on connection lifecycle, workspace/terminal
state, and core transport concerns.
🪄 Autofix (Beta)
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
Run ID: 18df0b6e-e3a8-4848-be85-ee6357b7f946
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerPendingAttachmentTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/ComposerSubmitRoutingTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift
| if let terminalID, | ||
| !workspaces.contains(where: { $0.terminals.contains(where: { $0.id.rawValue == terminalID }) }) { | ||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid rescanning every workspace/terminal per staged photo.
TerminalComposerView.stagePickedItems calls this guarded add once per picked image, so this nested workspaces.contains { terminals.contains { ... } } repeats a full topology scan up to the picker cap. Keep a terminal-id index in the store, or validate the target once for the whole batch before appending attachments. As per coding guidelines, attachment staging/picker paths should not repeatedly rescan scalable workspace/terminal collections.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 2784 - 2786, The nested workspaces.contains and terminals.contains
check is being executed repeatedly for each staged photo in the batch operation
called by TerminalComposerView.stagePickedItems, causing unnecessary topology
scans. Instead of validating the terminalID existence on each item individually,
maintain a terminal-id index in the store or perform the terminal validation
once before processing the entire batch of attachments, then proceed with
appending them without repeating the contains checks for each item.
Source: Coding guidelines
Fix three autoreview findings in the iOS composer image-attachment staging and send path. Finding 1 (OOM): replace the full-raster pngData() encode with a bounded ImageIO path. prepare() now downsamples the picked item via CGImageSourceCreateThumbnailAtIndex (send payload capped at 2048 px longest edge, thumbnail at 168 px) and re-encodes the bounded CGImage through CGImageDestination, so a large HEIC/JPEG/panorama is never materialized as a full-resolution raster. The send payload tries PNG at the send size, then JPEG at decreasing quality, then smaller dimensions, all enforced under the 8 MB per-image cap. Returns Data only across the concurrency boundary (Sendable-safe). Finding 2 (caps bypass): enforce the count cap (10), total-byte budget (32 MB), and per-image cap (8 MB) atomically inside the store's addPendingAttachment, computed against the current staged set at mutation time on the MainActor. Two racing picker batches can no longer both append past the cap. The view now tracks the staging Task and cancels the prior one when a new batch starts, and treats the store as the authoritative cap (its own checks are only a pre-filter). Sign-in generation guard kept. Finding 3 (removed-but-uploaded): submitComposer re-checks each attachment is still staged for the captured terminal before uploading it, so a chip the user deletes mid-send is skipped instead of uploaded from the local snapshot. Failure-keeps-attachments and re-entrancy behavior unchanged. Tests: store-enforced count and byte caps (including racing adds against the same starting budget) and a mid-send removal skipping that attachment. Budget TSV bumped for the two touched files.
|
Round-3 hardening (autoreview findings):
Tests: added store-enforced count/byte cap coverage (including racing adds against the same starting budget) in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e60e7a9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// terminals keeps each draft's own attachments (mirroring how the text draft | ||
| /// is keyed). Observed so the composer's chip row re-renders on add/remove. | ||
| /// Sent in order on the next submit and then cleared for that terminal. | ||
| private var pendingAttachmentsByTerminalID: [String: [MobilePendingAttachment]] = [:] |
There was a problem hiding this comment.
When a workspace or terminal disappears after a remote workspace-list refresh/close, nothing removes its entry from this new per-terminal map (the workspaces setter only bumps workspaceTopologyVersion). That leaves up to 32 MiB of Data per closed terminal retained but no longer reachable by any composer chip or submit path, and because the cap is per terminal a user can repeatedly stage photos and close workspaces to grow memory unbounded until sign-out. Prune pendingAttachmentsByTerminalID against the current terminal IDs whenever workspace topology is replaced or merged.
Useful? React with 👍 / 👎.
…ker load Finding 1: the store could retain orphaned photo Data for stale terminal ids. The base addPendingAttachment now validates the target id exists in the current topology (shared terminalExistsInTopology check, used by both add paths so no unchecked entry point remains), and the workspaces didSet prunes pendingAttachmentsByTerminalID for any terminal that disappears from topology so multi-MB staged bytes are released on a sync rather than held until sign-out. Sign-out clear is unchanged. Added tests: base-path add for a missing id is rejected; a topology update dropping a terminal prunes its attachments while a surviving (and a moved-elsewhere) terminal keeps its. Finding 2: the picker loaded the full original asset into memory before any cap. Replaced loadTransferable(Data) with a file-backed ImportedImageFile Transferable (FileRepresentation(contentType: .image)) that copies the import to a temp URL, so a huge ProRAW/DNG/panorama never enters memory as Data. The picker now size-gates the file on disk (60 MB raw bound, above the 8 MB per-image cap since HEIC decodes larger) before reading, downsamples straight from the URL via CGImageSourceCreateWithURL, and deletes the temp file after encoding. Per-image and total caps, atomic store enforcement, the session-generation guard, and the staging Task cancellation are all preserved. Only Data/String/URL cross concurrency boundaries. Refreshed the swift file-length budget for the two touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the two remaining memory-safety findings (commit 42847cf). Finding 1 (orphaned bytes for stale terminal ids): The base Finding 2 (picker slurped full original asset): Replaced Tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42847cf998
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let sent = await submitTerminalPasteImage( | ||
| attachment.data, | ||
| format: attachment.format, | ||
| workspaceID: workspaceID, | ||
| terminalID: submittedTerminalID |
There was a problem hiding this comment.
When a composer submit includes two attachments, or an attachment plus caption that does not start with whitespace, each successful terminal.paste_image is followed immediately by the next image/text paste. The Mac handler (checked Sources/TerminalController.swift:14329) injects only the shell-escaped path with no trailing delimiter, so the terminal receives /tmp/a.png/tmp/b.png or /tmp/a.pngplease...; agents won't parse the intended file path. Add a boundary space/newline after each image before continuing.
Useful? React with 👍 / 👎.
Finding 1: pending attachment bytes were capped per terminal but unbounded across all terminals, so staging photos across many terminals/workspaces grew linearly with terminal count and could OOM. Add global all-terminals caps (maxPendingAttachmentTotalBytesAllTerminals = 64 MB, maxPendingAttachmentCountAllTerminals = 20) enforced atomically in addPendingAttachment after the per-terminal checks, as a hard reject. The sum across all keys is consistent because the add runs on @mainactor. Per-terminal caps stay. Host tests cover the multi-terminal case (global byte and count budgets bind while each terminal is under its own cap; per-terminal cap still binds under global headroom). Finding 2: the picker's ImageIO decode/encode ran in an unstructured Task.detached that did not inherit cancellation, so cancelling the staging task (re-pick, terminal switch, view disappear) left the decode running and fanning out temp files. Run prepare() in a structured background-priority child task group so cancellation propagates; check Task.isCancelled before launching and before the heavy CGImageSourceCreateWithURL and the thumbnail encode. Cancel the staging task on composer .onDisappear and on a terminalID change, in addition to the existing cancel-on-new-batch. Temp files are still cleaned on the cancellation path via the existing per-iteration defer. Only Sendable Data crosses concurrency boundaries. Bump swift-file-length-budget.tsv for the two touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the two remaining autoreview findings (commit a8461d0). Finding 1 (global byte cap): added Finding 2 (cancelled picker batches): replaced the fire-and-forget File-length budget guard green (bumped the two touched files to their new actuals). |
# Conflicts: # .github/swift-file-length-budget.tsv
submitComposer() snapshots attachments and text up front but awaits a paste_image RPC per image. sendRemoteTerminalPasteImage returns true even when a superseded connection answered, so a sign-out, account switch, Mac switch, or reconnect that landed during an image await let the loop keep going: the next staged image and then the captured text were sent through whatever remoteClient is now current, leaking the previous user's or previous Mac's unsent content into a different session. Capture signInGeneration (sign-out / account switch) and connectionGeneration (Mac switch / reconnect / disconnect) at the start of the run and re-check both before every image send and before the text send via isComposerSubmitIdentityCurrent. On mismatch, abort the whole submit (stop the loop, do not send the text) and leave attachments and text staged for a retry. Both generations already existed and are bumped on those edges. Add host-testable coverage in ComposerSubmitRoutingTests: a sign-out and a connection swap between the first and second image send each abort the submit so no further image and no text reach the new session, and the connection-swap case keeps the unsent attachment and text staged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed the P1 stale-session continuation finding in The submit loop awaited a Fix: capture a submit-time identity once at the start of the run and re-check it before every image send and before the text send via Tests (ComposerSubmitRoutingTests, over the real RPC wire): a sign-out and a connection swap each between the first and second image send abort the submit so no further image and no text reach the new session, and the connection-swap case asserts the unsent attachment and text stay staged. Verified red without the guard (text reaches the new session, staged content lost), green with it. 36 tests pass; file-length budget respected. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 02e276a. Configure here.
| terminalID: submittedTerminalID | ||
| ) | ||
| guard sent else { return } | ||
| removePendingAttachment(id: attachment.id, forTerminalID: submittedTerminalID.rawValue) |
There was a problem hiding this comment.
Removed chip still uploads image
Medium Severity
Removing a staged attachment chip only prevents uploads that have not started yet. If the user taps remove while that image’s paste_image RPC is already in flight, the send still completes and the Mac receives the image even though the chip is gone from the composer.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 02e276a. Configure here.
…6199) The Release/archive build (Swift 6 language mode, strict isolation) failed to compile TerminalComposerView: the nonisolated detached prepare() task calls the image-encode helpers and reads the bounding constants, but boundedSendPayload / downsampledImageData and the maxImageBytes / thumbnailMaxPixelSize / sendMaxPixelSize constants inherited the View's @mainactor isolation, so they were not accessible from the nonisolated context. Mark the pure ImageIO helpers and the constants they read nonisolated (and the store's maxPendingAttachmentImageBytes they mirror). Pure functions over CGImageSource/Data plus immutable Int constants, so nonisolated is correct. Verified with a local Swift 6 simulator build (BUILD SUCCEEDED). This unblocks the iOS Release archive / TestFlight build, which the timed-out ios-simulator CI never confirmed before #6102 merged. Co-authored-by: cmux-lawrence <cmux-lawrence@cmux-lawrences-Mac-mini.local>
This branch was successfully deployed
1 participant
What changed
Adds iMessage-style image attachments to the iOS mobile composer and fixes a few composer ergonomics issues. Reuses the existing terminal.paste_image transport (no new wire protocol).
Store (CmuxMobileShell)
MobilePendingAttachmentvalue type (image data + lowercase format hint + stable id), host-testable (no UIKit).composerCanSend(forTerminalID:): true when text is non-empty OR at least one attachment is staged (images-only send allowed).submitComposer(): sends staged images in pick order (awaited, so the injected file paths land first), then submits the text, then clears the staged set for the submitted terminal.Composer UI (TerminalComposerView)
ComposerDockIntent.closeComposer->dismissComposer()), so no dismiss path is lost.submitComposer()and re-measures the band height.Entitlements / localization
NSPhotoLibraryUsageDescriptiontoios/Config/Info.plist.How to test
cd Packages/CmuxMobileShell && swift test --filter ComposerPendingAttachmentTests(10 tests, all green).Caveats
mobile.composer.closelocalization key is now unused (the chevron it labeled was removed) but left in place to avoid extraction churn.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Large in-memory attachment state and async multi-RPC submit interact with account switch, reconnect, and terminal topology; mitigated by caps, pruning, identity guards, and broad unit/routing tests, but still user-data and sync-sensitive.
Overview
Adds iMessage-style photo attachments to the iOS terminal composer: pick from the library, stage per-terminal chips, then send images first (existing
terminal.paste_image) followed by the composed text via newsubmitComposer().MobileShellCompositegains per-terminalMobilePendingAttachmentstaging with atomic per-terminal and global count/byte caps, topology validation/pruning on workspace sync, sign-out wipe, and session-generation guards for in-flight picks. Submit paths capture workspace/terminal/text up front, block double-send, abort on sign-out or connection swap mid-flight, and keep staged content on RPC failure (bool results from paste-image).TerminalComposerViewswaps the band chevron for a paperclip +PhotosPicker, chip row, Send gated on text or attachments, file-backed import with ImageIO downsampling and cancellable staging tasks, plus composer autocorrect/sentence case and tighter top padding. Adds photo-library usage strings and updates UI tests to dismiss via the toolbar compose toggle.Reviewed by Cursor Bugbot for commit 02e276a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds iMessage-style photo attachments to the iOS composer with a paperclip picker, per-terminal staging chips, and images-first sending via
submitComposer(). Reliability is tightened across routing, memory, cancellation, and now aborts mid-send if the session or connection changes.New Features
PhotosUI; stage per terminal as removable chips; Send works with text or attachments (images first viasubmitComposer()).MobilePendingAttachment,composerCanSend, whole-submit re-entrancy guard, and targeted RPCs that capture workspace/terminal/text at start for correct routing.NSPhotoLibraryUsageDescriptionand localized attach/remove labels.Bug Fixes
Written for commit 02e276a. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests