Repository navigation
terminal: propagate NSWindow occlusion to Ghostty surface occlusion - #7621
austinywang wants to merge 2 commits into
Conversation
Ghostty occlusion was driven only by portal/UI visibility; NSWindow occlusion was a documented no-op, so every mounted surface in a fully covered or miniaturized cmux window kept rendering (updateFrame hot stacks in the #7596 audit; relates #7186). Make Ghostty occlusion the AND of two axes combined inside TerminalSurface: UI visibility (existing setOcclusion callers) and a new window axis driven per hosted view by NSWindow.didChangeOcclusionStateNotification, pushed once on window attach and deliberately left unchanged across detach/reparent transients. Calls into ghostty_surface_set_occlusion are deduplicated, and surface (re)creation now replays the current effective occlusion — previously a surface created while hidden rendered as if visible. Part of #7596 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a two-axis surface occlusion state, threads it through ChangesTwo-axis Surface Occlusion Tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NSWindow
participant GhosttyNSView
participant TerminalSurface
participant ghostty_surface_set_occlusion
NSWindow-->>GhosttyNSView: didChangeOcclusionStateNotification
GhosttyNSView->>TerminalSurface: setWindowOcclusionVisible(visible)
TerminalSurface->>TerminalSurface: occlusionState.windowVisible = visible
TerminalSurface->>TerminalSurface: applyOcclusionIfNeeded()
alt effectiveVisible changed
TerminalSurface->>ghostty_surface_set_occlusion: set_occlusion(effectiveVisible)
TerminalSurface->>TerminalSurface: lastAppliedOcclusionVisible = effectiveVisible
end
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ 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 wires AppKit window occlusion into Ghostty surface rendering. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "terminal: seed window occlusion when a s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let self, let window, self.window === window else { return } | ||
| self.terminalSurface?.setWindowOcclusionVisible(window.occlusionState.contains(.visible)) | ||
| } | ||
| terminalSurface?.setWindowOcclusionVisible(window.occlusionState.contains(.visible)) |
There was a problem hiding this comment.
When a GhosttyNSView already belongs to a miniaturized or fully covered window and a TerminalSurface is assigned afterward, this window-axis seed has already run. The new surface keeps windowVisible at its default true until a later occlusion notification, so a surface created in an already-hidden window can keep rendering off-screen.
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!
There was a problem hiding this comment.
Fixed in 00343ea: attachSurface(_:) now seeds the window-occlusion axis from the current window (window.occlusionState.contains(.visible)) when the view is already in a window, so a surface attached after viewDidMoveToWindow ran no longer keeps the default windowVisible = true inside a miniaturized/covered window.
| /// Ghostty should render only when the surface is both visible in the UI | ||
| /// (portal/canvas visibility) and its host window is visible according to | ||
| /// `NSWindow.occlusionState`. | ||
| public struct SurfaceOcclusionState: Equatable, Sendable { |
There was a problem hiding this comment.
This new public Sendable value model is used by nonisolated TerminalSurface methods and main-actor lifecycle code, but it is not declared nonisolated. Under cmux's Swift 6 actor-isolation rule, pure Sendable models should opt out explicitly so later compiler or module isolation changes do not couple this runtime state to the main actor and produce isolation diagnostics in the setter paths.
| public struct SurfaceOcclusionState: Equatable, Sendable { | |
| nonisolated public struct SurfaceOcclusionState: Equatable, Sendable { |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Declining this one: the CmuxTerminal package compiles in plain Swift 6 language mode with no defaultIsolation(MainActor.self) setting, so a top-level public struct here is already nonisolated. Every sibling public Sendable value type in the package (TerminalSurfaceSpawnPolicy, TerminalSurfaceRuntimeFilesystem, TerminalSurfaceRegistryDiagnosticSnapshot) is declared without an explicit nonisolated keyword, so adding it only to SurfaceOcclusionState would deviate from the package's existing convention rather than follow it.
…ow view viewDidMoveToWindow only seeds the window-occlusion axis for the surface attached at that moment. A surface attached later to a view already sitting in a miniaturized or fully covered window kept the default windowVisible state and continued rendering off-screen. Seed the axis from the current window in attachSurface (Greptile P1 on #7621). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 00343ea. Configure here.
| // sitting in an occluded window does not keep rendering off-screen. | ||
| if let window { | ||
| surface.setWindowOcclusionVisible(window.occlusionState.contains(.visible)) | ||
| } |
There was a problem hiding this comment.
Window occlusion seeded after create
Medium Severity
setWindowOcclusionVisible runs after attachToView, which can synchronously createSurface and apply occlusionState while windowVisible still defaults to true. An occluded host window can briefly get ghostty_surface_set_occlusion(true) plus forceRefreshSurface/ghostty_surface_refresh before the window axis is corrected.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 00343ea. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Part of #7596 (memory audit, slice 4). Relates to #7186.
cmux drove
ghostty_surface_set_occlusionfrom portal/UI visibility only (workspace/tab selection); window-level occlusion was a documented no-op (updateOcclusionState(), dead code). Result: with the cmux window fully covered by another app or miniaturized, the mounted workspace's surfaces kept rendering — the audit's 10ssampleshowedrenderer.updateFrame/drawFramehot stacks for surfaces that were not on screen.Design
SurfaceOcclusionState(CmuxTerminal package): two axes,uiVisibleandwindowVisible; Ghostty seesuiVisible && windowVisible.TerminalSurface.setOcclusion(_:)keeps its signature and becomes the UI-axis setter (existing callers — portalsetVisibleInUI, canvas panes — unchanged in semantics since the window axis defaults to visible). NewsetWindowOcclusionVisible(_:)sets the window axis. Both funnel through one deduplicated apply.NSWindow.didChangeOcclusionStateNotificationfor its current window (registered next to the existing screen-change observer, removed on detach/deinit), pushes the current state once on attach, and pushes nothing on detach — reparent transients keep the last window state, preserving the intent of the old no-op ("avoid transient clears during reparenting").Tests
New
SurfaceOcclusionStateTests(CmuxTerminal package, Swift Testing): defaults, full AND truth table, hide/show sequences.swift testinPackages/macOS/CmuxTerminal: 56 tests in 10 suites pass (the runner's XCTest arch preflight exits nonzero in this environment; Swift Testing reports all passing — same artifact as previous package runs). A live-render dedup test against the C seam isn't feasible without new stub instrumentation; the rendering-stops-when-covered behavior itself is runtime/perf work verified by the #7596 re-measurement procedure.python3 scripts/swift_file_length_budget.pypasses with no TSV changes (GhosttyTerminalView.swift 12,506 ≤ 12,511; TerminalSurface.swift 605 ≤ 607; RuntimeLifecycle stays exactly at 655).No user-facing strings → no localization changes.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes when Ghostty stops rendering (visibility/occlusion path) and touches main-thread window notifications; behavior is localized but affects renderer lifecycle and off-screen CPU/GPU use.
Overview
Stops Ghostty from rendering when the cmux window is covered or miniaturized, not only when the portal hides the pane. Previously
ghostty_surface_set_occlusionfollowed UI/portal visibility; window occlusion was intentionally ignored (including a no-opupdateOcclusionState()), so off-screen windows could still drivedrawFramework.Introduces
SurfaceOcclusionStatewithuiVisibleandwindowVisible; Ghostty receivesuiVisible && windowVisible. ExistingsetOcclusion(_:)updates the UI axis; newsetWindowOcclusionVisible(_:)updates the window axis. Both paths dedupe vialastAppliedOcclusionVisiblebefore callingghostty_surface_set_occlusion.GhosttyTerminalViewobservesNSWindow.didChangeOcclusionStateNotification, seeds window visibility on surface attach, and refreshes it when the view enters a window. Detach does not reset the window axis (same reparenting behavior as the old no-op). On runtime surface (re)creation, effective occlusion is replayed alongside focus so surfaces born while hidden do not render until both axes are visible. Occlusion tracking resets on agent-hibernation suspend.Adds
SurfaceOcclusionStateTestsfor defaults, AND logic, and hide/show ordering.Reviewed by Cursor Bugbot for commit 00343ea. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Propagates
NSWindowocclusion to Ghostty surface occlusion so a surface renders only when it’s visible in the UI and the window is visible. Also seeds window visibility when attaching a surface to a view already in a window, so hidden windows don’t keep rendering.SurfaceOcclusionState(uiVisibleANDwindowVisible) inCmuxTerminal;setWindowOcclusionVisible(_:)is new,setOcclusion(_:)remains the UI-axis setter. Calls toghostty_surface_set_occlusionare deduplicated.GhosttyTerminalViewobservesNSWindow.didChangeOcclusionStateNotification, seeds the window axis on window attach and when attaching a surface to an in-window view, and leaves it unchanged during detach/reparent.SurfaceOcclusionStateTestscovering defaults, AND semantics, and hide/show sequences.Written for commit 00343ea. Summary will update on new commits.
Summary by CodeRabbit