Repository navigation
iOS: view artifacts referenced in agent sessions - #7674
Merged
Merged
Conversation
Adds session-scoped mobile.chat.artifact.{stat,fetch,thumbnail,list} RPCs on
the Mac host (capability chat.artifact.v1), scoped to paths the session's
transcript actually references (attachment hostPaths, fileEdit filePaths,
tool referenced_paths) with symlink-resolved canonical comparison, uniform
forbidden errors, and one-level access into referenced directories. Fetch is
chunked at 3 MiB raw per frame under the 8 MiB frame cap; thumbnails are
downscaled via ImageIO off the main actor through the AgentChatArtifactIndex
actor.
Parsers now emit .attachment messages for cmux clipboard-materialized image
paths in user prompts (so attachments survive transcript reload) and populate
referenced_paths on tool-use messages fail-open on the wire.
iOS renders attachment thumbnails in the transcript, a full-screen viewer
(image/text/binary/too-large/missing/unreachable states), and View file /
Browse folder affordances in tool and file-edit detail sheets, all gated on
the host capability with graceful degradation against older Macs.
Co-Authored-By: Claude Fable 5 <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 chat and terminal artifact support, including shared models, transcript extraction, path authorization, RPC transport, filesystem access, iOS viewers, terminal integration, localization, tests, and build wiring. ChangesChat Artifact Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 errors, 1 warning)
✅ Passed checks (16 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}
Contributor
Greptile SummaryThis PR adds an end-to-end Mac→iOS artifact plane that lets iOS users preview files referenced in agent chat sessions and terminal output. A new
Confidence Score: 5/5Safe to merge; the authorization model is sound and no access-control bypass was found. The Sources/Mobile/AgentChat/AgentChatArtifactIndex.swift — the session snapshot cache has no eviction policy and the cache-miss path blocks the actor's cooperative thread pool thread with synchronous filesystem I/O. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant iOS as iOS Client
participant Mac as TerminalController (Mac)
participant Index as AgentChatArtifactIndex (actor)
participant Scope as ChatArtifactScope
participant FS as Filesystem
iOS->>Mac: mobile.chat.artifact.stat(session_id, path)
Mac->>Index: await canonicalPath(sessionID, agentKind, transcriptPath)
Index->>FS: attributesOfItem(transcriptPath) [cache-freshness stat]
alt Cache miss
Index->>FS: Data(contentsOf: transcriptPath, .mappedIfSafe)
Index->>Index: parse JSONL to ChatArtifactScope
end
Index->>Scope: canonicalFilePath(for: requestedPath)
Scope->>FS: resolveSymlinks(requestedPath)
Scope-->>Index: canonical path or nil
Index-->>Mac: success or notInSet or canonicalizationFailed
alt Forbidden
Mac-->>iOS: forbidden (before any stat)
else Authorized
Mac->>FS: Task.detached ArtifactByteReader stat
Mac-->>iOS: stat result
end
%%{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 iOS as iOS Client
participant Mac as TerminalController (Mac)
participant Index as AgentChatArtifactIndex (actor)
participant Scope as ChatArtifactScope
participant FS as Filesystem
iOS->>Mac: mobile.chat.artifact.stat(session_id, path)
Mac->>Index: await canonicalPath(sessionID, agentKind, transcriptPath)
Index->>FS: attributesOfItem(transcriptPath) [cache-freshness stat]
alt Cache miss
Index->>FS: Data(contentsOf: transcriptPath, .mappedIfSafe)
Index->>Index: parse JSONL to ChatArtifactScope
end
Index->>Scope: canonicalFilePath(for: requestedPath)
Scope->>FS: resolveSymlinks(requestedPath)
Scope-->>Index: canonical path or nil
Index-->>Mac: success or notInSet or canonicalizationFailed
alt Forbidden
Mac-->>iOS: forbidden (before any stat)
else Authorized
Mac->>FS: Task.detached ArtifactByteReader stat
Mac-->>iOS: stat result
end
Reviews (15): Last reviewed commit: "Guard terminal artifact denial logs for ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+266
to
+270
| offset = chunk.offset + Int64(chunk.data.count) | ||
| progress?(offset, chunk.totalSize) | ||
| if chunk.eof { | ||
| return result | ||
| } |
Contributor
There was a problem hiding this comment.
When the Mac-hosted file changes after the viewer's initial stat, this loop keeps appending chunks without checking the cumulative size or whether the returned chunk.offset moved backward. A growing file can bypass the 64 MB preview cap, and a shrinking file can return stale trailing bytes that no longer exist in the current file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swift (1)
89-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared icon+title header to remove duplication.
The
Image(systemName: "photo") + Text(displayName)header block is duplicated betweenbubbleandthumbnailBubble.♻️ Proposed refactor
+ private var titleRow: some View { + HStack(spacing: 6) { + Image(systemName: "photo") + .font(.caption) + Text(displayName) + .font(.caption) + .lineLimit(1) + .truncationMode(.middle) + } + .foregroundStyle(.white) + } + private var bubble: some View { VStack(alignment: .leading, spacing: 3) { - HStack(spacing: 6) { - Image(systemName: "photo") - .font(.caption) - Text(displayName) - .font(.caption) - .lineLimit(1) - .truncationMode(.middle) - } - .foregroundStyle(.white) + titleRow if let hostPath = attachment.hostPath, !hostPath.isEmpty { ... } } ... } private func thumbnailBubble(hostPath: String) -> some View { HStack(spacing: 8) { ... VStack(alignment: .leading, spacing: 3) { - HStack(spacing: 6) { - Image(systemName: "photo") - .font(.caption) - Text(displayName) - .font(.caption) - .lineLimit(1) - .truncationMode(.middle) - } + titleRow Text(hostPath) ... } } ... - .foregroundStyle(.white) ... }Also applies to: 113-139
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swift` around lines 89 - 111, The `bubble` header duplicates the same `Image(systemName: "photo")` plus `Text(displayName)` layout used in `thumbnailBubble`, so extract that shared icon/title row into a reusable helper view or computed subview in `ChatAttachmentBubbleView`. Update both `bubble` and `thumbnailBubble` to call the shared header so the styling, truncation, and foreground treatment stay consistent in one place.
🤖 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/Artifacts/ChatArtifactFolderView.swift`:
- Around line 158-175: The image decoding logic in artifactImage(data:) is
duplicated in ChatArtifactFolderView and ChatArtifactViewerSheet, so refactor it
into a shared cross-platform helper in the Artifacts module. Extract the common
UIKit/AppKit image-loading branch into a reusable view builder or helper type,
then update artifactImage(data:) in both places to call that shared symbol and
keep iconName fallback behavior unchanged.
- Around line 105-114: The load() flow in ChatArtifactFolderView is treating
task cancellation as a real failure, which causes state to briefly become
.failed when .task(id: path) supersedes an in-flight listing. Update load() to
recognize CancellationError separately from genuine errors: after the await on
loader.list(path:), or in the catch path, bail out without changing state when
the task was cancelled, and only set state = .failed for non-cancellation
failures. Use the existing load() method, state updates via MainActor.run, and
the loader.list(path:) call as the key points to adjust.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`:
- Around line 78-83: The “too large” preview message in ChatArtifactViewerSheet
is hardcoded with a stale size and should use the actual policy limit instead.
Update the .tooLarge branch in the artifact state handling to derive the
localized message from ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes
(or the passed limit value) rather than embedding “64 MB”, while keeping
formattedSize(limit) as the detailed value. This keeps the text consistent with
the enforced preview limit and the existing ChatArtifactTransferPolicy /
formattedSize usage.
- Around line 244-251: The `.unsupportedMedia` fallback in
`ChatArtifactViewerSheet` is constructing a synthetic `ChatArtifactStat` with a
fake zero size, which causes the `.binary` preview to show an incorrect size
detail. Update the `.unsupportedMedia` handling in `ChatArtifactStat.Kind`/the
related initializer path so it does not fabricate a zero-byte stat, and change
the `.binary` preview view to accept an optional stat and omit the detail line
when the stat is nil. Use the `.binary` case and the preview rendering logic in
`ChatArtifactViewerSheet` to locate the fix.
- Around line 105-144: The progress updates in ChatArtifactViewerSheet.load are
firing detached Task { `@MainActor` in ... } work that outlives the caller and can
race after path changes. Replace the fire-and-forget progress callback with a
cancellation-aware approach tied to the load() task (for example by checking
Task.isCancelled before updating state and avoiding unstructured inner Tasks),
so stale fetch ticks from a previous file cannot overwrite the current loading
state.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactStat.swift`:
- Around line 52-64: Add a test covering the dual modifiedAt decoding behavior
in ChatArtifactStat.init(from:), since encode(to:) only writes the Double epoch
form and the Date fallback can regress silently. Create a round-trip/decoding
test that verifies the model still decodes a legacy Date-encoded modified_at as
well as the current Double-encoded form, using ChatArtifactStat and its
CodingKeys.modifiedAt path to locate the logic.
---
Outside diff comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swift`:
- Around line 89-111: The `bubble` header duplicates the same `Image(systemName:
"photo")` plus `Text(displayName)` layout used in `thumbnailBubble`, so extract
that shared icon/title row into a reusable helper view or computed subview in
`ChatAttachmentBubbleView`. Update both `bubble` and `thumbnailBubble` to call
the shared header so the styling, truncation, and foreground treatment stay
consistent in one place.
🪄 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: 94ec57cc-0914-4c5e-865f-6c1b2fe9c14f
📒 Files selected for processing (41)
.gitignorePackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactChunk.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactDirectoryEntry.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactDirectoryListing.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactError.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactKind.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactScope.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactStat.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactThumbnail.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactTransferPolicy.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Model/ChatToolUse.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatAttachmentTokenExtractor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatToolReferencedPathExtractor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ClaudeTranscriptParser.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/CodexTranscriptParser.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/TranscriptToolCompletion.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Source/ChatEventSource.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatArtifactScopeTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatArtifactTransferPolicyTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ClaudeTranscriptParserTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/CodexTranscriptParserTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactLoader.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstringsPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetail.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailBuilder.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailSheetView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swiftPackages/iOS/CmuxAgentChatUI/Tests/CmuxAgentChatUITests/ChatArtifactLoaderTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+AgentChat.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftResources/Localizable.xcstringsSources/Mobile/AgentChat/AgentChatArtifactIndex.swiftSources/Mobile/MobileHostService+Capabilities.swiftSources/TerminalController+MobileChat.swiftSources/TerminalController+MobileChatArtifacts.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+105
to
+114
| private func load() async { | ||
| await MainActor.run { state = .loading } | ||
| do { | ||
| let listing = try await loader.list(path: path) | ||
| guard !Task.isCancelled else { return } | ||
| await MainActor.run { state = .entries(listing.entries) } | ||
| } catch { | ||
| await MainActor.run { state = .failed } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cancellation errors are treated as load failures.
If path changes mid-listing, the outer .task(id: path) cancels the in-flight load(); the resulting CancellationError falls into the catch block and sets state = .failed, briefly showing "Couldn't load this folder" for a load that was merely superseded rather than actually failed.
🔧 Proposed fix
} catch {
+ guard !Task.isCancelled else { return }
await MainActor.run { state = .failed }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func load() async { | |
| await MainActor.run { state = .loading } | |
| do { | |
| let listing = try await loader.list(path: path) | |
| guard !Task.isCancelled else { return } | |
| await MainActor.run { state = .entries(listing.entries) } | |
| } catch { | |
| await MainActor.run { state = .failed } | |
| } | |
| } | |
| private func load() async { | |
| await MainActor.run { state = .loading } | |
| do { | |
| let listing = try await loader.list(path: path) | |
| guard !Task.isCancelled else { return } | |
| await MainActor.run { state = .entries(listing.entries) } | |
| } catch { | |
| guard !Task.isCancelled else { return } | |
| await MainActor.run { state = .failed } | |
| } | |
| } |
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swift`
around lines 105 - 114, The load() flow in ChatArtifactFolderView is treating
task cancellation as a real failure, which causes state to briefly become
.failed when .task(id: path) supersedes an in-flight listing. Update load() to
recognize CancellationError separately from genuine errors: after the await on
loader.list(path:), or in the catch path, bail out without changing state when
the task was cancelled, and only set state = .failed for non-cancellation
failures. Use the existing load() method, state updates via MainActor.run, and
the loader.list(path:) call as the key points to adjust.
Comment on lines
+158
to
+175
| @ViewBuilder | ||
| private func artifactImage(data: Data) -> some View { | ||
| #if canImport(UIKit) | ||
| if let image = UIImage(data: data) { | ||
| Image(uiImage: image).resizable() | ||
| } else { | ||
| Image(systemName: iconName) | ||
| } | ||
| #elseif canImport(AppKit) | ||
| if let image = NSImage(data: data) { | ||
| Image(nsImage: image).resizable() | ||
| } else { | ||
| Image(systemName: iconName) | ||
| } | ||
| #else | ||
| Image(systemName: iconName) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
artifactImage(data:) duplicates the same helper in ChatArtifactViewerSheet.swift.
Both files implement the identical #if canImport(UIKit)/#elseif canImport(AppKit) image-decoding branch (see ChatArtifactViewerSheet.swift lines 181-200). Consider extracting a shared helper (e.g., a small cross-platform PlatformImage view builder in the Artifacts module) to avoid maintaining two copies.
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swift`
around lines 158 - 175, The image decoding logic in artifactImage(data:) is
duplicated in ChatArtifactFolderView and ChatArtifactViewerSheet, so refactor it
into a shared cross-platform helper in the Artifacts module. Extract the common
UIKit/AppKit image-loading branch into a reusable view builder or helper type,
then update artifactImage(data:) in both places to call that shared symbol and
keep iconName fallback behavior unchanged.
Comment on lines
+105
to
+144
| private func load() async { | ||
| await MainActor.run { | ||
| state = .loading(fetched: 0, total: nil) | ||
| } | ||
| do { | ||
| let stat = try await loader.stat(path: path) | ||
| guard !stat.isDirectory else { | ||
| await MainActor.run { state = .binary(stat: stat) } | ||
| return | ||
| } | ||
| guard stat.size <= ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes else { | ||
| await MainActor.run { | ||
| state = .tooLarge(limit: ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes) | ||
| } | ||
| return | ||
| } | ||
| let data = try await loader.fetch(path: path) { fetched, total in | ||
| Task { @MainActor in | ||
| state = .loading(fetched: fetched, total: total) | ||
| } | ||
| } | ||
| guard !Task.isCancelled else { return } | ||
| switch stat.kind { | ||
| case .image: | ||
| await MainActor.run { state = .image(data: data) } | ||
| case .text: | ||
| if let text = String(data: data, encoding: .utf8) { | ||
| await MainActor.run { state = .text(text: text) } | ||
| } else { | ||
| await MainActor.run { state = .binary(stat: stat) } | ||
| } | ||
| case .binary, .directory: | ||
| await MainActor.run { state = .binary(stat: stat) } | ||
| } | ||
| } catch { | ||
| await MainActor.run { | ||
| state = LoadState(error: error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Progress-callback Tasks aren't cancellation-aware; stale ticks can overwrite newer state.
Each progress tick spawns a new unstructured Task { @mainactor in ... } (lines 121-125). When path changes, .task(id: path) cancels the outer load() Task, but these inner detached Tasks are not children of it and keep running, so a late progress tick from the previous file's fetch can overwrite state after the new load has already reset it — briefly showing the wrong progress under the new file's title.
🔧 Proposed fix
let data = try await loader.fetch(path: path) { fetched, total in
+ guard !Task.isCancelled else { return }
Task { `@MainActor` in
state = .loading(fetched: fetched, total: total)
}
}As per coding guidelines: "Flag fire-and-forget Task { ... } work with meaningful lifecycle that is not stored, cancelled, or tied to a caller-owned operation."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func load() async { | |
| await MainActor.run { | |
| state = .loading(fetched: 0, total: nil) | |
| } | |
| do { | |
| let stat = try await loader.stat(path: path) | |
| guard !stat.isDirectory else { | |
| await MainActor.run { state = .binary(stat: stat) } | |
| return | |
| } | |
| guard stat.size <= ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes else { | |
| await MainActor.run { | |
| state = .tooLarge(limit: ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes) | |
| } | |
| return | |
| } | |
| let data = try await loader.fetch(path: path) { fetched, total in | |
| Task { @MainActor in | |
| state = .loading(fetched: fetched, total: total) | |
| } | |
| } | |
| guard !Task.isCancelled else { return } | |
| switch stat.kind { | |
| case .image: | |
| await MainActor.run { state = .image(data: data) } | |
| case .text: | |
| if let text = String(data: data, encoding: .utf8) { | |
| await MainActor.run { state = .text(text: text) } | |
| } else { | |
| await MainActor.run { state = .binary(stat: stat) } | |
| } | |
| case .binary, .directory: | |
| await MainActor.run { state = .binary(stat: stat) } | |
| } | |
| } catch { | |
| await MainActor.run { | |
| state = LoadState(error: error) | |
| } | |
| } | |
| } | |
| private func load() async { | |
| await MainActor.run { | |
| state = .loading(fetched: 0, total: nil) | |
| } | |
| do { | |
| let stat = try await loader.stat(path: path) | |
| guard !stat.isDirectory else { | |
| await MainActor.run { state = .binary(stat: stat) } | |
| return | |
| } | |
| guard stat.size <= ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes else { | |
| await MainActor.run { | |
| state = .tooLarge(limit: ChatArtifactTransferPolicy.defaultPolicy.maxPreviewBytes) | |
| } | |
| return | |
| } | |
| let data = try await loader.fetch(path: path) { fetched, total in | |
| guard !Task.isCancelled else { return } | |
| Task { `@MainActor` in | |
| state = .loading(fetched: fetched, total: total) | |
| } | |
| } | |
| guard !Task.isCancelled else { return } | |
| switch stat.kind { | |
| case .image: | |
| await MainActor.run { state = .image(data: data) } | |
| case .text: | |
| if let text = String(data: data, encoding: .utf8) { | |
| await MainActor.run { state = .text(text: text) } | |
| } else { | |
| await MainActor.run { state = .binary(stat: stat) } | |
| } | |
| case .binary, .directory: | |
| await MainActor.run { state = .binary(stat: stat) } | |
| } | |
| } catch { | |
| await MainActor.run { | |
| state = LoadState(error: error) | |
| } | |
| } | |
| } |
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`
around lines 105 - 144, The progress updates in ChatArtifactViewerSheet.load are
firing detached Task { `@MainActor` in ... } work that outlives the caller and can
race after path changes. Replace the fire-and-forget progress callback with a
cancellation-aware approach tied to the load() task (for example by checking
Task.isCancelled before updating state and avoiding unstructured inner Tasks),
so stale fetch ticks from a previous file cannot overwrite the current loading
state.
Source: Coding guidelines
Extends artifact viewing from the agent-chat surface to the iOS terminal
view. New session-independent RPCs mobile.terminal.artifact.{scan,stat,fetch,
thumbnail} (capability terminal.artifact.v1), scoped to file paths that
currently appear in that terminal's on-screen + scrollback text: the Mac
captures its own buffer text on the main actor per request, rebuilds the
allowed set, and canonically compares before any stat (uniform forbidden for
off-screen/traversal/symlink-escape). Relative tokens resolve against the
terminal cwd. Byte IO is factored into a shared ArtifactByteReader reused by
the chat handlers unchanged; detection/scope/IO run off the main actor.
iOS: a Files button in the terminal toolbar lists the on-screen paths (image
kinds show thumbnails) and opens the shared artifact viewer; tapping a path
token directly in the terminal opens it, while a miss falls through to normal
terminal click so input/scroll/selection are unaffected. ChatArtifactLoader
generalized to .chat/.terminal scopes; all gated on terminal.artifact.v1 with
graceful degradation. Strings localized EN+JA.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
azooz2003-bit
force-pushed
the
feat-ios-artifact-view
branch
from
July 9, 2026 04:23
4c5448c to
8da4b52
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift (1)
468-489: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winArtifact taps should not override mouse-reporting TUI clicks.
didTapAtColcan consume a tap as an artifact open whenever the visible text contains a path-like token, even though this path is meant to forward the click so TUIs with mouse mode receive it. Gate the artifact lookup on the same alt-screen/mouse-report state, or fall back toclickTerminalwhen that state is active.🤖 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/GhosttySurfaceRepresentable.swift` around lines 468 - 489, `ghosttySurfaceView(_:didTapAtCol:row:)` is letting artifact path detection intercept taps that should be forwarded to mouse-reporting TUIs. Update the tap handling in `GhosttySurfaceRepresentable` so artifact lookup only runs when the surface is not in the alt-screen/mouse-reporting state, using the same state check as the terminal click forwarding path. If that state is active, skip `TerminalArtifactTapHitTester().path(in:col:row:)` and always fall through to `store?.clickTerminal(surfaceID:col:row:)`; keep `onArtifactPathTapped(_:)` only for non-TUI artifact taps.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swift (1)
245-275: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
terminalArtifactFetchduplicatesartifactFetch's chunked-loop logic verbatim.Both functions implement the identical offset-tracking
while truefetch loop (capacity reservation, progress callback, EOF check, empty-chunk-throws-macUnreachableguard) — the only difference is the RPC method name and whether params carrysession_idvsworkspace_id/surface_id. Any future fix to this loop (e.g. adding a max-iteration guard against a server that never setseof, or boundingreserveCapacityagainst an untrustedtotalSize) now has to be applied in two places and can silently drift.♻️ Proposed extraction
+ private func chunkedFetch( + method: String, + baseParams: [String: Any], + progress: (`@Sendable` (_ fetchedBytes: Int64, _ totalBytes: Int64) -> Void)? + ) async throws -> Data { + var offset: Int64 = 0 + var result = Data() + while true { + var params = baseParams + params["offset"] = offset + params["length"] = ChatArtifactTransferPolicy.defaultPolicy.maxRawChunkBytes + let chunk: ChatArtifactChunk = try await artifactCall(method: method, params: params) + if result.isEmpty, chunk.totalSize > 0, chunk.totalSize <= Int64(Int.max) { + result.reserveCapacity(Int(chunk.totalSize)) + } + result.append(chunk.data) + offset = chunk.offset + Int64(chunk.data.count) + progress?(offset, chunk.totalSize) + if chunk.eof { + return result + } + guard !chunk.data.isEmpty else { + throw ChatArtifactError.macUnreachable + } + } + } public func artifactFetch( sessionID: String, path: String, progress: (`@Sendable` (_ fetchedBytes: Int64, _ totalBytes: Int64) -> Void)? ) async throws -> Data { - var offset: Int64 = 0 - var result = Data() - while true { - let chunk: ChatArtifactChunk = try await artifactCall( - method: "mobile.chat.artifact.fetch", - params: [ - "session_id": sessionID, - "path": path, - "offset": offset, - "length": ChatArtifactTransferPolicy.defaultPolicy.maxRawChunkBytes, - ] - ) - if result.isEmpty, chunk.totalSize > 0, chunk.totalSize <= Int64(Int.max) { - result.reserveCapacity(Int(chunk.totalSize)) - } - result.append(chunk.data) - offset = chunk.offset + Int64(chunk.data.count) - progress?(offset, chunk.totalSize) - if chunk.eof { - return result - } - guard !chunk.data.isEmpty else { - throw ChatArtifactError.macUnreachable - } - } + try await chunkedFetch( + method: "mobile.chat.artifact.fetch", + baseParams: ["session_id": sessionID, "path": path], + progress: progress + ) }And similarly for
terminalArtifactFetch, passing["workspace_id": workspaceID, "surface_id": surfaceID, "path": path].Also applies to: 330-362
🤖 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/MobileChatEventSource.swift` around lines 245 - 275, `artifactFetch` and `terminalArtifactFetch` contain the same chunked fetch loop, so extract the shared offset/progress/EOF logic into a single helper and have both methods call it with their RPC method name and params (`sessionID` vs `workspaceID`/`surfaceID`). Keep the existing behaviors in the helper, including capacity reservation, progress updates, EOF handling, and the empty-chunk `macUnreachable` guard, so future fixes only need to be made in one place.
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 320-340: The files sheet is creating two separate
MobileChatEventSource instances during one presentation by calling
store.makeChatEventSource() both in the TerminalArtifactFilesSheet setup and
again inside terminalArtifactLoader(). Update WorkspaceDetailView so the source
is created once for a given sheet presentation and then reused for both the
sheet’s source parameter and the loader/environment setup, keeping the
single-source decision centralized in the relevant sheet-building path.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 3778-3824: The new visibleTextSnapshot(surface:generation:)
repeats the same continuation/pending snapshot flow already used by
visibleSnapshotSection(surface:generation:grid:font), including the guards,
pendingVisibleSnapshot replacement, deadline pump setup, and queue.async to
`@MainActor` completion hop. Factor that shared control flow into a private helper
that both methods call, passing only the final formatting/computation as a
closure so the text-specific and section-specific logic stay separate. Make sure
the helper always resumes or cancels the checked continuation on every path,
including when self is deallocated or the surface/generation changes mid-flight,
to avoid dangling continuations.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swift`:
- Around line 54-73: `canonicalPath(for:)` is using a different scope rule than
`artifactPaths()`, causing inconsistent authorization for detected terminal
paths. Update `TerminalArtifactScope.canonicalPath(for:)` to reuse the same
candidate filtering as `artifactPaths()`—specifically the
`resolver.isDirectory(candidate) != nil` existence check before
canonicalizing—so both entry points in `TerminalArtifactScope` apply one
authoritative path-validation flow and
`ChatArtifactScope.canonicalizedPath(_:resolver:)` is only reached for trusted
candidates.
In `@Sources/TerminalController`+MobileTerminalArtifacts.swift:
- Around line 266-273: The `payload` helper in `TerminalArtifactWire` is
returning an optional dictionary even though every caller already falls back
with `?? [:]`. Update `payload<T: Encodable>` to return an empty dictionary
directly when encoding or JSON parsing fails, and change the return type to a
non-optional `[String: Any]` so the redundant optional collection is removed and
the SwiftLint hint is satisfied.
- Line 44: The forbidden-access debug logging in
TerminalController+MobileTerminalArtifacts is leaking the raw requested
filesystem path. Update the cmuxDebugLog calls in the forbidden handling paths
to avoid interpolating context.requestedPath directly; instead log only
non-sensitive metadata such as the operation name and a presence/length
indicator, or use a redacted/private representation. Apply the same fix
consistently to the related log sites in the MobileTerminalArtifacts flow so no
client-supplied path content is written to debug logs.
- Around line 103-143: `mobileTerminalArtifactContext` is re-resolving the
terminal artifact scope and rereading full scrollback on every chunked
stat/fetch/thumbnail call, which makes long transfers repeatedly pay the
main-actor export cost. Update the terminal-artifact flow so the resolved scope
and canonical path from
`mobileResolveWorkspaceAndSurface`/`TerminalArtifactReadContext` are cached and
reused for the lifetime of a transfer, instead of reconstructing them inside
`v2MainSync` for each request. Keep the existing resolution path in
`TerminalController+MobileTerminalArtifacts` but ensure repeated fetch chunks
reuse the same context rather than calling `readTerminalTextForSnapshot` again.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swift`:
- Around line 245-275: `artifactFetch` and `terminalArtifactFetch` contain the
same chunked fetch loop, so extract the shared offset/progress/EOF logic into a
single helper and have both methods call it with their RPC method name and
params (`sessionID` vs `workspaceID`/`surfaceID`). Keep the existing behaviors
in the helper, including capacity reservation, progress updates, EOF handling,
and the empty-chunk `macUnreachable` guard, so future fixes only need to be made
in one place.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift`:
- Around line 468-489: `ghosttySurfaceView(_:didTapAtCol:row:)` is letting
artifact path detection intercept taps that should be forwarded to
mouse-reporting TUIs. Update the tap handling in `GhosttySurfaceRepresentable`
so artifact lookup only runs when the surface is not in the
alt-screen/mouse-reporting state, using the same state check as the terminal
click forwarding path. If that state is active, skip
`TerminalArtifactTapHitTester().path(in:col:row:)` and always fall through to
`store?.clickTerminal(surfaceID:col:row:)`; keep `onArtifactPathTapped(_:)` only
for non-TUI artifact taps.
🪄 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: 10a282be-4e03-4899-bd50-c3a6ddf84ffa
📒 Files selected for processing (30)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ArtifactByteReader.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactScope.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactPathDetector.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScanResponse.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactPathDetectorTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactScopeTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactLoader.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swiftPackages/iOS/CmuxAgentChatUI/Tests/CmuxAgentChatUITests/ChatArtifactLoaderTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalAccessoryChatCompatibility.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceViewDelegate.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputAccessoryAction.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftResources/Localizable.xcstringsSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileChatArtifacts.swiftSources/TerminalController+MobileTerminalArtifacts.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstrings
| }.value | ||
| return .ok(TerminalArtifactWire.payload(stat) ?? [:]) | ||
| } catch TerminalArtifactReadContext.Error.forbidden { | ||
| cmuxDebugLog("mobile.terminal.artifact.forbidden op=stat path=\(context.requestedPath ?? "nil")") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Raw requested path logged on forbidden-access attempts.
cmuxDebugLog("mobile.terminal.artifact.forbidden op=... path=\(context.requestedPath ?? "nil")") logs the full client-requested filesystem path (which can embed usernames, document/project names, or other personal/customer content) to the local debug log. Prior guidance in this repo is to avoid logging raw paths even in DEBUG builds; prefer logging only non-sensitive metadata (op name, byte length of the path, or a presence flag).
🔒️ Proposed fix
- cmuxDebugLog("mobile.terminal.artifact.forbidden op=stat path=\(context.requestedPath ?? "nil")")
+ cmuxDebugLog("mobile.terminal.artifact.forbidden op=stat pathBytes=\(context.requestedPath?.utf8.count ?? 0)")Based on learnings: "avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content)", and as per coding guidelines: "Do not log secrets, tokens, passwords, private keys, customer content, or personal data unless the value is explicitly redacted with .private or equivalent private redaction."
Also applies to: 71-71, 94-94
🤖 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 `@Sources/TerminalController`+MobileTerminalArtifacts.swift at line 44, The
forbidden-access debug logging in TerminalController+MobileTerminalArtifacts is
leaking the raw requested filesystem path. Update the cmuxDebugLog calls in the
forbidden handling paths to avoid interpolating context.requestedPath directly;
instead log only non-sensitive metadata such as the operation name and a
presence/length indicator, or use a redacted/private representation. Apply the
same fix consistently to the related log sites in the MobileTerminalArtifacts
flow so no client-supplied path content is written to debug logs.
Sources: Coding guidelines, Learnings
Comment on lines
+103
to
+143
| private func mobileTerminalArtifactContext( | ||
| params: [String: Any], | ||
| requiresPath: Bool | ||
| ) -> TerminalArtifactContextResolution { | ||
| guard let workspaceID = v2RawString(params, "workspace_id")?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| let surfaceID = v2RawString(params, "surface_id")?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !workspaceID.isEmpty, | ||
| !surfaceID.isEmpty, | ||
| !requiresPath || v2RawString(params, "path")?.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty == false else { | ||
| return .failure(.err( | ||
| code: "invalid_params", | ||
| message: String( | ||
| localized: "mobile.terminal.artifact.error.invalidParams", | ||
| defaultValue: "workspace_id, surface_id, and path are required." | ||
| ), | ||
| data: nil | ||
| )) | ||
| } | ||
| return v2MainSync { () -> TerminalArtifactContextResolution in | ||
| guard let resolved = mobileResolveWorkspaceAndSurface(params: params, requireTerminal: true), | ||
| let resolvedSurfaceID = resolved.surfaceId, | ||
| let terminalPanel = resolved.workspace.terminalPanel(for: resolvedSurfaceID) else { | ||
| return .failure(mobileTerminalArtifactError(.notFound, path: v2RawString(params, "path"))) | ||
| } | ||
| let workingDirectory = resolved.workspace.effectivePanelDirectory( | ||
| panelId: resolvedSurfaceID, | ||
| localFallback: mobileNonEmpty(terminalPanel.directory) ?? mobileNonEmpty(terminalPanel.requestedWorkingDirectory) | ||
| ) ?? resolved.workspace.currentDirectory | ||
| let terminalText = readTerminalTextForSnapshot( | ||
| terminalPanel: terminalPanel, | ||
| includeScrollback: true, | ||
| lineLimit: nil, | ||
| allowVTExport: true | ||
| ) ?? "" | ||
| return .success(TerminalArtifactReadContext( | ||
| terminalText: terminalText, | ||
| workingDirectory: workingDirectory, | ||
| requestedPath: v2RawString(params, "path") | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' Sources/TerminalController+MobileTerminalArtifacts.swiftRepository: manaflow-ai/cmux
Length of output: 11523
🏁 Script executed:
# Inspect the target file and related helpers.
sed -n '1,260p' Sources/TerminalController+MobileTerminalArtifacts.swift
printf '\n===== search =====\n'
rg -n "v2MobileTerminalArtifactFetch|mobileTerminalArtifactContext|authorizedRead|readTerminalTextForSnapshot|cmuxDebugLog|canonicalPath\\(for:" Sources -g '*.swift'Repository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('Sources/TerminalController+MobileTerminalArtifacts.swift')
text = p.read_text()
for needle in [
'mobileTerminalArtifactContext',
'v2MobileTerminalArtifactFetch',
'authorizedRead',
'readTerminalTextForSnapshot',
'cmuxDebugLog',
'canonicalPath(for:',
]:
print(f'===== {needle} =====')
for i, line in enumerate(text.splitlines(), start=1):
if needle in line:
for j in range(max(1, i-20), min(len(text.splitlines()), i+80)+1):
print(f'{j:4d}: {text.splitlines()[j-1]}')
print()
break
PYRepository: manaflow-ai/cmux
Length of output: 29468
🏁 Script executed:
sed -n '260,520p' Sources/TerminalController+MobileTerminalArtifacts.swiftRepository: manaflow-ai/cmux
Length of output: 598
Cache the terminal-artifact scope across fetch chunks. mobileTerminalArtifactContext rebuilds the scope from the full terminal scrollback inside v2MainSync for every stat/fetch/thumbnail request; because fetch is chunked, a large download pays that main-actor export again on each chunk. Reuse the resolved scope/canonical-path for the lifetime of a transfer instead of re-reading the whole terminal text each time.
🤖 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 `@Sources/TerminalController`+MobileTerminalArtifacts.swift around lines 103 -
143, `mobileTerminalArtifactContext` is re-resolving the terminal artifact scope
and rereading full scrollback on every chunked stat/fetch/thumbnail call, which
makes long transfers repeatedly pay the main-actor export cost. Update the
terminal-artifact flow so the resolved scope and canonical path from
`mobileResolveWorkspaceAndSurface`/`TerminalArtifactReadContext` are cached and
reused for the lifetime of a transfer, instead of reconstructing them inside
`v2MainSync` for each request. Keep the existing resolution path in
`TerminalController+MobileTerminalArtifacts` but ensure repeated fetch chunks
reuse the same context rather than calling `readTerminalTextForSnapshot` again.
Comment on lines
+266
to
+273
| static func payload<T: Encodable>(_ value: T) -> [String: Any]? { | ||
| let coding = ChatWireCoding() | ||
| guard let data = try? coding.encode(value), | ||
| let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { | ||
| return nil | ||
| } | ||
| return object | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Optional: address the SwiftLint discouraged_optional_collection hint.
TerminalArtifactWire.payload returns [String: Any]?; every call site already normalizes with ?? [:]. Returning [:] directly here would remove the redundant optional and satisfy the linter.
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 266-266: Prefer empty collection over optional collection
(discouraged_optional_collection)
🤖 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 `@Sources/TerminalController`+MobileTerminalArtifacts.swift around lines 266 -
273, The `payload` helper in `TerminalArtifactWire` is returning an optional
dictionary even though every caller already falls back with `?? [:]`. Update
`payload<T: Encodable>` to return an empty dictionary directly when encoding or
JSON parsing fails, and change the return type to a non-optional `[String: Any]`
so the redundant optional collection is removed and the SwiftLint hint is
satisfied.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swift`:
- Around line 330-362: The chunked fetch logic in terminalArtifactFetch is
duplicated from artifactFetch, including the reserve-capacity, offset update,
eof return, and empty-chunk guard. Extract that shared loop into a private
helper (for example, a common artifact fetch loop used by artifactFetch and
terminalArtifactFetch) and have both methods call it with their specific method
name and base params so the EOF/empty handling stays consistent in one place.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swift`:
- Around line 176-180: The fold for
TerminalAccessoryConfiguration.defaultConfigurableOrder is inserting
TerminalInputAccessoryAction.files in the wrong spot because it anchors after
.zoomOut, which can place it between .zoomOut and .zoomIn instead of before the
zoom controls. Update the ordering logic in defaultConfigurableOrder to anchor
.files against .pageDown, or otherwise reorder the trailing defaults so .files
consistently folds in ahead of .zoomOut and .zoomIn while preserving the
intended curated order.
In `@Sources/TerminalController`+MobileChatArtifacts.swift:
- Line 18: The four MobileChatArtifacts RPC handlers are masking serialization
failures by turning ChatArtifactWire.payload(...) nil results into .ok([:]),
which makes an encode error look like success; update each call site in
TerminalController+MobileChatArtifacts so the ChatArtifactStat,
ChatArtifactChunk, ChatArtifactThumbnail, and ChatArtifactDirectoryListing
responses return an internal_error (or equivalent failure) when
ChatArtifactWire.payload(...) fails instead of defaulting to an empty
dictionary. Use the ChatArtifactWire.payload helper as the failure check and
keep the success path only for valid encoded payloads.
🪄 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: 2dad4a9b-7d02-4a21-b600-413847e74fea
📒 Files selected for processing (30)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ArtifactByteReader.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactScope.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactPathDetector.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScanResponse.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactPathDetectorTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactScopeTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactLoader.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swiftPackages/iOS/CmuxAgentChatUI/Tests/CmuxAgentChatUITests/ChatArtifactLoaderTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalAccessoryChatCompatibility.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceViewDelegate.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputAccessoryAction.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftResources/Localizable.xcstringsSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileChatArtifacts.swiftSources/TerminalController+MobileTerminalArtifacts.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstrings
Comment on lines
+330
to
+362
| public func terminalArtifactFetch( | ||
| workspaceID: String, | ||
| surfaceID: String, | ||
| path: String, | ||
| progress: (@Sendable (_ fetchedBytes: Int64, _ totalBytes: Int64) -> Void)? | ||
| ) async throws -> Data { | ||
| var offset: Int64 = 0 | ||
| var result = Data() | ||
| while true { | ||
| let chunk: ChatArtifactChunk = try await artifactCall( | ||
| method: "mobile.terminal.artifact.fetch", | ||
| params: [ | ||
| "workspace_id": workspaceID, | ||
| "surface_id": surfaceID, | ||
| "path": path, | ||
| "offset": offset, | ||
| "length": ChatArtifactTransferPolicy.defaultPolicy.maxRawChunkBytes, | ||
| ] | ||
| ) | ||
| if result.isEmpty, chunk.totalSize > 0, chunk.totalSize <= Int64(Int.max) { | ||
| result.reserveCapacity(Int(chunk.totalSize)) | ||
| } | ||
| result.append(chunk.data) | ||
| offset = chunk.offset + Int64(chunk.data.count) | ||
| progress?(offset, chunk.totalSize) | ||
| if chunk.eof { | ||
| return result | ||
| } | ||
| guard !chunk.data.isEmpty else { | ||
| throw ChatArtifactError.macUnreachable | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
De-duplicate the chunked-fetch loop. terminalArtifactFetch is a near-verbatim copy of artifactFetch (lines 245–275); only the method name and the params prefix differ. Extract the loop (reserve-capacity, offset advance, eof return, empty-chunk guard) into one private helper so the correctness-critical EOF/empty handling stays in a single place and cannot drift between the chat and terminal paths.
♻️ Sketch
private func artifactFetchLoop(
method: String,
baseParams: [String: Any],
progress: (`@Sendable` (_ fetchedBytes: Int64, _ totalBytes: Int64) -> Void)?
) async throws -> Data {
var offset: Int64 = 0
var result = Data()
while true {
var params = baseParams
params["offset"] = offset
params["length"] = ChatArtifactTransferPolicy.defaultPolicy.maxRawChunkBytes
let chunk: ChatArtifactChunk = try await artifactCall(method: method, params: params)
if result.isEmpty, chunk.totalSize > 0, chunk.totalSize <= Int64(Int.max) {
result.reserveCapacity(Int(chunk.totalSize))
}
result.append(chunk.data)
offset = chunk.offset + Int64(chunk.data.count)
progress?(offset, chunk.totalSize)
if chunk.eof { return result }
guard !chunk.data.isEmpty else { throw ChatArtifactError.macUnreachable }
}
}🤖 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/MobileChatEventSource.swift`
around lines 330 - 362, The chunked fetch logic in terminalArtifactFetch is
duplicated from artifactFetch, including the reserve-capacity, offset update,
eof return, and empty-chunk guard. Extract that shared loop into a private
helper (for example, a common artifact fetch loop used by artifactFetch and
terminalArtifactFetch) and have both methods call it with their specific method
name and base params so the EOF/empty handling stays consistent in one place.
| let stat = try await Task.detached { | ||
| try ArtifactByteReader().stat(path: resolved.canonicalPath) | ||
| }.value | ||
| return .ok(ChatArtifactWire.payload(stat) ?? [:]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Encoding failures silently become fake successful empty responses.
At all four call sites, ChatArtifactWire.payload(...) ?? [:] folds an encoding/serialization failure into .ok([:]). Since ChatArtifactWire.payload (lines 213-222) returns nil on encode/serialize failure, any future field/type change that breaks encoding will make the RPC return a "successful" empty object rather than an error — the iOS client will then attempt to decode an empty dict into ChatArtifactStat/ChatArtifactChunk/ChatArtifactThumbnail/ChatArtifactDirectoryListing and fail in a way that's hard to diagnose, instead of surfacing a clear internal_error.
🔧 Proposed fix (repeat at each of the 4 call sites)
- return .ok(ChatArtifactWire.payload(stat) ?? [:])
+ guard let payload = ChatArtifactWire.payload(stat) else {
+ return .err(code: "internal_error", message: "Failed to encode artifact response.", data: nil)
+ }
+ return .ok(payload)Also applies to: 40-40, 58-58, 77-77, 213-222
🤖 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 `@Sources/TerminalController`+MobileChatArtifacts.swift at line 18, The four
MobileChatArtifacts RPC handlers are masking serialization failures by turning
ChatArtifactWire.payload(...) nil results into .ok([:]), which makes an encode
error look like success; update each call site in
TerminalController+MobileChatArtifacts so the ChatArtifactStat,
ChatArtifactChunk, ChatArtifactThumbnail, and ChatArtifactDirectoryListing
responses return an internal_error (or equivalent failure) when
ChatArtifactWire.payload(...) fails instead of defaulting to an empty
dictionary. Use the ChatArtifactWire.payload helper as the failure check and
keep the success path only for valid encoded payloads.
…ered frame Upgrades the terminal bottom-bar Files button into an inline popover gallery of the artifacts recognized in the currently rendered terminal frame. Two modes like the Files app: a list (thumbnail/icon + name + modified date + size) and a 3-column icon grid, toggled in the popover header; both request the same 256px thumbnail so switching modes reuses the scope-aware thumbnail cache with no refetch. Tapping an item opens the existing artifact viewer. Scan gains visible_only (default false, so tap-a-path and existing callers are unchanged): when true the Mac reads only the visible viewport, not scrollback. TerminalArtifactReference carries optional size + modified_at (unix seconds, fail-open decode). Directories are excluded from gallery results. Per-file fetch/stat/thumbnail authorization against the terminal buffer is unchanged. v1 deliberately covers only the rendered frame; folders and high-artifact-count sessions are left as a documented deferred seam. Strings localized EN+JA. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (1)
3781-3823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
visibleTextSnapshotre-duplicates thevisibleSnapshotSectioncontinuation dance.
visibleTextSnapshot(surface:generation:)repeats the same guard checks,pendingVisibleSnapshotcancel-and-replace logic,ensureSurfaceOperationDeadlinePump()call, and off-mainqueue.async→@MainActorcompletion hop asvisibleSnapshotSection(surface:generation:grid:font:)(lines 3825-3865), differing only in the final text formatting. This is the same duplication already flagged in a previous review round on this file; it's still present in this cohort. Factoring the shared continuation/pending-snapshot control flow into one private helper (formatting supplied via closure) would let future fixes to the cancellation/completion contract land in one place instead of two.♻️ Sketch of a shared helper
- private func visibleTextSnapshot(surface: ghostty_surface_t, generation: UInt64) async -> String? { - guard self.surface == surface, - surfaceGeneration == generation, - !renderPipelineRecoveryPaused else { - return nil - } - return await withCheckedContinuation { continuation in - let operationID = makeSurfaceOperationID() - if let existing = pendingVisibleSnapshot { - pendingVisibleSnapshot = nil - existing.continuation.resume(returning: nil) - } - pendingVisibleSnapshot = PendingVisibleSnapshot( - id: operationID, - startedAt: CACurrentMediaTime(), - continuation: continuation - ) - ensureSurfaceOperationDeadlinePump() - let queue = outputQueue - let read = VisibleTextRead(surface: surface, generation: generation) - queue.async { - let text = Self.surfaceText(read.surface, pointTag: GHOSTTY_POINT_VIEWPORT) - Task { `@MainActor` [weak self] in - guard let view = self else { return } - guard view.surface == read.surface, - view.surfaceGeneration == read.generation else { - view.completePendingVisibleSnapshot(id: operationID, returning: nil) - return - } - view.completePendingVisibleSnapshot(id: operationID, returning: text) - } - } - } - } + private func visibleTextSnapshot(surface: ghostty_surface_t, generation: UInt64) async -> String? { + await performVisibleSnapshotRead(surface: surface, generation: generation) { rawText in + rawText + } + }🤖 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 3781 - 3823, `visibleTextSnapshot(surface:generation:)` duplicates the same pending-snapshot continuation flow already used by `visibleSnapshotSection(surface:generation:grid:font:)`, including the guard checks, `pendingVisibleSnapshot` replacement, deadline pump setup, and queue-to-MainActor completion hop. Extract that shared control flow into one private helper in `GhosttySurfaceView` that accepts a formatting/reading closure and handles the continuation lifecycle once. Update both snapshot methods to call the helper so the cancellation and completion contract lives in one place.
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactGalleryItemView.swift`:
- Around line 111-125: The `metadataText` computed property in
`TerminalArtifactGalleryItemView` is creating a new `ByteCountFormatter` on
every render, which is too expensive in this hot path. Hoist the formatter to a
cached reusable instance (for example, a static formatter on
`TerminalArtifactGalleryItemView` or a shared helper) and have `metadataText`
reuse it when formatting `artifact.size`, while keeping the existing
`modifiedAt` formatting logic unchanged.
---
Duplicate comments:
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 3781-3823: `visibleTextSnapshot(surface:generation:)` duplicates
the same pending-snapshot continuation flow already used by
`visibleSnapshotSection(surface:generation:grid:font:)`, including the guard
checks, `pendingVisibleSnapshot` replacement, deadline pump setup, and
queue-to-MainActor completion hop. Extract that shared control flow into one
private helper in `GhosttySurfaceView` that accepts a formatting/reading closure
and handles the continuation lifecycle once. Update both snapshot methods to
call the helper so the cancellation and completion contract lives in one place.
🪄 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: cee3c7d6-35b4-4da5-9e3b-f425fd6fbe79
📒 Files selected for processing (13)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScanResponse.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalWireCodableTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShellUI/Package.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactGalleryItemView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceViewDelegate.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftSources/TerminalController+MobileTerminalArtifacts.swift
Tapping a file path that the phone re-wrapped across its narrower terminal width used to truncate at the row boundary (/tmp/.../notes.md opened as notes.m and was denied). The tap hit-tester (now in the shared package so it's unit-tested) reads the surface's real grid column count and stitches a path across soft-wrap continuation rows: when a token fills to the last column and the next row begins with a path continuation, they join with no separator, recursing for multi-row wraps; a next row that is whitespace or a fresh prompt does not over-stitch, and continuation-row taps resolve to the full path. The artifact viewer gains a scope (.chat default, terminal callers pass .terminal) so a terminal-opened file shows terminal-appropriate error copy instead of the chat-flavored 'not referenced by the conversation.' EN+JA. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift (1)
127-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProgress-callback Tasks are still not cancellation-aware; stale ticks can overwrite newer state.
The fire-and-forget
Task {@mainactorin ... }inside thefetchprogress callback remains unstructured and untied to theload()task's lifecycle. Whenpathchanges,.task(id: path)cancelsload(), but these inner Tasks keep running — a late progress tick from the previous file can overwritestateafter the new load has already reset it.🔧 Proposed fix
let data = try await loader.fetch(path: path) { fetched, total in + guard !Task.isCancelled else { return } Task { `@MainActor` in state = .loading(fetched: fetched, total: total) } }🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift` around lines 127 - 131, Make progress updates in load() cancellation-aware: remove the fire-and-forget Task created in the loader.fetch progress callback, or explicitly retain and cancel any spawned task when load() is cancelled. Before applying progress, verify the load is still active and the path/generation matches the current request so late callbacks cannot overwrite newer state.
🤖 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.
Duplicate comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`:
- Around line 127-131: Make progress updates in load() cancellation-aware:
remove the fire-and-forget Task created in the loader.fetch progress callback,
or explicitly retain and cancel any spawned task when load() is cancelled.
Before applying progress, verify the load is still active and the
path/generation matches the current request so late callbacks cannot overwrite
newer state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c32e3dc1-3d34-4c8d-8d2f-b418b520f295
📒 Files selected for processing (9)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactTapHitTester.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactTapHitTesterTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerScope.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
Replaces the buried accessory-bar Files button (which sat off-screen at the far right of the horizontally-scrolling key strip) with an auto-surfacing floating chip above the accessory bar. The chip shows a live count of file paths recognized in the currently rendered terminal frame and fades in only when that count is > 0 (and the terminal.artifact.v1 capability is present, in terminal mode); tapping it opens the same gallery (list/grid) as a detented sheet on iPhone, popover on iPad. The count is computed locally by running the path detector over the visible viewport text on the existing coalesced frame-settle signal (8 quiet frames), never per keystroke/output/render, so typing latency is unaffected; the Mac scan stays authoritative when the gallery opens. The chip sits at z=1050 below the zoom HUD, yields while zooming, and only its pill frame takes touches. .files is removed from the default accessory strip via a one-time idempotent migration (kept in Settings, same shared action path). Stale-count state is reset on capability re-enable and surface reattach so the chip re-appears after a Mac reconnect. Strings localized EN+JA incl. plural. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The soft-wrap tap stitcher decided whether a token was cut by the wrap using the trailing-TRIMMED path length, so a path wrapping immediately after a trailing-trimmable character (row ends /tmp/.../notes., next row md) read as not-full-width and did not stitch, resolving /tmp/.../notes instead of /tmp/.../notes.md. The hit-tester now tracks the raw (untrimmed) token extent for the wrap-cut decision, concatenates raw fragments across rows, and applies path detection/trimming once to the final assembled string. Same bug class as the notes.m/d report, at the punctuation boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e, chip, viewer)
Design-review batch refining the terminal artifact gallery, chip, and viewer:
Gallery sheet: image thumbnails now fit (not fill/crop); default system sheet
background; removed the fixed height clamp that left dead space at the large
detent; wrapped in a NavigationStack with an inline Files title, a Done button,
and the list/grid toggle in the toolbar; adaptive grid columns.
Gallery items: gray thumbnail card only for images (bare hierarchical, tinted
glyphs for text/binary/folder, given a square footprint so grid rows align);
localized abbreviated "Jul 9, 2026 · 12 KB" metadata; centered grid labels;
Dynamic-Type-safe min heights; pressed-state feedback; combined VoiceOver.
Chip: correct singular/plural ("1 file"/"3 files"), a files-appropriate icon
instead of the magnifying glass, an adaptive hairline that holds on light and
dark terminals, and an upward chevron matching the upward reveal.
Viewer: large text now renders in a scrollable UITextView instead of one
monolithic Text (no layout stutter); image is cleanly fitted (removed the dead
two-axis scroll); the too-large message interpolates the real limit; the
unsupported-media state no longer shows "Zero KB". Strings localized EN+JA.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lass) Owner dogfood: the list/grid view-mode picker was a segmented control sitting on the nav bar's glass (glass-on-glass); it's now a single toolbar Menu button whose icon reflects the current mode. Sharpen pass: the floating chip now uses real iOS 26 Liquid Glass (.glassEffect(.regular.interactive(), in: .capsule)) with a regularMaterial fallback below 26, so it lenses the terminal behind it and drops the redundant border and hard shadow. Grid cells all use a uniform rounded tile (images fit inside, glyphs centered) so a mixed grid no longer looks ragged; the chip icon and gallery glyphs/heights scale with Dynamic Type via @ScaledMetric; the press style no longer dims the whole row (thumbnail included), only a subtle background; a single-file chip is no longer over-padded (min width 120→88). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| }.value | ||
| return .ok(TerminalArtifactWire.payload(stat) ?? [:]) | ||
| } catch TerminalArtifactReadContext.Error.forbidden { | ||
| cmuxDebugLog("mobile.terminal.artifact.forbidden op=stat path=\(context.requestedPath ?? "nil")") |
Contributor
There was a problem hiding this comment.
cmuxDebugLog called without #if DEBUG guard — release build failure
cmuxDebugLog is defined only inside #if DEBUG in Sources/App/DebugLogging.swift. Every other call site in the codebase wraps it in #if DEBUG/#endif. These three bare calls (stat at line 49, fetch at line 76, thumbnail at line 99) will produce a "use of unresolved identifier 'cmuxDebugLog'" compile error in non-DEBUG builds. All three need to be wrapped in #if DEBUG … #endif to match the established convention.
…, search, disk cache The gallery previously showed only artifacts in the currently rendered frame. It now offers an In view | Session scope switcher (content-area segmented control; Session is the default when the terminal has a bound agent session) backed by a new session-scoped mobile.chat.artifact.gallery RPC (capability chat.artifact.gallery.v1). The transcript index now tracks provenance and last reference position, and the gallery renders Products-first disclosure sections: Created by agent and You attached arrive complete on the first page; Referenced pages ~60 at a time through a generation-pinned cursor ordered by last reference, so pages append without ever reshuffling visible rows; the paging footer lives inside the lazy containers so fetches happen on scroll, not render. Whole-session server-side search returns flat results with provenance subtitles. Files deleted since being referenced render dimmed with a localized 'No longer on your Mac' badge. Session items authorize through the existing session-scoped chat artifact verbs; the terminal scan response carries the bound session id (fail-open). Thumbnails persist in a ~100 MB disk LRU keyed by path+mtime+size+dimension with next-page prefetch, so large sessions scroll smoothly and reopen instantly. Strings localized EN+JA. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Modern codex CLI rollouts emit tool activity as event_msg payloads instead of response_item function/custom tool calls, so new codex sessions derived zero Created artifacts (a real 59-patch rollout produced nothing). Successful patch_apply_end events now synthesize an apply_patch tool use whose referenced paths are the changes keys plus non-null move_path destinations, flowing into Created provenance; failed events emit nothing; stdout/stderr/diffs never surface. Real-rollout cross-check: 0 -> 49 Created items. Also adds an env-gated single-transcript ground-truth dump test (CMUX_ARTIFACT_DUMP, paths-only) used for the verification pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner reported the In-view tab finding more artifacts than the Session tab — an inversion, since terminal text is the session's own output. An independent adversarial spec set the governing rule (Session must contain every absolute path the shared detector can see in session text) with a per-scenario taxonomy and a hard parity metric over real transcripts. Derivation now scans every text channel with the same detector the terminal uses: assistant prose, thinking, shell command strings, raw pre-truncation tool outputs (Glob/LS listings, grep path:line:col hits, error messages), user prose, and sub-agent (sidechain) activity — all as referenced; only structured mutation channels yield created, and only the mutation target (sidechain Write content blobs no longer become created artifacts). The shared detector gains markdown-link and line:col handling (In view improves in lockstep) and symmetrically rejects junk (bare /, code fragments with interior parens or quotes, template placeholders). Key-agnostic structured values count only as single whitespace-free absolute tokens. /var and /etc fold like /tmp; ~ and file:// forms resolve. Parity audit over 300 real transcripts (150 Claude + 150 Codex, read-only): BEFORE 217 violating transcripts with 115,388 missed path references (codex 145/150); AFTER 0 violations, zero junk in the gallery, median growth 1 item. Adversarial fixtures pin every taxonomy row by ID; 272 package tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The files accessory case was inserted before ollama, shifting ollama's persisted rawValue from 30 to 31 and failing the append-only pin in TerminalAccessoryConfigurationTests on both simulator jobs. Move files after ollama (rawValue 31) and pin it. The bar position is unaffected: defaultConfigurableOrder curates placement explicitly. Also untrack ios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolved, committed by accident in 976512f; check-package-resolved-policy.py rejects that location and failed workflow-guard-tests before the other guard steps could run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The floating artifact chip counted only path tokens in the visible frame, so it disagreed with the Session-first files sheet it opens. The settle cadence still triggers locally, but when the visible snapshot changes the host now asks the Mac for the bound session's complete gallery count via a count_only terminal artifact scan (no terminal-text capture, no stat; the same derivation snapshot the gallery pages, so missing files still count). TerminalArtifactChipCountState keeps one request in flight with one trailing coalesce; generation guards drop responses from a previous attachment. Falls back to the local frame count when the gallery capability, a bound session, or the RPC is unavailable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…at tests in CI A count response dropped for a surface-generation mismatch (generations bump on every output apply, scroll snap, and grid change) with no trailing request left the chip stuck until the visible text changed. The state machine now distinguishes that drop and re-issues one request tagged with the current generation, bounded to three consecutive re-arms per state generation. A zero session total with path tokens on screen reports the local frame count so the chip stays as the entry point to the In-view tab; a positive session total always wins. Stale tasks can no longer clear a newer request's handle. Pays the whole PR's file-length debt by pure code motion into new sub-500 line files (GhosttySurfaceView+Artifacts, GhosttySurfaceCoordinator+ Artifacts, TerminalArtifactFilesSheet+Content, CodexTranscriptParser+ ToolOutput, and seven more); no budget allowance raised. Adds CmuxAgentChat to the swift-package-tests CI lane so the 274 parity and wire-schema tests run on PRs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
cmuxDebugLog exists only under #if DEBUG; the three bare denial-log calls in the terminal artifact RPCs failed non-DEBUG compiles (Greptile P1). Route them through a #if DEBUG helper like the chat-side denial log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Collaborator
Author
|
@greptileai review |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 27
♻️ Duplicate comments (2)
Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift (1)
133-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProgress-callback Tasks aren't cancellation-aware; stale ticks can overwrite newer state.
Each progress tick spawns a new unstructured
Task {@mainactorin ... }. Whenpathchanges,.task(id: path)cancels the outerload()Task, but these inner detached Tasks are not children of it and keep running, so a late progress tick from the previous file's fetch can overwritestateafter the new load has already reset it — briefly showing the wrong progress under the new file's title.As per coding guidelines, do not create fire-and-forget
Task { ... }work with meaningful lifecycle unless it is stored, cancellable, or tied to a caller-owned operation.🔧 Proposed fix
- let data = try await loader.fetch(path: path) { fetched, total in - Task { `@MainActor` in - state = .loading(fetched: fetched, total: total) - } - } + let data = try await loader.fetch(path: path) { fetched, total in + guard !Task.isCancelled else { return } + Task { `@MainActor` in + state = .loading(fetched: fetched, total: total) + } + }🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift` around lines 133 - 137, Update the progress callback in the load flow around loader.fetch so it does not spawn unstructured Task work. Propagate progress updates through the caller-owned async operation, or otherwise make the update cancellation-aware and tied to the active load(path) task, ensuring callbacks from a cancelled previous fetch cannot mutate state after a new path begins loading.Source: Coding guidelines
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swift (1)
54-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
canonicalPath(for:)skips the existence filter thatartifactPaths()applies.
artifactPaths()(Line 38) requiresresolver.isDirectory(candidate) != nilbefore canonicalizing a detected candidate, i.e. it only trusts candidates that actually exist on disk.canonicalPath(for:)(lines 62-67) canonicalizes every detected candidate directly, without that existence check, so authorization for a single requested path is derived from a broader, less-verified candidate set than the one exposed viaartifactPaths(). The two entry points should apply identical scope rules; otherwise a path could be authorized throughcanonicalPath(for:)under conditions thatartifactPaths()would have excluded (or vice versa) purely due to this inconsistency.As per path instructions,
.github/review-bot-rules/reliability-single-source-of-truth.mdrequires a single authoritative scope check rather than divergent authorization paths for the same correctness-critical decision (which paths a session may access).🛡️ Proposed fix: share one filtering path
public func canonicalPath(for path: String) -> String? { guard let absoluteRequest = absolutePath(for: path) else { return nil } guard let canonicalRequest = ChatArtifactScope.canonicalizedPath(absoluteRequest, resolver: resolver) else { return nil } var seen: Set<String> = [] for candidate in detector.paths(in: terminalText).compactMap(absolutePath(for:)) { - guard let canonical = ChatArtifactScope.canonicalizedPath(candidate, resolver: resolver), + guard resolver.isDirectory(candidate) != nil, + let canonical = ChatArtifactScope.canonicalizedPath(candidate, resolver: resolver), !seen.contains(canonical) else { continue }🤖 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/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swift` around lines 54 - 73, Update canonicalPath(for:) to apply the same existence filtering as artifactPaths() before canonicalizing detected candidates, using resolver.isDirectory(candidate) != nil. Reuse the existing authoritative filtering path or matching rule so both entry points enforce identical artifact scope decisions, while preserving the current deduplication and requested-path matching behavior.Source: Path instructions
🤖 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/Artifacts/ChatArtifactFolderView.swift`:
- Around line 88-90: Update the file-size display in ChatArtifactFolderView’s
list-row rendering to reuse a cached ByteCountFormatter or reusable format style
rather than calling ByteCountFormatter.string on every render. Keep the existing
file count style and displayed value unchanged.
- Around line 77-103: Update row(_ entry:) so directory taps navigate to a
ChatArtifactFolderView for childPath(named: entry.name), while non-directory
taps continue assigning selectedFile for the file viewer. Remove the directory
guard and disabled(entry.isDirectory), preserving the existing row presentation.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swift`:
- Around line 15-22: Update the font assignment in ChatArtifactTextView so the
monospaced base font is passed through UIFontMetrics(forTextStyle:
.body).scaledFont(for:). Keep adjustsFontForContentSizeCategory enabled and
preserve the existing body point size and regular weight.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`:
- Around line 247-249: Update formattedSize(_:) to reuse a cached or shared
ByteCountFormatter instead of calling the static formatting API per render. Keep
the existing .file count style and formatted output unchanged while ensuring
formatter allocation occurs only once.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailSheetView.swift`:
- Around line 153-179: Update the row state around stat and the directory/file
action in ChatBlockDetailSheetView to explicitly represent loading, resolved
file, resolved directory, and failure instead of using optional metadata.
Disable browsing/viewing actions until stat succeeds, route only confirmed
directories to selectedFolder and files to selectedArtifact, and show
retry/error UI when stat fails rather than silently discarding the error.
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swift`:
- Around line 141-166: Update the thumbnail state and loading flow in
ChatAttachmentBubbleView: replace thumbnailData with a stored loadedThumbnail
Image, decode the artifact data once inside loadThumbnail, and mark
thumbnailFailed when decoding is unsupported or unsuccessful. Simplify
thumbnailImage to render the stored Image with resizable/scaledToFill or fall
back to placeholderThumbnail, preserving the existing path-change and failure
guards.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swift`:
- Around line 252-282: Extract the shared chunked-transfer logic from
artifactFetch and terminalArtifactFetch into a private artifactFetchLoop helper
that accepts the RPC method, base parameters, and progress callback. Preserve
the existing reserve-capacity, offset, EOF, and empty-chunk handling exactly,
then have both public methods supply their method-specific names and parameters
to the helper.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipCountState.swift`:
- Around line 84-116: Remove the automatic mismatch-repair re-arm from
TerminalArtifactChipCountState’s completion handling so dropped mismatched
completions return no nextRequest; the current surface must trigger a new
request with its own localCount. Update
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipCountState.swift
lines 84-116 accordingly, and update
Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalArtifactChipCountStateTests.swift
lines 71-123 to replace bounded re-arm expectations with fail-closed dropping
followed by a caller-triggered current-surface request.
- Around line 54-66: Update trigger in the supportsSessionCount == false branch
to invalidate any existing inFlight request before reporting the local fallback.
Ensure a later completion cannot overwrite the fallback, while preserving the
current Report behavior and supported-session flow.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipView.swift`:
- Around line 41-47: Update localizedCount to use the project's explicit
pluralization convention with a stable localization key and separate .one/.other
entries, matching existing count strings such as
statusMenu.unreadCount.one/.other. Ensure Localizable.xcstrings contains the
exact plural keys and translations for every supported locale, while preserving
the current count output.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swift`:
- Around line 111-121: Update the session loader initialization in the
session-selection flow to pass the sheet’s existing shared thumbnail cache into
ChatArtifactLoader instead of relying on its default cache. Preserve the current
source and resolvedSessionID values while ensuring session-scoped and in-view
lookups reuse the same cache.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView`+Artifacts.swift:
- Around line 137-222: Extract the shared pending-snapshot lifecycle from
visibleTextSnapshot and visibleSnapshotSection into one helper that owns
validation, cancellation/replacement, deadline-pump setup, outputQueue dispatch,
generation checks, and MainActor completion. Make the helper return the
generalized (text: String, columns: Int) payload, while accepting only the
differing formatting step or result transformation; update both methods to use
it and preserve their existing output behavior.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryOrdering.swift`:
- Around line 31-41: The ChatArtifactGalleryOrdering.items method should page
from one ordered snapshot instead of filtering and materializing the full
remainder: sort once, seek directly past strictlyAfter using the existing
ordering, and return only the requested bounded page. In
AgentChatArtifactGalleryBuilder, update the candidates flow to reuse that
ordered snapshot and apply the page limit during retrieval; remove the
re-sorting and full-remainder materialization before prefix(pageSize). Apply
these changes at
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryOrdering.swift:31-41
and Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift:18-30.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryPage.swift`:
- Around line 47-56: Update ChatArtifactGalleryPage.init(from:) to log decoding
failures for the created, attached, and referenced artifact arrays before
falling back to empty arrays. Preserve the existing fallback behavior, but
capture each decode error separately so malformed sections are diagnosable
rather than indistinguishable from genuinely empty results.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactIndexedReference.swift`:
- Line 4: Mark the pure value-model struct ChatArtifactIndexedReference as
nonisolated. Apply the same declaration change to ChatArtifactPathNormalizer in
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactPathNormalizer.swift
(line 4); no other changes are needed.
- Around line 115-118: The file-mutation tool predicate is duplicated and can
drift between artifact and transcript parsing. Add one shared helper near the
provenance/shared artifact types that normalizes tool names and owns the
complete mutation-tool set; update
ChatArtifactIndexedReference.isFileMutationTool and
ClaudeTranscriptParser.isMutationTool to delegate to it, removing their
independent tool lists. Apply this in
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactIndexedReference.swift
lines 115-118 and
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ClaudeTranscriptParser.swift
lines 468-472.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactPathNormalizer.swift`:
- Around line 97-105: Make inferredHomeDirectory() fail closed by returning no
home directory when workingDirectory is absent or not a valid /Users/<name>
path, instead of calling NSHomeDirectory(). Update the artifact path
normalization call site to leave tilde-prefixed paths unresolved or reject them
when no inferred home is available, rather than falling back to a host
directory.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactPathDetector.swift`:
- Around line 89-109: The isPathLike guard currently rejects explicit
single-segment relative paths before their prefix handling. Update isPathLike so
./ and ../ prefixed candidates are accepted before hasEnoughPathComponents is
evaluated, while preserving the existing validation and behavior for absolute
and other path-like candidates.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScanResponse.swift`:
- Around line 48-58: Update the TerminalArtifactScanResponse decoder’s optional
metadata handling for size and modifiedAt to use decodeIfPresent, preserving nil
only when fields are absent while propagating malformed type or element errors.
Keep the existing timestamp conversion from numeric seconds to Date and the Date
decoding fallback, but remove blanket try? usage that can hide invalid payloads.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactTapHitTester.swift`:
- Around line 19-31: The terminal hit-testing loop around stitchedPath must
avoid rescanning forward from every token. First identify the wrapped
logical-line region containing the tapped row, then scan that region once to
evaluate candidate tokens, preserving the existing bestMatch selection and hit
containment behavior.
- Around line 47-58: Update the continuation and related hit-testing
calculations in TerminalArtifactTapHitTester, including the logic around
leadingContinuation and GridSegment construction, to use the terminal cell-width
utility instead of String.count or Character-based offsets. Apply this
consistently at the referenced wrap-boundary and segment-length calculations so
wide and combining Unicode characters produce correct grid columns.
In `@Resources/Localizable.xcstrings`:
- Around line 124046-124079: Replace the English and Japanese values for
mobile.chat.artifact.error.galleryInvalidParams and
mobile.chat.artifact.error.invalidParams in Resources/Localizable.xcstrings
lines 124046-124079 with product-focused messages that do not expose internal
parameter names or session IDs. Apply the same user-friendly wording change to
mobile.terminal.artifact.error.invalidParams in Resources/Localizable.xcstrings
lines 124148-124164, preserving both locale entries.
In `@Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift`:
- Around line 40-51: Update the first-page non-search path in the artifact
gallery builder so statItems only enriches at most pageSize created and attached
artifacts, rather than the full filtered item sets. Preserve the existing
ordering and provenance filtering, and leave referenced handling unchanged.
In `@Sources/Mobile/AgentChat/AgentChatArtifactIndex.swift`:
- Line 40: Bound the session snapshot cache represented by cacheBySessionID to
prevent indefinite growth in the long-lived artifact index actor. Add a simple
maximum-count eviction policy or an explicit session-removal path, and ensure
accesses to new sessions cannot leave more than the configured limit stored.
In `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift`:
- Around line 262-264: Replace the full-record filtering and max scan in the
session lookup with a mostRecentSessionIDBySurfaceID index keyed by surfaceID.
Update this index whenever records are added, updated, or removed, then resolve
the indexed session after liveSession while preserving the latest lastActivityAt
behavior.
In `@Sources/TerminalController`+MobileChatArtifacts.swift:
- Line 54: Replace each ChatArtifactWire.payload fallback in
Sources/TerminalController+MobileChatArtifacts.swift at lines 54, 93, 121, 145,
and 167 with guard-let handling for the page, stat, chunk, thumbnail, and
listing payloads. Return .err(code: "internal_error", ...) when encoding returns
nil; otherwise preserve the existing successful response flow.
In `@Sources/TerminalController`+MobileTerminalArtifacts.swift:
- Around line 276-296: The Sendable value type TerminalArtifactReadContext must
be explicitly excluded from MainActor-by-default isolation. Mark the struct
nonisolated while preserving its stored properties, initializer, and nested
Error definition unchanged.
---
Duplicate comments:
In
`@Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`:
- Around line 133-137: Update the progress callback in the load flow around
loader.fetch so it does not spawn unstructured Task work. Propagate progress
updates through the caller-owned async operation, or otherwise make the update
cancellation-aware and tied to the active load(path) task, ensuring callbacks
from a cancelled previous fetch cannot mutate state after a new path begins
loading.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swift`:
- Around line 54-73: Update canonicalPath(for:) to apply the same existence
filtering as artifactPaths() before canonicalizing detected candidates, using
resolver.isDirectory(candidate) != nil. Reuse the existing authoritative
filtering path or matching rule so both entry points enforce identical artifact
scope decisions, while preserving the current deduplication and requested-path
matching behavior.
🪄 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: bad2128d-e387-4665-9a2e-8282549f256b
📒 Files selected for processing (116)
.github/workflows/ci.yml.gitignorePackages/Shared/CmuxAgentChat/Package.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ArtifactByteReader.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactChunk.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactDirectoryEntry.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactDirectoryListing.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactError.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryCursor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryItem.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryOrdering.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactGalleryPage.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactIndexedReference.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactKind.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactPathNormalizer.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactProvenance.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactScope.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactStat.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactThumbnail.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactTranscriptReference.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/ChatArtifactTransferPolicy.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactPathDetector.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScanResponse.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactScope.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Artifacts/TerminalArtifactTapHitTester.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Model/ChatToolUse.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatArtifactTextReferenceExtractor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatAttachmentTokenExtractor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatToolReferencedPathExtractor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ChatTranscriptParseResult.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/ClaudeTranscriptParser.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/CodexTranscriptParser+ToolOutput.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/CodexTranscriptParser.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/TranscriptBatchAssembler.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/TranscriptToolCompletion.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Source/ChatEventSource.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactDiscoveryAudit+Reporting.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactDiscoveryAudit.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactDiscoveryAuditTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactGroundTruthDumpTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactParityAuditSnapshot.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactParityFixture.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ArtifactParityFixtureTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatArtifactGalleryTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatArtifactScopeTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatArtifactTransferPolicyTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ClaudeTranscriptParserProseTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ClaudeTranscriptParserTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/CodexTranscriptParserTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/Resources/ArtifactParity/claude-adversarial.jsonlPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/Resources/ArtifactParity/codex-adversarial.jsonlPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactPathDetectorTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactScopeTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalArtifactTapHitTesterTests.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/TerminalWireCodableTests.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactLoader.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactThumbnailDiskCache.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerScope.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Resources/Localizable.xcstringsPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetail.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailBuilder.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailSheetView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swiftPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatAttachmentBubbleView.swiftPackages/iOS/CmuxAgentChatUI/Tests/CmuxAgentChatUITests/ChatArtifactLoaderTests.swiftPackages/iOS/CmuxAgentChatUI/Tests/CmuxAgentChatUITests/ChatArtifactThumbnailDiskCacheTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+AgentChat.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+Capabilities.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Package.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceCoordinator+Artifacts.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/Resources/Localizable.xcstringsPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalAccessoryChatCompatibility.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipCountState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipSurfaceModifier.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactChipView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet+Content.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactFilesSheet.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactGalleryButtonStyle.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactGalleryDisplayItem.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalArtifactGalleryItemView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalArtifacts.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalArtifactChipCountStateTests.swiftPackages/iOS/CmuxMobileTerminal/Package.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceArtifactChipHost.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+Artifacts.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceViewDelegate.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputAccessoryAction.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView+CommittedTextSequences.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftResources/Localizable.xcstringsSources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swiftSources/Mobile/AgentChat/AgentChatArtifactIndex.swiftSources/Mobile/AgentChat/AgentChatSessionRegistry.swiftSources/Mobile/AgentChat/AgentChatTranscriptService+ArtifactSession.swiftSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService+TicketAuthorization.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileChat.swiftSources/TerminalController+MobileChatArtifacts.swiftSources/TerminalController+MobileTerminalArtifacts.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift
💤 Files with no reviewable changes (1)
- Sources/Mobile/MobileHostService.swift
Comment on lines
+77
to
+103
| private func row(_ entry: ChatArtifactDirectoryEntry) -> some View { | ||
| Button { | ||
| guard !entry.isDirectory else { return } | ||
| selectedFile = ChatArtifactPathSelection(path: childPath(named: entry.name)) | ||
| } label: { | ||
| HStack(spacing: 10) { | ||
| ChatArtifactFolderThumbnail(path: childPath(named: entry.name), entry: entry) | ||
| VStack(alignment: .leading, spacing: 2) { | ||
| Text(entry.name) | ||
| .lineLimit(1) | ||
| .truncationMode(.middle) | ||
| if !entry.isDirectory { | ||
| Text(ByteCountFormatter.string(fromByteCount: entry.size, countStyle: .file)) | ||
| .font(.caption) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } | ||
| Spacer(minLength: 8) | ||
| if entry.isDirectory { | ||
| Image(systemName: "folder") | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } | ||
| } | ||
| .buttonStyle(.plain) | ||
| .disabled(entry.isDirectory) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow directory rows to open their contents.
Lines 79 and 102 make every directory inert, so artifacts in nested folders are unreachable. Route directory taps to another ChatArtifactFolderView while keeping files on the viewer 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/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swift`
around lines 77 - 103, Update row(_ entry:) so directory taps navigate to a
ChatArtifactFolderView for childPath(named: entry.name), while non-directory
taps continue assigning selectedFile for the file viewer. Remove the directory
guard and disabled(entry.isDirectory), preserving the existing row presentation.
Comment on lines
+88
to
+90
| if !entry.isDirectory { | ||
| Text(ByteCountFormatter.string(fromByteCount: entry.size, countStyle: .file)) | ||
| .font(.caption) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Reuse the byte-count formatter outside the list-row path.
Line 89 formats on every row render. Cache a ByteCountFormatter under the UI actor, or use a reusable format style, instead of the per-call convenience API.
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactFolderView.swift`
around lines 88 - 90, Update the file-size display in ChatArtifactFolderView’s
list-row rendering to reuse a cached ByteCountFormatter or reusable format style
rather than calling ByteCountFormatter.string on every render. Keep the existing
file count style and displayed value unchanged.
Sources: Coding guidelines, Path instructions
Comment on lines
+15
to
+22
| textView.adjustsFontForContentSizeCategory = true | ||
| textView.font = .monospacedSystemFont( | ||
| ofSize: UIFont.preferredFont(forTextStyle: .body).pointSize, | ||
| weight: .regular | ||
| ) | ||
| textView.textContainerInset = UIEdgeInsets(top: 16, left: 16, bottom: 16, right: 16) | ||
| textView.textContainer.lineFragmentPadding = 0 | ||
| textView.text = text |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- File outline ---\n'
ast-grep outline Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swift --view expanded || true
printf '\n--- File excerpt ---\n'
cat -n Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swift | sed -n '1,200p'Repository: manaflow-ai/cmux
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swift')
text = p.read_text()
for needle in [
'adjustsFontForContentSizeCategory',
'monospacedSystemFont',
'preferredFont(forTextStyle:',
'UIFontMetrics',
'updateUIView',
]:
print(f'--- {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: manaflow-ai/cmux
Length of output: 564
🌐 Web query:
UIFont.monospacedSystemFont(ofSize:weight:) adjustsFontForContentSizeCategory UIFontMetrics scaledFont dynamic type
💡 Result:
UIFont.monospacedSystemFont(ofSize:weight:) returns a fixed-size font and does not automatically support Dynamic Type scaling [1][2]. To support Dynamic Type with a monospaced font, you must combine it with UIFontMetrics [3][4]. To achieve this, you should use UIFontMetrics to scale the monospaced font instance [4]. Additionally, you must set the adjustsFontForContentSizeCategory property to true on your UI element (such as a UILabel or UITextView) to ensure the control updates when the user changes their Dynamic Type setting [4]. Example implementation: let monospacedFont = UIFont.monospacedSystemFont(ofSize: 17, weight:.regular) let scaledFont = UIFontMetrics(forTextStyle:.body).scaledFont(for: monospacedFont) label.font = scaledFont label.adjustsFontForContentSizeCategory = true Using UIFontMetrics(forTextStyle:) allows the system to apply scaling behavior consistent with the chosen text style [4]. Note that for cases where only digits need to be monospaced, you can use UIFont.monospacedDigitSystemFont(ofSize:weight:), though scaling it with UIFontMetrics is still required to support Dynamic Type [2][4].
Citations:
- 1: https://developer.apple.com/documentation/uikit/uifont/monospacedsystemfont(ofsize:weight:)
- 2: https://sosumi.ai/documentation/uikit/uifont/monospacedsystemfont(ofsize:weight:)
- 3: https://developer.apple.com/documentation/uikit/uifontmetrics
- 4: https://apple-docs.everest.mt/docs/uikit/scaling-fonts-automatically/
Use a scaled monospaced font here. adjustsFontForContentSizeCategory won’t rescale the plain .monospacedSystemFont(ofSize:weight:) font, so this text can stay stuck at the initial size when Dynamic Type changes. Wrap the base font in UIFontMetrics(forTextStyle: .body).scaledFont(for:) instead.
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactTextView.swift`
around lines 15 - 22, Update the font assignment in ChatArtifactTextView so the
monospaced base font is passed through UIFontMetrics(forTextStyle:
.body).scaledFont(for:). Keep adjustsFontForContentSizeCategory enabled and
preserve the existing body point size and regular weight.
Comment on lines
+247
to
+249
| private func formattedSize(_ bytes: Int64) -> String { | ||
| ByteCountFormatter.string(fromByteCount: bytes, countStyle: .file) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid allocating ByteCountFormatter per call in a frequent render path.
ByteCountFormatter.string(fromByteCount:countStyle:) allocates a new formatter instance under the hood on every call. Because this is called on every progress tick to re-render the view, it creates unnecessary allocation overhead on the main thread.
As per coding guidelines, do not allocate ByteCountFormatter per call; allocate once and reuse a cached or shared formatter.
♻️ Proposed fix
+ private static let byteFormatter: ByteCountFormatter = {
+ let formatter = ByteCountFormatter()
+ formatter.countStyle = .file
+ return formatter
+ }()
+
private func formattedSize(_ bytes: Int64) -> String {
- ByteCountFormatter.string(fromByteCount: bytes, countStyle: .file)
+ Self.byteFormatter.string(fromByteCount: bytes)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func formattedSize(_ bytes: Int64) -> String { | |
| ByteCountFormatter.string(fromByteCount: bytes, countStyle: .file) | |
| } | |
| private static let byteFormatter: ByteCountFormatter = { | |
| let formatter = ByteCountFormatter() | |
| formatter.countStyle = .file | |
| return formatter | |
| }() | |
| private func formattedSize(_ bytes: Int64) -> String { | |
| Self.byteFormatter.string(fromByteCount: bytes) | |
| } |
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Artifacts/ChatArtifactViewerSheet.swift`
around lines 247 - 249, Update formattedSize(_:) to reuse a cached or shared
ByteCountFormatter instead of calling the static formatting API per render. Keep
the existing .file count style and formatted output unchanged while ensuring
formatter allocation occurs only once.
Source: Coding guidelines
Comment on lines
+153
to
+179
| if stat?.isDirectory == true { | ||
| Button { | ||
| selectedFolder = ChatArtifactPathSelection(path: path) | ||
| } label: { | ||
| Label( | ||
| String(localized: "chat.artifact.browse_folder", defaultValue: "Browse folder", bundle: .module), | ||
| systemImage: "folder" | ||
| ) | ||
| } | ||
| .labelStyle(.iconOnly) | ||
| } else { | ||
| Button { | ||
| selectedArtifact = ChatArtifactPathSelection(path: path) | ||
| } label: { | ||
| Label( | ||
| String(localized: "chat.artifact.view_file", defaultValue: "View file", bundle: .module), | ||
| systemImage: "doc.text.magnifyingglass" | ||
| ) | ||
| } | ||
| .labelStyle(.iconOnly) | ||
| } | ||
| } | ||
| .padding(10) | ||
| .background(.quaternary.opacity(0.5), in: .rect(cornerRadius: 8)) | ||
| .task(id: path) { | ||
| stat = try? await loader.stat(path: path) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat unresolved metadata as a file.
While stat is loading—or if it fails—the row exposes “View file.” A directory tapped in that state is routed to the wrong viewer, and errors are silently discarded. Represent loading, file, directory, and failure explicitly; disable the action until resolved and provide retry/error UI.
As per coding guidelines: “flag fixes that patch symptoms while leaving bad state representable.” <coding_guidelines>
🤖 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/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatBlockDetailSheetView.swift`
around lines 153 - 179, Update the row state around stat and the directory/file
action in ChatBlockDetailSheetView to explicitly represent loading, resolved
file, resolved directory, and failure instead of using optional metadata.
Disable browsing/viewing actions until stat succeeds, route only confirmed
directories to selectedFolder and files to selectedArtifact, and show
retry/error UI when stat fails rather than silently discarding the error.
Source: Coding guidelines
Comment on lines
+40
to
+51
| let includeCompleteSections = cursor == nil && !isSearch | ||
| let created = includeCompleteSections | ||
| ? statItems(ordering.sorted(items.filter { $0.provenance == .created })) | ||
| : [] | ||
| let attached = includeCompleteSections | ||
| ? statItems(ordering.sorted(items.filter { $0.provenance == .attached })) | ||
| : [] | ||
| return ChatArtifactGalleryPage( | ||
| sessionID: sessionID, | ||
| created: created, | ||
| attached: attached, | ||
| referenced: statItems(pageReferences), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File with line numbers\n'
cat -n Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift
printf '\n## AST outline\n'
ast-grep outline Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift --view expandedRepository: manaflow-ai/cmux
Length of output: 4536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swiftRepository: manaflow-ai/cmux
Length of output: 4312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "func statItems|statItems\(" Sources/Mobile/AgentChat -SRepository: manaflow-ai/cmux
Length of output: 668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift | nl -baRepository: manaflow-ai/cmux
Length of output: 194
Bound the first-page stat work. The first non-search page still stats every created and attached artifact, so this path scales with the full item set instead of pageSize. Cap those sections or move the enrichment behind a paged/background snapshot 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 `@Sources/Mobile/AgentChat/AgentChatArtifactGalleryBuilder.swift` around lines
40 - 51, Update the first-page non-search path in the artifact gallery builder
so statItems only enriches at most pageSize created and attached artifacts,
rather than the full filtered item sets. Preserve the existing ordering and
provenance filtering, and leave referenced handling unchanged.
Source: Coding guidelines
| let snapshot: Snapshot | ||
| } | ||
|
|
||
| private var cacheBySessionID: [String: CacheEntry] = [:] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded cache growth causes a memory leak.
cacheBySessionID stores session snapshots indefinitely. Because this actor is owned by a long-lived app singleton (TerminalControllerChatArtifactIndexProvider.shared) and lacks an eviction mechanism, this dictionary will grow unbounded over the app's lifetime as new sessions are accessed.
Please bound the size of this cache (e.g., using an LRU policy or a simple maximum count limit) or expose a method to remove sessions when they end.
🤖 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 `@Sources/Mobile/AgentChat/AgentChatArtifactIndex.swift` at line 40, Bound the
session snapshot cache represented by cacheBySessionID to prevent indefinite
growth in the long-lived artifact index actor. Add a simple maximum-count
eviction policy or an explicit session-removal path, and ensure accesses to new
sessions cannot leave more than the configured limit stored.
Comment on lines
+262
to
+264
| return records.values | ||
| .filter { $0.surfaceID == surfaceID } | ||
| .max { $0.lastActivityAt < $1.lastActivityAt } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Index the latest session by surface instead of scanning every record.
This runs an allocating O(session count) scan for every artifact request, including each fetch chunk. Maintain a mostRecentSessionIDBySurfaceID index as records change and use it after liveSession.
As per coding guidelines: “Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code.” <coding_guidelines>
🤖 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 `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift` around lines 262 -
264, Replace the full-record filtering and max scan in the session lookup with a
mostRecentSessionIDBySurfaceID index keyed by surfaceID. Update this index
whenever records are added, updated, or removed, then resolve the indexed
session after liveSession while preserving the latest lastActivityAt behavior.
Sources: Coding guidelines, Path instructions
| query: query | ||
| ) | ||
| }.value | ||
| var payload = ChatArtifactWire.payload(page) ?? [:] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Encoding failures silently become fake successful empty responses.
At all five call sites, ChatArtifactWire.payload(...) ?? [:] folds an encoding/serialization failure into .ok([:]). Since ChatArtifactWire.payload returns nil on encode/serialize failure, any future field/type change that breaks encoding will make the RPC return a "successful" empty object rather than an error — the iOS client will then attempt to decode an empty dict and fail in a way that's hard to diagnose, instead of surfacing a clear internal_error.
Sources/TerminalController+MobileChatArtifacts.swift#L54-L54: Use aguard letcheck and return.err(code: "internal_error", ...)ifpayloadis nil.Sources/TerminalController+MobileChatArtifacts.swift#L93-L93: Apply the sameguard letcheck for thestatpayload.Sources/TerminalController+MobileChatArtifacts.swift#L121-L121: Apply the sameguard letcheck for thechunkpayload.Sources/TerminalController+MobileChatArtifacts.swift#L145-L145: Apply the sameguard letcheck for thethumbnailpayload.Sources/TerminalController+MobileChatArtifacts.swift#L167-L167: Apply the sameguard letcheck for thelistingpayload.
📍 Affects 1 file
Sources/TerminalController+MobileChatArtifacts.swift#L54-L54(this comment)Sources/TerminalController+MobileChatArtifacts.swift#L93-L93Sources/TerminalController+MobileChatArtifacts.swift#L121-L121Sources/TerminalController+MobileChatArtifacts.swift#L145-L145Sources/TerminalController+MobileChatArtifacts.swift#L167-L167
🤖 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 `@Sources/TerminalController`+MobileChatArtifacts.swift at line 54, Replace
each ChatArtifactWire.payload fallback in
Sources/TerminalController+MobileChatArtifacts.swift at lines 54, 93, 121, 145,
and 167 with guard-let handling for the page, stat, chunk, thumbnail, and
listing payloads. Return .err(code: "internal_error", ...) when encoding returns
nil; otherwise preserve the existing successful response flow.
Comment on lines
+276
to
+296
| private struct TerminalArtifactReadContext: Sendable { | ||
| enum Error: Swift.Error { | ||
| case forbidden | ||
| } | ||
|
|
||
| private let terminalText: String | ||
| private let workingDirectory: String? | ||
| let requestedPath: String? | ||
| let sessionID: String? | ||
|
|
||
| init( | ||
| terminalText: String, | ||
| workingDirectory: String?, | ||
| requestedPath: String?, | ||
| sessionID: String? | ||
| ) { | ||
| self.terminalText = terminalText | ||
| self.workingDirectory = workingDirectory | ||
| self.requestedPath = requestedPath | ||
| self.sessionID = sessionID | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'Package.swift|project.pbxproj|swift-version|tool-versions' \
-x rg -n -C2 'defaultIsolation|SWIFT_DEFAULT_ACTOR_ISOLATION|swiftLanguageMode|SWIFT_VERSION' {}Repository: manaflow-ai/cmux
Length of output: 30817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Sources/TerminalController+MobileTerminalArtifacts.swift'
echo '--- surrounding lines ---'
sed -n '240,340p' "$file" | cat -n
echo
echo '--- search for nonisolated type declarations in repo ---'
rg -n --glob='*.swift' '^\s*nonisolated\s+(?:struct|class|enum|protocol)|^\s*(?:struct|class|enum|protocol)\s+\w+.*nonisolated' Sources CLI Packages cmuxTests cmuxUITests || true
echo
echo '--- search for nonisolated Sendable value types in repo ---'
rg -n --glob='*.swift' 'nonisolated.*Sendable|Sendable.*nonisolated' Sources CLI Packages cmuxTests cmuxUITests || trueRepository: manaflow-ai/cmux
Length of output: 18958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '240,340p' 'Sources/TerminalController+MobileTerminalArtifacts.swift' | cat -nRepository: manaflow-ai/cmux
Length of output: 4443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Sources/TerminalController+MobileTerminalArtifacts.swift'
echo '--- TerminalArtifactReadContext usages ---'
rg -n 'TerminalArtifactReadContext|Task\.detached' "$file"
echo
echo '--- broader context around the detached task ---'
sed -n '120,240p' "$file" | cat -nRepository: manaflow-ai/cmux
Length of output: 7430
Mark TerminalArtifactReadContext nonisolated.
Sources/TerminalController+MobileTerminalArtifacts.swift:276
This is a pure Sendable value passed into Task.detached, so keep it explicitly off MainActor-by-default isolation.
🤖 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 `@Sources/TerminalController`+MobileTerminalArtifacts.swift around lines 276 -
296, The Sendable value type TerminalArtifactReadContext must be explicitly
excluded from MainActor-by-default isolation. Mark the struct nonisolated while
preserving its stored properties, initializer, and nested Error definition
unchanged.
Sources: Coding guidelines, Path instructions
# Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
The round-18 bare-root gate standardized every candidate before the two-component floor, so a single-segment relative token like ./notes.md collapsed to one component and stopped being tappable; the floor now applies only to absolute tokens, which is the shape the junk rule was aimed at. The three artifact invalid-params errors named wire parameters (session_id, workspace_id); they now say what cmux couldn't determine, in English and Japanese. Both from CodeRabbit's full review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
11 tasks
Collaborator
Author
|
Adjudication of the CodeRabbit full review (27 actionable comments), for the merge record. Fixed on this PR (bffef35 + e47b9e7):
Rejected — Critical: tilde fallback "file disclosure" (ChatArtifactPathNormalizer). The artifact scope only ever contains paths the user's own transcript references, and the only client that can request them is the same Stack account's paired device; resolving Rejected: fail-open wire decode in TerminalArtifactScanResponse is the deliberate cross-version skew contract (pinned by tests); gallery page-1 completeness for created/attached is an owner-locked design decision. Deferred to #8044 (real, not merge-blocking): CJK cell-width tap extents, artifact index cache bound, folder drill-in, viewer progress-tick cancellation, detail-sheet stat states, fetch-loop dedupe, registry surface index, page-1 stat bounding, ordering snapshot reuse, encode-failure surfacing, chip localCount ownership. 🤖 Generated with Claude Code |
This was referenced Jul 16, 2026
iOS: complete chip files gallery — dedup, folders, streaming viewer, previews, gallery UX, QoL
#8287
Merged
6 of 8 tasks
This branch was successfully deployed
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.
iOS users could not view artifacts their agent sessions reference (images attached to the session, files/images the agent read or wrote, images in directories the agent mentioned) because the bytes live only on the Mac. The iOS transcript showed an SF Symbol plus a host path string for attachments and nothing at all for agent-referenced files.
This adds a Mac→iOS artifact plane and viewer:
Mac host. Four new session-scoped RPCs under the existing chat dispatch:
mobile.chat.artifact.stat,.fetch,.thumbnail,.list, advertised as capabilitychat.artifact.v1. A request is allowed only when the path is actually referenced by that session's transcript (attachment hostPaths, fileEdit filePaths, tool-usereferenced_paths), compared after symlink resolution and standardization; out-of-scope requests get a uniformforbiddenbefore any stat so existence never leaks. Referenced directories grant one-level access (covers agent-mentioned screenshot folders) andlistworks only on directories that are themselves referenced. Fetch is chunked at 3 MiB raw per frame (base64 stays under the 8 MiBMobileSyncFrameCodeccap, pinned by test); thumbnails are ImageIO-downscaled JPEGs. Scope construction, transcript parsing, canonicalization, file reads, and thumbnail decode all run off the main actor via the newAgentChatArtifactIndexactor with a transcript-keyed cache. Auth posture is identical to the othermobile.chat.*verbs (Stack same-account gate); the auth switches, frame codec, and listener are untouched.Parsers (shared).
ClaudeTranscriptParser/CodexTranscriptParsernow emit.attachmentmessages for cmux clipboard-materialized image paths in user prompts, so attachments survive transcript reload instead of existing only as the optimistic local echo, and populate an optional fail-openreferenced_pathsfield on tool-use messages.iOS. Attachment bubbles render real thumbnails (cached, no refetch on cell reuse) and open a full-screen viewer with explicit states: image, monospaced text, binary/unsupported, too large (64 MB viewer cap checked via stat before fetching), file missing on Mac, Mac unreachable with retry. Tool-use and file-edit detail sheets gain "View file" and "Browse folder". Everything is gated on
chat.artifact.v1: against an older Mac the UI renders exactly as before and issues zero artifact RPCs. All new strings localized EN+JA.Tests: 191 CmuxAgentChat package tests (scope traversal/symlink/one-level/uniform-forbidden, chunk-cap pin, parser attachment extraction, referenced_paths wire round-trip incl. legacy decode) and 39 CmuxAgentChatUI tests (loader cache) pass.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Introduces Mac filesystem reads over mobile RPC with transcript/terminal path scoping; mistakes in
ChatArtifactScopeor handlers could leak or over-expose host files, though the diff adds explicit forbidden-before-stat rules and broad tests.Overview
Adds an end-to-end Mac-hosted artifact path so iOS can preview files referenced in agent transcripts and paths visible in terminal output, instead of showing only placeholder attachment UI and host path strings.
Shared (
CmuxAgentChat) introduces artifact models, transfer limits, transcript-derived indexing (provenance, paging, search),ChatArtifactScope/TerminalArtifactScopepath authorization,ArtifactByteReader, and terminal path detection plus soft-wrap tap hit testing.ChatEventSourcegains optionalartifactStat/fetch/thumbnail/listhooks (default unsupported). Claude/Codex parsers now split leading clipboard image paths into attachment messages and populatereferenced_pathson tool uses (including apply_patch paths).iOS (
CmuxAgentChatUI) wires aChatArtifactLoader(memory + LRU disk thumbnail cache), viewer and folder sheets, thumbnails on attachment bubbles, and “Referenced Files” actions on tool/file detail sheets—all gated onsupportsArtifacts. Mobile RPC extends terminal ticket coverage formobile.terminal.artifact.*scan/stat/fetch/thumbnail..gitignoreis updated so Swift sources under packageArtifacts/folders are not ignored by the localartifacts/rule.Reviewed by Cursor Bugbot for commit 6f70169. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Lets iOS preview Mac-hosted files referenced in agent chats and the terminal, with real thumbnails, a full‑screen viewer, and a session‑wide gallery with sections, stable paging, and search; the floating chip now shows a live session‑wide file count.
New Features
mobile.chat.artifact.{stat,fetch,thumbnail,list,gallery}(capabilitieschat.artifact.v1,chat.artifact.gallery.v1) and terminalmobile.terminal.artifact.{scan,stat,fetch,thumbnail}(terminal.artifact.v1).patch_apply_endevents populate Created/moved paths.CmuxAgentChattests;.gitignoreunignoresArtifacts/sources.Security and Performance
/tmp↔/private/tmp,/var,/etcnormalize.chat.artifact.v1,chat.artifact.gallery.v1,terminal.artifact.v1) ensure graceful fallback../filetokens for in‑view taps; invalid‑params errors use product wording and are localized (EN/JA).Written for commit bffef35. Summary will update on new commits.
Summary by CodeRabbit