fix(visits): surface batch write failures instead of a silent 200

Found by driving /rep in a real browser session, not by a unit test:
submitting a check-in returned 200 OK with body {"created":0,"pinned":1},
and the visit vanished with no trace while the customer coordinate pin
still landed. Reproduced against the live database (confirmed via
direct PocketBase query) and again after the fix, with the response
now 502 and the entry correctly retained in the outbox.

Root cause was two-layered:

1. visits-service.ts caught every per-visit write error and continued
   silently, discarding the message. A per-item failure (one stale
   customer id in a batch) is meant to be swallowed so it doesn't cost
   the other visits — but the same catch also swallowed a systemic
   failure (every item in the batch hitting an identical PB validation
   error), which looks completely different: a bare `created: 0` gives
   the caller no way to tell "nothing to do" from "everything failed".
   recordVisits now returns an `errors: {customerId, message}[]` array
   instead of discarding the exception.

2. POST /api/v1/visits returned HTTP 200 whenever the request was
   well-formed, regardless of whether anything was actually written.
   The offline outbox treats any 2xx as delivered and discards the
   entry — so a total write failure was indistinguishable from success
   at the one layer that decides whether to keep retrying. The route
   now returns 502 when `created === 0`, which the outbox already
   retries (see the existing 5xx-keeps-the-entry test), rather than a
   4xx, which it treats as permanent and drops.

The concrete trigger was live: rep_visits.position_source doesn't yet
allow "phone" (flagged in the previous commit). That specific gap will
close once it's added to the PB select, but this fix stands on its own
— any future rejected field would have hit the identical silent-loss
path.

12 new tests (service: captured errors on per-item and total failure;
route: 502 vs 200 boundary, repId taken from the session not the body;
outbox: a 502 keeps the entry queued rather than treating it as sent).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/app/(rep)/rep/outbox.test.ts

@@ -121,6 +121,22 @@ describe("flushOutbox", () => {
     expect(readOutbox()[0].lastError).toMatch(/Failed to fetch/);
   });
 
+  it("keeps a check-in queued when the server accepted the request but wrote nothing", async () => {
+    // Live-reproduced bug: recordVisits can create 0 rows (a rejected
+    // position_source value, or any batch where every item fails) —
+    // the route now answers 502 rather than 200 for that case. This
+    // proves the outbox's existing 5xx-retry path actually catches it,
+    // so a fully-failed write is retried instead of being treated as
+    // delivered and discarded.
+    enqueue(CHECK_IN);
+    const fetchImpl = vi.fn(async () => response(502)) as unknown as typeof fetch;
+
+    const result = await flushOutbox("tok", fetchImpl);
+
+    expect(result).toMatchObject({ sent: 0, remaining: 1 });
+    expect(readOutbox()).toHaveLength(1);
+  });
+
   it("keeps an entry on a 5xx and counts the attempt", async () => {
     enqueue(CHECK_IN);
     const fetchImpl = vi.fn(async () => response(503)) as unknown as typeof fetch;

src/app/api/v1/visits/route.test.ts

@@ -0,0 +1,122 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { NextRequest } from "next/server";
+
+const getBrowserSessionUser = vi.fn();
+const recordVisits = vi.fn();
+
+vi.mock("@/lib/server/browser-session", () => ({
+  getBrowserSessionUser: () => getBrowserSessionUser(),
+}));
+
+vi.mock("@/lib/server/visits-service", () => ({
+  recordVisits: (args: unknown) => recordVisits(args),
+}));
+
+import { POST } from "./route";
+
+function request(body: unknown, token = "tok"): NextRequest {
+  return new NextRequest("http://localhost:4001/api/v1/visits", {
+    method: "POST",
+    headers: { "Content-Type": "application/json", Authorization: `Bearer ${token}` },
+    body: JSON.stringify(body),
+  });
+}
+
+const VALID_BODY = {
+  lat: -26.4,
+  lon: 27.4,
+  positionSource: "phone",
+  visits: [{ customerId: "c1" }],
+};
+
+beforeEach(() => {
+  vi.clearAllMocks();
+  getBrowserSessionUser.mockResolvedValue({ id: "u-tiaan", user_type: ["REP"] });
+});
+
+describe("POST /api/v1/visits", () => {
+  it("scopes the write to the session's own rep id, not anything the client sends", async () => {
+    recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] });
+
+    await POST(request({ ...VALID_BODY, repId: "someone-else" }));
+
+    expect(recordVisits).toHaveBeenCalledWith(
+      expect.objectContaining({ repId: "u-tiaan" })
+    );
+  });
+
+  it("returns 502, not 200, when every visit in the batch failed to write", async () => {
+    // This is the failure this route exists to prevent: recordVisits
+    // can legitimately create 0 rows (e.g. a PB schema mismatch on
+    // position_source, reproduced live before this fix), and a 200 in
+    // that case tells the offline outbox the entry was delivered — it
+    // gets discarded and the visit is lost with no trace.
+    recordVisits.mockResolvedValue({
+      created: 0,
+      pinned: 1,
+      visitIds: [],
+      errors: [{ customerId: "c1", message: "Invalid value phone." }],
+    });
+
+    const res = await POST(request(VALID_BODY));
+
+    expect(res.status).toBe(502);
+    const json = await res.json();
+    expect(json.errors[0].message).toMatch(/Invalid value phone/);
+  });
+
+  it("returns 200 when at least one visit was written, even alongside partial failures", async () => {
+    recordVisits.mockResolvedValue({
+      created: 1,
+      pinned: 0,
+      visitIds: ["v1"],
+      errors: [{ customerId: "c2", message: "stale customer id" }],
+    });
+
+    const res = await POST(request({ ...VALID_BODY, visits: [{ customerId: "c1" }, { customerId: "c2" }] }));
+
+    expect(res.status).toBe(200);
+  });
+
+  it("defaults an unrecognised positionSource to phone", async () => {
+    recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] });
+
+    await POST(request({ ...VALID_BODY, positionSource: "carrier-pigeon" }));
+
+    expect(recordVisits).toHaveBeenCalledWith(
+      expect.objectContaining({ positionSource: "phone" })
+    );
+  });
+
+  it("rejects a request with no visits before calling the service", async () => {
+    const res = await POST(request({ ...VALID_BODY, visits: [] }));
+    expect(res.status).toBe(400);
+    expect(recordVisits).not.toHaveBeenCalled();
+  });
+
+  it("rejects non-numeric coordinates", async () => {
+    const res = await POST(request({ ...VALID_BODY, lat: "not a number" }));
+    expect(res.status).toBe(400);
+    expect(recordVisits).not.toHaveBeenCalled();
+  });
+
+  it("rejects an unauthenticated caller", async () => {
+    getBrowserSessionUser.mockRejectedValue(
+      Object.assign(new Error("Not authenticated."), { status: 401 })
+    );
+
+    const res = await POST(request(VALID_BODY));
+    expect(res.status).toBe(401);
+    expect(recordVisits).not.toHaveBeenCalled();
+  });
+
+  it("drops a visit entry with a blank customerId rather than passing it through", async () => {
+    recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] });
+
+    await POST(request({ ...VALID_BODY, visits: [{ customerId: "" }, { customerId: "c1" }] }));
+
+    expect(recordVisits).toHaveBeenCalledWith(
+      expect.objectContaining({ visits: [{ customerId: "c1", notes: undefined, pinCustomer: false }] })
+    );
+  });
+});

src/app/api/v1/visits/route.ts

@@ -106,6 +106,17 @@ export async function POST(req: NextRequest) {
       visits,
     });
 
+    // Nothing written is a failed submission, not a successful empty
+    // one — a 200 here would tell the outbox the entry was delivered
+    // and it would discard it, silently losing every visit in the
+    // batch. 502 (not a 4xx) so the outbox's "permanent failure, drop
+    // it" rule doesn't apply: this is very likely transient or a
+    // config issue (see the errors array), and it should keep retrying
+    // until it either succeeds or someone looks at the message.
+    if (result.created === 0) {
+      return NextResponse.json(result, { status: 502 });
+    }
+
     return NextResponse.json(result);
   } catch (error) {
     const message = error instanceof Error ? error.message : "Failed to record visits";

src/lib/server/visits-service.test.ts

@@ -188,6 +188,39 @@ describe("recordVisits", () => {
     expect(pb.store.rep_visits[0].customer).toBe("c2");
   });
 
+  it("reports the failure reason rather than swallowing it", async () => {
+    // Live-verified failure mode: a `rep_visits.position_source` value
+    // PocketBase hasn't been given yet fails every item in the batch
+    // identically. A bare `created: 0` with no explanation looks
+    // indistinguishable from "nothing to do" — the actual cause has to
+    // survive the catch.
+    const pb = fakePb({ customers: [{ id: "c1" }] as Row[] });
+    pb.failOn("rep_visits", "c1");
+
+    const result = await recordVisits({ ...base, visits: [{ customerId: "c1" }] }, pb);
+
+    expect(result.errors).toEqual([{ customerId: "c1", message: "create failed" }]);
+  });
+
+  it("reports one error per failed customer in a mixed batch", async () => {
+    const pb = fakePb({ customers: [{ id: "c1" }, { id: "c2" }] as Row[] });
+    pb.failOn("rep_visits", "c1");
+
+    const result = await recordVisits(
+      { ...base, visits: [{ customerId: "c1" }, { customerId: "c2" }] },
+      pb
+    );
+
+    expect(result.errors).toEqual([{ customerId: "c1", message: "create failed" }]);
+    expect(result.created).toBe(1);
+  });
+
+  it("returns no errors on a clean run", async () => {
+    const pb = fakePb({ customers: [{ id: "c1" }] as Row[] });
+    const result = await recordVisits({ ...base, visits: [{ customerId: "c1" }] }, pb);
+    expect(result.errors).toEqual([]);
+  });
+
   it("updates lastvisiteddate on the customer", async () => {
     const pb = fakePb({ customers: [{ id: "c1" }] as Row[] });
 

src/lib/server/visits-service.ts

@@ -86,6 +86,14 @@ export interface RecordVisitsResult {
   created: number;
   pinned: number;
   visitIds: string[];
+  /**
+   * One entry per customer whose visit row failed to write, with the
+   * underlying PocketBase message. Never swallowed silently — a schema
+   * mismatch (e.g. an unapproved `position_source` value) fails every
+   * item in the batch identically, and the caller needs to see why
+   * rather than receive a bare `created: 0`.
+   */
+  errors: { customerId: string; message: string }[];
 }
 
 /**
@@ -158,7 +166,7 @@ export async function recordVisits(
   }
 
   const visitedAt = args.visitedAt ?? new Date().toISOString();
-  const result: RecordVisitsResult = { created: 0, pinned: 0, visitIds: [] };
+  const result: RecordVisitsResult = { created: 0, pinned: 0, visitIds: [], errors: [] };
 
   for (const visit of args.visits) {
     if (!visit.customerId) continue;
@@ -191,7 +199,16 @@ export async function recordVisits(
       });
       result.created++;
       result.visitIds.push((row as { id: string }).id);
-    } catch {
+    } catch (error) {
+      // Caught here, not thrown, so one stale customer id in a batch
+      // doesn't cost the rest of the visits — but the message is kept
+      // rather than discarded, so a *systemic* failure (every item in
+      // the batch hitting the same PB validation error) is visible to
+      // the caller instead of looking like a clean, empty success.
+      result.errors.push({
+        customerId: visit.customerId,
+        message: error instanceof Error ? error.message : "Failed to record visit",
+      });
       continue;
     }