docs(14): add code review report and phase verification (passed)

14-REVIEW.md: 0 critical, 6 warnings, 3 info (standard depth, 11 files). 14-VERIFICATION.md: 5/5 success criteria verified after closing the SC5/CRM-12 generate_new dead-branch gap.
2026-06-14 15:20:08 +02:00
parent c8d53134ba
commit 2c227f4ef1
2 changed files with 465 additions and 0 deletions
@@ -0,0 +1,291 @@
+---
+phase: 14-crm-attio-style-fix
+reviewed: 2026-06-14T11:35:00Z
+depth: standard
+files_reviewed: 11
+files_reviewed_list:
+  - src/app/admin/leads/[id]/page.tsx
+  - src/app/admin/leads/LeadsSearch.tsx
+  - src/app/admin/leads/actions.ts
+  - src/app/admin/leads/page.tsx
+  - src/components/admin/dashboard/FollowUpWidget.tsx
+  - src/components/admin/leads/LeadDetail.tsx
+  - src/components/admin/leads/LeadForm.tsx
+  - src/components/admin/leads/LeadTable.tsx
+  - src/components/admin/leads/SendQuoteModal.tsx
+  - src/components/ui/form.tsx
+  - src/lib/admin-queries.ts
+status: issues_found
+---
+
+# Phase 14: Code Review Report
+
+**Reviewed:** 2026-06-14T11:35:00Z
+**Depth:** standard
+**Files Reviewed:** 11
+**Status:** issues_found
+
+## Summary
+
+Reviewed the Phase 14 CRM Attio-style redesign of `/admin/leads`: the new data layer (`getLeadsWithTags`, `getLeadFieldOptions`, `updateLeadField`, lead-tag CRUD in `actions.ts`), the rewritten `LeadTable`/`LeadsSearch`/`LeadDetail`, and the 14-03 cleanup of `FollowUpWidget`, `LeadForm`, and `SendQuoteModal`.
+
+The new inline-edit / tag CRUD layer (14-01) is generally sound and follows the established `OptionMultiSelect`/`EditableCell` patterns from Phase 11. However, there are real correctness gaps:
+
+- A stale-UI bug affects every non-inline-edit mutation path (`EditLeadModal`, `SendQuoteModal`, `LogActivityModal`) — none of them call `router.refresh()`, so `revalidatePath` alone does not update the already-rendered client tree, leaving the lead detail page showing stale data after a successful save.
+- `renameLeadTag` can throw an unhandled Postgres unique-constraint violation when the new name collides with an existing tag on the same lead, and that raw DB error message is surfaced directly to the admin UI.
+- `updateLeadField("email", ...)` bypasses the email-format validation that `createLeadSchema`/`updateLeadSchema` enforce elsewhere, so inline edits can store invalid email strings.
+- Authorization (`requireAdmin()`) was added only to the new Phase 14 actions; the pre-existing `createLead`/`updateLead`/`deleteLead`/`logActivity`/`assignQuoteToLead` actions remain unguarded, which is now an inconsistent pattern within the same file.
+- Leftover dead code from the 14-03 "remove dead onSubmit branch" fix: the `generate_new` branch in `assignQuoteToLead` (actions.ts) is now unreachable, and `SendQuoteModal`'s local duplicate `assignQuoteSchema` plus `useForm<any>` were not cleaned up to match the stated fix description.
+
+## Critical Issues
+
+None found at Critical severity — the issues above are functional/UX correctness bugs and inconsistent authorization, not exploitable from outside the admin session boundary as currently scoped. See Warnings for details and required fixes.
+
+## Warnings
+
+### WR-01: Stale UI after EditLeadModal / SendQuoteModal / LogActivityModal saves — missing router.refresh()
+
+**File:** `src/components/admin/leads/LeadForm.tsx:214-226` (EditLeadModal `onSubmit`), `src/components/admin/leads/SendQuoteModal.tsx:48-65` (`onSubmit`), `src/components/admin/leads/LogActivityModal.tsx:51-69` (`onSubmit`)
+
+**Issue:** All three modals call a server action that internally calls `revalidatePath(...)`, then on success simply `setOpen(false)` (and `form.reset()` where applicable). None of them call `router.refresh()`. `revalidatePath` invalidates the Next.js Router Cache for that path, but it does **not** force the currently-mounted client component tree to refetch — that requires an explicit `router.refresh()` (as `LeadTable`'s and `LeadDetail`'s own `run()` helpers correctly do for inline edits, see `LeadTable.tsx:126` and `LeadDetail.tsx:55`).
+
+Concretely:
+- `EditLeadModal` lets the admin change `name`, `email`, `phone`, `company`, `status`, `notes` on `/admin/leads/[id]`. After a successful save, the page header (`lead.name`, `lead.company`) and Profile card (`lead.status`, `lead.email`, etc.) keep showing the pre-edit values until the admin manually navigates or reloads.
+- `SendQuoteModal`'s "existing quote" tab calls `assignQuoteToLead`, which updates the lead's `status` to `"proposal_sent"` server-side — the Profile card's status badge will not reflect this until a manual refresh.
+- `LogActivityModal` updates `last_contact_date` via `createActivity` — the "Ultimo contatto" field and the Activity Log list will not show the new entry until a manual refresh.
+
+**Fix:**
+```tsx
+// LeadForm.tsx EditLeadModal (and CreateLeadModal), SendQuoteModal.tsx, LogActivityModal.tsx
+import { useRouter } from "next/navigation";
+
+// inside the component
+const router = useRouter();
+
+async function onSubmit(data: ...) {
+  setLoading(true);
+  try {
+    const result = await updateLead(lead.id, data); // or assignQuoteToLead / logActivity
+    if (result.success) {
+      setOpen(false);
+      router.refresh();
+    } else {
+      console.error("Error updating lead:", result.error);
+    }
+  } finally {
+    setLoading(false);
+  }
+}
+```
+
+### WR-02: renameLeadTag can throw an unhandled unique-constraint violation surfaced raw to the UI
+
+**File:** `src/app/admin/leads/actions.ts:248-260`
+
+**Issue:** `renameLeadTag(oldValue, newValue)` runs:
+```ts
+await db
+  .update(tags)
+  .set({ name: next })
+  .where(and(eq(tags.entity_type, LEADS_TAG_ENTITY), eq(tags.name, oldValue)));
+```
+This updates **every** `tags` row across **all leads** that currently have `name === oldValue` to `name = next`. The `tags` table has a unique index on `(entity_type, entity_id, name)` (`src/db/schema.ts:133-137`). If any lead that has tag `oldValue` *also already has* a tag named `next`, that row's update violates the unique constraint and Drizzle/Postgres throws.
+
+Unlike `createLead`/`updateLead`/`deleteLead`/`logActivity`/`assignQuoteToLead`, this function (and `addLeadTag`/`removeLeadTag`) has **no try/catch** — the thrown error propagates up through the `"use server"` boundary to the client's `run()` wrapper in `LeadTable.tsx`/`LeadDetail.tsx`, which does `setError(e instanceof Error ? e.message : ...)` and renders that message directly in the table row. This surfaces a raw Postgres error message (e.g. `duplicate key value violates unique constraint "tags_entity_name_unique"`) to the admin UI instead of a friendly message.
+
+**Fix:**
+```ts
+export async function renameLeadTag(oldValue: string, newValue: string) {
+  await requireAdmin();
+  const next = newValue.trim();
+  if (next.length === 0) throw new Error("Nuovo nome richiesto");
+  if (next === oldValue) return;
+
+  try {
+    await db
+      .update(tags)
+      .set({ name: next })
+      .where(and(eq(tags.entity_type, LEADS_TAG_ENTITY), eq(tags.name, oldValue)));
+  } catch (error) {
+    console.error("renameLeadTag error:", error);
+    throw new Error(`Esiste già un tag "${next}" su uno o più lead`);
+  }
+
+  revalidatePath("/admin/leads");
+}
+```
+
+### WR-03: updateLeadField("email", ...) bypasses email format validation
+
+**File:** `src/app/admin/leads/actions.ts:200-207`
+
+**Issue:** `createLeadSchema`/`updateLeadSchema` (`src/lib/lead-validators.ts:18`) validate `email` with `z.string().email(...)`. But the inline-edit path `updateLeadField(leadId, "email", value)` falls into the generic `else` branch:
+```ts
+} else {
+  // email | phone | company | next_action — nullable text fields, empty clears
+  const s = value.trim();
+  await db
+    .update(leads)
+    .set({ [fieldName]: s || null, updated_at: new Date() })
+    .where(eq(leads.id, leadId));
+}
+```
+No format check is applied, so an admin can type `"not an email"` into the inline `email` cell (`LeadTable.tsx:144-151`, `EditableCell` with `type="text"`) and it will be persisted as-is. This is inconsistent with the create/edit-modal path which rejects invalid emails via Zod.
+
+**Fix:**
+```ts
+} else if (fieldName === "email") {
+  const s = value.trim();
+  if (s && !z.string().email().safeParse(s).success) {
+    throw new Error("Email non valida");
+  }
+  await db.update(leads).set({ email: s || null, updated_at: new Date() }).where(eq(leads.id, leadId));
+} else {
+  // phone | company | next_action — nullable text fields, empty clears
+  const s = value.trim();
+  await db
+    .update(leads)
+    .set({ [fieldName]: s || null, updated_at: new Date() })
+    .where(eq(leads.id, leadId));
+}
+```
+
+### WR-04: Inconsistent authorization — requireAdmin() only guards the new Phase 14 actions
+
+**File:** `src/app/admin/leads/actions.ts:18-168` vs `170-260`
+
+**Issue:** `requireAdmin()` (lines 170-173) is called by `updateLeadField`, `addLeadTag`, `removeLeadTag`, and `renameLeadTag` (the Phase 14 additions), but **not** by the pre-existing `createLead`, `updateLead`, `deleteLead`, `logActivity`, or `assignQuoteToLead`. All of these are exported `"use server"` actions reachable by anyone who can call them directly (e.g. via a crafted request to the server action endpoint), regardless of whether they can see the `/admin/leads` UI — and `src/app/admin/layout.tsx:10-18` does **not** redirect unauthenticated users away from `/admin/*`, it only conditionally renders the sidebar. This means the new code introduces a visible double-standard: half the mutations in this file require a session, half don't, even though they operate on the same `leads` table and are reachable from the same unauthenticated page.
+
+**Fix:** Apply `await requireAdmin();` consistently at the top of every exported action in this file (or factor the check into a shared wrapper), e.g.:
+```ts
+export async function createLead(data: z.infer<typeof createLeadSchema>) {
+  await requireAdmin();
+  const parsed = createLeadSchema.safeParse(data);
+  ...
+}
+```
+Note: fixing this fully also requires the admin layout/middleware gap (no redirect for unauthenticated `/admin/*` requests) to be addressed, but that is pre-existing and out of this phase's file set — flagging here because the new code makes the inconsistency visible within a single file.
+
+### WR-05: Dead `generate_new` branch in assignQuoteToLead is now unreachable
+
+**File:** `src/app/admin/leads/actions.ts:121-132`
+
+**Issue:** The 14-03 commit message states "remove dead onSubmit branch in SendQuoteModal (CRM-12)" — and indeed `SendQuoteModal.tsx` now only calls `assignQuoteToLead({ lead_id: leadId, quote_token: data.quote_token, generate_new: false })` (hardcoded `false`, line 54 of `SendQuoteModal.tsx`), and the "Crea Nuovo" tab navigates via `window.location.href` without calling the action at all. As a result, the server-side branch:
+```ts
+if (parsed.data.generate_new) {
+  // Redirect handled on client; this is a no-op
+  return { success: true };
+}
+```
+in `assignQuoteToLead` (lines 129-132) is now dead code — `generate_new` is always `false` from the only caller. The `assignQuoteSchema` field `generate_new: z.boolean()` (line 118) and its branch should be removed for consistency with the stated cleanup.
+
+**Fix:**
+```ts
+const assignQuoteSchema = z.object({
+  lead_id: z.string().min(1),
+  quote_token: z.string().optional(),
+});
+
+export async function assignQuoteToLead(data: z.infer<typeof assignQuoteSchema>) {
+  const parsed = assignQuoteSchema.safeParse(data);
+  if (!parsed.success) {
+    return { success: false, error: parsed.error.issues[0].message };
+  }
+  try {
+    const token = parsed.data.quote_token;
+    const leadId = parsed.data.lead_id;
+    if (!token) {
+      return { success: false, error: "Token preventivo richiesto" };
+    }
+    // ... rest unchanged, drop the generate_new check entirely
+  }
+}
+```
+And update `SendQuoteModal.tsx`'s call site / local schema to match (see WR-06).
+
+### WR-06: SendQuoteModal duplicates the assignQuoteSchema with `useForm<any>`, diverging from the 14-03 typing fix
+
+**File:** `src/components/admin/leads/SendQuoteModal.tsx:28-32, 39, 48`
+
+**Issue:** The 14-03 commit `ee509cd fix(14-03): type LeadForm useForm with CreateLeadInput (CRM-11)` typed `LeadForm.tsx`'s `useForm` hooks with `CreateLeadInput` instead of `any`. `SendQuoteModal.tsx` was not updated to match: it still uses `useForm<any>(...)` (line 39) and `onSubmit(data: any)` (line 48), and additionally defines its **own copy** of `assignQuoteSchema` (lines 28-32) that is structurally similar to but independent from the one in `actions.ts` (lines 115-119) — including a `generate_new: z.boolean().default(false)` field that, per WR-05, should be removed server-side. If the two schemas drift (e.g., someone adds a field to one but not the other), `zodResolver` validation on the client and `safeParse` on the server will disagree silently.
+
+**Fix:**
+- Replace `useForm<any>` with a proper type, e.g. infer from a schema that only models what this form actually needs:
+```ts
+const sendQuoteFormSchema = z.object({
+  quote_token: z.string().min(1, "Token richiesto"),
+});
+type SendQuoteFormInput = z.infer<typeof sendQuoteFormSchema>;
+
+const form = useForm<SendQuoteFormInput>({
+  resolver: zodResolver(sendQuoteFormSchema),
+  defaultValues: { quote_token: "" },
+});
+
+async function onSubmit(data: SendQuoteFormInput) {
+  ...
+  const result = await assignQuoteToLead({ lead_id: leadId, quote_token: data.quote_token });
+  ...
+}
+```
+- Remove the local `assignQuoteSchema` duplicate and the unused `lead_id`/`generate_new` fields from the client-side schema — `lead_id` is already known from props and should not be a form field.
+
+## Info
+
+### IN-01: Unused `Button` import in LeadDetail.tsx
+
+**File:** `src/components/admin/leads/LeadDetail.tsx:8`
+
+**Issue:** `import { Button } from "@/components/ui/button";` is imported but never referenced anywhere in the file — all action buttons are rendered inside `LogActivityModal`, `SendQuoteModal`, and `EditLeadModal`, which manage their own `<Button>` triggers.
+
+**Fix:** Remove the unused import:
+```tsx
+// remove this line
+import { Button } from "@/components/ui/button";
+```
+
+### IN-02: StatusCell dispatches an update even when selecting the already-active status
+
+**File:** `src/components/admin/leads/LeadTable.tsx:82-102`
+
+**Issue:** Unlike `EditableCell.commit()` (which has an explicit "skip redundant server action when nothing changed" guard, see `editable-cell.tsx:49-54`), `StatusCell`'s dropdown `onClick` handler calls `run(() => updateLeadField(lead.id, "status", stage))` unconditionally — including when `stage === lead.status`. This triggers an unnecessary `UPDATE`, `revalidatePath`, and `router.refresh()` for a true no-op.
+
+**Fix:**
+```tsx
+onClick={() => {
+  setIsOpen(false);
+  if (stage === lead.status) return;
+  run(() => updateLeadField(lead.id, "status", stage));
+}}
+```
+
+### IN-03: `LeadDetail` prop typed as `Lead` but actually receives `LeadWithTags`
+
+**File:** `src/components/admin/leads/LeadDetail.tsx:40` vs `src/app/admin/leads/[id]/page.tsx:24-30`
+
+**Issue:** `page.tsx` passes `lead={lead}` where `lead` is a `LeadWithTags` (i.e., `Lead & { tags: string[] }`, from `getLeadsWithTags()`), but `LeadDetail`'s prop type declares `lead: Lead`. This happens to type-check because `LeadWithTags` is a structural superset of `Lead`, but it means the component's declared type doesn't reflect what it actually receives (it separately receives `tags: string[]` as its own prop, duplicating data already present on `lead.tags`).
+
+**Fix:** Either type the prop as `LeadWithTags` and drop the separate `tags` prop (reading `lead.tags` directly), or keep `Lead` and have `page.tsx` strip `tags` before passing — for clarity, prefer:
+```ts
+import type { LeadWithTags } from "@/lib/admin-queries";
+
+export function LeadDetail({
+  lead,
+  activities,
+  reminders,
+  tagOptions,
+}: {
+  lead: LeadWithTags;
+  activities: Activity[];
+  reminders: Reminder[];
+  tagOptions: string[];
+}) {
+  // use lead.tags instead of a separate `tags` prop
+}
+```
+
+---
+
+_Reviewed: 2026-06-14T11:35:00Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_