💭 I'm looking at the diff involving hosted browser webview visibility, activity leases, PiP flags, and preview automation host changes. There's a concern that a removed comment explained an important Electron constraint—that guest CSS visibility needs to stay active even offscreen because CDP Runtime/Input commands can stall under visibility:hidden—and I want to check whether this PR preserves that intentional exception. The new renderingActive flag seems designed to address this by keeping visibility active during background activity, recording, or PiP, but I need to trace through the actual code to see if there's still a path where offscreen automation could run without an active lease.

1. browse_code — apps/web/src/browser/hostedBrowserWebviewStyle.ts

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

💭 I'm worried that removing the visibility:hidden CDP stall comment and gating on renderingActive might break consumers doing offscreen automation without an activity lease, like browser tool calls or preview bridges. I need to check PreviewAutomationHosts.tsx and other paths that render HostedBrowserWebview to see what else depends on it. I should also grep for readRenderedViewport, executeJavaScript, and other places running CDP against tabs, including server-driven browser actions.

3. browse_code — apps/web/src/components/preview/PreviewAutomationHosts.tsx

💭 I'm noticing that the status operation for a hidden background tab now skips the viewport read entirely, since renderingActive gates that path before requireReadyTab is even reached. That's a behavior change worth flagging—status previously reported viewport info regardless of visibility, so this could be seen as a regression in UI consistency even if it's arguably intentional. Checking the visibility logic further, I confirm that when the wrapper is actively rendering, the style resolver omits the visibility key entirely, so React clears it and the check correctly treats empty string as "not hidden." There's also a subtle race worth noting: the lease is acquired using a runtimeTabId derived from the server epoch, so if that epoch changes mid-request, the lease could end up bound to a stale tab—a minor but real edge case. I'm also verifying the mini player path—it likely registers through the same browserSurfaceStore visibility/rect mechanism, so it should behave consistently with other active-surface consumers. Beyond that, I want to check other consumers of offscreen webviews, like screenshot capture tools or main-process CDP screencasting, to make sure they don't assume visibility state incorrectly outside the paths already covered by recording. Now I want to trace ElectronBrowserHost's pictureInPicture logic and confirm its dependency list correctly includes desktopByTabId, and double-check that requireReadyTab correctly resolves the runtime tab ID matching the active tab in the "open" flow.

4. browse_code — apps/web/src/browser/ElectronBrowserHost.tsx

5. browse_code — apps/web/src/browser/browserSurfaceStore.ts

💭 I should also check other places that drive the offscreen webview, like screenshot tools for agents, automation snapshots, annotation capture, and screencast recording, plus any thumbnail capture in the mini player. I want to grep for automation calls and screenshot capture to see if the web app invokes previewBridge automation directly outside the usual hosts, such as UI buttons for screenshots.

6. git_tools — previewBridge.(automation|navigate|capture|setColorScheme)

7. git_tools — acquireBrowserSurfaceActivity|activityByTabId

💭 That navigation call looks like user-driven behavior in a visible panel, so it's fine. I'm now checking other consumers of the offscreen webview—like the recording path, which goes through screencast via desktopBridge rather than previewBridge—and tracing which agent browser tools invoke start-related calls that might need similar handling. For the standalone "status" operation, no lease is held, so a background tab won't return a viewport—that seems like intentional behavior to avoid stalling automation calls rather than a bug. I'm now looking for concrete UI regressions worth flagging, starting with the removal of an explanatory comment tied to the visibility change. That comment explained why rendering must stay active for offscreen automation, recording, and PiP cases, and the guidelines specifically call out preserving Electron-constraint comments—so losing it while replacing the behavior with an explicit gate seems like a plausible intended finding, though a mild one. A second candidate issue: the automation code now reads inline visibility style directly off the DOM element, which is presentation state owned by a different module. That's essentially scraping a CSS implementation detail instead of reading from a shared store, which looks like an ownership boundary violation. If that styling module ever switches from inline styles to something like a class, display, or content-visibility, the automation gate would silently break since style.visibility would just return empty and be treated as "rendering." I think this is a solid, concrete finding worth flagging since it touches changed lines directly. I'm also noting this check doesn't account for computed visibility from hidden ancestors, though since styles are applied via inline React props that's less of an immediate concern given the polling-based wait logic already in place. I'm now considering whether there's an issue with the webview staying mounted even when inactive and not rendering, checking if that's actually fine as-is. Looking at the active-state style branch, I'm confirming that omitting the visibility key when transitioning to active just lets React clean up the property naturally, which should be fine. I'm also checking that pictureInPicture state reads consistently from the same source as zoomFactor, and that looks correct too. For recording ordering, I want to verify what happens if starting the screencast fails after publishing — specifically whether the activeRecordings entry gets properly removed and republished — so I'm going to look at the relevant section of browserRecording.ts.

8. browse_code — apps/web/src/browser/browserRecording.ts

9. browse_code — apps/web/src/browser/browserRecording.ts

💭 I'm wondering about a timing subtlety: since startScreencast fires right after the tab id atom updates but before React actually commits the visibility change, does the guest webContents still deliver its first compositor frame if the host is visibility:hidden at that instant? Turning to the bigger UI-consistency concern: isPreviewWebviewRendering in PreviewAutomationHosts reads the DOM's inline style.visibility directly instead of deriving from the same state sources the presentation layer owns (activity, byTabId.visible, recording atom, PiP). That's fragile — if the visibility mechanism ever shifts away from inline styles, wrapper.style.visibility would just return an empty string and silently break the automation gate. I also want to flag preserving the comment explaining why renderingActive must stay true for offscreen automation/recording/PiP, since it documents a real Electron CDP-stall constraint that shouldn't get lost. There's also a potential behavioral regression: currentStatus used to return rendered viewport info for background (offscreen-but-invisible) tabs, but now returns none since hidden tabs skip readRenderedViewport to avoid the executeJavaScript stall — likely intentional, but worth confirming it doesn't break callers relying on viewport data for background tabs. Checking through the remaining changes: waitForDesktopOverlay now gates on isPreviewWebviewRendering, but since callers route through requireReadyTab which acquires the lease first, this looks consistent; the aria-hidden and pointer-events handling in HostedBrowserWebview remain unchanged and fine, and I'm now looking at how useActiveBrowserRecordingTabIds() is wired into that component. I want to trace whether the mini player for a background thread relies on the desktop overlay presentation, since if visibility now flips to false whenever a background thread isn't presenting, a previously offscreen-but-painting mini player could stop rendering — so I need to check previewMiniPlayerStore and the mini player component to see how they source their live thumbnail.

10. browse_code — How does the preview mini player (usePreviewMiniPlayerStore / PreviewMiniPlayer component) display the browser tab content in apps/web/src? Does it rely on the HostedBrowserWebview being painted (e.g. acquireBrowserSurface presenting a rect with visible=true), or does it use a separate screencast/thumbnail? Explain which component acquires the browser surface for the mini player and whether it works for background (non-active) threads.

💭 Confirming visibility:hidden on the wrapper affects the whole subtree including the webview, but that's fine since switching threads reactivates it. Now I want to check the navigate logic in PreviewView.tsx around line 165 for related issues. Checking whether waitForNavigationReadiness is invoked there without an activity lease — but since the panel would be visible when navigating, that path seems fine. I'm now scanning for a real gap: some path where automation or CDP operations run against a hidden webview without a lease held. So far, status checks, viewport waits during resize, and browser recording start/stop all appear gated correctly by renderingActive or recordingActive. I still need to verify the stop-recording finalize step properly extends the lease. Now I want to check whether the automation status calls in waitForDesktopOverlay occur only after a visibility check, and whether usePreviewBridge does any CDP-style work like sending viewport/zoom metrics without a lease — probably just registration, so likely fine. I should also look for screenshot capture code referencing runtimeTabId to see if that path is covered.

11. git_tools — screenshot|captureVisible|capturePage

