Repository navigation
fix(producer): local renders keep the system font the preview shows - #6592
miguel-heygen wants to merge 1 commit into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 1 nit · e300c3c2
- blocker: the required
regressioncheck is red, and it's this change. Shard 9 failsgsap-redraw-start-and-overlap(PSNR 24.3, 100/100 frames) andparallel-capture-regression(PSNR 28.6, 100/100 frames). Both putsystem-uiorsans-seriffirst, and their goldens were rendered in-process in Docker with Inter promoted. The other 8 shards were cancelled by fail-fast, so more goldens may fail (e.g.css-spinner-render-compatandaudio-mux-parityalso start withsystem-ui).regressionis green on main. ↓ inline - nit: the PR body says the loudness test's "result on the base without this change is below", but nothing follows it.
Scope
Read all 5 files, plus normalizeSystemFontPrimaryFamilies/GENERIC_FAMILIES in core and every allowSystemFontCapture caller (only distributed/plan.ts and fontLocalizeCli.ts pass false). The lint rule and the compile gate look correct. CI and Windows were still running when I posted. No prior reviews.
— Genesis
| const fontHtml = injectTextRenderingRule( | ||
| coalesceHeadStylesAndBodyScripts(promoteCssImportsToLinkTags(sanitizedHtml)), | ||
| ); | ||
| const hostFontsMissing = options.allowSystemFontCapture === false; |
There was a problem hiding this comment.
blocker (scope): only distributed/plan.ts and font localize pass allowSystemFontCapture: false, so every in-process render stops promoting Inter, including render --docker and the producer /render server. Docker pins its own font set, so --docker output for a system-first stack moves from Inter to the image's fallback (DejaVu/Noto/Liberation), and so do the Docker-rendered regression goldens. If that's intended, regenerate the affected goldens in this PR and say in the body that --docker output changes. The lint message ("in Inter on cloud renders") then also needs to cover Docker, which matches neither the preview nor the cloud. Otherwise, keep the promotion when the render runs in the container.
|
Agreed on the blocker. The goldens were rendered with Inter promoted, and 25 of the 85 fixtures start a font stack with a system or generic family ( Same root, wider: The loudness note gets the base result once that run finishes. |
Edit accuracy: accurate 271 (base branch 271), smooth 129 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
What changes
A local render now draws a system font stack (
system-ui,-apple-system,sans-seriffirst) in the same font the preview andsnapshotshow. Only cloud renders, which have no system fonts, still put Inter first. A newcheckwarning,system_font_inter_in_cloud, says so plainly, so the author knows the cloud render will differ.Why
compileForRenderput Inter ahead of every system-primary stack on every render. That rewrite was added so distributed plans pass system-font validation, but local renders capture host fonts and the preview never got the rewrite. The same text was a different width in the preview and in the MP4.Measured on a Linux box,
1,234,567.89set insystem-ui, -apple-system, BlinkMacSystemFont, sans-serifat 160 px:text-rendering: geometricPrecisionis not part of the gap: the preview bundler injects the same rule, so player and render already agree on it.How
htmlCompiler.ts: the Inter promotion runs only whenallowSystemFontCapture === false(distributed plans and font localize set it).lint/rules/fonts.ts:system_font_inter_in_cloud(warning) when afont-familyin a style block starts with a system or generic family. CSS-wide keywords (inheritand so on) are not flagged. No bundled template or registry example starts a stack with a system font, so no existing project changes verdict.docs/reference/lint-rules.mdx.Tests
htmlCompiler.test.ts: a local compile keepssystem-ui, -apple-system, sans-seriffirst (fails on main). The two existing promotion tests now compile withallowSystemFontCapture: falseand still get Inter.fonts.test.ts: the warning fires for a system stack, with its line, and stays quiet for a named primary and forinherit.inheritturns the lint tests red.htmlCompiler,distributed/plan,render/planValidationand the deterministic-font suites green apart from one loudness test noted below.checkprints the new warning on that project.distributed/plan.test.ts > stitched mp4 plans > keeps assembled loudness within 0.1 LUFSfails in this run; it measures audio and does not touch fonts. Its result on the base without this change is below.Next step (decided, not yet pushed)
The regression goldens and
render --dockerwere rendered with Inter promoted, so this head changes 25 of the 85 regression fixtures. Decision: only a plain local render on the author's own machine uses host fonts. Distributed plans,render --dockerand the regression harness all count as having no host fonts and keep Inter, so the goldens stay unchanged and--dockerstays equal to the cloud render. The plan is one decision,rendersWithAuthorFonts(options, env): false forallowSystemFontCapture: falseorCONTAINER=true, which both Docker images set. Tests: a container compile keeps Inter, and both Dockerfiles must keepENV CONTAINER=true. The lint message will then name cloud and--dockerrenders.