fix(design-sync): match mono/code as whole words in font classifier (CE-7 review) - #473
Conversation
PR #472 review F1: prefix matching on "mono"/"code" classed sans and display fonts (Codec Pro, Monotype Corsiva, Monoton) as monospace, so they fell back to Courier New, worse than the old blanket sans-serif. Brand stems (courier, consol, menlo, monaco) keep prefix matching. Also F5: heading-override unit test that fails if _replace_heading_font goes back to html.escape; F6: tighten the serif endswith assert; F7: rename tests that now check stripping, not escaping. Plan D3 updated. Converter output unchanged: all 7 snapshot tests pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
linardsb
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (with two follow-ups). Recorded as a comment because GitHub blocks a formal approval of a self-authored PR.
PR #473 review — match mono/code as whole words in font classifier (CE-7 review)
Head d685e100 · Base main @ da1524e381d1ec7957328eaf76ec49a5d4bebd83 (live origin/main tip at review time = same sha) · round 1
Summary
Carries the PR #472 round-1 fixes F1, F5, F6, F7. The F1 fix is correct for the names it targets: Codec Pro, Monotype Corsiva, Monoton, Monofett now get a sans stack and Monotype Garamond a serif stack. The change has one side effect the PR body does not mention: real monospace fonts whose name is a single word starting with mono, plus a CSS generic monospace in first position, now also fall to sans. Neither is in the corpus (converter output is byte-identical, observed), so neither blocks merge.
Issues
Critical — none. High — none.
Medium
F1 · app/design_sync/font_stacks.py:57,102 — single-word monospace fonts now get a sans fallback (undocumented).
Whole-word mono drops every mono font whose name is one token beginning with mono (observed, old vs new font_category run against origin/main:font_stacks.py):
| Family | origin/main | this PR |
|---|---|---|
MonoLisa |
monospace | sans-serif |
Monoid |
monospace | sans-serif |
Mononoki |
monospace | sans-serif |
Monofur |
monospace | sans-serif |
CodeNewRoman |
monospace | sans-serif |
The PR body says only that Consolas stays mono; it does not state that these move. The behaviour equals pre-#472 (everything was sans), which is why this is Medium, not High.
Fix: add the stems to _MONO_PREFIXES (monolisa, mononoki, monoid, monofur) with parametrize cases, or record the trade in the PR body and plan D3. Prefix monoid does not catch Monotype/Monoton/Monofett.
Low
F2 · app/design_sync/font_stacks.py:102,131 — a generic monospace in first position now classifies as sans.
font_stack short-circuits only when the last family is generic. A list that starts with one and does not end with one reaches font_category, which no longer matches monospace (observed):
"ui-monospace, Menlo"→ old…, 'Courier New', Courier, monospace; new…, Helvetica, Arial, sans-serif."monospace, Foo"→ same flip.
Low because the input needs a hand-authored list that starts with a generic and lacks a trailing one; no current producer emits that.
Fix: add monospace to _MONO_TOKENS (matches ui-monospace via the hyphen split) plus a test case.
Reviewer agent also proposed Nanum Gothic Coding as a regression; rejected: "coding".startswith("code") is false, so it was sans on origin/main too (observed).
Numbers pass
| Claim (PR body) | Check | Result |
|---|---|---|
| 6 files, 29+/9− | gh pr view additions/deletions, git diff --stat origin/main...HEAD |
matches (observed) |
make check-full exit 0: 9130 passed, 123 skipped; vitest 780 |
re-ran make check-full on d685e100 |
9130 passed, 123 skipped, 7 xfailed; vitest 780; exit 0 (observed) |
test_font_stacks.py has 86 tests |
pytest --collect-only |
86 (observed) |
RED: 5 failed, 24 passed with -k font_category on unfixed code |
-k font_category collects 29; old classifier returns MONO for exactly the 5 new F1 names, SANS-expecting none else |
consistent: 29 − 5 = 24 (derived, not re-run) |
| F5 mutation → 1 failed | reviewer agent read the test: html.escape produces ', failing both asserts |
consistent (derived, mutation not re-run) |
Tree identical to 56c31a47 |
git diff --stat 56c31a47 HEAD |
empty (observed) |
| A3 unchanged | derived from byte-identical output; correctly labelled derived | OK |
Validation
| Gate | Result |
|---|---|
make check-full @ d685e100 |
exit 0: ruff clean, mypy "no issues in 1398 files", pyright 0 errors, pytest 9130 passed / 123 skipped, vitest 780 passed, flag audit 0 errors / 14 warnings, .env.example in sync (observed) |
CI (gh pr checks 473) |
all 14 checks pass, incl. backend, frontend, CodeQL, Semgrep (observed) |
Code-scanning alerts on refs/pull/473/merge |
0 open (observed) |
| Deferred items | no entry references font_stacks, CE-7 or font_category |
Side note: make check-full rewrites the date in app/ai/agents/{dark_mode,scaffolder}/skill-versions.yaml to today. Not this PR's concern; restored after the run.
Done well
- Three-line production change; the comments on
_MONO_TOKENSand_MONO_PREFIXESsay why each is whole-word or prefix. Monotype Garamond→ SERIF also proves the serif branch still runs once mono is ruled out.test_heading_font_override_quotes_unescapedpins the exact output string and the absence of', so a revert tohtml.escapefails it.- F6's
endswith(", serif")closes thesans-seriffalse pass. - Plan D3 and T2 updated to match the code.
Recommendation
Approve. F1 is a two-line fix plus four test cases and is worth doing in this PR or a follow-up; F2 can ride with it.
Whole-word mono/code matching (PR #472 F1) dropped one-word monospace families (MonoLisa, Monoid, Mononoki, Monofur, CodeNewRoman) and a leading generic monospace/ui-monospace to the sans stack. Add the brand stems as prefixes and monospace as a whole token (PR #473 review F1, F2). Converter output unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
PR #472 was merged before its round-1 review fixes landed. This PR carries them. The main fix is review F1: the font classifier matched
monoandcodeas name prefixes, so sans and display fonts such asCodec Pro,Monotype CorsivaandMonotongot a Courier New fallback. That is worse than the blanketsans-serifused before #472. Nowmono/codematch only as whole words. The brand stems (courier,consol,menlo,monaco) still match as prefixes, soConsolasstays mono.What changed
app/design_sync/font_stacks.py: new_MONO_TOKENS = {"mono", "code", "monospace"}(whole tokens);_MONO_PREFIXEScut down to the brand stems, plus the one-word mono brandsmonolisa,monoid,mononoki,monofur,codenewroman(fix(design-sync): match mono/code as whole words in font classifier (CE-7 review) #473 review F1).monospacekeeps a leadingui-monospace/monospacegeneric mono (fix(design-sync): match mono/code as whole words in font classifier (CE-7 review) #473 review F2).test_font_stacks.py: the reviewer's namesCodec Pro,Monotype Corsiva,Monoton,Monofett→ sans,Monotype Garamond→ serif,Source Code Pro→ mono; fix(design-sync): match mono/code as whole words in font classifier (CE-7 review) #473 review:MonoLisa,Monoid,Mononoki,Monofur,CodeNewRoman→ mono, and"ui-monospace, Menlo"/"monospace, Foo"pinned to theirorigin/mainstacks. The serif check now usesendswith(", serif"), sosans-serifno longer passes it (F6).test_component_renderer.py:test_heading_font_override_quotes_unescaped, which fails if_replace_heading_fontgoes back tohtml.escape(F5).*escape*font tests to*strip*, since they now check that characters are stripped (F7)..agents/plans/ce-7-font-fallback-stacks.md: D3 and the classifier IMPLEMENT line now describe whole-word matching.Diff: 6 files, 60+/9− (observed,
git diff --stat origin/main...HEADat94e84b95).Validation
observed—make check-fullviarecord-gate.sh, based685e100plus the uncommitted #473 review fixes (the same app/plan tree committed as94e84b95; derived from the staged set), exit 0:pytest test_font_stacks.py -k font_category). F5: the reviewer's mutation (html.escape(font_stack(font), quote=True)in_replace_heading_font) → 1 failed (observed).test_font_stacks.pyhas 93 tests (observed,pytest test_font_stacks.py93 passed). fix(design-sync): match mono/code as whole words in font classifier (CE-7 review) #473 review RED: unfixed classifier with the 7 new cases → 7 failed, 86 passed (observed). 9130 + 7 = 9137 (derived).data/changed (observed). The A3 scores are therefore unchanged (derived from identical output; the scorer was not re-run). None of the corpus first families (Arial, Courier New, Geist Mono, Helvetica, Inter, Noto Sans, Roboto,-apple-system) contains a token that starts withmono/codewithout being exactly that word.Notes for the reviewer
56c31a47, also sits on the merged branchfeat/ce-7-font-fallback-stacks. This PR cherry-picks it ontoda1524e3, and the tree is identical to56c31a47.Linked
Review fixes (round 1 of #473):
.claude/reports/pr-473-review-fixes.md(F1, F2).Review:
.claude/code-reviews/pr-472-review.md(F1, F5–F7). Follows #472.Opened as a draft; CI's
readyjob flips it when every ci.yml job (backend,frontend,sdk-check,trivy,migrations,integration,migration-lint,commit-lint,e2e-smoke) andcodeqlare green on this head. A red job leaves it here with the failing check on the PR.🤖 Generated with Claude Code