Skip to content

fix(studio): resize an animated element to the size it was dropped at - #3076

Merged
miguel-heygen merged 7 commits into
mainfrom
fix-studio-resize-box
Aug 7, 2026
Merged

fix(studio): resize an animated element to the size it was dropped at#3076
miguel-heygen merged 7 commits into
mainfrom
fix-studio-resize-box

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

What

Five bugs in the same gesture. Resizing an element whose visual size is driven by a scale animation put it at the wrong size, and then a second drag appeared to do nothing at all. Once those two were fixed, the element landed at the right size but not in the right place: it held the drop point for one frame and then slid away.

1. The scale was computed against a hardcoded 200px. A 630px-wide chip dropped at 1260px wide committed a scale of 6.3 instead of 2, landing at over three times where it was dropped. The next drag compounded it, because the wrong scale then counted as the element's live scale.

2. A uniform drag wrote a scale GSAP ignored. It committed the scale shorthand into keyframes that already stated scaleX and scaleY. GSAP animates each property name independently, so the keyframe ran as { scaleX: 1, scaleY: 1, scale: 0.61 } and the longhands won. The resize computed correctly, wrote to the file, and the element snapped straight back to its old size on release.

3. The first resize of an element skipped its drop-point correction. The correction reads where the committed scale rendered the box and shifts the position by the difference, but on a first resize the timeline had not re-seeked, so it measured the element at its natural size, computed a residual of zero, and did nothing. The scale then landed, GSAP rendered it around the element's centre, and the element jumped by the whole drag distance.

5. The correction was measured against a scale the file never gets. A near-uniform drag collapses to the scale shorthand, but the correction was measured at the per-axis pair, so it came out tilted by the difference between them.

4. The correction was measured against the wrong origin. It measured while the gesture's own translation was still applied, but the position commit adds the correction onto the element's pre-gesture position. The two disagreed by the drag distance, so the persisted position was a drag-length from the drop point.

Why

1. The scale route needs the element's original box to work out dropped / original. The only original size recorded was the element's inline width and height, captured so a reset can put them back. Those are empty for anything sized by a stylesheet, which is how compositions are written, so almost every element fell through to the 200 fallback.

2. The mixing hazard was already known in the other direction: a non-uniform drag takes a rewrite path whose comment says it exists because "a shorthand/longhand mix would leave the old scale sub-tween running against the new scaleX/scaleY". The uniform direction had no such guard, so it created exactly the mix the other branch was written to avoid.

3. Whether the commit had rendered before the measurement was luck. Elements that had been resized before got a correction only because their previous scale made the residual non-zero, which is why this read as "the first drag is broken, the second is fine".

4. The commit composes its delta onto the base pose the gesture stamped at drag start, not onto the element's live value. Measuring from the live (dragged) value and persisting against the base value silently mixes two coordinate origins.

How

1. The draft records the box it measured, once, in the branch that already runs exactly once before it writes a width of its own. That is the last moment the element still has the box the user started with, and offset sizes are layout, so a scale animation on the element does not distort them. The intercept prefers that measurement and falls back to the inline value, which is still real for elements that carry one.

The two new attributes are record-only. They ride the existing BOX_SIZE_ORIG_ATTRS list so they are persisted, cleared and snapshot-restored with the rest, and restore no style property, the same shape data-hf-studio-original-transform-display already uses.

2. The condition becomes symmetric: whenever the tween already speaks longhands, a uniform drag speaks them too, and takes the same normalizing rewrite path. The tween never holds both forms in either direction.

3. Put the committed scale on the live element before measuring. It costs nothing when the commit has already rendered (same value) and makes the measurement mean what it says either way.

4. Move the element back to the gesture's base pose before measuring, so the residual and the commit share one origin. For an element whose position is a static hold that usually means no correction at all, which is the right answer: scaling about the centre already leaves it on the drop point. An element whose position is animated gets the correction written into that tween at the playhead, through the same commit a drag on it uses, rather than a static hold the tween overrides a frame later.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)

Regression tests in gsapResizeIntercept.test.ts, each verified to fail on the previous code:

  • resizing a stylesheet-sized element to exactly twice its box commits a scale of about 2, where the old code committed 6.3
  • a uniform drop onto a longhand tween emits no keyframe holding both forms, where the old code emitted { scaleX: 1, scaleY: 1, scale: 1 }
  • a scale resize of a statically positioned element persists its original position, where the old code persisted one a drag-length away

Plus a sweep in gsapResizeDropPoint.test.ts over the shapes a composition produces — shrink, grow, first resize, rotated, steeply rotated, non-uniform, near-zero, inline-sized, no position write, animated position, and two drags in a row. Each drives one resize through the real intercept, then re-renders the element from the persisted scale and position and checks it is still on the drop point. Asserting on persisted rather than live values is the point: every bug here showed as a correct drop frame followed by the element sliding to whatever reached disk. The geometry model is calibrated against real gesture traces — the same inputs reproduce the rects the browser reported to three decimals.

manualEditsDomPatches.test.ts covers both new attributes for build, clear and ordering. Full studio suite green: 3494 passing.

All five were reproduced in Studio with hf-resize-debug enabled, against a real composition and a purpose-built fixture covering longhand scale, shorthand scale, rotated, unanimated and inline-sized elements. Bug 1 showed as a scaleX: 11.955 keyframe on a 630px element after two drags. Bug 2 showed as intercept-route computing the correct newScaleX: 0.61 followed by a settle reporting the element back at its original 630px. Bugs 3, 4 and 5 showed as a settle pair whose t0 sat on the drop point and whose t200 had moved by the drag distance. All five fixture elements now hold their drop point across repeated drags.

Resizing an element whose size is driven by a scale animation committed a
scale computed against a hardcoded 200px fallback, because the only
original size the draft recorded was the element's INLINE width, and a
composition sizes its elements from the stylesheet.

A 630px chip dropped at 1260px wide committed a scale of 6.3 instead of 2,
so it landed at over three times the size it was dropped at. The next drag
compounded it, because that wrong scale then counted as the element's live
one.

The draft now records the box it measured, once, before it writes a width
of its own, and the intercept reads that. The inline attributes keep their
own job of restoring an inline style, which is why they cannot answer this
question.
A uniform drag committed the `scale` shorthand. If the tween's keyframes
already stated `scaleX` and `scaleY`, the commit left both forms in the
same keyframe, and GSAP animates each property name independently, so the
longhands ran alongside the shorthand and won.

The resize therefore computed the right number, wrote it to the file, and
did nothing: the element snapped back to its old size the moment the
handle was released. Reproduced from a real session, where a drop at 384px
on a 630px element wrote {scaleX: 1, scaleY: 1, scale: 0.61} and rendered
at the original size.

The mixing hazard was already known in the other direction, where a
non-uniform drag takes a rewrite path that normalizes every keyframe to
the longhands. This makes the condition symmetric: whenever the tween
already speaks longhands, a uniform drag speaks them too.
@miguel-heygen miguel-heygen changed the title fix(studio): resize from the element's real box, not a 200px guess fix(studio): resize an animated element to the size it was dropped at Aug 6, 2026
The finalize step measures where the committed scale put the box and
shifts the position hold by the difference. Whether the commit had
actually rendered when it measured was luck: on a first resize the
timeline had not re-seeked, so it measured the element at its natural
size still sitting on the drop point, saw no residual, and skipped the
correction. The scale then landed, GSAP rendered it about the element's
centre, and the element jumped by the whole drag distance. Elements
resized before got a correction only because their previous scale made
the residual non-zero by accident.

The committed scale is now applied to the live element before measuring,
so the measurement means what its comment says either way, and a skipped
correction is logged rather than silent.

Confirmed against a real session: a first resize of a 630px chip now
reports residual -109.93 and lands on the drop point, where it previously
logged no scale-finalize at all.
An element whose position is animated left the drop point anyway. The
finalize step wrote its correction as a static position hold, and the
element's position tween rendered its own value a frame later and won.
Before that it stood down entirely on such elements, on the grounds that
a keyframed path has no single anchor to preserve, which had the same
visible result: the element moved.

It has an anchor, the frame the user is looking at. The correction now
goes into that tween at the playhead, through commitGsapPositionFromDrag,
which is the same commit a drag on the same element already uses. Static
holds keep the existing path.

This is the difference the debug log showed between an element carrying
position:to, which moved after release, and one carrying position:set,
which did not.
A scale resize measured its drop-point correction while the gesture's own
translation was still applied, but the position commit adds that correction
onto the element's PRE-gesture position, which it reads from the gesture's
base attributes. The two disagreed by the whole drag distance, so the commit
persisted a position a drag-length from where the element was dropped: it
held the drop point for one frame and then slid off.

Move the element back to that base before measuring, so the residual and the
commit share one origin. For an element whose position is a static hold that
usually means no correction at all, which is the right answer: scaling about
the centre already leaves it on the drop point.
@miguel-heygen
miguel-heygen force-pushed the fix-studio-resize-box branch from 0e7173d to ee5ae96 Compare August 7, 2026 00:05

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 adversarial pass — APPROVE. Grade: A.


Correctness — CORRECT. All four fixes trace end-to-end and each has a regression test that fails on the prior code.

  1. originalBoxSize (measured attr → inline attr → 200) at packages/studio/src/hooks/gsapResizeIntercept.ts lines ~197-205: the stylesheet-sized-element case now has a real measurement. The two new attrs ride the existing BOX_SIZE_ORIG_ATTRS list in manualEditsDomPatches.ts (marked with "" style prop so they clear without restoring), snapshot/restore land in manualEditsSnapshot.ts, and the write-once site in manualEditsDom.ts writeStudioBoxSizeVars uses offsetWidth/offsetHeight — layout numbers, so an in-flight scale animation doesn't distort them.
  2. tweenUsesScaleLonghands (checks both keyframes.keyframes frames and top-level properties for scaleX/scaleY) → useScaleLonghands = nonUniformScale || tweenUsesScaleLonghands(anim), then the outside-range keyframe-normalize branch downstream is gated on useScaleLonghands instead of nonUniformScale. The shorthand-longhand-mix trap the non-uniform branch already documented is now closed in both directions. Test asserts no keyframe holds both forms and the resize still lands at ~0.61.
  3. setElementGsapScale(scaleDraftEl, committedScale.x, committedScale.y) immediately before the finalize measurement removes the "first resize luck" branch — the residual now means the same thing whether the commit has re-rendered yet or not.
  4. computeDraggedGsapPosition(selection.element, {x:0,y:0}, gsapPos) returns the pre-gesture base pose (the helper is a pure math function; no side effects). Then setElementGsapPosition(scaleDraftEl, base.x, base.y) puts the element back to base before measuring; delta and corrected are computed off base instead of gsapPos. The commit is now composed onto the same origin the measurement was taken from.
  5. The former hasLivePositionTween → skipped guard is now REPLACED with pickClosestToPlayhead(position tweens with duration>0)commitGsapPositionFromDrag(..., delta, base, ...). Keyframed-position elements no longer silently ship uncorrected.

Adversarial pass — 6 boundary axes at fix boundary, editor-UI + parity lens, Standards checklist.

  • Attr-empty regression: originalBoxSize still falls to 200 only if BOTH measured and inline are empty/≤0. writeStudioBoxSizeVars runs unconditionally in the !hasAttribute(STUDIO_BOX_SIZE_ATTR) branch, so by the time the intercept runs on any resize-drafted element the measured attrs should be present. Only prior-marked-but-attr-missing elements (persisted from before this PR loaded) would hit the fallback — a one-time transitional case, harmless.
  • Persisted-snapshot compatibility: restoreStudioBoxSize reads previous.originalBoxWidth/Height from the snapshot object — a snapshot serialized by pre-PR code would have undefined for those, and restoreAttribute(element, ATTR, undefined) needs to behave like "no restore." Given the pattern is identical to the existing originalMinWidth/etc. entries which have shipped, this is well-trodden.
  • tweenUsesScaleLonghands domain: checks anim?.keyframes?.keyframes frames and top-level anim?.properties, not any sibling tween on the element. Correct — the outside-range branch normalizes THIS tween's keyframes, sibling tweens don't need touching.
  • Element-identity across finalize: the scaleDraftEl capture happens inside the resizeGroup === "scale" block and is used again in the closure. No re-query between those points, so no drift.
  • setElementGsap* side effects post-return: the live-element scale + position sets in finalize aren't undone if the code returns early, but commitMutation triggers a soft reload that will overwrite the live state from persistence. Idempotent.
  • Number rounding: Math.round(base.x + residual.x) + Math.round(base.y + residual.y) matches the existing convention in computeDraggedGsapPosition. Live runtime and persisted file compose the same value.
  • Delta parity with existing drag: the switch from commitStaticGsapPosition(selection, {corrected - gsapPos}, gsapPos, ...) to commitGsapPositionFromDrag(selection, positionTween, delta, base, ...) — same base semantic (drag-scratch attr fallback via computeDraggedGsapPosition), same delta semantic. Route change lands on an already-used call site.

Non-blocker findings:

  1. gsapResizeIntercept.test.ts line ~283-350 — the commitGsapPositionFromDrag path is untested. Test 3's positionHold fixture has duration: 0, which fails the resolveTweenDuration(a) > 0 filter, so pickClosestToPlayhead returns undefined and the code falls to commitStaticGsapPosition. The keyframed-position path — where the PR body says "gets the correction written into that tween at the playhead" — has no regression test. Suggest a follow-up test with an animated position tween (duration > 0), asserting commitGsapPositionFromDrag is called with the expected delta and base.

  2. setElementGsapScale / setElementGsapPosition (utils/elementGsap.ts) return false when gsap is unreachable in the element's realm, but both callers in the new finalize path ignore the return. In practice, if we reached finalize the iframe has gsap. But if it ever doesn't — cross-realm timing, teardown race — the finalize measurement below sits on stale geometry and computes a wrong correction, silently. A logResize("scale-finalize", { warn: "gsap-set-unreachable" }) on !ok would surface it for a future debugger.

  3. Sibling attr use in packages/studio/src/hooks/gsapDragCommit.ts:342-344 (resolvePriorSize) reads STUDIO_ORIGINAL_WIDTH_ATTR the way this PR just deprecated for the scale route — the inline attr, empty for stylesheet-sized elements. Different downstream: the fallback is fallbackW (the new size), so the symptom is quieter — prior keyframes get pinned to the new size rather than the wildly-wrong 3× jump — but same class of empty-for-stylesheet read. Mirror originalBoxSize here in a follow-up so the keyframed-size route reads the same measured box.


Merge on green. Tests are per-bug and shaped like real regressions rather than passing-tautologies.

Review by Via

A near-uniform drag collapses to the `scale` shorthand, but the finalize step
measured the element at the per-axis pair it computed rather than the single
value the commit writes. The element was measured at a scaleY the file never
gets, so the position correction came out tilted by the difference.

Adds a sweep over the shapes a composition produces — shrink, grow, first
resize, rotated, steeply rotated, non-uniform, near-zero, inline-sized, no
position write, animated position, and two drags in a row — each checking the
element renders on its drop point from the PERSISTED scale and position. The
geometry model is calibrated against real gesture traces: the same inputs
reproduce the rects the browser reported to three decimals.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at ee5ae961.

Four independent-but-entangled bugs in the same gesture, and the PR body reads unusually cleanly: each bug has a numbered What / Why / How, a real repro from the Studio debug log (630px chip → scaleX 11.955 after two drags, intercept-routesettle back at 630, settle pair with t0 on the drop point and t200 a drag away), and a regression test verified to fail on the previous code. Miguel-style rigor — the diagnosis reads convincing on every one of the four.

Small details I liked:

  • The data-hf-studio-original-box-* capture at manualEditsDom.ts:370-371 runs inside the !hasAttribute(STUDIO_BOX_SIZE_ATTR) branch, so it fires exactly once per gesture cycle — before applyStudioBoxSizeDimensions writes an inline width — which is the only point where offsetWidth is still the pre-gesture layout box. The ponytail: on the sibling comment ("Offset sizes are layout, so a scale animation ... does not distort them") saves a future reader the trip.
  • tweenUsesScaleLonghands (gsapResizeIntercept.ts:86-92) checks BOTH keyframes.keyframes[].properties and root anim.properties. The obvious version reads only keyframes and misses flat tweens; catching both is the shape the fix needs.
  • The rewritten route symmetry: useScaleLonghands = nonUniformScale || tweenUsesScaleLonghands(anim) (iframe.ts:243) — the earlier code had a comment on the non-uniform branch explaining why the mix hazard existed there, but no guard on the uniform side, and the uniform side then produced the exact mix the other branch was written to avoid. Symmetric now.
  • The 3D-rotation caveat spelled out in the finalize docstring (iframe.ts:284, "the rects are AABBs, so the anchor is approximate rather than corner-exact") — the known limitation lands in the code rather than the follow-up bug that reports it.
  • finalizeScaleResizeCommit moved the setElementGsapScale + setElementGsapPosition(base) steps in front of the measurement, so the getBoundingClientRect at iframe.ts:326 finally reports "where the commit puts the element" rather than "where it happened to be mid-flight." The comment at :305 naming the FIRST-resize luck (previous scale making the residual non-zero) is the shape of comment that outlives its own bug.
  • Persistence symmetry landed: manualEditsDomPatches.ts:141-142 records data-hf-studio-original-box-* with styleProp = "" so the loop in buildClearBoxSizePatches skips restoring a style but still emits the null-attr op — and the test at manualEditsDomPatches.test.ts:228-229 pins that.

Findings inline — one concern on a new code path that isn't regression-pinned, and one nit on a legacy-file migration hole where the exact bug the PR fixes could still fire. Both non-blocking; the shape is right.

One passing observation (not a finding, no action needed). The live DOM clearStudioBoxSize in manualEditsSnapshot.ts:292-321 doesn't remove the two new data-hf-studio-original-box-* attrs, unlike the persistence-path buildClearBoxSizePatches which does. In practice the asymmetry self-heals: the next gesture on the same element either sees STUDIO_BOX_SIZE_ATTR re-added or (after clear) enters the outer if and setAttribute overwrites the lingering values with fresh offsetWidth. So this is symmetry-for-future-maintainers, not a behavior bug.

Review by Rames D Jusso

Comment thread packages/studio/src/hooks/gsapResizeIntercept.ts
Comment thread packages/studio/src/hooks/gsapResizeIntercept.ts

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 delta @ a693b12. APPROVE. Grade unchanged: A.

(Both fixes I asked for AND the fifth-bug catch that was implicit in my R1 grade-A-not-A+ comment are in this commit.)


Bug 5 — shorthand-write measurement drift (new):

-committedScale = { x: newScaleX, y: newScaleY };
+committedScale = useScaleLonghands
+  ? { x: newScaleX, y: newScaleY }
+  : { x: newScaleX, y: newScaleX };

Correct. When useScaleLonghands is false, the commit writes { scale: newScaleX } — a single number that GSAP applies as BOTH scaleX and scaleY. Taking the per-axis pair here meant finalize measured the live rect at a scaleY the persisted file never gets. For near-uniform drags (|newScaleX - newScaleY| ≤ 0.01 — the threshold that flips nonUniformScale), the two are 1% apart, tilting the correction by ~1% of the box. The fix mirrors exactly what the file writes.


