Follow up equalize splits shortcut fixes - #3309
Merged
Merged
Conversation
Ghostty already has an equalize_splits action, but cmux did not expose an editable app-level shortcut for restoring split pane divider balance. Route Cmd+Ctrl+= through the cmux shortcut layer to the existing TabManager equalize implementation, show that shortcut in the command palette/menu/docs, and allow settings.json overrides via the existing shortcut registry. Constraint: New cmux-owned shortcuts must be in KeyboardShortcutSettings, settings.json schema, and docs. Rejected: Rely on the existing RPC workaround | not discoverable for keyboard-driven pane workflows Confidence: medium Scope-risk: narrow Directive: Keep equalize_splits wired to the existing TabManager.equalizeSplits path unless split layout ownership changes. Tested: git diff --check; JSON parse for web/data/cmux-settings.schema.json and Resources/Localizable.xcstrings Not-tested: Full Xcode app build blocked locally because Zig is not installed and GhosttyKit.xcframework is absent
|
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 an "Equalize Splits" command and keyboard shortcut (Cmd+Ctrl+=) that centers all split dividers (0.5) in the selected workspace; implements core equalization, UI/menu wiring, AppDelegate routing, follow-up reattempts and geometry reconcile, localization, tests, and project file updates. ChangesEqualize Splits Feature
Sequence DiagramsequenceDiagram
actor User
participant OS
participant AppDelegate
participant TabManager
participant Workspace
participant BonsplitController
User->>OS: Press Cmd+Ctrl+=
OS->>AppDelegate: deliver shortcut event
AppDelegate->>AppDelegate: matchConfiguredShortcut(.equalizeSplits)
AppDelegate->>AppDelegate: performEqualizeSplitsShortcut()
AppDelegate->>TabManager: equalizeSplits(tabId:)
TabManager->>TabManager: equalizeSplitsOnce(in:)
TabManager->>BonsplitController: traverse tree & set divider = 0.5
BonsplitController-->>TabManager: report success/failure
TabManager->>Workspace: didProgrammaticallyChangeSplitGeometry()
Workspace->>BonsplitController: layoutSnapshot()
BonsplitController-->>Workspace: return layout snapshot
Workspace->>Workspace: scheduleTerminalGeometryReconcile()
Note over TabManager: Schedule immediate follow-up reattempt
Note over TabManager: Schedule delayed reattempt (0.15s)
TabManager->>BonsplitController: re-traverse & verify equalization persists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 |
Contributor
Greptile SummaryThis PR follows up #3200 by wiring Confidence Score: 4/5Safe to merge; only P2 findings present — an edge-case stale-snapshot scenario and a test helper gap. All findings are P2. The partial-equalization snapshot issue requires setDividerPosition to fail on at least one split while succeeding on others — an unlikely runtime condition given IDs are sourced from the live snapshot. The test helper oversight is a quality concern, not a correctness blocker. Sources/TabManager.swift — the didEqualize guard condition that gates didProgrammaticallyChangeSplitGeometry(). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Keyboard / Menu / Palette]) --> B{selectedWorkspace?}
B -- no --> C[silent no-op]
B -- yes --> D[TabManager.equalizeSplits]
D --> E[traverse treeSnapshot recursively]
E --> F{setDividerPosition 0.5 for each split}
F -- all succeed --> G[didEqualize = true]
F -- any fails --> H[allSucceeded = false\ndidEqualize = false]
G --> I[tab.didProgrammaticallyChangeSplitGeometry]
H --> J[snapshot NOT refreshed\n⚠️ stale if any splits moved]
I --> K[tmuxLayoutSnapshot = layoutSnapshot]
I --> L[scheduleTerminalGeometryReconcile]
Reviews (1): Last reviewed commit: "Make equalize splits shortcut merge-read..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
679-701:⚠️ Potential issue | 🟡 MinorUse the actual
=binding here.The fixture currently records
cmd+ctrl+e, so this test does not exercise the newCmd+Ctrl+=shortcut. If the equalize-splits binding regresses, this assertion would still pass.🛠️ Suggested fix
- "equalizeSplits": "cmd+ctrl+e", + "equalizeSplits": "cmd+ctrl+=",- StoredShortcut(key: "e", command: true, shift: false, option: false, control: true) + StoredShortcut(key: "=", command: true, shift: false, option: false, control: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 679 - 701, The test fixture currently uses "cmd+ctrl+e" so it doesn't validate the new equalize-splits binding; update the JSON fixture string where the "equalizeSplits" binding is specified to use "cmd+ctrl+=", and then update the expected assertion for store.override(for: .equalizeSplits) to expect StoredShortcut(key: "=", command: true, shift: false, option: false, control: true) so KeyboardShortcutSettingsFileStore.override(for:) is actually validated for the "=" key.
🤖 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/ContentView.swift`:
- Around line 7767-7770: The registry callback currently calls
tabManager.equalizeSplits(tabId:) and ignores its Bool result; change the
callback (used in ContentView.swift alongside similar usages in cmuxApp.swift
and AppDelegate.swift) to capture the return value and handle failure (e.g., log
an error via your logger, present an alert to the user, or update UI state) when
equalizeSplits returns false so failures such as "no splits found" or "divider
change rejected" are surfaced rather than silently discarded.
---
Outside diff comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 679-701: The test fixture currently uses "cmd+ctrl+e" so it
doesn't validate the new equalize-splits binding; update the JSON fixture string
where the "equalizeSplits" binding is specified to use "cmd+ctrl+=", and then
update the expected assertion for store.override(for: .equalizeSplits) to expect
StoredShortcut(key: "=", command: true, shift: false, option: false, control:
true) so KeyboardShortcutSettingsFileStore.override(for:) is actually validated
for the "=" key.
🪄 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: 970eb3ef-e4b3-4fe7-bddb-aec7ee8d07e7
📒 Files selected for processing (11)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/data/cmux-settings.schema.jsonweb/data/cmux-shortcuts.ts
Follow-up to community PR #3200. The shortcut was exposed, but review and manual verification found three rough edges: the regression did not prove the seeded layout was uneven, no-op single-pane invocations beeped, and a single programmatic equalize pass let parent split resizing leave nested split views stale until a later interaction. This keeps the shortcut configurable through KeyboardShortcutSettings and settings.json, gives its Settings label a dedicated localization key, treats single-pane equalize as a silent no-op, and reasserts equalized divider positions on deferred main-queue follow-up passes so nested horizontal and vertical splits settle from one shortcut invocation. Constraint: Follow-up to community PR #3200; push only to manaflow-ai/cmux Rejected: Treat equalize false as user-facing error | single-pane workspaces are a valid no-op Rejected: Only refresh terminal geometry cache | manual verification showed the nested NSSplitView itself still needed a deferred divider reassertion Confidence: high Scope-risk: narrow Tested: git diff --check Tested: JSON parse for Resources/Localizable.xcstrings and web/data/cmux-settings.schema.json Tested: ./scripts/reload.sh --tag codex-equalize-splits-shortcut-followup --launch Tested: Manual tagged build verification with uneven nested horizontal+vertical splits; one Cmd+Ctrl+= changed root 925/405 to 665/665 and nested 298/618 to 458/458 without another click or keypress Not-tested: Local unit tests because repo runner invokes xcodebuild directly
austinywang
force-pushed
the
codex/equalize-splits-shortcut-followup
branch
from
April 29, 2026 19:10
c337696 to
c8a583e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
772-787: Derive the trigger from the configured shortcut instead of hard-codingCmd+Ctrl+=.
equalizeSplitsis now settings-backed, so this regression is more stable if it synthesizes the event from the resolved shortcut for.equalizeSplitsrather than baking in the current default. Otherwise a futuresettings.jsondefault change will break the test without a routing regression.Based on learnings:
KeyboardShortcutSettings.setShortcut(...)is a no-op whenKeyboardShortcutSettings.isManagedBySettingsFile(action)returns true, preserving the settings file as the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 772 - 787, Test currently hard-codes a Cmd+Ctrl+=" key event; instead fetch the resolved shortcut for the .equalizeSplits action (via the KeyboardShortcutSettings API used in the app) and synthesize the keydown event from that shortcut (use the resolved key, modifiers and keyCode when calling makeKeyDownEvent) so the test follows settings-backed shortcuts; ensure you still call appDelegate.debugHandleCustomShortcut(event: event) and keep the DEBUG-only assertion, and do not call KeyboardShortcutSettings.setShortcut(...) in the test when KeyboardShortcutSettings.isManagedBySettingsFile(action) may be true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 772-787: Test currently hard-codes a Cmd+Ctrl+=" key event;
instead fetch the resolved shortcut for the .equalizeSplits action (via the
KeyboardShortcutSettings API used in the app) and synthesize the keydown event
from that shortcut (use the resolved key, modifiers and keyCode when calling
makeKeyDownEvent) so the test follows settings-backed shortcuts; ensure you
still call appDelegate.debugHandleCustomShortcut(event: event) and keep the
DEBUG-only assertion, and do not call KeyboardShortcutSettings.setShortcut(...)
in the test when KeyboardShortcutSettings.isManagedBySettingsFile(action) may be
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3cc4997-d817-4fb1-960e-834fa175c02e
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (4)
- Resources/Localizable.xcstrings
- cmuxTests/WorkspaceUnitTests.swift
- Sources/Workspace.swift
- Sources/ContentView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/cmuxApp.swift
…litGeometry to Workspace+EqualizeSplitsSupport.swift Preserve main's refactored command, menu, schema, and shortcut structure while reapplying the equalize-splits feature through the shared shortcut action and TabManager geometry path. Keep file-length CI green by moving the new geometry-cache helper out of Workspace and the shortcut parser assertion out of WorkspaceUnitTests. Constraint: Build must use ./scripts/reload.sh with the branch tag; local test suites are intentionally not run.\nRejected: Bump Swift file-length budgets | CI requires staying within existing budgets.\nConfidence: high\nScope-risk: moderate\nTested: git diff --check; git diff --cached --check; conflict-marker scan; swift_file_length_budget.py; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; remote verified as origin=https://github.com/manaflow-ai/cmux.git\nNot-tested: Local unit/UI test suites, per repo policy.
…apshots The helper now lives outside Workspace.swift to satisfy the file-length budget, so the cached tmux layout snapshot needs a module-internal setter for same-module Workspace extensions. Constraint: Workspace.swift must remain within the existing file-length budget.\nRejected: Put didProgrammaticallyChangeSplitGeometry back in Workspace.swift | would regress the budget fix.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; swift_file_length_budget.py; conflict-marker scan.\nNot-tested: Local unit/UI test suites, per repo policy.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Sources/AppDelegate+EqualizeSplitsShortcut.swift (2)
3-4: 💤 Low valueRedundant double optional-chain on
tabManager— capture it once.Inside the
if let workspaceblock,tabManageris already proven non-nil (it returned a non-nilselectedWorkspace), yet line 4 re-applies?.. CapturingtabManagerupfront removes the redundant check and aligns the style with the command-palette caller (Context snippet 2), which holds a captured non-optional reference.♻️ Proposed refactor
extension AppDelegate { func performEqualizeSplitsShortcut() { - if let workspace = tabManager?.selectedWorkspace { - _ = tabManager?.equalizeSplits(tabId: workspace.id) + guard let tabManager, let workspace = tabManager.selectedWorkspace else { return } + `#if` DEBUG + cmuxDebugLog("equalizeSplits shortcut; workspaceId=\(workspace.id)") + `#endif` + _ = tabManager.equalizeSplits(tabId: workspace.id) - } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate`+EqualizeSplitsShortcut.swift around lines 3 - 4, The code redundantly optional-chains tabManager inside the if let workspace block; instead capture tabManager once and use the non-optional reference when calling equalizeSplits. Change the conditional to bind tabManager and its selectedWorkspace together (e.g. if let tabManager = tabManager, let workspace = tabManager.selectedWorkspace) and then call tabManager.equalizeSplits(tabId: workspace.id) (remove the `?.`), referencing the symbols tabManager, selectedWorkspace, equalizeSplits, tabId, and workspace.id.
2-6: ⚡ Quick winAdd
cmuxDebugLogtracing per the AppDelegate debug-logging guideline.
performEqualizeSplitsShortcut()is a keyboard-shortcut handler living in an AppDelegate extension, which squarely falls under the "log key events inAppDelegate.swift" rule. Without a debug trace it is invisible in the ring-buffer log when diagnosing shortcut routing regressions.♻️ Proposed addition
extension AppDelegate { func performEqualizeSplitsShortcut() { + `#if` DEBUG + cmuxDebugLog("equalizeSplits shortcut invoked; hasWorkspace=\(tabManager?.selectedWorkspace != nil)") + `#endif` if let workspace = tabManager?.selectedWorkspace { _ = tabManager?.equalizeSplits(tabId: workspace.id) } } }As per coding guidelines: "Log key events in
AppDelegate.swift… usecmuxDebugLog("message")… all call sites must be wrapped in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate`+EqualizeSplitsShortcut.swift around lines 2 - 6, Add a DEBUG-only cmuxDebugLog call to the performEqualizeSplitsShortcut() handler to record the shortcut invocation and relevant context; specifically, inside performEqualizeSplitsShortcut() (before calling tabManager?.equalizeSplits) wrap a cmuxDebugLog invocation in `#if` DEBUG / `#endif` and log a concise message including the workspace id (e.g., workspace.id) and that equalizeSplits was requested so the shortcut routing shows up in AppDelegate traces.cmuxTests/AppDelegateEqualizeSplitsShortcutTests.swift (1)
76-76: ⚡ Quick winConsider widening the RunLoop wait to reduce timing flakiness.
The
scheduleEqualizeSplitsFollowUpdefers a second equalize pass viaasyncAfter(deadline: .now() + 0.15). The current 0.2 s wait leaves only ~50 ms of margin; under CI scheduler pressure this can cause the assertion to run before the follow-up pass settles.♻️ Suggested change
- RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.2)) + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.35))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateEqualizeSplitsShortcutTests.swift` at line 76, The RunLoop wait in the test is too tight for scheduleEqualizeSplitsFollowUp (which calls asyncAfter(deadline: .now() + 0.15)), so increase the wait duration (e.g., from 0.2s to something larger like 0.35–0.5s) where RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.2)) is used in AppDelegateEqualizeSplitsShortcutTests to give the follow-up equalize pass enough margin under CI scheduling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/AppDelegateEqualizeSplitsShortcutTests.swift`:
- Around line 71-75: The else branch currently calls
XCTFail("debugHandleCustomShortcut is only available in DEBUG") but does not
stop execution, causing the test to continue into RunLoop.main.run(...) and
subsequent assertions; after the XCTFail in the non-DEBUG branch add an explicit
return so the test exits early (i.e., modify the `#else` to call XCTFail(...)
followed by return) to prevent spurious cascading failures from
appDelegate.debugHandleCustomShortcut not being invoked.
In `@Sources/KeyboardShortcutSettings.swift`:
- Line 158: The equalize-splits shortcut handler performEqualizeSplitsShortcut()
in AppDelegate+EqualizeSplitsShortcut.swift is missing the transient
terminal-focus guard; add an early return that calls
shouldSuppressSplitShortcutForTransientTerminalFocusState() and exits if true,
mirroring the checks in splitRight and splitDown. Keep the rest of the method
unchanged (it should still call bonsplitController to adjust geometry and then
didProgrammaticallyChangeSplitGeometry()), so the operation is suppressed
whenever the terminal view is hidden, not attached, or lacks a focused panel.
In `@Sources/TabManager`+EqualizeSplits.swift:
- Around line 56-65: The code currently does an early return when
UUID(uuidString: splitNode.id) fails inside the .split case, which stops
recursion into splitNode.first/second; change the guard to handle the nil UUID
without returning — mark allSucceeded = false when UUID parsing fails and then
continue to recurse into splitNode.first and splitNode.second (same recursion
used when the UUID is valid). Keep the existing
controller.setDividerPosition(0.5, forSplit: splitId) failure handling (set
allSucceeded = false) only when you have a valid splitId; ensure the .split case
never returns early so nested valid splits are still equalized by
equalizeSplitsOnce.
- Line 63: In TabManager+EqualizeSplits.swift update the setDividerPosition call
to pass fromExternal: true so it matches other layout-change paths; specifically
change the invocation on controller.setDividerPosition(..., forSplit: splitId)
to include fromExternal: true (keeping the same 0.5 position and splitId) so
downstream geometry notification/reconciliation behaves consistently with
resizeSplit, TerminalController, and workspace restore.
---
Nitpick comments:
In `@cmuxTests/AppDelegateEqualizeSplitsShortcutTests.swift`:
- Line 76: The RunLoop wait in the test is too tight for
scheduleEqualizeSplitsFollowUp (which calls asyncAfter(deadline: .now() +
0.15)), so increase the wait duration (e.g., from 0.2s to something larger like
0.35–0.5s) where RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.2)) is
used in AppDelegateEqualizeSplitsShortcutTests to give the follow-up equalize
pass enough margin under CI scheduling.
In `@Sources/AppDelegate`+EqualizeSplitsShortcut.swift:
- Around line 3-4: The code redundantly optional-chains tabManager inside the if
let workspace block; instead capture tabManager once and use the non-optional
reference when calling equalizeSplits. Change the conditional to bind tabManager
and its selectedWorkspace together (e.g. if let tabManager = tabManager, let
workspace = tabManager.selectedWorkspace) and then call
tabManager.equalizeSplits(tabId: workspace.id) (remove the `?.`), referencing
the symbols tabManager, selectedWorkspace, equalizeSplits, tabId, and
workspace.id.
- Around line 2-6: Add a DEBUG-only cmuxDebugLog call to the
performEqualizeSplitsShortcut() handler to record the shortcut invocation and
relevant context; specifically, inside performEqualizeSplitsShortcut() (before
calling tabManager?.equalizeSplits) wrap a cmuxDebugLog invocation in `#if` DEBUG
/ `#endif` and log a concise message including the workspace id (e.g.,
workspace.id) and that equalizeSplits was requested so the shortcut routing
shows up in AppDelegate traces.
🪄 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: 3859313f-32e5-4d8b-accb-1d85b02ffdea
📒 Files selected for processing (15)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate+EqualizeSplitsShortcut.swiftSources/AppDelegate.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager+EqualizeSplits.swiftSources/TabManager.swiftSources/Workspace+EqualizeSplitsSupport.swiftSources/Workspace.swiftSources/cmuxApp+EqualizeSplitsMenu.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateEqualizeSplitsShortcutTests.swiftcmuxTests/KeyboardShortcutSettingsEqualizeSplitsTests.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager.swift
✅ Files skipped from review due to trivial changes (2)
- cmuxTests/KeyboardShortcutSettingsEqualizeSplitsTests.swift
- Resources/Localizable.xcstrings
Branch-style tags can contain slashes. reload.sh already derives a filesystem-safe tag slug for paths, sockets, and DerivedData; use that same slug for the default copied app bundle name so the mandatory branch-name tag can build and launch. Constraint: The user-required reload command passes a branch name containing '/'.\nRejected: Require a different --tag value | task requires the branch name exactly.\nConfidence: high\nScope-risk: narrow\nTested: bash -n scripts/reload.sh; git diff --check scripts/reload.sh.\nNot-tested: Local test suites, per repo policy.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/reload.sh (1)
340-340: Consider keeping a separateDISPLAY_NAMEfor plist entries to preserve original-case tags in the DockUsing
TAG_SLUGinstead ofTAGcorrectly fixes filesystem-safety andpkill-pattern issues (lines 543, 662). However,CFBundleNameandCFBundleDisplayName(lines 548–551) will now display as lowercase-hyphenated slugs (e.g.,cmux DEV my-feature) instead of the original mixed-case tag. If developers rely on human-readable tag names in the macOS Dock or Activity Monitor, consider usingTAGfor a separateDISPLAY_NAMEvariable in the plist while retainingAPP_NAMEas the sanitized slug for paths and process patterns.Note:
scripts/reloads.sh(staging) still setsAPP_NAME="cmux STAGING ${TAG}"(line 112) and shares the same filesystem-safety risk, but that is outside this PR's scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` at line 340, The plist currently uses the sanitized APP_NAME ("APP_NAME=\"cmux DEV ${TAG_SLUG}\"") which makes CFBundleName/CFBundleDisplayName display the slug; introduce a separate DISPLAY_NAME variable built from TAG (preserving original case) and use DISPLAY_NAME for CFBundleName and CFBundleDisplayName while keeping APP_NAME as the filesystem/process-safe slug (TAG_SLUG) for paths, pkill patterns and other uses; update the plist generation to reference DISPLAY_NAME for human-facing fields and leave APP_NAME unchanged for all filesystem/process operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/reload.sh`:
- Line 340: The plist currently uses the sanitized APP_NAME ("APP_NAME=\"cmux
DEV ${TAG_SLUG}\"") which makes CFBundleName/CFBundleDisplayName display the
slug; introduce a separate DISPLAY_NAME variable built from TAG (preserving
original case) and use DISPLAY_NAME for CFBundleName and CFBundleDisplayName
while keeping APP_NAME as the filesystem/process-safe slug (TAG_SLUG) for paths,
pkill patterns and other uses; update the plist generation to reference
DISPLAY_NAME for human-facing fields and leave APP_NAME unchanged for all
filesystem/process operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9064e31-d4f3-4498-904e-00d6a128fd10
📒 Files selected for processing (2)
Sources/Workspace.swiftscripts/reload.sh
Settings-file parsing should normalize action-specific shortcut shapes without consulting the shared settings store that is currently being constructed. Using the existing conflict-ignoring normalization path prevents the launch-time dispatch_once recursion crash while keeping numbered and system-wide shortcut validation. Constraint: KeyboardShortcutSettingsFileStore.swift is at its file-length budget.\nRejected: Use full normalizedRecordedShortcut during config load | it consults KeyboardShortcutSettings.settingsFileStore and recursively initializes the store.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check; swift_file_length_budget.py.\nNot-tested: Local unit/UI test suites, per repo policy.
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 82221b4. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 865-868: resolvedRecordedShortcutIgnoringConflicts still triggers
KeyboardShortcutSettings.shared during config parsing because
normalizedSystemWideHotkeyShortcutResult calls systemWideHotkeyConflicts which
in turn queries reservedSystemWideHotkeyShortcuts and the live store; to fix,
change normalizedSystemWideHotkeyShortcutResult (or
resolvedRecordedShortcutIgnoringConflicts) to accept a "parsing" flag (or detect
CmuxSettingsFileStore.parse context) and when parsing is true skip calling
systemWideHotkeyConflicts (i.e., treat conflicts as non-existent) OR modify
reservedSystemWideHotkeyShortcuts to return a static reserved list without
querying KeyboardShortcutSettings.shortcut(for:) or
settingsFileStore.override(for:), ensuring no access to the shared store during
parse.
🪄 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: bf024f2c-874d-4750-96a2-88c8dfb55563
📒 Files selected for processing (1)
Sources/KeyboardShortcutSettingsFileStore.swift
Review feedback showed the equalize shortcut could mutate some divider positions while skipping snapshot refresh, terminal reconciliation, or follow-up passes when another divider failed. The handler now uses the same transient terminal-focus guard as split creation, records DEBUG routing traces, and debug-logs failed no-op paths without surfacing single-pane workspaces to users. Equalize traversal now keeps recursing after malformed split IDs, uses Bonsplit's external-change path, and refreshes workspace geometry whenever a split node was visited. Shortcut config parsing also avoids live system-wide conflict checks while loading settings files so it cannot recurse through the shared store during initialization. Constraint: Single-pane equalize remains a silent no-op for users. Rejected: Surface an alert on false equalize results | The PR behavior explicitly keeps single-pane workspaces silent; DEBUG logging gives diagnostics without changing UX. Confidence: high Scope-risk: moderate Directive: Keep programmatic split geometry changes on the same Workspace splitTabBar(didChangeGeometry:) path so layout snapshot, terminal geometry, and focus reconciliation stay coupled. Tested: git diff --check; ./scripts/reload.sh --tag pr3309-comments Not-tested: Local unit tests; project policy says tests run in CI/prefer CI.
The conflict-bypass fix for settings-file parsing was behaviorally correct, but formatting alone pushed two already-budgeted Swift files over the CI line-count guard. Collapse the added parsing arguments back into the existing compact style without changing the recursion fix. Constraint: workflow-guard-tests enforces existing Swift file length budgets. Rejected: Refresh the budget | This PR can stay inside the current budget without accepting new debt. Confidence: high Scope-risk: narrow Tested: git diff --check; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; ./scripts/reload.sh --tag pr3309-comments Not-tested: Local unit tests; project policy says tests run in CI/prefer CI.
This was referenced May 4, 2026
This was referenced May 4, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
References #3200.
Note
Medium Risk
Touches split-geometry manipulation and shortcut routing, including async follow-up passes and layout snapshot refreshes that could affect pane layout/focus timing. No auth or data-handling changes.
Overview
Adds a dedicated Equalize Splits shortcut action (default
Cmd+Ctrl+=) with new localization (shortcut.equalizeSplits.label), and wires it through keyboard handling, the command palette, and a new main-menu command.Refactors equalize logic into
TabManager+EqualizeSplitsto run multi-pass divider normalization (including deferred main-queue follow-ups) and explicitly refresh workspace layout state after programmatic geometry changes, while making single-pane workspaces a silent no-op (debug-logged only).Hardens shortcut/settings parsing by allowing settings-file shortcut normalization without system-wide conflict checks, and adds regression tests covering shortcut routing, nested-split equalization, and settings-file parsing.
Reviewed by Cursor Bugbot for commit 0aaf57a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Behavior
Localization
Tests