Repository navigation
Add customizable terminal faces - #8394
Closed
lawrencecchen wants to merge 2 commits into
Closed
lawrencecchen wants to merge 2 commits into
lawrencecchen wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds configurable terminal face settings, persistence, SwiftUI/AppKit editing, animated rendering, agent-event reactions, workspace and terminal overrides, and menu/control-command entry points. ChangesTerminal Face
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentHook
participant TerminalController
participant TerminalFaceController
participant TerminalPanel
participant TerminalFaceOverlayHost
participant TerminalFaceAppKitView
AgentHook->>TerminalController: push hook event
TerminalController->>TerminalFaceController: noteHookEvent(event)
TerminalFaceController->>TerminalPanel: refresh(panel)
TerminalFaceController->>TerminalFaceOverlayHost: update configuration and state
TerminalFaceOverlayHost->>TerminalFaceAppKitView: update overlay
TerminalFaceAppKitView->>TerminalFaceAppKitView: draw and animate face
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 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 |
![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
🤖 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/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceConfigurationEditor.swift`:
- Around line 96-118: Add .accessibilityLabel(title) to the Slider and both
interactive controls in colorField: TerminalFaceColorWell and TextField. Apply
the labels directly to each control using the title parameter, preserving the
existing layout and behavior.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceSettingsCard.swift`:
- Around line 27-35: Unify the enable controls in the TerminalFace settings card
so the header Toggle and editor enable toggle use the same persistence path.
Remove the header Toggle’s direct save behavior and route both through the
Apply-owned draft flow, or otherwise persist only the enabled field consistently
without committing pending slider or color edits.
In `@Sources/TerminalFaceController.swift`:
- Around line 128-143: Update TerminalFaceController.panel(for:) to fail closed:
require a valid event.surfaceId and resolve only the terminal panel matching
that exact surface ID. Remove the workspace.focusedPanelId fallback, and reject
events with a conflicting or unresolved workspaceId rather than searching other
workspaces or applying the event to a focused panel.
- Around line 158-167: Update the standalone editor window creation in the
window-opening method to assign a stable cmux.* identifier, then add that
identifier to cmuxAuxiliaryWindowIdentifiers so shared Cmd+W recognizes the
editor window as its close-shortcut owner. Keep the existing window presentation
and lifecycle behavior unchanged.
- Around line 83-89: Replace the substring check in the event-state switch with
structured boolean decoding of event.extraFieldsJSON, using a helper such as
toolUseFailed near the state-selection logic. Parse the JSON into a top-level
dictionary and return true only when the is_error field is actually the Boolean
true, then use that helper for the .postToolUse error case while preserving the
existing state mappings.
🪄 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: d24cdf78-cc5e-4d85-872b-413fb2fcee66
📒 Files selected for processing (26)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/System/ControlCommandCoordinator+SystemTabAction.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorTabActionTests.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/TerminalCatalogSection.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/TerminalFaceConfiguration.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/TerminalFaceConfigurationTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceConfigurationEditor.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceSettingsCard.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/Panels/TerminalPanelView.swiftSources/SessionPersistence.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swiftSources/TerminalController+ControlSystemContext2.swiftSources/TerminalController.swiftSources/TerminalFaceController.swiftSources/Workspace.swiftTHIRD_PARTY_LICENSES.mdcmux.xcodeproj/project.pbxprojweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
Comment on lines
+158
to
+167
| let window = NSWindow(contentViewController: NSHostingController(rootView: view)) | ||
| window.title = title | ||
| window.styleMask = [.titled, .closable, .resizable] | ||
| window.setContentSize(NSSize(width: 540, height: 650)) | ||
| window.center() | ||
| let controller = NSWindowController(window: window) | ||
| editorWindows.removeAll { $0.window?.isVisible != true } | ||
| editorWindows.append(controller) | ||
| controller.showWindow(nil) | ||
| NSApp.activate(ignoringOtherApps: true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register the editor window for shared Cmd+W ownership.
This standalone window has no stable identifier, so Cmd+W can fall through to workspace-panel closing. Assign a cmux.* identifier and register it in cmuxAuxiliaryWindowIdentifiers.
Proposed local change
let window = NSWindow(contentViewController: NSHostingController(rootView: view))
+ window.identifier = NSUserInterfaceItemIdentifier("cmux.terminalFaceEditor")
window.title = titleAs per coding guidelines, every standalone cmux-owned window must have a stable identifier and use the shared close-shortcut owner.
🤖 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/TerminalFaceController.swift` around lines 158 - 167, Update the
standalone editor window creation in the window-opening method to assign a
stable cmux.* identifier, then add that identifier to
cmuxAuxiliaryWindowIdentifiers so shared Cmd+W recognizes the editor window as
its close-shortcut owner. Keep the existing window presentation and lifecycle
behavior unchanged.
Source: Coding guidelines
Contributor
Greptile SummaryThis PR adds a customizable terminal face overlay that renders dot-matrix shapes behind terminal text and reacts to agent lifecycle events (thinking, working, done, needs input, error). The feature ships global, workspace, and per-terminal scoped overrides persisted in the session manifest, plus settings UI, context-menu items, and
Confidence Score: 4/5Safe to merge after fixing the color-space mismatch in the face renderer. The renderer's stateColor creates NSColor with calibratedRed: (device color space) while the color picker in TerminalFaceColorWell uses srgbRed:. For the same hex value the two color spaces produce visibly different hues, so the face renders with a different color than the user chose. Everything else — configuration model, session persistence, i18n, animation, control-socket routing, and the isDirty draft guard — looks correct. Sources/TerminalFaceController.swift — the private stateColor computed property at line 441. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as Agent hook event
participant TC as TerminalController
participant TFC as TerminalFaceController
participant TFP as TerminalFacePresentation
participant CALayer as CALayer (AppKit)
Agent->>TC: workstream hook event
TC->>TFC: noteHookEvent(event) [MainActor Task]
TFC->>TFC: resolve TerminalFaceState from hookEventName
TFC->>TFC: "states[panel.id] = state"
TFC->>TFP: update(configuration:state:)
TFP-->>CALayer: "@Observable change triggers updateNSView"
CALayer->>CALayer: rebuildPaths() + refreshAnimations()
Note over TFC: Config change path
TFC->>TFC: settings Task observes JSONConfigStore
TFC->>TFC: "globalConfiguration = value"
TFC->>TFC: refreshAll() each workspace each TerminalPanel
TFC->>TFP: update(configuration:state:)
%%{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 Agent as Agent hook event
participant TC as TerminalController
participant TFC as TerminalFaceController
participant TFP as TerminalFacePresentation
participant CALayer as CALayer (AppKit)
Agent->>TC: workstream hook event
TC->>TFC: noteHookEvent(event) [MainActor Task]
TFC->>TFC: resolve TerminalFaceState from hookEventName
TFC->>TFC: "states[panel.id] = state"
TFC->>TFP: update(configuration:state:)
TFP-->>CALayer: "@Observable change triggers updateNSView"
CALayer->>CALayer: rebuildPaths() + refreshAnimations()
Note over TFC: Config change path
TFC->>TFC: settings Task observes JSONConfigStore
TFC->>TFC: "globalConfiguration = value"
TFC->>TFC: refreshAll() each workspace each TerminalPanel
TFC->>TFP: update(configuration:state:)
Reviews (2): Last reviewed commit: "Address terminal face review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,536 @@ | |||
| import AppKit | |||
Contributor
There was a problem hiding this comment.
states dict growth
Every UUID that ever passes through noteHookEvent is inserted into states but never removed. In a long-lived session where terminals are opened and closed frequently, this dict grows without bound. Because TerminalFaceController is @MainActor and is never deallocated for the lifetime of the app, the accumulated entries are never reclaimed. A compaction pass — e.g., removing entries whose UUID no longer appears in any live mainWindowContext — should be added to refresh(workspace:) or refreshAll().
| @@ -0,0 +1,73 @@ | |||
| import CmuxSettings | |||
Contributor
There was a problem hiding this comment.
The .onChange(of: model.current) handler overwrites draft with the new external value whenever the persisted setting changes from outside the view. If the user has made edits to sliders or color wells but has not yet saved, those edits are silently discarded the moment any other agent or process writes a new value to the store. A guard (if !userHasEdited) or a confirmation prompt is needed before replacing an in-progress draft.
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/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceSettingsCard.swift`:
- Line 9: Update TerminalFaceSettingsCard’s dirtiness and apply flow so isDirty
is derived from whether draft differs from model.current, rather than being
cleared immediately after model.set. Preserve the draft when persistence fails
by comparing the draft against the previous model value, and keep Apply enabled
until the asynchronous write successfully updates model.current, allowing
retries on failure.
🪄 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: 0a294f55-ffb5-40cd-9e3f-7ff4eb2f4abf
📒 Files selected for processing (4)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceConfigurationEditor.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceSettingsCard.swiftSources/TerminalFaceController.swiftSources/cmuxApp.swift
| @State private var model: JSONValueModel<TerminalFaceConfiguration> | ||
| @State private var draft: TerminalFaceConfiguration | ||
| @State private var expanded = false | ||
| @State private var isDirty = false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep Apply enabled until persistence actually succeeds.
model.set returns before its asynchronous write completes, but Line 70 immediately clears isDirty. If the write fails, draft still differs from model.current while Apply remains disabled, preventing a retry. Derive dirtiness from those values and preserve drafts by comparing against the previous model value.
Proposed fix
- `@State` private var isDirty = false
- .disabled(!isDirty)
+ .disabled(draft == model.current)
- .onChange(of: model.current) { _, value in
- if !isDirty, value != draft { draft = value }
+ .onChange(of: model.current) { previous, value in
+ if draft == previous { draft = value }
}
- .onChange(of: draft) { _, value in
- isDirty = value != model.current
- }
private func save() {
var sanitized = draft
sanitized.sanitize()
+ draft = sanitized
model.set(sanitized)
- isDirty = false
}Also applies to: 48-48, 58-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/TerminalFace/TerminalFaceSettingsCard.swift`
at line 9, Update TerminalFaceSettingsCard’s dirtiness and apply flow so isDirty
is derived from whether draft differs from model.current, rather than being
cleared immediately after model.set. Preserve the draft when persistence fails
by comparing the draft against the previous model value, and keep Apply enabled
until the asynchronous write successfully updates model.current, allowing
retries on failure.
Comment on lines
+439
to
+445
| let value = UInt64(hex.dropFirst(), radix: 16) ?? 0xFFFFFF | ||
| return NSColor( | ||
| calibratedRed: CGFloat((value >> 16) & 0xFF) / 255, | ||
| green: CGFloat((value >> 8) & 0xFF) / 255, | ||
| blue: CGFloat(value & 0xFF) / 255, | ||
| alpha: 1 | ||
| ) |
Contributor
There was a problem hiding this comment.
stateColor creates an NSColor with calibratedRed: (device/calibrated color space) while the color picker in TerminalFaceColorWell.color(from:) creates it with srgbRed:. For a hex value like #3D70E0 the two color spaces produce observably different values, so the face renders with a different color than the user chose in the editor.
Suggested change
| let value = UInt64(hex.dropFirst(), radix: 16) ?? 0xFFFFFF | |
| return NSColor( | |
| calibratedRed: CGFloat((value >> 16) & 0xFF) / 255, | |
| green: CGFloat((value >> 8) & 0xFF) / 255, | |
| blue: CGFloat(value & 0xFF) / 255, | |
| alpha: 1 | |
| ) | |
| let value = UInt64(hex.dropFirst(), radix: 16) ?? 0xFFFFFF | |
| return NSColor( | |
| srgbRed: CGFloat((value >> 16) & 0xFF) / 255, | |
| green: CGFloat((value >> 8) & 0xFF) / 255, | |
| blue: CGFloat(value & 0xFF) / 255, | |
| alpha: 1 | |
| ) |
Contributor
Author
|
Configuration preview: https://cmux-czvfp70e3-manaflow.vercel.app/docs/configuration Validated live with HTTP 200 at commit c4882f9. |
lawrencecchen
added
the
stale-revisit
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.
Summary
Test plan
swift test --package-path Packages/macOS/CmuxSettingsswift test --package-path Packages/macOS/CmuxSettingsUIswift test --package-path Packages/macOS/CmuxControlSocket --filter ControlCommandCoordinatorTabActionTestsNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a customizable terminal face overlay that reacts to agent activity and can be configured globally, per workspace, or per terminal. Integrated in the terminal view as a Core Animation overlay with fast rendering and persisted overrides.
terminal.faceJSON setting with global default, workspace override, and per-terminal override; deterministic resolution and session/workspace persistence.customize_faceandtoggle_face; CLI addscustomize-faceandtoggle-face.Written for commit c4882f9. Summary will update on new commits.
Summary by CodeRabbit