Repository navigation
Support macOS clear glass background blur - #3313
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:
📝 WalkthroughWalkthroughAdds Ghostty background-blur support through config, snapshots, and GhosttyApp; refactors window-glass into a GlassBackgroundView with a style parameter and reuse semantics; updates ContentView titlebar/backdrop handling and file-drop overlay idempotency; introduces refreshID-based deduping in WindowAccessor and routes portal installation to WindowGlassEffect helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as GhosttyConfig
participant App as GhosttyApp
participant Snapshot as WindowAppearanceSnapshot
participant Glass as WindowGlassEffect
participant Window as NSWindow
Config->>App: provide backgroundBlur
App->>Snapshot: build appearance (terminalBackgroundBlur, tint)
Snapshot->>App: resolved glass settings (style, shouldApply)
App->>Glass: apply(to: window, tintColor, style)
Glass->>Window: install/update GlassRoot/GlassBackgroundView and foregroundContainer
Window-->>Glass: report contentView/root state
Glass-->>App: applied/changed (indicates whether root changed)
App->>PortalRegistries: schedule external geometry sync if changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds
Confidence Score: 3/5The One P1 (glass-style forcing transparent hosting on macOS < 26 regardless of availability) pulls the score below the P1 ceiling of 4. The remaining P2 findings (IMP nil-guard, Int16 width assumption, missing regular-variant test) are non-blocking but add minor risk. Sources/Windowing/WindowAppearanceSnapshot.swift — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Ghostty config file\nbackground-blur = macos-glass-clear"] --> B["GhosttyConfig.parseBackgroundBlur()\n→ .macosGlassClear"]
A2["C API ghostty_config_get\n(Int16 value: -2)"] --> B2["GhosttyBackgroundBlur.init(cValue:)\n→ .macosGlassClear"]
B --> C["GhosttyApp.defaultBackgroundBlur\n(stored, change notified)"]
B2 --> C
C --> D["WindowAppearanceSnapshot.current()\nterminalBackgroundBlur = .macosGlassClear"]
D --> E{"terminalBackgroundBlur\n.isMacOSGlassStyle?"}
E -- yes --> F["terminalBackdropPolicy() → .clear\n(clears normal root backdrop)"]
E -- yes --> G["WindowGlassSettingsSnapshot\n.shouldApply() → true ⚠️ ignores glassEffectAvailable"]
E -- no --> H["Normal ghosttyTerminalBackdrop policy"]
G --> I{"NSGlassEffectView\navailable? (macOS 26+)"}
I -- yes --> J["WindowGlassEffect.apply()\nstyle: .clear (rawValue 1)\nunsafeBitCast IMP → setStyle:"]
I -- no --> K["NSVisualEffectView fallback\n(no style distinction)"]
F --> L["Window: backgroundColor = clear\nisOpaque = false\nskip applyWindowBlurIfNeeded"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 215-219: Add a second assertion exercising the other public blur
value: in the testParseBackgroundBlurReadsMacOSGlassClear test (or a new
adjacent test), call config.parse("background-blur = macos-glass-regular") on a
fresh GhosttyConfig and assert that config.backgroundBlur == .macosGlassRegular
so both macos-glass-clear and macos-glass-regular parser paths are covered.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3870-3880: The compositing blur state can remain stale because
applyWindowBlurIfNeeded(window) is skipped when
defaultBackgroundBlur.isMacOSGlassStyle; always invoke the blur updater on every
background-application path (including inside cmuxShouldUseClearWindowBackground
handling) so transitions/disable are applied and stale CGS blur radius is reset
— ensure applyWindowBlurIfNeeded(window) is called even for macOS glass-style
branches and when blur should be disabled pass a radius of 0; update the other
similar branch (the code around the other occurrence near the second location)
the same way and keep logBackground calls as-is.
In `@Sources/WorkspaceContentView.swift`:
- Line 567: The refresh logic currently gating calls to
applyWindowBackgroundIfActive() only on background color/opacity changes must
also consider config.backgroundBlur so blur-only updates run; update the
comparison/dirty-checks (the branches around where
applyWindowBackgroundIfActive() is invoked) to include backgroundBlur in the
equality/changed set and always call applyWindowBackgroundIfActive() when blur
changed, ensuring applyWindowBackgroundIfActive() receives a radius of 0 to
explicitly disable blur when appropriate (instead of skipping the update).
Locate references to applyWindowBackgroundIfActive(), config.backgroundBlur, and
the surrounding titlebar/window refresh logic (the conditional blocks around the
existing background color/opacity checks) and adjust them so blur changes cannot
be skipped and blur is explicitly applied or removed every update.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10a9bc3d-6b6a-4649-96d3-d731520677cc
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftSources/WorkspaceContentView.swiftcmuxTests/GhosttyConfigTests.swift
4a80c0c to
f8ad970
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/Windowing/WindowGlassEffect.swift (1)
7-16: ⚡ Quick winReplace the raw glass-style integers with SDK constants if they're available.
Hard-coding
0/1makes the clear/regular mapping easy to invert silently if the native enum ordering differs from what this code assumes. If the macOS 26 SDK exposes the typed style enum, prefer that and verify both modes render distinctly on-device.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Windowing/WindowGlassEffect.swift` around lines 7 - 16, The Style enum currently returns hard-coded integers in fileprivate var rawNSGlassEffectViewStyle which can break if native ordering changes; update rawNSGlassEffectViewStyle to return the platform SDK's typed constants or native enum cases (for example use the macOS SDK's NSGlassEffectView/NSVisualEffectView style constants or map from the native enum's .regular/.clear cases) instead of 0/1, using the native enum's cases or rawValue to drive the mapping in Style (keep the mapping inside Style), and verify on-device that both .regular and .clear produce distinct visuals.Sources/Windowing/WindowAppearanceSnapshot.swift (1)
169-170: ⚡ Quick winDon't force
.regularon the legacy window-glass path.
stylenow resolves to.regularfor.disabledand.radius(_), so existing callers that enable window glass will start explicitly mutating the native style too. I'd keep this optional and only return a concrete style for the newmacos-glass-*modes, then verify the pre-PR bg-glass appearance stays unchanged.Suggested change
- var style: WindowGlassEffect.Style { - terminalBackgroundBlur.windowGlassStyle ?? .regular + var style: WindowGlassEffect.Style? { + terminalBackgroundBlur.windowGlassStyle }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Windowing/WindowAppearanceSnapshot.swift` around lines 169 - 170, The computed property WindowAppearanceSnapshot.var style should not force a fallback of .regular for legacy window-glass cases; change the logic so style returns terminalBackgroundBlur.windowGlassStyle as an Optional (nil for legacy/disabled/radius(_) cases) and only map to a concrete WindowGlassEffect.Style for the new macos-glass-* modes, ensuring callers that rely on the old bg-glass appearance continue to receive nil and thus preserve native behavior instead of being forced to .regular.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 485-500: The parseBackgroundBlur helper currently maps malformed
tokens to .disabled which can overwrite a prior valid setting; change
parseBackgroundBlur to return an optional GhosttyBackgroundBlur? (i.e.
GhosttyBackgroundBlur?) and return nil for invalid tokens so callers can ignore
malformed values; keep explicit mappings for "false" -> .disabled, "true" ->
.radius(20), "macos-glass-regular" -> .macosGlassRegular, "macos-glass-clear" ->
.macosGlassClear, and only return .radius(radius) when
parseIntegerLiteral(value) yields a valid radius (1...UInt8.max), otherwise
return nil. Ensure callers that consume parseBackgroundBlur handle the optional
by keeping the existing value when nil.
---
Nitpick comments:
In `@Sources/Windowing/WindowAppearanceSnapshot.swift`:
- Around line 169-170: The computed property WindowAppearanceSnapshot.var style
should not force a fallback of .regular for legacy window-glass cases; change
the logic so style returns terminalBackgroundBlur.windowGlassStyle as an
Optional (nil for legacy/disabled/radius(_) cases) and only map to a concrete
WindowGlassEffect.Style for the new macos-glass-* modes, ensuring callers that
rely on the old bg-glass appearance continue to receive nil and thus preserve
native behavior instead of being forced to .regular.
In `@Sources/Windowing/WindowGlassEffect.swift`:
- Around line 7-16: The Style enum currently returns hard-coded integers in
fileprivate var rawNSGlassEffectViewStyle which can break if native ordering
changes; update rawNSGlassEffectViewStyle to return the platform SDK's typed
constants or native enum cases (for example use the macOS SDK's
NSGlassEffectView/NSVisualEffectView style constants or map from the native
enum's .regular/.clear cases) instead of 0/1, using the native enum's cases or
rawValue to drive the mapping in Style (keep the mapping inside Style), and
verify on-device that both .regular and .clear produce distinct visuals.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9104d080-486b-4add-afd6-8f5512cd7119
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftSources/WorkspaceContentView.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/WindowAndDragTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/WorkspaceContentView.swift
- Sources/GhosttyTerminalView.swift
- cmuxTests/GhosttyConfigTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8ad970244
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f8ad970 to
9faf327
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/WorkspaceContentView.swift (1)
631-637:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlur-only updates still skip titlebar/window background refresh paths.
config.backgroundBluris now part of the signature, but blur-only changes still won’t triggeronThemeRefreshRequestorapplyWindowBackgroundIfActive()because these flags ignore blur deltas. Please include blur in both refresh predicates.💡 Suggested patch
let configChanged = previousSignature != nextSignature let backgroundChanged = previousBackgroundHex != next.backgroundColor.hexString() let opacityChanged = abs(config.backgroundOpacity - next.backgroundOpacity) > 0.0001 + let blurChanged = config.backgroundBlur != next.backgroundBlur let shouldForceInitialApply = forceInitialApply || reason == "onAppear" - let shouldRequestTitlebarRefresh = backgroundChanged || opacityChanged || shouldForceInitialApply + let shouldRequestTitlebarRefresh = + backgroundChanged || opacityChanged || blurChanged || shouldForceInitialApply let shouldApplyChrome = configChanged || shouldForceInitialApply - let shouldRefreshWindowBackground = backgroundChanged || opacityChanged || shouldForceInitialApply + let shouldRefreshWindowBackground = + backgroundChanged || opacityChanged || blurChanged || shouldForceInitialApplyBased on learnings: “CGS window background blur is stateful… always call cmuxApplyBackgroundBlur(...), passing radius 0 when blur should be disabled.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 631 - 637, Add a blur-delta boolean and include it in the titlebar/window refresh predicates: compute something like let blurChanged = config.backgroundBlur != next.backgroundBlur (or compare previousBackgroundBlur and next.backgroundBlur if those exist) and then update shouldRequestTitlebarRefresh and shouldRefreshWindowBackground to include blurChanged (i.e. let shouldRequestTitlebarRefresh = backgroundChanged || opacityChanged || blurChanged || shouldForceInitialApply and similarly for shouldRefreshWindowBackground) so blur-only changes trigger onThemeRefreshRequest/applyWindowBackgroundIfActive().Sources/GhosttyConfig.swift (1)
420-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMalformed
background-blurvalues currently wipe prior valid config.Invalid tokens are converted to
.disabledand assigned immediately, so a typo in a later config/include can silently reset a previously valid blur mode instead of being ignored.Suggested fix
- case "background-blur": - backgroundBlur = Self.parseBackgroundBlur(value) + case "background-blur": + if let blur = Self.parseBackgroundBlur(value) { + backgroundBlur = blur + } - private static func parseBackgroundBlur(_ value: String) -> GhosttyBackgroundBlur { + private static func parseBackgroundBlur(_ value: String) -> GhosttyBackgroundBlur? { switch value { case "false": - return .disabled + return .disabled case "true": - return .radius(20) + return .radius(20) case "macos-glass-regular": - return .macosGlassRegular + return .macosGlassRegular case "macos-glass-clear": - return .macosGlassClear + return .macosGlassClear default: guard let radius = parseIntegerLiteral(value), radius > 0, radius <= Int(UInt8.max) else { - return .disabled + return nil } return .radius(radius) } }Also applies to: 485-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 420 - 421, The current assignment backgroundBlur = Self.parseBackgroundBlur(value) overwrites a prior valid setting because parseBackgroundBlur converts malformed tokens to .disabled; change the parsing/assignment so invalid inputs are ignored instead of assigned: update Self.parseBackgroundBlur to return an optional (e.g. BackgroundBlur?) or a Result and have the caller check the return before assigning (only assign backgroundBlur when the parse result is non-nil / successful), and apply the same guard/conditional assignment pattern to the other parsing site that uses Self.parseBackgroundBlur in the file (the similar block around the later background blur handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 420-421: The current assignment backgroundBlur =
Self.parseBackgroundBlur(value) overwrites a prior valid setting because
parseBackgroundBlur converts malformed tokens to .disabled; change the
parsing/assignment so invalid inputs are ignored instead of assigned: update
Self.parseBackgroundBlur to return an optional (e.g. BackgroundBlur?) or a
Result and have the caller check the return before assigning (only assign
backgroundBlur when the parse result is non-nil / successful), and apply the
same guard/conditional assignment pattern to the other parsing site that uses
Self.parseBackgroundBlur in the file (the similar block around the later
background blur handling).
In `@Sources/WorkspaceContentView.swift`:
- Around line 631-637: Add a blur-delta boolean and include it in the
titlebar/window refresh predicates: compute something like let blurChanged =
config.backgroundBlur != next.backgroundBlur (or compare previousBackgroundBlur
and next.backgroundBlur if those exist) and then update
shouldRequestTitlebarRefresh and shouldRefreshWindowBackground to include
blurChanged (i.e. let shouldRequestTitlebarRefresh = backgroundChanged ||
opacityChanged || blurChanged || shouldForceInitialApply and similarly for
shouldRefreshWindowBackground) so blur-only changes trigger
onThemeRefreshRequest/applyWindowBackgroundIfActive().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec43e873-82f1-4a53-bdb9-ebebc482ae78
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftSources/WorkspaceContentView.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/WindowAndDragTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Windowing/WindowGlassEffect.swift
9faf327 to
204b672
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
Sources/GhosttyConfig.swift (1)
420-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the existing blur value on parse failures.
The new parser still maps every unrecognized
background-blurtoken to.disabled, so a typo in a later config can silently clobber an earlier valid blur mode. Keep the prior value when parsing fails instead of resetting it here.💡 Suggested fix
case "background-blur": - backgroundBlur = Self.parseBackgroundBlur(value) + if let blur = Self.parseBackgroundBlur(value) { + backgroundBlur = blur + } @@ - private static func parseBackgroundBlur(_ value: String) -> GhosttyBackgroundBlur { + private static func parseBackgroundBlur(_ value: String) -> GhosttyBackgroundBlur? { switch value { case "false": return .disabled case "true": return .radius(20) case "macos-glass-regular": return .macosGlassRegular case "macos-glass-clear": return .macosGlassClear default: guard let radius = parseIntegerLiteral(value), radius > 0, radius <= Int(UInt8.max) else { - return .disabled + return nil } return .radius(radius) } }Based on learnings: malformed
background-blurvalues should be ignored so earlier valid settings survive.Also applies to: 485-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 420 - 421, The current parsing in the "background-blur" case unconditionally assigns backgroundBlur = Self.parseBackgroundBlur(value), which treats unknown/malformed tokens as .disabled and can clobber a previously valid value; change this to only update backgroundBlur when the parser returns a valid result (i.e., non-nil or a success case) and otherwise leave backgroundBlur unchanged. Locate the "background-blur" switch case and adjust the logic around Self.parseBackgroundBlur(value) so it preserves the existing backgroundBlur on parse failure (apply the same defensive check to the other parse sites in the nearby block at lines ~485–501 that use the same parser).Sources/WorkspaceContentView.swift (1)
563-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlur-only changes still skip the refresh path.
backgroundBlurnow affectsconfigChanged, butshouldRequestTitlebarRefreshandshouldRefreshWindowBackgroundstill ignore it. A blur-only config change can therefore updateconfigwithout reapplying the window blur/glass state.💡 Suggested fix
let nextUsesHostLayerBackground = GhosttyApp.shared.usesHostLayerBackground let nextSignature = Self.ghosttyAppearanceSignature( next, usesHostLayerBackground: nextUsesHostLayerBackground ) + let blurChanged = config.backgroundBlur != next.backgroundBlur let eventLabel = backgroundEventId.map(String.init) ?? "nil" let sourceLabel = backgroundSource ?? "nil" let payloadLabel = notificationPayloadHex ?? "nil" let configChanged = previousSignature != nextSignature let backgroundChanged = previousBackgroundHex != next.backgroundColor.hexString() let opacityChanged = abs(config.backgroundOpacity - next.backgroundOpacity) > 0.0001 let shouldForceInitialApply = forceInitialApply || reason == "onAppear" - let shouldRequestTitlebarRefresh = backgroundChanged || opacityChanged || shouldForceInitialApply + let shouldRequestTitlebarRefresh = + backgroundChanged || opacityChanged || blurChanged || shouldForceInitialApply let shouldApplyChrome = configChanged || shouldForceInitialApply - let shouldRefreshWindowBackground = backgroundChanged || opacityChanged || shouldForceInitialApply + let shouldRefreshWindowBackground = + backgroundChanged || opacityChanged || blurChanged || shouldForceInitialApplyBased on learnings: CGS window background blur is stateful, so background updates must explicitly reapply or clear the blur radius.
Also applies to: 631-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 563 - 573, The change to include backgroundBlur in the appearance signature means blur-only updates can now change config but won't trigger UI refresh; update the decision logic in shouldRequestTitlebarRefresh and shouldRefreshWindowBackground to consider changes to config.backgroundBlur (compare old vs new blur) so blur-only diffs return true, and when a blur change is detected ensure the window blur state is explicitly reapplied/cleared (call the existing window blur/ glass reapply routine used elsewhere) so CGS receives the new radius; check the related configChanged handling that uses ghosttyAppearanceSignature and mirror this blur-aware refresh behavior there as well.cmuxTests/GhosttyConfigTests.swift (1)
215-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover
macos-glass-regulartoo.This still only exercises
macos-glass-clear; please add the sibling parse assertion here or in an adjacent test so the new public blur value stays covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 215 - 219, Add coverage for the "macos-glass-regular" variant: create either a new test (e.g., testParseBackgroundBlurReadsMacOSGlassRegular) or extend testParseBackgroundBlurReadsMacOSGlassClear to parse "background-blur = macos-glass-regular" on a GhosttyConfig instance and assert the parsed value equals the enum case (config.backgroundBlur == .macosGlassRegular) so the public blur value is exercised.Sources/GhosttyTerminalView.swift (1)
3870-3888:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't gate the blur updater out of the glass path.
applyWindowBlurIfNeeded(_:)is stateful, so skipping it whendefaultBackgroundBlur.isMacOSGlassStylecan leave the previous CGS radius in place when users toggle between radius blur and native glass. Please still drive the reset path on every background application and let the helper clear the compositor state instead of short-circuiting here. Based on learnings: CGS window background blur is stateful. Always callcmuxApplyBackgroundBlur(to: NSWindow, radius: Int)on background updates and pass radius 0 when blur should be disabled (opacity >= 1.0 or configured radius == 0).Also applies to: 6041-6065
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3870 - 3888, The blur reset/update is being skipped when defaultBackgroundBlur.isMacOSGlassStyle, which leaves previous CGS blur state intact; remove the short-circuit and always invoke the blur updater so the compositor can be reset—call applyWindowBlurIfNeeded(window) (or directly cmuxApplyBackgroundBlur(to:radius:) with radius 0 when blur should be disabled) in both branches after setting window.backgroundColor/isOpaque, ensuring you pass radius 0 for fully opaque or configured-radius==0 cases; keep backgroundLogEnabled/logBackground behavior as-is but ensure the blur helper runs on every background application (also apply the same change in the other occurrence mentioned).
🧹 Nitpick comments (1)
Sources/Windowing/WindowGlassEffect.swift (1)
95-109: ⚡ Quick winMake
style == nilsemantics explicit to prevent stale native glass style.On a reused
GlassBackgroundView,style: nilskipssetStyle:. That means a prior.clearstyle can persist across later re-applies unless the view is recreated. Consider resolving style explicitly inapply(e.g., default.regular) and reserving “preserve current style” forupdateTintonly.Proposed direction
- func configure( + func configure( tintColor: NSColor?, style: Style?, cornerRadius: CGFloat?, - isKeyWindow: Bool + isKeyWindow: Bool, + preserveExistingStyleWhenNil: Bool = false ) { + let resolvedStyle: Style? = { + if preserveExistingStyleWhenNil { return style } + return style ?? .regular + }() effectView.layer?.cornerRadius = cornerRadius ?? 0 if usesNativeGlass { updateNativeGlassConfiguration( on: effectView, color: tintColor, - style: style, + style: resolvedStyle, cornerRadius: cornerRadius ) updateInactiveTintOverlay(tintColor: tintColor, isKeyWindow: isKeyWindow) } else if let tintColor { ... } }- glassView.configure( + glassView.configure( tintColor: color, style: nil, cornerRadius: windowCornerRadius(for: window), - isKeyWindow: window.isKeyWindow + isKeyWindow: window.isKeyWindow, + preserveExistingStyleWhenNil: true )Also applies to: 190-198, 222-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Windowing/WindowGlassEffect.swift` around lines 95 - 109, The configure/apply path currently treats style == nil as "do nothing", letting a previous native glass style persist; change it so configure/apply resolves nil to an explicit default (e.g., .regular) before calling updateNativeGlassConfiguration or setStyle:, ensuring updateNativeGlassConfiguration(on:effectView, color:tintColor, style:resolvedStyle, cornerRadius:cornerRadius) always receives a non-nil Style; reserve the "preserve existing style" semantics for updateTint (or a new updateTint(preserveStyle: Bool)) so that updateTint can skip changing style when preservation is intended, while configure/apply always sets an explicit style on GlassBackgroundView via setStyle:/updateNativeGlassConfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 215-219: Add coverage for the "macos-glass-regular" variant:
create either a new test (e.g., testParseBackgroundBlurReadsMacOSGlassRegular)
or extend testParseBackgroundBlurReadsMacOSGlassClear to parse "background-blur
= macos-glass-regular" on a GhosttyConfig instance and assert the parsed value
equals the enum case (config.backgroundBlur == .macosGlassRegular) so the public
blur value is exercised.
In `@Sources/GhosttyConfig.swift`:
- Around line 420-421: The current parsing in the "background-blur" case
unconditionally assigns backgroundBlur = Self.parseBackgroundBlur(value), which
treats unknown/malformed tokens as .disabled and can clobber a previously valid
value; change this to only update backgroundBlur when the parser returns a valid
result (i.e., non-nil or a success case) and otherwise leave backgroundBlur
unchanged. Locate the "background-blur" switch case and adjust the logic around
Self.parseBackgroundBlur(value) so it preserves the existing backgroundBlur on
parse failure (apply the same defensive check to the other parse sites in the
nearby block at lines ~485–501 that use the same parser).
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3870-3888: The blur reset/update is being skipped when
defaultBackgroundBlur.isMacOSGlassStyle, which leaves previous CGS blur state
intact; remove the short-circuit and always invoke the blur updater so the
compositor can be reset—call applyWindowBlurIfNeeded(window) (or directly
cmuxApplyBackgroundBlur(to:radius:) with radius 0 when blur should be disabled)
in both branches after setting window.backgroundColor/isOpaque, ensuring you
pass radius 0 for fully opaque or configured-radius==0 cases; keep
backgroundLogEnabled/logBackground behavior as-is but ensure the blur helper
runs on every background application (also apply the same change in the other
occurrence mentioned).
In `@Sources/WorkspaceContentView.swift`:
- Around line 563-573: The change to include backgroundBlur in the appearance
signature means blur-only updates can now change config but won't trigger UI
refresh; update the decision logic in shouldRequestTitlebarRefresh and
shouldRefreshWindowBackground to consider changes to config.backgroundBlur
(compare old vs new blur) so blur-only diffs return true, and when a blur change
is detected ensure the window blur state is explicitly reapplied/cleared (call
the existing window blur/ glass reapply routine used elsewhere) so CGS receives
the new radius; check the related configChanged handling that uses
ghosttyAppearanceSignature and mirror this blur-aware refresh behavior there as
well.
---
Nitpick comments:
In `@Sources/Windowing/WindowGlassEffect.swift`:
- Around line 95-109: The configure/apply path currently treats style == nil as
"do nothing", letting a previous native glass style persist; change it so
configure/apply resolves nil to an explicit default (e.g., .regular) before
calling updateNativeGlassConfiguration or setStyle:, ensuring
updateNativeGlassConfiguration(on:effectView, color:tintColor,
style:resolvedStyle, cornerRadius:cornerRadius) always receives a non-nil Style;
reserve the "preserve existing style" semantics for updateTint (or a new
updateTint(preserveStyle: Bool)) so that updateTint can skip changing style when
preservation is intended, while configure/apply always sets an explicit style on
GlassBackgroundView via setStyle:/updateNativeGlassConfiguration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4f2f19c-cf7c-470f-b047-e35a82b20be5
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftSources/WorkspaceContentView.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/WindowAndDragTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Windowing/WindowAppearanceSnapshot.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 204b67244a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f3c1443b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/WindowAndDragTests.swift (1)
41-43: ⚡ Quick winAssert portal-installation cleanup on remove.
This test proves the portal installation target exists in the applied state on Line 36, but it never checks that
WindowGlassEffect.remove(from:)clears that association. Adding the symmetricXCTAssertNil(WindowGlassEffect.portalInstallationTarget(for: window))here would catch stale portal-host targets leaking across glass lifecycle transitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WindowAndDragTests.swift` around lines 41 - 43, The test currently asserts that foregroundContainer and originalContentView are cleared and that no glass background remains after removing glass, but it omits verifying the portal installation target is cleared; update the post-removal assertions in the test to also call XCTAssertNil(WindowGlassEffect.portalInstallationTarget(for: window)) to ensure WindowGlassEffect.remove(from:) clears the portal-installation association and prevents leaking stale portal-host targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/WindowAndDragTests.swift`:
- Around line 41-43: The test currently asserts that foregroundContainer and
originalContentView are cleared and that no glass background remains after
removing glass, but it omits verifying the portal installation target is
cleared; update the post-removal assertions in the test to also call
XCTAssertNil(WindowGlassEffect.portalInstallationTarget(for: window)) to ensure
WindowGlassEffect.remove(from:) clears the portal-installation association and
prevents leaking stale portal-host targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e854641-1998-4fb5-8413-27ab5e76f3bc
📒 Files selected for processing (7)
Sources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/TerminalWindowPortal.swiftSources/WindowAccessor.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftcmuxTests/WindowAndDragTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 582c4f1fab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This branch was successfully deployed
1 participant
Summary
background-blur = macos-glass-clearandmacos-glass-regularin cmux window appearance.Testing
./scripts/reload.sh --tag bgclrpassed.Issues
background-blur = macos-glass-clearin cmuxSummary by cubic
Adds native macOS glass background blur for terminal windows, including a clear glass style, applied at the window root for safe layering. Glass forces transparent hosting at any opacity, tints from the terminal background, clears native titlebar fills, and updates tint when the window gains/loses focus; fulfills the task to support
background-blur = macos-glass-clear.New Features
background-blur(macos-glass-clear,macos-glass-regular,true/radius); snapshots/signatures include blur/style to drive refresh.regular/clear; fall back toNSVisualEffectView; treat glass as transparent even at 1.0; add inactive tint overlay that reacts to key window state; clear titlebar backgrounds only when glass is active.WindowAccessoraccepts arefreshID;appearance.appKitWindowMutationIDis used to re-run window setup on appearance changes.Bug Fixes
WindowAccessorrefresh dedupe, parsing, snapshot decisions, and transparency outcomes.Written for commit 7914f4e. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests