test(agent): cover Agent Inbox popover presentation and lifecycle - #1514
Merged
Conversation
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
Contributor
✅ Code Coverage: 79.4399%Threshold: 70% Logic-only lines: 80788/101697 Coverage is at or above the required threshold. Generated by CI — see job summary for detailed file-level breakdown. |
`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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of 2 for #1491, on top of #1513. Does not close it — criteria 6 and 13 remain unmet, see below.
Three defects fixed, each found by review and each proven by mutation
A deferred binding write was cancelled by its own synchronous effect.
viewDidUpdatereturns a binding lift in exactly one case, and in that same case the synchronous.presentruns first and consumes the unserved-request marker that was the lift's premise. The deferred block then re-derived against a state whose precondition it had just destroyed and wrote nothing. User-visible: the Inbox opened withisPresentedfalse, and the next render pass closed it — it blinked and vanished, and the keystroke read as swallowed. The deferred block now asks the popover that is actually on screen before falling back to the rule. The existing test passed only because it used an unmounted anchor, i.e. the branch where the present fails anyway; the new one uses a mounted anchor and walks the reported path.A request queued for a host whose anchor never mounts waited forever. The anchor lives in a
ToolbarItemand unregisters whenever AppKit pulls the toolbar view out of the window — toolbar hidden, toolbar overflow on a narrow window, and (untested, stated as likely) full screen, where the toolbar moves toNSToolbarFullScreenWindow, which carries noCloseDelegateand can never be a candidate. Two visible consequences: ⇧⌘I, the View menu and the Dock did nothing at all with no fallback; and the request stayed armed on the shared router and was delivered whenever that anchor next mounted — the Inbox opening by itself, minutes later. Now bounded: an existing host gets ~1 s of anchor polling (matching the existingawaitVisibleWelcomeWindowbudget), then the request is retired before falling back to Welcome, so nothing can be delivered to the anchorless window in the meantime. Only the create-Welcome path still waits without end, because it is the only path that knows its anchor is coming.anchorWindowDidChangepresented synchronously fromviewDidMoveToWindow, before layout had given the anchor a non-zerobounds— the exact hazard the router's own hand-off defers for. Only the.presenteffect is deferred;.closestays synchronous, because holding it would leave a popover on a window the anchor has already left.The fixture now reproduces
@StateThe old fixture substituted a
Bindingover a plain storedBool, which reads back the written value immediately. Production reads a binding captured at the lastupdateNSView, and every deferred decision reads it between update passes. The fixture now snapshots at eachupdate(), and the coordinator prefers its own last written value until the next pass clears it.This earned itself immediately: it exposed two of this branch's own new tests as staging a state SwiftUI would never hand the anchor. They were rewritten to go through a real update pass.
What is not covered, stated plainly
Criterion 6 — not met. A minimized project window is silently bypassed in favour of Welcome, because
isVisibleis false in the Dock and that is a term in the eligibility conjunction. Only the protocol contract is pinned, and the test says in its own doc comment that this is not evidence of user-visible behaviour. Filed as #1507.Criterion 13 — not covered, and there is no production code for it.
keyWindowSession()is sticky and falls back towindowSessionRefs.last, so with one project window the View menu reaches it regardless of focus. Focus restoration isNSPopover's own behaviour; that reliance is now written down rather than asserted.One mutation survives and is reported rather than hidden:
NSWindow.focusHost()'smakeKeyAndOrderFront(nil)→orderFront(nil)raises the host without focusing it, so the Inbox opens over a window the user is not typing in. It cannot be caught in a background test host — the same limitation #1513 documented for the key-window source. Stated in the test's doc comment.Note for review: this PR edits a test from #1513
AgentInboxHostOptionsTests:454comparedoptions.map(\.host)by value.AgentInboxHostOption.hostbecomes existential in this part (the existential is what makes restore-then-focus observable without a window server), and an existential is notEquatable, so the comparison stops compiling. It is rewritten as anObjectIdentifiercomparison — same order, same count, same identity claim, only the comparison changed. Flagging it because an unexplained edit to another PR's test deserves suspicion.AgentInboxHostOptionitself existed in both places after the part 1 merge (NSWindowinPineApp.swift, existential in the coordinator). Main's copy was removed and its doc wording carried over.Liveness
Both new coordinators are load-bearing, verified by construction rather than argument: renaming either makes the app target fail to build.
Verification
138 tests across the 8 Agent Inbox suites green, three consecutive runs. Ten mutations applied one at a time, each reverted and confirmed byte-identical by sha256; nine were killed by a named test, the tenth is the
focusHostcase above.git diff --checkclean;swiftlint --strict0 violations across 793 files;check-no-post-under-inout.pyandcheck_nonisolated.pyclean; shard validatordelta=2(the rebalance is still required — without it the spread is 4).xcodebuild buildandbuild-for-testingboth succeeded with no new warnings. UI tests were not run locally.A pre-existing flake was investigated rather than assumed. Running these suites alongside twelve adjacent ones produced 1–2 failures in
AgentInboxTests, which is byte-identical to main and passes standalone. A throwaway worktree atdeaa1c4erunning the identical batch fails 5 out of 5 on the same two tests at the same line; this branch fails 4 of 5. Load-sensitive wait on a blocking canonicalizer, pre-existing.Environment: macOS 27.0 (26A5416b), Xcode 27.0 (27A5237l), SDK 27.0 (26A5406c).
Known risks handed over, not hidden
An existing host whose anchor takes longer than ~1 s to mount now gets a Welcome window instead of waiting. Better than the two current outcomes, and it matches an existing budget — but it is a real behavioural change and nobody has measured how long a deminiaturized window actually takes to re-mount a toolbar anchor.
anchorWindowDidChangealso costs one extra runloop turn before the popover appears; correct for layout, unasserted as latency. The unreported-close guard is a count, not a clock.