From 3f61f070349968d5c25fdf29f70dfbd50b0caefc Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 19:03:49 -0400 Subject: [PATCH] Address Gadfly review on #8: modal focus/close, dim validation, dedup Fixes from the PR #27 adversarial review (considered; not graded). Correctness / error-handling - Modal: focus once on mount and attach the keydown listener once (latest onClose/busy via refs), so a parent re-render (e.g. a background refetch) no longer re-fires focus() and steals it from the input being typed in. - Modal takes a `busy` prop; while a save/delete is in flight, Escape and backdrop clicks don't dismiss it (form/delete modals pass busy=pending), so an action can't be interrupted mid-flight and lose its error. - Form validates the *converted* centimeter values against the server's [1cm, 100m] bounds, so sub-cm / over-100m sizes fail client-side with a clear message instead of a generic server error. - displayFromCm rounds imperial to 0.1 ft so an 8 ft garden (244 cm) prefills as "8", not "8.01". Maintainability - Extracted ui/field.ts (useFieldId + fieldControlClass) shared by TextField/TextArea/Select. Added a `danger` Button variant, used by the delete modal; card edit/delete buttons gained focus-visible rings. - useUpdateGarden takes the id in its mutation variables (no dummy 0 during create). GardensPage shares a close handler; dropped a redundant zod annotation; 409 rebase reuses dimString; renamed `del` -> `deletion`. Verified in a browser: 8 ft round-trips to a prefilled "8"; a sub-cm value is rejected client-side with the modal staying open. tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- .../components/gardens/DeleteGardenModal.tsx | 17 +++----- web/src/components/gardens/GardenCard.tsx | 4 +- .../components/gardens/GardenFormModal.tsx | 41 +++++++++++-------- web/src/components/ui/Button.tsx | 3 +- web/src/components/ui/Modal.tsx | 24 +++++++---- web/src/components/ui/Select.tsx | 19 ++------- web/src/components/ui/TextArea.tsx | 21 ++-------- web/src/components/ui/TextField.tsx | 17 ++------ web/src/components/ui/field.ts | 18 ++++++++ web/src/lib/gardens.ts | 9 ++-- web/src/lib/units.ts | 21 ++++++++-- web/src/pages/GardensPage.tsx | 12 +++--- 12 files changed, 110 insertions(+), 96 deletions(-) create mode 100644 web/src/components/ui/field.ts diff --git a/web/src/components/gardens/DeleteGardenModal.tsx b/web/src/components/gardens/DeleteGardenModal.tsx index 1c374b0..195552b 100644 --- a/web/src/components/gardens/DeleteGardenModal.tsx +++ b/web/src/components/gardens/DeleteGardenModal.tsx @@ -7,13 +7,13 @@ import { useDeleteGarden, type Garden } from '@/lib/gardens' /** Confirmation dialog for deleting a garden (and everything in it). */ export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) { - const del = useDeleteGarden() + const deletion = useDeleteGarden() const [error, setError] = useState(null) async function onConfirm() { setError(null) try { - await del.mutateAsync(garden.id) + await deletion.mutateAsync(garden.id) onClose() } catch (err) { setError(errorMessage(err, 'Could not delete the garden.')) @@ -21,7 +21,7 @@ export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose } return ( - +

Delete {garden.name} and everything planned in it? @@ -29,16 +29,11 @@ export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose

