fix: resolve font URLs dynamically via GitHub API - #60
Conversation
Replace hardcoded jsDelivr font URLs with GitHub Contents API resolution. Fonts are defined by (owner, repo, path, glob) and the actual filename is discovered at runtime, making downloads resilient to upstream file renames and directory reorganizations. Fixes noto-color-emoji 404 (file moved and renamed), arabic/thai/ devanagari filename changes ([wdth,wght] axis added), and jsDelivr 403 errors on large font files by switching to raw.githubusercontent. Resolved URLs are cached in-memory for the process lifetime. Falls back to hardcoded raw GitHub URLs if the API is unreachable.
There was a problem hiding this comment.
Solid fix — dynamic resolution with a fallback layer is the right approach when upstream repos keep renaming files. The _GitHubFontSpec dataclass is clean and the in-memory cache avoids repeated API calls within a process. Approved.
Three notes (first one is a concrete suggestion, others are non-blocking):
-
Use
entry["download_url"]from the API response instead of constructing the URL manually. The current code builds{_RAW_GH}/{owner}/{repo}/main/{path}/{filename}— this hardcodes themainbranch. The Contents API already returns adownload_urlfield with the correct raw URL, including the right branch. Using it directly is simpler and avoids the assumption. Same applies to the fallback URL construction in_gf()if you keep that as-is. -
GitHub API rate limit: 60 req/hour unauthenticated. With 9 fonts, each cold start burns 9 calls (one per directory path). Frequent restarts, multiple workers, or dev/test loops could hit the limit. Worth considering either: (a) batching — fonts sharing the same
(owner, repo, path)could share a single API call, or (b) supporting an optionalGITHUB_TOKENenv var for authenticated requests (5000/hr). -
Fallback results get permanently cached. When the API is temporarily unreachable, the stale fallback URL is cached in
_resolved_url_cachefor the entire process lifetime. If that fallback URL is itself broken (which is the whole reason for dynamic resolution), the font stays broken until restart. Consider either not caching fallback results or using a short TTL so the API is retried on the next request.
There was a problem hiding this comment.
Good change — structured specs + runtime resolution is much more resilient than hardcoded jsDelivr URLs that break on every upstream rename.
One substantive suggestion and two minor notes:
Use entry["download_url"] from the API response instead of constructing the URL manually
The current code builds the download URL by hand:
url = f"{_RAW_GH}/{spec.owner}/{spec.repo}/main/{spec.path}/{filename}"Two issues with this:
- Hardcoded
mainbranch — if any upstream repo usesmasteror another default, the constructed URL 404s. The GitHub Contents API response already includesdownload_urlwith the correct branch. - URL encoding —
filenamecomes fromentry["name"](raw, e.g.NotoSansSC[wght].ttf) but square brackets need percent-encoding in URLs. The fallback URLs already use%5B/%5D, so the dynamic path would produce different URLs than the fallback for the same file.entry["download_url"]is already properly encoded.
Using entry["download_url"] directly is simpler, correct, and avoids both problems.
Minor (non-blocking):
- Fallback results are cached for process lifetime — if the API is transiently down, the fallback URL gets cached and sticks until restart. Not a problem in practice (the fallback URLs are reasonable), just means a restart is needed to pick up renames after a transient API failure.
_download_fontiterates all_KNOWN_FONT_KEYSresolving each via API to match a filename — on cold start with a cache miss this resolves up to 5 fonts. Fine at this scale, but a lazy reverse-map would be cleaner if the list grows.
There was a problem hiding this comment.
Approved ✅ — the move from hardcoded jsDelivr URLs to dynamic resolution is the right fix, and the glob + fallback design is solid.
Three suggestions worth considering:
1. Use entry["download_url"] from the API response
The resolved URL is currently constructed manually:
url = f"{_RAW_GH}/{spec.owner}/{spec.repo}/main/{spec.path}/{filename}"This hardcodes the main branch and requires manual URL encoding. The GitHub Contents API response already includes a download_url field with the correct raw URL, proper encoding, and correct default branch:
url = entry["download_url"]Simpler and more robust (e.g. if lxgw/LxgwWenKai ever changes its default branch).
2. Fallback caching prevents retry
_resolved_url_cache caches fallback results too. If the GitHub API is unreachable at startup (transient network blip), the fallback URL gets cached permanently — subsequent calls never retry the API. Consider either:
- Not caching fallback results (so the next call retries the API), or
- Adding a TTL / distinguishing cached-from-api vs cached-from-fallback
3. _download_font is O(n) with potential blocking
The old _KNOWN_FONTS dict gave O(1) filename→URL lookup. The new code iterates all _KNOWN_FONT_KEYS, calling _resolve_github_font_url for each to check if resolved_name == filename. On a cold cache in a request handler, this could make up to 5 synchronous HTTP calls (10s timeout each), blocking the event loop.
Two options:
- Build a reverse cache (
filename → url) after resolution, or - Resolve all known fonts eagerly at startup (they're needed anyway for install)
- Use entry["download_url"] from GitHub API instead of constructing raw URLs manually (avoids hardcoding branch name and URL encoding) - Batch API calls via _fetch_github_dir() cached per directory path, so fonts sharing the same repo directory use a single API call - Support GITHUB_TOKEN env var for authenticated API requests (5000/hr vs 60/hr unauthenticated) - Don't cache fallback results so transient API failures are retried on subsequent calls
|
Thanks for the reviews! All notes addressed in dabc2f4: Shared feedback (all 3 reviewers):
@milo-oaklight:
@clementine-oaklight:
@elena-oaklight:
|
Summary
(owner, repo, path, glob)— actual filename discovered at runtime via APIghCDN (returning 403 for large files) toraw.githubusercontent.comFixes:
googlefonts/noto-emojitogoogle/fonts, renamed toNotoColorEmoji-Regular.ttf)[wdth,wght]axis)Test plan
ruff checkandruff formatpassty checkpassesNotoColorEmoji-Regular.ttf)