diff --git a/Kanban/App/WindowAccessor.swift b/Kanban/App/WindowAccessor.swift index 07736c4..a360eee 100644 --- a/Kanban/App/WindowAccessor.swift +++ b/Kanban/App/WindowAccessor.swift @@ -127,11 +127,38 @@ final class HostedWindowController: NSObject, NSWindowDelegate { /// directions. private var extendsUnderTitlebar: Bool? + /// **Which `WindowAccessor` view this controller is currently attached through** — the identity + /// `detach(through:)` checks before it tears anything down. + /// + /// SwiftUI's dismantle-then-make pair for the background representable is not guaranteed to + /// arrive in the order it logically pairs in (`detach()`'s own doc comment: "observed on macOS + /// 26" is an empirical note, not a documented ordering). A content swap deep in a window's tree — + /// the raw-source outlet replacing the whole card-window content area is the one this was traced + /// to — can make SwiftUI recreate this representable, and if the *old* instance's `dismantleNSView` + /// is delivered **after** the *new* instance has already attached, an identity-blind `detach()` + /// would tear down the toolbar, the titlebar accessory and the delegate proxy that the newer + /// attach just installed — leaving the window with no toolbar at all, which collapses a two-line + /// title-and-subtitle down to AppKit's single combined line (the malformed titlebar this guards + /// against). Tracking *which* view is the current owner is what lets a stale teardown recognize + /// itself as stale and refuse, rather than winning a race it does not know it is in. + /// + /// `nil` for every caller that attaches without naming a view — every direct call in this file's + /// own tests — which keeps their `attach`/`detach` pair exactly as unconditional as it always was; + /// the guard only ever engages when both sides of a call name one. + private weak var attachedThroughView: NSView? + private static let logger = Logger(subsystem: "dev.rzen.indie.Kanban", category: "window") // MARK: Attachment - func attach(to window: NSWindow) { + /// - Parameter view: The `WindowAccessor`'s own sensing view, when the caller is one — `nil` for + /// a caller with no view of its own (every direct call this file's tests make), which attaches + /// exactly as before. Recorded **before** the same-window early return, so a fresh view mounted + /// over an unchanged window still updates who owns the next `detach(through:)`. + func attach(to window: NSWindow, through view: NSView? = nil) { + if let view { + attachedThroughView = view + } guard self.window !== window else { return } self.window = window if window.delegate !== self { @@ -161,7 +188,15 @@ final class HostedWindowController: NSObject, NSWindowDelegate { /// window, a dismantle follows, and only *then* does the real window attach) — so chrome /// discarded here would never reach the window it was made for. `attach` reinstalls whatever is /// held; a controller that is genuinely done takes its slots down with it. - func detach() { + /// + /// - Parameter view: The `WindowAccessor` view being dismantled, when the caller is one — `nil` + /// for a caller with no view (every direct call this file's tests make), which detaches exactly + /// as before, unconditionally. A named view that is **not** the one `attachedThroughView` last + /// recorded is refused outright: a fresher `attach(to:through:)` already owns this window's + /// chrome, and this call is a stale teardown arriving for a view that lost the race — see + /// `attachedThroughView`. + func detach(through view: NSView? = nil) { + if let view, view !== attachedThroughView { return } removeTitlebarAccessory() removeToolbar() guard let window, window.delegate === self else { return } @@ -429,8 +464,12 @@ struct WindowAccessor: NSViewRepresentable { func makeNSView(context: Context) -> NSView { let view = WindowSensingView() - view.onWindow = { [controller] window in - controller.attach(to: window) + // **Named**, not bare — this instance is the token `attach(to:through:)`/`detach(through:)` + // race-guard on (`HostedWindowController.attachedThroughView`), so a stale dismantle for a + // view a later `makeNSView` has already superseded refuses to undo that newer attach's work. + view.onWindow = { [controller, weak view] window in + guard let view else { return } + controller.attach(to: window, through: view) } return view } @@ -438,7 +477,7 @@ struct WindowAccessor: NSViewRepresentable { func updateNSView(_ nsView: NSView, context: Context) {} static func dismantleNSView(_ nsView: NSView, coordinator: HostedWindowController) { - coordinator.detach() + coordinator.detach(through: nsView) } } diff --git a/KanbanTests/ToolbarTests.swift b/KanbanTests/ToolbarTests.swift index 7339274..fd66cc1 100644 --- a/KanbanTests/ToolbarTests.swift +++ b/KanbanTests/ToolbarTests.swift @@ -828,6 +828,129 @@ struct CardToolbarTests { } } +// MARK: - The toolbar survives a stale dismantle + +/// **The malformed-titlebar bug**: toggling a card window between Edit and Raw Source could leave it +/// with no toolbar at all — and losing the toolbar is what collapses AppKit's two-line title-and- +/// subtitle down to its single combined line, the "⟨title⟩ — ⟨board⟩ › ⟨lane⟩" strip a user reported +/// seeing after exactly that toggle. +/// +/// The root cause lived in `HostedWindowController.attach`/`detach`, not in the card window's own +/// chrome code: `WindowAccessor`'s own doc comment already recorded that SwiftUI "dismantles and +/// re-makes the background representable" on macOS 26, and a content swap deep in the window's tree +/// (raw source replacing the whole content area is the one this traces to) can trigger exactly that — +/// but the dismantle for the *old* representable and the attach for the *new* one were never +/// guaranteed to arrive in the order they logically pair in. When the stale dismantle landed **after** +/// the fresh attach, an identity-blind `detach()` tore the toolbar (and the titlebar accessory, and the +/// delegate proxy) right back off a window a newer attach had just finished configuring — silently, +/// with nothing to put it back until something else attached again. +/// +/// `attach(to:through:)`/`detach(through:)` fix this by tracking *which* `WindowAccessor` view is the +/// current owner and refusing a detach for any other — see `HostedWindowController.attachedThroughView`. +/// These tests drive that seam directly, standing in for the two views SwiftUI would otherwise recreate. +@MainActor +@Suite("Toolbar ▸ survives a stale dismantle") +struct ToolbarStaleDismantleTests { + + private func makeWindow() -> NSWindow { + NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 600, height: 400), + styleMask: [.titled, .closable, .miniaturizable, .resizable], + backing: .buffered, + defer: true + ) + } + + /// A real, installable card toolbar — the exact shape `CardWindowHost.configureWindow` hands + /// `installToolbar`, built fresh rather than borrowed from `CardToolbarTests` (`private` there, + /// on purpose: a struct's own fixtures are not a second suite's to reach into). + private func makeToolbar() -> WindowToolbarController { + CardToolbar.controller( + body: CardBodyPresentation(), + rawSource: CardRawSourceSession(), + attachments: CardAttachments(), + actions: CardWindowActions() + ) + } + + @Test("A dismantle for a view a newer attach has already superseded does not remove the toolbar") + func aStaleDismantleIsRefused() { + let window = makeWindow() + let controller = HostedWindowController() + let toolbarController = makeToolbar() + controller.installToolbar(toolbarController) + + let firstView = NSView() + let secondView = NSView() + + // The normal-order half: the first mount installs the toolbar. + controller.attach(to: window, through: firstView) + #expect(window.toolbar === toolbarController.toolbar) + + // A second `WindowAccessor` instance mounts over the *same* window — the racy content-swap + // case, standing in for SwiftUI recreating the representable without the window itself + // changing. It becomes the new owner. + controller.attach(to: window, through: secondView) + #expect(window.toolbar === toolbarController.toolbar, "the still-current window keeps its toolbar") + + // The first view's teardown arrives *after* the second view's attach — the ordering + // `WindowAccessor`'s own doc comment says is not guaranteed. A stale detach for a superseded + // view must not undo what the fresher attach just did. + controller.detach(through: firstView) + #expect(window.toolbar === toolbarController.toolbar, "a stale dismantle must not remove the toolbar") + #expect(window.delegate === controller, "nor hand the delegate back to SwiftUI's own") + } + + @Test("The legitimate owner's dismantle still tears the toolbar down") + func theCurrentOwnersDismantleStillWorks() { + let window = makeWindow() + let controller = HostedWindowController() + let toolbarController = makeToolbar() + controller.installToolbar(toolbarController) + + let view = NSView() + controller.attach(to: window, through: view) + #expect(window.toolbar === toolbarController.toolbar) + + controller.detach(through: view) + #expect(window.toolbar == nil, "the view that actually owns the attachment can still tear it down") + } + + @Test("The normal order — detach, then attach — reinstalls the toolbar exactly as before") + func theOrdinaryOrderStillSelfHeals() { + let window = makeWindow() + let controller = HostedWindowController() + let toolbarController = makeToolbar() + controller.installToolbar(toolbarController) + + let firstView = NSView() + controller.attach(to: window, through: firstView) + controller.detach(through: firstView) + #expect(window.toolbar == nil) + + let secondView = NSView() + controller.attach(to: window, through: secondView) + #expect(window.toolbar === toolbarController.toolbar, "the next attach reinstalls it") + } + + @Test("A caller that names no view keeps the original, unconditional attach/detach") + func viewlessCallsAreUnaffected() { + // `BoardChromeTests` and every other direct caller in this test target attaches and detaches + // without a view — the shape `attach(to:)`/`detach()` always had. That pair must keep working + // exactly as before: the view-identity guard only ever engages when both sides name one. + let window = makeWindow() + let controller = HostedWindowController() + let toolbarController = makeToolbar() + controller.installToolbar(toolbarController) + + controller.attach(to: window) + #expect(window.toolbar === toolbarController.toolbar) + + controller.detach() + #expect(window.toolbar == nil) + } +} + // MARK: - ⌘F and the search field's two homes /// "⌘F always summons search: with the field removed from the toolbar, invoking it surfaces the