feat: add Quick Terminal with global hotkey and split support - #1712
Open
songhanlin wants to merge 1 commit into
Open
songhanlin wants to merge 1 commit into
songhanlin wants to merge 1 commit into
Conversation
|
@songhanlin is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Quick Terminal subsystem (Quake-style drop-down) with window, positioning, global hotkey, controller, SwiftUI content, AppDelegate integration for shortcut routing, a focusable terminal view, localization, and Xcode project entries for the new source files. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant HotKey as QuickTerminalHotKey
participant Controller as QuickTerminalController
participant Window as QuickTerminalWindow
participant TabMgr as TabManager
participant Surface as TerminalSurface
User->>HotKey: press global hotkey
HotKey->>Controller: invoke action
Controller->>Controller: toggle()
alt hidden -> show
Controller->>Window: setInitial (off-screen) / animateIn
Window->>Window: setFinal (on-screen)
Controller->>Surface: makeFirstResponder / focus
Surface-->>User: ready for input
else visible -> hide
Controller->>Window: animateOut -> setInitial (off-screen)
Controller->>User: restore previous app focus
end
User->>Controller: keyboard shortcut (while visible)
Controller->>TabMgr: route shortcut (split/focus/close)
TabMgr->>Surface: update panes/render
Surface-->>User: updated terminal UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
Sources/AppDelegate.swift (1)
8445-8446: Prefer visibility-gated routing for Quick Terminal shortcut scope.Using only
QuickTerminalWindowtype checks can transiently capture shortcuts during panel animation/focus transitions. Gate with the controller’svisiblestate too.♻️ Suggested hardening
- if event.window is QuickTerminalWindow || NSApp.keyWindow is QuickTerminalWindow { + if quickTerminalController?.visible == true, + (event.window is QuickTerminalWindow || NSApp.keyWindow is QuickTerminalWindow) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8445 - 8446, The shortcut handling currently checks only the window type (QuickTerminalWindow / NSApp.keyWindow) which can transiently match during animations; update the conditional to also verify the quick terminal controller is visible before routing the toggle. Specifically, when checking event.window is QuickTerminalWindow or NSApp.keyWindow is QuickTerminalWindow, also verify the associated controller’s visible flag (e.g., window.controller?.visible == true or QuickTerminalController.shared.visible) before calling matchShortcut(event: shortcut:) with KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal), so the shortcut only routes while the Quick Terminal is actually visible.Sources/QuickTerminal/QuickTerminalHotKey.swift (1)
75-129: Consider expanding key mapping for common modifier keys.The current mapping covers letters, digits, and punctuation but omits function keys (F1–F12), Escape, arrow keys, and Delete. While the default Cmd+
`works, users who want to remap may hit unsupported keys.💡 Additional key mappings to consider
case "\t": return UInt32(kVK_Tab) + case "escape": return UInt32(kVK_Escape) + case "delete": return UInt32(kVK_Delete) + case "f1": return UInt32(kVK_F1) + case "f2": return UInt32(kVK_F2) + case "f3": return UInt32(kVK_F3) + case "f4": return UInt32(kVK_F4) + case "f5": return UInt32(kVK_F5) + case "f6": return UInt32(kVK_F6) + case "f7": return UInt32(kVK_F7) + case "f8": return UInt32(kVK_F8) + case "f9": return UInt32(kVK_F9) + case "f10": return UInt32(kVK_F10) + case "f11": return UInt32(kVK_F11) + case "f12": return UInt32(kVK_F12) default: return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminal/QuickTerminalHotKey.swift` around lines 75 - 129, The carbonKeyCode(for:) mapper omits common non-character keys (Escape, Delete, arrow keys, F1–F12, Home/End, PageUp/PageDown, etc.), so extend carbonKeyCode(for key: String) to handle those by adding cases (or switch-to-dictionary) that map the string tokens you accept (e.g. "escape", "esc", "delete", "left", "right", "up", "down", "f1"..."f12", "home", "end", "pageup", "pagedown") to the corresponding kVK_* constants (returned as UInt32), or centralize the mapping in a static [String: UInt32] lookup and return lookup[key.lowercased()]. Ensure you reference the same function carbonKeyCode(for:) and use the macOS kVK_* constants (e.g. kVK_Escape, kVK_Delete, kVK_LeftArrow, kVK_F1, kVK_PageUp) for consistency.
🤖 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 8494-8497: The current handler in AppDelegate.swift
unconditionally returns true when any Command key is present (using
event.modifierFlags.intersection(.deviceIndependentFlagsMask) and
flags.contains(.command)), which swallows all app‑wide Command shortcuts; change
the logic so Quick Terminal only consumes the specific Command key combos it
cares about: inspect the event’s keyCode/charactersIgnoringModifiers (and
modifierFlags) and return true only for the explicit shortcuts used by Quick
Terminal (e.g., Cmd+Return or the specific character/keyCodes), otherwise return
false so global Command shortcuts (Quit/Hide/Preferences) are preserved.
In `@Sources/QuickTerminal/QuickTerminalController.swift`:
- Around line 148-154: The enqueuePanelTitleUpdate method is queuing updates for
panels from any window because it lacks the TabManager ownership guard; update
enqueuePanelTitleUpdate(tabId: UUID, panelId: UUID, title: String) to early-exit
if the tabId does not belong to this TabManager by adding the same check used in
markPanelReadOnFocusIfActive (guard selectedTabId == tabId else { return })
before mutating pendingPanelTitleUpdates so only updates for the currently
selected tab are enqueued.
In `@Sources/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 7-14: QuickTerminalHotKey can be deallocated while an event
handler remains installed causing a dangling userData pointer; add a deinit to
call the existing unregister() logic (or inline the same cleanup) to remove the
EventHandlerRef and EventHotKeyRef and nil out hotKeyRef and handlerRef so
passUnretained's pointer is never left pointing at freed memory; reference the
QuickTerminalHotKey class, its unregister() behavior, and the
hotKeyRef/handlerRef fields when implementing the deinit.
- Around line 57-58: InstallEventHandler and RegisterEventHotKey return OSStatus
which must be checked: capture both return values, verify RegisterEventHotKey
succeeded (OSStatus == noErr) before keeping hotKeyRef and handlerRef, and if
RegisterEventHotKey fails, call RemoveEventHandler (or the appropriate teardown
used by unregister()) to remove the installed handler and log/propagate the
error; also ensure unregister() only tries to remove hotKeyRef/handlerRef if
they were successfully created to avoid leaving orphaned handlers. Use the
symbols InstallEventHandler, RegisterEventHotKey, hotKeyRef, handlerRef, and
unregister() when locating and updating the code.
- Around line 16-20: The register(shortcut: StoredShortcut) currently returns
void and silently aborts when carbonKeyCode(for:) yields nil; change
register(shortcut:) to return Bool (or make it throwing) and when
carbonKeyCode(for:) is nil or shortcut.key.isEmpty use the return/throw to
indicate failure; then update QuickTerminalController.registerGlobalHotKey() to
check the Bool/handle the thrown error and surface a user-facing warning/log
explaining the key is unsupported (reference carbonKeyCode(for:),
register(shortcut:), and QuickTerminalController.registerGlobalHotKey()).
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 8445-8446: The shortcut handling currently checks only the window
type (QuickTerminalWindow / NSApp.keyWindow) which can transiently match during
animations; update the conditional to also verify the quick terminal controller
is visible before routing the toggle. Specifically, when checking event.window
is QuickTerminalWindow or NSApp.keyWindow is QuickTerminalWindow, also verify
the associated controller’s visible flag (e.g., window.controller?.visible ==
true or QuickTerminalController.shared.visible) before calling
matchShortcut(event: shortcut:) with KeyboardShortcutSettings.shortcut(for:
.toggleQuickTerminal), so the shortcut only routes while the Quick Terminal is
actually visible.
In `@Sources/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 75-129: The carbonKeyCode(for:) mapper omits common non-character
keys (Escape, Delete, arrow keys, F1–F12, Home/End, PageUp/PageDown, etc.), so
extend carbonKeyCode(for key: String) to handle those by adding cases (or
switch-to-dictionary) that map the string tokens you accept (e.g. "escape",
"esc", "delete", "left", "right", "up", "down", "f1"..."f12", "home", "end",
"pageup", "pagedown") to the corresponding kVK_* constants (returned as UInt32),
or centralize the mapping in a static [String: UInt32] lookup and return
lookup[key.lowercased()]. Ensure you reference the same function
carbonKeyCode(for:) and use the macOS kVK_* constants (e.g. kVK_Escape,
kVK_Delete, kVK_LeftArrow, kVK_F1, kVK_PageUp) for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15c7320e-03dc-4921-b3b5-e81dc9e2d211
📒 Files selected for processing (10)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettings.swiftSources/QuickTerminal/QuickTerminalContentView.swiftSources/QuickTerminal/QuickTerminalController.swiftSources/QuickTerminal/QuickTerminalHotKey.swiftSources/QuickTerminal/QuickTerminalPosition.swiftSources/QuickTerminal/QuickTerminalWindow.swift
![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 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/QuickTerminal/QuickTerminalHotKey.swift">
<violation number="1" location="Sources/QuickTerminal/QuickTerminalHotKey.swift:57">
P2: Global hotkey registration failures are silent because unsupported keys and Carbon API OSStatus errors are ignored, leaving the feature disabled without diagnostics or recovery.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:8495">
P1: Don't consume every unhandled Command shortcut while the Quick Terminal is focused; this blocks app-wide menu commands. Let unhandled Cmd chords fall through so AppKit can still process standard actions like Quit/Hide/Preferences.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
songhanlin
force-pushed
the
feat/quick-terminal
branch
from
March 18, 2026 08:16
e396d6b to
f52aaa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/QuickTerminal/QuickTerminalHotKey.swift (1)
29-30: Consider adding debug logging for registration failures.Silent
return falseon failure paths makes it difficult to diagnose why the hotkey didn't register (unsupported key, handler install failure, or key registration failure).💡 Suggested debug logging
guard let carbonKeyCode = carbonKeyCode(for: shortcut.key), - !shortcut.key.isEmpty else { return false } + !shortcut.key.isEmpty else { + `#if` DEBUG + dlog("QuickTerminalHotKey: unsupported key '\(shortcut.key)'") + `#endif` + return false + }guard handlerStatus == noErr, let installedHandler else { + `#if` DEBUG + dlog("QuickTerminalHotKey: InstallEventHandler failed (\(handlerStatus))") + `#endif` return false }guard keyStatus == noErr, let registeredKey else { + `#if` DEBUG + dlog("QuickTerminalHotKey: RegisterEventHotKey failed (\(keyStatus))") + `#endif` RemoveEventHandler(installedHandler) handlerRef = nil return false }As per coding guidelines: "All debug events... Use free function dlog()... Wrap all call sites in
#ifDEBUG /#endif."Also applies to: 71-72, 80-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminal/QuickTerminalHotKey.swift` around lines 29 - 30, Add debug logging for each hotkey registration failure path: when carbonKeyCode(for:) returns nil or shortcut.key.isEmpty, when the hotkey event handler installation (InstallEventHandler / hotKey handler setup) fails, and when RegisterEventHotKey returns an error. Use the free function dlog() and wrap each call site in `#if` DEBUG / `#endif`; include the shortcut description and the underlying error/return code in the message so you can tell whether the key was unsupported, the handler failed, or the registration call failed. Ensure you add these logs adjacent to the existing guard/early-return points (the carbonKeyCode(for:) check and the handler/register failure branches) so failures are no longer silent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 61962-61996: The shortcut.toggleQuickTerminal.label entry is
missing translations for several supported locales; update the "localizations"
object for shortcut.toggleQuickTerminal.label to include the same set of
language keys used by shortcut.equalizeSplits.label (add de, es, fr, it, da, pl,
ru, bs, ar, nb, pt-BR, th, tr) and supply appropriate translated "value" strings
(or copy the corresponding translations from shortcut.equalizeSplits.label as
placeholders) while keeping "stringUnit.state": "translated" for each new locale
so clients in those locales won't fallback to English.
In `@Sources/AppDelegate.swift`:
- Around line 8444-8497: The Quick Terminal branch currently routes many
shortcuts to the quickTerminalController.tabManager but omits handling for the
equalizeSplits shortcut, so that shortcut falls through and isn’t applied to the
QT TabManager; inside the same conditional where qtTabManager is unwrapped
(quickTerminalController?.tabManager), add a matchShortcut check for
KeyboardShortcutSettings.shortcut(for: .equalizeSplits) (or StoredShortcut
equivalent) and call qtTabManager.equalizeSplits() (or the appropriate method
name) and return true so the equalizeSplits shortcut is consumed by the Quick
Terminal instead of falling through to the outer return false.
---
Nitpick comments:
In `@Sources/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 29-30: Add debug logging for each hotkey registration failure
path: when carbonKeyCode(for:) returns nil or shortcut.key.isEmpty, when the
hotkey event handler installation (InstallEventHandler / hotKey handler setup)
fails, and when RegisterEventHotKey returns an error. Use the free function
dlog() and wrap each call site in `#if` DEBUG / `#endif`; include the shortcut
description and the underlying error/return code in the message so you can tell
whether the key was unsupported, the handler failed, or the registration call
failed. Ensure you add these logs adjacent to the existing guard/early-return
points (the carbonKeyCode(for:) check and the handler/register failure branches)
so failures are no longer silent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2771d3fa-a622-4680-89b3-115cd2face2d
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/QuickTerminal/QuickTerminalHotKey.swiftSources/TabManager.swift
Comment on lines
+8444
to
+8497
| // When the Quick Terminal is active, route shortcuts to its own TabManager. | ||
| if quickTerminalController?.visible == true && | ||
| (event.window is QuickTerminalWindow || NSApp.keyWindow is QuickTerminalWindow) { | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal)) { | ||
| quickTerminalController?.toggle() | ||
| return true | ||
| } | ||
| // Route split/tab shortcuts to the quick terminal's TabManager. | ||
| if let qtTabManager = quickTerminalController?.tabManager { | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitRight)) { | ||
| qtTabManager.createSplit(direction: .right) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitDown)) { | ||
| qtTabManager.createSplit(direction: .down) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusLeft)) { | ||
| qtTabManager.movePaneFocus(direction: .left) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusRight)) { | ||
| qtTabManager.movePaneFocus(direction: .right) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusUp)) { | ||
| qtTabManager.movePaneFocus(direction: .up) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusDown)) { | ||
| qtTabManager.movePaneFocus(direction: .down) | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleSplitZoom)) { | ||
| _ = qtTabManager.toggleFocusedSplitZoom() | ||
| return true | ||
| } | ||
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .newSurface)) { | ||
| qtTabManager.newSurface() | ||
| return true | ||
| } | ||
| // Cmd+W: close the focused pane in the quick terminal. | ||
| if matchShortcut(event: event, shortcut: StoredShortcut(key: "w", command: true, shift: false, option: false, control: false)) { | ||
| if let workspace = qtTabManager.selectedWorkspace, | ||
| let panelId = workspace.focusedPanelId { | ||
| qtTabManager.closePanelWithConfirmation(tabId: workspace.id, surfaceId: panelId) | ||
| } | ||
| return true | ||
| } | ||
| } | ||
| // Don't consume unhandled Cmd shortcuts — let system commands | ||
| // (Cmd+Q, Cmd+H, Cmd+M, etc.) pass through to the main menu. | ||
| return false | ||
| } |
There was a problem hiding this comment.
Route equalizeSplits inside the Quick Terminal branch to prevent scope leakage.
When this branch is active, .equalizeSplits is not handled and falls through to return false on Line 8496, so the shortcut won’t be applied to the Quick Terminal TabManager.
💡 Suggested fix
if let qtTabManager = quickTerminalController?.tabManager {
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitRight)) {
qtTabManager.createSplit(direction: .right)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitDown)) {
qtTabManager.createSplit(direction: .down)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusLeft)) {
qtTabManager.movePaneFocus(direction: .left)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusRight)) {
qtTabManager.movePaneFocus(direction: .right)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusUp)) {
qtTabManager.movePaneFocus(direction: .up)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusDown)) {
qtTabManager.movePaneFocus(direction: .down)
return true
}
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleSplitZoom)) {
_ = qtTabManager.toggleFocusedSplitZoom()
return true
}
+ if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .equalizeSplits)) {
+ if let workspace = qtTabManager.selectedWorkspace {
+ _ = qtTabManager.equalizeSplits(tabId: workspace.id)
+ }
+ return true
+ }
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .newSurface)) {
qtTabManager.newSurface()
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 8444 - 8497, The Quick Terminal
branch currently routes many shortcuts to the quickTerminalController.tabManager
but omits handling for the equalizeSplits shortcut, so that shortcut falls
through and isn’t applied to the QT TabManager; inside the same conditional
where qtTabManager is unwrapped (quickTerminalController?.tabManager), add a
matchShortcut check for KeyboardShortcutSettings.shortcut(for: .equalizeSplits)
(or StoredShortcut equivalent) and call qtTabManager.equalizeSplits() (or the
appropriate method name) and return true so the equalizeSplits shortcut is
consumed by the Quick Terminal instead of falling through to the outer return
false.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
8452-8496:⚠️ Potential issue | 🟠 MajorRoute
equalizeSplitsin the Quick Terminal shortcut branch.Line 8496 exits this branch for unhandled shortcuts, but
.equalizeSplitsis not handled in the Quick Terminal path. That means equalize can miss the Quick TerminalTabManagerwhen Quick Terminal is active.💡 Suggested fix
if let qtTabManager = quickTerminalController?.tabManager { if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitRight)) { qtTabManager.createSplit(direction: .right) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .splitDown)) { qtTabManager.createSplit(direction: .down) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusLeft)) { qtTabManager.movePaneFocus(direction: .left) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusRight)) { qtTabManager.movePaneFocus(direction: .right) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusUp)) { qtTabManager.movePaneFocus(direction: .up) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .focusDown)) { qtTabManager.movePaneFocus(direction: .down) return true } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleSplitZoom)) { _ = qtTabManager.toggleFocusedSplitZoom() return true } + if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .equalizeSplits)) { + if let workspace = qtTabManager.selectedWorkspace { + _ = qtTabManager.equalizeSplits(tabId: workspace.id) + } + return true + } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .newSurface)) { qtTabManager.newSurface() 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 8452 - 8496, The Quick Terminal branch misses handling the .equalizeSplits shortcut, so add an if block like the other shortcuts that checks matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .equalizeSplits)) and, when matched, call the Quick Terminal tab manager’s equalize method (e.g. qtTabManager.equalizeSplits() or the exact method name on TabManager that equalizes panes) then return true; place this alongside the other qtTabManager shortcut handlers (createSplit, movePaneFocus, toggleFocusedSplitZoom, newSurface, etc.) so equalizeSplits gets routed to the Quick Terminal TabManager.
🧹 Nitpick comments (1)
Sources/QuickTerminal/QuickTerminalHotKey.swift (1)
71-83: Consider adding debug logging for registration failures.Per coding guidelines, debug events should be logged using
dlog(). Adding logging here would help diagnose why hotkey registration failed (e.g., key already claimed by another app, unsupported key).💡 Proposed enhancement
guard handlerStatus == noErr, let installedHandler else { + `#if` DEBUG + dlog("QuickTerminalHotKey: InstallEventHandler failed with status \(handlerStatus)") + `#endif` return false } handlerRef = installedHandler var registeredKey: EventHotKeyRef? let keyStatus = RegisterEventHotKey( carbonKeyCode, modifiers, hotKeyID, GetApplicationEventTarget(), 0, ®isteredKey ) guard keyStatus == noErr, let registeredKey else { + `#if` DEBUG + dlog("QuickTerminalHotKey: RegisterEventHotKey failed with status \(keyStatus)") + `#endif` RemoveEventHandler(installedHandler) handlerRef = nil return false }As per coding guidelines: "All debug events... in DEBUG builds go to the debug event log. Use free function
dlog("message")to log events. Wrap all call sites in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminal/QuickTerminalHotKey.swift` around lines 71 - 83, Add DEBUG-only dlog() statements around the hotkey installation/registration failure branches: when checking handlerStatus/installedHandler after InstallEventHandler (referencing handlerStatus and installedHandler/handlerRef) and when checking keyStatus/registeredKey after RegisterEventHotKey (referencing keyStatus, registeredKey and the RegisterEventHotKey call). Wrap each dlog(...) in `#if` DEBUG / `#endif` and include contextual messages with the status/error (e.g., handlerStatus or keyStatus) so failures are logged before calling RemoveEventHandler or returning false.
🤖 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 8452-8496: The Quick Terminal branch misses handling the
.equalizeSplits shortcut, so add an if block like the other shortcuts that
checks matchShortcut(event: event, shortcut:
KeyboardShortcutSettings.shortcut(for: .equalizeSplits)) and, when matched, call
the Quick Terminal tab manager’s equalize method (e.g.
qtTabManager.equalizeSplits() or the exact method name on TabManager that
equalizes panes) then return true; place this alongside the other qtTabManager
shortcut handlers (createSplit, movePaneFocus, toggleFocusedSplitZoom,
newSurface, etc.) so equalizeSplits gets routed to the Quick Terminal
TabManager.
---
Nitpick comments:
In `@Sources/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 71-83: Add DEBUG-only dlog() statements around the hotkey
installation/registration failure branches: when checking
handlerStatus/installedHandler after InstallEventHandler (referencing
handlerStatus and installedHandler/handlerRef) and when checking
keyStatus/registeredKey after RegisterEventHotKey (referencing keyStatus,
registeredKey and the RegisterEventHotKey call). Wrap each dlog(...) in `#if`
DEBUG / `#endif` and include contextual messages with the status/error (e.g.,
handlerStatus or keyStatus) so failures are logged before calling
RemoveEventHandler or returning false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5738cf57-ce91-4eb2-8934-95bbce21bac6
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/QuickTerminal/QuickTerminalHotKey.swift
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/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 53-70: The handler currently extracts EventHotKeyID but never
checks it; modify the handler closure (the EventHandlerUPP block that calls
GetEventParameter and then DispatchQueue.main.async { this.action() }) to
validate the retrieved hotKeyID.signature and hotKeyID.id against the expected
values before invoking this.action(); compare against stored/known values on the
QuickTerminalHotKey instance (e.g., a property like
expectedHotKeySignature/expectedHotKeyID or the values used when registering the
hotkey) and only call this.action() when they match, otherwise return a nonErr
or eventNotHandledErr as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a4dc631-44c8-436e-b8d2-07f869f339ef
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/QuickTerminal/QuickTerminalHotKey.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/QuickTerminal/QuickTerminalController.swift (1)
36-43:⚠️ Potential issue | 🟡 MinorHandle
register(shortcut:)failure instead of ignoring it.
QuickTerminalHotKey.register(shortcut:)returnsBool, but failure is currently dropped. Unsupported/conflicting shortcuts can fail silently.🔧 Proposed fix
- func registerGlobalHotKey() { + `@discardableResult` + func registerGlobalHotKey() -> Bool { let shortcut = KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal) let hotKey = QuickTerminalHotKey { [weak self] in self?.toggle() } - hotKey.register(shortcut: shortcut) - globalHotKey = hotKey + let didRegister = hotKey.register(shortcut: shortcut) + globalHotKey = didRegister ? hotKey : nil +#if DEBUG + if !didRegister { + dlog("quickTerminal.hotKey.controller register failed for key=\"\(shortcut.key)\"") + } +#endif + return didRegister }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminal/QuickTerminalController.swift` around lines 36 - 43, In registerGlobalHotKey(), don't ignore the Bool returned by QuickTerminalHotKey.register(shortcut:); check its result and handle failure: obtain the shortcut via KeyboardShortcutSettings.shortcut(for: .toggleQuickTerminal), call hotKey.register(...), and if it returns false then avoid overwriting globalHotKey and surface an error (e.g., log via a logger, post a user notification, or present an alert) so the unsupported/conflicting shortcut is visible; keep the existing hotKey/toggle closure and only assign globalHotKey = hotKey on success.
🧹 Nitpick comments (1)
Sources/QuickTerminal/QuickTerminalHotKey.swift (1)
44-45: Extract hotkey signature/ID into shared constants to avoid drift.The registration path and callback validation currently duplicate the same identity literals. Centralizing them in one place avoids accidental mismatch.
♻️ Proposed refactor
`@MainActor` final class QuickTerminalHotKey { + private static let hotKeySignature = OSType(0x434D5558) // "CMUX" + private static let hotKeyEventID: UInt32 = 1 + private var hotKeyRef: EventHotKeyRef? private var handlerRef: EventHandlerRef? private let action: `@MainActor` () -> Void @@ - let hotKeyID = EventHotKeyID(signature: OSType(0x434D5558), // "CMUX" - id: 1) + let hotKeyID = EventHotKeyID( + signature: Self.hotKeySignature, + id: Self.hotKeyEventID + ) @@ - guard hotKeyID.signature == OSType(0x434D5558), // "CMUX" - hotKeyID.id == 1 else { + guard hotKeyID.signature == Self.hotKeySignature, + hotKeyID.id == Self.hotKeyEventID else { return OSStatus(eventNotHandledErr) }Also applies to: 67-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminal/QuickTerminalHotKey.swift` around lines 44 - 45, Extract the hard-coded hotkey identity literals into shared constants so the registration and callback validation use the same values: define a constant (e.g., HOTKEY_SIGNATURE as OSType(0x434D5558) and HOTKEY_ID = 1) and replace the inline literals used when creating EventHotKeyID (hotKeyID) and in the callback validation logic that checks signature/id; update any references in QuickTerminalHotKey (including the creation of hotKeyID and the validation code paths around lines 44-45 and 67-68) to use these constants to prevent drift.
🤖 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/QuickTerminal/QuickTerminalContentView.swift`:
- Around line 17-19: The view sets isWorkspaceInputActive: true but assigns
workspacePortalPriority: 0 which is the unselected tier; change the assignment
so that when the Quick Terminal is active it uses the selected-workspace portal
priority (use 2) instead of 0 — update the initialization/prop for
workspacePortalPriority in QuickTerminalContentView (where
isWorkspaceInputActive is set) to pass 2 for the active workspace to avoid
portal z-order/focus contention.
In `@Sources/QuickTerminal/QuickTerminalController.swift`:
- Around line 58-72: In animateIn(), the controller sets visible = true before
confirming a valid screen (NSScreen.main) which can leave the controller in a
shown state even if no UI appears; move the visible = true assignment to after
the guard that ensures NSScreen.main (or alternatively, keep it where it is but
revert visible to false if the screen guard fails) so that visible accurately
reflects whether the UI can be displayed; update the animateIn() implementation
(referencing animateIn(), visible, and the NSScreen.main guard/ensureWindow())
so visible only becomes true when a screen is available and the window can be
shown.
---
Duplicate comments:
In `@Sources/QuickTerminal/QuickTerminalController.swift`:
- Around line 36-43: In registerGlobalHotKey(), don't ignore the Bool returned
by QuickTerminalHotKey.register(shortcut:); check its result and handle failure:
obtain the shortcut via KeyboardShortcutSettings.shortcut(for:
.toggleQuickTerminal), call hotKey.register(...), and if it returns false then
avoid overwriting globalHotKey and surface an error (e.g., log via a logger,
post a user notification, or present an alert) so the unsupported/conflicting
shortcut is visible; keep the existing hotKey/toggle closure and only assign
globalHotKey = hotKey on success.
---
Nitpick comments:
In `@Sources/QuickTerminal/QuickTerminalHotKey.swift`:
- Around line 44-45: Extract the hard-coded hotkey identity literals into shared
constants so the registration and callback validation use the same values:
define a constant (e.g., HOTKEY_SIGNATURE as OSType(0x434D5558) and HOTKEY_ID =
1) and replace the inline literals used when creating EventHotKeyID (hotKeyID)
and in the callback validation logic that checks signature/id; update any
references in QuickTerminalHotKey (including the creation of hotKeyID and the
validation code paths around lines 44-45 and 67-68) to use these constants to
prevent drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7faab7cd-8977-4f8e-b081-40788521e8be
📒 Files selected for processing (4)
Sources/QuickTerminal/QuickTerminalContentView.swiftSources/QuickTerminal/QuickTerminalController.swiftSources/QuickTerminal/QuickTerminalHotKey.swiftSources/QuickTerminal/QuickTerminalWindow.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/QuickTerminal/QuickTerminalWindow.swift
Add a Quake-style drop-down Quick Terminal that can be toggled from any application via a configurable global hotkey (default Cmd+`). The Quick Terminal has its own independent TabManager for splits, tabs, and workspace features. - Register system-wide Carbon hot key with signature/ID validation - Slide-in/out animation from configurable screen edge (top/bottom/left/right/center) - Route split/focus/zoom/equalizeSplits shortcuts to the QT TabManager - Validate tabId ownership in enqueuePanelTitleUpdate to prevent cross-window notification contamination - Add toggleQuickTerminal to KeyboardShortcutSettings with localization for 18 languages - Add docstrings across Quick Terminal and KeyboardShortcutSettings files
songhanlin
force-pushed
the
feat/quick-terminal
branch
from
March 18, 2026 12:39
de5385f to
50ab7c0
Compare
|
There's also #1523 which tries to achieve the same |
Author
|
Thanks for the pointer, @mrueg — I hadn't seen #1523. I went through both branches. They overlap on the core quake/visor window but took fairly different directions:
So the two feel more complementary than duplicate. I'd rather converge than have two competing PRs — a couple of options:
Maintainers — do you have a preferred direction here? Happy to go whichever way is least work to review. |
Closed
teamleaderleo
added
area: layout
This branch has not been 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
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.
Summary
What changed?
Added a Quake-style Quick Terminal feature, inspired by ghostty's implementation. A floating terminal
panel slides in from the top of the screen via the global hotkey `Cmd+``. The Quick Terminal has its
own TabManager, supporting full split/pane operations independently from the main window.
Why?
ghostty natively supports Quick Terminal, but cmux lacked this capability. Quick Terminal is a
high-frequency workflow feature that allows running commands without leaving the current application
context.
New Features
Cmd+D/Cmd+Shift+D)Alt+Cmd+←→↑↓)Cmd+Wto close focused pane,Cmd+Tfor new surfaceNew Files
Sources/QuickTerminal/QuickTerminalWindow.swift— Borderless floating NSPanelSources/QuickTerminal/QuickTerminalPosition.swift— Position calculation (supportstop/bottom/left/right/center)
Sources/QuickTerminal/QuickTerminalController.swift— Core controller (animation, TabManager, focusmanagement)
Sources/QuickTerminal/QuickTerminalHotKey.swift— Global hotkey registration via Carbon APISources/QuickTerminal/QuickTerminalContentView.swift— SwiftUI rendering layer(WorkspaceContentView wrapper)
Modified Files
KeyboardShortcutSettings.swift— AddedtoggleQuickTerminalactionAppDelegate.swift— Initialize controller + Quick Terminal shortcut routingGhosttyTerminalView.swift— ExposedfocusableViewfor focus managementLocalizable.xcstrings— Localized strings (en/ja/zh-Hans/zh-Hant/ko)Testing
./scripts/reload.sh --tag quick-terminalCmd+D/Cmd+Shift+Dcreates horizontal/vertical splits inside Quick TerminalAlt+Cmd+arrow keysnavigates focus between panesCmd+Wcloses the focused paneCmd+Tcreates a new surfaceDemo Video
Checklist
Summary by cubic
Adds a Quake-style Quick Terminal you can summon from any app with Cmd+`. It runs its own TabManager in a borderless floating panel with smooth slide animation and adds docstrings to improve code coverage.
New Features
) viaCarbon` to toggle Quick Terminal from any app.NSPanelwith slide-in/out animation and equal horizontal padding; supports top/bottom/left/right/center positions.Bug Fixes
EventHotKeyID, clean upCarbonhandlers on deinit, check API return values;register()returns Bool and logs OSStatus codes.TabManager.enqueuePanelTitleUpdateto prevent cross-window title updates.Written for commit 50ab7c0. Summary will update on new commits.
Summary by CodeRabbit