💭 I'm concluding the screenshot capture happens outside this scope, and now focusing my findings on the deleted comment explaining a constraint that should have been preserved, plus checking whether the diff respects CSS/component ownership rules. I keep coming back to isPreviewWebviewRendering reading a DOM inline style set by another module as a genuine issue — it's a fragile cross-module coupling that should instead derive its answer from shared state like activityByTabId or the recording/PiP status. There's also a subtler correctness concern: the check for wrapper.style.visibility !== "hidden" may behave unexpectedly if the wrapper element itself is missing, which I want to verify before flagging it as a real bug. Native PiP shouldn't be an issue since it likely paints from the same webContents. But I'm now worried about the case where hasWebContents is false before the webview registers — the wrapper starts hidden, which could prevent the webContents from ever attaching or painting since visibility:hidden may block Electron's guest view rendering. The key distinction here is display:none vs visibility:hidden — webviews generally still attach and load under visibility:hidden since it's display:none that historically blocks attachment, so the compositor should still be able to render even while hidden. I shouldn't claim a deadlock without evidence, so I'll hold off on that. Now weighing which findings are worth reporting — the inline style ownership issue seems solid, and the removed comment about visibility:hidden matters because it documented real Electron behavior: CDP Runtime/Input commands can stall while status queries still succeed, which is critical context for anyone debugging offscreen automation later. Looking at waitForDesktopOverlay, it correctly checks rendering state before calling status, and currentStatus calling into automation.status even when hidden lines up with that stale-metadata behavior. But there's a subtler issue: HostedBrowserWebview derives rendering state from React, while the automation gate checks the DOM directly, and there's a window where requireReadyTab grabs a lease before the re-render catches up. I'm also checking if concurrent requests on the same tab could collide — each gets its own lease so refcounting should handle that fine. But the ??= pattern for release means if a request re-acquires with a different runtimeTabId mid-request (say the epoch changes), the second lease is silently skipped, leaving the new tab hidden and the operation stuck until timeout — an edge case, but worth noting. There might be a more visible bug around visibility staying hidden while active state disagrees. I want to flag isPreviewWebviewRendering in PreviewAutomationHosts.tsx, which infers rendering state by reading inline styles from another module's DOM output — I'd suggest exporting a shared predicate from the browser surface module instead of relying on style inspection. There's also a silent-failure risk: if that inline style.visibility check ever gets swapped for a class or different wrapper element, the gate would silently report "rendering" and reintroduce the stalled-CDP wait. A safer fix would be having HostedBrowserWebview expose an explicit data-preview-rendering attribute as a real contract rather than relying on incidental DOM structure, or better yet reading the state directly from shared state. I also want to flag the deleted comment in hostedBrowserWebviewStyle.ts explaining the Electron constraint — worth restoring a short note clarifying that offscreen guests must stay visible whenever automation, recording, or PiP holds renderingActive, since otherwise CDP Runtime/Input commands stall even though status metadata stays available. Weighing whether these findings warrant failing the check, I think the cross-module coupling issue is genuine enough to count as a real risk, and since any findings push the state to failure, I'll mark this as a failure with these two points. I'm also noting a fragility concern: the check comparing wrapper style visibility treats "unknown" states ambiguously, which adds to my confidence in flagging this. There's also a third concern worth mentioning — the status operation no longer reports viewport size for hidden tabs, a quiet behavior change without any covering test. I'll fold that into the summary as a minor note rather than a full inline comment, since the more significant issues already have test coverage gaps addressed. Now I'm pinning down exact line numbers for the two review comments — lines 127-130 in PreviewAutomationHosts.tsx and line 48 in hostedBrowserWebviewStyle.ts — confirming both files fall within scope before posting the review.

12. modify_pr — UI consistency review: two ownership findings on the new offscreen-rendering gate. The behavior change itself (suspending paint for idle offscreen guests) looks sound; the concerns are about where the "is this guest painting?" fact lives and about the Electron constraint documentation that was dropped with the old visibility: "visible" line.