fix: add CJK font fallback to prevent decorative font rendering - #1017
Merged
Merged
Conversation
On macOS, Core Text's CTFontCreateForString may pick an inappropriate fallback font (e.g. LingWai, a decorative calligraphic font) for CJK characters when the primary font (e.g. Menlo) does not cover them. This adds automatic CJK font fallback based on the system's preferred language: - ja → Hiragino Sans - ko → Apple SD Gothic Neo - zh-Hant/zh-TW/zh-HK → PingFang TC - zh → PingFang SC The fallback is only applied when: 1. The user has not set any font-codepoint-map in their Ghostty config 2. A CJK language is detected in the system's preferred languages This ensures CJK text renders with appropriate system fonts instead of relying on Core Text's unpredictable fallback chain.
|
@atani is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInjects a locale-aware CJK font-codepoint-map into Ghostty’s config loading when no user-provided CJK map exists: detects existing maps (including includes), computes language-specific font mappings, writes a temporary mapping file, and integrates it during default config loading. Tests cover mapping logic and include traversal. Changes
Sequence Diagram(s)sequenceDiagram
participant App as GhosttyApp
participant Loader as Config Loader
participant Locale as System Locale
participant FS as File System
participant Temp as Temp File
App->>Loader: loadDefaultConfigFilesWithLegacyFallback(config)
Loader->>App: invoke loadCJKFontFallbackIfNeeded(config)
App->>FS: scan default config paths (and included files) for font-codepoint-map
alt user config contains map
App-->>Loader: skip fallback injection
else
App->>Locale: read preferredLanguages
Locale-->>App: return languages
App->>App: compute cjkFontMappings(preferredLanguages)
alt mappings available
App->>Temp: write temporary font-codepoint-map
Temp-->>FS: persist temp mapping
App->>Loader: include temp mapping in load sequence
else
App-->>Loader: no mapping injected
end
end
Loader-->>App: finalize config load
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
1395-1426: Consider extracting temp config file setup into a helper to reduce duplication.The setup/cleanup pattern is repeated in multiple tests; a small helper would make this block shorter and easier to maintain.
♻️ Optional refactor sketch
+ private func withTempConfigFile( + contents: String, + body: (String) -> Void + ) throws { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-test-cjk-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let file = dir.appendingPathComponent("config") + try contents.write(to: file, atomically: true, encoding: .utf8) + body(file.path) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1395 - 1426, Extract the repeated temp config setup/cleanup into a small helper (e.g., createTempConfigFile(contents: String) -> String or with signature createTempDirAndFile(contents:name:) returning the file path and handling directory creation and a cleanup closure), then replace the duplicated code in testUserConfigContainsCJKCodepointMapReturnsTrue, testUserConfigContainsCJKCodepointMapReturnsFalseWhenAbsent, and testUserConfigContainsCJKCodepointMapIgnoresComments to call that helper and use its returned path (or cleanup closure) instead of manually creating tmpDir, writing files, and deferring removal.
🤖 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`:
- Line 1003: The guard that prevents injecting the fallback CJK map runs before
recursive includes are loaded, and userConfigContainsCJKCodepointMap only scans
a hardcoded top-level path list, so maps defined in included files are missed;
fix by running ghostty_config_load_recursive_files(config) before calling
userConfigContainsCJKCodepointMap (or update userConfigContainsCJKCodepointMap
to scan the full merged/recursive config state rather than only the top-level
paths) and ensure the fallback-injection logic (the guard around injecting the
font-codepoint-map) queries that updated/merged config so it detects maps from
included files.
- Around line 1022-1028: The temp file is created at a predictable path
(tmpPath) which risks races and symlink attacks; instead create a uniquely named
temporary file URL (e.g., using FileManager.temporaryDirectory and UUID or
FileManager.createTemporaryDirectory/createFile) and write `lines` to that file,
call `ghostty_config_load_file(config, path)` with the unique file's C string,
and ensure removal in a `defer` block (or equivalent guaranteed cleanup) rather
than an unconditional try? removeItem call; update references to `tmpPath`,
`lines.write(toFile:…,encoding:)`, `ghostty_config_load_file`, and
`FileManager.default.removeItem(atPath:)` accordingly.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1395-1426: Extract the repeated temp config setup/cleanup into a
small helper (e.g., createTempConfigFile(contents: String) -> String or with
signature createTempDirAndFile(contents:name:) returning the file path and
handling directory creation and a cleanup closure), then replace the duplicated
code in testUserConfigContainsCJKCodepointMapReturnsTrue,
testUserConfigContainsCJKCodepointMapReturnsFalseWhenAbsent, and
testUserConfigContainsCJKCodepointMapIgnoresComments to call that helper and use
its returned path (or cleanup closure) instead of manually creating tmpDir,
writing files, and deferring removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1416762-6f61-4b07-a624-a6aa42eef9ae
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
Contributor
Greptile SummaryThis PR injects a Critical issues:
Minor issue:
Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant App as GhosttyApp
participant FS as FileSystem
participant Lang as Locale.preferredLanguages
participant Ghostty as ghostty_config_t
App->>FS: userConfigContainsCJKCodepointMap()<br/>reads hardcoded config paths
FS-->>App: true / false
alt user already has font-codepoint-map
App-->>App: skip (no injection)
else no existing config found
App->>Lang: preferredCJKFontFamily()
Lang-->>App: "Hiragino Sans" / "Apple SD Gothic Neo" / "PingFang SC/TC" / nil
alt CJK language detected
App->>FS: write cmux-cjk-font-fallback.conf<br/>(static temp path — race condition)
App->>Ghostty: ghostty_config_load_file(tmpPath)
App->>FS: removeItem(tmpPath)
end
end
App->>Ghostty: ghostty_config_load_recursive_files()<br/>(user config loaded here, overrides fallback)
App->>Ghostty: ghostty_config_finalize()
Last reviewed commit: f5c2f2d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:1002">
P2: CJK fallback is decided before recursive includes are loaded, so `font-codepoint-map` settings in included config files can be missed and defaults may be injected unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Split Unicode ranges by language to avoid mapping Hangul to Hiragino Sans or Kana to Apple SD Gothic Neo. Shared CJK ranges (ideographs, symbols, fullwidth forms) use the first CJK language's font, while script-specific ranges (Kana, Hangul) only map to their own font. - Use UUID-based temp file path to prevent race conditions on concurrent launches. - Move fallback injection after ghostty_config_load_recursive_files so that config-file includes are already loaded when checking for existing font-codepoint-map entries. - Follow config-file directives when scanning for existing font-codepoint-map entries. - Extract test helper withTempConfig to reduce duplication. - Add tests for multi-language mappings and config-file includes. - Replace placeholder issue URL with actual PR link.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
1362-1402: PreferXCTUnwrapover force-unwrapping in tests.Using
!here can crash without context;XCTUnwrapgives clearer assertion failures.Suggested patch
func testCJKFontMappingsReturnsHiraginoWithKanaForJapanese() { - let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "en-US"])! + let mappings = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "en-US"]) + ) @@ func testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean() { - let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ko-KR"])! + let mappings = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["ko-KR"]) + ) @@ func testCJKFontMappingsReturnsPingFangForChinese() { - let mappingsTW = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hant-TW"])! + let mappingsTW = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hant-TW"]) + ) @@ - let mappingsCN = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hans-CN"])! + let mappingsCN = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hans-CN"]) + ) @@ - let mappingsHK = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-HK"])! + let mappingsHK = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-HK"]) + ) @@ func testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges() { - let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "ko-KR"])! + let mappings = try! XCTUnwrap( + GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "ko-KR"]) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1362 - 1402, The tests currently force-unwrap GhosttyApp.cjkFontMappings(...) with "!" which can crash without helpful context; replace those force-unwraps with XCTest's XCTUnwrap to produce readable assertion failures. For each test that does a "...!": update the test signature to "throws" and change lines like let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: [...])! to let mappings = try XCTUnwrap(GhosttyApp.cjkFontMappings(preferredLanguages: [...])) (apply the same change for mappingsTW/mappingsCN/mappingsHK and any other forced unwraps), keeping all existing assertions unchanged. Ensure you import XCTest and handle the thrown errors by marking the test methods (e.g., testCJKFontMappingsReturnsPingFangForChinese, testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean, testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges, etc.) with "throws".
🤖 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 1433-1436: The test
testUserConfigContainsCJKCodepointMapReturnsFalseForMissingFiles should use a
deterministic per-run non-existent path instead of a fixed absolute path; change
the hardcoded "/nonexistent/path/config" to a path created under the system temp
directory (e.g., NSTemporaryDirectory or FileManager.temporaryDirectory)
appended with a UUID filename so it is unique and guaranteed to not exist, then
pass that generated path into
GhosttyApp.userConfigContainsCJKCodepointMap(configPaths:).
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1123-1141: The function configFileContainsCodepointMap currently
resolves includes only via expanding tildes and can recurse indefinitely; update
it to resolve relative include paths against the parent file directory (use the
current file's directory—e.g., (path as NSString).deletingLastPathComponent—to
join non-absolute includePath before expanding) and add a visited-set parameter
(e.g., visited: inout Set<String> or a private helper that accepts visited) to
track already-checked absolute paths and return false immediately on repeats to
prevent include cycles; ensure recursion uses the normalized/absolute resolved
path when checking/adding to the visited set and when calling
configFileContainsCodepointMap recursively.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1362-1402: The tests currently force-unwrap
GhosttyApp.cjkFontMappings(...) with "!" which can crash without helpful
context; replace those force-unwraps with XCTest's XCTUnwrap to produce readable
assertion failures. For each test that does a "...!": update the test signature
to "throws" and change lines like let mappings =
GhosttyApp.cjkFontMappings(preferredLanguages: [...])! to let mappings = try
XCTUnwrap(GhosttyApp.cjkFontMappings(preferredLanguages: [...])) (apply the same
change for mappingsTW/mappingsCN/mappingsHK and any other forced unwraps),
keeping all existing assertions unchanged. Ensure you import XCTest and handle
the thrown errors by marking the test methods (e.g.,
testCJKFontMappingsReturnsPingFangForChinese,
testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean,
testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges, etc.) with "throws".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4522a04f-9a6c-4da2-a311-a840cbd4eb5b
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:1139">
P2: `config-file` include path parsing is incomplete (relative and optional `?` paths), so existing user `font-codepoint-map` entries in included files may be missed.</violation>
<violation number="2" location="Sources/GhosttyTerminalView.swift:1140">
P1: Recursive include scanning lacks cycle detection, which can cause infinite recursion/stack overflow on cyclic `config-file` includes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Add cycle detection via visited path set to prevent infinite recursion on cyclic config-file includes. - Resolve relative include paths against the parent directory of the including config file. - Strip trailing '?' from optional include paths (Ghostty convention). - Use UUID-based path for missing file test. - Add tests for relative includes, optional includes, and cyclic includes.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmuxTests/GhosttyConfigTests.swift (1)
1342-1357: CJK font fallback tests are misplaced inGhosttyMouseFocusTests.The CJK font fallback tests and helper function are added inside
GhosttyMouseFocusTests, which is semantically unrelated to mouse focus behavior. Consider moving these to a dedicatedCJKFontFallbackTestsclass or intoGhosttyConfigTestsfor better organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1342 - 1357, The CJK font fallback helper withTempConfig and its related tests were added inside the GhosttyMouseFocusTests class but belong in a config-related test suite; move the private func withTempConfig(_ contents: String, body: (String) -> Void) and any CJK-specific test methods out of GhosttyMouseFocusTests and into a new CJKFontFallbackTests (or into the existing GhosttyConfigTests) class to restore correct semantics and organization, updating any references to withTempConfig accordingly and ensuring the new test class has the same imports and test target visibility.Sources/GhosttyTerminalView.swift (1)
1138-1146: Prefer exact key parsing over prefix checks in config scanning.Using
hasPrefix("font-codepoint-map")/hasPrefix("config-file")can match unintended keys and produce false positives. Parsing the key token before=and matching equality is safer.Suggested refactor
- for line in contents.components(separatedBy: .newlines) { - let trimmed = line.trimmingCharacters(in: .whitespaces) - if trimmed.hasPrefix("#") { continue } - if trimmed.hasPrefix("font-codepoint-map") { + for line in contents.components(separatedBy: .newlines) { + let trimmed = line.trimmingCharacters(in: .whitespaces) + if trimmed.isEmpty || trimmed.hasPrefix("#") { continue } + let parts = trimmed.split(separator: "=", maxSplits: 1) + let key = parts.first?.trimmingCharacters(in: .whitespaces) ?? "" + + if key == "font-codepoint-map" { return true } - if trimmed.hasPrefix("config-file") { - let parts = trimmed.split(separator: "=", maxSplits: 1) - if parts.count == 2 { + if key == "config-file", parts.count == 2 { var includePath = parts[1] .trimmingCharacters(in: .whitespaces) .trimmingCharacters(in: CharacterSet(charactersIn: "\"")) @@ - } } }Also applies to: 1141-1145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1138 - 1146, The loop that scans lines using trimmed.hasPrefix("font-codepoint-map") and trimmed.hasPrefix("config-file") can false-match; instead split each non-comment line on the first "=" (as already begun with trimmed.split(separator: "=", maxSplits: 1)), take the left-hand token (key) trimming whitespace, and compare key == "font-codepoint-map" or key == "config-file" for exact equality; update the checks around the for loop that uses contents.components(separatedBy: .newlines), the trimmed variable, and the parts handling so the code returns true only on exact key matches and then proceeds to handle the right-hand side as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1342-1357: The CJK font fallback helper withTempConfig and its
related tests were added inside the GhosttyMouseFocusTests class but belong in a
config-related test suite; move the private func withTempConfig(_ contents:
String, body: (String) -> Void) and any CJK-specific test methods out of
GhosttyMouseFocusTests and into a new CJKFontFallbackTests (or into the existing
GhosttyConfigTests) class to restore correct semantics and organization,
updating any references to withTempConfig accordingly and ensuring the new test
class has the same imports and test target visibility.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1138-1146: The loop that scans lines using
trimmed.hasPrefix("font-codepoint-map") and trimmed.hasPrefix("config-file") can
false-match; instead split each non-comment line on the first "=" (as already
begun with trimmed.split(separator: "=", maxSplits: 1)), take the left-hand
token (key) trimming whitespace, and compare key == "font-codepoint-map" or key
== "config-file" for exact equality; update the checks around the for loop that
uses contents.components(separatedBy: .newlines), the trimmed variable, and the
parts handling so the code returns true only on exact key matches and then
proceeds to handle the right-hand side as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8b8a419-5a2c-4508-8984-dfa44ae708bf
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0ba82f8be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Include ~/Library/Application Support/<bundle-id>/config(.ghostty) paths in the codepoint-map detection scan. This ensures that font-codepoint-map entries in the release app-support config (loaded by loadReleaseAppSupportGhosttyConfigIfNeeded for debug builds) are detected before injecting CJK font fallback defaults.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
1068-1097: Consider deduplicating emitted mappings for repeated language variants.If
Locale.preferredLanguagescontains repeated variants (for exampleja-JP,ja), the same range/font tuples can be appended multiple times. This is harmless but avoidable churn during config injection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1068 - 1097, The loop over Locale.preferredLanguages can append duplicate (range,font) tuples when variants repeat; modify GhosttyTerminalView's logic (inside the for lang in preferredLanguages block) to deduplicate before appending to mappings by tracking seen pairs (e.g. a Set of "range|font" or a Set of tuples) and only append if not already present, while preserving the existing coveredShared and sharedCJKRanges handling; ensure checks use the same identifiers (mappings, coveredShared, sharedCJKRanges, langRanges, font) so repeated language entries like "ja-JP" and "ja" do not produce duplicate mappings.
🤖 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 1170-1176: The includePath trimming logic in
GhosttyTerminalView.swift (the includePath variable handling) removes
surrounding quotes before stripping a trailing '?' which causes inputs like
`"config-file = \"foo\"?"` to become malformed (foo"); fix by handling the
optional '?' before or together with quote trimming: detect and remove a
trailing '?' from the raw parts[1] first (or strip both trailing characters in
one step), then remove surrounding quotes and whitespace so the resulting
includePath is the clean filename; update the block that currently trims quotes
then drops the last character to instead either drop '?' first or perform a
combined trim of both '"' and '?' (reference includePath and parts[1] in this
function).
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1068-1097: The loop over Locale.preferredLanguages can append
duplicate (range,font) tuples when variants repeat; modify GhosttyTerminalView's
logic (inside the for lang in preferredLanguages block) to deduplicate before
appending to mappings by tracking seen pairs (e.g. a Set of "range|font" or a
Set of tuples) and only append if not already present, while preserving the
existing coveredShared and sharedCJKRanges handling; ensure checks use the same
identifiers (mappings, coveredShared, sharedCJKRanges, langRanges, font) so
repeated language entries like "ja-JP" and "ja" do not produce duplicate
mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6bb404c-9f33-4a1c-96ac-d4cab991581e
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
Reorder the trimming so that the optional include marker '?' is removed before surrounding quotes are stripped. This prevents quoted paths like "foo"? from being misparsed as foo".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
1164-1182: Prefer exact key parsing overhasPrefixin config scanning.Line 1164 and Line 1167 use prefix checks, which can match unintended keys and cause false positives/incorrect include traversal.
♻️ Suggested hardening
- if trimmed.hasPrefix("font-codepoint-map") { + let parts = trimmed.split(separator: "=", maxSplits: 1) + let key = parts.first?.trimmingCharacters(in: .whitespaces) ?? "" + if key == "font-codepoint-map" { return true } - if trimmed.hasPrefix("config-file") { - let parts = trimmed.split(separator: "=", maxSplits: 1) - if parts.count == 2 { + if key == "config-file", parts.count == 2 { var includePath = parts[1] .trimmingCharacters(in: .whitespaces) // Ghostty supports optional includes with a trailing '?' if includePath.hasSuffix("?") { includePath.removeLast() } includePath = includePath .trimmingCharacters(in: CharacterSet(charactersIn: "\"")) let expanded = NSString(string: includePath).expandingTildeInPath let absolute = (expanded as NSString).isAbsolutePath ? expanded : (parentDir as NSString).appendingPathComponent(expanded) if configFileContainsCodepointMap(atPath: absolute, visited: &visited) { return true } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1164 - 1182, The code uses trimmed.hasPrefix("font-codepoint-map") and trimmed.hasPrefix("config-file"), which can false-positive on keys like "font-codepoint-map-extra" or "config-file-other"; instead parse the line into a key and value (e.g., split once on "=" or whitespace) and compare the key exactly to "font-codepoint-map" and "config-file" before handling includes; update the block around configFileContainsCodepointMap(atPath:visited:) to extract the key, trim quotes/whitespace from the value, handle the optional trailing "?" exactly, and then compute expanded/absolute paths using parentDir and pass visited through as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1164-1182: The code uses trimmed.hasPrefix("font-codepoint-map")
and trimmed.hasPrefix("config-file"), which can false-positive on keys like
"font-codepoint-map-extra" or "config-file-other"; instead parse the line into a
key and value (e.g., split once on "=" or whitespace) and compare the key
exactly to "font-codepoint-map" and "config-file" before handling includes;
update the block around configFileContainsCodepointMap(atPath:visited:) to
extract the key, trim quotes/whitespace from the value, handle the optional
trailing "?" exactly, and then compute expanded/absolute paths using parentDir
and pass visited through as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b3f8691-70b7-426c-95bb-ae377a92ab41
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
Contributor
Author
|
Hi @lawrencecchen! 👋 This PR adds automatic CJK font fallback to prevent macOS Core Text from picking decorative fonts (like LingWai) for CJK characters. All bot review feedback has been addressed and tests are passing. Would love your feedback when you get a chance. Happy to adjust anything! |
…havior The fallback issue is caused by CTFontCollection scoring prioritizing monospace fonts, not just CTFontCreateForString. The selected decorative font varies by environment (e.g. AB_appare from Adobe CC, or LingWai).
Contributor
|
Thanks again for all these contributions! |
This was referenced Mar 13, 2026
seohyeon1578
pushed a commit
to seohyeon1578/cmux
that referenced
this pull request
Mar 18, 2026
The automatic CJK font-codepoint-map injection (PR manaflow-ai#1017) maps Korean ranges to Apple SD Gothic Neo, which has a different style/weight from the primary terminal font. This overrides Ghostty's native CTFontCreateForString fallback, which dynamically selects a better-matching font for Hangul. Ghostty itself (ghostty-org/ghostty) has no hardcoded CJK font names and relies entirely on CTFontCreateForString for fallback. For Korean, this native fallback produces visually consistent results with the primary font. Skip Korean ranges from auto-injection so Ghostty's native fallback handles Hangul rendering. Japanese and Chinese mappings are unaffected.
This was referenced Apr 19, 2026
israeligal
pushed a commit
to israeligal/cmux
that referenced
this pull request
May 2, 2026
…-ai#1700) * fix: skip Korean from CJK font-codepoint-map auto-injection The automatic CJK font-codepoint-map injection (PR manaflow-ai#1017) maps Korean ranges to Apple SD Gothic Neo, which has a different style/weight from the primary terminal font. This overrides Ghostty's native CTFontCreateForString fallback, which dynamically selects a better-matching font for Hangul. Ghostty itself (ghostty-org/ghostty) has no hardcoded CJK font names and relies entirely on CTFontCreateForString for fallback. For Korean, this native fallback produces visually consistent results with the primary font. Remove the Korean branch from cjkFontMappings() so Ghostty's native fallback handles Hangul rendering. Japanese and Chinese mappings are unaffected. * test: update CJK font mapping tests for Korean removal - testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean → renamed to testCJKFontMappingsReturnsNilForKoreanOnly → asserts nil since Korean is no longer auto-mapped - testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges → renamed to testCJKFontMappingsMultiLanguageSkipsKorean → asserts no Apple SD Gothic Neo mapping exists → Japanese mappings remain unchanged --------- Co-authored-by: dante-ad-shield <danate@ad-shield.io> (cherry picked from commit 1c4c1bb)
3 tasks done
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Jul 31, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Jul 31, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 11, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 13, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 14, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 15, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 22, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 22, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 22, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 23, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 25, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 27, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 28, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 29, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Aug 30, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Sep 1, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
…-ai#1700) * fix: skip Korean from CJK font-codepoint-map auto-injection The automatic CJK font-codepoint-map injection (PR manaflow-ai#1017) maps Korean ranges to Apple SD Gothic Neo, which has a different style/weight from the primary terminal font. This overrides Ghostty's native CTFontCreateForString fallback, which dynamically selects a better-matching font for Hangul. Ghostty itself (ghostty-org/ghostty) has no hardcoded CJK font names and relies entirely on CTFontCreateForString for fallback. For Korean, this native fallback produces visually consistent results with the primary font. Remove the Korean branch from cjkFontMappings() so Ghostty's native fallback handles Hangul rendering. Japanese and Chinese mappings are unaffected. * test: update CJK font mapping tests for Korean removal - testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean → renamed to testCJKFontMappingsReturnsNilForKoreanOnly → asserts nil since Korean is no longer auto-mapped - testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges → renamed to testCJKFontMappingsMultiLanguageSkipsKorean → asserts no Apple SD Gothic Neo mapping exists → Japanese mappings remain unchanged --------- Co-authored-by: dante-ad-shield <danate@ad-shield.io> (cherry picked from commit cdec978)
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Sep 6, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Sep 8, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Sep 11, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado
added a commit
to ammachado/cmux
that referenced
this pull request
Sep 17, 2026
Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from manaflow-ai#1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Oct 2, 2026
…, ▰/▱ gauges) (#9193) * Inject a symbol-glyph font fallback like the existing CJK one Ghostty's CTFontCollection scoring in discoverFallback() is monospace-biased, so a codepoint outside the configured font can land on whatever installed "monospace" font happens to also claim coverage instead of the narrower substitute CoreText's own CTFontCreateForString cascade would pick (see manaflow-ai/ghostty's discovery.zig, and the upstream analysis at Nanako0129/coralline#47, which traced this exact scoring bug through the Geometric Shapes and Miscellaneous Symbols and Arrows ranges: the hexagon glyph U+2B21/U+2B22 and gauge characters like ▰/▱ used by status-line tools). cmux already works around this class of bug for CJK ranges (loadCJKFontFallbackIfNeeded, from #1017). This extends the same font-codepoint-map injection mechanism to the symbol ranges implicated by the coralline report, pointing them at Apple Symbols (the font CoreText's own cascade resolves to), so Ghostty's fallback choice is predictable instead of landing on an arbitrary wide "monospace" font. Like the CJK path, this only injects when the user hasn't already configured font-codepoint-map, an explicit multi-font fallback chain, or a primary font that already covers the range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fail closed on unprobeable configured fonts in fallback injection When fontProbe.configuredFont(...) returns nil (family name doesn't resolve), both autoInjectedSymbolFontMappings and the pre-existing autoInjectedCJKFontMappings previously fell through and returned every mapping unfiltered, forcing the injected font (Apple Symbols / a CJK font) over ranges whose actual coverage is unknown. That's not "keeping Ghostty's behavior" for the uncertain case, it's overriding it on a guess. Return nil instead so cmux injects nothing and Ghostty's own discoverFallback() runs unmodified when we can't tell whether an override is actually needed. Addresses CodeRabbit review comment on #9193: #9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Sample the actual gauge glyphs and use #require in the symbol test Address two follow-up CodeRabbit findings on #9193: - symbolCoverageSampleCharactersByRange only sampled U+25A0/U+25CF for the Geometric Shapes range, so a font could pass the coverage check while still missing U+25B0/U+25B1/U+25CB (the coralline bar glyphs ▰/▱/○ this PR exists to fix) and silently skip the Apple Symbols fallback for exactly the characters that need it. - autoInjectedSymbolFontMappingsFiltersRangesCoveredByConfiguredFont used #expect(mappings != nil) followed by a force-unwrap, which traps instead of failing the assertion cleanly if it regresses. Switched to try #require(...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Check symbol font fallback coverage per codepoint, not per block Fonts (Nerd Fonts especially) often patch in some but not all glyphs in a Unicode block. The previous all-or-nothing per-range coverage check meant a single missing glyph (e.g. U+25B0 in JetBrainsMono Nerd Font) forced the whole Geometric Shapes block onto Apple Symbols, clobbering glyphs the configured font already rendered correctly. Verified against Ghostty's discovery.zig/CodepointResolver.zig and live CoreText coverage checks; see PR discussion for details. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix surrogate-pair handling and dedupe config scan in symbol fallback - fontContainsGlyph mishandled codepoints above U+FFFF (single UniChar truncation), which would silently force-override any future non-BMP symbol codepoint regardless of actual font coverage. Encode a proper UTF-16 surrogate pair instead, with a regression test. - loadCJKFontFallbackIfNeeded and loadSymbolFontFallbackIfNeeded each independently re-scanned and re-parsed the same config files on every config load. Resolve the scan paths once and share them. - Flatten symbolCodepointsByRange to symbolCodepoints: the range keys were never read (coverage is per-codepoint, not per-range) and implied broader coverage than the hardcoded list actually provides. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address CodeRabbit follow-up: literal test expectations, guard system font - symbolFontMappingsCoverAllSymbolCodepoints now asserts against a literal expected codepoint set (plus a count check) instead of deriving it from the production symbolCodepoints list, so the test can catch a wrong or incomplete production list. - fontContainsGlyphHandlesSupplementaryPlaneCodepoints now guards that "Apple Color Emoji" actually resolved before asserting on its glyph coverage, since it's a system font rather than a repo-controlled fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add failing test: default face coverage ignored with no font-family With no `font-family` configured, `autoInjectedSymbolFontMappings` returns every managed codepoint without probing anything. Ghostty's primary face in that case is its embedded JetBrains Mono, and a `font-codepoint-map` entry outranks that face, so cmux pushes U+25A0/U+25CB/U+25CF onto Apple Symbols even though the default face renders them. This commit adds the test only, so CI shows it red before the fix lands. Reported by Cursor Bugbot on #9193. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Filter default-face coverage when no font-family is configured Only inject Apple Symbols for the managed codepoints Ghostty's built-in primary face actually lacks (▰ ▱ ⬡ ⬢), instead of all seven. The default face is embedded in Ghostty's binary rather than installed system-wide, so it cannot be resolved by family name through GhosttyFontProbing; its coverage is recorded in `defaultFaceCoveredSymbolCodepoints`. `defaultFaceCoveredSymbolCodepointsMatchGhosttysEmbeddedFont` probes the vendored ghostty/src/font/res/JetBrainsMonoNoNF-Regular.ttf via CoreText, so the table fails the build if the submodule moves to a default face with different coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: handle AgentHookFailureStage.journalAppend in failure reporting origin/main's journal-events commit (26614b0) added the .journalAppend case to AgentHookFailureStage but never updated this switch, leaving it non-exhaustive and the whole target unbuildable. Unrelated to shell completion; fixed here because it blocked rebasing this branch onto origin/main. * Address cubic review: drop duplicate string key, harden test, share loader - Resources/Localizable.xcstrings: the branch added a second `cli.agentHook.error.journalAppend` entry, but origin/main already carries that key. An .xcstrings catalog is a dictionary, so the two blocks collapse to one on parse and the added translations were dead weight. Restore the file to main's version; the CLI switch case that consumes the key is already on main, so nothing here was needed. - GhosttyConfigDiscoveryTests: fontContainsGlyphHandlesSupplementaryPlane Codepoints guarded on the resolved family name and returned early, so it passed as a silent no-op wherever CoreText substituted another font for "Apple Color Emoji". Require the exact family instead, so a substitution fails loudly rather than faking coverage validation. - GhosttyTerminalView: loadSymbolFontFallbackIfNeeded was a near-verbatim copy of loadCJKFontFallbackIfNeeded. Extract the shared directive builder into loadInjectedFontCodepointMap so the two paths can't drift apart when the injection format changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: skip optional Ghostty font drift check without submodule Co-authored-by: Adriano Machado <60320+ammachado@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Leo Li <cheerleaderleo@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CTFontCreateForStringhandles CJK Unified Ideographs (U+4E00-9FFF) and typically selects Hiragino Sans correctly. However,CTFontCollectionwith scoring handles Hiragana, Katakana, and symbols — and may select an unintended decorative font.font-codepoint-mapinjection based on the system's preferred language to bypass the unreliable fallback.font-codepoint-mapis already configured by the user.Details
When the primary font (e.g. Menlo) lacks CJK glyphs, libghostty uses two fallback mechanisms.
CTFontCreateForStringhandles CJK Unified Ideographs (U+4E00-9FFF) and usually selects Hiragino Sans W3 correctly. For Hiragana, Katakana, and CJK symbols,CTFontCollectionscoring is used instead — and the result depends on which fonts are installed on the system.The scoring prioritizes monospace fonts, which causes decorative fonts with monospace attributes to be selected unexpectedly. For example, AB_appare (synced via Adobe Creative Cloud) or LingWai may be chosen over appropriate system fonts like Hiragino Sans.
This fix injects
font-codepoint-mapdefaults based onLocale.preferredLanguages:jakozh-Hant/zh-TW/zh-HKzhCovered Unicode ranges:
If the user has any
font-codepoint-mapin their Ghostty config, the automatic fallback is skipped.Before
Hiragana and Katakana render in a decorative font (varies by environment — AB_appare, LingWai, etc.).
After
Japanese text renders in Hiragino Sans, the standard macOS system font.
Test plan
preferredCJKFontFamily(ja/ko/zh-Hant/zh-Hans/non-CJK)userConfigContainsCJKCodepointMap(present/absent/commented/missing)