clienthub/.planning/phases/14-crm-attio-style-fix/14-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	status
14-crm-attio-style-fix	2026-06-14T11:35:00Z	standard	11	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	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:

// 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:

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:

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:

} 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:

} 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.:

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:

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:

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:

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:

// 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:

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:

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