Repository navigation
fix(core): the fonts gate starts loading declared faces no laid-out text uses yet - #6415
Conversation
…ext uses yet Before composition scripts run, waitForFonts forces one layout and awaits document.fonts.ready. A face that only script-built text uses has not started loading at that point, so ready does not wait for it and the script measures the fallback face. Start every face still unloaded after the forced layout, under the existing timeout; a load that rejects falls through to the same timeout warning instead of stopping the scripts. Refs heygen-com#5112 Co-Authored-By: Claude Fable 5.1 <[email protected]>
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for this, it is exactly the change we discussed on #5112, and the PR description makes the cost easy to see.
What works well:
afterFonts.ts:18-20starts only theunloadedfaces and leaves their promises to the existingready/timeout race, so the gate stays bounded and a broken face can't throw.- I ran your repro's shape in Chromium against the base and head
waitForFonts: a face only script-built text uses measured 324.4 px (fallback) at script time on the base and 465.4 px (the loaded face) with this change. A 404 face ends inerrorwithout delaying anything; a face whose server never answers ends at the 5 s timeout, as the Cost section says. - Both new tests fail when the loading loop is removed, so they guard the fix.
One thing to fix before merge:
- The
Commentscheck is red:afterFonts.tsgoes from 6.4% to 8.0% comment lines, and the ratchet only lets that share go down. Keeping the added comment lines at zero is enough, for example replacing the comment above the forced layout and the new three-line comment with a single line:// Layout starts the faces laid-out text uses; load() starts the rest so ready covers them too.
Optional:
- In
entry.afterFonts.test.ts, the timeout test could include a face whoseload()never settles (load: vi.fn(() => new Promise(() => {}))). Today, changing the loop toawait face.load()still passes all 15 tests; that face would catch it.
Happy to approve once Comments is green.
Verdict: COMMENT
Reasoning: The fix is correct and verified in Chromium, but the PR turns the Comments check red with three added comment lines; approve once that is trimmed.
…at never settles Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
Done in the second commit: the comment above the forced layout is now your one-liner and the three added lines are gone, so the file's comment share is below the base (the ratchet passes locally against the merge base). I also took the optional test: the timeout case now includes a face whose |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks, both points are done. The comments are down to one line (afterFonts.ts:13), so Comments no longer fails, and the timeout test now includes a face whose load() never settles. It also expects Silent Face in the timeout diagnostic's loadingFamilies, which pins that the gate still ends at the 5 s limit with such a face. Every check is green at this head.
The fix itself is unchanged from my last review, where I checked it in Chromium: a face used only by script-built text now measures with the loaded font.
Verdict: APPROVE
Reasoning: The fix matches what we agreed on #5112, both review points are addressed, and all checks pass at this head.
Merge 8 upstream commits (85dcc19..7d6c02f: heygen-com#6418, heygen-com#5273, heygen-com#6245, heygen-com#5593, heygen-com#5447, heygen-com#6415, heygen-com#6373, heygen-com#6375). Keep the fork's Windows-only ci.yml. For cancellableProcess.ts take upstream's heygen-com#5273, which supersedes the fork's own 744a620 port (same StringDecoder design, but the output limit counts raw bytes). Record the batch in docs/DECISIONS.md, mark heygen-com#6373/heygen-com#6375/heygen-com#6415/heygen-com#6418 as merged, and advance the commit watermark to 7d6c02f. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
What
waitForFontsinpackages/core/src/runtime/afterFonts.tsforces one layout and awaitsdocument.fonts.readybefore composition scripts run. Layout only starts loading the faces that laid-out text uses, so a face used solely by text a script builds has not started loading whenreadyresolves, and the script measures the fallback face (title centred from the wrong width, underline drawn to the wrong length).After the forced layout, the gate now calls
load()on every entry indocument.fontswhose status is stillunloaded, then keeps awaitingreadyunder the existingFONT_WAIT_TIMEOUT_MSrace. A face that never loads still ends in the existing timeout warning and theruntime_font_wait_timeoutdiagnostic. A rejectedload()(a missing font file) is swallowed and falls through to that same path, so one broken face cannot stop composition scripts from running.Cost
Faces a page declares but never uses will now load once at startup, bounded by the same timeout. Pages whose declared faces are all used by static text see no change: those faces were already loading.
Tests
packages/core/src/runtime/entry.afterFonts.test.ts, two cases added:load()is called once on it and not on an already-loaded face, and the scripts run only after that face andreadyresolve;load()rejects: the scripts still run at the timeout and the diagnostic reports the families still loading.Both fail on the unpatched gate and pass with it. The file's 13 existing cases pass unchanged. Also run: core typecheck (both configs), runtime artifact build, repo lint, runtime preview-guard lint, the runtime contract and behavior lanes.
Fixes #5112
🤖 Generated with Claude Code