fix: handle zoom in preview #2666
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
This is great! Can we also try this: make it so that you can scroll but if you over-scroll on the iframe, you just scroll the canvas instead? I just want to see how that feels since a lot of people got confused when they can't scroll in preview mode. Or if you're scrolling inside the canvas, let's just have an indication since we're now detecting it. |
|
One more bug and you can see it from the preview video, the mouse doesn't line up correctly with the mouse outside. If this is not fixable, I think it's ok to do nothing on zoom which is still an improvement. Be sure to test this with projects that have some zoom functionality inside of it like a Figma clone. |
WalkthroughAdds cross-frame wheel forwarding: a new Penpal parent method onFrameWheel, parent-side handler translating iframe wheel events into WheelEvents on the canvas container, preload wheel listener to capture Ctrl/Meta wheel in iframes and forward when appropriate, and removal of the prior global wheel interceptor in main.tsx. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Iframe as Iframe (preload)
participant Penpal as Penpal Link
participant Parent as Parent Frame
participant Canvas as Canvas Container
User->>Iframe: wheel event (ctrl/meta)
Iframe->>Iframe: run heuristics (scrollable/transform/touch-action/tag)
alt Not handled locally
Iframe->>Penpal: call onFrameWheel({deltaY, clientX, clientY, ctrlKey, metaKey})
Penpal->>Parent: invoke registered handler
Parent->>Parent: get iframe bounding rect, compute parent coords
Parent->>Canvas: dispatch WheelEvent with adjusted coords and modifiers
else Handled locally
Iframe-->>User: allow default/local zoom/scroll
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/penpal/src/parent.ts (1)
11-17: API addition looks good; consider extending payload for fidelity + plan for back-compatThe new method is fine. Two follow-ups:
- Add deltaX/deltaZ/deltaMode for higher-fidelity wheel handling (trackpads, non-pixel deltas).
- Ensure consumers are upgraded together (this is a breaking addition to PenpalParentMethods for any other implementers).
Proposed type extension:
- onFrameWheel: (data: { - deltaY: number; - clientX: number; - clientY: number; - ctrlKey: boolean; - metaKey: boolean; - }) => void; + onFrameWheel: (data: { + // Wheel deltas + deltaY: number; + deltaX?: number; + deltaZ?: number; + deltaMode?: 0 | 1 | 2; // DOM_DELTA_PIXEL | DOM_DELTA_LINE | DOM_DELTA_PAGE + // Pointer position in iframe coords + clientX: number; + clientY: number; + // Modifiers + ctrlKey: boolean; + metaKey: boolean; + shiftKey?: boolean; + altKey?: boolean; + }) => void;If there are other parent implementations, please confirm they are updated alongside this change or bump the package version appropriately.
apps/web/preload/script/api/events/index.ts (1)
7-8: Ensure idempotency to avoid duplicate listenersIf listenForDomChanges() can be invoked more than once, listenForWheelZoom() should guard against adding multiple global listeners.
Apply an init guard inside listenForWheelZoom():
export function listenForWheelZoom() { + // Prevent duplicate registrations + if ((window as any).__onlookWheelZoomInit__) return; + (window as any).__onlookWheelZoomInit__ = true; function handleWheel(event: WheelEvent) { ... } window.addEventListener('wheel', handleWheel, { passive: false, capture: true }); }apps/web/preload/script/api/events/wheel.ts (3)
63-64: Make listener registration idempotentAvoid multiple registrations if listenForWheelZoom() is called more than once.
-export function listenForWheelZoom() { +let __onlookWheelZoomInit = false; +export function listenForWheelZoom() { + if (__onlookWheelZoomInit) return; + __onlookWheelZoomInit = true; function handleWheel(event: WheelEvent) { ... } window.addEventListener('wheel', handleWheel, { passive: false, capture: true }); }
3-61: Optional: implement “scroll chaining” on overscroll to improve preview UXPer feedback, allow normal scrolling within the iframe, but when the user overscrolls (at scroll boundaries), route the wheel to the canvas. This can reuse the same onFrameWheel path without requiring a new API.
Apply this diff to add overscroll bridging while preserving existing zoom behavior:
export function listenForWheelZoom() { function handleWheel(event: WheelEvent) { - if (!(event.ctrlKey || event.metaKey)) { - return; - } - - const path = event.composedPath() as HTMLElement[]; + const isZoom = event.ctrlKey || event.metaKey; + const path = event.composedPath(); let zoomHandledInside = false; + let shouldForwardToParent = false; for (const el of path) { if (el instanceof Element) { try { const style = window.getComputedStyle(el); // Heuristic #1: It's explicitly scrollable const scrollable = /(auto|scroll)/.test( style.overflow + style.overflowX + style.overflowY, ); // Heuristic #2: It's explicitly interactive / transforms const hasTransform = style.transform !== 'none'; // Heuristic #3: Prevents pinch gestures in browser defaults const disablesDefaultTouch = style.touchAction === 'none'; // Heuristic #4: Specific tag types often used for zoomable content const zoomableTags = ['CANVAS', 'SVG', 'IMG']; const isZoomableTag = zoomableTags.includes(el.tagName); - if (scrollable || hasTransform || disablesDefaultTouch || isZoomableTag) { - zoomHandledInside = true; - break; - } + if (isZoom) { + if (scrollable || hasTransform || disablesDefaultTouch || isZoomableTag) { + zoomHandledInside = true; + break; + } + } else if (scrollable) { + // Overscroll detection: if user tries to scroll beyond the element’s bounds, + // we’ll forward to the parent canvas. + const canScrollY = el.scrollHeight > el.clientHeight; + const canScrollX = el.scrollWidth > el.clientWidth; + const atTop = el.scrollTop <= 0; + const atBottom = el.scrollTop + el.clientHeight >= el.scrollHeight - 1; + const atLeft = el.scrollLeft <= 0; + const atRight = el.scrollLeft + el.clientWidth >= el.scrollWidth - 1; + + const yOverscrollUp = event.deltaY < 0 && canScrollY && atTop; + const yOverscrollDown = event.deltaY > 0 && canScrollY && atBottom; + const xOverscrollLeft = event.deltaX < 0 && canScrollX && atLeft; + const xOverscrollRight = event.deltaX > 0 && canScrollX && atRight; + + if (yOverscrollUp || yOverscrollDown || xOverscrollLeft || xOverscrollRight) { + shouldForwardToParent = true; + break; + } else { + // A scrollable ancestor can still consume this wheel; let it handle. + // Note: we do not break here so an earlier element in the path could be considered. + } + } } catch (err) { console.warn('Could not get computed style for node:', el, err); } } } - if (!zoomHandledInside) { + // For zoom gestures, forward when nothing inside handles zoom. + // For normal scroll, forward only on overscroll-at-boundary. + if ((isZoom && !zoomHandledInside) || (!isZoom && shouldForwardToParent)) { event.preventDefault(); event.stopPropagation(); if (penpalParent) { - penpalParent - .onFrameWheel({ - deltaY: event.deltaY, - clientX: event.clientX, - clientY: event.clientY, - ctrlKey: event.ctrlKey, - metaKey: event.metaKey, - }) - .catch(() => { - // ignore - }); + const maybeCall = (penpalParent as any).onFrameWheel; + if (typeof maybeCall === 'function') { + Promise.resolve( + maybeCall({ + deltaY: event.deltaY, + clientX: event.clientX, + clientY: event.clientY, + ctrlKey: event.ctrlKey, + metaKey: event.metaKey, + // If you extend the types, forward these too: + // deltaX: event.deltaX, + // deltaZ: event.deltaZ, + // deltaMode: event.deltaMode, + // shiftKey: event.shiftKey, + // altKey: event.altKey, + }), + ).catch(() => { + // ignore + }); + } } } } window.addEventListener('wheel', handleWheel, { passive: false, capture: true }); }This keeps default scrolling intact but forwards wheel to the canvas when an inner scroller hits its boundary.
47-59: Forward complete wheel signal (deltaX/deltaZ/deltaMode) for trackpad fidelityIf the parent API is extended, include deltaX/deltaZ/deltaMode and modifier keys to preserve gesture fidelity (especially on macOS trackpads).
- .onFrameWheel({ - deltaY: event.deltaY, - clientX: event.clientX, - clientY: event.clientY, - ctrlKey: event.ctrlKey, - metaKey: event.metaKey, - }) + .onFrameWheel({ + deltaY: event.deltaY, + deltaX: event.deltaX, + deltaZ: event.deltaZ, + deltaMode: event.deltaMode, + clientX: event.clientX, + clientY: event.clientY, + ctrlKey: event.ctrlKey, + metaKey: event.metaKey, + shiftKey: event.shiftKey, + altKey: event.altKey, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/main.tsx(0 hunks)apps/web/preload/script/api/events/index.ts(1 hunks)apps/web/preload/script/api/events/wheel.ts(1 hunks)packages/penpal/src/parent.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/client/src/app/project/[id]/_components/main.tsx
🔇 Additional comments (2)
apps/web/preload/script/api/events/index.ts (1)
4-8: Wiring the wheel listener into the lifecycle: LGTMHooking listenForWheelZoom() after DOM/resize listeners is appropriate. No functional issues here.
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
126-158: Follow-up: verify UX on pages with their own zoom handlersPlease test with apps that implement their own zoom (e.g., Figma-like editors) to ensure our heuristics avoid double-handling and the zoom center feels correct at different scales.
Suggested manual checks:
- Different iframe zoom levels (0.5x, 1x, 2x) and canvas zoom levels.
- Trackpad vs mouse wheel on macOS/Windows.
- Pages using Canvas/SVG zoom to confirm we don’t forward when the page is handling it.
- Verify the zoom focal point tracks the pointer accurately at non-1x scales.
| onFrameWheel: (data: { | ||
| deltaY: number; | ||
| clientX: number; | ||
| clientY: number; | ||
| ctrlKey: boolean; | ||
| metaKey: boolean; | ||
| }) => { | ||
| try { | ||
| const iframe = iframeRef.current; | ||
| if (!iframe) return; | ||
| const rect = iframe.getBoundingClientRect(); | ||
| const parentClientX = rect.left + data.clientX; | ||
| const parentClientY = rect.top + data.clientY; | ||
|
|
||
| const container = document.getElementById( | ||
| EditorAttributes.CANVAS_CONTAINER_ID, | ||
| ); | ||
| if (!container) return; | ||
|
|
||
| const wheelEvent = new WheelEvent('wheel', { | ||
| deltaY: data.deltaY, | ||
| clientX: parentClientX, | ||
| clientY: parentClientY, | ||
| ctrlKey: data.ctrlKey, | ||
| metaKey: data.metaKey, | ||
| bubbles: true, | ||
| cancelable: true, | ||
| }); | ||
| container.dispatchEvent(wheelEvent); | ||
| } catch (error) { | ||
| console.error('Failed to forward wheel event to canvas', error); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fix coordinate mapping: account for iframe CSS transform scale (pointer mismatch bug)
clientX/clientY from inside the iframe are unscaled CSS pixels. Since the iframe is scaled via transform (setZoomLevel), you must multiply by the current scale. This likely explains the pointer misalignment called out in the preview.
Apply this diff:
- const parentClientX = rect.left + data.clientX;
- const parentClientY = rect.top + data.clientY;
+ const scale = zoomLevel.current || 1;
+ const parentClientX = rect.left + data.clientX * scale;
+ const parentClientY = rect.top + data.clientY * scale;Optional: if you extend the payload (per parent.ts suggestion), also forward deltaX/deltaZ/deltaMode and modifiers:
- const wheelEvent = new WheelEvent('wheel', {
+ const wheelEvent = new WheelEvent('wheel', {
deltaY: data.deltaY,
+ deltaX: data.deltaX ?? 0,
+ deltaZ: data.deltaZ ?? 0,
+ deltaMode: data.deltaMode ?? 0,
clientX: parentClientX,
clientY: parentClientY,
ctrlKey: data.ctrlKey,
metaKey: data.metaKey,
+ shiftKey: data.shiftKey ?? false,
+ altKey: data.altKey ?? false,
bubbles: true,
cancelable: true,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx
around lines 126 to 158, the forwarded clientX/clientY are using unscaled iframe
coordinates so pointer events mismatch when the iframe is CSS-transformed
(scaled) by setZoomLevel; multiply the iframe-local clientX/clientY by the
current scale factor before adding rect.left/top (i.e. compute parentClientX =
rect.left + data.clientX * scale and parentClientY = rect.top + data.clientY *
scale), obtain the scale from the iframe’s computed transform or the component’s
zoom state, and include any additional wheel fields (deltaX, deltaZ, deltaMode)
and modifier keys when constructing the WheelEvent so the parent receives the
correct, fully-scaled and complete event payload.
| return; | ||
| } | ||
|
|
||
| const path = event.composedPath() as HTMLElement[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the composedPath typing
event.composedPath() returns EventTarget[], not HTMLElement[]. Casting to HTMLElement[] is unsafe. Use the correct type and keep the instanceof guard.
- const path = event.composedPath() as HTMLElement[];
+ const path = event.composedPath();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const path = event.composedPath() as HTMLElement[]; | |
| const path = event.composedPath(); |
🤖 Prompt for AI Agents
In apps/web/preload/script/api/events/wheel.ts around line 9, the code casts
event.composedPath() to HTMLElement[] which is unsafe; change the type to
EventTarget[] (e.g. const path = event.composedPath() as EventTarget[]) and
retain the existing instanceof HTMLElement guards before accessing
HTMLElement-specific properties so you only treat elements as HTMLElements after
a runtime type check.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/main.tsx (1)
68-68: Addingoverflow-hiddento the root container is sensible; verify browser zoom behavior outside the canvas and consider a minimal guardThis helps prevent scroll bleed/scrollbars and keeps the app visually contained. One thing to double‑check after removing the global wheel handler: Ctrl/Cmd + wheel (or pinch on macOS trackpads that sets
ctrlKey) anywhere outside the canvas/iframe will now zoom the browser by default. If that’s not desired, consider reintroducing a minimal guard that only prevents browser zoom outside the canvas while staying out of the new iframe → canvas forwarding path.
- Please verify:
- Ctrl/Cmd + wheel or pinch outside the canvas no longer causes unintended browser zoom (unless that is now intended).
- The new iframe wheel forwarding works on macOS trackpad pinch and Windows/Linux Ctrl+wheel.
- No regressions for apps that implement their own in-iframe zoom.
- Overlays/popovers remain visible and aren’t clipped by
overflow-hidden(should be fine if they’re portaled).Optional local guard (keeps the previous “don’t browser-zoom outside the canvas” UX):
// Also add these imports at the top of the file: import { useEffect } from 'react'; import { EditorAttributes } from '@onlook/constants'; // Add inside the Main component: useEffect(() => { const onWheel = (e: WheelEvent) => { // Prevent browser zoom when zoom gesture happens outside the canvas if (e.ctrlKey || e.metaKey) { const canvasEl = document.getElementById(EditorAttributes.CANVAS_CONTAINER_ID); if (!canvasEl || !canvasEl.contains(e.target as Node)) { e.preventDefault(); e.stopPropagation(); } } }; // Capture early and mark passive:false so preventDefault works window.addEventListener('wheel', onWheel, { capture: true, passive: false }); return () => window.removeEventListener('wheel', onWheel, true); }, []);This keeps page zoom disabled across the app UI while letting the new cross-frame forwarding handle canvas zoom. If the intentional UX is to allow browser zoom outside the canvas, ignore this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/main.tsx(2 hunks)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/main.tsx (1)
11-11: React import cleanup after dropping the global wheel listener — LGTMImporting only
useRefis correct now that the global wheeluseEffectwas removed.
Description
Screen.Recording.2025-08-13.at.23.14.04.mp4
Screen.Recording.2025-08-13.at.23.18.31.mp4
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Fix zoom handling in preview mode by adding a wheel event handler to forward events to the canvas container and updating event handling logic.
onFrameWheelhandler inweb-frame.tsxto forward wheel events to the canvas container.listenForWheelZoominwheel.tsto intercept wheel events withctrlKeyormetaKeyand prevent default behavior if not handled inside.PenpalParentMethodsinparent.tsto includeonFrameWheel.listenForWheelZoomtoindex.tsto start listening for wheel events.main.tsxto prevent unintended interception.This description was created by
for 2132243. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes