--- 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 && ( )} ``` ### 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: `

{card.internal_name}

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