Repository navigation
Add configurable cmux.json workspace and tab bar actions - #3084
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:
📝 WalkthroughWalkthroughCentralizes new-workspace (Cmd+N) routing in AppDelegate, threads an optional per-window Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI (Menu/Titlebar/Shortcut)
participant AD as AppDelegate
participant Store as CmuxConfigStore
participant TabMgr as TabManager
participant Exec as CmuxConfigExecutor
User->>UI: trigger New Workspace (Cmd+N / menu / titlebar)
UI->>AD: performNewWorkspaceAction(tabManager?, event?, debugSource)
AD->>Store: resolvedNewWorkspaceCommand()
alt Store resolves workspace-targeting action
AD->>Exec: execute(action, commands, commandSourcePaths,...)
Exec->>Exec: confirm/trust -> run workspace action (open/replace/close as defined)
else Store resolves terminal command or no action
AD->>TabMgr: create workspace or call preferred tab manager
TabMgr->>Workspace: new workspace / new surface (initialInput if needed)
Workspace->>Exec: execute terminal command or send prepared shell input
end
alt No available preferred tab manager
AD->>AD: open new main window (register with cmuxConfigStore if available)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a root-level Confidence Score: 5/5Safe to merge; all findings are P2 style/test suggestions with no blocking correctness issues. The routing logic is well-structured with correct fallback chains, the config validation is sound (blank-value guard at decode time), and all call sites have been updated consistently. The only gaps are a missing whitespace-trim test and no test coverage for the resolvedNewWorkspaceCommand() guard paths — neither affects runtime correctness. No files require special attention, though Sources/CmuxConfig.swift would benefit from surfacing misconfiguration to the user rather than silently falling back. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cmd+N / File Menu / Command Palette / Titlebar +] --> B[performNewWorkspaceAction]
B --> C{mainWindowContexts empty\nAND preferredContext nil?}
C -- Yes --> D[openNewMainWindow]
C -- No --> E[Resolve context:\npreferredContext ?? preferredMainWindowContext]
E --> F{executeConfiguredNewWorkspace\nCommandIfAvailable?}
F -- Yes: configured command found --> G[CmuxConfigExecutor.execute\nconfigured command]
F -- No --> H{preferredTabManager\nAND preferredContext non-nil?}
H -- Yes --> I[preferredTabManager.addWorkspace]
H -- No --> J[addWorkspaceInPreferredMainWindow]
J --> K{returns nil?}
K -- Yes --> D
K -- No --> L[Done]
Reviews (1): Last reviewed commit: "Add configurable new workspace command" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: ccf8aed0b9
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Sources/CmuxConfig.swift (2)
19-36: Hard-throwing on blanknewWorkspaceCommanddrops the entire config.Because
parseConfigcatches decoding errors and returnsnil(onlyNSLog), a user who writes"newWorkspaceCommand": ""(or whitespace) will silently lose every command from that file, not just the misconfigured field. Other blank validations in this file (e.g., blankcommand) already behave this way, so this is consistent — but given this is a brand-new, top-level, optional field, it may be friendlier to treat blank asnil(equivalent to omission) rather than invalidate the whole config. Up to you; current behavior is defensible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 19 - 36, The initializer init(from decoder:) currently throws DecodingError.dataCorrupted when CodingKeys.newWorkspaceCommand is present but blank; change this so that if the decoded rawNewWorkspaceCommand trims to an empty string you set newWorkspaceCommand = nil (treat blank as omission) instead of throwing, while preserving the existing behavior of assigning trimmed to newWorkspaceCommand when non-empty and still decoding commands via commands = try container.decode([CmuxCommandDefinition].self, forKey: .commands).
431-442: Resolution and workspace-scope validation look good.Returning
nilon mismatch/misconfiguration lets the caller fall through to the default new-workspace path (perexecuteConfiguredNewWorkspaceCommandIfAvailableinAppDelegate.swift), which is the right behavior.NSLogmatches the existing parse-error logging style in this file.One minor thought: the mismatch log fires every time the user triggers Cmd+N / File › New Workspace while the config is misconfigured, which can flood Console. Consider logging once per
configRevisionchange if that becomes noisy in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 431 - 442, The current resolvedNewWorkspaceCommand() logs a mismatch every time Cmd+N is pressed when the config is invalid, which can flood Console; add a stored property on CmuxConfig (e.g., lastLoggedInvalidNewWorkspaceConfigRevision) and in resolvedNewWorkspaceCommand() check the current configRevision against that property before calling NSLog, only logging and updating lastLoggedInvalidNewWorkspaceConfigRevision when the revision changed; keep the existing nil return behavior and ensure the property is updated for both "no matching command" and "not a workspace command" cases (add thread-safety if CmuxConfig is accessed from multiple threads).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6599-6624: The code currently skips honoring an explicitly passed
preferredTabManager when mainWindowContext(for:) returns nil; change the logic
so that after resolving context and attempting
executeConfiguredNewWorkspaceCommandIfAvailable(in:debugSource:), if
preferredTabManager is non-nil you call preferredTabManager.addWorkspace() and
return true regardless of preferredContext being nil (i.e., remove the
preferredContext != nil check), ensuring preferredTabManager is used as the
fallback target; refer to preferredTabManager, mainWindowContext(for:),
preferredMainWindowContextForWorkspaceCreation(event:debugSource:),
preferredContext, and addWorkspace() when making this change.
- Around line 6646-6670: The method
executeConfiguredNewWorkspaceCommandIfAvailable currently treats any call to
CmuxConfigExecutor.execute(...) as a successful execution; change this so we
only return true when the command actually ran: update
CmuxConfigExecutor.execute to return a Bool (or optional workspace ID)
indicating success, then in executeConfiguredNewWorkspaceCommandIfAvailable call
the new return value and only return true when it indicates success;
alternatively, if you prefer not to change the executor signature, pre-validate
here that the configured command can run (e.g., check for a focused terminal or
that the command is a workspace-creating command) before calling execute and
only return true when that validation passes. Ensure you update references to
resolvedNewWorkspaceCommand(), tabManager (e.g.,
selectedWorkspace/currentDirectory or focused terminal check) and the call site
to use the new Bool/ID result.
- Around line 4840-4853: The re-registration path unconditionally overwrites a
previously registered cmuxConfigStore because the parameter defaults to nil;
update the branches that find an existing MainWindowContext (the first if-let
existing = mainWindowContexts[key] and the else-if using
mainWindowContexts.values.first where $0.windowId == windowId) to only replace
existing.cmuxConfigStore when the incoming cmuxConfigStore is non-nil (i.e.,
keep the current existing.cmuxConfigStore if the parameter is nil), leaving
other assignments (existing.window, reindexMainWindowContextIfNeeded) as-is.
---
Nitpick comments:
In `@Sources/CmuxConfig.swift`:
- Around line 19-36: The initializer init(from decoder:) currently throws
DecodingError.dataCorrupted when CodingKeys.newWorkspaceCommand is present but
blank; change this so that if the decoded rawNewWorkspaceCommand trims to an
empty string you set newWorkspaceCommand = nil (treat blank as omission) instead
of throwing, while preserving the existing behavior of assigning trimmed to
newWorkspaceCommand when non-empty and still decoding commands via commands =
try container.decode([CmuxCommandDefinition].self, forKey: .commands).
- Around line 431-442: The current resolvedNewWorkspaceCommand() logs a mismatch
every time Cmd+N is pressed when the config is invalid, which can flood Console;
add a stored property on CmuxConfig (e.g.,
lastLoggedInvalidNewWorkspaceConfigRevision) and in
resolvedNewWorkspaceCommand() check the current configRevision against that
property before calling NSLog, only logging and updating
lastLoggedInvalidNewWorkspaceConfigRevision when the revision changed; keep the
existing nil return behavior and ensure the property is updated for both "no
matching command" and "not a workspace command" cases (add thread-safety if
CmuxConfig is accessed from multiple threads).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f4b321-551c-4798-9e8b-cae490670c74
📒 Files selected for processing (7)
Sources/AppDelegate.swiftSources/CmuxConfig.swiftSources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/cmuxApp.swiftcmuxTests/CmuxConfigTests.swiftweb/app/[locale]/docs/custom-commands/page.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:6622">
P1: The `preferredContext != nil` guard causes the explicitly provided `preferredTabManager` to be silently ignored when `mainWindowContext(for:)` returns `nil`. This can happen during close/reparent races where the context briefly has `window == nil`, causing the workspace to be created in a different window (or a new window) instead of the one the caller intended. Remove the `preferredContext != nil` check so the explicit tab manager is always honored as a fallback.</violation>
<violation number="2" location="Sources/AppDelegate.swift:6663">
P2: `executeConfiguredNewWorkspaceCommandIfAvailable` reports success even when the configured shell command cannot run (no focused terminal), which swallows New Workspace without creating anything.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e18e044c
ℹ️ 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".
f2e18e0 to
cc254a8
Compare
cc254a8 to
02bcff1
Compare
02bcff1 to
faf8cbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/CmuxConfig.swift (1)
403-408:$tabssink will re-apply buttons on every tabs mutation (and once on subscribe).
@Published.projectedValueemits the current value to new subscribers, so this sink fires immediately afterwireDirectoryTracking— duplicating theapplySurfaceTabBarButtonsToCurrentManager()call that already happens at the end ofloadAll(). It then fires again on every add/remove/reorder, re-applying the same button configuration to every workspace each time. Functionally correct, but noisy.Consider dropping the initial emission and/or de-duplicating, e.g.:
tabManager.$tabs + .dropFirst() + .map { tabs in tabs.map(\.id) } + .removeDuplicates() .receive(on: DispatchQueue.main) - .sink { [weak self] _ in + .sink { [weak self] _ in self?.applySurfaceTabBarButtonsToCurrentManager() } .store(in: &cancellables)If
TabManager.applySurfaceTabBarButtons(_:)only needs to run when the tab set changes (not when an existing workspace mutates), keying ontabs.map(\.id)is cheaper than hashing the whole workspace array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 403 - 408, The sink on tabManager.$tabs currently receives the initial value and every mutation, causing duplicate/noisy re-applications of applySurfaceTabBarButtonsToCurrentManager(); change the publisher chain to ignore the initial emission and de-duplicate only when the tab identities change (e.g. use tabManager.$tabs.map { $0.map(\.id) }.removeDuplicates() or at minimum .dropFirst()) so applySurfaceTabBarButtonsToCurrentManager() is invoked only when the set/order of tabs changes; update the existing sink (the closure that calls applySurfaceTabBarButtonsToCurrentManager()) to subscribe to that transformed publisher instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/CmuxConfig.swift`:
- Around line 505-516: resolvedNewWorkspaceCommand() currently calls NSLog every
time an invalid newWorkspaceCommandName is hit; add a small cache on the
CmuxConfig instance (e.g., a property lastLoggedInvalidName: (String, UInt64)?)
keyed by the invalid name and the current configRevision to dedupe logs so the
same warning is emitted only once per reload. In practice: before each NSLog
check lastLoggedInvalidName against (newWorkspaceCommandName, configRevision)
and skip logging if it matches; if you do log, set lastLoggedInvalidName =
(newWorkspaceCommandName, configRevision). Update the logic in
resolvedNewWorkspaceCommand() (referencing newWorkspaceCommandName,
loadedCommands, commandSourcePaths, and configRevision) to consult and update
this cache so repeated Cmd+N invocations don’t spam the log.
- Around line 26-60: The current CmuxConfig.init(from decoder:) throws
DecodingError for a blank newWorkspaceCommand and for duplicate
surfaceTabBarButtons, which causes JSONDecoder().decode to fail and the whole
file to be discarded; change the decoder to be lenient: in init(from decoder:)
for newWorkspaceCommand, decode and trim but if empty set newWorkspaceCommand =
nil and NSLog a clear warning instead of throwing; for surfaceTabBarButtons,
decode the array, remove duplicates (preserve first occurrence) into
surfaceTabBarButtons and NSLog which duplicates were dropped instead of
throwing; keep commands decoding unchanged. Update the logic in
CmuxConfig.init(from:) (referencing newWorkspaceCommand and
surfaceTabBarButtons) so bad top-level values are normalized and logged rather
than causing a thrown DecodingError that drops the whole config.
---
Nitpick comments:
In `@Sources/CmuxConfig.swift`:
- Around line 403-408: The sink on tabManager.$tabs currently receives the
initial value and every mutation, causing duplicate/noisy re-applications of
applySurfaceTabBarButtonsToCurrentManager(); change the publisher chain to
ignore the initial emission and de-duplicate only when the tab identities change
(e.g. use tabManager.$tabs.map { $0.map(\.id) }.removeDuplicates() or at minimum
.dropFirst()) so applySurfaceTabBarButtonsToCurrentManager() is invoked only
when the set/order of tabs changes; update the existing sink (the closure that
calls applySurfaceTabBarButtonsToCurrentManager()) to subscribe to that
transformed publisher instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b85f1ea-242a-46bf-ad1e-152a069e2165
📒 Files selected for processing (6)
Sources/CmuxConfig.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/CmuxConfigTests.swiftvendor/bonsplitweb/app/[locale]/docs/custom-commands/page.tsx
✅ Files skipped from review due to trivial changes (2)
- web/app/[locale]/docs/custom-commands/page.tsx
- vendor/bonsplit
🚧 Files skipped from review as they are similar to previous changes (2)
- cmuxTests/CmuxConfigTests.swift
- Sources/Workspace.swift
faf8cbc to
fad4350
Compare
fad4350 to
4475e43
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/CmuxConfig.swift (2)
505-515:⚠️ Potential issue | 🟡 MinorDeduplicate invalid
newWorkspaceCommandwarnings per config revision.A persistent bad setting still logs on every Cmd+N/menu/palette/titlebar invocation. Cache the last invalid
(name, reason, configRevision)so the warning emits once per reload.🧹 Proposed log dedupe
final class CmuxConfigStore: ObservableObject { `@Published` private(set) var loadedCommands: [CmuxCommandDefinition] = [] `@Published` private(set) var newWorkspaceCommandName: String? `@Published` private(set) var surfaceTabBarButtons: [CmuxSurfaceTabBarButton] = CmuxSurfaceTabBarButton.defaults `@Published` private(set) var configRevision: UInt64 = 0 /// Which config file each command came from, keyed by command id. private(set) var commandSourcePaths: [String: String] = [:] + private var lastLoggedInvalidNewWorkspaceCommand: (name: String, revision: UInt64, reason: String)? private(set) var localConfigPath: String? private weak var tabManager: TabManager?func resolvedNewWorkspaceCommand() -> CmuxResolvedCommand? { guard let commandName = newWorkspaceCommandName else { return nil } guard let command = loadedCommands.first(where: { $0.name == commandName }) else { - NSLog("[CmuxConfig] newWorkspaceCommand '%@' does not match any loaded command", commandName) + logInvalidNewWorkspaceCommand(commandName, reason: "does not match any loaded command") return nil } guard command.workspace != nil else { - NSLog("[CmuxConfig] newWorkspaceCommand '%@' must reference a workspace command", commandName) + logInvalidNewWorkspaceCommand(commandName, reason: "must reference a workspace command") return nil } return CmuxResolvedCommand(command: command, sourcePath: commandSourcePaths[command.id]) } + + private func logInvalidNewWorkspaceCommand(_ commandName: String, reason: String) { + let last = lastLoggedInvalidNewWorkspaceCommand + guard last?.name != commandName || + last?.revision != configRevision || + last?.reason != reason else { + return + } + lastLoggedInvalidNewWorkspaceCommand = (commandName, configRevision, reason) + NSLog("[CmuxConfig] newWorkspaceCommand '%@' %@", commandName, reason) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 505 - 515, The resolvedNewWorkspaceCommand() currently logs the same invalid newWorkspaceCommand every time; add a small dedupe cache (e.g., a stored property like lastInvalidNewWorkspaceCommand tuple containing name, reason, and configRevision) and use it inside resolvedNewWorkspaceCommand() to only call NSLog when the (newWorkspaceCommandName, reason, configRevision) differs from the cached tuple; update the cache with the new tuple whenever you return nil for a different reason or revision. Reference resolvedNewWorkspaceCommand(), newWorkspaceCommandName, loadedCommands, commandSourcePaths, and the configRevision value so the logic detects three distinct reasons (not found, not a workspace command, or nil) and suppresses repeated logs until the configRevision or name/reason changes.
26-58:⚠️ Potential issue | 🟠 MajorKeep optional top-level config validation non-fatal.
This still lets a blank
newWorkspaceCommandor duplicatesurfaceTabBarButtonsthrow during top-level decode; Line 527 then fails the whole file, dropping otherwise validcommands. Prefer normalizing/logging just the bad top-level value and preserving the rest of the config.♻️ Proposed normalization
init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) if let rawNewWorkspaceCommand = try container.decodeIfPresent(String.self, forKey: .newWorkspaceCommand) { let trimmed = rawNewWorkspaceCommand.trimmingCharacters(in: .whitespacesAndNewlines) if trimmed.isEmpty { - throw DecodingError.dataCorrupted( - DecodingError.Context( - codingPath: decoder.codingPath + [CodingKeys.newWorkspaceCommand], - debugDescription: "newWorkspaceCommand must not be blank" - ) - ) + NSLog("[CmuxConfig] ignoring blank newWorkspaceCommand") + newWorkspaceCommand = nil + } else { + newWorkspaceCommand = trimmed } - newWorkspaceCommand = trimmed } else { newWorkspaceCommand = nil } if let buttons = try container.decodeIfPresent([CmuxSurfaceTabBarButton].self, forKey: .surfaceTabBarButtons) { var seen = Set<CmuxSurfaceTabBarButton>() + var normalizedButtons: [CmuxSurfaceTabBarButton] = [] + var duplicateButtons: [String] = [] for button in buttons { if !seen.insert(button).inserted { - throw DecodingError.dataCorrupted( - DecodingError.Context( - codingPath: decoder.codingPath + [CodingKeys.surfaceTabBarButtons], - debugDescription: "surfaceTabBarButtons must not contain duplicate values" - ) - ) + duplicateButtons.append(button.rawValue) + } else { + normalizedButtons.append(button) } } - surfaceTabBarButtons = buttons + if !duplicateButtons.isEmpty { + NSLog( + "[CmuxConfig] ignoring duplicate surfaceTabBarButtons values: %@", + duplicateButtons.joined(separator: ", ") + ) + } + surfaceTabBarButtons = normalizedButtons } else { surfaceTabBarButtons = nil } commands = try container.decodeIfPresent([CmuxCommandDefinition].self, forKey: .commands) ?? [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 26 - 58, The init(from decoder: Decoder) currently throws DecodingError when a top-level optional value is invalid; change it to normalize and preserve the rest of the config by replacing throwing behavior in init(from decoder: Decoder): for newWorkspaceCommand, trim and if empty set newWorkspaceCommand = nil and emit a non-fatal log/warning (do not throw); for surfaceTabBarButtons, deduplicate the decoded [CmuxSurfaceTabBarButton] (e.g., build a filtered array keeping first occurrences) and assign the deduped array (or nil if empty) while logging a warning instead of throwing; keep commands = try container.decodeIfPresent([...]) ?? [] unchanged so invalid top-level values don’t discard commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/CmuxConfig.swift`:
- Around line 505-515: The resolvedNewWorkspaceCommand() currently logs the same
invalid newWorkspaceCommand every time; add a small dedupe cache (e.g., a stored
property like lastInvalidNewWorkspaceCommand tuple containing name, reason, and
configRevision) and use it inside resolvedNewWorkspaceCommand() to only call
NSLog when the (newWorkspaceCommandName, reason, configRevision) differs from
the cached tuple; update the cache with the new tuple whenever you return nil
for a different reason or revision. Reference resolvedNewWorkspaceCommand(),
newWorkspaceCommandName, loadedCommands, commandSourcePaths, and the
configRevision value so the logic detects three distinct reasons (not found, not
a workspace command, or nil) and suppresses repeated logs until the
configRevision or name/reason changes.
- Around line 26-58: The init(from decoder: Decoder) currently throws
DecodingError when a top-level optional value is invalid; change it to normalize
and preserve the rest of the config by replacing throwing behavior in init(from
decoder: Decoder): for newWorkspaceCommand, trim and if empty set
newWorkspaceCommand = nil and emit a non-fatal log/warning (do not throw); for
surfaceTabBarButtons, deduplicate the decoded [CmuxSurfaceTabBarButton] (e.g.,
build a filtered array keeping first occurrences) and assign the deduped array
(or nil if empty) while logging a warning instead of throwing; keep commands =
try container.decodeIfPresent([...]) ?? [] unchanged so invalid top-level values
don’t discard commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6988ecc4-493b-4d5d-b5a8-a6a7790a9775
📒 Files selected for processing (6)
Sources/CmuxConfig.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/CmuxConfigTests.swiftvendor/bonsplitweb/app/[locale]/docs/custom-commands/page.tsx
✅ Files skipped from review due to trivial changes (1)
- web/app/[locale]/docs/custom-commands/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- vendor/bonsplit
- cmuxTests/CmuxConfigTests.swift
4475e43 to
ed45d1b
Compare
lawrencecchen
changed the title
ed45d1b to
171bf7c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 171bf7ca51
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Resources/Localizable.xcstrings (1)
16860-16890:⚠️ Potential issue | 🟡 MinorReusing existing keys with changed semantics leaves non-
en/jatranslations stale.
dialog.cmuxConfig.confirmCommand.runanddialog.cmuxConfig.confirmCommand.titlechanged from generic "Run"/"Run Command" wording to "Run Once"/"Run Project Action?" but 16 locales (ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant) retain their old translations. This creates a semantic mismatch where the trust dialog shows translated "Run Command" instead of "Run Once".Consider introducing new semantic keys, or updating the translations on these two keys across all locales.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 16860 - 16890, The localization keys dialog.cmuxConfig.confirmCommand.run and dialog.cmuxConfig.confirmCommand.title were repurposed from "Run"/"Run Command" to "Run Once"/"Run Project Action?", causing non-en/ja translations to be semantically stale; either add new keys (e.g., dialog.cmuxConfig.confirmCommand.runOnce and dialog.cmuxConfig.confirmCommand.titleProjectAction) and update all locale files with the new strings, or update the existing non-English entries for those two keys across the affected locales (ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant) to match the new English/ja semantics so the trust dialog displays correct translations.Sources/CmuxConfig.swift (1)
1874-1899:⚠️ Potential issue | 🟠 MajorDon’t cancel the action-trust observer when wiring directory tracking.
wireDirectoryTrackingremoves allcancellables, which cancels theCmuxActionTrust.didChangeNotificationsubscription installed ininit. After this, “Trust and Run” changes won’t reapply tab-bar button trust/lock state.Proposed fix
+ private var trustCancellables = Set<AnyCancellable>() private var cancellables = Set<AnyCancellable>() @@ NotificationCenter.default.publisher(for: CmuxActionTrust.didChangeNotification) .receive(on: DispatchQueue.main) .sink { [weak self] _ in @@ self.configRevision &+= 1 } - .store(in: &cancellables) + .store(in: &trustCancellables) @@ func wireDirectoryTracking(tabManager: TabManager) { cancellables.removeAll()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 1874 - 1899, The NotificationCenter subscription to CmuxActionTrust.didChangeNotification was stored in cancellables in init and gets cancelled by wireDirectoryTracking's cancellables.removeAll(), so create a dedicated AnyCancellable property (e.g., actionTrustObserver: AnyCancellable?) and store the NotificationCenter publisher's sink there instead of into cancellables; then leave cancellables.removeAll() in wireDirectoryTracking to clear only tab-related subscriptions, and cancel actionTrustObserver in deinit (or when no longer needed). This preserves the CmuxActionTrust.didChangeNotification observer while still allowing tabManager-related cancellables to be cleared.
♻️ Duplicate comments (3)
Sources/AppDelegate.swift (1)
4964-4984:⚠️ Potential issue | 🟠 MajorHonor
preferredTabManagerbefore creating a fallback window.Line 4964 takes the empty-registry fallback before the later preferred-manager fallback. If a caller passes a live
preferredTabManagerbefore its window context is registered, this creates a second main window instead of adding the workspace to the explicit manager.Suggested fix
- if mainWindowContexts.isEmpty && livePreferredContext == nil { + if mainWindowContexts.isEmpty && livePreferredContext == nil { + if let preferredTabManager, preferredContext == nil { + preferredTabManager.addWorkspace() + return true + } `#if` DEBUG logWorkspaceCreationRouting( phase: "fallback_new_window",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4964 - 4984, The fallback branch currently creates a new main window when mainWindowContexts.isEmpty and livePreferredContext == nil; reorder or extend this logic to honor the caller-provided preferredTabManager before creating a fallback window by checking preferredTabManager (and resolving livePreferredContext from it) earlier: if a livePreferredContext can be obtained for the preferredTabManager, use that context's tabManager/selectedWorkspace and call executeConfiguredNewWorkspaceCommandIfAvailable(in:context, debugSource:..., replacingInitialWorkspace:...) instead of calling createMainWindow(); only call createMainWindow() when no preferredTabManager/livePreferredContext exists and mainWindowContexts is truly empty. Ensure you reference mainWindowContexts, livePreferredContext, preferredTabManager, createMainWindow(), executeConfiguredNewWorkspaceCommandIfAvailable(in:), and tabManager.selectedWorkspace when making the change.Sources/CmuxConfigExecutor.swift (2)
78-96:⚠️ Potential issue | 🟠 MajorCheck
.currentTerminalavailability before preparing trusted input.For action-based terminal commands,
preparedShellInput(...)can show confirmation and persist trust before Line 95 discovers there is no focused terminal. Guard the focused terminal first whentarget == .currentTerminal.Proposed fix
guard let command = action.terminalCommand else { return false } let target = action.terminalCommandTarget ?? .newTabInCurrentPane + let currentTerminal = tabManager.selectedWorkspace?.focusedTerminalPanel + if target == .currentTerminal, currentTerminal == nil { + return false + } guard let shellInput = preparedShellInput( command, confirm: action.confirm ?? false, @@ switch target { case .currentTerminal: - guard let terminal = tabManager.selectedWorkspace?.focusedTerminalPanel else { return false } + guard let terminal = currentTerminal else { return false } terminal.sendInput(shellInput) return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigExecutor.swift` around lines 78 - 96, When target == .currentTerminal you must ensure a focused terminal exists before calling preparedShellInput to avoid persisting trust or showing confirmation for a command that cannot be delivered; modify the flow in CmuxConfigExecutor so that after setting let target = action.terminalCommandTarget you first check if target == .currentTerminal and guard let terminal = tabManager.selectedWorkspace?.focusedTerminalPanel else { return false }, and only then call preparedShellInput(...); keep references to preparedShellInput(...), action.terminalCommandTarget/.currentTerminal, tabManager.selectedWorkspace?.focusedTerminalPanel and terminal.sendInput in the updated order.
146-154:⚠️ Potential issue | 🟠 MajorHonor
confirm: trueeven when the source path is unavailable.Line 146 treats a nil
configSourcePathas trusted, so an explicitly confirmed command can run without any prompt. UseglobalConfigPathas the fallback display/trust path or add a no-path confirmation path.Proposed fix
- guard let sourcePath = configSourcePath, - sourcePath != globalConfigPath else { + let sourcePath = configSourcePath ?? globalConfigPath + guard sourcePath != globalConfigPath || confirm else { return true }If global-config commands are intended to stay trusted by default, keep that fast path only when
confirm == false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigExecutor.swift` around lines 146 - 154, The early-return currently treats a nil configSourcePath as trusted; change the logic so we only bypass confirmation when confirm is false and the path equals globalConfigPath, and otherwise use a fallback display/trust path (let displayPath = configSourcePath ?? globalConfigPath) for both the CmuxActionTrust.shared.isTrusted(descriptor) check and the showConfirmDialog(command: displayCommand, descriptor: descriptor, configPath: displayPath) call; update the guard/if branches accordingly so explicit confirm == true always routes to showConfirmDialog with the fallback path.
🧹 Nitpick comments (1)
web/app/[locale]/docs/custom-commands/page.tsx (1)
56-107: Icon schema documented inconsistently across examples.The documentation shows
iconin two different forms:
- Lines 56–107: object form
{ "type": "image", "path": "./icons/codex.svg" }- Lines 260–280: string shorthand
"./icons/codex.svg"and bare symbol"play.circle"The prose (lines 122–123) mentions only "SF Symbols, emoji, or image paths" without explaining the object form or string variants.
The accepted encodings are:
{ "type": "symbol" / "sfSymbol" / "systemImage", "name": "..." }{ "type": "emoji", "value": "..." }{ "type": "image" / "file", "path": "..." }"emoji:..."(shorthand)"file:..."(shorthand)- bare SF Symbol name (e.g.,
"play.circle")- relative image path (auto-detected by extension)
Consider consolidating the examples to use one canonical form or adding a subsection that enumerates all accepted icon encodings so readers can map examples to the schema unambiguously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/`[locale]/docs/custom-commands/page.tsx around lines 56 - 107, The examples for icon encoding are inconsistent and the prose doesn't enumerate accepted encodings; update the docs around the CodeBlock example that defines "cmux.newTerminal" / "claude" and the later examples (the sections showing bare `"play.circle"` and `"./icons/codex.svg"`) to either use one canonical icon form or add a concise "Icon encodings" subsection that lists the accepted encodings and their canonical keys: object forms `{ "type":"symbol"|"sfSymbol"|"systemImage","name":"..." }`, `{ "type":"emoji","value":"..." }`, `{ "type":"image"|"file","path":"..." }`, string shorthands `"emoji:..."`, `"file:..."`, bare SF Symbol names like `"play.circle"`, and relative image paths; reference the `icon` fields in the CodeBlock and any other examples so all snippets match the documented schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/CmuxActionTrust.swift`:
- Around line 15-20: The current fingerprint computed in var fingerprint: String
masks JSON encoding failures by hashing empty Data; change the API to surface
encoding errors (make fingerprint either throwing or optional: e.g., func
fingerprint() throws -> String or var fingerprint: String?) so encoding failure
returns nil/throws instead of producing a deterministic empty-data hash; update
Self.sha256Hex usage to only be called when encoding succeeds and ensure callers
of fingerprint/trust-check (the other similar property at the second occurrence
around lines 54-61) treat a nil/throw as "deny trust" (fail-closed) rather than
accepting a default fingerprint.
In `@Sources/CmuxConfig.swift`:
- Around line 2189-2240: The resolver currently drops source path info for
direct buttons and explicit icon overrides; update resolvedSurfaceTabBarButtons
to accept a button-list source path parameter and propagate it into
resolvedSurfaceTabBarButton so that when a button is direct (command/agent) the
returned ResolvedSurfaceTabBarButtonEntry.actionSourcePath is set to the
button-list source path (not nil), and when an icon is explicitly overridden set
the CmuxSurfaceTabBarButton.iconSourcePath to the button-list source path;
ensure terminalCommandSourcePaths and existing actionSourcePath logic (from
entry.actionSourcePath) remain unchanged and only fill missing source fields
with the provided button-list source path.
In `@Sources/ContentView.swift`:
- Around line 6056-6060: The palette item with commandId "palette.newWorkspace"
isn't being mapped to the configured "new-workspace" action, so cmux.json
overrides don't apply; update the lookup that uses
commandPaletteConfigActionID(for:) (or the code that computes
configuredPaletteAction via commandPaletteConfigActionID(...).flatMap {
cmuxConfigStore.resolvedAction(id: $0) }) to translate "palette.newWorkspace" to
the configured action id "new-workspace" before resolving (e.g., return
"new-workspace" from commandPaletteConfigActionID(for:) for that commandId or
substitute the id prior to calling cmuxConfigStore.resolvedAction), and make the
same change for the other occurrences referenced (the blocks around the other
ranges) so palette rows for new-workspace honor
palette.hide/title/keywords/shortcut overrides.
- Around line 7127-7134: Handlers currently call executeConfiguredAction(id:)
and fall back to built-ins when it returns false, but false conflates "no
configured action" and "configured action declined" — change the handlers (the
registry.register blocks that reference
CmuxSurfaceTabBarBuiltInAction.newTerminal, newBrowser and the split handlers)
to first check for the presence of a configured action and only call the
built-in fallback when no configured action exists; i.e., replace the pattern
`if !executeConfiguredAction(id: ...) { <builtin>() }` with logic like `if
hasConfiguredAction(id: ...) { _ = executeConfiguredAction(id: ...) ; return }
else { <builtin>() }` (implement a small helper hasConfiguredAction(id:) that
queries the same configuration store used by executeConfiguredAction if one does
not exist) and apply this change to the browser, terminal and split handlers
mentioned.
- Around line 7468-7480: In executeConfiguredAction, derive baseCwd from the
currently focused surface’s tracked directory instead of always using
tabManager.selectedWorkspace?.currentDirectory: first attempt to read
focusedSurface?.trackedDirectory (or focusedPanel?.surface?.trackedDirectory)
and use it if non-empty, otherwise fall back to
tabManager.selectedWorkspace?.currentDirectory and finally
FileManager.default.homeDirectoryForCurrentUser.path; then pass that baseCwd
into CmuxConfigExecutor.execute (preserving the existing parameter list) so
configured actions run in the focused pane’s directory.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 1986-2001: The helper configKeyString(preserveDigit:) currently
returns key for both branches; implement the intended behavior so that when
preserveDigit is true it returns the original key, and when false it
canonicalizes digit keys to a generic placeholder (e.g., replace
single-character numeric keys "0".."9" with a stable token like "digit" or "#"
or whatever the codebase expects) while leaving non-digit keys unchanged; update
the configKeyString(_:), referenced by configString(preserveDigit:) and callers
such as KeyboardShortcutSettingsFileStore, to perform this transformation and
ensure any case/normalization used elsewhere (e.g., lowercasing) is preserved.
- Around line 2003-2046: parseConfigKeyToken currently rejects multi-character
tokens (so configString values like "f5" or "media.playPause" can't be
round-tripped); update parseConfigKeyToken to accept and return function-key
tokens ("f1"…"f20") and media-key tokens ("media.*") by recognizing these
patterns in the lowered input (e.g., regex for ^f([1-9]|1[0-9]|20)$ and
^media\.[a-z0-9]+$) and returning the lowered token unchanged; keep all existing
single-char and named-key handling intact and implement this logic inside
parseConfigKeyToken so hand-edited "cmd+f5" and "cmd+media.playPause" parse
successfully.
In `@Sources/TabManager.swift`:
- Around line 5290-5306: The call in TabManager.applySurfaceTabBarButtons(...)
only updates existing tabs and needs to persist the resolved config so future
workspaces get the same buttons; update TabManager to store the resolved tuple
(buttons, sourcePath, globalConfigPath, terminalCommandSourcePaths,
workspaceCommands) as a property (e.g., currentSurfaceTabBarConfig), modify
applySurfaceTabBarButtons to save that config before iterating, and add a helper
applyCurrentSurfaceTabBarButtons(to workspace:) that applies the cached config
to a single workspace by calling workspace.applySurfaceTabBarButtons(...);
finally, invoke applyCurrentSurfaceTabBarButtons(to:) from workspace
creation/attachment/restore code paths (e.g., inside addWorkspace(...),
attachWorkspace(...), and the session restore handler) so newly added or
restored workspaces receive the persisted tab-bar buttons.
In `@Sources/Workspace.swift`:
- Around line 12448-12450: The delegate method
splitTabBar(_:didRequestCustomAction:inPane:) currently forwards the action to
executeSurfaceTabBarCommandButton without logging; add a debug-only dlog() call
before dispatching so custom tab-bar actions are recorded. Specifically, inside
func splitTabBar(_ controller: BonsplitController, didRequestCustomAction
identifier: String, inPane pane: PaneID) insert a dlog(...) invocation wrapped
in `#if` DEBUG / `#endif` that logs the identifier and pane (or pane.description)
and then call executeSurfaceTabBarCommandButton(identifier: identifier, inPane:
pane) as before.
In `@web/app/`[locale]/docs/custom-commands/page.tsx:
- Around line 29-47: The new hard-coded English strings on this page should be
moved into the translations and referenced with t(...): add keys under the
docs.customCommands namespace (e.g. fallbackLocal, nightlyCallout, trustCallout,
schemaIntro, nightlyActionRegistry*, etc.) in web/messages/*.json and replace
the inline English JSX text with t("docs.customCommands.<key>") calls; update
the fallback-local list item (the <li> that currently says "./cmux.json"), both
Callout components (the two <Callout type="info"> blocks), the schema intro
paragraph, and the Nightly action registry section to use t(...) just like the
other strings already using t, ensuring key names match the suggested
identifiers and mirror existing usage of the t function and Callout component in
this file.
---
Outside diff comments:
In `@Resources/Localizable.xcstrings`:
- Around line 16860-16890: The localization keys
dialog.cmuxConfig.confirmCommand.run and dialog.cmuxConfig.confirmCommand.title
were repurposed from "Run"/"Run Command" to "Run Once"/"Run Project Action?",
causing non-en/ja translations to be semantically stale; either add new keys
(e.g., dialog.cmuxConfig.confirmCommand.runOnce and
dialog.cmuxConfig.confirmCommand.titleProjectAction) and update all locale files
with the new strings, or update the existing non-English entries for those two
keys across the affected locales (ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR,
ru, th, tr, uk, zh-Hans, zh-Hant) to match the new English/ja semantics so the
trust dialog displays correct translations.
In `@Sources/CmuxConfig.swift`:
- Around line 1874-1899: The NotificationCenter subscription to
CmuxActionTrust.didChangeNotification was stored in cancellables in init and
gets cancelled by wireDirectoryTracking's cancellables.removeAll(), so create a
dedicated AnyCancellable property (e.g., actionTrustObserver: AnyCancellable?)
and store the NotificationCenter publisher's sink there instead of into
cancellables; then leave cancellables.removeAll() in wireDirectoryTracking to
clear only tab-related subscriptions, and cancel actionTrustObserver in deinit
(or when no longer needed). This preserves the
CmuxActionTrust.didChangeNotification observer while still allowing
tabManager-related cancellables to be cleared.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 4964-4984: The fallback branch currently creates a new main window
when mainWindowContexts.isEmpty and livePreferredContext == nil; reorder or
extend this logic to honor the caller-provided preferredTabManager before
creating a fallback window by checking preferredTabManager (and resolving
livePreferredContext from it) earlier: if a livePreferredContext can be obtained
for the preferredTabManager, use that context's tabManager/selectedWorkspace and
call executeConfiguredNewWorkspaceCommandIfAvailable(in:context,
debugSource:..., replacingInitialWorkspace:...) instead of calling
createMainWindow(); only call createMainWindow() when no
preferredTabManager/livePreferredContext exists and mainWindowContexts is truly
empty. Ensure you reference mainWindowContexts, livePreferredContext,
preferredTabManager, createMainWindow(),
executeConfiguredNewWorkspaceCommandIfAvailable(in:), and
tabManager.selectedWorkspace when making the change.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 78-96: When target == .currentTerminal you must ensure a focused
terminal exists before calling preparedShellInput to avoid persisting trust or
showing confirmation for a command that cannot be delivered; modify the flow in
CmuxConfigExecutor so that after setting let target =
action.terminalCommandTarget you first check if target == .currentTerminal and
guard let terminal = tabManager.selectedWorkspace?.focusedTerminalPanel else {
return false }, and only then call preparedShellInput(...); keep references to
preparedShellInput(...), action.terminalCommandTarget/.currentTerminal,
tabManager.selectedWorkspace?.focusedTerminalPanel and terminal.sendInput in the
updated order.
- Around line 146-154: The early-return currently treats a nil configSourcePath
as trusted; change the logic so we only bypass confirmation when confirm is
false and the path equals globalConfigPath, and otherwise use a fallback
display/trust path (let displayPath = configSourcePath ?? globalConfigPath) for
both the CmuxActionTrust.shared.isTrusted(descriptor) check and the
showConfirmDialog(command: displayCommand, descriptor: descriptor, configPath:
displayPath) call; update the guard/if branches accordingly so explicit confirm
== true always routes to showConfirmDialog with the fallback path.
---
Nitpick comments:
In `@web/app/`[locale]/docs/custom-commands/page.tsx:
- Around line 56-107: The examples for icon encoding are inconsistent and the
prose doesn't enumerate accepted encodings; update the docs around the CodeBlock
example that defines "cmux.newTerminal" / "claude" and the later examples (the
sections showing bare `"play.circle"` and `"./icons/codex.svg"`) to either use
one canonical icon form or add a concise "Icon encodings" subsection that lists
the accepted encodings and their canonical keys: object forms `{
"type":"symbol"|"sfSymbol"|"systemImage","name":"..." }`, `{
"type":"emoji","value":"..." }`, `{ "type":"image"|"file","path":"..." }`,
string shorthands `"emoji:..."`, `"file:..."`, bare SF Symbol names like
`"play.circle"`, and relative image paths; reference the `icon` fields in the
CodeBlock and any other examples so all snippets match the documented schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1489f37c-78da-43d5-ae40-960711714f0d
📒 Files selected for processing (18)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxActionTrust.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/CmuxDirectoryTrust.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/CmuxConfigTests.swiftweb/app/[locale]/docs/custom-commands/page.tsxweb/data/cmux-settings.schema.jsonweb/messages/en.jsonweb/messages/ja.json
💤 Files with no reviewable changes (2)
- web/data/cmux-settings.schema.json
- Sources/CmuxDirectoryTrust.swift
✅ Files skipped from review due to trivial changes (2)
- web/messages/en.json
- web/messages/ja.json
| var fingerprint: String { | ||
| let encoder = JSONEncoder() | ||
| encoder.outputFormatting = [.sortedKeys] | ||
| let data = (try? encoder.encode(self)) ?? Data() | ||
| return Self.sha256Hex(data) | ||
| } |
There was a problem hiding this comment.
Fail closed when trust descriptor encoding fails.
Line 18 hashes empty data on any encoding failure, causing all failed descriptors to share the same trusted fingerprint. Make fingerprinting optional/throwing and deny trust when encoding fails.
🛡️ Proposed fail-closed fix
- var fingerprint: String {
+ var fingerprint: String? {
let encoder = JSONEncoder()
encoder.outputFormatting = [.sortedKeys]
- let data = (try? encoder.encode(self)) ?? Data()
+ guard let data = try? encoder.encode(self) else { return nil }
return Self.sha256Hex(data)
}
@@
func isTrusted(_ descriptor: CmuxActionTrustDescriptor) -> Bool {
- trustedFingerprints.contains(descriptor.fingerprint)
+ guard let fingerprint = descriptor.fingerprint else { return false }
+ return trustedFingerprints.contains(fingerprint)
}
func trust(_ descriptor: CmuxActionTrustDescriptor) {
- trustedFingerprints.insert(descriptor.fingerprint)
+ guard let fingerprint = descriptor.fingerprint else { return }
+ guard trustedFingerprints.insert(fingerprint).inserted else { return }
save()
}Also applies to: 54-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CmuxActionTrust.swift` around lines 15 - 20, The current fingerprint
computed in var fingerprint: String masks JSON encoding failures by hashing
empty Data; change the API to surface encoding errors (make fingerprint either
throwing or optional: e.g., func fingerprint() throws -> String or var
fingerprint: String?) so encoding failure returns nil/throws instead of
producing a deterministic empty-data hash; update Self.sha256Hex usage to only
be called when encoding succeeds and ensure callers of fingerprint/trust-check
(the other similar property at the second occurrence around lines 54-61) treat a
nil/throw as "deny trust" (fail-closed) rather than accepting a default
fingerprint.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03c0eb649b
ℹ️ 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".
| ?? (((directory as NSString).appendingPathComponent(".cmux") as NSString) | ||
| .appendingPathComponent("cmux.json")) |
There was a problem hiding this comment.
When no project config is found, updateLocalConfigPath now defaults to .../.cmux/cmux.json. The file watcher then tries to watch that .cmux directory, but if .cmux does not exist yet, startLocalDirectoryWatcher() exits early and no watcher remains active. In that state, creating either .cmux/cmux.json or legacy cmux.json later in the project is not detected until the workspace directory changes or the app restarts, so live config loading silently regresses for new projects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by watching the project directory while waiting for the first local config, including the empty .cmux directory plus legacy cmux.json case.
— Claude Code
| if let commandName = action.workspaceCommandName, | ||
| let command = commands.first(where: { $0.name == commandName }) { | ||
| return execute( |
There was a problem hiding this comment.
This branch executes any command matched by name without verifying that it is actually a workspace command. If an action is declared as type: "workspaceCommand" but points to a simple command entry, it will run terminal input instead of being rejected, which contradicts the workspace-only validation already applied in other paths (for example new-workspace and tab-bar workspace resolution). This makes misconfigured actions behave inconsistently and can run unintended shell commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by rejecting workspaceCommand actions unless the referenced command has a workspace definition.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b643c55cc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let targetTerminal = (target == .currentTerminal) ? tabManager.selectedWorkspace?.focusedTerminalPanel : nil | ||
| let targetWorkspace = (target == .newTabInCurrentPane) ? tabManager.selectedWorkspace : nil |
There was a problem hiding this comment.
execute(action:...) computes targetTerminal as optional for .currentTerminal actions, but it still proceeds through authorization and returns true even when no terminal is focused. In that state (for example, browser-focused workspaces), the action is marked handled while no input is sent, so callers can skip their fallback behavior and users get a silent no-op. Add an early guard for .currentTerminal so this path returns false when there is no terminal target.
Useful? React with 👍 / 👎.
| case .codex: | ||
| return "Codex" | ||
| case .claudeCode: | ||
| return "Claude Code" |
There was a problem hiding this comment.
defaultTitle(for:) introduces hardcoded English labels ("Codex", "Claude Code") for user-visible action titles when config omits title, so these remain untranslated in non-English locales. /workspace/cmux/AGENTS.md explicitly requires all user-facing strings to be localized; these defaults should use localized keys (and corresponding Localizable.xcstrings entries) instead of string literals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by localizing the default Codex and Claude Code action titles.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8072c47443
ℹ️ 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".
| case .currentTerminal: | ||
| self.selectedTerminalPanel(inPane: pane)?.sendInput(shellInput) |
There was a problem hiding this comment.
When a surface-tab-bar button is configured with target: "currentTerminal", this path always reports success after authorization but sends input via selectedTerminalPanel(inPane:)?.sendInput(...). If the selected tab in that pane is a browser (no terminal panel), the optional send becomes a silent no-op: the click is consumed (and may even show/trust the confirmation dialog) while no command runs. This makes current-terminal actions appear broken in mixed browser/terminal panes and prevents any fallback behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c0e9344d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if didExecute, | ||
| let initialWorkspace, | ||
| context.tabManager.tabs.count > 1, | ||
| context.tabManager.tabs.contains(where: { $0 === initialWorkspace }), | ||
| context.tabManager.selectedWorkspace !== initialWorkspace { |
There was a problem hiding this comment.
This cleanup check assumes didExecute means the configured workspace command has already run, but for untrusted project actions CmuxConfigExecutor.authorizeProjectActionIfNeeded returns true immediately after starting a sheet and runs the command later in the callback. In the no-window fallback path (performNewWorkspaceAction), that means the initial default workspace is checked/removed too early and remains open after the user clicks Run Once/Trust and Run, so one New Workspace action can still leave two workspaces. Fresh evidence: the executor returns before user response in CmuxConfigExecutor (presentConfirmDialog branch).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by moving initial-workspace cleanup into an executor callback that runs only after the configured command is authorized and executed.
— Claude Code
| if let action = globalActions[newWorkspaceActionID] { | ||
| configuredNewWorkspaceAction = action.definition |
There was a problem hiding this comment.
When only the global config provides ui.newWorkspace.action, this branch resolves the action ID from globalActions only, so a local config cannot override that action definition by reusing the same ID. This breaks the documented local-precedence merge behavior used elsewhere (for commands and tab-bar action resolution) and forces projects to duplicate the ui.newWorkspace.action placement just to customize the action body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by resolving a global ui.newWorkspace.action through the merged action map so local action definitions can override the global action body.
— Claude Code
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e20e59caef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let configSourcePath = terminalCommandSourcePath | ||
| ?? workspaceCommand?.sourcePath | ||
| ?? button.actionSourcePath |
There was a problem hiding this comment.
surfaceButtonTrustDescriptor only derives configSourcePath from command/workspace sources (terminalCommandSourcePath, workspaceCommand?.sourcePath, button.actionSourcePath) and ignores the tab-bar config file source. For inline local buttons (or local icon overrides on global actions), this leaves configPath nil/global, so isTrustedSurfaceButton treats them as trusted and Workspace.applySurfaceTabBarButtons renders project-local image icons instead of the lock placeholder before trust is granted. This weakens the trust boundary for untrusted project configs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by including the surface-tab-bar config source in the button trust descriptor, including inline local icon overrides.
— Claude Code
This branch was successfully deployed
1 participant
Summary
uiplacements for the new-workspace action and surface tab-bar buttons.newWorkspaceCommandandsurfaceTabBarButtonscompatibility, including[]to disable default tab-bar buttons.Related
Testing
swift testinvendor/bonsplit, verified split button configuration, icon handling, duplicate action handling, and tab bar behavior../scripts/test-unit.sh -derivedDataPath /tmp/cmux-btncfg-unit test -only-testing:cmuxTests/CmuxConfigDecodingTests, verified cmux config decoding and command/button resolution../scripts/test-unit.sh -derivedDataPath /tmp/cmux-feat-custom-new-command-unit test -only-testing:cmuxTests/CmuxConfigDecodingTests, re-ran the same config tests after review fixes.bunx tsc --noEmitinweb, verified the docs page still typechecks.git diff --checkandgit -C vendor/bonsplit diff --check, verified whitespace cleanliness../scripts/reload.sh --tag feat-custom-new-command, verified the tagged macOS dogfood build compiles after the latest app/runtime changes.Demo Video
Review Trigger
@coderabbitai reviewrequested after the latest push.Checklist
Summary by CodeRabbit
New Features
Tests
Documentation
Chores