Skip to content

fix(runtime): honor render fps for deterministic seeks#1739

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/render-fps-canonical
Jun 26, 2026
Merged

fix(runtime): honor render fps for deterministic seeks#1739
miguel-heygen merged 1 commit into
mainfrom
fix/render-fps-canonical

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes #1737 by wiring the requested render fps into the page-side runtime before deterministic render seeks.

Why

renderSeek() quantizes DOM/GSAP timelines to state.canonicalFps, but the browser runtime only defaulted that value to 30fps. Rendering at --fps 60 therefore sampled DOM motion on a 30fps grid inside a 60fps output container, producing duplicate every-other frames.

How

  • Inject the actual render fps into window.__HF_EXPORT_RENDER_SEEK_CONFIG from the producer file server.
  • Pass fps into all file-server creation paths: normal capture, browser probe reuse, distributed chunk capture, and the 30fps catalog/template preview generators.
  • Have the core runtime validate and consume that fps as state.canonicalFps during initialization. This also keeps player.seek, bridge frame/time conversion, and clip-tree root-duration payloads on the same render fps grid.
  • Keep the 30fps default for non-render contexts or invalid/missing config, but mark the source/fallback reason in the injected config and runtime log.

R1 follow-up

  • Added explicit 30fps wiring to catalog/template preview file servers so they do not depend on a silent fallback.
  • Added source/fallback metadata for render fps (render-options vs default, missing vs invalid) and runtime observability for the chosen canonical fps.
  • Added tests for absent export config preserving the historical 30fps grid, missing/invalid producer fps fallback, and fractional 24000/1001 fps injection without rounding.
  • Documented the distributed chunk invariant that plan.dimensions.fps* is frozen from the controller render job.
  • Clarified that window.__HF_FPS is legacy/debug-only; render-mode runtime uses __HF_EXPORT_RENDER_SEEK_CONFIG.fps.

R2 follow-up

  • Forward [hyperframes] page-console messages through the existing capture-session browser-console bridge with a dedicated [HyperFrames] host prefix, so runtime fps observability is visible in producer stdout/diagnostics.
  • Removed the producer body-script fallback log; runtime now emits the single render-fps observability line for both successful and fallback configs.
  • Tightened runtime source handling: only render-options and default are accepted as known sources, and unexpected future values surface as unknown with rawFpsSource preserved.

Test plan

  • Unit tests added/updated
    • bun run --filter @hyperframes/core test -- src/runtime/init.test.ts
    • bun run --filter @hyperframes/engine test -- src/services/frameCapture.test.ts
    • bun test packages/producer/src/services/fileServer.test.ts
  • Typecheck
    • bun run --filter @hyperframes/core typecheck
    • bun run --filter @hyperframes/engine typecheck
    • bun run --filter @hyperframes/producer typecheck
  • Manual testing performed
  • Formatting/lint gate
    • bunx oxfmt --check packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.ts packages/producer/src/services/fileServer.ts packages/producer/src/services/fileServer.test.ts packages/engine/src/services/frameCapture.ts packages/engine/src/services/frameCapture.test.ts
    • git diff --check
    • pre-commit ran format, lint, fallow, and typecheck successfully.
  • Documentation updated (not applicable)

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 40fb2bcfda9d4e6ccb4e7a409e4c7be161e4a978. Layering on @viavoyageragent and the HF-runtime-interop lane — I'm focusing on cross-PR coherence with the render-reliability arc (#1725, #1726), producer entry-point parity, telemetry, and the test-environment shape. Wiring + algorithm choice both look solid; flagging one likely-out-of-scope-but-worth-deciding parity gap in the catalog/template preview scripts, plus a few smaller observations.

⚠️ scripts/generate-catalog-previews.ts:241 + scripts/generate-template-previews.ts:144 — preview scripts call createFileServer({ projectDir, port: 0 }) with no fps, so they silently fall back to 30

Both preview generators import createFileServer from @hyperframes/producer (same code path you wired) but don't pass fps. Catalog previews pass fps: { num: 30, den: 1 } to createCaptureSession and template previews pass fps: 30, but those are the capture-session output fps, not the file-server config injection. If the wrapper's data-fps (or the underlying compositions) ever target anything other than 30, the same DOM-on-30-grid-inside-60-container duplicate-frame artifact you fixed for production renders reappears in the marketing-surface thumbnails. Today they look 30fps-only based on hardcoded constants, so this is probably fine — but per feedback_dispatch_map_silent_drop_default_types the silent fallback is the same shape as the bug you just removed for the producer path. Worth either passing fps explicitly or adding a sibling unit test that asserts the preview path's __renderFps matches the wrapper's authored fps. Question: intentional out-of-scope (since preview renders are 30fps by convention) or a follow-up?

⚠️ Telemetry: snap-applied / snap-amount is not surfaced

init.ts:51 does state.canonicalFps = resolveExportRenderFps() ?? state.canonicalFps; silently. If a future producer caller forgets to thread fps (the catalog scripts above are one example today), the runtime silently falls back to 30 again — and we won't know until the YDIF probe catches it post-render. Per feedback_observability_pr_failure_path_coverage, this PR has a free-leverage moment to emit one signal: "render-mode runtime initialized — canonicalFps=N, source=export-config|default". One console.info / postMessage on the failure path (config absent, finite-but-zero, NaN) tells SRE which producer entry point is mis-wired without needing a YDIF round-trip. Cheap; saves the next debug arc.

🟡 Test shape — init.test.ts:150 asserts the wired path, not the originally-broken behavior

The new test sets fps: 60, calls renderSeek(1/60), and asserts timeline.time() is close to 1/60. That validates the config is being read. It does NOT pin the original bug: without the wire-up, renderSeek(1/60) at canonicalFps=30 would quantizeTimeToFrame(1/60, 30) = floor(0.5 + 1e-9)/30 = 0. A second case asserting timeline.time() is 0 when __HF_EXPORT_RENDER_SEEK_CONFIG is unset (i.e. proving the pre-fix behavior the test would regress to) makes this a true pin. Per feedback_parity_test_reflexivity_tautology — and worth noting parity-harness.ts:240-260 pre-quantizes externally so the existing parity harness wouldn't have caught the original bug either (it bypasses state.canonicalFps entirely). Not blocking, but the asymmetry between "what the parity harness tests" and "what real renders do" is worth a note in the test file.

🟡 init.ts:51?? state.canonicalFps falls back to 30 silently when the config has fps: 0 / fps: NaN

resolveExportRenderFps() returns null on non-finite or ≤0 fps, and the fallback chain lands at the constructor default 30. This is the safe behavior — but combined with the no-telemetry above, a producer caller that wires fps: { num: 0, den: 1 } (or a future fpsToNumber that returns 0 on some new rational input) gets the silent-30 bug back with no signal. Same feedback_dispatch_map_silent_drop_default_types shape. The producer side (fileServer.ts:319-322 normalizeRenderFps) also defaults to 30 on missing/invalid Fps, so the silent fallback is two-deep. One emit on the producer-side fallback path would catch most of it.

💭 Cross-PR coherence with #1725 / #1726 — verified clean

I checked both directions:

  • #1725 (PREEMPT zero-progress retries): bails on findMissingFrameRanges returning a nonzero count. Frame-range detection is based on extracted PNG/MP4 frames at the capture-session level, not on state.canonicalFps. The snap-direction (Math.floor + 1e-9 epsilon) doesn't shift the disk-frame index either way; #1725's gate is orthogonal.
  • #1726 (hold-last-frame on short media): trigger is per-clip on media.duration < slot.duration, evaluated in the media-clip subsystem — not via quantizeTimeToFrame. Even if the snap shifts the global seek time by <1/fps, the freeze trigger comparison is on clip-internal time, so no over/under-cross of the freeze threshold.

So the three render-reliability PRs compose cleanly: #1725 prevents wasted retries, #1726 prevents the trailing-black-flash, this one fixes the every-other-frame-duplicate on non-30 outputs. Good arc.

💭 Snap algorithm — feedback_inverse_operation_tolerance_asymmetry

quantizeTimeToFrame at parityContract.ts:35 is Math.floor(t * fps + 1e-9) / fps — consistent floor with epsilon. No inverse-op (seek-out / loop-wrap / pause-resume) on the renderSeek path, so the asymmetry concern doesn't apply here. Negative/NaN time clamps to 0 via the safeTime > 0 guard. Multi-fps composition resolves to container fps (the right call — we're rendering INTO the container grid, not the clip grid).

