Repository navigation
fix(engine): a composition whose script throws is skipped with a warning, not refused - #6156
Conversation
…ing, not waited on and refused A script that threw before registering its timeline kept the capture waiting the full 45 s readiness timeout, then the render was refused. After a page error with timelines still missing the wait now ends after a short grace, each such composition gets an empty paused timeline of its authored length, and a script_error_isolated warning names the element. The render goes on without that animation.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 1 important · 0 nits · head e74596a9
This undoes the #3352 guard (renderOrchestrator.ts:828-833, "fail loudly rather than shipping garbage"). Any page that has window.gsap, which is every real composition, now finishes with exit 0 and only a non-blocking warning. Details inline.
Scope
Read all 5 changed files, plus pollSubCompositionTimelines, page-error attribution (classifyPageError, which already drops cross-origin widget errors, so the "unrelated error" limit only applies to project scripts), the producer's blocking list, and scriptFailureAttribution.test.ts. I did not run the tests locally. CI (CI, regression, Windows, CodeQL) was still running at review time.
| shouldStop: () => session.vfxFailure !== undefined, | ||
| knownUnregisteredIds: memo.unregisteredIds, | ||
| }); | ||
| if (isScriptFailure(session)) { |
There was a problem hiding this comment.
blocker: Isolation turns every script_failure into the non-blocking script_error_isolated. Two cases now ship a broken video with exit 0:
- Missing script files, not only thrown errors. A 404'd project script also ends in
script_failure(the load-failure path), so it gets isolated too. The PR body only describes scripts that throw. - The isolated composition is the whole animation. This is decision-tree example renders 2 unique frames, and check + render both report success #3352 again: the decision-tree scene throws and the result is a still image. Your browser test locks in that exact shape: the only composition,
root, gets isolated and the render goes ahead.
The #3352 regression tests in scriptFailureAttribution.test.ts still pass only because their fixtures never load gsap, so isolateUnregisteredCompositions returns [] and the old blocking code still fires.
Suggested fix: limit isolation to page errors (leave load failures blocking). Also keep sub_timeline_script_failure when the root composition is isolated, or when no composition with a real timeline is left. If dropping the #3352 policy is intended, the PR should say so and update the orchestrator comment and its test.
| const pending = session.pendingTimelineIds ?? []; | ||
| const pendingSuffix = pending.length > 0 ? ` (still unregistered: ${pending.join(", ")})` : ""; | ||
| const isolated = session.isolatedCompositions ?? []; | ||
| if (isolated.length > 0) { |
There was a problem hiding this comment.
important: No test runs the real producer path with a gsap-bearing composition whose script throws. The engine browser test calls the two helpers directly with a fake gsap, so nothing checks end to end that script_error_isolated stays non-blocking in renderOrchestrator while a missing script or a fully isolated page still fails. A gsap case in scriptFailureAttribution.test.ts would cover both sides of that line.
…failures or a root
|
Both findings addressed at head 813e979.
Tests in |
Edit accuracy: accurate 271 (base branch 271), smooth 144 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (3)
|
…position, not a load error
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — 0 resolved, 2 open, 1 new · e74596a9 → bf3614ea
The blocker is only partly fixed: load failures and a root with no timeline now stay blocking, but the #3352 case itself still ships (inline). The producer-path test is still open.
| var top = hosts[i]; | ||
| var topId = top.getAttribute("data-composition-id"); | ||
| var isRoot = !top.parentElement || !top.parentElement.closest("[data-composition-id]"); | ||
| if (isRoot && topId && !timelines[topId] && !top.hasAttribute("data-no-timeline")) return out; |
There was a problem hiding this comment.
blocker (still open): The guard only looks at whether the root registered a timeline. A root that registers an empty one passes. That is the shipped registry/examples/decision-tree shape: index.html:51-53 registers an empty main timeline, and all the animation lives in the nested decision-tree composition, whose throw is #3352. On this head that nested composition gets isolated and the render exits 0 with a still image and a warning. So the PR thread's claim that "a page whose only animation failed is still refused" doesn't hold for the most common template layout.
Suggested fix: also keep sub_timeline_script_failure when no nested composition with a real timeline would be left after isolation, i.e. every nested host that wants a timeline is being isolated. Add a browser case shaped like decision-tree (root registers an empty timeline, the only nested composition throws) and assert the blocking code.
| if (failures.length > 0 && now - start >= scriptFailureGraceMs) { | ||
| const errors = getPageErrors?.() ?? []; | ||
| const failed = failures.length > 0 ? failures : errors; | ||
| const graceMs = failures.length > 0 ? scriptFailureGraceMs : scriptErrorGraceMs; |
There was a problem hiding this comment.
important (new): With a root that registers late, this 5 s grace turns a render that used to succeed into a refused one. Take a root that builds its timeline after an async fetch, which the timeout warning explicitly supports, plus any project-script page error, even a harmless one. Before this PR the wait went on until the root registered (up to 45 s) and the render succeeded. Now the poll stops 5 s after it started, isolation declines because the root is unregistered, and sub_timeline_script_failure blocks the render. The grace is measured from when the poll started, not from when the error happened. Consider skipping the page-error grace while the root is still unregistered, or starting the grace at the first error and only for the composition that threw.
…he start of the wait
|
Head 98e37ec. Late root (important): the page-error grace now starts at the first error instead of when the wait began, with a poll test for it. A root that registers after a slow fetch is only cut off if an error appeared and 5 s then passed with the root still unregistered. A root that throws before registering cannot be told apart from a late one at that point; failing fast on it is the intended behaviour. Empty root plus a single throwing nested composition (blocker): not changed in this push. Whether that shape (the decision-tree layout) ships with a warning or stays refused is a product decision, and I have asked for it; I will follow the answer in this PR. |
|
Head 141f08f, one line per point:
|
There was a problem hiding this comment.
Verdict: APPROVE — 1 resolved, 2 open (important, not blocking), 0 new · bf3614ea → 141f08fe
✅ Blocker fixed in 141f08fe: isolation now needs another live nested timeline, so a decision-tree-shaped page fails the render again, and the new browser case checks that.
Still open: (1) the grace is partly fixed. It now starts at the first error, but an early harmless project-script error plus a root that registers more than 5 s later (after an async fetch) still gets refused where it used to succeed. (2) There is no producer-path test with gsap on the page.
What
A composition whose own script throws (a lookup of an element id that is not in the page, for example) used to leave its timeline unregistered. The renderer waited the full 45 s readiness timeout and then refused the render. Two changes:
script_error_isolatedwarning naming its element, but only while another nested composition with a live timeline remains. If nothing live would be left (a root that never registered, a single nested composition that is the whole animation, or a script file that failed to load), the render is still refused withsub_timeline_script_failure, now in seconds.How
frameCapture.ts: the page-error grace is counted from the first error (load failures keep their 2 s grace).isolateUnregisteredCompositionsregisters an empty paused timeline of the host'sdata-durationfor each unregistered nested composition, and returns nothing when the root is unregistered or no nested timeline is live.recordSubTimelineWarningemitsscript_error_isolatedwith the element ids, or the blocking warning with the thrown error.types.ts: newscript_error_isolatedwarning code; the orchestrator's blocking list is unchanged.Known limit: isolation needs
window.gsapon the page.Evidence
Synthetic compositions, Linux, 1280x720, 4 s. Each has a static title, a nested composition whose script looks up a missing id, and (except the last row) a second nested composition with a live timeline.
appendChildof a missing iddesigngetTotalLengthof a missing idgetBoundingClientRectof a missing id_gsapread on a missing elementdesignA root-level throw is refused in 15 s total with the error naming the root composition.
Tests
frameCapture-subTimelinePoll.test.ts: bails after the grace when a script threw and nothing registers, counts the grace from the first error, still takes a late registration.frameCapture-isolateScript.browser.test.ts(real Chrome): isolates a nested throw next to a live nested timeline; keeps the blocking code for a failed root, for a single nested composition, and for a load failure. The first and the single-nested cases fail on main.frameCapture-subTimelineWarning.test.ts,frameCapture-subTimelineMemo.test.ts: the thrown-script message and outcome.