refig - custom fonts support#553
Merged
softmarshmallow merged 15 commits intomainfrom Mar 1, 2026
Merged
Conversation
- Introduced a bring-your-own-font flow, allowing users to supply custom font files for designs that utilize non-default typefaces. - Updated AGENTS.md and FONTS.md to clarify the font loading process and rationale behind the current API design. - Enhanced README.md with examples for rendering with custom fonts and detailed instructions on discovering and loading font families. This update aims to improve the flexibility and usability of font handling in the renderer.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
WalkthroughAdds bring-your-own-font support: new Canvas/Renderer addFont APIs, WASM and Rust bindings, SDK/CLI wiring to accept font bytes, document font-family discovery, FIG streaming parser, tests, fixtures (Pretendard license), and documentation updates for font metadata and BYOF flows. Changes
Sequence DiagramsequenceDiagram
participant User
participant FigmaDoc as FigmaDocument
participant Renderer as FigmaRenderer
participant Canvas as Canvas API
participant FontRepo as FontRepository
User->>FigmaDoc: listFontFamilies(rootNodeId?)
FigmaDoc->>FigmaDoc: _resolve(...) -> ResolvedScene
FigmaDoc-->>User: string[] (unique font families)
User->>Renderer: new FigmaRenderer({ fonts: {Family: bytes} })
Renderer->>Renderer: ensureCanvas(...)
Renderer->>Canvas: addFont("Family", bytes)
Canvas->>FontRepo: store bytes (hash -> res://fonts/{family})
FontRepo-->>Canvas: family/typeface registered
User->>Renderer: render(nodeId)
Renderer->>Canvas: loadScene(sceneJson)
Canvas->>FontRepo: lookup family -> typeface bytes
FontRepo-->>Canvas: resolved font(s)
Canvas-->>Renderer: rendered image output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lities - Introduced a detailed file overview comment in `lib.ts` for the Figma data conversion utilities, outlining the purpose and functionality of the module. - Included a reference link to the Fig.kiwi format glossary for better understanding of the data formats involved. This update enhances documentation clarity and provides essential context for developers working with Figma data conversions.
- Reformatted the properties table in the Fig.kiwi glossary to enhance readability and maintain consistency in presentation. - Adjusted column headers and alignment for improved visual structure, ensuring better accessibility for developers referencing the schema. This update aims to improve the documentation quality and usability for users working with the Kiwi schema.
- Added functionality to find and utilize font metadata for improved font weight and italic style handling in text rendering. - Updated the documentation with a TODO section outlining incomplete mappings for rich text and line types, providing clarity on future enhancements needed for full support. This update aims to improve the fidelity of text rendering by leveraging font metadata, ensuring a more accurate representation of styles from Figma.
…raction - Introduced caching mechanisms for scene resolution based on Figma file types, improving performance during rendering. - Added a method to list unique font families used in the document, enhancing font management capabilities. - Updated the Figma renderer to utilize the new resolution logic, streamlining the scene loading process. This update aims to optimize the handling of Figma documents and improve the fidelity of font rendering.
- Updated the font registration methods across various modules to support multiple typefaces under the same family, allowing for better handling of font variations (e.g., Regular, Bold, Italic). - Improved documentation to clarify usage and expectations for font family names, ensuring consistency in text layout and rendering. This update aims to enhance the flexibility and usability of font management in the renderer.
- Enhanced the FigmaRenderer to accept multiple font files per family, allowing for better handling of font variations (e.g., Regular, Bold, Italic). - Updated the README.md to clarify the usage of the new fonts option, including examples for passing arrays of font files. - Added a test case to verify rendering with custom fonts, ensuring the new functionality works as intended. This update aims to improve the flexibility and usability of font management in the renderer.
- Enhanced the CLI to accept a `--fonts` option, allowing users to specify a directory containing custom font files (TTF/OTF) for rendering. - Updated the `resolveInput` function to handle fonts alongside images, improving the flexibility of input handling. - Added a new function to read and parse font files, grouping them by family for better management. - Updated the README.md with examples on how to use the new fonts feature in CLI commands. - Included tests to verify the correct rendering of documents with custom fonts. This update aims to improve the usability of the Figma renderer by allowing custom font integration directly from specified directories.
- Introduced the Pretendard font family, including multiple static styles (Black, Bold, ExtraBold, ExtraLight, Light, Medium, Regular, SemiBold, Thin) and a variable font file. - Added a LICENSE file for Pretendard, detailing the SIL Open Font License. - Updated README.md to reflect the inclusion of Pretendard and its variations, enhancing the font directory documentation. This update aims to expand the available font options for users, improving design flexibility in rendering.
…gration - Integrated the `kiwi_has_export_settings_trait` into multiple node types, including RectangleNode, EllipseNode, LineNode, TextNode, GroupNode, InstanceNode, VectorNode, and RegularPolygonNode. - This enhancement allows for better export configuration options for these node types, improving the overall functionality of the Figma integration. This update aims to enhance the export capabilities of the Figma renderer, providing users with more control over their design assets.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95542bb91f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * @returns Unique font family names (e.g. `["Inter", "Caveat", "Roboto"]`). | ||
| */ | ||
| listFontFamilies(rootNodeId?: string): string[] { | ||
| const resolved = this._resolve(rootNodeId); |
There was a problem hiding this comment.
listFontFamilies() claims that omitting rootNodeId covers the full document, but it delegates to _resolve(rootNodeId), and both restJsonToSceneJson and figFileToSceneJson default to the first page when rootNodeId is undefined. In multi-page files, families used only on later pages are omitted, so callers will skip loading required custom fonts and then get fallback/tofu text when rendering those pages.
Useful? React with 👍 / 👎.
Comment on lines
+129
to
+133
| const cached = this._sceneCache.get(cacheKey); | ||
| if (cached) return cached; | ||
| const resolved = this._figToScene(rootNodeId); | ||
| this._sceneCache.set(cacheKey, resolved); | ||
| return resolved; |
There was a problem hiding this comment.
This new cache stores a ResolvedScene for every distinct rootNodeId and never evicts entries. In the --export-all flow, render() is called for many different node IDs, so _sceneCache grows linearly with export count while retaining full scene JSON (and .fig image maps), which can cause high memory usage or OOM on large documents.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/grida-canvas/src/runtime/scene.rs (1)
327-332:⚠️ Potential issue | 🟡 MinorDesign inconsistency: Font storage uses family-based resource ID instead of hash-based.
When adding multiple font files for the same family,
resources.insert("res://fonts/{family}", ...)overwrites previous entries, storing only the last-added font bytes. This differs from the image handling pattern, which uses hash-based resource IDs (res://images/{hash}).The overwrite is not currently a functional issue because font resolution happens through
FontRepository, which correctly supports multiple files per family by storing each font's bytes separately inByteStore(indexed by hash) and maintaining aVec<u64>of hashes per family name. However, the inconsistency could become problematic if future code attempts to retrieve font bytes via the resource ID.Recommended fix: Either (1) remove font storage from resources entirely since
FontRepositoryis the source of truth, or (2) adopt hash-based resource IDs:res://fonts/{hash}to match the image pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 327 - 332, The add_font method currently stores font bytes under a family-based resource id using resources.insert(&rid, bytes.to_vec()) which overwrites when multiple files share a family; update add_font to either stop inserting into resources (remove the resources.insert call) so FontRepository remains sole source of truth, or change the resource id to use the computed hash (e.g., format!("res://fonts/{}", hash)) so storage mirrors image handling; ensure you keep fonts.add(hash, family) unchanged and that any consumer expecting resource ids will use the new hash-based id if you choose that option.packages/grida-canvas-io-figma/lib.ts (1)
2222-2242:⚠️ Potential issue | 🟡 MinorPreserve style when
fontMetaDatais missing.Current fallback forces regular text (
400, non-italic) when metadata is absent, even ifnc.fontName.styleindicates bold/italic. This can silently regress text appearance on metadata-light Kiwi payloads.💡 Suggested fallback-safe patch
function kiwi_text_style_trait(nc: figkiwi.NodeChange) { const characters = nc.textData?.characters ?? ""; const fontMetaData = nc.derivedTextData?.fontMetaData ?? nc.textData?.fontMetaData; const fontMeta = findFontMetaDataEntry(fontMetaData, nc.fontName); - const fontWeight = (fontMeta?.fontWeight ?? 400) as cg.NFontWeight; - const italic = fontMeta?.fontStyle === "ITALIC"; + const styleToken = nc.fontName?.style?.toLowerCase() ?? ""; + const fallbackItalic = /italic|oblique/.test(styleToken); + const fallbackWeight: cg.NFontWeight = + /black|heavy|extra\s?bold|ultra\s?bold/.test(styleToken) + ? 900 + : /bold/.test(styleToken) + ? 700 + : /semi\s?bold|demi\s?bold/.test(styleToken) + ? 600 + : /medium/.test(styleToken) + ? 500 + : /light/.test(styleToken) + ? 300 + : 400; + const fontWeight = + (fontMeta?.fontWeight as cg.NFontWeight | undefined) ?? fallbackWeight; + const italic = + fontMeta?.fontStyle != null + ? /italic|oblique/i.test(fontMeta.fontStyle) + : fallbackItalic; return { characters, fills: nc.fillPaints ? paints(nc.fillPaints) : [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/lib.ts` around lines 2222 - 2242, The current mapping uses fontMeta (from findFontMetaDataEntry) to set fontWeight and italic, which falls back to 400/non-italic when fontMetaData is missing; update the logic in the block that computes fontMeta, fontWeight and italic so that if fontMeta is undefined you derive weight and italic from nc.fontName.style (e.g., parse "Bold"/"SemiBold"/numeric weight tokens for fontWeight and "Italic"/"Oblique" for italic) before falling back to 400/non-italic; ensure references are to fontMetaData, findFontMetaDataEntry, fontMeta, fontWeight, italic and nc.fontName.style so the style is preserved for metadata-light payloads.
🧹 Nitpick comments (1)
packages/grida-canvas-sdk-render-figma/cli.ts (1)
179-186: Consider usingpush()for slightly better performance with many font variants.The array spread
[...existing, bytes]creates a new array on each addition, resulting in O(n²) allocations for n files in the same family. While negligible for typical font families (2-8 variants), apush()-based approach would be O(n).♻️ Proposed refactor using push
const existing = out[family]; if (existing === undefined) { - out[family] = bytes; + out[family] = [bytes]; } else { - out[family] = Array.isArray(existing) - ? [...existing, bytes] - : [existing, bytes]; + (out[family] as Uint8Array[]).push(bytes); }Note: This changes the return type to always be
Uint8Array[], which may require downstream adjustments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-sdk-render-figma/cli.ts` around lines 179 - 186, The current aggregation for out[family] uses array spread ([...existing, bytes]) which reallocates on each add; replace that with an in-place push: if existing is undefined set out[family] = [bytes], else if Array.isArray(existing) call existing.push(bytes), else set out[family] = [existing, bytes] then push; update any callers expecting a single Uint8Array to handle Uint8Array[] since out[family] will always be an array (refer to the variables out, family, existing, and bytes in cli.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 327-332: The add_font method currently stores font bytes under a
family-based resource id using resources.insert(&rid, bytes.to_vec()) which
overwrites when multiple files share a family; update add_font to either stop
inserting into resources (remove the resources.insert call) so FontRepository
remains sole source of truth, or change the resource id to use the computed hash
(e.g., format!("res://fonts/{}", hash)) so storage mirrors image handling;
ensure you keep fonts.add(hash, family) unchanged and that any consumer
expecting resource ids will use the new hash-based id if you choose that option.
In `@packages/grida-canvas-io-figma/lib.ts`:
- Around line 2222-2242: The current mapping uses fontMeta (from
findFontMetaDataEntry) to set fontWeight and italic, which falls back to
400/non-italic when fontMetaData is missing; update the logic in the block that
computes fontMeta, fontWeight and italic so that if fontMeta is undefined you
derive weight and italic from nc.fontName.style (e.g., parse
"Bold"/"SemiBold"/numeric weight tokens for fontWeight and "Italic"/"Oblique"
for italic) before falling back to 400/non-italic; ensure references are to
fontMetaData, findFontMetaDataEntry, fontMeta, fontWeight, italic and
nc.fontName.style so the style is preserved for metadata-light payloads.
---
Nitpick comments:
In `@packages/grida-canvas-sdk-render-figma/cli.ts`:
- Around line 179-186: The current aggregation for out[family] uses array spread
([...existing, bytes]) which reallocates on each add; replace that with an
in-place push: if existing is undefined set out[family] = [bytes], else if
Array.isArray(existing) call existing.push(bytes), else set out[family] =
[existing, bytes] then push; update any callers expecting a single Uint8Array to
handle Uint8Array[] since out[family] will always be an array (refer to the
variables out, family, existing, and bytes in cli.ts).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
fixtures/fonts/Pretendard/static/Pretendard-Black.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-Bold.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-ExtraBold.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-ExtraLight.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-Light.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-Medium.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-Regular.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-SemiBold.otfis excluded by!**/*.otffixtures/fonts/Pretendard/static/Pretendard-Thin.otfis excluded by!**/*.otffixtures/fonts/Pretendard/variable/PretendardVariable.ttfis excluded by!**/*.ttfpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
crates/grida-canvas-wasm/lib/api.d.tscrates/grida-canvas-wasm/lib/index.tscrates/grida-canvas-wasm/lib/modules/canvas.tscrates/grida-canvas-wasm/src/wasm_application.rscrates/grida-canvas/src/runtime/font_repository.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/application_emscripten.rsdocs/wg/feat-fig/glossary/fig.kiwi.mdfixtures/fonts/Pretendard/LICENSE.txtfixtures/fonts/README.mdpackages/grida-canvas-io-figma/lib.tspackages/grida-canvas-sdk-render-figma/AGENTS.mdpackages/grida-canvas-sdk-render-figma/FONTS.mdpackages/grida-canvas-sdk-render-figma/README.mdpackages/grida-canvas-sdk-render-figma/__tests__/refig.cli.test.tspackages/grida-canvas-sdk-render-figma/__tests__/refig.test.tspackages/grida-canvas-sdk-render-figma/cli.tspackages/grida-canvas-sdk-render-figma/lib.tspackages/grida-canvas-sdk-render-figma/package.json
…tion - Refactored the `readFontsFromDir` function to ensure it returns an array of `Uint8Array` for better memory management. - Introduced a bounded cache for scenes in the `FigmaDocument` class to prevent unbounded memory growth during exports. - Added methods for scene cache retrieval and setting, implementing LRU eviction when the cache limit is exceeded. - Enhanced the `listFontFamilies` method to traverse all pages when `rootNodeId` is omitted, ensuring comprehensive font family extraction. - Added tests to verify the new behavior of font family listing across multiple pages. This update aims to improve font management and optimize memory usage during Figma document processing.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/grida-canvas-sdk-render-figma/lib.ts (1)
133-140: Consider page-scoped cache keys for fig-file resolution.For
.fig, cache keys are node IDs. In export-all flows, many node IDs map to the same page-scene, so cache hit rate drops and LRU churn increases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-sdk-render-figma/lib.ts` around lines 133 - 140, The current fig-file caching uses rootNodeId as cacheKey which causes low hit rate because multiple node IDs map to the same page scene; change the key derivation when this.sourceType === "fig-file" to be page-scoped (e.g., derive a pageId or pageKey from rootNodeId or the node's parent/page identifier) before calling _sceneCacheGet and _sceneCacheSet so that _figToScene results are cached per page rather than per node; update the block handling this.sourceType to compute pageKey, use pageKey for cacheKey, and keep calling _figToScene(rootNodeId) to resolve the scene while storing/retrieving under the page-scoped key.packages/grida-canvas-sdk-render-figma/cli.ts (1)
106-107: Validate explicit--fontspath early for clearer errors.
resolveInputresolves explicit font paths without existence/type checks. When invalid, users get late low-level FS errors fromreaddirSyncinstead of a targeted CLI error.Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-sdk-render-figma/cli.ts` around lines 106 - 107, resolveInput is accepting an explicitFontsDir (the --fonts flag) without verifying the path exists or is a directory, which leads to late low-level FS errors; add an early validation: when explicitFontsDir is truthy, resolve it with path.resolve and then use fs.statSync or fs.existsSync + fs.stat to confirm the path exists and is a directory, and if not throw a clear CLI Error or exit with a descriptive message before calling resolveInput (update the code paths where fontsDir is set and any other place resolveInput is invoked with explicitFontsDir).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/grida-canvas-sdk-render-figma/cli.ts`:
- Around line 167-173: The parsed font family from Typr.parse (const [font] =
Typr.parse(bytes)) may be a non-string; update the assignment for family so you
read font.name, extract fontFamily, then guard with typeof fontFamily ===
"string" and a .trim() truthiness check before using it as a map key, otherwise
fall back to basenameNoExt; change the logic around the family variable
initialization in the try block (where font?.name and basenameNoExt are
currently used) to perform this explicit runtime type and trim validation.
In `@packages/grida-canvas-sdk-render-figma/lib.ts`:
- Around line 245-247: listFontFamilies currently calls figFileToSceneJson for
every page which triggers full image extraction; change figFileToSceneJson to
accept an option (e.g., skipImages or extractImages = true) and early-skip the
image-unzipping logic when that option is false, then update listFontFamilies to
call figFileToSceneJson(figFile, { extractImages: false }, i) (or similar) so
_collectFontFamiliesFromSceneJson still receives the scene JSON but no image
bytes are extracted; ensure the new option is respected by the image extraction
block and default behavior remains unchanged for other call sites.
---
Nitpick comments:
In `@packages/grida-canvas-sdk-render-figma/cli.ts`:
- Around line 106-107: resolveInput is accepting an explicitFontsDir (the
--fonts flag) without verifying the path exists or is a directory, which leads
to late low-level FS errors; add an early validation: when explicitFontsDir is
truthy, resolve it with path.resolve and then use fs.statSync or fs.existsSync +
fs.stat to confirm the path exists and is a directory, and if not throw a clear
CLI Error or exit with a descriptive message before calling resolveInput (update
the code paths where fontsDir is set and any other place resolveInput is invoked
with explicitFontsDir).
In `@packages/grida-canvas-sdk-render-figma/lib.ts`:
- Around line 133-140: The current fig-file caching uses rootNodeId as cacheKey
which causes low hit rate because multiple node IDs map to the same page scene;
change the key derivation when this.sourceType === "fig-file" to be page-scoped
(e.g., derive a pageId or pageKey from rootNodeId or the node's parent/page
identifier) before calling _sceneCacheGet and _sceneCacheSet so that _figToScene
results are cached per page rather than per node; update the block handling
this.sourceType to compute pageKey, use pageKey for cacheKey, and keep calling
_figToScene(rootNodeId) to resolve the scene while storing/retrieving under the
page-scoped key.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/grida-canvas-sdk-render-figma/__tests__/refig.test.tspackages/grida-canvas-sdk-render-figma/cli.tspackages/grida-canvas-sdk-render-figma/lib.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/grida-canvas-sdk-render-figma/tests/refig.test.ts
Comment on lines
+167
to
+173
| let family: string; | ||
| try { | ||
| const [font] = Typr.parse(bytes); | ||
| family = | ||
| (font?.name as { fontFamily?: string } | undefined)?.fontFamily ?? | ||
| basenameNoExt; | ||
| } catch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Typr type surface + current CLI extraction assumptions.
echo "=== Typr Type Definitions ==="
fd '_Typr.d.ts|Typr.d.ts' | head -5 | xargs -r sh -c 'for f do sed -n "1,220p" "$f"; done' _
echo ""
echo "=== readFontsFromDir + Typr.parse + fontFamily usage ==="
rg -n -C4 'readFontsFromDir|Typr.parse|fontFamily' \
packages/grida-canvas-sdk-render-figma/cli.ts || echo "File not found or pattern not matching"
echo ""
echo "=== Full context of lines 167-173 ==="
if [ -f "packages/grida-canvas-sdk-render-figma/cli.ts" ]; then
sed -n '160,180p' packages/grida-canvas-sdk-render-figma/cli.ts
fiRepository: gridaco/grida
Length of output: 9112
🏁 Script executed:
# Also check if Typr.parse actually returns name table with fontFamily
echo "=== Checking Typr module structure ==="
fd -e ts -e d.ts | xargs rg -l "Typr" | head -10
echo ""
echo "=== Checking fontFamily property access patterns ==="
rg "fontFamily" --type ts --type tsx -B3 -A3 | head -50Repository: gridaco/grida
Length of output: 582
🏁 Script executed:
# Get the full FontObject definition from _Typr.d.ts
if [ -f "packages/grida-fonts/typr/_Typr.d.ts" ]; then
wc -l packages/grida-fonts/typr/_Typr.d.ts
echo ""
echo "=== Searching for FontObject interface ==="
rg -n "interface FontObject|export.*FontObject" packages/grida-fonts/typr/_Typr.d.ts
echo ""
echo "=== Searching for name table definition ==="
rg -n "name\s*:" packages/grida-fonts/typr/_Typr.d.ts -A2 | head -40
fiRepository: gridaco/grida
Length of output: 338
🏁 Script executed:
# Also check the test file to see how fontFamily is actually used
if [ -f "packages/grida-fonts/__tests__/typr.test.ts" ]; then
echo "=== Typr test file content ==="
cat packages/grida-fonts/__tests__/typr.test.ts
fiRepository: gridaco/grida
Length of output: 11228
🏁 Script executed:
# Check the actual Typr wrapper/index to see how it's used
if [ -f "packages/grida-fonts/typr/index.ts" ]; then
echo "=== Typr index wrapper ==="
cat packages/grida-fonts/typr/index.ts
fiRepository: gridaco/grida
Length of output: 50369
Add runtime type guard for fontFamily to prevent non-string values from being used as map keys.
The parsed fontFamily from the name table can vary in malformed fonts. Even with the nullish coalescing fallback, a non-string truthy value (e.g., an object) would stringify incorrectly as a key. Add an explicit typeof check and trim validation to ensure only valid strings are used.
Proposed fix
let family: string;
try {
const [font] = Typr.parse(bytes);
- family =
- (font?.name as { fontFamily?: string } | undefined)?.fontFamily ??
- basenameNoExt;
+ const parsedFamily = (
+ font?.name as { fontFamily?: unknown } | undefined
+ )?.fontFamily;
+ family =
+ typeof parsedFamily === "string" && parsedFamily.trim()
+ ? parsedFamily
+ : basenameNoExt;
} catch {
family = basenameNoExt;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-sdk-render-figma/cli.ts` around lines 167 - 173, The
parsed font family from Typr.parse (const [font] = Typr.parse(bytes)) may be a
non-string; update the assignment for family so you read font.name, extract
fontFamily, then guard with typeof fontFamily === "string" and a .trim()
truthiness check before using it as a map key, otherwise fall back to
basenameNoExt; change the logic around the family variable initialization in the
try block (where font?.name and basenameNoExt are currently used) to perform
this explicit runtime type and trim validation.
Comment on lines
+245
to
+247
| for (let i = 0; i < sortedPages.length; i++) { | ||
| const resolved = figFileToSceneJson(figFile, undefined, i); | ||
| for (const f of this._collectFontFamiliesFromSceneJson( |
There was a problem hiding this comment.
Avoid full image extraction in the all-pages font listing path.
listFontFamilies() loops pages (Line 245-247), but each figFileToSceneJson call unzips all images (Line 776-785). For multi-page .fig files this creates avoidable CPU/memory spikes even though font enumeration does not use image bytes.
Proposed fix
-function figFileToSceneJson(
+function figFileToSceneJson(
figFile: FigFileDocument,
rootNodeId?: string,
- pageIndex?: number
+ pageIndex?: number,
+ includeImages: boolean = true
): ResolvedScene {
@@
- const imagesMap = iofigma.kiwi.extractImages(figFile.zip_files);
+ const imagesMap = includeImages
+ ? iofigma.kiwi.extractImages(figFile.zip_files)
+ : new Map<string, Uint8Array>();
const images: Record<string, Uint8Array> = {};
- imagesMap.forEach((bytes, ref) => {
- images[ref] = bytes;
- });
+ if (includeImages) {
+ imagesMap.forEach((bytes, ref) => {
+ images[ref] = bytes;
+ });
+ }
@@
- const resolveImageSrc =
- imagesMap.size > 0
+ const resolveImageSrc =
+ includeImages && imagesMap.size > 0
? (ref: string) => (imagesMap.has(ref) ? `res://images/${ref}` : null)
: undefined;- const resolved = figFileToSceneJson(figFile, undefined, i);
+ const resolved = figFileToSceneJson(figFile, undefined, i, false);Also applies to: 776-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-sdk-render-figma/lib.ts` around lines 245 - 247,
listFontFamilies currently calls figFileToSceneJson for every page which
triggers full image extraction; change figFileToSceneJson to accept an option
(e.g., skipImages or extractImages = true) and early-skip the image-unzipping
logic when that option is false, then update listFontFamilies to call
figFileToSceneJson(figFile, { extractImages: false }, i) (or similar) so
_collectFontFamiliesFromSceneJson still receives the scene JSON but no image
bytes are extracted; ensure the new option is respected by the image extraction
block and default behavior remains unchanged for other call sites.
- Introduced `readFigFileFromStream` to handle .fig (ZIP) archives larger than 2GB by streaming data instead of loading it entirely into memory. - Updated `parseFileFromStream` to extract pages from a .fig file stream, enhancing performance for large files. - Modified the CLI to utilize streaming for .fig files exceeding the 2GB threshold, improving file handling and memory management. - Enhanced the `FigmaDocument` class to accept pre-parsed FigFileDocument, allowing for better integration of streamed data. This update aims to improve the handling of large Figma files, optimizing memory usage and performance during document processing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/grida-canvas-sdk-render-figma/cli.ts (1)
171-179:⚠️ Potential issue | 🟡 MinorAdd runtime type guard for
fontFamilyto prevent non-string values from being used as map keys.The parsed
fontFamilyfrom the name table could be a non-string truthy value in malformed fonts. Even with the nullish coalescing fallback, a non-string truthy value would be used incorrectly as a map key. Add an explicittypeofcheck.Proposed fix
let family: string; try { const [font] = Typr.parse(bytes); - family = - (font?.name as { fontFamily?: string } | undefined)?.fontFamily ?? - basenameNoExt; + const parsedFamily = ( + font?.name as { fontFamily?: unknown } | undefined + )?.fontFamily; + family = + typeof parsedFamily === "string" && parsedFamily.trim() + ? parsedFamily + : basenameNoExt; } catch { family = basenameNoExt; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-sdk-render-figma/cli.ts` around lines 171 - 179, The parsed font name may yield a non-string truthy fontFamily which can be incorrectly used as a map key; update the Typr.parse handling in the block that sets family (using Typr.parse, font?.name, family and basenameNoExt) to check typeof font?.name?.fontFamily === "string" before using it, and fall back to basenameNoExt when it's absent or not a string so only string keys are produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/grida-canvas-io-figma/fig-kiwi/index.ts`:
- Around line 303-305: In the file.ondata callback (where fileChunks is pushed),
don't throw inside the callback; instead capture the error into a local variable
(e.g., streamError) and return early; after the stream completes (in the
surrounding Promise resolution path / after the read loop or in
file.onend/file.onfinish), check streamError and reject the outer Promise or
propagate the error so callers see it—update file.ondata to set streamError =
err and avoid throwing, then perform a single error check and reject/resume once
the stream is finished.
---
Duplicate comments:
In `@packages/grida-canvas-sdk-render-figma/cli.ts`:
- Around line 171-179: The parsed font name may yield a non-string truthy
fontFamily which can be incorrectly used as a map key; update the Typr.parse
handling in the block that sets family (using Typr.parse, font?.name, family and
basenameNoExt) to check typeof font?.name?.fontFamily === "string" before using
it, and fall back to basenameNoExt when it's absent or not a string so only
string keys are produced.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/grida-canvas-io-figma/fig-kiwi/index.tspackages/grida-canvas-io-figma/lib.tspackages/grida-canvas-sdk-render-figma/cli.tspackages/grida-canvas-sdk-render-figma/lib.ts
Comment on lines
+303
to
+305
| file.ondata = (err, data, final) => { | ||
| if (err) throw err; | ||
| if (data && data.length > 0) fileChunks.push(data); |
There was a problem hiding this comment.
Error thrown inside ondata callback won't propagate to the caller.
When an error occurs in the ondata callback (Line 304), throwing will not reject the outer Promise. The error will be swallowed or cause an unhandled rejection. Consider collecting errors and checking them after the stream completes.
Proposed fix
export async function readFigFileFromStream(
stream: AsyncIterable<Uint8Array>
): Promise<ParsedFigmaArchive> {
const zipFiles: { [key: string]: Uint8Array } = {};
+ let streamError: Error | null = null;
const unzip = new Unzip((file) => {
const fileChunks: Uint8Array[] = [];
file.ondata = (err, data, final) => {
- if (err) throw err;
+ if (err) {
+ streamError = err;
+ return;
+ }
if (data && data.length > 0) fileChunks.push(data);
if (final) {
const size = fileChunks.reduce((s, c) => s + c.length, 0);
const result = new Uint8Array(size);
let off = 0;
for (const c of fileChunks) {
result.set(c, off);
off += c.length;
}
zipFiles[file.name] = result;
}
};
file.start();
});Then check streamError after the loop completes:
unzip.push(new Uint8Array(0), true);
+
+ if (streamError) {
+ throw streamError;
+ }
const keys = Object.keys(zipFiles);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-figma/fig-kiwi/index.ts` around lines 303 - 305, In
the file.ondata callback (where fileChunks is pushed), don't throw inside the
callback; instead capture the error into a local variable (e.g., streamError)
and return early; after the stream completes (in the surrounding Promise
resolution path / after the read loop or in file.onend/file.onfinish), check
streamError and reject the outer Promise or propagate the error so callers see
it—update file.ondata to set streamError = err and avoid throwing, then perform
a single error check and reject/resume once the stream is finished.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Features
--fontsoptionDocumentation