diff --git a/packages/studio/src/utils/sdkCutover.familyGate.test.ts b/packages/studio/src/utils/sdkCutover.familyGate.test.ts index 2046998b06..f2dcca7091 100644 --- a/packages/studio/src/utils/sdkCutover.familyGate.test.ts +++ b/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" }); }); }); diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index faf33b4b1f..14d3a46cbe 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/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 { - 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)); diff --git a/packages/studio/src/utils/sdkCutoverEligibility.test.ts b/packages/studio/src/utils/sdkCutoverEligibility.test.ts index fadd02cf12..e3efd20f2f 100644 --- a/packages/studio/src/utils/sdkCutoverEligibility.test.ts +++ b/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); + } + }); +}); diff --git a/packages/studio/src/utils/sdkCutoverEligibility.ts b/packages/studio/src/utils/sdkCutoverEligibility.ts index 2dbc0aee63..1e57bc9dca 100644 --- a/packages/studio/src/utils/sdkCutoverEligibility.ts +++ b/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; }