From cbb21e75130c8a24c1519c8f736bf54b4b12b67c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C5=ABdolfs=20O=C5=A1i=C5=86=C5=A1?= <rudolfs@osins.org>
Date: Wed, 30 Sep 2026 15:31:09 +0200
Subject: [PATCH] Move diff annotation helpers into lib

The snippet and the code thread had their own copies.
---
 src/components/PierreDiff.svelte       | 79 +++------------------
 src/components/PierreSnippet.svelte    | 11 ++-
 src/components/ReviewCodeThread.svelte | 17 +----
 src/lib/pierreComments.ts              | 95 +++++++++++++++++++++++--
 tests/unit/pierreComments.test.ts      | 96 ++++++++++++++++++++++++++
 5 files changed, 202 insertions(+), 96 deletions(-)

diff --git a/src/components/PierreDiff.svelte b/src/components/PierreDiff.svelte
index 48993bc2a..18013d72e 100644
--- a/src/components/PierreDiff.svelte
+++ b/src/components/PierreDiff.svelte
@@ -32,8 +32,13 @@
   } from "@app/lib/pierreComments";
   import {
     anchorOf,
+    annotationSignature,
+    commentedLines,
+    composerLines,
+    entryStatus,
     fileAnnotations,
     isCommentableStatus,
+    itemIds,
   } from "@app/lib/pierreComments";
   import { parsePatch } from "@app/lib/pierreParse";
   import {
@@ -504,7 +509,7 @@
     if (!instance) {
       return;
     }
-    for (const id of itemIds(parsedFiles)) {
+    for (const id of itemIds(parsedFiles.map(file => file.name))) {
       const item = instance.getItem(id);
       if (item) {
         instance.updateItem({
@@ -558,18 +563,8 @@
     mountedHeaders.clear();
   }
 
-  // How this entry changed. `fileStatuses` is keyed by path, so the two entries
-  // of a type change would both read as whichever half the structured diff
-  // recorded last; Pierre parsed each entry on its own and knows which this is.
   function statusOf(fileDiff: FileDiffMetadata): FileStatus | undefined {
-    const status = fileStatuses?.get(fileDiff.name);
-    if (status === "added" && fileDiff.type === "deleted") {
-      return "deleted";
-    }
-    if (status === "deleted" && fileDiff.type === "new") {
-      return "added";
-    }
-    return status;
+    return entryStatus(fileStatuses?.get(fileDiff.name), fileDiff.type);
   }
 
   // Collapse or expand a single file, by CodeView item. Bumps `version`:
@@ -986,25 +981,10 @@
     };
   }
 
-  // The item id for each file, in the order they were given. A type change, a
-  // symlink replaced by a regular file for instance, is one path arriving as two
-  // entries, and CodeView throws on a duplicate id, so the repeats are numbered.
-  // The first keeps the bare path, which is what scroll targets and collapsed
-  // paths resolve by. NUL is the separator because a path cannot contain one.
-  function itemIds(files: FileDiffMetadata[]): string[] {
-    // eslint-disable-next-line svelte/prefer-svelte-reactivity -- transient local map, never reactive
-    const occurrences = new Map<string, number>();
-    return files.map(file => {
-      const seen = occurrences.get(file.name) ?? 0;
-      occurrences.set(file.name, seen + 1);
-      return seen === 0 ? file.name : `${file.name}\u0000${seen + 1}`;
-    });
-  }
-
   function buildItems(
     files: FileDiffMetadata[],
   ): CodeViewItem<LineAnnotation>[] {
-    const ids = itemIds(files);
+    const ids = itemIds(files.map(file => file.name));
     return files.map((fileDiff, index) => ({
       id: ids[index],
       type: "diff",
@@ -1046,17 +1026,6 @@
     tintStyle = undefined;
   }
 
-  function commentedLines(location: CodeLocation): number[] {
-    const range = location.new ?? location.old;
-    if (!range) return [];
-    if (range.type === "chars") return [range.line];
-    const lines: number[] = [];
-    for (let line = range.range.start; line < range.range.end; line++) {
-      lines.push(line);
-    }
-    return lines;
-  }
-
   function tintSelectors(
     side: AnnotationSide,
     line: number,
@@ -1109,23 +1078,6 @@
     tintStyle = style;
   }
 
-  // Which slots a file's annotations occupy — and only that. Replacing a file's
-  // annotation array makes Pierre drop every measured annotation height in that
-  // file and lay it out as if the comments had no height at all, which yanks the
-  // scroll position; so it is only told when the set of slots really changes.
-  // What goes *in* a slot is pushed straight into the mounted component instead
-  // (see `syncAnnotationSlots`), which is all a reply, an edit or a resolve
-  // touches.
-  function annotationSignature(
-    annotations: DiffLineAnnotation<LineAnnotation>[] | undefined,
-  ): string {
-    if (!annotations || annotations.length === 0) return "";
-    return annotations
-      .map(({ side, lineNumber }) => `${side}:${lineNumber}`)
-      .sort()
-      .join("|");
-  }
-
   // Open the new-comment composer on what the gutter marker covers. Pierre
   // reports the line range it was dragged over, on the side it started from.
   function openComposer(
@@ -1140,18 +1092,7 @@
     if (!isCommentableStatus(fileStatuses?.get(path))) {
       return;
     }
-    const side = range.side ?? "additions";
-    // A drag in a split diff can end on the other side, where the end line
-    // counts a different file. There is no range that means both, so keep the
-    // line it started from.
-    const crossSide = range.endSide !== undefined && range.endSide !== side;
-    const end = crossSide ? range.start : range.end;
-    composer = {
-      path,
-      side,
-      firstLine: Math.min(range.start, end),
-      lastLine: Math.max(range.start, end),
-    };
+    composer = { path, ...composerLines(range) };
     composerBody = "";
   }
 
@@ -1172,7 +1113,7 @@
     }
     untrack(() => {
       let changed = false;
-      for (const id of itemIds(parsedFiles)) {
+      for (const id of itemIds(parsedFiles.map(file => file.name))) {
         const item = instance.getItem(id);
         if (item?.type !== "diff") {
           continue;
diff --git a/src/components/PierreSnippet.svelte b/src/components/PierreSnippet.svelte
index 175934020..df250805d 100644
--- a/src/components/PierreSnippet.svelte
+++ b/src/components/PierreSnippet.svelte
@@ -10,7 +10,10 @@
   import type { CodeComments } from "@app/lib/codeComments";
   import { forwardPatchActivityContext } from "@app/lib/patchActivityContext";
   import type { LineAnnotation } from "@app/lib/pierreComments";
-  import { fileAnnotations } from "@app/lib/pierreComments";
+  import {
+    annotationSignature,
+    fileAnnotations,
+  } from "@app/lib/pierreComments";
   import {
     codeLineHeight,
     getWorkerPool,
@@ -90,11 +93,7 @@
   const annotationList = $derived.by(() =>
     fileAnnotations(path, threads, undefined),
   );
-  const annotationKey = $derived(
-    annotationList
-      .map(({ side, lineNumber }) => `${side}:${lineNumber}`)
-      .join("|"),
-  );
+  const annotationKey = $derived(annotationSignature(annotationList));
   let stableAnnotations = $state.raw<DiffLineAnnotation<LineAnnotation>[]>([]);
   let stableKey: string | undefined;
   $effect(() => {
diff --git a/src/components/ReviewCodeThread.svelte b/src/components/ReviewCodeThread.svelte
index f5d76679a..136894fe3 100644
--- a/src/components/ReviewCodeThread.svelte
+++ b/src/components/ReviewCodeThread.svelte
@@ -10,6 +10,7 @@
   import { diffOptions } from "@app/lib/diffOptions.svelte";
   import { cachedGetDiffText } from "@app/lib/invoke";
   import { sliceForRange } from "@app/lib/patchSlice";
+  import { lineRangeOf } from "@app/lib/pierreComments";
 
   import PierreSnippet from "@app/components/PierreSnippet.svelte";
 
@@ -86,20 +87,6 @@
       resolvedBy,
     };
   }
-
-  /// Which lines of which side a comment is about, in the numbering
-  /// `sliceForRange` matches on.
-  function rangeOf(
-    location: CodeLocation,
-  ): { side: "old" | "new"; start: number; end: number } | undefined {
-    const range = location.new ?? location.old;
-    if (!range) return undefined;
-    const side = location.new ? "new" : "old";
-    if (range.type === "chars") {
-      return { side, start: range.line, end: range.line + 1 };
-    }
-    return { side, start: range.range.start, end: range.range.end };
-  }
 </script>
 
 <style>
@@ -154,7 +141,7 @@
       {#each threads as thread (thread.root.id)}
         {@const location = thread.root.location}
         {#if location}
-          {@const range = rangeOf(location)}
+          {@const range = lineRangeOf(location)}
           {@const slice =
             range &&
             sliceForRange(filePatch, range.side, range.start, range.end)}
diff --git a/src/lib/pierreComments.ts b/src/lib/pierreComments.ts
index 637e5b04e..0b2d922a6 100644
--- a/src/lib/pierreComments.ts
+++ b/src/lib/pierreComments.ts
@@ -103,13 +103,13 @@ export function locationOf(
 /// The lines a comment covers, as text: `R12` for one line on the new side,
 /// `L3-L9` for a range on the old one.
 export function formatAnchorLines(location: CodeLocation): string | undefined {
-  const range = location.new ?? location.old;
+  const range = lineRangeOf(location);
   if (!range) return undefined;
-  const marker = location.new ? "R" : "L";
-  if (range.type === "chars") return `${marker}${range.line}`;
-  const start = range.range.start;
-  const end = range.range.end - 1;
-  return start >= end ? `${marker}${end}` : `${marker}${start}-${marker}${end}`;
+  const marker = range.side === "new" ? "R" : "L";
+  const end = range.end - 1;
+  return range.start >= end
+    ? `${marker}${end}`
+    : `${marker}${range.start}-${marker}${end}`;
 }
 
 /// A comment's anchor with the file it is in, e.g. `foo.ts:R12` or
@@ -201,3 +201,86 @@ export function commentCountsByPath(
   }
   return counts;
 }
+
+// Which lines of which side a comment is about, end exclusive.
+export function lineRangeOf(
+  location: CodeLocation,
+): { side: "old" | "new"; start: number; end: number } | undefined {
+  const range = location.new ?? location.old;
+  if (!range) return undefined;
+  const side = location.new ? "new" : "old";
+  if (range.type === "chars") {
+    return { side, start: range.line, end: range.line + 1 };
+  }
+  return { side, start: range.range.start, end: range.range.end };
+}
+
+export function commentedLines(location: CodeLocation): number[] {
+  const range = lineRangeOf(location);
+  if (!range) return [];
+  const lines: number[] = [];
+  for (let line = range.start; line < range.end; line++) {
+    lines.push(line);
+  }
+  return lines;
+}
+
+// Which slots a file's annotations occupy, and only that. Replacing a file's
+// annotation array makes Pierre drop every measured annotation height in that
+// file and lay it out as if the comments had no height at all, which yanks the
+// scroll position; so it is only told when the set of slots really changes.
+// What goes *in* a slot is pushed straight into the mounted component instead
+// (see `syncAnnotationSlots` in PierreDiff), which is all a reply, an edit or a
+// resolve touches.
+export function annotationSignature(
+  annotations: DiffLineAnnotation<LineAnnotation>[] | undefined,
+): string {
+  return (annotations ?? [])
+    .map(({ side, lineNumber }) => `${side}:${lineNumber}`)
+    .sort()
+    .join("|");
+}
+
+// The lines a gutter drag covers. A drag in a split diff can end on the other
+// side, where the end line counts a different file. There is no range that
+// means both, so keep the line it started from.
+export function composerLines(range: {
+  start: number;
+  end: number;
+  side?: AnnotationSide;
+  endSide?: AnnotationSide;
+}): { side: AnnotationSide; firstLine: number; lastLine: number } {
+  const side = range.side ?? "additions";
+  const crossSide = range.endSide !== undefined && range.endSide !== side;
+  const end = crossSide ? range.start : range.end;
+  return {
+    side,
+    firstLine: Math.min(range.start, end),
+    lastLine: Math.max(range.start, end),
+  };
+}
+
+// How a diff entry changed. `fileStatuses` is keyed by path, so the two entries
+// of a type change would both read as whichever half the structured diff
+// recorded last; Pierre parsed each entry on its own and knows which this is.
+export function entryStatus(
+  status: FileStatus | undefined,
+  entryType: string,
+): FileStatus | undefined {
+  if (status === "added" && entryType === "deleted") return "deleted";
+  if (status === "deleted" && entryType === "new") return "added";
+  return status;
+}
+
+// An id per file, in order. A type change is one path arriving as two entries,
+// and CodeView throws on a duplicate id, so the repeats are numbered. The
+// first keeps the bare path, which is what scroll targets and collapsed paths
+// resolve by. NUL is the separator because a path cannot contain one.
+export function itemIds(names: string[]): string[] {
+  const occurrences = new Map<string, number>();
+  return names.map(name => {
+    const seen = occurrences.get(name) ?? 0;
+    occurrences.set(name, seen + 1);
+    return seen === 0 ? name : `${name}\u0000${seen + 1}`;
+  });
+}
diff --git a/tests/unit/pierreComments.test.ts b/tests/unit/pierreComments.test.ts
index e2589bab4..cfb3afc50 100644
--- a/tests/unit/pierreComments.test.ts
+++ b/tests/unit/pierreComments.test.ts
@@ -6,11 +6,17 @@ import { describe, expect, test } from "vitest";
 import type { ComposerTarget } from "@app/lib/pierreComments";
 import {
   anchorOf,
+  annotationSignature,
   commentCountsByPath,
+  commentedLines,
+  composerLines,
+  entryStatus,
   fileAnnotations,
   formatAnchor,
   formatAnchorLines,
   isCommentableStatus,
+  itemIds,
+  lineRangeOf,
   locationOf,
 } from "@app/lib/pierreComments";
 
@@ -258,3 +264,93 @@ describe("commentCountsByPath", () => {
     expect(counts.get("a.ts")).toEqual({ resolved: 0, unresolved: 1 });
   });
 });
+
+describe("line ranges", () => {
+  test("lineRangeOf prefers the new side and ends after the last line", () => {
+    expect(
+      lineRangeOf(at("a.ts", { old: lines(1, 2), new: lines(3, 5) })),
+    ).toEqual({ side: "new", start: 3, end: 5 });
+    expect(lineRangeOf(at("a.ts", { old: lines(1, 2) }))).toEqual({
+      side: "old",
+      start: 1,
+      end: 2,
+    });
+    expect(
+      lineRangeOf(
+        at("a.ts", {
+          new: { type: "chars", line: 7, range: { start: 0, end: 3 } },
+        }),
+      ),
+    ).toEqual({ side: "new", start: 7, end: 8 });
+    expect(lineRangeOf(at("a.ts", {}))).toBeUndefined();
+  });
+
+  test("commentedLines lists each line of the range", () => {
+    expect(commentedLines(at("a.ts", { new: lines(3, 6) }))).toEqual([3, 4, 5]);
+    expect(commentedLines(at("a.ts", {}))).toEqual([]);
+  });
+});
+
+test("annotationSignature only depends on which slots are taken", () => {
+  const a = { side: "additions" as const, lineNumber: 1, metadata: {} };
+  const b = { side: "deletions" as const, lineNumber: 2, metadata: {} };
+
+  expect(annotationSignature([b, a] as never)).toBe(
+    annotationSignature([a, b] as never),
+  );
+  expect(annotationSignature([a] as never)).not.toBe(
+    annotationSignature([b] as never),
+  );
+  expect(annotationSignature(undefined)).toBe("");
+});
+
+describe("composerLines", () => {
+  test("orders a range dragged upwards", () => {
+    expect(composerLines({ start: 9, end: 4, side: "deletions" })).toEqual({
+      side: "deletions",
+      firstLine: 4,
+      lastLine: 9,
+    });
+  });
+
+  test("keeps the starting line of a drag that ends on the other side", () => {
+    expect(
+      composerLines({
+        start: 3,
+        end: 8,
+        side: "additions",
+        endSide: "deletions",
+      }),
+    ).toEqual({ side: "additions", firstLine: 3, lastLine: 3 });
+  });
+
+  test("defaults to the new side", () => {
+    expect(composerLines({ start: 2, end: 5, endSide: "additions" })).toEqual({
+      side: "additions",
+      firstLine: 2,
+      lastLine: 5,
+    });
+  });
+});
+
+test.each([
+  ["added", "deleted", "deleted"],
+  ["deleted", "new", "added"],
+  ["added", "new", "added"],
+  ["modified", "change", "modified"],
+  [undefined, "new", undefined],
+] as const)(
+  "entryStatus of a %j path's %j entry is %j",
+  (status, type, expected) => {
+    expect(entryStatus(status, type)).toBe(expected);
+  },
+);
+
+test("itemIds numbers repeated paths, keeping the first bare", () => {
+  expect(itemIds(["a", "b", "a", "a"])).toEqual([
+    "a",
+    "b",
+    "a\u00002",
+    "a\u00003",
+  ]);
+});