From fa698ae3af1156111de7812a33e92925388e9bd5 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Sun, 27 Sep 2026 05:13:57 -0700 Subject: [PATCH 1/7] test: cover popover closes whose didClose never arrives reaches popoverWillClose but never popoverDidClose. The click-away test simulates that fault and fails on main: nothing bounds the close, so the presenter stays closing with isShown true and the next toggle cannot present a new popover. A second test fails the same way and pins that a repeated willClose keeps the first deadline. The toggle-close test pins that a programmatic close is never reported as an external dismissal, whether didClose or the deadline ends it. The presenter takes the clock its close deadline will run on, so the tests advance a manual clock instead of sleeping; SidebarTestManualClock gains a sleeper count for deadline-bounded polls. Also cover the checklist section restoring close animation when its popover survives an anchor reparent. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../SidebarRowSwiftUIPopoverPresenter.swift | 11 ++ .../SidebarWorkspaceRowChecklistSection.swift | 6 + cmux.xcodeproj/project.pbxproj | 4 + ...debarRowSwiftUIPopoverPresenterTests.swift | 135 ++++++++++++++++++ .../SidebarSelectionCoalescerTests.swift | 7 + .../SidebarWorkspaceRowSuspensionTests.swift | 60 ++++++++ 6 files changed, 223 insertions(+) create mode 100644 cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift index e96ff73da71f..20bc2051e831 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift @@ -46,6 +46,17 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { /// must know whether a close already began check this as well. private(set) var isClosing = false + /// - Parameter closeCompletionClock: Drives the close-completion + /// fallback's deadline. Tests pass a clock they advance by hand. + init(closeCompletionClock: any Clock = ContinuousClock()) { + super.init() + } + +#if DEBUG + /// The current popover's `animates`, or nil when none is presented. + var animatesForTesting: Bool? { popover?.animates } +#endif + func present( _ root: AnyView, relativeTo rect: NSRect, diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift b/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift index 1c6e6b6a81ee..c5d2c49b03a6 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift @@ -518,6 +518,12 @@ final class SidebarRowChecklistSection: NSView { } } +#if DEBUG + /// Whether the shown checklist popover animates its close, or nil when + /// no popover is presented. + var checklistPopoverAnimatesForTesting: Bool? { popoverPresenter.animatesForTesting } +#endif + private func checklistPopoverModel(_ model: SidebarWorkspaceRowModel) -> SidebarWorkspaceChecklistPopoverModel { let snapshot = model.snapshot return SidebarWorkspaceChecklistPopoverModel( diff --git a/cmux.xcodeproj/project.pbxproj b/cmux.xcodeproj/project.pbxproj index 822f745832e9..838981dd6701 100644 --- a/cmux.xcodeproj/project.pbxproj +++ b/cmux.xcodeproj/project.pbxproj @@ -2959,6 +2959,7 @@ C12791000000000000000003 /* SidebarRowSnapshotCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = C12791000000000000000004 /* SidebarRowSnapshotCache.swift */; }; C12791000000000000000005 /* SidebarRowSnapshotCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C12791000000000000000006 /* SidebarRowSnapshotCacheTests.swift */; }; B804A0310000000000000031 /* SidebarRowSwiftUIPopoverPresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = B804B0310000000000000031 /* SidebarRowSwiftUIPopoverPresenter.swift */; }; + D70D754898C65B4C6FA39754 /* SidebarRowSwiftUIPopoverPresenterTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3A6437480E46CCF7BE14124 /* SidebarRowSwiftUIPopoverPresenterTests.swift */; }; B804A0320000000000000032 /* SidebarRowTaskStatusGlyphButton.swift in Sources */ = {isa = PBXBuildFile; fileRef = B804B0320000000000000032 /* SidebarRowTaskStatusGlyphButton.swift */; }; B804A03A000000000000003A /* SidebarRowTextAccessibilityLink.swift in Sources */ = {isa = PBXBuildFile; fileRef = B804B03A000000000000003A /* SidebarRowTextAccessibilityLink.swift */; }; 5CDA4D369D5728DCD6B39BEF /* SidebarRowTextLinkLayout.swift in Sources */ = {isa = PBXBuildFile; fileRef = 28782D804EEE07F5CC3E872C /* SidebarRowTextLinkLayout.swift */; }; @@ -6933,6 +6934,7 @@ B804B0370000000000000037 /* SidebarRowPressedDim.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sidebar/AppKitList/Cells/SidebarRowPressedDim.swift; sourceTree = ""; }; C12791000000000000000006 /* SidebarRowSnapshotCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SidebarRowSnapshotCacheTests.swift; sourceTree = ""; }; B804B0310000000000000031 /* SidebarRowSwiftUIPopoverPresenter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift; sourceTree = ""; }; + B3A6437480E46CCF7BE14124 /* SidebarRowSwiftUIPopoverPresenterTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SidebarRowSwiftUIPopoverPresenterTests.swift"; sourceTree = ""; }; B804B0320000000000000032 /* SidebarRowTaskStatusGlyphButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sidebar/AppKitList/Cells/SidebarRowTaskStatusGlyphButton.swift; sourceTree = ""; }; B804B03A000000000000003A /* SidebarRowTextAccessibilityLink.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sidebar/AppKitList/Cells/SidebarRowTextAccessibilityLink.swift; sourceTree = ""; }; 28782D804EEE07F5CC3E872C /* SidebarRowTextLinkLayout.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Sidebar/AppKitList/Cells/SidebarRowTextLinkLayout.swift; sourceTree = ""; }; @@ -12152,6 +12154,7 @@ DA19B32D9CDA472886E0646F /* SessionSnapshotOverwriteGuardAppTests.swift */, AA11C5ADC15512A75E032CCA /* TerminalSurfaceTestTeardown.swift */, A634CA3FCA39C6F02FA78DC7 /* UpdateRelaunchAgentResumeTests.swift */, + B3A6437480E46CCF7BE14124 /* SidebarRowSwiftUIPopoverPresenterTests.swift */, ); path = cmuxTests; sourceTree = ""; @@ -16523,6 +16526,7 @@ 5B0C00010000000000000003 /* SidebarResizerOcclusionResolverTests.swift in Sources */, 3D0EDA5CB6EA473DB4DB9464 /* SidebarRowPaletteTests.swift in Sources */, C12791000000000000000005 /* SidebarRowSnapshotCacheTests.swift in Sources */, + D70D754898C65B4C6FA39754 /* SidebarRowSwiftUIPopoverPresenterTests.swift in Sources */, C9A57513C9A57513C9A57513 /* SidebarScrollViewConfiguratorTests.swift in Sources */, F6528A82B7124269968F9A99 /* SidebarSearchFieldTests.swift in Sources */, B804C0020000000000000002 /* SidebarSelectionCoalescerTests.swift in Sources */, diff --git a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift new file mode 100644 index 000000000000..c954fb2f5846 --- /dev/null +++ b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift @@ -0,0 +1,135 @@ +import AppKit +import SwiftUI +import Testing +@testable import cmux_DEV + +/// On some owned Mac minis an animated `NSPopover` close starts +/// (`popoverWillClose`) but never finishes (`popoverDidClose`). The presenter +/// must not stay "closing" forever: that left `isShown` true, so every later +/// toggle closed the stuck popover again instead of presenting a new one. +@Suite(.serialized) +@MainActor +struct SidebarRowSwiftUIPopoverPresenterTests { + @MainActor + private final class Host { + let anchor = NSView(frame: NSRect(x: 0, y: 0, width: 320, height: 80)) + let window: NSWindow + + init() { + window = NSWindow( + contentRect: anchor.bounds, + styleMask: [.borderless], + backing: .buffered, + defer: false + ) + window.contentView = anchor + window.orderFront(nil) + } + + func present(_ presenter: SidebarRowSwiftUIPopoverPresenter) { + presenter.present( + AnyView(Text(verbatim: "Checklist")), + relativeTo: NSRect(x: anchor.bounds.width - 1, y: 0, width: 1, height: 1), + of: anchor, + preferredEdge: .maxX + ) + } + + func tearDown(_ presenter: SidebarRowSwiftUIPopoverPresenter) { + presenter.onExternalDismiss = nil + presenter.close() + window.contentView = nil + window.close() + } + } + + /// Waits until the presenter's close fallback is sleeping on `clock`. + private func fallbackArmed(on clock: SidebarTestManualClock) async -> Bool { + await AppKitTestEventPump().waitUntil(timeout: .seconds(3)) { clock.sleeperCount == 1 } + } + + private func closeCompleted(_ presenter: SidebarRowSwiftUIPopoverPresenter) async -> Bool { + await AppKitTestEventPump().waitUntil(timeout: .seconds(3)) { + !presenter.isShown && !presenter.isClosing + } + } + + @Test + func userCloseWhoseAnimationNeverFinishesStillCompletes() async throws { + let host = Host() + let clock = SidebarTestManualClock() + let presenter = SidebarRowSwiftUIPopoverPresenter(closeCompletionClock: clock) + defer { host.tearDown(presenter) } + var dismissals = 0 + presenter.onExternalDismiss = { dismissals += 1 } + host.present(presenter) + try #require(presenter.isShown) + + // AppKit starts a click-away close, and its `popoverDidClose` never + // arrives, as on the affected hosts. + presenter.popoverWillClose(Notification(name: NSPopover.willCloseNotification)) + #expect(presenter.isClosing) + #expect(await fallbackArmed(on: clock), "willClose should arm a bounded close fallback") + #expect(presenter.isClosing, "The close is still in flight before the deadline") + + clock.advance(by: .seconds(1)) + #expect(await closeCompleted(presenter), "A close whose animation never finishes should still complete") + #expect(dismissals == 1, "The click-away should be reported as an external dismissal once") + + // The next toggle presents a new popover instead of closing the stuck + // one, and that popover animates its own close again. + host.present(presenter) + #expect(presenter.isShown) + #expect(!presenter.isClosing) + #expect(presenter.animatesForTesting == true) + } + + @Test + func repeatedWillCloseKeepsTheFirstDeadline() async throws { + let host = Host() + let clock = SidebarTestManualClock() + let presenter = SidebarRowSwiftUIPopoverPresenter(closeCompletionClock: clock) + defer { host.tearDown(presenter) } + host.present(presenter) + try #require(presenter.isShown) + + let willClose = Notification(name: NSPopover.willCloseNotification) + presenter.popoverWillClose(willClose) + #expect(await fallbackArmed(on: clock)) + clock.advance(by: .milliseconds(600)) + presenter.popoverWillClose(willClose) + await AppKitTestEventPump().drain() + + // One second after the first willClose, not after the second. + clock.advance(by: .milliseconds(400)) + #expect(await closeCompleted(presenter), "A repeated willClose must not push completion back") + } + + @Test + func toggleCloseIsNeverAnExternalDismissal() async throws { + let host = Host() + let clock = SidebarTestManualClock() + let presenter = SidebarRowSwiftUIPopoverPresenter(closeCompletionClock: clock) + defer { host.tearDown(presenter) } + var dismissals = 0 + presenter.onExternalDismiss = { dismissals += 1 } + host.present(presenter) + try #require(presenter.isShown) + + // A real animated close. Healthy hosts deliver didClose; affected + // hosts never do. Past the fallback's deadline, either way ends the + // close, and neither may report the toggle as the user dismissing + // the popover from outside. + presenter.close() + _ = await AppKitTestEventPump().waitUntil(timeout: .seconds(3)) { + clock.sleeperCount == 1 || !presenter.isClosing + } + clock.advance(by: .seconds(1)) + #expect(await closeCompleted(presenter)) + await AppKitTestEventPump().drain() + #expect(dismissals == 0) + + host.present(presenter) + #expect(presenter.isShown) + } +} diff --git a/cmuxTests/SidebarSelectionCoalescerTests.swift b/cmuxTests/SidebarSelectionCoalescerTests.swift index 3c2c4094a25c..058826f8f2e0 100644 --- a/cmuxTests/SidebarSelectionCoalescerTests.swift +++ b/cmuxTests/SidebarSelectionCoalescerTests.swift @@ -126,6 +126,13 @@ final class SidebarTestManualClock: Clock, @unchecked Sendable { var minimumResolution: Duration { .zero } + /// Sleeps registered and not yet resumed, for deadline-bounded polls. + var sleeperCount: Int { + lock.lock() + defer { lock.unlock() } + return sleepers.count + } + var retainedCancellationMarkerCount: Int { lock.lock() defer { lock.unlock() } diff --git a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift index d6bc6edd41a7..e4cd57982e64 100644 --- a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift +++ b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift @@ -350,6 +350,66 @@ struct SidebarWorkspaceRowSuspensionTests { #expect(tokenConsumptions == 0) } + @Test + func checklistPopoverThatSurvivesReparentAnimatesItsLaterClose() async throws { + let application = NSApplication.shared + let model = Self.makeModel( + checklistAddFieldActivationToken: 1, + checklistItems: [WorkspaceChecklistItem(text: "Draft item")], + isChecklistPopoverPresented: true, + checklistStyle: .popover + ) + var presentationChanges: [Bool] = [] + let cell = SidebarWorkspaceRowTableCellView( + frame: NSRect(x: 0, y: 0, width: 320, height: 100) + ) + let window = NSWindow( + contentRect: cell.bounds, + styleMask: [.borderless], + backing: .buffered, + defer: false + ) + window.contentView = cell + window.orderFront(nil) + defer { window.close() } + let existingWindowIds = Set(application.windows.map(ObjectIdentifier.init)) + cell.configure( + model: model, + actions: Self.makeActions( + model: model, + onChecklistPopoverPresentedChange: { presentationChanges.append($0) } + ), + isPointerHovering: false, + contextMenuDidOpen: {}, + contextMenuDidClose: {} + ) + _ = cell.layoutContent(model: model, width: cell.bounds.width, apply: true) + cell.layoutSubtreeIfNeeded() + let popoverWindow = try #require( + application.windows.first { + !existingWindowIds.contains(ObjectIdentifier($0)) && $0.isVisible + } + ) + let section = try #require( + Self.descendants(of: cell).compactMap { $0 as? SidebarRowChecklistSection }.first + ) + #expect(section.checklistPopoverAnimatesForTesting == true) + + // A reparent the popover survives: the anchor announces that it is + // leaving its window and then that it is back, and AppKit never + // closes the popover in between. + section.viewWillMove(toWindow: nil) + #expect(section.checklistPopoverAnimatesForTesting == false) + section.viewDidMoveToWindow() + + let restored = await AppKitTestEventPump().waitUntil { + section.checklistPopoverAnimatesForTesting == true + } + #expect(restored, "A popover that survives a reparent should animate its later close again") + #expect(popoverWindow.isVisible) + #expect(presentationChanges.isEmpty) + } + @Test func checklistDraftCommitsOnlyOnceWhenFocusEndsBeforeSuspension() async throws { let model = Self.makeModel(checklistAddFieldActivationToken: 1, checklistStyle: .inline) From ff8429186a91376afc3908dd462d3f655c3a1f9b Mon Sep 17 00:00:00 2001 From: Leo Li Date: Sun, 27 Sep 2026 05:14:43 -0700 Subject: [PATCH 2/7] fix(sidebar): finish popover closes whose didClose never arrives #14895 found that on some owned Mac minis an animated NSPopover close reaches popoverWillClose but never popoverDidClose, and fixed only the anchor-detach close by turning its animation off. An ordinary user close (click-away, Esc, toggle) still animates, so on those hosts the presenter stayed closing with isShown true: the next toggle closed the stuck popover again instead of presenting, and a click-away was never written back. popoverWillClose now arms a one-second deadline, the close fade plus a margin, on a MainActorDeferredActionScheduler. A repeated willClose for the same popover keeps the first deadline. If didClose has not arrived by then, the presenter abandons the stuck popover (drops its delegate, closes it without animation, orders its window out) and runs the same completion didClose would. The next popover is new, animates normally, and gets its own hosting controller so a late teardown of the abandoned one cannot take its content view. Notifications from an abandoned popover are ignored. Animation stays on everywhere else. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../SidebarRowSwiftUIPopoverPresenter.swift | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift index 20bc2051e831..d11eda6a3d8b 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift @@ -1,5 +1,6 @@ import AppKit import CmuxAppKitSupportUI +import CmuxFoundation import CmuxWorkspaces import SwiftUI @@ -32,6 +33,16 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { private var popover: NSPopover? private var presentationCount = 0 private var closingProgrammatically = false + /// Completes a close whose `popoverDidClose` never arrives; see + /// `armCloseCompletionFallback(for:)`. + private let closeCompletionFallback: MainActorDeferredActionScheduler + /// The popover the pending fallback will complete. + private weak var closeCompletionFallbackTarget: NSPopover? + + /// How long an animated close may take before the presenter completes + /// it itself: NSPopover's close fade (about 0.2 s) plus a wide margin + /// for a busy main thread. + static let closeCompletionTimeout: Duration = .seconds(1) /// Visible refreshes arrive from the table's configure pass (inside a /// representable update turn); defer + coalesce them like /// `SidebarWorkspaceTodoPopoverHost` does instead of forcing synchronous @@ -49,6 +60,7 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { /// - Parameter closeCompletionClock: Drives the close-completion /// fallback's deadline. Tests pass a clock they advance by hand. init(closeCompletionClock: any Clock = ContinuousClock()) { + closeCompletionFallback = MainActorDeferredActionScheduler(clock: closeCompletionClock) super.init() } @@ -146,10 +158,69 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { } func popoverWillClose(_ notification: Notification) { + guard isCurrentPopover(notification) else { return } isClosing = true + if let popover { + armCloseCompletionFallback(for: popover) + } } func popoverDidClose(_ notification: Notification) { + guard isCurrentPopover(notification) else { return } + finishClose() + } + + /// A notification from a popover this presenter already gave up on (see + /// the fallback below) must not tear down the one presented since. + private func isCurrentPopover(_ notification: Notification) -> Bool { + guard let sender = notification.object as? NSPopover else { return true } + return sender === popover + } + + /// An animated close reaches `popoverDidClose` only when its animation + /// finishes, and on some owned Mac minis that never happens (#14895). + /// The popover then stays `isShown` and the presenter stays closing, so + /// every later toggle closes the stuck popover again instead of showing + /// a new one, and a click-away is never written back to the container. + /// If the close hasn't finished within the timeout, finish it here: + /// detach from the stuck popover, take its window down, and run the same + /// completion `popoverDidClose` would have. + /// + /// A repeated `popoverWillClose` for the same popover keeps the first + /// deadline, so it cannot keep pushing the completion back. + private func armCloseCompletionFallback(for closing: NSPopover) { + if closeCompletionFallback.isScheduled, closeCompletionFallbackTarget === closing { + return + } + closeCompletionFallbackTarget = closing + closeCompletionFallback.schedule(after: Self.closeCompletionTimeout) { [weak self, weak closing] in + guard let self, let closing, closing === self.popover, self.isClosing else { return } + self.abandon(closing) + } + } + + private func abandon(_ closing: NSPopover) { + closing.delegate = nil + // Only this stuck popover loses its animation; the next one is new. + closing.animates = false + let closingWindow = closing.contentViewController?.view.window + if closing.isShown { + closing.close() + } + closingWindow?.orderOut(nil) + // The abandoned popover keeps its hosting controller. Should its + // stalled close ever resume, it must not tear the content view out of + // the popover presented next, so that one gets its own controller. + // Swap before `finishClose()`, whose dismissal callback may present. + let abandonedController = hostingController + hostingController = NSHostingController(rootView: AnyView(EmptyView())) + abandonedController.rootView = AnyView(EmptyView()) + finishClose() + } + + private func finishClose() { + closeCompletionFallback.cancel() + closeCompletionFallbackTarget = nil isClosing = false visibleUpdateScheduler.cancel() pendingRoot = nil From a05a7c3e04ee29d1bcb7c668b56fa79981b26e43 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 28 Sep 2026 09:55:24 -0400 Subject: [PATCH 3/7] fix(sidebar): re-present cancels a pending close fallback; drop test seams Showing a hidden popover again now cancels the close fallback armed by its earlier willClose, so the stale deadline cannot abandon the popover that is visible again. Tests read the presenter's popover through @testable import instead of DEBUG-only accessors. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../SidebarRowSwiftUIPopoverPresenter.swift | 13 ++++---- .../SidebarWorkspaceRowChecklistSection.swift | 8 +---- ...debarRowSwiftUIPopoverPresenterTests.swift | 33 ++++++++++++++++++- .../SidebarWorkspaceRowSuspensionTests.swift | 6 ++-- 4 files changed, 43 insertions(+), 17 deletions(-) diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift index d11eda6a3d8b..cccb5cc2da3d 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift @@ -30,7 +30,7 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { /// Lazy: cells allocate presenters eagerly, but the hosting machinery /// only spins up when a popover actually presents (off the scroll path). private lazy var hostingController = NSHostingController(rootView: AnyView(EmptyView())) - private var popover: NSPopover? + private(set) var popover: NSPopover? private var presentationCount = 0 private var closingProgrammatically = false /// Completes a close whose `popoverDidClose` never arrives; see @@ -64,11 +64,6 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { super.init() } -#if DEBUG - /// The current popover's `animates`, or nil when none is presented. - var animatesForTesting: Bool? { popover?.animates } -#endif - func present( _ root: AnyView, relativeTo rect: NSRect, @@ -83,6 +78,12 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { } visibleUpdateScheduler.cancel() pendingRoot = nil + // Showing a hidden popover again supersedes any close still in + // flight for it: its pending fallback must not abandon the popover + // that is about to be visible. + closeCompletionFallback.cancel() + closeCompletionFallbackTarget = nil + isClosing = false presentationCount += 1 applyRootView(root) popover.show(relativeTo: rect, of: view, preferredEdge: preferredEdge) diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift b/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift index c5d2c49b03a6..cf1d0e5170c8 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection.swift @@ -25,7 +25,7 @@ final class SidebarRowChecklistSection: NSView { private var orderedLines: [SidebarRowChecklistItemLine] = [] private var freeLines: [SidebarRowChecklistItemLine] = [] private let addRow = SidebarRowChecklistAddRow() - private let popoverPresenter = SidebarRowSwiftUIPopoverPresenter() + let popoverPresenter = SidebarRowSwiftUIPopoverPresenter() private var model: SidebarWorkspaceRowModel? private var actions: SidebarAppKitRowActions? @@ -518,12 +518,6 @@ final class SidebarRowChecklistSection: NSView { } } -#if DEBUG - /// Whether the shown checklist popover animates its close, or nil when - /// no popover is presented. - var checklistPopoverAnimatesForTesting: Bool? { popoverPresenter.animatesForTesting } -#endif - private func checklistPopoverModel(_ model: SidebarWorkspaceRowModel) -> SidebarWorkspaceChecklistPopoverModel { let snapshot = model.snapshot return SidebarWorkspaceChecklistPopoverModel( diff --git a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift index c954fb2f5846..34ea3eade684 100644 --- a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift +++ b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift @@ -81,7 +81,7 @@ struct SidebarRowSwiftUIPopoverPresenterTests { host.present(presenter) #expect(presenter.isShown) #expect(!presenter.isClosing) - #expect(presenter.animatesForTesting == true) + #expect(presenter.popover?.animates == true) } @Test @@ -105,6 +105,37 @@ struct SidebarRowSwiftUIPopoverPresenterTests { #expect(await closeCompleted(presenter), "A repeated willClose must not push completion back") } + @Test + func representingAHiddenPopoverCancelsThePendingFallback() async throws { + let host = Host() + let clock = SidebarTestManualClock() + let presenter = SidebarRowSwiftUIPopoverPresenter(closeCompletionClock: clock) + defer { host.tearDown(presenter) } + host.present(presenter) + let popover = try #require(presenter.popover) + try #require(presenter.isShown) + + // A close starts and the popover goes hidden, but its didClose has + // not reached the presenter yet when the container presents again. + presenter.popoverWillClose(Notification(name: NSPopover.willCloseNotification, object: popover)) + #expect(await fallbackArmed(on: clock)) + popover.delegate = nil + popover.animates = false + popover.close() + popover.delegate = presenter + try #require(!presenter.isShown) + + host.present(presenter) + #expect(presenter.isShown) + #expect(!presenter.isClosing, "Presenting again supersedes the close in flight") + + // The old deadline passing must not abandon the popover now showing. + clock.advance(by: .seconds(1)) + await AppKitTestEventPump().drain() + #expect(presenter.isShown, "The superseded fallback must not close the re-presented popover") + #expect(presenter.popover === popover) + } + @Test func toggleCloseIsNeverAnExternalDismissal() async throws { let host = Host() diff --git a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift index e4cd57982e64..908141bc37ae 100644 --- a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift +++ b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift @@ -393,17 +393,17 @@ struct SidebarWorkspaceRowSuspensionTests { let section = try #require( Self.descendants(of: cell).compactMap { $0 as? SidebarRowChecklistSection }.first ) - #expect(section.checklistPopoverAnimatesForTesting == true) + #expect(section.popoverPresenter.popover?.animates == true) // A reparent the popover survives: the anchor announces that it is // leaving its window and then that it is back, and AppKit never // closes the popover in between. section.viewWillMove(toWindow: nil) - #expect(section.checklistPopoverAnimatesForTesting == false) + #expect(section.popoverPresenter.popover?.animates == false) section.viewDidMoveToWindow() let restored = await AppKitTestEventPump().waitUntil { - section.checklistPopoverAnimatesForTesting == true + section.popoverPresenter.popover?.animates == true } #expect(restored, "A popover that survives a reparent should animate its later close again") #expect(popoverWindow.isVisible) From 1c2e7ba7adf6ed51d3c8b5196cb196fc2e3b7ccb Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 28 Sep 2026 10:19:09 -0400 Subject: [PATCH 4/7] dogfood: tour the checklist popover's click-away and toggle closes Opens the checklist popover from the sidebar summary line, closes it by clicking away and by toggling, and reopens it after each close, so the PR media shows the presenter never stays stuck closing. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../checklist-popover-close-tour.json | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 dogfood/scenarios/checklist-popover-close-tour.json diff --git a/dogfood/scenarios/checklist-popover-close-tour.json b/dogfood/scenarios/checklist-popover-close-tour.json new file mode 100644 index 000000000000..51a2e6855ae0 --- /dev/null +++ b/dogfood/scenarios/checklist-popover-close-tour.json @@ -0,0 +1,35 @@ +{ + "paths": ["Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter*", "Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection*", "Sources/Sidebar/AppKitList/Cells/SidebarRowChecklistSummaryLine*", "Sources/SidebarWorkspaceChecklistPopover*"], + "launch": {"args": ["-sidebar.beta.workspaceTodos.controls.enabled", "YES", "-sidebarWorkspaceTodosChecklistStyle", "popover"]}, + "steps": [ + {"socket": "workspace.current", "save": "ws"}, + {"socket": "workspace.todo.add", "params": {"workspace_id": "${ws.workspace_id}", "text": "Ship the popover close fix"}}, + {"socket": "workspace.todo.add", "params": {"workspace_id": "${ws.workspace_id}", "text": "Check the sidebar after a click-away"}}, + {"wait": 1}, + {"shot": "checklist-summary"}, + {"click": {"id": "SidebarChecklistSummaryLine"}}, + {"wait": 1}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, + {"shot": "popover-open"}, + {"clickAt": {"x": 0.65, "y": 0.6}}, + {"wait": 2}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, + {"shot": "after-click-away"}, + {"click": {"id": "SidebarChecklistSummaryLine"}}, + {"wait": 1}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, + {"shot": "reopened-after-click-away"}, + {"click": {"id": "SidebarChecklistSummaryLine"}}, + {"wait": 2}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, + {"shot": "after-toggle-close"}, + {"click": {"id": "SidebarChecklistSummaryLine"}}, + {"wait": 1}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, + {"shot": "reopened-after-toggle"}, + {"clickAt": {"x": 0.65, "y": 0.6}}, + {"wait": 2}, + {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, + {"shot": "end", "screen": true} + ] +} From 747317b8d5de26994a6be9de53881fe9a0a055b6 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 28 Sep 2026 10:23:12 -0400 Subject: [PATCH 5/7] fix(sidebar): re-present also resets the programmatic-close flag A programmatic close superseded by a re-present no longer marks the next click-away as programmatic. The re-present test now checks the fallback was cancelled, the reparent test closes its popover, and the dogfood tour clicks away farther from the popover. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift | 1 + cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift | 1 + cmuxTests/SidebarWorkspaceRowSuspensionTests.swift | 2 ++ dogfood/scenarios/checklist-popover-close-tour.json | 4 ++-- 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift index cccb5cc2da3d..e558676743c5 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift @@ -84,6 +84,7 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { closeCompletionFallback.cancel() closeCompletionFallbackTarget = nil isClosing = false + closingProgrammatically = false presentationCount += 1 applyRootView(root) popover.show(relativeTo: rect, of: view, preferredEdge: preferredEdge) diff --git a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift index 34ea3eade684..df3eb3c0e5c1 100644 --- a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift +++ b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift @@ -128,6 +128,7 @@ struct SidebarRowSwiftUIPopoverPresenterTests { host.present(presenter) #expect(presenter.isShown) #expect(!presenter.isClosing, "Presenting again supersedes the close in flight") + #expect(clock.sleeperCount == 0, "Presenting again cancels the superseded fallback") // The old deadline passing must not abandon the popover now showing. clock.advance(by: .seconds(1)) diff --git a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift index 908141bc37ae..a3399462fc43 100644 --- a/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift +++ b/cmuxTests/SidebarWorkspaceRowSuspensionTests.swift @@ -408,6 +408,8 @@ struct SidebarWorkspaceRowSuspensionTests { #expect(restored, "A popover that survives a reparent should animate its later close again") #expect(popoverWindow.isVisible) #expect(presentationChanges.isEmpty) + section.popoverPresenter.onExternalDismiss = nil + section.popoverPresenter.close() } @Test diff --git a/dogfood/scenarios/checklist-popover-close-tour.json b/dogfood/scenarios/checklist-popover-close-tour.json index 51a2e6855ae0..cc73270f22aa 100644 --- a/dogfood/scenarios/checklist-popover-close-tour.json +++ b/dogfood/scenarios/checklist-popover-close-tour.json @@ -11,7 +11,7 @@ {"wait": 1}, {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, {"shot": "popover-open"}, - {"clickAt": {"x": 0.65, "y": 0.6}}, + {"clickAt": {"x": 0.9, "y": 0.85}}, {"wait": 2}, {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, {"shot": "after-click-away"}, @@ -27,7 +27,7 @@ {"wait": 1}, {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, {"shot": "reopened-after-toggle"}, - {"clickAt": {"x": 0.65, "y": 0.6}}, + {"clickAt": {"x": 0.9, "y": 0.85}}, {"wait": 2}, {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, {"shot": "end", "screen": true} From 44f2f0e136a6ccfa98edcfc711cd4e128b38e3ac Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 28 Sep 2026 10:28:12 -0400 Subject: [PATCH 6/7] fix(sidebar): ignore a superseded close's late didClose after re-present Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Cells/SidebarRowSwiftUIPopoverPresenter.swift | 4 +++- cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift index e558676743c5..c3107d4f6c26 100644 --- a/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift +++ b/Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter.swift @@ -168,7 +168,9 @@ final class SidebarRowSwiftUIPopoverPresenter: NSObject, NSPopoverDelegate { } func popoverDidClose(_ notification: Notification) { - guard isCurrentPopover(notification) else { return } + // A didClose that lands after `present` showed the same popover again + // belongs to the superseded close; the popover on screen stays. + guard isCurrentPopover(notification), popover?.isShown != true else { return } finishClose() } diff --git a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift index df3eb3c0e5c1..89a1065e7827 100644 --- a/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift +++ b/cmuxTests/SidebarRowSwiftUIPopoverPresenterTests.swift @@ -111,6 +111,8 @@ struct SidebarRowSwiftUIPopoverPresenterTests { let clock = SidebarTestManualClock() let presenter = SidebarRowSwiftUIPopoverPresenter(closeCompletionClock: clock) defer { host.tearDown(presenter) } + var dismissals = 0 + presenter.onExternalDismiss = { dismissals += 1 } host.present(presenter) let popover = try #require(presenter.popover) try #require(presenter.isShown) @@ -135,6 +137,13 @@ struct SidebarRowSwiftUIPopoverPresenterTests { await AppKitTestEventPump().drain() #expect(presenter.isShown, "The superseded fallback must not close the re-presented popover") #expect(presenter.popover === popover) + + // The superseded close's didClose arriving late must not tear down + // or report a dismissal of the popover now showing. + presenter.popoverDidClose(Notification(name: NSPopover.didCloseNotification, object: popover)) + #expect(presenter.isShown, "A late didClose must not close the re-presented popover") + #expect(presenter.popover === popover) + #expect(dismissals == 0, "A late didClose is not an external dismissal") } @Test From bc19ecc381bc4529efa365fb0ce85c3a81dd1e07 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 28 Sep 2026 12:55:43 -0400 Subject: [PATCH 7/7] dogfood: click the checklist summary line by position The summary line is not reachable by identifier from the UI test, so the tour clicks where it draws and records the sidebar tree for diagnosis. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../checklist-popover-close-tour.json | 181 +++++++++++++++--- 1 file changed, 150 insertions(+), 31 deletions(-) diff --git a/dogfood/scenarios/checklist-popover-close-tour.json b/dogfood/scenarios/checklist-popover-close-tour.json index cc73270f22aa..f11c1ad10c61 100644 --- a/dogfood/scenarios/checklist-popover-close-tour.json +++ b/dogfood/scenarios/checklist-popover-close-tour.json @@ -1,35 +1,154 @@ { - "paths": ["Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter*", "Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection*", "Sources/Sidebar/AppKitList/Cells/SidebarRowChecklistSummaryLine*", "Sources/SidebarWorkspaceChecklistPopover*"], - "launch": {"args": ["-sidebar.beta.workspaceTodos.controls.enabled", "YES", "-sidebarWorkspaceTodosChecklistStyle", "popover"]}, + "paths": [ + "Sources/Sidebar/AppKitList/Cells/SidebarRowSwiftUIPopoverPresenter*", + "Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowChecklistSection*", + "Sources/Sidebar/AppKitList/Cells/SidebarRowChecklistSummaryLine*", + "Sources/SidebarWorkspaceChecklistPopover*" + ], + "launch": { + "args": [ + "-sidebar.beta.workspaceTodos.controls.enabled", + "YES", + "-sidebarWorkspaceTodosChecklistStyle", + "popover" + ] + }, "steps": [ - {"socket": "workspace.current", "save": "ws"}, - {"socket": "workspace.todo.add", "params": {"workspace_id": "${ws.workspace_id}", "text": "Ship the popover close fix"}}, - {"socket": "workspace.todo.add", "params": {"workspace_id": "${ws.workspace_id}", "text": "Check the sidebar after a click-away"}}, - {"wait": 1}, - {"shot": "checklist-summary"}, - {"click": {"id": "SidebarChecklistSummaryLine"}}, - {"wait": 1}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, - {"shot": "popover-open"}, - {"clickAt": {"x": 0.9, "y": 0.85}}, - {"wait": 2}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, - {"shot": "after-click-away"}, - {"click": {"id": "SidebarChecklistSummaryLine"}}, - {"wait": 1}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, - {"shot": "reopened-after-click-away"}, - {"click": {"id": "SidebarChecklistSummaryLine"}}, - {"wait": 2}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, - {"shot": "after-toggle-close"}, - {"click": {"id": "SidebarChecklistSummaryLine"}}, - {"wait": 1}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}}, - {"shot": "reopened-after-toggle"}, - {"clickAt": {"x": 0.9, "y": 0.85}}, - {"wait": 2}, - {"expect": {"id": "SidebarWorkspaceChecklistPopover"}, "exists": false}, - {"shot": "end", "screen": true} + { + "socket": "workspace.current", + "save": "ws" + }, + { + "socket": "workspace.todo.add", + "params": { + "workspace_id": "${ws.workspace_id}", + "text": "Ship the popover close fix" + } + }, + { + "socket": "workspace.todo.add", + "params": { + "workspace_id": "${ws.workspace_id}", + "text": "Check the sidebar after a click-away" + } + }, + { + "wait": 1 + }, + { + "shot": "checklist-summary" + }, + { + "tree": "checklist-summary" + }, + { + "tree": "sidebar-with-checklist" + }, + { + "clickAt": { + "x": 0.05, + "y": 0.084 + } + }, + { + "wait": 1 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + } + }, + { + "shot": "popover-open" + }, + { + "clickAt": { + "x": 0.9, + "y": 0.85 + } + }, + { + "wait": 2 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + }, + "exists": false + }, + { + "shot": "after-click-away" + }, + { + "clickAt": { + "x": 0.05, + "y": 0.084 + } + }, + { + "wait": 1 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + } + }, + { + "shot": "reopened-after-click-away" + }, + { + "clickAt": { + "x": 0.05, + "y": 0.084 + } + }, + { + "wait": 2 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + }, + "exists": false + }, + { + "shot": "after-toggle-close" + }, + { + "clickAt": { + "x": 0.05, + "y": 0.084 + } + }, + { + "wait": 1 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + } + }, + { + "shot": "reopened-after-toggle" + }, + { + "clickAt": { + "x": 0.9, + "y": 0.85 + } + }, + { + "wait": 2 + }, + { + "expect": { + "id": "SidebarWorkspaceChecklistPopover" + }, + "exists": false + }, + { + "shot": "end", + "screen": true + } ] }