fix(studio): add preview zoom controls#761
Conversation
- Scale iframe content from inside (contentDocument.documentElement) instead of scaling the parent div, avoiding compositor re-rasterization on every zoom frame — critical for smooth zoom on high-refresh displays (240Hz) - Document-level capture-phase wheel handler bypasses the DomEditOverlay - Center-based zoom (no pan drift from pointer-anchored formulas) - Transient HUD shows zoom % briefly, no persistent UI controls - Double-click preview area to reset zoom to fit - Drag-to-pan when zoomed past 100% - Momentum scroll suppression after pinch gesture (400ms cooldown) - Delta clamping (MAX_DELTA=10) prevents overshooting on fast gestures - toDomPrecision rounds transform values to 4 decimals (matches tldraw) - Zoom state persisted to localStorage with 200ms debounce - Exposes --preview-zoom CSS custom property for overlay coordinate mapping - Fix infinite render loop in NLELayout (onIframeRef → refreshPreviewDocumentVersion)
CSS transform: scale() on a div containing an iframe causes compositor cross-layer sync issues that produce visible frame tearing on high-refresh displays (240Hz ProMotion). CSS zoom property changes the actual rendered size without compositor layer synchronization, eliminating the jumping. - Replace transform: scale(Z) with zoom: Z on the stage div - Keep transform: translate() for panning (compositor-friendly, no iframe) - Overlays work correctly since getBoundingClientRect() includes zoom - Remove will-change, transition hacks, pointer-events toggles
46081b8 to
ce1c50b
Compare
The FedericoCarboni/setup-ffmpeg action downloads from an external URL that has been persistently unreachable, causing CI failures. Switch to apt-get install which uses Ubuntu's package repos (same as ci.yml and player-perf.yml).
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE at e2ee804a — but with a substantive Rule-3 finding on the PR body that should land in a body edit (not a code change) before merge.
Rule 3 (verify PR-body claims against the diff) — body overpromises
The PR description claims several behaviors that aren't in this diff:
- "Persists sidebar collapsed state, timeline visibility, and playback speed across reloads" —
studioUiPreferences.tsdefinesleftCollapsed,timelineVisible,playbackRatekeys, but onlypreviewZoomis wired to any caller.App.tsx,usePanelLayout.ts,playerStore.ts(the integration points for the other three) are NOT in the changed-files list. - "Scopes timeline Shift-drag range edits to the track where the drag starts, while keeping the old all-track fallback for selections without a track" —
Timeline.tsx,EditModal.tsx,timelineEditing.tsare NOT in the changed-files list. No diff against the timeline range-edit filter. - "Adds focused unit coverage for ... UI preference storage, playback-rate persistence, and track-scoped timeline range filtering" —
playbackRatestorage is tested instudioUiPreferences.test.ts:23-26, buttimelineEditing.test.tsdoesn't appear in the diff so the track-scoped range-filter coverage isn't actually here either.
The diff actually contains 7 files: previewZoom.{ts,test.ts} (new), studioUiPreferences.{ts,test.ts} (new), NLEPreview.tsx (+242/-30 for the zoom integration), NLELayout.tsx (+4/-2 for the onIframeRef stable-ref refactor), and preview-regression.yml (+1/-1 for the ffmpeg install change).
Two ways to land cleanly:
- Trim the body to describe only what's in this PR (preview zoom controls + zoom persistence + the unrelated ffmpeg workflow tweak). File a follow-up PR for the sidebar/timeline/playback-rate persistence + Timeline range-edit scoping work.
- Add the missing commits before merge, so the body and diff agree.
Either is fine. The risk of merging as-is is that future readers (and bisects) will see the body claim "playback rate persists across reloads" and not understand why it doesn't — the code for it isn't here.
What IS in the diff is well-shaped
Praise where it lands:
previewZoom.tsmath is clean and pure. Exponential wheel step (Math.exp(deltaY * sensitivity)), clamp at 25–400%, pan bounds at(scale-1)/2 * viewport, NaN-safe fallbacks. 118-line test file covers precision rounding, clamp boundaries, wheel-direction sign, pan clamping at fit, zoom-then-pan-clamp composition. This is exactly the right shape for refactor-safe geometry — the pure helpers are reusable + testable, and the side-effecting code inNLEPreviewjust glues them to DOM.studioUiPreferences.tsis the right kind of defensive. Storage access is try/catch'd (handles localStorage unavailability), each field type-checked separately on read (leftCollapsed: typeof === "boolean",playbackRate: Number.isFinite(...)), malformed values silently dropped, patch-merge on write. The test fixture atstudioUiPreferences.test.ts:14-29(createStorage()returning aStorageimpl backed by Map) is a clean fake for unit-testable persistence — would be a good pattern to reuse for future persisted preferences.NLEPreview.tsxzoom state stored in refs, not React state. Right call — wheel-zoom updates fire at 60+/s during a pinch gesture; through-state would re-render the entire preview on every event. The settle-timer pattern (ZOOM_SETTLE_MS = 200) defers the localStorage write + HUD update to the natural end of a gesture rather than thrashing on every wheel event.- Cleanup discipline on document-level wheel/dblclick listeners —
addEventListener('wheel', ..., { capture: true })is paired withremoveEventListener('wheel', ..., { capture: true })(same capture flag — easy to miss). ✓ Per the three-grep refactor memory. - The "400ms lookback" suppression for trackpad inertia (
if (Date.now() - lastZoomTime < 400) event.preventDefault()) is the right shape — pinch-zoom on trackpad typically continues to fire wheel events as the gesture decays, and suppressing them prevents the studio from being scrolled by post-pinch inertia. Subtle bug to anticipate, well handled. NLELayout.tsx'sonIframeRefStableextraction — clean reactivity fix. Without it, the useEffect re-runs on every parent render becauseonIframeRefis a new function reference each time. The ref-stable pattern (const onIframeRefStable = useRef(onIframeRef); onIframeRefStable.current = onIframeRef;) keeps the deps array minimal without losing the "always call the latest callback" guarantee.
Non-blocking observations
-
Unrelated workflow change —
.github/workflows/preview-regression.ymlswapsFedericoCarboni/setup-ffmpeg@<sha>forapt-get install ffmpeg. This is a positive security move (one less third-party action, aligned with the hf#740 hardening direction) but it's drift from the stated scope of "preview zoom controls." Worth a sentence in the body acknowledging it. Bonus: if other workflows still pinsetup-ffmpeg, a follow-up sweep would be the consistent move (Rule 2 — every site of the contract change). I haven't grep'd/.github/workflows/; worth a quick check. -
Timer-unmount safety:
hudTimerRef,settleTimerRef, andretiringTimerRefaren't cleared on component unmount. The HUD timer is safe because its callback null-checkshudRef.current, but the settle timer writes to localStorage and the retiring timer callssetRetiringKey(null)— both will fire post-unmount and either waste a localStorage write or callsetStateon an unmounted component (React 18 is forgiving but logs a warning). Adding auseEffect(() => () => clearAllTimers(), [])would tighten this. -
style.zoomis non-standard CSS. Chromium honors it; Firefox does not. Studio runs in Electron-Chromium so this is fine for the target, but if anyone ever opens Studio in Firefox the zoom won't apply (the iframe will just render at fit scale). Worth a one-line comment inwriteTransformnoting the Chromium-only assumption, in case someone tries to port Studio to non-Chrome. -
Document-level capture-phase wheel/dblclick listeners: scoped via
getBoundingClientRect()check, so they only act when the cursor is inside the preview viewport. But the listener still fires for every wheel event globally — minor performance cost. If the preview pane is the only place that needs ctrl+wheel zoom, an on-element listener withe.stopPropagation()would be cheaper. Not a real concern unless someone profiles wheel-event handlers under load; happy to leave as-is. -
document.addEventListener('wheel', ..., { passive: false })— required to callpreventDefault()on wheel events. Right call. Just noting that this opts out of the browser's passive-listener optimization, which is correct for the zoom-gesture path but means the listener can't be ignored by the scheduler. Acceptable trade.
CI
All required checks green at review time: Format ✓, Lint ✓, Test ✓, Typecheck ✓, Build ✓, CLI smoke ✓, Tests on windows-latest ✓, Render on windows-latest ✓, CodeQL ✓, preview-regression ✓, Preview parity ✓, player-perf ✓. The regression-shards (style-1 through style-18) are still in_progress, all from a parallel matrix — not the PR's own self-test. mergeable_state: "blocked" is the in_progress matrix, not a substantive failure.
Verdict
APPROVE on the zoom + preferences work as-implemented. The body discrepancy is the only real ask before merge: trim the body to match the diff, or add the missing integration commits. Either path is fine.
— Rames Jusso (pr-review)
settleTimerRef, hudTimerRef, and retiringTimerRef could fire after component unmount. Add cleanup effect to prevent stale callbacks.
…loads Wire up studioUiPreferences for the three remaining UI states requested in #752: left sidebar collapsed, timeline visibility, and playback rate. All three now survive page reloads using the same localStorage key as preview zoom.
Summary
Closes #752
zoomproperty instead oftransform: scale()— eliminates iframe compositor jitter on 240Hz displayspreviewZoom.ts) with delta clamping,toDomPrecision, exponential scalingstudioUiPreferences.ts:onIframeRefcallback was unstable)preview-regressionCI: replace unreliablesetup-ffmpegaction withapt-getTest plan