fix(studio): align preview fonts with render - #799
Conversation
- Wrap transformPreviewHtml calls in try/catch so a failing transform (e.g. network error during font fetch) degrades gracefully rather than surfacing a 500 to the user - Add test coverage for the bundle()→null (reads from disk) path, which was exercised by the existing code but had no test - Add test coverage for the bundle-throws catch-block fallback path - Add test that verifies graceful fallback to original HTML on transform error
|
It may be better to use some local tools to check the differences between the render and the preview snapshots, and find all the mismatches, instead of relying entirely on the agent to find them through code |
I'll implement screenshots for this btw |
vanceingalls
left a comment
There was a problem hiding this comment.
First review at f0a40240. CI clean (all required green, including Format/Lint/Typecheck/Test/CLI smoke). No prior reviews on this SHA.
Audited
packages/core/src/studio-api/routes/preview.ts:127-187(the transform hook + try/catch fallback)packages/core/src/studio-api/routes/preview.test.ts:146-265(the five new test cases)packages/studio/vite.adapter.ts:157-160,packages/cli/src/server/studioServer.ts:212-216(both adapter wirings)packages/core/src/studio-api/types.ts:55-62(the adapter contract)
Strengths
- Hook contract is right-shaped. Optional callback (
adapter.transformPreviewHtml?),Promise<string> | stringreturn so adapters can be sync or async — matches the existing pattern forbundle()/getProjectSignature()intypes.ts. The single boundary that calls it (transformPreviewHtmlhelper atpreview.ts:127-143) is the right factoring — no scatteredif (adapter.transformPreviewHtml)checks at call sites. - Graceful degradation is enforced at the boundary, not pushed to adapters. The wrapper's
try { await adapter.transformPreviewHtml(...) } catch { return html }(preview.ts:135-141) means adapter authors don't have to defensively wrap their own logic. The testfalls back to original HTML when transformPreviewHtml throws(preview.test.ts:222-241) pins this contract. - Test coverage matches the PR-body table 1:1. All five paths are tested with positive assertions on the injected marker:
- Bundled root preview (
:146-167) - Sub-composition preview (
:169-196) - Bundle returns null → disk fallback (
:198-219) - Bundle throws → catch-block fallback (
:222-241) - Transform throws → preserves original (
:222-241second test)
The marker (<meta name="preview-path" content="...">) is a clean positive-pin assertion shape — not a brittle exact-string check.
- Bundled root preview (
activeCompositionPathplumbing is correct. Root preview path uses"index.html"; sub-composition uses the actualcompPath. Adapters get the context they need without leaking project internals.
Important — duplicate adapter implementation between CLI and Vite
The same body ((await import(... deterministicFonts ...)).injectDeterministicFontFaces(html)) appears in two places:
packages/cli/src/server/studioServer.ts:213-215packages/studio/vite.adapter.ts:157-159
Not a blocker — both adapters legitimately want the same behavior — but factor injectDeterministicFontFaces invocation into a single shared helper (e.g. a withDeterministicFonts(adapter) wrapper or a transformPreviewHtml default export in packages/producer) before a third adapter copy-pastes it. Today the cost is two import sites; tomorrow when font handling changes (new SDK arg, error reporting), it's two patches in lockstep.
Nit
studioServer.ts:213usesawait import("../../../producer/src/services/deterministicFonts.js")whilevite.adapter.ts:158usesawait import("../producer/src/services/deterministicFonts.js"). The dynamic-import path resolves at runtime, not build time, so a folder reorg could break one without breaking the other in lockstep. Consider exportinginjectDeterministicFontFacesfrom a package entry point (e.g.@hyperframes/producer) so both adapters consume the same import specifier.
Verdict
Verdict: APPROVE
Reasoning: Hook contract is well-shaped, the boundary catches and degrades gracefully without leaking error-handling to adapters, all five paths in the PR-body table have positive-pin tests, CI green. The duplicate adapter wiring is a follow-up factor, not a merge blocker.
— Vai
## What Align Studio preview font handling with final render, and harden the transform hook against failures. ## Why Preview and render use different font handling. This bug changes text width and makes text layout look different between preview and final render. ## How - Add a `transformPreviewHtml` hook in `StudioApiAdapter` that adapters can implement to post-process preview HTML before Studio augments it - Use it in both the Vite adapter and the CLI studio server to inject the same deterministic `@font-face` rules that render uses - Wrap the hook in a try/catch so a failing transform (e.g. network error during Google Fonts fetch) degrades gracefully — the preview still loads with the original HTML ## Edge cases covered | Path | Covered | |------|---------| | Bundled HTML (adapter returns string) | ✓ | | Bundle returns null → reads index.html from disk | ✓ | | Bundle throws → catch-block fallback reads index.html | ✓ | | Sub-composition preview | ✓ | | Transform hook throws → graceful fallback to original HTML | ✓ | ## Test plan - [x] Unit tests added for all five paths above - [x] Manual testing performed Closes heygen-com#797
What
Align Studio preview font handling with final render, and harden the transform hook against failures.
Why
Preview and render use different font handling. This bug changes text width and makes text layout look different between preview and final render.
How
transformPreviewHtmlhook inStudioApiAdapterthat adapters can implement to post-process preview HTML before Studio augments it@font-facerules that render usesEdge cases covered
Test plan
Closes #797