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.
This commit is contained in:
Miguel Angel Simon Sierra
2026-07-27 19:02:21 +02:00
parent d05ecb1091
commit 3c7400af89
8 changed files with 93 additions and 31 deletions
@@ -4,7 +4,7 @@
*/ */
import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore"; import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore";
import { toClipKeyframes } from "./gsapShared"; import { idFromSelector, toClipKeyframes } from "./gsapShared";
import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth";
export function updateKeyframeCacheFromParsed( export function updateKeyframeCacheFromParsed(
@@ -18,7 +18,7 @@ export function updateKeyframeCacheFromParsed(
const merged = new Map<string, KeyframeCacheEntry>(); const merged = new Map<string, KeyframeCacheEntry>();
const sourceAnimations = new Map<string, GsapAnimation[]>(); const sourceAnimations = new Map<string, GsapAnimation[]>();
for (const anim of animations) { for (const anim of animations) {
const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1]; const id = idFromSelector(anim.targetSelector);
const kfSource = const kfSource =
anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? []; anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? [];
if (!id || kfSource.length === 0) continue; if (!id || kfSource.length === 0) continue;
@@ -61,8 +61,7 @@ export function updateKeyframeCacheFromParsed(
writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id)); writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id));
} }
const targetId = const targetId =
(mutation as { targetSelector?: string }).targetSelector?.match(/^#([\w-]+)/)?.[1] ?? idFromSelector((mutation as { targetSelector?: string }).targetSelector) ?? selectionId;
selectionId;
if (targetId && !idsWithKeyframes.has(targetId)) { if (targetId && !idsWithKeyframes.has(targetId)) {
clearKeyframeCacheForElement(targetPath, targetId); clearKeyframeCacheForElement(targetPath, targetId);
} }
+20 -2
View File
@@ -1,6 +1,7 @@
import { describe, it, expect } from "vitest"; import { describe, it, expect } from "vitest";
import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import { import {
idFromSelector,
idSelector, idSelector,
isInstantHold, isInstantHold,
parsePercentageKeyframes, parsePercentageKeyframes,
@@ -125,13 +126,16 @@ describe("toClipPercentage", () => {
}); });
describe("toClipKeyframes", () => { 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", id: "a1",
method: "to", method: "to",
targetSelector: "#box", targetSelector: "#box",
vars: {}, vars: {},
resolvedStart: 0, resolvedStart: 0,
} as GsapAnimation; } as unknown as GsapAnimation;
// A tween with no duration spans its clip everywhere else in Studio // A tween with no duration spans its clip everywhere else in Studio
// (resolveEditableTweenDuration), so the cache rows have to agree: a fixed 1s // (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" }); 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();
});
});
+21
View File
@@ -80,6 +80,27 @@ export function idSelector(id: string): string {
return SAFE_HASH_ID.test(id) ? `#${id}` : `[id="${id.replace(/(["\\])/g, "\\$1")}"]`; 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 { export function selectorFromSelection(selection: DomEditSelection): string | null {
if (selection.id) return idSelector(selection.id); if (selection.id) return idSelector(selection.id);
if (selection.selector) return selection.selector; if (selection.selector) return selection.selector;
+25 -1
View File
@@ -5,6 +5,23 @@ import type {
} from "@hyperframes/core/gsap-parser"; } from "@hyperframes/core/gsap-parser";
import { PROPERTY_DEFAULTS } from "./gsapShared"; 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< export function deduplicateKeyframes<
T extends GsapPercentageKeyframe & { animationId?: string; easeAmbiguous?: boolean }, T extends GsapPercentageKeyframe & { animationId?: string; easeAmbiguous?: boolean },
>(keyframes: T[]): T[] { >(keyframes: T[]): T[] {
@@ -25,7 +42,14 @@ export function deduplicateKeyframes<
) { ) {
existing.easeAmbiguous = true; 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 { } else {
byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } }); byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } });
} }
+7 -23
View File
@@ -9,7 +9,11 @@ import {
writeGsapAnimationsForElement, writeGsapAnimationsForElement,
} from "./gsapKeyframeCacheHelpers"; } from "./gsapKeyframeCacheHelpers";
import { toAbsoluteTime, toClipPercentage, toClipKeyframes } from "./gsapShared"; import { toAbsoluteTime, toClipPercentage, toClipKeyframes } from "./gsapShared";
import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; import {
deduplicateKeyframes,
isStaticPositionHold,
synthesizeFlatTweenKeyframes,
} from "./gsapTweenSynth";
function extractIdFromSelector(selector: string): string | null { function extractIdFromSelector(selector: string): string | null {
const match = selector.match(/^#([\w-]+)/); const match = selector.match(/^#([\w-]+)/);
@@ -357,18 +361,7 @@ export function useGsapAnimationsForElement(
let ease: string | undefined; let ease: string | undefined;
let easeEach: string | undefined; let easeEach: string | undefined;
for (const anim of animations) { for (const anim of animations) {
// A static position hold (only x/y, no real motion) is a `set`, not a if (isStaticPositionHold(anim)) continue;
// 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;
const kf = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim); const kf = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim);
if (!kf) continue; if (!kf) continue;
// Convert tween-relative percentages to clip-relative so diamonds // Convert tween-relative percentages to clip-relative so diamonds
@@ -469,16 +462,7 @@ export function usePopulateKeyframeCacheForFile(
const sourceByElement = new Map<string, GsapAnimation[]>(); const sourceByElement = new Map<string, GsapAnimation[]>();
for (const anim of parsed.animations) { for (const anim of parsed.animations) {
if (anim.hasUnresolvedKeyframes) continue; if (anim.hasUnresolvedKeyframes) continue;
// Position-only static holds are not keyframed animations — skip them so if (isStaticPositionHold(anim)) continue;
// 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;
}
}
const kfData = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim); const kfData = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim);
if (!kfData) continue; if (!kfData) continue;
// Attribute the tween to every element it animates (handles class / // Attribute the tween to every element it animates (handles class /
@@ -45,7 +45,9 @@ export function useStudioTestHooks({
}; };
(window as unknown as { __studioTest?: typeof api }).__studioTest = api; (window as unknown as { __studioTest?: typeof api }).__studioTest = api;
return () => { 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]); }, [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef]);
} }
@@ -92,6 +92,13 @@ export function createKeyframeSlice(set: StoreApi<KeyframeSlice>["setState"]): K
keyframeCache: new Map(), keyframeCache: new Map(),
setKeyframeCache: (elementId, data) => setKeyframeCache: (elementId, data) =>
set((state) => { 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); const next = new Map(state.keyframeCache);
if (data) next.set(elementId, data); if (data) next.set(elementId, data);
else next.delete(elementId); else next.delete(elementId);
@@ -100,6 +107,12 @@ export function createKeyframeSlice(set: StoreApi<KeyframeSlice>["setState"]): K
gsapAnimations: new Map(), gsapAnimations: new Map(),
setGsapAnimations: (elementId, animations) => setGsapAnimations: (elementId, animations) =>
set((state) => { set((state) => {
if (
animations
? state.gsapAnimations.get(elementId) === animations
: !state.gsapAnimations.has(elementId)
)
return state;
const next = new Map(state.gsapAnimations); const next = new Map(state.gsapAnimations);
if (animations) next.set(elementId, animations); if (animations) next.set(elementId, animations);
else next.delete(elementId); else next.delete(elementId);
@@ -531,6 +531,7 @@ export const usePlayerStore = create<PlayerState>((set, get) => ({
activeTool: "select", activeTool: "select",
selectedKeyframes: new Set(), selectedKeyframes: new Set(),
expandedClipIds: new Set(), expandedClipIds: new Set(),
focusedEaseSegment: null,
selectedElementIds: new Set(), selectedElementIds: new Set(),
clipRevealRequest: null, clipRevealRequest: null,
keyframeCache: new Map(), keyframeCache: new Map(),