Send sidebar links through the external-open rules

[ejc3]

browser.urlsToAlwaysOpenExternally sends a matching link to the system browser instead of cmux's embedded one. Terminal links, the intercepted open command and link clicks inside the embedded browser already follow it. Pull-request and port links in the sidebar do not: they open embedded even when a rule matches, so a site that only works in a managed browser dead-ends there.

Repro: add a rule such as github.com, then click a pull-request link in the sidebar. It opens in the embedded browser.

What changes

The sidebar's pull-request and port links, and the action that opens every pull request of a workspace, now ask BrowserExternalNavigationHandler.sidebarLinkDestination(for:prefersEmbeddedBrowser:) where to open. A link that matches a rule goes to the system browser even when "open sidebar links in the cmux browser" is on, because a rule-listed site cannot work in the embedded web view at all. Only web schemes are checked against the rules. Other schemes keep following the preference.

This PR used to carry a second half that changed when a link inside a page may leave for the system browser. The in-page routing has landed separately in #10634, and the user-activation check here was wrong (it read NSApp.currentEvent at decision time, which can be a later mouse move), so that half is removed. The diff is now three files.

Tests

BrowserLinkOpenSettingsTests adds four: a sidebar link with no matching rule follows the preference, a pull-request link that matches a rule goes to the system browser, a port link that matches a rule does too while localhost stays embedded, and a non-web scheme follows the preference even when a rule's text matches it.

Against a sidebarLinkDestination that only follows the preference, three of the four fail, each reporting embeddedBrowser where systemBrowser is expected. With the rule check in place, all 27 tests in the suite pass.

Main's test target does not compile at 00f182fc0fa (RightSidebarTabCustomizationTests.swift, which #17211 repairs), so I ran both commits with that file and CloudMachineOrderingTests.swift removed locally. Neither removal is in this branch.

Changelog

Fixed: Sidebar pull request and port links follow the “always open externally” URL rules

[vercel]

@ejc3 is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds link escape routing for browser navigations, new-tab and popup opens, sidebar/pull-request link handling, and end-to-end UI test coverage with updated fixture text and locale instructions.

Changes

External link escape routing

Layer / File(s)	Summary
escape predicate and pattern parsing Sources/Panels/BrowserPanel.swift	Adds BrowserLinkOpenSettings.linkEscapesToSystemBrowser(_:), hardens external-open regex parsing with guarded try? and DEBUG logging, and changes browserOpenExternalNavigationURL from private to internal.
WebKit navigation and popup interception Sources/Panels/BrowserNavigationDelegate.swift, Sources/Panels/BrowserPanel.swift	decidePolicyFor cancels link-activated navigations matching the escape check and routes them externally; openLinkInNewTab and createWebViewWith similarly route matching URLs to browserOpenExternalNavigationURL instead of embedded tab/popup creation.
Sidebar and pull-request link opening Sources/ContentView.swift	Pull-request link opening and openSidebarLink/openPullRequestLink/openPortLink now check linkEscapesToSystemBrowser to choose between embedded tabManager.openBrowser and NSWorkspace.shared.open fallback.
UI-test socket and fixture harness cmuxUITests/BrowserFixtureInteractionUITests.swift, cmux.xcodeproj/project.pbxproj	Adds launch/configuration hooks, socket fallback paths, readiness polling, logging, and project wiring for the new UI test source.
External-open routing UI tests and fixtures cmuxUITests/BrowserExternalOpenRoutingUITests.swift, cmuxUITests/BrowserFixtures/*	Adds capture-based UI tests plus HTML fixtures that exercise matched and unmatched external-open cases, popup opens, and form POST routing.
Unit tests and locale wording cmuxTests/BrowserConfigTests.swift, Resources/Localizable.xcstrings	New tests validate domain pattern matching and escape behavior; locale strings update the link-opening instructions for embedded browser and sidebar links.

Estimated code review effort: 4 (Complex) | ~45 minutes

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 errors, 1 warning)

Check name	Status	Explanation	Resolution
Cmux Full Internationalization	❌ Error	Edited settings.browser.externalPatterns and its subtitle cover 19 locales but omit supported km, violating the catalog's full-i18n rule.	Add km translations for both settings.browser.externalPatterns and settings.browser.externalPatterns.subtitle in Resources/Localizable.xcstrings and keep every supported locale filled.
Cmux No Test Or Debug Seam In Production Source	❌ Error	BrowserPanel.swift adds a DEBUG-only UI-test capture sink in browserOpenExternalNavigationURL, a production-source test seam with no product caller.	Move the capture/observation hook into the test target or a dedicated debug-only file; keep production code internal and use @testable import for tests.
Cmux No Ambient Global State	❌ Error	Sources/Panels/BrowserPanel.swift:1223 widens browserOpenExternalNavigationURL from private to module-internal file-scope API, violating the no-ambient-global-state rule.	Move the open-routing behavior onto BrowserPanel (or an injected router/service) and keep file-scope helpers private/fileprivate; don’t expose a module-global free function.
Description check	⚠️ Warning	The description gives a clear problem, implementation summary, test results, and changelog. However, it says the diff is three files and that in-page routing was removed, while the supplied change sum…	Reconcile the description with the final diff and objectives. Clarify which browser-routing and localization changes are included, and update the testing details accordingly. Add the applicable localization-audit result, a demo video or scr…

✅ Passed checks (21 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	The new routing helpers are top-level pure utilities; call sites stay inside @MainActor BrowserPanel/BrowserNavigationDelegate/ContentView, with no new Sendable/shared-mutable types.
Cmux Swift Blocking Runtime	✅ Passed	PASS: the only new waits/polling are in BrowserFixtureInteractionUITests scaffolding; the production changes add no new blocking primitives.
Cmux Browser Automation Off-Main	✅ Passed	Wait-heavy browser verbs stay on the socket worker, use explicit main hops, and have policy-test coverage; this PR only adds link-routing tests/helpers.
Cmux Expensive Synchronous Load	✅ Passed	Touched browser-routing paths add no new synchronous agent-history loads; no RestorableAgentSessionIndex/JSONL/Data(contentsOf:) work was introduced on main/interactive paths.
Cmux Cache Substitution Correctness	✅ Passed	No production diff swaps authoritative loads for cached values in persistence/history/undo/snapshot paths; the changes only add link-routing predicates and UI-test capture plumbing.
Cmux No Hacky Sleeps	✅ Passed	No changed JS/TS/shell/build runtime files are in scope, and the new polling/wait helpers are Swift test scaffolding, which this check excludes.
Cmux Algorithmic Complexity	✅ Passed	No new nested scans or per-record rescans on scalable collections; added iteration is over small settings/pattern lists or test fixtures, and hot paths stay linear.
Cmux Swift Concurrency	✅ Passed	App-side link-routing changes are synchronous; the only new DispatchQueue is XCTest pipe-drain plumbing, which is a permitted test boundary.
Cmux Swift @Concurrent	✅ Passed	Only BrowserFixtureInteractionUITests changed; the added CLI-sink helper is synchronous, with no new nonisolated async work or @concurrent misuse.
Cmux Swift File And Package Boundaries	✅ Passed	PASS: Production changes are small glue in already-oversized app files; no file grows >250 lines, and the larger additions are tests/fixtures, which are exempt.
Cmux Swiftpm Lockfiles	✅ Passed	Diff only adds a UITest file to cmux.xcodeproj; no Package.swift, Package.resolved, .gitignore, or SwiftPM package-reference changes are present.
Cmux Swift Logging	✅ Passed	Added production logs are DEBUG-guarded cmuxDebugLog calls; remaining NSLog usage is preexisting or confined to UI tests.
Cmux User-Facing Error Privacy	✅ Passed	Changed user-facing copy stays generic: the external-open alert says cmux could not open the link, and the settings subtitle only documents the public config key.
Cmux Swiftui State Layout	✅ Passed	Diff only adds link-routing helpers/checks in existing views; no new ObservableObject/@published, GeometryReader layout changes, row store refs, or render-time state writes.
Cmux Architecture Rethink	✅ Passed	PASS: the new polling/capture code is UI-test-only, and production routing uses explicit WebKit callback bridges into one shared external-open helper with clear ownership.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	The PR only changes browser-routing logic and test fixtures; it adds no standalone NSWindow/NSPanel/WindowGroup code or cmuxAuxiliaryWindowIdentifiers entries.
Cmux Source Artifacts	✅ Passed	Changed paths are source/config/localization/tests/fixtures only; no logs, caches, temp dirs, build output, or other artifact paths appear in the diff.
Title check	✅ Passed	The title clearly identifies the sidebar-link change and its use of external-open rules.

Full details: Description check

Explanation

The description gives a clear problem, implementation summary, test results, and changelog. However, it says the diff is three files and that in-page routing was removed, while the supplied change summary and objectives include broader browser-routing, localization, and UI-test changes.

Resolution

Reconcile the description with the final diff and objectives. Clarify which browser-routing and localization changes are included, and update the testing details accordingly. Add the applicable localization-audit result, a demo video or screenshot for the behavior change, and the relevant checklist items.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR extends the browser.urlsToAlwaysOpenExternally escape rules — previously consulted only for terminal link clicks and intercepted open calls — to apply wherever cmux opens an http/https link: main-frame and target=_blank link activations inside the embedded browser, context-menu "Open Link in New Tab", popup-window navigations, and both sidebar link-open paths. A new linkEscapesToSystemBrowser gate wraps the existing substring/regex matcher with a web-scheme guard. The existing UITestCaptureSink DEBUG capture pattern intercepts escapes in CI so no real browser is launched during tests.

• BrowserNavigationDelegate and BrowserUIDelegate each gain an early escape check for .linkActivated navigations that match the rules; form POSTs, downloads, and scripted window.open are intentionally excluded.
• BrowserPopupWindowController mirrors both delegate escape paths so popup-window link clicks obey the same policy.
• ContentView.swift sidebar PR-link and port-link paths add a !linkEscapesToSystemBrowser(url) guard ahead of the embedded-open attempt, falling through to the pre-existing NSWorkspace.shared.open fallback.
• Localization strings are updated in all 19 supported locales, and new unit + socket UI tests cover matched/unmatched links, scripted window.open, and form-POST routing.

Confidence Score: 5/5

Safe to merge. All six link-open entry points are updated consistently, matching semantics are unchanged, and the escape gate is restricted to http/https with a web-scheme guard.

Every new escape check follows the same .linkActivated + non-download guard used by the existing checkout-escape path. Form POSTs, scripted window.open, and downloads are deliberately excluded and verified by the new UI test suite. The UITestCaptureSink #if DEBUG interception is the established pattern already present in GhosttyTerminalView, WindowDragHandleView, and other production files. Localization is complete across all 19 locales. No new global state, no blocking primitives, and no actor-isolation issues were introduced.

No files require special attention.

Important Files Changed

Filename	Overview
Sources/Panels/BrowserNavigationDelegate.swift	Adds escape check before the insecure-HTTP gate; correctly restricted to .linkActivated, main-frame-or-nil target, non-download navigations. Uses injectable presentAlert and conditionally reports terminal cancellation only on success.
Sources/Panels/BrowserPanel.swift	Adds linkEscapesToSystemBrowser static helper (web-scheme guard + existing matcher), escape check in openLinkInNewTab (context-menu path), escape check in BrowserUIDelegate.createWebViewWith for target=_blank, and UITestCaptureSink interception under #if DEBUG in browserOpenExternalNavigationURL — follows the established capture-sink pattern used by GhosttyTerminalView and others.
Sources/Panels/BrowserPopupWindowController.swift	Mirrors the main-browser escape policy for both PopupNavigationDelegate (link-activated, non-download, pre-insecure-HTTP-gate) and PopupUIDelegate (target=_blank path). No presentAlert injection on the popup nav path (uses default browserPresentAlert), consistent with popup context.
Sources/ContentView.swift	Three sidebar link-open sites gain !BrowserLinkOpenSettings.linkEscapesToSystemBrowser(url) guards; matched URLs fall through to the pre-existing NSWorkspace.shared.open path — correct for a context that has no WKWebView to anchor an alert.
cmuxUITests/BrowserExternalOpenRoutingUITests.swift	New end-to-end suite exercising matched link escape, unmatched embedded stay, scripted window.open non-escape, and form-POST non-escape through real WebKit link activations; capture-file sink avoids launching a real browser in CI.
cmuxUITests/BrowserFixtureInteractionUITests.swift	Hardens BrowserFixtureSocketTestCase with nc and bundled-CLI fallback transports, hidden-webview discard env var, lastWorkspaceID tracking, waitForCondition/waitForSelector/waitForEmbeddedTab polling helpers, and openTabURLs helper — all test-only scaffolding improvements.
Resources/Localizable.xcstrings	Settings subtitle for 'URLs to Always Open Externally' updated in all 19 supported locales to describe full link-open coverage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Link activation event] --> B{Source?}
    B -->|Terminal link click| C[Existing path: TerminalController]
    B -->|intercepted open call| D[Existing path: open handler]
    B -->|Main-frame click in embedded browser| E[BrowserNavigationDelegate decidePolicyFor]
    B -->|target=_blank click in embedded browser| F[BrowserUIDelegate createWebViewWith]
    B -->|Context menu Open Link in New Tab| G[BrowserPanel openLinkInNewTab]
    B -->|Link click in popup window| H[PopupNavigationDelegate + PopupUIDelegate]
    B -->|Sidebar PR/port link| I[ContentView sidebarLink handlers]
    E --> J{linkEscapesToSystemBrowser?}
    F --> J
    G --> J
    H --> J
    I --> K{linkEscapesToSystemBrowser?}
    J -->|Yes: http/https + rule matches| L[browserOpenExternalNavigationURL]
    J -->|No| M[Embedded browser normal flow]
    K -->|Yes| N[NSWorkspace.shared.open]
    K -->|No| O[tabManager.openBrowser embedded tab]
    L --> P{DEBUG + CMUX_UI_TEST_CAPTURE_EXTERNAL_OPEN_PATH set?}
    P -->|Yes| Q[Write to capture file — no real browser]
    P -->|No| R[NSWorkspace.shared.open in system browser]

Loading

%%{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[Link activation event] --> B{Source?}
    B -->|Terminal link click| C[Existing path: TerminalController]
    B -->|intercepted open call| D[Existing path: open handler]
    B -->|Main-frame click in embedded browser| E[BrowserNavigationDelegate decidePolicyFor]
    B -->|target=_blank click in embedded browser| F[BrowserUIDelegate createWebViewWith]
    B -->|Context menu Open Link in New Tab| G[BrowserPanel openLinkInNewTab]
    B -->|Link click in popup window| H[PopupNavigationDelegate + PopupUIDelegate]
    B -->|Sidebar PR/port link| I[ContentView sidebarLink handlers]
    E --> J{linkEscapesToSystemBrowser?}
    F --> J
    G --> J
    H --> J
    I --> K{linkEscapesToSystemBrowser?}
    J -->|Yes: http/https + rule matches| L[browserOpenExternalNavigationURL]
    J -->|No| M[Embedded browser normal flow]
    K -->|Yes| N[NSWorkspace.shared.open]
    K -->|No| O[tabManager.openBrowser embedded tab]
    L --> P{DEBUG + CMUX_UI_TEST_CAPTURE_EXTERNAL_OPEN_PATH set?}
    P -->|Yes| Q[Write to capture file — no real browser]
    P -->|No| R[NSWorkspace.shared.open in system browser]

Loading

_{Reviews (11): Last reviewed commit: "browser: close the escape-rule gaps revi..." | Re-trigger Greptile}

[coderabbitai]

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/Panels/BrowserNavigationDelegate.swift`:
- Around line 272-294: The debug log in BrowserNavigationDelegate’s
external-link escape path uses raw url.absoluteString instead of the same
redacted helper used elsewhere. Update the cmuxDebugLog call in the escape
branch to use browserNavigationDebugURL(url) (or the shared URL formatting
helper) so BrowserNavigationDelegate keeps debug URL logging consistent and
avoids leaking full URLs.

In `@Sources/Panels/BrowserPanel.swift`:
- Around line 8797-8815: The escape-path handling in BrowserPanel’s
createWebView logic is fine, but the new debug log should match the existing
redaction pattern used elsewhere in this method. Update the `cmuxDebugLog` call
inside the `.linkActivated` /
`BrowserLinkOpenSettings.linkEscapesToSystemBrowser` branch to use
`browserNavigationDebugURL(...)` instead of `url.absoluteString`, so the
`browserOpenExternalNavigationURL` path keeps debug output consistent and avoids
exposing raw URLs.

🪄 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: 2c9693b4-f49a-4007-9731-a62939d42682

📥 Commits

Reviewing files that changed from the base of the PR and between c1e4b26 and b18f1d0.

📒 Files selected for processing (5)

• Resources/Localizable.xcstrings
• Sources/ContentView.swift
• Sources/Panels/BrowserNavigationDelegate.swift
• Sources/Panels/BrowserPanel.swift
• cmuxTests/BrowserConfigTests.swift

[coderabbitai]

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 `@cmuxUITests/BrowserFixtureInteractionUITests.swift`:
- Around line 332-361: `waitForEmbeddedTab` currently lets a single transient
`socketResult`/`openTabURLs` failure abort the whole poll instead of retrying
until `timeout`. Update `waitForEmbeddedTab` to mirror the retry-tolerant
pattern used by `waitForCondition` and `waitForSelector`: catch per-attempt
errors inside the loop, ignore transient failures, and keep polling
`openTabURLs` until `browser.tab.list` succeeds and a matching URL is found or
the deadline expires. Preserve the existing `waitForEmbeddedTab` signature and
use its `openTabURLs` helper so the fix stays localized.
- Around line 128-173: In socketEnvelopeViaBundledCLI, the Process is waiting on
exit without draining standardError, and stdout is only read after
waitUntilExit(), which can deadlock if cmux rpc writes enough output. Update
this fallback path to consume both stdout and stderr concurrently (or otherwise
continuously) before waiting, while preserving the existing success-envelope
behavior and timeout handling in socketEnvelopeViaBundledCLI.

🪄 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: 0951cec8-6e64-43ad-8ac1-31d24768d5cb

📥 Commits

Reviewing files that changed from the base of the PR and between 99bee55 and f6d78f3.

📒 Files selected for processing (6)

• Sources/Panels/BrowserPanel.swift
• cmux.xcodeproj/project.pbxproj
• cmuxUITests/BrowserExternalOpenRoutingUITests.swift
• cmuxUITests/BrowserFixtureInteractionUITests.swift
• cmuxUITests/BrowserFixtures/external-open-routing.html
• cmuxUITests/BrowserFixtures/external-open-target.html

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit d6f1c4e. Configure here.}

[github-actions]

CI failure attribution

CI passes on 24301818f0 (run 37204211567 attempt 2).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

[github-actions]

CI fast guards passes on 24301818f0 (https://github.com/manaflow-ai/cmux/actions/runs/37204211324).

[teamleaderleo]

Taking this: checking the remaining sidebar-link slice against the browser routing that already landed.

• OrchardSpoon g1 🌀

[teamleaderleo]

Thanks for the sidebar routing work. That part is still useful, but the in-browser half already landed in #10634 and this branch still changes its user-activation policy.

The new default hasUserActivation: browserNavigationHasSimpleUserActivation() samples NSApp.currentEvent when WebKit asks for a navigation decision. A later mouse-move event can replace the original click, so a real click on a rule-matched link can remain in the embedded browser. This needs event tracking rather than another check of the current event.

Could you trim and refresh this PR to the sidebar matcher, its three open paths, and focused sidebar-routing tests, as suggested in the earlier review? That will let us validate and land the useful sidebar slice while the browser gesture change gets its own fix.

• OrchardSpoon g1 🌀

[ejc3]

Trimmed as you asked. The branch is now the sidebar matcher, its three open paths and four tests in BrowserLinkOpenSettingsTests: three files, 80 lines added. The in-page activation change and its UI tests are gone.

[teamleaderleo]

Merged, thank you @ejc3! PR and port links in the sidebar now follow your external-open rules.

[github-actions]

Merge receipt for 24301818f0: every check was green at merge (15 verified; 21 skipped by policy). Full suite runs on main after merge.