Skip to content

fix(text): isolate CJK fallback language and cache entries - #1907

Open
TimotheusG wants to merge 1 commit into
storytold:mainfrom
TimotheusG:fix/cjk-fallback-isolation
Open

TimotheusG wants to merge 1 commit into
storytold:mainfrom
TimotheusG:fix/cjk-fallback-isolation

Conversation

@TimotheusG

@TimotheusG TimotheusG commented Oct 19, 2026 •

Copy link
Copy Markdown

Dependencies and merge order

Independent feature; proposed landing position 6, before #1972. The two PRs overlap in fonts.rs. Their integration must retain both captured locale and database revision in the family-cache key. We will carry the saved, tested resolution into #1972 after this lands. #1890 itself merges cleanly with this PR.

Full nine-PR index, merge instructions, audited heads and validation evidence. The first five positions are independent, not a serial dependency chain. Combined CI evidence is not a claim of nine standalone full-suite passes.

Concurrent interface initialization can install the wrong CJK fallback font. One context sets the global language to Simplified Chinese, another sets Japanese, and the first context then looks up its Han probe using the second context's language. The family-order cache also omitted locale, so a lookup finishing after a cache clear could repopulate an entry with the wrong language's ordering.

On the affected Windows installation, Source Han Sans JP covers 页 but lacks 选删层设导; Microsoft YaHei covers all of them. This explains the intermittent missing-glyph failure left after #1143 (related issue #1141). The unmodified UI test binary reproduced it on stress run 23. This is evidence of concurrent initialization failure, not a claim that every normal single-window launch fails.

Pass the interface language explicitly into fallback lookup, capture one language policy for the whole lookup, and include that policy in the family-order cache key. Use four finite policy values (Japanese, Korean, Simplified, Traditional) instead of arbitrary locale-string keys. Preserve the existing process-wide request for composition text. Update the comments that incorrectly assumed Japanese faces could not cover 页.

Validation

  • A synthetic-font regression test forces the original global-language interleaving, verifies cache reuse across all four policies, and runs four concurrent language clients. It runs in an isolated child process so it neither depends on installed fonts nor changes other tests' global database.
  • Negative checks: the regression fails if explicit lookups read the global language again, and independently fails if the cache stops distinguishing languages. The restored implementation passes.
  • Full cargo xtask ci passed on Windows: 3,658 tests passed, zero failed, 32 existing ignored tests. Formatting, strict release/all-target Clippy, dependency/layering/asset audits, and WebAssembly checks for 32 crates passed. Release LTO disabled; 16 codegen units. One existing unrelated WASM dead-code warning remains.
  • 100 complete UI library test runs passed with --test-threads=64; no failures.
  • Built the headless UI snapshot example and visually inspected Japanese → Simplified Chinese → Traditional Chinese → Simplified Chinese switching in one session. Visible translated labels rendered without missing-glyph boxes. Test settings were isolated from the normal app configuration.

The full Windows CI validation combines this patch with the companion fixes in #1889, #1890, and #1891. This PR contains only the CJK change (two source files), based on main; it does not include those separate PR commits or the audio follow-up.

Summary by CodeRabbit

  • New Features
    • CJK font fallback now follows the selected interface language, with support for Japanese, Korean, Simplified Chinese, and Traditional Chinese.
    • Font lookups use a consistent language setting, so concurrent locale changes won’t mix fallback results across languages.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ee02ecaa-135c-4a7d-bd3d-30d9f00be1fb
📥 Commits

Reviewing files that changed from the base of the PR and between ef74d82 and d6afa87.

📒 Files selected for processing: 2
  • crates/text/src/fonts.rs
  • crates/ui-egui/src/theme.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

CJK fallback lookups now use a captured locale policy for family ordering and cache keys. The UI passes its interface language to system-font fallback lookups.

Changes: Locale-aware CJK font fallback

Layer / File(s) Summary
Locale-scoped fallback lookup
crates/text/src/fonts.rs
Locale tags map to bounded CJK policies. The fallback lookup carries the selected policy through family ordering and cache access. A native test checks locale-specific results, cache coexistence, and concurrent explicit-locale lookups.
Interface-language fallback integration
crates/ui-egui/src/theme.rs
System-font fallback installation passes the interface language’s CJK locale explicitly. Comments describe the Han probe and concurrent fallback case.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Theme as UI theme
  participant Fallback as fallback_for_locale
  participant Families as family_faces
  participant Cache as Family-face cache
  Theme->>Fallback: Pass character, preferred face, and interface-language locale
  Fallback->>Families: Resolve fallback with captured locale policy
  Families->>Cache: Read or write locale-keyed family faces
  Families-->>Fallback: Return selected face
  Fallback-->>Theme: Return fallback face
Loading

Suggested reviewers: echelon

Merge Risk: ⚪ Minimal · up to d6afa

The reviewed locale mapping preserves Traditional Chinese font fallback selection. No actionable merge-blocking risk remains after normal checks.

Security architecture risk: ⚪ Minimal · up to d6afa

The change isolates concurrent language choices while preserving existing font-loading permissions. No material security risk introduced or worsened by this change was found.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed authority is limited to in-process font preference and cache selection. The explicit API reaches the same fallback engine as the existing API; locale input does not supply a font path, font bytes or execution target. Existing registered font sources remain the source of bytes read and parsed.

Trust Boundaries and Controls

  • observed — Explicit locale selection removes dependence on another context's global language request during UI fallback installation. It is a rendering-policy separation, not an authorization or tenant-isolation boundary. The process-wide request remains intentionally available for composition text and ambient callers.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: isolating CJK fallback language policies and cache entries.
Docstring Coverage Passed Docstring coverage is 86.36% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.98.1)

Clippy execution timed out

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant