feat(player): add retained runtime data channels - #3471
Conversation
a2482fa to
4470f78
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at exact head 4470f787fbde59157e2a96d9f6296c7daeeab70f. No other reviews or comments existed on this PR when I started, so nothing here is additive to a peer — it is a first read.
Verdict: approving. The feature is correct end to end and I verified the wiring at source rather than from the diff. Three follow-ups below, none blocking.
What I verified (negative results, so nobody re-derives them)
- The same-origin fast path is genuinely wired, not aspirational.
_trySetRuntimeDataDirectprobescontentWindow.__hyperframes, andpackages/core/src/runtime/entry.ts:32-39really does publish{fitTextFontSize, getVariables, pretext, registerRuntimeDataHandler, setRuntimeData, clearRuntimeData}on that exact global at script-evaluation time — beforeDOMContentLoaded, so it is present ahead of thereadymessage that flips_runtimeBridgeReady. Cross-origin throws on thecontentWindowread and thecatchfalls through topostMessage, which is the right shape. - The
RuntimeTimelineClipsplit is shape-identical. Comparing member sets rather than counts:Identitytakes{id, label, start, duration, track, kind, tagName, compositionId, parentCompositionId, compositionSrc, assetUrl}andRuntimeTimelineClip = Identity & {zIndex, stackingContextId, compositionAncestors, nodePath, playbackStart, playbackRate, timelineRole, timelineLabel, timelineGroup, …}— the union is the pre-PR type exactly, includingassetUrlmoving from the tail intoIdentity.ClipManifestClip extends RuntimeTimelineClipIdentityalso preserves required-vs-optional per field: the eleven it inherits were all required before, and the five it keeps local (zIndex,stackingContextId,compositionAncestors,playbackStart,playbackRate) were all optional before and still are. No widening in either direction. _runtimeBridgeReadyis reset on every path that can swap the document —srcandsrcdocinattributeChangedCallback,_onIframeLoad, and the disconnect reset. I looked for a fourth and did not find one.- CI is green on the real gates. hyperframes
mainrequires 8 contexts via ruleset14211637(Build,Render on windows-latest,Semantic PR title,Test,Test: runtime contract,Tests on windows-latest,Typecheck,regression). All 8 pass at this head. Thecancelledduplicates in the check-runs list are superseded runs from the earlier push (32780752382/32780752420/32780752475), not failures — the authoritativepull_requestruns are32780789520,32780789559,32780789483, allsuccess.Skills: *,Catalog:,Codex plugin package,CLI: npx shimandGCP BeginFrame image contractareskippedby path filters and are not in the required set.
1. runtime-data is advertised as a capability but nothing ever consults it — so version skew fails silently
protocol.ts adds "runtime-data" to RUNTIME_PROTOCOL_CAPABILITIES. That constant is referenced in exactly one file in the repo: its own definition. And capabilities appears zero times anywhere under packages/player — the player calls inspectRuntimeProtocol(data) and consumes protocol.status and protocol.fps, never the capability array.
Pair that with the bridge's dispatch (bridge.ts:130-131):
const fn = CONTROL_HANDLERS[action];
if (fn) fn(data, deps);An unknown action is dropped with no reply. So a player carrying this change, pointed at a runtime bundle that predates it, does this: setRuntimeData("captions", …) → _runtimeBridgeReady is true → _trySetRuntimeDataDirect returns false (no setRuntimeData on the old __hyperframes) → postMessage("set-runtime-data") → old bridge has no handler → nothing. The parent keeps the value in _runtimeData, replays it on every source swap, and never learns it is talking to a runtime that cannot receive it.
The reason this is worth raising rather than shrugging at: this PR added runtime-data-error precisely so this class of failure is observable, and it is structurally blind to the most likely instance of it. The error reporter lives inside runtimeData.ts — the module the old bundle does not have — so it can only report exceptions thrown by a handler that already registered. The one failure where the data never arrives at all is the one nothing reports.
Not a blocker: within a single deployed version there is no skew, and this is added-only code, so nothing that worked stops working. But the player and the runtime bundle version independently, which is what makes it reachable in production rather than theoretical.
Sizing the fix honestly — it is not a one-liner. inspectRuntimeProtocol already parses the capability list, so the work is one callback out of handleRuntimeMessage (the shape setRuntimeFps already uses) plus one check at onRuntimeReady: if runtime-data is absent and _runtimeData is non-empty, dispatch runtimedataerror with a skew code instead of replaying into the void. If you would rather not carry that now, the cheap alternative is a sentence in the PR body saying the capability is declarative for the moment and skew is undetected, so the next reader does not assume the gate exists.
2. Neither channel validator is pinned, and the two disagree on what invalid means
runtimeData.ts and hyperframes-player.ts both gate on /^[a-z][a-z0-9-]{0,63}$/, and they do opposite things with a failure:
- core
setRuntimeData/clearRuntimeData:if (!validChannel(channel)) return;— silent no-op - core
registerRuntimeDataHandler: throws - player
setRuntimeData/clearRuntimeData: throws
So the same channel string is a thrown Error through the player API and a silent discard through the in-iframe global, and a malformed channel arriving over the bridge is dropped without emitting the runtime-data-error this PR just added. I do not think the choice is wrong — not throwing on postMessage input is defensible — but three sites and two behaviours want a sentence saying which is intended.
The pinning half is concrete: no test in this PR passes an invalid channel to either module. The four runtimeData.test.ts cases use "captions" and "telemetry"; the six new player cases use "captions". Replace validChannel's body with return true and all four core tests stay green. Taking the regex operator by operator, nothing exercises uppercase ("Captions"), a leading digit ("1captions"), the empty string, a trailing hyphen, or a name past 64 characters — so the anchors, the character class and the length bound are all unconstrained. Two added cases would fix it.
Related and cheap: that regex is written out three times across two packages with nothing linking the copies (once in validChannel, twice inline in the player). Whichever one is edited first, the others drift silently and no test on either side fails.
3. The two delivery paths differ in aliasing
setRuntimeData structured-clones on ingress, so the caller cannot mutate what the player retained. Good. But the direct path then hands the guest that retained object by reference:
bridge.setRuntimeData(channel, payload); // payload === the retained clonewhile the postMessage path clones again on the wire. A same-origin guest that mutates the object it receives is therefore mutating what _replayRuntimeData() will hand the next document after a source swap — and it behaves differently from the cross-origin case, which is immune. One structuredClone at egress in _trySetRuntimeDataDirect, or an Object.freeze on the retained copy, closes it. Nit — the guest is your own runtime — but the two paths should not have different mutation semantics when the whole point of the clone was isolation.
Small things
- The body says "443 changed lines across 14 files"; the API lists 17. The line count is exact (422 + 21 = 443); only the file count is off. 12 source + 5 test files.
- The five added lines in
packages/player/vitest.config.tsare the only change here that does not trace to runtime data. They are legitimate —parent-media.ts:14imports@hyperframes/core/composition-contract, which isexport * from "@hyperframes/parsers/composition-contract", so both hops need mapping, and it matches the two subpath aliases already in that file. Worth one line in the body saying whether the player suite needed these onmainalready or whether the new test is what first pulled the chain into the graph, because as written they read as unrelated. resetRuntimeDataForTestsalso resetsreportError, which is what keeps the error-reporter test from leaking into its neighbours. Deliberate and easy to lose in a later refactor — worth the comment it does not have.
Part 2 (#3472) is where this contract gets a consumer, so the producer/consumer seam is worth checking against a live payload rather than against the types on whichever PR lands second.
— Rames Jusso (James's assistant — not James, despite the shared account)
Part 1 of 2 in the HyperFrames caption convergence stack.
What changes
Review size
422 additions, 21 deletions; 443 changed lines across 14 files.
Verification
The full Node 22 player suite passes all 349 tests on the stack top.