PR review: #235 — Fix T-1898: Cancel macOS Add Task save

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 1a53f93..8c4c943 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased]  - T-1820: MCP JSON-RPC notification regressions now prove that valid mutating `tools/call` notifications execute their normal side effects while suppressing only their response. Coverage spans direct handler dispatch, successful and failing single HTTP notifications that return 202/no body, ordered mixed batches where a following valid request observes the mutation while notification failures remain response-suppressed, and a batched `initialize` notification that cannot block the following valid request. The MCP route, batch, and notification suites now share one in-process HTTP transport harness. Explicit-null invalid requests, standalone lifecycle rules, all-notification HTTP semantics, and valid request responses remain covered.+- T-1898: macOS New Task now retains its in-flight save task and cancels it only when the window/view disappears before persistence completes. The shared T-1858 create-save lifecycle records success before dismissal, so successful creation is not cancelled by its own disappearance; `CancellationError` returns silently while real failures retain the form for retry. iOS dismissal blocking, T-2037 milestone validation, and `TaskService` cancellation safeguards remain intact. A reopened singleton window visibly disables Save while a prior cancellation settles, rather than accepting a Save action that cannot start. Deterministic gated-allocation tests prove a dismissed create inserts no task, alongside successful-dismissal, cancellation-pending, and retry coverage.  - T-1816: Regression coverage now directly proves that `query_tasks` validates malformed `status`, `not_status`, `type`, `priority`, `unfinished`, `search`, and `displayId` filters before either a missing milestone name or missing milestone display ID can return a successful no-match `[]`. This pins the validation order introduced by T-1608 / PR #217 without changing its production behavior. - T-1749: Visual Add Task no longer reports `NO_PROJECTS` when it cannot read the project table while resolving an unselected project. The existence check now propagates its SwiftData error through the existing injectable fetch seam, and the visual intent returns `INTERNAL_ERROR` with the storage-failure guidance. Genuine empty stores still return `NO_PROJECTS`; stale selected projects remain on the separate `PROJECT_NOT_FOUND` / storage-failure path.diff --git a/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift b/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swiftindex 32bbead..3658f06 100644--- a/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift+++ b/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift@@ -1,11 +1,15 @@ import SwiftData import SwiftUI +/// Ties the shared creation lifecycle to Add Task terminology at its call sites+/// and in the regression suite.+typealias AddTaskSaveLifecycle = CreateSaveLifecycle+ // MARK: - Actions  extension AddTaskSheet { -    func save() async {+    func save() {         guard let project = selectedProject else { return }         let trimmedName = name.trimmedForFormInput()         guard !trimmedName.isEmpty else { return }@@ -19,19 +23,45 @@ extension AddTaskSheet {             projectID: project.id,             milestone: selectedMilestone         )+        guard saveLifecycle.beginSave() else { return }++        let task = Task { @MainActor in+            do {+                try await Self.persist(draft: draft, taskService: taskService) -        isSaving = true-        defer { isSaving = false }--        do {-            try await Self.persist(-                draft: draft,-                taskService: taskService-            )-            dismiss()-        } catch {-            errorMessage = error.localizedDescription+                // TaskService checks cancellation immediately before insertion.+                // Both this continuation and `onDisappear` run on MainActor with+                // no suspension here, so a returning persist is committed success.+                // Record it before dismissal so disappearance cannot cancel it.+                guard saveLifecycle.completeSave() else {+                    assertionFailure("Persisted Add Task must still own its save lifecycle")+                    return+                }+                saveTask = nil+                dismiss()+            } catch is CancellationError {+                finishCancelledSave()+            } catch {+                if Task.isCancelled {+                    finishCancelledSave()+                } else {+                    saveLifecycle.completeFailure()+                    saveTask = nil+                    errorMessage = error.localizedDescription+                }+            }         }+        saveTask = task+    }++    func cancelSaveForDisappearance() {+        guard saveLifecycle.cancelForDisappearance() else { return }+        saveTask?.cancel()+    }++    func finishCancelledSave() {+        saveLifecycle.completeCancellation()+        saveTask = nil     }      /// Fields collected by the New Task form, ready to be persisted.diff --git a/Transit/Transit/Views/AddTask/AddTaskSheet.swift b/Transit/Transit/Views/AddTask/AddTaskSheet.swiftindex adecae5..668d3ea 100644--- a/Transit/Transit/Views/AddTask/AddTaskSheet.swift+++ b/Transit/Transit/Views/AddTask/AddTaskSheet.swift@@ -21,9 +21,24 @@ struct AddTaskSheet: View {     @State private var selectedProjectID: UUID?     @State var selectedMilestone: Milestone?     @State private var selectedDetent: PresentationDetent = .large-    @State var isSaving = false+    @State var saveLifecycle = AddTaskSaveLifecycle()+    @State var saveTask: Task<Void, Never>?     @State var errorMessage: String? +    var isSaving: Bool {+        saveLifecycle.blocksDismissal+    }++    /// This remains disabled while a prior cancellation unwinds so a newly+    /// presented form cannot accept a Save action that cannot start yet.+    var isSaveActionDisabled: Bool {+        saveLifecycle.blocksSaveAction+    }++    var saveButtonTitle: String {+        saveLifecycle.isCancellationPending ? "Cancelling…" : "Save"+    }+     var selectedProject: Project? {         guard let id = selectedProjectID else { return nil }         return projects.first { $0.id == id }@@ -68,15 +83,15 @@ struct AddTaskSheet: View {                 #endif                 ToolbarItem(placement: .confirmationAction) {                     #if os(macOS)-                    Button("Save") {-                        Task { await save() }+                    Button(saveButtonTitle) {+                        save()                     }-                    .disabled(!canSave || isSaving)+                    .disabled(!canSave || isSaveActionDisabled)                     #else-                    Button("Save", systemImage: "checkmark") {-                        Task { await save() }+                    Button(saveButtonTitle, systemImage: "checkmark") {+                        save()                     }-                    .disabled(!canSave || isSaving)+                    .disabled(!canSave || isSaveActionDisabled)                     #endif                 }             }@@ -113,6 +128,7 @@ struct AddTaskSheet: View {                 selectedMilestone = nil             }         }+        .onDisappear { cancelSaveForDisappearance() }     }      // MARK: - iOS Layout@@ -273,7 +289,9 @@ struct AddTaskSheet: View {             from: projects, current: selectedProjectID         )         errorMessage = nil-        isSaving = false+        // Reset form fields for every presentation. A pending cancellation still+        // owns its task handle, so this leaves Save disabled until it settles.+        saveLifecycle.resetAfterSuccessfulDismissal()     }     #endif diff --git a/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift b/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swiftindex b615237..9c6ae96 100644--- a/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift+++ b/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift@@ -1,65 +1,3 @@-/// Models the create-mode save lifecycle for `MilestoneEditView`.-///-/// A successful save transitions before `dismiss()` so `onDisappear` cannot-/// cancel an operation that has already persisted. A pending create instead-/// transitions to cancellation, allowing the service's persistence-boundary-/// cancellation checks to prevent a ghost milestone.-nonisolated struct MilestoneCreateSaveLifecycle: Equatable {-    private enum State: Equatable {-        case idle-        case saving-        case cancellationPending-        case savedAwaitingDismissal-    }--    private var state: State = .idle--    /// Keeps navigation and interactive dismissal unavailable until either a-    /// save error/cancellation returns the editor to idle or the save dismisses it.-    var blocksDismissal: Bool {-        switch state {-        case .saving, .savedAwaitingDismissal:-            true-        case .idle, .cancellationPending:-            false-        }-    }--    var isCancellationPending: Bool {-        state == .cancellationPending-    }--    /// Starts one create operation. Repeated save actions are ignored while the-    /// current operation owns the lifecycle.-    mutating func beginSave() -> Bool {-        guard state == .idle else { return false }-        state = .saving-        return true-    }--    /// Marks a still-pending create for cancellation when its view disappears.-    /// A completed save intentionally returns false so its own `dismiss()` does-    /// not cancel the operation after persistence has succeeded.-    mutating func cancelForDisappearance() -> Bool {-        guard state == .saving else { return false }-        state = .cancellationPending-        return true-    }--    /// Records persistence success before the view is dismissed.-    mutating func completeSave() -> Bool {-        guard state == .saving else { return false }-        state = .savedAwaitingDismissal-        return true-    }--    mutating func completeFailure() {-        guard state == .saving else { return }-        state = .idle-    }--    mutating func completeCancellation() {-        guard state == .saving || state == .cancellationPending else { return }-        state = .idle-    }-}+/// Backwards-compatible name for the create-save lifecycle introduced by T-1858.+/// New creation views should use `CreateSaveLifecycle` directly.+typealias MilestoneCreateSaveLifecycle = CreateSaveLifecyclediff --git a/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift b/Transit/Transit/Views/Shared/CreateSaveLifecycle.swiftsimilarity index 62%copy from Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swiftcopy to Transit/Transit/Views/Shared/CreateSaveLifecycle.swiftindex b615237..b1628c9 100644--- a/Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift+++ b/Transit/Transit/Views/Shared/CreateSaveLifecycle.swift@@ -1,10 +1,12 @@-/// Models the create-mode save lifecycle for `MilestoneEditView`.+/// Models an asynchronous creation save from start through either cancellation,+/// failure, or a successful dismissal. ///-/// A successful save transitions before `dismiss()` so `onDisappear` cannot-/// cancel an operation that has already persisted. A pending create instead-/// transitions to cancellation, allowing the service's persistence-boundary-/// cancellation checks to prevent a ghost milestone.-nonisolated struct MilestoneCreateSaveLifecycle: Equatable {+/// A successful save transitions before `dismiss()` so its resulting+/// `onDisappear` cannot cancel data that has already persisted. A view that+/// disappears during a pending create instead transitions to cancellation,+/// allowing the service's persistence-boundary cancellation checks to prevent+/// a ghost record.+nonisolated struct CreateSaveLifecycle: Equatable {     private enum State: Equatable {         case idle         case saving@@ -29,6 +31,12 @@ nonisolated struct MilestoneCreateSaveLifecycle: Equatable {         state == .cancellationPending     } +    /// Prevents a reused creation view from accepting a replacement save until+    /// its prior create task has either completed or finished cancellation.+    var blocksSaveAction: Bool {+        state != .idle+    }+     /// Starts one create operation. Repeated save actions are ignored while the     /// current operation owns the lifecycle.     mutating func beginSave() -> Bool {@@ -53,6 +61,14 @@ nonisolated struct MilestoneCreateSaveLifecycle: Equatable {         return true     } +    /// Prepares a reusable view for its next presentation after its previous+    /// successful save dismissed it. A cancellation still owns its task handle+    /// until it settles, so this deliberately leaves other states unchanged.+    mutating func resetAfterSuccessfulDismissal() {+        guard state == .savedAwaitingDismissal else { return }+        state = .idle+    }+     mutating func completeFailure() {         guard state == .saving else { return }         state = .idlediff --git a/Transit/TransitTests/AddTaskSaveLifecycleTests.swift b/Transit/TransitTests/AddTaskSaveLifecycleTests.swiftnew file mode 100644index 0000000..34fbaed--- /dev/null+++ b/Transit/TransitTests/AddTaskSaveLifecycleTests.swift@@ -0,0 +1,125 @@+import Foundation+import SwiftData+import Testing+@testable import Transit++/// Regression tests for T-1898. Add Task must retain its create operation,+/// cancel it when its macOS window disappears, and distinguish that cancellation+/// from a successful save-driven dismissal.+@MainActor @Suite(.serialized)+struct AddTaskSaveLifecycleTests {++    private func makeProject(in context: ModelContext) -> Project {+        let project = Project(+            name: "Lifecycle Project",+            description: "Test project",+            gitRepo: nil,+            colorHex: "#FF0000"+        )+        context.insert(project)+        return project+    }++    private func makeDraft(project: Project) -> AddTaskSheet.TaskDraft {+        AddTaskSheet.TaskDraft(+            name: "Lifecycle Task",+            description: nil,+            type: .feature,+            priority: .medium,+            projectID: project.id,+            milestone: nil+        )+    }++    @Test func disappearanceDuringGatedAllocationCancelsCreateBeforeTaskInsertion() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let counterStore = AllocationGatedCounterStore()+        let taskService = TaskService(+            modelContext: context,+            displayIDAllocator: DisplayIDAllocator(store: counterStore)+        )+        let project = makeProject(in: context)+        var lifecycle = AddTaskSaveLifecycle()++        let didBegin = lifecycle.beginSave()+        #expect(didBegin)+        let saveTask = Task { @MainActor in+            try await AddTaskSheet.persist(draft: makeDraft(project: project), taskService: taskService)+        }++        let reachedAllocation = await counterStore.waitUntilAllocationStarts()+        #expect(reachedAllocation, "The test must cancel while display-ID allocation is suspended")+        let didRequestCancellation = lifecycle.cancelForDisappearance()+        #expect(didRequestCancellation)+        saveTask.cancel()+        await counterStore.releaseAllocation()++        await #expect(throws: CancellationError.self) {+            try await saveTask.value+        }++        lifecycle.completeCancellation()+        #expect(lifecycle.blocksDismissal == false)+        #expect(+            try context.fetch(FetchDescriptor<TransitTask>()).isEmpty,+            "Closing Add Task during a gated allocation must not insert a task [T-1898]"+        )+    }++    @Test func successfulCreateDoesNotCancelWhenItsDismissalMakesTheViewDisappear() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let taskService = TaskService(+            modelContext: context,+            displayIDAllocator: DisplayIDAllocator(store: InMemoryCounterStore())+        )+        let project = makeProject(in: context)+        var lifecycle = AddTaskSaveLifecycle()++        let didBegin = lifecycle.beginSave()+        #expect(didBegin)+        try await AddTaskSheet.persist(draft: makeDraft(project: project), taskService: taskService)+        let didCompleteSave = lifecycle.completeSave()+        let didRequestCancellation = lifecycle.cancelForDisappearance()+        #expect(didCompleteSave)+        #expect(didRequestCancellation == false)+        #expect(lifecycle.blocksDismissal)+        lifecycle.resetAfterSuccessfulDismissal()+        #expect(lifecycle.blocksDismissal == false)+        let canBeginNextSave = lifecycle.beginSave()+        #expect(canBeginNextSave)+        #expect(try context.fetch(FetchDescriptor<TransitTask>()).count == 1)+    }++    @Test func cancellationPendingBlocksReplacementSaveUntilOriginalTaskSettles() {+        var lifecycle = AddTaskSaveLifecycle()++        let didBegin = lifecycle.beginSave()+        let didRequestCancellation = lifecycle.cancelForDisappearance()+        #expect(didBegin)+        #expect(didRequestCancellation)+        #expect(lifecycle.blocksDismissal == false)+        #expect(lifecycle.isCancellationPending)+        #expect(lifecycle.blocksSaveAction)+        let didBeginReplacementSave = lifecycle.beginSave()+        #expect(didBeginReplacementSave == false)++        lifecycle.completeCancellation()+        #expect(lifecycle.blocksSaveAction == false)+        let didBeginAfterCancellation = lifecycle.beginSave()+        #expect(didBeginAfterCancellation)+    }++    @Test func failedSaveReenablesDismissalAndRetry() {+        var lifecycle = AddTaskSaveLifecycle()++        let didBegin = lifecycle.beginSave()+        lifecycle.completeFailure()+        #expect(didBegin)+        #expect(lifecycle.blocksDismissal == false)++        let didRetry = lifecycle.beginSave()+        #expect(didRetry)+    }+}diff --git a/specs/bugfixes/macos-add-task-save-cancellation/report.md b/specs/bugfixes/macos-add-task-save-cancellation/report.mdnew file mode 100644index 0000000..a5f7b42--- /dev/null+++ b/specs/bugfixes/macos-add-task-save-cancellation/report.md@@ -0,0 +1,94 @@+# Bugfix Report: macOS Add Task Save Cancellation++**Date:** 2026-08-05+**Status:** Fixed++## Description of the Issue++On macOS, the singleton **New Task** window launched an unretained asynchronous `Task` when Save was pressed. If the window closed while display-ID allocation was suspended, the task could continue and persist a new task after the user dismissed the window.++**Reproduction steps:**+1. Open New Task on macOS and enter a valid task.+2. Press Save while display-ID allocation is delayed.+3. Close the New Task window before allocation completes.+4. Release allocation and observe that the task can be inserted after the window closed.++**Impact:** A user could create an unintended task by closing the creation window during a slow allocation.++## Investigation Summary++- **Symptoms examined:** The `Task { await save() }` launched by the Add Task toolbar was not stored or cancelled on disappearance.+- **Code inspected:** `AddTaskSheet.swift`, `AddTaskSheet+Save.swift`, `TaskService.swift`, `DisplayIDAllocator.swift`, and T-1858's milestone create lifecycle.+- **Hypotheses tested:** `TaskService` might persist despite cancellation. It already propagates `CancellationError` and checks cancellation immediately before insertion (T-1765); the missing layer was the Add Task view lifecycle.++## Discovered Root Cause++The macOS Add Task view had no retained save-task handle or state transition for window disappearance. Closing the view therefore did not deliver cancellation to the in-flight `Task`; service safeguards only take effect once its caller is cancelled.++**Defect type:** Lifecycle management / asynchronous cancellation gap.++**Why it occurred:**+1. Save created an unretained task.+2. The view had no `onDisappear` cancellation path.+3. Slow display-ID allocation permitted the window to close first.+4. The still-running task completed allocation and called the create service.+5. The service correctly observed no cancellation and inserted the task.++**Contributing factors:** iOS already disables interactive dismissal while saving, but the macOS singleton window had no equivalent lifecycle handling. T-1858 introduced the required state machine for milestone creation, but Add Task did not use it.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Views/AddTask/AddTaskSheet.swift` — retains the save task and shared lifecycle, disables existing iOS dismissal affordances from lifecycle state, cancels a pending save on disappearance, resets the singleton window only after a successful dismissal, and visibly disables replacement Save actions while a cancellation still settles.+- `Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift` — starts one retained task, completes success before dismissal, suppresses `CancellationError` alerts, and restores error/retry behavior for genuine failures.+- `Transit/Transit/Views/Shared/CreateSaveLifecycle.swift` — generalizes T-1858's lifecycle state machine and adds success-only reset support for reusable windows.+- `Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift` — retains the existing milestone name as a compatibility alias.+- `Transit/TransitTests/AddTaskSaveLifecycleTests.swift` — adds deterministic lifecycle and gated-allocation regressions.++**Approach rationale:** The view now owns the operation that its lifecycle can invalidate, while `TaskService` remains the persistence boundary that prevents an already-cancelled allocation from inserting. This preserves T-2037's milestone validation and existing create-service cancellation behavior.++**Alternatives considered:**+- Cancel only on macOS without explicit save state — rejected because a successful `dismiss()` would race its own `onDisappear` and could be incorrectly treated as a failure.+- Add compensating deletion after cancellation — rejected because cancellation before the service insertion boundary is atomic and avoids deleting a successfully persisted task.++## Regression Test++**Test file:** `Transit/TransitTests/AddTaskSaveLifecycleTests.swift`++**What it verifies:** A deterministic `AllocationGatedCounterStore` test cancels while display-ID allocation is suspended and proves `CancellationError` plus zero persisted tasks. Companion lifecycle tests prove successful dismissal is not cancelled, a reused window cannot start a replacement save until cancellation settles, and a genuine failure reenables retry.++**Test scope:** These unit tests intentionally exercise the lifecycle state machine and `TaskService` persistence boundary rather than instantiate `AddTaskSheet` and drive its SwiftUI `@State` / `@Environment` action wiring. They do not independently prove the retained `saveTask` and `onDisappear` calls; those remain integration behavior compiled with the view.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Views/AddTask/AddTaskSheet.swift` | Retained save ownership and disappearance cancellation. |+| `Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift` | Lifecycle-aware save outcomes and silent cancellation handling. |+| `Transit/Transit/Views/Shared/CreateSaveLifecycle.swift` | Shared creation lifecycle helper. |+| `Transit/Transit/Views/Settings/MilestoneCreateSaveLifecycle.swift` | T-1858 compatibility alias. |+| `Transit/TransitTests/AddTaskSaveLifecycleTests.swift` | Deterministic regressions. |+| `CHANGELOG.md` | T-1898 release note. |++## Verification++**Automated:**+- [x] Focused `AddTaskSaveLifecycleTests` passes on macOS.+- [x] `make test-quick` passes.+- [x] `make build-macos` passes.+- [x] `make lint` passes.++**Manual verification:** Not run; the deterministic gated allocator regression simulates closing New Task during the exact allocation suspension window.++## Prevention++Creation views that can disappear during asynchronous persistence must retain the task and use an explicit lifecycle state machine. Successful persistence must transition before `dismiss()` so its own disappearance cannot cancel it.++## Related++- T-1898+- T-1858+- T-1765+- T-2037