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

---
phase: 12-offer-composition-drag-drop-csv-import
reviewed: 2026-06-15T10:36:00Z
depth: standard
files_reviewed: 12
files_reviewed_list:
  - 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
findings:
  critical: 0
  warning: 6
  info: 4
  total: 10
status: 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):

```ts
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:

```ts
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:

```ts
} 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)`:

```tsx
{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:

```ts
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:

```ts
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:

```ts
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:
```ts
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:**

```ts
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`:

```ts
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:**

```ts
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:**

```ts
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):**

```ts
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_