fix(core): install runtime bridge after transport setup#2558
Conversation
7e38b13 to
f38042c
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Summary
Bridge install is now the last step of initSandboxRuntimeModular, gated after every transport helper it dispatches to. Two hot-path helpers (seekTimelineAndAdapters, applyWebAudioRate) are lifted from const arrows to function declarations so hoisting covers their earlier call sites inside the transport object (lines 2161/2215/2237/2254) as well as the bridge closures. The race the fix closes is real and specific: installRuntimeControlBridge in packages/core/src/runtime/bridge.ts:125-131 posts {source: "hf-preview", type: "ready"} immediately after attaching the message listener, and the parent replays queued set-muted / set-volume / set-playback-rate / seek messages on that ready — which under the old ordering could re-enter the runtime before applyWebAudioRate / seekTimelineAndAdapters (both const arrows declared later in the function body) had crossed their TDZ. Fires deterministically in warm-cache reloads and the Claude desktop Electron client (both documented in the bridge comment). New test locks all three axes: value-side (timeline.time() === 2, timeScale called with 2), reachability-side (mock parent replays two control messages synchronously on ready), and idempotency (two inits → exactly two ready outbound messages, exactly two composition_seeked analytics events).
Findings
- Race-window closure verified. Post-fix, the only synchronous-during-init
postStatecalls that emittype: "state"fire at lines 2925-2926, immediately before the bridge install at line 2933. Parent only replays onhf-preview:ready, which is now emitted exclusively byinstallRuntimeControlBridgeat line 131 ofbridge.ts— so no replay path exists until every dispatch target is declared. The otherpostState(true)sites (1614, 1641, 1987/1988, 2182, 2198, 2223, 2243, 2400, 2808, 2815) all live inside reactive callbacks that fire in later ticks, not during the sync init frame. - Const→function conversion is required, not stylistic.
transport.play(line 2161),transport.seek(2215),transport.renderSeek(2237) referenceseekTimelineAndAdaptersbefore its line-2592 declaration;transport.setPlaybackRate(2254) referencesapplyWebAudioRatebefore its line-2899 declaration. Under the oldconstarrow form + the new install ordering, transport callbacks invoked by the player pre-bridge would still hit TDZ. Hoisting closes that too. - No consumer regression on state fields. State fields written by bridge handlers (
bridgeMuted,bridgeVolume,mediaOutputMuted,nativeMediaSyncDisabled,webAudioMediaDisabled,playbackRate) are read at lines 1870-71 and 1892-1904 bypostState, but those reads only happen inside thepostStateclosure, which is invoked at 2925-26 with initial defaults just before bridge install. If the parent had queued aset-mutedbefore the iframe loaded, the initial state message reflectsbridgeMuted: false, then the replayedset-mutedimmediately overwrites — a one-tick UI flicker window that existed pre-fix as well; not a new regression. - Test coverage locks are strong. The
outbound.filter(type=analytics && event=composition_seeked).toHaveLength(2)assertion is a good regression lock: it catches both silent-replay-drop (would go to 0) and duplicate-listener-stacking (would go to 4) on re-init. Theoutbound.filter(type=ready).toHaveLength(2)complements it on the announcement side. - Diff shape matches the fix thesis.
+175/-117decomposes to: init.test.ts+53(new test only); init.ts+122/-117= 117-line block relocated verbatim + 5-line explanatory comment + two arrow→function-declaration site rewrites at zero-sum churn. No embedded redesign hidden inside the churn. - Spec anchor. The 5-line comment at lines 2928-2932 is now the invariant's sole in-repo home. Fine as-is; a
packages/core/src/runtime/README.mdnote calling out "bridge install must be the LAST synchronous step of init" would help future contributors. Non-blocking.
Recommendation
APPROVE. Race is real and specific, fix mechanism is the minimum viable one (relocate + hoist), consumer surface unchanged, test locks value + reachability + idempotency, CI clean at f38042c (all required checks SUCCESS on runs 29533676985 and 29539348222; earlier CANCELLED run 29533490877 was superseded).
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Adversarial pass at HEAD f38042c01f03177e987334664abe72a913df5968. Context: HF#2558 is slice 1/20 of a new keyframe-UI series decomposing the unreviewed golden reference #2387 (OPEN, REVIEW_REQUIRED). Not a byte-identical carve-out from an approved parent — first-look at a slice.
Byte-identity vs #2387 (informational)
Both files DIFFER — init.test.ts and init.ts. Not a byte-for-byte carve-out; targeted refactor standing on its own.
Findings
No blockers. Fix is correct; ordering is verified textually; const-to-function-decl changes are nice hardening.
Concerns (1):
- C1 Outbound-message ordering contract changed for
type:"ready"vsstate/timeline. OLD order: bridge postsreadyearly → many things run →postTimeline()→postState(true). NEW order:postTimeline()→postState(true)→ bridge postsreadyatinit.ts:2925-2933. Any parent-side handler that treatedreadyas "first signal from iframe" now seesstate/timelinefirst. Verified via grep: no consumer oftype:"ready"exists in current codebase — posted-but-never-received. No live regression today, but this slice establishes the ordering as the contract before any consumer lands. Later slices should be aware thatreadynow arrives AFTER the firststate/timelinepost.
Nits (4):
- N1 [informational] The PR describes this as a "race fix", but in real browsers
window.parent.postMessageis asynchronous per spec — the parent's response can't dispatch synchronously to the same tick, soapplyWebAudioRate/seekTimelineAndAdapterswould always be initialized by the time a real control message fires. The test atinit.test.ts:1701-1759simulates the race by makingwindow.parent.postMessagesynchronously dispatchMessageEvents — a JSDOM affordance, not production behavior. Reordering is still worthwhile (belt-and-suspenders + robust to future refactors), but the PR body should be honest that this is preventive rather than a live bug users hit. - N2
not.toThrow()assertions atinit.test.ts:1746-1747are largely decorative —dispatchEventcatches uncaught listener errors and routes them through the DOM "report the exception" algorithm. The load-bearing assertion is thecomposition_seekedanalytics count = 2 on the second seek. Consider spying onwebAudio.setRateor asserting noerrorevent on window to lock the invariant. - N3 The test title says "without duplicate listeners" but the mechanism verified is really "second
initSandboxRuntimeModular()teardowns the first bridge viawindow.__hfRuntimeTeardown" — teardown-on-reinit logic untouched by this PR. Consider explicit assertion onwindow.addEventListener("message", ...)call count. - N4 Listener-leak on install failure at
bridge.ts:125-131—window.addEventListenerruns beforepostRuntimeMessage. If a throw happens between them, listener leaks andstate.controlBridgeHandlerstays null so teardown skips it. Not introduced by this PR; unreachable in practice (postRuntimeMessageswallows own errors). Noting only.
Green surface (verified)
- Init-order correctness —
installRuntimeControlBridgeatinit.ts:2933is the last significant statement beforeteardowndefinition. All handler dependencies verified declared before it:applyPlaybackRateat :2128,growRootDurationLiveat :1970,syncMediaForCurrentStateat :1798,scheduleWebAudioForActiveClipsat :2850,applyWebAudioRateat :2899 (hoistedfunction),seekTimelineAndAdaptersat :2592 (hoistedfunction).player/webAudio/clock/picker/colorGradingall created earlier. No forward references at bridge-install time. - Belt-and-suspenders via function-decl hoisting —
applyWebAudioRate+seekTimelineAndAdapterschanged fromconst arrowtofunction. Neither usesthis; hoisting safe. Runtime resilient to future re-orderings. - Reinit path —
window.__hfRuntimeTeardownatinit.ts:142-149cleans up previous instance before second init. Removes previous message listener viainit.ts:3063-3066. New test exercises this path twice. - Ordering guarantee is trivial — synchronous sequential code inside single function; no promises/events/async. "AFTER transport setup" enforced by textual order.
- No public API changes —
installRuntimeControlBridge+initSandboxRuntimeModularsignatures unchanged. Diff is code-shuffle + 2 arrow-to-function-decl conversions + test addition. Nothing exported changed. - Idempotency confirmed — on second
initSandboxRuntimeModular(), teardown unwires prior listener before new one installs. Test asserts via message counts. - Cross-frame race window empty in production — asynchronous
postMessagemeans bridge is always installed by the time the parent replays controls.
What I didn't verify
- Parent side of the
type:"ready"handshake in #2387 (unreviewed). - Behavior in the Claude desktop Electron client mentioned in
bridge.ts:130("deterministic race on warm-cache reloads") — not reproducible from static analysis. - Interaction with hot-reload/HMR in Studio.
- Full 2,800/853 test-suite claims — only read the new test.
Merge gate
Clean init-order fix. No blockers. C1's ordering-contract change is the one worth naming for downstream slices — the parent-side consumer of ready (which will presumably arrive in a later slice) needs to know state/timeline arrive first.
LGTM from my side — leaving as COMMENTED.
3 participants
Summary
The Studio preview now installs the runtime bridge only after its transport is ready, preventing initialization-order gaps without changing the bridge contract.
Stack
Part 1 of 20. Parent:
main. Next: #2559. The golden reference, #2387, remains open and unchanged.Test plan
Post-Deploy Monitoring & Validation
Validation window: first 24 hours after the stack merges. Owner: Studio maintainers. Watch browser console and support reports for
[Timeline],gsap-parser, failed keyframe mutations, or preview/render easing mismatches. Healthy means edits persist and preview/render agree; revert the first failing layer if authored animation data changes unexpectedly.