Repository navigation
Add global hotkey popup terminal - #1353
Open
ogabrielluiz wants to merge 8 commits into
Open
ogabrielluiz wants to merge 8 commits into
ogabrielluiz wants to merge 8 commits into
Conversation
|
@ogabrielluiz 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.
|
To use Codex here, create an environment for this repo. |
Author
|
@codex review |
📝 WalkthroughWalkthroughAdds a macOS popup terminal: CLI commands and server handlers (toggle/show/hide), a PopupTerminalController with show/hide/reposition and settings persisted in UserDefaults, global Carbon hotkey support (function-key handling, default F10), UI settings integration, and unit tests for settings and frame calculations. Changes
Sequence DiagramsequenceDiagram
actor User
participant AppDelegate
participant TerminalController
participant PopupTerminalController
participant NSPanel
User->>AppDelegate: Presses registered hotkey (e.g., F10)
AppDelegate->>TerminalController: Dispatch v2 "popup-terminal.toggle"
TerminalController->>PopupTerminalController: toggle()
alt popup hidden
PopupTerminalController->>PopupTerminalController: capture previous app & hide regular windows
PopupTerminalController->>NSPanel: create/retrieve panel and set content
PopupTerminalController->>NSPanel: animate to target frame & activate
NSPanel-->>User: popup shown
else popup visible
PopupTerminalController->>NSPanel: animate offscreen and hide
PopupTerminalController->>PopupTerminalController: restore previous app focus
NSPanel-->>User: popup hidden
end
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 |
@ogabrielluiz I have started the AI code review. It will take a few minutes to complete. |
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.
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.
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Add a Guake/Warp-style popup terminal that slides in from a screen edge via a global hotkey. Uses Carbon RegisterEventHotKey for system-wide F10 capture without requiring Accessibility permissions. - PopupTerminalController: singleton managing floating NSPanel lifecycle, activation policy dance, and focus restoration - PopupTerminalSettings: UserDefaults-backed settings with testable accessors and pure frame computation functions - Carbon hotkey registration in AppDelegate - KeyboardShortcutSettings: togglePopupTerminal action with F10 default
Add configurable settings in Preferences for position, screen, size, auto-hide on focus loss, and animation duration. Includes onChange handler to re-register hotkey when enable/disable is toggled.
Add popup-terminal.toggle/show/hide v2 socket commands and corresponding CLI commands for scripting and automation.
Unit tests for UserDefaults persistence (defaults, round-trips, enum fallbacks) and pure frame computation (all positions, clamping, offscreen displacement, offset screens).
ogabrielluiz
force-pushed
the
feature/popup-terminal
branch
from
March 13, 2026 11:51
6d53667 to
b38256c
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d53667e55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // panel from reappearing if the user clicks the dock icon. | ||
| panel.orderOut(nil) | ||
|
|
||
| restoreFocus(to: appToRestore) |
There was a problem hiding this comment.
When auto-hide fires on NSApplication.didResignActiveNotification, this hide() path still restores previousApp captured at show time. If the popup was opened from app A and the user clicks app B, cmux immediately re-activates app A, stealing focus from the app the user just selected. This makes app switching unreliable whenever autoHideOnFocusLoss is enabled.
Useful? React with 👍 / 👎.
Comment on lines
+292
to
+294
| self.removeFocusLossObserver() | ||
| self.panel = nil | ||
| self.isVisible = false |
There was a problem hiding this comment.
The panel is created as .closable, but windowWillClose only clears controller state; it does not restore activation policy or previously focused app. Closing via the close button/Cmd+W while visible can leave the app in .accessory mode with regular windows still ordered out, which is inconsistent with the normal hide() teardown and can strand users without expected Dock/window restoration behavior.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| } | ||
| .onChange(of: popupEnabled) { _ in AppDelegate.shared?.registerPopupTerminalHotKey() } |
There was a problem hiding this comment.
This settings section only re-registers the Carbon hotkey when popupEnabled changes. Updating ShortcutSettingRow(action: .togglePopupTerminal) writes a new shortcut to defaults, but no re-registration is triggered here, so the old keybinding remains active until restart (or a manual enable toggle).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
Sources/TerminalController.swift (1)
2879-2887: Return observed visibility forshow/hideresponses (matchtoggle).
togglereports actual visibility, butshow/hidereturn constants. Prefer readingPopupTerminalController.shared.isVisibleafter the action for consistent API behavior.Proposed refactor
private func v2PopupTerminalShow(params _: [String: Any]) -> V2CallResult { v2MainSync { PopupTerminalController.shared.show() } - return .ok(["visible": true]) + return .ok(["visible": v2MainSync { PopupTerminalController.shared.isVisible }]) } private func v2PopupTerminalHide(params _: [String: Any]) -> V2CallResult { v2MainSync { PopupTerminalController.shared.hide() } - return .ok(["visible": false]) + return .ok(["visible": v2MainSync { PopupTerminalController.shared.isVisible }]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 2879 - 2887, The show/hide handlers currently return constant visibility rather than the actual state; update v2PopupTerminalShow and v2PopupTerminalHide to call PopupTerminalController.shared.show()/hide() inside v2MainSync and then read PopupTerminalController.shared.isVisible and return that value (e.g., .ok(["visible": PopupTerminalController.shared.isVisible])) so their responses match the behavior of toggle; ensure you read isVisible after performing the action to capture the real resulting state.Sources/AppDelegate.swift (1)
9371-9374: Consider centralizing function-key keyCode mapping to one source of truth.This map duplicates the F1–F12 mapping logic already maintained in
Sources/KeyboardShortcutSettings.swiftand can drift over time.💡 Refactor direction
- private static let functionKeyCodeMap: [String: UInt16] = [ - "f1": 122, "f2": 120, "f3": 99, "f4": 118, "f5": 96, "f6": 97, - "f7": 98, "f8": 100, "f9": 101, "f10": 109, "f11": 103, "f12": 111, - ] + private static let functionKeyCodeMap: [String: UInt16] = KeyboardShortcutSettings.functionKeyCodeMap(Or expose a shared helper from one module and consume it in both places.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9371 - 9374, The function-key code map defined as private static let functionKeyCodeMap in AppDelegate duplicates the mapping in KeyboardShortcutSettings; remove the duplicate and reference a single source of truth instead—expose the existing mapping from KeyboardShortcutSettings (for example by adding a public static property or helper like KeyboardShortcutSettings.functionKeyCodeMap or a dedicated KeyboardKeyCodes helper) and update AppDelegate to use that exposed property; ensure the visibility is public/internal as needed and update any callers to import or reference the shared symbol so only one mapping is maintained.cmuxTests/PopupTerminalSettingsTests.swift (1)
209-232: Consider adding negative-percentage clamping cases.You already test low positive and >100 values; adding negative inputs would harden edge-case coverage for persisted corrupt values.
➕ Optional test addition
+ func testNegativePercentagesClampToMinimum10Percent() { + let frame = PopupTerminalSettings.computeTargetFrame( + position: .top, widthPercent: -25, heightPercent: -10, + visibleFrame: standardScreen + ) + XCTAssertEqual(frame.width, 1920 * 0.1, accuracy: 0.01) + XCTAssertEqual(frame.height, 1080 * 0.1, accuracy: 0.01) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/PopupTerminalSettingsTests.swift` around lines 209 - 232, Add tests that pass negative widthPercent and heightPercent into PopupTerminalSettings.computeTargetFrame to ensure they are clamped to the minimum (10%). Specifically, add one test calling computeTargetFrame with widthPercent: -50, heightPercent: 50 and assert frame.width == 1920 * 0.1 (accuracy 0.01), and another with widthPercent: 50, heightPercent: -20 and assert frame.height == 1080 * 0.1 (accuracy 0.01); reference the existing test naming pattern (e.g., testWidthPercentClampedToMinimum10Percent / testHeightPercentClampedToMinimum10Percent) and reuse standardScreen as the visibleFrame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 1770-1782: The subcommand help system doesn't recognize the new
popup-terminal commands because subcommandUsage(_:) lacks cases for
"popup-terminal-toggle", "popup-terminal-show", and "popup-terminal-hide";
update subcommandUsage(_:) to include these three cases returning appropriate
usage/help text (matching the style of the other entries) so that
dispatchSubcommandHelp(...) can display help for cmux popup-terminal-*. Use the
existing patterns in subcommandUsage(_:) for naming and formatting to ensure
consistent help output.
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Line 101: The PBX object IDs for PopupTerminalSettingsTests.swift are
duplicated with GhosttyEnsureFocusWindowActivationTests.swift (PBXBuildFile
entries and fileRef IDs); to fix, assign new unique PBX IDs for each duplicate
occurrence related to PopupTerminalSettingsTests.swift (both the PBXBuildFile
and its fileRef) and update every matching reference in the project.pbxproj so
they no longer reuse the IDs from GhosttyEnsureFocusWindowActivationTests.swift,
ensuring PBXBuildFile/fileRef pairs (and any other duplicated IDs at the
mentioned occurrences) are unique across the file.
In `@Sources/AppDelegate.swift`:
- Around line 7546-7548: Replace the debug print call inside the guard that
checks Self.functionKeyCodeMap (the branch that prints "[PopupTerminal] Shortcut
key '\(shortcut.key)' is not a function key; hotkey not registered") with a
dlog() call and wrap that call in `#if` DEBUG / `#endif`; do the same for the other
similar print usage later in the same scope (the second diagnostic that reports
an unregistered hotkey), ensuring you call dlog(...) with the same formatted
message instead of print(...).
- Around line 7579-7620: The installCarbonHotKeyHandler currently sets
Self.carbonHandlerInstalled = true before calling InstallEventHandler and does
not capture the returned handler, so failures permanently block retries and
prevent cleanup; change the flow in installCarbonHotKeyHandler to call
InstallEventHandler first, check its OSStatus result, and only set
Self.carbonHandlerInstalled = true when status == noErr; also capture the
created EventHandlerRef (or store it in a new static var like
Self.carbonEventHandlerRef) so you can remove the handler later (using
RemoveEventHandler) on teardown or on failure, and if InstallEventHandler fails,
log the error and leave carbonHandlerInstalled false to allow retries.
In `@Sources/cmuxApp.swift`:
- Around line 3100-3105: resetAllSettings() currently doesn't clear the new
popup-terminal AppStorage properties, so add logic to reset popupEnabled,
popupPosition, popupScreen, popupWidth, popupHeight, and popupAutoHide back to
their defaults (use PopupTerminalSettings.defaultEnabled,
.defaultPosition.rawValue, .defaultScreen.rawValue, .defaultWidthPercent,
.defaultHeightPercent, and .defaultAutoHideOnFocusLoss respectively) inside
resetAllSettings() so the popup-terminal state is cleared on a true "Reset All".
- Around line 3501-3506: Add immediate propagation for popupAutoHide changes:
implement a PopupTerminalController method refreshAutoHideBehavior() that checks
isVisible and then calls installFocusLossObserver() when
PopupTerminalSettings.autoHideOnFocusLoss is true or removeFocusLossObserver()
when false, and wire the UI by adding an .onChange(of: popupAutoHide) { _ in
PopupTerminalController.shared.refreshAutoHideBehavior() } alongside the
existing .onChange handlers so changing popupAutoHide updates focus-loss
behavior instantly.
In `@Sources/PopupTerminalController.swift`:
- Around line 72-75: getOrCreatePanel currently can return a brand-new NSPanel
via `panel ?? NSPanel()` without assigning it to the controller, which lets
`show()` proceed but leaves `self.panel` nil so `hide()` no-ops; update
getOrCreatePanel (the method referenced) to always assign any newly created
panel to `self.panel` before returning (or return an optional and let `show()`
guard-fail), ensuring `self.panel` is the same instance used by `show()` and
`hide()` (referencing the `panel` property, `show()`, and `hide()` to locate the
logic).
In `@Sources/PopupTerminalSettings.swift`:
- Around line 110-118: The widthPercent(defaults:) and heightPercent(defaults:)
accessors currently only check for >0, but must clamp persisted values to the
10–100 range to match runtime layout; update both functions (referencing
widthPercentKey, heightPercentKey, defaultWidthPercent, defaultHeightPercent) to
read the stored Double, and if it's <=0 return the respective default, otherwise
clamp the value into the inclusive range 10...100 (e.g. using min/max or a clamp
helper) and return that clamped value so stored and effective values remain
consistent.
In `@Sources/TerminalController.swift`:
- Around line 1995-2002: v2Capabilities() does not advertise the newly added RPC
handlers (v2PopupTerminalToggle, v2PopupTerminalShow, v2PopupTerminalHide), so
capability-driven clients will ignore them; update the v2Capabilities()
implementation to include the strings "popup-terminal.toggle",
"popup-terminal.show", and "popup-terminal.hide" in the returned capabilities
list/set so these methods are reported as supported alongside the other
capabilities.
---
Nitpick comments:
In `@cmuxTests/PopupTerminalSettingsTests.swift`:
- Around line 209-232: Add tests that pass negative widthPercent and
heightPercent into PopupTerminalSettings.computeTargetFrame to ensure they are
clamped to the minimum (10%). Specifically, add one test calling
computeTargetFrame with widthPercent: -50, heightPercent: 50 and assert
frame.width == 1920 * 0.1 (accuracy 0.01), and another with widthPercent: 50,
heightPercent: -20 and assert frame.height == 1080 * 0.1 (accuracy 0.01);
reference the existing test naming pattern (e.g.,
testWidthPercentClampedToMinimum10Percent /
testHeightPercentClampedToMinimum10Percent) and reuse standardScreen as the
visibleFrame.
In `@Sources/AppDelegate.swift`:
- Around line 9371-9374: The function-key code map defined as private static let
functionKeyCodeMap in AppDelegate duplicates the mapping in
KeyboardShortcutSettings; remove the duplicate and reference a single source of
truth instead—expose the existing mapping from KeyboardShortcutSettings (for
example by adding a public static property or helper like
KeyboardShortcutSettings.functionKeyCodeMap or a dedicated KeyboardKeyCodes
helper) and update AppDelegate to use that exposed property; ensure the
visibility is public/internal as needed and update any callers to import or
reference the shared symbol so only one mapping is maintained.
In `@Sources/TerminalController.swift`:
- Around line 2879-2887: The show/hide handlers currently return constant
visibility rather than the actual state; update v2PopupTerminalShow and
v2PopupTerminalHide to call PopupTerminalController.shared.show()/hide() inside
v2MainSync and then read PopupTerminalController.shared.isVisible and return
that value (e.g., .ok(["visible": PopupTerminalController.shared.isVisible])) so
their responses match the behavior of toggle; ensure you read isVisible after
performing the action to capture the real resulting state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bb3fe34-d267-4e29-ba46-ee4601cf45c5
📒 Files selected for processing (9)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftSources/PopupTerminalController.swiftSources/PopupTerminalSettings.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/PopupTerminalSettingsTests.swift
Comment on lines
+7579
to
+7620
| private func installCarbonHotKeyHandler() { | ||
| guard !Self.carbonHandlerInstalled else { return } | ||
| Self.carbonHandlerInstalled = true | ||
|
|
||
| var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed)) | ||
| InstallEventHandler( | ||
| GetApplicationEventTarget(), | ||
| { _, event, _ -> OSStatus in | ||
| var hotKeyID = EventHotKeyID() | ||
| let status = GetEventParameter( | ||
| event, | ||
| EventParamName(kEventParamDirectObject), | ||
| EventParamType(typeEventHotKeyID), | ||
| nil, | ||
| MemoryLayout<EventHotKeyID>.size, | ||
| nil, | ||
| &hotKeyID | ||
| ) | ||
| guard status == noErr else { return status } | ||
|
|
||
| // Check if this is our popup terminal hotkey. | ||
| // Carbon RegisterEventHotKey consumes the key event at the system | ||
| // level, so no window (local or popup) ever sees the registered | ||
| // hotkey as a keyDown. This handler is the sole toggle path for | ||
| // the popup terminal. | ||
| if hotKeyID.signature == AppDelegate.popupTerminalHotKeyID.signature, | ||
| hotKeyID.id == AppDelegate.popupTerminalHotKeyID.id { | ||
| // Carbon event handlers on GetApplicationEventTarget() run | ||
| // on the main thread, so we can call toggle directly. | ||
| if PopupTerminalSettings.isEnabled { | ||
| PopupTerminalController.shared.toggle() | ||
| } | ||
| return noErr | ||
| } | ||
|
|
||
| return OSStatus(eventNotHandledErr) | ||
| }, | ||
| 1, | ||
| &eventType, | ||
| nil, | ||
| nil | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current Carbon handler installation flow and status handling.
rg -n -C4 'carbonHandlerInstalled|InstallEventHandler\(|registerPopupTerminalHotKey\(' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 2052
🏁 Script executed:
#!/bin/bash
# Check the unregisterPopupTerminalHotKey implementation and globalHotkeyRef usage
rg -n -B2 -A10 'globalHotkeyRef|UnregisterEventHotKey' Sources/AppDelegate.swift | head -50Repository: manaflow-ai/cmux
Length of output: 1881
🏁 Script executed:
#!/bin/bash
# Get the complete installCarbonHotKeyHandler function
rg -n -A 45 'private func installCarbonHotKeyHandler' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 2249
Check InstallEventHandler status before setting carbonHandlerInstalled.
carbonHandlerInstalled is set to true before verifying handler installation succeeds. If InstallEventHandler fails, retries are permanently blocked and the hotkey system is effectively dead. The handler reference is also not captured, making cleanup impossible if needed.
Suggested fix
private func installCarbonHotKeyHandler() {
guard !Self.carbonHandlerInstalled else { return }
- Self.carbonHandlerInstalled = true
var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed))
- InstallEventHandler(
+ var handlerRef: EventHandlerRef?
+ let status = InstallEventHandler(
GetApplicationEventTarget(),
{ _, event, _ -> OSStatus in
var hotKeyID = EventHotKeyID()
let status = GetEventParameter(
event,
EventParamName(kEventParamDirectObject),
EventParamType(typeEventHotKeyID),
nil,
MemoryLayout<EventHotKeyID>.size,
nil,
&hotKeyID
)
guard status == noErr else { return status }
// Check if this is our popup terminal hotkey.
// Carbon RegisterEventHotKey consumes the key event at the system
// level, so no window (local or popup) ever sees the registered
// hotkey as a keyDown. This handler is the sole toggle path for
// the popup terminal.
if hotKeyID.signature == AppDelegate.popupTerminalHotKeyID.signature,
hotKeyID.id == AppDelegate.popupTerminalHotKeyID.id {
// Carbon event handlers on GetApplicationEventTarget() run
// on the main thread, so we can call toggle directly.
if PopupTerminalSettings.isEnabled {
PopupTerminalController.shared.toggle()
}
return noErr
}
return OSStatus(eventNotHandledErr)
},
1,
&eventType,
nil,
- nil
+ &handlerRef
)
+ guard status == noErr else { return }
+ Self.carbonHandlerInstalled = true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 7579 - 7620, The
installCarbonHotKeyHandler currently sets Self.carbonHandlerInstalled = true
before calling InstallEventHandler and does not capture the returned handler, so
failures permanently block retries and prevent cleanup; change the flow in
installCarbonHotKeyHandler to call InstallEventHandler first, check its OSStatus
result, and only set Self.carbonHandlerInstalled = true when status == noErr;
also capture the created EventHandlerRef (or store it in a new static var like
Self.carbonEventHandlerRef) so you can remove the handler later (using
RemoveEventHandler) on teardown or on failure, and if InstallEventHandler fails,
log the error and leave carbonHandlerInstalled false to allow retries.
Comment on lines
+3501
to
+3506
| .onChange(of: popupEnabled) { _ in AppDelegate.shared?.registerPopupTerminalHotKey() } | ||
| .onChange(of: popupPosition) { _ in PopupTerminalController.shared.reposition() } | ||
| .onChange(of: popupScreen) { _ in PopupTerminalController.shared.reposition() } | ||
| .onChange(of: popupWidth) { _ in PopupTerminalController.shared.reposition() } | ||
| .onChange(of: popupHeight) { _ in PopupTerminalController.shared.reposition() } | ||
| } |
There was a problem hiding this comment.
Apply auto-hide preference changes immediately while the popup is visible.
Line 3501–Line 3505 reacts to most popup settings, but popupAutoHide changes are not propagated. If the panel is already shown, focus-loss behavior stays stale until the next toggle cycle.
Proposed fix
.onChange(of: popupEnabled) { _ in AppDelegate.shared?.registerPopupTerminalHotKey() }
.onChange(of: popupPosition) { _ in PopupTerminalController.shared.reposition() }
.onChange(of: popupScreen) { _ in PopupTerminalController.shared.reposition() }
.onChange(of: popupWidth) { _ in PopupTerminalController.shared.reposition() }
.onChange(of: popupHeight) { _ in PopupTerminalController.shared.reposition() }
+ .onChange(of: popupAutoHide) { _ in PopupTerminalController.shared.refreshAutoHideBehavior() }// Add in PopupTerminalController
func refreshAutoHideBehavior() {
guard isVisible else { return }
if PopupTerminalSettings.autoHideOnFocusLoss {
installFocusLossObserver()
} else {
removeFocusLossObserver()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 3501 - 3506, Add immediate propagation
for popupAutoHide changes: implement a PopupTerminalController method
refreshAutoHideBehavior() that checks isVisible and then calls
installFocusLossObserver() when PopupTerminalSettings.autoHideOnFocusLoss is
true or removeFocusLossObserver() when false, and wire the UI by adding an
.onChange(of: popupAutoHide) { _ in
PopupTerminalController.shared.refreshAutoHideBehavior() } alongside the
existing .onChange handlers so changing popupAutoHide updates focus-loss
behavior instantly.
Comment on lines
+72
to
+75
| func show() { | ||
| isAnimating = true | ||
| let panel = getOrCreatePanel() | ||
|
|
There was a problem hiding this comment.
Don’t return an untracked fallback panel when AppDelegate.shared is nil.
At Line 184, panel ?? NSPanel() can return a fresh panel that is never stored in self.panel. If that path is hit, show() can mark visible, but hide() later no-ops because self.panel is nil.
Proposed fix
-func show() {
+func show() {
isAnimating = true
- let panel = getOrCreatePanel()
+ guard let panel = getOrCreatePanel() else {
+ isAnimating = false
+ return
+ }-private func getOrCreatePanel() -> NSPanel {
+private func getOrCreatePanel() -> NSPanel? {
if let existing = panel {
return existing
}
@@
- guard let appDelegate = AppDelegate.shared else {
- return panel ?? NSPanel()
- }
+ guard let appDelegate = AppDelegate.shared else {
+ return nil
+ }
@@
- return newPanel
+ return newPanel
}Also applies to: 172-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/PopupTerminalController.swift` around lines 72 - 75, getOrCreatePanel
currently can return a brand-new NSPanel via `panel ?? NSPanel()` without
assigning it to the controller, which lets `show()` proceed but leaves
`self.panel` nil so `hide()` no-ops; update getOrCreatePanel (the method
referenced) to always assign any newly created panel to `self.panel` before
returning (or return an optional and let `show()` guard-fail), ensuring
`self.panel` is the same instance used by `show()` and `hide()` (referencing the
`panel` property, `show()`, and `hide()` to locate the logic).
Comment on lines
+110
to
+118
| static func widthPercent(defaults: UserDefaults) -> Double { | ||
| let val = defaults.double(forKey: widthPercentKey) | ||
| return val > 0 ? val : defaultWidthPercent | ||
| } | ||
|
|
||
| static func heightPercent(defaults: UserDefaults) -> Double { | ||
| let val = defaults.double(forKey: heightPercentKey) | ||
| return val > 0 ? val : defaultHeightPercent | ||
| } |
There was a problem hiding this comment.
Clamp persisted percent settings to 10–100 to avoid UI/runtime drift.
At Line 110 and Line 115, values are only validated as > 0. Values like 5 or 500 remain persisted and shown in UI, while runtime layout later clamps to 10–100. Please clamp at accessor level too so stored and effective values stay consistent.
Proposed fix
static func widthPercent(defaults: UserDefaults) -> Double {
let val = defaults.double(forKey: widthPercentKey)
- return val > 0 ? val : defaultWidthPercent
+ let resolved = val > 0 ? val : defaultWidthPercent
+ return min(100, max(10, resolved))
}
static func heightPercent(defaults: UserDefaults) -> Double {
let val = defaults.double(forKey: heightPercentKey)
- return val > 0 ? val : defaultHeightPercent
+ let resolved = val > 0 ? val : defaultHeightPercent
+ return min(100, max(10, resolved))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static func widthPercent(defaults: UserDefaults) -> Double { | |
| let val = defaults.double(forKey: widthPercentKey) | |
| return val > 0 ? val : defaultWidthPercent | |
| } | |
| static func heightPercent(defaults: UserDefaults) -> Double { | |
| let val = defaults.double(forKey: heightPercentKey) | |
| return val > 0 ? val : defaultHeightPercent | |
| } | |
| static func widthPercent(defaults: UserDefaults) -> Double { | |
| let val = defaults.double(forKey: widthPercentKey) | |
| let resolved = val > 0 ? val : defaultWidthPercent | |
| return min(100, max(10, resolved)) | |
| } | |
| static func heightPercent(defaults: UserDefaults) -> Double { | |
| let val = defaults.double(forKey: heightPercentKey) | |
| let resolved = val > 0 ? val : defaultHeightPercent | |
| return min(100, max(10, resolved)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/PopupTerminalSettings.swift` around lines 110 - 118, The
widthPercent(defaults:) and heightPercent(defaults:) accessors currently only
check for >0, but must clamp persisted values to the 10–100 range to match
runtime layout; update both functions (referencing widthPercentKey,
heightPercentKey, defaultWidthPercent, defaultHeightPercent) to read the stored
Double, and if it's <=0 return the respective default, otherwise clamp the value
into the inclusive range 10...100 (e.g. using min/max or a clamp helper) and
return that clamped value so stored and effective values remain consistent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 9 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/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:3501">
P2: Changing the popup-terminal shortcut does not re-register the global hotkey, so the old key binding remains active.</violation>
</file>
<file name="Sources/PopupTerminalController.swift">
<violation number="1" location="Sources/PopupTerminalController.swift:115">
P1: Auto-hide on focus loss restores focus to the old app, which can steal focus from the app the user just clicked.</violation>
<violation number="2" location="Sources/PopupTerminalController.swift:290">
P2: Closing the popup window directly bypasses `hide()`, so activation policy may stay `.accessory` and app visibility state becomes inconsistent.</violation>
</file>
<file name="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:101">
P1: The new test target entries reuse existing PBX UUIDs, causing project object ID collisions.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:2885">
P2: `popup-terminal.hide` calls `hide()` directly and bypasses the `toggle()` animation guard. A hide request during show animation can run through an unsupported transition path and destabilize popup focus-loss observer lifecycle.</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.
| } | ||
|
|
||
| private func v2PopupTerminalHide(params _: [String: Any]) -> V2CallResult { | ||
| v2MainSync { PopupTerminalController.shared.hide() } |
There was a problem hiding this comment.
P2: popup-terminal.hide calls hide() directly and bypasses the toggle() animation guard. A hide request during show animation can run through an unsupported transition path and destabilize popup focus-loss observer lifecycle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalController.swift, line 2885:
<comment>`popup-terminal.hide` calls `hide()` directly and bypasses the `toggle()` animation guard. A hide request during show animation can run through an unsupported transition path and destabilize popup focus-loss observer lifecycle.</comment>
<file context>
@@ -2861,6 +2869,23 @@ class TerminalController {
+ }
+
+ private func v2PopupTerminalHide(params _: [String: Any]) -> V2CallResult {
+ v2MainSync { PopupTerminalController.shared.hide() }
+ return .ok(["visible": false])
+ }
</file context>
- Fix auto-hide restoring focus to wrong app (restore skipped when auto-hide triggers, since user already switched to another app) - Route windowWillClose through hide() so activation policy and focus are properly restored on panel close via Cmd+W - Re-register global hotkey when shortcut changes in settings - Fix PBX UUID collision with GhosttyEnsureFocusWindowActivationTests - Add popup-terminal commands to system.capabilities - Add CLI help entries for popup-terminal-toggle/show/hide - Include popup settings in resetAllSettings() - Add refreshAutoHideBehavior() for live auto-hide toggle - Use dlog() instead of print() per project conventions
Author
|
@codex review |
@ogabrielluiz I have started the AI code review. It will take a few minutes to complete. |
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.
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcfcea9a24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+172
to
+175
| case .top: | ||
| return targetFrame.offsetBy(dx: 0, dy: targetFrame.height) | ||
| case .bottom: | ||
| return targetFrame.offsetBy(dx: 0, dy: -targetFrame.height) |
There was a problem hiding this comment.
For .top/.bottom, computeOffscreenFrame shifts by targetFrame.height instead of using the screen bounds, so when visibleFrame is inset by the menu bar or Dock, part of the panel can still be on-screen before the slide animation starts. On typical macOS layouts this produces a visible strip/flicker and a shortened slide distance, unlike the left/right branches that account for screenFrame explicitly.
Useful? React with 👍 / 👎.
Comment on lines
+7546
to
+7550
| guard let fkCode = Self.functionKeyCodeMap[key] else { | ||
| #if DEBUG | ||
| dlog("popupTerminal.hotkey skipped reason=nonFunctionKey key=\(shortcut.key)") | ||
| #endif | ||
| return |
There was a problem hiding this comment.
This path silently skips registration for any non-function key, but the popup shortcut is configured through the generic recorder, which still allows combinations like ⌘K; users can save such a shortcut and end up with a non-working global hotkey with no feedback. That creates an easy-to-hit broken configuration state until they manually reset to an F key, so this should be rejected or surfaced at save time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
7583-7624:⚠️ Potential issue | 🟠 Major
carbonHandlerInstalledis set too early in handler installation flow.Line 7585 marks installation complete before confirming
InstallEventHandlersucceeded. If install fails, retries are blocked and the hotkey path can remain dead for the rest of the process. (Downstream: hotkey registration can still proceed without a working dispatcher.)Suggested fix
- private static var carbonHandlerInstalled = false - private func installCarbonHotKeyHandler() { - guard !Self.carbonHandlerInstalled else { return } - Self.carbonHandlerInstalled = true + private static var carbonHandlerInstalled = false + private static var carbonEventHandlerRef: EventHandlerRef? + private func installCarbonHotKeyHandler() -> Bool { + guard !Self.carbonHandlerInstalled else { return true } var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed)) - InstallEventHandler( + var handlerRef: EventHandlerRef? + let status = InstallEventHandler( GetApplicationEventTarget(), { _, event, _ -> OSStatus in var hotKeyID = EventHotKeyID() @@ 1, &eventType, nil, - nil + &handlerRef ) + guard status == noErr else { +#if DEBUG + dlog("popupTerminal.hotkey handlerInstallFailed status=\(status)") +#endif + return false + } + Self.carbonEventHandlerRef = handlerRef + Self.carbonHandlerInstalled = true + return true }- installCarbonHotKeyHandler() + guard installCarbonHotKeyHandler() else { return }#!/bin/bash # Verify current Carbon handler installation flow and status handling. rg -n -A55 -B8 'private func installCarbonHotKeyHandler\(' Sources/AppDelegate.swift # Verify whether installed flag is set before checking install status. rg -n 'carbonHandlerInstalled = true|InstallEventHandler\(|status == noErr|EventHandlerRef|RemoveEventHandler' Sources/AppDelegate.swiftExpected after fix:
carbonHandlerInstalled = trueappears only after astatus == noErrguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7583 - 7624, installCarbonHotKeyHandler currently sets Self.carbonHandlerInstalled = true before calling InstallEventHandler, which means a failed InstallEventHandler will permanently mark the handler installed; change the flow so you call InstallEventHandler first, capture its returned OSStatus (and any EventHandlerRef if used), check status == noErr, and only then set Self.carbonHandlerInstalled = true; ensure you return or log the error and leave the flag false when InstallEventHandler fails (referencing installCarbonHotKeyHandler, Self.carbonHandlerInstalled, and InstallEventHandler to locate the code).
🧹 Nitpick comments (1)
Sources/cmuxApp.swift (1)
3507-3512: Scope hotkey re-registration to actual shortcut changes.Line 3507 currently reacts to every
UserDefaultsmutation, triggering unnecessary unregister/register cycles inregisterPopupTerminalHotKey()during unrelated settings edits. Gate this observer on.togglePopupTerminalshortcut changes only by tracking the last known shortcut value and re-registering only when it actually changes.This pattern already exists elsewhere in cmuxApp (lines 4933–4937):
.onReceive(NotificationCenter.default.publisher(for: UserDefaults.didChangeNotification)) { _ in let latest = KeyboardShortcutSettings.shortcut(for: action) if latest != shortcut { shortcut = latest } }Apply the same approach: add a
@Stateto cache the last popup terminal shortcut and compare on each notification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3507 - 3512, The current observer unconditionally calls AppDelegate.shared?.registerPopupTerminalHotKey() on any UserDefaults change; instead add a `@State` property (e.g., popupShortcut) to cache KeyboardShortcutSettings.shortcut(for: .togglePopupTerminal) and in the .onReceive(NotificationCenter.default.publisher(for: UserDefaults.didChangeNotification)) handler read the latest via KeyboardShortcutSettings.shortcut(for: .togglePopupTerminal) and only call registerPopupTerminalHotKey() (and update popupShortcut) when the latest != popupShortcut; this mirrors the existing pattern used elsewhere (lines using KeyboardShortcutSettings.shortcut and shortcut comparison) so the unregister/register cycle only runs when the togglePopupTerminal shortcut actually changes.
🤖 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 7583-7624: installCarbonHotKeyHandler currently sets
Self.carbonHandlerInstalled = true before calling InstallEventHandler, which
means a failed InstallEventHandler will permanently mark the handler installed;
change the flow so you call InstallEventHandler first, capture its returned
OSStatus (and any EventHandlerRef if used), check status == noErr, and only then
set Self.carbonHandlerInstalled = true; ensure you return or log the error and
leave the flag false when InstallEventHandler fails (referencing
installCarbonHotKeyHandler, Self.carbonHandlerInstalled, and InstallEventHandler
to locate the code).
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 3507-3512: The current observer unconditionally calls
AppDelegate.shared?.registerPopupTerminalHotKey() on any UserDefaults change;
instead add a `@State` property (e.g., popupShortcut) to cache
KeyboardShortcutSettings.shortcut(for: .togglePopupTerminal) and in the
.onReceive(NotificationCenter.default.publisher(for:
UserDefaults.didChangeNotification)) handler read the latest via
KeyboardShortcutSettings.shortcut(for: .togglePopupTerminal) and only call
registerPopupTerminalHotKey() (and update popupShortcut) when the latest !=
popupShortcut; this mirrors the existing pattern used elsewhere (lines using
KeyboardShortcutSettings.shortcut and shortcut comparison) so the
unregister/register cycle only runs when the togglePopupTerminal shortcut
actually changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcf7dacc-4a73-43c9-82f0-aa9b828b1f84
📒 Files selected for processing (6)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/PopupTerminalController.swiftSources/TerminalController.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/TerminalController.swift
- CLI/cmux.swift
- Sources/PopupTerminalController.swift
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:7546">
P2: Non-function popup shortcuts can be recorded and saved, but this guard silently skips registration so the popup shortcut stops working.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
7583-7624:⚠️ Potential issue | 🟠 MajorGate
carbonHandlerInstalledon successful handler installation.Line 7585 flips
carbonHandlerInstalledbefore checkingInstallEventHandlersuccess. If install fails once, future retries are blocked and hotkey dispatch stays broken.💡 Suggested fix
private static var carbonHandlerInstalled = false +private static var carbonHandlerRef: EventHandlerRef? private func installCarbonHotKeyHandler() { guard !Self.carbonHandlerInstalled else { return } - Self.carbonHandlerInstalled = true var eventType = EventTypeSpec(eventClass: OSType(kEventClassKeyboard), eventKind: UInt32(kEventHotKeyPressed)) - InstallEventHandler( + var handlerRef: EventHandlerRef? + let status = InstallEventHandler( GetApplicationEventTarget(), { _, event, _ -> OSStatus in var hotKeyID = EventHotKeyID() let status = GetEventParameter( event, @@ 1, &eventType, nil, - nil + &handlerRef ) + guard status == noErr else { +#if DEBUG + dlog("popupTerminal.hotkey installHandlerFailed status=\(status)") +#endif + return + } + Self.carbonHandlerRef = handlerRef + Self.carbonHandlerInstalled = true }#!/bin/bash # Verify the install flow currently sets the installed flag before checking Carbon status. rg -n -C6 'private func installCarbonHotKeyHandler|carbonHandlerInstalled|InstallEventHandler\(' Sources/AppDelegate.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7583 - 7624, The code sets Self.carbonHandlerInstalled = true at the start of installCarbonHotKeyHandler(), which marks the handler installed even if InstallEventHandler fails; change the flow so you only set Self.carbonHandlerInstalled = true after a successful InstallEventHandler call (i.e., check InstallEventHandler's return/OSStatus and only flip Self.carbonHandlerInstalled on success), leaving it false on failure so future retries can attempt installation; locate the logic in installCarbonHotKeyHandler() around the InstallEventHandler call and adjust accordingly.
🤖 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 9389-9391: The reverse key-code map currently maps "\r" to 36
which loses keypad-enter (76) when round-tripping; update the reverse mapping
entry for "\r" (the dictionary that currently contains "\"\\r\": 36") to
include/return the keypad-enter key code 76 (e.g., change the mapping so "\r"
corresponds to 76 or otherwise prefer 76 when choosing between 36 and 76) so
that storedKey(from:) (which normalizes 36 and 76 to "\r") round-trips correctly
to keypad-enter.
In `@Sources/cmuxApp.swift`:
- Around line 3480-3492: The popup width/height TextField bindings (popupWidth
and popupHeight used in the SettingsCardRow blocks and TextField views)
currently accept unrestricted numbers; add input-time clamping to enforce 10–100
so invalid values aren't persisted. Implement an onChange (or a custom Binding)
for each TextField (the ones bound to popupWidth and popupHeight) that clamps
the value to the 10–100 range and writes the clamped value back to the state
variable, ensuring the UI and persisted settings always remain within bounds.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 7583-7624: The code sets Self.carbonHandlerInstalled = true at the
start of installCarbonHotKeyHandler(), which marks the handler installed even if
InstallEventHandler fails; change the flow so you only set
Self.carbonHandlerInstalled = true after a successful InstallEventHandler call
(i.e., check InstallEventHandler's return/OSStatus and only flip
Self.carbonHandlerInstalled on success), leaving it false on failure so future
retries can attempt installation; locate the logic in
installCarbonHotKeyHandler() around the InstallEventHandler call and adjust
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 476eb668-304c-4fc0-a382-7e820e5002ad
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/cmuxApp.swift
Comment on lines
+9389
to
+9391
| "\t": 48, "\r": 36, | ||
| "[": 33, "]": 30, "-": 27, "=": 24, | ||
| ",": 43, ".": 47, "/": 44, ";": 41, "'": 39, "`": 50, "\\": 42, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify normalization vs registration mapping for Return/Keypad Enter.
rg -n -C3 'case 36, 76: return "\\r"|case 36, 76: return "\r"' Sources/KeyboardShortcutSettings.swift
rg -n -C3 '"\\r": 36|"\\r": 36' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 811
Keypad-enter is missing from the reverse key-code mapping.
storedKey(from:) normalizes both Return (36) and Keypad Enter (76) to "\r", but the reverse map at line 9389 only registers "\r": 36. A shortcut recorded with keypad-enter will map back to Return's key code instead of Keypad Enter's.
Mapping gap
// Normalization (KeyboardShortcutSettings.swift:394)
case 36, 76: return "\r" // return, keypad enter
// Reverse mapping (AppDelegate.swift:9389)
"\r": 36, // missing 76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 9389 - 9391, The reverse key-code map
currently maps "\r" to 36 which loses keypad-enter (76) when round-tripping;
update the reverse mapping entry for "\r" (the dictionary that currently
contains "\"\\r\": 36") to include/return the keypad-enter key code 76 (e.g.,
change the mapping so "\r" corresponds to 76 or otherwise prefer 76 when
choosing between 36 and 76) so that storedKey(from:) (which normalizes 36 and 76
to "\r") round-trips correctly to keypad-enter.
Comment on lines
+3480
to
+3492
| SettingsCardRow(String(localized: "popupTerminal.settings.widthPercent", defaultValue: "Width %")) { | ||
| TextField("", value: $popupWidth, format: .number) | ||
| .frame(width: 60) | ||
| .textFieldStyle(.roundedBorder) | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow(String(localized: "popupTerminal.settings.heightPercent", defaultValue: "Height %")) { | ||
| TextField("", value: $popupHeight, format: .number) | ||
| .frame(width: 60) | ||
| .textFieldStyle(.roundedBorder) | ||
| } |
There was a problem hiding this comment.
Enforce 10–100 bounds for popup width/height at input time.
Line 3481 and Line 3489 currently accept unrestricted numeric values. Clamping before repositioning prevents invalid persisted settings and keeps behavior predictable.
Suggested fix
- .onChange(of: popupWidth) { _ in PopupTerminalController.shared.reposition() }
- .onChange(of: popupHeight) { _ in PopupTerminalController.shared.reposition() }
+ .onChange(of: popupWidth) { _ in
+ popupWidth = min(max(popupWidth, 10), 100)
+ PopupTerminalController.shared.reposition()
+ }
+ .onChange(of: popupHeight) { _ in
+ popupHeight = min(max(popupHeight, 10), 100)
+ PopupTerminalController.shared.reposition()
+ }Also applies to: 3505-3506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 3480 - 3492, The popup width/height
TextField bindings (popupWidth and popupHeight used in the SettingsCardRow
blocks and TextField views) currently accept unrestricted numbers; add
input-time clamping to enforce 10–100 so invalid values aren't persisted.
Implement an onChange (or a custom Binding) for each TextField (the ones bound
to popupWidth and popupHeight) that clamps the value to the 10–100 range and
writes the clamped value back to the state variable, ensuring the UI and
persisted settings always remain within bounds.
teamleaderleo
added
the
area: terminal
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
2 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
Hey all. I wanted to build this feature because it was the only thing keeping me from switching to cmux.
What it does is add a global hotkey popup terminal, similar to what Guake on Linux and Warp on macOS offer. Press F10 from anywhere (even when cmux isn't focused) and a terminal slides in from the top of the screen. Press F10 again and it slides away, restoring focus to whatever app you were using before.
How it works
RegisterEventHotKeyAPI to capture the hotkey globally. This is the same approach macOS apps like Warp use, and it doesn't require Accessibility or Input Monitoring permissions.NSPanelwith.floatinglevel that appears above all other windows. When shown, cmux switches to.accessoryactivation policy so it doesn't appear in the Dock or app switcher..transientand.ignoresCyclecollection behavior so it won't reappear on dock click or Cmd+` cycling.Settings
All configurable in Preferences:
What's included
PopupTerminalController- singleton managing the panel lifecycle, activation policy, and focus restoration. Includes design notes documenting the three AppKit subsystems that interact and why each step is needed.PopupTerminalSettings- UserDefaults-backed settings with testable accessors (accept injectedUserDefaults) and pure frame computation functions extracted for unit testingAppDelegateSettingsCardRow,SettingsPickerRow,ShortcutSettingRow)popup-terminal.toggle,popup-terminal.show,popup-terminal.hidepopup-terminal-toggle,popup-terminal-show,popup-terminal-hideTesting
./scripts/reload.sh --tag popup-terminalxcodebuild -scheme cmux-unit)Manual test plan
Demo Video
CleanShot.2026-03-12.at.10.30.13.mp4
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Adds a global popup terminal toggled by F10 (or any remapped key) that slides in from any screen edge and stays independent of normal windows. Includes Preferences, socket/CLI commands, tests, and fixes for focus and hotkey behavior.
New Features
RegisterEventHotKey(default F10; remappable to any key with optional modifiers; F-keys allowed without modifiers; no Accessibility/Input Monitoring). Logs registration failures.NSPanelabove all apps; app switches to.accessoryon show and back on hide. Regular windows hide on show and aren’t restored by the hotkey; focus returns to the previous app on hide. Optional auto-hide on app switch.popup-terminal.toggle|show|hideand CLIpopup-terminal-toggle|show|hide. Shortcut handling adds keycode matching for function/special keys and supports non‑F keys. Unit tests for settings persistence and frame computation.Bug Fixes
hide().Written for commit 03230a2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Settings
Keyboard Shortcuts
CLI
Tests