💭 I'm checking the current PR diff for the web files in scope, particularly HostedBrowserWebview.tsx, PreviewAutomationHosts.tsx, and previewGuestViewport.ts, and confirming that earlier flagged issues—like the PreviewView.tsx duplication and the rAF timing problem—have since been consolidated into HostedBrowserWebview. tsx with the addition of hasWebContents gating around line 207.

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

2. browse_code — apps/web/src/components/preview/previewGuestViewport.ts

💭 I'm considering whether applying the viewport override on every drag frame could cause a performance issue, since effectiveViewport changes on each pointermove and would keep re-triggering the effect that syncs guestViewportRef. current — but canceling the previous rAF and scheduling a new one coalesces this to one CDP call per frame, which seems reasonable. I also check that CSS pixel sizing accounts correctly for zoomFactor scaling in the webview wrapper. I'm now flagging a couple of concerns: mutating guestViewportRef.current directly during render is a minor anti-pattern that could write a stale value if a render gets discarded under concurrent mode, and I want to double check how the registerWebview path interacts with the effect that applies the ref value afterward. I'm also questioning whether desktopByTabId is keyed by the server tabId or the runtime tabId, since HostedBrowserWebview distinguishes between them and using the wrong key could break the hasWebContents gating. I need to trace this through previewStateStore, and separately I want to verify whether device metrics overrides properly account for viewportScale when the layout is scaled, since applying them to effectiveViewport without that scaling could cause a visual mismatch. I should check resolveFittedBrowserViewport in browserViewportLayout.ts to see if there's a real mismatch between the effectiveViewport dims applied to the guest and the fitted source viewport used for the webview element's CSS sizing. This PR is mostly logic changes around CDP viewport override rather than styling, so the most relevant rule category is visual/layout preservation—making sure responsive geometry and panel/inline-preview modes stay intact. Since prior runs have flagged issues each time, I want to look closely at whether this edge case actually breaks anything.

3. browse_code — apps/web/src/browser/browserViewportLayout.ts

4. browse_code — apps/web/src/browser/useBrowserViewportResize.ts

💭 I'm now working through the fitSourceContent case, where the fitted source viewport is derived from the source content itself and fed into resolveBrowserViewportLayout to compute the element's width relative to zoom and scale. Since the setting resolves to "fill", the guest override effect clears the override entirely, meaning the guest simply reports its own element size — which matches the fitted size, so the behavior stays consistent. I check the non-fill case too and confirm fittedSourceViewport just returns the setting unchanged, so both paths align. Then I trace through how zoomFactor affects the rendered width: the element's CSS width ends up equal to the setting's width times zoom, since the scale factor cancels out between the viewport width and viewport scale calculations. I'm now considering whether Electron's device metrics override interacts oddly with zoom factor, but that's a desktop-specific concern outside this file's scope, so I set it aside. Turning to the actual in-scope code, I'm looking at the effect around lines 210-219 in HostedBrowserWebview to spot real issues — one candidate is that assigning to guestViewportRef.current during render is a side effect in the render body, though it's more of a style nit than a UI consistency bug. A more concerning candidate is that the override might never get cleared on unmount or tab switch, which could leave stale state when the tab lease is released. I'm checking whether switching runtimeTabId properly clears overrides — since each tab maps to its own webContents, the cleanup that only cancels the animation frame seems fine there. I'm also confirming that transitioning viewport to "fill" correctly triggers the clear flag, which it does. Now I want to verify how hasWebContents is derived from the thread preview state's desktopByTabId map, so I need to check whether that's keyed consistently by server tabId in the preview state store.

5. git_tools — desktopByTabId

💭 I confirm desktopByTabId is keyed correctly by server tab id. Now I'm noticing that using useThreadPreviewState without a selector would subscribe to the entire state object, potentially causing HostedBrowserWebview to re-render on every preview event—I should check the hook's signature to see if that's actually a performance concern.

6. browse_code — apps/web/src/previewStateStore.ts

7. git_tools — export (function|const) useThreadPreviewState

💭 I'm realizing this is a genuine regression: useThreadPreviewState subscribes to the whole preview atom, so HostedBrowserWebview now re-renders on every preview event—navigation, loading, title, favicon, audible, zoom—whereas before it only tracked the browser surface store and webview config with a shallow selector. Since this component is mounted per tab app-wide, that broadened subscription could cause unnecessary re-renders across all tabs. Though this is more of a soft performance concern than a UI consistency issue per the rubric's focus on component system, theming, and layout preservation, I'm noting that the syncContentPresentation callback already gets a fresh identity every render because layout is recreated each time, triggering the presentation effect regardless—so this new subscription just adds more triggers to an existing pattern rather than causing a new loop. I want to move on to checking whether the guest override actually applies when the preview state changes. Given the PR title references a hidden-viewport fix, I'm now suspecting the real regression is applying device metrics overrides on every drag frame during resize, toggling mobile emulation mid-drag as the viewport crosses the breakpoint, which forces repeated re-layouts. This seems to be the actual behavior the fix is targeting, so I want to read through the PreviewAutomationHosts changes in full to confirm.

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

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

💭 I'm noticing a potential duplicate update: automation resize triggers the agent-controlled viewport path, which then propagates through the server snapshot and causes the human-controlled effect to fire again with the same value. Since both paths end up setting the same viewport, it's redundant but harmless—though the firing order between them isn't guaranteed. Now I want to refocus on what the UI consistency reviewer should actually flag here, since this PR touches shared component behavior but has essentially no styling changes. The relevant question is around visual and layout preservation — specifically whether hidden background tabs in "fill" mode still report incorrect fallback dimensions to the guest, since only pinned viewport tabs get the fix while fill tabs may still measure against the offscreen wrapper's fallback size when hidden. I'm also considering the ref mutation during render (guestViewportRef.current = effectiveViewport), which React discourages — in an interrupted or abandoned concurrent render, this could leave the ref holding a viewport value that was never actually committed to the DOM, causing a later registration to apply a stale, uncommitted viewport. The bigger issue is that the effect re-applying the viewport override only depends on guestViewportKey, hasWebContents, and runtimeTabId — it never re-runs when the webview element itself is recreated during crash recovery. That's covered separately though, since register() runs on the new element mount effect (which does depend on webviewGeneration) and re-registers the webview after the generation bump. But there's a subtler problem: hasWebContents is derived from thread preview state keyed by server tab id, while bridge calls all use runtimeTabId. If the desktop runtime restarts and runtimeTabId changes, the stale hasWebContents flag tied to the old tab id could still read true, causing the effect to fire with the new runtime id before the webview has actually re-registered. Considering whether double-application causes visible issues -- register applies the override on mount, then the effect reapplies once hasWebContents flips true, redundant but harmless for fill since it just sends clear commands twice. The real concern is that the register path unconditionally calls applyViewportOverride for every tab, including plain fill previews, and that function attaches the Chrome debugger via ensureControlSession -- meaning every webview mount attaches a debugger even when device metrics overrides are never needed. That's actually significant: attaching the debugger can conflict with DevTools (Electron blocks attaching if DevTools is open, and vice versa) and adds unnecessary overhead just to clear a non-existent override. I want to check ensureControlSession in Manager.ts to see if there's already a guard like isDevToolsOpened, and the likely fix is to skip applying the override in HostedBrowserWebview when the setting is fill at mount time.

10. git_tools — const ensureControlSession = ",|const ensureControlSession

💭 I realize that ensureControlSession now attaches a debugger session on every webview attach, even in fill mode where the call is essentially a no-op. That means if the user has DevTools open on the preview guest, the attach silently fails and gets swallowed by the catch handler, which could cause real viewport changes to fail quietly. Also, attaching the debugger at mount time could interfere with the user later opening DevTools on that guest, since Electron detaches the debugger when DevTools opens — previously this only mattered for the agent path, but now it applies to every preview tab. I want to check whether the effect uses applyViewportOverride deliberately to skip the control-session badge logic, and whether the debugger attach itself has side effects like enabling Page/Runtime/Input domains that could matter beyond the badge. Let me look at what createControlSession actually does under the hood.

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

