feat(studio): export timeline editing for a host outside EditorShell - #4201

APPROVING at 99e45f0b — the etag-guarded writer is a clean byte-identical lift and the canEdit gate lands at the persist boundary, each handler resolving targets against the same source the handler itself already uses. No -heygen disagreement with the author's own scope note.

Extracted writer is byte-identical

useProjectFileWriter.ts is a straight lift of the read/write pair that used to live in useFileManager. Verified against the removed lines:

• readProjectFile — same throw new Error("Failed to read ${path}"), same data.content !== "string" throw, same fileVersions.set(path, data.version ?? response.headers.get("etag")).
• readOptionalProjectFile — same ?optional=1, same missing-content → "" fallback, same version capture.
• writeProjectFile — same studioExpectedFileVersion preflight, same expectedVersion === undefined → preflight → 404 → null path, same StudioFileConflictError on non-404 preflight failure, same retryStudioSave wrap, same If-Match / If-None-Match: "*" header logic, same 409-with-matching-content silent dedup (if (currentVersion && conflict?.currentContent === content) { fileVersions.set(path, currentVersion); return; }), same missing-version-in-response throw.
• useCallback deps preserved ([fileVersions, projectId]).
• Terminal useMemo on the return shape is dep-tight, so a consumer using the object in an effect dep won't churn on unrelated renders.

The setEditingFile mirror is the ONE piece the extraction couldn't hoist (it's editor-tab-specific), and it's preserved via a thin wrap in useFileManager around writeProjectFileRaw. The old code closure-captured writeProjectId = projectId and compared to projectIdRef.current at await-completion; the new wrap does writeProjectId = projectIdRef.current at call-time and compares the same way. Both fire iff projectId hasn't moved during the write — semantically equivalent.

canEdit gate: right seam, right target resolution

The gate lands at the persist boundary of each write kind, via checkEditable(resolveTargets(...)) inside guard. checkEditable short-circuits to true when canEdit is absent, so Studio's own behaviour is unchanged when a host doesn't opt in. Coverage matches the PR body:

• Move / resize / single-delete / element fx setQuiet: (element) => [element].
• Multi-delete: passes the argument array itself.
• Group move / resize: (changes) => changes.map(c => c.element).
• Razor-split-all: reuses the handler's own selectSplittableElements selection — not the raw store list.
• Track hide / element hide: timelineRowElements (the expanded rows the hide handlers themselves resolve against), not the raw timelineElements prop — verified by the deliberate-disagreement test at useTimelineEditing.test.tsx around L682-720.
• Audio-group setQuiet: the two-array lookup mirrored by syncStoredGroupAttribute — state.elements.filter(el.audioGroup === groupId) UNION state.domClipChildren.filter(child.audioGroup === groupId). A sub-composition-only group has no flat twin, only the domClipChildren entry, and the sub-comp-only test around L839-889 proves the resolver reaches it (last fixup commit 99e45f0b is exactly this fix).

guard and track both key their wrappers via WeakMap<fn, wrapper> refs, so a consumer using a returned handler as a memo / effect dependency doesn't re-run on every render (asserted at L895-938). trackedRazorSplit is assigned to both handleTimelineElementSplit and handleRazorSplit so the two aliases share identity.

Drop handlers (handleTimelineAssetDrop, handleTimelineFileDrop, handleTimelineCompositionDrop) are track()-only, without a guard() — the PR body explicitly scopes canEdit to edits of existing elements and calls out the drag-start/pointerdown gate as a follow-up. Non-blocking; scope boundary is named by the author.

Race regression is genuinely gated

useTimelineDeleteOps.test.tsx L437-543 exercises the historical race — a delete's write can reach disk before its history entry lands, so a fast Undo used to find nothing to restore. The regression harness uses a manually-resolved gate promise standing in for the still-in-flight recordEdit, awaits flushStudioPendingEdits() before the gate resolves, and asserts order === []. Only after gate.resolve() does order grow to ["deleted", "flushed"]. The comment explicitly notes that a real timer raced the fix's own microtasks unpredictably — the manual gate is what makes the assertion robust. Deleting useTrackPendingTimelineEdit's registration would flip this test to order === ["flushed"] before "deleted".

Public surface proven end-to-end

hostMount.test.tsx mounts hand editing OUTSIDE EditorShell using ONLY the new ./index exports — useProjectFileWriter, usePersistentEditHistory, useEditHistoryActions, useTimelineEditing, flushStudioPendingEdits, StudioFileConflictError, ExternalFileConflictBanner. Two integration cases:

• A move writes through the history path (data-start="5" on disk) and Undo restores it (data-start="0").
• A stale If-Match (via editElsewhereAfterFirstRead) produces a real 409, which the host catches as StudioFileConflictError, mounts the banner, and the banner's changed outside Studio copy appears in the DOM.

The stub disk uses studioFileContentVersion — the same content-hash the real backend and useProjectFileWriter derive If-Match from — so the conflict path exercises the actual etag algorithm rather than an ad-hoc counter that would make every write look stale.

TimelinePane.timelineOverlay

Absolute-positioned inside the pane's own bounds, pointer-events-none z-20. Below timelineDisabled's z-30 overlay, so the disabled-state cover still wins. The overlay doesn't intercept clicks, so it can't accidentally break drag/drop or clip selection. Fine.

Stamp mechanics

reviewDecision: REVIEW_REQUIRED, mergeStateStatus: BLOCKED because no reviews yet — one approval clears both. Head 99e45f0bd2ea51336e44837dfd7521f6888cbf7a authored by miguel-heygen (trusted stamper per operator note); 59/59 checks green (all SUCCESS/SKIPPED, including Windows tests, CodeQL, regression, preview-regression, and the SDK/producer/studio test suites); no concurrent reviews at this head at time of stamping.

— Review by tai (pr-review)

Reviewed the full diff at 99e45f0bd2ea51336e44837dfd7521f6888cbf7a — all 13 files, both the extraction and the new gate. Two blockers, both in the canEdit gate rather than in the extraction.

Strengths

• useProjectFileWriter.ts is a genuine lift, and I checked it rather than taking the description's word for it: extracted each moved function at the merge-base and diffed. readProjectFile 15/15 and readOptionalProjectFile 14/14 byte-identical; writeProjectFile 76 → 74 lines with exactly two deltas — the added fallow-ignore-next-line, and the 3-line editor-tab mirror relocated into useFileManager.ts. Retry, the 409 content-dedup, If-Match/If-None-Match and the missing-version throw are all preserved.
• hostMount.test.tsx is the right test for a "we now export this" PR: it mounts through the public exports only and proves write → undo plus the 409 banner, so the new index.ts surface is exercised as a contract instead of asserted.
• The resolver comments hold up under checking. handleRazorSplitAll's guard runs selectSplittableElements(usePlayerStore.getState().elements, splitTime) — the identical expression the handler itself runs at useRazorSplit.ts:123. Resolving the hide paths against useTimelineRowElements() rather than the raw store list is the right call for virtual sub-composition children.

Blockers

1. useTimelineEditing.ts:72-91 — the guard cache pins the first canEdit it ever sees, and the gate then fails open

guardedRef is a useRef WeakMap that lives for the component's lifetime, keyed on the inner handler fn. wrapped (:86) closes over checkEditable, which is useCallback(…, [canEdit, showToast]) and so takes a new identity whenever the host's canEdit does. But fn's identity does not depend on canEdit, so the next render hits the cache at :84 and gets back the wrapper built with the previous canEdit. Nothing invalidates the entry.

I simulated the exact shape (the only React semantics involved are "useRef keeps one object across renders" and "useCallback rebuilds when its dep changes"):

render1 (canEdit=BLOCK)  -> writes: []                    OK refused
render2 (canEdit=ALLOW)  -> writes: []                    STALE: still refusing with render-1 canEdit
wrapper identity reused across renders: true
allow-then-BLOCK         -> writes: ["clip-1","clip-1"]   STALE: write went through while BLOCKED

The second direction is the one that matters: a host that allows a clip and later marks it reserved keeps getting writes through. For a feature whose contract is "blocked means no write, no history entry," the gate does not hold past the first canEdit identity.

This PR's own stability test is what makes it reachable rather than theoretical — useTimelineEditing.test.tsx renders, bumps state, and asserts seen[0] === seen[1], i.e. the wrapper is reused across renders. That assertion and this bug are the same mechanism. And no test varies canEdit: every canEdit case builds an inline arrow and renders once, so the suite cannot see it.

Fix that keeps the wrapper identity stable (so the bump test still passes) while reading the gate live — the pattern projectIdRef already uses in this file:

const checkEditableRef = useRef(checkEditable);
checkEditableRef.current = checkEditable;
// …inside wrapped:
checkEditableRef.current(resolveTargets(...args)) ? fn(...args) : Promise.resolve()

Either way, worth stating: nothing documents that canEdit has to be referentially stable, and showToast sits in the same dep list.

2. useTimelineEditing.ts:500 — handleAutoGroupCarveSources is track()-ed but not guard()-ed, and it is not in the "Not covered" list

