diff --git a/.planning/phases/14-crm-attio-style-fix/14-REVIEW.md b/.planning/phases/14-crm-attio-style-fix/14-REVIEW.md new file mode 100644 index 0000000..1d9d746 --- /dev/null +++ b/.planning/phases/14-crm-attio-style-fix/14-REVIEW.md @@ -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` 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) { + 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) { + 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`, 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(...)` (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` 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; + +const form = useForm({ + 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 `