refactor(producer): move shared render helpers to render/shared.ts

[jrusso1020]

Stacked on #717 (PR 1.1) → #718 (PR 1.2) → #719 (PR 1.3) → this. Rebase to `main` once #719 merges.

What

Breaks the runtime circular import between `renderOrchestrator.ts` and the stage files under `services/render/stages/`. Adds `packages/producer/src/services/render/shared.ts` and moves four small helpers plus one interface into it:

• `CompositionMetadata` interface
• `applyRenderModeHints`
• `resolveDeviceScaleFactor`
• `writeCompiledArtifacts`
• `projectBrowserEndToCompositionTimeline`

`renderOrchestrator.ts` re-exports everything from `shared.ts`, so external callers (the existing `renderOrchestrator.test.ts` and any other code importing these symbols from the orchestrator) keep working with zero churn.

Why

Before this PR, the stages imported runtime helpers back from `renderOrchestrator.ts`, which itself imports the stage functions. The cycle resolved at build time because both modules finished initializing before any stage was invoked, but it was fragile (any future top-level access to those symbols during module init would TDZ) and would keep growing as more stages were extracted. By PR 1.10 the cycle would be ~8 functions deep.

Land this small intermediate PR now so PRs 1.4–1.10 import from `shared.ts` instead of `renderOrchestrator.ts` and the cycle never grows.

A `/simplify` review on the stack flagged this as the highest-confidence "worth considering" item; the call here is to fix the cycle now rather than ship 7 more stage PRs with a steadily-growing back-edge.

How

• New file `render/shared.ts` containing the four functions + one interface (lifted verbatim from `renderOrchestrator.ts`, including JSDoc and comments).
• `renderOrchestrator.ts`:
  • Removes the four function definitions and the `CompositionMetadata` interface.
  • Adds `export { ... } from "./render/shared.js"` for back-compat.
  • Drops the now-orphaned imports (`CANVAS_DIMENSIONS`, `VideoElement`, `AudioElement`, `ImageElement`) that oxlint flags.
• `compileStage.ts` and `probeStage.ts`:
  • Switch the runtime imports of `writeCompiledArtifacts`, `applyRenderModeHints`, `resolveDeviceScaleFactor`, `projectBrowserEndToCompositionTimeline`, and the `CompositionMetadata` type from `../../renderOrchestrator.js` to `../shared.js`.
  • Keep `import type { RenderJob } from "../../renderOrchestrator.js"` — that one is erased at runtime and creates no cycle.

Preserved invariants

• No behavior change. The moved functions are identical to their previous definitions; only the file path differs.
• Public re-exports from `renderOrchestrator.ts` mean `renderOrchestrator.test.ts` and any external caller keep their existing imports unchanged.

Test plan

• `bunx oxlint packages/producer/src/services/render/ packages/producer/src/services/renderOrchestrator.ts` — clean.
• `bunx oxfmt --check packages/producer/src/services/render/ packages/producer/src/services/renderOrchestrator.ts` — clean.
• `bun run --filter @hyperframes/producer typecheck` — clean.
• `bun run --filter @hyperframes/producer build` — clean.
• `bun test packages/producer/src/services/` — 173 pass, same single pre-existing failure on `writeCompiledArtifacts — rejects a maliciously crafted key` (unrelated and present on `main`).
• `docker run ... hyperframes-producer:test --sequential font-variant-numeric many-cuts variables-prod` — 3/3 PASS with audio correlations 1.000 / 0.994 / 0.975 (identical to PR 1.3).
• Full regression matrix on CI via the `regression` workflow.

🤖 Generated with Claude Code

[jrusso1020]

• refactor(producer): move shared render helpers to render/shared.ts #720 👈 (View in Graphite)
• refactor(producer): extract probeStage from executeRenderJob #719
• refactor(producer): extract compileStage from executeRenderJob #718
• refactor(producer): scaffold services/render/stages/ + planHash utility #717
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

The right call to break the circular import now rather than letting it grow through 7 more stage PRs. The re-export from renderOrchestrator.ts preserves backward compatibility for external callers — zero churn for test files.

The 5 moved items (CompositionMetadata, applyRenderModeHints, resolveDeviceScaleFactor, writeCompiledArtifacts, projectBrowserEndToCompositionTimeline) are pure functions / types with no dependency on the orchestrator's runtime state, so the extraction is clean.

Good that compileStage.ts and probeStage.ts switch to importing from `../shared.js` while keeping `import type { RenderJob } from "../../renderOrchestrator.js"` — type-only imports are erased at runtime and don't form cycles.

LGTM — nice clean stack overall.

[vanceingalls]

Verdict: Approve.

Right call to land this cycle-break now, before PRs 1.4-1.10 each add a back-edge to renderOrchestrator.ts. The /simplify flag was correct.

I verified every helper moved to render/shared.ts is used by ≥2 sites: applyRenderModeHints, resolveDeviceScaleFactor, writeCompiledArtifacts, projectBrowserEndToCompositionTimeline — all consumed by both compileStage.ts and probeStage.ts (and writeCompiledArtifacts additionally by the back-compat re-export from renderOrchestrator.ts). Nothing is moved on a hypothesis of future use. CompositionMetadata is similarly consumed by both stages.

The back-compat re-export at renderOrchestrator.ts:546 correctly keeps renderOrchestrator.test.ts working without churn — I checked the test file and it imports all five symbols from ./renderOrchestrator.js. No external consumer breaks.

