feat: support project font persistence#3362
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 generic font embedding in SVGs, replacing the previous Scratch-specific implementation. It adds a new FontFamily model, updates SpxProject to manage and export font preferences, and implements a utility to inject referenced font files as base64 data URIs into SVGs. The code reviewer provided valuable feedback to improve performance and robustness: they suggested using the asynchronous FileReader.readAsDataURL API instead of a synchronous manual chunking loop to avoid blocking the main thread when encoding large font files, and recommended filtering out empty strings when parsing font preferences and extracting used font families to handle malformed inputs gracefully.
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.
| function arrayBufferToDataUri(file: File, arrayBuffer: ArrayBuffer) { | ||
| const bytes = new Uint8Array(arrayBuffer) | ||
| let binary = '' | ||
| 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)}` | ||
| } | ||
|
|
||
| function makeFontFaceRule(family: string, dataUri: string) { | ||
| return `@font-face { font-family: ${JSON.stringify(family)}; src: url(${JSON.stringify(dataUri)}); }` | ||
| } | ||
|
|
||
| /** Applies project font preferences and embeds the referenced font files into an SVG. */ | ||
| export async function injectFontsToSvgText(svgText: string, fontPreferences: string[], fontFiles: Map<string, File>) { | ||
| const svg = parseSVGText(svgText) | ||
| const fontPreferencesRule = makeFontPreferencesRule(svg, fontPreferences) | ||
| const usedFontFamilies = new Set(getUsedFontFamilies(svg)) | ||
| if (fontPreferencesRule != null) fontPreferences.forEach((family) => usedFontFamilies.add(family)) | ||
| const rules = await Promise.all( | ||
| Array.from(usedFontFamilies).map(async (family) => { | ||
| const file = fontFiles.get(family) | ||
| if (file == null) return null | ||
| return makeFontFaceRule(family, arrayBufferToDataUri(file, await file.arrayBuffer())) | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Converting large font files (such as Noto Sans CJK SC, which can be several megabytes) to base64 synchronously in the main thread using a manual chunking loop and btoa is highly inefficient. It can block the main thread, leading to UI freezes, and causes significant memory overhead due to temporary string allocations.
Since injectFontsToSvgText is already asynchronous, we can leverage the native browser FileReader.readAsDataURL API. This offloads the base64 encoding to highly optimized browser internals, making it much faster and memory-efficient.
function fileToDataUri(file: File, arrayBuffer: ArrayBuffer): Promise<string> {
return new Promise((resolve, reject) => {
const blob = new Blob([arrayBuffer], { type: file.type || 'font/opentype' })
const reader = new FileReader()
reader.onload = () => resolve(reader.result as string)
reader.onerror = () => reject(reader.error)
reader.readAsDataURL(blob)
})
}
function makeFontFaceRule(family: string, dataUri: string) {
return `@font-face { font-family: ${JSON.stringify(family)}; src: url(${JSON.stringify(dataUri)}); }`
}
/** Applies project font preferences and embeds the referenced font files into an SVG. */
export async function injectFontsToSvgText(svgText: string, fontPreferences: string[], fontFiles: Map<string, File>) {
const svg = parseSVGText(svgText)
const fontPreferencesRule = makeFontPreferencesRule(svg, fontPreferences)
const usedFontFamilies = new Set(getUsedFontFamilies(svg))
if (fontPreferencesRule != null) fontPreferences.forEach((family) => usedFontFamilies.add(family))
const rules = await Promise.all(
Array.from(usedFontFamilies).map(async (family) => {
const file = fontFiles.get(family)
if (file == null) return null
const dataUri = await fileToDataUri(file, await file.arrayBuffer())
return makeFontFaceRule(family, dataUri)
})
)| function getUsedFontFamilies(svg: SVGSVGElement) { | ||
| const used = new Set<string>() | ||
| for (const el of [svg, ...Array.from(svg.querySelectorAll('[font-family]'))]) { | ||
| const attrValue = el.getAttribute('font-family') | ||
| if (attrValue == null) continue | ||
| attrValue | ||
| .split(',') | ||
| .map(stripQuotes) | ||
| .forEach((family) => used.add(family)) | ||
| } | ||
| return Array.from(used) | ||
| } |
There was a problem hiding this comment.
If the font-family attribute contains empty or malformed values (e.g., trailing commas or extra spaces like Arial, , sans-serif), getUsedFontFamilies will add an empty string "" to the set of used font families. Filtering out empty strings prevents unnecessary processing and potential lookup issues.
| function getUsedFontFamilies(svg: SVGSVGElement) { | |
| const used = new Set<string>() | |
| for (const el of [svg, ...Array.from(svg.querySelectorAll('[font-family]'))]) { | |
| const attrValue = el.getAttribute('font-family') | |
| if (attrValue == null) continue | |
| attrValue | |
| .split(',') | |
| .map(stripQuotes) | |
| .forEach((family) => used.add(family)) | |
| } | |
| return Array.from(used) | |
| } | |
| function getUsedFontFamilies(svg: SVGSVGElement) { | |
| const used = new Set<string>() | |
| for (const el of [svg, ...Array.from(svg.querySelectorAll('[font-family]'))]) { | |
| const attrValue = el.getAttribute('font-family') | |
| if (attrValue == null) continue | |
| attrValue | |
| .split(',') | |
| .map(stripQuotes) | |
| .filter((family) => family !== '') | |
| .forEach((family) => used.add(family)) | |
| } | |
| return Array.from(used) | |
| } |
There was a problem hiding this comment.
Handled in 1a0b50b: empty family names are now ignored before lookup.
| export function parseFontPreferences(fontPreferences: string) { | ||
| return fontPreferences.split(',').map(stripFontPreferenceQuotes) | ||
| } |
There was a problem hiding this comment.
If fontPreferences contains empty or malformed values (e.g., trailing commas or extra spaces like basic-chinese, , default), parseFontPreferences will return an array containing empty strings (e.g., ['basic-chinese', '', 'default']). This can lead to invalid CSS rules or state. Filtering out empty strings ensures robustness.
| export function parseFontPreferences(fontPreferences: string) { | |
| return fontPreferences.split(',').map(stripFontPreferenceQuotes) | |
| } | |
| export function parseFontPreferences(fontPreferences: string) { | |
| return fontPreferences | |
| .split(',') | |
| .map(stripFontPreferenceQuotes) | |
| .filter((name) => name !== '') | |
| } |
There was a problem hiding this comment.
Handled in 1a0b50b: parsed preferences now omit empty entries.
1c6d62e to
1d9307a
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR cleanly adds a project-font persistence layer (FontFamily model, project.fonts / fontPreferences, config round-trip, legacy-project migration) with good persistence-side test coverage, and refactors the Scratch-specific SVG font injection into a generic injectFontsToSvgText(svgText, fontPreferences, fontFiles).
The main concern is that the new mechanism is not actually wired into the runtime rendering path, which makes the feature inert at render time and regresses the previous Scratch-font embedding. A few smaller robustness/perf items follow. Security review found no exploitable issues (CSS font-family values are escaped via JSON.stringify, style is injected through XMLSerializer, and faces[].path is only used for in-memory map lookups, not filesystem access).
Inline comments cover the concrete diff-line findings. Additional items that don't map to a single changed line:
- maintainability — duplicated quote-stripping:
stripQuotesinspx-gui/src/utils/svg-font.ts:9andstripFontPreferenceQuotesinspx-gui/src/models/spx/project.ts:40are byte-for-byte identical. Extract a single shared helper so preference parsing and SVG rule generation can't diverge on family-name semantics. - maintainability — migration condition coupling:
spx-gui/src/models/spx/project.ts:469triggers legacy migration only whenfontPreferences == null && fonts.length === 0. Thenullcheck alone is the real signal of a pre-feature project; the extrafonts.length === 0clause is subtle and undocumented. Consider keying migration onfontPreferences == nullonly, or add a comment explaining why both are required. - stale comment (outside this diff):
spx-gui/src/utils/file.ts:125still saysuseRenderableImageUrl"applies rendering-specific handling such as Scratch font injection for SVG files." Sincescratch-svg-font.tswas removed, this should read "font injection for SVG files." - licensing hygiene: the deleted
scratch/fonts vendored their license text (OFL.txt/LICENSE.txt), but the newbasic-chinese/README.mdonly links to the source repo. If policy is to vendor the OFL text alongside binary font assets, consider adding it. - test gap:
spx-gui/src/utils/img-rendering.test.tsfully mocksinjectFontsToSvgText, so no test asserts what arguments the real render path passes — which is why the wiring gap below is invisible to CI. Once wiring is added, assert thatgetRenderableImageUrlforwards the project's preferences and font files.
| .then(async (ab) => { | ||
| const svgText = new TextDecoder().decode(ab) | ||
| const injectedSvgText = await injectScratchFontsToSvgText(svgText) | ||
| const injectedSvgText = await injectFontsToSvgText(svgText, [], new Map()) |
There was a problem hiding this comment.
Project fonts are never wired into runtime SVG rendering (likely functional gap + regression).
getRenderableImageUrl is the only caller of injectFontsToSvgText in the codebase, and it always passes an empty fontPreferences ([]) and an empty fontFiles (new Map()). As a result:
makeFontPreferencesRulereturnsnull(empty preferences), and everyfontFiles.get(family)isundefined, so no@font-facerules are produced — the function always returnsnulland the source SVG bytes are used unchanged.- The new
project.fonts/project.fontPreferencesare persisted and exported but never reach the render path, so the feature has no observable runtime effect. - This also regresses the removed
scratch-svg-font.ts, which auto-embedded vendored fonts by inspecting SVGfont-familyattributes; SVGs relying on those fonts will now fall back to system fonts.
getRenderableImageUrl has no access to the owning Project, so plumbing to pass project.fontPreferences and a name -> File map built from project.fonts appears missing. Please wire this up, or, if runtime wiring is intentionally deferred, call that out in the PR description and address the Scratch-font embedding regression.
| 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.
Synchronous main-thread base64 encoding of large fonts, with no result caching.
Despite the async wrapper, this loop plus btoa runs synchronously on the main thread. For the newly added Noto Sans CJK SC (~16MB NotoSansCJKsc-Regular.otf) this builds a ~16MB binary string and a ~21MB base64 string, blocking the UI and allocating 50MB+ transiently — once per SVG, since the encoded data URI isn't cached (the old scratch-svg-font.ts had a per-family fontDataUriCache that this rewrite dropped; File.arrayBuffer() caches only the raw buffer, not the base64).
Suggest offloading via async FileReader.readAsDataURL (or a Web Worker), and caching the encoded data URI per File/family so a shared font is encoded at most once. (Consistent with the gemini-code-assist high-priority note on this hunk.)
| } | ||
|
|
||
| export function parseFontPreferences(fontPreferences: string) { | ||
| return fontPreferences.split(',').map(stripFontPreferenceQuotes) |
There was a problem hiding this comment.
parseFontPreferences does not filter empty entries.
An empty or trailing-comma input (e.g. '' or 'basic-chinese, , default') yields entries like [''] / ['basic-chinese', '', 'default']. Those empty names get persisted and, in svg-font.ts makeFontPreferencesRule, emit invalid CSS such as svg { font-family: ""; }. Note an explicitly empty string bypasses the fontPreferences == null migration branch and lands here.
Suggest filtering: .map(stripFontPreferenceQuotes).filter((name) => name !== ''). The same empty-value handling applies to getUsedFontFamilies in spx-gui/src/utils/svg-font.ts (flagged by gemini-code-assist).
There was a problem hiding this comment.
Handled in 1a0b50b: parsed preferences now omit empty entries.
| Object.assign(files, stageFiles) | ||
| Object.assign(files, ...this.sprites.map((s) => s.export({ sounds: this.sounds }))) | ||
| Object.assign(files, ...this.sounds.map((s) => s.export())) | ||
| Object.assign(files, ...this.fonts.map((fontFamily) => fontFamily.export())) |
There was a problem hiding this comment.
The ~16MB default font is embedded into every project export/save.
export() now serializes each FontFamily's file into the project's file set, and load() auto-materializes createBasicChineseFontFamily() for legacy projects. This means the ~16MB Noto CJK binary becomes part of the hashed/uploaded payload on every save, adding significant per-save hashing, I/O, and upload cost. Consider deduplicating shared/default fonts by reference (a shared asset URL) rather than embedding the full binary per project, or confirm the storage layer already skips unchanged blobs by content hash.
There was a problem hiding this comment.
Kept as designed. Project font files must be exported with the XBP for self-contained offline/runtime portability; switching to a shared URL changes that contract. Storage-level deduplication is a separate concern.
1d9307a to
1a0b50b
Compare
|
Review-summary follow-up:
|
fe2af2d to
37557ed
Compare
37557ed to
ce1482c
Compare
ce1482c to
9da2b15
Compare
9da2b15 to
3bcc08a
Compare
1 participant
Closes #3342
Adds Builder support for SPX project fonts:
assets/fonts/<family>/and thefontPreferencesarray inassets/index.json;.xbptemplate;Editor previews intentionally do not consume project fonts or preferences in this PR. That integration remains in #3348 and will be addressed separately because large fonts currently make SVG preview rendering too slow.