Add configurable menu bar from cmux.json - #4052
lawrencecchen wants to merge 19 commits into
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:
📝 WalkthroughWalkthroughAdds a configurable macOS menu-bar: new UI config and resolved models for ui.menuBar, CmuxConfigStore loading/resolution and published resolved menus, dynamic-source authorization helpers, AppDelegate rendering with observers and debounced refresh, dynamic-source execution/refresh and JSON decoding, task-manager/terminal payload integration, tests, docs/schema, and localization. ChangesMenu-bar configuration and dispatch
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant ConfigStore as CmuxConfigStore
participant NSAppMainMenu as NSApp.mainMenu
participant Executor as CmuxConfigExecutor
participant ConfigController as ConfiguredMenuBarController
participant ActionExecutor as executeConfiguredCmuxAction
participant User
AppDelegate->>ConfigStore: installConfiguredMenuBarObserversIfNeeded()
ConfigStore-->>AppDelegate: publish menuBarMenus / menuBarExtensions
AppDelegate->>ConfigController: refresh / build NSMenu trees
ConfigController->>NSAppMainMenu: insert/update NSMenu items (actions, dynamic sources)
User->>NSAppMainMenu: open dynamic submenu / click menu item
NSAppMainMenu->>ConfigController: request dynamic content or call action handler
ConfigController->>Executor: authorizeDynamicMenuSourceIfNeeded (if dynamic)
Executor-->>ConfigController: sanitized command / trust result
ConfigController->>ConfigController: run command (bash), decode JSON, update cache
ConfigController->>AppDelegate: notify dynamic failure (if any)
AppDelegate->>ActionExecutor: executeConfiguredCmuxAction(action, MainWindowContext)
ActionExecutor-->>AppDelegate: action result (stdout/stderr/exit/duration)
AppDelegate->>NSAppMainMenu: update dynamic submenu contents or error state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61a1e79ce6
ℹ️ 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 store = preferredRegisteredMainWindowContext(preferredWindow: preferredWindow)?.cmuxConfigStore else { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
When all main windows are closed (a normal state in this app, especially with menu-bar-only usage), configuredMenuBarMenusForCommands returns [] because it requires an existing window context. The next refresh path removes previously injected top-level custom menus and then exits early, so global ui.menuBar commands become inaccessible until a new main window is created through some other entrypoint. This regresses menu-bar command availability in no-window sessions.
Useful? React with 👍 / 👎.
61a1e79 to
d5cb1fe
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CmuxConfigContextMenuTests.swift`:
- Around line 587-589: The test asserts that a missing menuBar action yields
kind .newWorkspaceActionNotFound which is confusing; update the issue-kind to be
clearer by either renaming the enum case to .actionNotFound (if shared across
contexts) or adding a specific case .menuBarActionNotFound and use that in the
code path that emits configuration issues for menu bar items (search for where
configuration issues are created — e.g., the code that sets
store.configurationIssues, the factory/validator that currently emits
.newWorkspaceActionNotFound — then replace the emitted kind and update this test
to expect the new enum case).
In `@Sources/CmuxConfigUI.swift`:
- Around line 271-281: The decoder currently treats any object with .items as a
submenu which allows mixed objects like { "items": [], "action": "newTerminal" }
to be silently accepted; update the initializer decoding logic (the init(from
decoder:) that uses Self.trimmedString, CodingKeys, and constructs .submenu via
CmuxConfigMenuDefinition or .action via CmuxConfigMenuBarActionItem) to detect
when both submenu keys (items) and action keys are present and throw a
DecodingError.dataCorrupted (with a clear message) instead of choosing .submenu,
otherwise proceed as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 215367c6-c5e5-48e6-acf0-5d6b252bca1c
📒 Files selected for processing (6)
Sources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigUI.swiftcmuxTests/CmuxConfigContextMenuTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| XCTAssertEqual(store.configurationIssues.first?.kind, .newWorkspaceActionNotFound) | ||
| XCTAssertEqual(store.configurationIssues.first?.settingName, "ui.menuBar.menus[0].items[0]") | ||
| XCTAssertEqual(store.configurationIssues.first?.commandName, "missing-action") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider a more specific or generic issue kind for menuBar validation.
The test validates that missing menuBar actions produce a .newWorkspaceActionNotFound issue, but the "newWorkspace" prefix is misleading since the setting path correctly identifies this as ui.menuBar.menus[0].items[0].
If the same issue kind is intentionally reused across different UI contexts, consider renaming it to something generic like .actionNotFound. Otherwise, introduce a dedicated .menuBarActionNotFound kind for clarity when debugging configuration issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/CmuxConfigContextMenuTests.swift` around lines 587 - 589, The test
asserts that a missing menuBar action yields kind .newWorkspaceActionNotFound
which is confusing; update the issue-kind to be clearer by either renaming the
enum case to .actionNotFound (if shared across contexts) or adding a specific
case .menuBarActionNotFound and use that in the code path that emits
configuration issues for menu bar items (search for where configuration issues
are created — e.g., the code that sets store.configurationIssues, the
factory/validator that currently emits .newWorkspaceActionNotFound — then
replace the emitted kind and update this test to expect the new enum case).
Greptile SummaryThis PR adds a fully configurable macOS menu bar driven by
Confidence Score: 3/5The dynamic-source subprocess is not terminated when its owning Swift Task is cancelled, the cooperative thread pool is held for the full user-configurable timeout per concurrent source, and valid resolved items are silently discarded whenever any single issue is present in a dynamic command's output. Three independent defects compound in the dynamic-source execution path: orphaned subprocesses accumulate on rapid reloads, the cooperative thread pool can starve when multiple interval sources fire simultaneously with long timeouts, and any validation issue in a dynamic command's JSON output causes the entire result to be thrown away rather than partially rendered. Sources/ConfiguredMenuBarController.swift needs the most attention: subprocess lifecycle, cooperative pool usage, GCD/MainActor hop patterns, the 1000+ line file scope, and the finishDynamicSource item-discard bug all live there. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant CMBC as ConfiguredMenuBarController
participant Runner as DynamicRunner
participant Bash as /bin/bash
participant Store as CmuxConfigStore
AppDelegate->>CMBC: installObserversIfNeeded()
Note over CMBC: Subscribes to config/window/menu notifications
Store-->>CMBC: cmuxConfigStoreDidChange (NotificationCenter)
CMBC->>CMBC: scheduleRefresh() [GCD hop]
CMBC->>AppDelegate: configuredMenuBarRuntimeContext()
AppDelegate-->>CMBC: menus + extensions + configStore
CMBC->>CMBC: removeConfiguredItems() rebuild NSMenu hierarchy
alt Dynamic source present
CMBC->>CMBC: dynamicSourceMenuItem() registers DynamicMenuDelegate
opt "refresh == .interval"
CMBC->>CMBC: scheduleIntervalDynamicSourcesIfNeeded() DispatchSourceTimer
end
end
User->>CMBC: menuWillOpen (DynamicMenuDelegate)
CMBC->>CmuxConfigExecutor: authorizeDynamicMenuSourceIfNeeded()
CmuxConfigExecutor-->>CMBC: onAuthorized(command)
CMBC->>Runner: run(command, cwd, timeout)
Runner->>Bash: Process.run()
Bash-->>Runner: stdout/stderr
Runner-->>CMBC: ConfiguredMenuBarDynamicCommandResult
alt stdout parses OK
CMBC->>Store: resolveGeneratedMenuBarItems()
Store-->>CMBC: resolved items
CMBC->>CMBC: "state.phase = .loaded"
else parse/resolve error
CMBC->>AppDelegate: notifyConfiguredMenuBarDynamicFailure()
CMBC->>CMBC: "state.phase = .failed"
end
Reviews (17): Last reviewed commit: "Deduplicate menu bar shortcut routing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| NSLog("[CmuxConfig] %@", issue.logMessage) | ||
| return (nil, issue) | ||
| } | ||
| action = resolved | ||
| } else if let inlineAction = item.inlineAction { | ||
| let id = "cmux.menuBar." + Self.generatedMenuID( | ||
| title: [settingName, item.title].compactMap { $0 }.joined(separator: "."), | ||
| index: 0 | ||
| ) | ||
| guard let resolved = CmuxResolvedConfigAction.fromDefinition( | ||
| id: id, | ||
| definition: inlineAction, | ||
| sourcePath: settingSourcePath | ||
| ) else { | ||
| let issue = CmuxConfigIssue( | ||
| kind: .newWorkspaceActionNotFound, | ||
| settingName: settingName, | ||
| commandName: item.title, | ||
| sourcePath: settingSourcePath | ||
| ) | ||
| NSLog("[CmuxConfig] %@", issue.logMessage) |
There was a problem hiding this comment.
Both resolvedMenuBarAction error paths use NSLog instead of the project's Logger-based unified logging. NSLog is not addressed by any of the allowed exceptions (not in Sources/Providers/, not #if DEBUG guarded, and these are new additions — not pre-existing code moved by this PR). The issue logMessage may also include action identifiers that could leak user-configured command names in system crash logs.
Use the existing nonisolated private let logger = Logger(subsystem: Logging.subsystem, category: "CmuxConfig") pattern and call logger.warning(...) or logger.error(...) for these paths, consistent with how the rest of the codebase handles diagnostic output.
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
| "tooltip": { | ||
| "type": "string", | ||
| "description": "Optional tooltip metadata." | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
icon property is supported by CmuxConfigMenuBarActionItem (decoded and rendered as a menu-item image) but is absent from the menuBarItem schema object. Because additionalProperties: true, schema validation won't fail, but editors and docs won't surface icon as an available key. Users relying on schema-driven autocomplete will have no guidance for it.
| "tooltip": { | |
| "type": "string", | |
| "description": "Optional tooltip metadata." | |
| } | |
| } | |
| } | |
| ] | |
| }, | |
| "tooltip": { | |
| "type": "string", | |
| "description": "Optional tooltip metadata." | |
| }, | |
| "icon": { | |
| "type": "string", | |
| "description": "Optional SF Symbol name shown as the menu-item image (e.g. \"play.fill\")." | |
| } | |
| } | |
| } | |
| ] | |
| }, |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6253-6398: Move all configured menu-bar logic out of AppDelegate
into a new ConfiguredMenuBarController class: create ConfiguredMenuBarController
and copy the private functions and state that begin with configuredMenuBar* plus
configuredMenuBarMenusForCommands(_:),
performConfiguredMenuBarAction(_:preferredWindow:),
configuredMenuBarImage(for:), configuredMenuBarMenu(from:),
performConfiguredMenuBarMenuItem(_:),
installConfiguredMenuBarObserversIfNeeded(), scheduleConfiguredMenuBarRefresh(),
refreshConfiguredMenuBar(), configuredMenuBarInsertionIndex(in:), and
createMainWindowContextForConfiguredMenuBarAction() and the arrays
configuredMenuBarTopLevelItems/configuredMenuBarActionBoxes/configuredMenuBarObserverTokens/configuredMenuBarRefreshScheduled
into it; make them instance members and resolve dependencies by injecting
required collaborators from AppDelegate (e.g., a CmuxConfigStore provider /
configured menus accessor, mainWindowContexts or a resolver function
preferredRegisteredMainWindowContext(preferredWindow:), createMainWindow(),
resolvedWindow(for:), executeConfiguredCmuxAction(_:context:preferredWindow:)),
and NSApp/main menu access; update the `@objc` selector
performConfiguredMenuBarMenuItem(_:)'s target to the new controller; in
AppDelegate replace the copied implementations with a single stored instance of
ConfiguredMenuBarController, instantiate it during setup, and forward or pass
injected closures/properties as needed so AppDelegate becomes wiring-only.
In `@Sources/CmuxConfigUI.swift`:
- Around line 207-218: The encode(to:) currently only serializes inlineAction
when action is nil, which drops the outer overrides (title/icon/tooltip) for
CmuxConfigMenuBarActionItem; update encode(to encoder: Encoder) so that when
action is nil but inlineAction is present you still create a keyed container
(keyedBy: CodingKeys.self), encode the inlineAction into the appropriate key
(using either container.encode(inlineAction, forKey: .inlineAction) or
container.superEncoder(forKey: .inlineAction) then inlineAction.encode(to:)),
and also encodeIfPresent title, icon, and tooltip into the same container so the
outer overrides are preserved on round-trip; keep the existing branch that
encodes action unchanged.
In `@web/data/cmux.schema.json`:
- Around line 41-48: The object-form schema for ui.menuBar currently allows any
object because it doesn't require the "menus" property; update the schema block
that defines the object with "properties": { "menus": { "$ref":
"#/$defs/menuBarMenus" } } (the ui.menuBar object-form) to include a "required":
["menus"] entry so that an empty or unrelated object no longer validates as
ui.menuBar and the menus property is mandatory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 116658d0-78a3-4078-a565-085cba00a8d4
📒 Files selected for processing (6)
Sources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigUI.swiftcmuxTests/CmuxConfigContextMenuTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| func configuredMenuBarMenusForCommands(preferredWindow: NSWindow? = nil) -> [CmuxResolvedMenuBarMenu] { | ||
| guard let store = preferredRegisteredMainWindowContext(preferredWindow: preferredWindow)?.cmuxConfigStore else { | ||
| return [] | ||
| } | ||
| return store.menuBarMenus | ||
| } | ||
|
|
||
| @discardableResult | ||
| func performConfiguredMenuBarAction( | ||
| _ action: CmuxResolvedConfigAction, | ||
| preferredWindow: NSWindow? = nil | ||
| ) -> Bool { | ||
| let context = preferredRegisteredMainWindowContext(preferredWindow: preferredWindow) | ||
| ?? createMainWindowContextForConfiguredMenuBarAction() | ||
| guard let context else { | ||
| NSSound.beep() | ||
| return false | ||
| } | ||
| let preferredWindow = resolvedWindow(for: context) ?? preferredWindow ?? NSApp.keyWindow ?? NSApp.mainWindow | ||
| guard executeConfiguredCmuxAction(action, context: context, preferredWindow: preferredWindow) else { | ||
| NSSound.beep() | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| private func createMainWindowContextForConfiguredMenuBarAction() -> MainWindowContext? { | ||
| guard mainWindowContexts.isEmpty else { | ||
| return preferredMainWindowContextForWorkspaceCreation(debugSource: "menuBar.configured") | ||
| } | ||
| let windowId = createMainWindow() | ||
| return mainWindowContexts.values.first { $0.windowId == windowId } | ||
| } | ||
|
|
||
| private func installConfiguredMenuBarObserversIfNeeded() { | ||
| guard configuredMenuBarObserverTokens.isEmpty else { return } | ||
| let names: [Notification.Name] = [ | ||
| .cmuxConfigStoreDidChange, | ||
| .mainWindowContextsDidChange, | ||
| NSWindow.didBecomeKeyNotification, | ||
| NSWindow.didBecomeMainNotification, | ||
| ] | ||
| configuredMenuBarObserverTokens = names.map { name in | ||
| NotificationCenter.default.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in | ||
| Task { @MainActor [weak self] in | ||
| self?.scheduleConfiguredMenuBarRefresh() | ||
| } | ||
| } | ||
| } | ||
| scheduleConfiguredMenuBarRefresh() | ||
| } | ||
|
|
||
| private func scheduleConfiguredMenuBarRefresh() { | ||
| guard !configuredMenuBarRefreshScheduled else { return } | ||
| configuredMenuBarRefreshScheduled = true | ||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self else { return } | ||
| self.configuredMenuBarRefreshScheduled = false | ||
| self.refreshConfiguredMenuBar() | ||
| } | ||
| } | ||
|
|
||
| private func refreshConfiguredMenuBar() { | ||
| guard let mainMenu = NSApp.mainMenu else { return } | ||
|
|
||
| for item in configuredMenuBarTopLevelItems { | ||
| let index = mainMenu.index(of: item) | ||
| if index >= 0 { | ||
| mainMenu.removeItem(at: index) | ||
| } | ||
| } | ||
| configuredMenuBarTopLevelItems.removeAll() | ||
| configuredMenuBarActionBoxes.removeAll() | ||
|
|
||
| let menus = configuredMenuBarMenusForCommands(preferredWindow: NSApp.keyWindow ?? NSApp.mainWindow) | ||
| guard !menus.isEmpty else { return } | ||
|
|
||
| var insertionIndex = configuredMenuBarInsertionIndex(in: mainMenu) | ||
| for menu in menus { | ||
| let item = NSMenuItem(title: menu.title, action: nil, keyEquivalent: "") | ||
| item.submenu = configuredMenuBarMenu(from: menu) | ||
| mainMenu.insertItem(item, at: insertionIndex) | ||
| configuredMenuBarTopLevelItems.append(item) | ||
| insertionIndex += 1 | ||
| } | ||
| } | ||
|
|
||
| private func configuredMenuBarInsertionIndex(in mainMenu: NSMenu) -> Int { | ||
| let notificationsTitle = String(localized: "menu.notifications.title", defaultValue: "Notifications") | ||
| if let notificationsIndex = mainMenu.items.lastIndex(where: { $0.title == notificationsTitle }) { | ||
| return notificationsIndex + 1 | ||
| } | ||
| #if DEBUG | ||
| if let debugIndex = mainMenu.items.lastIndex(where: { $0.title == "Debug" }) { | ||
| return debugIndex | ||
| } | ||
| #endif | ||
| return min(mainMenu.items.count, max(1, mainMenu.items.count - 1)) | ||
| } | ||
|
|
||
| private func configuredMenuBarMenu(from menu: CmuxResolvedMenuBarMenu) -> NSMenu { | ||
| let nsMenu = NSMenu(title: menu.title) | ||
| for item in menu.items { | ||
| switch item { | ||
| case .separator: | ||
| if !nsMenu.items.isEmpty, nsMenu.items.last?.isSeparatorItem == false { | ||
| nsMenu.addItem(.separator()) | ||
| } | ||
| case .submenu(let submenu): | ||
| let item = NSMenuItem(title: submenu.title, action: nil, keyEquivalent: "") | ||
| item.submenu = configuredMenuBarMenu(from: submenu) | ||
| nsMenu.addItem(item) | ||
| case .action(let menuAction): | ||
| let item = NSMenuItem( | ||
| title: menuAction.title, | ||
| action: #selector(performConfiguredMenuBarMenuItem(_:)), | ||
| keyEquivalent: "" | ||
| ) | ||
| item.target = self | ||
| let box = ConfiguredMenuBarActionBox(action: menuAction.action) | ||
| configuredMenuBarActionBoxes.append(box) | ||
| item.representedObject = box | ||
| item.toolTip = menuAction.tooltip | ||
| item.image = configuredMenuBarImage(for: menuAction.icon ?? menuAction.action.icon) | ||
| nsMenu.addItem(item) | ||
| } | ||
| } | ||
|
|
||
| while nsMenu.items.last?.isSeparatorItem == true { | ||
| nsMenu.removeItem(at: nsMenu.items.count - 1) | ||
| } | ||
| return nsMenu | ||
| } | ||
|
|
||
| private func configuredMenuBarImage(for icon: CmuxButtonIcon?) -> NSImage? { | ||
| guard case .some(.symbol(let symbolName)) = icon else { return nil } | ||
| return NSImage(systemSymbolName: symbolName, accessibilityDescription: nil) | ||
| } | ||
|
|
||
| @objc private func performConfiguredMenuBarMenuItem(_ sender: NSMenuItem) { | ||
| guard let box = sender.representedObject as? ConfiguredMenuBarActionBox else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| performConfiguredMenuBarAction(box.action, preferredWindow: NSApp.keyWindow ?? NSApp.mainWindow) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract configurable menu-bar construction/dispatch out of AppDelegate.
This block adds more UI menu rendering + action-routing responsibility to an already cross-cutting file. Please move this feature into a focused coordinator (e.g., ConfiguredMenuBarController) and keep AppDelegate as wiring-only.
As per coding guidelines: "Flag Swift files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppDelegate.swift` around lines 6253 - 6398, Move all configured
menu-bar logic out of AppDelegate into a new ConfiguredMenuBarController class:
create ConfiguredMenuBarController and copy the private functions and state that
begin with configuredMenuBar* plus configuredMenuBarMenusForCommands(_:),
performConfiguredMenuBarAction(_:preferredWindow:),
configuredMenuBarImage(for:), configuredMenuBarMenu(from:),
performConfiguredMenuBarMenuItem(_:),
installConfiguredMenuBarObserversIfNeeded(), scheduleConfiguredMenuBarRefresh(),
refreshConfiguredMenuBar(), configuredMenuBarInsertionIndex(in:), and
createMainWindowContextForConfiguredMenuBarAction() and the arrays
configuredMenuBarTopLevelItems/configuredMenuBarActionBoxes/configuredMenuBarObserverTokens/configuredMenuBarRefreshScheduled
into it; make them instance members and resolve dependencies by injecting
required collaborators from AppDelegate (e.g., a CmuxConfigStore provider /
configured menus accessor, mainWindowContexts or a resolver function
preferredRegisteredMainWindowContext(preferredWindow:), createMainWindow(),
resolvedWindow(for:), executeConfiguredCmuxAction(_:context:preferredWindow:)),
and NSApp/main menu access; update the `@objc` selector
performConfiguredMenuBarMenuItem(_:)'s target to the new controller; in
AppDelegate replace the copied implementations with a single stored instance of
ConfiguredMenuBarController, instantiate it during setup, and forward or pass
injected closures/properties as needed so AppDelegate becomes wiring-only.
| "type": "object", | ||
| "additionalProperties": true, | ||
| "properties": { | ||
| "menus": { | ||
| "$ref": "#/$defs/menuBarMenus" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Require menus in object-form ui.menuBar schema.
Right now {} (and any unrelated object) validates as ui.menuBar, which weakens config validation and can hide user mistakes.
Suggested schema fix
{
"type": "object",
"additionalProperties": true,
+ "required": ["menus"],
"properties": {
"menus": {
"$ref": "#/$defs/menuBarMenus"
}
}
}📝 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.
| "type": "object", | |
| "additionalProperties": true, | |
| "properties": { | |
| "menus": { | |
| "$ref": "#/$defs/menuBarMenus" | |
| } | |
| } | |
| } | |
| "type": "object", | |
| "additionalProperties": true, | |
| "required": ["menus"], | |
| "properties": { | |
| "menus": { | |
| "$ref": "#/$defs/menuBarMenus" | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/data/cmux.schema.json` around lines 41 - 48, The object-form schema for
ui.menuBar currently allows any object because it doesn't require the "menus"
property; update the schema block that defines the object with "properties": {
"menus": { "$ref": "#/$defs/menuBarMenus" } } (the ui.menuBar object-form) to
include a "required": ["menus"] entry so that an empty or unrelated object no
longer validates as ui.menuBar and the menus property is mandatory.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| mainMenu: mainMenu, | ||
| customMenusByConfigID: customMenusByConfigID | ||
| ) else { | ||
| NSLog("[MenuBarConfig] Skipping extension for missing menu target %@", menuExtension.targetID) |
There was a problem hiding this comment.
NSLog("[MenuBarConfig] Skipping extension for missing menu target %@", menuExtension.targetID) writes directly to the system log as unstructured text, bypassing the project's Logger-based unified logging. It also interpolates a user-configured target ID into the message, which ends up in Console.app and crash reports. Use the existing nonisolated private let logger = Logger(subsystem: Logging.subsystem, category: "AppDelegate") pattern and call logger.warning(...) with the target ID at .private sensitivity if it could contain user data.
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
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/CmuxConfig.swift (1)
1740-1769: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the menu-bar resolver out of
CmuxConfigStore.This file is already over 3k lines, and this PR adds another large subsystem for menu-bar grouping, resolution, issue building, and publication on top of parsing and file-watching. That makes the store harder to test in isolation and breaches the repo’s large-file budget for existing production Swift files. As per coding guidelines: "do not accept more than 250 lines added to an existing production Swift file that is already over 800 lines, unless an extraction exception is met by removing/moving mixed responsibilities and decreasing total line count by more than 200 lines."
Also applies to: 1933-2806
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/CmuxConfig.swift` around lines 1740 - 1769, The CmuxConfigStore has grown too large and the menu-bar resolution subsystem (types and logic around menu-bar grouping, resolution, issue building and publication) must be extracted into a separate resolver component: create a new file (e.g., MenuBarResolver) and move the related structs and logic (MenuBarConfigGroup, ResolvedSurfaceTabBarButtonEntry, ResolvedSurfaceTabBarButtons, ResolvedContextMenuItems, ResolvedMenuBarMenus, ResolvedMenuBarItems and any helper functions/methods that build/resolve menus or publish menu-bar state) into that new type, refactor CmuxConfigStore to call into the new MenuBarResolver API for parsing/watching results and issue aggregation, update visibility/access control and dependency injection so tests can target MenuBarResolver in isolation, and remove the moved code from CmuxConfigStore to ensure the original file's total line count is reduced by at least ~200 lines.
♻️ Duplicate comments (3)
web/data/cmux.schema.json (1)
41-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire
menusin object-formui.menuBarschema.Right now
{}(and any unrelated object) validates asui.menuBar, which weakens config validation and can hide user mistakes.🔧 Suggested schema fix
{ "type": "object", "additionalProperties": true, + "required": ["menus"], "properties": { "menus": { "$ref": "#/$defs/menuBarMenus" } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/data/cmux.schema.json` around lines 41 - 48, The schema for the object-form ui.menuBar currently allows {} because "menus" is optional; update the object schema (the definition that has "properties": { "menus": { "$ref": "#/$defs/menuBarMenus" } }) to require the menus property by adding "required": ["menus"] so only objects containing the menus (as defined by $defs/menuBarMenus) validate as ui.menuBar.cmuxTests/CmuxConfigContextMenuTests.swift (1)
721-723: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsider a more specific issue kind for menuBar validation.
The test validates that missing menuBar actions produce a
.newWorkspaceActionNotFoundissue, but the "newWorkspace" prefix is misleading since the setting path correctly identifies this asui.menuBar.menus[0].items[0].If the same issue kind is intentionally reused across different UI contexts, consider renaming it to something generic like
.actionNotFound. Otherwise, introduce a dedicated.menuBarActionNotFoundkind for clarity when debugging configuration issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/CmuxConfigContextMenuTests.swift` around lines 721 - 723, The test asserts the issue kind as .newWorkspaceActionNotFound for a missing menu bar action; rename or replace that case with a clearer symbol and update usages: either add a new enum case .menuBarActionNotFound and change the validator that currently emits .newWorkspaceActionNotFound (and all references) to emit .menuBarActionNotFound, or rename the case to a generic .actionNotFound and update the test assertion (XCTAssertEqual(store.configurationIssues.first?.kind, .menuBarActionNotFound or .actionNotFound)) and any code that constructs that issue; ensure the settingName check ("ui.menuBar.menus[0].items[0]") remains unchanged.Sources/AppDelegate.swift (1)
6520-7197: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSame concern: configurable menu-bar rendering/dispatch keeps growing inside
AppDelegate.This adds another ~680 lines of menu construction, dynamic-source orchestration, subprocess execution wiring, task-manager payload shaping, and three new
@objcaction selectors directly toAppDelegate.swift, which is already a multi-thousand-line file mixing AppKit, window/workspace coordination, shortcut routing, and notifications. The previous reviewer feedback to extract a focused coordinator (e.g.,ConfiguredMenuBarController) still applies and would letAppDelegatestay wiring-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AppDelegate.swift` around lines 6520 - 7197, The AppDelegate has ~680 lines of menu-bar logic that should be extracted into a dedicated coordinator to keep AppDelegate wiring-only; move all functions and state related to configured menu bar rendering/dispatch (e.g., configuredMenuBarMenusForCommands, configuredMenuBarExtensionsForCommands, performConfiguredMenuBarAction, createMainWindowContextForConfiguredMenuBarAction, installConfiguredMenuBarObserversIfNeeded, scheduleConfiguredMenuBarRefresh, refreshConfiguredMenuBar, configuredMenuBarInsertionIndex, configuredMenuBarMenu, configuredMenuBarMenuItems, configuredMenuBarImage, configuredMenuBarDynamicSourceMenuItem, configuredMenuBarTargetMenu, normalizedConfiguredMenuBarTargetID, runConfigReloadDynamicMenuSourcesIfNeeded, scheduleIntervalDynamicMenuSourcesIfNeeded, cancelRemovedConfiguredMenuBarDynamicTimers, cancelConfiguredMenuBarDynamicTimer, configuredMenuBarDynamicMenuWillOpen, renderConfiguredMenuBarDynamicSourceMenu, configuredMenuBarDisabledItem, dynamicMenuEmptyTitle, configuredMenuBarAddSeparatorIfNeeded, refreshConfiguredMenuBarDynamicSource, startConfiguredMenuBarDynamicSource, finishConfiguredMenuBarDynamicSource, notifyConfiguredMenuBarDynamicFailure, configuredDynamicMenuTaskManagerPayload, configuredDynamicMenuPhaseLabel, performConfiguredMenuBarMenuItem, reloadConfiguredMenuBarDynamicSource, copyConfiguredMenuBarDynamicSourceError and all associated stored properties/state/boxes/delegates) into a new ConfiguredMenuBarController (or similar) class; have AppDelegate instantiate and delegate observer registration, menu refresh triggers, and the three `@objc` selectors to that controller, and update callers to use controller methods (e.g., controller.refresh(), controller.performAction(_:preferredWindow:), controller.configuredMenusForCommands()). Ensure all references to AppDelegate-only globals (tabManager, notificationStore, createMainWindow, preferredRegisteredMainWindowContext, etc.) are passed via initializer or small protocol so the controller is testable and AppDelegate remains a thin coordinator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 7124-7141: The code is formatting countable strings with
String(format:) which won't pluralize correctly; add ICU plural entries for
taskManager.dynamicMenu.items (and taskManager.dynamicMenu.duration if desired)
in Localizable.xcstrings with .one/.other forms, then change the call sites that
build detailParts (where generatedItemCount and durationMS are used) to use the
localized plural selection API (use the String(localized:..., count:) or
equivalent ICU-aware initializer) instead of wrapping String(localized: ...) in
String(format:), so the correct plural form is selected for each locale.
- Around line 142-244: The DispatchSource timer created by
DispatchSource.makeTimerSource(...) is suspended and may be released without
ever being resumed if process.run() throws; resume the timer before any early
return so the source is not released suspended. Update the flow around the try/
catch and timer usage (symbols: timer, finish(_:) and process.run()) to ensure
timer.resume() is called before calling finish or returning on error — either
resume the timer immediately after configuring it (before try process.run()) or
call timer.resume() in the catch path before invoking finish — so the suspended
source is always resumed prior to potential cancellation/release.
In `@Sources/CmuxConfig.swift`:
- Around line 2553-2608: The code can publish duplicate top-level menu configIDs
(generated by generatedMenuID and sanitized by sanitizeConfigText), causing
ambiguous extends routing; fix by tracking seen configIDs in the loop that
builds CmuxResolvedMenuBarMenu (the block that produces
menus/extensions/issues), and if a computed configID (configID) is already in
the seen set, append a CmuxConfigIssue(kind: .menuBarInvalidMenu, settingName:
menuSettingName, sourcePath: group.sourcePath, message: "duplicate menu id") and
skip adding the duplicate menu instead of publishing it; apply the same
seen-configID check and error emission in the other analogous menu-building
location (the block referenced around the other menu loop) so duplicates across
config groups are rejected consistently.
In `@Sources/TaskManagerSnapshot.swift`:
- Around line 82-84: The UI shows incorrect "1 sources" because the code uses a
simple format string for dynamicMenus count; update localization to use ICU
pluralization (provide keys like "taskManager.row.dynamicMenus.count.one" and
".other" or an ICU plural entry) and change the call in TaskManagerSnapshot
where dynamicMenus.count is formatted (the String(format: String(localized:
"taskManager.row.dynamicMenus.count", ...), dynamicMenus.count) usage) to use
the ICU-aware localization API (e.g., the localized plural lookup /
String.localizedStringWithFormat or Swift's String(localized:) with a pluralized
localization entry) so the singular and plural forms are rendered correctly
across locales.
---
Outside diff comments:
In `@Sources/CmuxConfig.swift`:
- Around line 1740-1769: The CmuxConfigStore has grown too large and the
menu-bar resolution subsystem (types and logic around menu-bar grouping,
resolution, issue building and publication) must be extracted into a separate
resolver component: create a new file (e.g., MenuBarResolver) and move the
related structs and logic (MenuBarConfigGroup, ResolvedSurfaceTabBarButtonEntry,
ResolvedSurfaceTabBarButtons, ResolvedContextMenuItems, ResolvedMenuBarMenus,
ResolvedMenuBarItems and any helper functions/methods that build/resolve menus
or publish menu-bar state) into that new type, refactor CmuxConfigStore to call
into the new MenuBarResolver API for parsing/watching results and issue
aggregation, update visibility/access control and dependency injection so tests
can target MenuBarResolver in isolation, and remove the moved code from
CmuxConfigStore to ensure the original file's total line count is reduced by at
least ~200 lines.
---
Duplicate comments:
In `@cmuxTests/CmuxConfigContextMenuTests.swift`:
- Around line 721-723: The test asserts the issue kind as
.newWorkspaceActionNotFound for a missing menu bar action; rename or replace
that case with a clearer symbol and update usages: either add a new enum case
.menuBarActionNotFound and change the validator that currently emits
.newWorkspaceActionNotFound (and all references) to emit .menuBarActionNotFound,
or rename the case to a generic .actionNotFound and update the test assertion
(XCTAssertEqual(store.configurationIssues.first?.kind, .menuBarActionNotFound or
.actionNotFound)) and any code that constructs that issue; ensure the
settingName check ("ui.menuBar.menus[0].items[0]") remains unchanged.
In `@Sources/AppDelegate.swift`:
- Around line 6520-7197: The AppDelegate has ~680 lines of menu-bar logic that
should be extracted into a dedicated coordinator to keep AppDelegate
wiring-only; move all functions and state related to configured menu bar
rendering/dispatch (e.g., configuredMenuBarMenusForCommands,
configuredMenuBarExtensionsForCommands, performConfiguredMenuBarAction,
createMainWindowContextForConfiguredMenuBarAction,
installConfiguredMenuBarObserversIfNeeded, scheduleConfiguredMenuBarRefresh,
refreshConfiguredMenuBar, configuredMenuBarInsertionIndex,
configuredMenuBarMenu, configuredMenuBarMenuItems, configuredMenuBarImage,
configuredMenuBarDynamicSourceMenuItem, configuredMenuBarTargetMenu,
normalizedConfiguredMenuBarTargetID, runConfigReloadDynamicMenuSourcesIfNeeded,
scheduleIntervalDynamicMenuSourcesIfNeeded,
cancelRemovedConfiguredMenuBarDynamicTimers,
cancelConfiguredMenuBarDynamicTimer, configuredMenuBarDynamicMenuWillOpen,
renderConfiguredMenuBarDynamicSourceMenu, configuredMenuBarDisabledItem,
dynamicMenuEmptyTitle, configuredMenuBarAddSeparatorIfNeeded,
refreshConfiguredMenuBarDynamicSource, startConfiguredMenuBarDynamicSource,
finishConfiguredMenuBarDynamicSource, notifyConfiguredMenuBarDynamicFailure,
configuredDynamicMenuTaskManagerPayload, configuredDynamicMenuPhaseLabel,
performConfiguredMenuBarMenuItem, reloadConfiguredMenuBarDynamicSource,
copyConfiguredMenuBarDynamicSourceError and all associated stored
properties/state/boxes/delegates) into a new ConfiguredMenuBarController (or
similar) class; have AppDelegate instantiate and delegate observer registration,
menu refresh triggers, and the three `@objc` selectors to that controller, and
update callers to use controller methods (e.g., controller.refresh(),
controller.performAction(_:preferredWindow:),
controller.configuredMenusForCommands()). Ensure all references to
AppDelegate-only globals (tabManager, notificationStore, createMainWindow,
preferredRegisteredMainWindowContext, etc.) are passed via initializer or small
protocol so the controller is testable and AppDelegate remains a thin
coordinator.
In `@web/data/cmux.schema.json`:
- Around line 41-48: The schema for the object-form ui.menuBar currently allows
{} because "menus" is optional; update the object schema (the definition that
has "properties": { "menus": { "$ref": "#/$defs/menuBarMenus" } }) to require
the menus property by adding "required": ["menus"] so only objects containing
the menus (as defined by $defs/menuBarMenus) validate as ui.menuBar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8ce583a-a1cc-4d8a-9fa2-e3ad98c0770f
📒 Files selected for processing (13)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/CmuxConfigUI.swiftSources/ContentView.swiftSources/TaskManagerSnapshot.swiftSources/TaskManagerTypes.swiftSources/TerminalController.swiftSources/TerminalControllerTopSupport.swiftcmuxTests/CmuxConfigContextMenuTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| if let generatedItemCount = state.generatedItemCount { | ||
| detailParts.append(String( | ||
| format: String( | ||
| localized: "taskManager.dynamicMenu.items", | ||
| defaultValue: "%d items" | ||
| ), | ||
| generatedItemCount | ||
| )) | ||
| } | ||
| if let durationMS = state.durationMS { | ||
| detailParts.append(String( | ||
| format: String( | ||
| localized: "taskManager.dynamicMenu.duration", | ||
| defaultValue: "%d ms" | ||
| ), | ||
| durationMS | ||
| )) | ||
| } |
There was a problem hiding this comment.
Use ICU plural forms for the user-visible count strings.
"%d items" (and to a lesser extent "%d ms") is a countable noun that doesn't pluralize correctly across locales when emitted via String(format:). Define taskManager.dynamicMenu.items (and similar count-based strings used in user-facing menu/task-manager UI) with .one/.other plural variants in Localizable.xcstrings, and select via String(localized: ...) so each locale gets the right form.
Based on learnings: "Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other ... ensures correct pluralization across locales and makes localization keys explicit."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppDelegate.swift` around lines 7124 - 7141, The code is formatting
countable strings with String(format:) which won't pluralize correctly; add ICU
plural entries for taskManager.dynamicMenu.items (and
taskManager.dynamicMenu.duration if desired) in Localizable.xcstrings with
.one/.other forms, then change the call sites that build detailParts (where
generatedItemCount and durationMS are used) to use the localized plural
selection API (use the String(localized:..., count:) or equivalent ICU-aware
initializer) instead of wrapping String(localized: ...) in String(format:), so
the correct plural form is selected for each locale.
| var menus: [CmuxResolvedMenuBarMenu] = [] | ||
| var extensions: [CmuxResolvedMenuBarExtension] = [] | ||
| var issues: [CmuxConfigIssue] = [] | ||
|
|
||
| for (groupIndex, group) in configuredGroups.enumerated() { | ||
| for (index, menu) in group.menus.enumerated() { | ||
| let menuSettingName = "\(group.settingName)[\(index)]" | ||
| let menuIdentityName = "\(group.settingName).source\(groupIndex)[\(index)]" | ||
| let resolvedItems = resolvedMenuBarItems( | ||
| menu.items, | ||
| actions: actions, | ||
| commands: commands, | ||
| sourcePaths: sourcePaths, | ||
| settingName: "\(menuSettingName).items", | ||
| settingSourcePath: group.sourcePath, | ||
| identityName: "\(menuIdentityName).items" | ||
| ) | ||
| issues.append(contentsOf: resolvedItems.issues) | ||
| guard !resolvedItems.items.isEmpty else { continue } | ||
|
|
||
| if let targetID = menu.extends { | ||
| let sanitizedTargetID = sanitizeConfigText(targetID, fallback: targetID) | ||
| extensions.append( | ||
| CmuxResolvedMenuBarExtension( | ||
| id: "\(menuIdentityName).extends.\(sanitizedTargetID)", | ||
| targetID: sanitizedTargetID, | ||
| items: resolvedItems.items | ||
| ) | ||
| ) | ||
| continue | ||
| } | ||
|
|
||
| guard let title = menu.title else { | ||
| issues.append(CmuxConfigIssue( | ||
| kind: .menuBarInvalidMenu, | ||
| settingName: menuSettingName, | ||
| sourcePath: group.sourcePath, | ||
| message: "menuBar menu must define title or extends" | ||
| )) | ||
| continue | ||
| } | ||
|
|
||
| let fallbackID = menu.id ?? Self.generatedMenuID(title: title, index: index) | ||
| let configID = sanitizeConfigText(fallbackID, fallback: String(index)) | ||
| menus.append( | ||
| CmuxResolvedMenuBarMenu( | ||
| id: "\(menuIdentityName).\(configID)", | ||
| configID: configID, | ||
| title: sanitizeConfigText(title, fallback: fallbackID), | ||
| items: resolvedItems.items | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return ResolvedMenuBarMenus(menus: menus, extensions: extensions, issues: issues) |
There was a problem hiding this comment.
Validate duplicate top-level menu IDs before publishing them.
generatedMenuID(title:index) reuses the encoded title whenever it is non-empty, so two menus with the same title from different config groups resolve to the same configID. Because extends targets menus by that identifier, duplicates make extension routing ambiguous and can attach items to the wrong menu. Track seen configIDs here and surface a .menuBarInvalidMenu issue on duplicates instead of publishing both.
Also applies to: 2802-2806
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/CmuxConfig.swift` around lines 2553 - 2608, The code can publish
duplicate top-level menu configIDs (generated by generatedMenuID and sanitized
by sanitizeConfigText), causing ambiguous extends routing; fix by tracking seen
configIDs in the loop that builds CmuxResolvedMenuBarMenu (the block that
produces menus/extensions/issues), and if a computed configID (configID) is
already in the seen set, append a CmuxConfigIssue(kind: .menuBarInvalidMenu,
settingName: menuSettingName, sourcePath: group.sourcePath, message: "duplicate
menu id") and skip adding the duplicate menu instead of publishing it; apply the
same seen-configID check and error emission in the other analogous menu-building
location (the block referenced around the other menu loop) so duplicates across
config groups are rejected consistently.
| detail: String( | ||
| format: String(localized: "taskManager.row.dynamicMenus.count", defaultValue: "%d sources"), | ||
| dynamicMenus.count |
There was a problem hiding this comment.
Use ICU plural forms for dynamic menu count localization.
The current format string will produce "1 sources" for a single source. Based on learnings, pluralized strings should use separate localization keys with .one and .other suffixes to ensure correct pluralization across all locales.
🌐 Proposed fix using ICU plural forms
- detail: String(
- format: String(localized: "taskManager.row.dynamicMenus.count", defaultValue: "%d sources"),
- dynamicMenus.count
- ),
+ detail: dynamicMenus.count == 1
+ ? String(localized: "taskManager.row.dynamicMenus.count.one", defaultValue: "1 source")
+ : String(localized: "taskManager.row.dynamicMenus.count.other", defaultValue: "\(dynamicMenus.count) sources"),📝 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.
| detail: String( | |
| format: String(localized: "taskManager.row.dynamicMenus.count", defaultValue: "%d sources"), | |
| dynamicMenus.count | |
| detail: dynamicMenus.count == 1 | |
| ? String(localized: "taskManager.row.dynamicMenus.count.one", defaultValue: "1 source") | |
| : String(localized: "taskManager.row.dynamicMenus.count.other", defaultValue: "\(dynamicMenus.count) sources"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TaskManagerSnapshot.swift` around lines 82 - 84, The UI shows
incorrect "1 sources" because the code uses a simple format string for
dynamicMenus count; update localization to use ICU pluralization (provide keys
like "taskManager.row.dynamicMenus.count.one" and ".other" or an ICU plural
entry) and change the call in TaskManagerSnapshot where dynamicMenus.count is
formatted (the String(format: String(localized:
"taskManager.row.dynamicMenus.count", ...), dynamicMenus.count) usage) to use
the ICU-aware localization API (e.g., the localized plural lookup /
String.localizedStringWithFormat or Swift's String(localized:) with a pluralized
localization entry) so the singular and plural forms are rendered correctly
across 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CmuxConfig.swift (1)
1740-1770: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract menu-bar configuration plumbing into a dedicated file.
Sources/CmuxConfig.swiftis already ~3,250 lines and mixes config models, the SVG security inspector,CmuxConfigStorestate ownership, file/directory watchers, JSONC parsing, and now all of menu-bar resolution. This PR adds ~337 lines of menu-bar-specific code (MenuBarConfigGroup,ResolvedMenuBarMenus,ResolvedMenuBarItems,resolvedMenuBarMenus,resolvedMenuBarItems,appendMenuBarSeparatorIfNeeded,resolvedMenuBarAction,resolveGeneratedMenuBarItems,generatedMenuID, plus the new published properties andloadAll()glue) to a file that is already well past the budgeted threshold for a coherent-responsibility file.The resolution logic is self-contained (operates over
actions,commands,sourcePathsplussanitizeConfigText/canonicalActionID) and would fit naturally in something likeSources/CmuxConfigMenuBar.swiftas an extension ofCmuxConfigStore, keeping only the@Publishedstorage, theloadAll()wiring, andMenuBarConfigGroupinside this file (or moving them too). That keeps this file from continuing to grow as the menu-bar surface area expands (icons, dynamic sources, extensions, etc.).As per coding guidelines, "do not accept more than 250 lines added to an existing production Swift file that is already over 800 lines, unless an extraction exception is met by removing/moving mixed responsibilities and decreasing total line count by more than 200 lines" and "Flag Swift files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place".
Also applies to: 2087-2118, 2543-2723
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/CmuxConfig.swift` around lines 1740 - 1770, The file is too large and the menu-bar resolution logic should be extracted into a new dedicated file; move the menu-bar-specific types and functions (MenuBarConfigGroup, ResolvedSurfaceTabBarButtonEntry, ResolvedSurfaceTabBarButtons, ResolvedContextMenuItems, ResolvedMenuBarMenus, ResolvedMenuBarItems and the functions resolvedMenuBarMenus, resolvedMenuBarItems, appendMenuBarSeparatorIfNeeded, resolvedMenuBarAction, resolveGeneratedMenuBarItems, generatedMenuID plus any helpers that operate only on actions/commands/sourcePaths/sanitizeConfigText/canonicalActionID) into a new Sources/CmuxConfigMenuBar.swift as an extension of CmuxConfigStore, preserving access control and imports, and update references; keep only the `@Published` storage and the loadAll() wiring in the original file (or move them too if you prefer) so resolution logic is isolated and compilation/tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CmuxConfig.swift`:
- Around line 2746-2775: The menu-bar error cases in the resolvedMenuBarAction
flow incorrectly use CmuxConfigIssue(kind: .newWorkspaceActionNotFound, ...) —
change both occurrences that construct CmuxConfigIssue in the branch that
handles item.action (using canonicalActionID/actionReference) and the branch
that builds an inline action via CmuxResolvedConfigAction.fromDefinition (using
item.title) to use .menuBarInvalidMenu instead of .newWorkspaceActionNotFound so
menu-bar problems are reported with the new menuBarInvalidMenu kind (keep the
same settingName, commandName, and sourcePath parameters).
- Around line 1971-1977: Clarify that ui.menuBar.menus uses composition (both
global and local contribute) by adding an inline comment next to the
MenuBarConfigGroup append and by updating the PR summary/docs; explicitly
mention that configuredMenuBarGroups.append(...) for ui.menuBar.menus
intentionally composes local menus with global menus (see
testResolvedMenuBarSupportsLocalAndGlobalMenus) rather than overriding like
newWorkspaceCommand/newWorkspace.action/newWorkspace.contextMenu/surfaceTabBarButtons
which check configuredNewWorkspaceActionID. Add a brief comment above the if-let
block referencing the composition semantics and note in the PR summary/docs that
menus are combined (local appended, global prepended) so readers aren’t misled
by “local-first, global-fallback.”
In `@Sources/CmuxConfigUI.swift`:
- Around line 92-109: Create a single reusable decoding helper by adding an
extension on KeyedDecodingContainer with two methods:
decodeTrimmedString(forKey:allowBlankAsNil:) -> String? and
decodeRequiredTrimmedString(forKey:) -> String that encapsulate the trimming,
blank-as-nil handling, and DecodingError.dataCorruptedError construction (use
key.stringValue in messages). Replace the duplicate private static
trimmedString/requiredTrimmedString helpers in types CmuxConfigButtonPlacement,
CmuxConfigContextMenuActionItem, CmuxConfigContextMenuItem (and any other files
noted) to call these new extension methods instead, keeping behavior and error
messages identical. Ensure the extension uses the container’s Key generic (Key:
CodingKey) so it works with each type’s CodingKeys and update callers to remove
the old private helpers.
In `@Sources/ConfiguredMenuBarController.swift`:
- Around line 691-696: The cancelDynamicTask currently cancels the Swift Task
and clears dynamicStates[sourceID].activeRunID/activePID but never signals the
underlying Process, leaving orphaned bash processes; update cancellation to
terminate the subprocess by sending a signal to the captured PID
(dynamicStates[sourceID].activePID) before clearing it (e.g., call kill(pid,
SIGTERM) or Process.terminate()), and/or modify
ConfiguredMenuBarDynamicRunner.run to wrap its withCheckedContinuation in
withTaskCancellationHandler so the cancellation handler calls
process.terminate() (or kills the PID) to ensure the subprocess is stopped when
dynamicTasks[sourceID]?.cancel() is invoked.
- Around line 1-982: The file mixes subprocess/output buffering logic with
AppKit UI/controller code; extract the subprocess pieces into a separate target
by moving ConfiguredMenuBarOutputCollector,
ConfiguredMenuBarDynamicCommandResult, and ConfiguredMenuBarDynamicRunner out of
ConfiguredMenuBarController into their own Swift source(s) in a new SwiftPM
target (e.g., CmuxSubprocess). Update the types' access levels to
public/internal as needed so ConfiguredMenuBarController can still call
ConfiguredMenuBarDynamicRunner.run and use
ConfiguredMenuBarDynamicCommandResult/ConfiguredMenuBarOutputCollector, add the
new package import where the controller references these symbols, and leave
ConfiguredMenuBarController (and all AppKit-dependent logic) in the existing app
module. Ensure you preserve behavior: keep the same APIs (function signatures,
constants like defaultTimeoutSeconds and outputLimitBytes) and tests can be
added against the new package.
- Around line 220-230: The timeout handler currently calls process.terminate()
(see timer.setEventHandler, finishLock, didFinish, didTimeOut) which only
SIGTERM's the bash parent and can leave child processes running; replace this by
spawning the command as a new process group using POSIX APIs (import Darwin,
create posix_spawnattr_t with POSIX_SPAWN_SETPGROUP and call
posix_spawnattr_setpgroup(attr, 0) so the child becomes a group leader) and
store the child PID, then on timeout send the signal to the whole group (use
kill(-childPid, SIGTERM) and optionally SIGKILL after a grace period) instead of
calling process.terminate(); ensure cleanup paths (cancel/normal finish) also
signal the group and avoid races with finishLock/didFinish/didTimeOut.
In `@web/data/cmux.schema.json`:
- Around line 996-1012: Add a JSON Schema conditional so intervalSeconds is
required when refresh equals "interval": modify the schema block containing the
"refresh" and "intervalSeconds" properties to include an if/then (or allOf with
if/then) that checks {"properties":{"refresh":{"const":"interval"}}} and in the
then clause adds "required":["intervalSeconds"] and optionally constraints on
intervalSeconds; follow the same pattern used for sessionIdSource to locate
where to insert the conditional so the validator rejects objects missing
intervalSeconds when refresh is "interval".
---
Outside diff comments:
In `@Sources/CmuxConfig.swift`:
- Around line 1740-1770: The file is too large and the menu-bar resolution logic
should be extracted into a new dedicated file; move the menu-bar-specific types
and functions (MenuBarConfigGroup, ResolvedSurfaceTabBarButtonEntry,
ResolvedSurfaceTabBarButtons, ResolvedContextMenuItems, ResolvedMenuBarMenus,
ResolvedMenuBarItems and the functions resolvedMenuBarMenus,
resolvedMenuBarItems, appendMenuBarSeparatorIfNeeded, resolvedMenuBarAction,
resolveGeneratedMenuBarItems, generatedMenuID plus any helpers that operate only
on actions/commands/sourcePaths/sanitizeConfigText/canonicalActionID) into a new
Sources/CmuxConfigMenuBar.swift as an extension of CmuxConfigStore, preserving
access control and imports, and update references; keep only the `@Published`
storage and the loadAll() wiring in the original file (or move them too if you
prefer) so resolution logic is isolated and compilation/tests pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 550b4308-b77e-4ba7-9985-63202a264cc7
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigUI.swiftSources/ConfiguredMenuBarController.swiftcmuxTests/CmuxConfigContextMenuTests.swiftweb/data/cmux.schema.json
| private static func trimmedString( | ||
| forKey key: CodingKeys, | ||
| in container: KeyedDecodingContainer<CodingKeys>, | ||
| allowBlankAsNil: Bool = false | ||
| ) throws -> String? { | ||
| guard container.contains(key) else { return nil } | ||
| let raw = try container.decode(String.self, forKey: key) | ||
| let trimmed = raw.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if trimmed.isEmpty { | ||
| if allowBlankAsNil { return nil } | ||
| throw DecodingError.dataCorruptedError( | ||
| forKey: key, | ||
| in: container, | ||
| debugDescription: "\(key.stringValue) must not be blank" | ||
| ) | ||
| } | ||
| return trimmed | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consolidate the duplicated trimmedString/requiredTrimmedString helpers.
Each new type re-implements its own private trimmedString(forKey:in:allowBlankAsNil:) (and several also requiredTrimmedString) keyed by its own CodingKeys. The logic is identical and there are already similar copies in CmuxConfigButtonPlacement, CmuxConfigContextMenuActionItem, and CmuxConfigContextMenuItem. A single generic helper
extension KeyedDecodingContainer {
func decodeTrimmedString(forKey key: Key, allowBlankAsNil: Bool = false) throws -> String? { ... }
func decodeRequiredTrimmedString(forKey key: Key) throws -> String { ... }
}would remove ~5 redundant copies and keep the error messages consistent.
Also applies to: 187-220, 261-294, 421-438, 505-520
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/CmuxConfigUI.swift` around lines 92 - 109, Create a single reusable
decoding helper by adding an extension on KeyedDecodingContainer with two
methods: decodeTrimmedString(forKey:allowBlankAsNil:) -> String? and
decodeRequiredTrimmedString(forKey:) -> String that encapsulate the trimming,
blank-as-nil handling, and DecodingError.dataCorruptedError construction (use
key.stringValue in messages). Replace the duplicate private static
trimmedString/requiredTrimmedString helpers in types CmuxConfigButtonPlacement,
CmuxConfigContextMenuActionItem, CmuxConfigContextMenuItem (and any other files
noted) to call these new extension methods instead, keeping behavior and error
messages identical. Ensure the extension uses the container’s Key generic (Key:
CodingKey) so it works with each type’s CodingKeys and update callers to remove
the old private helpers.
| import AppKit | ||
| import Foundation | ||
|
|
||
| struct ConfiguredMenuBarRuntimeContext { | ||
| var menus: [CmuxResolvedMenuBarMenu] | ||
| var extensions: [CmuxResolvedMenuBarExtension] | ||
| var configStore: CmuxConfigStore? | ||
| var workingDirectory: String | ||
| } | ||
|
|
||
| private enum ConfiguredMenuBarDynamicPhase: String, Sendable { | ||
| case idle | ||
| case running | ||
| case loaded | ||
| case failed | ||
| } | ||
|
|
||
| private struct ConfiguredMenuBarDynamicState: Sendable { | ||
| var phase: ConfiguredMenuBarDynamicPhase = .idle | ||
| var items: [CmuxResolvedMenuBarItem] = [] | ||
| var error: String? | ||
| var lastRunAt: Date? | ||
| var durationMS: Int? | ||
| var exitStatus: Int32? | ||
| var activePID: Int32? | ||
| var activeRunID: UUID? | ||
| var generatedItemCount: Int? | ||
| var command: String? | ||
| var lastConfigRevisionRun: UInt64? | ||
| } | ||
|
|
||
| private struct ConfiguredMenuBarDynamicCommandResult: Sendable { | ||
| var stdout: String | ||
| var stderr: String | ||
| var exitStatus: Int32? | ||
| var durationMS: Int | ||
| var errorMessage: String? | ||
| } | ||
|
|
||
| private final class ConfiguredMenuBarOutputCollector: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private let limit: Int | ||
| private var data = Data() | ||
| private var exceededLimit = false | ||
|
|
||
| init(limit: Int) { | ||
| self.limit = limit | ||
| } | ||
|
|
||
| func append(_ chunk: Data) { | ||
| guard !chunk.isEmpty else { return } | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| if data.count + chunk.count > limit { | ||
| exceededLimit = true | ||
| let remaining = max(0, limit - data.count) | ||
| if remaining > 0 { | ||
| data.append(chunk.prefix(remaining)) | ||
| } | ||
| return | ||
| } | ||
| data.append(chunk) | ||
| } | ||
|
|
||
| func string() -> String { | ||
| lock.lock() | ||
| let snapshot = data | ||
| lock.unlock() | ||
| return String(data: snapshot, encoding: .utf8) | ||
| ?? String(decoding: snapshot, as: UTF8.self) | ||
| } | ||
|
|
||
| func didExceedLimit() -> Bool { | ||
| lock.lock() | ||
| let exceeded = exceededLimit | ||
| lock.unlock() | ||
| return exceeded | ||
| } | ||
| } | ||
|
|
||
| private enum ConfiguredMenuBarDynamicRunner { | ||
| static let defaultTimeoutSeconds: Double = 5 | ||
| static let outputLimitBytes = 256 * 1024 | ||
|
|
||
| static func run( | ||
| command: String, | ||
| cwd: String, | ||
| timeoutSeconds: Double, | ||
| onStarted: @escaping (Int32) -> Void | ||
| ) async -> ConfiguredMenuBarDynamicCommandResult { | ||
| await withCheckedContinuation { continuation in | ||
| DispatchQueue.global(qos: .utility).async { | ||
| runBlocking( | ||
| command: command, | ||
| cwd: cwd, | ||
| timeoutSeconds: timeoutSeconds, | ||
| onStarted: onStarted, | ||
| continuation: continuation | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static func runBlocking( | ||
| command: String, | ||
| cwd: String, | ||
| timeoutSeconds: Double, | ||
| onStarted: @escaping (Int32) -> Void, | ||
| continuation: CheckedContinuation<ConfiguredMenuBarDynamicCommandResult, Never> | ||
| ) { | ||
| let startedAt = Date() | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/bin/bash") | ||
| process.arguments = ["-lc", command] | ||
| process.currentDirectoryURL = URL(fileURLWithPath: cwd, isDirectory: true) | ||
|
|
||
| let stdoutPipe = Pipe() | ||
| let stderrPipe = Pipe() | ||
| let stdout = ConfiguredMenuBarOutputCollector(limit: outputLimitBytes) | ||
| let stderr = ConfiguredMenuBarOutputCollector(limit: outputLimitBytes) | ||
| stdoutPipe.fileHandleForReading.readabilityHandler = { handle in | ||
| stdout.append(handle.availableData) | ||
| } | ||
| stderrPipe.fileHandleForReading.readabilityHandler = { handle in | ||
| stderr.append(handle.availableData) | ||
| } | ||
| process.standardOutput = stdoutPipe | ||
| process.standardError = stderrPipe | ||
|
|
||
| let finishLock = NSLock() | ||
| var didFinish = false | ||
| var didTimeOut = false | ||
| let timer = DispatchSource.makeTimerSource(queue: DispatchQueue.global(qos: .utility)) | ||
|
|
||
| func finish(_ result: ConfiguredMenuBarDynamicCommandResult) { | ||
| finishLock.lock() | ||
| guard !didFinish else { | ||
| finishLock.unlock() | ||
| return | ||
| } | ||
| didFinish = true | ||
| finishLock.unlock() | ||
| timer.cancel() | ||
| continuation.resume(returning: result) | ||
| } | ||
|
|
||
| process.terminationHandler = { terminatedProcess in | ||
| stdoutPipe.fileHandleForReading.readabilityHandler = nil | ||
| stderrPipe.fileHandleForReading.readabilityHandler = nil | ||
| stdout.append(stdoutPipe.fileHandleForReading.readDataToEndOfFile()) | ||
| stderr.append(stderrPipe.fileHandleForReading.readDataToEndOfFile()) | ||
| let durationMS = max(0, Int(Date().timeIntervalSince(startedAt) * 1000)) | ||
| finishLock.lock() | ||
| let timedOut = didTimeOut | ||
| finishLock.unlock() | ||
|
|
||
| let stdoutText = stdout.string() | ||
| let stderrText = stderr.string() | ||
| let errorMessage: String? | ||
| if timedOut { | ||
| errorMessage = String( | ||
| format: String( | ||
| localized: "menuBar.dynamic.error.timeout", | ||
| defaultValue: "Command timed out after %.1f seconds." | ||
| ), | ||
| timeoutSeconds | ||
| ) | ||
| } else if stdout.didExceedLimit() || stderr.didExceedLimit() { | ||
| errorMessage = String( | ||
| format: String( | ||
| localized: "menuBar.dynamic.error.outputLimit", | ||
| defaultValue: "Command output exceeded %d bytes." | ||
| ), | ||
| outputLimitBytes | ||
| ) | ||
| } else if terminatedProcess.terminationStatus != 0 { | ||
| let detail = stderrText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | ||
| ? stdoutText.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| : stderrText.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| errorMessage = [ | ||
| String( | ||
| format: String( | ||
| localized: "menuBar.dynamic.error.exitStatus", | ||
| defaultValue: "Command exited with status %d." | ||
| ), | ||
| Int(terminatedProcess.terminationStatus) | ||
| ), | ||
| detail | ||
| ].filter { !$0.isEmpty }.joined(separator: "\n") | ||
| } else { | ||
| errorMessage = nil | ||
| } | ||
|
|
||
| finish(ConfiguredMenuBarDynamicCommandResult( | ||
| stdout: stdoutText, | ||
| stderr: stderrText, | ||
| exitStatus: terminatedProcess.terminationStatus, | ||
| durationMS: durationMS, | ||
| errorMessage: errorMessage | ||
| )) | ||
| } | ||
|
|
||
| do { | ||
| try process.run() | ||
| onStarted(process.processIdentifier) | ||
| } catch { | ||
| let durationMS = max(0, Int(Date().timeIntervalSince(startedAt) * 1000)) | ||
| stdoutPipe.fileHandleForReading.readabilityHandler = nil | ||
| stderrPipe.fileHandleForReading.readabilityHandler = nil | ||
| finish(ConfiguredMenuBarDynamicCommandResult( | ||
| stdout: "", | ||
| stderr: "", | ||
| exitStatus: nil, | ||
| durationMS: durationMS, | ||
| errorMessage: error.localizedDescription | ||
| )) | ||
| return | ||
| } | ||
|
|
||
| timer.schedule(deadline: .now() + max(0.1, timeoutSeconds)) | ||
| timer.setEventHandler { | ||
| finishLock.lock() | ||
| guard !didFinish else { | ||
| finishLock.unlock() | ||
| return | ||
| } | ||
| didTimeOut = true | ||
| finishLock.unlock() | ||
| process.terminate() | ||
| } | ||
| timer.resume() | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| final class ConfiguredMenuBarController: NSObject { | ||
| private final class ActionBox: NSObject { | ||
| let action: CmuxResolvedConfigAction | ||
|
|
||
| init(action: CmuxResolvedConfigAction) { | ||
| self.action = action | ||
| } | ||
| } | ||
|
|
||
| private final class DynamicSourceBox: NSObject { | ||
| let sourceID: String | ||
|
|
||
| init(sourceID: String) { | ||
| self.sourceID = sourceID | ||
| } | ||
| } | ||
|
|
||
| private final class DynamicErrorBox: NSObject { | ||
| let error: String | ||
|
|
||
| init(error: String) { | ||
| self.error = error | ||
| } | ||
| } | ||
|
|
||
| private final class DynamicMenuDelegate: NSObject, NSMenuDelegate { | ||
| weak var owner: ConfiguredMenuBarController? | ||
| let sourceID: String | ||
| weak var preferredWindow: NSWindow? | ||
|
|
||
| init(owner: ConfiguredMenuBarController, sourceID: String, preferredWindow: NSWindow?) { | ||
| self.owner = owner | ||
| self.sourceID = sourceID | ||
| self.preferredWindow = preferredWindow | ||
| } | ||
|
|
||
| func menuWillOpen(_ menu: NSMenu) { | ||
| owner?.dynamicMenuWillOpen(sourceID: sourceID, preferredWindow: preferredWindow) | ||
| } | ||
| } | ||
|
|
||
| private enum DynamicRefreshReason { | ||
| case open | ||
| case manual | ||
| case configReload | ||
| case interval | ||
| } | ||
|
|
||
| private weak var owner: AppDelegate? | ||
| private let notificationCenter: NotificationCenter | ||
| private var observerTokens: [NSObjectProtocol] = [] | ||
| private var topLevelItems: [NSMenuItem] = [] | ||
| private var extensionItems: [NSMenuItem] = [] | ||
| private var actionBoxes: [ActionBox] = [] | ||
| private var dynamicSourceBoxes: [DynamicSourceBox] = [] | ||
| private var dynamicErrorBoxes: [DynamicErrorBox] = [] | ||
| private var menuDelegates: [DynamicMenuDelegate] = [] | ||
| private var dynamicStates: [String: ConfiguredMenuBarDynamicState] = [:] | ||
| private var dynamicSourceByID: [String: CmuxResolvedMenuBarDynamicSource] = [:] | ||
| private var dynamicMenus: [String: NSMenu] = [:] | ||
| private var dynamicTimers: [String: DispatchSourceTimer] = [:] | ||
| private var dynamicTimerKeys: [String: String] = [:] | ||
| private var dynamicTasks: [String: Task<Void, Never>] = [:] | ||
| private var refreshScheduled = false | ||
|
|
||
| init(owner: AppDelegate, notificationCenter: NotificationCenter = .default) { | ||
| self.owner = owner | ||
| self.notificationCenter = notificationCenter | ||
| super.init() | ||
| } | ||
|
|
||
| deinit { | ||
| for token in observerTokens { | ||
| notificationCenter.removeObserver(token) | ||
| } | ||
| for timer in dynamicTimers.values { | ||
| timer.cancel() | ||
| } | ||
| for task in dynamicTasks.values { | ||
| task.cancel() | ||
| } | ||
| } | ||
|
|
||
| func installObserversIfNeeded() { | ||
| guard observerTokens.isEmpty else { return } | ||
| let names: [Notification.Name] = [ | ||
| .cmuxConfigStoreDidChange, | ||
| .mainWindowContextsDidChange, | ||
| NSWindow.didBecomeKeyNotification, | ||
| NSWindow.didBecomeMainNotification, | ||
| ] | ||
| observerTokens = names.map { name in | ||
| notificationCenter.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in | ||
| MainActor.assumeIsolated { | ||
| self?.scheduleRefresh() | ||
| } | ||
| } | ||
| } | ||
| scheduleRefresh() | ||
| } | ||
|
|
||
| func taskManagerPayload() -> [[String: Any]] { | ||
| dynamicSourceByID.values | ||
| .sorted { lhs, rhs in | ||
| if lhs.title == rhs.title { return lhs.id < rhs.id } | ||
| return lhs.title < rhs.title | ||
| } | ||
| .map { source in | ||
| let state = dynamicStates[source.id] ?? ConfiguredMenuBarDynamicState() | ||
| var detailParts: [String] = [phaseLabel(state.phase)] | ||
| if let generatedItemCount = state.generatedItemCount { | ||
| detailParts.append(String( | ||
| format: String( | ||
| localized: "taskManager.dynamicMenu.items", | ||
| defaultValue: "%d items" | ||
| ), | ||
| generatedItemCount | ||
| )) | ||
| } | ||
| if let durationMS = state.durationMS { | ||
| detailParts.append(String( | ||
| format: String( | ||
| localized: "taskManager.dynamicMenu.duration", | ||
| defaultValue: "%d ms" | ||
| ), | ||
| durationMS | ||
| )) | ||
| } | ||
| let pids = state.activePID.map { [Int($0)] } ?? [] | ||
| return [ | ||
| "id": source.id, | ||
| "title": source.title, | ||
| "detail": detailParts.joined(separator: " / "), | ||
| "state": state.phase.rawValue, | ||
| "active_pid": state.activePID.map { Int($0) } as Any? ?? NSNull(), | ||
| "root_pids": pids, | ||
| "pids": pids, | ||
| "source_path": source.settingSourcePath as Any? ?? NSNull(), | ||
| "resources": CmuxTaskManagerResources.zeroPayload | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| private func scheduleRefresh() { | ||
| guard !refreshScheduled else { return } | ||
| refreshScheduled = true | ||
| DispatchQueue.main.async { [weak self] in | ||
| MainActor.assumeIsolated { | ||
| guard let self else { return } | ||
| self.refreshScheduled = false | ||
| self.refresh() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func refresh() { | ||
| guard let mainMenu = NSApp.mainMenu else { return } | ||
|
|
||
| removeConfiguredItems(from: mainMenu) | ||
|
|
||
| let preferredWindow = NSApp.keyWindow ?? NSApp.mainWindow | ||
| let runtime = owner?.configuredMenuBarRuntimeContext(preferredWindow: preferredWindow) | ||
| let menus = runtime?.menus ?? [] | ||
| let extensions = runtime?.extensions ?? [] | ||
| guard !menus.isEmpty || !extensions.isEmpty else { | ||
| resetDynamicSources() | ||
| return | ||
| } | ||
|
|
||
| var insertionIndex = insertionIndex(in: mainMenu) | ||
| var customMenusByConfigID: [String: NSMenu] = [:] | ||
| for menu in menus { | ||
| let item = NSMenuItem(title: menu.title, action: nil, keyEquivalent: "") | ||
| let submenu = configuredMenu(from: menu, preferredWindow: preferredWindow) | ||
| item.submenu = submenu | ||
| mainMenu.insertItem(item, at: insertionIndex) | ||
| topLevelItems.append(item) | ||
| customMenusByConfigID[menu.configID] = submenu | ||
| insertionIndex += 1 | ||
| } | ||
|
|
||
| for menuExtension in extensions { | ||
| guard let targetMenu = targetMenu( | ||
| for: menuExtension.targetID, | ||
| mainMenu: mainMenu, | ||
| customMenusByConfigID: customMenusByConfigID | ||
| ) else { | ||
| continue | ||
| } | ||
| let items = menuItems(from: menuExtension.items, preferredWindow: preferredWindow) | ||
| guard !items.isEmpty else { continue } | ||
| if !targetMenu.items.isEmpty, items.first?.isSeparatorItem == false { | ||
| let separator = NSMenuItem.separator() | ||
| targetMenu.addItem(separator) | ||
| extensionItems.append(separator) | ||
| } | ||
| for item in items { | ||
| targetMenu.addItem(item) | ||
| extensionItems.append(item) | ||
| } | ||
| } | ||
|
|
||
| dynamicStates = dynamicStates.filter { | ||
| dynamicSourceByID[$0.key] != nil | ||
| } | ||
| cancelRemovedDynamicTimers() | ||
| cancelRemovedDynamicTasks() | ||
| scheduleIntervalDynamicSourcesIfNeeded(store: runtime?.configStore, preferredWindow: preferredWindow) | ||
| runConfigReloadDynamicSourcesIfNeeded(store: runtime?.configStore, preferredWindow: preferredWindow) | ||
| } | ||
|
|
||
| private func removeConfiguredItems(from mainMenu: NSMenu) { | ||
| for item in extensionItems { | ||
| let menu = item.menu | ||
| let index = menu?.index(of: item) ?? -1 | ||
| if let menu, index >= 0 { | ||
| menu.removeItem(at: index) | ||
| } | ||
| } | ||
| extensionItems.removeAll() | ||
|
|
||
| for item in topLevelItems { | ||
| let index = mainMenu.index(of: item) | ||
| if index >= 0 { | ||
| mainMenu.removeItem(at: index) | ||
| } | ||
| } | ||
| topLevelItems.removeAll() | ||
| actionBoxes.removeAll() | ||
| dynamicSourceBoxes.removeAll() | ||
| dynamicErrorBoxes.removeAll() | ||
| menuDelegates.removeAll() | ||
| dynamicSourceByID.removeAll() | ||
| dynamicMenus.removeAll() | ||
| } | ||
|
|
||
| private func resetDynamicSources() { | ||
| dynamicStates.removeAll() | ||
| for sourceID in Array(dynamicTimers.keys) { | ||
| cancelDynamicTimer(sourceID: sourceID) | ||
| } | ||
| for sourceID in Array(dynamicTasks.keys) { | ||
| cancelDynamicTask(sourceID: sourceID) | ||
| } | ||
| } | ||
|
|
||
| private func insertionIndex(in mainMenu: NSMenu) -> Int { | ||
| let notificationsTitle = String(localized: "menu.notifications.title", defaultValue: "Notifications") | ||
| if let notificationsIndex = mainMenu.items.lastIndex(where: { $0.title == notificationsTitle }) { | ||
| return notificationsIndex + 1 | ||
| } | ||
| #if DEBUG | ||
| if let debugIndex = mainMenu.items.lastIndex(where: { $0.title == "Debug" }) { | ||
| return debugIndex | ||
| } | ||
| #endif | ||
| return min(mainMenu.items.count, max(1, mainMenu.items.count - 1)) | ||
| } | ||
|
|
||
| private func configuredMenu( | ||
| from menu: CmuxResolvedMenuBarMenu, | ||
| preferredWindow: NSWindow? | ||
| ) -> NSMenu { | ||
| let nsMenu = NSMenu(title: menu.title) | ||
| for item in menuItems(from: menu.items, preferredWindow: preferredWindow) { | ||
| nsMenu.addItem(item) | ||
| } | ||
| return nsMenu | ||
| } | ||
|
|
||
| private func menuItems( | ||
| from items: [CmuxResolvedMenuBarItem], | ||
| preferredWindow: NSWindow? | ||
| ) -> [NSMenuItem] { | ||
| var nsItems: [NSMenuItem] = [] | ||
| for item in items { | ||
| switch item { | ||
| case .separator: | ||
| if !nsItems.isEmpty, nsItems.last?.isSeparatorItem == false { | ||
| nsItems.append(.separator()) | ||
| } | ||
| case .submenu(let submenu): | ||
| let item = NSMenuItem(title: submenu.title, action: nil, keyEquivalent: "") | ||
| item.submenu = configuredMenu(from: submenu, preferredWindow: preferredWindow) | ||
| nsItems.append(item) | ||
| case .dynamicSource(let source): | ||
| nsItems.append(dynamicSourceMenuItem(for: source, preferredWindow: preferredWindow)) | ||
| case .action(let menuAction): | ||
| let item = NSMenuItem( | ||
| title: menuAction.title, | ||
| action: #selector(performMenuItem(_:)), | ||
| keyEquivalent: "" | ||
| ) | ||
| item.target = self | ||
| let box = ActionBox(action: menuAction.action) | ||
| actionBoxes.append(box) | ||
| item.representedObject = box | ||
| item.toolTip = menuAction.tooltip | ||
| item.image = menuImage(for: menuAction.icon ?? menuAction.action.icon) | ||
| nsItems.append(item) | ||
| } | ||
| } | ||
|
|
||
| while nsItems.last?.isSeparatorItem == true { | ||
| nsItems.removeLast() | ||
| } | ||
| return nsItems | ||
| } | ||
|
|
||
| private func menuImage(for icon: CmuxButtonIcon?) -> NSImage? { | ||
| guard case .some(.symbol(let symbolName)) = icon else { return nil } | ||
| return NSImage(systemSymbolName: symbolName, accessibilityDescription: nil) | ||
| } | ||
|
|
||
| private func dynamicSourceMenuItem( | ||
| for source: CmuxResolvedMenuBarDynamicSource, | ||
| preferredWindow: NSWindow? | ||
| ) -> NSMenuItem { | ||
| var state = dynamicStates[source.id] ?? ConfiguredMenuBarDynamicState() | ||
| if state.command != nil, state.command != source.source.command { | ||
| state = ConfiguredMenuBarDynamicState() | ||
| cancelDynamicTimer(sourceID: source.id) | ||
| cancelDynamicTask(sourceID: source.id) | ||
| } | ||
| state.command = source.source.command | ||
| dynamicStates[source.id] = state | ||
| dynamicSourceByID[source.id] = source | ||
|
|
||
| let item = NSMenuItem(title: source.title, action: nil, keyEquivalent: "") | ||
| item.image = menuImage(for: source.icon) | ||
| item.toolTip = source.tooltip | ||
| let submenu = NSMenu(title: source.title) | ||
| let delegate = DynamicMenuDelegate(owner: self, sourceID: source.id, preferredWindow: preferredWindow) | ||
| submenu.delegate = delegate | ||
| menuDelegates.append(delegate) | ||
| dynamicMenus[source.id] = submenu | ||
| item.submenu = submenu | ||
| renderDynamicSourceMenu(sourceID: source.id, preferredWindow: preferredWindow) | ||
| return item | ||
| } | ||
|
|
||
| private func targetMenu( | ||
| for targetID: String, | ||
| mainMenu: NSMenu, | ||
| customMenusByConfigID: [String: NSMenu] | ||
| ) -> NSMenu? { | ||
| if let customMenu = customMenusByConfigID[targetID] { | ||
| return customMenu | ||
| } | ||
| let normalized = normalizedTargetID(targetID) | ||
| if let customMenu = customMenusByConfigID.first(where: { | ||
| normalizedTargetID($0.key) == normalized | ||
| })?.value { | ||
| return customMenu | ||
| } | ||
|
|
||
| let builtinTitles: [String: String] = [ | ||
| "application": "", | ||
| "app": "", | ||
| "cmux": "", | ||
| "file": String(localized: "menu.file.title", defaultValue: "File"), | ||
| "edit": String(localized: "menu.edit.title", defaultValue: "Edit"), | ||
| "view": String(localized: "menu.view.title", defaultValue: "View"), | ||
| "notifications": String(localized: "menu.notifications.title", defaultValue: "Notifications"), | ||
| "window": String(localized: "menu.window.title", defaultValue: "Window"), | ||
| "help": String(localized: "menu.help.title", defaultValue: "Help"), | ||
| ] | ||
| if ["application", "app", "cmux"].contains(normalized) { | ||
| return mainMenu.items.first?.submenu | ||
| } | ||
| guard let title = builtinTitles[normalized] else { return nil } | ||
| return mainMenu.items.first(where: { $0.title == title })?.submenu | ||
| } | ||
|
|
||
| private func normalizedTargetID(_ raw: String) -> String { | ||
| raw.lowercased().filter { $0.isLetter || $0.isNumber } | ||
| } | ||
|
|
||
| private func runConfigReloadDynamicSourcesIfNeeded(store: CmuxConfigStore?, preferredWindow: NSWindow?) { | ||
| guard let store else { return } | ||
| for source in dynamicSourceByID.values { | ||
| guard source.source.refresh == .onConfigReload else { continue } | ||
| let lastRevision = dynamicStates[source.id]?.lastConfigRevisionRun | ||
| guard lastRevision != store.configRevision else { continue } | ||
| dynamicStates[source.id, default: ConfiguredMenuBarDynamicState()].lastConfigRevisionRun = store.configRevision | ||
| refreshDynamicSource(sourceID: source.id, preferredWindow: preferredWindow, reason: .configReload) | ||
| } | ||
| } | ||
|
|
||
| private func scheduleIntervalDynamicSourcesIfNeeded(store: CmuxConfigStore?, preferredWindow: NSWindow?) { | ||
| guard let store else { return } | ||
| for source in dynamicSourceByID.values { | ||
| guard source.source.refresh == .interval, | ||
| let interval = source.source.intervalSeconds else { | ||
| cancelDynamicTimer(sourceID: source.id) | ||
| continue | ||
| } | ||
| let timerKey = [ | ||
| source.source.command, | ||
| String(interval), | ||
| source.settingSourcePath ?? "", | ||
| ].joined(separator: "\u{1F}") | ||
| guard dynamicTimerKeys[source.id] != timerKey else { continue } | ||
| cancelDynamicTimer(sourceID: source.id) | ||
|
|
||
| let timer = DispatchSource.makeTimerSource(queue: .main) | ||
| timer.schedule(deadline: .now() + interval, repeating: interval) | ||
| timer.setEventHandler { [weak self, weak store] in | ||
| MainActor.assumeIsolated { | ||
| guard let self, let store else { return } | ||
| guard let currentSource = self.dynamicSourceByID[source.id] else { | ||
| self.cancelDynamicTimer(sourceID: source.id) | ||
| return | ||
| } | ||
| guard CmuxConfigExecutor.isTrustedDynamicMenuSource( | ||
| command: currentSource.source.command, | ||
| sourceID: currentSource.id, | ||
| configSourcePath: currentSource.settingSourcePath, | ||
| globalConfigPath: store.globalConfigPath | ||
| ) else { | ||
| return | ||
| } | ||
| self.refreshDynamicSource( | ||
| sourceID: currentSource.id, | ||
| preferredWindow: NSApp.keyWindow ?? NSApp.mainWindow, | ||
| reason: .interval | ||
| ) | ||
| } | ||
| } | ||
| dynamicTimers[source.id] = timer | ||
| dynamicTimerKeys[source.id] = timerKey | ||
| timer.resume() | ||
| } | ||
| } | ||
|
|
||
| private func cancelRemovedDynamicTimers() { | ||
| let activeSourceIDs = Set(dynamicSourceByID.keys) | ||
| for sourceID in Array(dynamicTimers.keys) where !activeSourceIDs.contains(sourceID) { | ||
| cancelDynamicTimer(sourceID: sourceID) | ||
| } | ||
| } | ||
|
|
||
| private func cancelRemovedDynamicTasks() { | ||
| let activeSourceIDs = Set(dynamicSourceByID.keys) | ||
| for sourceID in Array(dynamicTasks.keys) where !activeSourceIDs.contains(sourceID) { | ||
| cancelDynamicTask(sourceID: sourceID) | ||
| } | ||
| } | ||
|
|
||
| private func cancelDynamicTimer(sourceID: String) { | ||
| dynamicTimers[sourceID]?.cancel() | ||
| dynamicTimers.removeValue(forKey: sourceID) | ||
| dynamicTimerKeys.removeValue(forKey: sourceID) | ||
| } | ||
|
|
||
| private func cancelDynamicTask(sourceID: String) { | ||
| dynamicTasks[sourceID]?.cancel() | ||
| dynamicTasks.removeValue(forKey: sourceID) | ||
| dynamicStates[sourceID]?.activeRunID = nil | ||
| dynamicStates[sourceID]?.activePID = nil | ||
| } | ||
|
|
||
| private func dynamicMenuWillOpen(sourceID: String, preferredWindow: NSWindow?) { | ||
| guard let source = dynamicSourceByID[sourceID] else { return } | ||
| renderDynamicSourceMenu(sourceID: sourceID, preferredWindow: preferredWindow) | ||
| if (source.source.refresh ?? .onOpen) == .onOpen { | ||
| refreshDynamicSource(sourceID: sourceID, preferredWindow: preferredWindow, reason: .open) | ||
| } | ||
| } | ||
|
|
||
| private func renderDynamicSourceMenu(sourceID: String, preferredWindow: NSWindow?) { | ||
| guard let menu = dynamicMenus[sourceID], | ||
| let source = dynamicSourceByID[sourceID] else { return } | ||
| let state = dynamicStates[sourceID] ?? ConfiguredMenuBarDynamicState() | ||
| menu.removeAllItems() | ||
|
|
||
| let cachedItems = menuItems(from: state.items, preferredWindow: preferredWindow) | ||
| for item in cachedItems { | ||
| menu.addItem(item) | ||
| } | ||
| if cachedItems.isEmpty { | ||
| menu.addItem(disabledItem(title: dynamicMenuEmptyTitle(for: state))) | ||
| } | ||
|
|
||
| if state.phase == .running { | ||
| addSeparatorIfNeeded(to: menu) | ||
| menu.addItem(disabledItem(title: String( | ||
| localized: "menuBar.dynamic.running", | ||
| defaultValue: "Loading..." | ||
| ))) | ||
| } | ||
|
|
||
| if let error = state.error, !error.isEmpty { | ||
| addSeparatorIfNeeded(to: menu) | ||
| menu.addItem(disabledItem(title: String( | ||
| localized: "menuBar.dynamic.failed", | ||
| defaultValue: "Dynamic menu failed" | ||
| ))) | ||
| let copyItem = NSMenuItem( | ||
| title: String(localized: "menuBar.dynamic.copyError", defaultValue: "Copy Error"), | ||
| action: #selector(copyDynamicSourceError(_:)), | ||
| keyEquivalent: "" | ||
| ) | ||
| copyItem.target = self | ||
| let box = DynamicErrorBox(error: error) | ||
| dynamicErrorBoxes.append(box) | ||
| copyItem.representedObject = box | ||
| menu.addItem(copyItem) | ||
| } | ||
|
|
||
| addSeparatorIfNeeded(to: menu) | ||
| let reloadTitle = state.phase == .idle && (source.source.refresh ?? .onOpen) == .manual | ||
| ? String(localized: "menuBar.dynamic.load", defaultValue: "Load Dynamic Menu") | ||
| : String(localized: "menuBar.dynamic.reload", defaultValue: "Reload") | ||
| let reloadItem = NSMenuItem( | ||
| title: reloadTitle, | ||
| action: #selector(reloadDynamicSource(_:)), | ||
| keyEquivalent: "" | ||
| ) | ||
| reloadItem.target = self | ||
| reloadItem.isEnabled = state.phase != .running | ||
| let box = DynamicSourceBox(sourceID: sourceID) | ||
| dynamicSourceBoxes.append(box) | ||
| reloadItem.representedObject = box | ||
| menu.addItem(reloadItem) | ||
| } | ||
|
|
||
| private func disabledItem(title: String) -> NSMenuItem { | ||
| let item = NSMenuItem(title: title, action: nil, keyEquivalent: "") | ||
| item.isEnabled = false | ||
| return item | ||
| } | ||
|
|
||
| private func dynamicMenuEmptyTitle(for state: ConfiguredMenuBarDynamicState) -> String { | ||
| switch state.phase { | ||
| case .running: | ||
| return String(localized: "menuBar.dynamic.loading", defaultValue: "Loading...") | ||
| case .failed where state.items.isEmpty: | ||
| return String(localized: "menuBar.dynamic.noCachedItems", defaultValue: "No cached items") | ||
| case .loaded: | ||
| return String(localized: "menuBar.dynamic.noItems", defaultValue: "No items") | ||
| case .idle, .failed: | ||
| return String(localized: "menuBar.dynamic.notLoaded", defaultValue: "Not loaded") | ||
| } | ||
| } | ||
|
|
||
| private func addSeparatorIfNeeded(to menu: NSMenu) { | ||
| if !menu.items.isEmpty, menu.items.last?.isSeparatorItem == false { | ||
| menu.addItem(.separator()) | ||
| } | ||
| } | ||
|
|
||
| private func refreshDynamicSource( | ||
| sourceID: String, | ||
| preferredWindow: NSWindow?, | ||
| reason: DynamicRefreshReason | ||
| ) { | ||
| guard let source = dynamicSourceByID[sourceID] else { return } | ||
| guard dynamicStates[sourceID]?.phase != .running else { return } | ||
| guard let store = owner?.configuredMenuBarRuntimeContext(preferredWindow: preferredWindow).configStore else { return } | ||
|
|
||
| let title = String( | ||
| format: String( | ||
| localized: "dialog.cmuxConfig.confirmDynamicMenu.title", | ||
| defaultValue: "Run Dynamic Menu Source: %@" | ||
| ), | ||
| source.title | ||
| ) | ||
| let authorized = CmuxConfigExecutor.authorizeDynamicMenuSourceIfNeeded( | ||
| command: source.source.command, | ||
| sourceID: source.id, | ||
| configSourcePath: source.settingSourcePath, | ||
| globalConfigPath: store.globalConfigPath, | ||
| displayTitle: title, | ||
| presentingWindow: preferredWindow | ||
| ) { [weak self, weak store, weak preferredWindow] command in | ||
| guard let self, let store else { return } | ||
| self.startDynamicSource( | ||
| source, | ||
| command: command, | ||
| store: store, | ||
| preferredWindow: preferredWindow, | ||
| reason: reason | ||
| ) | ||
| } | ||
| if !authorized { | ||
| var state = dynamicStates[sourceID] ?? ConfiguredMenuBarDynamicState() | ||
| state.phase = state.items.isEmpty ? .idle : .loaded | ||
| dynamicStates[sourceID] = state | ||
| renderDynamicSourceMenu(sourceID: sourceID, preferredWindow: preferredWindow) | ||
| } | ||
| } | ||
|
|
||
| private func startDynamicSource( | ||
| _ source: CmuxResolvedMenuBarDynamicSource, | ||
| command: String, | ||
| store: CmuxConfigStore, | ||
| preferredWindow: NSWindow?, | ||
| reason: DynamicRefreshReason | ||
| ) { | ||
| let runID = UUID() | ||
| var state = dynamicStates[source.id] ?? ConfiguredMenuBarDynamicState() | ||
| state.phase = .running | ||
| state.error = nil | ||
| state.command = command | ||
| state.lastRunAt = Date() | ||
| state.activeRunID = runID | ||
| state.activePID = nil | ||
| dynamicStates[source.id] = state | ||
| renderDynamicSourceMenu(sourceID: source.id, preferredWindow: preferredWindow) | ||
|
|
||
| let runtime = owner?.configuredMenuBarRuntimeContext(preferredWindow: preferredWindow) | ||
| let workingDirectory = runtime?.workingDirectory ?? FileManager.default.homeDirectoryForCurrentUser.path | ||
| let timeout = source.source.timeoutSeconds ?? ConfiguredMenuBarDynamicRunner.defaultTimeoutSeconds | ||
|
|
||
| cancelDynamicTask(sourceID: source.id) | ||
| dynamicStates[source.id, default: ConfiguredMenuBarDynamicState()].activeRunID = runID | ||
| dynamicStates[source.id, default: ConfiguredMenuBarDynamicState()].phase = .running | ||
|
|
||
| dynamicTasks[source.id] = Task { @MainActor [weak self, weak store, weak preferredWindow] in | ||
| let sourceID = source.id | ||
| let result = await ConfiguredMenuBarDynamicRunner.run( | ||
| command: command, | ||
| cwd: workingDirectory, | ||
| timeoutSeconds: timeout | ||
| ) { [weak self, sourceID, runID, weak preferredWindow] pid in | ||
| DispatchQueue.main.async { | ||
| MainActor.assumeIsolated { | ||
| guard let self else { return } | ||
| guard self.dynamicStates[sourceID]?.activeRunID == runID else { return } | ||
| self.dynamicStates[sourceID, default: ConfiguredMenuBarDynamicState()].activePID = pid | ||
| self.renderDynamicSourceMenu(sourceID: sourceID, preferredWindow: preferredWindow) | ||
| } | ||
| } | ||
| } | ||
| guard !Task.isCancelled else { return } | ||
| guard let self, let store else { return } | ||
| guard self.dynamicStates[sourceID]?.activeRunID == runID else { return } | ||
| self.finishDynamicSource( | ||
| source, | ||
| runID: runID, | ||
| result: result, | ||
| store: store, | ||
| preferredWindow: preferredWindow | ||
| ) | ||
| self.dynamicTasks.removeValue(forKey: sourceID) | ||
| } | ||
| } | ||
|
|
||
| private func finishDynamicSource( | ||
| _ source: CmuxResolvedMenuBarDynamicSource, | ||
| runID: UUID, | ||
| result: ConfiguredMenuBarDynamicCommandResult, | ||
| store: CmuxConfigStore, | ||
| preferredWindow: NSWindow? | ||
| ) { | ||
| guard dynamicStates[source.id]?.activeRunID == runID else { return } | ||
| var state = dynamicStates[source.id] ?? ConfiguredMenuBarDynamicState() | ||
| state.activeRunID = nil | ||
| state.activePID = nil | ||
| state.durationMS = result.durationMS | ||
| state.exitStatus = result.exitStatus | ||
|
|
||
| if let error = result.errorMessage { | ||
| state.phase = .failed | ||
| state.error = error | ||
| dynamicStates[source.id] = state | ||
| renderDynamicSourceMenu(sourceID: source.id, preferredWindow: preferredWindow) | ||
| owner?.notifyConfiguredMenuBarDynamicFailure(source: source, error: error, preferredWindow: preferredWindow) | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| let data = Data(result.stdout.utf8) | ||
| let generatedItems = try JSONDecoder().decode([CmuxConfigMenuBarItem].self, from: data) | ||
| let resolved = store.resolveGeneratedMenuBarItems( | ||
| generatedItems, | ||
| settingName: "\(source.settingName).generated", | ||
| settingSourcePath: source.settingSourcePath | ||
| ) | ||
| if let issue = resolved.issues.first { | ||
| throw NSError(domain: "CmuxDynamicMenu", code: 1, userInfo: [ | ||
| NSLocalizedDescriptionKey: issue.logMessage | ||
| ]) | ||
| } | ||
| state.phase = .loaded | ||
| state.items = resolved.items | ||
| state.error = nil | ||
| state.generatedItemCount = resolved.items.count | ||
| } catch { | ||
| state.phase = .failed | ||
| state.error = error.localizedDescription | ||
| owner?.notifyConfiguredMenuBarDynamicFailure( | ||
| source: source, | ||
| error: error.localizedDescription, | ||
| preferredWindow: preferredWindow | ||
| ) | ||
| } | ||
| dynamicStates[source.id] = state | ||
| renderDynamicSourceMenu(sourceID: source.id, preferredWindow: preferredWindow) | ||
| } | ||
|
|
||
| private func phaseLabel(_ phase: ConfiguredMenuBarDynamicPhase) -> String { | ||
| switch phase { | ||
| case .idle: | ||
| return String(localized: "taskManager.dynamicMenu.idle", defaultValue: "Idle") | ||
| case .running: | ||
| return String(localized: "taskManager.dynamicMenu.running", defaultValue: "Running") | ||
| case .loaded: | ||
| return String(localized: "taskManager.dynamicMenu.loaded", defaultValue: "Loaded") | ||
| case .failed: | ||
| return String(localized: "taskManager.dynamicMenu.failed", defaultValue: "Failed") | ||
| } | ||
| } | ||
|
|
||
| @objc private func performMenuItem(_ sender: NSMenuItem) { | ||
| guard let box = sender.representedObject as? ActionBox else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| guard owner?.performConfiguredMenuBarAction(box.action, preferredWindow: NSApp.keyWindow ?? NSApp.mainWindow) == true else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| } | ||
|
|
||
| @objc private func reloadDynamicSource(_ sender: NSMenuItem) { | ||
| guard let box = sender.representedObject as? DynamicSourceBox else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| refreshDynamicSource( | ||
| sourceID: box.sourceID, | ||
| preferredWindow: NSApp.keyWindow ?? NSApp.mainWindow, | ||
| reason: .manual | ||
| ) | ||
| } | ||
|
|
||
| @objc private func copyDynamicSourceError(_ sender: NSMenuItem) { | ||
| guard let box = sender.representedObject as? DynamicErrorBox else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| NSPasteboard.general.clearContents() | ||
| NSPasteboard.general.setString(box.error, forType: .string) | ||
| } | ||
| } |
There was a problem hiding this comment.
Split this 982-line file: extract runner/output-collector into a SwiftPM package.
This new production Swift file is 982 lines and mixes multiple distinct responsibilities in one place: subprocess execution (ConfiguredMenuBarDynamicRunner, lines 81–233), thread-safe stdout/stderr buffering (ConfiguredMenuBarOutputCollector, lines 40–79), AppKit NSMenu rendering and mutation (lines 405–575, 698–761), runtime dynamic-source state ownership, JSON parsing of generated items (lines 908–924), and Task Manager payload shaping (lines 337–377). Both the runner and the output collector are pure Foundation code that can compile and be unit-tested without AppKit, AppDelegate, or any cmux-app singletons, so they qualify for isolation in a small SwiftPM target where they can have their own fakes/fixtures.
Recommend, at minimum, splitting along these seams in this PR:
ConfiguredMenuBarOutputCollector→ its own file (or a package target likeCmuxSubprocess).ConfiguredMenuBarDynamicRunner+ConfiguredMenuBarDynamicCommandResult→ same package target; this is reusable subprocess infrastructure with no UI/state coupling.- Keep
ConfiguredMenuBarController(AppKit/state/UI glue) inSources/.
That brings the controller well under the 800-line ceiling and matches the rule that reusable, lifecycle-independent domain logic lives behind a SwiftPM target.
As per coding guidelines: "do not accept a new production Swift file over 400 lines without a clear single responsibility, or over 800 lines even when the responsibility is mostly coherent", "Flag Swift files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place", and "Flag features implemented directly in the app target/module root Sources/ path when their core logic is independent of cmux app lifecycle and can compile/test without AppKit, SwiftUI view state, Ghostty globals, or process-wide singletons".
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 70-70: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
[Warning] 40-40: Classes should have an explicit deinit method
(required_deinit)
[Warning] 237-237: Classes should have an explicit deinit method
(required_deinit)
[Warning] 245-245: Classes should have an explicit deinit method
(required_deinit)
[Warning] 253-253: Classes should have an explicit deinit method
(required_deinit)
[Warning] 261-261: Classes should have an explicit deinit method
(required_deinit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ConfiguredMenuBarController.swift` around lines 1 - 982, The file
mixes subprocess/output buffering logic with AppKit UI/controller code; extract
the subprocess pieces into a separate target by moving
ConfiguredMenuBarOutputCollector, ConfiguredMenuBarDynamicCommandResult, and
ConfiguredMenuBarDynamicRunner out of ConfiguredMenuBarController into their own
Swift source(s) in a new SwiftPM target (e.g., CmuxSubprocess). Update the
types' access levels to public/internal as needed so ConfiguredMenuBarController
can still call ConfiguredMenuBarDynamicRunner.run and use
ConfiguredMenuBarDynamicCommandResult/ConfiguredMenuBarOutputCollector, add the
new package import where the controller references these symbols, and leave
ConfiguredMenuBarController (and all AppKit-dependent logic) in the existing app
module. Ensure you preserve behavior: keep the same APIs (function signatures,
constants like defaultTimeoutSeconds and outputLimitBytes) and tests can be
added against the new package.
| timer.schedule(deadline: .now() + max(0.1, timeoutSeconds)) | ||
| timer.setEventHandler { | ||
| finishLock.lock() | ||
| guard !didFinish else { | ||
| finishLock.unlock() | ||
| return | ||
| } | ||
| didTimeOut = true | ||
| finishLock.unlock() | ||
| process.terminate() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
🧩 Analysis chain
🌐 Web query:
macOS Swift Process spawn new process group setpgid posix_spawn
💡 Result:
In POSIX posix_spawn, “set a new process group” is done via the spawn attributes object: set the spawn-flags bit POSIX_SPAWN_SETPGROUP and set spawn-pgroup (often to 0) with posix_spawnattr_setpgroup(). When the flag is set and spawn-pgroup is 0, the child is placed in a new process group with a PGID equal to its PID (equivalent to setpgid(pid=0, pgid=pid)-style behavior). [1] How this maps to the APIs you’d use from Swift on macOS 1) Initialize a posix_spawnattr_t with posix_spawnattr_init(). [2] 2) Enable process-group setting by calling posix_spawnattr_setflags(..., POSIX_SPAWN_SETPGROUP); that flag makes the child behave “as if the setpgid(2) function had been called with a pid parameter of 0 and a pgid parameter equal to the value of the spawn-pgroup attribute”. [3] 3) Set the spawn-pgroup attribute with posix_spawnattr_setpgroup(attr, pgroup). The pgroup argument is the process group the new process will join/create when POSIX_SPAWN_SETPGROUP is set; its default is 0 (which means a new process group will be created). [4] 4) Call posix_spawn() with attrp pointing at that configured attributes object. [5][1] Key semantics (authoritative) - If POSIX_SPAWN_SETPGROUP is set and spawn-pgroup is non-zero, the child’s process group is set to that specified value. [1] - Special case: if POSIX_SPAWN_SETPGROUP is set and spawn-pgroup is 0, the child is put into a new process group with PGID == child PID. [1] - If POSIX_SPAWN_SETPGROUP is not set, the child inherits the parent’s process group. [1] Swift notes - Swift itself doesn’t expose setpgid directly for Process/NSTask; the POSIX mechanism is through posix_spawnattr_setflags/posix_spawnattr_setpgroup + posix_spawn. (You can call these C functions from Swift by importing Darwin.) [3][4] - If you want the “new process group” behavior, set spawn-pgroup to 0 and set POSIX_SPAWN_SETPGROUP in the attributes flags; that yields PGID==PID for the spawned child. [1] Example (C semantics expressed as steps) - posix_spawnattr_init(&attr) - posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP) - posix_spawnattr_setpgroup(&attr, 0) // request a new pgroup; PGID==child PID [1] - posix_spawn(&pid, path, file_actions, &attr, argv, envp) [5][1] If you share your current Swift code (where you build the argv/envp and which spawn API you’re using), I can translate this into a concrete Swift/Darwin snippet that compiles.
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9799919799/functions/posix_spawn.html
- 2: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/posix_spawnattr_init.3.html
- 3: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/posix_spawnattr_setflags.3.html
- 4: https://manp.gs/mac/3/posix_spawnattr_setpgroup
- 5: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/posix_spawn.2.html
🏁 Script executed:
# First, let's look at the file and understand the context around lines 220-230
head -240 Sources/ConfiguredMenuBarController.swift | tail -50Repository: manaflow-ai/cmux
Length of output: 1615
🏁 Script executed:
# Let's see the full function containing the timeout code to understand process setup
grep -n "func\|let process\|var process\|bash" Sources/ConfiguredMenuBarController.swift | head -30Repository: manaflow-ai/cmux
Length of output: 1730
🏁 Script executed:
# Let's examine the full runBlocking function starting from line 104
sed -n '104,235p' Sources/ConfiguredMenuBarController.swiftRepository: manaflow-ai/cmux
Length of output: 5233
🏁 Script executed:
# Now let's verify the technical claim about bash signal handling
# Check if there's any existing process group handling in the codebase
rg -i "setpgid\|POSIX_SPAWN_SETPGROUP\|process.*group" Sources/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Let's check if there's any documentation or tests related to dynamic command execution
find . -name "*.swift" -type f | xargs grep -l "ConfiguredMenuBar\|dynamicCommand" | head -5Repository: manaflow-ai/cmux
Length of output: 132
🏁 Script executed:
# Let's verify the actual behavior of bash with -lc regarding signal forwarding
# Search for any existing literature or tests in the repo about this
rg -i "SIGTERM\|terminate\|signal\|bash" Sources/ConfiguredMenuBarController.swift -A 2 -B 2Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Let's check what happens after process.terminate() is called
# and verify if there's any process cleanup code
rg "terminate\|Process\|kill" Sources/ConfiguredMenuBarController.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Let's verify the actual file and see if there's any file length concern
wc -l Sources/ConfiguredMenuBarController.swiftRepository: manaflow-ai/cmux
Length of output: 106
🏁 Script executed:
# Let's check for any existing process management or signal handling patterns in the codebase
rg -i "Darwin\|Foundation.*Process\|posix" Sources/ | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Let's check the cmux custom lint rules mentioned in coding guidelines
cat .github/review-bot-rules/*.swift 2>/dev/null | head -50Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if the review-bot-rules directory exists and what's in it
ls -la .github/review-bot-rules/ 2>/dev/null || echo "Directory not found"Repository: manaflow-ai/cmux
Length of output: 1012
🏁 Script executed:
# Check the blocking runtime rule which seems relevant to this process timeout scenario
cat .github/review-bot-rules/swift-blocking-runtime.mdRepository: manaflow-ai/cmux
Length of output: 2279
🏁 Script executed:
# Let's verify the actual behavior of bash with signal forwarding
# Create a simple test to understand the claim better
cat > /tmp/test_bash_signals.sh << 'EOF'
#!/bin/bash
# This script demonstrates the signal forwarding behavior
# When bash receives SIGTERM, it may not forward to all descendants
# Example: sleep in background
bash -lc "sleep 1000 &" &
PID=$!
# Get the process group
ps -o pid,ppid,pgid,cmd | grep $PID
# Kill the parent bash
kill -TERM $PID 2>/dev/null
# Check if sleep is still running
sleep 0.5
ps aux | grep "sleep 1000" | grep -v grep || echo "Sleep process was cleaned up"
EOF
chmod +x /tmp/test_bash_signals.sh
bash /tmp/test_bash_signals.sh 2>&1 || trueRepository: manaflow-ai/cmux
Length of output: 249
🏁 Script executed:
# Let's understand how this function is being called and what commands are executed
grep -n "runBlocking\|ConfiguredMenuBarDynamicCommandResult" Sources/ConfiguredMenuBarController.swift | head -20Repository: manaflow-ai/cmux
Length of output: 609
🏁 Script executed:
# Let's verify if there are any tests or examples of how commands are actually used
grep -n "dynamicCommand\|async\|run" Sources/ConfiguredMenuBarController.swift | grep -i "func\|test" | head -10Repository: manaflow-ai/cmux
Length of output: 239
🏁 Script executed:
# Let's check if there are any integration tests or examples that show what kinds of commands are executed
find . -name "*.swift" -type f | xargs grep -l "ConfiguredMenuBar\|dynamic.*command" | xargs grep -h "command.*=" | head -10Repository: manaflow-ai/cmux
Length of output: 850
Timeout only sends SIGTERM to bash, not its child processes—consider process-group signaling for cleanup.
When process.terminate() is called on a bash process launched with -lc, it sends SIGTERM only to the bash parent. Child processes spawned within that command (e.g., sleep 1000 | jq ...) may not receive the signal and can persist as orphans after the timeout, continuing to run invisibly even though the menu has finished.
To ensure all descendants are terminated on timeout, consider starting the process as a process-group leader using posix_spawn attributes (Darwin/POSIX APIs: set POSIX_SPAWN_SETPGROUP and call posix_spawnattr_setpgroup(attr, 0) to create a new group with PGID==child PID), then signal the entire process group on timeout/cancel. This requires importing Darwin and using low-level spawn attributes since Foundation's Process class does not expose process group configuration.
This is an optional improvement for robustness in menus that execute arbitrary user-supplied shell commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ConfiguredMenuBarController.swift` around lines 220 - 230, The
timeout handler currently calls process.terminate() (see timer.setEventHandler,
finishLock, didFinish, didTimeOut) which only SIGTERM's the bash parent and can
leave child processes running; replace this by spawning the command as a new
process group using POSIX APIs (import Darwin, create posix_spawnattr_t with
POSIX_SPAWN_SETPGROUP and call posix_spawnattr_setpgroup(attr, 0) so the child
becomes a group leader) and store the child PID, then on timeout send the signal
to the whole group (use kill(-childPid, SIGTERM) and optionally SIGKILL after a
grace period) instead of calling process.terminate(); ensure cleanup paths
(cancel/normal finish) also signal the group and avoid races with
finishLock/didFinish/didTimeOut.
| private func cancelDynamicTask(sourceID: String) { | ||
| dynamicTasks[sourceID]?.cancel() | ||
| dynamicTasks.removeValue(forKey: sourceID) | ||
| dynamicStates[sourceID]?.activeRunID = nil | ||
| dynamicStates[sourceID]?.activePID = nil | ||
| } |
There was a problem hiding this comment.
Subprocess survives Task cancellation — orphaned bash runs until timeout.
cancelDynamicTask cancels the Swift Task and clears activeRunID/activePID, but the underlying Process started inside ConfiguredMenuBarDynamicRunner.run(...) is not signaled. Because run uses a plain withCheckedContinuation that resumes only on terminationHandler or the timeout timer, the bash process keeps running (and consuming CPU/output buffers) for up to timeoutSeconds after cancellation. This path is reachable from:
dynamicSourceMenuItemwhen a source'scommandchanges (line 558),resetDynamicSources()when configured menus are removed,deinit(lines 314–316) — orphan bash invocations at app teardown,- and any future caller of
cancelDynamicTask.
The runID guard at line 873 prevents stale results from corrupting state, but it does not stop the process. Recommend wiring cancellation through to process.terminate() (or kill(pid, SIGTERM)), e.g. via withTaskCancellationHandler in the runner, or by capturing the started PID in dynamicStates[...].activePID and signaling it before clearing in cancelDynamicTask.
🧹 Sketch: terminate the captured PID on cancellation
private func cancelDynamicTask(sourceID: String) {
+ if let pid = dynamicStates[sourceID]?.activePID, pid > 0 {
+ kill(pid, SIGTERM)
+ }
dynamicTasks[sourceID]?.cancel()
dynamicTasks.removeValue(forKey: sourceID)
dynamicStates[sourceID]?.activeRunID = nil
dynamicStates[sourceID]?.activePID = nil
}A cleaner alternative is for ConfiguredMenuBarDynamicRunner.run to wrap its continuation in withTaskCancellationHandler and call process.terminate() from the cancellation handler, so callers don't need PID bookkeeping.
Also applies to: 829-883
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ConfiguredMenuBarController.swift` around lines 691 - 696, The
cancelDynamicTask currently cancels the Swift Task and clears
dynamicStates[sourceID].activeRunID/activePID but never signals the underlying
Process, leaving orphaned bash processes; update cancellation to terminate the
subprocess by sending a signal to the captured PID
(dynamicStates[sourceID].activePID) before clearing it (e.g., call kill(pid,
SIGTERM) or Process.terminate()), and/or modify
ConfiguredMenuBarDynamicRunner.run to wrap its withCheckedContinuation in
withTaskCancellationHandler so the cancellation handler calls
process.terminate() (or kills the PID) to ensure the subprocess is stopped when
dynamicTasks[sourceID]?.cancel() is invoked.
| "refresh": { | ||
| "type": "string", | ||
| "enum": ["onOpen", "manual", "onConfigReload", "interval"], | ||
| "default": "onOpen", | ||
| "description": "When cmux runs this source." | ||
| }, | ||
| "timeoutSeconds": { | ||
| "type": "number", | ||
| "exclusiveMinimum": 0, | ||
| "default": 5, | ||
| "description": "Maximum runtime before cmux terminates the command." | ||
| }, | ||
| "intervalSeconds": { | ||
| "type": "number", | ||
| "minimum": 10, | ||
| "description": "Required when refresh is interval. cmux only runs interval sources automatically after the source has already been trusted or when it comes from global config." | ||
| } |
There was a problem hiding this comment.
intervalSeconds is documented as required when refresh is interval but the schema never enforces it.
The description on line 1011 states intervalSeconds is required when refresh is interval, but the schema lacks a conditional constraint, so { "command": "...", "refresh": "interval" } validates. Add an allOf/if/then (similar to the pattern used in sessionIdSource above) so users get a schema error instead of relying on runtime behavior.
Suggested fix
"menuBarSource": {
"type": "object",
"additionalProperties": false,
"required": ["command"],
"properties": {
...
"intervalSeconds": {
"type": "number",
"minimum": 10,
"description": "Required when refresh is interval. cmux only runs interval sources automatically after the source has already been trusted or when it comes from global config."
}
- }
+ },
+ "allOf": [
+ {
+ "if": {
+ "properties": { "refresh": { "const": "interval" } },
+ "required": ["refresh"]
+ },
+ "then": { "required": ["intervalSeconds"] }
+ }
+ ]
}📝 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.
| "refresh": { | |
| "type": "string", | |
| "enum": ["onOpen", "manual", "onConfigReload", "interval"], | |
| "default": "onOpen", | |
| "description": "When cmux runs this source." | |
| }, | |
| "timeoutSeconds": { | |
| "type": "number", | |
| "exclusiveMinimum": 0, | |
| "default": 5, | |
| "description": "Maximum runtime before cmux terminates the command." | |
| }, | |
| "intervalSeconds": { | |
| "type": "number", | |
| "minimum": 10, | |
| "description": "Required when refresh is interval. cmux only runs interval sources automatically after the source has already been trusted or when it comes from global config." | |
| } | |
| "refresh": { | |
| "type": "string", | |
| "enum": ["onOpen", "manual", "onConfigReload", "interval"], | |
| "default": "onOpen", | |
| "description": "When cmux runs this source." | |
| }, | |
| "timeoutSeconds": { | |
| "type": "number", | |
| "exclusiveMinimum": 0, | |
| "default": 5, | |
| "description": "Maximum runtime before cmux terminates the command." | |
| }, | |
| "intervalSeconds": { | |
| "type": "number", | |
| "minimum": 10, | |
| "description": "Required when refresh is interval. cmux only runs interval sources automatically after the source has already been trusted or when it comes from global config." | |
| } | |
| }, | |
| "allOf": [ | |
| { | |
| "if": { | |
| "properties": { "refresh": { "const": "interval" } }, | |
| "required": ["refresh"] | |
| }, | |
| "then": { "required": ["intervalSeconds"] } | |
| } | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/data/cmux.schema.json` around lines 996 - 1012, Add a JSON Schema
conditional so intervalSeconds is required when refresh equals "interval":
modify the schema block containing the "refresh" and "intervalSeconds"
properties to include an if/then (or allOf with if/then) that checks
{"properties":{"refresh":{"const":"interval"}}} and in the then clause adds
"required":["intervalSeconds"] and optionally constraints on intervalSeconds;
follow the same pattern used for sessionIdSource to locate where to insert the
conditional so the validator rejects objects missing intervalSeconds when
refresh is "interval".
|
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ConfiguredMenuBarController.swift`:
- Around line 366-372: The current call that appends the item count uses a
simple "%d items" format and the localization key
"taskManager.dynamicMenu.items", which will produce incorrect singular forms;
update the usage to employ pluralized localization entries (ICU-style .one and
.other) for "taskManager.dynamicMenu.items" and format using the plural-aware
API so that detailParts.append(...) uses the pluralized localization based on
generatedItemCount (reference the existing call site with detailParts.append and
the generatedItemCount variable) ensuring the .one and .other keys are present
in the strings file and the runtime selects the correct plural form.
- Around line 133-145: The timer created by DispatchSource.makeTimerSource
(timer) must be armed/resumed before any path can call finish(), because
finish() may cancel the timer and calling cancel() on a suspended timer is
undefined; resume (or otherwise schedule/activate) the timer right after
creation and before calling process.run() so that finish() can safely call
timer.cancel(); ensure you only resume/arm the timer once (avoid double-resume)
and reference the existing finish(), finishLock, didFinish, continuation and the
process.run() call when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45a3ed65-f606-4201-abdd-3af0b7b4893d
📒 Files selected for processing (1)
Sources/ConfiguredMenuBarController.swift
| let timer = DispatchSource.makeTimerSource(queue: DispatchQueue.global(qos: .utility)) | ||
|
|
||
| func finish(_ result: ConfiguredMenuBarDynamicCommandResult) { | ||
| finishLock.lock() | ||
| guard !didFinish else { | ||
| finishLock.unlock() | ||
| return | ||
| } | ||
| didFinish = true | ||
| finishLock.unlock() | ||
| timer.cancel() | ||
| continuation.resume(returning: result) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l Sources/ConfiguredMenuBarController.swiftRepository: manaflow-ai/cmux
Length of output: 107
🏁 Script executed:
sed -n '120,250p' Sources/ConfiguredMenuBarController.swiftRepository: manaflow-ai/cmux
Length of output: 4981
Don't cancel the timer before it has been resumed.
If process.run() throws at line 203, finish() cancels the timer at line 143 before line 220 ever resumes it. Calling cancel() on a suspended DispatchSourceTimer produces undefined behavior, which can crash on systems with certain dispatch implementations. Arm the timer before attempting process.run() to ensure it's always in a safe state before cancellation.
🩹 Minimal fix
+ timer.schedule(deadline: .now() + max(0.1, timeoutSeconds))
+ timer.setEventHandler {
+ finishLock.lock()
+ guard !didFinish else {
+ finishLock.unlock()
+ return
+ }
+ didTimeOut = true
+ finishLock.unlock()
+ process.terminate()
+ }
+ timer.resume()
+
do {
try process.run()
onStarted(process.processIdentifier)
} catch {
let durationMS = max(0, Int(Date().timeIntervalSince(startedAt) * 1000))
stdoutPipe.fileHandleForReading.readabilityHandler = nil
stderrPipe.fileHandleForReading.readabilityHandler = nil
finish(ConfiguredMenuBarDynamicCommandResult(
stdout: "",
stderr: "",
exitStatus: nil,
durationMS: durationMS,
errorMessage: error.localizedDescription
))
return
}
-
- timer.schedule(deadline: .now() + max(0.1, timeoutSeconds))
- timer.setEventHandler {
- finishLock.lock()
- guard !didFinish else {
- finishLock.unlock()
- return
- }
- didTimeOut = true
- finishLock.unlock()
- process.terminate()
- }
- timer.resume()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ConfiguredMenuBarController.swift` around lines 133 - 145, The timer
created by DispatchSource.makeTimerSource (timer) must be armed/resumed before
any path can call finish(), because finish() may cancel the timer and calling
cancel() on a suspended timer is undefined; resume (or otherwise
schedule/activate) the timer right after creation and before calling
process.run() so that finish() can safely call timer.cancel(); ensure you only
resume/arm the timer once (avoid double-resume) and reference the existing
finish(), finishLock, didFinish, continuation and the process.run() call when
making the change.
| detailParts.append(String( | ||
| format: String( | ||
| localized: "taskManager.dynamicMenu.items", | ||
| defaultValue: "%d items" | ||
| ), | ||
| generatedItemCount | ||
| )) |
There was a problem hiding this comment.
Use pluralized localization keys for the item count.
"%d items" will render "1 items" for the singular case and won’t localize correctly across locales. Please switch this to .one / .other keys for taskManager.dynamicMenu.items. Based on learnings: "In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ConfiguredMenuBarController.swift` around lines 366 - 372, The
current call that appends the item count uses a simple "%d items" format and the
localization key "taskManager.dynamicMenu.items", which will produce incorrect
singular forms; update the usage to employ pluralized localization entries
(ICU-style .one and .other) for "taskManager.dynamicMenu.items" and format using
the plural-aware API so that detailParts.append(...) uses the pluralized
localization based on generatedItemCount (reference the existing call site with
detailParts.append and the generatedItemCount variable) ensuring the .one and
.other keys are present in the strings file and the runtime selects the correct
plural form.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| if let issue = resolved.issues.first { | ||
| throw NSError(domain: "CmuxDynamicMenu", code: 1, userInfo: [ | ||
| NSLocalizedDescriptionKey: issue.logMessage | ||
| ]) | ||
| } | ||
| state.phase = .loaded | ||
| state.items = resolved.items | ||
| state.error = nil | ||
| state.generatedItemCount = resolved.items.count |
There was a problem hiding this comment.
resolveGeneratedMenuBarItems already skips invalid items via continue, so resolved.items contains only the items that resolved successfully. Throwing on the first issue discards those valid items too: a dynamic command that returns 9 valid action refs plus 1 bad ref fails completely instead of rendering the 9 good items. The state transition to .failed happens in the catch block anyway, so the error notification and "Dynamic menu failed" UI are unaffected by updating state.items first.
| if let issue = resolved.issues.first { | |
| throw NSError(domain: "CmuxDynamicMenu", code: 1, userInfo: [ | |
| NSLocalizedDescriptionKey: issue.logMessage | |
| ]) | |
| } | |
| state.phase = .loaded | |
| state.items = resolved.items | |
| state.error = nil | |
| state.generatedItemCount = resolved.items.count | |
| state.phase = resolved.issues.isEmpty ? .loaded : .failed | |
| state.items = resolved.items | |
| state.error = resolved.issues.first.map { $0.logMessage } | |
| state.generatedItemCount = resolved.items.count | |
| if let issue = resolved.issues.first { | |
| owner?.notifyConfiguredMenuBarDynamicFailure( | |
| source: source, | |
| error: issue.logMessage, | |
| preferredWindow: preferredWindow | |
| ) | |
| } |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f45866d. Configure here.
|
|
||
| private func dynamicMenuWillOpen(sourceID: String, preferredWindow: NSWindow?) { | ||
| guard let source = dynamicSourceByID[sourceID] else { return } | ||
| renderDynamicSourceMenu(sourceID: sourceID, preferredWindow: preferredWindow) | ||
| if (source.source.refresh ?? .onOpen) == .onOpen { | ||
| refreshDynamicSource(sourceID: sourceID, preferredWindow: preferredWindow, reason: .open) |
There was a problem hiding this comment.
cancelDynamicTask calls task.cancel() but never terminates the underlying Process. Because ConfiguredMenuBarDynamicRunner.run uses withCheckedContinuation with no withTaskCancellationHandler, Swift Task cancellation has no effect on the GCD block running runBlocking — the subprocess continues until it exits or the timeout fires (up to timeoutSeconds, user-configurable). When startDynamicSource cancels and re-spawns (e.g., rapid user-triggered reloads or interval misfires during config reload), every prior subprocess runs to completion invisibly: the PID is cleared from dynamicStates by cancelDynamicTask, so it disappears from Task Manager even though the process is still alive. For interval sources with custom timeoutSeconds values, multiple orphaned processes can accumulate.
The fix is to pass the Process reference out of run via the existing onStarted callback pattern and call process.terminate() inside a withTaskCancellationHandler around the withCheckedContinuation call, or store the Process on the task side and cancel it when the Task is cancelled.
| @@ -2313,6 +2393,36 @@ final class CmuxConfigStore: ObservableObject { | |||
| } | |||
There was a problem hiding this comment.
menuBarShortcutActions() appends every menu bar action with a non-nil shortcut. When a menu bar item uses an action reference (the item.action path in resolvedMenuBarAction), the resolved CmuxResolvedConfigAction comes directly from loadedActions. If that action already satisfies the configuredActions filter — shortcut != nil && (builtInIDs.contains(id) || actionSourcePath != nil) — it appears in both configuredActions and menuBarShortcutActions, causing shortcutActions() to return it twice. Depending on how callers register keyboard shortcuts, this can result in the same shortcut firing the same action twice per keypress, or conflicting handler registrations.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
ui.menuBarsupport for custom top-level menus, explicitextends, duplicate-title preservation, action refs, inline commands, separators, nested submenus, and dynamic bash-backed menu sources.onOpen,manual,onConfigReload,interval) with trust gating, timeout/output limits, last-good cached items, failure UI, notifications, and Task Manager visibility.Testing
./scripts/reload.sh --tag menubarxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-menubar-tests -only-testing:cmuxTests/CmuxConfigContextMenuTests testjq empty web/data/cmux.schema.json && jq empty Resources/Localizable.xcstringsplutil -lint GhosttyTabs.xcodeproj/project.pbxprojgit diff --checkDemo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
Note
High Risk
Introduces a new menu-bar execution path that can run user-supplied shell commands (dynamic sources) and mutates the macOS main menu at runtime; bugs here could impact security/trust prompts, menu stability, or background process management.
Overview
Adds
ui.menuBarsupport inCmuxConfigStore/CmuxConfigUIto define custom top-level menus and menu extensions, with validated parsing/resolution for action refs, inline actions, separators, nested submenus, placement (before/after), and config issues (.menuBarInvalidMenu).Introduces
ConfiguredMenuBarControllerto apply these menus toNSApp.mainMenu, execute selected actions viaAppDelegate, and implement dynamic menu sources that run Bash, decode JSON menu items, cache last-good output, enforce timeout/output limits, require trust for project-local interval refresh, and emit user notifications on failures.Extends Task Manager payloads/UI to include dynamic menu sources (state, PIDs, resources) and updates docs/schema/localizations/tests accordingly; CI e2e recording is made more robust by preferring
screencapture, adding safer shutdown/transcoding, and falling back to screenshot-based video generation.Reviewed by Cursor Bugbot for commit 9ace32b. Bugbot is set up for automated code reviews on this repo. Configure here.