feat: render project fonts in editor#3348
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for project-specific font preferences and custom font families, specifically adding basic Chinese font support and integrating it with SVG rendering. Feedback on the changes focuses on performance optimizations and code simplification: it is recommended to avoid redundant SVG render cache invalidation by checking for actual changes in font preferences, cache the network fetch promise for the basic Chinese font to prevent repeated network requests, simplify the font initialization logic in the project creation modal, and refactor manual DOM traversal in SVG font injection using the closest() API.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
临时使用 NotoSansCJKsc-Regular.otf 占位，PR 合入前换成 spx 当前使用的中文字体
There was a problem hiding this comment.
临时使用 NotoSans-Medium.ttf 占位，PR 合入前换成 spx 当前使用的 Latin 字体
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: support project font collection
Solid, well-tested feature. Fonts are carried through file serialization (so they survive duplication/remix), the SVG-injection escaping via JSON.stringify is safe against CSS/markup breakout, the WeakMap-backed derived-SVG cache with catch-driven eviction is clean, and arrayBufferToDataUri chunks base64 to avoid stack overflow. No security issues found.
A few concrete points below (inline). Summary of the non-line-specific ones:
Performance — same font re-encoded per SVG. svg-font.ts:arrayBufferToDataUri base64-encodes the whole font file on every injectFontsToSvgText call, and SvgFontContext.cache is keyed on the SVG File, not the font. So for N SVGs sharing one font family, the font is base64-encoded N times (and each derived blob carries its own ~1.33× copy of the font). With the multi-MB NotoSansCJKsc-Regular.otf, an editor rendering many text-bearing sprites/backdrops does O(N × fontSize) redundant CPU, and a single fontPreferences change discards the cache and re-encodes everything. Consider memoizing the data-URI (or the whole @font-face rule string) per font File in a module-level WeakMap<File, string> so it survives context rebuilds. See inline note at svg-font.ts:30.
default family is render-only and never persisted (asymmetry worth a comment). ProjectEditor.vue injects createDefaultFontFile() under defaultFontFamilyName into the render-time map, and the constructor defaults fontPreferences = ['default'], but 'default' is never added to project.fonts and so is not in the exported assets/fonts collection. This looks intentional (the runtime supplies default), but fontPreferences referencing a family absent from the exported collection is non-obvious — a one-line comment near ProjectEditor.vue or in font.ts would prevent a future reader from reading it as a missing-export bug.
Placeholders before merge. The two unresolved threads still apply: NotoSansCJKsc-Regular.otf and NotoSans-Medium.ttf are placeholders to swap for spx's actual fonts. Note default/README.md labels its font "a temporary Latin placeholder" but basic-chinese/README.md has no such note — if that OTF is likewise temporary, add the same wording for parity.
| } | ||
|
|
||
| export function parseFontPreferences(fontPreferences: string) { | ||
| return fontPreferences.split(',').map(stripFontPreferenceQuotes) |
There was a problem hiding this comment.
parseFontPreferences doesn't drop empty entries. An empty or misconfigured VITE_DEFAULT_FONT_PREFERENCES, or a trailing/double comma in a project's stored fontPreferences, yields entries like ['default', ''] (or ['']). These empties get stored, re-serialized on export (round-trips as "default, "), and flow into svg-font.ts producing an invalid CSS token font-family: default, ""; plus usedFontFamilies.add(""). Suggest .map(stripFontPreferenceQuotes).filter((f) => f !== '').
| return trimmed.slice(1, -1) | ||
| } | ||
| return trimmed | ||
| } |
There was a problem hiding this comment.
stripQuotes here is byte-for-byte identical to stripFontPreferenceQuotes in project.ts, and both parse a comma-separated font-family list. Worth extracting one shared helper — otherwise the two can drift (e.g. one later handling escaped quotes/whitespace differently) and silently break round-tripping between what's embedded in the SVG and what's stored in fontPreferences.
| attrValue | ||
| .split(',') | ||
| .map(stripQuotes) | ||
| .forEach((family) => used.add(family)) |
There was a problem hiding this comment.
getUsedFontFamilies doesn't skip empty/whitespace values, so font-family="" (or " , ") adds "" to the used set. It's harmless today only because fontFiles.get("") is null and filtered later, but it's fragile — add .filter((family) => family !== '') after .map(stripQuotes) to make the intent explicit.
| for (let i = 0; i < bytes.byteLength; i += 8192) { | ||
| binary += String.fromCharCode(...bytes.subarray(i, i + 8192)) | ||
| } | ||
| return `data:${file.type || 'font/opentype'};base64,${btoa(binary)}` |
There was a problem hiding this comment.
This base64-encodes the entire font on every call, and the derived-SVG cache in img-rendering.ts is keyed on the SVG file, not the font — so the same font is re-encoded once per SVG that uses it, and each cached blob holds its own ~1.33× copy. For the multi-MB CJK OTF across many text-bearing SVGs this is significant CPU + memory. Consider memoizing the data-URI (or the @font-face rule) per font File in a module-level WeakMap<File, string> so it's reused across SVGs and survives SvgFontContext rebuilds.
| const fonts = await FontFamily.loadAll(files) | ||
| // Projects created before project-font support have no font config, but may rely on Builder's | ||
| // previous Chinese SVG fallback. Materialize the equivalent project font on their next save. | ||
| if (fontPreferences == null && fonts.length === 0) { |
There was a problem hiding this comment.
Minor: the migration guard couples fontPreferences == null with fonts.length === 0. A legacy project with fontPreferences == null but a non-empty assets/fonts dir would fall into the else branch and get ['default'], loading those font families into this.fonts without any preference referencing them. Unlikely in practice, but a one-line comment on why fonts.length === 0 is part of the guard would help.
677be1f to
c1730e5
Compare
nighca
changed the title
dee72f7 to
061134a
Compare
1 participant
Depends on #3362.
Summary
fontPreferencesarray to the editor SVG rendering context;Status
This PR remains draft. Large fonts currently make SVG previews slow because they are embedded as data URIs; the editor rendering strategy needs further optimization before this layer is ready to merge.