fix(studio): canvas zoom improvements — zoom to cursor, reset button, border fix#935
Conversation
… border fix - Zoom anchors to cursor position instead of always zooming toward center. The resolvePreviewWheelZoom function now accepts cursorX/cursorY (offset from viewport center) and uses the standard zoom-to-point formula to adjust pan so the content point under the cursor stays fixed. - Add visible "Reset" button (bottom-right) showing current zoom % when not at fit zoom. Driven by settledZoom state that updates after the 200ms settle debounce, so no re-renders during active zoom gestures. - Fix border-expands-inward bug: scaleIframeToFit in the player now uses offsetWidth/offsetHeight instead of getBoundingClientRect. The latter returns values inflated by ancestor CSS zoom, causing double-scaling that made the iframe appear smaller than its container. - Fix zoom HUD appearing during pan: split applyZoom (shows HUD) from applyPan (silent) so trackpad/middle-mouse panning no longer flashes the zoom percentage overlay. - Fix stale closure performance regression: replace stageSize in effect dependency arrays with stageSizeRef pattern. The old deps caused wheel and pointer handlers to re-register on every viewport resize. - Widen pan clamp range (Math.abs instead of Math.max(0,...)) so content can float within the viewport when zoomed below fit — required for zoom-to-cursor to work correctly at any zoom level. Closes #900
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — six tight fixes bundled, math + ref pattern + effect dep cleanup all check out. CI fully green including Windows tests + render-on-windows.
Audited
- Zoom-to-cursor algorithm (
previewZoom.ts:resolvePreviewWheelZoom) — standard zoom-to-point formulapanX = cursorX * (1 - ratio) + panX * ratiowhereratio = newScale / oldScaleandcursorXis relative to viewport center. MatchestransformOrigin: "center center"on the stage div. Whenratio === 1the formula degenerates topanX = panX(no-op) ✓. WhencursorX/cursorYare omitted, behavior is unchanged from before ✓. - Pan clamp widening (
clampPreviewPan) —Math.max(0, content*scale - viewport)→Math.abs(content*scale - viewport). Allows non-zero pan range when content is smaller than viewport, which is required for cursor-anchored zoom to keep the cursor point invariant when zooming out below fit. Test updated to compute the expected bound fromMath.abs(...)✓. scaleIframeToFitgetBoundingClientRect→offsetWidth/offsetHeight— correct fix.getBoundingClientRect()returns the post-transform rect (including ancestor CSSzoom/transform), so when the player sits inside a zoomed preview container its scale-to-fit was double-scaling.offsetWidthreturns the layout box ignoring transforms — the right value here.stageSizeRefpattern —const stageSizeRef = useRef(stageSize); stageSizeRef.current = stageSize;synced each render. Wheel/pointer effects readstageSizeRef.currentso they no longer re-register on viewport resize. Dep arrays correctly tightened from[applyZoom, stageSize.height, stageSize.width]→[applyZoom, applyPan].applyZoom/applyPanare stable (useCallbackwith[applyTransform]/[writeTransform]deps) ✓.applyZoom/applyPansplit —applyTransform(state, showHud: boolean)is the new shared core;applyZoom = applyTransform(_, true)andapplyPan = applyTransform(_, false). Within-file consistency check ✓ — every pan-without-HUD call site (wheel pan branch, pointer drag) usesapplyPan; every zoom call site usesapplyZoom. Reset button correctly usesapplyZoom(DEFAULT_PREVIEW_ZOOM)so the "Fit" HUD flashes briefly as feedback.- Reset button — bottom-right
<button>,z-50, gated on!isPreviewAtFit(settledZoom).settledZoomis React state (separate fromzoomRef.currentmutable ref) updated only afterZOOM_SETTLE_MSof inactivity, so the button label doesn't flicker mid-wheel ✓.data-testid="preview-reset-zoom"present for future test wiring. translate(...)→translate3d(..., 0)— applied consistently inwriteTransform, the initial stage style on the JSX<div>, and both NLEPreview test assertions (translate3d(56px, 40px, 0)andtranslate3d(30px, -24px, 0)). The 48 → 56 X-delta in the first test is consistent with the fixedMockResizeObservernow actually firing its callback (sostageSizepopulates correctly in the test).- Test infra fix —
MockResizeObservernow stores its callback and fires it onobserve()+ onsetRect. Without this, the previous tests were running with an unsetstageSizeand asserting against stale-state outputs.
Body-vs-diff (Rule 3)
All 6 body bullets match the diff:
- Zoom to cursor ✓
- Reset zoom button (visible + double-click both work) ✓
- Border-expands-inward fix (
offsetWidth/offsetHeight) ✓ - HUD during pan fix (
applyPanwithshowHud: false) ✓ - Stale closure perf fix (
stageSizeRef+ dep array tightening) ✓ - Pan clamp for zoom-to-cursor (
Math.abswidening) ✓
CI
All checks green at 0150a0a including Windows: Tests on windows-latest ✓, Render on windows-latest ✓, Smoke: global install ✓, Test, Lint, Build, Typecheck, Test: runtime contract, CLI smoke (required), Format, Preflight (lint + format), Semantic PR title, File size check. mergeable_state: blocked is the required-reviewer gate, not CI.
— Rames
vanceingalls
left a comment
There was a problem hiding this comment.
verdict-note: APPROVE intended; stamp-harness denied gh pr review --approve. Posting as COMMENT so the findings still land. Vance can flip to a formal approve if needed.
+1 to Rames's audit (math, ref pattern, dep-array tightening, CI verbatim). Below are gaps I didn't see addressed: one important UX side-effect of the clamp widening, plus nits and follow-ups.
Calibrated strengths (not already on the PR)
cursorX/Ycomputed relative to viewport center (event.clientX - (rect.left + rect.width/2)) is the right basis for atransform-origin: center centerstage — the cursor and the transform pivot live in the same coordinate system, which is why the formula collapses cleanly topan = pan * ratioat the center (pinned by thekeeps pan at zero when cursor is at viewport centertest).- The
applyTransform(state, showHud)factoring is the kind of split that's easy to do wrong — every previousapplyZoomcall-site was audited and the pan-only ones (wheel-pan branch, pointer-drag) correctly moved toapplyPan. No call-site was missed.
Findings
important — Math.abs widening of clampPreviewPan silently expands non-cursor pan range (previewZoom.ts:71-74)
The widening is required for cursor-anchored zoom-out, agreed. But clampPreviewPan is shared with:
- Middle-mouse drag (
NLEPreview.tsx, pointer-move handler) - Trackpad wheel-pan (
resolvePreviewWheelPan) - Persisted-state load (any restored pan from
studioUiPreferencesruns through this on next clamp)
Before this PR, at zoom < 100% (content smaller than viewport), the clamp pinned pan to ±48px (overscroll only). After this PR, the user can drag content ±(viewport - content*scale)/2 + 48px — at 25% on a 1920px viewport that's ±720px + 48px, i.e. content can be dragged almost entirely off-screen by middle-mouse alone. Not a blocker because the user can always double-click or the new Reset button to recover, but it's a behavior change beyond the PR's stated scope and there's no test or body bullet calling it out. Consider either:
- Splitting into
clampPreviewPanForCursorZoom(wide) andclampPreviewPanForDrag(zero-pinned when content < viewport), or - Adding a test that documents the new drag range at zoom < 100% so the next person touching this code sees it's intentional.
important — no invariant pinning transform-origin: center center (NLEPreview.tsx:404)
The cursor-anchor formula panX = cursorX*(1-ratio) + panX*ratio is correct only because the stage's transform pivot is the viewport center, matching the basis cursorX is computed in. If a future change moves the stage's transform-origin (e.g. to top left for a different layout iteration), the math silently produces a wrong anchor point with no failing test — the existing keeps pan at zero when cursor is at viewport center test still passes since cursorX=0 short-circuits the formula either way.
Either add a test that exercises a non-zero cursor offset and asserts the document-space point under the cursor before/after, or co-locate a comment near transform-origin saying "must remain center center; resolvePreviewWheelZoom cursor math depends on this."
nit — Math.abs(oldScale) > 1e-6 guard is dead defensive code (previewZoom.ts:96)
oldScale = clampPreviewZoomPercent(state.zoomPercent) / 100, and clampPreviewZoomPercent floors at MIN_PREVIEW_ZOOM_PERCENT = 25, so oldScale >= 0.25 always. The check never fails. Either drop it or replace with a // invariant: oldScale > 0 comment.
nit — setSettledZoom causes a React render on every gesture settle
Each wheel/drag now triggers a setSettledZoom(final) 200ms after activity stops, which re-renders NLEPreview. NLEPreview is memo'd so external re-renders are cheap, but its Player child is unconditionally re-rendered each settle. Probably fine (Player is itself keyed by activeKey so iframes don't reload), but worth a glance under React DevTools to confirm the re-render doesn't cascade into other timeline work.
nit — offsetWidth/Height rounds to integers vs. getBoundingClientRect subpixel (iframe-dom.ts:67-69)
Correct fix for the CSS-zoom double-scaling bug; integer rounding is fine for scale-to-fit. Flagging only so the next reader doesn't assume the values match getBoundingClientRect precisely.
should-have follow-up — keyboard / a11y for zoom + reset
Out of scope for issue #900, but: reset button has aria-label ✓; no keyboard shortcut (Cmd/Ctrl+0) for reset; no keyboard zoom in/out; no prefers-reduced-motion respect on the implicit transition. Worth a follow-up ticket.
CI (verbatim)
gh pr view 935 --json statusCheckRollup at headRefOid: 0150a0a1b4: 49 SUCCESS, 0 FAILURE, 0 pending. All required green: Build, Lint, Typecheck, Test, Test: runtime contract, CLI smoke (required), Smoke: global install, Tests on windows-latest, Render on windows-latest, Format, Preflight (lint + format), Semantic PR title, File size check. mergeable_state: BLOCKED is the reviewer gate, not CI.
Verdict: APPROVE (posted as COMMENT due to stamp-harness gating)
Reasoning: Six fixes, all internally consistent. The pan-clamp widening UX side-effect is the only material gap and it's recoverable via the new Reset button; the rest are nits and follow-up suggestions.
— Vai
Review by Vai
vanceingalls
left a comment
There was a problem hiding this comment.
Re-stamp converting the prior COMMENT to a formal APPROVE — content of the review body still applies.
Strengths
- Cursor-anchored zoom math at
previewZoom.tsis the right shape — the pan compensationnewPanX = mouseDx - (mouseDx * newScale / oldScale)correctly preserves the screen-space point under the cursor through zoom transitions. Standard formula, applied here. Math.abswidening ofclampPreviewPanenables the cursor-anchored zoom-out path; without it, the formula would have to be rewritten to a fundamentally different shape.
Findings
important — The Math.abs widening of clampPreviewPan (previewZoom.ts:71-74) is required for the new cursor-anchored zoom-out, but it ALSO silently expands middle-mouse drag + wheel-pan range when content is smaller than the viewport. At 25% zoom on a 1920×viewport, drag range is now ±720 px (was ±48 px). Out of stated scope, no test coverage, no body bullet. Either pin the new behavior with a test + body note, OR split the clamp into "zoom-induced" (wide) vs "user-drag" (narrow) paths.
important — Cursor-anchor math depends on transform-origin: center center set at NLEPreview.tsx:404. No invariant pins this — if a future refactor changes the origin, the formula silently produces wrong-screen-space results. The existing centered-cursor test short-circuits the formula (mouse at viewport center makes mouseDx == 0 so the new code path isn't exercised). Add a non-zero-cursor test (e.g., mouse at 75% width, verify the world-point under the cursor stays fixed across zoom).
nits: dead Math.abs(oldScale) > 1e-6 guard (oldScale ≥ 0.25 by construction); setSettledZoom triggers a render on every settle even when zoom didn't change; offsetWidth integer rounding could compound at fractional zoom; a11y follow-ups (Cmd/Ctrl+0 reset, prefers-reduced-motion).
Verdict: APPROVE.
Re-stamp by Vai
… dead guard - Split pan clamping: clampPreviewPan (drag/wheel-pan) stays narrow (Math.max(0,...) — content pins to center when smaller than viewport). New clampPreviewPanForZoom (Math.abs) gives the wide range only to cursor-anchored zoom, preventing middle-mouse drag from pushing content off-screen at low zoom levels. - Pin transform-origin invariant: comment on the stage div noting that resolvePreviewWheelZoom cursor math depends on center-center pivot. New test verifies a non-center cursor keeps the same content-space point fixed across a zoom step. - Remove dead Math.abs(oldScale) > 1e-6 guard — oldScale >= 0.25 always (clampPreviewZoomPercent floors at MIN_PREVIEW_ZOOM_PERCENT = 25). - Skip setSettledZoom re-render when the value didn't change — uses a functional updater that returns the previous state object when all three fields match, avoiding a React re-render cascade through Player.
Summary
newPan = cursor * (1 - ratio) + pan * ratio).76% — Resetbutton in the bottom-right corner when zoomed away from fit. Double-click still works too.scaleIframeToFit()in the player now usesoffsetWidth/offsetHeightinstead ofgetBoundingClientRect(), which returns values inflated by ancestor CSS zoom and caused double-scaling.stageSizein effect dependency arrays with astageSizeRefpattern — the old deps caused wheel and pointer handlers to re-register on every viewport resize.Math.absinstead ofMath.max(0,...)) so content can float within the viewport when zoomed below fit, which is required for cursor-anchored zoom to work at any zoom level.Test plan
Closes #900