Add set-workspace-color and clear-workspace-color CLI commands - #1825
Closed
thunter009 wants to merge 5 commits into
Closed
thunter009 wants to merge 5 commits into
thunter009 wants to merge 5 commits into
Conversation
Expose the existing tab color functionality (currently only available via right-click context menu) through the CLI and socket API. Closes manaflow-ai#1041 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@thunter009 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds two CLI subcommands ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client CLI
participant Socket as V2 Socket Dispatcher
participant Controller as TerminalController
participant Tabs as TabManager
CLI->>Socket: sendV2 { method: workspace.set_color / workspace.clear_color, params }
Socket->>Controller: dispatch v2 command
Controller->>Tabs: check tabManager.tabs for workspace_id
alt workspace exists
Controller->>Tabs: setTabColor(tabId, color) / setTabColor(tabId, nil)
Tabs-->>Controller: ack
Controller-->>Socket: ok payload (workspace/window refs, color?)
Socket-->>CLI: ok response
else not found
Controller-->>Socket: not_found payload
Socket-->>CLI: not_found response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Greptile SummaryThis PR adds Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Shell / .zshrc
participant CLI as cmux CLI
participant Socket as Socket API
participant TC as TerminalController
participant TM as TabManager
participant WS as Workspace
Shell->>CLI: cmux set-workspace-color "#C0392B"
CLI->>CLI: parseOption (--workspace)
CLI->>CLI: validate non-empty color
CLI->>Socket: workspace.set_color {workspace_id, color}
Socket->>TC: v2WorkspaceSetColor(params)
TC->>TC: validate non-empty (only check!)
TC->>TM: setTabColor(tabId, color)
TM->>WS: setCustomColor(hex)
WS->>WS: normalizedHex(hex) → nil if invalid!
WS-->>TM: customColor = nil (silent failure)
TM-->>TC: (no error returned)
TC-->>Socket: .ok { color: "original string" }
Socket-->>CLI: success payload
CLI-->>Shell: prints OK (false positive for invalid hex)
Last reviewed commit: "Add set-workspace-co..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3591
to
+3618
| guard let colorRaw = v2String(params, "color"), | ||
| !colorRaw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid color", data: nil) | ||
| } | ||
|
|
||
| let color = colorRaw.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| var applied = false | ||
| v2MainSync { | ||
| guard tabManager.tabs.contains(where: { $0.id == workspaceId }) else { return } | ||
| tabManager.setTabColor(tabId: workspaceId, color: color) | ||
| applied = true | ||
| } | ||
|
|
||
| guard applied else { | ||
| return .err(code: "not_found", message: "Workspace not found", data: [ | ||
| "workspace_id": workspaceId.uuidString, | ||
| "workspace_ref": v2Ref(kind: .workspace, uuid: workspaceId) | ||
| ]) | ||
| } | ||
|
|
||
| let windowId = v2ResolveWindowId(tabManager: tabManager) | ||
| return .ok([ | ||
| "workspace_id": workspaceId.uuidString, | ||
| "workspace_ref": v2Ref(kind: .workspace, uuid: workspaceId), | ||
| "window_id": v2OrNull(windowId?.uuidString), | ||
| "window_ref": v2Ref(kind: .window, uuid: windowId), | ||
| "color": color | ||
| ]) |
Contributor
There was a problem hiding this comment.
v2WorkspaceSetColor only checks that the color string is non-empty, but setCustomColor internally calls WorkspaceTabColorSettings.normalizedHex, which returns nil for any malformed input (wrong length, non-hex characters, etc.). When normalizedHex returns nil, customColor is set to nil — effectively the same as clearing the color — yet the response returns "color": color with the original invalid string, giving the caller a false success signal.
Additionally, even for a valid hex input, the response echoes back the raw (non-normalized) string (e.g. "#c0392b") while the persisted value is the uppercased form "#C0392B" produced by normalizedHex.
The fix is to validate and normalize via normalizedHex before calling setTabColor, returning an error on invalid input and the normalized value on success:
guard let colorRaw = v2String(params, "color"),
!colorRaw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {
return .err(code: "invalid_params", message: "Missing or invalid color", data: nil)
}
let normalized = WorkspaceTabColorSettings.normalizedHex(colorRaw)
guard let color = normalized else {
return .err(code: "invalid_params",
message: "Invalid hex color '\(colorRaw)'. Expected a 6-digit hex string e.g. \"#C0392B\"",
data: nil)
}
// ... (use `color` — already normalized — when calling setTabColor and in the response)
Author
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 11116-11117: Update the top-level usage strings for the commands
set-workspace-color and clear-workspace-color to reflect that
resolveWorkspaceId(...) accepts index handles as well as id and ref; change the
advertised parameter from "--workspace <id|ref>" to include the index variant
(e.g., "--workspace <id|ref|index>" or "--workspace <handle>") so the usage
matches actual behavior of resolveWorkspaceId and avoid confusing users.
- Around line 1948-1968: The set-workspace-color handler currently only checks
for a non-empty color string (color/colorArgs) and clear-workspace-color ignores
trailing args; update both handlers to strictly validate and reject extra
tokens: after parsing with parseOption (wsArg, rem0) ensure rem0 contains
exactly one argument (or one token after an optional "--") and validate that the
resulting color matches a strict hex pattern (e.g. required leading "#" followed
by 6 or 8 hex digits) before calling resolveWorkspaceId and
client.sendV2("workspace.set_color", params) or
client.sendV2("workspace.clear_color", params); on validation failure throw a
CLIError with a clear message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1b2252e-49ff-4063-b655-cc6dafd8fe4d
📒 Files selected for processing (2)
CLI/cmux.swiftSources/TerminalController.swift
- CLI: validate hex format (#RRGGBB or #RRGGBBAA), reject extra args - Server: validate via normalizedHex before applying, return error for invalid hex - Fix top-level usage to show --workspace <id|ref|index> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use normalizedHex result directly so the response echoes the persisted uppercased form (#C0392B) rather than the raw input (#c0392b). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 3597-3599: The error message returned when color parsing fails
should mention that both 6- and 8-digit hex are accepted; update the message in
the guard that calls WorkspaceTabColorSettings.normalizedHex (the block that
returns .err(code: "invalid_params", message: ... , data: nil)) to include
acceptance of either "#RRGGBB" or "#RRGGBBAA" (e.g. change "expected `#RRGGBB`" to
"expected `#RRGGBB` or `#RRGGBBAA`"), keeping the rest of the returned payload
identical and still referencing the trimmed input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 178b6a56-bb8c-4749-b39c-8ea55f3dfdde
📒 Files selected for processing (1)
Sources/TerminalController.swift
Comment on lines
+3597
to
+3599
| guard let color = WorkspaceTabColorSettings.normalizedHex(trimmed) else { | ||
| return .err(code: "invalid_params", message: "Invalid hex color \"\(trimmed)\" (expected #RRGGBB)", data: nil) | ||
| } |
There was a problem hiding this comment.
Update the invalid-color error message to include alpha hex format.
The message currently says only #RRGGBB, but this command contract allows #RRGGBBAA as well; clients get incorrect remediation guidance.
💡 Proposed fix
- guard let color = WorkspaceTabColorSettings.normalizedHex(trimmed) else {
- return .err(code: "invalid_params", message: "Invalid hex color \"\(trimmed)\" (expected `#RRGGBB`)", data: nil)
+ guard let color = WorkspaceTabColorSettings.normalizedHex(trimmed) else {
+ return .err(
+ code: "invalid_params",
+ message: "Invalid hex color \"\(trimmed)\" (expected `#RRGGBB` or `#RRGGBBAA`)",
+ 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 3597 - 3599, The error message
returned when color parsing fails should mention that both 6- and 8-digit hex
are accepted; update the message in the guard that calls
WorkspaceTabColorSettings.normalizedHex (the block that returns .err(code:
"invalid_params", message: ... , data: nil)) to include acceptance of either
"#RRGGBB" or "#RRGGBBAA" (e.g. change "expected `#RRGGBB`" to "expected `#RRGGBB` or
`#RRGGBBAA`"), keeping the rest of the returned payload identical and still
referencing the trimmed input.
Author
There was a problem hiding this comment.
Not applicable — normalizedHex only accepts 6-digit hex (body.count == 6 in TabManager.swift:367). The error message correctly says #RRGGBB only.
There was a problem hiding this comment.
@thunter009 Agreed—no change needed. WorkspaceTabColorSettings.normalizedHex(...) only accepts 6‑digit hex (#RRGGBB), and the CLI validation is aligned to that, so the current error text is correct. I’ll avoid suggesting #RRGGBBAA for this path going forward.
✏️ Learnings added
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: Repo: manaflow-ai/cmux — Color validation: WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) accept only 6‑digit `#RRGGBB` (no alpha). CLI/cmux.swift also enforces `#RRGGBB`. Error/help text for workspace color should reference only `#RRGGBB`, not `#RRGGBBAA`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:01:33.134Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, sendPickedElementToTerminal(workspaceId:summary:) resolves the target workspace via AppDelegate.shared?.workspaceFor(tabId: workspaceId), which searches across all mainWindowContexts (not self.tabManager which is active-window only). It first injects text into the focused terminal panel if it's a terminal, then falls back to iterating all panels in the workspace.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — TabManager.setTabColor(tabId:color:) normalizes hex values on write, ensuring the reported "color" and persisted value remain consistent; extra normalization in the caller is redundant.
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/TerminalController.swift:3180-3193
Timestamp: 2026-03-09T02:08:14.574Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceClearTags(params:) must only clear all tags when the "source" key is absent. If "source" is present but blank or non-string (v2String(...) returns nil), the API should return invalid_params. Current implementation uses hasSourceKey = params.keys.contains("source") and guards with if hasSourceKey && source == nil { return .err(...)}.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:12:42.553Z
Learning: Repo: manaflow-ai/cmux — In Sources/Panels/BrowserPanel.swift, BrowserPanel.updateWorkspaceId(_:) is a dedicated method (var workspaceId) that updates both BrowserPanel.workspaceId and pickerMessageHandler?.updateWorkspaceId(_:) atomically. It is called from BrowserPanel.reattachToWorkspace(_:) when a panel moves between workspaces. This ensures BrowserPickerMessageHandler always posts notifications with the current workspaceId, not a stale one from panel initialization.
Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:4012-4023
Timestamp: 2026-03-16T08:05:16.048Z
Learning: In Sources/TerminalController.swift, implement v2SurfaceSplitSized(params:) to validate the ratio parameter as follows: if ratio is present, it must be numeric (NSNumber/Double-coercible) and strictly 0 < ratio < 1; if not, return invalid_params. If ratio is absent, use the default 0.6. This enforces the rule that a present-but-invalid parameter should yield invalid_params rather than silently falling back to defaults.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
normalizedHex only accepts 6-digit hex; CLI was also accepting 8-digit which would pass client-side but fail server-side. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (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:1959">
P2: `set-workspace-color` regressed by rejecting previously accepted 8-digit hex colors (`#RRGGBBAA`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+1959
to
+1960
| guard body.count == 6, UInt64(body, radix: 16) != nil else { | ||
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB)") |
There was a problem hiding this comment.
P2: set-workspace-color regressed by rejecting previously accepted 8-digit hex colors (#RRGGBBAA).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/cmux.swift, line 1959:
<comment>`set-workspace-color` regressed by rejecting previously accepted 8-digit hex colors (`#RRGGBBAA`).</comment>
<file context>
@@ -1956,8 +1956,8 @@ struct CMUXCLI {
let body = color.hasPrefix("#") ? String(color.dropFirst()) : color
- guard body.count == 6 || body.count == 8, UInt64(body.prefix(8), radix: 16) != nil else {
- throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB or #RRGGBBAA)")
+ guard body.count == 6, UInt64(body, radix: 16) != nil else {
+ throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB)")
}
</file context>
Suggested change
| guard body.count == 6, UInt64(body, radix: 16) != nil else { | |
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB)") | |
| guard (body.count == 6 || body.count == 8), UInt64(body, radix: 16) != nil else { | |
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB or #RRGGBBAA)") | |
| } |
Author
There was a problem hiding this comment.
Not a regression — normalizedHex (TabManager.swift:367) only accepts 6-digit hex (body.count == 6). The previous CLI code accepting 8-digit would pass CLI validation but fail server-side. Aligning CLI to 6-digit only is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/TerminalController.swift (1)
3607-3676: Extract the shared workspace-color mutation path.
workspace.set_color/workspace.clear_colornow duplicate logic that already exists underworkspace.action, so validation, payload shape, and future fixes can drift. A small helper for workspace lookup +setTabColor+ payload assembly would keep the dedicated RPCs hex-only while centralizing the mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3607 - 3676, Both v2WorkspaceSetColor and v2WorkspaceClearColor duplicate workspace lookup, mutation and response assembly that exists under workspace.action; extract a shared helper (e.g. v2ApplyWorkspaceColor(workspaceId: UUID, colorHex: String?) -> V2CallResult or -> (success: Bool, payload: [String: Any])) that: validates/normalizes hex using WorkspaceTabColorSettings.normalizedHex for non-nil colors, uses v2ResolveTabManager/v2UUID and v2MainSync to check tabManager.tabs and call tabManager.setTabColor(tabId:workspaceId, color: colorOrNil), and builds the standard payload using v2ResolveWindowId, v2Ref and v2OrNull; then replace v2WorkspaceSetColor and v2WorkspaceClearColor to call this helper (Set passes normalized hex, Clear passes nil) to centralize mutation, validation and response assembly.
🤖 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 1958-1960: The current validation accepts bare RRGGBB; change it
to require an explicit leading '#' by first checking color.hasPrefix("#") and
throwing the existing CLIError if missing, then validate that
String(color.dropFirst()) has exactly 6 characters and parses as hex (e.g., via
UInt64(..., radix:16)); keep using the same CLIError message for invalid format
and reference the same "color" variable and the guard/throw code path so callers
of set-workspace-color only accept inputs matching "#RRGGBB".
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 3607-3676: Both v2WorkspaceSetColor and v2WorkspaceClearColor
duplicate workspace lookup, mutation and response assembly that exists under
workspace.action; extract a shared helper (e.g.
v2ApplyWorkspaceColor(workspaceId: UUID, colorHex: String?) -> V2CallResult or
-> (success: Bool, payload: [String: Any])) that: validates/normalizes hex using
WorkspaceTabColorSettings.normalizedHex for non-nil colors, uses
v2ResolveTabManager/v2UUID and v2MainSync to check tabManager.tabs and call
tabManager.setTabColor(tabId:workspaceId, color: colorOrNil), and builds the
standard payload using v2ResolveWindowId, v2Ref and v2OrNull; then replace
v2WorkspaceSetColor and v2WorkspaceClearColor to call this helper (Set passes
normalized hex, Clear passes nil) to centralize mutation, validation and
response assembly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0064dbf8-c1c2-458c-b5aa-56f3c034654d
📒 Files selected for processing (2)
CLI/cmux.swiftSources/TerminalController.swift
Comment on lines
+1958
to
+1960
| let body = color.hasPrefix("#") ? String(color.dropFirst()) : color | ||
| guard body.count == 6, UInt64(body, radix: 16) != nil else { | ||
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB)") |
There was a problem hiding this comment.
Require explicit # prefix for color input.
set-workspace-color currently accepts bare RRGGBB (Line 1958), but the command contract is documented as #RRGGBB. Tighten validation so only # + 6 hex digits is accepted.
Suggested fix
- let body = color.hasPrefix("#") ? String(color.dropFirst()) : color
- guard body.count == 6, UInt64(body, radix: 16) != nil else {
+ guard color.hasPrefix("#") else {
+ throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected `#RRGGBB`)")
+ }
+ let body = String(color.dropFirst())
+ guard body.count == 6, UInt64(body, radix: 16) != nil else {
throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected `#RRGGBB`)")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let body = color.hasPrefix("#") ? String(color.dropFirst()) : color | |
| guard body.count == 6, UInt64(body, radix: 16) != nil else { | |
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected #RRGGBB)") | |
| let (wsArg, rem0) = parseOption(commandArgs, name: "--workspace") | |
| let workspaceArg = wsArg ?? (windowId == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil) | |
| let colorArgs = rem0.dropFirst(rem0.first == "--" ? 1 : 0) | |
| let color = colorArgs.joined(separator: " ").trimmingCharacters(in: .whitespacesAndNewlines) | |
| guard color.hasPrefix("#") else { | |
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected `#RRGGBB`)") | |
| } | |
| let body = String(color.dropFirst()) | |
| guard body.count == 6, UInt64(body, radix: 16) != nil else { | |
| throw CLIError(message: "set-workspace-color: invalid hex color \"\(color)\" (expected `#RRGGBB`)") | |
| } | |
| let wsId = try resolveWorkspaceId(workspaceArg, client: client) | |
| let params: [String: Any] = ["workspace_id": wsId, "color": color] | |
| let payload = try client.sendV2(method: "workspace.set_color", params: params) | |
| printV2Payload(payload, jsonOutput: jsonOutput, idFormat: idFormat, fallbackText: v2OKSummary(payload, idFormat: idFormat, kinds: ["workspace"])) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1958 - 1960, The current validation accepts bare
RRGGBB; change it to require an explicit leading '#' by first checking
color.hasPrefix("#") and throwing the existing CLIError if missing, then
validate that String(color.dropFirst()) has exactly 6 characters and parses as
hex (e.g., via UInt64(..., radix:16)); keep using the same CLIError message for
invalid format and reference the same "color" variable and the guard/throw code
path so callers of set-workspace-color only accept inputs matching "#RRGGBB".
Author
There was a problem hiding this comment.
Intentional — CLI is lenient (accepts both #RRGGBB and bare RRGGBB) for ergonomics, matching normalizedHex which strips the # prefix internally.
There was a problem hiding this comment.
@thunter009 Thanks for the clarification — acknowledged. I’ll withdraw my earlier suggestion. Accepting both “#RRGGBB” and “RRGGBB” in the CLI is consistent with the server-side normalization flow we’ve noted before (normalizedHex/TabManager normalization), so the current implementation looks good.
Optional follow-ups for clarity only:
- Subcommand help/usage and PR summary could mention “#RRGGBB or RRGGBB” so users aren’t surprised that the bare form also works.
- If you prefer, the invalid input message could say “expected 6 hex digits (RRGGBB, optional ‘#’)” — purely ergonomic.
Want me to open a small follow-up to tweak the help text/message, or leave as-is?
✏️ Learnings added
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: CLI/cmux.swift:1958-1960
Timestamp: 2026-03-25T00:32:58.190Z
Learning: Repo: manaflow-ai/cmux — CLI/cmux.swift set-workspace-color intentionally accepts both "#RRGGBB" and "RRGGBB" for ergonomics, matching server-side WorkspaceTabColorSettings.normalizedHex/TabManager normalization. Do not suggest enforcing a mandatory leading '#'; at most, suggest clarifying help/usage text.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:12:42.553Z
Learning: Repo: manaflow-ai/cmux — In Sources/Panels/BrowserPanel.swift, BrowserPanel.updateWorkspaceId(_:) is a dedicated method (var workspaceId) that updates both BrowserPanel.workspaceId and pickerMessageHandler?.updateWorkspaceId(_:) atomically. It is called from BrowserPanel.reattachToWorkspace(_:) when a panel moves between workspaces. This ensures BrowserPickerMessageHandler always posts notifications with the current workspaceId, not a stale one from panel initialization.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — TabManager.setTabColor(tabId:color:) normalizes hex values on write, ensuring the reported "color" and persisted value remain consistent; extra normalization in the caller is redundant.
Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:01:33.134Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, sendPickedElementToTerminal(workspaceId:summary:) resolves the target workspace via AppDelegate.shared?.workspaceFor(tabId: workspaceId), which searches across all mainWindowContexts (not self.tabManager which is active-window only). It first injects text into the focused terminal panel if it's a terminal, then falls back to iterating all panels in the workspace.
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/TerminalController.swift:3180-3193
Timestamp: 2026-03-09T02:08:14.574Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceClearTags(params:) must only clear all tags when the "source" key is absent. If "source" is present but blank or non-string (v2String(...) returns nil), the API should return invalid_params. Current implementation uses hasSourceKey = params.keys.contains("source") and guards with if hasSourceKey && source == nil { return .err(...)}.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:13.558Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, socket v2 methods that accept surface_id/panel_id must handle cross-window routing. For panel background state mutations, v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:) first try v2ResolveTabManager(params:), but only accept it if that manager actually owns the panelId; otherwise they fall back to AppDelegate.shared?.locateSurface(surfaceId:) to find the correct TabManager across windows. This pattern should be used for any panel_id-only routes to avoid active-window bias.
Learnt from: outoftime
Repo: manaflow-ai/cmux PR: 1528
File: Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:541-546
Timestamp: 2026-03-17T13:59:10.665Z
Learning: Repo: manaflow-ai/cmux — The socket command `report_git_branch` (parsed in Sources/TerminalController.swift) expects the branch name as an **unquoted, bare token**. Wrapping the branch name in double quotes causes it to be silently discarded by the parser. This matches the bash/zsh shell integration convention. Do not suggest quoting the branch argument for this command; the fix was attempted in commit 829437c7 and immediately reverted because it broke branch reporting.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:4012-4023
Timestamp: 2026-03-16T08:05:21.899Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2SurfaceSplitSized(params:) must validate "ratio" as follows: if the "ratio" key is present, it must be numeric (NSNumber/Double-coercible) and strictly 0 < ratio < 1, otherwise return invalid_params; when "ratio" is absent, use default 0.6. This mirrors the general pattern that a present-but-invalid param should yield invalid_params rather than falling back.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T08:16:21.950Z
Learning: Applies to **/*.swift : Socket/CLI commands must not steal macOS app focus (no app activation/window raising side effects). Only explicit focus-intent commands may mutate in-app focus/selection (`window.focus`, `workspace.select/next/previous/last`, `surface.focus`, `pane.focus/last`, browser focus commands, and v1 focus equivalents).
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: CLI/cmux.swift:462-499
Timestamp: 2026-03-09T02:08:51.778Z
Learning: For Claude Code session tag extraction in CLI/cmux.swift, in ClaudeHookTagExtractor.extractTags(subtitle:body:), pre-redact sensitive spans (UUIDs, emails, access tokens, filesystem paths, ENV_VAR=..., long numerics) across the combined body+subtitle using unanchored sensitiveSpanPatterns before tokenization. Then tokenize and still filter each token with anchored sensitivePatterns. Rationale: prevents PII/path fragments from slipping into searchable tags after delimiter splitting.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Contributor
|
Thanks for the contribution! This has been addressed by #1873 which was merged. |
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
set-workspace-color <hex>CLI command to programmatically set workspace tab colorsclear-workspace-colorCLI command to remove custom tab colorsworkspace.set_color/workspace.clear_color)Uses the existing
tabManager.setTabColor()andWorkspace.setCustomColor()— no model changes needed.Closes #1041
Motivation
Tab color is available via the right-click context menu but not through the CLI or socket API. This blocks programmatic workflows like auto-coloring workspaces by project directory from
.zshrc:Usage
Test plan
cmux set-workspace-color "#C0392B"sets the current workspace tab colorcmux set-workspace-color --workspace workspace:2 "#2D1B69"sets a specific workspace's colorcmux clear-workspace-colorremoves the custom color🤖 Generated with Claude Code
Summary by cubic
Add CLI commands to set and clear workspace tab colors and expose matching socket API, with strict 6‑digit hex validation and normalized responses for reliable scripting. Enables scripts to color‑code workspaces.
cmux set-workspace-color [--workspace <id|ref|index>] <hex>andcmux clear-workspace-color [--workspace <id|ref|index>], defaulting to current workspace or$CMUX_WORKSPACE_ID.#RRGGBB) and rejects extra args; server validates vianormalizedHexand applies only valid colors.workspace.set_colorandworkspace.clear_color;set_colorresponse echoes the persisted, normalized uppercase hex.--workspace <id|ref|index>.Written for commit db7aa46. Summary will update on new commits.
Summary by CodeRabbit