Repository navigation
Extract Ghostty config-file discovery from GhosttyTerminalView into CmuxGhosttyConfigLoader - #6177
azooz2003-bit wants to merge 1 commit into
Conversation
…muxGhosttyConfigLoader Pull the pure config-file discovery + scanning domain out of GhosttyApp (in the 12k-line GhosttyTerminalView.swift god file) into a new leaf package, CmuxGhosttyConfigLoader, as a constructor-injected value type GhosttyConfigDiscovery. What moved (now a real type with inverted seams, not statics on GhosttyApp): - CJK font-fallback mapping (cjkFontMappings, autoInjectedCJKFontMappings, shouldInjectCJKFontFallback, resolvedInjectedCJKFontName, font coverage) - user font-config scanning (userFontConfigSummary, config-file include following, parsedConfigEntry/applyConfigFileDirective, UserFontConfigSummary) - scan-path resolution (loadedGhosttyConfigScanPaths/loadedCJKScanPaths, cmuxAppSupportConfigURLs) - legacy-config decisions (shouldLoadLegacyGhosttyConfig, shouldIncludeLegacyGhosttyConfigInScanPaths, shouldIgnoreNativeLegacyBaselineForUnparsedAppearance) - conditional-theme override + managed-default-appearance decisions Seams (inverted, constructor-injected, defaults reproduce prior behavior): - GhosttyConfigFileReading (default FileManagerGhosttyConfigFileReader) for every config file size/contents read that used FileManager.default / String(contentsOfFile:) - GhosttyFontProbing (default CoreTextGhosttyFontProbe) for the CoreText CTFont family-name discovery and glyph-coverage probing - CmuxGhosttyConfigPathResolver injected for app-support URL resolution Byte-identical: every directive key, scan-path ordering, legacy-fallback size rule, config-file include semantics, CJK range set, and theme-override text is preserved exactly. The C-API config-load instance methods (loadDefaultConfigFilesWithLegacyFallback, loadCJKFontFallbackIfNeeded, reloadConfiguration, synchronizeThemeWithAppearance) stay in the app target and now call the new type to decide *what* to load, performing the actual ghostty_config_load_* calls themselves. The former GhosttyApp statics remain as thin one-line forwards so all call sites (incl. cmuxTests) are unchanged. Left in the app target on purpose: everything that mutates ghostty_config_t, GhosttyApp instance state (config/app/lastAppearanceColorScheme/default background scope), or reaches AppDelegate.shared / NotificationCenter — those are not pure and not part of this clean subset. Seam is synchronous (value type, not actor) because discovery runs inside the synchronous launch-path config-load sequence; making it async would change the ordering of the C-API calls. Principled. Adds behavior unit tests (17) with in-memory fakes for both seams. Wires the package into cmux.xcodeproj (6 pbxproj entries) + app target dep. Giant shrinks 12093 -> 11691 lines; budget reconciled. NOTE: full app build is validated by GitHub CI (local app xcodebuild skipped to avoid melting the machine across parallel refactor agents). New package builds + tests pass locally; package-conventions lint is clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 2 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ 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 |
Greptile SummaryThis PR extracts the pure Ghostty config-file discovery and CJK font-fallback logic from the
Confidence Score: 4/5The extraction is byte-identical to the original logic, all call sites are unchanged, and 17 new unit tests cover the core decision paths. The three findings are non-blocking quality notes. The refactor is well-scoped and the new package compiles under Swift 6 strict mode. The only gaps are:
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["GhosttyApp static methods\ne.g. cjkFontMappings\nautoInjectedCJKFontMappings\nconditionalThemeOverrideConfigContents"] -->|one-line forward| B["GhosttyConfigDiscovery\nprivate static let configDiscovery"]
B --> C{Decision logic}
C -->|scan-path resolution| D["CmuxGhosttyConfigPathResolver\nfrom CmuxFoundation"]
C -->|file size / contents reads| E["GhosttyConfigFileReading seam\ndefault: FileManagerGhosttyConfigFileReader"]
C -->|font family / glyph coverage| F["GhosttyFontProbing seam\ndefault: CoreTextGhosttyFontProbe"]
C -->|appearance + theme helpers| G["GhosttyConfig\nfrom CmuxTerminalCore"]
E -->|in tests| H["FakeFileReader\nin-memory map"]
F -->|in tests| I["NoFontProbe\nreturns nil"]
B -->|returns decisions| A
A -->|still issues ghostty_config_load_* calls| J["Ghostty C-API\nghotsty_config_t mutation stays in app target"]
Reviews (1): Last reviewed commit: "Extract Ghostty config-file discovery fr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public func resolvedInjectedCJKFontName( | ||
| named name: String, | ||
| size: CGFloat = 12 | ||
| ) -> String { | ||
| let trimmed = name.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmed.isEmpty else { return name } | ||
| guard let regularWeightFont = fontProbe.discoveredFont(named: trimmed, size: size, weightTrait: 0.0) else { | ||
| return trimmed | ||
| } | ||
|
|
||
| let candidateNames = [ | ||
| CTFontCopyName(regularWeightFont, kCTFontFullNameKey) as String?, | ||
| CTFontCopyName(regularWeightFont, kCTFontPostScriptNameKey) as String?, | ||
| ].compactMap { $0 } | ||
| let expectedFullName = CTFontCopyFullName(regularWeightFont) as String | ||
| let expectedPostScriptName = CTFontCopyPostScriptName(regularWeightFont) as String | ||
|
|
||
| for candidate in candidateNames { | ||
| guard let verifiedFont = fontProbe.discoveredFont(named: candidate, size: size, weightTrait: nil) else { continue } | ||
| let verifiedNames = [ | ||
| CTFontCopyName(verifiedFont, kCTFontFamilyNameKey) as String?, | ||
| CTFontCopyName(verifiedFont, kCTFontFullNameKey) as String?, | ||
| CTFontCopyName(verifiedFont, kCTFontPostScriptNameKey) as String?, | ||
| ].compactMap { $0 } | ||
| let matchesRegularWeightFace = verifiedNames.contains { | ||
| Self.normalizedFontName($0) == Self.normalizedFontName(expectedFullName) || | ||
| Self.normalizedFontName($0) == Self.normalizedFontName(expectedPostScriptName) | ||
| } | ||
| if matchesRegularWeightFace { | ||
| return candidate | ||
| } | ||
| } | ||
|
|
||
| return trimmed |
There was a problem hiding this comment.
resolvedInjectedCJKFontName partially bypasses the GhosttyFontProbing seam
After calling fontProbe.discoveredFont(...) to obtain regularWeightFont, the method calls CTFontCopyName, CTFontCopyFullName, CTFontCopyPostScriptName, and another CTFontCopyName set directly on the returned CTFont — outside the injected probe. The verification loop then calls fontProbe.discoveredFont again, but the subsequent name lookups (CTFontCopyName for family/full/PostScript) are also direct CoreText calls.
In practice the existing tests are unaffected because NoFontProbe.discoveredFont returns nil, which short-circuits the function at line 243 and avoids the direct calls entirely. However, a test that wants to verify the face-selection logic (e.g., confirming that "Hiragino Sans" resolves to the regular-weight face rather than W0) cannot do so without a real system font — the probe seam doesn't cover the downstream CTFontCopy* reads. Consider exposing a fontNameAttributes(for: CTFont) -> (full: String, postScript: String, family: String) requirement on GhosttyFontProbing so the entire resolution path is injectable and independently testable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @Suite struct GhosttyConfigDiscoveryThemeOverrideTests { | ||
| @Test func nonConditionalThemeNeedsNoOverride() { | ||
| let path = "/cfg/config" | ||
| let reader = FakeFileReader(contentsByPath: [path: "theme = Dracula"]) | ||
| let discovery = GhosttyConfigDiscovery(fileReader: reader, fontProbe: NoFontProbe()) | ||
| #expect(discovery.conditionalThemeOverrideConfigContents( | ||
| preferredColorScheme: .dark, | ||
| configPaths: [path] | ||
| ) == nil) | ||
| } | ||
|
|
||
| @Test func noThemeDirectiveYieldsNil() { | ||
| let path = "/cfg/config" | ||
| let reader = FakeFileReader(contentsByPath: [path: "font-family = Mono"]) | ||
| let discovery = GhosttyConfigDiscovery(fileReader: reader, fontProbe: NoFontProbe()) | ||
| #expect(discovery.conditionalThemeOverrideConfigContents( | ||
| preferredColorScheme: .dark, | ||
| configPaths: [path] | ||
| ) == nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
conditionalThemeOverrideConfigContents positive path is untested
Both existing tests in GhosttyConfigDiscoveryThemeOverrideTests only exercise nil-return scenarios — a plain (non-conditional) theme and a config with no theme directive at all. There is no test for the case where the function should return a non-nil override string, for example theme = light:Dracula,dark:Nord with a .dark preference returning "theme = Nord". This is the live bug path that the fix in issue #3459 targets, so a regression here would be invisible to the test suite.
| public protocol GhosttyConfigFileReading { | ||
| /// Returns the byte size of the file at `path`, or `nil` when it does not | ||
| /// exist or its attributes cannot be read. | ||
| func fileSize(atPath path: String) -> Int? | ||
|
|
||
| /// Returns the UTF-8 contents of the file at `path`, or `nil` when it cannot | ||
| /// be read. | ||
| func contents(atPath path: String) -> String? | ||
| } |
There was a problem hiding this comment.
GhosttyConfigFileReading (and GhosttyFontProbing) lack Sendable; GhosttyConfigDiscovery can't synthesize it
The doc comment correctly states that the seam is "intentionally not Sendable" because discovery only runs synchronously on the launch path. However, GhosttyConfigDiscovery stores both protocols as existentials (any GhosttyConfigFileReading, any GhosttyFontProbing), and a struct that holds non-Sendable stored properties cannot synthesize Sendable itself. The result is that private static let configDiscovery = GhosttyConfigDiscovery() in GhosttyApp is only safe today because GhosttyApp is @MainActor-isolated — the static is implicitly actor-bound, so Swift 6 doesn't require the type to be Sendable. If GhosttyApp's isolation is ever relaxed or the static is accessed from a nonisolated context in a future refactor, Swift 6 strict mode will surface a latent error. Documenting the coupling between "non-Sendable by design" and "caller must be @MainActor-isolated" at the static declaration site — or alternatively adding @unchecked Sendable to GhosttyConfigDiscovery with an explanatory comment — would make the invariant explicit and durable.
|
Superseded by #6227 — consolidated into the existing CmuxTerminalCore package (no new micro-package) per over-engineering review. The extraction is preserved there. |
This branch was successfully deployed
1 participant
Pulls the pure Ghostty config-file discovery + scanning domain out of
GhosttyApp(inside the ~12k-lineSources/GhosttyTerminalView.swiftgod file) into a new leaf packageCmuxGhosttyConfigLoader, as a constructor-injected value typeGhosttyConfigDiscovery.What moved
Now a real type with inverted seams instead of statics welded onto
GhosttyApp:cjkFontMappings,autoInjectedCJKFontMappings,shouldInjectCJKFontFallback,resolvedInjectedCJKFontName, glyph-coverage probing.userFontConfigSummary,config-fileinclude following,parsedConfigEntry/applyConfigFileDirective,UserFontConfigSummary.loadedGhosttyConfigScanPaths/loadedCJKScanPaths,cmuxAppSupportConfigURLs.shouldLoadLegacyGhosttyConfig,shouldIncludeLegacyGhosttyConfigInScanPaths,shouldIgnoreNativeLegacyBaselineForUnparsedAppearance.Seams (inverted, constructor-injected; defaults reproduce prior behavior)
GhosttyConfigFileReading(defaultFileManagerGhosttyConfigFileReader) inverts every config file size/contents read that usedFileManager.default/String(contentsOfFile:).GhosttyFontProbing(defaultCoreTextGhosttyFontProbe) inverts the CoreTextCTFontfamily-name discovery and glyph-coverage probing.CmuxGhosttyConfigPathResolver(fromCmuxFoundation) injected for app-support URL resolution.One major public type per file;
///DocC on every public symbol; no caseless-enum namespace, no free functions, nolint:allow.Byte-identical
Every directive key, scan-path ordering, legacy-fallback size rule,
config-fileinclude semantics, CJK range set, and theme-override text is preserved exactly. The C-API config-load instance methods (loadDefaultConfigFilesWithLegacyFallback,loadCJKFontFallbackIfNeeded,reloadConfiguration,synchronizeThemeWithAppearance) stay in the app target and now call the new type to decide what to load while still issuing theghostty_config_load_*calls themselves. The formerGhosttyAppstatics remain as thin one-line forwards, so all call sites (includingcmuxTests) are unchanged.Left in the app target on purpose: everything that mutates
ghostty_config_t,GhosttyAppinstance state, or reachesAppDelegate.shared/NotificationCenter— not pure, not part of this clean subset.The seam is a synchronous value type (not an actor) because discovery runs inside the synchronous launch-path config-load sequence; an async actor would reorder the C-API calls. Principled over the actor shape the audit suggested.
Tests
17 behavior unit tests with in-memory fakes for both seams (CJK mapping, font-config scanning incl.
config-fileincludes, legacy-config rules, scan-path composition, theme override).Stats / wiring
GhosttyTerminalView.swiftshrinks 12093 → 11691 lines (−402);.github/swift-file-length-budget.tsvreconciled.cmux.xcodeproj(6 pbxproj entries) + app-target dependency.NOTE: full app build is validated by GitHub CI; local app
xcodebuildwas skipped intentionally (parallel refactor agents). The new package builds and tests pass locally, andscripts/lint-ios-package-conventions.shis clean with zero newlint:allow.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted Ghostty config discovery/scanning into a new leaf package
CmuxGhosttyConfigLoaderwithGhosttyConfigDiscovery, reducingGhosttyTerminalView.swiftby 402 lines and keeping behavior identical. Improves testability and isolates pure logic from app state.GhosttyConfigDiscovery.GhosttyConfigFileReading(defaultFileManagerGhosttyConfigFileReader),GhosttyFontProbing(defaultCoreTextGhosttyFontProbe); injectsCmuxGhosttyConfigPathResolver.ghostty_config_*calls.CmuxGhosttyConfigLoaderintocmux.xcodeproj.Written for commit b56a7bb. Summary will update on new commits.