Pre-push review: T-1805 remote refresh outlives document

diff --git a/prism/ViewModels/RemoteRefreshFlow.swift b/prism/ViewModels/RemoteRefreshFlow.swiftnew file mode 100644index 0000000..3ecc6c2--- /dev/null+++ b/prism/ViewModels/RemoteRefreshFlow.swift@@ -0,0 +1,160 @@+//+//  RemoteRefreshFlow.swift+//  prism+//+//  Created by Claude on 16/8/2026.+//++import Foundation++/// Drives the asynchronous half of a URL document refresh (Req 5.x).+///+/// `DocumentReaderView.refreshFromURL()` used to fire an untracked, uncanceled+/// `Task` with no staleness check: closing the document (or starting a second+/// refresh before the first finished) left the first request free to keep+/// running in the background, and on completion it would still call+/// `RecentFilesManager.updateTitle(forRemoteURL:title:)` for a document the+/// user had already moved on from — overwriting the recents title with stale+/// data (T-1805).+///+/// This follows the same task-tracking + monotonic-epoch pattern+/// `RemoteContentCoordinator` uses for the initial URL open (T-862): starting+/// a new refresh cancels any in-flight one, and a request id captured at+/// start time lets a canceled/superseded request's completion recognize it+/// is no longer current and skip writing state (`isRefreshing`,+/// `refreshError`, the recents title) that now belongs to someone else.+///+/// Which mechanism covers which case — worth being precise, because an+/// earlier version of this comment got it backwards:+///+/// - **Superseded refresh** (the user taps Refresh again): `refresh()` itself+///   bumps `currentRequestID`, so the epoch guard alone is sufficient. No+///   external call is needed.+/// - **Closed document** (the reader goes away with one refresh in flight):+///   the epoch moves ONLY when `refresh()` or `cancel()` is called on this+///   instance, and neither happens by itself. So `DocumentReaderView`'s+///   `.onDisappear { refreshFlow.cancel() }` is the PRIMARY mechanism here,+///   not a nicety — without it nothing bumps the epoch and the orphaned task+///   would run to completion and write the stale recents title, which is+///   exactly T-1805. The task body's `[weak self]` capture is the backstop:+///   once the view's `@State` releases this flow, a still-running request+///   finds `self == nil` and writes nothing even if `cancel()` never ran.+@Observable+@MainActor+final class RemoteRefreshFlow {+    /// Loader closure type. Extracted to allow tests to inject a controllable+    /// loader that can delay or fail deterministically.+    typealias Loader = (URL) async throws -> URLDocumentLoader.LoadResult++    /// Whether a refresh is currently in flight.+    private(set) var isRefreshing = false++    /// Localized error message from the most recently failed refresh.+    ///+    /// Written only by this flow; the view dismisses it via `dismissError()`.+    private(set) var refreshError: String?++    /// The in-flight refresh task, tracked so `cancel()` can stop it.+    ///+    /// `private(set)` rather than `private` so tests can join on the exact+    /// request they started (`await handle?.value`) instead of sleeping for a+    /// fixed settle window — the same test join point+    /// `RemoteContentCoordinator.downloadTask` exposes for T-862.+    private(set) var task: Task<Void, Never>?++    /// Identity of the most recently started refresh. A finishing task only+    /// applies its result — or clears `isRefreshing` — when this still+    /// matches its own id, so a canceled/superseded refresh cannot write+    /// state that belongs to a replacement refresh, or to a document the+    /// reader has since closed. (T-1805, following T-862.)+    private var currentRequestID: UInt64 = 0++    /// Injected loader used to fetch remote content. Defaults to+    /// `URLDocumentLoader.load(from:)` for production use.+    private let loader: Loader++    init(loader: @escaping Loader = { try await URLDocumentLoader.load(from: $0) }) {+        self.loader = loader+    }++    /// Re-downloads content from the remote URL and updates the session.+    ///+    /// On success, updates the rendered content while preserving notes (Req+    /// 5.4) and refreshes the cached recents title. On failure, records an+    /// error message and retains existing content (Req 5.5).+    func refresh(session: DocumentSession, recentFilesManager: RecentFilesManager) {+        guard case .url(let remote, _) = session.source else { return }++        // Cancel any in-flight refresh before starting a new one.+        task?.cancel()++        refreshError = nil+        isRefreshing = true++        // Bump epoch so a superseded task's defer/completion skips writing+        // state for the replacement request.+        currentRequestID &+= 1+        let requestID = currentRequestID++        // `[weak self]` matches `RemoteContentCoordinator` (T-862) and is the+        // backstop for a reader torn down without `.onDisappear` running: the+        // epoch cannot move on its own, so a strong capture would keep this+        // flow — and the stale write — alive for the whole download.+        task = Task { [weak self] in+            defer {+                if let self, self.currentRequestID == requestID {+                    self.isRefreshing = false+                    self.task = nil+                }+            }++            do {+                guard let loader = self?.loader else { return }+                // `remote` is already the fetch URL (e.g., raw.githubusercontent.com),+                // so GitHubURLTransformer inside load() will pass it through unchanged.+                let result = try await loader(remote)++                guard let self, !Task.isCancelled, self.currentRequestID == requestID else { return }++                await session.reloadContent(markdownString: result.content)++                guard !Task.isCancelled, self.currentRequestID == requestID else { return }++                // Update cached title in recent files in case the document title changed.+                if case .url(_, let displayURL) = session.source {+                    recentFilesManager.updateTitle(forRemoteURL: displayURL, title: session.cachedDocumentTitle)+                }+            } catch is CancellationError {+                // Unreachable with the production loader: `URLDocumentLoader.load`+                // wraps every non-`LoadError` — including `URLError.cancelled` and+                // `CancellationError` — as `LoadError.networkError`. Kept for+                // injected loaders. Real cancellation is suppressed by the+                // `!Task.isCancelled` guard in the catch below.+            } catch {+                guard let self, !Task.isCancelled, self.currentRequestID == requestID else { return }+                self.refreshError = error.localizedDescription+            }+        }+    }++    /// Dismisses the failure alert without touching the in-flight request.+    ///+    /// Acknowledging a failure is a separate act from abandoning a request,+    /// which is why `cancel()` deliberately leaves `refreshError` alone.+    func dismissError() {+        refreshError = nil+    }++    /// Cancels the in-flight refresh (used when the reader disappears).+    ///+    /// Does NOT clear `refreshError`: an unacknowledged failure belongs to the+    /// user, not to the request being abandoned.+    func cancel() {+        task?.cancel()+        task = nil+        // Bump epoch so the canceled task's completion skips further state+        // mutation even if cancellation doesn't unwind it promptly.+        currentRequestID &+= 1+        isRefreshing = false+    }+}

