diff --git a/Sources/UI/Settings/HomeView.swift b/Sources/UI/Settings/HomeView.swift index ef4cdaa4..06923494 100644 --- a/Sources/UI/Settings/HomeView.swift +++ b/Sources/UI/Settings/HomeView.swift @@ -837,13 +837,20 @@ struct HomeRowMoreMenuButton: NSViewRepresentable { /// A menu item that owns its action closure and acts as its own target. /// - /// The menu retains its items for the whole `popUp` tracking loop, so the - /// handler stays alive and fires no matter how SwiftUI tears down and - /// recreates this representable's coordinator while the menu is open. The - /// earlier design stored a separate target on the (weakly referenced) - /// `NSMenuItem.target` and deferred the call with `DispatchQueue.main.async`; - /// the target could be deallocated before the deferred block ran, so the - /// closures silently never fired (delete, reveal, and report all no-op'd). + /// Two things have to hold for a handler to both fire *and* be able to drive + /// SwiftUI presentation: + /// 1. The item owns its handler instead of pointing `NSMenuItem.target` at + /// a separately, weakly-retained object. The `NSMenu` retains its items + /// for the whole `popUp` tracking loop, so the handler can't be torn + /// down with the SwiftUI coordinator while the menu is open. (The old + /// design's weak target could deallocate first, so closures silently + /// never fired — delete, reveal, and report all no-op'd.) + /// 2. The handler runs on the next main-runloop turn, *after* `popUp`'s + /// modal tracking loop exits. A handler that mutates SwiftUI state to + /// present an `.alert(item:)`/`.sheet(item:)` (e.g. the Home delete + /// confirmation) won't present if it runs synchronously inside that + /// loop. The async block captures `handler` strongly, so deferring is + /// safe here — the dealloc trap from (1) does not reappear. final class ClosureMenuItem: NSMenuItem { private let handler: () -> Void @@ -861,7 +868,7 @@ struct HomeRowMoreMenuButton: NSViewRepresentable { @objc private func invoke() { guard isEnabled else { return } - handler() + DispatchQueue.main.async { [handler] in handler() } } } diff --git a/Sources/UI/Settings/TranscriptedSettingsView.swift b/Sources/UI/Settings/TranscriptedSettingsView.swift index c291a4b5..31dec2fd 100644 --- a/Sources/UI/Settings/TranscriptedSettingsView.swift +++ b/Sources/UI/Settings/TranscriptedSettingsView.swift @@ -216,32 +216,38 @@ struct TranscriptedSettingsView: View { } ) } - .alert(item: $homeDeleteConfirmation) { confirmation in - Alert( - title: Text(confirmation.title), - message: Text(confirmation.message), - primaryButton: .destructive(Text(confirmation.confirmTitle)) { - confirmation.perform() - }, - secondaryButton: .cancel() - ) - } - .alert(item: $homeDeleteFailure) { failure in - Alert( - title: Text(failure.title), - message: Text(failure.message), - dismissButton: .default(Text("OK")) - ) - } - .alert(item: $pendingAudioRetentionWindow) { window in - Alert( - title: Text("Delete old replay audio?"), - message: Text("Transcripted will keep your Markdown transcripts, but retained replay audio older than \(window.title) will be permanently removed now and cleaned up automatically later."), - primaryButton: .destructive(Text("Delete Old Audio")) { - applyAudioRetentionWindow(window) - }, - secondaryButton: .cancel() - ) + // One alert modifier, not three. Stacking multiple legacy `.alert(item:)` + // on the same view shadows all but the last — which silently killed the + // meeting-delete confirmation (first of three). `rootAlertBinding` routes + // the three independent states through a single presenter so the active + // one always shows; every existing call site stays unchanged. + .alert(item: rootAlertBinding) { alert in + switch alert { + case .deleteConfirmation(let confirmation): + return Alert( + title: Text(confirmation.title), + message: Text(confirmation.message), + primaryButton: .destructive(Text(confirmation.confirmTitle)) { + confirmation.perform() + }, + secondaryButton: .cancel() + ) + case .deleteFailure(let failure): + return Alert( + title: Text(failure.title), + message: Text(failure.message), + dismissButton: .default(Text("OK")) + ) + case .audioRetention(let window): + return Alert( + title: Text("Delete old replay audio?"), + message: Text("Transcripted will keep your Markdown transcripts, but retained replay audio older than \(window.title) will be permanently removed now and cleaned up automatically later."), + primaryButton: .destructive(Text("Delete Old Audio")) { + applyAudioRetentionWindow(window) + }, + secondaryButton: .cancel() + ) + } } .task(id: navigation.presentationID) { refreshState() @@ -1186,6 +1192,51 @@ struct TranscriptedSettingsView: View { return items } + // MARK: - Home root alert + + /// The Home surface drives three independent confirmation/failure alerts + /// (`homeDeleteConfirmation`, `homeDeleteFailure`, `pendingAudioRetentionWindow`). + /// They are presented through a *single* `.alert(item:)` because SwiftUI + /// shadows all but the last when several legacy `.alert(item:)` are stacked + /// on one view — that is what silently broke the meeting-delete confirmation. + private enum RootAlert: Identifiable { + case deleteConfirmation(HomeDeleteConfirmation) + case deleteFailure(HomeDeleteFailure) + case audioRetention(AudioRetentionWindow) + + var id: String { + switch self { + case .deleteConfirmation(let confirmation): return "delete-confirmation-\(confirmation.id)" + case .deleteFailure(let failure): return "delete-failure-\(failure.id)" + case .audioRetention(let window): return "audio-retention-\(window.id)" + } + } + } + + /// Whichever Home alert state is currently set, in presentation priority order. + /// At most one is non-nil at a time in practice. + private var activeRootAlert: RootAlert? { + if let confirmation = homeDeleteConfirmation { return .deleteConfirmation(confirmation) } + if let failure = homeDeleteFailure { return .deleteFailure(failure) } + if let window = pendingAudioRetentionWindow { return .audioRetention(window) } + return nil + } + + /// Binds the single alert presenter to the three underlying states. Dismissal + /// clears all of them (only one is ever set), so call sites keep setting their + /// own `@State` directly. + private var rootAlertBinding: Binding { + Binding( + get: { activeRootAlert }, + set: { newValue in + guard newValue == nil else { return } + homeDeleteConfirmation = nil + homeDeleteFailure = nil + pendingAudioRetentionWindow = nil + } + ) + } + private func deleteMeeting(_ item: RecentMeetingItem) { if homeMeetingPreview?.transcriptURL == item.transcriptURL { homeMeetingPreview = nil diff --git a/Tests/UIAutomationSurfaceContractTests.swift b/Tests/UIAutomationSurfaceContractTests.swift index c65650e0..4df81d2f 100644 --- a/Tests/UIAutomationSurfaceContractTests.swift +++ b/Tests/UIAutomationSurfaceContractTests.swift @@ -136,6 +136,34 @@ func testUIAutomationSurfaceContract() { "Home row menus should not depend on unstable SwiftUI-generated menu item IDs" ) + // Regression guards for fix/home-delete-confirmation-menu-loop. The Home + // recent-meeting "Delete meeting" confirmation silently no-op'd because of + // two stacked defects the fast suite cannot exercise at runtime: + // 1. Row-menu handlers fired synchronously inside NSMenu.popUp's modal + // tracking loop, so the SwiftUI alert they set never presented. + // ClosureMenuItem must hop to the next main-runloop turn. + // 2. Three legacy `.alert(item:)` modifiers were stacked on the settings + // root view; SwiftUI keeps only the last, shadowing the (first) + // delete confirmation. The three states must share one presenter. + assertTrue( + homeSource.contains("DispatchQueue.main.async { [handler] in handler() }"), + "ClosureMenuItem should defer its handler off the NSMenu.popUp tracking loop so menu-triggered SwiftUI alerts/sheets present" + ) + assertTrue( + settingsSource.contains(".alert(item: rootAlertBinding)") + && settingsSource.contains("enum RootAlert") + && settingsSource.contains("case deleteConfirmation") + && settingsSource.contains("case deleteFailure") + && settingsSource.contains("case audioRetention"), + "the Home delete, delete-failure, and audio-retention alerts should present through one rootAlertBinding so none is shadowed" + ) + assertFalse( + settingsSource.contains(".alert(item: $homeDeleteConfirmation)") + || settingsSource.contains(".alert(item: $homeDeleteFailure)") + || settingsSource.contains(".alert(item: $pendingAudioRetentionWindow)"), + "Home alerts must not be re-stacked as separate `.alert(item:)` modifiers — stacked legacy alerts shadow all but the last" + ) + // Row-interaction affordances from fix/home-row-actions, which have no // behavioral coverage in the fast suite (it greps source, never runs the // UI). The overflow actions only reveal on hover, so the row needs a