feat(core): aggregate template sub-comp variable defaults from the root div on the render path#2084
Conversation
jrusso1020
force-pushed
the
07-08-feat_subcomp_variable_render_path
branch
from
2c63546 to
bd7187d
Compare
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
from
9588b6d to
223de02
Compare
jrusso1020
force-pushed
the
07-08-feat_subcomp_variable_render_path
branch
from
bd7187d to
778b6cd
Compare
jrusso1020
force-pushed
the
07-08-feat_editable_template_subcomps_and_subcomp_promote
branch
from
223de02 to
a8972ad
Compare
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM — minimal, correct, well-tested.
The one-line addition reads variable defaults from the inner composition root div (where #2081 stores template sub-comp declarations) before applying host overrides. Spread order is correct: documentElement defaults → innerRoot defaults → host data-variable-values. The innerRoot guard prevents NPE for full-document sub-comps. Both test cases (default aggregation + per-instance override) cover the exact merge behavior.
The .gitignore broadening and fallow-ignore-next-line suppression are both justified. Remaining gaps (CLI --variables validation, runtime compositionLoader) are documented and off the render path this fixes.
— 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 778b6cd. Stack context: render-path lane (#2049 / #2054 / #2084) — 3-of-12 in template-variables stack.
🟢 What's good
- Right fix in the right place. The bundler's aggregation path (
inlineSubCompositions.ts:244-253) was the divergence point named in #2081's deferred-follow-up section —readVariableDefaults(compDoc.documentElement)only, missing the root-div declarations that template comps use. Adding the secondreadVariableDefaults(innerRoot)read closes the gap that made #2054'sdata-var-*bindings on template sub-comps silently render authored fallbacks instead of declared defaults. - Precedence order lands correctly: html defaults < inner-root defaults < per-instance host
data-variable-values. Tested both directions — default aggregation at line 51-61 and host-override-wins at line 63-74. Sub-comp isolation viavariablesByComp[runtimeCompId]is unchanged. .gitignorebroadening from.hyperframes/backup/to.hyperframes/is a good hygiene fix — the PR body notes preview-generated state leaked into a fixture commit, so widening the ignore prevents recurrence. The inline comment explaining "always runtime-generated, never committed" is exactly the docs-in-the-diff pattern this repo rewards.
🟠 Hold — deferred readers create a partial-fix gap
The PR body flags this honestly: two secondary readers still read data-composition-variables from <html> only. I want to make sure the render-path claim in the title is calibrated to what's actually fixed vs. deferred.
Fixed by this PR (bundler + producer inline path):
inlineSubCompositions— the flattener that the bundler AND producer use → covershyperframes bundleoutput and the standard render.
Still broken (per PR body):
packages/core/src/runtime/compositionLoader.ts:701—readDeclaredDefaults(doc.documentElement)for external lazy-loaded sub-comps. If a runtime consumer loads a template comp without going through the bundler/producer inline path (i.e. lazy composition load in the studio preview), root-div defaults are still dropped and per-instance overrides won't have a base to layer on.extractCompositionMetadata— CLI--variablesvalidation only when rendering a sub-comp file standalone.
For today's studio-preview + producer-render primary paths this PR is sufficient. But if any preview flow uses the runtime lazy loader (loadExternalCompositions path in init.ts) for a template comp, #2054's declarative bindings inside that sub-comp will still see the un-defaulted state. Worth linking to a follow-up ticket rather than leaving this as prose-only in the PR body — the "Remaining (minor, documented)" section will get lost once this merges.
🟡 Nits / questions
- Behavior change on full-doc sub-comps that declare defaults on BOTH
<html>AND the root div: pre-#2084, only<html>defaults applied. Post-#2084, root-div defaults override<html>defaults for the same id. This is almost certainly a no-op in practice (nobody double-declares), but it IS a semantics change on an existing path — worth one line in the PR body noting "no known template does this, but the merge order now favors the root-div reading" so a future bisector can spot it. readVariableDefaults(innerRoot)whereinnerRootis the FIRST matching[data-composition-id]in the parsed content. If a template happens to have nested[data-composition-id]elements (unusual but not forbidden — see the sub-composition-video regression shard), the outermost wins. Sensible default; worth a code comment oninnerRoot's resolution.// fallow-ignore-next-line complexity— accepted as the PR body says the 262-line function is otherwise untouched and refactoring is out of scope. Second-time the ignore has been added to this function in the stack — accumulating ignores is a smell but not this PR's problem to fix. Consider a/// TODO: extractmarker so a future refactor pass can find it.- The
.gitignorebroadening is unrelated to the aggregation fix. Not blocking; but stack-order reasoning gets easier if unrelated fixes ride separate PRs. Optional. - No new lint rule for "declared on root div in a full-document comp": if an author puts
data-composition-variableson the root div of a file that also has<html data-composition-variables>, they now silently get root-div-wins semantics. Avariable_declaration_on_both_root_div_and_htmlwarning would prevent surprise. Low priority.
🔴 Blockers
None. Small, well-scoped, tests cover both directions, and the deferred readers are documented.
CI: green modulo the pre-existing Perf: matrix.shard template-eval failure (real perf shards all pass). One regression shard pending — unrelated to this diff.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2084 at 778b6cd6
🟢 LGTM concurring with Miga; concur with Rames-D's 🟠 partial-fix framing — worth linking a follow-up ticket rather than leaving it as prose-only PR-body text.
Verified independently:
- Single point of mutation at
inlineSubCompositions.ts:~240— newreadVariableDefaults(innerRoot)line betweendocumentElementand per-instance host override. Spread order = precedence: html defaults < root-div defaults < instance override. Correct. - Why root div, not
<html>. Template sub-comps have a synthetic<html>stripped on serialize; the durable declaration holder is the[data-composition-id]root div, matching #2081'sdeclarationElement()write path. - Precedence test. "Lets a per-instance host value override the declared default" confirms host
data-variable-valuesbeats both html and root-div defaults. - Multi-inline handling. N hosts of the same sub-comp →
variablesByComp[runtimeCompId]overwritten each iteration, but defaults are composition-level (identical) so no semantic divergence. Instance-override values remain per-host. - Idempotency at bundle time. Aggregation runs once per host in the
for (const host of hosts)loop. No re-entry risk. - Fallow suppression narrow.
// fallow-ignore-next-line complexityonly on the new line; the 262-line function is otherwise untouched. .gitignorewidening from.hyperframes/backup/→.hyperframes/— the inline "runtime-generated, never committed" comment is the docs-in-the-diff pattern.- CI green (Perf: matrix.shard template-eval red is pre-existing, unrelated).
Concurring with Miga's LGTM.
Concurring with Rames-D's 🟠 Hold on the partial-fix gap:
compositionLoader.ts:701(readDeclaredDefaults(doc.documentElement)on external lazy-loaded sub-comps) — still reads only from<html>. If a preview or runtime flow ever loads a template comp vialoadExternalCompositionsrather than the bundler-inliner path, #2054's declarative bindings inside that sub-comp will see un-defaulted state. For today's studio-preview + producer-render primary paths this PR is sufficient; the deferred reader is a real gap that should be tracked outside the PR body so it isn't lost after merge.extractCompositionMetadata(CLI--variablesvalidation on standalone sub-comp render) — same class of gap.
Would suggest a Linear/GitHub issue linked from the PR body rather than "Remaining (minor, documented)" prose.
Concurring with Rames-D's 🟡 nits — all reasonable, none blocking:
- Behavior change on full-doc sub-comps that declare on BOTH
<html>AND root div: post-#2084, root-div wins. Almost certainly no-op in practice, but worth one line in the PR body for bisectability. innerRoot= first matching[data-composition-id]— sensible default; worth a code comment on the outermost-wins resolution.- Repeat
fallow-ignore-next-line complexityon the same function is an accumulation smell; TODO marker for a future refactor pass. - The
.gitignorebroadening is unrelated to the aggregation fix — separate-PR-ideal for stack-order reasoning, not blocking.
No unique blockers beyond Rames-D's coverage.
R1 by Via
jrusso1020
force-pushed
the
07-08-feat_subcomp_variable_render_path
branch
from
778b6cd to
bf92f3b
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_subcomp_variable_render_path
branch
from
bf92f3b to
bdc5a8d
Compare
|
Addressed R1 🟠 Hold (deferred readers). Rather than leave the gap prose-only, I added
A Linear follow-up ticket is still worth filing — flagging to @james-russo-rames-d-jusso to confirm the owning team (HyperFrames isn't its own Linear team) before I create it. |
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_subcomp_variable_render_path
branch
from
bdc5a8d to
16a797d
Compare
5 participants
What
Follow-up to #2081: makes a template sub-composition's variable declarations (which #2081 stores on the composition root div) reach the render path. The bundler/producer inliner aggregated each sub-comp's variable defaults by reading only its
<html>— which template comps don't have — so a promoted variable's default (and any per-instance override) was dropped at render, diverging preview from render.Change
inlineSubCompositionsvariable-defaults aggregation now readsreadVariableDefaults(compDoc.documentElement)ANDreadVariableDefaults(innerRoot)(the[data-composition-id]root div), before applying the per-instance hostdata-variable-valuesoverride. Values land invariablesByCompbefore any innerHTML step, so they survive both the bundler (flatten) and producer (named-host) inline paths.// fallow-ignore-next-line complexity) — the function is otherwise untouched; refactoring it is out of scope.Verified
New tests in
inlineSubCompositions.test.ts: a default declared on a template sub-comp's root div is aggregated intovariablesByComp["card"], and a per-instancedata-variable-valuesoverride wins. Core suite green (1137).Remaining (minor, documented)
Two secondary readers still read declarations from
<html>only, both off the bundle/producer render path this fixes:extractCompositionMetadata→ CLI--variablesvalidation (only when rendering a sub-comp file standalone).compositionLoaderexternal-load (runtime-loaded, non-bundled sub-comps).Stacked on #2081.