fix(fonts): unify preview and render font resolution - #3492
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
ed5bc37 to
d6de083
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
PR state: HEAD d6de0834 · REVIEW_REQUIRED, MERGEABLE · CI green across all required check names (Build, Typecheck, Lint, Format, Preview parity, Producer unit/integration, Windows Render, all 9 regression shards, CLI smoke, CLI npx shim on ubuntu/macos/windows, Studio load smoke, CodeQL, File size, Fallow audit, SDK unit/contract/smoke). One Windows render verification workflow shows CANCELLED (runs/32918250043) — same HEAD SHA as the SUCCESS run (runs/32918276370), so this is a concurrency-group supersede of a duplicate trigger, not a real failure. On-PR reviews: none yet (GET /pulls/3492/reviews returns []); the only issue comment is Mintlify's preview bot. Magi's ask arrived on Slack, not on the PR — no peer-post to layer on top of.
The code does what the PR body says at the level that matters for the reported incident: extractGoogleFontsText now unions every source character with its String#toUpperCase()/toLowerCase() variants before the 1,700-byte URL gate (packages/producer/src/services/deterministicFonts.ts:1201-1217), so a mixed-case source under text-transform: uppercase no longer drops uppercase glyphs from Google Fonts' text= subset. The new hyperframes-localize-fonts binary reuses the exact injectDeterministicFontFaces binding via @hyperframes/producer in packages/cli/src/fontLocalizeCli.ts:2 — that same symbol is what the render path calls (packages/producer/src/index.ts:121, packages/cli/src/server/studioServer.ts:418, packages/cli/src/utils/bundleWithLocalizedFonts.ts:44-48). It's a shared binding, not a re-implementation, and tsup.config.ts:66-72 inlines producer into the CLI bundle, so downstream consumers can't drift by pinning a stale producer.
Blockers — none.
Concerns 🟡
• Locale-sensitive case gaps in the blanket closure. packages/producer/src/services/deterministicFonts.ts:1209 uses locale-independent String#toUpperCase()/toLowerCase(). That's correct for the reported Latin/English incident, but a browser rendering text-transform: uppercase on lang="tr" produces İ (U+0130) for i — JS returns I. Modern German orthography renders ß as ẞ (U+1E9E) under uppercase — JS returns SS. Neither of these will land in the text= subset today. This is a scope note, not a bug against the reported title, but the PR body's phrasing "covers CSS uppercase, lowercase, and capitalize without incident-specific logic" over-promises the closure's exhaustiveness. Worth an inline comment acknowledging the locale scope so the next incident-response engineer doesn't burn a shift on İ before finding this file.
• Non-Unicode text-transform values not covered. text-transform: full-width (Latin A → U+FF21 A) and text-transform: full-size-kana (small kana → full-size) both create glyphs at paint time that .toUpperCase()/.toLowerCase() won't emit. Rare in HeyGen compositions but common in Japanese web design; if the streamed-preview publisher is ever pointed at a Japanese-authored template, the same split-brain returns for those two transforms. Fine to defer with a follow-up ticket; not a merge-block.
• Version stamp is diagnostic-only, no consumer. grep -rn hyperframes-font-compiler-version across the repo finds only the writer (packages/cli/src/fontLocalize.ts:7, its unit test, and the CLI wrapper) — no compiler-side reader that would refuse or warn on drift between the stamp value and the compiler running at render time. That's consistent with Miguel's PR-body phrase "production traceability" (not enforcement), but it means the stamp is a post-hoc forensic marker, not a guardrail. If the intent was to catch localizer/compiler skew at render, a reader is still owed. If diagnostic-only is the whole plan, please say so in the JSDoc on stampFontCompilerVersion so a future maintainer doesn't assume enforcement.
• VERSION stamps the CLI package version, not the producer version. packages/cli/src/fontLocalizeCli.ts:4 imports VERSION from ./version.js, which is defined from __CLI_VERSION__ at bundle time (packages/cli/src/version.ts:2, packages/cli/tsup.config.ts:82 reading packages/cli/package.json = 0.8.14). The "font compiler" the stamp names is actually injectDeterministicFontFaces in the producer package. In workspace lockstep this is fine, but a release that bumps packages/cli without bumping packages/producer would stamp a misleading version. Consider stamping producer's package version (or both, cli@X · producer@Y) if the stamp ever gains a reader.
• Case closure is unconditional and blanket across every caller of injectDeterministicFontFaces. No flag; the change ships to studioServer.ts:438 (studio local preview transformer, called on every preview transform), bundleWithLocalizedFonts (audit/snapshot), the distributed render path, and the new localizer. Blast radius: every composition now sends a larger text= param. Above the 1,700-byte budget it falls back to no-text= (full-font download) — safe visually, but a byte-size regression on compositions that previously fit under the budget with mixed-case content and would now overflow. packages/producer/src/services/deterministicFonts-textSubset.test.ts:48-67 proves the fallback branch is wired, but no test measures subset byte count on a realistic composition to bound the size regression. Preview-regression CI shard passed, so no observable perceptual break — flagging for awareness, not merge-block.
• Fail-closed localizer trips on any unresolved family in a font-family cascade, not just the primary. extractRequestedFontFamilies (deterministicFonts.ts:452-467) iterates every family in every font-family declaration, filters only GENERIC_FAMILIES, and hands the full set to the fetcher. In fail-closed mode (deterministicFonts.ts:1278-1285), any unresolved family throws FontFetchError — so a composition with font-family: Inter, "Author Custom Fallback", sans-serif where "Author Custom Fallback" isn't in FONT_ALIAS_MAP will exit the localizer with code 1 even though the primary Inter subset resolved fine. The PR body confirms the downstream integration lands after release, so today's blast radius is zero, but the downstream owner needs to know: if any live composition names an unregistered secondary family, its preview publish will start failing the moment they flip the pin. Worth pre-flagging in that follow-up PR's description; not a block here.
Nits
• packages/cli/src/fontLocalize.ts:10-15 — stampFontCompilerVersion has three insertion strategies (before </head>, after doctype, at document start) but only the </head> branch has a unit test (fontLocalize.test.ts:74-84). Bun/vitest fixtures for the other two would take three lines each and pin the fall-through order.
• packages/producer/src/services/deterministicFonts.ts:1204 — the closure iterates [...Array.from(html), ...Array.from(decodedBodyText)] over the entire raw HTML source, which includes base64 data URIs, class names, script bodies, and attribute values. Correctness-safe (over-approximation), and the incremental byte cost is bounded because ASCII alphabet is already fully in uniqueCharacters early. Fine as-is; a one-line comment saying "yes, this deliberately runs over source text plus decoded body text, and that's why base64 doesn't cost you" would save a future reader the same re-derivation.
• deterministicFonts.ts:1209 — the ${character.toUpperCase()}${character.toLowerCase()} template concatenation followed by a for..of iteration is code-point-safe (both eszett ß→"SS" and any surrogate-pair-producing casing are handled). Consider a comment noting the two-char expansion case (Eszett) so nobody "simplifies" this to a single add later.
Questions — for Miguel
• Is the text-transform: full-width / full-size-kana scope gap intentional-defer or oversight? If defer, worth a // TODO(fonts): cover full-width/full-size-kana next to line 1209 so it's greppable.
• Is there a follow-up PR/ticket for the streamed-preview publisher pin bump you mentioned in the rollout order ("update the downstream package pin, then run this executable after streamed-preview bundling and immediately before upload")? Link would help stitch the two-PR rollout together in review history.
• Any objection to shipping a companion CLI flag (or env override) to allowSystemFontCapture / failClosedFontFetch on hyperframes-localize-fonts? Right now they're hard-coded in fontLocalizeCli.ts:24-25; a future incident that needs the localizer to fall open temporarily has no lever short of a code change.
Adversarial ledger
• Split package boundary drift. Considered whether hyperframes-localize-fonts re-exports a wrapper that narrows the input surface vs. binding directly to producer. Verified: fontLocalizeCli.ts:2 imports injectDeterministicFontFaces straight from @hyperframes/producer, and tsup.config.ts:66-72 inlines producer into the CLI bundle via noExternal. Single binding. Held negative.
• Signed URL leakage in localizer failure output. Considered whether runFontLocalize echoes source HTML or the resolver's error message (which per deterministicFonts.ts:1279-1284 embeds unresolved family names) to stderr. Verified: fontLocalize.ts:47 emits only a sanitized error class name; test fontLocalize.test.ts:44-57 explicitly asserts signed.example is absent. Held negative.
• Blanket closure over html source exploding the URL budget on real compositions. Considered whether adding case variants across every source character (including base64 data URIs, script bodies) inflates encodeURIComponent(fontText).length past 1,700 for typical compositions. Since ASCII alphabet is already saturated in uniqueCharacters after the first few characters of any real HTML, incremental cost is bounded by the non-ASCII glyph count with case pairs. The URL budget test at deterministicFonts-textSubset.test.ts:59-67 specifically pins the fallback path with 300 case-changing chars. Held negative.
• text-transform: capitalize semantics vs. the "add every uppercase variant" closure. Considered whether blanket adding uppercase-of-every-lowercase-char over-covers the capitalize case. It does — but capitalize only renders first-letter titlecase, which for Latin/BMP is .toUpperCase(), and the browser only asks for the codepoints it needs. Over-approximation is safe. Held negative.
• Windows-render CANCELLED status masking a real Windows regression. Considered whether the CANCELLED workflow run at runs/32918250043 reflects a real failure. Verified via gh api /repos/heygen-com/hyperframes/actions/runs/32918250043 — same head SHA d6de0834 as the SUCCESS run runs/32918276370, and only Detect changes even started before the concurrency-group cancel. It's a superseded duplicate trigger, not a red signal. Held negative.
Tests
• Case-closure correctness on the incident fixture: covered — deterministicFonts-textSubset.test.ts:32-45 asserts every glyph in "YOUR KIDNEY TRANSPLANT:WHAT HAPPENS NEXT" appears in the requested text= param for the Miguel fixture (font-family: "Inter", sans-serif; font-weight: 800; text-transform: uppercase).
• URL-budget fallback: covered — deterministicFonts-textSubset.test.ts:48-67 synthesizes 300 case-changing characters and asserts url.searchParams.has("text") === false.
• Fails-closed localizer: covered end-to-end at the process boundary — empty stdin → exit 2, resolver throws → exit 1 with sanitized error name (and signed-URL scrub verified), resolver returns whitespace-only → exit 1. packages/cli/src/fontLocalize.test.ts:22-72. Solid.
• Version stamp: only the </head>-present branch is tested (fontLocalize.test.ts:74-84). Missing coverage for the doctype-only and neither-tag-present branches. Low priority — the linkedom pipeline emits </head> in practice — but three lines of additional assertions would close it.
• Geometry (345→351px title width claim): deliberately not asserted at the unit level; the compiler can't measure pixel geometry without a browser. This is the right layering: Preview parity and regression shards (both green at HEAD) are the geometry proof. Not a gap.
• Locale-sensitive case coverage (Turkish İ, German ẞ): no fixture pins the current scope — worth a negative test with a // scope: JS locale-independent casing comment so the intentional gap is documented in code.
• text-transform: capitalize fixture: no dedicated test. Covered by the blanket-closure logic in practice, but a one-line fixture pinning "hello world" under capitalize → asserting H, W land in text= would document the intent.
Peer-coverage layering — Magi's summoning is Slack-side; the GitHub reviews array is empty. Nothing to layer on or avoid parallel-posting with. Miguel's PR body is the only pre-existing signal, and this review orthogonally covers: (a) the closure exhaustiveness gaps (locale, full-width), (b) the stamp-has-no-consumer observation, (c) the unconditional-blast-radius framing, (d) the fail-closed cascade-family behavior for the downstream integration. All net-new signal to whatever Magi posts on Slack.
Stamp stance — HyperFrames PR. Peer-bot summoning is not an OG stamp authorization for this org. Not recommending OG-stamp routing on this pass. If Miguel wants the stamp after his own reads, that's his call to invoke — the standing rule is Miguel authorization needed for stamp routing.
— Review by Rames D Jusso
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at d6de0834. I independently re-pinned the live head, verified the shared producer/localizer resolver contract, case-variant subset closure, fail-closed CLI behavior, and green required checks.
Locale-specific and non-case CSS transforms remain reasonable follow-ups rather than regressions in the reported contract. No merge/enqueue action taken.
— Jerrai
Addressed at
Declined a runtime fail-open CLI flag: a hidden operational bypass would violate the deterministic preview/render contract. Recovery remains an explicit package rollback or code change. Fresh verification on this head: producer unit 1,913/1,913, CLI 2,852 passed / 3 skipped, producer and CLI typechecks, full format/lint, canonical monorepo build, packed-manifest consumer validation, and a built-binary localization smoke with both |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 refresh at 7c40efbc. Verified each R1 concern inline against contents?ref=7c40efbc62bd7bd70bcfdb80cd55aef4bafb39d6 (no re-spawn):
R1 concerns → verified closed
• Locale-independent scope narrowed at source. deterministicFonts.ts:1211-1213 — new inline comment states verbatim: "This closes locale-independent Unicode casing, including multi-code-point expansions such as ß -> SS. Locale/context transforms (for example Turkish İ) and CSS full-width/full-size-kana need a transform-aware follow-up rather than pretending this code-point closure is exhaustive." This is exactly what I asked for — the scope note is now IN the source, not just PR body. ✅
• full-width / full-size-kana scope. Filed as #3496 with browser fixtures + lang context coverage + payload-size benchmark. Cross-referenced in the source comment. ✅
• Version stamp is diagnostic-only. stampFontVersions JSDoc at fontLocalize.ts:19-22 states verbatim: "These stamps are traceability metadata, not an enforcement mechanism." Downstream enforcement is explicit ownership of the pinned publisher. Miguel's rationale in the review-address comment tightens this to be intentional. ✅
• Producer vs CLI version stamp asymmetry. Now stamps BOTH: hyperframes-font-compiler-version from PRODUCER_VERSION AND hyperframes-font-localizer-version from VERSION. version.ts reads __CLI_VERSION__ and __PRODUCER_VERSION__ from tsup define block (tsup.config.ts — __PRODUCER_VERSION__: JSON.stringify(producerPkg.version)). Two independent versions at bundle time; a non-lockstep bump now surfaces cleanly on inspection. ✅
• Unconditional case-closure blast-radius. Incident-fixture subset-size ceiling + capitalize contract test added in deterministicFonts-textSubset.test.ts (+60/-18). 1,700-byte fallback intact. ✅
• Fail-closed cascade-family behavior. Pinned by "fails closed when a secondary family in the authored cascade is unresolved" at deterministicFonts-failClosed.test.ts — the test explicitly asserts the intentional-behavior contract (familyName contains "Author Custom Fallback" when Inter succeeds but the secondary 400s). Miguel notes loosening this would reintroduce glyph-specific host fallback when the primary lacks a glyph. Documented contract, not a bug. ✅
R1 nits → verified closed
• Doctype-only + no-head/no-doctype stamp branches — Miguel confirms added; source at fontLocalize.ts:24-29 handles all three branches (</head> present, doctype-only, neither). ✅
• Source-text over-approximation comment — present at deterministicFonts.ts:1204-1205. ✅
• Multi-code-point case expansion comment — present at deterministicFonts.ts:1211-1213 (calls out ß -> SS explicitly). ✅
R1 questions → answered
• full-width / full-size-kana — deferred to #3496 (verified, issue open, scope covers Turkish/Azeri + German + full-width + full-size-kana + browser fixtures + payload benchmark).
• Downstream publisher pin — enforcement contract ownership, not this PR's scope. Ack.
• Fail-open CLI flag — Miguel declined with reasoned rationale: "hidden operational bypass would violate the deterministic preview/render contract. Recovery remains an explicit package rollback or code change." Fair — a fail-open lever undermines the whole determinism guarantee this PR restores. Ack.
New considerations at HEAD
• safeVersion regex /[^A-Za-z0-9.+-]/g — permits ., +, -, so semver + prerelease + build metadata pass through cleanly. Empty-after-strip falls back to "unknown". Correct.
• __PRODUCER_VERSION__ is undefined in a raw ts-node / bun test path (no tsup define), so PRODUCER_VERSION falls back to "0.0.0-dev" — matches the CLI version fallback semantics. Consistent.
CI note (non-blocking)
Three rows on the check-list show fail with 0 duration and unresolved matrix expressions (Producer: ${{ matrix.lane }} tests, Test, and a bare regression-shards). These look like matrix-placeholder rows from an unrelated workflow config rather than actual failures at this HEAD — the individually-named regression-shards (shard-1..9, ...) rows are all pending, and the required checks (Producer unit tests, Build, Typecheck, Preview parity, CLI smoke, Windows Render, etc.) are still running fresh at 7c40efbc. Magi noted fresh hosted CI has no failures; treating those three rows as workflow-config artifacts unless the fresh run settles into them.
Peer-coverage layering
Only R1 review on this PR is my own; no parallel-post concern. Layered on Miguel's review-address comment #issuecomment-5419983352 at 03:03:07Z.
Stamp stance
Miguel has authorized merge after fresh gates. Magi will merge + release. HF PR + human authorization present ✓ — but Magi is the merger of record here, and this isn't a bot-summoned OG-stamp scenario. Not routing OG on this pass. If Miguel wants an explicit OG stamp before Magi lands the release, ping and I'll route.
R1 review at d6de0834 = #3492 (review). R2 verdict: ship it.
— Review by Rames D Jusso
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-approved at 7c40efbc. I re-pinned the live head and independently verified the post-approval delta: separate producer/localizer provenance, diagnostic-only stamp semantics, scoped transform limits, and the added insertion/capitalize/cascade regressions are sound. Current CI is green or in progress, with no failing exact-head signal.
No merge/enqueue action taken.
— Jerrai
What
Studio previews can now be compiled with the same deterministic font resolver as final HyperFrames renders. Locale-independent CSS case variants are included in Google Fonts
text=subsets, while broader locale/context and non-case transforms are explicitly tracked in #3496.This PR adds a pipe-safe
hyperframes-localize-fontsexecutable so downstream preview publishers can reuse the exact renderer font pipeline. The downstream integration will land after this version is released.Why
A production report exposed two connected failures. Authored HTML requested Inter 800 with
text-transform: uppercasebut carried no font resource, so a streamed preview used the browser's system sans. Final compilation resolved Inter 800, but its subset was derived from mixed-case source; some uppercase glyphs existed only after CSS transformed the text and fell back glyph-by-glyph in the rendered output.Fixing only the subset would remove the mixed glyphs but leave preview/render parity broken. Fixing only this title, this weight, or these three letters would hide the architectural issue.
How
injectDeterministicFontFacesas the single resolver. The new executable is only a stdin/stdout adapter and owns no alias, weight, fetch, or fallback policy.uppercasecontract and the same code-point closure used bylowercase/capitalize; locale/context-sensitive casing andfull-width/full-size-kanaremain scoped in fonts: make Google Fonts subsetting CSS text-transform aware #3496 rather than being over-claimed here.Rollout order: land/release this PR, update the downstream package pin, then run this executable after streamed-preview bundling and immediately before upload. Until that integration lands, final-render subset correctness is fixed but external streamed previews remain host-font-dependent.
Test plan
0, keeps stdout HTML-only, stamps the compiler version, embeds Inter 800, and includes every transformed glyph in its cmap