From 9ab1c19c7594a22c99c68838d8d1b1cbdb649bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A4=D0=B5=D0=B4=D0=BE=D1=80=20=D0=91=D0=B0=D1=82=D0=BE?= =?UTF-8?q?=D0=BD=D0=BE=D0=B3=D0=BE=D0=B2?= Date: Fri, 21 Aug 2026 12:04:26 +0300 Subject: [PATCH 1/2] test(agent): cover Agent Inbox popover presentation and lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part 2 of #1491. Extracts the popover presentation workflow into a testable coordinator plus a pure state machine, and covers the routing lifecycle the toolbar, ⇧⌘I, the View menu and the Dock all share. Three defects found by review are fixed here. A deferred binding write was cancelled by its own synchronous effect, so a served request opened the Inbox with `isPresented` false and the next render closed it. A request queued for a host whose anchor never mounts waited forever, so ⇧⌘I did nothing and the Inbox later opened by itself; it is now bounded and falls back to Welcome after retiring the request. `anchorWindowDidChange` presented synchronously from `viewDidMoveToWindow`, before layout gave the anchor a non-zero bounds. The test fixture now reproduces SwiftUI's `@State` read-back semantics rather than reading a plain stored value; that change immediately exposed two of this branch's own new tests as staging states SwiftUI would never produce. Criteria 6 and 13 are not met and are not claimed: a minimized project window is bypassed rather than restored (#1507), and focus restoration has no production code and no test. Refs #1491 --- .github/workflows/ci.yml | 6 +- Pine/Agent/AgentInboxPopoverCoordinator.swift | 428 ++++++++++ .../AgentInboxPopoverPresentationState.swift | 195 +++++ Pine/Agent/AgentInboxPopoverPresenter.swift | 264 +++--- .../AgentInboxPresentationCoordinator.swift | 289 +++++++ Pine/PineApp.swift | 104 +-- PineTests/AgentInboxHostOptionsTests.swift | 9 +- .../AgentInboxPopoverCoordinatorTests.swift | 551 ++++++++++++ ...ntInboxPopoverPresentationStateTests.swift | 570 +++++++++++++ PineTests/AgentInboxPopoverRouterTests.swift | 392 ++++++++- .../AgentInboxPopoverSystemObjectsTests.swift | 292 +++++++ ...entInboxPresentationCoordinatorTests.swift | 783 ++++++++++++++++++ .../AgentInboxToolbarButtonTests.swift | 139 +++- 13 files changed, 3788 insertions(+), 234 deletions(-) create mode 100644 Pine/Agent/AgentInboxPopoverCoordinator.swift create mode 100644 Pine/Agent/AgentInboxPopoverPresentationState.swift create mode 100644 Pine/Agent/AgentInboxPresentationCoordinator.swift create mode 100644 PineTests/AgentInboxPopoverCoordinatorTests.swift create mode 100644 PineTests/AgentInboxPopoverPresentationStateTests.swift create mode 100644 PineTests/AgentInboxPopoverSystemObjectsTests.swift create mode 100644 PineTests/AgentInboxPresentationCoordinatorTests.swift diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d64a1ad7..eab79d5c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -592,7 +592,7 @@ jobs: -only-testing:PineUITests/EditorTabNavigationTests -only-testing:PineUITests/DiffNavigationUITests -only-testing:PineUITests/AccessibilityLocalizationSmokeTests - # Shard 4 — Editor Chrome (32 tests) + # Shard 4 — Editor Chrome (35 tests) - shard-name: "Editor Chrome" test-classes: >- -only-testing:PineUITests/EditorWindowTests @@ -600,6 +600,8 @@ jobs: -only-testing:PineUITests/BranchSwitcherTests -only-testing:PineUITests/CheckForUpdatesTests -only-testing:PineUITests/SplitPaneRoutingUITests + -only-testing:PineUITests/BlameViewTests + -only-testing:PineUITests/ToggleCommentTests # Shard 5 — Files & Save (34 tests, release smoke skips outside release CI) - shard-name: "Files & Save" test-classes: >- @@ -630,9 +632,7 @@ jobs: -only-testing:PineUITests/InlineRenameAlignmentTests -only-testing:PineUITests/LineNumberGutterUITests -only-testing:PineUITests/SettingsUITests - -only-testing:PineUITests/BlameViewTests -only-testing:PineUITests/HCLFormatOnSaveTests - -only-testing:PineUITests/ToggleCommentTests steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 diff --git a/Pine/Agent/AgentInboxPopoverCoordinator.swift b/Pine/Agent/AgentInboxPopoverCoordinator.swift new file mode 100644 index 00000000..56420bb4 --- /dev/null +++ b/Pine/Agent/AgentInboxPopoverCoordinator.swift @@ -0,0 +1,428 @@ +// +// AgentInboxPopoverCoordinator.swift +// Pine +// +// The anchor-side glue between SwiftUI, the router, and one NSPopover +// (#1491). +// + +import AppKit +import SwiftUI + +/// The single AppKit object an Inbox anchor owns, reduced to what the anchor +/// actually does with it. +/// +/// Substituting it is what makes the close orderings verifiable. They are +/// runloop interleavings around an animated close — not window-server +/// behavior — and every one of them can leave the anchor in a state where the +/// window refuses to open the Inbox ever again. +@MainActor +protocol AgentInboxPopoverHandle: AnyObject { + /// AppKit's own answer. `NSPopover.isShown` already reads `false` part way + /// through a close animation, so no caller may read it as "gone". + var isPopoverVisible: Bool { get } + func showPopover(from anchor: NSView) + func closePopover() +} + +extension NSPopover: AgentInboxPopoverHandle { + var isPopoverVisible: Bool { isShown } + + func showPopover(from anchor: NSView) { + show(relativeTo: anchor.bounds, of: anchor, preferredEdge: .minY) + } + + func closePopover() { + performClose(nil) + } +} + +/// Reconciles one window's Inbox anchor: the SwiftUI binding, the router's +/// requests, and AppKit's own popover. +/// +/// The rule itself lives in `AgentInboxPopoverPresentationState`; this type +/// owns only the wiring — when a resolution may touch SwiftUI, which AppKit +/// notification belongs to which popover, and when a verdict has to be +/// re-derived instead of replayed. +@MainActor +final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, + AgentInboxPopoverPresenting { + /// Everything a popover needs that the anchor learns from SwiftUI. + struct Context { + let registry: ProjectRegistry? + let openProjectWindow: ((URL) -> Void)? + let reduceMotion: Bool + let delegate: any NSPopoverDelegate + let onDismiss: @MainActor () -> Void + } + + /// Builds — but does not show — the popover for one presentation. + /// Returning `nil` means the anchor is not ready and nothing is recorded. + typealias PopoverFactory = @MainActor ( + AgentInboxPopoverAnchorView, + Context + ) -> (any AgentInboxPopoverHandle)? + + private typealias State = AgentInboxPopoverPresentationState + + private let router: AgentInboxPopoverRouter + private let makePopover: PopoverFactory + private weak var anchor: AgentInboxPopoverAnchorView? + private weak var registeredWindow: NSWindow? + private var isPresented: Binding? + private var registry: ProjectRegistry? + private var openProjectWindow: ((URL) -> Void)? + private var reduceMotion = false + private var state = State() + private var popover: (any AgentInboxPopoverHandle)? + /// How many `.close` resolutions have landed on an invisible popover while + /// this anchor still believes AppKit is animating that popover away. + private var unreportedCloseCount = 0 + /// The last value this anchor wrote into the binding, held only until the + /// next update pass makes SwiftUI's own value authoritative again. + private var lastWrittenIsPresented: Bool? + + /// How many may land before the anchor stops waiting for a close + /// notification that is not coming. + /// + /// ``AgentInboxPopoverPresentationState/isClosing`` is retired by exactly + /// one thing — `popoverDidClose` — and while it stands every entry point + /// resolves to `.inert`. Holding the reference through the animation is + /// deliberate: dropping it leaves that notification with no sender to + /// match, which is the regression `closePopover()` below is written + /// against. But it also means one notification AppKit never posts costs + /// this window its Inbox for as long as it stays open, with nothing left + /// able to release it — the toolbar button, ⇧⌘I, the View menu and the + /// Dock all go quiet. + /// + /// No production path was found where AppKit skips the notification, so + /// this bounds a latent single point of failure rather than fixing a + /// reproduced one. That is why it is generous: a close that is genuinely + /// still animating is measured in a handful of SwiftUI passes, not dozens. + private static let unreportedCloseTolerance = 32 + + private var isPopoverShown: Bool { + popover?.isPopoverVisible == true + } + + /// What the anchor believes SwiftUI's presentation state is right now. + /// + /// Its own last write outranks the binding until the next update pass. + /// SwiftUI hands `updateNSView` a `Binding` over the snapshot taken for + /// *that* pass; reading it back between passes answers from the snapshot, + /// not from a write made since. Every deferred hop in this type reads this + /// property a whole runloop turn after the pass that handed the binding + /// over, so without the memory a hop can read back the value it just + /// replaced — `settledClose` would see a binding that is still up, decide + /// nothing needs writing, and let the next pass reopen the popover the user + /// dismissed. Escape would read as opening the Inbox again. + private var bindingIsPresented: Bool { + lastWrittenIsPresented ?? (isPresented?.wrappedValue == true) + } + + init( + router: AgentInboxPopoverRouter = .shared, + makePopover: @escaping PopoverFactory = AgentInboxPopoverCoordinator + .makeSystemPopover + ) { + self.router = router + self.makePopover = makePopover + } + + /// The production popover: a transient, Liquid-Glass-friendly `NSPopover` + /// hosting `AgentInboxView`. + static func makeSystemPopover( + anchor: AgentInboxPopoverAnchorView, + context: Context + ) -> (any AgentInboxPopoverHandle)? { + guard let registry = context.registry, + let openProjectWindow = context.openProjectWindow else { + return nil + } + let contentSize = NSSize(width: 520, height: 540) + let rootView = AgentInboxView( + registry: registry, + onDismiss: context.onDismiss, + openProjectWindow: openProjectWindow + ) + let hostingController = NSHostingController(rootView: rootView) + hostingController.preferredContentSize = contentSize + + let popover = NSPopover() + popover.behavior = .transient + popover.animates = !context.reduceMotion + popover.contentSize = contentSize + popover.contentViewController = hostingController + popover.delegate = context.delegate + return popover + } + + func attach(to anchor: AgentInboxPopoverAnchorView) { + self.anchor = anchor + updateRegistration(for: anchor.window) + } + + func update( + anchor: AgentInboxPopoverAnchorView, + isPresented: Binding, + registry: ProjectRegistry, + openProjectWindow: @escaping (URL) -> Void, + reduceMotion: Bool + ) { + self.anchor = anchor + self.isPresented = isPresented + self.registry = registry + self.openProjectWindow = openProjectWindow + self.reduceMotion = reduceMotion + // SwiftUI has just handed over a fresh snapshot, so its value outranks + // anything this anchor wrote against the previous one. + lastWrittenIsPresented = nil + updateRegistration(for: anchor.window) + + let resolution = state.viewDidUpdate( + bindingIsPresented: isPresented.wrappedValue, + isPopoverShown: isPopoverShown + ) + // The AppKit half is safe here: it touches no SwiftUI state, and + // delaying a close would leave the popover on screen for a whole turn + // after the binding said it was gone. + performEffect(resolution.effect) + guard resolution.bindingIsPresented != nil else { return } + // The SwiftUI half is not. `update` runs inside a live view update + // pass, and writing `@State` there re-enters it — the mutation + // AGENTS.md requires observers to defer. + NativeCommandDelivery.deferToNextMainRunLoop { [weak self] in + self?.writeSettledUpdate() + } + } + + /// The SwiftUI half of an update pass, one runloop turn later. + /// + /// The verdict is not carried across the hop: a turn is long enough for the + /// user to close the Inbox by hand. But re-deriving it from scratch gets + /// the *only* case that defers exactly backwards. `viewDidUpdate` returns a + /// write in one situation — a router request outranking a lowered binding — + /// and the `.present` that comes with it runs **synchronously**, before + /// this hop. A show that succeeds consumes the outstanding request that was + /// the whole premise of the write, so asking the rule again reads "binding + /// down, nothing pending", answers `.close`, and writes nothing at all. The + /// Inbox is then visible with `isPresented == false`, and the next pass + /// closes it: it blinks open and shuts, and the ⇧⌘I that opened it reads as + /// swallowed. The write survived only when the show *failed*. + /// + /// So the popover actually on screen is asked first, and only a hop that + /// did not leave one behind falls back to the rule. + private func writeSettledUpdate() { + guard !isPopoverShown else { + writeBinding(bindingIsPresented ? nil : true) + return + } + writeBinding(state.viewDidUpdate( + bindingIsPresented: bindingIsPresented, + isPopoverShown: false + ).bindingIsPresented) + } + + /// The anchor moved into — or between — windows. + /// + /// This runs inside AppKit's `viewDidMoveToWindow`, which fires before + /// layout: the anchor's `bounds` is still zero, and a popover shown + /// relative to it hangs off the window's origin instead of the toolbar + /// button. `AgentInboxPopoverRouter` defers its own hand-off for exactly + /// that reason, and a show started here needs the same turn. The verdict is + /// re-derived on arrival rather than replayed, because a turn is long + /// enough for the anchor to move again or for the binding to come down. + /// + /// Only `.present` waits. A `.close` cannot arrive from this entry point — + /// the rule returns `.unchanged` when nothing wants the Inbox — and holding + /// one back would leave a popover attached to a window the anchor has + /// already left. + func anchorWindowDidChange(_ anchor: AgentInboxPopoverAnchorView) { + self.anchor = anchor + updateRegistration(for: anchor.window) + let resolution = state.anchorWindowDidChange( + bindingIsPresented: bindingIsPresented, + isPopoverShown: isPopoverShown + ) + guard resolution.effect == .present else { + apply(resolution) + return + } + NativeCommandDelivery.deferToNextMainRunLoop { [weak self] in + guard let self else { return } + self.apply(self.state.anchorWindowDidChange( + bindingIsPresented: self.bindingIsPresented, + isPopoverShown: self.isPopoverShown + )) + } + } + + func presentAgentInbox() { + apply(state.routerRequestedPresentation( + bindingIsPresented: bindingIsPresented, + isPopoverShown: isPopoverShown + )) + } + + func detach() { + if let registeredWindow { + router.unregister(self, from: registeredWindow) + } + registeredWindow = nil + apply(state.anchorDidDetach()) + anchor = nil + } + + // MARK: - NSPopoverDelegate + + func popoverWillClose(_ notification: Notification) { + popoverWillClose(sender: notification.object as AnyObject?) + } + + func popoverDidClose(_ notification: Notification) { + popoverDidClose(sender: notification.object as AnyObject?) + } + + /// A close has begun. + /// + /// The popover is `.transient`, so Escape and a click outside are handled + /// by AppKit without asking the anchor first. Without this callback the + /// whole close animation runs with the state machine believing nothing is + /// closing, and a request that lands inside it builds a *second* popover + /// on the same anchor while the first is still leaving — with only one + /// strong reference between them. + func popoverWillClose(sender: AnyObject?) { + guard let popover, sender === popover else { return } + state.popoverWillClose() + } + + /// A close has finished. + func popoverDidClose(sender: AnyObject?) { + // A popover this anchor already replaced can still report its own + // close. Acting on it would drop the reference to the live one, + // leaving an Inbox that nothing can close. + guard let current = popover, sender === current else { return } + retireClosedPopover() + } + + /// Lets go of a popover whose close is over. + /// + /// Retired synchronously, not with the SwiftUI write below: the in-flight + /// close is what makes every entry point inert, and every extra hop it + /// stands is a hop in which a request is held against a close that is + /// already finished. + private func retireClosedPopover() { + popover = nil + unreportedCloseCount = 0 + state.popoverDidClose() + // `@State` written inside AppKit's own notification re-enters the live + // view update, so the SwiftUI half lands a turn later — and is derived + // there, because by then a newer request may have opened a new popover + // that this verdict would lower the binding on. + NativeCommandDelivery.deferToNextMainRunLoop { [weak self] in + guard let self else { return } + self.apply(self.state.settledClose( + bindingIsPresented: self.bindingIsPresented, + isPopoverShown: self.isPopoverShown + )) + } + } + + // MARK: - Applying resolutions + + /// Writes the SwiftUI binding first, then performs the AppKit effect, so + /// the popover is never created against a binding that still reads + /// `false`. + private func apply(_ resolution: State.Resolution) { + writeBinding(resolution.bindingIsPresented) + performEffect(resolution.effect) + } + + private func writeBinding(_ value: Bool?) { + guard let value else { return } + lastWrittenIsPresented = value + isPresented?.wrappedValue = value + } + + private func performEffect(_ effect: State.Effect) { + switch effect { + case .present: + presentIfReady() + case .close: + closePopover() + case .inert: + break + } + } + + private func updateRegistration(for window: NSWindow?) { + guard registeredWindow !== window else { return } + if let registeredWindow { + router.unregister(self, from: registeredWindow) + } + registeredWindow = window + if let window { + router.register(self, for: window) + } + } + + private func presentIfReady() { + guard !isPopoverShown, + let anchor, + anchor.window != nil, + let handle = makePopover(anchor, Context( + registry: registry, + openProjectWindow: openProjectWindow, + reduceMotion: reduceMotion, + delegate: self, + onDismiss: { [weak self] in self?.dismiss() } + )) else { return } + popover = handle + unreportedCloseCount = 0 + state.popoverWillShow() + handle.showPopover(from: anchor) + } + + private func dismiss() { + apply(state.contentRequestedDismiss( + bindingIsPresented: bindingIsPresented + )) + } + + private func closePopover() { + guard let popover else { return } + guard popover.isPopoverVisible else { + // A close this anchor already started is still animating: AppKit + // reports `isShown == false` well before it posts the close + // notification, and SwiftUI re-renders often enough that a second + // `.close` inside that window is all but guaranteed. Dropping the + // reference here would leave that notification with no sender to + // match — and the in-flight close, which only that notification + // clears, armed forever. Every later request then resolves to + // `.inert`: the toolbar button, ⇧⌘I, the View menu and the Dock + // all stop opening the Inbox in this window until it is closed. + guard state.isClosing else { + self.popover = nil + unreportedCloseCount = 0 + return + } + // …but the wait cannot be unconditional. Holding the reference is + // worth something only while a notification can still arrive to + // match it, and holding the close costs this window the Inbox. Past + // the tolerance the anchor concludes the notification is not coming + // and retires the close itself, on exactly the terms the real one + // would have. + unreportedCloseCount += 1 + guard unreportedCloseCount >= Self.unreportedCloseTolerance else { + return + } + retireClosedPopover() + return + } + // `performClose` animates, so the window stays busy until the close + // notification lands. The state machine holds any request that + // arrives in between rather than racing it. + state.popoverWillClose() + popover.closePopover() + } +} diff --git a/Pine/Agent/AgentInboxPopoverPresentationState.swift b/Pine/Agent/AgentInboxPopoverPresentationState.swift new file mode 100644 index 00000000..095e86dc --- /dev/null +++ b/Pine/Agent/AgentInboxPopoverPresentationState.swift @@ -0,0 +1,195 @@ +// +// AgentInboxPopoverPresentationState.swift +// Pine +// +// Reconciliation rules for the Agent Inbox popover's three truths (#1491). +// + +import Foundation + +/// Reconciles the three independent truths about Agent Inbox popover +/// visibility so they converge on one visible state: +/// +/// - the SwiftUI `isPresented` binding driven by the toolbar and Welcome +/// buttons; +/// - a router request, which can arrive before the anchor has a window; +/// - AppKit's own popover, which a transient dismissal — an outside click or +/// Escape — closes without telling SwiftUI. +/// +/// Holding the rule in a value type is what makes duplicate-request, +/// detached-anchor, and outside-dismiss behavior verifiable without a window +/// server, and keeps the anchor coordinator free of ad-hoc boolean juggling. +struct AgentInboxPopoverPresentationState: Equatable { + /// What the anchor must do to its AppKit popover. + enum Effect: Equatable { + case present + case close + /// Neither AppKit nor SwiftUI needs to change. + case inert + } + + struct Resolution: Equatable { + let effect: Effect + /// Non-nil when the SwiftUI binding has to be written to converge. + let bindingIsPresented: Bool? + + static let unchanged = Resolution( + effect: .inert, + bindingIsPresented: nil + ) + } + + /// A router request this anchor accepted but has not yet turned into a + /// visible popover, because its window had not mounted yet or because the + /// previous popover was still closing. + private(set) var hasUnservedRouterRequest = false + + /// True from the moment a close of the popover begins until its close + /// notification arrives. + /// + /// `NSPopover.performClose` animates unless Reduce Motion is on, so there + /// is a real window in which the popover is neither usable nor gone. A + /// second `show` inside it would orphan the one still leaving. + /// + /// Closes this anchor did not start count too. The popover is `.transient`, + /// so Escape and a click outside are handled by AppKit alone; the anchor + /// learns about them from `popoverWillClose`, and without that the whole + /// animation would run with this flag down. + private(set) var isClosing = false + + /// A request delivered by `AgentInboxPopoverRouter`. + /// + /// A repeated request while the popover is already shown is absorbed: a + /// second `show` would orphan the first popover's hosting controller and + /// leave an Inbox nobody can dismiss. Absorbed means *served*, so no + /// unserved marker is left behind — leaving one would make the next + /// SwiftUI update pass override a binding the user just lowered, and the + /// toolbar button would refuse to close the Inbox it opened. + mutating func routerRequestedPresentation( + bindingIsPresented: Bool, + isPopoverShown: Bool + ) -> Resolution { + hasUnservedRouterRequest = !isPopoverShown || isClosing + return Resolution( + effect: isPopoverShown || isClosing ? .inert : .present, + // The binding lags the router, so converge it here; the toolbar + // button's own highlight reads from it. + bindingIsPresented: bindingIsPresented ? nil : true + ) + } + + /// A SwiftUI update pass. This is the only entry point that may close the + /// popover on its own, because it is the only one that observes the + /// binding turning false. + func viewDidUpdate( + bindingIsPresented: Bool, + isPopoverShown: Bool + ) -> Resolution { + guard bindingIsPresented || hasUnservedRouterRequest else { + return Resolution(effect: .close, bindingIsPresented: nil) + } + return Resolution( + effect: isPopoverShown || isClosing ? .inert : .present, + bindingIsPresented: + hasUnservedRouterRequest && !bindingIsPresented ? true : nil + ) + } + + /// The anchor moved into — or between — windows. + /// + /// Deliberately never writes the binding: this runs inside AppKit's + /// `viewDidMoveToWindow`, and SwiftUI state written there would re-enter + /// the live view update. + func anchorWindowDidChange( + bindingIsPresented: Bool, + isPopoverShown: Bool + ) -> Resolution { + guard bindingIsPresented || hasUnservedRouterRequest else { + return .unchanged + } + return Resolution( + effect: isPopoverShown || isClosing ? .inert : .present, + bindingIsPresented: nil + ) + } + + /// The anchor created and showed a popover, serving the outstanding + /// request. + mutating func popoverWillShow() { + hasUnservedRouterRequest = false + isClosing = false + } + + /// A close of the popover has begun — either because the anchor asked for + /// one, or because AppKit dismissed the `.transient` popover itself. + /// Nothing may be shown in this window until the close notification + /// confirms it is gone. + mutating func popoverWillClose() { + isClosing = true + } + + /// The Inbox content dismissed itself after a successful navigation or + /// recovery. + mutating func contentRequestedDismiss( + bindingIsPresented: Bool + ) -> Resolution { + hasUnservedRouterRequest = false + return Resolution( + effect: .close, + bindingIsPresented: bindingIsPresented ? false : nil + ) + } + + /// AppKit finished closing the popover — an outside click, Escape, or a + /// programmatic close. + /// + /// The window is free the moment this arrives, so the in-flight close is + /// retired **here**, synchronously, and never together with the SwiftUI + /// half below. `isClosing` is the flag that makes every entry point inert; + /// leaving it armed for even one more hop means a request that lands in + /// that gap is held against a close that is already over, with nothing + /// left to release it. + /// + /// A request that arrived during a close this anchor announced survives + /// it: it was deliberately held back rather than dropped, so the window + /// can open the Inbox now. A request queued against an anchor that never + /// showed anything is stale and goes with the close. + mutating func popoverDidClose() { + hasUnservedRouterRequest = isClosing && hasUnservedRouterRequest + isClosing = false + } + + /// What a finished close means for SwiftUI, derived at the moment that + /// write actually reaches it. + /// + /// ``popoverDidClose()`` runs inside AppKit's own close notification, so + /// the `@State` write it implies has to land a runloop turn later. A whole + /// turn is enough for a newer request to open a new popover; replaying the + /// verdict that was true when the old one vanished would lower the binding + /// under a visible Inbox, and the next update pass would close it. + func settledClose( + bindingIsPresented: Bool, + isPopoverShown: Bool + ) -> Resolution { + // Something newer already owns the popover and wrote its own binding. + guard !isPopoverShown, !isClosing else { return .unchanged } + guard hasUnservedRouterRequest else { + return Resolution( + effect: .inert, + bindingIsPresented: bindingIsPresented ? false : nil + ) + } + return Resolution( + effect: .present, + bindingIsPresented: bindingIsPresented ? nil : true + ) + } + + /// The anchor left the view hierarchy. A detached anchor must not keep a + /// request that its replacement would then serve a second time. + mutating func anchorDidDetach() -> Resolution { + hasUnservedRouterRequest = false + isClosing = false + return Resolution(effect: .close, bindingIsPresented: nil) + } +} diff --git a/Pine/Agent/AgentInboxPopoverPresenter.swift b/Pine/Agent/AgentInboxPopoverPresenter.swift index e57209ed..95ab521b 100644 --- a/Pine/Agent/AgentInboxPopoverPresenter.swift +++ b/Pine/Agent/AgentInboxPopoverPresenter.swift @@ -24,28 +24,77 @@ final class AgentInboxPopoverRouter { case queued } + /// How a request queued for a not-yet-mounted anchor reaches that anchor + /// once it registers. + typealias QueuedDelivery = + @MainActor (@escaping @MainActor () -> Void) -> Void + static let shared = AgentInboxPopoverRouter() - private final class WeakPresenter { - weak var value: AgentInboxPopoverPresenting? + /// Both sides are held weakly. `ObjectIdentifier` is unique only among + /// live objects, so a released window's address can be handed to a new + /// one; the host is re-checked by identity before its anchor is used. + private final class Registration { + weak var host: AnyObject? + weak var presenter: AgentInboxPopoverPresenting? - init(_ value: AgentInboxPopoverPresenting) { - self.value = value + init(host: AnyObject, presenter: AgentInboxPopoverPresenting) { + self.host = host + self.presenter = presenter } } - private var presenters: [ObjectIdentifier: WeakPresenter] = [:] + private var registrations: [ObjectIdentifier: Registration] = [:] private weak var pendingHost: AnyObject? + /// Retires a hand-off that has already been scheduled. + /// + /// `register` turns a queued request into a closure on the next runloop + /// turn and clears `pendingHost` in the same breath, so for a whole turn + /// the request is in flight and no longer reachable through `pendingHost`. + /// A workflow that retires the old request and starts a new one inside + /// that turn would otherwise open the Inbox in two windows at once. + private var deliveryGeneration = 0 + private let deliverQueuedRequest: QueuedDelivery + + /// - Parameter deliverQueuedRequest: escape hatch used by tests to make + /// the hand-off observable in one turn. Production always defers. + init( + deliverQueuedRequest: @escaping QueuedDelivery = { operation in + NativeCommandDelivery.deferToNextMainRunLoop(operation) + } + ) { + self.deliverQueuedRequest = deliverQueuedRequest + } func register( _ presenter: AgentInboxPopoverPresenting, for host: AnyObject ) { - removeReleasedPresenters() - presenters[ObjectIdentifier(host)] = WeakPresenter(presenter) + removeReleasedRegistrations() + registrations[ObjectIdentifier(host)] = Registration( + host: host, + presenter: presenter + ) guard pendingHost === host else { return } pendingHost = nil - presenter.presentAgentInbox() + let generation = deliveryGeneration + // Anchors register from `viewDidMoveToWindow` and `updateNSView`. + // Presenting synchronously there writes SwiftUI state inside a live + // update pass — the mutation AGENTS.md requires observers to defer — + // and shows the popover before layout has given the anchor a non-zero + // `bounds`, which would pin it to the window's origin. + deliverQueuedRequest { [weak self, weak host] in + guard let self, + let host, + self.deliveryGeneration == generation else { return } + // The hand-off is bound to the *window*, not to the coordinator + // that owned its anchor when it was scheduled. SwiftUI may rebuild + // that coordinator inside the deferral, and a request captured + // against the demounted one has nobody left to re-send it: this + // resolves the window's current anchor instead, and re-queues the + // request when there is none. + self.requestPresentation(in: host) + } } func unregister( @@ -53,15 +102,50 @@ final class AgentInboxPopoverRouter { from host: AnyObject ) { let key = ObjectIdentifier(host) - guard presenters[key]?.value === presenter else { return } - presenters.removeValue(forKey: key) + guard let registration = registrations[key], + registration.host === host, + registration.presenter === presenter else { return } + registrations.removeValue(forKey: key) + } + + /// Retires a request that is still waiting for its window's anchor. + /// + /// A queued request has no expiry of its own. Nothing else ever clears it, + /// so a request left over for the singleton Welcome window stays armed on + /// this shared object until that window next mounts an anchor — which can + /// be minutes later and reads to the user as the Inbox opening by itself. + /// The presentation workflow retires the previous request before starting + /// a new one, which is what keeps exactly one in flight. + func cancelQueuedRequest() { + pendingHost = nil + // A request that `register` already scheduled is past `pendingHost`. + // Retiring only the queue would let it land a turn later, in a second + // window, on behalf of a request the caller has just superseded. + deliveryGeneration &+= 1 + } + + /// True while a request addressed to `host` is still waiting for that + /// host's anchor to register. + /// + /// A queued request has no expiry of its own, so the presentation workflow + /// polls this to bound how long it waits for a host whose anchor may never + /// mount at all. `false` covers both endings that retire a wait: the anchor + /// registered and the hand-off is under way, or a newer request superseded + /// this one. + func hasQueuedRequest(for host: AnyObject) -> Bool { + pendingHost === host } @discardableResult func requestPresentation(in host: AnyObject) -> RequestResult { - removeReleasedPresenters() + removeReleasedRegistrations() + // A newer request supersedes any hand-off still in flight for the one + // before it, whichever window that one was addressed to. + deliveryGeneration &+= 1 let key = ObjectIdentifier(host) - guard let presenter = presenters[key]?.value else { + guard let registration = registrations[key], + registration.host === host, + let presenter = registration.presenter else { pendingHost = host return .queued } @@ -70,8 +154,10 @@ final class AgentInboxPopoverRouter { return .presented } - private func removeReleasedPresenters() { - presenters = presenters.filter { $0.value.value != nil } + private func removeReleasedRegistrations() { + registrations = registrations.filter { + $0.value.host != nil && $0.value.presenter != nil + } } } @@ -79,7 +165,7 @@ final class AgentInboxPopoverRouter { /// `NSPopover` a stable attachment point in both regular content and a /// Liquid Glass toolbar on macOS 26. @MainActor -private final class AgentInboxPopoverAnchorView: NSView { +final class AgentInboxPopoverAnchorView: NSView { var onWindowChange: ((AgentInboxPopoverAnchorView) -> Void)? override func viewDidMoveToWindow() { @@ -97,8 +183,8 @@ private struct AgentInboxPopoverAnchor: NSViewRepresentable { @Environment(\.openWindow) private var openWindow @Environment(\.accessibilityReduceMotion) private var reduceMotion - func makeCoordinator() -> Coordinator { - Coordinator(router: .shared) + func makeCoordinator() -> AgentInboxPopoverCoordinator { + AgentInboxPopoverCoordinator(router: .shared) } func makeNSView(context: Context) -> AgentInboxPopoverAnchorView { @@ -129,153 +215,11 @@ private struct AgentInboxPopoverAnchor: NSViewRepresentable { static func dismantleNSView( _ nsView: AgentInboxPopoverAnchorView, - coordinator: Coordinator + coordinator: AgentInboxPopoverCoordinator ) { nsView.onWindowChange = nil coordinator.detach() } - - @MainActor - final class Coordinator: NSObject, NSPopoverDelegate, - AgentInboxPopoverPresenting { - private let router: AgentInboxPopoverRouter - private weak var anchor: AgentInboxPopoverAnchorView? - private weak var registeredWindow: NSWindow? - private var isPresented: Binding? - private var registry: ProjectRegistry? - private var openProjectWindow: ((URL) -> Void)? - private var reduceMotion = false - private var routerRequestedPresentation = false - private var popover: NSPopover? - - init(router: AgentInboxPopoverRouter) { - self.router = router - } - - func attach(to anchor: AgentInboxPopoverAnchorView) { - self.anchor = anchor - updateRegistration(for: anchor.window) - } - - func update( - anchor: AgentInboxPopoverAnchorView, - isPresented: Binding, - registry: ProjectRegistry, - openProjectWindow: @escaping (URL) -> Void, - reduceMotion: Bool - ) { - self.anchor = anchor - self.isPresented = isPresented - self.registry = registry - self.openProjectWindow = openProjectWindow - self.reduceMotion = reduceMotion - updateRegistration(for: anchor.window) - - if isPresented.wrappedValue || routerRequestedPresentation { - if routerRequestedPresentation && !isPresented.wrappedValue { - isPresented.wrappedValue = true - } - presentIfReady() - } else { - closePopover() - } - } - - func anchorWindowDidChange(_ anchor: AgentInboxPopoverAnchorView) { - self.anchor = anchor - updateRegistration(for: anchor.window) - if isPresented?.wrappedValue == true - || routerRequestedPresentation { - presentIfReady() - } - } - - func presentAgentInbox() { - routerRequestedPresentation = true - if let isPresented, !isPresented.wrappedValue { - isPresented.wrappedValue = true - } - presentIfReady() - } - - func detach() { - if let registeredWindow { - router.unregister(self, from: registeredWindow) - } - registeredWindow = nil - routerRequestedPresentation = false - closePopover() - anchor = nil - } - - private func updateRegistration(for window: NSWindow?) { - guard registeredWindow !== window else { return } - if let registeredWindow { - router.unregister(self, from: registeredWindow) - } - registeredWindow = window - if let window { - router.register(self, for: window) - } - } - - private func presentIfReady() { - guard popover?.isShown != true, - let anchor, - anchor.window != nil, - let registry, - let openProjectWindow else { return } - - let contentSize = NSSize(width: 520, height: 540) - let rootView = AgentInboxView( - registry: registry, - onDismiss: { [weak self] in self?.dismiss() }, - openProjectWindow: openProjectWindow - ) - let hostingController = NSHostingController(rootView: rootView) - hostingController.preferredContentSize = contentSize - - let popover = NSPopover() - popover.behavior = .transient - popover.animates = !reduceMotion - popover.contentSize = contentSize - popover.contentViewController = hostingController - popover.delegate = self - self.popover = popover - routerRequestedPresentation = false - popover.show( - relativeTo: anchor.bounds, - of: anchor, - preferredEdge: .minY - ) - } - - private func dismiss() { - routerRequestedPresentation = false - if isPresented?.wrappedValue == true { - isPresented?.wrappedValue = false - } - closePopover() - } - - private func closePopover() { - guard let popover else { return } - if popover.isShown { - popover.performClose(nil) - } else { - self.popover = nil - } - } - - func popoverDidClose(_ notification: Notification) { - popover = nil - routerRequestedPresentation = false - guard isPresented?.wrappedValue == true else { return } - DispatchQueue.main.async { [weak self] in - self?.isPresented?.wrappedValue = false - } - } - } } private struct AgentInboxPopoverModifier: ViewModifier { diff --git a/Pine/Agent/AgentInboxPresentationCoordinator.swift b/Pine/Agent/AgentInboxPresentationCoordinator.swift new file mode 100644 index 00000000..b26827ea --- /dev/null +++ b/Pine/Agent/AgentInboxPresentationCoordinator.swift @@ -0,0 +1,289 @@ +// +// AgentInboxPresentationCoordinator.swift +// Pine +// +// The application-level Agent Inbox presentation workflow (#1491). +// + +import AppKit + +/// One window that can own the Agent Inbox popover, reduced to the operations +/// the presentation workflow performs on it. +/// +/// `NSWindow` conforms directly, so production routes through the very object +/// `AgentInboxPopoverRouter` keys presentation by. Tests substitute a +/// deterministic double, which is what makes restore-then-focus ordering +/// observable without a window server. +@MainActor +protocol AgentInboxHosting: AnyObject { + var isHostMiniaturized: Bool { get } + func restoreHostFromMiniaturized() + func focusHost() +} + +extension NSWindow: AgentInboxHosting { + var isHostMiniaturized: Bool { isMiniaturized } + + func restoreHostFromMiniaturized() { + deminiaturize(nil) + } + + func focusHost() { + makeKeyAndOrderFront(nil) + } +} + +/// One Agent Inbox host candidate paired with the window a winning decision +/// routes to. +/// +/// The pair is produced in a single pass so the pure rule in +/// ``AgentInboxHostRouting`` and the window it selects can never disagree +/// about the window list they were derived from. The host is expressed as +/// ``AgentInboxHosting`` rather than `NSWindow` so the restore-then-focus +/// ordering the workflow performs on it is observable without a window +/// server; `NSWindow` conforms, so production still routes through the very +/// object `AgentInboxPopoverRouter` keys presentation by. +struct AgentInboxHostOption { + let candidate: AgentInboxHostCandidate + let host: any AgentInboxHosting +} + +/// The window-system facts and effects the Agent Inbox presentation workflow +/// depends on. `AppDelegate` implements it over `NSApp` and `ProjectRegistry`. +@MainActor +protocol AgentInboxHostEnvironment: AnyObject { + /// Every window that could own the popover, in `NSApp.windows` order. + func agentInboxHostOptions() -> [AgentInboxHostOption] + /// Brings the application forward before any window work. + func activateApplicationForAgentInbox() + /// Starts creating the Welcome window. Synchronous, because the request + /// that needs it has already been accepted. + func createAgentInboxWelcomeHost() + /// Waits, bounded, for the created Welcome window's live visible owner. + func awaitAgentInboxWelcomeHost() async -> (any AgentInboxHosting)? + /// Breaks synchronous menu / notification / SwiftUI-button delivery before + /// the popover is asked to present. + func deliverAgentInboxRequest( + _ operation: @escaping @MainActor () -> Void + ) + /// Waits one polling interval before the workflow re-checks whether a + /// queued request's host has mounted the anchor it is addressed to. + func waitForAgentInboxAnchor() async +} + +/// Runs one Agent Inbox request end to end: choose a host window, restore and +/// focus it, and hand exactly one presentation request to the popover router. +/// +/// The workflow owns the policy — selection order, restore-before-present, and +/// the single-in-flight rule for a Welcome window that is still mounting. The +/// environment owns only the AppKit facts. +@MainActor +final class AgentInboxPresentationCoordinator { + enum Outcome: Equatable { + /// The request was handed to an already-existing host window. + case routedToExistingHost + /// No window could host the Inbox, so Welcome was created and this + /// single request waits for its anchor to mount. + case awaitingCreatedWelcomeHost + /// The environment was released before a host could be chosen. + case unavailable + } + + /// How many times a request handed to an *existing* host re-checks whether + /// that host has mounted its anchor before the workflow gives up on it. + /// Paired with ``AgentInboxHostEnvironment/waitForAgentInboxAnchor()``'s + /// interval this is the same order of wait `awaitVisibleWelcomeWindow()` + /// already spends on a window that is being created. + private static let queuedAnchorAttempts = 40 + + private let router: AgentInboxPopoverRouter + private weak var environment: (any AgentInboxHostEnvironment)? + private var pendingWelcomeHostTask: Task? + private var welcomeHostGeneration = 0 + private var pendingAnchorTask: Task? + private var anchorGeneration = 0 + + init( + router: AgentInboxPopoverRouter = .shared, + environment: any AgentInboxHostEnvironment + ) { + self.router = router + self.environment = environment + } + + /// True while a created-Welcome request is still waiting for its host. + var isAwaitingCreatedWelcomeHost: Bool { + pendingWelcomeHostTask != nil + } + + @discardableResult + func present() -> Outcome { + present(retriesRemaining: 1) + } + + /// - Parameter retriesRemaining: how many times a request whose chosen + /// window vanished, or produced no anchor, may re-run selection. Bounded + /// so a desktop that keeps losing its host cannot spin. + @discardableResult + private func present(retriesRemaining: Int) -> Outcome { + guard let environment else { return .unavailable } + environment.activateApplicationForAgentInbox() + // A newer request supersedes the one before it — the Welcome window + // still mounting, the existing host still being waited on, and any + // request already sitting on the router's queue — so at most one is + // ever in flight. + pendingWelcomeHostTask?.cancel() + pendingWelcomeHostTask = nil + pendingAnchorTask?.cancel() + pendingAnchorTask = nil + router.cancelQueuedRequest() + + let options = environment.agentInboxHostOptions() + let decision = AgentInboxHostRouting.decision( + among: options.map(\.candidate) + ) + if case .existingHost(let index) = decision, + options.indices.contains(index) { + let host = options[index].host + prepare(host, in: environment) + // The host is held weakly across the deferral: delivery happens a + // whole runloop turn later, and keeping a closing `NSWindow` alive + // for that turn is precisely the lifetime this workflow must not + // extend. A host that does not answer is retried, never dropped. + environment.deliverAgentInboxRequest { [weak self, weak host] in + guard let self else { return } + guard let host else { + // The chosen window died between selection and delivery. + // There is nothing left to wait for, so selection re-runs. + guard retriesRemaining > 0 else { return } + self.present(retriesRemaining: retriesRemaining - 1) + return + } + guard self.router.requestPresentation(in: host) == .queued + else { return } + self.awaitQueuedAnchor(on: host) + } + return .routedToExistingHost + } + + return createWelcomeHost(in: environment) + } + + /// Waits, bounded, for an existing host to mount the anchor a queued + /// request is addressed to. + /// + /// `.queued` is a healthy answer for a window that is alive but has not run + /// `updateNSView` yet — one that was just deminiaturized or raised — and + /// the router hands the request over the moment that anchor appears. + /// Re-running selection on the spot would activate the app and raise a + /// window again unasked, and because `makeKeyAndOrderFront` is asynchronous + /// the second pass can still read a pre-focus key window and land the Inbox + /// somewhere else entirely. So the request stays where it is and the host + /// is simply given time. + /// + /// Only the created-Welcome path may wait without end, because it is the + /// one path that *knows* its anchor is still on its way. An existing host + /// does not. The anchor lives in a `ToolbarItem`, registration is keyed by + /// `anchor.window`, and AppKit takes that view out of its window whenever + /// the toolbar is collapsed, whenever it overflows on a narrow window, and + /// when full screen moves the toolbar container into + /// `NSToolbarFullScreenWindow` — which carries no `CloseDelegate` and can + /// never become a candidate at all. In each of those the anchor mounts + /// *never*, and an unbounded wait costs twice over: ⇧⌘I, View > Agent + /// Inbox and the Dock do nothing whatsoever, and the request stays armed on + /// the shared router to be delivered whenever that window next mounts an + /// anchor — the Inbox opening by itself minutes later, which is the exact + /// hazard ``AgentInboxPopoverRouter/cancelQueuedRequest()`` documents. + /// + /// When the budget is spent the request is retired and Welcome is created + /// instead: the same answer the workflow already gives when no window can + /// host the Inbox (#1486), and a visible Inbox rather than a dead + /// keystroke. + private func awaitQueuedAnchor(on host: any AgentInboxHosting) { + anchorGeneration &+= 1 + let generation = anchorGeneration + pendingAnchorTask = Task { @MainActor [weak self, weak host] in + defer { self?.finishAnchorTask(generation: generation) } + for _ in 0.. Outcome { + environment.createAgentInboxWelcomeHost() + welcomeHostGeneration &+= 1 + let generation = welcomeHostGeneration + pendingWelcomeHostTask = Task { @MainActor [weak self] in + defer { self?.finishWelcomeHostTask(generation: generation) } + guard let self, let environment = self.environment else { return } + let host = await environment.awaitAgentInboxWelcomeHost() + guard !Task.isCancelled, let host else { return } + self.prepare(host, in: environment) + // One more turn so the freshly raised Welcome window's SwiftUI + // anchor can mount before the request reaches the router. + await Task.yield() + guard !Task.isCancelled else { return } + self.router.requestPresentation(in: host) + } + return .awaitingCreatedWelcomeHost + } + + /// Clears the anchor wait's marker only for the wait that still owns it. + private func finishAnchorTask(generation: Int) { + guard anchorGeneration == generation else { return } + pendingAnchorTask = nil + } + + /// Clears the in-flight marker only for the task that still owns it, so a + /// superseded task cannot erase its successor's pending request. + private func finishWelcomeHostTask(generation: Int) { + guard welcomeHostGeneration == generation else { return } + pendingWelcomeHostTask = nil + } + + /// Restores a miniaturized host before raising it: an `NSPopover` shown + /// relative to an anchor inside a miniaturized window has nowhere to draw. + /// + /// The restore branch is a protocol guarantee, not a reachable production + /// path today. `AppDelegate` projects project-window eligibility through + /// `NSWindow.isVisible`, which reads `false` while a window is in the + /// Dock, so a miniaturized *project* window is never selected: the request + /// silently falls through to Welcome instead of returning the user to the + /// project they minimized. A minimized *Welcome* window is restored today, + /// but by `ensureWelcomeVisible()` on the create path rather than here. + /// #1491's "minimized hosts are restored before presentation" is therefore + /// unmet for project windows, and no test here claims otherwise; widening + /// eligibility changes where ⇧⌘I lands and belongs in its own change (#1507). + private func prepare( + _ host: any AgentInboxHosting, + in environment: any AgentInboxHostEnvironment + ) { + if host.isHostMiniaturized { + host.restoreHostFromMiniaturized() + } + host.focusHost() + environment.activateApplicationForAgentInbox() + } +} diff --git a/Pine/PineApp.swift b/Pine/PineApp.swift index b3d30051..562f02d2 100644 --- a/Pine/PineApp.swift +++ b/Pine/PineApp.swift @@ -901,19 +901,9 @@ struct AgentInboxWindowSources { var welcomeWindow: () -> NSWindow? } -/// One Agent Inbox host candidate paired with the window a winning decision -/// routes to. -/// -/// The pair is produced in a single pass so the pure rule in -/// ``AgentInboxHostRouting`` and the window it selects can never disagree -/// about the window list they were derived from. -struct AgentInboxHostOption { - let candidate: AgentInboxHostCandidate - let host: NSWindow -} - class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, - GlobalTabSwitcherKeyControllerDelegate { + GlobalTabSwitcherKeyControllerDelegate, + AgentInboxHostEnvironment { typealias TerminationAlertPresenter = @MainActor ( AlertTemplate, DialogPresentationContext, @@ -1053,7 +1043,16 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, private var welcomeVisibilityGeneration = 0 private var pendingWelcomeEnsureTask: Task? - private var pendingAgentInboxPresentationTask: Task? + + /// Owns the application-level Agent Inbox presentation workflow: host + /// selection, restore-and-focus, and the single-request rule (#1491). + /// `AppDelegate` supplies only the AppKit facts, through + /// `AgentInboxHostEnvironment`. + private(set) lazy var agentInboxPresentation = + AgentInboxPresentationCoordinator( + router: .shared, + environment: self + ) /// Closure to open a named SwiftUI window, set by PineApp on launch. var openNamedWindow: ((String) -> Void)? @@ -1061,53 +1060,10 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, var openProjectWindow: ((URL) -> Void)? func showAgentInbox() { - NSApp.activate() - pendingAgentInboxPresentationTask?.cancel() - - if let hostWindow = agentInboxHostWindow() { - prepareAgentInboxHostWindow(hostWindow) - NativeCommandDelivery.deferToNextMainRunLoop { - AgentInboxPopoverRouter.shared.requestPresentation( - in: hostWindow - ) - } - return - } - - // With no eligible project or Welcome owner, create Welcome first so - // the Inbox still has a stable, discoverable anchor (#1486). - showWelcome() - pendingAgentInboxPresentationTask = Task { @MainActor [weak self] in - guard let self, - let window = await awaitVisibleWelcomeWindow(), - !Task.isCancelled else { return } - prepareAgentInboxHostWindow(window) - await Task.yield() - guard !Task.isCancelled else { return } - AgentInboxPopoverRouter.shared.requestPresentation( - in: window - ) - pendingAgentInboxPresentationTask = nil - } + agentInboxPresentation.present() } - /// Chooses an existing owner for the application-level Inbox: project - /// every window that could host the popover, then apply the single - /// ordering rule in ``AgentInboxHostRouting``. `nil` means no existing - /// window may host it, and the caller creates Welcome instead. - /// - /// The rule reads candidates produced by one projection pass, and the - /// winning index is resolved against that same pass, so the decision and - /// the window it names can never disagree about the list they came from. - private func agentInboxHostWindow() -> NSWindow? { - let options = agentInboxHostOptions() - guard case .existingHost(let index) = AgentInboxHostRouting.decision( - among: options.map(\.candidate) - ), options.indices.contains(index) else { - return nil - } - return options[index].host - } + // MARK: - AgentInboxHostEnvironment /// Every window that could own the Inbox popover, read through /// ``agentInboxWindowSources`` so which fact reaches which parameter is @@ -1168,6 +1124,30 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, return options } + func activateApplicationForAgentInbox() { + NSApp.activate() + } + + func createAgentInboxWelcomeHost() { + showWelcome() + } + + func awaitAgentInboxWelcomeHost() async -> (any AgentInboxHosting)? { + await awaitVisibleWelcomeWindow() + } + + func deliverAgentInboxRequest( + _ operation: @escaping @MainActor () -> Void + ) { + NativeCommandDelivery.deferToNextMainRunLoop(operation) + } + + /// The same 25 ms interval `awaitVisibleWelcomeWindow()` polls on, so the + /// two waits this workflow can perform are bounded on the same scale. + func waitForAgentInboxAnchor() async { + try? await Task.sleep(for: .milliseconds(25)) + } + /// The project shown by the window that most recently became key. It is /// the Inbox destination while an auxiliary window — Settings, About — /// holds key and therefore cannot host the popover itself. @@ -1200,14 +1180,6 @@ class AppDelegate: NSObject, NSApplicationDelegate, SPUUpdaterDelegate, registry.openProjects.values.contains { $0 === project } } - private func prepareAgentInboxHostWindow(_ window: NSWindow) { - if window.isMiniaturized { - window.deminiaturize(nil) - } - window.makeKeyAndOrderFront(nil) - NSApp.activate() - } - /// Focuses one exact agent route on behalf of an explicit user action — /// a notification response, or a Dock live-task entry (#1492). This is the /// single navigation authority for both: every failure mode degrades to diff --git a/PineTests/AgentInboxHostOptionsTests.swift b/PineTests/AgentInboxHostOptionsTests.swift index 3c713abd..083d4b0a 100644 --- a/PineTests/AgentInboxHostOptionsTests.swift +++ b/PineTests/AgentInboxHostOptionsTests.swift @@ -451,7 +451,14 @@ struct AgentInboxHostOptionsTests { .project, .project, .welcome, ]) #expect(options.map(\.candidate.isKeyWindow) == [false, true, false]) - #expect(options.map(\.host) == [alpha, beta, welcome]) + // By identity: `host` is `any AgentInboxHosting` so the workflow can + // substitute a double for the restore-then-focus ordering, and an + // existential is not `Equatable`. `ObjectIdentifier` still pins order, + // count and the exact object, which is the whole claim here. + #expect( + options.map { ObjectIdentifier($0.host) } + == [alpha, beta, welcome].map(ObjectIdentifier.init) + ) } @Test("a wrapper with no Welcome source builds no Welcome candidate") diff --git a/PineTests/AgentInboxPopoverCoordinatorTests.swift b/PineTests/AgentInboxPopoverCoordinatorTests.swift new file mode 100644 index 00000000..ee1e078b --- /dev/null +++ b/PineTests/AgentInboxPopoverCoordinatorTests.swift @@ -0,0 +1,551 @@ +// +// AgentInboxPopoverCoordinatorTests.swift +// PineTests +// +// The anchor-side glue: which AppKit notification belongs to which popover, +// when a resolution may touch SwiftUI, and what happens across the runloop +// turn a close takes (#1491). +// +// `AgentInboxPopoverPresentationState` is verified as a value and +// `AgentInboxPopoverRouter` in isolation. Neither can see the failures that +// live only in the seam between them — an anchor that ends up holding a +// close nothing can clear, or a second popover built on top of one that is +// still leaving. +// + +import AppKit +import SwiftUI +import Testing + +@testable import Pine + +@Suite("Agent Inbox popover coordinator", .serialized) +@MainActor +struct AgentInboxPopoverCoordinatorTests { + // MARK: - A close that never reports back + + /// The regression this suite exists for. + /// + /// `NSPopover.performClose` animates, and AppKit answers `isShown == false` + /// well before it posts the close notification. SwiftUI re-renders + /// constantly, and `viewDidUpdate` returns `.close` for every pass with the + /// binding down — so a second `closePopover()` inside that animation is not + /// a race, it is the normal case. If it drops the popover reference, the + /// close notification has no sender to match, the in-flight close is never + /// retired, and every entry point resolves to `.inert`: this window can + /// never open the Inbox again — not the toolbar button, not ⇧⌘I, not the + /// View menu, not the Dock — until it is closed. + @Test( + "a second close during the animation cannot lock the window out", + arguments: [false, true] + ) + func repeatedCloseDuringTheAnimationKeepsTheWindowUsable( + appKitReportsVisibleWhileClosing: Bool + ) throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.reportsVisibleWhileClosing = appKitReportsVisibleWhileClosing + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + #expect(fixture.popovers.count == 1) + #expect(fixture.isPresented) + + // The user closes the Inbox from the toolbar button. + fixture.isPresented = false + fixture.update() + #expect(first.closeCount == 1) + + // AppKit is still animating; SwiftUI updates again in the meantime. + fixture.update() + fixture.update() + + // The close finally reports back, addressed to the popover that + // started it. + fixture.coordinator.popoverDidClose(sender: first) + + // ⇧⌘I must open the Inbox in this window again. + fixture.coordinator.presentAgentInbox() + + #expect(fixture.popovers.count == 2) + #expect(fixture.popovers.last?.showCount == 1) + #expect(fixture.isPresented) + } + + /// The other edge of the same rule. + /// + /// `isClosing` is retired by exactly one thing — the close notification — + /// and while it stands every entry point is inert. Not dropping the + /// reference mid-animation is right, but unbounded it leaves a single point + /// of failure with no exit: a notification AppKit never posts costs this + /// window its Inbox for as long as it stays open. + /// + /// No production path was found where AppKit skips that notification, so + /// this covers a guard against a latent failure rather than a reproduced + /// one. The tolerance is deliberately far above what a real animated close + /// produces, which is what + /// `repeatedCloseDuringTheAnimationKeepsTheWindowUsable` above pins from + /// the other side. + @Test("a close that never reports back cannot park the window forever") + func anUnreportedCloseIsEventuallyRetired() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + + // The user closes it and AppKit starts the animation — but the close + // notification never arrives. + fixture.isPresented = false + fixture.update() + #expect(first.closeCount == 1) + + // Every later SwiftUI pass resolves to `.close` on an invisible + // popover, which is the only signal the anchor is given. + for _ in 0..<64 { fixture.update() } + + fixture.coordinator.presentAgentInbox() + + #expect( + fixture.popovers.count == 2, + "A notification AppKit never sent must not cost the Inbox" + ) + #expect(fixture.popovers.last?.showCount == 1) + #expect(fixture.isPresented) + } + + // MARK: - Closes AppKit starts on its own + + /// The popover is `.transient`: Escape and a click outside are handled by + /// AppKit without asking. Without `popoverWillClose`, the entire close + /// animation runs with the state machine believing nothing is closing, and + /// a request that lands inside it builds a second popover on the same + /// anchor — overwriting the only strong reference to the one still + /// leaving. + @Test("a request during an AppKit-started close never builds a second one") + func requestDuringAnAppKitCloseIsHeld() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + + // Escape: AppKit announces the close and already answers `isShown` + // with `false` while the animation runs. + fixture.coordinator.popoverWillClose(sender: first) + first.isPopoverVisible = false + + fixture.coordinator.presentAgentInbox() + #expect( + fixture.popovers.count == 1, + "A second popover would orphan the one still animating out" + ) + + // Held, not swallowed: the close serves it — one turn later, because + // the request comes back through SwiftUI rather than out of AppKit's + // own notification. + fixture.coordinator.popoverDidClose(sender: first) + await fixture.nextRunLoopTurn() + #expect(fixture.popovers.count == 2) + #expect(fixture.popovers.last?.showCount == 1) + #expect(fixture.isPresented) + } + + @Test("a close announced for another popover is ignored") + func foreignWillCloseIsIgnored() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + // This popover is gone without ever reporting it. + first.isPopoverVisible = false + + fixture.coordinator.popoverWillClose(sender: FakePopover()) + + // A stranger's close must not park this anchor: the request opens. + fixture.coordinator.presentAgentInbox() + #expect(fixture.popovers.count == 2) + } + + @Test("a replaced popover's close never drops the live one") + func replacedPopoverCloseIsIgnored() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let live = try #require(fixture.popovers.last) + + fixture.coordinator.popoverDidClose(sender: FakePopover()) + + // Acting on it would drop the reference to the live popover, leaving + // an Inbox that nothing can close. + fixture.isPresented = false + fixture.update() + #expect(live.closeCount == 1) + } + + // MARK: - The runloop turn a close takes + + /// The SwiftUI half of a close lands a turn after AppKit's notification — + /// it cannot be written inside it. That turn is long enough for a newer + /// request to open a new popover, and replaying the verdict computed for + /// the old one lowers the binding under a visible Inbox: it blinks open + /// and shuts, and the keystroke that opened it reads as swallowed. + @Test("a settled close never closes the popover that replaced it") + func settledCloseLeavesANewerPopoverAlone() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + + // The user clicks outside; AppKit closes the popover. + fixture.coordinator.popoverWillClose(sender: first) + first.isPopoverVisible = false + fixture.coordinator.popoverDidClose(sender: first) + + // ⇧⌘I in the same turn opens a new one. + fixture.coordinator.presentAgentInbox() + let second = try #require(fixture.popovers.last) + #expect(fixture.popovers.count == 2) + + await fixture.nextRunLoopTurn() + + #expect(fixture.isPresented) + #expect(second.closeCount == 0) + #expect(second.isPopoverVisible) + } + + @Test("a settled close with nothing behind it lowers the binding") + func settledCloseLowersTheBinding() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + #expect(fixture.isPresented) + + fixture.coordinator.popoverWillClose(sender: first) + first.isPopoverVisible = false + fixture.coordinator.popoverDidClose(sender: first) + + // Until SwiftUI is told, the toolbar button still reads as active and + // the anchor would refuse the next request as a duplicate. + await fixture.nextRunLoopTurn() + #expect(!fixture.isPresented) + } + + // MARK: - Moving between windows + + /// `viewDidMoveToWindow` fires before layout, so the anchor's `bounds` is + /// still zero and a popover shown from it hangs off the window's origin + /// rather than the toolbar button. The router defers its own hand-off for + /// precisely this reason; a show the anchor starts for itself has to wait + /// the same turn. + @Test("a window change never shows the popover before layout") + func anchorWindowChangeDefersItsPresentation() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + // The binding is up while the anchor has no window: the state a + // toolbar item is in while AppKit is re-parenting its view. + fixture.isPresented = true + fixture.update() + #expect(fixture.popovers.isEmpty) + + fixture.attachToWindow() + fixture.coordinator.anchorWindowDidChange(fixture.anchor) + #expect( + fixture.popovers.isEmpty, + "Shown inside viewDidMoveToWindow, before the anchor has bounds" + ) + + await fixture.nextRunLoopTurn() + #expect(fixture.popovers.count == 1) + #expect(fixture.popovers.last?.showCount == 1) + } + + /// The other half of that turn: it is long enough for the reason to + /// disappear, so the verdict is re-derived on arrival rather than replayed. + @Test("a window change that stops being wanted presents nothing") + func staleAnchorWindowChangeIsAbandoned() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.isPresented = true + fixture.update() + + fixture.attachToWindow() + fixture.coordinator.anchorWindowDidChange(fixture.anchor) + // The user closes the Inbox inside that same turn, and SwiftUI's pass + // for it lands before the deferred show does. + fixture.isPresented = false + fixture.update() + + await fixture.nextRunLoopTurn() + #expect(fixture.popovers.isEmpty) + } + + // MARK: - Writing SwiftUI state from an update pass + + /// `update` runs inside SwiftUI's live view update, so the `@State` write + /// this resolution carries has to be deferred — the exact mutation + /// AGENTS.md requires observers to move off the pass. + @Test("an update pass never writes the binding inside itself") + func updateDefersItsBindingWrite() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + // No window: the request is accepted but cannot be shown, so it stays + // outstanding and outranks the binding on the next pass. + fixture.update() + fixture.coordinator.presentAgentInbox() + #expect(fixture.popovers.isEmpty) + + fixture.isPresented = false + fixture.update() + #expect( + !fixture.isPresented, + "The binding was written inside the update pass" + ) + + await fixture.nextRunLoopTurn() + #expect(fixture.isPresented) + } + + /// The write that closes the Inbox the instant it opens. + /// + /// `viewDidUpdate` defers exactly one binding write, and it defers it in + /// exactly one situation: an outstanding router request outranking a + /// lowered binding. The `.present` that comes with it runs *synchronously*, + /// and a show that succeeds calls `popoverWillShow()`, which clears the + /// very request the write was derived from. A deferred block that asks the + /// rule again therefore reads "binding down, nothing pending", answers + /// `.close`, and writes nothing — so the lift is lost precisely when the + /// show *worked*, and survives only when it failed. + /// + /// The window is left showing an Inbox with `isPresented == false`, and the + /// next SwiftUI pass closes it: the Inbox blinks and vanishes, and the + /// keystroke reads as swallowed. + /// + /// The anchor is mounted here on purpose. `updateDefersItsBindingWrite` + /// below uses an unmounted one, which is the branch where the show fails — + /// the only branch in which the naive re-derivation is correct. + @Test("a request served by an update pass keeps the binding it raised") + func servedUpdateRequestKeepsItsRaisedBinding() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let first = try #require(fixture.popovers.last) + + // Escape, or a click outside: AppKit announces the close and already + // answers `isShown` with `false` while the animation runs. + fixture.coordinator.popoverWillClose(sender: first) + first.isPopoverVisible = false + + // ⇧⌘I inside the animation. It is held, not served — so it is still + // outstanding when the close finishes. + fixture.coordinator.presentAgentInbox() + #expect(fixture.popovers.count == 1) + + // The user lowers the binding from the toolbar button, and the close + // reports back in the same turn. + fixture.isPresented = false + fixture.coordinator.popoverDidClose(sender: first) + + // SwiftUI's pass arrives in that same turn and serves the held request. + fixture.update() + let second = try #require(fixture.popovers.last) + #expect(fixture.popovers.count == 2) + #expect(second.isPopoverVisible) + + await fixture.nextRunLoopTurn() + + #expect( + fixture.isPresented, + "The Inbox is on screen; SwiftUI must not be told it is closed" + ) + + // What the lost write costs: the very next pass closes the popover the + // user just asked for. + fixture.update() + #expect(second.closeCount == 0) + #expect(fixture.popovers.count == 2) + } + + // MARK: - The content's own dismissal + + /// #1491's "successful task navigation or recovery dismisses the popover". + /// + /// `AgentInboxView` is handed a closure and calls it; that the view calls + /// what it was given is covered where the view is. What is only observable + /// here is what that closure was *bound to* — and nothing else in this + /// suite looks at it, so `onDismiss: {}` would leave the Inbox standing + /// over the window the user was just navigated to with every test green. + @Test("the content's own dismissal closes the popover it was built for") + func contentDismissClosesThePopoverItWasBuiltFor() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.coordinator.presentAgentInbox() + let popover = try #require(fixture.popovers.last) + let context = try #require(fixture.contexts.last) + #expect(fixture.isPresented) + + context.onDismiss() + + #expect( + popover.closeCount == 1, + "A successful route must take the Inbox down with it" + ) + #expect(!fixture.isPresented) + } + + // MARK: - Fixture + + @MainActor + private final class Fixture { + let router = AgentInboxPopoverRouter { operation in operation() } + let registry: ProjectRegistry + private(set) var popovers: [FakePopover] = [] + /// The `Context` each popover was built from. + /// + /// Kept because it is the only place the anchor's own callbacks are + /// observable. A factory that drops the argument makes every wiring + /// mistake inside it invisible: `onDismiss: {}` would leave the popover + /// open over the window the user was just moved to, and every test + /// here would still be green. + private(set) var contexts: [AgentInboxPopoverCoordinator.Context] = [] + var reportsVisibleWhileClosing = false + /// The value SwiftUI's `@State` itself holds. + var isPresented = false + /// What the binding captured on the last `update()` reads back. + /// + /// SwiftUI hands `updateNSView` a `Binding` over the snapshot taken for + /// that pass, so a read between passes answers from the snapshot rather + /// than from a write made since. A plain `Binding` over a stored `Bool` + /// reads back whatever was last written and hides every decision that + /// turns on the difference — and every deferred hop in the coordinator + /// is exactly such a decision. + private var snapshot = false + let anchor = AgentInboxPopoverAnchorView(frame: .zero) + private let window: NSWindow + private let suiteName: String + private let defaults: UserDefaults + private var openedProjectWindows: [URL] = [] + lazy var coordinator = AgentInboxPopoverCoordinator( + router: router, + makePopover: { [weak self] _, context in + guard let self else { return nil } + let popover = FakePopover() + popover.reportsVisibleWhileClosing = + self.reportsVisibleWhileClosing + self.popovers.append(popover) + self.contexts.append(context) + return popover + } + ) + + init() throws { + suiteName = "AgentInboxPopoverCoordinatorTests.\(UUID())" + defaults = try #require(UserDefaults(suiteName: suiteName)) + defaults.removePersistentDomain(forName: suiteName) + registry = ProjectRegistry( + defaults: defaults, + agentTasks: AgentTaskRegistry(), + // No `ps` polling: this suite is about popover lifecycle. + agentDetectionProcessRunner: { _, _, _, _ in + ProcessRunResult( + stdout: "", + stderr: "", + exitCode: 0, + timedOut: false + ) + }, + agentDetectionPollInterval: 3_600, + agentDetectionInitialPollDelay: 3_600 + ) + registry.recentProjects = [] + window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 320, height: 240), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.contentView = NSView( + frame: window.contentRect(forFrameRect: window.frame) + ) + } + + /// Puts the anchor in a window, which is what lets it present. + func mount() { + attachToWindow() + update() + } + + /// The AppKit half of mounting on its own, without the SwiftUI pass — + /// the order `viewDidMoveToWindow` actually fires in. + func attachToWindow() { + window.contentView?.addSubview(anchor) + coordinator.attach(to: anchor) + } + + func update() { + snapshot = isPresented + coordinator.update( + anchor: anchor, + isPresented: Binding( + get: { [weak self] in self?.snapshot ?? false }, + set: { [weak self] value in self?.isPresented = value } + ), + registry: registry, + openProjectWindow: { [weak self] url in + self?.openedProjectWindows.append(url) + }, + reduceMotion: true + ) + } + + func nextRunLoopTurn() async { + await withCheckedContinuation { continuation in + DispatchQueue.main.async { continuation.resume() } + } + } + + func cleanup() { + coordinator.detach() + anchor.removeFromSuperview() + window.orderOut(nil) + defaults.removePersistentDomain(forName: suiteName) + } + } + + /// Stands in for `NSPopover`, including its most dangerous habit: + /// answering `isShown` with `false` long before the close notification + /// that retires it. + @MainActor + private final class FakePopover: AgentInboxPopoverHandle { + var isPopoverVisible = false + var reportsVisibleWhileClosing = false + private(set) var showCount = 0 + private(set) var closeCount = 0 + + func showPopover(from anchor: NSView) { + showCount += 1 + isPopoverVisible = true + } + + func closePopover() { + closeCount += 1 + isPopoverVisible = reportsVisibleWhileClosing + } + } +} diff --git a/PineTests/AgentInboxPopoverPresentationStateTests.swift b/PineTests/AgentInboxPopoverPresentationStateTests.swift new file mode 100644 index 00000000..3d86a1b4 --- /dev/null +++ b/PineTests/AgentInboxPopoverPresentationStateTests.swift @@ -0,0 +1,570 @@ +// +// AgentInboxPopoverPresentationStateTests.swift +// PineTests +// +// Reconciliation of the Agent Inbox popover's binding, router request, and +// AppKit visibility (#1491). +// + +import Testing + +@testable import Pine + +@Suite("Agent Inbox popover presentation state") +struct AgentInboxPopoverPresentationStateTests { + private typealias State = AgentInboxPopoverPresentationState + + // MARK: - Router requests + + @Test("a request on a hidden popover presents and raises the binding") + func requestPresentsAndRaisesBinding() { + var state = State() + + let resolution = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + + #expect(resolution.effect == .present) + #expect(resolution.bindingIsPresented == true) + #expect(state.hasUnservedRouterRequest) + } + + @Test("a request does not rewrite a binding that is already true") + func requestLeavesRaisedBindingAlone() { + var state = State() + + let resolution = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + + #expect(resolution.effect == .present) + #expect(resolution.bindingIsPresented == nil) + } + + @Test("a request while the popover is shown never presents again") + func requestWhileShownIsAbsorbed() { + var state = State() + state.popoverWillShow() + + let resolution = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: true + ) + + // A second `show` would orphan the first popover's hosting + // controller and leave an Inbox nobody can dismiss. + #expect(resolution.effect == .inert) + #expect(resolution.bindingIsPresented == nil) + // Absorbed means served. An unserved marker left here outranks the + // binding on the next update pass, so the toolbar button that opened + // the Inbox could no longer close it. + #expect(!state.hasUnservedRouterRequest) + #expect( + state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: true + ).effect == .close + ) + } + + @Test("the toolbar button still closes an Inbox that absorbed a request") + func absorbedRequestDoesNotBlockTheToolbarToggle() { + var state = State() + var binding = true + state.popoverWillShow() + + // The Inbox is open and the user presses the shortcut again; the + // request is absorbed. Then they click the toolbar button to close. + _ = state.routerRequestedPresentation( + bindingIsPresented: binding, + isPopoverShown: true + ) + binding.toggle() + let update = state.viewDidUpdate( + bindingIsPresented: binding, + isPopoverShown: true + ) + + #expect(update.effect == .close) + #expect(update.bindingIsPresented == nil) + } + + @Test("a burst of requests while shown produces no extra popover") + func repeatedRequestsWhileShownProduceNoPresent() { + var state = State() + state.popoverWillShow() + + let effects = (0..<25).map { index in + state.routerRequestedPresentation( + bindingIsPresented: index.isMultiple(of: 2), + isPopoverShown: true + ).effect + } + + #expect(effects.allSatisfy { $0 == .inert }) + } + + @Test("a request while shown still converges a lagging binding") + func requestWhileShownConvergesBinding() { + var state = State() + state.popoverWillShow() + + let resolution = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: true + ) + + #expect(resolution.effect == .inert) + #expect(resolution.bindingIsPresented == true) + } + + // MARK: - SwiftUI update passes + + @Test("a lowered binding with nothing pending closes the popover") + func loweredBindingCloses() { + let state = State() + + let resolution = state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: true + ) + + #expect(resolution.effect == .close) + #expect(resolution.bindingIsPresented == nil) + } + + @Test("an update serves a request that arrived before the anchor") + func updateServesQueuedRequest() { + var state = State() + // The anchor had no window yet, so the request could not be shown. + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + + let resolution = state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: false + ) + + #expect(resolution.effect == .present) + #expect(resolution.bindingIsPresented == true) + } + + @Test("an update on an already shown popover changes nothing") + func updateOnShownPopoverIsInert() { + let state = State() + + let resolution = state.viewDidUpdate( + bindingIsPresented: true, + isPopoverShown: true + ) + + #expect(resolution == State.Resolution( + effect: .inert, + bindingIsPresented: nil + )) + } + + // MARK: - Anchor window changes + + @Test("an anchor window change never writes SwiftUI state") + func anchorWindowChangeNeverWritesBinding() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + + let resolution = state.anchorWindowDidChange( + bindingIsPresented: false, + isPopoverShown: false + ) + + // This runs inside AppKit's viewDidMoveToWindow; writing @State there + // re-enters the live SwiftUI update. + #expect(resolution.effect == .present) + #expect(resolution.bindingIsPresented == nil) + } + + @Test("an idle anchor window change never closes a popover") + func idleAnchorWindowChangeIsUnchanged() { + let state = State() + + let resolution = state.anchorWindowDidChange( + bindingIsPresented: false, + isPopoverShown: true + ) + + #expect(resolution == .unchanged) + } + + // MARK: - Serving and closing + + @Test("showing the popover consumes the outstanding request") + func showingConsumesTheRequest() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + + state.popoverWillShow() + + #expect(!state.hasUnservedRouterRequest) + // With the request consumed, a lowered binding is authoritative again. + #expect( + state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: true + ).effect == .close + ) + } + + @Test("a request during a close is held, then served by the close") + func requestDuringCloseIsHeldAndThenServed() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + + // `performClose` animates. AppKit may still answer `isShown` either + // way during it, so both readings must behave identically. + for isPopoverShown in [true, false] { + let request = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: isPopoverShown + ) + #expect(request.effect == .inert) + #expect(state.hasUnservedRouterRequest) + } + + state.popoverDidClose() + // The close is retired the instant AppKit reports it, not with the + // deferred SwiftUI write below. + #expect(!state.isClosing) + #expect(state.hasUnservedRouterRequest) + + let closed = state.settledClose( + bindingIsPresented: true, + isPopoverShown: false + ) + + // Held, not swallowed: ⇧⌘I during the close animation still opens. + #expect(closed.effect == .present) + #expect(closed.bindingIsPresented == nil) + } + + @Test("a close with no request behind it never reopens the Inbox") + func closeWithoutRequestStaysClosed() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + + state.popoverDidClose() + let closed = state.settledClose( + bindingIsPresented: true, + isPopoverShown: false + ) + + #expect(closed.effect == .inert) + #expect(closed.bindingIsPresented == false) + #expect(!state.isClosing) + } + + @Test("a request during a close raises a binding that was left down") + func requestDuringCloseConvergesLoweredBinding() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: true + ) + + state.popoverDidClose() + let closed = state.settledClose( + bindingIsPresented: false, + isPopoverShown: false + ) + + #expect(closed.effect == .present) + #expect(closed.bindingIsPresented == true) + } + + @Test("no SwiftUI pass may open a popover that is still closing") + func updatesDoNotRaceAClose() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + _ = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + + // Both observers run freely while AppKit animates the close; neither + // may create a second popover against the same anchor. + #expect( + state.viewDidUpdate( + bindingIsPresented: true, + isPopoverShown: false + ).effect == .inert + ) + #expect( + state.anchorWindowDidChange( + bindingIsPresented: true, + isPopoverShown: false + ).effect == .inert + ) + } + + /// A defensive invariant of the value type only. The anchor cannot + /// produce this order — it never shows a popover while one is closing — + /// so this is not coverage of a stuck close. That failure is an anchor + /// bug, not a rule bug, and lives in + /// `AgentInboxPopoverCoordinatorTests`. + @Test("showing a popover clears a close that never reported back") + func showingClearsAStuckClose() { + var state = State() + state.popoverWillClose() + state.popoverWillShow() + + #expect(!state.isClosing) + #expect( + state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ).effect == .present + ) + } + + @Test("a close that settles under a newer popover leaves it alone") + func settledCloseUnderANewerPopoverIsUnchanged() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + state.popoverDidClose() + + // The SwiftUI half of that close is still a runloop turn away when a + // new request opens a new popover. + _ = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + state.popoverWillShow() + + // Replaying the old verdict here would lower the binding under a + // visible Inbox, and the next update pass would close it: the popover + // blinks open and shuts, and the keystroke reads as swallowed. + #expect( + state.settledClose( + bindingIsPresented: true, + isPopoverShown: true + ) == .unchanged + ) + } + + @Test("a close that settles during a newer close leaves it alone") + func settledCloseDuringANewerCloseIsUnchanged() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + state.popoverDidClose() + + state.popoverWillShow() + state.popoverWillClose() + + // The newer close owns the binding now; it will settle on its own. + #expect( + state.settledClose( + bindingIsPresented: true, + isPopoverShown: false + ) == .unchanged + ) + } + + @Test("a detached anchor is never left mid-close") + func detachClearsTheClosingState() { + var state = State() + state.popoverWillShow() + state.popoverWillClose() + + _ = state.anchorDidDetach() + + #expect(!state.isClosing) + #expect(!state.hasUnservedRouterRequest) + } + + @Test("an outside click or Escape lowers the SwiftUI binding") + func appKitCloseLowersBinding() { + var state = State() + state.popoverWillShow() + + state.popoverDidClose() + let resolution = state.settledClose( + bindingIsPresented: true, + isPopoverShown: false + ) + + #expect(resolution.effect == .inert) + #expect(resolution.bindingIsPresented == false) + } + + @Test("an AppKit close does not rewrite an already lowered binding") + func appKitCloseLeavesLoweredBindingAlone() { + var state = State() + + state.popoverDidClose() + let resolution = state.settledClose( + bindingIsPresented: false, + isPopoverShown: false + ) + + #expect(resolution == State.Resolution( + effect: .inert, + bindingIsPresented: nil + )) + } + + @Test("an AppKit close discards a request that never reached a popover") + func appKitCloseDiscardsUnservedRequest() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + + state.popoverDidClose() + + #expect(!state.hasUnservedRouterRequest) + #expect( + state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: false + ).effect == .close + ) + } + + @Test("successful navigation or recovery closes and lowers the binding") + func contentDismissClosesAndLowersBinding() { + var state = State() + state.popoverWillShow() + + let resolution = state.contentRequestedDismiss( + bindingIsPresented: true + ) + + #expect(resolution.effect == .close) + #expect(resolution.bindingIsPresented == false) + #expect(!state.hasUnservedRouterRequest) + } + + @Test("a content dismiss also drops a request queued behind it") + func contentDismissDropsQueuedRequest() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + + _ = state.contentRequestedDismiss(bindingIsPresented: true) + + #expect(!state.hasUnservedRouterRequest) + } + + // MARK: - Detaching + + @Test("a detached anchor closes and abandons its request") + func detachClosesAndAbandonsRequest() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + + let resolution = state.anchorDidDetach() + + #expect(resolution.effect == .close) + #expect(resolution.bindingIsPresented == nil) + #expect(!state.hasUnservedRouterRequest) + } + + @Test("a detached anchor cannot resurrect a stale request") + func detachedAnchorDoesNotResurrectRequest() { + var state = State() + _ = state.routerRequestedPresentation( + bindingIsPresented: false, + isPopoverShown: false + ) + _ = state.anchorDidDetach() + + // The window came back — with the binding still false, nothing may + // reopen the Inbox on its own. + let reattached = state.anchorWindowDidChange( + bindingIsPresented: false, + isPopoverShown: false + ) + let update = state.viewDidUpdate( + bindingIsPresented: false, + isPopoverShown: false + ) + + #expect(reattached == .unchanged) + #expect(update.effect == .close) + } + + // MARK: - Sequences + + @Test("a full open, outside-dismiss, reopen cycle converges each time") + func openDismissReopenCycleConverges() { + var state = State() + var binding = false + var isShown = false + + for _ in 0..<5 { + let request = state.routerRequestedPresentation( + bindingIsPresented: binding, + isPopoverShown: isShown + ) + if let value = request.bindingIsPresented { binding = value } + #expect(request.effect == .present) + state.popoverWillShow() + isShown = true + + // AppKit dismisses the transient popover behind SwiftUI's back. + isShown = false + state.popoverDidClose() + let closed = state.settledClose( + bindingIsPresented: binding, + isPopoverShown: isShown + ) + if let value = closed.bindingIsPresented { binding = value } + + #expect(!binding) + #expect(!state.hasUnservedRouterRequest) + } + } + + @Test("a request that races an AppKit close still ends visible") + func requestRacingCloseEndsVisible() { + var state = State() + state.popoverWillShow() + + // AppKit closes, and the menu command lands in the same runloop turn + // before SwiftUI has processed the deferred binding write. + state.popoverDidClose() + let closed = state.settledClose( + bindingIsPresented: true, + isPopoverShown: false + ) + let request = state.routerRequestedPresentation( + bindingIsPresented: true, + isPopoverShown: false + ) + + #expect(closed.bindingIsPresented == false) + #expect(request.effect == .present) + #expect(state.hasUnservedRouterRequest) + } +} diff --git a/PineTests/AgentInboxPopoverRouterTests.swift b/PineTests/AgentInboxPopoverRouterTests.swift index 370f21ef..753ee52a 100644 --- a/PineTests/AgentInboxPopoverRouterTests.swift +++ b/PineTests/AgentInboxPopoverRouterTests.swift @@ -5,6 +5,7 @@ // Window-identity and late-anchor coverage for Agent Inbox presentation. // +import Foundation import Testing @testable import Pine @@ -26,21 +27,82 @@ struct AgentInboxPopoverRouterTests { @Test("a request waits for a newly mounted window anchor") func requestWaitsForAnchor() { - let router = AgentInboxPopoverRouter() + let (router, deliveries) = makeRouter() + let host = Host() + let presenter = Presenter() + + #expect(router.requestPresentation(in: host) == .queued) + #expect(presenter.presentationCount == 0) + + router.register(presenter, for: host) + deliveries.flush() + + #expect(presenter.presentationCount == 1) + } + + @Test("registering never presents inside the caller's own frame") + func queuedRequestIsHandedOffOutOfBand() { + let (router, deliveries) = makeRouter() let host = Host() let presenter = Presenter() #expect(router.requestPresentation(in: host) == .queued) + router.register(presenter, for: host) + + // Anchors register from `viewDidMoveToWindow` and `updateNSView`. + // Presenting there writes SwiftUI state inside a live update pass and + // shows the popover before layout has sized the anchor. #expect(presenter.presentationCount == 0) + #expect(deliveries.count == 1) + + deliveries.flush() + #expect(presenter.presentationCount == 1) + } + + @Test("the production router defers the hand-off by one runloop turn") + func productionRouterDefersHandOff() async { + // The injected deliverer above is only a lens. This asserts the real + // default, so the deferral cannot be lost while the tests stay green. + let router = AgentInboxPopoverRouter() + let host = Host() + let presenter = Presenter() + #expect(router.requestPresentation(in: host) == .queued) router.register(presenter, for: host) + #expect(presenter.presentationCount == 0) + + await nextMainRunLoopTurn() #expect(presenter.presentationCount == 1) } + @Test("an anchor released during the deferral re-queues, never resurrects") + func anchorReleasedDuringDeferralRequeuesTheRequest() { + let (router, deliveries) = makeRouter() + let host = Host() + + #expect(router.requestPresentation(in: host) == .queued) + do { + let doomed = Presenter() + router.register(doomed, for: host) + } + + // The anchor died in the turn between registering and delivery. The + // router holds it weakly, so nothing is resurrected and nothing traps + // — and because delivery re-resolves the window rather than replaying + // a captured anchor, the user's request goes back on the queue for + // whichever anchor this window mounts next. + deliveries.flush() + + let replacement = Presenter() + router.register(replacement, for: host) + deliveries.flush() + #expect(replacement.presentationCount == 1) + } + @Test("a request never leaks into another window") func requestTargetsExactWindow() { - let router = AgentInboxPopoverRouter() + let (router, deliveries) = makeRouter() let requestedHost = Host() let otherHost = Host() let otherPresenter = Presenter() @@ -51,6 +113,7 @@ struct AgentInboxPopoverRouterTests { #expect(otherPresenter.presentationCount == 0) router.register(requestedPresenter, for: requestedHost) + deliveries.flush() #expect(requestedPresenter.presentationCount == 1) #expect(otherPresenter.presentationCount == 0) } @@ -71,6 +134,331 @@ struct AgentInboxPopoverRouterTests { #expect(replacementPresenter.presentationCount == 1) } + @Test("a stale unregistration from another host changes nothing") + func unregistrationIsScopedToItsHost() { + let router = AgentInboxPopoverRouter() + let host = Host() + let otherHost = Host() + let presenter = Presenter() + + router.register(presenter, for: host) + router.unregister(presenter, from: otherHost) + + #expect(router.requestPresentation(in: host) == .presented) + #expect(presenter.presentationCount == 1) + } + + @Test("an unregistered anchor stops receiving its window's requests") + func unregisteredAnchorStopsReceivingRequests() { + let router = AgentInboxPopoverRouter() + let host = Host() + let presenter = Presenter() + + router.register(presenter, for: host) + router.unregister(presenter, from: host) + + #expect(router.requestPresentation(in: host) == .queued) + #expect(presenter.presentationCount == 0) + } + + @Test("a released anchor cannot serve a queued request") + func releasedAnchorCannotServeQueuedRequest() { + let (router, deliveries) = makeRouter() + let host = Host() + + do { + let doomedPresenter = Presenter() + router.register(doomedPresenter, for: host) + } + + // The window's SwiftUI anchor was torn down without unregistering. + // Its slot must not answer for the window that outlived it. + #expect(router.requestPresentation(in: host) == .queued) + + let replacement = Presenter() + router.register(replacement, for: host) + deliveries.flush() + #expect(replacement.presentationCount == 1) + } + + @Test("a queued request dies with the window it was queued for") + func queuedRequestDiesWithItsHost() { + let (router, deliveries) = makeRouter() + + do { + let doomedHost = Host() + #expect(router.requestPresentation(in: doomedHost) == .queued) + } + + // A different window mounting later must not inherit the request. + let survivingHost = Host() + let presenter = Presenter() + router.register(presenter, for: survivingHost) + deliveries.flush() + + #expect(presenter.presentationCount == 0) + } + + @Test("a queued request can be retired before its window ever mounts") + func queuedRequestCanBeCancelled() { + let (router, deliveries) = makeRouter() + let host = Host() + + #expect(router.requestPresentation(in: host) == .queued) + // Nothing else expires a queued request. Left armed on this shared + // object it waits for the singleton Welcome window's next anchor, + // which can be minutes later and reads as the Inbox opening by itself. + router.cancelQueuedRequest() + + let presenter = Presenter() + router.register(presenter, for: host) + deliveries.flush() + #expect(presenter.presentationCount == 0) + // The window is still perfectly usable for a request made later. + #expect(router.requestPresentation(in: host) == .presented) + #expect(presenter.presentationCount == 1) + } + + @Test("retiring a queued request is safe when nothing is queued") + func cancellingWithNothingQueuedIsHarmless() { + let (router, deliveries) = makeRouter() + let host = Host() + let presenter = Presenter() + + router.register(presenter, for: host) + router.cancelQueuedRequest() + router.cancelQueuedRequest() + + #expect(router.requestPresentation(in: host) == .presented) + deliveries.flush() + #expect(presenter.presentationCount == 1) + } + + @Test("a stray teardown never cancels a window's queued request") + func unregisteringAnUnknownAnchorKeepsTheQueuedRequest() { + let (router, deliveries) = makeRouter() + let host = Host() + let strayPresenter = Presenter() + + #expect(router.requestPresentation(in: host) == .queued) + // Nothing was ever registered for this window: this is exactly the + // created-Welcome case the queue exists for, and a teardown from an + // anchor that never owned it must not cancel the user's request. + router.unregister(strayPresenter, from: host) + + let presenter = Presenter() + router.register(presenter, for: host) + deliveries.flush() + #expect(presenter.presentationCount == 1) + } + + @Test("unregistering one window leaves another window's request armed") + func unregisteringIsScopedToItsOwnWindow() { + let (router, deliveries) = makeRouter() + let host = Host() + let otherHost = Host() + let otherPresenter = Presenter() + + router.register(otherPresenter, for: otherHost) + #expect(router.requestPresentation(in: host) == .queued) + router.unregister(otherPresenter, from: otherHost) + + let presenter = Presenter() + router.register(presenter, for: host) + deliveries.flush() + #expect(presenter.presentationCount == 1) + } + + @Test("a newer queued request supersedes the one before it") + func newerQueuedRequestSupersedesTheOlder() { + let (router, deliveries) = makeRouter() + let firstHost = Host() + let secondHost = Host() + let firstPresenter = Presenter() + let secondPresenter = Presenter() + + #expect(router.requestPresentation(in: firstHost) == .queued) + #expect(router.requestPresentation(in: secondHost) == .queued) + + router.register(firstPresenter, for: firstHost) + router.register(secondPresenter, for: secondHost) + deliveries.flush() + + #expect(firstPresenter.presentationCount == 0) + #expect(secondPresenter.presentationCount == 1) + } + + @Test("a request reaches only its own window when both are registered") + func registeredWindowsDoNotSharePresentations() { + let router = AgentInboxPopoverRouter() + let firstHost = Host() + let secondHost = Host() + let firstPresenter = Presenter() + let secondPresenter = Presenter() + + router.register(firstPresenter, for: firstHost) + router.register(secondPresenter, for: secondHost) + + #expect(router.requestPresentation(in: secondHost) == .presented) + + #expect(firstPresenter.presentationCount == 0) + #expect(secondPresenter.presentationCount == 1) + } + + @Test("a rebuilt anchor inherits the request exactly once, never twice") + func queuedRequestIsDeliveredOnceToTheLiveAnchor() { + let (router, deliveries) = makeRouter() + let host = Host() + let superseded = Presenter() + + #expect(router.requestPresentation(in: host) == .queued) + router.register(superseded, for: host) + + // SwiftUI rebuilt the anchor's coordinator inside the deferral. The + // superseded one is already detached — it owns no popover and would + // drop the request on the floor — so delivery has to resolve the + // window's *current* anchor. Delivering to both would open two + // popovers on one anchor. + let replacement = Presenter() + router.register(replacement, for: host) + deliveries.flush() + + #expect(superseded.presentationCount == 0) + #expect(replacement.presentationCount == 1) + #expect(deliveries.isEmpty) + } + + @Test("retiring a request also retires the hand-off already scheduled") + func cancellingRetiresAnInFlightHandOff() { + let (router, deliveries) = makeRouter() + let welcome = Host() + let welcomeAnchor = Presenter() + + #expect(router.requestPresentation(in: welcome) == .queued) + // The window mounts its anchor, so the request leaves the queue and + // becomes a scheduled closure — unreachable through the queue alone + // for a whole runloop turn. + router.register(welcomeAnchor, for: welcome) + router.cancelQueuedRequest() + + deliveries.flush() + #expect(welcomeAnchor.presentationCount == 0) + } + + @Test("a newer request retires the hand-off already scheduled for the old") + func newerRequestRetiresAnInFlightHandOff() { + let (router, deliveries) = makeRouter() + let welcome = Host() + let project = Host() + let welcomeAnchor = Presenter() + let projectAnchor = Presenter() + router.register(projectAnchor, for: project) + + #expect(router.requestPresentation(in: welcome) == .queued) + router.register(welcomeAnchor, for: welcome) + // One ⇧⌘I, superseded inside the deferral by a second one that finds + // a mounted project window. Without a delivery token the first lands a + // turn later and the single request opens the Inbox in two windows. + #expect(router.requestPresentation(in: project) == .presented) + + deliveries.flush() + #expect(welcomeAnchor.presentationCount == 0) + #expect(projectAnchor.presentationCount == 1) + } + + @Test("every repeated request reaches the anchor exactly once") + func repeatedRequestsAreForwardedOneForOne() { + let router = AgentInboxPopoverRouter() + let host = Host() + let otherHost = Host() + let presenter = Presenter() + let otherPresenter = Presenter() + + router.register(presenter, for: host) + router.register(otherPresenter, for: otherHost) + + for _ in 0..<5 { + #expect(router.requestPresentation(in: host) == .presented) + } + + // Dropping duplicates is the anchor's decision, not the router's; + // leaking them into another window is never allowed. + #expect(presenter.presentationCount == 5) + #expect(otherPresenter.presentationCount == 0) + } + + @Test("re-registering the same anchor keeps exactly one live slot") + func repeatedRegistrationIsIdempotent() { + let router = AgentInboxPopoverRouter() + let host = Host() + let presenter = Presenter() + + for _ in 0..<4 { + router.register(presenter, for: host) + } + #expect(presenter.presentationCount == 0) + + router.unregister(presenter, from: host) + #expect(router.requestPresentation(in: host) == .queued) + } + + @Test("a superseded anchor cannot unregister its replacement's window") + func supersededAnchorCannotUnregisterReplacement() { + let router = AgentInboxPopoverRouter() + let host = Host() + let stalePresenter = Presenter() + let replacementPresenter = Presenter() + + router.register(stalePresenter, for: host) + router.register(replacementPresenter, for: host) + for _ in 0..<3 { + router.unregister(stalePresenter, from: host) + } + + #expect(router.requestPresentation(in: host) == .presented) + #expect(replacementPresenter.presentationCount == 1) + #expect(stalePresenter.presentationCount == 0) + } + + // MARK: - Fixture + + /// Captures the router's deferred hand-off so a test can observe the + /// moment before and after it instead of guessing at runloop timing. + @MainActor + private final class Deliveries { + private var pending: [@MainActor () -> Void] = [] + + var count: Int { pending.count } + + var isEmpty: Bool { pending.isEmpty } + + func enqueue(_ operation: @escaping @MainActor () -> Void) { + pending.append(operation) + } + + func flush() { + let operations = pending + pending.removeAll() + for operation in operations { operation() } + } + } + + private func makeRouter() -> (AgentInboxPopoverRouter, Deliveries) { + let deliveries = Deliveries() + return ( + AgentInboxPopoverRouter { operation in + deliveries.enqueue(operation) + }, + deliveries + ) + } + + private func nextMainRunLoopTurn() async { + await withCheckedContinuation { continuation in + DispatchQueue.main.async { continuation.resume() } + } + } + private final class Host {} private final class Presenter: AgentInboxPopoverPresenting { diff --git a/PineTests/AgentInboxPopoverSystemObjectsTests.swift b/PineTests/AgentInboxPopoverSystemObjectsTests.swift new file mode 100644 index 00000000..5ed5d70c --- /dev/null +++ b/PineTests/AgentInboxPopoverSystemObjectsTests.swift @@ -0,0 +1,292 @@ +// +// AgentInboxPopoverSystemObjectsTests.swift +// PineTests +// +// The production `NSPopover` the anchor builds, and the AppKit conformances +// every other suite substitutes away (#1491). +// +// `AgentInboxPopoverCoordinatorTests` proves the orderings against a fake +// handle, and `AgentInboxPresentationCoordinatorTests` proves host selection +// against a fake host. That is what makes those orderings observable at all — +// but it also means the real objects underneath them are reached by nothing: +// `makeSystemPopover` could return a popover with the wrong behavior, the +// wrong size, or no delegate, and `showPopover(from:)` could hang the Inbox +// off the wrong edge of the toolbar button, with every one of those suites +// green. +// + +import AppKit +import SwiftUI +import Testing + +@testable import Pine + +@Suite("Agent Inbox popover system objects", .serialized) +@MainActor +struct AgentInboxPopoverSystemObjectsTests { + // MARK: - The popover the anchor builds + + @Test("the production popover is transient, sized, and delegated") + func systemPopoverIsConfiguredForTheToolbar() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + let delegate = Delegate() + + let handle = try #require( + AgentInboxPopoverCoordinator.makeSystemPopover( + anchor: fixture.anchor, + context: fixture.context(delegate: delegate, reduceMotion: false) + ) + ) + let popover = try #require(handle as? NSPopover) + + // `.transient` is what hands Escape and outside clicks to AppKit — + // the whole reason the anchor needs `popoverWillClose` at all. + #expect(popover.behavior == .transient) + #expect(popover.contentSize == NSSize(width: 520, height: 540)) + #expect( + popover.delegate === delegate, + "Without the delegate the anchor never hears a transient close" + ) + let hosting = try #require( + popover.contentViewController as? NSHostingController + ) + #expect(hosting.preferredContentSize == popover.contentSize) + } + + @Test("Reduce Motion turns the popover's animation off", arguments: [ + false, true, + ]) + func systemPopoverFollowsReduceMotion(reduceMotion: Bool) throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + + let handle = try #require( + AgentInboxPopoverCoordinator.makeSystemPopover( + anchor: fixture.anchor, + context: fixture.context( + delegate: Delegate(), + reduceMotion: reduceMotion + ) + ) + ) + let popover = try #require(handle as? NSPopover) + + #expect(popover.animates == !reduceMotion) + } + + /// An anchor SwiftUI has not finished configuring builds nothing, and + /// records nothing — the caller must be able to tell "not ready" from + /// "shown", because it stores the result as the live popover either way. + @Test("an incompletely configured anchor builds no popover", arguments: [ + (hasRegistry: true, hasOpenWindow: false), + (hasRegistry: false, hasOpenWindow: true), + (hasRegistry: false, hasOpenWindow: false), + ]) + func systemPopoverRefusesAnUnreadyAnchor( + hasRegistry: Bool, + hasOpenWindow: Bool + ) throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + + let handle = AgentInboxPopoverCoordinator.makeSystemPopover( + anchor: fixture.anchor, + context: fixture.context( + delegate: Delegate(), + reduceMotion: true, + hasRegistry: hasRegistry, + hasOpenWindow: hasOpenWindow + ) + ) + + #expect(handle == nil) + } + + // MARK: - NSPopover's conformance + + /// `isPopoverVisible` exists so no caller reads `NSPopover.isShown` + /// directly, and the whole close state machine is written over its answer. + /// A conformance that returned a constant would make every ordering in + /// `AgentInboxPopoverCoordinatorTests` vacuous. + @Test("the popover handle reports AppKit's own visibility") + func popoverHandleTracksAppKitVisibility() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + let popover = fixture.makeBarePopover() + + #expect(!popover.isPopoverVisible) + popover.showPopover(from: fixture.anchor) + #expect(popover.isPopoverVisible) + #expect(popover.isShown) + + popover.closePopover() + #expect(!popover.isPopoverVisible) + } + + /// The Inbox hangs *below* the toolbar button, which is what + /// `preferredEdge: .minY` means and the only part of the show call a + /// caller can observe afterwards. The other edge would put a 540-point + /// popover above a button that sits in the title bar. + @Test("the popover is anchored below the button, not above it") + func popoverHangsBelowItsAnchor() throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + let popover = fixture.makeBarePopover() + + popover.showPopover(from: fixture.anchor) + defer { popover.closePopover() } + + let anchorFrame = fixture.anchorFrameInScreen() + let popoverFrame = try #require( + popover.contentViewController?.view.window?.frame + ) + #expect(popoverFrame.maxY <= anchorFrame.minY) + // Not a tautology about screen geometry: the anchor is placed with + // room on both sides, so `.maxY` would have put the popover here. + #expect(anchorFrame.maxY < fixture.windowFrameInScreen().maxY) + } + + // MARK: - NSWindow's conformance + + /// `NSWindow`'s side of ``AgentInboxHosting``, as far as this host can see + /// it — which is not all of it. + /// + /// **`makeKeyAndOrderFront` versus `orderFront` is not covered and cannot + /// be covered here.** The unit-test host is a background application: + /// `NSApp.isActive` is `false`, `makeKeyAndOrderFront` does not confer key + /// status, and `NSApp.keyWindow` stays `nil` — measured from inside this + /// host, not assumed. #1513 documents the same limitation for + /// `AgentInboxWindowSources.keyWindow`. Mutating `focusHost()` to + /// `orderFront(nil)`, which raises the host without focusing it so the + /// Inbox opens over a window the user is not typing in, survives this + /// suite. It is reported rather than papered over. + /// + /// `restoreHostFromMiniaturized()` is unreachable for the same reason: + /// `miniaturize(nil)` does not reach the Dock in a background application, + /// so `isMiniaturized` never becomes `true` and there is nothing to + /// restore. What remains reachable is the ordering half below. + @Test("focusing a host brings it back on screen") + func focusingAHostBringsItBackOnScreen() { + let host = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 200, height: 160), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + defer { host.orderOut(nil) } + host.orderFront(nil) + #expect(!host.isHostMiniaturized) + + host.orderOut(nil) + #expect(!host.isVisible) + + host.focusHost() + + #expect( + host.isVisible, + "A host that is not on screen has nowhere to draw the popover" + ) + } + + // MARK: - Fixture + + @MainActor + private final class Fixture { + let anchor = AgentInboxPopoverAnchorView( + frame: NSRect(x: 150, y: 260, width: 40, height: 24) + ) + private let registry: ProjectRegistry + private let window: NSWindow + private let suiteName: String + private let defaults: UserDefaults + + init() throws { + suiteName = "AgentInboxPopoverSystemObjectsTests.\(UUID())" + defaults = try #require(UserDefaults(suiteName: suiteName)) + defaults.removePersistentDomain(forName: suiteName) + registry = ProjectRegistry( + defaults: defaults, + agentTasks: AgentTaskRegistry(), + // No `ps` polling: this suite is about AppKit objects. + agentDetectionProcessRunner: { _, _, _, _ in + ProcessRunResult( + stdout: "", + stderr: "", + exitCode: 0, + timedOut: false + ) + }, + agentDetectionPollInterval: 3_600, + agentDetectionInitialPollDelay: 3_600 + ) + registry.recentProjects = [] + // Centred, so the edge assertion is about `preferredEdge` and not + // about which screen corner the window happened to land in. + let screen = NSScreen.main?.visibleFrame + ?? NSRect(x: 0, y: 0, width: 1_440, height: 900) + window = NSWindow( + contentRect: NSRect( + x: screen.midX - 200, + y: screen.midY - 200, + width: 400, + height: 400 + ), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.contentView = NSView( + frame: NSRect(x: 0, y: 0, width: 400, height: 400) + ) + window.contentView?.addSubview(anchor) + window.orderFront(nil) + } + + func context( + delegate: any NSPopoverDelegate, + reduceMotion: Bool, + hasRegistry: Bool = true, + hasOpenWindow: Bool = true + ) -> AgentInboxPopoverCoordinator.Context { + AgentInboxPopoverCoordinator.Context( + registry: hasRegistry ? registry : nil, + openProjectWindow: hasOpenWindow ? { _ in } : nil, + reduceMotion: reduceMotion, + delegate: delegate, + onDismiss: {} + ) + } + + /// A popover with no SwiftUI content, so showing it exercises the + /// conformance rather than the Inbox's own view body. + func makeBarePopover() -> NSPopover { + let popover = NSPopover() + popover.behavior = .applicationDefined + popover.animates = false + popover.contentSize = NSSize(width: 120, height: 90) + let controller = NSViewController() + controller.view = NSView( + frame: NSRect(x: 0, y: 0, width: 120, height: 90) + ) + popover.contentViewController = controller + return popover + } + + func anchorFrameInScreen() -> NSRect { + window.convertToScreen(anchor.convert(anchor.bounds, to: nil)) + } + + func windowFrameInScreen() -> NSRect { + window.frame + } + + func cleanup() { + anchor.removeFromSuperview() + window.orderOut(nil) + defaults.removePersistentDomain(forName: suiteName) + } + } + + private final class Delegate: NSObject, NSPopoverDelegate {} +} diff --git a/PineTests/AgentInboxPresentationCoordinatorTests.swift b/PineTests/AgentInboxPresentationCoordinatorTests.swift new file mode 100644 index 00000000..1e03d3a6 --- /dev/null +++ b/PineTests/AgentInboxPresentationCoordinatorTests.swift @@ -0,0 +1,783 @@ +// +// AgentInboxPresentationCoordinatorTests.swift +// PineTests +// +// End-to-end coverage for the Agent Inbox presentation workflow (#1491): +// host selection, restore-and-focus, and the single-request rule. +// + +import Testing + +@testable import Pine + +@Suite("Agent Inbox presentation coordinator", .serialized) +@MainActor +struct AgentInboxPresentationCoordinatorTests { + // MARK: - Existing hosts + + @Test("the chosen host is focused and receives the deferred request") + func routesToChosenHost() { + let fixture = Fixture() + let host = fixture.addProject(name: "alpha", isKey: true) + let presenter = fixture.registerAnchor(for: host) + + #expect(fixture.coordinator.present() == .routedToExistingHost) + // Delivery is deferred so a menu command cannot present the popover + // inside its own synchronous dispatch. + #expect(presenter.presentationCount == 0) + + fixture.environment.flushDeliveries() + + #expect(presenter.presentationCount == 1) + #expect(fixture.environment.journal == [ + "activate", + "focus(alpha)", + "activate", + "deliver", + "present(alpha)", + ]) + } + + /// `AgentInboxHosting` promises restore-then-focus, and this pins that + /// ordering — but it is a **protocol contract test, not evidence of a + /// user-visible behavior**. `AppDelegate.agentInboxHostOptions` derives + /// eligibility from `NSWindow.isVisible`, which reads `false` while a + /// window is in the Dock, and `visibleWelcomeWindow()` rejects + /// miniaturized windows outright — so this workflow never selects a + /// miniaturized host and cannot reach `restoreHostFromMiniaturized()`. + /// + /// The gap is narrower than "minimized hosts are not restored": a + /// minimized Welcome window *is* brought back, because selection skips it, + /// the decision falls through to `.createWelcomeHost`, and + /// `ensureWelcomeVisible()` deminiaturizes it. What no code does is return + /// the user to a minimized **project** window: that window is silently + /// bypassed in favour of Welcome. #1491's "minimized hosts are restored + /// before presentation" is therefore **not covered** for project windows; + /// widening eligibility changes where ⇧⌘I lands and belongs in its own + /// change (#1507). This test exists so that change inherits a specified + /// workflow rather than an unspecified one. + @Test("the hosting contract restores a host before it focuses it") + func hostingContractRestoresBeforeFocusing() { + let fixture = Fixture() + let host = fixture.addProject( + name: "alpha", + isKey: true, + isMiniaturized: true + ) + let presenter = fixture.registerAnchor(for: host) + + fixture.coordinator.present() + fixture.environment.flushDeliveries() + + #expect(fixture.environment.journal == [ + "activate", + "restore(alpha)", + "focus(alpha)", + "activate", + "deliver", + "present(alpha)", + ]) + #expect(!host.isHostMiniaturized) + #expect(presenter.presentationCount == 1) + } + + @Test("a host that is already on screen is not restored") + func doesNotRestoreVisibleHost() { + let fixture = Fixture() + let host = fixture.addProject(name: "alpha", isKey: true) + fixture.registerAnchor(for: host) + + fixture.coordinator.present() + fixture.environment.flushDeliveries() + + #expect(!fixture.environment.journal.contains("restore(alpha)")) + } + + @Test("one request never opens a popover in a second window") + func requestNeverFansOut() { + let fixture = Fixture() + let background = fixture.addProject(name: "background", isKey: false) + let key = fixture.addProject(name: "key", isKey: true) + let welcome = fixture.addWelcome(name: "welcome", isKey: false) + let backgroundAnchor = fixture.registerAnchor(for: background) + let keyAnchor = fixture.registerAnchor(for: key) + let welcomeAnchor = fixture.registerAnchor(for: welcome) + + fixture.coordinator.present() + fixture.environment.flushDeliveries() + + #expect(keyAnchor.presentationCount == 1) + #expect(backgroundAnchor.presentationCount == 0) + #expect(welcomeAnchor.presentationCount == 0) + #expect(!fixture.environment.journal.contains("focus(background)")) + #expect(!fixture.environment.journal.contains("focus(welcome)")) + } + + /// A window that is alive but has not mounted its anchor answers + /// `.queued`, and that is a healthy answer: the router hands the request + /// over the moment the anchor appears. Treating it as a lost host re-runs + /// selection, raising a window the user did not ask for — and, because + /// focus is asynchronous, the second pass can read a pre-focus key window + /// and route the Inbox somewhere else. + @Test("a host whose anchor has not mounted is waited for, not re-selected") + func queuedHostIsNotTreatedAsLost() async { + let fixture = Fixture() + let host = fixture.addProject(name: "alpha", isKey: true) + let other = fixture.addProject( + name: "other", + isKey: false, + showsMostRecentlyActiveProject: true + ) + let otherAnchor = fixture.registerAnchor(for: other) + + #expect(fixture.coordinator.present() == .routedToExistingHost) + fixture.environment.flushDeliveries() + // A retry would deliver a second time and focus a second window. + fixture.environment.flushDeliveries() + + #expect(fixture.environment.deliveryCount == 1) + #expect(fixture.environment.welcomeCreationCount == 0) + #expect(otherAnchor.presentationCount == 0) + #expect( + fixture.environment.journal.filter { $0 == "focus(alpha)" }.count + == 1 + ) + #expect(!fixture.environment.journal.contains("focus(other)")) + + // The request was not dropped either: it is still armed on the router + // and the window serves it as soon as its anchor mounts. + let anchor = fixture.registerAnchor(for: host) + #expect(anchor.presentationCount == 1) + + // The bounded wait standing behind it retires quietly once the request + // has been served. A wait that kept polling would either present a + // second time into the window the user is already looking at, or spend + // its budget and open Welcome behind the Inbox it opened. + await fixture.settle(turns: 200) + #expect(anchor.presentationCount == 1) + #expect(fixture.environment.welcomeCreationCount == 0) + #expect(!fixture.coordinator.isAwaitingCreatedWelcomeHost) + } + + /// The bound on that wait, and why it has to exist. + /// + /// The anchor lives in a `ToolbarItem` and its registration is keyed by + /// `anchor.window`, so AppKit takes it out of the window whenever the + /// toolbar is collapsed, whenever it overflows on a narrow window, and when + /// full screen moves the toolbar container into `NSToolbarFullScreenWindow` + /// — which carries no `CloseDelegate` and never becomes a candidate. In all + /// of those the anchor mounts **never**, which + /// `queuedHostIsNotTreatedAsLost` above cannot distinguish from "mounts a + /// turn later" and does not try to. + /// + /// Unbounded, that costs twice: the command does nothing at all, and the + /// request stays armed on the shared router until that window next mounts + /// an anchor — the Inbox opening by itself minutes later, which is the + /// hazard `cancelQueuedRequest` claims to have removed. + @Test("a host whose anchor never mounts is retired, not left armed") + func anchorlessHostRetiresItsRequestAndFallsBack() async { + let fixture = Fixture() + let host = fixture.addProject(name: "alpha", isKey: true) + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + // Welcome is created but does not arrive yet, which is the state the + // real `awaitVisibleWelcomeWindow()` spends up to a second in. + fixture.environment.holdsWelcomeHost = true + + #expect(fixture.coordinator.present() == .routedToExistingHost) + fixture.environment.flushDeliveries() + #expect(fixture.router.hasQueuedRequest(for: host)) + + await fixture.settle(turns: 200) + #expect(fixture.environment.anchorWaitCount > 0) + #expect(fixture.environment.welcomeCreationCount == 1) + + // The request is retired *before* Welcome is asked for, not after it + // arrives. Anything left armed across that second would be handed to + // the anchorless window the moment it did mount an anchor — an Inbox + // there and a Welcome window created beside it, from one keystroke. + #expect(!fixture.router.hasQueuedRequest(for: host)) + let late = fixture.registerAnchor(for: host) + #expect(late.presentationCount == 0) + + // And the keystroke was not swallowed: Welcome hosts it instead, which + // is the same answer an ineligible desktop already gets (#1486). + fixture.environment.releaseWelcomeHost() + await fixture.settle(turns: 200) + let welcomeAnchor = fixture.registerAnchor(for: welcome) + #expect(welcomeAnchor.presentationCount == 1) + } + + @Test("an auxiliary key window routes to the most recent project") + func auxiliaryKeyWindowRoutesToMostRecentProject() { + let fixture = Fixture() + // Settings holds key, so no candidate is key at all. + let other = fixture.addProject(name: "other", isKey: false) + let recent = fixture.addProject( + name: "recent", + isKey: false, + showsMostRecentlyActiveProject: true + ) + fixture.addWelcome(name: "welcome", isKey: false) + let otherAnchor = fixture.registerAnchor(for: other) + let recentAnchor = fixture.registerAnchor(for: recent) + + fixture.coordinator.present() + fixture.environment.flushDeliveries() + + #expect(recentAnchor.presentationCount == 1) + #expect(otherAnchor.presentationCount == 0) + } + + @Test("a visible Welcome window is the final existing-window host") + func visibleWelcomeIsFinalFallback() { + let fixture = Fixture() + fixture.addProject(name: "unrelated", isKey: false) + let welcome = fixture.addWelcome(name: "welcome", isKey: false) + let welcomeAnchor = fixture.registerAnchor(for: welcome) + + #expect(fixture.coordinator.present() == .routedToExistingHost) + fixture.environment.flushDeliveries() + + #expect(welcomeAnchor.presentationCount == 1) + #expect(fixture.environment.welcomeCreationCount == 0) + } + + /// Named for what it proves: every repeat lands in the *same* window and + /// focuses it again. It says nothing about focus returning to that window + /// after the popover closes — #1491's last criterion, which has no + /// production code behind it and is left to `NSPopover`'s own key-window + /// restoration. **That criterion is not covered.** No test here or in + /// `AgentInboxToolbarButtonTests` can stand in for it: host selection + /// prefers the most recently active project as well as the key window, so + /// a command still reaches a single project window whether or not focus + /// came back to it. + @Test("every repeated request focuses and reuses the same host window") + func repeatedRequestsReuseAndRefocusTheSameHost() { + let fixture = Fixture() + let other = fixture.addProject(name: "other", isKey: false) + let key = fixture.addProject(name: "key", isKey: true) + let otherAnchor = fixture.registerAnchor(for: other) + let keyAnchor = fixture.registerAnchor(for: key) + + for _ in 0..<3 { + fixture.coordinator.present() + fixture.environment.flushDeliveries() + } + + #expect(keyAnchor.presentationCount == 3) + #expect(otherAnchor.presentationCount == 0) + #expect( + fixture.environment.journal.filter { $0 == "focus(key)" }.count == 3 + ) + } + + // MARK: - Creating Welcome + + @Test("no eligible window creates Welcome exactly once per request") + func createsWelcomeWhenNothingIsEligible() async { + let fixture = Fixture() + fixture.addProject(name: "closing", isKey: true, isEligible: false) + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + #expect(fixture.environment.welcomeCreationCount == 1) + #expect(fixture.coordinator.isAwaitingCreatedWelcomeHost) + + await fixture.settle() + + // The anchor has not mounted yet, so the single request waits. + let presenter = fixture.registerAnchor(for: welcome) + #expect(presenter.presentationCount == 1) + #expect(!fixture.coordinator.isAwaitingCreatedWelcomeHost) + } + + @Test("the created Welcome host is focused before routing") + func createdWelcomeHostIsPrepared() async { + let fixture = Fixture() + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + let presenter = fixture.registerAnchor(for: welcome) + + fixture.coordinator.present() + await fixture.settle() + + #expect(presenter.presentationCount == 1) + // A created Welcome window is resolved through + // `awaitVisibleWelcomeWindow()`, which only ever yields a visible, + // non-miniaturized window, so no restore step appears here. + #expect(fixture.environment.journal == [ + "activate", + "createWelcome", + "awaitWelcome", + "focus(welcome)", + "activate", + "present(welcome)", + ]) + } + + @Test("repeated requests while Welcome mounts present exactly once") + func repeatedRequestsWhileWelcomeMountsPresentOnce() async { + let fixture = Fixture() + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + let presenter = fixture.registerAnchor(for: welcome) + + for _ in 0..<4 { + #expect( + fixture.coordinator.present() == .awaitingCreatedWelcomeHost + ) + } + await fixture.settle() + + // Every superseded request is cancelled, so the user gets one popover + // rather than four stacked in the same window. + #expect(presenter.presentationCount == 1) + #expect(fixture.environment.welcomeCreationCount == 4) + } + + @Test("a request superseded before its host arrives is abandoned") + func supersededWelcomeRequestIsAbandoned() async { + let fixture = Fixture() + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + fixture.environment.holdsWelcomeHost = true + let welcomeAnchor = fixture.registerAnchor(for: welcome) + + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + await fixture.settle() + #expect(fixture.environment.pendingWelcomeRequestCount == 1) + + // A project window appears and a second request takes it. + let project = fixture.addProject(name: "alpha", isKey: true) + let projectAnchor = fixture.registerAnchor(for: project) + #expect(fixture.coordinator.present() == .routedToExistingHost) + fixture.environment.flushDeliveries() + + // The stalled Welcome finally arrives; it must not open a second one. + fixture.environment.releaseWelcomeHost() + await fixture.settle() + + #expect(projectAnchor.presentationCount == 1) + #expect(welcomeAnchor.presentationCount == 0) + } + + @Test("a superseded created-Welcome task cannot clear its successor") + func supersededWelcomeTaskCannotClearItsSuccessor() async { + let fixture = Fixture() + let first = fixture.environment.stageWelcomeHost(name: "welcomeA") + fixture.environment.holdsWelcomeHost = true + + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + await fixture.settle() + #expect(fixture.environment.pendingWelcomeRequestCount == 1) + + // A second request supersedes the first while it is still suspended, + // so two Welcome hand-offs are outstanding at once. + let second = fixture.environment.stageWelcomeHost(name: "welcomeB") + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + await fixture.settle() + #expect(fixture.environment.pendingWelcomeRequestCount == 2) + + // Let only the superseded one finish. Without the generation guard its + // cleanup clears the in-flight marker that now belongs to its + // successor, and the live request reports itself as already finished. + fixture.environment.releaseWelcomeHost(at: 0) + await fixture.settle() + #expect(fixture.coordinator.isAwaitingCreatedWelcomeHost) + + let firstAnchor = fixture.registerAnchor(for: first) + let secondAnchor = fixture.registerAnchor(for: second) + fixture.environment.releaseWelcomeHost(at: 0) + await fixture.settle() + + #expect(firstAnchor.presentationCount == 0) + #expect(secondAnchor.presentationCount == 1) + #expect(!fixture.coordinator.isAwaitingCreatedWelcomeHost) + } + + @Test("a new request retires the one still queued on the shared router") + func newRequestRetiresTheQueuedRequest() async { + let fixture = Fixture() + let welcome = fixture.environment.stageWelcomeHost(name: "welcome") + + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + await fixture.settle() + + // Welcome exists but has not mounted its anchor, so the request sits + // on the router with nothing of its own to expire it. + let project = fixture.addProject(name: "alpha", isKey: true) + let projectAnchor = fixture.registerAnchor(for: project) + #expect(fixture.coordinator.present() == .routedToExistingHost) + + // Welcome mounts its anchor in the turn before the new request is + // delivered. Unless the superseded request was retired the moment it + // lost, this single ⇧⌘I opens the Inbox in two windows at once. + let welcomeAnchor = fixture.registerAnchor(for: welcome) + fixture.environment.flushDeliveries() + await fixture.settle() + + #expect(welcomeAnchor.presentationCount == 0) + #expect(projectAnchor.presentationCount == 1) + } + + @Test("a Welcome host that never appears leaves nothing pending") + func missingWelcomeHostLeavesNothingPending() async { + let fixture = Fixture() + fixture.environment.welcomeHost = nil + + #expect(fixture.coordinator.present() == .awaitingCreatedWelcomeHost) + await fixture.settle() + + #expect(!fixture.coordinator.isAwaitingCreatedWelcomeHost) + #expect(fixture.environment.journal == [ + "activate", + "createWelcome", + "awaitWelcome", + ]) + + // The workflow is still usable afterwards. + let host = fixture.addProject(name: "alpha", isKey: true) + let presenter = fixture.registerAnchor(for: host) + fixture.coordinator.present() + fixture.environment.flushDeliveries() + #expect(presenter.presentationCount == 1) + } + + // MARK: - Lifecycle + + @Test("a released environment cannot be presented into") + func releasedEnvironmentIsUnavailable() { + let router = AgentInboxPopoverRouter() + var coordinator: AgentInboxPresentationCoordinator? + do { + let environment = Environment() + coordinator = AgentInboxPresentationCoordinator( + router: router, + environment: environment + ) + } + + #expect(coordinator?.present() == .unavailable) + } + + @Test("a released host window drops its queued request") + func releasedHostDropsQueuedRequest() async { + let fixture = Fixture() + fixture.environment.stageWelcomeHost(name: "welcome") + + fixture.coordinator.present() + await fixture.settle() + + // The Welcome window is torn down before its anchor ever mounts. + fixture.environment.welcomeHost = nil + let unrelated = fixture.addProject(name: "alpha", isKey: true) + let unrelatedAnchor = fixture.registerAnchor(for: unrelated) + + // Mounting an unrelated window must not inherit the dead request… + #expect(unrelatedAnchor.presentationCount == 0) + // …while that window still answers a request addressed to it, which + // is what makes the line above evidence rather than a tautology. + #expect(fixture.router.requestPresentation(in: unrelated) == .presented) + #expect(unrelatedAnchor.presentationCount == 1) + } + + @Test("an undelivered request never keeps its host window alive") + func deliveryDoesNotRetainItsHost() { + let fixture = Fixture() + weak var weakHost: Host? + + do { + let host = fixture.addProject(name: "closing", isKey: true) + weakHost = host + #expect(fixture.coordinator.present() == .routedToExistingHost) + } + fixture.environment.options.removeAll() + + // Delivery is still outstanding. A strong capture here would hold a + // closing NSWindow alive for a whole extra runloop turn. + #expect(weakHost == nil) + } + + @Test("a host that dies before delivery re-selects instead of vanishing") + func lostHostReselectsRatherThanDroppingTheRequest() { + let fixture = Fixture() + var doomed: Host? = fixture.addProject(name: "doomed", isKey: true) + let survivor = fixture.addProject( + name: "survivor", + isKey: false, + showsMostRecentlyActiveProject: true + ) + let survivorAnchor = fixture.registerAnchor(for: survivor) + + #expect(fixture.coordinator.present() == .routedToExistingHost) + + // The chosen window closes in the turn between selection and delivery. + fixture.environment.removeOption(for: doomed) + doomed = nil + + fixture.environment.flushDeliveries() + // The retry re-runs selection and is itself deferred. + fixture.environment.flushDeliveries() + + #expect(survivorAnchor.presentationCount == 1) + } + + @Test("re-selection is bounded, so a hostless desktop cannot spin") + func lostHostRetryIsBounded() async { + let fixture = Fixture() + var doomed: Host? = fixture.addProject(name: "doomed", isKey: true) + + #expect(fixture.coordinator.present() == .routedToExistingHost) + fixture.environment.removeOption(for: doomed) + doomed = nil + + for _ in 0..<6 { + fixture.environment.flushDeliveries() + } + + // One retry; it finds nothing eligible and falls through to creating + // Welcome rather than re-delivering forever. + #expect(fixture.environment.deliveryCount == 1) + #expect(fixture.environment.welcomeCreationCount == 1) + + await fixture.settle() + #expect(!fixture.coordinator.isAwaitingCreatedWelcomeHost) + } + + // MARK: - Fixture + + @MainActor + private final class Fixture { + // The router's own deferral is its contract and is covered in + // `AgentInboxPopoverRouterTests`; here it would only blur the + // selection ordering these tests exist to pin. + let router = AgentInboxPopoverRouter { operation in operation() } + let environment = Environment() + lazy var coordinator = AgentInboxPresentationCoordinator( + router: router, + environment: environment + ) + private var anchors: [Anchor] = [] + + @discardableResult + func addProject( + name: String, + isKey: Bool, + isEligible: Bool = true, + isMiniaturized: Bool = false, + showsMostRecentlyActiveProject: Bool = false + ) -> Host { + let host = Host( + name: name, + journal: environment, + isMiniaturized: isMiniaturized + ) + environment.options.append(AgentInboxHostOption( + candidate: AgentInboxHostCandidate( + kind: .project, + isKeyWindow: isKey, + isEligibleWindow: isEligible, + showsMostRecentlyActiveProject: + showsMostRecentlyActiveProject + ), + host: host + )) + return host + } + + @discardableResult + func addWelcome(name: String, isKey: Bool) -> Host { + let host = Host(name: name, journal: environment) + environment.options.append(AgentInboxHostOption( + candidate: AgentInboxHostCandidate( + kind: .welcome, + isKeyWindow: isKey + ), + host: host + )) + return host + } + + @discardableResult + func registerAnchor(for host: Host) -> Anchor { + let anchor = Anchor(name: host.name, journal: environment) + anchors.append(anchor) + router.register(anchor, for: host) + return anchor + } + + /// Lets every superseded and surviving presentation task run to + /// completion. A created-Welcome request suspends at most twice; a + /// request waiting on an existing host's anchor suspends once per + /// attempt, so exhausting that budget needs a much larger `turns`. + func settle(turns: Int = 12) async { + for _ in 0.. + } + + var options: [AgentInboxHostOption] = [] + var welcomeHost: Host? + /// Suspends `awaitAgentInboxWelcomeHost()` until explicitly released. + var holdsWelcomeHost = false + private(set) var journal: [String] = [] + private(set) var welcomeCreationCount = 0 + private(set) var deliveryCount = 0 + private(set) var anchorWaitCount = 0 + private var deliveries: [@MainActor () -> Void] = [] + private var pendingWelcomes: [PendingWelcome] = [] + + var pendingWelcomeRequestCount: Int { + pendingWelcomes.count + } + + func removeOption(for host: Host?) { + guard let host else { return } + options.removeAll { $0.host === host } + } + + func record(_ entry: String) { + journal.append(entry) + } + + @discardableResult + func stageWelcomeHost( + name: String, + isMiniaturized: Bool = false + ) -> Host { + let host = Host( + name: name, + journal: self, + isMiniaturized: isMiniaturized + ) + welcomeHost = host + return host + } + + func releaseWelcomeHost() { + holdsWelcomeHost = false + let pending = pendingWelcomes + pendingWelcomes.removeAll() + for entry in pending { + entry.continuation.resume(returning: entry.host) + } + } + + /// Finishes exactly one outstanding hand-off, leaving the rest + /// suspended. + func releaseWelcomeHost(at index: Int) { + guard pendingWelcomes.indices.contains(index) else { return } + let entry = pendingWelcomes.remove(at: index) + entry.continuation.resume(returning: entry.host) + } + + func flushDeliveries() { + let pending = deliveries + deliveries.removeAll() + for operation in pending { + operation() + } + } + + // MARK: AgentInboxHostEnvironment + + func agentInboxHostOptions() -> [AgentInboxHostOption] { + options + } + + func activateApplicationForAgentInbox() { + record("activate") + } + + func createAgentInboxWelcomeHost() { + welcomeCreationCount += 1 + record("createWelcome") + } + + func awaitAgentInboxWelcomeHost() async -> (any AgentInboxHosting)? { + record("awaitWelcome") + guard holdsWelcomeHost else { return welcomeHost } + // The staged host is captured now, not on release: a later + // request stages its own window and must not retarget this one. + let staged = welcomeHost + let host: Host? = await withCheckedContinuation { continuation in + pendingWelcomes.append(PendingWelcome( + host: staged, + continuation: continuation + )) + } + return host + } + + func deliverAgentInboxRequest( + _ operation: @escaping @MainActor () -> Void + ) { + record("deliver") + deliveryCount += 1 + deliveries.append(operation) + } + + /// Counted rather than journalled: the bounded wait runs dozens of + /// times and would drown the effect orderings the journal exists to + /// pin. + func waitForAgentInboxAnchor() async { + anchorWaitCount += 1 + await Task.yield() + } + } + + @MainActor + private final class Host: AgentInboxHosting { + let name: String + private(set) var isHostMiniaturized: Bool + private unowned let journal: Environment + + init( + name: String, + journal: Environment, + isMiniaturized: Bool = false + ) { + self.name = name + self.journal = journal + self.isHostMiniaturized = isMiniaturized + } + + func restoreHostFromMiniaturized() { + isHostMiniaturized = false + journal.record("restore(\(name))") + } + + func focusHost() { + journal.record("focus(\(name))") + } + } + + @MainActor + private final class Anchor: AgentInboxPopoverPresenting { + let name: String + private(set) var presentationCount = 0 + private unowned let journal: Environment + + init(name: String, journal: Environment) { + self.name = name + self.journal = journal + } + + func presentAgentInbox() { + presentationCount += 1 + journal.record("present(\(name))") + } + } +} diff --git a/PineUITests/AgentInboxToolbarButtonTests.swift b/PineUITests/AgentInboxToolbarButtonTests.swift index 47109bf8..1cee68e8 100644 --- a/PineUITests/AgentInboxToolbarButtonTests.swift +++ b/PineUITests/AgentInboxToolbarButtonTests.swift @@ -17,6 +17,22 @@ final class AgentInboxToolbarButtonTests: PineUITestCase { app.buttons["agentInboxToolbarButton"].firstMatch } + private var inbox: XCUIElement { + app.descendants(matching: .any)["agentInbox"].firstMatch + } + + /// Binds the project window by its title. + /// + /// `app.windows.firstMatch` must never be used here: AX window lists run + /// front to back, so once a popover is open `firstMatch` can resolve to + /// the popover's own window and a "click outside" would land inside the + /// Inbox list. A title-bound query can only ever name the project window. + private func projectWindows(for url: URL) -> XCUIElementQuery { + app.windows.matching( + NSPredicate(format: "title == %@", url.lastPathComponent) + ) + } + override func tearDownWithError() throws { for url in projectURLs { cleanupProject(url) } try super.tearDownWithError() @@ -44,8 +60,6 @@ final class AgentInboxToolbarButtonTests: PineUITestCase { ) toolbarButton.click() - let inbox = app.descendants(matching: .any)["agentInbox"] - .firstMatch XCTAssertTrue( waitForExistence(inbox, timeout: 5), "Clicking the toolbar button should open the Agent Inbox popover" @@ -60,6 +74,127 @@ final class AgentInboxToolbarButtonTests: PineUITestCase { inbox.waitForNonExistence(timeout: 3), "Escape should dismiss the Agent Inbox popover" ) + + // Escape closes the popover behind SwiftUI's back, and the binding is + // lowered a runloop turn later. Without that write the anchor still + // believes it is presenting and silently refuses the next request. + toolbarButton.click() + XCTAssertTrue( + waitForExistence(inbox, timeout: 5), + "The toolbar button should reopen the Inbox after Escape" + ) + } + + /// AppKit owns transient dismissal: no unit seam can prove that clicking + /// outside really closes the popover, that SwiftUI's binding follows it, + /// or that the window stays usable afterwards. + /// + /// It is **not** evidence for #1491's "focus returns to the correct host + /// after dismissal", and does not claim to be. Host selection prefers the + /// most recently active project as well as the key window, and + /// `ProjectRegistry.keyWindowSession()` stays non-nil for the last + /// registered window whatever AppKit's key status is — so with one project + /// window open the View menu lands here either way. The click that + /// dismisses the popover also makes this window key by itself. That + /// criterion has no production code and no test. + func testOutsideClickDismissesInboxAndStaysRoutable() throws { + let url = try createTempProject(files: ["hello.swift": "// hi\n"]) + projectURLs.append(url) + launchWithProject(url) + + let window = projectWindows(for: url).firstMatch + XCTAssertTrue(waitForExistence(window, timeout: 10)) + XCTAssertTrue(waitForExistence(toolbarButton, timeout: 10)) + toolbarButton.click() + + XCTAssertTrue( + waitForExistence(inbox, timeout: 5), + "The toolbar button should open the Agent Inbox popover" + ) + + // Bottom-left of the project window: clear of a 520x540 popover + // hanging below the trailing-edge toolbar button, and clear of the + // single file row at the top of the sidebar, so the window keeps its + // title and this element keeps resolving. + window.coordinate( + withNormalizedOffset: CGVector(dx: 0.1, dy: 0.93) + ).click() + + XCTAssertTrue( + inbox.waitForNonExistence(timeout: 5), + "Clicking outside should dismiss the Agent Inbox popover" + ) + + // If the binding had not followed AppKit's dismissal, the anchor would + // still believe it is presenting and refuse the next request. + toolbarButton.click() + XCTAssertTrue( + waitForExistence(inbox, timeout: 5), + "The same window should host the Inbox again after a dismissal" + ) + app.typeKey(.escape, modifierFlags: []) + XCTAssertTrue(inbox.waitForNonExistence(timeout: 5)) + + clickMenuBarItem("View") + let menuItem = app.menuItems["Agent Inbox"] + XCTAssertTrue(waitForExistence(menuItem, timeout: 5)) + menuItem.click() + + XCTAssertTrue( + waitForExistence(inbox, timeout: 10), + "The window must still be routable after a transient dismissal" + ) + XCTAssertEqual( + projectWindows(for: url).count, + 1, + "Reopening must reuse the project window, not add another" + ) + XCTAssertFalse( + app.windows["Agent Inbox"].exists, + "Agent Inbox should never become a separate window" + ) + } + + /// The View menu goes through `AppDelegate`'s real host selection rather + /// than the anchor's own binding, so only a running app can prove that an + /// application-level command lands in the project window it is issued + /// from — and reuses it instead of creating Welcome beside it. + /// + /// Single-window only. #1491's multi-window criteria are covered at the + /// `AgentInboxPresentationCoordinator` seam, not end to end. + func testViewMenuOpensInboxInTheProjectWindowItReuses() throws { + let url = try createTempProject(files: ["hello.swift": "// hi\n"]) + projectURLs.append(url) + launchWithProject(url) + + let window = projectWindows(for: url).firstMatch + XCTAssertTrue(waitForExistence(window, timeout: 10)) + XCTAssertTrue(waitForExistence(toolbarButton, timeout: 10)) + + clickMenuBarItem("View") + let menuItem = app.menuItems["Agent Inbox"] + XCTAssertTrue( + waitForExistence(menuItem, timeout: 5), + "View should expose the Agent Inbox command" + ) + menuItem.click() + + XCTAssertTrue( + waitForExistence(inbox, timeout: 10), + "View > Agent Inbox should open the popover in the project window" + ) + // Counting *project* windows, not `app.windows`: nothing in this suite + // establishes how `_NSPopoverWindow` shows up in the AX tree, and the + // claim under test is that no second project window was opened. + XCTAssertEqual( + projectWindows(for: url).count, + 1, + "Routing must reuse the project window instead of opening another" + ) + XCTAssertFalse( + waitForExistence(app.windows["welcome"], timeout: 2), + "An eligible project window must not be bypassed for Welcome" + ) } func testAgentInboxExposesContextualHelp() throws { From 534b24fbe4d3d890b1a2b2ae0ff9518f7437bbd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A4=D0=B5=D0=B4=D0=BE=D1=80=20=D0=91=D0=B0=D1=82=D0=BE?= =?UTF-8?q?=D0=BD=D0=BE=D0=B3=D0=BE=D0=B2?= Date: Fri, 21 Aug 2026 14:37:37 +0300 Subject: [PATCH 2/2] fix(agent): stop a settling close from relatching the Inbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retireClosedPopover()` frees the window synchronously, but the binding write that close implies is deferred one runloop turn. For that turn the anchor holds no popover, no in-flight close, and a binding SwiftUI still reads as presented — so an update pass landing in the gap rebuilt the popover the user had just dismissed. The deferred resolution then saw a popover on screen, returned unchanged, and never lowered the binding. From there the state is self-consistent and permanent: the Inbox is latched open and nothing can take it down until the window closes. This is a regression against the previous shape, where the deferred write was unconditional: the same interleaving produced a blink that healed itself. Re-deriving the verdict cannot distinguish a popover a router request opened in the gap, which must be left alone, from one the gap itself rebuilt, which must be lowered. Track the settling turn explicitly so an undelivered write outranks the snapshot it is on its way to replace. Anything that genuinely reaches the screen ends the settling, which keeps the earlier repairs' invariant that a newer popover is never torn down. Refs #1491 --- Pine/Agent/AgentInboxPopoverCoordinator.swift | 36 ++++- .../AgentInboxPopoverCoordinatorTests.swift | 131 ++++++++++++++++++ 2 files changed, 165 insertions(+), 2 deletions(-) diff --git a/Pine/Agent/AgentInboxPopoverCoordinator.swift b/Pine/Agent/AgentInboxPopoverCoordinator.swift index 56420bb4..39a6d04b 100644 --- a/Pine/Agent/AgentInboxPopoverCoordinator.swift +++ b/Pine/Agent/AgentInboxPopoverCoordinator.swift @@ -81,6 +81,25 @@ final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, /// The last value this anchor wrote into the binding, held only until the /// next update pass makes SwiftUI's own value authoritative again. private var lastWrittenIsPresented: Bool? + /// True from the moment a finished close is retired until the SwiftUI + /// write that close implies has actually been delivered. + /// + /// ``retireClosedPopover()`` frees the window synchronously — it drops the + /// popover and clears the in-flight close, which is what lets the next + /// request through — but the `@State` write cannot be made inside AppKit's + /// own notification and lands a runloop turn later. For that one turn the + /// anchor holds no popover, no close, and a binding SwiftUI still reads as + /// `true`. An update pass landing there resolves to `.present` and rebuilds + /// the popover the user has just dismissed; ``settledClose`` then finds a + /// visible popover, declines to write, and the Inbox is latched open with + /// nothing left able to lower it. Escape and the outside click stop + /// working in that window for good. + /// + /// Ranking the undelivered write above the stale snapshot for that one + /// turn is what closes the gap. It is not the same thing as + /// ``lastWrittenIsPresented``: that one remembers a write SwiftUI has + /// already been given, this one a write still on its way. + private var isCloseSettling = false /// How many may land before the anchor stops waiting for a close /// notification that is not coming. @@ -117,7 +136,11 @@ final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, /// nothing needs writing, and let the next pass reopen the popover the user /// dismissed. Escape would read as opening the Inbox again. private var bindingIsPresented: Bool { - lastWrittenIsPresented ?? (isPresented?.wrappedValue == true) + // A close that has been retired but whose SwiftUI write has not landed + // yet has already decided this; every snapshot until it arrives + // predates it. + if isCloseSettling { return false } + return lastWrittenIsPresented ?? (isPresented?.wrappedValue == true) } init( @@ -180,7 +203,11 @@ final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, updateRegistration(for: anchor.window) let resolution = state.viewDidUpdate( - bindingIsPresented: isPresented.wrappedValue, + // `lastWrittenIsPresented` was just cleared, so this is SwiftUI's + // own snapshot — except across the turn a retired close still owes + // SwiftUI its write, where the snapshot is the value that close is + // on its way to replace. + bindingIsPresented: bindingIsPresented, isPopoverShown: isPopoverShown ) // The AppKit half is safe here: it touches no SwiftUI state, and @@ -315,12 +342,14 @@ final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, popover = nil unreportedCloseCount = 0 state.popoverDidClose() + isCloseSettling = true // `@State` written inside AppKit's own notification re-enters the live // view update, so the SwiftUI half lands a turn later — and is derived // there, because by then a newer request may have opened a new popover // that this verdict would lower the binding on. NativeCommandDelivery.deferToNextMainRunLoop { [weak self] in guard let self else { return } + self.isCloseSettling = false self.apply(self.state.settledClose( bindingIsPresented: self.bindingIsPresented, isPopoverShown: self.isPopoverShown @@ -379,6 +408,9 @@ final class AgentInboxPopoverCoordinator: NSObject, NSPopoverDelegate, )) else { return } popover = handle unreportedCloseCount = 0 + // Something newer is on screen, so the close no longer speaks for this + // anchor: the binding the popover was built against is the live one. + isCloseSettling = false state.popoverWillShow() handle.showPopover(from: anchor) } diff --git a/PineTests/AgentInboxPopoverCoordinatorTests.swift b/PineTests/AgentInboxPopoverCoordinatorTests.swift index ee1e078b..1d57a79f 100644 --- a/PineTests/AgentInboxPopoverCoordinatorTests.swift +++ b/PineTests/AgentInboxPopoverCoordinatorTests.swift @@ -409,6 +409,137 @@ struct AgentInboxPopoverCoordinatorTests { #expect(!fixture.isPresented) } + // MARK: - The turn a retired close owes SwiftUI its write + + /// The regression #1514's UI coverage caught. + /// + /// `popoverDidClose` retires the close synchronously — dropping the + /// popover and clearing `isClosing` is what frees the window for the next + /// request — but the `@State` write it implies cannot be made inside + /// AppKit's notification and lands a runloop turn later. For that one turn + /// the anchor holds no popover, no in-flight close, and a binding SwiftUI + /// still reads as `true`: `viewDidUpdate` answers `.present` and rebuilds + /// the Inbox the user has just dismissed. + /// + /// What makes it worse than a blink is `settledClose`. It re-derives, sees + /// a visible popover, and declines to write — so the binding is never + /// lowered, the rebuilt popover is exactly what every later pass wants, and + /// nothing is left that can take it down. The Inbox is latched open: + /// Escape and the outside click go dead in that window. + /// + /// Replaying it unconditionally instead is not the fix — that is the + /// regression ``settledCloseLeavesANewerPopoverAlone()`` pins. + @Test("an update pass inside a settling close cannot relatch the Inbox") + func updateInsideASettlingCloseCannotRelatchTheInbox() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + // The toolbar button raises the binding; the anchor opens the Inbox. + fixture.isPresented = true + fixture.update() + let popover = try #require(fixture.popovers.last) + + // Escape. AppKit closes the popover behind SwiftUI's back and reports + // both halves of the close before the binding has been touched. + fixture.coordinator.popoverWillClose(sender: popover) + popover.isPopoverVisible = false + fixture.coordinator.popoverDidClose(sender: popover) + + // Anything at all re-renders the window inside that turn — a toolbar + // item redisplayed as the popover gives key focus back is enough. + fixture.update() + #expect( + fixture.popovers.count == 1, + """ + A dismissed Inbox must not be rebuilt by the pass that still reads \ + the binding the close is on its way to lower + """ + ) + + await fixture.nextRunLoopTurn() + #expect( + !fixture.isPresented, + "The close still owes SwiftUI its write once the turn is over" + ) + + // And it stays down: the next pass has nothing left to present. + fixture.update() + await fixture.nextRunLoopTurn() + #expect(fixture.popovers.count == 1) + #expect(!fixture.isPresented) + } + + /// The guard above must not swallow a real request that lands in the same + /// turn. ⇧⌘I, the View menu and the Dock all arrive through the router, + /// and a turn is easily long enough to hold one. + @Test("a router request inside a settling close still opens the Inbox") + func routerRequestInsideASettlingCloseIsServed() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + fixture.isPresented = true + fixture.update() + let first = try #require(fixture.popovers.last) + + fixture.coordinator.popoverWillClose(sender: first) + first.isPopoverVisible = false + fixture.coordinator.popoverDidClose(sender: first) + + fixture.coordinator.presentAgentInbox() + let second = try #require(fixture.popovers.last) + #expect(fixture.popovers.count == 2) + #expect(fixture.isPresented) + + // A pass in the same turn must leave the newer popover alone: the + // close stopped speaking for this anchor the moment it was replaced. + fixture.update() + #expect( + second.closeCount == 0, + "The settling close must not close the Inbox that replaced it" + ) + + await fixture.nextRunLoopTurn() + #expect(fixture.isPresented) + #expect(second.isPopoverVisible) + #expect(fixture.popovers.count == 2) + } + + /// The exact end-to-end shape #1514's UI test walks: open, dismiss by + /// clicking outside, reopen from the toolbar, dismiss with Escape. The + /// second dismissal is the one that latched. + @Test("two dismissal cycles leave the window with no Inbox and no binding") + func twoDismissalCyclesConverge() async throws { + let fixture = try Fixture() + defer { fixture.cleanup() } + fixture.mount() + + for cycle in 0..<2 { + fixture.isPresented = true + fixture.update() + let popover = try #require(fixture.popovers.last) + #expect(fixture.popovers.count == cycle + 1) + + fixture.coordinator.popoverWillClose(sender: popover) + popover.isPopoverVisible = false + fixture.coordinator.popoverDidClose(sender: popover) + // The window re-renders while the close still owes its write. + fixture.update() + await fixture.nextRunLoopTurn() + + #expect( + !fixture.isPresented, + "Cycle \(cycle) left the binding raised" + ) + #expect( + fixture.popovers.count == cycle + 1, + "Cycle \(cycle) rebuilt the popover it had just dismissed" + ) + fixture.update() + } + } + // MARK: - Fixture @MainActor