fix(desktop): support OAuth popups in browser previews - #8413

💭 Looking at HostedBrowserWebview.tsx, the current version at head 4a1d865 now includes bootstrap navigation entry removal logic, which resolves the earlier concern about about:blank causing canGoBack issues. I'll move on to checking the rest of the file for UI consistency.

1. browse_code — apps/web/src/browser/HostedBrowserWebview.tsx

2. git_tools

💭 I'm checking whether moving the webview's allowpopups attribute and src handling into React props, rather than setting them imperatively, could cause remounting or timing issues—particularly around the webviewGeneration key and how targetSrc/initialSrc interact in the effect dependencies. I'm also verifying whether the "webview" tag is properly declared in JSX.IntrinsicElements somewhere in the codebase, since the original code renders it with typed props like partition and webpreferences, and casting via ComponentProps<"webview"> would fail without that declaration. Now I'm spotting a real regression: since the webview's src is hardcoded to "about:blank", it no longer loads the URL directly and instead relies entirely on the bridge registration succeeding. If previewBridge is unavailable or registration fails, the preview will stay blank forever, whereas before it would still navigate via the src attribute as a fallback. For crash recovery, I'm checking that remounting with a new webviewGeneration key correctly computes the pendingUrl by preferring the current navStatus URL over initialUrl, which looks correct since it preserves the latest loaded URL after a crash. Now I'm digging into whether removeBootstrapNavigationEntry actually fixes the back-button issue -- the removal happens after loadURL resolves, so there's a window where the navigation history has both about:blank at index 0 and the real URL at index 1, meaning canGoBack briefly reports true and clicking Back would land on about:blank before the entry gets cleaned up. So actually there's a worse case: if loadURL fails and the mitigation effect never runs (since it's chained via ignore on failure), the about:blank history entry never gets removed, leaving Back permanently enabled to a blank page — that's a real persistent bug, especially likely when a dev server isn't ready yet. But this all lives in apps/desktop/src/preview/Manager.ts, which falls outside the include patterns for inline comments restricted to apps/web files. The root cause traces back to the web change (pinning src to about:blank creates the placeholder entry), so I could still post an inline comment on line 281 of HostedBrowserWebview.tsx pointing out that a failed bootstrap load leaves that placeholder entry stuck, letting Back navigate to a blank page. Let me double-check the desktop code path to make sure that's accurate before writing it up.

3. browse_code — apps/desktop/src/preview/Manager.ts

💭 I'm considering whether a failed loadURL call, like a connection refused error when the dev server isn't running, still commits an error page entry to history—which would make canGoBack true even though the intended page never loaded. This also matters for aborted navigations from redirects or user-initiated navigation, since a skipped tap in those cases could leave the Back button pointing at about:blank instead of the original target, a regression from before. But since the Manager.ts logic sits outside the files I'm allowed to modify, I'm weighing whether this counts as an in-scope UI consistency concern versus a desktop behavior issue outside my check's remit. Looking at the web file changes, I want to verify a couple things: whether moving allowpopups="true" into JSX still works given React only passes through string attributes on non-custom elements like "webview," and whether it now gets applied before the guest attaches rather than after via the ref callback, which was the original bug. I also want to check whether the type cast omitting "allowpopups" from webview's component props actually type-checks, since I'm not sure React's intrinsic elements even declare a "webview" tag.

4. git_tools — webview

5. git_tools — IntrinsicElements

💭 Actually, I recall now that React's type definitions do include a legacy webview entry via WebViewHTMLAttributes, which explains why that JSX previously typechecked with attributes like allowFullScreen and allowpopups. React treats unknown attributes with boolean true by warning and stripping them, so a cast workaround for allowpopups is reasonable rather than a bug. Looking at the effect's dependency array, re-running on targetSrc changes when recoverySrc updates just re-registers cleanly, which seems fine, though I want to check what happens during a crash scenario. On crash recovery, generation bumps and the key change forces a remount, which naturally reruns the registration effect—that path checks out. I'm now weighing whether keeping src="about:blank" during remount versus loading recoverySrc directly changes resilience if the main-process registration silently fails, but since both did-attach and dom-ready trigger re-registration, there's still a retry path so it's acceptable. I also want to verify that navigation within the preview before a crash is captured correctly via latestUrlRef, which it should be since it always holds the most recent URL for recovery purposes. I should also verify whether canGoBack is affected separately from navStatus—checking the code around line 1521 where canGoBack/canGoForward get computed from wc.navigationHistory. It seems during the bootstrap load, canGoBack briefly publishes as true right after the real URL commits, enabling the UI's Back button, then flips false once removeBootstrapNavigationEntry runs—so there's a transient flicker worth confirming. Now I'm considering the failure case: if loadURL rejects (e.g. dev server down, ERR_CONNECTION_REFUSED), the bootstrap entry never gets removed, and since Chromium commits error pages as their own navigation entries, canGoBack stays true. So on a preview tab with no dev server running, Back would be enabled and pressing it would just take you to about:blank. I think the practical fix belongs in the desktop Manager, cleaning up the placeholder entry on both success and failure paths rather than only on success. Since my review scope is only the web file, I'll flag this on the relevant line in HostedBrowserWebview.tsx, pointing to the desktop-side gap and suggesting the cleanup run unconditionally — though I'm second-guessing whether this really falls under a UI consistency review's scope. There's also the ERR_ABORTED case for aborted loads, which could also leave the about:blank entry surviving with Back enabled — so multiple reachable paths lead to the same stale-history symptom. Looking at the existing test for "does not clear history from a newer navigation," I want to check whether the abort scenario during manager.navigate's loadURL call is already handled by that ordering or if it's a genuinely distinct gap. Given this, I'll flag the about:blank line in the diff with a single inline comment describing the failure/abort path as a suggestion rather than a diff, since the actual fix spans other files. I'll mark the check as failed since I'm surfacing a real finding, and I'm confirming that this line is indeed part of the diff's changed lines before posting. I also want to differentiate this from an earlier comment on the same line by clarifying that I'm addressing the failure path specifically rather than duplicating. Let me verify how attemptPromise behaves with Effect.tap on failure, and check whether there's a did-fail-load handler that clears the placeholder—searching for "bootstrap" references in Manager.ts to confirm.

6. git_tools — bootstrap

7. git_tools — canGoBack

💭 I've confirmed the Back button's enabled state comes from canGoBack in the desktop state, so a failed load leaves the placeholder showing while Back stays clickable, producing a blank page. I'll place my comment on line 281 where the src gets pinned, since that's the actual pinning point, and keep it brief without a suggested diff since the real fix belongs in Manager.ts.

8. modify_pr — 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.