Repository navigation
Fix browser deep-link popups (slack://, discord://, etc.) - #4226
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds centralized external-navigation routing for native app deep-links ( ChangesExternal Navigation Routing and Intent URL Fallback
Sequence DiagramsequenceDiagram
participant WKNav as WKNavigationDelegate
participant BrowserPanel
participant URLCheck as browserShouldRouteExternalNavigation
participant IntentParser as browserIntentFallbackURL
participant Handler as browserHandleExternalNavigation
participant NSWrksp as NSWorkspace.shared
participant Alert as NSAlert
WKNav->>URLCheck: Check if URL is external-routable
URLCheck-->>WKNav: true for scheme like slack://
WKNav->>Handler: Route external navigation
Handler->>IntentParser: Extract fallback from intent://
IntentParser-->>Handler: HTTPS URL or nil
alt Fallback extracted
Handler->>BrowserPanel: Load fallback URL
else No fallback
Handler->>NSWrksp: Attempt open URL
end
alt NSWorkspace succeeds
NSWrksp-->>Handler: true
Handler-->>WKNav: true (handled)
else NSWorkspace fails
NSWrksp-->>Handler: false
Handler->>Alert: Present failure alert with app name
Alert-->>Handler: User dismisses or copies URL
Handler-->>WKNav: false (failed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 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 |
![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 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:1084">
P2: The localized failure message expects a `%@` argument, but `appName` is never formatted into the string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Contributor
Greptile SummaryThis PR fixes external deep-link (e.g.
Confidence Score: 5/5Safe to merge; the change is well-scoped to external-URL routing with no regressions to normal navigation policy and targeted regression tests added. The external-nav consolidation is correct and the diff is internally consistent: decisionHandler is called exactly once on all code paths, intent fallback extraction validates scheme before loading, the failure dialog is localized and exposes no implementation details, and the old NSLog calls have been removed. The only gap is that the new UI presentation helpers interact with AppKit modal APIs without explicit isolation annotation, which is a code hygiene concern rather than a runtime defect given WebKit always delivers delegate callbacks on the main thread. Sources/Panels/BrowserPanel.swift — the new private presentation helpers should be annotated @mainactor to express their isolation contract explicitly. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebKit navigation delegate\nany frame] --> B{URL in request?}
B -- No --> C[decisionHandler .allow]
B -- Yes --> D{browserShouldRouteExternalNavigation}
D -- No --> E[Continue normal policy checks]
D -- Yes --> F[browserHandleExternalNavigation]
F --> G{browserExternalNavigationAction}
G -- intent:// with HTTPS fallback --> H[.browserFallback\nload fallback URL in WebView]
G -- non-embedded scheme --> I[.promptToOpenApp\nShow Open External App? sheet]
H --> J[loadFallbackRequest\ncurrentTab / popup requestNavigation]
I -- User: Stay in Browser --> K[No-op / DEBUG log]
I -- User: Open App --> L[NSWorkspace.shared.open]
L -- success --> M[DEBUG log opened=1]
L -- failure --> N[Show failure dialog\nCannot Open Link]
N --> O[User: Copy Link to NSPasteboard\nUser: OK to dismiss]
F --> P[decisionHandler .cancel]
Reviews (3): Last reviewed commit: "fix: remove frame parameter from externa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 1354-1357: The NSLog call in BrowserPanel that logs
url.absoluteString when NSWorkspace.shared.open(url) fails should not emit raw
external URLs; remove or replace that NSLog call to emit only sanitized metadata
(e.g., host, path components count, or a redacted URL) consistent with the debug
path, keep the logic that calls browserPresentExternalNavigationFailure(for:
url, in: webView) intact, and ensure the change touches the block where 'opened'
is checked after NSWorkspace.shared.open(url) to avoid logging sensitive data.
- Around line 1276-1311: The alert shown by
browserPresentExternalNavigationFailure(for:in:) exposes vendor/service names
via appName; change the sheet to use cmux-branded, generic copy only (remove
appName from alert.informativeText), e.g. set a localized generic message like
"cmux could not open this link. You can copy the link and open it in another
app." and ensure any user-facing buttons/labels likewise avoid vendor names;
keep browserExternalApplicationName(for:) unused in the sheet body and still use
browserCopyExternalNavigationURL(_:) for the copy action.
- Around line 1331-1366: The function browserHandleExternalNavigation currently
takes an optional loadFallbackRequest and directly calls browserLoadRequest when
nil, which bypasses normal navigation checks; change the signature of
browserHandleExternalNavigation to require a non-optional loadFallbackRequest:
((URLRequest) -> Void) so all fallback requests are routed through the panel’s
standard navigation path, remove the fallback to browserLoadRequest inside
browserHandleExternalNavigation, and update all callers (the delegate sites
referenced) to pass their existing panel navigation handler into
loadFallbackRequest (instead of relying on the default) so intent:// fallbacks
produced by browserIntentFallbackURL go through the same insecure-HTTP prompt
and request-rewriting logic as ordinary navigations.
🪄 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: 7ae1865f-51fb-4a6e-8dd0-73b49f248ccb
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/BrowserConfigTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 9a2e1cc. Configure here.
austinywang
dismissed
coderabbitai[bot]’s stale review
May 17, 2026 02:22
Stale CodeRabbit changes-requested review. The referenced inline findings have been addressed/resolved, current CodeRabbit check is passing, and CI/CD is green.
Merged
This was referenced Jul 5, 2026
5 of 6 tasks
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.
Fixes #4224.
Summary
browser.navigation.external ... opened=logs.intent://browser fallback URLs and load them in the web view.Reproduction
cmux78: #4224 browser deep-link popups.vscode://file/Users/austinwang/manaflow/term/cmux78/README.md.iframe firedand inserted the iframe, but Visual Studio Code did not launch./usr/bin/openwith the samevscode://URL launched Visual Studio Code, confirming the scheme handler worked and the failure was in cmux/WebKit routing.Testing
test: cover browser external deep links; it fails before the fix because subframe external routing stays false and intent fallback extraction returns nil.Note
Medium Risk
Changes browser navigation routing for external schemes and
intent://URLs across main and popup WebViews, which can affect link-opening behavior and user flows. Adds new prompts/sheets and clipboard copy behavior that should be validated across navigation paths.Overview
Fixes external deep-link handling so non-HTTP(S) URLs (e.g.,
slack://,discord://) are routed out to macOS even when triggered from subframes and popups, using a sharedbrowserHandleExternalNavigationpath.Adds user-facing UX around external navigation: a localized “Open external app?” prompt before handing off to
NSWorkspace, plus a localized failure sheet with Copy Link (copies to pasteboard) when macOS can’t open the URL.Introduces parsing of
intent://URLs to extract a safe HTTPSS.browser_fallback_urland load it in the WebView instead of launching an app, and adds regression tests covering subframe routing and intent fallback extraction.Reviewed by Cursor Bugbot for commit f87b55b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes deep-link handling so app URLs like slack://, discord://, vscode://, and zoommtg:// open from pages, subframes, and popups (#4224). Adds an “Open App?” prompt, safe
intent://HTTPS fallbacks, a localized failure dialog with Copy Link, and masks URLs in debug logs.S.browser_fallback_urlfromintent://and load it via browser navigation.Written for commit f87b55b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
New Features
Tests