{error && {error}}
- -
diff --git a/web/src/components/gardens/GardenCard.tsx b/web/src/components/gardens/GardenCard.tsx index 1f34424..61828f5 100644 --- a/web/src/components/gardens/GardenCard.tsx +++ b/web/src/components/gardens/GardenCard.tsx @@ -32,14 +32,14 @@ export function GardenCard({ diff --git a/web/src/components/gardens/GardenFormModal.tsx b/web/src/components/gardens/GardenFormModal.tsx index 7a4db68..14a7969 100644 --- a/web/src/components/gardens/GardenFormModal.tsx +++ b/web/src/components/gardens/GardenFormModal.tsx @@ -7,7 +7,13 @@ import { TextArea } from '@/components/ui/TextArea' import { TextField } from '@/components/ui/TextField' import { errorMessage } from '@/lib/api' import { conflictGarden, useCreateGarden, useUpdateGarden, type Garden } from '@/lib/gardens' -import { cmFromDisplay, dimensionUnitLabel, displayFromCm, type UnitPref } from '@/lib/units' +import { + cmFromDisplay, + dimensionUnitLabel, + displayFromCm, + isValidDimensionCm, + type UnitPref, +} from '@/lib/units' const DEFAULT_METERS = 10 // matches the server's 10 m default @@ -29,7 +35,7 @@ function dimString(cm: number | undefined, unit: UnitPref): string { export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) { const isEdit = !!garden const create = useCreateGarden() - const update = useUpdateGarden(garden?.id ?? 0) + const update = useUpdateGarden() const pending = create.isPending || update.isPending const [name, setName] = useState(garden?.name ?? '') @@ -56,24 +62,25 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: setFormError(null) setConflict(null) - const w = parseFloat(width) - const h = parseFloat(height) - if (!name.trim() || !Number.isFinite(w) || w <= 0 || !Number.isFinite(h) || h <= 0) { - setFormError('Enter a name and a positive width and height.') + if (!name.trim()) { + setFormError('Enter a name for the garden.') + return + } + // Validate the converted centimeter values against the same bounds the + // server enforces, so sub-cm or over-100m sizes fail here with a clear + // message instead of a generic server error. + const widthCm = cmFromDisplay(parseFloat(width), unit) + const heightCm = cmFromDisplay(parseFloat(height), unit) + if (!isValidDimensionCm(widthCm) || !isValidDimensionCm(heightCm)) { + setFormError('Width and height must be between 1 cm and 100 m.') return } - const input = { - name: name.trim(), - widthCm: cmFromDisplay(w, unit), - heightCm: cmFromDisplay(h, unit), - unitPref: unit, - notes: notes.trim(), - } + const input = { name: name.trim(), widthCm, heightCm, unitPref: unit, notes: notes.trim() } try { if (isEdit) { - await update.mutateAsync({ ...input, version }) + await update.mutateAsync({ id: garden.id, ...input, version }) } else { await create.mutateAsync(input) } @@ -86,8 +93,8 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: setVersion(current.version) setName(current.name) setUnit(current.unitPref) - setWidth(String(displayFromCm(current.widthCm, current.unitPref))) - setHeight(String(displayFromCm(current.heightCm, current.unitPref))) + setWidth(dimString(current.widthCm, current.unitPref)) + setHeight(dimString(current.heightCm, current.unitPref)) setNotes(current.notes) setConflict('This garden changed elsewhere. The latest values are shown — review and save again.') return @@ -99,7 +106,7 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: const unitLabel = dimensionUnitLabel(unit) return ( - +
{conflict && {conflict}} diff --git a/web/src/components/ui/Button.tsx b/web/src/components/ui/Button.tsx index 331078f..9165349 100644 --- a/web/src/components/ui/Button.tsx +++ b/web/src/components/ui/Button.tsx @@ -1,7 +1,7 @@ import type { ButtonHTMLAttributes } from 'react' import { cn } from '@/lib/cn' -export type ButtonVariant = 'primary' | 'ghost' +export type ButtonVariant = 'primary' | 'ghost' | 'danger' /** Shared button styling, exported so anchor "buttons" (e.g. the OIDC link) match. */ export function buttonClasses(variant: ButtonVariant = 'primary', className?: string) { @@ -11,6 +11,7 @@ export function buttonClasses(variant: ButtonVariant = 'primary', className?: st 'disabled:cursor-not-allowed disabled:opacity-60', variant === 'primary' && 'bg-accent text-accent-contrast hover:bg-accent-strong', variant === 'ghost' && 'border border-border text-fg hover:bg-border/50', + variant === 'danger' && 'bg-red-600 text-white hover:bg-red-700 focus-visible:ring-red-500/40', className, ) } diff --git a/web/src/components/ui/Modal.tsx b/web/src/components/ui/Modal.tsx index 1a5693b..e671552 100644 --- a/web/src/components/ui/Modal.tsx +++ b/web/src/components/ui/Modal.tsx @@ -1,35 +1,45 @@ import { useEffect, useRef, type ReactNode } from 'react' /** - * A centered modal dialog rendered over a dimmed backdrop. Closes on Escape or a - * backdrop click. Wraps content in a native -like card; the caller owns - * the open/closed state (render it only when open). + * A centered modal dialog over a dimmed backdrop. Closes on Escape or a backdrop + * click, unless `busy` (a mutation is in flight) — then it stays put so the + * action can finish and report. The caller owns open/closed state (render only + * when open). */ export function Modal({ title, onClose, + busy = false, children, }: { title: string onClose: () => void + busy?: boolean children: ReactNode }) { const cardRef = useRef(null) + // Keep the latest onClose/busy in refs so the mount-only effect below never + // re-runs (which would re-attach the listener and steal focus on every parent + // re-render, e.g. during a background refetch). + const onCloseRef = useRef(onClose) + onCloseRef.current = onClose + const busyRef = useRef(busy) + busyRef.current = busy useEffect(() => { + cardRef.current?.focus() function onKey(e: KeyboardEvent) { - if (e.key === 'Escape') onClose() + if (e.key === 'Escape' && !busyRef.current) onCloseRef.current() } document.addEventListener('keydown', onKey) - cardRef.current?.focus() return () => document.removeEventListener('keydown', onKey) - }, [onClose]) + }, []) return (
{ - if (e.target === e.currentTarget) onClose() + if (e.target === e.currentTarget && !busy) onClose() }} >
{ label: string options: { value: string; label: string }[] } -// Labelled native +