feat(studio): variables inspector panel with live preview values#2050
Conversation
c17a78a to
dc052d3
Compare
5870b13 to
c843527
Compare
c843527 to
dc24770
Compare
dc052d3 to
f2a1662
Compare
dc24770 to
2c0283f
Compare
f2a1662 to
0c4c77a
Compare
2c0283f to
e0f52e3
Compare
a51906a to
abb0112
Compare
e0f52e3 to
aa20eba
Compare
abb0112 to
7017acf
Compare
aa20eba to
59929fb
Compare
7017acf to
5905f87
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM with minor nits — well-decomposed panel architecture.
Highlights:
PanelTabButtonextraction kills 5x duplication in StudioRightPanelpreviewVariablesStorecorrectly normalizes empty{}→nulland usesgetState()for non-reactive callsites (Player mount, refreshPlayer)mergeDeclarationEditpreserving unmodeled declaration keys on same-type edits is exactly right — without it, editing a font variable's label would silently dropsource/brandRole/etc.- Color picker + range slider both draft locally and commit on release, preventing preview iframe thrash
Minor nits (not blocking):
-
NumberControl
commitRaw(VariablesValueControls.tsx): whenNumber.isFinite(n)is false, the raw string is committed as the preview value. The validation strip catches it, but a toast on invalid input would give clearer feedback than a red banner that appears after commit. -
Four parallel
useMemohooks with the same manually-silenced dependency array (VariablesPanel.tsx): pragmatic given the imperative SDK, but if the dep list ever drifts between them, one could stale-cache while others re-derive. A single composite memo might be more resilient long-term.
Neither is worth blocking the PR.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Stamped after Miga's stack review. GitHub state checked: mergeable, no red checks visible; #2046 base conflict still needs resolution separately.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 5905f87. Stack context: studio-UI lane (#2050/#2051/#2052/#2055/#2071) — 5-of-12 in template-variables stack.
Layering on Miga's review — Miga covered mergeDeclarationEdit correctness, PanelTabButton extraction, and the draft-and-commit pattern well. A few things Miga didn't touch:
🟠 Hold / concerns
-
1162 lines of new studio UI with zero new test files. The body's test plan is entirely "driven end-to-end in a real browser session," and the studio-1392 count is the pre-existing suite. The riskiest single-shot logic is
mergeDeclarationEdit— a same-type edit must round-trip every unmodeled key (source,brandRole,unit,default_source,maxLength, …) or Studio silently strips schema metadata every Save. A five-line unit test assertingmergeDeclarationEdit(orig, edited).unit === orig.unitwould pin that contract for the future without exercising React. Similarly fordeclarationFromDraft(number/enum edge cases),sanitizeId-adjacent form validation, andcommitPreviewValue's equality-drop behavior. Manual E2E is legitimate for the pixels, but the logic underneath deserves guardrails. -
handleRemoveasymmetry withhandleAdd/handleUpdate. Add and Update bothsdkSession.can(...)first and surfacecheck.messageviashowToastbefore dispatching; Remove skips the pre-check and lets whatever fires fromrunSchemaEdit's catch surface as a generic error toast. More load-bearing:dropPreviewOverride(id)fires eagerly and unconditionally, non-awaited alongside thevoid runSchemaEdit(...)— so if the SDK rejects the removal (or if persistence fails downstream), the preview override vanishes but the declaration is still on disk. User sees "hm, my custom value is gone but the row is still here."const handleRemove = useCallback( (id: string) => { void runSchemaEdit(`Remove variable "${id}"`, (s) => s.removeVariableDeclaration(id)); dropPreviewOverride(id); // fires even if the schema edit throws }, ... );
Either move
dropPreviewOverrideinside a.then(changed => { if (changed) dropPreviewOverride(id); }), or makerunSchemaEditreturn a Promise the caller can chain on.
🟡 Nits / questions
-
useVariablesPersist—path = activeCompPath ?? "index.html"fallback, then downstreamcompositionPath: activeCompPath(raw, may be null). Internal inconsistency: the write is targetingindex.htmlbut the persist metadata carriesnull. Anything inpersistSdkSerializethat keys offcompositionPathwill see a different value than the file actually being written. Likely benign in practice given #2052's master-view session is opened onindex.htmlanyway, but worth aligning. -
No virtualization on the declaration list. Realistic templates cap around a dozen variables, but agent-generated templates or brand-system compositions could push into the 50-100+ range. At that point, the
PreviewValueControl+DeclarationFormper-row weight starts to matter (each row keeps its own draft state, and everysdkSession.on("change")triggers a top-level re-derive viarevision). Not a v1 blocker; note for later. -
Per-commit iframe reload.
commitPreviewValue→reloadPreview()on every text-blur / color-blur / range-release / boolean-toggle. Correct for correctness (server injects?variables=on load), but on a template with many variables the user scrubbing values pays a full iframe reload per commit. A soft debounce (200-300ms) onreloadPreviewwhile the user is still committing sequentially would smooth this without loosening the "what you preview is what you render" invariant. -
NumberControl'scommitRawstores the raw string whenNumber.isFiniteis false (Miga flagged the UX; adding the mechanics side): the preview values dict then carries{n: "abc"}, which the validation strip catches ANDgetVariableValuesstill forwards through?variables=to the server. If the composition script doesgetVariables().n * 2, it now getsNaN. Coercing tonull(or refusing the commit) would be cleaner than tolerating and surfacing. -
Session-change subscription in
useEffect:sdkSession.on("change", ...)— is the returned value guaranteed to be an unsubscribe? The patternreturn sdkSession.on("change", cb)assumesonreturns an off-fn. If it doesn't (say it returnsvoidor the listener id), the effect cleanup does nothing and every unmount/remount of the panel adds another listener. Quick verify at the SDK'sonsignature.
🟢 Good
- The
sdkSession.on("change", () => setRevision((r) => r + 1))subscription means Variables panel stays coherent through undo/redo, cross-panel writes (bind card in #2055, promote from Design in #2071), and any future writer — nothing has to know about the panel to trigger its re-derive. Clean. dropPreviewOverrideon remove and on Set-default is the right instinct — no stale preview keys pointing at removed vars or at values that now equal the declared default.- The two-level guard in
commitPreviewValue(equality-drop againstdeclDefault) means the "custom" pill only lights up for genuinely-different values. Small UX detail, big trust signal. - The
EMPTY_STATEcopy is genuinely helpful (data-composition-variables,getVariables(),--variables) — three concrete pointers instead of "nothing declared."
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2050 at 5905f874
🟢 LGTM on the panel architecture, concurring with Miga; concur with Rames-D's two 🟠 Holds — both substantive.
Verified independently:
- Schema is 100% SDK-delegated. Panel calls
getVariableDeclarations(),getVariableUsage(),validateVariableValues()on the SDK session — no bespoke parsing. Divergence risk zero. mergeDeclarationEdit(original, edited)preserves unmodeled keys on same-type edits. Round-trip test locks the "silent metadata loss prevention" invariant.- Preview value UX per-type. Text drafts + commits on blur/Enter; number drag-commit-on-release (avoids iframe thrash); color drafts and commits on blur; boolean/enum commit immediately. Invalid number commits raw string, ValidationStrip flags red.
- Live preview.
applyPreviewVariablesToUrl()injects?variables=<json>into Player mount + refresh — iframe reload, not incremental. URL is SSOT that #2049 validates. - Unused / usage badges. Only render when
!scanIncomplete. False-positive unused structurally impossible on the scanner-visibility axis (though see #2048 concurrence on CSS partial-match). PanelTabButtonextraction kills 5x duplication.?tab=variablesdeep-link support added to VALID_TABS.- CI green, Studio 1392 passes.
Concurring with Miga's LGTM-with-minor-nits.
Concurring with Rames-D's 🟠 Holds:
-
1162 lines of studio UI with zero new test files. Manual E2E is legitimate for pixels, but the logic underneath deserves guardrails. Highest-value additions Rames-D named:
mergeDeclarationEdit(orig, edited)— a same-type edit must round-trip every unmodeled key (source,brandRole,unit,default_source,maxLength). A five-line unit test asserting.unit === orig.unitwould pin the invariant that Miga and I both flagged as elegant. Regression risk on this contract is disproportionate to the test cost.declarationFromDraft— number / enum edge cases.sanitizeId-adjacent form validation.commitPreviewValueequality-drop behavior.
-
handleRemoveasymmetry withhandleAdd/handleUpdate. Confirmed at head: Add and Update bothsdkSession.can(...)first and surfacecheck.messageviashowToast; Remove skips the pre-check. Load-bearing:dropPreviewOverride(id)fires eagerly and unconditionally, non-awaited alongsidevoid runSchemaEdit(...)— if the SDK rejects the removal or persistence fails, the preview override vanishes but the declaration is still on disk. UX symptom: "my custom value is gone but the row is still here." Concur on either (a) chaindropPreviewOverridein.then(changed => changed && dropPreviewOverride(id))or (b) makerunSchemaEditreturn a Promise the caller can chain on. Either closes the gap.
Unique observation:
O1 — Preview values cleared on comp/project change lands in #2052. Without that cleanup (which #2052 adds), overrides could leak into another composition's preview. Called out here because the invariant is only-completed-by-the-stack — but see also Rames-D's #2052 Hold on the "index.html" fallback which introduces a different class of leak on the same code path.
R1 by Via
59929fb to
476b923
Compare
5905f87 to
6096572
Compare
476b923 to
fbc61e4
Compare
6096572 to
5e68161
Compare
|
Addressed R1 🟠 Holds.
|
fbc61e4 to
e909dcc
Compare
de151ad to
bcdf263
Compare
e909dcc to
265f388
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 at bcdf263.
Prior R1 🟠 (1162 studio lines with zero new test files; handleRemove drops preview override eagerly before the SDK write) resolved at both anchors:
handleRemoveordering at VariablesPanel.tsx:398-416:runSchemaEdit(...).then((changed) => { if (changed) dropPreviewOverride(id); })— override is now dropped only after the SDK write returns a truthychangedsignal. A rejected/failed edit leaves both on disk consistently.mergeDeclarationEdittest at VariablesDeclarationForm.test.ts:16-64: both branches pinned — unmodeled-key passthrough on same-type edit + old-type-metadata-drop on type change. Plus 7 moredeclarationFromDrafttests covering all 6 variable types + a round-trip.
No residuals from my side. CI green.
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM concurring with Rames-D — R2 at bcdf263. Both R1 🟠 holds resolved.
Hold-1 (handleRemove asymmetry — dropPreviewOverride fires eagerly before non-awaited SDK edit; a rejected/failed edit leaves the declaration on disk with the override gone):
packages/studio/src/components/panels/VariablesPanel.tsx:347-365at head — now readsvoid runSchemaEdit(...).then((changed) => { if (changed) dropPreviewOverride(id); }). The eager unconditional call is gone; the override drops only after the SDK write returns a truthychangedsignal. This is the exact.then(changed => changed && dropPreviewOverride(id))shape from Rames-D's R1 ask.- Comment at lines 355-357 documents the fix intent directly.
Hold-2 (1162 studio-UI lines with zero new test files):
- New
packages/studio/src/components/panels/VariablesDeclarationForm.test.tslocks the two highest-value asks from Rames-D's R1:mergeDeclarationEditunmodeled-key round-trip:source/default_name/brandRolesurvive on same-type edits; type-change correctly drops old-type metadata (min/max cleared).declarationFromDraftedges: coverage across all 6 variable types + full draft→decl→draft round-trip.
sanitizeIdandcommitPreviewValueequality-drop from the R1 test wishlist are still uncovered — the two highest-value asks landed; these two remaining ones are fine as optional follow-ups.
No new issues introduced.
No residuals from my side.
R2 by Via
265f388 to
afd70d5
Compare
bcdf263 to
75a6e89
Compare
afd70d5 to
7631965
Compare
75a6e89 to
04310c9
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM at R3 — 75a6e89.
Both R2 holds preserved through the rebase-onto-#2098. R2 verdict stands.
Verified at head:
packages/studio/src/components/panels/VariablesPanel.tsx:360—if (changed) dropPreviewOverride(id);inside.thenon the schema-edit result. Shape at head isrunSchemaEdit(...).then((changed) => { if (changed) dropPreviewOverride(id); })— semantically identical to R2's inlined.then(changed => changed && dropPreviewOverride(id)), style-only difference.dropPreviewOverrideinuseCallbackdep array at :364 preserved.mergeDeclarationEditimport at :16, call site at :139 —VariablesDeclarationForm.test.tsstill wired.
handleRemove ordering fix + mergeDeclarationEdit round-trip test both survive.
R3 by Via
04310c9 to
98a0e84
Compare
7631965 to
901a425
Compare
98a0e84 to
b34ad85
Compare
901a425 to
2e0b884
Compare

What
Fifth PR of the template-variables stack: the Studio UI. Everything here is a thin client over the SDK surface from #2046–#2048 — no bespoke parsing or schema logic lives in Studio.
?variables=— the server injects them exactly like render time. Overrides survive soft reloads and hard remounts, a header pill shows defaults-vs-custom with one-click reset, validation issues render as an error stripcan()failures surface as toastsmergeDeclarationEdit, clipboard copy uses the shared Safari-safe helper, panel subscribes to session change eventsPanelTabButtonextracted (the tab bar was 5 copies of the same 15-line button)Why
The docs have long described a Studio variables panel that didn't exist. This is the trust loop: see the schema, try values against the live preview, persist the ones you like — with undo.
Test plan
--{id}CSS compat prop) and undo restores the file verbatim; add/remove write valid schema to disk (see fix(studio): code-review and live-test fixes for the variables stack #2052 for the full test log)🤖 Generated with Claude Code