Extend Ghostty CJK font-fallback injection to symbol ranges (⬡ U+2B21, ▰/▱ gauges) - #9193
teamleaderleo merged 13 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
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:
📝 WalkthroughWalkthroughChangesGhostty now discovers symbol fallback mappings, checks configured glyph coverage, and injects missing Symbol font fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GhosttyApp
participant GhosttyConfigDiscovery
participant CoreText
participant GhosttyConfig
GhosttyApp->>GhosttyConfigDiscovery: resolve symbol fallback mappings
GhosttyConfigDiscovery->>CoreText: check configured font glyph coverage
CoreText-->>GhosttyConfigDiscovery: return covered codepoints
GhosttyConfigDiscovery-->>GhosttyApp: return uncovered mappings
GhosttyApp->>GhosttyConfig: load font-codepoint-map directives
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift`:
- Around line 113-153: Update autoInjectedSymbolFontMappings so a configured
font family with no rangeCoverageProbe returns nil when
fontProbe.configuredFont(named:size:) cannot produce a font, rather than
retaining mappings and injecting Apple Symbols; preserve coverage-based removal
when probing succeeds. Add a regression test using a configured font-family and
NoFontProbe that verifies no mappings are returned.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae64ea65-18e0-45d4-910b-ba94b6112a8b
📒 Files selected for processing (3)
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swiftSources/GhosttyTerminalView.swift
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift (1)
99-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProbe the symbol characters this feature is meant to protect.
The geometric-shapes range samples
■and●, but the PR also targets▰,▱, and○. A font can cover the sampled characters while missing those glyphs; Line 142 would then remove the entire range and skip the intendedApple Symbolsfallback. IncludeU+25B0,U+25B1, andU+25CB, or probe the actual managed glyph set.As per path instructions, fallback injection decisions must use a reliable coverage signal for the characters being managed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift` around lines 99 - 101, The symbol coverage sampling in symbolCoverageSampleCharactersByRange does not probe all managed geometric-shape glyphs, allowing missing glyphs to bypass the Apple Symbols fallback. Add probes for U+25B0, U+25B1, and U+25CB, or otherwise use the complete managed glyph set so fallback decisions reflect coverage of every character the feature protects.Source: Path instructions
Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift (1)
112-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
#requirebefore force-unwrappingmappings.
#expect(mappings != nil)is non-fatal; if this call returnsnil,mappings!traps and masks the assertion failure. Bind the value withtry#require(discovery.autoInjectedSymbolFontMappings(...))and usemappingsdirectly for the range assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift` around lines 112 - 113, Replace the non-fatal nil assertion and force unwrap in the mapping test with a throwing `#require` binding around discovery.autoInjectedSymbolFontMappings(...). Use the resulting mappings value directly in the Set range assertion, preserving the existing expected range.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift`:
- Around line 99-101: The symbol coverage sampling in
symbolCoverageSampleCharactersByRange does not probe all managed geometric-shape
glyphs, allowing missing glyphs to bypass the Apple Symbols fallback. Add probes
for U+25B0, U+25B1, and U+25CB, or otherwise use the complete managed glyph set
so fallback decisions reflect coverage of every character the feature protects.
In
`@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift`:
- Around line 112-113: Replace the non-fatal nil assertion and force unwrap in
the mapping test with a throwing `#require` binding around
discovery.autoInjectedSymbolFontMappings(...). Use the resulting mappings value
directly in the Set range assertion, preserving the existing expected range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d42cabd-d100-4716-a02b-401bcb210f77
📒 Files selected for processing (2)
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift
Address two follow-up CodeRabbit findings on manaflow-ai#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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift (1)
90-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the fallback-chain test independent of
Apple Symbols.The requirement is to skip injection for any explicit multi-entry
font-familychain, but this fixture includesApple Symbols. A regression that special-cases that font could pass without proving the generic chain gate. Use two unrelated font families here; keep a separateApple Symbolscase only if that behavior is independently required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift` around lines 90 - 97, Update autoInjectedSymbolFontMappingsSkipsWhenExplicitFallbackChainPresent to use two unrelated font-family values instead of including Apple Symbols, ensuring the test verifies generic multi-entry fallback-chain detection rather than a font-specific special case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift`:
- Around line 90-97: Update
autoInjectedSymbolFontMappingsSkipsWhenExplicitFallbackChainPresent to use two
unrelated font-family values instead of including Apple Symbols, ensuring the
test verifies generic multi-entry fallback-chain detection rather than a
font-specific special case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99d60baf-afd1-4cf1-b40b-3f9803ca1ed6
📒 Files selected for processing (2)
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift
Follow-up: fact-checking this fix against Ghostty's actual fallback behaviorI went back and verified the root cause and the mitigation against Ghostty's source ( The root-cause mechanism is real, and it's inherited from Ghostty itself
This is unmodified in cmux's fork (only Windows But the mitigation's per-range coverage check is too coarse, and can make Nerd Font users' rendering worse
I verified live on a machine with several Nerd Fonts installed (a very plausible stand-in for the coralline reporter, given the issue mentions ▰/▱ gauge glyphs, which are a Nerd/Powerline-adjacent idiom): JetBrainsMono Nerd Font has U+25A0/U+25CB/U+25CF in its own cmap (Nerd Fonts patch these in deliberately, monospace-width, visually matched to the rest of the font) but is missing U+25B0/U+25B1. Because the coverage check requires all five sample chars ( The "matches a plain CoreText app" justification doesn't hold in this case eitherThe doc comment on Plain CoreText only cascades to Apple Symbols for the codepoints the base font is actually missing (here, the hexagon range). For codepoints the font already has, CoreText keeps using the font's own glyph — it doesn't uniformly redirect the whole block to Apple Symbols. So "matches a plain CoreText app" is only true for the hexagon/arrows range in practice, not for the Geometric Shapes range when a font has partial coverage. Net effectFor Nerd-Font-using terminal power users — arguably the exact audience most likely to hit gauge/powerline-style glyph bugs like the one in the coralline issue — this PR as currently written can regress rendering for glyphs their font already handles correctly, in the process of fixing the ones it doesn't. Planned fixTighten the coverage check (and the resulting override) to be per-codepoint rather than per-range, so only the specific missing scalars get remapped, and correct the doc comment to reflect that the CoreText-cascade parity claim only holds for genuinely-missing codepoints. Pushing that now. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift (1)
74-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the concrete symbol contract, not the production list itself.
The expected set is derived from
GhosttyConfigDiscovery.symbolCodepoints, so this test passes even if that production list is incomplete or contains the wrong codepoints. TheSetconversion also hides duplicate mappings. Use a literal expected set of requiredU+...values and assertmappings.count == expected.count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift` around lines 74 - 81, Update symbolFontMappingsCoverAllSymbolCodepoints to use a literal set of the required U+... codepoint strings instead of deriving expected from GhosttyConfigDiscovery.symbolCodepoints. Also assert mappings.count equals expected.count before comparing sets, while preserving the fallback-font assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift`:
- Around line 135-145: Replace the system-dependent “Apple Color Emoji” font in
fontContainsGlyphHandlesSupplementaryPlaneCodepoints with a
repository-controlled font fixture whose glyph coverage for 0x1F600 and 0x10FFFE
is validated. If the fixture cannot be resolved or validated, skip the test
rather than asserting against platform-dependent CoreText behavior.
---
Outside diff comments:
In
`@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swift`:
- Around line 74-81: Update symbolFontMappingsCoverAllSymbolCodepoints to use a
literal set of the required U+... codepoint strings instead of deriving expected
from GhosttyConfigDiscovery.symbolCodepoints. Also assert mappings.count equals
expected.count before comparing sets, while preserving the fallback-font
assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6f53e99-d0bb-4308-996f-2a44ba9ad52a
📒 Files selected for processing (3)
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swiftSources/GhosttyTerminalView.swift
55cbe8a to
439dc8e
Compare
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
439dc8e to
0602eb7
Compare
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
0602eb7 to
4b08145
Compare
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4b08145. Configure here.
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 manaflow-ai#9193. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7c77e24 to
ccbf7e5
Compare
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
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 manaflow-ai#9193. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
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 manaflow-ai#9193. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
58d9097 to
6848d64
Compare
|
All contributors have signed the CLA ✍️ ✅ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
I have read the CLA Document and I hereby sign the CLA |
a5b2ef0 to
f7e9bc7
Compare
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
|
recheck |
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>
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 manaflow-ai#9193: manaflow-ai#9193 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address two follow-up CodeRabbit findings on manaflow-ai#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>
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>
- 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>
… 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>
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 manaflow-ai#9193. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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.
…oader - 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>
Co-authored-by: Adriano Machado <60320+ammachado@users.noreply.github.com>
|
Review: symbol mappings and per-codepoint coverage look correct; the submodule-free package-test blocker was verified. Fixed: the optional Ghostty font drift check now skips cleanly when the fixture is unavailable (5f15a9c). Left: macOS tests were not runnable on this Linux lane; CI will validate them. |
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
|
Taking this: checking symbol font fallback against current main, preserving explicit font choices, and refreshing CI.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge receipt for
Labeled |
2 participants
Summary
CTFontCollectionscoring indiscoverFallback()(src/font/discovery.zig) is monospace-biased: when a codepoint isn't in the configured font, it can pick whatever installed "monospace" font also claims coverage instead of the narrower substitute CoreText's ownCTFontCreateForStringcascade would choose.loadCJKFontFallbackIfNeeded(fix: add CJK font fallback to prevent decorative font rendering #1017 /GhosttyConfigDiscovery.autoInjectedCJKFontMappings), but that injection is scoped to CJK-only ranges.Nanako0129/coralline#47traced this exact scoring bug through the Geometric Shapes (▰/▱/●/○) and Miscellaneous Symbols and Arrows (⬡U+2B21,⬢U+2B22) ranges, used by status-line tools rendering gauges/icons inside the terminal — and confirmed it reproduces specifically inside cmux (which vends its own Ghostty fork with the identicaldiscovery.zigfallback logic for the CoreText path).font-codepoint-mapauto-injection mechanism to those symbol ranges, pointing them atApple Symbols(the font CoreText's own cascade resolves to for these ranges), so the fallback choice is predictable instead of landing on an arbitrary, potentially much wider "monospace" font.font-codepoint-map, an explicit multi-fontfont-familyfallback chain, or a primary font that already covers the range — so it never overrides user-managed fallback.Changes
GhosttyConfigDiscovery.swift: addssymbolRanges,symbolFallbackFont,symbolCoverageSampleCharactersByRange,symbolFontMappings(),autoInjectedSymbolFontMappings(),shouldInjectSymbolFontFallback().GhosttyTerminalView.swift: wiresloadSymbolFontFallbackIfNeededintoloadDefaultConfigFilesWithLegacyFallbackright after the existing CJK loader, plus thin static forwarders mirroring the CJK ones.GhosttyConfigDiscoveryTests.swift: newGhosttyConfigDiscoverySymbolTestssuite covering mapping generation and override precedence.Test plan
swift test --filter GhosttyConfigDiscoveryinPackages/macOS/CmuxTerminalCore— 23/23 pass, including the 6 new symbol-fallback tests.cmuxapp-target build (xcodebuild -scheme cmux, via./scripts/reload.sh) — passes. The earlierzig 0.15.2gap was environment-specific and resolved with./scripts/setup.sh(zig 0.16.0, cachedGhosttyKit.xcframework). Build then hit an unrelated pre-existingmainbug (CLI/CMUXCLI+AgentHookFailureReporting.swifthad a non-exhaustive switch missingAgentHookFailureStage.journalAppend, from an unrelated upstream commit); fixed via a cherry-picked commit (fix: handle AgentHookFailureStage.journalAppend in failure reporting) so this branch builds clean end to end.XDG_CONFIG_HOMEpointing atfont-family = JetBrainsMono Nerd Font(no CJK language, no explicitfont-familyfallback chain, nofont-codepoint-map). Printed⬡ ⬢ ▰ ▱ ● ○inside the terminal — all render as compact, near-1-cell-width, predictable glyphs, not an arbitrary wide substitute.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Extends Ghostty's font-fallback auto-injection to symbol codepoints (⬡ U+2B21, ▰/▱ gauges) so status-line glyphs resolve to
Apple Symbolsinstead of an arbitrary wide monospace font. Injection matches CoreText's cascade and only covers codepoints the configured font — or the default embedded JetBrains Mono face, when nofont-familyis set — doesn't already render.font-codepoint-mapor a multi-entryfont-familychain, and fails closed when the configured font can't be probed (for both symbol and CJK injection).font-codepoint-mapdirective builder between the CJK and symbol loaders so the two paths can't drift apart; coverage probing handles non-BMP codepoints via surrogate pairs.Written for commit 4049bca. Summary will update on new commits.
Summary by CodeRabbit
New Features
font-codepoint-mapdirectives when needed.Bug Fixes
font-codepoint-mapor explicit multi-stepfont-familyfallback chains.Tests
Note
Low Risk
Startup-only Ghostty config injection for font fallbacks; behavior is gated on user config and glyph coverage, with no auth or data-path changes.
Overview
Adds symbol-glyph
font-codepoint-mapauto-injection alongside the existing CJK workaround so glyphs like ⬡ (U+2B21) and ▰/▱ gauges map to Apple Symbols instead of an arbitrary wide monospace fallback from Ghostty’s CoreText scoring.Injection is per codepoint (with coverage probing via new
fontContainsGlyph, including surrogate pairs), skips userfont-codepoint-map/ multi-font-familychains, and when there is nofont-familyonly overrides codepoints the embedded default JetBrains Mono lacks. CJK auto-injection now fails closed when the configured font family cannot be resolved, matching the symbol path.GhosttyTerminalViewresolves config scan paths once, runs the new symbol loader after CJK, and sharesloadInjectedFontCodepointMapfor both. Tests cover override rules, partial Nerd Font coverage, default-face filtering, and vendored-font drift fordefaultFaceCoveredSymbolCodepoints.Reviewed by Cursor Bugbot for commit 79da9c0. Bugbot is set up for automated code reviews on this repo. Configure here.