clienthub/.planning/phases/12-offer-composition-drag-drop-csv-import/12-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
12-offer-composition-drag-drop-csv-import	2026-06-15T10:36:00Z	standard	12	scripts/push-12-offer-tier-schema.ts scripts/verify-12-03-actions.ts scripts/verify-12-03-queries.ts src/app/admin/offers/[id]/edit/page.tsx src/app/admin/offers/actions.ts src/app/admin/offers/page.tsx src/components/admin/offers/OfferEditorClient.tsx src/components/admin/offers/OfferListClient.tsx src/db/migrations/0008_offer_tier_schema.sql src/db/migrations/meta/_journal.json src/db/schema.ts src/lib/offer-queries.ts	critical warning info total 0 6 4 10	issues_found

Phase 12: Code Review Report

Reviewed: 2026-06-15T10:36:00Z Depth: standard Files Reviewed: 12 Status: issues_found

Summary

This batch implements the Phase 12 Offer Editor: an additive DB migration (tier designation, public pricing, archive flag, category/ticket dimensions, tipo/obiettivo tags), a query layer (offer-queries.ts), server actions (saveOfferEditor/toggleOfferArchived/addOfferTag/removeOfferTag/ renameOfferOption/createOfferMacro), and the editor/list client components.

The migration and push script are correctly additive/idempotent and respect the Data Safety constraints in CLAUDE.md (no drops/truncates, ADD COLUMN IF NOT EXISTS, guarded DO $$ block for the CHECK constraint). requireAdmin() is consistently called at the top of every new server action.

No critical/security issues were found. The main concerns are: (1) the tier upsert in saveOfferEditor doesn't enforce uniqueness of tier_letter within a single macro, which can desync the UI's padTiers() assumption and silently orphan rows; (2) renameOfferOption("tipo"/"obiettivo", ...) can throw an uncaught unique-constraint violation when the target name already exists, surfacing as a generic 500 with no user-facing message; (3) the "unarchive" path (toggleOfferArchived(macroId, false)) is implemented server-side and covered by the verify script but has no reachable UI entry point, making it effectively dead code from the user's perspective; and (4) several smaller robustness/UX gaps around empty tiers being persisted with placeholder names, and total mismatches when an assigned service falls outside the category filter.

Warnings

WR-01: saveOfferEditor does not enforce unique tier_letter per macro

File: src/app/admin/offers/actions.ts:152-238 Issue: tierSchema validates each tier's tier_letter is one of "A"|"B"|"C" and saveOfferEditorSchema.tiers is capped at .max(3), but nothing prevents two tiers in the same payload from sharing the same tier_letter (e.g., two tiers both "A"). The loop at lines 200-238 will happily update/insert both rows independently — both ending up in offer_micros with macro_id = macroId and tier_letter = "A".

On the next load, getOfferEditorData (src/lib/offer-queries.ts:163-167) orders by tierOrder (A→B→C, ties broken arbitrarily/by insertion), and OfferEditorClient.padTiers() (src/components/admin/offers/OfferEditorClient.tsx:33-37) uses tiers.find((t) => t.tier_letter === letter) — which returns only the FIRST matching tier. The second "A" tier becomes invisible in the UI but still exists in the DB (with its own offer_tier_services rows), and a subsequent save will silently drop it from the tiers array sent to saveOfferEditor (since OfferEditorClient state only ever holds 3 padded tiers) — leaving an orphaned offer_micros row that's never cleaned up.

Fix: Either enforce uniqueness in saveOfferEditorSchema via a .refine() check, or — more robustly — have saveOfferEditor delete any existing tiers for macroId whose tier_letter is NOT present in the incoming payload (so stale/ duplicate rows get cleaned up):

const saveOfferEditorSchema = z.object({
  // ...
  tiers: z.array(tierSchema).max(3).refine(
    (tiers) => {
      const letters = tiers.map((t) => t.tier_letter);
      return new Set(letters).size === letters.length;
    },
    { message: "Ogni tier deve avere una lettera univoca (A/B/C)" }
  ),
  // ...
});

WR-02: renameOfferOption("tipo"/"obiettivo", ...) can throw on unique constraint violation

File: src/app/admin/offers/actions.ts:337-341 Issue: For field === "tipo" | "obiettivo", the action runs:

await db
  .update(tags)
  .set({ name: next })
  .where(and(eq(tags.entity_type, OFFER_TAG_ENTITY[field]), eq(tags.name, oldValue)));

This is a global rename across ALL macros that have a tag named oldValue for that entity_type. The tags table has a unique index tags_entity_name_unique on (entity_type, entity_id, name) (src/db/schema.ts:133-137). If any macro already has a tag named next for the same entity_type (e.g., renaming "Audit" → "Coaching" but macro X already has a "Coaching" tag), the UPDATE on macro X's row will violate the unique index and throw a raw Postgres error. This error is not caught here, and propagates up to OfferEditorClient.handleRenameTipo/handleRenameObiettivo (src/components/admin/offers/OfferEditorClient.tsx:210-232), which catch it generically and show "Errore nel salvataggio. Verifica i campi." — but by that point the optimistic client-side state update (setTipoOptions, setTipoTags) has ALREADY applied for ALL macros' local view, while the DB only partially applied the rename (some rows renamed, the colliding row left as oldValue or the whole statement rolled back depending on transaction semantics — Drizzle here issues a single statement so it's atomic per-statement, but the local UI state for THIS macro is now inconsistent with the DB: tipoTags shows next even though the DB row may not have been updated due to the throw on a different row update within the same UPDATE statement — actually since it's one UPDATE affecting multiple rows, a constraint violation on any row aborts the whole statement, meaning NO rows are renamed in the DB, but the UI already shows the renamed tag).

Fix: Wrap in onConflictDoNothing-style handling isn't directly possible for UPDATE; instead, pre-check or catch the specific error and surface a meaningful message:

} else if (field === "tipo" || field === "obiettivo") {
  try {
    await db
      .update(tags)
      .set({ name: next })
      .where(and(eq(tags.entity_type, OFFER_TAG_ENTITY[field]), eq(tags.name, oldValue)));
  } catch (err) {
    throw new Error(`Esiste già un tag "${next}" per questo campo`);
  }
}

WR-03: toggleOfferArchived(macroId, false) has no reachable UI entry point

File: src/components/admin/offers/OfferEditorClient.tsx:174-184, 487-496 Issue: handleArchive only ever calls toggleOfferArchived(data.macro.id, true) (line 178), and the "Archivia" button is only rendered when !initialArchived (line 487). There is no button/action that calls toggleOfferArchived(macroId, false) to un-archive an offer. Once an offer is archived, the only way back is presumably a direct DB edit — OfferListClient's "Mostra offerte archiviate" toggle (line 116-124 in OfferListClient.tsx) lets the admin SEE archived offers and click through to /admin/offers/[id]/edit, but the edit page gives them no way to restore it. toggleOfferArchived(macroId, false) is exercised in scripts/verify-12-03-actions.ts (Test 6) but is effectively dead code from a product standpoint.

Fix: Add a "Ripristina" (restore) button shown when initialArchived is true, calling toggleOfferArchived(data.macro.id, false):

{initialArchived && (
  <button
    type="button"
    onClick={() => startTransition(async () => {
      try {
        await toggleOfferArchived(data.macro.id, false);
        router.push("/admin/offers");
      } catch {
        setSaveError("Errore nel salvataggio. Verifica i campi.");
      }
    })}
    disabled={isPending}
    className="text-[#1A463C] text-sm font-semibold px-6 py-4 rounded hover:bg-[#f0f7f4] transition-colors duration-150 disabled:opacity-50"
  >
    Ripristina
  </button>
)}

WR-04: Empty/unused tier slots are persisted as real offer_micros rows with placeholder names

File: src/components/admin/offers/OfferEditorClient.tsx:138-172, src/app/admin/offers/actions.ts:200-238 Issue: padTiers() always produces exactly 3 OfferTierData entries (A/B/C), filling missing ones with emptyTier(letter) (id: "", empty names, assignedServiceIds: []). handleSave (lines 151-159) maps ALL 3 tiers unconditionally into the payload, defaulting internal_name to `Tier ${tier.tier_letter}` and public_name to t.tier_letter || "Tier" when empty. saveOfferEditor's tier loop (lines 200-238) then inserts a NEW offer_micros row for every tier without an id — including these placeholder "empty" tiers — as long as canSave is true (i.e., AT LEAST ONE of the 3 tiers has assigned services).

