Pre-push review: T-1756 failed reload removes retry banner

diff --git a/prism/Services/FileChangeObserver.swift b/prism/Services/FileChangeObserver.swiftindex f30971e..eaf1b6b 100644--- a/prism/Services/FileChangeObserver.swift+++ b/prism/Services/FileChangeObserver.swift@@ -34,6 +34,14 @@ final class FileChangeObserver: NSObject, NSFilePresenter {     /// Indicates whether the file has been modified externally since last acknowledged.     var fileChangedExternally = false +    /// Monotonically increasing count of external-change notifications.+    ///+    /// Callers that handle a change asynchronously (e.g. a reload that awaits+    /// background parsing) capture this before starting and acknowledge with+    /// `acknowledgeChange(upTo:)` so a change arriving mid-handling is not+    /// silently cleared (T-1756).+    private(set) var changeGeneration = 0+     /// The URL of the file being monitored.     private let fileURL: URL @@ -71,15 +79,37 @@ final class FileChangeObserver: NSObject, NSFilePresenter {     /// is modified by another process or iCloud sync.     nonisolated func presentedItemDidChange() {         Task { @MainActor in-            fileChangedExternally = true+            recordExternalChange()         }     } -    /// Acknowledges that the change has been handled.+    /// Records an external change: raises the banner flag and bumps the+    /// change generation. Split out of `presentedItemDidChange()` so it runs+    /// synchronously on the MainActor (and so tests can simulate a change+    /// deterministically, without the notification's task hop).+    func recordExternalChange() {+        fileChangedExternally = true+        changeGeneration += 1+    }++    /// Acknowledges that the change has been handled unconditionally.     ///-    /// Call this method when the user dismisses the reload banner or after-    /// the document has been reloaded.+    /// Call this method when the user dismisses the reload banner — an+    /// explicit dismissal covers whatever changes have arrived so far.     func acknowledgeChange() {         fileChangedExternally = false     }++    /// Acknowledges the change only if no newer change has arrived since+    /// `generation` was captured.+    ///+    /// Used by the reload path: the reload reads the file and awaits+    /// background parsing, so a change notification can arrive while it is+    /// in flight. Clearing the flag unconditionally on success would swallow+    /// that change — same symptom as T-1756, narrowed to a race window. With+    /// a stale generation this is a no-op and the banner stays up.+    func acknowledgeChange(upTo generation: Int) {+        guard changeGeneration == generation else { return }+        fileChangedExternally = false+    } }

diff --git a/prism/Views/DocumentLayoutCoordinator.swift b/prism/Views/DocumentLayoutCoordinator.swiftindex 5860dae..77c7a34 100644--- a/prism/Views/DocumentLayoutCoordinator.swift+++ b/prism/Views/DocumentLayoutCoordinator.swift@@ -626,14 +626,33 @@ final class DocumentLayoutCoordinator {     /// parse pipeline (footnotes, comment blocks, expansion reset, table     /// display mode reset) rather than calling `MarkdownBlockParser.parse`     /// directly. See T-724.-    func reloadDocument(session: DocumentSession) {-        guard case .file(let url) = session.source else { return }+    ///+    /// - Returns: The reload task, so tests can await the success-path+    ///   acknowledgement deterministically instead of sleeping (T-1756);+    ///   `nil` when the session is not file-backed. Production callers+    ///   discard it.+    @discardableResult+    func reloadDocument(session: DocumentSession) -> Task<Void, Never>? {+        guard case .file(let url) = session.source else { return nil } -        session.fileObserver?.acknowledgeChange()+        // Capture the change generation before the reload starts. The reload+        // awaits background parsing, so a fresh external-change notification+        // can arrive while it is in flight; the generation-guarded+        // acknowledgement below then no-ops and the banner stays up for the+        // change this reload never saw.+        let observedGeneration = session.fileObserver?.changeGeneration ?? 0 -        Task {+        return Task {             do {                 try await session.reloadContent(from: url)+                // Acknowledge only once the reload actually succeeds, and+                // only if no newer change arrived mid-reload. Acknowledging+                // unconditionally up front hid the retry banner (gated on+                // `fileChangedExternally`) even when the reload failed,+                // leaving the user with no way to retry short of waiting for+                // another external-change notification or reopening the+                // document (T-1756).+                session.fileObserver?.acknowledgeChange(upTo: observedGeneration)             } catch {                 reloadError = String(localized: "error.reload",                                      defaultValue: "Reload failed: \(error.localizedDescription)")

diff --git a/prismTests/DocumentLayoutCoordinatorReloadTests.swift b/prismTests/DocumentLayoutCoordinatorReloadTests.swiftindex 8fcd04a..143a973 100644--- a/prismTests/DocumentLayoutCoordinatorReloadTests.swift+++ b/prismTests/DocumentLayoutCoordinatorReloadTests.swift@@ -168,4 +168,113 @@ struct DocumentLayoutCoordinatorReloadTests {         #expect(session.content == updatedMarkdown,                 "Session content must be updated to match the reloaded file")     }++    // MARK: - Retry Banner Survives a Failed Reload (T-1756)++    @Test("reloadDocument keeps the retry banner visible when reloadContent throws")+    @MainActor+    func reloadDocumentKeepsBannerOnFailure() async throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent("reload-failure-\(UUID().uuidString).md")+        let originalMarkdown = "# Document\n\nOriginal content"+        try originalMarkdown.write(to: tempURL, atomically: true, encoding: .utf8)++        let session = DocumentSession(url: tempURL, content: originalMarkdown)+        await session.parseContent()++        // Simulate the file-change notification that shows the retry banner.+        session.fileObserver?.recordExternalChange()++        // Remove the file out from under the session so the async reload's+        // `Data(contentsOf:)` read throws, forcing `reloadDocument` down the+        // catch path. No `defer` cleanup needed: the file is already gone.+        try FileManager.default.removeItem(at: tempURL)++        let coordinator = DocumentLayoutCoordinator()+        // Await the returned reload task: the error-path assignment to+        // `reloadError` has run by the time it completes, so no polling or+        // fixed sleep is needed.+        let reloadTask = try #require(coordinator.reloadDocument(session: session))+        await reloadTask.value++        // Expected: a failed reload must leave `fileChangedExternally` true so+        // the retry banner (gated on that flag) stays available. Actual (bug):+        // `reloadDocument` acknowledged the change unconditionally before the+        // async reload ran, so the banner disappeared even on failure.+        #expect(coordinator.reloadError != nil, "Reload should have failed and set reloadError")+        #expect(session.fileObserver?.fileChangedExternally == true,+                "Retry banner flag must remain set after a failed reload")+    }++    @Test("reloadDocument clears the retry banner on a clean successful reload")+    @MainActor+    func reloadDocumentClearsBannerOnSuccess() async throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent("reload-success-banner-\(UUID().uuidString).md")+        let originalMarkdown = "# Document\n\nOriginal content"+        try originalMarkdown.write(to: tempURL, atomically: true, encoding: .utf8)+        defer { try? FileManager.default.removeItem(at: tempURL) }++        let session = DocumentSession(url: tempURL, content: originalMarkdown)+        await session.parseContent()++        // Simulate the file-change notification that shows the retry banner,+        // then write the changed content it announced.+        session.fileObserver?.recordExternalChange()+        let updatedMarkdown = "# Updated\n\nNew content"+        try updatedMarkdown.write(to: tempURL, atomically: true, encoding: .utf8)++        let coordinator = DocumentLayoutCoordinator()+        // Await the returned reload task: the success-path acknowledgement+        // has run by the time it completes, so no polling is needed.+        let reloadTask = try #require(coordinator.reloadDocument(session: session))+        await reloadTask.value++        #expect(coordinator.reloadError == nil, "Clean reload must not set reloadError")+        #expect(session.content == updatedMarkdown,+                "Session content must reflect the reloaded file")+        #expect(session.fileObserver?.fileChangedExternally == false,+                "Banner flag must clear once the reload succeeds with no newer change")+    }++    @Test("reloadDocument keeps the banner when a change arrives mid-reload")+    @MainActor+    func reloadDocumentKeepsBannerWhenChangeArrivesMidReload() async throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent("reload-midflight-change-\(UUID().uuidString).md")+        let originalMarkdown = "# Document\n\nOriginal content"+        try originalMarkdown.write(to: tempURL, atomically: true, encoding: .utf8)+        defer { try? FileManager.default.removeItem(at: tempURL) }++        let session = DocumentSession(url: tempURL, content: originalMarkdown)+        await session.parseContent()++        // First external change: banner comes up, reload will be started.+        session.fileObserver?.recordExternalChange()+        let updatedMarkdown = "# Updated\n\nNew content"+        try updatedMarkdown.write(to: tempURL, atomically: true, encoding: .utf8)++        let coordinator = DocumentLayoutCoordinator()+        // reloadDocument captures the change generation synchronously before+        // its async work starts, and both this test and the reload task run+        // on the MainActor with no suspension in between…+        let reloadTask = try #require(coordinator.reloadDocument(session: session))+        // …so bumping the generation here is deterministically "mid-reload":+        // it lands after the capture and before the success-path+        // acknowledgement, exactly like a second external change arriving+        // while `reloadContent` awaits background parsing.+        session.fileObserver?.recordExternalChange()++        // Await the reload task: its success-path acknowledgement (where the+        // pre-fix code wrongly cleared the flag) has run by the time it+        // completes. Under a lazy-capture regression (generation read inside+        // the task rather than before it) the acknowledgement would match+        // and clear the flag, failing the expectation below deterministically.+        await reloadTask.value++        #expect(session.content == updatedMarkdown,+                "The reload itself must still succeed")+        #expect(session.fileObserver?.fileChangedExternally == true,+                "A change arriving mid-reload must keep the banner up — the reload never saw it")+    } }

diff --git a/prismTests/FileChangeObserverTests.swift b/prismTests/FileChangeObserverTests.swiftindex 58b91c4..d751014 100644--- a/prismTests/FileChangeObserverTests.swift+++ b/prismTests/FileChangeObserverTests.swift@@ -142,4 +142,74 @@ struct FileChangeObserverTests {         observer.acknowledgeChange()         #expect(observer.fileChangedExternally == false)     }++    // MARK: - Generation-Guarded Acknowledgement (T-1756)++    @Test("presentedItemDidChange bumps the change generation")+    @MainActor+    func presentedItemDidChangeBumpsGeneration() async throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent(UUID().uuidString)+            .appendingPathExtension("md")++        try "# Test".write(to: tempURL, atomically: true, encoding: .utf8)+        defer { try? FileManager.default.removeItem(at: tempURL) }++        let observer = FileChangeObserver(fileURL: tempURL)+        #expect(observer.changeGeneration == 0)++        observer.presentedItemDidChange()++        // `presentedItemDidChange()` hops to the MainActor via a `Task`, so+        // poll for the bump rather than sleeping a fixed duration — under+        // full-suite load a flat 10ms wait is a scheduling-delay flake.+        for _ in 0..<200 where observer.changeGeneration == 0 {+            try await Task.sleep(for: .milliseconds(5))+        }++        #expect(observer.changeGeneration == 1)+    }++    @Test("acknowledgeChange(upTo:) with the captured generation clears the flag")+    @MainActor+    func generationAcknowledgeClearsWhenCurrent() throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent(UUID().uuidString)+            .appendingPathExtension("md")++        try "# Test".write(to: tempURL, atomically: true, encoding: .utf8)+        defer { try? FileManager.default.removeItem(at: tempURL) }++        let observer = FileChangeObserver(fileURL: tempURL)+        observer.recordExternalChange()+        let captured = observer.changeGeneration++        observer.acknowledgeChange(upTo: captured)++        #expect(observer.fileChangedExternally == false)+    }++    @Test("acknowledgeChange(upTo:) with a stale generation keeps the flag set")+    @MainActor+    func generationAcknowledgeNoOpsWhenStale() throws {+        let tempURL = FileManager.default.temporaryDirectory+            .appendingPathComponent(UUID().uuidString)+            .appendingPathExtension("md")++        try "# Test".write(to: tempURL, atomically: true, encoding: .utf8)+        defer { try? FileManager.default.removeItem(at: tempURL) }++        let observer = FileChangeObserver(fileURL: tempURL)+        observer.recordExternalChange()+        let captured = observer.changeGeneration++        // A second change arrives after the caller captured the generation+        // (e.g. mid-reload). Acknowledging with the stale token must not+        // swallow it.+        observer.recordExternalChange()+        observer.acknowledgeChange(upTo: captured)++        #expect(observer.fileChangedExternally == true,+                "A newer change must survive a stale acknowledgement")+    } }