Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Sources/GhosttyTerminalView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3058,6 +3058,9 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {

func applyWindowBackgroundIfActive() {
guard let window else { return }
// Always maintain the surface-level background to prevent blank panes.
// This is safe even when window-level background is skipped (idempotent).
applySurfaceBackground()
let appDelegate = AppDelegate.shared
let owningManager = tabId.flatMap { appDelegate?.tabManagerFor(tabId: $0) }
let owningSelectedTabId = owningManager?.selectedTabId
Expand All @@ -3070,7 +3073,6 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
) else {
return
}
applySurfaceBackground()
let color = effectiveBackgroundColor()
if cmuxShouldUseClearWindowBackground(for: color.alphaComponent) {
window.backgroundColor = cmuxTransparentWindowBaseColor()
Expand Down
7 changes: 6 additions & 1 deletion Sources/TerminalNotificationStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,12 @@ final class TerminalNotificationStore: ObservableObject {
}

if WorkspaceAutoReorderSettings.isEnabled() {
AppDelegate.shared?.tabManager?.moveTabToTop(tabId)
// Defer tab reordering to the next run loop iteration to avoid
// cascading SwiftUI re-renders (tabs + notifications @Published
// changes in the same frame) that can blank the active pane (#914).
DispatchQueue.main.async {
AppDelegate.shared?.tabManager?.moveTabToTop(tabId)
}
}

let notification = TerminalNotification(
Expand Down
116 changes: 116 additions & 0 deletions cmuxTests/GhosttyConfigTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,122 @@ final class WindowBackgroundSelectionGateTests: XCTestCase {
}
}

// MARK: - Issue #914 regression tests

/// Validates that `shouldApplyWindowBackground` returning false does NOT
/// prevent surface background from being maintained. The fix moves
/// `applySurfaceBackground()` before the guard so it always runs.
///
/// These tests verify the decision logic that was previously the sole
/// gate for surface background application. When the guard rejects,
/// the surface background must still be applied (tested via the code
/// change, not a mock — these tests document the rejection scenarios).
final class WindowBackgroundSurfaceBackgroundGuaranteeTests: XCTestCase {
func testShouldApplyRejectsWhenOwningSelectionDiffersButSurfaceMustStayVisible() {
let surfaceTabId = UUID()
let differentSelectedTab = UUID()

// This is the scenario that triggers #914: the owning manager's
// selectedTabId temporarily differs from the surface's tabId
// (e.g., during notification-triggered tab reorder).
let rejected = !GhosttyNSView.shouldApplyWindowBackground(
surfaceTabId: surfaceTabId,
owningManagerExists: true,
owningSelectedTabId: differentSelectedTab,
activeSelectedTabId: surfaceTabId
)
XCTAssertTrue(rejected, "Guard should reject when owning selection differs — this is the #914 trigger condition")
}

func testShouldApplyRejectsWhenActiveSelectionDiffersWithNoOwningManager() {
let surfaceTabId = UUID()
let differentActiveTab = UUID()

let rejected = !GhosttyNSView.shouldApplyWindowBackground(
surfaceTabId: surfaceTabId,
owningManagerExists: false,
owningSelectedTabId: nil,
activeSelectedTabId: differentActiveTab
)
XCTAssertTrue(rejected, "Guard should reject when active selection differs — surface background must still be maintained")
}
}

/// Validates that notification-driven tab reorder is deferred to avoid
/// cascading @Published changes in the same run loop frame (#914).
///
/// Uses source-code inspection (same pattern as
/// `TabManagerNotificationOrderingSourceTests`) to assert that
/// `moveTabToTop` is wrapped in `DispatchQueue.main.async` inside
/// `addNotification`. This avoids singleton state pollution from
/// `TerminalNotificationStore.shared` and provides a meaningful
/// regression guard — unlike a runtime test, this will fail if the
/// async wrapper is removed.
final class NotificationTabReorderDeferralTests: XCTestCase {
func testMoveTabToTopIsDeferredInsideAddNotification() throws {
let projectRoot = findProjectRoot()
let storeURL = projectRoot.appendingPathComponent("Sources/TerminalNotificationStore.swift")
let source = try String(contentsOf: storeURL, encoding: .utf8)
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This test asserts source-code strings instead of runtime behavior, violating the repository test policy and creating brittle regression coverage.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GhosttyConfigTests.swift, line 807:

<comment>This test asserts source-code strings instead of runtime behavior, violating the repository test policy and creating brittle regression coverage.</comment>

<file context>
@@ -792,42 +792,76 @@ final class WindowBackgroundSurfaceBackgroundGuaranteeTests: XCTestCase {
+    func testMoveTabToTopIsDeferredInsideAddNotification() throws {
+        let projectRoot = findProjectRoot()
+        let storeURL = projectRoot.appendingPathComponent("Sources/TerminalNotificationStore.swift")
+        let source = try String(contentsOf: storeURL, encoding: .utf8)
 
-        // Capture the tab count before adding a notification.
</file context>
Fix with Cubic


// Locate the addNotification method body.
guard let methodStart = source.range(of: "func addNotification(tabId:") else {
XCTFail("Failed to locate addNotification(tabId:) in TerminalNotificationStore.swift")
return
}

// Find the next top-level `func` after addNotification to bound the search.
let searchRange = methodStart.upperBound..<source.endIndex
let methodEnd = source.range(of: "\n func ", range: searchRange)?.lowerBound ?? source.endIndex
let methodBody = String(source[methodStart.lowerBound..<methodEnd])

// The critical invariant: moveTabToTop must be called inside
// DispatchQueue.main.async, NOT directly. A synchronous call
// causes two @Published mutations in the same run loop frame
// (tabs reorder + notifications update), triggering a cascading
// SwiftUI re-render that blanks the active pane (#914).
XCTAssertTrue(
methodBody.contains("DispatchQueue.main.async"),
"""
addNotification must defer moveTabToTop via DispatchQueue.main.async \
to prevent cascading @Published changes that blank the active pane (#914).
"""
)
XCTAssertTrue(
methodBody.contains("moveTabToTop"),
"addNotification must call moveTabToTop for auto-reorder behavior."
)

// Verify moveTabToTop appears ONLY inside the async block, not outside it.
// Split on DispatchQueue.main.async and check that moveTabToTop doesn't
// appear in the portion before the async block within the reorder section.
if let reorderStart = methodBody.range(of: "WorkspaceAutoReorderSettings.isEnabled()") {
let reorderSection = String(methodBody[reorderStart.lowerBound..<methodBody.endIndex])
if let asyncStart = reorderSection.range(of: "DispatchQueue.main.async") {
let beforeAsync = String(reorderSection[reorderSection.startIndex..<asyncStart.lowerBound])
XCTAssertFalse(
beforeAsync.contains("moveTabToTop"),
"""
moveTabToTop must NOT be called synchronously before the \
DispatchQueue.main.async block — it must be inside it (#914).
"""
)
}
}
}

private func findProjectRoot() -> URL {
var dir = URL(fileURLWithPath: #file).deletingLastPathComponent().deletingLastPathComponent()
for _ in 0..<10 {
let marker = dir.appendingPathComponent("GhosttyTabs.xcodeproj")
if FileManager.default.fileExists(atPath: marker.path) {
return dir
}
dir = dir.deletingLastPathComponent()
}
return URL(fileURLWithPath: FileManager.default.currentDirectoryPath)
}
Comment on lines +780 to +826
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Test does not verify the claimed deferral behavior.

The test claims to validate that moveTabToTop() is NOT called synchronously during addNotification(), but it doesn't actually verify this:

  1. No observable state is captured before/after the synchronous call to detect if moveTabToTop fired early
  2. The expectation just proves DispatchQueue.main.async executes, which is always true
  3. There's no mock or spy to verify call ordering

The verbose comments (lines 797-806, 810-813, 822-828) describe the intent, but the actual test body doesn't enforce it. Per learnings, tests should verify observable runtime behavior through executable paths.

Consider either:

  • Mocking AppDelegate.shared?.tabManager to capture and verify call timing
  • Using an observable side effect (e.g., a counter or flag) that changes when moveTabToTop is called, then asserting it hasn't changed immediately after addNotification() returns but has changed after the expectation completes

Based on learnings: "Tests must verify observable runtime behavior through executable paths (unit/integration/e2e/CLI), not implementation shape".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/GhosttyConfigTests.swift` around lines 795 - 831, The test
TestMoveTabToTopIsNotCalledSynchronouslyDuringAddNotification doesn't actually
assert that moveTabToTop was deferred; update
NotificationTabReorderDeferralTests.testMoveTabToTopIsNotCalledSynchronouslyDuringAddNotification
to observe a concrete side-effect: install a spy or observable flag on the tab
manager (e.g., stub AppDelegate.shared?.tabManager or inject a test TabManager)
that flips a boolean or increments a counter when moveTabToTop() is called,
capture that value before calling
TerminalNotificationStore.addNotification(...), assert it is unchanged
immediately after addNotification returns, then schedule the existing
DispatchQueue.main.async expectation and assert the flag/counter changed after
the expectation completes; reference TerminalNotificationStore.addNotification
and moveTabToTop to locate the code under test.

}

final class NotificationBurstCoalescerTests: XCTestCase {
func testSignalsInSameBurstFlushOnce() {
let coalescer = NotificationBurstCoalescer(delay: 0.01)
Expand Down