Fix window.open() and target=_blank not opening in new tab#693
Closed
lark1115 wants to merge 2 commits intomanaflow-ai:mainfrom
Closed
Fix window.open() and target=_blank not opening in new tab#693lark1115 wants to merge 2 commits intomanaflow-ai:mainfrom
lark1115 wants to merge 2 commits intomanaflow-ai:mainfrom
Conversation
…ai#606) When a page calls window.open() or uses target=_blank, createWebViewWith is invoked regardless of user gesture. The old code checked modifier keys and mouse button to decide new-tab vs in-place, which always chose in-place for programmatic calls. Now createWebViewWith always opens a new tab, since it is only called when the page explicitly requests a new window. Also fix the insecure HTTP navigation path to treat targetFrame==nil as new-tab intent, consistent with the secure path. Closes manaflow-ai#606
|
@lark1115 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. ���� ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBrowserPanel navigation and UI-delegate behavior adjusted so nil Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant WebPage as Web Page
participant WKWebView
participant BrowserPanel
participant OpenInNewTab as openInNewTab Handler
User->>WebPage: Trigger link/navigation or window.open(url, '_blank')
WebPage->>WKWebView: navigationAction / window.open call
WKWebView->>BrowserPanel: decidePolicyFor / createWebViewWith delegate
BrowserPanel->>BrowserPanel: Treat nil targetFrame or createWebViewWith as new-tab intent
BrowserPanel->>OpenInNewTab: Call openInNewTab(url)
OpenInNewTab->>OpenInNewTab: Create new tab / surface and load URL
OpenInNewTab-->>User: New tab displays URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanel.swift (1)
3243-3245: Recommended cleanup: remove now-dead conditional branches increateWebViewWith.Since Line 3245 hard-codes
shouldOpenInNewTab = true, the.currentTabandloadInPlacebranches at Lines 3271 and 3284-3288 are unreachable. Simplifying this will make the intent clearer and reduce maintenance risk.♻️ Proposed simplification
- // createWebViewWith is only called when the page requests a new window, - // so always treat as new-tab intent regardless of modifiers/button. - let shouldOpenInNewTab = true + // createWebViewWith is only called when the page requests a new window, + // so always treat as new-tab intent regardless of modifiers/button. + let intent: BrowserInsecureHTTPNavigationIntent = .newTab @@ - if let requestNavigation { - let intent: BrowserInsecureHTTPNavigationIntent = - shouldOpenInNewTab ? .newTab : .currentTab + if let requestNavigation { `#if` DEBUG dlog( "browser.nav.createWebView.action kind=requestNavigation intent=\(intent == .newTab ? "newTab" : "currentTab") " + "url=\(url.absoluteString)" ) `#endif` requestNavigation(navigationAction.request, intent) - } else if shouldOpenInNewTab { + } else { `#if` DEBUG dlog("browser.nav.createWebView.action kind=openInNewTab url=\(url.absoluteString)") `#endif` openInNewTab?(url) - } else { -#if DEBUG - dlog("browser.nav.createWebView.action kind=loadInPlace url=\(url.absoluteString)") -#endif - webView.load(navigationAction.request) }Also applies to: 3270-3289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 3243 - 3245, The conditional branches in createWebViewWith that handle .currentTab and the loadInPlace path are now dead because shouldOpenInNewTab is hard-coded true; remove the unreachable .currentTab handling and the loadInPlace branch (including any variables or comments only used for those cases) and simplify the function to always follow the new-tab flow (use the new-tab helper/logic that currently runs when shouldOpenInNewTab == true), keeping any shared setup and callbacks intact and updating comments to reflect the always-new-tab intent.
🤖 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/Panels/BrowserPanel.swift`:
- Around line 3243-3245: The conditional branches in createWebViewWith that
handle .currentTab and the loadInPlace path are now dead because
shouldOpenInNewTab is hard-coded true; remove the unreachable .currentTab
handling and the loadInPlace branch (including any variables or comments only
used for those cases) and simplify the function to always follow the new-tab
flow (use the new-tab helper/logic that currently runs when shouldOpenInNewTab
== true), keeping any shared setup and callbacks intact and updating comments to
reflect the always-new-tab intent.
Greptile SummaryFixed Key changes:
The implementation correctly addresses the bug where target=_blank links would navigate in-place instead of opening new tabs. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 048ba3a |
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sources/Panels/BrowserPanel.swift
Outdated
| for navigationAction: WKNavigationAction, | ||
| windowFeatures: WKWindowFeatures | ||
| ) -> WKWebView? { | ||
| let hasRecentMiddleClickIntent = CmuxWebView.hasRecentMiddleClickIntent(for: webView) |
There was a problem hiding this comment.
hasRecentMiddleClickIntent is computed but no longer used after setting shouldOpenInNewTab = true. Consider removing this line.
Address review feedback: remove unused hasRecentMiddleClickIntent, shouldOpenInNewTab constant, and unreachable loadInPlace branch. Simplify intent to always .newTab since createWebViewWith is only called for page-initiated window requests. Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Mar 2, 2026
Always open programmatic navigation (window.open, target=_blank) in a new tab. Fix insecure HTTP path to treat nil targetFrame as new-tab intent. Closes #606 Co-authored-by: lark <lark1115caster@gmail.com>
Contributor
|
Merged manually after rebasing to resolve conflicts. Thanks @lark1115! |
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
createWebViewWithis only called when the page requests a new window (window.open(),target=_blank). Always open in a new tab regardless of modifier keys/mouse button, since programmatic calls have no user gesture.targetFrame == nilas new-tab intent, consistent with the secure path.Closes #606
Test plan
window.open()— verify it opens in a new tabtarget=_blanklink — verify it opens in a new tabwindow.open()— verify the insecure HTTP prompt shows with new-tab intent🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes