refactor(core): extract shared studio API module#113
Conversation
f12d1f6 to
f0692ed
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Code Review: refactor(core): extract shared studio API module
Overall Assessment
This is a well-structured extraction that cleanly separates host-specific concerns from shared API route logic via the StudioApiAdapter interface. The adapter pattern is a solid choice here. CI is green on all core checks (build, typecheck, lint, tests). The PR description is honest about follow-up work, which is appreciated.
What follows is organized by severity.
Critical Issues
1. Missing mkdirSync in the shared file-write route
The original studioServer.ts ensures the parent directory exists before writing a file:
// Original (studioServer.ts, line ~314-316)
const dir = dirname(file);
if (!existsSync(dir)) mkdirSync(dir, { recursive: true });The new packages/core/src/studio-api/routes/files.ts omits this entirely -- writeFileSync will throw ENOENT if the user creates a file in a directory that does not yet exist (e.g., compositions/new-scene.html where compositions/ has not been created). This is a behavioral regression.
Fix: Add the mkdirSync call (and the dirname import) back into the shared files.ts write route, since this is host-agnostic behavior that both consumers need.
Important Issues
2. Telemetry dropped from CLI render path
The original studioServer.ts calls trackRenderComplete and trackRenderError from ../telemetry/events.js with detailed perf metrics (fps, quality, workers, docker, gpu, compositionDurationMs, speedRatio, captureAvgMs, capturePeakMs, memory). The refactored startRender adapter implementation in studioServer.ts has none of this.
This is intentional in the sense that telemetry is CLI-specific and should not live in the shared module, but the adapter's startRender implementation should still call the telemetry. This looks like it was accidentally lost during the extraction, not deliberately removed.
Fix: Re-add the telemetry calls to the CLI adapter's startRender implementation.
3. Module-level mutable singleton: renderJobs map in routes/render.ts
The renderJobs Map is declared at module scope:
const renderJobs = new Map<string, RenderJobState>();This means every caller of createStudioApi() shares the same job store. If two different server instances are created (e.g., in tests, or a hypothetical multi-tenant setup), their render jobs would collide. The map should either live on the Hono app instance (via c.set/c.get or Hono's Variables generic), or be scoped inside registerRenderRoutes and captured in a closure.
The companion setInterval TTL cleanup at module scope compounds this -- it runs unconditionally on import (modulo the environment check) and has no way to be cleared, which will keep the Node process alive in test environments and leak timers.
Fix: Move the renderJobs map into registerRenderRoutes as a closure variable, and either remove the setInterval or return a cleanup function.
4. TTL cleanup key parsing is fragile
now - parseInt(key.split("-").pop() || "0") > 300_000Job IDs are formatted as ${project.id}_${datePart}_${timePart} (e.g., myproject_2026-03-28_02-36-48). Splitting on - and taking .pop() yields "48", which parseInt converts to 48 -- not a timestamp. This TTL logic will essentially never evict anything (48ms is always < 300000ms ago relative to Date.now()). The intent seems to be a staleness check, but it needs the actual creation timestamp, not a substring of the time component.
Fix: Store a createdAt: Date.now() field on RenderJobState and compare against that.
Suggestions (Nice to Have)
5. Duplicate MIME_TYPES map in CLI's studioServer.ts
The refactored studioServer.ts still defines its own inline MIME_TYPES map inside a local getMimeType function (used for SPA static file serving). The shared module now exports getMimeType from @hyperframes/core/studio-api. The CLI should import and reuse it to avoid the duplication.
6. resolveProject is async in routes but sync in CLI adapter
The adapter type correctly allows Promise<T> | T return types, so both work. However, the routes all await the result. This is fine functionally, but it means every route handler pays the microtask overhead even for the CLI's synchronous case. Not a real performance concern, just worth noting that the design intentionally supports both async (Vite multi-project scanning) and sync (CLI single-project) adapters.
7. baseHref hardcodes /api/ prefix in preview routes
const baseHref = `/api/projects/${project.id}/preview/`;The shared module assumes it will be mounted at /api, but the mounting is done by the consumer. If a consumer mounts it at a different prefix, these URLs break. Consider making the base path configurable on the adapter, or documenting that the module must be mounted under /api.
8. Sub-composition builder change in behavior
The original buildSubCompositionHtml performed recursive inlining of nested data-composition-src references. The new implementation in helpers/subComposition.ts does not -- it reads the sub-composition, strips <template>, and wraps it in the project's <head>. This is a deliberate simplification (the old inlining was complex and fragile), but it is a behavior change for compositions with nested sub-compositions. The PR description should call this out.
9. walkDir uses string concatenation instead of join
files.push(...walkDir(`${dir}/${entry.name}`, rel));This works on macOS/Linux but is not fully cross-platform. Using join(dir, entry.name) from node:path would be more robust and consistent with the rest of the codebase.
10. The stage field on progress SSE
The original CLI progress handler did not emit a stage field. The new shared render progress route emits stage: current.stage. This is additive and fine, but worth confirming the studio frontend handles the extra field gracefully (it likely does since it just ignores unknown keys in the progress JSON).
What Was Done Well
- The
StudioApiAdapterinterface is clean, well-documented, and correctly uses union return types (Promise<T> | T) for flexibility. - Route modules are well-organized into separate files by domain (projects, files, preview, lint, render, thumbnail).
- The
isSafePathandwalkDirhelpers are properly extracted with theIGNORE_DIRSimprovement. - The
honopeer dependency is correctly marked as optional inpeerDependenciesMeta. - The
publishConfigexports map is correctly updated for both dev and dist paths. - CI passes on all core checks.
Verdict
Requesting changes due to the missing mkdirSync in the file write route (#1), which is a correctness regression that will cause errors for users creating files in new directories.
Issues #2 (telemetry), #3 (singleton render jobs map), and #4 (broken TTL logic) are important and should ideally be addressed before merge, but I would accept them as tracked follow-ups given the PR description already acknowledges this is an incremental refactor with follow-up work planned.
vanceingalls
left a comment
There was a problem hiding this comment.
Requesting changes for the missing mkdirSync in the shared file write route (packages/core/src/studio-api/routes/files.ts). See detailed review comment above.
vanceingalls
left a comment
There was a problem hiding this comment.
Review: refactor(core): extract shared studio API module
Critical
Missing mkdirSync in file write handler — packages/core/src/studio-api/routes/files.ts
The PUT handler calls writeFileSync but doesn't create parent directories first. The original studioServer.ts had mkdirSync(dirname(filePath), { recursive: true }) before the write. Without it, writing to a new subdirectory throws ENOENT.
Important
-
Telemetry dropped from render —
packages/cli/src/server/studioServer.tsno longer callstrackRenderComplete/trackRenderErrorafter the extraction. These were in the original CLI adapter's render flow. -
Singleton
renderJobsMap with leaked timer —packages/core/src/studio-api/routes/render.tshas a module-levelrenderJobsMap andsetIntervalfor TTL cleanup that's shared across allcreateStudioApi()calls and can never be cleaned up. -
TTL cleanup parses job IDs incorrectly — The cleanup logic extracts a timestamp from job IDs but the parsing yields wrong values (e.g.,
48instead of a real timestamp), so jobs are never actually evicted.
Verdict
Requesting changes for the missing mkdirSync (will cause file write failures) and the dropped telemetry.
d4f502a to
ef0597c
Compare
9c1ebaa to
206f2c6
Compare
Addressed review feedbackThanks for the thorough review @vanceingalls! All issues addressed in the latest push: Critical — Fixed
Important — Fixed
Suggestions — Fixed
Acknowledged (no change needed)
|
ef0597c to
3c61465
Compare
206f2c6 to
89e22ef
Compare
3c61465 to
a762f46
Compare
89e22ef to
e051b17
Compare
a762f46 to
c741fd5
Compare
e051b17 to
3f65cce
Compare
3f65cce to
d99cfca
Compare
beefcb9 to
f44a316
Compare
910d51b to
7085ca1
Compare
f44a316 to
faccb37
Compare
3b76ee2 to
0a1a093
Compare
faccb37 to
229f62d
Compare
0a1a093 to
d551be4
Compare
229f62d to
31701fe
Compare
d551be4 to
2a2a676
Compare
31701fe to
69a4de3
Compare
2a2a676 to
87a108d
Compare
69a4de3 to
8a2af77
Compare
87a108d to
126b1b6
Compare
8a2af77 to
029329e
Compare
126b1b6 to
327d0c5
Compare
029329e to
9356f4e
Compare
327d0c5 to
348d260
Compare
9356f4e to
37bbab3
Compare
91c73ec to
c807643
Compare
37bbab3 to
a57d95f
Compare
a57d95f to
99f4d36
Compare
c807643 to
fecd4c8
Compare
Create @hyperframes/core/studio-api — a Hono-based shared API module that both the vite dev server and CLI embedded server can mount. Architecture: - StudioApiAdapter interface: consumers inject host-specific behavior (project resolution, bundling, rendering, thumbnails) - Shared route modules: projects, files, preview, lint, render, thumbnail - Shared helpers: isSafePath, walkDir, getMimeType, buildSubCompositionHtml This module contains all API route logic in one place. Both consumers will be refactored to mount this module with their own adapter: - Vite: SSR module loading, Puppeteer thumbnails, producer HTTP proxy - CLI: in-process rendering, local runtime, single project Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
99f4d36 to
644ad12
Compare
Merge activity
|

Summary
Extracts all studio API routes into a shared Hono-based module at
@hyperframes/core/studio-api.Architecture
StudioApiAdapterinterface — consumers inject host-specific behavior (project resolution, bundling, rendering, thumbnails)isSafePath,walkDir,getMimeType,buildSubCompositionHtmlWhat this PR does
vite.config.tsandstudioServer.tsWhat stays in each consumer
executeRenderJob, local runtime serving, browser management, SPA static file servingFollow-up needed
packages/studio/vite.config.tsto usecreateStudioApi(adapter)via@hono/node-server'sgetRequestListenerpackages/cli/src/server/studioServer.tsto usecreateStudioApi(adapter)./studio-apiexport path topackages/core/package.jsonhonoas peer dependency of@hyperframes/coreTest plan
🤖 Generated with Claude Code