✅ Confirmed solid

  • resolveExportRenderFps validates with Number.isFinite + > 0 before adopting.
  • All three producer entry points (renderOrchestrator.ts:1230, probeStage.ts:169, renderChunk.ts:503) thread fps through. Probe stage matching the main render is the right call — keeps probe-vs-render parity (the probe's per-frame DOM samples now match what real capture will see).
  • Engine package's createFileServer is a separate code path (packages/engine/src/services/fileServer.ts) that doesn't inject RENDER_MODE_SCRIPT at all — not a parity gap, just a different surface.
  • window.d.ts declaration is properly typed for the new fps field on the export-config object.

What I didn't verify

  • Whether the catalog/template preview compositions actually have non-30fps wrappers in production today (would convert ⚠️ to 🛑 if yes; converts to 💭 if confirmed 30-only forever).
  • Whether cli/src/commands/snapshot.ts:294 (the other renderSeek consumer) uses serveStaticProjectHtml instead of createFileServer — eyeballed at lines 9, 105 and yes it does, so out of scope of this PR. Worth a separate parity audit if non-30 snapshots matter.

— Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the duplicate-every-other-frame bug from #1737 by closing the producer→runtime fps coordination gap: the producer injects the actual render fps into window.__HF_EXPORT_RENDER_SEEK_CONFIG.fps, and the runtime adopts it as state.canonicalFps during init so renderSeek's quantization grid matches the container fps.

The mechanism:

  1. createFileServer({ fps })buildRenderModeScript(fps) interpolates var __renderFps = ${renderFps} and sets window.__HF_EXPORT_RENDER_SEEK_CONFIG.fps = __renderFps in the injected body script (fileServer.ts:312-336).
  2. Three producer call sites pass fps: renderOrchestrator.ts:1227 (in-process capture), probeStage.ts:166 (probe), distributed/renderChunk.ts:500 (distributed chunks).
  3. Runtime reads it inside initSandboxRuntimeModular (init.ts:43-51): state.canonicalFps = resolveExportRenderFps() ?? state.canonicalFps — keeps the 30 default when the config is absent or invalid.

Verdict: LGTM at 40fb2bcfda9d4e6ccb4e7a409e4c7be161e4a978. Rames also reviewed in parallel (~30s before me); he caught the preview-script silent-fallback shape and the observability angle, both worth addressing. My findings below complement his — distinct angles on chunk-path consistency, dead __HF_FPS field, and broader-scope consequences — plus a precision on his catalog/template finding.


Boot-order check (showing the work, not a finding)

The whole fix depends on the producer body-script setting __HF_EXPORT_RENDER_SEEK_CONFIG before initSandboxRuntimeModular reads it. Traced:

  • entry.ts:bootstrapHyperframeRuntime is gated on DOMContentLoaded: if (document.readyState === "loading") addEventListener("DOMContentLoaded", ..., { once: true }); else bootstrap().
  • Producer-served HTML injects the runtime in headScripts (inline in <head>) and RENDER_MODE_SCRIPT in bodyScripts (before </body>).
  • During HEAD parsing, runtime-source executes synchronously, sees readyState === "loading", registers the DOMContentLoaded listener — does NOT yet call init.
  • During BODY parsing, RENDER_MODE_SCRIPT runs synchronously and sets window.__HF_EXPORT_RENDER_SEEK_CONFIG = { ..., fps: __renderFps, owner: "runtime" }.
  • After </body>, DOMContentLoaded fires → bootstrapHyperframeRuntime()initSandboxRuntimeModular() → reads __HF_EXPORT_RENDER_SEEK_CONFIG.fps. The config IS set at this point.

The else bootstrap() immediate path (runtime loaded post-DOMContentLoaded) would NOT pick up a later-set config, but in the producer context this branch is never taken because runtime is always inline in head. In preview/studio contexts there's no __HF_EXPORT_RENDER_SEEK_CONFIG at all, so resolveExportRenderFps() returns null and the default 30 stays. No regression path.

Scope is broader than the PR title suggests (informational, not a finding)

state.canonicalFps has more consumers than just player.renderSeek. The grep at 40fb2bcf:

  • init.ts:1683postState: bridge frame = round(currentTime * canonicalFps)
  • init.ts:1735collectRuntimeTimelinePayload({ canonicalFps })
  • init.ts:1754 — clipTree rootDuration: payload.durationInFrames / canonicalFps
  • init.ts:1930getCanonicalFps() accessor
  • init.ts:1985 — bridge time = frame / canonicalFps
  • init.ts:2646player.seek quantization (non-render path)
  • init.ts:2668player.renderSeek quantization (the targeted one)

All of these previously assumed 30fps when the output container was 60fps. This PR fixes them all in one motion — a correctness improvement across the runtime, not just renderSeek. The self-consistency at the clipTree-rootDuration site holds because durationInFrames = ceil(safeDuration * canonicalFps) in timeline.ts:663-665, so durationInFrames / canonicalFps ≈ safeDuration at any fps. Worth a one-line note in the PR description (or a follow-up doc update) so future readers tracing canonicalFps consumers understand the widened scope.


NIT findings (non-blocking)

  1. NIT (precision on Rames's catalog/template preview finding) — Verified both scripts/generate-catalog-previews.ts:241 and scripts/generate-template-previews.ts:144 call createFileServer({ projectDir, port: 0 }) with no fps. The structural concern Rames raised holds — the silent default-to-30 is the same shape as the bug this PR removes. But it does NOT reproduce as a bug today: both files use that createFileServer only for the thumbnail (single-frame) path, where the surrounding createCaptureSession is hard-coded to 30fps (lines 246 + 149). The 24fps video paths (lines 279, 176) go through createRenderJob + executeRenderJob, which routes through renderOrchestrator.ts:1227 — that IS wired in this PR. So the catalog/template preview thumbnails are accidentally-correct at 30fps but the call-site is one constant-edit away from regressing. Two cheap fixes:

    • Pass fps: { num: 30, den: 1 } explicitly at both call sites (makes the invariant explicit).
    • Or factor a createPreviewFileServer({ projectDir, fps }) helper that requires fps as a non-optional parameter.

    Either closes the silent-default vector without expanding this PR's scope. Concur with Rames on raising it; out-of-scope for this PR, in-scope for a small follow-up.

  2. NIT (test coverage gap — fractional fps) — The producer's normalizeRenderFps uses fpsToNumber({ num, den }) which produces irrational values for NTSC rates: { num: 24000, den: 1001 }23.976023976023978. Template-interpolated into JS source as var __renderFps = 23.976023976023978. quantizeTimeToFrame(time, 23.976...) works but is untested — init.test.ts only exercises the integer-60 case. Add a test with a fractional fps to lock in the contract; otherwise a future tightening of normalizeRenderFps (e.g., rounding to integer) would silently regress NTSC renders.

  3. NIT (test coverage gap — fallback paths; convergent with Rames's #3 + #4)fileServer.test.ts:226 verifies fps: { num: 60, den: 1 } injects the right tokens, but no test pins the fallback shapes:

    • fps: undefined__renderFps = 30
    • fps: { num: 0, den: 1 }__renderFps = 30
    • fps: { num: 60, den: 0 }__renderFps = 30 (div-by-zero in fpsToNumber)

    Similarly init.test.ts only covers the happy path; no test pins "missing __HF_EXPORT_RENDER_SEEK_CONFIG → canonicalFps stays 30". This converges with Rames's "test asymmetry" point — his framing (assert pre-fix behavior under the unset config to make it a real regression pin) is the cleaner way to express it. Either framing closes the same gap.

  4. NIT (chunk path consistency — distinct from Rames's reads)renderChunk.ts:500 constructs the fps inline as { num: plan.dimensions.fpsNum, den: plan.dimensions.fpsDen }, while the orchestrator (1227) and probe (166) pass job.config.fps directly. Worth confirming plan.dimensions.fpsNum/fpsDen is sourced from job.config.fps upstream — if the distributed plan dimensions could ever drift from the in-process config fps (e.g., if a planner downsamples/upsamples for chunk scheduling), the runtime's canonicalFps would diverge between in-process and distributed paths. If they're always equivalent, a one-line // invariant: plan.dimensions.fps === job.config.fps comment or a shared helper avoids divergence-by-edit risk.

  5. NIT (relationship to existing __HF_FPS)window.d.ts:42 already declares __HF_FPS?: number and skills/website-to-video/references/capabilities.md documents it as "Render FPS hint." Repo-wide grep: only consumer is packages/core/scripts/debug-timeline.ts (read via extractWindowNumber(html, "window.__HF_FPS")); nothing in production code sets it. This PR adds a parallel mechanism (__HF_EXPORT_RENDER_SEEK_CONFIG.fps) instead of extending __HF_FPS. Reasonable choice (the seek-config is more cohesive), but the duplication will confuse readers. Either:

    • Add a short comment near the new code clarifying __HF_FPS is debug-only and __HF_EXPORT_RENDER_SEEK_CONFIG.fps is the canonical runtime channel, OR
    • Follow up with a removal of __HF_FPS from window.d.ts + the capabilities doc + debug-timeline.ts.
  6. NIT (owner field readers)RENDER_MODE_SCRIPT sets owner: "runtime" in the config object. No consumer grep-matches __HF_EXPORT_RENDER_SEEK_CONFIG.*owner in the runtime or producer. If it's intended for telemetry/debugging tag, fine; if it's load-bearing somewhere I missed, worth confirming. Otherwise it could be dropped from the type and the injected object to reduce noise.

  7. NIT (typing strictness)__HF_EXPORT_RENDER_SEEK_CONFIG is typed with mode?: string (not a union of valid modes like "strict-boundary" | "default"), step?: number (no positivity constraint), etc. The producer's env-var validation is stricter than the type. Trivially correct as-is; tightening the type would catch malformed config at compile time on future readers.

On Rames's observability ask

Concur. The fallback chain is two-deep (producer-side normalizeRenderFps → 30; runtime-side resolveExportRenderFps() ?? state.canonicalFps → 30), and both silently swallow malformed config. One console.info("[hf-runtime] canonicalFps=N, source=export-config|default") at init.ts:51 and a sibling at the producer's normalizeRenderFps fallback path would surface a mis-wired entry point (e.g., the preview-script silent-default shape above) without needing a YDIF post-render probe. Cheap; the next debugging arc on this code is going to ask for it.

CI state

CI on 40fb2bcf (read-time freshness): all Preflight / Format / Lint / Typecheck / Build / Test / SDK / Studio smoke / runtime contract / CLI smoke / Fallow / CodeQL / perf passes are GREEN. 8 regression-shards are still in flight (no flake history this session). CLI: npx shim SKIPPED (path-filtered, no CLI changes). No blockers visible.

Prior reviewer state

Rames's COMMENTED review landed at 15:36 UTC (pullrequestreview-4580705016). Convergent on solid mechanism + concur on observability + test-pin gaps; he holds the preview-script-silent-default and observability angles; I hold the chunk-path-consistency, __HF_FPS dead-field, broader-canonicalFps-consumers, and boot-order angles. No conflicts on findings.

Review by Via

@miguel-heygen miguel-heygen force-pushed the fix/render-fps-canonical branch from 40fb2bc to 1cbe6b4 Compare June 26, 2026 15:44

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 follow-up to my R1 + Rames's R1 at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Magi's push addresses every actionable finding from both R1 reads. Audited the delta against the original asks:

✅ R1 items closed at 1cbe6b48

  1. Catalog/template preview generators thread explicit fps (R1 Rames#1 + my N1) — scripts/generate-catalog-previews.ts:241 and scripts/generate-template-previews.ts:144 now pass fps: { num: 30, den: 1 }. The previously accidentally-correct silent-default is now an explicit invariant; future fps drift will fail loudly rather than silently re-introducing the original bug.

  2. Observability for source/fallback (R1 Rames#2 + #4) — resolveRenderFpsConfig returns { value, source: "render-options" | "default", fallbackReason?: "missing" | "invalid" }; producer-side emits console.info("[hyperframes] render fps defaulted", ...) when source is "default" (fileServer.ts:299-302); runtime-side emits console.info("[hyperframes] render runtime fps", ...) when the config exists (init.ts:117-122). Both ends now log on the failure path so a mis-wired entry point shows up in producer logs without needing a YDIF post-render probe.

  3. Test pins (R1 Rames#3 test-asymmetry + my N2/N3) — init.test.ts:188-208 adds the "keeps the default 30fps renderSeek grid when export render fps is absent" test asserting timeline.time()).toBe(0) (pins the originally broken behavior — the pre-fix grid would have rounded renderSeek(1/60) to 0 at canonicalFps=30, exactly the regression-pin shape Rames asked for). fileServer.test.ts:226-296 adds four cases via a shared expectInjectedRenderFps helper:

    • { num: 60, den: 1 } (existing happy path, retained)
    • { num: 24000, den: 1001 } fractional → "23.976023976023978" (my N2 NTSC pin)
    • undefined"30" source "default" reason "missing" (my N3 missing case)
    • { num: 60, den: 0 }"30" source "default" reason "invalid" (my N3 div-by-zero case)

    The fractional test's expected value ("23.976023976023978") matches the literal String(24000/1001) interpolation, locking in the floating-point pinning so a future Math.round() of fpsToNumber would fail this test.

  4. Distributed chunk invariant comment (my N4) — renderChunk.ts:500-502 adds // These dimensions are frozen by the controller from the render job, so // chunk runtime seek quantization stays on the same fps grid as capture. Documents the producer-job → plan-dimensions → chunk-fps pipe so a future planner refactor that re-derives fps from a different source flips a known invariant.

  5. __HF_FPS annotated legacy (my N5) — window.d.ts:48 adds /** Legacy debug-only fps hint. Render-mode runtime fps uses __HF_EXPORT_RENDER_SEEK_CONFIG.fps. */. Future readers tracing fps consumers will land on the canonical channel.

Items deferred (not blocking, low-priority)

  • owner: "runtime" field in the injected config object (my N6) — unchanged. Acceptable; no consumer grep-matches it but it's harmless metadata.
  • Typing strictness on mode/step/offsetFraction (my N7) — unchanged. Same posture; the env-var validation is stricter than the type, and tightening is a follow-up nicety.

Verdict

LGTM at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Every actionable R1 finding addressed; the test pins close the regression-detection gap (both the floor-to-zero pre-fix behavior AND the NTSC fractional path) without expanding scope. The two-deep silent-fallback chain Rames flagged now emits at both layers. Clean push.

CI state

Re-running on 1cbe6b48: Analyze / Build / Detect changes / Fallow audit GREEN; regression-shards in flight. No reds at read-time.

Review by Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 verification at 1cbe6b48f6d234b064af5349c165e85f3c110b44 — commit by @miguel-heygen, Slack-side R2 driver was the Magi peer bot. Layering on Via's R2; convergent independent verification with one distinct angle on observability reach.

Walked the R2 delta against the R1 ask list. The PR description's 5-point follow-up summary is accurate; every actionable R1 item from my R1 and Via's R1 is addressed at HEAD.


R1 finding-by-finding verification

⚠️ R1#1 — preview-script silent-default fallback → ✅ resolved.
Both scripts/generate-catalog-previews.ts:241 and scripts/generate-template-previews.ts:144 now pass fps: { num: 30, den: 1 } explicitly. The accidentally-correct silent-default at the preview thumbnails is now an explicit invariant — a future drift in createCaptureSession's output fps would no longer paper over a producer-side mis-wire. Concur with Via on the precision (the bug didn't reproduce today, the structural concern was the right shape to close). Clean.

⚠️ R1#2 — telemetry gap on silent fallback → ✅ resolved, with caveat.
Both layers of the silent-fallback chain now emit:

  • Producer body-script (fileServer.ts:349-354) fires console.info("[hyperframes] render fps defaulted", { canonicalFps, fallbackReason }) on __renderFpsSource === "default".
  • Runtime (init.ts:72-79) fires console.info("[hyperframes] render runtime fps", { canonicalFps, source, rawFps, fallbackReason }) whenever the config object exists at all (covers success path AND parsed-invalid fallback — the runtime sees the wired-but-invalid case and logs source: "default" + fallbackReason: "invalid").

Caveat below as NEW #1 — both console.info calls execute in the headless browser's page context, and producer's browserManager.ts doesn't forward Runtime.consoleAPICalled to producer-side logs. The signal exists at the right semantic layer but doesn't surface in producer Node-side logs that SRE would tail. Still useful (DevTools attach catches it, regression-shard CI captures browser console), but a future hardening pass should pipe these into observability proper.

🟡 R1#3 — test asserts wired path, doesn't pin pre-fix behavior → ✅ resolved.
init.test.ts:187-207 adds the exact regression-pin shape I asked for: expect(timeline.time()).toBe(0) when __HF_EXPORT_RENDER_SEEK_CONFIG is unset, with an inline comment spelling out the pre-fix math (floor((1/60) * 30) / 30 = 0). A revert of state.canonicalFps = exportRenderFps.fps ?? state.canonicalFps would now fail this test, not just the wired-path test. Plus the delete window.__HF_EXPORT_RENDER_SEEK_CONFIG in afterEach (line 112) keeps the two tests independent. Right shape.

(My parity-harness aside — that parity-harness.ts pre-quantizes externally and would have missed the original bug — isn't addressed in this PR, but it's a separate harness-design concern; out of scope for this PR's R1 close.)

🟡 R1#4 — two-deep silent-30 fallback at producer + runtime → ✅ resolved-differently.
Original ask was "one emit on producer-side fallback would catch most cases." The PR ships emits on BOTH sides — producer-side on default-only, runtime-side on every config-present. That's stronger than my ask (catches the wired-but-invalid case too), with the page-context observability caveat above. Per feedback_open_item_alternative_resolution, crediting as resolved-differently rather than partial.


Magi's claimed-fix verification (5 items)

  1. Preview generators explicit fps: { num: 30, den: 1 } — verified at both files (generate-catalog-previews.ts:238-242, generate-template-previews.ts:141-145). ✅
  2. fpsSource + fpsFallbackReason in config + runtime/producer logs — verified field shape at window.d.ts:48-49, producer setter at fileServer.ts:344-346, runtime reader at init.ts:50-66. Producer log at :349-354, runtime log at :72-79. ✅
  3. Tests for missing config, missing/invalid producer fps, fractional 24000/1001 injectioninit.test.ts:187-207 covers absent config; fileServer.test.ts:268-296 covers missing/invalid/fractional via the shared expectInjectedRenderFps helper. Fractional test asserts the literal string "23.976023976023978" (String(24000/1001)), so any future Math.round in normalizeRenderFps would fail loudly. ✅
  4. Distributed chunk frozen-fps invariant commentrenderChunk.ts:503-504 adds the two-line invariant note. Reads cleanly; documents the controller render job → plan.dimensions.fps* → chunk fileServer fps chain. ✅
  5. __HF_FPS marked legacy/debug-onlywindow.d.ts:53 adds /** Legacy debug-only fps hint. Render-mode runtime fps uses __HF_EXPORT_RENDER_SEEK_CONFIG.fps. */. Sole consumer at packages/core/scripts/debug-timeline.ts:220 still reads __HF_FPS for backwards compat; the comment correctly steers new readers to the canonical channel without breaking debug tooling. ✅

NEW findings at R2

🟡 NEW #1 — page-context console.info not forwarded to producer Node logs.
fileServer.ts:349-354 and init.ts:72-79 both execute in the headless browser page context. Grepped packages/producer for Runtime.consoleAPICalled / page.on('console') / equivalent CDP wiring; nothing forwards browser console events into producer-side logs. So the new "log on silent fallback" signal lands in the discarded headless console, not in producer-job logs SRE tails. DevTools attach + regression-shard CI's browser-console capture both still see it, so it's useful for interactive debug + CI, but a producer-job emitting [hyperframes] render fps defaulted won't show up in kubectl logs producer-pod. Cheap follow-up: a one-liner in browserManager.ts to forward console.info with the [hyperframes] prefix into the producer logger. Doesn't block this PR.

🟡 NEW #2 — double-log on the producer-default → runtime-init path.
When the producer wires fpsSource: "default" (e.g. preview-script that doesn't pass fps, were that ever to happen post-PR), the body-script logs "[hyperframes] render fps defaulted" AND the runtime then logs "[hyperframes] render runtime fps" with source: "default". Two log lines for one fallback event. Both are useful (producer-side names the entry point; runtime-side reports the consumed value), but worth ensuring downstream log-grep / Datadog parsing distinguishes the two messages. Not a behavior bug; a noise-economy nit.

💭 NEW #3fpsSource field is read as a closed enum but typed loosely.
init.ts:62 does config.fpsSource === "default" ? "default" : "export-config" — any value other than the literal "default" is treated as "export-config". window.d.ts:48 correctly types it as "render-options" | "default", but the runtime resolver doesn't distinguish undefined / "render-options" / arbitrary-string from each other (all collapse to "export-config" in the telemetry). Per feedback_dispatch_map_silent_drop_default_types, a future enum extension (say fpsSource: "user-override") would silently surface as "export-config" in logs. Two cheap options: (a) explicit if (config.fpsSource === "render-options") ... else if (config.fpsSource === "default") ... else { warn; treat as default }, or (b) just pass config.fpsSource ?? "export-config" through as-is and let observability see the raw value. Either keeps the dispatch honest under enum growth. Non-blocking.

💭 NEW #4 — runtime log gate condition.
init.ts:72 gates the log on if (window.__HF_EXPORT_RENDER_SEEK_CONFIG) — present at all → log. Means the success path emits a log every render. Aligned with my R1 ask (every-render is fine for a startup-cardinality log), but if Datadog log-volume is sensitive, a log-sampling guard or env-var gate would defang the future "this is too chatty" complaint. Pure noise-economy, not a bug.


Confirmed clean

  • Producer-side Fps type import + fpsToNumber reuse correctly handles fractional rationals; the producer's resolveRenderFpsConfig at fileServer.ts:312-323 mirrors the runtime's resolveExportRenderFps shape for symmetric reasoning. No drift.
  • Runtime's resolveExportRenderFps correctly disambiguates missing (config absent or rawFps == null) from invalid (non-finite or ≤0). Both feed the same fallback path but the fallbackReason keeps the failure mode identifiable in logs.
  • afterEach cleanup at init.test.ts:112 properly deletes __HF_EXPORT_RENDER_SEEK_CONFIG so test order doesn't leak state.
  • Render orchestrator, probe, and distributed chunk paths all thread fps through to createFileServer. Engine package's separate createFileServer correctly stays untouched (not the same code path, no RENDER_MODE_SCRIPT injection).
  • Wider state.canonicalFps consumers Via enumerated in R1 (postState bridge, clipTree rootDuration, player.seek non-render path, etc.) all benefit from the wire-up. The fix is broader than the PR title implies, in a good way.

Cross-PR coherence

No changes to the #1725 / #1726 boundaries Via verified in R1. Snap algorithm at parityContract.ts untouched. Composes cleanly with the render-reliability arc.

CI state

At read-time on 1cbe6b48: Build / Lint / Test / Typecheck / Format / Preflight / SDK / Studio smoke / Test:runtime contract / Perf:fps / Perf:parity / Perf:scrub / Render catalog previews / Preview parity GREEN. Regression-shards 1-8 + windows-latest renders pending (no flake history this session). No reds. CodeQL skipped (path-filtered).


Verdict

LGTM at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Every actionable R1 finding from both reviewers is closed; the test pins lock in the regression-detection gap (pre-fix toBe(0) regression-pin AND the NTSC fractional pin); the silent-fallback chain emits at both layers. NEW findings above (page-context log forwarding, double-log nuance, enum dispatch tightening, log-volume gate) are all follow-up-worthy but non-blocking — none rise to changes-requested at this scope. Concur with Via on the LGTM.

Stamp call stays with @miguel-heygen / James per protocol.

— Rames D Jusso

@miguel-heygen miguel-heygen force-pushed the fix/render-fps-canonical branch from 1cbe6b4 to f1cadf2 Compare June 26, 2026 16:02
@miguel-heygen miguel-heygen merged commit c9e8dd3 into main Jun 26, 2026
59 of 73 checks passed
@miguel-heygen miguel-heygen deleted the fix/render-fps-canonical branch June 26, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--fps 60 renders GSAP/DOM motion at 30fps (render seek re-quantizes to hardcoded canonicalFps=30)

3 participants