Skip to content

refactor(producer): document executeRenderJob as a thin sequencer#735

Open
jrusso1020 wants to merge 2 commits into
05-12-refactor_producer_extract_encodestage_and_assemblestagefrom
05-12-refactor_producer_document_executerenderjob_as_a_thin_sequencer
Open

refactor(producer): document executeRenderJob as a thin sequencer#735
jrusso1020 wants to merge 2 commits into
05-12-refactor_producer_extract_encodestage_and_assemblestagefrom
05-12-refactor_producer_document_executerenderjob_as_a_thin_sequencer

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 12, 2026

Stacked on #725#726#730#731#733#734 → this. Last PR of Phase 1.

What

Final polish PR of the Phase 1 stack. Comment-only — zero code change.

After the prior eight extraction PRs, executeRenderJob now composes eight stage modules instead of inlining the pipeline. This PR updates the file-level JSDoc to point at each stage module and explains the orchestrator's residual responsibilities. Also adds JSDoc on executeRenderJob itself.

Why

Closing-out cleanup of Phase 1. The orchestrator is now a thin sequencer over the eight stage modules; the comment surface needs to reflect that so the next reader (or the next set of PRs from Phase 2) doesn't have to reconstruct the architecture from import statements.

The "thin sequencer (~150 lines)" goal in the original Phase 1 plan was aspirational — the realistic floor without further extraction is ~880 lines. The remaining lines are the in-sequencer concerns that don't naturally compose into a stage:

  • Work-dir / output-path / debug-logger setup
  • Calibration, worker resolution, preset selection, HDR auto-detection
  • The try/finally resource ownership (file server, capture sessions, frame-data caches, memory sampler)
  • Final perf-summary assembly + error diagnostics

These could be further factored in Phase 2 if useful, but the Phase 1 contract was "no behavior change" — and the call sites that orchestrate stages are already as thin as they can be.

How

  • File-level JSDoc updated to enumerate the eight stage modules and their roles.
  • New JSDoc on executeRenderJob summarising its contract (returns once outputPath exists; throws on cancellation / stage failure with a diagnostic summary).
  • No source changes outside doc comments.

Phase 1 stack summary

PR Stage extracted LOC moved
#725 extractVideosStage (+ materializeSymlinks param) ~120
#726 audioStage ~25
#730 captureStage (SDR disk) ~95
#731 captureStreamingStage (single-machine fusion) ~160
#733 captureHdrStage (Z-ordered layered composite) ~745
#734 encodeStage + assembleStage (Stage 5 + Stage 6) ~100
#735 doc polish (this PR) 0

