[TimotheusG]

The metric-font substitution test assumes Calibri and Cambria are absent. On a Windows host with those fonts installed, the resolver correctly selects the originals and the test incorrectly expects Carlito/Caladea.

Separate resolution against a fixed face list from system-font discovery, and exercise the existing resolver logic using local synthetic fonts. The scenario covers missing originals with substitutes, missing substitutes falling back to Inter, unknown families, synthetic bold, and installed originals taking precedence without a missing-font flag. It neither changes the shared font database nor depends on installed proprietary fonts. Related: #1677.

All 62 text-crate unit tests passed. Companion fixes: Unix-only test-helper guard (#1889) and socket write cancellation (#1891).

Validation on Windows x64 (release, LTO disabled and 16 codegen units):

• cargo xtask ci passed with all three maintenance fixes applied together: 3,657 tests passed, 32 existing opt-in tests ignored; format, strict Clippy, dependency/layer/asset audits and all 32 wasm crate checks passed.
• The independent branches retain the failures addressed by their companion PRs; the full-CI result is for their combination.
• An earlier run hit the existing intermittent theme::script_font_tests::simplified_chinese_interface_draws_simplified_hanzi_after_another_language failure. The identical failure was reproduced in the pre-change UI test binary (run 23 with --test-threads=64). Five complete UI runs with this patch passed, followed by the successful full CI rerun. This separate intermittent issue remains unresolved; no test was disabled to get the passing run.

Summary by CodeRabbit

• Bug Fixes
  • Improved font matching across family, style, full-name, PostScript-name, and metric-alias options.
  • When the requested font is unavailable, fallback selection now respects available fonts and uses Inter or a default face. The requested family remains marked as missing when it is not Inter.
  • Improved handling of synthetic bold and installed-font precedence.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: cdbea34d-641f-4064-a66a-2bf8228c7c2c

📥 Commits

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

📒 Files selected for processing: 1

• crates/text/src/fonts.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

Font resolution now separates face discovery from resolution against an available face list. The resolver checks family, style, and name matches, then metric aliases and fallback faces. The metric-substitution test uses a local face list.

Changes: Font Resolution

Layer / File(s)	Summary
Face-list resolution and metric-substitution tests crates/text/src/fonts.rs	Resolution and lookup helpers now search a supplied face list. Tests cover Inter fallback, metric substitutions, missing-family status, synthetic bold, and precedence for installed original families.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: feiliuem

Merge Risk: ⚪ Minimal · up to 33b23

The font-resolution change is mergeable after normal checks; no actionable issue remains identified.

### Architecture Summary

Architecture risk: 🟠 High · up to 33b23

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in crates/text/src/fonts.rs: resolve now delegates its final lookup to resolve_available after any system scan, instead of retrying family/name resolution and then performing alias and Inter fallback inline. The new helper applies those steps against a supplied face list and marks a nonempty, non-Inter request missing.
• observed — Modified behavior in crates/text/src/fonts.rs: resolve_alias now takes a face list and searches substitutes within it. resolve_in passes the global face list to the new resolve_in_faces helper, which accepts an explicit list for family/style resolution.
• observed — Modified behavior in crates/text/src/fonts.rs: resolve_by_name now takes a face list rather than fetching all faces internally; its trimming and empty-name rejection are unchanged.
• observed — Modified behavior in crates/text/src/fonts.rs: The metric-substitute test replaces shared-database font registration and global resolution with a locally constructed face list. It checks that missing Calibri falls back to Inter without substitutes, Calibri and Cambria select Carlito and Caladea while remaining marked missing, an unknown family falls back to Inter, and installed original families take precedence and are not marked missing.

Reliability and maintainability

• inferred — Risk-relevant change factors for crates: blast_radius_2; blast_radius_3; blast_radius_5; direct_dependents_1; direct_dependents_2; direct_dependents_3

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly describes the primary goal: isolating metric-font fallback tests from host-installed fonts. It is concise, specific, and related to the production changes that enable fixed face-list…
Docstring Coverage		Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 1 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title clearly describes the primary goal: isolating metric-font fallback tests from host-installed fonts. It is concise, specific, and related to the production changes that enable fixed face-list resolution.

✨ 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.}

[TimotheusG]