💭 I'm realizing that ensureControlSession attaches a full heavyweight debugger session with Network, Accessibility, Runtime, and Log domains enabled — and the new mount-time applyPreviewGuestViewport call now triggers this for every preview webview at attach time, even fill tabs where the actual command does nothing. The real cost here is twofold: unnecessary network/accessibility overhead on every preview, and it blocks the user from opening DevTools on the guest since Electron won't allow two debugger clients — this silently errors out with PreviewAutomationDevToolsOpenError, swallowed. The fix should skip attaching for fill settings unless an override was already applied, tracking whether a previous override exists so clearing only happens on transition away from one. I'm deciding where to place the comment — likely on the mount-path lines in HostedBrowserWebview.tsx covering both the register block and the effect, though I'm second-guessing whether this counts as a "UI consistency" issue per the rubric. Given the intro's goal of preserving interaction, theming, accessibility, layout, and browser behavior, attaching a debugger to every guest does affect browser behavior since it impacts DevTools availability, so I think this is a legitimate finding — consistent with how prior runs treated behavioral issues like the agent badge and rAF race as fair game. I want to verify this isn't already a known/handled pattern elsewhere, so I'm checking whether some other path — like previewBridge, diagnostics, or console capture — already attaches at mount by searching for other usages of ensureControlSession.

12. git_tools — ensureControlSession|withControlSession

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

💭 I notice restoreControlSession only restores color-scheme on debugger reattach, not viewport overrides. This means if a user opens DevTools on a preview guest with a fixed viewport, detaching the debugger loses the device metrics override, and reattaching won't restore it — the guest silently reverts to host dimensions while the toolbar still displays the fixed viewport, creating a mismatch. But that fix lives in apps/desktop, outside scope for this review. Since I can only comment on apps/web/src files, I should focus there: gating mount-time debugger attachment for non-fill settings would prevent the regression where every preview tab (including default fill tabs, which previously had no debugger attached) now gets one attached — a real behavior and performance concern, especially since attaching the debugger affects how DevTools can be opened on that webContents. That's a real, concrete cost for fill previews specifically. I check whether the control session affects the controller badge state, but the tests confirm it doesn't show "agent" for setViewport, so that's not an issue. The actual finding is that applying clearDeviceMetricsOverride on mount for a fill tab is unnecessary and forces attaching a full CDP debugger session with domain enablement and diagnostics buffering — the fix should skip that initial apply when the setting is already fill, while still handling the fixed→fill transition case. I'm considering tracking an "applied override" ref per webview so clearing only happens if an override was previously set, but that touches multiple hunks and states, so it's better framed as a described fix rather than a diff. I'm also eyeing a second issue: an unfiltered preview state subscription that could cause every hosted webview to re-render on every preview event. Actually, HostedBrowserWebview renders inside ElectronBrowserHost, which already subscribes to all sessions and re-renders anyway — so this redundant subscription might not add much practical harm. Let me check ElectronBrowserHost to confirm.

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

💭 I realize the parent already re-renders all webviews on every preview event, so that subscription isn't actually a meaningful finding—I'm dropping it. My real finding is the debugger attach behavior for fill tabs, and I'm verifying that the fill case does send clearDeviceMetricsOverride at mount. I trace the fill viewport path through to confirming a control session gets established and the device metrics override cleared, and I note this effect also fires whenever hasWebContents flips to true. The severity concerns me: attaching a CDP debugger with Network.enable to every preview guest increases memory usage and instruments network behavior in a way that changes guest performance, plus there's a minor issue with DevToolsOpenError being swallowed on tabs where the user already opened DevTools manually. This seems worth flagging as a comment. Now I want to check a trickier edge case—whether hasWebContents can toggle false then true again during webview recreation, and whether the effect might apply an override to a new webContents before it's properly registered. I trace through what happens when a guest process crashes and a new webview mounts while the main process still references the old webContentsId until registration completes: if the user resizes during that gap, the effect targets the stale destroyed webContents, the error gets silently swallowed, and once registration happens the correct viewport gets applied afterward—so this case looks handled. Then I consider the drag interaction, where each frame during a pointer drag fires a setViewport IPC call, and unlike the control-session-based calls, these aren't serialized through the semaphore, so with ~60 calls per second there's a real risk of out-of-order completion when multiple overrides overlap. If the last commit loses that race, the guest could stay one frame off, but the probability is low and any fix belongs in desktop code, so I'll skip it. The mobile heuristic in desktop code is also out of scope for comments. The new previewGuestViewport.ts module looks fine since it's a small shared helper used by two call sites, following the "smallest durable contract" principle, and there's a corresponding test file for it. The undefined-input test doesn't actually assert anything meaningful, but that's minor and doesn't violate the rubric requiring focused tests for behavior changes. Now I want to trace through the zoomFactor interaction since HostedBrowserWebview owns both properties — setting up a scenario with a 390x844 viewport preset and 1.5x zoom to see how the element CSS size and guest dimensions scale relative to each other. Working through how Chromium's setDeviceMetricsOverride interacts with Electron's zoom, it seems like the device metrics width sets the widget size in DIP, and then Electron's page zoom further scales that, meaning window.innerWidth would come out to 390/1.5 = 260 rather than the expected 390 — which suggests there could be a compounding scale issue between these two features. I'm uncertain whether DevTools device mode actually divides by page zoom or ignores it entirely — but the previously working behavior (585px element, 1.5 zoom giving innerWidth 390) suggests zoom isn't applied on top of the override. If it were, the automation's viewport check comparing innerWidth to the declared width would fail whenever preview zoom isn't 1, breaking agent resizes. I'm also noting that setting deviceScaleFactor: 1 in the override forces the guest's devicePixelRatio to 1 regardless of the actual display — on a Retina Mac, that means previews render at DPR 1 instead of 2, causing blurrier images and different srcset/media-query selections. But that code lives in apps/desktop/src/preview/Manager.ts, which is outside the scope of files I'm allowed to comment on since the include patterns only cover apps/web. I'm weighing whether it's still worth flagging this in the summary even though I can't leave an inline comment on that file. Turning back to the fill/debugger-attach issue, which is in scope for HostedBrowserWebview.tsx: this one's defensible since every preview webview, even plain fill previews that never touch device emulation, now triggers a CDP debugger attach at mount plus several domain enables and diagnostics buffering, when the command being issued for fill is really a no-op clear. The fix would be to skip applying the override when the setting is fill and nothing has been applied yet, tracked via a ref, and I should also note the side effect that eagerly attaching changes other preview behavior. Also this preemptive attach means PreviewAutomationDevToolsOpenError can now fire silently if a user already has DevTools open on the guest — worth a brief mention. I want to double check there's no existing guard that makes ensureControlSession cheap on a fresh guest, and confirm whether registerWebview already attaches its own control session.

15. git_tools — const registerWebview = Effect.fn

💭 Actually, this invalidates my earlier finding — registerWebview already calls restoreControlSession for every registration, and that function attaches the debugger unconditionally before any colorScheme check. So debugger attachment on every preview guest was already happening pre-existing, not something new I need to flag. But this surfaces a real gap: restoreControlSession runs concurrently with applyPreviewGuestViewport, and while it restores color scheme after a detach (which happens whenever DevTools opens/closes on the guest), it never restores the viewport override. That means after toggling DevTools on a fixed-viewport preview, the override silently disappears and nothing re-triggers reapplication — reproducing the exact bug this PR is meant to fix. The proper fix likely belongs in Manager.ts, though, which is outside this scope. I'm also checking whether navigation or cross-origin loads could break the override — they shouldn't, since the emulation setting persists within a debugger session — and confirming that webContents recreation after a crash correctly triggers re-registration. Since the debugger attachment path seems solid, I'm now shifting focus back to the "hidden viewport" scenario this PR specifically targets, reconsidering how the wrapper's offscreen sizing when the preview panel is inactive interacts with the guest's reported dimensions once the override is in place. I realize there's a mismatch case worth examining: if the override pins the guest's viewport to a fixed size while the actual widget element is a different size, the rendered content could end up scaled or letterboxed to fit the widget rather than matching pixel-for-pixel, similar to how DevTools device emulation mode handles size mismatches. When viewport is non-fill, the fitted source viewport equals the viewport itself, so the same logic applies; when fill, the override clears, keeping things consistent. The scaling behavior also checks out: element CSS size is the unscaled viewport times zoom, then a CSS transform scales it down, which explains why they divide by scale elsewhere. Now I want to trace zoomFactor more carefully since the hosted browser webview owns both the CSS zoom and the guest's actual Electron zoom. I'm trying to work out whether Chromium's device metrics override interacts with page zoom multiplicatively or whether device emulation mode disables zoom entirely — I recall DevTools has a comment suggesting page zoom gets reset in device emulation mode, which would resolve the discrepancy I was worried about. I'm too uncertain to flag this definitively, and any fix would need to happen desktop-side. I'm also wondering if this breaks automation—the readiness check comparing window dimensions against declared viewport values could fail if zoom shrinks the effective size, causing resize automation to time out and roll back whenever the user has zoomed the preview. Given the uncertainty and that the fix location is out-of-scope, I'd rather verify by checking whether the readiness check already normalizes for zoom, since that would tell me the codebase's existing stance on this rather than me guessing at speculative Chromium behavior.

16. browse_code — apps/web/src/components/preview/previewViewportReadiness.ts

💭 That comment about fractional pixel rounding suggests that the existing element-size-driven approach already gives innerWidth matching the setting width regardless of zoom, but overriding in DIP would break readiness checks when zoom isn't 1, since innerWidth would then be setting.width divided by zoom, causing the automation resize to time out and roll back. Digging into Chromium's device emulation internals, I trace how setDeviceMetricsOverride sets the view size in DIP through blink's widget resize path, and confirm that innerWidth divides by the effective zoom factor — so a device emulation override of 390 with 1.5x page zoom would report an innerWidth of 260. I'm now checking whether DevToolsEmulator also suppresses or resets page zoom when applying device emulation, since that would change this whole calculation. If device emulation actually resets zoom to 1 during that state, that would explain another visible regression: a preview at 1.5x zoom would suddenly render at zoom 1 when a device viewport becomes active, making content appear smaller in a way that looks broken. Either way, combining a non-default zoom factor with a fixed viewport override seems suspicious in this code path — one behavior breaks the innerWidth/automation guarantee, the other silently ignores the guest's zoom. Since HostedBrowserWebview applies the viewport override unadjusted alongside the zoom factor applied to element geometry, that's a concrete, in-scope regression worth flagging, though the exact fix depends on which of these two behaviors is actually happening. But I'm wary of asserting this confidently since I don't know for certain how Chromium's emulation handles page zoom — it might reset it entirely, which would invalidate my reasoning. I think the safer move is to phrase this as a request for verification rather than a definitive claim, and to check whether there's an existing test covering the zoom case before committing to this conclusion. Confirming that zoomFactor comes from the preview toolbar's zoom control, applied through the desktop manager calling the webContents zoom method — so this is genuine Chromium page zoom, and the interaction I'm suspicious of is real. Given that, I'll write this up as an inline comment on the HostedBrowserWebview effect, framing it as a verification request with concrete reasoning rather than a flat assertion. Actually I realize both PreviewAutomationHosts and the mount path also apply the same unzoomed dimensions, and both trace back to a shared helper in previewGuestViewport.ts that maps settings to dimensions without any zoom input — so a single comment there might cover both call sites, though the actual fix would still require threading zoomFactor through each caller, which PreviewAutomationHosts can already access via the thread preview state. This feels speculative enough that I want to check the risk of a false positive by searching the codebase for existing precedent — anywhere CDP emulation and zoom are already handled together, like in screenshot or screencast logic — and specifically look for setZoomFactor alongside Emulation usage or comments referencing zoom in Manager.ts.

