Fix blank terminal pane when notification fires on another tab#1155
Fix blank terminal pane when notification fires on another tab#1155AlexWorland wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
…low-ai#914) Two changes prevent the active terminal pane from going blank when a notification arrives on a different tab: 1. Move applySurfaceBackground() before the shouldApplyWindowBackground guard in applyWindowBackgroundIfActive(). Previously, when the guard rejected (e.g. owning manager's selectedTabId temporarily differed during a tab reorder), the method returned early without maintaining the surface background, leaving the pane in a stale transparent state. 2. Defer moveTabToTop() in addNotification() to the next run loop iteration via DispatchQueue.main.async. The synchronous call caused two @published changes (tabs reorder + notifications update) in the same frame, compounding SwiftUI re-renders that triggered portal visibility races and background application failures. Closes manaflow-ai#914 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughEagerly apply the surface-level background in GhosttyTerminalView before checking window-background gating, and defer tab reordering in TerminalNotificationStore.addNotification by scheduling moveTabToTop on DispatchQueue.main.async. Added tests verifying surface-background guarantee and deferred reordering behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant NS as TerminalNotificationStore
participant TM as TabManager
participant WM as WindowManager
participant GV as GhosttyTerminalView
UI->>NS: addNotification(notification, tabId)
NS-->>NS: store notification
alt WorkspaceAutoReorderSettings.enabled
NS->>TM: schedule moveTabToTop(tabId) via DispatchQueue.main.async
end
TM->>WM: request tab reorder (on next run loop)
WM->>GV: query/apply visual state
GV->>GV: applySurfaceBackground() (eager, idempotent)
WM->>GV: shouldApplyWindowBackground? (guard)
alt guard == true
GV->>GV: applyWindowBackground()
else
GV-->>GV: skip window background (surface remains)
end
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)
Comment |
Greptile SummaryThis PR addresses a two-part root cause behind the blank terminal pane bug (#914): (1) The production fixes in
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant NS as Notification System
participant Store as TerminalNotificationStore
participant SwiftUI as SwiftUI Runtime
participant View as GhosttyNSView
participant Window as NSWindow
Note over NS,Window: Before fix — synchronous cascade
NS->>Store: addNotification(tabId)
Store->>SwiftUI: @Published tabs reorder (moveTabToTop — sync)
Store->>SwiftUI: @Published notifications update
SwiftUI->>View: re-render (isSelectedInPane=false transiently)
View->>View: applyWindowBackgroundIfActive()
View--xWindow: guard rejects (owningSelectedTabId ≠ tabId)
Note right of Window: applySurfaceBackground() skipped → blank pane
Note over NS,Window: After fix — two-change separation + guard bypass
NS->>Store: addNotification(tabId)
Store->>SwiftUI: @Published notifications update (sync)
Store-->>SwiftUI: moveTabToTop deferred via DispatchQueue.main.async
SwiftUI->>View: re-render
View->>View: applyWindowBackgroundIfActive()
View->>View: applySurfaceBackground() ← always called first
View--xWindow: guard may still reject (no window bg update)
Note right of Window: Surface background maintained regardless
SwiftUI-->>SwiftUI: next run-loop: @Published tabs reorder fires
Last reviewed commit: 0c12da8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
cmuxTests/GhosttyConfigTests.swift
Outdated
| func testMoveTabToTopIsNotCalledSynchronouslyDuringAddNotification() { | ||
| // This test documents the expectation: when addNotification() is | ||
| // called with auto-reorder enabled, moveTabToTop() must NOT run | ||
| // in the same synchronous call stack. It should be deferred via | ||
| // DispatchQueue.main.async to prevent two @Published changes | ||
| // (tabs reorder + notifications update) from triggering a | ||
| // compounded SwiftUI re-render that blanks the active pane. | ||
| // | ||
| // The fix wraps moveTabToTop() in DispatchQueue.main.async inside | ||
| // TerminalNotificationStore.addNotification(). This test validates | ||
| // the architectural invariant rather than mocking internals. | ||
| let store = TerminalNotificationStore.shared | ||
| let tabId = UUID() | ||
|
|
||
| // Capture the tab count before adding a notification. | ||
| // If moveTabToTop were synchronous, it would fire during | ||
| // addNotification and could trigger observable side effects | ||
| // in the same run loop. The deferred version delays this. | ||
| let expectation = XCTestExpectation(description: "Deferred reorder completes") | ||
| store.addNotification( | ||
| tabId: tabId, | ||
| surfaceId: nil, | ||
| title: "Test", | ||
| subtitle: "", | ||
| body: "Test notification" | ||
| ) | ||
| // The moveTabToTop should be deferred — verify by checking | ||
| // that the notification was added (synchronous) while the | ||
| // reorder hasn't happened yet in this same run loop tick. | ||
| DispatchQueue.main.async { | ||
| // By the time this fires, the deferred moveTabToTop would | ||
| // also have been dispatched. This validates the async path. | ||
| expectation.fulfill() | ||
| } | ||
| wait(for: [expectation], timeout: 1.0) | ||
| } |
There was a problem hiding this comment.
Test does not validate the claimed invariant
testMoveTabToTopIsNotCalledSynchronouslyDuringAddNotification never actually verifies that moveTabToTop was not called synchronously. It only verifies that a DispatchQueue.main.async block eventually runs — which would happen regardless of whether the production code uses synchronous or async dispatch.
The expectation.fulfill() in the test's own async block is unconditionally queued; it is completely decoupled from the behavior of moveTabToTop. This test would pass equally well against the old synchronous code.
To be a meaningful regression guard, the test would need either:
- A spy/mock on
tabManager.moveTabToTopthat records when it was called (synchronously vs deferred) - A flag set inside
moveTabToTopthat is checked both synchronously afteraddNotification(expectfalse) and after the next run-loop tick (expecttrue)
As written, the test provides zero protection against a future regression that reverts the DispatchQueue.main.async wrapper.
cmuxTests/GhosttyConfigTests.swift
Outdated
| // The fix wraps moveTabToTop() in DispatchQueue.main.async inside | ||
| // TerminalNotificationStore.addNotification(). This test validates | ||
| // the architectural invariant rather than mocking internals. | ||
| let store = TerminalNotificationStore.shared |
There was a problem hiding this comment.
Shared singleton creates test-state pollution risk
TerminalNotificationStore.shared is a mutable singleton. Calling addNotification(...) on it in a unit test permanently adds a notification entry to the shared store, which can bleed into other test cases that use the same singleton later in the same test run (particularly any tests that read or count notifications).
Consider initializing a fresh TerminalNotificationStore instance for this test, or tearing down the added notification in a tearDown() override, to keep the test hermetic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2910981-b3e5-4ea3-ad8f-502cd575ef80
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/TerminalNotificationStore.swiftcmuxTests/GhosttyConfigTests.swift
| final class NotificationTabReorderDeferralTests: XCTestCase { | ||
| func testMoveTabToTopIsNotCalledSynchronouslyDuringAddNotification() { | ||
| // This test documents the expectation: when addNotification() is | ||
| // called with auto-reorder enabled, moveTabToTop() must NOT run | ||
| // in the same synchronous call stack. It should be deferred via | ||
| // DispatchQueue.main.async to prevent two @Published changes | ||
| // (tabs reorder + notifications update) from triggering a | ||
| // compounded SwiftUI re-render that blanks the active pane. | ||
| // | ||
| // The fix wraps moveTabToTop() in DispatchQueue.main.async inside | ||
| // TerminalNotificationStore.addNotification(). This test validates | ||
| // the architectural invariant rather than mocking internals. | ||
| let store = TerminalNotificationStore.shared | ||
| let tabId = UUID() | ||
|
|
||
| // Capture the tab count before adding a notification. | ||
| // If moveTabToTop were synchronous, it would fire during | ||
| // addNotification and could trigger observable side effects | ||
| // in the same run loop. The deferred version delays this. | ||
| let expectation = XCTestExpectation(description: "Deferred reorder completes") | ||
| store.addNotification( | ||
| tabId: tabId, | ||
| surfaceId: nil, | ||
| title: "Test", | ||
| subtitle: "", | ||
| body: "Test notification" | ||
| ) | ||
| // The moveTabToTop should be deferred — verify by checking | ||
| // that the notification was added (synchronous) while the | ||
| // reorder hasn't happened yet in this same run loop tick. | ||
| DispatchQueue.main.async { | ||
| // By the time this fires, the deferred moveTabToTop would | ||
| // also have been dispatched. This validates the async path. | ||
| expectation.fulfill() | ||
| } | ||
| wait(for: [expectation], timeout: 1.0) | ||
| } |
There was a problem hiding this comment.
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:
- No observable state is captured before/after the synchronous call to detect if
moveTabToTopfired early - The expectation just proves
DispatchQueue.main.asyncexecutes, which is always true - 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?.tabManagerto capture and verify call timing - Using an observable side effect (e.g., a counter or flag) that changes when
moveTabToTopis called, then asserting it hasn't changed immediately afteraddNotification()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.
…tion Address PR review feedback from Greptile and CodeRabbit: the previous runtime test was vacuous — it would pass regardless of sync vs async dispatch. Replace with source-code inspection (same pattern as TabManagerNotificationOrderingSourceTests) that reads the addNotification method body and verifies moveTabToTop is wrapped in DispatchQueue.main.async, not called synchronously. This eliminates singleton state pollution from TerminalNotificationStore .shared and provides a meaningful regression guard that will fail if the async deferral is removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/GhosttyConfigTests.swift">
<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:770">
P2: The new #914 “surface background guarantee” tests only re-check guard rejection logic and do not validate the behavior changed by the fix.</violation>
<violation number="2" location="cmuxTests/GhosttyConfigTests.swift:807">
P3: Using the shared notification store in this test introduces cross-test state leakage because `addNotification` mutates global singleton state; this can make test outcomes order-dependent.</violation>
<violation number="3" location="cmuxTests/GhosttyConfigTests.swift:825">
P2: This regression test does not actually verify reorder deferral; it only waits one async tick and always passes.</violation>
</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
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/GhosttyConfigTests.swift">
<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:807">
P2: This test asserts source-code strings instead of runtime behavior, violating the repository test policy and creating brittle regression coverage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| func testMoveTabToTopIsDeferredInsideAddNotification() throws { | ||
| let projectRoot = findProjectRoot() | ||
| let storeURL = projectRoot.appendingPathComponent("Sources/TerminalNotificationStore.swift") | ||
| let source = try String(contentsOf: storeURL, encoding: .utf8) |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
803-866: Source-code inspection tests trade-off acknowledged, but consider the fragility.This test reads source code and asserts on string patterns, which contradicts the learning that tests should verify observable runtime behavior rather than implementation shape. However, I see this follows the existing pattern from
TabManagerNotificationOrderingSourceTests(line 1058), and the comments explain the rationale: a runtime test couldn't meaningfully verify call timing without invasive mocking.A few fragility concerns worth noting:
Line 817: The heuristic
"\n func "to find method boundaries assumes 4-space indentation and a specific code style. If formatting changes, the test could silently pass on a truncated method body.Lines 840-851: The check that
moveTabToTopdoesn't appear "before" the async block relies on string ordering, which could give false confidence if the code is refactored (e.g., multiple reorder paths).Given the documented trade-off and existing precedent, this is acceptable as a regression guard. If the async wrapper is removed, this test will catch it.
Based on learnings: "Tests must verify observable runtime behavior through executable paths (unit/integration/e2e/CLI), not implementation shape" and "Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 803 - 866, The test is brittle because it extracts addNotification using a fixed "\n func " indentation and checks moveTabToTop with simple string ordering; instead, locate the addNotification(tabId:) body by finding its start (range(of: "func addNotification(tabId:")) and then parse forward matching braces to get the full method body (handle nested braces) so formatting changes don't break it, and then verify every occurrence of "moveTabToTop" falls inside the DispatchQueue.main.async block by finding the async block range and asserting all moveTabToTop ranges are contained within it (reference addNotification, moveTabToTop, WorkspaceAutoReorderSettings.isEnabled(), and findProjectRoot to locate the file).
🤖 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/GhosttyConfigTests.swift`:
- Around line 803-866: The test is brittle because it extracts addNotification
using a fixed "\n func " indentation and checks moveTabToTop with simple
string ordering; instead, locate the addNotification(tabId:) body by finding its
start (range(of: "func addNotification(tabId:")) and then parse forward matching
braces to get the full method body (handle nested braces) so formatting changes
don't break it, and then verify every occurrence of "moveTabToTop" falls inside
the DispatchQueue.main.async block by finding the async block range and
asserting all moveTabToTop ranges are contained within it (reference
addNotification, moveTabToTop, WorkspaceAutoReorderSettings.isEnabled(), and
findProjectRoot to locate the file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 603e6e1e-2977-4415-ade1-94a7e4a8350b
📒 Files selected for processing (1)
cmuxTests/GhosttyConfigTests.swift
No other test class in this file uses /// doc comments, MARK sections, or inline comments inside test methods. Align with the existing zero-comment convention where assertion messages carry intent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
816-826: Consider extracting the duplicatedfindProjectRoot()helper.This helper is duplicated at lines 1052-1062 in
TabManagerNotificationOrderingSourceTests. If source-inspection tests are retained, extract to a shared test utility.♻️ Example shared helper
// In a shared test utilities file: enum TestProjectRoot { static func find() -> 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) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 816 - 826, The findProjectRoot() helper in GhosttyConfigTests.swift is duplicated in TabManagerNotificationOrderingSourceTests; extract it to a shared test utility (e.g., a TestProjectRoot enum or static function) and replace both findProjectRoot() implementations with calls to that shared helper. Create the shared helper in a test utilities file included by both test targets (preserve the same logic: start from URL(fileURLWithPath: `#file`).deletingLastPathComponent().deletingLastPathComponent(), loop up to 10 levels checking for "GhosttyTabs.xcodeproj", and fall back to FileManager.default.currentDirectoryPath), then update GhosttyConfigTests.findProjectRoot() usages and the duplicate in TabManagerNotificationOrderingSourceTests to call TestProjectRoot.find() (or whatever name you choose).
🤖 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/GhosttyConfigTests.swift`:
- Around line 816-826: The findProjectRoot() helper in GhosttyConfigTests.swift
is duplicated in TabManagerNotificationOrderingSourceTests; extract it to a
shared test utility (e.g., a TestProjectRoot enum or static function) and
replace both findProjectRoot() implementations with calls to that shared helper.
Create the shared helper in a test utilities file included by both test targets
(preserve the same logic: start from URL(fileURLWithPath:
`#file`).deletingLastPathComponent().deletingLastPathComponent(), loop up to 10
levels checking for "GhosttyTabs.xcodeproj", and fall back to
FileManager.default.currentDirectoryPath), then update
GhosttyConfigTests.findProjectRoot() usages and the duplicate in
TabManagerNotificationOrderingSourceTests to call TestProjectRoot.find() (or
whatever name you choose).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09f61e22-4e62-480b-9606-86438af143ad
📒 Files selected for processing (1)
cmuxTests/GhosttyConfigTests.swift
1 participant
Summary
Fixes the active terminal pane going blank when a notification fires on a different tab (#914).
Root cause: Two compounding issues triggered by the same event chain:
Surface background not maintained on guard rejection —
applyWindowBackgroundIfActive()calledapplySurfaceBackground()only after theshouldApplyWindowBackground()guard. When a notification-triggered tab reorder caused the owning manager'sselectedTabIdto temporarily differ from the surface'stabId, the guard rejected and the method returned early without maintaining the surface background, leaving the pane in a stale transparent state.Synchronous
moveTabToTop()duringaddNotification()— When workspace auto-reorder is enabled,moveTabToTop()was called synchronously insideaddNotification(), producing two@Publishedchanges (tabsreorder +notificationsupdate) in the same run loop frame. This compounded SwiftUI re-renders that triggered portal visibility races (Bonsplit transiently reportingisSelectedInPane=false) and background application failures.Changes
GhosttyTerminalView.swift: MoveapplySurfaceBackground()before theshouldApplyWindowBackground()guard so it always runs, even when window-level background application is skipped. The call is idempotent.TerminalNotificationStore.swift: WrapmoveTabToTop()inDispatchQueue.main.asyncto defer tab reordering out of the notification hot path, breaking the synchronous cascade.GhosttyConfigTests.swift: Add regression tests documenting the Terminal pane goes blank when notification fires on new tab #914 trigger conditions.Test plan
WindowBackgroundSurfaceBackgroundGuaranteeTests— verifies the guard rejection scenarios that previously caused blankingNotificationTabReorderDeferralTests— validates deferred reorder via async expectationCloses #914
Summary by cubic
Fixes a blank active terminal pane when a notification fires on another tab (#914). Ensures the surface background is always applied and defers tab reordering to prevent SwiftUI race conditions.
GhosttyTerminalView.swift, callapplySurfaceBackground()before theshouldApplyWindowBackground()guard so the pane never goes transparent.TerminalNotificationStore.swift, defermoveTabToTop()withDispatchQueue.main.asyncto avoid tabs + notifications updates in the same frame.NotificationTabReorderDeferralTeststo assert deferral via source inspection, and strip comments to match project style.Written for commit bf962a4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests