Add default terminal registration - #4935
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 macOS default-terminal registration and UI, validates and routes terminal-document opens into seeded workspaces with initial terminal input, adds standard ssh:// URL parsing, updates Info.plist and localizations, integrates a command-palette action, and adds tests for SSH URLs and terminal file requests. ChangesDefault Terminal Registration and SSH URL Handling
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Settings as Settings UI
participant UserAction as DefaultTerminalUserAction
participant DefaultReg as DefaultTerminalRegistration
participant LaunchServices as Launch Services
participant Workspace as NSWorkspace
User->>Settings: Click "Set as Default Terminal"
Settings->>Settings: Show confirmation dialog
User->>Settings: Confirm
Settings->>UserAction: invoke setAsDefault()
UserAction->>DefaultReg: setAsDefault()
DefaultReg->>LaunchServices: LSRegisterURL / resolve handlers
DefaultReg->>Workspace: NSWorkspace.setDefaultApplication(for: ssh & UTTypes)
Workspace-->>DefaultReg: success or error
DefaultReg-->>UserAction: result
UserAction-->>Settings: post .defaultTerminalRegistrationDidChange / show alert on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (12 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR adds default terminal registration for cmux — registering it as the macOS handler for
Confidence Score: 3/5Safe to merge with one logic fix: the coalesced-caller path in registerAsDefault() can leave isSettingDefaultTerminal stuck in Settings after a successful registration. The registerAsDefault() deduplication always returns false for a second caller that coalesces onto an already-in-flight successful task. SettingsView's setDefaultTerminal() only clears isSettingDefaultTerminal in its own .do/.catch branches — the coalesced caller's early-return false path skips both, leaving the button stuck in 'Setting...' if the notification races ahead. Sources/AppDelegate+CmuxSSHURL.swift — the registerAsDefault() coalesced-caller return path; Sources/ContentView.swift — proactive currentStatus() IPC on every command-palette open. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Menu/Palette as Menu / Palette
participant DTUA as DefaultTerminalUserAction
participant DTR as DefaultTerminalRegistration
participant LS as LaunchServices
participant NC as NotificationCenter
participant UI as SettingsView / ContentView
User->>Menu/Palette: Click Make cmux Default Terminal
Menu/Palette->>DTUA: setAsDefault(debugSource:)
DTUA->>DTR: setAsDefault(bundleURL:) async throws
DTR->>LS: LSRegisterURL(bundleURL)
DTR->>LS: setDefaultApplication(ssh://)
DTR->>LS: setDefaultApplication(.shell-script)
DTR->>LS: setDefaultApplication(.unix-executable)
Note over DTR: defer fires
DTR->>NC: post(.defaultTerminalRegistrationDidChange)
NC-->>UI: onReceive refresh
DTR-->>DTUA: success / throw
alt error
DTUA->>User: NSAlert error
end
User->>LS: Opens ssh:// URL or .command/executable
LS->>AppDelegate: macOS routes to cmux
alt ssh:// URL
AppDelegate->>User: confirmCmuxSSHURLRequest modal
User-->>AppDelegate: confirmed
AppDelegate->>CmuxSSHURLProcessLauncher: start(request)
else .command / executable
AppDelegate->>TerminalDefaultFileOpenRequest: requests(from: externalFileURLs)
AppDelegate->>Workspace: addWorkspace(initialTerminalInput: shellQuotedPath)
end
Reviews (13): Last reviewed commit: "Handle stripped IPv6 hosts in SSH URLs" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+19
to
+26
| var errorDescription: String? { | ||
| switch self { | ||
| case .launchServicesRegistrationFailed(let status): | ||
| return "Launch Services registration failed with status \(status)." | ||
| case .missingContentType(let identifier): | ||
| return "macOS does not know the content type \(identifier)." | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
LocalizedError.errorDescription exposes implementation details
Both errorDescription values embed internal Apple-subsystem identifiers that the cmux error-copy rule disallows: "Launch Services" is an internal macOS subsystem name, the raw OSStatus integer is a vendor-specific numeric code, and the UTI string (e.g. com.apple.terminal.shell-script) is an internal type identifier. Because this type conforms to LocalizedError, any call site that reaches for error.localizedDescription or passes the error to Alert(title:message:error:) — including future callers — would expose these details directly to users. The current setDefaultTerminal() catch block happens to suppress them by showing a fixed localized string, but the conformance is a latent surface. Both descriptions should be rephrased in cmux terms (e.g. "cmux could not register as the default terminal app." / "A required file-type handler is unavailable on this system.").
Comment on lines
+5385
to
+5391
| @State private var defaultTerminalStatus = DefaultTerminalRegistrationStatus( | ||
| matchedTargetCount: 0, | ||
| targetCount: DefaultTerminalRegistration.targetCount | ||
| ) | ||
| @State private var isSettingDefaultTerminal = false | ||
| @State private var showSetDefaultTerminalConfirmation = false | ||
| @State private var defaultTerminalErrorMessage: String? |
Contributor
There was a problem hiding this comment.
cmuxApp.swift is currently 9 302 lines; the cmux file-boundary rule flags large additions to any file already past 800 lines. This PR adds another ~110 lines of new @State vars, computed subtitle/action-title properties, and a setDefaultTerminal() action — all logically cohesive as a self-contained "Default Terminal Settings" feature. Extracting DefaultTerminalSettingsRow (or equivalent view) into its own file would keep the feature independently readable and testable without touching the rest of SettingsView.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/SettingsNavigation.swift (2)
403-491: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd settings path anchor mapping for Default Terminal.
The
settingsPathAnchorIDsdictionary is missing a mapping for the Default Terminal setting path. Without it, references to this setting fromcmux.jsonor the command palette cannot navigate to the UI anchor.Add an entry following the existing pattern:
"app.defaultTerminal": settingID(for: .app, idSuffix: "default-terminal"),🤖 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/SettingsNavigation.swift` around lines 403 - 491, The settingsPathAnchorIDs dictionary is missing a mapping for the Default Terminal setting; add a new entry mapping the path "app.defaultTerminal" to the generated anchor using settingID(for: .app, idSuffix: "default-terminal") inside the settingsPathAnchorIDs constant (where other "app.*" entries are defined) so navigation from cmux.json/command palette can resolve the UI anchor.
290-395: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a dedicated setting entry for Default Terminal.
The
settingEntriesarray is missing an entry for the Default Terminal setting. Without it, users cannot jump directly to the setting row via search—they will only reach the App section.Add an entry following the existing pattern:
setting(.app, "default-terminal", String(localized: "settings.app.defaultTerminal", defaultValue: "Default Terminal"), "handler registration ssh links command tool unix executable launch services"),🤖 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/SettingsNavigation.swift` around lines 290 - 395, The settings search list (settingEntries) is missing an entry for the Default Terminal setting; add a new SettingsSearchEntry into the private static let settingEntries array using the same pattern as other entries by calling setting(.app, "default-terminal", String(localized: "settings.app.defaultTerminal", defaultValue: "Default Terminal"), "handler registration ssh links command tool unix executable launch services") so the search can jump directly to the Default Terminal row (locate where other .app entries are defined and insert this entry in the appropriate app section).
🤖 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 `@Resources/Info.plist`:
- Around line 29-48: Add the missing LSHandlerRank entries to the new handler
dictionaries: for the CFBundleTypeName "Terminal shell script" and "UNIX
executable" add LSHandlerRank with value "Alternate", and for the SSH URL
handler (the dict defining the SSH URL type) add LSHandlerRank with value
"Default"; locate these by their CFBundleTypeName/URL type strings and insert
the LSHandlerRank key/value alongside the existing CFBundleTypeRole and
LSItemContentTypes entries to match the pattern used by other handlers.
In `@Sources/AppDelegate`+CmuxSSHURL.swift:
- Line 44: currentStatus and setAsDefault use different URL normalization which
can misreport status when symlinks are present; change setAsDefault to use the
same helper normalizedApplicationURL (which calls standardizedFileURL and
resolvingSymlinksInPath) instead of only standardizedFileURL so both
currentStatus and setAsDefault normalize bundleURL identically; update all
places that compute normalizedBundleURL (including the other occurrence where
setAsDefault builds the URL) to call normalizedApplicationURL(bundleURL) and
keep standardizedFileURL + resolvingSymlinksInPath behavior centralized in
normalizedApplicationURL.
- Around line 19-26: The current errorDescription on the Error enum exposes
internal details (OSStatus and UTType identifier) in the
.launchServicesRegistrationFailed and .missingContentType cases; change the
user-facing strings returned by errorDescription to generic, non-technical
messages (e.g. "Failed to register application with the system." and
"Unsupported document type.") and remove interpolation of status/identifier
there, while preserving the original status/identifier for diagnostics/logging
elsewhere (e.g. include them in process logs or an internal debugDescription).
Update the switch in errorDescription to return only those friendly messages for
the symbols errorDescription, .launchServicesRegistrationFailed, and
.missingContentType.
In `@Sources/cmuxApp.swift`:
- Around line 6251-6253: refreshDefaultTerminalStatus currently updates
defaultTerminalStatus from DefaultTerminalRegistration.currentStatus() but
leaves defaultTerminalErrorMessage set from a prior failure; modify
refreshDefaultTerminalStatus to also clear defaultTerminalErrorMessage (set to
nil/empty) whenever you do a normal status refresh so stale errors don't
persist, and ensure any code that displays an immediate failure state still sets
defaultTerminalErrorMessage only when constructing/rendering that failure path
(leave the explicit failure-assignment locations unchanged). Also apply the same
clear-on-refresh change to the other similar refresh points referenced in the
review so they reset defaultTerminalErrorMessage when updating
defaultTerminalStatus.
In `@Sources/CmuxSSHURLRequest.swift`:
- Around line 242-251: The code builds a destination like "`@host`" when
components.user exists but trims to empty; change the logic so destination is
built only when the trimmed userValue is non-empty. Specifically, in the block
around components.user -> userValue, ensure the guards (guard
!userValue.isEmpty, !userValue.hasPrefix("-"), and isAllowedSSHUser(userValue))
are applied to the trimmed string and that destination uses only a non-empty
userValue (so use conditional creation when userValue is non-empty) rather than
userValue.map unconditionally; update references to userValue, destination,
normalizedHost, isAllowedSSHUser, and the error cases
(.destinationStartsWithDash, .destinationContainsUnsafeCharacters) accordingly.
---
Outside diff comments:
In `@Sources/SettingsNavigation.swift`:
- Around line 403-491: The settingsPathAnchorIDs dictionary is missing a mapping
for the Default Terminal setting; add a new entry mapping the path
"app.defaultTerminal" to the generated anchor using settingID(for: .app,
idSuffix: "default-terminal") inside the settingsPathAnchorIDs constant (where
other "app.*" entries are defined) so navigation from cmux.json/command palette
can resolve the UI anchor.
- Around line 290-395: The settings search list (settingEntries) is missing an
entry for the Default Terminal setting; add a new SettingsSearchEntry into the
private static let settingEntries array using the same pattern as other entries
by calling setting(.app, "default-terminal", String(localized:
"settings.app.defaultTerminal", defaultValue: "Default Terminal"), "handler
registration ssh links command tool unix executable launch services") so the
search can jump directly to the Default Terminal row (locate where other .app
entries are defined and insert this entry in the appropriate app section).
🪄 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: 9749e05f-b9e3-45fa-a059-23bceaa9e170
📒 Files selected for processing (10)
Resources/Info.plistResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/CmuxSSHURLRequest.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CmuxSSHURLRequestTests.swiftcmuxTests/WindowAndDragTests.swift
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 28, 2026 10:00
3c2edc1 to
3a27a6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
Resources/Info.plist (2)
101-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
LSHandlerRankto SSH URL type for consistency.The SSH URL handler is missing the
LSHandlerRankkey, which is present in both existing URL handlers (Web at line 81-82, Auth at line 94-95). Explicit declaration ensures cmux appears as a primary handler option for ssh:// links.📝 Proposed fix
<dict> <key>CFBundleTypeRole</key> <string>Viewer</string> <key>CFBundleURLName</key> <string>SSH URL</string> + <key>LSHandlerRank</key> + <string>Default</string> <key>CFBundleURLSchemes</key> <array> <string>ssh</string> </array> </dict>🤖 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 `@Resources/Info.plist` around lines 101 - 110, The SSH URL type entry (the <dict> containing CFBundleURLName "SSH URL" and CFBundleURLSchemes "ssh") is missing the LSHandlerRank key; add an <key>LSHandlerRank</key> with an appropriate value (e.g., <string>Owner</string> or matching the other handlers) inside that same dict so the SSH handler declares its rank consistently with the Web and Auth URL handler entries.
29-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
LSHandlerRankto new document types for consistency.The Terminal shell script and UNIX executable document type declarations are missing the
LSHandlerRankkey, which is present in the existing Folder document type (line 22-23). While the system may apply defaults, explicit declaration ensures predictable handler priority and consistency with the rest of the file.📝 Proposed fix
<dict> <key>CFBundleTypeName</key> <string>Terminal shell script</string> <key>CFBundleTypeRole</key> <string>Shell</string> + <key>LSHandlerRank</key> + <string>Alternate</string> <key>LSItemContentTypes</key> <array> <string>com.apple.terminal.shell-script</string> </array> </dict> <dict> <key>CFBundleTypeName</key> <string>UNIX executable</string> <key>CFBundleTypeRole</key> <string>Shell</string> + <key>LSHandlerRank</key> + <string>Alternate</string> <key>LSItemContentTypes</key> <array> <string>public.unix-executable</string> </array> </dict>🤖 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 `@Resources/Info.plist` around lines 29 - 48, Add the missing LSHandlerRank key to the two new CFBundleDocumentType dicts so they match the existing Folder entry; inside the dicts for CFBundleTypeName "Terminal shell script" and "UNIX executable" add an <key>LSHandlerRank</key> with a string value (e.g. "Owner") immediately alongside CFBundleTypeRole so the document types explicitly declare their handler rank.
🤖 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/AppDelegate`+CmuxSSHURL.swift:
- Around line 35-42: The bug is that contentTypeIdentifiers are counted and
treated as targets even when their UTType is unavailable, preventing
isDefault/currentStatus from ever reaching "default"; update targetCount to only
include contentTypeIdentifiers that resolve to an available UTType (e.g.,
map/filter using UTType(…) != nil or UTType.available), and change any logic in
currentStatus (and related checks around setAsDefault) to treat
unresolved/missing UTTypes as non-applicable (ignore them when comparing
registered handlers) so only supported identifiers are required for the
"default" outcome; ensure the same filtering approach is applied wherever
contentTypeIdentifiers is used (as noted for the other occurrences).
- Around line 67-93: The function setAsDefault can throw after making partial
Launch Services changes but currently posts
.defaultTerminalRegistrationDidChange only on the success path; add a defer
block near the start of setAsDefault that always posts the notification so
observers are notified even when a later setDefaultApplication call throws.
Because defer cannot await, use defer { Task { await MainActor.run {
NotificationCenter.default.post(name: .defaultTerminalRegistrationDidChange,
object: nil) } } } so the notification runs on the main actor regardless of
whether setAsDefault returns normally or throws.
In `@Sources/cmuxApp.swift`:
- Around line 259-261: The menu currently calls
DefaultTerminalUserAction.setAsDefault(...) which spawns a fresh Task each time
and allows overlapping NSWorkspace default-handler writes; move the in-flight
guard/serialization into the shared registration helper
(DefaultTerminalUserAction.setAsDefault or the helper it delegates to) by adding
a single shared synchronization primitive (e.g., an actor, serial DispatchQueue,
or an async Task/Bool guard stored on DefaultTerminalUserAction) that returns
the existing in-flight operation or awaits its completion before starting a new
registration; remove any per-caller guards so menu, palette, and Settings all
call the same serialized path and ensure the NSWorkspace write executes only
once at a time and errors/results are propagated back to callers.
In `@Sources/ContentView.swift`:
- Around line 6267-6270: Replace the inline call to
DefaultTerminalRegistration.currentStatus() inside snapshot.setBool for
CommandPaletteContextKeys.defaultTerminalIsDefault with a cached Boolean (e.g.
cachedDefaultTerminalIsDefault) stored on the ContentView (or the view-model
that builds the snapshot); initialize it once, update it on default-terminal
registration-status notifications (or when the command palette opens), and call
the existing snapshot-refresh code when the cache changes so the command-palette
snapshot uses the cached value instead of recomputing
DefaultTerminalRegistration.currentStatus() on every snapshot.
In `@Sources/SettingsSearchAliases.swift`:
- Line 46: Add the missing searchable setting and path anchor for the new
"app:default-terminal" alias: in SettingsNavigation.swift add a corresponding
setting entry to the settingEntries array (use the same pattern as other app
entries, e.g. setting(.app, idSuffix: "default-terminal", title: localized(...),
section: .app, ...)) so the setting is discoverable, and add a mapping in
settingsPathAnchorIDs mapping "app.defaultTerminal" to settingID(for: .app,
idSuffix: "default-terminal") so navigation anchors resolve correctly; ensure
keys and idSuffix exactly match the alias in SettingsSearchAliases.swift.
---
Duplicate comments:
In `@Resources/Info.plist`:
- Around line 101-110: The SSH URL type entry (the <dict> containing
CFBundleURLName "SSH URL" and CFBundleURLSchemes "ssh") is missing the
LSHandlerRank key; add an <key>LSHandlerRank</key> with an appropriate value
(e.g., <string>Owner</string> or matching the other handlers) inside that same
dict so the SSH handler declares its rank consistently with the Web and Auth URL
handler entries.
- Around line 29-48: Add the missing LSHandlerRank key to the two new
CFBundleDocumentType dicts so they match the existing Folder entry; inside the
dicts for CFBundleTypeName "Terminal shell script" and "UNIX executable" add an
<key>LSHandlerRank</key> with a string value (e.g. "Owner") immediately
alongside CFBundleTypeRole so the document types explicitly declare their
handler rank.
🪄 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: 14a678c1-5b5d-4bbe-8f75-f5169cecac3d
📒 Files selected for processing (11)
Resources/Info.plistResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/CmuxSSHURLRequest.swiftSources/ContentView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CmuxSSHURLRequestTests.swiftcmuxTests/WindowAndDragTests.swift
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 28, 2026 10:27
3a27a6b to
7eeda09
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 20748-20763: The two identical localization entries
dialog.defaultTerminal.setFailed.message and
settings.app.defaultTerminal.error.subtitle should be consolidated into a single
shared key (e.g., defaultTerminal.updateFailed.message) to avoid duplication;
update the Resources/Localizable.xcstrings to keep one key containing the
English and Japanese string values and remove the duplicate, then update any
code/UI references that use dialog.defaultTerminal.setFailed.message or
settings.app.defaultTerminal.error.subtitle to point to the new shared key
(ensure both en and ja entries are preserved under the new key).
In `@Sources/AppDelegate`+CmuxSSHURL.swift:
- Around line 187-197: The initializer init?(fileURL: URL, contentType: UTType?
= nil) (and the similar initializer around the 223–229 block) must explicitly
reject directories before falling back to treating names ending in
.command/.tool as terminal scripts: after computing standardizedURL (and before
calling Self.contentType(for:) / Self.shouldRunInTerminal), check that
standardizedURL is not a directory (e.g., standardizedURL.hasDirectoryPath or
FileManager fileExists(atPath:isDirectory:)) and return nil if it is; apply the
same directory check to the other initializer to prevent directory folders with
.command/.tool suffixes from being routed to the terminal.
In `@Sources/ContentView.swift`:
- Around line 6815-6838: Replace the hard-coded English keyword list for the
CommandPaletteCommandContribution (commandId "palette.makeDefaultTerminal") with
locale-aware strings: source each alias from localized resources using the same
localized API used for title/subtitle (e.g., String(localized: ...)) or a
localized comma/array entry, and populate the keywords array with those
localized strings so search aliases are translated; update the keywords property
in the CommandPaletteCommandContribution initializer to use those localized
entries instead of the current English literals.
🪄 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: 05c27646-1ddf-45f8-b077-cb89e1559794
📒 Files selected for processing (11)
Resources/Info.plistResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/CmuxSSHURLRequest.swiftSources/ContentView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CmuxSSHURLRequestTests.swiftcmuxTests/WindowAndDragTests.swift
Comment on lines
+20748
to
+20763
| "dialog.defaultTerminal.setFailed.message": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "macOS could not update every default terminal handler." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "macOS はすべてのデフォルトターミナルハンドラを更新できませんでした。" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider consolidating duplicate error messages.
The error message "macOS could not update every default terminal handler." appears in two separate keys:
dialog.defaultTerminal.setFailed.message(Line 20754)settings.app.defaultTerminal.error.subtitle(Line 55782)
Both English and Japanese translations are identical. If these serve the same purpose in different UI contexts, consider using a single shared key to reduce duplication and maintenance cost.
Also applies to: 55776-55792
🤖 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 `@Resources/Localizable.xcstrings` around lines 20748 - 20763, The two
identical localization entries dialog.defaultTerminal.setFailed.message and
settings.app.defaultTerminal.error.subtitle should be consolidated into a single
shared key (e.g., defaultTerminal.updateFailed.message) to avoid duplication;
update the Resources/Localizable.xcstrings to keep one key containing the
English and Japanese string values and remove the duplicate, then update any
code/UI references that use dialog.defaultTerminal.setFailed.message or
settings.app.defaultTerminal.error.subtitle to point to the new shared key
(ensure both en and ja entries are preserved under the new key).
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 28, 2026 10:50
7eeda09 to
093b45c
Compare
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 `@Sources/AppDelegate`+CmuxSSHURL.swift:
- Around line 122-163: registerAsDefault() currently deduplicates work but lets
every caller rethrow the shared failure; change its signature to return a Bool
(or similar flag) indicating whether the caller was the creator/owner of the
RegistrationOperation (based on inFlightRegistration?.id vs operation.id) and
only propagate/throw the error for that owner; non-owner callers should await
the shared task but return normally. Update registerAsDefault() (and its use of
RegistrationOperation and inFlightRegistration) to set and return the ownership
flag, and update setAsDefault(debugSource:) to call the new registerAsDefault(),
catching errors only when the returned flag indicates this caller is the owner
before calling presentSetAsDefaultError.
- Around line 165-175: presentSetAsDefaultError currently always shows a generic
informativeText; update it to check whether the passed Error is a
DefaultTerminalRegistrationError (or implements LocalizedError) and, if so, use
its errorDescription as the alert.informativeText (falling back to the existing
localized "defaultTerminal.updateFailed.message" when nil). Modify
presentSetAsDefaultError to cast the error and prefer
DefaultTerminalRegistrationError.errorDescription (or
error.localizedDescription) so the user sees the typed, localized failure
message for that specific registration error.
🪄 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: efc2ce37-ba1d-448b-ac73-f828fac71cb0
📒 Files selected for processing (11)
Resources/Info.plistResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/CmuxSSHURLRequest.swiftSources/ContentView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CmuxSSHURLRequestTests.swiftcmuxTests/WindowAndDragTests.swift
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 28, 2026 11:07
093b45c to
74e1ea6
Compare
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 `@Sources/AppDelegate`+CmuxSSHURL.swift:
- Around line 193-206: The initializer init?(fileURL:contentType:) should
resolve symlinks before the directory check and before storing fileURL so the
stored path is consistent with how
DefaultTerminalRegistration.normalizedApplicationURL treats symlinks; change use
of standardizedURL to a resolved URL (e.g., call resolvingSymlinksInPath() on
standardizedFileURL), perform resourceValues(forKeys: [.isDirectoryKey]) against
the resolved URL, and then set self.fileURL, self.workingDirectory and
self.initialInput using the resolved (not just standardized) URL while leaving
the rest of the logic (contentType resolution and shouldRunInTerminal call)
unchanged.
In `@Sources/cmuxApp.swift`:
- Around line 6262-6284: The confirm flow in setDefaultTerminal() can call
DefaultTerminalUserAction.registerAsDefault() even after
.defaultTerminalRegistrationDidChange has already made the app the default; to
fix, before invoking registerAsDefault() inside the Task check the current
registration state (for example by awaiting
DefaultTerminalUserAction.isRegistered() or a similar status query) and if it
reports already-registered skip calling registerAsDefault(), set
isSettingDefaultTerminal = false on the MainActor and call
refreshDefaultTerminalStatus() (or refreshDefaultTerminalStatus(clearError:
false) as appropriate) so the stale confirmation cannot re-trigger Launch
Services writes.
🪄 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: 87aea7e7-d2c0-4bcd-857f-5f5a0b0ef8e9
📒 Files selected for processing (11)
Resources/Info.plistResources/Localizable.xcstringsSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/CmuxSSHURLRequest.swiftSources/ContentView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CmuxSSHURLRequestTests.swiftcmuxTests/WindowAndDragTests.swift
Comment on lines
+193
to
+206
| init?(fileURL: URL, contentType: UTType? = nil) { | ||
| guard fileURL.isFileURL else { return nil } | ||
| let standardizedURL = fileURL.standardizedFileURL | ||
| let resourceValues = try? standardizedURL.resourceValues(forKeys: [.isDirectoryKey]) | ||
| guard resourceValues?.isDirectory != true else { return nil } | ||
| let resolvedContentType = contentType ?? Self.contentType(for: standardizedURL) | ||
| guard Self.shouldRunInTerminal(fileURL: standardizedURL, contentType: resolvedContentType) else { | ||
| return nil | ||
| } | ||
|
|
||
| self.fileURL = standardizedURL | ||
| self.workingDirectory = standardizedURL.deletingLastPathComponent().path(percentEncoded: false) | ||
| self.initialInput = "\(Self.shellSingleQuoted(standardizedURL.path(percentEncoded: false)))\n" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider resolving symlinks before the directory check.
standardizedFileURL normalizes the path but does not resolve symlinks. If a user opens a symlink pointing to a directory, the isDirectoryKey check on line 196-197 will inspect the symlink's target (which is correct), but the stored fileURL will be the symlink path rather than the resolved target. This is likely fine for execution purposes since the shell will follow the symlink, but it creates a minor inconsistency with how DefaultTerminalRegistration.normalizedApplicationURL handles symlinks.
If consistency is desired:
Optional: resolve symlinks before storing
init?(fileURL: URL, contentType: UTType? = nil) {
guard fileURL.isFileURL else { return nil }
- let standardizedURL = fileURL.standardizedFileURL
+ let standardizedURL = fileURL.standardizedFileURL.resolvingSymlinksInPath()
let resourceValues = try? standardizedURL.resourceValues(forKeys: [.isDirectoryKey])🤖 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/AppDelegate`+CmuxSSHURL.swift around lines 193 - 206, The initializer
init?(fileURL:contentType:) should resolve symlinks before the directory check
and before storing fileURL so the stored path is consistent with how
DefaultTerminalRegistration.normalizedApplicationURL treats symlinks; change use
of standardizedURL to a resolved URL (e.g., call resolvingSymlinksInPath() on
standardizedFileURL), perform resourceValues(forKeys: [.isDirectoryKey]) against
the resolved URL, and then set self.fileURL, self.workingDirectory and
self.initialInput using the resolved (not just standardized) URL while leaving
the rest of the logic (contentType resolution and shouldRunInTerminal call)
unchanged.
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 28, 2026 11:20
74e1ea6 to
35c89e8
Compare
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
May 28, 2026 11:34
Addressed on latest head; CodeRabbit status is passing/skipped for current commit.
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 07:40
2122006 to
5db2c61
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 07:45
5db2c61 to
7a8f959
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 07:50
7a8f959 to
2371417
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 07:57
2371417 to
44cf5be
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 08:07
44cf5be to
686f209
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 08:16
686f209 to
c239dc4
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 08:30
c239dc4 to
2d704bc
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 08:47
2d704bc to
858d0b4
Compare
lawrencecchen
force-pushed
the
feat-default-terminal
branch
from
May 30, 2026 08:56
858d0b4 to
abede0f
Compare
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 abede0f. Configure here.
Contributor
Author
|
Verified the Greptile summary concern about coalesced default-terminal registration callers. No code change needed: SettingsView.setDefaultTerminal() does not branch on registerAsDefault()'s Bool return. It always reaches the do block after the shared task completes and clears isSettingDefaultTerminal on MainActor, so a coalesced successful caller cannot leave the button stuck in Setting... |
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
May 30, 2026
- Keep workspace group creation in place (manaflow-ai#4989) - Allow narrower sidebar with titlebar accessories (manaflow-ai#5013) - Add beta TextBox defaults settings (manaflow-ai#4773) - Add default terminal registration (manaflow-ai#4935) - Make group new workspace placement configurable (manaflow-ai#5018) - Fix nightly publishing runner selection (manaflow-ai#5022) Conflicts resolved: - ContentView: take upstream's SidebarWorkspaceTopDropIndicator extraction. - cmuxApp: keep fork's QuickTerminal/WorkspaceTopTabsVisibility @AppStorage + TerminalCopyOnSelectSettings; adopt upstream's Setting(\.terminal.*) for textBoxMaxLines + showTextBoxOnNewTerminals + focusTextBoxOnNewTerminals. - xcstrings: keep both fork's settings.app.workspaceTopTabsVisibility[.*] keys and upstream's settings.app.workspaceGroupNewWorkspacePlacement. - TabManagerUnitTests: drop fork's testNewSurfaceCreatesAndFocusesTopLevelTab (upstream renames + retains coverage via testNewSurfaceFocusesCreatedSurface).
This branch was successfully 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
1 participant
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
Entitlements
Verification
Note
Medium Risk
Changes system default handlers and external open routing (ssh URLs and executables), which affects launch behavior app-wide though existing SSH confirmation flows largely remain.
Overview
This PR lets cmux register as macOS’s default terminal and handle the opens that go with it.
Launch Services & handlers:
Info.plistnow advertisessshURLs, terminal shell scripts (.command/.tool), and UNIX executables, with an imported UT type for shell scripts.Registration & UI: New
DefaultTerminalRegistration/DefaultTerminalUserActionlogic registers the app via Launch Services andNSWorkspace, tracks whether all targets point at cmux, and surfaces Settings → App → Default Terminal, an app menu item, and a command-palette action (hidden when already default), with confirmation, progress, and error strings (EN/JA).Runtime behavior: Standard
ssh://URLs are parsed inCmuxSSHURLRequest(IPv6, ports, validation).TerminalDefaultFileOpenRequestturns script/executable opens into workspaces with a shell-quoted path as initial terminal input;application(_:open:)splits those from normal file previews and skips opening the app bundle’s own executable as a workspace.Tests: Coverage for standard SSH URL parsing, default-terminal registration targets, and terminal file open requests.
Reviewed by Cursor Bugbot for commit 8320e99. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Settings & Search
Localization
Tests