Round-6 (final gate) review of PR #418 — git diff origin/main...HEAD, rebased on 693af8ff. Five production files, one new 30-test suite, one bugfix report.
FileChangeObserver now implements presentedItemDidMove(to:) under a Mutex, retargeting before any actor hop; DocumentSession.source follows, and every URL-derived feature (title, Save destination, Reload, scroll key, page image context) follows for free.migrateNotes are true of the code: the memory-only entry guard (NotesManager.swift:1313), the clipboard-vs-file identity assumption, and the flip-then-migrate ordering.retiringSourceRecordIsSafe is correct and is genuine reuse. NotesStore.load has no in-memory cache and decodes the stored identifier, so on a case-insensitive (or normalisation-insensitive APFS) volume reading the old identifier back really does return the record just written — the guard adapts to the volume instead of guessing at its collation.accommodatePresentedItemDeletion) was wrong; the measurement table and the isTrashed guard that follows from it match the code. The macOS measurement has no committed artifact, and the iOS limit is stated.startMigration(forMovedFileFrom:toFileURL:sessionID:) (no such parameter) and claims migrateNotes is "now private" (it is internal and still called by ClipboardSaveFlow).Ready to push
Ready to push, with two named follow-ups. The round-5 redesign holds up under verification: the move path's own migration, the store-asking delete guard, the synchronous container rebind, the initial: true store fallback and the measured trash guard all do what they claim, and I traced each against the code rather than the prose. 72/72 targeted tests pass across all four locale configurations; make lint, make verify-test-isolation and make verify-make-guards are clean.
Two majors are outstanding and neither blocks the merge. (1) A second external move consumed while the first migration is still in flight migrates from an identifier the first migration has already emptied, and reports success — the ticket's own symptom, one interleaving over. It is not a regression: on main the very first move loses the notes unconditionally, so this is a strictly narrower version of the bug being fixed, and it needs two external moves inside one migration's duration. The cheap hardening is a one-line change in DocumentReaderView (derive the origin from session.notesRecordURL at consumption instead of trusting the snapshot in move.from), which no existing test contradicts. (2) The new CLAUDE.md section describes the round-1/3 design that round 5 replaced, and asserts an API signature and an access level that do not exist. That is a text-only fix, but CLAUDE.md is this repo's canonical agent contract, so it should land before merge.
Holding a fix that eliminates the 100% case in order to avoid a narrow timing variant of it would leave users strictly worse off. Merge, then file the race.
165ec31c Fix T-1881: open documents do not follow external file moves 6ec88d00 Fix T-1881 review: claim the moved-to identity before the migration task runs 8b136a15 Fix T-1881 review round 3: coalesce consecutive moves, correct the claim boundary e976ed78 Fix T-1881 review round 5: give the move its own notes migration 2dc3c6dc Fix T-1881: retire the old notes record even if the reader moved on Prism keeps a document open while you read it. If you renamed or moved that file in Finder or the Files app, Prism did not notice: it kept pointing at the old location. Reload failed on a file that was perfectly readable, the window title kept the old name, images stored next to the document were looked for in the old folder, and — worst of all — your notes were left filed under a name nothing would ever look up again.
Prism watches the open file through a system object called a file presenter. That object has a method the system calls when the file moves. Prism had never implemented it. This change implements it, and then wires the move all the way through the app.
Almost everything about an open document is derived from one property — DocumentSession.source, the URL of the file. Move that one property and the title, the Save destination, Reload, the remembered scroll position and the images all follow for free. Only the notes needed real work, because notes are saved to disk in a file named after the document's path, so they have to be physically moved.
Three things the change is careful about: renaming readme.md to README.md is one file on a Mac or iPhone disk, so writing the new record and then deleting the old one would delete the file just written; putting a document in the Trash arrives as a move, so Prism now refuses to follow moves into the Trash; and if the notes cannot be written to the new location you now get an alert instead of silence.
The change is a chain with one hop per layer, and each hop is deliberately narrow.
FileChangeObserver (an NSFilePresenter) implements presentedItemDidMove(to:). Its presented URL moves from a let to a Mutex<URL> because presentedItemURL is read off-actor by the file-coordination machinery; the retarget happens under the lock before any actor hop, so the presenter never answers with a location the coordinator has already superseded. It then reports (from, to) through a new onMove closure — in place when already on the main thread (which presentedItemOperationQueue = .main makes the normal case), because two Task { @MainActor } hops are scheduled, not ordered, and a reversed A→B / B→C pair leaves the session at B while the file is at C.DocumentSession.followFileMove(to:) moves source and publishes pendingFileMove. It refuses a destination inside .Trash/.Trashes, and it derives the move's origin from a new property, notesRecordURL, so consecutive moves coalesce to (origin, latest) and a round trip coalesces to nothing.DocumentReaderView consumes pendingFileMove with .onChange(of:initial:), mirroring the existing pendingSave hook, and calls NotesManager.startMigration(forMovedFileFrom:toFileURL:).DocumentFlowCoordinator.handleMoveMigrationFailed(session:notesRemainAt:) raises the same "Note Migration Failed" alert Save As uses, guarded by currentSession === session so a migration that outlives a document switch cannot alert over a different document.The central design decision (review round 5) is that the move path gets its own notes migration rather than reusing the clipboard Save As one. The Save As migration holds three preconditions a rename does not satisfy:
guard var notes = documentNotes else { return true } — memory only. A rename's record is on disk, and "nothing loaded" is the normal state on the initial: true pass, so the borrowed function reported success having moved nothing.readme.md and README.md are two identifiers and one file on a case-insensitive volume, so a string-inequality delete guard removed the record it had just written.session.source has not flipped yet. A move flips it first, so a note created inside the migration window resolves the target, finds a container still filed under the source, and clearNoteState() empties the pane.The answers: read the store when memory has nothing; replace the delete guard with retiringSourceRecordIsSafe, which asks the store whether reading the old identifier back returns the record just written (so it is right for any filesystem aliasing, including Unicode normalisation, not just case); and rebind the loaded container onto the target synchronously inside startMigration, before it returns, because a migration claim cannot help a creation — the container is the thing being judged.
retiringSourceRecordIsSafe costs one extra store read per migration, at rename frequency. It also now guards the existing Save As delete, which is a change to a pre-existing path. The flip-then-migrate order (source moves before the notes) is chosen over migrate-then-flip so the retarget is not conditional on a mounted reader view; the window it opens is closed from the consumption turn onward by the claim and the rebind, and argued unreachable before that.
Presenter. fileURL: Mutex<URL> with nonisolated var presentedItemURL. retarget(to:) returns the previous URL under the lock, or nil for a self-move, which makes "nothing changed" a single source of truth for both the presenter callback and the recordMove(to:) test seam. presentedItemDidMove is nonisolated and uses MainActor.assumeIsolated on the main thread with a Task { @MainActor } fallback — deliberately not assumeIsolated unconditionally, so a violated queue contract degrades to a late report rather than a trap. The class is @Observable @MainActor final, so it is Sendable and the Task capture is sound.
Session. notesRecordURL is the new fact: where the record actually is, as distinct from source (where the file is) and pendingFileMove (an unconsumed notification). It is set at the file initializer and at didSave, cleared at revertToClipboard, and advanced only by notesRecordFollowedFile(to:) on migration success. didSave's assignment is sound because ClipboardSaveFlow's failure path calls didSave(to: notesRemainAt), so every path reaching it has the record at the URL it is given.
Migration. startMigration does three things synchronously: an early return when the two identifiers resolve equal; beginMigration on both source and target (target claimed against itself, so isMigratingAway retires a racing loadNotes(target) at its entry guard and migrationDestination(target) is a no-op redirect); and the rebind of documentNotes from source identity to target identity. Only the disk work is in the returned unstructured Task, whose defer releases both tickets on every exit including cancellation. migrateNotesForMovedFile is private so the identity work cannot be skipped by calling it directly.
notesToMove returns the live container when it already carries the target identity (ownsMemory: true), otherwise falls back to store.load(source) and re-reads across that suspension, unioning by note id so a creation that established the target container mid-load is not dropped. The disk-sourced container is deliberately not published, because the reader's own loadNotes relocates block ids against current blocks before showing anything.
retiringSourceRecordIsSafe reads store.load(source) and refuses the delete when the returned record's identifier equals the target. NotesStore.load has no in-memory cache and reads the file, so on a case-insensitive (or normalisation-insensitive APFS) volume the two spellings genuinely resolve to one file and the guard fires. The extra load also triggers the legacy-encoding migration path, whose result is then correctly retired.isTrashed checks path components rather than FileManager.url(for: .trashDirectory), covering per-volume /Volumes/X/.Trashes/<uid> and provider-container trashes. Note that the presenter retargets before the session refuses, so after a trash observer.presentedItemURL != session.source; it self-heals on Put Back because the return move is a session no-op.initial: true pass takes the claim before the freshly-mounted view's .task(id: parseRevision) load reaches its entry guard, so that load is retired and documentNotes stays nil. The record is correct on disk; the pane is empty until a reload, a reopen, or a note creation (noteContainer consults the store). Non-deterministic, display-only.FileChangeObserver.swift
Why it matters. This is the root cause. The presenter never implemented presentedItemDidMove(to:), so it kept answering with the old URL and the coordinator stopped matching it against operations on the file. Everything downstream is a consequence.
What to look at. FileChangeObserver.swift:47-58 (Mutex<URL>), :132-157 (presentedItemDidMove + retarget)
DocumentSession.swift
Why it matters. Retargeting one property is the whole fix for the title, Save destination, Reload, scroll key and the page's image context. The second property, notesRecordURL, is the round-5 answer to a defect where a failed migration left the next move naming a path the record had never reached.
What to look at. DocumentSession.swift:151-225 (FileMove, notesRecordURL, pendingFileMove), :483-502 (followFileMove, notesRecordFollowedFile)
NotesManager.swift
Why it matters. This is the substance of round 5 and the reason the PR exists in its current shape. Three preconditions of migrateNotes hold for a clipboard Save As and not for a rename, and each cost the user their notes.
What to look at. NotesManager.swift:1469-1740 (startMigration, migrateNotesForMovedFile, notesToMove, retiringSourceRecordIsSafe)
NotesManager.swift
Why it matters. It is the fix for a total-loss defect on an ordinary rename (readme.md → README.md), and it now also guards the pre-existing Save As delete — a change to a path this PR is not otherwise about.
What to look at. NotesManager.swift:1714-1740; called at :1438 (Save As) and :1658 (move)
DocumentSession.swift
Why it matters. Rounds 1-4 rested on a claim recorded with no source — that a Finder trash arrives as accommodatePresentedItemDeletion rather than a move. It was measured this round and is false. Without the guard, trashing an open document rewrote the notes under a ~/.Trash path and deleted the record at the real path.
What to look at. DocumentSession.swift:505-538 (isTrashed), report.md:285-320 (measurement table)
DocumentReaderView.swift
Why it matters. The whole notes half of the fix hangs off one .onChange, which no unit test can mount. T-1943 is the precedent for what happens when production wiring is only ever exercised by direct invocation.
What to look at. DocumentReaderView.swift:386-401; pinned by ExternalFileMoveTests.swift:779-829
Verified against the code. All three preconditions cited are real: migrateNotes's memory-only entry guard at NotesManager.swift:1313, notesBelongToSaveChain accepting only the clipboard and previousDestination identities at :1234, and ClipboardSaveFlow.swift:213 calling didSave only after the migration returns. The duplication that results is justified; see the finding about extracting the shared claim/rebind skeleton.
The opposite order from Save As, and deliberately so: a move has already happened by the time the app is told, so leaving source on the old path for the length of an iCloud write means a Reload in that window reads a file that is not there — and it would make the retarget conditional on a mounted reader view running an async migration. The window this opens is closed from the consumption turn onward by the claim plus the synchronous rebind, and argued unreachable before that (no parseRevision bump, no reload banner). I checked both halves of that argument and they hold today.
Correct in principle — the two facts are genuinely different. But FileMove.from is a snapshot of notesRecordURL taken at report time, so the separation is only half-achieved; see the major finding.
A deliberate divergence from the Save As migration, which abandons the rebind and the retirement together when memory has moved on. The argument is sound: the write landed, so the record under the source is a duplicate filed where the file no longer is, and leaving it is the orphan half of this ticket's symptom. Pinned by aReaderMovingOnMidMigrationDoesNotStrandTheOldRecord.
The lease argument is from the sandbox's documented model, not from observation, and the report says so — a URL synthesised from a move notification carries no scope of its own, so starting a lease on it gains nothing, while dropping the existing one is the only way to actually lose access. If a provider does revoke on rename, the symptom is a reload failure after the move and the fix is additive. Recents staleness is explicitly T-1842 / T-2172 / T-2173.
Not called out as a decision anywhere, but retiringSourceRecordIsSafe replaced sourceIdentifier != targetIdentifier at NotesManager.swift:1438, on a pre-existing path. I verified the behaviour is equivalent except in the aliasing case and when the source load fails (where the delete is now skipped, leaving an orphan rather than removing a record — and NotesStore.load's legacy-path fallback means a legacy-encoded record still resolves, so the skip is not reachable for a readable record). It deserves a line in CLAUDE.md; see the documentation finding.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| major | DocumentReaderView.swift:386-401 / NotesManager.swift:1542 / DocumentSession.swift:491 | A second external move consumed while the first migration is still in flight migrates from an identifier the first migration has already emptied, and reports success. Interleaving for A→B then B→C: move 1 is consumed, startMigration(A→B) saves under B and deletes A; move 2 is reported before the view's awaiting Task calls notesRecordFollowedFile(to: B), so followFileMove reads notesRecordURL — still A — and publishes FileMove(from: A, to: C); the .onChange body pass consumes it after migration 1's delete, so notesToMove finds documentNotes under B (not C) and store.load(A) returns nil, and migrateNotesForMovedFile returns true. End state: the record is on disk under B, session.source is at C, notesRecordURL claims C, and the next loadNotes(C) empties the pane. That is this ticket's own symptom, reported as success — the failure mode the :136 test's comment says the return value cannot be trusted for. A slightly earlier delivery gives the benign variant instead: records under both B and C, an orphan but no loss. Root cause: FileMove.from is a snapshot of notesRecordURL taken at REPORT time, so the design's own invariant ('from is where the notes record still is') is violated by any turn between the report and the consumption. | NOT A MERGE BLOCKER, and not a regression — on main the FIRST move loses the notes unconditionally, so this is a strictly narrower variant of the bug being fixed, needing two external moves inside one migration's duration. File it as a follow-up. Cheap hardening if the author wants it now: in DocumentReaderView's onChange, derive the origin live rather than trusting the snapshot — `guard let origin = session.notesRecordURL, origin != move.to else { return }` and pass `origin` to startMigration. No existing test contradicts that (every notes test calls startMigration directly, and the structural pin only requires the `notesManager.startMigration(` and `session.notesRecordFollowedFile(to: move.to)` spellings). The complete fix additionally serialises migrations by holding the in-flight Task, the way ClipboardSaveFlow is held in the same file. |
| major | CLAUDE.md:47-56 | The new 'External File Moves' section is round-1/3 prose that round 5 replaced, and it asserts things that are false of the shipped code. Line 51 says the notes are migrated 'through the same machinery a Save As uses' — the opposite of the decision this PR was ultimately made for. Line 53 names `NotesManager.startMigration(forMovedFileFrom:toFileURL:sessionID:)`; the real signature has no sessionID. Line 53 also says `migrateNotes` is 'now private'; it is internal at NotesManager.swift:1309 and still called from ClipboardSaveFlow.swift:171. Line 54 attributes origin-carrying to pendingFileMove and to notesBelongToSaveChain, but the shipped design derives it from notesRecordURL and the move path never enters notesBelongToSaveChain. Three things that DO belong are missing: the trash guard, notesRecordURL/notesRecordFollowedFile, and — most importantly — that retiringSourceRecordIsSafe also replaced the delete guard in the existing Save As migration. | Text-only, and worth doing before merge: CLAUDE.md is this repo's canonical agent contract, and a future session reading it would build against an API that does not exist. Rewrite the section against the shipped code. The same stale signatures appear at report.md:87-89 and report.md:438 (Affected Files), and in the failure message at ExternalFileMoveTests.swift:798. |
| minor | NotesManager.swift:1438 / prismTests | retiringSourceRecordIsSafe on the SAVE AS path is pinned by no test. caseOnlyRenameKeepsTheNotes drives only startMigration → migrateNotesForMovedFile:1658, so reverting line 1438 alone to `sourceIdentifier != targetIdentifier` fails nothing. ConsecutiveSaveAsTests and NotesManagerClipboardTests contain no case-differing previousDestination and no case-folding store. The report and the guard's own doc comment both assert that a supersession chain can hit the same collision. | Add a case-only previousDestination test on the Save As path, or drop the claim that that path is protected. Not blocking — the guard is strictly safer than what it replaced on both paths. |
| minor | NotesManager.swift:1552-1580 vs :1360-1378; :1647-1649 vs :1427-1429 | The claim/ticket skeleton (`[a, b].map { beginMigration(...) }` plus the `zip(...)` release loop), the three-line cached-identity rebind (cachedDocumentPath / cachedDocumentIdentifier / documentPath) and the six-line save-failure identity revert are each duplicated verbatim between the two migrations. The decision to fork the migration is justified, but the shared skeleton need not be — and this is code whose bugs delete user notes, so drift is the risk. | Extract `withMigrationClaims(_:to:displayName:body:)` and `adoptDocumentIdentity(_:displayName:)`. ~25 lines, and one place instead of two for the next T-2231-shaped fix. Follow-up, not blocking. |
| minor | DocumentReaderView.swift:386 / NotesManager.swift:1690-1710 | A move consumed on the `initial: true` pass takes the target claim before the freshly-mounted view's `.task(id: parseRevision)` load reaches its entry guard, so isMigratingAway retires that load and documentNotes stays nil. notesToMove's disk path deliberately does not publish, so the notes pane is empty for the rest of the session until a reload, a reopen, or a note creation (noteContainer consults the store, so no loss). The comment at NotesManager.swift:1706 says 'the reader's own loadNotes relocates the record before it shows anything' — which is exactly the load the claim retires. Non-deterministic: whether the load resumes inside or outside the claim window decides it. | Display-only and self-healing; the notes are correct on disk. Worth a re-load after a migration that owned no memory, or a note in the comment that the reader's load may have been retired. Follow-up. |
| minor | FileChangeObserver.swift:160 | recordMove(to:) is documented as 'the counterpart of recordExternalChange()', but recordExternalChange has a production caller (presentedItemDidChange:97) and recordMove has none — it is a test-only seam on the production API surface. The codebase's precedent for that is DocumentSession.didSave(to:fileAccessResourceForTests:), which is #if DEBUG. | Wrap in #if DEBUG, or fold into presentedItemDidMove's main-thread branch so it sits on a real path. |
| minor | prismTests/ExternalFileMoveTests.swift / report.md | Two behaviours added to DocumentSession are mutation-survivable: didSave's `notesRecordURL = url` and revertToClipboard's `pendingFileMove = nil` / `notesRecordURL = nil`. saveClearsAnUnconsumedMove asserts only pendingFileMove and source, so deleting the didSave assignment leaves notesRecordURL at the pre-save URL — the exact defect aFailedMigrationLeavesTheNextMoveReportingTheOrigin exists to prevent, one path over. Separately, the F4 measurement (the PR's load-bearing empirical claim) has no committed harness or log, and the two trash tests synthesise .Trash URLs by hand, so they pin isTrashed rather than F4. | Add the two session assertions; commit the F4 harness or its raw output under specs/bugfixes/follow-external-file-moves/. Follow-up. |
| nit | report.md:447 / ExternalFileMoveTests.swift:773 / :798 / CHANGELOG.md | report.md:447 says 'Regression suite passes (31 tests)'; the file contains 30 (report.md:369+387 add up correctly, so only the later count drifted). ExternalFileMoveTests.swift:773 carries a vacuous `#expect(newURL.lastPathComponent.isEmpty == false)` that exists only to consume an unused binding. :798 names the wrong startMigration signature. The CHANGELOG's 'Two files moved in quick succession' describes one file moved twice. | Cosmetic. Everything else in the CHANGELOG entry checks out against the code. |
| nit | FileChangeObserver.swift:132-147 / DocumentSession.swift:483 | The presenter retargets BEFORE the session consults isTrashed, so after a trash `observer.presentedItemURL == ~/.Trash/…` while `session.source` stays at the original path — `presentedItemURL == source` stops being an invariant and the change banner tracks the trashed copy. It self-heals on Put Back (the return move is a session no-op), and keeping the presenter tracking the item is what NSFilePresenter's contract asks for, so this is correct; it is just undocumented. Also, `notesRecordURL ?? oldURL` at DocumentSession.swift:491 is unreachable — every path that sets `source = .file` sets notesRecordURL in the same breath. | One sentence in the isTrashed doc comment and one assertion in trashingTheDocumentIsNotFollowedAsARename would cover it. |
Source: local run at 2026-09-06T20:55:54.087084+10:00 · snapshot 2dc3c6dc
Baseline: none
Execution: passed (partial results) · JUnit: 1 file · Coverage: none · Baseline: absent
Coverage scope: as the project configures it
Totals: 72 passed · 0 failed · 0 skipped · 0 errored · 0 flaky
Derived by declaration name, from the diff (no baseline run).
Files that import a changed file on the left, changed files in the centre, files a changed file imports on the right. Snapshot working-tree against base 693af8ff7fc2a5d7c48625073a166de313b87f2b.
prism/Resources/mermaid.min.js — blob over 1 MBClick to expand.
diff --git a/prism/Services/FileChangeObserver.swift b/prism/Services/FileChangeObserver.swiftindex eaf1b6b2..b35b8e67 100644--- a/prism/Services/FileChangeObserver.swift+++ b/prism/Services/FileChangeObserver.swift@@ -6,6 +6,7 @@ // import Foundation+import Synchronization /// Monitors a file for external changes using `NSFilePresenter`. ///@@ -43,14 +44,28 @@ final class FileChangeObserver: NSObject, NSFilePresenter { private(set) var changeGeneration = 0 /// The URL of the file being monitored.- private let fileURL: URL+ ///+ /// Mutable, because the presented item can be renamed or moved out from+ /// under the session (T-1881), and `Mutex`-backed rather than+ /// MainActor-isolated because `presentedItemURL` is read by the file+ /// coordination machinery on arbitrary threads. The move notification+ /// writes it synchronously, before any actor hop, so the presenter never+ /// answers with a path the coordinator has already told it is stale.+ private let fileURL: Mutex<URL> /// The operation queue for file presenter callbacks. /// Using main queue to ensure UI updates happen on the main thread. let presentedItemOperationQueue = OperationQueue.main /// The URL of the item being presented (required by NSFilePresenter).- var presentedItemURL: URL? { fileURL }+ nonisolated var presentedItemURL: URL? { fileURL.withLock { $0 } }++ /// Reports an external move of the presented item: `(from, to)`.+ ///+ /// Set by the owning `DocumentSession`, which retargets its own source.+ /// `@ObservationIgnored` — wiring, not view state.+ @ObservationIgnored+ var onMove: ((URL, URL) -> Void)? /// Creates a new file change observer for the specified URL. ///@@ -59,7 +74,7 @@ final class FileChangeObserver: NSObject, NSFilePresenter { /// /// - Parameter fileURL: The URL of the file to monitor. init(fileURL: URL) {- self.fileURL = fileURL+ self.fileURL = Mutex(fileURL) super.init() NSFileCoordinator.addFilePresenter(self) }@@ -92,6 +107,61 @@ final class FileChangeObserver: NSObject, NSFilePresenter { changeGeneration += 1 } + /// Called when the presented item is renamed or moved.+ ///+ /// Required by the `NSFilePresenter` contract: a presenter that does not+ /// retarget `presentedItemURL` keeps answering with the old location, so+ /// the coordinator stops matching it against operations on the file and+ /// every URL-derived feature of the open document goes stale — reload, the+ /// change banner, Save, the title, relative image resolution (T-1881).+ ///+ /// The retarget itself is done under the lock, without hopping, so the+ /// presenter is already answering with the new URL by the time this+ /// returns; only the report to the session needs the MainActor.+ ///+ /// And usually it needs nothing more than the assertion:+ /// `presentedItemOperationQueue` is `OperationQueue.main`, so this callback+ /// already arrives on the main thread, in the order the coordinator+ /// delivered the moves. Hopping through `Task { @MainActor }` threw that+ /// order away — two unstructured tasks are scheduled, not ordered — and a+ /// reversed A→B, B→C pair leaves `session.source` at B while the file is at+ /// C, because `followFileMove(to:)` derives its `from` from the live+ /// `source`. Reporting in place keeps the ordering the callback already had.+ /// The hop stays as a fallback rather than `assumeIsolated` unconditionally:+ /// that would trap the app if the queue contract were ever not honoured, and+ /// a move delivered off the main thread is still worth reporting late.+ nonisolated func presentedItemDidMove(to newURL: URL) {+ guard let oldURL = retarget(to: newURL) else { return }+ guard Thread.isMainThread else {+ Task { @MainActor in+ self.onMove?(oldURL, newURL)+ }+ return+ }+ MainActor.assumeIsolated {+ self.onMove?(oldURL, newURL)+ }+ }++ /// Retargets the presenter, returning the URL it was previously presenting,+ /// or `nil` when the move is to where it already points (nothing changed,+ /// so nothing to report).+ nonisolated private func retarget(to newURL: URL) -> URL? {+ fileURL.withLock { current in+ guard current != newURL else { return nil }+ defer { current = newURL }+ return current+ }+ }++ /// Applies a move synchronously and reports it, for tests that need the+ /// move to land deterministically without `presentedItemDidMove`'s task+ /// hop — the counterpart of `recordExternalChange()`.+ func recordMove(to newURL: URL) {+ guard let oldURL = retarget(to: newURL) else { return }+ onMove?(oldURL, newURL)+ }+ /// Acknowledges that the change has been handled unconditionally. /// /// Call this method when the user dismisses the reload banner — an
diff --git a/prism/Models/DocumentSession.swift b/prism/Models/DocumentSession.swiftindex 578bec24..9fca7fff 100644--- a/prism/Models/DocumentSession.swift+++ b/prism/Models/DocumentSession.swift@@ -135,8 +135,95 @@ final class DocumentSession: Identifiable { /// Only created for file-based documents. When clipboard content is /// saved, a new observer is created for the saved file. /// Requirement 2.4: No FileChangeObserver for clipboard-sourced content.+ ///+ /// Always built through `makeFileObserver(for:)`, never with the+ /// initializer directly, so every observer this session owns reports+ /// external moves back into `followFileMove(to:)` (T-1881). var fileObserver: FileChangeObserver? + /// One external move of this session's file: where it was, where it went.+ ///+ /// `from` is where the **notes record still is** (`notesRecordURL`), not+ /// necessarily where the previous move notification said the file was. The+ /// two part company whenever the record has not caught up with the file:+ /// two moves landing before the view consumes either, or a move whose+ /// migration failed — see `followFileMove`.+ struct FileMove: Equatable, Sendable {+ let from: URL+ let to: URL+ }++ /// Where this session's notes record is filed, as far as this session knows.+ ///+ /// Deliberately not derived from `source`. An external move retargets+ /// `source` at once — that is the point of `followFileMove` — while the+ /// notes record only follows when the migration the reader view runs+ /// actually lands, and a failed migration leaves it behind for good. Every+ /// move is reported *from here*, so the identifier a migration is told to+ /// migrate from is the one the record is really under, however many moves+ /// and failed attempts it took to get there (T-1881).+ ///+ /// It used to be inferred from `pendingFileMove?.from`, which conflated+ /// "where the record is" with "a move the view has not consumed yet". The+ /// view clears the second as soon as it starts the migration, so a failed+ /// migration left the next move naming the path the record had *not* moved+ /// to, and `migrateNotes` would then find nothing filed there.+ ///+ /// `nil` for a session with no file identity (clipboard, bundled, URL).+ private(set) var notesRecordURL: URL?++ /// An external move this session has already followed, still awaiting the+ /// identity work only the view layer can do — today, migrating the notes+ /// record onto the new location (T-1881).+ ///+ /// Set by `followFileMove(to:)` *after* `source` has already moved, not+ /// before. That is the opposite order from the Save As flow, which migrates+ /// notes first and only then calls `didSave(to:)` (Decision 6), and the+ /// difference is deliberate: a Save As chooses when it happens, whereas a+ /// move has already happened by the time we are told about it — leaving+ /// `source` on the old path for the length of an iCloud write would mean a+ /// Reload in that window reads a file that is no longer there — and it would+ /// make the retarget conditional on a mounted reader view running an async+ /// migration, where today it happens whether or not one is on screen.+ ///+ /// The price of that order is a window in which `source` names an identity+ /// nothing is filed under yet, so **everything** that resolves notes through+ /// `source` resolves that identity: a `loadNotes` from a reload *and* a note+ /// creation, which the first version of this comment claimed was safe. It+ /// was not, and it was the more damaging of the two: a creation resolves the+ /// target, `NotesManager.migrationDestination` redirects the target onto the+ /// target (a no-op), the container in memory is still filed under the source,+ /// so `noteContainer` reads it as another document's and clears it — every+ /// existing note gone from the pane, and the record behind them deleted by+ /// the migration that then believes it moved them.+ ///+ /// Two parts of that window, with different covers:+ ///+ /// - **From the consumer's turn to the end of the migration.**+ /// `NotesManager.startMigration(forMovedFileFrom:toFileURL:)` closes it+ /// from the same main-actor turn the consumer below observes this property+ /// in, and holds until the migration finishes. It does two synchronous+ /// things there, and both are needed: it *claims* the target, which retires+ /// a racing load, and it *rebinds the loaded container* onto the target,+ /// which is what a racing creation needs — a claim cannot help a creation,+ /// because the container it finds is the thing being judged.+ /// - **From the `source` flip in `followFileMove` until that turn.** Nothing+ /// covers the target here: `followFileMove` runs on the file presenter's+ /// main-thread callback, and the consumer runs in a *later* main-actor+ /// turn that Observation schedules. `startMigration` cannot reach back into+ /// this window — it has not been called yet. It is unreachable today rather+ /// than covered: nothing between the two turns resolves notes, because a+ /// move bumps no `parseRevision` (so the reader's `.task(id:)` notes load+ /// does not re-fire) and deliberately raises no reload banner (so the user+ /// cannot start one either), and a note creation needs a tap, which is a+ /// turn of its own. If any of that ever changes, both halves have to move+ /// into `followFileMove` itself.+ ///+ /// Consumed (and cleared) by `DocumentReaderView`, mirroring `pendingSave`.+ /// Clearing it does *not* record that the notes moved — `notesRecordURL`+ /// does, and only on success.+ var pendingFileMove: FileMove?+ /// This session's own security-scoped access lease on its file source /// (nil for non-file sources). ///@@ -336,9 +423,118 @@ final class DocumentSession: Identifiable { self.id = UUID() self.source = .file(url: url) self.content = content- self.fileObserver = FileChangeObserver(fileURL: url)+ self.notesRecordURL = url self.fileAccessLease = SecurityScopedResourceLease(resource: fileAccessResource) wireSearchClosures()+ self.fileObserver = makeFileObserver(for: url)+ }++ /// Builds the observer for `url` and wires its move report back into this+ /// session, so an external rename or move retargets the whole document+ /// rather than only the presenter (T-1881).+ ///+ /// `[weak self]`: the session owns the observer, and the observer's callback+ /// would otherwise own the session straight back.+ private func makeFileObserver(for url: URL) -> FileChangeObserver {+ let observer = FileChangeObserver(fileURL: url)+ observer.onMove = { [weak self] _, newURL in+ self?.followFileMove(to: newURL)+ }+ return observer+ }++ /// Retargets this session onto the new location of its file after an+ /// external rename or move.+ ///+ /// Everything URL-derived follows from `source`, so moving it is the whole+ /// fix for most of the symptom: `reloadDocument` reads the file that exists,+ /// the change banner keeps working (the presenter has already retargeted+ /// itself), the title and Save destination follow, and+ /// `WebDocumentStateSynchronizer` re-bases the page's image context off+ /// `source.imageSourceContext` and reloads at the same revision (T-1784).+ ///+ /// Deliberately left alone:+ ///+ /// - **The security-scoped lease.** The sandbox extension this session holds+ /// was issued for the *file* the user picked and survives its rename;+ /// a `URL` synthesised from a move notification carries no scope of its+ /// own, so starting a lease on it would return `false` and gain nothing,+ /// while dropping the existing one is the only way to actually lose access.+ /// - **The recent-files entry.** It keeps naming the path it was opened+ /// from; recents staleness is T-1842 / T-2172 / T-2173, not this.+ /// - **The stored scroll position.** `persistScrollPosition` keys off+ /// `source`, so from here on the position is written under the new+ /// location — where a later reopen looks for it. The record under the old+ /// path is left behind, unread and harmless.+ /// - Note: `pendingFileMove` carries the **origin** forward, not the+ /// previous hop. `.onChange` delivers only the last value a property took+ /// before the next body pass, so two moves landing in one pass (A→B, then+ /// B→C) are consumed as a single notification. Recording `(B, C)` for that+ /// pair reported a migration from an identifier the notes were never filed+ /// under: `migrateNotes` would evaluate `notesBelongToSaveChain` against a+ /// container still under `resolve(A)`, match neither the clipboard identity+ /// nor `resolve(B)`, skip — **and return `true`**. The session ends at C,+ /// the record stays at A, and the next `loadNotes(C)` has `applicableNotes`+ /// decline the A container and `clearNoteState()` empty the pane: this+ /// ticket's own symptom, reported as success. Reporting from+ /// `notesRecordURL` is the whole of the fix, and it is the right shape+ /// rather than a patch — the migration's job is "move the notes from where+ /// they are to where the file is", and A is where they are however many+ /// hops, and however many failed migrations, it took to get to C. A pair+ /// that returns home (A→B→A) reports nothing at all.+ func followFileMove(to newURL: URL) {+ guard case .file(let oldURL) = source, oldURL != newURL else { return }+ // A trashed document has not been renamed, whatever the notification+ // says. See `isTrashed(_:)`.+ guard !Self.isTrashed(newURL) else { return }+ source = .file(url: newURL)+ let origin = notesRecordURL ?? oldURL+ pendingFileMove = origin == newURL ? nil : FileMove(from: origin, to: newURL)+ }++ /// Records that the notes record has caught up with the file at `url`,+ /// i.e. the migration the reader view ran actually landed (T-1881).+ ///+ /// Only success calls this. A failure leaves `notesRecordURL` where it is,+ /// which is what makes the *next* move report the identifier the record is+ /// genuinely still under rather than the one the failed attempt aimed at.+ func notesRecordFollowedFile(to url: URL) {+ notesRecordURL = url+ }++ /// Whether a move notification is really the file being **deleted**.+ ///+ /// Trashing an open document arrives as `presentedItemDidMove(to:)` naming+ /// the file's new home inside the Trash — not, as this fix originally+ /// assumed, as `accommodatePresentedItemDeletion`. Measured on macOS 26+ /// with a standalone `NSFilePresenter` harness (see+ /// `specs/bugfixes/follow-external-file-moves/report.md`, F4):+ ///+ /// - `FileManager.trashItem` uncoordinated → `presentedItemDidMove` only.+ /// - the same call inside an `NSFileCoordinator` `.forDeleting` claim, which+ /// is what Finder does → `accommodatePresentedItemDeletion` **and then**+ /// `presentedItemDidMove`.+ /// - `removeItem` (a real unlink) → no callback at all.+ ///+ /// So without this guard, trashing an open document retargets the session+ /// onto `~/.Trash/…`, rewrites the notes record under a Trash path, and+ /// deletes the record under the path the file came from — losing the notes+ /// for good the moment the user empties the Trash, and losing them silently+ /// even if they Put Back, since nothing is filed under the restored path any+ /// more. Ignoring the move keeps the session on the path the document was+ /// opened from, which is where Put Back returns it to, and leaves the notes+ /// where the restored file will look for them. Emptying the Trash leaves a+ /// stale URL and a reload that fails, which is the honest report for a file+ /// the user deleted.+ ///+ /// Detection is by path component rather than by comparing against+ /// `FileManager.url(for: .trashDirectory, …)`: an item trashed on a+ /// secondary volume lands in `/Volumes/<name>/.Trashes/<uid>/`, and on iOS a+ /// file provider's own trash sits inside the provider's container — one+ /// query cannot name them all, while both spellings are reserved on every+ /// platform Prism runs on.+ static func isTrashed(_ url: URL) -> Bool {+ url.pathComponents.contains { $0 == ".Trash" || $0 == ".Trashes" } } #if DEBUG@@ -675,7 +871,17 @@ final class DocumentSession: Identifiable { /// zero mid-swap; the two leases briefly overlap instead. private func didSave(to url: URL, fileAccessResource: SecurityScopedResourceAccessing) { source = .file(url: url)- fileObserver = FileChangeObserver(fileURL: url)+ fileObserver = makeFileObserver(for: url)+ // A save supersedes any move this session has not finished following:+ // the notes are about to be migrated onto the save's destination, which+ // is where they belong, and migrating them onto the move's target first+ // would only move them somewhere the document no longer is.+ pendingFileMove = nil+ // The save chain runs its own notes migration *before* calling this+ // (Decision 6), and its failure paths settle the session onto whichever+ // URL the notes ended up under (`handleSaveFailed(notesRemainAt:)`), so+ // on every path that reaches here the record is at `url`.+ notesRecordURL = url if let pendingSaveLease, pendingSave?.url == url { fileAccessLease = pendingSaveLease } else {@@ -694,6 +900,10 @@ final class DocumentSession: Identifiable { source = .clipboard fileObserver = nil fileAccessLease = nil+ // No file left to have moved, and the notes are back under the+ // clipboard identifier rather than any file path.+ pendingFileMove = nil+ notesRecordURL = nil // Also releases a lease registered for the failed attempt that never // reached didSave(to:) to promote it — it has nothing left to cover. pendingSaveLease = nil
diff --git a/prism/Services/NotesManager.swift b/prism/Services/NotesManager.swiftindex b7ae9227..34ce9d2b 100644--- a/prism/Services/NotesManager.swift+++ b/prism/Services/NotesManager.swift@@ -1430,8 +1430,12 @@ final class NotesManager { // Delete the old notes file (orphan on failure is accepted — Decision 5). // Re-saving to the same destination has nothing to retire and must not- // delete the file just written.- if sourceIdentifier != targetIdentifier {+ // delete the file just written — nor may a chain that reaches here from+ // a `previousDestination` differing from the target only in case, which+ // is one file on a case-insensitive volume however different the two+ // identifiers look (T-1881). `retiringSourceRecordIsSafe` asks the store+ // rather than the identifiers.+ if await retiringSourceRecordIsSafe(source: sourceIdentifier, target: targetIdentifier) { await store.delete(for: sourceIdentifier) } return true@@ -1462,6 +1466,279 @@ final class NotesManager { } ++// MARK: - External File Move Migration++/// The move path has its own migration rather than an entry into the Save As+/// one — the preconditions differ on all three points that matter, and each+/// difference cost the user their notes. See `startMigration` for the argument.+///+/// An extension in this file, not a file of its own: it reads the private+/// migration claims and the private cached identity, and Swift `private` is+/// file-scoped. Splitting it out would mean widening those.+extension NotesManager {++ /// Starts the notes migration for an externally moved file (T-1881).+ ///+ /// **Its own migration, not an entry into the Save As one.** That is the+ /// substance of this fix rather than a refactor: `migrateNotes(fromClipboard+ /// Session:previousDestination:toFileURL:)` holds three preconditions that a+ /// Save As satisfies and a move does not, and every one of them failed in a+ /// way that ends with the user's notes gone.+ ///+ /// 1. **The notes are already in memory.** It opens on+ /// `guard var notes = documentNotes else { return true }`, which is sound+ /// for a clipboard document — its record cannot exist unless this session+ /// created it, so nothing loaded means nothing to move. For a move the+ /// record is on disk and "nothing loaded" is the *normal* state whenever+ /// the move outruns the first `loadNotes`, which is exactly what+ /// `.onChange(…, initial: true)` exists to catch: the move was consumed,+ /// cleared, and reported migrated, having moved nothing.+ /// 2. **The source identity cannot collide with the target.** A clipboard+ /// source is `clipboard/<UUID>`; here both sides are file paths, and the+ /// filename `NotesStore` derives from one is a *case-preserving*+ /// percent-encoding. Renaming `readme.md` to `README.md` therefore writes+ /// the record and then deletes the file it just wrote, because on a+ /// case-insensitive volume — the default on both platforms — the two+ /// names are one file. See `retiringSourceRecordIsSafe`.+ /// 3. **`session.source` has not flipped yet.** Save As calls `didSave(to:)`+ /// only after its migration returns, so a racing load or creation resolves+ /// the identity the migration has already claimed. A move flips `source`+ /// before the reader is told, so both resolve the *target* — and a claim+ /// on the target answers a load but cannot answer a creation, whose+ /// `noteContainer` judges the container it finds against that identity and+ /// clears it when it does not match. That is why the rebind below is+ /// synchronous.+ ///+ /// So this function does, before returning and therefore in the same+ /// main-actor turn the move is consumed in, everything that has to be true+ /// from that turn onwards:+ ///+ /// - **Claims** the source (a racing load of the old identity is retired, a+ /// racing creation on it is redirected onto the target) and the target+ /// itself (a racing load of the new identity is retired, since its record+ /// is not written yet). Both are retired on every exit of the returned+ /// task, including cancellation: an unstructured task's body always runs.+ /// - **Rebinds** an already-loaded container onto the target, so a creation+ /// resolving the flipped `source` finds its own document's container+ /// instead of clearing it.+ ///+ /// The disk work — and only the disk work — happens in the returned task.+ ///+ /// It cannot claim any earlier than this: `followFileMove(to:)` runs on the+ /// file presenter's callback and only publishes `pendingFileMove`, and+ /// `DocumentReaderView`'s `.onChange` calls this from the later main-actor+ /// turn Observation schedules. That stretch is argued unreachable in+ /// `DocumentSession.pendingFileMove`'s own comment, not covered here.+ ///+ /// - Parameters:+ /// - oldURL: where the notes record is filed — `DocumentSession.notes+ /// RecordURL`, not necessarily the file's previous location.+ /// - newURL: where the file now is.+ /// - Returns: the migration, so a caller can order work after it. The reader+ /// view awaits it to report failure and to record that the record has+ /// caught up with the file.+ @discardableResult+ func startMigration(forMovedFileFrom oldURL: URL, toFileURL newURL: URL) -> Task<Bool, Never> {+ let sourceIdentifier = identifierResolver.resolve(from: oldURL)+ let targetIdentifier = identifierResolver.resolve(from: newURL)+ let targetDisplayName = newURL.lastPathComponent++ // Identical identifiers mean the record is already where the file is —+ // two different URLs can resolve to one identifier, since the resolver+ // extracts a relative path. Nothing to claim, nothing to move.+ guard sourceIdentifier != targetIdentifier else { return Task { true } }++ let tickets = [sourceIdentifier, targetIdentifier].map {+ beginMigration(from: $0, to: targetIdentifier, displayName: targetDisplayName)+ }++ // The synchronous rebind. A creation racing this resolves the flipped+ // `session.source`, so it arrives at `noteContainer(for:)` naming the+ // target; without this the container in memory is still filed under the+ // source, `noteContainer` reads it as another document's, and+ // `clearNoteState()` takes every existing note out of the pane — after+ // which the migration's own post-save guard is *satisfied* by the+ // replacement (same identifier) and retires the source record on top.+ // `migrationDestination` cannot help there: it redirects the identity a+ // creation asks about, and this creation is already asking about the+ // right one.+ if var notes = documentNotes, notes.identifier == sourceIdentifier {+ notes.identifier = targetIdentifier+ notes.displayName = targetDisplayName+ documentNotes = notes+ }++ return Task { @MainActor in+ defer {+ for (identifier, ticket) in zip([sourceIdentifier, targetIdentifier], tickets) {+ self.endMigration(from: identifier, ticket: ticket)+ }+ }+ return await self.migrateNotesForMovedFile(+ from: sourceIdentifier,+ to: targetIdentifier,+ sourceDisplayName: oldURL.lastPathComponent,+ targetDisplayName: targetDisplayName+ )+ }+ }++ /// The disk half of a moved file's notes migration. See `startMigration`.+ ///+ /// `private` on purpose rather than by convention: the identity work this+ /// depends on has to have happened in the turn the move was consumed in,+ /// which is earlier than this function's own first line, so `startMigration`+ /// is the only correct entry. A doc comment saying so is not a rule the+ /// compiler enforces; the access level is.+ ///+ /// - Returns: `true` when the notes moved, or there were none to move;+ /// `false` when the store write failed.+ private func migrateNotesForMovedFile(+ from sourceIdentifier: DocumentIdentifier,+ to targetIdentifier: DocumentIdentifier,+ sourceDisplayName: String,+ targetDisplayName: String+ ) async -> Bool {+ guard let subject = await notesToMove(+ from: sourceIdentifier,+ to: targetIdentifier,+ targetDisplayName: targetDisplayName+ ) else { return true }++ // The write gate `notesBelongToSaveChain`'s own comment declares+ // mandatory, which the move path omitted: `store.save` replaces the+ // target's record atomically, so writing an *empty* container over it+ // destroys whatever is filed there — and unlike a Save As, where the+ // destination is a file the user just picked, a move's target can be a+ // path some earlier document left a record under. An empty container has+ // nothing to migrate, so there is nothing to trade for that.+ guard !subject.notes.notes.isEmpty else { return true }++ do {+ try await store.save(subject.notes)+ } catch {+ logger.error("Failed to save notes migrated behind a file move: \(error, privacy: .public)")+ // Revert the identity — but only the identity, and only while it is+ // still ours to revert. Read the *live* container rather than+ // republishing the snapshot above: a creation redirected onto the+ // target by `noteContainer(for:)` appends to `documentNotes` and+ // republishes it inside this very window without changing its+ // identifier, and overwriting would drop that note (the same+ // reasoning as the Save As migration's revert).+ guard subject.ownsMemory,+ var live = documentNotes,+ live.identifier == targetIdentifier else { return false }+ live.identifier = sourceIdentifier+ live.displayName = sourceDisplayName+ documentNotes = live+ return false+ }++ // The save is a suspension point, so re-check before rebinding anything+ // document-wide: memory may have moved on to a genuinely different+ // document while this was parked in the store, and rebinding the cached+ // identity onto this file would scope that document's imported-note+ // toggles to the wrong path (T-839). Nil passes where the Save As+ // migration would refuse, because this migration is allowed to run with+ // nothing loaded at all (precondition 1) — but there is then no memory+ // to rebind either, so it changes nothing.+ if subject.ownsMemory, documentNotes?.identifier == targetIdentifier {+ cachedDocumentPath = targetIdentifier.path+ cachedDocumentIdentifier = targetIdentifier+ documentPath = targetDisplayName+ }++ // The retirement is *not* conditional on any of that, unlike the Save As+ // migration's, which abandons both together. The save landed: the record+ // exists under the target, and the one under the source is a duplicate of+ // it filed where the file no longer is. Whatever the reader has moved on+ // to since does not make that less true, and leaving it behind is the+ // orphan half of this ticket's own symptom.+ if await retiringSourceRecordIsSafe(source: sourceIdentifier, target: targetIdentifier) {+ await store.delete(for: sourceIdentifier)+ }+ return true+ }++ /// The container a moved file's migration should write, and whether it is+ /// also the one on screen.+ ///+ /// `startMigration` has already rebound a loaded container onto the target,+ /// so a container carrying the target identity is this migration's own.+ /// Anything else means nothing of this document is loaded, and the record has+ /// to come off disk — the case the Save As migration cannot serve at all,+ /// since it opens on `documentNotes` and reports success when it is nil.+ private func notesToMove(+ from sourceIdentifier: DocumentIdentifier,+ to targetIdentifier: DocumentIdentifier,+ targetDisplayName: String+ ) async -> (notes: DocumentNotes, ownsMemory: Bool)? {+ if let live = documentNotes, live.identifier == targetIdentifier {+ return (live, true)+ }++ guard var stored = await store.load(for: sourceIdentifier) else { return nil }++ // Re-read across that suspension, the way `noteContainer` does. A note+ // created while this was parked in the store establishes the target+ // container itself — from an empty baseline, since the target's record+ // does not exist yet — and persists it. Writing `stored` over that would+ // drop the new note; writing the live container alone would drop+ // everything this migration is moving. The union is both.+ if let live = documentNotes, live.identifier == targetIdentifier {+ var merged = live+ let liveIDs = Set(live.notes.map(\.id))+ merged.notes = stored.notes.filter { !liveIDs.contains($0.id) } + live.notes+ merged.modifiedAt = Date()+ documentNotes = merged+ rebuildAnchoredNotes()+ return (merged, true)+ }++ stored.identifier = targetIdentifier+ stored.displayName = targetDisplayName++ // Deliberately not published. With nothing loaded, the reader's own+ // `loadNotes` relocates the record against the current blocks before it+ // shows anything, and publishing here would put pre-relocation block ids+ // on screen; moving the record is this migration's whole job in that+ // case. And if memory holds some *other* document, publishing would be+ // worse still — so the same answer covers both.+ return (stored, false)+ }++ /// Whether deleting the record under `source` retires a genuinely different+ /// record from the one just written under `target`.+ ///+ /// String inequality of the two identifiers is not that question, and+ /// answering it that way destroys notes on an ordinary rename (T-1881).+ /// `DocumentIdentifier.path` is case-preserving and `urlSafeEncoded` is a+ /// case-preserving percent-encoding of it, so `readme.md` and `README.md`+ /// produce two identifiers and one file on a case-insensitive volume — the+ /// default on macOS (APFS, which is also normalization-insensitive, so the+ /// same holds for a rename that only recomposes an accent) and on iOS. The+ /// save writes the record; the delete then removes the file it just wrote.+ /// Ordinary action, total loss, no alert.+ ///+ /// Asking the store instead of guessing at the filesystem's collation rules+ /// is what makes this right for every such aliasing rather than for the case+ /// one: if reading the old identifier back hands us the record we just wrote,+ /// the two names address one file and the delete would undo the save. It+ /// costs one extra read per migration, which happens at rename frequency.+ ///+ /// Used by both migrations — a Save As chain can reach the same collision+ /// through `previousDestination`, which is a file URL like any other.+ private func retiringSourceRecordIsSafe(+ source: DocumentIdentifier,+ target: DocumentIdentifier+ ) async -> Bool {+ guard source != target else { return false }+ guard let residual = await store.load(for: source) else { return false }+ return residual.identifier != target+ }+}+ // MARK: - Persistence Helpers extension NotesManager {
diff --git a/prism/Views/DocumentReaderView.swift b/prism/Views/DocumentReaderView.swiftindex 88c14271..2077b65b 100644--- a/prism/Views/DocumentReaderView.swift+++ b/prism/Views/DocumentReaderView.swift@@ -70,6 +70,20 @@ struct DocumentReaderView: View { /// Called when a link tap resolves to an in-app navigation target. var onOpenLink: ((ResolvedLink) -> Void)? + /// Called when the notes migration behind an external file move fails,+ /// carrying the session it belongs to and the path the notes remain filed+ /// under (T-1881).+ ///+ /// The failure is the same event Save As reports through `onSaveFailed`:+ /// the store write did not land, so the migration reverted the in-memory+ /// identity to the old path while `session.source` is already at the new+ /// one. Left unreported, the notes simply stop being this document's notes+ /// with nothing on screen to say so. The session is passed for the same+ /// reason `onSaveFailed` passes it — the migration outlives a document+ /// switch, and an alert about a document the user has already left is worse+ /// than none.+ var onMoveMigrationFailed: ((DocumentSession, URL) -> Void)?+ /// Shared NotesManager instance for both document content and sidebar. /// /// Created once here and injected via environment to all child views.@@ -332,6 +346,59 @@ struct DocumentReaderView: View { onFailed: onSaveFailed ) }+ // Follow the notes when the open file is renamed or moved externally+ // (T-1881). The session has already retargeted itself — everything+ // URL-derived follows from `source` — but the notes record is filed+ // under a path-derived identifier, so without this the next reload+ // would look for it under the new location and find nothing, and a+ // later reopen would never find it at all.+ //+ // Cleared first, so a move back to a URL this session has already+ // moved from is still a change `onChange` fires for, and so a+ // failure cannot re-enter this hook in a loop. Clearing it is *not*+ // the record of where the notes went — `notesRecordFollowedFile(to:)`+ // is, and only success calls it, which is what leaves a later move+ // reporting the identifier a failed attempt did not move the record+ // off.+ //+ // `startMigration` rather than a `Task` around it: the move has+ // already retargeted `session.source`, so the notes manager has to+ // claim the new identity, and rebind the loaded container onto it,+ // in *this* main-actor turn rather than in whichever turn an+ // unstructured task gets scheduled in. A reload landing in that gap+ // would load the new identity's empty record over the notes the+ // migration is moving; a note created in it would clear them.+ //+ // `initial: true`: the session follows a move whether or not a+ // reader view is mounted (that is the point of doing it in+ // `followFileMove`), so a move can land while this view is off+ // screen — a backgrounded scene, or before the navigation+ // destination is built. Without the initial pass that move is never+ // consumed and the notes are left behind under the old path, which+ // is the bug this whole hook exists to prevent. The guard makes the+ // usual case — nothing pending — a no-op.+ //+ // The result is not discarded: a failed store write reverts the+ // in-memory notes to the old identity while `session.source` is+ // already at the new one, so the notes silently stop being the+ // document's. Save As reports the same event with a "Note Migration+ // Failed" alert; so does this.+ .onChange(of: session.pendingFileMove, initial: true) { _, move in+ guard let move else { return }+ session.pendingFileMove = nil+ let migration = notesManager.startMigration(+ forMovedFileFrom: move.from,+ toFileURL: move.to+ )+ Task { @MainActor in+ let migrated = await migration.value+ if migrated {+ session.notesRecordFollowedFile(to: move.to)+ } else {+ onMoveMigrationFailed?(session, move.from)+ }+ }+ } // Cancel any pending task when view disappears .onDisappear { saveFlow.cancel()
diff --git a/prism/ViewModels/DocumentFlowCoordinator.swift b/prism/ViewModels/DocumentFlowCoordinator.swiftindex 4a91e21b..89d16eb7 100644--- a/prism/ViewModels/DocumentFlowCoordinator.swift+++ b/prism/ViewModels/DocumentFlowCoordinator.swift@@ -637,6 +637,37 @@ final class DocumentFlowCoordinator { } } + /// Reports a failed notes migration behind an external file move (T-1881).+ ///+ /// Same event as `handleSaveFailed`'s `notesRemainAt` branch, arriving from+ /// the file system instead of the exporter: the store write did not land, so+ /// the notes are still filed under the path the file has left while the open+ /// document is at its new one. Nothing else on screen says so — a move+ /// deliberately raises no banner — so it gets the same alert Save As does.+ ///+ /// Nothing is unwound here. The session has already followed the file and+ /// should stay there: the document the user is reading is the one at the new+ /// path, and reverting `source` to a path with no file at it would break+ /// reload to save a notes record the alert has just told them about.+ /// - Parameters:+ /// - session: The session whose file moved. The migration is a task that+ /// outlives the turn it started in, so by the time it fails the window+ /// may be showing a different document — and `migrationError` is one+ /// alert for the whole coordinator, so reporting a move of a document the+ /// user has already left names a file that is not on screen. Same guard,+ /// same reason, as `handleSaveFailed`.+ /// - notesRemainAt: The path the notes are still filed under.+ func handleMoveMigrationFailed(session: DocumentSession, notesRemainAt: URL) {+ guard currentSession === session else {+ logger.notice("handleMoveMigrationFailed: notes for \(notesRemainAt.lastPathComponent, privacy: .public) stayed put after its document was replaced; not reporting over the current document")+ return+ }+ let fileName = notesRemainAt.lastPathComponent+ migrationError = String(+ localized: "Your notes could not be moved to the file's new location, so they are still filed under its previous name, \(fileName)."+ )+ }+ /// Opens a bundled markdown document by resource name. func openBundledDocument(resourceName: String) { guard let url = BundledDocument.url(for: resourceName) else {
diff --git a/prism/prismApp.swift b/prism/prismApp.swiftindex 87b2acf7..65c9da92 100644--- a/prism/prismApp.swift+++ b/prism/prismApp.swift@@ -319,6 +319,12 @@ struct MainContentView: View { }, onOpenLink: { resolved in handleResolvedLink(resolved)+ },+ onMoveMigrationFailed: { movedSession, notesRemainAt in+ flowCoordinator.handleMoveMigrationFailed(+ session: movedSession,+ notesRemainAt: notesRemainAt+ ) } ) }
diff --git a/prism/Localizable.xcstrings b/prism/Localizable.xcstringsindex d4285bd1..1c27fbcf 100644--- a/prism/Localizable.xcstrings+++ b/prism/Localizable.xcstrings@@ -6050,6 +6050,29 @@ } } },+ "Your notes could not be moved to the file's new location, so they are still filed under its previous name, %@.": {+ "extractionState": "manual",+ "localizations": {+ "en": {+ "stringUnit": {+ "state": "translated",+ "value": "Your notes could not be moved to the file's new location, so they are still filed under its previous name, %@."+ }+ },+ "en-GB": {+ "stringUnit": {+ "state": "translated",+ "value": "Your notes could not be moved to the file's new location, so they are still filed under its previous name, %@."+ }+ },+ "en-US": {+ "stringUnit": {+ "state": "translated",+ "value": "Your notes could not be moved to the file's new location, so they are still filed under its previous name, %@."+ }+ }+ }+ }, "Zoom In": { "extractionState": "manual", "localizations": {
diff --git a/prismTests/ExternalFileMoveTests.swift b/prismTests/ExternalFileMoveTests.swiftnew file mode 100644index 00000000..4b2e4eff--- /dev/null+++ b/prismTests/ExternalFileMoveTests.swift@@ -0,0 +1,969 @@+//+// ExternalFileMoveTests.swift+// prismTests+//+// T-1881: an open document does not follow its file when the file is renamed+// or moved externally (Finder, Files, iCloud, another file provider).+//+// `FileChangeObserver` is an `NSFilePresenter` that implemented no+// `presentedItemDidMove(to:)`, so the presenter kept answering with the+// original URL and `DocumentSession.source` kept naming it. Everything+// URL-derived then pointed at a file that is no longer there: Reload failed,+// the title kept the old name, relative images resolved against the old+// folder, and the notes record was left under an identifier the document+// would never resolve again.+//++import Foundation+import Testing+@testable import prism++@Suite("External file moves")+@MainActor+struct ExternalFileMoveTests {++ // MARK: - Presenter++ @Test("presentedItemDidMove retargets the presented URL")+ func presenterRetargetsOnMove() throws {+ let (oldURL, newURL) = movePair()+ let observer = FileChangeObserver(fileURL: oldURL)++ observer.presentedItemDidMove(to: newURL)++ // Bug: the presenter had no `presentedItemDidMove(to:)` at all, so it+ // kept presenting `oldURL` and the coordinator stopped matching it+ // against operations on the file.+ #expect(observer.presentedItemURL == newURL)+ }++ @Test("a move is reported to the observer's owner exactly once")+ func moveIsReportedOnce() throws {+ let (oldURL, newURL) = movePair()+ let observer = FileChangeObserver(fileURL: oldURL)+ var reported: [DocumentSession.FileMove] = []+ observer.onMove = { from, to in reported.append(.init(from: from, to: to)) }++ observer.recordMove(to: newURL)+ // A second notification naming where the presenter already points is+ // not a move: nothing changed, so nothing is reported.+ observer.recordMove(to: newURL)++ #expect(reported == [.init(from: oldURL, to: newURL)])+ }++ @Test("a move does not raise the external-change banner")+ func moveDoesNotRaiseTheChangeBanner() throws {+ let (oldURL, newURL) = movePair()+ let observer = FileChangeObserver(fileURL: oldURL)++ observer.recordMove(to: newURL)++ // Renaming a file changes where it is, not what is in it — the reload+ // banner would be offering to re-read content that has not changed.+ #expect(observer.fileChangedExternally == false)+ #expect(observer.changeGeneration == 0)+ }++ // MARK: - Session++ @Test("the session follows its file to the new location")+ func sessionFollowsTheMove() throws {+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")++ session.fileObserver?.recordMove(to: newURL)++ #expect(session.source == .file(url: newURL))+ #expect(session.pendingFileMove == .init(from: oldURL, to: newURL))+ }++ @Test("the title and the image base follow the move")+ func titleAndImageBaseFollowTheMove() throws {+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "no heading here")++ session.fileObserver?.recordMove(to: newURL)++ // Both are derived from `source`, which is the point: retargeting the+ // one property is what makes the window title, the relative-image base+ // and the page's image context follow (T-1784's shared box is written+ // from `source.imageSourceContext` by the synchronizer).+ #expect(session.navigationTitle == newURL.lastPathComponent)+ #expect(session.source.imageBaseURL == newURL.deletingLastPathComponent())+ }++ @Test("a move to where the session already points changes nothing")+ func selfMoveIsIgnored() throws {+ let (oldURL, _) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")++ session.followFileMove(to: oldURL)++ #expect(session.pendingFileMove == nil)+ }++ // MARK: - Two moves before either is consumed++ /// `.onChange` delivers only the last value a property took before the next+ /// body pass, so A→B followed by B→C inside one pass is consumed as a single+ /// notification. Recording the last *hop* there — `(B, C)` — names an+ /// identifier the notes were never filed under.+ @Test("two moves before the view consumes either carry the origin forward")+ func consecutiveMovesCarryTheOriginForward() throws {+ let (urlA, urlB, urlC) = moveChain()+ let session = DocumentSession(url: urlA, content: "# Doc")++ session.fileObserver?.recordMove(to: urlB)+ session.fileObserver?.recordMove(to: urlC)++ // The notes are at A however many hops it took to reach C, and A is+ // what the one surviving notification has to name.+ #expect(session.source == .file(url: urlC))+ #expect(session.pendingFileMove == .init(from: urlA, to: urlC))+ }++ /// The data-loss half of the same bug, end to end.+ ///+ /// With `(B, C)` recorded, `migrateNotes` evaluates `notesBelongToSaveChain`+ /// against a container still filed under `resolve(A)` — matching neither the+ /// clipboard identity nor `resolve(B)` — logs "migrateNotes skipped" and+ /// returns **`true`**. The session ends at C, the record stays at A, and the+ /// next `loadNotes(C)` has `applicableNotes` decline the A container, so+ /// `clearNoteState()` empties the pane: this ticket's own symptom, reported+ /// as success. Which is also why the migration's return value cannot stand+ /// in for the assertions below — it was `true` while the notes were lost.+ @Test("two moves before the view consumes either still migrate the notes")+ func consecutiveMovesMigrateTheNotesFromTheOriginalPath() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (urlA, urlB, urlC) = moveChain()+ let (session, manager) = await makeFileSessionWithNote(url: urlA, store: store)++ session.fileObserver?.recordMove(to: urlB)+ session.fileObserver?.recordMove(to: urlC)++ // One consumption, because one notification is all the view gets.+ let move = try #require(session.pendingFileMove)+ let migrated = await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value+ #expect(migrated)++ #expect(manager.documentNotes?.identifier == resolver.resolve(from: urlC))+ #expect(await store.storedNotes[resolver.resolve(from: urlC).path]?.notes+ .map(\.content) == ["Note on the original"])+ #expect(await store.storedNotes[resolver.resolve(from: urlA).path] == nil)++ // The reader's own load, resolved through the retargeted source: this is+ // where the stranded record showed up as an empty pane.+ await manager.loadNotes(+ source: session.source, sessionID: session.id, blocks: session.parsedBlocks+ )+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])+ }++ @Test("a move that returns to where it started leaves nothing to migrate")+ func aRoundTripCoalescesToNothing() throws {+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")++ session.fileObserver?.recordMove(to: newURL)+ session.fileObserver?.recordMove(to: oldURL)++ // The notes never left `oldURL` and the file is back at it, so a+ // migration would be from an identifier onto itself — a store write to+ // achieve nothing. Carrying the origin forward makes that fall out.+ #expect(session.source == .file(url: oldURL))+ #expect(session.pendingFileMove == nil)+ }++ @Test("saving supersedes a move that has not been consumed yet")+ func saveClearsAnUnconsumedMove() throws {+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")+ session.fileObserver?.recordMove(to: newURL)++ let savedURL = FileManager.default.temporaryDirectory+ .appendingPathComponent("move-saved-\(UUID().uuidString).md")+ session.didSave(to: savedURL)++ // The notes are about to be migrated onto the save's destination;+ // migrating them onto the move's target first would only move them+ // somewhere the document no longer is.+ #expect(session.pendingFileMove == nil)+ #expect(session.source == .file(url: savedURL))+ }++ @Test("the observer created by a save also follows moves")+ func observerFromSaveFollowsMoves() throws {+ let session = DocumentSession(clipboardContent: "# Pasted")+ let savedURL = FileManager.default.temporaryDirectory+ .appendingPathComponent("move-after-save-\(UUID().uuidString).md")+ let movedURL = FileManager.default.temporaryDirectory+ .appendingPathComponent("move-after-save-renamed-\(UUID().uuidString).md")+ session.didSave(to: savedURL)++ session.fileObserver?.recordMove(to: movedURL)++ // `didSave` builds a second observer; wiring only the initializer's+ // would leave a Save As'd document unable to follow a later rename.+ #expect(session.source == .file(url: movedURL))+ }++ // MARK: - Reload (the headline symptom)++ @Test("reload after a rename reads the file at its new location")+ func reloadFollowsTheRename() async throws {+ let directory = FileManager.default.temporaryDirectory+ let oldURL = directory.appendingPathComponent("move-reload-\(UUID().uuidString).md")+ let newURL = directory.appendingPathComponent("move-reload-renamed-\(UUID().uuidString).md")+ try "# Original".write(to: oldURL, atomically: true, encoding: .utf8)+ defer { try? FileManager.default.removeItem(at: newURL) }++ let session = DocumentSession(url: oldURL, content: "# Original")+ await session.parseContent()++ // Rename on disk, then deliver the move the file coordinator would.+ try FileManager.default.moveItem(at: oldURL, to: newURL)+ try "# Renamed and edited".write(to: newURL, atomically: true, encoding: .utf8)+ session.fileObserver?.recordMove(to: newURL)++ let coordinator = DocumentLayoutCoordinator()+ let reload = try #require(coordinator.reloadDocument(session: session))+ await reload.value++ // Bug: the session still named `oldURL`, so the read threw and the user+ // was shown "Reload failed" for a document that is perfectly readable.+ #expect(coordinator.reloadError == nil)+ #expect(session.cachedDocumentTitle == "Renamed and edited")+ }++ // MARK: - Notes identity++ @Test("the notes follow the file to its new identifier")+ func notesFollowTheMove() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)+ _ = await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value++ // A rename is the same identity change as a Save As, so it gets the same+ // answer: the notes move with the document, and the record under the old+ // path — which nothing will ever resolve again — is retired.+ #expect(manager.documentNotes?.identifier == resolver.resolve(from: newURL))+ #expect(manager.documentNotes?.displayName == newURL.lastPathComponent)+ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content) == ["Note on the original"])+ #expect(await store.storedNotes[resolver.resolve(from: oldURL).path] == nil)+ }++ @Test("a reload after the move finds the migrated notes")+ func reloadAfterMoveFindsTheNotes() async throws {+ let store = MockNotesStore()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)+ _ = await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value++ // The notes pipeline reloads on every parse revision, keyed off+ // `session.source` — the very thing the move retargeted. Without the+ // migration this load finds nothing and the notes vanish from the pane.+ await manager.loadNotes(+ source: session.source, sessionID: session.id, blocks: session.parsedBlocks+ )++ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])+ }++ @Test("a move on a document with no notes is a no-op")+ func moveWithoutNotesDoesNothing() async throws {+ let store = MockNotesStore()+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")+ let manager = NotesManager.makeForTesting(store: store)++ let migrated = await manager.startMigration(+ forMovedFileFrom: oldURL, toFileURL: newURL+ ).value++ #expect(migrated)+ #expect(await store.saveCount == 0)+ #expect(await store.deleteCount == 0)+ }++ // MARK: - A reload racing the migration++ /// The move path's version of T-2231, one identity over.+ ///+ /// `followFileMove(to:)` retargets `source` to the new URL *before* the+ /// notes migration runs — the opposite order from Save As, and deliberately+ /// so (see `pendingFileMove`). The cost is that between the retarget and the+ /// end of the migration, `session.source` names an identity whose notes+ /// record does not exist yet, and everything that resolves notes through+ /// `session.source` will resolve *that* one — including the+ /// `.task(id: session.parseRevision)` load a reload landing near the move+ /// starts.+ ///+ /// `migrateNotes` claims the source and clipboard identities, never the+ /// target, so before `startMigration` that load fell through every guard: it+ /// read the target's absent record, `applicableNotes` declined the container+ /// the migration had already rebound (unchanged across the load's own+ /// await), and `clearNoteState()` wiped it. The migration then read its own+ /// post-save guard as "a newer document replaced me" and abandoned the+ /// rebind — empty notes panel, orphaned record under the old path.+ @Test("a reload landing inside the move's migration does not wipe the notes")+ func reloadInsideTheMigrationDoesNotWipeTheNotes() async throws {+ let store = ParkedSaveNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)++ // Exactly what the reader view's notes task would call, resolved through+ // the already-retargeted source, delivered while the migration is parked+ // in its save with the target's record still unwritten.+ let reloadSource = session.source+ let sessionID = session.id+ let blocks = session.parsedBlocks+ await store.setOnSave { [manager] in+ await manager.loadNotes(source: reloadSource, sessionID: sessionID, blocks: blocks)+ }++ let migrated = await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value++ #expect(migrated)+ #expect(manager.documentNotes?.identifier == resolver.resolve(from: newURL))+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])+ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content) == ["Note on the original"])+ #expect(await store.storedNotes[resolver.resolve(from: oldURL).path] == nil)+ }++ /// The other half of the same window, and the reason the claim is taken by+ /// `startMigration` rather than left to `migrateNotes`.+ ///+ /// `migrateNotes` is `async`, so its first line — claims included — runs a+ /// task later than the turn the move was consumed in. A load starting in+ /// that gap resolves the target against an in-memory container still filed+ /// under the *old* identifier, which `applicableNotes` declines outright as+ /// another document's.+ ///+ /// The load is retired at its entry guard, which is before its first+ /// suspension point — so it returns having written nothing, and in+ /// particular without moving the cached identity onto the target. That+ /// prologue write is the part no later guard undoes (T-2231), which is why+ /// it is what this pins.+ @Test("consuming a move claims the new identity before the migration task runs")+ func consumingAMoveClaimsTheTargetSynchronously() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)++ let migration = manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ )+ // No suspension between the call above and this load reaching its entry+ // guard, so the migration task has not run: only a claim `startMigration`+ // took before returning can retire it.+ await manager.loadNotes(+ source: session.source, sessionID: session.id, blocks: session.parsedBlocks+ )++ #expect(manager.cachedDocumentIdentifier == resolver.resolve(from: oldURL))+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])++ #expect(await migration.value)+ #expect(manager.documentNotes?.identifier == resolver.resolve(from: newURL))+ #expect(manager.cachedDocumentIdentifier == resolver.resolve(from: newURL))+ }++ // MARK: - Wiring++ /// Every other test in this suite reaches the session through+ /// `recordMove(to:)`, which is the seam that *skips* the production entry+ /// point. This one uses the entry point the file coordinator actually calls+ /// — the T-1943 lesson, that a direct-invocation test cannot see missing+ /// wiring.+ @Test("the file coordinator's own callback reaches the session")+ func presentedItemDidMoveReachesTheSession() async throws {+ let (oldURL, newURL) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")+ let observer = try #require(session.fileObserver)++ observer.presentedItemDidMove(to: newURL)++ // Retargeting the presenter is synchronous — the coordinator must not+ // see a stale `presentedItemURL` even for one hop. So is the report,+ // when the callback arrives on the main thread as the presenter's own+ // `presentedItemOperationQueue` guarantees; the next test is why that+ // matters.+ #expect(observer.presentedItemURL == newURL)+ #expect(session.source == .file(url: newURL))+ #expect(session.pendingFileMove == .init(from: oldURL, to: newURL))+ }++ /// The coordinator delivers moves in order on `OperationQueue.main`, and+ /// `followFileMove(to:)` derives its `from` from the live `source`, so the+ /// order has to survive the trip. It did not while the report went through+ /// `Task { @MainActor }`: two unstructured tasks are scheduled, not ordered,+ /// and a reversed pair leaves the session at B while the file is at C.+ @Test("consecutive coordinator callbacks land in the order they arrived")+ func presentedItemDidMovePreservesOrdering() async throws {+ let (urlA, urlB, urlC) = moveChain()+ let session = DocumentSession(url: urlA, content: "# Doc")+ let observer = try #require(session.fileObserver)++ observer.presentedItemDidMove(to: urlB)+ observer.presentedItemDidMove(to: urlC)++ #expect(observer.presentedItemURL == urlC)+ #expect(session.source == .file(url: urlC))+ #expect(session.pendingFileMove == .init(from: urlA, to: urlC))+ }++ // MARK: - The move path's own preconditions (round 5)++ /// **A case-only rename destroyed every note.**+ ///+ /// `DocumentIdentifier.path` is case-preserving and `NotesStore` derives the+ /// record's filename from it by a case-preserving percent-encoding, so+ /// `readme.md` and `README.md` are two identifiers — and, on a+ /// case-insensitive volume (the default on macOS and iOS), one file. The+ /// borrowed Save As migration guards its source delete on string inequality,+ /// so it saved the record and then deleted the file it had just written.+ /// Ordinary action, total loss, no alert. The Save As caller cannot reach it+ /// (its source is a clipboard UUID); the move path can, on any rename that+ /// only changes case.+ @Test("a case-only rename keeps the notes")+ func caseOnlyRenameKeepsTheNotes() async throws {+ let store = CaseFoldingNotesStore()+ let resolver = DocumentIdentifierResolver()+ let directory = FileManager.default.temporaryDirectory+ let stem = UUID().uuidString+ let oldURL = directory.appendingPathComponent("case-\(stem)-readme.md")+ let newURL = directory.appendingPathComponent("case-\(stem)-README.md")+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)+ let migrated = await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value++ #expect(migrated)+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])+ // The one record on the volume, whichever spelling is used to read it.+ #expect(await store.load(for: resolver.resolve(from: newURL))?.notes+ .map(\.content) == ["Note on the original"])++ // And the reader's own load, which is where the loss showed up.+ await manager.loadNotes(+ source: session.source, sessionID: session.id, blocks: session.parsedBlocks+ )+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])+ }++ /// **The `initial: true` branch could not migrate anything.**+ ///+ /// The borrowed migration opens on `guard var notes = documentNotes else+ /// { return true }` — memory only, never the store. `notesManager` is+ /// `@State` on `DocumentReaderView`, so on the initial pass `documentNotes`+ /// is guaranteed nil: the move was consumed, cleared and reported migrated,+ /// having moved nothing. The same hole swallowed any move arriving before the+ /// first `loadNotes` completed.+ @Test("a move migrates the stored record with nothing loaded")+ func aMoveMigratesTheStoredRecordWithNothingLoaded() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let sourceIdentifier = resolver.resolve(from: oldURL)+ await store.preload(+ DocumentNotes(+ identifier: sourceIdentifier,+ displayName: oldURL.lastPathComponent,+ notes: [makeNote(content: "Written before this session opened")]+ )+ )++ // A manager that has never loaded: exactly the state the initial pass+ // consumes a move in.+ let manager = NotesManager.makeForTesting(store: store)+ #expect(manager.documentNotes == nil)++ let migrated = await manager.startMigration(+ forMovedFileFrom: oldURL, toFileURL: newURL+ ).value++ #expect(migrated)+ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content) == ["Written before this session opened"])+ #expect(await store.storedNotes[sourceIdentifier.path] == nil)+ }++ /// **A note created in the migration window orphaned every existing note.**+ ///+ /// The exact inverse of what `startMigration`'s first doc comment claimed.+ /// A creation resolves the *already-flipped* `session.source`, so it names+ /// the target; `migrationDestination(for: target)` returns the target and+ /// redirects nothing; the container in memory is still filed under the+ /// source, so `noteContainer` reads it as another document's and+ /// `clearNoteState()` empties the pane — after which the migration's+ /// post-save guard is *satisfied* by the replacement and retires the source+ /// record on top of it. A claim cannot cover this: the container is the thing+ /// being judged. Rebinding it synchronously is what does.+ @Test("a note created while the migration runs keeps the existing notes")+ func aNoteCreatedWhileTheMigrationRunsKeepsTheExistingNotes() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)++ let migration = manager.startMigration(forMovedFileFrom: move.from, toFileURL: move.to)++ // The deterministic half. No suspension since the line above, so the+ // migration task has not run a line — this can only be the rebind+ // `startMigration` did before returning, and it is what a creation+ // resolving the flipped `session.source` needs to find.+ #expect(manager.documentNotes?.identifier == resolver.resolve(from: newURL))+ #expect(manager.documentNotes?.notes.map(\.content) == ["Note on the original"])++ // The behavioural half: the creation itself.+ let block = try #require(session.parsedBlocks.first)+ await manager.createNote(+ content: "Added while the file was being renamed",+ for: block,+ sourceIndex: 0,+ in: MarkdownSectionBuilder.build(from: [block]),+ source: session.source,+ sessionID: session.id+ )+ #expect(await migration.value)++ #expect(manager.documentNotes?.identifier == resolver.resolve(from: newURL))+ #expect(manager.documentNotes?.notes.map(\.content).sorted() == [+ "Added while the file was being renamed", "Note on the original"+ ])+ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content).sorted() == [+ "Added while the file was being renamed", "Note on the original"+ ])+ }++ /// The write gate `notesBelongToSaveChain`'s own comment declares mandatory,+ /// which the move path omitted. `store.save` replaces the target's record+ /// atomically, so an emptied container writes over whatever is filed there —+ /// and a move's target, unlike a Save As destination the user just picked,+ /// can be a path an earlier document left a record under.+ @Test("a move with an empty container does not overwrite the target's record")+ func anEmptyContainerDoesNotOverwriteTheTargetsRecord() async throws {+ let store = MockNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ // An empty record for this document, and someone else's notes already+ // filed under the path the file is moving to.+ await store.preload(+ DocumentNotes(identifier: resolver.resolve(from: oldURL), displayName: oldURL.lastPathComponent)+ )+ await store.preload(+ DocumentNotes(+ identifier: resolver.resolve(from: newURL),+ displayName: newURL.lastPathComponent,+ notes: [makeNote(content: "Notes of whatever was here before")]+ )+ )++ let session = DocumentSession(url: oldURL, content: "Original")+ session.parsedBlocks = [MarkdownBlock.paragraph(markdown: "Original")]+ let manager = NotesManager.makeForTesting(store: store)+ await manager.loadNotes(+ source: session.source, sessionID: session.id, blocks: session.parsedBlocks+ )+ #expect(manager.documentNotes?.notes.isEmpty == true)++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)+ #expect(await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value)++ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content) == ["Notes of whatever was here before"])+ }++ /// The save landed, so the record is under the target and the one under the+ /// source is a duplicate filed where the file no longer is. The Save As+ /// migration abandons its rebind *and* its delete together when memory has+ /// moved on to another document; this one keeps the delete, because "the+ /// reader has moved on" says nothing about whether the write happened.+ /// Leaving it behind is the orphaned-record half of this ticket's symptom.+ @Test("a reader that moves on mid-migration does not strand the old record")+ func aReaderMovingOnMidMigrationDoesNotStrandTheOldRecord() async throws {+ let store = ParkedSaveNotesStore()+ let resolver = DocumentIdentifierResolver()+ let (oldURL, newURL) = movePair()+ let (session, manager) = await makeFileSessionWithNote(url: oldURL, store: store)++ // A different document, with notes of its own, opened while the+ // migration is parked in its save.+ let otherURL = FileManager.default.temporaryDirectory+ .appendingPathComponent("other-\(UUID().uuidString).md")+ await store.preload(+ DocumentNotes(+ identifier: resolver.resolve(from: otherURL),+ displayName: otherURL.lastPathComponent,+ notes: [makeNote(content: "Another document's note")]+ )+ )+ await store.setOnSave { [manager] in+ await manager.loadNotes(source: .file(url: otherURL), sessionID: UUID(), blocks: [])+ }++ session.fileObserver?.recordMove(to: newURL)+ let move = try #require(session.pendingFileMove)+ #expect(await manager.startMigration(+ forMovedFileFrom: move.from, toFileURL: move.to+ ).value)++ #expect(await store.storedNotes[resolver.resolve(from: newURL).path]?.notes+ .map(\.content) == ["Note on the original"])+ #expect(await store.storedNotes[resolver.resolve(from: oldURL).path] == nil)+ // And the document the reader actually moved on to is untouched.+ #expect(manager.documentNotes?.notes.map(\.content) == ["Another document's note"])+ }++ // MARK: - Trashing is not a rename++ /// **Trashing an open document arrives as a move.**+ ///+ /// The claim this fix was built on — that a Finder trash reaches a presenter+ /// as `accommodatePresentedItemDeletion` rather than+ /// `presentedItemDidMove(to:)` — is false. Measured on macOS 26 with a+ /// standalone `NSFilePresenter` registered against a real file+ /// (`specs/bugfixes/follow-external-file-moves/report.md`, F4):+ ///+ /// - `FileManager.trashItem`, uncoordinated → `presentedItemDidMove(to:+ /// ~/.Trash/…)` and nothing else.+ /// - the same inside an `NSFileCoordinator` `.forDeleting` claim, which is+ /// the Finder shape → `accommodatePresentedItemDeletion` **followed by**+ /// `presentedItemDidMove`.+ /// - `removeItem`, a real unlink → no callback at all.+ ///+ /// So the move arrives either way, and without this guard trashing a+ /// document rewrote its notes under a `~/.Trash` path and deleted the record+ /// at the path the file came from — losing them outright on Empty Trash, and+ /// losing them silently even on Put Back.+ @Test("trashing the open document is not followed as a rename")+ func trashingTheDocumentIsNotFollowedAsARename() throws {+ let (oldURL, _) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")+ let trashed = URL(fileURLWithPath: NSHomeDirectory())+ .appendingPathComponent(".Trash")+ .appendingPathComponent(oldURL.lastPathComponent)++ session.fileObserver?.recordMove(to: trashed)++ // The session stays where Put Back will return the file to, and the+ // notes stay where the restored file will look for them.+ #expect(session.source == .file(url: oldURL))+ #expect(session.pendingFileMove == nil)+ }++ @Test("a per-volume .Trashes destination is treated the same")+ func perVolumeTrashIsTreatedTheSame() throws {+ let (oldURL, _) = movePair()+ let session = DocumentSession(url: oldURL, content: "# Doc")+ let trashed = URL(fileURLWithPath: "/Volumes/Backup/.Trashes/501")+ .appendingPathComponent(oldURL.lastPathComponent)++ session.fileObserver?.recordMove(to: trashed)++ #expect(session.source == .file(url: oldURL))+ #expect(session.pendingFileMove == nil)+ }++ // MARK: - Where the notes record actually is++ /// Consuming a move is not the same event as the notes arriving. The view+ /// clears `pendingFileMove` when it *starts* the migration, so inferring the+ /// record's location from it left a failed migration reporting the next move+ /// from a path the record had never reached — and `migrateNotes` would then+ /// find nothing filed there and report success again.+ @Test("a failed migration leaves the next move reporting the record's real location")+ func aFailedMigrationLeavesTheNextMoveReportingTheOrigin() async throws {+ let store = MockNotesStore()+ let (urlA, urlB, urlC) = moveChain()+ let (session, manager) = await makeFileSessionWithNote(url: urlA, store: store)+ await store.set(simulateSaveError: NSError(domain: "test", code: 1))++ session.fileObserver?.recordMove(to: urlB)+ let first = try #require(session.pendingFileMove)+ session.pendingFileMove = nil+ let migrated = await manager.startMigration(+ forMovedFileFrom: first.from, toFileURL: first.to+ ).value+ #expect(migrated == false)+ // Failure: the view does not call `notesRecordFollowedFile`.++ session.fileObserver?.recordMove(to: urlC)++ // The record never left A, so that is what the next move has to name.+ #expect(session.pendingFileMove == .init(from: urlA, to: urlC))+ }++ @Test("a successful migration moves where the next move is reported from")+ func aSuccessfulMigrationMovesTheReportedOrigin() async throws {+ let store = MockNotesStore()+ let (urlA, urlB, urlC) = moveChain()+ let (session, manager) = await makeFileSessionWithNote(url: urlA, store: store)++ session.fileObserver?.recordMove(to: urlB)+ let first = try #require(session.pendingFileMove)+ session.pendingFileMove = nil+ #expect(await manager.startMigration(+ forMovedFileFrom: first.from, toFileURL: first.to+ ).value)+ session.notesRecordFollowedFile(to: first.to)++ session.fileObserver?.recordMove(to: urlC)++ #expect(session.pendingFileMove == .init(from: urlB, to: urlC))+ }++ // MARK: - Reporting the failure++ /// The migration outlives the turn it started in, and `migrationError` is+ /// one alert for the whole coordinator, so a failure landing after the user+ /// has moved on would name a file that is not on screen. Same guard, same+ /// reason, as `handleSaveFailed`.+ @Test("a move failure is not reported over a different document")+ func moveFailureIsNotReportedOverADifferentDocument() throws {+ let (oldURL, newURL) = movePair()+ let moved = DocumentSession(url: oldURL, content: "# Moved")+ let coordinator = DocumentFlowCoordinator()+ coordinator.currentSession = DocumentSession(clipboardContent: "# Something else")++ coordinator.handleMoveMigrationFailed(session: moved, notesRemainAt: oldURL)+ #expect(coordinator.migrationError == nil)++ coordinator.currentSession = moved+ coordinator.handleMoveMigrationFailed(session: moved, notesRemainAt: oldURL)+ #expect(coordinator.migrationError?.contains(oldURL.lastPathComponent) == true)+ #expect(newURL.lastPathComponent.isEmpty == false)+ }+ /// The notes half of the wiring is a SwiftUI view modifier, which a unit test+ /// cannot mount. Pin it structurally instead, the way `PaywallPresenterTests`+ /// pins the paywall hosts: without this hook the session retargets and the+ /// notes record is simply left behind under a path nothing resolves again.+ @Test("the reader view consumes pending file moves through the claiming entry point")+ func readerViewConsumesPendingFileMoves() throws {+ let source = try Self.readerViewSource()++ #expect(+ source.contains(".onChange(of: session.pendingFileMove, initial: true)"),+ """+ DocumentReaderView must observe `session.pendingFileMove`, and with \+ `initial: true`. It is the only consumer, so without the observation \+ `followFileMove(to:)` retargets the session and the notes record \+ stays filed under the old path; without the initial pass the same \+ thing happens to a move that lands while this view is not mounted, \+ since the session follows moves whether or not one is (T-1881).+ """+ )+ #expect(+ source.contains("notesManager.startMigration("),+ """+ The migration must be started through \+ `NotesManager.startMigration(forMovedFileFrom:toFileURL:sessionID:)`, \+ not by wrapping `migrateNotes` in a task here. The move has already \+ retargeted `session.source`, so the new identity has to be claimed in \+ this main-actor turn — a task's first line is a turn too late, and a \+ reload landing in that gap loads the new identity's empty record over \+ the notes the migration is moving (T-1881).+ """+ )+ #expect(+ source.contains("onMoveMigrationFailed?(session,"),+ """+ A failed migration must be reported, and reported *with its session*. \+ `startMigration` returns `false` when the store write fails, having \+ reverted the in-memory notes to the old identity while \+ `session.source` is already at the new one — discarding that leaves \+ the notes silently detached from the document, where Save As raises \+ "Note Migration Failed" for the same event. The session is what lets \+ the coordinator refuse to raise that alert over a document the user \+ has since moved on to (T-1881).+ """+ )+ #expect(+ source.contains("session.notesRecordFollowedFile(to: move.to)"),+ """+ Success — and only success — must record that the notes record has \+ caught up with the file. Clearing `pendingFileMove` says the view \+ consumed the move, not that the record moved; inferring one from the \+ other left a failed migration reporting the *next* move from a path \+ the record never reached, and the migration finding nothing filed \+ there (T-1881).+ """+ )+ }++ private static func readerViewSource() throws -> String {+ let url = URL(fileURLWithPath: #filePath)+ .deletingLastPathComponent() // prismTests+ .deletingLastPathComponent() // repo root+ .appendingPathComponent("prism")+ .appendingPathComponent("Views")+ .appendingPathComponent("DocumentReaderView.swift")+ return try String(contentsOf: url, encoding: .utf8)+ }+}++// MARK: - Helpers++/// A store that parks `save` *before* it writes.+///+/// `MockNotesStore` cannot reproduce the race this suite needs: its `save` is+/// indivisible from the caller's side, so work landing "inside" a migration+/// always finds the destination record already written and the load it delivers+/// has nothing to get wrong. Parking ahead of the write puts a caller in the+/// state the migration is genuinely in for the length of an iCloud write — notes+/// rebound in memory, nothing on disk yet.+private actor ParkedSaveNotesStore: NotesStoreProtocol {+ var storedNotes: [String: DocumentNotes] = [:]+ var isAvailable: Bool { true }+ private var onSave: (@Sendable () async -> Void)?++ /// One-shot: the setup note's own save must not consume it.+ func setOnSave(_ hook: @escaping @Sendable () async -> Void) {+ onSave = hook+ }++ func preload(_ notes: DocumentNotes) {+ storedNotes[notes.identifier.path] = notes+ }++ func load(for identifier: DocumentIdentifier) async -> DocumentNotes? {+ storedNotes[identifier.path]+ }++ func save(_ notes: DocumentNotes) async throws {+ if let hook = onSave {+ onSave = nil+ await hook()+ }+ storedNotes[notes.identifier.path] = notes+ }++ func delete(for identifier: DocumentIdentifier) async {+ storedNotes.removeValue(forKey: identifier.path)+ }+}++/// A store that behaves the way a **case-insensitive volume** does: one record+/// per identifier once case is folded away, which is what `NotesStore` gets from+/// the filesystem it writes into (APFS and HFS+ are case-insensitive by default+/// on macOS, and iOS has no case-sensitive option at all). `MockNotesStore` is+/// case-*sensitive*, so it cannot show the case-only rename losing its notes —+/// under it, saving `README.md` and deleting `readme.md` really are two files.+private actor CaseFoldingNotesStore: NotesStoreProtocol {+ private var storedNotes: [String: DocumentNotes] = [:]+ var isAvailable: Bool { true }++ /// The filename `NotesStore` would derive, folded the way the volume folds it.+ private func key(_ identifier: DocumentIdentifier) -> String {+ identifier.urlSafeEncoded.lowercased()+ }++ func load(for identifier: DocumentIdentifier) async -> DocumentNotes? {+ storedNotes[key(identifier)]+ }++ func save(_ notes: DocumentNotes) async throws {+ storedNotes[key(notes.identifier)] = notes+ }++ func delete(for identifier: DocumentIdentifier) async {+ storedNotes.removeValue(forKey: key(identifier))+ }+}++/// A stored note, for the tests that seed a record instead of creating one+/// through the manager.+private func makeNote(content: String) -> BlockNote {+ BlockNote(+ blockId: "block-1",+ contextQuote: "Original",+ content: content,+ status: .active,+ createdAt: Date(),+ modifiedAt: Date()+ )+}++/// Two distinct URLs in one directory: where the document was, and where an+/// external rename put it. Neither is written to unless a test says so.+private func movePair() -> (old: URL, new: URL) {+ let directory = FileManager.default.temporaryDirectory+ let stem = UUID().uuidString+ return (+ directory.appendingPathComponent("move-\(stem).md"),+ directory.appendingPathComponent("move-\(stem)-renamed.md")+ )+}++/// Three distinct URLs: an original and two successive renames.+private func moveChain() -> (a: URL, b: URL, c: URL) {+ let directory = FileManager.default.temporaryDirectory+ let stem = UUID().uuidString+ return (+ directory.appendingPathComponent("move-chain-\(stem)-a.md"),+ directory.appendingPathComponent("move-chain-\(stem)-b.md"),+ directory.appendingPathComponent("move-chain-\(stem)-c.md")+ )+}++/// A file-backed session with one note already stored under that file's+/// identifier — the state an external rename arrives into.+@MainActor+private func makeFileSessionWithNote(+ url: URL,+ store: any NotesStoreProtocol+) async -> (session: DocumentSession, manager: NotesManager) {+ let session = DocumentSession(url: url, content: "Original")+ let block = MarkdownBlock.paragraph(markdown: "Original")+ session.parsedBlocks = [block]++ let manager = NotesManager.makeForTesting(store: store)+ await manager.loadNotes(source: session.source, sessionID: session.id, blocks: [block])+ await manager.createNote(+ content: "Note on the original",+ for: block,+ sourceIndex: 0,+ in: MarkdownSectionBuilder.build(from: [block]),+ source: session.source,+ sessionID: session.id+ )+ return (session, manager)+}
diff --git a/specs/bugfixes/follow-external-file-moves/report.md b/specs/bugfixes/follow-external-file-moves/report.mdnew file mode 100644index 00000000..0300ebbc--- /dev/null+++ b/specs/bugfixes/follow-external-file-moves/report.md@@ -0,0 +1,486 @@+# Bugfix Report: Open Documents Do Not Follow External File Moves++**Date:** 2026-09-06+**Status:** Fixed+**Ticket:** T-1881++## Description of the Issue++A file-backed `DocumentSession` kept its original URL after the open file was+renamed or moved by Finder, the Files app, iCloud, or another file provider.+`FileChangeObserver` is an `NSFilePresenter`, but it implemented no+`presentedItemDidMove(to:)`, so it kept answering `presentedItemURL` with the+original location and the session's `source` kept naming it.++**Reproduction steps:**++1. Open a local Markdown file.+2. Rename or move it in Finder (or the Files app).+3. Edit it and use Reload, or just look at the title.++Observed: "Reload failed" for a file that is perfectly readable, the title still+names the old filename, relative images resolve against the old folder, and the+notes record stays filed under a path nothing will resolve again.++**Impact:** Medium. Every URL-derived feature of the open document — reload, the+external-change banner, Save, the window title, relative image and link+resolution, the persisted scroll position, the notes identifier — silently+detaches from the file the user is reading. Recovery required closing and+reopening the document.++## Investigation Summary++- **Symptoms examined:** reload failure after an external rename; stale title;+ stale relative-image base.+- **Code inspected:** `FileChangeObserver`, `DocumentSession`,+ `DocumentLayoutCoordinator.reloadDocument`, `DocumentSource`,+ `DocumentImageSource` / `WebDocumentStateSynchronizer` (T-1784),+ `SecurityScopedResourceLease` (T-1849), `NotesManager.migrateNotes` and+ clipboard-notes Decision 8 (T-2231), `DocumentIdentifierResolver`.+- **Hypotheses ruled out:** that the reload path itself held a stale URL — it+ reads `session.source` each time, so the single stale value is `source`; and+ that the image context needed its own plumbing — `WebDocumentStateSynchronizer`+ already derives it from `source.imageSourceContext` and issues a same-revision+ reload when it changes (T-1784), so it follows for free.++## Discovered Root Cause++`FileChangeObserver` stored its URL in a `let` and implemented only+`presentedItemDidChange()`. The `NSFilePresenter` contract requires a presenter+to retarget `presentedItemURL` when it is told the presented item moved; a+presenter that does not is left registered against a location the coordinator no+longer matches against the file.++**Defect type:** Missing protocol conformance (unimplemented optional+requirement) plus immutable state that made implementing it impossible.++**Why it occurred:** the observer was written for one job — raise a banner when+the file's *contents* change (Req 7.3/7.4) — and the presenter's other half of+the contract, that the item can also *move*, was never in scope. `source` being+`private(set)` and set once at construction encoded the same assumption one layer+up.++## Resolution for the Issue++**Changes made:**++- `prism/Services/FileChangeObserver.swift` — the presented URL is now a+ `Mutex<URL>` rather than a `let`, `presentedItemURL` is `nonisolated` and reads+ it under the lock, and `presentedItemDidMove(to:)` retargets it *before* any+ actor hop (so the presenter never answers with a location the coordinator has+ already superseded) and then reports `(from, to)` to its owner via `onMove` —+ in place when the callback is already on the main thread, which+ `presentedItemOperationQueue = .main` makes the usual case. `recordMove(to:)`+ is the synchronous MainActor seam, mirroring the existing+ `recordExternalChange()`.+- `prism/Models/DocumentSession.swift` — `makeFileObserver(for:)` is now the+ single way an observer is built (both the file initializer and `didSave`), so+ every observer the session owns reports moves. `followFileMove(to:)` retargets+ `source` and publishes `pendingFileMove`, carrying the *origin* forward across+ a second move that lands before the first is consumed. A save or a revert to+ clipboard supersedes an unconsumed move.+- `prism/Views/DocumentReaderView.swift` — consumes `pendingFileMove` and+ migrates the notes, mirroring the existing `pendingSave` hook, with+ `initial: true` so a move landing while the view is unmounted is still+ consumed, and reporting a failed migration through `onMoveMigrationFailed`.+- `prism/Services/NotesManager.swift` — a private+ `migrateNotes(fromMovedFileURL:toFileURL:sessionID:)`, a thin entry into the+ existing migration, reachable only through+ `startMigration(forMovedFileFrom:toFileURL:sessionID:)`, which claims the+ target identity synchronously (see below).+- `prism/ViewModels/DocumentFlowCoordinator.swift` —+ `handleMoveMigrationFailed(notesRemainAt:)`, which raises the existing "Note+ Migration Failed" alert for the move path's version of the same event.++**Approach rationale:** everything URL-derived in the document is a function of+`DocumentSession.source`, so retargeting that one property is the whole fix for+the title, the Save destination, the reload path, the persisted scroll key, and+the rendered page's image context (which `WebDocumentStateSynchronizer` re-bases+from `source.imageSourceContext` and reloads at the same revision — T-1784).+Only the notes needed anything more, because they are filed under a+path-derived identifier.++### Decision: the notes follow the file++A rename is the same identity change as a Save As — one document's notes move+from one file identifier to another — so it gets the same *answer*: the notes+follow, and the record at the old path is retired rather than stranded under a+path nothing will resolve again. It does **not** get the same machinery; the+first four rounds tried to, and "Decision: the move gets its own migration"+below is why that was wrong.++Not migrating was considered seriously and rejected. Notes are reloaded on every+parse revision, keyed off `source` — the property this fix retargets — so+leaving them alone does not leave them *where they were*: the next reload would+look for them under the new location and find nothing, and the notes would+vanish from the pane. The residual risk is a false-positive move notification — and one turned out to+be real rather than residual: **trashing the file arrives as a move.** The claim+that stood here, that a Finder trash reaches a presenter as+`accommodatePresentedItemDeletion` rather than `presentedItemDidMove(to:)`, was+recorded without a source and is false; see "F4" below for the measurement and+the guard.++### Decision: the target identity is claimed synchronously (review round 1)++Retargeting `source` *before* the migration has a price the first pass only+half-accounted for. From the retarget until the migration finishes,+`session.source` names an identity whose notes record does not exist yet, and+everything that resolves notes through `session.source` resolves *that* one.+Note creation was already safe — `noteContainer(for:)` is target-aware via+`migrationDestination(for:)` — but `loadNotes` is not, and the reader loads+notes on every parse revision (`DocumentReaderView`'s+`.task(id: session.parseRevision)`), so any reload landing near the move starts+one.++`migrateNotes` claims the source and clipboard identities (clipboard-notes+Decision 8), never the target. So that load fell through every guard: it read+the target's absent record, `applicableNotes` declined the container the+migration had already rebound, and `clearNoteState()` wiped it — after which the+migration's post-save guard read its own container as replaced and abandoned the+rebind. Empty notes panel, orphaned record under the old path. T-2231 one+identity over. The mutation check reproduces exactly that: `documentNotes` nil,+old record still present.++The fix claims the target for the length of the migration, which is the same+mechanism rather than a second one — `isMigratingAway` retires the load, and+`migrationDestination` redirects the target onto the target, the no-op it should+be. The claim is taken by `startMigration`, not inside `migrateNotes`, because+`migrateNotes` is `async`: its first line runs a turn later than the one the move+was consumed in, and a load starting in that gap sees no claim at all.++**The claim starts where `startMigration` is called, which is not where `source`+flips.** `followFileMove(to:)` runs on the file presenter's callback and only+publishes `pendingFileMove`; `DocumentReaderView`'s `.onChange` calls+`startMigration` in a *later* main-actor turn, the one Observation schedules. So+the window between the flip and that consumption has no claim on the target, and+cannot get one from `startMigration` — that function has not been called yet. It+is **unreachable today rather than covered**, on two independent counts: a move+bumps no `parseRevision`, so the reader's `.task(id: session.parseRevision)`+notes load does not re-fire, and a move deliberately raises no reload banner, so+the user cannot start one either. Nothing else resolves notes between the two+turns. If either of those changes — a move that reparses, or a banner offered on+a rename — the claim has to be taken in `followFileMove` itself, which means+handing `DocumentSession` a reference to the notes manager it does not have+today. That is the price of the flip-then-migrate order, stated rather than+hidden; it was not worth paying for a window nothing can enter.++*Ordering the source flip after the migration instead* was rejected a second+time, and for a stronger reason than the first: it would make the retarget+conditional on a mounted reader view running an async migration, where today it+happens whether or not one is on screen.++### Decision: `pendingFileMove` carries the origin (review round 3)++`.onChange` delivers only the last value a property took before the next body+pass, so two moves landing in one pass — A→B, then B→C — are consumed as a+single notification. Recording the last *hop* there, `(B, C)`, named an+identifier the notes were never filed under: `migrateNotes` evaluated+`notesBelongToSaveChain` against a container still under `resolve(A)`, matched+neither the clipboard identity nor `resolve(B)`, logged "migrateNotes skipped"+and returned **`true`**. The session ended at C, the record stayed at A, and the+next `loadNotes(C)` had `applicableNotes` decline the A container and+`clearNoteState()` empty the pane. This ticket's own symptom, reported as+success — which is why the migration's `Bool` cannot stand in for asserting on+the store.++`followFileMove(to:)` therefore coalesces: `from` is `pendingFileMove?.from ??+oldURL`, and a pair that returns home (A→B→A) coalesces to `nil` rather than a+migration from an identifier onto itself. This is the right shape rather than a+patch — the migration's job is "move the notes from where they are to where the+file is", and A is where they are however many hops it took to reach C.++Ordering is what makes the coalescing meaningful, and it was being thrown away+one layer down: `presentedItemDidMove` hopped through `Task { @MainActor }`, and+two unstructured tasks are scheduled, not ordered. A reversed pair left `source`+at B while the file was at C, because `followFileMove` derives `oldURL` from the+live `source`. The callback already arrives on the main thread+(`presentedItemOperationQueue` is `OperationQueue.main`), so it now reports in+place, with the hop kept only for the off-main case that queue contract says+cannot happen.++### Decision: the move gets its own migration (review round 5)++Rounds 1–4 kept the move path entering `migrateNotes(fromClipboardSession:+previousDestination:toFileURL:)` through its `previousDestination` parameter,+and patched the differences as each one surfaced. Round 5 found three more, all+on the notes data path, and they share one cause: **that function holds three+preconditions a clipboard Save As satisfies and a move does not.** Patching a+borrowed function's violated preconditions one at a time is how a fourth one+gets found in round 6, so the move path now has its own migration —+`NotesManager.startMigration(forMovedFileFrom:toFileURL:)` and its private+`migrateNotesForMovedFile`, in a dedicated extension of the same file.++The preconditions, and what each one cost:++1. **The notes are already in memory.** `migrateNotes` opens on+ `guard var notes = documentNotes else { return true }`. Sound for a clipboard+ document — its record cannot exist unless this session created it, so nothing+ loaded really does mean nothing to move. For a move the record is on disk, and+ "nothing loaded" is the *normal* state whenever the move outruns the first+ `loadNotes` — which is precisely what `.onChange(…, initial: true)` was added+ to catch. `notesManager` is `@State` on `DocumentReaderView`, so at the initial+ pass `documentNotes` is guaranteed nil: the move was consumed, cleared, and+ reported migrated, having moved nothing. The new migration reads the store+ when memory has nothing, so that branch can do its job.+2. **The source identity cannot collide with the target.** A clipboard source is+ `clipboard/<UUID>`; here both sides are file paths.+ `DocumentIdentifier.path` is case-preserving and `urlSafeEncoded` is a+ case-preserving percent-encoding of it, so `readme.md` and `README.md` are two+ identifiers and — on a case-insensitive volume, the default on macOS (APFS,+ also normalization-insensitive) and the only option on iOS — one file.+ `store.save` wrote the record; `store.delete(for: source)`, guarded on string+ inequality, then deleted the file it had just written. Ordinary action, total+ loss, no alert. The guard is now `retiringSourceRecordIsSafe`, which asks the+ *store* whether reading the old identifier back hands us the record we just+ wrote, rather than guessing at the volume's collation rules — so it is right+ for every such aliasing, not just the case one. The Save As migration uses it+ too: a supersession chain reaches `previousDestination` with a file URL like+ any other, so it can hit the same collision.+3. **`session.source` has not flipped yet.** Save As calls `didSave(to:)` only+ after its migration returns, so a racing load or creation resolves an identity+ the migration has already claimed. A move has happened before we are told, so+ both resolve the **target** — and a claim answers a load but cannot answer a+ creation. `noteContainer(for:)` judges the container it *finds* against the+ identity it was asked about; `migrationDestination(for: target)` returns the+ target and redirects nothing; the container in memory is still filed under the+ source, so it reads as another document's and `clearNoteState()` empties the+ pane — after which the migration's post-save guard is *satisfied* by the+ replacement and retires the source record on top of it. Every existing note+ gone, from memory and from disk, on a note created in a one-turn window. This+ is the exact inverse of what round 1's comment claimed ("creation was already+ safe; only loads were exposed"), and it is why `startMigration` now **rebinds+ the loaded container onto the target synchronously**, before returning, in the+ same main-actor turn the move is consumed in. The claim (round 1) is kept and+ extended to the source as well as the target; the rebind is what covers+ creations.++Two smaller omissions of the same kind are fixed alongside. The write gate+`notesBelongToSaveChain`'s own comment declares mandatory — "callers that gate a+*write* on this must additionally require `hasNotes`" — was never applied on the+move path, so an emptied container could atomically replace a record already+filed under the target (a move's target, unlike a Save As destination the user+just picked, can be a path an earlier document left a record under). And the+source identity was never claimed on the move path at all; it is now, alongside+the target.++### Decision: `notesRecordURL`, not `pendingFileMove`, says where the notes are++`followFileMove` derived a move's origin from `pendingFileMove?.from ?? oldURL`,+which conflated two different facts: *where the notes record is* and *whether the+view has consumed a move yet*. The view clears `pendingFileMove` when it+**starts** the migration, so after a failed migration the next move reported an+origin the record had never reached, and the migration then found nothing filed+there and reported success. Restoring `pendingFileMove` on failure is not the fix+— it re-enters `.onChange` and retries in a loop for as long as the store keeps+failing.++`DocumentSession.notesRecordURL` now holds that fact on its own: set at the file+initializer and at `didSave` (the save chain migrates before calling it, and its+failure paths settle the session onto whichever URL the notes ended up under),+cleared on `revertToClipboard`, and advanced by `notesRecordFollowedFile(to:)`+which **only success** calls. `pendingFileMove` goes back to being what its name+says — one unconsumed notification — and is still cleared as soon as the view+starts a migration.++### F4: trashing a file arrives as a move, not as a deletion (measured)++Rounds 1–4 recorded, as fact and with no source, that "a Finder trash is a+coordinated *deletion* (`accommodatePresentedItemDeletion`), not a move". That+was the sole argument that the move path could not be entered by a deletion, and+it is **false**. Measured on macOS 26 with a standalone `NSFilePresenter`+registered against a real file (harness: `presentedItemOperationQueue =+OperationQueue.main`, all three callbacks logged, run once per mode):++| operation | callbacks, in order |+| --- | --- |+| `FileManager.trashItem`, uncoordinated | `presentedItemDidMove(to: ~/.Trash/…)` |+| `trashItem` inside an `NSFileCoordinator` `.forDeleting` claim (the Finder shape) | `accommodatePresentedItemDeletion`, **then** `presentedItemDidMove(to: ~/.Trash/…)` |+| coordinated `.forMoving` rename | `presentedItemDidMove(to: …renamed.md)` |+| uncoordinated `moveItem` | `presentedItemDidMove(to: …renamed.md)` |+| `removeItem` (real unlink) | *none* |++So the move arrives either way, and the coordinated deletion the old note relied+on does not replace it — it merely precedes it. Left unhandled, trashing an open+document retargeted the session onto `~/.Trash/…`, rewrote the notes record under+a Trash path, and deleted the record at the path the file came from: lost outright+on Empty Trash, and lost silently even on Put Back, since nothing would be filed+under the restored path any more.++`DocumentSession.isTrashed(_:)` now refuses to follow a move whose destination+carries a `.Trash` or `.Trashes` path component, so the session stays on the path+the document was opened from — where Put Back returns the file, and where its+notes are waiting. Emptying the Trash leaves a stale URL and a reload that fails,+which is the honest report for a file the user deleted. Detection is by path+component rather than by comparing against `FileManager.url(for: .trashDirectory,+…)` because an item trashed on a secondary volume lands in+`/Volumes/<name>/.Trashes/<uid>/` and an iOS file provider's trash sits inside+its own container: one query cannot name them all, while both spellings are+reserved on every platform Prism runs on.++Honest limit: the measurement is macOS. The iOS Files app was not exercised, and+the guard is a superset — if a provider delivers a deletion some other way, the+move simply never arrives and nothing is followed, which is the safe direction.++### Deliberately unchanged++- **The security-scoped lease (T-1849).** The sandbox extension the session+ holds was issued for the file the user picked and survives its rename. A `URL`+ synthesised from a move notification carries no scope of its own, so starting+ a lease on it returns `false` and gains nothing, while dropping the existing+ one is the only way to actually lose access. So the lease is left alone.++ Honest limit: that reasoning is from the sandbox's documented model, not from+ observation. Whether an existing extension keeps working across an external+ rename cannot be exercised from a unit-test host — the sandbox behaviour that+ issues one is not reachable there, which is why `DocumentSession` carries a+ `fileAccessResourceForTests` seam at all (T-1849). `make build-ios` confirms+ the change compiles for iOS; it says nothing about Files-provider revocation+ semantics. If a provider *does* revoke on rename, the symptom is a reload+ failure after the move, and the fix is a new lease at `followFileMove` — a+ strictly additive change to this one.+- **The recent-files entry.** It keeps naming the path it was opened from.+ Recents staleness is T-1842 / T-2172 / T-2173.+- **The stored scroll position.** `persistScrollPosition` keys off `source`, so+ from here on the position is written under the new location — where a later+ reopen looks for it. The record under the old path is left behind, unread.++**Alternatives considered:**++- *Re-register the presenter (`removeFilePresenter` + `addFilePresenter`) on a+ move* — rejected: `NSFilePresenter`'s documented contract is that the+ presenter updates `presentedItemURL`, and re-registering opens a window in+ which the file is unobserved.+- *Migrate the notes non-destructively (copy, leave the old record)* — rejected:+ it trades one lost strand for two live ones for the same document, and the+ duplicate is written under a path the document has left.+- *Migrate the notes before retargeting `source`, as the Save As flow does* —+ rejected twice: a Save As chooses when it happens, whereas a move has already+ happened by the time we are told about it. Leaving `source` on the old path+ for the length of an iCloud write would mean a Reload in that window reads a+ file that is not there — and it would make the retarget conditional on a+ mounted reader view running an async migration, where today it happens whether+ or not one is on screen. The exposure that order does create is closed by the+ synchronous target claim above.++## Regression Test++**Test file:** `prismTests/ExternalFileMoveTests.swift`++Twenty tests over seven areas: the presenter retargets and reports exactly once+and does not raise the content-change banner; the session's source, title and+image base follow, a self-move is ignored, and a save supersedes an unconsumed+move; two moves before either is consumed carry the origin forward — as a+`pendingFileMove` assertion, and end to end through a real migration and the+reload after it — while a round trip coalesces to nothing; `reloadDocument`+after a rename reads the file at its new location (the headline symptom); the+notes migrate, are found by the reload that follows, and a document with no+notes writes nothing; a reload landing *inside* the migration's save (via a+store that parks ahead of its write) does not wipe the notes, and the target+identity is claimed before the migration task runs at all; and the wiring itself+is covered rather than assumed — two tests drive the real+`presentedItemDidMove(to:)`, one of them for the ordering of a consecutive pair,+and one pins `DocumentReaderView`'s `.onChange(of: session.pendingFileMove,+initial: true)` hook, its failure reporting and its success bookkeeping+structurally, the way `PaywallPresenterTests` pins the paywall hosts. A+direct-invocation test cannot see missing wiring (T-1943).++Round 5 adds ten, one per defect and each mutation-checked against the code it+pins (mutation applied, only the named test fails, mutation reverted):++| test | mutation that must fail it |+| --- | --- |+| `caseOnlyRenameKeepsTheNotes` | `retiringSourceRecordIsSafe` → `source != target` |+| `aMoveMigratesTheStoredRecordWithNothingLoaded` | `notesToMove` requires `documentNotes != nil` |+| `aNoteCreatedWhileTheMigrationRunsKeepsTheExistingNotes` | the synchronous rebind in `startMigration` removed |+| `anEmptyContainerDoesNotOverwriteTheTargetsRecord` | the `hasNotes` write gate removed |+| `trashingTheDocumentIsNotFollowedAsARename`, `perVolumeTrashIsTreatedTheSame` | the `isTrashed` guard removed |+| `aFailedMigrationLeavesTheNextMoveReportingTheOrigin` | origin back to `pendingFileMove?.from ?? oldURL` |+| `moveFailureIsNotReportedOverADifferentDocument` | the `currentSession === session` guard removed |++Plus `aSuccessfulMigrationMovesTheReportedOrigin`, which pins the other side of+`notesRecordFollowedFile` so the failure test cannot be satisfied by never+advancing the record location at all, and+`aReaderMovingOnMidMigrationDoesNotStrandTheOldRecord`, which pins the one place+this migration deliberately diverges from the Save As one after a successful+save: the retirement of the source record is not abandoned when memory has moved+on to a different document, because the write landed either way and the leftover+is the orphan half of this ticket's symptom.++The case-only rename needs a store that behaves as the volume does:+`MockNotesStore` is case-*sensitive*, so under it saving `README.md` and deleting+`readme.md` really are two files and the defect cannot occur.+`CaseFoldingNotesStore` keys on the folded `urlSafeEncoded` filename — the name+`NotesStore` actually derives — which is the one property of the real filesystem+this defect depends on.++The creation test pins the rebind **deterministically**, by asserting the+container's identity on the line after `startMigration` returns with no+suspension in between; asserting only the end state let the mutant pass, because+the ordering of an unstructured task against an `await`ed creation is not fixed.++**Run command:**++```+xcodebuild test -project prism.xcodeproj -scheme prism \+ -destination 'platform=macOS' -configuration Debug \+ -only-testing:prismTests/ExternalFileMoveTests test+```++## Affected Files++| File | Change |+|------|--------|+| `prism/Services/FileChangeObserver.swift` | Mutex-backed presented URL; `presentedItemDidMove(to:)` reporting in place; `onMove`; `recordMove(to:)` |+| `prism/Models/DocumentSession.swift` | `makeFileObserver(for:)`, `followFileMove(to:)` with origin coalescing, `pendingFileMove` |+| `prism/Views/DocumentReaderView.swift` | Consumes `pendingFileMove` (`initial: true`), migrates notes, reports failure |+| `prism/ViewModels/DocumentFlowCoordinator.swift` | `handleMoveMigrationFailed(notesRemainAt:)` |+| `prism/prismApp.swift` | Wires `onMoveMigrationFailed` to the coordinator |+| `prism/Services/NotesManager.swift` | `startMigration(forMovedFileFrom:toFileURL:sessionID:)`, private `migrateNotes(fromMovedFileURL:toFileURL:sessionID:)` |+| `prism/Localizable.xcstrings` | Alert message for a failed move migration |+| `prismTests/ExternalFileMoveTests.swift` | New regression suite |+| `CHANGELOG.md` | `[Unreleased] / Fixed` entry |++## Verification++**Automated:**++- Regression suite passes (31 tests), of which ten are mutation-checked — the+ seven in the table above plus the round 1/3 pair (removing the synchronous+ target claim fails both race tests, with `documentNotes` nil and the old+ identifier's record still on disk; reverting `followFileMove` to record the+ last hop fails the two-move migration test with the migration itself still+ returning `true`).+- 234 tests pass across `ExternalFileMoveTests`, `NotesManagerClipboardTests`,+ `ConsecutiveSaveAsTests`, `DocumentSessionTests`, `FileChangeObserverTests`,+ `SaveFlowIntegrationTests`, `ClipboardPasteIntegrationTests`,+ `StatePersistenceIntegrationTests`, `LateSaveFinalisationTests`,+ `NotesManagerLoadRaceTests` and `DocumentLayoutCoordinatorReloadTests`,+ confirmed by `Tools/check-test-results.sh`. The clipboard and save-chain+ suites matter here because `retiringSourceRecordIsSafe` replaced the delete+ guard on *both* migrations.+- `make lint`, `make build-macos`, `make build-ios`, `make verify-test-isolation`.++**Not run**: the per-locale sweep (`make test-locales`) — a local pre-merge step+since PR #414. This change adds one user-visible string (the failed-move+alert), so it is worth running before merge rather than skipping on the+"unaffected" argument the earlier rounds had.++## Prevention++- A protocol whose optional requirements carry real contracts (`NSFilePresenter`,+ `NSFileManagerDelegate`) is worth reading in full when adopting it: the missing+ half here was not a bug in the code that existed, it was code that was never+ written.+- Anything derived from a document's URL should be derived from+ `DocumentSession.source`, never captured. Every consumer that already did+ (title, image context, scroll key, reload) followed this move for free; the+ only one that needed work was the notes identifier, which is written into a+ persisted record rather than read on demand.++## Related++- T-1849 (session-lifetime security-scoped lease), T-1784 (shared image-source+ box), T-2231 / clipboard-notes Decision 8 (migration claims), T-1812+ (consecutive Save As).+- Out of scope, still open: T-1842, T-2172, T-2173 (recent-file title/path+ staleness).
diff --git a/CLAUDE.md b/CLAUDE.mdindex 216955cc..55398d14 100644--- a/CLAUDE.md+++ b/CLAUDE.md@@ -41,9 +41,21 @@ xcodebuild test -project prism.xcodeproj -scheme prism \ 2. `MarkdownDocument` (FileDocument) validates and loads markdown content (10MB limit, UTF-8 only) 3. For remote URLs, `URLDocumentLoader` downloads content via ephemeral URLSession with streaming (10MB limit), and `GitHubURLTransformer` converts GitHub blob URLs to raw URLs 4. `DocumentReaderView` selects a layout (`CompactDocumentLayout` or `RegularDocumentLayout`) that parses content into blocks via `MarkdownBlockParser` and renders them through the WebKit document path (`WebDocumentView`/`WebDocumentController` over `BlockHTMLEmitter`; see Markdown Rendering below)-5. `FileChangeObserver` (NSFilePresenter) monitors files and shows `ReloadBanner` on external changes; URL-sourced documents use a manual Refresh button instead+5. `FileChangeObserver` (NSFilePresenter) monitors files and shows `ReloadBanner` on external changes; URL-sourced documents use a manual Refresh button instead. It also implements `presentedItemDidMove(to:)`, which is how an open document follows an external rename — see External File Moves below (T-1881) 6. A file-backed `DocumentSession` holds its own `SecurityScopedResourceLease` for the session's whole lifetime — started while the caller that produced the URL (file importer, resolved recent-file bookmark, or a Save As's resolved export bookmark) still holds its own live scope, so it outlives that caller's transient access and keeps `FileChangeObserver` and reload working after it returns (T-1849) +### External File Moves (T-1881)++`FileChangeObserver.presentedItemDidMove(to:)` retargets the presented URL synchronously, under a `Mutex` and before any actor hop, because `presentedItemURL` is read off-actor by the coordination machinery. It then reports the move to `DocumentSession.followFileMove(to:)`, which moves `source`. That one property is the whole fix for most of the symptom: the title, the Save destination, `reloadDocument`, the persisted scroll key and the page's image context (re-based by `WebDocumentStateSynchronizer` from `source.imageSourceContext`) are all derived from it. The report is made in place when the callback is already on the main thread — `presentedItemOperationQueue` is `OperationQueue.main` — because two `Task { @MainActor }` hops are scheduled, not ordered, and a reversed A→B, B→C pair would leave `source` at B while the file is at C.++Only the notes do not follow `source`: they are filed under a path-derived identifier. So the move is republished as `session.pendingFileMove` and `DocumentReaderView`'s `.onChange(of:initial:)` migrates them through the same machinery a Save As uses, since a rename is the same identity change. Three things about that hop are load-bearing:++- **The entry point is `NotesManager.startMigration(forMovedFileFrom:toFileURL:sessionID:)`, never `migrateNotes` (now `private`).** The flip-then-migrate order means `session.source` names an identity whose notes record does not exist yet, and `migrateNotes` claims the source and clipboard identities (clipboard-notes Decision 8) but never the target. A `loadNotes` resolving through the retargeted source — the reader's own `.task(id: session.parseRevision)`, from a reload landing near the move — therefore fell through every guard, read the target's absent record, and `clearNoteState()` wiped the notes mid-migration; the post-save guard then read its own container as replaced and abandoned the rebind: empty panel, orphaned record under the old path (T-2231 one identity over). `startMigration` claims the target synchronously, in the turn the move is *consumed* in — `migrateNotes` is `async`, so its own first line is a turn too late. **That claim does not reach back to the `source` flip**, which happens in the presenter's callback, an earlier turn: the stretch between the two has no claim. It is unreachable rather than covered — a move bumps no `parseRevision` and deliberately raises no reload banner, so nothing resolves notes in it. Change either and the claim has to move into `followFileMove`.+- **`pendingFileMove` carries the origin, not the last hop.** `.onChange` delivers only the last value before the next body pass, so A→B then B→C is consumed once. Recording `(B, C)` made `migrateNotes` test `notesBelongToSaveChain` against a container still under `resolve(A)`, match nothing, skip — and return `true`, stranding the notes at A while reporting success. `followFileMove` coalesces to `(A, C)`, and a round trip A→B→A to nothing at all.+- **`initial: true`, and the result is not discarded.** The session follows a move whether or not a reader view is mounted, so without the initial pass a move landing off screen is never consumed. And `startMigration` returning `false` means the store write failed and the notes reverted to the old identity while `source` is at the new one — reported through `onMoveMigrationFailed` to the same "Note Migration Failed" alert Save As uses.++The security-scoped lease is deliberately untouched (the extension was issued for the file and survives its rename; a URL synthesised from a move notification carries no scope of its own) and so is the recent-files entry (T-1842/T-2172/T-2173).+ ### Markdown Rendering (WebKit document path, T-1542) The document is rendered by WebKit-for-SwiftUI (`WebView`/`WebPage`). The SwiftUI/Textual in-flow renderer was retired in the T-1542 cutover — there is no legacy path or render-policy switch any more (see the CHANGELOG `[Unreleased]` "Changed" entry). Native stays the source of truth: parsing, search counting, notes logic, and persistence all run natively; the web view only renders HTML and reports interactions back over the bridge.
diff --git a/CHANGELOG.md b/CHANGELOG.mdindex c9f6d3a5..8b947fb7 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Tapping a note in the compact Notes panel or the regular Notes sidebar now scrolls to the block that note is actually anchored to, instead of always the first occurrence of identical content elsewhere in the document (T-1929). Both note UIs navigated by passing the note's bare content-hash block id, which `BlockDOMID` resolves to its first (or first-visible) occurrence by design — the note's own stored heading path, already recorded for note storage/display disambiguation (T-209), was never consulted. Navigation now resolves the note's specific occurrence against that heading path and builds the same verified composite target TOC/search/scroll-restore already use, falling back to the previous first-occurrence behaviour only when a note carries no heading path (legacy notes) or its heading path no longer matches any occurrence (the block was relocated). A table-row or list-item note anchor resolves to its parent block's correct occurrence, since rows and items are not independently scrollable.+- An open document now follows its file when you rename or move it in Finder, the Files app, or another file provider (T-1881). The app watches the open file through a file presenter, and that presenter never implemented the half of its contract that deals with the file moving: it kept pointing at where the file used to be, and so did the document. Reload then failed on a file that is perfectly readable, the title kept the old name, images beside the document were looked for in the old folder, and the notes were left filed under a path nothing would ever look up again. The presenter now retargets itself the moment it is told the file moved, and the document follows it — which is what carries the title, the Save destination, Reload, the remembered reading position and the page's image resolution across the move, since all of them are derived from the one property. The notes move with the document, and the record left at the old path is retired rather than stranded there. Moving them is its own operation rather than a reuse of the one Save As runs, because three things a Save As can take for granted are not true of a rename, and each of them lost notes: a renamed file's notes may not be loaded yet (a rename arriving before you have opened the notes pane, or before the document has finished loading, used to be reported as migrated while nothing moved); an old and a new name can be the same file, so renaming `readme.md` to `README.md` used to write the notes and then delete the file it had just written, taking every note with it; and the document is already at its new name by the time the app is told, so a note written in the instant the rename lands used to clear every note already on the document. Trashing an open document is no longer mistaken for a rename either — it arrives as one, measurably, so the notes would have been rewritten under a path inside the Trash and the ones at the real path deleted; the document now stays where it was, which is where Put Back returns it and where its notes are waiting. Two things deliberately do not move: the Recent Files entry still names the path you opened from (that is T-1842 / T-2172 / T-2173), and the security-scoped access the session holds is left alone — it was granted for the file itself and survives the rename, whereas releasing it is the one way to actually lose access to the file. Two files moved in quick succession — a rename followed by a drag into another folder, or an iCloud reorganisation — are followed all the way, with the notes taken from where they actually are rather than from the intermediate location the document only passed through; before this they were left behind at the original path while everything reported success. And if the notes cannot be written to their new location, you are now told so, with the same alert Save As raises, instead of the notes quietly ceasing to be the document's. - `HTMLImageSourceRewriter` no longer re-emits a mediated `src`/`srcset` value with an embedded quote character left unescaped (T-1942). The rewriter always re-emits these attributes double-quoted, but a value could still contain a raw `"`: the `rewrite` closure's `data:` passthrough hands unencoded `data:` URIs back unchanged, and `rewriteSrcset` re-joins a candidate's descriptor half — never passed through `rewrite` — verbatim. Either one could terminate the re-emitted attribute early, splicing the remainder into attribute position (e.g. `srcset='a.png 1x" onerror=… z='`). The value is now escaped for its double-quoted context before being written, so any quote characters it carries round-trip intact instead of breaking out. The escape leaves a character reference the value already carries alone (the `data:` passthrough re-emits the attribute's text as written, references undecoded, and the browser decodes it once), so an SVG `data:` URI whose author correctly wrote `&amp;` for a literal ampersand still renders that ampersand rather than the reference. This is defence in depth rather than a live exploit: the subsequent `HTMLSanitizer` (SwiftSoup) pass is the actual security boundary and already reduced the broken-out remainder to non-allowlisted junk. - A document opened from a URL that redirects now resolves its relative images and links against the address the content was actually served from, not the one you typed (T-1810). A version alias, a shortened link or a page that has moved to another folder or host sends the request on to its final address, and the app kept the original one as the base for everything relative in the document, so every image beside it was looked up in the wrong place and showed the error placeholder. The address shown in the title, in recents and in the notes store is unchanged: it stays the one you opened. Refreshing such a document asks for the address you opened again — not for wherever it redirected to last time — and follows the redirect afresh, so an alias that has been re-pointed since the document was opened picks up its new target, and the image base moves with it; the refresh costs one page reload, not one for the new content and another for the new base. - SVG diagrams that adapt to light/dark mode via CSS `@media (prefers-color-scheme: dark)` now rasterize using the appearance Prism actually asked for, instead of whatever the offscreen render window (macOS) or app (iOS) happened to be reporting at the time (T-1896). `SVGRenderer` accepted a `colorScheme` parameter but never used it past the cache key, so a light-appearance SVG could render with its dark-mode colors — or vice versa — while being cached under the *other* scheme's key, compounding the mismatch on the next lookup. The renderer now forces its WebView's own appearance (`overrideUserInterfaceStyle` on iOS, `NSAppearance` on macOS) to match the requested scheme before every render, not only when the WebView is first created — the pool reuses WebViews across calls that can request a different scheme each time.
I traced the A→B / B→C interleaving from the code before either subagent reported, and the code-quality agent arrived at the same interleaving independently. That convergence is the reason it is written up as Major rather than as a theoretical window — but it is also why the counter-argument matters: it is a narrower instance of the symptom the PR removes, and on main the first move already loses the notes. Verify the timing assumption yourself if you want to size it: the lossy ordering needs the second notification delivered after migration 1's store.delete and before its awaiting continuation runs, or delivered earlier with the SwiftUI body pass landing after that delete.
.onChange(initial:) actions run at appearance, before .task starts, so on a fresh mount the migration's claims are usually in place when the reader's own loadNotes reaches its entry guard — which retires it. Worth confirming on a real device that a document reopened after an off-screen move shows its notes without a further reload.
I ran one macOS test target (the tool budget for this review). make build-ios and the iOS simulator suite were not run here; the report says the author ran make build-ios. The code adds no platform conditionals and Synchronization.Mutex is available on both targets, so the risk is low, but the Files-app move shape and the provider trash container are untested on device.
Per PR #414 it is a local pre-merge step and CI runs no tests at all. My targeted run happened to execute all four locale configurations for the six suites I selected (72 tests each, all passing), which covers the new string's suites but not the rest of the sweep.