test(core): add T9 CompositionVariable font/image parse spec (before R1)

[vanceingalls]

T9 — CompositionVariable font/image parse spec (before R1)

Spec tests for net-new variable types that gate brand-kit editability. Written before R1 adds them so the addition is test-driven.

What this gates

R1 needs to:

• Add "font" and "image" to CompositionVariableType
• Update parseCompositionVariables to accept and preserve them
• Extend ColorVariable with optional brandRole field

Brand-kit applies (setVariableValue with font: and logo: roles) depend on the parser correctly passing these variables through.

Test split

Red (spec — intentionally failing until R1):

• type: "font" variable is parsed and returned (currently filtered out)
• type: "image" variable with brandRole is parsed and returned (currently filtered out)
• type: "color" variable preserves brandRole field (field not yet on ColorVariable type)

Green (baseline — already work, must not regress):

• Unknown type is rejected gracefully without throwing; variable is skipped

File

packages/core/src/parsers/htmlParser.test.ts — new tests appended to describe("extractCompositionMetadata")

🤖 Generated with Claude Code

[vanceingalls]

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

• test(core): add T9 CompositionVariable font/image parse spec (before R1) #1246 👈 (View in Graphite)
• test(core): add T2 stable id spec for parse-to-hf id contract (before R1) #1245
• test(studio): add T4 op-contract stubs for editor dispatch boundary #1243
• test(studio): add T11 history coalescing and cross-prop separation suite #1242
• test(core): add T8 override-set merge semantics suite for getVariables #1241
• test(core): add T1 round-trip idempotence suite for parse/serialize #1240
• main

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

[miguel-heygen]

T9 — the font and image spec tests are clear. One scope note:

The 3rd spec test ("color variable with brandRole") is actually testing ColorVariable gaining a brandRole field, not font/image type support. The PR title/description only mentions font and image. This test failing is correct (brandRole isn't on ColorVariable yet), but it documents a 3rd thing R1 needs to do that isn't mentioned in the PR body. Worth adding a bullet to the PR description: - Extend ColorVariable with optional \brandRole` field`.

The (v as Record<string, unknown>)?.brandRole cast is pragmatic for pre-type-exists specs — fine here, remove once R1 adds the field to the type.

✅ Approve.

[james-russo-rames-d-jusso]

Hey Vance — T9 pins the parse contract for font and image variable types plus the brandRole field, all of which gate brand-kit editability. No blockers. Two notes on the cast pattern + an asymmetry in reject-surface coverage.

What I verified

Walked each test against "what could R1 silently break that this catches first":

Red (spec — intentionally failing until R1):

• type: "font" parses with name + source — pins R1 extends CompositionVariableType to include "font" and parseCompositionVariables accepts the extra source / default_name / default_source fields. Load-bearing for brand-kit font apply.
• type: "image" with brandRole parses and preserves brandRole — pins both the type extension and the brand-role field passthrough. The brand-role assertion (brandRole: "logo:primary") is the more fragile one — see Concerns.
• type: "color" with brandRole preserves brandRole as extra field — pins that existing types get the same brandRole passthrough. This is the test that catches "R1 added brandRole to image/font but not to color" — the asymmetry trap.

Green (baseline):

• Unknown type rejected gracefully — pins that parseCompositionVariables is forgiving on unknown types. Critical for downlevel SDK compatibility (an older studio reading a newer composition with a type: "lottie" it doesn't know about should skip, not throw).

Concerns

• The (v as Record<string, unknown>)?.brandRole cast suppresses TypeScript's type-safety net. If R1 lands brandRoll (typo'd) or brand_role (snake-case) or role (renamed during review), this assertion would still pass against the raw parsed object — but every downstream consumer typed against the proper brandRole would silently miss the field. Two cheap fixes:
  • (a) Add a post-R1 cleanup comment: // TODO: drop the Record cast once ImageVariable type is extended in R1, so the next reader knows the cast is intentional debt with a defined endpoint.
  • (b) Better — after R1, the field is on the type; a runtime test exercising the typed path (expect(v.brandRole).toBe(...) once it compiles) is the right shape. Today's cast is fine if the cleanup is tracked.
• Reject-surface coverage is asymmetric. The "unknown type rejected gracefully" baseline test covers type: "widget". But the spec doesn't test:
  • type: "font" without a source field — does R1 reject (require source)? Or accept with empty source?
  • type: "image" without a default — what's the fallback?
  • type: "font" with default: null or default: 123 (type-mismatched) — graceful or thrown?
  • The "accept" surface is more aggressively tested than the "reject" surface. For a parser, both matter equally — a forgiving parser that accepts garbage in source is just as fragile as a strict one that throws on a real composition.
• The extractCompositionMetadata API is being exercised, but the underlying parseCompositionVariables (mentioned in the PR body as the function being changed) isn't tested directly. Probably right — the public boundary is extractCompositionMetadata and direct-unit testing the internal parser would couple tests to internals. But worth a sentence confirming that intent.

Nits

• The font-variable fixture has six fields (id, type, label, default, source, default_name, default_source). The assertions only check id and type. Other fields (source, default_name, default_source) could be silently dropped by R1's parser and the test would still pass. If those fields are load-bearing for the brand-kit apply path, assert them. (nit)
• The image-variable test's default: "" — is empty-string a valid default for an image variable? If R1 normalizes it to null or throws on empty, the test catches it on the typed path. Worth confirming the contract for "no default image." (nit)
• No test for type: "color" without brandRole — does it parse normally? Implied by the existing test file (line 639+ of pre-PR), but pinning that "brandRole is optional, not required" is cheap. (nit)
• Comment block at the top says "tests 1, 2, 3" red and "test 4" green — numbering goes stale if anyone reorders. The [spec] prefix convention (used in #1245) is more durable; consider adopting here. (nit, applies stack-wide)
• brandRole: "logo:primary" and brandRole: "color:primary" use a colon-namespaced format. Worth a one-line comment naming the format spec (<role-namespace>:<role-name>) so the next reader doesn't reverse-engineer the convention from the fixture. Especially important since brand-kit apply will dispatch on this string. (nit)

Questions

• Is the brandRole namespace closed (logo: / color: / font: only) or open-ended? Affects whether R1 should validate the prefix or accept arbitrary strings.
• The default_name / default_source fields on the font variable — are those load-bearing for the brand-kit apply (e.g. fallback if the brand kit doesn't provide a font), or vestigial / migration cruft? If load-bearing, they should have assertions; if vestigial, consider not including them in the spec fixture.
• For type: "image", is default: "" semantically "no image" or "image with no default value, must be provided at apply time"? The contract should be pinned somewhere — either by an assertion or a doc comment.

What I didn't verify

• The HeyGenverse plan doc (HTTP 403 unauthenticated). Trusted the PR body's T9 framing and the brand-kit dependency.
• parseCompositionVariables's current rejection behavior — assumed from the PR body that font and image are filtered out today.
• Whether brandRole is supposed to be optional or required on the new types. The tests only exercise the "with brandRole" path; the "without brandRole" path is uncovered.
• Whether the studio editor's brand-kit apply path consumes brandRole by string-matching or by typed dispatch — affects whether the namespace-prefix format matters at the contract level.

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

The field passthrough gap Rames flagged is the one I'd prioritize: the font-variable fixture includes source, default_name, and default_source, but the assertions only check id and type. R1 could drop source entirely and this test would still green. If source is how the brand-kit apply path injects the font URL, it needs an assertion now — otherwise the first brand-kit integration test will be the first to catch the regression.

Reject-surface asymmetry: font without source and image without default are the two missing negative-path cases Rames called out. These are spec tests, so they're appropriately punted until R1, but they should be stubs (it.todo(...)) rather than absent — otherwise they're invisible to the R1 author.

The (v as Record<string, unknown>)?.brandRole cast: Rames's cleanup comment suggestion is the minimum bar. Adding // TODO(R1): remove cast once ImageVariable.brandRole is typed keeps the debt tracked. Agree with Rames that the cast is acceptable pre-type — just needs the breadcrumb.

The brandRole without-value case (Rames's nit): type: "color" without brandRole — this is the most likely field to be made accidentally required by R1 if the schema is updated carelessly. One baseline test: "color variable without brandRole parses normally" would catch that regression for free.

[miguel-heygen]

Follow-up: all concerns addressed.

• ✅ Font spec now asserts source, default_name, default_source — the three load-bearing fields for brand-kit font apply
• ✅ TODO(R1) cleanup comments on both cast sites
• ✅ New baseline: "color variable without brandRole parses normally" pins brandRole as optional
• ✅ Two new .todo stubs for reject-surface: font without source, image without default

✅ Re-approve.

[vanceingalls]

Addressed in latest push:

• source/default_name/default_source field assertions (miguel + Rames): font test now asserts all three fields. These are load-bearing for brand-kit font apply — R1 must not drop them.
• Cast cleanup comments (miguel + Rames): added // TODO(R1): remove cast once ImageVariable.brandRole is typed and // TODO(R1): remove cast once ColorVariable.brandRole is typed on both as Record<string, unknown> casts.
• Color-without-brandRole baseline (miguel): added — asserts a plain type: "color" variable without brandRole parses normally and brandRole is undefined. Guards R1 from accidentally making brandRole required.
• Reject-surface stubs (miguel + Rames): added it.todo for "font without source is rejected" and "image without default is rejected" — these are spec-level questions for R1 to answer when extending parseCompositionVariables.
• brandRole namespace (Rames Q): open-ended string for now (<namespace>:<name> convention, e.g. logo:primary, color:primary). R1 will define the closed set when brand-kit apply is implemented.