feat(studio): flat inspector foundation + Text group#2120
feat(studio): flat inspector foundation + Text group#2120vanceingalls wants to merge 12 commits into
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miga-heygen
left a comment
There was a problem hiding this comment.
Full Stack Review — Edit Panel Redesign (15 PRs, ~10,700 additions)
Reviewed the entire stack in three tiers: foundation (#2120–#2122), domain groups + retirement (#2123–#2127), and UX polish (#2128–#2190). Per-PR verdicts below; no blockers across the stack.
Tier 1: Foundation (#2120–#2122)
#2120 — flat inspector foundation + Text group — Clean. Introduces the shell (header, footer, accordion, value-tier system, primitives) behind STUDIO_FLAT_INSPECTOR_ENABLED. resolveValueTier is a well-designed single decision point. hideOwnHeading flag on TextSection prevents doubled headings without duplication.
#2121 — Style group — Clean. Each sub-block (Fill, Stroke, Radius, Shadow, Blend, Blur, Overflow, Mask, Opacity) owns exactly one CSS domain. Helpers come from propertyPanelHelpers.ts (single source). FlatRadiusRow reuses legacy BorderRadiusEditor for non-uniform corners — good reuse.
#2122 — Layout + Motion groups — Clean. The standout SSOT contribution of the stack: deriveElementTiming extracts the "how do we compute start/duration?" decision into one function consumed by both Layout's keyframe gutter and Motion's timing row. Fixes a real bug where Layout used elDuration ?? 1 while Motion inferred from tweens.
Tier 2: Domain Groups + Retirement (#2123–#2127)
#2123 — Media group — Clean. Legacy MediaSection fully replaced, not duplicated. FlatToggle is a genuine new primitive.
#2124 — Grade (color grading) — Nit. The useColorGradingController extraction is the strongest SSOT win in the stack — turns a 200-line inline state machine into a shared hook. During the hybrid window both legacy and flat sections instantiate the hook (two debounce timers, two polling cascades). Resolved by #2126.
#2125 — persisted pinning + multi-field Text — Clean. FlatGroupDescriptor[] data model eliminates per-group copy-paste. usePersistedPinnedGroups is a clean single-path persistence hook.
#2126 — retire legacy Style and Grade — Clean. Pure SSOT cleanup — removes the dual-rendering transition state. The hideFlex escape hatch is gone.
#2127 — parity gaps from legacy-retirement audit — Clean. visibleIntensity helper centralizes the "revive from 0% strength" decision. Radius path simplified by always delegating to BorderRadiusEditor. Stroke width clamping reuses existing normalizePanelPxValue. Keyboard accessibility for compare hold button is thorough.
Tier 3: UX Polish (#2128–#2190)
#2128 — sticky accordion headers — Nit. Dual sticky anchoring (top-0 bottom-0) is unconventional but intentional per the design.
#2134 — scroll refactor — Clean. Well-structured three-region layout (before-open, scrollable body, after-open). FlatGroup → FlatGroupHeader rename correctly reflects narrowed responsibility.
#2135 — remove section pinning — Clean. Complete disciplined removal — no orphaned props, no dead code.
#2136 — animate expand/collapse — Clean. justToggledIds mechanism handles the Chromium re-trigger edge case thoughtfully. Respects prefers-reduced-motion. Well-commented.
#2142 — widen slider hit area — Clean. 2px → 20px invisible hit area. Minimal, correct fix.
#2186 — slider drag support — Clean. Pointer capture API is the correct modern approach. Properly layered before the debounce PR.
#2190 — debounce slider drag commits — Clean. Draft/debounce pattern gives instant visual feedback while debouncing commits at 40ms. Flush-on-release ensures final value always commits immediately. Broad test updates across sibling sections show thoroughness.
Cross-Stack Architecture
The stack follows a disciplined pattern:
- Flag-gated parallel path — flat inspector runs alongside legacy, gated by
STUDIO_FLAT_INSPECTOR_ENABLED. No legacy behavior changes. - Suppression flags over removal — when a flat group absorbs a legacy section, the legacy gets
hideOwnHeading/hideFlexrather than being removed, enabling safe coexistence. Retired cleanly in #2126. - Helpers carry new knowledge; primitives carry new rendering — every helper justifies itself with a fact the caller doesn't know. No trivial wrappers.
- Self-correcting SSOT — #2122 identifies and fixes the timing-basis disagreement that would have been an SSOT violation across Layout and Motion. The stack improves as it progresses.
CI: 0 failures across all 15 PRs. Ship the stack.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Blocker on current head 444639d: GitHub reports this PR as CONFLICTING / mergeState DIRTY. Resolve the branch against its stack/base and rerun the required checks before approval. The code review findings are non-blocking, but this head is not mergeable as submitted.
444639d to
6e9d99e
Compare
|
Rebased the stack base onto latest |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review after rebase: current head 6e9d99ef8f3aec054aebd05c9b2201f15aec4665 is mergeable, required checks have no failures/pending checks, and prior conflict blocker is resolved. No unresolved review threads; foundation/value-tier and cross-stack contracts remain clean.
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
6e9d99e to
535a072
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 535a072917cafb6d3940b914b175faf5b2602814. The foundation still has direct correctness blockers:
StudioRightPanel.tsx:346-349passeselement.id ?? element.selectorto a lookup whose canonical timeline identity includes source scope and selector occurrence. “Hide all” can silently miss its targets. The same loop starts whole-file read/modify/write operations concurrently, so same-file selections can clobber one another.PropertyPanel.tsx:265-273andPropertyPanelEmptyState.tsx:104-109omit source,hfId, andselectorIndexfrom React identity, allowing repeated-selector siblings to reuse stale panel state and duplicate list keys.- Flat Text still bypasses the legacy
PromotableControlpath for bound content/color/font fields. - Current CI is not green: File size check reports
StudioRightPanel.tsxat 603 lines (limit 600).
Use the canonical DOM/timeline target identity everywhere, batch/serialize the hide mutation into one save, restore variable-binding parity, and add repeated-selector/same-file regression tests.
Verdict: REQUEST CHANGES
Reasoning: Core element identity is noncanonical and Hide All can either no-op or lose updates; the head also has a required CI failure.
— Deepwork
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head . The earlier identity, Hide All, variable-parity, and file-size blockers remain unchanged.\n\nStrength: keeps each choice as a native button, so basic focus/click behavior is preserved ().\n\n**[important] Glyph-only segmented choices have no accessible name or selected-state semantics.** exposes only , , and , but the button renders the glyph without , , or radio semantics (). The Text consumers discard their declared alignment labels and render , , plus two visually identical buttons for normal vs italic (). A screen reader cannot identify the choices or announce which one is selected. Add an accessible option label and pressed/radiogroup semantics, with a regression for the two choices.\n\nVerdict: REQUEST CHANGES\nReasoning: The current head is byte-identical to the prior requested-changes head and still contains those blockers; this pass adds an accessibility contract gap in the shared segmented primitive.\n\n— Deepwork
Formatting was corrupted during submission; replacing with corrected review.
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head 535a072917cafb6d3940b914b175faf5b2602814. The earlier identity, Hide All, variable-parity, and file-size blockers remain unchanged.
Strength: FlatSegmentedRow keeps each choice as a native button, preserving basic focus behavior (propertyPanelFlatPrimitives.tsx:108-129).
[important] Glyph-only segmented choices lack accessible names and selected-state semantics. FlatSegmentOption exposes only key, node, and active, while the button has no aria-label, aria-pressed, or radio semantics (propertyPanelFlatPrimitives.tsx:87-129). Text renders L/C/R/J, AG/ag/Ag, and two indistinguishable A buttons for normal versus italic (propertyPanelFlatTextSection.tsx:27-39,151-181). Add accessible option labels and pressed or radiogroup semantics, with a regression for the two A choices.
Verdict: REQUEST CHANGES
Reasoning: This head is byte-identical to the prior requested-changes head and still contains those blockers; this pass adds an accessibility contract gap in the shared segmented primitive.
— Deepwork
…ld, TextAreaField
…AT_INSPECTOR_ENABLED Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eading in multi-field fallback The flat inspector's Text FlatGroup rendered unconditionally, showing an empty "Text" header for non-text elements (image, video, etc). Gate it on isTextEditableSelection(element) so it disappears entirely when there's no text to edit. Also, the legacy multi-field TextSection (used as a fallback when an element has 2+ text fields) rendered its own internal "Text" heading nested inside the new flat Text FlatGroup, producing a doubled "Text" heading. Add a hideOwnHeading prop to TextSection (default false, so its other — legacy, non-flat — call site is unaffected) and pass it from FlatTextSection's fallback path.
535a072 to
7b5d8c7
Compare
|
Status on this review's findings, checked against the current stack tip: Fixed here — PR #2416, commit
Already fixed downstream, in PR #2225 (stacked above this PR, so it doesn't show at this PR's own head):
The file-size note is presumably about this PR's own diff size — no action taken there since it's outside the scope of the adversarial-review findings. Re-review whenever convenient — nothing outstanding from this pass that isn't already addressed somewhere in the stack. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 7b5d8c7d44264ea4345763d1af69a028fb14a1e1.
git range-diff marks all 12 incremental commits patch-equivalent to the previously reviewed head, so none of the requested fixes landed in this force-push. The existing identity/duplicate-selector, variable-promotion, Hide All lost-update, and accessibility findings therefore remain. In particular, StudioRightPanel.tsx:346-349 still launches independent read-modify-write visibility toggles, and propertyPanelFlatPrimitives.tsx:108-130 still gives glyph-only segmented buttons neither accessible option names nor selected-state semantics.
Fresh adversarial pass also found that FlatSelectRow disables only its <select> while leaving the reset button active (propertyPanelFlatPrimitives.tsx:423-454), so disabled Style rows can still mutate through their reset callbacks. CI is red: StudioRightPanel.tsx is 603 lines (limit 600), and Windows tests fail in the browser install-lock concurrency test.
Verdict: REQUEST CHANGES
Reasoning: This is a mechanical rebase, not a fix round; the prior correctness blockers and two required CI failures remain at the exact current head.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |

What
First PR in a 6-PR stack migrating Studio's right-panel property inspector from an always-expanded stacked-sections layout to a "flat" one-open-at-a-time accordion. This PR lays the foundation: the
STUDIO_FLAT_INSPECTOR_ENABLEDfeature flag, the accordion primitives (FlatRow,FlatSegmentedRow,FlatGroup,PinnedZoneDivider), the flat identity header/footer, and the first migrated group — Text.Stack: #2120 (this) → #2121 (Style) → #2122 (Layout+Motion) → #2123 (Media) → #2124 (Grade) → #2125 (Pinning + multi-field Text).
Why
The legacy inspector renders every applicable section expanded at once, which gets unwieldy as an element accumulates properties across style/layout/motion/media/grade. The flat redesign shows one section at a time (plus pinned sections), matching a design handoff mock.
How
FlatGroupowns the one-open accordion state (openGroupId/onToggleOpen) and pin affordance (onTogglePin), styled per the design mock.FlatTextSectionis the first migrated group and the reference implementation every later group's task followed for theisOpen/onToggleOpen/onTogglePin/summarywiring pattern.FlatGroupwas rendering unconditionally regardless of element type (empty for non-text elements), and the multi-field fallback doubled the "Text" heading. Fixed by gating onisTextEditableSelectionand adding ahideOwnHeadingprop to the legacyTextSectionfallback.STUDIO_FLAT_INSPECTOR_ENABLED(default off) — the legacy panel is untouched and remains the default for all users.Test plan
oxlint/oxfmtclean; this repo'sfallowcomplexity/duplication gate passes.