fix: stabilize apple master timeline and playback#419
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Solid bug fix with real user-visible impact (Apple deck collapsing to a short master timeline). Diagnosis is correct: init.ts strips data-duration/data-end off non-root compositions for visibility-system parity, but the Studio timeline payload generator and the reference-start resolver were downstream of that strip and saw zeros. The fix preserves authored values in shadow attrs before the strip and teaches the downstream code to fall back. CI green, tests targeted at the regression shape. Approving — four notes below, none blockers, all follow-up-safe.
What's good
- Diagnosis → fix are tight. The three bullets in the PR description map 1:1 to three concrete code changes (
init.tspreservation,startResolver.tsfallback,timeline.tsroot-duration inference). - Reference-start resolver tests cover the real regression shape (
slide-1 → slide-2 → slide-3chain, withdata-start="slide-1"resolution now relying on the preserved duration). - Composition-window floor in root-duration inference is a genuinely nice improvement. Previously
rootDurationFromTimeline ?? mediaWindowDurationCandidate; now also considers authored composition-chain end, andtimelineLooksLoopInflatedwas upgraded to use this too, which makes the loop-inflation guard more robust. parseNumeric/parseNumnow short-circuit onnull/\"\". Small, correct, long overdue —Number(\"\")returning0is part of what caused this bug.
Concerns (non-blocking)
1. AUTHORED_DURATION_ATTR / AUTHORED_END_ATTR constants are defined three times — in init.ts, startResolver.ts, and timeline.ts. These three files form a contract: init writes, the other two read. Three copies means an inevitable future rename/typo will silently break reads. Would extract into a shared module (e.g. runtime/internalAttrs.ts or hanging off ./types) and import. Cheap to do before merging.
2. DOM-as-data-store is a pattern worth questioning. The reason these attrs get stripped is because they feed the visibility system (DOM-reading consumer). The reason this PR has to re-preserve them is because two other consumers (Studio payload, start resolver) also read DOM. A cleaner long-term design would store authored timing in a WeakMap<Element, { duration, end }> inside runtime state at parse time, and have all consumers read from that map — then init.ts could strip public attrs freely with no shadow-attr round trip. Not for this PR, but worth a follow-up: every new consumer now has to remember the public-vs-private rule, and "data goes into the DOM and something later mutates it" is a well-known footgun.
3. The non-loop-inflated root-duration path changed behavior and isn't fully covered by a test.
Before:
preferredRootDuration = attrDuration ?? (timelineLooksLoopInflated ? media : (timeline ?? media))After:
preferredRootDuration = attrDuration ?? (timelineLooksLoopInflated ? max(media, compositionWindow) : max(timeline, media, compositionWindow))For projects where the authored composition chain is longer than the GSAP timeline with no loop inflation, this now extends root duration to the composition-chain end rather than trusting the shorter timeline. Probably the right call for the Apple case — but it's a behavior change for all projects, not only the broken ones. Would add a test asserting:
- (a) non-inflated project where compositionWindow > timelineDuration → root uses compositionWindow ✅
- (b) non-inflated project where compositionWindow < timelineDuration (happy path) → nothing regresses
The new timeline test covers (a) implicitly via durationInFrames === 42 * 30, but the (b) "normal, don't over-extend me" case isn't asserted.
4. Scene-duration fallback calls parseElementEndAttr(compositionNode) twice (around line 583), once for the null check and once for the assignment. Trivial — cache to a local.
Nits
maxDefinedNumber:values.filter((v): v is number => Number.isFinite(v ?? null))— the?? nullis load-bearing for nothing (Number.isFinite(undefined)is alreadyfalse). JustNumber.isFinite(v)reads cleaner.- The
if (authoredDuration != null && !node.hasAttribute(AUTHORED_DURATION_ATTR))idempotency guard ininit.tsmatters ifsanitizeCompositionDurationAttributescan run more than once. Worth a one-line comment saying why the idempotency matters — avoids clobbering with a previously-stripped-to-empty state on HMR or re-init — since the reason isn't obvious at the call site.
Ask before merge: shared-constants extraction (#1). Everything else is follow-up fodder.
* ci(regression): build test Docker image once, share across shards Splits regression.yml into a `build-image` job + the existing `regression-shards` matrix. The build job produces a Docker tarball via `docker/build-push-action` with `outputs: type=docker,dest=...`, uploads it as a GHA artifact (retention 1 day, gzip level 1), and each shard downloads + `docker load`s it instead of rebuilding. Measured on PR #419 regression runs before the change: - Docker build step: ~234s per shard WITH GHA layer cache hit - 11 shards × ~234s = ~43 min of runner time per PR just on redundant image builds Cold-cache cases are much worse — happening right now on PR #419 after release commit b6f50ce bumped every `packages/*/package.json`, invalidating the COPY layer that feeds `bun install --frozen-lockfile`. All 10 shards are currently 25-30+ min into a parallel rebuild, thundering-herding the same npm packages from 10 runners. After this change: - 1× build (~4 min warm, ~15 min cold) + 11× (download + `docker load`) - Expected ~15-20s overhead per shard for artifact download + load - Net savings: ~30-40 min of runner time per PR run on warm cache, substantially more on cold cache The build job doesn't checkout LFS — Dockerfile.test only COPYs source + package manifests, never the golden baselines, so the image build never needed LFS. Shards still need LFS for the tests/**/output/output.mp4 baselines they validate against. * ci(regression): add explicit least-privilege permissions Addresses CodeQL warning 'Workflow does not contain permissions'. Defaults the workflow GITHUB_TOKEN to `contents: read` only. The build-image job elevates to `actions: write` because `docker/build-push-action` with `cache-from/to: type=gha` uses the GitHub Actions cache API, which needs read+write on the actions scope.
74ac422 to
0d726d2
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewing current head 0d726d2 after the force-push — prior approval on 42d69c6 is stale.
Core timing fix — the split design is right
createRuntimeStartTimeResolver({ includeAuthoredTimingAttrs }) being opt-in with a safe false default, and packages/core/src/runtime/timeline.ts:122 being the only call site that opts in (for Studio timeline payload generation), is exactly the isolation needed to keep authored timing out of the runtime resolver. Both sides of the boolean are covered explicitly in startResolver.test.ts ("ignores preserved authored duration by default" vs "resolves preserved authored duration when runtime stripped the public attr"), and the Apple-shape deck scenario in timeline.test.ts ("keeps composition clips sequential when authored durations were preserved privately") asserts durationInFrames === 42*30 end-to-end. The private data-hf-authored-* attr naming is right — no new public DOM surface.
Blockers
1. Scope creep — unrelated changes bundled. The PR declares three goals (authored-timing preservation, Studio dev runtime freshness, runtime-resolver split) and explicitly lists as a non-goal: "does not add general subtimeline authoring support." The following change categories in this diff are outside that scope:
packages/core/src/lint/rules/media.ts+media.test.ts: brand-newimperative_media_controllint rule flaggingvideo.play()/pause()/currentTime=(~136 lines of regex-driven detection). Real feature, zero relation to authored timing.packages/studio/src/player/components/VideoThumbnail.tsx: drops the 6-frame film-strip in favor of single-midpoint-frame rendering. Behavior change for every Studio user's timeline, not just the Apple case.packages/studio/src/player/components/CompositionThumbnail.tsx+App.tsx+sidebar/CompositionsTab.tsx: newseekTime/playbackRateprops plumbed through, dynamic thumbnail time derived fromdata-duration, and a newfetch(\/api/projects/${projectId}/files/${encodeURIComponent(comp)}`)inside auseEffect`. Pure UX.packages/studio/src/player/components/Timeline.tsx: removes the "Copy to Agent" button path and thebuildTimelineElementAgentPromptimport, and adds a double-click drill-down on composition clips. UX + API-surface removal.
Please split: keep the timing fix + studioServer.ts / vite.config.ts runtime-source freshness here, spin the imperative_media_control rule and the Studio thumbnail/timeline UX changes into separate PRs. Bundled, the "what broke / what to revert" surface for this PR is much wider than it needs to be, and the scope-creep bits shouldn't ride along under a fix title.
2. Shared-constants extraction still not done. Held over from my prior review — I flagged this as "ask before merge" and it's still open. Three copies of AUTHORED_DURATION_ATTR / AUTHORED_END_ATTR at packages/core/src/runtime/init.ts:19-20, packages/core/src/runtime/startResolver.ts:3-4, packages/core/src/runtime/timeline.ts:9-10 form a write-and-read contract across three files. Extract into packages/core/src/runtime/internalAttrs.ts (or hang off ./types) and import from the three sites. Five-minute change that closes the future-rename-silently-breaks-reads risk.
3. maxDefinedNumber root-duration — case (b) test still missing. Also held over. The non-loop-inflated branch in timeline.ts now resolves to max(timelineDurationCandidate, mediaWindowDurationCandidate, compositionWindowDurationCandidate) — a behavior change for all projects, not only the broken ones. The Apple case (compositionWindow > timelineDuration) is covered by the new test; the happy-path case (compositionWindow < timelineDuration, should keep using the authored GSAP timeline with no regression) isn't asserted. One test in timeline.test.ts guarding case (b) prevents a future refactor from silently flipping the max direction on healthy projects.
Non-blocking — but look at before merging
- Docker producer CI is in-flight. All
regression-shardsjobs on this SHA (styles-athroughstyles-g,hdr,fast,render-compat) are stillin_progress. Devboxstyle-1/5/9/12-prodpassing is reassuring but it isn't the Dockerized GitHub Actions path (host Chrome/ffmpeg drift vs the pinned container is the exact class of thing the split regression was caught by). Don't merge until shards go green. Preventive: if any committedoutput.mp4golden gets regenerated in follow-up, it must come fromdocker:test:updateonDockerfile.test, not hosttest:update— host/Docker chrome-headless-shell drift has historically tripped PSNR comparisons at dozens of checkpoints. - Runtime-source preference inversion (
loadRuntimeSourceFallback()→ disk-artifact fallback) instudioServer.tsis correct for dev freshness. In a published CLI,loadRuntimeSourceFallback()returning a non-null value would silently bypass the shippedhyperframe.runtime.iife.jsthatbuild-copyput atruntimePath. If that fallback is only source-backed when Vite/monorepo is present, you're fine — but a one-line comment at the call site stating that invariant would save the next reader from having to trace it, and a test pinning the behavior in a CLI-like env would be nice to have.
Graphite
https://app.graphite.com/github/pr/heygen-com/hyperframes/419
Review by hyperframes
0d726d2 to
cb910cd
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Approving per @jrusso1020's direction — this supersedes my earlier CHANGES_REQUESTED on the same head (0d726d2).
Two items from that review intentionally left as non-blocking followups:
- Extract
AUTHORED_DURATION_ATTR/AUTHORED_END_ATTRinto a shared module — currently triple-defined acrosspackages/core/src/runtime/init.ts,startResolver.ts, andtimeline.ts. Keeps the init-writes / resolver-reads contract in one place. - Add a
timeline.test.tsassertion for the non-loop-inflated,compositionWindow < timelineDurationcase so a future refactor can't silently flip themaxDefinedNumberdirection on healthy projects.
The scope-creep flag in the earlier review was a misread on my end — the imperative_media_control lint rule, thumbnail changes, and Timeline drill-down hunks are already on main and showed up in the diff as pre-rebase drift. Withdrawn.
Docker regression-shards were still in_progress at last check — worth waiting on those regardless of review state.
Merge activity
|
Summary
/api/runtime.jsWhat this fixes
This PR fixes the Apple presentation class of failures where the root
index.html/Masterview looked correct at first and then collapsed into an incorrect short timeline.Before this change:
0:12instead of the real deck length (2:21in the Apple project)After this change:
Root cause
There were two related issues:
0instead ofnulldata-duration/data-endwere stripped before Studio timing resolution could use themWhy the later regression fix was needed
The initial runtime change fixed the Apple master timeline, but it also widened timing inference in the core runtime too far. That caused Dockerized producer regressions because rendered visibility started respecting preserved authored timing where it should have relied on the live resolved runtime state.
The latest commit fixes that by splitting the behavior:
That preserves the Apple master timeline fix without changing producer render semantics.
Verification
Local checks
bunx oxlint packages/core/src/runtime/init.ts packages/core/src/runtime/startResolver.ts packages/core/src/runtime/timeline.ts packages/core/src/runtime/startResolver.test.ts packages/core/src/runtime/timeline.test.ts packages/studio/vite.config.ts packages/cli/src/server/studioServer.tsbunx oxfmt --check packages/core/src/runtime/init.ts packages/core/src/runtime/startResolver.ts packages/core/src/runtime/timeline.ts packages/core/src/runtime/startResolver.test.ts packages/core/src/runtime/timeline.test.ts packages/studio/vite.config.ts packages/cli/src/server/studioServer.tsbun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/studio typecheckbun run --filter @hyperframes/cli typecheckcd packages/core && bun run test src/runtime/startResolver.test.ts src/runtime/timeline.test.tsbun test packages/cli/src/server/studioServer.test.ts --timeout 20000Browser proof
Tested in Studio with
agent-browseragainst the Apple presentation project.0:00 / 2:21slide-1 -> slide-2 -> slide-3 ...)120slands on a late slide instead of a collapsed early timeline stateCI-equivalent regression proof on devbox
The previously failing producer regressions were rerun on devbox using the same Dockerized path GitHub Actions uses:
docker build -f Dockerfile.test -t hyperframes-producer:test .docker run ... hyperframes-producer:test style-1-prod style-5-prod style-9-prod style-12-prod --sequentialThose previously failing suites all passed after the runtime split fix:
style-1-prodstyle-5-prodstyle-9-prodstyle-12-prodNotes