gsapResizeDropPoint.test.ts (+389, new file) — persisted-geometry sweep.

Model: Pose(box/pos/scale) + renderRect(AABB of translate/rotate/scale about centre, matching what getBoundingClientRect returns) + runCase — drives the real intercept, then reconstructs settled from the PERSISTED writes (not the live state) and asserts settleddropPoint. The shape is the load-bearing bit: every bug in this class rendered fine at drop and then slid to the persisted value; asserting on persisted-not-live is what actually catches drift.

Coverage sweep (10 cases + second-drag):

  1. shrink stylesheet-sized ✓
  2. grow stylesheet-sized ✓
  3. first-resize (liveScale: 1,1) ✓ — regression coverage for the R1 setElementGsapScale set-before-measure fix
  4. rotated -8° with longhand tween ✓
  5. steeply rotated -47° with longhand ✓
  6. non-uniform drag with longhand ✓
  7. shrink almost to nothing (13x5) ✓ — extreme-scale edge
  8. inline-sized element (uses the pre-PR data-hf-studio-original-width inline-attr fallback branch) ✓
  9. no position write at all (positionWrite: "none") ✓
  10. animated position (positionWrite: "keyframed-tween") ✓ — the exact R1 gap I flagged. commitGsapPositionFromDrag branch now rehearsed.
  11. Second drag in a row (bug 1 regression at real numbers: first drag settles to 900/630 = 1.429, second drag verifies bug isn't compounding) ✓

Rotation coverage is a genuinely useful axis I didn't ask for — the getBoundingClientRect AABB approximation is called out in the code's own "ponytail" comment, and cases 4-5 verify the approximation holds at both mild and steep angles.


Adversarial pass on the delta:

  • renderRect matches browser behavior: AABB uses |cos| + |sin| on the scaled box dimensions, centered on LAYOUT + box/2 + pos. That's the standard formula for the AABB of a rotated rectangle about its centre. ✓
  • Test model reproducibility: PR body claims it's calibrated against real hf-resize-debug output to three decimals — the CASES fixtures use non-round numbers (liveScale: 1.192, 1.2 at rotation -47°) that would only survive if the model actually reproduces real geometry.
  • inlineSized branch (case 8) tests the fallback path: writes data-hf-studio-original-width = "500px" (with px suffix), and originalBoxSize uses parseFloat which handles the suffix. Good — this proves the fallback still works for the legacy-marked case.
  • Second-drag case: verifies settledPos = writes.at(-1) composes correctly onto the next drag. The liveScale: 1.429 is 900/630 — the first drag's actual committed scale. That's the sanity-check I would have wanted.
  • runCase gsap stub reads rotation as a getter-only property (no set on rotation) — the intercept doesn't set rotation, so this is correct scope.
  • AABB anchor is approximate for rotated: the PR body concedes this and the tests use toBeCloseTo(..., 0) (± 0.5 px tolerance). The 1-px slack absorbs the anchor approximation on rotated cases 4-5.

Non-blockers from R1 still open (unchanged in delta):

  • setElementGsapScale / setElementGsapPosition return false on unreachable gsap — callers still ignore.
  • gsapDragCommit.ts:342-344 resolvePriorSize still reads STUDIO_ORIGINAL_WIDTH_ATTR (inline attr) for the keyframed-size route — quieter symptom class.

Both carry forward as follow-up notes, not blockers.


CI status at a693b12: 26 success, 11 skipped, 6 pending, 1 neutral, 1 failure — Fallow audit.

Fallow report on this head has 9 total findings; the new-vs-baseline set that's blocking is:

  • packages/studio/src/hooks/gsapResizeDropPoint.test.ts:181runCase CRAP 31.6 (threshold 30.0, cyclomatic 10). Minor severity. The sweep driver has become non-trivial; either extract the DOM-setup + gsap-stub + rect calibration into helper functions to bring cyclomatic under 10, or annotate with // fallow-ignore-next-line high-crap-score if the reviewer judges the current shape load-bearing.
  • Possibly new fallow/code-duplication entries in gsapResizeIntercept.test.ts at lines 296 and 342 (the 3 new tests from the previous commit added a 16-line clone pair). If so, factor out the shared JSDOM + gsapStub scaffolding into a helper.
  • Pre-existing Fallow health findings in manualEditsDom.ts (stripGsapTranslateFromTransform / applyStudioPathOffsetViaGsap) and gsapRuntimeBridge.test.ts duplication are baseline noise, not new — this PR's only touch on manualEditsDom.ts is a small writeStudioBoxSizeVars addition + import block.

Merge on green — approval is contingent on the Fallow gate clearing. Nothing about the code correctness or the sweep design is at issue; only the health-metric threshold on the new test driver.

Review by Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at a693b12 (delta since ee5ae961).

Both R1 findings addressed, and a fifth bug picked up in the same pass:

  • Concern closed with more rigor than asked. packages/studio/src/hooks/gsapResizeDropPoint.test.ts is a data-driven sweep across 10 scale-resize shapes — stylesheet-sized, inline-sized, first-time, rotated (mild and steep), non-uniform, near-tiny, no-position-write, keyframed-position, and the second-drag-across-first-commit — each asserting the settled AABB lands on the drop-point AABB. The keyframed-tween case is the exact shape I asked for (duration: 2 position tween, keyframes at 0/100%, drop invariant checked against persistedPositions which extracts x/y from BOTH mutation.properties and keyframes[].properties) and persistedScale pulls the commit at percentage 0. The renderRect model calibrated against real hf-resize-debug output (three-decimal-place match on the reported browser rect) is the shape of test that carries — one file, all future geometry regressions in this gesture surface here.
  • Nit stands as a follow-up per the R1 suggestion, no code change here. That's fine.
  • Fifth bug fixed. gsapResizeIntercept.ts:258-264 — when useScaleLonghands=false the commit writes the shorthand and both axes render at newScaleX, but the pre-fix committedScale = { x: newScaleX, y: newScaleY } had the finalize measure at a newScaleY the file never gets. On any near-uniform drag whose per-axis ratios diverged by even a hundredth, the correction tilted by the difference. The new tri-branch (useScaleLonghands ? per-axis : { x: newScaleX, y: newScaleX }) matches what the commit actually writes, and the shorthand-path cases in the sweep (shrinks, grows, first-time, shrinks-almost-to-nothing) pin it — the "grows" case at 1319/630 = 2.093 vs 527/252 = 2.091 is exactly the near-uniform shape the bug hid in. Comment (A near-uniform drag collapses to the shorthand, so taking the per-axis pair here measured the element at a scaleY the file never gets and tilted the correction by the difference) is the shape of comment that stays useful past the fix.

The persisted-vs-live-invariant framing in the file docstring (every bug in this class showed as a correct-looking drop frame followed by the element sliding to whatever got written to disk) is the right invariant to codify — it makes drop-point regressions in this gesture a one-line assertion instead of a manual repro chase.

LGTM from my side — leaving as a comment.

Review by Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full Studio resize path at exact head 12e637fb, including the measured-box snapshot lifecycle, scale shorthand/longhand normalization, live committed-pose measurement, static and keyframed position persistence, and the new persisted-geometry sweep.

I found one valid compatibility hole in the prior head: a pre-PR element could already carry data-hf-studio-box-size while lacking the new measured-box attrs, leaving the 200px fallback reachable. Fixed here by giving each measured attr its own migration-safe guard before the draft overwrites dimensions, with the exact 630×252 legacy-marker regression fixture.

I also cleared the exact Fallow blocker by extracting the element-fixture setup from runCase, and suppressed the two intentional test-shape clone blocks already proven equivalent to the runtime-bridge fixtures. Evidence: 46/46 targeted tests, full build, lint clean, format clean, typecheck via pre-commit, and Fallow reports no new gated issues. Both substantive review threads have evidence replies and are resolved.

No remaining code blocker found. The two utility-set observability and sibling keyframed-size measurement notes are legitimate follow-ups, but neither changes this PR's scale-resize correctness boundary.

Verdict: WAIT — code is ready, but the new exact head still needs an exact-head approval and terminal-green required CI.
Reasoning: The implementation and regression evidence are sound; the remaining gates are review provenance and CI completion, not code defects.
— Magi

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3 delta @ 12e637fb. APPROVE. Grade unchanged: A.


Fix (1) — legacy resize-marked backfill (manualEditsDom.ts):

Moved the two setAttribute(STUDIO_ORIGINAL_BOX_*_ATTR) writes out of the !hasAttribute(STUDIO_BOX_SIZE_ATTR) guard and onto their own !hasAttribute(STUDIO_ORIGINAL_BOX_*_ATTR) migration-safe guards. The inline-attr writes (width/height/min/max/transform) still ride the outer marker check — those were correctly captured on first-mark and are already present on legacy elements, so no need to re-write. Only the NEW attrs need the standalone guard, and they're written before the caller overwrites el.style.width/height, so offsetWidth/offsetHeight still reflect the pre-resize layout box. Correct.

Adversarial checks:

  • Attribute-write idempotence: !hasAttribute is strictly stronger than value-match, so repeated resize passes on an already-migrated element skip the write entirely — no re-measurement drift across gestures. ✓
  • offsetWidth === 0 (hidden element): box-width attr gets set to "0", which originalBoxSize's > 0 check rejects → falls through to inline attr → then 200. Acceptable — hidden elements can't be dragged anyway.
  • Ordering vs el.style.width mutation: the draft mutation happens outside this function in the caller flow, so the offsetWidth read here still sees the pre-draft box. ✓

Fix (1) test — manualEditsDomPatches.test.ts:267-280: creates a div(), sets STUDIO_BOX_SIZE_ATTR="true" (simulating a legacy resize-marked element), stubs offsetWidth/offsetHeight via Object.defineProperties, calls applyStudioBoxSize, asserts the two new box attrs land. Exactly the regression the PR body claims. ✓


Fix (2) — Fallow CRAP refactor in gsapResizeDropPoint.test.ts:

Extracted createResizeElement(testCase): HTMLElement — the DOM creation + attr-writing that was inline in runCase. runCase now calls it as a one-liner. Cyclomatic drops (the if/else on inlineSized moved into the helper), bringing CRAP below the 30.0 threshold. CI Fallow audit is now green at this head, so the target was hit.

Intentional-clone scope in gsapResizeIntercept.test.ts: // fallow-ignore-next-line code-duplication markers added on the it() lines for the bug-1 and bug-2 tests (293, 340), matching the Fallow-reported line numbers (296, 342) for the clone group. The two per-test DOM+gsapStub scaffolds are load-bearing to the test's readability as a per-bug demonstration; over-abstracting them into a shared helper would erase the "one bug, one test" narrative Miguel's structure preserves. Reasonable scope call.


CI at 12e637fb: 20 success (incl. Fallow audit), 1 neutral (CodeQL), 11 skipped, 10 pending. No failures. Windows / Producer / Preview / Typecheck / Studio matrix jobs still running.

Merge on green.


Two R1 non-blockers carry forward unchanged as follow-up notes:

  • setElementGsapScale / setElementGsapPosition return false on unreachable gsap — callers still ignore.
  • gsapDragCommit.ts:342-344 resolvePriorSize still reads STUDIO_ORIGINAL_WIDTH_ATTR for the keyframed-size route — quieter symptom class.

Neither blocks; both are optional cleanups for a follow-up PR.

Review by Via

@miguel-heygen
miguel-heygen merged commit c7b2199 into main Aug 7, 2026
74 of 76 checks passed
@miguel-heygen
miguel-heygen deleted the fix-studio-resize-box branch August 7, 2026 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants