fix(preview): apply viewport changes when the panel is hidden - #7303
fix(preview): apply viewport changes when the panel is hidden#7303gbarros-dev wants to merge 10 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:
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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the web-side changes for UI/behavior consistency. Two concerns about the new guest viewport override and how it interacts with the existing viewport-readiness contract; both are in apps/web/src/components/preview.
Posted via Macroscope — UI Consistency
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. |
6a56ea4 to
441e9f9
Compare
There was a problem hiding this comment.
Reviewed the web-side viewport override wiring (PreviewView.tsx, PreviewAutomationHosts.tsx, previewGuestViewport.ts). Two issues in the new PreviewView effect affect visible preview chrome; details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new guest viewport override is synced only from the committed viewport, so it desyncs from the <webview> box during a resize-handle drag.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding on the new guest-viewport application path. See the inline comment on apps/web/src/browser/HostedBrowserWebview.tsx.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One ownership finding on the new guest-viewport override plumbing. The rest of the web-side changes (shared previewGuestViewport helper, automation rollback path, webview-owner effect) look consistent with the existing preview surface contracts.
Posted via Macroscope — UI Consistency
6b94fa7 to
a202af0
Compare
preview_resize only updated the CSS/React chrome. If the browser panel was hidden, the guest never changed size and wait timed out. Resize now persists the setting, then applies a CDP device-metrics override so the guest viewport changes even when the tab is not visible.
A size-only ready check could resolve resize before React committed the webview geometry. A failed CDP apply also left the persisted snapshot on the new size. Resize now waits for the existing ready gates, rolls the snapshot back when the override fails, and re-applies the committed setting when the guest attaches.
The restore effect depended on the viewport object, so every preview event re-sent CDP and flashed the agent-controlling badge. Toolbar resizes now use a setViewport path that does not take agent control, and the effect keys only on the viewport size string.
Width-only mobile detection treated 844x390 phones as desktop. The guest also stayed pinned to the last committed size while the frame was dragged. mobile now uses the shortest side. HostedBrowserWebview applies a frame-throttled CDP override from the effective (including drag) viewport.
The HostedBrowserWebview override ran one frame after mount, before registerWebview finished, then swallowed the failure. Crash recovery and mini-player never retried. The override now runs after a successful register, and drag updates wait for hasWebContents.
PreviewView reapplied the committed size after the webview owner already set the live override. That raced drags and keyboard resizes. The active-tab effect is gone; HostedBrowserWebview is the single owner.
a202af0 to
af0ec53
Compare
There was a problem hiding this comment.
One finding on the guest viewport override lifecycle. See the inline comment on apps/web/src/browser/HostedBrowserWebview.tsx.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new guest viewport override is session-scoped state that the renderer alone cannot keep alive across a CDP detach. Details inline.
Posted via Macroscope — UI Consistency
Device-metrics overrides live on the CDP session, so DevTools and webview swaps dropped a fixed viewport while the toolbar still showed it. Remember the last override and re-apply it from restoreControlSession, like color scheme. Guest rollback now rechecks runtime/epoch and only persists previous if the guest actually rolled back, so a replaced tab or a failed CDP clear cannot desync store and guest.
There was a problem hiding this comment.
One finding on the new guest viewport override: the CDP size is sent without accounting for the tab's page zoom, which the rest of the preview layout consistently scales by.
Posted via Macroscope — UI Consistency
setDeviceMetricsOverride is a widget DIP size, so page zoom still divides it. Desktop presets now send setting * zoom so innerWidth matches the toolbar. Phone sizes stay unscaled because mobile emulation pins zoom to 1.
Apply-failure never put the new override on the guest, so the store still rolls back even if the guest CDP clear throws. Timeout still rolls the guest first and only persists previous if that apply succeeds; a failed store resize puts the guest back on the requested size.
There was a problem hiding this comment.
One finding: the mobile/desktop classification split between this helper and PreviewManager.deviceMetricsOverride diverges once the tab is zoomed out, flipping desktop viewports into mobile emulation.
Posted via Macroscope — UI Consistency
| // The override is a widget DIP size; page zoom still divides it. Mobile | ||
| // emulation pins page zoom to 1, so those sizes stay in CSS pixels. | ||
| const scale = previewGuestViewportIsMobile(setting.width, setting.height) ? 1 : zoom; |
There was a problem hiding this comment.
The mobile decision is made here on the unscaled CSS size, but PreviewManager.deviceMetricsOverride re-derives it from the scaled size it receives (mobile: Math.min(width, height) < 768, apps/desktop/src/preview/Manager.ts:2462). The two disagree whenever zoomFactor < 1, and PREVIEW_ZOOM_LEVELS goes down to 0.25.
Example: a 1024x768 desktop viewport at zoom 0.5 is treated as non-mobile here, scaled to 512x384, and the desktop then sets mobile: true. Per the comment just above, mobile emulation pins page zoom to 1, so the guest ends up laying out at ~512 CSS px with mobile viewport semantics while the toolbar still says 1024x768 — the same toolbar/guest mismatch this PR is fixing, reappearing on zoom-out.
Smallest fix is to keep the classification on the CSS-pixel setting on both sides: either send the intended mobile flag (or the unscaled size) through DesktopPreviewAutomationSetViewportInputSchema and have deviceMetricsOverride use it, or move the zoom scaling into PreviewManager, which already knows tab.zoomFactor. Worth a case in previewGuestViewport.test.ts covering a desktop size at a sub-1 zoom.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the guest/host mobile emulation decision is derived from two different values, so zoomed-out desktop viewports can silently flip the preview into mobile emulation. Details inline.
Posted via Macroscope — UI Consistency
| // The override is a widget DIP size; page zoom still divides it. Mobile | ||
| // emulation pins page zoom to 1, so those sizes stay in CSS pixels. | ||
| const scale = previewGuestViewportIsMobile(setting.width, setting.height) ? 1 : zoom; |
There was a problem hiding this comment.
scale is decided from the unscaled setting here, but the desktop decides mobile from the scaled payload it receives (Manager.ts deviceMetricsOverride: mobile: Math.min(width, height) < 768). At zoom < 1 the two disagree, breaking the Keep in sync with PreviewManager.deviceMetricsOverride contract above: iPad-portrait 768x1024 at 0.9 zoom, or 1440x900 at 0.8, is non-mobile here, so it is scaled to 691x922 / 1152x720, and the main process then turns mobile emulation on for it.
Consequences: the guest renders the site's mobile layout with touch emulation while the toolbar still shows the desktop size, and since mobile emulation pins page zoom to 1 the measured innerWidth stays the scaled value, so isPreviewViewportReady never matches the requested setting and an agent resize fails with a viewport timeout.
Suggest deciding mobile once from the CSS-pixel setting and carrying it in the override payload (add mobile to PreviewGuestViewportOverride / DesktopPreviewAutomationSetViewportInputSchema and have deviceMetricsOverride use the provided flag) rather than re-deriving it from post-zoom dimensions.
Posted via Macroscope — UI Consistency
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 79fa0bd. Configure here.
| return { | ||
| width: Math.max(1, Math.round(setting.width * scale)), | ||
| height: Math.max(1, Math.round(setting.height * scale)), | ||
| }; |
There was a problem hiding this comment.
Zoom scales break mobile flag
Medium Severity
Desktop overrides are scaled by zoom before IPC, but PreviewManager.deviceMetricsOverride still decides mobile from those already-scaled sizes. Zooming out a logical desktop or tablet viewport (for example 1024×768 or ipad-mini at 0.9) can drop the shortest side below 768, so CDP gets mobile: true while the renderer still treated the setting as desktop and applied zoom scaling. That can desync innerWidth from the toolbar/automation readiness target and wrongly enable mobile emulation.
Reviewed by Cursor Bugbot for commit 79fa0bd. Configure here.
1 participant
preview_resizeonly updated the CSS/React chrome. If the browser panel was hidden, the guest never changed size and wait timed out.Resize now persists the setting, then applies a CDP device-metrics override so the guest viewport changes even when the tab is not visible.
Fixes #3712.
Split out of closed #7127. Land after #7236 if both are touching preview hosts in the same week; this branch is based on current
mainand does not include #7236.Tests:
vp test run packages/contracts/src/preview.test.ts apps/web/src/components/preview/previewGuestViewport.test.tsImplemented with Grok 4.6 through Grok CLI.
Note
Medium Risk
Touches desktop CDP/debugger control and automation resize rollback. Incorrect metrics or control-session handling can mis-size pages or flash agent control.
Overview
Hidden-panel resize now changes the guest page, not just the CSS chrome.
preview_resizepreviously timed out when the browser panel was offscreen becauseinnerWidthnever updated.The desktop preview manager stores a per-tab viewport override and applies
Emulation.setDeviceMetricsOverride(or clear). ToolbarsetViewportdoes this without taking agent control; automationsetViewportrecords aresizeaction. Overrides are re-applied after webview swap/DevTools detach and dropped on tab close.Renderer mapping:
fillclears the override; desktop sizes scale with page zoom; mobile (shortest side < 768) stays unscaled. Hosted webviews apply the override on attach and on viewport/zoom changes. Automation resize persists the server setting, then the guest override, and rolls both back if apply or wait-for-rendered-size fails.Reviewed by Cursor Bugbot for commit 79fa0bd. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Apply preview viewport overrides via CDP when panel is hidden
setViewportandautomationSetViewportIPC methods so the renderer can apply or clear per-tab guest device-metrics overrides viaEmulation.setDeviceMetricsOverride.PreviewManagerpersists overrides per tab, applies them without taking agent control for human invokes, and re-applies them after webview swaps or debugger re-attachments.previewBridge.setViewporton webview attachment and whenever the viewport setting or zoom changes, using a shared utility that maps fill → clear and scales non-mobile sizes by zoom.deviceMetricsOverridetreats shortest side < 768 as mobile (PREVIEW_GUEST_MOBILE_MAX_SHORTEST_SIDE); sizes near that boundary may switch between mobile and non-mobile CDP metrics, and zoom scaling only applies to non-mobile sizes.Macroscope summarized 79fa0bd.