132 lines
9.6 KiB
Markdown
132 lines
9.6 KiB
Markdown
---
|
|
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" ? (
|
|
<input
|
|
ref={inputRef as React.Ref<HTMLInputElement>}
|
|
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 `<input>` 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 `<td>` 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 `<td colSpan={6}>` inside the same `<tr>` 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 `<tr>` below the data row, or in a dedicated status area.
|
|
**Fix:** Move the error into its own row: `{error && <tr><td colSpan={6} className="...">{error}</td></tr>}` rendered after the data `<tr>`.
|
|
|
|
### 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_
|