feat(studio): Timing inspector + fix mixed-content text editing

[miguel-heygen]

Summary

• Timing section: Start, End, Duration fields in the Design panel for elements with data-start/data-duration
• Mixed-content text fix: collectDomEditTextFields now captures bare text nodes alongside child elements, preventing text loss when editing styles on mixed-content elements like If you are <span>turning 65</span> soon...

Test plan

• Build passes, 42 domEditing tests pass, lint clean
• Manual: change font on headline with mixed content — full text preserved
• Manual: Timing section appears for timed elements, edits persist

[miguel-heygen]

• feat(studio): Timing inspector + fix mixed-content text editing #896 👈 (View in Graphite)
• 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
• main

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

[jrusso1020]

LGTM — Timing inspector is clean. Mixed-content text fix is the substantive piece and the shape is right.

Audited

Mixed-content text fix (collectDomEditTextFields)

The bug shape: an element like <h1>If you are turning 65 soon, <strong>read this</strong>.</h1> previously had its bare text nodes ("If you are turning 65 soon, " and ".") silently dropped because collectDomEditTextFields only iterated el.children (Element children), not el.childNodes (all node types including text). On style edit + serialize-back, only <strong>read this</strong> survived → text loss.

Fix:

const hasMixedContent = Array.from(el.childNodes).some(
  (node) => node.nodeType === 3 && node.textContent?.trim(),
);

if (hasMixedContent) {
  const fields: DomEditTextField[] = [];
  let childIdx = 0;
  for (const node of el.childNodes) {
    if (node.nodeType === 3) {
      // text-node field
    } else if (isHtmlElement(node) && isEditableTextLeaf(node)) {
      // child field
    }
  }
  return fields;
}

Mixed-content detection is correct (nodeType === 3 = text node; trim check filters whitespace-only nodes). Interleaved traversal preserves DOM order. New source: "text-node" field type plumbed through domEditingTypes.ts and serialized via escapeHtmlText(field.value) (no wrapping tag). ✓

childIdx++ is used in both branches and as the key suffix, so text + element children both get unique sequential indices. ✓

Timing inspector (TimingSection)

Renders Start / End / Duration MetricFields conditionally on element.dataAttributes.start != null. Three commit handlers:

• commitStart — parse seconds, write data-start
• commitDuration — parse seconds, guard parsed <= 0 rejects, write data-duration
• commitEnd — parse seconds, guard parsed <= start rejects, compute duration = end - start and write data-duration

Cross-field correctness: the End field is derived (end = start + duration) but commits as duration = end - start. So editing End updates Duration in the source, then on re-render Start stays put and End reflects the new Duration. ✓

handleDomAttributeCommit

Reuses the same PatchOperation { type: "attribute" } infrastructure that useDomEditTextCommits already plumbs through persistDomEditOperations. Live preview update via direct el.setAttribute("data-${attr}", value) (correct — the patch system updates source-of-truth on disk, the direct setAttribute keeps the preview in sync without waiting for a reload). ✓

Non-blocking notes

1. Whitespace-only text nodes dropped on mixed-content: the if (!text.trim()) continue; skip means a \n between block elements is silently lost on round-trip. For most elements this is fine (whitespace collapses in normal HTML rendering), but for <pre>, <code>, or any element with white-space: pre / pre-wrap, the whitespace IS visually significant. Worth either gating the skip on the element's computed white-space value, or noting the limitation in a comment.

2. handleDomAttributeCommit label hardcoded to "Edit timing" — the handler is generic ((attr, value) => ...) but the history label assumes the only caller is the Timing section. If this gets reused for other attribute editors (visibility, role, aria-label, etc.), undo entries will be mis-labeled. Worth either threading a label parameter through or naming the function handleDomTimingAttributeCommit to match its actual scope.

3. Round-trip identity on non-mixed-content elements — the new hasMixedContent branch only runs when there's bare text between children. Pure-child-elements case (existing path) is byte-identical to before, no regression risk. ✓

CI

mergeable_state: unstable — top of the stack, depends on hf#895 + hf#894 passing CI first. Standard.

— Review by Rames Jusso (pr-review)

[vanceingalls]

Two-in-one PR: a Timing inspector section and a mixed-content text-edit fix. The mixed-content fix is structurally correct and reuses existing infrastructure. The Timing inspector has some UX rough edges worth surfacing.

Calibrated strengths

