feat(studio): bind selected element properties to variables by jrusso1020 · Pull Request #2055 · heygen-com/hyperframes

jrusso1020 · 2026-07-08T06:48:04Z

What

Ninth PR of the template-variables stack: the promote-a-property gesture. Select an element on the canvas/timeline, open the Variables tab, and a "Bind selected" card offers per-property actions built from the selection:

Image/media source (img/video/audio — <source> deliberately excluded, rewriting it post-resource-selection is a spec no-op) → data-var-src
Text (leaf elements only — the promote-never-changes-the-render guarantee doesn't hold for containers) → data-var-text
Text color / Background / Font → <prop>: var(--id) style bindings

Each action declares a variable whose default is the element's current value (computed rgb()/rgba() colors convert to hex; first computed font family becomes the font default). Declare + bind is one batched schema edit (single undo step). Binding to an existing id is allowed only when the types agree (toast otherwise); the card requires the SDK session to resolve the selected element (no dead binds on unmapped sub-comp elements) and resets its form state per selection.

UX polish from live testing feedback:

Selecting elements no longer yanks the panel to Design while the Variables tab is open — swap between canvas elements with the tab pinned, the bind card follows the selection

Also fixes two pre-existing bugs surfaced by this work:

buildSubCompositionHtml's extractElementAttrs rebuilt <html>/<body> attributes without HTML-escaping — data-composition-variables (JSON) was shredded into bogus markup, so getVariables() silently returned {} on every /preview/comp/* page (regression test added)
The capture pipeline's htmlExtractor had the same escape gap for &

Test plan

studio 1392 / studio-server 260 (incl. attribute-integrity regression test) / core 1127; all typechecks + builds
Driven end-to-end in a live browser session twice (initial + post-review-fixes): select headline → bind Text color → declaration + var(--headline-color) on disk → override in panel → player iframe carries the override, runtime applies the custom prop, element renders the override; tab-pinning and selection-swap verified live

🤖 Generated with Claude Code

jrusso1020 · 2026-07-08T06:48:24Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

miga-heygen

LGTM — the bind flow is well-guarded and the attribute-escaping fixes are real bugs worth calling out.

On the bind flow: leaf-only text bindings, <source> exclusion, isCompositionHost guard, and source-file matching on bindableSelection are all correct safety gates. The type-compat check in handleBind prevents silently wiring incompatible types. The key={hfId} reset on the bind card is the right pattern for per-selection form state.

The escapeAttrValue fix in subComposition.ts is the most important change in this PR — without it, data-composition-variables JSON was being shredded into bogus attributes on the /preview/comp/* route, silently breaking getVariables(). The regression test round-trips JSON through the builder and verifies decode integrity. The parallel fix in htmlExtractor.ts is necessarily inline (Puppeteer browser context), not an SSOT violation.

The selection UX fixes (tab-pinning via ref, selection bounds visibility for the variables tab) are clean and don't regress existing Design-tab behavior.

Minor nit: sanitizeId returns "variable" for empty input and the Bind button has no disabled state for empty/whitespace IDs — a user could accidentally create a variable named "variable". Very minor since the ID field is visible.

— Miga

miguel-heygen

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

Reviewed at 064663e. Stack context: studio-UI lane (#2050/#2051/#2052/#2055/#2071) — 5-of-12 in template-variables stack.

Layering on Miga's review — Miga covered the leaf-only text bind, <source> exclusion, isCompositionHost guard, sanitizeId fallback, and the two real escape-integrity fixes (subComposition + htmlExtractor). One spec-vs-impl finding they didn't hit.

🟠 Hold

"Bind to existing id" silently changes the render — contradicting the PR body's "promoting never changes the render" guarantee.

In applyBind (VariablesBindElement.tsx:158):
```
session.batch(() => {
  if (!session.getVariableDeclarations().some((d) => d.id === id)) {
    session.declareVariable(action.declaration(id));
  }
  // ... setStyle / setAttribute using `var(--id)` / `data-var-<kind>`
});
```
When the id already exists, action.declaration(id) (which carries the selected element's current value as default) is discarded — the binding is wired, and the element now resolves through the existing variable's default, which came from a different element. Concrete failure:
1. Element A is red. User types "accent" → declares {id:"accent", type:"color", default:"#ff0000"}. A now uses var(--accent) → red. ✓
2. Element B is blue. User types "accent" (reusing). Type-compat guard in handleBind passes (both color). applyBind sees "accent" exists → skips declaration. Applies style="color: var(--accent)" to B. B renders red, not blue. Render just changed silently.
The body body reads: "Each action declares a variable whose default is the element's current value (…) Binding to an existing id is allowed only when the types agree." The "existing id" branch is acknowledged as allowed but the copy doesn't warn that the render will change. Nor does the UI — the Bind button is fully live regardless of whether the id exists.

Three ways to resolve, roughly in ascending order of user surprise:
1. Detect existing-id at "Bind" click, show an inline "This will change {element} to {var}'s current default ({value}) — bind anyway?" confirmation.
2. On existing-id bind, also route through setVariableValue(id, element_current_value) so the shared variable's default snaps to the newest element. Surprising because it silently changes OTHER elements bound to the same var.
3. Update the PR body + panel copy to acknowledge the coupling behavior, leave the code as-is (advanced users typing an existing id are doing it deliberately). Lowest-cost, but the current copy is misleading.
#2071's right-click promote sidesteps this entirely via uniqueId(action.suggestedId, declarations) — good split between "auto-name = always fresh" and "user-typed = intent respected." Just want the Variables-tab card to be either honest about the trade-off or safer at click time.

🟡 Nits / questions

sanitizeId fallback → "variable" (Miga flagged the empty-input case): worth adding a guarded Bind button (disabled={idDraft.trim().length === 0}) rather than relying on the fallback string. Two elements accidentally binding to variable would collide in the type-compat check and toast, but the primary preventive should be at the button.
buildBindActions — capabilities.canEditStyles gate: the three style bindings (color / background / font) all appear together when the capability is set. What about elements where canEditStyles=true but the element has no visible text (say, a container <div> with no children of its own)? "Text color → variable" appears but "Text" doesn't (leaf gate excludes it), and clicking "Text color" declares a color variable defaulting to the container's computedStyles["color"] — which for a container is often the inherited #000000. Not a bug, but the resulting bind may not affect anything visible. Consider hiding "Text color" when there's no OWN text on the element.
preview.test.ts regression test decodes " then & — order works for the test data (no double-encoded entities), but a real browser HTML decoder handles both simultaneously. Fine for the assertion; noting for anyone extending the test with entity-heavy payloads.
firstFontFamily(value) — computed font-family returns the full font stack (e.g. "Inter", "Helvetica Neue", sans-serif). Taking the first entry as the variable default is the right choice, but if the first is a fallback like -apple-system, the variable's default is meaningless outside macOS. Worth acknowledging that "default = first computed family" is a heuristic that might need tightening once real templates land.

🟢 Good

The session.batch() around declare-then-bind gives users a single undo step — critical UX. Missing batch here would surface as "undo unbound but the declaration lingers, undo again to get the declaration."
useDomSelection's conditional setRightPanelTab("design") — keeping the Variables tab pinned while the user swaps selections — is precisely the polish the "bind selected" flow needed. Without it the panel yanks away every selection change.
The two escape fixes are both real bugs: subComposition's attribute rebuild was shredding any quote-bearing attribute (silent zero-variables regression on /preview/comp/*), and htmlExtractor had the same &-escape gap in a different context. The escapeAttrValue helper is centralized and reused in the shared path.
Type-compat check in handleBind is the right guard against wiring a color style to a string variable. The toast surfaces the specific mismatch ("accent" is already a color variable — pick another id for this font binding) — actionable.

— Review by Rames D Jusso

vanceingalls

R1 — hyperframes #2055 at 064663e2

🟢 LGTM on the bind mechanism + attribute-escaping fixes, concurring with Miga; concur with Rames-D's 🟠 Hold on the existing-id silent-render-change — the code is right, the copy is misleading.

Verified independently:

Bindable-selection gate. Card renders only when domEditSelection?.hfId && sourceFile === activeCompPath && domEditGroupSelections.length <= 1. Single-file, single-element. Composition-host / non-leaf-text guards prevent binding text on containers.
Bindable properties enumerated. buildBindActions() handles src (img/video/audio), text (leaf-only), style-props (color, background, font-family) via CSS custom props. <source> tags deliberately excluded.
Auto-declare + type-conflict guard. applyBind() at VariablesBindElement.tsx:271-287 — single batched edit: declares only if not already declared, then binds. Type mismatch (existing.type !== wanted) toasts an error rather than silent overwrite.
Attribute shape. setAttribute(hfId, "data-var-${action.kind}", id) for src/text; setStyle() writes color: var(--varId) for style-props. Round-trip test in #2071's variablePromoteIntegration.test.ts:790-802 confirms readable.
Undo integrity. Single batched edit = single undo step. key={bindableSelection.hfId} resets idDraft on selection change.
escapeAttrValue() pre-existing bug fix. subComposition.ts:66-68 + htmlExtractor.ts:27 — now escape both & and ". Regression test at preview.test.ts:91-113. Real bug independent of the bind feature; would be worth landing on its own merits.
Tab-pinning fix. useDomSelection.ts:446-460 — selection swap in Variables tab no longer yanks to Design.
CI green.

Concurring with Miga's LGTM.

Concurring with Rames-D's 🟠 Hold:

Existing-id bind silently changes the render. Rames-D's failure trace holds up at head: applyBind at VariablesBindElement.tsx:158 skips the declareVariable call when the id already exists, then wires the element to the existing variable's default (which came from a different element). Concrete case Rames-D named:
1. Element A is red → declares {id: "accent", type: "color", default: "#ff0000"}. A now resolves through var(--accent) → red. ✓
2. Element B is blue → user types "accent" (reusing). Type-compat guard passes (both color). applyBind sees id exists → skips declaration. Applies style="color: var(--accent)" to B. B renders red, not blue. Render just changed silently.
The PR body's "promoting never changes the render" guarantee is truthful only for the fresh-id branch. The existing-id branch is acknowledged as allowed ("Binding to an existing id is allowed only when the types agree") but neither the copy nor the UI warns about the visual change.
Concur on Rames-D's three-option escalation:
1. Detect existing-id at click, show inline confirmation ("This will change {element} to {var}'s current default ({value}) — bind anyway?") — safest, minimal user surprise.
2. Route through setVariableValue(id, element_current_value) on existing-id bind — silently changes OTHER elements bound to the same var; worse trade.
3. Update PR body + panel copy to acknowledge coupling — lowest cost, and the current copy IS misleading.
Notable contrast: #2071's Design-panel promote sidesteps this entirely via uniqueId(action.suggestedId, declarations) — auto-name always fresh. Good split between "auto-name = always fresh" (#2071) and "user-typed = intent respected" (this PR); just need the Variables-tab card to be either honest about the trade-off or safer at click time.

No unique blockers beyond Rames-D's coverage.

R1 by Via

jrusso1020 · 2026-07-09T07:59:36Z

Addressed R1 🟠 Hold (existing-id bind silently changes the render). The Bind card now detects when the typed id already exists and shows an inline amber warning naming the existing default the element will adopt; the button reads "Bind anyway" in that case. The Bind button is disabled on an empty/whitespace id (Miga's nit). The module docstring is corrected: "binding to a NEW id never changes the render; binding to an EXISTING id adopts that variable's default — the card warns first." Code behavior (user-typed id = deliberate reuse) is unchanged, per the #2071 split.

james-russo-rames-d-jusso

R2 at c094ba5.

Prior R1 🟠 ("bind to existing id" silently changes the render — applyBind skips declareVariable when id exists, so element snaps to the existing variable's default from a different element) resolved via a pre-commit warning at VariablesBindElement.tsx:191-218: the form looks up existingDecl by matching trimmedId against the declaration set and, when a hit exists, renders an amber notice text: "{trimmedId}" already exists. This element will use its current value… — user sees the hazard before committing rather than after silently snapping. The applyBind batch behavior itself is unchanged; the warning is at the UI layer where the user still has choice.

No residuals from my side. CI green.

— Review by Rames D Jusso

vanceingalls

🟢 LGTM concurring with Rames-D — R2 at c094ba5.

R1 🟠 hold (existing-id bind silently changes render — element B snaps to element A's color because applyBind skips declareVariable when the id already exists) resolved via option (1) — inline confirmation UI with a pre-commit value preview.

Verified at head:

packages/studio/src/components/panels/VariablesBindElement.tsx:212-215 — existing-id detection fires reactively as the user types: const existingDecl = active ? sdkSession.getVariableDeclarations().find((d) => d.id === trimmedId) : undefined;
Lines 229-236 — amber warning banner renders when existingDecl is truthy: "{trimmedId}" already exists. This element will use its current value ({existingDecl.default}), not the element's own — binding won't change "{trimmedId}". User sees both the coupling AND the concrete value they'll snap to.
Line 249 — commit button label switches to "Bind anyway" when existing-id detected. Explicit acknowledgment gesture.
Component docstring (lines 11-14) documents the coupling directly: "the card warns before committing, since that does change the render."

The concrete failure case I named in R1 (Element A red → declares accent; Element B blue → user types accent → B renders red) is now caught pre-commit with the value preview.

No residuals from my side.

R2 by Via

Ninth PR of the template-variables stack: the promote-a-property gesture. Select an element on the canvas/timeline, open the Variables tab, and the panel offers per-property bind actions. - "Bind selected" card in the Variables panel, built from the selection: image/media source (img/video/audio), text, text color, background, and font. Each action declares a variable whose default is the element's CURRENT value (promoting never changes the render — computed rgb colors convert to hex, the first computed font family becomes the font default) and writes the declarative binding the runtime resolves: data-var-src / data-var-text attributes or `<prop>: var(--id)` styles. Declare + bind run as one batched schema edit (one undo step); binding to an already-declared id skips the declare and just binds. - guarded to selections from the composition the session models — a selection in another source file never writes bindings into this one. - core: extract readVariablesForElement into runtime/variableScope.ts, shared by color grading and the declarative bindings (was duplicated). - fix(studio-server): buildSubCompositionHtml's extractElementAttrs rebuilt html/body attributes without HTML-escaping values, shredding quote-bearing attributes — data-composition-variables (a JSON array) came out as mangled bogus attributes, so getVariables() silently returned {} on every /preview/comp/* page (no declared defaults, no runtime bindings). Pre-existing bug surfaced by live-testing this feature; regression test added. Verified end-to-end in a live session: select headline → Bind text color → declaration + var(--headline-color) written to disk → override in the panel → runtime applies the custom prop and the element renders the override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

vanceingalls

🟢 LGTM at R3 — 1d4b6aa.

R2 hold preserved through the rebase-onto-#2098. R2 verdict stands.

Verified at head — packages/studio/src/components/panels/VariablesBindElement.tsx:

Line 190 — coupling-warning comment preserved.
Line 215-216 — amber banner text ("{trimmedId}" already exists. This element will use its current value...) preserved.
Line 240 — button label swap {existingDecl ? "Bind anyway" : "Bind"} preserved.
Line 154 — batched-schema-edit comment (declare (unless the id already exists) + bind) preserved.

Existing-id pre-commit UI + value preview + "Bind anyway" gesture all survive.

R3 by Via

jrusso1020 mentioned this pull request Jul 8, 2026

feat(core): declarative variable bindings — data-var-src, data-var-text, css custom props #2054

Open

4 tasks

jrusso1020 marked this pull request as ready for review July 8, 2026 06:48

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 605efea to a6b9835 Compare July 8, 2026 08:00

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch 2 times, most recently from 18e9bfc to f734181 Compare July 8, 2026 16:17

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch 2 times, most recently from d0a1444 to f3d95fb Compare July 8, 2026 16:24

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch 2 times, most recently from 5845084 to ea58279 Compare July 8, 2026 16:31

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from f3d95fb to d8f353c Compare July 8, 2026 17:10

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from ea58279 to 8a79d04 Compare July 8, 2026 17:10

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from d8f353c to 14b190e Compare July 8, 2026 18:42

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from 8a79d04 to 6f620b5 Compare July 8, 2026 18:42

jrusso1020 mentioned this pull request Jul 8, 2026

feat(studio): promote to variable from the design panel #2071

Open

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 14b190e to 1169f0a Compare July 8, 2026 21:38

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from 6f620b5 to 064663e Compare July 8, 2026 21:38

This was referenced Jul 8, 2026

feat(sdk,studio): editable template sub-compositions + promote sub-comp element properties #2081

Open

feat(core): aggregate template sub-comp variable defaults from the root div on the render path #2084

Open

miga-heygen reviewed Jul 9, 2026

View reviewed changes

miguel-heygen approved these changes Jul 9, 2026

View reviewed changes

james-russo-rames-d-jusso reviewed Jul 9, 2026

View reviewed changes

vanceingalls reviewed Jul 9, 2026

View reviewed changes

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 1169f0a to 02b028c Compare July 9, 2026 06:49

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from 064663e to 1f4e4c2 Compare July 9, 2026 06:49

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 02b028c to 9e9a12f Compare July 9, 2026 07:57

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from 1f4e4c2 to 1705bdd Compare July 9, 2026 07:57

jrusso1020 mentioned this pull request Jul 9, 2026

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

Closed

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 9e9a12f to 82fc1c4 Compare July 9, 2026 13:17

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch 2 times, most recently from 59bc240 to c094ba5 Compare July 9, 2026 18:06

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 82fc1c4 to bea0c7a Compare July 9, 2026 18:06

james-russo-rames-d-jusso reviewed Jul 9, 2026

View reviewed changes

vanceingalls reviewed Jul 9, 2026

View reviewed changes

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from bea0c7a to 2780e4b Compare July 9, 2026 19:34

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from c094ba5 to 1d4b6aa Compare July 9, 2026 19:34

jrusso1020 force-pushed the 07-07-feat_studio_bind_selected_element_properties_to_variables branch from 1d4b6aa to 5e1670d Compare July 9, 2026 19:46

jrusso1020 force-pushed the 07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_props branch from 2780e4b to 3b0a51d Compare July 9, 2026 19:46

vanceingalls reviewed Jul 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(studio): bind selected element properties to variables#2055

feat(studio): bind selected element properties to variables#2055
jrusso1020 wants to merge 1 commit into
07-07-feat_core_declarative_variable_bindings_data-var-src_data-var-text_css_custom_propsfrom
07-07-feat_studio_bind_selected_element_properties_to_variables

jrusso1020 commented Jul 8, 2026 •

edited

Loading

Uh oh!

jrusso1020 commented Jul 8, 2026 •

edited

Loading

Uh oh!

miga-heygen left a comment

Uh oh!

miguel-heygen left a comment

Uh oh!

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

Uh oh!

vanceingalls left a comment

Uh oh!

jrusso1020 commented Jul 9, 2026

Uh oh!

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

Uh oh!

vanceingalls left a comment

Uh oh!

vanceingalls left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

jrusso1020 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Test plan

Uh oh!

jrusso1020 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miga-heygen left a comment

Choose a reason for hiding this comment

Uh oh!

miguel-heygen left a comment

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

🟠 Hold

🟡 Nits / questions

🟢 Good

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

Uh oh!

jrusso1020 commented Jul 9, 2026

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jrusso1020 commented Jul 8, 2026 •

edited

Loading

jrusso1020 commented Jul 8, 2026 •

edited

Loading