Repository navigation
Add cmux browser disable switch - #3256
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 centralized browser-availability toggle (UserDefaults + notification) controllable via CLI, Settings, and the shell wrapper; when disabled, browser creation and embedded routing short‑circuit across wrapper, app UI, RPC, and panels to fall back to the system browser or return external-open metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User / CLI
participant OpenScript as Resources/bin/open
participant CmuxRPC as cmux App (RPC)
participant Settings as BrowserAvailabilitySettings
participant System as System Browser (NSWorkspace)
User->>OpenScript: run open <url>
OpenScript->>Settings: read `browserDisabledOverride`
alt override == true
OpenScript->>System: system `open` <url> (bypass cmux)
System-->>OpenScript: exit status
OpenScript-->>User: exit status
else override != true
OpenScript->>CmuxRPC: forward to cmux
CmuxRPC->>Settings: isDisabled()
alt disabled == true
CmuxRPC->>System: NSWorkspace.open(<url>) (fallback)
CmuxRPC-->>User: respond ok + metadata (browser_disabled/opened_externally)
else disabled == false
CmuxRPC->>CmuxRPC: create browser surface/tab/split
CmuxRPC-->>User: respond ok (embedded browser created)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds a global "browser disabled" override that routes all terminal links, intercepted
Confidence Score: 3/5Not safe to merge as-is — the missing One clear P1 defect (double URL open in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[URL open request] --> B{Source}
B -->|bin/open script| C{settings_domain set?}
C -->|yes| D{browserDisabledOverride = true?}
D -->|yes| E[system_open ALL args]
E -->|⚠️ no exit — falls through| F{open_in_cmux setting}
D -->|no| F
F -->|false| G[system_open]
F -->|true / unset| H[cmux browser open URL]
H -->|browser disabled: error| I[failed_urls list]
I --> J[system_open failed URLs ← 2nd open!]
B -->|CLI socket| K{BrowserAvailabilitySettings.isEnabled?}
K -->|no| L[return browser_disabled error / NSWorkspace.open]
K -->|yes| M[Create browser panel in app]
B -->|Command Palette| N{browserDisabled context key}
N -->|true| O[browser commands hidden]
N -->|false| P[browser commands shown]
B -->|Settings toggle| Q[BrowserAvailabilitySettings.setDisabled]
Q --> R[UserDefaults write + notification]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: e7aed0d7bb
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Sources/AppDelegate.swift (2)
10976-10986: Optional security hardening:redactedDebugURLstill includes URL path.
redactedDebugURL(_:)clearsuser,password,query, andfragment, but it leavespath. In some environments, the path can still contain sensitive tokens/identifiers, so the “blocked browser disabled” DEBUG log could leak more than intended.Optional tightening: consider logging only scheme + host (and maybe port), or replacing path with a constant like
"/<redacted>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 10976 - 10986, The redactedDebugURL(_:) function currently strips user, password, query, and fragment but leaves the URL path which can leak sensitive tokens; update redactedDebugURL(_:) to also remove or replace the path (e.g., set components.path = "/<redacted>" or empty) and ensure scheme, host, and port (if present) are retained; also handle URLs that lack a host by returning a safe placeholder like "<redacted>" or "<invalid>" so no sensitive path data is ever included in logs.
10446-10761: Potential UX/behavior bug:.focusBrowserAddressBarshortcut may fall through when browser is disabled.Right now, the handler only returns
trueifopenBrowserAndFocusAddressBar(...) != nil. When the browser is disabled,openBrowserAndFocusAddressBar(...)will returnnil, and the function will likely continue checking later shortcuts / responder behavior instead of deterministically consuming the matched shortcut.Suggested behavior: if the shortcut is matched and the browser is disabled, consume it (return
true), so the keybinding doesn’t “act like unhandled”.🛠️ Proposed fix
if matchConfiguredShortcut(event: event, action: .focusBrowserAddressBar) { if let focusedPanel = tabManager?.focusedBrowserPanel { focusBrowserAddressBar(in: focusedPanel) return true } if let browserAddressBarFocusedPanelId, focusBrowserAddressBar(panelId: browserAddressBarFocusedPanelId) { return true } - if openBrowserAndFocusAddressBar(insertAtEnd: true) != nil { - return true - } + let openedPanelId = openBrowserAndFocusAddressBar(insertAtEnd: true) + if openedPanelId != nil { + return true + } + // If the browser feature is disabled, still consume the shortcut so it doesn't fall through. + if !BrowserAvailabilitySettings.isEnabled() { + return true + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 10446 - 10761, The .focusBrowserAddressBar branch can match but currently only returns true when focus succeeds; change the logic in the matchConfiguredShortcut(event: event, action: .focusBrowserAddressBar) block so that after the existing early-success returns (focusedBrowserPanel, browserAddressBarFocusedPanelId, openBrowserAndFocusAddressBar != nil) you always consume the shortcut (return true) even when openBrowserAndFocusAddressBar(...) returns nil (i.e., browser disabled). Update the block that references matchConfiguredShortcut, focusBrowserAddressBar(in:), focusBrowserAddressBar(panelId:), and openBrowserAndFocusAddressBar(...) to ensure a final unconditional return true when the shortcut matched.Sources/Panels/BrowserPanel.swift (1)
696-711: RemoveUserDefaults.synchronize()calls on lines 697 and 710.Apple's documentation explicitly discourages calling
synchronize()on every read/write—the method is "unnecessary and shouldn't be used." Calling it inisDisabled()(line 697) andsetDisabled()(line 710) adds avoidable latency to frequently-checked settings. Only callsynchronize()if you can prove a cross-process consistency gap; for typical preference reads/writes, rely on UserDefaults' automatic buffering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 696 - 711, Remove the unnecessary UserDefaults.synchronize() calls in the BrowserPanel helpers: delete the call inside static func isDisabled(defaults: UserDefaults = .standard) and the call inside static func setDisabled(_ disabled: Bool, defaults: UserDefaults = .standard); keep reading with defaults.object(forKey: disabledKey)/defaults.bool(forKey: disabledKey) and keep defaults.set(...)/NotificationCenter.default.post(name: didChangeNotification, object: nil) as-is, relying on UserDefaults' automatic buffering for persistence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 63095-63112: Update the "settings.browser.enabled.subtitleOff"
localization entries to match the surfaces called out in
"settings.browser.enabled.subtitleOn": explicitly mention terminal link clicks
and intercepted open commands in both the "en" and "ja" stringUnit.value texts
so the off-state describes that terminal link clicks and intercepted open
commands are routed to the default browser (or not handled by the app) just as
subtitleOn references; modify the English and Japanese values under
settings.browser.enabled.subtitleOff accordingly to mirror the same user-facing
surfaces.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 618-621: Move the empty-URL validation to run before the
short-circuit that checks browser availability: ensure the code trims and checks
rawURL (the target computed from rawURL.trimmingCharacters(in:
.whitespacesAndNewlines) and the guard !target.isEmpty) before calling
BrowserAvailabilitySettings.isEnabled(defaults: defaults); this way blank input
is rejected (returns false) even when the browser is disabled instead of being
treated as "open externally."
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 10976-10986: The redactedDebugURL(_:) function currently strips
user, password, query, and fragment but leaves the URL path which can leak
sensitive tokens; update redactedDebugURL(_:) to also remove or replace the path
(e.g., set components.path = "/<redacted>" or empty) and ensure scheme, host,
and port (if present) are retained; also handle URLs that lack a host by
returning a safe placeholder like "<redacted>" or "<invalid>" so no sensitive
path data is ever included in logs.
- Around line 10446-10761: The .focusBrowserAddressBar branch can match but
currently only returns true when focus succeeds; change the logic in the
matchConfiguredShortcut(event: event, action: .focusBrowserAddressBar) block so
that after the existing early-success returns (focusedBrowserPanel,
browserAddressBarFocusedPanelId, openBrowserAndFocusAddressBar != nil) you
always consume the shortcut (return true) even when
openBrowserAndFocusAddressBar(...) returns nil (i.e., browser disabled). Update
the block that references matchConfiguredShortcut, focusBrowserAddressBar(in:),
focusBrowserAddressBar(panelId:), and openBrowserAndFocusAddressBar(...) to
ensure a final unconditional return true when the shortcut matched.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 696-711: Remove the unnecessary UserDefaults.synchronize() calls
in the BrowserPanel helpers: delete the call inside static func
isDisabled(defaults: UserDefaults = .standard) and the call inside static func
setDisabled(_ disabled: Bool, defaults: UserDefaults = .standard); keep reading
with defaults.object(forKey: disabledKey)/defaults.bool(forKey: disabledKey) and
keep defaults.set(...)/NotificationCenter.default.post(name:
didChangeNotification, object: nil) as-is, relying on UserDefaults' automatic
buffering for persistence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6891259c-71a2-4870-baa1-accc8191f41c
📒 Files selected for processing (10)
CLI/cmux.swiftResources/Localizable.xcstringsResources/bin/openSources/AppDelegate.swiftSources/ContentView.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ac4cd2c2
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Panels/BrowserPanel.swift`:
- Around line 696-711: Remove all calls to UserDefaults.synchronize() in
BrowserPanel: delete synchronize() from isDisabled(defaults:) and
setDisabled(_:defaults:), and keep isEnabled(defaults:) as the negation of
isDisabled; additionally, since didChangeNotification has no observers and state
is managed via `@AppStorage` in cmuxApp.swift, remove the
NotificationCenter.default.post(name: didChangeNotification, object: nil) call
from setDisabled(_:defaults:) as well so reads/writes rely on UserDefaults
without deprecated synchronization or unused notifications (referencing
isDisabled, isEnabled, setDisabled, disabledKey, and didChangeNotification).
In `@Sources/TerminalController.swift`:
- Around line 3330-3359: v2BrowserDisabledExternalOpenResult currently collapses
malformed-but-nonempty URLs into a browser-disabled error because it only
receives URL?; change the helper to accept the original input string (e.g., add
parameter originalURLString: String?) or an enum indicating empty vs malformed,
and implement logic: if originalURLString != nil && url == nil return a distinct
malformed-URL error (e.g., .err(code: "malformed_url", message: "Invalid URL",
data: ["url": originalURLString])), but only return the browser_disabled error
when the caller actually omitted the URL (originalURLString == nil and url ==
nil); update callers of v2BrowserDisabledExternalOpenResult (and the analogous
helper used at the other location) to pass the original string so malformed URLs
are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 561f0959-a6bd-4100-8805-c5de1acd0080
📒 Files selected for processing (7)
Resources/Localizable.xcstringsResources/bin/openSources/ContentView.swiftSources/Panels/BrowserPanel.swiftSources/TerminalController.swiftSources/Workspace.swifttests/test_open_wrapper.py
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Workspace.swift
- Sources/ContentView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
821-828:⚠️ Potential issue | 🟠 MajorHonor
shouldRenderWebViewbefore reopening DevTools.
browserSnapshot.shouldRenderWebViewis now persisted, but this restore path still reopens DevTools wheneverdeveloperToolsVisibleis true. That can force browser-side restore work even when the snapshot explicitly says not to attach/render the web view yet.Suggested fix
- if browserSnapshot.developerToolsVisible && BrowserAvailabilitySettings.isEnabled() { + if browserSnapshot.developerToolsVisible && + browserSnapshot.shouldRenderWebView && + BrowserAvailabilitySettings.isEnabled() { _ = browserPanel.showDeveloperTools() browserPanel.requestDeveloperToolsRefreshAfterNextAttach(reason: "session_restore") } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 821 - 828, When restoring a snapshot, don't unconditionally reopen DevTools — honor browserSnapshot.shouldRenderWebView first: after calling browserPanel.restoreSessionSnapshot(browserSnapshot) check browserSnapshot.shouldRenderWebView and BrowserAvailabilitySettings.isEnabled() before calling browserPanel.showDeveloperTools(); if shouldRenderWebView is false, call browserPanel.hideDeveloperTools() and skip requestDeveloperToolsRefreshAfterNextAttach(reason:), otherwise call showDeveloperTools() and then requestDeveloperToolsRefreshAfterNextAttach(reason: "session_restore"); keep the existing hideDeveloperTools() path for the false case as well.
🤖 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/GhosttyTerminalView.swift`:
- Around line 3826-3834: The handler currently fires a detached Task that calls
Self.openEmbeddedBrowserLink(...) and then returns true immediately, hiding any
failure from openEmbeddedBrowserLink; replace the fire-and-forget Task with an
awaited call so the Bool result is observed and returned (or propagate the
failure), e.g. await Self.openEmbeddedBrowserLink(...) on the MainActor and
return that Bool instead of unconditionally returning true, ensuring the call
still executes on the MainActor.
In `@Sources/Workspace.swift`:
- Around line 10141-10143: The guard in the browser-creation helper (checking
BrowserAvailabilitySettings.isEnabled() ||
creationPolicy.permitsCreationWhenBrowserDisabled) returns nil before inspecting
url/initialRequest, which causes createBrowserToRight and
duplicateBrowserToRight to drop navigations when the internal browser is
disabled; change the helper so that if the browser is disabled it still detects
a provided url or initialRequest and triggers the external-open fallback (the
same fallback used in newBrowserSplit), while only returning the
browser_disabled path for creations with no URL/initialRequest; update logic
around BrowserAvailabilitySettings.isEnabled(),
creationPolicy.permitsCreationWhenBrowserDisabled, and the url/initialRequest
checks so URL-bearing calls always open externally and non-URL calls return the
disabled error.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 821-828: When restoring a snapshot, don't unconditionally reopen
DevTools — honor browserSnapshot.shouldRenderWebView first: after calling
browserPanel.restoreSessionSnapshot(browserSnapshot) check
browserSnapshot.shouldRenderWebView and BrowserAvailabilitySettings.isEnabled()
before calling browserPanel.showDeveloperTools(); if shouldRenderWebView is
false, call browserPanel.hideDeveloperTools() and skip
requestDeveloperToolsRefreshAfterNextAttach(reason:), otherwise call
showDeveloperTools() and then
requestDeveloperToolsRefreshAfterNextAttach(reason: "session_restore"); keep the
existing hideDeveloperTools() path for the false case as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44113692-b884-48ac-bb02-430b3a575e2d
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af073adf52
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c969c56dba
ℹ️ 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".
| if BrowserAvailabilitySettings.isDisabled() { | ||
| return v2BrowserDisabledExternalOpenResult(rawURL: urlStr, url: url, tabManager: tabManager) |
There was a problem hiding this comment.
When browser mode is disabled, this early return skips all workspace/surface resolution in v2BrowserOpenSplit, so requests with stale or invalid workspace_id/surface_id still open the URL externally and can return success. In enabled mode the same invalid handles return not_found, so this changes scoped-command semantics and can trigger unintended external opens from bad refs. The disabled path should still validate routing context before performing side effects.
Useful? React with 👍 / 👎.
| preferredProfileID: UUID? = nil, | ||
| insertAtEnd: Bool = false | ||
| ) -> UUID? { | ||
| guard BrowserAvailabilitySettings.isEnabled() else { return nil } |
There was a problem hiding this comment.
This guard prevents TabManager.openBrowser from reaching Workspace.newBrowserSurface/newBrowserSplit, which now contain the disabled-mode external-open fallback for non-nil URLs. As a result, callers that only check for nil (for example AppDelegate.openDirectoryInInlineVSCode) now fail/beep instead of opening the generated URL in the default browser when browser mode is disabled.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Summary
Testing
Summary by cubic
Adds a global, per-bundle switch to disable the embedded
cmuxbrowser, overridingsettings.json. When disabled, links open in the system browser, browser creation is blocked, and session restore preserves layout without attaching web views or DevTools; all fallback paths are hardened.New Features
cmux disable-browser | enable-browser | browser-statusandcmux browser disable | enable | status(supports--json); persisted asbrowserDisabledOverridein app defaults for the detected bundle (respectsCMUX_BUNDLE_ID).browserDisabledOverride.Resources/bin/openfall back to the system browser; UI/socket/CLI browser create/split are blocked and socket/v2 returnopened_externally: truewithbrowser_disabled: true; session restore keeps browser panels/layout without attaching a web view or opening DevTools.Bug Fixes
Resources/bin/opennow honorsbrowserDisabledOverrideand forwards system open exit codes; stops processing after external opens.Written for commit c969c56. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Behavior Changes
Tests