• domEditingLayers.ts:79-106 — preserving source-order interleaving via a single childIdx counter that increments for both text-node and child-element pushes is exactly right. Round-trip via serializeDomEditTextFields (line 131-148) keeps the [text, span, text] order, which is what makes If you are <span>turning 65</span> soon survive a font change.
• useDomEditTextCommits.ts:116-146 — handleDomAttributeCommit correctly reuses the existing { type: "attribute" } PatchOperation. sourcePatcher.ts:285,307 auto-prefixes data- so passing the bare "start"/"duration" is consistent with how useTimelineEditing.ts:115-122,197-205 already drives the same patch type. No new patch path was needed — good.
• domEditingTypes.ts:68 — widening the discriminant to "self" | "child" | "text-node" rather than overloading "child" keeps the type system honest. The new branch in serializeDomEditTextFields (field.source === "text-node") won't silently regress if a future field shape gets added.

Findings

important — no test covers the mixed-content fix. The PR body cites "42 domEditing tests pass" but adds zero tests for the new text-node path. The fix's whole point is that collectDomEditTextFields + serializeDomEditTextFields now round-trip <h1>If you are <span>turning 65</span> soon</h1> losslessly. domEditing.test.ts:1034-1063 already has the exact pattern for testing serialization — adding a "collects + serializes mixed text/element content" case would take ~15 lines and would lock in the regression you just fixed. As-is, a future refactor of collectDomEditTextFields has no signal that mixed-content is load-bearing. Why: this is the most likely failure mode to come back, and the test gap is the cheapest finding in the PR to close.

important — Timing scrub is effectively broken. propertyPanelPrimitives.tsx:128-136 commits String(Math.round(state.startValue + delta)) where delta is raw pixels. That UX works for px-based metric fields (1px scrub → 1px value) but for seconds it means: (a) 1px of cursor motion = 1 full second, (b) Math.round discards all sub-second precision, (c) so scrubbing a 2.50s clip even one pixel snaps it to 3.00s. Combined with commitEnd's parsed <= start rejection (PropertyPanel.tsx:158), scrubbing the End field left of Start silently drops every event. Either pass scrub={false} on these fields, or thread a per-field scrub multiplier (e.g., 0.05s per px) through MetricField. The text-input commit path looks fine — that's probably the one you validated in "edits persist."

important — silent rejection of valid edits. PropertyPanel.tsx:146,152,158 all return early without any user-visible signal:

• commitStart rejects parsed == null (e.g., typed "abc").
• commitDuration rejects parsed <= 0 (user wants to zero a clip → nothing happens).
• commitEnd rejects parsed <= start (user wants to set End before Start → nothing happens).

Either clamp to the closest valid value, or surface the rejection (revert the displayed value, flash the field). Right now the field accepts the keystroke and shows the typed value momentarily, but the next render snaps it back without explanation. Failure mode: user assumes the field is broken.

nit — whitespace-only text nodes dropped on round-trip. domEditingLayers.ts:89 (if (!text.trim()) continue;) skips them. <span>A</span>\n <span>B</span> collapses the indentation on save. Doesn't affect runtime rendering, but creates diff churn on the source file every time a user opens + commits an edit. Consider preserving whitespace-only text nodes too — they round-trip safely via escapeHtmlText.

nit — parseTimingValue regex doesn't match all human inputs. /s$/i matches "1.5s" but not "1.5 s" (space). parseFloat rescues the parse so it ends up working, but the cleanup is misleading — drop the regex (since parseFloat is forgiving anyway) or extend to /\s*s$/i.

nit — dataAttributes.start != null is a loose gate. Any element with a data-start attribute renders the Timing section, even without data-duration. Then end = start + 0 = start, and every End-field commit rejects (see above). Either require both data-start and data-duration to render, or handle the "duration unset" case explicitly.

nit — 2dp precision may be too coarse. .toFixed(2) everywhere caps timing precision at 10ms. At 60fps a frame is 16.6ms, so frame-accurate timing isn't expressible. Matches formatTimelineAttributeNumber (probably — worth verifying), so fine for consistency, but flag if frame-accuracy ever matters here.

Verdict: COMMENT
Reasoning: The mixed-content fix is correct and the attribute-commit plumbing is clean. The Timing inspector ships a confusing UX (scrub semantics + silent rejection) plus a test gap on the fix you actually shipped. None are merge-blocking, but please file follow-ups (or address inline) before this stack lands.

Review by Vai

[jrusso1020]

Self-correction acknowledging Vai's three importants — my APPROVE underrated the UX surface of the Timing inspector.

What I missed

Important 1 — zero tests for the mixed-content fix

I closed my review with "Round-trip identity on non-mixed-content elements... no regression risk" — which is true, but I didn't check whether the new mixed-content path has tests. Vai's right: the body claims "42 domEditing tests pass" but none of those exercise the source: "text-node" field type or the interleaved childNodes traversal. The bug I just complimented as "the substantive piece" has no regression lock-in. This is exactly the bug-class most likely to come back next time someone refactors collectDomEditTextFields — the original failure was silent, and the new path can silently regress to the same shape.

A test fixture like:

