Repository navigation
Make plus button actions configurable - #3348
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches new‑workspace handling from command-based to action-based routing, adds configurable right‑click context menus, extends CmuxConfig parsing/resolution and executor semantics, updates UI hit-testing/titlebar right‑click wiring, expands docs/localizations, and adds unit tests and project registration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Titlebar/Sidebar Control
participant App as AppDelegate
participant Store as CmuxConfigStore
participant Exec as CmuxConfigExecutor
participant TabM as TabManager
User->>UI: right-click / click new-workspace
UI->>App: rightClickAction(event, view) / click
App->>Store: request resolvedNewWorkspaceContextMenuItems / resolvedNewWorkspaceAction(actionID)
Store-->>App: resolved menu items / resolved action (or nil + issue)
App->>UI: present NSMenu at event location
User->>UI: select menu item
UI->>App: invoke boxed (windowId, action)
App->>TabM: resolve preferred window / swap tab manager if needed
App->>Exec: executeConfiguredCmuxAction(action, preferredWindow, onExecuted)
Exec->>Exec: run builtin/command/terminal/browser (may use TabManager)
Exec-->>App: onExecuted(success/failure)
App->>UI: call onExecuted only on success (or beep on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
12331-12333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the focused pane CWD here.
These lines now derive
baseCwdfromselectedWorkspace.currentDirectoryonly. That drops the focused-pane override (panelDirectories[focusedPanelId]/terminalPanel.requestedWorkingDirectory), so plus-button command/agent/workspaceCommand actions can execute in the wrong directory whenever the focused pane hascd’d away from the workspace root. Please reuse the same base-CWD resolution contract as the existing configured-action path instead of re-deriving it here.Based on learnings:
executeConfiguredAction(_:) derives baseCwd via configuredActionBaseCwd(), prioritizing the focused pane’s directory (workspace.panelDirectories[focusedPanelId] or terminalPanel.requestedWorkingDirectory), then workspace.currentDirectory, then the user’s home dir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12331 - 12333, The code sets baseCwd from context.tabManager.selectedWorkspace?.currentDirectory (rawCwd) and falls back to homeDirectory, but this ignores the focused-pane override; change this to reuse the same resolution used by executeConfiguredAction(_:), i.e. call configuredActionBaseCwd() (which already checks panelDirectories[focusedPanelId], terminalPanel.requestedWorkingDirectory, then workspace.currentDirectory, then home) instead of re-deriving baseCwd from rawCwd so plus-button command/agent/workspaceCommand actions run in the focused pane’s CWD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 12331-12333: The code sets baseCwd from
context.tabManager.selectedWorkspace?.currentDirectory (rawCwd) and falls back
to homeDirectory, but this ignores the focused-pane override; change this to
reuse the same resolution used by executeConfiguredAction(_:), i.e. call
configuredActionBaseCwd() (which already checks
panelDirectories[focusedPanelId], terminalPanel.requestedWorkingDirectory, then
workspace.currentDirectory, then home) instead of re-deriving baseCwd from
rawCwd so plus-button command/agent/workspaceCommand actions run in the focused
pane’s CWD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc73e37-a100-4a04-bf1c-0fd2588769ee
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/ContentView.swiftSources/Update/MinimalModeSidebarControls.swiftSources/Update/UpdateTitlebarAccessory.swiftcmuxTests/CmuxConfigTests.swift
Greptile SummaryThis PR makes the plus-button action and right-click menu fully configurable through the cmux action registry. The implementation is well-structured: the resolution pipeline correctly deduplicates consecutive separators, trims leading/trailing separators, and maps the old Confidence Score: 4/5Safe to merge; all findings are P2 (style/best-practice). No runtime errors or data-loss paths identified. Only P2 findings: silent drop of unresolved context-menu action IDs (inconsistent with newWorkspace.action surfacing), a non-exhaustive icon-type switch in menuImage, and a temporary global tabManager swap in the .newBrowser case that could be fragile under reentrancy. No P0/P1 issues found. Sources/CmuxConfig.swift (resolvedConfigContextMenuItems silent-drop behavior) and Sources/AppDelegate.swift (menuImage exhaustiveness, tabManager swap in .newBrowser). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Plus button left-click"] --> B["executeConfiguredNewWorkspaceActionIfAvailable"]
C["Plus button right-click"] --> D["showNewWorkspaceContextMenu"]
B --> E{"resolvedNewWorkspaceActionCache set?"}
E -- No --> F["Default new workspace behavior"]
E -- Yes --> G["executeConfiguredCmuxAction(action)"]
G --> H{action.action}
H --> I[".builtIn(newTerminal) → tabManager.newSurface()"]
H --> J[".builtIn(newBrowser) → openBrowserAndFocusAddressBar()"]
H --> K[".builtIn(splitRight/Down) → performSplitShortcut()"]
H --> L[".command/.agent/.workspaceCommand → CmuxConfigExecutor.execute()"]
D --> M{"newWorkspaceContextMenuItems empty?"}
M -- Yes --> N["return false"]
M -- No --> O["Build NSMenu from resolved items"]
O --> P["NSMenu.popUpContextMenu"]
P --> Q["User picks item → performNewWorkspaceContextMenuItem"]
Q --> G
subgraph Resolution["Config resolution (loadAll)"]
R["resolvedConfiguredNewWorkspaceAction"] --> S{"actionID set?"}
S -- Yes --> T{"action in registry?"}
T -- No --> U["CmuxConfigIssue.newWorkspaceActionNotFound"]
T -- Yes --> V{"workspaceCommandName?"}
V -- Yes --> W["resolvedConfiguredNewWorkspaceCommand"]
V -- No --> X["cache action only"]
S -- No --> Y{"commandName set?"}
Y -- Yes --> Z["wrap in synthetic CmuxResolvedConfigAction"]
Y -- No --> AA["no-op"]
R2["resolvedConfigContextMenuItems"] --> BB["For each config item"]
BB --> CC{".separator?"}
CC -- Yes --> DD["skip leading/consecutive; append"]
CC -- No --> EE{"action in registry?"}
EE -- No --> FF["NSLog only (silent drop)"]
EE -- Yes --> GG["CmuxResolvedConfigMenuAction with overrides"]
end
Reviews (1): Last reviewed commit: "Make plus button actions configurable" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let value = try trimmedString(forKey: key, in: container) else { | ||
| throw DecodingError.keyNotFound( | ||
| key, | ||
| DecodingError.Context( |
There was a problem hiding this comment.
ui.newWorkspace.action
When resolvedConfigContextMenuItems encounters an action ID that doesn't exist in the registry it silently skips the item and only writes to NSLog. By contrast, a missing ui.newWorkspace.action ID is surfaced as a CmuxConfigIssue and shown to the user in the UI. A user who mistypes an action ID in contextMenu will see their menu item quietly disappear with no diagnostic — this is inconsistent with every other config-validation path in this store.
There was a problem hiding this comment.
Fixed by surfacing missing ui.newWorkspace.contextMenu[...] action IDs as config issues and filtering the broken menu item instead of silently dropping it.
— Claude Code
| private func menuImage(for icon: CmuxButtonIcon?) -> NSImage? { | ||
| guard let icon else { return nil } | ||
| guard case .symbol(let symbolName) = icon else { return nil } | ||
| return NSImage(systemSymbolName: symbolName, accessibilityDescription: nil) | ||
| } |
There was a problem hiding this comment.
menuImage silently ignores non-symbol icon types
The helper only handles the .symbol case and returns nil for everything else. If a future CmuxButtonIcon variant is added (e.g. a custom image path), any icon set on a context-menu item using that variant will silently produce no image with no indication to the developer. A default: assertionFailure(…) branch (in #if DEBUG) or an explicit exhaustive check would prevent silent regressions.
There was a problem hiding this comment.
Fixed with an explicit CmuxButtonIcon switch for context-menu icons. Non-SF-symbol icons now assert in DEBUG instead of being silently ignored.
— Claude Code
| preferredWindow: NSWindow? = nil, | ||
| onExecuted: (() -> Void)? = nil | ||
| ) -> Bool { | ||
| switch action.action { | ||
| case .builtIn(let builtIn): | ||
| switch builtIn { | ||
| case .newTerminal: | ||
| tabManager?.newSurface() | ||
| context.tabManager.newSurface() |
There was a problem hiding this comment.
tabManager swap in .newBrowser case
The .newBrowser path mutates self.tabManager (a global AppDelegate property) to context.tabManager, calls openBrowserAndFocusAddressBar, then restores the previous value via defer. If openBrowserAndFocusAddressBar synchronously dispatches any UI callback that reads self.tabManager (e.g. a KVO observer or notification posted during the call), it will observe the temporarily-swapped value. While main-actor serialization prevents true concurrency, reentrant code paths could still be affected. Threading tabManager as an explicit parameter to openBrowserAndFocusAddressBar would be safer.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 295dc8f939
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shouldSuppressSplitShortcutForTransientTerminalFocusState(direction: .right) { | ||
| return true |
There was a problem hiding this comment.
When a plus-button context-menu item resolves to splitRight/splitDown, this path passes an explicit context but still calls shouldSuppressSplitShortcutForTransientTerminalFocusState, which reads the global tabManager instead of context.tabManager. In multi-window sessions, if the global manager is from a different window, the split can be suppressed (or focus reconciliation can run) based on the wrong workspace, so the configured action may no-op even though the clicked window is valid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by passing the clicked window's TabManager into the split suppression check, so context-menu split actions validate against the target workspace.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3839f655cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let action = actions[item.action] else { | ||
| NSLog("[CmuxConfig] %@ action '%@' does not match any loaded action", settingName, item.action) | ||
| continue | ||
| } | ||
| resolvedItems.append( |
There was a problem hiding this comment.
resolvedConfigContextMenuItems currently accepts any action ID that exists in the action registry, but it does not validate workspaceCommand actions against the loaded command set. In the case where ui.newWorkspace.contextMenu references an action whose workspaceCommand is missing (or resolves to a non-workspace command), the item is still shown, yet selecting it silently does nothing because CmuxConfigExecutor.execute(action:...) bails out on guard command.workspace != nil. This creates user-visible broken menu entries; these items should be filtered or flagged during resolution, similar to other workspace-command validation paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by validating workspaceCommand actions while resolving the plus menu. Missing or non-workspace commands now surface config issues and are filtered before display.
— Claude Code
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 `@web/app/`[locale]/docs/custom-commands/page.tsx:
- Around line 175-177: Replace the unsupported object separator entry in the
"contextMenu" example—currently shown as { "type": "separator" }—with the
supported string separator syntax (e.g., "-" or "separator") so the
"contextMenu" array uses string entries for separators; update the snippet where
"contextMenu" is defined to show the separator as a plain string instead of an
object.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52414f4a-d44e-45fd-af31-4c2a3823cdac
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (21)
web/app/[locale]/docs/custom-commands/page.tsxweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
✅ Files skipped from review due to trivial changes (20)
- web/messages/zh-TW.json
- web/messages/it.json
- web/messages/no.json
- web/messages/fr.json
- web/messages/zh-CN.json
- web/messages/ru.json
- web/messages/bs.json
- web/messages/en.json
- web/messages/km.json
- web/messages/ja.json
- web/messages/uk.json
- web/messages/ar.json
- web/messages/th.json
- web/messages/de.json
- web/messages/pt-BR.json
- web/messages/pl.json
- web/messages/es.json
- web/messages/da.json
- web/messages/tr.json
- web/messages/ko.json
| "contextMenu": [ | ||
| { "action": "codex-worktree", "title": "Codex Worktree" }, | ||
| { "type": "separator" }, |
There was a problem hiding this comment.
Use the supported separator syntax in the example.
The docs snippet currently shows { "type": "separator" }, but the config shape documented by this PR expects separators as string entries ("-" or "separator"). As written, this can lead readers to copy an unsupported form.
Suggested fix
"contextMenu": [
{ "action": "codex-worktree", "title": "Codex Worktree" },
- { "type": "separator" },
+ "separator",
{ "action": "cmux.newTerminal", "title": "New Terminal" },
{ "action": "cmux.newBrowser", "title": "New Browser" }
]📝 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.
| "contextMenu": [ | |
| { "action": "codex-worktree", "title": "Codex Worktree" }, | |
| { "type": "separator" }, | |
| "contextMenu": [ | |
| { "action": "codex-worktree", "title": "Codex Worktree" }, | |
| "separator", | |
| { "action": "cmux.newTerminal", "title": "New Terminal" }, | |
| { "action": "cmux.newBrowser", "title": "New Browser" } | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/app/`[locale]/docs/custom-commands/page.tsx around lines 175 - 177,
Replace the unsupported object separator entry in the "contextMenu"
example—currently shown as { "type": "separator" }—with the supported string
separator syntax (e.g., "-" or "separator") so the "contextMenu" array uses
string entries for separators; update the snippet where "contextMenu" is defined
to show the separator as a plain string instead of an object.
There was a problem hiding this comment.
Leaving this example as-is: the decoder supports object separators as well as string separators, and the docs intentionally show the object form.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen, understood — thanks for the clarification! I'll note this for future reviews.
(ᵔ◡ᵔ)づ
🧠 Learnings used
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2475
File: CLI/cmux.swift:1710-1712
Timestamp: 2026-04-03T07:19:50.892Z
Learning: Repo manaflow-ai/cmux — Server path: TerminalController workspace.create applies Workspace.setCustomDescription(_:) which trims/normalizes and treats whitespace-only descriptions as nil. Therefore, CLI new-workspace can safely forward raw --description; whitespace-only input will not persist. No client-side error is required for this case.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2475
File: Sources/ContentView.swift:12775-12779
Timestamp: 2026-04-03T07:19:43.478Z
Learning: Repo: manaflow-ai/cmux — Sources/Workspace.swift — Workspace.setCustomDescription(_:) calls normalizedCustomDescription(...) which converts CRLF/CR to "\n", trims whitespace/newlines, and returns nil for empty/whitespace-only input. Therefore UI sites (e.g., TabItemView in Sources/ContentView.swift) can rely on workspace.customDescription being nil when blank and should not add extra trimmed-string checks before rendering.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3247
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-29T01:08:44.497Z
Learning: Repo: manaflow-ai/cmux — Behavior contract: Sidebar “Copy Workspace ID(s)” (e.g., in Sources/ContentView.swift TabItemView context menu) must copy plain UUIDs (IDs-only), not refs. Command palette identifier-copy commands should explicitly pass includeRefs: true when refs are desired. This split preserves backward compatibility for scripts while enabling richer payloads in the palette.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:776-777
Timestamp: 2026-04-03T03:35:40.100Z
Learning: Repo: manaflow-ai/cmux — As of PR `#2528`, workspace digit mapping (Cmd+1–9 and sidebar digits) intentionally uses TabManager.tabs as the canonical ordering for both sidebar rendering and app shortcut handling. Do not suggest switching to orderedSidebarWorkspaceIds() unless the model changes to a sidebar-ordered, group/collapse-aware source of truth.
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: rodchristiansen
Repo: manaflow-ai/cmux PR: 2646
File: Sources/CmuxConfigExecutor.swift:107-118
Timestamp: 2026-04-14T19:56:42.696Z
Learning: Repo: manaflow-ai/cmux — In Sources/CmuxConfigExecutor.swift, executeWorkspaceCommand for wsDef.target == .current intentionally uses closePanel(panelId, force: true) before calling applyCustomLayout. This is by design: soft-closing (force: false) leaves detached panes that applyCustomLayout cannot reconcile. If a non-destructive autoApply variant is needed in the future, it should be opted into explicitly via a separate flag rather than changing the default force: true behavior.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3084
File: Sources/ContentView.swift:7635-7645
Timestamp: 2026-04-23T09:52:22.298Z
Learning: Repo: manaflow-ai/cmux — In Sources/ContentView.swift, executeConfiguredAction(_:) derives baseCwd via configuredActionBaseCwd(), prioritizing the focused pane’s directory (workspace.panelDirectories[focusedPanelId] or terminalPanel.requestedWorkingDirectory), then workspace.currentDirectory, then the user’s home dir. This ensures configured palette actions run in the focused pane’s CWD.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2918-3021
Timestamp: 2026-04-14T20:00:16.490Z
Learning: Repo: manaflow-ai/cmux — Accessibility convention: For toolbar actions in macOS 26+ SwiftUI .toolbar (Sources/ContentView.swift), use localized .accessibilityLabel (and identifiers) for VoiceOver; AppKit NSToolbarItems in Sources/WindowToolbarController.swift set localized label/toolTip/accessibilityDescription. Label(...).labelStyle(.iconOnly) is not required here as VO picks up the localized accessibilityLabel.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2564
File: Sources/Workspace.swift:1072-1075
Timestamp: 2026-04-04T02:21:20.453Z
Learning: Repo: manaflow-ai/cmux — Foreground-auth deferral: In CLI/cmux.swift, PermitLocalCommand and LocalCommand are injected only into the foreground startup SSH command (e.g., via deferredRemoteReconnectLocalCommand(...)) and are not added to the ssh_options payload sent in workspace.remote.configure. Consequently, WorkspaceRemoteSSHBatchCommandBuilder.batchArguments(...) and WorkspaceRemoteSessionController.backgroundSSHOptions(_:) do not need to filter LocalCommand/PermitLocalCommand for batch operations.
Learnt from: pgbezerra
Repo: manaflow-ai/cmux PR: 3307
File: Sources/cmuxApp.swift:6413-6417
Timestamp: 2026-04-30T11:55:33.046Z
Learning: Repo: manaflow-ai/cmux — Settings search/navigation: When adding a new Settings section (e.g., Workspaces in Sources/cmuxApp.swift), do not add a raw anchor string alone. Instead, register a real SettingsNavigationTarget case (e.g., .workspaces) with localized title, symbol, search text, and aliases, add the corresponding entry in SettingsSearchIndex so the sidebar/search can navigate, and then apply .settingsSearchAnchor(SettingsSearchIndex.sectionID(for: .workspaces)) on the section header. This holistic wiring avoids broken jump-to behavior.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-05T21:26:10.710Z
Learning: Repo: manaflow-ai/cmux — In Sources/Workspace.swift, applySessionPanelMetadata() must gate listeningPorts restoration on the per-panel snapshot.terminal?.isRemoteBacked flag (not workspace-wide remoteTerminalStartupCommand()), so that local panels are always eligible for port restore regardless of current SSH state. Fixed in commit 4d0fd871 (PR `#2545`).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:58.703Z
Learning: Repo: manaflow-ai/cmux — KeyboardShortcutSettings.setShortcut(...) is a no-op when KeyboardShortcutSettings.isManagedBySettingsFile(action) returns true (i.e., the action is managed by settings.json). This prevents backfilling overrides into UserDefaults and preserves the settings file as the source of truth.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3139
File: Sources/Panels/FilePreviewPanel.swift:534-537
Timestamp: 2026-04-28T05:45:34.719Z
Learning: Repo: manaflow-ai/cmux — Sources/Workspace.swift & Sources/Panels/FilePreviewPanel.swift (PR `#3139`): The shared close-confirmation gate in Workspace uses a generic `panel.isDirty` check (line ~8703) that applies to all panel types, including FilePreviewPanel. FilePreviewPanel exposes `Published private(set) var isDirty` and installFilePreviewPanelSubscription keeps the Bonsplit tab's isDirty flag in sync with it. All three teardown paths (tab close, pane collapse, workspace close) go through this gate, so no special-casing is needed for FilePreviewPanel's dirty state on teardown.
…on-overrides # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52ec18e4f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let workspaceName = wsDef.name ?? command.name | ||
| let restart = command.restart ?? .ignore | ||
|
|
||
| if let existing = tabManager.tabs.first(where: { $0.customTitle == workspaceName }) { | ||
| switch restart { | ||
| case .ignore: | ||
| tabManager.selectWorkspace(existing) | ||
| return | ||
| case .recreate: | ||
| tabManager.closeWorkspace(existing) | ||
| case .confirm: | ||
| let alert = NSAlert() | ||
| alert.messageText = String( | ||
| localized: "dialog.cmuxConfig.confirmRestart.title", | ||
| defaultValue: "Workspace Already Exists" | ||
| ) | ||
| alert.informativeText = String( | ||
| localized: "dialog.cmuxConfig.confirmRestart.message", | ||
| defaultValue: "A workspace with this name already exists. Close it and create a new one?" | ||
| ) | ||
| alert.alertStyle = .warning | ||
| alert.addButton(withTitle: String(localized: "dialog.cmuxConfig.confirmRestart.recreate", defaultValue: "Recreate")) | ||
| alert.addButton(withTitle: String(localized: "dialog.cmuxConfig.confirmRestart.cancel", defaultValue: "Cancel")) | ||
| guard alert.runModal() == .alertFirstButtonReturn else { | ||
| tabManager.selectWorkspace(existing) | ||
| return | ||
| } | ||
| tabManager.closeWorkspace(existing) | ||
| } | ||
| } | ||
|
|
||
| let resolvedCwd = CmuxConfigStore.resolveCwd(wsDef.cwd, relativeTo: baseCwd) | ||
| let newWorkspace = tabManager.addWorkspace(workingDirectory: resolvedCwd) | ||
| newWorkspace.setCustomTitle(workspaceName) |
There was a problem hiding this comment.
executeWorkspaceCommand now always creates a new workspace with the target name, but it no longer checks command.restart or existing tabs first. As a result, commands configured with restart: "ignore" or restart: "confirm" silently lose their documented behavior and will create duplicate workspaces instead of reusing/confirming, which is a regression for any existing workspace-command automation that relied on those policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by adding restart: new as the default while preserving explicit ignore, recreate, and confirm behavior for workspace commands.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
web/messages/uk.json (1)
314-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the new
customCommandssubsection.This entire block is still in English, so the new action-system / plus-button docs are effectively untranslated in a high-confidence locale. Please localize these strings; the English-only
docs.dock.agentPromptexception does not apply here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/messages/uk.json` around lines 314 - 355, The new "customCommands" subsection strings (e.g., "newWorkspaceButton", "newWorkspaceButtonDesc", "newWorkspaceWorktreeNote", "plusButtonScreenshotCaption", "plusButtonScreenshotAlt", and related keys in this diff) are still in English—translate these values into Ukrainian to match the locale; replace the English text with Ukrainian equivalents while preserving any embedded tags like <action>, <contextMenu>, <rightClick>, <separator>, and inline code/IDs (e.g., ui.newWorkspace.action, worktree-agents) exactly as-is so markup and IDs remain unchanged.web/messages/ja.json (1)
314-359:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the new
customCommandssubsection.This new action-system / plus-button docs block is still English, which leaves the Japanese locale partially untranslated. Please localize these strings; the English-only
docs.dock.agentPromptexception does not apply here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/messages/ja.json` around lines 314 - 359, The review notes that several new localization keys remain in English and must be translated to Japanese; update the Japanese locale entries for "newWorkspaceButton", "newWorkspaceButtonDesc", "newWorkspaceWorktreeNote" and any adjacent English strings in this block (e.g., plusButtonScreenshotCaption, plusButtonScreenshotAlt, newWorkspaceButtonDesc) by replacing the English copy with natural, concise Japanese translations that preserve inline tags like <action>, <contextMenu>, <rightClick>, <commands>, <separator>, <worktree>, <action> and code samples exactly as-is (do not remove or alter tags), and ensure terminology and tone match existing translations elsewhere in the file.web/messages/zh-TW.json (2)
314-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the file-location callout copy.
fallbackLocal*,nightlyFeatureCallout,trustCallout, andschemaErrorCalloutare still English in a zh-TW locale, so this page will render mixed-language copy. Please localize those strings while keeping the technical identifiers unchanged. Based on learnings: high-confidence locales inweb/messages/*.jsonare expected to contain fully translated copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/messages/zh-TW.json` around lines 314 - 318, The strings for keys "fallbackLocal", "fallbackLocalDesc", "nightlyFeatureCallout", "trustCallout", and "schemaErrorCallout" are still in English for the zh-TW locale; update their values to accurate Traditional Chinese translations (preserving the technical inline tags like <actions>, <shortcut>, <buttons>, and <title> exactly as they appear) so the file no longer contains mixed-language copy—ensure "fallbackLocal" and "fallbackLocalDesc" convey the fallback/local support meaning, "nightlyFeatureCallout" indicates the nightly feature and installation instruction, "trustCallout" explains project-local action trust behavior and fingerprint locking, and "schemaErrorCallout" describes the cmux.json schema error fallback and the Command Palette row that opens the config file.
325-331:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new action-registry docs block.
schemaIntro,nightlyActionRegistry*,paletteDesc,iconsDesc,buttonEntriesDesc, andpermissionFlagsDescare still English. These are user-facing docs in a high-confidence locale, so they should be translated instead of shipping mixed-language prose. Based on learnings: high-confidence locales inweb/messages/*.jsonare expected to contain fully translated copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/messages/zh-TW.json` around lines 325 - 331, Translate the seven English strings into Traditional Chinese: schemaIntro, nightlyActionRegistry, nightlyActionRegistryDesc, paletteDesc, iconsDesc, buttonEntriesDesc, and permissionFlagsDesc; replace the English text with accurate zh-TW copy while preserving all inline tags/markup (e.g., <commands>, <actions>, <newTerminal>cmux.newTerminal</newTerminal>), code tokens/IDs (cmux.newTerminal, cmux.newBrowser, etc.), example shortcuts, JSON/object examples, emoji and scale values, and punctuation/escaping exactly as-is so only the user-facing prose is localized and all technical tokens remain unchanged.Sources/AppDelegate.swift (1)
12447-12463:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the focused pane CWD for configured plus-button actions.
This path now resolves
baseCwdfromselectedWorkspace.currentDirectoryonly, so command/agent/workspaceCommand overrides launched from the plus button or its context menu will run in the wrong directory whenever the focused terminal has diverged from the workspace root. Please reuse the existing focused-pane CWD precedence here instead of introducing a second resolver.Suggested fix
case .command, .agent, .workspaceCommand: guard let cmuxConfigStore = context.cmuxConfigStore else { return false } - let rawCwd = context.tabManager.selectedWorkspace?.currentDirectory + let workspace = context.tabManager.selectedWorkspace + let focusedPanelId = workspace?.focusedPanelId + let focusedTerminal = focusedPanelId.flatMap { workspace?.terminalPanel(for: $0) } + let rawCwd = + focusedPanelId.flatMap { workspace?.panelDirectories[$0] } + ?? focusedTerminal?.requestedWorkingDirectory + ?? workspace?.currentDirectory let baseCwd = (rawCwd?.isEmpty == false) ? rawCwd! : FileManager.default.homeDirectoryForCurrentUser.path return CmuxConfigExecutor.execute( action: action, commands: cmuxConfigStore.loadedCommands,Based on learnings:
Sources/ContentView.swift::executeConfiguredAction(_:)intentionally resolvesbaseCwdfrom the focused pane first, thenworkspace.currentDirectory, then the home directory, and shared behavior should stay on one action/model path across entrypoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12447 - 12463, The current branch in AppDelegate is deriving baseCwd only from selectedWorkspace.currentDirectory (or home) which ignores the focused pane override; update the baseCwd resolution in the .command/.agent/.workspaceCommand case to reuse the same focused-pane-first resolver used by ContentView.executeConfiguredAction(_:): first try the focused pane CWD (via the tabManager's focused/selected tab and active pane currentDirectory), then fallback to selectedWorkspace.currentDirectory, then to FileManager.default.homeDirectoryForCurrentUser.path, and pass that value as baseCwd into CmuxConfigExecutor.execute; reference context.cmuxConfigStore, context.tabManager, and the executeConfiguredAction logic to match behavior.
🤖 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/AppDelegate.swift`:
- Around line 6096-6104: The handler currently only beeps on lookup failures but
ignores failures from executeConfiguredCmuxAction; update
performNewWorkspaceContextMenuItem to capture the Bool result of
executeConfiguredCmuxAction(box.action, context: context, preferredWindow:
window) and if it returns false, call NSSound.beep() (and optionally log a
warning) so the user is notified of the failure; reference the method
performNewWorkspaceContextMenuItem and the executeConfiguredCmuxAction(...) call
when making the change.
In `@Sources/CmuxConfig.swift`:
- Around line 1662-1668: CmuxResolvedConfigMenuAction currently drops the
originating config path metadata needed by CmuxButtonIcon.imagePath, breaking
per-item image overrides for ui.newWorkspace.contextMenu; restore and propagate
the source path by adding a configSource (or similar) field to
CmuxResolvedConfigMenuAction and wire it through wherever these actions are
created so image resolution and project-local trust checks (used by
CmuxButtonIcon.imagePath and surfaceTabBarButtons) can access the original
config path; ensure constructors/creators that build
CmuxResolvedConfigMenuAction and any code that consumes it (context menu
assembly, command palette/context menu factories) populate and use the new
field.
- Around line 2659-2666: The menu construction is forwarding
item.title/action.title and tooltip verbatim, allowing invisible control
characters through; update the resolvedItems.append call that creates
CmuxResolvedConfigMenuAction so title and tooltip use sanitizeConfigText(_:),
e.g. pass sanitizeConfigText(item.title ?? action.title) and
sanitizeConfigText(item.tooltip ?? action.tooltip) (and similarly wrap any other
text fields like icon if applicable) to ensure zero-width and bidi controls are
stripped before the menu UI is materialized.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2916-2924: The off-main path currently uses a fire-and-forget Task
{ `@MainActor` in ... } which allows callers to proceed before
AppDelegate.reloadCmuxConfigStores(source:) finishes; change the off-main branch
to synchronously hop to the MainActor and await the call (use await
MainActor.run { AppDelegate.shared?.reloadCmuxConfigStores(source: source) } or
equivalent) so that when Thread.isMainThread is false the method does not return
until reloadCmuxConfigStores completes, preserving reload sequencing.
In `@web/messages/es.json`:
- Around line 353-354: The Spanish locale file contains English fallback text
for the keys "newWorkspaceButton" and "newWorkspaceButtonDesc"; update es.json
by providing proper Spanish translations for these two keys (newWorkspaceButton
and newWorkspaceButtonDesc), preserving any markup/placeholder tokens like
<action>, <contextMenu>, <rightClick>, and <separator> exactly as in the diff so
existing rendering and action IDs continue to work.
In `@web/messages/it.json`:
- Around line 353-354: The Italian locale entries "newWorkspaceButton" and
"newWorkspaceButtonDesc" are still in English; replace their English values in
web/messages/it.json with Italian translations preserving the JSON keys
"newWorkspaceButton" and "newWorkspaceButtonDesc" and any embedded markup tags
(<action>, <contextMenu>, <rightClick>, <separator>) exactly as-is; ensure the
translated "newWorkspaceButtonDesc" conveys the same meaning and keeps the
example JSON fragment intact and properly escaped.
In `@web/messages/ru.json`:
- Around line 353-354: Translate the two strings for keys newWorkspaceButton and
newWorkspaceButtonDesc into Russian keeping all XML-like placeholders and JSON
separator token intact (preserve <action>, <contextMenu>, <rightClick>,
<separator> and the { "type": "separator" } content exactly as-is); update the
values so newWorkspaceButton is a concise Russian label for "Custom plus button
actions" and newWorkspaceButtonDesc is a fluent Russian sentence describing how
to override the plus button and define the right-click menu while preserving the
exact placeholder tags and examples.
In `@web/messages/zh-CN.json`:
- Around line 353-354: The zh-CN locale file has two untranslated keys
("newWorkspaceButton" and "newWorkspaceButtonDesc") left in English; replace
their values with appropriate Simplified Chinese translations preserving any
inline tags (<action>, <contextMenu>, <rightClick>, <separator>) and JSON string
formatting so keys "newWorkspaceButton" and "newWorkspaceButtonDesc" retain the
same structure but contain localized Chinese copy that explains the same actions
and examples.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 12447-12463: The current branch in AppDelegate is deriving baseCwd
only from selectedWorkspace.currentDirectory (or home) which ignores the focused
pane override; update the baseCwd resolution in the
.command/.agent/.workspaceCommand case to reuse the same focused-pane-first
resolver used by ContentView.executeConfiguredAction(_:): first try the focused
pane CWD (via the tabManager's focused/selected tab and active pane
currentDirectory), then fallback to selectedWorkspace.currentDirectory, then to
FileManager.default.homeDirectoryForCurrentUser.path, and pass that value as
baseCwd into CmuxConfigExecutor.execute; reference context.cmuxConfigStore,
context.tabManager, and the executeConfiguredAction logic to match behavior.
In `@web/messages/ja.json`:
- Around line 314-359: The review notes that several new localization keys
remain in English and must be translated to Japanese; update the Japanese locale
entries for "newWorkspaceButton", "newWorkspaceButtonDesc",
"newWorkspaceWorktreeNote" and any adjacent English strings in this block (e.g.,
plusButtonScreenshotCaption, plusButtonScreenshotAlt, newWorkspaceButtonDesc) by
replacing the English copy with natural, concise Japanese translations that
preserve inline tags like <action>, <contextMenu>, <rightClick>, <commands>,
<separator>, <worktree>, <action> and code samples exactly as-is (do not remove
or alter tags), and ensure terminology and tone match existing translations
elsewhere in the file.
In `@web/messages/uk.json`:
- Around line 314-355: The new "customCommands" subsection strings (e.g.,
"newWorkspaceButton", "newWorkspaceButtonDesc", "newWorkspaceWorktreeNote",
"plusButtonScreenshotCaption", "plusButtonScreenshotAlt", and related keys in
this diff) are still in English—translate these values into Ukrainian to match
the locale; replace the English text with Ukrainian equivalents while preserving
any embedded tags like <action>, <contextMenu>, <rightClick>, <separator>, and
inline code/IDs (e.g., ui.newWorkspace.action, worktree-agents) exactly as-is so
markup and IDs remain unchanged.
In `@web/messages/zh-TW.json`:
- Around line 314-318: The strings for keys "fallbackLocal",
"fallbackLocalDesc", "nightlyFeatureCallout", "trustCallout", and
"schemaErrorCallout" are still in English for the zh-TW locale; update their
values to accurate Traditional Chinese translations (preserving the technical
inline tags like <actions>, <shortcut>, <buttons>, and <title> exactly as they
appear) so the file no longer contains mixed-language copy—ensure
"fallbackLocal" and "fallbackLocalDesc" convey the fallback/local support
meaning, "nightlyFeatureCallout" indicates the nightly feature and installation
instruction, "trustCallout" explains project-local action trust behavior and
fingerprint locking, and "schemaErrorCallout" describes the cmux.json schema
error fallback and the Command Palette row that opens the config file.
- Around line 325-331: Translate the seven English strings into Traditional
Chinese: schemaIntro, nightlyActionRegistry, nightlyActionRegistryDesc,
paletteDesc, iconsDesc, buttonEntriesDesc, and permissionFlagsDesc; replace the
English text with accurate zh-TW copy while preserving all inline tags/markup
(e.g., <commands>, <actions>, <newTerminal>cmux.newTerminal</newTerminal>), code
tokens/IDs (cmux.newTerminal, cmux.newBrowser, etc.), example shortcuts,
JSON/object examples, emoji and scale values, and punctuation/escaping exactly
as-is so only the user-facing prose is localized and all technical tokens remain
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 932ee5c5-4c4e-4e01-b072-14557cf55aee
⛔ Files ignored due to path filters (3)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsvweb/public/docs/custom-actions-command-palette.pngis excluded by!**/*.pngweb/public/docs/custom-actions-plus-menu.pngis excluded by!**/*.png
📒 Files selected for processing (31)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftcmuxTests/CmuxConfigTests.swiftcmuxTests/SidebarIdentifierFormattingTests.swiftscripts/launch-tagged-automation.shweb/app/[locale]/docs/custom-commands/page.tsxweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
💤 Files with no reviewable changes (1)
- Sources/CmuxConfigExecutor.swift
✅ Files skipped from review due to trivial changes (4)
- Resources/Localizable.xcstrings
- scripts/launch-tagged-automation.sh
- GhosttyTabs.xcodeproj/project.pbxproj
- web/messages/ko.json
🚧 Files skipped from review as they are similar to previous changes (12)
- web/app/[locale]/docs/custom-commands/page.tsx
- web/messages/fr.json
- web/messages/da.json
- web/messages/pl.json
- web/messages/pt-BR.json
- web/messages/th.json
- Sources/ContentView.swift
- cmuxTests/CmuxConfigTests.swift
- web/messages/de.json
- web/messages/km.json
- web/messages/no.json
- web/messages/tr.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a88d8ac634
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return executeConfiguredCmuxAction( | ||
| action, | ||
| context: context, | ||
| preferredWindow: window, | ||
| onExecuted: onExecuted |
There was a problem hiding this comment.
This method returns the raw success result of executeConfiguredCmuxAction, so any runtime failure (for example cmux.newBrowser when browser surfaces are disabled, or an action targeting a missing terminal) is interpreted upstream as “no configured action” and falls back to creating a default workspace. That means users can get an unexpected extra workspace even though ui.newWorkspace.action was explicitly configured; once a valid action is resolved, this path should be treated as handled even if execution fails.
Useful? React with 👍 / 👎.
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 (1)
Sources/AppDelegate.swift (1)
12464-12480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve
baseCwdfrom the focused pane, not justworkspace.currentDirectory.Configured plus-button
command,agent, andworkspaceCommandactions will run in the wrong directory whenever the focused pane has a pane-specific cwd/requested working directory that differs fromcurrentDirectory.Based on learnings: `executeConfiguredAction(_:)` derives baseCwd via configuredActionBaseCwd(), prioritizing the focused pane’s directory (`workspace.panelDirectories[focusedPanelId]` or `terminalPanel.requestedWorkingDirectory`), then `workspace.currentDirectory`, then the user’s home dir.💡 Suggested fix
- let rawCwd = context.tabManager.selectedWorkspace?.currentDirectory - let baseCwd = (rawCwd?.isEmpty == false) ? rawCwd! - : FileManager.default.homeDirectoryForCurrentUser.path + let workspace = context.tabManager.selectedWorkspace + let focusedPanelId = workspace?.focusedPanelId + let focusedPaneCwd = focusedPanelId.flatMap { panelId in + workspace?.panelDirectories[panelId] + ?? workspace?.terminalPanel(for: panelId)?.requestedWorkingDirectory + } + let homeCwd = FileManager.default.homeDirectoryForCurrentUser.path + let baseCwd = [focusedPaneCwd, workspace?.currentDirectory] + .compactMap { value in + guard let value, !value.isEmpty else { return nil } + return value + } + .first ?? homeCwd return CmuxConfigExecutor.execute( action: action, commands: cmuxConfigStore.loadedCommands,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12464 - 12480, The code currently computes baseCwd from context.tabManager.selectedWorkspace?.currentDirectory which ignores a focused pane's pane-specific cwd; update this block to use the same resolution logic as configuredActionBaseCwd()/executeConfiguredAction(_:): first check the focused pane directory (workspace.panelDirectories[focusedPanelId] and the terminalPanel.requestedWorkingDirectory for the focused panel), then fallback to workspace.currentDirectory, and finally FileManager.default.homeDirectoryForCurrentUser.path; implement this by retrieving the focusedPanelId and terminalPanel (via context.tabManager.selectedWorkspace?.panelDirectories and context.tabManager.selectedWorkspace?.terminalPanels or equivalent APIs used elsewhere) and using those values to set baseCwd before calling CmuxConfigExecutor.execute so the action runs in the pane-focused directory when present.
♻️ Duplicate comments (3)
Sources/CmuxConfig.swift (2)
2701-2708:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize resolved menu labels and tooltips.
This path still forwards
item.title/ inheritedaction.titleandtooltipverbatim. Invisible BiDi/zero-width controls therefore survive into the context menu, which contradicts the new stripping behavior and lets labels/tooltips render misleading text.Suggested fix
resolvedItems.append( .action( CmuxResolvedConfigMenuAction( id: "\(settingName).\(index).\(action.id)", - title: item.title ?? action.title, + title: sanitizeConfigText(item.title ?? action.title), icon: item.icon ?? action.icon, - tooltip: item.tooltip ?? action.tooltip, + tooltip: (item.tooltip ?? action.tooltip).map(sanitizeConfigText), action: action ) ) )Based on learnings: In Sources/TerminalController.swift and Sources/Panels/BrowserPanel.swift, BiDi override (U+202A–202E, U+2066–2069) and zero-width char (U+200B–200F, U+FEFF) filtering is implemented via a shared dangerousScalars pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 2701 - 2708, The title and tooltip passed into CmuxResolvedConfigMenuAction are not being sanitized and may contain BiDi/zero-width characters; before constructing the action in resolvedItems.append, normalize the label and tooltip by filtering out the dangerousScalars (the shared pattern used elsewhere, e.g. in TerminalController/BrowserPanel) and use the sanitized strings for title: (item.title ?? action.title) and tooltip: (item.tooltip ?? action.tooltip) so the menu receives stripped values; apply the same sanitization whenever you resolve item.title/action.title or item.tooltip/action.tooltip prior to creating CmuxResolvedConfigMenuAction.
1662-1668:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve source-path metadata on resolved menu actions.
CmuxResolvedConfigMenuActioncan carry an overriddenicon, but it has no matching source-path field. That breaksui.newWorkspace.contextMenuimage overrides: relative.imagePathvalues no longer resolve against the config file, and project-local trust/lock handling can be skipped because the icon now looks “global”.Based on learnings: When a behavior is exposed through multiple entrypoints (keyboard shortcut, command palette, context menu, CLI, settings, debug menu), implement one shared action/model path and verify every entrypoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 1662 - 1668, CmuxResolvedConfigMenuAction currently lacks source-path metadata so overridden icons (CmuxButtonIcon.imagePath) lose their config-relative resolution; add a sourcePath: String? (or similarly named source metadata) field to CmuxResolvedConfigMenuAction, populate it when you create resolved menu actions from the config (i.e. when mapping from the source CmuxResolvedConfigAction), and thread that sourcePath through the image resolution/trust/lock logic used by ui.newWorkspace.contextMenu so relative imagePath values are resolved against the config file and project-local trust checks are preserved.Sources/AppDelegate.swift (1)
6096-6104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBeep when a context-menu action returns
false.Line 6103 only beeps on lookup failures. If the action resolves and then fails to execute, the click is silently dropped.
💡 Suggested fix
- _ = executeConfiguredCmuxAction(box.action, context: context, preferredWindow: window) + guard executeConfiguredCmuxAction(box.action, context: context, preferredWindow: window) else { + NSSound.beep() + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6096 - 6104, The handler performNewWorkspaceContextMenuItem currently only beeps when lookups fail but ignores the boolean result of executeConfiguredCmuxAction; update it to capture the return value of executeConfiguredCmuxAction(box.action, context: context, preferredWindow: window) and call NSSound.beep() when that call returns false so failed action executions produce an audible cue; reference NewWorkspaceContextMenuActionBox, mainWindowContexts, resolvedWindow(for:), and executeConfiguredCmuxAction to locate and change the logic.
🧹 Nitpick comments (2)
Sources/AppDelegate.swift (1)
6047-6104: ⚡ Quick winLog the new plus-menu mouse actions in DEBUG.
This adds a new right-click/menu-execution path, but neither the popup nor the selection handler emits a unified debug-event log entry. Add
cmuxDebugLog(...)around menu show/selection so these mouse/tab actions stay traceable in the existing DEBUG logs.As per coding guidelines: "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds. Use the free function
cmuxDebugLog("message")to log with timestamp and append to file in real time. Wrap all call sites in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6047 - 6104, Add DEBUG-only unified debug logging for right-click/menu actions by calling cmuxDebugLog(...) when the menu is popped in showNewWorkspaceContextMenu (include context.windowId and a short description of the displayed items or event.debugSource) and again in performNewWorkspaceContextMenuItem when a menu item is chosen (log windowId, action id/name from NewWorkspaceContextMenuActionBox and any tooltip/icon identifier). Wrap both calls in `#if` DEBUG / `#endif` blocks, placing the show log just before NSMenu.popUpContextMenu(...) in showNewWorkspaceContextMenu and the selection log at the start of performNewWorkspaceContextMenuItem after unwrapping the box/context/window so logs correlate with executed executeConfiguredCmuxAction.cmuxTests/CmuxConfigTests.swift (1)
554-599: ⚡ Quick winAdd coverage for the
"-"and"separator"separator aliases.This test only exercises
{ "type": "separator" }, butCmuxConfigContextMenuItemalso accepts the two string shorthands. A regression in either alias would slip through even though they’re part of the supported config surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxConfigTests.swift` around lines 554 - 599, Update the test testDecodeNewWorkspaceContextMenuPreservesOrder to also exercise the two shorthand separator aliases by adding context-menu entries that are the strings "-" and "separator" (in addition to the existing { "type": "separator" } object), decode using the existing decode(json) helper, and assert those entries decode to CmuxConfigContextMenuItem.separator (e.g., check menu items at the appropriate indexes are .separator); this ensures the parsing of string shorthands for separators is covered alongside the object form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxConfigTests.swift`:
- Around line 1829-1853: The test
testWorkspaceCommandAlwaysCreatesNewWorkspaceWhenNameAlreadyExists currently
asserts selection by title which can be ambiguous; change the final assertion to
verify the selected workspace is the newly created tab by ID: after calling
CmuxConfigExecutor.execute and confirming manager.tabs.count == 2, find the
newly created workspace (e.g., the tab whose id != existingWorkspace.id or by
index) and assert manager.selectedWorkspace?.id == newWorkspace.id so selection
is validated by unique identifier rather than customTitle.
In `@Sources/AppDelegate.swift`:
- Around line 6118-6127: In menuImage(for:) do not call assertionFailure for
.emoji or .imagePath since they are valid CmuxButtonIcon cases; instead remove
the assertion (or replace it with a non-fatal diagnostic such as a debug log)
and just return nil so the UI falls back gracefully. Locate the menuImage(for:)
function and the switch handling CmuxButtonIcon (cases .symbol, .emoji,
.imagePath) and change the .emoji/.imagePath branch to a soft-fail path (no
assertion) and optionally emit a debug-only log message indicating unsupported
icon type so validation can be handled earlier during config resolution.
In `@web/messages/pt-BR.json`:
- Around line 353-355: Update the pt-BR localized strings to mention the two
string shorthand separator forms accepted by the config parser: add that the
menu separator can be specified as "-" or "separator" in the keys that explain
contextMenu syntax (specifically update "newWorkspaceButtonDesc" and/or
"newWorkspaceWorktreeNote" to include the shorthand forms in the description
alongside the existing { "type": "separator" } example so the documentation
matches parser behavior).
In `@web/messages/zh-TW.json`:
- Around line 353-355: The zh-Hant documentation strings for the new workspace
button are missing the alternative separator shorthands; update the relevant
localization entries (e.g., "newWorkspaceButtonDesc" and/or adjacent
"newWorkspaceWorktreeNote") to explicitly state that the context menu accepts
the separator as an object { "type": "separator" } as well as the string forms
"-" and "separator", mirroring the parser behavior so the zh-Hant copy matches
en and other high-confidence locales.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 12464-12480: The code currently computes baseCwd from
context.tabManager.selectedWorkspace?.currentDirectory which ignores a focused
pane's pane-specific cwd; update this block to use the same resolution logic as
configuredActionBaseCwd()/executeConfiguredAction(_:): first check the focused
pane directory (workspace.panelDirectories[focusedPanelId] and the
terminalPanel.requestedWorkingDirectory for the focused panel), then fallback to
workspace.currentDirectory, and finally
FileManager.default.homeDirectoryForCurrentUser.path; implement this by
retrieving the focusedPanelId and terminalPanel (via
context.tabManager.selectedWorkspace?.panelDirectories and
context.tabManager.selectedWorkspace?.terminalPanels or equivalent APIs used
elsewhere) and using those values to set baseCwd before calling
CmuxConfigExecutor.execute so the action runs in the pane-focused directory when
present.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 6096-6104: The handler performNewWorkspaceContextMenuItem
currently only beeps when lookups fail but ignores the boolean result of
executeConfiguredCmuxAction; update it to capture the return value of
executeConfiguredCmuxAction(box.action, context: context, preferredWindow:
window) and call NSSound.beep() when that call returns false so failed action
executions produce an audible cue; reference NewWorkspaceContextMenuActionBox,
mainWindowContexts, resolvedWindow(for:), and executeConfiguredCmuxAction to
locate and change the logic.
In `@Sources/CmuxConfig.swift`:
- Around line 2701-2708: The title and tooltip passed into
CmuxResolvedConfigMenuAction are not being sanitized and may contain
BiDi/zero-width characters; before constructing the action in
resolvedItems.append, normalize the label and tooltip by filtering out the
dangerousScalars (the shared pattern used elsewhere, e.g. in
TerminalController/BrowserPanel) and use the sanitized strings for title:
(item.title ?? action.title) and tooltip: (item.tooltip ?? action.tooltip) so
the menu receives stripped values; apply the same sanitization whenever you
resolve item.title/action.title or item.tooltip/action.tooltip prior to creating
CmuxResolvedConfigMenuAction.
- Around line 1662-1668: CmuxResolvedConfigMenuAction currently lacks
source-path metadata so overridden icons (CmuxButtonIcon.imagePath) lose their
config-relative resolution; add a sourcePath: String? (or similarly named source
metadata) field to CmuxResolvedConfigMenuAction, populate it when you create
resolved menu actions from the config (i.e. when mapping from the source
CmuxResolvedConfigAction), and thread that sourcePath through the image
resolution/trust/lock logic used by ui.newWorkspace.contextMenu so relative
imagePath values are resolved against the config file and project-local trust
checks are preserved.
---
Nitpick comments:
In `@cmuxTests/CmuxConfigTests.swift`:
- Around line 554-599: Update the test
testDecodeNewWorkspaceContextMenuPreservesOrder to also exercise the two
shorthand separator aliases by adding context-menu entries that are the strings
"-" and "separator" (in addition to the existing { "type": "separator" }
object), decode using the existing decode(json) helper, and assert those entries
decode to CmuxConfigContextMenuItem.separator (e.g., check menu items at the
appropriate indexes are .separator); this ensures the parsing of string
shorthands for separators is covered alongside the object form.
In `@Sources/AppDelegate.swift`:
- Around line 6047-6104: Add DEBUG-only unified debug logging for
right-click/menu actions by calling cmuxDebugLog(...) when the menu is popped in
showNewWorkspaceContextMenu (include context.windowId and a short description of
the displayed items or event.debugSource) and again in
performNewWorkspaceContextMenuItem when a menu item is chosen (log windowId,
action id/name from NewWorkspaceContextMenuActionBox and any tooltip/icon
identifier). Wrap both calls in `#if` DEBUG / `#endif` blocks, placing the show log
just before NSMenu.popUpContextMenu(...) in showNewWorkspaceContextMenu and the
selection log at the start of performNewWorkspaceContextMenuItem after
unwrapping the box/context/window so logs correlate with executed
executeConfiguredCmuxAction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43e9796a-885b-42f2-98f5-2f67e1c7f63f
📒 Files selected for processing (22)
Sources/AppDelegate.swiftSources/CmuxConfig.swiftcmuxTests/CmuxConfigTests.swiftweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
✅ Files skipped from review due to trivial changes (8)
- web/messages/da.json
- web/messages/th.json
- web/messages/no.json
- web/messages/zh-CN.json
- web/messages/bs.json
- web/messages/ja.json
- web/messages/ar.json
- web/messages/ko.json
🚧 Files skipped from review as they are similar to previous changes (6)
- web/messages/de.json
- web/messages/fr.json
- web/messages/tr.json
- web/messages/es.json
- web/messages/km.json
- web/messages/ru.json
| func testWorkspaceCommandAlwaysCreatesNewWorkspaceWhenNameAlreadyExists() { | ||
| for restart in [CmuxRestartBehavior.ignore, .recreate] { | ||
| let manager = TabManager() | ||
| let existingWorkspace = manager.tabs[0] | ||
| existingWorkspace.setCustomTitle("Dev") | ||
|
|
||
| let command = CmuxCommandDefinition( | ||
| name: "Dev command", | ||
| restart: restart, | ||
| workspace: CmuxWorkspaceDefinition(name: "Dev") | ||
| ) | ||
|
|
||
| XCTAssertTrue(CmuxConfigExecutor.execute( | ||
| command: command, | ||
| tabManager: manager, | ||
| baseCwd: NSTemporaryDirectory(), | ||
| configSourcePath: nil, | ||
| globalConfigPath: "/tmp/cmux-test-global-config.json" | ||
| )) | ||
|
|
||
| XCTAssertEqual(manager.tabs.count, 2) | ||
| XCTAssertTrue(manager.tabs.contains(where: { $0.id == existingWorkspace.id })) | ||
| XCTAssertEqual(manager.tabs.filter { $0.customTitle == "Dev" }.count, 2) | ||
| XCTAssertEqual(manager.selectedWorkspace?.customTitle, "Dev") | ||
| } |
There was a problem hiding this comment.
Assert the selected workspace by ID, not just by title.
selectedWorkspace?.customTitle == "Dev" still passes if execution leaves focus on the original workspace, because both tabs share that title after the command runs. Tightening this to the new tab’s ID will actually guard the selection behavior this test is trying to verify.
Proposed assertion tweak
XCTAssertEqual(manager.tabs.count, 2)
XCTAssertTrue(manager.tabs.contains(where: { $0.id == existingWorkspace.id }))
XCTAssertEqual(manager.tabs.filter { $0.customTitle == "Dev" }.count, 2)
- XCTAssertEqual(manager.selectedWorkspace?.customTitle, "Dev")
+ XCTAssertNotEqual(manager.selectedWorkspace?.id, existingWorkspace.id)
+ XCTAssertEqual(manager.selectedWorkspace?.id, manager.tabs.last?.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxConfigTests.swift` around lines 1829 - 1853, The test
testWorkspaceCommandAlwaysCreatesNewWorkspaceWhenNameAlreadyExists currently
asserts selection by title which can be ambiguous; change the final assertion to
verify the selected workspace is the newly created tab by ID: after calling
CmuxConfigExecutor.execute and confirming manager.tabs.count == 2, find the
newly created workspace (e.g., the tab whose id != existingWorkspace.id or by
index) and assert manager.selectedWorkspace?.id == newWorkspace.id so selection
is validated by unique identifier rather than customTitle.
| "newWorkspaceButton": "Ações personalizadas do botão de mais", | ||
| "newWorkspaceButtonDesc": "Use <action>ui.newWorkspace.action</action> para sobrescrever o que o botão de mais faz. Use <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ou o alias <rightClick>rightClick</rightClick>) para definir o menu ordenado do clique direito. Entradas de menu podem ser IDs de ação, objetos de ação ou <separator>{ \"type\": \"separator\" }</separator>.", | ||
| "newWorkspaceWorktreeNote": "Este exemplo faz o clique normal no botão de mais executar a ação <action>worktree-agents</action>. O comando de workspace em <commands>commands</commands> usa um terminal de preparação visível para criar primeiro o <worktree>worktree</worktree> do Git. Codex e Claude iniciam ao mesmo tempo, esperam o arquivo de estado específico do workspace e então entram no diretório criado antes de exec.", |
There was a problem hiding this comment.
Include the "-" and "separator" forms in this localized docs copy.
The text currently documents only { "type": "separator" }, but the config parser also accepts the two string shorthands. Without them, the pt-BR docs under-describe the supported ui.newWorkspace.contextMenu syntax.
Based on learnings: Only the explicitly high-confidence locales—en, de, fr, es, ja, ko, zh-Hans, zh-Hant, pt-BR, ru, it, pl, uk, tr, da, no—are expected to contain fully translated copy for strings changed in a given PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/messages/pt-BR.json` around lines 353 - 355, Update the pt-BR localized
strings to mention the two string shorthand separator forms accepted by the
config parser: add that the menu separator can be specified as "-" or
"separator" in the keys that explain contextMenu syntax (specifically update
"newWorkspaceButtonDesc" and/or "newWorkspaceWorktreeNote" to include the
shorthand forms in the description alongside the existing { "type": "separator"
} example so the documentation matches parser behavior).
| "newWorkspaceButton": "加號按鈕的自訂動作", | ||
| "newWorkspaceButtonDesc": "使用 <action>ui.newWorkspace.action</action> 覆寫加號按鈕的行為。使用 <contextMenu>ui.newWorkspace.contextMenu</contextMenu>（或別名 <rightClick>rightClick</rightClick>）定義有順序的右鍵選單。選單項目可以是動作 ID、動作物件，或 <separator>{ \"type\": \"separator\" }</separator>。", | ||
| "newWorkspaceWorktreeNote": "此範例讓一般加號點擊執行 <action>worktree-agents</action> 動作。來自 <commands>commands</commands> 的工作區命令會先使用可見的設定終端機建立 Git <worktree>worktree</worktree>。Codex 與 Claude 同時啟動，等待該工作區專用的狀態檔，然後在 exec 前進入建立的目錄。", |
There was a problem hiding this comment.
Document the string separator shorthands here.
This copy only mentions { "type": "separator" }, but ui.newWorkspace.contextMenu also accepts "-" and "separator" string entries. Leaving those out makes the zh-Hant docs incomplete versus the parser and the PR behavior.
Based on learnings: Only the explicitly high-confidence locales—en, de, fr, es, ja, ko, zh-Hans, zh-Hant, pt-BR, ru, it, pl, uk, tr, da, no—are expected to contain fully translated copy for strings changed in a given PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/messages/zh-TW.json` around lines 353 - 355, The zh-Hant documentation
strings for the new workspace button are missing the alternative separator
shorthands; update the relevant localization entries (e.g.,
"newWorkspaceButtonDesc" and/or adjacent "newWorkspaceWorktreeNote") to
explicitly state that the context menu accepts the separator as an object {
"type": "separator" } as well as the string forms "-" and "separator", mirroring
the parser behavior so the zh-Hant copy matches en and other high-confidence
locales.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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/CmuxConfigExecutor.swift (1)
424-485:⚠️ Potential issue | 🟠 MajorRestore workspace-target branching in
executeWorkspaceCommand.Lines 424–485 currently always create a new workspace and never check
wsDef.target. The struct definition (Sources/CmuxWorkspaceDefinition.swift) has notargetproperty, and no.current-specific logic exists.If workspace targets (particularly
.currentto apply layout to the current workspace) are still supported, thetargetproperty must be added to the struct, andexecuteWorkspaceCommandmust branch on it. The.currentpath should retain the intentionalclosePanel(panelId, force: true)beforeapplyCustomLayoutto prevent detached-pane reconciliation issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigExecutor.swift` around lines 424 - 485, Add a workspace "target" field to CmuxWorkspaceDefinition and update executeWorkspaceCommand to branch on that target instead of always creating a new workspace: if target is .new (or default) keep the current flow (use CmuxConfigStore.resolveCwd, tabManager.addWorkspace, setCustomTitle/setCustomColor, close any existingWorkspaceToClose, then applyCustomLayout), if target is .current locate the current workspace via tabManager (select or use the active workspace), call closePanel(panelId, force: true) on that workspace where the panel would be reconciled, then call applyCustomLayout on the existing workspace (do not add a new workspace), and if target is .ignore/.recreate/.confirm preserve the existing restart logic (selectWorkspace, closeWorkspace, prompt)—update references to CmuxWorkspaceDefinition, executeWorkspaceCommand, closePanel, applyCustomLayout, tabManager.addWorkspace, tabManager.selectWorkspace, and tabManager.closeWorkspace accordingly.Sources/AppDelegate.swift (1)
12471-12483:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve configured-action
baseCwdfrom the focused pane, not justworkspace.currentDirectory.Line 12471 currently ignores the focused pane's directory, so plus-button
command/agent/workspaceCommandactions can execute in the wrong cwd when the selected split has diverged fromworkspace.currentDirectory.Based on learnings: In
Sources/ContentView.swift,executeConfiguredAction(_:)derivesbaseCwdviaconfiguredActionBaseCwd(), prioritizing the focused pane’s directory (workspace.panelDirectories[focusedPanelId]orterminalPanel.requestedWorkingDirectory), thenworkspace.currentDirectory, then the user’s home dir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12471 - 12483, Replace the current baseCwd resolution (rawCwd / workspace.currentDirectory / home) with the same prioritized logic used in configuredActionBaseCwd(): first use the focused pane’s directory (workspace.panelDirectories[focusedPanelId] or the focused terminalPanel.requestedWorkingDirectory), then fall back to workspace.currentDirectory, then the user’s home directory; update the code that sets baseCwd before calling CmuxConfigExecutor.execute (replace the rawCwd/baseCwd block) or call configuredActionBaseCwd() to obtain the correct baseCwd so actions execute in the focused pane’s working directory.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (1)
6121-6130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't assert on unsupported menu icon kinds here.
.emojiand.imagePathare still validCmuxButtonIconcases, so opening this menu in a DEBUG build can tripassertionFailurefor an otherwise valid config. Soft-fail withnil(or a debug log) instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6121 - 6130, The menuImage(for:) implementation currently triggers assertionFailure for CmuxButtonIcon cases .emoji and .imagePath; remove the assertionFailure call in menuImage(for icon: CmuxButtonIcon?) and instead soft-fail by returning nil (or emitting a debug log) for those cases so valid configs don't crash in DEBUG builds; update the switch in menuImage(for:) to handle .emoji and .imagePath by returning nil (optionally log via NSLog/OSLog or a debug-only print) rather than asserting.Sources/CmuxConfig.swift (1)
1662-1668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve source metadata for per-item context-menu image overrides.
item.iconoverrides lose their config base here becauseCmuxResolvedConfigMenuActiononly stores theCmuxButtonIcon. Relative image paths inui.newWorkspace.contextMenutherefore can’t be resolved or trust-scoped once the menu is materialized, even though inherited action icons still can.Please carry an
iconSourcePath/configSourcePathalongside the resolved menu icon, and set it tosettingSourcePathfor per-item overrides.Also applies to: 2702-2709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 1662 - 1668, CmuxResolvedConfigMenuAction currently drops the icon's origin so per-item overrides can't resolve relative/trust-scoped image paths; add a stored property (e.g., iconSourcePath or configSourcePath: String?) to CmuxResolvedConfigMenuAction to carry the source path alongside the resolved CmuxButtonIcon, update wherever instances are created (including the related menu-action construction around the other block mentioned) to set that property to the original settingSourcePath when an item-level override is used, and thread that source through any consumers that resolve relative icon URLs so inherited icons keep working and per-item icons can be resolved correctly.
🧹 Nitpick comments (2)
Sources/AppDelegate.swift (1)
6047-6094: ⚡ Quick winAdd DEBUG logging for the new plus-button context-menu path.
These handlers introduce new mouse-driven routing, but they never emit
cmuxDebugLog(...), so menu show/selection/failure won't appear in the unified debug event log when this path regresses.As per coding guidelines: "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds."
Also applies to: 6096-6107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6047 - 6094, In showNewWorkspaceContextMenu:add cmuxDebugLog calls so menu lifecycle events are recorded: log when entering/showing the context menu (include debugSource, context.windowId, and configuredItems count), log when early-returning due to no context or empty configuredItems, and log when the menu is popped up or when pop fails; also add corresponding cmuxDebugLog emissions in the selection handler (performNewWorkspaceContextMenuItem(_:)) to record which NewWorkspaceContextMenuAction (action.id and title) was chosen and any failure handling; use the existing cmuxDebugLog signature/format used elsewhere in the app and reference the same debugSource string (e.g., "titlebar.newWorkspace.contextMenu") so entries join the unified debug event log.cmuxTests/CmuxConfigTests.swift (1)
554-599: ⚡ Quick winAdd coverage for the compatibility spellings.
These tests cover
contextMenuplus object separators, but the PR contract also exposes therightClickalias and string separators ("-"/"separator"). A small decode/resolve case for those inputs would lock down the most compatibility-sensitive part of this config surface.Also applies to: 1014-1242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxConfigTests.swift` around lines 554 - 599, Extend the existing test suite (around testDecodeNewWorkspaceContextMenuPreservesOrder and the decode(json) helper) to add a new test that verifies the compatibility spellings: decode a payload that uses the alias "rightClick" instead of "contextMenu" and uses string separators (e.g., "-" and "separator") as well as object separators, then assert the resulting config.ui?.newWorkspace?.contextMenu (or the rightClick-resolved equivalent) has the correct order and that string separators deserialize to the same .separator enum case and action strings map to .action items; reuse the same pattern of XCTUnwrap, XCTAssertEqual(menu.count,...), and case matching used in testDecodeNewWorkspaceContextMenuPreservesOrder to validate items and icons.
🤖 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/CmuxConfig.swift`:
- Around line 2629-2643: The synthetic CmuxResolvedConfigAction created in the
legacy newWorkspaceCommand fallback is directly using command.command.name and
command.command.description; wrap the user-facing strings through
sanitizeConfigText(_:) before assigning them to title, subtitle (use sanitized
description or sanitize the localized default), and tooltip so zero-width/BiDi
controls are removed; update the CmuxResolvedConfigAction initializer arguments
(title, subtitle, tooltip) to call sanitizeConfigText(command.command.name),
sanitizeConfigText(command.command.description ?? String(localized:
"command.cmuxConfig.subtitle", defaultValue: "cmux.json")), and
sanitizeConfigText(command.command.description) respectively.
In `@web/messages/km.json`:
- Line 354: Update the documentation string for the key newWorkspaceButtonDesc
to use the fully-qualified alias key `ui.newWorkspace.rightClick` instead of
`rightClick`; locate the message value under "newWorkspaceButtonDesc" and
replace the alias text inside the <rightClick>…</rightClick> tag so the example
shows `ui.newWorkspace.rightClick`, keeping the surrounding markup (<action>,
<contextMenu>, <rightClick>, <separator>) intact.
In `@web/messages/no.json`:
- Around line 353-355: Update the localized documentation for ui.newWorkspace in
no.json to fully describe the supported menu syntax: mention that rightClick can
be referenced as ui.newWorkspace.rightClick (the fully qualified alias), and
document the shorthands for separators (both "-" and "separator" or the object {
"type": "separator" }); also ensure newWorkspaceButtonDesc (and any related
strings referencing ui.newWorkspace) explicitly lists that menu entries may be
action IDs, action objects, separators using "-" or "separator", and the fully
qualified rightClick alias so translators capture the complete config surface.
In `@web/messages/uk.json`:
- Around line 323-325: Update the "schemaDesc" Ukrainian string so it reflects
the new config shape: mention that cmux.json can contain both a top-level
"actions" registry and a "commands" array, and adjust any language that
currently implies only "commands"; also ensure "schemaIntro" wording remains
consistent with the new description (references to <actions>actions</actions>
should align with the updated "schemaDesc").
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 12471-12483: Replace the current baseCwd resolution (rawCwd /
workspace.currentDirectory / home) with the same prioritized logic used in
configuredActionBaseCwd(): first use the focused pane’s directory
(workspace.panelDirectories[focusedPanelId] or the focused
terminalPanel.requestedWorkingDirectory), then fall back to
workspace.currentDirectory, then the user’s home directory; update the code that
sets baseCwd before calling CmuxConfigExecutor.execute (replace the
rawCwd/baseCwd block) or call configuredActionBaseCwd() to obtain the correct
baseCwd so actions execute in the focused pane’s working directory.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 424-485: Add a workspace "target" field to CmuxWorkspaceDefinition
and update executeWorkspaceCommand to branch on that target instead of always
creating a new workspace: if target is .new (or default) keep the current flow
(use CmuxConfigStore.resolveCwd, tabManager.addWorkspace,
setCustomTitle/setCustomColor, close any existingWorkspaceToClose, then
applyCustomLayout), if target is .current locate the current workspace via
tabManager (select or use the active workspace), call closePanel(panelId, force:
true) on that workspace where the panel would be reconciled, then call
applyCustomLayout on the existing workspace (do not add a new workspace), and if
target is .ignore/.recreate/.confirm preserve the existing restart logic
(selectWorkspace, closeWorkspace, prompt)—update references to
CmuxWorkspaceDefinition, executeWorkspaceCommand, closePanel, applyCustomLayout,
tabManager.addWorkspace, tabManager.selectWorkspace, and
tabManager.closeWorkspace accordingly.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 6121-6130: The menuImage(for:) implementation currently triggers
assertionFailure for CmuxButtonIcon cases .emoji and .imagePath; remove the
assertionFailure call in menuImage(for icon: CmuxButtonIcon?) and instead
soft-fail by returning nil (or emitting a debug log) for those cases so valid
configs don't crash in DEBUG builds; update the switch in menuImage(for:) to
handle .emoji and .imagePath by returning nil (optionally log via NSLog/OSLog or
a debug-only print) rather than asserting.
In `@Sources/CmuxConfig.swift`:
- Around line 1662-1668: CmuxResolvedConfigMenuAction currently drops the icon's
origin so per-item overrides can't resolve relative/trust-scoped image paths;
add a stored property (e.g., iconSourcePath or configSourcePath: String?) to
CmuxResolvedConfigMenuAction to carry the source path alongside the resolved
CmuxButtonIcon, update wherever instances are created (including the related
menu-action construction around the other block mentioned) to set that property
to the original settingSourcePath when an item-level override is used, and
thread that source through any consumers that resolve relative icon URLs so
inherited icons keep working and per-item icons can be resolved correctly.
---
Nitpick comments:
In `@cmuxTests/CmuxConfigTests.swift`:
- Around line 554-599: Extend the existing test suite (around
testDecodeNewWorkspaceContextMenuPreservesOrder and the decode(json) helper) to
add a new test that verifies the compatibility spellings: decode a payload that
uses the alias "rightClick" instead of "contextMenu" and uses string separators
(e.g., "-" and "separator") as well as object separators, then assert the
resulting config.ui?.newWorkspace?.contextMenu (or the rightClick-resolved
equivalent) has the correct order and that string separators deserialize to the
same .separator enum case and action strings map to .action items; reuse the
same pattern of XCTUnwrap, XCTAssertEqual(menu.count,...), and case matching
used in testDecodeNewWorkspaceContextMenuPreservesOrder to validate items and
icons.
In `@Sources/AppDelegate.swift`:
- Around line 6047-6094: In showNewWorkspaceContextMenu:add cmuxDebugLog calls
so menu lifecycle events are recorded: log when entering/showing the context
menu (include debugSource, context.windowId, and configuredItems count), log
when early-returning due to no context or empty configuredItems, and log when
the menu is popped up or when pop fails; also add corresponding cmuxDebugLog
emissions in the selection handler (performNewWorkspaceContextMenuItem(_:)) to
record which NewWorkspaceContextMenuAction (action.id and title) was chosen and
any failure handling; use the existing cmuxDebugLog signature/format used
elsewhere in the app and reference the same debugSource string (e.g.,
"titlebar.newWorkspace.contextMenu") so entries join the unified debug event
log.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77811b6c-1f21-4aae-b0a5-98bbc905654f
📒 Files selected for processing (26)
Sources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/GhosttyTerminalView.swiftcmuxTests/CmuxConfigTests.swiftweb/app/[locale]/docs/custom-commands/page.tsxweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
✅ Files skipped from review due to trivial changes (4)
- web/messages/en.json
- web/messages/th.json
- web/messages/tr.json
- web/messages/ja.json
🚧 Files skipped from review as they are similar to previous changes (6)
- web/app/[locale]/docs/custom-commands/page.tsx
- web/messages/ar.json
- web/messages/zh-TW.json
- web/messages/bs.json
- web/messages/da.json
- web/messages/ko.json
| "plusButtonScreenshotAlt": "Right-click menu របស់ cmux plus button បង្ហាញ Worktree Agents, New Terminal និង New Browser", | ||
| "plusButtonScreenshotCaption": "Right-click menu ធ្វើតាមលំដាប់ពី ui.newWorkspace.contextMenu។", | ||
| "newWorkspaceButton": "Actions ផ្ទាល់ខ្លួនសម្រាប់ប៊ូតុង plus", | ||
| "newWorkspaceButtonDesc": "ប្រើ <action>ui.newWorkspace.action</action> ដើម្បី override អ្វីដែលប៊ូតុង plus ធ្វើ។ ប្រើ <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ឬ alias <rightClick>rightClick</rightClick>) ដើម្បីកំណត់ right-click menu ដែលមានលំដាប់។ Menu entries អាចជា action IDs, action objects, ឬ <separator>{ \"type\": \"separator\" }</separator>។", |
There was a problem hiding this comment.
Use fully-qualified alias key in docs text
Line 354 currently shows alias as rightClick, but this should be documented as ui.newWorkspace.rightClick to avoid user confusion when copying config keys.
✏️ Proposed fix
- "newWorkspaceButtonDesc": "ប្រើ <action>ui.newWorkspace.action</action> ដើម្បី override អ្វីដែលប៊ូតុង plus ធ្វើ។ ប្រើ <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ឬ alias <rightClick>rightClick</rightClick>) ដើម្បីកំណត់ right-click menu ដែលមានលំដាប់។ Menu entries អាចជា action IDs, action objects, ឬ <separator>{ \"type\": \"separator\" }</separator>។",
+ "newWorkspaceButtonDesc": "ប្រើ <action>ui.newWorkspace.action</action> ដើម្បី override អ្វីដែលប៊ូតុង plus ធ្វើ។ ប្រើ <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ឬ alias <rightClick>ui.newWorkspace.rightClick</rightClick>) ដើម្បីកំណត់ right-click menu ដែលមានលំដាប់។ Menu entries អាចជា action IDs, action objects, ឬ <separator>{ \"type\": \"separator\" }</separator>។",📝 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.
| "newWorkspaceButtonDesc": "ប្រើ <action>ui.newWorkspace.action</action> ដើម្បី override អ្វីដែលប៊ូតុង plus ធ្វើ។ ប្រើ <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ឬ alias <rightClick>rightClick</rightClick>) ដើម្បីកំណត់ right-click menu ដែលមានលំដាប់។ Menu entries អាចជា action IDs, action objects, ឬ <separator>{ \"type\": \"separator\" }</separator>។", | |
| "newWorkspaceButtonDesc": "ប្រើ <action>ui.newWorkspace.action</action> ដើម្បី override អ្វីដែលប៊ូតុង plus ធ្វើ។ ប្រើ <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (ឬ alias <rightClick>ui.newWorkspace.rightClick</rightClick>) ដើម្បីកំណត់ right-click menu ដែលមានលំដាប់។ Menu entries អាចជា action IDs, action objects, ឬ <separator>{ \"type\": \"separator\" }</separator>។", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/messages/km.json` at line 354, Update the documentation string for the
key newWorkspaceButtonDesc to use the fully-qualified alias key
`ui.newWorkspace.rightClick` instead of `rightClick`; locate the message value
under "newWorkspaceButtonDesc" and replace the alias text inside the
<rightClick>…</rightClick> tag so the example shows
`ui.newWorkspace.rightClick`, keeping the surrounding markup (<action>,
<contextMenu>, <rightClick>, <separator>) intact.
| "newWorkspaceButton": "Tilpassede handlinger for plussknappen", | ||
| "newWorkspaceButtonDesc": "Bruk <action>ui.newWorkspace.action</action> for å overstyre hva plussknappen gjør. Bruk <contextMenu>ui.newWorkspace.contextMenu</contextMenu> (eller aliaset <rightClick>rightClick</rightClick>) for å definere den ordnede høyreklikkmenyen. Menyoppføringer kan være handlings-ID-er, handlingsobjekter eller <separator>{ \"type\": \"separator\" }</separator>.", | ||
| "newWorkspaceWorktreeNote": "Dette eksempelet gjør at et vanlig plussklikk kjører handlingen <action>worktree-agents</action>. Arbeidsområdekommandoen fra <commands>commands</commands> bruker en synlig oppsettsterminal til først å opprette Git-<worktree>worktree</worktree>. Codex og Claude starter samtidig, venter på den arbeidsområdespesifikke tilstandsfilen og går deretter inn i den opprettede mappen før exec.", |
There was a problem hiding this comment.
Document the full ui.newWorkspace menu syntax here.
This copy only mentions bare rightClick and { "type": "separator" }. The new menu config also accepts the fully qualified alias ui.newWorkspace.rightClick plus the "-" / "separator" shorthands, so the localized docs still under-describe the supported config for the plus-button menu.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/messages/no.json` around lines 353 - 355, Update the localized
documentation for ui.newWorkspace in no.json to fully describe the supported
menu syntax: mention that rightClick can be referenced as
ui.newWorkspace.rightClick (the fully qualified alias), and document the
shorthands for separators (both "-" and "separator" or the object { "type":
"separator" }); also ensure newWorkspaceButtonDesc (and any related strings
referencing ui.newWorkspace) explicitly lists that menu entries may be action
IDs, action objects, separators using "-" or "separator", and the fully
qualified rightClick alias so translators capture the complete config surface.
| "schema": "Схема", | ||
| "schemaDesc": "Файл cmux.json містить масив commands. Кожна команда — це або проста shell-команда, або повне визначення робочого простору:", | ||
| "schemaIntro": "<commands>commands</commands> still define reusable shell commands and workspace layouts. Nightly builds add an <actions>actions</actions> registry. Actions are the public IDs shared by the surface tab bar, the Command Palette, and action-level shortcuts.", | ||
| "nightlyActionRegistry": "Nightly action registry", | ||
| "nightlyActionRegistryDesc": "<actions>actions</actions> maps stable IDs to runnable behavior. Use the built-in IDs <newTerminal>cmux.newTerminal</newTerminal>, <newBrowser>cmux.newBrowser</newBrowser>, <splitRight>cmux.splitRight</splitRight>, and <splitDown>cmux.splitDown</splitDown> to override the defaults. Use your own IDs for project-specific tools.", | ||
| "paletteDesc": "<palette>palette</palette> defaults to <trueValue>true</trueValue>. Set it to <falseValue>false</falseValue> to keep an action out of Command Palette while still making it available to the surface tab bar or a shortcut. <shortcut>shortcut</shortcut> uses the same syntax as settings shortcuts, for example <singleShortcut>cmd+shift+c</singleShortcut> or <chordShortcut>[\"cmd+k\", \"cmd+c\"]</chordShortcut>.", | ||
| "iconsDesc": "<buttons>ui.surfaceTabBar.buttons</buttons> replaces the default button list when present. Leave out a built-in ID to hide it. Icons always use an object shape: <symbolIcon>{ \"type\": \"symbol\", \"name\": \"play.circle\" }</symbolIcon>, <emojiIcon>{ \"type\": \"emoji\", \"value\": \"🧪\", \"scale\": 0.9 }</emojiIcon>, or <imageIcon>{ \"type\": \"image\", \"path\": \"./icons/codex.svg\" }</imageIcon>. Image paths are relative to the config file. Emoji <scale>scale</scale> is optional and defaults to <defaultScale>1</defaultScale>. SVG, PDF, PNG, JPEG, GIF, TIFF, BMP, HEIC, HEIF, WebP, AVIF, ICO, and ICNS are supported.", | ||
| "buttonEntriesDesc": "Each button entry can be either an action ID string or a button object. Use a button object when you want the same action with a different surface label, icon, or tooltip. The resolved button title is also used as the trust prompt title.", | ||
| "permissionFlagsDesc": "Put any approval or permission flags directly in the command string you actually want to run. The default action target is <target>newTabInCurrentPane</target>, so the common pattern is to open a new terminal tab in the current pane and start Codex, Claude Code, or OpenCode there.", | ||
| "schemaIntro": "<commands>commands</commands> і далі визначає повторно використовувані shell-команди та розкладки workspace. Nightly-збірки додають реєстр <actions>actions</actions>. Дії — це публічні ID, спільні для surface tab bar, Command Palette та скорочень рівня дії.", |
There was a problem hiding this comment.
schemaDesc still describes the pre-actions schema.
This text still says cmux.json contains only a commands array, but the updated section now documents top-level actions too. Please update the Ukrainian copy so it doesn’t contradict the new config shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/messages/uk.json` around lines 323 - 325, Update the "schemaDesc"
Ukrainian string so it reflects the new config shape: mention that cmux.json can
contain both a top-level "actions" registry and a "commands" array, and adjust
any language that currently implies only "commands"; also ensure "schemaIntro"
wording remains consistent with the new description (references to
<actions>actions</actions> should align with the updated "schemaDesc").
…on-overrides # Conflicts: # .github/swift-file-length-budget.tsv
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
2 participants
Summary:
Testing:
Summary by cubic
Makes the titlebar “+” button fully configurable and adds a right‑click menu for quick actions. Works in the titlebar and minimal mode.
ui.newWorkspace.actionresolves via the action registry; supports built‑ins (cmux.newTerminal,cmux.newBrowser,cmux.splitRight,cmux.splitDown), command, agent, andworkspaceCommandactions; falls back tonewWorkspaceCommandwhen set.ui.newWorkspace.contextMenu(alias:ui.newWorkspace.rightClick) accepts action IDs or objects; supports separators via"-","separator", or{ "type": "separator" }, plus per‑itemtitle,icon, andtooltip.icon: { type: "symbol", name: "..." }.ui.newWorkspace.actionreferences a missing action (localized). Docs added to the Custom Commands page with a full plus‑button example.Written for commit 3839f65. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
Behavior Change
Localization
Documentation
Tests