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
This commit is contained in:
ukimsanov
2026-07-13 16:48:52 -07:00
parent 760b88a6f3
commit a33b3f35e1
23 changed files with 1326 additions and 253 deletions
+55 -20
View File
@@ -13,7 +13,7 @@ export async function readFileContent(projectId: string, targetPath: string): Pr
throw new Error(`Unsafe path: ${targetPath}`);
}
const response = await fetch(
`/api/projects/${projectId}/files/${encodeURIComponent(targetPath)}`,
`/api/projects/${encodeURIComponent(projectId)}/files/${encodeURIComponent(targetPath)}`,
);
if (!response.ok) {
throw new Error(`Failed to read ${targetPath}`);
@@ -55,6 +55,23 @@ export function syncPreviewContentDuration(iframe: HTMLIFrameElement | null): vo
}
}
/**
* Snapshot the store duration BEFORE an optimistic duration update
* (extendRootDurationIfNeeded + syncPreviewContentDuration) and return a
* rollback closure for the persist-failure path. The rollback restores BOTH
* the store duration and the live root's `data-duration` — otherwise a failed
* write leaves the readout/seek bar and the live root advertising a duration
* the saved source never got. No-op when the duration never changed.
*/
export function captureDurationRollback(iframe: HTMLIFrameElement | null): () => void {
const previousDuration = usePlayerStore.getState().duration;
return () => {
if (usePlayerStore.getState().duration === previousDuration) return;
usePlayerStore.getState().setDuration(previousDuration);
patchIframeRootDuration(iframe, previousDuration);
};
}
/**
* The bits of the server GSAP-mutation response the timeline edit path needs.
* `scriptText` is the rewritten root GSAP script — feeding it to `applySoftReload`
@@ -151,6 +168,12 @@ const GSAP_HISTORY_COALESCE_MS = 10_000;
* conflict. This snapshots every touched file, runs the mutation, then records a follow-up
* edit under the same coalesceKey with a window wide enough to survive the GSAP round-trip,
* folding both writes into one undo step. Returns the mutation status for caller reloads.
*
* Failure domains are separate: a MUTATION failure propagates (nothing was applied, so
* the caller must skip the preview sync), but a failure in the history-FOLD step
* (re-read / recordEdit) after a successful mutation is surfaced via `onFoldError` and
* the mutation status is still returned — the server rewrite already landed on disk, so
* the caller must still sync the preview or it shows stale GSAP positions.
*/
async function foldGsapMutationIntoHistory(input: {
projectId: string;
@@ -159,30 +182,37 @@ async function foldGsapMutationIntoHistory(input: {
coalesceKey?: string;
recordEdit: (edit: RecordEditInput) => Promise<void>;
gsapMutation: () => Promise<GsapMutationStatus>;
onFoldError: (error: unknown) => void;
}): Promise<GsapMutationStatus> {
const uniquePaths = [...new Set(input.paths)];
const before = new Map<string, string>();
// A `before`-snapshot failure propagates like a mutation failure: the mutation
// has not run yet, so nothing landed on disk and skipping the sync is correct.
for (const path of uniquePaths) {
before.set(path, await readFileContent(input.projectId, path));
}
const status = await input.gsapMutation();
if (status.mutated) {
const files: Record<string, { before: string; after: string }> = {};
for (const path of uniquePaths) {
const priorContent = before.get(path);
const finalContent = await readFileContent(input.projectId, path);
if (priorContent !== undefined && finalContent !== priorContent) {
files[path] = { before: priorContent, after: finalContent };
try {
const files: Record<string, { before: string; after: string }> = {};
for (const path of uniquePaths) {
const priorContent = before.get(path);
const finalContent = await readFileContent(input.projectId, path);
if (priorContent !== undefined && finalContent !== priorContent) {
files[path] = { before: priorContent, after: finalContent };
}
}
}
if (Object.keys(files).length > 0) {
await input.recordEdit({
label: input.label,
kind: "timeline",
coalesceKey: input.coalesceKey,
coalesceMs: GSAP_HISTORY_COALESCE_MS,
files,
});
if (Object.keys(files).length > 0) {
await input.recordEdit({
label: input.label,
kind: "timeline",
coalesceKey: input.coalesceKey,
coalesceMs: GSAP_HISTORY_COALESCE_MS,
files,
});
}
} catch (error) {
input.onFoldError(error);
}
}
return status;
@@ -201,7 +231,7 @@ export async function shiftGsapPositions(
): Promise<GsapMutationStatus> {
if (delta === 0 || !elementId) return { mutated: false, scriptText: null };
const res = await fetch(
`/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`,
`/api/projects/${encodeURIComponent(projectId)}/gsap-mutations/${encodeURIComponent(filePath)}`,
{
method: "POST",
headers: { "Content-Type": "application/json" },
@@ -233,7 +263,7 @@ export async function scaleGsapPositions(
if (oldStart === newStart && oldDuration === newDuration)
return { mutated: false, scriptText: null };
const res = await fetch(
`/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`,
`/api/projects/${encodeURIComponent(projectId)}/gsap-mutations/${encodeURIComponent(filePath)}`,
{
method: "POST",
headers: { "Content-Type": "application/json" },
@@ -298,6 +328,8 @@ export function finishClipTimingFallback(input: {
edit.to.start,
edit.to.duration,
);
const onGsapError = (err: unknown) =>
console.error(`[Timeline] Failed to ${edit.kind} GSAP positions`, err);
return finishTimelineTimingFallback({
iframe: input.iframe,
reloadPreview: input.reloadPreview,
@@ -311,9 +343,10 @@ export function finishClipTimingFallback(input: {
coalesceKey: input.coalesceKey,
recordEdit: input.recordEdit,
gsapMutation: () => runMutation(projectId, domId),
onFoldError: onGsapError,
})
: undefined,
onGsapError: (err) => console.error(`[Timeline] Failed to ${edit.kind} GSAP positions`, err),
onGsapError,
});
}
@@ -346,6 +379,7 @@ export async function finishGroupTimingGsapFallback<C extends { element: Timelin
const otherFileChanged = input.changes.some(
(change) => input.resolveChangePath(change.element) !== activePath,
);
const onGsapError = (err: unknown) => console.error(`[Timeline] ${input.errorLabel}`, err);
await finishTimelineTimingFallback({
iframe: input.iframe,
reloadPreview: input.reloadPreview,
@@ -371,7 +405,8 @@ export async function finishGroupTimingGsapFallback<C extends { element: Timelin
}
return { mutated, scriptText: otherFileChanged ? null : scriptText };
},
onFoldError: onGsapError,
}),
onGsapError: (err) => console.error(`[Timeline] ${input.errorLabel}`, err),
onGsapError,
});
}