The body excludes "the three drop handlers, since they add new content rather than edit an existing element." That reasoning does not reach this one. useAudioGroupCarveAssignment → createAudioGroupAndAssignMembers resolves existing clips, patches hf-audio-group onto each via buildPatchTarget(element), and persists through writeProjectFile + recordEdit (timelineAudioGroupCreate.ts:235-236). That is a write to existing elements with a history entry — exactly the shape canEdit exists to refuse — so a host that blocks a clip can still have it silently re-attributed into an audio group on disk.

The resolver is already sitting in the handler: the same elements array it builds from expandedElements (timelineAudioGroupCreate.ts:286-291). Either gate it there, or move it into the "Not covered" list with the real reason.

Important

3. timelineElementFxAttribute.ts:96 and timelineAudioGroupVolume.ts:214 — only setQuiet is guarded, setLive is not

Consistent with the stated "persist boundary" scope, but it has a visible consequence worth an explicit decision. setLive patches the preview iframe directly, and the volume strip calls it on every drag then setQuiet once on release. On a blocked element the user drags, watches the value change, releases, and gets the refusal toast — while the live DOM keeps the un-persisted value until a reload. Nothing reverts the optimistic patch on refusal, the way the carve's own failure path does at timelineAudioGroupCreate.ts:241-248.

Nit

4. useFileManager.ts — the wrapper's writeProjectId is now read from the ref at call time

The original read the closure's projectId; the new wrapper reads projectIdRef.current. For the actual race — the project switching during the await — these agree, because the ref still held the old id at call entry. They diverge only when a stale wrapper is invoked after a switch: the old code compared against the project the callback was built for and skipped the mirror, while the new code compares the current project against itself, so the mirror fires and the editor tab can show content written to the previous project. I could not construct a reachable caller for that, and the write itself lands in the correct project either way — so this is a note, not a defect claim. Capturing the closure's projectId restores exact parity for free.

Scope

Read end-to-end: all 13 changed files, plus useTimelineGroupEditing.ts, useRazorSplit.ts, timelineElementSplit.ts, timelineAudioGroupVolume.ts, timelineElementFxAttribute.ts, timelineAudioGroupCreate.ts, studioPendingEdits.ts, and the merge-base useFileManager.ts. Checked that replacing ...groupEditing with two explicit keys drops nothing — useTimelineGroupEditing.ts:483 returns exactly those two. I did not run the suite.

Additive note: the existing approval at this head covers the extraction parity and the resolver choices, and I reached the same conclusion on both independently, so I have not repeated that ground. The two blockers are gaps outside it. The one claim I measured differently is the writeProjectFile projectId capture, and I have narrowed it to note 4 rather than carrying it as a defect.

Verdict: REQUEST CHANGES
Reasoning: The extraction is clean and provably byte-identical, but the gate this PR adds does not hold — it is pinned to the first canEdit identity and fails open in the allow→block direction, and one persisting edit path on existing elements is neither gated nor disclosed.

— Rames Jusso

Re-review at 22c9fcc0db233fa8d2ccb9f86ed7d4abe2fc27fe, clearing my own CHANGES_REQUESTED from 99e45f0b. Both prior blockers are resolved; I re-measured rather than reading the patch.

I measured the fixes at 8ec8e999 and the head moved while I was writing this up. The 8ec8e999 → 22c9fcc0 push is one line break in a type intersection in playerStore.ts:63-64 and nothing else — I diffed it rather than taking the commit subject for it, and the guard code quoted below is byte-identical at both.

Prior blocker 1 — the guard cache pinned the first canEdit, so the gate failed open on allow→block

Fixed at useTimelineEditing.ts:78-79,104-115: the wrapper now reads checkEditableRef.current(...), resolveTargets became a mutable field on the cached entry so a fresh resolver closure is picked up too, and the factory's dep list is [].

Verified by re-running the same standalone simulation I used to raise it (useRef keeps one object for the component's life; useCallback rebuilds the factory closure), with both guard shapes side by side:

=== arm 99e45f0b ===
  BLOCK then ALLOW -> writes: []                     STALE: still refusing with render-1 canEdit
  ALLOW then BLOCK -> writes: ["clip-1","clip-1"]    STALE: write went through while BLOCKED
  wrapper identity reused across renders: true

=== arm 22c9fcc0 ===
  BLOCK then ALLOW -> writes: ["clip-1"]             live
  ALLOW then BLOCK -> writes: ["clip-1"]             live: refused
  wrapper identity reused across renders: true

The third line of each arm is the part worth calling out: identity stability is preserved, so the existing "same wrapped handler across renders" regression test still holds for the reason it was written rather than because of the bug. And re-reads canEdit after the host changes its verdict is the right pin — flipping the verdict with useState after the first render is precisely what the previous suite structurally could not see, since every earlier canEdit case built an inline arrow and rendered once.

Prior blocker 2 — handleAutoGroupCarveSources tracked but never gated

Fixed at timelineAudioGroupCreate.ts:283,306,351 by threading checkEditable into useAudioGroupCarveAssignment and asserting before the try.

I checked the conditional (if (elements.length === wanted.size)) for a bypass and there isn't one: the !== branch throws Cannot group: no timeline clip for … at :312, inside the try and before any write, so no path reaches createAudioGroupAndAssignMembers with the check skipped. Rejecting rather than resolving quietly is also the right shape here specifically — it is the same contract the "rethrown, not just reported" comment already depends on to stop the carve persisting sources: [groupId] for a group that was never written. Test asserts no fetch, no writeProjectFile, no recordEdit, no data-audio-group on the survivor, and no group element created.

Prior important — setLive ungated on both attribute hooks

Closed a better way than I proposed. setLive records the pre-drag value once per (target, attr) and guard's new onRefused reverts it on refusal, with the audio-group path re-syncing the store (timelineAudioGroupVolume.ts:235-252, timelineElementFxAttribute.ts:117-131). The test asserts both halves — DOM data-volume back to 1 and usePlayerStore audioGroupVolume back to 1 — which is the assertion that would have caught a half-revert.

Also re-checked on the new surface: createTimelineResetState moving out of playerStore.ts into timelineResetState.ts is line-for-line identical (46/46) apart from the one intended rangeSelection: null, addition, and it stays re-exported from playerStore, so existing importers are untouched.

important — the description documents fcb2759f, not this head

"Not covered by this PR: the three drop handlers, since they add new content rather than edit an existing element" is no longer true: dropAssetAt (useTimelineAssetDropOps.ts:97), handleTimelineFileDrop (:215) and handleTimelineCompositionDrop (:261) are all gated at this head. The description also doesn't mention onRangeSelect / TimelineTimeRange / usePublishRangeSelection, the new rangeSelection store field, the HyperframesLoader export, or the revert-on-refusal behaviour — all host-visible.

On a PR whose entire value is a host-facing API contract, the description is the contract document, and it currently tells an integrator the opposite of what the code does. Worth a pass before merge rather than after.

important — the drop gate asks canEdit about a fabricated element the contract doesn't describe

timelineDropTarget (useTimelineAssetDropOps.ts:41-53) synthesises { id: "timeline-drop", tag: "div", duration: 0, sourceFile, start, track } and hands it to the host's predicate, whose documented contract is canEdit(element) — "a host's verdict on one element" (timelineEditPermission.ts:4-7).

A host has no way to answer that correctly without knowing the sentinel. The natural implementation — look the id up in my reserved set, allow what I don't recognise — returns true for "timeline-drop", so every drop is permitted and the protection silently doesn't apply on this path. The opposite convention (deny what I don't recognise) blocks every drop instead. The meaningful payload is sourceFile / start / track, not the id, and nothing says so.

Neither drop test exercises this: both pass checkEditable in directly as a blanket () => false, so useTimelineEditGate — and therefore a real canEdit(element) — is never on the path. The plumbing is pinned; the host contract isn't.

Cheapest fix is to document the placement-probe shape on CanEditTimelineElement and in the description. A separate placement-level predicate would be cleaner, but that's a bigger change than this PR needs.

nit — the file-drop pre-check probes a placement it may not use

handleTimelineFileDrop gates on placement ?? { start: 0, track: 0 } (:215) while each file's real placement is computed later and re-gated inside dropAssetAt. A host that blocks by track can refuse a drop at track 0 that would actually have landed on track 3. Availability-only — the per-file check is still authoritative — but the first probe can lie.

Verdict: APPROVE
Reasoning: Both blockers I raised are fixed and each is now pinned by a test that fails against the old code; I re-ran the guard simulation against both heads rather than reading the patch, and confirmed the extraction in this delta is byte-identical apart from one intended field. The two remaining items are a stale description and an undocumented host convention on a new opt-in path — neither regresses existing behaviour, since canEdit is absent by default.

One mechanic worth stating, since this PR has been moving: require_last_push_approval: true here means an approval counts only at the exact head it was posted against. This one is pinned to 22c9fcc0; anything that lands after it needs a fresh stamp.

— Rames Jusso