test(studio): add T5b rotation+motion build-patches characterization

[vanceingalls]

Summary

T5 (part 2 of 3) — extends manualEditsDomPatches.test.ts with the rotation and motion pairs.

Same 4-pattern structure as T5a: populated, empty, clear restores originals, build/clear symmetry.

Notable behaviors locked down:

• buildRotationPatches emits STUDIO_ROTATION_ATTR = "true" always; buildClearRotationPatches also nulls STUDIO_ROTATION_DRAFT_ATTR defensively (not recorded by build — tested explicitly)
• buildMotionPatches returns [] when STUDIO_MOTION_ATTR is absent (early exit, no ops)
• buildClearMotionPatches takes _element (ignored) and unconditionally nulls all 4 motion attrs

Stack

• test(studio): add T5a pathOffset+boxSize build-patches characterization #1257 — pathOffset + boxSize pairs
• test(studio): add T5b rotation+motion build-patches characterization #1258 (this PR) — rotation + motion pairs
• test(studio): add T5c review-fix gaps in manualEditsDomPatches characterization #1259 — review-fix gaps (ordered clear assertion, coercion paths, edge cases)

[vanceingalls]

• test(core,studio): add T3+T7 hfId targeting stubs (spec for R1) #1267
• test(core): add T6a GSAP parser golden baselines (Recast/Babel snapshot) #1263
• test(core): add T10 PreviewAdapter contract stubs (spec for R7) #1262
• refactor(core): extract maxEndTime+serialize to parsers/test-utils.ts (TU) #1261
• test(studio): add T5c review-fix gaps in manualEditsDomPatches characterization #1259
• test(studio): add T5b rotation+motion build-patches characterization #1258 👈 (View in Graphite)
• test(studio): add T5a pathOffset+boxSize build-patches characterization #1257
• main

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

[miguel-heygen]

Consistent with T5a's 4-pattern. Two behaviors worth calling out that the tests lock down well:

• buildRotationPatches always emits STUDIO_ROTATION_ATTR = "true" (marker, not the rotation value itself) — tested in both populated and empty cases.
• buildClearRotationPatches nulls STUDIO_ROTATION_DRAFT_ATTR defensively even though buildRotationPatches never records it. This asymmetry is documented in the test description and is correct — explicit is better than silent.
• buildMotionPatches(div()) → [] early exit is a meaningful edge case that's easy to miss. Good that it's pinned.

The motion clear test only exercises div() here; T5c adds populatedMotionEl() to verify input-independence. Fine as a staging step.

Approved — no issues.

[james-russo-rames-d-jusso]

Round-1 review of T5b (rotation + motion characterization). Walked the delta from #1257 (+130/-0, two new describe blocks) against buildRotationPatches/buildClearRotationPatches/buildMotionPatches/buildClearMotionPatches in manualEditsDomPatches.ts:181-237. Consistent with T5a's 4-pattern and tighter than T5a in two spots — the rotation clear populated test uses full toEqual (no arrayContaining escape hatch), which is the form T5a's box-size clear should have used.

What's strong

• Rotation clear populated test uses full toEqual. Asserts all 10 ops in exact declaration order — including the defensive STUDIO_ROTATION_DRAFT_ATTR: null that's emitted by clear but not by build (line 203 in the source). This catches the orphan-pair concern from T5a's symmetry check without needing a reverse invariant.
• Motion empty: returns [] when STUDIO_MOTION_ATTR is absent. The if (!motionJson) return [] early-exit in buildMotionPatches:222 is a real branch — a future refactor that read other attrs first (e.g. accidentally calling collectAttributeOps before the guard) would push ops on bare elements and break the "skip drafts on un-motioned elements" invariant. Test covers the early exit.
• MOTION_JSON constant pulled out. Avoids inline JSON noise in the assertion and makes the "JSON payload survives" property explicit.

Concerns

[important — buildClearMotionPatches input-independence is not verified] The function signature is _element: HTMLElement (underscore-prefix → intentionally unused) and the implementation at line 230-237 hardcodes the four null ops without reading the element. Your test only calls buildClearMotionPatches(div()):

it("clear: always nulls all four motion attrs regardless of element state", () => {
  expect(buildClearMotionPatches(div())).toEqual([…]);
});

A future refactor that accidentally reads from _element (e.g. dropping the underscore and conditionally emitting based on attrs) would pass this test on an empty div() while failing on a populated motion element — exactly the kind of silent regression characterization tests are supposed to catch. Worth adding expect(buildClearMotionPatches(populatedMotionEl())).toEqual(expected) alongside the empty-div assertion to lock in the no-op-on-input semantics.

Looking at the stack — #1259 T5c addresses this exactly:

const expected = [ … ];
expect(buildClearMotionPatches(div())).toEqual(expected);
expect(buildClearMotionPatches(populatedMotionEl())).toEqual(expected);

Same pattern as my T5a comment — gap real, closed at stack head.

[important — rotation clear test setup is sparse] Lines 274-290, the populated rotation clear test sets only 3 of the 4 ROTATION_ORIG_ATTRS:

e.setAttribute(STUDIO_ORIGINAL_INLINE_ROTATE_ATTR, "30deg");
e.setAttribute(STUDIO_ORIGINAL_ROTATION_TRANSFORM_ORIGIN_ATTR, "top left");
e.setAttribute(STUDIO_ORIGINAL_TRANSFORM_DISPLAY_ATTR, "grid");

STUDIO_ORIGINAL_ROTATE_ATTR (line 175 in source's ROTATION_ORIG_ATTRS) isn't set — so the collectAttributeOps branch for that attr is never exercised. The test asserts null for the corresponding output op, which is correct given the input — but doesn't verify the populated branch. Worth either (a) setting all four orig attrs on the test element, or (b) explicitly noting which branches each clear test exercises.

Also: the rotation clear has a tricky distinct null pathway — origRotationTransformOrigin !== null ? origRotationTransformOrigin || null : null (line 200). The empty-string-coerce-to-null path (origRotationTransformOrigin = "" → value: null) is reachable but uncovered by this PR. #1259 adds clear: empty STUDIO_ORIGINAL_INLINE_ROTATE_ATTR coerces to null for inline-rotate but doesn't cover the transform-origin coerce — minor gap that survives the stack.

Nits

• MOTION_JSON = '{"kind":"gsap-motion","start":0,"duration":1}' is a representative payload but the test never asserts the shape is valid GSAP motion JSON — it just round-trips the opaque string through buildMotionPatches. The function doesn't parse the JSON either, so this is correct behavior to test — just worth a comment noting "any non-empty string for the marker; shape is validated elsewhere" so a future reader doesn't waste time wondering if the test should validate the JSON.
• expect(buildMotionPatches(div())).toEqual([]) — [] instead of toHaveLength(0). Either works; toEqual([]) catches a null/undefined return that toHaveLength would throw on. Current choice is correct, leaving as a note.

Plan-doc alignment

T5b = "rotation + motion pairs" matches what's in the diff. Stack-wise: T5a (#1257) → T5b (#1258) → T5c (#1259). Confirms the staged-rollout pattern from the prior batch.

Verdict

Consistent with T5a's quality. The motion-input-independence gap and the rotation-clear-setup-sparsity are real concerns but both flagged-and-addressed (or partially-addressed) by #1259. Solid. Leaving as a comment.

— Review by Rames D Jusso

The base branch was changed.