diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index 520b4e8554..199971e079 100644 --- a/packages/studio/src/App.tsx +++ b/packages/studio/src/App.tsx @@ -65,6 +65,7 @@ import { readStudioUrlStateFromWindow, resolveMasterCompositionPath, } from "./utils/studioUrlState"; +const getTimelineSelectionSet = () => usePlayerStore.getState().selectedElementIds; // fallow-ignore-next-line complexity export function StudioApp() { const { projectId, resolving, waitingForServer } = useServerConnection(); @@ -91,9 +92,9 @@ export function StudioApp() { const captionSync = useCaptionSync(projectId); const timelineElements = usePlayerStore((s) => s.elements); const setSelectedTimelineElementId = usePlayerStore((s) => s.setSelectedElementId); + const setTimelineSelectionSet = usePlayerStore((s) => s.setSelectedElementIds); const timelineDuration = usePlayerStore((s) => s.duration); const isPlaying = usePlayerStore((s) => s.isPlaying); - const isMasterView = !activeCompPath || activeCompPath === "index.html"; const effectiveTimelineDuration = useMemo(() => { const maxEnd = timelineElements.length > 0 @@ -270,13 +271,14 @@ export function StudioApp() { const domEditSession = useDomEditSession({ projectId, activeCompPath, - isMasterView, compIdToSrc, captionEditMode, compositionLoading, previewIframeRef, timelineElements, + getTimelineSelectionSet, setSelectedTimelineElementId, + setTimelineSelectionSet, setRightCollapsed: panelLayout.setRightCollapsed, setRightPanelTab: panelLayout.setRightPanelTab, showToast, @@ -416,6 +418,8 @@ export function StudioApp() { rightCollapsed: panelLayout.rightCollapsed, activeCompPathHydrated, domEditSelection: domEditSession.domEditSelection, + domEditGroupSelections: domEditSession.domEditGroupSelections, + applyMarqueeSelection: domEditSession.applyMarqueeSelection, buildDomSelectionFromTarget: domEditSession.buildDomSelectionFromTarget, applyDomSelection: domEditSession.applyDomSelection, setRightPanelTab: panelLayout.setRightPanelTab, diff --git a/packages/studio/src/components/editor/DomEditOverlay.test.ts b/packages/studio/src/components/editor/DomEditOverlay.test.ts index 736bd7ecb2..a7270849cd 100644 --- a/packages/studio/src/components/editor/DomEditOverlay.test.ts +++ b/packages/studio/src/components/editor/DomEditOverlay.test.ts @@ -14,7 +14,10 @@ import { resolveDomEditRotationGesture, } from "./DomEditOverlay"; import type { DomEditSelection } from "./domEditing"; -import { resolveResizeCenterAnchorOffset } from "./domEditOverlayGestures"; +import { + hoverCacheDescribesPoint, + resolveResizeCenterAnchorOffset, +} from "./domEditOverlayGestures"; // React 19 warns unless the test environment opts into act(). globalThis.IS_REACT_ACT_ENVIRONMENT = true; @@ -278,7 +281,9 @@ describe("DomEditOverlay", () => { const host = document.createElement("div"); document.body.append(host); const root = createRoot(host); - const iframeRef = { current: document.createElement("iframe") as HTMLIFrameElement | null }; + const iframeRef: { current: HTMLIFrameElement | null } = { + current: document.createElement("iframe"), + }; const onCanvasMouseDown = vi.fn(); const onMarqueeSelect = vi.fn(); @@ -323,6 +328,44 @@ describe("DomEditOverlay", () => { host.remove(); }); + it("starts a marquee from outside the composition frame", async () => { + const restoreRect = stubViewportRect(); + const originalPointerCapture = HTMLDivElement.prototype.setPointerCapture; + const setPointerCapture = vi.fn(); + HTMLDivElement.prototype.setPointerCapture = setPointerCapture; + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + const iframeRef: { current: HTMLIFrameElement | null } = { + current: document.createElement("iframe"), + }; + + act(() => { + root.render( + React.createElement(DomEditOverlay, { + ...createOverlayProps({ + iframeRef, + selection: null, + hoverSelection: null, + onSelectionChange: () => {}, + }), + onMarqueeSelect: vi.fn(), + }), + ); + }); + await flushOverlayRaf(); + + // Negative x is outside the 0..800 composition frame but still reaches the + // overlay in a real pointer event when the user starts in the grey margin. + dispatchOverlayPointerDown(getOverlay(host), -40, 100); + expect(setPointerCapture).toHaveBeenCalledTimes(1); + + act(() => root.unmount()); + HTMLDivElement.prototype.setPointerCapture = originalPointerCapture; + restoreRect(); + host.remove(); + }); + it("does not start a drag from a stale hover target on canvas pointer-down", () => { const host = document.createElement("div"); document.body.append(host); @@ -628,6 +671,50 @@ describe("resolveDomEditRotationGesture", () => { }); }); +/** + * Shift-click reads the hover cache instead of hit-testing, and the cache is + * filled asynchronously as the pointer moves. Pass over one element on the way to + * another and the cache still names the one you left, so the shift-click added + * THAT element and the click looked like it selected something at random. The + * guard is what makes the cache usable only when it is about the point clicked. + */ +describe("hoverCacheDescribesPoint", () => { + const doc = new Window().document; + + it("rejects a cache left behind by an element the pointer passed over", () => { + const passedOver = doc.createElement("div"); + const clicked = doc.createElement("div"); + doc.body.append(passedOver, clicked); + + expect(hoverCacheDescribesPoint(passedOver, clicked)).toBe(false); + }); + + it("accepts the cache when it names the element at the point", () => { + const clicked = doc.createElement("div"); + doc.body.append(clicked); + + expect(hoverCacheDescribesPoint(clicked, clicked)).toBe(true); + }); + + // The resolver is allowed to hand back a clip ancestor of the raw target, which + // still describes the same click — rejecting it would drop the fast path on + // every element that has children. + it("accepts an ancestor of the element at the point", () => { + const clip = doc.createElement("div"); + const child = doc.createElement("span"); + clip.append(child); + doc.body.append(clip); + + expect(hoverCacheDescribesPoint(clip, child)).toBe(true); + }); + + it("rejects a missing cache or an empty point", () => { + const el = doc.createElement("div"); + expect(hoverCacheDescribesPoint(null, el)).toBe(false); + expect(hoverCacheDescribesPoint(el, null)).toBe(false); + }); +}); + // resolveResizeCenterAnchorOffset is the UNROTATED (AABB) fallback used only when // the element's real transformed corners can't be measured. Center-anchored: a // width/height change grows the box from its top-left, drifting the center by half diff --git a/packages/studio/src/components/editor/DomEditOverlay.tsx b/packages/studio/src/components/editor/DomEditOverlay.tsx index 2c6d6add3c..261b82f24b 100644 --- a/packages/studio/src/components/editor/DomEditOverlay.tsx +++ b/packages/studio/src/components/editor/DomEditOverlay.tsx @@ -13,6 +13,7 @@ import { type GestureState, type GroupGestureState, focusDomEditOverlayElement, + resolveShiftClickCandidate, } from "./domEditOverlayGestures"; import { useDomEditOverlayRects } from "./useDomEditOverlayRects"; import { OffCanvasIndicators, type OffCanvasRect } from "./OffCanvasIndicators"; @@ -31,6 +32,7 @@ import { startOffCanvasIndicatorRefresh } from "./offCanvasIndicatorRefresh"; import { CanvasContextMenu } from "./CanvasContextMenu"; import type { ZOrderAction, ZOrderPatch } from "./canvasContextMenuZOrder"; import { getPreviewTargetFromPointer } from "../../utils/studioPreviewHelpers"; +import { logSelect } from "../../utils/selectDebug"; // Re-exports for external consumers — preserving existing import paths. export { @@ -318,6 +320,7 @@ export const DomEditOverlay = memo(function DomEditOverlay({ const handleOverlayMouseDown = (event: React.MouseEvent) => { if (!allowCanvasMovement) return; if (suppressNextOverlayMouseDownRef.current) { + logSelect("mousedown-suppressed", { shift: event.shiftKey }); suppressNextOverlayMouseDownRef.current = false; suppressNextBoxMouseDownRef.current = false; suppressNextBoxClickRef.current = false; @@ -326,7 +329,9 @@ export const DomEditOverlay = memo(function DomEditOverlay({ return; } const target = event.target as HTMLElement | null; - if (target?.closest('[data-dom-edit-selection-box="true"]')) return; + const onBox = Boolean(target?.closest('[data-dom-edit-selection-box="true"]')); + logSelect("mousedown", { shift: event.shiftKey, onBox }); + if (onBox) return; // Allow clicks anywhere on the overlay — GSAP-translated elements can // extend beyond the composition rect into the gray zone, and users need // to select/deselect them by clicking there. @@ -341,8 +346,20 @@ export const DomEditOverlay = memo(function DomEditOverlay({ const handleOverlayPointerDown = (event: React.PointerEvent) => { if (!allowCanvasMovement || event.button !== 0) return; if (event.shiftKey) { - // Use the already-updated hover selection rather than re-resolving async - const candidate = hoverSelectionRef.current; + const shiftIframe = iframeRef.current; + const candidate = resolveShiftClickCandidate({ + cached: hoverSelectionRef.current, + elementAtPoint: shiftIframe + ? getPreviewTargetFromPointer( + shiftIframe, + event.clientX, + event.clientY, + activeCompositionPathRef.current, + ) + : null, + }); + // Not confident: fall through untouched — no preventDefault, no suppression — + // so the mousedown path resolves this point instead of guessing here. if (!candidate) return; event.preventDefault(); event.stopPropagation(); @@ -376,28 +393,27 @@ export const DomEditOverlay = memo(function DomEditOverlay({ const overlayEl = overlayRef.current; if (overlayEl) { const oRect = overlayEl.getBoundingClientRect(); + // Anywhere empty on the overlay starts one, not just inside the frame. + // An element dragged past the edge sits OUT there in the grey, and a + // rubber band that refuses to start there cannot reach it — which left + // the timeline as the only way to select something you can plainly see. + // The hit test collects in overlay space and never clipped to the frame, + // so those elements were always selectable once the band could begin. + event.preventDefault(); + event.stopPropagation(); + suppressNextOverlayMouseDownRef.current = true; + (event.currentTarget as HTMLElement).setPointerCapture(event.pointerId); const cx = event.clientX - oRect.left; const cy = event.clientY - oRect.top; - const inComp = - cx >= compRect.left && - cx <= compRect.left + compRect.width && - cy >= compRect.top && - cy <= compRect.top + compRect.height; - if (inComp) { - event.preventDefault(); - event.stopPropagation(); - suppressNextOverlayMouseDownRef.current = true; - (event.currentTarget as HTMLElement).setPointerCapture(event.pointerId); - marquee.marqueeRef.current = { - startX: cx, - startY: cy, - currentX: cx, - currentY: cy, - pointerId: event.pointerId, - pastThreshold: false, - }; - return; - } + marquee.marqueeRef.current = { + startX: cx, + startY: cy, + currentX: cx, + currentY: cy, + pointerId: event.pointerId, + pastThreshold: false, + }; + return; } } }; @@ -503,6 +519,7 @@ export const DomEditOverlay = memo(function DomEditOverlay({ top: cr.top, width: cr.width, height: cr.height, + transform: cr.angle ? `rotate(${cr.angle}deg)` : undefined, }} /> ))} diff --git a/packages/studio/src/components/editor/domEditOverlayGeometry.test.ts b/packages/studio/src/components/editor/domEditOverlayGeometry.test.ts index 217f3d3430..0554bb4f4a 100644 --- a/packages/studio/src/components/editor/domEditOverlayGeometry.test.ts +++ b/packages/studio/src/components/editor/domEditOverlayGeometry.test.ts @@ -5,6 +5,7 @@ import { orientedGroupAwareOverlayRect, overlayCornersCentroid, selectionCacheKey, + orientedVisibleOverlayRect, } from "./domEditOverlayGeometry"; describe("overlayCornersCentroid", () => { @@ -67,6 +68,17 @@ describe("orientedOverlayRect — rotation gate (perf fix, V15 18a/18b)", () => number, ]; } + /** `this` applied outside `other`, the way an ancestor composes over a child. */ + multiply(other: { a: number; b: number; c: number; d: number; e: number; f: number }) { + const out = new (this.constructor as new (init?: string) => this)(); + out.a = this.a * other.a + this.c * other.b; + out.b = this.b * other.a + this.d * other.b; + out.c = this.a * other.c + this.c * other.d; + out.d = this.b * other.c + this.d * other.d; + out.e = this.a * other.e + this.c * other.f + this.e; + out.f = this.b * other.e + this.d * other.f + this.f; + return out; + } transformPoint(pt: { x: number; y: number }) { return { x: this.a * pt.x + this.c * pt.y + this.e, @@ -148,6 +160,30 @@ describe("orientedOverlayRect — rotation gate (perf fix, V15 18a/18b)", () => expect(rect!.angle ?? 0).toBe(0); }); + /** + * A child outline is drawn ON the child, not around it. Measured axis-aligned, + * a text layer inside a rotated card got an upright dashed box sitting across + * the rotated glyphs — the parent's chrome rotated and its children's did not. + */ + it("child outlines carry the element's angle, so they can co-rotate with it", () => { + const { overlayEl, iframe, el } = buildHarness(); + el.style.transform = ROTATE_30DEG_MATRIX; + const rect = orientedVisibleOverlayRect(overlayEl, iframe, el); + expect(rect).not.toBeNull(); + expect(rect!.angle).toBeCloseTo(30, 3); + }); + + it("an unrotated child outline is unchanged — no angle, same box as before", () => { + const { overlayEl, iframe, el } = buildHarness(); + const rect = orientedVisibleOverlayRect(overlayEl, iframe, el); + expect(rect).not.toBeNull(); + expect(rect!.angle ?? 0).toBe(0); + expect(rect!.left).toBeCloseTo(400, 5); + expect(rect!.top).toBeCloseTo(450, 5); + expect(rect!.width).toBeCloseTo(200, 5); + expect(rect!.height).toBeCloseTo(100, 5); + }); + it("rotated element takes the corner-geometry path — reports the live angle", () => { const { overlayEl, iframe, el } = buildHarness(); el.style.transform = ROTATE_30DEG_MATRIX; @@ -156,6 +192,64 @@ describe("orientedOverlayRect — rotation gate (perf fix, V15 18a/18b)", () => expect(rect!.angle).toBeCloseTo(30, 3); }); + /** + * The selection box is drawn at the size the element PAINTS, which is the + * product of every transform between it and the composition root. + * + * A text layer inside a card carrying `scale(1.2)` was drawn at 1/1.2 of the + * text: the top-left was right, because the caller anchors that to the real + * bounding rect, and the right and bottom edges fell short. The same read + * decides whether to draw the box rotated, so an element inside a rotated + * parent got an upright box. + */ + const SCALE_1_2_MATRIX = "matrix(1.2, 0, 0, 1.2, 0, 0)"; + const MIRROR_X_MATRIX = "matrix(-1, 0, 0, 1, 0, 0)"; + + it("sizes the box by the accumulated transform, not the element's own", () => { + const { overlayEl, iframe, el } = buildHarness(); + // The element carries no transform; its parent scales it by 1.2, so it + // paints at 240x120 and its bounding rect says so. + el.parentElement!.style.transform = SCALE_1_2_MATRIX; + el.style.transform = ROTATE_30DEG_MATRIX; + stubRect(el, { left: 400, top: 450, width: 240, height: 120 }); + + const rect = orientedOverlayRect(overlayEl, iframe, el); + + expect(rect).not.toBeNull(); + // 200x100 local, scaled by the ancestor, then rotated: the oriented box is + // the scaled local box, and the AABB it is anchored to is wider again. + expect(rect!.width).toBeCloseTo(240, 3); + expect(rect!.height).toBeCloseTo(120, 3); + expect(rect!.angle).toBeCloseTo(30, 3); + }); + + it("takes the rotated path when only an ANCESTOR is rotated", () => { + const { overlayEl, iframe, el } = buildHarness(); + el.parentElement!.style.transform = ROTATE_30DEG_MATRIX; + + const rect = orientedOverlayRect(overlayEl, iframe, el); + + expect(rect!.angle).toBeCloseTo(30, 3); + }); + + it("does not misread a mirrored ancestor as a 180-degree rotation", () => { + const { overlayEl, iframe, el } = buildHarness(); + el.parentElement!.style.transform = MIRROR_X_MATRIX; + + const rect = orientedOverlayRect(overlayEl, iframe, el); + + expect(rect?.angle ?? 0).toBe(0); + }); + + it("stops transform composition at the composition root", () => { + const { overlayEl, iframe, el } = buildHarness(); + iframe.contentDocument!.body.style.transform = ROTATE_30DEG_MATRIX; + + const rect = orientedOverlayRect(overlayEl, iframe, el); + + expect(rect?.angle ?? 0).toBe(0); + }); + it("preserves an ordinary element's rotation through the group-aware entry point", () => { const { overlayEl, iframe, el } = buildHarness(); el.style.transform = ROTATE_30DEG_MATRIX; diff --git a/packages/studio/src/components/editor/domEditOverlayGeometry.ts b/packages/studio/src/components/editor/domEditOverlayGeometry.ts index 70f2500958..004bef4369 100644 --- a/packages/studio/src/components/editor/domEditOverlayGeometry.ts +++ b/packages/studio/src/components/editor/domEditOverlayGeometry.ts @@ -117,6 +117,28 @@ interface ElementTransformSnapshot { cs: CSSStyleDeclaration; } +/** + * The transform from the element's own box to the composition's, ACCUMULATED + * over its ancestors rather than read from the element alone. + * + * What the user sees is the product of every transform between the element and + * the composition root, and an element is routinely a child of something + * scaled or rotated. Reading only its own transform drew the selection box at + * the element's untransformed size: a text layer inside a card carrying + * `scale(1.2)` got a box at 1/1.2 of the text, with the top-left correct (the + * caller anchors that to the real bounding rect) and the right and bottom + * edges falling short. The same read decides whether to draw the box rotated, + * so an element inside a rotated parent got an upright box too. + * + * Only the linear part matters here. Each transform's origin contributes + * translation, and the caller discards translation by matching the corners' + * bounding box to the element's real one, so composing the matrices alone is + * enough and there is no per-ancestor origin to unpick. + * + * The walk stops at the composition document's root. The canvas zoom lives on + * the iframe element in Studio's own document and is applied separately by + * `computeOverlayRootScale`; including it here would count it twice. + */ function readElementTransformSnapshot( win: Window, element: HTMLElement, @@ -125,7 +147,15 @@ function readElementTransformSnapshot( if (!DOMMatrixCtor) return null; const cs = win.getComputedStyle(element); try { - const matrix = new DOMMatrixCtor(cs.transform === "none" ? "" : cs.transform); + let matrix = new DOMMatrixCtor(); + for (let node: HTMLElement | null = element; node; node = node.parentElement) { + const transform = node === element ? cs.transform : win.getComputedStyle(node).transform; + if (transform && transform !== "none") { + // An ancestor applies outside, so it multiplies on the left. + matrix = new DOMMatrixCtor(transform).multiply(matrix); + } + if (node.hasAttribute("data-composition-id")) break; + } return { matrix, cs }; } catch { return null; @@ -141,7 +171,16 @@ function readElementTransformSnapshot( function rotationDegreesFromMatrix(matrix: DOMMatrix): number { const a = Number.isFinite(matrix.a) ? matrix.a : 1; const b = Number.isFinite(matrix.b) ? matrix.b : 0; - const deg = (Math.atan2(b, a) * 180) / Math.PI; + const c = Number.isFinite(matrix.c) ? matrix.c : 0; + const d = Number.isFinite(matrix.d) ? matrix.d : 1; + const fromX = (Math.atan2(b, a) * 180) / Math.PI; + const determinant = a * d - b * c; + // A reflection makes one basis direction read 180° away from the authored + // rotation. For cursor/handle orientation those directions are equivalent; + // choose the representative nearest zero instead of drawing a pure mirror's + // rotate handle on the opposite side of the element. + const fromY = (Math.atan2(-c, d) * 180) / Math.PI; + const deg = determinant < 0 && Math.abs(fromY) < Math.abs(fromX) ? fromY : fromX; return Number.isFinite(deg) ? deg : 0; } @@ -391,6 +430,24 @@ export function orientedOverlayRect( }; } +/** + * `toVisibleOverlayRect`'s oriented twin: the element's crop-hugged box plus its + * live rotation, for chrome that has to sit on a rotated element rather than + * around it. Rendering the result with `transform: rotate(angle)` about its + * centre lands it on the element's real corners. + * + * At angle 0 `orientedOverlayRect` returns the plain AABB, so an unrotated + * element measures exactly as it did before. + */ +export function orientedVisibleOverlayRect( + overlayEl: HTMLDivElement, + iframe: HTMLIFrameElement, + element: HTMLElement, +): OverlayRect | null { + const rect = orientedOverlayRect(overlayEl, iframe, element); + return rect ? { ...rect, ...hugRectForElement(rect, element) } : null; +} + const OVERLAY_RECT_EPSILON_PX = 0.5; const OVERLAY_RECT_ANGLE_EPSILON_DEG = 0.1; diff --git a/packages/studio/src/components/editor/domEditOverlayGestures.ts b/packages/studio/src/components/editor/domEditOverlayGestures.ts index d7fec86a87..34d6519c19 100644 --- a/packages/studio/src/components/editor/domEditOverlayGestures.ts +++ b/packages/studio/src/components/editor/domEditOverlayGestures.ts @@ -10,6 +10,7 @@ import type { GroupOverlayItem, OverlayRect } from "./domEditOverlayGeometry"; import type { SnapContext } from "./snapTargetCollection"; import type { SnapGuidesState } from "./SnapGuideOverlay"; import type { PreviewMouseDownOptions } from "../../hooks/usePreviewInteraction"; +import { logSelect } from "../../utils/selectDebug"; export type GestureKind = "drag" | "resize" | "rotate"; @@ -112,6 +113,47 @@ export function focusDomEditOverlayElement(element: FocusableDomEditOverlay | nu element?.focus({ preventScroll: true }); } +/** + * Whether the hover cache may stand in for a hit-test at this point. + * + * The cache is filled asynchronously as the pointer moves, so it can describe an + * element the pointer has already left. That is harmless for drawing a hover + * outline and wrong for a shift-click, which would add the stale element to the + * selection instead of the one under the pointer. True only when the cached + * element IS the element at the point, or contains it — the resolver is allowed + * to hand back a clip ancestor of the raw target, and that still describes the + * same click. + */ +export function hoverCacheDescribesPoint( + cachedElement: Element | null | undefined, + elementAtPoint: Element | null | undefined, +): boolean { + if (!cachedElement || !elementAtPoint) return false; + return cachedElement === elementAtPoint || cachedElement.contains(elementAtPoint); +} + +/** + * The element a shift-click should add, or null to let the slower path resolve it. + * + * Reading the hover cache without checking is safe for a hover outline and wrong + * for a shift-click: the click silently adds whatever the pointer last passed + * over instead of the element under it, which reads as multi-select picking + * things at random. Returning null means "not confident", and the caller must + * then fall through untouched so the mousedown path resolves the point properly. + */ +export function resolveShiftClickCandidate(input: { + cached: T | null; + elementAtPoint: Element | null; +}): T | null { + const describes = hoverCacheDescribesPoint(input.cached?.element, input.elementAtPoint); + logSelect("shift-pointerdown", { + candidate: input.cached ? ((input.cached as { selector?: string }).selector ?? null) : null, + pointTarget: input.elementAtPoint?.id ?? input.elementAtPoint?.tagName ?? null, + cacheIsAboutThisPoint: describes, + }); + return describes ? input.cached : null; +} + /** * Overlay-px translation that keeps the element's CENTER fixed while a corner * resizes: a CSS width/height change grows the layout box from its top-left, so diff --git a/packages/studio/src/components/editor/domEditOverlayStartGesture.ts b/packages/studio/src/components/editor/domEditOverlayStartGesture.ts index 6576a9b5ba..390e05e66e 100644 --- a/packages/studio/src/components/editor/domEditOverlayStartGesture.ts +++ b/packages/studio/src/components/editor/domEditOverlayStartGesture.ts @@ -33,6 +33,7 @@ import { } from "./domEditOverlayGestures"; import { collectSnapContext, buildExcludeElements } from "./snapTargetCollection"; import { logResize, resetResizeMoveLog } from "../../utils/resizeDebug"; +import { logDrag, readDragPositions, resetDragMoveLog } from "../../utils/dragDebug"; export function startGroupDrag( e: React.PointerEvent, @@ -70,6 +71,22 @@ export function startGroupDrag( } members.push(result.member); } + resetDragMoveLog(); + logDrag("group-start", { + // A member whose mapping differs from its neighbours travels a different + // distance for the same pointer delta, which is the group coming apart. + members: Object.fromEntries( + members.map((member) => [ + member.key, + { + map: `${member.screenToOffset.a.toFixed(3)},${member.screenToOffset.d.toFixed(3)}`, + base: `${Math.round(member.baseGsap.x)},${Math.round(member.baseGsap.y)}`, + offset: `${Math.round(member.initialOffset.x)},${Math.round(member.initialOffset.y)}`, + }, + ]), + ), + at: readDragPositions(members), + }); const overlayEl = opts.overlayRef.current; const iframe = opts.iframeRef.current; diff --git a/packages/studio/src/components/editor/groupDragMove.ts b/packages/studio/src/components/editor/groupDragMove.ts new file mode 100644 index 0000000000..7a0fe732a4 --- /dev/null +++ b/packages/studio/src/components/editor/groupDragMove.ts @@ -0,0 +1,110 @@ +import { resolveDomEditGroupOverlayRect } from "./domEditOverlayGeometry"; +import { + resolveEquidistanceGuides, + resolveSnapAdjustment, + snapEngagedForTravel, + SNAP_THRESHOLD_PX, +} from "./snapEngine"; +import { applyManualOffsetDragDraft } from "./manualOffsetDrag"; +import type { GroupGestureState, UseDomEditOverlayGesturesOptions } from "./domEditOverlayGestures"; +import type { GroupOverlayItem } from "./domEditOverlayGeometry"; +import { + findNonRigidMembers, + logDrag, + logDragMove, + readDragPositions, +} from "../../utils/dragDebug"; + +/** + * One frame of a group drag, kept out of onPointerMove — which already handles + * four gesture kinds and reads better without this one's snapping arithmetic. + * The previous frame's positions live in the closure so the rigidity check below + * compares against the frame before, not against whatever was last sampled. + */ +export function createGroupDragMover( + opts: UseDomEditOverlayGesturesOptions, + setDraftGroupOverlayItems: (items: GroupOverlayItem[]) => void, +) { + let lastGroupPositions: Record = {}; + let lastGesture: GroupGestureState | null = null; + + /** Snap the group's delta to nearby edges, publishing the guides drawn for it. */ + // fallow-ignore-next-line complexity + const snapGroupDelta = ( + groupG: GroupGestureState, + e: React.PointerEvent, + proposed: { dx: number; dy: number }, + ) => { + const sc = groupG.snapContext; + if (!sc?.snapEnabled || sc.targets.length === 0) return proposed; + if (!snapEngagedForTravel(proposed.dx, proposed.dy)) { + opts.snapGuidesRef.current = null; + return proposed; + } + const groupBounds = resolveDomEditGroupOverlayRect(groupG.originItems.map((i) => i.rect)); + if (!groupBounds) return proposed; + const allTargets = sc.compositionTarget ? [...sc.targets, sc.compositionTarget] : sc.targets; + const snap = resolveSnapAdjustment({ + movingRect: groupBounds, + proposedDx: proposed.dx, + proposedDy: proposed.dy, + targets: allTargets, + gridEdges: sc.gridEdges ?? undefined, + threshold: SNAP_THRESHOLD_PX, + disabled: e.altKey, + }); + const movingRect = { + ...groupBounds, + left: groupBounds.left + snap.dx, + top: groupBounds.top + snap.dy, + }; + const spacingGuides = e.altKey + ? [] + : resolveEquidistanceGuides({ + movingRect, + targets: allTargets, + threshold: SNAP_THRESHOLD_PX, + }); + opts.snapGuidesRef.current = { guides: snap.guides, spacingGuides }; + return { dx: snap.dx, dy: snap.dy }; + }; + + /** One frame of a group drag: snap the delta, redraw the boxes, move every member. */ + const moveGroupDrag = (groupG: GroupGestureState, e: React.PointerEvent) => { + if (groupG !== lastGesture) { + lastGesture = groupG; + lastGroupPositions = {}; + } + const { dx, dy } = snapGroupDelta(groupG, e, { + dx: e.clientX - groupG.startX, + dy: e.clientY - groupG.startY, + }); + groupG.lastSnappedDx = dx; + groupG.lastSnappedDy = dy; + + setDraftGroupOverlayItems( + groupG.originItems.map((i) => ({ + ...i, + rect: { ...i.rect, left: i.rect.left + dx, top: i.rect.top + dy }, + })), + ); + const offsets: Record = {}; + for (const m of groupG.members) { + const n = applyManualOffsetDragDraft(m, dx, dy); + offsets[m.key] = `${Math.round(n.x)},${Math.round(n.y)}`; + } + const at = readDragPositions(groupG.members); + const px = Math.round(e.clientX - groupG.startX); + const py = Math.round(e.clientY - groupG.startY); + // A member breaking away IS the fault, so it reports on the frame it happens; + // the throttled line below would step over it. A gap between pointer and + // applied there is snapping pulling the group off the cursor. + const trace = { pointer: `${px},${py}`, applied: `${Math.round(dx)},${Math.round(dy)}`, at }; + const drift = findNonRigidMembers(lastGroupPositions, at); + if (drift.length > 0) logDrag("drift", { ...trace, drift }); + lastGroupPositions = at; + logDragMove({ ...trace, offsets }); + }; + + return moveGroupDrag; +} diff --git a/packages/studio/src/components/editor/groupDropKeepsSelection.test.ts b/packages/studio/src/components/editor/groupDropKeepsSelection.test.ts new file mode 100644 index 0000000000..3fc95b6742 --- /dev/null +++ b/packages/studio/src/components/editor/groupDropKeepsSelection.test.ts @@ -0,0 +1,70 @@ +// @vitest-environment happy-dom +import { describe, expect, it, vi } from "vitest"; +import { createDomEditOverlayGestureHandlers } from "./useDomEditOverlayGestures"; +import type { GroupGestureState } from "./domEditOverlayGestures"; + +/** + * A group drag ended by deselecting the group it had just moved. + * + * Every pointerup trails a click. The gesture ref is cleared before the commit, + * so by the time that click arrives the box no longer looks busy and it reaches + * the canvas as an ordinary click — landing in the gap between the members, + * resolving to nothing, and clearing the selection. Captured live as a + * `[hf-select] clear` with `hadGroup: 3` two milliseconds after the drop. + * + * The under-threshold path already ate that click; the committed path has to as + * well, and the flag is set before the two diverge so neither can forget. + */ +describe("dropping a dragged group eats the click that follows", () => { + function harness(travel: { dx: number; dy: number }) { + const suppressNextBoxClickRef = { current: false }; + const groupGestureRef = { + current: { + startX: 0, + startY: 0, + originItems: [], + members: [], + } as unknown as GroupGestureState, + }; + const handlers = createDomEditOverlayGestureHandlers({ + overlayRef: { current: null }, + iframeRef: { current: null }, + boxRef: { current: null }, + selectionRef: { current: null }, + hoverSelectionRef: { current: null }, + overlayRectRef: { current: null }, + groupOverlayItemsRef: { current: [] }, + gestureRef: { current: null }, + groupGestureRef, + blockedMoveRef: { current: null }, + rafPausedRef: { current: false }, + suppressNextBoxClickRef, + setOverlayRect: vi.fn(), + setGroupOverlayItems: vi.fn(), + onBlockedMoveRef: { current: vi.fn() }, + onManualDragStartRef: { current: vi.fn() }, + onPathOffsetCommitRef: { current: vi.fn() }, + onGroupPathOffsetCommitRef: { current: vi.fn() }, + onBoxSizeCommitRef: { current: vi.fn() }, + onRotationCommitRef: { current: vi.fn() }, + onCanvasPointerMoveRef: { current: vi.fn() }, + onCanvasMouseDown: vi.fn(), + snapGuidesRef: { current: null }, + } as never); + + handlers.onPointerUp({ + clientX: travel.dx, + clientY: travel.dy, + currentTarget: { releasePointerCapture: vi.fn() }, + } as never); + return suppressNextBoxClickRef; + } + + it("eats the click after a drag that moved", () => { + expect(harness({ dx: 120, dy: 60 }).current).toBe(true); + }); + + it("still eats it after a press that never travelled", () => { + expect(harness({ dx: 1, dy: 0 }).current).toBe(true); + }); +}); diff --git a/packages/studio/src/components/editor/manualEditsDom.ts b/packages/studio/src/components/editor/manualEditsDom.ts index 9ac4588a26..4a041e020f 100644 --- a/packages/studio/src/components/editor/manualEditsDom.ts +++ b/packages/studio/src/components/editor/manualEditsDom.ts @@ -221,6 +221,7 @@ function isIdentityAfterTranslateStrip(m: DOMMatrix): boolean { return m.is2D && m.a === 1 && m.b === 0 && m.c === 0 && m.d === 1; } +// fallow-ignore-next-line complexity function stripGsapTranslateFromTransform(element: HTMLElement): void { if (element.hasAttribute(STUDIO_MANUAL_EDIT_GESTURE_ATTR)) return; const transform = element.style.getPropertyValue("transform"); @@ -256,6 +257,7 @@ function stripGsapTranslateFromTransform(element: HTMLElement): void { // and push the offset straight into GSAP's x/y via gsap.set; the var() offset is // still persisted (buildPathOffsetPatches), and GSAP re-reads it at init on // reload. Returns true when handled as GSAP (caller must skip the CSS path). +// fallow-ignore-next-line complexity function applyStudioPathOffsetViaGsap( element: HTMLElement, offset: { x: number; y: number }, @@ -553,26 +555,26 @@ function queryStudioElements(doc: Document, attr: string): HTMLElement[] { function reapplyPathOffsets(doc: Document): void { for (const el of queryStudioElements(doc, STUDIO_PATH_OFFSET_ATTR)) { - const gsapSkip = gsapAnimatesProperty(el, "x", "y"); + // Unlike size below, the offset channels COMPOSE — applying both doubles the move. + if (gsapAnimatesProperty(el, "x", "y")) continue; const x = el.style.getPropertyValue(STUDIO_OFFSET_X_PROP); const y = el.style.getPropertyValue(STUDIO_OFFSET_Y_PROP); - if (gsapSkip) continue; - if (x || y) { - applyStudioPathOffset( - el, - { - x: Number.parseFloat(x) || 0, - y: Number.parseFloat(y) || 0, - }, - { updateBase: false }, - ); - } + if (!x && !y) continue; + const offset = { x: Number.parseFloat(x) || 0, y: Number.parseFloat(y) || 0 }; + applyStudioPathOffset(el, offset, { updateBase: false }); } } +/** + * Put the studio's committed size back after a seek, GSAP-sized elements included. + * Size does not compose the way the offset above does: both channels write width + * and height, so the later write wins on the same number. Standing aside meant + * nothing held the size while a soft reload reverted the old timeline (GSAP hands + * back each tween's recorded starting width), so the element sat at its stylesheet + * size until the new one rendered — the jump after a resize. + */ function reapplyBoxSizes(doc: Document): void { for (const el of queryStudioElements(doc, STUDIO_BOX_SIZE_ATTR)) { - if (gsapAnimatesProperty(el, "width", "height")) continue; const w = Number.parseFloat(el.style.getPropertyValue(STUDIO_WIDTH_PROP)); const h = Number.parseFloat(el.style.getPropertyValue(STUDIO_HEIGHT_PROP)); if (Number.isFinite(w) && Number.isFinite(h) && w > 0 && h > 0) { diff --git a/packages/studio/src/components/editor/manualOffsetDrag.test.ts b/packages/studio/src/components/editor/manualOffsetDrag.test.ts index 5af32996a0..0db15b1149 100644 --- a/packages/studio/src/components/editor/manualOffsetDrag.test.ts +++ b/packages/studio/src/components/editor/manualOffsetDrag.test.ts @@ -88,6 +88,41 @@ describe("measureManualOffsetDragScreenToOffsetMatrix", () => { expect(element.style.getPropertyValue("translate")).toBe(""); }); + /** + * The element that has never been offset is the common case, and it used to skip + * the measurement and assume the canvas zoom was the whole story. Any transform + * above the element makes that assumption wrong: the mirrored parent here sends a + * rightward drag left, so the overlay followed the pointer while the element went + * the other way, and only on drop did the overlay jump to where the element really + * was. The fixture mirrors x and scales both axes by 1.2, as a `rotationY: 180` + * card at `scale: 1.2` does. + */ + it("measures a mirrored parent even when the element carries no offset yet", () => { + const window = new Window(); + const element = window.document.createElement("div"); + window.document.body.append(element); + + element.getBoundingClientRect = () => { + const offsetX = Number.parseFloat(element.style.getPropertyValue(STUDIO_OFFSET_X_PROP)) || 0; + const offsetY = Number.parseFloat(element.style.getPropertyValue(STUDIO_OFFSET_Y_PROP)) || 0; + return new window.DOMRect(100 - 1.2 * offsetX, 200 + 1.2 * offsetY, 40, 20); + }; + + const measured = measureManualOffsetDragScreenToOffsetMatrix(element, { x: 0, y: 0 }); + if (!measured.ok) throw new Error(measured.reason); + + // Dragging one screen px right must move the element one screen px right, which + // on a mirrored parent means writing a NEGATIVE offset. + const offset = resolveManualOffsetForPointerDelta({ + initialOffset: { x: 0, y: 0 }, + screenToOffset: measured.matrix, + dx: 60, + dy: 60, + }); + expect(offset.x).toBeCloseTo(-50, 6); + expect(offset.y).toBeCloseTo(50, 6); + }); + it("measures movement in parent viewport pixels when the element is inside a scaled iframe", () => { const window = new Window(); const iframe = window.document.createElement("iframe"); @@ -133,7 +168,12 @@ describe("measureManualOffsetDragScreenToOffsetMatrix", () => { expect(nextOffset).toEqual({ x: 100, y: 50 }); }); - it("returns identity matrix for non-path-offset elements with zero initial offset", () => { + // Carrying no path offset used to be taken as permission to assume the response + // instead of measuring it. It is not a signal about the transforms above the + // element, so it no longer changes the answer: an element that does not move is + // unmeasurable either way, and the caller falls back rather than being handed a + // matrix that was never checked. + it("does not treat a missing path offset as a measurable response", () => { const window = new Window(); const element = window.document.createElement("div"); window.document.body.append(element); @@ -141,10 +181,7 @@ describe("measureManualOffsetDragScreenToOffsetMatrix", () => { const measured = measureManualOffsetDragScreenToOffsetMatrix(element, { x: 0, y: 0 }); - expect(measured.ok).toBe(true); - if (measured.ok) { - expectMatrixClose(measured.matrix, { a: 1, b: 0, c: 0, d: 1 }); - } + expect(measured.ok).toBe(false); }); it("rejects path-offset elements whose movement response cannot be measured", () => { @@ -160,6 +197,56 @@ describe("measureManualOffsetDragScreenToOffsetMatrix", () => { }); }); +/** + * A group drag is rigid: every member is handed the SAME pointer delta and must + * travel the same distance on screen, or the group visibly comes apart mid-drag. + * Members do not share a mapping though — each measures its own, because each can + * sit under different ancestor transforms. A member whose movement cannot be + * measured falls back to a guess, and this pins what that guess costs the group. + */ +describe("group drag stays rigid", () => { + function member(key: string, response: number, measurable: boolean) { + const window = new Window(); + const element = window.document.createElement("div"); + window.document.body.append(element); + element.getBoundingClientRect = () => { + const ox = Number.parseFloat(element.style.getPropertyValue(STUDIO_OFFSET_X_PROP)) || 0; + const oy = Number.parseFloat(element.style.getPropertyValue(STUDIO_OFFSET_Y_PROP)) || 0; + const move = measurable ? response : 0; + return new window.DOMRect(100 + move * ox, 200 + move * oy, 40, 20); + }; + const result = createManualOffsetDragMember({ + key, + selection: { element } as never, + element, + rect: { left: 100, top: 200, width: 40, height: 20, editScaleX: 1, editScaleY: 1 }, + }); + if (!result.ok) throw new Error(result.reason); + return { member: result.member, response }; + } + + /** Screen distance this member travels for a pointer delta of `d`. */ + function screenTravel(entry: ReturnType, d: number): number { + const offset = resolveManualOffsetForPointerDelta({ + initialOffset: entry.member.initialOffset, + screenToOffset: entry.member.screenToOffset, + dx: d, + dy: 0, + }); + return offset.x * entry.response; + } + + it("moves every measurable member the same distance for one pointer delta", () => { + // Two members under different ancestor scales: one 1:1, one inside a half-scale + // parent. Different offsets, identical screen travel — that is what rigid means. + const a = member("a", 1, true); + const b = member("b", 0.5, true); + + expect(screenTravel(a, 60)).toBeCloseTo(60, 6); + expect(screenTravel(b, 60)).toBeCloseTo(60, 6); + }); +}); + describe("createManualOffsetDragMember uses raw CSS var offset", () => { it("ignores GSAP transform — initialOffset comes from CSS vars only", () => { const window = new Window(); diff --git a/packages/studio/src/components/editor/manualOffsetDrag.ts b/packages/studio/src/components/editor/manualOffsetDrag.ts index a808c792fb..75609a73c3 100644 --- a/packages/studio/src/components/editor/manualOffsetDrag.ts +++ b/packages/studio/src/components/editor/manualOffsetDrag.ts @@ -213,9 +213,9 @@ export function applyManualOffsetDragMatrix(matrix: ManualOffsetDragMatrix, poin * The perspective w-divisor (matrix3d m44) of the element's current transform. * For a plain `translateZ(z)` under `perspective(p)`, m44 = (p - z) / p, so the * element renders 1/m44× larger and a translate of `d` composition px moves - * `d / m44` px on screen. Returns 1 for 2D transforms (no foreshortening). Used - * to keep the drag offset → screen-movement mapping correct for depth elements, - * which the flat-scale fast path below would otherwise get wrong by 1/m44. + * `d / m44` px on screen. Returns 1 for 2D transforms (no foreshortening). Only + * the unmeasurable-element fallback needs this — the measured path reads the + * foreshortening off the element's real movement along with everything else. */ function readTransformWDivisor(element: HTMLElement): number { const t = element.ownerDocument.defaultView?.getComputedStyle(element).transform; @@ -225,25 +225,25 @@ function readTransformWDivisor(element: HTMLElement): number { return Number.isFinite(w) && w > 0 ? w : 1; } +/** + * How far the element actually moves on screen per unit of drag offset, measured + * rather than assumed. + * + * The offset is written on the element, but what reaches the screen is that offset + * put through every transform above it. A parent carrying a rotation, a mirror, a + * scale or a perspective changes both the direction and the distance — a card at + * `rotationY: 180` sends a rightward drag left. Guessing this from the canvas zoom + * alone was wrong for every such element: the overlay tracked the pointer while the + * element went somewhere else, and the overlay only jumped to the truth on drop, + * when it re-measured. Moving the element and watching where it lands costs three + * layout reads once per gesture and is right for any transform, including ones no + * closed-form fast path would cover. + */ export function measureManualOffsetDragScreenToOffsetMatrix( element: HTMLElement, initialOffset: { x: number; y: number }, options: { probeSize?: number; scaleX?: number; scaleY?: number } = {}, ): { ok: true; matrix: ManualOffsetDragMatrix } | { ok: false; reason: string } { - if ( - !element.hasAttribute("data-hf-studio-path-offset") && - initialOffset.x === 0 && - initialOffset.y === 0 - ) { - const sx = options.scaleX || 1; - const sy = options.scaleY || 1; - // Fold in the perspective foreshortening: a depth element (z≠0) moves - // 1/m44× faster on screen than its flat scale implies, so the screen→offset - // matrix must scale by m44 or the element outruns the pointer/overlay. - const w = readTransformWDivisor(element); - return { ok: true, matrix: { a: w / sx, b: 0, c: 0, d: w / sy } }; - } - const probeSize = options.probeSize ?? DEFAULT_OFFSET_PROBE_PX; if (!Number.isFinite(probeSize) || probeSize <= 0) { return { ok: false, reason: "Invalid movement probe size." }; @@ -325,6 +325,8 @@ export function resolveManualOffsetForPointerDelta(input: { }; } +// Pre-existing complexity — surfaced by this branch touching the file, not by new logic. +// fallow-ignore-next-line complexity export function createManualOffsetDragMember(input: { key: string; selection: DomEditSelection; @@ -515,6 +517,7 @@ function restoreManualOffsetDragMember(member: ManualOffsetDragMember): void { endStudioManualEditGesture(member.element, member.gestureToken); } +/** Roll back a FAILED drag to the exact gesture-start state. */ export function restoreManualOffsetDragMembers(members: ManualOffsetDragMember[]): void { for (const member of members) { restoreManualOffsetDragMember(member); @@ -522,6 +525,7 @@ export function restoreManualOffsetDragMembers(members: ManualOffsetDragMember[] } } +/** Teardown after a COMMITTED drag. */ export function endManualOffsetDragMembers(members: ManualOffsetDragMember[]): void { for (const member of members) { endStudioManualEditGesture(member.element, member.gestureToken); @@ -550,6 +554,7 @@ export function endManualOffsetDragMembers(members: ManualOffsetDragMember[]): v } } +/** Shared timeline teardown for either the committed or restored path. */ export function resumeGsapTimelines(element: HTMLElement): void { const ids = element.getAttribute("data-hf-drag-paused-timelines"); element.removeAttribute("data-hf-drag-paused-timelines"); diff --git a/packages/studio/src/components/editor/reapplyBoxSizeAfterSeek.test.ts b/packages/studio/src/components/editor/reapplyBoxSizeAfterSeek.test.ts new file mode 100644 index 0000000000..92a9087968 --- /dev/null +++ b/packages/studio/src/components/editor/reapplyBoxSizeAfterSeek.test.ts @@ -0,0 +1,64 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it } from "vitest"; +import { reapplyPositionEditsAfterSeek } from "./manualEditsDom"; +import { STUDIO_BOX_SIZE_ATTR, STUDIO_HEIGHT_PROP, STUDIO_WIDTH_PROP } from "./manualEditsTypes"; + +/** + * A resize commit hands the size to a GSAP tween, and a soft reload reverts the + * old timeline before the new one renders — GSAP restores each tween's recorded + * starting width on the way out. Nothing else held the size across that window, + * so the element sat at its stylesheet size for a few hundred milliseconds: the + * jump after a resize. Worse, the next gesture then started from a box that + * disagreed with the studio's own vars and snapped on its first move. + * + * The seek reapply is what closes the window, and it used to stand aside for + * exactly the elements that need it — the ones GSAP sizes. + */ +describe("box size survives a seek while GSAP owns the size", () => { + afterEach(() => { + document.body.innerHTML = ""; + Reflect.deleteProperty(window, "__timelines"); + }); + + function cardSizedByGsap(): HTMLElement { + const el = document.createElement("div"); + el.id = "card"; + el.setAttribute(STUDIO_BOX_SIZE_ATTR, "true"); + el.style.setProperty(STUDIO_WIDTH_PROP, "305px"); + el.style.setProperty(STUDIO_HEIGHT_PROP, "202px"); + document.body.append(el); + // A timeline that animates this element's width/height, as the committed + // resize leaves behind. + Object.assign(window, { + __timelines: { + main: { + getChildren: () => [{ targets: () => [el], vars: { width: 305, height: 202 } }], + }, + }, + }); + return el; + } + + it("re-applies the committed size after the timeline gave it back", () => { + const el = cardSizedByGsap(); + // The revert: GSAP puts the tween's recorded starting size back. + el.style.width = "395px"; + el.style.height = "261px"; + + reapplyPositionEditsAfterSeek(document); + + expect(el.style.width).toBe("305px"); + expect(el.style.height).toBe("202px"); + }); + + it("leaves an element alone once its studio size is cleared", () => { + const el = cardSizedByGsap(); + el.style.removeProperty(STUDIO_WIDTH_PROP); + el.style.removeProperty(STUDIO_HEIGHT_PROP); + el.style.width = "395px"; + + reapplyPositionEditsAfterSeek(document); + + expect(el.style.width).toBe("395px"); + }); +}); diff --git a/packages/studio/src/components/editor/snapEngageTravel.test.ts b/packages/studio/src/components/editor/snapEngageTravel.test.ts new file mode 100644 index 0000000000..74b045bd58 --- /dev/null +++ b/packages/studio/src/components/editor/snapEngageTravel.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { + resolveSnapAdjustment, + snapEngagedForTravel, + SNAP_THRESHOLD_PX, + type SnapTarget, +} from "./snapEngine"; + +/** + * Picking a selection up used to move it. An element resting within the snap + * threshold of a guide is already snappable, so the snap computed on the first + * frame of a drag displaced it by up to the threshold while the pointer had not + * moved at all — captured live as `pointer "0,0"` against `applied "4,-3"`, with + * every member of the group jumping 12,-8 composition px before the drag had + * started. Snapping pulls toward a guide as you drag; it has nothing to say about + * a gesture that has not moved. + */ +describe("snapping waits for the drag to travel", () => { + // Moving box's right edge is at 150; the target's left edge is at 154, so the + // pair is 4px apart — inside the threshold, and snappable the moment it is asked. + const movingRect = { left: 100, top: 50, width: 50, height: 40 }; + const target: SnapTarget = { + left: 154, + top: 50, + right: 254, + bottom: 90, + centerX: 204, + centerY: 70, + id: "neighbour", + }; + + const snapAt = (dx: number, dy: number) => + resolveSnapAdjustment({ + movingRect, + proposedDx: dx, + proposedDy: dy, + targets: [target], + threshold: SNAP_THRESHOLD_PX, + disabled: false, + disabledForTravel: !snapEngagedForTravel(dx, dy), + }); + + it("does not move a selection that has not been dragged yet", () => { + expect(snapAt(0, 0)).toMatchObject({ dx: 0, dy: 0 }); + }); + + it("leaves a sub-threshold twitch alone", () => { + expect(snapAt(1, -1)).toMatchObject({ dx: 1, dy: -1 }); + }); + + it("still snaps once the drag is a real one", () => { + expect(snapEngagedForTravel(0, 0)).toBe(false); + expect(snapEngagedForTravel(10, 0)).toBe(true); + // Without the travel gate the same delta snaps, which is the behaviour to keep. + const engaged = resolveSnapAdjustment({ + movingRect, + proposedDx: 0, + proposedDy: 0, + targets: [target], + threshold: SNAP_THRESHOLD_PX, + disabled: false, + }); + expect(engaged.dx).toBe(4); + }); +}); diff --git a/packages/studio/src/components/editor/snapEngine.ts b/packages/studio/src/components/editor/snapEngine.ts index 6a64a22a29..f80db5f3b4 100644 --- a/packages/studio/src/components/editor/snapEngine.ts +++ b/packages/studio/src/components/editor/snapEngine.ts @@ -3,6 +3,24 @@ // All position values are in overlay-space (screen) pixels. export const SNAP_THRESHOLD_PX = 6; +/** + * Pointer travel a MOVE must reach before snapping is allowed to touch it. + * + * An element resting within the threshold of a guide is already "snappable", so + * a snap computed on the very first frame displaces it by up to the threshold + * while the pointer has moved nothing — pick a selection up and the whole thing + * teleports before you have dragged at all. Snapping is meant to pull toward a + * guide as the user drags, so it does not participate until the drag is real. + * The value matches the distance a drag must cover to count as a drag rather + * than a click, so nothing below it moves anything. + */ +const SNAP_ENGAGE_TRAVEL_PX = 4; + +/** Whether a move of this size has travelled far enough for snapping to apply. */ +export function snapEngagedForTravel(dx: number, dy: number): boolean { + return Math.hypot(dx, dy) >= SNAP_ENGAGE_TRAVEL_PX; +} + const EQUIDISTANCE_TOLERANCE_PX = 1; // --------------------------------------------------------------------------- @@ -359,8 +377,10 @@ export function resolveSnapAdjustment(input: { gridEdges?: { x: SnapEdge[]; y: SnapEdge[] }; threshold: number; disabled: boolean; + /** Set when the gesture has not travelled far enough for snapping yet. */ + disabledForTravel?: boolean; }): SnapResult { - if (input.disabled || input.threshold <= 0) { + if (input.disabled || input.disabledForTravel || input.threshold <= 0) { return DISABLED_RESULT(input.proposedDx, input.proposedDy); } diff --git a/packages/studio/src/components/editor/useDomEditOverlayGestures.ts b/packages/studio/src/components/editor/useDomEditOverlayGestures.ts index 7af8df2e7b..65e8bb4222 100644 --- a/packages/studio/src/components/editor/useDomEditOverlayGestures.ts +++ b/packages/studio/src/components/editor/useDomEditOverlayGestures.ts @@ -30,7 +30,6 @@ import { type GroupOverlayItem, type OverlayRect, orientedOverlayRect, - resolveDomEditGroupOverlayRect, } from "./domEditOverlayGeometry"; import { BLOCKED_MOVE_THRESHOLD_PX, @@ -50,8 +49,15 @@ import { startGroupDrag as _startGroupDrag, } from "./domEditOverlayStartGesture"; import { hugRectForElement } from "./domEditOverlayCrop"; -import { resolveSnapAdjustment, resolveEquidistanceGuides, SNAP_THRESHOLD_PX } from "./snapEngine"; +import { + resolveSnapAdjustment, + resolveEquidistanceGuides, + snapEngagedForTravel, + SNAP_THRESHOLD_PX, +} from "./snapEngine"; import { logResize, logResizeMove, logResizeSettle } from "../../utils/resizeDebug"; +import { logDrag, logDragSettle, readDragPositions } from "../../utils/dragDebug"; +import { createGroupDragMover } from "./groupDragMove"; export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGesturesOptions) { const setDraftOverlayRect = (next: OverlayRect) => { @@ -91,6 +97,8 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu }, ) => _startGesture(kind, e, opts, options); + const moveGroupDrag = createGroupDragMover(opts, setDraftGroupOverlayItems); + // fallow-ignore-next-line complexity const onPointerMove = (e: React.PointerEvent) => { const g = opts.gestureRef.current; @@ -114,55 +122,7 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu } if (groupG) { - let dx = e.clientX - groupG.startX; - let dy = e.clientY - groupG.startY; - - const sc = groupG.snapContext; - if (sc?.snapEnabled && sc.targets.length > 0) { - const groupBounds = resolveDomEditGroupOverlayRect( - groupG.originItems.map((item) => item.rect), - ); - if (groupBounds) { - const allTargets = sc.compositionTarget - ? [...sc.targets, sc.compositionTarget] - : sc.targets; - const snap = resolveSnapAdjustment({ - movingRect: groupBounds, - proposedDx: dx, - proposedDy: dy, - targets: allTargets, - gridEdges: sc.gridEdges ?? undefined, - threshold: SNAP_THRESHOLD_PX, - disabled: e.altKey, - }); - dx = snap.dx; - dy = snap.dy; - const movedRect = { - left: groupBounds.left + dx, - top: groupBounds.top + dy, - width: groupBounds.width, - height: groupBounds.height, - }; - const spacingGuides = e.altKey - ? [] - : resolveEquidistanceGuides({ - movingRect: movedRect, - targets: allTargets, - threshold: SNAP_THRESHOLD_PX, - }); - opts.snapGuidesRef.current = { guides: snap.guides, spacingGuides }; - } - } - groupG.lastSnappedDx = dx; - groupG.lastSnappedDy = dy; - - setDraftGroupOverlayItems( - groupG.originItems.map((item) => ({ - ...item, - rect: { ...item.rect, left: item.rect.left + dx, top: item.rect.top + dy }, - })), - ); - for (const member of groupG.members) applyManualOffsetDragDraft(member, dx, dy); + moveGroupDrag(groupG, e); return; } @@ -215,6 +175,9 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu movingRect, proposedDx: dx, proposedDy: dy, + // Same reason as the group path: a snap on a drag that has not travelled + // yet moves the element while the pointer is still. + disabledForTravel: !snapEngagedForTravel(dx, dy), targets: allTargets, gridEdges: sc.gridEdges ?? undefined, threshold: SNAP_THRESHOLD_PX, @@ -319,9 +282,14 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu opts.rafPausedRef.current = false; const rawDx = e.clientX - groupG.startX; const rawDy = e.clientY - groupG.startY; + // The click that trails every pointerup has to be eaten either way. The + // gesture ref is already cleared above, so by the time it arrives the box + // no longer looks busy, and handleBoxClick hands it to the canvas as an + // ordinary click — which lands between the members, resolves to nothing, + // and deselects the group the drag just moved. + opts.suppressNextBoxClickRef.current = true; if (Math.hypot(rawDx, rawDy) < BLOCKED_MOVE_THRESHOLD_PX) { restoreGroupPathOffsets(groupG); - opts.suppressNextBoxClickRef.current = true; return; } const dx = groupG.lastSnappedDx ?? rawDx; @@ -336,6 +304,17 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu selection: member.selection, next: applyManualOffsetDragCommit(member, dx, dy), })); + logDrag("drop", { + pointer: `${Math.round(rawDx)},${Math.round(rawDy)}`, + applied: `${Math.round(dx)},${Math.round(dy)}`, + committed: Object.fromEntries( + updates.map((update, index) => [ + groupG.members[index]?.key ?? String(index), + `${Math.round(update.next.x)},${Math.round(update.next.y)}`, + ]), + ), + at: readDragPositions(groupG.members), + }); void Promise.resolve(opts.onGroupPathOffsetCommitRef.current(updates)) .catch(() => { for (const member of groupG.members) { @@ -346,7 +325,15 @@ export function createDomEditOverlayGestureHandlers(opts: UseDomEditOverlayGestu restoreStudioPathOffset(member.element, member.initialPathOffset); } }) - .finally(() => endManualOffsetDragMembers(groupG.members)); + .finally(() => { + logDrag("committed", { at: readDragPositions(groupG.members) }); + endManualOffsetDragMembers(groupG.members); + // The gesture teardown resumes the paused timelines and re-seeks the + // player, which re-renders from whatever the preview currently holds. + // If the reloaded source has not landed yet that is the OLD position, + // so this is where a snap-back would show. + logDragSettle("settle", groupG.members); + }); return; } diff --git a/packages/studio/src/components/editor/useDomEditOverlayRects.ts b/packages/studio/src/components/editor/useDomEditOverlayRects.ts index b45ff6f26c..ba402bafb6 100644 --- a/packages/studio/src/components/editor/useDomEditOverlayRects.ts +++ b/packages/studio/src/components/editor/useDomEditOverlayRects.ts @@ -17,7 +17,7 @@ import { rectsEqual, resolveElementForOverlay, selectionCacheKey, - toVisibleOverlayRect, + orientedVisibleOverlayRect, } from "./domEditOverlayGeometry"; function childRectsEqual(a: OverlayRect[], b: OverlayRect[]): boolean { @@ -172,7 +172,9 @@ export function useDomEditOverlayRects({ for (let i = 0; i < descendants.length; i++) { const child = descendants[i] as HTMLElement; if (!child.getBoundingClientRect) continue; - const r = toVisibleOverlayRect(overlayEl, iframe, child); + // Oriented, not axis-aligned: a child of a rotated element drew its + // outline square around the rotated glyphs instead of on them. + const r = orientedVisibleOverlayRect(overlayEl, iframe, child); if (r && r.width > 2 && r.height > 2) nextChildRects.push(r); } if (!childRectsEqual(childRectsRef.current, nextChildRects)) { diff --git a/packages/studio/src/hooks/domSelectionTimelineMirror.ts b/packages/studio/src/hooks/domSelectionTimelineMirror.ts new file mode 100644 index 0000000000..f67577925a --- /dev/null +++ b/packages/studio/src/hooks/domSelectionTimelineMirror.ts @@ -0,0 +1,73 @@ +import type { SelectElementOptions, TimelineElement } from "../player"; +import { findMatchingTimelineElementId, findTimelineIdByAncestor } from "../utils/studioHelpers"; +import type { DomEditSelection } from "../components/editor/domEditing"; +import { logSelect } from "../utils/selectDebug"; + +interface TimelineMirrorDeps { + timelineElements: TimelineElement[]; + getTimelineSelectionSet: () => ReadonlySet; + setSelectedTimelineElementId: (id: string | null, options?: SelectElementOptions) => void; + setTimelineSelectionSet: (ids: Set) => void; +} + +/** + * Mirror a canvas selection onto the timeline: the whole set first, then the + * primary as its anchor. + * + * The timeline is the source of truth for what is selected and it syncs back — + * whatever it holds replaces the canvas selection a moment later. Announcing only + * the primary therefore drops every other member. Worse, anchoring with + * `preserveSet` on an id the set does not yet contain empties the set outright, + * and an empty set syncs back as "nothing is selected" — which is how adding a + * second element, or moving a group, could wipe the selection instead of keeping + * it. Publishing the members first is what makes the anchor a member, so + * preserving the set is meaningful rather than destructive. + */ +export function announceTimelineSelection( + deps: TimelineMirrorDeps, + group: DomEditSelection[], + primary: DomEditSelection | null, +): void { + const { + timelineElements, + getTimelineSelectionSet, + setSelectedTimelineElementId, + setTimelineSelectionSet, + } = deps; + if (!primary) { + setTimelineSelectionSet(new Set()); + setSelectedTimelineElementId(null); + return; + } + const timelineIdFor = (selection: DomEditSelection) => + findMatchingTimelineElementId(selection, timelineElements) ?? + findTimelineIdByAncestor( + selection.element, + timelineElements, + selection.sourceFile || "index.html", + ); + const members = group.map(timelineIdFor).filter((id): id is string => Boolean(id)); + const anchor = timelineIdFor(primary); + const publishedMembers = new Set(members); + if (anchor) publishedMembers.add(anchor); + const timelineAnchor = anchor ?? members[0] ?? null; + // A member with no timeline row of its own resolves to null and is dropped here, + // so a group can announce fewer ids than it has — or none, which reads back as an + // empty selection and takes the canvas selection with it. + logSelect("announce", { + group: group.length, + published: publishedMembers.size, + anchor, + anchorPublished: anchor != null && publishedMembers.has(anchor), + }); + // A canvas target can be editable without owning a timeline row. Preserve that + // canvas-only selection when the timeline has nothing truthful to represent. + if (!timelineAnchor) return; + // A late async primary that already belongs to the live set must preserve the + // group. A fresh single click does not belong to it, so publish the singleton + // first; otherwise `preserveSet` clears the set and sync wipes the canvas. + if (group.length > 1 || !getTimelineSelectionSet().has(timelineAnchor)) { + setTimelineSelectionSet(publishedMembers); + } + setSelectedTimelineElementId(timelineAnchor, { preserveSet: true }); +} diff --git a/packages/studio/src/hooks/gestureTransaction.test.ts b/packages/studio/src/hooks/gestureTransaction.test.ts index 3cc8e45530..8ff3ff28f8 100644 --- a/packages/studio/src/hooks/gestureTransaction.test.ts +++ b/packages/studio/src/hooks/gestureTransaction.test.ts @@ -36,6 +36,7 @@ function runTwoMutationTransaction( describe("runGestureTransaction", () => { beforeEach(() => { trackStudioEventMock.mockReset(); + localStorage.clear(); }); it("settles synchronously before persist reaches its first await", async () => { @@ -249,7 +250,7 @@ describe("runGestureTransaction", () => { .spyOn(element, "getBoundingClientRect") .mockReturnValueOnce(rect(10.04, 20.05, 100.05, 80.05)) .mockReturnValueOnce(rect(11.19, 17.89, 100.29, 78.99)); - const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); const now = vi.spyOn(performance, "now").mockReturnValueOnce(50).mockReturnValueOnce(58.44); await runGestureTransaction({ @@ -261,13 +262,7 @@ describe("runGestureTransaction", () => { }); expect(getRect).toHaveBeenCalledTimes(2); - expect(error).toHaveBeenCalledWith( - "[hf-commit] persist changed pixels", - expect.objectContaining({ - label: "Resize layer", - delta: expect.objectContaining({ x: expect.any(Number) }), - }), - ); + expect(log).not.toHaveBeenCalled(); expect(trackStudioEventMock).toHaveBeenCalledWith("commit_invariant_violation", { label: "Resize layer", delta_x: 1.2, @@ -283,13 +278,13 @@ describe("runGestureTransaction", () => { expect.objectContaining({ pixel_asserted: true }), ); now.mockRestore(); - error.mockRestore(); + log.mockRestore(); }); it("skips the pixel assertion for live position tweens", async () => { const element = document.createElement("div"); const getRect = vi.spyOn(element, "getBoundingClientRect"); - const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); await runGestureTransaction({ element, @@ -301,11 +296,11 @@ describe("runGestureTransaction", () => { }); expect(getRect).not.toHaveBeenCalled(); - expect(error).not.toHaveBeenCalledWith("[hf-commit] persist changed pixels", expect.anything()); + expect(log).not.toHaveBeenCalled(); expect(trackStudioEventMock).not.toHaveBeenCalledWith( "commit_invariant_violation", expect.anything(), ); - error.mockRestore(); + log.mockRestore(); }); }); diff --git a/packages/studio/src/hooks/gestureTransaction.ts b/packages/studio/src/hooks/gestureTransaction.ts index 4b398d6636..da7d61f487 100644 --- a/packages/studio/src/hooks/gestureTransaction.ts +++ b/packages/studio/src/hooks/gestureTransaction.ts @@ -4,6 +4,7 @@ import type { CommitMutationOptions, } from "./gsapScriptCommitTypes"; import { trackStudioEvent } from "../utils/studioTelemetry"; +import { makeStudioDebugLogger } from "../utils/studioDebug"; type PixelRect = Pick; @@ -108,14 +109,7 @@ async function dispatchBufferedCommits(calls: BufferedCommit[]): Promise return reloadsRequested(calls); } -/** - * Dev-only [hf-commit] lifecycle trace. The production observability lives in - * the trackStudioEvent commit_* events (always on); these console lines are a - * developer aid and stay out of end users' consoles. - */ -function traceCommit(stage: string, data: Record): void { - if (import.meta.env.DEV) console.info(`[hf-commit] ${stage}`, data); -} +const logCommit = makeStudioDebugLogger("commit"); /** * Owns the visual + persistence + history lifecycle for one gesture release. @@ -127,9 +121,9 @@ export function runGestureTransaction(tx: GestureTransaction): Promise { let mutationCount = 0; let reloadCount = 0; const bufferedCommits: BufferedCommit[] = []; - traceCommit("start", { label: tx.label, coalesceKey }); + logCommit("start", { label: tx.label, coalesceKey }); tx.settle(); - traceCommit("settled", { label: tx.label, coalesceKey }); + logCommit("settled", { label: tx.label, coalesceKey }); const before = !tx.skipPixelAssert ? readPixelRect(tx.element) : null; const commit: TxCommit = (commitMutation) => { @@ -152,19 +146,12 @@ export function runGestureTransaction(tx: GestureTransaction): Promise { .then(async () => { reloadCount = await dispatchBufferedCommits(bufferedCommits); const durationMs = Math.round(performance.now() - startedAt); - traceCommit("persisted", { label: tx.label, coalesceKey }); + logCommit("persisted", { label: tx.label, coalesceKey }); if (before) { const after = readPixelRect(tx.element); const delta = pixelDelta(before, after); if (exceedsPixelTolerance(delta)) { - if (import.meta.env.DEV) { - console.error("[hf-commit] persist changed pixels", { - label: tx.label, - before, - after, - delta, - }); - } + logCommit("persist-changed-pixels", { label: tx.label, before, after, delta }); trackStudioEvent("commit_invariant_violation", { label: tx.label, delta_x: roundToOneDecimal(delta.x), @@ -193,7 +180,7 @@ export function runGestureTransaction(tx: GestureTransaction): Promise { error_name: error instanceof Error ? error.name : "unknown", restore_ran: true, }); - traceCommit("restore", { label: tx.label, coalesceKey }); + logCommit("restore", { label: tx.label, coalesceKey }); throw error; }); } diff --git a/packages/studio/src/hooks/gsapRuntimePatch.test.ts b/packages/studio/src/hooks/gsapRuntimePatch.test.ts index 1a17f7e063..e34d293a41 100644 --- a/packages/studio/src/hooks/gsapRuntimePatch.test.ts +++ b/packages/studio/src/hooks/gsapRuntimePatch.test.ts @@ -523,3 +523,46 @@ describe("patchRuntimeTweenInPlace — composition isolation", () => { expect(otherTween.invalidate).not.toHaveBeenCalled(); }); }); + +describe("patchRuntimeTweenInPlace — deferSeek", () => { + /** + * A group drag commits one member at a time. Each in-place patch used to seek, + * and a seek re-renders the WHOLE timeline — so every member still queued behind + * the current one got repainted from its un-patched tween, back to where it sat + * before the drag, and stayed there until its own patch landed. That is the jump. + */ + it("does not seek while a group commit is still writing its other members", () => { + const a = { id: "a" }; + const rendered = { a: 0, b: 0 }; + const tweenA = makeTween({ vars: { x: 0 }, targetIds: ["a"], duration: 0 }, a); + const tweenB = makeTween({ vars: { x: 0 }, targetIds: ["b"], duration: 0 }, a); + const { iframe, seek } = fakeIframe(a, [tweenA, tweenB], { + onSeek: () => { + rendered.a = tweenA.vars.x as number; + rendered.b = tweenB.vars.x as number; + }, + }); + + const first = patchRuntimeTweenInPlace( + iframe, + "#a", + { kind: "set", props: { x: 500 } }, + undefined, + true, + ); + + expect(first).toBe(true); + expect(tweenA.vars.x).toBe(500); + // No repaint yet: "b" keeps the transform the gesture left on it instead of + // being rendered from its own tween, which still holds the pre-drag value. + expect(seek).not.toHaveBeenCalled(); + expect(rendered).toEqual({ a: 0, b: 0 }); + + tweenB.vars.x = 600; + const last = patchRuntimeTweenInPlace(iframe, "#a", { kind: "set", props: { x: 500 } }); + + expect(last).toBe(true); + expect(seek).toHaveBeenCalledTimes(1); + expect(rendered).toEqual({ a: 500, b: 600 }); + }); +}); diff --git a/packages/studio/src/hooks/gsapRuntimePatch.ts b/packages/studio/src/hooks/gsapRuntimePatch.ts index de2c6480e4..9d32650387 100644 --- a/packages/studio/src/hooks/gsapRuntimePatch.ts +++ b/packages/studio/src/hooks/gsapRuntimePatch.ts @@ -277,12 +277,16 @@ function applyChange(tween: RuntimeTween, change: RuntimeTweenChange): boolean { /** * Edit one tween in `window.__timelines` in place + re-seek to the current playhead. * Returns `true` on a confident patch, `false` otherwise (caller soft-reloads). + * + * `deferSeek` skips the re-render, for a caller patching several tweens in a row + * that will render once after the last one. */ export function patchRuntimeTweenInPlace( iframe: HTMLIFrameElement | null, selector: string, change: RuntimeTweenChange, compositionId?: string, + deferSeek = false, ): boolean { if (!iframe) return false; // A base `gsap.set` has no timeline tween to resolve — apply the value straight @@ -312,7 +316,13 @@ export function patchRuntimeTweenInPlace( if (change.kind !== "keyframe-rebuild") { tween.invalidate?.(); } - seekToCurrent(iframe, timeline); + // A seek re-renders the WHOLE timeline, not just the tween we patched. Under a + // multi-element commit that is a visible jump: the members still queued behind + // this one get repainted from their un-patched tweens, back to where they were + // before the gesture, and stay there until their own patch lands. Deferring + // leaves them showing the gesture's own transform, and the caller's last patch + // seeks once for the whole group. + if (!deferSeek) seekToCurrent(iframe, timeline); return true; } catch { return false; diff --git a/packages/studio/src/hooks/gsapScriptCommitTypes.ts b/packages/studio/src/hooks/gsapScriptCommitTypes.ts index 929e79b8ec..85071ab6d9 100644 --- a/packages/studio/src/hooks/gsapScriptCommitTypes.ts +++ b/packages/studio/src/hooks/gsapScriptCommitTypes.ts @@ -22,6 +22,18 @@ export interface CommitMutationOptions { coalesceMs?: number; softReload?: boolean; skipReload?: boolean; + /** + * Write the source but leave the preview alone; the caller renders once when it + * is done. For a multi-write action like a group drag, rendering after each + * write shows a source where the members not yet written still hold their old + * values, so they snap back until their own write lands. This also defers the + * in-place runtime patch's seek, which re-renders the whole timeline and repaints + * the queued members the same way. Unlike `skipReload` this changes nothing about + * error handling — a failed write still throws. + */ + deferPreviewSync?: boolean; + /** Shares an in-place patch miss with the final render of one multi-write action. */ + previewFallbackLatch?: { pending: boolean }; beforeReload?: () => void; /** * Serialize this commit against others sharing the same key. Used to chain @@ -39,6 +51,14 @@ export interface CommitMutationOptions { * existing soft/full reload path. Structural edits omit this and reload as before. */ instantPatch?: { selector: string; change: RuntimeTweenChange }; + /** + * The same fast path for a batched commit: one patch per element the batch + * wrote, applied in order. All of them must land for the reload to be skipped + * — one that can't be applied leaves the preview half-patched, so the whole + * batch falls back to the reload. Only the last patch re-renders (see + * `deferSeek`), so a ten-element batch repaints once. + */ + instantPatches?: Array<{ selector: string; change: RuntimeTweenChange }>; } export interface CommitMutationCall { diff --git a/packages/studio/src/hooks/keyframeCacheAstLoad.test.ts b/packages/studio/src/hooks/keyframeCacheAstLoad.test.ts new file mode 100644 index 0000000000..05d5c95d4c --- /dev/null +++ b/packages/studio/src/hooks/keyframeCacheAstLoad.test.ts @@ -0,0 +1,108 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { fetchParsedAnimations } from "./keyframeCacheAstLoad"; + +/** + * Parsing a composition is a whole-file read + parse on the server, and a + * multi-element action asks for the same file once per element. Callers that + * overlap in time share one request; a caller that comes after the last one + * settled does not, so a parse issued after a write is never served a + * pre-write answer. + */ +describe("fetchParsedAnimations — in-flight sharing", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + function stubFetch(): { calls: () => number; settle: () => void } { + let calls = 0; + const pending: Array<() => void> = []; + vi.stubGlobal("fetch", () => { + calls++; + return new Promise((resolve) => { + pending.push(() => + resolve({ + ok: true, + json: () => Promise.resolve({ animations: [{ id: "a", targetSelector: "#a" }] }), + } as Response), + ); + }); + }); + return { + calls: () => calls, + settle: () => { + for (const release of pending.splice(0, pending.length)) release(); + }, + }; + } + + it("serves overlapping reads of one file from a single request", async () => { + const fetchStub = stubFetch(); + + const pending = [ + fetchParsedAnimations("p", "index.html"), + fetchParsedAnimations("p", "index.html"), + fetchParsedAnimations("p", "index.html"), + ]; + fetchStub.settle(); + const results = await Promise.all(pending); + + expect(fetchStub.calls()).toBe(1); + expect(results.map((parsed) => parsed?.animations.length)).toEqual([1, 1, 1]); + }); + + it("does not share across files", async () => { + const fetchStub = stubFetch(); + + const pending = [ + fetchParsedAnimations("p", "index.html"), + fetchParsedAnimations("p", "other.html"), + ]; + fetchStub.settle(); + await Promise.all(pending); + + expect(fetchStub.calls()).toBe(2); + }); + + it("re-requests once the previous read has settled", async () => { + const fetchStub = stubFetch(); + + const first = fetchParsedAnimations("p", "index.html"); + fetchStub.settle(); + await first; + const second = fetchParsedAnimations("p", "index.html"); + fetchStub.settle(); + await second; + + expect(fetchStub.calls()).toBe(2); + }); + + it("supersedes an in-flight pre-write parse with a fresh post-write read", async () => { + const releases: Array<(response: Response) => void> = []; + const fetch = vi.fn( + () => + new Promise((resolve) => { + releases.push(resolve); + }), + ); + vi.stubGlobal("fetch", fetch); + const response = (id: string) => + ({ + ok: true, + json: () => Promise.resolve({ animations: [{ id, targetSelector: `#${id}` }] }), + }) as Response; + + const stale = fetchParsedAnimations("p", "index.html"); + const fresh = fetchParsedAnimations("p", "index.html", { fresh: true }); + expect(fetch).toHaveBeenCalledTimes(2); + + releases[0]?.(response("stale")); + await stale; + const overlappingFreshRead = fetchParsedAnimations("p", "index.html"); + expect(fetch).toHaveBeenCalledTimes(2); + + releases[1]?.(response("fresh")); + const [freshResult, sharedResult] = await Promise.all([fresh, overlappingFreshRead]); + expect(freshResult?.animations[0]?.id).toBe("fresh"); + expect(sharedResult?.animations[0]?.id).toBe("fresh"); + }); +}); diff --git a/packages/studio/src/hooks/keyframeCacheAstLoad.ts b/packages/studio/src/hooks/keyframeCacheAstLoad.ts index 71e38bbb61..390208e328 100644 --- a/packages/studio/src/hooks/keyframeCacheAstLoad.ts +++ b/packages/studio/src/hooks/keyframeCacheAstLoad.ts @@ -42,7 +42,35 @@ function hasAnimations(value: unknown): value is ParsedGsapAnimations { ); } -export async function fetchParsedAnimations( +/** + * Requests for the same file that overlap in time, keyed `projectId|sourceFile`. + * + * Every parse re-reads and re-parses the whole composition server-side, and a + * multi-element action asks for the same file once per element. Sharing the + * in-flight promise makes that one request. Only OVERLAPPING calls share: the + * entry is dropped the moment it settles, so a call made after a write still + * gets a fresh parse. + */ +const inFlightParses = new Map>(); + +export function fetchParsedAnimations( + projectId: string, + sourceFile: string, + options: { fresh?: boolean } = {}, +): Promise { + const key = `${projectId}|${sourceFile}`; + if (options.fresh) inFlightParses.delete(key); + const inFlight = inFlightParses.get(key); + if (inFlight) return inFlight; + const request = requestParsedAnimations(projectId, sourceFile).finally(() => { + // A superseded pre-write request must not evict the fresh post-write one. + if (inFlightParses.get(key) === request) inFlightParses.delete(key); + }); + inFlightParses.set(key, request); + return request; +} + +async function requestParsedAnimations( projectId: string, sourceFile: string, ): Promise { diff --git a/packages/studio/src/hooks/useDomEditPreviewSync.ts b/packages/studio/src/hooks/useDomEditPreviewSync.ts index 6a288b6ee6..cb0c334fe1 100644 --- a/packages/studio/src/hooks/useDomEditPreviewSync.ts +++ b/packages/studio/src/hooks/useDomEditPreviewSync.ts @@ -8,13 +8,17 @@ import { findElementForSelection, type DomEditSelection } from "../components/ed import { reapplyPositionEditsAfterSeek } from "../components/editor/manualEdits"; import type { SidebarTab } from "../components/sidebar/LeftSidebar"; import type { PatchTarget } from "../utils/sourcePatcher"; +import { logSelect } from "../utils/selectDebug"; interface UseDomEditPreviewSyncParams { previewIframe: HTMLIFrameElement | null; activeCompPath: string | null; captionEditMode: boolean; domEditSelectionRef: React.MutableRefObject; + domEditGroupSelectionsRef: React.MutableRefObject; domEditSelection: DomEditSelection | null; + /** Re-resolves a whole multi-selection against the current preview document. */ + refreshDomEditGroupSelectionsFromPreview: (selections: DomEditSelection[]) => Promise; applyDomSelection: ( selection: DomEditSelection | null, options?: { revealPanel?: boolean; preserveGroup?: boolean }, @@ -35,8 +39,10 @@ export function useDomEditPreviewSync({ activeCompPath, captionEditMode, domEditSelectionRef, + domEditGroupSelectionsRef, domEditSelection, applyDomSelection, + refreshDomEditGroupSelectionsFromPreview, buildDomSelectionFromTarget, refreshPreviewDocumentVersion, syncPreviewHistoryHotkey, @@ -72,6 +78,21 @@ export function useDomEditPreviewSync({ // Clear so overlay geometry isn't computed on a stale, detached node. // (Drag-release-in-gray-zone is handled separately by // suppressNextBoxClickRef; the dragged element still resolves here.) + // + // One lost member is not the whole selection though. A multi-select that + // loses its primary here used to be wiped entirely, so moving a group and + // having any one of its elements fail to re-resolve deselected all of + // them. Re-resolve the group instead and keep whoever survived; it only + // clears when nobody did. + const group = domEditGroupSelectionsRef.current; + logSelect("preview-sync-lost", { + target: currentSelection.selector ?? currentSelection.id ?? null, + group: group.length, + }); + if (group.length > 1) { + await refreshDomEditGroupSelectionsFromPreview(group); + return; + } applyDomSelection(null, { revealPanel: false }); return; } @@ -103,8 +124,10 @@ export function useDomEditPreviewSync({ applyDomSelection, buildDomSelectionFromTarget, captionEditMode, + domEditGroupSelectionsRef, domEditSelectionRef, previewIframe, + refreshDomEditGroupSelectionsFromPreview, refreshPreviewDocumentVersion, syncPreviewHistoryHotkey, applyStudioManualEditsToPreviewRef, diff --git a/packages/studio/src/hooks/useDomEditSession.test.tsx b/packages/studio/src/hooks/useDomEditSession.test.tsx index 3c5a3e0ffd..e629b2d1b1 100644 --- a/packages/studio/src/hooks/useDomEditSession.test.tsx +++ b/packages/studio/src/hooks/useDomEditSession.test.tsx @@ -60,6 +60,46 @@ const capturedOnReorderShadow: { fn: ((targets: string[]) => void) | undefined } const domEditSelectionRef: { current: DomEditSelection | null } = { current: null }; const gsapCommitMutation = Object.assign(vi.fn(), { batch: vi.fn() }); +function createSessionParams( + overrides: Partial = {}, +): UseDomEditSessionParams { + return { + projectId: "proj-1", + activeCompPath: "index.html", + compIdToSrc: new Map(), + captionEditMode: false, + compositionLoading: false, + previewIframeRef: { current: null }, + timelineElements: [], + getTimelineSelectionSet: () => new Set(), + setSelectedTimelineElementId: vi.fn(), + setTimelineSelectionSet: vi.fn(), + setRightCollapsed: vi.fn(), + setRightPanelTab: vi.fn(), + showToast: vi.fn(), + refreshPreviewDocumentVersion: vi.fn(), + queueDomEditSave: async (save: () => Promise) => save(), + readProjectFile: async () => "", + writeProjectFile: async () => {}, + updateEditingFileContent: vi.fn(), + domEditSaveTimestampRef: { current: 0 }, + editHistory: { recordEdit: async () => {} }, + fileTree: [], + importedFontAssetsRef: { current: [] }, + projectDir: null, + projectIdRef: { current: "proj-1" }, + previewIframe: null, + refreshKey: 0, + previewDocumentVersion: 0, + rightPanelTab: "design", + applyStudioManualEditsToPreviewRef: { current: async () => {} }, + syncPreviewHistoryHotkey: vi.fn(), + reloadPreview: vi.fn(), + setRefreshKey: vi.fn(), + ...overrides, + }; +} + vi.mock("../utils/sdkResolverShadow", () => ({ runResolverShadow: vi.fn(), recordResolverParity: (...args: unknown[]) => recordResolverParity(...args), @@ -220,43 +260,16 @@ describe("onReorderShadow source filter", () => { const sdkSession = {} as unknown as Composition; function Probe() { - const params: UseDomEditSessionParams = { - projectId: "proj-1", - activeCompPath: "index.html", - isMasterView: false, - compIdToSrc: new Map(), - captionEditMode: false, - compositionLoading: false, - previewIframeRef: { current: null }, - timelineElements: [], - setSelectedTimelineElementId: vi.fn(), - setRightCollapsed: vi.fn(), - setRightPanelTab: vi.fn(), - showToast: vi.fn(), - refreshPreviewDocumentVersion: vi.fn(), + const params = createSessionParams({ queueDomEditSave: vi.fn(async (save: () => Promise) => save()) as ( save: () => Promise, ) => Promise, readProjectFile, writeProjectFile: vi.fn(async () => {}), - updateEditingFileContent: vi.fn(), - domEditSaveTimestampRef: { current: 0 }, editHistory: { recordEdit: vi.fn(async () => {}) }, - fileTree: [], - importedFontAssetsRef: { current: [] }, - projectDir: null, - projectIdRef: { current: "proj-1" }, - previewIframe: null, - refreshKey: 0, - previewDocumentVersion: 0, - rightPanelTab: "design", - applyStudioManualEditsToPreviewRef: { current: async () => {} }, - syncPreviewHistoryHotkey: vi.fn(), - reloadPreview: vi.fn(), - setRefreshKey: vi.fn(), sdkSession, forceReloadSdkSession: vi.fn(), - }; + }); useDomEditSession(params); return null; } @@ -318,39 +331,7 @@ describe("bulk segment ease commits", () => { | undefined; function Probe() { - const params: UseDomEditSessionParams = { - projectId: "proj-1", - activeCompPath: "index.html", - isMasterView: false, - compIdToSrc: new Map(), - captionEditMode: false, - compositionLoading: false, - previewIframeRef: { current: null }, - timelineElements: [], - setSelectedTimelineElementId: vi.fn(), - setRightCollapsed: vi.fn(), - setRightPanelTab: vi.fn(), - showToast: vi.fn(), - refreshPreviewDocumentVersion: vi.fn(), - queueDomEditSave: async (save: () => Promise) => save(), - readProjectFile: async () => "", - writeProjectFile: async () => {}, - updateEditingFileContent: vi.fn(), - domEditSaveTimestampRef: { current: 0 }, - editHistory: { recordEdit: async () => {} }, - fileTree: [], - importedFontAssetsRef: { current: [] }, - projectDir: null, - projectIdRef: { current: "proj-1" }, - previewIframe: null, - refreshKey: 0, - previewDocumentVersion: 0, - rightPanelTab: "design", - applyStudioManualEditsToPreviewRef: { current: async () => {} }, - syncPreviewHistoryHotkey: vi.fn(), - reloadPreview: vi.fn(), - setRefreshKey: vi.fn(), - }; + const params = createSessionParams(); updateSegmentEase = useDomEditSession(params).handleUpdateSegmentEase; return null; } diff --git a/packages/studio/src/hooks/useDomEditSession.ts b/packages/studio/src/hooks/useDomEditSession.ts index efec011f95..1ad7ab3fce 100644 --- a/packages/studio/src/hooks/useDomEditSession.ts +++ b/packages/studio/src/hooks/useDomEditSession.ts @@ -31,13 +31,14 @@ interface RecordEditInput { export interface UseDomEditSessionParams { projectId: string | null; activeCompPath: string | null; - isMasterView: boolean; compIdToSrc: Map; captionEditMode: boolean; compositionLoading: boolean; previewIframeRef: React.MutableRefObject; timelineElements: TimelineElement[]; + getTimelineSelectionSet: () => ReadonlySet; setSelectedTimelineElementId: (id: string | null, options?: SelectElementOptions) => void; + setTimelineSelectionSet: (ids: Set) => void; setRightCollapsed: (collapsed: boolean) => void; setRightPanelTab: (tab: RightPanelTab) => void; showToast: (message: string, tone?: "error" | "info") => void; @@ -73,13 +74,14 @@ export interface UseDomEditSessionParams { export function useDomEditSession({ projectId, activeCompPath, - isMasterView, compIdToSrc, captionEditMode, compositionLoading, previewIframeRef, timelineElements, + getTimelineSelectionSet, setSelectedTimelineElementId, + setTimelineSelectionSet, setRightCollapsed, setRightPanelTab, showToast, @@ -109,6 +111,7 @@ export function useDomEditSession({ publishSdkSession, forceReloadSdkSession, }: UseDomEditSessionParams) { + const isMasterView = !activeCompPath || activeCompPath === "index.html"; void _setRefreshKey; const { domEditSelection, @@ -127,6 +130,7 @@ export function useDomEditSession({ buildDomSelectionForTimelineElement, handleTimelineElementSelect, refreshDomEditSelectionFromPreview, + refreshDomEditGroupSelectionsFromPreview, applyMarqueeSelection, } = useDomSelection({ projectId, @@ -136,7 +140,9 @@ export function useDomEditSession({ captionEditMode, previewIframeRef, timelineElements, + getTimelineSelectionSet, setSelectedTimelineElementId, + setTimelineSelectionSet, setRightCollapsed, setRightPanelTab, previewIframe, @@ -382,6 +388,8 @@ export function useDomEditSession({ activeCompPath, domEditSelection, domEditSelectionRef, + domEditGroupSelectionsRef, + refreshDomEditGroupSelectionsFromPreview, previewIframeRef, previewIframe, captionEditMode, diff --git a/packages/studio/src/hooks/useDomEditWiring.ts b/packages/studio/src/hooks/useDomEditWiring.ts index fbd049482e..9b49f16fa7 100644 --- a/packages/studio/src/hooks/useDomEditWiring.ts +++ b/packages/studio/src/hooks/useDomEditWiring.ts @@ -23,6 +23,8 @@ export interface UseDomEditWiringParams { activeCompPath: string | null; domEditSelection: DomEditSelection | null; domEditSelectionRef: React.MutableRefObject; + domEditGroupSelectionsRef: React.MutableRefObject; + refreshDomEditGroupSelectionsFromPreview: (selections: DomEditSelection[]) => Promise; previewIframeRef: React.RefObject; previewIframe: HTMLIFrameElement | null; captionEditMode: boolean; @@ -115,6 +117,8 @@ export function useDomEditWiring({ activeCompPath, domEditSelection, domEditSelectionRef, + domEditGroupSelectionsRef, + refreshDomEditGroupSelectionsFromPreview, previewIframeRef, previewIframe, captionEditMode, @@ -254,8 +258,10 @@ export function useDomEditWiring({ activeCompPath, captionEditMode, domEditSelectionRef, + domEditGroupSelectionsRef, domEditSelection, applyDomSelection, + refreshDomEditGroupSelectionsFromPreview, buildDomSelectionFromTarget, refreshPreviewDocumentVersion, syncPreviewHistoryHotkey, diff --git a/packages/studio/src/hooks/useDomSelection.test.ts b/packages/studio/src/hooks/useDomSelection.test.ts index 53f4524644..4b407c35e4 100644 --- a/packages/studio/src/hooks/useDomSelection.test.ts +++ b/packages/studio/src/hooks/useDomSelection.test.ts @@ -5,6 +5,7 @@ import { createRoot } from "react-dom/client"; import { describe, expect, it, vi } from "vitest"; import { installReactActEnvironment, makeSelection } from "./domSelectionTestHarness"; import { useDomSelection } from "./useDomSelection"; +import type { TimelineElement } from "../player"; installReactActEnvironment(); @@ -14,11 +15,24 @@ interface HarnessProps { refreshKey: number; } -function renderHarness(initialProps: HarnessProps): { +interface TimelineSpies { + setSelectedTimelineElementId: ReturnType; + setTimelineSelectionSet: ReturnType; +} + +function renderHarness( + initialProps: HarnessProps, + options: { timelineElements?: TimelineElement[] } = {}, +): { current: () => ReturnType; rerender: (props: HarnessProps) => void; cleanup: () => void; + timeline: TimelineSpies; } { + const timeline: TimelineSpies = { + setSelectedTimelineElementId: vi.fn(), + setTimelineSelectionSet: vi.fn(), + }; const host = document.createElement("div"); document.body.append(host); const root = createRoot(host); @@ -32,8 +46,10 @@ function renderHarness(initialProps: HarnessProps): { compIdToSrc: new Map(), captionEditMode: false, previewIframeRef: { current: null }, - timelineElements: [], - setSelectedTimelineElementId: vi.fn(), + timelineElements: options.timelineElements ?? [], + getTimelineSelectionSet: () => new Set(), + setSelectedTimelineElementId: timeline.setSelectedTimelineElementId, + setTimelineSelectionSet: timeline.setTimelineSelectionSet, setRightCollapsed: vi.fn(), setRightPanelTab: vi.fn(), previewIframe: null, @@ -61,6 +77,7 @@ function renderHarness(initialProps: HarnessProps): { act(() => root.unmount()); host.remove(); }, + timeline, }; } @@ -77,6 +94,119 @@ function setupSelectedHarness() { return { selection, harness }; } +function timelineElement(domId: string): TimelineElement { + return { + id: domId, + key: domId, + domId, + tag: "div", + start: 0, + duration: 1, + track: 0, + sourceFile: "index.html", + } as TimelineElement; +} + +/** + * A marquee builds the group correctly and then used to lose it: it announced only + * the primary to the timeline, the timeline is the source of truth for what is + * selected, and the sync back to the canvas replaced the group with that one + * element a moment after the drop. The whole set has to be announced, with the + * primary as its anchor rather than as a new single selection. + */ +describe("useDomSelection marquee", () => { + it("announces every marquee'd element to the timeline, anchored on the primary", () => { + const first = document.createElement("div"); + first.id = "card"; + const second = document.createElement("div"); + second.id = "chip"; + document.body.append(first, second); + const harness = renderHarness( + { activeCompPath: "index.html", projectId: "project-1", refreshKey: 0 }, + { timelineElements: [timelineElement("card"), timelineElement("chip")] }, + ); + + act(() => + harness + .current() + .applyMarqueeSelection( + [makeSelection("Card", first), makeSelection("Chip", second)], + false, + ), + ); + + expect(harness.current().domEditGroupSelections).toHaveLength(2); + expect(harness.timeline.setTimelineSelectionSet).toHaveBeenCalledWith( + new Set(["card", "chip"]), + ); + expect(harness.timeline.setSelectedTimelineElementId).toHaveBeenCalledWith("card", { + preserveSet: true, + }); + harness.cleanup(); + }); + + it("uses a surviving group member as the timeline anchor when the canvas primary has no row", () => { + const canvasOnly = document.createElement("div"); + canvasOnly.id = "canvas-only"; + const card = document.createElement("div"); + card.id = "card"; + document.body.append(canvasOnly, card); + const harness = renderHarness( + { activeCompPath: "index.html", projectId: "project-1", refreshKey: 0 }, + { timelineElements: [timelineElement("card")] }, + ); + + act(() => + harness + .current() + .applyMarqueeSelection( + [makeSelection("Canvas only", canvasOnly), makeSelection("Card", card)], + false, + ), + ); + + expect(harness.timeline.setTimelineSelectionSet).toHaveBeenCalledWith(new Set(["card"])); + expect(harness.timeline.setSelectedTimelineElementId).toHaveBeenCalledWith("card", { + preserveSet: true, + }); + harness.cleanup(); + }); +}); + +/** + * Adding a second element announced only that element, with preserveSet — and + * preserving a set that does not contain the id empties it. An empty timeline + * selection syncs back as "nothing is selected", so growing a group could wipe + * it instead, and so could re-resolving one after a move. + */ +describe("useDomSelection additive", () => { + it("announces both members when a second element joins the selection", () => { + const first = document.createElement("div"); + first.id = "card"; + const second = document.createElement("div"); + second.id = "chip"; + document.body.append(first, second); + const harness = renderHarness( + { activeCompPath: "index.html", projectId: "project-1", refreshKey: 0 }, + { timelineElements: [timelineElement("card"), timelineElement("chip")] }, + ); + + act(() => harness.current().applyDomSelection(makeSelection("Card", first))); + act(() => + harness.current().applyDomSelection(makeSelection("Chip", second), { additive: true }), + ); + + expect(harness.current().domEditGroupSelections).toHaveLength(2); + expect(harness.timeline.setTimelineSelectionSet).toHaveBeenLastCalledWith( + new Set(["card", "chip"]), + ); + expect(harness.timeline.setSelectedTimelineElementId).toHaveBeenLastCalledWith("chip", { + preserveSet: true, + }); + harness.cleanup(); + }); +}); + describe("useDomSelection", () => { it("clears a committed selection when the active composition path changes", () => { const { selection, harness } = setupSelectedHarness(); diff --git a/packages/studio/src/hooks/useDomSelection.ts b/packages/studio/src/hooks/useDomSelection.ts index 32682b0810..115d54fca2 100644 --- a/packages/studio/src/hooks/useDomSelection.ts +++ b/packages/studio/src/hooks/useDomSelection.ts @@ -4,11 +4,7 @@ import { getAllPreviewTargetsFromPointer, getPreviewTargetFromPointer, } from "../utils/studioPreviewHelpers"; -import { - findMatchingTimelineElementId, - findTimelineIdByAncestor, - type RightPanelTab, -} from "../utils/studioHelpers"; +import { type RightPanelTab } from "../utils/studioHelpers"; import { domEditSelectionsTargetSame, domEditSelectionInGroup, @@ -24,6 +20,8 @@ import { } from "../components/editor/domEditing"; import { reapplyPositionEditsAfterSeek } from "../components/editor/manualEdits"; import { useStudioTestHooks } from "./useStudioTestHooks"; +import { logSelect } from "../utils/selectDebug"; +import { announceTimelineSelection as announceSelectionToTimeline } from "./domSelectionTimelineMirror"; // ── Types ── @@ -47,7 +45,10 @@ export interface UseDomSelectionParams { captionEditMode: boolean; previewIframeRef: React.MutableRefObject; timelineElements: TimelineElement[]; + getTimelineSelectionSet: () => ReadonlySet; setSelectedTimelineElementId: (id: string | null, options?: SelectElementOptions) => void; + /** Publishes a whole multi-selection to the timeline; the anchor is set separately. */ + setTimelineSelectionSet: (ids: Set) => void; setRightCollapsed: (collapsed: boolean) => void; setRightPanelTab: (tab: RightPanelTab) => void; previewIframe: HTMLIFrameElement | null; @@ -109,7 +110,9 @@ export function useDomSelection({ captionEditMode, previewIframeRef, timelineElements, + getTimelineSelectionSet, setSelectedTimelineElementId, + setTimelineSelectionSet, setRightCollapsed, setRightPanelTab, previewIframe, @@ -145,6 +148,26 @@ export function useDomSelection({ // ── Callbacks ── + const announceTimelineSelection = useCallback( + (group: DomEditSelection[], primary: DomEditSelection | null) => + announceSelectionToTimeline( + { + timelineElements, + getTimelineSelectionSet, + setSelectedTimelineElementId, + setTimelineSelectionSet, + }, + group, + primary, + ), + [ + getTimelineSelectionSet, + setSelectedTimelineElementId, + setTimelineSelectionSet, + timelineElements, + ], + ); + const applyDomSelection = useCallback( // fallow-ignore-next-line complexity ( @@ -156,11 +179,12 @@ export function useDomSelection({ }, ) => { if (!selection) { + logSelect("clear", { hadGroup: domEditGroupSelectionsRef.current.length }); domEditSelectionRef.current = null; domEditGroupSelectionsRef.current = []; setDomEditSelection(null); setDomEditGroupSelections([]); - setSelectedTimelineElementId(null); + announceTimelineSelection([], null); return; } @@ -186,6 +210,13 @@ export function useDomSelection({ : (nextGroup[0] ?? null) : selection; + logSelect("apply", { + additive: isAdditiveSelection, + target: selection.selector ?? selection.id ?? null, + wasInGroup, + prevGroup: previousGroup.length, + nextGroup: nextGroup.length, + }); domEditSelectionRef.current = nextSelection; domEditGroupSelectionsRef.current = nextGroup; setDomEditSelection(nextSelection); @@ -208,21 +239,13 @@ export function useDomSelection({ setRightPanelTab("design"); } } - const nextSelectedTimelineId = - findMatchingTimelineElementId(nextSelection, timelineElements) ?? - findTimelineIdByAncestor( - nextSelection.element, - timelineElements, - nextSelection.sourceFile || "index.html", - ); - // Late marquee notify: a primary already in the live set must not collapse it. - setSelectedTimelineElementId(nextSelectedTimelineId, { preserveSet: true }); + announceTimelineSelection(nextGroup, nextSelection); return; } - setSelectedTimelineElementId(null); + announceTimelineSelection([], null); }, - [setSelectedTimelineElementId, timelineElements, setRightCollapsed, setRightPanelTab], + [announceTimelineSelection, setRightCollapsed, setRightPanelTab], ); const clearDomSelection = useCallback(() => { @@ -375,6 +398,13 @@ export function useDomSelection({ [applyDomSelection, buildDomSelectionForTimelineElement], ); + // Forward handle to the group refresher defined below: the single-selection + // refresher falls back to it when the primary is gone, and a ref keeps that from + // forcing either callback to be declared in the other's dependency list. + const refreshDomEditGroupSelectionsFromPreviewRef = useRef< + (selections: DomEditSelection[]) => Promise + >(async () => {}); + const refreshDomEditSelectionFromPreview = useCallback( // fallow-ignore-next-line complexity async (selection: DomEditSelection) => { @@ -389,6 +419,17 @@ export function useDomSelection({ const element = findElementForSelection(doc, selection, activeCompPath); if (!element) { + // Losing the primary is not losing the selection. When a group is live, + // re-resolve it and keep whoever still exists rather than wiping the lot. + const group = domEditGroupSelectionsRef.current; + logSelect("refresh-lost", { + target: selection.selector ?? selection.id ?? null, + group: group.length, + }); + if (group.length > 1) { + await refreshDomEditGroupSelectionsFromPreviewRef.current(group); + return; + } applyDomSelection(null, { revealPanel: false }); return; } @@ -436,25 +477,17 @@ export function useDomSelection({ setDomEditSelection(nextSelection); setDomEditGroupSelections(nextGroup); - if (nextSelection) { - setSelectedTimelineElementId( - findMatchingTimelineElementId(nextSelection, timelineElements), - ); - } else { - setSelectedTimelineElementId(null); - } + announceTimelineSelection(nextGroup, nextSelection); }, - [ - activeCompPath, - buildDomSelectionFromTarget, - setSelectedTimelineElementId, - timelineElements, - previewIframeRef, - ], + [activeCompPath, announceTimelineSelection, buildDomSelectionFromTarget, previewIframeRef], ); // ── Effects ── + useEffect(() => { + refreshDomEditGroupSelectionsFromPreviewRef.current = refreshDomEditGroupSelectionsFromPreview; + }, [refreshDomEditGroupSelectionsFromPreview]); + // Clear hover unconditionally on composition/project/preview change // eslint-disable-next-line no-restricted-syntax useEffect(() => { @@ -503,6 +536,7 @@ export function useDomSelection({ const applyMarqueeSelection = useCallback( // fallow-ignore-next-line complexity (selections: DomEditSelection[], additive: boolean) => { + logSelect("marquee", { hits: selections.length, additive }); if (selections.length === 0) { if (!additive) applyDomSelection(null, { revealPanel: false }); return; @@ -527,16 +561,9 @@ export function useDomSelection({ domEditGroupSelectionsRef.current = nextGroup; setDomEditSelection(nextSelection); setDomEditGroupSelections(nextGroup); - const nextTimelineId = - findMatchingTimelineElementId(nextSelection, timelineElements) ?? - findTimelineIdByAncestor( - nextSelection.element, - timelineElements, - nextSelection.sourceFile || "index.html", - ); - setSelectedTimelineElementId(nextTimelineId); + announceTimelineSelection(nextGroup, nextSelection); }, - [applyDomSelection, timelineElements, setSelectedTimelineElementId], + [applyDomSelection, announceTimelineSelection], ); return { diff --git a/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts b/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts index dce0655946..a61ee38fa3 100644 --- a/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts +++ b/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts @@ -49,6 +49,7 @@ interface HarnessProps { iframe: HTMLIFrameElement | null; timelineElements: TimelineElement[]; setSelectedTimelineElementId?: (id: string | null, options?: SelectElementOptions) => void; + setTimelineSelectionSet?: (ids: Set) => void; } function renderHarness(props: HarnessProps) { @@ -66,7 +67,10 @@ function renderHarness(props: HarnessProps) { captionEditMode: false, previewIframeRef: { current: props.iframe }, timelineElements: props.timelineElements, + getTimelineSelectionSet: () => usePlayerStore.getState().selectedElementIds, setSelectedTimelineElementId: props.setSelectedTimelineElementId ?? vi.fn(), + setTimelineSelectionSet: + props.setTimelineSelectionSet ?? usePlayerStore.getState().setSelectedElementIds, setRightCollapsed: vi.fn(), setRightPanelTab: props.setRightPanelTab, previewIframe: props.iframe, @@ -236,7 +240,7 @@ describe("useDomSelection — marquee multi-select survives the late async prima iframe.remove(); }); - it("collapses the set when a late primary-set targets a non-member (fresh click)", async () => { + it("collapses the set to a fresh single-click target instead of publishing an empty set", async () => { const iframe = document.createElement("iframe"); document.body.append(iframe); const doc = iframe.contentDocument!; @@ -268,7 +272,7 @@ describe("useDomSelection — marquee multi-select survives the late async prima await pending; }); - expect(usePlayerStore.getState().selectedElementIds.size).toBe(0); + expect([...usePlayerStore.getState().selectedElementIds]).toEqual(["d"]); expect(usePlayerStore.getState().selectedElementId).toBe("d"); harness.cleanup(); iframe.remove(); diff --git a/packages/studio/src/hooks/useGsapAnimationFetchFallback.ts b/packages/studio/src/hooks/useGsapAnimationFetchFallback.ts index 8dd9ba9757..f7ad8529f1 100644 --- a/packages/studio/src/hooks/useGsapAnimationFetchFallback.ts +++ b/packages/studio/src/hooks/useGsapAnimationFetchFallback.ts @@ -33,6 +33,8 @@ export type ElementAnimationsOutcome = export interface GsapAnimationFetchOptions { /** Refuse the edit when the parse endpoint is unavailable instead of treating it as no motion. */ failOnFetchError?: boolean; + /** Ignore an overlapping pre-write parse and read the source after a durable write. */ + fresh?: boolean; } /** @@ -56,11 +58,13 @@ async function fetchElementAnimationsWithRetry( gsapSourceFile: string, target: { id: string | null; selector: string | null }, failOnFetchError: boolean, + fresh: boolean, ): Promise { let coldAttempts = 0; let errorAttempts = 0; for (;;) { - const parsed = await fetchParsedAnimations(projectId, gsapSourceFile); + const parsed = await fetchParsedAnimations(projectId, gsapSourceFile, { fresh }); + fresh = false; const outcome = selectElementAnimationsOrRetry(parsed, target); if (outcome.kind === "resolved") return outcome.animations; if (outcome.kind === "fetch-error") { @@ -89,6 +93,7 @@ export function useGsapAnimationFetchFallback(projectId: string | null, gsapSour gsapSourceFile, target, options?.failOnFetchError === true, + options?.fresh === true, ); }, [projectId, gsapSourceFile], diff --git a/packages/studio/src/hooks/useGsapAwareEditing.test.tsx b/packages/studio/src/hooks/useGsapAwareEditing.test.tsx index 89ab3f6fdc..9d16157b2d 100644 --- a/packages/studio/src/hooks/useGsapAwareEditing.test.tsx +++ b/packages/studio/src/hooks/useGsapAwareEditing.test.tsx @@ -301,6 +301,42 @@ describe("useGsapAwareEditing anchored resize", () => { act(() => root.unmount()); }); + it("reports only the first group preflight failure in input order", async () => { + const failures = [new Error("first blocked"), new Error("second blocked")]; + const trackGsapInteractionFailure = vi.fn(); + const priorDragImplementation = mocks.drag.getMockImplementation(); + mocks.drag.mockImplementation(async (selection) => { + throw selection.id === "a" ? failures[0] : failures[1]; + }); + const { groupCommit, root } = mountGroupHandler({ + gsapCommitMutation: vi.fn().mockResolvedValue(undefined), + makeFetchFallback: () => vi.fn().mockResolvedValue([]), + trackGsapInteractionFailure, + }); + const updates = [ + { + selection: { element: document.createElement("div"), id: "a", selector: "#a" }, + next: { x: 10, y: 10 }, + }, + { + selection: { element: document.createElement("div"), id: "b", selector: "#b" }, + next: { x: 20, y: 20 }, + }, + ] as unknown as DomEditGroupPathOffsetCommit[]; + + await expect(groupCommit(updates)).rejects.toBe(failures[0]); + expect(trackGsapInteractionFailure).toHaveBeenCalledOnce(); + expect(trackGsapInteractionFailure).toHaveBeenCalledWith( + failures[0], + updates[0]?.selection, + "drag", + "Move animated layer (group)", + ); + mocks.drag.mockReset(); + if (priorDragImplementation) mocks.drag.mockImplementation(priorDragImplementation); + act(() => root.unmount()); + }); + it("restores once when resize persistence fails", async () => { const error = new Error("resize failed"); const restore = vi.fn(); diff --git a/packages/studio/src/hooks/useGsapAwareEditing.ts b/packages/studio/src/hooks/useGsapAwareEditing.ts index 0e84c15826..384527f5fb 100644 --- a/packages/studio/src/hooks/useGsapAwareEditing.ts +++ b/packages/studio/src/hooks/useGsapAwareEditing.ts @@ -24,7 +24,11 @@ import { useGsapSaveFailureTelemetry, useSafeGsapCommitMutation, } from "./useSafeGsapCommitMutation"; -import type { CommitMutation } from "./gsapScriptCommitTypes"; +import type { + CommitMutation, + CommitMutationCall, + CommitMutationOptions, +} from "./gsapScriptCommitTypes"; import { setElementGsapPosition } from "../utils/elementGsap"; import { logResize, logResizeSettle } from "../utils/resizeDebug"; import type { DomEditGroupPathOffsetCommit } from "../components/editor/DomEditOverlay"; @@ -37,6 +41,18 @@ import type { GsapAnimationFetchOptions } from "./useGsapAnimationFetchFallback" // into one another's undo entry (module-local counter, not Date.now()). let groupDragCommitCounter = 0; +function firstPreflightFailure( + results: PromiseSettledResult[], + updates: DomEditGroupPathOffsetCommit[], +): { error: unknown; selection: DomEditSelection } | null { + for (const [index, result] of results.entries()) { + if (result.status !== "rejected") continue; + const selection = updates[index]?.selection; + if (selection) return { error: result.reason, selection }; + } + return null; +} + export interface UseGsapAwareEditingParams { domEditSelection: DomEditSelection | null; selectedGsapAnimations: GsapAnimation[]; @@ -50,7 +66,7 @@ export interface UseGsapAwareEditingParams { ) => () => Promise; trackGsapInteractionFailure: ( error: unknown, - selection: DomEditSelection, + selection: DomEditSelection | null, mutationType: string, label: string, ) => void; @@ -155,19 +171,54 @@ export function useGsapAwareEditing({ // it survives the N sequential server round-trips) onto each commit — // otherwise each member records its own entry and it takes N presses to undo. const coalesceKey = `group-drag:${++groupDragCommitCounter}`; - const coalescedCommit: typeof gsapCommitMutation = (selection, mutation, options) => - gsapCommitMutation(selection, mutation, { - ...options, - coalesceKey, - coalesceMs: Number.POSITIVE_INFINITY, + // Members are written one at a time, and a write that re-renders the preview + // re-runs the whole script — which still holds the OLD position of every + // member not yet written. Those members snap back to where they started and + // stay there until their own write lands, which is the single element seen + // jumping mid-commit while the rest of the group sat still. The drafted + // positions are already on screen, so holding the render until the last + // member has been written costs nothing and never shows a half-moved group. + let renderOnCommit = false; + const previewFallbackLatch = { pending: false }; + const withGroupOptions = (options: CommitMutationOptions): CommitMutationOptions => ({ + ...options, + coalesceKey, + coalesceMs: Number.POSITIVE_INFINITY, + deferPreviewSync: !renderOnCommit, + previewFallbackLatch, + }); + // Every member writes the same file. Queue their mutations and send them as + // ONE request instead of one round trip per member: the server reads, parses + // and writes the composition once, and the preview patches once. + const queued: CommitMutationCall[] = []; + const flushQueued = async () => { + if (queued.length === 0) return; + const calls = queued.splice(0, queued.length); + if (!gsapCommitMutation.batch) { + for (const call of calls) { + await gsapCommitMutation(call.selection, call.mutation, call.options); + } + return; + } + await gsapCommitMutation.batch(calls, { + ...(calls.at(-1)?.options ?? { label: "Move animated layer (group)" }), + label: "Move animated layer (group)", }); + }; + const coalescedCommit: typeof gsapCommitMutation = (selection, mutation, options) => { + queued.push({ selection, mutation, options: withGroupOptions(options) }); + return Promise.resolve(); + }; const preflightAnimations = new Map(); // Editability is user-atomic: prove every member can be written before // the first source mutation. Network failures after this point retain the // existing multi-request semantics, but a blocked member can never leave // earlier siblings partially moved. - for (const { selection } of updates) { - try { + // Every member reads the same file, and a preflight writes nothing — so run + // them together. The parse layer shares one in-flight request per file, which + // turns N sequential round trips into one. + const preflightResults = await Promise.allSettled( + updates.map(async ({ selection }) => { const animations = await makeFetchFallback(selection, { failOnFetchError: true })(); preflightAnimations.set(selection, animations); const outcome = await tryGsapDragIntercept( @@ -180,12 +231,20 @@ export function useGsapAwareEditing({ { preflightOnly: true }, ); assertGsapEditPersisted(outcome); - } catch (error) { - trackGsapInteractionFailure(error, selection, "drag", "Move animated layer (group)"); - throw error; - } + }), + ); + const preflightFailure = firstPreflightFailure(preflightResults, updates); + if (preflightFailure) { + trackGsapInteractionFailure( + preflightFailure.error, + preflightFailure.selection, + "drag", + "Move animated layer (group)", + ); + throw preflightFailure.error; } - for (const { selection, next } of updates) { + for (const [index, { selection, next }] of updates.entries()) { + renderOnCommit = index === updates.length - 1; try { const outcome = await tryGsapDragIntercept( selection, @@ -193,7 +252,13 @@ export function useGsapAwareEditing({ preflightAnimations.get(selection) ?? [], previewIframeRef.current, coalescedCommit, - makeFetchFallback(selection), + // The intercept re-reads the file to resolve a stale or shared tween. + // Anything already queued has to be on disk before that read, or it + // resolves against a file missing writes it is about to build on. + async () => { + await flushQueued(); + return makeFetchFallback(selection, { fresh: true })(); + }, { preflightPassed: true }, ); assertGsapEditPersisted(outcome); @@ -202,6 +267,14 @@ export function useGsapAwareEditing({ throw error; } } + try { + await flushQueued(); + } catch (error) { + // The aggregate write has no uniquely failing member; do not misattribute + // its telemetry to whichever member happened to be last in the array. + trackGsapInteractionFailure(error, null, "drag", "Move animated layer (group)"); + throw error; + } }, [gsapCommitMutation, previewIframeRef, makeFetchFallback, trackGsapInteractionFailure], ); diff --git a/packages/studio/src/hooks/useGsapInteractionFailureTelemetry.ts b/packages/studio/src/hooks/useGsapInteractionFailureTelemetry.ts index 80bef7f7c3..cce77f55b1 100644 --- a/packages/studio/src/hooks/useGsapInteractionFailureTelemetry.ts +++ b/packages/studio/src/hooks/useGsapInteractionFailureTelemetry.ts @@ -8,16 +8,16 @@ export function useGsapInteractionFailureTelemetry( showToast: (message: string, tone?: "error" | "info") => void, ) { return useCallback( - (error: unknown, selection: DomEditSelection, mutationType: string, label: string) => { + (error: unknown, selection: DomEditSelection | null, mutationType: string, label: string) => { trackStudioSaveFailure({ source: "gsap_commit", error, - filePath: selection.sourceFile ?? activeCompPath ?? "index.html", + filePath: selection?.sourceFile ?? activeCompPath ?? "index.html", mutationType, label, - targetId: selection.id, - targetSelector: selection.selector, - targetSourceFile: selection.sourceFile, + targetId: selection?.id, + targetSelector: selection?.selector, + targetSourceFile: selection?.sourceFile, }); showToast( isGsapEditBlockedError(error) ? error.message : "Failed to save animated edit.", diff --git a/packages/studio/src/hooks/useGsapScriptCommits.test.tsx b/packages/studio/src/hooks/useGsapScriptCommits.test.tsx index 6236d032ab..e9deae6ccf 100644 --- a/packages/studio/src/hooks/useGsapScriptCommits.test.tsx +++ b/packages/studio/src/hooks/useGsapScriptCommits.test.tsx @@ -73,14 +73,159 @@ describe("applyPreviewSync", () => { syncDragPreview(result(), reloadPreview); - expect(patchRuntimeTweenInPlace).toHaveBeenCalledWith(FAKE_IFRAME, "#a", { - kind: "set", - props: { x: 10 }, - }); + expect(patchRuntimeTweenInPlace).toHaveBeenCalledWith( + FAKE_IFRAME, + "#a", + { + kind: "set", + props: { x: 10 }, + }, + undefined, + false, + ); expect(applySoftReload).not.toHaveBeenCalled(); expect(reloadPreview).not.toHaveBeenCalled(); }); + it("instantPatches: patches every element the batch wrote, rendering once at the end", () => { + patchRuntimeTweenInPlace.mockReturnValue(true); + const reloadPreview = vi.fn(); + + applyPreviewSync( + FAKE_IFRAME, + result(), + { + label: "Move animated layer (group)", + softReload: true, + instantPatches: [ + { selector: "#a", change: { kind: "set" as const, props: { x: 1 } } }, + { selector: "#b", change: { kind: "set" as const, props: { x: 2 } } }, + { selector: "#c", change: { kind: "set" as const, props: { x: 3 } } }, + ], + }, + reloadPreview, + ); + + // Only the last patch re-renders — the earlier two defer their seek, so the + // group repaints once instead of once per member. + expect(patchRuntimeTweenInPlace.mock.calls.map((call) => [call[1], call[4]])).toEqual([ + ["#a", true], + ["#b", true], + ["#c", false], + ]); + expect(applySoftReload).not.toHaveBeenCalled(); + expect(reloadPreview).not.toHaveBeenCalled(); + }); + + it("applies both plural and singular patches when a caller supplies both", () => { + patchRuntimeTweenInPlace.mockReturnValue(true); + + applyPreviewSync( + FAKE_IFRAME, + result(), + { + label: "mixed patch contract", + instantPatches: [ + { selector: "#group-a", change: { kind: "set" as const, props: { x: 1 } } }, + ], + instantPatch: { + selector: "#single-b", + change: { kind: "set" as const, props: { x: 2 } }, + }, + }, + vi.fn(), + ); + + expect(patchRuntimeTweenInPlace.mock.calls.map((call) => [call[1], call[4]])).toEqual([ + ["#group-a", true], + ["#single-b", false], + ]); + }); + + it("instantPatches: one patch that misses falls the whole batch back to the reload", () => { + patchRuntimeTweenInPlace.mockImplementation((_iframe, selector) => selector !== "#b"); + applySoftReload.mockReturnValue("applied"); + const reloadPreview = vi.fn(); + + applyPreviewSync( + FAKE_IFRAME, + result({ scriptText: "SCRIPT" }), + { + label: "Move animated layer (group)", + softReload: true, + instantPatches: [ + { selector: "#a", change: { kind: "set" as const, props: { x: 1 } } }, + { selector: "#b", change: { kind: "set" as const, props: { x: 2 } } }, + ], + }, + reloadPreview, + ); + + // A half-patched preview is worse than a reloaded one: "#a" landed, "#b" did + // not, so the reload repaints both from the written source. + expect(applySoftReload).toHaveBeenCalled(); + expect(trackStudioEvent).toHaveBeenCalledWith("gsap_instant_patch_fallback", { + selector: "#b", + }); + }); + + it("carries a deferred patch miss into the final batch render", () => { + const previewFallbackLatch = { pending: false }; + applySoftReload.mockReturnValue("applied"); + const reloadPreview = vi.fn(); + patchRuntimeTweenInPlace.mockReturnValueOnce(false).mockReturnValueOnce(true); + + applyPreviewSync( + FAKE_IFRAME, + result({ scriptText: "SCRIPT" }), + { + label: "Move animated layer (group)", + softReload: true, + deferPreviewSync: true, + previewFallbackLatch, + instantPatch: { selector: "#missed", change: { kind: "set", props: { x: 1 } } }, + }, + reloadPreview, + ); + + expect(previewFallbackLatch.pending).toBe(true); + expect(applySoftReload).not.toHaveBeenCalled(); + + applyPreviewSync( + FAKE_IFRAME, + result({ scriptText: "SCRIPT" }), + { + label: "Move animated layer (group)", + softReload: true, + previewFallbackLatch, + instantPatch: { selector: "#final", change: { kind: "set", props: { x: 2 } } }, + }, + reloadPreview, + ); + + expect(previewFallbackLatch.pending).toBe(false); + expect(applySoftReload).toHaveBeenCalledTimes(1); + }); + + it("falls back immediately when a deferred patch miss has no final-render latch", () => { + patchRuntimeTweenInPlace.mockReturnValue(false); + applySoftReload.mockReturnValue("applied"); + + applyPreviewSync( + FAKE_IFRAME, + result({ scriptText: "SCRIPT" }), + { + label: "Deferred standalone write", + softReload: true, + deferPreviewSync: true, + instantPatch: { selector: "#missed", change: { kind: "set", props: { x: 1 } } }, + }, + vi.fn(), + ); + + expect(applySoftReload).toHaveBeenCalledTimes(1); + }); + it("instantPatch + patch fails: falls back to the soft reload, passing onAsyncFailure", () => { patchRuntimeTweenInPlace.mockReturnValue(false); applySoftReload.mockReturnValue("applied"); @@ -338,10 +483,51 @@ describe("runCommit — instantPatch wiring", () => { // The file already matched (changed:false) but the runtime patch deferred // from the paired first commit must still land. - expect(patchRuntimeTweenInPlace).toHaveBeenCalledWith(FAKE_IFRAME, "#a", { - kind: "set", - props: { x: 485, y: 311 }, + expect(patchRuntimeTweenInPlace).toHaveBeenCalledWith( + FAKE_IFRAME, + "#a", + { + kind: "set", + props: { x: 485, y: 311 }, + }, + undefined, + false, + ); + expect(deps.reloadPreview).not.toHaveBeenCalled(); + }); + + it("no-op batch still applies every plural instant patch", async () => { + patchRuntimeTweenInPlace.mockReturnValue(true); + mockFetchResult({ changed: false }); + const deps = renderCommitHook(); + const batch = deps.api.commitMutation.batch; + if (!batch) throw new Error("batch capability missing"); + + await act(async () => { + await batch( + [ + { + selection, + mutation: { type: "update-property", property: "x", value: 10 }, + options: { + label: "Move layer", + instantPatch: { selector: "#a", change: { kind: "set", props: { x: 10 } } }, + }, + }, + { + selection: { ...selection, id: "b", selector: "#b" }, + mutation: { type: "update-property", property: "x", value: 20 }, + options: { + label: "Move layer", + instantPatch: { selector: "#b", change: { kind: "set", props: { x: 20 } } }, + }, + }, + ], + { label: "Move animated layer (group)" }, + ); }); + + expect(patchRuntimeTweenInPlace.mock.calls.map((call) => call[1])).toEqual(["#a", "#b"]); expect(deps.reloadPreview).not.toHaveBeenCalled(); }); diff --git a/packages/studio/src/hooks/useGsapScriptCommits.ts b/packages/studio/src/hooks/useGsapScriptCommits.ts index f1a21a3afe..8a902ce97a 100644 --- a/packages/studio/src/hooks/useGsapScriptCommits.ts +++ b/packages/studio/src/hooks/useGsapScriptCommits.ts @@ -126,7 +126,10 @@ function finishUnchangedMutation( reloadPreview: () => void, ): boolean { if (result.changed !== false) return false; - if (!options.skipReload && options.instantPatch) { + if ( + !options.skipReload && + (instantPatchesFor(options).length > 0 || options.previewFallbackLatch?.pending) + ) { applyPreviewSync(iframe, result, options, reloadPreview); } return true; @@ -249,19 +252,37 @@ export function applyPreviewSync( options: CommitMutationOptions, reloadPreview: () => void, ): void { - if (options.instantPatch) { - const patched = patchRuntimeTweenInPlace( - iframe, - options.instantPatch.selector, - options.instantPatch.change, + const patches = instantPatchesFor(options); + let needsFallback = options.previewFallbackLatch?.pending === true; + if (patches.length > 0) { + const deferSeek = options.deferPreviewSync === true; + const missed = patches.find( + (patch, index) => + !patchRuntimeTweenInPlace( + iframe, + patch.selector, + patch.change, + undefined, + deferSeek || index < patches.length - 1, + ), ); - // Patched in place — element is already correct on screen; no reload needed. - if (patched) return; - // The instant path couldn't patch in place — record the fallback so we can - // track how often the fast path misses before the soft/full reload below. - trackStudioEvent("gsap_instant_patch_fallback", { selector: options.instantPatch.selector }); - // Fall through to the soft/full reload path below. + if (missed) { + // The instant path couldn't patch in place — record the fallback so we can + // track how often the fast path misses before the soft/full reload below. + trackStudioEvent("gsap_instant_patch_fallback", { selector: missed.selector }); + needsFallback = true; + } + // Patched in place — elements are already correct on screen; no reload needed + // unless an earlier deferred batch left one member unpatched. + if (!needsFallback) return; + } + // Written, but the caller has more writes to make and will render after the last. + if (options.deferPreviewSync && options.previewFallbackLatch) { + options.previewFallbackLatch.pending = needsFallback; + return; } + if (options.deferPreviewSync && !needsFallback) return; + if (options.previewFallbackLatch) options.previewFallbackLatch.pending = false; if (options.softReload && result.scriptText) { // A soft-reloadable edit escalates to a full iframe remount ONLY on the // PERMANENT "cannot-soft-reload" result (the preview is genuinely stale/ @@ -281,6 +302,15 @@ export function applyPreviewSync( } } +function instantPatchesFor( + options: CommitMutationOptions, +): NonNullable { + return [ + ...(options.instantPatches ?? []), + ...(options.instantPatch ? [options.instantPatch] : []), + ]; +} + // oxfmt-ignore // fallow-ignore-next-line complexity export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIframeRef, editHistory, domEditSaveTimestampRef, reloadPreview, onCacheInvalidate, onFileContentChanged, showToast, sdkSession, publishSdkSession, writeProjectFile, forceReloadSdkSession }: GsapScriptCommitsParams) { @@ -356,7 +386,13 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra ); if (!result) return; options.onResult?.(result); - await finalizeSuccessfulMutation(pid, compositionPath, last.selection, last.mutation, targetPath, result, options); + // Each call brings its own fast-path patch; the batch wrote them all, so the + // preview sync applies them all rather than just the last call's. + const instantPatches = calls + .map(({ options: callOptions }) => callOptions.instantPatch) + .filter((patch) => patch !== undefined); + const { instantPatch: _instantPatch, ...batchOptions } = options; + await finalizeSuccessfulMutation(pid, compositionPath, last.selection, last.mutation, targetPath, result, instantPatches.length > 0 ? { ...batchOptions, instantPatches } : batchOptions); }, [showToast, finalizeSuccessfulMutation]); // Every GSAP-script commit is a read-modify-write of one file. Overlapping diff --git a/packages/studio/src/hooks/useStudioUrlState.ts b/packages/studio/src/hooks/useStudioUrlState.ts index e67e6d346a..629f04e517 100644 --- a/packages/studio/src/hooks/useStudioUrlState.ts +++ b/packages/studio/src/hooks/useStudioUrlState.ts @@ -7,6 +7,7 @@ import { buildStudioHash, parseStudioUrlStateFromHash, type StudioUrlSelectionState, + type StudioUrlSelectionTarget, type StudioUrlState, } from "../utils/studioUrlState"; @@ -22,6 +23,8 @@ interface UseStudioUrlStateParams { rightCollapsed: boolean; activeCompPathHydrated: boolean; domEditSelection: DomEditSelection | null; + domEditGroupSelections: DomEditSelection[]; + applyMarqueeSelection: (selections: DomEditSelection[], additive: boolean) => void; buildDomSelectionFromTarget: ( target: HTMLElement, options?: { preferClipAncestor?: boolean }, @@ -38,8 +41,7 @@ interface UseStudioUrlStateParams { initialState: StudioUrlState; } -function toPersistedSelection(selection: DomEditSelection | null): StudioUrlSelectionState | null { - if (!selection) return null; +function toPersistedTarget(selection: DomEditSelection): StudioUrlSelectionTarget | null { if (!selection.id && !selection.selector) return null; return { sourceFile: selection.sourceFile || undefined, @@ -49,12 +51,109 @@ function toPersistedSelection(selection: DomEditSelection | null): StudioUrlSele }; } +function selectionTargetKey(selection: StudioUrlSelectionTarget): string { + return [ + selection.sourceFile ?? "", + selection.id ?? "", + selection.selector ?? "", + selection.selectorIndex ?? "", + ].join("|"); +} + +function toPersistedSelection( + selection: DomEditSelection | null, + // Optional: a caller that only ever has one selection has nothing to add, and + // the URL must still carry that one rather than throwing on the way out. + group: DomEditSelection[] = [], +): StudioUrlSelectionState | null { + if (!selection) return null; + const primary = toPersistedTarget(selection); + if (!primary) return null; + // The primary is already carried by the top-level fields; the rest ride along so the link + // reopens the same multi-selection instead of a single element. + const primaryKey = selectionTargetKey(primary); + const members = new Map(); + for (const member of group) { + const target = toPersistedTarget(member); + if (!target) continue; + const key = selectionTargetKey(target); + if (key !== primaryKey) members.set(key, target); + } + return { + ...primary, + group: members.size > 0 ? [...members.values()] : undefined, + }; +} + function replaceHash(nextHash: string) { if (typeof window === "undefined") return; if (window.location.hash === nextHash) return; window.history.replaceState(null, "", nextHash); } +interface ResolveUrlSelectionsParams { + doc: Document; + primaryElement: HTMLElement; + selection: StudioUrlSelectionState; + group: StudioUrlSelectionTarget[]; + activeCompPath: string | null; + isCurrent: () => boolean; + buildDomSelection: UseStudioUrlStateParams["buildDomSelectionFromTarget"]; +} + +function findUrlSelectionElement( + doc: Document, + target: StudioUrlSelectionTarget, + fallbackSourceFile: string, + activeCompPath: string | null, +): HTMLElement | null { + return findElementForSelection( + doc, + { + sourceFile: target.sourceFile ?? fallbackSourceFile, + id: target.id, + selector: target.selector, + selectorIndex: target.selectorIndex, + }, + activeCompPath, + ); +} + +async function buildOptionalDomSelection( + element: HTMLElement | null, + buildDomSelection: UseStudioUrlStateParams["buildDomSelectionFromTarget"], +): Promise { + if (!element) return null; + return buildDomSelection(element, { preferClipAncestor: false }); +} + +async function resolveUrlSelections({ + doc, + primaryElement, + selection, + group, + activeCompPath, + isCurrent, + buildDomSelection, +}: ResolveUrlSelectionsParams): Promise { + const primary = await buildDomSelection(primaryElement, { preferClipAncestor: false }); + if (!isCurrent()) return null; + if (!primary) return []; + const members = [primary]; + for (const member of group) { + const element = findUrlSelectionElement( + doc, + member, + selection.sourceFile ?? "", + activeCompPath, + ); + const resolved = await buildOptionalDomSelection(element, buildDomSelection); + if (!isCurrent()) return null; + if (resolved) members.push(resolved); + } + return members; +} + export function useStudioUrlState({ projectId, activeCompPath, @@ -67,6 +166,8 @@ export function useStudioUrlState({ rightCollapsed, activeCompPathHydrated, domEditSelection, + domEditGroupSelections, + applyMarqueeSelection, buildDomSelectionFromTarget, applyDomSelection, setRightPanelTab, @@ -82,6 +183,7 @@ export function useStudioUrlState({ const [selectionHydrated, setSelectionHydrated] = useState(initialState.selection == null); const pendingSelectionRef = useRef(initialState.selection); const stableTimeRef = useRef(initialState.currentTime); + const selectionApplySeqRef = useRef(0); const buildUrlState = useCallback( (): StudioUrlState => ({ @@ -91,10 +193,10 @@ export function useStudioUrlState({ rightCollapsed, timelineVisible: null, selection: hydratedSelectionRef.current - ? toPersistedSelection(domEditSelection) + ? toPersistedSelection(domEditSelection, domEditGroupSelections) : pendingSelectionRef.current, }), - [activeCompPath, domEditSelection, rightCollapsed, rightPanelTab], + [activeCompPath, domEditGroupSelections, domEditSelection, rightCollapsed, rightPanelTab], ); // Resolve a URL selection to a live element and apply it. Shared by the initial @@ -103,6 +205,7 @@ export function useStudioUrlState({ // a missing element or null selection clears the selection and returns true. const applyUrlSelection = useCallback( (selection: StudioUrlSelectionState | null): boolean => { + const applySeq = ++selectionApplySeqRef.current; if (!selection) { applyDomSelection(null, { revealPanel: false }); return true; @@ -128,12 +231,32 @@ export function useStudioUrlState({ applyDomSelection(null, { revealPanel: false }); return true; } - void buildDomSelectionFromTarget(element, { preferClipAncestor: false }).then((resolved) => { - applyDomSelection(resolved, { revealPanel: false }); + const group = selection.group ?? []; + void resolveUrlSelections({ + doc, + primaryElement: element, + selection, + group, + activeCompPath, + isCurrent: () => applySeq === selectionApplySeqRef.current, + buildDomSelection: buildDomSelectionFromTarget, + }).then((members) => { + if (!members) return; + const primary = members[0]; + if (!primary) return applyDomSelection(null, { revealPanel: false }); + if (group.length === 0) return applyDomSelection(primary, { revealPanel: false }); + // Missing group members are dropped without failing the rest. + applyMarqueeSelection(members, false); }); return true; }, - [activeCompPath, applyDomSelection, buildDomSelectionFromTarget, previewIframeRef], + [ + activeCompPath, + applyDomSelection, + applyMarqueeSelection, + buildDomSelectionFromTarget, + previewIframeRef, + ], ); useEffect(() => { diff --git a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts index 656568cb93..f57ebcd2e6 100644 --- a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts +++ b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts @@ -2,6 +2,7 @@ import { useEffect, useMemo, useRef } from "react"; import type { TimelineElement } from "../player"; import type { DomEditSelection } from "../components/editor/domEditing"; import { resolveTimelineIdForSelection } from "../utils/studioHelpers"; +import { logSelect } from "../utils/selectDebug"; interface UseTimelineSelectionPreviewSyncParams { selectedElementId: string | null; @@ -93,6 +94,13 @@ export function useTimelineSelectionPreviewSync({ if (selectedIds.length === 0) { missingSelectionKeyRef.current = ""; + // The timeline holds nothing, so the canvas is about to hold nothing either. + // This is the path that silently drops a selection the user can still see. + logSelect("timeline-empty", { + had: currentIds.length, + previousKey: previousSelectedKey.length > 0, + clearing: previousSelectedKey.length > 0 && currentIds.length > 0, + }); if (previousSelectedKey.length > 0 && currentIds.length > 0) { applyDomSelection(null, { revealPanel: false }); } @@ -127,6 +135,11 @@ export function useTimelineSelectionPreviewSync({ return; } missingSelectionKeyRef.current = ""; + logSelect("timeline-sync", { + wanted: selectedIds.length, + had: currentIds.length, + resolved: selections.length, + }); if (selections.length === 0) { applyDomSelection(null, { revealPanel: false }); } else if (selections.length === 1) { diff --git a/packages/studio/src/utils/dragDebug.ts b/packages/studio/src/utils/dragDebug.ts new file mode 100644 index 0000000000..e0f1949c28 --- /dev/null +++ b/packages/studio/src/utils/dragDebug.ts @@ -0,0 +1,98 @@ +// Canvas drag diagnostics — grep [hf-drag]. Off by default; opt in per session +// with `localStorage.setItem("hf-drag-debug", "1")` (then reload). +// +// A drag that "jumps" is a position that changed without the pointer asking. The +// pointer delta, what snapping did to it, what each member was told to move, and +// where each member actually ended up are logged at every stage, so the frame the +// position diverges from the pointer is visible rather than inferred. +import { makeStudioDebugLogger } from "./studioDebug"; + +export const logDrag = makeStudioDebugLogger("drag"); + +let moveN = 0; + +/** Per-pointermove logging, throttled: the first move then every 8th. */ +export function logDragMove(data: Record): void { + logDrag("move", () => { + moveN += 1; + return moveN % 8 === 1 ? { n: moveN, ...data } : null; + }); +} + +export function resetDragMoveLog(): void { + moveN = 0; +} + +/** Where these elements are rendered right now, in preview-document pixels. */ +export function readDragPositions( + elements: Array<{ key: string; element: HTMLElement }>, +): Record { + const positions: Record = {}; + for (const { key, element } of elements) { + const rect = element.getBoundingClientRect(); + positions[key] = `${Math.round(rect.left)},${Math.round(rect.top)}`; + } + return positions; +} + +/** + * Members whose screen movement disagrees with the rest of the group this frame. + * + * A group moves as one object, so every member travels the same distance; one + * that does not is the whole bug, and averaged-looking samples hide it. Compares + * each member's movement against the group's median and names the outliers, so a + * single element drifting shows up as itself rather than as "the group jumped". + */ +export function findNonRigidMembers( + before: Record, + after: Record, +): string[] { + const moves = new Map(); + for (const key of Object.keys(after)) { + const from = before[key]?.split(",").map(Number); + const to = after[key]?.split(",").map(Number); + if (!from || !to || from.length !== 2 || to.length !== 2) continue; + moves.set(key, `${Math.round(to[0]! - from[0]!)},${Math.round(to[1]! - from[1]!)}`); + } + const counts = new Map(); + for (const move of moves.values()) counts.set(move, (counts.get(move) ?? 0) + 1); + let common = ""; + let best = 0; + for (const [move, count] of counts) { + if (count > best) [common, best] = [move, count]; + } + return [...moves] + .filter(([, move]) => move !== common) + .map(([key, move]) => `${key.split("|")[2] ?? key} moved ${move}, group moved ${common}`); +} + +/** + * Sample the group now and again after the commit has had time to land. The drop + * is the one moment a jump can hide: the source write, the preview reload and the + * timeline resume all happen within a few frames of each other, and any of them + * can put the elements back where they started before the new position arrives. + */ +export function logDragSettle( + stage: string, + elements: Array<{ key: string; element: HTMLElement }>, +): void { + logDrag(stage, () => { + const at = readDragPositions(elements); + const win = elements[0]?.element.ownerDocument.defaultView; + if (win) { + win.setTimeout( + () => logDrag(`${stage}+120ms`, () => ({ at: readDragPositions(elements) })), + 120, + ); + win.setTimeout( + () => logDrag(`${stage}+400ms`, () => ({ at: readDragPositions(elements) })), + 400, + ); + win.setTimeout( + () => logDrag(`${stage}+900ms`, () => ({ at: readDragPositions(elements) })), + 900, + ); + } + return { at }; + }); +} diff --git a/packages/studio/src/utils/reloadDebug.ts b/packages/studio/src/utils/reloadDebug.ts index b884aa9f35..c9f4594dd5 100644 --- a/packages/studio/src/utils/reloadDebug.ts +++ b/packages/studio/src/utils/reloadDebug.ts @@ -5,26 +5,6 @@ // ask for reads as a flash. These lines answer the only question that matters // when one appears: who asked for it, and why the write that triggered it was // not recognised as Studio's own. -let enabled: boolean | null = null; +import { makeStudioDebugLogger } from "./studioDebug"; -function isEnabled(): boolean { - if (enabled === null) { - try { - enabled = localStorage.getItem("hf-reload-debug") === "1"; - } catch { - enabled = false; - } - } - return enabled; -} - -export function logReload( - stage: string, - data: Record | (() => Record) = {}, -): void { - if (!isEnabled()) return; - const details = typeof data === "function" ? data() : data; - console.log( - `[hf-reload] ${JSON.stringify({ stage, t: Math.round(performance.now()), ...details })}`, - ); -} +export const logReload = makeStudioDebugLogger("reload"); diff --git a/packages/studio/src/utils/resizeDebug.ts b/packages/studio/src/utils/resizeDebug.ts index 1e6172ce3f..01b41e1fe7 100644 --- a/packages/studio/src/utils/resizeDebug.ts +++ b/packages/studio/src/utils/resizeDebug.ts @@ -1,33 +1,18 @@ // Resize/gesture diagnostics — grep [hf-resize]. Off by default; opt in per // session with `localStorage.setItem("hf-resize-debug", "1")` (then reload). -// Granular per-move/per-gesture tracing that complements the always-on -// [hf-commit] transaction telemetry in gestureTransaction.ts. -let moveN = 0; -let enabled: boolean | null = null; +// Granular per-move/per-gesture tracing for resize investigation. +import { makeStudioDebugLogger } from "./studioDebug"; -function isEnabled(): boolean { - if (enabled === null) { - try { - enabled = localStorage.getItem("hf-resize-debug") === "1"; - } catch { - enabled = false; - } - } - return enabled; -} +export const logResize = makeStudioDebugLogger("resize"); -export function logResize(stage: string, data: Record): void { - if (!isEnabled()) return; - console.log( - `[hf-resize] ${JSON.stringify({ stage, t: Math.round(performance.now()), ...data })}`, - ); -} +let moveN = 0; /** Per-pointermove logging, throttled: first move then every 8th. */ export function logResizeMove(data: Record): void { - if (!isEnabled()) return; - moveN += 1; - if (moveN % 8 === 1) logResize("move", { n: moveN, ...data }); + logResize("move", () => { + moveN += 1; + return moveN % 8 === 1 ? { n: moveN, ...data } : null; + }); } export function resetResizeMoveLog(): void { @@ -36,11 +21,10 @@ export function resetResizeMoveLog(): void { /** Snapshot the element's live geometry now and again after 200ms (jump detector). */ export function logResizeSettle(el: HTMLElement, tag: string): void { - if (!isEnabled()) return; - const snap = (phase: string) => { + const snapshot = (phase: string) => { const r = el.getBoundingClientRect(); const cs = el.ownerDocument.defaultView?.getComputedStyle(el); - logResize("settle", { + return { tag, phase, rect: { x: r.x, y: r.y, w: r.width, h: r.height }, @@ -48,8 +32,14 @@ export function logResizeSettle(el: HTMLElement, tag: string): void { cssH: cs?.height, transform: cs?.transform, inlineStyle: el.getAttribute("style"), - }); + }; }; - snap("t0"); - setTimeout(() => snap("t200"), 200); + logResize("settle", () => { + const current = snapshot("t0"); + el.ownerDocument.defaultView?.setTimeout( + () => logResize("settle", () => snapshot("t200")), + 200, + ); + return current; + }); } diff --git a/packages/studio/src/utils/selectDebug.ts b/packages/studio/src/utils/selectDebug.ts new file mode 100644 index 0000000000..364f16e85e --- /dev/null +++ b/packages/studio/src/utils/selectDebug.ts @@ -0,0 +1,9 @@ +// Canvas selection diagnostics — grep [hf-select]. Off by default; opt in with +// `localStorage.setItem("hf-select-debug", "1")` (then reload). +// +// Selection failures are silent by nature: a handler returns early and nothing +// happens, which looks identical to a click that never landed. These lines say +// which branch ran and what it decided. +import { makeStudioDebugLogger } from "./studioDebug"; + +export const logSelect = makeStudioDebugLogger("select"); diff --git a/packages/studio/src/utils/studioDebug.test.ts b/packages/studio/src/utils/studioDebug.test.ts new file mode 100644 index 0000000000..89731749c7 --- /dev/null +++ b/packages/studio/src/utils/studioDebug.test.ts @@ -0,0 +1,32 @@ +// @vitest-environment happy-dom + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { makeStudioDebugLogger } from "./studioDebug"; + +describe("makeStudioDebugLogger", () => { + beforeEach(() => { + localStorage.clear(); + vi.restoreAllMocks(); + }); + + it("does no console work while its channel is disabled", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const buildDetails = vi.fn(() => ({ expensive: true })); + + makeStudioDebugLogger("commit")("persisted", buildDetails); + + expect(buildDetails).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + }); + + it("logs only after its exact channel is enabled", () => { + localStorage.setItem("hf-commit-debug", "1"); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + + makeStudioDebugLogger("commit")("persisted", { mutations: 2 }); + + expect(log).toHaveBeenCalledOnce(); + expect(log.mock.calls[0]?.[0]).toContain('[hf-commit] {"stage":"persisted"'); + expect(log.mock.calls[0]?.[0]).toContain('"mutations":2'); + }); +}); diff --git a/packages/studio/src/utils/studioDebug.ts b/packages/studio/src/utils/studioDebug.ts new file mode 100644 index 0000000000..73b3833fe6 --- /dev/null +++ b/packages/studio/src/utils/studioDebug.ts @@ -0,0 +1,30 @@ +// Opt-in diagnostic channels — one per question worth tracing, all off by +// default. Turn one on for the session with `localStorage.setItem("hf--debug", +// "1")` and reload, then grep the console for `[hf-]`. +// Live channels: reload, select, drag, resize, commit. +// +// These exist because the interesting failures here are decisions, not crashes: +// a preview that reloads when it should not, a shift-click that selects nothing. +// Nothing is thrown and nothing is logged by default, so without a trace of the +// decision the only way to find the cause is to guess. +type DebugDetails = Record | (() => Record | null); +type DebugLogger = (stage: string, data?: DebugDetails) => void; + +export function makeStudioDebugLogger(name: string): DebugLogger { + let enabled: boolean | null = null; + return (stage, data = {}) => { + if (enabled === null) { + try { + enabled = localStorage.getItem(`hf-${name}-debug`) === "1"; + } catch { + enabled = false; + } + } + if (!enabled) return; + const details = typeof data === "function" ? data() : data; + if (!details) return; + console.log( + `[hf-${name}] ${JSON.stringify({ stage, t: Math.round(performance.now()), ...details })}`, + ); + }; +} diff --git a/packages/studio/src/utils/studioUrlState.test.ts b/packages/studio/src/utils/studioUrlState.test.ts index 4d2c6eb801..19530c2ffc 100644 --- a/packages/studio/src/utils/studioUrlState.test.ts +++ b/packages/studio/src/utils/studioUrlState.test.ts @@ -77,8 +77,11 @@ function renderStudioUrlStateHarness( rightCollapsed: true, activeCompPathHydrated: true, domEditSelection: null, + domEditGroupSelections: [], + applyMarqueeSelection: () => {}, buildDomSelectionFromTarget: () => Promise.resolve(null), applyDomSelection: () => {}, + setRightPanelTab: () => {}, initialState: { activeCompPath: null, currentTime: 4.2, @@ -116,6 +119,12 @@ function StudioUrlStateHarness(props: Parameters[0]) { return null; } +function previewIframeFor(contentDocument: Document): HTMLIFrameElement { + const iframe = document.createElement("iframe"); + Object.defineProperty(iframe, "contentDocument", { value: contentDocument }); + return iframe; +} + describe("studio url state", () => { it("parses persisted studio state from project hash", () => { const state = parseStudioUrlStateFromHash( @@ -132,7 +141,135 @@ describe("studio url state", () => { id: "hero", selector: undefined, selectorIndex: undefined, + group: undefined, + }); + }); + + /** + * A link to a bug hit while several elements were selected has to carry the + * whole selection. Without the group the URL reopens one element, the report + * cannot be reproduced from it, and it reads as "works for me". + */ + it("round-trips a multi-selection through the hash", () => { + const hash = buildStudioHash("demo", { + activeCompPath: null, + currentTime: null, + rightPanelTab: null, + rightCollapsed: null, + timelineVisible: null, + selection: { + sourceFile: "index.html", + id: "chip", + group: [ + { sourceFile: "index.html", id: "card" }, + { sourceFile: "index.html", selector: ".dot", selectorIndex: 1 }, + ], + }, + }); + + expect(parseStudioUrlStateFromHash(hash).selection?.group).toEqual([ + { sourceFile: "index.html", id: "card" }, + { sourceFile: "index.html", selector: ".dot", selectorIndex: 1 }, + ]); + }); + + it("reads a single selection as having no group", () => { + const hash = parseStudioUrlStateFromHash("#project/demo?v=1&selFile=index.html&selId=hero"); + expect(hash.selection?.group).toBeUndefined(); + }); + + it("restores selector-based multi-selection members from the hash", async () => { + const previewDoc = document.implementation.createHTMLDocument("preview"); + const primaryElement = previewDoc.createElement("div"); + primaryElement.id = "hero"; + const memberElement = previewDoc.createElement("div"); + memberElement.className = "dot"; + previewDoc.body.append(primaryElement, memberElement); + const primary = { element: primaryElement, id: "hero", sourceFile: "index.html" }; + const member = { + element: memberElement, + selector: ".dot", + selectorIndex: 0, + sourceFile: "index.html", + }; + const applyMarqueeSelection = vi.fn(); + + const harness = renderStudioUrlStateHarness({ + previewIframeRef: { + current: previewIframeFor(previewDoc), + }, + applyMarqueeSelection, + buildDomSelectionFromTarget: (target) => + Promise.resolve(target === primaryElement ? primary : member), + initialState: { + activeCompPath: null, + currentTime: null, + rightPanelTab: null, + rightCollapsed: null, + timelineVisible: null, + selection: { + sourceFile: "index.html", + id: "hero", + group: [{ sourceFile: "index.html", selector: ".dot", selectorIndex: 0 }], + }, + }, + }); + + await act(async () => { + await Promise.resolve(); + }); + expect(applyMarqueeSelection).toHaveBeenCalledWith([primary, member], false); + harness.unmount(); + }); + + it("does not let an older async URL selection overwrite a newer hash", async () => { + const previewDoc = document.implementation.createHTMLDocument("preview"); + const firstElement = previewDoc.createElement("div"); + firstElement.id = "first"; + const secondElement = previewDoc.createElement("div"); + secondElement.id = "second"; + previewDoc.body.append(firstElement, secondElement); + const first = { element: firstElement, id: "first", sourceFile: "index.html" }; + const second = { element: secondElement, id: "second", sourceFile: "index.html" }; + let resolveFirst = (_selection: typeof first) => undefined; + const firstResolution = new Promise((resolve) => { + resolveFirst = resolve; + }); + const applyDomSelection = vi.fn(); + const harness = renderStudioUrlStateHarness({ + previewIframeRef: { current: previewIframeFor(previewDoc) }, + applyDomSelection, + buildDomSelectionFromTarget: (target) => + target === firstElement ? firstResolution : Promise.resolve(second), + initialState: { + activeCompPath: null, + currentTime: null, + rightPanelTab: null, + rightCollapsed: null, + timelineVisible: null, + selection: null, + }, + }); + + act(() => { + window.history.replaceState(null, "", "#project/demo?v=1&selId=first"); + window.dispatchEvent(new HashChangeEvent("hashchange")); + window.history.replaceState(null, "", "#project/demo?v=1&selId=second"); + window.dispatchEvent(new HashChangeEvent("hashchange")); + }); + await act(async () => { + await Promise.resolve(); }); + expect(applyDomSelection).toHaveBeenCalled(); + expect(applyDomSelection.mock.calls.every(([selection]) => selection === second)).toBe(true); + const appliedBeforeOlderResolution = applyDomSelection.mock.calls.length; + + await act(async () => { + resolveFirst(first); + await firstResolution; + }); + expect(applyDomSelection).toHaveBeenCalledTimes(appliedBeforeOlderResolution); + harness.unmount(); }); it("builds a project hash with persisted studio state", () => { @@ -228,7 +365,7 @@ describe("studio url state", () => { const harness = renderStudioUrlStateHarness({ previewIframeRef: { - current: { contentDocument: previewDoc } as HTMLIFrameElement, + current: previewIframeFor(previewDoc), }, rightPanelTab: "design", rightCollapsed: false, @@ -279,6 +416,31 @@ describe("studio url state", () => { expect(window.location.hash).toContain("t=4.2"); expect(window.location.hash).toContain("selId=hero"); + const selectorMember = { + ...restoredSelection, + element: document.createElement("div"), + id: "", + selector: ".dot", + selectorIndex: 1, + label: "Dot", + }; + harness.rerender({ + currentTime: 4.2, + domEditSelection: restoredSelection, + domEditGroupSelections: [restoredSelection, selectorMember], + }); + act(() => { + vi.advanceTimersByTime(250); + }); + expect(parseStudioUrlStateFromHash(window.location.hash).selection?.group).toEqual([ + { + sourceFile: "index.html", + id: undefined, + selector: ".dot", + selectorIndex: 1, + }, + ]); + harness.unmount(); }); }); diff --git a/packages/studio/src/utils/studioUrlState.ts b/packages/studio/src/utils/studioUrlState.ts index e295ca578b..1dfe976f10 100644 --- a/packages/studio/src/utils/studioUrlState.ts +++ b/packages/studio/src/utils/studioUrlState.ts @@ -2,13 +2,23 @@ import type { RightPanelTab } from "./studioHelpers"; import { buildProjectHash, parseProjectHashRoute } from "./projectRouting"; import { roundTo3 } from "./rounding"; -export interface StudioUrlSelectionState { +export interface StudioUrlSelectionTarget { sourceFile?: string; id?: string; selector?: string; selectorIndex?: number; } +export interface StudioUrlSelectionState extends StudioUrlSelectionTarget { + /** + * The other members of a multi-selection, primary excluded. + * A link to a bug in a group edit is only reproducible if it carries the group; + * without this, opening the URL lands on one element and the report reads as + * "works for me". + */ + group?: StudioUrlSelectionTarget[]; +} + export interface StudioUrlState { activeCompPath: string | null; currentTime: number | null; @@ -63,19 +73,63 @@ function parseTab(value: string | null): RightPanelTab | null { return VALID_TABS.includes(value as RightPanelTab) ? (value as RightPanelTab) : null; } +function optionalString(value: unknown): string | undefined { + return typeof value === "string" ? value : undefined; +} + +function normalizedIndex(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) + ? Math.max(0, Math.floor(value)) + : undefined; +} + +function parseSelectionTarget(value: unknown): StudioUrlSelectionTarget | null { + if (!value || typeof value !== "object") return null; + const sourceFile = optionalString(Reflect.get(value, "sourceFile")); + const id = optionalString(Reflect.get(value, "id")); + const selector = optionalString(Reflect.get(value, "selector")); + if (!id && !selector) return null; + return { + sourceFile, + id, + selector, + selectorIndex: normalizedIndex(Reflect.get(value, "selectorIndex")), + }; +} + +/** The other members of a multi-selection, dropping invalid hand-edited entries. */ +function parseGroup(value: string | null): StudioUrlSelectionTarget[] | undefined { + if (!value) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(value); + } catch { + // Compatibility with links produced by the first id-only implementation. + const legacy = value + .split(",") + .map((id) => id.trim()) + .filter(Boolean) + .map((id) => ({ id })); + return legacy.length > 0 ? legacy : undefined; + } + if (!Array.isArray(parsed)) return undefined; + const targets = parsed.map(parseSelectionTarget).filter((target) => target !== null); + return targets.length > 0 ? targets : undefined; +} + function normalizeSelection(params: URLSearchParams): StudioUrlSelectionState | null { const sourceFile = params.get("selFile") || undefined; const id = params.get("selId") || undefined; const selector = params.get("selSelector") || undefined; - const selectorIndex = parseNumber(params.get("selIndex")); - if (!sourceFile && !id && !selector) return null; + const selectorIndex = parseNumber(params.get("selIndex")); return { sourceFile, id, selector, selectorIndex: selectorIndex != null ? Math.max(0, Math.floor(selectorIndex)) : undefined, + group: parseGroup(params.get("selGroup")), }; } @@ -130,6 +184,9 @@ export function buildStudioHash(projectId: string, state: StudioUrlState): strin if (typeof state.selection.selectorIndex === "number") { params.set("selIndex", String(Math.max(0, Math.floor(state.selection.selectorIndex)))); } + if (state.selection.group?.length) { + params.set("selGroup", JSON.stringify(state.selection.group)); + } } return buildProjectHash(projectId, params);