Repository navigation
Consolidate ghostty extractions into CmuxTerminalCore (no new packages) - #6227
Merged
Merged
Conversation
Moves GhosttyConfigDiscovery and its GhosttyConfigFileReading / GhosttyFontProbing / UserFontConfigSummary seams (byte-identical logic) from the rejected per-sliver CmuxGhosttyConfigLoader package into the existing CmuxTerminalCore domain package under ConfigDiscovery/. The discovery type already targeted CmuxTerminalCore's GhosttyConfig and CmuxFoundation's CmuxGhosttyConfigPathResolver, so the only change is dropping the now-internal 'import CmuxTerminalCore' and re-homing the test as @testable import CmuxTerminalCore. GhosttyTerminalView forwards to it via the existing CmuxTerminalCore import (no new app-side import). The CmuxGhosttyConfigLoader package and its 6 pbxproj entries are removed; zero new top-level packages. Supersedes PR for feat-ghostty-engine-config-loader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Core Moves GhosttyTriggerShortcutDecoder and its value types (GhosttyTriggerInput, GhosttyModifierMask, GhosttyTriggerPhysicalKey, GhosttyTriggerShortcut) plus the CmuxConfigStoreReloadCoordinator and its CmuxConfigStoreReloading / CmuxConfigStoreReloadEnvironment protocol seams (byte-identical logic) from the rejected per-sliver CmuxGhosttyShortcutDecoding package into the existing CmuxTerminalCore domain package under ShortcutDecoding/. Because CmuxTerminalCore already re-vends the GhosttyKit binary target, the GhosttyTriggerPhysicalKey+GhosttyKit boundary adapter (mapping ghostty_input_key_e onto the key token) moves into the package alongside its type instead of staying app-side; its import flips from CmuxGhosttyShortcutDecoding to GhosttyKit, the switch body is unchanged. The empty CmuxConfigStore: CmuxConfigStoreReloading conformer stays in the app target with its import flipped to CmuxTerminalCore. AppDelegate forwards through its existing CmuxTerminalCore import (redundant CmuxGhosttyShortcutDecoding import dropped). The coordinator already holds its environment weakly, so the app delegate owning it creates no retain cycle. The CmuxGhosttyShortcutDecoding package and its pbxproj wiring are removed, along with the now-dangling app-target source entries for the moved GhosttyKit adapter; zero new top-level packages. Supersedes PR for feat-menu-and-configuration-reload-wiring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent 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 (2)
📝 WalkthroughWalkthroughExtracts Ghostty config discovery logic (CJK font fallback, theme override, scan-path resolution, config parsing) from ChangesGhosttyConfigDiscovery extraction and delegation
Ghostty trigger-to-shortcut decoding pipeline
Config store reload coordination and AppDelegate integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryConsolidates two previously-rejected per-package Ghostty feature branches (
Confidence Score: 5/5Safe to merge — all moved logic is byte-identical to the app-target originals, 122 passing tests cover the relocated suites, and the two deleted packages are fully replaced by equivalent code inside CmuxTerminalCore. The change is a structural consolidation with no new behavior. All CJK, legacy-config, theme-override, shortcut-decoder, and reload-coordinator logic is provably byte-identical to what it replaces. The injected seams (GhosttyConfigFileReading, GhosttyFontProbing, CmuxConfigStoreReloadEnvironment) are correctly narrow, the weak-reference retain-cycle avoidance in the coordinator is sound, and the GhosttyKit modifier bit positions are documented against the fixed Ghostty ABI. No actor isolation or concurrency regressions introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph AppTarget["App Target"]
AD["AppDelegate\n(CmuxConfigStoreReloadEnvironment)"]
GTV["GhosttyApp\n(GhosttyTerminalView.swift)"]
CCS["CmuxConfigStore\n(CmuxConfigStoreReloading)"]
ADConf["configStoreReloadCoordinator\n(lazy)"]
ADStatic["private static let\nconfigDiscovery"]
end
subgraph CmuxTerminalCore["Packages/CmuxTerminalCore"]
GCD["GhosttyConfigDiscovery\n(ConfigDiscovery/)"]
GCFR["GhosttyConfigFileReading\n(protocol seam)"]
GFP["GhosttyFontProbing\n(protocol seam)"]
CCSRC["CmuxConfigStoreReloadCoordinator\n(ShortcutDecoding/)"]
CCSR["CmuxConfigStoreReloading\n(protocol seam)"]
CCSRE["CmuxConfigStoreReloadEnvironment\n(protocol seam)"]
GTSD["GhosttyTriggerShortcutDecoder"]
GTPK["GhosttyTriggerPhysicalKey\n+GhosttyKit boundary"]
GMM["GhosttyModifierMask"]
end
AD --> |"conforms to"| CCSRE
AD --> |"owns"| ADConf
ADConf --> |"wraps"| CCSRC
CCSRC --> |"weak ref via"| CCSRE
CCS --> |"conforms to"| CCSR
CCSRC --> |"drives reloads via"| CCSR
GTV --> |"owns static"| ADStatic
ADStatic --> |"instance of"| GCD
GCD --> |"uses"| GCFR
GCD --> |"uses"| GFP
GTV --> |"delegates CJK/theme/path calls to"| GCD
GTV --> |"static decoder"| GTSD
GTSD --> |"uses"| GTPK
GTSD --> |"uses"| GMM
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph AppTarget["App Target"]
AD["AppDelegate\n(CmuxConfigStoreReloadEnvironment)"]
GTV["GhosttyApp\n(GhosttyTerminalView.swift)"]
CCS["CmuxConfigStore\n(CmuxConfigStoreReloading)"]
ADConf["configStoreReloadCoordinator\n(lazy)"]
ADStatic["private static let\nconfigDiscovery"]
end
subgraph CmuxTerminalCore["Packages/CmuxTerminalCore"]
GCD["GhosttyConfigDiscovery\n(ConfigDiscovery/)"]
GCFR["GhosttyConfigFileReading\n(protocol seam)"]
GFP["GhosttyFontProbing\n(protocol seam)"]
CCSRC["CmuxConfigStoreReloadCoordinator\n(ShortcutDecoding/)"]
CCSR["CmuxConfigStoreReloading\n(protocol seam)"]
CCSRE["CmuxConfigStoreReloadEnvironment\n(protocol seam)"]
GTSD["GhosttyTriggerShortcutDecoder"]
GTPK["GhosttyTriggerPhysicalKey\n+GhosttyKit boundary"]
GMM["GhosttyModifierMask"]
end
AD --> |"conforms to"| CCSRE
AD --> |"owns"| ADConf
ADConf --> |"wraps"| CCSRC
CCSRC --> |"weak ref via"| CCSRE
CCS --> |"conforms to"| CCSR
CCSRC --> |"drives reloads via"| CCSR
GTV --> |"owns static"| ADStatic
ADStatic --> |"instance of"| GCD
GCD --> |"uses"| GCFR
GCD --> |"uses"| GFP
GTV --> |"delegates CJK/theme/path calls to"| GCD
GTV --> |"static decoder"| GTSD
GTSD --> |"uses"| GTPK
GTSD --> |"uses"| GMM
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+54
| public import Observation | ||
|
|
||
| /// Orchestrates reloading every per-window cmux configuration store and refreshing | ||
| /// window titles, as the app delegate's `reloadCmuxConfigStores(source:)` did. | ||
| /// | ||
| /// Owned by the app delegate and given a weak reference to it through | ||
| /// `CmuxConfigStoreReloadEnvironment`, so the delegate retaining the coordinator | ||
| /// does not create a retain cycle. The coordinator does no I/O itself; it sequences | ||
| /// the stores' own `loadAll()` and the environment's title refresh, deduping shared | ||
| /// stores by object identity exactly as before. | ||
| @MainActor | ||
| @Observable | ||
| public final class CmuxConfigStoreReloadCoordinator { | ||
| private weak var environment: (any CmuxConfigStoreReloadEnvironment)? | ||
| private let onReload: (@MainActor (_ source: String, _ storeCount: Int) -> Void)? | ||
|
|
||
| /// Creates a coordinator. | ||
| /// - Parameters: | ||
| /// - environment: The source of per-window stores and the title refresher. | ||
| /// Held weakly because the environment (the app delegate) owns this | ||
| /// coordinator. | ||
| /// - onReload: An optional hook invoked after each reload with the reload | ||
| /// source and the number of distinct stores reloaded. The app target wires | ||
| /// this to its debug log; tests use it to observe behavior. | ||
| public init( | ||
| environment: any CmuxConfigStoreReloadEnvironment, | ||
| onReload: (@MainActor (_ source: String, _ storeCount: Int) -> Void)? = nil | ||
| ) { | ||
| self.environment = environment | ||
| self.onReload = onReload | ||
| } | ||
|
|
||
| /// Reloads every distinct per-window configuration store, then refreshes window | ||
| /// titles, then reports the reload through `onReload`. | ||
| /// | ||
| /// Distinct stores are determined by object identity so a store shared across | ||
| /// windows reloads once, preserving the original iteration-and-dedupe order. | ||
| /// - Parameter source: A short tag describing what triggered the reload. | ||
| public func reload(source: String) { | ||
| guard let environment else { | ||
| onReload?(source, 0) | ||
| return | ||
| } | ||
|
|
||
| var seenStores = Set<ObjectIdentifier>() | ||
| for store in environment.reloadableConfigStores { | ||
| let identifier = ObjectIdentifier(store) | ||
| guard seenStores.insert(identifier).inserted else { continue } | ||
| store.loadAll() | ||
| } | ||
| environment.refreshWindowTitlesAfterConfigReload() | ||
| onReload?(source, seenStores.count) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
ShortcutDecoding/ directory
CmuxConfigStoreReloadCoordinator, CmuxConfigStoreReloading, and CmuxConfigStoreReloadEnvironment are conceptually about config-store lifecycle and reload coordination, not shortcut decoding. Grouping them under ShortcutDecoding/ means a future reader tracing the reload flow won't find them there. A ConfigReload/ subdirectory alongside ConfigDiscovery/ and ShortcutDecoding/ would make the directory layout self-documenting.
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!
…observable The GhosttyKit-to-core mapping init init?(ghosttyPhysicalKey:) was internal to CmuxTerminalCore but called from the app target (AppDelegate), an inaccessible- initializer compile error. Make it public and promote the GhosttyKit import to public import so the C parameter type is valid in the public signature. CmuxConfigStoreReloadCoordinator was @observable but nothing observes it (the app delegate holds it as a lazy var and only calls reload(source:)); the weak environment seam should not be tracked. Drop @observable and the Observation import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
![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
🤖 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/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift`:
- Around line 461-482: The early return statement in the guard at
currentBundleIdentifier prevents the release-bundle config path evaluation block
from executing when the current bundle identifier is nil or empty. Move the
release-bundle config discovery logic (the releaseDir, releaseLegacyConfig,
releaseConfig, and the shouldIncludeLegacyGhosttyConfigInScanPaths check)
outside of and after the guard statement so that release-bundle paths are
evaluated regardless of whether currentBundleIdentifier exists. Keep the
appSupportConfigURLs evaluation inside the guard since it depends on the current
bundle identifier.
- Around line 315-335: The code has two performance issues: the initial loop
scanning configPaths (lines 315-321) lacks deduplication and will rescan paths,
and the while loop (line 325) uses removeFirst() on an Array which is O(n) per
operation. Initialize loadedRecursivePaths before the first loop (not just
before the while loop), then in the initial loop standardize each path and check
if it's already in loadedRecursivePaths before calling scanFontConfigFile,
adding it afterward. For the while loop, replace the inefficient
Array.removeFirst() pattern by changing recursiveConfigPaths to use a Deque
collection type (or similar efficient queue) to achieve O(1) popleft operations,
or alternatively convert to using a Set and iterating while removing elements.
🪄 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
Run ID: 1bace6f7-85d0-4bd2-abcb-251654fba9fc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (19)
Packages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigFileReading.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyFontProbing.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/UserFontConfigSummary.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/CmuxConfigStoreReloadCoordinator.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/CmuxConfigStoreReloading.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyModifierMask.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerInput.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerPhysicalKey+GhosttyKit.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerPhysicalKey.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerShortcut.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerShortcutDecoder.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/CmuxConfigStoreReloadCoordinatorTests.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyTriggerShortcutDecoderTests.swiftSources/AppDelegate.swiftSources/CmuxConfigStore+ConfigStoreReloading.swiftSources/GhosttyTerminalView.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+315
to
+335
| for path in configPaths.map({ NSString(string: $0).expandingTildeInPath }) { | ||
| scanFontConfigFile( | ||
| atPath: path, | ||
| summary: &summary, | ||
| recursiveConfigPaths: &recursiveConfigPaths | ||
| ) | ||
| } | ||
|
|
||
| var loadedRecursivePaths = Set<String>() | ||
| while !recursiveConfigPaths.isEmpty { | ||
| let path = recursiveConfigPaths.removeFirst() | ||
| let resolved = (path as NSString).standardizingPath | ||
| guard !loadedRecursivePaths.contains(resolved) else { continue } | ||
| loadedRecursivePaths.insert(resolved) | ||
|
|
||
| scanFontConfigFile( | ||
| atPath: path, | ||
| summary: &summary, | ||
| recursiveConfigPaths: &recursiveConfigPaths | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid O(n²) queue churn and duplicate top-level rescans in include traversal.
Line 325 uses removeFirst() on an Array, which is O(n) per pop, and Lines 315-321 scan top-level paths without a dedupe set. In a large include tree, this creates avoidable quadratic work and repeated file parses on launch-path config discovery.
Suggested refactor
- var recursiveConfigPaths: [String] = []
-
- for path in configPaths.map({ NSString(string: $0).expandingTildeInPath }) {
- scanFontConfigFile(
- atPath: path,
- summary: &summary,
- recursiveConfigPaths: &recursiveConfigPaths
- )
- }
-
- var loadedRecursivePaths = Set<String>()
- while !recursiveConfigPaths.isEmpty {
- let path = recursiveConfigPaths.removeFirst()
- let resolved = (path as NSString).standardizingPath
- guard !loadedRecursivePaths.contains(resolved) else { continue }
- loadedRecursivePaths.insert(resolved)
+ var recursiveConfigPaths: [String] = []
+ var enqueued = Set<String>()
+ func enqueue(_ rawPath: String) {
+ let expanded = NSString(string: rawPath).expandingTildeInPath
+ let resolved = (expanded as NSString).standardizingPath
+ if enqueued.insert(resolved).inserted {
+ recursiveConfigPaths.append(resolved)
+ }
+ }
+
+ for path in configPaths {
+ enqueue(path)
+ }
+
+ var index = 0
+ while index < recursiveConfigPaths.count {
+ let path = recursiveConfigPaths[index]
+ index += 1
scanFontConfigFile(
atPath: path,
summary: &summary,
recursiveConfigPaths: &recursiveConfigPaths
)
}As per coding guidelines, config discovery over user-provided path/include collections should dedupe absolute paths and avoid repeated full rescans.
🤖 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/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift`
around lines 315 - 335, The code has two performance issues: the initial loop
scanning configPaths (lines 315-321) lacks deduplication and will rescan paths,
and the while loop (line 325) uses removeFirst() on an Array which is O(n) per
operation. Initialize loadedRecursivePaths before the first loop (not just
before the while loop), then in the initial loop standardize each path and check
if it's already in loadedRecursivePaths before calling scanFontConfigFile,
adding it afterward. For the while loop, replace the inefficient
Array.removeFirst() pattern by changing recursiveConfigPaths to use a Deque
collection type (or similar efficient queue) to achieve O(1) popleft operations,
or alternatively convert to using a Set and iterating while removing elements.
Source: Coding guidelines
Comment on lines
+461
to
+482
| guard let bundleId = currentBundleIdentifier, | ||
| !bundleId.isEmpty else { return paths } | ||
|
|
||
| let appSupportConfigURLs = cmuxAppSupportConfigURLs( | ||
| currentBundleIdentifier: bundleId, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) | ||
| paths.append(contentsOf: appSupportConfigURLs.map(\.path)) | ||
|
|
||
| let releaseDir = appSupportDirectory.appendingPathComponent(Self.releaseBundleIdentifier, isDirectory: true) | ||
| let releaseLegacyConfig = releaseDir.appendingPathComponent("config", isDirectory: false) | ||
| let releaseConfig = releaseDir.appendingPathComponent("config.ghostty", isDirectory: false) | ||
|
|
||
| let releaseConfigSize = fileReader.fileSize(atPath: releaseConfig.path) | ||
| let releaseLegacyConfigSize = fileReader.fileSize(atPath: releaseLegacyConfig.path) | ||
|
|
||
| if Self.shouldIncludeLegacyGhosttyConfigInScanPaths( | ||
| newConfigFileSize: releaseConfigSize, | ||
| legacyConfigFileSize: releaseLegacyConfigSize | ||
| ), !paths.contains(releaseLegacyConfig.path) { | ||
| paths.append(releaseLegacyConfig.path) | ||
| } |
There was a problem hiding this comment.
Do not return before evaluating release-bundle config paths.
Line 461 returns early when currentBundleIdentifier is nil/empty, so the release-bundle block at Lines 470-482 is skipped entirely. That drops release Application Support config discovery in a valid call path.
Suggested fix
- guard let bundleId = currentBundleIdentifier,
- !bundleId.isEmpty else { return paths }
-
- let appSupportConfigURLs = cmuxAppSupportConfigURLs(
- currentBundleIdentifier: bundleId,
- appSupportDirectory: appSupportDirectory
- )
- paths.append(contentsOf: appSupportConfigURLs.map(\.path))
+ if let bundleId = currentBundleIdentifier, !bundleId.isEmpty {
+ let appSupportConfigURLs = cmuxAppSupportConfigURLs(
+ currentBundleIdentifier: bundleId,
+ appSupportDirectory: appSupportDirectory
+ )
+ paths.append(contentsOf: appSupportConfigURLs.map(\.path))
+ }Based on PR objectives, release-bundle support directories should be part of scan-path resolution regardless of current build identity.
🤖 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/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swift`
around lines 461 - 482, The early return statement in the guard at
currentBundleIdentifier prevents the release-bundle config path evaluation block
from executing when the current bundle identifier is nil or empty. Move the
release-bundle config discovery logic (the releaseDir, releaseLegacyConfig,
releaseConfig, and the shouldIncludeLegacyGhosttyConfigInScanPaths check)
outside of and after the guard statement so that release-bundle paths are
evaluated regardless of whether currentBundleIdentifier exists. Keep the
appSupportConfigURLs evaluation inside the guard since it depends on the current
bundle identifier.
# Conflicts: # .github/swift-file-length-budget.tsv
azooz2003-bit
force-pushed
the
feat-consolidate-ghostty
branch
from
June 16, 2026 22:02
0b55d6a to
7c6d4e7
Compare
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/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/UserFontConfigSummary.swift`:
- Around line 41-51: The recordFontFamily method performs an O(n) array contains
check on every insertion, creating quadratic time complexity during large
directive scans. Refactor this by maintaining a separate Set property alongside
the effectiveFontFamilies array: use the Set for O(1) membership lookups in the
guard statement at line 47, and update both the Set and array when appending a
new font family. This preserves the order of effectiveFontFamilies while
achieving constant-time deduplication.
🪄 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
Run ID: d503d93a-4e53-4dab-92e0-932088b29128
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (19)
Packages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigDiscovery.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyConfigFileReading.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/GhosttyFontProbing.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/UserFontConfigSummary.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/CmuxConfigStoreReloadCoordinator.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/CmuxConfigStoreReloading.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyModifierMask.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerInput.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerPhysicalKey+GhosttyKit.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerPhysicalKey.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerShortcut.swiftPackages/CmuxTerminalCore/Sources/CmuxTerminalCore/ShortcutDecoding/GhosttyTriggerShortcutDecoder.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/CmuxConfigStoreReloadCoordinatorTests.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigDiscoveryTests.swiftPackages/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyTriggerShortcutDecoderTests.swiftSources/AppDelegate.swiftSources/CmuxConfigStore+ConfigStoreReloading.swiftSources/GhosttyTerminalView.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+41
to
+51
| public mutating func recordFontFamily(_ value: String) { | ||
| if value.isEmpty { | ||
| effectiveFontFamilies.removeAll() | ||
| return | ||
| } | ||
|
|
||
| guard !effectiveFontFamilies.contains(value) else { | ||
| return | ||
| } | ||
|
|
||
| effectiveFontFamilies.append(value) |
There was a problem hiding this comment.
Use constant-time dedupe tracking for font-family entries.
Line 47 does an array contains check for every insert, which makes large directive scans quadratic. Keep order in effectiveFontFamilies, but track membership in a Set for O(1) dedupe.
♻️ Suggested change
public struct UserFontConfigSummary: Equatable, Sendable {
@@
public var effectiveFontFamilies: [String] = []
+ private var seenFontFamilies: Set<String> = []
@@
public mutating func recordFontFamily(_ value: String) {
if value.isEmpty {
effectiveFontFamilies.removeAll()
+ seenFontFamilies.removeAll()
return
}
- guard !effectiveFontFamilies.contains(value) else {
+ guard seenFontFamilies.insert(value).inserted else {
return
}
effectiveFontFamilies.append(value)
}
}As per coding guidelines, config discovery over user-controlled collections should avoid repeated full scans and prefer Set/Dictionary deduping.
🤖 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/CmuxTerminalCore/Sources/CmuxTerminalCore/ConfigDiscovery/UserFontConfigSummary.swift`
around lines 41 - 51, The recordFontFamily method performs an O(n) array
contains check on every insertion, creating quadratic time complexity during
large directive scans. Refactor this by maintaining a separate Set property
alongside the effectiveFontFamilies array: use the Set for O(1) membership
lookups in the guard statement at line 47, and update both the Set and array
when appending a new font family. This preserves the order of
effectiveFontFamilies while achieving constant-time deduplication.
Source: Coding guidelines
# Conflicts: # .github/swift-file-length-budget.tsv
This was referenced Jun 17, 2026
azooz2003-bit
added a commit
that referenced
this pull request
Jun 20, 2026
…hful lift remains Scope ad-config-reload-17 (drain ghostty/cmux config-reload + menu-item glue into a package) maps entirely onto residue that is already extracted, frozen, or app-coupled. No byte-identical source change is available here. Findings (all verified against base 2078bcb): - Config-store reload orchestration (reloadCmuxConfigStores / reloadableConfigStores / refreshWindowTitlesAfterConfigReload / configStore(for:|forTabManager:) / firstContextWithConfigStore) was ALREADY drained into CmuxConfigStoreReloadCoordinator + CmuxConfigStoreReloadEnvironment in CmuxTerminalCore by #6227 (441fa2c, an ancestor of this base). AppDelegate's methods are thin forwarders / irreducible live-state seam conformance reading app-target windowConfigStores + mainWindowContexts. Coordinator is wired (lazy var configStoreReloadCoordinator, environment: self) and tested (CmuxConfigStoreReloadCoordinatorTests, 150 pkg tests green). - Ghostty trigger decode (storedShortcutFromGhosttyTrigger) already routes through the package decoder GhosttyTriggerShortcut(decoding:) + GhosttyTriggerInput / GhosttyTriggerPhysicalKey / GhosttyModifierMask in CmuxTerminalCore (#6227). The app residue lifts the C ghostty_input_trigger_s and maps to app-target StoredShortcut: irreducible C-type + app-type coupling. - CmuxThemeNotifications.reloadConfig ("com.cmuxterm.themes.reload-config") is a DistributedNotificationCenter CROSS-PROCESS wire contract with the CLI (CLI/CMUXCLI+Themes.swift cmuxThemesReloadNotificationName). Frozen; cannot become an AsyncStream without breaking `cmux themes`. - .ghosttyConfigDidReload is an in-process notification with 8+ external SwiftUI .onReceive observers; its Notification.Name is owned by the HIGHER package CmuxTerminal (depends on CmuxTerminalCore), so an AsyncStream bridge cannot live in CmuxTerminalCore or CmuxSettings without a DAG cycle, and the NotificationCenter->AsyncStream conversion is behavior-affecting (main-hop / lifecycle change), flagged as deferred modernization in AppDelegate.plan.md L96. Not a byte-identical lift; out of scope here. - Menu-item glue (installReloadConfigurationMenuItemAction / scheduleReloadConfigurationMenuItemRefresh / menuNeedsUpdate / configureReloadConfigurationMenuItem / reloadConfigurationMenuItem), reloadConfiguration, and refreshTerminalSurfacesAfterGhosttyConfigReload are NSMenu + GhosttyApp.shared + TabManager/TerminalPanel iteration. Charter keeps NSMenu hosting app-side; localized title resolution must stay app-bundle-bound. No source change. Build SUCCEEDED at base; CmuxTerminalCore 150 tests green. Residue documented for the integrator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
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
1 participant
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.
Folds two rejected per-sliver Ghostty refactor branches into the existing CmuxTerminalCore domain package. No new top-level packages are created; the extracted types live where the rest of the terminal/Ghostty domain already lives, and the app target loses two package dependencies.
Slivers folded (logic byte-identical):
feat-ghostty-engine-config-loader ->
Packages/CmuxTerminalCore/.../ConfigDiscovery/.GhosttyConfigDiscoveryplus itsGhosttyConfigFileReading/GhosttyFontProbingseams andUserFontConfigSummary. The discovery type already targeted CmuxTerminalCore'sGhosttyConfigand CmuxFoundation'sCmuxGhosttyConfigPathResolver, so the only change is dropping the now-internalpublic import CmuxTerminalCore.GhosttyTerminalViewforwards through its existingimport CmuxTerminalCore.feat-menu-and-configuration-reload-wiring ->
Packages/CmuxTerminalCore/.../ShortcutDecoding/.GhosttyTriggerShortcutDecoderand its value types (GhosttyTriggerInput,GhosttyModifierMask,GhosttyTriggerPhysicalKey,GhosttyTriggerShortcut), plusCmuxConfigStoreReloadCoordinatorand itsCmuxConfigStoreReloading/CmuxConfigStoreReloadEnvironmentseams. Because CmuxTerminalCore already re-vends the GhosttyKit binary target, theGhosttyTriggerPhysicalKey+GhosttyKitboundary adapter moves into the package alongside its type (import flips from the old package toGhosttyKit, switch body unchanged) instead of staying app-side. The emptyCmuxConfigStore: CmuxConfigStoreReloadingconformer stays in the app target with its import flipped toCmuxTerminalCore. The coordinator already holds its environment weakly, so the app delegate owning it creates no retain cycle.Both
Packages/CmuxGhosttyConfigLoader/andPackages/CmuxGhosttyShortcutDecoding/directories are deleted, along with their pbxproj wiring and the now-dangling app-target source entries for the moved GhosttyKit adapter.AppDelegateandGhosttyTerminalViewdrop the redundant per-sliver imports.Verification:
scripts/lint-ios-package-conventions.shshows no new ERROR lines from these files and zero newlint:allow(the one pre-existing ERROR,ComposerDictationTextMerge, is byte-identical on main and untouched here).swift buildandswift testinPackages/CmuxTerminalCorepass (122 tests, 26 suites), including the movedGhosttyConfigDiscovery*,GhosttyTriggerShortcutDecoder, andCmuxConfigStoreReloadCoordinatorsuites..github/swift-file-length-budget.tsvreconciled (budget respected;GhosttyConfigDiscovery.swiftretracked at the new path). pbxproj passesplutil -lint,normalize-pbxproj.py(no-op),check-pbxproj.sh, andlint-pbxproj-test-wiring.sh. Full-app build left to CI.Supersedes the PRs for feat-ghostty-engine-config-loader and feat-menu-and-configuration-reload-wiring.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Consolidates Ghostty config discovery, shortcut decoding, and config reload orchestration into
CmuxTerminalCore, removing two packages and simplifying app code with no behavior changes. Fixes a cross‑module access error by making the GhosttyKit key‑mapping initializer public.Refactors
GhosttyConfigDiscovery(+GhosttyConfigFileReading,GhosttyFontProbing,UserFontConfigSummary) toPackages/CmuxTerminalCore/.../ConfigDiscovery/;GhosttyTerminalViewnow calls this and drops large inline logic.GhosttyTriggerShortcutDecoder(+ value types) andCmuxConfigStoreReloadCoordinator(+ seams) toPackages/CmuxTerminalCore/.../ShortcutDecoding/; integratedGhosttyTriggerPhysicalKey+GhosttyKitinside the package.AppDelegateconforms toCmuxConfigStoreReloadEnvironmentand owns the coordinator; addedSources/CmuxConfigStore+ConfigStoreReloading.swiftsoCmuxConfigStoreconforms.Packages/CmuxGhosttyConfigLoader/andPackages/CmuxGhosttyShortcutDecoding/; tests re-homed underPackages/CmuxTerminalCore/Tests/; file-length budget and pbxproj updated.Bug Fixes
GhosttyTriggerPhysicalKey.init?(ghosttyPhysicalKey:)public and usedpublic import GhosttyKitto resolve cross-module access.@ObservablefromCmuxConfigStoreReloadCoordinator.Written for commit 80ff2f7. Summary will update on new commits.
Summary by CodeRabbit