Claude/project usability review ad4940

server/__tests__/mail-batch.test.ts

@@ -0,0 +1,137 @@
+process.env.NODE_ENV = 'test';
+process.env.JWT_SECRET = 'test-secret-key';
+
+import { describe, it, expect, beforeAll } from 'vitest';
+import request from 'supertest';
+import type { Express } from 'express';
+import { createTestApp, login } from './helpers.js';
+
+/**
+ * Exercises the batched invoice-send route (`POST /api/mail/send-invoices`).
+ * SMTP is not configured in tests, so `sendInvoiceEmail` reports failure —
+ * which is exactly what lets us assert the server LOOPS over every id and
+ * reports a per-invoice outcome without needing a live mail server. The two
+ * deterministic reasons ("no client email" for a client without an email,
+ * "invoice not found" for an unknown id) are asserted directly.
+ */
+let app: Express;
+let token: string;
+let clientWithEmailId: string;
+let clientNoEmailId: string;
+let invoiceWithEmailId: string;
+let invoiceNoEmailId: string;
+
+const INVOICE_WITH_EMAIL = 'MAIL-TEST-0001';
+const INVOICE_NO_EMAIL = 'MAIL-TEST-0002';
+
+function auth(req: request.Test) {
+  return req.set('Authorization', `Bearer ${token}`);
+}
+
+beforeAll(async () => {
+  app = await createTestApp();
+  token = await login(app);
+
+  const clientWithEmail = await auth(request(app).post('/api/clients')).send({
+    name: 'Client With Email',
+    contact_email: 'billing@withemail.example',
+  });
+  expect(clientWithEmail.status).toBe(201);
+  clientWithEmailId = clientWithEmail.body.id;
+
+  const clientNoEmail = await auth(request(app).post('/api/clients')).send({
+    name: 'Client No Email',
+  });
+  expect(clientNoEmail.status).toBe(201);
+  clientNoEmailId = clientNoEmail.body.id;
+
+  const invoiceWithEmail = await auth(request(app).post('/api/invoices')).send({
+    invoice_number: INVOICE_WITH_EMAIL,
+    client_id: clientWithEmailId,
+    issue_date: '2026-07-01',
+    due_date: '2026-08-01',
+    subtotal: 100,
+    tax_total: 10,
+    total: 110,
+  });
+  expect(invoiceWithEmail.status).toBe(201);
+  invoiceWithEmailId = invoiceWithEmail.body.id;
+
+  const invoiceNoEmail = await auth(request(app).post('/api/invoices')).send({
+    invoice_number: INVOICE_NO_EMAIL,
+    client_id: clientNoEmailId,
+    issue_date: '2026-07-02',
+    due_date: '2026-08-02',
+    subtotal: 50,
+    tax_total: 5,
+    total: 55,
+  });
+  expect(invoiceNoEmail.status).toBe(201);
+  invoiceNoEmailId = invoiceNoEmail.body.id;
+});
+
+describe('POST /api/mail/send-invoices', () => {
+  it('rejects unauthenticated requests with 401', async () => {
+    const res = await request(app)
+      .post('/api/mail/send-invoices')
+      .send({ invoiceIds: [invoiceWithEmailId] });
+    expect(res.status).toBe(401);
+  });
+
+  it('returns 400 when invoiceIds is not an array', async () => {
+    const res = await auth(request(app).post('/api/mail/send-invoices')).send({
+      invoiceIds: 'not-an-array',
+    });
+    expect(res.status).toBe(400);
+  });
+
+  it('returns 400 for an empty array', async () => {
+    const res = await auth(request(app).post('/api/mail/send-invoices')).send({
+      invoiceIds: [],
+    });
+    expect(res.status).toBe(400);
+  });
+
+  it('reports "no client email" for an invoice whose client has no email', async () => {
+    const res = await auth(request(app).post('/api/mail/send-invoices')).send({
+      invoiceIds: [invoiceNoEmailId],
+    });
+    expect(res.status).toBe(200);
+    expect(res.body.sent).toBe(0);
+    expect(res.body.total).toBe(1);
+    expect(res.body.failures).toEqual([
+      { invoiceNumber: INVOICE_NO_EMAIL, reason: 'no client email' },
+    ]);
+  });
+
+  it('reports "invoice not found" for an unknown id', async () => {
+    const res = await auth(request(app).post('/api/mail/send-invoices')).send({
+      invoiceIds: ['does-not-exist'],
+    });
+    expect(res.status).toBe(200);
+    expect(res.body.failures).toEqual([
+      { invoiceNumber: 'does-not-exist', reason: 'invoice not found' },
+    ]);
+  });
+
+  it('loops over every id, reporting one outcome per invoice', async () => {
+    // With SMTP unconfigured, the invoice that HAS an email still fails to
+    // send ("SMTP not configured"), but that proves the send was attempted;
+    // the no-email invoice fails for its own distinct reason. Both are
+    // reported, confirming the server iterates the whole list.
+    const res = await auth(request(app).post('/api/mail/send-invoices')).send({
+      invoiceIds: [invoiceWithEmailId, invoiceNoEmailId],
+    });
+    expect(res.status).toBe(200);
+    expect(res.body.total).toBe(2);
+    expect(res.body.sent).toBe(0);
+    expect(res.body.failed).toBe(2);
+
+    const byNumber = Object.fromEntries(
+      res.body.failures.map((f: { invoiceNumber: string; reason: string }) => [f.invoiceNumber, f.reason]),
+    );
+    expect(byNumber[INVOICE_NO_EMAIL]).toBe('no client email');
+    expect(byNumber[INVOICE_WITH_EMAIL]).toBeDefined();
+    expect(byNumber[INVOICE_WITH_EMAIL]).not.toBe('no client email');
+  });
+});

