Merge pull request #4100 from heygen-com/sdk-cutover-decline-reason

feat(studio): split ineligible_operation into its four real causes

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

@@ -41,6 +41,10 @@ describe("SDK family gate mapping", () => {
         {} as never,
         deps,
       ),
-    ).resolves.toMatchObject({ status: "declined", reason: "ineligible_operation" });
+      // The dom family is off, so the gate declines on the FLAG, not on the op
+      // batch (which is a perfectly eligible inline-style edit). Before the
+      // reason split this reported `ineligible_operation`, which read as "the
+      // SDK cannot express this edit" when the truth was "this family is off".
+    ).resolves.toMatchObject({ status: "declined", reason: "feature_disabled" });
   });
 });

packages/studio/src/utils/sdkCutover.ts

@@ -7,8 +7,8 @@ import { patchOpsToSdkEditOps } from "./sdkOpMapping";
 import { recordResolverParity, recordAnimationResolverParity } from "./sdkResolverShadow";
 import {
   isResolverDisagreement,
+  sdkCutoverIneligibleReason,
   shouldDeclineTextCutoverForTarget,
-  shouldUseSdkCutover,
 } from "./sdkCutoverEligibility";
 import {
   asCutoverError,
@@ -81,11 +81,14 @@ export async function sdkCutoverPersist(
   deps: CutoverDeps,
   options?: CutoverOptions,
 ): Promise<CutoverResult> {
-  if (!shouldUseSdkCutover(sdkFamilyEnabled("dom"), !!sdkSession, selection.hfId, ops))
-    return declinedCutover("ineligible_operation", "dom");
+  // Name WHICH eligibility check failed: on v0.8.47 this one string covered 26 of
+  // 30 dom declines. The two below aren't properties of the batch, so they differ.
+  if (!sdkFamilyEnabled("dom")) return declinedCutover("feature_disabled", "dom");
   if (!sdkSession) return declinedCutover("session_unavailable", "dom");
   const hfId = selection.hfId;
-  if (!hfId) return declinedCutover("target_unaddressable", "dom");
+  const ineligible = sdkCutoverIneligibleReason(hfId, ops);
+  // `!hfId` is already a reason; testing it again narrows the type for the rest.
+  if (ineligible || !hfId) return declinedCutover(ineligible ?? "target_unaddressable", "dom");
   const target = sdkSession.getElement(hfId);
   if (!target)
     return declinedCutover("target_not_found", "dom", isResolverDisagreement(sdkSession, hfId));

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

@@ -1,6 +1,27 @@
 import { describe, expect, it } from "vitest";
 import type { PatchOperation } from "./sourcePatcher";
-import { shouldUseSdkCutover } from "./sdkCutoverEligibility";
+import { shouldUseSdkCutover, sdkCutoverIneligibleReason } from "./sdkCutoverEligibility";
+
+const styleOp = (property: string, value: string): PatchOperation => ({
+  type: "inline-style",
+  property,
+  value,
+});
+const textOp = (value: string): PatchOperation => ({
+  type: "text-content",
+  property: "text",
+  value,
+});
+const attrOp = (property: string, value: string): PatchOperation => ({
+  type: "attribute",
+  property,
+  value,
+});
+const htmlAttrOp = (property: string, value: string): PatchOperation => ({
+  type: "html-attribute",
+  property,
+  value,
+});
 
 const childStyleOp: PatchOperation = {
   type: "inline-style",
@@ -15,3 +36,77 @@ describe("shouldUseSdkCutover child-scoped operations", () => {
     expect(shouldUseSdkCutover(true, true, "hf-parent", [childStyleOp])).toBe(false);
   });
 });
+
+describe("sdkCutoverIneligibleReason", () => {
+  // `ineligible_operation` was one string covering four distinct causes, which
+  // made the post-flip decline telemetry unactionable: 26 of 30 declines on
+  // v0.8.47 landed there, with no way to separate a structural edit the SDK has
+  // no vocabulary for (expected, permanent) from a reserved-attribute or
+  // unsafe-attribute decline (narrower, and possibly fixable).
+  it("names an op type the SDK cannot express", () => {
+    expect(
+      sdkCutoverIneligibleReason("hf-abc", [
+        { type: "split", property: "x", value: "1" } as unknown as PatchOperation,
+      ]),
+    ).toBe("unsupported_op_type");
+  });
+
+  it("names a child-scoped op", () => {
+    expect(sdkCutoverIneligibleReason("hf-parent", [childStyleOp])).toBe("child_scoped_op");
+  });
+
+  it("names a reserved attribute", () => {
+    expect(sdkCutoverIneligibleReason("hf-abc", [attrOp("end", "2")])).toBe("reserved_attribute");
+    expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("DATA-START", "1")])).toBe(
+      "reserved_attribute",
+    );
+  });
+
+  it("names an unsafe html attribute", () => {
+    expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("onclick", "alert(1)")])).toBe(
+      "unsafe_html_attribute",
+    );
+    expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("href", "javascript:alert(1)")])).toBe(
+      "unsafe_html_attribute",
+    );
+  });
+
+  it("names an empty batch and an unaddressable target", () => {
+    expect(sdkCutoverIneligibleReason("hf-abc", [])).toBe("no_ops");
+    expect(sdkCutoverIneligibleReason(null, [styleOp("color", "red")])).toBe(
+      "target_unaddressable",
+    );
+  });
+
+  it("returns null when the batch is eligible", () => {
+    expect(sdkCutoverIneligibleReason("hf-abc", [styleOp("color", "red")])).toBeNull();
+    expect(
+      sdkCutoverIneligibleReason("hf-abc", [
+        styleOp("color", "red"),
+        textOp("hello"),
+        attrOp("x", "1"),
+        htmlAttrOp("class", "foo"),
+      ]),
+    ).toBeNull();
+  });
+
+  // The reason function must not drift from the predicate it explains: a batch
+  // the gate accepts must have no reason, and one it declines must have one.
+  it("agrees with shouldUseSdkCutover on every case", () => {
+    const cases: PatchOperation[][] = [
+      [styleOp("color", "red")],
+      [textOp("hello")],
+      [attrOp("x", "1")],
+      [htmlAttrOp("class", "foo")],
+      [attrOp("end", "2")],
+      [htmlAttrOp("onclick", "alert(1)")],
+      [childStyleOp],
+      [{ type: "split", property: "x", value: "1" } as unknown as PatchOperation],
+      [],
+    ];
+    for (const ops of cases) {
+      const eligible = shouldUseSdkCutover(true, true, "hf-abc", ops);
+      expect(sdkCutoverIneligibleReason("hf-abc", ops) === null).toBe(eligible);
+    }
+  });
+});

packages/studio/src/utils/sdkCutoverEligibility.ts

@@ -123,21 +123,44 @@ export function isResolverDisagreement(session: Composition, hfId: string): bool
   }
 }
 
+/**
+ * Why a batch cannot take the SDK cutover path, or null when it can.
+ *
+ * `ineligible_operation` used to be a single decline reason covering all of
+ * these, which made the post-flip telemetry unactionable — a structural edit the
+ * SDK has no vocabulary for (expected and permanent) was indistinguishable from
+ * a reserved-attribute decline (narrow, and possibly worth fixing). The order
+ * mirrors `shouldUseSdkCutover`'s conjunction, so the first failing check names
+ * the reason.
+ */
+export type SdkCutoverIneligibleReason =
+  | "target_unaddressable"
+  | "no_ops"
+  | "unsupported_op_type"
+  | "child_scoped_op"
+  | "reserved_attribute"
+  | "unsafe_html_attribute";
+
+export function sdkCutoverIneligibleReason(
+  hfId: string | null | undefined,
+  ops: PatchOperation[],
+): SdkCutoverIneligibleReason | null {
+  if (!hfId) return "target_unaddressable";
+  if (ops.length === 0) return "no_ops";
+  if (!ops.every((o) => CUTOVER_OP_TYPES.has(o.type))) return "unsupported_op_type";
+  if (hasChildScopedOp(ops)) return "child_scoped_op";
+  if (ops.some(mapsToReservedAttr)) return "reserved_attribute";
+  if (hasUnsafeHtmlAttributeOp(ops)) return "unsafe_html_attribute";
+  return null;
+}
+
 export function shouldUseSdkCutover(
   flagEnabled: boolean,
   hasSession: boolean,
   hfId: string | null | undefined,
   ops: PatchOperation[],
 ): boolean {
-  return (
-    flagEnabled &&
-    hasSession &&
-    !!hfId &&
-    ops.length > 0 &&
-    ops.every((o) => CUTOVER_OP_TYPES.has(o.type)) &&
-    // SDK edit ops target only the element hfId; child-scoped patch ops need the server path.
-    !hasChildScopedOp(ops) &&
-    !ops.some(mapsToReservedAttr) &&
-    !hasUnsafeHtmlAttributeOp(ops)
-  );
+  // Derived from the reason function so the two can never disagree about which
+  // batches are eligible.
+  return flagEnabled && hasSession && sdkCutoverIneligibleReason(hfId, ops) === null;
 }