fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel - #1965
fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel#1965vanceingalls wants to merge 1 commit into
Conversation
c35d8ee to
7e3235d
Compare
cb5c524 to
2675a4f
Compare
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
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
2675a4f to
d89e8e4
Compare
7e3235d to
8f0c6f3
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
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
d89e8e4 to
c601125
Compare
90b25e2 to
b89c8ac
Compare
|
@miguel-heygen The block-param write path is reworked at head
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 |
1823e47 to
db3f931
Compare
b89c8ac to
1fad64d
Compare
|
Rebased onto main after #1964 merged (new head
Rebase also reconciled two editor files against main's newer versions: color-grading (kept main's collapsible LUT UI + grafted in the import spinner/error) and media-section (kept main's Cutout block + re-applied the slider aria-labels). tsc clean, 358 editor tests pass. @miguel-heygen re-requesting review — the write-path blocker should be resolved. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at c0ef6b8 (post-fallow-audit rewrite).
Layering on top of Miga's Approve-with-nits and Magi's CHANGES_REQUESTED at 8f0c6f3. Miga's structural read is right; Magi's blocker at their SHA was real and load-bearing. This review has two jobs: (a) verify Magi's blocker at current HEAD, (b) second-pass lens on top.
🔴 Magi's blocker: MITIGATED at HEAD, but the core hazard survives on first commit
Between Magi's review (8f0c6f3) and current HEAD (c0ef6b8b), Vance replaced content.split(previous).join(nextValue) with a token-boundary regex + occurrence-count invariance guard (BlockParamsPanel.tsx:53-75). Two real improvements land:
- Token boundaries:
(?<![-\w#])<previous>(?![-\w])correctly prevents#0c0c0cfrom clobbering the first six chars of#0c0c0cff(verified against the new test atBlockParamsPanel.test.tsx:99-112). - Divergence guard: if the file's occurrence count of
previouschanges between commits (e.g., something external appeared with the same literal), the second write is refused with a clear error.
But Magi's original data-corruption scenario is not fully closed. The guard captures expected = matches on the FIRST commit and doesn't fire until matches !== expected on a LATER commit. On the first commit for a given param, ALL matching occurrences are unconditionally replaced. BlockParamsPanel.tsx:67 (const expected = (expectedCountRef.current[key] ??= matches);) is the tell — first-call sets expected to whatever count exists, then matches !== expected is false → mass write proceeds.
Concrete failure scenario (independent of Magi's phrasing):
- Composition file contains Block A (
<circle fill="#FFFFFF" ...>) AND some unrelated adjacent element<rect fill="#FFFFFF" ...>(either a sibling block, a design comment, or a header decoration). - Block A's registered
--fill-colordefault is#FFFFFF. - User opens Block A's params panel, changes fill to
#000000. - First commit:
matches = 2,expectedinitialized to2,matches !== expectedis false →content.replace(matcher, "#000000")rewrites BOTH the circle AND the rect. Silent unrelated mutation. This is exactly Magi's failure mode. - Subsequent edits then ride the mass-replacement (
expectedis now2, matches stays2because both moved together), so the panel keeps silently mass-writing until an external hand-edit causes divergence.
The new test at BlockParamsPanel.test.tsx:99-112 actually bakes this behavior in as intentional (.a { background: #0c0c0c; } .b { border-color: #0c0c0c; } — both replaced on first commit, described as "both belong to the param"). But the panel has no source-range or block-instance metadata to distinguish "belongs to this param" from "coincidentally the same literal in adjacent content" — the two are indistinguishable at the string level. The PR body's own line acknowledges this: "registry params are metadata-only defaults; block HTML hardcodes literal values, so substituting the previously-applied value is the correct apply mechanism" — this is the right diagnosis and the wrong fix.
Extended fix guidance (Magi's "targeted metadata or ambiguity guard + test" is directionally right; here's the concrete shape):
- Minimum viable: refuse first-commit writes when
matches > 1, surface"'${previous}' appears N× in ${compositionPath} — edit the file directly to disambiguate". Legitimate cases where the same literal is used twice within one block (the test'sbackground+border-colorcase) become one-time manual edits, which is annoying but not data-corrupting. Cost: 3 lines of code + one test flip. - Ideal: persist a
data-param-<key>="<value>"attribute on the block's root element at install time; the panel writes into the specific element's attribute, not the file's global text. This is the "real target contract" Magi named. Bigger refactor, right long-term shape. - Middle ground: confirm-on-first-write UX ("found N places matching
#FFFFFF— replace all?"). Better than silent, cheaper than a metadata rewrite.
The current guard is a correct safety net for FUTURE drift; it does not protect against the initial multi-occurrence write that Magi flagged. I'd hold at 🔴 until the first-commit ambiguity is addressed.
🟠 No telemetry on the new error surface
commitState.tone === "error" (BlockParamsPanel.tsx:185-189) surfaces failures to the user but doesn't emit to packages/studio/src/telemetry/events.ts. If the ambiguity guard fires in production, or if writeProjectFile fails, ops has no way to dashboard the rate. This whole panel is a first-of-its-kind write surface (literal substitution against composition files) — the "how often does this actually work" question has no answer. Suggest a trackBlockParamCommit({ tone, path, key }) on each state transition, especially the error tones.
🟡 blockName is declared in BlockParamsPanelProps (line 7) but not destructured or used
Dead prop in the interface. Either destructure and use it (e.g., in an ARIA label), or remove from the interface. Not a blocker.
🟢 Confirmed good
escapedRefpattern inpropertyPanelPrimitives.tsx:20-107is applied consistently onCommitField. The range-slideronBlurat line 295 doesn't take Escape, which is correct — sliders are mouse-first, keyboard revert isn't the expected model.- Per-param debounce timers + serialized commit chain (
BlockParamsPanel.tsx:44,88-96) correctly preempts two race classes Magi didn't call out (cross-param cancel + concurrent read-modify-write). - Flush-on-unmount (
BlockParamsPanel.tsx:117-126) captures the latestvaluesclosure by reassigningflushRef.currentper render — that's the right shape foruseLatest.
Verdict: 🔴 with concrete extended-fix guidance above. Magi's blocker is the right call; the fallow-audit rewrite improves the picture but doesn't close the failure mode named in the review.
— Review by Rames D Jusso
0c01be8 to
f328775
Compare
|
Addressed @james-russo-rames-d-jusso's review at head 🔴 First-commit multi-occurrence write — the guard now closes the initial-write hole, not just future drift. On a param's first commit, if the value matches >1 site, the panel no longer silently mass-replaces — it shows a confirm prompt ("appears N× in this block — Replace all / Cancel") and only writes on explicit accept. Single-match commits stay silent (the safe common case). This is the confirm-on-ambiguity middle ground you named; refusing outright would've broken every block that legitimately uses its color in 2 CSS rules. 🟠 Telemetry — added 🟡 Dead Also rebased onto current main; DomEditOverlay reconciled against main's |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
The first-commit hazard is still not closed. BlockParamsPanel now asks for confirmation when previous matches more than once, but “Replace all” still executes content.replace(matcher, nextValue) across every matching occurrence. The new regression test explicitly clicks Replace all and asserts both sibling literals change, so it blesses the original corruption mode rather than preventing it.
Because the panel has no occurrence metadata, the safe behavior for an ambiguous first commit is to refuse/no-op with an actionable error (or add truly targeted binding metadata). Please add a regression asserting that a duplicate literal does not write/mutate an unrelated sibling; do not make the test approve Replace all as the normal path. CI is green, but this remains a blocker.
f328775 to
1b41329
Compare
|
@james-russo-rames-d-jusso You're right — the confirm flow still executed the mass replace and the test blessed it. Fixed properly at head
Accepted the usability tradeoff you called: a block that legitimately uses one literal in two rules now needs a manual edit. Safe-but-annoying over convenient-but-corrupting. The targeted binding-metadata approach (install-time tsc clean, 395 editor/telemetry tests pass, fallow clean. |
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review at 1b41329d — BlockParamsPanel first-commit ambiguity
Verdict: LGTM 🟢 — the blocker is closed.
What changed since f3287752
The "Replace all" confirmation UX is gone. The panel now refuses any write when the target literal matches more than once (BlockParamsPanel.tsx:93–100):
if (matches > 1) {
setCommitState({
tone: "error",
message: `"${previous}" appears ${matches}× in ${compositionPath} …`,
});
return;
}This is exactly the "minimum viable" fix shape that all three prior reviews (mine, Magi, Rames) converged on — refuse ambiguous first-commits rather than risking silent sibling mutation. Only a unique, single-occurrence literal is safe to rewrite automatically; everything else gets an actionable error directing the user to edit the file by hand.
Test coverage
Four tests cover the critical paths:
| Test | Scenario | Assertion |
|---|---|---|
refuses a multi-occurrence value |
Default #0c0c0c appears in .block AND .unrelated |
writeProjectFile never called, content untouched, "appears 2×" shown |
writes a unique occurrence |
#0c0c0c unique; #0c0c0cff must NOT match |
Single write, hex8 preserved (token boundary) |
refuses once a previously-unique value later collides |
First commit works (unique), second refused when new value matches existing content | Only one write total |
keeps a pending commit for one param |
Two params edited within debounce window | Both params written (per-param timers, not shared) |
The first test is the regression Magi asked for — it proves a duplicate literal does NOT mutate an unrelated sibling. The old test that blessed "Replace all" on coincidental matches is replaced.
Other improvements in this push
- Telemetry:
trackBlockParamCommit({ tone, blockName, key })on every save/error path — addresses Rames' 🟠 about no observability on the new write surface. - Dead prop fixed:
blockNameis now destructured and used (telemetry), closing Rames' 🟡. - Per-param debounce timers:
commitTimersRefis aMap<string, ...>— editing param B no longer cancels param A's pending commit. - Commit chain serialization: concurrent read-modify-write on the same file is prevented.
- Flush on unmount: closing the panel within the debounce window no longer silently drops the last edit.
All CI checks green. No remaining blockers from my side.
Review by Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at current head 1b41329da8a8bfea5db218673600e199b9e1c8a5. Duplicate-literal ambiguity is now refused before mutation, with regression coverage proving sibling nodes remain unchanged. Required checks have no failures/pending checks; no unresolved current threads. Approved.
4 participants
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):
Commit-field safety:
pointercancel/capture loss.Destructive-edit guards:
File tree:
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:
role="switch"; sectionsaria-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.Verification
Stack
PR 4/7 of the studio UX-review fixes.
🤖 Generated with Claude Code