Add quick terminal - #4830
Add quick terminal#4830austinywang wants to merge 21 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Quick Terminal panel feature: a new ChangesQuick Terminal Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant SystemWideHotkey as SystemWideHotkeyController
participant AppDelegate
participant QuickTerminalController
participant TerminalSurface
User->>SystemWideHotkey: ⌥⌘` pressed
SystemWideHotkey->>AppDelegate: toggleQuickTerminalVisibility(activateApp: true)
AppDelegate->>QuickTerminalController: toggle()
alt currently hidden
QuickTerminalController->>QuickTerminalController: ensurePanel() – lazy init
QuickTerminalController->>TerminalSurface: setVisibility(true), becomeActive
QuickTerminalController->>QuickTerminalController: animate frame + alpha to finalFrame
QuickTerminalController-->>AppDelegate: visible
else currently visible
QuickTerminalController->>QuickTerminalController: animate frame + alpha to hiddenFrame
QuickTerminalController->>TerminalSurface: setVisibility(false), resignActive
QuickTerminalController->>QuickTerminalController: finishHide() – orderOut, restore app
QuickTerminalController-->>AppDelegate: hidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 21 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (21 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryAdds a Ghostty-style Quick Terminal floating panel (
Confidence Score: 4/5Safe to merge with one small localization gap to resolve before shipping to non-English/Japanese users. The core implementation — actor isolation, animation lifecycle, screen-safety, port ordinal allocation, hotkey routing — is solid and the previous-thread concerns have all been addressed. The only remaining gap is that the four settings search alias keys (settings.search.alias.setting.terminal.quick-terminal-*) landed with only en+ja translations while every adjacent key in the same catalog section carries 20 locales. This doesn't break functionality, but it means users in 18 other locales won't find the quick terminal settings via locale-specific search terms. Resources/Localizable.xcstrings — the four settings.search.alias.setting.terminal.quick-terminal-* entries need the remaining 18 locale translations added. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[⌥⌘` Global Hotkey] --> T[toggleQuickTerminalVisibility]
B[View Menu] --> T
C[Command Palette] --> T
D[Socket: quick_terminal.toggle] --> T
E[CLI: cmux quick-terminal] --> D
T --> QTC[QuickTerminalController.toggle]
QTC -->|isTransitioning| PQ[pendingTransitionAction queue]
QTC -->|not visible| SHOW[show]
QTC -->|visible| HIDE[hide]
SHOW --> EP[ensurePanel / TerminalSurface]
EP --> PO[TabManager.allocatePortOrdinal]
SHOW --> ANIM_IN[NSAnimationContext slide-in]
ANIM_IN --> VIS[phase = .visible]
VIS --> FOCUS[makeFirstResponder terminalSurface]
HIDE --> ANIM_OUT[NSAnimationContext slide-out]
ANIM_OUT --> FH[finishHide]
FH --> RESTORE[activate previousFrontmostApp?]
FH --> PR[replayPendingTransitionAction]
VIS -->|windowDidResignKey + autoHide| HIDE
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[⌥⌘` Global Hotkey] --> T[toggleQuickTerminalVisibility]
B[View Menu] --> T
C[Command Palette] --> T
D[Socket: quick_terminal.toggle] --> T
E[CLI: cmux quick-terminal] --> D
T --> QTC[QuickTerminalController.toggle]
QTC -->|isTransitioning| PQ[pendingTransitionAction queue]
QTC -->|not visible| SHOW[show]
QTC -->|visible| HIDE[hide]
SHOW --> EP[ensurePanel / TerminalSurface]
EP --> PO[TabManager.allocatePortOrdinal]
SHOW --> ANIM_IN[NSAnimationContext slide-in]
ANIM_IN --> VIS[phase = .visible]
VIS --> FOCUS[makeFirstResponder terminalSurface]
HIDE --> ANIM_OUT[NSAnimationContext slide-out]
ANIM_OUT --> FH[finishHide]
FH --> RESTORE[activate previousFrontmostApp?]
FH --> PR[replayPendingTransitionAction]
VIS -->|windowDidResignKey + autoHide| HIDE
Reviews (13): Last reviewed commit: "fix: redact CLI argument errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the Greptile summary localization note in f12cc86: the 19 new Quick Terminal string-catalog keys now include the full supported locale set, with English fallbacks marked needs_review for non-English/Japanese locales and the Japanese strings preserved. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 8: The new Quick Terminal bullet was added to CHANGELOG.md but you must
also add matching localized changelog entries for every locale defined in
web/i18n/routing.ts; update each locale's localized changelog file (or
localization mapping) with a translated/identical entry for "Quick Terminal
panel with a configurable position/size, Option-Command-backtick shortcut,
command palette action, and `cmux quick-terminal` CLI controls" so the localized
changelog coverage matches the CHANGELOG.md addition and the locales referenced
in web/i18n/routing.ts.
In `@CLI/cmux.swift`:
- Around line 4767-4769: The error currently interpolates and prints the raw
unexpected argument value from remaining.first into CLIError (the string
"quick-terminal \(subcommand): unexpected argument '\(unknown)'"), which can
leak secrets; change this to avoid echoing the raw value by using a sanitized
placeholder or redaction (e.g., "<redacted>" or show only a safe,
truncated/sanitized preview), or report the presence/count of unexpected
arguments instead; update the code that constructs the CLIError message (the
branch that checks remaining.first and creates CLIError) to use the
redacted/safe text rather than the raw unknown variable.
In `@Sources/TerminalController.swift`:
- Around line 11006-11030: The RPC error leaks the internal symbol "AppDelegate"
and conflates a missing delegate with an action failure; update
v2QuickTerminalShow, v2QuickTerminalHide (and the toggle handler if present) to
first check whether AppDelegate.shared is nil and return a
non-implementation-specific error (e.g., code "unavailable" or
"service_unavailable" with a generic message like "service not available") when
the delegate is nil, and otherwise, when the delegate exists but the action
returns false, return a distinct error (e.g., code "action_failed" with message
like "could not show/hide quick terminal") so clients can distinguish
availability from action failure without seeing internal class names. Ensure the
returned messages do not reference "AppDelegate" and use v2QuickTerminalStatus()
only on success.
In `@web/data/cmux.schema.json`:
- Around line 378-402: The new schema properties quickTerminalPosition,
quickTerminalPrimarySizeRatio, quickTerminalSecondarySizeRatio, and
quickTerminalAutoHide currently use English-only description strings; replace
each "description" value with a "descriptionKey" (unique message keys, e.g.
schema.quickTerminal.position, schema.quickTerminal.primarySizeRatio,
schema.quickTerminal.secondarySizeRatio, schema.quickTerminal.autoHide) and add
corresponding translated messages for those keys into every locale file listed
in web/i18n/routing.ts so all docs locales have coverage.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d60e8b7-aa9f-46bc-b3f0-bfa3701ffeb0
📒 Files selected for processing (20)
CHANGELOG.mdCLI/cmux.swiftREADME.mdResources/Localizable.xcstringsSources/App/ShortcutBareStartRouting.swiftSources/AppDelegate.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/QuickTerminalController.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GlobalSearchShortcutSettingsTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux-shortcuts.tsweb/data/cmux.schema.json
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0b9e4ec. Configure here.
PR manaflow-ai#4828 main-window visibility controller renamed to MainWindowQuickTerminalController/Position to avoid name collision with PR manaflow-ai#4830's QuickTerminalController (the floating panel with the global hotkey). AppDelegate now hosts both controllers via separate lazy vars; teardown/toggle/statusPayload route to the PR manaflow-ai#4830 controller, while restoreSession/snapshot/closeShortcut route to the PR manaflow-ai#4828 controller. Default Quick Terminal hotkey: Option+Cmd+\`
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CmuxSettings/Sources/CmuxSettings/Keys/TerminalCatalogSection.swift`:
- Around line 53-75: Add Swift-DocC triple-slash comments for the four new
public DefaultsKey properties (quickTerminalPosition,
quickTerminalPrimarySizeRatio, quickTerminalSecondarySizeRatio,
quickTerminalAutoHide); each comment should include a one-line summary, valid
values/ranges (e.g., allowed position strings, 0.0–1.0 ranges for ratios,
boolean behavior), any relevant notes about interaction (which ratio is
primary/secondary and when secondary is ignored), and a brief example/usage
line. Place the comments immediately above the declarations of
quickTerminalPosition, quickTerminalPrimarySizeRatio,
quickTerminalSecondarySizeRatio, and quickTerminalAutoHide following the
project's DocC style (triple-slash ///) and include defaults where appropriate.
- Around line 53-75: The catalog ids for quickTerminalPosition,
quickTerminalPrimarySizeRatio, quickTerminalSecondarySizeRatio, and
quickTerminalAutoHide use the "terminal.quickTerminal*" namespace but their
userDefaultsKey values drop the "terminal." prefix, causing an inconsistent
mapping; update each DefaultsKey's userDefaultsKey to exactly match its id
(e.g., change "quickTerminal.position" to "terminal.quickTerminalPosition",
"quickTerminal.primarySizeRatio" to "terminal.quickTerminalPrimarySizeRatio",
etc.), or if the differing namespace was intentional, add a brief inline comment
above these four keys explaining why the userDefaultsKey intentionally differs
from the catalog id.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9d2c8b5-c5d5-4902-92b2-ed1e36f575a2
📒 Files selected for processing (5)
CHANGELOG.mdCLI/cmux.swiftPackages/CmuxSettings/Sources/CmuxSettings/Keys/TerminalCatalogSection.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift
💤 Files with no reviewable changes (3)
- Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swift
- Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift
- CLI/cmux.swift
![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 50 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…upport-quick-terminal # Conflicts: # Sources/CmuxSettingsJSONPathSupport.swift # Sources/TabManager.swift # Sources/TerminalController.swift
…upport-quick-terminal
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/QuickTerminalController.swift (1)
171-178: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDocument or extract the magic offset constant.
The
+ 18offset for the center position's hidden frame is unexplained. Consider extracting to a named constant or adding a comment explaining its purpose (e.g., if it relates to menu bar clearance or visual aesthetics).case .center: + // Small offset above screen to ensure panel is fully off-screen during hide + let centerHiddenOffset: CGFloat = 18 return NSRect( x: finalFrame.origin.x, - y: visibleFrame.maxY + 18, + y: visibleFrame.maxY + centerHiddenOffset, width: finalFrame.width, height: finalFrame.height )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/QuickTerminalController.swift` around lines 171 - 178, The magic number 18 in the y-position calculation for the center case position is unexplained and makes the code harder to understand. Extract this constant to a named property at the class or file level with a descriptive name that explains its purpose (such as an offset for menu bar clearance or spacing), and then replace the hardcoded 18 with a reference to that named constant. Alternatively, if a constant is not appropriate, add an inline comment above or next to the + 18 offset explaining what it represents and why that specific value is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 13304-13306: Add the `private` access modifier to the three `@State`
properties workspaceFinderDirectoryOpenRequest, metadataRowsExpanded, and
metadataBlocksExpanded in ContentView.swift to follow SwiftUI conventions and
resolve the SwiftLint warning. If these properties require access from external
code for testing or initialization purposes, you may keep them as
package-internal; otherwise, restore the private access modifier to each
declaration.
---
Outside diff comments:
In `@Sources/QuickTerminalController.swift`:
- Around line 171-178: The magic number 18 in the y-position calculation for the
center case position is unexplained and makes the code harder to understand.
Extract this constant to a named property at the class or file level with a
descriptive name that explains its purpose (such as an offset for menu bar
clearance or spacing), and then replace the hardcoded 18 with a reference to
that named constant. Alternatively, if a constant is not appropriate, add an
inline comment above or next to the + 18 offset explaining what it represents
and why that specific value is used.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 482b6440-aa8d-4206-98ec-a38ba82c8859
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/QuickTerminalController.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/QuickTerminalController.swift (1)
171-178: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDocument or extract the magic offset constant.
The
+ 18offset for the center position's hidden frame is unexplained. Consider extracting to a named constant or adding a comment explaining its purpose (e.g., if it relates to menu bar clearance or visual aesthetics).case .center: + // Small offset above screen to ensure panel is fully off-screen during hide + let centerHiddenOffset: CGFloat = 18 return NSRect( x: finalFrame.origin.x, - y: visibleFrame.maxY + 18, + y: visibleFrame.maxY + centerHiddenOffset, width: finalFrame.width, height: finalFrame.height )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/QuickTerminalController.swift` around lines 171 - 178, The magic number 18 in the y-position calculation for the center case position is unexplained and makes the code harder to understand. Extract this constant to a named property at the class or file level with a descriptive name that explains its purpose (such as an offset for menu bar clearance or spacing), and then replace the hardcoded 18 with a reference to that named constant. Alternatively, if a constant is not appropriate, add an inline comment above or next to the + 18 offset explaining what it represents and why that specific value is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 13304-13306: Add the `private` access modifier to the three `@State`
properties workspaceFinderDirectoryOpenRequest, metadataRowsExpanded, and
metadataBlocksExpanded in ContentView.swift to follow SwiftUI conventions and
resolve the SwiftLint warning. If these properties require access from external
code for testing or initialization purposes, you may keep them as
package-internal; otherwise, restore the private access modifier to each
declaration.
---
Outside diff comments:
In `@Sources/QuickTerminalController.swift`:
- Around line 171-178: The magic number 18 in the y-position calculation for the
center case position is unexplained and makes the code harder to understand.
Extract this constant to a named property at the class or file level with a
descriptive name that explains its purpose (such as an offset for menu bar
clearance or spacing), and then replace the hardcoded 18 with a reference to
that named constant. Alternatively, if a constant is not appropriate, add an
inline comment above or next to the + 18 offset explaining what it represents
and why that specific value is used.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 482b6440-aa8d-4206-98ec-a38ba82c8859
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/QuickTerminalController.swiftcmux.xcodeproj/project.pbxproj
🛑 Comments failed to post (1)
Sources/ContentView.swift (1)
13304-13306: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
SwiftLint warns that
@Stateproperties should be private.Three
@Stateproperties were changed fromprivateto package-internal:
workspaceFinderDirectoryOpenRequestmetadataRowsExpandedmetadataBlocksExpandedIf these properties need broader access for testing or initialization, this is acceptable. Otherwise, restore
privateaccess to follow SwiftUI conventions.🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 13304-13304: SwiftUI state properties should be private
(private_swiftui_state)
[Warning] 13305-13305: SwiftUI state properties should be private
(private_swiftui_state)
[Warning] 13306-13306: SwiftUI state properties should be private
(private_swiftui_state)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/ContentView.swift` around lines 13304 - 13306, Add the `private` access modifier to the three `@State` properties workspaceFinderDirectoryOpenRequest, metadataRowsExpanded, and metadataBlocksExpanded in ContentView.swift to follow SwiftUI conventions and resolve the SwiftLint warning. If these properties require access from external code for testing or initialization purposes, you may keep them as package-internal; otherwise, restore the private access modifier to each declaration.Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
7551-7560:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop echoing raw user input in new CLI error messages.
Several new error paths include unredacted user-provided values (unknown args and
KEY=VALUEentries). That can leak secrets into shell history/logs.As per coding guidelines, user-facing errors must not expose credentials/tokens or unredacted payload-like values.
Suggested minimal hardening
-throw CLIError(message: String( - format: String( - localized: "cli.workspace.create.error.unknownFlag", - defaultValue: "%@: unknown flag '%@'. Known flags: ..." - ), - locale: .current, - commandName, - unknown -)) +throw CLIError(message: String( + format: String( + localized: "cli.workspace.create.error.unknownFlag", + defaultValue: "%@: unknown flag. Known flags: ..." + ), + locale: .current, + commandName +)) -throw CLIError(message: String( - format: String( - localized: "cli.workspace.env.error.invalidAssignment", - defaultValue: "%@: %@ entry '%@' must be in KEY=VALUE form" - ), - locale: .current, - commandName, - source, - raw -)) +throw CLIError(message: String( + format: String( + localized: "cli.workspace.env.error.invalidAssignment", + defaultValue: "%@: %@ entry must be in KEY=VALUE form" + ), + locale: .current, + commandName, + source +)) -throw CLIError(message: "ssh-tmux: unexpected extra argument '\(arg)'") +throw CLIError(message: "ssh-tmux: unexpected extra argument")Also applies to: 7698-7721, 7764-7772, 8837-8841
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 7551 - 7560, The error message in the CLIError being thrown is echoing back the unknown flag provided by the user, which can leak sensitive information into shell history. Remove the reference to the `unknown` variable from the error message and replace it with a generic indicator (such as "unknown flag provided") that does not expose the actual user input. Apply this same redaction approach to all other error paths mentioned in the comment (7698-7721, 7764-7772, 8837-8841) where user-provided values like unknown arguments or KEY=VALUE entries are being directly included in error messages.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 7774-7781: The code currently extracts only the first positional
argument using first(where:) on rem1 while silently ignoring any additional
positional arguments that don't start with "--". This can lead to unexpected
behavior where users provide multiple positional arguments but only the first is
used. After extracting the positional argument from rem1, validate that there
are no additional positional arguments by checking if there are other elements
in rem1 that don't start with "--", and if extra positional arguments are found,
return an error rejecting them rather than silently ignoring them.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 7551-7560: The error message in the CLIError being thrown is
echoing back the unknown flag provided by the user, which can leak sensitive
information into shell history. Remove the reference to the `unknown` variable
from the error message and replace it with a generic indicator (such as "unknown
flag provided") that does not expose the actual user input. Apply this same
redaction approach to all other error paths mentioned in the comment (7698-7721,
7764-7772, 8837-8841) where user-provided values like unknown arguments or
KEY=VALUE entries are being directly included in error messages.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78667f2f-3927-49e8-8ff9-f8a003dd1243
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
CHANGELOG.mdCLI/cmux.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
7551-7560:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop echoing raw user input in new CLI error messages.
Several new error paths include unredacted user-provided values (unknown args and
KEY=VALUEentries). That can leak secrets into shell history/logs.As per coding guidelines, user-facing errors must not expose credentials/tokens or unredacted payload-like values.
Suggested minimal hardening
-throw CLIError(message: String( - format: String( - localized: "cli.workspace.create.error.unknownFlag", - defaultValue: "%@: unknown flag '%@'. Known flags: ..." - ), - locale: .current, - commandName, - unknown -)) +throw CLIError(message: String( + format: String( + localized: "cli.workspace.create.error.unknownFlag", + defaultValue: "%@: unknown flag. Known flags: ..." + ), + locale: .current, + commandName +)) -throw CLIError(message: String( - format: String( - localized: "cli.workspace.env.error.invalidAssignment", - defaultValue: "%@: %@ entry '%@' must be in KEY=VALUE form" - ), - locale: .current, - commandName, - source, - raw -)) +throw CLIError(message: String( + format: String( + localized: "cli.workspace.env.error.invalidAssignment", + defaultValue: "%@: %@ entry must be in KEY=VALUE form" + ), + locale: .current, + commandName, + source +)) -throw CLIError(message: "ssh-tmux: unexpected extra argument '\(arg)'") +throw CLIError(message: "ssh-tmux: unexpected extra argument")Also applies to: 7698-7721, 7764-7772, 8837-8841
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 7551 - 7560, The error message in the CLIError being thrown is echoing back the unknown flag provided by the user, which can leak sensitive information into shell history. Remove the reference to the `unknown` variable from the error message and replace it with a generic indicator (such as "unknown flag provided") that does not expose the actual user input. Apply this same redaction approach to all other error paths mentioned in the comment (7698-7721, 7764-7772, 8837-8841) where user-provided values like unknown arguments or KEY=VALUE entries are being directly included in error messages.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 7774-7781: The code currently extracts only the first positional
argument using first(where:) on rem1 while silently ignoring any additional
positional arguments that don't start with "--". This can lead to unexpected
behavior where users provide multiple positional arguments but only the first is
used. After extracting the positional argument from rem1, validate that there
are no additional positional arguments by checking if there are other elements
in rem1 that don't start with "--", and if extra positional arguments are found,
return an error rejecting them rather than silently ignoring them.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 7551-7560: The error message in the CLIError being thrown is
echoing back the unknown flag provided by the user, which can leak sensitive
information into shell history. Remove the reference to the `unknown` variable
from the error message and replace it with a generic indicator (such as "unknown
flag provided") that does not expose the actual user input. Apply this same
redaction approach to all other error paths mentioned in the comment (7698-7721,
7764-7772, 8837-8841) where user-provided values like unknown arguments or
KEY=VALUE entries are being directly included in error messages.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78667f2f-3927-49e8-8ff9-f8a003dd1243
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
CHANGELOG.mdCLI/cmux.swift
🛑 Comments failed to post (1)
CLI/cmux.swift (1)
7774-7781:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject extra positional arguments in
workspace env.The command currently picks the first positional handle and silently ignores additional positional args, which can target the wrong workspace unexpectedly.
Suggested fix
- let positional = rem1.first(where: { !$0.hasPrefix("--") }) + let positionals = rem1.filter { !$0.hasPrefix("--") } + if positionals.count > 1 { + throw CLIError(message: "workspace env: unexpected extra argument") + } + let positional = positionals.first📝 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.let positionals = rem1.filter { !$0.hasPrefix("--") } if positionals.count > 1 { throw CLIError(message: "workspace env: unexpected extra argument") } let positional = positionals.first let windowRaw = windowFromArgsOrOverride(commandArgs, windowOverride: windowOverride) // Match reconnect/disconnect: default to the caller's workspace // ($CMUX_WORKSPACE_ID) before the selected one, but only when no explicit // --window is given (the caller's workspace may live in another window). let target = workspaceArg ?? positional ?? (windowRaw == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 7774 - 7781, The code currently extracts only the first positional argument using first(where:) on rem1 while silently ignoring any additional positional arguments that don't start with "--". This can lead to unexpected behavior where users provide multiple positional arguments but only the first is used. After extracting the positional argument from rem1, validate that there are no additional positional arguments by checking if there are other elements in rem1 that don't start with "--", and if extra positional arguments are found, return an error rejecting them rather than silently ignoring them.
|
Chiming in from #4828 (I left a note there about the fullscreen-overlay gap) — this PR's dedicated I've been dogfooding a quick terminal built on the same non-activating-panel recipe for ~3 weeks, including over third-party native-fullscreen Spaces (branch for reference: https://github.com/Caligone/cmux/tree/quick-term-on-main). A few field notes that may save a round-trip, since I hit all of these empirically:
Happy to share more from the branch if useful (per-display remembered height, the |
|
Is there any timeline when this would be available in cmux? This is currently the last feature keeping me from making cmux my daily driver. Thanks for your work! |
|
cmux-reconcile: useful Usefulness verdict: Use as the primary Quick Terminal reconciliation candidate; resolve overlap with #1996 before implementation review. The inspected diff includes package settings/UI, shortcut/menu/CLI integration, and coverage, whereas the current inspected main CLI does not expose Reviewed patch head: Older issue/PR tracking index — remaining scope and competing implementations are recorded there. |
|
Mac fleet instructions for head JOB_JSON=$(~/.local/bin/cmux-ci submit --kind cmux --command 'CMUX_FLEET_BUILD_TAG=pr-4830-eb674c32 /Users/Shared/cmux-build-fleet/recipes/cmux.sh https://github.com/manaflow-ai/cmux.git eb674c323ca815ba4b0516993dfdea1b54af5775' --artifact artifacts/cmux.app.zip --workspace https://github.com/manaflow-ai/cmux/pull/4830 --source-digest eb674c323ca815ba4b0516993dfdea1b54af5775 --cache-key cmux:pr-4830 --min-free-bytes 268435456000 --label cmux --label ram48)
JOB_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["id"])' <<<"$JOB_JSON")
~/.local/bin/cmux-ci wait "$JOB_ID" --receipt artifacts/fleet/$JOB_ID.json
~/.local/bin/cmux-ci publish-hq "$JOB_ID"Use an existing campaign job ID if one is already posted; do not submit a duplicate. A wait timeout leaves the remote job running. Published results will include an exact-head artifact link and timing/disk receipt. This recipe validates the macOS app only, not iOS or tests. Never use maclease or put credentials in a PR comment. |
|
Quick Terminal has continued through later main changes and this branch is superseded by the current implementation. |
This branch was successfully deployed
5 participants
Summary
cmux quick-terminalCLI.Fixes #327
Related: #1523, #1712
Regression coverage
Verification
git diff --checkResources/Localizable.xcstringsandweb/data/cmux.schema.jsonLocal builds/tests were not run per task instructions; HQ should run:
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-327-does-cmux-support-quick-terminal --launchNeed help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Introduces a system-wide hotkey, floating window focus/restore behavior, and a separate terminal surface with shared port allocation—moderate UX and integration risk, not security-critical.
Overview
Adds a Quick Terminal: a floating Ghostty terminal panel you can summon from anywhere on macOS, with slide-in animation, optional auto-hide on focus loss, and placement/size controlled via settings or
cmux.json.Controls: system-wide **⌥⌘
** (registered like global search), in-app shortcut, View menu, command palette, socket methodsquick_terminal.*, and **cmux quick-terminal** (toggle/show/hide/status`). New settings cover position (top/bottom/left/right/center), primary/secondary size ratios, and auto-hide; docs, schema, shortcuts page, and changelog are updated.Implementation notes:
QuickTerminalControllerhosts a dedicatedTerminalSurface(CMUX_QUICK_TERMINAL=1) in a borderless panel; port ordinals use sharedTabManager.allocatePortOrdinal()so quick terminal ports don’t collide with workspace ranges. Shortcut routing excludes quick terminal from bare-start/chord paths that would break global hotkeys.Reviewed by Cursor Bugbot for commit 6520b9b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a Ghostty-style Quick Terminal you can drop over any app with a system-wide ⌥⌘` hotkey, View menu, command palette, CLI, or socket API. It’s configurable (position, size ratios, auto-hide) and wired into settings, docs, and tests. Fixes #327.
New Features
(configurable), View menu, command palette; CLIcmux quick-terminal [toggle|show|hide|status]with--json; socket APIquick_terminal.toggle|show|hide|status`.quickTerminalPosition,quickTerminalPrimarySizeRatio,quickTerminalSecondarySizeRatio,quickTerminalAutoHideadded to Settings UI,cmux.schema.json, README, examples, and localized shortcuts.Bug Fixes
TabManager.allocatePortOrdinal()to prevent overlapping port ranges across windows..toggleQuickTerminal(and.globalSearch) from bare-start routing to keep system-wide hotkeys reliable; added localized settings search aliases; redacted CLI argument errors.Written for commit eb674c3. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
cmux quick-terminalCLI commands (toggle/show/hide/status).Documentation
Tests