iOS terminal: inherit the Mac's Ghostty theme instead of hardcoding Monokai#6119
iOS terminal: inherit the Mac's Ghostty theme instead of hardcoding Monokai#6119lawrencecchen wants to merge 13 commits into
Conversation
…onokai The iOS terminal hardcoded Monokai: the phone's local libghostty surface was built with a fixed background + palette 0..15, and the chrome (composer bar, pre-output placeholder) used a hardcoded Monokai color. A Mac running a different Ghostty theme had its ANSI-indexed output and the phone chrome mismatch it. The render-grid replay already sent the Mac's runtime default fg/bg/cursor (OSC 10/11/12), but never the 16-color ANSI palette, the configured (non-dynamic) theme defaults, or the UI chrome. Propagate the Mac's resolved theme on each full render-grid frame and apply it on iOS: - Frame: add terminal_palette (16 #RRGGBB entries, indices 0..15) to MobileTerminalRenderGridFrame, alongside the existing terminal_fg/bg/cursor. Additive, full-snapshot only, nil'd on deltas. New MobileInheritedTerminalTheme value type + frame.applyInheritedTheme(_:) shared between producer and tests. - Mac producer (TerminalSurface.mobileRenderGridFrame): stamp the resolved palette + default fg/bg/cursor from the same parsed GhosttyConfig the Mac renders with (the C config getter can't return the palette array, so read the parsed Swift config). Defaults backfill only where the program has not set a runtime OSC color, so a program's live colors still win. Resolved once and cached in MobileTerminalRenderObserver, invalidated on .ghosttyConfigDidReload, so the config-parse + color-format work stays off the per-keystroke path. - Replay: emit the inherited palette as OSC 4 in the full-snapshot byte stream so the phone resolves ANSI-indexed colors against the Mac's theme. - Chrome: the composer/input-accessory bar and the pre-output placeholder follow the inherited background (recorded per surface in MobileShellComposite), not a constant. Monokai stays ONLY as the explicit pre-first-frame fallback. - Live theme changes: the observer tracks a theme signature and forces a full frame when the theme changes, so a light/dark switch or edited Ghostty config updates an already-attached phone without a cold re-attach. Per-Mac correctness: the theme rides each Mac's frames (per surface/connection), so switching Macs/workspaces shows each Mac's own theme; nothing caches a global. Tests: palette round-trip + OSC 4 emission + partial-palette drop + delta drop + applyInheritedTheme backfill-vs-program-wins (CMUXMobileCore, 110 green); inherited-background recorded + survives deltas (CmuxMobileShell). Verified: macOS app build, iOS simulator build, CmuxMobileShell build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mobileRenderGridFrame only stamps the theme when given an inheritedTheme, but the mobile.terminal.replay RPC and scroll-prefetch path (the full snapshot that establishes a newly mounted iOS surface's baseline) called it without one, so a freshly attached phone stayed on the Monokai fallback until a later live full event arrived. Route the same cached inherited theme from MobileTerminalRenderObserver through TerminalController.mobileTerminalRenderGridFrame so the cold-attach snapshot is themed on the first frame. The observer's inheritedTheme() is now the shared accessor (still resolved once, cached, invalidated on config reload), so every full-snapshot producer stamps the same value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…invariant Two autoreview follow-ups: - Surface-only Ghostty reloads (GhosttyApp.reloadSurfaceConfiguration -> finishSurfaceConfigurationReload) invalidate GhosttyConfig's load cache without posting .ghosttyConfigDidReload (its observers read app-scoped state). The mobile inherited-theme cache derives from that parsed config, so a surface-only reload could leave paired phones stamped with the stale palette/colors. Drop the cache there too. invalidateInheritedThemeCache() is now nonisolated + lock-guarded (a tiny invalidation flag consumed by the main-actor inheritedTheme()), so either reload path can signal it without an actor hop or fragile assumeIsolated. - applyInheritedTheme assigned theme.palette directly, bypassing the frame initializer's "exactly 16 entries" rule, so a MobileInheritedTerminalTheme built with a partial palette could emit a partial OSC 4 frame. Enforce the 16-entry invariant in the helper. Added a regression test. CMUXMobileCore 111 tests green; macOS app + iOS simulator builds succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
recordInheritedTerminalBackground treated terminalBackground == nil the same for deltas and full snapshots, only ever preserving the last value. A delta legitimately omits the field, but a full snapshot with no background is authoritative (the Mac's configured default background was removed or no longer resolves), so the chrome must fall back rather than keep the stale theme color. Distinguish via frame.full: preserve on nil deltas, clear on nil full frames. Added a regression test. iOS simulator + CmuxMobileShell test builds succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Invalidating the theme cache only marked it dirty; the themeChanged force-full branch lives in emitRenderGrid, which an idle/background terminal never reaches without content changes. So a theme/config reload with no terminal activity left an attached phone on the old palette/chrome until unrelated output or a remount. invalidateInheritedThemeCache() now also schedules a global render-grid update (and a Ghostty tick, for background workspaces), so the forced full snapshot is actually pushed. No-op when no phone is subscribed. macOS app build succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Aziz documentation policy: public package symbols need triple-slash docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Mac-to-iOS terminal theme inheritance: introduces ChangesMac-to-iOS Terminal Theme Inheritance
Sequence DiagramsequenceDiagram
rect rgba(100, 100, 200, 0.5)
note over GhosttyConfig,MobileTerminalRenderObserver: Mac side — theme resolution
end
participant GhosttyConfig
participant MobileTerminalRenderObserver
participant TerminalSurface
participant MobileTerminalRenderGridFrame
rect rgba(200, 100, 100, 0.5)
note over MobileShellComposite,GhosttySurfaceView: iOS side — chrome application
end
participant MobileShellComposite
participant GhosttySurfaceRepresentable
participant GhosttySurfaceView
participant TerminalInputTextView
GhosttyConfig-->>MobileTerminalRenderObserver: .ghosttyConfigDidReload → invalidateInheritedThemeCache()
MobileTerminalRenderObserver->>TerminalSurface: mobileRenderGridFrame(inheritedTheme: inheritedTheme())
TerminalSurface->>GhosttyConfig: resolvedTerminalTheme()
GhosttyConfig-->>TerminalSurface: MobileInheritedTerminalTheme
TerminalSurface->>MobileTerminalRenderGridFrame: applyInheritedTheme(_:) on full frame
MobileTerminalRenderGridFrame-->>TerminalSurface: frame with terminalPalette + OSC 4 bytes on replay
TerminalSurface-->>MobileTerminalRenderObserver: (frame, rows)
MobileTerminalRenderObserver->>MobileShellComposite: deliverTerminalRenderGrid(frame, surfaceID:)
MobileShellComposite->>MobileShellComposite: recordInheritedTerminalBackground(from: frame)
MobileShellComposite-->>GhosttySurfaceRepresentable: terminal output chunk
GhosttySurfaceRepresentable->>GhosttySurfaceRepresentable: processOutputAndWait(chunk.data)
GhosttySurfaceRepresentable->>GhosttySurfaceView: applyInheritedChromeBackground(hex: inheritedTerminalBackground(surfaceID:))
GhosttySurfaceView->>TerminalInputTextView: applyBarBackgroundColor(_:)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adee376102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// notifications) still flushes. | ||
| private func scheduleThemeRefreshEmit() { | ||
| guard hasAnyRenderEventSubscribers else { return } | ||
| enqueueTerminalUpdate(surfaceID: nil) |
There was a problem hiding this comment.
When a Ghostty config/theme reload happens while any per-surface render update is already queued, this global enqueue can be coalesced into the same flushTerminalUpdates() pass; that method expands to all surfaces only when surfaceIDs.isEmpty, otherwise it emits only the pending surface IDs. In that common race, idle attached surfaces never receive the forced full frame that carries the new palette/chrome color, so they stay on the old theme until unrelated terminal activity or a reattach. Ensure theme refreshes force all subscribed surfaces even when specific updates are pending.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CMUXMobileCore/Sources/CMUXMobileCore/MobileInheritedTerminalTheme.swift`:
- Around line 54-61: The palette validation in the terminalPalette assignment
only checks that count equals 16, but does not validate the format of individual
entries. This allows malformed color entries to be accepted, causing partial
updates to the ANSI palette when invalid entries are skipped during replay.
Modify the condition to validate not only that theme.palette has exactly 16
entries, but also that each entry is a valid hex color in the format `#RRGGBB`. If
any entry fails validation, set terminalPalette to nil (atomic rejection)
instead of accepting the palette with malformed entries.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellInheritedThemeTests.swift`:
- Around line 88-98: The test is currently using
renderGridEventFrame(surfaceID:seq:text:) which creates a full snapshot fixture
rather than a delta frame, so it is not actually testing delta persistence
behavior. Replace the call to renderGridEventFrame() with a function that
generates a real delta frame instead, ensuring the test properly validates that
delta frames without background information rely on persisting the background
from the previous full frame.
🪄 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: b9771ddb-8a39-4350-84fb-3254c44b4570
📒 Files selected for processing (16)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/MobileInheritedTerminalTheme.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGrid.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGridReplay.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileTerminalRenderGridTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellInheritedThemeTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputTextView.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Mobile.swiftSources/GhosttyApp+SurfaceConfigurationReload.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController+MobileScrollPrefetch.swift
| // Enforce the same 16-entry invariant the frame initializer applies, so a | ||
| // partial palette never reaches OSC 4 (which would mix inherited and | ||
| // fallback colors). A non-16 palette is dropped, keeping the phone's | ||
| // consistent built-in fallback. | ||
| terminalPalette = (theme.palette?.count == 16) ? theme.palette : nil | ||
| if terminalForeground == nil { terminalForeground = theme.foreground } | ||
| if terminalBackground == nil { terminalBackground = theme.background } | ||
| if terminalCursorColor == nil { terminalCursorColor = theme.cursor } |
There was a problem hiding this comment.
Validate palette entry format before accepting inherited palette.
At Line 58, the gate only checks count == 16. That still admits malformed entries, and replay later skips invalid entries individually, which can leave a partially updated ANSI palette (stale slots from prior state). Please normalize atomically: accept the palette only when all 16 entries are valid #RRGGBB, otherwise drop the whole palette to nil.
Proposed fix
public mutating func applyInheritedTheme(_ theme: MobileInheritedTerminalTheme) {
- terminalPalette = (theme.palette?.count == 16) ? theme.palette : nil
+ terminalPalette = Self.normalizedTerminalPalette(theme.palette)
if terminalForeground == nil { terminalForeground = theme.foreground }
if terminalBackground == nil { terminalBackground = theme.background }
if terminalCursorColor == nil { terminalCursorColor = theme.cursor }
}
+
+private static func normalizedTerminalPalette(_ palette: [String]?) -> [String]? {
+ guard let palette, palette.count == 16 else { return nil }
+ let isValidHex: (String) -> Bool = { value in
+ guard value.hasPrefix("#"), value.count == 7 else { return false }
+ return Int(value.dropFirst(), radix: 16) != nil
+ }
+ return palette.allSatisfy(isValidHex) ? palette : 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/CMUXMobileCore/Sources/CMUXMobileCore/MobileInheritedTerminalTheme.swift`
around lines 54 - 61, The palette validation in the terminalPalette assignment
only checks that count equals 16, but does not validate the format of individual
entries. This allows malformed color entries to be accepted, causing partial
updates to the ANSI palette when invalid entries are skipped during replay.
Modify the condition to validate not only that theme.palette has exactly 16
entries, but also that each entry is a valid hex color in the format `#RRGGBB`. If
any entry fails validation, set terminalPalette to nil (atomic rejection)
instead of accepting the palette with malformed entries.
Greptile SummaryThis PR propagates the Mac's resolved Ghostty theme (16-color ANSI palette + default fg/bg/cursor) to paired iOS terminals so the phone inherits the Mac's theme instead of always showing a hardcoded Monokai palette.
Confidence Score: 5/5Safe to merge; all call sites that touch the new theme-cache and chrome-recoloring paths are main-actor-isolated and the GhosttyConfig cache is lock-protected, so no data-race exposure was introduced. The wire-protocol changes are additive and backward-compatible (new field is optional, delta frames drop it). The theme-cache lifecycle is well-bounded: populated lazily on the first full emit, cleared on config reload, and the forced full-frame emit covers idle terminals. Background recording is correctly gated on delivery success, preventing stale-chrome after unregister. Unit tests cover palette invariants, OSC 4 replay, JSON round-trip, delta suppression, and the delivery-gate guard. No files require special attention. The cold-attach path in TerminalController+MobileScrollPrefetch.swift and the live-emit path in MobileTerminalRenderObserver.swift both correctly share the same cached theme instance. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GC as GhosttyConfig
participant MTRO as RenderObserver
participant TS as TerminalSurface
participant Replay as GridReplay
participant MSC as MobileShellComposite
participant GSV as GhosttySurfaceView
Note over MTRO: emitRenderGrid triggered
MTRO->>MTRO: inheritedTheme() - check cache
alt cache miss
MTRO->>GC: resolvedTerminalTheme()
GC-->>MTRO: MobileInheritedTerminalTheme
end
MTRO->>TS: "mobileRenderGridFrame(full=true, inheritedTheme)"
TS->>TS: applyInheritedTheme on full snapshot
TS-->>MTRO: frame with palette+fg/bg/cursor
MTRO->>MTRO: compare themeSignature - force full if changed
MTRO->>MSC: emitEvent terminal.render_grid
Note over MSC,GSV: iOS replay path
MSC->>Replay: vtPatchBytes()
Replay-->>MSC: OSC4x16 + OSC10/11/12 bytes
MSC->>GSV: processOutputAndWait(bytes)
MSC->>GSV: applyInheritedChromeBackground(hex)
GSV->>GSV: tint accessory bar and placeholder
Note over MTRO: On ghosttyConfigDidReload
MTRO->>MTRO: invalidateInheritedThemeCache
MTRO->>MTRO: scheduleThemeRefreshEmit
%%{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 GC as GhosttyConfig
participant MTRO as RenderObserver
participant TS as TerminalSurface
participant Replay as GridReplay
participant MSC as MobileShellComposite
participant GSV as GhosttySurfaceView
Note over MTRO: emitRenderGrid triggered
MTRO->>MTRO: inheritedTheme() - check cache
alt cache miss
MTRO->>GC: resolvedTerminalTheme()
GC-->>MTRO: MobileInheritedTerminalTheme
end
MTRO->>TS: "mobileRenderGridFrame(full=true, inheritedTheme)"
TS->>TS: applyInheritedTheme on full snapshot
TS-->>MTRO: frame with palette+fg/bg/cursor
MTRO->>MTRO: compare themeSignature - force full if changed
MTRO->>MSC: emitEvent terminal.render_grid
Note over MSC,GSV: iOS replay path
MSC->>Replay: vtPatchBytes()
Replay-->>MSC: OSC4x16 + OSC10/11/12 bytes
MSC->>GSV: processOutputAndWait(bytes)
MSC->>GSV: applyInheritedChromeBackground(hex)
GSV->>GSV: tint accessory bar and placeholder
Note over MTRO: On ghosttyConfigDidReload
MTRO->>MTRO: invalidateInheritedThemeCache
MTRO->>MTRO: scheduleThemeRefreshEmit
Reviews (8): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// The Monokai background used as the chrome fallback before the Mac's theme | ||
| /// is known (matches the launch ghostty config and the bar's default fill). | ||
| private static let monokaiChromeBackground = UIColor(red: 0x27 / 255.0, green: 0x28 / 255.0, blue: 0x22 / 255.0, alpha: 1) |
There was a problem hiding this comment.
The new constant was inserted immediately after the closing /// No sleep / 'asyncAfter'… line of setComposerActive's long doc comment. Swift associates a contiguous /// block with the next declaration, so the entire setComposerActive doc block (including the "No sleep / asyncAfter" tail) now attaches to monokaiChromeBackground instead. setComposerActive is left with no doc comment. A blank line before the new /// breaks the block and restores the intended attachment.
| /// A tiny lock-guarded boolean so config invalidation (which can originate off | ||
| /// the main actor) can signal the main-actor theme cache without an actor hop. | ||
| private final class ThemeCacheInvalidationFlag: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private var value = false | ||
|
|
||
| func set() { | ||
| lock.lock(); defer { lock.unlock() } | ||
| value = true | ||
| } | ||
|
|
||
| /// Returns whether the flag was set and clears it, atomically. | ||
| func consume() -> Bool { | ||
| lock.lock(); defer { lock.unlock() } | ||
| defer { value = false } | ||
| return value | ||
| } | ||
| } |
There was a problem hiding this comment.
ThemeCacheInvalidationFlag wraps NSLock with @unchecked Sendable to provide a thread-safe boolean. Swift 6's Synchronization module ships Atomic<Bool> for exactly this case — it is Sendable by design, eliminating the custom class, the manual lock/unlock, and the unsafe conformance annotation.
| /// A tiny lock-guarded boolean so config invalidation (which can originate off | |
| /// the main actor) can signal the main-actor theme cache without an actor hop. | |
| private final class ThemeCacheInvalidationFlag: @unchecked Sendable { | |
| private let lock = NSLock() | |
| private var value = false | |
| func set() { | |
| lock.lock(); defer { lock.unlock() } | |
| value = true | |
| } | |
| /// Returns whether the flag was set and clears it, atomically. | |
| func consume() -> Bool { | |
| lock.lock(); defer { lock.unlock() } | |
| defer { value = false } | |
| return value | |
| } | |
| } | |
| /// A thread-safe boolean so config invalidation (which can originate off | |
| /// the main actor) can signal the main-actor theme cache without an actor hop. | |
| private struct ThemeCacheInvalidationFlag: Sendable { | |
| private let value = Atomic<Bool>(false) | |
| func set() { | |
| value.store(true, ordering: .relaxed) | |
| } | |
| /// Returns whether the flag was set and clears it, atomically. | |
| func consume() -> Bool { | |
| value.exchange(false, ordering: .relaxed) | |
| } | |
| } |
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!
| if (0...15).allSatisfy({ config.palette[$0] != nil }) { | ||
| palette = (0...15).map { config.palette[$0]!.hexString() } | ||
| } else { | ||
| palette = nil | ||
| } |
There was a problem hiding this comment.
map is guarded by an allSatisfy check immediately before it, so it cannot fail on a single-threaded call — but the two passes over config.palette are easy to misread and fragile if the palette type ever changes. A single compactMap + count guard expresses the same intent with no unsafe operator.
| if (0...15).allSatisfy({ config.palette[$0] != nil }) { | |
| palette = (0...15).map { config.palette[$0]!.hexString() } | |
| } else { | |
| palette = nil | |
| } | |
| let resolved = (0...15).compactMap { config.palette[$0]?.hexString() } | |
| palette = resolved.count == 16 ? resolved : nil |
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!
- The theme-cache invalidation entry point referenced the @mainactor `shared` observer (and a main-actor static flag) from the nonisolated surface-reload path, a Swift 6 actor-isolation warning that tripped the warning-budget CI gate. Make invalidateInheritedThemeCache() a nonisolated static func backed by a nonisolated static thread-safe flag; it hops to the main actor only to schedule the refresh emit. No new warnings. - Refresh the Swift file-length budget to accept the feature's modest growth across the touched files plus the new test file. CMUXMobileCore 111 tests green; macOS app build clean (no new warnings); both budget guards pass locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
inheritedTerminalBackgroundSurvivesDeltaFrames delivered a full frame (via renderGridEventFrame) but asserted delta behavior. With the new rule that a full frame's nil background clears the stored chrome color, that full frame would clear the value and fail the persist assertion. Add deltaRenderGridEventFrame (full: false, no theme) and use it, so the test exercises the delta path it asserts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
deliverTerminalRenderGrid recorded the inherited background before deliverTerminalOutput verified a stream/token still exists, so a late replay arriving after unregisterTerminalOutput would repopulate the per-surface background that unregister had just cleared, leaving the chrome stale on a later remount. deliverTerminalOutput now returns whether the frame was accepted into the stream, and the background is recorded only when it was. Ties the chrome state update to the same delivery guard as the bytes. iOS simulator + CmuxMobileShell test builds succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86cf60a3d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// instead of a hardcoded Monokai. Only updated when a frame actually carries | ||
| /// a background (deltas omit it), so the last known value survives across | ||
| /// deltas. Read via ``inheritedTerminalBackground(surfaceID:)``. | ||
| private var inheritedTerminalBackgroundBySurfaceID: [String: String] = [:] |
There was a problem hiding this comment.
This new per-surface background cache is not cleared by resetTerminalOutputTracking() (called when remoteClient becomes nil and also resets queues/tokens/transport), so a mounted terminal that previously received a render-grid theme can reconnect or fall back to raw-byte transport while inheritedTerminalBackground(surfaceID:) still returns the old Mac's color. Since GhosttySurfaceRepresentable reapplies that value after every output chunk, the input accessory chrome can remain stuck on a stale theme until a later render-grid full frame happens to overwrite it; include this dictionary in the reset/transport fallback path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 14-31: Add a regression test to
MobileShellInheritedThemeTests.swift that verifies the guard delivery check in
the deliverTerminalRenderGrid method. The test should: register a surface and
establish a stream, record an inherited background from a full frame, then
unregister the surface to clear the continuation and stream token, deliver a
late replay frame with a different background, and assert that the background
was not recorded (remains cleared). This ensures the guard statement preventing
recordInheritedTerminalBackground from executing when deliverTerminalOutput
returns false is properly tested and prevents future regressions on the timing
issue where late replays could repopulate stale chrome backgrounds.
🪄 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: 3ba77758-2b64-410f-8139-f3c2762b3e14
📒 Files selected for processing (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift
| func deliverTerminalRenderGrid(_ frame: MobileTerminalRenderGridFrame, surfaceID: String) { | ||
| deliverTerminalOutput( | ||
| let delivered = deliverTerminalOutput( | ||
| TerminalOutputDelivery( | ||
| renderGrid: frame, | ||
| replaceable: frame.isReplaceableViewportPatchForMobileDelivery | ||
| ), | ||
| surfaceID: surfaceID | ||
| ) | ||
| // Record the Mac's inherited theme background so the phone's chrome (the | ||
| // composer/input-accessory bar) can match it, but ONLY for a frame that | ||
| // was actually accepted into the surface's output stream. A late replay | ||
| // that arrives after the surface unregistered is dropped by | ||
| // `deliverTerminalOutput`; recording it anyway would repopulate the | ||
| // per-surface background that `unregisterTerminalOutput` just cleared and | ||
| // leave the chrome stale on a later remount. | ||
| guard delivered else { return } | ||
| recordInheritedTerminalBackground(from: frame) | ||
| } |
There was a problem hiding this comment.
Add test coverage for the failed-delivery scenario.
The guard logic correctly prevents stale background recording when deliverTerminalOutput returns false (e.g., a late replay arriving after surface unregistration). However, the existing test suite in MobileShellInheritedThemeTests.swift covers positive cases (background recorded from full frame, persists across deltas, cleared by backgroundless frame) but does not verify the negative case where delivery fails.
Add a regression test that:
- Registers a surface and establishes a stream
- Records an inherited background from a full frame
- Unregisters the surface (clearing the continuation and stream token)
- Delivers a late replay frame carrying a different background
- Verifies the background is not recorded (remains cleared)
This would provide explicit coverage for the timing issue described in the PR objectives and prevent future regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift
around lines 14 - 31, Add a regression test to
MobileShellInheritedThemeTests.swift that verifies the guard delivery check in
the deliverTerminalRenderGrid method. The test should: register a surface and
establish a stream, record an inherited background from a full frame, then
unregister the surface to clear the continuation and stream token, deliver a
late replay frame with a different background, and assert that the background
was not recorded (remains cleared). This ensures the guard statement preventing
recordInheritedTerminalBackground from executing when deliverTerminalOutput
returns false is properly tested and prevents future regressions on the timing
issue where late replays could repopulate stale chrome backgrounds.
The inherited theme is derived from the app-wide GhosttyConfig.load() and is the same for every surface, so invalidating it from the surface-only reload hook (finishSurfaceConfigurationReload) broke that path's isolation contract: a reload of one surface forced a global mobile theme refresh, and could stamp the freshly parsed global theme onto mobile sinks for surfaces whose Mac config was unchanged. Invalidate only on .ghosttyConfigDidReload (the genuine app-wide theme-change signal); a surface-only reload leaves the app-wide theme unchanged. This also lets invalidateInheritedThemeCache() go back to a plain @mainactor instance method (its sole caller is the main-actor observer), removing the lock-guarded flag and nested type, which also clears the Aziz concurrency/file-org policy findings. macOS app + iOS simulator builds clean (no new warnings); file-length and warning budgets pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fb7725b. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb7725bae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // full frame re-resolves and the attached phone picks up the new theme; | ||
| // the per-keystroke path never re-resolves. | ||
| observers.append(NotificationCenter.default.addObserver( | ||
| forName: .ghosttyConfigDidReload, |
There was a problem hiding this comment.
When a phone is already attached and the user reloads Ghostty config from a terminal surface, the Mac surface is updated through GHOSTTY_ACTION_RELOAD_CONFIG (Sources/GhosttyTerminalView.swift:3378-3389) and reloadSurfaceConfiguration, but that path only invalidates GhosttyConfig and intentionally does not post .ghosttyConfigDidReload. Since this new cache is cleared only by that notification, subsequent render-grid replays keep stamping the old palette/background even though the Mac surface has switched themes, so the phone stays on stale colors until a separate app-wide config reload or observer restart occurs.
Useful? React with 👍 / 👎.
The first version of MobileShellInheritedThemeTests used makeConnectedStore, whose loopback connect cannot bind in sandboxed environments, and CmuxMobileShell package tests are not run by any CI workflow, so those tests would never execute. Rewrite them to drive the recording path directly: a connectionless MobileShellComposite.preview() store with an output sink registered via terminalOutputStream, then deliverTerminalRenderGrid. The four tests now run anywhere `swift test` runs and cover: full-frame records background, delta preserves it, backgroundless full frame clears it, and a frame with no sink is dropped without recording (covering the accepted-frame delivery guard). Revert the now-unused helpers added to the liveness test support file. All four pass locally (no loopback needed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e7f98b652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stateSeq: seq, | ||
| scrollbackLines: scrollbackLines | ||
| scrollbackLines: scrollbackLines, | ||
| inheritedTheme: MobileTerminalRenderObserver.shared.inheritedTheme() |
There was a problem hiding this comment.
Because this path now stamps theme metadata into cold-attach/scroll-prefetch full snapshots, a replay captured just before a Ghostty theme reload can arrive after the live theme-refresh frame. Theme-only refreshes do not advance MobileTerminalByteTee's byte stateSeq, and the iOS replay path only rejects frames when deliveredSeq > replaySeq, so an equal-seq stale replay is still accepted and can overwrite the phone palette/chrome with the old theme; the Mac observer then will not repair it because it already recorded the new theme signature at the same seq. Include a theme generation/signature in the stale check or otherwise advance ordering for theme-only full frames.
Useful? React with 👍 / 👎.
…theme # Conflicts: # .github/swift-file-length-budget.tsv
…theme # Conflicts: # .github/swift-file-length-budget.tsv
1 participant
Inherit the Mac's Ghostty theme on iOS instead of hardcoding Monokai
The iOS terminal hardcoded the Monokai theme. The phone's local libghostty surface was built with a fixed
background = #272822+palette 0..15 = <monokai>, and the chrome (composer/input-accessory bar, pre-output placeholder) used a hardcoded Monokai color. If a Mac ran a different Ghostty theme, the phone's ANSI-indexed colors and chrome did not match it.What already propagated: the render-grid replay sent the Mac's runtime default foreground/background/cursor as OSC 10/11/12, so dynamic default fg/bg followed. What did not: the 16-color ANSI palette, the configured (non-dynamic) theme defaults, and the UI chrome. Indexed output and chrome stayed Monokai regardless of the Mac's theme.
The fix
The Mac now sends its resolved Ghostty theme on each full render-grid frame, and iOS applies it:
terminal_palette(16#RRGGBBentries, indices 0..15) toMobileTerminalRenderGridFrame, alongside the existingterminal_foreground/background/cursor. Additive, carried only on full snapshots, nil'd on deltas (mirrors the existing color fields).TerminalSurface.mobileRenderGridFrame): stamps the resolved palette + default fg/bg/cursor from the same parsedGhosttyConfigthe Mac renders with. libghostty's C config getter cannot return the palette array, so it reads the parsed Swift config directly. Defaults only backfill where the program has not set a runtime OSC 10/11/12, so a running program's live colors still win. The theme is resolved once and cached, invalidated on.ghosttyConfigDidReload, so the config-parse + color-format work stays off the per-keystroke render path.MobileShellComposite), not a constant.mobile.terminal.replay/ scroll-prefetch path (the first snapshot a newly mounted iOS surface gets) stamps the same cached theme, so the phone is themed on its first frame, not only after a later live event.MobileTerminalRenderObservertracks a theme signature and forces a full frame when the theme changes, and config-reload invalidation (both app-level.ghosttyConfigDidReloadand the surface-only reload path) drops the cached theme AND schedules a render-grid emit, so a light↔dark switch or edited Ghostty config updates an already-attached phone even on an idle terminal, without waiting for a cold re-attach. A full snapshot with no background clears the inherited chrome color so it never goes stale.Per-Mac correctness
The theme rides each Mac's render-grid frames, so it is per-surface/per-connection. Switching between paired Macs or workspaces shows each Mac's own theme; nothing caches one global theme across Macs.
Tests
applyInheritedThemebackfills defaults but a program's live OSC color wins.MobileShellInheritedThemeTests).swift testCMUXMobileCore green (110 tests). macOS app build, iOS simulator build, and CmuxMobileShell build all succeed.Verified vs UNVERIFIED
Verified: both-surface compile (macOS app + iOS simulator), CMUXMobileCore unit suite, the producer reads the resolved palette and the replay emits OSC 4. UNVERIFIED: the live on-device visual (a non-Monokai-themed Mac paired to a phone, showing the phone inherit that exact palette + chrome). That needs the full paired-device flow and iOS device signing creds that are unavailable in this environment; it is left for dogfood with a real paired iPhone.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile terminal sync, render-grid protocol, and theme caching on the typing path; mistakes could cause stale colors or extra full frames, but changes are additive with tests and no auth/data handling.
Overview
Paired iOS terminals no longer stay on hardcoded Monokai for ANSI palette, configured defaults, and surrounding chrome when the Mac uses a different Ghostty theme.
Wire format & replay: Full render-grid snapshots gain optional
terminal_palette(16#RRGGBBentries) plus existing default color fields; deltas omit palette/theme metadata. Full-snapshot VT replay now emits OSC 4 for the inherited palette before OSC 10/11/12. Partial palettes are dropped so the phone never mixes inherited and fallback colors; program-set dynamic defaults still win over static theme backfill viaapplyInheritedTheme.Mac producer:
TerminalSurfaceresolves theme from parsedGhosttyConfig, stamps it on full exports, and exposesresolvedTerminalTheme().MobileTerminalRenderObservercaches the theme, tracks a theme signature to force a full frame on theme-only changes, invalidates on.ghosttyConfigDidReload, and cold-attach replay uses the same cache.iOS UI:
MobileShellCompositerecords per-surface inherited background only when delivery succeeds; full frames without background clear chrome color.GhosttySurfaceView/ input accessory bar adopt that background after each applied output chunk; Monokai remains pre-first-frame fallback.Reviewed by Cursor Bugbot for commit ae713b1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
iOS now inherits the Mac’s Ghostty theme so the 16‑color palette, default colors, and chrome match the Mac from the first frame and stay in sync on theme changes. Background recording is guarded and clears on backgroundless full frames so chrome never shows stale colors after unmounts or config updates.
New Features
terminal_palette(16#RRGGBB) toMobileTerminalRenderGridFrame; replay emits OSC 4 for all 16 entries before OSC 10/11/12; dropped on deltas and if not exactly 16 entries.TerminalSurface.resolvedTerminalTheme; cached inMobileTerminalRenderObserver; applied on live emits and cold‑attach replay; program OSC 10/11/12 still win..ghosttyConfigDidReload; a theme signature forces a full snapshot and schedules a global emit so attached phones update even when idle or in the background.Bug Fixes
Written for commit ae713b1. Summary will update on new commits.
Summary by CodeRabbit