test(studio): add T5c review-fix gaps in manualEditsDomPatches characterization

[vanceingalls]

Summary

T5 (part 3 of 3) — gaps identified in max-setting code review of T5a/T5b, fixed here.

Findings addressed

Important (concrete bug-survival gaps):

1. Box-size clear ordered assertion — the existing test used expect.arrayContaining on a partial element, leaving the interleaved restore-then-null loop order completely untested. Replaced with a full toEqual on populatedBoxEl() asserting all 30 ops in exact order.

2. Motion clear input-independence — buildClearMotionPatches takes _element (intentionally ignored) but the test only called it with div(). A future refactor accidentally reading the element would pass on an empty element while failing on a populated one. Fixed by asserting buildClearMotionPatches(populatedMotionEl()) produces the same output as buildClearMotionPatches(div()).

Suggestions (edge-case branches, reachable in production):

3. Empty-string coercion — buildClear* functions use origVal || null to coerce an empty-string saved attribute into a null op value (meaning "remove the style, don't set it to ""). Added coercion tests for pathOffset (translate), boxSize (width), and rotation (rotate).

4. Rotation clear with absent STUDIO_ORIGINAL_ROTATION_TRANSFORM_ORIGIN_ATTR — the null !== null branch in the ternary produces value: null via a distinct path from the coercion path. Added explicit test.

Stack

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

[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 👈 (View in Graphite)
• test(studio): add T5b rotation+motion build-patches characterization #1258
• 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]

This PR delivers the tightening pass I expected after T5a/b.

Three gaps closed:

1. buildClearBoxSizePatches full ordered toEqual — 30 ops, exact interleaved restore-then-null order. The previous arrayContaining would have passed if ops were reordered or extra ops were silently added. The full ordered assertion is the right level of strictness for a clear-patches characterization.

2. buildClearMotionPatches input-independence — populatedMotionEl() produces the same output as div(). This is the only place in the suite where the function's underscore parameter is tested behaviorally. Critical — without this, a future refactor that accidentally reads the element wouldn't be caught.

3. Empty-string coercion — STUDIO_ORIGINAL_*_ATTR="" → value: null via the origVal || null pattern. Three cases covered (translate, width, rotate). This pins the contract that empty attributes mean "remove the property", not "set it to an empty string".

No issues. Solid closure pass.

[james-russo-rames-d-jusso]

Round-1 review of T5c (review-fix gaps). Walked the delta from #1258 (+69/-16) and verified each of the four called-out gaps is closed by a concrete test, with the implementation paths in manualEditsDomPatches.ts cross-referenced. This PR is the load-bearing-fix layer of the T5 stack — every issue I would have raised on #1257 and #1258 is addressed here, plus two coercion-path gaps I didn't have on my own list.

What's strong (the four called-out gaps)

1. Box-size clear ordered assertion. Lines 213-247, full toEqual over populatedBoxEl() asserting all 30 ops in exact interleaved restore-then-null order. The previous expect.arrayContaining left the order of the for (const [attrName, styleProp] of BOX_SIZE_ORIG_ATTRS) loop in buildClearBoxSizePatches:155-161 completely untested. Now locked in. ✓

2. Motion clear input-independence. Lines 381-389, the two-call expect(…).toEqual(expected); expect(buildClearMotionPatches(populatedMotionEl())).toEqual(expected); pattern. Matches my expected fix shape — a future refactor that accidentally reads _element would pass on bare div() but fail on the populated case. ✓

3. Empty-string coercion paths. Three tests (pathOffset translate at line 121-126, boxSize width at 248-253, rotation rotate at 333-338) covering the origVal || null coercion in buildClear* functions. The semantics — "an empty saved attribute means remove the style, not set it to """ — is now an explicit invariant rather than an implementation detail. ✓

4. Rotation clear with absent STUDIO_ORIGINAL_ROTATION_TRANSFORM_ORIGIN_ATTR. Line 327-330, asserts the absent-attr path through the origRotationTransformOrigin !== null ? origRotationTransformOrigin || null : null ternary at manualEditsDomPatches.ts:200. That ternary has a distinct null pathway from the coercion path (absent vs. empty-string → null), and now both are covered. ✓

Concerns

[nit — toHaveLength(17) is a magic number] Line 261:

it("clear: bare element emits only null ops — no style restores fire when orig attrs are absent", () => {
  const ops = buildClearBoxSizePatches(div());
  // 3 fixed (studio-width, studio-height, box-size marker) + 14 attr-null pushes (one per BOX_SIZE_ORIG_ATTR)
  expect(ops).toHaveLength(17);
  expect(ops.every((op) => op.value === null)).toBe(true);
});

The 17 is explained in the comment, but it's tied to BOX_SIZE_ORIG_ATTRS.length + 3 and would break opaquely if a new orig attr is added in the next refactor. Two options:

• Compute the expected length inline: expect(ops).toHaveLength(BOX_SIZE_ORIG_ATTRS.length + 3) — but that requires importing the array, which inverts the test/impl coupling.
• Drop the length check, keep the "every op is null" check. The shape expect(ops.every((op) => op.value === null)).toBe(true) covers the real invariant (no style restores fire on bare element); the length is incidental.

Lean toward the second — the load-bearing assertion is "no restores fire," not "exactly 17 ops emitted."

[nit — ops.find(o => o.property === "width")?.value is fragile to duplicates] Used in three places (lines 124, 251, 336). If a future refactor accidentally emits two ops with the same property (e.g. one inline-style and one attribute, both named "width"), .find() returns only the first and the test passes even though the second op is wrong. Lower-risk pattern: expect(ops.filter(o => o.property === "width" && o.type === "inline-style")).toEqual([{ … }]). Not blocking — duplicates would surface in the toEqual populated tests in the same describe block.

Verdict

Clean review-fix PR. The four called-out gaps are all closed with the right fix shape, and the two new coercion tests (pathOffset / rotation empty-string) cover paths I hadn't called out on the earlier PRs. The stack converges cleanly at this layer. Two small nits on assertion shape that are non-blocking. Leaving as a comment.

— Review by Rames D Jusso

The base branch was changed.