Skip to content

fix(studio): show Layers full-height in the flat inspector, not split with Design#2497

Merged
vanceingalls merged 2 commits into
mainfrom
fix/studio-flat-layers-panel-standalone
Jul 16, 2026
Merged

fix(studio): show Layers full-height in the flat inspector, not split with Design#2497
vanceingalls merged 2 commits into
mainfrom
fix/studio-flat-layers-panel-standalone

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The flat inspector's Layers tab split into a vertically-resizable stacked pair with Design whenever both panes were toggled on, mirroring the legacy panel's layout. For the flat redesign this read as two competing panels crammed into one column; Layers should always render full-height by itself there instead.
  • Gated the split-view branch behind !STUDIO_FLAT_INSPECTOR_ENABLED so it still applies to the legacy panel, and falls through to rendering Layers alone under the flat flag.
  • Added setExclusiveRightInspectorPane (radio-style: selecting one pane turns the other off) and used it for the Design/Layers tab clicks under the flat flag — leaving both panes independently toggleable would highlight both tabs as "active" while only one actually renders.

Test plan

  • Verified live in a running Studio dev instance: Layers renders full-height alone, Design/Layers tabs are mutually exclusive under the flat flag
  • New usePanelLayout.test.ts covers both the existing toggle behavior and the new exclusive variant
  • Full studio suite green; typecheck/oxlint/oxfmt clean

🤖 Generated with Claude Code

… with Design

The flat inspector split Layers and Design into a vertically-resizable
stacked pair whenever both panes were toggled on, mirroring the legacy
panel's layout. For the flat redesign this reads as two competing panels
crammed into one column; Layers should always render full-height by
itself there instead.

Gate the split-view branch behind !STUDIO_FLAT_INSPECTOR_ENABLED so it
still applies to the legacy panel, and fall through to Layers rendering
alone (the existing `layersPaneOpen` branch already does this — it just
never got reached previously because the split check ran first).

Also added setExclusiveRightInspectorPane (radio-style: selecting one pane
turns the other off) and use it for the Design/Layers tab clicks under the
flat flag, since leaving both panes independently toggleable would highlight
both tabs as "active" while only one actually renders.

New usePanelLayout.test.ts covers both the existing toggle behavior and the
new exclusive variant. Full studio suite (2634 tests) green; typecheck/
oxlint/oxfmt clean.

@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.

Review — #2497: show Layers full-height in flat inspector

Invariant: in flat-inspector mode, exactly one right-inspector pane is visible (radio-style); in legacy mode, panes toggle independently with a last-pane guard.

Data carriers: RightInspectorPane (union "layers" | "design"), rightInspectorPanes state, setExclusiveRightInspectorPane callback, STUDIO_FLAT_INSPECTOR_ENABLED gate.

What I checked

Area Verdict
setExclusiveRightInspectorPane exhaustive over RightInspectorPane union ✅ — union is exactly "layers" | "design", setter covers both
handleToggleInspectorPane routes correctly per flag ✅ — early return under STUDIO_FLAT_INSPECTOR_ENABLED calls setExclusiveRightInspectorPane, legacy path falls through to toggleRightInspectorPane
Render branch gated on !STUDIO_FLAT_INSPECTOR_ENABLED ✅ — split-container only renders in legacy mode; flat-inspector falls through to single-pane branches
Bypass risk (direct toggleRightInspectorPane call while flat inspector is on) Acceptable — last-pane guard prevents both-false; render branch prevents split view; worst case both show "active" in state but only one renders
Test coverage ✅ — independent toggle, last-pane guard, and exclusive radio-style toggle all exercised
PanelLayoutContext threading ✅ — new setter threaded through provider value consistently
SSOT ✅ — "flat = exclusive" decision lives in one place (handleToggleInspectorPane); render gate is a separate concern (layout, not decision)

Disconfirming probe: can setExclusiveRightInspectorPane("layers") followed by toggleRightInspectorPane("design") leave both panes on? Yes in state — but only if someone bypasses handleToggleInspectorPane. The UI handler is the only call site, and it gates on the flag. Residual risk: negligible.

Verdict

Code: clean. CI: some checks still in-progress (typecheck, test, windows render/build). Merge gate: wait for CI.


Review by Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed at 79b688f2. Traced the exclusivity invariant across every writer of rightInspectorPanes and every caller of setRightPanelTab("design"|"layers").

Blockers

(none)

Concerns

🟠 trackedSetRightPanelTab bypasses the new exclusivity invariant — trivially reproducible on fresh boot.

The Design/Layers tab-click handler correctly calls setExclusiveRightInspectorPane when inspectorTabActive === true (line 247-249 in StudioRightPanel.tsx), but the entry path (!inspectorTabActive at line 240-242) falls back to setRightPanelTab(pane) — and that resolves to trackedSetRightPanelTab in usePanelLayout.ts:80-90, which does the additive setRightInspectorPanes((panes) => ({ ...panes, [tab]: true })) regardless of the flat flag. Three external callers (StudioHeader.tsx:344, App.tsx:523, useDomSelection.ts:216) also route through the same trackedSetRightPanelTab without any flat-flag exclusivity handling.

Concrete repro from STUDIO_FLAT_INSPECTOR_ENABLED=true fresh boot:

  1. Default state: rightPanelTab="renders", rightInspectorPanes={design:true, layers:false} (per getInitialRightInspectorPanes).
  2. User clicks the Layers tab. handleInspectorPaneButtonClick("layers") sees !inspectorTabActive === truesetRightPanelTab("layers")trackedSetRightPanelTab runs the additive spread → new state rightInspectorPanes={design:true, layers:true} and rightPanelTab="layers".
  3. Now inspectorTabActive === true, both designPaneOpen and layersPaneOpen evaluate true (assuming designPanelActive and STUDIO_INSPECTOR_PANELS_ENABLED), so both tab-button active states light up.
  4. Render tree falls to layersPaneOpen ? <LayersPanel /> (branch 2 in the ternary, since the split branch is gated !STUDIO_FLAT_INSPECTOR_ENABLED). Design pane never renders.

Result: both tabs highlight, only Layers renders — the exact "highlight both tabs as 'active' while only one actually shows" outcome the PR body says this change prevents. The same shape reproduces via any external caller that reaches setRightPanelTab("design") (element-select → useDomSelection.ts:216, closing block-params → App.tsx:523, the Inspector button in StudioHeader.tsx:344).

Two clean fix shapes:

  • In the hook (my preference — centralizes the invariant): make trackedSetRightPanelTab flat-aware.

    if (tab === "design" || tab === "layers") {
      if (STUDIO_FLAT_INSPECTOR_ENABLED) {
        setRightInspectorPanes({ design: tab === "design", layers: tab === "layers" });
      } else {
        setRightInspectorPanes((panes) => ({ ...panes, [tab]: true }));
      }
    }

    Adds the flag import to usePanelLayout.ts, one branch, closes the invariant across every current and future caller of setRightPanelTab("design"|"layers").

  • At each call site: pair every setRightPanelTab("design"|"layers") with a setExclusiveRightInspectorPane(...) under the flat flag. Works, but four sites (this PR's handleInspectorPaneButtonClick entry branch + the three external callers I traced) all need it individually — one gets forgotten, invariant leaks again.

Either fix should be paired with a new usePanelLayout.test.ts case pinning trackedSetRightPanelTab("layers") under flat=on from a state where design is already true — right now the tests only exercise the toggle/exclusive primitives directly, not the tab-switch path that hits trackedSetRightPanelTab.

Nits

(none)

Green notes

🟢 setExclusiveRightInspectorPane primitive itself is clean. Radio semantics correctly enforced ({ design: pane === "design", layers: pane === "layers" }), stable useCallback([], ...), test at :57-67 pins both directions plus the transition from both-true → exclusive. Threading through PanelLayoutContext.tsx is complete and consistent.

🟢 Render-branch gating is correct in isolation. layersPaneOpen && designPaneOpen && !STUDIO_FLAT_INSPECTOR_ENABLED short-circuits split view under flat, and the ordered fallthrough (layersPaneOpen before designPaneOpen) matches the PR body's "Layers renders full-height by itself" contract. The inspectorTabActive "unavailable" branch at line 583+ still catches the pathological {design:false, layers:false} state gracefully.

🟢 toggleRightInspectorPane's existing "refuses to turn off the last remaining pane" guard is preserved. No collateral change to that invariant.

🟢 Test file is well-scoped. New usePanelLayout.test.ts uses happy-dom + React.act, covers the two existing behaviors + the new exclusive primitive at the hook level. Adding one more case for the trackedSetRightPanelTab entry path under flat=on would fill the gap I flagged above.

Hostile counterexamples

  • Layers-first initial state (initialState.rightPanelTab === "layers"getInitialRightInspectorPanes returns {layers:true, design:false}): first click on Design tab under flat=on hits !inspectorTabActive branch → same buggy state. Symmetric to the boot-fresh repro above.
  • Programmatic element-select landing on Renders/Slideshow/Variables: user is on the Variables tab, selects an element in preview → useDomSelection.ts:216 gates on rightPanelTabRef.current !== "variables" and no-ops. Safe. But from any other tab (renders, slideshow, block-params) the same select triggers setRightPanelTab("design") → buggy state.
  • Block-params close: App.tsx:522-524 onCloseBlockParamssetRightPanelTab("design"). If the user opened block-params from the Layers tab, closing block-params lands the buggy {design:true, layers:true} state.

What I didn't verify

  • Didn't run the studio dev instance in flat mode to eye-check the actual DOM. My concern is a static-analysis walk of the exclusivity invariant across writers; a manual click-through would confirm the tab-highlight side of the bug. The state-shape divergence I traced is confident.

Verdict framing

Fix is directionally right but leaks the exclusivity invariant on every non-direct-click writer. Best addressed at the hook (one branch) rather than every call site. LGTM from my side once trackedSetRightPanelTab (or the equivalent choke-point) is flat-aware and a test pins the entry path.

Review by Rames D Jusso

…rect tab click

Review feedback on #2497 (Rames D Jusso) found a real gap: the exclusivity
this PR introduced only applied to the direct in-panel tab click, which
calls setExclusiveRightInspectorPane. Every OTHER caller that reaches
setRightPanelTab("design"|"layers") — element select (useDomSelection.ts),
closing block-params (App.tsx), the header Inspector button
(StudioHeader.tsx), and even this PR's own "!inspectorTabActive" entry
branch in handleInspectorPaneButtonClick — went through
trackedSetRightPanelTab's old unconditional additive
`{...panes, [tab]: true}`, reproducing the exact "both tabs highlight, only
one renders" bug this PR claims to fix. Confirmed via the reviewer's traced
repro: fresh boot, click Layers tab while no inspector tab is yet active →
rightInspectorPanes ends up {design:true, layers:true}.

Fixed at the reviewer's preferred choke point: trackedSetRightPanelTab
itself is now flat-aware, applying the same exclusive-radio update
setExclusiveRightInspectorPane does whenever STUDIO_FLAT_INSPECTOR_ENABLED
is on, falling back to the legacy additive update otherwise. This closes
the gap for every current and future caller of setRightPanelTab, not just
the one call site this PR touched.

New usePanelLayout.test.ts cases pin both directions: setRightPanelTab
stays additive under flat=off (legacy split-view behavior unchanged), and
enforces exclusivity under flat=on even when called directly (not through
the tab-click handler) — using the vi.doMock(manualEditingAvailability)
pattern already established in PropertyPanel.test.tsx for flag-dependent
module state.

Full studio suite (2643 tests) green; typecheck/oxlint/oxfmt clean.
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Fixed per your traced repro — thanks for tracking it through every writer.

trackedSetRightPanelTab (usePanelLayout.ts) is now flat-aware itself: it applies the same exclusive-radio update setExclusiveRightInspectorPane does whenever STUDIO_FLAT_INSPECTOR_ENABLED is on, falling back to the old additive update otherwise. This closes the gap at your preferred choke point rather than patching each of the four call sites — useDomSelection.ts:216, App.tsx:523, StudioHeader.tsx:344, and this PR's own !inspectorTabActive entry branch all now go through the corrected function automatically.

Added usePanelLayout.test.ts cases pinning both directions: setRightPanelTab stays additive under flat=off, and enforces exclusivity under flat=on even when called directly (not through the tab-click handler) — using the vi.doMock(manualEditingAvailability) pattern already established in PropertyPanel.test.tsx for flag-dependent module state.

Full studio suite green, typecheck/oxlint/oxfmt clean. Pushed as fb24ecac2.

@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.

Re-review — #2497 v2: flat-inspector exclusivity in setRightPanelTab

The v2 closes the bypass path I flagged in round 1. Previously, only the in-panel tab click (handleToggleInspectorPanesetExclusiveRightInspectorPane) enforced radio-style exclusivity — every other caller reaching trackedSetRightPanelTab directly (element select, closing block-params, header Inspector button) would still additively open both panes, causing the "both tabs highlight, only one renders" bug.

What changed

usePanelLayout.ts: trackedSetRightPanelTab now gates on STUDIO_FLAT_INSPECTOR_ENABLED:

  • Flat mode → exclusive: { design: tab === "design", layers: tab === "layers" }
  • Legacy mode → additive: (panes) => ({ ...panes, [tab]: true })

Tests: Two new cases with vi.doMock to toggle the flag per test:

  • Legacy (STUDIO_FLAT_INSPECTOR_ENABLED: false): setRightPanelTab("layers") leaves both panes on — additive, as expected.
  • Flat (STUDIO_FLAT_INSPECTOR_ENABLED: true): setRightPanelTab("layers") sets layers-only — exclusive, closing the bypass.

renderPanelLayoutWith refactored to accept the hook as a parameter for module-mocked variants. Clean.

Verification

The exclusivity decision now lives in two places: setExclusiveRightInspectorPane (direct tab click) and trackedSetRightPanelTab (all other callers). Both use the same { design: tab === "design", layers: tab === "layers" } expression. This is NOT a SSOT violation — they're different entry points for the same invariant, and the expression is trivial enough that extracting it would add indirection without value.

Disconfirming probe (resolved): Can any caller still additively open both panes under flat mode? No — setRightPanelTab covers the indirect paths, setExclusiveRightInspectorPane covers the direct tab click. The raw toggleRightInspectorPane is still independently toggleable, but it's not called under the flat flag (gated in handleToggleInspectorPane).

Verdict

Code: clean. Probe gap from v1 is closed. LGTM.


Review by Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-verified at fb24ecac. Δ from 79b688f2: 2 files / +66 / -4 — one new fix commit landing at the reviewer-preferred choke point.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 R1 concern closed at the hook layer, as suggested. usePanelLayout.ts:81-97 now makes trackedSetRightPanelTab itself flat-aware — the ternary

setRightInspectorPanes(
  STUDIO_FLAT_INSPECTOR_ENABLED
    ? { design: tab === "design", layers: tab === "layers" }
    : (panes) => ({ ...panes, [tab]: true }),
);

matches the exclusive-radio shape setExclusiveRightInspectorPane uses, so every caller reaching setRightPanelTab("design"|"layers") — the !inspectorTabActive entry branch in handleInspectorPaneButtonClick, StudioHeader.tsx:344, App.tsx:523, useDomSelection.ts:216, and any future caller — now enforces exclusivity under the flat flag without needing per-site coordination. Under STUDIO_FLAT_INSPECTOR_ENABLED=false, the legacy additive spread runs unchanged, so split-view behavior is preserved.

🟢 New tests pin both directions of the invariant. usePanelLayout.test.ts:76-92 covers the flat-off legacy additive path ({design:true, layers:false} → setRightPanelTab("layers") → {design:true, layers:true}), and :94-116 covers the flat-on exclusive path (same starting state → {design:false, layers:true}). The vi.doMock(manualEditingAvailability) + vi.resetModules() + re-import pattern is the right shape for a module-load-time flag constant — and the vi.doUnmock + vi.resetModules() in afterEach cleanly restores between cases so the earlier renderPanelLayout()-based tests aren't affected.

🟢 Commit message credits the R1 walkthrough and reproduces the failure mode. Every concrete flow I traced (element-select via useDomSelection, block-params close via App.tsx, header Inspector button via StudioHeader, and this PR's own !inspectorTabActive entry branch) is named explicitly — future readers can see WHY the choke point moved.

🟢 No collateral change. setExclusiveRightInspectorPane, toggleRightInspectorPane, and the render-tree gating in StudioRightPanel are byte-identical to R1. Delta is limited to usePanelLayout.ts + its test file.

Verdict framing

Concern closed at the choke point I preferred, with test coverage for both flag states. Clean second round. LGTM from my side.

Review by Rames D Jusso

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