Comments: no false load error after a write; steady signing notice

apps/writer/src/backend/native.test.ts

@@ -359,6 +359,47 @@ describe("native Writer domain backend", () => {
     expect(review).toMatchObject({state: "resolved", deleted: expect.any(String), modified: expect.any(String)});
     expect(raw.frontmatter.has("deleted_at")).toBe(false);
   });
+  it("reloads comments without an error while a comment write is still pending (issue 91)", async () => {
+    // The watch reports a write before it confirms, and the panel reloads at once.
+    const h = await fixture();
+    const created = await h.backend.createComment({document: "manuscripts/original.md", text: "First"});
+    if (!created.ok) throw new Error(created.message);
+    expect((await h.backend.comments(["manuscripts/original.md"])).ok).toBe(true);
+    const changed = vi.fn(); h.backend.onCollectionChange(changed);
+    h.replica.stallConfirmations();
+    const writes = [h.backend.changeComment(created.value, {kind: "resolve"})];
+    await vi.waitFor(() => expect(changed).toHaveBeenCalled());
+    const pendingChange = await h.backend.comments(["manuscripts/original.md"]);
+    expect(pendingChange).toMatchObject({ok: true, value: [{path: created.value.path, status: "resolved"}]});
+    h.replica.confirmAll(); expect((await writes[0])?.ok).toBe(true);
+    changed.mockClear();
+    const reply = h.backend.createComment({document: "manuscripts/original.md", text: "A reply", intent: "reply-1"});
+    await vi.waitFor(() => expect(changed).toHaveBeenCalled());
+    const pendingCreate = await h.backend.comments(["manuscripts/original.md"]);
+    expect(pendingCreate.ok && pendingCreate.value.map(comment => comment.text)).toEqual(expect.arrayContaining(["First", "A reply"]));
+    h.replica.confirmAll(); h.replica.stallConfirmations(false);
+    expect((await reply).ok).toBe(true);
+  });
+  it("checks the account once more after a transient directory failure (issue 91)", async () => {
+    const h = await fixture();
+    const account = {id: "fixture-account", displayName: "Fixture", identities: []};
+    const directory = vi.spyOn(MdbasePeopleClient.prototype, "directory")
+      .mockRejectedValueOnce(new MdbaseError({code: "unavailable", recovery: "retry", message: "people read session changed", reason: "people_fenced"}))
+      .mockResolvedValueOnce({account, me: {status: "linked", person: {id: "p", path: "people/me.md", name: "Me", identities: [], typeNames: ["person"]}}, people: [], invalid: []} as never);
+    expect(await h.backend.people()).toMatchObject({signing: {kind: "linked"}, me: {name: "Me"}});
+    expect(directory).toHaveBeenCalledTimes(2);
+    directory.mockRejectedValue(new MdbaseError({code: "unavailable", recovery: "retry", message: "people read session changed", reason: "people_fenced"}));
+    expect(await h.backend.people({fresh: true})).toMatchObject({signing: {kind: "unavailable"}});
+    expect(directory).toHaveBeenCalledTimes(4);
+  });
+  it("still refuses a held or conflicted comment body on a display read", async () => {
+    const h = await fixture();
+    const created = await h.backend.createComment({document: "manuscripts/original.md", text: "Held"});
+    if (!created.ok) throw new Error(created.message);
+    const get = h.client.get.bind(h.client);
+    vi.spyOn(h.client, "get").mockImplementation(async (...args) => {const record = await get(...args); return {...record, state: {...record.state, unresolved: 1}};});
+    expect(await h.backend.comments(["manuscripts/original.md"])).toMatchObject({ok: false, message: "Writer's selected record changed or is protected."});
+  });
   it.each(["untyped", "sdk-mid", "mismatched-sdk-mid"])("recovers a %s lost create reply by the original SDK MID without creating a duplicate", async mode => {
     const h = await fixture();
     const original = h.client.create.bind(h.client);

apps/writer/src/backend/native.ts

@@ -24,9 +24,15 @@ interface NativeMetadata {
   readonly files: ReadonlyMap<string, wire.FileView>;
   readonly generation: number;
 }
-const RECONNECT_MIN_MS = 500, RECONNECT_MAX_MS = 30_000;
+const RECONNECT_MIN_MS = 500, RECONNECT_MAX_MS = 30_000, PEOPLE_RETRY_MS = 250;
 function unprotected(record: wire.RecordView): boolean {
-  return record.state.state === "confirmed" && !record.state.hold && !(record.state.unresolved ?? 0);
+  return record.state.state === "confirmed" && readable(record);
+}
+/** A record a display read may show: no hold or unresolved conflict. It may still be
+ * pending, as every write is between the watch's change and its confirmation; a
+ * rejected write is rolled back and arrives as another change. Writes need `unprotected`. */
+function readable(record: wire.RecordView): boolean {
+  return !record.state.hold && !(record.state.unresolved ?? 0);
 }
 
 export class NativeWriterBackend implements WriterBackend {
@@ -284,9 +290,9 @@ export class NativeWriterBackend implements WriterBackend {
         const identity = metadata.identities.get(path);
         if (!identity) throw new Error("Writer lost the original record identity.");
         const record = await this.client.get(identity.id, {body: true}, this.signal); this.check(metadata.generation);
-        if (record.id !== identity.id || record.path !== path || record.revision !== identity.revision || !record.types.includes(entry.type) || !unprotected(record)) throw new Error("Writer's selected record changed or is protected.");
+        if (record.id !== identity.id || record.path !== path || record.revision !== identity.revision || !record.types.includes(entry.type) || !readable(record)) throw new Error("Writer's selected record changed or is protected.");
         const contract = metadata.store.comments.has(path) ? commentContract : annotationContract;
-        const projected = await this.contractRecord(record, {...contract, type: entry.type}); this.check(metadata.generation);
+        const projected = await this.contractRecord(record, {...contract, type: entry.type}, readable); this.check(metadata.generation);
         rows.push({path: record.path, types: record.types, contractFields: nativeObject(projected.fields), body: record.body ?? ""});
       }
       return rows;
@@ -334,7 +340,13 @@ export class NativeWriterBackend implements WriterBackend {
   private async loadPeople(): Promise<People> {
     try {
       this.check();
-      const directory = await this.connection.people.directory({members: "omit", signal: this.signal}); this.check();
+      const directory = await this.connection.people.directory({members: "omit", signal: this.signal}).catch(async (error: unknown) => {
+        // A read session replaced mid-check (a reconnect) or a briefly incomplete
+        // inventory is transient: look once more before saying the account is unchecked.
+        if (!isMdbaseError(error, "unavailable")) throw error;
+        await new Promise(resolve => setTimeout(resolve, PEOPLE_RETRY_MS)); this.check();
+        return this.connection.people.directory({members: "omit", signal: this.signal});
+      }); this.check();
       return peopleFromDirectory(directory);
     } catch (error) {
       // Names only, never a substitute account/author or guessed identity.
@@ -404,15 +416,15 @@ export class NativeWriterBackend implements WriterBackend {
       throw error;
     }
   }
-  private async contractRecord(original: wire.RecordView, contract: ContractSelector): Promise<ContractRecordView> {
+  private async contractRecord(original: wire.RecordView, contract: ContractSelector, usable: (record: wire.RecordView) => boolean = unprotected): Promise<ContractRecordView> {
     const projected = await this.client.getContract(original.id, contract, this.signal); this.check();
     const expected = (await this.metadata()).schema.description.contracts.find(candidate => candidate.id === contract.id);
     const digest = manifestContractDigest(contract.id, contract.version);
     if (!expected || projected.id !== original.id || projected.path !== original.path || projected.revision !== original.revision
       || projected.contract.id !== contract.id || projected.contract.version !== contract.version || projected.contract.type !== contract.type
       || (digest !== undefined && !sameDigest(projected.contract.digest, digest))) throw new Error("Writer's contract record binding changed.");
     const current = await this.client.get(original.id, {body: true, document: true}, this.signal); this.check();
-    if (!unprotected(current) || current.id !== original.id || current.path !== original.path || current.revision !== original.revision) throw new Error("Writer's contract record changed or is protected.");
+    if (!usable(current) || current.id !== original.id || current.path !== original.path || current.revision !== original.revision) throw new Error("Writer's contract record changed or is protected.");
     return projected;
   }
   private async confirmed(write: Write, id: string, contract?: ContractSelector): Promise<wire.RecordView> {

apps/writer/src/workspace/workspace.test.ts

@@ -197,6 +197,21 @@ describe("workspace reliability", () => {
     expect(retry).toEqual(expect.any(String));
     expect(retry).toBe(first);
   });
+  it("never lets an older account check replace a newer one (issue 91)", async () => {
+    const { backend, open } = fixture();
+    const workspace = open(); await ready(workspace);
+    await vi.waitFor(() => expect(workspace.getSnapshot().commentsLoad.phase).toBe("ready"));
+    const answers: ((people: Awaited<ReturnType<WriterBackend["people"]>>) => void)[] = [];
+    backend.people = () => new Promise(resolve => { answers.push(resolve); });
+    const comments = workspace.loadComments();
+    const refreshed = workspace.refreshPeople();
+    answers[1]!({ ...NO_PEOPLE, me: { link: "[[people/me]]", name: "Me" }, signing: { kind: "linked" } });
+    await refreshed;
+    answers[0]!({ ...NO_PEOPLE, signing: { kind: "unlinked" } });
+    await comments;
+    expect(workspace.getSnapshot().commentsLoad.phase).toBe("ready");
+    expect(workspace.getSnapshot().people.signing).toEqual({ kind: "linked" });
+  });
   it("sends only worker deltas and no unchanged collection data for unrelated edits", async () => {
     const { backend, open } = fixture();
     const known = { key: "known", path: "sources/known.md", title: "Known", item: { id: "known", type: "book", title: "Known" } };

apps/writer/src/workspace/workspace.ts

@@ -382,11 +382,24 @@ export class ManuscriptWorkspace {
   }
 
   private peopleCheckedAt = 0;
+  /** Order of people checks: a check never replaces the result of one started after it. */
+  private peopleRequested = 0;
+  private peopleShown = 0;
+
+  /** The account's people, or undefined when a later check has already been shown. */
+  private async checkPeople(options?: { fresh?: boolean }): Promise<People | undefined> {
+    const request = ++this.peopleRequested;
+    this.peopleCheckedAt = Date.now();
+    const people = await this.backend.people(options);
+    if (request < this.peopleShown) return undefined;
+    this.peopleShown = request;
+    return people;
+  }
 
   /** Checks the account against the collection's person records again. */
   async refreshPeople(): Promise<void> {
-    this.peopleCheckedAt = Date.now();
-    this.update({ people: await this.backend.people({ fresh: true }) });
+    const people = await this.checkPeople({ fresh: true });
+    if (people && !this.disposed) this.update({ people });
   }
 
   /** Asks Connect to approve Writer again (to allow the identity permission), then checks the account. */
@@ -448,11 +461,11 @@ export class ManuscriptWorkspace {
     const scope = this.readingOrder();
     this.commentsScope = scope.join("\u0000");
     this.update({ commentsLoad: LOADING });
-    this.peopleCheckedAt = Date.now();
     try {
-      const [comments, people] = await Promise.all([this.backend.comments(scope), this.backend.people()]);
+      const [comments, checked] = await Promise.all([this.backend.comments(scope), this.checkPeople()]);
       if (this.disposed || generation !== this.commentsGeneration) return;
-      this.update(comments.ok ? { comments: comments.value, commentsProblem: undefined, commentsLoad: READY, people } : { commentsProblem: comments.message, commentsLoad: { phase: "failed", problem: comments.message }, people });
+      const people = checked ? { people: checked } : {};
+      this.update(comments.ok ? { comments: comments.value, commentsProblem: undefined, commentsLoad: READY, ...people } : { commentsProblem: comments.message, commentsLoad: { phase: "failed", problem: comments.message }, ...people });
     } catch (error) { if (generation === this.commentsGeneration) this.update({ commentsProblem: errorMessage(error), commentsLoad: { phase: "failed", problem: errorMessage(error) } }); }
   }
 

changelog.d/comments-false-errors.md

@@ -0,0 +1,4 @@
+## Fixed
+
+- The comments panel no longer shows "Comments could not be loaded" right after adding, replying to, resolving, reopening or withdrawing a comment. Its reload had refused the record the write had just made because that write was still being confirmed.
+- The "Your comments are not signed" notice no longer flips between states while your account is linked. An older account check can no longer replace a newer one, and a brief connection change is checked again before Writer says it could not check the account.

docs/architecture.md

@@ -39,7 +39,14 @@
   editor shows it as a tracked change, and accepting it edits the record
   through its session. Authors are links to the signed-in account's
   `mdbase.person` record, found through Connect's optional identity
-  permission; without it, comments are unsigned.
+  permission; without it, comments are unsigned. The comment and annotation
+  body reads behind the panel accept a record that is still pending (the watch
+  reports each write before it confirms, and the panel reloads at once); only a
+  hold or an unresolved conflict refuses them. Writes and their readbacks still
+  require a confirmed record. Account checks are applied in the order they were
+  started, so a slow, older check never replaces a newer signing state, and a
+  transient `unavailable` directory failure is checked once more before the
+  notice says the account could not be checked.
 - **Images** are collection files, referenced relative to the record
   (`![Caption](figures/plot.png){#fig-plot}`) or by wikilink (`![[plot.png]]`).