Repository navigation
Fix #1793: skip CJK fallback injection when font-family already covers glyphs - #2241
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReworks CJK font-fallback auto-injection by adding CoreText-based glyph-coverage probing, introducing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GhosttyTerminalView
participant Config as Config System
participant CoreText
User->>GhosttyTerminalView: Launch terminal (has `font-family`)
GhosttyTerminalView->>Config: Read font config
Config-->>GhosttyTerminalView: Return font settings
GhosttyTerminalView->>GhosttyTerminalView: shouldInjectCJKFontFallback(rangeCoverageProbe?)
alt Explicit `font-codepoint-map` or multi-entry `font-family`
GhosttyTerminalView-->>User: Skip auto-injection
else Use coverage probe / CoreText
GhosttyTerminalView->>CoreText: configuredCTFont(primary font)
CoreText-->>GhosttyTerminalView: CTFont reference
loop for each CJK range
GhosttyTerminalView->>GhosttyTerminalView: fontContainsGlyphs(CTFont, range) or probe called
GhosttyTerminalView->>CoreText: CTFontGetGlyphsForCharacters (if used)
CoreText-->>GhosttyTerminalView: glyph availability
end
alt Primary font covers all ranges
GhosttyTerminalView-->>User: Do not inject mappings
else
GhosttyTerminalView-->>User: Inject filtered `font-codepoint-map` for uncovered ranges
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes #1793 by teaching cmux's CJK font-fallback injection to first probe the user's configured Confidence Score: 4/5Safe to merge after addressing the two-commit policy and the minor redundant guard; core CJK probe logic and tests are correct. The CJK coverage-probe design is sound — the injectable probe, conservative fallback when the font can't be resolved, and per-range sample approach are all correct. The bundled configTemplate refactor looks correct. The only actionable items are a trivial redundant check and the CLAUDE.md commit-ordering policy (not a runtime correctness issue). Sources/GhosttyTerminalView.swift — redundant inner count guard (line 3607) and the Hangul dead-code entries warrant attention before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[loadCJKFontFallbackIfNeeded] --> B[autoInjectedCJKFontMappings]
B --> C{cjkFontMappings returns nil?}
C -- yes --> Z[return nil — no injection]
C -- no --> D{containsCodepointMap OR\nhasExplicitFallbackChain?}
D -- yes --> Z
D -- no --> E{effectiveFontFamilies.first?}
E -- nil --> F[return all mappings]
E -- found --> G{rangeCoverageProbe set?}
G -- yes --> H[probe each range via closure]
G -- no --> I[configuredCTFont named:]
I --> J{font resolved?}
J -- no --> F
J -- yes --> K[fontContainsGlyphs per range\nvia CTFontGetGlyphsForCharacters]
H --> L[removeAll covered ranges]
K --> L
L --> M{mappings empty?}
M -- yes --> Z
M -- no --> N[return filtered mappings\n→ inject partial fallback]
Reviews (1): Last reviewed commit: "fix: honor CJK-capable font-family befor..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let count = Int(surfaceConfig.env_var_count) | ||
| if count > 0 { |
There was a problem hiding this comment.
count > 0 guard
The outer condition surfaceConfig.env_var_count > 0 already establishes that env_var_count is positive, so converting to Int and re-checking count > 0 on line 3607 is unreachable as false. It can be removed to reduce nesting and make intent clearer.
| let count = Int(surfaceConfig.env_var_count) | |
| if count > 0 { | |
| var env: [String: String] = [:] | |
| if surfaceConfig.env_var_count > 0, let existingEnv = surfaceConfig.env_vars { | |
| let count = Int(surfaceConfig.env_var_count) | |
| for i in 0..<count { |
| /// Representative scalars used to detect whether the configured primary | ||
| /// font already covers the ranges cmux would otherwise auto-map. | ||
| private static let cjkCoverageSampleCharactersByRange: [String: [UniChar]] = [ | ||
| "U+3000-U+303F": [0x3001, 0x300C], | ||
| "U+4E00-U+9FFF": [0x4E00, 0x65E5, 0x6C34], | ||
| "U+F900-U+FAFF": [0xF900], | ||
| "U+FF00-U+FFEF": [0xFF10, 0xFF21], | ||
| "U+3400-U+4DBF": [0x3400], | ||
| "U+1100-U+11FF": [0x1100, 0x1161], | ||
| "U+3130-U+318F": [0x3131, 0x314F], | ||
| "U+3040-U+309F": [0x3042, 0x3093], | ||
| "U+30A0-U+30FF": [0x30A2, 0x30F3], | ||
| "U+AC00-U+D7AF": [0xAC00, 0xD55C], | ||
| ] |
There was a problem hiding this comment.
cjkCoverageSampleCharactersByRange includes three Hangul ranges (U+1100-U+11FF, U+3130-U+318F, U+AC00-U+D7AF) that fontContainsGlyphs would look up when filtering. However, cjkFontMappings has no ko/Korean language branch, so those ranges are never present in the mappings array that drives removeAll. The fontContainsGlyphs call will never be invoked with any of those range keys.
The PR description notes these entries are intentionally forward-looking for #2037 Korean support — a comment in the dictionary explaining this would help future readers understand why entries exist that no current code path exercises.
| /// Representative scalars used to detect whether the configured primary | |
| /// font already covers the ranges cmux would otherwise auto-map. | |
| private static let cjkCoverageSampleCharactersByRange: [String: [UniChar]] = [ | |
| "U+3000-U+303F": [0x3001, 0x300C], | |
| "U+4E00-U+9FFF": [0x4E00, 0x65E5, 0x6C34], | |
| "U+F900-U+FAFF": [0xF900], | |
| "U+FF00-U+FFEF": [0xFF10, 0xFF21], | |
| "U+3400-U+4DBF": [0x3400], | |
| "U+1100-U+11FF": [0x1100, 0x1161], | |
| "U+3130-U+318F": [0x3131, 0x314F], | |
| "U+3040-U+309F": [0x3042, 0x3093], | |
| "U+30A0-U+30FF": [0x30A2, 0x30F3], | |
| "U+AC00-U+D7AF": [0xAC00, 0xD55C], | |
| ] | |
| /// Representative scalars used to detect whether the configured primary | |
| /// font already covers the ranges cmux would otherwise auto-map. | |
| /// Hangul ranges (U+1100, U+3130, U+AC00) are included ahead of Korean | |
| /// language support (see #2037) so the probe table stays in sync when that | |
| /// work lands. | |
| private static let cjkCoverageSampleCharactersByRange: [String: [UniChar]] = [ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1407-1420: The new Hangul probe entries in
cjkCoverageSampleCharactersByRange are never checked because
autoInjectedCJKFontMappings() only filters ranges emitted by cjkFontMappings(),
and cjkFontMappings() currently only yields JP/CN ranges; update
cjkFontMappings() to also emit the Korean ranges ("U+1100-U+11FF",
"U+3130-U+318F", "U+AC00-U+D7AF") when Korean locale is detected (or include
them unconditionally if that's intended), or alternatively change
autoInjectedCJKFontMappings() to consult cjkCoverageSampleCharactersByRange
directly rather than relying solely on cjkFontMappings(); reference the
functions cjkFontMappings(), autoInjectedCJKFontMappings(), and the constant
cjkCoverageSampleCharactersByRange to locate where to add those ranges and
adjust the filtering logic accordingly.
- Around line 1517-1524: The current logic uses fontContainsGlyphs to probe a
few code points and then removes the entire fallback range from mappings, which
incorrectly suppresses fallback for partially-covered Unicode blocks; replace
this with a full-range coverage check (e.g., implement and call a
fontFullyContains(range: RangeType, font: CTFont) that verifies every code
point/glyph in the range is present) and use that result in the
mappings.removeAll closures instead of fontContainsGlyphs; apply the same
replacement where similar logic appears (the rangeCoverageProbe branch and the
branch using configuredCTFont(named:) — references: rangeCoverageProbe,
configuredCTFont(named:), fontContainsGlyphs, and the mappings.removeAll
closures).
- Around line 2965-2966: The stored type change of configTemplate to
ghostty_surface_config_s? breaks callers that still construct
TerminalSurface(configTemplate:) with CmuxSurfaceConfigTemplate? and loses extra
metadata (fontFamily, fontFeatures); revert the stored/initializer type back to
CmuxSurfaceConfigTemplate? (or make TerminalSurface initializer
generic/overloaded to accept CmuxSurfaceConfigTemplate and extract its .cConfig
into ghostty_surface_config_s while preserving metadata fields), updating the
TerminalSurface initializer and the stored property configTemplate so callers
using CmuxSurfaceConfigTemplate continue to compile and metadata is retained.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9af789be-e859-4e80-bd90-4c480a6d7fed
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
| if let rangeCoverageProbe { | ||
| mappings.removeAll { range, _ in | ||
| rangeCoverageProbe(configuredFontFamily, range) | ||
| } | ||
| } else if let configuredFont = configuredCTFont(named: configuredFontFamily) { | ||
| mappings.removeAll { range, _ in | ||
| fontContainsGlyphs(configuredFont, forRange: range) | ||
| } |
There was a problem hiding this comment.
A few probe glyphs are not enough to suppress an entire Unicode block.
fontContainsGlyphs only proves that a small sample exists, but Lines 1518-1524 remove the whole fallback range when that returns true. Partial-coverage fonts will therefore skip the injected fallback and still render missing glyphs for unsupported code points inside the same block.
Also applies to: 1579-1589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 1517 - 1524, The current
logic uses fontContainsGlyphs to probe a few code points and then removes the
entire fallback range from mappings, which incorrectly suppresses fallback for
partially-covered Unicode blocks; replace this with a full-range coverage check
(e.g., implement and call a fontFullyContains(range: RangeType, font: CTFont)
that verifies every code point/glyph in the range is present) and use that
result in the mappings.removeAll closures instead of fontContainsGlyphs; apply
the same replacement where similar logic appears (the rangeCoverageProbe branch
and the branch using configuredCTFont(named:) — references: rangeCoverageProbe,
configuredCTFont(named:), fontContainsGlyphs, and the mappings.removeAll
closures).
| private let configTemplate: ghostty_surface_config_s? | ||
| private let workingDirectory: String? |
There was a problem hiding this comment.
This configTemplate type change is a compile blocker.
Sources/Workspace.swift:5701-5757 still constructs TerminalSurface(configTemplate:) with CmuxSurfaceConfigTemplate?, so switching the stored/initializer type here to ghostty_surface_config_s? stops that call site from type-checking. Even if callers are changed to pass only .cConfig, that also drops the extra template metadata (fontFamily, fontFeatures) still carried by CmuxSurfaceConfigTemplate.
Also applies to: 3050-3054
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 2965 - 2966, The stored type
change of configTemplate to ghostty_surface_config_s? breaks callers that still
construct TerminalSurface(configTemplate:) with CmuxSurfaceConfigTemplate? and
loses extra metadata (fontFamily, fontFeatures); revert the stored/initializer
type back to CmuxSurfaceConfigTemplate? (or make TerminalSurface initializer
generic/overloaded to accept CmuxSurfaceConfigTemplate and extract its .cConfig
into ghostty_surface_config_s while preserving metadata fields), updating the
TerminalSurface initializer and the stored property configTemplate so callers
using CmuxSurfaceConfigTemplate continue to compile and metadata is retained.
e640b22 to
089bbe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/GhosttyTerminalView.swift (2)
1517-1524:⚠️ Potential issue | 🟠 MajorSample glyph hits are too weak to drop an entire block.
Lines 1517-1524 remove a whole fallback range when
fontContainsGlyphssucceeds, but Lines 1579-1589 only verify a few representative scalars. A partially covered font can therefore lose the injected fallback for missing code points inside that block. This needs a full-range coverage check, or a more conservative suppression rule that only removes ranges you can fully verify.Also applies to: 1579-1589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1517 - 1524, The current logic removes entire fallback ranges when fontContainsGlyphs reports success but that function only checks a few representative scalars; instead ensure you only drop a mapping when you can verify full-range coverage: modify the removal branch that uses configuredCTFont(named:) and fontContainsGlyphs (and the alternative rangeCoverageProbe path) so it either (a) calls a new or extended fontContainsGlyphs that iterates every scalar in the given Range and returns true only if all code points are present, or (b) changes the removeAll predicate to be conservative and only remove a range when a full-rangeCoverageProbe verifies every code point; update both the mappings.removeAll closures (the branch using rangeCoverageProbe and the branch using configuredCTFont(named:)) so they use the full-range verification before removing the fallback mapping.
1407-1420:⚠️ Potential issue | 🟠 MajorHangul filtering still can't run on the real Korean path.
Line 1506 still seeds filtering from
cjkFontMappings(...), and that generator only emits Japanese/Chinese ranges in this file. The newU+1100-U+11FF,U+3130-U+318F, andU+AC00-U+D7AFsamples can be exercised viarangeCoverageProbe, butko*locales still produce no Hangul mappings to filter, so the D2Coding-style override regression remains. Please emit Korean mappings from the baseline generator, or drive filtering from the sample-map directly.Also applies to: 1501-1506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1407 - 1420, The current CJK filtering is still seeded from cjkFontMappings(...) which omits Korean ranges, so update the filtering source to include Hangul: either extend the baseline generator function cjkFontMappings to emit mappings for Korean ranges (U+1100-U+11FF, U+3130-U+318F, U+AC00-U+D7AF) using the sample scalars in cjkCoverageSampleCharactersByRange, or change the filter initialization (the place that currently calls cjkFontMappings to seed filtering) to drive rangeCoverageProbe (or equivalent coverage probing) directly from cjkCoverageSampleCharactersByRange so ko* locales will produce Hangul mappings to filter (reference symbols: cjkFontMappings, cjkCoverageSampleCharactersByRange, rangeCoverageProbe).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 2152-2161: The test currently calls
GhosttyApp.autoInjectedCJKFontMappings with a rangeCoverageProbe but never
asserts that the probe was invoked; add a small local flag or counter in the
surrounding withTempConfig closure, increment or set it inside the provided
rangeCoverageProbe (for the call to GhosttyApp.autoInjectedCJKFontMappings) and
after the call assert that the flag/counter is >0 so the test fails if
rangeCoverageProbe was never exercised; apply the same change to the other test
using GhosttyApp.autoInjectedCJKFontMappings at the second location.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1517-1524: The current logic removes entire fallback ranges when
fontContainsGlyphs reports success but that function only checks a few
representative scalars; instead ensure you only drop a mapping when you can
verify full-range coverage: modify the removal branch that uses
configuredCTFont(named:) and fontContainsGlyphs (and the alternative
rangeCoverageProbe path) so it either (a) calls a new or extended
fontContainsGlyphs that iterates every scalar in the given Range and returns
true only if all code points are present, or (b) changes the removeAll predicate
to be conservative and only remove a range when a full-rangeCoverageProbe
verifies every code point; update both the mappings.removeAll closures (the
branch using rangeCoverageProbe and the branch using configuredCTFont(named:))
so they use the full-range verification before removing the fallback mapping.
- Around line 1407-1420: The current CJK filtering is still seeded from
cjkFontMappings(...) which omits Korean ranges, so update the filtering source
to include Hangul: either extend the baseline generator function cjkFontMappings
to emit mappings for Korean ranges (U+1100-U+11FF, U+3130-U+318F, U+AC00-U+D7AF)
using the sample scalars in cjkCoverageSampleCharactersByRange, or change the
filter initialization (the place that currently calls cjkFontMappings to seed
filtering) to drive rangeCoverageProbe (or equivalent coverage probing) directly
from cjkCoverageSampleCharactersByRange so ko* locales will produce Hangul
mappings to filter (reference symbols: cjkFontMappings,
cjkCoverageSampleCharactersByRange, rangeCoverageProbe).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3677e162-931e-48ff-b0d1-9c3c6603473c
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
| try withTempConfig("font-family = Sarasa Mono K\n") { path in | ||
| XCTAssertNil( | ||
| GhosttyApp.autoInjectedCJKFontMappings( | ||
| preferredLanguages: ["zh-Hans-CN"], | ||
| configPaths: [path], | ||
| rangeCoverageProbe: { fontFamily, range in | ||
| XCTAssertEqual(fontFamily, "Sarasa Mono K") | ||
| return coveredRanges.contains(range) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Assert that rangeCoverageProbe is actually invoked to avoid false positives.
Both tests can pass even if the probe path is never exercised. Add an explicit invocation flag/counter and assert it after the call so regressions in probe wiring are caught.
Proposed test-hardening diff
func testAutoInjectedCJKFontMappingsSkipsRangesCoveredByConfiguredPrimaryFont() throws {
let coveredRanges: Set<String> = [
"U+3000-U+303F",
"U+4E00-U+9FFF",
"U+F900-U+FAFF",
"U+FF00-U+FFEF",
"U+3400-U+4DBF",
]
try withTempConfig("font-family = Sarasa Mono K\n") { path in
+ var probeInvoked = false
XCTAssertNil(
GhosttyApp.autoInjectedCJKFontMappings(
preferredLanguages: ["zh-Hans-CN"],
configPaths: [path],
rangeCoverageProbe: { fontFamily, range in
+ probeInvoked = true
XCTAssertEqual(fontFamily, "Sarasa Mono K")
return coveredRanges.contains(range)
}
)
)
+ XCTAssertTrue(probeInvoked)
}
}
func testShouldInjectCJKFontFallbackSkipsConfiguredFontThatAlreadyCoversMappedRanges() throws {
let coveredRanges: Set<String> = [
"U+3000-U+303F",
"U+4E00-U+9FFF",
"U+F900-U+FAFF",
"U+FF00-U+FFEF",
"U+3400-U+4DBF",
]
try withTempConfig("font-family = Sarasa Mono K\n") { path in
+ var probeInvoked = false
XCTAssertFalse(
GhosttyApp.shouldInjectCJKFontFallback(
preferredLanguages: ["zh-Hans-CN"],
configPaths: [path],
rangeCoverageProbe: { fontFamily, range in
+ probeInvoked = true
XCTAssertEqual(fontFamily, "Sarasa Mono K")
return coveredRanges.contains(range)
}
)
)
+ XCTAssertTrue(probeInvoked)
}
}Also applies to: 2446-2455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/GhosttyConfigTests.swift` around lines 2152 - 2161, The test
currently calls GhosttyApp.autoInjectedCJKFontMappings with a rangeCoverageProbe
but never asserts that the probe was invoked; add a small local flag or counter
in the surrounding withTempConfig closure, increment or set it inside the
provided rangeCoverageProbe (for the call to
GhosttyApp.autoInjectedCJKFontMappings) and after the call assert that the
flag/counter is >0 so the test fails if rangeCoverageProbe was never exercised;
apply the same change to the other test using
GhosttyApp.autoInjectedCJKFontMappings at the second location.
This branch was successfully deployed
1 participant
Closes #1793.
Summary
font-familywith CoreText before auto-injecting CJKfont-codepoint-mapentriesfont-codepoint-mapsettings and multi-font fallback chainsVerification
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag cjk-font-override --launchbecause the plainreload.shpath currently fails on this machine in an unrelated Ghostty helper link stepSummary by CodeRabbit
Improvements
Tests