feat(sdk,studio): editable template sub-compositions + promote sub-comp element properties#2081
Conversation
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
2 times, most recently
from
223de02 to
a8972ad
Compare
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM with nits — the declarationElement SSOT refactor is textbook-clean. Every call site that previously hardcoded document.documentElement or gated on parsed.wrapped now routes through the single resolver. The variableModel.ts signature change from Document to Element | null pushes resolution responsibility to the caller, keeping the model pure.
Stale lint fix hint (packages/lint/src/rules/composition.ts:671): the fixHint for unknown_variable_binding still says "Declare the variable on <html>: ...". After this PR, template sub-comps declare on the composition root div. The hint should read something like "Declare the variable in data-composition-variables on the composition root element" to avoid misleading sub-comp authors.
Sequential file reads (useProjectCompositionVariables.ts): readGroup calls are awaited in a for loop. For projects with many sub-comp HTML files, Promise.all would parallelize the reads. Non-blocking performance note.
Positive call-outs:
isCompositionTemplatecorrectly distinguishes Form A (data-composition-idon<template>) from Form B (on the wrapped root div) while excluding plain clone-source templatesfindRootfallback ordering is correct and doesn't break the existing resolution chainfragmentCompositionErrinversion from "refuse wrapped" to "refuse when no declaration element" correctly allows template sub-comps while guarding bare rootless fragments
— Miga
miguel-heygen
left a comment
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
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at a8972ad. Stack context: integration cap #2081 — sits atop the 12-PR template-variables stack, composes with #2047 (edit ops), #2050 (inspector), #2054 (declarative bindings), #2071 (promote), #2084 (sub-comp defaults). Layering onto Miga's review; concerns below are cross-PR seams they didn't cover.
🔴 Blockers
None. The SDK refactor is clean, tests are focused, CI is green.
🟠 Hold — worth clarifying before land
1. data-var-* bindings + per-instance overrides on promoted sub-comp variables silently no-op at render UNTIL #2084 lands. The PR body flags this as a "deferred follow-up" and is correct that the default render is unchanged (element retains its inline value). But the composition seam is subtler than "affects only overriding":
- CSS bindings work:
htmlBundler.ts:1113scans[data-composition-variables]broadly, so declarer rules from a root-div declaration DO get emitted (verified at HEAD). data-var-*bindings and host overrides don't:inlineSubCompositions.ts:242readsreadVariableDefaults(compDoc.documentElement)only, sovariablesByComp[runtimeCompId]is empty for template sub-comps → runtimedata-var-text/data-var-srcresolution on inlined sub-comp elements can't find the id, and hostdata-variable-valueshas nothing to override.
The screenshot flow in the PR body — promoting .timer text inside frames/02-problem.html and writing data-var-text — is exactly the case that would preview-fine but render-empty if 2081 landed alone. Since 2084 stacks directly on this PR, that's usually fine in a batch merge; the ask is don't land 2081 without 2084 in the same batch, and consider tightening the PR body from "affects overriding at render time" to "affects data-var-* binding resolution and overrides at render time — CSS-bound sub-comp vars still work."
2. Public docs stale, not touched by a dual-scope SDK+studio PR. The PR body claims dual scope but no docs/* files changed. Two spots that now teach the wrong contract:
docs/concepts/variables.mdx:10— "Adddata-composition-variablesto the<html>root of any composition." No longer true for template sub-comps.docs/concepts/variables.mdx:293— the precedence table's "Declared defaults" row still saysdata-composition-variables on <html>.docs/sdk/reference/composition.mdx:127—declareVariable's error listing still saysE_FRAGMENT_COMPOSITION — fragment sources have no <html> to carry the schema. After this PR, a fragment WITH a root element accepts declarations; the error only fires when there's genuinely no root.
Suggest a short section in variables.mdx — "Where declarations live" — noting the two carriers (<html> for full-document comps, composition root div for wrapped template/fragment comps), and updating the error listing.
3. E_FRAGMENT_COMPOSITION code kept but semantics narrowed. The error code name still says "fragment"; the code now fires only when there's no root element at all (a bare fragment). Any external caller matching on the code to say "add an <html>" will now give bad advice — the fix in the new world is "add a composition root element." Two options: rename to E_NO_ROOT_ELEMENT (breaking, do in a follow-up), or at least document the semantic shift in the composition reference. Same message string suggests the latter is fine for now (packages/sdk/src/engine/mutate.ts:930).
🟡 Nits and questions
Symmetry with #2071 — integration test gap for the sub-comp path. variablePromoteIntegration.test.ts still uses a full-document composition (<!DOCTYPE html><html><body>…) — round-trips applyBind → readBindingFrom. That covers the top-level flow (unchanged). What's not covered is the new sub-comp targeting: given a selection whose sourceFile differs from activeCompPath, verify the promote lands in sourceFile and not the host. A one-shot integration test that opens two files (host + sub-comp), simulates a selection.sourceFile = "sub.html" promote, and asserts the declaration ends up in sub.html's bytes (and the host's untouched) would close the SDK↔Studio wiring seam this PR introduces. The SDK-side tests (session.template.test.ts) prove the sub-comp declare/serialize round-trip; the Studio-side glue is where the wiring lives.
Two SDK sessions on the same file when targetPath === activeCompPath. With #2071's shape, DesignPanelPromoteProvider reused the top-level sdkSession prop. Now it opens its own via useSdkSession(projectId, targetPath, …). When no sub-comp element is selected, targetPath falls back to activeCompPath — so the promote provider opens a second linkedom document on the same file. Both sessions receive the file-change event on writes and both trigger openComposition reloads. Not a correctness bug (isSelfWriteEcho is content-identity based), but memory + reload cost doubles on the active-comp path. Was that an intentional trade for the simpler prop shape, or worth a useMemo fast path when targetPath === activeCompPath to reuse the caller's session?
useProjectCompositionVariables — deps include an unstable fileTree reference. The useEffect deps [fileTree, excludePath, readProjectFile, refreshKey] will re-fire on every render if useFileManagerContext() returns a fresh fileTree array reference (which context providers often do without deep memoization). The eslint-disable on the line above hints something was intentionally waved through — worth a comment explaining why, or a .join("|") derived key. Combined with Miga's serial-vs-parallel note: on a rapidly-refreshing panel with a mid-sized project, sequential openComposition per html file per render is O(n) I/O per render.
readGroup swallows all openComposition errors silently. Fine for panel resilience, but a real corruption bug (persist wrote invalid HTML) would just make a variable quietly vanish from the "Other compositions" section with no signal. Consider console.warn/telemetry on the catch branch — this is the exact class of failure the studio's observability wants to catch.
Undo-history label ambiguity. useEditVariablesInFile records "Update variable \"title-color\"" — no file path. Two sub-comps with the same variable id would produce indistinguishable history entries. Consider suffixing with the file basename (Update variable "title-color" (frames/02-problem.html)).
Substring guard + regex in the lint rule both match tokens in HTML comments. useProjectCompositionVariables.ts:24 and lint/composition.ts collectAllDeclaredVariableIds's <[a-zA-Z][^>]*\bdata-composition-variables\b[^>]*> — an occurrence inside <!-- --> would over-declare, but that only reduces false-positive lint findings and includes extra scanning targets. Safe direction, worth a code comment noting the intentional imprecision.
isCompositionTemplate child-scan is direct-children-only. for (const child of Array.from(el.children)) — the wrapped-root-div form is direct-child in the current scaffold, so this matches the shape. If a future scaffold nests the [data-composition-id] root under a wrapper <div> or <section>, this predicate silently regresses to "not a comp template" and enumeration/mutation stop reaching those elements. Docstring pins the current contract; keep an eye on it if scaffold changes.
🟢 What's good
declarationElement(document, wrapped)is the exact SSOT this refactor needed — everyparsed.wrappedguard and hardcodeddocumentElementnow routes through the resolver, andvariableModel.tssignature moved toElement | nullpushing resolution to the caller. Textbook.- New tests in
session.template.test.tsare the right differentials: enumeration, round-trip through serialize (proves the root-div carrier survives), and the clone-source exclusion. - The updated
session.variabledecls.test.tssplits "fragment with root (accepted)" from "fragment with no root (refused)" — the exact new contract. - Backward-compat argument in the PR body is sound: default = current value, so existing sub-comp render is unchanged at bit level; the extension is purely capability-expansion.
findRoot's newquerySelectorAllDeep(document, "[data-composition-id]")[0]fallback keeps the resolution chain ordered so full-document comps hit the earlier branches; template comps hit the new one. Clean.- Very high-quality PR body: explicitly enumerates the deferred readers with file:line anchors, documents the current-value default rationale, and stacks the render-path fix as its own PR (#2084).
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2081 at a8972ad1
🟢 LGTM on the SSOT refactor, concurring with Miga; concur with Rames-D's three 🟠 Holds — none block, but all worth landing before this becomes a stable public-API contract.
Verified independently:
- Template recognition refined.
isCompositionTemplateatpackages/parsers/src/hfIds.tsnow recognizes both<template data-composition-id>and<template id="X-template"><div data-composition-id>…. Plain clone-source templates correctly excluded. declarationElement(document, wrapped)is textbook SSOT. Full-doc comps targetdocumentElement; wrapped templates/fragments callfindRoot()→[data-composition-id]root div (the durable declaration holder). Round-trip test insession.template.test.tslocks in serialize preservation.- Model / patch / mutate paths. Every mutate/patch/
can()route throughdeclarationElement(), not hardcoded<html>. Grep-verified. - Lint rule expansion.
unknown_variable_bindingincomposition.tsunions declared ids viacollectAllDeclaredVariableIds()— a promoteddata-var-*on a template-comp element no longer false-flagged. - Studio promote/bind retargeting.
DesignPanelPromoteProvider.tsxopens the SDK session onselection?.sourceFile || activeCompPath || "index.html". Reusability invariant: a promoted prop on a sub-comp element is visible to any other parent that uses the same sub-comp. - Reusability inspector.
VariablesOtherCompositions+useProjectCompositionVariableslist per-file declarations viaopenComposition(...).getVariableDeclarations()across the file tree; substring guard skips clean files before full parse. - -123 lines = deletion of legacy "fragments refuse declarations" code path (updated to new contract).
- CI green including preview-regression + player-perf.
Concurring with Miga's LGTM.
Concurring with Rames-D's 🟠 Holds:
-
data-var-*bindings + host overrides silently no-op at render until #2084 lands. Rames-D's cross-PR analysis is right and worth reproducing here:htmlBundler.ts:1113scans[data-composition-variables]broadly so CSS binding rules ARE emitted, butinlineSubCompositions.ts:242readsreadVariableDefaults(compDoc.documentElement)only — sovariablesByComp[runtimeCompId]is empty for template sub-comps and runtimedata-var-*resolution on inlined sub-comp elements can't find the id. The screenshot flow in the PR body (promote.timerinsideframes/02-problem.htmland writedata-var-text) is exactly the case that would preview-fine but render-empty if #2081 landed alone. Ask: don't land #2081 without #2084 in the same batch, and tighten the PR body's "affects overriding at render time" phrasing to "affectsdata-var-*binding resolution AND overrides at render time — CSS-bound sub-comp vars still work." -
Docs stale.
docs/concepts/variables.mdx:10("Adddata-composition-variablesto the<html>root of any composition"),:293(precedence table's "Declared defaults" row), anddocs/sdk/reference/composition.mdx:127(declareVariable's error listing still cites<html>as required carrier). PR body claims dual SDK+studio scope but nodocs/*changed. Concur on a short "Where declarations live" section covering the two carriers plus the error-listing update. -
E_FRAGMENT_COMPOSITIONcode name/semantics drift. Code fires only when there's no root element at all now, but the identifier still says "fragment." An external caller matching on the code to say "add an<html>" now gives bad advice — new-world fix is "add a composition root element." Concur: either rename toE_NO_ROOT_ELEMENT(breaking, do in follow-up) or document the semantic shift.
Concurring with Rames-D's nits — I'll add one small independent piece and defer to his coverage on the others:
O1 — Two SDK sessions on the same file when targetPath === activeCompPath. Rames-D flagged this and it's real: DesignPanelPromoteProvider opens a second linkedom document via useSdkSession on the same file when no sub-comp element is selected. Not a correctness bug (isSelfWriteEcho is content-identity based), but memory + reload cost doubles on the hot path. A useMemo fast-path when targetPath === activeCompPath (reuse the caller's session) would eliminate the doubling without complicating the sub-comp path. Small; keep the change surface tight.
O2 — Envelope-clean. Verified: no Co-Authored-By: Claude in commits, no 🤖 Generated with [Claude Code] in PR body. Consistent with #2071.
R1 by Via — concur with all three 🟠 Holds; ordering constraint on #2081/#2084 batch merge is the load-bearing takeaway.
jrusso1020
force-pushed
the
07-08-feat_studio_promote_to_variable_from_the_design_panel
branch
from
f5d8df9 to
842294f
Compare
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
from
a8972ad to
cf726f6
Compare
jrusso1020
force-pushed
the
07-08-feat_studio_promote_to_variable_from_the_design_panel
branch
from
842294f to
8cc6c81
Compare
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
from
cf726f6 to
5ed4778
Compare
|
Addressed R1 🟠 Holds (stale docs +
|
…mp element properties
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
from
5ed4778 to
816a860
Compare
jrusso1020
force-pushed
the
07-08-feat_studio_promote_to_variable_from_the_design_panel
branch
from
8cc6c81 to
738398a
Compare
|
Follow-up from the #2101 stack review (Miguel): updated the two spots still teaching |
5 participants
What
Makes
<template>-based sub-compositions first-class editable in the SDK, and retargets the Design-panel promote/bind so it operates on the sub-comp's own file — so you can promote a property of an element inside an inlined sub-comp and it becomes a variable on that reusable frame.Fixes the gap found while live-testing #2071: promote (and the shipped #2055 "Bind selected" card) silently did nothing on sub-composition elements, because the SDK didn't model template comps (
getElements()returned0) and refused variable declarations on fragments (E_FRAGMENT_COMPOSITION).SDK — template comps as first-class editable
isCompositionTemplatenow recognizes the authored form<template id="X-template"><div data-composition-id>…(id on the wrapped root div), not onlydata-composition-idon the template element. This one predicate feeds hf-id minting and element enumeration; mirrored inquerySelectorAllDeepso dispatch/resolution descend into the same templates. Plain clone-source templates (list items, particles) are still excluded (guarded by "wraps a[data-composition-id]root").findRootdescends into a composition template to return the inner[data-composition-id]root (so value/metadata writes don't land on the<template>shell).<html>to holddata-composition-variables, and the synthetic one is stripped on serialize. A singledeclarationElement(document, wrapped)resolves<html>(full-doc) vs the root div (wrapped); the declaration model + every mutate / patch-replay /can()path routes through it. Declaring is now refused only when there is genuinely no root element.Studio
DesignPanelPromoteProvideropens the SDK session and persists againstselection.sourceFile(the sub-comp file), not the hostactiveCompPath— so a promoted variable's declaration + binding land in the frame's own file, applying to every instance.Lint
unknown_variable_bindingrule now unions declared ids from the root div too (not just<html>), so a promoteddata-var-*on a template comp isn't false-flagged as undeclared.Verified
.timerinsideframes/02-problem.htmlnow shows the◇ varpill; clicking it writesdata-composition-variables+data-var-textinto the frame's own file and flips to the◆chip.Deferred follow-up — producer render-path
The producer/inliner variable aggregation (
inlineSubCompositionsreads defaults fromdocumentElementonly) and a few secondary readers (extractCompositionMetadata→ CLI--variables, runtime external-loadcompositionLoader, named-hostinnerHTMLroot-div preservation) still read declarations from<html>only. This only affects overriding a template-sub-comp variable at render time — the promote default equals the element's current value, so default render is unchanged. Deferred because it touches a 262-line critical-complexity inliner function and warrants an actual-render verification pass. Tracked with file:line anchors from the change map.Stacked on #2071.
Variables tab — all variables, grouped by composition
Because promote writes a variable into the sub-comp's own file, that variable wouldn't show in the host-scoped Variables tab (reported while testing). Fixed: the Variables tab now lists the active composition's variables plus an "Other compositions" group per sub-comp file that declares any, each with edit / remove (
VariablesOtherCompositions+useProjectCompositionVariables, reading declarations viaopenComposition(...).getVariableDeclarations()per file, editing via an ad-hoc per-file persist). So the panel reflects the whole template's variable surface.Live-preview override of a sub-comp variable is the one deferred piece (values are per-composition-scope, tied to #2070's per-instance routing) — see + manage works now; host-level preview is unchanged.