Skip to content

feat(studio): fold variables into the design panel + summonable manager#2101

Closed
jrusso1020 wants to merge 2 commits into
07-08-feat_subcomp_variable_render_pathfrom
07-09-feat_studio_variables_design_panel_slideover
Closed

feat(studio): fold variables into the design panel + summonable manager#2101
jrusso1020 wants to merge 2 commits into
07-08-feat_subcomp_variable_render_pathfrom
07-09-feat_studio_variables_design_panel_slideover

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

What

Retires the dedicated Variables tab and folds variable management into the Design panel, per Miguel's feedback ("variables doesn't need a new tab… can we modify them directly in the design panel"). Stacked on #2084.

Design rationale (adversarially reviewed)

Before building, I ran two design subagents — one attacking the "just fold it in" proposal, one steelmanning it. They converged on:

  1. Drop the tab — creation + default-editing already live in the Design panel (the ◇ var promote pill / ◆ {id} bound chip), so the tab was a redundant second home.
  2. The one real trap is losing bind-to-existing (define-once, bind-many). This PR avoids it by construction: the manager hosts the existing VariablesPanel unchanged, which still contains the "Bind selected" card.
  3. Keep the "previewing N overrides" signal always visible — the trigger button carries it while the manager is closed.

How

  • ◆ Variables trigger in the inspector header (VariablesManagerTrigger) opens an on-demand slide-over; accent-styled + count when preview overrides are active.
  • VariablesManagerSlideOver — right-anchored overlay hosting VariablesPanel verbatim (declare / edit / remove / preview / handoff / other-comps / bind-to-existing). Closes on backdrop click or Esc.
  • Bound chip in the Design panel is now clickable → opens the manager (manage-where-you-see-it).
  • Tab removed from StudioRightPanel, RightPanelTab, and VALID_TABS; ?tab=variables deep links fall back to the default. Selection-outline logic that special-cased the old tab is simplified (the manager overlays the Design panel, so bounds already show).

Follow-up (deferred, not a regression)

Per-control "New variable / Bind to existing…" on the pill (bind-to-existing from the control itself) — the capability is preserved today via the manager's bind card; this would add locality. Left for a follow-up.

Testing

Full studio suite green (1423). Typecheck + lint + fallow + filesize gates pass. ⚠️ Live browser UX (slide-over layout, chip-opens-manager, override-signal accent) wants a human pass — agent-browser canvas-selection driving is unreliable in this repo.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Collaborator Author

@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-reviewing the updated stack at 35f2b4527. The design direction is right: the permanent Variables tab is removed from the tab union (packages/studio/src/utils/studioHelpers.ts:16), stale tab=variables URLs now normalize away (packages/studio/src/utils/studioUrlState.ts:22), and the full manager becomes summonable from the Design surface instead of occupying a permanent tab (packages/studio/src/components/StudioRightPanel.tsx:507). Focused local coverage passes after building workspace deps: studioUrlState.test.ts + variablePromoteIntegration.test.ts = 15/15.

Blocker

  • packages/studio/src/components/editor/PromotableControl.tsx:88 opens the Variables manager with only a global boolean, while the manager renders the right panel's sdkSession (packages/studio/src/components/StudioRightPanel.tsx:582, packages/studio/src/components/panels/VariablesManagerSlideOver.tsx:97). That session is the active/master composition session. But Design-panel promotion itself deliberately targets selection.sourceFile || activeCompPath || "index.html" (packages/studio/src/components/DesignPanelPromoteProvider.tsx:33) so selecting an element inside an inlined sub-composition edits the sub-comp file, not the host. Result: promote/bound-state works against compositions/card.html, then clicking the bound chip opens a manager for the host composition and the variable you just bound is missing or edits the wrong file. This regresses exactly the sub-composition variable workflow the stack adds. Fix shape: make the manager store/open carry a target path (or target session) and have the bound chip open it for the same selection.sourceFile session that VariablePromoteProvider uses; the header trigger can keep using the active/master target.

