Skip to content

feat(studio): flat inspector foundation + Text group#2120

Open
vanceingalls wants to merge 12 commits into
mainfrom
studio-flat-01-foundation-text
Open

feat(studio): flat inspector foundation + Text group#2120
vanceingalls wants to merge 12 commits into
mainfrom
studio-flat-01-foundation-text

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

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_ENABLED feature 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

  • FlatGroup owns the one-open accordion state (openGroupId/onToggleOpen) and pin affordance (onTogglePin), styled per the design mock.
  • FlatTextSection is the first migrated group and the reference implementation every later group's task followed for the isOpen/onToggleOpen/onTogglePin/summary wiring pattern.
  • Includes a same-PR bugfix (found via live browser testing, not caught by any automated test): the Text FlatGroup was rendering unconditionally regardless of element type (empty for non-text elements), and the multi-field fallback doubled the "Text" heading. Fixed by gating on isTextEditableSelection and adding a hideOwnHeading prop to the legacy TextSection fallback.
  • Entirely gated behind STUDIO_FLAT_INSPECTOR_ENABLED (default off) — the legacy panel is untouched and remains the default for all users.

Test plan

  • Every primitive and the Text group have dedicated Vitest suites using real DOM events (click/pointerdown) with exact assertions, not shallow snapshots.
  • Manually verified in Studio via live browser testing against the design mock (this is what caught the bugfix above).
  • Full monorepo test suite green; oxlint/oxfmt clean; this repo's fallow complexity/duplication gate passes.
  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (not applicable — internal Studio UI behind an off-by-default flag)

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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). FlatGroupFlatGroupHeader 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:

  1. Flag-gated parallel path — flat inspector runs alongside legacy, gated by STUDIO_FLAT_INSPECTOR_ENABLED. No legacy behavior changes.
  2. Suppression flags over removal — when a flat group absorbs a legacy section, the legacy gets hideOwnHeading/hideFlex rather than being removed, enabling safe coexistence. Retired cleanly in #2126.
  3. Helpers carry new knowledge; primitives carry new rendering — every helper justifies itself with a fact the caller doesn't know. No trivial wrappers.
  4. 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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vanceingalls vanceingalls force-pushed the studio-flat-01-foundation-text branch from 444639d to 6e9d99e Compare July 11, 2026 01:34
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Rebased the stack base onto latest main (one real conflict in StudioRightPanel.tsx, resolved by merging the DesignPanelPromoteProvider wrapper with the group-selection/hide-all wiring) and force-pushed. Current head 6e9d99ef8 is MERGEABLE, CI is green (32/32). @miguel-heygen — the conflict blocker should be clear now, please re-review when you get a chance.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 535a072917cafb6d3940b914b175faf5b2602814. The foundation still has direct correctness blockers:

  • StudioRightPanel.tsx:346-349 passes element.id ?? element.selector to 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-273 and PropertyPanelEmptyState.tsx:104-109 omit source, hfId, and selectorIndex from React identity, allowing repeated-selector siblings to reuse stale panel state and duplicate list keys.
  • Flat Text still bypasses the legacy PromotableControl path for bound content/color/font fields.
  • Current CI is not green: File size check reports StudioRightPanel.tsx at 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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@miguel-heygen miguel-heygen dismissed their stale review July 14, 2026 07:12

Formatting was corrupted during submission; replacing with corrected review.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vanceingalls and others added 12 commits July 14, 2026 00:59
…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.
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Status on this review's findings, checked against the current stack tip:

Fixed here — PR #2416, commit 267cdfce1:

  • Glyph-only segmented choices (FlatSegmentedRow) now carry aria-label + aria-pressed. FlatSegmentOption gained a required label field; all three call sites (Align, Case·Style incl. the two "A" upright/italic buttons, Flex Direction, Fill mode) supply distinct accessible names.

Already fixed downstream, in PR #2225 (stacked above this PR, so it doesn't show at this PR's own head):

  • Identity/remount staleness — PropertyPanelFlat/ColorGradingSection now key/dedupe via the 4-part selectionIdentityKey.
  • "Hide All" — atomic batch write via timelineKeysForSelections + setElementsHidden.
  • Variable-parity — PromotableControl wrappers restored around Content/Color/Font family in propertyPanelFlatTextSection.tsx.

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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vanceingalls

Copy link
Copy Markdown
Collaborator Author

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 (#2120 → ... → #2190 → #2225 → #2369 → #2416).

This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite (gt merge / sequential merges), not left partially merged with #2225/#2416 dangling for some indefinite period. Main never sits on this PR's head as a final state — by the time this PR merges, the rest of the stack merges with it in the same pass, landing the fixes immediately after.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants