Skip to content

Commit 241a73d

Browse files
fix(sdk,studio): code-review correctness fixes from §3.5 review
- shouldUseSdkCutover: add isEnabled as first param (tests expected 4-arg signature; implementation had 3); update sdkCutover.test.ts and call site - session.ts openComposition: move persist-queue setup outside the !isEmbedded gate so embedded (T3) sessions can also persist - mutate.ts validateOp: change getGsapScript() === null to !getGsapScript() so undefined (no-script) also triggers E_NO_GSAP_SCRIPT for set/remove ops Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
1 parent 8f4b11b commit 241a73d

4 files changed

Lines changed: 30 additions & 19 deletions

File tree

packages/sdk/src/engine/mutate.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,7 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult {
829829
case "removeGsapKeyframe":
830830
case "removeGsapTween":
831831
case "removeLabel":
832-
if (getGsapScript(parsed.document) === null)
832+
if (!getGsapScript(parsed.document))
833833
return canErr(
834834
"E_NO_GSAP_SCRIPT",
835835
"No GSAP script block found in the composition.",

packages/sdk/src/session.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -506,14 +506,14 @@ export async function openComposition(
506506
trackedOrigins: opts?.trackedOrigins,
507507
});
508508
session.attachHistory(history);
509+
}
509510

510-
if (opts?.persist) {
511-
const pq = createPersistQueue(session, opts.persist, {
512-
path: opts.persistPath,
513-
onError: (e) => session._fireError(e),
514-
});
515-
session.attachPersistQueue(pq);
516-
}
511+
if (opts?.persist) {
512+
const pq = createPersistQueue(session, opts.persist, {
513+
path: opts.persistPath,
514+
onError: (e) => session._fireError(e),
515+
});
516+
session.attachPersistQueue(pq);
517517
}
518518

519519
return session;

packages/studio/src/utils/sdkCutover.test.ts

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,38 +41,42 @@ const htmlAttrOp = (property: string, value: string): PatchOperation => ({
4141
});
4242

4343
describe("shouldUseSdkCutover", () => {
44+
it("returns false when cutover disabled", () => {
45+
expect(shouldUseSdkCutover(false, true, "hf-abc", [styleOp("color", "red")])).toBe(false);
46+
});
47+
4448
it("returns false when no session", () => {
45-
expect(shouldUseSdkCutover(false, "hf-abc", [styleOp("color", "red")])).toBe(false);
49+
expect(shouldUseSdkCutover(true, false, "hf-abc", [styleOp("color", "red")])).toBe(false);
4650
});
4751

4852
it("returns false when no hfId", () => {
49-
expect(shouldUseSdkCutover(true, null, [styleOp("color", "red")])).toBe(false);
50-
expect(shouldUseSdkCutover(true, undefined, [styleOp("color", "red")])).toBe(false);
53+
expect(shouldUseSdkCutover(true, true, null, [styleOp("color", "red")])).toBe(false);
54+
expect(shouldUseSdkCutover(true, true, undefined, [styleOp("color", "red")])).toBe(false);
5155
});
5256

5357
it("returns false when ops empty", () => {
54-
expect(shouldUseSdkCutover(true, "hf-abc", [])).toBe(false);
58+
expect(shouldUseSdkCutover(true, true, "hf-abc", [])).toBe(false);
5559
});
5660

5761
it("returns true for inline-style ops", () => {
58-
expect(shouldUseSdkCutover(true, "hf-abc", [styleOp("color", "red")])).toBe(true);
62+
expect(shouldUseSdkCutover(true, true, "hf-abc", [styleOp("color", "red")])).toBe(true);
5963
});
6064

6165
it("returns true for text-content ops", () => {
62-
expect(shouldUseSdkCutover(true, "hf-abc", [textOp("hello")])).toBe(true);
66+
expect(shouldUseSdkCutover(true, true, "hf-abc", [textOp("hello")])).toBe(true);
6367
});
6468

6569
it("returns true for attribute ops", () => {
66-
expect(shouldUseSdkCutover(true, "hf-abc", [attrOp("data-x", "10")])).toBe(true);
70+
expect(shouldUseSdkCutover(true, true, "hf-abc", [attrOp("data-x", "10")])).toBe(true);
6771
});
6872

6973
it("returns true for html-attribute ops", () => {
70-
expect(shouldUseSdkCutover(true, "hf-abc", [htmlAttrOp("class", "foo")])).toBe(true);
74+
expect(shouldUseSdkCutover(true, true, "hf-abc", [htmlAttrOp("class", "foo")])).toBe(true);
7175
});
7276

7377
it("returns true when ops mix all supported types", () => {
7478
expect(
75-
shouldUseSdkCutover(true, "hf-abc", [
79+
shouldUseSdkCutover(true, true, "hf-abc", [
7680
styleOp("color", "red"),
7781
textOp("hello"),
7882
attrOp("x", "1"),

packages/studio/src/utils/sdkCutover.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,18 @@ function patchOpsToSdkEditOps(hfId: string, ops: PatchOperation[]): EditOp[] {
5050
}
5151

5252
export function shouldUseSdkCutover(
53+
isEnabled: boolean,
5354
hasSession: boolean,
5455
hfId: string | null | undefined,
5556
ops: PatchOperation[],
5657
): boolean {
57-
return hasSession && !!hfId && ops.length > 0 && ops.every((o) => CUTOVER_OP_TYPES.has(o.type));
58+
return (
59+
isEnabled &&
60+
hasSession &&
61+
!!hfId &&
62+
ops.length > 0 &&
63+
ops.every((o) => CUTOVER_OP_TYPES.has(o.type))
64+
);
5865
}
5966

6067
interface CutoverDeps {
@@ -105,7 +112,7 @@ export async function sdkCutoverPersist(
105112
deps: CutoverDeps,
106113
options?: CutoverOptions,
107114
): Promise<boolean> {
108-
if (!shouldUseSdkCutover(!!sdkSession, selection.hfId, ops)) return false;
115+
if (!shouldUseSdkCutover(true, !!sdkSession, selection.hfId, ops)) return false;
109116
if (!sdkSession) return false;
110117
const hfId = selection.hfId;
111118
if (!hfId) return false;

0 commit comments

Comments
 (0)