Skip to content

fix(engine): name the data-no-timeline opt-out in the timeline-timeout warning - #4410

Open
WaterrrForever wants to merge 2 commits into
mainfrom
fix/sub-timeline-warning-names-remedy
Open

WaterrrForever wants to merge 2 commits into
mainfrom
fix/sub-timeline-warning-names-remedy

Conversation

@WaterrrForever

Copy link
Copy Markdown
Collaborator

The gap

A sub-composition timeline timeout emits two messages, and only one of them is useful:

names the opt-out names the pending ids who reads it
stderr, inside pollSubCompositionTimelines yes yes a human watching the run
structured CaptureWarning no no render's summary, result.warnings, the SDK, CI

The structured one said only:

[sub_timeline_readiness_timeout] Sub-composition timelines did not become ready within 45000ms

render.ts re-prints that in the completed_with_warnings block and returns it in the
result's warnings array, so a programmatic caller gets the symptom with no remedy — and a
human reading scrollback gets it after the rich line has scrolled away.

It also reads as a fault to diagnose. The commonest cause is the opposite: a composition
driven by CSS animations or rAF never registers window.__timelines[id], so it is polled
for the whole playerReadyTimeout (45 s, packages/engine/src/config.ts) and then proceeds
with a warning. That composition is correct. It is paying 45 s a render because nothing told
the author the wait was theirs to switch off.

The change

  • The timeout message says the wait can be intentional, names data-no-timeline as the
    opt-out, and keeps the async-setup case as the other branch.
  • The pending composition ids were already computed for the stderr line and thrown away. An
    optional onPending callback carries them to the session, so the structured warning and
    its details.pendingCompositionIds name them too — matching the sub_timeline_script_failure
    branch, which already names its failed sources.
  • The stderr text is unchanged; missing is still ids.join(", ").
  • The script-failure branch is untouched and deliberately does not gain the
    data-no-timeline advice: there the timeline can never arrive, so that would be the wrong
    remedy.

Before / after, for a composition with no timeline:

- Sub-composition timelines did not become ready within 45000ms
+ Sub-composition timelines did not become ready within 45000ms (still unregistered: scene-2).
+ This can be intentional: a composition driven by CSS animations or rAF never registers
+ window.__timelines[id], and marking its host with data-no-timeline skips the wait entirely.
+ Otherwise, a composition that sets up asynchronously must register window.__timelines[id]
+ once setup completes.

Tests

packages/engine/src/services/frameCapture-subTimelineWarning.test.ts — 6 cases:
no warning on ready/absent outcome, the remedy named, the ids named, no empty
parenthetical when the list is empty, and the script-failure branch left alone.

I could not run vitest in this environment (no installed node_modules for the
workspace), so I will not claim the suite is green. What I did instead, and what I can
stand behind: I extracted recordCaptureWarnings and recordSubTimelineWarning verbatim
from the patched file and executed them under node --experimental-strip-types against the
same assertions plus a dedup case — 16/16 pass. Two mutants confirm the assertions
discriminate rather than passing by construction:

  • the pre-patch message → 4 red (remedy, "intentional", window.__timelines[id], ids);
  • the plausible-but-incomplete fix — names data-no-timeline but omits that the wait can
    be intentional → exactly 1 red, the assertion for that clause.

Please still run the real suite in CI; the extraction shares the production lines but not
the production module graph.

🤖 Generated with Claude Code

WaterrrForever and others added 2 commits September 24, 2026 19:43
…t warning

When the sub-composition timeline wait times out, two things are emitted:
the stderr line inside pollSubCompositionTimelines, which names the pending
composition ids and the data-no-timeline opt-out, and the structured warning
on the session, which said only "Sub-composition timelines did not become
ready within 45000ms".

The structured one is what a programmatic caller reads: render re-prints it
in the completed_with_warnings summary and returns it in the result's
`warnings` array, so a CI consumer or an SDK caller sees the symptom with no
remedy attached. It also reads as a fault to diagnose, when the commonest
cause is a composition that legitimately has no GSAP timeline -- CSS
animations or rAF -- and is paying the full playerReadyTimeout on every
render because nobody told the author the wait is theirs to switch off.

The message now says the wait can be intentional, names data-no-timeline as
the opt-out, and keeps the async-setup case as the other branch. The pending
composition ids were already computed for the stderr line and thrown away;
an optional onPending callback carries them to the session so the structured
warning and its details can name them too, matching the script-failure
branch, which already names its failed sources. The stderr text is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enumerate step cast `page.evaluate`'s result with `as string[]`, which is a
claim the call site cannot enforce: `evaluate` is loosely typed, and anything
that returns a non-array turned the cast into a TypeError thrown on the very
path that exists to REPORT a problem. Normalise the result instead.

`pendingCompositionIds` was also never declared on `CaptureWarning.details`,
whose type is closed, so the engine build failed and every downstream package
failed with it.

Adds coverage for the poll's own `onPending` handoff, which the existing tests
in this PR could not see: they exercised `recordSubTimelineWarning` (the
message text) and never `pollSubCompositionTimelines` (where the throw was).
The non-array case is the one that discriminates this fix from the tempting
alternative of leaving the cast and adjusting the stubs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified end-to-end against the diff at 3374069 (fresh clone, merge-base 326df8d vs main, matches the stated +193/-5 over 3 files exactly).

Both self-reported defects, verified fixed:

  • CaptureWarning.details.pendingCompositionIds?: string[] is genuinely declared on the type (packages/engine/src/types.ts:33), not just used under a loosened/any shape.
  • The enumerate step's cast is replaced with a real, total normalisation (frameCapture.ts:1816): Array.isArray(evaluated) ? evaluated.map((id) => String(id)) : []. Traced every shape page.evaluate can return (array, object, null, undefined, false) — none throw. Confirmed the actual regression by mutation test: reverting to (evaluated as string[]).join(", ") reproduces the exact original break (TypeError: ids is not iterable inside pollSubCompositionTimelines), and the new test file's "degrades to no ids instead of throwing" case catches it. Restored, re-ran green (7/7).
  • Ran the two previously-broken tests in frameCapture-subTimelinePoll.test.ts directly: 4/4 pass at this head.

Wiring traced, not assumed: onPendingsession.pendingTimelineIdsrecordSubTimelineWarningsession.warningsapplyRenderWarningPolicy (in renderOrchestrator.ts) → job.warnings, the same pre-existing pipeline every other warning code already uses (proven correct by existing coverage on audio_processing_failed/media_readiness_timeout). cloneCaptureWarning spreads details before deep-cloning the array fields it knows about, so pendingCompositionIds survives the clone, just not via a dedicated deep-copy line like sources/failureReasons get — a shallow-clone gap that isn't live today (the array is freshly literal-constructed at the call site) but worth a one-line addition to cloneCaptureWarning if this ever gets mutated post-clone. Nit, not blocking.

Script-failure branch correctly left alone: confirmed by reading the branch condition directly — scriptFailure ? <unchanged messages> : <new timeout message with the remedy>. The reasoning holds: a script-load/runtime failure means the timeline can never register regardless of data-no-timeline, so pointing at that opt-out there would misdirect the fix. Test asserts the message excludes "data-no-timeline" on this branch.

Pending-id list is bounded by the number of [data-composition-id] hosts on the page, same bound the pre-existing scriptLoadFailures field already carries — no new unbounded-payload risk.

CI: all required checks green at this head (Build, Lint, Preflight, all regression shards, CLI smoke). No prior reviews on this PR to be additive to.

Verdict: APPROVE
Reasoning: Both self-reported defects are genuinely fixed, the fix is proven by mutation test (not just present), the reporting path is now total for every observed input shape, and the script-failure branch's exclusion of the remedy is deliberate and correct.

— Miga

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants