From 17ac986bfe0fffbbc8aac2a68b9e9814f561b562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Sun, 9 Aug 2026 16:58:34 -0700 Subject: [PATCH] fix(studio): canvas selection, drag and resize correctness (#3146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(studio): size the selection box by the transform the element actually paints under The box around a text layer inside the playground card stopped mid-word. The layer is 260px wide and paints 313, because its parent carries `scale(1.2)`, and the chrome read only the element's OWN transform. The top-left looked right, since the corners are anchored to the real bounding rect, so only the right and bottom edges fell short, by exactly 1/1.2. The same read decides whether to draw the box rotated at all, so an element whose parent is rotated got an upright box over a rotated one. The transform is now accumulated from the element up to the composition root. Only the linear part matters: each transform's origin contributes translation, and translation is already discarded by matching the corners to the element's bounding rect, so composing the matrices is enough and no per-ancestor origin has to be unpicked. The walk stops inside the composition document, because the canvas zoom lives on the iframe in Studio's own document and is applied separately. The fake DOMMatrix the geometry tests use gained the `multiply` it now needs. * fix(studio): drag by the movement the element actually makes, not the one assumed An element that had never been dragged skipped the movement measurement and took the canvas zoom as the whole screen mapping. Nothing above the element was considered, so any parent transform broke the drag: a card at rotationY 180 with scale 1.2 maps a rightward drag to -1.2x the zoom, meaning the text walked LEFT while the overlay followed the pointer, and the overlay only snapped onto the text at drop, when it re-measured. Measured on the live element in that card: one unit of drag offset moved it -0.757 px on x and +0.757 on y, where the skipped path assumed +0.631 on both. The measurement it skipped already handles this — it moves the element, watches where it lands, and inverts that, which is right for rotation, mirroring, scale and perspective alike. So the special case is gone and every drag measures. Same element after: a 120x80 pointer drag moves it 120.3x80.2. Rewrote the test that asserted the skipped path's identity matrix for an unmovable element. It now asserts the honest outcome: an element with no measurable movement is reported unmeasurable whether or not it carries a path offset, and the caller's existing fallback covers it. * fix(studio): shift-click adds the element under the pointer, not the last one hovered Shift-click read the hover cache and used it without checking what it described. That cache is filled asynchronously as the pointer moves, so passing over one element on the way to another leaves it naming the element you left. The shift-click then added THAT element, and because the same branch prevented the default and set the suppression flags, the mousedown path that would have resolved the point correctly never ran. Multi-select looked like it grabbed things at random, or like it did nothing. Reproduced on the canvas with a trace: hover #card, shift-click #dot-b, and the group gained #card. Same gesture after: the guard rejects the cache, the mousedown path resolves the point, and the group gains #dot-b. The cache is still used when it is provably about the point clicked, including when it names a clip ancestor of the element there, so the fast path survives for the common case of clicking straight at something. Adds `hf-select-debug` (localStorage, off by default) recording which selection branch ran and what it decided, and pulls the flag/format shared with `hf-reload-debug` into one place rather than copying it. * fix(studio): keep every element a marquee caught, not just the first The marquee built the group correctly and then threw it away. It announced only the primary to the timeline, and the timeline is the source of truth for what is selected: the sync back to the canvas saw one selected id against a group of several, decided the canvas was stale, and replaced the group with that single element a moment after the drop. Drag a box around four things, get one. The whole set is announced now, and the primary goes in as its anchor rather than as a new single selection, so the set it just joined survives. This is the same reason the single-select path already anchors with preserveSet. A test drives applyMarqueeSelection with two elements and asserts both reach the timeline; it fails against the old single-id announce. * fix(studio): stop a group selection from erasing itself on the timeline Every canvas selection is mirrored onto the timeline, and the timeline syncs back — whatever it holds replaces the canvas selection a moment later. The mirror announced only the primary and anchored it with preserveSet, but preserving a set that does not contain the id empties the set, and an empty set syncs back as "nothing is selected". Adding a second element, or re-resolving a group after moving it, could therefore drop the whole selection rather than keep it. One helper now owns the mirror: publish the members, then anchor. A single selection keeps the previous contract deliberately, so a late async primary still cannot collapse a live group and a fresh click still collapses a stale one. The group re-resolve path also gains the ancestor id fallback the other callers already had — without it a member with no direct timeline row resolved to null and deselected everything. Two tests: a second element joining a selection, and a marquee, both assert the full set reaches the timeline. Both fail against the announce-the-primary-only version. * chore(studio): trace what moves a dragged group and when A drag that jumps is a position that changed without the pointer asking for it, and nothing on that path says anything today, so the frame it diverges can only be guessed at. `hf-drag-debug` (localStorage, off by default) records the whole gesture: the mapping and start position each member got, the pointer delta against the delta actually applied on every eighth move, what each member was told to commit, and where they all sit at the drop, once the commit resolves, and 120/400/900ms later. That last group is the point of it. The source write, the preview reload and the timeline resume all land within a few frames of the drop, and any of them can put the elements back where they started before the new position arrives — a snap-back shows up as a settle sample reverting to the gesture-start reading. A gap between `pointer` and `applied` instead means snapping pulled the group off the cursor, which is a different fault with a different fix. * chore(studio): name the path that clears a selection after a group move The drag trace showed the group landing exactly where it was dropped and staying there — no snap-back at any settle sample, and the pointer and the applied delta never more than 2px apart — but two milliseconds after the drop the selection was cleared with seven members still in it. The clear comes from the timeline sync deciding the timeline holds nothing, and that branch said nothing. It says so now, along with whether it is about to act on it. The mirror alongside it reports how many members it managed to publish and whether the anchor was among them, because a member with no timeline row of its own resolves to null and is dropped silently — publish none and the sync reads it back as an empty selection. * fix(studio): losing one member of a group no longer deselects all of it After a move the preview re-syncs and the selection is re-resolved against the new document. When the primary could not be found there, both re-resolve paths cleared the entire selection — so a group of five, all still on screen, was deselected because one of them failed to resolve. The trace showed the clear landing 600ms after the drop with five members still held, and the timeline sync running afterwards on an already-empty canvas, which ruled it out as the cause. A live group now re-resolves as a group and keeps whoever survived, picking a new primary from them; it only clears when nobody did. That is what refreshDomEditGroupSelectionsFromPreview was written for — it existed and was never called. Both clears also say which one they are and how many members were held, so if this is not the last of it the next trace names the path immediately. * feat(studio): carry a multi-selection in the URL, and name the member that breaks away A link to a bug hit with several elements selected only reproduced one of them, so the report read as "works for me". The hash now carries the rest as selGroup and reopens the whole selection; members whose element is gone are dropped rather than failing the others. Verified end to end in a real browser: select three, copy the hash, open it fresh, the same three come back. The drag trace also gains a rigidity check. A group moves as one object, so every member travels the same distance; one that does not IS the fault. Drift was being computed but only printed on every eighth frame, which is exactly how a single-frame divergence hides — it now prints on the frame it happens. The frame handler moves to its own module on the way past. It had grown a snap block and a trace block inside a function already juggling four gesture kinds, and it was over both the complexity and file-size gates. Not fixed: the jump itself. Two headful runs driving a real group drag showed the members staying rigid to the pixel, at the drop and 900ms after, so I have not reproduced it yet and will not guess at a fix. * fix(studio): stop snapping from moving a selection you have not dragged yet Your log caught it on the first frame of the drag: pointer "0,0", applied "4,-3", and all four members jumped 12,-8 composition px before the pointer had moved at all. An element resting within the 6px snap threshold of a guide is already snappable, so the snap computed on frame one closes that gap immediately — picking the selection up moves it. Snapping now sits out until the gesture has travelled the same 4px a drag needs to count as a drag rather than a click, on both the group and single-element paths. Nothing below that distance moves anything, and a real drag snaps exactly as before. The test builds a box resting 4px from a guide and asserts the ungated call still returns dx 4 — the very displacement from your log — while the gated one returns 0 for a pointer that has not moved. * fix(studio): a dropped group stays selected Your Jam confirmed the first-frame jump is gone — pointer "0,0" now reads applied "0,0" — and caught what was left: two milliseconds after each drop, a `[hf-select] clear` with the group still holding three, then four members. Every pointerup trails a click. The group gesture ref is cleared before the commit runs, 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 the drag just moved. The under-threshold path already ate that click; the committed path never did. The flag is now set before the two paths diverge, so neither can forget it. The test drives a real pointerup through the handlers and fails on the committed path with the flag moved back down. * feat(studio): marquee from anywhere on the canvas, including outside the frame An element dragged past the edge sits out in the grey, and the rubber band refused to start there — it only began when the press landed inside the composition rect. The one gesture that could reach those elements could not be begun near them, so the timeline was the only way to select something plainly visible on screen. The collecting half never had that limit: it compares rects in overlay space and never clipped to the frame, so those elements have always been selectable once the band could begin. Only the start gate had to go. A press in the grey that never travels still commits an empty selection, which is the deselect it used to be, so the old behaviour of clicking out there to clear is unchanged. * refactor(studio): keep the selection files under the size cap The selection work above pushed four files past the 600-line gate. Same split the branch made later, landed with the changes that caused it. * fix(studio): preserve selector groups in share URLs * fix(studio): close multi-selection review gaps * fix(studio): stabilize selection store reads * fix(studio): preserve canvas-only group anchors * fix(studio): stop a group drag from jumping one element back Dragging several elements at once and dropping them made one of them snap back to where it started for a frame or two, then jump forward again. Each member of the group is written separately, and every write patched the live GSAP tween in place and then seeked the player. A seek re-renders the WHOLE timeline, not the tween that changed, so the members still queued behind that write got repainted from their un-patched tweens: back to their pre-drag position, where they sat until their own write landed. Only members whose tween actually renders at the playhead showed it, which is why a group of three flashed one element and left the others still. The group commit now defers the seek for every member but the last, so the queued members keep the transform the gesture left on them and the whole group repaints once, from the fully patched timeline. * perf(studio): commit a group drag in one request Dragging N elements cost N writes and 9 reads for a three-element group: each member fetched the composition's parse to preflight, fetched it again to resolve its tween, then wrote the file on its own round trip. Every one of those writes re-read, re-parsed and re-serialized the whole composition. Three changes, same behaviour: - The parse endpoint shares an in-flight request per file, so callers asking for the same composition at the same moment get one request. Only overlapping calls share — the entry is dropped as soon as it settles, so a read after a write still gets a fresh parse. - The group preflight runs its members together instead of one at a time. A preflight writes nothing, so there is nothing to order. - Members' mutations are queued and sent as one batch write. Anything that re-reads the file flushes the queue first, so a member resolving a shared or stale tween never reads a composition missing writes it is about to build on. The batch carries each member's runtime patch, and only the last one re-renders. A three-element group drag now issues 2 reads and 1 write, down from 9 and 3. * fix(studio): harden batched drag commits * fix(studio): carry deferred preview fallbacks * chore(studio): name whoever puts the pre-resize size back Resizing the card commits correctly — the source and a fresh load both read 273x181 — but 200ms after the drop, mid-commit, the element renders at 395x261 with the studio size vars still holding 273x181. Something writes the pre-gesture size back inline while the reload is still in flight, and every writer of that size was silent. Both are traced now under the existing hf-resize-debug flag, each with the size going in, the size being replaced, and a short stack. Restoring the pre-gesture size is right on a cancel and wrong after a successful commit, and the function doing it cannot tell the two apart from the inside — so the caller has to be named before this can be fixed at the right end. * fix(studio): hold a resized element's size while the timeline is rebuilt Your log caught it across two resizes. The first commits 305x202 and the element is 305x202 at the drop; 200ms later it renders 395x261, its stylesheet size, while --hf-studio-width still reads 305. The second gesture then starts with `actual` at 305 against a live box of 395, and its very first move — a pointer delta of 0.1px — snaps the element back to 305. That snap is the jump. The gap belongs to the soft reload: it reverts the old timeline before building the new one, and GSAP hands back each tween's recorded starting width on the way out. Nothing held the size in between, because the seek reapply that exists for exactly this stands aside for elements GSAP animates. Standing aside is right for the offset — those channels compose, and applying both doubles the move — and wrong for size, where both channels write width and height so the later write simply wins on the same committed number. It applies now. Only an element mid-edit carries the vars, so nothing else is touched. A test seeks an element whose size GSAP owns after the revert put the stylesheet size back, and fails with the skip restored. * refactor(studio): keep the resize files under the size cap * docs(studio): fold the resize note into the size-reapply comment * fix(studio): rotate the child outlines with the element they outline Selecting a rotated element drew upright dashed boxes across its children: the chrome co-rotated with the element and the child outlines did not, so a text layer inside a rotated card got a square outline lying across the rotated glyphs. The chrome already measures an oriented box; the child outlines were still measured axis-aligned. They now use the same oriented measurement and render with the same rotation. An unrotated element measures identically to before, since the oriented rect returns the plain bounding box at angle 0. --- packages/studio/src/App.tsx | 8 +- .../components/editor/DomEditOverlay.test.ts | 91 +++++++- .../src/components/editor/DomEditOverlay.tsx | 63 ++++-- .../editor/domEditOverlayGeometry.test.ts | 94 ++++++++ .../editor/domEditOverlayGeometry.ts | 61 +++++- .../editor/domEditOverlayGestures.ts | 42 ++++ .../editor/domEditOverlayStartGesture.ts | 17 ++ .../src/components/editor/groupDragMove.ts | 110 ++++++++++ .../editor/groupDropKeepsSelection.test.ts | 70 ++++++ .../src/components/editor/manualEditsDom.ts | 28 +-- .../editor/manualOffsetDrag.test.ts | 97 ++++++++- .../src/components/editor/manualOffsetDrag.ts | 39 ++-- .../editor/reapplyBoxSizeAfterSeek.test.ts | 64 ++++++ .../editor/snapEngageTravel.test.ts | 65 ++++++ .../src/components/editor/snapEngine.ts | 22 +- .../editor/useDomEditOverlayGestures.ts | 93 ++++---- .../editor/useDomEditOverlayRects.ts | 6 +- .../src/hooks/domSelectionTimelineMirror.ts | 73 +++++++ .../src/hooks/gestureTransaction.test.ts | 19 +- .../studio/src/hooks/gestureTransaction.ts | 27 +-- .../studio/src/hooks/gsapRuntimePatch.test.ts | 43 ++++ packages/studio/src/hooks/gsapRuntimePatch.ts | 12 +- .../studio/src/hooks/gsapScriptCommitTypes.ts | 20 ++ .../src/hooks/keyframeCacheAstLoad.test.ts | 108 ++++++++++ .../studio/src/hooks/keyframeCacheAstLoad.ts | 30 ++- .../studio/src/hooks/useDomEditPreviewSync.ts | 23 ++ .../src/hooks/useDomEditSession.test.tsx | 105 ++++----- .../studio/src/hooks/useDomEditSession.ts | 12 +- packages/studio/src/hooks/useDomEditWiring.ts | 6 + .../studio/src/hooks/useDomSelection.test.ts | 136 +++++++++++- packages/studio/src/hooks/useDomSelection.ts | 107 ++++++---- .../useDomSelectionSelectionGuards.test.ts | 8 +- .../hooks/useGsapAnimationFetchFallback.ts | 7 +- .../src/hooks/useGsapAwareEditing.test.tsx | 36 ++++ .../studio/src/hooks/useGsapAwareEditing.ts | 103 +++++++-- .../useGsapInteractionFailureTelemetry.ts | 10 +- .../src/hooks/useGsapScriptCommits.test.tsx | 200 +++++++++++++++++- .../studio/src/hooks/useGsapScriptCommits.ts | 62 ++++-- .../studio/src/hooks/useStudioUrlState.ts | 137 +++++++++++- .../hooks/useTimelineSelectionPreviewSync.ts | 13 ++ packages/studio/src/utils/dragDebug.ts | 98 +++++++++ packages/studio/src/utils/reloadDebug.ts | 24 +-- packages/studio/src/utils/resizeDebug.ts | 50 ++--- packages/studio/src/utils/selectDebug.ts | 9 + packages/studio/src/utils/studioDebug.test.ts | 32 +++ packages/studio/src/utils/studioDebug.ts | 30 +++ .../studio/src/utils/studioUrlState.test.ts | 164 +++++++++++++- packages/studio/src/utils/studioUrlState.ts | 63 +++++- 48 files changed, 2370 insertions(+), 367 deletions(-) create mode 100644 packages/studio/src/components/editor/groupDragMove.ts create mode 100644 packages/studio/src/components/editor/groupDropKeepsSelection.test.ts create mode 100644 packages/studio/src/components/editor/reapplyBoxSizeAfterSeek.test.ts create mode 100644 packages/studio/src/components/editor/snapEngageTravel.test.ts create mode 100644 packages/studio/src/hooks/domSelectionTimelineMirror.ts create mode 100644 packages/studio/src/hooks/keyframeCacheAstLoad.test.ts create mode 100644 packages/studio/src/utils/dragDebug.ts create mode 100644 packages/studio/src/utils/selectDebug.ts create mode 100644 packages/studio/src/utils/studioDebug.test.ts create mode 100644 packages/studio/src/utils/studioDebug.ts diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index 520b4e855..199971e07 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 736bd7ecb..a7270849c 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 2c6d6add3..261b82f24 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 217f3d343..0554bb4f4 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 70f250095..004bef436 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 d7fec86a8..34d6519c1 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 6576a9b5b..390e05e66 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 000000000..7a0fe732a --- /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 000000000..3fc95b674 --- /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 9ac4588a2..4a041e020 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 5af32996a..0db15b114 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 a808c792f..75609a73c 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 000000000..92a908796 --- /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 000000000..74b045bd5 --- /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 6a64a22a2..f80db5f3b 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 7af8df2e7..65e8bb422 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 b45ff6f26..ba402bafb 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 000000000..f67577925 --- /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 3cc8e4553..8ff3ff28f 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 4b398d663..da7d61f48 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 1a17f7e06..e34d293a4 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 de2c6480e..9d3265038 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 929e79b8e..85071ab6d 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 000000000..05d5c95d4 --- /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 71e38bbb6..390208e32 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 6a288b6ee..cb0c334fe 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 3c5a3e0ff..e629b2d1b 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 efec011f9..1ad7ab3fc 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 fbd049482..9b49f16fa 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 53f452464..4b407c35e 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 32682b081..115d54fca 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 dce065594..a61ee38fa 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 8dd9ba975..f7ad8529f 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 89ab3f6fd..9d16157b2 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 0e84c1582..384527f5f 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 80bef7f7c..cce77f55b 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 6236d032a..e9deae6cc 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 f1a21a3af..8a902ce97 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 e67e6d346..629f04e51 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 656568cb9..f57ebcd2e 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 000000000..e0f1949c2 --- /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 b884aa9f3..c9f4594dd 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 1e6172ce3..01b41e1fe 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. +// Granular per-move/per-gesture tracing for resize investigation. +import { makeStudioDebugLogger } from "./studioDebug"; + +export const logResize = makeStudioDebugLogger("resize"); + let moveN = 0; -let enabled: boolean | null = null; - -function isEnabled(): boolean { - if (enabled === null) { - try { - enabled = localStorage.getItem("hf-resize-debug") === "1"; - } catch { - enabled = false; - } - } - return enabled; -} - -export function logResize(stage: string, data: Record): void { - if (!isEnabled()) return; - console.log( - `[hf-resize] ${JSON.stringify({ stage, t: Math.round(performance.now()), ...data })}`, - ); -} /** 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 000000000..364f16e85 --- /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 000000000..89731749c --- /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 000000000..73b3833fe --- /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 4d2c6eb80..19530c2ff 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,9 +141,137 @@ 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", () => { expect( buildStudioHash("demo", { @@ -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 e295ca578..1dfe976f1 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);