feat(studio): add Ctrl+C/V/X copy/paste for timeline clips and DOM elements

[miguel-heygen]

Summary

• Adds Ctrl+C / Ctrl+V / Ctrl+X keyboard shortcuts for copying, pasting, and cutting timeline clips and DOM elements
• Contextual dispatch: timeline clip selected → clip operations; DOM element selected → element operations
• Pasted timeline clips re-timed to playhead position with deduplicated IDs
• DOM elements pasted as siblings (same parent context) preserving CSS inheritance
• All operations fully undoable via Ctrl+Z
• Cross-frame instanceof fix for iframe DOM access

Test plan

• 5 unit tests for clipboard payload (dedup, serialization, round-trip)
• Build passes, lint clean
• Manual: copy/paste timeline clips, DOM elements, undo all work

[miguel-heygen]

• feat(studio): Timing inspector + fix mixed-content text editing #896
• perf(studio): lightweight preview reload, skip asset overlay #895
• feat(studio): add Ctrl+C/V/X copy/paste for timeline clips and DOM elements #894 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[jrusso1020]

LGTM — clean Ctrl+C/V/X plumbing with right defensive shape on the high-risk surfaces.

Audited

• isEditableTarget guard (useAppHotkeys.ts) on every Cmd/Ctrl+C/V/X handler — prevents the system shortcut hijack when focus is in a text input. Critical and present. ✓
• Contextual dispatch in handleCopy — timeline-clip-selected branch first, DOM-element-selected second, false otherwise. Priority order is consistent with the rest of the studio. ✓
• deduplicateIds counter-with-Set pattern correctly avoids collision-of-collisions: each renamed ID is added to the existing set so a pasted clip with id="hero" colliding twice produces hero-2 then hero-3, not two hero-2s. ✓
• handlePaste re-reads file content fresh before applying — uses /api/projects/{pid}/files/{path} to get the current source-of-truth, then collectHtmlIds(originalContent) to compute the fresh existing-ID set. ✓ Drift-tolerant.
• saveProjectFilesWithHistory integration — paste is undoable via Ctrl+Z, body's claim verified. ✓
• handleCut = copy-then-delete, with re-read of selection state between copy and delete. Tiny race window but the selection ref is synchronous, so practically negligible.

Non-blocking notes

1. insertAsSibling regex HTML walker is brittle but fail-soft — the selector parsing only handles #id / .class / [attr=...] shapes; compound selectors (.scene .video) fall through to root-fallback. The class regex requires double-quote class="..." (single-quote class='...' won't match). The findClosingTagPosition walker doesn't handle HTML comments, <script> / <style> text content, or void elements without />. For paste UX this is acceptable — worst case is "inserts somewhere in the composition" rather than data corruption — but worth a comment noting "HTML parsing here is regex-based and fails soft to root-insert for anything fancy."

2. Single-occurrence data-start regex replace on timeline-clip paste:

   deduped.replace(/data-start="[^"]*"/, `data-start="${formatTimelineAttributeNumber(currentTime)}"`)

   .replace(regex, str) is single-match by default. If a pasted timeline clip has nested elements with their own data-start (e.g., a wrapper clip containing inner timeline elements), only the outermost gets retimed; inner data-starts stay at their original absolute values. Edge case, but if nested timeline clips exist in this codebase it'd produce visual offset bugs. Worth either confirming nested case is impossible or switching to .replace(...g) with a per-occurrence delta calculation.

3. System clipboard is one-way — copyTextToClipboard(serializeClipboardPayload(...)) writes the payload but handlePaste only reads clipboardRef.current (in-memory). So cross-window / cross-tab paste from the system clipboard isn't supported — pasting from another studio window won't work. Likely intentional (prevents foreign HTML injection from arbitrary clipboard content), but worth a one-line comment documenting the design choice.

4. Test coverage on pure-function layer only — 5 unit tests cover deduplicateIds, serialize/deserialize round-trip, invalid-JSON rejection. But insertAsSibling (the complex HTML walker, the part most likely to silently mis-insert) and findClosingTagPosition aren't tested. The whole handleCopy / handlePaste / handleCut integration also isn't exercised — no test asserts "copy clip A, paste, file contains clip B with retimed data-start." Worth adding before the next refactor of this code touches it.

CI

mergeable_state: blocked is reviewer-gate. Will trust the standard CI matrix.

— Review by Rames Jusso (pr-review)

[vanceingalls]

One-line summary: solid framing (in-memory ref + system-clipboard serialization), but the regex used for ID dedup corrupts data-*-id attributes on every paste, and the capture-phase keydown handler eats native browser copy/paste outside true inputs.

Calibrated strengths

• useClipboard integration into App.tsx is clean — dispatch is contextual (timeline-clip vs DOM-element) and reuses saveProjectFilesWithHistory so paste is undoable through the existing history pipe (useClipboard.ts:145-153). Nice.
• serialize/deserialize with a _marker discriminator is the right shape for round-tripping payloads (clipboardPayload.ts:18-46). Round-trip tests pin both kinds.
• Fallback chain in insertAsSibling — selector match, then composition root, then end-of-source — is the right "never silently drop" posture (clipboardPayload.ts:81-99).

Blockers

• deduplicateIds regex \bid="([^"]+)" matches data-*-id="..." and rewrites them. \b is a word boundary; the - before id is a non-word char, so the boundary fires inside data-composition-id, data-clip-id, data-element-id, data-track-id, etc. Verified locally with Node:

  <div data-composition-id="hero"> => ['hero']  ← matched & subject to rewrite
  

  Every composition root in the codebase uses data-composition-id="<name>" (grep shows ~50+ in packages/studio/src/). When you copy any clip whose subtree includes a composition root, paste rewrites the composition id → manifest cross-references break, and insertTimelineAssetIntoSource may then fail to re-find the root in a subsequent paste. Fix: anchor the regex with (?:^|\s)id="..." (or a lookbehind) so only literal HTML id attributes are matched. Add a test:

  expect(deduplicateIds('<div data-composition-id="hero">', ['hero']))
    .toBe('<div data-composition-id="hero">');

  Same flaw lives in studioHelpers.ts:166-168 (collectHtmlIds), pre-existing — but this PR is the first place where matches are rewritten, so the existing leniency becomes a corruption bug. (clipboardPayload.ts:166-176)

• Capture-phase Ctrl/Cmd+C handler eats native browser copy for text selections outside <input>/<textarea>/contenteditable. The hotkey listener is attached window.addEventListener("keydown", ..., true) and additionally forwarded into the preview iframe's contentWindow (useAppHotkeys.ts:237-238, 254-256). When a user selects text in the preview iframe (a <p>, <span>, label, error message — anywhere outside a true input) and hits Ctrl/Cmd+C, isEditableTarget(event.target) returns false (it walks closest("input, textarea, select, [contenteditable='true'], [role='textbox'], .cm-editor")), so this branch runs and calls event.preventDefault() — clobbering the browser's default text copy. Same for Ctrl/Cmd+X (cut). The current Delete/Backspace handler at line 206-227 gets away with this because no one expects native Delete to copy text; Ctrl+C does. Two viable fixes: (1) only call event.preventDefault() after you've confirmed there's a selected timeline clip or DOM element, and explicitly return early if neither — let native copy run; (2) check window.getSelection()?.toString() and defer to native when text is selected. Today the early-return inside handleCopy returns false but the keydown handler already called preventDefault() before invoking it. (useAppHotkeys.ts:212-217, 219-224, 226-231)

Important

