Repository navigation
Add quick terminal (quake/visor mode) support - #1523
TraderSamwise wants to merge 10 commits into
Conversation
|
@TraderSamwise 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}
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 (visor) feature: new QuickTerminalController singleton with global hotkey, SwiftUI floating window, session restore/persistence integration, GhosttyConfig options, AppDelegate startup wiring, localization entry, and session snapshot marking. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App as AppDelegate
participant Config as GhosttyConfig
participant QTC as QuickTerminalController
participant Hotkey as CarbonHotkey
participant Window as NSWindow
App->>QTC: installQuickTerminal()
QTC->>Config: loadConfiguration()
Config-->>QTC: keybind/position/animation/screenFraction
QTC->>Hotkey: register global hotkey / add NSEvent monitor
App->>App: attemptStartupSessionRestoreIfNeeded()
alt snapshot contains isQuickTerminal
App->>QTC: restoreSession(quickTerminalSnapshot)
QTC->>QTC: stash pending snapshot
end
sequenceDiagram
actor User
participant Hotkey as CarbonHotkey
participant QTC as QuickTerminalController
participant TabMgr as Tab/Workspace Manager
participant Window as NSWindow
User->>Hotkey: press hotkey
Hotkey->>QTC: toggle()
alt Quick-terminal hidden
QTC->>QTC: createQuickTerminalWindow()
QTC->>TabMgr: apply pending session snapshot (if any)
QTC->>Window: show with slide animation
else Quick-terminal visible
QTC->>Window: hide with slide animation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Adds a visor-style terminal window that slides in from a screen edge (or fades in at center) via a global hotkey, similar to iTerm2/Guake. Features: - Global hotkey via Carbon RegisterEventHotKey (no Accessibility needed) - Slide animation from top/bottom/left/right or fade for center - Remembers user-resized frame between show/hide cycles - Hotkey-only dismiss (no blur dismiss), matching iTerm2/Warp behavior - Returns focus to previously active app on dismiss - Cmd+W hides instead of destroying the window - Session restore tags visor window in snapshot and restores into visor - Reads config from Ghostty config files via existing GhosttyConfig parser Config keys (in ghostty config): - keybind = global:super+grave_accent=toggle_quick_terminal - quick-terminal-position = top|bottom|left|right|center - quick-terminal-animation-duration = 0.15 - quick-terminal-screen-fraction = 0.5 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
3369-3379:⚠️ Potential issue | 🟠 MajorSkip the quick terminal when persisting fallback regular-window geometry.
After this tag is introduced, a focused visor can become
snapshot.windows.first.saveSessionSnapshot()uses that first snapshot to populatepersistedWindowGeometryDefaultsKey, so the next ordinary window can reopen with the visor's edge-attached frame. Pick the first non-quick-terminal snapshot for fallback geometry persistence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3369 - 3379, The persistence logic currently uses snapshot.windows.first for fallback geometry which can be the quick terminal; update saveSessionSnapshot() so when selecting the snapshot to populate persistedWindowGeometryDefaultsKey it picks the first SessionWindowSnapshot whose isQuickTerminal is not true (e.g. first(where: { $0.isQuickTerminal != true })) and only fall back to .first if none found; reference SessionWindowSnapshot, isQuickTerminal, saveSessionSnapshot(), and persistedWindowGeometryDefaultsKey to locate and change the selection logic.
🧹 Nitpick comments (3)
Sources/QuickTerminalController.swift (3)
95-97: Consider validatinganimationDurationfor non-negative values.
screenFractionis clamped at line 99, butanimationDurationis assigned directly without validation. A negative or excessively large duration from misconfigured settings could cause unexpected animation behavior.🔧 Suggested fix
if let d = config.quickTerminalAnimationDuration { - animationDuration = d + animationDuration = max(0.0, min(1.0, d)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminalController.swift` around lines 95 - 97, Validate and clamp the configured animation duration before assigning it to animationDuration: when reading config.quickTerminalAnimationDuration check for nil, ensure the value is >= 0 (and optionally cap to a sane maximum), then assign the validated value to animationDuration; update the assignment logic around config.quickTerminalAnimationDuration and animationDuration in QuickTerminalController to perform this non-negative (and bounded) validation similar to how screenFraction is clamped.
545-554: Center position ignoresscreenFractionconfiguration.Other positions use the configured
screenFraction, but center position hardcodes0.8. This may be intentional for UX reasons, but it's inconsistent with the documented configuration behavior.🔧 Option to use screenFraction
case .center: - let width = visibleFrame.width * 0.8 - let height = visibleFrame.height * 0.8 + let width = visibleFrame.width * screenFraction + let height = visibleFrame.height * screenFraction return NSRect(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminalController.swift` around lines 545 - 554, The .center branch currently hardcodes 0.8 for width/height, ignoring the configured screenFraction; update the center case in the QuickTerminalController (the method computing the NSRect for positions where case .center is handled) to use the existing screenFraction value (or the same variable used by other branches) instead of 0.8, preserving the calculation pattern: compute width = visibleFrame.width * screenFraction and height = visibleFrame.height * screenFraction and position using visibleFrame.midX/midY as before; if a deliberate override is desired, add a named constant or a conditional flag (e.g., useCenterOverridesScreenFraction) and document its use but default to screenFraction for consistency.
401-410: Window visibility check may match unintended windows.The check at lines 402-406 considers any window with
.titledor.fullSizeContentViewas a "main" window. This could include panels, popovers, or other auxiliary windows, preventingNSApp.hide(nil)from running when it should.Consider filtering by window identifier prefix (e.g., checking for
"cmux.main"or excluding"cmux.quickTerminal","cmux.browser-popup", etc.) for more precise detection.🔧 Suggested improvement
DispatchQueue.main.async { let hasOtherVisibleWindow = NSApp.windows.contains { w in w !== win && w.isVisible && !w.isMiniaturized && w.windowNumber > 0 - && (w.styleMask.contains(.titled) || w.styleMask.contains(.fullSizeContentView)) + && w.identifier?.rawValue.hasPrefix("cmux.main") == true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/QuickTerminalController.swift` around lines 401 - 410, The current DispatchQueue.main.async block computes hasOtherVisibleWindow by inspecting w.styleMask (titled/fullSizeContentView), which can incorrectly match panels/popovers; update the predicate used to set hasOtherVisibleWindow to instead rely on window.identifier?.rawValue (or filter out specific identifiers) — for example, include windows whose identifier prefix equals "cmux.main" and exclude identifiers like "cmux.quickTerminal" and "cmux.browser-popup" — so replace the styleMask checks in the hasOtherVisibleWindow closure with an identifier-based check and then keep the existing NSApp.hide(nil) call when no matching main windows remain.
🤖 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 2445-2457: The quick-terminal snapshot is being removed from
persisted windows too early; change the logic so you only exclude the
quickTerminalSnapshot from regularWindows when you are certain the visor feature
is enabled and will be materialized. In practice: compute quickTerminalSnapshot
as now, call QuickTerminalController.shared.loadConfiguration(), then if
QuickTerminalController.shared.keybind == nil treat the quickTerminalSnapshot as
a normal window (do not filter it out of regularWindows) so it persists; if
keybind != nil call
QuickTerminalController.shared.restoreSession(quickTerminalSnapshot) to stage
the visor but also preserve the snapshot in the persisted/mainWindowContexts
state until the visor window is actually created (i.e., don’t let staging be the
sole place you keep the session), ensuring the staged visor workspace is saved
across quit/restart.
In `@Sources/GhosttyConfig.swift`:
- Around line 202-205: The keybind parsing uses
value.contains("toggle_quick_terminal") which can match unintended substrings;
in the keybind handling (case "keybind") extract the action token from value by
taking the substring after the last '=' (trim whitespace) and compare it exactly
to "toggle_quick_terminal" before assigning quickTerminalKeybindRaw, so only
exact action matches set quickTerminalKeybindRaw.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2212-2216: The toggle handler for
GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL is currently inside the branch guarded by a
check for GHOSTTY_TARGET_SURFACE, so APP-scoped dispatches (GHOSTTY_TARGET_APP)
are dropped; move the entire case for GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL
(which calls performOnMain { QuickTerminalController.shared.toggle(); return
true }) out of that surface-only block and place it before the surface-target
guard (i.e., before the tag == GHOSTTY_TARGET_SURFACE check) so it executes for
all targets.
In `@Sources/QuickTerminalController.swift`:
- Around line 279-288: The local NSEvent monitor and the Ghostty action can both
call Self.shared.toggle(), causing duplicate toggles if the user configured both
bindings; add a short dedupe check in QuickTerminalController to ignore a second
toggle call within a small threshold: add a property like lastToggleTimestamp
(Double) and a constant duplicateToggleThreshold (e.g., 0.3s), update the toggle
entry point used by both the local monitor and Ghostty to early-return if
Date().timeIntervalSince1970 - lastToggleTimestamp < duplicateToggleThreshold
(also preserving the existing isAnimating guard), and update the local monitor
call site (where NSEvent.addLocalMonitorForEvents and Self.eventMatchesKeybind
are used) to rely on this dedupe instead of attempting to coordinate with
Ghostty directly so redundant calls are suppressed.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3369-3379: The persistence logic currently uses
snapshot.windows.first for fallback geometry which can be the quick terminal;
update saveSessionSnapshot() so when selecting the snapshot to populate
persistedWindowGeometryDefaultsKey it picks the first SessionWindowSnapshot
whose isQuickTerminal is not true (e.g. first(where: { $0.isQuickTerminal !=
true })) and only fall back to .first if none found; reference
SessionWindowSnapshot, isQuickTerminal, saveSessionSnapshot(), and
persistedWindowGeometryDefaultsKey to locate and change the selection logic.
---
Nitpick comments:
In `@Sources/QuickTerminalController.swift`:
- Around line 95-97: Validate and clamp the configured animation duration before
assigning it to animationDuration: when reading
config.quickTerminalAnimationDuration check for nil, ensure the value is >= 0
(and optionally cap to a sane maximum), then assign the validated value to
animationDuration; update the assignment logic around
config.quickTerminalAnimationDuration and animationDuration in
QuickTerminalController to perform this non-negative (and bounded) validation
similar to how screenFraction is clamped.
- Around line 545-554: The .center branch currently hardcodes 0.8 for
width/height, ignoring the configured screenFraction; update the center case in
the QuickTerminalController (the method computing the NSRect for positions where
case .center is handled) to use the existing screenFraction value (or the same
variable used by other branches) instead of 0.8, preserving the calculation
pattern: compute width = visibleFrame.width * screenFraction and height =
visibleFrame.height * screenFraction and position using visibleFrame.midX/midY
as before; if a deliberate override is desired, add a named constant or a
conditional flag (e.g., useCenterOverridesScreenFraction) and document its use
but default to screenFraction for consistency.
- Around line 401-410: The current DispatchQueue.main.async block computes
hasOtherVisibleWindow by inspecting w.styleMask (titled/fullSizeContentView),
which can incorrectly match panels/popovers; update the predicate used to set
hasOtherVisibleWindow to instead rely on window.identifier?.rawValue (or filter
out specific identifiers) — for example, include windows whose identifier prefix
equals "cmux.main" and exclude identifiers like "cmux.quickTerminal" and
"cmux.browser-popup" — so replace the styleMask checks in the
hasOtherVisibleWindow closure with an identifier-based check and then keep the
existing NSApp.hide(nil) call when no matching main windows remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53d2751a-d42e-477f-9657-ad2da08784e4
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/QuickTerminalController.swiftSources/SessionPersistence.swift
| case GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL: | ||
| return performOnMain { | ||
| QuickTerminalController.shared.toggle() | ||
| return true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate toggle quick-terminal action definitions/usages =="
rg -n -C4 'GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL|toggle_quick_terminal|GHOSTTY_TARGET_APP|GHOSTTY_TARGET_SURFACE' --type=swift --type=c --type=cpp --type=objc --type=zig
echo
echo "== Inspect action routing handlers =="
rg -n -C6 'func handleAction\(target: ghostty_target_s, action: ghostty_action_s\)|target.tag != GHOSTTY_TARGET_SURFACE|case GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL' --type=swift
echo
echo "== Check tests for target routing coverage =="
rg -n -C4 'GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL|toggle_quick_terminal|handleAction\(|GHOSTTY_TARGET_APP' --glob '*test*' --type=swiftRepository: manaflow-ai/cmux
Length of output: 7679
🏁 Script executed:
sed -n '1730,2230p' Sources/GhosttyTerminalView.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 27007
🏁 Script executed:
# Verify that TOGGLE_QUICK_TERMINAL keybind is marked as global and thus should dispatch with APP target
rg -n -C3 'global.*toggle_quick_terminal|toggle_quick_terminal.*global' Sources/Repository: manaflow-ai/cmux
Length of output: 668
Move quick-terminal toggle handler outside surface-target guard.
The GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL action is defined with a global: keybind scope, which means Ghostty will dispatch it with GHOSTTY_TARGET_APP. However, the current handler at line 2212 is only reachable when target.tag == GHOSTTY_TARGET_SURFACE. For APP-target dispatches, the function returns false at line 1783 and the action is dropped.
Move the GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL case before the surface-target guard (before line 1731) to handle it for all target types:
Proposed fix
private func handleAction(target: ghostty_target_s, action: ghostty_action_s) -> Bool {
+ if action.tag == GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL {
+ return performOnMain {
+ QuickTerminalController.shared.toggle()
+ return true
+ }
+ }
+
if target.tag != GHOSTTY_TARGET_SURFACE {
if action.tag == GHOSTTY_ACTION_RELOAD_CONFIG ||
action.tag == GHOSTTY_ACTION_CONFIG_CHANGE ||
action.tag == GHOSTTY_ACTION_COLOR_CHANGE {
logAction(action, target: target, tabId: nil, surfaceId: nil)
@@
- case GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL:
- return performOnMain {
- QuickTerminalController.shared.toggle()
- return true
- }
default:
return false
}📝 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.
| case GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL: | |
| return performOnMain { | |
| QuickTerminalController.shared.toggle() | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 2212 - 2216, The toggle
handler for GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL is currently inside the branch
guarded by a check for GHOSTTY_TARGET_SURFACE, so APP-scoped dispatches
(GHOSTTY_TARGET_APP) are dropped; move the entire case for
GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL (which calls performOnMain {
QuickTerminalController.shared.toggle(); return true }) out of that surface-only
block and place it before the surface-target guard (i.e., before the tag ==
GHOSTTY_TARGET_SURFACE check) so it executes for all targets.
| // Local monitor: catches the hotkey when the app IS active (Carbon global | ||
| // hotkeys don't fire for the owning app's own key events). | ||
| localMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { event in | ||
| if Self.eventMatchesKeybind(event, bind) { | ||
| Self.shared.toggle() | ||
| return nil | ||
| } | ||
| return event | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL is handled and if it consumes the event
rg -n -A5 "GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL" --type swiftRepository: manaflow-ai/cmux
Length of output: 494
🏁 Script executed:
#!/bin/bash
# Find where actions are processed/invoked in response to events
rg -n "performAction\|handleAction" --type swift | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the action handler dispatch mechanism - look for where GHOSTTY_ACTION_* handlers are invoked
rg -n -B5 "case GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL" --type swiftRepository: manaflow-ai/cmux
Length of output: 550
🏁 Script executed:
#!/bin/bash
# Understand the QuickTerminalController context - check if isAnimating guard exists
sed -n '300,320p' Sources/QuickTerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Find where Ghostty actions are invoked - search for handleGhosttyAction or similar
rg -n "handleGhosttyAction\|performAction\|action.*callback" --type swift | head -30Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for where keyDown events are processed and how they trigger actions
rg -n -B3 -A3 "keyDown\|NSEvent.keyDown" --type swift | head -50Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there's any coordination between local monitor and action handlers
rg -n "localMonitor\|addLocalMonitor" Sources/QuickTerminalController.swift -A10Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read the action handler context - examine the function/switch containing GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL
sed -n '2180,2220p' Sources/GhosttyTerminalView.swiftRepository: manaflow-ai/cmux
Length of output: 1924
🏁 Script executed:
#!/bin/bash
# Find the function that contains this action handler switch
sed -n '2100,2220p' Sources/GhosttyTerminalView.swift | head -100Repository: manaflow-ai/cmux
Length of output: 4582
🏁 Script executed:
#!/bin/bash
# Look for how local monitors interact with the event system
grep -n "addLocalMonitor\|NSEvent.add" Sources/QuickTerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Find the function that contains the action handler switch
sed -n '1900,2220p' Sources/GhosttyTerminalView.swift | grep -n "func\|case GHOSTTY_ACTION_TOGGLE" | head -20Repository: manaflow-ai/cmux
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Search for the function name by looking backwards from line 2212
sed -n '2100,2220p' Sources/GhosttyTerminalView.swift | grep -n "func " | tail -1Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Get broader context - find function signature
sed -n '2050,2220p' Sources/GhosttyTerminalView.swift | grep -B50 "GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL" | head -60Repository: manaflow-ai/cmux
Length of output: 2589
🏁 Script executed:
#!/bin/bash
# Search for the function name directly
rg -n "func.*action" Sources/GhosttyTerminalView.swift | grep -i "callback\|action\|perform" | head -10Repository: manaflow-ai/cmux
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Find what invokes actions - look for ghostty_action callback or similar
rg -n "ghostty_action_callback\|actionCallback" Sources/GhosttyTerminalView.swift -B3 -A3Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/antml/bash
# Get the full function signature by reading wider range
sed -n '1950,2220p' Sources/GhosttyTerminalView.swift | head -100Repository: manaflow-ai/cmux
Length of output: 5036
🏁 Script executed:
#!/bin/bash
# Find where handleAction is called
rg -n "handleAction\(" Sources/GhosttyTerminalView.swift -B2 -A2Repository: manaflow-ai/cmux
Length of output: 551
🏁 Script executed:
#!/an/bash
# Look for Ghostty event callbacks that might invoke handleAction
rg -n "ghostty.*callback\|surfaceCallback" Sources/GhosttyTerminalView.swift | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if actions are triggered from Ghostty's event system (not NSEvent)
# Look for where Ghostty action callbacks are registered
rg -n "action.*=" Sources/GhosttyTerminalView.swift | grep -i callback | head -10Repository: manaflow-ai/cmux
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Verify where Ghostty keybindings are processed - understand the event ordering
rg -n "keybind\|performBindingAction" Sources/GhosttyTerminalView.swift | head -15Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check how NSEvent reaches Ghostty - understand event layering
sed -n '850,870p' Sources/GhosttyTerminalView.swiftRepository: manaflow-ai/cmux
Length of output: 1027
🏁 Script executed:
#!/bin/bash
# See if local monitor can prevent event from reaching Ghostty's keybinding handler
rg -n "NSEvent.*keyDown\|keyDown.*event" Sources/GhosttyTerminalView.swift | head -10Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Understand: Are Ghostty keybindings processed through AppKit's normal event chain?
# Check if Ghostty processes NSEvents or has its own event system
rg -n "NSApplication\|responder\|event.*dispatch" Sources/GhosttyTerminalView.swift | head -15Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if the quick terminal hotkey could overlap with a Ghostty keybinding
# Look for default Ghostty keybinding config
rg -n "toggle.*quick\|quick.*terminal" --type swift -i Sources/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Verify: does addLocalMonitorForEvents prevent events from reaching Ghostty?
# Look for any documentation or code showing event handling order
rg -n "NSEvent.addLocalMonitor\|event.*order\|event.*chain" Sources/ --type swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL is explicitly configured as a keybinding
rg -n "TOGGLE_QUICK_TERMINAL\|toggle-quick-terminal" --type swift --type toml --type yaml --type json --type confRepository: manaflow-ai/cmux
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for Ghostty config files that might define keybindings
find . -name "ghostty*" -o -name "*config*" | grep -i ghostty | head -20Repository: manaflow-ai/cmux
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Check if there are default Ghostty bindings or keybind mappings
rg -n "keybind\|key.*bind" Sources/ --type swift -A2 | head -30Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check Resources/ghostty for keybinding config
cat Resources/ghostty 2>/dev/null | head -50Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for how Ghostty processes events - does it get events from AppKit?
rg -n "ghostty_surface_key\|sendEvent\|handleEvent" Sources/GhosttyTerminalView.swift | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/an/bash
# Verify: when local monitor returns nil, does Ghostty still process the event?
# Check if Ghostty has its own event loop or uses AppKit's
rg -n "ghostty.*run\|ghostty.*tick\|event.*loop" Sources/GhosttyTerminalView.swift | head -15Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check QuickTerminalController to see what keybind is used for quick terminal
cat Sources/QuickTerminalController.swift | head -50Repository: manaflow-ai/cmux
Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Look for the actual keybind configuration in QuickTerminalController
sed -n '200,280p' Sources/QuickTerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 2980
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about Ghostty action handling
head -100 Sources/GhosttyTerminalView.swift | grep -A10 -B10 "action"Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if toggle-quick-terminal is configured as a Ghostty keybinding by default
# Look at how keybindings are loaded from config
rg -n "toggle.*quick\|quick.*terminal\|GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL" Resources/ --type toml --type yaml --type conf 2>/dev/null || rg -n "toggle.*quick\|quick.*terminal\|GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL" Resources/ -aRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Examine the event flow more carefully:
# When local monitor returns nil, does the event reach Ghostty's event processing?
# Check if Ghostty terminal view is in the responder chain after local monitor
rg -n "responder\|nextResponder\|becomeFirstResponder" Sources/GhosttyTerminalView.swift | head -10Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/an/bash
# Final check: Is toggle() idempotent or does rapid calling cause issues?
sed -n '307,330p' Sources/QuickTerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 692
Consider handling the case where user configures both cmux quick-terminal keybind and Ghostty toggle-quick-terminal action for the same key.
If a user explicitly configures both a cmux keybind (via quick terminal settings) and a Ghostty toggle-quick-terminal keybinding for the same key, the action handler and local monitor could both invoke toggle(). The isAnimating guard provides some protection, but it only gates the logic—the redundant calls still occur.
This is an edge case requiring explicit user configuration of both bindings, but worth documenting or preventing if feasible. If a user encounters unexpected behavior, ensure their keybind configuration doesn't duplicate the quick terminal hotkey in their Ghostty config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/QuickTerminalController.swift` around lines 279 - 288, The local
NSEvent monitor and the Ghostty action can both call Self.shared.toggle(),
causing duplicate toggles if the user configured both bindings; add a short
dedupe check in QuickTerminalController to ignore a second toggle call within a
small threshold: add a property like lastToggleTimestamp (Double) and a constant
duplicateToggleThreshold (e.g., 0.3s), update the toggle entry point used by
both the local monitor and Ghostty to early-return if
Date().timeIntervalSince1970 - lastToggleTimestamp < duplicateToggleThreshold
(also preserving the existing isAnimating guard), and update the local monitor
call site (where NSEvent.addLocalMonitorForEvents and Self.eventMatchesKeybind
are used) to rely on this dedupe instead of attempting to coordinate with
Ghostty directly so redundant calls are suppressed.
7da110f to
5df34e1
Compare
![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 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/QuickTerminalController.swift">
<violation number="1" location="Sources/QuickTerminalController.swift:144">
P2: Reject keybinds that do not map to a Carbon keyCode; otherwise the config can be accepted but no hotkey is installed.</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.
|
|
||
| guard let key = keyName else { return nil } | ||
|
|
||
| return QuickTerminalKeybind( |
There was a problem hiding this comment.
P2: Reject keybinds that do not map to a Carbon keyCode; otherwise the config can be accepted but no hotkey is installed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/QuickTerminalController.swift, line 144:
<comment>Reject keybinds that do not map to a Carbon keyCode; otherwise the config can be accepted but no hotkey is installed.</comment>
<file context>
@@ -0,0 +1,569 @@
+
+ guard let key = keyName else { return nil }
+
+ return QuickTerminalKeybind(
+ keyCode: ghosttyKeyNameToKeyCode(key),
+ characters: ghosttyKeyNameToCharacters(key),
</file context>
- Skip quick terminal window when persisting fallback window geometry so regular windows don't reopen with the visor's edge-attached frame - Tighten keybind pre-filter to exact action match instead of substring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
2495-2507:⚠️ Potential issue | 🔴 CriticalDon't drop the quick-terminal snapshot before the visor exists.
regularWindowsstrips the quick-terminal snapshot out before you know it can become a real window, and startup restore later persists only livemainWindowContexts. So if the hotkey is unset, that saved visor session never restores at all; if it is set but the visor is still only staged, the staged session can be overwritten by the temporary SwiftUI window before the quick terminal is registered. Keep the quick-terminal snapshot in persisted state untilQuickTerminalControllerhas actually materialized its window, and treat it as a normal window when the feature is disabled.Also applies to: 2524-2531
🤖 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 7700-7704: The quick-terminal setup in installQuickTerminal uses
QuickTerminalController.shared.loadConfiguration() and installGlobalHotkey()
once, so config reloads don't take effect; add a reconfigure path subscribed to
the .ghosttyConfigDidReload notification that calls
QuickTerminalController.shared.loadConfiguration(), then unregisters and
re-registers the Carbon hotkey (e.g., call an existing
unregisterGlobalHotkey()/installGlobalHotkey() pair or add an unregister method
on QuickTerminalController) to ensure quick-terminal-* settings are updated or
disabled on reload instead of only at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51dc1a69-e6a2-47eb-ba39-f6cfd3494150
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/GhosttyConfig.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyConfig.swift
| private func installQuickTerminal() { | ||
| let controller = QuickTerminalController.shared | ||
| controller.loadConfiguration() | ||
| controller.installGlobalHotkey() | ||
| } |
There was a problem hiding this comment.
Quick-terminal config will stay stale after a Ghostty reload.
loadConfiguration() is single-shot and the Carbon hotkey is only installed here, so reloading Ghostty config cannot update or disable quick-terminal-* settings until restart. Please add a reconfigure path that unregisters/re-registers the hotkey on .ghosttyConfigDidReload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 7700 - 7704, The quick-terminal setup
in installQuickTerminal uses QuickTerminalController.shared.loadConfiguration()
and installGlobalHotkey() once, so config reloads don't take effect; add a
reconfigure path subscribed to the .ghosttyConfigDidReload notification that
calls QuickTerminalController.shared.loadConfiguration(), then unregisters and
re-registers the Carbon hotkey (e.g., call an existing
unregisterGlobalHotkey()/installGlobalHotkey() pair or add an unregister method
on QuickTerminalController) to ensure quick-terminal-* settings are updated or
disabled on reload instead of only at startup.
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyConfig.swift">
<violation number="1" location="Sources/GhosttyConfig.swift:323">
P2: Validate `quick-terminal-animation-duration` before storing it; invalid values currently flow into animation timing unchanged.</violation>
</file>
<file name="Sources/QuickTerminalController.swift">
<violation number="1" location="Sources/QuickTerminalController.swift:243">
P2: `installGlobalHotkey()` returns early when `keyCode` is nil, so character-fallback keybinds never get any handler installed.</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.
| quickTerminalPosition = value | ||
| case "quick-terminal-animation-duration": | ||
| if let dur = Double(value) { | ||
| quickTerminalAnimationDuration = dur |
There was a problem hiding this comment.
P2: Validate quick-terminal-animation-duration before storing it; invalid values currently flow into animation timing unchanged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyConfig.swift, line 323:
<comment>Validate `quick-terminal-animation-duration` before storing it; invalid values currently flow into animation timing unchanged.</comment>
<file context>
@@ -304,6 +310,22 @@ struct GhosttyConfig {
+ quickTerminalPosition = value
+ case "quick-terminal-animation-duration":
+ if let dur = Double(value) {
+ quickTerminalAnimationDuration = dur
+ }
+ case "quick-terminal-screen-fraction":
</file context>
| quickTerminalAnimationDuration = dur | |
| quickTerminalAnimationDuration = dur.isFinite ? max(0, dur) : nil |
| } | ||
|
|
||
| func installGlobalHotkey() { | ||
| guard let bind = keybind, let keyCode = bind.keyCode else { return } |
There was a problem hiding this comment.
P2: installGlobalHotkey() returns early when keyCode is nil, so character-fallback keybinds never get any handler installed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/QuickTerminalController.swift, line 243:
<comment>`installGlobalHotkey()` returns early when `keyCode` is nil, so character-fallback keybinds never get any handler installed.</comment>
<file context>
@@ -0,0 +1,569 @@
+ }
+
+ func installGlobalHotkey() {
+ guard let bind = keybind, let keyCode = bind.keyCode else { return }
+
+ // Register a Carbon global hotkey — works system-wide without Accessibility permissions.
</file context>
When the quick terminal hotkey is removed from config, visor workspaces are now treated as regular windows during session restore instead of being silently dropped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
Sources/AppDelegate.swift (3)
7708-7722:⚠️ Potential issue | 🟠 MajorQuick-terminal hotkey/config remains stale after Ghostty config reload.
installQuickTerminal()is startup-only, and the reload observer (Line 7716-7722) does not reconfigure quick-terminal hotkey state. Changes toquick-terminal-*settings won’t apply until restart.💡 Suggested fix
private func installGhosttyConfigObserver() { guard ghosttyConfigObserver == nil else { return } ghosttyConfigObserver = NotificationCenter.default.addObserver( forName: .ghosttyConfigDidReload, object: nil, queue: .main ) { [weak self] _ in self?.refreshGhosttyGotoSplitShortcuts() + self?.reconfigureQuickTerminal() } } + + private func reconfigureQuickTerminal() { + let controller = QuickTerminalController.shared + controller.unregisterGlobalHotkey() + controller.loadConfiguration() + controller.installGlobalHotkey() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7708 - 7722, The quick-terminal hotkey/config is only set at startup by installQuickTerminal() so changes to quick-terminal-* aren’t applied on Ghostty reload; update installGhosttyConfigObserver() (the observer for .ghosttyConfigDidReload using ghosttyConfigObserver) to also refresh the quick-terminal state by calling QuickTerminalController.shared.loadConfiguration() and QuickTerminalController.shared.installGlobalHotkey() (or simply call installQuickTerminal()) inside the reload handler so hotkey/config changes take effect without restart.
2499-2515:⚠️ Potential issue | 🟠 MajorPending visor session can still be lost before first visor materialization.
Line 2514 only stages the quick-terminal snapshot in the controller; later snapshot saves are built from live
mainWindowContexts. If the visor window is never created in that run, the staged session is not persisted and can be dropped on autosave/quit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2499 - 2515, The quick-terminal snapshot is only staged via QuickTerminalController.shared.restoreSession(quickTerminalSnapshot) which means if the visor never materializes that staged session won't be included in later saves built from mainWindowContexts and can be lost; after staging (inside the same block where quickTerminalSnapshot is non-nil) persist the snapshot immediately using the same persistence path/mechanism you use for mainWindowContexts (either call the controller's existing persistence API or add a method to write the quickTerminalSnapshot to disk/auto-save storage) so the staged session survives autosave/quit.
2532-2577:⚠️ Potential issue | 🟠 MajorStartup restore is finalized before the deferred visor swap completes.
Line 2536 queues async close/toggle, but Line 2576 can call
completeStartupSessionRestore()first. That allows a premature save while the temporary SwiftUI window is still the only live context.💡 Suggested fix
- } else if quickTerminalSnapshot != nil, QuickTerminalController.shared.keybind != nil { + } else if quickTerminalSnapshot != nil, QuickTerminalController.shared.keybind != nil { // No regular windows to restore — hide the SwiftUI-created primary // window and show the visor with the restored session. + isApplyingStartupSessionRestore = true primaryWindow.orderOut(nil) - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in primaryWindow.close() QuickTerminalController.shared.toggle() + self?.completeStartupSessionRestore() } + return } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2532 - 2577, The startup restore can finish before the async visor swap completes; update the quick-terminal branch so completion of the restore is deferred until after the visor swap and window close finish: in the branch that checks quickTerminalSnapshot and QuickTerminalController.shared.keybind, keep primaryWindow.orderOut(nil) but move the finalization (call to completeStartupSessionRestore()) into the DispatchQueue.main.async block and invoke it only after primaryWindow.close() and QuickTerminalController.shared.toggle() have completed (add/use a completion callback on QuickTerminalController.shared.toggle() if it’s asynchronous, or call completeStartupSessionRestore() immediately after toggle() if toggle() is synchronous). Ensure you reference and update the code paths around QuickTerminalController.shared.toggle(), primaryWindow.close(), and completeStartupSessionRestore() so the restore is never finalized while the temporary SwiftUI primary window is still active.
🤖 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 7708-7722: The quick-terminal hotkey/config is only set at startup
by installQuickTerminal() so changes to quick-terminal-* aren’t applied on
Ghostty reload; update installGhosttyConfigObserver() (the observer for
.ghosttyConfigDidReload using ghosttyConfigObserver) to also refresh the
quick-terminal state by calling
QuickTerminalController.shared.loadConfiguration() and
QuickTerminalController.shared.installGlobalHotkey() (or simply call
installQuickTerminal()) inside the reload handler so hotkey/config changes take
effect without restart.
- Around line 2499-2515: The quick-terminal snapshot is only staged via
QuickTerminalController.shared.restoreSession(quickTerminalSnapshot) which means
if the visor never materializes that staged session won't be included in later
saves built from mainWindowContexts and can be lost; after staging (inside the
same block where quickTerminalSnapshot is non-nil) persist the snapshot
immediately using the same persistence path/mechanism you use for
mainWindowContexts (either call the controller's existing persistence API or add
a method to write the quickTerminalSnapshot to disk/auto-save storage) so the
staged session survives autosave/quit.
- Around line 2532-2577: The startup restore can finish before the async visor
swap completes; update the quick-terminal branch so completion of the restore is
deferred until after the visor swap and window close finish: in the branch that
checks quickTerminalSnapshot and QuickTerminalController.shared.keybind, keep
primaryWindow.orderOut(nil) but move the finalization (call to
completeStartupSessionRestore()) into the DispatchQueue.main.async block and
invoke it only after primaryWindow.close() and
QuickTerminalController.shared.toggle() have completed (add/use a completion
callback on QuickTerminalController.shared.toggle() if it’s asynchronous, or
call completeStartupSessionRestore() immediately after toggle() if toggle() is
synchronous). Ensure you reference and update the code paths around
QuickTerminalController.shared.toggle(), primaryWindow.close(), and
completeStartupSessionRestore() so the restore is never finalized while the
temporary SwiftUI primary window is still active.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Sources/AppDelegate.swift (3)
7734-7742:⚠️ Potential issue | 🟠 MajorQuick-terminal config/hotkey still won’t refresh on Ghostty reload.
Observer currently refreshes only split shortcuts. Quick-terminal
keybindand related config remain stale until restart.Proposed fix
) { [weak self] _ in self?.refreshGhosttyGotoSplitShortcuts() + self?.installQuickTerminal() }If
installGlobalHotkey()is not idempotent, reconfigure with an explicit unregister-then-register path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7734 - 7742, The observer installed in installGhosttyConfigObserver only calls refreshGhosttyGotoSplitShortcuts, leaving quick-terminal keybinds stale; update the closure to also reconfigure global hotkeys by invoking installGlobalHotkey (and if installGlobalHotkey is not idempotent, call the corresponding unregister function first—e.g. unregisterGlobalHotkey or removeGlobalHotkey—then call installGlobalHotkey) so that both refreshGhosttyGotoSplitShortcuts and the global hotkey re-registration run on .ghosttyConfigDidReload; ensure ghosttyConfigObserver remains added as before and use [weak self] to avoid retain cycles.
2532-2577:⚠️ Potential issue | 🟠 MajorRestore finalization still races async quick-terminal swap.
At Line 2536-Line 2539, close/toggle is deferred, but at Line 2576 restore can finalize/autosave first. That can snapshot the temporary primary window before the visor materializes.
Proposed fix
} else if quickTerminalSnapshot != nil, QuickTerminalController.shared.keybind != nil { // No regular windows to restore — hide the SwiftUI-created primary // window and show the visor with the restored session. + isApplyingStartupSessionRestore = true primaryWindow.orderOut(nil) - DispatchQueue.main.async { + DispatchQueue.main.async { [weak self] in + guard let self else { return } primaryWindow.close() QuickTerminalController.shared.toggle() + self.completeStartupSessionRestore() } + return } else { let displays = currentDisplayGeometries() let fallbackGeometry = persistedWindowGeometry() if let restoredFrame = Self.resolvedStartupPrimaryWindowFrame(
3443-3469:⚠️ Potential issue | 🟠 MajorPending visor merge is unreachable when no windows exist.
This merge helper is good, but
buildSessionSnapshotexits at Line 3423 whencontextsis empty, so pending quick-terminal state is still dropped in no-window saves.Proposed fix
- guard !contexts.isEmpty else { return nil } - let windows: [SessionWindowSnapshot] = contexts .prefix(SessionPersistencePolicy.maxWindowsPerSnapshot) .map { context in let window = context.window ?? windowForMainWindowId(context.windowId) let isQuickTerminal = context.windowId == QuickTerminalController.shared.windowId return SessionWindowSnapshot( frame: window.map { SessionRectSnapshot($0.frame) }, display: displaySnapshot(for: window), tabManager: context.tabManager.sessionSnapshot(includeScrollback: includeScrollback), sidebar: SessionSidebarSnapshot( isVisible: context.sidebarState.isVisible, selection: SessionSidebarSelection(selection: context.sidebarSelectionState.selection), width: SessionPersistencePolicy.sanitizedSidebarWidth(Double(context.sidebarState.persistedWidth)) ), isQuickTerminal: isQuickTerminal ? true : nil ) } let windowsWithPendingQuickTerminal = Self.includingPendingQuickTerminalSnapshot( windows, pendingQuickTerminalSnapshot: QuickTerminalController.shared.pendingSessionSnapshotForPersistence() ) guard !windowsWithPendingQuickTerminal.isEmpty else { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3443 - 3469, The early return in buildSessionSnapshot skips persisting a pending quick-terminal snapshot when there are zero windows; update buildSessionSnapshot to obtain pendingQuickTerminalSnapshot = QuickTerminalController.shared.pendingSessionSnapshotForPersistence() and call includingPendingQuickTerminalSnapshot(_:pendingQuickTerminalSnapshot:) before the empty-context early return (or change the guard to allow proceeding when a pending snapshot exists) so the pending quick terminal is merged and saved; keep using includingPendingQuickTerminalSnapshot, which already respects SessionPersistencePolicy.maxWindowsPerSnapshot and the isQuickTerminal check.
🤖 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/QuickTerminalController.swift`:
- Around line 242-288: installGlobalHotkey currently overwrites carbonHotkeyRef,
carbonHandlerRef, and localMonitor on repeated calls and ignores OSStatus from
InstallEventHandler/RegisterEventHotKey; update it to first unregister/uninstall
any existing carbonHotkeyRef (UnregisterEventHotKey), remove existing handler
(RemoveEventHandler) and remove localMonitor (NSEvent.removeMonitor) before
registering new ones, and capture the return OSStatus values from
InstallEventHandler and RegisterEventHotKey (check for errors like
eventHotKeyExistsErr) and surface/log them (or return a Bool/throw) so failures
aren’t silent; reference the installGlobalHotkey method and the carbonHotkeyRef,
carbonHandlerRef, localMonitor variables as the targets to modify and handle the
InstallEventHandler/RegisterEventHotKey return values.
- Around line 319-346: The show() method uses lastFrame unconditionally which
can leave the visor off-screen after display topology changes; update show() to
validate lastFrame against the current visibleFrame (use quickTerminalFrame(in:)
as the fallback) — e.g. in show() compute targetFrame = (lastFrame != nil &&
lastFrame!.intersects(visibleFrame.insetBy(dx: 10, dy: 10))) ? lastFrame! :
quickTerminalFrame(in: visibleFrame), or use a similar intersection/clamping
heuristics (non-trivial overlap) before accepting lastFrame; reference the
functions/vars show(), lastFrame, quickTerminalFrame(in:), and visibleFrame when
implementing the check so the window is always placed within the current screen
bounds.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 7734-7742: The observer installed in installGhosttyConfigObserver
only calls refreshGhosttyGotoSplitShortcuts, leaving quick-terminal keybinds
stale; update the closure to also reconfigure global hotkeys by invoking
installGlobalHotkey (and if installGlobalHotkey is not idempotent, call the
corresponding unregister function first—e.g. unregisterGlobalHotkey or
removeGlobalHotkey—then call installGlobalHotkey) so that both
refreshGhosttyGotoSplitShortcuts and the global hotkey re-registration run on
.ghosttyConfigDidReload; ensure ghosttyConfigObserver remains added as before
and use [weak self] to avoid retain cycles.
- Around line 3443-3469: The early return in buildSessionSnapshot skips
persisting a pending quick-terminal snapshot when there are zero windows; update
buildSessionSnapshot to obtain pendingQuickTerminalSnapshot =
QuickTerminalController.shared.pendingSessionSnapshotForPersistence() and call
includingPendingQuickTerminalSnapshot(_:pendingQuickTerminalSnapshot:) before
the empty-context early return (or change the guard to allow proceeding when a
pending snapshot exists) so the pending quick terminal is merged and saved; keep
using includingPendingQuickTerminalSnapshot, which already respects
SessionPersistencePolicy.maxWindowsPerSnapshot and the isQuickTerminal check.
🪄 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: b65368a3-8d98-423e-8d73-26eb8e6b4f88
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/QuickTerminalController.swiftcmuxTests/SessionPersistenceTests.swift
| func installGlobalHotkey() { | ||
| guard let bind = keybind, let keyCode = bind.keyCode else { return } | ||
|
|
||
| // Register a Carbon global hotkey — works system-wide without Accessibility permissions. | ||
| let hotkeyID = EventHotKeyID(signature: OSType(0x636D7578), id: 1) // "cmux" | ||
| let modifiers = Self.carbonModifiers(for: bind) | ||
|
|
||
| var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed)) | ||
| let handlerCallback: EventHandlerUPP = { _, event, _ -> OSStatus in | ||
| DispatchQueue.main.async { | ||
| QuickTerminalController.shared.toggle() | ||
| } | ||
| return noErr | ||
| } | ||
|
|
||
| var handlerRef: EventHandlerRef? | ||
| InstallEventHandler( | ||
| GetApplicationEventTarget(), | ||
| handlerCallback, | ||
| 1, | ||
| &eventType, | ||
| nil, | ||
| &handlerRef | ||
| ) | ||
| carbonHandlerRef = handlerRef | ||
|
|
||
| var hotkeyRef: EventHotKeyRef? | ||
| RegisterEventHotKey( | ||
| UInt32(keyCode), | ||
| modifiers, | ||
| hotkeyID, | ||
| GetApplicationEventTarget(), | ||
| 0, | ||
| &hotkeyRef | ||
| ) | ||
| carbonHotkeyRef = hotkeyRef | ||
|
|
||
| // Local monitor: catches the hotkey when the app IS active (Carbon global | ||
| // hotkeys don't fire for the owning app's own key events). | ||
| localMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { event in | ||
| if Self.eventMatchesKeybind(event, bind) { | ||
| Self.shared.toggle() | ||
| return nil | ||
| } | ||
| return event | ||
| } | ||
| } |
There was a problem hiding this comment.
Make installGlobalHotkey() idempotent and surface Carbon registration failures.
Two concerns:
- No re-entrancy guard. If
installGlobalHotkey()is ever called twice (e.g., future config reload), the previouscarbonHotkeyRef,carbonHandlerRef, andlocalMonitorare overwritten without being unregistered — leaking the Carbon handler/hotkey and installing a second local monitor that will firetoggle()twice per keypress. - OSStatus ignored. Both
InstallEventHandlerandRegisterEventHotKeyreturn anOSStatusthat is discarded. A common real-world failure iseventHotKeyExistsErrwhen another app already owns the chord — in that case the user silently gets no global hotkey, and from their perspective the feature is broken with no diagnostic.
🛡️ Proposed fix
func installGlobalHotkey() {
guard let bind = keybind, let keyCode = bind.keyCode else { return }
+ // Idempotent: tear down any prior registration first.
+ removeGlobalHotkey()
let hotkeyID = EventHotKeyID(signature: OSType(0x636D7578), id: 1) // "cmux"
let modifiers = Self.carbonModifiers(for: bind)
var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed))
let handlerCallback: EventHandlerUPP = { _, event, _ -> OSStatus in
DispatchQueue.main.async {
QuickTerminalController.shared.toggle()
}
return noErr
}
var handlerRef: EventHandlerRef?
- InstallEventHandler(
+ let installStatus = InstallEventHandler(
GetApplicationEventTarget(),
handlerCallback,
1,
&eventType,
nil,
&handlerRef
)
carbonHandlerRef = handlerRef
var hotkeyRef: EventHotKeyRef?
- RegisterEventHotKey(
+ let registerStatus = RegisterEventHotKey(
UInt32(keyCode),
modifiers,
hotkeyID,
GetApplicationEventTarget(),
0,
&hotkeyRef
)
carbonHotkeyRef = hotkeyRef
+ `#if` DEBUG
+ if installStatus != noErr || registerStatus != noErr {
+ dlog("QuickTerminal: hotkey registration failed install=\(installStatus) register=\(registerStatus)")
+ }
+ `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/QuickTerminalController.swift` around lines 242 - 288,
installGlobalHotkey currently overwrites carbonHotkeyRef, carbonHandlerRef, and
localMonitor on repeated calls and ignores OSStatus from
InstallEventHandler/RegisterEventHotKey; update it to first unregister/uninstall
any existing carbonHotkeyRef (UnregisterEventHotKey), remove existing handler
(RemoveEventHandler) and remove localMonitor (NSEvent.removeMonitor) before
registering new ones, and capture the return OSStatus values from
InstallEventHandler and RegisterEventHotKey (check for errors like
eventHotKeyExistsErr) and surface/log them (or return a Bool/throw) so failures
aren’t silent; reference the installGlobalHotkey method and the carbonHotkeyRef,
carbonHandlerRef, localMonitor variables as the targets to modify and handle the
InstallEventHandler/RegisterEventHotKey return values.
| private func show() { | ||
| let win = window ?? createQuickTerminalWindow() | ||
| window = win | ||
|
|
||
| guard let screen = NSScreen.main ?? NSScreen.screens.first else { return } | ||
| let visibleFrame = screen.visibleFrame | ||
| let targetFrame = lastFrame ?? quickTerminalFrame(in: visibleFrame) | ||
|
|
||
| // Set initial off-screen frame for slide animation | ||
| var startFrame = targetFrame | ||
| switch position { | ||
| case .top: | ||
| startFrame.origin.y = visibleFrame.maxY | ||
| case .bottom: | ||
| startFrame.origin.y = visibleFrame.minY - targetFrame.height | ||
| case .left: | ||
| startFrame.origin.x = visibleFrame.minX - targetFrame.width | ||
| case .right: | ||
| startFrame.origin.x = visibleFrame.maxX | ||
| case .center: | ||
| break | ||
| } | ||
|
|
||
| win.setFrame(startFrame, display: false) | ||
| win.alphaValue = position == .center ? 0 : 1 | ||
| win.orderFrontRegardless() | ||
| NSApp.activate(ignoringOtherApps: true) | ||
| win.makeKey() |
There was a problem hiding this comment.
lastFrame may place the visor window off-screen after display topology changes.
targetFrame = lastFrame ?? quickTerminalFrame(in: visibleFrame) reuses the previously stored frame unconditionally. If the user disconnected the monitor that held the visor, changed resolution, or moved the current NSScreen.main, the saved frame can fall entirely outside screen.visibleFrame, producing a window that slides in off-screen and is only dismissible via the hotkey.
Consider validating the saved frame against visibleFrame (e.g., require non-trivial intersection) and falling back to quickTerminalFrame(in:) when it doesn't fit — analogous to the existing AppDelegate.resolvedWindowFrame(...) clamping used for main windows.
- let targetFrame = lastFrame ?? quickTerminalFrame(in: visibleFrame)
+ let defaultFrame = quickTerminalFrame(in: visibleFrame)
+ let targetFrame: NSRect = {
+ guard let saved = lastFrame,
+ visibleFrame.intersection(saved).width >= 100,
+ visibleFrame.intersection(saved).height >= 100 else {
+ return defaultFrame
+ }
+ return saved
+ }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/QuickTerminalController.swift` around lines 319 - 346, The show()
method uses lastFrame unconditionally which can leave the visor off-screen after
display topology changes; update show() to validate lastFrame against the
current visibleFrame (use quickTerminalFrame(in:) as the fallback) — e.g. in
show() compute targetFrame = (lastFrame != nil &&
lastFrame!.intersects(visibleFrame.insetBy(dx: 10, dy: 10))) ? lastFrame! :
quickTerminalFrame(in: visibleFrame), or use a similar intersection/clamping
heuristics (non-trivial overlap) before accepting lastFrame; reference the
functions/vars show(), lastFrame, quickTerminalFrame(in:), and visibleFrame when
implementing the check so the window is always placed within the current screen
bounds.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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:3465">
P2: Pending quick-terminal persistence can evict one regular window snapshot when the session is already at the max window limit, causing loss of a saved workspace/window on restore.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
is this feature available now? whats the hot key for it 🚀 |
Two related defenses for visor and main-window session restore: - Set isTerminatingApp synchronously inside the willPowerOff observer so SwiftUI's WindowGroup teardown during logout/restart sees the flag and skips the unregister-time removeWhenEmpty save that was wiping the snapshot before applicationShouldTerminate could persist it. - Add a trivial-overwrite shield in persistSessionSnapshot: once a non-trivial snapshot has been persisted, refuse to overwrite (or remove) it with a snapshot that looks like a fresh launch (≤1 window with ≤1 default workspace and no custom state). The cache is seeded from the on-disk snapshot at startup and updated after each successful write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds a quake/visor-style quick terminal to cmux — a floating
Confidence Score: 4/5The quick-terminal logic is well-structured and the session persistence changes are careful, but the project file registers 13 additional source files whose contents are not in the diff and cannot be reviewed here. The quick-terminal window lifecycle, Carbon hotkey bridge, session restore split, and trivial-snapshot guard are all sound and well-tested. The concern is the project file adding
Important Files Changed
Sequence DiagramsequenceDiagram
participant Config as GhosttyConfig
participant App as AppDelegate
participant QTC as QuickTerminalController
participant Carbon as Carbon API
participant Win as NSWindow (visor)
participant Persist as SessionPersistenceStore
Note over App: applicationDidFinishLaunching
App->>QTC: loadConfiguration()
QTC->>Config: GhosttyConfig.load()
Config-->>QTC: keybind, position, duration, fraction
App->>QTC: installGlobalHotkey()
QTC->>Carbon: RegisterEventHotKey + InstallEventHandler
QTC->>QTC: addLocalMonitorForEvents
Note over App: attemptStartupSessionRestoreIfNeeded
App->>Persist: SessionPersistenceStore.load()
Persist-->>App: AppSessionSnapshot
App->>App: split visor vs regular windows
App->>QTC: restoreSession(quickTerminalSnapshot)
QTC->>QTC: "pendingSessionSnapshot = snapshot"
Note over Carbon: hotkey pressed
Carbon-->>QTC: toggle() via DispatchQueue.main.async
QTC->>Win: createQuickTerminalWindow() if nil
QTC->>App: registerMainWindow(window)
QTC->>Win: slide-in animation
Note over Win: Cmd+W
Win->>QTC: windowShouldClose returns false
QTC->>Win: slide-out + orderOut
QTC->>QTC: NSApp.hide if no other visible windows
Note over App: autosave tick
App->>QTC: pendingSessionSnapshotForPersistence()
App->>App: includingPendingQuickTerminalSnapshot
App->>Persist: save(merged)
|
| guard snapshot != nil || removeWhenEmpty || persistedGeometryData != nil else { return } | ||
|
|
||
| let willPersistSnapshot = snapshot != nil || removeWhenEmpty | ||
| let newIsTrivial = Self.isTrivialSnapshot(snapshot) | ||
| let blockSnapshotPersist = willPersistSnapshot | ||
| && newIsTrivial | ||
| && lastPersistedSnapshotIsNonTrivial | ||
|
|
||
| #if DEBUG | ||
| if blockSnapshotPersist { | ||
| dlog("session.save.skipped reason=trivial_overwrite_protect removeWhenEmpty=\(removeWhenEmpty ? 1 : 0)") | ||
| } | ||
| #endif | ||
|
|
||
| let writeBlock = { | ||
| if let persistedGeometryData { | ||
| UserDefaults.standard.set( | ||
| persistedGeometryData, | ||
| forKey: Self.persistedWindowGeometryDefaultsKey | ||
| ) | ||
| } | ||
| if blockSnapshotPersist { return } | ||
| if let snapshot { | ||
| _ = SessionPersistenceStore.save(snapshot) | ||
| } else if removeWhenEmpty { |
There was a problem hiding this comment.
removeWhenEmpty session deletion
isTrivialSnapshot(nil) returns true, so blockSnapshotPersist becomes true whenever lastPersistedSnapshotIsNonTrivial is set and snapshot is nil — exactly the case where removeWhenEmpty = true (user closed every window without quitting). The writeBlock returns early, the session file is never deleted, and the next cold launch silently restores the user's old session even though they deliberately closed everything. The guard should only apply to writes of an actual snapshot, not to deletions.
| NSAnimationContext.runAnimationGroup({ context in | ||
| context.duration = hideDuration | ||
| context.timingFunction = CAMediaTimingFunction(name: .easeIn) | ||
| win.animator().setFrame(endFrame, display: true) | ||
| win.animator().alphaValue = 0 | ||
| }, completionHandler: { [weak self] in |
There was a problem hiding this comment.
hide() and show()
hide() unconditionally animates alphaValue to 0 for every position, but show() only animates alpha for .center. For edge-docked positions (top, bottom, left, right) this produces a fade+slide on dismiss but a pure slide on reveal, which looks asymmetric. The alpha animation in hide() should be conditioned on .center to match.
| NSAnimationContext.runAnimationGroup({ context in | |
| context.duration = hideDuration | |
| context.timingFunction = CAMediaTimingFunction(name: .easeIn) | |
| win.animator().setFrame(endFrame, display: true) | |
| win.animator().alphaValue = 0 | |
| }, completionHandler: { [weak self] in | |
| NSAnimationContext.runAnimationGroup({ context in | |
| context.duration = hideDuration | |
| context.timingFunction = CAMediaTimingFunction(name: .easeIn) | |
| win.animator().setFrame(endFrame, display: true) | |
| if position == .center { | |
| win.animator().alphaValue = 0 | |
| } | |
| }, completionHandler: { [weak self] in |
# Conflicts: # GhosttyTabs.xcodeproj/project.pbxproj # Resources/Localizable.xcstrings # Sources/AppDelegate.swift # cmuxTests/SessionPersistenceTests.swift
| } else if quickTerminalSnapshot != nil, QuickTerminalController.shared.keybind != nil { | ||
| // No regular windows to restore — hide the SwiftUI-created primary | ||
| // window and show the visor with the restored session. | ||
| primaryWindow.orderOut(nil) | ||
| DispatchQueue.main.async { | ||
| primaryWindow.close() | ||
| QuickTerminalController.shared.toggle() | ||
| } |
There was a problem hiding this comment.
primaryWindow.orderOut(nil) hides the window synchronously, then DispatchQueue.main.async defers close() and toggle() to avoid re-entering registerMainWindow during the session restore call stack. The comment names the symptom but not the invariant: nothing prevents toggle() from being called during session restore other than this run-loop deferral. If SwiftUI's WindowGroup responds to close() by spawning a new window (to maintain at least one live window) before the deferred block fires, toggle() would race with the newly created primary window's registration, potentially corrupting mainWindowContexts or triggering a second session restore attempt. The fix should make registerMainWindow safe to call from the createQuickTerminalWindow path during startup, or gate toggle() on a post-restore flag so the timing is enforced structurally rather than by run-loop ordering.
File Used: .github/review-bot-rules/swift-architectural-rethink.md (source)
Upstream main added @EnvironmentObject FileExplorerState and CmuxConfigStore to ContentView. QuickTerminalController's NSHostingView only injected the four older env objects, so opening the visor crashed with SIGILL the moment ContentView.body tried to read fileExplorerState. Create both objects (wiring CmuxConfigStore to the visor's TabManager and calling loadAll(), matching createMainWindow), inject them via .environmentObject, pass them to registerMainWindow, and clear them on willClose so they don't outlive the visor window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itlement Upstream main wired the Release config to Resources/cmux.entitlements, which declares keychain-access-groups. codesign refuses that entitlement under ad-hoc signing because it requires the team prefix to be validated, so local Release builds (and any machine without a development cert) failed with "entitlements that require signing with a development certificate". Clear the Release config's CODE_SIGN_ENTITLEMENTS and remove the file. The auth code's FallbackTokenStore already falls back to the file store when keychain writes fail, so removing the shared keychain group has no runtime impact on this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a "Personal fork notes (read first)" section to CLAUDE.md covering: - Why the visor needs FileExplorerState + CmuxConfigStore injected (and what to do when upstream adds more @EnvironmentObjects to ContentView) - Why Resources/cmux.entitlements must stay deleted and the Release config's CODE_SIGN_ENTITLEMENTS must stay empty (ad-hoc-only signing) - Fresh-machine clone/build/install sequence Add a one-paragraph personal-fork notice at the top of README.md pointing at the CLAUDE.md section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This branch has not been deployed
3 participants
Summary
Details
RegisterEventHotKey(no Accessibility permissions needed)keybind,quick-terminal-position,quick-terminal-animation-duration,quick-terminal-screen-fractionGhosttyConfigparser (no duplicate file parsing)registerMainWindowduring startupConfig example
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds a quick “visor” terminal toggled by a global hotkey that slides from any screen edge (or fades at center). It remembers size, hides on Cmd+W, restores visor sessions without flashing, and protects session data; if the hotkey is removed, visor workspaces reopen as regular windows.
New Features
GhosttyConfig; localized title (EN/JA).Bug Fixes
...=toggle_quick_terminal).FileExplorerStateandCmuxConfigStoreinto the visorContentViewand registering them with the window.AuthManagerfalls back to the file store when keychain writes fail; added fork build notes inCLAUDE.mdand a notice inREADME.md.Written for commit 39f4c58. Summary will update on new commits.
Summary by CodeRabbit
New Features
Startup & Session Restore
Configuration
Localization
Tests