test(studio): add T4 op-contract stubs for editor dispatch boundary#1243
Conversation
0b25203 to
f6031a6
Compare
e76d2e8 to
c4540f8
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
T4 — all stubs, nothing executable. That's fine as intent documentation for R5/R6, but a couple thoughts:
-
The
dispatch boundarydescribe block includes"dispatching an unknown op type throws at the boundary, not silently fails"— this is an important correctness contract; once R5 lands, make sure this one gets implemented first (silent failures are worse than loud ones). -
"applyPatches with origin:'applyPatches' does not push to undo stack"— this one pairs directly with T11's todo aboutorigin:applyPatches. They should land together in the same pass.
No changes needed — stubs serve their purpose.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Hey Vance — T4 is 14 .todo stubs across five describe blocks, defining the op-dispatch contract before R5/R6 implement it. No blockers. This is the most documentation-shaped PR in the stack — calling that out so the framing is honest, then a few notes on which stubs could become actual assertions today.
What I verified
This file ships zero regression-net coverage today (all .todo). Its value is forward-looking: it's an acceptance criterion R5 / R6 can be reviewed against. That's a legitimate purpose; it just means the "is this load-bearing?" question gets a different answer than the other five PRs in the stack — not yet, but designed to be.
Walked each describe block:
- move / resize / retime / style op shapes — pure-shape stubs (e.g. "move op has
{ type: 'move', id, x, y }shape"). These are TypeScript-discriminated-union assertions in disguise; see Concerns. - "applied to element produces updated style/attribute values" — actual behavior tests. Need a dispatcher to assert against. Genuinely
.todountil R5. - "recorded in edit history with label X" — integration with the existing edit history (which T11 / #1242 already covers). Could be a thin assertion the moment the
dispatchfunction lands. - "coalesces same id+prop within window" — composes T11's coalescing logic with the op-dispatch boundary. Honest
.todountil R5. - dispatch boundary — three stubs:
origin: 'studio'emission,applyPatchesundo exclusion, unknown-op-throws. These are the sharp invariants. The third one (unknown-op-throws) is the canary that catches a refactor accidentally silent-skipping malformed ops.
Concerns
- Several "op has
{...}shape" stubs could be TypeScript-level assertions today without waiting for R5. If theOpdiscriminated union is going to live inpackages/studio/src/types/ops.ts(or similar) and R5 just wires the dispatcher, you can land the type definitions now and assert shapes viaexpectTypeOf<MoveOp>().toMatchTypeOf<{ type: 'move'; id: string; x: number; y: number }>(). That converts 6+ of the 14 stubs into hard pass/fail assertions on day one — not just for R5's benefit but for anyone who lands a type-level change between now and R5. Cheap upgrade if the Op union is ready. - The "unknown op throws" stub is the highest-value future test. Today, if R5 ships and an unknown op silently no-ops, the symptom is "the SDK sent an edit but nothing happened" — extremely hard to debug from logs alone. A test pinning that the dispatcher throws on unknown
typeis the kind of thing that prevents a class of "ghost edit" bug. Worth marking in the R5 PR description as a hard merge requirement. - No
.todofor the "dispatch returns a result type" / "dispatch is async" question. If R5's dispatcher returnsPromise<DispatchResult>(e.g. patch list + history entry), the test surface is bigger than what's here. If sync-fire-and-forget, the current stubs cover it. Worth pinning the shape now even if the stubs stay.todo. (See nit.)
Nits
- Stub for
dispatchreturn shape. One moreit.todo("dispatch returns { ops: Op[]; patches: Patch[]; entry: EditHistoryEntry } | void")or similar — even if the exact shape changes, the act of naming the return contract now is cheap and load-bearing for R5's API design. (nit) - No describe block for the "no-op" case. E.g.
dispatch(move with same x/y as current)— does it produce an undo entry or skip silently? Either is defensible; locking the contract is the point. (nit) - Style-op
propisstring. What's the validation contract? CSS-property whitelist, or arbitrary strings? An.todo("style op rejects unknown CSS properties at dispatch")or.todo("style op accepts any string prop, validation deferred to renderer")would pin which side the design lands on. (nit) - Header comment says "op-shape refactor (R5) and runtime bridge (R6)." Worth linking the plan doc URL in the file header so a reader can navigate without leaving the IDE. (nit, applies stack-wide)
- Existential risk for any all-
.todofile is "it sits forever, no one converts the stubs." Worth a// FIXME: convert when R5 dispatcher lands — see <plan-doc-url>at the top, or a Linear ticket tracking the conversion. (nit)
Questions
- Is the plan to convert these stubs to assertions inside R5/R6's PRs (one stub at a time), or to land R5/R6 and then land a separate "fill in the contract tests" PR? Asking because the answer determines whether R5/R6's PR descriptions should reference this file by name as a merge precondition.
- The
originfield — is it'studio'(string literal) or a discriminated union with other origins ('sdk','applyPatches', etc)? The stub on line 42 implies a literal; the stub on line 43 implies'applyPatches'is a distinct origin value. Worth one stub naming the full union now. - Are there other op types not yet in this contract (e.g.
delete,create,reorder,groupSelect)? The stack-level plan probably covers these elsewhere, but worth confirming this contract is the complete edit-op surface, not just the demo subset.
What I didn't verify
- The HeyGenverse plan doc (HTTP 403 unauthenticated). Trusted the PR body's T4 framing.
- Whether R5/R6 are scoped to land separately or jointly — the stub design assumes "R5 ships the types and dispatcher, R6 wires the SDK origin guard" but they could be one PR.
- Whether any current code already uses
dispatch-like nomenclature that this contract would conflict with — grep fordispatchinpackages/studio/src/would close that; punted as out-of-scope for a.todo-only PR.
— Review by Rames D Jusso
c4540f8 to
c317301
Compare
f6031a6 to
5caae48
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Building on Rames's review.
Rames's TypeScript-level shape assertions point is the highest-value upgrade here: if the Op discriminated union is being authored in R5, several "op has {type, id, x, y} shape" stubs could be expectTypeOf<MoveOp>().toMatchTypeOf<{...}>() calls right now — before R5 lands, they'd just fail to compile if the type doesn't exist, which is the right signal. Consider landing the union type definition in a thin PR ahead of R5 so these stubs can convert.
The "unknown op throws" stub is the one Rames and I both flagged independently — making it a hard merge requirement for R5 is the right call. Silent dispatch failures are nearly impossible to debug from logs.
No-op case: dispatch(move, {x: sameX, y: sameY}) — does it produce an undo entry or skip? This is a common refactor footgun. Worth one stub even if the answer is "skip silently," because pinning "skip" prevents a future change from accidentally adding an empty undo entry per drag event.
5caae48 to
fdc3073
Compare
c317301 to
eaba44c
Compare
|
No code changes — stubs serve their purpose as acknowledged. Notes on the open questions:
|
…1240) ## What Adds `htmlParser.roundtrip.test.ts` — T1 from the SDK migration test plan. Tests that `parseHtml → generateHyperframesHtml → parseHtml` is lossless for element structure and timing. Scope is DOM/timing only; GSAP script round-trip is T6 territory. ## Tests **Inline fixtures (5):** - element count + ids preserved - `startTime` / `duration` preserved - element types preserved (`text`, `video`, `img`, `audio`) - double-serialize stability (`serialize(parse(serialize(parse(html)))) === serialize(parse(html))`) - empty stage doesn't throw **Registry block sampling (10):** first 10 blocks in `registry/blocks/` — each asserts element count survives a round-trip. ## Finding Stability test surfaced a real bug: `generateHyperframesHtml` defaults `compositionId` to `` `comp-${Date.now()}` ``. Since `ParsedHtml` doesn't capture this, every re-serialize emits a different id. The test works around it by passing a fixed `compositionId: "test-comp"` so structural instability is still detectable. The root cause is tracked as **R1 (stable hf- ids)**. ## Stack Prerequisite for: T8 (#1241), T11 (#1242), T4 (#1243)
fdc3073 to
c74cfde
Compare
eaba44c to
d5e1cac
Compare
#1241) ## What Extends `getVariables.test.ts` with T8 from the SDK migration test plan: override-set merge semantics. ## Tests (4 new) - **last-write-wins** — calling `setOverrides` twice; second value wins - **sparse override** — override one key, unmentioned declared defaults survive intact - **batch override (brand kit)** — setting all keys at once via a single override object - **manual override after batch** — replacing one key from a kit batch; others untouched ## Scope note Tests are labeled **"flat-merge, current behaviour"** — `getVariables` does `{...defaults, ...overrides}`. Dotted-key path resolution (`"headline.color"` as `id.prop`) is a future SDK concern; these tests validate the flat-merge contract that exists today, not the future path-resolution semantics. ## Stack Stacked on T1 (#1240). Prerequisite for T11 (#1242), T4 (#1243).
8d9db68 to
b474f12
Compare
d5e1cac to
e1260cc
Compare
…ite (#1242) ## What Extends `editHistory.test.ts` with T11 from the SDK migration test plan: history coalescing gaps and origin guard stubs. ## Tests **New passing test (1):** - **cross-prop coalescing separation** — two edits within the coalesce window but with *different* `coalesceKey` values produce two separate undo entries, not one coalesced entry. Fills the gap left by the existing same-file coalescing tests (lines 176–243). **`.todo` stubs (2):** - `gesture-start/commit collapses intermediate drag steps into one undo entry` — requires gesture lifecycle API not yet built - `origin:applyPatches edits excluded from undo stack` — requires SDK session object (`session.on("patch", ...)`, `session.dispatch(...)`) which doesn't exist yet; needed to prevent undo loops when SDK patches are applied ## Stack Stacked on T8 (#1241). Prerequisite for T4 (#1243).
b474f12 to
9aebc8d
Compare
e1260cc to
94fb3cd
Compare
The base branch was changed.
94fb3cd to
afa1ab4
Compare

What
Adds
ops.contract.test.ts— T4 from the SDK migration test plan. All tests are.todostubs defining the expected op-dispatch contract for the Studio editor.Purpose
Specifies the shape and behaviour of the editor operation boundary before R5 (op-shape) and R6 (runtime bridge) implement it. Having the contract written as failing tests first:
Op types covered
move{ type, id, x, y }resize{ type, id, width, height }retime{ type, id, startTime, duration }style{ type, id, prop, value }origin:'studio'on every op,applyPatchesexcluded from undo, unknown op throwsStack
Stacked on T11 (#1242). Top of the stack.