Share browser context with OAuth popups#1600
Merged
lawrencecchen merged 2 commits intomainfrom Mar 20, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR refactors WKWebViewConfiguration setup in BrowserPanel into a reusable static helper method and applies it to browser popups so they inherit the opener's browser context, including process pool and website data store, with supporting tests added. Changes
Sequence DiagramsequenceDiagram
participant Opener as Opener Panel
participant Popup as BrowserPopupWindowController
participant Config as BrowserPanel<br/>(Helper)
participant WebView as WKWebView
Opener->>Popup: Create popup window
Popup->>Popup: Compute browserContextSource<br/>from parent/opener
Popup->>Config: configureWebViewConfiguration<br/>(config, websiteDataStore, processPool)
Config->>Config: Apply processPool,<br/>settings, scripts
Popup->>WebView: Create WKWebView<br/>with configured config
Popup->>WebView: Set background color<br/>from theme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanel.swift (1)
2376-2391: Make user-script registration idempotent for popup-provided configurations.When
WKUIDelegate.webView(_:createWebViewWith:for:windowFeatures:)is called, the providedWKWebViewConfigurationinherits the opener's user scripts. CallingconfigureWebViewConfiguration()at lines 97–101 in BrowserPopupWindowController unconditionally appends the telemetry and address-bar-focus scripts again, causing duplication on popup creation.Proposed fix
+ let existingSources = Set(configuration.userContentController.userScripts.map(\.source)) + if !existingSources.contains(Self.telemetryHookBootstrapScriptSource) { + configuration.userContentController.addUserScript( + WKUserScript( + source: Self.telemetryHookBootstrapScriptSource, + injectionTime: .atDocumentStart, + forMainFrameOnly: false + ) + ) + } // Track the last editable focused element continuously so omnibar exit can // restore page input focus even if capture runs after first-responder handoff. + if !existingSources.contains(Self.addressBarFocusTrackingBootstrapScript) { + configuration.userContentController.addUserScript( + WKUserScript( + source: Self.addressBarFocusTrackingBootstrapScript, + injectionTime: .atDocumentStart, + forMainFrameOnly: false + ) + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 2376 - 2391, Make user-script registration idempotent by checking for existing user scripts before adding the telemetry and focus scripts: when configuring a WKWebViewConfiguration (the call site that currently calls configuration.userContentController.addUserScript with Self.telemetryHookBootstrapScriptSource and Self.addressBarFocusTrackingBootstrapScript), inspect configuration.userContentController.userScripts and skip adding if a WKUserScript exists with the same source, injectionTime and forMainFrameOnly for each of Self.telemetryHookBootstrapScriptSource and Self.addressBarFocusTrackingBootstrapScript; apply this change where configureWebViewConfiguration is invoked (including BrowserPopupWindowController handling of WKUIDelegate.webView(_:createWebViewWith:for:windowFeatures:)) so popup-created configurations don’t accumulate duplicates.
🤖 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 2376-2391: Make user-script registration idempotent by checking
for existing user scripts before adding the telemetry and focus scripts: when
configuring a WKWebViewConfiguration (the call site that currently calls
configuration.userContentController.addUserScript with
Self.telemetryHookBootstrapScriptSource and
Self.addressBarFocusTrackingBootstrapScript), inspect
configuration.userContentController.userScripts and skip adding if a
WKUserScript exists with the same source, injectionTime and forMainFrameOnly for
each of Self.telemetryHookBootstrapScriptSource and
Self.addressBarFocusTrackingBootstrapScript; apply this change where
configureWebViewConfiguration is invoked (including BrowserPopupWindowController
handling of WKUIDelegate.webView(_:createWebViewWith:for:windowFeatures:)) so
popup-created configurations don’t accumulate duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a7e338d-42b6-4d4e-b10b-136bc192956a
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/GhosttyConfigTests.swift
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add popup browser context regression tests * Share browser context with OAuth popups --------- 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
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
Testing
xcodebuild test -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-google-oauth-shared-popup-context-tests -only-testing:cmuxTests/BrowserPanelPopupContextTests(fails on the test-only commit, passes with the fix)Task
Summary by cubic
Fix Google OAuth popups by sharing the opener browser context with popup
WKWebViews. OAuth windows now keep cookies/storage andwindow.opener/postMessagelinkage so sign-in flows (e.g., Figma) complete.Bug Fixes
processPoolandwebsiteDataStore, preserving cookies/storage and opener linkage required for OAuth.Refactors
BrowserPanel.configureWebViewConfiguration(...)to centralize and reuse WebView setup across panels and popups.Written for commit 4c66f46. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests