Repository navigation
Conversation
Add programmatic workspace color control via CLI and socket API, and propagate workspace colors to notifications for visual identification. New features: - workspace.set_color v2 method + set-workspace-color CLI command - custom_color field in workspace.list response - Notifications auto-inherit workspace color (dot + left-edge bar) - --color flag on cmux notify for explicit override - Skill documentation in skills/cmux-workspace-colors/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@apollow is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds workspace color management end-to-end: CLI subcommand to set/clear workspace colors, a v2 Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmux)
participant Controller as TerminalController (v2)
participant TabMgr as TabManager / Workspace state
participant NotifStore as TerminalNotificationStore
participant UI as NotificationsPage
CLI->>Controller: set-workspace-color(workspace_id, color|null)
Controller->>TabMgr: update workspace.customColor
TabMgr-->>Controller: confirmation
CLI->>Controller: notify(target?, surface?, color?)
Controller->>TabMgr: resolve workspace/surface handles (normalize)
Controller->>NotifStore: create notification (workspace?, surface?, color resolved)
NotifStore->>NotifStore: persist TerminalNotification(color)
NotifStore-->>UI: publish notification
UI->>UI: compute dotColor from notification.color + colorScheme
UI-->>User: render notification with colored dot and leading bar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR wires workspace color support end-to-end: a new Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant TC as TerminalController (v2)
participant TM as TabManager
participant NS as TerminalNotificationStore
participant UI as NotificationsPage
Note over CLI,UI: set-workspace-color flow
CLI->>TC: workspace.set_color {workspace_id, color}
TC->>TC: normalizedHex(color) validation
TC->>TM: setTabColor(tabId, color)
TC-->>CLI: {workspace_id, color}
Note over CLI,UI: notify flow (color inheritance)
CLI->>TC: notification.create / create_for_surface / create_for_target
TC->>TM: resolve workspace → ws.customColor
TC->>TC: resolvedColor = explicit color ?? ws.customColor
TC->>NS: addNotification(..., color: resolvedColor)
NS-->>TC: notification stored
TC-->>CLI: {workspace_id, surface_id, ...}
Note over UI: rendering
UI->>NS: read notifications
NS-->>UI: [TerminalNotification(color?)]
UI->>UI: WorkspaceTabColorSettings.displayColor(hex)
UI->>UI: render dot + left-edge bar in resolved color
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let title = (params["title"] as? String) ?? "Notification" | ||
| let subtitle = (params["subtitle"] as? String) ?? "" | ||
| let body = (params["body"] as? String) ?? "" | ||
| let color = v2String(params, "color") |
Contributor
There was a problem hiding this comment.
No color validation in notification creation methods
The color param accepted here is stored directly into TerminalNotification.color without any format validation. v2WorkspaceSetColor validates the hex with WorkspaceTabColorSettings.normalizedHex(), but all three notification creation paths (v2NotificationCreate, v2NotificationCreateForSurface, v2NotificationCreateForTarget) skip this check entirely.
If a caller passes "notacolor" or "#GGGGGG" via --color, WorkspaceTabColorSettings.displayColor(hex:colorScheme:) in NotificationRow will return nil and fall back silently to the accent color — but the invalid string still gets persisted in TerminalNotificationStore and is returned through notification.list.
Apply the same guard that v2WorkspaceSetColor uses before storing the value:
let color: String?
if let raw = v2String(params, "color") {
guard let normalized = WorkspaceTabColorSettings.normalizedHex(raw) else {
return .err(code: "invalid_params", message: "Invalid color hex. Use format #RRGGBB.", data: nil)
}
color = normalized
} else {
color = nil
}This same pattern needs to be applied in v2NotificationCreateForSurface and v2NotificationCreateForTarget as well.
| body: body, | ||
| color: resolvedColor | ||
| ) | ||
| result = .ok(["workspace_id": ws.id.uuidString, "workspace_ref": v2Ref(kind: .workspace, uuid: ws.id), "surface_id": surfaceId.uuidString, "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), "window_id": v2OrNull(v2ResolveWindowId(tabManager: tabManager)?.uuidString), "window_ref": v2Ref(kind: .window, uuid: v2ResolveWindowId(tabManager: tabManager))]) |
Contributor
There was a problem hiding this comment.
v2ResolveWindowId called twice in the same expression
v2ResolveWindowId(tabManager: tabManager) is invoked twice on this single line — once for window_id and again for window_ref. Depending on the implementation, this may trigger duplicate work on the main thread. The same issue exists in v2NotificationCreateForTarget at the equivalent result-construction line.
Store the result in a local variable before building the dictionary:
let windowId = v2ResolveWindowId(tabManager: tabManager)
result = .ok([
"workspace_id": ws.id.uuidString,
"workspace_ref": v2Ref(kind: .workspace, uuid: ws.id),
"surface_id": surfaceId.uuidString,
"surface_ref": v2Ref(kind: .surface, uuid: surfaceId),
"window_id": v2OrNull(windowId?.uuidString),
"window_ref": v2Ref(kind: .window, uuid: windowId)
])![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GhosttyTerminalView.swift (1)
1723-1734:⚠️ Potential issue | 🟠 MajorResolve
wsColorfrom the owning tab manager, not the global one.Line 1727 hard-codes
AppDelegate.shared?.tabManager?.tabs, so notifications emitted from terminals in another window/manager will silently lose their workspace color. This breaks the new inheritance behavior for that path.🛠️ Suggested fix
performOnMain { let tabTitle = AppDelegate.shared?.tabManager?.titleForTab(tabId) ?? "Terminal" let command = actionTitle.isEmpty ? tabTitle : actionTitle let body = actionBody - let wsColor = AppDelegate.shared?.tabManager?.tabs.first(where: { $0.id == tabId })?.customColor + let manager = AppDelegate.shared?.tabManagerFor(tabId: tabId) ?? AppDelegate.shared?.tabManager + let wsColor = manager?.tabs.first(where: { $0.id == tabId })?.customColor TerminalNotificationStore.shared.addNotification( tabId: tabId, surfaceId: surfaceId, title: command, subtitle: "", body: body, color: wsColor ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1723 - 1734, The code resolves wsColor using the global AppDelegate.shared?.tabManager?.tabs which can pick the wrong window's tab list; change the lookup to use the owning tab manager for this terminal (use the same TabManager instance that provided titleForTab(tabId) or the owner reference available in GhosttyTerminalView) so wsColor is fetched from that manager's tabs (e.g., replace AppDelegate.shared?.tabManager?.tabs.first(where: { $0.id == tabId }) with the owningManager.tabs.first(where: { $0.id == tabId })). Ensure you still pass tabId, surfaceId, title (command), subtitle and body into TerminalNotificationStore.shared.addNotification after fetching wsColor from the correct tab manager.Sources/TerminalController.swift (1)
5310-5339:⚠️ Potential issue | 🟠 MajorValidate notification override colors the same way workspace colors are validated.
These paths persist any non-empty string as a notification color. That bypasses the hex normalizer used by
workspace.set_color, so bad override values can leak intonotification.listand the new colorized notification UI. Normalize here too and fail fast on invalid overrides.Suggested fix pattern
- let color = v2String(params, "color") + let color: String? + if params.keys.contains("color") { + if params["color"] is NSNull { + color = nil + } else if let raw = params["color"] as? String, + let normalized = WorkspaceTabColorSettings.normalizedHex(raw) { + color = normalized + } else { + return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil) + } + } else { + color = nil + }Apply the same parsing in all three notification creation methods before
resolvedColoris computed.Also applies to: 5352-5376, 5392-5416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5310 - 5339, The notification handlers currently accept any non-empty string for color and set resolvedColor directly; instead, run the same workspace color normalization/parsing logic used by workspace.set_color (the hex normalizer) on the incoming override before computing resolvedColor and calling TerminalNotificationStore.shared.addNotification (e.g., in the block that computes resolvedColor and calls addNotification). If the parser fails, return an error V2CallResult (fail fast) rather than persisting the invalid value; apply this fix to all three notification creation methods (the blocks around resolvedColor and TerminalNotificationStore.shared.addNotification).
🧹 Nitpick comments (2)
skills/cmux-workspace-colors/references/commands.md (2)
22-26: Consider showing both set and clear payload examples.The text mentions omitting
colorto clear, but showing both examples would make this more immediately clear to developers:**Socket method:** `workspace.set_color` ```json // Set color { "workspace_id": "<uuid>", "color": "#C0392B" } // Clear color { "workspace_id": "<uuid>" } ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/cmux-workspace-colors/references/commands.md` around lines 22 - 26, Update the docs for the socket method workspace.set_color to include both a "set" and a "clear" JSON payload example so it's explicit how to clear the color; show one example with { "workspace_id": "<uuid>", "color": "#C0392B" } and another with only { "workspace_id": "<uuid>" } and add a short comment label for each (e.g., "Set color" and "Clear color") so developers immediately see both use cases.
7-9: Clarify mutual exclusivity of--clearand<#hex>in syntax.The current syntax shows
<#hex>as required, but line 19 demonstrates using--clearwithout providing a color. Standard CLI documentation conventions would make the mutual exclusivity clearer, for example:# Either set a color: cmux set-workspace-color [--workspace <id|ref>] [--] <#hex> # Or clear the color: cmux set-workspace-color [--workspace <id|ref>] --clearOr using a single line with alternation:
cmux set-workspace-color [--workspace <id|ref>] (--clear | [--] <#hex>)This prevents users from being confused about whether both arguments are allowed simultaneously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/cmux-workspace-colors/references/commands.md` around lines 7 - 9, Update the usage line for the `cmux set-workspace-color` command to indicate that `--clear` and `<#hex>` are mutually exclusive; specifically, modify the signature shown (`cmux set-workspace-color [--workspace <id|ref>] [--clear] [--] <#hex>`) to either present two separate usages (one for setting a color with `<#hex>` and one for clearing with `--clear`) or use an alternation group such as `(--clear | [--] <#hex>)` so readers of the `commands.md` doc clearly understand they cannot supply both `--clear` and a hex color together when invoking `cmux set-workspace-color`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 1369-1384: The code currently strips only "--clear" and then
treats remaining items as the color, which lets unknown flags like "--colour"
slip into the color string; before building colorArgs validate rem1 for unknown
flags by checking rem1.filter { $0.hasPrefix("--") && $0 != "--" } and if that
yields any entries throw a CLIError listing the invalid flags; perform this
check after computing isClear/rem1 (and before computing colorArgs/joining into
color) so functions like resolveWorkspaceId, the params dictionary, and the
set-workspace-color flow reject unknown flags instead of folding them into the
color value.
- Around line 1375-1376: The workspace-ID resolution needs to be scoped to the
selected window: change calls to resolveWorkspaceId (here in
set-workspace-color) to pass the window handle (e.g., windowArg/windowHandle)
and update resolveWorkspaceId to accept windowHandle: when a numeric workspace
identifier is given and a window override is present, call the backend
workspace.list with parameters including ["window_id": windowHandle] so the
lookup only searches workspaces in that window; ensure the returned wsId is
derived from that filtered list and keep the existing params dictionary (var
params) intact for later use.
In `@Sources/TerminalController.swift`:
- Around line 3134-3143: v2String(params, "color") returns nil for both missing
and non-string values, so currently inputs like {"color": 123} silently clear
color; fix by explicitly checking whether the "color" key exists in params and
branch on its actual value: if params does not contain "color", treat it as “no
change” (keep existing behavior outside this snippet); if params["color"] is
null/NSNull, set color = nil to explicitly clear; if params["color"] is a
String, validate it with WorkspaceTabColorSettings.normalizedHex (as currently
done) and use the normalized value; otherwise (any other non-string, non-null
value) return .err(code: "invalid_params", message: "Invalid color: expected
string or null.", data: nil). Update the logic around colorRaw / v2String to use
this presence/type check so malformed non-string values no longer silently clear
state.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1723-1734: The code resolves wsColor using the global
AppDelegate.shared?.tabManager?.tabs which can pick the wrong window's tab list;
change the lookup to use the owning tab manager for this terminal (use the same
TabManager instance that provided titleForTab(tabId) or the owner reference
available in GhosttyTerminalView) so wsColor is fetched from that manager's tabs
(e.g., replace AppDelegate.shared?.tabManager?.tabs.first(where: { $0.id ==
tabId }) with the owningManager.tabs.first(where: { $0.id == tabId })). Ensure
you still pass tabId, surfaceId, title (command), subtitle and body into
TerminalNotificationStore.shared.addNotification after fetching wsColor from the
correct tab manager.
In `@Sources/TerminalController.swift`:
- Around line 5310-5339: The notification handlers currently accept any
non-empty string for color and set resolvedColor directly; instead, run the same
workspace color normalization/parsing logic used by workspace.set_color (the hex
normalizer) on the incoming override before computing resolvedColor and calling
TerminalNotificationStore.shared.addNotification (e.g., in the block that
computes resolvedColor and calls addNotification). If the parser fails, return
an error V2CallResult (fail fast) rather than persisting the invalid value;
apply this fix to all three notification creation methods (the blocks around
resolvedColor and TerminalNotificationStore.shared.addNotification).
---
Nitpick comments:
In `@skills/cmux-workspace-colors/references/commands.md`:
- Around line 22-26: Update the docs for the socket method workspace.set_color
to include both a "set" and a "clear" JSON payload example so it's explicit how
to clear the color; show one example with { "workspace_id": "<uuid>", "color":
"#C0392B" } and another with only { "workspace_id": "<uuid>" } and add a short
comment label for each (e.g., "Set color" and "Clear color") so developers
immediately see both use cases.
- Around line 7-9: Update the usage line for the `cmux set-workspace-color`
command to indicate that `--clear` and `<#hex>` are mutually exclusive;
specifically, modify the signature shown (`cmux set-workspace-color [--workspace
<id|ref>] [--clear] [--] <#hex>`) to either present two separate usages (one for
setting a color with `<#hex>` and one for clearing with `--clear`) or use an
alternation group such as `(--clear | [--] <#hex>)` so readers of the
`commands.md` doc clearly understand they cannot supply both `--clear` and a hex
color together when invoking `cmux set-workspace-color`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e7f718a-f389-41a6-b58d-e19b777f6394
📒 Files selected for processing (7)
CLI/cmux.swiftSources/GhosttyTerminalView.swiftSources/NotificationsPage.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftskills/cmux-workspace-colors/SKILL.mdskills/cmux-workspace-colors/references/commands.md
Comment on lines
+1375
to
+1376
| let wsId = try resolveWorkspaceId(workspaceArg, client: client) | ||
| var params: [String: Any] = ["workspace_id": wsId] |
There was a problem hiding this comment.
Scope workspace index resolution to the selected --window.
Line 1375 calls resolveWorkspaceId without any window context. cmux --window <w> set-workspace-color --workspace 1 ... can therefore update the first workspace index 1 returned across all windows, not the workspace inside the selected window.
🧭 Suggested direction
- let wsId = try resolveWorkspaceId(workspaceArg, client: client)
+ let windowHandle = try normalizeWindowHandle(windowId, client: client)
+ let wsId = try resolveWorkspaceId(workspaceArg, windowHandle: windowHandle, client: client)private func resolveWorkspaceId(_ raw: String?, windowHandle: String?, client: SocketClient) throws -> String {
// For numeric handles, call workspace.list with ["window_id": windowHandle]
// when a window override is present.
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1375 - 1376, The workspace-ID resolution needs
to be scoped to the selected window: change calls to resolveWorkspaceId (here in
set-workspace-color) to pass the window handle (e.g., windowArg/windowHandle)
and update resolveWorkspaceId to accept windowHandle: when a numeric workspace
identifier is given and a window override is present, call the backend
workspace.list with parameters including ["window_id": windowHandle] so the
lookup only searches workspaces in that window; ensure the returned wsId is
derived from that filtered list and keep the existing params dictionary (var
params) intact for later use.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:7006">
P3: The usage text incorrectly implies `<#hex>` is always required, even though `--clear` is a valid no-color path.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Validate color hex in all 3 notification creation methods - Cache v2ResolveWindowId to avoid duplicate calls - Reject unknown flags in set-workspace-color CLI - Handle non-string color params in workspace.set_color - Fix usage text to show --clear and hex as mutually exclusive Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:4574">
P3: The usage line now says `<id|ref|index>` but the `--workspace` flag description still says `<id|ref>`. Update the flag description to match.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CLI/cmux.swift (1)
1378-1388:⚠️ Potential issue | 🟠 MajorPass
--windowinto workspace resolution instead of depending on current selection.Line 1378 still resolves the workspace without window context. For
cmux --window <w> set-workspace-color --workspace 1 ..., this only works because the top-level--windowpath focuses that window first, so a non-focus command mutates selection just to disambiguate the index.🧭 Suggested direction
- let wsId = try resolveWorkspaceId(workspaceArg, client: client) + let windowHandle = try normalizeWindowHandle(windowId, client: client) + let wsId = try resolveWorkspaceId(workspaceArg, windowHandle: windowHandle, client: client)Update
resolveWorkspaceIdoutside this hunk to useworkspace.listwithwindow_idwhen a window handle is present.Based on learnings, "Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 1378 - 1388, The code calls resolveWorkspaceId(workspaceArg, client: client) without the optional window context, causing non-focus commands to mutate app selection; pass the current window handle into resolveWorkspaceId (e.g., resolveWorkspaceId(workspaceArg, window: windowHandle, client: client)) when present instead of relying on global focus, and update the resolveWorkspaceId implementation to call the "workspace.list" RPC with params including "window_id" when a window handle is supplied so workspace indices are resolved in the context of that window; keep existing behavior for the no-window case and do not change sendV2("workspace.set_color", params: params) usage other than using the workspace id returned by the updated resolver.Sources/TerminalController.swift (1)
3134-3148:⚠️ Potential issue | 🟠 MajorRequire an explicit
colorvalue forworkspace.set_color.Line 3146 currently treats an omitted
colorkey asnil, so a request that forgetscolorwill succeed and clear the workspace color. That makes an incomplete call destructive. Only explicitnullshould clear; a missing key should returninvalid_params.💡 Suggested fix
- let color: String? - if params.keys.contains("color") { - if params["color"] is NSNull { - color = nil - } else if let colorRaw = params["color"] as? String { - guard let normalized = WorkspaceTabColorSettings.normalizedHex(colorRaw) else { - return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil) - } - color = normalized - } else { - return .err(code: "invalid_params", message: "color must be a string or null", data: nil) - } - } else { - color = nil - } + guard params.keys.contains("color") else { + return .err(code: "invalid_params", message: "Missing color. Use null to clear.", data: nil) + } + + let color: String? + if params["color"] is NSNull { + color = nil + } else if let colorRaw = params["color"] as? String { + guard let normalized = WorkspaceTabColorSettings.normalizedHex(colorRaw) else { + return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil) + } + color = normalized + } else { + return .err(code: "invalid_params", message: "color must be a string or null", data: nil) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3134 - 3148, The handler for workspace.set_color currently treats a missing "color" key as nil (clearing the color); change the logic so that if the "color" key is absent you return .err(code: "invalid_params", message: "color must be provided", data: nil) instead of setting color = nil, while preserving the existing branches that accept NSNull (explicit null -> color = nil) and String (validate via WorkspaceTabColorSettings.normalizedHex and return the same "invalid_params" error on bad hex); update the error path for missing key accordingly in the same scope where you reference WorkspaceTabColorSettings.normalizedHex so only explicit null clears the workspace color.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 5318-5326: The current parsing uses v2String(params, "color")
which treats non-strings and whitespace-only strings as nil and silently
inherits ws.customColor; change each occurrence so that if params contains the
"color" key but v2String(params, "color") returns nil you return .err(code:
"invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil)
immediately; otherwise (key absent) set color = nil, and if a string is present
validate via WorkspaceTabColorSettings.normalizedHex(raw) as before. Update all
three code paths that call v2String(params, "color") (the blocks using variable
color and WorkspaceTabColorSettings.normalizedHex) to perform this presence/type
check.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 1378-1388: The code calls resolveWorkspaceId(workspaceArg, client:
client) without the optional window context, causing non-focus commands to
mutate app selection; pass the current window handle into resolveWorkspaceId
(e.g., resolveWorkspaceId(workspaceArg, window: windowHandle, client: client))
when present instead of relying on global focus, and update the
resolveWorkspaceId implementation to call the "workspace.list" RPC with params
including "window_id" when a window handle is supplied so workspace indices are
resolved in the context of that window; keep existing behavior for the no-window
case and do not change sendV2("workspace.set_color", params: params) usage other
than using the workspace id returned by the updated resolver.
In `@Sources/TerminalController.swift`:
- Around line 3134-3148: The handler for workspace.set_color currently treats a
missing "color" key as nil (clearing the color); change the logic so that if the
"color" key is absent you return .err(code: "invalid_params", message: "color
must be provided", data: nil) instead of setting color = nil, while preserving
the existing branches that accept NSNull (explicit null -> color = nil) and
String (validate via WorkspaceTabColorSettings.normalizedHex and return the same
"invalid_params" error on bad hex); update the error path for missing key
accordingly in the same scope where you reference
WorkspaceTabColorSettings.normalizedHex so only explicit null clears the
workspace color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b937196-a5a5-4813-a300-a31e69932a81
📒 Files selected for processing (2)
CLI/cmux.swiftSources/TerminalController.swift
- Missing color key now returns invalid_params (only null clears) - CLI --clear sends explicit NSNull instead of omitting key - Update skill docs: usage syntax, set/clear examples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
CLI/cmux.swift (1)
1378-1391:⚠️ Potential issue | 🟠 MajorResolve
set-workspace-coloragainst--windowwithout relying on focus side effects.Line 1378 still calls
resolveWorkspaceId(workspaceArg, client:), which ignores the explicit window context. Right now this only works because the command path focuses the target window earlier, so a passive metadata command mutates selection/focus as a routing side effect. Please thread the normalizedwindow_idinto workspace resolution instead. Based on learnings: Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 1378 - 1391, The call to resolveWorkspaceId(workspaceArg, client:) currently ignores an explicit --window context and relies on side-effect focus changes; update workspace resolution to accept and use the normalized window id (the parsed --window value) instead of relying on global focus—e.g., add/use a resolveWorkspaceId overload or parameter that takes windowId and pass the normalized window id when constructing params for workspace.set_color; ensure the code path that computes the window id (the CLI's window normalization) is used and do not perform any focus-changing operations during resolution.Sources/TerminalController.swift (1)
5318-5326:⚠️ Potential issue | 🟠 MajorReject invalid notification
colorvalues instead of inheriting.These three paths still parse via
v2String(params, "color"), so values like123or" "collapse toniland silently fall back tows.customColor. That breaks the new validation contract for badcolorparams.Suggested fix
- let color: String? - if let raw = v2String(params, "color") { - guard let normalized = WorkspaceTabColorSettings.normalizedHex(raw) else { - return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil) - } - color = normalized - } else { - color = nil - } + let color: String? + if let rawValue = params["color"] { + if rawValue is NSNull { + color = nil + } else if let raw = rawValue as? String { + guard let normalized = WorkspaceTabColorSettings.normalizedHex(raw) else { + return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil) + } + color = normalized + } else { + return .err(code: "invalid_params", message: "color must be a hex string or null", data: nil) + } + } else { + color = nil + }Also applies to: 5368-5376, 5417-5425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5318 - 5326, The current parsing uses v2String(params, "color") which treats invalid inputs like "123" or whitespace as nil and silently falls back to ws.customColor; instead detect when a color key was provided but normalization failed and return an error. Update the color parsing branch (the code that calls v2String and WorkspaceTabColorSettings.normalizedHex) to: first check whether the params contain the "color" key (e.g., params["color"] != nil or a helper that checks presence); if present, call v2String to get raw, then if WorkspaceTabColorSettings.normalizedHex(raw) returns nil return .err(code: "invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil); only treat color as nil when the key was not provided at all so inheritance to ws.customColor remains unchanged. Ensure the same fix is applied to the other two occurrences that call v2String/normalizedHex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 4861-4867: The help text for the new notify command currently
lists "--workspace <id|ref>" and "--surface <id|ref>" but the implementation
accepts indexes via normalizeWorkspaceHandle and normalizeSurfaceHandle; update
the flag usage strings for the notify command (the lines showing --workspace and
--surface) to advertise "<id|ref|index>" (or "|index") to match other v2-backed
commands so users know indexes are accepted; ensure both occurrences (the block
around the notify flags and the duplicate at the other documented location) are
changed to the same wording.
- Around line 1509-1521: The CLI currently uses optionValue(commandArgs, name:
"--color") and accepts the next token even if it's another flag, so invoking
`--color` with no value (e.g. `--color --workspace ...`) produces a server error
later; change the notify parsing to detect a provided --color flag with a
missing/invalid value: after calling optionValue for "--color" (and/or by
checking commandArgs contains "--color"), validate that the returned color is
non-nil and does not start with "-" (indicating another flag); if invalid, throw
or return a user-facing parse error immediately before building params (the code
around the color variable and the params dictionary) so the CLI fails fast with
a clear message.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 1378-1391: The call to resolveWorkspaceId(workspaceArg, client:)
currently ignores an explicit --window context and relies on side-effect focus
changes; update workspace resolution to accept and use the normalized window id
(the parsed --window value) instead of relying on global focus—e.g., add/use a
resolveWorkspaceId overload or parameter that takes windowId and pass the
normalized window id when constructing params for workspace.set_color; ensure
the code path that computes the window id (the CLI's window normalization) is
used and do not perform any focus-changing operations during resolution.
In `@Sources/TerminalController.swift`:
- Around line 5318-5326: The current parsing uses v2String(params, "color")
which treats invalid inputs like "123" or whitespace as nil and silently falls
back to ws.customColor; instead detect when a color key was provided but
normalization failed and return an error. Update the color parsing branch (the
code that calls v2String and WorkspaceTabColorSettings.normalizedHex) to: first
check whether the params contain the "color" key (e.g., params["color"] != nil
or a helper that checks presence); if present, call v2String to get raw, then if
WorkspaceTabColorSettings.normalizedHex(raw) returns nil return .err(code:
"invalid_params", message: "Invalid color hex. Use format `#RRGGBB`.", data: nil);
only treat color as nil when the key was not provided at all so inheritance to
ws.customColor remains unchanged. Ensure the same fix is applied to the other
two occurrences that call v2String/normalizedHex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cda8d8b-6a66-4a42-81ef-92cbf9663c85
📒 Files selected for processing (3)
CLI/cmux.swiftSources/TerminalController.swiftskills/cmux-workspace-colors/references/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/cmux-workspace-colors/references/commands.md
- Notification color: reject non-string/invalid types (not just strings) - CLI notify: validate --color has a value before sending request - Update notify help text to show index option for workspace/surface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CLI/cmux.swift (2)
1378-1379:⚠️ Potential issue | 🟠 MajorScope
set-workspace-colorresolution to the selected window.Line 1378 still goes through
resolveWorkspaceId(...), which scans across windows for short refs and ignores--windowfor index lookups.cmux --window window:2 set-workspace-color --workspace workspace:1 ...can therefore recolor the first matching workspace in another window instead of the target one. This path should use the same window-aware normalization flow as the newer v2 commands instead of the global resolver.🛠️ Suggested direction
- let wsId = try resolveWorkspaceId(workspaceArg, client: client) + let windowHandle = try normalizeWindowHandle(windowId, client: client) + let wsId = try resolveWorkspaceId(workspaceArg, windowHandle: windowHandle, client: client)private func resolveWorkspaceId(_ raw: String?, windowHandle: String?, client: SocketClient) throws -> String { // Reuse window-scoped workspace.list lookup when resolving short refs/indexes. }Based on learnings: Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 1378 - 1379, The set-workspace-color command currently calls resolveWorkspaceId(workspaceArg, client: client) which uses the global resolver that ignores the --window option and can match a workspace in another window; change the resolution to the window-aware normalization used by v2 commands by adding/using a variant like resolveWorkspaceId(_ raw: String?, windowHandle: String?, client: SocketClient) (or pass the existing window handle into the resolver) and perform a window-scoped workspace.list lookup for short refs/indexes so index/short-ref resolution is limited to the selected window instead of the global resolver; ensure set-workspace-color uses that window-aware resolver and does not trigger focus changes.
7021-7021:⚠️ Potential issue | 🟡 MinorAdvertise index handles in the top-level
notifyusage.Line 7021 still documents
--workspace <id|ref>/--surface <id|ref>, but Lines 1518-1519 normalize both through index-aware helpers. The summary usage should match the actual accepted handle formats.📝 Suggested wording
- notify --title <text> [--subtitle <text>] [--body <text>] [--color <#hex>] [--workspace <id|ref>] [--surface <id|ref>] + notify --title <text> [--subtitle <text>] [--body <text>] [--color <#hex>] [--workspace <id|ref|index>] [--surface <id|ref|index>]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` at line 7021, Update the top-level usage string for the notify command (the line starting with "notify --title <text> ...") to advertise the index-aware handle formats used by the helpers: replace "--workspace <id|ref>" and "--surface <id|ref>" with the normalized form that includes index (e.g. "--workspace <id|ref|index>" and "--surface <id|ref|index>"). Locate the usage text emitted for the notify command in CLI/cmux.swift (the "notify --title <text> ..." literal) and make the string match the normalized handle formats used by the index-aware helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 5347-5355: The v2 create handlers omit the resolved color from
successful responses; update the response dictionaries in
v2NotificationCreateForSurface, v2NotificationCreateForTarget (and the shown
handler building result with window_id/window_ref) to include the resolvedColor
(or resolvedColor?.uuidString as appropriate) under the "color" key using the
existing v2OrNull/v2Ref helpers so the create response returns "color":
v2OrNull(resolvedColor?.uuidString) (or the equivalent reference) alongside
workspace_id/workspace_ref/surface_id/window_id entries.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 1378-1379: The set-workspace-color command currently calls
resolveWorkspaceId(workspaceArg, client: client) which uses the global resolver
that ignores the --window option and can match a workspace in another window;
change the resolution to the window-aware normalization used by v2 commands by
adding/using a variant like resolveWorkspaceId(_ raw: String?, windowHandle:
String?, client: SocketClient) (or pass the existing window handle into the
resolver) and perform a window-scoped workspace.list lookup for short
refs/indexes so index/short-ref resolution is limited to the selected window
instead of the global resolver; ensure set-workspace-color uses that
window-aware resolver and does not trigger focus changes.
- Line 7021: Update the top-level usage string for the notify command (the line
starting with "notify --title <text> ...") to advertise the index-aware handle
formats used by the helpers: replace "--workspace <id|ref>" and "--surface
<id|ref>" with the normalized form that includes index (e.g. "--workspace
<id|ref|index>" and "--surface <id|ref|index>"). Locate the usage text emitted
for the notify command in CLI/cmux.swift (the "notify --title <text> ..."
literal) and make the string match the normalized handle formats used by the
index-aware helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fdeacc7-87cd-48d5-bf54-49abd24bc121
📒 Files selected for processing (2)
CLI/cmux.swiftSources/TerminalController.swift
- All 3 notification create methods now include color in response - Top-level notify usage text shows id|ref|index for workspace/surface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
@austinywang undertsood the idea based on async convo -- closing this since they will own it :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workspace.set_colorv2 socket method +cmux set-workspace-colorCLI command for programmatic workspace color controlcustom_colorfield toworkspace.listresponse--colorflag oncmux notifyfor explicit color overrideskills/cmux-workspace-colors/Details
New CLI commands
How it works
set-workspace-colorcalls the existingtabManager.setTabColor()via a newworkspace.set_colorv2 methodcolor = explicit ?? workspace.customColorNotificationRowrenders the color as a dot indicator + left-edge bar on the cardworkspace.listnow includescustom_colorso tools can query assigned colorsFiles changed
Sources/TerminalController.swiftworkspace.set_colorv2 handler, color in workspace.list, color in all notification methodsSources/TerminalNotificationStore.swiftcolorfield onTerminalNotificationstructSources/NotificationsPage.swiftSources/GhosttyTerminalView.swiftCLI/cmux.swiftset-workspace-colorcommand,--coloron notify, migrated notify to v2skills/cmux-workspace-colors/Test plan
./scripts/reload.sh --tag workspace-colorscmux set-workspace-color "#C0392B"— verify sidebar tab turns redcmux notify --title Test --body Hello— verify notification shows red dot + left-edge barcmux notify --title Test --color "#1565C0"— verify explicit blue overrides workspace redcmux set-workspace-color --clear— verify tab returns to defaultcmux list-workspaces --json— verifycustom_colorfield presentcmux set-workspace-color --clear "#C0392B"errors (mutually exclusive)Summary by cubic
Add workspace color support and color‑coded notifications. Workspaces can set a custom color, notifications inherit it by default, and the API/CLI now require an explicit
colorkey (use null to clear).New Features
workspace.set_colorAPI andcmux set-workspace-colorCLI; accepts#RRGGBBornullto clear (--clearand a color are mutually exclusive).cmux notify --color. UI shows a colored dot and left-edge bar.workspace.listreturnscustom_color, and notification create responses andnotification.listincludecolor. Docs added atskills/cmux-workspace-colors/.Bug Fixes
#RRGGBBand reject non-string/invalid types.notify --colornow requires a value;set-workspace-colorrejects unknown flags;notifyusage/help show workspace/surfaceid|ref|index.workspace.set_colorrequires an explicitcolorparam (missing key returnsinvalid_params);cmux set-workspace-color --clearsends explicitnull; cache window ID in v2 notification responses.Written for commit 3bf469e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation