Fix bilibili search popup opening detached window#2836
Open
austinywang wants to merge 2 commits intomainfrom
Open
Fix bilibili search popup opening detached window#2836austinywang wants to merge 2 commits intomainfrom
austinywang wants to merge 2 commits intomainfrom
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:
📝 WalkthroughWalkthroughRoutes certain keyboard-triggered, same-site window.open/form navigations into the current tab (using URLRequest) instead of creating popup windows; adds request-based new-tab API, event-aware helpers, richer debug logs, unit tests, and updates the xcodebuild reload script to stream logs via a FIFO and use -quiet. Changes
Sequence DiagramsequenceDiagram
participant User
participant WK as "WKWebView"
participant Nav as "BrowserNavigationDelegate"
participant Utils as "Gesture/Popup Helpers"
participant Panel as "BrowserPanel / Router"
participant UI as "Workspace / Panel Manager"
User->>WK: Trigger navigation (window.open or form submit)
WK->>Nav: decidePolicyFor(navigationAction, request)
Nav->>Utils: Inspect currentEventType, request method/URL, popup features, opener URL
Utils-->>Nav: simpleUserActivation? same-site? featuresSpecified?
Nav->>Panel: shouldRetargetToCurrentTab?
alt simple user-gesture + same-site + GET + no features
Panel->>UI: openNavigationRequestInNewTab(URLRequest, bypass?)
UI-->>WK: navigateWithoutInsecureHTTPPrompt(request)
else popup/create new window
Panel->>UI: create popup WebView (BrowserPopupWindowController)
UI-->>WK: return popup webview
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 |
![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
🤖 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 5955-5959: The current check in BrowserPanel.swift (the if using
isUserNewTab and browserNavigationHasExplicitUserGesture(currentEventType:)) is
too broad and downgrades legitimate user-initiated popups; restrict the
gesture-based tab fallback to only the form-submit/same-origin case. Replace the
use of browserNavigationHasExplicitUserGesture(...) with a focused helper (e.g.,
browserNavigationIsUserFormSubmitSameOrigin(currentEventType:)) or add an
explicit condition that detects same-origin form submit/new-tab semantics, and
only return false when isUserNewTab ||
browserNavigationIsUserFormSubmitSameOrigin(...) is true; leave other
explicit-gesture paths (OAuth/payment popups) to continue opening as popups so
window.opener/postMessage flows 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: e8182d55-a3d0-478f-afe5-ee66e1eed138
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
Contributor
Greptile SummaryThis PR fixes bilibili-style search popups opening as detached windows by introducing
Confidence Score: 4/5Core fix is sound; two P2 gaps in test coverage and OAuth regression risk should be addressed before merge. The keyDown regression is well-covered and the routing logic is correct. However, the existing cmuxTests/BrowserConfigTests.swift — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebKit navigation with targetFrame == nil] --> B{navigationType == .other?}
B -- No --> C[browserNavigationShouldFallbackNilTargetToNewTab true - Open in new tab]
B -- Yes --> D{browserNavigationHasExplicitUserGesture? leftMouse / rightMouse / otherMouse / keyDown}
D -- Yes user gesture --> E[browserNavigationShouldFallbackNilTargetToNewTab true - Open in new tab - Fix]
D -- No gesture scripted --> F[browserNavigationShouldFallbackNilTargetToNewTab false - Fall through to createWebViewWith]
F --> G{browserNavigationShouldCreatePopup?}
G -- navigationType != .other OR isUserNewTab OR gesture --> H[No popup - openInNewTab fallback]
G -- .other AND no gesture AND not user new-tab --> I[isScriptedPopup = true - Create popup window - OAuth preserved]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 2 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:5957">
P1: This blanket gesture check will suppress popup creation for click-triggered `window.open()` calls (e.g. OAuth/payment flows). WebKit delivers `createWebViewWith` synchronously during JS execution, so `NSApp.currentEvent` is still the mouse event when the delegate fires. Since `browserNavigationHasExplicitUserGesture` matches `.leftMouseDown`/`.leftMouseUp`, any click-initiated popup (which OAuth flows rely on for `window.opener`/`postMessage`) will be downgraded to a tab.
Consider narrowing the gesture fallback to keyboard events only (the bilibili submit case), or adding a same-origin/form-submit heuristic so that click-triggered cross-origin popups are preserved.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6036-6047:⚠️ Potential issue | 🟠 MajorThe “same-site” key is over-broad for public-suffix domains.
parts.suffix(2)makesfoo.co.ukandbar.co.ukboth look likeco.uk. That can still retarget unrelated cross-site popups into tabs and breakwindow.openerflows on ccTLD domains. Exact host matching is safer here unless you already have a public-suffix-aware registrable-domain helper.Suggested fix
private func browserNavigationSiteKey(_ url: URL?) -> String? { - guard let host = url?.host?.lowercased(), !host.isEmpty else { return nil } - - let isIPv4 = host.allSatisfy { $0.isNumber || $0 == "." } - if isIPv4 || host.contains(":") { - return host - } - - let parts = host.split(separator: ".") - guard parts.count >= 2 else { return host } - return parts.suffix(2).joined(separator: ".") + guard let host = url?.host?.lowercased(), !host.isEmpty else { return nil } + return host }Also applies to: 6072-6076
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 6036 - 6047, The browserNavigationSiteKey function currently reduces hosts to the last two labels (parts.suffix(2)) which treats public suffixes like "co.uk" as registrable domains; change this to use exact host matching unless you have a public-suffix-aware helper: in browserNavigationSiteKey(_:) (and the analogous spot around the other occurrence), after validating host and handling IPv4/port cases, return the full host string (host) instead of parts.suffix(2).joined(separator: "."); alternatively wire a proper publicSuffix/registrable-domain helper if available and use that to compute the registrable domain.
🧹 Nitpick comments (2)
cmuxTests/BrowserConfigTests.swift (1)
1576-1629: Add akeyDownvariant to this retargeting test suite.These tests currently exercise only
.keyUp, but the production helper accepts both.keyDownand.keyUp. Adding one same-siteGET+.keyDowncase would harden coverage for Enter-submit behavior.Proposed test addition
final class BrowserSimpleUserGesturePopupRetargetingTests: XCTestCase { + func testKeyboardKeyDownSameSiteGETWithoutPopupFeaturesPrefersNewTab() { + XCTAssertTrue( + browserNavigationShouldOpenSimpleUserGesturePopupInNewTab( + navigationType: .other, + requestMethod: "GET", + requestURL: URL(string: "https://search.bilibili.com/all?keyword=test"), + openerURL: URL(string: "https://www.bilibili.com/video/BV1"), + currentEventType: .keyDown, + popupFeaturesWereSpecified: false + ) + ) + } + func testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab() { XCTAssertTrue( browserNavigationShouldOpenSimpleUserGesturePopupInNewTab(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserConfigTests.swift` around lines 1576 - 1629, Add a new test in the BrowserSimpleUserGesturePopupRetargetingTests class that mirrors testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab but uses currentEventType: .keyDown; specifically call browserNavigationShouldOpenSimpleUserGesturePopupInNewTab with navigationType: .other, requestMethod: "GET", the same requestURL and openerURL, popupFeaturesWereSpecified: false, assertTrue that it returns true, and name it e.g. testKeyboardSameSiteGETKeyDownPrefersNewTab so the suite covers both .keyUp and .keyDown keyboard events.Sources/Panels/BrowserPanel.swift (1)
6260-6267: Redact query strings in these new DEBUG URL logs.These call sites log full request URLs, which can include search terms, OAuth codes, and other sensitive parameters. Scheme/host/path is usually enough for debugging navigation routing.
Suggested fix
- let requestURL = navigationAction.request.url?.absoluteString ?? "nil" + let requestURL = browserNavigationDebugURL(navigationAction.request.url) @@ - let requestURL = navigationAction.request.url?.absoluteString ?? "nil" + let requestURL = browserNavigationDebugURL(navigationAction.request.url)Add a small helper near the other
browserNavigation*helpers:private func browserNavigationDebugURL(_ url: URL?) -> String { guard let url, var components = URLComponents(url: url, resolvingAgainstBaseURL: false) else { return "nil" } components.query = nil components.fragment = nil return components.string ?? "\(url.scheme ?? "unknown")://\(url.host ?? "")" }Based on learnings: In Swift source files like
Sources/GhosttyTerminalView.swift, avoid logging raw sensitive input even in DEBUG and prefer non-sensitive metadata instead.Also applies to: 6453-6471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 6260 - 6267, The debug logs currently print full request URLs (see the dlog call that uses requestURL) which may leak sensitive query parameters; add a helper function browserNavigationDebugURL(_:) (as suggested) that strips components.query and components.fragment and returns components.string or a safe fallback, then replace uses of requestURL in the dlog formatting (and the other log at lines ~6453-6471) to call browserNavigationDebugURL(navigationAction.request.url) so only scheme/host/path are logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/reload.sh`:
- Around line 345-357: The teardown of the temporary FIFO and follower processes
must be moved into a trap so it always runs on script exit or interrupt: create
a trap on EXIT INT TERM that kills $TAIL_PID and $GREP_PID (if set), waits for
them, and removes $XCODE_FILTER_DIR/$XCODE_FILTER_FIFO; set $XCODE_FILTER_DIR,
$XCODE_FILTER_FIFO, $TAIL_PID and $GREP_PID before registering the trap so the
handler can reference them, and after the normal cleanup (the current
kill/wait/rm -rf block that runs after wait "$XCODE_PID") clear the trap to
avoid double-running the handler.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6005-6014: The helper browserNavigationHasKeyboardActivation
currently only treats .keyDown/.keyUp as user activation and misses
WebKit-reported mouse clicks that come through as .other; update
browserNavigationHasKeyboardActivation to also return true when currentEventType
== .other AND the actual NSEvent (NSApp.currentEvent) indicates a mouse click
(e.g. check NSApp.currentEvent?.clickCount > 0 or
NSApp.currentEvent?.buttonNumber >= 0) so that those mouse-triggered submits are
considered user activation; make the same change where the same logic is used
(the code around createWebViewWith referenced in the comment) so left-click
submits no longer fall through to createWebViewWith.
---
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6036-6047: The browserNavigationSiteKey function currently reduces
hosts to the last two labels (parts.suffix(2)) which treats public suffixes like
"co.uk" as registrable domains; change this to use exact host matching unless
you have a public-suffix-aware helper: in browserNavigationSiteKey(_:) (and the
analogous spot around the other occurrence), after validating host and handling
IPv4/port cases, return the full host string (host) instead of
parts.suffix(2).joined(separator: "."); alternatively wire a proper
publicSuffix/registrable-domain helper if available and use that to compute the
registrable domain.
---
Nitpick comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 1576-1629: Add a new test in the
BrowserSimpleUserGesturePopupRetargetingTests class that mirrors
testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab but uses
currentEventType: .keyDown; specifically call
browserNavigationShouldOpenSimpleUserGesturePopupInNewTab with navigationType:
.other, requestMethod: "GET", the same requestURL and openerURL,
popupFeaturesWereSpecified: false, assertTrue that it returns true, and name it
e.g. testKeyboardSameSiteGETKeyDownPrefersNewTab so the suite covers both .keyUp
and .keyDown keyboard events.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6260-6267: The debug logs currently print full request URLs (see
the dlog call that uses requestURL) which may leak sensitive query parameters;
add a helper function browserNavigationDebugURL(_:) (as suggested) that strips
components.query and components.fragment and returns components.string or a safe
fallback, then replace uses of requestURL in the dlog formatting (and the other
log at lines ~6453-6471) to call
browserNavigationDebugURL(navigationAction.request.url) so only scheme/host/path
are logged.
🪄 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: 0847121b-8576-4052-8fd9-ac8182ded5a2
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swiftscripts/reload.sh
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:6044">
P2: Same-site detection uses `suffix(2)` host parsing, which misclassifies many public-suffix domains (e.g. `*.co.uk`) as same-site.</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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6005-6014:⚠️ Potential issue | 🟠 Major
.otherclick activations can still miss this retarget path.This helper only treats literal
.leftMouseDown/.leftMouseUpas mouse activation. WebKit can surface click-driven.othernavigations whileNSApp.currentEventis still.other, so those submits still fall through to popup creation instead of the new-tab fallback.Suggested fix
func browserNavigationHasSimpleUserActivation( - currentEventType: NSEvent.EventType? = NSApp.currentEvent?.type + currentEventType: NSEvent.EventType? = NSApp.currentEvent?.type, + currentEventButtonNumber: Int? = NSApp.currentEvent?.buttonNumber, + currentEventClickCount: Int? = NSApp.currentEvent?.clickCount ) -> Bool { switch currentEventType { case .keyDown, .keyUp, .leftMouseDown, .leftMouseUp: return true + case .other: + return (currentEventClickCount ?? 0) > 0 || (currentEventButtonNumber ?? -1) >= 0 default: return false } }Also applies to: 6064-6065
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 6005 - 6014, The helper browserNavigationHasSimpleUserActivation currently only treats .leftMouseDown/.leftMouseUp as mouse activation, but click-driven navigations can be reported as .other; update browserNavigationHasSimpleUserActivation to also return true when currentEventType is .other and the actual NSEvent (use NSApp.currentEvent) has a clickCount > 0 (or otherwise indicates a mouse click), so clicks surfaced as .other are treated as activations; apply the same change where the function is called/duplicated (the similar handling around lines 6064-6065) so .other click events use the new-tab fallback instead of creating a popup.
🧹 Nitpick comments (1)
cmuxTests/BrowserConfigTests.swift (1)
1577-1588: Consider adding an explicit.keyDownpositive case for keyboard retargeting.Line 1584 currently validates keyboard behavior via
.keyUponly. A paired.keyDowntest would make the event-phase contract harder to regress.Suggested test addition
final class BrowserSimpleUserGesturePopupRetargetingTests: XCTestCase { + func testKeyboardKeyDownSameSiteGETWithoutPopupFeaturesPrefersNewTab() { + XCTAssertTrue( + browserNavigationShouldOpenSimpleUserGesturePopupInNewTab( + navigationType: .other, + requestMethod: "GET", + requestURL: URL(string: "https://search.bilibili.com/all?keyword=test"), + openerURL: URL(string: "https://www.bilibili.com/video/BV1"), + currentEventType: .keyDown, + popupFeaturesWereSpecified: false + ) + ) + } + func testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab() { XCTAssertTrue( browserNavigationShouldOpenSimpleUserGesturePopupInNewTab(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserConfigTests.swift` around lines 1577 - 1588, Add a paired positive test that validates keyboard retargeting for the keyDown phase: create a new test function mirroring testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab but set currentEventType: .keyDown and assert XCTAssertTrue on browserNavigationShouldOpenSimpleUserGesturePopupInNewTab with the same parameters (navigationType: .other, requestMethod: "GET", same requestURL and openerURL, popupFeaturesWereSpecified: false) so the keyDown event-phase behaviour is explicitly covered.
🤖 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 6036-6046: The current browserNavigationSiteKey(_:) uses
parts.suffix(2) which incorrectly reduces domains like foo.co.uk to co.uk;
replace that logic with a public-suffix-aware registrable-domain helper (e.g.,
registrableDomain(for: URL) or
domainRegistry.truncateToRegistrableDomain(host:)) that consults the Public
Suffix List (or an existing library) to compute the effective registrable
domain; update browserNavigationSiteKey(_:) to call that helper for
non-IP/non-port hosts and make the same replacement for the other similar helper
used at the second location (the function similar to browserNavigationSiteKey
around the later occurrence) so site-key logic uses the PSL-aware registrable
domain instead of parts.suffix(2).
---
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6005-6014: The helper browserNavigationHasSimpleUserActivation
currently only treats .leftMouseDown/.leftMouseUp as mouse activation, but
click-driven navigations can be reported as .other; update
browserNavigationHasSimpleUserActivation to also return true when
currentEventType is .other and the actual NSEvent (use NSApp.currentEvent) has a
clickCount > 0 (or otherwise indicates a mouse click), so clicks surfaced as
.other are treated as activations; apply the same change where the function is
called/duplicated (the similar handling around lines 6064-6065) so .other click
events use the new-tab fallback instead of creating a popup.
---
Nitpick comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 1577-1588: Add a paired positive test that validates keyboard
retargeting for the keyDown phase: create a new test function mirroring
testKeyboardSameSiteGETWithoutPopupFeaturesPrefersNewTab but set
currentEventType: .keyDown and assert XCTAssertTrue on
browserNavigationShouldOpenSimpleUserGesturePopupInNewTab with the same
parameters (navigationType: .other, requestMethod: "GET", same requestURL and
openerURL, popupFeaturesWereSpecified: false) so the keyDown event-phase
behaviour is explicitly covered.
🪄 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: f9eef8fa-472b-411e-89ef-e002fc6b538f
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6036-6046:⚠️ Potential issue | 🟠 MajorUse a PSL-aware registrable-domain helper here.
suffix(2)still collapses hosts likefoo.co.ukandbar.co.uktoco.uk, so genuinely cross-site popups can be treated as same-site and retargeted into the current tab. That breaks thewindow.opener/postMessagesemantics this change is trying to preserve.Also applies to: 6073-6077
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 6036 - 6046, The current browserNavigationSiteKey implementation uses naive suffix(2) logic which collapses multi-part public suffixes (e.g., foo.co.uk → co.uk); replace that logic with a PSL-aware registrable-domain lookup (call a Public Suffix List helper or library function to compute the registrable domain from host) and fall back to the original host when the helper cannot determine a registrable domain; do the same replacement for the duplicate logic at the other occurrence (the code block around the second mention, the lines corresponding to the similar host-suffix handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 1576-1642: Add a regression test that covers the public-suffix
edge case in the site-key logic (BrowserPanel.swift uses parts.suffix(2));
create a new test in BrowserSimpleUserGesturePopupRetargetingTests named
something like testPublicSuffixTwoLabelEdgeCaseDoesNotRetarget which calls
browserNavigationShouldOpenSimpleUserGesturePopupInCurrentTab with
request/opener pairs that share the same two trailing labels (e.g., requestURL
"https://foo.example.co.uk/..." and openerURL "https://bar.attacker.co.uk/..."
or similar) and assert FALSE (popup should stay popup), ensuring cross-site
origins with multi-part public suffixes are not treated as same-site.
---
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6036-6046: The current browserNavigationSiteKey implementation
uses naive suffix(2) logic which collapses multi-part public suffixes (e.g.,
foo.co.uk → co.uk); replace that logic with a PSL-aware registrable-domain
lookup (call a Public Suffix List helper or library function to compute the
registrable domain from host) and fall back to the original host when the helper
cannot determine a registrable domain; do the same replacement for the duplicate
logic at the other occurrence (the code block around the second mention, the
lines corresponding to the similar host-suffix handling).
🪄 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: f416b9c1-7126-465b-8367-373846936aab
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6043-6064:⚠️ Potential issue | 🟠 MajorUse a registrable-domain helper here.
This is still not public-suffix aware. Hosts like
foo.github.ioandbar.github.iocollapse to the same key here, so unrelated cross-site popups can be retargeted into a tab and losewindow.opener/postMessagesemantics. This needs a PSL-aware registrable-domain calculation instead of the hardcodedsuffix(2/3)heuristic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 6043 - 6064, The function browserNavigationSiteKey(_:) currently uses a simple suffix(2/3) heuristic and fails to respect the public suffix list, causing distinct registrable domains like foo.github.io and bar.github.io to collapse; replace the manual logic in browserNavigationSiteKey(_:) with a PSL-aware registrable-domain lookup (use an existing Public Suffix List/registrable-domain helper or add one as a dependency), call that helper to derive the site key for a given URL.host (fall back to host for IPv4/ports/unresolvable cases), and ensure you still handle IPv4 and host-empty guards the same way while removing the hardcoded parts/suffix logic.
🤖 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 4287-4301: The code is consuming the one-time insecure-HTTP bypass
on the new BrowserSurface before running the first navigation policy check,
because it calls panel.consumeOneTimeInsecureHTTPBypassIfNeeded(for: url) prior
to panel.navigateWithoutInsecureHTTPPrompt(...), which causes the bypass to be
lost for same-host redirects; remove or move that consume call so the bypass
remains armed until WebKit performs its first checked navigation. Specifically,
in the newBrowserSurface success path (workspace.newBrowserSurface / panel), do
not call _ = panel.consumeOneTimeInsecureHTTPBypassIfNeeded(for: url) before
invoking panel.navigateWithoutInsecureHTTPPrompt(request:request,...); instead
let the panel/navigation code consume the bypass at the appropriate policy check
time (or consume it after the first policy check completes).
- Around line 6525-6547: The current branch in
browserNavigationShouldOpenSimpleUserGesturePopupInCurrentTab can swallow
explicit new-tab gestures (Cmd-click or middle-click) because it runs before the
popup/new-tab fallback; update the logic so explicit new-tab intent wins by
checking for those intents before acting on the simple-user-gesture heuristic:
in the block using browserNavigationShouldOpenSimpleUserGesturePopupInCurrentTab
(referencing navigationAction.navigationType, navigationAction.request,
requestNavigation, and browserLoadRequest), detect explicit new-tab signals
(e.g. navigationAction.navigationType == .other combined with modifierFlags
containing .command or a middle-button click / whichever platform API indicates
a new-tab gesture, or an incoming explicit .newTab intent from
requestNavigation) and if present skip this simple-user-gesture branch
(returning nil only after letting the popup/new-tab fallback run) so
Cmd-click/middle-click open a new tab instead of loading in the current tab.
---
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6043-6064: The function browserNavigationSiteKey(_:) currently
uses a simple suffix(2/3) heuristic and fails to respect the public suffix list,
causing distinct registrable domains like foo.github.io and bar.github.io to
collapse; replace the manual logic in browserNavigationSiteKey(_:) with a
PSL-aware registrable-domain lookup (use an existing Public Suffix
List/registrable-domain helper or add one as a dependency), call that helper to
derive the site key for a given URL.host (fall back to host for
IPv4/ports/unresolvable cases), and ensure you still handle IPv4 and host-empty
guards the same way while removing the hardcoded parts/suffix logic.
🪄 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: 432865c0-975d-4787-a365-b72cc0e2f30f
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
3 issues found across 10 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="scripts/reload.sh">
<violation number="1" location="scripts/reload.sh:40">
P2: INT/TERM are trapped without exiting, so Ctrl-C/termination can be swallowed and the script may continue running after cleanup. Exit explicitly from signal traps to honor cancellation.</violation>
</file>
<file name="scripts/xcodebuild-guard.sh">
<violation number="1" location="scripts/xcodebuild-guard.sh:132">
P1: `prune_stale_xcodebuild_lock` can delete a live lock when metadata files are not written yet, allowing concurrent builds.</violation>
<violation number="2" location="scripts/xcodebuild-guard.sh:179">
P2: Manual `ps` line splitting is fragile and can mis-parse padded output, causing existing xcodebuild processes to be missed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
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 d7e2350. Configure here.
| "workspace=\(workspace.id.uuidString.prefix(5)) pane=\(paneId.id.uuidString.prefix(5))" | ||
| ) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Duplicated boilerplate between new-tab navigation methods
Low Severity
openNavigationRequestInNewTab duplicates nearly all of openLinkInNewTab's boilerplate — AppDelegate.shared guard, workspaceContainingPanel lookup, paneId lookup, newBrowserSurface call, and debug log scaffolding. The only meaningful difference is how the final navigation is dispatched (URL vs full URLRequest). Extracting the shared workspace/pane resolution into a small helper would reduce the maintenance surface and the risk of these two paths diverging.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d7e2350. Configure here.
austinywang
force-pushed
the
issue-2834-bilibili-search-popup
branch
from
d7e2350 to
cb1a6de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.othernil-target browser navigations back to tab fallback instead of detached popup windowsCloses #2834
Note
Medium Risk
Changes popup and new-tab routing logic in
WKNavigationDelegate/WKUIDelegate, which can subtly impact OAuth/opener flows and navigation edge cases. Guardrails (gesture/method/feature checks and host aliasing) plus added tests mitigate but don’t eliminate behavior-regression risk.Overview
Fixes bilibili-style search submits that were opening as detached popup windows by selectively retargeting
.otherwindow.open()navigations into the current tab when they look like simple user gestures (keyboard/left-click), areGET, have no popup chrome features, and match a small allowlist of same-scheme/port host aliases.New-tab navigations triggered from
WKUIDelegate.createWebViewWithnow open using the fullURLRequest(not justURL), including consuming the one-time insecure-HTTP bypass in the destination panel; debug logging was expanded to redact query/fragment and include method/target/window-features context.Removes the WebAuthn coordinator/bridge wiring from
BrowserPanel, and replaces the prior WebAuthn-focused unit tests with coverage for the new popup-retargeting decision logic and edge cases (cross-site/port/scheme, popup features, POST, cmd/middle-click).Reviewed by Cursor Bugbot for commit cb1a6de. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes bilibili search opening as a detached popup by routing simple user‑gesture
window.openGETs (same scheme/port, bilibili host aliases, no popup features) into the current tab. Keeps OAuth/scripted/cross‑site/cross‑port/POST/feature‑requested cases on the popup/new‑tab path and opens new tabs from the fullURLRequestwith correct insecure‑HTTP bypass. Closes #2834..otherkeyboard/left‑click nil‑target popups to the current tab when GET + same scheme/port + bilibili aliases + no features; Cmd/middle‑click or mismatched scheme/port/host/POST stay popups/new tabs.URLRequestand consume the one‑time insecure‑HTTP bypass in the destination; expand debug logs (method, redacted URL, target frame, window features); add regression tests for alias/origin checks and guardrails (cross‑site tenants, mixed scheme/port, popup features, POST).Written for commit cb1a6de. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores