Repository navigation
Combine iOS workspace close, terminal themes, and text capture fixes - #6743
Closed
lawrencecchen wants to merge 35 commits into
Closed
lawrencecchen wants to merge 35 commits into
lawrencecchen wants to merge 35 commits into
Conversation
Remove the workspace row from the local list immediately on close, send the close RPC, and reconcile against the authoritative list. While the close is unconfirmed the id is filtered out of every applied snapshot so a stale fetch cannot resurrect the row and a real confirmation cannot double-remove it. A failed close (offline or rejected, e.g. the last workspace) restores the row before the re-sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The iOS terminal hardcoded the Monokai palette in two places in GhosttyRuntime (the per-process config and the on-disk default config), plus the SwiftUI letterbox chrome and the input accessory bar. Introduce a shared TerminalTheme value type in CMUXMobileCore (bg, fg, cursor, cursor-text, selection, and the 16 ANSI palette colors as hex strings, Codable/Equatable/Sendable) with a built-in Monokai default and a ghostty config directive generator. TerminalThemeStore holds the active theme process-wide; GhosttyRuntime builds its config from it and exposes setTheme(_:), and the terminal chrome reads the same store so it blends with any theme. Invalid or incomplete themes fall back to Monokai. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two causes made the sheet show "No terminal text available" with visible output: 1. The registry pick for the capture is visibility-scoped (window/hidden/ alpha). It ran from the sheet's own .task, after presentation, when the presenter's window/alpha can transiently drop, so the one live surface could be filtered out and the read returned nil. Resolve and FIFO-enqueue the read now happen in openTextSheetFromMenu, at tap time, while the surface is still fully on screen; the sheet just awaits that capture. 2. surfaceText returns a non-nil empty string for a zero-byte range, so screen ?? viewport never fell back when SCREEN read empty-but-ok. The new resolvedText treats a nil OR empty SCREEN as "try VIEWPORT". Extract both decisions into CopyableTerminalTextSelection (pure, no UIKit/ GhosttyKit) and unit-test the id-scoped pick, the no-surface/off-screen exclusions, and the empty-SCREEN-falls-back-to-viewport rule. Add a concise DEBUG-only anchormux diagnostic recording the requested id, every candidate's state, and the SCREEN/VIEWPORT read outcomes so the path is self-explaining in the dogfood log. Co-Authored-By: Claude Opus 4.8 <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:
📝 WalkthroughWalkthroughThis PR adds a portable ChangesTerminal theming system
Workspace close handling
View as Text capture flow
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceDetailView
participant GhosttySurfaceRegistry
participant CopyableTerminalTextSelection
participant TerminalTextSheetView
participant TerminalTextCaptureRaceState
User->>WorkspaceDetailView: tap "View as Text"
WorkspaceDetailView->>GhosttySurfaceRegistry: copyableTerminalTextCapture(surfaceID:)
GhosttySurfaceRegistry->>CopyableTerminalTextSelection: chosenIndex(from: candidates, for: surfaceID)
CopyableTerminalTextSelection-->>GhosttySurfaceRegistry: chosen index
GhosttySurfaceRegistry->>GhosttySurfaceRegistry: read SCREEN + VIEWPORT on outputQueue
GhosttySurfaceRegistry->>CopyableTerminalTextSelection: resolvedText(screen:viewport:)
GhosttySurfaceRegistry-->>WorkspaceDetailView: Task<String?, Never>
WorkspaceDetailView->>WorkspaceDetailView: store textSheetCapture, present sheet
TerminalTextSheetView->>TerminalTextCaptureRaceState: install continuation, set waiter + timer
alt capture finishes before timeout
TerminalTextCaptureRaceState-->>TerminalTextSheetView: finish(timedOut: false, text: result)
TerminalTextSheetView->>User: show terminal text snapshot
else timeout fires first
TerminalTextCaptureRaceState-->>TerminalTextSheetView: finish(timedOut: true, text: nil)
TerminalTextSheetView->>User: show timeout errorMessage
end
sequenceDiagram
participant iOSMobileShellComposite
participant GhosttyRuntime
participant TerminalPalette
participant MacRPC
iOSMobileShellComposite->>GhosttyRuntime: setTheme(_:)
GhosttyRuntime->>GhosttyRuntime: init(theme: configuredTheme)
GhosttyRuntime-->>TerminalPalette: currentTheme
TerminalPalette->>TerminalPalette: derive colors from theme
iOSMobileShellComposite->>MacRPC: workspace.close
alt closed: true
MacRPC-->>iOSMobileShellComposite: confirmed
iOSMobileShellComposite->>iOSMobileShellComposite: retire pending close snapshot
else blocked or rejected
MacRPC-->>iOSMobileShellComposite: error / unconfirmed
iOSMobileShellComposite->>iOSMobileShellComposite: rollbackOptimisticWorkspaceClose(id:)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (17 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 bundles three independent iOS features: optimistic workspace close (instant row removal with rollback on RPC failure or
Confidence Score: 5/5Safe to merge; the three features each have dedicated unit tests and the core state machines (optimistic close rollback, capture race, theme signature delta) are straightforward to reason about. All three feature areas are well-contained with explicit rollback paths and error UI. The workspace close reconciliation correctly filters pending ids from derived lists and restores selection on failure. The capture timeout actor handles every interleaving correctly. The only findings are a duplicated luminance formula between two modules that could silently diverge if adjusted, and a public top-level notification user-info key constant that belongs on a namespaced type — neither affects runtime correctness. No files require special attention. GhosttyRuntime.swift carries a pre-existing static-var isolation debt noted in a prior review cycle, unchanged by this PR. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User (iOS)
participant WDV as WorkspaceDetailView
participant MSC as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Mac as macOS ControlSocket
Note over U,Mac: Optimistic Workspace Close
U->>WDV: Tap Close Workspace
WDV->>MSC: closeWorkspace(id:)
MSC->>MSC: applyOptimisticWorkspaceClose(id:)
Note right of MSC: Row removed, snapshot saved, filtered from derived lists
MSC->>RPC: sendRequest(workspace.close)
RPC->>Mac: RPC over socket
alt "Success closed=true"
Mac-->>RPC: ok closed true
RPC-->>MSC: response data
MSC->>MSC: workspaceCloseResponseConfirmsClosed true
Note right of MSC: Optimistic close confirmed
else Blocked last workspace
Mac-->>RPC: err code protected reason pinned
RPC-->>MSC: throws rpcError
MSC->>MSC: rollbackOptimisticWorkspaceClose(id:)
Note right of MSC: Row restored, selection restored
else Legacy Mac no closed field
Mac-->>RPC: ok window_id
RPC-->>MSC: response data
MSC->>MSC: workspaceCloseResponseConfirmsClosed true default
end
MSC->>MSC: refreshAfterWorkspaceMutation
Note over U,Mac: Terminal Theme Propagation
Mac->>Mac: ghosttyDefaultBackgroundDidChange
Mac->>Mac: MobileTerminalRenderObserver.refreshCachedTerminalTheme
Mac->>Mac: enqueueTerminalThemeRefresh full frame with scrollback
Mac-->>MSC: MobileTerminalRenderGridFrame terminalTheme set
MSC->>MSC: deliverTerminalRenderGrid cache theme per surfaceID
MSC->>WDV: activeTerminalTheme changed
WDV->>WDV: applyTerminalTheme
WDV->>WDV: terminalPalette.setTheme SwiftUI chrome
WDV->>WDV: GhosttyRuntime.setTheme libghostty config UIKit chrome
%%{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 U as User (iOS)
participant WDV as WorkspaceDetailView
participant MSC as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Mac as macOS ControlSocket
Note over U,Mac: Optimistic Workspace Close
U->>WDV: Tap Close Workspace
WDV->>MSC: closeWorkspace(id:)
MSC->>MSC: applyOptimisticWorkspaceClose(id:)
Note right of MSC: Row removed, snapshot saved, filtered from derived lists
MSC->>RPC: sendRequest(workspace.close)
RPC->>Mac: RPC over socket
alt "Success closed=true"
Mac-->>RPC: ok closed true
RPC-->>MSC: response data
MSC->>MSC: workspaceCloseResponseConfirmsClosed true
Note right of MSC: Optimistic close confirmed
else Blocked last workspace
Mac-->>RPC: err code protected reason pinned
RPC-->>MSC: throws rpcError
MSC->>MSC: rollbackOptimisticWorkspaceClose(id:)
Note right of MSC: Row restored, selection restored
else Legacy Mac no closed field
Mac-->>RPC: ok window_id
RPC-->>MSC: response data
MSC->>MSC: workspaceCloseResponseConfirmsClosed true default
end
MSC->>MSC: refreshAfterWorkspaceMutation
Note over U,Mac: Terminal Theme Propagation
Mac->>Mac: ghosttyDefaultBackgroundDidChange
Mac->>Mac: MobileTerminalRenderObserver.refreshCachedTerminalTheme
Mac->>Mac: enqueueTerminalThemeRefresh full frame with scrollback
Mac-->>MSC: MobileTerminalRenderGridFrame terminalTheme set
MSC->>MSC: deliverTerminalRenderGrid cache theme per surfaceID
MSC->>WDV: activeTerminalTheme changed
WDV->>WDV: applyTerminalTheme
WDV->>WDV: terminalPalette.setTheme SwiftUI chrome
WDV->>WDV: GhosttyRuntime.setTheme libghostty config UIKit chrome
Reviews (11): Last reviewed commit: "Remove remaining iOS Monokai chrome fall..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// live terminal under any theme instead of flashing a hardcoded color. They | ||
| /// fall back to Monokai when no theme has been supplied. | ||
| @MainActor | ||
| let terminalPalette = TerminalPalette() |
Contributor
There was a problem hiding this comment.
terminalPalette is a file-scope let with internal visibility, making it ambient API across every view in CmuxMobileShellUI. The old TerminalPalette was a static-only struct (equally ambient), so this is a shape change rather than a net worsening, and because GhosttyRuntime.setTheme is gated to pre-init the value never changes after startup — so there is no staleness risk in practice. The idiomatic SwiftUI shape here would be an EnvironmentKey/EnvironmentValues extension injected at the app seam so views receive the palette through the environment rather than reaching for it as a global name.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift (1)
282-339: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSnapshot
terminalPalette.foregroundonce per render.Each read goes back through
TerminalTheme.rgbComponentsinTerminalPalette, and this view now hits the same foreground color several times in the composer/typing path. Capture a locallet foreground = terminalPalette.foreground(and derived opacities if helpful) so one body pass does not redo the same parse work.Also applies to: 388-388, 793-820
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift` around lines 282 - 339, The composer view is repeatedly reading terminalPalette.foreground during a single render, which re-runs the same color parsing work in TerminalPalette.rgbComponents. In TerminalComposerView, snapshot the foreground color once per body pass with a local value (and reuse derived opacity variants if needed) so the toolbar, field, and send button all share the cached result instead of recomputing it on every access.
🤖 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/MobileShellComposite.swift`:
- Around line 3407-3416: The filtering in MobileShellComposite.swift is doing a
nested scan: derived.removeAll currently checks every workspace against all
values in optimisticallyClosedWorkspaces via pendingOptimisticClose, which makes
recomputation scale poorly. Refactor this block to precompute an O(1)-lookup
structure keyed by the relevant Mac/remote IDs from
optimisticallyClosedWorkspaces, then use that membership check inside
derived.removeAll instead of iterating the pending snapshots for each workspace.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalTextSheetView.swift`:
- Line 19: Remove the fixed timeout-based capture fallback in
TerminalTextSheetView and drive the sheet state from the capture pipeline’s real
completion or failure signal instead. Update the capture path around
captureTimeout and the related logic in TerminalTextSheetView so it waits for
authoritative success/failure instead of racing a 3-second deadline, and keep
the UI update flow aligned with the capture task outcome rather than timing out.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 453-454: The sheet dismissal flow in WorkspaceDetailView currently
clears textSheetCapture without stopping the underlying capture task, which can
leave the stalled capture running and allow duplicates on reopen. Update the
.sheet onDismiss handling and the surrounding textSheetCapture lifecycle so the
active capture is explicitly cancelled/stopped before the reference is nilled
out, using the existing textSheetCapture and isTextSheetPresented state in
WorkspaceDetailView.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntimeConfigLoader.swift`:
- Line 31: Make XDG_CONFIG_HOME authoritative in GhosttyRuntimeConfigLoader so
the loader and ensureDefaultiOSConfig use the same config root. Right now
setenv("XDG_CONFIG_HOME", appSupport.path, 0) can preserve an inherited value,
which may cause ghostty_config_load_default_files to read a different location
than where defaults were written. Update the configuration setup to always use
the resolved Application Support path for both operations, or change the setenv
call to overwrite the existing value, and keep the logic centered around
GhosttyRuntimeConfigLoader and ensureDefaultiOSConfig.
In
`@Packages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/TerminalThemeTests.swift`:
- Around line 69-77: The TerminalTheme.rgbComponents parser coverage is missing
an explicit rejection for 8-digit hex values with alpha, so add tests in
rgbComponentsParseHex that verify both `#RRGGBBAA` and RRGGBBAA return nil. Keep
the existing success cases for `#RRGGBB/RRGGBB` and extend the invalid-input
assertions alongside the current short, non-hex, and nil checks to lock in the
exactly-six-hex-digits contract.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift`:
- Around line 282-339: The composer view is repeatedly reading
terminalPalette.foreground during a single render, which re-runs the same color
parsing work in TerminalPalette.rgbComponents. In TerminalComposerView, snapshot
the foreground color once per body pass with a local value (and reuse derived
opacity variants if needed) so the toolbar, field, and send button all share the
cached result instead of recomputing it on every access.
🪄 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: 30be074d-978f-4c67-b716-adfd3a828b40
📒 Files selected for processing (27)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TerminalTheme.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/TerminalThemeHexParser.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/TerminalThemeTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/OptimisticWorkspaceCloseTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalPalette.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalTextCaptureRaceState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalTextSheetView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntimeConfigLoader.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/CopyableTerminalTextCandidate.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/CopyableTerminalTextSelection.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/CopyableTerminalTextSelectionTests.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+Workspace.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceCloseResolution.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceStrings.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftSources/TerminalController+ControlWorkspaceContext.swiftios/cmux/Resources/Localizable.xcstrings
| /// capped to `TerminalTextSnapshot.defaultLineBudget` lines, with a banner | ||
| /// when older lines were dropped. | ||
| struct TerminalTextSheetView: View { | ||
| private static let captureTimeout: Duration = .seconds(3) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove the fixed timeout from the capture path.
This hard-codes a 3-second race against the capture task, so slow-but-correct reads will now surface a false error instead of eventually showing text. Repo policy treats new timing-based repair paths in shipped Swift as failures, especially when they paper over lifecycle/rendering races like this one. Please drive the sheet from an authoritative completion/failure signal from the capture pipeline instead of a sleep-based deadline. As per coding guidelines, "Flag Thread.sleep, usleep, sleep, Task.sleep, DispatchQueue.asyncAfter, timers, or polling loops in shipped app/runtime Swift code as failures by default" and "Report a failure when a diff introduces or materially expands timing or blocking repair paths ... used to paper over lifecycle, focus, rendering ... races in Swift."
Also applies to: 169-185
🤖 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/TerminalTextSheetView.swift`
at line 19, Remove the fixed timeout-based capture fallback in
TerminalTextSheetView and drive the sheet state from the capture pipeline’s real
completion or failure signal instead. Update the capture path around
captureTimeout and the related logic in TerminalTextSheetView so it waits for
authoritative success/failure instead of racing a 3-second deadline, and keep
the UI update flow aligned with the capture task outcome rather than timing out.
Source: Coding guidelines
Comment on lines
+453
to
+454
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) { | ||
| TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel textSheetCapture before clearing it.
Dropping the reference does not stop the started Task. In the stalled-capture path this PR is handling, dismissing the sheet can leave the old capture running indefinitely, and the next open starts another one.
Suggested fix
- .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) {
+ .sheet(isPresented: $isTextSheetPresented, onDismiss: {
+ textSheetCapture?.cancel()
+ textSheetCapture = nil
+ }) {
TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture)
}📝 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
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) { | |
| TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture) | |
| .sheet(isPresented: $isTextSheetPresented, onDismiss: { | |
| textSheetCapture?.cancel() | |
| textSheetCapture = nil | |
| }) { | |
| TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture) |
🤖 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/WorkspaceDetailView.swift`
around lines 453 - 454, The sheet dismissal flow in WorkspaceDetailView
currently clears textSheetCapture without stopping the underlying capture task,
which can leave the stalled capture running and allow duplicates on reopen.
Update the .sheet onDismiss handling and the surrounding textSheetCapture
lifecycle so the active capture is explicitly cancelled/stopped before the
reference is nilled out, using the existing textSheetCapture and
isTextSheetPresented state in WorkspaceDetailView.
| private func setupiOSConfigEnvironment() { | ||
| let appSupport = fileManager.urls(for: .applicationSupportDirectory, in: .userDomainMask).first | ||
| ?? fileManager.temporaryDirectory | ||
| setenv("XDG_CONFIG_HOME", appSupport.path, 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make XDG_CONFIG_HOME authoritative here.
setenv("XDG_CONFIG_HOME", appSupport.path, 0) preserves any inherited value, but ensureDefaultiOSConfig always writes under Application Support. That gives ghostty_config_load_default_files and the default-config writer different roots, so this loader can silently read the wrong config and miss the theme/defaults it just prepared. Use one resolved path for both operations, or set overwrite to 1. As per path instructions, "Ensure there is a single authoritative source for each correctness fact" and avoid non-authoritative fallback branches.
🤖 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/GhosttyRuntimeConfigLoader.swift`
at line 31, Make XDG_CONFIG_HOME authoritative in GhosttyRuntimeConfigLoader so
the loader and ensureDefaultiOSConfig use the same config root. Right now
setenv("XDG_CONFIG_HOME", appSupport.path, 0) can preserve an inherited value,
which may cause ghostty_config_load_default_files to read a different location
than where defaults were written. Update the configuration setup to always use
the resolved Application Support path for both operations, or change the setenv
call to overwrite the existing value, and keep the logic centered around
GhosttyRuntimeConfigLoader and ensureDefaultiOSConfig.
Source: Path instructions
Comment on lines
+69
to
+77
| @Test func rgbComponentsParseHex() { | ||
| #expect(TerminalTheme.rgbComponents("#ff8000")! == (255, 128, 0)) | ||
| #expect(TerminalTheme.rgbComponents("ff8000")! == (255, 128, 0)) | ||
| #expect(TerminalTheme.rgbComponents("#fff") == nil) | ||
| #expect(TerminalTheme.rgbComponents("zzzzzz") == nil) | ||
| #expect(TerminalTheme.rgbComponents("#+fffff") == nil) | ||
| #expect(TerminalTheme.rgbComponents("-00001") == nil) | ||
| #expect(TerminalTheme.rgbComponents(nil) == nil) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add the no-alpha regression to the parser coverage.
The new validation contract is “exactly six hex digits”, but this test only locks in short and non-hex failures. Please add an explicit #RRGGBBAA / RRGGBBAA rejection so a future parser change cannot silently re-allow alpha. Based on learnings, this repo only accepts and uses 6-digit hex colors in the form #RRGGBB with no alpha.
Proposed test addition
`@Test` func rgbComponentsParseHex() {
`#expect`(TerminalTheme.rgbComponents("`#ff8000`")! == (255, 128, 0))
`#expect`(TerminalTheme.rgbComponents("ff8000")! == (255, 128, 0))
+ `#expect`(TerminalTheme.rgbComponents("`#11223344`") == nil)
+ `#expect`(TerminalTheme.rgbComponents("11223344") == nil)
`#expect`(TerminalTheme.rgbComponents("`#fff`") == nil)
`#expect`(TerminalTheme.rgbComponents("zzzzzz") == nil)
`#expect`(TerminalTheme.rgbComponents("#+fffff") == nil)📝 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
| @Test func rgbComponentsParseHex() { | |
| #expect(TerminalTheme.rgbComponents("#ff8000")! == (255, 128, 0)) | |
| #expect(TerminalTheme.rgbComponents("ff8000")! == (255, 128, 0)) | |
| #expect(TerminalTheme.rgbComponents("#fff") == nil) | |
| #expect(TerminalTheme.rgbComponents("zzzzzz") == nil) | |
| #expect(TerminalTheme.rgbComponents("#+fffff") == nil) | |
| #expect(TerminalTheme.rgbComponents("-00001") == nil) | |
| #expect(TerminalTheme.rgbComponents(nil) == nil) | |
| } | |
| `@Test` func rgbComponentsParseHex() { | |
| `#expect`(TerminalTheme.rgbComponents("`#ff8000`")! == (255, 128, 0)) | |
| `#expect`(TerminalTheme.rgbComponents("ff8000")! == (255, 128, 0)) | |
| `#expect`(TerminalTheme.rgbComponents("`#11223344`") == nil) | |
| `#expect`(TerminalTheme.rgbComponents("11223344") == nil) | |
| `#expect`(TerminalTheme.rgbComponents("`#fff`") == nil) | |
| `#expect`(TerminalTheme.rgbComponents("zzzzzz") == nil) | |
| `#expect`(TerminalTheme.rgbComponents("#+fffff") == nil) | |
| `#expect`(TerminalTheme.rgbComponents("-00001") == nil) | |
| `#expect`(TerminalTheme.rgbComponents(nil) == nil) | |
| } |
🤖 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/CMUXMobileCore/Tests/CMUXMobileCoreTests/TerminalThemeTests.swift`
around lines 69 - 77, The TerminalTheme.rgbComponents parser coverage is missing
an explicit rejection for 8-digit hex values with alpha, so add tests in
rgbComponentsParseHex that verify both `#RRGGBBAA` and RRGGBBAA return nil. Keep
the existing success cases for `#RRGGBB/RRGGBB` and extend the invalid-input
assertions alongside the current short, non-hex, and nil checks to lock in the
exactly-six-hex-digits contract.
Source: Learnings
lawrencecchen
force-pushed
the
feat-ios-combined-6663-6664-6736
branch
from
June 25, 2026 00:12
efab424 to
452344c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/MobileShellComposite`+WorkspaceActions.swift:
- Around line 179-181: The `workspaceCloseResponseConfirmsClosed(_:)` check is
too permissive because it defaults to true when `closed` is missing, invalid, or
legacy-shaped. Update this method to fail closed by returning true only when the
parsed JSON in `object["closed"]` is explicitly a Bool true, and return false
for anything else so `WorkspaceActions` does not treat uncertain responses as
confirmed closes.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Line 40: The scene-specific terminal palette is still being synchronized with
the process-global runtime, causing theme changes in one scene to overwrite
other scenes. Update WorkspaceDetailView’s terminal theme flow so
`terminalPalette` stays fully local and `applyTerminalTheme` no longer pushes
every change into `GhosttyRuntime.setTheme`; if runtime-backed theming is still
needed, scope it per surface instead of using view lifecycle callbacks. Also
ensure `TerminalPalette` initialization does not seed scene state from
`GhosttyRuntime.currentTheme`, so each scene starts from its own local palette
state consistently.
In `@Sources/Mobile/MobileTerminalRenderObserver.swift`:
- Around line 28-32: The theme cache in MobileTerminalRenderObserver is
incorrectly shared across all surfaces, while render-grid emission is
surface-specific. Refactor the cachedTerminalTheme and
cachedTerminalThemeSignature usage so theme/palette resolution is scoped per
surfaceID inside the render state path that emits the grid, likely in the emit
logic around renderGridStatesBySurfaceID rather than as global observer state.
Update the code paths that build the emitted state to read from the target
surface’s terminal config each time, and ensure any refresh detection compares
against the per-surface cached signature instead of a single observer-wide
cache.
In `@Sources/TerminalController`+MobileScrollPrefetch.swift:
- Around line 24-29: The mobile scroll prefetch/render-grid snapshot is still
pulling theme data from app-wide defaults instead of the target surface’s
configuration, which can flatten surface-specific overrides. Update the snapshot
path in TerminalController+MobileScrollPrefetch, including the logic that builds
the frame and the helper that constructs the theme snapshot, so it reads the
terminalPanel’s surface/config-scoped theme rather than GhosttyApp defaults.
Keep GhosttyApp.reloadSurfaceConfiguration(_:soft:source:) surface-scoped and
only use app-level config in app-level helpers, and make the same fix in the
related snapshot code around the MobileScrollPrefetch helper.
🪄 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: 28d60743-5bfa-43dd-9452-d20f6a8e3daa
📒 Files selected for processing (23)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGrid.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGridReplay.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileTerminalRenderGridTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/OptimisticWorkspaceCloseTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalPalette.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/MobileTerminalThemeNotification.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/CopyableTerminalTextSelection.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/CopyableTerminalTextSelectionTests.swiftSources/GhosttyTerminalView.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController+MobileScrollPrefetch.swift
Comment on lines
+179
to
+181
| func workspaceCloseResponseConfirmsClosed(_ data: Data) -> Bool { | ||
| let object = (try? JSONSerialization.jsonObject(with: data)) as? [String: Any] | ||
| return object?["closed"] as? Bool ?? true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when closed is missing or malformed.
Line 181 treats empty, invalid, or legacy-shaped data as confirmed. If the Mac did not actually close the workspace, the pending close remains filtered after refresh and the row can stay hidden. Require explicit closed: true; only rollback-blocking responses should be unmistakable.
Proposed fix
func workspaceCloseResponseConfirmsClosed(_ data: Data) -> Bool {
let object = (try? JSONSerialization.jsonObject(with: data)) as? [String: Any]
- return object?["closed"] as? Bool ?? true
+ return object?["closed"] as? Bool == true
}As per path instructions, correctness-critical state should fail closed when the reliable signal is missing.
📝 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
| func workspaceCloseResponseConfirmsClosed(_ data: Data) -> Bool { | |
| let object = (try? JSONSerialization.jsonObject(with: data)) as? [String: Any] | |
| return object?["closed"] as? Bool ?? true | |
| func workspaceCloseResponseConfirmsClosed(_ data: Data) -> Bool { | |
| let object = (try? JSONSerialization.jsonObject(with: data)) as? [String: Any] | |
| return object?["closed"] as? Bool == true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+WorkspaceActions.swift
around lines 179 - 181, The `workspaceCloseResponseConfirmsClosed(_:)` check is
too permissive because it defaults to true when `closed` is missing, invalid, or
legacy-shaped. Update this method to fail closed by returning true only when the
parsed JSON in `object["closed"]` is explicitly a Bool true, and return false
for anything else so `WorkspaceActions` does not treat uncertain responses as
confirmed closes.
Source: Path instructions
Comment on lines
+28
to
+32
| private var hasPendingThemeRefresh = false | ||
| private var isEmitFlushScheduled = false | ||
| private var renderGridStatesBySurfaceID: [UUID: RenderGridState] = [:] | ||
| private var cachedTerminalTheme: TerminalTheme? | ||
| private var cachedTerminalThemeSignature: String? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Theme caching is global, but the emitted render-grid state is surface-scoped.
Lines 28-32 keep a single cached theme/signature for the whole observer, and Lines 230-277 reuse it for every emitted surface. If two surfaces can carry different terminal configs, a change on one surface will overwrite the cache used for the other surface and can send the wrong theme/palette to iOS while also forcing false full-frame refreshes.
As per path instructions, .github/review-bot-rules/reliability-single-source-of-truth.md requires correctness-critical state to come from one authoritative typed source, and based on learnings surface config reloads must remain scoped to the target surface rather than being promoted into shared app state.
Also applies to: 230-277
🤖 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/MobileTerminalRenderObserver.swift` around lines 28 - 32, The
theme cache in MobileTerminalRenderObserver is incorrectly shared across all
surfaces, while render-grid emission is surface-specific. Refactor the
cachedTerminalTheme and cachedTerminalThemeSignature usage so theme/palette
resolution is scoped per surfaceID inside the render state path that emits the
grid, likely in the emit logic around renderGridStatesBySurfaceID rather than as
global observer state. Update the code paths that build the emitted state to
read from the target surface’s terminal config each time, and ensure any refresh
detection compares against the per-surface cached signature instead of a single
observer-wide cache.
Sources: Path instructions, Learnings
Comment on lines
+24
to
+29
| var frame = terminalPanel.surface.mobileRenderGridFrame( | ||
| stateSeq: seq, | ||
| scrollbackLines: scrollbackLines | ||
| )?.frame | ||
| frame?.terminalTheme = TerminalTheme.currentMacTerminalThemeSnapshot() | ||
| return frame |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Surface snapshots are still sourced from app-global theme state.
Line 73 builds the snapshot from GhosttyApp.default*, and Lines 24-29 attach that same value to a specific terminalPanel frame. That means any surface-scoped Ghostty override gets flattened into the app default, so scroll-prefetch/render-grid payloads can advertise the wrong palette for terminals that do not share the global config. Please plumb the theme from the target surface/config snapshot instead of GhosttyApp’s app-wide defaults.
Based on learnings, GhosttyApp.reloadSurfaceConfiguration(_:soft:source:) must stay surface-scoped and app-level helpers should keep reading app-level config until a full reload path.
Also applies to: 71-84
🤖 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`+MobileScrollPrefetch.swift around lines 24 - 29,
The mobile scroll prefetch/render-grid snapshot is still pulling theme data from
app-wide defaults instead of the target surface’s configuration, which can
flatten surface-specific overrides. Update the snapshot path in
TerminalController+MobileScrollPrefetch, including the logic that builds the
frame and the helper that constructs the theme snapshot, so it reads the
terminalPanel’s surface/config-scoped theme rather than GhosttyApp defaults.
Keep GhosttyApp.reloadSurfaceConfiguration(_:soft:source:) surface-scoped and
only use app-level config in app-level helpers, and make the same fix in the
related snapshot code around the MobileScrollPrefetch helper.
Source: Learnings
lawrencecchen
force-pushed
the
feat-ios-combined-6663-6664-6736
branch
2 times, most recently
from
June 25, 2026 00:24
8a6f944 to
0f17f22
Compare
Comment on lines
+48
to
+79
| private static var configuredTheme: TerminalTheme = .monokai | ||
| private static var clipboardReader: @MainActor () -> String? = { UIPasteboard.general.string } | ||
| private static var clipboardWriter: @MainActor (String?) -> Void = { UIPasteboard.general.string = $0 } | ||
|
|
||
| /// Replaces the theme used when the runtime builds its config and the theme | ||
| /// the surrounding terminal chrome blends with. Pass `nil`, an invalid theme, | ||
| /// or an incomplete palette to fall back to Monokai. Calls after | ||
| /// ``shared()`` update the live theme value for SwiftUI/UIKit chrome; the | ||
| /// terminal grid itself is repainted by the Mac render-grid frame's VT | ||
| /// snapshot bytes. | ||
| public static func setTheme(_ theme: TerminalTheme?) { | ||
| let resolvedTheme = theme?.validatedOrDefault() ?? .monokai | ||
| guard currentTheme != resolvedTheme else { return } | ||
| configuredTheme = resolvedTheme | ||
| if case .success(let runtime) = sharedResult { | ||
| runtime.theme = resolvedTheme | ||
| } | ||
| NotificationCenter.default.post( | ||
| name: .cmuxMobileTerminalThemeDidChange, | ||
| object: nil, | ||
| userInfo: [cmuxMobileTerminalThemeNotificationThemeKey: resolvedTheme] | ||
| ) | ||
| } | ||
|
|
||
| /// The effective theme for the live runtime, or the theme that will be used | ||
| /// when the runtime is first initialized. | ||
| public static var currentTheme: TerminalTheme { | ||
| if case .success(let runtime) = sharedResult { | ||
| return runtime.theme | ||
| } | ||
| return configuredTheme | ||
| } |
Contributor
There was a problem hiding this comment.
configuredTheme is a new private static var on a non-@MainActor class, read and written by the unannotated setTheme and currentTheme static members. In Swift 6 strict concurrency mode all three accesses are data races. The existing backendInitialized/sharedResult statics carry the same debt, but configuredTheme is new. Since every real call site is @MainActor (WorkspaceDetailView.applyTerminalTheme, TerminalController+MobileScrollPrefetch), annotating setTheme and currentTheme with @MainActor (or marking configuredTheme as nonisolated(unsafe) with a comment matching the existing app/config precedent) would close the gap without changing observable behavior.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift (1)
414-416: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPass palette values into
AttachmentChipinstead of reading the observable environment in the row.
AttachmentChipis below aForEach, so@Environment(TerminalPalette.self)puts an observable store reference inside the row subtree. Snapshot the needed colors in the parent and pass them as values.Proposed refactor
- AttachmentChip(thumbnail: thumbnailCache.image(for: attachment.id)) { + AttachmentChip( + thumbnail: thumbnailCache.image(for: attachment.id), + foreground: terminalPalette.foreground + ) { store.removePendingAttachment(id: attachment.id, forTerminalID: terminalID) thumbnailCache.remove(attachment.id) requestHeightRemeasure() }private struct AttachmentChip: View { let thumbnail: UIImage? + let foreground: Color let onRemove: () -> Void - `@Environment`(TerminalPalette.self) private var terminalPalette private let side: CGFloat = 56- .strokeBorder(terminalPalette.foreground.opacity(0.15), lineWidth: 1) + .strokeBorder(foreground.opacity(0.15), lineWidth: 1)- .fill(terminalPalette.foreground.opacity(0.12)) + .fill(foreground.opacity(0.12)) .overlay( Image(systemName: "photo") - .foregroundStyle(terminalPalette.foreground.opacity(0.5)) + .foregroundStyle(foreground.opacity(0.5)) )As per coding guidelines, “Do not use store references (
@ObservedObject,@EnvironmentObject,@StateObject,@Bindable, or plain store properties) inLazyVStack,LazyHStack,List, orForEachrow subtrees; use immutable snapshots plus action closures instead.”Also applies to: 781-824
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift` around lines 414 - 416, Snapshot the palette values in the parent view and pass them into AttachmentChip as plain values instead of letting the row subtree read `@Environment`(TerminalPalette.self). Update the ForEach in TerminalComposerView so each attachment row receives immutable color values, and refactor AttachmentChip to accept those palette parameters while keeping the removePendingAttachment action closure unchanged. Also apply the same snapshot-and-pass approach anywhere else in the affected attachment/row subtree range (including the later referenced block) to avoid store references inside ForEach rows.Source: Coding guidelines
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift (1)
14-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
frame.surfaceIDas the single authoritative key here.This path now keys
terminalThemesBySurfaceID, selected-theme updates, and the downstream delivery off the method parameter even though the frame already carries its ownsurfaceID. If those two values ever diverge, the queued render grid andactiveTerminalThemecan be applied to different surfaces. Useframe.surfaceIDthroughout (or assert equality once, then reuse that value).Suggested change
func deliverTerminalRenderGrid(_ frame: MobileTerminalRenderGridFrame, surfaceID: String) { - if let theme = frame.terminalTheme?.validatedOrDefault() { - terminalThemesBySurfaceID[surfaceID] = theme - if selectedTerminalID?.rawValue == surfaceID, activeTerminalTheme != theme { + assert(frame.surfaceID == surfaceID) + let authoritativeSurfaceID = frame.surfaceID + if let theme = frame.terminalTheme?.validatedOrDefault() { + terminalThemesBySurfaceID[authoritativeSurfaceID] = theme + if selectedTerminalID?.rawValue == authoritativeSurfaceID, activeTerminalTheme != theme { activeTerminalTheme = theme } } deliverTerminalOutput( TerminalOutputDelivery( renderGrid: frame, replaceable: frame.isReplaceableViewportPatchForMobileDelivery ), - surfaceID: surfaceID + surfaceID: authoritativeSurfaceID ) }As per path instructions,
.github/review-bot-rules/reliability-single-source-of-truth.mdsays correctness-critical identity must have a single authoritative source and forbids disagreeing sources of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift around lines 14 - 27, Use a single authoritative surface identifier in deliverTerminalRenderGrid(_:surfaceID:) by switching all theme storage, selected-terminal comparison, and deliverTerminalOutput delivery to frame.surfaceID. If the method parameter is still needed, assert it matches frame.surfaceID once and then reuse the frame value everywhere in MobileShellComposite+TerminalOutputDelivery to avoid split state between terminalThemesBySurfaceID, activeTerminalTheme, and the downstream render-grid delivery.Source: Path instructions
♻️ Duplicate comments (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (2)
41-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the terminal theme source of truth scene-local.
Line 41 seeds
terminalPalettefromGhosttyRuntime.currentTheme, and Lines 434-438 still mirror each scene’s theme back into the process-global runtime. In a multi-window flow, the last scene to appear wins, so another scene can render its initial chrome/background with the wrong palette until it reapplies its own theme. Seed the palette from the workspace/store theme and stop usingGhosttyRuntimeas shared scene state. As per path instructions, “require a single authoritative source … and forbid … more than one disagreeing source of truth.”Also applies to: 433-439
🤖 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/WorkspaceDetailView.swift` at line 41, Keep the terminal theme state scene-local and remove the shared runtime source of truth. In WorkspaceDetailView, initialize and drive terminalPalette from the workspace/store theme instead of GhosttyRuntime.currentTheme, and update the scene-specific theme path so each window uses its own palette consistently. In the code around WorkspaceDetailView and the theme sync logic near the scene update methods, stop writing each scene’s theme back into GhosttyRuntime and ensure any theme changes flow only through the local view/store state.Source: Path instructions
464-465: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the in-flight text capture on dismiss.
Clearing the
Taskreference does not stop the capture. If the waiter stalls, dismissing the sheet leaves the old read running indefinitely and reopening can start a second capture against the same surface.Suggested fix
- .sheet(isPresented: $isTextSheetPresented, onDismiss: { textSheetCapture = nil }) { + .sheet(isPresented: $isTextSheetPresented, onDismiss: { + textSheetCapture?.cancel() + textSheetCapture = nil + }) { TerminalTextSheetView(surfaceID: textSheetSurfaceID, capture: textSheetCapture) }🤖 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/WorkspaceDetailView.swift` around lines 464 - 465, The sheet dismiss path in WorkspaceDetailView is only clearing the Task reference, so the in-flight text capture keeps running. Update the .sheet onDismiss handler to actively cancel the existing capture task before niling state, using the textSheetCapture reference (and any capture-start/stop helpers involved) so dismissing the sheet stops the waiter and prevents a second capture from starting on reopen.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 14-27: Use a single authoritative surface identifier in
deliverTerminalRenderGrid(_:surfaceID:) by switching all theme storage,
selected-terminal comparison, and deliverTerminalOutput delivery to
frame.surfaceID. If the method parameter is still needed, assert it matches
frame.surfaceID once and then reuse the frame value everywhere in
MobileShellComposite+TerminalOutputDelivery to avoid split state between
terminalThemesBySurfaceID, activeTerminalTheme, and the downstream render-grid
delivery.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swift`:
- Around line 414-416: Snapshot the palette values in the parent view and pass
them into AttachmentChip as plain values instead of letting the row subtree read
`@Environment`(TerminalPalette.self). Update the ForEach in TerminalComposerView
so each attachment row receives immutable color values, and refactor
AttachmentChip to accept those palette parameters while keeping the
removePendingAttachment action closure unchanged. Also apply the same
snapshot-and-pass approach anywhere else in the affected attachment/row subtree
range (including the later referenced block) to avoid store references inside
ForEach rows.
---
Duplicate comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Line 41: Keep the terminal theme state scene-local and remove the shared
runtime source of truth. In WorkspaceDetailView, initialize and drive
terminalPalette from the workspace/store theme instead of
GhosttyRuntime.currentTheme, and update the scene-specific theme path so each
window uses its own palette consistently. In the code around WorkspaceDetailView
and the theme sync logic near the scene update methods, stop writing each
scene’s theme back into GhosttyRuntime and ensure any theme changes flow only
through the local view/store state.
- Around line 464-465: The sheet dismiss path in WorkspaceDetailView is only
clearing the Task reference, so the in-flight text capture keeps running. Update
the .sheet onDismiss handler to actively cancel the existing capture task before
niling state, using the textSheetCapture reference (and any capture-start/stop
helpers involved) so dismissing the sheet stops the waiter and prevents a second
capture from starting on reopen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 258595ba-8b55-4184-8e91-85f6319be72c
📒 Files selected for processing (23)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGridReplay.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileTerminalRenderGridTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/OptimisticWorkspaceCloseTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalComposerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalPalette.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/MobileTerminalThemeNotification.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/CopyableTerminalTextSelection.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/CopyableTerminalTextSelectionTests.swiftSources/GhosttyTerminalScrollBoost.swiftSources/GhosttyTerminalView.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController+MobileScrollPrefetch.swiftcmuxTests/GhosttyScrollPreciseBoostTests.swift
lawrencecchen
force-pushed
the
feat-ios-combined-6663-6664-6736
branch
from
June 25, 2026 00:36
0f17f22 to
4147c28
Compare
…3-6664-6736 # Conflicts: # .github/swift-file-length-budget.tsv
…3-6664-6736 # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fb23a7a. Configure here.
lawrencecchen
added
the
stale-revisit
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
2 participants
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.
Combines and fixes:
Review fixes included:
workspace.closeto explicitly returnclosed: trueand report last-workspace close blocks without pretending the workspace is pinned.#rrggbbGhostty directives.Checks run:
$autoreview --mode branch --base origin/maincleanswift testinPackages/iOS/CmuxMobileShellswift testinPackages/Shared/CMUXMobileCoreswift testinPackages/iOS/CmuxMobileTerminalKitswift testinPackages/macOS/CmuxControlSocketgit diff --checkLocal note:
swift testinPackages/iOS/CmuxMobileShellUIandPackages/iOS/CmuxMobileTerminalis blocked locally becauseGhosttyKit.xcframeworkis absent in this worktree; tagged app reload will compile those targets via the normal builder.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile sync/reconciliation for workspace close and theme propagation over render-grid frames; regressions could show wrong chrome colors, flicker on theme change, or briefly wrong workspace lists, but changes are well-tested and scoped to iOS/mobile core.
Overview
Bundles three mobile-facing improvements: Mac-driven terminal theming, instant workspace close, and a more reliable View as Text flow.
Terminal themes add a shared
TerminalThememodel (validated hex, Monokai default, Ghostty config emission). Full mobile render-grid frames now carryterminal_theme; VT replay applies default/selection colors and the 16-color palette. On iOS, incoming grids update per-surface theme cache andactiveTerminalTheme; SwiftUI chrome moves from fixed Monokai to a scene-scopedTerminalPalette, and Ghostty config loading is split intoGhosttyRuntimeConfigLoaderso libghostty and UIKit accessory/composer chrome follow the active theme.Workspace close on iOS removes the row immediately, tracks pending closes so stale
workspace.listsnapshots cannot resurrect them, reconciles when the Mac confirms removal, and rolls back row/selection on RPC failure orclosed: false. Auth failures are classified viaMobileShellAuthorizationFailureClassifierfor disconnect handling.View as Text arms capture at menu tap (before the sheet presents), tightens surface eligibility (id match, live surface, not dismantled), improves SCREEN→VIEWPORT text resolution, and adds a ~3s bounded wait with a user-visible timeout in the sheet.
Reviewed by Cursor Bugbot for commit 4312252. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Combines three iOS upgrades: instant workspace close, Mac‑driven terminal theming with palette‑tinted UI (SwiftUI chrome and the input bar), and a reliable “View as Text” sheet with a 3s timeout. Removes remaining Monokai fallbacks so iOS chrome fully matches the active theme.
New Features
TerminalThemeinCMUXMobileCore(strict hex validation); full frames carry theme + 16‑color palette; VT replay applies defaults/selection/cursor and palette; Mac attaches live theme and pushes a full frame on theme changes; iOS caches themes per surface, updates the selected terminal and scene‑scoped palette, and tints nav chrome and the input bar via a new config loader; per‑surface theming prevents cross‑scene tint leaks; removed hardcoded Monokai chrome fallbacks.Bug Fixes
workspace.close: success returnsclosed: true; closing the last workspace reportsblockedwithreasonandpinned; iOS handles both and preserves selection on rollback.#rrggbbfor@GhosttyKit; preserve iOS scrollback on theme refresh; fix chrome backgrounds by driving SwiftUI/UIKit from the active palette and eliminating Monokai fallbacks.Written for commit 4312252. Summary will update on new commits.
Summary by CodeRabbit