server/routes/mail.ts

@@ -7,34 +7,42 @@ import { formatDisplayDate } from '../utils/dates.js';
 
 const router = Router();
 
-// Send invoice via email
-router.post('/send-invoice', authMiddleware, async (req: AuthRequest, res: Response) => {
-  const requireRead = requirePermission('invoices', 'read');
-  await requireRead(req, res, async () => {
-    const { invoiceId, recipientEmail } = req.body;
+/** Outcome of attempting to email a single invoice. */
+interface InvoiceSendOutcome {
+  ok: boolean;
+  /** Invoice number when known, else the requested id (for the failure list). */
+  invoiceNumber: string;
+  recipientEmail?: string;
+  reason?: string;
+}
 
-    if (!invoiceId) {
-      res.status(400).json({ error: 'Invoice ID is required' });
-      return;
-    }
+/**
+ * Build and send the email for one invoice. The recipient defaults to the
+ * invoice's client email (contact_email, falling back to email); pass
+ * `recipientEmailOverride` to send elsewhere. Never throws — every failure
+ * path returns an outcome with a human-readable `reason`, so callers
+ * (single send and bulk send) can report uniformly.
+ */
+async function sendInvoiceById(
+  invoiceId: string,
+  recipientEmailOverride?: string,
+): Promise<InvoiceSendOutcome> {
+  const invoiceResult = queryOne<any>('SELECT * FROM invoices WHERE id = ?', [invoiceId]);
+  if (invoiceResult.error || !invoiceResult.data) {
+    return { ok: false, invoiceNumber: invoiceId, reason: 'invoice not found' };
+  }
 
-    if (!recipientEmail) {
-      res.status(400).json({ error: 'Recipient email is required' });
-      return;
-    }
+  const invoice = invoiceResult.data;
+  const client = invoice.client_id
+    ? queryOne<any>('SELECT * FROM clients WHERE id = ?', [invoice.client_id]).data
+    : null;
 
-    // Get invoice
-    const invoiceResult = queryOne<any>('SELECT * FROM invoices WHERE id = ?', [invoiceId]);
-    if (invoiceResult.error || !invoiceResult.data) {
-      res.status(404).json({ error: 'Invoice not found' });
-      return;
-    }
+  const recipientEmail = recipientEmailOverride || client?.contact_email || client?.email;
+  if (!recipientEmail) {
+    return { ok: false, invoiceNumber: invoice.invoice_number, reason: 'no client email' };
+  }
 
-    const invoice = invoiceResult.data;
-    const client = invoice.client_id
-      ? queryOne<any>('SELECT * FROM clients WHERE id = ?', [invoice.client_id]).data
-      : null;
-    const job = invoice.job_id
+  const job = invoice.job_id
       ? queryOne<any>('SELECT * FROM jobs WHERE id = ?', [invoice.job_id]).data
       : null;
     const tradingName = job?.trading_name_id
@@ -176,20 +184,79 @@ router.post('/send-invoice', authMiddleware, async (req: AuthRequest, res: Respo
 </div>
     `;
 
-    // Send email
-    const result = await sendInvoiceEmail(
-      recipientEmail,
-      invoice.invoice_number,
-      invoiceHtml,
-      companyDisplayName
-    );
+  // Send email
+  const result = await sendInvoiceEmail(
+    recipientEmail,
+    invoice.invoice_number,
+    invoiceHtml,
+    companyDisplayName
+  );
 
-    if (!result.success) {
-      res.status(500).json({ error: result.error || 'Failed to send email' });
+  if (!result.success) {
+    return { ok: false, invoiceNumber: invoice.invoice_number, reason: result.error || 'failed to send' };
+  }
+
+  return { ok: true, invoiceNumber: invoice.invoice_number, recipientEmail };
+}
+
+// Send a single invoice via email (recipient supplied by the caller).
+router.post('/send-invoice', authMiddleware, async (req: AuthRequest, res: Response) => {
+  const requireRead = requirePermission('invoices', 'read');
+  await requireRead(req, res, async () => {
+    const { invoiceId, recipientEmail } = req.body;
+
+    if (!invoiceId) {
+      res.status(400).json({ error: 'Invoice ID is required' });
       return;
     }
 
-    res.json({ success: true, message: `Invoice sent to ${recipientEmail}` });
+    if (!recipientEmail) {
+      res.status(400).json({ error: 'Recipient email is required' });
+      return;
+    }
+
+    const outcome = await sendInvoiceById(invoiceId, recipientEmail);
+    if (outcome.ok) {
+      res.json({ success: true, message: `Invoice sent to ${outcome.recipientEmail}` });
+      return;
+    }
+    if (outcome.reason === 'invoice not found') {
+      res.status(404).json({ error: 'Invoice not found' });
+      return;
+    }
+    res.status(500).json({ error: outcome.reason || 'Failed to send email' });
+  });
+});
+
+// Send many invoices in one request; the server loops so the client makes a
+// single round-trip and SMTP pacing stays server-side. Recipients are
+// resolved per invoice from its client. Always 200 with a per-invoice summary.
+router.post('/send-invoices', authMiddleware, async (req: AuthRequest, res: Response) => {
+  const requireRead = requirePermission('invoices', 'read');
+  await requireRead(req, res, async () => {
+    const { invoiceIds } = req.body;
+    if (
+      !Array.isArray(invoiceIds) ||
+      invoiceIds.length === 0 ||
+      invoiceIds.length > 100 ||
+      !invoiceIds.every((id) => typeof id === 'string')
+    ) {
+      res.status(400).json({ error: 'invoiceIds must be a non-empty array of at most 100 invoice ids' });
+      return;
+    }
+
+    let sent = 0;
+    const failures: { invoiceNumber: string; reason: string }[] = [];
+    for (const invoiceId of invoiceIds) {
+      const outcome = await sendInvoiceById(invoiceId);
+      if (outcome.ok) {
+        sent += 1;
+      } else {
+        failures.push({ invoiceNumber: outcome.invoiceNumber, reason: outcome.reason || 'failed to send' });
+      }
+    }
+
+    res.json({ sent, failed: failures.length, total: invoiceIds.length, failures });
   });
 });
 

src/components/ListPagination.tsx

@@ -0,0 +1,51 @@
+import { ChevronLeft, ChevronRight } from 'lucide-react';
+import { Button } from '@/components/ui/button';
+
+interface ListPaginationProps {
+  page: number;
+  totalPages: number;
+  total: number;
+  startIndex: number;
+  endIndex: number;
+  onPageChange: (page: number) => void;
+}
+
+/** Prev/next pager shown under a paginated list. Renders nothing for a single page. */
+export function ListPagination({
+  page,
+  totalPages,
+  total,
+  startIndex,
+  endIndex,
+  onPageChange,
+}: ListPaginationProps) {
+  if (totalPages <= 1) return null;
+
+  return (
+    <div className="flex items-center justify-between gap-4">
+      <p className="text-sm text-muted-foreground">
+        Showing {startIndex}–{endIndex} of {total}
+      </p>
+      <div className="flex items-center gap-2">
+        <Button
+          variant="outline"
+          size="sm"
+          onClick={() => onPageChange(page - 1)}
+          disabled={page <= 1}
+        >
+          <ChevronLeft className="h-4 w-4" />
+          <span className="hidden sm:inline">Previous</span>
+        </Button>
+        <Button
+          variant="outline"
+          size="sm"
+          onClick={() => onPageChange(page + 1)}
+          disabled={page >= totalPages}
+        >
+          <span className="hidden sm:inline">Next</span>
+          <ChevronRight className="h-4 w-4" />
+        </Button>
+      </div>
+    </div>
+  );
+}

src/hooks/usePagination.ts

@@ -0,0 +1,49 @@
+import { useState } from 'react';
+
+interface UsePaginationResult<T> {
+  /** Effective 1-based current page (clamped to the valid range). */
+  page: number;
+  setPage: (page: number) => void;
+  /** The slice of `items` belonging to the current page. */
+  pageItems: T[];
+  /** Total number of pages (minimum 1, even when `items` is empty). */
+  totalPages: number;
+  /** Total number of items across all pages. */
+  total: number;
+  /** 1-based index of the first item on the current page (0 when `total` is 0). */
+  startIndex: number;
+  /** 1-based index of the last item on the current page (0 when `total` is 0). */
+  endIndex: number;
+}
+
+/**
+ * Client-side pagination over an already-loaded array. Data stays fully
+ * loaded (so search/filter/counts remain instant over the complete set) —
+ * only the rendered slice is paginated.
+ *
+ * The current page is clamped at derive time (`Math.min(page, totalPages)`),
+ * so shrinking the input array (e.g. via search/filter) never strands the
+ * caller on a page that no longer exists — no effect needed to reset it.
+ */
+export function usePagination<T>(items: T[], pageSize = 25): UsePaginationResult<T> {
+  const [page, setPage] = useState(1);
+
+  const total = items.length;
+  const totalPages = Math.max(1, Math.ceil(total / pageSize));
+  const effectivePage = Math.min(page, totalPages);
+
+  const startIndex = total === 0 ? 0 : (effectivePage - 1) * pageSize + 1;
+  const endIndex = total === 0 ? 0 : Math.min(effectivePage * pageSize, total);
+
+  const pageItems = items.slice((effectivePage - 1) * pageSize, effectivePage * pageSize);
+
+  return {
+    page: effectivePage,
+    setPage,
+    pageItems,
+    totalPages,
+    total,
+    startIndex,
+    endIndex,
+  };
+}