Important follow-ups still present from prior stack review

  • packages/sdk/src/types.ts:457 still says getVariableValues() mirrors runtime getVariables() exactly, but the implementation is now intentionally per-file/per-declaration-element scoped. That is the right product model for Studio sessions, but the public hover docs should say that rather than promising bundled-document runtime parity.
  • docs/concepts/compositions.mdx:134 and packages/lint/src/rules/composition.ts:671 still tell authors to declare variables on <html> only. The runtime/lint implementation now supports <html> plus template/fragment root divs, so these should be updated to avoid teaching the old model.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The tab-removal/summonable-manager direction is good, but the manager currently opens the wrong composition session for selected sub-composition elements, breaking the core Design-panel variable flow until it carries the same target path as promotion.

Retire the dedicated Variables tab (Miguel's feedback: variables shouldn't need
their own tab). Variable creation and default-editing already live inline in the
Design panel via the promote pill / bound chip; this moves the full list into an
on-demand slide-over summoned by a "Variables" button in the inspector header
(and by clicking a bound control's chip). The slide-over hosts the existing
VariablesPanel unchanged, so the "Bind selected" card — bind-to-existing /
define-once-bind-many — is preserved. The button carries the "previewing N
overrides" signal so it stays visible while the manager is closed.

Adversarially reviewed by two design subagents before building.
Addresses Miguel's CHANGES_REQUESTED on #2101: the bound chip opened the manager
with only a global boolean, so it rendered the host/master session even when the
element and its promoted variable live in an inlined sub-composition file. The
manager store now carries a targetPath; the bound chip passes the promote
context's targetPath, and the slide-over opens a session for it (VariablesPanel
gains compPathOverride so declarations, persist target, handoff, and other-comps
exclusion all key off that file). The header trigger still targets the active
composition.
@jrusso1020 jrusso1020 force-pushed the 07-09-feat_studio_variables_design_panel_slideover branch from 35f2b45 to dbaa962 Compare July 9, 2026 13:17
@jrusso1020 jrusso1020 force-pushed the 07-08-feat_subcomp_variable_render_path branch from bdc5a8d to 16a797d Compare July 9, 2026 13:17
@jrusso1020

Copy link
Copy Markdown
Collaborator Author

Thanks @miguel-heygen — all three addressed.

Blocker (manager opened the wrong session for sub-comp elements) — fixed. The manager store now carries a targetPath. The bound chip passes promote.targetPath (the same selection.sourceFile || activeCompPath resolution DesignPanelPromoteProvider/VariablePromoteProvider use), and the slide-over opens a dedicated session for it via useSdkSession, only while overriding (target set AND ≠ active comp). VariablesPanel gained a compPathOverride prop so its declarations, persist write-target, handoff command, and "other compositions" exclusion all key off that file. The header trigger passes no target → active/master, unchanged. The slide-over header now shows the targeted file path. Pure decision extracted as resolveManagerOverridePath + unit-tested (5 cases: closed / no-target / same-file / different-file / master-view).

Follow-up 1 — getVariableValues hover docs (#2046, types.ts). Rewrote the JSDoc to drop the "mirrors runtime getVariables() exactly" claim; it now states per-file/per-declaration-element scope explicitly and notes the two only diverge when sub-comp declarers are inlined into one bundled document.

Follow-up 2 — <html>-only guidance (#2081). docs/concepts/compositions.mdx and the unknown_variable_binding lint fixHint (packages/lint/src/rules/composition.ts) now say "the composition root — <html>, or the [data-composition-id] root element for a template/fragment comp."

Full studio suite green (1428). Live browser UX still wants a human pass (agent-browser canvas driving is unreliable here). Re-requesting review.

@jrusso1020 jrusso1020 requested a review from miguel-heygen July 9, 2026 13:18
@jrusso1020

Copy link
Copy Markdown
Collaborator Author

Closing this — we're reverting to the dedicated Variables tab (#2050) rather than folding the overview into a summoned surface.

Rationale: the inline promote/edit in the Design panel (#2071 — the ◇ var pill / chip) already delivers Miguel's core ask ('modify variables directly in the design panel'). That stays. The overview/manage-all surface, though, is a browse-and-manage task that wants a persistent, roomy, discoverable home — a tab you opt into reads correctly whether empty or full, whereas a drawer/popover that appears mostly-empty (or dims the live preview) does not. So: keep inline editing in the Design panel; keep the tab as the roster. No net capability change vs. this PR; the sub-comp targeting and doc/JSDoc fixes from the review are preserved on #2046/#2081.

@jrusso1020 jrusso1020 closed this Jul 9, 2026
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.

2 participants