diff --git a/prism/Views/DocumentReaderView.swift b/prism/Views/DocumentReaderView.swiftindex eff17e0..665cb37 100644--- a/prism/Views/DocumentReaderView.swift+++ b/prism/Views/DocumentReaderView.swift@@ -87,11 +87,10 @@ struct DocumentReaderView: View {     /// Sheet presentation for notes in compact mode.     @State private var showNotesSheet = false -    /// Error message when URL refresh fails (Req 5.5).-    @State private var refreshError: String?--    /// Whether a URL refresh is in progress.-    @State private var isRefreshing = false+    /// Drives the asynchronous URL refresh, tracking the in-flight task and+    /// guarding against a canceled/superseded refresh writing stale state+    /// (T-1805).+    @State private var refreshFlow = RemoteRefreshFlow()      #if os(macOS)     /// Whether the username prompt alert is shown for file export.@@ -132,7 +131,12 @@ struct DocumentReaderView: View {     }      var body: some View {-        GeometryReader { geo in+        // Tracked read of the refresh flow's error (see the "Refresh Failed"+        // alert below): Observation only records properties read during body+        // evaluation, not ones read inside a Binding's closures.+        let refreshFlowError = refreshFlow.refreshError++        return GeometryReader { geo in             #if os(macOS)             let useCompact = false             #else@@ -194,24 +198,29 @@ struct DocumentReaderView: View {                 if session.source.isURL {                     ToolbarItem(placement: .primaryAction) {                         Button {-                            refreshFromURL()+                            refreshFlow.refresh(session: session, recentFilesManager: recentFilesManager)                         } label: {                             Image(systemName: "arrow.clockwise")                         }                         .accessibilityLabel(LocalizedStringKey("Refresh"))                         .help("Refresh")-                        .disabled(isRefreshing)+                        .disabled(refreshFlow.isRefreshing)                     }                 }             }+            // `refreshError` is read here, in `body`, so Observation registers the+            // dependency. Reading it only inside the Binding's `get` and the+            // `message:` builder would not: neither closure is a tracked access,+            // and presentation would then rely on `isRefreshing` (read by+            // `.disabled` above) happening to change in the same MainActor turn.             .alert("Refresh Failed", isPresented: Binding(-                get: { refreshError != nil },-                set: { if !$0 { refreshError = nil } }+                get: { refreshFlowError != nil },+                set: { if !$0 { refreshFlow.dismissError() } }             )) {-                Button("OK") { refreshError = nil }+                Button("OK") { refreshFlow.dismissError() }             } message: {-                if let error = refreshError {-                    Text(error)+                if let refreshFlowError {+                    Text(refreshFlowError)                 }             }             // Inject expansion coordinator for DetailsBlockView (collapsible-sections)@@ -281,7 +290,7 @@ struct DocumentReaderView: View {             // T-987: Sync notes whenever parse output changes.             //             // `parseRevision` bumps on every successful parse (initial load,-            // file reload via `reloadDocument`, URL refresh via `refreshFromURL`).+            // file reload via `reloadDocument`, URL refresh via `RemoteRefreshFlow.refresh`).             // Tying notes loading to this revision — instead of `session.id`,             // which is stable across reloads — ensures persisted notes are             // re-relocated and imported notes are rebuilt against the new@@ -321,6 +330,32 @@ struct DocumentReaderView: View {             // Cancel any pending task when view disappears             .onDisappear {                 saveFlow.cancel()+                // T-1805: stop an in-flight URL refresh from outliving the+                // document — otherwise it keeps downloading after the reader+                // has closed, and writes that URL's Recent Files title on+                // completion for a document the user has already left.+                //+                // This call is the PRIMARY mechanism for the close case, not a+                // nicety. RemoteRefreshFlow's monotonic epoch only advances+                // when refresh() or cancel() is called on it, and closing a+                // document calls neither by itself — so without this line+                // nothing invalidates the in-flight request and the stale write+                // goes through. (The epoch guard is self-sufficient only for+                // the OTHER case: a second Refresh superseding the first.)+                //+                // Residual assumption: `.onDisappear` firing on document close+                // and document switch. It holds today because both+                // `DocumentFlowCoordinator.closeDocument` and `activateSession`+                // reset `navigationPath`, popping this view off the+                // NavigationStack; backgrounding (`handleScenePhaseChange`)+                // correctly does not. What is NOT pinned is SwiftUI itself+                // always delivering `.onDisappear`. The backstop for that is+                // the `[weak self]` capture in RemoteRefreshFlow's task: once+                // this view's `@State` releases the flow, a still-running+                // request writes nothing. The wiring (that this call exists in+                // this modifier at all) IS pinned, by+                // `RemoteRefreshTeardownWiringTests`.+                refreshFlow.cancel()             }             // Initialize sidebar visibility from persisted settings             .onAppear {@@ -613,34 +648,6 @@ struct DocumentReaderView: View {         }     }     #endif--    // MARK: - URL Refresh--    /// Re-downloads content from the remote URL and updates the session.-    ///-    /// On success, updates the rendered content while preserving notes (Req 5.4).-    /// On failure, shows an error alert and retains existing content (Req 5.5).-    private func refreshFromURL() {-        guard case .url(let remote, _) = session.source else { return }--        isRefreshing = true-        Task {-            defer { isRefreshing = false }-            do {-                // `remote` is already the fetch URL (e.g., raw.githubusercontent.com),-                // so GitHubURLTransformer inside load() will pass it through unchanged.-                let result = try await URLDocumentLoader.load(from: remote)-                await session.reloadContent(markdownString: result.content)--                // Update cached title in recent files in case the document title changed-                if case .url(_, let displayURL) = session.source {-                    recentFilesManager.updateTitle(forRemoteURL: displayURL, title: session.cachedDocumentTitle)-                }-            } catch {-                refreshError = error.localizedDescription-            }-        }-    } }  // MARK: - Document Link Router Environment (T-1590)

diff --git a/prismTests/RemoteRefreshFlowTests.swift b/prismTests/RemoteRefreshFlowTests.swiftnew file mode 100644index 0000000..0e711c0--- /dev/null+++ b/prismTests/RemoteRefreshFlowTests.swift@@ -0,0 +1,587 @@+//+//  RemoteRefreshFlowTests.swift+//  prismTests+//+//  Regression tests for T-1805: Remote URL refresh outlives document and+//  can overwrite recent title.+//++import Foundation+import Testing+@testable import prism++/// Tests for `RemoteRefreshFlow` focused on the race between a canceled+/// refresh (document closed, or a replacement refresh started) and its+/// completion still writing state that no longer belongs to it.+@Suite("RemoteRefreshFlow", .serialized)+@MainActor+struct RemoteRefreshFlowTests {++    private func makeRecentFilesManager(suiteName: String) -> RecentFilesManager {+        let defaults = UserDefaults(suiteName: suiteName)!+        defaults.removePersistentDomain(forName: suiteName)+        return RecentFilesManager(storage: defaults)+    }++    /// Regression for T-1805.+    ///+    /// When the reader closes the document (`cancel()`, mirroring+    /// `.onDisappear`) while a refresh is still in flight, the refresh's+    /// eventual completion must not write the recents title for the URL the+    /// user has moved on from.+    ///+    /// Before the fix, the refresh ran as an untracked `Task` with no+    /// staleness check, so it always called+    /// `RecentFilesManager.updateTitle(forRemoteURL:title:)` on completion —+    /// even after the document was closed.+    @Test("Canceled refresh does not overwrite the recents title after the document closes")+    func canceledRefreshDoesNotOverwriteRecentTitle() async throws {+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.canceled")++        // Seed a recents entry with the title the user currently sees.+        recentFilesManager.addEntry(RecentFileEntry(displayURL: displayURL).withTitle("Original Title"))++        let refreshStarted = AsyncSignal()+        let letRefreshFinish = AsyncSignal()++        let loader: RemoteRefreshFlow.Loader = { _ in+            refreshStarted.fire()+            await letRefreshFinish.wait()+            return URLDocumentLoader.LoadResult(+                content: "# Stale Title\n\nContent fetched after the reader closed.",+                fetchURL: displayURL,+                displayURL: displayURL+            )+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original Title")++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        #expect(flow.isRefreshing == true)++        // Wait until the loader is actually in flight before "closing" the document.+        await refreshStarted.wait()++        // Capture the handle BEFORE cancel() nils it, so the assertion below can+        // join on the orphan deterministically. Sleeping for a fixed settle+        // window instead would let this test pass VACUOUSLY on a loaded machine:+        // it asserts the absence of a write, so an orphan that simply hasn't run+        // yet looks identical to an orphan that was correctly suppressed.+        let orphan = flow.task++        // Simulate the reader disappearing (document closed) mid-refresh.+        flow.cancel()+        #expect(flow.isRefreshing == false)++        // Now let the (canceled) load resolve, and wait for it to finish fully.+        letRefreshFinish.fire()+        await orphan?.value++        #expect(+            recentFilesManager.recentFiles.first?.title == "Original Title",+            "A refresh canceled by the document closing must not overwrite the recents title"+        )+    }++    /// Verifies that starting a second refresh cancels the first, and the+    /// first's (superseded) completion does not clear `isRefreshing` or+    /// `refreshError` for the second, in-flight request. Mirrors the+    /// `RemoteContentCoordinator` T-862 regression shape.+    @Test("Superseded refresh does not clear state for the replacement refresh")+    func supersededRefreshDoesNotClearReplacementState() async throws {+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.superseded")++        actor CallCounter { var count = 0; func next() -> Int { count += 1; return count } }+        let counter = CallCounter()++        let firstRequestStarted = AsyncSignal()+        let firstRequestFinished = AsyncSignal()+        let secondRequestStarted = AsyncSignal()++        let loader: RemoteRefreshFlow.Loader = { _ in+            let index = await counter.next()+            if index == 1 {+                firstRequestStarted.fire()+                await firstRequestFinished.wait()+                throw CancellationError()+            } else {+                secondRequestStarted.fire()+                try await Task.sleep(for: .seconds(60))+                return URLDocumentLoader.LoadResult(+                    content: "# ok", fetchURL: displayURL, displayURL: displayURL+                )+            }+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original")+        defer { flow.cancel() }++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        #expect(flow.isRefreshing == true)++        // Wait for request 1 to actually be in flight before superseding it, so+        // the CallCounter indices cannot invert. Without this the two refresh()+        // calls race, and an inverted order would hang the test on+        // `secondRequestStarted` rather than fail it.+        await firstRequestStarted.wait()+        let requestA = flow.task++        // Start the replacement refresh — cancels the first.+        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        #expect(flow.isRefreshing == true)++        await secondRequestStarted.wait()++        // Let the first request's cancellation finish unwinding, deterministically.+        firstRequestFinished.fire()+        await requestA?.value++        #expect(+            flow.isRefreshing == true,+            "The first request's defer must not clear isRefreshing for the still in-flight replacement"+        )+    }++    /// The literal "two overlapping refreshes, no defined winner" scenario.+    ///+    /// `supersededRefreshDoesNotClearReplacementState` above only reaches the+    /// `catch is CancellationError` branch, because its first loader throws+    /// once released. That leaves the SUCCESS-path guards+    /// (`guard !Task.isCancelled, self.currentRequestID == requestID`) untested+    /// for a request superseded by a second `refresh()` rather than by+    /// `cancel()` — i.e. request A returning real data, normally, after+    /// request B has already taken over.+    ///+    /// A loader that ignores cancellation is exactly what production faces:+    /// `URLDocumentLoader.load` can already have the bytes in hand when the+    /// supersede lands, so it returns a value instead of throwing. Without the+    /// post-load guard, A's stale content would be written into the session+    /// (`reloadContent`) under B — the document silently showing the loser's+    /// data. Asserting on `session.content` pins the FIRST guard specifically:+    /// the second guard, further down, would still block the recents-title+    /// write on its own, so a recents-only assertion would stay green with the+    /// first guard deleted.+    @Test("Superseded refresh completing successfully does not apply its result")+    func supersededRefreshCompletingSuccessfullyDoesNotApplyResult() async throws {+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.supersededSuccess")+        recentFilesManager.addEntry(RecentFileEntry(displayURL: displayURL).withTitle("Original Title"))++        actor CallCounter { var count = 0; func next() -> Int { count += 1; return count } }+        let counter = CallCounter()++        let firstRequestInFlight = AsyncSignal()+        let letFirstRequestSucceed = AsyncSignal()+        let secondRequestStarted = AsyncSignal()++        let loader: RemoteRefreshFlow.Loader = { _ in+            let index = await counter.next()+            if index == 1 {+                firstRequestInFlight.fire()+                // Deliberately NOT cancellation-aware: request A resolves with+                // real data after being superseded, which is the case the+                // success-path guard exists for.+                await letFirstRequestSucceed.wait()+                return URLDocumentLoader.LoadResult(+                    content: "# Stale Title\n\nContent from the superseded request.",+                    fetchURL: displayURL,+                    displayURL: displayURL+                )+            } else {+                secondRequestStarted.fire()+                try await Task.sleep(for: .seconds(60))+                return URLDocumentLoader.LoadResult(+                    content: "# Winner", fetchURL: displayURL, displayURL: displayURL+                )+            }+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original Title")++        // Request A starts and is genuinely in flight.+        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await firstRequestInFlight.wait()++        // Hold A's handle before B replaces it, so the assertions below can join+        // on A deterministically rather than sleeping — these are absence-of-write+        // assertions, which a too-short settle window would satisfy vacuously.+        let requestA = flow.task++        // Request B supersedes it (no cancel() anywhere in this test).+        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await secondRequestStarted.wait()++        // Only now does A resolve — successfully — and we wait for it to finish.+        letFirstRequestSucceed.fire()+        await requestA?.value++        #expect(+            session.content == "# Original Title",+            """+            The superseded request must not call session.reloadContent: its content \+            belongs to a request the user has already replaced (T-1805).+            """+        )+        #expect(+            recentFilesManager.recentFiles.first?.title == "Original Title",+            "The superseded request must not write the recents title"+        )+        #expect(+            flow.isRefreshing == true,+            "The superseded request must not clear isRefreshing, which belongs to the replacement"+        )++        flow.cancel()+    }++    /// A retry must start from a clean error state: `refresh()` clears+    /// `refreshError` up front, so the alert bound to it in+    /// `DocumentReaderView` cannot keep showing the previous attempt's failure+    /// while the new attempt is in flight. The pre-extraction inline+    /// implementation never reset it, so this is new behaviour worth pinning.+    @Test("Starting a refresh clears the previous attempt's error")+    func refreshClearsPreviousError() async throws {+        struct DummyError: LocalizedError {+            var errorDescription: String? { "dummy error" }+        }+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.clearsError")++        actor CallCounter { var count = 0; func next() -> Int { count += 1; return count } }+        let counter = CallCounter()+        let secondRequestStarted = AsyncSignal()++        let loader: RemoteRefreshFlow.Loader = { _ in+            let index = await counter.next()+            if index == 1 { throw DummyError() }+            secondRequestStarted.fire()+            try await Task.sleep(for: .seconds(60))+            return URLDocumentLoader.LoadResult(+                content: "# ok", fetchURL: displayURL, displayURL: displayURL+            )+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original")+        defer { flow.cancel() }++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await flow.task?.value+        #expect(flow.refreshError == "dummy error")++        // Retry: the stale error must be gone as soon as the attempt starts.+        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        #expect(+            flow.refreshError == nil,+            "A new refresh must clear the previous attempt's error so the alert does not linger"+        )++        await secondRequestStarted.wait()+        #expect(flow.refreshError == nil)+    }++    /// Pins `task?.cancel()` — the half of the fix the CHANGELOG leads with+    /// ("no longer leaves a background download running loose").+    ///+    /// Every other test here uses a loader that deliberately ignores+    /// cancellation, so all of them stay green with both `task?.cancel()` calls+    /// deleted: they only prove the RESULT is discarded, never that the download+    /// is stopped. This one uses a cancellation-aware loader — the shape+    /// `URLDocumentLoader.load` actually has — and fails (by hanging on+    /// `observedCancellation`) if the cancel is gone.+    @Test("Canceling a refresh actually cancels the in-flight download")+    func cancelStopsTheInFlightDownload() async throws {+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.cancelStops")++        let loaderStarted = AsyncSignal()+        let observedCancellation = AsyncSignal()++        let loader: RemoteRefreshFlow.Loader = { _ in+            loaderStarted.fire()+            do {+                try await Task.sleep(for: .seconds(60))+            } catch {+                observedCancellation.fire()+                throw error+            }+            return URLDocumentLoader.LoadResult(+                content: "# never", fetchURL: displayURL, displayURL: displayURL+            )+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original")++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await loaderStarted.wait()++        flow.cancel()++        // Bounded, so a regression fails legibly here instead of hanging the run.+        #expect(+            await observedCancellation.waitUntilFired(timeout: .seconds(5)),+            """+            cancel() must propagate cancellation into the running loader, not just \+            invalidate its result — otherwise the download keeps going after the \+            document closes, which is the headline half of T-1805.+            """+        )+    }++    /// A non-URL session must be a no-op — in particular it must not leave+    /// `isRefreshing` true, which would disable the Refresh button forever.+    @Test("Refreshing a non-URL session does nothing")+    func refreshIgnoresNonURLSession() async throws {+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.nonURL")++        let flow = RemoteRefreshFlow(loader: { _ in+            Issue.record("The loader must not run for a non-URL session")+            return URLDocumentLoader.LoadResult(+                content: "", fetchURL: URL(string: "https://example.com")!,+                displayURL: URL(string: "https://example.com")!+            )+        })+        let session = DocumentSession(clipboardContent: "# Clipboard")++        flow.refresh(session: session, recentFilesManager: recentFilesManager)++        #expect(flow.isRefreshing == false)+        #expect(flow.task == nil)+    }++    /// Happy path: a successful refresh updates the recents title and+    /// clears `isRefreshing`.+    @Test("Successful refresh updates the recents title")+    func successfulRefreshUpdatesRecentTitle() async throws {+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.success")+        recentFilesManager.addEntry(RecentFileEntry(displayURL: displayURL).withTitle("Original Title"))++        let loader: RemoteRefreshFlow.Loader = { _ in+            URLDocumentLoader.LoadResult(+                content: "# New Title\n\nUpdated content.",+                fetchURL: displayURL,+                displayURL: displayURL+            )+        }++        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original Title")++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await flow.task?.value++        #expect(flow.isRefreshing == false)+        #expect(flow.refreshError == nil)+        #expect(recentFilesManager.recentFiles.first?.title == "New Title")+    }++    /// A failed refresh records the error and clears `isRefreshing`.+    @Test("Failed refresh records the error")+    func failedRefreshRecordsError() async throws {+        struct DummyError: LocalizedError {+            var errorDescription: String? { "dummy error" }+        }+        let displayURL = URL(string: "https://example.com/doc.md")!+        let recentFilesManager = makeRecentFilesManager(suiteName: "RemoteRefreshFlowTests.failure")++        let loader: RemoteRefreshFlow.Loader = { _ in throw DummyError() }+        let flow = RemoteRefreshFlow(loader: loader)+        let session = DocumentSession(remoteURL: displayURL, displayURL: displayURL, content: "# Original")++        flow.refresh(session: session, recentFilesManager: recentFilesManager)+        await flow.task?.value++        #expect(flow.isRefreshing == false)+        #expect(flow.refreshError == "dummy error")+    }+}++/// Pins the production WIRING of `RemoteRefreshFlow.cancel()` into+/// `DocumentReaderView`'s teardown.+///+/// Everything in `RemoteRefreshFlowTests` above drives the flow directly, which+/// — by construction — proves the flow behaves once called and proves nothing+/// about whether anything calls it. That is the exact shape+/// `WebContentTerminationWiringTests` documents: T-1943 shipped a fully+/// unit-tested recovery path that NOTHING IN PRODUCTION EVER INVOKED, and+/// T-1099 regressed the same way after the WebKit cutover.+///+/// This call is load-bearing, not defence in depth. `currentRequestID` only+/// advances when `refresh()` or `cancel()` is called on the flow, and closing a+/// document calls neither on its own — so for the close-mid-refresh case (which+/// IS T-1805) the epoch guard cannot fire unless something invokes `cancel()`,+/// and `.onDisappear` is the only thing that does. The epoch guard is+/// self-sufficient only for the supersede case, pinned separately by+/// `supersededRefreshCompletingSuccessfullyDoesNotApplyResult`. Removing this+/// call REINSTATES the bug rather than merely degrading behaviour.+///+/// What this test can and cannot see: it pins that the two cancels sit in one+/// closure literal. It does NOT pin which view that modifier is attached to, or+/// that the closure is ever mounted — moving the whole modifier onto a child+/// view that never tears down would keep it green.+///+/// It is a source-structural check because there is no SwiftUI view-hosting+/// harness in this project (no ViewInspector, no NSHostingController pump) to+/// mount `DocumentReaderView` and drive a real `.onDisappear`, and+/// `refreshFlow` is private `@State` with no injection seam. This mirrors+/// `FootnotePresentationHostTests` (T-1893) and+/// `ModalPresentationSuspendsScrollHostTests` (T-1099), which pin their+/// view-modifier wiring the same way for the same reason.+@Suite("Remote refresh teardown wiring (T-1805)")+struct RemoteRefreshTeardownWiringTests {++    /// Reads a view source relative to this file, the same `#filePath`+    /// approach `FootnotePresentationHostTests` uses so the check needs no+    /// bundle resource wiring.+    private static func viewSource(_ fileName: String) throws -> String {+        let viewsDirectory = URL(fileURLWithPath: #filePath)+            .deletingLastPathComponent()   // prismTests+            .deletingLastPathComponent()   // repo root+            .appendingPathComponent("prism")+            .appendingPathComponent("Views")+        return try String(+            contentsOf: viewsDirectory.appendingPathComponent(fileName),+            encoding: .utf8+        )+    }++    /// Strips `//` line comments and collapses whitespace, so the assertions+    /// below match on CODE only — surviving both reformatting and the+    /// (deliberately long) explanatory comments inside the teardown block.+    private static func normalizedCode(_ source: String) -> String {+        source+            .split(separator: "\n", omittingEmptySubsequences: false)+            .map { line -> Substring in+                guard let comment = line.range(of: "//") else { return line }+                return line[..<comment.lowerBound]+            }+            .joined(separator: " ")+            .split(whereSeparator: \.isWhitespace)+            .joined(separator: " ")+    }++    /// Returns the balanced closure body from `text`, which must begin just+    /// INSIDE an opening brace. Tracks brace DEPTH rather than stopping at the+    /// first `}` — a naive first-`}` scan would truncate the region, and fail+    /// spuriously, the moment anyone put a nested `{ … }` (an `if`, a+    /// `withAnimation`) inside the teardown block.+    private static func balancedBody(of text: some StringProtocol) -> String? {+        var depth = 1+        var body = ""+        for character in text {+            if character == "{" {+                depth += 1+            } else if character == "}" {+                depth -= 1+                if depth == 0 { return body }+            }+            body.append(character)+        }+        return nil+    }++    @Test("DocumentReaderView cancels the refresh flow when the reader disappears")+    func readerCancelsRefreshFlowOnDisappear() throws {+        let source = try Self.normalizedCode(Self.viewSource("DocumentReaderView.swift"))++        // The cancel must live inside the SAME `.onDisappear` as the+        // pre-existing `saveFlow.cancel()` — a `refreshFlow.cancel()` sitting+        // anywhere else in the file would satisfy a bare `contains` check while+        // never running at teardown. Anchoring on `.onDisappear {` alone (not on+        // `saveFlow.cancel()`) keeps this test from failing over an unrelated+        // reordering or rename inside the same block.+        let teardowns = source.components(separatedBy: ".onDisappear {")+            .dropFirst()+            .compactMap { Self.balancedBody(of: $0) }++        guard !teardowns.isEmpty else {+            Issue.record("Could not locate DocumentReaderView's teardown .onDisappear")+            return+        }++        #expect(+            teardowns.contains { $0.contains("refreshFlow.cancel()") && $0.contains("saveFlow.cancel()") },+            """+            DocumentReaderView's teardown `.onDisappear` must call \+            `refreshFlow.cancel()` alongside `saveFlow.cancel()`. Without it an \+            in-flight URL refresh keeps downloading after the document closes AND \+            writes that URL's stale Recent Files title on completion (T-1805). \+            Nothing else prevents that write: RemoteRefreshFlow's epoch only \+            advances when refresh() or cancel() is called, and closing a document \+            calls neither — so removing this line reinstates the bug.+            """+        )+    }++    /// The `@State` declaration is the other half of the wiring: a reader that+    /// stopped owning a `RemoteRefreshFlow` would make the cancel above+    /// meaningless.+    @Test("DocumentReaderView owns the RemoteRefreshFlow it cancels")+    func readerOwnsRefreshFlow() throws {+        let source = try Self.viewSource("DocumentReaderView.swift")+        #expect(source.contains("@State private var refreshFlow = RemoteRefreshFlow()"))+        #expect(+            source.contains("refreshFlow.refresh(session: session, recentFilesManager: recentFilesManager)"),+            "The Refresh button must route through RemoteRefreshFlow, not an inline Task (T-1805)"+        )+    }+}++/// Minimal async signal helper for tests. Awaiters suspend until `fire()`+/// is called; subsequent `wait()` calls return immediately.+///+/// `wait()` is deliberately not cancellation-aware, so an ordering mistake+/// shows up as a hang rather than a failure. Use `waitUntilFired(timeout:)`+/// wherever a MISSING fire is the thing under test — that turns the hang into+/// a legible assertion failure.+private final class AsyncSignal: @unchecked Sendable {+    private let lock = NSLock()+    private var fired = false+    private var waiters: [CheckedContinuation<Void, Never>] = []++    /// Waits for `fire()`, giving up after `timeout`. Returns whether it fired.+    func waitUntilFired(timeout: Duration) async -> Bool {+        let deadline = ContinuousClock.now + timeout+        while ContinuousClock.now < deadline {+            if isFired { return true }+            try? await Task.sleep(for: .milliseconds(5))+        }+        return isFired+    }++    private var isFired: Bool {+        lock.lock()+        defer { lock.unlock() }+        return fired+    }++    func wait() async {+        await withCheckedContinuation { cont in+            lock.lock()+            if fired {+                lock.unlock()+                cont.resume()+            } else {+                waiters.append(cont)+                lock.unlock()+            }+        }+    }++    func fire() {+        lock.lock()+        guard !fired else { lock.unlock(); return }+        fired = true+        let pending = waiters+        waiters.removeAll()+        lock.unlock()+        for cont in pending { cont.resume() }+    }+}

diff --git a/prism/Models/DocumentSession.swift b/prism/Models/DocumentSession.swiftindex cef4620..d02cac5 100644--- a/prism/Models/DocumentSession.swift+++ b/prism/Models/DocumentSession.swift@@ -156,7 +156,7 @@ final class DocumentSession: Identifiable {     /// `DocumentReaderView`) observe this property as a `.task(id:)` key     /// to react to in-place content reloads — file reload via     /// `DocumentLayoutCoordinator.reloadDocument` and URL refresh via-    /// `DocumentReaderView.refreshFromURL` — without requiring the+    /// `RemoteRefreshFlow.refresh` — without requiring the     /// session ID to change.     ///     /// Only bumped after the T-718 staleness guard passes, so stale parse

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 2814e59..eab8ad4 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0  ### Fixed +- Refreshing a document opened from a URL no longer leaves a background download running loose after you close it (T-1805). Tapping Refresh started an untracked download with no way to stop it: closing the document, or navigating to a different one, before the download finished let it keep running, and when it finally completed it could still overwrite that URL's title in Recent Files — even though you had already moved on — or race a second refresh for a stale result. Closing the document, or starting another refresh, now cancels the one in flight, and a refresh that has been superseded or cancelled no longer applies its result once another one has taken over. - A `prism://open?url=…` link now opens the address it names, even when that address mixes already-escaped and unescaped characters (T-2140). `…/my%20file and more.md` was fetched as `…/my%2520file%20and%20more.md` — a different resource, with no error shown — because the address had already been unescaped one layer by the time it was read, and was then escaped a second time in full. Investigating it surfaced a second fault of the same kind, live on every markdown link and image in every document: the escaping used for addresses turned an escaped `%2F` back into a real `/`, splitting one path segment into two. That silently broke any address that identifies something by an escaped path — a GitLab project URL, for instance, which 404s once `group%2Fproj` becomes `group/proj`. Escaped slashes and escaped ampersands now survive every route into the app: typed and pasted addresses, deep links, document links and images, `mailto:` links, and the GitHub blob-to-raw rewrite. One narrow side effect of reworking that rewrite: a GitHub address written with a doubled slash in it (`github.com//owner/repo/blob/…`) is no longer recognised as a file address, so it now reports an unsupported content type instead of opening.  - In a document opened from a URL, an image or link whose query or anchor was already partly escaped no longer resolves to a corrupted address (T-1663). `/images/logo.png?token=a%20b c` was resolved as `?token=a%2520b%20c`, so the image failed to load and the link opened the wrong page: the already-escaped `%20` was escaped a second time because the whole query was treated as though none of it had been escaped yet. This affected both site-root addresses (starting with `/`) and the far more common document-relative form (`images/logo.png?token=a%20b c`); both are fixed. Query and anchor are now escaped the same way absolute URLs already were (T-1624) — an existing escape is left alone and only genuinely unescaped characters are encoded, so an escaped `%26` stays a literal character instead of decoding into a parameter separator and requesting a different resource.

diff --git a/specs/open-from-url/design.md b/specs/open-from-url/design.mdindex bae23ff..66c2b3f 100644--- a/specs/open-from-url/design.md+++ b/specs/open-from-url/design.md@@ -552,22 +552,34 @@ if case .url(let remote, _) = session.source { } ``` -The `refreshFromURL` method re-downloads content and calls `session.reloadContent(markdownString:)`:+The refresh itself is owned by `RemoteRefreshFlow` (`prism/ViewModels/`), which+`DocumentReaderView` holds as `@State`; the button delegates to it and cancels it+from `.onDisappear`:  ```swift-private func refreshFromURL(_ url: URL) {-    Task {-        do {-            let result = try await URLDocumentLoader.load(from: url)-            await session.reloadContent(markdownString: result.content)-        } catch {-            // Show error but retain existing content (Req 5.5)-            refreshError = error.localizedDescription-        }-    }+Button {+    refreshFlow.refresh(session: session, recentFilesManager: recentFilesManager)+} label: {+    Image(systemName: "arrow.clockwise") }+.disabled(refreshFlow.isRefreshing) ``` +`RemoteRefreshFlow.refresh` re-downloads content, calls+`session.reloadContent(markdownString:)`, and updates the cached recents title. It+follows the task-tracking + monotonic-epoch pattern `RemoteContentCoordinator`+uses for the initial open (T-862): a new refresh cancels the in-flight one, and a+request id captured at start is re-checked after every suspension before applying+the reload, writing the recents title, or clearing `isRefreshing`.++> **T-1805.** This was originally a private `refreshFromURL()` on+> `DocumentReaderView` that fired an untracked `Task` with no staleness check, so a+> refresh could outlive the document and overwrite that URL's recents title after+> the user had moved on. Note that `.onDisappear { refreshFlow.cancel() }` is the+> PRIMARY mechanism for the document-close case, not defence in depth: the epoch+> only advances when `refresh()` or `cancel()` is called, and closing a document+> calls neither.+ The `reloadContent(markdownString:)` method (defined in Section 4) updates `content`, resets expansion state, and re-parses blocks. Notes are preserved since the document identifier hasn't changed.  ---@@ -593,6 +605,7 @@ While downloading, the app needs a loading indicator. Two approaches: | `prism/Services/URLDocumentLoader.swift` | Enum | Download, validate, and return markdown content (streaming) | | `prism/Services/GitHubURLTransformer.swift` | Enum | GitHub blob → raw URL transformation | | `prism/Views/URLInputSheet.swift` | View | URL input dialog with paste and validation |+| `prism/ViewModels/RemoteRefreshFlow.swift` | Class | Owns the URL-refresh task: cancellation + monotonic-epoch staleness guard (added T-1805) |  ### Modified Files 

diff --git a/specs/open-from-url/decision_log.md b/specs/open-from-url/decision_log.mdindex 782aec8..790461a 100644--- a/specs/open-from-url/decision_log.md+++ b/specs/open-from-url/decision_log.md@@ -1,5 +1,11 @@ # Decision Log: Open File from URL +## Quick Decisions++| ID | Date | Decision | Rationale |+|----|------|----------|-----------|+| Q1 | 2026-08-17 | Move the URL refresh out of `DocumentReaderView` into `RemoteRefreshFlow` (`prism/ViewModels/`) | A view struct cannot own a cancellable task lifetime, which is what T-1805 needed. Applies the task-tracking + monotonic-epoch pattern already used by `RemoteContentCoordinator` (T-862) and the `@State`-owned flow shape of `ClipboardSaveFlow` — convention application, no contested alternative. |+ ## Decision 1: Use Display URL for Identity, Fetch URL for Downloading  **Date**: 2026-03-11

diff --git a/docs/agent-notes/open-from-url.md b/docs/agent-notes/open-from-url.mdindex ff019b7..b09e971 100644--- a/docs/agent-notes/open-from-url.md+++ b/docs/agent-notes/open-from-url.md@@ -35,11 +35,14 @@ The feature adds a fourth document source (`.url(remote:display:)`) alongside `.  ### DocumentReaderView - Refresh toolbar button (`arrow.clockwise`) visible only for `.url` sources-- `refreshFromURL()` re-downloads via `URLDocumentLoader.load`, calls `session.reloadContent(markdownString:)` - Error alert retains existing content on failure (Req 5.5) - Note loading handles `.url(_, let displayURL)` case - Title update for URL recent entries after parsing +`RemoteRefreshFlow` (`prism/ViewModels/`) owns the refresh, held by `DocumentReaderView` as `@State`. It mirrors `RemoteContentCoordinator`'s pattern above: `refresh(session:recentFilesManager:)` cancels any in-flight request, bumps a monotonic `currentRequestID`, and re-checks that id after every suspension before applying the reload, writing the recents title, or clearing `isRefreshing`. Loader is injectable, and `task` is `private(set)` so tests join on the exact request (`await handle?.value`) instead of sleeping. Extracted from the old inline `DocumentReaderView.refreshFromURL()` in T-1805.++The non-obvious bit: the epoch only advances when `refresh()` or `cancel()` is called on the instance. For the SUPERSEDE case that is self-sufficient. For the CLOSE case — the actual T-1805 bug — nothing bumps it on its own, so `DocumentReaderView`'s `.onDisappear { refreshFlow.cancel() }` is the primary mechanism, not defence in depth; removing it reinstates the stale recents-title write. The task's `[weak self]` capture is the backstop if SwiftUI ever skips `.onDisappear`. The wiring is pinned by `RemoteRefreshTeardownWiringTests` (source-structural, like `FootnotePresentationHostTests`).+ ### RecentFilesManager - `addRemoteURLRecentFile(displayURL:)` creates URL-based entries - `updateTitle(forRemoteURL:title:)` matches by URL string (separate from file path matching)

diff --git a/docs/agent-notes/document-reader.md b/docs/agent-notes/document-reader.mdindex d8ad68f..a2de95c 100644--- a/docs/agent-notes/document-reader.md+++ b/docs/agent-notes/document-reader.md@@ -1,18 +1,10 @@ # Document Reader -## Reload and Refresh Do Not Resync NotesManager+## Reload and Refresh Resync NotesManager (RESOLVED — T-987) -`DocumentReaderView` performs its initial parse and note loading inside `.task(id: session.id)`. That means the note pipeline runs when a session is first shown, but not when the same session later reparses new content.+Historical: note loading used to hang off `.task(id: session.id)`, so it ran when a session was first shown but not when the same session reparsed new content. Both in-place reload paths — `DocumentLayoutCoordinator.reloadDocument(session:)` for file-backed documents and `RemoteRefreshFlow.refresh(session:recentFilesManager:)` (was `DocumentReaderView.refreshFromURL()`, extracted in T-1805) for remote ones — left persisted notes un-relocated and imported comment notes stale. -Two existing reload paths update `DocumentSession` without reloading `NotesManager`:-- `DocumentLayoutCoordinator.reloadDocument(session:)` calls `session.reloadContent(from:)` for file-backed documents.-- `DocumentReaderView.refreshFromURL()` calls `session.reloadContent(markdownString:)` for remote documents.--After either path runs, `session.parsedBlocks`, `pendingImportedNotes`, `pendingTaggedRanges`, and `content` are updated, but `notesManager.loadNotes(...)` and `notesManager.loadImportedNotes(...)` are not called again. Effects:-- persisted notes are not re-relocated against the new block IDs/content-- imported comment notes can stay stale until the document is reopened--Any fix should add a single resync path after reload/refresh and cover both persisted and imported notes with a regression test.+Fixed by T-987: notes loading is keyed on `session.parseRevision`, which bumps on every successful parse, so both reload paths re-relocate persisted notes and rebuild imported ones. See the comment on the `.task(id:)` in `DocumentReaderView`. Kept here only so the old symptom isn't re-investigated.  ## Raw Source Width Uses Default Font Metrics