Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): support OAuth popups in browser previews - #8413
fix(desktop): support OAuth popups in browser previews#8413MatthewFeroz wants to merge 4 commits into
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds OAuth popup authentication support and changes preview bootstrap navigation, popup window security, and navigation-history behavior across production desktop and web paths. An unresolved failure case can leave Back navigation enabled to return to the blank bootstrap page when loading fails or is superseded. You can add or adjust custom eligibility rules. Learn more. |
Co-authored-by: Matthew Feroz <136640686+MatthewFeroz@users.noreply.github.com>
There was a problem hiding this comment.
One concern with the new about:blank bootstrap in HostedBrowserWebview — see the inline note on the guest's navigation history / Back button state.
Posted via Macroscope — UI Consistency
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Matthew Feroz <136640686+MatthewFeroz@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort 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 76c040d. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Remove only the captured about:blank entry after the bootstrap load, and serialize that removal with preview state updates. This keeps newer navigation history intact and prevents detached guests from publishing stale Back state. Reported-by: Cursor Bugbot
There was a problem hiding this comment.
One finding on the preview guest bootstrap: the about:blank placeholder history entry is only cleaned up when the bootstrap load resolves, so a failed or aborted first load leaves the preview's Back button enabled and pointing at a blank page.
Posted via Macroscope — UI Consistency
| key={webviewGeneration} | ||
| ref={setWebviewRef} | ||
| src={webviewGeneration === 0 ? initialSrc : recoverySrc} | ||
| src="about:blank" |
There was a problem hiding this comment.
Pinning src to about:blank and deferring the real navigation to registerWebview makes the blank page a real guest history entry, and the cleanup for it only runs on success: in apps/desktop/src/preview/Manager.tsremoveBootstrapNavigationEntry is wired through Effect.tap on the loadURL promise, so it is skipped whenever that promise rejects (ERR_CONNECTION_REFUSED on a dev server that is not up yet, or ERR_ABORTED when a redirect/subsequent navigate supersedes the bootstrap load). Chromium still commits the error page / next page as entry 1, so syncWebContentsState publishes canGoBack: true and PreviewChromeRow's Back button stays enabled for the life of the tab, sending the preview back to a blank page. Before this change the target URL was the guest's first entry, so Back stayed disabled.
Smallest fix is on the desktop side: run the placeholder removal on both outcomes (e.g. Effect.onExit/ensuring instead of Effect.tap, still guarded by the existing generation/webContents identity checks), or drop the placeholder entry from a did-navigate/did-fail-load observation rather than from the loadURL resolution.
Posted via Macroscope — UI Consistency
t3dotgg
commented
Aug 28, 2026
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. We selected #8435 as the active OAuth popup path. Its current head installs If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |
Fixes#6561.
Browser previews denied every
window.open()request and loaded the requested URL in the opener. OAuth libraries therefore reported the popup as blocked, and the opener/child handshake could never run.This change permits real child windows for HTTP, HTTPS, and exact
about:blanktargets. Each child shares the opener session, runs sandboxed without Node or nested webviews, closes with its opener, and cannot create another popup. Unsafe and malformed URLs remain blocked through the shared preview URL validation.The preview webview now attaches at
about:blankand sends its intended URL through the typed registration IPC. The main process installs the popup policy before loading that URL, while preserving any newer navigation already queued for the tab.This ports the sound parts of #6620 to current
mainand includes the URL-validation intent from #6156.Verification:
vp test run apps/desktop/src/preview/Manager.test.ts apps/desktop/src/ipc/methods/preview.test.tsvp run --filter @t3tools/desktop typecheckvp run --filter @t3tools/web typecheckvp run --filter @t3tools/contracts typecheckvp lintandvp fmt --checkon all changed filesMade with GPT-5.6 Sol in T3 Code using the Codex harness.
Note
Medium Risk
Changes preview navigation, popup policy, and OAuth-related window behavior in Electron; mis-handled
initialUrlcould leave previews stuck onabout:blank, while popup session sharing affects login flows.Overview
Desktop browser previews no longer treat every
window.open()as navigation in the opener. PreviewManager installs asetWindowOpenHandlerthat allows sandboxed child windows for validatedhttp/httpsURLs and exactabout:blank, shares the guest session, parents them to the host window when possible, and denies unsafe schemes plus nested popups from auth windows.Registration bootstrap changes so popups are configured before the first real load:
registerWebviewgains optionalinitialUrlend-to-end (contracts IPC schema, preload, IPC handler). The hosted<webview>always mounts atabout:blankwithallowpopups="true"and passes the intended URL at registration; the main process loads it after listeners are attached, keeps loading state through blank guest events, and strips the placeholderabout:blankhistory entry when appropriate without racing newer navigations or replaced guests.Reviewed by Cursor Bugbot for commit 4a1d865. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Allow OAuth popups in desktop browser previews via main-process URL loading
initialUrlthat the main process loads after installing popup policy and listeners, replacing directsrcnavigation on the webview element.about:blankand normalizablehttp/httpsURLs; approved popups open in a separate sandboxedBrowserWindowwith nonodeIntegrationorwebviewTag, sharing the parent session. Nested popups are denied and child windows hide the menu bar.about:blankplaceholder entry is removed from navigation history after the bootstrap URL loads;did-stop-loadingfromabout:blankno longer overwrites a pending bootstrap navigation insyncWebContentsState.PreviewWebviewalias forcesallowpopupsas a string attribute so Electron receives it before guest attach, replacing the previous imperativesetAttributecall.desktopBridge.preview.registerWebviewand the IPC schema in ipc.ts now require a nullableinitialUrlargument; any caller not updated will fail type validation.Macroscope summarized 4a1d865.
request provenance