fix: harden CLI edge-case repros#591
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Big PR — solid intent on the dedup, but blocking a stamp on one regression that breaks studio test startup on a clean checkout. Walking through what I see:
What's good
The refactor is well-motivated. The producer's services/timingCompiler.ts literally carried a comment "Keep this aligned with core timing compiler behavior" — explicit duplicate that's now gone (-212 lines). The centralize shared html helpers commit nets -118 lines across 21 files; the engine and producer each lost ~60-85 lines of duplicated fileServer.ts logic. Real cross-package consolidation, not just syntactic.
Each of the 6 closed issues (#584-590) has a fix with a test. Mapping:
| Issue | Fix landing site | Test |
|---|---|---|
| #590 GSAP infinite-repeat false-positive on JS comments | lint/rules/gsap.ts:stripJsComments (state-machine, preserves strings/template literals) |
gsap.test.ts:"does not error on repeat: -1 inside JavaScript comments" |
| #589 Template-wrapped sub-comp 0 elements | cli/commands/compositions.ts:parseSubComposition (uses template.content when present) |
compositions.test.ts:"reads template-wrapped sub-composition contents" |
| #588 Contrast NaN | contrast-audit.browser.js viewport bounds-check + sample bounds-check |
(browser-eval; covered by sandbox tests) |
| #587 Snapshot/validate viewport | cli/utils/compositionViewport.ts (NEW) |
compositionViewport.test.ts |
| #586 Muted video + audio StaticGuard | htmlBundler.ts runtime bootstrap script protection |
htmlBundler.test.ts:"does not merge author scripts into the runtime bootstrap placeholder" |
| #584 hf-seek listeners unreachable during capture | runtime/init.ts |
(covered by init.test.ts cases that were updated) |
The blocker
Studio test suite fails to start on a clean checkout with this PR.
@hyperframes/studio test: failed to load config from packages/studio/vite.config.ts
@hyperframes/studio test: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts"
for packages/core/src/studio-api/helpers/screenshotClip.ts
Root cause: the centralize shared html helpers commit added a value-level import to packages/studio/vite.config.ts:
import { getElementScreenshotClip } from "@hyperframes/core/studio-api/screenshot-clip";The new @hyperframes/core/studio-api/screenshot-clip export resolves to .ts source (./src/studio-api/helpers/screenshotClip.ts). When vitest reads vite.config.ts, it goes through Node's regular module loader before any TS transform, and Node can't load a .ts file directly. The type-only import next to it (import type { ScreenshotClip }) is fine because types erase, but the value import doesn't.
Symmetric proof:
bun run --filter @hyperframes/studio testonorigin/main: ✅ 256/256 passes (19 files)- Same command on PR head: ❌ test runner crashes at config load (0 tests run)
- Same command after I commented out just the
getElementScreenshotClipvalue import invite.config.ts: ✅ 256/256 passes again
Two ways to fix
- Inline
getElementScreenshotClipback intovite.config.ts. It's ~15 lines; losing the dedup for this one function (which exists only to bepage.evaluate'd, so it has to be self-contained anyway) is way cheaper than breaking studio test startup. - Move the helper to a code path that's pre-built to
.js(or expose it from adist/path so the .ts isn't on the package's import surface). More architectural; could be deferred to a follow-up.
Suggest (1) for this PR.
Smaller smells worth flagging (non-blocking)
- Bundling 6 fixes in one commit (
b0e77b8d) makes bisection impossible. The visibility-clamp change inruntime/init.tsin that commit was unintentional (none of the 6 closed issues touch composition-host visibility) and required a follow-up commitbca99a0d "restore sub-composition visibility clamp"to revert. Per-issue commits would have made this catch-and-revert trivial; bundled, it surfaced as a behavior change buried inside an unrelated fix commit. Future bug-bash PRs would benefit from one-commit-per-issue. - Miguel's PR description says "5 issues" but closes 6 (the
Closes #584-#590list — six issues). Minor copy fix. - Other test suites pass cleanly:
@hyperframes/core601/601,@hyperframes/cli196/196,@hyperframes/engineclean. So the regression is studio-only.
tl;dr
LGTM as a refactor and as a bug-fix bundle for #584-590. Blocked on the studio test regression — fix the vite.config.ts import (suggest inlining getElementScreenshotClip) and I'll restamp.
— Review by Rames Jusso
|
@jrusso1020 addressed the Studio startup blocker in
|
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Blocker resolved — re-stamping. Fix commit 72bacf4f "fix: avoid source import in studio vite config" applies option (1) from my prior review: removed both imports of @hyperframes/core/studio-api/screenshot-clip from packages/studio/vite.config.ts, declared a local ScreenshotClip interface, and inlined the helper body into page.evaluate. Clean.
The shared getElementScreenshotClip in packages/core/src/studio-api/helpers/screenshotClip.ts is correctly preserved — it's still used by production studio-api routes through the runtime path; the inline duplicate only lives in dev-time vite.config.ts where it has to be self-contained for page.evaluate anyway. ~15 LOC duplication for a clean Node-loader-vs-TS-source boundary is the right trade.
Verified locally:
@hyperframes/studiotest: 19 files / 256/256 tests pass (was crashing on config load before)@hyperframes/core: 601/601 still pass@hyperframes/cli: 196/196 still pass
— Re-review by Rames Jusso
3 participants
Problem
I reproduced the selected open issue batch one by one and confirmed the reports were valid. The fixes all touch the CLI/runtime capture boundary, then the follow-up regression run exposed one over-broad runtime change in sub-composition host visibility and one CI-only baseline trap.
Closes #590, #589, #588, #587, #586, and #584.
What this fixes
CLI/runtime edge cases
repeat:-1text in comments is not flagged.<template>content, count visual-only template descendants, estimate simple GSAP durations, and suppress rootdata-startwarnings in sub-composition lint mode.<head>when source HTML has no head.data-width/data-heightinstead of falling back to 1920x1080.null:1/NaN:1.data-has-audio="false"in the core timing compiler, which fixes the same-src muted<video>+ separate<audio>StaticGuard case.hf-seeklisteners reachable during capture by preventing author scripts from being merged into the runtime bootstrap path.Shared helper cleanup
.tssource.Regression hardening
</script >.<script>tags, so authored</scriptand<!--text cannot break out of the injected wrapper script.vfr-screen-recordingvideo golden to the CI-stable baseline; the current CI failure showed the Linux render matches the old golden, while the locally refreshed macOS golden was the mismatch.Root cause
The CLI paths had accumulated assumptions that held for simple direct-root landscape compositions but not for current composition patterns: DOM queries did not enter template content, snapshot/validate used a fixed viewport, runtime and author scripts shared a coalescing bucket, and timing compilation treated every video as audio-bearing unless authors manually overrode it.
The style shard failures were not product regressions. Local and CI media probing disagreed on the short SFX clip duration by about 45 ms, and the compiler was clamping authored durations to the locally probed value. The shared clamp tolerance preserves explicit author/source durations for small probe precision differences while still clamping real overflows.
The vfr fast-shard failure was a bad baseline refresh: CI actual frames matched the old
vfr-screen-recordingbaseline at 40+ dB PSNR, but mismatched the macOS-refreshed golden at ~18-22 dB. The fix is to keep the Docker/Linux-stable video golden and only retain the deterministic compiled snapshot change.The sub-composition regression came from treating a host's authored parent window as the only visibility boundary. That made settled child overlays stay visible after their own live GSAP timeline ended. The corrected runtime behavior respects both contracts: parent clips still bound where the host can appear, and the child live timeline can end the host earlier.
Verification
Local checks
bunx oxfmt --check packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.tsbunx oxlint packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.tsbun run --cwd packages/core test src/runtime/init.test.tsbun run --cwd packages/cli test src/commands/compositions.test.ts src/utils/compositionViewport.test.tsbun run build:hyperframes-runtimebun run --cwd packages/producer test --keep-temp --sequential style-12-prod style-5-prodbun run --cwd packages/producer test --sequential vfr-screen-recording hdr-hlg-regression style-7-prodbun run --cwd packages/core test src/compiler/htmlCompiler.test.ts src/compiler/timingCompiler.test.ts src/index.test.tsbunx oxfmt --check packages/core/src/compiler/timingCompiler.ts packages/core/src/compiler/htmlCompiler.ts packages/core/src/compiler/htmlCompiler.test.ts packages/core/src/compiler/index.ts packages/core/src/index.ts packages/core/src/index.test.ts packages/producer/src/services/htmlCompiler.tsbunx oxlint packages/core/src/compiler/timingCompiler.ts packages/core/src/compiler/htmlCompiler.ts packages/core/src/compiler/htmlCompiler.test.ts packages/core/src/compiler/index.ts packages/core/src/index.ts packages/core/src/index.test.ts packages/producer/src/services/htmlCompiler.tsbun run --cwd packages/core typecheckbun run --cwd packages/producer typecheckbun run --cwd packages/producer test --sequential style-1-prod style-9-prodbun run --filter @hyperframes/studio testwithpackages/core/disttemporarily hidden to simulate clean-checkout config loadinggit diff --checkCI artifact checks
25225854394job73969147096: style-1 failed only onclick-sfx1.044898vs1duration/end.25225854394job73969147061: style-9 failed only on SFX1.044898-based duration/end mismatches.25225854394job73969147048:vfr-screen-recordingcompilation/audio passed, visual failed after comparing against the macOS-refreshed golden.40.444705, above the fixture threshold of28.Repro checks
bun packages/cli/src/cli.ts lint /tmp/hf-590-repronow passes withoutgsap_infinite_repeat.bun packages/cli/src/cli.ts snapshot /tmp/hf-587-repro --at 0.5 --timeout 1000now writes a 1080x1920 PNG.bun packages/cli/src/cli.ts validate /tmp/hf-588-repro --timeout 500no longer emitsnull:1/NaN:1contrast output.bun packages/cli/src/cli.ts validate /tmp/hf-586-repro --timeout 500 --contrast falseno longer emits the muted-video StaticGuard contract error.bun packages/cli/src/cli.ts compositions /tmp/hf-589-gsap-repronow reportsfoo 0.5s 1920x1080 1 element.bun packages/cli/src/cli.ts snapshot /tmp/hf-589-gsap-repro --at 0.25 --timeout 2000captures the expected template-backed red frame.bun packages/cli/src/cli.ts snapshot /tmp/hf-584-repro --at 0.5,1.5 --timeout 500captures the expected post-seek green frame.Browser verification
qa-artifacts/pr-591-video-compare/index.html.agent-browserto loadstyle-12-prod, play both videos quickly to the failed window, pause, and inspect the side-by-side frame.qa-artifacts/pr-591-video-compare/browser-proof/fixed-style12-labeled.png.qa-artifacts/pr-591-video-compare/browser-proof/fixed-style12.webm.qa-artifacts/dedupe-refactor-preview.png,qa-artifacts/dedupe-refactor-preview-after-play.png,qa-artifacts/dedupe-refactor-preview.webm.Notes
.tsimports are transformed.vfr-screen-recording/output/compiled.htmlbut no longer changesvfr-screen-recording/output/output.mp4relative tomain.linux/amd64Docker validation to mirror CI, but the local Docker build was blocked by Debian package download failures. The arm64 Docker image also cannot launch the x64 Puppeteer headless shell under OrbStack. The vfr baseline decision is therefore based on the uploaded CI artifact comparison above.