feat(sdk): stage 6 — arc path ops (setArcPath, updateArcSegment, removeArcPath)#1500
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Summary of intent. Stage 6 of the SDK port: adds three GSAP arc-path ops — setArcPath, updateArcSegment, removeArcPath — to both the EditOp union and the acorn writer. Hoists three new helpers (extractArcWaypoints, buildMotionPathObjectCode, numericXY) from the arc-path domain into gsapSerialize.ts so both writers (recast already had setArcPathInScript at gsapParser.ts:2155; acorn now has the parallel) share waypoint extraction and motion-path serialization.
PR-template flag. +449/-4 across 6 files, boilerplate body. The arc-path domain is non-obvious to anyone who hasn't worked through the existing recast implementation — a sentence on what "arc path" means here (a motionPath plugin object that replaces flat x/y with a curve) and which user gesture triggers it would save a real chunk of archeology time. Soft ask.
GSAP multi-layer regime contract audit — and the band-aid finding.
- Generator layer (
gsapSerialize.ts): addsextractArcWaypoints,buildMotionPathObjectCode,cubicControlPoints,buildCubicPathEntries. Both recast (gsapParser.ts) and acorn (gsapWriterAcorn.ts) writers now route through these. ✓ shared-helper discipline matches #1499's pattern. - Parser layer (
gsapParser.ts:773-845, 973): no parser changes in this PR — the existingarcPathextraction inparseGsapScriptis unchanged. Both writers consumeanimation.arcPathfrom the same parser. ✓ - Writer layer — here's where I want to flag a silent scope gap. The parity harness at
gsapWriter.parity.test.ts:140-178adds three describe blocks —setArcPathInScript: acorn output correctness,updateArcSegmentInScript: acorn output correctness,removeArcPathFromScript: acorn output correctness. They only test the acorn side. The recast writer'ssetArcPathInScriptatgsapParser.ts:2155,updateArcSegmentInScriptatgsapParser.ts:2244, andremoveArcPathFromScriptatgsapParser.ts:2281are not imported into the parity test, and no assertion compares recast and acorn outputs.
The file's own header (line 1-9) reads: "Parity harness — recast writer (gsapParser.ts) vs acorn writer (gsapWriterAcorn.ts). Both must produce scripts that REPARSE to the same animation model... each ported op gets a fixture row here proving it matches the battle-tested original." The arc-path block silently breaks that discipline. It's labelled acorn output correctness, not parity. The describe headings change just enough to telegraph the regime drift but not enough to surface it in CI.
This is band-aid pattern #3 (scope gap that undercuts the PR's stated coverage). #1499 establishes a parity-harness regime; #1500 lands an op family that opts out of it. If the recast and acorn writers diverge on cubic control-point synthesis, the spline curviness defaults, or autoRotate serialization, the existing test suite will not catch it.
Recommended fix: import setArcPathInScript, updateArcSegmentInScript, removeArcPathFromScript from gsapParser.js as setArcRecast / etc., and add three describe("parity: ...") blocks that mirror the existing keyframe-ops style — both writers run, both outputs reparsed, expect(acornShape).toEqual(recastShape). The fixture matrix can stay small (the three you already have, just doubled).
Under Miguel's band-aid bar this is request-changes territory because the parity discipline is the entire point of the WS-3 migration arc. Cheap to fix — but please don't leave the harness with this gap.
Dispatch chain audit.
EditOpaddssetArcPath(with nestedconfig: { enabled, autoRotate, segments }),updateArcSegment,removeArcPath. (types.ts:137-160)mutate.tsaddsapplyArcPathOp(lines 191-215) called fromapplyGsapOpafterapplyGsapKeyframeOp. Three switch arms. ✓validateOp(mutate.ts:1061-1063) — three new cases added to the existing "requires GSAP script" group. ✓- No new Studio cutover helpers in this PR; the SDK ops are exposed but the Studio panel that drives them isn't wired here. Confirm in the next stage that the panel exists and dispatches via
comp.dispatch({ type: "setArcPath", ... })rather than re-routing through server commit. If the panel falls back to server, the cutover is unfinished.
Per-file findings.
packages/core/src/parsers/gsapSerialize.ts:18-30(extractArcWaypoints): returns[{ x:0, y:0 }, { x:destX, y:destY }]when there aren't 2+ keyframe waypoints butproperties.xorproperties.yis numeric. Edge: a tween with onlyproperties.y = 50(no x) gets waypoints[{0,0}, {0,50}]— a vertical arc. That may be intentional (interpret missing x as 0) but the auto-injection of an origin waypoint at{0,0}is silent. Worth a one-line comment ("origin waypoint synthesized when only a single endpoint is known"), and confirm the arc-path UX produces a sane preview for the missing-axis case.packages/core/src/parsers/gsapSerialize.ts:40-55(cubicControlPoints): thecp1/cp2overrides win when both are set, otherwise the function synthesizes control points usingc * Math.abs(dx) * 0.25. The factor0.25and the0.33/0.66interpolation positions are not in any shared constant. If recast has a different curve shape on the synthesis path, every cubic motion-path will reparse to a subtly different curve. Cheap detection:grep '0.33\|0.66\|0.25' packages/core/src/parsers/gsapParser.tsand confirm recast does the same arithmetic. If recast doesn't have a synthesis branch (only the explicitcp1/cp2path), then this is a regression hazard the missing parity test should catch.packages/core/src/parsers/gsapWriterAcorn.ts:213-227(removePropsByKey): theblockRemoveRangelogic groups consecutive marked-for-removal props into blocks to avoid overlapping MagicString ranges. The block boundaries[blockStart-1.end, blockEnd-1.end]correctly include the trailing comma when an internal block ends, and[0.start, [last].end]removes the whole tail when every prop is marked. Edge: an isolated last-prop removal (single prop at end of object) falls into theblockStart-1.end → blockEnd-1.endbranch — does that include the trailing comma? If the object is{ a: 1, b: 2 }andbis removed, the range is[1.end, 2.end]which is, b: 2— correct. ✓packages/core/src/parsers/gsapWriterAcorn.ts:267-275(stripXYFromKeyframes): stripsxandyfrom each percentage-keyframe when arc-path is enabled. Correct contract (arc-path drives x/y via motionPath; standalone x/y would dual-write). Edge: if the script's keyframes usexPercent/yPercentinstead ofx/y(GSAP supports both), they survive the strip and silently coexist with the motionPath. Probably not a common shape, but worth a regression check.packages/core/src/parsers/gsapWriterAcorn.ts:286-289(segments fallback): whenconfig.segments.length !== waypoints.length - 1, segments are synthesized as[{ curviness: 1 }, ...]. That silently drops user-customized segments on a mismatch — e.g. if the user had 3 segments for 4 waypoints and a keyframe is added making 5 waypoints, the user's curvature edits are lost. Worth a console-warn or askippedSegments: numberreturn field so the panel can surface the loss.packages/sdk/src/engine/mutate.ts:198-201(segments curviness default):config.segments.map((seg) => ({ ...seg, curviness: seg.curviness ?? 1 })). This is the right defaulting only for thesetArcPathop;updateArcSegmentpasses its update straight through without the default. Asymmetric. If a panel sends{ update: { cp1: ..., cp2: ... } }without curviness on a segment that previously hadcurviness: undefined, the segment's curviness stays unset and the cubic-vs-curviness branch atbuildMotionPathObjectCode(line 80) treats segments missingcp1/cp2as the curviness path with?? 1. Probably fine downstream because the writer defaults again, but worth aligning.
CI. regression and regression-shards shard-5 FAILURE on head, six other shards CANCELLED. Same pattern as #1499 — likely the cancellations are downstream of the first failure. With arc-path being a writer-surface change, the shard-5 failure is suspicious. Please confirm the shard list before merging.
Verdict. Hard request-changes on the parity-harness gap — please import the recast arc-path writers and add three real parity describe blocks. Comment-tier on the rest (segments-drop, control-point factors, single-axis arc, asymmetric curviness default). The shared-helper discipline at the serializer is genuinely the right move; the regression is letting the harness's purpose silently lapse for this op family.
Cleared SHA: a298b6a0. Any push past that voids this review.
Review by Via
53ba244 to
f48279a
Compare
a298b6a to
0207a93
Compare
f48279a to
2eb7516
Compare
0bf4569 to
f9ced28
Compare
ccd350d to
64553a4
Compare
49af212 to
1ba678f
Compare
7804909 to
a29a0c6
Compare
1ba678f to
262854c
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
PR Review — feat(sdk): stage 6 — arc path ops
Vance, this is a strong, well-structured addition. The extraction of buildMotionPathObjectCode and extractArcWaypoints into gsapSerialize.ts as shared helpers is the right call — having the recast and acorn writers produce identical arc configs from a single source of truth eliminates an entire class of drift bugs. The MagicString surgery in gsapWriterAcorn.ts is careful and the test coverage hits the important round-trip paths.
That said, I found a few things worth discussing:
1. blockRemoveRange comma/whitespace residue (potential bug)
function blockRemoveRange(allProps: any[], blockStart: number, blockEnd: number): [number, number] {
if (blockStart === 0 && blockEnd === allProps.length)
return [allProps[0].start, allProps[allProps.length - 1].end];
if (blockStart === 0) return [allProps[0].start, allProps[blockEnd].start];
return [allProps[blockStart - 1].end, allProps[blockEnd - 1].end];
}
When a middle block of properties is removed (not first, not last), the range is [prevProp.end, lastMarkedProp.end]. This eats the comma before the block but leaves the comma after the last marked prop (the one between the removed block and the next surviving prop). Depending on how MagicString renders the result, you could end up with { duration: 2,, ease: "power2" } or a leading comma. The existing removeProp helper in this file handles the "am I last / first / middle" dance — removePropsByKey should mirror that logic for the block boundaries, or at minimum the range should extend to the start of the next surviving prop (eating its leading comma/whitespace) rather than the end of the last removed prop.
Worth a quick manual test: remove x and y from { x: 100, y: 50, duration: 2 } and inspect the output string. If removePropsByKey is only called via enableArcPath (where upsertProp("motionPath", ...) runs first), the property order might save you — but the function is general-purpose and will bite in other orderings.
2. extractArcWaypoints silent fallback to {x: 0, y: 0} origin
return [
{ x: 0, y: 0 },
{ x: destX, y: destY },
];
When there aren't enough keyframe waypoints, this assumes the element starts at (0, 0). That's the GSAP-relative default, but if the element has a CSS transform or an initial x/y set via fromProperties, the arc origin will be wrong. The anim.fromProperties bag is right there — worth checking it for x/y before defaulting to zero. Not a blocker (the current behavior matches what GSAP would do with a bare tl.to(...) from implicit origin), but it's a latent incorrectness that'll surface when someone wonders why their arc doesn't start where the element visually is.
3. Type mismatch: ArcPathSegment.curviness is required in the interface, optional in EditOp
In gsapSerialize.ts:
export interface ArcPathSegment {
curviness: number; // required
In types.ts (EditOp's setArcPath config):
segments: Array<{
curviness?: number; // optional
This works at runtime because applyArcPathOp does seg.curviness ?? 1, but the type contract is lying — callers of the EditOp think curviness is optional, but ArcPathSegment says it's required. Either make ArcPathSegment.curviness optional (with the ?? 1 default documented), or make the EditOp config reference ArcPathSegment directly and require callers to pass it. The inline anonymous type in EditOp is already a smell — it duplicates ArcPathSegment with different optionality. Same story for the update field on updateArcSegment.
4. cubicControlPoints hardcoded magic numbers
return [
`{x: ${wp.x + dx * 0.33}, y: ${wp.y + dy * 0.33 - c * Math.abs(dx) * 0.25}}`,
`{x: ${wp.x + dx * 0.66}, y: ${wp.y + dy * 0.66 - c * Math.abs(dx) * 0.25}}`,
];
The 0.33, 0.66, 0.25 are reasonable cubic Bezier defaults, but they're magic. A named constant or a brief comment explaining the heuristic ("approximate a smooth arc by placing control points at 1/3 and 2/3 of the chord, offset perpendicular by curviness * 25% of chord width") would help the next person who touches this. Also, Math.abs(dx) * 0.25 means the perpendicular offset is always in the negative y direction — the arc always bows upward. Is that intentional? If curviness can be negative (nothing in the type prevents it), you get a downward bow, which is fine. But if it's always positive, arcs going left-to-right and right-to-left will both bow upward, which might not be what the user expects for certain path directions. Worth a comment on the design intent.
5. PR description mentions retiring keyframeIndex variant of removeGsapKeyframe — but the diff doesn't touch it
The summary says "Retires the keyframeIndex variant of removeGsapKeyframe; only percentage-based removal remains" but I see no changes to removeGsapKeyframe in the diff. Either this landed in a prior commit on the stack or the description is stale. Flagging so it doesn't cause confusion during stack review.
6. Deleted tests: sub-composition and addGsapTween empty-return tests
The diff removes ~80 lines of tests from mutate.gsap.test.ts — the freshSubComp helper, the sub-composition selector tests, the setTiming on comp-root test, and the removeElement cascade test. These look like they were relocated or superseded by the no-script-block tests, but the new test file doesn't cover the sub-composition selector behavior. If those tests moved to another file in this stack, great — but if they're just gone, that's a regression in test coverage for a pretty important edge case (composition-id → canonical hf-id selector resolution).
7. Behavioral change: addGsapTween / setGsapTween / removeGsapTween now throw instead of returning EMPTY
This is the right direction (silent no-ops are worse than clear errors), but it's a breaking behavioral change for any caller that was relying on EMPTY returns to detect "no script block." The validateOp path already returns E_NO_GSAP_SCRIPT, so callers using can() before apply() are fine — but any caller that was doing try-apply-check-forward-length will now need a try/catch. Worth calling out in the PR description or a migration note if this is a public SDK surface.
Minor nits
fallow-ignore-next-line complexity— typo? Should this bebiome-ignoreoreslint-disable? If it's a custom lint pragma, ignore me.readLastWaypointXYreturns{ x: number | null; y: number | null }— consider usingundefinedfor consistency with the rest of the codebase's "absent value" convention (I seeundefinedused elsewhere in the parser types).
Overall this is solid work — the architecture of shared serialization helpers is clean, the test coverage hits the important paths, and the MagicString manipulations are well-contained. The blockRemoveRange comma handling (#1) is the only thing I'd want verified before merge; the rest are improvement suggestions.
LGTM — pinging @magi for the stamp. <@U0B1J4SL8H3>
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Independent pass, layered on Via + Miga.
Verdict. Request-changes — co-signing Via's parity-harness-gap blocker, with independent verification.
The blocker (verified). Via's headline finding is exactly right and the evidence is plain. I cross-checked:
-
The recast arc-path writers exist at
gsapParser.ts:2216(setArcPathInScript),gsapParser.ts:2305(updateArcSegmentInScript),gsapParser.ts:2342(removeArcPathFromScript). They've been in the codebase. They're importable. -
The parity harness at
gsapWriter.parity.test.ts:1-9declares its purpose as "Both must produce scripts that REPARSE to the same animation model... each ported op gets a fixture row here proving it matches the battle-tested original." -
This PR's new test blocks at
gsapWriter.parity.test.ts:418-462only importsetArcAcorn,updateArcSegmentAcorn,removeArcAcorn. The describe headings deliberately read"setArcPathInScript: acorn output correctness"rather than"parity: setArcPathInScript (recast vs acorn)"— the keyframe-ops pattern from #1499. No fixture compares recast and acorn outputs.
The discipline #1499 established (every ported op gets a parity row) is silently broken for the arc-path op family. If recast and acorn diverge on cubic control-point synthesis, curviness-vs-cp1/cp2 dispatch in buildMotionPathObjectCode, or extractArcWaypoints origin-fallback ({0,0} vs computed start), the existing test suite cannot detect it.
Fix shape (cheap): Import setArcPathInScript as setArcRecast, etc., from ./gsapParser.js. Rename the three describes to "parity: setArcPathInScript (recast vs acorn)" matching the keyframe-ops style. For each test, run both writers, reparse both, expect(acornShape).toEqual(recastShape). Same fixture matrix you already have, doubled.
Independent finding — PR-body / diff mismatch ([[feedback_author_claim_diff_mismatch]]). PR body claims "Retires the keyframeIndex variant of removeGsapKeyframe; only percentage-based removal remains." Verified at 262854c: packages/sdk/src/types.ts:104-105 still has both variants of removeGsapKeyframe, the dispatcher at mutate.ts:148-154 still discriminates via "percentage" in op, and handleRemoveGsapKeyframe + resolveKeyframe (mutate.ts:903-913, 999-1014) are still present. Miga also flagged this. The retirement isn't in this PR or in #1498 — please either (a) actually land the retirement here per the PR body, or (b) fix the PR body so the next archeologist isn't misled.
Independent finding — silent behavioral change at handleAddGsapTween/handleSetGsapTween/handleRemoveGsapTween/handleAddGsapKeyframe (mutate.ts:715, 748, 788, 959). The diff changes if (!script) return EMPTY; to throw new Error("No GSAP script block found in the composition."); for four handlers. This is a public-API behavioral change for SDK consumers. Miga flagged this; I want to amplify two things:
-
The PR body and title don't mention this. A consumer reading the title ("stage 6 — arc path ops") has zero signal that base-op error semantics changed underneath them.
-
The change is correct IMO (silent no-op on missing GSAP block was hiding bugs), but the asymmetry is now worse:
handleRemoveGsapPropertyandhandleDeleteAllForSelector(atmutate.ts:725, 753) and the new arc-path handler still returnEMPTYon missing script. So you have a mix ofreturn EMPTYandthrowon the same precondition across sibling handlers. Make the policy uniform (throw everywhere with a clear typed error class, or rely onvalidateOpreturningE_NO_GSAP_SCRIPTand keepapply()silent) — but please don't ship both shapes side-by-side.
Independent finding — ArcPathSegment.curviness type contradiction (verified Miga #3). gsapSerialize.ts:18 declares curviness: number (required). types.ts:148 declares the EditOp's setArcPath.config.segments[].curviness?: number (optional). The runtime defaults via ?? 1 at mutate.ts:198-201, but only for the setArcPath op. updateArcSegment's update field passes through untouched, so a panel sending { update: { cp1: ..., cp2: ... } } (no curviness) leaves the segment's curviness wherever it was. Reconcile by either (a) making ArcPathSegment.curviness optional and documenting the writer-side default in buildMotionPathObjectCode, or (b) making the EditOp's segments-array type reference ArcPathSegment directly. The current "required in serializer, optional in EditOp" shape is the kind of contract drift that produces "works in tests, wrong in prod" bugs.
Independent nit — cubicControlPoints asymmetric bow (verified Miga #4 angle). The c * Math.abs(dx) * 0.25 perpendicular offset is always in the negative-y direction. Concur with Miga: if curviness is intended to be always-positive, you get arcs that always bow upward regardless of direction, which is fine for "up and over" but inverted for "down and under." If curviness is signed, negative values flip the bow. The type doesn't constrain sign. A one-line "negative curviness flips bow direction" comment would close the door.
On Via's extractArcWaypoints finding (gsapSerialize.ts:18-30). Concur. The [{x:0,y:0}, {x:destX,y:destY}] fallback silently injects an origin waypoint. For a tween with only properties.y = 50, you get [{0,0}, {0,50}] — a vertical arc. May be intentional; should be documented.
CI. player-perf failing on 262854c (5s — short failure suggests a setup/test-discovery issue, not a real perf regression). Worth confirming before merge.
— Rames D Jusso
(Reviewed at 262854ce3d45a8b83d94c6b5667e8e7a5c10be45. Part of 18-PR HF SDK cutover stack, Group D review pass.)
vanceingalls
left a comment
There was a problem hiding this comment.
Second pass at HEAD 262854ce, after rebase from prior cleared SHA a298b6a0. Co-reviewers (Miga, Rames) have landed independent passes at this SHA; Miga raised 7 substantive findings and Rames co-signed my prior parity-gap blocker. Reading the stack at final state (per [[graphite-stack-review-final-state]]), I find the parity-gap blocker is resolved at the stack tip — see below.
Verdict. Clear with minor nits — ready for stamp once #1498's union-shape blocker is addressed (#1498's fix is the hard prereq; this PR's specific findings are all comment-tier).
On the prior parity-gap blocker — withdrawn. My 06-16 review flagged a hard request-changes finding: the arc-path trio shipped setArcPathInScript: acorn output correctness, updateArcSegmentInScript: acorn output correctness, removeArcPathFromScript: acorn output correctness describe blocks at gsapWriter.parity.test.ts — acorn-only correctness assertions, no recast comparison. Under the per-PR view that is exactly the silent scope-gap pattern (band-aid #3) that two PRs prior (#1499) had eliminated by establishing the parity: <op> (recast vs acorn) discipline with expect(acornShape).toEqual(recastShape).
Stack-tip rule applies (per [[graphite-stack-review-final-state]]): for a Graphite stack that merges atomically, the verification artifact is the final state the merge produces on main, not the per-PR diff. I re-checked the parity test file at #1501's head (db16b977) and the gap is closed. #1501 adds five new describe blocks that DO carry real recast-vs-acorn parity:
parity: arc path trio (recast vs acorn)atgsapWriter.parity.test.ts:592, withmodelOf(acornOut).toEqual(modelOf(recastOut))at lines 605-606, 615-619, 628-629.parity: unrollDynamicAnimations (recast vs acorn)at line 695, equality at 710.parity: addKeyframeToScript (recast vs acorn)at line 752, equality at 764.parity: removeKeyframeFromScript (recast vs acorn)at line 846, equality at 850-851.parity: addAnimationWithKeyframesToScript (recast vs acorn)at line 899, equality at 907 / 918.- Plus shift/scale parity at lines 929 and 968.
All real equality assertions via modelOf() — the same differential approach #1499 used. The "acorn output correctness" describes at this PR (#1500) remain as acorn-specific output assertions, which is fine — they complement, not substitute for, the cross-writer parity.
Per [[feedback_verify_full_dispatch_chain]] and [[project_gsap_loader_validator_contract]]: the parity-discipline regime contract is intact at the stack tip. The per-PR view at #1500 looked like a band-aid; the final-state view at #1501 shows the discipline restored. That is what the stack-merge math produces on main. So this PR is no longer the blocker — the resolution lives one PR up.
Findings worth banking at #1500 specifically (all comment-tier, none blocking).
- Concur with Miga's
blockRemoveRangecomma-residue finding (real bug). I traced this atgsapWriterAcorn.ts:1338-1344. The middle-block branch returns[allProps[blockStart - 1].end, allProps[blockEnd - 1].end]. For an object{a, x, y, duration}removing the middle block (indices 1,2): blockStart=1, blockEnd=3. Branch returns[a.end, y.end]. This eats, x: 100, y: 50— but does not eat the comma betweenyandduration. Result:{a duration}(no comma). This survives in practice today becauseremovePropsByKeyis only invoked atdisableArcPath(viaremovePropsByKey(ms, call.varsArg, new Set(["x", "y"]))) where the surviving property order typically placesmotionPathfirst viaupsertProp, so the removedx/yblock is last, hitting theblockEnd === allProps.lengthbranch which is correct. Cheap fix: mirror theremoveProp"am I first / middle / last?" logic for the block boundaries. Otherwise the next caller ofremovePropsByKeywith a different property order will trip the bug. - Concur with Miga + Rames on the
ArcPathSegment.curvinesstype contradiction. AtgsapSerialize.ts:96the interface declarescurviness: number(required). Attypes.ts:144the EditOp'ssetArcPath.config.segments[].curviness?: numberis optional. The runtime defaults via?? 1atmutate.ts:199but only for thesetArcPathop —updateArcSegment'supdatefield atmutate.ts:207passes through toupdateArcSegmentInScriptuntouched. A panel sendingupdateArcSegment({ update: { cp1, cp2 } })(no curviness) leaves the segment's curviness wherever it was. Reconcile by either (a) makingArcPathSegment.curvinessoptional with the writer-side?? 1default documented, or (b) making the EditOp's segments-array type referenceArcPathSegmentdirectly. The current "required in serializer, optional in EditOp" shape is contract drift that produces works-in-tests-wrong-in-prod bugs. - Concur with Miga on
extractArcWaypoints{x:0, y:0}fallback origin. When there aren't enough keyframe waypoints, the function assumes the element starts at(0, 0). That matches GSAP-relative default, but if the element has a CSS transform or an initialx/yset viafromProperties, the arc origin is wrong. Theanim.fromPropertiesbag is right there — worth checking before defaulting to zero. Not a blocker — current behavior matches what GSAP would do with a baretl.to(...)from implicit origin — but it's a latent incorrectness that'll surface when someone wonders why their arc doesn't start where the element visually is. - Concur with Miga + Rames on
cubicControlPointsasymmetric bow. Thec * Math.abs(dx) * 0.25perpendicular offset is always in the negative-y direction — arcs always bow upward. Ifcurvinessis intended to be signed, negative values flip the bow; the type doesn't constrain sign. A one-line "negative curviness flips bow direction" comment plus named constants for0.33/0.66/0.25would close the door. - Concur with Miga + Rames on the silent
apply()error-shape asymmetry. This PR changeshandleAddGsapTween,handleSetGsapTween,handleRemoveGsapTween, andhandleAddGsapKeyframefromif (!script) return EMPTYtothrow new Error("No GSAP script block found ..."). Correct direction — silent no-op was hiding bugs. But the asymmetry is now worse:handleRemoveGsapProperty,handleDeleteAllForSelector, and the new arc-path handlers still returnEMPTYon missing script. Pick one policy (throw everywhere with a typed error class, or rely onvalidateOpreturningE_NO_GSAP_SCRIPTand keepapply()silent) — don't ship both shapes side-by-side. Also: the PR title doesn't mention this behavioral change. A migration note would help any SDK consumer that was relying onEMPTYreturns to detect "no script block." - Concur with Miga on the deleted sub-composition tests. The diff removes ~80 lines from
mutate.gsap.test.ts—freshSubComp, the sub-composition selector tests, thesetTimingon comp-root test, theremoveElementcascade test. If they moved elsewhere in this stack, great — but if they're gone, that's a regression in test coverage for composition-id → canonical hf-id selector resolution. Worth confirming where the coverage moved to. - PR-body / diff mismatch (real concern). The PR body claims "Retires the keyframeIndex variant of removeGsapKeyframe; only percentage-based removal remains." Verified at
262854c:types.ts:104-105still has both variants; the dispatcher atmutate.ts:148-154still discriminates via"percentage" in op;handleRemoveGsapKeyframe+resolveKeyframeatmutate.ts:903-913, 999-1014are still present. The retirement is not in this PR. Either land it here (the blocker on #1498) or fix the body so the next archaeologist isn't misled. Three reviewers (Miga, Rames, me) have flagged this — please don't leave the description as-is.
On the applyArcPathOp factor itself. The split out of applyGsapOp is clean and follows #1499's applyGsapKeyframeOp pattern. Adding a third arm at #1501 (unrollDynamicAnimations) by routing through applyArcPathOp is semantically awkward (Miga flagged this for #1501; concur) — unrollDynamicAnimations has zero conceptual relationship to arc paths. Recommend renaming handleArcPathScript to handleScriptRewrite (it's already generic — just setGsapScript + gsapScriptChange) and routing unroll through its own applyUnrollOp or directly via applyGsapOp. That belongs in #1501 though, not here.
Dispatch chain (re-traced at current head). Every new arc-path op has a handler in applyArcPathOp, a switch arm in the writer (gsapWriterAcorn.ts), and a validator arm in validateOp at mutate.ts:1107-1109 (lumped as one case with addGsapKeyframe / removeAllKeyframes etc — which is fine but worth noting). Studio dispatch lives via the cutover helpers in useArcPathOps.ts. ✓
CI. player-perf shows FAILURE because Perf: parity was CANCELLED at 17:53:50Z. Pulled the parity job log — the bun process was killed during orphan-process cleanup ("Terminate orphan process: pid (2312) (bun)"); the previous shard already had "no files were found with the provided path" for results, suggesting the shard never finished its run. Infra flake — this PR is 7 TypeScript files in packages/{core,sdk,studio}, does not touch player perf surface. Re-run will clear it.
Cleared at 262854ce with the seven nits above. Re-verify if HEAD moves before stamp.
Review by Via
|
Review findings addressed in #1539 (commit
|
miguel-heygen
left a comment
There was a problem hiding this comment.
Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.
jrusso1020
left a comment
There was a problem hiding this comment.
Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.
a29a0c6 to
78e6a05
Compare
The base branch was changed.
…veArcPath) Port arc path trio from recast to browser-safe acorn+MagicString writer. Add SDK op types and mutate.ts handlers for setArcPath / updateArcSegment / removeArcPath. Decompose buildMotionPathObjectCode into small sub-functions in gsapSerialize.ts to stay within fallow complexity thresholds. Tests verify acorn output re-parses to correct arcPath shape. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
262854c to
dc9e25f
Compare
The §3.2 sdkTimingPersist rewrite regressed the non-SDK fallback path vs the pre-cutover behavior. Restored, on both fallback entry points (no-session and sdkTimingPersist-returned-unhandled): - Resize live DOM patch dropped the conditional data-playback-start/media-start attr — restored so a start-trim updates the preview's in-point immediately. - Move/resize fallback dropped the GSAP-position sync (shift/scaleGsapPositions) + reloadPreview — restored so server-path edits keep GSAP tweens in sync and refresh the preview (the SDK path folds both into setTiming). - Undo-coalesce drift: fallback enqueueEdit carried no coalesceKey while the SDK branch did — plumbed coalesceKey through persistTimelineEdit so undo granularity is identical on either path. - Documented the hasPbsAdjustment second clause + sdkTimingPersist before-capture transition limitation. Flag-off (dark launch) so this lands as one fix PR at the stack tip rather than restacking the mid-stack §3.2 commit. #1500 review items: parity-harness gap already closed at the tip (arc/unroll recast-vs-acorn parity added); blockRemoveRange flagged 'potential' but verified correct (no comma residue on any block position). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…) (#1539) * fix(studio): restore timeline move/resize fallback parity (review #1466) The §3.2 sdkTimingPersist rewrite regressed the non-SDK fallback path vs the pre-cutover behavior. Restored, on both fallback entry points (no-session and sdkTimingPersist-returned-unhandled): - Resize live DOM patch dropped the conditional data-playback-start/media-start attr — restored so a start-trim updates the preview's in-point immediately. - Move/resize fallback dropped the GSAP-position sync (shift/scaleGsapPositions) + reloadPreview — restored so server-path edits keep GSAP tweens in sync and refresh the preview (the SDK path folds both into setTiming). - Undo-coalesce drift: fallback enqueueEdit carried no coalesceKey while the SDK branch did — plumbed coalesceKey through persistTimelineEdit so undo granularity is identical on either path. - Documented the hasPbsAdjustment second clause + sdkTimingPersist before-capture transition limitation. Flag-off (dark launch) so this lands as one fix PR at the stack tip rather than restacking the mid-stack §3.2 commit. #1500 review items: parity-harness gap already closed at the tip (arc/unroll recast-vs-acorn parity added); blockRemoveRange flagged 'potential' but verified correct (no comma residue on any block position). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sdk): retire duplicate removeGsapKeyframe keyframeIndex variant (review #1498) EditOp had two removeGsapKeyframe members with the same discriminant but different shapes (keyframeIndex vs percentage) — TS can't discriminate them and a handler could get the wrong shape. Per both reviewers (option 2): retire the keyframeIndex variant. It had no production caller (Studio dispatches percentage only); removed the dead by-index handleRemoveGsapKeyframe + simplified the dispatcher. resolveKeyframe stays (setGsapKeyframe still uses keyframeIndex). Converted the one by-index test to the percentage API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(studio): gate ALL cutover persist paths on the flag — true dark launch (review #1469 finding #6) Only sdkCutoverPersist (style/text/attr) checked STUDIO_SDK_CUTOVER_ENABLED. sdkTimingPersist, dispatchGsapOpAndPersist (every GSAP op) and sdkDeletePersist guarded only on `!sdkSession` — and useSdkSession opens a session by default for shadow/selection, so timing/GSAP/keyframe/delete cutover was ALWAYS live regardless of the flag. Flipping the flag OFF could not disable it, so the data-loss bugs in those paths (single-prop wipe, wrong-keyframe match, tween collapse, arc strip) ship LIVE on merge instead of being dark-launched. Added the flag guard at all three chokepoints → flag OFF returns false → callers fall back to the legacy server path. Makes the stack genuinely dark-launchable: merge is now a no-op in prod, and the remaining cutover correctness bugs become flip-prerequisites rather than merge-blockers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core,sdk): correct 8 GSAP write-path review findings (#1539) Eight correctness bugs from the SDK-cutover review. Several were cases where BOTH writers were identically wrong, so the recast-vs-acorn parity suite stayed green; the new tests assert the real-world-correct result, not agreement. - #2 findKfPropByPct: match the CLOSEST keyframe within tolerance, not the first within 2% — removing/updating 50% on 0/49/50/100 no longer hits 49%. - #3 handleSetTiming: shift each tween by the start DELTA and scale duration by the clip-duration RATIO per-tween, instead of writing absolute newStart/ newDuration onto every tween (which collapsed staggers and blew durations). - #4 enableArcPath: insert motionPath via appendRight at the object start so the insertion can't collide with the x/y remove-range end (which made MagicString discard the append and emit '{}'). - #5 splitAnimationsInScript: compute the inherited baseline in a forward pre-pass so the split-spanning midpoint sees earlier tweens (the reverse write loop is kept for stable count-suffixed ids). - #9 unrollDynamicAnimations: preserve non-target loop-body statements (e.g. tl.set initial-state) per iteration instead of overwriting the whole loop. - #10 buildMotionPathObjectCode (both writers): emit the cubic form when segment curviness varies so per-segment curviness survives, not just segments[0]. - #11 readLastWaypointXY: handle UnaryExpression so negative destination coords are recovered when disabling an arc path. - #15 no-bang: removed every `!` non-null assertion in the touched files, replaced with guards/fallbacks. Tests: gsapWriter.reviewFixes.test.ts (#2/#4/#5/#9/#10/#11) and mutate.gsap.test.ts setTiming GSAP-sync block (#3). All fail on the base and pass after the fix; tsc + full core/sdk suites + parity stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(studio): SDK cutover review fixes — merge tween props, stabilize debounce, serialize gsap writes, on-disk undo baseline, self-write identity Addresses 5 SDK-cutover review findings (studio-only): - #1 useGsapPropertyDebounce: editing one GSAP tween property no longer drops the tween's other animated props. setGsapTween REPLACES the property set, so merge the single edit into the tween's CURRENT properties (read from the SDK doc) before dispatching, mirroring the legacy server merge. - #7 useGsapPropertyDebounce: stabilize the flush callback by reading sdk deps from a ref instead of an unmemoized literal, so a parent re-render mid-edit no longer tears down + flushes the debounce (one commit/undo entry per render). - #8 sdkCutover/useGsapScriptCommits: route SDK gsap-write persists through the same per-file keyed serializer the legacy commitMutation uses, so concurrent same-file read-modify-writes can't interleave and lose an edit. - #12 sdkCutover/useTimelineEditing: capture the exact on-disk bytes as the undo 'before' for timing/GSAP persists (matching the style/delete paths) instead of a normalized SDK serialize() re-emit that reformatted the whole file on undo. - #14 useSdkSession/sdkSelfWriteRegistry: discriminate a cutover echo from an undo write by CONTENT identity (registered self-write hash), not just the 2 s timestamp window — an undo write always reloads the SDK session. Tests: useGsapPropertyDebounce(.test), useGsapPropertyDebounceFlush.test, sdkSelfWriteRegistry.test, and new sdkCutover.test cases; each reproduces the review scenario and asserts the corrected behavior (verified red before fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): extract split/collapse helpers to satisfy no-fallow-ignore rule The #5 (split) and #15 (no-bang guards) fixes pushed splitAnimationsInScript and removeAllKeyframesFromScript over fallow's complexity threshold, and a fallow-ignore had been added to splitAnimationsInScript. Per the hard rule (never ignore — fix), extracted buildSpanningSplit + applyTweenSplit (split) and buildCollapsedFlatVars (collapse), and removed the ignore. Both functions now under threshold; fallow new-only gate reports 0 new findings. Behavior unchanged — core 1811 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(studio): pin dark-launch flag-gate contract (review #1539, Rames/Via) flag OFF ⇒ sdkTimingPersist / sdkGsapTweenPersist (GSAP-op chokepoint) / sdkDeletePersist all return false even with a valid session → legacy fallback. The prod flag-flip rests on this contract; sdkCutover.test.ts only mocks the flag TRUE, so a future gate refactor could silently re-enable cutover on flag-off without failing CI. This sibling file mocks it FALSE and locks the three guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(studio): leading flag-gate on sdkGsapTweenPersist (review #1539 nit, Via) The add-op getElement existence check ran before the inner gate, so flag-off did an SDK touch before falling back. Lead with the flag guard to match the other three chokepoints — flag-off is now a clean no-op at every entry point. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): unroll-preservation regressions — non-for loops + AST index substitution (review R2) The #9 unroll-preservation fix had two confirmed regressions: - Non-for loops (forEach/for-of/for-in/while): loopIndexVarName returns null, so substitution no-op'd and preserved siblings kept a now-undefined loop variable (e.g. `item`) → ReferenceError at render. Now returns null for those forms → caller falls back to the blanket loop overwrite (drops siblings, valid code). The #9 fixture only used `for(let i…)` so it never caught this. - substituteLoopIndex did a \bvar\b regex over raw source including string literals, corrupting selectors like ".row-i" → ".row-0". Now AST-based: substitutes only real Identifier uses, skipping string literals and non-computed member/key positions (extracted isIndexBindingPosition helper to stay under the fallow complexity threshold — no ignore added). Two regression tests added (forEach no-dangling-var; for-loop string-literal intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sdk,core): unrollDynamicAnimations rejects empty element list (R1 #1501b) An empty `elements` array has no unrolled form — the writer would overwrite the loop/statement with zero tween calls, silently deleting the animation. - gsapWriterAcorn: unrollDynamicAnimations returns the script verbatim on an empty list (no-op instead of a destructive overwrite). - validateOp: reject unrollDynamicAnimations with empty elements as E_INVALID_ARGS so callers get a clean error rather than silent corruption. - Tests: writer no-op on []; validateOp E_INVALID_ARGS on []. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(sdk): cache draft element in applyDraft, drop HTMLElement casts (R1 #1490a) applyDraft runs at 60fps during a drag but re-ran doc.querySelector on every call — the _draftEl/_draftId fields were only consumed by commit/cancel, never to skip the query. Reuse the tracked element when the id matches and the node is still connected; re-query only on id change or detach (iframe reload). Retypes _draftEl to HTMLElement | null (only ever set from querySelector<HTMLElement>), which removes the `as HTMLElement` casts in commitPreview / _clearDraft. Test asserts a repeated same-id drag queries once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sdk,core): round-3 correctness — unroll AST safety, single-dispatch undo, empty-arg guards, persist decouple Addresses the highest-severity round-3 review findings: - gsapWriterAcorn unroll (R3 #1/#2/#9): the round-2 AST-substitution fix emitted invalid GSAP for object shorthand `{ i }` (→ `{ 0 }`) and shadowed inner bindings (→ `for(let i=0;0<3;0++)`), and silently dropped sibling statements on non-`for` loops (forEach/for-of). The unroll now REFUSES (no-ops, leaving the dynamic loop intact) whenever siblings can't be safely reproduced — a non-`for` loop, an unmodeled statement, or an unsafe index use — instead of dropping or corrupting. Plain `for` loops with safe siblings still unroll. - session single-dispatch undo (R3 #5/#11): _dispatch now reverses the inverse patch list (parity with batch()). A single op emitting order-dependent inverse patches — a nested parent+child removeElement, an aliased multi-target — undid forward and dropped the child subtree / landed on an intermediate value. - materializeKeyframes empty-array (R3 #10): the unguarded twin of the just-fixed unrollDynamicAnimations. Writer no-ops on an empty keyframe list; validateOp rejects it as E_INVALID_ARGS (shared gsapScriptMissing helper). - history:false persist decouple (R3 #4): persist (auto-save) no longer lives inside the history-enable block, so opting out of SDK undo no longer silently disables all disk writes (data-loss trap for #1496's flag consumers). Tests: unroll refuse cases (shorthand/shadow/forEach) + safe-for-loop regression; nested removeElement undo; materializeKeyframes writer no-op + validateOp reject; history:false-still-persists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): stripGsapForId re-parses per removal so all tweens for a deleted element are stripped (R3 #3) Animation ids are count-based (positional), so removing one tween renumbers the survivors. stripGsapForId captured every matching id from a single up-front parse then removed against the mutating script — after the first removal the later ids were stale and silently no-op'd, leaving an orphaned tl.to() referencing the just-deleted element. Now re-parse after each removal and strip the first still-matching animation until none remain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): gsap writer — keyframe ease routing, convert preserves delay, addLabel dedup (R3 #7/#8/#12) - #7: updateAnimationInScript routes an ease update on a keyframe tween to keyframes.easeEach (per-keyframe), not a top-level ease that GSAP ignores — the user's keyframe-easing edit was silently a no-op. - #8: convertToKeyframesFromScript now preserves every non-editable vars key (delay/callbacks/stagger/yoyo/…) verbatim via preservedVarsEntries instead of rebuilding from the GsapAnimation object, which had no `delay` field and dropped it — shifting the tween's start time. - #12: addLabelToScript moves an existing same-named label (overwrites its position) instead of appending a duplicate; duplicates made removeLabel over-remove (it deletes every match, including a pre-existing label). Tests: easeEach routing, delay preservation, addLabel move-not-duplicate + hand-authored-dup removal. Updated the old "no dedup contract" corpus test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sdk): handleSetTiming #domId + data-duration sync; validateOp resolves ids + arc/selector (R3 #6/#13, CF2 #15/#16) CF2 #15: handleSetTiming re-synced GSAP tweens only when the selector matched the element's hf-id. The common #domId-targeted tween (authored by the Studio panel) never matched, so moving/resizing a clip via the SDK timing path left its animations unsynced. Now match the tween selector against the DOM id too. CF2 #16: handleSetTiming read/wrote only data-end. Clips authored with data-duration (what the runtime prefers) got a fresh data-end beside a stale data-duration (no playback change) and oldDuration=null collapsed the GSAP duration-scale ratio to 1. Now read duration preferring data-duration, and write back to whichever attribute the clip uses (timingPath gains a "duration" field). R3 #13b: deleteAllForSelector compared selectors with strict === and missed the alternate quote style ([data-hf-id='x'] vs "x"); now quote-insensitive. R3 #6/#13a: validateOp now resolves the animationId for id-bearing GSAP ops (E_TARGET_NOT_FOUND instead of a misleading ok that no-ops at apply), and updateArcSegment validates the arc is enabled + the segment index is in range. Tests: #domId move sync, data-duration resize + scale, quote-insensitive delete, unresolved-id rejection, arc-segment preconditions. Updated the loose-can() test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core,sdk): name the acorn-node type alias; keyToPath round-trips timing.duration (R3 #14) - gsapWriterAcorn: replace the bare `: any` AST-node annotations with the named `type Node = any` alias, matching the established convention in gsapParserAcorn.ts / gsapInline.ts ("acorn ESTree nodes are structurally untyped"). Documents intent and is greppable; type-identical (zero runtime change). A full ESTree typing is a deliberate architecture decision the codebase has not taken and is out of scope here. - patches: keyToPath/timingPath now include the "duration" timing field added for the data-duration resize fix, so a timing.duration override round-trips on T3 replay instead of being dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sdk): cascadeRemoveAnimations re-parses per removal (R4 — SDK twin of #3) cascadeRemoveAnimations captured every matching animation id from a single up-front parse, then removed against the mutating script — the SDK-side twin of the stripGsapForId bug (R3 #3). Animation ids are positional, so removing the first tween for an element renumbered the survivors and the stale later ids no-op'd, orphaning those tweens on the just-removed element. Now re-parse after each removal and strip the first still-matching animation until none remain. Also adds the reviewer's defense-in-depth test: an aliased multi-target setStyle (same id twice) undoes to the original, not the intermediate (exercises the single-dispatch inverse reversal from R3 #5/#11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Summary
setArcPathop: creates a GSAP MotionPath arc animation on an element from a start point, end point, and curvature; writesgsap.to(...)with amotionPathobject via the acorn writerupdateArcSegmentop: updates position or curvature of an existing arc path without re-creating the whole tweenremoveArcPathop: removes an arc path tween from the GSAP scriptbuildArcPath/buildMotionPathObjectCodehelpers extracted togsapSerialize.tsso the parser and runtime produce identical arc config from coordinatesGsapProvenance/KeyframeEditabilitytypes added to classify how a tween was built (literal/helper/loop/dynamic) and whether it is directly editablekeyframeIndexvariant ofremoveGsapKeyframe; only percentage-based removal remainsTest plan
mutate.gsap.test.ts(setArcPath, updateArcSegment, removeArcPath)bun run testgreen acrosscoreandsdk