Repository navigation
fix: apply background-opacity and background-blur to terminal rendering area (#879) - #1858
Conversation
|
@martinezhermes is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds window blur application and a Metal-backed translucent layer: a new helper applies blur to transparent NSWindows (calling into libghostty), and GhosttyNSView now provides a non-opaque CAMetalLayer for correct translucent rendering. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Greptile SummaryThis PR fixes two root causes that prevented Key changes:
Issues flagged:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant GhosttyNSView
participant GhosttyApp
participant libghostty
Note over GhosttyNSView: View initialisation
AppKit->>GhosttyNSView: makeBackingLayer()
GhosttyNSView-->>AppKit: CAMetalLayer(isOpaque=false, framebufferOnly=false)
AppKit->>GhosttyNSView: setup() → wantsLayer=true
Note over GhosttyNSView: Background/opacity change event
AppKit->>GhosttyNSView: applyWindowBackgroundIfActive()
GhosttyNSView->>GhosttyNSView: effectiveBackgroundColor()
alt opacity < threshold (transparent)
GhosttyNSView->>AppKit: window.isOpaque = false
GhosttyNSView->>GhosttyApp: applyWindowBlurIfNeeded(window)
GhosttyApp->>libghostty: ghostty_config_get(config, &blurRadius, "background-blur", ...)
libghostty-->>GhosttyApp: blurRadius (UInt32)
alt blurRadius > 0
GhosttyApp->>libghostty: ghostty_set_window_background_blur(app, window)
Note over libghostty: Adds NSVisualEffectView ⚠️ no idempotency guard
end
else opacity >= threshold (opaque)
GhosttyNSView->>AppKit: window.isOpaque = true
end
Last reviewed commit: "fix: apply backgroun..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| override func makeBackingLayer() -> CALayer { | ||
| let metalLayer = CAMetalLayer() | ||
| metalLayer.pixelFormat = .bgra8Unorm | ||
| metalLayer.isOpaque = false | ||
| metalLayer.framebufferOnly = false | ||
| return metalLayer | ||
| } |
There was a problem hiding this comment.
framebufferOnly = false should be documented
Setting framebufferOnly = false on a CAMetalLayer disables the Metal driver optimisation that lets it treat drawable textures as write-only. This is measurable in GPU-bound scenarios (especially on integrated graphics). While it may be required for the system compositor to read back the drawable during window blending, leaving it undocumented makes future maintainers likely to revert it assuming it was accidentally left off.
Adding a brief comment explaining the necessity would help:
| override func makeBackingLayer() -> CALayer { | |
| let metalLayer = CAMetalLayer() | |
| metalLayer.pixelFormat = .bgra8Unorm | |
| metalLayer.isOpaque = false | |
| metalLayer.framebufferOnly = false | |
| return metalLayer | |
| } | |
| override func makeBackingLayer() -> CALayer { | |
| let metalLayer = CAMetalLayer() | |
| metalLayer.pixelFormat = .bgra8Unorm | |
| metalLayer.isOpaque = false | |
| // Required for the macOS compositor to composite this layer with | |
| // the blurred/transparent window backdrop; matches SurfaceView behaviour. | |
| metalLayer.framebufferOnly = false | |
| return metalLayer | |
| } |
| func applyWindowBlurIfNeeded(_ window: NSWindow) { | ||
| guard let app = self.app, let config = self.config else { return } | ||
| var blurRadius: UInt32 = 0 | ||
| let key = "background-blur" | ||
| _ = ghostty_config_get(config, &blurRadius, key, UInt(key.lengthOfBytes(using: .utf8))) | ||
| guard blurRadius > 0 else { return } | ||
| ghostty_set_window_background_blur(app, Unmanaged.passUnretained(window).toOpaque()) | ||
| } |
There was a problem hiding this comment.
ghostty_set_window_background_blur called repeatedly without idempotency guard
applyWindowBlurIfNeeded is invoked from both applyBackgroundToKeyWindow() and applyWindowBackgroundIfActive(). Both callers are triggered on routine UI events — tab switches, focus changes, viewDidMoveToWindow, and config reloads — meaning this path can fire many times per session.
The ghostty_set_window_background_blur function is annotated in ghostty.h as a legacy/low-level API ("Don't use these unless you know what you're doing"). If its implementation adds an NSVisualEffectView to the window without first checking whether one already exists, each successive call stacks another view, producing visual artifacts and a steady memory leak.
Consider tracking whether blur has already been applied to a given window (e.g., using a weak NSHashTable<NSWindow> property on GhosttyApp) and skipping the C call if the window is already in the set. This ensures ghostty_set_window_background_blur is called at most once per window instance.
| var blurRadius: UInt32 = 0 | ||
| let key = "background-blur" | ||
| _ = ghostty_config_get(config, &blurRadius, key, UInt(key.lengthOfBytes(using: .utf8))) |
There was a problem hiding this comment.
background-blur config type may not match UInt32
ghostty_config_get resolves the requested config field by matching the byte-size and type-tag of the output pointer against the field's Zig type. If the backing Zig type for background-blur is bool (as it was in earlier Ghostty versions) or a narrower integer such as u8, the runtime type check inside Zig will fail, ghostty_config_get returns false, blurRadius stays 0, and the blur call is never reached — silently breaking the feature.
For comparison, the existing config reads in this file use Swift types that directly mirror the underlying Zig types (e.g., Double for background-opacity → Zig f64, Bool for focus-follows-mouse → Zig bool, CUnsignedInt for bell-features → Zig u32).
Before shipping, please confirm the exact Zig typedef for background-blur in the currently-linked libghostty, and update the Swift variable type accordingly. If the type is bool, the check should use a Bool variable rather than a UInt32.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
2409-2435:⚠️ Potential issue | 🟠 MajorClear blur explicitly when it should be off.
Line 2414 and Line 3952 only hit the blur path while the window is transparent, and Line 2433 exits immediately when
background-bluris0. Since the window blur state is sticky, toggling blur off or returning to an opaque background can leave the old blur attached. Please make the blur update unconditional for background changes and include an explicit disable/reset path when blur should be off.Based on learnings: "CGS window background blur is stateful. Always call cmuxApplyBackgroundBlur(to: NSWindow, radius: Int) on background updates and pass radius 0 when blur should be disabled (opacity >= 1.0 or configured radius == 0). Both apply paths (GhosttyApp.applyBackgroundToKeyWindow and GhosttyNSView.applyWindowBackgroundIfActive) follow this pattern."
Also applies to: 3933-3953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2409 - 2435, applyBackgroundToKeyWindow currently only calls applyWindowBlurIfNeeded in the transparent branch and applyWindowBlurIfNeeded early-returns when the config blur is 0, leaving the OS blur state sticky; update both applyBackgroundToKeyWindow and GhosttyNSView.applyWindowBackgroundIfActive to always invoke the blur update on background changes and change applyWindowBlurIfNeeded to never return early — read the "background-blur" value (ghostty_config_get) and compute an effective radius (use 0 when blur is disabled or when opacity >= 1.0), then always call ghostty_set_window_background_blur / cmuxApplyBackgroundBlur(to:radius:) passing that radius so blur is explicitly applied or cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2409-2435: applyBackgroundToKeyWindow currently only calls
applyWindowBlurIfNeeded in the transparent branch and applyWindowBlurIfNeeded
early-returns when the config blur is 0, leaving the OS blur state sticky;
update both applyBackgroundToKeyWindow and
GhosttyNSView.applyWindowBackgroundIfActive to always invoke the blur update on
background changes and change applyWindowBlurIfNeeded to never return early —
read the "background-blur" value (ghostty_config_get) and compute an effective
radius (use 0 when blur is disabled or when opacity >= 1.0), then always call
ghostty_set_window_background_blur / cmuxApplyBackgroundBlur(to:radius:) passing
that radius so blur is explicitly applied or cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b47948ac-8396-47bd-8f55-ff7624483f86
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
…ng area Two root causes for issue #879: 1. GhosttyNSView was missing makeBackingLayer(), so AppKit provided a generic CALayer as the view's backing layer. libghostty expects (view.layer as? CAMetalLayer) != nil to set up Metal rendering on the existing layer. Without a CAMetalLayer backing layer, the Metal surface defaulted to isOpaque=true, making the terminal area fully opaque regardless of background-opacity config. Fix: override makeBackingLayer() to return a CAMetalLayer with isOpaque=false and bgra8Unorm pixel format (matching standalone Ghostty's SurfaceView behavior). 2. ghostty_set_window_background_blur(app, window) is exposed in ghostty.h but was never called in cmux. Without this call the macOS window never gets the NSVisualEffectView blur backdrop that background-blur requires. Fix: add applyWindowBlurIfNeeded() on GhosttyApp that reads background-blur from config via ghostty_config_get and calls ghostty_set_window_background_blur when the value is non-zero. Called from applyBackgroundToKeyWindow() and applyWindowBackgroundIfActive() whenever the window is made transparent. Fixes #879 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
826ce60 to
1235daf
Compare
|
Thank you for the contribution! |
…d-opacity-blur-879 fix: apply background-opacity and background-blur to terminal rendering area (manaflow-ai#879)
2 participants
Problem
Fixes #879. Two independent bugs prevented
background-opacityandbackground-blurfrom affecting the terminal rendering area.Root causes & fixes
1.
background-opacity— opaque Metal layer (makeBackingLayer)GhosttyNSViewhad nomakeBackingLayer()override, so AppKit supplied a genericCALayeras the backing layer. libghostty expects the view's own layer to be aCAMetalLayer— evidenced by the(view.layer as? CAMetalLayer) != nilguard already in the file — and when it finds one it uses it directly for Metal rendering. Without it, the Metal surface defaulted toisOpaque = true, making the terminal completely opaque regardless of the configuredbackground-opacity.Fix: override
makeBackingLayer()inGhosttyNSViewto return aCAMetalLayerwith:isOpaque = false— allows the compositor to blend the layerpixelFormat = .bgra8Unorm— standard BGRA format with alpha channelframebufferOnly = false— lets the macOS compositor read the drawable when blending translucent/blurred window layers; matches standalone Ghostty'sSurfaceView2.
background-blur—ghostty_set_window_background_blurnever calledghostty_set_window_background_bluris declared inghostty.hbut was never invoked in cmux. Its Zig implementation callsCGSSetWindowBackgroundBlurRadius, a private CoreGraphics compositor API that sets the blur radius on the window's compositor layer — not an NSVisualEffectView. It readsbackground-blurandbackground-opacityfrom the app config internally, and is a no-op when opacity ≥ 1.0 or blur is disabled/zero. Because it is a direct compositor setter, it is idempotent.Fix: add
applyWindowBlurIfNeeded(_ window: NSWindow)onGhosttyAppthat callsghostty_set_window_background_blurunconditionally (the function self-guards). Called fromapplyBackgroundToKeyWindow()andapplyWindowBackgroundIfActive()whenever the window is set transparent — covering both initial window setup and per-surface activation.Reviewer notes
ghostty_config_getread forbackground-blurwas removed from an earlier draft.BackgroundBluris a Zig tagged union whosecval()returnsi16; reading it viaghostty_config_getwith a Swift integer pointer would silently fail. The Zig function reads its own config, so no Swift-side config read is needed.CGSSetWindowBackgroundBlurRadiusis idempotent — repeated calls from focus/tab events do not stack, they overwrite.Test plan
background-opacity = 0.8andbackground-blur = 1in~/Library/Application Support/com.mitchellh.ghostty/configbackground-opacity = 1— confirm terminal returns to fully opaque with no blurSummary by CodeRabbit
New Features
Style