fix(collection-confirm): thread null cost through appendCostNote to preserve audit on null→0

Co-authored-by: Orca <help@stably.ai>

src/app/(app)/collection-confirm/view-model.test.ts

@@ -9,6 +9,7 @@ import {
   buildConfirmPatch,
   buildReturnPatch,
   canConfirmRow,
+  formatNoteBody,
   groupRowsByPurchaseOrder,
   nextItemStatusForConfirm,
   parseNotesFeed,
@@ -443,6 +444,52 @@ describe("appendCostNote", () => {
       body: { prevCost: 45, newCost: 50 },
     });
   });
+
+  it("writes a cost note when prevCost is null and newCost is 0", () => {
+    // Regression: the old code lossy-fell-back `null → 0` to `0 → 0`,
+    // which short-circuited inside appendCostNote and silently skipped
+    // the audit note. The fix threads null through so the equality
+    // check `null === 0` is false and the note IS recorded.
+    const result = appendCostNote(null, { id: "u-1", name: "sj" }, null, 0);
+    const parsed = JSON.parse(result) as Array<Record<string, unknown>>;
+    expect(parsed).toHaveLength(1);
+    expect(parsed[0]).toMatchObject({
+      category: "cost",
+      body: { prevCost: null, newCost: 0 },
+    });
+  });
+
+  it("writes a cost note when prevCost is null and newCost is a positive number", () => {
+    const result = appendCostNote("", { id: "u-1", name: "sj" }, null, 50);
+    const parsed = JSON.parse(result) as Array<Record<string, unknown>>;
+    expect(parsed).toHaveLength(1);
+    expect(parsed[0]).toMatchObject({
+      category: "cost",
+      body: { prevCost: null, newCost: 50 },
+    });
+  });
+});
+
+describe("formatNoteBody", () => {
+  it("renders cost transitions as 'Cost changed: prev → next'", () => {
+    expect(formatNoteBody({ prevCost: 45, newCost: 50 })).toBe("Cost changed: 45 → 50");
+  });
+
+  it("renders a null prevCost (no prior cost) as 'none'", () => {
+    // Diagnostic UX: when the audit entry records a transition from
+    // "no prior cost" to a value, the operator sees "none → 50"
+    // rather than a raw JSON dump.
+    expect(formatNoteBody({ prevCost: null, newCost: 50 })).toBe(
+      "Cost changed: none → 50",
+    );
+    expect(formatNoteBody({ prevCost: null, newCost: 0 })).toBe(
+      "Cost changed: none → 0",
+    );
+  });
+
+  it("passes string bodies through unchanged", () => {
+    expect(formatNoteBody("hello world")).toBe("hello world");
+  });
 });
 
 describe("dedupeAttachments", () => {

src/app/(app)/collection-confirm/view-model.ts

@@ -1,6 +1,6 @@
 export type NoteCategory = "collection" | "note" | "problem" | "quantity" | "confirm" | "cost";
 
-export type NoteBody = string | { prevCost?: number; newCost?: number; [k: string]: unknown };
+export type NoteBody = string | { prevCost?: number | null; newCost?: number; [k: string]: unknown };
 
 export type NoteEntry = {
   ts: string;
@@ -292,7 +292,7 @@ export function buildReturnPatch(input: ReturnPatchInput): ReturnPatch {
 export function appendCostNote(
   rawNotes: string | null | undefined,
   actor: ConfirmActor,
-  prevCost: number,
+  prevCost: number | null,
   newCost: number,
 ): string {
   if (prevCost === newCost) return rawNotes ?? "";
@@ -337,10 +337,11 @@ export function formatNoteBody(body: NoteBody): string {
   if (typeof body === "string") return body;
   if (body && typeof body === "object") {
     if ("prevCost" in body || "newCost" in body) {
-      const prev = (body as { prevCost?: number }).prevCost;
+      const prev = (body as { prevCost?: number | null }).prevCost;
       const next = (body as { newCost?: number }).newCost;
-      if (typeof prev === "number" && typeof next === "number") {
-        return `Cost changed: ${prev} → ${next}`;
+      if (typeof next === "number") {
+        const prevLabel = typeof prev === "number" ? String(prev) : "none";
+        return `Cost changed: ${prevLabel} → ${next}`;
       }
     }
     return JSON.stringify(body);

src/lib/server/collection-confirm-service.test.ts

@@ -432,6 +432,62 @@ describe("updateOrderItemCost", () => {
     expect(failingGetOne).toHaveBeenCalledWith("row-1");
     expect(dummyUpdate).not.toHaveBeenCalled();
   });
+
+  it("writes cost_price and appends a cost note when going from null to 0", async () => {
+    // Regression: the lossy fallback `prevCost ?? 0` collapsed a true
+    // `null → 0` transition into `0 → 0` inside appendCostNote, which
+    // short-circuited and silently dropped the audit note. The fix
+    // threads the original `null` through; this test pins that
+    // behaviour.
+    const existing = { id: "row-1", cost_price: null, notes: null };
+    const { admin, update } = makeAdminForUpdate({ existing });
+    vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never);
+
+    const result = await updateOrderItemCost("row-1", {
+      cost_price: 0,
+      actor: { id: "u-1", name: "sj" },
+    });
+
+    expect(update).toHaveBeenCalledTimes(1);
+    const [, patch] = update.mock.calls[0] as [
+      string,
+      { cost_price: number; notes: string },
+    ];
+    expect(patch.cost_price).toBe(0);
+    const parsed = JSON.parse(patch.notes) as Array<Record<string, unknown>>;
+    expect(parsed).toHaveLength(1);
+    expect(parsed[0]).toMatchObject({
+      category: "cost",
+      actorId: "u-1",
+      actorName: "sj",
+      body: { prevCost: null, newCost: 0 },
+    });
+
+    expect(result.cost_price).toBe(0);
+  });
+
+  it("writes cost_price and appends a cost note when going from null to a positive number", async () => {
+    const existing = { id: "row-1", cost_price: null, notes: null };
+    const { admin, update } = makeAdminForUpdate({ existing });
+    vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never);
+
+    await updateOrderItemCost("row-1", {
+      cost_price: 50,
+      actor: { id: "u-1", name: "sj" },
+    });
+
+    const [, patch] = update.mock.calls[0] as [
+      string,
+      { cost_price: number; notes: string },
+    ];
+    expect(patch.cost_price).toBe(50);
+    const parsed = JSON.parse(patch.notes) as Array<Record<string, unknown>>;
+    expect(parsed).toHaveLength(1);
+    expect(parsed[0]).toMatchObject({
+      category: "cost",
+      body: { prevCost: null, newCost: 50 },
+    });
+  });
 });
 
 describe("confirmOrderItemsForInvoicing", () => {
@@ -607,4 +663,36 @@ describe("confirmOrderItemsForInvoicing", () => {
     const notes = JSON.parse(patch.notes as string) as Array<Record<string, unknown>>;
     expect(notes.map((entry) => entry.category)).toEqual(["confirm"]);
   });
+
+  it("writes cost_price and appends a cost note when overriding from null to 0", async () => {
+    // Regression for the lossy `prevCost ?? 0` fallback that
+    // collapsed a `null → 0` transition into `0 → 0` and skipped the
+    // cost note. The fix threads null through; this test pins it.
+    const rows = [
+      {
+        id: "row-1",
+        item_status: "collected",
+        cost_price: null,
+        notes: null,
+        order_item_details: null,
+      },
+    ];
+    const { admin, update } = makeAdminForConfirm({ rows });
+    vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never);
+
+    await confirmOrderItemsForInvoicing(["row-1"], {
+      invoice: { date: "2026-08-03", number: "INV-1" },
+      costOverrides: { "row-1": 0 },
+      actor: { id: "u-1", name: "sj" },
+    });
+
+    expect(update).toHaveBeenCalledTimes(1);
+    const patch = update.mock.calls[0]?.[1] as Record<string, unknown>;
+    expect(patch.cost_price).toBe(0);
+    const notes = JSON.parse(patch.notes as string) as Array<Record<string, unknown>>;
+    // The confirm note (from buildConfirmPatch) is appended first,
+    // then the cost note — same shape as any other override.
+    expect(notes.map((entry) => entry.category)).toEqual(["confirm", "cost"]);
+    expect(notes[1]?.body).toEqual({ prevCost: null, newCost: 0 });
+  });
 });

src/lib/server/collection-confirm-service.ts

@@ -217,14 +217,11 @@ export async function listCollectedItems(): Promise<ConfirmRow[]> {
  * newly-appended notes feed (preserving any prior entries) in a single
  * round-trip.
  *
- * `prevCost` is read from the existing row's `cost_price`. When the row
- * has no prior cost (null), `appendCostNote` receives `0` as the
- * previous cost — the early-return check still treats `null → 0` as a
- * change (since `null === 0` is `false`), and the note records the
- * transition as `prevCost: 0`. For `null → X > 0` the note correctly
- * captures the move; for `null → 0` the `appendCostNote` internal
- * short-circuit returns the existing notes unchanged, so the only
- * side-effect is the `cost_price: 0` write.
+ * `prevCost` is read from the existing row's `cost_price` and threaded
+ * through to `appendCostNote` verbatim (including `null` when the row
+ * has no prior cost). The `null === 0` check in `appendCostNote` is
+ * `false`, so `null → 0` is treated as a real change and a cost note
+ * IS appended with `{ prevCost: null, newCost: 0 }`.
  */
 export async function updateOrderItemCost(
   id: string,
@@ -240,7 +237,7 @@ export async function updateOrderItemCost(
   const newNotes = appendCostNote(
     (existing as { notes?: string | null }).notes ?? null,
     args.actor,
-    prevCost ?? 0,
+    prevCost,
     args.cost_price,
   );
   const updated = await pb
@@ -296,7 +293,7 @@ export async function confirmOrderItemsForInvoicing(
       notes = appendCostNote(
         confirmPatch.notes,
         args.actor,
-        currentCost ?? 0,
+        currentCost,
         costOverride,
       );
     }