test(studio): add T11 history coalescing and cross-prop separation suite

[vanceingalls]

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).

[vanceingalls]

• test(core): add T9 CompositionVariable font/image parse spec (before R1) #1246
• 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 👈 (View in Graphite)
• 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]

T11 — two substantive tests are good:

• Same coalesceKey → coalesces correctly (before from first entry, after from second, id from second ✅)
• Different coalesceKey → no coalesce ✅

The prop:title.color / prop:body.color key format isn't defined anywhere yet — fine for now, but R5/R6 should canonicalize this format and these tests should be updated to match.

The two .todos are placed between the new tests and an existing test ("does not coalesce source editor edits outside the coalesce window"), which breaks the visual flow a bit. Minor, but consider moving todos to the bottom of their describe block.

✅ Approve.

[james-russo-rames-d-jusso]

Hey Vance — T11 lands a clean cross-prop separation test plus two .todo stubs blocked on R5/R6 infrastructure. The cross-prop test is the load-bearing one and pins exactly the coalescer's key-discrimination logic the refactor will need to preserve. No blockers. Two notes on coverage shape + a PR-body count nit.

What I verified

Walked each test against "what could R5/R6 break that this catches":

• Same-coalesceKey within window (test 1) — coalesces to one entry, file-state runs before: "a", after: "c" end-to-end. Locks that the coalescer accumulates file deltas correctly across coalesced entries, not just the latest. Load-bearing for any refactor that touches the merge path.
• Cross-prop separation (test 2) — two entries with different coalesceKey values within the same coalesce window → two separate undo entries. The most load-bearing test in this PR. If R5 introduces a gesture lifecycle that auto-generates coalesceKeys, this test catches "two distinct prop edits got bucketed into one undo because the new keying scheme collided." Hard to over-praise this one.
• .todos — both are correctly punted to where the API exists. Gesture lifecycle needs R5; SDK session needs the SDK runtime bridge (R6). Honest scope.

Concerns

• PR body says "New passing test (1)" but the diff adds two passing tests (same-key + cross-prop) plus two .todos. Worth tightening the PR body so the reader's mental model matches the diff. The same-key test is a complementary prop:-keyed re-assertion that the existing same-key coalescing pattern still works under the new prop: keying convention — that's a useful safety net, just under-described.
• Window-edge coverage is missing. Tests 1 and 2 both use now: 100 and now: 200 with coalesceMs: 1000 — comfortably inside the window. There's no test that exercises the boundary, e.g. now: 100 and now: 1100 with coalesceMs: 1000 (exactly-at-window — coalesce? separate?). The contract at the edge is the kind of thing a refactor can flip silently. The existing test file (per the diff context at line 209) has "outside the coalesce window" — assuming that covers now: 1101+, but pinning exactly-at-window would close the boundary gap. Cheap one-line test. (See nit.)
• The .todo for origin:applyPatches is the highest-value future test in this PR. "SDK patches don't push to undo stack to prevent undo loops" is the kind of contract that, if it regresses, surfaces as user-visible weirdness (undo button does nothing, or worse, infinite undo loop). Worth flagging in the R6 PR description as a hard precondition for merge.

Nits

• Add a boundary test: now: 100 + now: 100 + coalesceMs — is exactly-at-window a coalesce or a split? Both behaviors are defensible; the codebase's current answer should be locked. One test, three lines. (nit)
• Both .todo stubs reference future APIs (session.on("patch", ...), session.dispatch(...)) only in the text. Worth a // blocked by: R5 gesture lifecycle / R6 SDK session — see plan doc comment so they don't sit forever as "todo without context." If there's a Linear ticket per R-number, the cross-link is even better. (nit)
• Coalesce-key format prop:title.color is implied but not documented in the test file. A one-line comment at the top of the describe block — "coalesceKey shape: prop:<elementId>.<propName> for studio prop edits" — would make the test self-explanatory for someone reading the file cold. The next reader will be the R5 author trying to honor this contract; cheap to help them. (nit)
• Both new tests use kind: "source". If kind participates in coalescing decisions (e.g. only source kinds coalesce, or source vs sdk kinds never coalesce), there's no test pinning that. Out of scope for this PR but worth a future ticket. (nit)

Questions

• The .todo for gesture-start/commit drag steps — what's the planned API shape? gesture.start(coalesceKey) ... gesture.commit() with a single undo entry on commit, or something else? Asking because the answer affects whether the existing coalesceKey + window approach gets replaced or extended.
• Is coalesceKey: undefined valid today (legacy edit shape), and if so, what's the coalesce behavior? No test covers the undefined-key path; if it matters for the migration, worth one assertion.
• The prop: format prefix — is the plan to namespace coalesce keys by source (prop:, gesture:, sdk:) or by intent (reversible:, final:)? The choice will shape every test name from R5 onward.

What I didn't verify

• The HeyGenverse plan doc (HTTP 403 unauthenticated). Trusted the PR body's T11 framing.
• The existing same-coalesceKey tests at lines 176-243 (the PR body references them but I only saw the line-209 context in the diff). Trusted the framing that the new same-key test complements rather than duplicates them.
• Behavior of pushEditHistoryEntry with the redo stack pre-populated — coalescing might interact with redo invalidation, and there's no test for that interaction here.
• Whether coalesceMs defaults to a reasonable value when omitted from pushEditHistoryEntry options (both new tests pass { coalesceMs: 1000 } explicitly).

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

The window-boundary test is the one I'd prioritize: now: 100 + coalesceMs (exactly at the boundary) — coalesce or split? The current tests at +100ms and +200ms are both comfortably inside the window. Fencepost bugs are the class of error a refactor is most likely to introduce, and the existing "outside the window" test from the pre-PR file presumably covers now: 1101+, leaving exactly the now: 1100 case uncovered.

Rames's note about the .todo placement (they're between test 2 and an existing test) — the does not coalesce source editor edits outside the coalesce window test appears right after the new todos. Move the todos to the end of their describe block before this merges; it's a one-move change and will save any future git blame reader from puzzling over the ordering.

The origin:applyPatches undo-exclusion todo pairing with T4's matching todo: both should reference a shared Linear ticket so they don't drift. If there's no ticket yet, create one now.

[vanceingalls]

Addressed in latest push:

• Todo placement: moved to bottom of describe block (done in prior push).
• Window-boundary test (miguel + Rames): Added. The coalesce condition is delta <= coalesceMs (line 123 of editHistory.ts) — inclusive boundary. Test uses now: 100 → now: 1100 with coalesceMs: 1000 (delta = 1000 ≤ 1000) → coalesces into one entry. This pins that fencepost against a future refactor flipping <= to <.
• PR body count (Rames): two passing tests land (same-coalesceKey + cross-prop), not one. PR description updated.
• origin:applyPatches + T4 pairing: both todos reference the same future behavior; they'll land together in the R6 pass.

[miguel-heygen]

Follow-up: both concerns addressed.

• ✅ Boundary test added: now: 1100 with coalesceMs: 1000 — boundary is <= (inclusive), comment documents it explicitly
• ✅ .todos moved to end of describe block

One remaining nit from Rames: the applyPatches undo-exclusion todo should ideally share a Linear ticket with T4's matching stub, but not a blocker.

✅ Re-approve.

The base branch was changed.

[vanceingalls]

Merge activity

• Jun 7, 1:27 AM UTC: @vanceingalls merged this pull request with Graphite.