The body of each moved helper is byte-identical to the original. I diffed writeCompiledArtifacts, resolveDeviceScaleFactor, applyRenderModeHints, and projectBrowserEndToCompositionTimeline line-by-line against main — comments, JSDoc, error messages, the cross-multiplication aspect-ratio check, the isPathInside Windows fix (GH #321), the includeSummary summary.json payload shape — all preserved.

compileStage.ts and probeStage.ts switch their runtime imports from ../../renderOrchestrator.js to ../shared.js for the four helpers + CompositionMetadata, and keep import type { RenderJob } from the orchestrator. Type-only imports are erased at runtime — no cycle. Correctly handled.

Important

• (important) renderOrchestrator.ts:546-552 — the back-compat re-export is correct behavior-wise, but it doubles the public surface for these symbols (./render/shared.js AND ./renderOrchestrator.js). New code in the stages directory imports from shared.js, but renderOrchestrator.test.ts and any future producer-internal caller could pick either path. Suggest one of:

  1. Add a JSDoc @deprecated Re-exported from ./render/shared.js for back-compat; new imports should use shared.js directly. to the re-export block, plus an oxlint rule for the producer package to flag new imports from renderOrchestrator.ts for these symbols.
  2. Migrate renderOrchestrator.test.ts to import from ./render/shared.js directly in this PR (zero behavior change, drops the re-export entirely).

  Option 2 is the right end-state. Option 1 is a fine intermediate step if you want to keep this PR small. Not blocking, but please don't leave the re-export hanging indefinitely — back-compat shims that outlive their purpose are how trees turn into rainforests.

Nits

• (nit) shared.ts is well-named for what it currently holds (four helpers + one interface), but if it ends up accreting > ~10 symbols across the next six PRs, consider splitting (shared/compositionMetadata.ts, shared/scaleFactor.ts, etc.). Not yet — premature for 196 lines.
• (nit) BROWSER_MEDIA_EPSILON is still duplicated in probeStage.ts:132. If a future stage also reconciles browser media (chunked rendering will likely re-probe), hoist it to shared.ts. Easy follow-up.
• (nit) shared.ts header comment mentions "the orchestrator imports the stage functions" — true today, but if you ever flip the dependency direction so stages own their own composition, this comment will mislead. Phrase it as "to keep stages free of runtime imports from the orchestrator" instead.

Praise

• The proactive cycle-break is correct engineering judgment. The cycle worked because module-init order happened to resolve it, but TDZ-on-future-top-level-access was a real latent risk. Fixing it after one stage extraction (not seven) is exactly the right time.
• Removing the now-orphaned CANVAS_DIMENSIONS, VideoElement, AudioElement, ImageElement imports from the orchestrator is good housekeeping.
• The PSNR/audio-correlation parity check (1.000 / 0.994 / 0.975, matching #719 exactly) is strong evidence of "moved, not changed".

Stack-coherence summary across #717-#720: the stage interface (typed input object → typed async result object, resources returned to sequencer for cleanup, no orchestrator imports at runtime) is settling into a clean contract. After this PR, a chunkStage would plug in cleanly with one caveat — the cfg.forceScreenshot mutation in compileStage (flagged on #718) needs to move into the result object before the freeze step can be replay-safe. Otherwise the foundation is good.

— Vai

[jrusso1020]

Thanks @vanceingalls @miguel-heygen — addressed all three items in commit 89d83fbb:

• Re-export removed: went with Vai's option 2 (the clean end-state). Migrated renderOrchestrator.test.ts to import the five symbols (applyRenderModeHints, projectBrowserEndToCompositionTimeline, resolveDeviceScaleFactor, writeCompiledArtifacts, CompositionMetadata) directly from ./render/shared.js, then deleted the back-compat re-export block from renderOrchestrator.ts. Verified via grep across packages/ that no other consumer went through the orchestrator path. The symbols now have exactly one import path.
• BROWSER_MEDIA_EPSILON: hoisted into shared.ts, imported back from there in probeStage.ts. Any future stage that reconciles browser media gets it for free.
• Header comment in shared.ts was left as-is — it's accurate now and the wording isn't load-bearing if the dependency direction ever flips; future-you can adjust then.

Renderer smoke-tested inside Dockerfile.test again — audio correlations 1.000 / 0.994 / 0.975, matching every prior PR in the stack.

Stack-coherence note: the cfg.forceScreenshot mutation in compileStage (which you raised in #718) now has a TODO(distributed-render): comment marking the spot where it needs to move into the result type before freezePlan wires up. Acknowledged here too — agreed it's the foundation work before chunked rendering can be replay-safe.

The base branch was changed.

[miguel-heygen]

Re-approving after new commits. Same verdict — clean cycle-break, back-compat re-exports correct, helpers byte-identical to originals. LGTM.

[vanceingalls]

Re-stamp at 89d83fbb.

My prior approval (pullrequestreview-4266433387 on 2925f24e) was dismissed by the branch protection rule after the new push. Verified the new commits address my prior important follow-up:

• 89d83fbb — "migrate test imports to render/shared.ts, drop re-export" — exactly what was asked for. The back-compat re-export shim I flagged ("the only internal consumer was a test file — don't let the shim outlive its purpose") is now removed, with the test file updated to import directly from render/shared.ts. Clean closure on the import path.
• Stack rebase on top of the now-landed #717-#719 pulled the stage scaffolding into #720's diff (shared.ts +204, stages/compileStage.ts +121, stages/probeStage.ts +369, renderOrchestrator.ts -495). All scoped to the moved helpers + stage extractions from the parent PRs — no new logic introduced in this PR.

— Vai