9.6 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 11-catalog-database-view-ux-legacy-consolidation | 2026-06-13T14:10:00Z | standard | 12 |
|
|
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:
} : 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:
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