GhosttyTerminalView decomposition: terminal surface model (stack D)#6024
Open
azooz2003-bit wants to merge 20 commits into
Open
GhosttyTerminalView decomposition: terminal surface model (stack D)#6024azooz2003-bit wants to merge 20 commits into
azooz2003-bit wants to merge 20 commits into
Conversation
…e 1) Drains the surface-model cluster out of Sources/GhosttyTerminalView.swift (15,221 -> 12,164 lines) into the new Packages/CmuxTerminal domain package, building on the CmuxTerminalCore/Engine/Services slice: - TerminalSurface + SearchState move to CmuxTerminal/Surface as a faithful lift (still ObservableObject/@published; modernization is a separate phase), split by role: Input, Sizing, Renderer, RuntimeLifecycle, CopyMode, PortalLease, Mobile, Debug, StartupEnvironment. - The legacy reach-ups (GhosttyApp.shared, TerminalController.shared, MobileTerminalByteTee.shared, RendererRealizationController.shared, AgentHibernationController.shared, terminalSurfaceRegistry static) invert through TerminalSurfaceRuntimeDependencies seams (TerminalEngineHosting, TerminalSurfaceViewProviding, TerminalSurfaceSpawnPolicyProviding, TerminalByteTeeBinding, TerminalRendererRealizationScheduling, AgentHibernationRecording); app conformances live in Sources/TerminalSurfaceRuntimeWiring.swift with a convenience init that keeps every legacy call site byte-identical. - TerminalSurfaceRuntimeTeardownCoordinator.shared actor singleton becomes an injected instance; the enqueue free functions fold into a member with a defaulted freeSurface parameter. - CmuxSurfaceConfigTemplate, the surface runtime probes, the Claude command shim, and the cmux context environment lift to CmuxTerminalCore (SurfaceValues/, Interop/); WorkspaceSurfaceConfig.swift keeps forwarding shims for existing callers. - GhosttyRuntimeTestStubs across Core/Engine/Services gain the ghostty_surface_quicklook_font stub the new probe references. - Behavior tests for the teardown coordinator (injected-free invocation, multi-surface drain); the app-side startup-environment and search tests keep exercising the moved model through the app target. Defaults keys, environment keys, and notification raw strings are byte-identical; an adversarial normalize-and-diff of createSurface and the teardown path against pre-tranche HEAD shows only the seam substitutions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…e model The cmuxTests files that name TerminalSurface as a type cannot see it through @testable import cmux_DEV alone after the lift; the package import is added unconditionally above the canImport(cmux_DEV) block (per the conditional-import CI trap in the refactor LEARNINGS). Cutover +1-line budget refresh for the three test files at their cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Greptile SummaryStack D of the modular refactor extracts
Confidence Score: 4/5Safe to merge; all behavioral changes are seam substitutions with byte-identical notification strings and environment keys, and the build gates are reported green. The refactor is disciplined and well-documented: isolation contracts are explicit, teardown ordering is preserved across both the MainActor and nonisolated deinit paths, and the new coordinator is independently tested. Two non-blocking observations remain: the 5-second Task.sleep in the diagnostic timeout observer is new production code that the blocking-runtime rule flags, and TerminalSurface/SearchState introduce ObservableObject/@published in a new Swift 6 package where Observation would be the preferred shape (explicitly deferred in the PR description). TerminalSurfaceRuntimeTeardownCoordinator.swift (the Task.sleep timeout observer) and TerminalSurface.swift (ObservableObject deferral tracking). Important Files Changed
Sequence DiagramsequenceDiagram
participant D as deinit (nonisolated)
participant TC as TerminalSurface (MainActor)
participant TW as TerminalSurfaceRuntimeWiring
participant C as TeardownCoordinator (actor)
participant W as Worker Task (utility)
participant G as ghostty_surface_free
Note over TC: teardownSurface() [@MainActor]
TC->>TC: "nil out surface pointer & callbackContext"
TC->>TC: registry.unregisterRuntimeSurface
TC->>G: "Task { @MainActor ghostty_surface_free } (next turn)"
Note over D: deinit (nonisolated)
D->>D: nil out surface pointer
D->>D: registry.unregisterRuntimeSurface
D->>C: enqueueRuntimeTeardown (nonisolated)
C->>C: "Task { await self.enqueue(request) }"
C->>W: Task.detached(priority: .utility)
W->>C: nextRequestForWorker()
W->>W: "Task { observeTimeout(id:) }"
W->>G: request.freeSurface(surface)
W->>TC: "MainActor.run { callbackContext?.release() }"
W->>C: complete(id:)
Note over TW: App composition root injects dependencies
TW->>TC: TerminalSurface(dependencies:)
TW->>C: runtimeTeardown: coordinator
Reviews (1): Last reviewed commit: "Hoist import CmuxTerminal into app-side ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+112
to
+126
| private func observeTimeout(id: UUID) async { | ||
| do { | ||
| // Genuine teardown deadline: report a stuck native free without blocking close. | ||
| try await Task.sleep(for: timeout) | ||
| } catch { | ||
| return | ||
| } | ||
| guard let reason = pendingReasonsById[id] else { return } | ||
| #if DEBUG | ||
| logDebugEvent( | ||
| "surface.lifecycle.nativeFree.timeout surface=\(id.uuidString.prefix(5)) " + | ||
| "reason=\(reason)" | ||
| ) | ||
| #endif | ||
| } |
Contributor
There was a problem hiding this comment.
Task.sleep in production timeout observer
observeTimeout uses Task.sleep(for: .seconds(5)) in the shipped coordinator actor. The blocking-runtime rule flags Task.sleep in production code by default. The real completion signal already exists — complete(id:) removes the entry from pendingReasonsById — so the observer can wait on that signal instead of polling with a sleep. A concurrency-correct alternative is to hold a task group or use withTimeout so the diagnostic path responds to the actual completion event rather than waking unconditionally and re-checking the dictionary.
File Used: .github/review-bot-rules/swift-blocking-runtime.md (source)
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!
Comment on lines
+25
to
+43
| public final class TerminalSurface: Identifiable, ObservableObject { | ||
| /// The live find-in-terminal session state for one surface. | ||
| public final class SearchState: ObservableObject { | ||
| /// The current search needle. | ||
| @Published public var needle: String | ||
|
|
||
| /// The 1-based index of the selected match, if known. | ||
| @Published public var selected: UInt? | ||
|
|
||
| /// The total number of matches, if known. | ||
| @Published public var total: UInt? | ||
|
|
||
| /// Creates search state with an initial needle. | ||
| public init(needle: String = "") { | ||
| self.needle = needle | ||
| self.selected = nil | ||
| self.total = nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
ObservableObject/@Published in a Swift 6 package
TerminalSurface and its nested SearchState both adopt ObservableObject with @Published properties in the new CmuxTerminal package, which is compiled with Swift Language Mode 6. The concurrency-modernization rule flags new ObservableObject/@Published usage when Observation and async/await are available. The PR description explicitly defers this as "a separate phase," but since this is newly introduced in a Swift 6 package rather than being an in-place edit of the existing god-file code, it registers as a new instance of the legacy pattern. Consider at least adding a TODO or a // MIGRATION(Observation): ... comment to the class-level doc block so the deferred upgrade has a clear tracking anchor.
File Used: .github/review-bot-rules/swift-concurrency-modernization.md (source)
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!
…dow (stack D, window-chrome tranche) Drains the window-chrome free functions out of the GhosttyTerminalView god file into a new CmuxWorkspaceWindow package, the first window-domain leaf of the stack-D decomposition. Moved (faithful lift, logic byte-identical): - cmuxShouldApplyWindowGlass / cmuxShouldUseTransparentBackgroundWindow / cmuxShouldUseClearWindowBackground / cmuxTransparentWindowBaseColor -> WindowBackgroundPolicy (Sendable value type). The two persisted settings are read through the injected WindowBackgroundSettingsReading seam instead of UserDefaults.standard; glassEffectAvailable stays a caller-supplied param so NSGlassEffectView lifecycle stays app-side. - cmuxResetCompositorBackgroundBlur + its two @_silgen_name CGS trampolines -> CompositorBlurController (the trampolines are the sanctioned C-API exception to the no-free-function rule). App side: WindowBackgroundComposition holds the transitional process-wide policy + blur instances (UserDefaults-backed settings conformance), mirroring the existing GhosttyApp composition statics. Five call sites updated across WindowAppearanceSnapshot, BrowserPanel, TerminalPanelView, and WindowBackdropController; each passes WindowGlassEffect.isAvailable explicitly to preserve the legacy default-arg behavior exactly. Latency-neutral: window background/glass/blur run on appearance changes and panel-appearance construction, never on the keystroke or render-frame hot path. GhosttyTerminalView.swift: 12226 -> 12181 lines. New package: 5 tests (WindowBackgroundPolicyTests) pass; app build SUCCEEDED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ck D, tranche A sub-1) Faithful lift of the three app-target config-path/setting-editor namespaces (CmuxApplicationSupportDirectories, CmuxGhosttyConfigPathResolver, CmuxGhosttyConfigSettingEditor) from Sources/CmuxApplicationSupportDirectories.swift into CmuxTerminalCore/ConfigPaths/, one type per file. These are the pure, Foundation-only leaf that GhosttyConfig and the GhosttyApp engine helpers both recurse through; lifting them first unblocks the GhosttyConfig <-> GhosttyApp config-helper de-recursion (Tranche A). Bodies are byte-identical to the app original (machine-diff verified); the only deltas are `public`/`public import Foundation` and DocC. The namespace-enum shape is preserved with a TRANSITIONAL `lint:allow namespace-type` justification; modernization into instantiated resolvers is deferred to the engine lift. App callers (GhosttyConfig, GhosttyTerminalView, ConfigSource, HostSettingsActions) and the two app-host tests gain `import CmuxTerminalCore`; the GhosttyApp.* config wrappers continue to delegate unchanged. Gates: CmuxTerminalCore swift build + 66 tests pass; lint exit 0; file-length budget refreshed for the +1 import lines (cutover precedent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…D, tranche A sub-2) Move the GhosttyBackgroundBlur enum (the `background-blur` config value, with `init(cValue:)` and `isMacOSGlassStyle`) from the window-domain WindowAppearanceSnapshot.swift into CmuxTerminalCore/ConfigValues/, so the lifted GhosttyConfig can store it without reaching back into the app target. The `windowGlassStyle -> WindowGlassEffect.Style?` bridge stays app-side as an extension on the lifted enum, because WindowGlassEffect is a window-chrome type the terminal core must not depend on (keeps the package DAG acyclic; the engine core never sees the window domain). Enum core is byte-identical to the app original (machine-diff verified); deltas are `public`, a trivially-correct `Sendable` conformance (all cases are value types), and DocC. Consumers of `.isMacOSGlassStyle` (BrowserPanel/TerminalPanelView/WindowBackdropController) and the moved enum gain `import CmuxTerminalCore`. Gates: CmuxTerminalCore swift build + tests pass; lint exit 0; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ranche A sub-3) Move GhosttyConfig's NSColor extension (init?(hex:), isLightColor, luminance, darken(by:)) into CmuxTerminalCore/Color/NSColor+Hex.swift. This is the sole NSColor(hex:) in the app (the Sidebar's hex initializer is on SwiftUI.Color, not NSColor, so there is no conflict), and GhosttyConfig stores NSColor values parsed via init?(hex:), so the color helpers must travel with it ahead of the GhosttyConfig lift. Member bodies are byte-identical to the app original (machine-diff verified); the only delta is `public` + DocC. All app call sites of NSColor(hex:), .isLightColor, .luminance, and .darken(by:) gain `import CmuxTerminalCore`. Gates: CmuxTerminalCore swift build + 66 tests pass; lint exit 0; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rship (stack D, tranche A sub-4a) The mutual recursion (GhosttyConfig.loadFromDisk -> GhosttyApp.shouldApply ManagedDefaultAppearance / .cmuxAppSupportConfigURLs; those helpers speak GhosttyConfig) blocked lifting the engine's config type. Resolved by moving the config-scanning logic to where it belongs (GhosttyConfig owns config parsing), not by a seam/closure: - The appearance-scan cluster (UserAppearanceConfigSummary, userAppearance ConfigSummary, scanAppearanceConfigFile, shouldApplyManagedDefaultAppearance) moves from GhosttyApp into GhosttyConfig, reusing GhosttyConfig's own byte-identical parsedConfigEntry/applyConfigFileDirective. Bodies are machine-diff identical to the originals; deltas are visibility (private-> internal so GhosttyApp can still read the summary) and a removed default param that no call site used. - GhosttyConfig.cmuxConfigPaths now calls CmuxGhosttyConfigPathResolver .loadConfigURLs (already in CmuxTerminalCore) directly instead of routing through GhosttyApp.cmuxAppSupportConfigURLs. - GhosttyApp.shouldApplyManagedDefaultAppearance and conditionalThemeOverride ConfigContents now delegate UP to GhosttyConfig (engine depends on config), keeping their existing internal callers and the GhosttyApp.* test surface working unchanged. GhosttyConfig.swift now has zero GhosttyApp references: the dependency is one-way (GhosttyApp -> GhosttyConfig), so GhosttyConfig is liftable to CmuxTerminalCore in the next sub-tranche. God file shrinks ~98 lines; config type grows by the moved cluster. Gates: lint exit 0; budget refreshed for the move. App compile pending. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ries.swift (stack D, tranche A fixup) Sub-tranche A1 git-rm'd Sources/CmuxApplicationSupportDirectories.swift but left its 6 pbxproj entries (PBXFileReference, group entry, and the PBXBuildFile + Sources-phase pairs for both the cmux and cmux-cli targets), so the app build failed with "Build input file cannot be found". Drop all 6; normalize + check pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…res it (stack D, tranche A fixup-2) The full app build revealed the cmux-cli target also compiles the config-path/setting-editor namespaces (CmuxApplicationSupportDirectories, CmuxGhosttyConfigPathResolver, CmuxGhosttyConfigSettingEditor) via CLI/CMUXCLI+ Themes/Config/ThemeSupport. They were briefly placed in CmuxTerminalCore (sub-1), but CmuxTerminalCore links the GhosttyKit binaryTarget + AppKit; forcing those into the standalone CLI is wrong. These three types are pure Foundation, so they belong in CmuxFoundation (zero deps, already linked by both the app and the CLI). Move them there; CmuxTerminal Core now depends on CmuxFoundation so the forthcoming GhosttyConfig lift can still use them. The app Sources, CLI, and tests gain `import CmuxFoundation`. Heavier config values (GhosttyBackgroundBlur, NSColor+Hex) stay in CmuxTerminal Core since the CLI does not use them. Gates: CmuxFoundation build + 51 tests pass; CmuxTerminalCore builds on the new edge; full app + cmux-cli xcodebuild BUILD SUCCEEDED; lint exit 0; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lor types (stack D, tranche A fixup-3) test-depot failed fast (~2min, pre-test phase) because the cmuxTests target referenced the lifted NSColor(hex:)/GhosttyBackgroundBlur types (now in CmuxTerminalCore) without linking that package, and GhosttyConfigPathResolver Tests carried a stale `import CmuxTerminalCore` for a target that did not link it. Fix: - Add CmuxTerminalCore to the cmuxTests target (6-entry pbxproj wiring mirroring the existing CmuxFoundation cmuxTests dependency: new XCSwiftPackageProduct Dependency + PBXBuildFile, added to the cmuxTests Frameworks phase and package ProductDependencies; the shared XCLocalSwiftPackageReference is reused). - Add `import CmuxTerminalCore` (hoisted above the canImport block) to the test files using NSColor(hex:)/GhosttyBackgroundBlur: GhosttyConfigTests, GhosttyNotificationDispatcher, SidebarWidthPolicy, WindowAppearanceSnapshot, WorkspaceAppearanceConfigResolution, WorkspaceUnit. - Drop the now-unneeded CmuxTerminalCore import from GhosttyConfigPathResolver Tests (it only needs CmuxFoundation for the config-path types). Exhaustive sweep: every Sources/CLI/cmuxTests reference to a lifted type now carries its package import. Gates: pbxproj check + test-wiring lint + package lint exit 0; full app xcodebuild BUILD SUCCEEDED; budget refreshed. Re-dispatching test-depot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…reset (stack D, tranche A fixup-4) test-depot's stricter Swift 6 toolchain rejected the prior window-chrome tranche's CompositorBlurController: it read NSWindow.windowNumber (a main-actor- isolated property) inside a nonisolated method, so "Testing cancelled because the build failed" before any test ran. Local xcodebuild's looser isolation never flagged it, which is why it landed red on depot. Fix: resetBackgroundBlur takes the already-resolved `windowNumber: Int` instead of an `NSWindow`, so the main-actor windowNumber read happens in the caller's isolation domain (WindowBackdropController, which already touches window.background Color/isOpaque there). The controller stays nonisolated/Sendable and the CGS C trampolines stay thread-agnostic — no @mainactor cascade, no assumeIsolated. The three WindowBackdropController call sites pass `window.windowNumber`. This is outside Tranche A proper but is the sole blocker (depot showed exactly one error) keeping the gate from validating the config lift. Gates: CmuxWorkspaceWindow build + 5 tests pass; full app xcodebuild BUILD SUCCEEDED; lint + budget exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… GhosttyKit from cmuxTests (stack D, tranche A fixup-5) test-depot's cmuxTests link failed with undefined C++ stdlib symbols (std::runtime_error/length_error). Root cause: fixup-3 added CmuxTerminalCore as a direct cmuxTests package product dependency, and CmuxTerminalCore re-vends the GhosttyKit binaryTarget (a C++ archive); the test binary then tried to link libghostty's C++ symbols without libc++. Fix, two parts: 1. NSColor+Hex and GhosttyBackgroundBlur are pure AppKit/Foundation value types with zero GhosttyKit dependency, so they move from CmuxTerminalCore to CmuxFoundation (which the app, CLI, and cmuxTests already link). All color/blur call sites across Sources, CLI, and tests now `import CmuxFoundation`; CmuxTerminalCore imports added only for color/blur are converted to CmuxFoundation, and the unused CmuxTerminalCore->CmuxFoundation package edge is dropped. 2. Revert the fixup-3 cmuxTests CmuxTerminalCore pbxproj wiring (the B2/B3 entries). Tests that use genuine CmuxTerminalCore types (TerminalAndGhostty Tests, WorkspaceUnitTests) resolve them via `@testable import cmux` exactly as at the base commit, so GhosttyKit's C++ symbols come from the app target, not the test binary. (The B2 XCSwiftPackageProductDependency body had to be removed as a whole block; a stray line-only deletion left an orphan that damaged the project file, now repaired and re-normalized.) CmuxFoundation is now the single home for the pure config-path AND config-value types; CmuxTerminalCore keeps only genuinely GhosttyKit-coupled code. Gates: CmuxFoundation build + 51 tests, CmuxTerminalCore build pass; full app xcodebuild BUILD SUCCEEDED; lint + pbxproj check + test-wiring lint + budget exit 0; project file parses (plutil convert). Re-dispatching test-depot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…functions (stack D, tranche A fixup-6) After the C++ link fix, the depot toolchain reached test-source compilation and failed: GhosttyConfigTests' WindowTransparencyDecisionTests class still called cmuxShouldUseTransparentBackgroundWindow / cmuxShouldUseClearWindowBackground / cmuxShouldApplyWindowGlass, which the prior window-chrome tranche deleted from the app target when it lifted them into CmuxWorkspaceWindow.WindowBackgroundPolicy. The stale test was left behind, so cmuxTests would not compile (this break predates this stack but only surfaced once the earlier compile/link blockers were cleared). Equivalent coverage already lives in CmuxWorkspaceWindow's WindowBackgroundPolicyTests (glass-requires-behind-window-and-enabled, glass-availability-does-not-change-decision, transparent-window-follows-settings, clear-background-on-transparency-or-low-opacity, transparent-base-color), so the dead class is removed with a pointer comment, not re-homed. No live coverage lost. Gates: lint + pbxproj + test-wiring + budget exit 0; the depot log showed these were the only remaining compile errors, all inside the removed class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce (stack D, tranche A sub-5a) Second blocker for moving GhosttyConfig into a package: GhosttyConfig still reached up into the app-target AppearanceSettings type. Its signature referenced AppearanceSettings.SystemAppearance, a pure system-interface-style value with no app-appearance-mode dependency. Lifted that value to CmuxTerminalCore as TerminalSystemAppearance (public, Equatable, Sendable). The body is byte-identical to the original (prefersDark caseInsensitiveCompare; current(defaults:) direct + global-domain fallback read of the frozen AppleInterfaceStyle key). Expected deltas only: type rename for the package namespace, internal->public + an explicit public init (memberwise init is non-public across a module boundary), and the two interface-style constants becoming static members on the struct. App-side AppearanceSettings keeps a `typealias SystemAppearance = TerminalSystemAppearance` so every call site (including the test that constructs SystemAppearance(interfaceStyle:)) stays byte-identical. GhosttyConfig now types its systemAppearance parameter as TerminalSystemAppearance directly, removing one of its two AppearanceSettings references. Gates: CmuxTerminalCore swift build + 69 tests pass (3 new TerminalSystemAppearance tests); lint clean; app xcodebuild Debug BUILD SUCCEEDED; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mePreference (stack D, tranche A sub-5b) GhosttyConfig.ColorSchemePreference is a terminal-domain value (it keys libghostty theme selection) referenced at ~40 sites across the terminal view/engine code. Lifted the two-case enum to CmuxTerminalCore as TerminalColorSchemePreference (Hashable, Sendable). Faithful: same two cases (.light/.dark), same Hashable conformance; only adds Sendable (trivially true for a value enum) and the package namespace. GhosttyConfig keeps a nested `typealias ColorSchemePreference = TerminalColorSchemePreference`, so every `GhosttyConfig.ColorSchemePreference` call site (AppDelegate, GhosttyTerminalView stored properties, AppearanceSettings return types, tests) stays byte-identical. This moves the type out of the app target ahead of the eventual GhosttyConfig move, without touching any call site. Gates: CmuxTerminalCore swift build + 69 tests pass; lint clean; app xcodebuild Debug BUILD SUCCEEDED; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…onfig now AppearanceSettings-free (stack D, tranche A sub-5c) Final blocker removed: GhosttyConfig.currentColorSchemePreference delegated to AppearanceSettings.terminalColorSchemePreference (app-appearance domain), keeping GhosttyConfig tied to the app target. Lifted the resolution into TerminalColorSchemePreference.resolve(appearanceMode RawValue:systemAppearance:) in CmuxTerminalCore. It is byte-faithful to the legacy logic: an explicit "light"/"dark" mode short-circuits, everything else (system/auto/unset/unknown) follows the system interface style. This matches AppearanceSettings.mode(for:) collapsing auto->system and unknown->system before only .light/.dark short-circuited. GhosttyConfig.currentColorSchemePreference now calls TerminalColorSchemePreference.current (reads the frozen "appearanceMode" key itself). App-side AppearanceSettings.terminalColorSchemePreference forwards its normalized mode rawValue into the same package resolver, so both surfaces share one source of truth and stay byte-identical. The app-domain AppearanceMode enum (localized, used across UI/keyboard-shortcut/pairing files) stays app-side. GhosttyConfig.swift now has ZERO AppearanceSettings references and ZERO GhosttyApp references: its only remaining app-target reach is the DEBUG-gated GhosttyStartupAppearancePreviewState (next sub-tranche). Both config<->app recursions are broken. Gates: CmuxTerminalCore swift build + 74 tests pass (5 new resolution tests); lint clean; app xcodebuild Debug BUILD SUCCEEDED; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… tranche A sub-5d) GhosttyConfig's sole remaining app-target reach was the DEBUG-gated GhosttyStartupAppearancePreviewState.profile read in loadFromDisk. Inverted it via a CmuxTerminalCore seam, TerminalStartupAppearancePreviewOverride: a DEBUG-only Sendable value carrying loadsRealUserConfig + a previewConfigContents closure, with a process-wide installed hook the app target is the sole writer of. GhosttyConfig.loadFromDisk now consults the seam (installed?.loadsRealUserConfig ?? true), so no override installed == the real-user-config path, matching the prior default profile (.realUserConfig). Control flow is byte-identical: the loadConfigFiles / applyCmuxDefaultAppearance / parse bodies are unchanged; only the gate source and the default-to-real coalescing changed. App-side GhosttyStartupAppearancePreviewState.profile becomes a computed property (DEBUG) that installs the override on write, keeping every call site (StartupAppearanceDebugView, applyPreview/reset, GhosttyConfigTests) byte-identical while making the app the single writer. The app-target enum keeps its localized display strings. GhosttyConfig.swift now has ZERO app-target type references (no GhosttyApp, AppearanceSettings, AppDelegate, Workspace, or GhosttyStartupAppearance*). Its dependencies are Foundation/AppKit + CmuxFoundation + CmuxTerminalCore symbols only, so the type is now fully liftable into CmuxTerminalCore (the file move + 6.4k-line test retarget is the remaining Tranche A step). Gates: CmuxTerminalCore swift build + 74 tests pass; lint clean (justified DEBUG-only nonisolated(unsafe) hook); app xcodebuild Debug BUILD SUCCEEDED; budget refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Moves the sRGB hex-encoding extension from the app target (ContentView.swift) to CmuxFoundation's NSColor+Hex.swift, alongside its inverse init?(hex:). This removes GhosttyConfig's last app-target reach-up (applySidebarAppearanceToUser Defaults() calls color.hexString()), unblocking the GhosttyConfig file-move into CmuxTerminalCore in the next sub-tranche. Adds DocC, makes it public, and imports CmuxFoundation in the four app files that used the symbol without already importing the module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completes the GhosttyConfig lift: the 1,163-line config value type drains out of the app target into CmuxTerminalCore/Config/GhosttyConfig.swift. The app target keeps a one-line `typealias GhosttyConfig = CmuxTerminalCore.GhosttyConfig` so every call site (including `GhosttyConfig.ColorSchemePreference` and `GhosttyConfig.UserAppearanceConfigSummary` member lookups) stays byte-identical. Adds the CmuxFoundation dependency to CmuxTerminalCore (no cycle: CmuxFoundation is a zero-dependency leaf). All ~50 members made public + DocC; explicit public init() since the memberwise init is not public across a module boundary. Two Swift-6 strict-concurrency boundaries the app target tolerated needed faithful, behavior-preserving fixes (not logic changes): - cachedConfigsByColorScheme: nonisolated(unsafe) static var with justification that loadCacheLock (NSLock) serializes every access — the lock contract is unchanged from the app-target original. - loadFromDisk: @usableFromInline internal (was private) so it can back the public `load` default-argument value; the body is not inlinable. The frozen wire format (directive keys, theme resolution, NSColor hex codecs, appearance-summary scanning) is preserved byte-identical; a machine-diff vs the pre-move body showed only public/DocC/import/explicit-init deltas. CmuxTerminalCore: swift build + swift test green (74 tests). App: xcodebuild Debug BUILD SUCCEEDED, 0 errors. GhosttyConfigTests stays in the app-host test target unchanged: it reaches GhosttyConfig only through its public API (no @testable-private access), so the typealias keeps every assertion byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…erminalCore (stack D, tranche A sub-6c) Moves the two self-contained Swift Testing @suite structs that test the lifted GhosttyConfig — SidebarFontSizeConfigTests and SurfaceTabBarFontSizeConfigTests — out of the app-host cmuxTests grab-bag and into the CmuxTerminalCore package test target, where the type they exercise now lives. They were already pure public-API tests (no @testable-private access) using @Suite/@Test/#expect, so the move is a faithful copy plus package imports; assertions are byte-identical. Adds CmuxFoundation to the package test target (for CmuxGhosttyConfigSettingEditor in the surface-tab-bar editor round-trip tests). The larger GhosttyConfigTests class and SidebarBackgroundConfigTests stay in the app-host target: GhosttyConfigTests is an inseparable grab-bag (it mixes genuine GhosttyConfig theme/parse/load tests with app-target Telemetry/Kiro/Claude integration and bundled-CLI tests in one class), and both reach app-target-only helpers. Splitting that class would risk weakening coverage for no boundary benefit; they keep working against the lifted type through the app typealias, which exposes only GhosttyConfig's public API (exactly what these tests use). CmuxTerminalCore: swift test green, 92 tests in 19 suites (the 18 moved tests included). App: xcodebuild Debug BUILD SUCCEEDED, 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jun 13, 2026
…nder welcome PRs included: - iOS notifications cross-device dismiss-sync + authoritative unread count - iOS workspace row actions (manaflow-ai#6022) - Sidebar perf: kill LazyVStack layout livelock (manaflow-ai#6019 + manaflow-ai#6024) - iOS sign-out local-first offline-safe revocation - iOS workspace list groups, unread dots, last-activity previews - Refresh notification fallback sound DnD - Email Founder's Edition customers welcome via Resend webhook - Misc swift-file-length-budget refresh + extract NotificationSoundSettings - cloud-testflight --marketing-version override Fork-side adjustments: - Drop fork's mobileAllowsWorkspaceAction static helper from TerminalController: upstream put it in Sources/TerminalController+MobileNotificationSync.swift - Restore fork's renameTopLevelLayoutTabContaining and closeTopLevelLayoutTabContaining methods on Workspace (TerminalController v2WorkspaceTopTabRename/Close call sites depended on them) - pbxproj keep-both for upstream new files + fork P41 entries - Budget tsv merged via max-per-path script
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.
Stack D of the modular refactor: decomposing
Sources/GhosttyTerminalView.swiftperGhosttyTerminalView.plan.md, stacked on the TerminalEngine slice (#5929). One large PR; tranches land as separate commits.Tranche 1 (landed): TerminalSurface model into CmuxTerminal
GhosttyTerminalView.swift15,221 -> 12,226 lines.New
Packages/CmuxTerminal(Wave 3 domain package, depends on CmuxTerminalCore + CmuxGhosttyKit):TerminalSurface+SearchStatelifted faithfully (stillObservableObject/@Published; modernization is a separate phase), split by role underSurface/,Spawn/,Lifecycle/,Hosting/,Runtime/,Events/.Seam inversions (all legacy reach-ups now injected through
TerminalSurfaceRuntimeDependencies):GhosttyApp.shared->TerminalEngineHostingTerminalSurfaceViewProviding(app injects theGhosttyNSView/GhosttySurfaceScrollViewfactory)TerminalSurfaceSpawnPolicyProvidingMobileTerminalByteTee.shared+Unmanagedtee context ->TerminalByteTeeBindingwith lease objectsRendererRealizationController.shared->TerminalRendererRealizationSchedulingrecordAgentHibernationTerminalInputfree function +AgentHibernationController.shared->AgentHibernationRecordingTerminalSurfaceRuntimeTeardownCoordinator.sharedactor singleton -> injected instance; its two enqueue free functions folded into one member with defaultedfreeSurfaceApp-side residue:
Sources/TerminalSurfaceRuntimeWiring.swiftholds the conformances/bridges plus a convenience init that keeps every legacyTerminalSurface(...)call site byte-identical.CmuxSurfaceConfigTemplate, the surface runtime probes, the Claude command shim, and the cmux context environment lifted toCmuxTerminalCore;WorkspaceSurfaceConfig.swiftkeeps forwarding shims. App-side tests naming the moved type getimport CmuxTerminalhoisted above thecanImport(cmux_DEV)block.Behavior invariants: Defaults keys, environment keys (
CMUX_*), and notification raw strings byte-identical (cmux.terminalSurfaceDidBecomeReadymoved, same string). Adversarial normalize-and-diff ofcreateSurfaceand the teardown path vs pre-tranche HEAD (8bbb21b) shows only the seam substitutions.Two deliberate compiler-enforced isolation upgrades of legacy main-thread-only contracts:
TerminalSurface.searchStateis@MainActor(its didSet always ran on main), sostartOrFocusTerminalSearchis now@MainActor(both callers already were);GhosttyNSView's copy-mode indicator accessors wentfileprivate-> internal as protocol witnesses.Gates: swift_file_length_budget (cutover budget write), lint-ios-package-conventions, pbxproj normalize/check + ID-collision sweep,
swift build+swift teston CmuxTerminal/Core/Engine/Services (Core 66, Engine 14, Services 15, Terminal 2 incl. new teardown-coordinator behavior tests), full appxcodebuild buildgreen.e2e:
FindSelectionShortcutUITestsPASSED at ea8d799 (exercises full app launch + terminal + the moved searchState path: manaflow-ai/cmux-dev-artifacts#2939).AutomationSocketUITestsfailed with app-activation timeouts (manaflow-ai/cmux-dev-artifacts#2942), but that class is red onmaintoo (manaflow-ai/cmux-dev-artifacts#2528, manaflow-ai/cmux-dev-artifacts#2479), pre-existing runner flake class, not introduced by this branch.Leg 2 status: remaining slices are blocked on prerequisite domain packaging
Leg 1 (TerminalSurface MODEL → CmuxTerminal) succeeded because the model's dependencies had already been seam-inverted into CmuxTerminalCore/Engine (
TerminalSurfaceRuntimeDependencies,TerminalSurfaceViewProviding, etc.). The model package never names a view type or an app singleton.Leg 2 surveyed the rest of the god file (now 12,226 lines) to continue the lift. Every remaining slice is deeply entangled with app-domain types that have not yet been extracted into packages, so none can be lifted faithfully and latency-neutrally in isolation right now:
View layer (
GhosttyNSView3865–7923,GhosttySurfaceScrollView8027–11245, theNSTextInputClientIME extension 11246–11751, theGhosttyTerminalViewrepresentable 11752–end; ~7,400 lines) references 51 distinct app-defined types across the Panels, Appearance, Find, Settings-shortcuts, Workspace, image-transfer, and portal domains, plus theGhosttyApp/AppDelegate/Workspacesingletons. ~28 of those are reached only transiently inside method bodies (seamable to primitives), but ~23 appear in stored-property or function-signature positions and would each need either co-moving (they are shared with 1–8 other app files, so co-moving breaks their other callers) or a behavioral seam with an app-side conformer. Reach-up neutrality is favorable — everyGhosttyApp.shared(11 members) andAppDelegate.shared(8 members) reference is on a COLD path (background/appearance/theme-sync/focus/config/debug), none in the per-keystroke or per-frame inner loop; the only hot-path app symbol isCmuxTypingTimingtelemetry. So a faithful lift is possible but is a large multi-seam effort that must be dogfooded on the typing-critical paths between steps (repo CLAUDE.md flagsTerminalSurface.forceRefresh(),WindowTerminalHostView.hitTest(), and theSurfaceSearchOverlay-must-mount-from-GhosttySurfaceScrollViewportal-layering contract as fragile).Engine wrapper (
GhosttyApp362–3855, astatic let shared@MainActorsingleton; ~3,500 lines) references 45 distinct app types (incl.GhosttyConfig,MobileTerminalByteTee,RendererRealizationController,WindowBackdropController, settings types) with 17AppDelegate.shared+ 12Workspace.sharedreach-ups. Its target packageCmuxTerminalEnginecurrently depends only onCmuxTerminalCore.Even the plan's "pure Sendable engine value DTOs" (
ScrollbarVisibility,AppearanceSynchronizationPlan,RuntimeColorSchemeSynchronizationDecision,DefaultBackgroundValues, the font/appearance summaries) cannot be promoted intoCmuxTerminalEnginein isolation: they are nested inGhosttyApp, carry app types (GhosttyConfig.ColorSchemePreference,GhosttyBackgroundBlur,NSColor), and are produced byGhosttyAppstatic/instance methods that call otherGhosttyAppappearance-engine helpers. They only separate cleanly as part of the fullGhosttyApplift, which itself needsGhosttyConfigpackaged first.Prerequisite (the unblocker): package
GhosttyConfigand the shared value/styling types the view+engine layers depend on (the Panels styling enums,WindowAppearanceSnapshot, image-transfer DTOs, the Settings shortcut/scrollbar value types) — work owned by the parallel AppDelegate/TabManager/ContentView stacks. Once those land, the seam surface for the view layer drops to behavioral-only and the engine value DTOs become liftable.Corrected sequencing for the next leg (once
GhosttyConfig+ shared value types are packaged):GhosttyApp→GhosttyAppServiceinCmuxTerminalEngine, lifting the engine value DTOs to top-level Sendable types; invert itsAppDelegate/Workspacereach-ups viaWorkspaceResolving/tab-routing seams. This removes the heaviest view-layer reach-up family (GhosttyApp.shared).CmuxTerminalSurface, with the GhosttyApp/AppDelegate/Workspace/portal reach-ups injected as the plan'sTerminalSurfaceBackgroundProviding/TerminalPortalBinding/TerminalPortalGeometryReportingseams (all cold-path, so latency-neutral), the IME/scroll/overlay transient state staying in the moved view types. Preserve theSurfaceSearchOverlayportal-mount contract.CmuxWorkspaceWindow.Leg 2 leaves the worktree at the verified-green leg-1 head (file-length budget + package-conventions lint pass; CmuxTerminal package
swift buildgreen; full appxcodebuild buildBUILD SUCCEEDED) rather than forcing a high-risk partial lift of the typing-latency-critical view in a state that cannot be dogfooded between steps.🤖 Generated with Claude Code