mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
fix(sdk): address review feedback on getRootElements/serialize/getElementTimings PR
Blocker (flagged by all three reviewers, still open after the CI fix):
- Composition.serialize() interface in types.ts never got the { stripRuntime? }
param the implementation already accepts, so a consumer holding a
Composition-typed ref (exactly pacific's case) got a strict-TS arity error
calling comp.serialize({ stripRuntime: true }). Widened the interface.
Also addresses:
- getRootElements() now cached like elementsCache (same 3 invalidation sites) —
cheap insurance if a layer panel calls it every render tick.
- getElementTimings' resolver now uses the already-parsed expr.value for the
absolute-number case instead of silently re-parsing via parseFloat, via a
small resolveReferenceStart helper split out to keep resolveStart's own
branching low.
- bareId's `?? scopedId` fallback gets a comment: it's unreachable at runtime
(split() always returns >=1 element) but required by noUncheckedIndexedAccess.
- serialize({ stripRuntime }) docblock generalized past "the editing iframe" —
it's for any host driving its own clock.
- Documented (and pinned with a test) the bare-id reference resolution's
cross-scope behavior: a sub-composition element referencing a colliding bare
id resolves to the canonical top-level match, same as the runtime's own
resolver — consistent, but a real authoring footgun worth calling out.
- New tests: chained (A->B->C) references, a direct self-reference cycle, a
mutual A<->B cycle, the cross-scope bare-id collision above, and an import
assertion that RUNTIME_BOOTSTRAP_ATTR is actually reachable from
@hyperframes/core and matches the marker generators stamp.
422/422 sdk tests passing (417 + 5 new). Full workspace build (incl. studio)
verified clean.
This commit is contained in:
@@ -142,6 +142,9 @@ export function resolveScoped(document: Document, id: string): Element | null {
|
||||
*/
|
||||
export function bareId(scopedId: string): string {
|
||||
const parts = scopedId.split("/");
|
||||
// split() always returns >=1 element, so this never actually falls through at
|
||||
// runtime — the fallback exists to satisfy noUncheckedIndexedAccess, not as a
|
||||
// reachable safety net.
|
||||
return parts[parts.length - 1] ?? scopedId;
|
||||
}
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { parseHTML } from "linkedom";
|
||||
import { ensureHfIds } from "@hyperframes/core/hf-ids";
|
||||
import { RUNTIME_BOOTSTRAP_ATTR } from "@hyperframes/core";
|
||||
import { resolveScoped, findById, isNewHostBoundary, bareId } from "./engine/model.js";
|
||||
import { parseMutable } from "./engine/model.js";
|
||||
import { buildRoots, flatElements } from "./document.js";
|
||||
@@ -647,4 +648,11 @@ describe("serialize({ stripRuntime })", () => {
|
||||
expect(out).not.toContain("hyperframe.runtime");
|
||||
expect(out).toContain('data-hf-id="hf-a"');
|
||||
});
|
||||
|
||||
it("re-exports RUNTIME_BOOTSTRAP_ATTR from @hyperframes/core, matching the marker generators stamp", async () => {
|
||||
expect(RUNTIME_BOOTSTRAP_ATTR).toBe("data-hyperframes-preview-runtime");
|
||||
// The fixture's marker attribute above is authored by hand — confirm it's not
|
||||
// drifted from the real constant a generator would actually stamp.
|
||||
expect(RUNTIME_SCRIPT).toContain(RUNTIME_BOOTSTRAP_ATTR);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -151,6 +151,83 @@ describe("getElementTimings — relative data-start references", () => {
|
||||
const timings = comp.getElementTimings();
|
||||
expect(timings["hf-orphan"]).toMatchObject({ enterAt: 0, exitAt: 2 });
|
||||
});
|
||||
|
||||
it("resolves a chained reference (A -> B -> C) through the recursive resolver", async () => {
|
||||
const html = `
|
||||
<div data-hf-id="hf-stage" data-hf-root style="width:1280px;height:720px">
|
||||
<h1 data-hf-id="hf-a" data-start="0" data-duration="2">A</h1>
|
||||
<p data-hf-id="hf-b" data-start="hf-a" data-duration="3">B</p>
|
||||
<p data-hf-id="hf-c" data-start="hf-b + 1" data-duration="1">C</p>
|
||||
</div>
|
||||
`.trim();
|
||||
const comp = await openComposition(html);
|
||||
const timings = comp.getElementTimings();
|
||||
|
||||
expect(timings["hf-a"]).toMatchObject({ enterAt: 0, exitAt: 2 });
|
||||
// hf-b: "hf-a" (no offset) = a's exitAt (2)
|
||||
expect(timings["hf-b"]).toMatchObject({ enterAt: 2, exitAt: 5 });
|
||||
// hf-c: "hf-b + 1" = b's exitAt (5) + 1 = 6
|
||||
expect(timings["hf-c"]).toMatchObject({ enterAt: 6, exitAt: 7 });
|
||||
});
|
||||
|
||||
it("terminates (not an infinite loop) on a direct self-reference", async () => {
|
||||
const html = `
|
||||
<div data-hf-id="hf-stage" data-hf-root style="width:1280px;height:720px">
|
||||
<p data-hf-id="hf-self" data-start="hf-self" data-duration="2"></p>
|
||||
</div>
|
||||
`.trim();
|
||||
const comp = await openComposition(html);
|
||||
const timings = comp.getElementTimings();
|
||||
// The cycle guard fires on the re-entrant call, contributing 0 for the
|
||||
// self-reference's own start — the element's OWN duration (2) still applies on
|
||||
// top of that, so enterAt=2, not 0. The guard's job is termination, not zeroing
|
||||
// the whole chain.
|
||||
expect(timings["hf-self"]).toMatchObject({ enterAt: 2, exitAt: 4 });
|
||||
expect(Number.isFinite(timings["hf-self"]?.enterAt)).toBe(true);
|
||||
});
|
||||
|
||||
it("terminates (not an infinite loop) on a mutual A <-> B reference cycle", async () => {
|
||||
const html = `
|
||||
<div data-hf-id="hf-stage" data-hf-root style="width:1280px;height:720px">
|
||||
<p data-hf-id="hf-a" data-start="hf-b" data-duration="2"></p>
|
||||
<p data-hf-id="hf-b" data-start="hf-a" data-duration="3"></p>
|
||||
</div>
|
||||
`.trim();
|
||||
const comp = await openComposition(html);
|
||||
const timings = comp.getElementTimings();
|
||||
// Document order resolves hf-a first: it recurses into hf-b, which recurses back
|
||||
// into hf-a — the guard fires there (returns 0), so hf-b's start = 0 + hf-a's
|
||||
// duration (2) = 2. Back in hf-a's own resolution: start = hf-b's start (2) +
|
||||
// hf-b's duration (3) = 5. Neither number is "correct" for a genuine cycle —
|
||||
// the point is both are finite and the recursion terminates.
|
||||
expect(timings["hf-a"]).toMatchObject({ enterAt: 5, exitAt: 7 });
|
||||
expect(timings["hf-b"]).toMatchObject({ enterAt: 2, exitAt: 5 });
|
||||
expect(Number.isFinite(timings["hf-a"]?.enterAt)).toBe(true);
|
||||
expect(Number.isFinite(timings["hf-b"]?.enterAt)).toBe(true);
|
||||
});
|
||||
|
||||
it("resolves a colliding bare id to the TOP-LEVEL match, not a same-scope sibling", async () => {
|
||||
// Bare ids have no scope syntax — resolveScoped's bare-id rule prefers the
|
||||
// canonical top-level match when one exists, same as the runtime's own (also
|
||||
// global, not scope-aware) resolver. Both the outer document AND the sub-comp
|
||||
// author an element with the SAME bare id "hf-intro" — a genuine collision.
|
||||
const html = `
|
||||
<!DOCTYPE html><html><body>
|
||||
<h1 data-hf-id="hf-intro" data-start="0" data-duration="10">Outer intro</h1>
|
||||
<div data-hf-id="hf-host" data-composition-file="sub.html">
|
||||
<h1 data-hf-id="hf-intro" data-start="0" data-duration="1">Inner intro (same bare id)</h1>
|
||||
<p data-hf-id="hf-outro" data-start="hf-intro + 1" data-duration="1">Inner outro</p>
|
||||
</div>
|
||||
</body></html>
|
||||
`.trim();
|
||||
const comp = await openComposition(html);
|
||||
const timings = comp.getElementTimings();
|
||||
// "hf-intro + 1", authored on an element INSIDE the sub-comp, still resolves
|
||||
// against the OUTER hf-intro (exitAt=10) — 10 + 1 = 11 — not the same-scope
|
||||
// inner hf-intro (exitAt=1, which would give 2). This pins current behavior;
|
||||
// it is a real authoring footgun, not a claim that it's the ideal semantics.
|
||||
expect(timings["hf-host/hf-outro"]).toMatchObject({ enterAt: 11, exitAt: 12 });
|
||||
});
|
||||
});
|
||||
|
||||
// ─── setElementTiming — sparse map + batched dispatch ────────────────────────
|
||||
|
||||
@@ -77,6 +77,8 @@ class CompositionImpl implements Composition {
|
||||
|
||||
/** Lazily-built element snapshot, invalidated on every mutation. */
|
||||
private elementsCache: ElementSnapshot[] | null = null;
|
||||
/** Lazily-built root snapshot (getRootElements), invalidated alongside elementsCache. */
|
||||
private rootsCache: ElementSnapshot[] | null = null;
|
||||
|
||||
private currentSelection: string[] = [];
|
||||
|
||||
@@ -183,8 +185,24 @@ class CompositionImpl implements Composition {
|
||||
// resolve to 0 (parseFloat("intro + 2") is NaN). Node-safe static counterpart of the
|
||||
// runtime's own resolver (runtime/startResolver.ts): no live GSAP timeline to fall back
|
||||
// on, so an unauthored sub-composition duration still resolves to 0, same as before.
|
||||
//
|
||||
// refId is always a BARE id (the reference grammar has no scope syntax), resolved via
|
||||
// resolveScoped's bare-id rule: prefer the canonical top-level match, else document
|
||||
// order. An element inside a sub-composition referencing a bare id that also exists at
|
||||
// the top level resolves to the TOP-LEVEL one, not a same-scope sibling — this matches
|
||||
// the runtime's own resolver (also a global, not scope-aware, lookup), so the two stay
|
||||
// consistent, but it means a bare-id collision across scopes is a real footgun for
|
||||
// authored content.
|
||||
const startCache = new Map<Element, number>();
|
||||
const visiting = new Set<Element>();
|
||||
// Split out of resolveStart so its own branching stays low — this is the ONE
|
||||
// path that recurses + calls resolveDuration, kept here so that's visible at a
|
||||
// glance rather than buried inside resolveStart's try block.
|
||||
const resolveReferenceStart = (refId: string, offset: number): number => {
|
||||
const target = resolveScoped(this.parsed.document, refId);
|
||||
if (!target) return 0;
|
||||
return Math.max(0, resolveStart(target) + (resolveDuration(target) ?? 0) + offset);
|
||||
};
|
||||
const resolveStart = (el: Element): number => {
|
||||
const cached = startCache.get(el);
|
||||
if (cached !== undefined) return cached;
|
||||
@@ -195,10 +213,9 @@ class CompositionImpl implements Composition {
|
||||
const startStr = el.getAttribute("data-start");
|
||||
const expr = parseStartExpression(startStr);
|
||||
if (expr?.kind === "reference") {
|
||||
const target = resolveScoped(this.parsed.document, expr.refId);
|
||||
resolved = target
|
||||
? Math.max(0, resolveStart(target) + (resolveDuration(target) ?? 0) + expr.offset)
|
||||
: 0;
|
||||
resolved = resolveReferenceStart(expr.refId, expr.offset);
|
||||
} else if (expr?.kind === "absolute") {
|
||||
resolved = expr.value;
|
||||
} else {
|
||||
resolved = startStr !== null ? parseFloat(startStr) : 0;
|
||||
}
|
||||
@@ -344,9 +361,12 @@ class CompositionImpl implements Composition {
|
||||
* includes every descendant a second time as its own top-level entry, since each
|
||||
* snapshot in it still carries its children. `buildRoots` already computes true roots
|
||||
* internally for `getElements()` to flatten — this just returns them unflattened.
|
||||
* Cached like elementsCache — a layer panel calling this every render tick shouldn't
|
||||
* repay the DOM walk each time.
|
||||
*/
|
||||
getRootElements(): ElementSnapshot[] {
|
||||
return buildRoots(this.parsed.document);
|
||||
this.rootsCache ??= buildRoots(this.parsed.document);
|
||||
return [...this.rootsCache];
|
||||
}
|
||||
|
||||
getElement(id: HfId): ElementSnapshot | null {
|
||||
@@ -443,6 +463,7 @@ class CompositionImpl implements Composition {
|
||||
}
|
||||
|
||||
this.elementsCache = null;
|
||||
this.rootsCache = null;
|
||||
|
||||
// Update override-set from forward patches
|
||||
for (const p of forward) {
|
||||
@@ -547,6 +568,7 @@ class CompositionImpl implements Composition {
|
||||
applyPatchesToDocument(this.parsed, [...this.batchInverse].reverse());
|
||||
this.overrides = { ...this.batchOverridesSnapshot };
|
||||
this.elementsCache = null;
|
||||
this.rootsCache = null;
|
||||
}
|
||||
this.resetBatchState();
|
||||
// Empty no-op batch: fire changeHandlers (parity with dispatch)
|
||||
@@ -612,9 +634,10 @@ class CompositionImpl implements Composition {
|
||||
serialize(opts?: { stripRuntime?: boolean }): string {
|
||||
const html = serializeDocument(this.parsed);
|
||||
// Newer agent-generated compositions embed hyperframe.runtime.iife.js in their own
|
||||
// HTML. A host driving its own clock (an editing iframe) must not let that runtime
|
||||
// self-init — it races the host's first seek and resets the timeline to t=0. Opt-in
|
||||
// (default false) since a host playing the composition normally wants the runtime.
|
||||
// HTML. Any host driving its own clock (not just an editing iframe — anything that
|
||||
// owns seeking/playback itself) must not let that runtime self-init: it races the
|
||||
// host's first seek and resets the timeline to t=0. Opt-in (default false) since a
|
||||
// host playing the composition normally wants the runtime.
|
||||
return opts?.stripRuntime ? stripEmbeddedRuntimeScripts(html) : html;
|
||||
}
|
||||
|
||||
@@ -634,6 +657,7 @@ class CompositionImpl implements Composition {
|
||||
// Emit a patch event so subscribers stay in sync.
|
||||
applyPatchesToDocument(this.parsed, patches);
|
||||
this.elementsCache = null;
|
||||
this.rootsCache = null;
|
||||
|
||||
// Update override-set
|
||||
for (const p of patches) {
|
||||
|
||||
@@ -500,7 +500,8 @@ export interface Composition {
|
||||
on(event: "persist:error", handler: (event: PersistErrorEvent) => void): () => void;
|
||||
|
||||
// ── Serialization ──────────────────────────────────────────────────────────
|
||||
serialize(): string;
|
||||
/** stripRuntime removes an embedded preview-runtime script — for a host driving its own clock. */
|
||||
serialize(opts?: { stripRuntime?: boolean }): string;
|
||||
|
||||
// ── T3 embedded-mode extras ────────────────────────────────────────────────
|
||||
/** Current override-set — serialize for host storage */
|
||||
|
||||
Reference in New Issue
Block a user