From 5760ef99662969e2d1ca1763872c29ac99b03ce0 Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 27 Aug 2026 01:00:43 +0200 Subject: [PATCH] fix(approvals): address Task 2 code quality findings + test mock update Four code-quality issues from review + one test-mock follow-up: 1. CRITICAL: isPlaceholderCustomer was called with a string arg but expects { id, customer, email } - TS error, and at runtime the function would always return true. Variable was dead (computed but never read) so remove the call, the explanatory comment, and the import entirely. The Option A behaviour (server handles placeholder via its 422) is preserved by simply not having the dead variable. 2. CRITICAL: toast(..., { type: 'info' }) doesn't compile under ExternalToast and silently drops the type option at runtime, so the invalid_status race toast renders with default styling instead of info (blue). Switch to toast.info(...). Updates the test assertion to check toast.info directly (the test mock defines toast.info as an independent vi.fn(), not a wrapper around toast, so the prior expect(toast).toHaveBeenCalledWith assertion would no longer match). 3. IMPORTANT: Spec asks for a dedicated 404 branch ('This quote no longer exists. Refreshing.') but the implementation fell through to the generic catch-all. Add the 404 branch between the mail-fail branch and the generic fallback. 4. NICE-TO-HAVE: formatCurrency was imported but never used. Drop. Co-Authored-By: Claude --- src/app/(app)/quotes/approvals/page.test.tsx | 5 ++--- src/app/(app)/quotes/approvals/page.tsx | 19 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/app/(app)/quotes/approvals/page.test.tsx b/src/app/(app)/quotes/approvals/page.test.tsx index 768b413..6858a9c 100644 --- a/src/app/(app)/quotes/approvals/page.test.tsx +++ b/src/app/(app)/quotes/approvals/page.test.tsx @@ -213,9 +213,8 @@ describe("ApprovalsPage — Approve & send from list", () => { }); const { toast } = await import("sonner"); await waitFor(() => { - expect(toast).toHaveBeenCalledWith( - expect.stringContaining("already actioned"), - expect.anything() + expect(toast.info).toHaveBeenCalledWith( + expect.stringContaining("already actioned") ); }); }); diff --git a/src/app/(app)/quotes/approvals/page.tsx b/src/app/(app)/quotes/approvals/page.tsx index f2866bd..d0d578b 100644 --- a/src/app/(app)/quotes/approvals/page.tsx +++ b/src/app/(app)/quotes/approvals/page.tsx @@ -8,14 +8,13 @@ import { Button } from "@/components/ui/button"; import { Textarea } from "@/components/ui/textarea"; import { toast } from "sonner"; import getPocketBase from "@/lib/pocketbase"; -import { formatDate, formatCurrency } from "@/lib/pb-helpers"; +import { formatDate } from "@/lib/pb-helpers"; import { quoteStatusClassName, formatQuoteStatus } from "@/lib/quote-status"; import { ArrowLeft, CheckCircle, XCircle, Loader2, FileText, ListChecks } from "lucide-react"; import { getClientIdentity } from "@/lib/dev-identity"; import { cn } from "@/lib/utils"; import type { Quote, QuoteItem, QuoteAttachment } from "@/lib/types"; import { SendQuoteModal } from "@/components/quotes/send-quote-modal"; -import { isPlaceholderCustomer } from "@/lib/placeholder-customers"; /** * /quotes/approvals — queue of quotes awaiting the rep's eyes. @@ -208,7 +207,7 @@ export default function ApprovalsPage() { // 3) Anything else (placeholder customer, validation, 5xx) — // generic error toast. if (body.code === "invalid_status") { - toast("This quote was already actioned — refreshing.", { type: "info" }); + toast.info("This quote was already actioned — refreshing."); router.refresh(); setSendModalQuoteId(null); return; @@ -221,6 +220,12 @@ export default function ApprovalsPage() { router.refresh(); return; } + if (res.status === 404) { + toast.info("This quote no longer exists. Refreshing."); + setSendModalQuoteId(null); + router.refresh(); + return; + } toast.error(body.error ?? `Approve failed (HTTP ${res.status})`); return; } @@ -543,17 +548,9 @@ export default function ApprovalsPage() { )} - {/* Placeholder-customer UX (disable submit) is delegated to the - server's 422 response. The detail page passes a customerIsPlaceholder - prop to SendQuoteModal; the list page deliberately doesn't bother — - placeholder customers shouldn't reach the approval queue in normal - flow, and the existing 422 handler shows the failure cleanly. */} {(() => { const openQuote = quotes.find((q) => q.id === sendModalQuoteId); if (!openQuote) return null; - const customerIsPlaceholder = isPlaceholderCustomer( - openQuote.expand?.customer?.customer ?? "" - ); return (