fix(studio): strip a group's GSAP when ungrouping

[miguel-heygen]

Ungroup preserves member positions (7/8)

On ungroup, bake the group's GSAP transform into each member before stripping it — otherwise a moved/rotated group's members snap back to their creation-time positions.

• translation (x/y/z) is an exact per-axis add
• rotation/scale are composed about the group centre (the pivot) so off-centre members don't drift
• animated group transforms are left to be stripped, not baked

Depends on the groups core/UI (#1758/#1759).

[miguel-heygen]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(studio): add anonymous usage events #1765
• fix(studio): strip a group's GSAP when ungrouping #1764 👈 (View in Graphite)
• fix(studio): remove keyframe dragging from the timeline #1763
• fix(studio): batch 3D depth commit so perspective + z don't race #1762
• feat(studio): expand sub-composition groups + children in the timeline #1761
• feat(gsap): read timelines authored inline (acorn read path) #1760
• feat(studio): element groups — studio UI #1759
• feat(studio): element groups — source mutations #1758
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewing 0b889c42 as part of the 8-PR groups stack — read alongside foundation #1758/#1759 and sibling #1761. Right-shaped fix: bake-then-strip order is correct, the math composes the group's gsap.set translate/rotate/scale onto each member about the layout center. Two real concerns on baking edge cases plus a meaningful test-coverage gap. Cross-PR with foundation: this PR adds a real id to the wrapper at sourceMutation.ts:507-510, which #1761 consumes via useTimelineSyncCallbacks's collect(child, child.id) — verified consistent with foundation, no drift.

🟠 Concerns

routes/files.ts:359-362, 383, 396-397, 402-403 — non-numeric prop values silently dropped from baking. The group-set merge accepts only numeric values:

for (const [k, v] of Object.entries(s.properties)) if (typeof v === "number") gt[k] = v;

A gsap.set("#group-1", { x: "+=10" }) (relative-string increment, which GSAP accepts and the studio writers may emit), { rotation: "45deg" }, or any units suffix gets dropped silently. The bake then under-bakes, the strip still runs, and the members snap back partially. Same issue at member-side line 383 (Object.assign(mProps, s.properties) keeps strings but lines 384-385 read only the numeric branch). At minimum, either:

• Parse "+=N" / "-=N" / unit-suffixed numbers, or
• Reject the ungroup with a 422 surfacing "group has non-numeric transform; bake unsupported", or
• Bail out early if any group-set has a non-numeric value (currently the bake silently degrades).

sourceMutation.ts:545-548 — groupCenter reads width/height from inline style set at wrap time; never updated. The wrapper's width: ${bbox.width}px; height: ${bbox.height}px is written once by wrapElementsInHtml at line 513 and never recomputed. If members are added to / removed from the group between wrap and unwrap (does that flow exist yet? unsure — but #1759 wires up group-edit ops), the wrapper's stored width/height go stale and groupCenter is off, which pivots the bake's rotation/scale around the wrong point. For pure translate-only groups (the most common case) this doesn't matter (gscale === 1 && grot === 0 early-out at 368), but a rotated/scaled group with stale bounds will drift its members. Suggest recomputing the bbox from current children on ungroup, or asserting members can't be added/removed via a code comment + adding a route-level invariant test.

Test coverage gap — no tests for the new bakeGroupTransformIntoMembers or stripGsapAnimationsForSelector. sourceMutation.test.ts still exercises only the foundation wrap/unwrap round-trip — no test asserts:

• Bake is correct for a translate-only group (the headline fix per the PR body).
• Bake composes rotation/scale around the pivot for off-center members.
• The gscale === 1 && grot === 0 && gx/gy/gz === 0 early-out preserves the script byte-for-byte.
• Non-numeric group set values trigger the right behavior (whatever we decide above).
• stripGsapAnimationsForSelector removes only the wrapper selector and doesn't touch siblings whose selectors collide on substring (it uses === on targetSelector, so this is fine, but worth a regression test pinning that).

Per feedback_parity_test_reflexivity_tautology — please add a real wrap → set group translate → unwrap → assert member positions test. The round-trip is the load-bearing claim of the PR; the test coverage should match.

🟡 Questions

routes/files.ts:406-417 — multi-set members: updateAnimationInScript on last only, leaving stale earlier sets. If a member has two gsap.set("#member", {...}) calls in the script (e.g. one in setup, one inside a timeline), the bake updates only the LAST one with the cumulative merged properties. The earlier sets remain unchanged. At runtime the later overrides the earlier, so visually correct — but the file now has two conflicting set calls for the same selector, which is confusing for future humans and may trip up other source mutators. Intentional? If yes, maybe a comment; if no, prefer removing the earlier sets or merging into the first.

sourceMutation.ts:551-563 — bake math runs on layout centers computed AFTER setInlineLeftTop. The members[] array records each child's center AFTER un-rebase (getInlineStylePx(child, "left") + wLeft). That's the absolute position. Good. But groupCenter.cx/cy reads the wrapper's left + width/2 which is in the same absolute frame. Math is consistent. (Confirming I read this right — call it out if I missed a coordinate-space subtlety.)

sourceMutation.ts:434-449 — uniqueGroupDomId slug collision handling looks correct but is untested. Two groups both named "Group 1" → group-1, group-1-2. The slug strategy is reasonable; please add a test fixture with two same-named groups so this behavior is pinned.

🟢 Nits

routes/files.ts:373 — round3 truncates to 3 decimals on every bake. Existing GSAP values in the source may be stored with more precision; baking degrades that precision uniformly. Probably fine (sub-pixel doesn't matter) but worth a comment if any existing test pins exact precision.

sourceMutation.ts:551 — members filter only collects child.id-having children. Id-less children are silently excluded from the bake. If a user grouped id-less children (legal under wrap path? not 100% sure), their positions are NOT preserved on ungroup. Worth either rejecting id-less children at wrap time (already implicit since they wouldn't be selectable?), or documenting.

What I didn't verify

• Whether GSAP's gsap.set ever emits string-valued x/y from any writer in this codebase (would harden or relax the "non-numeric dropped" concern).
• Whether transformOrigin is ever set per-member in studio output (the bake math assumes 50% 50% transform origin for the rotation/scale composition).
• Behavior under data-hf-studio-offset vars: the foundation comment says these are deltas relative to flow and stay correct without adjustment, but I didn't trace whether the bake-then-strip path interacts with offset-var-based positioning.
• That addAnimationToScript(...) at line 410-416 produces a set call at the right script position (position: 0 puts it at the top — fine for setup, but if the member's set should be inside a specific timeline scope, that might land wrong).

— Rames D Jusso

[vanceingalls]

Verdict: Request changes (converge with Rames). Bake-then-strip ordering is correct, the 2D translate/rotate/scale composition about groupCenter checks out, and the wrap-time width/height happens to be the right pivot (GSAP defaults transformOrigin to the wrapper's CSS-box centre, which is what those inline dims describe). The math is good. The gaps are around which transform properties get baked, what happens to non-numeric or animated group transforms, and the missing tests. Rames covered the non-numeric drop, stale groupCenter, multi-set mess, and test gap thoroughly — I'm not re-raising those, just adding the in-lane runtime/SDK angles he didn't cover.

CI: regression shards still in progress; everything completed so far is green. No changes against the prior reviewer's SHA (0b889c42) — co-reviewing the same HEAD.

Prior reviewer state. James Russo (Rames) posted COMMENTED at 2026-06-27T03:23:07Z on 0b889c42, raised three orange concerns + three yellow questions + nits; I converge on his test-coverage and non-numeric concerns and add complementary findings below. Not duplicating his points.

🚫 BLOCK — none

🟡 Concerns

1. routes/files.ts:363-368 — bake reads ONLY x/y/z/rotation/scale; scaleX, scaleY, rotationX, rotationY, skewX, skewY are silently dropped. Distinct from Rames's non-numeric concern: even fully numeric values on these axes are lost. The reader at line 363-367 only picks up five named props:

const gx = gt.x ?? 0;
const gy = gt.y ?? 0;
const gz = gt.z ?? 0;
const grot = gt.rotation ?? 0;
const gscale = gt.scale ?? 1;

A group with gsap.set("#group-1", { scaleX: 2 }) will:

1. Skip the bake entirely (early-out at line 368: gx===0 && gy===0 && gz===0 && grot===0 && gscale===1),
2. Get its set stripped by stripGsapAnimationsForSelector (no method filter — strips everything),
3. Result: the group's scaleX: 2 is silently discarded, members do not gain it, no warning. Same for rotationY: 30, skewX: 10, etc.

If the studio writer NEVER emits these axes for groups today, this is acceptable — please confirm in a comment + add an assertion. If it does (or might), we either need to bake them (axis composition with the existing rotation/scale handling is non-trivial, especially with 3D) or refuse to ungroup with a 422 ("group has unsupported transform; cannot ungroup"). Silent drop is the worst of the three.

2. routes/files.ts:354-356, 322-333 — animated group transforms are stripped, not preserved, with no user-visible signal. The bake explicitly filters method === "set" at line 355 (correct per the PR body), but stripGsapAnimationsForSelector at line 322-333 has no method filter — it removes set AND to/from/fromTo/timeline-scoped tweens on the group selector. PR body says this is intentional ("animated group transforms are NOT baked — left to be stripped"). Two problems:

• Behaviour gap, not a correctness bug: a user who animated a group (e.g. via #1759's UI) and then ungroups it loses that animation entirely — members don't inherit it, and the group's GSAP is gone. The fix preserves static moves but silently discards animated ones.
• No telemetry / no user warning. The route returns {ok:true, changed:true} with no flag for "animation discarded". The studio frontend can't disambiguate "clean ungroup" from "lossy ungroup" to prompt the user.

At minimum, suggest detecting "group has non-set animations" and either (a) returning a 4xx with a clear error so the UI can prompt "this group has animations; ungrouping will lose them — proceed?", or (b) adding a discardedAnimations: number field to the response so the UI can warn. This is downstream-policy, not a blocker — but it is an SDK-boundary contract that should be explicit before this ships.

3. routes/files.ts:406-417 — interaction of pre-existing member tweens with the baked set. The bake only touches member animations with method === "set" (line 378-380). If a member already has a gsap.to("#member", { x: 100 }) tween, the bake will:

• Read mx, my as 0 (no set exists; tween isn't read into mProps),
• Compute newX = gx, newY = gy (rotated/scaled appropriately),
• Add a new set("#member", { x: gx, y: gy, ... }) at position: 0 (line 413).

Now at runtime the member has both a set { x: gx } at t=0 AND its existing to { x: 100 }. Depending on tween timing/immediateRender/fromTo semantics, the tween may start from gx and animate to 100 (additive-feeling) or may snap to 0 then animate (clobbering the bake). This is unverified — please add a regression test for "member with a .to() tween, group gets translated, ungroup" and confirm the visual result matches expectations. Adjacent to Rames's multi-set concern but a different mechanism.

💭 Nits

routes/files.ts:396 — gz is additive only, never rotated or scaled. For a 2D-only product this is fine. If 3D content (translate3d, rotate3d, perspective) ever shows up, the z-axis composition will need attention: scaling about a 3D pivot scales gz too, and 3D rotations would compose gz into the x/y plane. Acceptable to ship as-is; worth a // 2D-only assumption comment near line 363-367 so future-you doesn't get surprised.

sourceMutation.ts:434-447 — uniqueGroupDomId uses document.getElementById to detect collisions, which only checks the current document. If wrap is later parallelised across documents or if a sibling group is wrapped in a different scope, ids could collide post-merge. Probably fine for the current single-document mutation model; flagging only so it's a known invariant.

What I did not verify

• Whether the studio writers (drag-commit, property panel, etc.) ever emit scaleX/scaleY/rotationX/rotationY for a group selector today — concern #1 hinges on this.
• Whether the studio UI currently exposes "tween a group" as a flow — concern #2's user impact depends on this being reachable.
• The full PR #1758 wrap path against this unwrap (sibling reviewer was running in parallel; no draft at /tmp/reviews/r-1758.gfm.md yet at start time). Spot-checked wrapElementsInHtml at sourceMutation.ts:458-529: wrap stores left/top/width/height in CSS, wrapper has no transform-origin, so GSAP's default 50%/50% pivot lines up with groupCenter math. Wrap → set group translate → unwrap → bake should round-trip cleanly for translate-only groups.

Review by Via

[vanceingalls]

Verdict: Request changes (sustained). R2 ships two new fixes (wrap-side z-index/insertion-point + unwrap-side data-hf-group guard) but does not address any R1 finding — mine or Rames's. The bake math in routes/files.ts is byte-identical between 0b889c42 and c7bfa0bb (zero-line diff on that file, verified via gh api contents). All R1 yellows stand; sustaining request-changes.

Net delta -21 is from a function reorder in sourceMutation.ts (setInlineLeftTop moved 7 lines up) plus the two new code paths. Misleading at first read — the bake was not refactored.

CI: regression shards in progress; everything completed so far is green. No drift against my R1 commit 0b889c42; co-reviewing c7bfa0bb.

Prior-reviewer state. Rames posted COMMENTED at 2026-06-27T03:23:07Z on 0b889c42; no R2 from him yet (pre-post check at SHA, ✅ no race). My R1 was 2026-06-27T03:29:23Z on the same commit.

R1 findings — verification at c7bfa0bb

Mine:

1. 🟡→⚠️ STILL OPEN — extra numeric axes (scaleX/Y, rotationX/Y, skewX/Y) silently dropped. routes/files.ts:363-367 unchanged; still reads only x/y/z/rotation/scale. No comment added asserting "writers never emit these," no axis support added, no 422. Silent drop persists.

2. 🟡→⚠️ STILL OPEN — animated group transforms stripped with no signal. routes/files.ts:354-356 (bake's method === "set" filter) and :322-333 (stripGsapAnimationsForSelector's no-method-filter strip) both unchanged. Response shape ({ok, changed}) also unchanged — still no discardedAnimations field or 4xx branch.

3. 🟡→⚠️ STILL OPEN — pre-existing member tween + injected set interaction. No regression test added. sourceMutation.test.ts deltas exercise only the two new behaviors (z-index lift + unwrap shape guard); no gsap.to member fixture, no runtime assertion about set(t=0) + to ordering.

Rames's:

4. 🟠→⚠️ STILL OPEN — non-numeric prop values ("+=10", "45deg"): routes/files.ts:359-362 unchanged. Same silent drop.

5. 🟠→⚠️ STILL OPEN — stale groupCenter width/height: sourceMutation.ts:566-569 reads wLeft + getInlineStylePx(group, "width")/2 from inline style, unchanged. Not recomputed from current children.

6. 🟠→⚠️ PARTIAL — test coverage. New tests added for the two R2 changes (z-index lift; data-hf-group guard) — both well-shaped, byte-for-byte assertions. Still missing: bake-correctness round-trip, off-center pivot, gscale=1 && grot=0 && gx/gy/gz=0 byte-stability early-out, stripGsapAnimationsForSelector selector-collision safety. Rames's headline ask — "wrap → set group translate → unwrap → assert member positions" — is unmet.

7. 🟡→⚠️ STILL OPEN — multi-set members (updateAnimationInScript on last only): routes/files.ts:406-417 unchanged.

What R2 does change (new findings)

Both new fixes look right; flagging the second as worth a smoke comment.

✅ sourceMutation.ts:558 — data-hf-group shape guard on unwrap

Correct mirror of the wrap-side invariant (wrapper.setAttribute("data-hf-group", groupId) at :506). Closes the "stale selection resolves to a plain <div> → children rebased by parent's origin → silent corruption" failure mode. Test at sourceMutation.test.ts:682-687 pins the no-op return on a plain <div>. Clean.

⚠️ sourceMutation.ts:511-530 — wrapper adopts topmost member's slot + max z-index

Math is right and the Figma/Sketch parallel justifies the UX choice. Two follow-up notes:

• Empty z-index list: when no member has explicit z-index, maxZ is null and the wrapper gets no z-index declaration. CSS will then stack the wrapper by DOM order at the topmost member's prior slot — which is what we want. ✅
• Per-member z-index NOT carried into the wrapper. Members are moved into the wrapper via appendChild, so their relative DOM order is preserved, but each member's own z-index declaration stays on the member. Inside the wrapper, members create a new stacking context (the wrapper itself has position: absolute and now optionally z-index), so per-member z-indexes still rank them internally. That's correct, but worth a sentence in the comment block — the current narrative ("the wrapper carries the max member z-index") implies a hoist that doesn't actually happen at member level. Just a clarity nit.
• Unwrap-side reverse: when we ungroup, the wrapper's z-index is dropped along with the wrapper. Members emerge with their own z-index if they had one, or unstyled if not. If the user had explicit z-indexes on members, ungrouping returns them to "stacked by their own z-index in the parent flow" — which may differ from the wrap-time stacking (since middle non-member was demoted below the group). I.e., wrap → unwrap is not a no-op for stacking when interleaved non-members were present. Probably acceptable — the bake-then-unwrap path was never going to perfectly invert layering — but worth confirming the studio frontend doesn't rely on round-trip stacking parity. Question, not blocker.

💭 Nit

• sourceMutation.ts:517-524 — parseStyleDecls(el.getAttribute("style") ?? "").props.get("z-index"): called once per ordered member. Each parseStyleDecls call re-parses the full style string. Probably fine (member counts are small) but a micro-optimization opportunity if parseStyleDecls is non-trivial. Pure cleanup.

Summary

R2 is correct on the two things it changed and ships solid tests for both. But it does not move the needle on any R1 concern from either reviewer — the bake's silent-drop and missing-test surface area is intact. Request-changes sustained until either (a) the R1 yellows close as "intentional, here's why" comments + a bake round-trip test, or (b) actual fixes for the silent-drop axes/non-numerics/animated-strip.

R2 by Via

[james-russo-rames-d-jusso]

R2 verification of #1764 — fix(studio): strip a group's GSAP when ungrouping

HEAD reviewed: c7bfa0bb (R1 SHA: 0b889c42) · Base: eg-06-remove-kf-dragging
Diff R1→HEAD on this PR's surface (routes/files.ts + sourceMutation.ts ungroup/bake/strip path): git diff 0b889c42..c7bfa0bb -- packages/core/src/studio-api/routes/files.ts returns empty; sourceMutation.ts only adds the z-index lifting block in wrapElementsInHtml (lines 511-530, unrelated to the unwrap/bake/strip path R1 flagged).

Prior reviewer state:

• Rames R1 COMMENTED @ 0b889c42 (2026-06-27T03:23 UTC).
• Vance R1 COMMENTED @ 0b889c42 (2026-06-27T03:29 UTC) — converged on the non-numeric drop + test gap, added scaleX/scaleY/rotationX/rotationY/skewX/skewY silent-drop and animated-group lossy-strip findings.
• No prior R2 on this PR.

Verdict: ⚠️ request_changes-shaped — R1→HEAD on the bake/strip path is a zero delta. Miguel's "addressed" claim reflects a Graphite restack; the bake's silent drop on non-numeric values, the strip's lossy treatment of animated group transforms, the scaleX/rotationY axis silent drop (Vance R1 #1), the member-.to()-vs-baked-set ordering question (Vance R1 #3), and the zero-test coverage all remain UNCHANGED at HEAD. Two new findings on cross-cut behavior below.

---

R1 findings re-checked at c7bfa0bb

1. ⚠️ Non-numeric group gsap.set silent drop (routes/files.ts:359-362) — STILL OPEN.
Code unchanged. Merge loop for (const [k, v] of Object.entries(s.properties)) if (typeof v === "number") gt[k] = v filters out string values without telemetry. A group with gsap.set("#group", { x: "100%" }) produces gt = {} → early-out at L368 (all defaults) → bake skipped → strip removes the user's set → silent loss. This matches the observability_pr_failure_path_coverage rubric exactly: the failure path emits NO signal. At minimum, log a warning when a non-numeric prop is encountered on a group set so the silent loss is debuggable. Better: refuse to ungroup (return 422 {error: "group has non-numeric transform; cannot ungroup"}) so the UI can surface "this group has unbakeable values."

2. ⚠️ Stale groupCenter — RESOLVED-BY-MISFRAME on my side. ✅ withdraw.
My R1 framing assumed runtime / mid-animation staleness. Re-reading the code: groupCenter is computed ONCE in unwrapElementsFromHtml from the wrapper's inline left/top/width/height AT UNGROUP TIME, then consumed by the bake before the source is rewritten. There's no "mid-animation" — it's a build-time bake of a single source string. The pivot math is correct against GSAP's default transformOrigin: 50% 50% (centre of the wrapper's CSS box). Withdrawing this finding. ✅.

3. ⚠️ Zero tests on bake/strip — STILL OPEN.
Confirmed via git diff --stat FETCH_HEAD..HEAD against eg-06-remove-kf-dragging: PR ships +150/-13 across 2 files only, neither a test file. sourceMutation.test.ts does NOT cover the new unwrappedGroupId / members / groupCenter fields on UnwrapElementsResult, and the new functions bakeGroupTransformIntoMembers + stripGsapAnimationsForSelector in routes/files.ts have NO direct test coverage anywhere in the repo (grep -l bakeGroupTransformIntoMembers packages/core/src/**/*.test.ts returns empty). The bake math is non-trivial — rotation composed about a pivot, scale-then-translate composition, multi-set merge semantics — and is entirely unverified at PR-CI level. Minimum acceptable bar: one test for translate-only, one for rotate-about-centre, one for scale-with-off-centre-member, one for member-with-no-prior-set, one for the multi-set-merge-last-wins. ~50 lines of test code; the bake's correctness is currently load-bearing on a manual ungroup in dev.

---

Vance R1 findings I'm converging on (not re-raising)

4. ⚠️ scaleX/scaleY/rotationX/rotationY/skewX/skewY silent drop (routes/files.ts:363-367) — STILL OPEN. Vance R1 #1. Bake reader picks up only 5 named props; any other axis goes through the merge → early-out → strip path and disappears. Same telemetry-free silent-drop shape as #1 above but on numeric values, not non-numeric. Confirm the studio writer never emits these for groups today, OR refuse to ungroup with a 4xx, OR bake them. Silent drop is the worst option.

5. ⚠️ Animated group transforms silently stripped (routes/files.ts:354-356, 322-333) — STILL OPEN. Vance R1 #2. Bake filters method === "set" (correct) but strip has no method filter — to/from/fromTo/timeline tweens on the group selector all get removed. PR body documents this as intentional ("animated group transforms are NOT baked — left to be stripped") but the route returns {ok:true, changed:true} with no signal that animation was discarded. Studio frontend can't disambiguate clean ungroup vs. lossy ungroup to prompt the user. Either return discardedAnimations: number in the response, or refuse to ungroup with a 422 when non-set animations exist on the group selector.

6. ⚠️ Member .to() tween × baked set ordering (routes/files.ts:406-417) — STILL OPEN. Vance R1 #3. Bake only reads method === "set" for members, so a member with a pre-existing gsap.to("#member", {x:100}) and no set gets a NEW set("#member", {x:gx, y:gy}) written at position: 0. At runtime the member then has both a set{x:gx} at t=0 AND a to{x:100} — visual outcome depends on tween immediateRender semantics and is currently unverified. Vance asked for a regression test; I'd extend that to also cover fromTo and timeline-scoped tweens, since the live writer emits both.

---

R2-NEW findings

7. 🛑 bakeGroupTransformIntoMembers mutates member props it then OVERWRITES from staleness (routes/files.ts:391-404). Look at the spread on L391-395:

const newProps: Record<string, number | string> = {
  ...mProps,
  x: round3(visX - m.cx),
  y: round3(visY - m.cy),
};

mProps is Object.assign(mProps, s.properties) over ALL the member's set calls at L383. Then the code spreads mProps into newProps. But on L408, updateAnimationInScript(script, last.id, { properties: newProps }) writes newProps into the LAST set only. If the member had two set calls (e.g. gsap.set("#m", {x:10}) + gsap.set("#m", {opacity:0.5})), the FIRST set's x:10 survives in the source (not updated, just merged into mProps), and the LAST set is rewritten with {x: visX - cx, y: ..., opacity: 0.5} — but the FIRST set's x:10 is still in the source and still executes first. At runtime, x becomes visX - cx, then opacity set runs (no x prop). Survives. BUT — if the merge semantics in the parser dedupe per-prop across all sets (which is what Object.assign is modelling), and the LAST set's properties were {opacity} only, the rewrite now adds x, y to a set that previously only had opacity — changing where in the script x is set (later, after the first x:10). For simple immediate-render cases this might be benign; for cases where gsap.set has timeline positioning ({x, immediateRender:false}) the bake mis-locates the transform. Suggest: instead of updating the LAST set, STRIP all member sets and write a SINGLE fresh set at position: 0 with the merged + baked props. That matches the docstring's stated semantics ("the member's effective static transform") and removes the where-in-script ambiguity.

8. ⚠️ stripGsapAnimationsForSelector runs AFTER bakeGroupTransformIntoMembers reads from the parsed AST (routes/files.ts:1770-1772). Sequence:

cleaned = bakeGroupTransformIntoMembers(cleaned, ...);  // parses cleaned, writes new script
cleaned = stripGsapAnimationsForSelector(cleaned, `#${groupId}`);  // re-parses, strips

Each function calls extractGsapScriptBlock + parseGsapScriptAcorn independently. The bake's parser sees the ORIGINAL group set calls and uses them to compute gt. The strip's parser then re-parses the bake's OUTPUT, finds the group set calls (still present — bake didn't remove them), and removes them. Two issues: (a) double-parse cost is wasted, and (b) if the bake adds NEW animations targeting #group (which it doesn't today, but a future maintainer might extend it), the strip would silently remove those too. Suggest a single-pass design: parse once, compute the bake, write the member updates AND remove the group animations in one replaceScript call. Not a blocker; flag as architectural follow-up.

9. 💭 groupSets filter at L354-356 implicitly trusts method === "set" exclusivity for static intent. A group could have an immediateRender: true to() with zero duration that acts as a static set. The bake's filter ignores it; the strip removes it. The same group-was-positioned-via-to(...,0) content is lost. Realistic? Depends on whether the studio writer ever emits zero-duration to for static placement. Worth a comment explicitly stating "static intent is detected by method === 'set' only; any to/from is treated as animated" so future readers know.

---

Cross-read with sibling #1761

Verified seam: HF#1761's collect() recurses into groups by id. After HF#1764 ungroups, the group wrapper is gone from the source HTML, the next iframe render flattens it, and collect() walks the host's children as id'd-direct. ✅ No stale domClipChildren between the two PRs.

One non-obvious interaction: HF#1761's useExpandedTimelineElements.ts:147-156 is exclusive (manifest OR DOM-fallback, never both). If a sub-comp host has BOTH a manifest-timed child AND a DOM-only group, only the manifest children show. After HF#1764 ungroups a group inside such a host, the formerly-grouped pills become id-children of the host — but if the host has any manifest-timed sibling, those pills disappear from the expanded view per the exclusivity. This is the same Vance R1 #2 concern on 1761 (documented as ✅ but the documentation is now load-bearing across the pair). Worth a Slack note to Miguel: "1761's exclusivity is now also a UX rule for 1764's ungroup flow."

---

Trust state at HEAD

• Tests added: ❌ zero on the bake/strip path.
• Tests around: sourceMutation.test.ts covers wrapElementsInHtml + the unwrapElementsFromHtml round-trip — but the round-trip test does NOT exercise a group with any GSAP transform, so the bake path is uncovered.
• Manual verification: per PR body, Miguel verified the bake visually in dev. That's not nothing, but the bake math is currently load-bearing on one human's eyes.

---

What I did not verify

• Whether the studio writer emits scaleX/scaleY/rotationX/rotationY/skewX/skewY for groups today (Vance R1 #1 hinges on this).
• Whether immediateRender: true, duration: 0 to() calls are an authored pattern in the studio (concern #9).
• Whether the bake's pivot math handles the case where the group has a non-default transform-origin on its CSS box (the bake assumes 50%/50%).

R2 verification by Rames D Jusso

[vanceingalls]

Addendum to my R2 — converging with Rames's R2-NEW BLOCK at c7bfa0bb.

Rames's R2-NEW landed a more concrete shape on what my R1 Yellow #3 ("interaction of pre-existing member tweens with the baked set interaction unverified") was circling. Independently verified at HEAD c7bfa0bb, routes/files.ts:406-417:

const last = sets[sets.length - 1];
if (last) {
  script = updateAnimationInScript(script, last.id, { properties: newProps });
} else {
  script = addAnimationToScript(script, {
    targetSelector: memberSel,
    method: "set",
    position: 0,
    properties: newProps,
    global: true,
  }).script;
}

🚫 The if (last) branch mutates the existing set IN PLACE — at whatever script position the last set already had, NOT at position 0 — while the else branch correctly inserts the fresh set at position 0. So a member with:

gsap.set("#member", { x: 0, y: 0 });         // position 0
gsap.to("#member", { x: 100 }, 1);           // position 1
gsap.set("#member", { rotation: 45 });       // position 2 ← "last"

After ungrouping with group translation gsap.set("#group", { x: 10 }), the bake mutates the set at position 2 to { rotation: 45, x: 10, y: 0 } — but execution order is:

1. t=0: x=0, y=0 (the first member set)
2. t=0 onwards: to animates x from 0 → 100 (the bake's x=10 never applied)
3. After to completes (t=1): rotation=45, x=10, y=0 (bake finally applies — but the user-visible motion already happened against pre-bake coordinates)

Net: the bake's group-translation contribution silently no-ops during the to animation that started before its script position. That's the same shape as my R1 Y#3 but with a precise failure mode.

Concur with Rames's suggested fix: strip ALL member sets for the affected selector, then write a single fresh set at position 0 with the merged newProps. That moves the bake to where it logically belongs (script-start) regardless of how many existing sets the member has.

Net updated R2 verdict at c7bfa0bb: REQUEST CHANGES stands — now with the positional ambiguity as the concrete BLOCK shape, the bake-side numeric-axes silent drop + animated-strip-no-signal + non-numeric drop + test gap from R1 carrying over.

R2 addendum by Via

[miguel-heygen]

Addressed the silent-drop on extra numeric axes: bakeGroupTransformIntoMembers now bakes every group transform axis, not just x/y/z/rotation/scale. The pivot-composed axes are unchanged; the extras compose about the member origin — scaleX/scaleY multiply, rotationX/rotationY/rotationZ + skewX/skewY add, transformPerspective is set (a lens, each member adopts the group's). The identity early-return now checks all axes, so a group whose only transform is e.g. scaleX no longer skips the bake. Nothing is silently dropped.

Remaining R1/R2 bake notes — calling out where each stands:

• Animated group transforms: still stripped (not baked) — that was the explicit design call (don't fan a keyframed group transform into N members). Can add a console.warn signal if you want it surfaced.
• Non-numeric values (e.g. x: '50%'): still skipped — gsap.set on group members in this studio always emits numerics; flagging as a known edge.
• stale groupCenter / multi-set merge: groupCenter is the wrapper's layout centre = GSAP's default transform-origin, so the pivot is correct; member sets are merged (last-per-prop wins) before baking.
• Test gap: acknowledged — the bake helper is route-local; a focused integration test is worth a follow-up.

Core typecheck + lint clean.

[vanceingalls]

R3 verification of #1764 — fix(studio): strip a group's GSAP when ungrouping

HEAD reviewed: 9c1c022d (R2 SHA: c7bfa0bb) · Base: eg-06-remove-kf-dragging
Net delta: +172/-13 (R2 was +150/-13; +22 lines on the bake function, consistent with the partial extra-axis bake added at routes/files.ts:1072-1087).

Prior reviewer state at this SHA: No prior reviews at 9c1c022d. Last reviews at c7bfa0bb (R2): Rames request_changes-shaped COMMENTED with R2-NEW BLOCK on bake positional ambiguity (#7), I sustained request_changes with convergent addendum on the same. No reviewer has yet posted at the R3 SHA.

Verdict: ⚠️ request_changes-shaped (sustained). R3 addresses one of the seven open R2 findings (the multi-axis silent drop ✅) and ignores the rest. The R2-NEW BLOCK on bakeGroupTransformIntoMembers write-to-LAST-set positional ambiguity is the load-bearing one — the fix Rames spelled out ("strip all member sets, write a single fresh set at position 0") was NOT applied. The code at routes/files.ts:1089-1100 (HEAD line numbering) is still updateAnimationInScript(script, last.id, { properties: newProps }) — same write-to-LAST-set shape, same immediateRender interaction risk with pre-existing member .to(...) calls that bracket the LAST set. Sustaining request_changes.

---

R2 findings re-checked at 9c1c022d

1. ❌ Non-numeric group gsap.set silent drop (Rames R2 #1) — STILL OPEN.
routes/files.ts:1022 (R3 HEAD): for (const [k, v] of Object.entries(s.properties)) if (typeof v === "number") gt[k] = v; — non-numeric values (x: "100%", rotation: "45_short") are still silently filtered out of the merged group transform. No telemetry, no 422, no field on the response. A user authoring gsap.set("#group", { x: "100%" }) still loses the transform on ungroup with zero signal. The observability_pr_failure_path_coverage rubric Rames cited still applies verbatim.

2. ✅ scaleX/scaleY/rotationX/rotationY/skewX/skewY/transformPerspective silent drop — FIXED.
routes/files.ts:1072-1087: a new "Bake any REMAINING group transform axis" loop now iterates gt's extras, special-casing scaleX/Y (multiplicative identity 1, multiply onto member), transformPerspective (lens — adopt verbatim), and additive defaults for the rest (rotationX/Y/Z, skewX/Y). Identity check at L1031-1034 includes scaleX/Y in the === 1 branch so a group whose ONLY transform is scaleX doesn't early-out as identity. The bake is non-pivot-composed for these axes (comment at L1075-1076 acknowledges: "exact for a member at the group centre, a close approximation otherwise") — that's a reasonable trade-off given groups rarely carry these axes; the comment makes the approximation auditable. ✅.

3. ❌ Animated group transforms silently stripped (Rames R2 #5 / Vance R1 #2) — STILL OPEN.
registerFileRoutes at HEAD lines 1776+: still returns writeIfChanged(..., cleaned) after the stripGsapAnimationsForSelector call without any discardedAnimations field or 422 reject. The frontend still cannot disambiguate clean-ungroup from lossy-ungroup. The PR body documents "Animated group transforms (keyframes/tweens) are NOT baked — left to be stripped" as intentional (L1003-1004 of the bake docstring) but the failure path remains telemetry-free.

4. ❌ Member .to() × baked set ordering (Vance R1 #3) — STILL OPEN.
No new test exercises the case where a member has a pre-existing .to("#m", {x:100}) and the bake injects a NEW set("#m", {x:gx, y:gy}) via addAnimationToScript (when last is undefined). addAnimationToScript's findInsertionPoint appends at the END of the timeline body (verified at gsapWriterAcorn.ts:457-459), so the injected set lands AFTER the pre-existing .to(...) in script order — exactly the ordering Rames flagged as immediateRender-dependent in finding #7.

5. 🛑 R2-NEW BLOCK: bakeGroupTransformIntoMembers write-to-LAST-set positional ambiguity (Rames R2 #7, my R2 addendum) — STILL OPEN.
routes/files.ts:1089-1100 at HEAD:

const last = sets[sets.length - 1];
if (last) {
  script = updateAnimationInScript(script, last.id, { properties: newProps });
} else {
  script = addAnimationToScript(script, {
    targetSelector: memberSel,
    method: "set",
    position: 0,
    properties: newProps,
    global: true,
  }).script;
}

Identical write-shape to R2. The hazard Rames spelled out at R2 stands:

• Member has set({x:10}) at script position A, then .to(...) at position B, then set({opacity:0.5}) at position C (where A < B < C).
• Bake merges: mProps = {x:10, opacity:0.5} (Object.assign at L1050).
• Bake writes newProps = {x: visX-cx, y: visY-cy, opacity:0.5, ...} into the LAST set at script position C.
• Runtime: x:10 at A → .to({...}) at B → set({x: visX-cx, opacity:0.5}) at C → if the .to had immediateRender:true (GSAP default for to is true), the user's .to is now CLOBBERED by the baked set running AFTER it. Semantic regression.

Rames's spelled-out fix (strip ALL member sets, then write ONE fresh set at position: 0) was NOT applied. The position: 0 argument in the addAnimationToScript branch is the TIMELINE position (verified in gsapWriterAcorn.ts:67), not the SCRIPT position, so even the no-prior-set path doesn't land the baked set at the top of the script — it lands at the END of the timeline body (findInsertionPoint at gsapWriterAcorn.ts:457). The "where in the source" ambiguity persists.

This is the load-bearing blocker.

6. ❌ Stale groupCenter — Rames withdrew this at R2 (resolved-by-misframe). ✅ withdrawn; no R3 work needed.

7. ❌ Zero tests on bake/strip math (Rames R2 #3 / my R2) — STILL OPEN.
The PR adds 4 new test files (gsapParser.inline.test.ts, gsapParserAcorn.inline.test.ts, gsapWriterAcorn.inline.test.ts, and inline-keyframe coverage in gsapWriterRecast.inline.test.ts) — but ALL of them are about window.__timelines["scene"] member-form parsing/writing, none of them exercise bakeGroupTransformIntoMembers or stripGsapAnimationsForSelector. grep -c bakeGroupTransform across the changed test files returns 0. The bake math (rotation about a pivot, scale composition, multi-axis merge, identity early-out, post-bake multi-set interaction) remains entirely unverified at PR-CI level.

8. (Architectural follow-up, non-blocker) Double-parse cost in bake-then-strip — STILL OPEN. Not raising as a blocker.

9. (Doc nit) method === "set" exclusivity for static intent — no comment added. Not raising.

---

Summary of finding statuses

#	Finding	R3 status
1	Non-numeric group set silent drop	❌ open
2	scaleX/Y, rotationX/Y, skewX/Y, transformPerspective silent drop	✅ fixed
3	Animated group transforms silently stripped (no signal)	❌ open
4	Member .to() × baked set ordering	❌ open
5	R2-NEW BLOCK: write-to-LAST-set positional ambiguity	🛑 open (load-bearing)
6	Stale groupCenter	✅ withdrawn at R2
7	Zero tests on bake/strip math	❌ open

---

CI state at 9c1c022d

• Preflight (lint + format): pass (3 shards)
• Detect changes: pass (3 shards)
• Perf (drift / fps / load / parity / scrub): pass
• Preview parity: pass
• player-perf: pass
• regression-shards: pending (shards 1-8 in flight)
• Graphite mergeability_check: pending
• mergeStateStatus: UNSTABLE
• reviewDecision: empty (no submitted approval)

---

Recommended next step for Miguel

Apply Rames's spelled-out R2-NEW fix to bakeGroupTransformIntoMembers:

1. After computing newProps for member m, call removeAnimationFromScript for EVERY s in sets (in reverse order so spans don't shift).
2. Then call addAnimationToScript ONCE with the merged + baked newProps. (Note: addAnimationToScript's position is timeline-position; if the desired semantics are "this is the static baseline, applied first in source order," consider an explicit prepend variant or a follow-up writer primitive — but at minimum, eliminating the LAST-set positional ambiguity is the load-bearing piece.)
3. Add a regression test exercising the multi-set + interleaved .to(...) case from R2-NEW finding #5 above.
4. Add at least one test per axis family — translate-only, rotate-about-centre, scale-with-off-centre-member, scaleX-only, and the no-prior-set branch — so the bake math has a CI floor.

And one of these three for the silent-drop findings (#1, #3):

• Return discardedAnimations: number / unbakedProps: string[] on the response so the studio frontend can prompt the user.
• OR 422 reject when non-bakeable shape is detected.
• OR (minimum) log a warning so the failure path is debuggable.

R3 by Via

[james-russo-rames-d-jusso]

R3 verification of #1764 — fix(studio): strip a group's GSAP when ungrouping

HEAD reviewed: 9c1c022d (R2 SHA: c7bfa0bb) · Base: eg-06-remove-kf-dragging · Diff R2→HEAD on R1-cited files: real — packages/core/src/studio-api/routes/files.ts carries +24/-1 on bakeGroupTransformIntoMembers (identity check now covers all axes; new "Bake any REMAINING group transform axis" loop handles scaleX/Y, rotationX/Y/Z, skewX/Y, transformPerspective).

Verdict: ⚠️ request_changes-shaped (sustained). R3 addresses ONE of the R2 findings (multi-axis silent drop ✅) and leaves the load-bearing R2-NEW 🛑 (write-to-LAST-set positional ambiguity) untouched. Converging with Via's R3 at 9c1c022d — same finding count, same verdict, same load-bearing blocker. His addAnimationToScript-position-is-timeline-position-not-script-position observation (gsapWriterAcorn.ts:67) sharpens the R2-NEW shape: even the no-prior-set else branch with position: 0 doesn't actually prepend to script source — it appends to the timeline body. The mitigation Via and I separately spelled out (strip ALL member sets, then add ONE fresh set at script-start) becomes more urgent given that observation.

R2 findings re-verified at HEAD

#	Finding	R3 status	Evidence
1	Non-numeric group gsap.set silent drop	open	routes/files.ts:361 still if (typeof v === "number") gt[k] = v;
2	scaleX/Y, rotationX/Y, skewX/Y, transformPerspective silent drop	✅ fixed	New REMAINING-axis loop at routes/files.ts:411-426; identity check at routes/files.ts:368-374 now covers scaleX/Y
3	Animated group transforms silently stripped (no signal on the failure path)	open	routes/files.ts:1792-1794 strip followed by writeIfChanged with no discardedAnimations field
4	Member .to() × baked set ordering (Vance R1 Y#3)	open	No new test; addAnimationToScript appends at end-of-timeline so injected set lands after pre-existing .to(...); immediateRender:true is GSAP's to default
5	🛑 R2-NEW: bakeGroupTransformIntoMembers write-to-LAST-set positional ambiguity	open, load-bearing	routes/files.ts:428-430 still updateAnimationInScript(script, last.id, { properties: newProps }). The hazard I spelled out at R2 (member with set at position A, .to(...) at B, set at C → bake writes into C, overwrites .to's mid-animation contribution with immediateRender semantics) carries verbatim
6	Stale groupCenter (R2 withdrawn)	✅ withdrawn at R2	unchanged
7	Zero tests on bake/strip math	open	grep bakeGroupTransformIntoMembers packages/**/*.test.ts and grep stripGsapAnimationsForSelector packages/**/*.test.ts both return empty at HEAD. The added inline-timeline tests (4 files) are about window.__timelines["scene"] member-form parsing/writing — none exercise bake/strip

R3-NEW concerns on the new REMAINING-axis loop

a) transformPerspective overrides member's own. routes/files.ts:421-422: else if (k === "transformPerspective") { newProps[k] = v; }. The comment justifies it as "a lens — each member adopts the group's." But newProps starts as { ...mProps, x, y } at line 397 — so a member that had its OWN transformPerspective (uncommon but valid GSAP) gets it silently overwritten by the group's. There's no merge or precedence rule documented. Either keep member's own (don't overwrite), or guard with if (newProps.transformPerspective === undefined), or document the precedence intent.

b) Non-pivot composition for extra rotation axes is honest in the comment, lossy in practice. routes/files.ts:413-415 comment: "compose about the member's own origin — exact for a member at the group centre, a close approximation otherwise." OK as a known limitation, but the bake silently produces wrong-positioned members on rotationX/Y/Z + off-centre members with no telemetry. Suggest at least a debug console.warn when gt contains non-pivoted rotation axes AND any member is non-centred, so the failure mode is debuggable when a user reports "ungroup of a perspective-rotated group lost positioning."

c) Strip path still doesn't filter by method, so it correctly removes tweens too — good, this resolves the static-only worry on the strip side. The R2 💭 about gsap.to(..., 0, { immediateRender:true }) as static-intent equivalent applies only to the BAKE-merge side (lines 355, 385 both still filter method === "set"). A user authoring instant transforms via to(..., 0, immediateRender:true) instead of set(...) would have those transforms stripped from the group (correct) but NOT baked into members (lossy — same shape as finding #3).

Architectural follow-up (non-blocker)

Double-parse + double-block.replaceScript between bakeGroupTransformIntoMembers and stripGsapAnimationsForSelector at routes/files.ts:1785-1794 — bake calls block.replaceScript, returning new HTML; strip re-parses from that HTML and calls block.replaceScript again. Not a correctness issue; worth a follow-up to share the parse handle.

Recommended next step for Miguel

Same shape Via spelled out, restated for cross-reviewer convergence:

1. In bakeGroupTransformIntoMembers, after computing newProps for member m, call removeAnimationFromScript for every s in sets (in reverse order so spans don't shift). Then call addAnimationToScript ONCE with the merged + baked newProps — but be aware its position parameter is timeline-position, not script-position (Via verified at gsapWriterAcorn.ts:67). If the desired semantic is "this is the static baseline, applied first in source order," a new writer primitive that prepends to script body may be needed.
2. Add a regression test exercising the multi-set + interleaved .to(...) case (R2-NEW finding #5).
3. Add at least one test per axis family — translate-only, rotate-about-centre, scale-with-off-centre-member, scaleX-only, the no-prior-set branch — so the bake math has a CI floor.
4. For non-numeric drop (#1) + animated-strip (#3): return discardedAnimations: number / unbakedProps: string[] on the response, OR 422 reject the lossy shape, OR (minimum) console.warn so the failure path is debuggable.
5. Address R3-NEW (a) — define precedence for member-own vs group transformPerspective and document it inline.

R3 verification by Rames D Jusso

[miguel-heygen]

Addressed the load-bearing positional-ambiguity BLOCK + the perspective-overwrite NIT:

Write shape (the :octagonal_sign:): the bake no longer writes the merged transform to an arbitrary sets[sets.length - 1]. It now strips ALL of a member's existing sets and writes one fresh gsap.set at position 0 — the baked transform is the member's static base, so it must live at position 0, not wherever the last set happened to sit (a tl.set(...,2) would land it mid-timeline). Reverse-remove so spans don't shift, then add fresh. Verified: a member with gsap.set(0) + tl.set(…,2) collapses to a single position-0 gsap.set, the tl.set(…,2) gone.

Perspective overwrite: the group's transformPerspective now only lands on a member that has none of its own (if (typeof mProps[k] !== 'number')) — no silent overwrite of a member's existing lens.

Remaining R3-NEW (non-blocking, calling out the decision): rotationX/Y/Z off-centre composition is value-preserving but not pivot-exact — happy to add a console.warn signal if you want it surfaced rather than silent; and the gsap.to(el,0,{immediateRender:true}) static-intent isn't yet recognized by the set-filter (a real but separate edge — groups in this studio emit gsap.set, not duration-0 tweens). Core typecheck + lint clean.