From 3c7400af890aae0140f7bf577bd002eb05050319 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 27 Jul 2026 19:02:21 +0200 Subject: [PATCH] fix(studio): close the review findings in this PR instead of at the stack tip The R1/R3 residuals on this PR were fixed at the top of the stack, so they only cleared once every branch above landed. They belong here, next to the code they correct: - `idFromSelector` inverts `idSelector` for both regex readers, so the post-commit cache refresh stops skipping the CSS-unsafe ids `idSelector` exists to support. - `deduplicateKeyframes` drops `ease` when it is ambiguous; the flag was the only honest answer and the last-writer-wins curve belonged to an arbitrary colliding tween. - `isStaticPositionHold` is now the single owner of the hold skip. The `sourceAnimations` filter and the `allKeyframes` filter had diverged on whether `immediateRender` counts as a property. - The keyframe-cache setters no-op when the write changes nothing, instead of handing every subscriber a fresh Map. - `reset()` clears `focusedEaseSegment`. - The test hook `delete`s its window key rather than setting it to undefined, so feature detection still works. - The `toClipKeyframes` fixture uses `as unknown as T` with the justification CONTRIBUTING.md asks for. --- .../src/hooks/gsapKeyframeCacheHelpers.ts | 7 ++--- packages/studio/src/hooks/gsapShared.test.ts | 22 ++++++++++++-- packages/studio/src/hooks/gsapShared.ts | 21 +++++++++++++ packages/studio/src/hooks/gsapTweenSynth.ts | 26 +++++++++++++++- .../studio/src/hooks/useGsapTweenCache.ts | 30 +++++-------------- .../studio/src/hooks/useStudioTestHooks.ts | 4 ++- .../studio/src/player/store/keyframeSlice.ts | 13 ++++++++ .../studio/src/player/store/playerStore.ts | 1 + 8 files changed, 93 insertions(+), 31 deletions(-) diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts index f9953f9fc..1e806a029 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts @@ -4,7 +4,7 @@ */ import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore"; -import { toClipKeyframes } from "./gsapShared"; +import { idFromSelector, toClipKeyframes } from "./gsapShared"; import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; export function updateKeyframeCacheFromParsed( @@ -18,7 +18,7 @@ export function updateKeyframeCacheFromParsed( const merged = new Map(); const sourceAnimations = new Map(); for (const anim of animations) { - const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1]; + const id = idFromSelector(anim.targetSelector); const kfSource = anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? []; if (!id || kfSource.length === 0) continue; @@ -61,8 +61,7 @@ export function updateKeyframeCacheFromParsed( writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id)); } const targetId = - (mutation as { targetSelector?: string }).targetSelector?.match(/^#([\w-]+)/)?.[1] ?? - selectionId; + idFromSelector((mutation as { targetSelector?: string }).targetSelector) ?? selectionId; if (targetId && !idsWithKeyframes.has(targetId)) { clearKeyframeCacheForElement(targetPath, targetId); } diff --git a/packages/studio/src/hooks/gsapShared.test.ts b/packages/studio/src/hooks/gsapShared.test.ts index 144dcfb7c..0fb17bfb7 100644 --- a/packages/studio/src/hooks/gsapShared.test.ts +++ b/packages/studio/src/hooks/gsapShared.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { + idFromSelector, idSelector, isInstantHold, parsePercentageKeyframes, @@ -125,13 +126,16 @@ describe("toClipPercentage", () => { }); describe("toClipKeyframes", () => { - const durationless: GsapAnimation = { + // Fixture carries only the fields the function under test reads; the + // double-cast is the documented way to stand in for the full runtime shape + // (CONTRIBUTING.md). + const durationless = { id: "a1", method: "to", targetSelector: "#box", vars: {}, resolvedStart: 0, - } as GsapAnimation; + } as unknown as GsapAnimation; // A tween with no duration spans its clip everywhere else in Studio // (resolveEditableTweenDuration), so the cache rows have to agree: a fixed 1s @@ -146,3 +150,17 @@ describe("toClipKeyframes", () => { expect(rows[0]).toMatchObject({ tweenPercentage: 50, animationId: "a1" }); }); }); + +describe("idFromSelector", () => { + it("round-trips every shape idSelector emits", () => { + for (const id of ["hero-word", "el_1", "01-hook-hero-word", "my.class", "1box", '1"x']) { + expect(idFromSelector(idSelector(id))).toBe(id); + } + }); + + it("returns null for a selector that does not address an id", () => { + expect(idFromSelector(".dot")).toBeNull(); + expect(idFromSelector("[data-hf-id='x']")).toBeNull(); + expect(idFromSelector(undefined)).toBeNull(); + }); +}); diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index 4780d7407..906b98419 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -80,6 +80,27 @@ export function idSelector(id: string): string { return SAFE_HASH_ID.test(id) ? `#${id}` : `[id="${id.replace(/(["\\])/g, "\\$1")}"]`; } +/** + * Inverse of {@link idSelector}: the element id a target selector addresses, or + * null for a selector that is not id-based (a class, a tag, a descendant path). + * + * Both shapes have to be read back, not just `#id`. Every writer emits through + * `idSelector`, so a digit-leading, dotted or otherwise CSS-unsafe id lands in + * the source as `[id="01-hook-hero"]`. A reader that only matched `#id` saw no + * id at all for those elements and skipped them — which is how the post-commit + * keyframe-cache refresh silently stopped running for exactly the ids + * `idSelector` was added to support. + */ +export function idFromSelector(selector: string | undefined | null): string | null { + if (!selector) return null; + const hash = selector.match(/^#([\w-]+)/); + if (hash) return hash[1] ?? null; + const attribute = selector.match(/^\[id="((?:\\.|[^"\\])*)"\]/); + if (!attribute) return null; + // Undo the quote/backslash escaping idSelector applies. + return (attribute[1] ?? "").replace(/\\(["\\])/g, "$1"); +} + export function selectorFromSelection(selection: DomEditSelection): string | null { if (selection.id) return idSelector(selection.id); if (selection.selector) return selection.selector; diff --git a/packages/studio/src/hooks/gsapTweenSynth.ts b/packages/studio/src/hooks/gsapTweenSynth.ts index 20daf0812..a3b100be4 100644 --- a/packages/studio/src/hooks/gsapTweenSynth.ts +++ b/packages/studio/src/hooks/gsapTweenSynth.ts @@ -5,6 +5,23 @@ import type { } from "@hyperframes/core/gsap-parser"; import { PROPERTY_DEFAULTS } from "./gsapShared"; +/** + * A static position hold (only x/y, no real motion) is a `set`, not a keyframe — + * it must not synthesize a diamond. Covers both `tl.set(...)` and the + * `tl.to({ duration: 0, immediateRender: true })` hold that remove-all-keyframes + * collapses to (otherwise shown as a stray 0% keyframe). + * + * Single owner: the collapsed keyframe cache and the expanded property lanes' + * `gsapAnimations` map MUST agree on it, or a hold draws a phantom expanded lane + * with no matching collapsed diamond. + */ +export function isStaticPositionHold(anim: GsapAnimation): boolean { + if (anim.keyframes) return false; + if (anim.method !== "set" && (anim.duration ?? 0) !== 0) return false; + const propKeys = Object.keys(anim.properties).filter((k) => k !== "immediateRender"); + return propKeys.length > 0 && propKeys.every((k) => k === "x" || k === "y"); +} + export function deduplicateKeyframes< T extends GsapPercentageKeyframe & { animationId?: string; easeAmbiguous?: boolean }, >(keyframes: T[]): T[] { @@ -25,7 +42,14 @@ export function deduplicateKeyframes< ) { existing.easeAmbiguous = true; } - if (kf.ease) existing.ease = kf.ease; + // Whichever tween iterated last used to win `ease`, so the merged + // keyframe carried an arbitrary one of the colliding curves. Readers that + // do not check easeAmbiguous (drag readouts, lane hints) then showed a + // curve belonging to a different animation than the one an edit targets. + // Drop it instead: ambiguous means "no single ease", and the flag is the + // only honest answer. + if (existing.easeAmbiguous) delete existing.ease; + else if (kf.ease) existing.ease = kf.ease; } else { byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } }); } diff --git a/packages/studio/src/hooks/useGsapTweenCache.ts b/packages/studio/src/hooks/useGsapTweenCache.ts index 766b0f39c..c1ae295b4 100644 --- a/packages/studio/src/hooks/useGsapTweenCache.ts +++ b/packages/studio/src/hooks/useGsapTweenCache.ts @@ -9,7 +9,11 @@ import { writeGsapAnimationsForElement, } from "./gsapKeyframeCacheHelpers"; import { toAbsoluteTime, toClipPercentage, toClipKeyframes } from "./gsapShared"; -import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; +import { + deduplicateKeyframes, + isStaticPositionHold, + synthesizeFlatTweenKeyframes, +} from "./gsapTweenSynth"; function extractIdFromSelector(selector: string): string | null { const match = selector.match(/^#([\w-]+)/); @@ -357,18 +361,7 @@ export function useGsapAnimationsForElement( let ease: string | undefined; let easeEach: string | undefined; for (const anim of animations) { - // A static position hold (only x/y, no real motion) is a `set`, not a - // keyframe — don't synthesize a diamond for it. Covers both `tl.set(...)` - // and the `tl.to({ duration: 0, immediateRender: true })` hold that - // remove-all-keyframes collapses to (which is otherwise shown as a stray - // 0% keyframe). - if ( - !anim.keyframes && - Object.keys(anim.properties).length > 0 && - Object.keys(anim.properties).every((k) => k === "x" || k === "y") && - (anim.method === "set" || (anim.duration ?? 0) === 0) - ) - continue; + if (isStaticPositionHold(anim)) continue; const kf = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim); if (!kf) continue; // Convert tween-relative percentages to clip-relative so diamonds @@ -469,16 +462,7 @@ export function usePopulateKeyframeCacheForFile( const sourceByElement = new Map(); for (const anim of parsed.animations) { if (anim.hasUnresolvedKeyframes) continue; - // Position-only static holds are not keyframed animations — skip them so - // they don't draw a timeline diamond. Covers both a `tl.set(...)` and the - // `tl.to({ duration: 0, immediateRender: true })` that remove-all-keyframes - // collapses a keyframed tween to. - if (!anim.keyframes && (anim.method === "set" || (anim.duration ?? 0) === 0)) { - const propKeys = Object.keys(anim.properties).filter((k) => k !== "immediateRender"); - if (propKeys.length > 0 && propKeys.every((k) => k === "x" || k === "y")) { - continue; - } - } + if (isStaticPositionHold(anim)) continue; const kfData = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim); if (!kfData) continue; // Attribute the tween to every element it animates (handles class / diff --git a/packages/studio/src/hooks/useStudioTestHooks.ts b/packages/studio/src/hooks/useStudioTestHooks.ts index a00e01a45..3453d0be8 100644 --- a/packages/studio/src/hooks/useStudioTestHooks.ts +++ b/packages/studio/src/hooks/useStudioTestHooks.ts @@ -45,7 +45,9 @@ export function useStudioTestHooks({ }; (window as unknown as { __studioTest?: typeof api }).__studioTest = api; return () => { - (window as unknown as { __studioTest?: typeof api }).__studioTest = undefined; + // delete, not `= undefined`: an own key holding undefined keeps + // `"__studioTest" in window` true, which defeats feature detection. + delete (window as unknown as { __studioTest?: typeof api }).__studioTest; }; }, [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef]); } diff --git a/packages/studio/src/player/store/keyframeSlice.ts b/packages/studio/src/player/store/keyframeSlice.ts index f219983bc..7a97d986c 100644 --- a/packages/studio/src/player/store/keyframeSlice.ts +++ b/packages/studio/src/player/store/keyframeSlice.ts @@ -92,6 +92,13 @@ export function createKeyframeSlice(set: StoreApi["setState"]): K keyframeCache: new Map(), setKeyframeCache: (elementId, data) => set((state) => { + // A write that changes nothing must not emit a new Map: the cache has + // several hot writers (per-element effect, file populate, post-commit + // updater, delete) and every no-op re-rendered every subscriber. + if ( + data ? state.keyframeCache.get(elementId) === data : !state.keyframeCache.has(elementId) + ) + return state; const next = new Map(state.keyframeCache); if (data) next.set(elementId, data); else next.delete(elementId); @@ -100,6 +107,12 @@ export function createKeyframeSlice(set: StoreApi["setState"]): K gsapAnimations: new Map(), setGsapAnimations: (elementId, animations) => set((state) => { + if ( + animations + ? state.gsapAnimations.get(elementId) === animations + : !state.gsapAnimations.has(elementId) + ) + return state; const next = new Map(state.gsapAnimations); if (animations) next.set(elementId, animations); else next.delete(elementId); diff --git a/packages/studio/src/player/store/playerStore.ts b/packages/studio/src/player/store/playerStore.ts index faf3776b5..e8c101c8f 100644 --- a/packages/studio/src/player/store/playerStore.ts +++ b/packages/studio/src/player/store/playerStore.ts @@ -531,6 +531,7 @@ export const usePlayerStore = create((set, get) => ({ activeTool: "select", selectedKeyframes: new Set(), expandedClipIds: new Set(), + focusedEaseSegment: null, selectedElementIds: new Set(), clipRevealRequest: null, keyframeCache: new Map(),