feat(fonts): add dynamic font selection and font manager improvements#232
feat(fonts): add dynamic font selection and font manager improvements#232ChuckBuilds merged 10 commits intomainfrom
Conversation
- Add font-selector widget for dynamic font selection in plugin configs - Enhance /api/v3/fonts/catalog with filename, display_name, and type - Add /api/v3/fonts/preview endpoint for server-side font rendering - Add /api/v3/fonts/<family> DELETE endpoint with system font protection - Fix /api/v3/fonts/upload to actually save uploaded font files - Update font manager tab with dynamic dropdowns, server-side preview, and font deletion - Add new BDF fonts: 6x10, 6x12, 6x13, 7x13, 7x14, 8x13, 9x15, 9x18, 10x20 (with bold/oblique variants) - Add tom-thumb, helvR12, clR6x12, texgyre-27 fonts Plugin authors can use x-widget: "font-selector" in schemas to enable dynamic font selection that automatically shows all available fonts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds font assets and docs; implements backend font endpoints (upload, preview, delete) with enriched catalog metadata and cache handling; introduces a frontend Font Selector widget and integrates it into templates and the fonts management UI for dynamic catalog-driven selects and server-rendered previews. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser
participant Widget as "Font Selector\nWidget"
participant API as "API Server\n(api_v3)"
participant Store as "Font Store\n(assets/fonts)"
participant Renderer as "Image Renderer\n(preview)"
User->>Browser: open Fonts UI
Browser->>Widget: render() / fetchCatalog()
Widget->>API: GET /api/v3/fonts/catalog
API->>Store: read files/metadata
Store-->>API: catalog entries
API-->>Widget: catalog JSON
Widget-->>Browser: render options
User->>Browser: upload font
Browser->>API: POST /api/v3/fonts/upload (file)
API->>Store: write safe filename
API-->>Browser: upload response
Browser->>Widget: clear cache & fetchCatalog()
User->>Browser: request preview
Browser->>API: GET /api/v3/fonts/preview?font=...&text=...
API->>Renderer: render PNG from Store font
Renderer-->>API: PNG/base64
API-->>Browser: preview image
Browser-->>User: display preview
User->>Browser: delete font
Browser->>API: DELETE /api/v3/fonts/<family>
API->>Store: remove file (if allowed)
API-->>Browser: deletion result
Browser->>Widget: clear cache & refresh UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/blueprints/api_v3.py (1)
5496-5500:⚠️ Potential issue | 🟠 MajorAllow
.otfuploads consistently with earlier validation.
validate_file_upload()accepts.otf, but the later extension check rejects it, so valid files fail.✅ Proposed fix
- allowed_extensions = ['.ttf', '.bdf'] - file_extension = font_file.filename.lower().split('.')[-1] - if f'.{file_extension}' not in allowed_extensions: - return jsonify({'status': 'error', 'message': 'Only .ttf and .bdf files are allowed'}), 400 + allowed_extensions = ['.ttf', '.otf', '.bdf'] + file_extension = os.path.splitext(font_file.filename)[1].lower() + if file_extension not in allowed_extensions: + return jsonify({'status': 'error', 'message': 'Only .ttf, .otf, and .bdf files are allowed'}), 400
🤖 Fix all issues with AI agents
In `@assets/fonts/README.md`:
- Around line 2-46: The README.md has typos, inconsistent filenames/casing,
missing hyphens, and several fenced code blocks lacking language identifiers
(MD040); fix wording typos ("human readable" -> "human-readable", "editbable" ->
"editable", "30pixel" -> "30-pixel", "fairly straight-foward" -> "fairly
straightforward"), correct filename casing/typos (ensure "texgyre-27.bdf" and
"texgyreadventor-regular.otf" are spelled consistently), and add language tags
("bash") to the code fences around otf2bdf and apt/build command blocks; update
the example otf2bdf and compilation instruction blocks to match the suggested
edits while keeping the referenced commands (otf2bdf, texgyre-27.bdf,
texgyreadventor-regular.otf) intact.
In `@web_interface/blueprints/api_v3.py`:
- Around line 5643-5645: There are two Flask view functions both named
delete_font causing an endpoint name collision; locate the legacy stub
delete_font (the one without implementation) and remove it or rename its
function to a unique name and update any references; ensure only the intended
DELETE handler (the fully implemented delete_font for route
'/fonts/<font_family>') remains registered so Flask will no longer raise an
AssertionError on startup.
- Around line 5550-5559: The code converts the query param directly with size =
int(request.args.get('size', 12)), which raises a ValueError for non-integer
input and causes a 500; wrap the conversion in a try/except ValueError (or use a
safe integer parsing helper) when reading request.args.get('size', 12) and
return a 400 JSON error (e.g., {'status':'error','message':'Invalid font size'})
on parse failure before the existing size range check so invalid inputs produce
a 400 instead of a 500.
- Around line 5664-5688: The code currently uses the user-controlled font_family
directly to build filesystem paths (variables font_family, potential_path,
filepath), allowing path traversal; fix by validating/sanitizing font_family
before any filesystem access: reject values containing path separators or '..'
and allow only a safe whitelist of characters (e.g., alphanumeric, hyphen,
underscore) or percent-encoded names; when constructing potential_path or
filepath, resolve the resulting Path and ensure it is inside fonts_dir (compare
potential_path.resolve().startswith(fonts_dir.resolve()) or use
Path.is_relative_to) before calling exists()/is_file()/unlink(); also avoid
using the empty-extension branch that could match directories and ensure
fonts_dir exists before listing it (os.listdir).
- Around line 5561-5574: The font_filename from the /fonts/preview endpoint is
user-controlled and can include path traversal; to fix, canonicalize and
validate it before building font_path: reject if it contains path separators or
.. (e.g., ensure font_filename == Path(font_filename).name), restrict/validate
extension against allowed list ('.ttf','.otf','.bdf'), then resolve the
constructed path and verify it is a descendant of fonts_dir (compare
resolved_path.resolve().is_relative_to(fonts_dir.resolve()) or equivalent)
before opening; if any check fails, return the existing 404/error response. Use
the existing variables font_filename, fonts_dir, font_path and the route handler
for patching.
In `@web_interface/templates/v3/partials/fonts.html`:
- Around line 490-518: font names/display names from fontCatalog are being
injected into container.innerHTML and inline onclick handlers
(deleteFont('${font.name}')), which allows stored XSS; instead build the list
using DOM APIs or safely-escaped data attributes and attach click handlers via
addEventListener. Locate the fontEntries mapping and the container.innerHTML
assignment, stop concatenating unescaped strings (displayName, name, fontType),
render each entry by creating elements (e.g., div, span, button) and set text
via textContent, or set a data-font-name attribute with a safely-encoded value
and bind the delete action with document.querySelectorAll(...).forEach(btn =>
btn.addEventListener('click', () => deleteFont(btn.dataset.fontName))). Ensure
system font badge also uses textContent and never interpolates raw user strings
into HTML.
- Around line 526-529: Move the declaration const baseUrl =
window.location.origin out of loadFontData() and into the IIFE/shared scope
where fontCatalog, fontTokens, fontOverrides, and selectedFontFiles are defined
so that deleteFont(), addFontOverride(), deleteFontOverride(),
updateFontPreview(), and uploadSelectedFonts() can reference it without a
ReferenceError; then remove the duplicate const baseUrl declaration inside
loadFontData() to avoid shadowing.
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
5523-5527: Log cache invalidation failures instead of silently swallowing them.Silent failures make remote debugging harder on Raspberry Pi.
📝 Proposed logging
- except (ImportError, Exception): - pass # Cache clearing failed, but file was saved + except (ImportError, Exception) as exc: + logger.warning("[FONTS] Failed to clear fonts_catalog cache: %s", exc)- except (ImportError, Exception): - pass # Cache clearing failed, but file was deleted + except (ImportError, Exception) as exc: + logger.warning("[FONTS] Failed to clear fonts_catalog cache: %s", exc)As per coding guidelines, Use structured logging with context (e.g., “[NHL Recent]”) for logging messages.
Also applies to: 5694-5698
- Fix README.md typos and add language tags to code fences - Remove duplicate delete_font function causing Flask endpoint collision - Add safe integer parsing for size parameter in preview endpoint - Fix path traversal vulnerability in /fonts/preview endpoint - Fix path traversal vulnerability in /fonts/<family> DELETE endpoint - Fix XSS vulnerability in fonts.html by using DOM APIs instead of innerHTML - Move baseUrl to shared scope to fix ReferenceError in multiple functions Security improvements: - Validate font filenames reject path separators and '..' - Validate paths are within fonts_dir before file operations - Use textContent and data attributes instead of inline onclick handlers - Restrict file extensions to known font types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web_interface/blueprints/api_v3.py`:
- Around line 5548-5641: The handler allows unbounded user-controlled text which
can create arbitrarily large images; add fast input validation to cap rendered
size: enforce a max character count for text (e.g., MAX_TEXT_CHARS) and/or max
lines, then after measuring bbox (using temp_draw.textbbox) compute
img_width/img_height and reject if width, height, or total pixels exceed safe
limits (e.g., MAX_DIM and MAX_PIXELS) by returning
jsonify({'status':'error','message':'Requested image too large'}) with 400;
apply these checks near the top (before heavy rendering) and immediately after
bbox calculation (before allocating the final Image), referencing variables
text, size, temp_draw.textbbox, text_width/text_height, img_width/img_height,
padding and return early to avoid memory/CPU spikes on Raspberry Pi.
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
5523-5527: Add logging and narrow the exception when cache invalidation fails.Swallowing all exceptions makes remote debugging harder; log unexpected failures with context and only ignore
ImportError.As per coding guidelines, Use structured logging with context (e.g., "[NHL Recent]") for logging messages.♻️ Proposed fix
- try: - from web_interface.cache import delete_cached - delete_cached('fonts_catalog') - except (ImportError, Exception): - pass # Cache clearing failed, but file was saved + try: + from web_interface.cache import delete_cached + delete_cached('fonts_catalog') + except ImportError: + pass + except Exception as exc: + logger.warning("[Fonts] Cache clear failed after upload: %s", exc, exc_info=True)- try: - from web_interface.cache import delete_cached - delete_cached('fonts_catalog') - except (ImportError, Exception): - pass # Cache clearing failed, but file was deleted + try: + from web_interface.cache import delete_cached + delete_cached('fonts_catalog') + except ImportError: + pass + except Exception as exc: + logger.warning("[Fonts] Cache clear failed after delete: %s", exc, exc_info=True)Also applies to: 5755-5759
- Move `import re` to module level, remove inline imports - Remove duplicate font_file assignment in upload_font() - Remove redundant validation with inconsistent allowed extensions - Remove redundant PathLib import, use already-imported Path - Fix XSS vulnerabilities in fonts.html by using DOM APIs instead of innerHTML with template literals for user-controlled data Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add input validation to prevent DoS via large image generation: - MAX_TEXT_CHARS (100): Limit text input length - MAX_TEXT_LINES (3): Limit number of newlines - MAX_DIM (1024): Limit max width/height - MAX_PIXELS (500000): Limit total pixel count Validates text early before processing and checks computed dimensions after bbox calculation but before image allocation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web_interface/blueprints/api_v3.py (1)
5471-5471:⚠️ Potential issue | 🟡 MinorAdd type hints for the new font endpoints.
These new functions should declare parameter and return types for consistency and clarity. As per coding guidelines, Use type hints for function parameters and return values.
Also applies to: 5533-5535, 5661-5663
web_interface/templates/v3/partials/fonts.html (1)
482-487:⚠️ Potential issue | 🟡 MinorNewlines in
textContentare collapsed — detected fonts render on a single line.
container.textContent = lines.join('\n')relies on whitespace preservation, but the#detected-fontscontainer (line 12) has nowhite-space: pre*styling. All entries will merge into one line.Easiest fix: add the Tailwind
whitespace-pre-wrapclass to the container.Proposed fix (HTML, line 12)
- <div id="detected-fonts" class="bg-gray-800 text-gray-100 font-mono text-sm p-3 rounded h-40 overflow-y-auto"> + <div id="detected-fonts" class="bg-gray-800 text-gray-100 font-mono text-sm p-3 rounded h-40 overflow-y-auto whitespace-pre-wrap">
🤖 Fix all issues with AI agents
In `@web_interface/blueprints/api_v3.py`:
- Around line 5516-5520: The code currently swallows all exceptions when calling
delete_cached('fonts_catalog') leaving no trace of failures; change the
try/except to catch ImportError and Exception as e and log the error with
structured context (e.g., logger.exception or logger.error with exc_info=True
and a prefix like "[NHL Recent]") before continuing, so import or runtime
failures are recorded; locate the cache invalidation blocks that call
web_interface.cache.delete_cached (and the similar block later that clears
cache) and replace the bare "except (ImportError, Exception): pass" with a
logged exception that includes the module/name (delete_cached) and contextual
tag, ensuring both cache-clear locations use the same pattern.
- Around line 5389-5405: The current catalog assignment uses family_name as a
single key so later files with the same family_name overwrite earlier ones;
change the logic in the block building catalog so you either (a) group entries
by family_name (e.g., make catalog[family_name] a dict/structure containing a
list/variants array and append a variant object with keys
'filename','display_name','path','type','metadata'), or (b) use a unique key per
file (e.g., family_name + filename) to avoid collisions; update the code around
catalog[...] (the creation that currently sets catalog[family_name] = {...}) to
append or insert the new variant instead of replacing the existing entry.
- Around line 5604-5615: The BDF branch currently always falls back to
ImageFont.load_default(), producing misleading previews; instead either
implement proper BDF rendering using freetype.Face (use
face.set_pixel_sizes(size), load glyph bitmaps and composite per-glyph into an
Image via Image.frombytes/ImageDraw.bitmap when freetype import/face creation
succeeds) or explicitly fail rather than silently using
ImageFont.load_default(); replace the fallback in the block that references
font_path, freetype.Face, face.set_pixel_sizes and ImageFont.load_default() with
either a real per-character rendering path using the freetype face or
raise/return a clear unsupported-BDF error (e.g. raise NotImplementedError or
return an HTTP error) so the preview doesn’t show the wrong font.
🧹 Nitpick comments (4)
web_interface/templates/v3/partials/fonts.html (4)
221-226:escapeHtmlis defined but never called — dead code.All rendering paths now use DOM APIs (
createElement+textContent), so this helper is unused. Remove it to avoid confusion.Proposed fix
-// Helper function to escape HTML and prevent XSS -function escapeHtml(text) { - const div = document.createElement('div'); - div.textContent = String(text); - return div.innerHTML; -}
228-241: Unbounded retry loop if DOM elements never appear.If
detected-fontsoravailable-fontselements are never rendered (e.g., template mismatch), this retries every 100 ms indefinitely. Add a max-attempts guard.Proposed fix
+var _fontsInitAttempts = 0; +var _fontsMaxInitAttempts = 50; // 5 seconds max + function initializeFontsTab() { const detectedEl = document.getElementById('detected-fonts'); const availableEl = document.getElementById('available-fonts'); if (!detectedEl || !availableEl) { + if (++_fontsInitAttempts >= _fontsMaxInitAttempts) { + console.error('Fonts tab elements not found after max retries, giving up.'); + return; + } console.log('Fonts tab elements not found, retrying...', { detectedFonts: !!detectedEl, availableFonts: !!availableEl }); setTimeout(initializeFontsTab, 100); return; } + _fontsInitAttempts = 0;
562-568: Missingresponse.okcheck before parsing JSON — opaque errors on non-JSON responses.If the server returns a non-JSON body (e.g., 502 HTML error page),
response.json()throws aSyntaxErrorand the user sees a cryptic message. This same pattern also appears inaddFontOverride(line 667),deleteFontOverride(line 695), anduploadSelectedFonts(line 940).Proposed fix (deleteFont example — apply analogously elsewhere)
const response = await fetch(`${baseUrl}/api/v3/fonts/${encodeURIComponent(fontFamily)}`, { method: 'DELETE' }); + if (!response.ok) { + throw new Error(`Server error: ${response.status} ${response.statusText}`); + } + const data = await response.json();
499-514: Hardcoded system-fonts list will drift from server-side protection.The DELETE endpoint already enforces system-font protection server-side. This client-side list (used only for UI badge display) must be kept in sync manually and uses substring matching (
includes) which can produce false positives (e.g., a user font named"my10x20custom"matches"10x20").Consider having the
/api/v3/fonts/catalogresponse include anis_systemflag per font so the UI can rely on a single source of truth.
- Add structured logging for cache invalidation failures instead of silent pass (FontUpload, FontDelete, FontCatalog contexts) - Use filename as unique catalog key to prevent collisions when multiple font files share the same family_name from metadata - Return explicit error for BDF font preview instead of showing misleading preview with default font Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Frontend (fonts.html): - Remove unused escapeHtml function (dead code) - Add max-attempts guard (50 retries) to initialization loop - Add response.ok checks before JSON parsing in deleteFont, addFontOverride, deleteFontOverride, uploadSelectedFonts - Use is_system flag from API instead of hardcoded client-side list Backend (api_v3.py): - Move SYSTEM_FONTS to module-level frozenset for single source of truth - Add is_system flag to font catalog entries - Simplify delete_font system font check using frozenset lookup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@web_interface/blueprints/api_v3.py`:
- Around line 5433-5436: The except blocks around the
set_cached('fonts_catalog', ...) call bind an unused exception variable (e)
which triggers lint errors; change "except Exception as e:" to "except
Exception:" (or alternatively log the exception variable via logger.error(...,
exc_info=True) / include the exception message) so the binding is removed or
used. Update both occurrences that wrap set_cached('fonts_catalog', catalog,
ttl_seconds=300) and the similar block at the later occurrence (lines
referencing set_cached and logger.error for fonts_catalog) to use a bare "except
Exception:" or to include the exception in the logger call.
- Around line 5744-5752: The delete fallback loop uses font_family_lower but it
isn't defined; before iterating fonts_dir set font_family_lower =
font_family.lower() (or compute a lowercase target from the incoming font_family
parameter) so the case-insensitive comparison in the loop
(name_without_ext.lower() == font_family_lower) works; ensure the variable is
defined in the same scope as the loop that builds filepath and used consistently
for the case-insensitive filename matching.
In `@web_interface/templates/v3/partials/fonts.html`:
- Around line 860-861: The fetch call that reads preview data uses "response"
and immediately calls response.json() without checking response.ok; update the
preview fetch logic to mirror other handlers (e.g., the patterns used in
deleteFont/addFontOverride/uploadSelectedFonts): if (!response.ok) parse the
error body (attempt response.json(), fall back to text) and throw or return a
descriptive error so the catch block shows the server error message instead of a
generic "Error loading preview"; keep using the same "response" and "data"
variables and ensure the control flow only calls response.json() for successful
(response.ok) responses.
🧹 Nitpick comments (4)
web_interface/blueprints/api_v3.py (3)
5558-5560: Add a return type hint for the new endpoint.
This keeps the new API surface compliant with the type-hint guideline.As per coding guidelines, Use type hints for function parameters and return values.✍️ Suggested update
`@api_v3.route`('/fonts/preview', methods=['GET']) -def get_font_preview(): +def get_font_preview() -> Response:
5650-5653: Catch specific PIL errors when loading fonts.
Narrow the handler to expected exceptions (e.g., invalid font file) instead of a blanket catch.As per coding guidelines, Catch specific exceptions, not bare `except:` statements.🛠️ Suggested update
- try: - font = ImageFont.truetype(str(font_path), size) - except Exception: - font = ImageFont.load_default() + try: + font = ImageFont.truetype(str(font_path), size) + except (OSError, ValueError): + font = ImageFont.load_default()
5700-5702: Add type hints for the delete endpoint signature.
Keeps the new endpoint aligned with typing guidelines.As per coding guidelines, Use type hints for function parameters and return values.✍️ Suggested update
`@api_v3.route`('/fonts/<font_family>', methods=['DELETE']) -def delete_font(font_family): +def delete_font(font_family: str) -> Response:web_interface/templates/v3/partials/fonts.html (1)
786-788: Minor inconsistency:innerHTMLwith static icon markup.The rest of the file carefully uses DOM APIs to avoid XSS, but this line uses
innerHTMLfor the trash icon. It's safe here since the string is a static literal, but switching to DOM APIs would keep the approach consistent.Proposed fix
const deleteBtn = document.createElement('button'); deleteBtn.className = 'btn bg-red-600 hover:bg-red-700 text-white px-3 py-1 text-sm'; - deleteBtn.innerHTML = '<i class="fas fa-trash mr-1"></i>Remove'; + const icon = document.createElement('i'); + icon.className = 'fas fa-trash mr-1'; + deleteBtn.appendChild(icon); + deleteBtn.appendChild(document.createTextNode('Remove'));
- Add .otf to accepted file extensions (HTML accept attribute, JS filter) - Update validation regex to allow hyphens (matching backend) - Preserve hyphens in auto-generated font family names - Update UI text to reflect all supported formats Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused exception binding in set_cached except block - Define font_family_lower before case-insensitive fallback loop - Add response.ok check to font preview fetch (consistent with other handlers) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add return type hints to get_font_preview and delete_font endpoints - Catch specific PIL exceptions (IOError/OSError) when loading fonts - Replace innerHTML with DOM APIs for trash icon (consistency) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Plugin authors can use x-widget: "font-selector" in schemas to enable dynamic font selection that automatically shows all available fonts.
Summary by CodeRabbit
New Features
Documentation
Note
Medium Risk
Adds new font upload/preview/delete endpoints and changes font catalog responses, which can affect file handling and API compatibility if clients rely on prior behavior.
Overview
Adds end-to-end dynamic font support: a new
font-selectorconfig widget for plugins and significant font manager UI updates to browse the server font catalog, preview fonts via a server-rendered image endpoint, and delete fonts.Extends the fonts API surface by enhancing
GET /api/v3/fonts/catalogmetadata (e.g., filename/display name/type), addingGET /api/v3/fonts/preview, addingDELETE /api/v3/fonts/<family>with protection for system fonts, and fixingPOST /api/v3/fonts/uploadto persist uploaded files. Also adds a large set of new bundled bitmap/BDF fonts and related documentation/credits.Written by Cursor Bugbot for commit 230aa1f. This will update automatically on new commits. Configure here.