Repository navigation
feat: Add blacklist mode for browser host filtering - #1164
Closed
sharunkumar wants to merge 10 commits into
Closed
sharunkumar wants to merge 10 commits into
sharunkumar wants to merge 10 commits into
Conversation
Add a Whitelist/Blacklist mode toggle to the browser host list setting, allowing users to specify domains that should be blocked from opening in the embedded browser instead of only being able to whitelist allowed domains. Data model (BrowserPanel.swift): - Add HostListMode enum (whitelist/blacklist) and browserHostListModeKey UserDefaults key (default: "whitelist" for backward compatibility) - Add shouldOpenInCmuxBrowser(host:) that respects the configured mode: whitelist mode matches existing behavior, blacklist mode inverts the check so listed hosts open externally and everything else opens in cmux - Keep hostMatchesWhitelist() intact for backward compatibility - Extract hostMatchesEntry() private helper to share pattern-matching logic between both code paths URL routing (GhosttyTerminalView.swift): - Replace hostMatchesWhitelist() call with shouldOpenInCmuxBrowser() at the terminal link click decision point Settings UI (cmuxApp.swift): - Add @AppStorage binding for browserHostListMode - Add segmented Picker control (Whitelist/Blacklist) in browser settings - Make host list label and subtitle text dynamic based on selected mode - Include new setting in the reset-to-defaults flow Shell open wrapper (Resources/bin/open): - Read browserHostListMode from UserDefaults - Add should_open_in_cmux() function that inverts the whitelist check when in blacklist mode - Update URL routing loop to use should_open_in_cmux() instead of host_matches_whitelist() directly Localization (Localizable.xcstrings): - Add English strings for hostListMode, hostBlacklist labels/subtitles and Whitelist/Blacklist picker options - Add Japanese translations (needs_review) Tests: - Add BrowserHostListModeTests (10 cases) covering default mode, mode reading, invalid fallback, whitelist-mode parity, and blacklist-mode behavior (empty list, exact match, miss, wildcard, case-insensitive, dot-boundary) - Add 5 Python open wrapper tests for blacklist mode (match, miss, wildcard, empty list, explicit whitelist mode)
|
@sharunkumar is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughThis pull request extends the browser host filtering feature by introducing support for both whitelist and blacklist modes. It adds localization strings, UI controls for mode selection, refactored decision logic across Swift and shell components, and test coverage for the new functionality. Changes
Sequence DiagramsequenceDiagram
participant User
participant Settings as Settings UI
participant Logic as Decision Logic
participant cmuxBrowser as cmux Browser
participant External as External Browser
User->>Settings: Configure host list mode<br/>(whitelist/blacklist)
Settings->>Settings: Store mode & patterns
User->>Logic: Click/open URL
activate Logic
Logic->>Logic: Read host list mode
Logic->>Logic: Extract host from URL
Logic->>Logic: Match host against patterns
alt Whitelist Mode
Logic->>Logic: Host in patterns?
alt Yes
Logic->>cmuxBrowser: Open in cmux
else No
Logic->>External: Open externally
end
else Blacklist Mode
Logic->>Logic: Host in patterns?
alt Yes
Logic->>External: Open externally
else No
Logic->>cmuxBrowser: Open in cmux
end
end
deactivate Logic
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
sharunkumar
changed the title
sharunkumar
marked this pull request as ready for review
March 11, 2026 17:52
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 7 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/test_open_wrapper.py (3)
843-871: Consider auto-discoveringtest_*functions.These new cases only run because they were manually appended here. That makes the harness easy to break silently when someone adds another test and forgets to register it in
main().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_open_wrapper.py` around lines 843 - 871, Replace the manual sequence of calls in main() with an auto-discovery loop: inspect the module globals for callables whose names start with "test_", sort the names for deterministic order, and invoke each function with the failures list (so functions like test_toggle_disabled_passthrough and others are discovered automatically); ensure main() still returns the appropriate exit code after running all discovered tests and keep the failures list parameter name unchanged.
34-45: Rename the helper arg now that it is not whitelist-only.
run_wrapper(..., whitelist=...)is now used by blacklist tests too, which makes those call sites misleading. A neutral name likehost_listorhost_entrieswould fit both modes and make future cases easier to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_open_wrapper.py` around lines 34 - 45, The helper parameter name whitelist in the run_wrapper function should be renamed to a neutral name (e.g., host_list or host_entries) because it’s used by both whitelist and blacklist tests; update the run_wrapper signature (def run_wrapper(..., host_list: str | None, ...)) and rename all usages within the function body and every test call site that passes whitelist= to use host_list= (or host_entries=) so call sites for blacklist tests become semantically correct and consistent with the new parameter name.
733-840: Add one blacklist IDN regression case.The new blacklist path only covers ASCII hosts. Since this wrapper already has Unicode/punycode edge cases in whitelist mode, one equivalent blacklist case would help catch a mode-specific regression where host normalization works in whitelist mode but not after the blacklist inversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_open_wrapper.py` around lines 733 - 840, Add a regression test for IDN handling in blacklist mode by adding a new test (e.g. test_blacklist_mode_idn_regression) that calls run_wrapper with a Unicode/punycode host URL and host_list_mode="blacklist" and a whitelist containing the corresponding punycode entry; assert with expect that the wrapper returns code 0 and that cmux_log is empty and open_log contains the URL (mirroring other blacklist tests like test_blacklist_mode_match_opens_externally), so host normalization for IDNs is exercised under the blacklist inversion.Sources/GhosttyTerminalView.swift (1)
2152-2156: Use neutral logging for the shared host-list decision.
shouldOpenInCmuxBrowser(host:)now covers both whitelist misses and blacklist matches, sohostList missis inaccurate in blacklist mode and will mislead debug traces. Prefer a neutral reason likehostList blockedhere.Suggested tweak
- dlog("link.openURL target=embedded but hostList miss host=\(host) url=\(url)") + dlog("link.openURL target=embedded but hostList blocked host=\(host) url=\(url)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2152 - 2156, Update the DEBUG logging in the branch where BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:) returns false to use a neutral message that covers both whitelist misses and blacklist matches; replace the existing dlog call that says "hostList miss host=\(host) url=\(url)" with a neutral phrase such as "hostList blocked host=\(host) url=\(url)" (or "hostList decision=blocked") so the log accurately reflects BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:) outcomes.
🤖 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/cmuxApp.swift`:
- Line 2859: The AppStorage-backed browserHostListMode (stored under
BrowserLinkOpenSettings.browserHostListModeKey) can contain unexpected strings,
which leaves the segmented control unselected; normalize/validate this value
before driving the UI by mapping the raw String to the known set of modes (using
the enum or known constants from BrowserLinkOpenSettings) and falling back to
BrowserLinkOpenSettings.defaultBrowserHostListMode when the rawValue is invalid;
implement this either via a computed property that converts browserHostListMode
to a validated enum/normalized String for UI use, or add a small normalization
step where browserHostListMode is read/initialized so the UI always receives a
guaranteed-valid mode (reference symbols: browserHostListMode,
BrowserLinkOpenSettings.browserHostListModeKey,
BrowserLinkOpenSettings.defaultBrowserHostListMode).
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2152-2156: Update the DEBUG logging in the branch where
BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:) returns false to use a
neutral message that covers both whitelist misses and blacklist matches; replace
the existing dlog call that says "hostList miss host=\(host) url=\(url)" with a
neutral phrase such as "hostList blocked host=\(host) url=\(url)" (or "hostList
decision=blocked") so the log accurately reflects
BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:) outcomes.
In `@tests/test_open_wrapper.py`:
- Around line 843-871: Replace the manual sequence of calls in main() with an
auto-discovery loop: inspect the module globals for callables whose names start
with "test_", sort the names for deterministic order, and invoke each function
with the failures list (so functions like test_toggle_disabled_passthrough and
others are discovered automatically); ensure main() still returns the
appropriate exit code after running all discovered tests and keep the failures
list parameter name unchanged.
- Around line 34-45: The helper parameter name whitelist in the run_wrapper
function should be renamed to a neutral name (e.g., host_list or host_entries)
because it’s used by both whitelist and blacklist tests; update the run_wrapper
signature (def run_wrapper(..., host_list: str | None, ...)) and rename all
usages within the function body and every test call site that passes whitelist=
to use host_list= (or host_entries=) so call sites for blacklist tests become
semantically correct and consistent with the new parameter name.
- Around line 733-840: Add a regression test for IDN handling in blacklist mode
by adding a new test (e.g. test_blacklist_mode_idn_regression) that calls
run_wrapper with a Unicode/punycode host URL and host_list_mode="blacklist" and
a whitelist containing the corresponding punycode entry; assert with expect that
the wrapper returns code 0 and that cmux_log is empty and open_log contains the
URL (mirroring other blacklist tests like
test_blacklist_mode_match_opens_externally), so host normalization for IDNs is
exercised under the blacklist inversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e731f41-a873-4e16-8acb-50e83bda68e7
📒 Files selected for processing (7)
Resources/Localizable.xcstringsResources/bin/openSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/cmuxApp.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swifttests/test_open_wrapper.py
Contributor
Greptile SummaryThis PR adds a whitelist/blacklist mode toggle to the browser host-list setting, letting users specify which domains to block from the embedded browser rather than only being able to allowlist. The change is implemented consistently across both the Swift app layer and the Bash Key changes:
Minor issues worth noting:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["URL clicked / open https://... intercepted"] --> B{openTerminalLinks\nor interceptOpen\nenabled?}
B -- No --> EXT1["system_open / NSWorkspace.open (external)"]
B -- Yes --> C{Host list\nempty?}
C -- Yes / Empty --> CMX1["Open in cmux browser"]
C -- No --> D{hostListMode}
D -- whitelist --> E{Host matches\nlist entry?}
E -- Yes --> CMX2["Open in cmux browser"]
E -- No --> EXT2["Open externally"]
D -- blacklist --> F{Host matches\nlist entry?}
F -- Yes --> EXT3["Open externally\n(blocked host)"]
F -- No --> CMX3["Open in cmux browser"]
style CMX1 fill:#4caf50,color:#fff
style CMX2 fill:#4caf50,color:#fff
style CMX3 fill:#4caf50,color:#fff
style EXT1 fill:#f44336,color:#fff
style EXT2 fill:#f44336,color:#fff
style EXT3 fill:#f44336,color:#fff
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Resources/bin/open (1)
342-356: Redundant empty-check is harmless but could be simplified.The
host_list_patternsempty check at line 344-346 is now redundant sinceshould_open_in_cmux()handles the empty case at lines 362-364 before calling this function. The check is defensive and harmless, but if this function is only called fromshould_open_in_cmux, it could be simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/bin/open` around lines 342 - 356, Remove the redundant empty-array guard inside host_matches_list_pattern(): delete the if block that returns 0 when host_list_patterns is empty, since should_open_in_cmux() already handles the empty-case before calling this function; keep the normalize_host and host_matches_pattern logic intact (leave host="$(normalize_host "$url")" and the for loop over host_list_patterns), and ensure callers like should_open_in_cmux() still perform the empty check.Sources/Panels/BrowserPanel.swift (1)
280-294: Consider deduplicatinghostMatchesWhitelistandhostMatchesEntry.Both functions share nearly identical pattern-matching logic:
- Normalize the host
- Iterate through raw patterns, normalize each, and check for match
The key difference is that
hostMatchesWhitelistreturnstruefor empty patterns whilehostMatchesEntrydoes not. SincehostMatchesWhitelistis now private, consider whether it's still needed or ifshouldOpenInCmuxBrowser(which handles the empty-patterns case explicitly) makes it obsolete.Also applies to: 314-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 280 - 294, The host-matching logic is duplicated between hostMatchesWhitelist(_:, defaults:) and hostMatchesEntry(_:, defaults:); extract the shared behavior into a single helper (e.g., hostMatchesPatterns(normalizedHost: String, rawPatterns: [String], allowEmpty: Bool) or a private function hostMatches(normalizedHost: String?, rawPatterns: [String], acceptEmpty: Bool)) that performs normalization of host via BrowserInsecureHTTPSettings.normalizeHost, normalizes each pattern via normalizeWhitelistPattern, and checks matches with hostMatchesPattern; then have hostMatchesWhitelist call the helper with acceptEmpty = true (or keep a thin wrapper that returns true for empty list) and have hostMatchesEntry call it with acceptEmpty = false (or directly use the helper), updating all callers (including shouldOpenInCmuxBrowser if relevant) to use the unified helper and remove the duplicated loop logic from hostMatchesWhitelist and hostMatchesEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Resources/bin/open`:
- Around line 342-356: Remove the redundant empty-array guard inside
host_matches_list_pattern(): delete the if block that returns 0 when
host_list_patterns is empty, since should_open_in_cmux() already handles the
empty-case before calling this function; keep the normalize_host and
host_matches_pattern logic intact (leave host="$(normalize_host "$url")" and the
for loop over host_list_patterns), and ensure callers like should_open_in_cmux()
still perform the empty check.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 280-294: The host-matching logic is duplicated between
hostMatchesWhitelist(_:, defaults:) and hostMatchesEntry(_:, defaults:); extract
the shared behavior into a single helper (e.g.,
hostMatchesPatterns(normalizedHost: String, rawPatterns: [String], allowEmpty:
Bool) or a private function hostMatches(normalizedHost: String?, rawPatterns:
[String], acceptEmpty: Bool)) that performs normalization of host via
BrowserInsecureHTTPSettings.normalizeHost, normalizes each pattern via
normalizeWhitelistPattern, and checks matches with hostMatchesPattern; then have
hostMatchesWhitelist call the helper with acceptEmpty = true (or keep a thin
wrapper that returns true for empty list) and have hostMatchesEntry call it with
acceptEmpty = false (or directly use the helper), updating all callers
(including shouldOpenInCmuxBrowser if relevant) to use the unified helper and
remove the duplicated loop logic from hostMatchesWhitelist and hostMatchesEntry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4f8fe91-6030-4bd8-a6ea-1912959d44b6
📒 Files selected for processing (3)
Resources/bin/openSources/Panels/BrowserPanel.swiftSources/cmuxApp.swift
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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:283">
P3: `hostMatchesWhitelist` is unused and duplicates `hostMatchesEntry`; keeping both helpers increases maintenance drift risk. Remove the unused function or consolidate to a single matcher.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
12527-12563: Avoid repeating the persisted"blacklist"literal in every behavior test.
testBlacklistModeIsReadFromDefaults()already pins the storage value. For the rest of this suite, deriving the stored mode from the enum/raw-value source of truth would reduce drift if the persisted spelling ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 12527 - 12563, Replace the repeated literal "blacklist" in tests with the canonical enum/raw-value from BrowserLinkOpenSettings so tests derive the persisted mode from the source-of-truth; for example, call defaults.set(BrowserLinkOpenSettings.BrowserHostListMode.blacklist.rawValue, forKey: BrowserLinkOpenSettings.browserHostListModeKey) (or the equivalent enum name available on BrowserLinkOpenSettings) in each test that currently uses "blacklist" so the stored value follows the enum/rawValue instead of a hard-coded string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 12527-12563: Replace the repeated literal "blacklist" in tests
with the canonical enum/raw-value from BrowserLinkOpenSettings so tests derive
the persisted mode from the source-of-truth; for example, call
defaults.set(BrowserLinkOpenSettings.BrowserHostListMode.blacklist.rawValue,
forKey: BrowserLinkOpenSettings.browserHostListModeKey) (or the equivalent enum
name available on BrowserLinkOpenSettings) in each test that currently uses
"blacklist" so the stored value follows the enum/rawValue instead of a
hard-coded string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81653aed-e6a7-45b4-b73b-1c12ba31b92e
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/BrowserPanel.swift
Sidebar PR link clicks (both individual and batch 'Open All Workspace PR Links') bypassed the host filtering entirely — they only checked the openSidebarPullRequestLinksInCmuxBrowser boolean toggle without consulting BrowserLinkOpenSettings.shouldOpenExternally() or BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:). This meant blacklisted domains (e.g. github.tesla.com) would still open in the embedded browser when clicked from the sidebar, even though terminal link clicks correctly routed them externally. Both openPullRequestLink() and openWorkspacePullRequestsInConfiguredBrowser() now apply the same external-open-pattern and host whitelist/blacklist checks that GhosttyTerminalView uses before routing to the embedded browser.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
5885-5891: Centralize the sidebar PR routing predicate.These two paths now reimplement the same “external patterns + host list mode” decision inline. It works today, but it makes sidebar PR routing easy to desync again the next time
BrowserLinkOpenSettings/ browser-panel routing changes. Please move this into one shared helper and call it from both the batch and single-link flows.♻️ Suggested consolidation
+ private func shouldOpenSidebarPullRequestExternally(_ url: URL) -> Bool { + if BrowserLinkOpenSettings.shouldOpenExternally(url) { + return true + } + + guard let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? "") else { + return false + } + + return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) + } + private func openWorkspacePullRequestsInConfiguredBrowser() -> Bool { guard let workspace = tabManager.selectedWorkspace else { return false } let pullRequests = workspace.sidebarPullRequestsInDisplayOrder() guard !pullRequests.isEmpty else { return false } var openedCount = 0 if openSidebarPullRequestLinksInCmuxBrowser { for pullRequest in pullRequests { let url = pullRequest.url - // Respect external-open patterns and host whitelist/blacklist settings. - let shouldOpenExternally = BrowserLinkOpenSettings.shouldOpenExternally(url) - || { - guard let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? "") else { return false } - return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) - }() - if shouldOpenExternally { + if shouldOpenSidebarPullRequestExternally(url) { if NSWorkspace.shared.open(url) { openedCount += 1 } } else if tabManager.openBrowser(url: url, insertAtEnd: true) != nil { openedCount += 1 } else if NSWorkspace.shared.open(url) { openedCount += 1 @@ private func openPullRequestLink(_ url: URL) { updateSelection() if openSidebarPullRequestLinksInCmuxBrowser { - // Respect external-open patterns and host whitelist/blacklist settings. - if BrowserLinkOpenSettings.shouldOpenExternally(url) { - NSWorkspace.shared.open(url) - return - } - if let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? ""), - !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) { + if shouldOpenSidebarPullRequestExternally(url) { NSWorkspace.shared.open(url) return } if tabManager.openBrowser( inWorkspace: tab.id,Also applies to: 10259-10268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 5885 - 5891, The inline predicate that decides "open externally vs in CMux browser" is duplicated; extract it into a single shared helper (e.g., add a method on BrowserLinkOpenSettings like shouldOpenExternallyForSidebar(url: URL) or a static helper function) that encapsulates the current logic: BrowserLinkOpenSettings.shouldOpenExternally(url) OR (normalize host via BrowserInsecureHTTPSettings.normalizeHost(...) and then !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:)). Replace the duplicated inline block (the local shouldOpenExternally variable used with pullRequest.url in both the single-link flow and the batch flow) to call the new helper so both paths use the same centralized predicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 5885-5891: The inline predicate that decides "open externally vs
in CMux browser" is duplicated; extract it into a single shared helper (e.g.,
add a method on BrowserLinkOpenSettings like shouldOpenExternallyForSidebar(url:
URL) or a static helper function) that encapsulates the current logic:
BrowserLinkOpenSettings.shouldOpenExternally(url) OR (normalize host via
BrowserInsecureHTTPSettings.normalizeHost(...) and then
!BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host:)). Replace the duplicated
inline block (the local shouldOpenExternally variable used with pullRequest.url
in both the single-link flow and the batch flow) to call the new helper so both
paths use the same centralized predicate.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
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/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:5889">
P2: Normalization failure is treated as "open in cmux" here, which can bypass host-list filtering for non-normalizable hosts.</violation>
<violation number="2" location="Sources/ContentView.swift:10264">
P2: This `if let` bypasses host-list enforcement when host normalization fails, allowing embedded open for hosts that should be treated as non-eligible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+10264
to
+10265
| if let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? ""), | ||
| !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) { |
There was a problem hiding this comment.
P2: This if let bypasses host-list enforcement when host normalization fails, allowing embedded open for hosts that should be treated as non-eligible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 10264:
<comment>This `if let` bypasses host-list enforcement when host normalization fails, allowing embedded open for hosts that should be treated as non-eligible.</comment>
<file context>
@@ -10247,6 +10256,16 @@ private struct TabItemView: View {
+ NSWorkspace.shared.open(url)
+ return
+ }
+ if let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? ""),
+ !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) {
+ NSWorkspace.shared.open(url)
</file context>
Suggested change
| if let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? ""), | |
| !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) { | |
| if !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: url.host ?? "") { |
Comment on lines
+5889
to
+5890
| guard let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? "") else { return false } | ||
| return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) |
There was a problem hiding this comment.
P2: Normalization failure is treated as "open in cmux" here, which can bypass host-list filtering for non-normalizable hosts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 5889:
<comment>Normalization failure is treated as "open in cmux" here, which can bypass host-list filtering for non-normalizable hosts.</comment>
<file context>
@@ -5882,9 +5882,18 @@ struct ContentView: View {
+ // Respect external-open patterns and host whitelist/blacklist settings.
+ let shouldOpenExternally = BrowserLinkOpenSettings.shouldOpenExternally(url)
+ || {
+ guard let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? "") else { return false }
+ return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host)
+ }()
</file context>
Suggested change
| guard let host = BrowserInsecureHTTPSettings.normalizeHost(url.host ?? "") else { return false } | |
| return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: host) | |
| return !BrowserLinkOpenSettings.shouldOpenInCmuxBrowser(host: url.host ?? "") |
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
2 participants
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
Adds a Whitelist/Blacklist mode toggle to the browser host list setting, so users can specify domains that should be blocked from opening in the embedded browser rather than only being able to whitelist allowed domains.
Motivation
Based on #124
Currently the only way to control which sites open in the cmux browser is a whitelist — you must list every host you want to open internally. For users who want most sites in cmux but need to block a few specific domains (e.g. sites that don't render well, SSO login pages, or sites with download flows), maintaining an exhaustive whitelist is impractical. A blacklist mode inverts the logic: list only the hosts you want to exclude, and everything else opens in cmux.
Behavior
Screenshots
Bug fix: sidebar PR links now respect host filtering
Sidebar PR link clicks (both individual clicks and batch "Open All Workspace PR Links") previously bypassed host whitelist/blacklist filtering entirely. They only checked the
openSidebarPullRequestLinksInCmuxBrowsertoggle without consultingshouldOpenExternally()orshouldOpenInCmuxBrowser(host:). This meant blacklisted domains would still open in the embedded browser when clicked from the sidebar, even though terminal link clicks correctly routed them externally. BothopenPullRequestLink()andopenWorkspacePullRequestsInConfiguredBrowser()now apply the same host filtering checks as the terminal link flow.Summary by CodeRabbit