diff --git a/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-REVIEW.md b/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-REVIEW.md new file mode 100644 index 0000000..8e0527b --- /dev/null +++ b/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-REVIEW.md @@ -0,0 +1,131 @@ +--- +phase: 11-catalog-database-view-ux-legacy-consolidation +reviewed: 2026-06-13T14:10:00Z +depth: standard +files_reviewed: 12 +files_reviewed_list: + - src/db/schema.ts + - src/db/migrations/0006_add_tags_table.sql + - scripts/push-11-tags-migration.ts + - scripts/migrate-tags.ts + - scripts/validate-tags-migration.ts + - src/lib/admin-queries.ts + - src/app/admin/catalog/actions.ts + - src/components/ui/editable-cell.tsx + - src/components/ui/tag-multi-select.tsx + - src/components/admin/catalog/ServiceTable.tsx + - src/app/admin/catalog/CatalogSearch.tsx + - src/app/admin/catalog/page.tsx +findings: + critical: 0 + warning: 4 + info: 5 + total: 9 +status: issues_found +--- + +# Phase 11: Code Review Report + +**Reviewed:** 2026-06-13T14:10:00Z +**Depth:** standard +**Files Reviewed:** 12 +**Status:** issues_found + +## Summary + +Reviewed the Phase 11 catalog database-view changes: the new polymorphic `tags` table, its additive migration + scripts, the `getAllServices` tag join, four new server actions, and the `EditableCell` / `TagMultiSelect` / `ServiceTable` / `CatalogSearch` UI. + +Security posture is solid. All four new server actions (`updateServiceField`, `addTagToService`, `removeTagFromService`, `quickAddService`) call `requireAdmin()` before any DB access, consistent with the established single-admin session model in `src/lib/auth.ts`. The polymorphic `entity_type` is hardcoded to the literal `"services"` at every call site (action, query, migration scripts) — there is no path for a caller to control it, so the polymorphic junction cannot be abused to read/write another entity's tags. All queries go through Drizzle's parameterized builder, so there is no SQL injection surface. No LOCKED constraint is touched: `quote_items` are not exposed, `clients.token` is untouched, and the migration is strictly additive. + +The migration (`0006_add_tags_table.sql`) and the production push script (`push-11-tags-migration.ts`) contain **no** `DROP`, `TRUNCATE`, `DELETE`, or `ALTER ... DROP` — they only `CREATE TABLE IF NOT EXISTS` + two `CREATE INDEX IF NOT EXISTS`. This satisfies the Data Safety LOCKED requirement. + +The `getAllServices` tag aggregation is correct: the `entity_type = "services"` scope is in the `leftJoin` ON-clause (so services with zero tags are preserved), null `tag_name` rows are guarded before being pushed, and the `serviceMap` collapses the row-multiplication from the join. No null leakage, no row duplication. + +The defects below are correctness/robustness issues in the inline-edit UI (no Criticals). The most important is a double server-action invocation in the `EditableCell` toggle path (WR-01). + +## Warnings + +### WR-01: EditableCell toggle can fire `onSave` twice (double server action) + +**File:** `src/components/ui/editable-cell.tsx:112-126` +**Issue:** In the `toggle` branch, `onChange` calls `onSave(next)` and then `setIsEditing(false)`. The same element also has `onBlur={commit}`, and `commit()` calls `onSave(tempValue)` again. When the user clicks the checkbox, the change handler fires (saving + closing), and the ensuing blur — which can fire before React unmounts the input — calls `commit()`, issuing a second `updateServiceField` for the same field. At best this is a redundant write + extra `revalidatePath`; combined with the async `tempValue` state update inside `onChange`, the second call may even persist a stale value. The toggle should not have a blur-commit, and it should not double-handle save. +**Fix:** +```tsx +} : type === "toggle" ? ( + } + type="checkbox" + checked={tempValue === "true"} + onChange={(e) => { + const next = e.target.checked ? "true" : "false"; + setTempValue(next); + onSave(next); + setIsEditing(false); + }} + // remove onBlur={commit} — onChange already saves+closes + onKeyDown={(e) => { if (e.key === "Escape") cancel(); }} + className="h-4 w-4 cursor-pointer accent-[#1A463C]" + /> +) +``` + +### WR-02: `onBlur={commit}` re-saves even when nothing changed and on cancel-via-blur + +**File:** `src/components/ui/editable-cell.tsx:107,134` (text/number/textarea inputs) +**Issue:** Every text/number/textarea input commits on blur. Two problems: (1) clicking into a cell and clicking away without editing still triggers `onSave` with the unchanged value, generating a needless server action + `revalidatePath` + `router.refresh()` per cell touched; (2) there is no dirty check, so opening many cells while navigating with the mouse causes a burst of writes. Add an equality guard so `commit()` is a no-op when `tempValue === String(value)`. +**Fix:** +```tsx +function commit() { + if (tempValue === String(value)) { setIsEditing(false); return; } + if (required && tempValue.trim().length === 0) { setError("Campo richiesto"); return; } + setError(null); + onSave(tempValue); + setIsEditing(false); +} +``` + +### WR-03: Required-field commit failure leaves the field unsaved with no recovery on blur + +**File:** `src/components/ui/editable-cell.tsx:48-56,107` +**Issue:** For a `required` field (name), if the user clears it and blurs, `commit()` sets the "Campo richiesto" error and returns *without* leaving edit mode — but blur has already moved focus away, so the input is now unfocused while still mounted, and the cell is stuck in an editing state the user may not notice. There is no auto-revert to the previous valid value. Either revert to `value` on a failed required commit, or block blur from committing an invalid required field (keep focus). Recommend reverting on blur for required fields so the row never persists an empty name and the UI never gets stuck. +**Fix:** On a failed required validation triggered by blur, call `cancel()` (revert to last valid `value`) instead of leaving the input in a half-edited error state; keep the inline error only for the Enter-key path. + +### WR-04: `unit_price` inline edit accepts locale-formatted input and silently truncates + +**File:** `src/components/ui/editable-cell.tsx:130-139` and `src/app/admin/catalog/actions.ts:95-98` +**Issue:** The price is displayed via `formatPrice` using `it-IT` locale (`€1.234,50`), but when editing, the input shows the raw value and `updateServiceField` parses with `parseFloat(String(value))`. If an admin types an Italian-formatted number such as `1.234,50` (which matches what they just saw), `parseFloat` returns `1.234` — silently storing the wrong price with no validation error. The number `` mitigates this in browsers that enforce numeric mode, but `parseFloat` will still accept `"1.234"`-style strings and the mismatch between display locale and parse locale is a data-integrity trap. Normalize the input (strip thousands separators, convert decimal comma) before `parseFloat`, or reject values that don't match a strict numeric regex. +**Fix:** In the action, validate strictly before parsing, e.g. `const raw = String(value).replace(/\./g, "").replace(",", "."); const num = Number(raw); if (!Number.isFinite(num) || num < 0) throw new Error("Prezzo invalido");` — and/or feed the input the raw unformatted decimal so display and parse agree. + +## Info + +### IN-01: Invalid table markup — error `` injected as a 7th cell in a 6-column row + +**File:** `src/components/admin/catalog/ServiceTable.tsx:88-92` +**Issue:** The error cell is rendered as a sibling `` inside the same `` that already contains the 6 data cells, producing a 7-column row (6 + a colSpan-6 cell). This breaks column alignment and is invalid table structure; React/browsers will render it but it will visually overflow or shift the row. Render the error in a separate full-width `` below the data row, or in a dedicated status area. +**Fix:** Move the error into its own row: `{error && {error}}` rendered after the data ``. + +### IN-02: Search omits description and category despite placeholder implying broader scope + +**File:** `src/app/admin/catalog/CatalogSearch.tsx:15-18` +**Issue:** Filtering matches only `name` and `tags`. The placeholder ("Cerca per nome o tag...") is honest, but operators will likely expect category/description matches in a database-style view. Low impact; flagging as a UX gap, not a bug. Consider also matching `category`. + +### IN-03: `getTagColorIndex` hash collisions are acceptable but worth a note + +**File:** `src/components/ui/tag-multi-select.tsx:23-30` +**Issue:** With only 7 palette buckets, distinct tag names frequently share a color (expected by D-07). Not a defect — just confirming this is by design; no fix required. The `hash |= 0` + `Math.abs` correctly avoids negative modulo. + +### IN-04: `migrate-tags.ts` SELECT-then-INSERT instead of `onConflictDoNothing` + +**File:** `scripts/migrate-tags.ts:17-39` +**Issue:** The one-shot migration checks existence with a SELECT then inserts, rather than relying on the `tags_entity_name_unique` index via `.onConflictDoNothing()` (as the runtime `addTagToService` action does). For a single-run script this is fine and idempotent, but it is inconsistent with the production action and does one extra round-trip per row. Optional: use `.onConflictDoNothing()` to match the action and simplify the loop. + +### IN-05: `EditableCell` toggle ignores `formatDisplay` / lacks keyboard toggle affordance + +**File:** `src/components/ui/editable-cell.tsx:76-77,112-126` +**Issue:** The toggle's display string is hardcoded ("✓ Attivo" / "✗ Disattivato") and the only way to flip it is a click that opens a raw checkbox. Minor: per D-14 the intent is a single-click inline toggle; the current two-step (click cell → click checkbox) adds an extra interaction. Consider toggling directly on cell click for the boolean type. Cosmetic / UX only. + +--- + +_Reviewed: 2026-06-13T14:10:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_ diff --git a/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-VERIFICATION.md b/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-VERIFICATION.md new file mode 100644 index 0000000..20c1780 --- /dev/null +++ b/.planning/phases/11-catalog-database-view-ux-legacy-consolidation/11-VERIFICATION.md @@ -0,0 +1,92 @@ +--- +status: human_needed +phase: 11-catalog-database-view-ux-legacy-consolidation +verified: 2026-06-13 +verifier: orchestrator-inline +reason: verifier-agent socket failure; code verified directly by orchestrator. DB-data must_haves blocked by firewall (user-owned manual migration). +requirements_code_verified: [OFFER-07, OFFER-08, OFFER-09, OFFER-10] +requirements_pending_db: [OFFER-13] +--- + +# Phase 11 Verification — Catalog Database-View UX & Legacy Consolidation + +## Goal + +Deliver a Notion/Airtable-style catalog database-view UX (inline-edit cells, tags, +instant search, quick-add, active/inactive split) on the unified `services` table, +AND complete the additive legacy consolidation of `service_catalog`/`offer_services` +into `services` (OFFER-13). + +## Verdict: HUMAN_NEEDED + +All **code** must-haves are verified and build-clean. The **data**-level guarantee +(OFFER-13 + physical existence of the `tags` table and consolidated rows) cannot be +verified in this environment — the dev/staging DB (`178.104.27.55:54321`) is firewalled +and unreachable. Applying the additive migration + consolidation scripts is a deliberate, +user-accepted manual step (SSH+docker exec). This is the only thing standing between this +phase and `passed`. + +## Code Must-Haves — VERIFIED ✓ + +| Requirement | Evidence | Status | +|-------------|----------|--------| +| OFFER-07 (inline-edit catalog table) | `ServiceTable.tsx` rewritten; every field is `EditableCell`; `updateServiceField` action gated by `requireAdmin()` | ✓ | +| OFFER-08 (multi-select tags + create-on-the-fly) | `tags` pgTable + `Tag`/`NewTag` types in `schema.ts`; `tags_entity_name_unique` index; `TagMultiSelect` component; `addTagToService`/`removeTagFromService` actions | ✓ | +| OFFER-09 (quick-add row, name+Enter) | `QuickAddRow` in `ServiceTable.tsx` → `quickAddService` action | ✓ | +| OFFER-10 (instant client-side search) | `CatalogSearch.tsx` (`useMemo` filter, no reload); `page.tsx` renders it; `ServiceForm.tsx` deleted | ✓ | +| Query layer | `getAllServices()` left-joins `tags` scoped to `entity_type="services"`, aggregates per-service via Map (null-guarded); `ServiceWithTags` type exported | ✓ | +| Security | All 4 new server actions call `requireAdmin()` (8 occurrences); `entity_type` hardcoded to `"services"` (no injection path) — confirmed by code review (0 Critical) | ✓ | +| Migration additive-only (LOCKED data-safety) | `0006_add_tags_table.sql` + `push-11-tags-migration.ts` contain only `CREATE TABLE IF NOT EXISTS` / `CREATE INDEX IF NOT EXISTS`; no DROP/TRUNCATE/DELETE/ALTER-DROP against any protected table | ✓ | +| Integration build | `npx tsc --noEmit` → exit 0, zero errors; `next build` clean (Wave 4) | ✓ | + +## Data Must-Haves — PENDING DB MIGRATION (human action required) + +These require the migration + consolidation scripts to run against the live DB. They are +NOT failures — they are blocked on firewall access the user owns. + +### 1. Apply the additive `tags` table migration +expected: `tags` table physically exists in the DB with the `tags_entity_name_unique` +unique index on `(entity_type, entity_id, name)`. +command: `npx tsx --env-file=.env.local scripts/push-11-tags-migration.ts` (prints +"✓ tags table created successfully" or "✓ already exists") +result: [pending] + +### 2. Run legacy consolidation (OFFER-13) +expected: `service_catalog` + `offer_services` rows fully present in `services` +(`migrated_from`/`migrated_id` populated), zero data loss. +command: `npx tsx --env-file=.env.local scripts/migrate-services.ts` then +`npx tsx --env-file=.env.local scripts/validate-services-migration.ts` → must print +`ALL CHECKS PASSED`. +result: [pending] + +### 3. Assign "Offerta" tag to migrated offer rows (D-02) +expected: every `services` row with `migrated_from='offer_services'` carries the "Offerta" +tag; count matches. +command: `npx tsx --env-file=.env.local scripts/migrate-tags.ts` then +`npx tsx --env-file=.env.local scripts/validate-tags-migration.ts` → must print +`ALL CHECKS PASSED`. +result: [pending] + +### 4. Mark OFFER-13 complete +expected: After steps 1-3 validate clean, set OFFER-13 to `Complete` in +`.planning/REQUIREMENTS.md` (currently `Pending`). +result: [pending] + +### 5. Runtime smoke test of /admin/catalog +expected: page loads, inline-edit saves, tag add/remove works, quick-add creates a service, +search filters instantly, inactive services sink below the divider. +result: [pending] + +## Code Review + +`11-REVIEW.md`: 0 Critical, 4 Warning, 5 Info. Security clean. Warnings are inline-edit +robustness bugs (WR-01 double `onSave` on toggle, WR-02/03 blur committing unchanged/invalid +values, WR-04 price locale-parse truncation). Non-blocking; address via +`/gsd-code-review 11 --fix` or in a follow-up. WR-04 (price truncation) is the most +data-relevant. + +## Production deploy note + +Per CLAUDE.md Data Safety + project memory (Gitea→Coolify, prod Postgres via SSH+docker exec): +the same additive migration MUST be applied to **production** before the Phase 11 +schema-dependent code is deployed.