Concretely: if the admin only fills in Tier A with services and leaves B/C empty, saving creates 3 offer_micros rows total — 2 of which are "ghost" tiers named "Tier B"/"Tier C" with duration_months: 1, public_price: null, and zero assigned services — visible in any other view that lists offer_micros for this macro (e.g., getCatalogWithMicros, used elsewhere in the admin).

Fix: Skip tiers that are both unsaved (!t.id) AND empty (assignedServiceIds.length === 0 and no internal_name/public_name entered) when building the payload:

tiers: tiers
  .filter((t) => t.id || t.assignedServiceIds.length > 0 || t.internal_name || t.public_name)
  .map((t) => ({ ... })),

WR-05: tierTotals silently treats services outside the category filter as €0, diverging from server-computed servicesTotal

File: src/components/admin/offers/OfferEditorClient.tsx:96-109 Issue: filteredServices (lines 96-100) is data.availableServices filtered by macro.category (client-side, reactive to the in-progress edit of macro.category). tierTotals (lines 102-109) computes each tier's total by looking up tier.assignedServiceIds against filteredServices only, defaulting to 0 via Number(service?.unit_price ?? 0) for any assigned service ID not found in filteredServices.

If a tier has services assigned that belong to a DIFFERENT category than the macro's current category (e.g., the macro's category was just changed in this editing session, or a service's category changed since assignment), those services drop out of filteredServices and their price silently becomes €0 in the "Totale Servizi" row — even though data.tiers[i].servicesTotal (computed server-side in getOfferEditorData, src/lib/offer-queries.ts:183-193, which does NOT filter by category) would include them. This produces a displayed total that's lower than what's stored/will be recomputed after save, with no indication to the admin why.

Fix: Compute tierTotals against data.availableServices (unfiltered) or a combined lookup map that includes all services ever referenced by assignedServiceIds, not just the category-filtered subset:

const serviceById = useMemo(
  () => new Map(data.availableServices.map((s) => [s.id, s])),
  [data.availableServices]
);
const tierTotals = useMemo(() => {
  return tiers.map((tier) =>
    tier.assignedServiceIds.reduce((sum, id) => {
      const service = serviceById.get(id);
      return sum + Number(service?.unit_price ?? 0);
    }, 0)
  );
}, [tiers, serviceById]);

(Note: this still won't be 100% accurate if a service belongs to a category outside data.availableServices entirely — but that's the same best-effort boundary the server-side query already has.)

WR-06: createOfferMacro allows whitespace-only internal_name

File: src/app/admin/offers/actions.ts:351-376 Issue: createOfferMacroSchema.internal_name is z.string().min(1, "Nome interno richiesto"). Zod's .min(1) checks string LENGTH, not trimmed content — a value of " " (single space) passes validation. Line 370 then does parsed.data.public_name?.trim() || parsed.data.internal_name — if public_name is empty/whitespace, public_name ends up being the untrimmed " " as well. The result is a new offer_macros row with internal_name = " " and public_name = " ", which will render as a blank card in OfferListClient (line 142: <h3>{card.internal_name}</h3> shows nothing visible) with no way to identify or rename it cleanly from the list view (the only EditableCell for internal_name is on the edit page, line 246-251, which itself has required but the same tempValue.trim().length === 0 check in EditableCell.commit — so once created, it actually CAN be fixed, but the initial blank card is confusing).

Fix: Trim and re-validate in the Zod schema:

const createOfferMacroSchema = z.object({
  internal_name: z.string().trim().min(1, "Nome interno richiesto"),
  public_name: z.string().optional(),
  description: z.string().optional(),
  category: z.string().optional(),
});

and at line 370, trim internal_name too:

public_name: parsed.data.public_name?.trim() || parsed.data.internal_name.trim(),

Info

IN-01: OfferListClient.handleCreateSubmit always calls router.refresh() even when createOfferMacro throws

