fix(11): address code-review findings in catalog inline-edit

- WR-01: remove blur-commit on EditableCell toggle (onChange already saves+closes; prevents double updateServiceField)
- WR-02: skip server action when cell value unchanged (commit + new commitOnBlur dirty-check)
- WR-03: on blur, revert required-empty field via cancel() instead of leaving the cell stuck in error state
- WR-04: normalize it-IT price input (1.234,50) before parse; Number() rejects trailing garbage
- IN-01: render row error in its own <tr> instead of an invalid 7th <td> in a 6-column row

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-06-13 21:02:50 +02:00
parent 8d62207ccb
commit 42c16e1bab
3 changed files with 45 additions and 9 deletions
+10 -2
View File
@@ -93,8 +93,16 @@ export async function updateServiceField(
const s = String(value).trim(); const s = String(value).trim();
await db.update(services).set({ category: s || null }).where(eq(services.id, serviceId)); await db.update(services).set({ category: s || null }).where(eq(services.id, serviceId));
} else if (fieldName === "unit_price") { } else if (fieldName === "unit_price") {
const num = parseFloat(String(value)); // Normalize locale-formatted input (WR-04): the cell displays it-IT (€1.234,50),
if (isNaN(num) || num < 0) throw new Error("Prezzo invalido"); // so an admin may type "1.234,50". When a comma is present, treat "." as thousands
// separators and "," as the decimal mark; otherwise parse "." as the decimal mark.
// Number() (not parseFloat) rejects trailing garbage like "12abc".
const rawInput = String(value).trim();
const normalized = rawInput.includes(",")
? rawInput.replace(/\./g, "").replace(",", ".")
: rawInput;
const num = Number(normalized);
if (!Number.isFinite(num) || num < 0) throw new Error("Prezzo invalido");
await db.update(services).set({ unit_price: num.toFixed(2) }).where(eq(services.id, serviceId)); await db.update(services).set({ unit_price: num.toFixed(2) }).where(eq(services.id, serviceId));
} else if (fieldName === "active") { } else if (fieldName === "active") {
const boolValue = typeof value === "boolean" ? value : value === "true"; const boolValue = typeof value === "boolean" ? value : value === "true";
@@ -34,6 +34,7 @@ function ServiceRow({ service }: { service: ServiceWithTags }) {
} }
return ( return (
<>
<tr <tr
className={`border-b border-[#e5e7eb] hover:bg-[#f9f9f9] transition-colors duration-150 ${ className={`border-b border-[#e5e7eb] hover:bg-[#f9f9f9] transition-colors duration-150 ${
!service.active ? "opacity-50" : "" !service.active ? "opacity-50" : ""
@@ -85,12 +86,15 @@ function ServiceRow({ service }: { service: ServiceWithTags }) {
onSave={(v) => saveField("active", v)} onSave={(v) => saveField("active", v)}
/> />
</td> </td>
</tr>
{error && ( {error && (
<tr>
<td className="py-1 px-3 text-xs text-red-600" colSpan={6}> <td className="py-1 px-3 text-xs text-red-600" colSpan={6}>
{error} {error}
</td> </td>
)}
</tr> </tr>
)}
</>
); );
} }
+28 -4
View File
@@ -46,6 +46,12 @@ export function EditableCell({
} }
function commit() { function commit() {
// WR-02: skip redundant server action when nothing changed
if (tempValue === String(value)) {
setError(null);
setIsEditing(false);
return;
}
if (required && tempValue.trim().length === 0) { if (required && tempValue.trim().length === 0) {
setError("Campo richiesto"); setError("Campo richiesto");
return; return;
@@ -55,6 +61,23 @@ export function EditableCell({
setIsEditing(false); setIsEditing(false);
} }
// Blur path: never leave the cell stuck. No-op if unchanged (WR-02);
// revert to the last valid value instead of persisting/holding an
// invalid required field while focus has already moved away (WR-03).
function commitOnBlur() {
if (tempValue === String(value)) {
cancel();
return;
}
if (required && tempValue.trim().length === 0) {
cancel();
return;
}
setError(null);
onSave(tempValue);
setIsEditing(false);
}
function cancel() { function cancel() {
setTempValue(String(value)); setTempValue(String(value));
setError(null); setError(null);
@@ -103,7 +126,7 @@ export function EditableCell({
ref={inputRef as React.Ref<HTMLTextAreaElement>} ref={inputRef as React.Ref<HTMLTextAreaElement>}
value={tempValue} value={tempValue}
onChange={(e) => setTempValue(e.target.value)} onChange={(e) => setTempValue(e.target.value)}
onBlur={commit} onBlur={commitOnBlur}
onKeyDown={handleKeyDown} onKeyDown={handleKeyDown}
placeholder={placeholder} placeholder={placeholder}
className={cn("ring-1 ring-primary resize-none text-sm", error && "ring-2 ring-red-500")} className={cn("ring-1 ring-primary resize-none text-sm", error && "ring-2 ring-red-500")}
@@ -120,8 +143,9 @@ export function EditableCell({
onSave(next); onSave(next);
setIsEditing(false); setIsEditing(false);
}} }}
onBlur={commit} // WR-01: onChange already saves + closes — a blur-commit here would
onKeyDown={handleKeyDown} // fire a second (possibly stale) updateServiceField for the same field.
onKeyDown={(e) => { if (e.key === "Escape") cancel(); }}
className="h-4 w-4 cursor-pointer accent-[#1A463C]" className="h-4 w-4 cursor-pointer accent-[#1A463C]"
/> />
) : ( ) : (
@@ -130,7 +154,7 @@ export function EditableCell({
type={type} type={type}
value={tempValue} value={tempValue}
onChange={(e) => setTempValue(e.target.value)} onChange={(e) => setTempValue(e.target.value)}
onBlur={commit} onBlur={commitOnBlur}
onKeyDown={handleKeyDown} onKeyDown={handleKeyDown}
placeholder={placeholder} placeholder={placeholder}
step={type === "number" ? "0.01" : undefined} step={type === "number" ? "0.01" : undefined}