fix(studio): address PR #2347 review findings (rounds 1-2)

Review 1 (restore commit):
- asset reveal now clears any open preview overlay (stuck-overlay repro:
  preview on A, click already-added B — A stayed open over the reveal)
- duration readout rolls back on failed persist: captureDurationRollback
  snapshots store + live root data-duration before the optimistic sync and
  restores both in every move/resize/delete/group catch (golden's
  previousDuration pattern)
- asset preview opened during running playback dismisses immediately (the
  RAF loop bypasses the store, so the subscription alone never fired)
- persistTimelineBatchEdit resolves the target (findTagByTarget) before
  treating identical output as a no-op — a mistargeted member now throws
  like the single-element path instead of being silently dropped
- a post-mutation history-fold failure no longer suppresses the preview
  sync: fold errors are surfaced separately and the rewritten script still
  syncs (previously the preview kept stale GSAP positions with no recovery)
- timelineRevealScroll guards degenerate viewports (windowSize <= 0)
- CodeQL: encodeURIComponent(projectId) at all timelineTimingSync fetches

Review 2 (single-source-of-truth pass):
- createTimelineElementFromManifestClip — the one manifest->element
  boundary — now carries authoredTrack and stackingContextId; expanded
  sub-comp children preserve both (authoredTrack in their OWN file's space)
- authoredTrackForLane scopes occupants to the dragged clip's sourceFile
  (a foreign file's authored values are a different coordinate space);
  nearest-same-file-lane offset fallback
- optimistic store updates mirror the persisted track into authoredTrack
  (and roll it back on failure), so consecutive drags before a reload
  resolve from fresh data
- spill sub-lanes: documented decision — dropping onto a spill lane is a
  legitimate same-track join (occupants share the authored track by
  construction); false 'never a lane-move target' docstring rewritten
- single-element fallback persists vertical-only moves (early return now
  requires neither start nor track changed; live DOM patch includes
  data-track-index)
- canonical contextKey helper for stacking-context normalization
- new pipeline test crosses the REAL factory boundary (sparse authored
  tracks -> factory -> expansion -> normalize -> drag commit -> persisted
  attribute), no injected fields

Author: ukimsanov

packages/studio/src/components/nle/AssetPreviewOverlay.test.tsx

@@ -0,0 +1,88 @@
+// @vitest-environment happy-dom
+
+import React, { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+import { afterEach, describe, expect, it } from "vitest";
+import { usePlayerStore } from "../../player/store/playerStore";
+import { useAssetPreviewStore } from "../../utils/assetPreviewStore";
+import { AssetPreviewOverlay } from "./AssetPreviewOverlay";
+
+(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+let root: Root | null = null;
+
+afterEach(() => {
+  if (root) {
+    act(() => root?.unmount());
+    root = null;
+  }
+  document.body.innerHTML = "";
+  usePlayerStore.getState().reset();
+  useAssetPreviewStore.getState().clearPreviewAsset();
+});
+
+function mountOverlay(): void {
+  const host = document.createElement("div");
+  document.body.append(host);
+  root = createRoot(host);
+  act(() => {
+    root?.render(<AssetPreviewOverlay />);
+  });
+}
+
+describe("AssetPreviewOverlay playback dismissal", () => {
+  it("dismisses immediately when opened while playback is ALREADY running", () => {
+    // The RAF playback loop bypasses the store, so no post-open store change
+    // will arrive — the dismiss check must be level-triggered, not edge-triggered.
+    usePlayerStore.setState({ isPlaying: true });
+    mountOverlay();
+
+    act(() => {
+      useAssetPreviewStore.getState().setPreviewAsset("assets/clip.mp3", "p1");
+    });
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBeNull();
+    expect(document.querySelector('[role="dialog"]')).toBeNull();
+  });
+
+  it("stays open when the playhead is idle", () => {
+    mountOverlay();
+
+    act(() => {
+      useAssetPreviewStore.getState().setPreviewAsset("assets/clip.mp3", "p1");
+    });
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBe("assets/clip.mp3");
+    expect(document.querySelector('[role="dialog"]')).not.toBeNull();
+  });
+
+  it("still dismisses when playback starts AFTER the preview opened", () => {
+    mountOverlay();
+
+    act(() => {
+      useAssetPreviewStore.getState().setPreviewAsset("assets/clip.mp3", "p1");
+    });
+    expect(useAssetPreviewStore.getState().previewAsset).toBe("assets/clip.mp3");
+
+    act(() => {
+      usePlayerStore.setState({ isPlaying: true });
+    });
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBeNull();
+  });
+
+  it("still dismisses when the playhead is scrubbed after opening", () => {
+    usePlayerStore.setState({ currentTime: 2 });
+    mountOverlay();
+
+    act(() => {
+      useAssetPreviewStore.getState().setPreviewAsset("assets/clip.mp3", "p1");
+    });
+
+    act(() => {
+      usePlayerStore.setState({ currentTime: 4.5 });
+    });
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBeNull();
+  });
+});

packages/studio/src/components/nle/AssetPreviewOverlay.tsx

@@ -101,7 +101,15 @@ export function AssetPreviewOverlay() {
   // so a stale render can never dismiss against the wrong reference time.
   useEffect(() => {
     if (!previewAsset) return;
-    const openedTime = usePlayerStore.getState().currentTime;
+    const opened = usePlayerStore.getState();
+    // Level-triggered, not edge-triggered: a preview opened while playback is
+    // ALREADY running gets no store change to react to (the RAF loop bypasses
+    // the store), so evaluate the current state once before subscribing.
+    if (opened.isPlaying) {
+      clearPreviewAsset();
+      return;
+    }
+    const openedTime = opened.currentTime;
     return usePlayerStore.subscribe((state) => {
       if (shouldDismissAssetPreview(openedTime, state)) clearPreviewAsset();
     });

packages/studio/src/components/sidebar/AssetCard.test.tsx

@@ -0,0 +1,106 @@
+// @vitest-environment happy-dom
+
+import React, { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { usePlayerStore, type TimelineElement } from "../../player/store/playerStore";
+import { useAssetPreviewStore } from "../../utils/assetPreviewStore";
+import { AssetCard } from "./AssetCard";
+import { AudioRow } from "./AudioRow";
+
+(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+let root: Root | null = null;
+
+afterEach(() => {
+  if (root) {
+    act(() => root?.unmount());
+    root = null;
+  }
+  document.body.innerHTML = "";
+  usePlayerStore.getState().reset();
+  useAssetPreviewStore.getState().clearPreviewAsset();
+  vi.restoreAllMocks();
+});
+
+function mount(node: React.ReactElement): HTMLElement {
+  const host = document.createElement("div");
+  document.body.append(host);
+  root = createRoot(host);
+  act(() => {
+    root?.render(node);
+  });
+  return host;
+}
+
+function clip(input: Partial<TimelineElement> & { id: string; src: string }): TimelineElement {
+  return { tag: "div", start: 0, duration: 5, track: 0, ...input };
+}
+
+/** Simulate a drag-free click: pointerdown + pointerup at the same point. */
+function clickCard(host: HTMLElement): void {
+  const card = host.querySelector('[draggable="true"]');
+  if (!card) throw new Error("Expected a draggable card root");
+  const PointerCtor = (window as { PointerEvent?: typeof MouseEvent }).PointerEvent ?? MouseEvent;
+  act(() => {
+    card.dispatchEvent(new PointerCtor("pointerdown", { bubbles: true, clientX: 5, clientY: 5 }));
+    card.dispatchEvent(new PointerCtor("pointerup", { bubbles: true, clientX: 5, clientY: 5 }));
+  });
+}
+
+describe("AssetCard click behavior", () => {
+  const cardProps = {
+    projectId: "p1",
+    onCopy: vi.fn(),
+    isCopied: false,
+  };
+
+  it("clears an open preview overlay when clicking an already-added asset (reveal branch)", () => {
+    usePlayerStore.getState().setElements([clip({ id: "img1", src: "assets/logo.png" })]);
+    // Preview overlay is open on ANOTHER asset — the reveal must dismiss it,
+    // or it stays stuck over the canvas while the timeline reveals the clip.
+    useAssetPreviewStore.getState().setPreviewAsset("assets/other.png", "p1");
+
+    const host = mount(<AssetCard {...cardProps} asset="assets/logo.png" used />);
+    clickCard(host);
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBeNull();
+    expect(usePlayerStore.getState().selectedElementId).toBe("img1");
+  });
+
+  it("opens the preview overlay for a not-yet-added asset", () => {
+    const host = mount(<AssetCard {...cardProps} asset="assets/logo.png" used={false} />);
+    clickCard(host);
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBe("assets/logo.png");
+    expect(useAssetPreviewStore.getState().previewProjectId).toBe("p1");
+  });
+});
+
+describe("AudioRow click behavior", () => {
+  const rowProps = {
+    projectId: "p1",
+    onCopy: vi.fn(),
+    isCopied: false,
+  };
+
+  it("clears an open preview overlay when clicking an already-added audio asset (reveal branch)", () => {
+    usePlayerStore
+      .getState()
+      .setElements([clip({ id: "bgm1", tag: "audio", src: "assets/bgm.mp3" })]);
+    useAssetPreviewStore.getState().setPreviewAsset("assets/other.mp3", "p1");
+
+    const host = mount(<AudioRow {...rowProps} asset="assets/bgm.mp3" used />);
+    clickCard(host);
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBeNull();
+    expect(usePlayerStore.getState().selectedElementId).toBe("bgm1");
+  });
+
+  it("opens the preview overlay for a not-yet-added audio asset", () => {
+    const host = mount(<AudioRow {...rowProps} asset="assets/bgm.mp3" used={false} />);
+    clickCard(host);
+
+    expect(useAssetPreviewStore.getState().previewAsset).toBe("assets/bgm.mp3");
+  });
+});

packages/studio/src/components/sidebar/AssetCard.tsx

@@ -136,6 +136,7 @@ export function AssetCard({
   const requestClipReveal = usePlayerStore((s) => s.requestClipReveal);
   const elements = usePlayerStore((s) => s.elements);
   const setPreviewAsset = useAssetPreviewStore((s) => s.setPreviewAsset);
+  const clearPreviewAsset = useAssetPreviewStore((s) => s.clearPreviewAsset);
 
   const handlePointerDown = useCallback((e: React.PointerEvent) => {
     pointerDownRef.current = { x: e.clientX, y: e.clientY };
@@ -151,6 +152,9 @@ export function AssetCard({
       if (used) {
         const clip = findClipForAsset(elements, asset);
         if (clip) {
+          // Dismiss any open preview overlay (from another asset) — the reveal
+          // must not leave a stale preview card floating over the canvas.
+          clearPreviewAsset();
           const clipKey = clip.key ?? clip.id;
           setSelectedElementId(clipKey);
           // Scroll the timeline so the selected clip is actually visible.
@@ -161,7 +165,16 @@ export function AssetCard({
       // Not added (or no matching clip found) → preview overlay
       setPreviewAsset(asset, projectId);
     },
-    [used, elements, asset, projectId, setSelectedElementId, requestClipReveal, setPreviewAsset],
+    [
+      used,
+      elements,
+      asset,
+      projectId,
+      setSelectedElementId,
+      requestClipReveal,
+      setPreviewAsset,
+      clearPreviewAsset,
+    ],
   );
 
   return (

packages/studio/src/components/sidebar/AudioRow.tsx

@@ -46,6 +46,7 @@ export function AudioRow({
   const requestClipReveal = usePlayerStore((s) => s.requestClipReveal);
   const elements = usePlayerStore((s) => s.elements);
   const setPreviewAsset = useAssetPreviewStore((s) => s.setPreviewAsset);
+  const clearPreviewAsset = useAssetPreviewStore((s) => s.clearPreviewAsset);
 
   const handlePointerDown = useCallback((e: React.PointerEvent) => {
     pointerDownRef.current = { x: e.clientX, y: e.clientY };
@@ -60,6 +61,9 @@ export function AudioRow({
       if (used) {
         const clip = findClipForAsset(elements, asset);
         if (clip) {
+          // Dismiss any open preview overlay (from another asset) — the reveal
+          // must not leave a stale preview card floating over the canvas.
+          clearPreviewAsset();
           const clipKey = clip.key ?? clip.id;
           setSelectedElementId(clipKey);
           // Scroll the timeline so the selected clip is actually visible.
@@ -70,7 +74,16 @@ export function AudioRow({
       // Not added → preview overlay (audio player)
       setPreviewAsset(asset, projectId);
     },
-    [used, elements, asset, projectId, setSelectedElementId, requestClipReveal, setPreviewAsset],
+    [
+      used,
+      elements,
+      asset,
+      projectId,
+      setSelectedElementId,
+      requestClipReveal,
+      setPreviewAsset,
+      clearPreviewAsset,
+    ],
   );
 
   useEffect(() => {

packages/studio/src/hooks/timelineEditingHelpers.test.ts

@@ -184,6 +184,19 @@ describe("persistTimelineBatchEdit", () => {
 
     expect(writes).toHaveLength(0);
   });
+
+  it("throws on a mistargeted member instead of silently dropping it", async () => {
+    // A member whose target does not resolve in the source (stale id) patches
+    // to the identical string too — but that is a targeting FAILURE, not an
+    // already-at-target no-op, and must abort the batch like the single path.
+    stubReadFileContent(SOURCE);
+    const writes: Array<[string, string]> = [];
+
+    await expect(
+      persistTimelineBatchEdit(batchInput([moveMember("ghost", 3, 0, 2)], writes)),
+    ).rejects.toThrow("Unable to patch timeline element ghost in index.html");
+    expect(writes).toHaveLength(0);
+  });
 });
 
 describe("deleteSelectedKeyframes", () => {

packages/studio/src/hooks/timelineEditingHelpers.ts

@@ -1,5 +1,5 @@
 import { type TimelineElement, usePlayerStore } from "../player/store/playerStore";
-import { applyPatchByTarget, readAttributeByTarget } from "../utils/sourcePatcher";
+import { applyPatchByTarget, findTagByTarget, readAttributeByTarget } from "../utils/sourcePatcher";
 import {
   formatTimelineAttributeNumber,
   type TimelineStackingReorderIntent,
@@ -319,10 +319,17 @@ export async function persistTimelineBatchEdit(
     }
 
     const current = patchedByPath.get(targetPath) ?? original;
+    // Resolve the target FIRST: byte-identical output below is only a legit
+    // no-op when the member actually resolved in the source. A mistargeted
+    // member (stale id/selector) must fail loudly like the single-edit path,
+    // not be silently dropped as "already at target".
+    if (!findTagByTarget(current, patchTarget)) {
+      throw new Error(`Unable to patch timeline element ${change.element.id} in ${targetPath}`);
+    }
     const patched = change.buildPatches(current, patchTarget);
-    // A member whose attributes already hold the target values patches to the
-    // identical string — e.g. a track-insert renumber where one clip's lane is
-    // already correct. That is a legitimate no-op, not a targeting failure:
+    // The target resolved, so a member whose attributes already hold the target
+    // values patches to the identical string — e.g. a track-insert renumber
+    // where one clip's lane is already correct. That is a legitimate no-op:
     // skip it instead of aborting (and rolling back) the whole batch.
     if (patched === current) continue;
     patchedByPath.set(targetPath, patched);