Harden font loading for charts#93647
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Allow null in chart font test mock return type and remove duplicate FinancialForce API parameter mappings introduced on main. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Pre-initialize the typeface record with typedKeys/typedFromEntries so loadFn only needs the asset, and remove redundant casts at call sites. Co-authored-by: Cursor <cursoragent@cursor.com>
Use Log.hmmm (which exists on Logger) instead of Log.error in the CLI, so the LogType cast and no-op assignment in src/libs/Log.ts are unnecessary. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81bb214c61
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Extract logChartFontLoadError into shared module imported by both chartFontsCache and loadChartFontsForCli (CONSISTENCY-3) - Fix hasAnyLoadedTypeface to check only font-manager keys, not CUSTOM_EMOJI_FONT which is never registered on fontMgr (Codex P2) - Add justification comment to typedKeys eslint-disable (CONSISTENCY-5) - Remove istanbul ignore comment (not used in this project) Co-authored-by: Cursor <cursoragent@cursor.com>
|
No new product considerations - removing my assignment and unsubscribing. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Share hasAnyLoadedChartTypeface between app and CLI so emoji-only loads do not build an empty fontMgr. Do not cache all-failed font loads so later chart mounts can retry. Restore the Logger decoupling comment in Log.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
Pass getVictoryChartTreeTypeface into processVictoryChartTree so axis labels use the same partial-load fallback chain as overlay text. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Restore Log.ts from main after accidental prettier reformatting, and add eslint disables to getVictoryChartTreeTypefaceTest helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Explanation of Change
One failed chart font asset no longer rejects the entire batch. Typefaces load with per-asset
Promise.allSettled, partial results are cached, andgetChartSkiaTypefacewalks same-family → EXP_NEUE → first-available fallback chains so Victory and native charts still render text when a variant is missing. The CLI PNG renderer uses the same shared loader.Fixed Issues
$ #92610
PROPOSAL:
Tests
Offline tests
N/A — font loading is local asset decoding; no network dependency.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari