fix: preserve the composition query and serve the runtime before author scripts - #3114
Merged
Conversation
Every src the player sets goes through withShaderQueryParams, which parsed the author's whole query with URLSearchParams and re-serialised it with toString(). That is a form encoder: it writes a space as +, while callers percent-encode and read back with decodeURIComponent. Those two codecs are not inverses, so any space in any query value arrived corrupted. It ran even when there was nothing to inject. With no shader attributes both params are deleted, so the round-trip was pure loss, on every src, for every consumer. Append the two params to the raw query instead of re-serialising it. The player now hands a composition its query back byte-identical. Empirically space was the only casualty: plus, ampersand, equals, hash, percent, question mark, quotes and non-ASCII all survived a URLSearchParams round-trip. That is narrow, but a space in a headline or in SVG path data is the common case, and invalid path data renders nothing at all. Latent until now: no shipped consumer depended on query preservation, so this surfaced only once compositions began carrying variable payloads.
injectRuntime appended its script before </body>, so it landed after any inline script the composition carried. At the moment a composition's own script ran, window.__hyperframes was undefined and getVariables() was unreachable: our documented API did not exist at the point authors are told to call it. Served order was gsap at line 6, the composition's init script at 20, the runtime at 37. A probe inside the composition's IIFE recorded hfTypeAtInit undefined with no variable keys, and the element rendered its hardcoded fallback rather than the declared value. The runtime is designed to load early. Its entry assigns __timelines, installs the authored-opacity capture (whose own comment says it must run while the document is still parsing), and exposes __hyperframes synchronously, deferring real work to DOMContentLoaded. End-of-body injection defeated all three, and nothing in it needs a parsed DOM, so no defer is wanted. Injects at head start instead, reusing the placement cascade injectScriptsAtHeadStart already implemented rather than adding a fourth copy of it. Head start rather than the closing tag so the runtime also precedes author scripts inside head. injectRuntime has exactly one consumer, the play server's composition route. Every other surface reaches the runtime through the bundler, which already injects into head, or deliberately serves raw. Two registry blocks had independently worked around this by parsing the authored attribute themselves. Those stay, but the workaround is no longer the only way to read a variable at init.
miguel-heygen
changed the title
vanceingalls
added a commit
that referenced
this pull request
Aug 8, 2026
…anary gate Deletes the `de-parallel-router` canary entry and the `isCanaryEnabled` guard in render.ts together, leaving the producer's default-ON in place. Net effect for users: the parallel drawElement router is on for everyone again. ## Why, and why not a ramp Gating at 5% was itself the regression. Measured 2026-08-08, the day after v0.7.101 shipped the canary: fleet router exposure fell from 3.13-4.25% of non-CI renders to **0.13%**, roughly 25x, because out-of-cohort installs are explicitly disarmed and #2840 deleted the everyone-armed trial in the same change. 2,537 installs lost a feature they already had. Severity is speed only, never output, and nothing is persisted to disk. PR #2840's body claimed "the canary does not make exposure smaller; it makes it chosen and revertible." That was true of the end state and false of the first step. This lands the end state. Entry and guard go together deliberately: at >=100 the evaluator short-circuits ahead of the CI/seedless exclusions, so removing only the entry would have flipped whatever still resolved false at deletion time, unstaged. ## Both stated blockers are void - **≤4-CPU / Docker coverage gap.** Docker renders never use drawElement — 0 of 4,281 across every CPU tier, software GL gates it out — and the router requires it. No percentage could ever expose Docker, so no ramp closes that gap. ≤4 CPUs yields ~42 drawElement candidates in three days. - **PRINFRA-372.** Its signature has hits on 0.4.12, 0.4.37, 0.6.52, 0.6.93, 0.6.109 and 0.6.110 — versions predating drawElement (v0.7.38) and therefore this router. It is real, still live on 0.7.101, and belongs to the screenshot/beginframe path. 11 reproduction runs across four configurations on the enriched profile (darwin/arm64 25.5.0) came back clean. ## Safety unchanged The per-install circuit breaker and the per-render self-verify are untouched; `HF_DE_PARALLEL_ROUTER=false` remains the user-facing kill switch. Post-canary data at 14 days: >8 CPUs 3.02% revert (177/5,857), 5-8 CPUs 2.40% (6/250) — consistent with the 2.75-3.16% baseline. Revert path is now a code revert rather than a registry edit. That is the trade this shape accepts in exchange for one release instead of two. ## Corrects two claims that shipped wrong `~17x jump in exposure onto <=4 CPUs / Docker` overstated the reach, and `~11% of installs already route` was an OUTCOME (the share clearing eligibility and the old 25-render cap), not an exposure setting — read as a rollout knob it inverts the arithmetic, which is how gating at 5% came to cut exposure rather than ramp it. Both are recorded in render.ts so they are not reintroduced. ## Tests Removed the core wiring assertion and the two CLI canary-gating tests, which pinned a gate that no longer exists. Added the inverse guarantee in its place: an ordinary install must come out of the breaker with the var UNSET so the producer default applies — writing "false" there is precisely what disarmed the fleet at 5%. core 1701 passing, cli 2491 passing, studio canary 29 passing. The 2 failures in play.test.ts reproduce on clean origin/main and are unrelated (#3114 area). oxlint and oxfmt clean. Note: telemetry for this rollout stops with the entry — `$feature/canary-de-parallel-router` and `canary_reason_de_parallel_router` are emitted from the registry, so the `Ramp —` tiles and the exposure-floor alert on PostHog dashboard 1918875 go blank once this ships. Watch drawElement engagement on 1807532 instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vanceingalls
added a commit
that referenced
this pull request
Aug 8, 2026
…anary gate (#3120) Deletes the `de-parallel-router` canary entry and the `isCanaryEnabled` guard in render.ts together, leaving the producer's default-ON in place. Net effect for users: the parallel drawElement router is on for everyone again. ## Why, and why not a ramp Gating at 5% was itself the regression. Measured 2026-08-08, the day after v0.7.101 shipped the canary: fleet router exposure fell from 3.13-4.25% of non-CI renders to **0.13%**, roughly 25x, because out-of-cohort installs are explicitly disarmed and #2840 deleted the everyone-armed trial in the same change. 2,537 installs lost a feature they already had. Severity is speed only, never output, and nothing is persisted to disk. PR #2840's body claimed "the canary does not make exposure smaller; it makes it chosen and revertible." That was true of the end state and false of the first step. This lands the end state. Entry and guard go together deliberately: at >=100 the evaluator short-circuits ahead of the CI/seedless exclusions, so removing only the entry would have flipped whatever still resolved false at deletion time, unstaged. ## Both stated blockers are void - **≤4-CPU / Docker coverage gap.** Docker renders never use drawElement — 0 of 4,281 across every CPU tier, software GL gates it out — and the router requires it. No percentage could ever expose Docker, so no ramp closes that gap. ≤4 CPUs yields ~42 drawElement candidates in three days. - **PRINFRA-372.** Its signature has hits on 0.4.12, 0.4.37, 0.6.52, 0.6.93, 0.6.109 and 0.6.110 — versions predating drawElement (v0.7.38) and therefore this router. It is real, still live on 0.7.101, and belongs to the screenshot/beginframe path. 11 reproduction runs across four configurations on the enriched profile (darwin/arm64 25.5.0) came back clean. ## Safety unchanged The per-install circuit breaker and the per-render self-verify are untouched; `HF_DE_PARALLEL_ROUTER=false` remains the user-facing kill switch. Post-canary data at 14 days: >8 CPUs 3.02% revert (177/5,857), 5-8 CPUs 2.40% (6/250) — consistent with the 2.75-3.16% baseline. Revert path is now a code revert rather than a registry edit. That is the trade this shape accepts in exchange for one release instead of two. ## Corrects two claims that shipped wrong `~17x jump in exposure onto <=4 CPUs / Docker` overstated the reach, and `~11% of installs already route` was an OUTCOME (the share clearing eligibility and the old 25-render cap), not an exposure setting — read as a rollout knob it inverts the arithmetic, which is how gating at 5% came to cut exposure rather than ramp it. Both are recorded in render.ts so they are not reintroduced. ## Tests Removed the core wiring assertion and the two CLI canary-gating tests, which pinned a gate that no longer exists. Added the inverse guarantee in its place: an ordinary install must come out of the breaker with the var UNSET so the producer default applies — writing "false" there is precisely what disarmed the fleet at 5%. core 1701 passing, cli 2491 passing, studio canary 29 passing. The 2 failures in play.test.ts reproduce on clean origin/main and are unrelated (#3114 area). oxlint and oxfmt clean. Note: telemetry for this rollout stops with the entry — `$feature/canary-de-parallel-router` and `canary_reason_de_parallel_router` are emitted from the registry, so the `Ramp —` tiles and the exposure-floor alert on PostHog dashboard 1918875 go blank once this ships. Watch drawElement engagement on 1807532 instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two framework fixes that together unbreak composition variables on the
playpath.<hyperframes-player>hands a composition its query back byte-identical instead of re-encoding it.window.__hyperframesexists when a composition's own inline script runs.Why 1: the player was corrupting every query
Every
srcthe player set went throughwithShaderQueryParams, which parsed the author's whole query withURLSearchParamsand re-serialised it withtoString(). That is a form encoder: it writes a space as+. Callers percent-encode and read back withdecodeURIComponent, which does not turn+into a space. The two codecs are not inverses.It ran even with nothing to inject. With no shader attributes both params are deleted, so the round-trip was pure loss, on every
src, for every consumer.Symptoms downstream: a headline reading
Ship+it+today, and SVG path data arriving asM+92+328+C+178+142..., which the browser rejects outright so the element renders nothing.Scope was measured, not assumed:
+,&,=,#,%,?, quotes and non-ASCII all survive aURLSearchParamsround-trip. Space was the only casualty. Narrow, but a space in a headline or in path data is the common case.Deliberately not fixed by replacing
+with a space on the read side — that corrupts any genuine literal+, and the catalog ships+312%today.Why 2: the runtime arrived too late to be used
injectRuntimeappended its script before</body>, landing after any inline script the composition carried. Served order was gsap at line 6, the composition's init script at line 20, the runtime at line 37. A probe inside the composition's own IIFE recordedhfTypeAtInit: "undefined"with no variable keys, and the element rendered its hardcoded fallback rather than the declared value.window.__hyperframesonly became an object after load.So the documented API did not exist at the moment authors are told to call it. Two registry blocks had independently worked around it by parsing the authored attribute themselves.
The runtime is designed to load early: its entry assigns
__timelines, installs the authored-opacity capture — whose own comment says it must run while the document is still parsing — and exposes__hyperframessynchronously, deferring real work toDOMContentLoaded. End-of-body injection defeated all three. Nothing in it needs a parsed DOM, so nodeferis wanted.Now injected at head start, reusing the placement cascade
injectScriptsAtHeadStartalready implements rather than adding a fourth copy. Head start rather than the closing tag, so the runtime also precedes author scripts inside<head>.Consumer ledger for the ordering change
injectRuntimehas exactly one consumer repo-wide: theplayserver's composition route.playcomposition routeplaycodec-map injection</head>presentpreview/ studiocheckbrowser gatecheckpassed exit 0snapshot/compare/layout/validateinjectRuntimerender/ producerhtmlCompiler+ the same head-start helper__hfis absentNothing depended on the runtime booting after DOM parse.
Test plan
packages/clipackages/corepackages/playerbun run lintBaselines were taken before any edit, and were fully green, so nothing is misattributed.
The ordering test is mutation-checked, not trusted: reverting the fix to the
</body>form fails exactly one test (expected 12 to be greater than 159) and nothing else.End to end after the fix, same repro:
hfTypeAtInit: "object", variable keys present, element renders the declared value. A real example project underplayexposes the full player API, 5 registered timelines, and 5 distinct screenshot hashes across seeks; two were viewed and show genuinely different composition state.Reviewer note: a fresh worktree needs
bun run buildbeforevitest, or ~68 files fail to collect on missing workspace build output. That failure looks alarming and reproduces on an unmodified checkout.Worth knowing
Fix 1 is latent on
mainand in the published CDN bundle rather than newly broken: no shipped consumer depended on query preservation until compositions began carrying variable payloads.Separately found and not fixed here: several registry blocks declare
data-composition-variablesas an object map, butreadDeclaredDefaultsrequires an array and returns{}for anything else, so those variables are unreadable even with these fixes. Lint does not catch it. That is an authoring-validation gap with its own PR.