From c7d5044ce9eba97022bd0ca9fb533b008c9ca9e4 Mon Sep 17 00:00:00 2001 From: callumalpass Date: Fri, 13 Nov 2026 00:30:27 +1100 Subject: [PATCH] Comments: no false load error after a write; steady signing notice The collection watch reports a write before it confirms, and the panel reloads comments 50 ms later. The comment body read required a confirmed record, so it refused the pending record the write had just made and showed "Writer's selected record changed or is protected". Display reads of comment and annotation bodies now accept a pending record and refuse only a hold or an unresolved conflict; writes and readbacks stay strict. The signing notice took its people state from whichever account check finished last, so a slow, older check could replace a newer one (a tab showed "not linked" while signing). Checks now apply in start order, and a transient `unavailable` people-directory failure is retried once before the notice says the account could not be checked. Refs #91 --- apps/writer/src/backend/native.test.ts | 41 +++++++++++++++++++++ apps/writer/src/backend/native.ts | 26 +++++++++---- apps/writer/src/workspace/workspace.test.ts | 15 ++++++++ apps/writer/src/workspace/workspace.ts | 23 +++++++++--- changelog.d/comments-false-errors.md | 4 ++ docs/architecture.md | 9 ++++- 6 files changed, 105 insertions(+), 13 deletions(-) create mode 100644 changelog.d/comments-false-errors.md diff --git a/apps/writer/src/backend/native.test.ts b/apps/writer/src/backend/native.test.ts index d18f3a40..1d2ffd30 100644 --- a/apps/writer/src/backend/native.test.ts +++ b/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); diff --git a/apps/writer/src/backend/native.ts b/apps/writer/src/backend/native.ts index 84404471..fbb6d31e 100644 --- a/apps/writer/src/backend/native.ts +++ b/apps/writer/src/backend/native.ts @@ -24,9 +24,15 @@ interface NativeMetadata { readonly files: ReadonlyMap; 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 { 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,7 +416,7 @@ export class NativeWriterBackend implements WriterBackend { throw error; } } - private async contractRecord(original: wire.RecordView, contract: ContractSelector): Promise { + private async contractRecord(original: wire.RecordView, contract: ContractSelector, usable: (record: wire.RecordView) => boolean = unprotected): Promise { 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); @@ -412,7 +424,7 @@ export class NativeWriterBackend implements WriterBackend { || 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 { diff --git a/apps/writer/src/workspace/workspace.test.ts b/apps/writer/src/workspace/workspace.test.ts index c3aaaaa2..ca2f0cf0 100644 --- a/apps/writer/src/workspace/workspace.test.ts +++ b/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>) => 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" } }; diff --git a/apps/writer/src/workspace/workspace.ts b/apps/writer/src/workspace/workspace.ts index ad7afe31..f740c911 100644 --- a/apps/writer/src/workspace/workspace.ts +++ b/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 { + 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 { - 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) } }); } } diff --git a/changelog.d/comments-false-errors.md b/changelog.d/comments-false-errors.md new file mode 100644 index 00000000..fe0c8656 --- /dev/null +++ b/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. diff --git a/docs/architecture.md b/docs/architecture.md index 919d0000..222299f4 100644 --- a/docs/architecture.md +++ b/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]]`).