Repository navigation
fix(preview): show the correct scene instead of every scene at once on some loads - #3848
Conversation
…other window The Studio preview sometimes builds the composition body in the editor window and adopts it into the preview frame's document. Adopted nodes keep the prototypes of the realm that created them, so `node instanceof HTMLElement` is false for every element in the composition even though the elements are ordinary HTML sitting in that document. Every guard in the runtime written as `if (!(node instanceof HTMLElement))` then skipped the whole document, silently: nothing threw, nothing logged, and readiness still reported success. The timed-element visibility pass wrote no inline visibility at all, so on those loads the preview painted every scene on top of every other and the editor drew off-canvas markers for elements the user could not see. The auto-stamp pass stopped stamping too, which is why the composition came up one timeline clip short. Replace every realm-sensitive element check in the runtime with structural predicates that ask what a node IS (node type, namespace, tag name) rather than which window's constructor made it. Two local `doc.defaultView.HTMLElement` workarounds are deleted with it: they fix only the case where the nodes belong to the document's own realm, and adoption is exactly what breaks that.
miguel-heygen
changed the title
…audio `resolveScrubAudioEl` tested `byId instanceof HTMLAudioElement` on a node from the preview iframe's document. That node is an instance of the IFRAME's `HTMLAudioElement`, never this module's, so the check was false on every load and the `musicId` hint was dead. Scrub fell through to the first `<audio>` in the document, which the comment right above it warns can be the voiceover, so dragging the playhead could preview the wrong track. Ask what the node is instead. Also close the gaps an independent review found in the runtime fix: - the cross-realm predicate test had no `<audio>` and no `<img>`, so reverting `isAudioElement` or `isImageElement` to `instanceof` left it green. It no longer does, and the audio case also pins `isMediaElement`, which composes from it and gates the media sync path. - nothing stopped the runtime regressing. `lint-runtime-preview-guards.ts` gains a second check kind: patterns that must be ABSENT under a directory, seeded with DOM-typed `instanceof` under `src/runtime`, pointing at domRealm.ts. Comment lines and tests are exempt, both on purpose. - domRealm.ts stated the adoption mechanism as settled fact. The mixed prototypes are measured; how the nodes get into the frame is not identified, and the docstring now says which is which. Its ownership claim is scoped to the runtime, since packages/studio still hand-rolls its own checks.
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
APPROVE at fd99a82900ce27f0979212df06b448f18288d5b3 — cross-realm instanceof HTMLElement failure mode replaced with structural discriminant across every source file under packages/core/src/runtime/; CI rule bans the full DOM-family shape from re-entering runtime; music-track hint scrub bug fixed by the same mechanism in the studio player.
Author + scope. miguel-heygen (trust-listed). Head fd99a8290. Base main at cd1a996b. +580/-105 across 22 files (268 lines of test per Miguel). mergeStateStatus BLOCKED (empty reviews at head — first). Auto-merge on stamp.
Symptom + cause
packages/core/src/runtime/domRealm.ts:14-21 documents the state honestly: mixed prototypes are MEASURED (Element.prototype tagging in the reproduction), the exact adoption mechanism is called out as an open question (no adoptNode/importNode/document.write caught; frame is populated by src/srcdoc). More conservative than a hand-waved "we know it's this specific API." What matters is the symptom is deterministic and the fix pins the symptom.
Structural discriminant
domRealm.ts:50-52 for Element: nodeType === 1 — cross-realm-safe because Node.ELEMENT_NODE === 1 is a numeric constant identical across realms. domRealm.ts:59-61 for HTMLElement: nodeType === 1 && namespaceURI === "http://www.w3.org/1999/xhtml" — preserves instanceof HTMLElement's SVG-exclusion, so sites that used HTMLElement to reject SVG still reject SVG. isStylableElement at domRealm.ts:104-110 accepts XHTML OR SVG namespaces — matches the OLD positionEdits.ts intent, which already had an SVGElement branch in the hand-roll. False-positive risk (a bare {nodeType:1} sneaking through) is not real because callers pass elements from document.querySelector* / getElementById.
Atomic migration — post-fix grep for survivors
git grep -En '\binstanceof\s+(Element|Node|Text|Document|DocumentFragment|ShadowRoot|HTML[A-Za-z]*Element|SVG[A-Za-z]*Element)\b' packages/core/src/runtime/*.ts on head: 0 non-comment non-test hits. On origin/main: 73+ hits across 16 files (Miguel's "82 sites" reasonable given multi-instanceof per line). Every migrated file imports the new domRealm predicates.
Notably load-bearing: positionEdits.ts:180-200 REPLACES a prior hand-rolled doc.defaultView?.HTMLElement attempt, and startResolver.ts:46-52 REPLACES the parallel defaultView.HTMLMediaElement hand-roll. Miguel calls these out as "two hand-rolled attempts" that were insufficient because nodes can come from a THIRD realm; the structural check has no such case.
CI ban rule (Via's Q1 — full family + scoping)
packages/core/scripts/lint-runtime-preview-guards.ts:31-41 BAN_SPECS pattern is: /\binstanceof\s+(?:Element|Node|Text|Document|DocumentFragment|ShadowRoot|HTML[A-Za-z]*Element|SVG[A-Za-z]*Element)\b/. Covers the FULL DOM-hierarchy family cross-realm breakage class — not just HTMLElement, but HTMLImageElement, HTMLCanvasElement, HTMLVideoElement, HTMLMediaElement, SVGElement, Element, Node, Text, DocumentFragment, ShadowRoot. Scope is PATH-BASED on src/runtime only — correctly excludes packages/studio/** which legitimately uses instanceof for same-realm host-page checks (resolvePreviewPlayerHost, resolveIframe). Errors via process.exit(1), not warns. Skips comment lines (isCommentLine) and *.test.ts (isScannableSource), so docstrings can name the shape and tests can assert the bug directly. Wired into required CI job Test: runtime contract via packages/core/package.json:682 → test:hyperframe-runtime-ci → lint:runtime-preview-guards; .github/workflows/ci.yml:508 runs it.
Small non-blocking observation on the ban regex: a x instanceof globalThis.HTMLElement (qualified) wouldn't match because the pattern requires the type name immediately after \s+. Not worth flagging as a real gap — Miguel's docstring makes the "why not" so clear that anyone hand-rolling a realm-bound check is already going out of their way, and the mainstream instanceof HTML… shape is fully caught. Also worth naming as follow-up: no meta-test in the diff that adds a novel instanceof HTMLImageElement inside src/runtime and asserts lint-runtime-preview-guards fails. Path-based scoping is fragile on file moves; a meta-test would close it. Non-blocking; not required for this stamp.
Tag-identity preservation (Via's Q2)
Helper module exposes tag-specific variants, not a single flatten-to-isElement. Present: isHtmlElement (SVG-excluding — mirrors instanceof HTMLElement), isStylableElement (SVG-including — mirrors old positionEdits intent), isMediaElement, isAudioNode (studio-side). Migration sites picked the right variant per original intent: startResolver.ts:46-52 maps old defaultView.HTMLMediaElement → new isMediaElement; positionEdits.ts:180-200 maps old doc.defaultView?.HTMLElement → isHtmlElement; timelineIframeHelpers.ts:229 maps old instanceof HTMLAudioElement → isAudioNode. No collapse to a looser predicate; SVG-inclusion parity preserved at isStylableElement per the OLD SVGElement branch.
Music-track hint scrub bug — same mechanism, enumeration scoping gap (Via's Q3)
The music-track hint scrub bug is NOT a separate mechanism (portal / postMessage / module-dedupe). It's the SAME cross-realm mechanism: packages/studio/src/player/lib/timelineIframeHelpers.ts runs in the studio host and queries into the preview iframe — the exact same host→iframe crossing. The 82-site enumeration missed it initially because the enumeration was scoped to packages/core/src/runtime/, not because a different mechanism is at play. Miguel found the ONE studio-side site (music-track scrub) and fixed it in the second commit; domRealm.ts:36-38 explicitly acknowledges the lint scope gap for studio.
Load-bearing test at timelineIframeHelpers.test.ts:99-125 "previews the track named by musicId, not the first audio in the document": mounts voiceover FIRST, music-bed SECOND, calls scrubPreviewAudio(iframe, 0.5, "music-bed", 1), asserts music.play called AND voiceover.play NOT called. Line 118 explicitly pins the cross-realm condition with expect(music instanceof HTMLAudioElement).toBe(false) — proving the failure is real, not hypothetical. Reverting isAudioNode to instanceof HTMLAudioElement flips this red.
So Via's residual worry ("if the enumeration was built off a search shape that would also miss whatever killed the music-track hint") lands as: NO different mechanism; the enumeration was scoped to src/runtime, missing a studio-side cross-realm site that ALSO needed the fix; Miguel found and fixed the single such site in commit 2. Predicts NO other guards outside the 82 that would still fail in the wild by a different mechanism.
Load-bearing tests that catch a discriminant regression
packages/core/src/runtime/domRealm.test.ts:121— "accepts an element adopted from another realm, where instanceof does not" (predicate parity across div/video/audio/img/svg)packages/core/src/runtime/domRealm.test.ts:174-200— "writes timed-element visibility for cross-realm clips" (the dark-scene symptom end-to-end:initSandboxRuntimeModularover a JSDOM foreign realm, assertsstyle.visibilityat two seek times). RevertingapplyTimedElementVisibility'sisHtmlElementguard atinit.ts:2260back toinstanceof HTMLElementflips this red.packages/studio/src/player/lib/timelineIframeHelpers.test.ts:99— the voiceover-scrub companion bug with explicitmusic instanceof HTMLAudioElement === falsepinpackages/core/scripts/lint-runtime-preview-guards.ts— the source-shape ban itself; any newinstanceof HTML*Elementinsrc/runtimenon-test/non-comment fails CI immediately
Render path untouched, same-realm behavior identical
Diff files are all under packages/core/src/runtime/, packages/studio/src/player/lib/timelineIframeHelpers.ts, or the generated position-edits-render-inline.ts (compiled from positionEdits.ts — same swap). Render path in packages/producer/src/services/render and packages/cli/src/commands/render — zero files touched. Same-realm behavior preserved because structural checks return true for same-realm nodes too (pinned by domRealm.test.ts:82-108).
Non-blocking observations
- Ban regex misses qualified
instanceof globalThis.HTMLElement(see above); mainstream shape covered. - Path-based lint scope + no meta-test → a same-PR test adding
instanceof HTMLImageElementinsrc/runtimeand asserting CI red would harden the guard against file-move drift. Follow-up hardening, not a block. - Legitimate same-realm
instanceof HTMLElement/instanceof HTMLIFrameElementretained inresolvePreviewPlayerHost/resolveIframe(studio HOST page, same-realm by construction) — correctly out of lint scope.
At-head reviews & CI
Empty reviews at head (this is the first). All required checks at head SUCCESS: Test: runtime contract (runs the new lint), Test, Preview parity, Player perf (load/fps/scrub/drift/parity), all 9 regression-shards, Windows render verification (studio-core, studio-engine-cli, Windows tests), Studio: load smoke, Typecheck, Lint, Fallow audit, Build, Producer: unit/integration, SDK: unit+contract+smoke, CLI smoke (required), Smoke: global install, CodeQL (actions + javascript-typescript + python), Format, File size check. Earlier FAILURE conclusions on regression / Test / Tests on windows-latest from runs starting 14:39 are all superseded — those runs were CANCELLED and re-run to green in later runs at 14:41-14:53 (classic head-thrash / amended-commits pattern; head-only reads are clean).
Coordination. Via (peer bot) deferred stamp to tai as primary with three sight-unseen shape-of-question angles: Q1 (CI rule breadth + scoping) — regex covers full DOM family, scope path-based on src/runtime, meta-test absent (follow-up); Q2 (tag identity preservation) — helper exposes tag-specific variants, sites picked correctly, SVG-exclusion parity preserved via namespaceURI check; Q3 (same-realm music-track bug mechanism) — SAME cross-realm mechanism, enumeration scoping gap Miguel closed in commit 2, predicts no other-mechanism sites. All three Via-concerns concurred.
— Review by tai (pr-review)
What this fixes
On some preview loads the editor showed every scene of a composition stacked on top of every
other, and the canvas filled with dashed off-canvas markers for elements that should have been
hidden. There was nothing the user could do about it except reload and hope. Playback still ran,
the playhead still moved, and nothing was logged, so it read as a rendering quirk rather than a
bug.
The preview now shows the correct scene on every load.
What was actually happening
The runtime hides and shows timed elements by walking the composition and writing an inline
visibilityon each one. Every guard in that walk, and in the pass that stamps timing ontoeditor-selectable elements, was written as
if (!(node instanceof HTMLElement)) ....instanceofdoes not ask what an object is. It asks which JavaScript realm created it. Eachbrowsing context has its own
HTMLElement, and moving a node between documents (the DOM adoptionstep) changes the node's owning document while leaving its prototype chain pointing at the realm
that built it.
On the affected loads, the composition body is built in the editor window and adopted into the
preview frame's document. Measured inside the preview document at readiness:
[data-start]elementsvisibilitynode instanceof <preview frame>.HTMLElementnode instanceof <editor window>.HTMLElementTagging each realm's
Element.prototypewith a unique id before any page script ran, then readingthe tag back off the live nodes, isolated it exactly:
<html>,<head>and<script>carry thepreview frame's tag, while
<body>and all 81 timed elements carry the editor window's.What is measured and what is not. The mixed prototypes are measured, repeatedly, by the tag
above. How the nodes come to be there is not identified: no
adoptNode,importNode,replaceChildordocument.writecall was caught doing it from either realm, and the frame ispopulated by
src/srcdoc. The fix does not depend on the answer — a structural test is correctwhatever the route — but the route is still an open question and the module says so.
So
instanceof HTMLElementreturnedfalsefor every element in the composition. It did not throwand did not warn; each guard simply skipped its whole document. The visibility pass wrote nothing,
the auto-stamp pass stamped nothing (that missing 82nd element is the tell), and readiness still
reported success because nothing had failed.
An earlier instance of this class was worked around locally with
instanceof doc.defaultView.HTMLElement. This change removes that too: it assumes a node's realmequals its document's realm, and here the nodes belong to a third realm that is neither the
module's nor the document's.
The invariant
Stated in the new module, in plain words: the runtime never assumes the composition DOM was built
by the same JavaScript realm the runtime is running in. It asks what a node IS (node type,
namespace, tag name), never which window's constructor made it.
isHtmlElementkeeps exact parity withinstanceof HTMLElement, including its exclusion of SVG:"is an
HTMLElement" and "is an element in the XHTML namespace" are the same set.The predicates own the runtime's answer to that question.
packages/studiostill hand-rollsits own checks; see the follow-up list below.
The change
One new module owns the question, and every realm-sensitive element check in the runtime routes
through it: 82 call sites across 15 files, each a 1:1 semantic replacement.
x instanceof HTMLElementbecomesisHtmlElement(x)x instanceof HTMLVideoElement/HTMLAudioElement/HTMLMediaElement/HTMLImageElement/HTMLStyleElement/HTMLLinkElementbecome the matching predicatex instanceof ElementbecomesisElementNode(x)Only the visibility and auto-stamp passes were needed to close the reported bug. The rest are the
same defect waiting for a different load: on an affected load the media-sync path, caption
overrides, colour grading and position edits were all skipping the composition too, for the same
reason and with the same silence.
The predicates avoid a rest parameter and
includesbecause several call sites are hot: measuredover 100M calls, the allocating form cost 1.7x on the single-tag check and 3.3x on the two-tag one.
Tests
domRealm.test.tsbuilds the composition in a second realm and adopts it into the runtime'sdocument, which is the hostile ordering, then runs the real runtime entry point over it:
visible/hiddenwritten as the playhead moves across their windowsinstanceoffor same-realm nodes, including the SVG exclusionnull, strings, documents, text nodes and look-alike plain objectsProven non-vacuous by mutation, one predicate at a time:
isElementNode,isHtmlElement,isHtmlTagback toinstanceof["", ""]where["visible", "hidden"]is expected,nullwhere the stampeddata-startis expectedisAudioElementalone back toinstanceof HTMLAudioElementexpect(isAudioElement(audio)).toBe(true)getsfalseexpect(music.play).toHaveBeenCalled()— "expected spy to be called at least once"The audio mutation survived the first version of this test, because the cross-realm case had no
<audio>in it. It does now, and it pinsisMediaElementtoo, which composes fromisAudioElementand gates the media sync path.Nothing stopped this regressing, so now something does
packages/core/scripts/lint-runtime-preview-guards.tsis the repo's existing CI source-shapeguard, and it only knew how to require a pattern be PRESENT in one file. It gains a second check
kind: patterns that must be ABSENT under a directory. The first entry bans DOM-typed
instanceof(
Element,Node,Text,Document,DocumentFragment,ShadowRoot,HTML*Element,SVG*Element) anywhere undersrc/runtimeand namesdomRealm.tsas the remedy.Comment lines are exempt, so a comment can still explain why the shape is banned;
.test.tsfilesare exempt, because the tests assert the banned shape on purpose to prove the predicates match it.
Proven to fire: reintroducing one
instanceof Elementinsrc/runtime/picker.ts, and separatelyone
instanceof HTMLElementin the nestedsrc/runtime/adapters/css.ts, each exits 1 with thefile, line and source text named. Both removed again; the check exits 0 on this head.
A live bug of the same class, fixed here
resolveScrubAudioElin the studio's timeline iframe helpers testedbyId instanceof HTMLAudioElementon a node from the preview iframe's document. Same mechanism,worse odds: that node belongs to the iframe realm on every load, not just affected ones, so the
check was always false and the
musicIdhint the timeline store had already resolved was dead. Itfell through to
querySelector("audio")— the first audio in the document — which the commentdirectly above it warns can be the voiceover. Dragging the playhead could preview the wrong track.
The existing test could not see it: with a single
<audio>in the fixture, the fallback reaches theright node by accident. The new test puts a voiceover first and the music second, which is what
tells the two paths apart.
Fixed with a local structural check rather than by importing the runtime predicate. Exporting
domRealmas a public@hyperframes/coresubpath is the better long-term shape and would make thefollow-up below trivial, but it is permanent public API surface on an open-source package, so it is
a call for a human rather than something to slip in here.
Reproduction, before and after
Local harness, same machine, same project, same server, same delay injected on sub-composition
requests to widen the window. Both arms built from source, and each arm's served runtime was
checked for the fix before the runs started.
[data-start]at readinessAn earlier baseline arm on the same base, run under heavier machine load, reproduced 7 of 13. The
rate moves with timing; the mode does not.
getBoundingClientRectcalls per seek, the cheap live mode detector, moved from 3.1 on healthyruns and 484–510 on affected ones, to 3.1–4.4 (31–50 under load) on every run after.
Follow-up, not in this PR
An independent review flagged eight hand-rolled studio checks. Opening each one rather than
inheriting the list changes it materially — most are already correct:
components/editor/LayersPanel.tsx:202instanceof HTMLElementon a node from the preview documentcomponents/editor/studioMotion.ts:76ownerDocument.defaultView.HTMLElement, which is right for same-realm nodes and wrong for the case measured hereplayer/lib/timelineGroupInfo.ts:72instanceof Elementon MutationObserver records, depends which document is observedhooks/gsapResizeIntercept.ts:219instanceoffor exactly this reasoncomponents/editor/useFxCarve.ts:103utils/gsapSoftReload.ts:380utils/gsapSoftReload.ts:318DOMParserdocument in the studio's own realmcomponents/editor/domEditInlineText.ts:61So the follow-up is two sites and one to confirm, not eight.
Render path
__renderReadysemantics are untouched: no readiness gate, ordering or flag timing changed. Theproduction render runs as a top-level page whose DOM is parsed by its own realm, so every predicate
returns exactly what
instanceofreturned there, and frame output is unchanged. The producer'sconsumers of the flag (
packages/producer/src/parity-harness.ts:80,packages/producer/src/services/fileServer.ts:653) see no change in when it flips.Verification