File: src/components/admin/offers/OfferListClient.tsx:34-41 Issue:

function handleCreateSubmit(formData: FormData) {
  startTransition(async () => {
    await createOfferMacro(formData);
    setShowCreateForm(false);
    setNewOfferName("");
    router.refresh();
  });
}

createOfferMacro can throw (e.g., Zod validation failure if internal_name is empty — though the required attribute on the <Input> at line 59 mitigates this for normal browser usage). If it throws, the await rejects, and since there's no try/catch, the rejection becomes an unhandled promise rejection inside startTransition's async callback — setShowCreateForm(false), setNewOfferName(""), and router.refresh() never run, but there's also no saveError-style state to inform the user anything went wrong. The form stays open with stale state and no feedback.

Fix: Wrap in try/catch and surface an error state, mirroring the pattern used in OfferEditorClient:

const [createError, setCreateError] = useState<string | null>(null);

function handleCreateSubmit(formData: FormData) {
  setCreateError(null);
  startTransition(async () => {
    try {
      await createOfferMacro(formData);
      setShowCreateForm(false);
      setNewOfferName("");
      router.refresh();
    } catch {
      setCreateError("Errore nella creazione dell'offerta.");
    }
  });
}

IN-02: requireAdmin() discards the session without using it for ownership/role checks

File: src/app/admin/offers/actions.ts:18-21 Issue:

async function requireAdmin() {
  const session = await getServerSession(authOptions);
  if (!session) throw new Error("Non autorizzato");
}

This is consistent with the existing pattern in this file (pre-existing, not introduced by this phase), so it's not a regression — but worth flagging as a pre-existing gap: any authenticated session is treated as "admin" with no role check. Since this is a single-admin app per CLAUDE.md's architecture, this is likely fine, but the function name requireAdmin implies a role check that doesn't exist. Purely informational — no change required unless multi-user admin roles are planned.

Fix: None required for this phase; consider renaming to requireSession for clarity in a future cleanup, or add a role check if multi-admin support is ever introduced.

IN-03: formatUnitPrice and formatEuro duplicate locale-formatting logic

File: src/components/admin/offers/OfferEditorClient.tsx:39-46 Issue:

function formatEuro(value: number): string {
  return `€${value.toLocaleString("it-IT", { minimumFractionDigits: 2, maximumFractionDigits: 2 })}`;
}

function formatUnitPrice(raw: string): string {
  const num = parseFloat(raw);
  return `€${(isNaN(num) ? 0 : num).toLocaleString("it-IT", { minimumFractionDigits: 2, maximumFractionDigits: 2 })}`;
}

Both functions produce identical output format; formatUnitPrice is just formatEuro(parseFloat(raw) || 0). Minor duplication — not a bug, but could be collapsed into one helper that accepts string | number.

Fix (optional):

function formatEuro(value: number | string): string {
  const num = typeof value === "string" ? parseFloat(value) : value;
  return `€${(isNaN(num) ? 0 : num).toLocaleString("it-IT", { minimumFractionDigits: 2, maximumFractionDigits: 2 })}`;
}

IN-04: push-12-offer-tier-schema.ts and 0008_offer_tier_schema.sql can drift independently

File: scripts/push-12-offer-tier-schema.ts:1-143, src/db/migrations/0008_offer_tier_schema.sql:1-45 Issue: The push script (scripts/push-12-offer-tier-schema.ts) is a hand-written duplicate of the SQL migration file's statements (per the comment at the top of 0008_offer_tier_schema.sql, this is because drizzle-kit generate is broken per project memory). Both files express the same schema change in two different forms (column lists in a TS array vs. raw ALTER TABLE statements). This is a deliberate, documented workaround, but creates a maintenance hazard: if either file is edited in isolation in a future phase, the two can silently diverge (e.g., a column added to the .sql migration but not to the push script's offerMacrosColumns/offerMicrosColumns arrays, or vice versa).

Fix (optional): Add a comment cross-reference in both files pointing at each other ("keep in sync with X"), or — longer-term — generate the push script's column lists programmatically by parsing the .sql file, to guarantee they can't diverge. Not blocking for this phase given the documented one-off nature.

---

Reviewed: 2026-06-15T10:36:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard