fix(core): gate first layout on the resolved font stack, not authored names - #562
fix(core): gate first layout on the resolved font stack, not authored names#562jan-kubica wants to merge 1 commit into
Conversation
… names Root cause of the measure/paint divergence found by the parity oracle: a repaired Arabic line measured 194px and painted 259px, a third of the line. `resolveFontFamily` expands an authored family into a CSS stack that appends folio's bundled substitutes and a script fallback. Every stack ends with the bundled Arabic face, so an authored "Arial" run paints its Arabic in that face: Arial -> Arial, Arimo, Helvetica, "Noto Sans Arabic", sans-serif Calibri -> Calibri, Carlito, Arial, Helvetica, "Noto Sans Arabic", sans-serif The readiness gate collected only the names the document wrote, so it released the first layout before those appended faces had loaded. The measurer then measured a pre-load fallback while the painter later drew the real face. The gate now expands each family through the resolver and waits for every concrete face in the stack, generics excluded. That also retires OFFICE_FONT_FAMILY_MAP, which was a second copy of the resolver's mapping and free to drift from it.
📝 WalkthroughWalkthroughFont readiness now expands authored font families through ChangesFont readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/controller/fontReadiness.ts`:
- Around line 275-285: Update resolvedStackFamilies to handle commas inside
quoted font-family names instead of splitting cssFallback at every comma; use a
CSS-aware parser that preserves entries such as "Font, Name" as one family, or
explicitly reject comma-containing names consistently with quoteFontName. Ensure
concrete fallback counting remains correct.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24f222b4-37d8-4911-9252-ddb6617d69f4
📒 Files selected for processing (3)
.changeset/font-readiness-resolved-stack.mdpackages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint, typecheck, test, build
- GitHub Check: DOCX kernel (Rust and WebAssembly)
- GitHub Check: Packaged-consumer build (tarballs)
- GitHub Check: Interaction e2e (playground)
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat legal data, personal data, and repository secrets as sensitive; keep generated repository artifacts limited to public engineering context.
Preserve security, auditability, least privilege, data minimization, workspace isolation, ownership boundaries, audit trails, encryption-aware handling, and explicit access checks.
Use Conventional Commits prefixes such as feat:, chore:, fix:, and docs:.
Rebase feature branches onto main to maintain linear history.
Enable git rerere and rerere.autoupdate for repeated conflict resolution.
Use vertical slices over horizontal layers; new capabilities should land in independent end-to-end slices and avoid unrelated existing code.
Never delete or regenerate bun.lock for package version bumps; run the workspace-version checker with --write, then bun install --frozen-lockfile.
Do not assume English language or typography conventions; highlight competing date, quotation, citation, and legal-terminology standards when relevant.
Do not publish private user, customer, infrastructure, incident, pricing, roadmap, competitive, identity, or security-architecture context in repository artifacts.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Prefer explicit TypeScript designs that make invalid states unrepresentable, including branded types, discriminated unions, exhaustive checks, and invariant/property tests for systemic defects.
Avoid boolean fields for extensible states; use named discriminators or domain types such as unions or enums.
Fail fast: validate at boundaries and return or throw early; minimize brace nesting with inverted conditions and early returns.
Use named constants instead of string literals for domain values.
Do not assign directly to document.cookie.
Avoid spread in loop accumulators; use .push().
Do not use enums; use as-const objects or union types.
Model mutually exclusive states as discriminated unions with a stable discriminator; avoid boolean flag sets with optional payloads.
Construct discriminated-union transitions by explicitly listing target-branch fields; read unions with switch and a never exhaustiveness check.
Avoid unnecessary as casts; narrow with type guards, in checks, or records, and document unavoidable casts with a SAFETY comment.
Trace type mismatches to their source rather than casting at the consumer.
Do not annotate or provide explicit type arguments when the compiler can infer them; let inference flow and narrow at boundaries.
Validate large-union object literals with as const satisfies T rather than a : T annotation.
Use .at(0) when an element may be absent; use [0] only after existence is established or with a SAFETY comment.
Skip barrel files named index.ts; import from explicit module paths.
Prefer arrow functions over function expressions.
Destructure parameters when the intermediate variable is not reused.
Prefer discriminator checks such as obj.type === "x" over in checks for discriminated unions; use in only without an available discriminator.
Use typed positional parameters for one argument and readable two-argument calls; use named options, args, or params objects for three or more or interchangeable arguments.
Reuse dependency-pr...
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Test only behavior that can evade the type system, framework, or linter; prefer invariants over examples for large input spaces.
Files:
packages/core/src/controller/fontReadiness.test.ts
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add a changeset for every published-package src change, selecting all affected packages and the appropriate bump; private playground packages need none.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
**/*.{ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve OOXML elements by namespace URI and local name, explicitly support Strict and Transitional profiles, bound ZIP/XML resource use, and preserve paragraph identifiers as facts rather than durable identities.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
packages/core/**/*
📄 CodeRabbit inference engine (packages/core/GEMINI.md)
Follow the coding guidelines and instructions defined in
AGENTS.md.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx,js,jsx}: Keep@stll/folio-coreReact-free: never importreact,react-dom, or React-package types. Put framework-agnostic UI behavior in a core manager extendingSubscribable, use thin framework bindings, and define minimal structural types in core instead of importing adapter types.
Preserve the parser, normalized model, measurement, pagination, and painting boundaries; central pipeline files should orchestrate, while new state concepts or compatibility policies belong in typed helpers or focused modules.
Express every fidelity fix as a reusable OOXML or layout invariant; never branch on fixture identity, source metadata, document text, or other corpus-specific signals.
After roughly five to ten behavior fixes in one subsystem, create a standalone, behavior-preserving consolidation before adding more conditions there.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Prefer discriminated state machines and explicit coordinate-space types over related booleans, optional-field combinations, and mutable flags.
Keep normalization and layout inputs immutable and idempotent; derive effective values instead of overwriting authored model values during measurement or pagination.
Consolidate shared OOXML syntax, units, geometry, and compatibility rules; do not allow feature parsers to develop subtly different implementations.
Files:
packages/core/src/controller/fontReadiness.test.tspackages/core/src/controller/fontReadiness.ts
🪛 LanguageTool
.changeset/font-readiness-resolved-stack.md
[misspelling] ~7-~7: This word is normally spelled as one.
Context: ...d loaded; measurement taken against the pre-load fallback then disagreed with what was u...
(EN_COMPOUNDS_PRE_LOAD)
🔇 Additional comments (4)
packages/core/src/controller/fontReadiness.ts (2)
16-16: LGTM!
255-266: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not skip resolver fallbacks for authored generic families.
addLayoutFontFamilyNameFacereturns before callingresolveFontFamily, so an authored generic value such assans-serifnever adds its concrete fallback faces, and the rendered/measure stack can differ from what the readiness gate waits for. Resolve the input family regardless of the generic value, and keep the generic entry out ofaddLayoutFontFaceusing a case-insensitive comparison. Add a regression test for an authored generic family.Proposed fix
const normalized = family.trim(); - if (!normalized || CSS_GENERIC_FONT_FAMILIES.has(normalized)) { + if (!normalized) { return; } - addLayoutFontFace(f faces, normalized, descriptor); + if (!CSS_GENERIC_FONT_FAMILIES.has(normalized.toLowerCase())) { + addLayoutFontFace(faces, normalized, descriptor); + }> Likely an incorrect or invalid review comment.packages/core/src/controller/fontReadiness.test.ts (1)
132-154: LGTM!.changeset/font-readiness-resolved-stack.md (1)
1-9: LGTM!
| function resolvedStackFamilies(family: string): string[] { | ||
| const { cssFallback } = resolveFontFamily(family); | ||
| const families: string[] = []; | ||
| for (const entry of cssFallback.split(",")) { | ||
| const name = entry.trim().replace(/^["']|["']$/gu, ""); | ||
| if (!name || CSS_GENERIC_FONT_FAMILIES.has(name)) { | ||
| continue; | ||
| } | ||
| families.push(name); | ||
| } | ||
| return families; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'quoteFontName|cssFallback|FONT_MAPPINGS|DEFAULT_FALLBACKS' \
packages/core/src/utils/fontResolver.tsRepository: stella/folio
Length of output: 8530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fontReadiness relevant slice =="
sed -n '240,310p' packages/core/src/controller/fontReadiness.ts
echo
echo "== fontResolver quote/build fallbacks slice =="
sed -n '760,910p' packages/core/src/utils/fontResolver.ts
echo
echo "== search callers of resolveFontFamily and cssexpression CSS parsing =="
rg -n -C 4 'resolveFontFamily\(|resolvedStackFamilies\(|CSS_.*FONT|withArabicFallback|isGeneric|quoteFontName|buildFontFamilyString' packages/core/srcRepository: stella/folio
Length of output: 38267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("packages/core/src/controller/fontReadiness.ts")
text = path.read_text()
m = re.search(r"function resolvedStackFamilies\(family: string\): string\[\] \{(?P<body>.*?)\n\}", text, re.S)
print("resolvedStackFamilies body:")
print(m.group("body") if m else "not found")
text2 = Path("packages/core/src/utils/fontResolver.ts").read_text()
m = re.search(r"function quoteFontName\(fontName: string\): string \{(?P<body>.*?)\n\}", text2, re.S)
print("quoteFontName body:")
print(m.group("body") if m else "not found")
# Reproduce the exact comma-split logic from the body if present
def quoteFontName(name:str) -> str:
generics=["serif","sans-serif","monospace","cursive","fantasy","system-ui"]
if name.lower() in generics:
return name
result = []
for char in name:
if char in ('"', "\\"):
result.append("\\")
if char in ("\n","\r","\f"):
result.append("\\a ")
else:
result.append(char)
return '"' + "".join(result) + '"'
def current(family:str) -> list[str]:
cssFallback = quoteFontName(family) + ", Arial, sans-serif"
families=[]
generic={"serif","sans-serif","monospace","cursive","fantasy","system-ui"}
for entry in cssFallback.split(","):
name=entry.strip().removeprefix('"').removesuffix('"')
if not name or name in generic:
continue
families.append(name)
return families
for input_ in ["Font, Name", "Arial", "Caveat Brush"]:
print(input_, "=>", current(input_))
PYRepository: stella/folio
Length of output: 996
Parse cssFallback with a quote-aware parser.
quoteFontName accepts commas in font-family strings and includes commas in its quoting rules, but resolvedStackFamilies splits resolveFontFamily(family).cssFallback at every comma while only stripping leading/trailing quotes. If cssFallback can contain "Font, Name", this creates two split families instead of one and miscounts the concrete fallback set, which blocks font-readiness for that family. Reject comma-containing fonts here or parse cssFallback with a CSS comma/string-aware parser.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/controller/fontReadiness.ts` around lines 275 - 285, Update
resolvedStackFamilies to handle commas inside quoted font-family names instead
of splitting cssFallback at every comma; use a CSS-aware parser that preserves
entries such as "Font, Name" as one family, or explicitly reject
comma-containing names consistently with quoteFontName. Ensure concrete fallback
counting remains correct.
1 participant
Root cause of the measure/paint divergence the parity oracle in #556 found: a
repaired Arabic line measured 194px and painted 259px, a third of the
line.
Why
resolveFontFamilyexpands an authored family into a CSS stack that appendsfolio's bundled metric-compatible substitutes and a script fallback:
Every stack ends with the bundled Arabic face. So an authored
Arialrun doesnot paint its Arabic in Arial or in Arimo — it paints in Noto Sans Arabic,
which is a webfont that has to load.
The readiness gate collected only the families the document names. It waited
for Arial, released the first layout, and the measurer measured whatever Arabic
face the OS supplied through
sans-serif. The webfont then loaded and thepainter drew that instead. Two faces, two sets of advances, one layout computed
from the wrong one.
This is not Arabic-specific in principle — it is any authored family whose
resolved stack contains a not-yet-loaded bundled face — but Arabic is where it
bites hardest, because the fallback face differs most from a Latin one.
What changed
The gate now expands each collected family through the resolver and waits for
every concrete face in the stack, generics excluded.
That also retires
OFFICE_FONT_FAMILY_MAP, a hand-kept table of Office→bundledsubstitutions that duplicated the resolver's own mapping and was free to drift
from it. The stack is now the single source of truth, which is the point: the
gate waits for exactly what
ctx.fontandstyle.fontFamilywill be set to.Verification
bun --filter @stll/folio-core test: 4534 pass, 0 failbun --filter @stll/folio-core typecheck,oxlint,api:check: cleanScope
Only which faces the gate waits for. The 2s timeout and the absence of
re-layout on a late font load are untouched, and one further gap is worth
noting separately: embedded fonts register under a scoped
folio-embedded-{nonce}-{name}family, and the gate still requests their rawDOCX name, so it waits on a family that is never registered.
Summary by CodeRabbit
Bug Fixes
Tests