<h1>Text before <strong>middle</strong> text after</h1>

fed through collectDomEditTextFields should return three fields in order: text-node / child / text-node. And serializeDomEditTextFields(fields) should round-trip back to the original (modulo whitespace normalization). Pure-function, easy to pin.

Important 2 — MetricField scrub effectively broken for timing units

I audited the commit handlers' math (commitStart, commitDuration, commitEnd) and the cross-field correctness without auditing the scrub control's actual interaction semantics. Vai's diagnosis is sharp: MetricField scrub commits Math.round(startValue + delta_px). For pixel fields (offset-x, width, height), 1px-per-drag-pixel is correct. For seconds fields, that means a user can only land on whole-second increments — can't scrub to 0.5s, can't fine-tune to frame-accuracy. The scrub UI is technically there but practically unusable for timing.

Right shape would be either a scaling factor in the commit (e.g., delta_px * 0.01 for 10ms-per-pixel) or a different scrub control for sub-unit fields. Worth landing before this ships to users who'll discover the scrub is broken.

Important 3 — silent no-op on invalid inputs

I noted the guards approvingly:

"User sets End < Start → guard parsed <= start rejects → no-op. ✓"
"User sets Duration to 0 → guard parsed <= 0 rejects. ✓"

But "no-op" means the field reverts to its previous value with NO user feedback explaining why their input was rejected. Vai's reframing is correct: the user types "5" for End when Start is 10 → field flickers back to the old value → user has no idea their input was out of range. Either a toast / inline validation message, or clamping to the valid range (e.g., End = max(Start + minDuration, parsed)) would close that gap.

Vai's nits (also agreeing)

• Whitespace-only text-node drop: convergent with my note. Same fix shape — gate the skip on the element's computed white-space value.
• dataAttributes.start != null without checking data-duration: I didn't flag this. If an element has data-start but no data-duration, the Timing inspector renders with duration = 0 and end = start, and the user can edit those fields but won't be editing real data. Either gate on start != null && duration != null, or treat absent duration as "edit creates it."
• .toFixed(2) precision: 33ms at 30fps, 17ms at 60fps. 2dp = 10ms granularity, which is one third of a frame at 30fps. Not frame-accurate. For an inspector that targets frame-accurate authoring, .toFixed(3) or .toFixed(4) would be more honest. Cosmetic but the kind of thing video editors notice.

Calibration

My APPROVE was too soft on the UX surface. The structural correctness of the mixed-content fix and the patch-system integration are clean (those parts I audited correctly), but the user-facing Timing inspector ships with three sharp UX problems: broken scrub, silent rejections, no regression test on the fix. Severity is "important non-blocking" (matches Vai's COMMENT) rather than "blocker," but my own review should have caught them. Treating my APPROVE as informational alongside Vai's COMMENT — happy to flip to a stronger verdict once the test gap + scrub precision land.

Lesson for next time: when a PR adds a new use of an existing UI control (like MetricField) in a new domain (seconds instead of pixels), audit the control's interaction semantics in the new domain, not just the commit handler's math.

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

[vanceingalls]

Re-reviewing at HEAD 581e17e8. PR was rebased onto a newer base after my prior review, but none of the three importants I flagged have been addressed and no new tests landed for the mixed-content fix. The PR is currently APPROVED by jrusso1020 and Rames's follow-up self-correction agrees with the three importants — flagging this re-review as informational rather than a merge gate.

Status of prior findings

important #1 — no test for text-node mixed-content round-trip — OPEN.
packages/studio/src/components/editor/domEditing.test.ts:1034-1063 still has only one serializeDomEditTextFields test, exercising two child-source fields. No test fixture covers text before <strong>middle</strong> text after round-tripping through collectDomEditTextFields → serializeDomEditTextFields with the interleaved text-node / child / text-node shape. The regression this PR fixes is unpinned.

important #2 — MetricField scrub effectively broken for timing — OPEN.
PropertyPanel.tsx:181-183 still passes scrub on all three Timing fields (Start, End, Duration) with no per-field multiplier. propertyPanelPrimitives.tsx:128-136 still commits String(Math.round(state.startValue + delta)) where delta is raw pixels — so a 1px scrub mutates a seconds value by 1.0s and Math.round discards the sub-second component. A user cannot scrub a 2.50s clip without it snapping to 3.00s. Either drop scrub on these fields or thread a scrubScale prop into MetricField.

important #3 — silent rejection of invalid inputs — OPEN.
PropertyPanel.tsx:144-156 still has the three guard returns with no user-visible signal:

• commitStart rejects parsed == null silently (line 145).
• commitDuration rejects parsed == null || parsed <= 0 silently (line 150).
• commitEnd rejects parsed == null || parsed <= start silently (line 155).

Failure mode unchanged: user types an out-of-range value, field flickers, next render snaps back without explanation.

Status of prior nits

• Whitespace-only text-node drop — OPEN. domEditingLayers.ts:89 if (!text.trim()) continue; unchanged.
• dataAttributes.start != null gate without data-duration — OPEN. PropertyPanel.tsx:389 unchanged.
• parseTimingValue regex /s$/i doesn't match "1.5 s" — OPEN. PropertyPanel.tsx:128 unchanged.
• 2dp precision (.toFixed(2)) below frame granularity — OPEN.

New findings

None. The rebase brought in unrelated fixes (deduplicateIds regex, native-copy hotkey guard, Player perf iframe reload) that look reasonable but are outside the surface I was re-checking.

Verdict

Verdict: COMMENT
Reasoning: Three importants and four nits from the prior review are all still open at HEAD. None are merge-blocking on their own, but the test gap on the load-bearing mixed-content fix and the scrub-snap-to-1s UX are worth closing in this PR or an immediate follow-up before this surface ships to users. PR is already team-approved, so this is informational — please file follow-ups if not addressing inline.

Review by Vai (re-review)

[jrusso1020]

Approving on 482abd3d. Two of three importants addressed; the remaining one is a UX papercut suitable for a follow-up.

Audited

Important 1 (mixed-content test gap) — fixed via new domEditingTextFields.test.ts:

Two tests added:

1. round-trips text-node + child element fields — pins the exact interleaved output: text → <span data-hf-text-key=... style=...> → text. Asserts order preservation, attribute carrying, data-hf-text-key insertion, and inline-style serialization. This is the regression lock-in the original fix was missing.
2. escapes HTML entities in text-node values — <, &, > correctly escaped to <, &, >.

Comprehensive — both the structural correctness and the safety-against-content-injection cases are pinned. ✓

Important 2 (scrub broken for timing) — fixed by removing scrub from all three Timing MetricFields:

- <MetricField label="Start" value={...} scrub onCommit={...} />
+ <MetricField label="Start" value={...} onCommit={...} />

The 1px=1sec problem is sidestepped entirely. Trade-off: users can only commit values by typing now, not by dragging — pragmatic given the existing MetricField doesn't support sub-unit scrub-delta scaling, and shipping a broken scrub interaction was worse than shipping no scrub for these fields. If you wanted to add fine-grained scrub later, threading a scrubDelta prop through MetricField (e.g., scrubDelta={0.01} for 10ms-per-pixel) would be the path. ✓

Important 3 (silent rejection feedback gap) — still open

commitStart / commitDuration / commitEnd still silently return on invalid values. No toast, no inline validation message, no field reset to last-good. User types invalid input → field flickers back → no idea why. This is the lightest of the three importants — UX papercut without functional impact — but worth a follow-up commit when you're back in this code. Either a toast on invalid commit, or auto-clamp to the valid range (e.g., commitEnd could clamp parsed = max(parsed, start + minDuration) and commit the clamp), or an inline error message.

CI

mergeable_state: unstable — top of the stack, depends on the rebased base. Standard.

Acceptable to ship with the silent-rejection note open. Flipping from COMMENT to APPROVE.

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

[vanceingalls]

Round-3 re-review at HEAD 482abd3. Flipping COMMENT → APPROVE — Miguel addressed the two substantive importants from prior round cleanly:

Status of prior 3 importants

• Imp #1 (no test for mixed-content fix) — ADDRESSED. New packages/studio/src/components/editor/domEditingTextFields.test.ts:5-60 covers interleaved text-node / child / text-node round-trip with attribute preservation + entity escaping. Pins the regression class.
• Imp #2 (MetricField scrub broken for seconds — 1px = 1s, no sub-second precision) — ADDRESSED. PropertyPanel.tsx:160-172 removes the scrub prop from the Start / End / Duration MetricFields. The shared Math.round primitive is untouched (so other call sites of MetricField that ARE integer-valued still work) — clean sidestep. Trade-off: scrub-to-adjust is no longer available for timing, but the alternative was a broken UX.
• Imp #3 (silent no-op on invalid commits) — STILL OPEN. PropertyPanel.tsx:144-156 still has bare return in all three commit handlers. Worth a follow-up (toast / inline error / clamp-with-feedback) but UX papercut, not blocking.

Notes

• The nits from prior rounds (whitespace text-node round-trip, start != null gate without data-duration, .toFixed(2) precision) weren't directly addressed — same follow-up territory as Imp #3.
• The "PR body claims 42 tests pass" sub-finding from round 1 is closed: round-3 added the actual coverage.

Verdict: APPROVE.
Reasoning: Two real issues addressed cleanly; one UX papercut remains, suitable as a follow-up. No new regressions in the round-3 commits.

Review by Vai (re-review round 3)

The base branch was changed.