fix(studio): hold a resized element's size while the timeline is rebuilt - #3140
fix(studio): hold a resized element's size while the timeline is rebuilt#3140miguel-heygen wants to merge 5 commits into
Conversation
556250e to
3eae95d
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 3eae95d23.
Right fix for the class. The size channel is last-writer-wins on the same numeric properties (unlike offset, which composes), and standing aside for GSAP-sized elements meant the seek reapply left nothing holding the studio size across the soft-reload window — element sat at the stylesheet size for the flash. Dropping the gsapAnimatesProperty(el, "width", "height") skip closes the window. The "only elements the studio has committed a size on carry these vars" invariant is what makes it safe to unconditionally reapply.
Test at reapplyBoxSizeAfterSeek.test.ts:35-43 is a direct contract lock (GSAP put the tween's starting size back → we put the studio's committed size back on top). The paired negative test at :45-53 proves the "leave GSAP alone once the studio props are cleared" reciprocal — this is the reassuring half, because it pins the reason the fix doesn't affect elements the user never resized.
Nice small refactor tucked in alongside: reapplyPathOffsets at manualEditsDom.ts:557-568 gets an early continue and a leading comment // Unlike size below, the offset channels COMPOSE — applying both doubles the move. — makes the two functions read as intentionally different, which the previous shape didn't quite convey.
Clean, no findings from my side. LGTM.
3eae95d to
7bd4d72
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review
Verdict: APPROVE (high confidence). Small, well-diagnosed fix. reapplyBoxSizes used to skip GSAP-sized elements, which left a hole during the soft-reload teardown/rebuild window: GSAP would restore each tween's recorded starting width on the way out, nothing held the committed size, and the element sat at its stylesheet size until the new timeline rendered — the visible "jump after resize." Removing the skip pins width/height to STUDIO_WIDTH_PROP / STUDIO_HEIGHT_PROP at every seek so the resized size holds. Diagnosis is corroborated by the preceding debug commit (chore(studio): name whoever puts the pre-resize size back) — Miguel first proved the mechanism with logging, then fixed the right end.
The reapplyPathOffsets refactor in the same hunk is behavior-preserving (moves the GSAP skip earlier, extracts the offset object literal). Old if (x || y) and new if (!x && !y) continue; are inverses of the same check; Number.parseFloat(x) || 0 is unchanged.
P1 blockers
None.
P2 non-blockers
- Docstring / PR-body claim is imprecise.
manualEditsDom.ts:568-575and the PR body say "Only an element mid-edit carries the vars, so this cannot affect anything the user is not actively resizing." That's not what the code does.STUDIO_BOX_SIZE_ATTR+STUDIO_WIDTH_PROPare written into the source HTML bybuildBoxSizePatches(manualEditsDomPatches.ts:152-163) and only cleared byclearStudioBoxSize(from "Reset layer edits" or fromfinalizeScaleResizeCommit). They persist across sessions on any element that has ever been resized in the studio. The fix is still correct — the intended scope is "any element the user has resized" — but the phrase "actively resizing" reads as "during the drag gesture only," which underspecifies the blast radius. Suggest: "Only an element the studio has resized carries the vars; untouched elements are unaffected." - Trade-off with user-authored width/height GSAP tweens. The studio's animation panel exposes
widthandheightas animatable properties (gsapAnimationConstants.ts:20-21, 53-54, 88-89, 162-163), and the seek reapply runs onseek()AND everyrAFtick during playback (viastartStudioManualEditPlaybackReapplyinmanualEdits.ts:168-182). An element that is both (a) resized via the studio gesture and (b) targeted by a user-authoredwidth/heightGSAP tween with different start/end values now has that tween pinned toSTUDIO_WIDTH_PROPon every frame — the animation freezes at the committed base size. Old code side-stepped this by skipping GSAP-sized elements entirely. Trade seems worth it (resize→scrub jump is common; resize + width-tween is exotic), but consider aTODO/follow-up note in the comment, or a test that documents the intended precedence.
P3 nits
- Test 2 has a fixture-initial tautology.
reapplyBoxSizeAfterSeek.test.ts:56-64setsel.style.width = "395px", clears the studio vars, callsreapplyPositionEditsAfterSeek, and assertsel.style.width === "395px". The fixture initial value equals the expected outcome, so a mutation that made the reapply a full no-op would still pass. Per the mutation-resistance principle, consider additionally asserting thatSTUDIO_BOX_SIZE_ATTRis still"true"and thatapplyStudioBoxSizewas not invoked (e.g. via a spy onsetProperty("width", …)), so the test discriminates "did nothing because vars are empty" from "did nothing at all." - Comment symmetry. The comment above
reapplyBoxSizes(L568-575) references "the offset above"; the two comments read as a pair, butreapplyPathOffsetsis only one line (manualEditsDom.ts:558) whilereapplyBoxSizesexplains itself in seven. Consider symmetrizing.
Per-lens findings
- What was the bug — resize → soft-reload window → GSAP hands back tween's recorded starting width → element sits at stylesheet size until new timeline renders → visible jump. Mechanism traced in the second commit body against real logs.
- Persistence mechanism — CSS custom properties (
STUDIO_WIDTH_PROP,STUDIO_HEIGHT_PROP) plus a data-attribute marker (STUDIO_BOX_SIZE_ATTR). Written to the source HTML viabuildBoxSizePatches. Not keyframe-based; base-size only. - Seek behavior — all seek entry points route through
__hfStudioManualEditsApply→reapplyPositionEditsAfterSeek. Wrapped seek/totalTime/play/pause on__hf,__player,__timeline, and every entry in__timelines. Includes aProxyon__timelinesso late-registered timelines get wrapped. Coverage complete. - Multi-keyframe elements — the fix pins to the base studio size at every seek/frame; no keyframe-aware routing. Elements with legitimate multi-keyframe width/height animations are the P2 concern.
- Undo/redo — snapshotted through
manualEditsSnapshot.ts:126-128. Not touched by this diff. - Instant-hold interaction (HF #3092) — that fix lives in
gsapResizeIntercept.ts(scale-route commit path), disjoint surface. No overlap. useEffect/ state syncing — none added. Pure DOM helpers.- Element-identity — marker + vars live on the DOM node itself; not keyed on id.
- Test coverage — first test proves the regression. Second test guards the "vars cleared" no-op path with the tautology caveat above. No test exercises playback rAF loop or animating (not constant-value) width tween.
- CI — at HEAD
3eae95d: no failures. Regression shards 2/3/5/7/8 + Windows still IN_PROGRESS at review time; every finished check green.
— Via
7bd4d72 to
a7f81e4
Compare
a7f81e4 to
1377e60
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 re-verify @ 1377e60cb
Verdict: APPROVE. Miguel's "unchanged code" claim is correct — manualEditsDom.ts and reapplyBoxSizeAfterSeek.test.ts are byte-identical vs prior head 3eae95d. PR description now precisely states the resize scope and the width/height precedence, closing the R1 P2 items at the doc level.
Per-finding delta:
- P2-1 (imprecise docstring, precise scope): FIXED via PR body. The imprecise sentence R1 quoted ("Only an element mid-edit carries the vars…") had already been folded out of the docstring by the prior head; R1 was actually quoting an earlier commit. PR body now states verbatim: "Only elements on which Studio has committed a size carry the reapply variables. Those variables intentionally remain authoritative across seeks; when the same element also has authored width or height GSAP tweens, the Studio-committed size wins until a later Studio resize changes it." Matches the R1 suggested rewrite semantically.
- P2-2 (precedence with user-authored width/height GSAP tweens): PARTIAL. FIXED in PR body ("Studio-committed size wins…") and in test plan (manual checklist item). NOT ADDRESSED in code comment (
manualEditsDom.ts:568-575docstring still doesn't mention the tween-precedence trade-off) or in an automated test (reapplyBoxSizeAfterSeek.test.tsunchanged). Non-blocking — the trade-off was flagged as non-blocking in R1, and the PR-body + test-plan resolution covers the immediate need — but PR bodies aren't durable, so a one-line note in the comment would close the doc drift for future readers.
P3 nits: UNCHANGED (informational).
- Test 2 fixture-initial tautology at
reapplyBoxSizeAfterSeek.test.ts:56-64— still present; the guard still parses NaN and skips, but no positive discriminator/spy added. - Comment symmetry between
reapplyBoxSizes(7 lines) andreapplyPathOffsets(1 line) — asymmetry preserved.
CI: Green on completed checks (Build, CLI smoke, CLI npx macos/ubuntu/windows, Format, Lint, Typecheck, Producer unit + integration, Test: runtime contract, Preview parity, Perf: drift/fps/load/scrub, Preflight, SDK: unit/contract/smoke, Fallow audit, File size, Studio: load smoke + timeline viewport gate, preview-regression, Analyze actions/python, Codex plugin package). In-progress: Perf: parity, Render+Tests on windows-latest, Test, Smoke: global install, regression-shards 1-9.
Stamp holds pending in-progress checks completing green.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-verified at 1377e60cb against prior R1 (3eae95d23).
The two files in this PR's own scope (manualEditsDom.ts + reapplyBoxSizeAfterSeek.test.ts) are byte-identical to the prior R1 head — the only change between the two is a docblock tightening on reapplyBoxSizes at manualEditsDom.ts:568-574 (folded the pre-write jump narrative into a shorter form without losing the load-bearing "size does not compose, later write wins on the same number; standing aside meant nothing held the size across the soft-reload window" invariant). Docblock still names why size differs from offset and why the reapply is safe.
Prior R1 LGTM stands. No new findings.
1377e60 to
0df3d7e
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-verified 0df3d7ee5 against prior R2 (1377e60cb).
Both files in this PR's own 2-file scope (manualEditsDom.ts + reapplyBoxSizeAfterSeek.test.ts) are byte-identical to the prior R2 head — the delta at this SHA is stack drift from the fileWatcher fix in #3137. Prior R1/R2 LGTM stands. Review at 4892125186.
No new findings.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R3 re-stamp @ 0df3d7e — Byte-clean rebase on #3137 R3 (f1de808). Own-delta against parent (f1de808e7...0df3d7ee5) = 47 files with blob SHAs and +/- counts identical to R2 own-delta (4d2a48ae6...1377e60cb); no code change to #3140 introduced by the rebase, only the fileWatcher burst-fix propagated up from #3137. Prior R2 approval stands. — Via
Resizing the card commits correctly — the source and a fresh load both read 273x181 — but 200ms after the drop, mid-commit, the element renders at 395x261 with the studio size vars still holding 273x181. Something writes the pre-gesture size back inline while the reload is still in flight, and every writer of that size was silent. Both are traced now under the existing hf-resize-debug flag, each with the size going in, the size being replaced, and a short stack. Restoring the pre-gesture size is right on a cancel and wrong after a successful commit, and the function doing it cannot tell the two apart from the inside — so the caller has to be named before this can be fixed at the right end.
Your log caught it across two resizes. The first commits 305x202 and the element is 305x202 at the drop; 200ms later it renders 395x261, its stylesheet size, while --hf-studio-width still reads 305. The second gesture then starts with `actual` at 305 against a live box of 395, and its very first move — a pointer delta of 0.1px — snaps the element back to 305. That snap is the jump. The gap belongs to the soft reload: it reverts the old timeline before building the new one, and GSAP hands back each tween's recorded starting width on the way out. Nothing held the size in between, because the seek reapply that exists for exactly this stands aside for elements GSAP animates. Standing aside is right for the offset — those channels compose, and applying both doubles the move — and wrong for size, where both channels write width and height so the later write simply wins on the same committed number. It applies now. Only an element mid-edit carries the vars, so nothing else is touched. A test seeks an element whose size GSAP owns after the revert put the stylesheet size back, and fails with the skip restored.
Selecting a rotated element drew upright dashed boxes across its children: the chrome co-rotated with the element and the child outlines did not, so a text layer inside a rotated card got a square outline lying across the rotated glyphs. The chrome already measures an oriented box; the child outlines were still measured axis-aligned. They now use the same oriented measurement and render with the same rotation. An unrotated element measures identically to before, since the oriented rect returns the plain bounding box at angle 0.
4ccfd7e to
d7ca044
Compare
|
Landed in #3146 — same commits, on an unstacked branch. GitHub refuses base retargeting and admin merge on a stacked PR, and the async merge endpoint it does allow does not apply the ruleset bypass, so this could not be merged in place. All content is on main. |
3 participants
What
A resized element keeps its new size across a seek instead of snapping back to its stylesheet size.
Why
Size does not compose the way the position offset does: both channels write width and height, so the later write wins. Standing aside meant nothing held the size while a soft reload reverted the old timeline, since GSAP hands back each tween's recorded starting width. The element sat at its stylesheet size until the new timeline rendered, and the next gesture then started from a box disagreeing with the committed size.
How
The size reapply no longer skips GSAP-sized elements. Only elements on which Studio has committed a size carry the reapply variables. Those variables intentionally remain authoritative across seeks; when the same element also has authored width or height GSAP tweens, the Studio-committed size wins until a later Studio resize changes it.
Test plan