fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel

[vanceingalls]

Summary

Property-panel / editor-component fixes from the studio UX review — headlined by wiring the Block Params panel, whose controls previously edited local state and went nowhere (critical dead surface).

Changes

BlockParamsPanel actually commits (critical):

• Literal-substitution rewrite of the installed block composition file via the file-manager context (registry params are metadata-only defaults; block HTML hardcodes literal values, so substituting the previously-applied value is the correct apply mechanism). Debounced 300ms, not-found guard, saving/saved/error status line, preview reload, disabled state without file access, empty-params state. Known follow-up: not yet in ⌘Z history (recordEdit isn't reachable from this panel).

Commit-field safety:

• Escape-to-cancel on every CommitField (restore + blur); rejected input reverts the draft instead of displaying a value that was never applied; MetricField scrub survives pointercancel/capture loss.

Destructive-edit guards:

• Solid↔Gradient toggle restores the last authored gradient (was: exploration destroyed it). Grading preset apply merges manual adjustments (was: wiped 9 sliders). Failed font import no longer commits the family (was: composition named a font that never loaded). Upload/LUT/font failures show inline errors (were silent unhandled rejections).

File tree:

• Context menu: role="menu", arrow-key nav, focus in/restore. Invalid rename keeps the input open with an inline error (was: silently cancelled the whole operation). Folder delete says "and everything inside it" and anchors at the invoked row (was: generic copy pinned to the panel bottom).

A11y / affordances:

• Gradient stop dots are draggable and no longer insert duplicates when clicked; "Maximum 6 stops" reason shown. Font rows render in their own typeface + ArrowUp/Down/Enter combobox nav. Toggles get role="switch"; sections aria-expanded; sliders labeled; keyframe nav buttons labeled; hit targets expanded to ≥24px (chevrons, diamonds, +file/+folder); resize/rotate handles get hit insets; Transform3DCube is keyboard-rotatable; color picker manages focus; transform-commit jargon reworded to user terms; copy toast fires only after the clipboard promise resolves.
• Ease bezier readout is paste-editable (Enter commits, Escape reverts).
• Empty states for FileTree, GsapAnimationSection, BlockParamsPanel.

Verification

• vitest: 1189 pass / 0 fail (SnapToolbar / DomEditOverlay / KeyframeNavigation suites included); oxlint/oxfmt/tsc clean

Stack

PR 4/7 of the studio UX-review fixes.

🤖 Generated with Claude Code

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(studio): captions UX — mode exit, undo, autosave surfacing, honest gating #1968
• fix(studio): player UX — keyboard timeline, honest waveform, keyframe menu actions #1967
• fix(studio): sidebar/panels UX — asset delete confirm, rename, search trap, undo #1966
• fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel #1965 👈 (View in Graphite)
• fix(studio): shell UX — data-loss guards, error surfacing, dialog contracts, toasts #1964
• feat(studio,studio-server,cli): render cancel end-to-end + renders/nle/storyboard UX #1963
• feat(studio): harden ui primitives — focus rings, keyboard tooltips, dialog hook #1962
• main

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

[miga-heygen]

TITLE: fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel

VERDICT: Approve (comment-only per workflow)

SUMMARY

This is solid, careful work across 27 files that addresses three intertwined concerns — data-loss prevention (commit safety), keyboard accessibility, and wiring BlockParamsPanel to actually persist edits. The PR description accurately reflects what the code does; nothing is hand-waved. CI is green (vitest 1189/0, preflight, preview parity all passing).

PONYTAIL

Approach quality: Strong. The PR correctly identifies that block params were a dead surface (editing local state that went nowhere) and fixes it with literal substitution against the composition file. The commit-field Escape-to-cancel pattern is consistent and well-factored. The a11y improvements (role="menu" + arrow nav on context menus, role="switch" on toggles, aria-expanded on collapsibles, combobox nav on font search) are real, not cosmetic. Gradient stop dots becoming draggable is a good UX win. The lastGradientRef for Solid↔Gradient toggle preservation is a thoughtful touch.

Risk assessment: Low. The biggest moving part — BlockParamsPanel's content.split(previous).join(nextValue) substitution — is inherently greedy (replaces ALL occurrences of the literal, not just the param binding site). The PR acknowledges the recordEdit gap as a follow-up. No new external dependencies. No state shape changes that would affect other PRs in the stack.

FINDINGS

[medium] BlockParamsPanel: greedy literal substitution may over-replace

packages/studio/src/components/editor/BlockParamsPanel.tsx

The substitution content.split(previous).join(nextValue) replaces every occurrence of previous in the entire file, not just the one corresponding to this param. If a block's HTML contains the same literal string in multiple places (e.g. a color #ffffff used as both a param default AND in a comment or another element), the substitution will corrupt unrelated content.

This is partially mitigated by the fact that previous tracks the last applied value (not the original default across all params), so collisions shrink over time as values diverge. But it's a real risk for freshly-installed blocks where multiple params share a common default like #000000 or 100.

Consider: could the registry encode a more specific anchor (line number, attribute path, or a sentinel comment like <!-- hf:param:key -->) so the substitution targets a single binding site? Even a content.replace(previous, nextValue) (first-match-only) would be safer than split/join (all-match).

[low] BlockParamsPanel: debounced commits can race on rapid param switching

If the user edits param A, then within 300ms switches to param B and edits it, the debounce timer for A is cleared (line ~if (commitTimerRef.current) clearTimeout(commitTimerRef.current)). The A change is silently dropped because there's a single commitTimerRef shared across all params.

For a v1 this is acceptable (rapid cross-param editing is rare), but a per-param timer map would prevent the edge case.

[low] BezierReadout: key={bezierText} forces remount on every external change

packages/studio/src/components/editor/EaseCurveSection.tsx

Using key={bezierText} means the BezierReadout component fully remounts whenever the parent's bezier text changes (e.g. picking a preset ease). This resets the internal draft state, which is the intended effect — but it also discards in-flight user edits if an external change arrives while typing. A controlled useEffect sync on bezierText would be gentler, though for this use case the interaction conflict is unlikely.

[nit] Gradient stop dots: role="slider" without tabIndex

packages/studio/src/components/editor/propertyPanelFill.tsx

The gradient stop dots get role="slider" + aria-valuenow/min/max, which is excellent, but the <div> has no tabIndex={0} so it's not keyboard-reachable. A keyboard user can't focus or drag these stops. Consider adding tabIndex={0} and arrow-key handlers to complete the interaction (similar to how Transform3DCube was wired up in this same PR).

[nit] Font combobox: data-font-option-index lookup uses querySelector instead of ref

packages/studio/src/components/editor/propertyPanelFont.tsx

The document.querySelector(\[data-font-option-index="${next}"]`)call for scrolling the active option into view works but bypasses React's ref system. In a portal or multi-instance scenario this could target the wrong element. AlistRefon the scroll container +children[next]` would be more robust.

[positive] Escape-to-cancel on CommitField is well-structured

The escapedRef flag to skip the blur-commit path when Escape triggers blur is a clean pattern that avoids the usual "Escape fires blur which re-commits" footgun. The scheduleResync to re-align the draft after rejected input is also a good catch.

[positive] Context menu focus management

Saving document.activeElement before opening and restoring on cleanup is correct. Arrow-key navigation with Home/End is a complete implementation. The role="menu" + role="menuitem" wiring follows WAI-ARIA menu pattern.

[positive] Error surfacing on previously-silent failures

Font import, LUT import, image upload, and clipboard copy all now show inline errors or toasts instead of swallowing promise rejections. This is a significant UX improvement.

DIFF_STATS

+627 / -145 across 27 files. Heaviest files: BlockParamsPanel (+102/-11), FileTreeNodes (+90/-20), EaseCurveSection (+68/-7), propertyPanelFont (+49/-10), propertyPanelPrimitives (+45/-15), propertyPanelFill (+42/-2).

---

Review by Miga

[miguel-heygen]

Additive review on PR 4/7.

Audited: packages/studio/src/components/editor/BlockParamsPanel.tsx, propertyPanelFill.tsx, FileTreeNodes.tsx, and the editor commit/a11y surfaces at head 8f0c6f3.

The commit-field Escape handling, pointer cancel coverage, and context-menu keyboard work all look directionally good. I’m holding the PR on the block-param write path below.

blocker: packages/studio/src/components/editor/BlockParamsPanel.tsx:61 uses content.split(previous).join(nextValue) to persist a param edit. That replaces every matching literal in the installed composition file, not the selected block param occurrence. The panel only receives compositionPath plus param defaults; it has no block instance id, source range, inserted snippet, or param binding metadata, and no later PR in the stack narrows this. So if a default value like a color/string appears elsewhere in the block file, editing one param silently mutates unrelated CSS/text. That is a data-corruption failure mode, not just polish.

Miga already flagged the over-replace risk; the additional gap is that the current data contract cannot identify the intended occurrence, so the safe fix needs either a real target contract from block installation/metadata or a guard that refuses ambiguous matches before writing. A minimal stopgap would be to require exactly one occurrence of previous and use a single replacement, with a test covering duplicate literals. A better fix is to persist params through explicit placeholders/metadata instead of literal search.

Verdict: REQUEST CHANGES
Reasoning: The PR turns a previously dead editing surface into a write path that can corrupt unrelated content when default literals repeat. It needs targeted/guarded persistence before this stack should merge.

— Magi

[vanceingalls]

@miguel-heygen The block-param write path is reworked at head b89c8ac27 (the split/join was replaced before your review landed; this head also adds the test you asked for):

• Token-boundary matching — #0c0c0c can no longer rewrite part of #0c0c0cff (lookaround regex, escaped literal).
• Occurrence-count guard — the param's site count is captured on its first commit; any later commit whose current value matches a different number of sites (i.e. collides with unrelated content) refuses to write and surfaces an explicit error instead. This is the guarded stopgap you outlined — stricter than exactly-one since multi-site defaults (e.g. vfx-magnetic's 2× --bg-color) are legitimate.
• Per-param debounce + serialized commits — a second param's edit no longer cancels the first's pending commit, and commits are chained so concurrent read-modify-writes can't drop each other.
• New BlockParamsPanel.test.tsx — covers duplicate literals with substring safety, the collision-refusal path, and the cross-param debounce race (3/3 pass).

Agreed the durable fix is real param binding metadata from block installation — left as the follow-up noted in the PR body.

🤖 Generated with Claude Code