Repository navigation
Extract window chrome domain from ContentView - #6147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extracts window chrome and backdrop logic into ChangesWindow chrome pipeline migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR extracts window chrome logic from
Confidence Score: 4/5The extraction itself is structurally sound and the main WindowAccessor path correctly threads glass availability. Two call sites (BrowserPanel, TerminalPanelView) and the mutation-ID method still hardcode glass unavailability, leaving macOS 26 glass mode with wrong window opacity and stale tint updates; these were flagged in the prior round and remain unaddressed in this commit. The refactoring compiles cleanly and the core chrome application path in ContentView now correctly consults glassEffect.isAvailable. The three remaining hardcoded-false sites (appKitWindowMutationID, BrowserPanel, TerminalPanelView) are real defects on macOS 26 when glass is enabled: the panel windows may not become transparent when they should, and the WindowAccessor will not re-fire when a user changes only the glass tint color. WindowAppearanceSnapshot.appKitWindowMutationID (hardcoded glassEffectAvailable: false), Sources/Panels/BrowserPanel.swift, and Sources/Panels/TerminalPanelView.swift both pass false to shouldUseTransparentBackgroundWindow. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CV[ContentView / View body] -->|"stores let windowChrome"| ACC[AppWindowChromeComposition]
ACC -->|creates| GE[WindowGlassEffect\ninstance]
ACC -->|creates| NTC[NativeTitlebarBackdropCoordinator\ninstance]
ACC -->|computed| BC[WindowBackdropController\nnew per access]
ACC -->|computed| CTR[WindowContentOverlayTargetResolver\nnew per access]
CV -->|refreshID = appKitWindowMutationID| WA[WindowAccessor]
WA -->|fires| CMC["configureMainWindowChrome()"]
CMC -->|glassEffectAvailable = GE.isAvailable ✅| BP[backdropPlan]
BP -->|apply| BC
BP2["BrowserPanel\nPanelAppearance"] -->|"glassEffectAvailable: false ❌"| SUT[shouldUseTransparentBackgroundWindow]
BP3["TerminalPanelView\nPanelAppearance"] -->|"glassEffectAvailable: false ❌"| SUT
MID["appKitWindowMutationID()"] -->|"glassEffectAvailable: false ❌"| MID2[backdropPlan → mutation ID\nmisses glass tint/style changes]
GE -.->|objc_associated| WIN[NSWindow\nassociated state]
NTC -.->|objc_associated| VIEW[NSView\ntitlebar state]
%%{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
CV[ContentView / View body] -->|"stores let windowChrome"| ACC[AppWindowChromeComposition]
ACC -->|creates| GE[WindowGlassEffect\ninstance]
ACC -->|creates| NTC[NativeTitlebarBackdropCoordinator\ninstance]
ACC -->|computed| BC[WindowBackdropController\nnew per access]
ACC -->|computed| CTR[WindowContentOverlayTargetResolver\nnew per access]
CV -->|refreshID = appKitWindowMutationID| WA[WindowAccessor]
WA -->|fires| CMC["configureMainWindowChrome()"]
CMC -->|glassEffectAvailable = GE.isAvailable ✅| BP[backdropPlan]
BP -->|apply| BC
BP2["BrowserPanel\nPanelAppearance"] -->|"glassEffectAvailable: false ❌"| SUT[shouldUseTransparentBackgroundWindow]
BP3["TerminalPanelView\nPanelAppearance"] -->|"glassEffectAvailable: false ❌"| SUT
MID["appKitWindowMutationID()"] -->|"glassEffectAvailable: false ❌"| MID2[backdropPlan → mutation ID\nmisses glass tint/style changes]
GE -.->|objc_associated| WIN[NSWindow\nassociated state]
NTC -.->|objc_associated| VIEW[NSView\ntitlebar state]
Reviews (14): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| for accessory in window.titlebarAccessoryViewControllers | ||
| where accessory.layoutAttribute == .leading || accessory.layoutAttribute == .left { | ||
| leading += accessory.view.frame.width | ||
| } | ||
| if leading != inset { | ||
| inset = leading | ||
| } | ||
| } |
There was a problem hiding this comment.
Legacy GCD dispatch in
NSViewRepresentable update
DispatchQueue.main.async { … } is the old concurrency pattern. Per cmux-swift-concurrency-modernization, this should use Task { @MainActor in … } instead. Both patterns defer the binding mutation past the SwiftUI update cycle, but the Task form integrates with Swift's structured concurrency and is consistent with the rest of the codebase.
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!
| @discardableResult | ||
| private func ensureInstalled() -> Bool { | ||
| guard let window, |
There was a problem hiding this comment.
Ephemeral
AppWindowChromeComposition on every event-driven call
AppWindowChromeComposition() is constructed inline here and in underlyingResponder(atWindowPoint:), WindowTmuxWorkspacePaneOverlayController.ensureInstalled(), installFileDropOverlay, WindowBrowserPortal.installationTarget, and WindowTerminalPortal.installationTarget — none of which share the windowChrome stored in ContentView. Each call allocates a fresh WindowGlassEffect + NativeTitlebarBackdropCoordinator pair just to run a single objc_getAssociatedObject lookup. The correctness is fine (state is keyed to the window), but underlyingResponder(atWindowPoint:) is called during pointer hit-testing and creates two class instances per event. Consider plumbing the contentOverlayTargetResolver from the AppWindowChromeComposition stored in ContentView down to the overlay controllers, or extracting a lightweight free function for the target lookup.
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!
| backgroundBlur: app.defaultBackgroundBlur, | ||
| usesHostLayerBackground: app.usesHostLayerBackground | ||
| ) | ||
| ).currentFromUserDefaults(defaults: .standard, colorScheme: .dark) |
There was a problem hiding this comment.
colorScheme: .dark hardcoded in titlebarControlForegroundNSColor
The old code derived colorScheme from NSApplication.shared.effectiveAppearance at the call site. The new call hardcodes .dark. While colorScheme doesn't affect compositedTerminalBackgroundColor (the only property consumed by this function), the hardcoded value is misleading and will silently produce wrong sidebar tint data if the snapshot is ever used for more than just terminal background composition. Prefer deriving the scheme from the effective appearance, as AppWindowChromeComposition.currentAppColorScheme() does.
| ).currentFromUserDefaults(defaults: .standard, colorScheme: .dark) | |
| ).currentFromUserDefaults( | |
| defaults: .standard, | |
| colorScheme: NSApplication.shared.effectiveAppearance.bestMatch(from: [.darkAqua, .aqua]) == .darkAqua ? .dark : .light | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/TerminalPanelView.swift (1)
269-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve runtime glass-availability in transparent-window policy resolution.
Line 273 hardcodes
glassEffectAvailable: false, which forces the no-glass branch and can computeusesTransparentWindow/usesClearContentBackgroundincorrectly on capable systems. Please pass the real availability signal (or require callers to provide it) instead of a fixedfalse.Based on the PR objective that window chrome decisions are now composed through injected app-side chrome composition, this path should remain capability-aware.
🤖 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 `@Sources/Panels/TerminalPanelView.swift` around lines 269 - 274, The fromConfig static method in TerminalPanelView is hardcoding glassEffectAvailable: false when calling the parameterized fromConfig, which forces the transparent window policy to ignore actual glass effect capabilities on capable systems. Replace the hardcoded false value with the actual glass effect availability signal, either by obtaining it from the current runtime environment or by refactoring the method signature to accept glassEffectAvailable as a parameter so that callers can provide the correct capability information. This ensures the WindowBackgroundComposition.policy correctly computes usesTransparentWindow and usesClearContentBackground based on real system capabilities.
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/NativeTitlebarBackdropCoordinator.swift`:
- Around line 37-47: The syncNativeTitlebarBackdrop method currently performs
multiple separate recursive traversals of the titlebar container: one
firstNativeDescendant call for "NSTitlebarView" and two separate
nativeDescendants calls for "NSTitlebarBackgroundView" and "NSVisualEffectView".
Replace these three separate scans with a single depth-first search traversal
that collects all three view types in one pass, returning them together. Update
the code at the mentioned location and the additional site at lines 210-240 to
use this consolidated single-pass collector instead of the repeated hierarchy
scans.
- Around line 31-78: The `syncNativeTitlebarBackdrop` method sets
`window.titlebarAppearsTransparent = true` when enabled (line 77) but never
restores the previous value in the disabled path (where
`restoreNativeTitlebarBackdropState` is called). Save the original value of
`window.titlebarAppearsTransparent` when remembering the state in the enabled
branch, storing it as an associated object on the window alongside the other
saved UI properties, then restore that saved value in the
`restoreNativeTitlebarBackdropState` method to ensure the titlebar transparency
mode returns to its original state when disabling.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/SidebarVisualEffectBackground.swift`:
- Around line 59-64: The glass tint persists when tintColor becomes nil because
the setTintColor: selector is only called when tintColor is non-nil. To fix
this, move the selector resolution and responds(to:) check outside the optional
binding, and always invoke nsView.perform(selector, with:...) with either the
unwrapped color value when tintColor is non-nil or nil when tintColor is nil.
This ensures that previously applied tints are properly cleared when tintColor
is removed.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowAppearanceSnapshot.swift`:
- Around line 54-57: There is a type mismatch in the call to
compositedTerminalColor on line 76: the function parameter opacity expects a
Double, but terminalBackgroundOpacity is of type CGFloat. Fix this by converting
terminalBackgroundOpacity to Double when passing it as an argument to the
compositedTerminalColor function call.
In `@Sources/BrowserWindowPortal.swift`:
- Around line 2337-2340: The code is unnecessarily creating a new
AppWindowChromeComposition instance on every call to this function, which is in
a hot UI path during portal install/sync flows. Cache or reuse the
AppWindowChromeComposition instance instead of constructing it repeatedly. Store
the composition as a property or static variable that persists across multiple
calls, then access the cached instance within the guard statement where
contentOverlayTargetResolver is called. This eliminates redundant object
construction in a frequently-triggered UI path.
In `@Sources/cmuxApp.swift`:
- Around line 3116-3118: Replace the hardcoded opacity value of 0.62 assigned to
sidebarTintOpacity with SidebarTintDefaults().opacity to maintain consistency
with the centralized defaults used elsewhere in the code (such as at line 2959).
This ensures that if the default opacity value changes in SidebarTintDefaults,
all reset operations will use the same updated value without requiring multiple
edits.
In `@Sources/Sidebar/SidebarAppearanceSupport.swift`:
- Around line 53-63: The titlebarControlForegroundNSColor function is hardcoding
colorScheme: .dark when calling currentFromUserDefaults on the
WindowAppearanceResolver, which ignores the actual system appearance and causes
incorrect titlebar foreground contrast in light mode. Replace the hardcoded
.dark with the current system color scheme or appearance setting to ensure the
WindowAppearanceSnapshot is computed correctly based on the actual environment
rather than always assuming dark mode.
---
Outside diff comments:
In `@Sources/Panels/TerminalPanelView.swift`:
- Around line 269-274: The fromConfig static method in TerminalPanelView is
hardcoding glassEffectAvailable: false when calling the parameterized
fromConfig, which forces the transparent window policy to ignore actual glass
effect capabilities on capable systems. Replace the hardcoded false value with
the actual glass effect availability signal, either by obtaining it from the
current runtime environment or by refactoring the method signature to accept
glassEffectAvailable as a parameter so that callers can provide the correct
capability information. This ensures the WindowBackgroundComposition.policy
correctly computes usesTransparentWindow and usesClearContentBackground based on
real system capabilities.
🪄 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: d82c4d2e-2650-43e6-849c-8f5790471fda
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (61)
Packages/CmuxAppKitSupportUI/Package.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/GhosttyBackgroundBlur+WindowGlassEffectStyle.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/GhosttyTerminalBackdropRenderingMode.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/LayerBackedBackdropColor.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/NativeTitlebarBackdropCoordinator.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/SidebarBackdropMaterialPolicy.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/SidebarBackdropSettingsSnapshot.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/SidebarVisualEffectBackground.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/TerminalSurfaceBackgroundFillOwner.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/TerminalSurfaceBackgroundFillPlan.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/TitlebarLeadingInsetReader.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowAppearanceResolver.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowAppearanceSnapshot.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowAppearanceUserSettingsSnapshot.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropApplicationResult.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropController.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropControllerDependencies.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropGlassPlan.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropHostingPhase.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropLayer.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropPlan.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropPolicy.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowBackdropRole.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeBorder.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeBorderOrientation.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeColorResolver.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeSidebarBlendModeOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeSidebarMaterialOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeSidebarPresetOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeSidebarStateOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowChromeSidebarTintDefaults.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowContentOverlayInstallationTarget.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowContentOverlayTargetResolver.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowGlassEffect.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowGlassEffectManaging.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowGlassEffectStyle.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowGlassSettingsSnapshot.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowRootBackdropResolution.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/WindowChrome/WindowTerminalAppearanceSnapshot.swiftPackages/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/WindowChrome/WindowAppearanceResolverTests.swiftPackages/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/WindowChrome/WindowBackdropControllerTests.swiftPackages/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/WindowChrome/WindowContentOverlayTargetResolverTests.swiftSources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/Panels/BrowserPanel.swiftSources/Panels/TerminalPanelView.swiftSources/RightSidebarChromeStyle.swiftSources/Sidebar/SidebarAppearanceSupport.swiftSources/TerminalWindowPortal.swiftSources/Windowing/AppWindowChromeComposition.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowBackdropController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GhosttyConfigTests.swiftcmuxTests/SidebarWidthPolicyTests.swiftcmuxTests/WindowAndDragTests.swiftcmuxTests/WindowAppearanceSnapshotTests.swift
💤 Files with no reviewable changes (2)
- Sources/Windowing/WindowBackdropController.swift
- Sources/Windowing/WindowAppearanceSnapshot.swift
| guard let target = AppWindowChromeComposition() | ||
| .contentOverlayTargetResolver | ||
| .installationTarget(for: window) else { return nil } | ||
| return (target.container, target.reference) |
There was a problem hiding this comment.
Avoid rebuilding the chrome composition on each installation-target lookup.
This path runs inside portal install/sync flows; constructing AppWindowChromeComposition() per call adds avoidable repeated work in a hot UI path.
💡 Suggested fix
final class WindowBrowserPortal: NSObject {
+ private let chromeComposition = AppWindowChromeComposition()
private static let transientRecoveryRetryBudget: Int = 12
@@
private func installationTarget(for window: NSWindow) -> (container: NSView, reference: NSView)? {
- guard let target = AppWindowChromeComposition()
+ guard let target = chromeComposition
.contentOverlayTargetResolver
.installationTarget(for: window) else { return nil }
return (target.container, target.reference)
}As per coding guidelines, frequently-triggered UI paths should avoid repeated per-event work when it can be cached/reused.
🤖 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 `@Sources/BrowserWindowPortal.swift` around lines 2337 - 2340, The code is
unnecessarily creating a new AppWindowChromeComposition instance on every call
to this function, which is in a hot UI path during portal install/sync flows.
Cache or reuse the AppWindowChromeComposition instance instead of constructing
it repeatedly. Store the composition as a property or static variable that
persists across multiple calls, then access the cached instance within the guard
statement where contentOverlayTargetResolver is called. This eliminates
redundant object construction in a frequently-triggered UI path.
Source: Coding guidelines
| sidebarTintOpacity = 0.62 | ||
| sidebarTintHex = SidebarTintDefaults.hex | ||
| sidebarTintHex = SidebarTintDefaults().hex | ||
| sidebarTintHexLight = nil |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Align reset tint opacity with centralized defaults.
Line 3116 still hardcodes 0.62 while Line 2959 now uses SidebarTintDefaults().opacity. Use the same default source in both places to avoid drift if defaults change.
Suggested patch
Button("Reset Tint") {
- sidebarTintOpacity = 0.62
+ sidebarTintOpacity = SidebarTintDefaults().opacity
sidebarTintHex = SidebarTintDefaults().hex
sidebarTintHexLight = nil
sidebarTintHexDark = nil
}🤖 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 `@Sources/cmuxApp.swift` around lines 3116 - 3118, Replace the hardcoded
opacity value of 0.62 assigned to sidebarTintOpacity with
SidebarTintDefaults().opacity to maintain consistency with the centralized
defaults used elsewhere in the code (such as at line 2959). This ensures that if
the default opacity value changes in SidebarTintDefaults, all reset operations
will use the same updated value without requiring multiple edits.
| func titlebarControlForegroundNSColor(opacity: CGFloat) -> NSColor { | ||
| let app = GhosttyApp.shared | ||
| let appearance = WindowAppearanceResolver( | ||
| terminalAppearance: WindowTerminalAppearanceSnapshot( | ||
| backgroundColor: app.defaultBackgroundColor, | ||
| backgroundOpacity: app.defaultBackgroundOpacity, | ||
| backgroundBlur: app.defaultBackgroundBlur, | ||
| usesHostLayerBackground: app.usesHostLayerBackground | ||
| ) | ||
| ).currentFromUserDefaults(defaults: .standard, colorScheme: .dark) | ||
| return titlebarControlForegroundNSColor( |
There was a problem hiding this comment.
Avoid hardcoding .dark when deriving WindowAppearanceSnapshot.
Using colorScheme: .dark unconditionally can compute the wrong snapshot defaults in light appearance, leading to incorrect titlebar foreground contrast selection.
Suggested fix
func titlebarControlForegroundNSColor(opacity: CGFloat) -> NSColor {
let app = GhosttyApp.shared
+ let bestMatch = NSApp?.effectiveAppearance.bestMatch(from: [.darkAqua, .aqua])
+ let colorScheme: ColorScheme = (bestMatch == .darkAqua) ? .dark : .light
let appearance = WindowAppearanceResolver(
terminalAppearance: WindowTerminalAppearanceSnapshot(
backgroundColor: app.defaultBackgroundColor,
backgroundOpacity: app.defaultBackgroundOpacity,
backgroundBlur: app.defaultBackgroundBlur,
usesHostLayerBackground: app.usesHostLayerBackground
)
- ).currentFromUserDefaults(defaults: .standard, colorScheme: .dark)
+ ).currentFromUserDefaults(defaults: .standard, colorScheme: colorScheme)
return titlebarControlForegroundNSColor(
opacity: opacity,
appearance: appearance
)
}🤖 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 `@Sources/Sidebar/SidebarAppearanceSupport.swift` around lines 53 - 63, The
titlebarControlForegroundNSColor function is hardcoding colorScheme: .dark when
calling currentFromUserDefaults on the WindowAppearanceResolver, which ignores
the actual system appearance and causes incorrect titlebar foreground contrast
in light mode. Replace the hardcoded .dark with the current system color scheme
or appearance setting to ensure the WindowAppearanceSnapshot is computed
correctly based on the actual environment rather than always assuming dark mode.
# Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
| public func appKitWindowMutationID(windowBackgroundPolicy: WindowBackgroundPolicy) -> String { | ||
| backdropPlan( | ||
| glassEffectAvailable: false, | ||
| windowBackgroundPolicy: windowBackgroundPolicy | ||
| ).appKitMutationID | ||
| } |
There was a problem hiding this comment.
appKitWindowMutationID always passes glassEffectAvailable: false, breaking glass-mode tint/style updates
The old implementation used WindowGlassEffect.isAvailable (the actual runtime value) as the default, so appKitWindowMutationID reflected the live glass plan. The new function hardcodes false: on macOS 26+ where NSGlassEffectView is available, the backdrop plan computed with false is a non-glass plan that omits glass tint and style. This means that when a user changes bgGlassTintHex or bgGlassTintOpacity while glass mode is already active, the mutation ID does not change, so the WindowAccessor refresh callback never fires, and the glass tint never updates via the AppKit path.
The fix is to thread glassEffectAvailable through the call site — the equivalent of the old backdropPlan(glassEffectAvailable: WindowGlassEffect.isAvailable).appKitMutationID — by adding glassEffectAvailable: Bool as a parameter and calling backdropPlan(glassEffectAvailable: glassEffectAvailable, windowBackgroundPolicy: windowBackgroundPolicy).appKitMutationID.
WindowAppearanceSnapshotPaneBackgroundTests references WindowAppearanceSnapshot, TerminalSurfaceBackgroundFillPlan, and the sidebar/glass snapshot types that this PR relocated to CmuxAppKitSupportUI, but it was left with only @testable import cmux and would fail to compile in the cmuxTests target. Add the package imports matching the sibling WindowAppearanceSnapshotTests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WindowGlassSettingsSnapshot.shouldApply moved into CmuxAppKitSupportUI and now requires a windowBackgroundPolicy: argument. This wired cmuxTests call still used the old one-argument signature, build-breaking the test target. Pass WindowBackgroundComposition.policy to match the sibling backdropPlan / shouldUseTransparentHosting calls in the same test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
After re-syncing onto main, two failures surfaced that the app-only build missed: - cmuxTests/WindowAndDragTests.swift referenced TitlebarLeadingInsetPassthroughView, which this PR moved from Sources/ContentView.swift into a private type inside CmuxAppKitSupportUI/WindowChrome/TitlebarLeadingInsetReader.swift. The test target stopped compiling (tests + activation-session jobs). Make the view internal and move its hit-test / mouseDownCanMoveWindow coverage into a package test where the type now lives; rename the remaining app-target class to MainWindowDragBehaviorTests (it only covers MainWindowHostingView/CmuxMainWindow). - AppWindowChromeComposition.swift emitted 3 new Swift concurrency warnings (over the 0 budget for the new file): main-actor default-arg evaluation of NSApplication.shared.effectiveAppearance and a non-Sendable fullscreenAuxiliaryWindows default closure. Resolve the actor-isolated defaults inside the @mainactor bodies instead of in nonisolated default-arg position; behavior unchanged (still defaults to NSApp.windows / current effectiveAppearance). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
4128-4153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate surface backdrop overrides to the focused panel.
Line 4147 now lets a surface-local
backgroundColordrive the shared window-root backdrop, but Lines 4135-4142 only verify that the workspace/tab is selected. In a split workspace, an unfocused terminal receiving an OSC/config background update can therefore recolor the entire window. Require the surface to be the focused panel before applying its override, or passnilfor unfocused surfaces.Proposed fix
`@MainActor` func applyWindowBackgroundIfActive() { guard let window else { return } let appDelegate = AppDelegate.shared let owningManager = tabId.flatMap { appDelegate?.tabManagerFor(tabId: $0) } let owningSelectedTabId = owningManager?.selectedTabId let activeSelectedTabId = owningManager == nil ? appDelegate?.tabManager?.selectedTabId : nil guard Self.shouldApplyWindowBackground( surfaceTabId: tabId, owningManagerExists: owningManager != nil, owningSelectedTabId: owningSelectedTabId, activeSelectedTabId: activeSelectedTabId ) else { return } + if let terminalSurface, + let workspace = owningManager?.tabs.first(where: { $0.id == terminalSurface.tabId }), + workspace.focusedPanelId != terminalSurface.id { + return + } applySurfaceBackground() let windowChrome = AppWindowChromeComposition() let windowRoot = windowChrome .appearanceSnapshotFromUserDefaults(app: GhosttyApp.shared) .windowRootBackdropResolution(surfaceBackgroundColor: backgroundColor)🤖 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 `@Sources/GhosttyTerminalView.swift` around lines 4128 - 4153, The applyWindowBackgroundIfActive() function applies a surface-local backgroundColor to the window-root backdrop without verifying that the surface is the focused panel, allowing unfocused terminals in split workspaces to unintentionally recolor the entire window. Enhance the guard condition in the shouldApplyWindowBackground() check to also verify that the surface is the focused panel before proceeding, or alternatively, pass nil for the backgroundColor parameter when the surface is not focused to prevent unfocused surfaces from overriding the window backdrop through the windowRoot.snapshot.windowRootBackdropResolution call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4128-4153: The applyWindowBackgroundIfActive() function applies a
surface-local backgroundColor to the window-root backdrop without verifying that
the surface is the focused panel, allowing unfocused terminals in split
workspaces to unintentionally recolor the entire window. Enhance the guard
condition in the shouldApplyWindowBackground() check to also verify that the
surface is the focused panel before proceeding, or alternatively, pass nil for
the backgroundColor parameter when the surface is not focused to prevent
unfocused surfaces from overriding the window backdrop through the
windowRoot.snapshot.windowRootBackdropResolution call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad9523c4-7c31-443b-b4d9-8d552f526516
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GhosttyConfigTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/GhosttyConfigTests.swift
The re-sync merge absorbed sibling tmux-overlay regression tests (WorkspaceContentViewVisibilityTests, added by "Add tmux attention regression tests" on main) that `import Bonsplit` and reference Bonsplit.PixelRect / PaneState / TabItem / DropZone directly, alongside the pre-existing PortalTabDragRoutingTests, BrowserPaneDropRoutingTests, and AppDelegateEqualizeSplitsShortcutTests. On main those symbols resolved transitively through a directly-linked package product. This PR's window-chrome extraction into CmuxAppKitSupportUI perturbed the symbol graph so the linker dead-stripped the Bonsplit objects the test-only references needed, breaking the cmuxTests bundle link (ld: symbol(s) not found for architecture arm64) in the `tests` job. The app-only `xcodebuild build` did not exercise the test target, so it passed locally. Fix: link the Bonsplit product directly into the cmuxTests target (packageProductDependencies + Frameworks phase), mirroring how the app target depends on it. A target that imports and uses Bonsplit should link it explicitly rather than rely on a fragile transitive path. Scope is the test target only; no app/runtime behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cmuxTests target shared the app target's Bonsplit product-dependency object (A5001261) and build file, so Xcode associated the link with the app target only and dropped it from the test bundle. WindowChrome extraction made two test files import CmuxWorkspaceWindow, which public-imports Bonsplit, so the test bundle now references Bonsplit type metadata and the missing link produced undefined Bonsplit.* symbols at link time. Give cmuxTests its own XCSwiftPackageProductDependency (A5001262) wired through both packageProductDependencies and its Frameworks build phase, mirroring the per-target B2/C2 pattern used by every other package. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
18fac58 to
ccb90f8
Compare
…muxAppKitSupportUI (absorb 6278 reorg)
Merge origin/main and adapt the window-chrome extraction to main's
Packages/{Shared,iOS,macOS}/ layout: move the new WindowChrome sources/tests
into Packages/macOS/CmuxAppKitSupportUI (main already carries the matching
CmuxFoundation + CmuxWorkspaceWindow deps). Regenerate the file-length budget
after relocation; preserve the cmuxTests Bonsplit per-target link fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Group the 40 window-chrome files into descriptive subfolders (Appearance, Backdrop, Glass, Titlebar, Border, Color, Sidebar, TerminalSurface, Overlay) so the package is navigable. Pure git mv, history preserved; no behavior change. Mirror the same split in the test target. Add a package-root README explaining what each subfolder and file is for, written for an unfamiliar reader. Every public type already carries a DocC /// summary. No de-static needed: the controllers (WindowBackdropController, WindowGlassEffect, NativeTitlebarBackdropCoordinator) are real instance types with constructor-injected dependencies; the only statics are constant identifiers, ObjC associated-object keys, and value-type factory methods (sanctioned by CONVENTIONS section 9), none a static-only namespace. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| usesTransparentWindow: WindowBackgroundComposition.policy | ||
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect.isAvailable) | ||
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: false) |
There was a problem hiding this comment.
Transparent-window selection regresses on macOS 26 because
glassEffectAvailable is hardcoded to false. The old call site used WindowGlassEffect.isAvailable (a static property that has since moved to an instance). When NSGlassEffectView is present and the user has glass enabled, shouldUseTransparentBackgroundWindow returns a different value than it did before this PR, so BrowserPanel may no longer set isOpaque = false on the window, breaking the compositing pass that glass rendering depends on.
| usesTransparentWindow: WindowBackgroundComposition.policy | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect.isAvailable) | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: false) | |
| usesTransparentWindow: WindowBackgroundComposition.policy | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect().isAvailable) |
| usesTransparentWindow: WindowBackgroundComposition.policy | ||
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect.isAvailable) | ||
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: false) |
There was a problem hiding this comment.
Same regression as in
BrowserPanel.swift: glassEffectAvailable is hardcoded to false, so PanelAppearance.fromCurrentConfig will always compute transparent-window mode as if glass is unavailable. On macOS 26 with glass enabled this can give the wrong opacity setting for the panel window.
| usesTransparentWindow: WindowBackgroundComposition.policy | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect.isAvailable) | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: false) | |
| usesTransparentWindow: WindowBackgroundComposition.policy | |
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: WindowGlassEffect().isAvailable) |
The file uses the CmuxFoundation NSColor.isLightColor extension but only imported AppKit; it currently resolves via a sibling file's public import under whole-module compilation, but Swift imports are file-scoped so a per-file/incremental build is fragile. Make the dependency explicit.
The split-off file uses String(localized:defaultValue:) (Foundation) but had no imports; it resolves via whole-module compilation today, but Swift imports are file-scoped so make Foundation explicit. (Other zero-import WindowChrome files are pure-stdlib enums and correctly need no import.)
…own file Addresses the cmux-policy file-organization P2 on AppWindowChromeComposition.swift: AppWindowBackdropControllerDependencies is a separate concrete WindowBackdropControllerDependencies adapter, not a tightly-coupled helper of the composition struct, so it lives in its own file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
bonsplit submodule: 6 commits (split-button polish, default keep-five-visible) cmux upstream highlights pulled in: - Cmd+Shift+K Clear Screen Keep Scrollback (manaflow-ai#6139) - Window chrome domain extracted from ContentView -> CmuxAppKitSupportUI (manaflow-ai#6147) - Sidebar models extracted to CmuxSidebar package (Status/Git/Detail/Layout) - PreferredEditor*Settings -> PreferredEditorService (CmuxFileOpen) - Renderer realization (off-screen GPU memory reclaim) Adapter changes (fork-side): - Resolve 5 conflict deletions (SentryNoiseFilter, Window backdrop/glass moved) - Take upstream TerminalSection/TerminalCatalogSection/PostHogAnalytics - Delete 10 local Sidebar* type defs in Workspace.swift (484 lines, now in CmuxSidebar) - Add 'import CmuxSidebar / CmuxFileOpen / CmuxAppKitSupportUI / CmuxCommandPalette / CmuxNotifications' to consumers - Stub fork-only TS methods on local TerminalSurface: clearScreenKeepingScrollback (returns false; not used in fork build) - Stub v2BrowserFindWithScript body (broken closure scope post-merge, cmux_term doesn't use browser-find RPCs) - Replace v2BrowserFindFirst/Last/Nth/FrameSelect ctx.webView -> browserPanel.webView - v2RunJavaScript: rename param world->contentWorld inside body - Wrap v2AwaitCallback calls in MainActor.assumeIsolated for nonisolated callers - Stub PostHogAnalytics.flushForApplicationTermination (removed upstream) - AppScrollerStylePolicy.applyAtLaunch -> direct UserDefaults write - SidebarBranchOrdering: add () for instance methods (multi-line sed missed) - SidebarBranchOrdering.orderedPanelIds removed -> uuid-sort fallback - ColorSchemePreference convert local -> CmuxTerminalCore type at boundary - @mainactor annotations on cmuxShouldUseTransparentBackgroundWindow, cmuxShouldUseClearWindowBackground, openCmuxSettingsFileInEditor, applyWindowBackgroundIfActive - WindowGlassEffect: () for instance ctor Verified intact: 6 cmux_term v2 handlers, chat_source.py, 14 Python lib files, herdrInbound case.
Addressing review (Aziz): why so many types, and folder separation
Why so many small types. "Window chrome" is everything cmux paints around and behind the terminal: the window background, the native titlebar backdrop, the sidebar material, the hairline borders, and the macOS 26 glass effect. The extraction resolves chrome in stages so each stage is independently unit-testable without a live window: read current state into a
*Snapshot, resolve it into a*Plan/*Policy, then a thin@MainActorcontroller applies the plan to a realNSWindowand returns a*Result. Most files are therefore tinySendablevalue types (snapshots, plans, options, results). The AppKit mutation lives only in three controller types. This is what kept every chrome file under the 500-line budget. One major public type per file, named after the type, per the conventions.Folder separation (this round). The 40 files now sit in descriptive subfolders by concern, all via
git mvso history is preserved (no behavior change). The test target mirrors the same split. A new package-rootREADME.md(Packages/macOS/CmuxAppKitSupportUI/README.md) explains each subfolder and file for an unfamiliar reader, and every public type already carries a DocC///summary.Appearance/resolves the window's overall appearance for one render pass (terminal + user-settings snapshots into a resolvedWindowAppearanceSnapshot).Backdrop/the window background fill: role, policy, hosting phase, plan, the controller that applies it, and the result.Glass/the macOS 26NSGlassEffectViewwindow glass with anNSVisualEffectViewfallback.Titlebar/native titlebar backdrop coordinator and the leading-inset reader.Border/the one-pixel chrome borders.Color/separator/compositing/readable-scheme color math.Sidebar/sidebar backdrop material policy plus the persisted preset/material/blend/state options.TerminalSurface/how an individual terminal surface paints its own background.Overlay/where window-level overlays are inserted in the AppKit hierarchy.De-static check (CONVENTIONS section 10). None needed. The three controllers (
WindowBackdropController,WindowGlassEffect,NativeTitlebarBackdropCoordinator) are real instance types with constructor-injected dependencies and aninit, not static-only namespaces. The onlystaticmembers are constantNSUserInterfaceItemIdentifiers, ObjC associated-object keys (a sanctioned AppKit pattern), and value-type factory/derivation methods onTerminalSurfaceBackgroundFillPlan/WindowAppearanceSnapshot(sanctioned by section 9). The lint's namespace-types check is clean.Re-verified after the rework:
scripts/lint-ios-package-conventions.shis clean (zero newlint:allow), andxcodebuild ... -scheme cmux buildreportsBUILD SUCCEEDED.Summary
Sources/ContentView.swiftintoPackages/CmuxAppKitSupportUIunderWindowChrome/.Sources/Windowing/AppWindowChromeComposition.swiftas the app-side composition root forGhosttyApp,WindowBackgroundComposition,NSApp.windows, and compositor blur side effects..github/swift-file-length-budget.tsvforSources/ContentView.swiftfrom 16705 to 16038 lines.Package Boundary
CmuxAppKitSupportUIis the boundary because the extracted domain is AppKit and SwiftUI chrome rendering, not generic window lifecycle. The package now depends onCmuxFoundationfor Ghostty background value types andCmuxWorkspaceWindowfor the existing injectedWindowBackgroundPolicy.Seams
WindowGlassEffectManaginghides native and fallback glass hierarchy operations.WindowBackdropControllerDependenciesinjects compositor blur reset and Ghostty blur application.WindowTerminalAppearanceSnapshotinjects terminal appearance instead of reaching intoGhosttyApp.sharedfrom the package.WindowContentOverlayTargetResolverreceives the glass-effect seam for overlay placement.NativeTitlebarBackdropCoordinatorreceives a fullscreen auxiliary window provider instead of reaching intoNSApp.windows.TitlebarLeadingInsetReaderreceives the debug inset provider from the app.Verification
scripts/lint-ios-package-conventions.shcd Packages/CmuxAppKitSupportUI && swift buildcd Packages/CmuxAppKitSupportUI && swift testxcodebuild -project cmux.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-contentchrome build > /tmp/cmux-contentchrome-build.log 2>&1grep '** BUILD SUCCEEDED **' /tmp/cmux-contentchrome-build.logNotes
This is a principled decomposition: the package owns value objects and coordinators with constructor-injected seams, while concrete app dependencies stay in the app target. No
lint:allowmarkers were added.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted window chrome (titlebar, glass, sidebars, borders) from
ContentView.swiftintoPackages/macOS/CmuxAppKitSupportUI/WindowChrome, addedAppWindowChromeCompositionfor glass/backdrop/overlay seams, and splitAppWindowBackdropControllerDependenciesinto its own file to clarify app-side wiring.Refactors
CmuxAppKitSupportUI/WindowChrome, grouped by concern with a new README.Sources/Windowing/AppWindowChromeComposition.swift; portals now resolve overlay targets and glass via this composition. SplitSources/Windowing/AppWindowBackdropControllerDependencies.swiftinto its own concrete adapter.WindowChromeSeparatorColorwithWindowChromeColorResolver; declaredCmuxAppKitSupportUIdeps onCmuxFoundationandCmuxWorkspaceWindow, and added app aliases to preserve sidebar setting types.Sources/ContentView.swiftsize and updated.github/swift-file-length-budget.tsv(ContentView now 15920 lines).Bug Fixes
WindowBackgroundComposition.policy; overlay install now falls back to the theme frame when glass is absent.CmuxAppKitSupportUIwhere needed, passwindowBackgroundPolicytoWindowGlassSettingsSnapshot.shouldApply, move titlebar inset hit-testing to a package test, and restore a directBonsplitlink incmuxTests.@MainActorwarnings inAppWindowChromeCompositionby moving default evaluations into actor-isolated bodies; added explicit imports to stabilize per-file incremental builds.Written for commit dffbd4e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements