diff --git a/apps/writer/src/backend/native.test.ts b/apps/writer/src/backend/native.test.ts index d18f3a4..1d2ffd3 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 8440447..fbb6d31 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 c3aaaaa..ca2f0cf 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 ad7afe3..f740c91 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 0000000..fe0c865 --- /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 919d000..222299f 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]]`).