Add unit tests for patch review logic and draft storage

Code review is the app's core workflow and had almost no coverage. A
mutation run checked that each test fails when the logic it covers breaks.

tests/unit/commentResolutions.test.ts

@@ -0,0 +1,71 @@
+import { describe, expect, test } from "vitest";
+
+import { resolutionsByComment } from "@app/lib/commentResolutions";
+
+import { author, operation } from "./support/cobs";
+
+const alice = author("alice");
+const bob = author("bob");
+
+function resolve(comment: string) {
+  return { type: "review.comment.resolve" as const, review: "r", comment };
+}
+
+function unresolve(comment: string) {
+  return { type: "review.comment.unresolve" as const, review: "r", comment };
+}
+
+describe("resolutionsByComment", () => {
+  test("records who resolved a comment and when", () => {
+    const resolutions = resolutionsByComment([
+      operation(alice, 10, [resolve("c1")]),
+    ]);
+
+    expect(resolutions.get("c1")).toEqual({ author: alice, timestamp: 10 });
+  });
+
+  test("the latest resolve wins", () => {
+    const resolutions = resolutionsByComment([
+      operation(alice, 10, [resolve("c1")]),
+      operation(bob, 20, [resolve("c1")]),
+    ]);
+
+    expect(resolutions.get("c1")).toEqual({ author: bob, timestamp: 20 });
+  });
+
+  test("replays operations in timestamp order, not array order", () => {
+    const resolutions = resolutionsByComment([
+      operation(bob, 20, [unresolve("c1")]),
+      operation(alice, 10, [resolve("c1")]),
+    ]);
+
+    expect(resolutions.has("c1")).toBe(false);
+  });
+
+  test("unresolving drops the entry", () => {
+    const resolutions = resolutionsByComment([
+      operation(alice, 10, [resolve("c1"), resolve("c2")]),
+      operation(bob, 20, [unresolve("c1")]),
+    ]);
+
+    expect([...resolutions.keys()]).toEqual(["c2"]);
+  });
+
+  test("ignores other actions", () => {
+    const resolutions = resolutionsByComment([
+      operation(alice, 10, [{ type: "label", labels: ["bug"] }]),
+    ]);
+
+    expect(resolutions.size).toBe(0);
+  });
+
+  test("does not reorder the caller's array", () => {
+    const activity = [
+      operation(bob, 20, [resolve("c1")]),
+      operation(alice, 10, [resolve("c2")]),
+    ];
+    resolutionsByComment(activity);
+
+    expect(activity.map(op => op.timestamp)).toEqual([20, 10]);
+  });
+});

tests/unit/commentSources.test.ts

@@ -0,0 +1,98 @@
+import { describe, expect, test } from "vitest";
+
+import { commentSourcesOf, STANDALONE_COMMENTS } from "@app/lib/commentSources";
+
+import { author, comment, location, review, revision } from "./support/cobs";
+
+const alice = author("alice");
+const bob = author("bob");
+const carol = author("carol");
+const code = location("src/main.rs", "new", 3);
+
+describe("commentSourcesOf", () => {
+  test("lists each review with code comments, counting only roots", () => {
+    const sources = commentSourcesOf(
+      revision({
+        reviews: [
+          review({
+            id: "r1",
+            author: alice,
+            comments: [
+              comment({ id: "a", location: code }),
+              comment({ id: "b", location: code }),
+              comment({ id: "reply", location: code, replyTo: "a" }),
+            ],
+          }),
+        ],
+      }),
+    );
+
+    expect(sources).toEqual([
+      { id: "r1", name: "alice", count: 2, nids: ["z6Mkalice"] },
+    ]);
+  });
+
+  test("skips reviews without code comments", () => {
+    const sources = commentSourcesOf(
+      revision({
+        reviews: [
+          review({ id: "r1", comments: [comment({ location: null })] }),
+          review({ id: "r2", comments: [] }),
+        ],
+      }),
+    );
+
+    expect(sources).toEqual([]);
+  });
+
+  test("names a review author without an alias by a short key", () => {
+    const [source] = commentSourcesOf(
+      revision({
+        reviews: [
+          review({
+            author: author("anonymous", null),
+            comments: [comment({ location: code })],
+          }),
+        ],
+      }),
+    );
+
+    expect(source.name).toBe("z6Mkan");
+  });
+
+  test("collects code comments outside reviews into their own bucket", () => {
+    const sources = commentSourcesOf(
+      revision({
+        discussion: [
+          comment({ id: "1", author: bob, location: code }),
+          comment({ id: "2", author: carol, location: code }),
+          comment({ id: "3", author: bob, location: code }),
+          comment({ id: "plain", author: alice, location: null }),
+          comment({ id: "reply", author: alice, location: code, replyTo: "1" }),
+        ],
+      }),
+    );
+
+    expect(sources).toEqual([
+      {
+        id: STANDALONE_COMMENTS,
+        name: "Not part of a review",
+        count: 3,
+        nids: ["z6Mkbob", "z6Mkcarol"],
+      },
+    ]);
+  });
+
+  test("puts reviews before the standalone bucket", () => {
+    const sources = commentSourcesOf(
+      revision({
+        reviews: [
+          review({ id: "r1", comments: [comment({ location: code })] }),
+        ],
+        discussion: [comment({ location: code })],
+      }),
+    );
+
+    expect(sources.map(s => s.id)).toEqual(["r1", STANDALONE_COMMENTS]);
+  });
+});

tests/unit/draftReviewStorage.test.ts

@@ -0,0 +1,294 @@
+import { beforeEach, describe, expect, test, vi } from "vitest";
+
+import { author, location } from "./support/cobs";
+
+const invoke = vi.fn();
+vi.mock("@app/lib/invoke", () => ({ invoke }));
+
+const key = "repo.patches.draftReviews";
+const alice = author("alice");
+const bob = author("bob");
+const aliceNid = "z6Mkalice";
+const bobNid = "z6Mkbob";
+
+// The store reads localStorage once, when the module loads.
+async function load() {
+  vi.resetModules();
+  return (await import("@app/lib/draftReviewStorage")).draftReviewStorage;
+}
+
+function stored(): Record<string, Record<string, unknown>> {
+  return JSON.parse(localStorage.getItem(key) ?? "{}");
+}
+
+beforeEach(() => {
+  localStorage.clear();
+  invoke.mockReset();
+  vi.spyOn(console, "error").mockReturnValue(undefined);
+});
+
+describe("draftReviewStorage", () => {
+  test("creates an empty accepting draft for its author", async () => {
+    const drafts = await load();
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+    expect(drafts.get(id, alice)).toEqual({
+      id,
+      draft: true,
+      rid: "rad:repo",
+      author: alice,
+      revisionId: "rev",
+      verdict: "accept",
+      summary: "",
+      labels: [],
+      comments: [],
+      checkedFiles: [],
+    });
+    expect(stored()[id]).toMatchObject({ nid: aliceNid, patchId: "patch" });
+  });
+
+  test("hides a draft from other identities", async () => {
+    const drafts = await load();
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+    expect(drafts.get(id, bob)).toBeUndefined();
+    expect(drafts.getForRevision("rev", bob)).toBeUndefined();
+    expect(drafts.hasForRevision("rev", bobNid)).toBe(false);
+  });
+
+  test("finds a draft by revision", async () => {
+    const drafts = await load();
+    drafts.create("rad:repo", "patch", "other", aliceNid);
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+    expect(drafts.getForRevision("rev", alice)?.id).toBe(id);
+    expect(drafts.hasForRevision("rev", aliceNid)).toBe(true);
+    expect(drafts.hasForRevision("missing", aliceNid)).toBe(false);
+  });
+
+  test("updates summary, verdict and labels", async () => {
+    const drafts = await load();
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+    drafts.update(id, { summary: "LGTM", verdict: undefined, labels: ["ok"] });
+
+    expect(drafts.get(id, alice)).toMatchObject({
+      summary: "LGTM",
+      verdict: undefined,
+      labels: ["ok"],
+    });
+  });
+
+  test("toggles checked files", async () => {
+    const drafts = await load();
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+    drafts.toggleCheckedFile(id, "a.ts");
+    drafts.toggleCheckedFile(id, "b.ts");
+    expect(drafts.isFileChecked(id, "a.ts")).toBe(true);
+
+    drafts.toggleCheckedFile(id, "a.ts");
+    expect(drafts.isFileChecked(id, "a.ts")).toBe(false);
+    expect(drafts.get(id, alice)?.checkedFiles).toEqual(["b.ts"]);
+    expect(drafts.isFileChecked("missing", "a.ts")).toBe(false);
+  });
+
+  describe("comments", () => {
+    test("adds a comment as the draft author's", async () => {
+      const drafts = await load();
+      const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+      const where = location("a.ts", "new", 3);
+      const commentId = drafts.addComment(id, { body: "nit", location: where });
+
+      expect(drafts.get(id, alice)?.comments).toEqual([
+        {
+          id: commentId,
+          author: alice,
+          edits: [{ author: alice, timestamp: 0, body: "nit" }],
+          reactions: [],
+          replyTo: null,
+          location: where,
+          resolved: false,
+        },
+      ]);
+    });
+
+    test("edits a comment's body", async () => {
+      const drafts = await load();
+      const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+      const commentId = drafts.addComment(id, {
+        body: "nit",
+        location: location("a.ts", "new", 3),
+      });
+      drafts.updateComment(id, commentId, { body: "fixed" });
+
+      expect(drafts.get(id, alice)?.comments[0].edits[0].body).toBe("fixed");
+      expect(() => drafts.updateComment(id, "missing", { body: "x" })).toThrow(
+        "Comment missing does not exist",
+      );
+    });
+
+    test("deletes only the given comment", async () => {
+      const drafts = await load();
+      const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+      const where = location("a.ts", "new", 3);
+      const first = drafts.addComment(id, { body: "one", location: where });
+      const second = drafts.addComment(id, { body: "two", location: where });
+
+      drafts.deleteComment(id, "missing");
+      expect(drafts.get(id, alice)?.comments.map(c => c.id)).toEqual([
+        first,
+        second,
+      ]);
+
+      drafts.deleteComment(id, first);
+      expect(drafts.get(id, alice)?.comments.map(c => c.id)).toEqual([second]);
+    });
+
+    test("editing a draft that doesn't exist throws", async () => {
+      const drafts = await load();
+
+      expect(() =>
+        drafts.addComment("missing", {
+          body: "x",
+          location: location("a.ts", "new", 1),
+        }),
+      ).toThrow("Draft review missing does not exist");
+    });
+  });
+
+  test("deletes a draft and returns it", async () => {
+    const drafts = await load();
+    const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+    expect(drafts.delete(id)?.id).toBe(id);
+    expect(drafts.get(id, alice)).toBeUndefined();
+    expect(stored()).toEqual({});
+  });
+
+  test("prunes only this author's drafts on this patch's removed revisions", async () => {
+    const drafts = await load();
+    const gone = drafts.create("rad:repo", "patch", "gone", aliceNid);
+    const live = drafts.create("rad:repo", "patch", "live", aliceNid);
+    const otherPatch = drafts.create("rad:repo", "other", "gone", aliceNid);
+    const otherAuthor = drafts.create("rad:repo", "patch", "gone", bobNid);
+
+    drafts.pruneStale("patch", ["live"], aliceNid);
+
+    expect(Object.keys(stored()).sort()).toEqual(
+      [live, otherPatch, otherAuthor].sort(),
+    );
+    expect(stored()[gone]).toBeUndefined();
+  });
+
+  describe("publish", () => {
+    test("publishes the review, then deletes the draft", async () => {
+      invoke.mockResolvedValue(undefined);
+      const drafts = await load();
+      const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+      drafts.update(id, {
+        summary: "LGTM",
+        verdict: undefined,
+        labels: ["ok"],
+      });
+      const where = location("a.ts", "new", 3);
+      drafts.addComment(id, { body: "nit", location: where });
+
+      await drafts.publish(id);
+
+      expect(invoke).toHaveBeenCalledWith("create_patch_review", {
+        args: {
+          rid: "rad:repo",
+          revision: "rev",
+          verdict: null,
+          summary: "LGTM",
+          labels: ["ok"],
+          comments: [{ body: "nit", location: where }],
+        },
+      });
+      expect(drafts.get(id, alice)).toBeUndefined();
+    });
+
+    test("publishes a comment without a location as null", async () => {
+      invoke.mockResolvedValue(undefined);
+      localStorage.setItem(
+        key,
+        JSON.stringify({
+          old: {
+            id: "old",
+            rid: "rad:repo",
+            revision: "rev",
+            labels: [],
+            comments: [{ id: "c", body: "general remark" }],
+          },
+        }),
+      );
+      const drafts = await load();
+
+      await drafts.publish("old");
+
+      expect(invoke.mock.calls[0][1].args.comments).toEqual([
+        { body: "general remark", location: null },
+      ]);
+    });
+
+    test("keeps the draft when publishing fails", async () => {
+      invoke.mockRejectedValue(new Error("offline"));
+      const drafts = await load();
+      const id = drafts.create("rad:repo", "patch", "rev", aliceNid);
+
+      await expect(drafts.publish(id)).rejects.toThrow("offline");
+      expect(drafts.get(id, alice)?.id).toBe(id);
+    });
+
+    test("rejects a draft that doesn't exist", async () => {
+      const drafts = await load();
+
+      await expect(drafts.publish("missing")).rejects.toThrow(
+        "Review missing does not exist",
+      );
+      expect(invoke).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("persistence", () => {
+    test("drafts survive a reload", async () => {
+      const id = (await load()).create("rad:repo", "patch", "rev", aliceNid);
+
+      expect((await load()).get(id, alice)?.revisionId).toBe("rev");
+    });
+
+    test("reads drafts stored before nid, patchId, summary and checkedFiles existed", async () => {
+      localStorage.setItem(
+        key,
+        JSON.stringify({
+          old: {
+            id: "old",
+            rid: "rad:repo",
+            revision: "rev",
+            labels: [],
+            comments: [{ id: "c", body: "legacy" }],
+          },
+        }),
+      );
+      const drafts = await load();
+
+      // Without a recorded author, the draft belongs to whoever is signed in.
+      expect(drafts.get("old", bob)).toMatchObject({
+        summary: "",
+        checkedFiles: [],
+        comments: [{ id: "c", location: null }],
+      });
+      // Without a patch id it can't be pruned.
+      drafts.pruneStale("patch", [], bobNid);
+      expect(drafts.get("old", bob)).toBeDefined();
+    });
+
+    test("starts empty when the stored data is invalid", async () => {
+      localStorage.setItem(key, "{not json");
+      expect((await load()).hasForRevision("rev", aliceNid)).toBe(false);
+
+      localStorage.setItem(key, JSON.stringify({ x: { id: 1 } }));
+      expect((await load()).get("x", alice)).toBeUndefined();
+    });
+  });
+});

tests/unit/patchContributions.test.ts

@@ -0,0 +1,55 @@
+import { expect, test } from "vitest";
+
+import { patchContributions } from "@app/lib/patchContributions";
+
+import { author, comment, review, revision } from "./support/cobs";
+
+const alice = author("alice");
+const bob = author("bob");
+
+const revisions = [
+  revision({
+    id: "r1",
+    author: alice,
+    discussion: [
+      comment({ author: alice }),
+      comment({ author: bob }),
+      comment({ author: bob }),
+    ],
+    reviews: [
+      review({
+        author: bob,
+        comments: [comment({ author: bob }), comment({ author: alice })],
+      }),
+    ],
+  }),
+  revision({
+    id: "r2",
+    author: bob,
+    reviews: [review({ author: alice }), review({ author: bob })],
+  }),
+];
+
+test("counts one person's revisions, comments and reviews", () => {
+  expect(patchContributions(revisions, bob.did)).toEqual({
+    revisionCount: 1,
+    commentCount: 3,
+    reviewCount: 2,
+  });
+});
+
+test("counts comments in revision discussions and in reviews", () => {
+  expect(patchContributions(revisions, alice.did)).toEqual({
+    revisionCount: 1,
+    commentCount: 2,
+    reviewCount: 1,
+  });
+});
+
+test("counts nothing for someone who took no part", () => {
+  expect(patchContributions(revisions, author("carol").did)).toEqual({
+    revisionCount: 0,
+    commentCount: 0,
+    reviewCount: 0,
+  });
+});

tests/unit/patchSlice.test.ts

@@ -0,0 +1,123 @@
+import { describe, expect, test } from "vitest";
+
+import { sliceForRange } from "@app/lib/patchSlice";
+
+const preamble = [
+  "diff --git a/f.txt b/f.txt",
+  "index 1111111..2222222 100644",
+  "--- a/f.txt",
+  "+++ b/f.txt",
+];
+
+// Old side: l1..l10 on lines 1-10. New side: l1-l4, L5, L5b, l6-l10 on lines
+// 1-11.
+const hunk = [
+  "@@ -1,10 +1,11 @@ fn main",
+  " l1",
+  " l2",
+  " l3",
+  " l4",
+  "-l5",
+  "+L5",
+  "+L5b",
+  " l6",
+  " l7",
+  " l8",
+  " l9",
+  " l10",
+];
+
+function patch(...lines: string[]): string {
+  return [...preamble, ...lines, ""].join("\n");
+}
+
+describe("sliceForRange", () => {
+  test("cuts a new-side range down to its context and renumbers the hunk", () => {
+    expect(sliceForRange(patch(...hunk), "new", 5, 6, 1)).toBe(
+      patch("@@ -5,1 +5,2 @@", "-l5", "+L5", "+L5b"),
+    );
+  });
+
+  test("keeps three lines of context by default", () => {
+    expect(sliceForRange(patch(...hunk), "new", 5, 6)).toBe(
+      patch(
+        "@@ -3,5 +3,6 @@",
+        " l3",
+        " l4",
+        "-l5",
+        "+L5",
+        "+L5b",
+        " l6",
+        " l7",
+      ),
+    );
+  });
+
+  test("cuts an old-side range", () => {
+    expect(sliceForRange(patch(...hunk), "old", 5, 6, 1)).toBe(
+      patch("@@ -4,2 +4,2 @@", " l4", "-l5", "+L5"),
+    );
+  });
+
+  test("covers a multi-line range with context on both ends", () => {
+    expect(sliceForRange(patch(...hunk), "new", 7, 9, 1)).toBe(
+      patch("@@ -6,3 +6,4 @@", "+L5b", " l6", " l7", " l8"),
+    );
+  });
+
+  test("keeps the hunk verbatim when the window covers all of it", () => {
+    expect(sliceForRange(patch(...hunk), "new", 5, 6, 20)).toBe(patch(...hunk));
+  });
+
+  test("writes 0 for a side the slice has no lines on", () => {
+    expect(sliceForRange(patch(...hunk), "new", 5, 7, 0)).toBe(
+      patch("@@ -0,0 +5,2 @@", "+L5", "+L5b"),
+    );
+  });
+
+  test("returns undefined when no hunk covers the range", () => {
+    expect(sliceForRange(patch(...hunk), "new", 50, 51)).toBeUndefined();
+    expect(sliceForRange(patch(...hunk), "old", 11, 12)).toBeUndefined();
+  });
+
+  test("uses the hunk that covers the range and keeps the preamble", () => {
+    const twoHunks = patch(
+      "@@ -1,2 +1,2 @@",
+      "-a",
+      "+A",
+      " b",
+      "@@ -20,3 +20,3 @@ fn second",
+      " x",
+      "-y",
+      "+Y",
+      " z",
+    );
+
+    expect(sliceForRange(twoHunks, "new", 21, 22, 0)).toBe(
+      patch("@@ -0,0 +21,1 @@", "+Y"),
+    );
+    expect(sliceForRange(twoHunks, "new", 21, 22, 5)).toBe(
+      patch("@@ -20,3 +20,3 @@ fn second", " x", "-y", "+Y", " z"),
+    );
+  });
+
+  test("reads a hunk header without counts", () => {
+    expect(sliceForRange(patch("@@ -3 +3 @@", "-c", "+C"), "new", 3, 4)).toBe(
+      patch("@@ -3 +3 @@", "-c", "+C"),
+    );
+  });
+
+  test("a no-newline marker doesn't shift the line numbers", () => {
+    const noNewline = patch(
+      "@@ -1,2 +1,2 @@",
+      "-a",
+      "\\ No newline at end of file",
+      "+A",
+      " b",
+    );
+
+    expect(sliceForRange(noNewline, "new", 2, 3, 0)).toBe(
+      patch("@@ -2,1 +2,1 @@", " b"),
+    );
+  });
+});

tests/unit/pierreComments.test.ts

@@ -0,0 +1,260 @@
+import type { CodeLocation } from "@bindings/cob/thread/CodeLocation";
+import type { CodeRange } from "@bindings/cob/thread/CodeRange";
+
+import { describe, expect, test } from "vitest";
+
+import type { ComposerTarget } from "@app/lib/pierreComments";
+import {
+  anchorOf,
+  commentCountsByPath,
+  fileAnnotations,
+  formatAnchor,
+  formatAnchorLines,
+  isCommentableStatus,
+  locationOf,
+} from "@app/lib/pierreComments";
+
+import { comment, location, thread } from "./support/cobs";
+
+function lines(start: number, end: number): CodeRange {
+  return { type: "lines", range: { start, end } };
+}
+
+function at(
+  path: string,
+  props: { old?: CodeRange; new?: CodeRange },
+): CodeLocation {
+  return {
+    commit: "c".repeat(40),
+    path,
+    old: props.old ?? null,
+    new: props.new ?? null,
+  };
+}
+
+describe("anchorOf", () => {
+  test("anchors a new-side range under its last line", () => {
+    expect(anchorOf(at("a.ts", { new: lines(3, 8) }))).toEqual({
+      path: "a.ts",
+      side: "additions",
+      line: 7,
+    });
+  });
+
+  test("anchors an old-side range on the deletions side", () => {
+    expect(anchorOf(at("a.ts", { old: lines(5, 6) }))).toEqual({
+      path: "a.ts",
+      side: "deletions",
+      line: 5,
+    });
+  });
+
+  test("prefers the new side when a location has both", () => {
+    expect(
+      anchorOf(at("a.ts", { old: lines(1, 2), new: lines(9, 10) }))?.side,
+    ).toBe("additions");
+  });
+
+  test("places a chars range on its line", () => {
+    expect(
+      anchorOf(
+        at("a.ts", {
+          new: { type: "chars", line: 4, range: { start: 0, end: 2 } },
+        }),
+      )?.line,
+    ).toBe(4);
+  });
+
+  test.each([null, undefined, at("a.ts", {})])(
+    "has no anchor for %j",
+    value => {
+      expect(anchorOf(value)).toBeUndefined();
+    },
+  );
+});
+
+describe("locationOf", () => {
+  const target: ComposerTarget = {
+    path: "a.ts",
+    side: "additions",
+    firstLine: 4,
+    lastLine: 6,
+  };
+
+  test("writes the range on the composer's side", () => {
+    expect(locationOf("abc", target)).toEqual({
+      commit: "abc",
+      path: "a.ts",
+      old: null,
+      new: lines(4, 7),
+    });
+  });
+
+  test("writes a deletions range on the old side", () => {
+    expect(locationOf("abc", { ...target, side: "deletions" })).toEqual({
+      commit: "abc",
+      path: "a.ts",
+      old: lines(4, 7),
+      new: null,
+    });
+  });
+
+  test("normalises a range dragged upwards", () => {
+    expect(
+      locationOf("abc", { ...target, firstLine: 6, lastLine: 4 }).new,
+    ).toEqual(lines(4, 7));
+  });
+
+  test("round-trips through anchorOf to the composer's last line", () => {
+    expect(anchorOf(locationOf("abc", target))).toEqual({
+      path: "a.ts",
+      side: "additions",
+      line: 6,
+    });
+  });
+});
+
+describe("formatAnchorLines", () => {
+  test.each<[CodeLocation, string | undefined]>([
+    [at("a.ts", { new: lines(12, 13) }), "R12"],
+    [at("a.ts", { old: lines(3, 10) }), "L3-L9"],
+    [at("a.ts", { new: lines(3, 10) }), "R3-R9"],
+    [
+      at("a.ts", {
+        old: { type: "chars", line: 7, range: { start: 1, end: 4 } },
+      }),
+      "L7",
+    ],
+    [at("a.ts", {}), undefined],
+  ])("formats %j as %j", (value, expected) => {
+    expect(formatAnchorLines(value)).toBe(expected);
+  });
+
+  test("formatAnchor prefixes the path", () => {
+    expect(formatAnchor(at("src/a.ts", { old: lines(3, 10) }))).toBe(
+      "src/a.ts:L3-L9",
+    );
+    expect(formatAnchor(at("src/a.ts", {}))).toBe("src/a.ts");
+  });
+});
+
+test.each([
+  ["added", true],
+  ["deleted", true],
+  ["modified", true],
+  [undefined, true],
+  ["moved", false],
+  ["copied", false],
+] as const)("isCommentableStatus(%j) is %j", (status, expected) => {
+  expect(isCommentableStatus(status)).toBe(expected);
+});
+
+describe("fileAnnotations", () => {
+  const onLine5 = location("a.ts", "new", 5);
+
+  test("keeps only this file's threads, one annotation per side and line", () => {
+    const annotations = fileAnnotations(
+      "a.ts",
+      [
+        thread(comment({ id: "1", location: onLine5 })),
+        thread(comment({ id: "2", location: location("a.ts", "old", 5) })),
+        thread(comment({ id: "3", location: location("b.ts", "new", 5) })),
+        thread(comment({ id: "4", location: null })),
+      ],
+      undefined,
+    );
+
+    expect(
+      annotations.map(a => [
+        a.side,
+        a.lineNumber,
+        a.metadata.threads.map(t => t.root.id),
+      ]),
+    ).toEqual([
+      ["additions", 5, ["1"]],
+      ["deletions", 5, ["2"]],
+    ]);
+  });
+
+  test("orders threads on the same line oldest first", () => {
+    const [annotation] = fileAnnotations(
+      "a.ts",
+      [
+        thread(comment({ id: "new", location: onLine5, timestamp: 20 })),
+        thread(comment({ id: "old", location: onLine5, timestamp: 10 })),
+      ],
+      undefined,
+    );
+
+    expect(annotation.metadata.threads.map(t => t.root.id)).toEqual([
+      "old",
+      "new",
+    ]);
+  });
+
+  test("puts the composer on its last line, sharing a slot with threads", () => {
+    const composer: ComposerTarget = {
+      path: "a.ts",
+      side: "additions",
+      firstLine: 2,
+      lastLine: 5,
+    };
+    const annotations = fileAnnotations(
+      "a.ts",
+      [thread(comment({ id: "1", location: onLine5 }))],
+      composer,
+    );
+
+    expect(annotations).toHaveLength(1);
+    expect(annotations[0].metadata.composer).toBe(composer);
+    expect(annotations[0].metadata.threads).toHaveLength(1);
+  });
+
+  test("ignores a composer in another file", () => {
+    const annotations = fileAnnotations("a.ts", [], {
+      path: "b.ts",
+      side: "additions",
+      firstLine: 1,
+      lastLine: 1,
+    });
+
+    expect(annotations).toEqual([]);
+  });
+});
+
+describe("commentCountsByPath", () => {
+  test("counts resolved and unresolved threads per file", () => {
+    const counts = commentCountsByPath(
+      [
+        thread(comment({ id: "1", location: location("a.ts", "new", 1) })),
+        thread(
+          comment({
+            id: "2",
+            location: location("a.ts", "new", 2),
+            resolved: true,
+          }),
+        ),
+        thread(comment({ id: "3", location: location("b.ts", "old", 1) })),
+        thread(comment({ id: "4", location: null })),
+      ],
+      () => true,
+    );
+
+    expect(Object.fromEntries(counts)).toEqual({
+      "a.ts": { resolved: 1, unresolved: 1 },
+      "b.ts": { resolved: 0, unresolved: 1 },
+    });
+  });
+
+  test("skips threads that can't be resolved", () => {
+    const counts = commentCountsByPath(
+      [
+        thread(comment({ id: "draft", location: location("a.ts", "new", 1) })),
+        thread(comment({ id: "real", location: location("a.ts", "new", 2) })),
+      ],
+      id => id !== "draft",
+    );
+
+    expect(counts.get("a.ts")).toEqual({ resolved: 0, unresolved: 1 });
+  });
+});

tests/unit/pierreParse.test.ts

@@ -0,0 +1,132 @@
+import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
+
+type Request = { id: number; patch: string; cacheKeyPrefix?: string };
+
+// Stands in for the parse worker. Tests decide how each instance behaves.
+class FakeWorker {
+  static instances: FakeWorker[] = [];
+  static failToStart = false;
+
+  onmessage: ((event: { data: unknown }) => void) | null = null;
+  onerror: ((event: { message: string }) => void) | null = null;
+  requests: Request[] = [];
+
+  constructor() {
+    if (FakeWorker.failToStart) throw new Error("no workers here");
+    FakeWorker.instances.push(this);
+  }
+
+  postMessage(request: Request) {
+    this.requests.push(request);
+  }
+
+  reply(data: unknown) {
+    this.onmessage?.({ data });
+  }
+}
+
+const processPatch = vi.fn((patch: string) => ({
+  files: [{ name: `main-thread:${patch}` }],
+}));
+
+vi.mock("@app/lib/pierreParse.worker?worker", () => ({ default: FakeWorker }));
+vi.mock("@pierre/diffs", () => ({ processPatch }));
+
+async function load() {
+  vi.resetModules();
+  return (await import("@app/lib/pierreParse")).parsePatch;
+}
+
+// Lets the worker receive the request before the test replies.
+async function tick() {
+  await new Promise(resolve => setTimeout(resolve, 0));
+}
+
+beforeEach(() => {
+  FakeWorker.instances = [];
+  FakeWorker.failToStart = false;
+  processPatch.mockClear();
+  vi.spyOn(console, "error").mockReturnValue(undefined);
+});
+
+afterEach(() => {
+  vi.restoreAllMocks();
+});
+
+describe("parsePatch", () => {
+  test("parses in the worker", async () => {
+    const parsePatch = await load();
+    const result = parsePatch("patch", "rev1");
+    await tick();
+    const [worker] = FakeWorker.instances;
+    const [request] = worker.requests;
+    expect(request).toMatchObject({ patch: "patch", cacheKeyPrefix: "rev1" });
+
+    worker.reply({ id: request.id, files: [{ name: "from-worker" }] });
+
+    expect(await result).toEqual([{ name: "from-worker" }]);
+    expect(processPatch).not.toHaveBeenCalled();
+  });
+
+  test("routes out-of-order replies to the right request", async () => {
+    const parsePatch = await load();
+    const first = parsePatch("one");
+    const second = parsePatch("two");
+    await tick();
+    const [worker] = FakeWorker.instances;
+    const [a, b] = worker.requests;
+
+    worker.reply({ id: b.id, files: [{ name: "two" }] });
+    worker.reply({ id: a.id, files: [{ name: "one" }] });
+
+    expect(await first).toEqual([{ name: "one" }]);
+    expect(await second).toEqual([{ name: "two" }]);
+    expect(FakeWorker.instances).toHaveLength(1);
+  });
+
+  test("parses on the main thread when the worker can't start", async () => {
+    FakeWorker.failToStart = true;
+    const parsePatch = await load();
+
+    expect(await parsePatch("patch", "rev1")).toEqual([
+      { name: "main-thread:patch" },
+    ]);
+    expect(processPatch).toHaveBeenCalledWith("patch", "rev1");
+  });
+
+  test("falls back to the main thread when the worker reports an error", async () => {
+    const parsePatch = await load();
+    const result = parsePatch("patch");
+    await tick();
+    const [worker] = FakeWorker.instances;
+
+    worker.reply({ id: worker.requests[0].id, error: "bad patch" });
+
+    expect(await result).toEqual([{ name: "main-thread:patch" }]);
+  });
+
+  test("a crashed worker fails over its pending requests and is not reused", async () => {
+    const parsePatch = await load();
+    const pending = parsePatch("one");
+    await tick();
+    const [worker] = FakeWorker.instances;
+
+    worker.onerror?.({ message: "crashed" });
+
+    expect(await pending).toEqual([{ name: "main-thread:one" }]);
+    expect(await parsePatch("two")).toEqual([{ name: "main-thread:two" }]);
+    expect(FakeWorker.instances).toHaveLength(1);
+  });
+
+  test("ignores replies for unknown requests", async () => {
+    const parsePatch = await load();
+    const result = parsePatch("patch");
+    await tick();
+    const [worker] = FakeWorker.instances;
+
+    worker.reply({ id: 999, files: [{ name: "stray" }] });
+    worker.reply({ id: worker.requests[0].id, files: [{ name: "real" }] });
+
+    expect(await result).toEqual([{ name: "real" }]);
+  });
+});

tests/unit/reviewSummary.test.ts

@@ -1,7 +1,15 @@
 import { describe, expect, test } from "vitest";
 
 import type { ReviewEntry } from "@app/lib/reviewSummary";
-import { reviewSummary } from "@app/lib/reviewSummary";
+import {
+  entriesFromReviewers,
+  entriesFromRevisions,
+  isOutdatedReview,
+  reviewSummary,
+  summaryTitle,
+} from "@app/lib/reviewSummary";
+
+import * as cobs from "./support/cobs";
 
 function review(
   alias: string,
@@ -112,3 +120,93 @@ describe("reviewSummary", () => {
     expect(state.allAccept).toBe(true);
   });
 });
+
+describe("entries", () => {
+  test("entriesFromRevisions numbers reviews by revision and marks delegates", () => {
+    const alice = cobs.author("alice");
+    const bob = cobs.author("bob");
+    const entries = entriesFromRevisions(
+      [
+        cobs.revision({
+          id: "rev1",
+          reviews: [cobs.review({ id: "a", author: alice, verdict: "accept" })],
+        }),
+        cobs.revision({ id: "rev2" }),
+        cobs.revision({
+          id: "rev3",
+          reviews: [cobs.review({ id: "b", author: bob, verdict: "reject" })],
+        }),
+      ],
+      [bob.did],
+    );
+
+    expect(entries).toEqual([
+      {
+        author: alice,
+        verdict: "accept",
+        revisionNumber: 1,
+        delegate: false,
+        reviewId: "a",
+      },
+      {
+        author: bob,
+        verdict: "reject",
+        revisionNumber: 3,
+        delegate: true,
+        reviewId: "b",
+      },
+    ]);
+  });
+
+  test("entriesFromReviewers keeps the reviewer fields, without a review id", () => {
+    const reviewer = {
+      author: cobs.author("alice"),
+      verdict: "accept" as const,
+      revisionNumber: 2,
+      delegate: true,
+    };
+
+    expect(entriesFromReviewers([reviewer])).toEqual([reviewer]);
+  });
+});
+
+test.each([
+  [2, 3, true],
+  [3, 3, false],
+  [1, 0, false],
+])(
+  "isOutdatedReview for revision %j of %j is %j",
+  (revisionNumber, revisionCount, expected) => {
+    expect(
+      isOutdatedReview(review("alice", revisionNumber), revisionCount),
+    ).toBe(expected);
+  },
+);
+
+describe("summaryTitle", () => {
+  test("counts reviews and reviewers, singular and plural", () => {
+    expect(summaryTitle(reviewSummary([review("alice", 1)], 1))).toBe(
+      "1 review by 1 reviewer",
+    );
+    expect(
+      summaryTitle(
+        reviewSummary(
+          [review("alice", 1), review("alice", 2), review("bob", 2)],
+          2,
+        ),
+      ),
+    ).toBe("3 reviews by 2 reviewers");
+  });
+
+  test("mentions delegates", () => {
+    expect(
+      summaryTitle(reviewSummary([review("alice", 1, undefined, true)], 1)),
+    ).toBe("1 review by 1 reviewer, 1 a delegate");
+  });
+
+  test("warns when the reviews are outdated", () => {
+    expect(summaryTitle(reviewSummary([review("alice", 2)], 4))).toBe(
+      "1 review by 1 reviewer — newest is of revision 2, so these may no longer apply",
+    );
+  });
+});

tests/unit/revisionList.test.ts

@@ -0,0 +1,95 @@
+import { describe, expect, test } from "vitest";
+
+import { orderRevisions, revisionNumbers } from "@app/lib/revisionList";
+import type { RevisionListSettings } from "@app/lib/revisionListSettings";
+
+import { author, revision } from "./support/cobs";
+
+const alice = author("alice");
+const bob = author("bob");
+const carol = author("carol");
+
+// Timeline order: bob and carol both push before the patch author's second
+// revision, so grouping has to move revisions around.
+const revisions = [
+  revision({ id: "a1", author: alice }),
+  revision({ id: "b1", author: bob }),
+  revision({ id: "c1", author: carol }),
+  revision({ id: "a2", author: alice }),
+  revision({ id: "b2", author: bob }),
+];
+
+function settings(props: Partial<RevisionListSettings>): RevisionListSettings {
+  return {
+    sortDesc: false,
+    groupByAuthor: false,
+    showNumber: false,
+    showStats: false,
+    showReviewers: true,
+    ...props,
+  };
+}
+
+function ids(ordered: { id: string }[]) {
+  return ordered.map(r => r.id);
+}
+
+test("revisionNumbers numbers revisions by timeline position from 1", () => {
+  expect(revisionNumbers(revisions)).toEqual({
+    a1: 1,
+    b1: 2,
+    c1: 3,
+    a2: 4,
+    b2: 5,
+  });
+});
+
+describe("orderRevisions", () => {
+  test("keeps the timeline order by default", () => {
+    expect(ids(orderRevisions(revisions, alice.did, settings({})))).toEqual([
+      "a1",
+      "b1",
+      "c1",
+      "a2",
+      "b2",
+    ]);
+  });
+
+  test("reverses the timeline when sorting descending", () => {
+    expect(
+      ids(orderRevisions(revisions, alice.did, settings({ sortDesc: true }))),
+    ).toEqual(["b2", "a2", "c1", "b1", "a1"]);
+  });
+
+  test("groups by author, patch author first, others by first appearance", () => {
+    expect(
+      ids(
+        orderRevisions(revisions, carol.did, settings({ groupByAuthor: true })),
+      ),
+    ).toEqual(["c1", "a1", "a2", "b1", "b2"]);
+  });
+
+  test("sorting descending reverses within groups, not the group order", () => {
+    expect(
+      ids(
+        orderRevisions(
+          revisions,
+          alice.did,
+          settings({ groupByAuthor: true, sortDesc: true }),
+        ),
+      ),
+    ).toEqual(["a2", "a1", "b2", "b1", "c1"]);
+  });
+
+  test("does not reorder the caller's array", () => {
+    const input = [...revisions];
+    orderRevisions(input, alice.did, settings({ sortDesc: true }));
+    orderRevisions(
+      input,
+      alice.did,
+      settings({ groupByAuthor: true, sortDesc: true }),
+    );
+
+    expect(ids(input)).toEqual(ids(revisions));
+  });
+});

tests/unit/support/cobs.ts

@@ -0,0 +1,81 @@
+import type { Author } from "@bindings/cob/Author";
+import type { Operation } from "@bindings/cob/Operation";
+import type { Action } from "@bindings/cob/patch/Action";
+import type { Review } from "@bindings/cob/patch/Review";
+import type { Revision } from "@bindings/cob/patch/Revision";
+import type { CodeLocation } from "@bindings/cob/thread/CodeLocation";
+import type { Comment } from "@bindings/cob/thread/Comment";
+import type { Thread } from "@bindings/cob/thread/Thread";
+
+// Pass `null` for an author without an alias.
+export function author(name: string, alias: string | null = name): Author {
+  return { did: `did:key:z6Mk${name}`, alias: alias ?? undefined };
+}
+
+export function location(
+  path: string,
+  side: "old" | "new",
+  start: number,
+  end = start + 1,
+): CodeLocation {
+  const range = { type: "lines" as const, range: { start, end } };
+  return {
+    commit: "c".repeat(40),
+    path,
+    old: side === "old" ? range : null,
+    new: side === "new" ? range : null,
+  };
+}
+
+export function comment(
+  props: Partial<Comment<CodeLocation>> & { timestamp?: number } = {},
+): Comment<CodeLocation> {
+  const { timestamp = 0, ...rest } = props;
+  const by = rest.author ?? author("alice");
+  return {
+    id: "comment",
+    author: by,
+    edits: [{ author: by, timestamp, body: "body" }],
+    reactions: [],
+    replyTo: null,
+    location: null,
+    resolved: false,
+    ...rest,
+  };
+}
+
+export function thread(root: Comment<CodeLocation>): Thread<CodeLocation> {
+  return { root, replies: [] };
+}
+
+export function review(props: Partial<Review> = {}): Review {
+  return {
+    id: "review",
+    author: author("alice"),
+    summary: "",
+    comments: [],
+    timestamp: 0,
+    labels: [],
+    ...props,
+  };
+}
+
+export function revision(props: Partial<Revision> = {}): Revision {
+  return {
+    id: "revision",
+    author: author("alice"),
+    description: [],
+    base: "b".repeat(40),
+    head: "h".repeat(40),
+    timestamp: 0,
+    ...props,
+  };
+}
+
+export function operation(
+  by: Author,
+  timestamp: number,
+  actions: Action[],
+): Operation<Action> {
+  return { id: `op-${timestamp}`, author: by, timestamp, actions };
+}