• data-start re-time uses non-global replace(): deduped.replace(/data-start=\"[^\"]*\"/, ...) in useClipboard.ts:127-130 only rewrites the first occurrence. If the copied subtree contains nested timeline clips (each with its own data-start), the inner clips keep their original start time → the pasted hierarchy is timing-broken in a way that silently looks fine on first render. Either add the g flag (rewrites every data-start to the same playhead — also wrong but predictable), or — better — only rewrite the root element's data-start by parsing the outer tag.

• reloadPreview behavior change is a drive-by, unrelated to copy/paste. App.tsx:113-117 swaps contentWindow.location.reload() for setRefreshKey(k => k + 1). The two are not equivalent: reload preserves the iframe DOM node (so refs, console-error capture, hotkey forwarding stay attached); refresh-key forces an unmount/remount, dropping all of that. This affects every existing caller of reloadPreview (timeline editing, manifest persistence, DOM edit session) — not just the new paste path. Either split this out into its own PR with a clear "why" in the body, or justify here why the old try/catch needed replacing.

• Cross-tab paste advertised but not implemented. copyTextToClipboard(serializeClipboardPayload(payload)) writes to the system clipboard (useClipboard.ts:78, 105), but handlePaste reads only from clipboardRef.current (useClipboard.ts:114-117). User copies in one Studio tab, switches tabs, pastes → "Nothing to paste." silently. If the system-clipboard write is intentional (for paste into external tools? unclear), add a paste path that reads via navigator.clipboard.readText(), parses with deserializeClipboardPayload, and falls back to the in-memory ref. If it's not intentional, remove the write — half-built plumbing is worse than none.

• Hand-rolled HTML parser is fragile. findClosingTagPosition in clipboardPayload.ts:108-152 uses html.indexOf(">", ...) to find tag ends, which breaks on attribute values containing > (e.g. data-x="a>b">), HTML comments containing tag-like text (<!-- <div> -->), and <script> contents. Verified locally: comments containing <div> make the parser return -1 (falls back to composition-root insert — wrong location). For paste-as-sibling on user-authored composition HTML this will rarely trip in practice, but the failure mode is "paste lands silently in the wrong place." Consider using the iframe's parsed DOM (previewIframeRef.current.contentDocument.querySelector) to compute insertion point, then serialize.

Nits

• event.altKey not checked in the new Ctrl+C/V/X branch. Cmd+Alt+V is "paste as plain text" in many editors and an OS gesture some users have muscle memory for. Add && !event.altKey to match the Delete branch's discipline.
• useClipboard.ts:142-150 deps list omits previewIframeRef and handleCopy's own deps list omits it too — refs don't need to be in deps, so technically fine, but inconsistent with the rest of the file which lists domEditSaveTimestampRef etc.
• The cut test plan is "manual" — no unit test asserts the copy-then-delete ordering or that handleCut no-ops when handleCopy returns false. A unit test with mocked handleTimelineElementDelete would be cheap insurance.
• showToast(payload.kind === "timeline-clip" ? "Pasted clip" : "Pasted element", "info") — minor, but the toast fires even when insertAsSibling falls through to the end-of-source fallback. Users get "Pasted element" with no indication the element landed somewhere they didn't expect.

Verdict: REQUEST CHANGES
Reasoning: Two correctness blockers — deduplicateIds corrupts data-*-id attrs on every paste that touches a composition root, and the capture-phase Ctrl+C handler clobbers native browser copy for any non-input text selection. Both are user-visible failures on common flows and need fixing before merge.

Review by Vai

[jrusso1020]

Self-correction: deferring to Vai's REQUEST_CHANGES — my APPROVE missed two real blockers and underweighted a third.

What I got wrong

Blocker 1 — deduplicateIds corrupts data-*-id attributes (real data corruption, not "fail-soft fragility")

I flagged the regex-based HTML manipulation in insertAsSibling as "brittle but fail-soft." That framing applied to insert-position-might-be-wrong shapes, not data-mutation-might-corrupt shapes. The deduplicateIds regex is the latter:

html.replace(/\bid="([^"]+)"/g, ...)

\b is a word boundary — it fires between any word/non-word transition. - is non-word, i is word. So \b fires in the gap between - and id= in data-composition-id="x". The regex matches id="x" inside data-composition-id="x" and rewrites it if "x" collides with the existing-ID set. Net: every paste touching the composition root corrupts data-composition-id, data-clip-id, and any other data-*-id attribute that shares a value with a top-level id. Manifest refs break silently.

I should have caught this — the lesson is "when reviewing regex-based attribute manipulation, enumerate which attribute names could false-positive given the regex's anchors. \b doesn't anchor against hyphens." Vai's fix shape — (?:^|\s)id="..." or operate on parsed DOM — is the right shape.

Blocker 2 — capture-phase preventDefault() eats native browser copy

I checked that isEditableTarget gates the studio shortcut paths — but missed that preventDefault() runs BEFORE handleCopy checks whether anything is actually selected in the studio. So when the user has text highlighted in the preview iframe (which the parent's isEditableTarget check doesn't see as editable), preventDefault() fires and handleCopy returns false → native browser copy is silently suppressed.

The right shape is what Vai suggests: check selection state first, only preventDefault() when the studio path is actually going to handle the event. Otherwise let the browser do its native thing.

Important 1 — non-global data-start replace — convergent finding; this stays at "important" severity in both reviews.

Important 2 — drive-by reloadPreview change: I noted the location.reload → refreshKey++ change without flagging the scope concern. Vai's right that this affects every caller of reloadPreview, not just the paste path this PR introduces. Either spin into a separate PR or document the cross-cutting effect. (hf#895 actually depends on this being the case — but that's a coupling worth being explicit about, not a free side effect.)

Important 3 — cross-tab paste claim: I noted "system clipboard is one-way" as a design choice; Vai's reframing is sharper — if copyTextToClipboard(serializeClipboardPayload(...)) writes to system clipboard but handlePaste only reads clipboardRef.current, the clipboard write is dead code that misleads readers into thinking cross-tab paste works. Either remove the clipboard write or wire handlePaste to read system clipboard as a fallback.

Important 4 — hand-rolled HTML parser fragility: convergent; Vai is sharper on the fix — DOMParser is the right tool for trusted HTML, removes the regex-anchor risk class entirely.

Calibration

My APPROVE underrated the severity of the regex shape. I flagged it as "brittle but fail-soft," but two specific anchor-edge cases (the \b-on-hyphen and the preventDefault-before-check) are fail-CORRUPT and fail-EAT-BROWSER-DEFAULT respectively. Treating my prior APPROVE as superseded by Vai's REQUEST_CHANGES.

Happy to re-stamp once:

1. deduplicateIds uses anchored matching that doesn't catch data-*-id (or switches to DOMParser)
2. preventDefault is gated on selection state, not just !isEditableTarget
3. Either non-global replace() on data-start becomes scoped or nested-timeline-clip case is documented as out of scope
4. Either the clipboard write is removed (in-memory-only paste) or handlePaste is wired to read system clipboard as a fallback

— Self-correction by Rames Jusso (pr-review)

[jrusso1020]

Re-approving on c9c14ac3. Both blockers from Vai's REQUEST_CHANGES + my self-correction are properly fixed.

Audited

Blocker 1 — deduplicateIds regex no longer matches data-*-id

// Before:  /\bid="([^"]+)"/g
// After:   /(?<=\s)id="([^"]+)"/g

Zero-width lookbehind (?<=\s) requires whitespace immediately before id=. Walking the cases:

• <div id="x"> — space before id ✓ matches
• <div data-foo="bar" id="x"> — space between attrs ✓ matches
• <div data-composition-id="x"> — - before id, NOT whitespace → doesn't match ✓ fixed
• <div\nid="x"> / <div\tid="x"> — newline/tab are whitespace ✓ matches (multi-line formatting safe)
• Attribute values containing id= substrings (e.g., data-prefix="some id=") — =" isn't whitespace ✓ no false-positive

New regression test pins all three cases in one assertion:

expect(result).toContain('data-composition-id="hero"');
expect(result).toContain('data-clip-id="hero"');
expect(result).toMatch(/\sid="hero-\d+"/);

That's the exact shape I'd want — proves the attribute distinguishing logic instead of just spot-checking one of them.

Blocker 2 — preventDefault conditional on actual studio handling + altKey guard

if (
  copyPasteKey === "c" &&
  !event.shiftKey &&
  !event.altKey &&
  !isEditableTarget(event.target)
) {
  if (handleCopyRef.current()) {
    event.preventDefault();
  }
  return;
}

Two changes:

1. !event.altKey excludes Alt/Option+C from interception (Mac Option+C / Linux Alt+C aren't standard copy shortcuts)
2. preventDefault runs only when handleCopy actually copied something. False return (no studio selection) → fall through to native browser copy.

handleCopy already returned boolean per the existing contract, so the gating is the minimal change. ✓

For Cmd/Ctrl+V and Cmd/Ctrl+X the altKey guard is added but preventDefault is still unconditional — correct asymmetry, since paste/cut don't have a useful native fallback on non-editable targets (the isEditableTarget check upstream already routes those to the browser).

Side cleanup — removed the no-op inner.replace(/"/g, '"') identity replace in the [attr=...] selector path. Cosmetic.

Remaining importants from prior round (not addressed, all still non-blocking)

• Non-global replace() on data-start re-time (nested timeline clips keep stale starts)
• Drive-by reloadPreview change affects all callers, not just paste
• Clipboard write is one-way (cross-tab paste claim implied but not supported)
• Hand-rolled HTML parser fragility (DOMParser would be cleaner for trusted HTML)

These were "important non-blocking" in both reviews. Miguel can ship without them; worth scheduling as follow-ups.

CI

mergeable_state: blocked is reviewer-gate. Trust the standard matrix.

Withdrawing my prior self-correction.

— Re-review by Rames Jusso (pr-review)

[vanceingalls]

Re-review (Vai). Thanks for the quick turn on the deduplicateIds regex + the Cmd+C guard + the altKey check — those are solid. One blocker remains as a sibling-site issue (Rule 2: audit every site that satisfies the contract), and four importants from the previous round are still open.

Status of prior findings

• Blocker #1 — deduplicateIds regex. ADDRESSED. The lookbehind (?<=\s)id="..." at packages/studio/src/utils/clipboardPayload.ts:166 correctly skips data-composition-id, data-clip-id, etc. The new test at clipboardPayload.test.ts:24-29 pins this behaviour. Nice fix.

• Blocker #2 — capture-phase keydown eats native browser copy. PARTIAL. The C branch now gates preventDefault() behind handleCopyRef.current() returning true (useAppHotkeys.ts:213-223) — exactly right. But the symmetric X branch (useAppHotkeys.ts:226-231) still calls preventDefault() unconditionally before invoking handleCut. The original review called out "Same for Ctrl/Cmd+X (cut)." — that site still has the same failure mode: a user selecting text in the preview iframe (or anywhere outside an input) and hitting Cmd+X gets native cut clobbered while handleCut→handleCopy silently returns false. Fix: mirror the C branch shape — let handleCut return a boolean (or pre-flight the "is anything selected to cut?" check), only preventDefault() when there's a target. (useAppHotkeys.ts:226-231)

• Important #1 — non-global data-start re-time. OPEN. useClipboard.ts:124-127 still uses a non-global replace(/data-start="[^"]*"/, ...). If a copied subtree contains nested timeline clips with their own data-start, only the outermost gets re-timed — nested clips keep stale start times. Either rewrite only the root tag's data-start (parse + replace on the outer opening tag) or, at minimum, document why the non-global behavior is intentional.

• Important #2 — drive-by reloadPreview change. OPEN. App.tsx:113-115 still swaps iframe.contentWindow.location.reload() for setRefreshKey(k => k + 1). This still affects every existing caller (timeline editing, manifest persistence, dom edit) by forcing an iframe unmount/remount instead of an in-place reload. If it's needed for the paste path, please justify in the PR body and call out the behavior change for the other callers; otherwise split it out.

• Important #3 — cross-tab paste advertised but not implemented. OPEN. useClipboard.ts:105 still writes serializeClipboardPayload(payload) to the system clipboard, but handlePaste at line 150 reads only clipboardRef.current. Cross-tab paste fails silently with "Nothing to paste." Either consume navigator.clipboard.readText() + deserializeClipboardPayload (with the in-memory ref as fast-path), or remove the clipboard write.

• Important #4 — hand-rolled HTML parser fragility. OPEN. findClosingTagPosition in clipboardPayload.ts:108-152 is unchanged. Attribute values containing >, HTML comments containing tag-like text, and <script> contents will still confuse indexOf(">") and produce a -1 return (fall through to composition-root insert). Consider parsing via the iframe's contentDocument.querySelector + serializing back, or scope the input domain to known-safe shapes.

Fresh observations on the new bytes

• useAppHotkeys.ts:218-223 — nit: now that handleCopy returns a boolean and gates preventDefault, the C handler skips its return if handleCopy returns false. That's intentional (let native copy run), but it means downstream branches in the same handler still get evaluated for the same event. Currently harmless because the lower branches require non-C keys, but a future maintainer might add a branch that fires on C and get a surprise. Consider an early return after the C/V/X copy/paste/cut block regardless of outcome.
• clipboardPayload.test.ts:24-29 is a good positive-pin test; consider adding a negative pin too: e.g., assert data-track-id="hero" survives the rewrite (covers a different -id suffix from -composition-id / -clip-id).

Verdict: REQUEST CHANGES
Reasoning: The Cmd+X branch still clobbers native cut (Rule 2 — same blocker pattern as the C-branch fix landed on, applied to a sibling site). The four importants from the prior round are all still open. The C-branch fix and deduplicateIds lookbehind are clean and ready.

Review by Vai (re-review)

[jrusso1020]

Self-correction: agreeing with Vai's REQUEST_CHANGES — I missed the X-branch parity.

In my re-approve I framed the V and X branches as "correct asymmetry — paste/cut don't have a useful native fallback on non-editable targets." That framing is wrong on X.

Why C and X should be symmetric, not C-only:

• Copy (C) is a source-side operation — reads selection, writes clipboard. Native browser Cmd+C works on non-editable text selections (copy to clipboard from read-only content).
• Paste (V) is a destination-side operation — reads clipboard, writes to a target. Native browser Cmd+V only does something in editable contexts (input, textarea, contenteditable) — the isEditableTarget upstream gate routes those to the browser.
• Cut (X) is also source-side — reads selection, writes clipboard, removes source. Native browser Cmd+X requires an editable source (you can't remove non-editable content), BUT browsers may still fire a cut event for instrumentation or extensions. Suppressing it before knowing whether the studio will actually cut something is the same shape as the C-branch bug.

So C and X both need the conditional-preventDefault shape; only V is correctly unconditional. The right fix is what Vai suggests — make handleCut return boolean (mirroring handleCopy) and gate preventDefault on the return value:

if (copyPasteKey === "x" && !shift && !altKey && !isEditableTarget(target)) {
  if (await handleCutRef.current()) {
    event.preventDefault();
  }
  return;
}

Since handleCut is async (calls delete handlers that return Promises), the gating needs to be inside the async path. Either await the result before deciding preventDefault, or — simpler — have handleCut synchronously check handleCopy()'s return value first (it already does this), preventDefault only when the synchronous check passes, then proceed with the async delete:

const willCut = handleCopyRef.current();
if (willCut) {
  event.preventDefault();
  void handleCutRef.current();
}
return;

That mirrors the C-branch shape without changing handleCut's signature.

Also acknowledging Vai's fresh nit on fallthrough docs: with conditional preventDefault, future maintainers might wonder why the return is still there when the event wasn't handled. A one-line comment like // Return even on no-op to avoid double-firing C as a subsequent hotkey would clarify the intent.

The 4 prior importants (non-global data-start replace, drive-by reloadPreview, clipboard cross-tab claim, hand-rolled HTML parser) are still open as follow-ups in both reviews. Treating my prior APPROVE as superseded by Vai's REQUEST_CHANGES once again. Happy to re-stamp when the X branch is mirrored.

— Self-correction by Rames Jusso (pr-review)

[jrusso1020]

Re-approving on f670eb9b. Both blockers fixed + three of four importants landed.

Audited

Blocker 2 fix (Cmd+X gating) — useAppHotkeys.ts:240-251:

const hasSelection =
  !!usePlayerStore.getState().selectedElementId || !!domEditSelectionRef.current;
if (hasSelection) {
  event.preventDefault();
  void handleCutRef.current();
}
return;

Mirrors the C-branch's "only preventDefault when there's something to handle" shape, but uses a synchronous selection-state check inline rather than gating on handleCut's return value. Cleaner — avoids the async-decide-preventDefault path. Race-analysis: hasSelection reads sync state; if selection clears between check and async cut, the cut is a no-op (handleCopy returns false → handleCut returns false → no destructive side effect). preventDefault has already fired, but that's fine — the user pressed Ctrl+X with a selection visible, intent was clear. ✓

handleCut signature widened from Promise<void> to Promise<boolean> for symmetry with handleCopy; not directly used by the X branch but type-consistent.

Important 1 (non-global data-start retiming) — fixed at useClipboard.ts:164-171:

const rootTagEnd = deduped.indexOf(">");
const rootTag = rootTagEnd >= 0 ? deduped.slice(0, rootTagEnd + 1) : deduped;
const patchedRootTag = rootTag.replace(/data-start="[^"]*"/, `data-start="${...}"`);
const withNewStart = patchedRootTag + deduped.slice(rootTagEnd + 1);

Scope-limits the regex to just the opening tag. Nested clips keep their own data-start attributes. Comment documents the contract. ✓

Theoretical fragility: indexOf(">") would split mid-attribute if a > literal appeared inside a quoted attribute value (e.g., data-foo="x>y"). In practice, outerHTML serializes > to > in attribute contexts where it could confuse a parser, so the real-world risk is near-zero. Worth a one-line comment noting "outerHTML guarantees no literal > inside attribute values" if you want to pin the invariant.

Important 2 (drive-by reloadPreview change) — reverted at App.tsx:112-118:

const reloadPreview = useCallback(() => {
  try {
    previewIframeRef.current?.contentWindow?.location.reload();
  } catch {
    setRefreshKey((k) => k + 1);
  }
}, []);

Back to the original location.reload() primary with setRefreshKey as cross-origin fallback. Scope cleanly unwound from this PR. The hf#895 refreshPlayer() perf path still kicks in via NLELayout's useEffect when the catch fires; both paths avoid Player teardown so user-observed behavior is unchanged. ✓

Important 3 (clipboard one-way / cross-tab claim) — fixed at useClipboard.ts:112,134:

Both void copyTextToClipboard(...) calls removed. Imports cleaned up. The clipboard is now explicitly in-memory-only via clipboardRef.current, no misleading half-implementation. ✓ Right call — better to remove the dead clipboard write than to leave readers wondering whether cross-tab paste should work.

Important 4 (hand-rolled HTML parser) — still open as expected; this would be a larger refactor (switch to DOMParser) and is appropriately follow-up scope.

Nit

handleCut always returns true after the dom-selection branch (line 224 in the new file), even though the explicit early-return at line 220 returns true and the only path to fall through is "no dom selection either." The final return true; could be return false; for accuracy ("nothing was actually cut"), or refactored to return !!domSelection; on the line above. Doesn't matter functionally since the X-branch caller doesn't use the boolean, but the signature change implies the return value is meaningful.

CI

mergeable_state: blocked reviewer-gate. Trust the standard matrix.

Withdrawing prior self-correction.

— Re-review by Rames Jusso (pr-review)

[vanceingalls]

Re-review round 3 (Vai). Clean turnaround — the Cmd+X gate is now properly mirrored on the Cmd+C pattern, the data-start regex is scoped to the root tag, the reloadPreview drive-by was reverted, and the dead system-clipboard write was removed. Shipping.

Status of prior findings

• Blocker #2 — Cmd+X clobbers native cut. ADDRESSED. useAppHotkeys.ts:240-256 now pre-checks hasSelection = !!usePlayerStore.getState().selectedElementId || !!domEditSelectionRef.current before calling preventDefault() and handleCut. When nothing is selected, native cut runs unmolested — exactly matching the Cmd+C fix shape from round 2.
• Important #1 — non-global data-start re-time. ADDRESSED. useClipboard.ts:159-170 now slices the root opening tag (deduped.slice(0, deduped.indexOf(">") + 1)), runs the non-global replace on just that, and concatenates the rest back unchanged. Nested clips keep their original data-start. The comment justifying the non-global regex is also a nice touch.
• Important #2 — drive-by reloadPreview change. ADDRESSED. App.tsx:113-118 reverted to previewIframeRef.current?.contentWindow?.location.reload() with a setRefreshKey fallback in a try/catch. Original behavior for every existing caller (timeline editing, manifest persistence, dom edit) is restored. The commit message also calls out PR #895 as the right home for the refresh-key path. Good split.
• Important #3 — cross-tab paste advertised but unused. ADDRESSED. Both copyTextToClipboard(serializeClipboardPayload(payload)) calls in useClipboard.ts are removed (and serializeClipboardPayload import dropped). In-memory ref is now the only read+write path — no false advertising.
• Important #4 — hand-rolled HTML parser fragility. OPEN, deferred. findClosingTagPosition in clipboardPayload.ts:108-152 is unchanged. The input domain is studio-generated outerHTML (not arbitrary HTML), so the practical risk is bounded; happy to land it as a follow-up rather than gate this PR.

Fresh observations

• nit: handleCut now returns Promise<boolean> (useClipboard.ts:207-225), but the caller useAppHotkeys.ts:252 still does void handleCutRef.current() — the return value is unused. The new hasSelection pre-check at the call site is what actually gates preventDefault(), so the boolean is structurally redundant. Not blocking; you may prefer to keep the symmetry with handleCopy, or drop the return type back to Promise<void> next time you're in this file.
• nit: in handleCut, both await handleTimelineElementDelete(...) and await handleDomEditElementDelete(...) branches end with return true, and there's a final return true after them. Compresses to: copy succeeded → run delete → return true. Tiny readability win available, not blocking.

Verdict: APPROVE
Reasoning: The Cmd+X blocker is resolved with the right gating pattern, the three highest-value importants from round 2 are addressed cleanly, and the one remaining important (hand-rolled HTML parser) is a constrained-input follow-up rather than a release blocker. CI on the required jobs is green; the few pending windows checks aren't blockers.

Review by Vai (re-review round 3)