17. git_tools — Emulation.

💭 I'm noticing that restoreControlSession only restores the color scheme after re-attach but doesn't reapply the viewport/device metrics override — that's the same CDP session state, so per the existing documented rule about emulated media needing re-application after every attach, this override is being silently dropped, which looks like a real gap introduced by this PR. The trouble is the actual fix lives in Manager.ts, which is outside the files I'm allowed to comment on, and there's no detach signal available in the in-scope renderer code to work around that. I think it's still worth flagging on the HostedBrowserWebview effect, since that's the only re-application path in scope and it doesn't cover this CDP detach case that the color-scheme logic already handles. Now I want to verify the actual claim: does Electron's debugger fire a 'detach' event when DevTools opens on the attached webContents, and is there any listener calling restoreControlSession besides the one in registerWebviewUnlocked? I need to trace where else that restore function gets invoked.

18. git_tools — restoreControlSession

💭 This confirms the DevTools open path detaches the control session and only restores color scheme on close, so the device-metrics override never gets reapplied — the guest silently falls back to host dimensions even though the toolbar still shows 390×844, and nothing triggers the renderer to notice since the viewport key and tab ID stay unchanged. I realize the actual regression only surfaces when the preview is hidden or misfit, since visible previews still lay out at viewport size. But since the fix belongs in the desktop's Manager.ts and my scope is limited to apps/web files, I can't comment there directly — instead I should anchor the comment on HostedBrowserWebview.tsx, explaining that the renderer lacks a re-apply path after a CDP detach and that the fix needs the desktop's restoreControlSession to also restore the viewport override alongside the color scheme. Since the desktop already owns zoom, mute, and color-scheme reconciliation for a guest, adding viewport reconciliation there follows the same pattern rather than making the renderer responsible for something it can't properly track across detach/reattach cycles. I'll add one comment on the effect in HostedBrowserWebview.tsx around the new lines 208-219, targeting line 219 with start_line 210, since both are added lines in the diff. I'm also weighing whether the redundant application in both register() and the effect is worth flagging, but it's harmless and actually fixes the earlier race, so I'll leave it. I'm now looking at whether the rollback path in persistViewport (on applyPreviewGuestViewport failure, it reverts the server viewport and re-applies the previous override before throwing) interacts problematically with the React effect. I'm tracing a potential race between the automation path (which calls setViewport via the agent bridge, shows the "controlling browser" badge, and records a resize action inside a control session) and the renderer effect reacting to the same snapshot update by immediately re-applying the human path's setViewport — since the human path doesn't take the same session semaphore, its calls could interleave inside the agent's session block. The values match though, so there's no real conflict since the effect only fires when necessary. I also notice the server snapshot updates before the guest override is applied on resize, so the React effect might fire and apply the human-path override before the automation call completes; if that automation call then fails and rolls back, both paths fail the same way since they share the same DevTools-open error condition, so behavior stays consistent. That's one finding worth flagging, but I'm weighing whether it alone is enough to fail the review given the rubric's threshold for what counts as a clear, actionable concern. I also realize openDevTools detaches immediately, and closing DevTools re-attaches and restores color scheme but not viewport — so the toolbar keeps showing the emulated size while the guest reverts to real dimensions, reintroducing the hidden-viewport bug and silently dropping mobile/DPR emulation for visible previews even though the element width stays the same. Now I'm drafting the actual review comment explaining this: the effect that applies the override is the only place that reapplies it, but it can't detect a CDP session detach, so after DevTools opens and closes on the guest the tracked state goes stale and nothing triggers a re-application. I'm suggesting the fix live alongside the other guest-state reconcilers in the main process, persisting the viewport on the tab state and re-applying it during session restore the same way color scheme and zoom are handled, while leaving this effect for user-driven changes. Then I'm posting the comment at the relevant line.

19. modify_pr — One finding: the new guest viewport override is session-scoped state that the renderer alone cannot keep alive across a CDP detach. Details inline.