feat: support window.open() popup windows#1150
Merged
lawrencecchen merged 8 commits intomanaflow-ai:mainfrom Mar 16, 2026
Merged
feat: support window.open() popup windows#1150lawrencecchen merged 8 commits intomanaflow-ai:mainfrom
lawrencecchen merged 8 commits intomanaflow-ai:mainfrom
Conversation
Return a live WKWebView from createWebViewWith using WebKit's supplied configuration, preserving popup browsing-context semantics (window.opener, postMessage). This fixes OAuth/OIDC flows and any site relying on standard popup patterns. - Add BrowserPopupWindowController: NSPanel-based popup with self-retention, KVO title/URL, read-only URL label, nested popup depth limit (3), insecure-HTTP prompt parity, auth challenge parity, download delegate - Classifier: scripted requests (window.open) create popups; user-initiated actions (Cmd+click, middle-click, context menu) open tabs - Retarget context menu "Open Link in New Tab" to bypass createWebViewWith, wired in both main browser and popup web views - Cmd+W fast path in AppDelegate for popup windows - Opener panel owns popup lifecycle; close() tears down all child popups
Add BrowserPopupPanel (NSPanel subclass) that intercepts Cmd+W in performKeyEquivalent before the swizzled cmux_performKeyEquivalent can dispatch it to the main menu's "Close Tab" action. Also refine the popup classifier to reuse browserNavigationShouldOpenInNewTab for Cmd+click/middle-click detection, add download delegate wiring, and wire onContextMenuOpenLinkInNewTab for popup web views.
|
@SuperManfred 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:
📝 WalkthroughWalkthroughAdds a macOS popup window system: new BrowserPopupWindowController/NSPanel, BrowserPanel popup integration and decision logic, CmuxWebView context-menu hook, AppDelegate Cmd+W handling for popups, localization for a popup loading title, and unit tests for popup decisions and geometry. Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript (window.open)
participant UI as BrowserUIDelegate / BrowserPanel
participant PopupCtrl as BrowserPopupWindowController
participant Panel as BrowserPopupPanel (NSPanel)
participant Web as CmuxWebView
JS->>UI: createWebViewWith(configuration, windowFeatures)
UI->>UI: classify request (scripted popup vs tab)
alt scripted popup
UI->>PopupCtrl: openPopup(configuration, windowFeatures)
PopupCtrl->>Web: create CmuxWebView with configuration
PopupCtrl->>Panel: build NSPanel, add URL label + web view
PopupCtrl->>Web: set uiDelegate/navigationDelegate
PopupCtrl->>Panel: clamp/center and show window
PopupCtrl-->>UI: return CmuxWebView
UI-->>JS: return WKWebView (window.opener established)
else fallback (new tab / external)
UI->>UI: open in new tab or external handler
end
Web->>PopupCtrl: navigation / title / URL updates
PopupCtrl->>Panel: update title/URL label
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 unit tests (beta)
📝 Coding Plan
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: 3
🧹 Nitpick comments (1)
Sources/Panels/BrowserPopupWindowController.swift (1)
474-539: Consider addingdecidePolicyFor navigationResponsefor download handling parity.The main browser's
BrowserNavigationDelegateimplementsdecidePolicyFor navigationResponseto detect downloads viaContent-Disposition: attachmentheaders and non-renderable MIME types, explicitly returning.downloadpolicy. Without this, popup windows may attempt to render binary content instead of initiating downloads for certain MIME types.The
didBecome downloadhandlers are present and will work for WebKit-initiated download conversions, but explicit response-policy handling ensures consistent download behavior with the main browser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPopupWindowController.swift` around lines 474 - 539, Add a webView(_:decidePolicyFor:navigationResponse:decisionHandler:) implementation to PopupNavigationDelegate that mirrors the main BrowserNavigationDelegate: inspect navigationResponse.response (check Content-Disposition for "attachment" and the MIME type for non-renderable/binary types), call decisionHandler(.download) when those conditions are met and decisionHandler(.allow) otherwise, and ensure any download conversion continues to use downloadDelegate (the existing webView(_:navigationAction:didBecome:) / webView(_:navigationResponse:didBecome:) handlers will attach the delegate). This keeps PopupNavigationDelegate consistent with BrowserNavigationDelegate’s download policy 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 `@Sources/AppDelegate.swift`:
- Around line 7394-7401: The current conditional only checks event.window when
NSApp.keyWindow is nil, missing cases where the popup (identifier
"cmux.browser-popup") is present but not key; update the logic in the Cmd+W
handler to test both possible popup candidates (NSApp.keyWindow and
event.window) before falling back to the workspace-panel branch: explicitly
inspect both windows' identifier?.rawValue for "cmux.browser-popup" (e.g.,
iterate or check both variables) and call performClose(nil) and return true if
either matches, otherwise proceed to the existing fallback that uses
event.window ?? NSApp.keyWindow ?? NSApp.mainWindow.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2260-2266: The loop mutates popupControllers because closePopup()
triggers windowWillClose -> removePopupController(self); fix by iterating a
copy: create a local copy of popupControllers (e.g., let controllers =
popupControllers) and iterate controllers calling popup.closeAllChildPopups()
and popup.closePopup(), then call popupControllers.removeAll() after the loop;
reference popupControllers, closeAllChildPopups(), closePopup(),
windowWillClose, and removePopupController(self) to locate the code.
In `@Sources/Panels/BrowserPopupWindowController.swift`:
- Around line 67-71: The BrowserPopupWindowController currently enables
webView.isInspectable only inside a `#if` DEBUG block; remove the `#if` DEBUG /
`#endif` so the isInspectable assignment runs in all builds (but keep the
availability check if `#available`(macOS 13.3, *) remains) — update the code in
BrowserPopupWindowController where webView.isInspectable = true is set to match
BrowserPanel.swift behavior so popup windows also have Web Inspector enabled in
production.
---
Nitpick comments:
In `@Sources/Panels/BrowserPopupWindowController.swift`:
- Around line 474-539: Add a
webView(_:decidePolicyFor:navigationResponse:decisionHandler:) implementation to
PopupNavigationDelegate that mirrors the main BrowserNavigationDelegate: inspect
navigationResponse.response (check Content-Disposition for "attachment" and the
MIME type for non-renderable/binary types), call decisionHandler(.download) when
those conditions are met and decisionHandler(.allow) otherwise, and ensure any
download conversion continues to use downloadDelegate (the existing
webView(_:navigationAction:didBecome:) /
webView(_:navigationResponse:didBecome:) handlers will attach the delegate).
This keeps PopupNavigationDelegate consistent with BrowserNavigationDelegate’s
download policy handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae6bad70-15c9-4f98-9d11-a22247001060
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView.swift
Contributor
Greptile SummaryThis PR adds full Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Page as Web Page (JS)
participant WK as WebKit
participant BUD as BrowserUIDelegate
participant BP as BrowserPanel
participant BPWC as BrowserPopupWindowController
participant PUD as PopupUIDelegate
participant PND as PopupNavigationDelegate
Page->>WK: window.open(url)
WK->>BUD: createWebViewWith(configuration, navigationAction)
BUD->>BUD: classify: isScriptedPopup?
alt scripted (navigationType == .other && !isUserNewTab)
BUD->>BP: openPopup(configuration, windowFeatures)
BP->>BPWC: init(configuration, windowFeatures, openerPanel)
BPWC->>BPWC: create CmuxWebView + NSPanel
BPWC->>BPWC: makeKeyAndOrderFront
BPWC-->>BP: controller
BP-->>BUD: controller.webView
BUD-->>WK: popupWebView (live)
WK->>PND: decidePolicyFor(navigationAction)
PND->>PND: check external / insecure HTTP
PND-->>WK: .allow
WK->>BPWC: loads URL in popup webView
else user-initiated (Cmd+click / middle-click)
BUD->>BP: openInNewTab(url)
BP-->>BUD: nil
BUD-->>WK: nil (no new window)
end
Note over Page,BPWC: Nested popup from popup window
Page->>WK: window.open() inside popup
WK->>PUD: createWebViewWith(...)
PUD->>BPWC: createNestedPopup(configuration, windowFeatures)
BPWC->>BPWC: depth check (max=3)
BPWC->>BPWC: init child BrowserPopupWindowController
BPWC-->>PUD: child.webView
PUD-->>WK: child webView
Note over BPWC,BP: Cleanup on popup close
BPWC->>BPWC: windowWillClose
BPWC->>BPWC: closeAllChildPopups()
BPWC->>BP: removePopupController(self)
|
![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 6 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/BrowserPopupWindowController.swift">
<violation number="1" location="Sources/Panels/BrowserPopupWindowController.swift:96">
P2: Convert the JavaScript `top` coordinate to AppKit's bottom-up coordinate system.</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
🤖 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/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2350-2363: The test testExplicitCoordinatesClampToVisibleFrame
only checks origin clamping but not size clamping; update the call to
browserPopupContentRect so requestedWidth and requestedHeight exceed the
visibleFrame (e.g., set requestedWidth > visibleFrame.width and requestedHeight
> visibleFrame.height) and then change the assertions to expect rect.width and
rect.height to equal the visibleFrame's width and height (with accuracy), and
keep/assert the clamped origin values as appropriate; reference
browserPopupContentRect and testExplicitCoordinatesClampToVisibleFrame when
locating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c57b43bc-5d8f-4447-a7b8-c3cfa1a1ea6b
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
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 `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2302-2332: Add a test in BrowserPopupDecisionTests to cover the
plain .linkActivated case: implement a new test method (e.g.,
testLinkActivatedPlainClickDoesNotCreatePopup) that calls
browserNavigationShouldCreatePopup(navigationType: .linkActivated,
modifierFlags: [], buttonNumber: 0) and asserts false with XCTAssertFalse;
follow the style of the existing tests (testOtherNavigationPlainLeftClick,
testLinkActivatedCmdClickDoesNotCreatePopup) so the
.linkActivated-without-modifiers branch is pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c41e4ea8-0512-470c-bacd-92ca1d0862c5
📒 Files selected for processing (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift
…-windows # Conflicts: # Sources/AppDelegate.swift
This was referenced Mar 16, 2026
2 tasks
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* feat: support window.open() popup windows (manaflow-ai#742) Return a live WKWebView from createWebViewWith using WebKit's supplied configuration, preserving popup browsing-context semantics (window.opener, postMessage). This fixes OAuth/OIDC flows and any site relying on standard popup patterns. - Add BrowserPopupWindowController: NSPanel-based popup with self-retention, KVO title/URL, read-only URL label, nested popup depth limit (3), insecure-HTTP prompt parity, auth challenge parity, download delegate - Classifier: scripted requests (window.open) create popups; user-initiated actions (Cmd+click, middle-click, context menu) open tabs - Retarget context menu "Open Link in New Tab" to bypass createWebViewWith, wired in both main browser and popup web views - Cmd+W fast path in AppDelegate for popup windows - Opener panel owns popup lifecycle; close() tears down all child popups * fix: Cmd+W closes only the popup, not the parent tab Add BrowserPopupPanel (NSPanel subclass) that intercepts Cmd+W in performKeyEquivalent before the swizzled cmux_performKeyEquivalent can dispatch it to the main menu's "Close Tab" action. Also refine the popup classifier to reuse browserNavigationShouldOpenInNewTab for Cmd+click/middle-click detection, add download delegate wiring, and wire onContextMenuOpenLinkInNewTab for popup web views. * fix: tighten popup routing and window behavior * test: cover oversized popup frame clamping * test: cover plain link-activated popup routing --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
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
WKWebViewfromcreateWebViewWithusing WebKit’s supplied configuration so scripted popup flows preserve popup browsing-context behavior, including a workingwindow.openerBrowserPopupWindowController: standaloneNSPanelpopup windows with opener-owned lifecycle, nested popup depth limit, title/URL updates, and a read-only URL labelwindow.open()requests to popup windows while keeping user-initiated new-window actions as tabs, including from popup contextsOpen Link in New Tabso it always opens a real tab, including when invoked from a popup or nested popupCmd+Wclose only the focused popup window and safely tear down opener-owned popupsCloses #742
Verification
Manual verification:
window.opener is: SETOpen Link in New Tabopens a tab in the main browserOpen Link in New Tabinside a popup opens a tab in the opener’s main browserOpen Link in New Tabinside a nested popup opens a tab in the opener’s main browserlocalhostloads without an insecure-HTTP prompt under the default allowlistCmd+Wcloses only the popup windowNot yet manually verified:
Local build:
./scripts/reload.sh --tag popup-pr1150Demo Video
Screen.Recording.2026-03-10.at.19.50.29.mov
Summary by CodeRabbit
New Features
Tests