Pre-push review: T-1865 replies to imported-only notes

diff --git a/prism/Services/NotesManager+DocumentLevel.swift b/prism/Services/NotesManager+DocumentLevel.swiftindex 27e9567..df1afa2 100644--- a/prism/Services/NotesManager+DocumentLevel.swift+++ b/prism/Services/NotesManager+DocumentLevel.swift@@ -87,28 +87,32 @@ extension NotesManager {         await persistNotes(notes)     } +    /// Resolves the current document's note-creation context.+    ///+    /// A document with only imported notes (never any user notes) has+    /// `documentNotes == nil`; falls back to `cachedDocumentIdentifier`+    /// (populated by `loadNotes` regardless of whether the store had prior+    /// notes) so creation can still synthesize a `DocumentNotes` container+    /// (T-1865). Returns nil when no document has been loaded.+    private var currentDocumentContext: (identifier: DocumentIdentifier, displayName: String)? {+        if let notes = documentNotes {+            return (identifier: notes.identifier, displayName: notes.displayName)+        }+        guard let identifier = cachedDocumentIdentifier else { return nil }+        return (identifier: identifier, displayName: documentDisplayName)+    }+     /// Creates a reply using the current document's stored context.     /// Used by NotesPanel and SidebarNotesView which don't have direct access to the document source.     func createReply(content: String, to parentNote: BlockNote) async {-        guard iCloudAvailable, let notes = documentNotes else { return }-        let context = (identifier: notes.identifier, displayName: notes.displayName)+        guard iCloudAvailable, let context = currentDocumentContext else { return }         await createReply(content: content, to: parentNote, context: context)     }      /// Creates a document-level note using the current document's stored context.     /// Used by NotesPanel and SidebarNotesView.     func createDocumentNote(content: String) async {-        guard iCloudAvailable else { return }--        let context: (identifier: DocumentIdentifier, displayName: String)-        if let notes = documentNotes {-            context = (identifier: notes.identifier, displayName: notes.displayName)-        } else if let identifier = cachedDocumentIdentifier {-            context = (identifier: identifier, displayName: documentDisplayName)-        } else {-            return-        }-+        guard iCloudAvailable, let context = currentDocumentContext else { return }         await handleDocumentNoteCreation(content: content, context: context)     } }

diff --git a/prismTests/NotesManagerDocumentLevelTests.swift b/prismTests/NotesManagerDocumentLevelTests.swiftindex bba0dea..3893dbd 100644--- a/prismTests/NotesManagerDocumentLevelTests.swift+++ b/prismTests/NotesManagerDocumentLevelTests.swift@@ -322,4 +322,38 @@ struct NotesManagerDocumentLevelTests {         await manager.clearResolved()         #expect(manager.documentNotes?.notes.isEmpty == true)     }++    // MARK: - T-1865: Reply on Imported-Only Document++    /// A document that has only imported notes (no user notes ever saved) has+    /// `documentNotes == nil`. `createReply(content:to:)` — the overload used+    /// by NotesPanel/SidebarNotesView — used to guard on `documentNotes` being+    /// non-nil and return early in that case, silently discarding the reply+    /// instead of creating a `DocumentNotes` container the way the sibling+    /// `createDocumentNote(content:)` and the source-aware `createReply(content:to:source:sessionID:)`+    /// overloads both do.+    @Test @MainActor+    func testCreateReply_createsContainerWhenDocumentNotesIsNilImportedOnly() async throws {+        let store = MockNotesStore()+        let manager = NotesManager.makeForTesting(store: store)++        // Establish cachedDocumentIdentifier without ever creating user notes,+        // mirroring a document that only has imported notes.+        await manager.loadNotes(source: .file(url: makeURL()), sessionID: UUID(), blocks: [])+        #expect(manager.documentNotes == nil, "Precondition: no user notes exist for this document")++        let importedRoot = makeNote(+            blockId: BlockNote.documentSentinelId,+            content: "Imported root",+            author: "Alice"+        )++        await manager.createReply(content: "My reply", to: importedRoot)++        let notes = manager.documentNotes?.notes ?? []+        try #require(notes.count == 1)+        #expect(notes.first?.content == "My reply")+        #expect(notes.first?.threadId == importedRoot.noteHash)+        #expect(await store.saveCount == 1, "Reply must be persisted, not silently discarded")+    } }

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 5224d5a..67e327a 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0  ### Fixed +- Replying to a document-level note is no longer silently discarded on a document that has only imported notes (T-1865). NotesPanel and SidebarNotesView create replies through a convenience method that used the document's saved user notes as its source of context; on a document where no user note had ever been created — only imported ones — that context was `nil`, so the guard returned early before the reply was ever built, leaving the tap with no visible effect and nothing written to disk. The method now falls back to the document's cached identifier, the same fallback its sibling document-note-creation method already used, so a reply always creates the note container it needs. - Choosing where to go while a document reloads now takes you there (T-1975). If a file changed on disk — or a URL document was refreshed — while you had it open, and during the moment the app spends preparing the new version you picked a table-of-contents entry, tapped a note, followed a link to a heading, or stepped to a search match, the reloaded document appeared at your saved reading position instead. What you chose was handed to the copy still on screen, which was about to be replaced, so nothing was left to say where you had asked to go and restoring your place won — and on a large document, where preparing the new version takes longest, that window is at its widest. The document on screen is now treated as superseded from the moment a reload starts rather than from the moment the new version is ready, so anything you choose in between is held for the version that is coming and takes precedence over your saved place, exactly as it already did when you chose a moment later. This holds when a file changes twice in quick succession, so a second reload beginning before the first has finished preparing still takes you where you asked rather than back to your saved place. Reloads you did not navigate during still return you to where you were reading, and once the reloaded document has taken you where you asked, the next reload restores your place normally. Scrolling while a reload prepares still counts too, however you do it — dragging, a trackpad or wheel, **Page Up** and **Page Down**, or **Scroll to Top** and **Scroll to Bottom** — because the document stays in front of you and stays scrollable the whole time: the place you scroll to is the place you are returned to. - Changing the reading font or text size no longer moves you somewhere else in the document (T-1965). Both settings already applied without reloading, but they reflow the whole document and nothing put you back afterwards: raising **Larger Text** to an accessibility size makes every block roughly three times as tall, so the text you were reading slid off the bottom of the screen and left you looking at something you had already been through. The app then recorded that new spot as where you were reading, so closing and reopening the document returned you to it as well. Your place is now kept across the change — including how far into a paragraph you were, so the same words stay in front of you rather than merely the same paragraph starting at the top — and a place you were never reading can no longer be saved while the document settles. Jumping somewhere while the change is settling wins: a table-of-contents entry, a link, a note, or a search match all take you where you asked, and the re-anchoring steps aside. Collapsing the section you were reading during the change leaves you at its heading rather than at content that is no longer shown. - A document that goes blank because its rendering process stopped now restores itself (T-1943). The app has always been able to recover from this — it reloads the document and puts back your theme, your reading position, your note markers, and any active search highlights — but nothing was ever watching for the rendering process to stop, so the recovery never actually ran. A large or image-heavy document whose renderer was shut down under memory pressure therefore showed an empty page, with no error and no way back except closing the file and opening it again. The app now watches for it and recovers on the spot. An ordinary failure to load — a link that goes nowhere, an image that cannot be fetched — is told apart from a stopped renderer, so it neither causes a needless reload nor stops the app watching for a real one afterwards. The recovery also covers its own failure: if the reload it starts cannot itself load the document, that counts as the recovery failing and is tried again, instead of leaving the page blank with nothing running. A reload that neither succeeds nor fails — one that simply never finishes — is covered too: it is given a generous time limit, well beyond what even a large document takes to appear, and is then treated as a failed recovery and tried again rather than leaving the page blank indefinitely. If reloading repeatedly fails to bring the document back, the app stops retrying rather than reloading over and over — and says so, with a banner offering to reload. Taking that reload also restores the document's ability to recover on its own again, so giving up is never permanent while the file stays open.

diff --git a/docs/agent-notes/notes-system.md b/docs/agent-notes/notes-system.mdindex 6d6c5ba..94704e4 100644--- a/docs/agent-notes/notes-system.md+++ b/docs/agent-notes/notes-system.md@@ -69,7 +69,7 @@ All public CRUD and loading methods accept `DocumentSource` + `UUID` (session ID  `NoteAnchor` (in `NoteModels.swift`) replaces the old optional parameters for sub-block targeting. The `handleNoteCreation` switch dispatches to the correct private creation method based on the anchor case. -Internally, `resolveNoteContext(source:sessionID:)` maps `DocumentSource` to `(DocumentIdentifier, displayName)`. Convenience methods `createReply(content:to:)` and `createDocumentNote(content:)` (no source params) use the already-loaded `documentNotes` context for NotesPanel/SidebarNotesView.+Internally, `resolveNoteContext(source:sessionID:)` maps `DocumentSource` to `(DocumentIdentifier, displayName)`. Convenience methods `createReply(content:to:)` and `createDocumentNote(content:)` (no source params, for NotesPanel/SidebarNotesView) resolve context via the shared `currentDocumentContext` helper in `NotesManager+DocumentLevel.swift`: the already-loaded `documentNotes` when available, else `cachedDocumentIdentifier`/`documentDisplayName` (populated by `loadNotes` regardless of whether the store had prior notes) so a `DocumentNotes` container is created on first use — a document with only imported notes has `documentNotes == nil` (T-1865).  `updateNote`, `deleteNote`, `toggleStatus`, `reattachNote` operate on the already-loaded `documentNotes.identifier` and need no source parameter. 

diff --git a/specs/bugfixes/replies-to-imported-only-notes-discarded/report.md b/specs/bugfixes/replies-to-imported-only-notes-discarded/report.mdnew file mode 100644index 0000000..133ef95--- /dev/null+++ b/specs/bugfixes/replies-to-imported-only-notes-discarded/report.md@@ -0,0 +1,82 @@+# Bugfix Report: Replies to Imported-Only Document Notes Are Silently Discarded++**Date:** 2026-08-10+**Status:** Fixed++## Description of the Issue++Replying to a document-level note (via NotesPanel or SidebarNotesView) silently did nothing when the current document had only imported notes and no user-created notes.++**Reproduction steps:**+1. Open a document that has imported document-level notes (e.g., extracted `[!COMMENT]` blocks) but has never had a user note created in it — `documentNotes` stays `nil` for the whole session.+2. Open the document-level note thread in NotesPanel (iOS) or SidebarNotesView (macOS/iPad) and reply to the imported root note.+3. The reply UI accepts the input, but no reply is created, nothing is persisted, and no error is surfaced.++**Impact:** Low-to-moderate severity, silent data loss. Any user attempting to reply to an imported note on a document without prior user notes loses their reply with no feedback.++## Investigation Summary++- **Symptoms examined:** `createReply(content:to:)` in `NotesManager+DocumentLevel.swift` is the overload NotesPanel/SidebarNotesView call (they have no direct access to the document source needed by the source-aware overload).+- **Code inspected:** `NotesManager+DocumentLevel.swift` (`createReply(content:to:)`, `createDocumentNote(content:)`), `NotesManager.swift` (`createReply(content:to:source:sessionID:)`, `createReply(content:to:context:)`, `loadNotes`, `cachedDocumentIdentifier`).+- **Hypotheses tested:** Confirmed the shared implementation `createReply(content:to:context:)` already builds a fallback `DocumentNotes` container when `documentNotes` is nil (`notes.notes.append` on a freshly constructed container) — so the bug is isolated to the caller guard, not the shared logic.++## Discovered Root Cause++`createReply(content: String, to parentNote: BlockNote)` guarded with `guard iCloudAvailable, let notes = documentNotes else { return }`. When `documentNotes` is `nil` (a document that only ever had imported notes), the `let notes = documentNotes` binding fails and the function returns early — before it ever reaches the shared `createReply(content:to:context:)` implementation that knows how to synthesize a `DocumentNotes` container.++Its sibling, `createDocumentNote(content:)` in the same file, already handles this correctly: when `documentNotes` is nil it falls back to `cachedDocumentIdentifier`/`documentDisplayName` (populated by `loadNotes` regardless of whether the store had any notes) before proceeding.++**Defect type:** Logic error — early-return guard used a wrong/incomplete precondition instead of the fallback the sibling method already used.++**Why it occurred:** `createReply(content:to:)` was likely written by directly reusing `documentNotes` as "the" source of context without noticing that `createDocumentNote(content:)`, added in the same file, already needed to solve the identical nil-context problem via `cachedDocumentIdentifier`.++**Contributing factors:** No test previously exercised `createReply(content:to:)` against a `documentNotes == nil` precondition — all existing document-level reply tests (`testDocumentNoteReply_sameTypeSentinel`, etc.) first call `createDocumentNote` to establish a root note, which as a side effect always populates `documentNotes`, masking the guard's bug.++## Resolution for the Issue++**Changes made:**+- `prism/Services/NotesManager+DocumentLevel.swift` — `createReply(content:to:)` now mirrors `createDocumentNote(content:)`: build `context` from `documentNotes` when available, else fall back to `cachedDocumentIdentifier`/`documentDisplayName`, and only return early if neither is available.++**Approach rationale:** The shared `createReply(content:to:context:)` implementation already does the right thing given a context; the bug was purely in how the caller derived (or failed to derive) that context. Mirroring the exact fallback pattern already used one function below keeps the file internally consistent and requires no changes to the shared logic.++**Alternatives considered:**+- Changing the shared `createReply(content:to:context:)` to accept an optional context and resolve it internally — rejected: it already takes an explicit context by design (used directly by the source-aware overload with its own resolved context), so pushing resolution logic into it would blur responsibilities.+- Making `documentNotes` non-optional / auto-vivified on load — rejected: much larger blast radius across the codebase for a fix that only needs to happen at the two document-level convenience call sites.++## Regression Test++**Test file:** `prismTests/NotesManagerDocumentLevelTests.swift`+**Test name:** `testCreateReply_createsContainerWhenDocumentNotesIsNilImportedOnly`++**What it verifies:** Loads notes for a document with no saved user notes (`documentNotes` stays `nil`, `cachedDocumentIdentifier` gets populated by `loadNotes`), constructs an imported-style root `BlockNote`, then calls `createReply(content:to:)`. Asserts the reply is appended to a newly created `documentNotes` container, has the correct `threadId`, and is persisted (`store.saveCount == 1`) — i.e. is not silently discarded.++**Run command:**+```bash+xcodebuild test -project prism.xcodeproj -scheme prism \+  -destination 'platform=macOS' \+  -only-testing:prismTests/NotesManagerDocumentLevelTests/testCreateReply_createsContainerWhenDocumentNotesIsNilImportedOnly test+```++## Affected Files++| File | Change |+|------|--------|+| `prism/Services/NotesManager+DocumentLevel.swift` | Fixed `createReply(content:to:)` to fall back to `cachedDocumentIdentifier`/`documentDisplayName` when `documentNotes` is nil, mirroring `createDocumentNote(content:)` |+| `prismTests/NotesManagerDocumentLevelTests.swift` | Added regression test for the imported-only-document reply path |++## Verification++**Automated:**+- [x] Regression test passes+- [x] Full unit test suite passes (`make test-quick`)+- [x] Linter passes (`make lint`)++## Prevention++**Recommendations to avoid similar bugs:**+- When two sibling convenience methods (`createDocumentNote(content:)` / `createReply(content:to:)`) both need to derive document context from possibly-nil `documentNotes`, factor the fallback into one shared helper rather than duplicating the `if let ... else if let cachedDocumentIdentifier ... else return` chain — a future third caller could otherwise reintroduce the same bug independently.+- Regression tests for document-level note operations should include an explicit `documentNotes == nil` precondition case (imported-only document), not just the common path where a prior `createDocumentNote` call incidentally populates it.++## Related++- Transit ticket: T-1865

diff --git a/specs/bugfixes/replies-to-imported-only-notes-discarded/implementation.md b/specs/bugfixes/replies-to-imported-only-notes-discarded/implementation.mdnew file mode 100644index 0000000..579148b--- /dev/null+++ b/specs/bugfixes/replies-to-imported-only-notes-discarded/implementation.md@@ -0,0 +1,50 @@+# Implementation Explanation: Replies to Imported-Only Document Notes (T-1865)++Generated during pre-push review of branch `T-1865/bugfix-replies-to-imported-only-notes-discarded`.++## Beginner Level++### What Changed+Prism lets you attach notes to a document, and other people's notes can be imported into it (for example comment blocks that came along with the file). You can reply to any note. Before this fix, replying to an imported note did nothing — no error, no reply — if you had never written a note of your own in that document. Now the reply is created and saved as expected.++### Why It Matters+A user typed a reply, tapped send, and it vanished. That is silent data loss: the worst kind of bug, because nothing tells you it happened.++### Key Concepts+- **Document-level note**: a note attached to the whole document rather than a specific paragraph.+- **Imported note**: a note that came from outside (extracted from the file), as opposed to one you created.+- **`DocumentNotes` container**: the on-disk record holding all your notes for one document. It is only created the first time you write a note. A document where you've only ever *seen* imported notes has no container yet — and the reply code refused to work without one, instead of creating it.++## Intermediate Level++### Changes Overview+- `prism/Services/NotesManager+DocumentLevel.swift`: `createReply(content:to:)` — the no-source-parameter convenience overload used by `NotesPanel` (iOS) and `SidebarNotesView` (macOS/iPad) — no longer requires `documentNotes` to be non-nil. Context resolution now goes through a new private computed property `currentDocumentContext`, shared with `createDocumentNote(content:)`.+- `prismTests/NotesManagerDocumentLevelTests.swift`: regression test `testCreateReply_createsContainerWhenDocumentNotesIsNilImportedOnly`.+- `CHANGELOG.md`, bugfix report, and `docs/agent-notes/notes-system.md` updated.++### Implementation Approach+The shared implementation `createReply(content:to:context:)` in `NotesManager.swift` already synthesized a `DocumentNotes` container when `documentNotes` was nil (`documentNotes ?? DocumentNotes(identifier:displayName:)`); it just needed a context (identifier + display name). The bug was entirely in the caller: `guard iCloudAvailable, let notes = documentNotes else { return }` bailed out before reaching that logic. The fix derives the context from `documentNotes` when present, else from `cachedDocumentIdentifier`/`documentDisplayName` — both populated unconditionally by `loadNotes` regardless of whether the store had any notes. This is exactly the fallback the sibling `createDocumentNote(content:)` already used; during review the now-identical chain was extracted into one `currentDocumentContext` helper so a future third caller cannot reintroduce the bug.++### Trade-offs+- **Fix the caller, not the shared implementation**: making `createReply(content:to:context:)` accept an optional context and resolve it internally was rejected — the source-aware overload passes its own resolved context by design, and blurring that would spread resolution logic across layers.+- **No auto-vivification of `documentNotes`**: making the container always exist after load would have a much larger blast radius than a two-call-site context fix.++## Expert Level++### Technical Deep Dive+- The reply inherits `parentNote.blockId` (the document sentinel for document-level notes) and `threadRoot = parentNote.threadId ?? parentNote.noteHash`, so replying to an imported *root* correctly threads on the root's `noteHash`. The parent is treated as an opaque value — it is never looked up in `importedNotes` — which is why the fix needs no imported-collection awareness.+- Actor safety: `NotesManager` is `@MainActor`; `currentDocumentContext` is read and the container mutation in the shared implementation completes synchronously before the first suspension point (`await persistNotes`), so a concurrent `loadNotes` cannot interleave between context capture and container creation. Exactly one `store.save` occurs (asserted by the regression test).+- Staleness: `cachedDocumentIdentifier` and `documentPath` (backing `documentDisplayName`) are set together at the top of every `loadNotes` and kept in lockstep by `migrateNotes`, so a non-nil identifier implies a current display name.++### Architecture Impact+Every other `guard ... documentNotes else { return }` in `NotesManager` operates on an existing note by ID (update/delete/toggle/reattach), where nil correctly means "nothing to do". Only the two no-source *creation* conveniences needed the fallback; both now share `currentDocumentContext`. The source-aware creation entry points resolve context from `DocumentSource` via `resolveNoteContext` and were never susceptible.++### Potential Issues+- Silent no-op remains when iCloud is unavailable or no document has been loaded — consistent with the house convention throughout `NotesManager`, but still invisible to the user; surfacing an error is a separate design discussion.+- The regression test establishes the `documentNotes == nil` precondition via `loadNotes` against an empty store but does not register the imported root via `setImportedNotes`; functionally equivalent today because the parent is opaque, but if reply creation ever starts consulting `importedNotes`, the test should be extended.++## Completeness Assessment++- **Fully implemented**: the reply fallback for imported-only documents; regression test covering the nil-`documentNotes` path including persistence; duplication removed via `currentDocumentContext`; CHANGELOG, bugfix report, and agent-notes updated.+- **Partially implemented**: nothing.+- **Missing**: nothing required. (Out of scope: user-visible feedback when note operations silently no-op without iCloud.)