refactor: dedupe resolution presets and clean up 4k stack

[jrusso1020]

What

Post-review cleanup of the 4K rendering stack (#660–#664). De-duplicates resolution constants/aliases/normalizer across 5 packages, simplifies useRenderQueue.startRender to an options object, removes verbose JSDoc, and drops a redundant parallel Map in the engine's frame cache.

Stacked on #664.

Why

Three review agents (reuse / quality / efficiency) flagged the same handful of issues:

• RESOLUTION_ALIASES + normalizeResolutionFlag were duplicated verbatim in cli/init.ts and cli/render.ts. The init.ts copy had already drifted (added a redundant "portrait-4k": "portrait-4k" self-mapping).
• VALID_RESOLUTIONS set/array appeared in 4 places (two CLI files, the studio-api route, and an inline string-union in studio-api/types.ts).
• useRenderQueue.startRender(fps, quality, format, resolution) had grown to 4 positional params, all defaulted, all string-shaped — fragile.
• The engine's byte-budgeted LRU tracked sizes in a parallel Map even though cache.get(key).length returns the same value. Real allocation pressure during a 4K render with thousands of frames.
• Two RenderConfig/RenderOptions JSDoc blocks restated resolveDeviceScaleFactor's contract verbatim.
• The resolveDeviceScaleFactor JSDoc had a confusing parenthetical about non-integer scales that contradicted itself (cited 720p → 4K as non-integer when 720×3 = 2160 is integer).
• Output dimensions were computed twice in executeRenderJob (once for log, once for perf summary).

How

Core (packages/core/src/core.types.ts, index.ts):

• Add canonical VALID_CANVAS_RESOLUTIONS (derived from Object.keys(CANVAS_DIMENSIONS)) and normalizeResolutionFlag() next to CANVAS_DIMENSIONS. Re-exported from the core barrel.
• 2 new tests in index.test.ts covering the new exports, including alias resolution and case-insensitivity.

CLI (init.ts, render.ts):

• Replace ~50 lines of duplicate constants/functions with import { normalizeResolutionFlag, ... } from "@hyperframes/core".

Studio-API route (render.ts):

• VALID_RESOLUTIONS now derives from VALID_CANVAS_RESOLUTIONS instead of hard-coded.
• Inline string-union type in studio-api/types.ts replaced with CanvasResolution.

Studio (useRenderQueue.ts, RenderQueue.tsx, App.tsx):

• startRender now takes StartRenderOptions instead of 4 positional args. Existing call site simplifies from (format, quality, resolution) => renderQueue.startRender(30, quality, format, resolution) to (format, quality, resolution) => renderQueue.startRender({ format, quality, resolution }).
• ResolutionPreset kept local (4 string literals) with a load-bearing comment explaining why — studio's tsconfig doesn't include node types and the core barrel transitively pulls in modules with node:fs imports. Adding a ./types core subpath would be more invasive than the duplication is worth.
• Dropdown labels now consistently mark orientation: 1080p ↔ / 1080p ↕ / 4K ↔ / 4K ↕ (was: only the portrait variants had a marker).

Engine (videoFrameInjector.ts):

• Drop the parallel sizes: Map<string, number> — derive byte size from cache.get(key)?.length on demand. Removes one Map allocation + per-insert/per-evict bookkeeping. Adds a JSDoc note about the bytesLimit < single-entry edge case being prevented by the 64MB floor on bytesLimitMb.

Producer (renderOrchestrator.ts):

• Compute outputWidth / outputHeight once after resolveDeviceScaleFactor and reuse for both the supersample log and RenderPerfSummary.resolution.
• Fix the misleading non-integer example in resolveDeviceScaleFactor JSDoc.
• Trim the RenderConfig.outputResolution JSDoc from 16 lines to 4 (the constraints live on resolveDeviceScaleFactor already).

Trim applyResolutionPreset JSDoc — drop the bullet list of which fields get rewritten (the function body shows that). Keep the load-bearing "regex not DOM" rationale.

Test plan

• Unit tests added/updated:
  • 2 new core export tests for VALID_CANVAS_RESOLUTIONS + normalizeResolutionFlag (covers alias resolution, case-insensitivity, undefined).
• Manual testing performed:
  • bun run --cwd packages/core test — 685/685 pass (2 new)
  • bun run --cwd packages/cli test — 283/283 pass
  • bun run --cwd packages/studio test — 271/271 pass
  • bun run --cwd packages/engine test — 541/541 pass
  • Producer bunx vitest — 50/51 (1 pre-existing failure on main, unrelated)
  • All packages typecheck clean
• Documentation updated — N/A; this is a pure refactor with no user-facing changes.

[jrusso1020]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(engine): supersample screenshot capture to honor deviceScaleFactor #666
• refactor: dedupe resolution presets and clean up 4k stack #665 👈 (View in Graphite)
• feat(studio): add resolution selector to render export bar #664
• feat(cli): add --resolution flag to hyperframes render for one-line 4k #663
• fix(engine): byte-budget the frame data uri cache to bound memory at 4k #662
• feat(cli): add --resolution flag to hyperframes init for 4k scaffolding #661
• feat(core): add 4k canvas resolution presets #660
• main

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

[vanceingalls]

Verdict: approve. Genuine cleanup. The dedupes are real, the dropped parallel Map in the engine cache is a measurable allocation win, and the useRenderQueue.startRender options-object refactor is overdue (4 positional defaulted params is a footgun). Behavior preservation looks right based on the tests.

Important

• important — packages/engine/src/services/videoFrameInjector.ts:54-69 — Dropping the parallel sizes Map is the right call, but the new code reads cache.get(oldestKey)?.length ?? 0 before the cache.delete(oldestKey). Correct today (the value is still in the map when we read it), but easy to break if someone reorders. Worth a one-line comment, or invert: const dropped = cache.get(oldestKey); cache.delete(oldestKey); totalBytes = Math.max(0, totalBytes - (dropped?.length ?? 0)); — same effect, harder to misread.

• important — packages/engine/src/services/videoFrameInjector.ts:38-44 — JSDoc acknowledges the bytesLimit < single-entry edge case ("the post-insert eviction loop will drop the entry we just inserted") and points to the 64 MB floor as protection. As I noted on #662: 64 MB is not enough for a worst-case 4K PNG (which can exceed 50 MB encoded). Either #662 fixes the underlying invariant before #665 lands, or this PR's JSDoc claim ("safely cacheable") is incorrect. Either fix the floor in #662 or soften the JSDoc here to "in normal operation" rather than "safely."

• important — packages/core/src/core.types.ts:31-33 — VALID_CANVAS_RESOLUTIONS = Object.keys(CANVAS_DIMENSIONS) as readonly CanvasResolution[] — the cast is doing real work. Object.keys returns string[]; the cast trusts that the keys exactly match CanvasResolution. Today they do because both are derived from the same source, but if anyone adds a key to CANVAS_DIMENSIONS and forgets to update CanvasResolution, the cast silently lies. Either:

  • Use satisfies to invert the trust: keep the explicit array and satisfies readonly CanvasResolution[] so TypeScript verifies it matches.
  • Or define CanvasResolution as keyof typeof CANVAS_DIMENSIONS (single source of truth).

  The second is cleaner — the type extends automatically when a preset is added.

• important — packages/studio/src/components/renders/useRenderQueue.ts:14-18 — The local ResolutionPreset type with the load-bearing comment ("studio's tsconfig doesn't include node types... core barrel transitively pulls in modules with node:fs imports") is a real architectural smell, but I get why this PR isn't the place to fix it. Worth filing a follow-up ticket: @hyperframes/core should have a ./types (or ./constants) subpath export with zero runtime/node deps, which removes the duplication permanently. Calling this an architecture nit, not a #665 blocker.

Nits

• nit — packages/core/src/core.types.ts:31-33 — VALID_CANVAS_RESOLUTIONS uses Object.keys which has no guaranteed order across JS engines (in practice V8 preserves insertion order for string keys). Tests at index.test.ts:15-22 pin the order — fine, but if anyone reorders CANVAS_DIMENSIONS, they'll get a test failure that looks like an unrelated bug. Worth a comment: "ordering matches insertion order in CANVAS_DIMENSIONS — change with care."

• nit — packages/studio/src/components/renders/RenderQueue.tsx:20-31 — Adding ↔ to the landscape labels makes orientation explicit (good), but ↔ is a horizontal-arrow glyph that some monospace fonts render oddly. 🖥️ / 📱 are unambiguous but emoji-y. Fine to leave; just a UX note.

• nit — packages/producer/src/services/renderOrchestrator.ts:2128-2130 — Hoisting outputWidth/outputHeight to a single computation site is the right cleanup. Only used in two places (log + perf summary) — fine. If RenderPerfSummary.resolution is consumed by metrics downstream, double-check that the change from "composition dims" → "output dims" doesn't break dashboards. From the JSDoc on #663 ("RenderPerfSummary.resolution reports the output dims, not composition dims, so telemetry stays correct") it sounds intended — just confirming.

• nit — packages/cli/src/commands/render.ts:524-525 — JSDoc trim is fine; the contract really does live on resolveDeviceScaleFactor. But the previous version was helpful for readers tracing CLI → orchestrator without jumping packages. A one-line "see resolveDeviceScaleFactor in producer" pointer is what you have — that's enough.

Cross-PR / Stack hygiene

This PR being needed at all is a stack-hygiene smell, and the team should know it: three review agents flagged the same duplications, which means #661 and #663 shipped near-identical code that had already started to drift ("portrait-4k": "portrait-4k" self-mapping in init.ts, missing in render.ts). The right move would have been:

1. #660 lands CanvasResolution, CANVAS_DIMENSIONS, and VALID_CANVAS_RESOLUTIONS + normalizeResolutionFlag — i.e., everything that's pure data + pure functions. The latter two are 10 lines of code; landing them in the foundation costs nothing.
2. #661 and #663 import the helpers from core. No duplication, no drift.
3. #665 doesn't exist (or is a much smaller PR for the JSDoc cleanups + parallel-Map drop only).

The pattern to internalize: if a helper is going to be needed by N consumers, land the helper with the foundation, not in the first consumer. This stack would have been 4 PRs instead of 5, and the merge order would have been more flexible. I'd note this in the next retro.

That said, given the duplication did happen, this PR is the correct way to clean it up — and the discipline of including dedicated tests for the new core exports (VALID_CANVAS_RESOLUTIONS, normalizeResolutionFlag) is right.

Praise

• The options-object refactor of useRenderQueue.startRender is a real ergonomic improvement. Positional booleans + defaults are the kind of thing that breaks subtly six months later when someone reorders.
• The parallel-Map drop in videoFrameInjector is the kind of tiny perf win that adds up across a million-frame render — good catch.
• Honest, thorough PR description (acknowledging the dedup smell rather than papering over it). That signal is valuable.

— Vai

[miguel-heygen]

Approving this refactor layer itself.

I did not find a new correctness issue in the #665 diff against #664. The shared CanvasResolution / normalizeResolutionFlag cleanup is the right direction, and the focused top-of-stack verification passed after bootstrapping generated runtime artifacts:

• bun test packages/core/src/index.test.ts packages/core/src/studio-api/routes/render.test.ts packages/producer/src/services/renderOrchestrator.test.ts packages/engine/src/services/videoFrameInjector.test.ts packages/cli/src/commands/init.test.ts packages/cli/src/utils/dockerRunArgs.test.ts
• bun run --filter @hyperframes/studio typecheck
• bun run --filter @hyperframes/cli typecheck
• bun run --filter @hyperframes/core typecheck
• bunx oxlint on the touched files

Caveats before the stack lands: this PR inherits the lower-layer blockers I requested changes on (#661, #663, #664), and several #665 regression shards were still pending when I rechecked. My approval is for the refactor layer, not for landing the full stack as-is.

I rechecked the live head before posting: 1753b493f854eac35b393afcd2a26e3f9fa9ca4c.

[vanceingalls]

Re-review — APPROVE

Verdict shift: COMMENT (1 important) → APPROVE.

Reference: prior review #665 (review)

Addressed since prior review (commit b39bb8d0)

• Important — type derivation from canonical table: packages/core/src/core.types.ts:31 — export type CanvasResolution = keyof typeof CANVAS_DIMENSIONS;. Adding a row to CANVAS_DIMENSIONS now extends the union automatically; the prior as readonly CanvasResolution[] cast on VALID_CANVAS_RESOLUTIONS no longer hides drift because the source-of-truth is the table. Inline comment explains why.
  • The cast on VALID_CANVAS_RESOLUTIONS = Object.keys(CANVAS_DIMENSIONS) as readonly CanvasResolution[] is still present, but now it's a runtime-list-of-keys-of-table — the type is derived from the same table, so drift is structurally impossible. Order is pinned by tests in index.test.ts per the comment. This matches the prior review's recommendation.
• JSDoc 64 MB claim corrected: packages/engine/src/config.ts:79-92 — the misleading "64 MB safely caches 4K" wording is gone. New doc explains the budget is sized so a worker stays "comfortably under a few GB even at 4K (where each PNG frame is ~25 MB and the base64 data URI is ~33 MB)" — accurate. The Math.max(64, …) floor in resolveConfig is now just an absolute hard floor without the misleading explanation.
• Dedupe achieved at the right surfaces: init.ts and render.ts both import normalizeResolutionFlag from @hyperframes/core — no local copies. studio-api/routes/render.ts imports VALID_CANVAS_RESOLUTIONS from canonical core types.

Still open

None of the prior items.

New findings (nit)

• useRenderQueue.ts:14 keeps a local ResolutionPreset copy with an explicit JSDoc explaining studio's tsconfig avoids the core barrel (node types). Documented exception — fine.
• vite.config.ts:80 keeps an inline literal union for the createRenderJob return type. Same justification (Vite SSR loader avoids importing the module it dynamically loads). 4th drift surface, but documented intent. Not blocking — both are stable and the canonical source is now firmly in core.types.ts.

— Vai