Pre-push review: T-1757 — replacing a document orphans macOS media windows

diff --git a/prism/ViewModels/DocumentFlowCoordinator.swift b/prism/ViewModels/DocumentFlowCoordinator.swiftindex dfa9fbf..123af43 100644--- a/prism/ViewModels/DocumentFlowCoordinator.swift+++ b/prism/ViewModels/DocumentFlowCoordinator.swift@@ -676,6 +676,40 @@ final class DocumentFlowCoordinator {         // not lose the position. No-op when there is no outgoing session or         // for clipboard sessions.         currentSession?.persistScrollPosition()+        #if os(macOS)+        // Close any image/diagram windows the outgoing session owns before it+        // is replaced. Every activation path (open, paste, bundled, remote)+        // routed through here previously skipped this teardown — only an+        // explicit closeDocument() performed it — so replacing a document left+        // its media windows on screen, now belonging to a document that is no+        // longer open (T-1757). Uses the same `diagramOwnerURL` key+        // closeDocument() uses so file and clipboard sessions are both covered.+        //+        // Skipped when the incoming session denotes the SAME document, because+        // a reopen is not a replacement. `diagramOwnerURL` is exactly the key+        // `closeWindows(ownedBy:)` matches against, so equality here means the+        // incoming session's windows register under the same owner — they are+        // adopted by the new session rather than orphaned, and its eventual+        // close still tears them down. Without this guard, reopening the+        // already-open document (Recent Files, re-dropping the same file)+        // would destroy the user's open diagram/image windows for no reason,+        // and would contradict the in-place reload paths (remote Refresh,+        // FileChangeObserver) which keep those windows across new content.+        //+        // The comparison is safe for sessions with no backing URL:+        // `diagramOwnerURL` is non-optional and synthesises a per-session+        // `prism://clipboard/{id}` for clipboard (and resource-less bundled)+        // sessions, so two such sessions never compare equal — pasting new+        // clipboard content is still treated as a replacement. Any identity+        // mismatch (e.g. `/tmp` vs `/private/tmp`) fails closed into the+        // teardown branch, which is the safe direction: it can close windows+        // the user would rather have kept, but never strands any.+        if let outgoingOwnerURL = currentSession?.diagramOwnerURL,+           outgoingOwnerURL != session.diagramOwnerURL {+            imageServices?.imageDetailWindowManager.closeWindows(ownedBy: outgoingOwnerURL)+            diagramWindowManager?.closeWindows(ownedBy: outgoingOwnerURL)+        }+        #endif         currentSession = session         navigationPath = NavigationPath()         navigationPath.append(session.id)

diff --git a/prismTests/DocumentFlowCoordinatorReplaceWindowCleanupTests.swift b/prismTests/DocumentFlowCoordinatorReplaceWindowCleanupTests.swiftnew file mode 100644index 0000000..904ce86--- /dev/null+++ b/prismTests/DocumentFlowCoordinatorReplaceWindowCleanupTests.swift@@ -0,0 +1,324 @@+//+//  DocumentFlowCoordinatorReplaceWindowCleanupTests.swift+//  prismTests+//+//  T-1757: Replacing a document leaves macOS media windows orphaned.+//+//  activateSession(...) — the shared installer behind openFile, pasteFromClipboard,+//  openBundledDocument, and openRemoteSession — replaced `currentSession` without+//  the image/diagram window teardown that closeDocument() performs for the+//  outgoing session. Only an explicit close tore those windows down, so+//  replacing a document (without going through requestClose/closeDocument+//  first) left its media windows on screen, now belonging to a document that+//  was no longer open.+//+//  These tests exercise activation paths directly (bypassing the unsaved-content+//  confirmation gate) to prove the outgoing session's image and diagram windows+//  close when a new document is activated.+//+//  They also pin the boundary of that teardown, which is document identity —+//  `diagramOwnerURL`, the exact key `closeWindows(ownedBy:)` matches on — and+//  not object identity:+//  - Reopening the document that is ALREADY open (Recent Files, re-dropping the+//    same file, refetching the same remote URL) installs a new session object+//    but is not a replacement. Its windows register under the same owner, so+//    they are adopted rather than orphaned and must survive.+//  - A genuinely different document is a replacement and must close them.+//  - Two sessions with no backing file (successive clipboard pastes) are NOT+//    the same document: `diagramOwnerURL` synthesises a per-session URL rather+//    than collapsing to a shared nil, so a paste still replaces.+//++#if os(macOS)+import AppKit+import Foundation+import Testing+@testable import prism++@Suite("DocumentFlowCoordinator replace-document window cleanup", .serialized)+@MainActor+struct DocumentFlowCoordinatorReplaceWindowCleanupTests {++    // MARK: - Helpers++    private func makeImageServices(_ manager: ImageDetailWindowManager) -> ImageServices {+        ImageServices(+            imageCache: ImageCache(),+            imageLoader: ImageLoader(),+            snapshotCache: SnapshotCache(),+            svgRenderer: SVGRenderer(),+            svgSourceLoader: SVGSourceLoader(),+            directoryAccessManager: DirectoryAccessManager(),+            diagramCache: DiagramCache(),+            imageDetailWindowManager: manager+        )+    }++    /// `isReleasedWhenClosed = false` prevents NSWindow's default auto-release+    /// on `close()` from over-releasing the local test reference.+    private func makeWindow() -> NSWindow {+        let window = NSWindow(+            contentRect: CGRect(x: 0, y: 0, width: 400, height: 300),+            styleMask: [.titled, .closable, .resizable],+            backing: .buffered,+            defer: true+        )+        window.isReleasedWhenClosed = false+        return window+    }++    private func writeTempMarkdownFile(named name: String) throws -> URL {+        let url = FileManager.default.temporaryDirectory.appendingPathComponent(name)+        try "# Incoming".write(to: url, atomically: true, encoding: .utf8)+        return url+    }++    // MARK: - openFile replaces a file-owned session++    @Test("openFile closes image and diagram windows owned by the outgoing file session")+    func openFileClosesOutgoingFileSessionWindows() async throws {+        let imageManager = ImageDetailWindowManager()+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(imageManager))+        flow.setDiagramWindowManager(diagramManager)++        let outgoingURL = URL(fileURLWithPath: "/tmp/outgoing-T1757-file.md")+        let outgoingSession = DocumentSession(url: outgoingURL, content: "# Outgoing\n\n![x](img.png)")+        flow.currentSession = outgoingSession++        let imageIdentifier = ImageDetailIdentifier(documentURL: outgoingURL, imageSource: "img.png")+        let imageWindow = makeWindow()+        imageManager.registerWindow(imageWindow, for: imageIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        let diagramIdentifier = DiagramIdentifier(documentURL: outgoingURL, byteOffset: 10)+        let diagramWindow = makeWindow()+        diagramManager.registerWindow(diagramWindow, for: diagramIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        #expect(imageManager.openWindowCount == 1)+        #expect(diagramManager.openWindowCount == 1)++        let newURL = try writeTempMarkdownFile(named: "incoming-T1757-\(UUID().uuidString).md")+        defer { try? FileManager.default.removeItem(at: newURL) }++        flow.openFile(at: newURL)++        // close() schedules unregistration via the willClose observer on the+        // main queue; yield so the run loop drains before asserting.+        try await Task.sleep(nanoseconds: 200_000_000)++        #expect(imageManager.openWindowCount == 0,+                "Image windows owned by the replaced document must close when a new document is opened")+        #expect(diagramManager.openWindowCount == 0,+                "Diagram windows owned by the replaced document must close when a new document is opened")+        #expect(flow.currentSession?.source.url == newURL)+    }++    // MARK: - pasteFromClipboard replaces a clipboard-owned session++    @Test("pasteFromClipboard closes image and diagram windows owned by the outgoing clipboard session")+    func pasteFromClipboardClosesOutgoingClipboardSessionWindows() async throws {+        let imageManager = ImageDetailWindowManager()+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(imageManager))+        flow.setDiagramWindowManager(diagramManager)++        // Outgoing session is itself a clipboard paste — source.url is nil, so+        // the fix must key off diagramOwnerURL (the synthetic+        // prism://clipboard/{id} URL), not source.url, to find its windows.+        //+        // This also pins the negative half of the same-document guard: BOTH+        // sessions here have a nil `source.url`, and they are still two+        // different documents. Because `diagramOwnerURL` synthesises a+        // per-session URL rather than collapsing to a shared nil, the guard+        // cannot mistake one paste for a reopen of the previous one.+        let outgoingSession = DocumentSession(clipboardContent: "# Outgoing pasted\n\n![x](img.png)")+        flow.currentSession = outgoingSession+        let outgoingOwnerURL = outgoingSession.diagramOwnerURL+        #expect(outgoingSession.source.url == nil)++        let imageIdentifier = ImageDetailIdentifier(documentURL: nil, imageSource: "img.png")+        let imageWindow = makeWindow()+        imageManager.registerWindow(imageWindow, for: imageIdentifier, ownedBy: outgoingOwnerURL)++        let diagramIdentifier = DiagramIdentifier(documentURL: nil, byteOffset: 3)+        let diagramWindow = makeWindow()+        diagramManager.registerWindow(diagramWindow, for: diagramIdentifier, ownedBy: outgoingOwnerURL)++        #expect(imageManager.openWindowCount == 1)+        #expect(diagramManager.openWindowCount == 1)++        NSPasteboard.general.clearContents()+        NSPasteboard.general.setString("# New pasted content", forType: .string)++        flow.pasteFromClipboard()++        try await Task.sleep(nanoseconds: 200_000_000)++        #expect(imageManager.openWindowCount == 0,+                "Image windows owned by the replaced clipboard document must close on paste")+        #expect(diagramManager.openWindowCount == 0,+                "Diagram windows owned by the replaced clipboard document must close on paste")+        #expect(flow.currentSession?.source == .clipboard)+    }++    // MARK: - Reopening the SAME document is not a replacement++    @Test("openFile reopening the already-open document keeps its media windows open")+    func openFileReopeningSameDocumentKeepsItsMediaWindowsOpen() async throws {+        let imageManager = ImageDetailWindowManager()+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(imageManager))+        flow.setDiagramWindowManager(diagramManager)++        // The file must exist on disk: openFile reads it, and it has to be the+        // very same URL the outgoing session was built from so that both+        // sessions resolve to the same `diagramOwnerURL`.+        let sharedURL = try writeTempMarkdownFile(named: "same-doc-T1757-\(UUID().uuidString).md")+        defer { try? FileManager.default.removeItem(at: sharedURL) }++        let outgoingSession = DocumentSession(url: sharedURL, content: "# Outgoing\n\n![x](img.png)")+        flow.currentSession = outgoingSession++        let imageIdentifier = ImageDetailIdentifier(documentURL: sharedURL, imageSource: "img.png")+        let imageWindow = makeWindow()+        imageManager.registerWindow(imageWindow, for: imageIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        let diagramIdentifier = DiagramIdentifier(documentURL: sharedURL, byteOffset: 10)+        let diagramWindow = makeWindow()+        diagramManager.registerWindow(diagramWindow, for: diagramIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        #expect(imageManager.openWindowCount == 1)+        #expect(diagramManager.openWindowCount == 1)++        // Reopening the current document (Recent Files, re-dropping the file,+        // re-importing the same path) still builds a brand new session object.+        flow.openFile(at: sharedURL)++        try await Task.sleep(nanoseconds: 200_000_000)++        let incomingSession = try #require(flow.currentSession)+        #expect(incomingSession !== outgoingSession,+                "A reopen installs a new session object — the guard must be about document identity, not object identity")+        #expect(incomingSession.diagramOwnerURL == outgoingSession.diagramOwnerURL,+                "Same document ⇒ same owner key, so the windows are adopted by the new session rather than orphaned")++        #expect(imageManager.openWindowCount == 1,+                "Reopening the already-open document must not destroy its image windows — nothing was replaced")+        #expect(diagramManager.openWindowCount == 1,+                "Reopening the already-open document must not destroy its diagram windows — nothing was replaced")+        #expect(imageManager.isOpen(imageIdentifier))+        #expect(diagramManager.isOpen(diagramIdentifier))+        // The surviving windows are still reachable under the new session's key,+        // so its eventual close (or a real replacement) still tears them down.+        #expect(diagramManager.identifiers(ownedBy: incomingSession.diagramOwnerURL).contains(diagramIdentifier))++        imageWindow.close()+        diagramWindow.close()+        imageManager.clearAllTracking()+        diagramManager.clearAllTracking()+    }++    @Test("openRemoteSession reopening the same remote URL keeps its media windows open")+    func openRemoteSessionReopeningSameURLKeepsItsMediaWindowsOpen() async throws {+        let imageManager = ImageDetailWindowManager()+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(imageManager))+        flow.setDiagramWindowManager(diagramManager)++        // Remote sessions key ownership on the DISPLAY URL, so a Recent Files+        // reopen of the same remote document must be recognised as the same+        // document even though the fetch happens again.+        let fetchURL = URL(string: "https://raw.githubusercontent.com/o/r/main/doc.md")!+        let displayURL = URL(string: "https://github.com/o/r/blob/main/doc.md")!++        let outgoingSession = DocumentSession(remoteURL: fetchURL, displayURL: displayURL, content: "# Outgoing remote")+        flow.currentSession = outgoingSession++        let diagramIdentifier = DiagramIdentifier(documentURL: displayURL, byteOffset: 7)+        let diagramWindow = makeWindow()+        diagramManager.registerWindow(diagramWindow, for: diagramIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        #expect(diagramManager.openWindowCount == 1)++        flow.openRemoteSession(fetchURL: fetchURL, displayURL: displayURL, content: "# Refetched remote")++        try await Task.sleep(nanoseconds: 200_000_000)++        #expect(diagramManager.openWindowCount == 1,+                "Reopening the same remote URL must not destroy its diagram windows")+        #expect(flow.currentSession?.diagramOwnerURL == displayURL)++        diagramWindow.close()+        diagramManager.clearAllTracking()+    }++    @Test("openRemoteSession replacing with a different remote URL still closes the outgoing windows")+    func openRemoteSessionReplacingWithDifferentURLClosesOutgoingWindows() async throws {+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(ImageDetailWindowManager()))+        flow.setDiagramWindowManager(diagramManager)++        let outgoingDisplayURL = URL(string: "https://github.com/o/r/blob/main/outgoing.md")!+        let outgoingSession = DocumentSession(+            remoteURL: outgoingDisplayURL,+            displayURL: outgoingDisplayURL,+            content: "# Outgoing remote"+        )+        flow.currentSession = outgoingSession++        let diagramIdentifier = DiagramIdentifier(documentURL: outgoingDisplayURL, byteOffset: 7)+        let diagramWindow = makeWindow()+        diagramManager.registerWindow(diagramWindow, for: diagramIdentifier, ownedBy: outgoingSession.diagramOwnerURL)++        #expect(diagramManager.openWindowCount == 1)++        let incomingDisplayURL = URL(string: "https://github.com/o/r/blob/main/incoming.md")!+        flow.openRemoteSession(+            fetchURL: incomingDisplayURL,+            displayURL: incomingDisplayURL,+            content: "# Incoming remote"+        )++        try await Task.sleep(nanoseconds: 200_000_000)++        #expect(diagramManager.openWindowCount == 0,+                "A genuinely different remote document is a replacement — the outgoing windows must close")+    }++    // MARK: - Windows owned by an unrelated document are untouched++    @Test("openFile leaves windows owned by an unrelated document open")+    func openFileLeavesUnrelatedDocumentWindowsOpen() async throws {+        let imageManager = ImageDetailWindowManager()+        let diagramManager = DiagramWindowManager()+        let flow = DocumentFlowCoordinator()+        flow.setImageServicesForTests(makeImageServices(imageManager))+        flow.setDiagramWindowManager(diagramManager)++        let currentURL = URL(fileURLWithPath: "/tmp/current-T1757.md")+        flow.currentSession = DocumentSession(url: currentURL, content: "# Current")++        let otherURL = URL(fileURLWithPath: "/tmp/other-T1757.md")+        let otherIdentifier = DiagramIdentifier(documentURL: otherURL, byteOffset: 1)+        let otherWindow = makeWindow()+        diagramManager.registerWindow(otherWindow, for: otherIdentifier, ownedBy: otherURL)++        let newURL = try writeTempMarkdownFile(named: "incoming-other-T1757-\(UUID().uuidString).md")+        defer { try? FileManager.default.removeItem(at: newURL) }++        flow.openFile(at: newURL)++        try await Task.sleep(nanoseconds: 200_000_000)++        #expect(diagramManager.isOpen(otherIdentifier))+        #expect(diagramManager.identifiers(ownedBy: otherURL).contains(otherIdentifier))++        diagramManager.clearAllTracking()+    }+}+#endif

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex e2ecf43..9609547 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -20,7 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0  ### Fixed -- A note imported from a document, anchored to a nested list item (a sub-item under a top-level list item), now shows its quoted text and the section it belongs to (T-1871). In the notes pane it appeared with a blank quote and no heading, and it could not be told apart from any other nested-item note in the document. The note itself was always attached to the correct item — only the surrounding context was missing, and it was missing every time the document was opened, not just on a reload. Imported notes are rebuilt from the document on each open, and that rebuild only recognised top-level list-item identifiers, so a nested item's identifier was never matched and the context came back empty. Nested identifiers are now recognised the same way every other part of the app that addresses list items already recognises them, so an imported note on a nested item gets the same context as one on a top-level item. - 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.@@ -92,6 +91,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - macOS: image detail windows opened from a pasted (clipboard) document now close when that document is closed, instead of being left open as orphaned windows. - Notes: rapidly reloading the same document no longer lets an older reload back up or persist notes state that a newer reload has already replaced. The load-generation guard is now re-checked after the backup step, before the state is saved. - Opening a file from the Recents list while a pasted (clipboard) document has unsaved content now shows the Unsaved Changes confirmation instead of silently replacing the unsaved document. Previously only bundled and URL-sourced recents were guarded; regular file recents skipped the dialog. Save and Discard both continue on to open the chosen recent file (T-1630).+- On the Mac, opening a document while another one is already open now closes the image and diagram windows belonging to the document you left, instead of stranding them (T-1757). Those windows were only ever cleared away by closing a document explicitly; opening a replacement — from the file picker, Recent Files, a paste, the bundled guides, or a URL — left them on screen belonging to a document that was no longer open, with no way left to clear them short of closing each one by hand. Reopening the document you are already reading is not a replacement and leaves its windows exactly where they are, so picking the current document again from Recent Files still costs you nothing; refreshing a URL document and reloading a file changed on disk keep those windows too, as they always have. Two pasted documents are never treated as the same document, so pasting fresh content still clears the previous paste's windows. - HTML comments embedded in paragraph text (and in headings, list items, and table cells) render again as dimmed inline annotations when "Show HTML comments" is on — they were silently dropped by the new rendering engine (T-1638). Stand-alone comments nested inside blockquotes or list items now get the same toggleable annotation treatment instead of leaking as always-visible plain text, and comment annotations regained their info indicator glyph. Search now stays aligned with what is actually rendered: comment shapes that are never displayed (conditional comments, note-infrastructure tags) no longer count as matches when the toggle is on, and hidden comment text nested inside blockquotes or list items no longer produces phantom highlights.  ### Security