@coderabbitai Please clarify the current architecture assessment for this PR: it labels architecture risk High, merge risk Minimal, and says no architecture-level concerns were identified. The explanation lists broad blast-radius/dependent-count factors for crates, but no specific architectural defect. What concrete concern supports High, or is this an impact score rather than a finding requiring a code change?

We independently compared the public resolver behavior before/after the extraction and checked its interaction with #1907. The focused alias/style/original-precedence tests pass. Expanded testing found a separate preexisting layout-cache invalidation bug on both the base and this PR; that fix is being prepared separately, not being attributed to this extraction.

The summary also retains 'Clippy execution timed out'. Please identify the executed command/tool budget or a supported way to rerun that tool with sufficient build time. A successful overall review status is not being treated as successful Clippy execution.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh api repos/storytold/effectcraft/issues/1890/comments --paginate --jq '.[] | {user: .user.login, body: .body}'
gh api repos/storytold/effectcraft/pulls/1890/reviews --paginate --jq '.[] | {user: .user.login, body: .body}'
rg -n -C3 'clippy|architecture' .github Cargo.toml README.md
fd -i 'xtask|config.toml|coderabbit'

Length of output: 14064

---

🤖 web_search executed:

CodeRabbit documentation Clippy tool timeout rerun configurable timeout architecture assessment risk blast radius dependent count

Length of output: 12092

---

🏁 Script executed:

#!/bin/bash
ast-grep outline xtask --items all --type function --match 'clippy|ci|main|run'
rg -n -C5 'clippy|timeout|all-targets|all-features' xtask .github CONTRIBUTING.md docs/testing.md
fd -H '^config\.toml$|^rust-toolchain(\.toml)?$'

Length of output: 3890

---

⚙️ CodeRabbit configuration inspected

Length of output: 98174

---

@TimotheusG, the posted report does not identify a concrete architectural defect that supports “High.” That label does not establish a code-change requirement for this PR.

Architecture assessment

I checked the published assessment through 33b2320. Its only risk factors are inferred blast-radius and dependent-count factors for the broad crates grouping. It explicitly says “No architecture-level concerns identified.”

The evidence supports reading “High” as an impact indicator, not a defect-severity finding. I cannot verify the scoring formula from the available report. Presenting that indicator as “Architecture risk” without this distinction is misleading.

“Merge risk: Minimal” is a separate assessment of the reviewed change. Neither label proves that validation tools passed. There is no supported architecture-specific blocker in the posted assessment.

Thanks for the clarification about your testing. Your reported base-versus-PR reproduction places the layout-cache invalidation bug outside this extraction. I have not independently reproduced those results.

Clippy timeout

The published warning establishes only:

• Tool: Clippy 1.98.1
• Result: execution timed out
• Review run: cdbea34d-641f-4064-a66a-2bf8228c7c2c

It does not expose the executed command, elapsed time, or timeout budget. I cannot supply those values from the available evidence.

I checked the current configuration schema. The Clippy settings expose only reviews.tools.clippy.enabled. The schema exposes no Clippy timeout override. I therefore cannot offer a supported way to increase the hosted tool’s budget through repository configuration. A review rerun does not guarantee more build time.

For independent verification, run the repository’s strict Clippy command locally or on a CI runner with sufficient build time:

cargo clippy --workspace --all-targets --release -- -D warnings

This is the command in xtask/src/main.rs, Line 412. It is not a verified reconstruction of CodeRabbit’s command. cargo xtask ci also runs it as part of the full checks.

Please retry that check on the exact PR revision and retain the command, toolchain, commit SHA, and exit result. Your reported combined-branch CI success remains evidence for that combined tree, not for each independent branch.

The hosted Clippy result remains incomplete, not passed. A successful overall review status does not change that result. To obtain the hosted command and budget, provide the run ID above to CodeRabbit support.

---

✏️ Learnings added

Learnt from: TimotheusG
URL: https://github.com/storytold/effectcraft/pull/1890

Timestamp: 2026-10-20T16:51:21.982Z
Learning: For storytold/effectcraft PR #1890, the author reports that expanded testing reproduced a separate layout-cache invalidation bug on both the base and the PR. Its fix is being prepared separately. Do not attribute that preexisting bug to the font-resolver extraction in crates/text/src/fonts.rs without evidence of a new regression.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.