feat: add quick Terminal panel, CLI/socket controls, and automation-safe focus behavior - #1996
Closed
jemutorres wants to merge 6 commits into
Closed
jemutorres wants to merge 6 commits into
jemutorres wants to merge 6 commits into
Conversation
|
@jemutorres is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughAdds a Quick Terminal feature: a toggleable NSPanel terminal with configurable position/size and auto-hide; exposes a keyboard shortcut (⌥⌘ Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as App UI (Menu / Palette / Shortcut)
participant CLI as CLI / External
participant App as AppDelegate
participant QT as QuickTerminalController
participant Term as TerminalSurface
User->>UI: trigger toggle/show/hide
CLI->>App: cmux quick-terminal [toggle|show|hide|status]
UI->>App: toggleQuickTerminalVisibility(activateApp?)
App->>QT: toggle/show/hide
alt Show flow
QT->>QT: compute finalFrame / hiddenFrame
QT->>QT: create/configure NSPanel
QT->>Term: instantiate/attach TerminalSurface
QT->>QT: animate frame & alpha in
QT-->>App: visible = true
else Hide flow
QT->>QT: animate frame & alpha out
QT->>QT: hide/detach NSPanel
QT-->>App: visible = false
end
App-->>UI: status / completion
App-->>CLI: JSON status (if requested)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR implements end-to-end Quick Terminal integration — a drop-down style floating terminal panel — wired up across keyboard shortcut, View menu, command palette, settings UI, socket v2 API, and CLI. The implementation follows established app patterns well: Key additions:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Shortcut/Menu/Palette
participant AppDelegate
participant QuickTerminalController
participant NSPanel
participant SocketCLI as Socket/CLI
participant TerminalController
User->>Shortcut/Menu/Palette: Toggle Quick Terminal
Shortcut/Menu/Palette->>AppDelegate: toggleQuickTerminal(_:)
AppDelegate->>QuickTerminalController: toggle(activateApp: true)
QuickTerminalController->>NSPanel: orderFront + makeKeyAndOrderFront
QuickTerminalController->>NSPanel: animate (hidden→final frame, α 0→1)
NSPanel-->>QuickTerminalController: completionHandler (Task @MainActor)
QuickTerminalController->>NSPanel: level .floating, makeFirstResponder
SocketCLI->>TerminalController: quick_terminal.toggle/show/hide
TerminalController->>AppDelegate: toggleQuickTerminalVisibility(activateApp: false)
AppDelegate->>QuickTerminalController: toggle(activateApp: false)
QuickTerminalController->>NSPanel: orderFront (no makeKeyAndOrderFront)
Note over QuickTerminalController,NSPanel: macOS app focus NOT stolen
TerminalController->>SocketCLI: status payload (visible, position, sizes, auto_hide)
NSPanel->>QuickTerminalController: windowDidResignKey (autoHide=true)
QuickTerminalController->>NSPanel: animate (final→hidden frame, α 1→0)
NSPanel-->>QuickTerminalController: completionHandler (Task @MainActor)
QuickTerminalController->>NSPanel: orderOut
QuickTerminalController->>AppDelegate: previousFrontmostApp.activate (if set)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 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:2128">
P2: Quick-terminal show/hide requests are dropped during animations: `show`/`hide` both `return` when `isTransitioning == true` and there’s no pending action replay in the completion handlers. A rapid `show` then `hide` (or toggle) will ignore the hide and leave the panel visible, causing automation-visible state mismatches.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 6
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
188-194: Add key-equivalent conversion assertions for the backtick shortcut.Since
`is a special key in menu routing paths, consider assertingkeyEquivalent/menuItemKeyEquivalentandeventModifiershere too (similar to other shortcut tests) to catch regressions earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 188 - 194, The test currently checks only the raw KeyboardShortcutSettings.Action.toggleQuickTerminal.defaultShortcut fields; add assertions that its keyEquivalent (or menuItemKeyEquivalent) equals "`" and that the computed eventModifiers/menu event modifiers match the expected set (command + option, no shift/control) so the shortcut's keyEquivalent/menuItemKeyEquivalent and eventModifiers are validated like the other shortcut tests; locate the shortcut via KeyboardShortcutSettings.Action.toggleQuickTerminal.defaultShortcut and assert both the menu key equivalent property and the event modifier representation to catch routing regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 8: In CHANGELOG.md, escape the literal backtick in the shortcut text so
Markdown doesn't start an inline code span: replace the unescaped sequence
"Option+Command+`" with an escaped backtick form (e.g., "Option+Command+\`") or
wrap the whole shortcut safely as a code span that includes the escaped backtick
(e.g., `Option+Command+\``) so the "(default: ...)" text renders and lints
correctly.
In `@CLI/cmux.swift`:
- Around line 2596-2599: The parsing treats "--" as a subcommand when it's the
first token; update the logic that computes subcommand and remaining to
normalize commandArgs by removing a leading "--" (and any leading-only
separators) before selecting subcommand. Specifically, in the block that reads
commandArgs and assigns subcommand and remaining, first strip a leading "--"
from commandArgs (or skip all leading elements equal to "--"), then compute
subcommand = commandArgs.first?.lowercased() ?? "toggle" and remaining =
Array(commandArgs.dropFirst()).filter { $0 != "--" }; keep the existing
unknown-argument error throw but base it on the normalized args so "cmux
quick-terminal -- status" correctly interprets "status" as the subcommand.
In `@README.md`:
- Line 138: The markdown table row contains a raw backtick in the shortcut text
("⌥ ⌘ `") which can break rendering; update the README table row to escape the
backtick (e.g., use "⌥ ⌘ \`") or wrap the key(s) in <kbd> tags (e.g.,
<kbd>⌥</kbd> <kbd>⌘</kbd> <kbd>`</kbd>) so the shortcut displays correctly; edit
the cell that currently contains "⌥ ⌘ `".
In `@Sources/AppDelegate.swift`:
- Around line 9636-9641: The quick-terminal toggle currently consumes bare Cmd+`
(keyCode 50) or Cmd+Shift+` before AppKit can perform window cycling; before
calling matchShortcut/toggleQuickTerminalVisibility in AppDelegate.swift, add a
guard that rejects events where event.keyCode == 50 and the modifier flags
represent Command alone or Command+Shift (i.e., contains .command and optionally
.shift but no other modifiers), returning false so AppKit can handle native
window switching; alternatively, ensure KeyboardShortcutSettings.shortcut(for:
.toggleQuickTerminal) cannot be set to that key combo, but the immediate fix is
to check event.keyCode == 50 and modifierFlags and skip handling in this branch.
In `@Sources/TerminalController.swift`:
- Around line 141-143: The focus-intent allowlist erroneously includes
quick_terminal.toggle, quick_terminal.show, and quick_terminal.hide which grants
socket/CLI automation the ability to change in-app focus; remove these three
command entries from the allowlist (the list/array that currently contains
"quick_terminal.toggle", "quick_terminal.show", "quick_terminal.hide") so only
explicit focus-intent commands remain (e.g., window.focus, workspace.select/...,
surface.focus, pane.focus, browser focus commands and v1 focus equivalents);
after removal, run existing focus-related unit/integration checks to ensure no
other code depends on those quick_terminal entries and update any test mocks or
documentation referencing them.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 188-194: The test currently checks only the raw
KeyboardShortcutSettings.Action.toggleQuickTerminal.defaultShortcut fields; add
assertions that its keyEquivalent (or menuItemKeyEquivalent) equals "`" and that
the computed eventModifiers/menu event modifiers match the expected set (command
+ option, no shift/control) so the shortcut's
keyEquivalent/menuItemKeyEquivalent and eventModifiers are validated like the
other shortcut tests; locate the shortcut via
KeyboardShortcutSettings.Action.toggleQuickTerminal.defaultShortcut and assert
both the menu key equivalent property and the event modifier representation to
catch routing regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24332016-970b-4090-a95e-b058dbf0ad31
📒 Files selected for processing (10)
CHANGELOG.mdCLI/cmux.swiftREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
9636-9641:⚠️ Potential issue | 🟠 MajorDon't let this bypass palette protection or native `Cmd+`` window cycling.
Because this branch runs before the palette-consumption guard and doesn't reserve
keyCode == 50, a user-assigned quick-terminal shortcut can still fire while the command palette is typing, and bareCmd+\`` /Cmd+Shift+`` can preempt AppKit's native window switcher.Based on learnings, "Cmd+` (command-backtick) should not be routed directly" and "when the command palette is visible ... do not reorder shortcut handlers to bypass this guard."🛡️ Suggested guard
+ let isReservedWindowCycleShortcut = + event.keyCode == 50 && + (normalizedFlags == [.command] || normalizedFlags == [.command, .shift]) + // Allow quick-terminal toggle even when the key event comes from the quick // panel itself (which is not a main terminal window context). - if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) { + if !commandPaletteEffectiveInTargetWindow, + !isReservedWindowCycleShortcut, + matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) { _ = toggleQuickTerminalVisibility() return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9636 - 9641, The current early-return branch that matches matchShortcut(... shortcut: .toggleQuickTerminal) and calls toggleQuickTerminalVisibility() must not bypass the command-palette guard or allow native Cmd+` (keyCode == 50) to be handled here; update the condition to first check the palette-consumption guard (the same guard used elsewhere to prevent palette key handling) and explicitly ignore events with keyCode == 50 (and when necessary with shift modifier for Cmd+Shift+`) before calling toggleQuickTerminalVisibility(); locate the matchShortcut call, KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal) lookup, and toggleQuickTerminalVisibility invocation and add the additional checks so the quick-terminal shortcut is suppressed while the palette is visible and does not preempt AppKit window cycling.
2119-2125:⚠️ Potential issue | 🟠 MajorReturn whether the quick-terminal request actually ran.
show/hidecan legitimately no-op here (isTransitioning, already visible/hidden, orensurePanel()failure). ReturningVoidmakes those dropped requests indistinguishable from a real state change for automation callers.Also applies to: 2127-2130, 2191-2198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2119 - 2125, Change the toggle/show/hide call paths to return a Bool indicating whether the requested show/hide actually ran (instead of Void) so automation callers can distinguish no-ops; update toggle(activateApp:) to return Bool and return the result of calling show(activateApp:) or hide(restorePreviousApp:), and similarly change the other related methods (the other toggle overloads and the show/hide functions referenced in this diff) to return Bool and propagate true only when the action proceeds (i.e. not isTransitioning, not already in the target state, and ensurePanel() succeeded) and false when they legitimately no-op.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
5764-5789: Default the scriptable quick-terminal APIs to non-activating behavior.These wrappers are the automation-facing entry points, but their defaults still opt into activation/restoration. Making the safe behavior the default here reduces the chance of a future CLI/socket caller regressing the no-focus-steal contract.
As per coding guidelines, "Socket/CLI commands must not steal macOS app focus (no app activation/window raising side effects)."♻️ Suggested change
- func toggleQuickTerminalVisibility(activateApp: Bool = true) -> Bool { + func toggleQuickTerminalVisibility(activateApp: Bool = false) -> Bool { quickTerminalController.toggle(activateApp: activateApp) return true } - func showQuickTerminal(activateApp: Bool = true) -> Bool { + func showQuickTerminal(activateApp: Bool = false) -> Bool { quickTerminalController.show(activateApp: activateApp) return true } - func hideQuickTerminal(restorePreviousApp: Bool = true) -> Bool { + func hideQuickTerminal(restorePreviousApp: Bool = false) -> Bool { quickTerminalController.hide(restorePreviousApp: restorePreviousApp) return true } `@objc` func toggleQuickTerminal(_ sender: Any?) { _ = sender - _ = toggleQuickTerminalVisibility() + _ = toggleQuickTerminalVisibility(activateApp: true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5764 - 5789, Change the scriptable quick-terminal wrapper defaults to avoid activating or restoring the app: update the default parameter values on toggleQuickTerminalVisibility(activateApp: Bool = true), showQuickTerminal(activateApp: Bool = true), and hideQuickTerminal(restorePreviousApp: Bool = true) to false, and ensure the ObjC wrapper toggleQuickTerminal(_:) calls toggleQuickTerminalVisibility(activateApp: false) (or otherwise forwards false) so automation/CLI/socket callers do not cause app activation; locate these symbols: toggleQuickTerminalVisibility, showQuickTerminal, hideQuickTerminal, and toggleQuickTerminal to make the edits.
🤖 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/AppDelegate.swift`:
- Around line 9636-9641: The current early-return branch that matches
matchShortcut(... shortcut: .toggleQuickTerminal) and calls
toggleQuickTerminalVisibility() must not bypass the command-palette guard or
allow native Cmd+` (keyCode == 50) to be handled here; update the condition to
first check the palette-consumption guard (the same guard used elsewhere to
prevent palette key handling) and explicitly ignore events with keyCode == 50
(and when necessary with shift modifier for Cmd+Shift+`) before calling
toggleQuickTerminalVisibility(); locate the matchShortcut call,
KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal) lookup, and
toggleQuickTerminalVisibility invocation and add the additional checks so the
quick-terminal shortcut is suppressed while the palette is visible and does not
preempt AppKit window cycling.
- Around line 2119-2125: Change the toggle/show/hide call paths to return a Bool
indicating whether the requested show/hide actually ran (instead of Void) so
automation callers can distinguish no-ops; update toggle(activateApp:) to return
Bool and return the result of calling show(activateApp:) or
hide(restorePreviousApp:), and similarly change the other related methods (the
other toggle overloads and the show/hide functions referenced in this diff) to
return Bool and propagate true only when the action proceeds (i.e. not
isTransitioning, not already in the target state, and ensurePanel() succeeded)
and false when they legitimately no-op.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 5764-5789: Change the scriptable quick-terminal wrapper defaults
to avoid activating or restoring the app: update the default parameter values on
toggleQuickTerminalVisibility(activateApp: Bool = true),
showQuickTerminal(activateApp: Bool = true), and
hideQuickTerminal(restorePreviousApp: Bool = true) to false, and ensure the ObjC
wrapper toggleQuickTerminal(_:) calls toggleQuickTerminalVisibility(activateApp:
false) (or otherwise forwards false) so automation/CLI/socket callers do not
cause app activation; locate these symbols: toggleQuickTerminalVisibility,
showQuickTerminal, hideQuickTerminal, and toggleQuickTerminal to make the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32da7582-8e06-454e-a983-b6f00dc0cbc9
📒 Files selected for processing (2)
CLI/cmux.swiftSources/AppDelegate.swift
✅ Files skipped from review due to trivial changes (1)
- CLI/cmux.swift
Author
|
Hi! The PR is ready for review. All checks pass except Vercel (likely permission-related). Thank you! :) |
|
Would be great to see this feature in. @jemutorres could you rebase this change so it applies cleanly? |
Implements Quick Terminal across app shortcut/menu/settings, command palette, socket v2, and CLI (cmux quick-terminal). Also ensures socket/CLI quick terminal commands do not steal macOS focus and wraps animation completion state updates on MainActor. Adds README and CHANGELOG notes. Closes manaflow-ai#327
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
jemutorres
force-pushed
the
feat/quick-terminal-327
branch
from
April 21, 2026 07:50
cd779b1 to
7a9b528
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
2396-2515: Add debug logs for Quick Terminal focus transitions.
show/hidechanges panel visibility, key-window state, and terminal focus, but the new paths do not emitdlog()entries. Add concise request/result logs around queued transitions, activation mode, and completion state.Example instrumentation
func show(activateApp: Bool = true) { +#if DEBUG + dlog("quickTerminal.show request activate=\(activateApp ? 1 : 0) visible=\(isVisible ? 1 : 0) transitioning=\(isTransitioning ? 1 : 0)") +#endif if isTransitioning { pendingTransitionAction = .show(activateApp: activateApp) return } @@ self.isTransitioning = false panel.level = .floating +#if DEBUG + dlog("quickTerminal.show complete activate=\(activateApp ? 1 : 0) visible=\(self.isVisible ? 1 : 0)") +#endif if activateApp { @@ func hide(restorePreviousApp: Bool) { +#if DEBUG + dlog("quickTerminal.hide request restorePreviousApp=\(restorePreviousApp ? 1 : 0) visible=\(isVisible ? 1 : 0) transitioning=\(isTransitioning ? 1 : 0)") +#endif if isTransitioning { pendingTransitionAction = .hide(restorePreviousApp: restorePreviousApp) return } @@ self.isTransitioning = false panel.orderOut(nil) +#if DEBUG + dlog("quickTerminal.hide complete restorePreviousApp=\(restorePreviousApp ? 1 : 0) visible=\(self.isVisible ? 1 : 0)") +#endif self.terminalSurface?.hostedView.setActive(false)As per coding guidelines,
**/*.swift: “All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug log using thedlog()function and wrapped in#if DEBUG/#endifpreprocessor directives”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2396 - 2515, The show/toggle/hide flows lack debug logging; add concise dlog() calls wrapped in `#if` DEBUG / `#endif` in toggle(activateApp:), show(activateApp:), and hide(restorePreviousApp:) to record transition requests (including pendingTransitionAction state), activation mode (activateApp / restorePreviousApp and whether NSApp was active), queued actions when isTransitioning is true, and completion results (visibility state, panel.level, previousFrontmostApp activation result, and terminalSurface focus changes). Place logs at the start of each method (log inputs and early returns), when you set pendingTransitionAction, before/after calling panel.orderFront/orderOut or makeKeyAndOrderFront, and inside both completionHandler blocks to report final state and any activation of previousFrontmostApp; reference symbols: toggle(activateApp:), show(activateApp:), hide(restorePreviousApp:), pendingTransitionAction, previousFrontmostApp, terminalSurface.hostedView, and panel. Ensure all added dlog() calls are inside `#if` DEBUG guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 2848-2853: The quick-terminal subcommand is being dispatched after
the global --window pre-focus block and thus can inadvertently trigger app
focusing; update the dispatch logic so quick-terminal does not run through that
pre-focus path by either moving the case "quick-terminal" branch (the call to
runQuickTerminal) to execute before the global --window pre-focus block or by
short-circuiting the pre-focus logic when the parsed subcommand equals
"quick-terminal"; change the condition around the pre-focus block (or add an
explicit guard) to skip focusing for this subcommand, referencing the case
"quick-terminal" and the runQuickTerminal(...) call to locate and adjust the
code.
In `@Sources/AppDelegate.swift`:
- Around line 11434-11439: The current quick-terminal shortcut check uses
matchShortcut(event: , shortcut:) which bypasses chord handling; replace that
check with the configured-chord-aware path by calling
activeConfiguredShortcutChordPrefixForCurrentEvent(event, shortcut:
KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) (or the app's
equivalent chord-aware matcher) so chorded .toggleQuickTerminal shortcuts can
arm and then execute properly, then call toggleQuickTerminalVisibility() and
return true when the chord-aware matcher indicates a full match.
In `@Sources/cmuxApp.swift`:
- Around line 4629-4631: Replace the hard-coded percent label in
quickTerminalRatioPercentLabel by using a localized formatted string key (e.g.
"settings.quickTerminal.ratioPercent") and feed the computed
Int((QuickTerminalSettings.clampRatio(ratio) * 100).rounded()) as the format
argument via String(localized:..., ...) so the percent symbol and number are
localizable; also add the "settings.quickTerminal.ratioPercent" entry to
Resources/Localizable.xcstrings for all supported languages with an appropriate
format placeholder (e.g. "%d%%") and translated variants.
- Around line 4650-4694: The sliders and toggle lack accessible labels/values;
update the controls in SettingsCardRow so each Slider (bound via
quickTerminalPrimarySizeBinding and quickTerminalSecondarySizeBinding) has an
accessibilityLabel using the same localized title (e.g.,
"settings.quickTerminal.primarySize" / "settings.quickTerminal.secondarySize")
and an accessibilityValue set to quickTerminalRatioPercentLabel(…) so screen
readers read the percent, and update the Toggle (bound to quickTerminalAutoHide)
to remove the empty-string label and instead set an accessibilityLabel using the
localized "settings.quickTerminal.autoHide" title and an accessibilityHint or
accessibilityValue reflecting the on/off subtitle strings so assistive tech gets
explicit labels and state.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 2396-2515: The show/toggle/hide flows lack debug logging; add
concise dlog() calls wrapped in `#if` DEBUG / `#endif` in toggle(activateApp:),
show(activateApp:), and hide(restorePreviousApp:) to record transition requests
(including pendingTransitionAction state), activation mode (activateApp /
restorePreviousApp and whether NSApp was active), queued actions when
isTransitioning is true, and completion results (visibility state, panel.level,
previousFrontmostApp activation result, and terminalSurface focus changes).
Place logs at the start of each method (log inputs and early returns), when you
set pendingTransitionAction, before/after calling panel.orderFront/orderOut or
makeKeyAndOrderFront, and inside both completionHandler blocks to report final
state and any activation of previousFrontmostApp; reference symbols:
toggle(activateApp:), show(activateApp:), hide(restorePreviousApp:),
pendingTransitionAction, previousFrontmostApp, terminalSurface.hostedView, and
panel. Ensure all added dlog() calls are inside `#if` DEBUG guards.
🪄 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: 577a641d-c1ce-4ffb-9063-7a175f5a5fde
📒 Files selected for processing (10)
CHANGELOG.mdCLI/cmux.swiftREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (5)
- README.md
- cmuxTests/WorkspaceUnitTests.swift
- CHANGELOG.md
- Resources/Localizable.xcstrings
- Sources/TerminalController.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/KeyboardShortcutSettings.swift
Comment on lines
+2848
to
+2853
| case "quick-terminal": | ||
| try runQuickTerminal( | ||
| commandArgs: commandArgs, | ||
| client: client, | ||
| jsonOutput: jsonOutput | ||
| ) |
There was a problem hiding this comment.
Bypass global --window focusing for Quick Terminal.
quick-terminal is dispatched after the global --window pre-focus block, so cmux --window ... quick-terminal status|show|hide|toggle can focus/raise cmux before running this non-focus automation command. Route it before that block, or explicitly skip pre-focus for this command.
Proposed fix
try authenticateClientIfNeeded(
client,
explicitPassword: socketPasswordArg,
socketPath: resolvedSocketPath
)
+ if command == "quick-terminal" {
+ try runQuickTerminal(
+ commandArgs: commandArgs,
+ client: client,
+ jsonOutput: jsonOutput
+ )
+ return
+ }
+
let idFormat = try resolvedIDFormat(jsonOutput: jsonOutput, raw: idFormatArg)
// If the user explicitly targets a window, focus it first so commands route correctly.
if let windowId {
let normalizedWindow = try normalizeWindowHandle(windowId, client: client) ?? windowId
_ = try client.sendV2(method: "window.focus", params: ["window_id": normalizedWindow])
}- case "quick-terminal":
- try runQuickTerminal(
- commandArgs: commandArgs,
- client: client,
- jsonOutput: jsonOutput
- )
-As per coding guidelines, Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "quick-terminal": | |
| try runQuickTerminal( | |
| commandArgs: commandArgs, | |
| client: client, | |
| jsonOutput: jsonOutput | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 2848 - 2853, The quick-terminal subcommand is
being dispatched after the global --window pre-focus block and thus can
inadvertently trigger app focusing; update the dispatch logic so quick-terminal
does not run through that pre-focus path by either moving the case
"quick-terminal" branch (the call to runQuickTerminal) to execute before the
global --window pre-focus block or by short-circuiting the pre-focus logic when
the parsed subcommand equals "quick-terminal"; change the condition around the
pre-focus block (or add an explicit guard) to skip focusing for this subcommand,
referencing the case "quick-terminal" and the runQuickTerminal(...) call to
locate and adjust the code.
Comment on lines
+11434
to
+11439
| // Allow quick-terminal toggle even when the key event comes from the quick | ||
| // panel itself (which is not a main terminal window context). | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) { | ||
| _ = toggleQuickTerminalVisibility() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Use configured-shortcut matching so Quick Terminal chords work.
This raw matchShortcut path bypasses activeConfiguredShortcutChordPrefixForCurrentEvent, so a chorded .toggleQuickTerminal shortcut can arm its first stroke later but never execute on the second stroke.
Proposed fix
- if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) {
+ if matchConfiguredShortcut(event: event, action: .toggleQuickTerminal) {
_ = toggleQuickTerminalVisibility()
return true
}
+
+ if activeConfiguredShortcutChordPrefixForCurrentEvent == nil,
+ armConfiguredShortcutChordIfNeeded(event: event, actions: [.toggleQuickTerminal]) {
+ return true
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 11434 - 11439, The current
quick-terminal shortcut check uses matchShortcut(event: , shortcut:) which
bypasses chord handling; replace that check with the configured-chord-aware path
by calling activeConfiguredShortcutChordPrefixForCurrentEvent(event, shortcut:
KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) (or the app's
equivalent chord-aware matcher) so chorded .toggleQuickTerminal shortcuts can
arm and then execute properly, then call toggleQuickTerminalVisibility() and
return true when the chord-aware matcher indicates a full match.
Comment on lines
+4629
to
+4631
| private func quickTerminalRatioPercentLabel(_ ratio: Double) -> String { | ||
| "\(Int((QuickTerminalSettings.clampRatio(ratio) * 100).rounded()))%" | ||
| } |
There was a problem hiding this comment.
Localize the percentage label.
Line 4630 creates user-facing Settings text without a localization key, so translators cannot adapt the percent label.
🌐 Proposed fix
private func quickTerminalRatioPercentLabel(_ ratio: Double) -> String {
- "\(Int((QuickTerminalSettings.clampRatio(ratio) * 100).rounded()))%"
+ let percent = Int((QuickTerminalSettings.clampRatio(ratio) * 100).rounded())
+ return String(
+ localized: "settings.quickTerminal.ratioPercent",
+ defaultValue: "\(percent)%"
+ )
}Also add settings.quickTerminal.ratioPercent to Resources/Localizable.xcstrings for all supported languages.
As per coding guidelines, All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") and added to Resources/Localizable.xcstrings with translations for all supported languages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 4629 - 4631, Replace the hard-coded
percent label in quickTerminalRatioPercentLabel by using a localized formatted
string key (e.g. "settings.quickTerminal.ratioPercent") and feed the computed
Int((QuickTerminalSettings.clampRatio(ratio) * 100).rounded()) as the format
argument via String(localized:..., ...) so the percent symbol and number are
localizable; also add the "settings.quickTerminal.ratioPercent" entry to
Resources/Localizable.xcstrings for all supported languages with an appropriate
format placeholder (e.g. "%d%%") and translated variants.
Comment on lines
+4650
to
+4694
| SettingsCardRow( | ||
| configurationReview: .settingsOnly, | ||
| String(localized: "settings.quickTerminal.primarySize", defaultValue: "Quick Terminal Primary Size"), | ||
| subtitle: String(localized: "settings.quickTerminal.primarySize.subtitle", defaultValue: "Size along the deployment axis.") | ||
| ) { | ||
| HStack(spacing: 8) { | ||
| Slider(value: quickTerminalPrimarySizeBinding, in: 0.2...1.0) | ||
| .frame(width: 132) | ||
| Text(quickTerminalRatioPercentLabel(quickTerminalPrimarySizeRatio)) | ||
| .font(.system(size: 11, weight: .medium)) | ||
| .foregroundStyle(.secondary) | ||
| .frame(width: 42, alignment: .trailing) | ||
| } | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow( | ||
| configurationReview: .settingsOnly, | ||
| String(localized: "settings.quickTerminal.secondarySize", defaultValue: "Quick Terminal Secondary Size"), | ||
| subtitle: String(localized: "settings.quickTerminal.secondarySize.subtitle", defaultValue: "Size on the opposite axis.") | ||
| ) { | ||
| HStack(spacing: 8) { | ||
| Slider(value: quickTerminalSecondarySizeBinding, in: 0.2...1.0) | ||
| .frame(width: 132) | ||
| Text(quickTerminalRatioPercentLabel(quickTerminalSecondarySizeRatio)) | ||
| .font(.system(size: 11, weight: .medium)) | ||
| .foregroundStyle(.secondary) | ||
| .frame(width: 42, alignment: .trailing) | ||
| } | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow( | ||
| configurationReview: .settingsOnly, | ||
| String(localized: "settings.quickTerminal.autoHide", defaultValue: "Quick Terminal Auto-Hide"), | ||
| subtitle: quickTerminalAutoHide | ||
| ? String(localized: "settings.quickTerminal.autoHide.subtitleOn", defaultValue: "Hide automatically when it loses focus.") | ||
| : String(localized: "settings.quickTerminal.autoHide.subtitleOff", defaultValue: "Keep visible when focus moves to another app/window.") | ||
| ) { | ||
| Toggle("", isOn: $quickTerminalAutoHide) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| } |
There was a problem hiding this comment.
Add accessible labels to the hidden Quick Terminal controls.
The two sliders and the empty-label toggle rely on nearby visual text, but that is not reliably associated for assistive tech.
♿ Proposed fix
HStack(spacing: 8) {
Slider(value: quickTerminalPrimarySizeBinding, in: 0.2...1.0)
.frame(width: 132)
+ .accessibilityLabel(
+ String(localized: "settings.quickTerminal.primarySize", defaultValue: "Quick Terminal Primary Size")
+ )
+ .accessibilityValue(quickTerminalRatioPercentLabel(quickTerminalPrimarySizeRatio))
Text(quickTerminalRatioPercentLabel(quickTerminalPrimarySizeRatio))
.font(.system(size: 11, weight: .medium))
.foregroundStyle(.secondary)
@@
HStack(spacing: 8) {
Slider(value: quickTerminalSecondarySizeBinding, in: 0.2...1.0)
.frame(width: 132)
+ .accessibilityLabel(
+ String(localized: "settings.quickTerminal.secondarySize", defaultValue: "Quick Terminal Secondary Size")
+ )
+ .accessibilityValue(quickTerminalRatioPercentLabel(quickTerminalSecondarySizeRatio))
Text(quickTerminalRatioPercentLabel(quickTerminalSecondarySizeRatio))
.font(.system(size: 11, weight: .medium))
.foregroundStyle(.secondary)
@@
Toggle("", isOn: $quickTerminalAutoHide)
.labelsHidden()
.controlSize(.small)
+ .accessibilityLabel(
+ String(localized: "settings.quickTerminal.autoHide", defaultValue: "Quick Terminal Auto-Hide")
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 4650 - 4694, The sliders and toggle lack
accessible labels/values; update the controls in SettingsCardRow so each Slider
(bound via quickTerminalPrimarySizeBinding and
quickTerminalSecondarySizeBinding) has an accessibilityLabel using the same
localized title (e.g., "settings.quickTerminal.primarySize" /
"settings.quickTerminal.secondarySize") and an accessibilityValue set to
quickTerminalRatioPercentLabel(…) so screen readers read the percent, and update
the Toggle (bound to quickTerminalAutoHide) to remove the empty-string label and
instead set an accessibilityLabel using the localized
"settings.quickTerminal.autoHide" title and an accessibilityHint or
accessibilityValue reflecting the on/off subtitle strings so assistive tech gets
explicit labels and state.
Collaborator
|
cmux-reconcile: partly-useful Usefulness verdict: Treat as an alternative Quick Terminal implementation; carry unique behavior into #4830 before retiring this branch. This diff integrates a quick terminal with shortcut/menu/settings, CLI/socket controls, and non-focus-stealing automation. #4830 addresses the same capability with broader package-based settings and tests. The requirement remains useful (#327), so closing both as stale would lose active feature work. Compare focus, auto-hide, position/sizing and state ownership; consolidate one implementation. No claim that either is merge-ready. Reviewed patch head: Older issue/PR tracking index — remaining scope and competing implementations are recorded there. |
Closed
Collaborator
|
This branch is superseded by the consolidation work in #4830, as noted in the maintainer reconciliation. The Quick Terminal requirement remains tracked there. |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Quick Terminal across app shortcut/menu/settings, command palette, socket v2, and CLI (cmux quick-terminal).
Also ensures socket/CLI quick terminal commands do not steal macOS focus and wraps animation completion state updates on MainActor.
Adds README and CHANGELOG notes.
Closes #327
Summary
Why
Testing
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Adds a drop-down Quick Terminal panel with keyboard/menu/palette controls and scriptable CLI/socket APIs. Automation-safe: CLI/socket calls don’t steal macOS focus; supports configurable position, size, auto-hide, and restores the previous app when hiding.
New Features
cmux quick-terminal [toggle|show|hide|status]andcmux --json quick-terminal status(prints availability, visibility, position, auto-hide, and size ratios).quick_terminal.toggle,quick_terminal.show,quick_terminal.hide,quick_terminal.status.Bug Fixes
Written for commit 7a9b528. Summary will update on new commits.
Summary by CodeRabbit