diff --git a/packages/sdk/src/engine/apply-patches.ts b/packages/sdk/src/engine/apply-patches.ts index fdcb56621..56e0c3026 100644 --- a/packages/sdk/src/engine/apply-patches.ts +++ b/packages/sdk/src/engine/apply-patches.ts @@ -256,13 +256,19 @@ function applyOne(parsed: ParsedDocument, patch: JsonPatchOp, p: ParsedPath): vo if (patch.op === "remove") { removeVariableDecl(parsed.document, p.id); } else { - // Undo of removeVariable bundles {decl, index} to reinsert at the - // original array position; a plain declareVariable forward/replace - // patch carries the bare decl. Disambiguate on shape — VariableDecl's - // own index signature means "in" narrowing can't fully eliminate it - // from the union, so re-cast explicitly once shape is confirmed. + // Undo of removeVariable bundles {__kind: "reinsert", decl, index} to + // reinsert at the original array position; a plain declareVariable + // forward/replace patch carries the bare decl. Disambiguate on the + // __kind tag, not structural "decl"/"index" key presence — VariableDecl + // has an open index signature, so a genuine variable schema could + // legally declare its own "decl"/"index" fields, and "in" narrowing + // alone can't rule that out. const value = patch.value; - if (value && typeof value === "object" && "decl" in value && "index" in value) { + if ( + value && + typeof value === "object" && + (value as { __kind?: unknown }).__kind === "reinsert" + ) { const wrapped = value as { decl: VariableDecl; index: number }; declareVariableDecl(parsed.document, wrapped.decl, { atIndex: wrapped.index }); } else { diff --git a/packages/sdk/src/engine/mutate.test.ts b/packages/sdk/src/engine/mutate.test.ts index 76abad2af..e898a8cfd 100644 --- a/packages/sdk/src/engine/mutate.test.ts +++ b/packages/sdk/src/engine/mutate.test.ts @@ -999,6 +999,25 @@ describe("declareVariable", () => { applyPatchesToDocument(parsed, edited.inverse); expect(serializeDocument(parsed)).toBe(before); }); + + it("a decl whose own extension data includes 'decl'/'index' keys isn't mistaken for a wrapped reinsert", () => { + // VariableDecl has an open index signature, so a real, weird schema can + // legally carry fields named "decl"/"index" as ordinary extension data — + // this must NOT be confused with removeVariable's {__kind: "reinsert", + // decl, index} inverse-patch wrapper, which is disambiguated by __kind, + // not by structural key presence. + const parsed = fresh(); + const pathologicalDecl = { + id: "weird-var", + type: "string", + label: "Weird", + default: "x", + decl: "not-a-real-decl", + index: 999, + }; + applyOp(parsed, { type: "declareVariable", decl: pathologicalDecl }); + expect(readVarDecl(parsed, "weird-var")).toEqual(pathologicalDecl); + }); }); describe("removeVariable", () => { @@ -1129,6 +1148,29 @@ describe("validateOp", () => { it("returns ok:true for setCompositionMetadata (no target)", () => { expect(validateOp(fresh(), { type: "setCompositionMetadata", width: 100 }).ok).toBe(true); }); + + it("returns ok:true for declareVariable / removeVariable when a root exists", () => { + expect( + validateOp(fresh(), { + type: "declareVariable", + decl: { id: "v1", type: "string", label: "V1", default: "x" }, + }).ok, + ).toBe(true); + expect(validateOp(fresh(), { type: "removeVariable", id: "v1" }).ok).toBe(true); + }); + + it("returns ok:false / E_NO_ROOT for declareVariable / removeVariable with no root", () => { + const parsed = parseMutable(`no elements at all — just text`); + const r1 = validateOp(parsed, { + type: "declareVariable", + decl: { id: "v1", type: "string", label: "V1", default: "x" }, + }); + expect(r1.ok).toBe(false); + if (!r1.ok) expect(r1.code).toBe("E_NO_ROOT"); + const r2 = validateOp(parsed, { type: "removeVariable", id: "v1" }); + expect(r2.ok).toBe(false); + if (!r2.ok) expect(r2.code).toBe("E_NO_ROOT"); + }); }); // ─── Phase 3b ops — graceful when no GSAP script, feature-detectable ──────── diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index f7bb061ec..c9ad4f8e7 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -933,10 +933,13 @@ function handleRemoveVariable(parsed: ParsedDocument, id: string): MutationResul const path = variableDeclPath(id); // Bundle the original array index so undo reinserts at the same position // instead of appending — mirrors handleRemoveElement's {html, parentId, - // siblingIndex} inverse value. + // siblingIndex} inverse value. Tagged with __kind (rather than relying on + // structural "decl"/"index" key presence) because VariableDecl has an open + // index signature — a genuine variable schema could legally declare its own + // "decl"/"index" fields, which a structural check alone can't rule out. return { forward: [patchRemove(path)], - inverse: [patchAdd(path, { decl: removed.decl, index: removed.index })], + inverse: [patchAdd(path, { __kind: "reinsert", decl: removed.decl, index: removed.index })], }; } @@ -1546,6 +1549,8 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { return CAN_OK; } case "setVariableValue": + case "declareVariable": + case "removeVariable": if (findRoot(parsed.document) === null) return canErr("E_NO_ROOT", "Composition root element not found."); return CAN_OK; diff --git a/packages/sdk/src/session.test.ts b/packages/sdk/src/session.test.ts index 1a37b5cae..94550fcdd 100644 --- a/packages/sdk/src/session.test.ts +++ b/packages/sdk/src/session.test.ts @@ -625,4 +625,23 @@ describe("variable declarations (Composition API)", () => { { id: "brand-color", type: "color", label: "Brand color", default: "#0066cc" }, ]); }); + + it("declareVariable / removeVariable both support redo after undo", async () => { + const comp = await openComposition(VARIABLES_HTML); + + comp.declareVariable({ id: "brand-title", type: "string", label: "Title", default: "Hi" }); + comp.undo(); + comp.redo(); + expect(comp.listVariables()).toEqual([ + { id: "brand-color", type: "color", label: "Brand color", default: "#0066cc" }, + { id: "brand-title", type: "string", label: "Title", default: "Hi" }, + ]); + + comp.removeVariable("brand-color"); + comp.undo(); + comp.redo(); + expect(comp.listVariables()).toEqual([ + { id: "brand-title", type: "string", label: "Title", default: "Hi" }, + ]); + }); }); diff --git a/packages/sdk/src/session.ts b/packages/sdk/src/session.ts index 3e0c52869..5c9b432f1 100644 --- a/packages/sdk/src/session.ts +++ b/packages/sdk/src/session.ts @@ -155,8 +155,17 @@ class CompositionImpl implements Composition { this.dispatch({ type: "setVariableValue", id, value }); } - getVariableValue(id: string): unknown { - return readVariableDefault(this.parsed.document, id); + getVariableValue(id: string): string | number | boolean | FontValue | ImageValue | undefined { + // readVariableDefault genuinely can't narrow beyond unknown — the schema + // isn't validated at read time — so the cast lives here at the SDK + // boundary rather than pushing it onto every caller of getVariableValue. + return readVariableDefault(this.parsed.document, id) as + | string + | number + | boolean + | FontValue + | ImageValue + | undefined; } listVariables(): CompositionVariable[] { diff --git a/packages/sdk/src/types.ts b/packages/sdk/src/types.ts index 94ddd2c55..79b2dd6e5 100644 --- a/packages/sdk/src/types.ts +++ b/packages/sdk/src/types.ts @@ -410,7 +410,7 @@ export interface Composition { addElement(parent: HfId | null, index: number, html: string): HfId; setVariableValue(id: string, value: string | number | boolean | FontValue | ImageValue): void; /** Current `default` value for a declared variable, or undefined if undeclared/unset. */ - getVariableValue(id: string): unknown; + getVariableValue(id: string): string | number | boolean | FontValue | ImageValue | undefined; /** Every declared variable's full schema (id/type/label/default/…), or [] when none. */ listVariables(): CompositionVariable[]; /**