From 05d5abb92613a9a6a5261149924bc605ecba3d29 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sun, 20 Sep 2026 00:42:37 -0700 Subject: [PATCH] feat(studio): a blocked GSAP edit records which of seven causes it hit (#4214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `edit_blocked` is the largest interaction failure in Studio: 575 users in 14 days, 17% of everyone who touches an editing control, and 41% of them are blocked on their first or second attempt. Only 30% ever commit anything afterwards. Almost all of it is one message. "This animation is computed at runtime. Edit the animation in the Code tab." covers ~460 of those users and is thrown from seven different places — runtime-dynamic provenance, unresolved keyframes, an unresolved selector, a geometry commit over unresolved source, and three live-tween-without-a-source-match cases across position and rotation. The telemetry carried only that string, so the classes were indistinguishable. The copy is also only literally true for the first. The rest are parser or source-matching limits where the animation may be plain authored source, and we send the user to the Code tab to rewrite something that could be fine. Adds a `detail` beside the coarse reason, set at each of the seven origins and emitted as `block_detail`. Telemetry only; nothing branches on it, and the user-facing copy is unchanged. Which cause dominates decides whether the fix is better copy, a parser gap, or an Unroll-style recovery path — and guessing that without the split is what this is meant to avoid. `directEditOutcomeForProperties` rebuilt blocked outcomes from `reason` alone, so it dropped the detail for the three causes that flow through it; it now carries it. Mutation testing caught that, not review. Co-authored-by: Claude Opus 5 (1M context) --- packages/studio/src/hooks/gsapDragCommit.ts | 2 +- .../studio/src/hooks/gsapEditOutcome.test.ts | 115 ++++++++++++++++++ packages/studio/src/hooks/gsapEditOutcome.ts | 49 ++++++-- .../src/hooks/gsapResizeIntercept.test.ts | 45 ++++++- .../studio/src/hooks/gsapResizeIntercept.ts | 14 ++- .../src/hooks/gsapRuntimeBridge.test.ts | 12 +- .../studio/src/hooks/gsapRuntimeBridge.ts | 17 ++- .../studio/src/utils/studioSaveDiagnostics.ts | 12 ++ 8 files changed, 245 insertions(+), 21 deletions(-) create mode 100644 packages/studio/src/hooks/gsapEditOutcome.test.ts diff --git a/packages/studio/src/hooks/gsapDragCommit.ts b/packages/studio/src/hooks/gsapDragCommit.ts index e6f4197987..d2d17c50b3 100644 --- a/packages/studio/src/hooks/gsapDragCommit.ts +++ b/packages/studio/src/hooks/gsapDragCommit.ts @@ -135,7 +135,7 @@ export async function materializeIfDynamic( void iframe; void commitMutation; void selection; - throw new GsapEditBlockedError("source-uneditable"); + throw new GsapEditBlockedError("source-uneditable", "geometry-unresolved-source"); } // ── Drag → GSAP position math ────────────────────────────────────────────── diff --git a/packages/studio/src/hooks/gsapEditOutcome.test.ts b/packages/studio/src/hooks/gsapEditOutcome.test.ts new file mode 100644 index 0000000000..12aa7ff158 --- /dev/null +++ b/packages/studio/src/hooks/gsapEditOutcome.test.ts @@ -0,0 +1,115 @@ +import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; +import { describe, expect, it } from "vitest"; +import { buildStudioSaveFailureProperties } from "../utils/studioSaveDiagnostics"; +import { + GsapEditBlockedError, + assertGsapEditPersisted, + directEditOutcomeForProperties, +} from "./gsapEditOutcome"; + +function blockDetailFor(error: unknown): unknown { + return buildStudioSaveFailureProperties({ source: "gsap_commit", error }).block_detail; +} + +/** Minimal animation that writes `x`, so the editability check actually runs. */ +function animation(overrides: Partial = {}): GsapAnimation { + return { + id: "a1", + targetSelector: "#box", + method: "to", + position: 0, + properties: { x: 100 }, + ...overrides, + } as GsapAnimation; +} + +const POSITION = new Set(["x"]); + +describe("a blocked GSAP edit names which of the nine causes it hit", () => { + // The three causes inside assertGsapAnimationDirectlyEditable share one + // message, so only the detail distinguishes them. Asserted through the + // exported wrapper because that is the path a real edit takes — and the + // wrapper used to rebuild the outcome from `reason` alone, dropping it. + it("reports a runtime-dynamic provenance", () => { + const outcome = directEditOutcomeForProperties( + [animation({ provenance: { kind: "runtime-dynamic" } })], + POSITION, + ); + expect(outcome).toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "provenance-runtime-dynamic", + }); + }); + + it("reports unresolved keyframes separately from provenance", () => { + const outcome = directEditOutcomeForProperties( + [animation({ hasUnresolvedKeyframes: true })], + POSITION, + ); + expect(outcome).toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "unresolved-keyframes", + }); + }); + + it("reports an unresolved selector separately from provenance", () => { + const outcome = directEditOutcomeForProperties( + [animation({ hasUnresolvedSelector: true })], + POSITION, + ); + expect(outcome).toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "unresolved-selector", + }); + }); + + it("still routes a helper or loop to unroll, which has its own message", () => { + const outcome = directEditOutcomeForProperties( + [animation({ provenance: { kind: "loop" } })], + POSITION, + ); + expect(outcome).toEqual({ status: "blocked", reason: "unroll-required", detail: undefined }); + }); + + it("leaves a literal tween editable", () => { + expect(directEditOutcomeForProperties([animation()], POSITION)).toEqual({ + status: "persisted", + }); + }); + + it("carries the detail through a returned outcome, not just a thrown one", () => { + // gsapRuntimeBridge returns blocked outcomes rather than throwing. + expect(() => + assertGsapEditPersisted({ + status: "blocked", + reason: "source-uneditable", + detail: "no-position-tween", + }), + ).toThrow( + expect.objectContaining({ name: "GsapEditBlockedError", detail: "no-position-tween" }), + ); + }); + + it("surfaces the detail on the telemetry payload", () => { + expect( + blockDetailFor( + new GsapEditBlockedError("source-uneditable", "live-rotation-no-source-tween"), + ), + ).toBe("live-rotation-no-source-tween"); + }); + + it("omits the detail for a block that has no specific cause", () => { + expect(blockDetailFor(new GsapEditBlockedError("no-selector"))).toBeUndefined(); + }); + + it("ignores a detail-shaped field on an error that is not a block", () => { + // Guards the name check: any error can carry a `detail`, and reporting a + // save failure's detail as a block cause would be a silent mislabel. + const impostor = Object.assign(new Error("network down"), { detail: "unresolved-selector" }); + expect(blockDetailFor(impostor)).toBeUndefined(); + expect(blockDetailFor("not an error")).toBeUndefined(); + }); +}); diff --git a/packages/studio/src/hooks/gsapEditOutcome.ts b/packages/studio/src/hooks/gsapEditOutcome.ts index 9ec2dddd1e..71f4cf0ab7 100644 --- a/packages/studio/src/hooks/gsapEditOutcome.ts +++ b/packages/studio/src/hooks/gsapEditOutcome.ts @@ -2,6 +2,27 @@ import { editabilityForProvenance, type GsapAnimation } from "@hyperframes/core/ export type GsapEditBlockReason = "no-selector" | "unroll-required" | "source-uneditable"; +/** + * Which of the nine situations produced a block. The user-facing `reason` stays + * coarse — three messages — but "source-uneditable" alone covers nine distinct + * causes, and `edit_blocked` telemetry could not tell them apart. That matters + * because the copy ("This animation is computed at runtime") is only literally + * true for `provenance-runtime-dynamic`; the others are parser or source-match + * limits, where the animation may well be plain authored source. + * + * Telemetry only. Nothing branches on it. + */ +export type GsapEditBlockDetail = + | "provenance-runtime-dynamic" + | "unresolved-keyframes" + | "unresolved-selector" + | "geometry-unresolved-source" + | "live-position-no-source-tween" + | "no-position-tween" + | "live-rotation-no-source-tween" + | "live-resize-no-source-tween" + | "zero-duration-tween"; + export type GsapEditOutcome = | { status: "persisted"; @@ -22,7 +43,7 @@ export type GsapEditOutcome = */ ownsDragOffset?: boolean; } - | { status: "blocked"; reason: GsapEditBlockReason }; + | { status: "blocked"; reason: GsapEditBlockReason; detail?: GsapEditBlockDetail }; const COPY: Record = { "no-selector": "This layer needs a stable selector before Studio can save the edit.", @@ -32,25 +53,32 @@ const COPY: Record = { }; export class GsapEditBlockedError extends Error { - constructor(readonly reason: GsapEditBlockReason) { + constructor( + readonly reason: GsapEditBlockReason, + readonly detail?: GsapEditBlockDetail, + ) { super(COPY[reason]); this.name = "GsapEditBlockedError"; } } export function assertGsapEditPersisted(outcome: GsapEditOutcome): void { - if (outcome.status === "blocked") throw new GsapEditBlockedError(outcome.reason); + if (outcome.status === "blocked") throw new GsapEditBlockedError(outcome.reason, outcome.detail); } function assertGsapAnimationDirectlyEditable(animation: GsapAnimation): void { const editability = editabilityForProvenance(animation.provenance); if (editability === "unroll") throw new GsapEditBlockedError("unroll-required"); - if ( - editability === "source" || - animation.hasUnresolvedKeyframes || - animation.hasUnresolvedSelector - ) { - throw new GsapEditBlockedError("source-uneditable"); + // Same message for all three, but they are different problems: only the first + // is genuinely a runtime-computed value. + if (editability === "source") { + throw new GsapEditBlockedError("source-uneditable", "provenance-runtime-dynamic"); + } + if (animation.hasUnresolvedKeyframes) { + throw new GsapEditBlockedError("source-uneditable", "unresolved-keyframes"); + } + if (animation.hasUnresolvedSelector) { + throw new GsapEditBlockedError("source-uneditable", "unresolved-selector"); } } @@ -84,7 +112,8 @@ export function directEditOutcomeForProperties( } return { status: "persisted" }; } catch (error) { - if (isGsapEditBlockedError(error)) return { status: "blocked", reason: error.reason }; + if (isGsapEditBlockedError(error)) + return { status: "blocked", reason: error.reason, detail: error.detail }; throw error; } } diff --git a/packages/studio/src/hooks/gsapResizeIntercept.test.ts b/packages/studio/src/hooks/gsapResizeIntercept.test.ts index 4045b28c8a..2037c7f19f 100644 --- a/packages/studio/src/hooks/gsapResizeIntercept.test.ts +++ b/packages/studio/src/hooks/gsapResizeIntercept.test.ts @@ -180,11 +180,15 @@ it("reuses the ownership parse instead of fetching a resolved size group twice", ); }); -it("blocks when runtime size motion exists but the authored tween cannot be resolved", async () => { +function makeBoxSelection(): { el: HTMLElement; selection: DomEditSelection } { const el = document.createElement("div"); el.id = "box"; document.body.append(el); - const selection = { id: "box", selector: "#box", element: el } as DomEditSelection; + return { el, selection: { id: "box", selector: "#box", element: el } as DomEditSelection }; +} + +it("blocks when runtime size motion exists but the authored tween cannot be resolved", async () => { + const { el, selection } = makeBoxSelection(); const liveSizeTween = { targets: () => [el], vars: { width: 300, duration: 1 }, @@ -199,7 +203,42 @@ it("blocks when runtime size motion exists but the authored tween cannot be reso await expect( tryGsapResizeIntercept(selection, { width: 344, height: 344 }, [], iframe, commitMutation), - ).resolves.toEqual({ status: "blocked", reason: "source-uneditable" }); + ).resolves.toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "live-resize-no-source-tween", + }); + expect(commitMutation).not.toHaveBeenCalled(); +}); + +it("blocks a tween with no positive duration instead of committing into it", async () => { + const { selection } = makeBoxSelection(); + // A zero-duration `from` is not an instant hold (those are `set`/`to`), so it + // reaches the duration check: a resize has no timeline position to land on. + const zeroDurationTween = { + id: "#box-size", + targetSelector: "#box", + propertyGroup: "size", + method: "from", + position: 0, + duration: 0, + properties: { width: 150, height: 150 }, + } as unknown as GsapAnimation; + const commitMutation = vi.fn(); + + await expect( + tryGsapResizeIntercept( + selection, + { width: 344, height: 344 }, + [zeroDurationTween], + null, + commitMutation, + ), + ).resolves.toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "zero-duration-tween", + }); expect(commitMutation).not.toHaveBeenCalled(); }); diff --git a/packages/studio/src/hooks/gsapResizeIntercept.ts b/packages/studio/src/hooks/gsapResizeIntercept.ts index 71b1adfba4..9353213dba 100644 --- a/packages/studio/src/hooks/gsapResizeIntercept.ts +++ b/packages/studio/src/hooks/gsapResizeIntercept.ts @@ -137,7 +137,12 @@ export async function tryGsapResizeIntercept( ? hasNonHoldTweenForElement(iframe, liveSelector, undefined, [...resizeProperties]) : false; if (!anim && hasLiveResizeTween) { - return { status: "blocked", reason: "source-uneditable" }; + // Third twin of the position/rotation cases: a live tween with no source match. + return { + status: "blocked", + reason: "source-uneditable", + detail: "live-resize-no-source-tween", + }; } logResize("intercept-enter", { hasScaleGroup, @@ -194,7 +199,12 @@ export async function tryGsapResizeIntercept( } const tweenDuration = resolveTweenDuration(anim); - if (tweenDuration <= 0) return { status: "blocked", reason: "source-uneditable" }; + if (tweenDuration <= 0) { + // The tween exists in source but has no positive duration, so there is no + // timeline position at which a resize could land — a different cause from + // the live-tween-without-a-source-match cases above. + return { status: "blocked", reason: "source-uneditable", detail: "zero-duration-tween" }; + } const { activeKeyframePct, setActiveKeyframePct } = usePlayerStore.getState(); const pct = activeKeyframePct ?? computeCurrentPercentage(selection, anim); diff --git a/packages/studio/src/hooks/gsapRuntimeBridge.test.ts b/packages/studio/src/hooks/gsapRuntimeBridge.test.ts index bc79366dfc..62d3cbfa91 100644 --- a/packages/studio/src/hooks/gsapRuntimeBridge.test.ts +++ b/packages/studio/src/hooks/gsapRuntimeBridge.test.ts @@ -94,7 +94,11 @@ describe("tryGsapDragIntercept — stale-parse guard (no resurrection after dele vi.fn().mockResolvedValue([]), ); - expect(result).toEqual({ status: "blocked", reason: "source-uneditable" }); + expect(result).toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "live-position-no-source-tween", + }); expect(commitMutation).not.toHaveBeenCalled(); }); @@ -354,7 +358,11 @@ describe("tryGsapRotationIntercept — instant holds", () => { fakeIframe("puck-b", [liveRotation]), commitMutation, ), - ).resolves.toEqual({ status: "blocked", reason: "source-uneditable" }); + ).resolves.toEqual({ + status: "blocked", + reason: "source-uneditable", + detail: "live-rotation-no-source-tween", + }); expect(commitMutation).not.toHaveBeenCalled(); }); diff --git a/packages/studio/src/hooks/gsapRuntimeBridge.ts b/packages/studio/src/hooks/gsapRuntimeBridge.ts index a35fb6d3f1..e66ed6335a 100644 --- a/packages/studio/src/hooks/gsapRuntimeBridge.ts +++ b/packages/studio/src/hooks/gsapRuntimeBridge.ts @@ -156,7 +156,13 @@ async function preflightGsapDragIntercept( const hasLivePosition = hasNonHoldTweenForElement(iframe, selector, undefined, POSITION_CHANNELS); if (hasLivePosition && !posAnim) { - return { status: "blocked", reason: "source-uneditable" }; + // GSAP is visibly moving this element but the parser found no position + // tween for it — a source-match gap, not necessarily computed source. + return { + status: "blocked", + reason: "source-uneditable", + detail: "live-position-no-source-tween", + }; } if (!posAnim && !writeTargetSelector(selection)) { return { status: "blocked", reason: "no-selector" }; @@ -268,7 +274,7 @@ export async function tryGsapDragIntercept( } if (!posAnim) { - return { status: "blocked", reason: "source-uneditable" }; + return { status: "blocked", reason: "source-uneditable", detail: "no-position-tween" }; } // Verify the anim ID is still valid in the current file. The React-state @@ -353,7 +359,12 @@ export async function tryGsapRotationIntercept( ? hasNonHoldTweenForElement(iframe, liveSelector, undefined, ROTATION_CHANNELS) : false; if (!anim && hasLiveRotationTween) { - return { status: "blocked", reason: "source-uneditable" }; + // Rotation twin of the position case above: live tween, no source match. + return { + status: "blocked", + reason: "source-uneditable", + detail: "live-rotation-no-source-tween", + }; } // `angle` is the ABSOLUTE target rotation resolved by the gesture (gsap base + diff --git a/packages/studio/src/utils/studioSaveDiagnostics.ts b/packages/studio/src/utils/studioSaveDiagnostics.ts index 865723193c..f4a8c3ebf0 100644 --- a/packages/studio/src/utils/studioSaveDiagnostics.ts +++ b/packages/studio/src/utils/studioSaveDiagnostics.ts @@ -165,9 +165,21 @@ export function buildStudioSaveFailureProperties( target_id: input.targetId ?? undefined, target_selector: input.targetSelector ?? undefined, target_source_file: input.targetSourceFile ?? undefined, + block_detail: gsapBlockDetail(input.error), }; } +/** + * The specific cause behind a blocked GSAP edit. `error_message` only carries + * the coarse copy, and one of those strings covers six different situations, + * so the message alone cannot say which one a user hit. + */ +function gsapBlockDetail(error: unknown): string | undefined { + if (!(error instanceof Error) || error.name !== "GsapEditBlockedError") return undefined; + const detail = (error as { detail?: unknown }).detail; + return typeof detail === "string" ? detail : undefined; +} + export function trackStudioSaveFailure(input: StudioSaveFailureInput): void { trackStudioEvent("save_failure", buildStudioSaveFailureProperties(input)); }