Plus earlier PRs (#717, #718, #719, #720): planHash scaffold, compileStage, probeStage, render/shared.ts cycle break.

executeRenderJob went from ~2,200 lines pre-Phase-1 to ~880 lines now — and the 880 are all sequencer / setup / cleanup, no inline stage bodies.

Test plan

  • bunx oxlint packages/producer/src/services/renderOrchestrator.ts — clean.
  • bunx oxfmt --check — clean.
  • bun run --filter @hyperframes/producer typecheck + build — clean.
  • bun test packages/producer/src/services/ — 176 pass, same single pre-existing unrelated failure on main.
  • Docker fixtures (4/4 PASS): font-variant-numeric (1.000), many-cuts (0.994), variables-prod (0.975), hdr-regression (1.000). Audio correlations identical to every prior PR in the Phase 1 stack.
  • Full regression matrix on CI via the regression workflow.

🤖 Generated with Claude Code

Final polish PR of the Phase 1 stack. Comment-only — zero code change.

After PRs #725, #726, #730, #731, #733, #734, the `executeRenderJob`
function now composes eight stage modules instead of inlining the
pipeline. Updates the file-level JSDoc to point at each stage module
and explains the orchestrator's residual responsibilities: shared
resource lifetime, perf counters, error diagnostics, and the
`try/finally` cleanup. Adds JSDoc on `executeRenderJob` itself
summarising what it returns and when it throws.

The function body is unchanged. The line count dropped from ~2,200
(pre-Phase-1) to ~880; the remainder is in-sequencer setup that doesn't
naturally compose into a stage (calibration, worker resolution, HDR
auto-detection, preset selection, final perf-summary assembly) plus
the orchestrator's `try/finally` resource ownership.

Verified inside `Dockerfile.test`: font-variant-numeric (1.000),
many-cuts (0.994), variables-prod (0.975), hdr-regression (1.000) —
4/4 PASS with audio correlations identical to every prior PR in the
Phase 1 stack.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment + interface cleanup driven by the /simplify review. No code
change beyond removing dead fields.

- audioStage: drop unused `job: RenderJob` from `AudioStageInput` (the
  stage destructures it but never references the value).
- encodeStage: drop unused `fps` + `useGpu` from `EncodeStageInput`;
  read both from `job.config.*` inside the stage (matches the pattern
  used by captureStage and captureStreamingStage).
- captureStreamingStage: drop the unused `captureDurationMs` field
  from `CaptureStreamingStageResult` (sequencer never reads it — it
  uses its own `Date.now() - stage4Start` for `perfStages.captureMs`).
  Also drops the now-dead `streamStart` local.
- captureStreamingStage: rewrite the "Known follow-up" header comment
  to drop the "PR 1.3.5" reference per `feedback_no_internal_track_names_in_source`.
- captureHdrStage: drop the "Lifted verbatim from `executeRenderJob`"
  refactor-narration sentence in the header doc (the "Hard constraints
  preserved verbatim" list below it is real long-term documentation
  and stays).
- Sequencer call sites updated to drop the now-removed fields.

Verified inside `Dockerfile.test`: 4/4 fixtures pass with PSNR / audio
correlations unchanged (font-variant-numeric, many-cuts, gsap-letters,
hdr-regression).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Verdict: approve.

Documentation + sequencer-cleanup PR. Three things happen:

  1. The file header on renderOrchestrator.ts is rewritten to document executeRenderJob as a thin sequencer over the eight stage modules, listing each stage with its file path. The stages are listed in execution order — useful for new readers.
  2. A JSDoc block is added immediately above executeRenderJob documenting the function as the in-process entry point.
  3. Three stage inputs that turned out to be dead code are removed: audioStage's job: RenderJob, encodeStage's fps: Fps and useGpu: boolean | undefined (both already available via job.config), and captureStreamingStage's captureDurationMs from the result (the sequencer always recomputed Date.now() - stage4Start itself, so the field was dead).

The cleanup tightens the interfaces but doesn't change behavior. Verified each removed field had exactly zero consumers on the sequencer side.

captureHdrStage.ts's header comment loses the "Lifted verbatim from executeRenderJob" line — the lineage is documented in the PR history, no need to carry it forever. Fine.

Nits:

  • The removed captureDurationMs field from CaptureStreamingStageResult should have been dropped in #731 instead of carried forward — see the #731 review nit. Folded in here is fine; future PRs in the stack should aim to land the stage-input cleanup in the same PR that introduces the stage.
  • The file-header block now lists stages 1, 1b, 2, 3, 4 (three files), 5, 6 — that's eight files for six "logical" stages. The "Stage 4" line spans three files (captureStage.ts, captureStreamingStage.ts, captureHdrStage.ts) which is accurate but mildly confusing in a single-glance read. A one-line note like "Stage 4 has three implementations selected by render mode" would help.

Praise: the file-header rewrite is the right artifact at the right time — once the sequencer is actually a sequencer, the docs should say so. The pre-stack header (1. Parse composition metadata → 6. Final assembly (audio mux + faststart)) was describing what executeRenderJob used to be; the new header describes what it is now. This is the kind of "while I was here, also kept the docs honest" change that's almost always worth landing.

— Vai

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Clean mechanical extraction — no behavior changes, no introduced bugs. Verified imports, error handling, and cleanup invariants are preserved. LGTM. — Magi

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.

3 participants