fix(studio): handle full HTML doc sub-compositions in preview#885
Conversation
buildSubCompositionHtml previously assumed all sub-compositions used <template> wrappers. Full HTML document compositions (like the north-korea-locked-down and nyc-paris-flight registry blocks) were nested as-is inside <body>, producing invalid HTML with nested <html> and <head> elements. This caused <style> and <script> tags from the composition's <head> to be misplaced, and asset paths to fail when combined with the injected <base> tag. Now detects full HTML documents and extracts head styles/scripts and body content separately, producing clean valid HTML where the composition's CSS lands in <head> and asset-relative paths resolve correctly via the <base> tag.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM — clean three-way dispatch addressing a real-bug class.
Audited
Root cause is precise: the old templateMatch?.[1] ?? rawComp fallback silently took the rawComp branch for any composition that didn't have a <template> wrapper. For full HTML docs, that meant the entire <!doctype html><html>...</html> got dumped inside <body>${content}</body>, producing invalid nested HTML. The composition's <head> styles + scripts (GSAP CDN, custom CSS) were left inside that nested-<head>-inside-<body> mess where browsers ignore them. ✓
Fix shape — three-way dispatch:
- Template-wrapped → extract template content + rewrite (existing behavior, refactored into the same DOM-rewrite block)
- Full HTML document → new
extractFullDocumentPartsextracts head styles/scripts and body content SEPARATELY via linkedom, rewrites assets across bothdoc.headanddoc.body, returns{ headStyles, headScripts, bodyContent } - Raw fragment (no template, no full doc) → parse as
<body>${rawComp}</body>and rewrite (new explicit branch)
The dispatch order (template-first → full-doc → fragment) is correct: a template wrapper takes precedence over its inner content shape, and only no-template files fall through to the doc-detection branch. ✓
isFullHtmlDocument predicate:
return /<!doctype\s|<html[\s>]/i.test(html);Catches <!DOCTYPE html> / <!doctype HTML> and <html> / <html lang="en">. Case-insensitive. Minor: pattern matches anywhere in the string, so a comp file with a commented-out <!-- <html> reference --> would false-positive into the full-doc branch. Practically benign — linkedom would still parse the comment-only HTML correctly and doc.body.innerHTML would return the actual content — but worth a one-line ^\s* anchor if you want to be strict.
extractFullDocumentParts:
- Iterates both
doc.headANDdoc.bodyfor asset rewrites ✓ (head can have<link href="...">references too) headStylesandheadScriptsextracted from<head>only — composition's body scripts stay in the body content where they belong ✓- Returns clean separated parts that the caller assembles into the final standalone page
Composition's head deps appended to assembled head:
if (compHeadStyles) headContent += `\n${compHeadStyles}`;
if (compHeadScripts) headContent += `\n${compHeadScripts}`;Both appended AFTER the index.html head content, so the composition's deps load last — if there's a conflict with index.html (e.g., both load different GSAP versions), the composition wins. Reasonable default for the "sub-comp declared its own deps" case the body cites.
Test coverage
New test pins the full-doc branch with 6 assertions:
- No
<html>or<head>AFTER<body>(verifies the nesting bug is fixed) - Composition styles preserved (
.map,#root) - Image src preserved (no over-rewriting bare relative paths)
<base href="...">for asset resolution- GSAP CDN script preserved
- Body script
__timelines["map-block"]preserved
Existing tests still cover the template-wrapped branch (+52 -0 is net-additive). ✓
Body claim verification
- "
buildSubCompositionHtmlassumed all sub-compositions used<template>wrappers" — verified, the old code'stemplateMatch?.[1] ?? rawCompis exactly that assumption ✓ - "Full HTML document blocks were nested as-is inside
<body>, producing invalid HTML with nested<html>and<head>elements" — verified by reading the old code path ✓ - "north-korea-locked-down and nyc-paris-flight" — concrete repro names; test fixture mirrors that shape via a
map-block.html✓
Non-blocking notes
-
Duplicate rewrite block in template-wrapped + raw-fragment branches (
subComposition.ts:79-107and120-150) — sameparseHTML + rewriteAssetPaths + rewriteInlineStyleAssetUrls + rewriteCssAssetUrlssequence in both. Could DRY into a helperrewriteFragmentContent(content, compPath) → string. Cosmetic. -
isFullHtmlDocumentregex matches anywhere —^\s*anchor would be strict-mode for the unlikely commented-out-<html>false-positive. Practical impact: nil. -
No test for the raw-fragment branch — the third path (no template, no full doc) is exercised by some pre-existing call site presumably, but isn't pinned by this commit's test additions. A short test for
compositions/fragment.htmlcontaining<div data-composition-id="x">...</div>(no doctype, no template) would round out the matrix. -
Comp's
<script>last-write-wins — appending after index.html's head means a comp that loads GSAP 3.14 will override an index.html GSAP 3.12. Body framing makes this sound intentional; worth a sentence in the docstring ofextractFullDocumentPartsso future readers know the precedence is deliberate.
CI
All required green or in-progress on cc9c7eb0. The earlier run shows Build, Test: runtime contract, Lint, Format, Preflight, Tests on windows-latest, Render on windows-latest, player-perf, Perf:fps all green; current run has Test, Typecheck, CLI smoke (required), Smoke: global install in-progress + regression-shards in-progress. No failures.
— Review by Rames Jusso (pr-review)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Summary. Targeted fix for full-HTML-document sub-compositions in the Studio preview: detect doctype/<html>, parse via linkedom, extract head <style>/<script> + body content separately, append to the project's head. Avoids the nested <html>/<head>-in-<body> bug. Scope is small (175/-26 across one .ts + one .test.ts), checks are green except two Windows jobs still in flight at the head sha.
Calibrated strengths.
subComposition.ts:10-15— extraction intoisFullHtmlDocument+extractFullDocumentPartskeeps the new branch isolated and readable, and the rewrite pass is correctly applied to bothdoc.headanddoc.body(subComposition.ts:25-43), so head-scoped<style>url(..)references get rewritten too — not just body assets.- Test pins the right negative invariant:
expect(afterBody).not.toContain("<html")andnot.toContain("<head>")(subComposition.test.ts:55-58), which is the actual regression class — not just "the maps render." - The three branches (
<template>/ full-doc / raw fragment) are mutually exclusive and ordered correctly:<template>first (so a future composition with a<template>wrapper containing a<html>reference inside doesn't get misclassified as full-doc), then full-doc, then raw-fragment fallback.
Findings.
important — composition <head> <link> / <meta> are silently dropped.
extractFullDocumentParts extracts only <style> and <script> from doc.head (subComposition.ts:38-44). Any <link rel="stylesheet" href="...">, <link rel="preload" ...>, or <meta> in the sub-comp's head gets lost when the doc is reassembled. The two blocks named in the PR body use inline <style> only, so the fix works for them — but the next composition someone adds with an external stylesheet will render unstyled with no error. Either widen the extraction to include link + meta (cheap — same outerHTML + join pattern), or add an explicit "unsupported head element" warning so the failure mode is loud, not silent. At minimum, document the assumption in the function jsdoc so the next author knows the contract.
nit — three near-identical rewrite blocks.
subComposition.ts:88-105, subComposition.ts:117-136, and the head/body loop inside extractFullDocumentParts (subComposition.ts:25-43) all do the same rewriteAssetPaths + rewriteInlineStyleAssetUrls + rewriteCssAssetUrls triad over a linkedom doc fragment. Extracting a single rewritePathsOnContainer(container, compPath) helper would let the three branches each be a 1-liner and would prevent future drift between them. Not blocking — file is small — but the duplication will accumulate the next time someone touches this.
nit — <html> / <body> attributes lost.
doc.body.innerHTML discards anything on the <html> element (e.g. lang="en") and the <body> element itself (class, data-*, inline style). The two known blocks don't use these, but worth a comment in the function so the constraint is explicit.
nit — bare-relative paths in nested compositions.
Pre-existing behavior, not introduced here, but the PR body claims "relative asset paths resolve correctly" — which is true only because <base> points at the project root AND the two blocks live at compositions/<name>.html (one level deep) with assets at project-root assets/. A composition at compositions/sub/foo.html referencing assets/map.png (meaning relative to its own dir) would still 404, since rewriteAssetPaths only rewrites ../ paths. Worth a follow-up if more nesting comes online.
Notes (non-blocking).
- Verified the only consumer is
studio-api/routes/preview.ts:215(preview-time, trusted-content path) — so hoisting composition<script>tags into the main page's<head>doesn't widen any attack surface. - CI:
mergeStateStatus=BLOCKEDisREVIEW_REQUIRED(reviewer-gate), not a CI-gate; required checks all SUCCESS at the head sha except Windows render/tests still IN_PROGRESS at review time.
Verdict: APPROVE
Reasoning: The fix is correct and tested for the two compositions named in the body, and the change is well-isolated. The dropped <link>/<meta> is the one substantive gap, but it's a "next composition" concern with a loud failure mode (missing styles) rather than a silent correctness regression — fine as a follow-up.
Review by Vai
…ward attrs Addresses review notes from hf#885: - Extract full <head> innerHTML instead of only <style>+<script>. This preserves <link rel="stylesheet">, <meta>, and any other head-level tags. Prevents silent-degrade when a composition ships external CSS. - Extract and forward <html> and <body> attributes (lang, class, data-*) to the assembled output page. - DRY the triplicated rewrite block into rewriteRelativePaths() shared across template, full-doc, and fragment branches. - Anchor isFullHtmlDocument regex with ^\s* to avoid false positives from stray <html> inside script/template content. - Add fragment-branch test (raw HTML without template or doctype). - Document the deliberate last-write-wins precedence for composition scripts appended after the project head.
Merge activity
|
3 participants
Summary
buildSubCompositionHtmlassumed all sub-compositions used<template>wrappers. Full HTML document blocks (likenorth-korea-locked-downandnyc-paris-flight) were nested as-is inside<body>, producing invalid HTML with nested<html>and<head>elements<style>tags ended up misplaced inside<body>, and<img src="assets/...">paths failed to resolve when combined with the injected<base>tag — resulting in missing map images in the Studio sub-composition preview<head>and relative asset paths resolve correctlyTest plan
<html>in<body><template>-wrapped compositions still rewrite../asset paths correctlynorth-korea-locked-downornyc-paris-flightas a sub-composition in Studio, click on the sub-comp in the timeline → map should be visible