Gardens UI: list page + create/edit/delete (#8) #27

Merged
steve merged 2 commits from phase-2-gardens-ui into main 2026-07-18 23:05:27 +00:00
12 changed files with 110 additions and 96 deletions
Showing only changes of commit 3f61f07034 - Show all commits
@@ -7,13 +7,13 @@ import { useDeleteGarden, type Garden } from '@/lib/gardens'
/** Confirmation dialog for deleting a garden (and everything in it). */ /** Confirmation dialog for deleting a garden (and everything in it). */
export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) { export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const del = useDeleteGarden() const deletion = useDeleteGarden()
Outdated
Review

Variable named del is unnecessarily terse

maintainability · flagged by 1 model

  • web/src/components/gardens/DeleteGardenModal.tsx:10 — Variable named del is unnecessarily terse. Fix: rename to deleteGarden or mutation.

🪰 Gadfly · advisory

⚪ **Variable named del is unnecessarily terse** _maintainability · flagged by 1 model_ - `web/src/components/gardens/DeleteGardenModal.tsx:10` — Variable named `del` is unnecessarily terse. **Fix:** rename to `deleteGarden` or `mutation`. <sub>🪰 Gadfly · advisory</sub>
const [error, setError] = useState<string | null>(null) const [error, setError] = useState<string | null>(null)
async function onConfirm() { async function onConfirm() {
setError(null) setError(null)
try { try {
await del.mutateAsync(garden.id) await deletion.mutateAsync(garden.id)
onClose() onClose()
} catch (err) { } catch (err) {
setError(errorMessage(err, 'Could not delete the garden.')) setError(errorMessage(err, 'Could not delete the garden.'))
@@ -21,7 +21,7 @@ export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose
} }
return ( return (
<Modal title="Delete garden" onClose={onClose}> <Modal title="Delete garden" onClose={onClose} busy={deletion.isPending}>
<div className="flex flex-col gap-4"> <div className="flex flex-col gap-4">
<p className="text-sm text-muted"> <p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{garden.name}</span> and everything planned in it? Delete <span className="font-medium text-fg">{garden.name}</span> and everything planned in it?
@@ -29,16 +29,11 @@ export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose
</p> </p>
{error && <Alert>{error}</Alert>} {error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2"> <div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={del.isPending}> <Button type="button" variant="ghost" onClick={onClose} disabled={deletion.isPending}>
Cancel Cancel
Review

🟡 Destructive-action button hardcodes red classes instead of adding a danger variant to buttonClasses

maintainability · flagged by 1 model

  • web/src/components/gardens/DeleteGardenModal.tsx:33-42 — the destructive Delete button uses <Button> (confirmed) but overrides its background via a hardcoded className="bg-red-600 text-white hover:bg-red-700" rather than the component exposing a danger variant. Since ButtonVariant in Button.tsx:4 is only 'primary' | 'ghost', this ad hoc override is the only way to get a red button today, and it's the kind of styling that's likely to get copy-pasted for the next destructive action…

🪰 Gadfly · advisory

🟡 **Destructive-action button hardcodes red classes instead of adding a danger variant to buttonClasses** _maintainability · flagged by 1 model_ - `web/src/components/gardens/DeleteGardenModal.tsx:33-42` — the destructive Delete button uses `<Button>` (confirmed) but overrides its background via a hardcoded `className="bg-red-600 text-white hover:bg-red-700"` rather than the component exposing a `danger` variant. Since `ButtonVariant` in `Button.tsx:4` is only `'primary' | 'ghost'`, this ad hoc override is the only way to get a red button today, and it's the kind of styling that's likely to get copy-pasted for the next destructive action… <sub>🪰 Gadfly · advisory</sub>
</Button> </Button>
<Button <Button type="button" variant="danger" onClick={onConfirm} disabled={deletion.isPending}>
type="button" {deletion.isPending ? 'Deleting' : 'Delete'}
onClick={onConfirm}
disabled={del.isPending}
className="bg-red-600 text-white hover:bg-red-700"
>
{del.isPending ? 'Deleting' : 'Delete'}
</Button> </Button>
</div> </div>
</div> </div>
+2 -2
View File
@@ -32,14 +32,14 @@ export function GardenCard({
<button <button
Review

🟠 Raw elements bypass shared Button component styles and accessibility

maintainability · flagged by 2 models

  • web/src/components/gardens/GardenCard.tsx:32 — The card footer uses raw <button> elements instead of the shared <Button> component introduced/used everywhere else in this PR. This duplicates hover/transition styles and, more importantly, omits the focus-visible:ring-2 focus-visible:ring-accent/40 and disabled:cursor-not-allowed disabled:opacity-60 behaviour that Button provides. If these actions ever need disabled states or if the design system changes, edits must be made in bo…

🪰 Gadfly · advisory

🟠 **Raw <button> elements bypass shared Button component styles and accessibility** _maintainability · flagged by 2 models_ - **`web/src/components/gardens/GardenCard.tsx:32`** — The card footer uses raw `<button>` elements instead of the shared `<Button>` component introduced/used everywhere else in this PR. This duplicates hover/transition styles and, more importantly, omits the `focus-visible:ring-2 focus-visible:ring-accent/40` and `disabled:cursor-not-allowed disabled:opacity-60` behaviour that `Button` provides. If these actions ever need disabled states or if the design system changes, edits must be made in bo… <sub>🪰 Gadfly · advisory</sub>
type="button" type="button"
onClick={onEdit} onClick={onEdit}
className="rounded-md px-2.5 py-1 text-sm font-medium text-muted transition-colors hover:bg-border/50 hover:text-fg" className="rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors hover:bg-border/50 hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
> >
Edit Edit
</button> </button>
<button <button
Review

🟠 Raw elements bypass shared Button component styles and accessibility

maintainability · flagged by 1 model

  • web/src/components/gardens/GardenCard.tsx:39 — The card footer uses raw <button> elements instead of the shared <Button> component introduced/used everywhere else in this PR. This duplicates hover/transition styles and, more importantly, omits the focus-visible:ring-2 focus-visible:ring-accent/40 and disabled:cursor-not-allowed disabled:opacity-60 behaviour that Button provides. If these actions ever need disabled states or if the design system changes, edits must be made in bo…

🪰 Gadfly · advisory

🟠 **Raw <button> elements bypass shared Button component styles and accessibility** _maintainability · flagged by 1 model_ - **`web/src/components/gardens/GardenCard.tsx:39`** — The card footer uses raw `<button>` elements instead of the shared `<Button>` component introduced/used everywhere else in this PR. This duplicates hover/transition styles and, more importantly, omits the `focus-visible:ring-2 focus-visible:ring-accent/40` and `disabled:cursor-not-allowed disabled:opacity-60` behaviour that `Button` provides. If these actions ever need disabled states or if the design system changes, edits must be made in bo… <sub>🪰 Gadfly · advisory</sub>
type="button" type="button"
onClick={onDelete} onClick={onDelete}
className="rounded-md px-2.5 py-1 text-sm font-medium text-muted transition-colors hover:bg-red-500/10 hover:text-red-600 dark:hover:text-red-400" className="rounded-md px-2.5 py-1 text-sm font-medium text-muted outline-none transition-colors hover:bg-red-500/10 hover:text-red-600 focus-visible:ring-2 focus-visible:ring-red-500/40 dark:hover:text-red-400"
> >
Delete Delete
</button> </button>
+24 -17
View File
@@ -7,7 +7,13 @@ import { TextArea } from '@/components/ui/TextArea'
import { TextField } from '@/components/ui/TextField' import { TextField } from '@/components/ui/TextField'
import { errorMessage } from '@/lib/api' import { errorMessage } from '@/lib/api'
import { conflictGarden, useCreateGarden, useUpdateGarden, type Garden } from '@/lib/gardens' 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 const DEFAULT_METERS = 10 // matches the server's 10 m default
1
@@ -29,7 +35,7 @@ function dimString(cm: number | undefined, unit: UnitPref): string {
export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) { export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) {
const isEdit = !!garden const isEdit = !!garden
const create = useCreateGarden() const create = useCreateGarden()
const update = useUpdateGarden(garden?.id ?? 0) const update = useUpdateGarden()
const pending = create.isPending || update.isPending const pending = create.isPending || update.isPending
const [name, setName] = useState(garden?.name ?? '') const [name, setName] = useState(garden?.name ?? '')
2
@@ -56,24 +62,25 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
setFormError(null) setFormError(null)
setConflict(null) setConflict(null)
const w = parseFloat(width) if (!name.trim()) {
const h = parseFloat(height) setFormError('Enter a name for the garden.')
Outdated
Review

🟠 Form validates parsed float > 0 but small positive values round to 0 cm in API

correctness, error-handling · flagged by 2 models

  • web/src/components/gardens/GardenFormModal.tsx:68-72 — Frontend validation checks w > 0 on the parsed float, but the API stores whole centimeters. A value such as 0.004 m (or 0.01 ft) is accepted as “positive” yet cmFromDisplay(0.004, 'metric') = Math.round(0.4) = 0 cm. The server rejects 0 cm on update (validDimensionCM requires v >= 1), and on create it silently defaults the dimension to 10 m. The user passes frontend validation only to hit a backend error or get an unexp…

🪰 Gadfly · advisory

🟠 **Form validates parsed float > 0 but small positive values round to 0 cm in API** _correctness, error-handling · flagged by 2 models_ - **`web/src/components/gardens/GardenFormModal.tsx:68-72`** — Frontend validation checks `w > 0` on the parsed float, but the API stores whole centimeters. A value such as `0.004` m (or `0.01` ft) is accepted as “positive” yet `cmFromDisplay(0.004, 'metric')` = `Math.round(0.4)` = 0 cm. The server rejects 0 cm on update (`validDimensionCM` requires `v >= 1`), and on create it silently defaults the dimension to 10 m. The user passes frontend validation only to hit a backend error or get an unexp… <sub>🪰 Gadfly · advisory</sub>
if (!name.trim() || !Number.isFinite(w) || w <= 0 || !Number.isFinite(h) || h <= 0) { return
setFormError('Enter a name and a positive width and height.') }
// 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 return
} }
const input = { const input = { name: name.trim(), widthCm, heightCm, unitPref: unit, notes: notes.trim() }
name: name.trim(),
widthCm: cmFromDisplay(w, unit),
heightCm: cmFromDisplay(h, unit),
unitPref: unit,
notes: notes.trim(),
}
try { try {
if (isEdit) { if (isEdit) {
await update.mutateAsync({ ...input, version }) await update.mutateAsync({ id: garden.id, ...input, version })
} else { } else {
await create.mutateAsync(input) await create.mutateAsync(input)
} }
1
@@ -86,8 +93,8 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
setVersion(current.version) setVersion(current.version)
setName(current.name) setName(current.name)
setUnit(current.unitPref) setUnit(current.unitPref)
setWidth(String(displayFromCm(current.widthCm, current.unitPref))) setWidth(dimString(current.widthCm, current.unitPref))
setHeight(String(displayFromCm(current.heightCm, current.unitPref))) setHeight(dimString(current.heightCm, current.unitPref))
setNotes(current.notes) setNotes(current.notes)
setConflict('This garden changed elsewhere. The latest values are shown — review and save again.') setConflict('This garden changed elsewhere. The latest values are shown — review and save again.')
return return
@@ -99,7 +106,7 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
const unitLabel = dimensionUnitLabel(unit) const unitLabel = dimensionUnitLabel(unit)
return ( return (
<Modal title={isEdit ? 'Edit garden' : 'New garden'} onClose={onClose}> <Modal title={isEdit ? 'Edit garden' : 'New garden'} onClose={onClose} busy={pending}>
<form onSubmit={onSubmit} className="flex flex-col gap-3"> <form onSubmit={onSubmit} className="flex flex-col gap-3">
{conflict && <Alert tone="info">{conflict}</Alert>} {conflict && <Alert tone="info">{conflict}</Alert>}
+2 -1
View File
@@ -1,7 +1,7 @@
import type { ButtonHTMLAttributes } from 'react' import type { ButtonHTMLAttributes } from 'react'
import { cn } from '@/lib/cn' 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. */ /** Shared button styling, exported so anchor "buttons" (e.g. the OIDC link) match. */
export function buttonClasses(variant: ButtonVariant = 'primary', className?: string) { 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', 'disabled:cursor-not-allowed disabled:opacity-60',
variant === 'primary' && 'bg-accent text-accent-contrast hover:bg-accent-strong', variant === 'primary' && 'bg-accent text-accent-contrast hover:bg-accent-strong',
variant === 'ghost' && 'border border-border text-fg hover:bg-border/50', 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, className,
) )
} }
+17 -7
View File
@@ -1,35 +1,45 @@
import { useEffect, useRef, type ReactNode } from 'react' import { useEffect, useRef, type ReactNode } from 'react'
/** /**
* A centered modal dialog rendered over a dimmed backdrop. Closes on Escape or a * A centered modal dialog over a dimmed backdrop. Closes on Escape or a backdrop
* backdrop click. Wraps content in a native <dialog>-like card; the caller owns * click, unless `busy` (a mutation is in flight) — then it stays put so the
* the open/closed state (render it only when open). * action can finish and report. The caller owns open/closed state (render only
* when open).
*/ */
export function Modal({ export function Modal({
title, title,
onClose, onClose,
busy = false,
children, children,
}: { }: {
title: string title: string
onClose: () => void onClose: () => void
busy?: boolean
Outdated
Review

🔴 Modal useEffect re-runs on every onClose change, repeatedly calling focus() and stealing focus from active inputs during parent re-renders

correctness, error-handling, maintainability, performance · flagged by 5 models

  • web/src/components/ui/Modal.tsx:19-26 — The useEffect depends on onClose, which is recreated on every parent render (e.g. () => setDialog(null) in GardensPage). Each time the parent re-renders — such as during a background React Query refetch — the effect re-runs, the old keydown listener is torn down and a new one added, and cardRef.current?.focus() fires again. This steals focus from any active input inside the modal (e.g. the user typing the garden name), blurring the field…

🪰 Gadfly · advisory

🔴 **Modal useEffect re-runs on every onClose change, repeatedly calling focus() and stealing focus from active inputs during parent re-renders** _correctness, error-handling, maintainability, performance · flagged by 5 models_ * **web/src/components/ui/Modal.tsx:19-26** — The `useEffect` depends on `onClose`, which is recreated on every parent render (e.g. `() => setDialog(null)` in `GardensPage`). Each time the parent re-renders — such as during a background React Query refetch — the effect re-runs, the old `keydown` listener is torn down and a new one added, and `cardRef.current?.focus()` fires again. This steals focus from any active input inside the modal (e.g. the user typing the garden name), blurring the field… <sub>🪰 Gadfly · advisory</sub>
children: ReactNode children: ReactNode
}) { }) {
const cardRef = useRef<HTMLDivElement>(null) const cardRef = useRef<HTMLDivElement>(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(() => { useEffect(() => {
cardRef.current?.focus()
function onKey(e: KeyboardEvent) { function onKey(e: KeyboardEvent) {
Review

🔴 Modal dismissible during pending mutation causing lost error feedback

correctness · flagged by 1 model

  • web/src/components/ui/Modal.tsx:31 and web/src/components/gardens/GardenFormModal.tsx:102 — The modal can be dismissed while a mutation is in flight. Escape and backdrop click unconditionally invoke onClose. If the user hits Escape (or clicks the backdrop) after pressing Save but before the network call finishes, the modal unmounts. Should the mutation then fail, the catch block in GardenFormModal sets error state on an unmounted component and the user never sees the failure. **Fix…

🪰 Gadfly · advisory

🔴 **Modal dismissible during pending mutation causing lost error feedback** _correctness · flagged by 1 model_ - **`web/src/components/ui/Modal.tsx:31` and `web/src/components/gardens/GardenFormModal.tsx:102`** — The modal can be dismissed while a mutation is in flight. Escape and backdrop click unconditionally invoke `onClose`. If the user hits Escape (or clicks the backdrop) after pressing Save but before the network call finishes, the modal unmounts. Should the mutation then fail, the catch block in `GardenFormModal` sets error state on an unmounted component and the user never sees the failure. **Fix… <sub>🪰 Gadfly · advisory</sub>
if (e.key === 'Escape') onClose() if (e.key === 'Escape' && !busyRef.current) onCloseRef.current()
} }
document.addEventListener('keydown', onKey) document.addEventListener('keydown', onKey)
cardRef.current?.focus()
return () => document.removeEventListener('keydown', onKey) return () => document.removeEventListener('keydown', onKey)
}, [onClose]) }, [])
return ( return (
<div <div
className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 p-4 sm:items-center" className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 p-4 sm:items-center"
onMouseDown={(e) => { onMouseDown={(e) => {
if (e.target === e.currentTarget) onClose() if (e.target === e.currentTarget && !busy) onClose()
}} }}
> >
<div <div
+4 -15
View File
@@ -1,34 +1,23 @@
import { forwardRef, useId, type SelectHTMLAttributes } from 'react' import { forwardRef, type SelectHTMLAttributes } from 'react'
import { cn } from '@/lib/cn' import { cn } from '@/lib/cn'
import { fieldControlClass, useFieldId } from './field'
interface SelectProps extends SelectHTMLAttributes<HTMLSelectElement> { interface SelectProps extends SelectHTMLAttributes<HTMLSelectElement> {
label: string label: string
options: { value: string; label: string }[] options: { value: string; label: string }[]
} }
// Labelled native <select>, styled to match the text inputs.
export const Select = forwardRef<HTMLSelectElement, SelectProps>(function Select( export const Select = forwardRef<HTMLSelectElement, SelectProps>(function Select(
{ label, options, id, name, className, ...props }, { label, options, id, name, className, ...props },
ref, ref,
) { ) {
const generatedId = useId() const fieldId = useFieldId(id, name)
Outdated
Review

🟡 Copied useId fallback logic from TextField instead of extracting shared hook

maintainability · flagged by 1 model

  • web/src/components/ui/Select.tsx:14 — The useId() + id ?? name ?? generatedId fallback chain is copied verbatim from the pre-existing TextField (the TextArea comment is even a near-duplicate of TextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. adding aria-describedby support or changing fallback priority—require touching three components. Fix: extract a tiny useFieldId(id?: string, name?: string) hook (or a shared FormControl wrappe…

🪰 Gadfly · advisory

🟡 **Copied useId fallback logic from TextField instead of extracting shared hook** _maintainability · flagged by 1 model_ - **`web/src/components/ui/Select.tsx:14`** — The `useId()` + `id ?? name ?? generatedId` fallback chain is copied verbatim from the pre-existing `TextField` (the `TextArea` comment is even a near-duplicate of `TextField`’s). Adding two more copies of this three-line pattern makes future changes—e.g. adding `aria-describedby` support or changing fallback priority—require touching three components. Fix: extract a tiny `useFieldId(id?: string, name?: string)` hook (or a shared `FormControl` wrappe… <sub>🪰 Gadfly · advisory</sub>
const fieldId = id ?? name ?? generatedId
return ( return (
<div className="flex flex-col gap-1.5"> <div className="flex flex-col gap-1.5">
<label htmlFor={fieldId} className="text-sm font-medium text-fg"> <label htmlFor={fieldId} className="text-sm font-medium text-fg">
{label} {label}
</label> </label>
<select <select ref={ref} id={fieldId} name={name} className={cn(fieldControlClass, className)} {...props}>
ref={ref}
id={fieldId}
name={name}
className={cn(
'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg',
'outline-none transition-colors focus:border-accent focus:ring-2 focus:ring-accent/30 disabled:opacity-60',
className,
)}
{...props}
>
{options.map((o) => ( {options.map((o) => (
<option key={o.value} value={o.value}> <option key={o.value} value={o.value}>
{o.label} {o.label}
1
+4 -17
View File
@@ -1,35 +1,22 @@
import { forwardRef, useId, type TextareaHTMLAttributes } from 'react' import { forwardRef, type TextareaHTMLAttributes } from 'react'
import { cn } from '@/lib/cn' import { cn } from '@/lib/cn'
import { fieldControlClass, useFieldId } from './field'
interface TextAreaProps extends TextareaHTMLAttributes<HTMLTextAreaElement> { interface TextAreaProps extends TextareaHTMLAttributes<HTMLTextAreaElement> {
label: string label: string
} }
// Labelled multi-line input, matching TextField's styling (16px text avoids iOS
// zoom). The id falls back to name then a generated id so the label always binds.
export const TextArea = forwardRef<HTMLTextAreaElement, TextAreaProps>(function TextArea( export const TextArea = forwardRef<HTMLTextAreaElement, TextAreaProps>(function TextArea(
{ label, id, name, className, ...props }, { label, id, name, className, ...props },
ref, ref,
) { ) {
const generatedId = useId() const fieldId = useFieldId(id, name)
const fieldId = id ?? name ?? generatedId
return ( return (
Review

🟡 Copied useId fallback logic from TextField instead of extracting shared hook

maintainability · flagged by 2 models

  • web/src/components/ui/TextArea.tsx:14 — The useId() + id ?? name ?? generatedId fallback chain is copied verbatim from the pre-existing TextField (the TextArea comment is even a near-duplicate of TextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. adding aria-describedby support or changing fallback priority—require touching three components. Fix: extract a tiny useFieldId(id?: string, name?: string) hook (or a shared FormControl wrap…

🪰 Gadfly · advisory

🟡 **Copied useId fallback logic from TextField instead of extracting shared hook** _maintainability · flagged by 2 models_ - **`web/src/components/ui/TextArea.tsx:14`** — The `useId()` + `id ?? name ?? generatedId` fallback chain is copied verbatim from the pre-existing `TextField` (the `TextArea` comment is even a near-duplicate of `TextField`’s). Adding two more copies of this three-line pattern makes future changes—e.g. adding `aria-describedby` support or changing fallback priority—require touching three components. Fix: extract a tiny `useFieldId(id?: string, name?: string)` hook (or a shared `FormControl` wrap… <sub>🪰 Gadfly · advisory</sub>
<div className="flex flex-col gap-1.5"> <div className="flex flex-col gap-1.5">
<label htmlFor={fieldId} className="text-sm font-medium text-fg"> <label htmlFor={fieldId} className="text-sm font-medium text-fg">
{label} {label}
</label> </label>
<textarea <textarea ref={ref} id={fieldId} name={name} className={cn(fieldControlClass, className)} {...props} />
ref={ref}
id={fieldId}
name={name}
className={cn(
'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg',
'placeholder:text-muted/70 outline-none transition-colors',
'focus:border-accent focus:ring-2 focus:ring-accent/30 disabled:opacity-60',
className,
)}
{...props}
/>
</div> </div>
) )
}) })
+4 -13
View File
@@ -1,20 +1,17 @@
import { forwardRef, useId, type InputHTMLAttributes } from 'react' import { forwardRef, type InputHTMLAttributes } from 'react'
import { cn } from '@/lib/cn' import { cn } from '@/lib/cn'
import { fieldControlClass, useFieldId } from './field'
interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> { interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> {
label: string label: string
hint?: string hint?: string
} }
// A labelled text input. text-base (16px) is deliberate: smaller fonts make iOS
// Safari zoom on focus. The field id falls back to the name, then to a generated
// id, so the label/hint associations always hold.
export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function TextField( export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function TextField(
{ label, hint, id, name, className, ...props }, { label, hint, id, name, className, ...props },
ref, ref,
) { ) {
const generatedId = useId() const inputId = useFieldId(id, name)
const inputId = id ?? name ?? generatedId
const hintId = hint ? `${inputId}-hint` : undefined const hintId = hint ? `${inputId}-hint` : undefined
return ( return (
<div className="flex flex-col gap-1.5"> <div className="flex flex-col gap-1.5">
@@ -26,13 +23,7 @@ export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function T
id={inputId} id={inputId}
name={name} name={name}
aria-describedby={hintId} aria-describedby={hintId}
className={cn( className={cn(fieldControlClass, className)}
'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg',
'placeholder:text-muted/70 outline-none transition-colors',
'focus:border-accent focus:ring-2 focus:ring-accent/30',
'disabled:opacity-60',
className,
)}
{...props} {...props}
/> />
{hint && ( {hint && (
+18
View File
@@ -0,0 +1,18 @@
import { useId } from 'react'
// Shared field plumbing for the labelled form controls (TextField, TextArea,
// Select) so their id-fallback and base styling stay in one place.
/** The field id: an explicit id, else the name, else a generated stable id, so
* the label's htmlFor always binds to something. */
export function useFieldId(id?: string, name?: string): string {
const generated = useId()
return id ?? name ?? generated
}
/** Base classes shared by the text/select/textarea controls. text-base (16px)
* keeps iOS Safari from zooming on focus. */
export const fieldControlClass =
'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg ' +
'outline-none transition-colors placeholder:text-muted/70 ' +
'focus:border-accent focus:ring-2 focus:ring-accent/30 disabled:opacity-60'
+6 -3
View File
@@ -6,7 +6,7 @@ import { z } from 'zod'
import { ApiError, api } from './api' import { ApiError, api } from './api'
import type { UnitPref } from './units' import type { UnitPref } from './units'
const unitPrefSchema: z.ZodType<UnitPref> = z.enum(['metric', 'imperial']) const unitPrefSchema = z.enum(['metric', 'imperial'])
Outdated
Review

Redundant z.ZodType annotation diverges from auth.ts sibling pattern

maintainability · flagged by 1 model

  • web/src/lib/gardens.ts:9const unitPrefSchema: z.ZodType<UnitPref> = z.enum(['metric', 'imperial']) carries an explicit annotation that auth.ts (the sibling data layer) doesn't use; z.enum already infers the literal union. The annotation is harmless but breaks the local convention for no real benefit. Trivial.

🪰 Gadfly · advisory

⚪ **Redundant z.ZodType<UnitPref> annotation diverges from auth.ts sibling pattern** _maintainability · flagged by 1 model_ - **`web/src/lib/gardens.ts:9`** — `const unitPrefSchema: z.ZodType<UnitPref> = z.enum(['metric', 'imperial'])` carries an explicit annotation that `auth.ts` (the sibling data layer) doesn't use; `z.enum` already infers the literal union. The annotation is harmless but breaks the local convention for no real benefit. Trivial. <sub>🪰 Gadfly · advisory</sub>
export const gardenSchema = z.object({ export const gardenSchema = z.object({
id: z.number(), id: z.number(),
2
@@ -51,13 +51,16 @@ export function useCreateGarden() {
} }
export interface GardenUpdate extends GardenInput { export interface GardenUpdate extends GardenInput {
id: number
version: number version: number
} }
export function useUpdateGarden(id: number) { export function useUpdateGarden() {
const qc = useQueryClient() const qc = useQueryClient()
return useMutation({ return useMutation({
mutationFn: async (input: GardenUpdate): Promise<Garden> => // id travels with the mutation variables (not the hook) so callers don't need
// a placeholder id before a garden is chosen.
Outdated
Review

🟠 Redundant full-list refetch after update mutation

performance · flagged by 1 model

  • web/src/lib/gardens.ts:49 / web/src/lib/gardens.ts:62Redundant full-list refetch after every create/update. useCreateGarden and useUpdateGarden both receive the fresh Garden row from the server (api.post / api.patch return the parsed object), but their onSuccess handlers ignore it and call qc.invalidateQueries({ queryKey: gardensKey }). That forces an extra GET /gardens round-trip even though the response already contains everything needed to update the cache. On slo…

🪰 Gadfly · advisory

🟠 **Redundant full-list refetch after update mutation** _performance · flagged by 1 model_ - `web/src/lib/gardens.ts:49` / `web/src/lib/gardens.ts:62` — **Redundant full-list refetch after every create/update.** `useCreateGarden` and `useUpdateGarden` both receive the fresh `Garden` row from the server (`api.post` / `api.patch` return the parsed object), but their `onSuccess` handlers ignore it and call `qc.invalidateQueries({ queryKey: gardensKey })`. That forces an extra `GET /gardens` round-trip even though the response already contains everything needed to update the cache. On slo… <sub>🪰 Gadfly · advisory</sub>
mutationFn: async ({ id, ...input }: GardenUpdate): Promise<Garden> =>
gardenSchema.parse(await api.patch(`/gardens/${id}`, input)), gardenSchema.parse(await api.patch(`/gardens/${id}`, input)),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }), onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),
}) })
+17 -4
View File
@@ -8,6 +8,15 @@ const CM_PER_INCH = 2.54
const CM_PER_METER = 100 const CM_PER_METER = 100
const INCHES_PER_FOOT = 12 const INCHES_PER_FOOT = 12
// Mirrors the server's garden dimension bounds (service/gardens.go): [1cm, 100m].
export const MIN_DIMENSION_CM = 1
Outdated
Review

cmFromFtIn/cmFromMeters exported with no current external consumers

maintainability · flagged by 1 model

  • web/src/lib/units.ts:12-19 — confirmed via repo-wide search that cmFromFtIn and cmFromMeters have no callers anywhere in the repo outside units.ts itself (only used internally by cmFromDisplay at line 23). Minor unnecessary public surface.

🪰 Gadfly · advisory

⚪ **cmFromFtIn/cmFromMeters exported with no current external consumers** _maintainability · flagged by 1 model_ - `web/src/lib/units.ts:12-19` — confirmed via repo-wide search that `cmFromFtIn` and `cmFromMeters` have no callers anywhere in the repo outside `units.ts` itself (only used internally by `cmFromDisplay` at line 23). Minor unnecessary public surface. <sub>🪰 Gadfly · advisory</sub>
export const MAX_DIMENSION_CM = 10_000
/** Whether a centimeter dimension is within the server's accepted range. */
export function isValidDimensionCm(cm: number): boolean {
return Number.isFinite(cm) && cm >= MIN_DIMENSION_CM && cm <= MAX_DIMENSION_CM
}
/** Feet (+ optional inches) → whole centimeters. */ /** Feet (+ optional inches) → whole centimeters. */
export function cmFromFtIn(feet: number, inches = 0): number { export function cmFromFtIn(feet: number, inches = 0): number {
return Math.round((feet * INCHES_PER_FOOT + inches) * CM_PER_INCH) return Math.round((feet * INCHES_PER_FOOT + inches) * CM_PER_INCH)
1
@@ -23,11 +32,15 @@ export function cmFromDisplay(value: number, unit: UnitPref): number {
return unit === 'imperial' ? cmFromFtIn(value) : cmFromMeters(value) return unit === 'imperial' ? cmFromFtIn(value) : cmFromMeters(value)
} }
/** Centimeters → a number in the given unit, for prefilling an input field. */ /** Centimeters → a number in the given unit, for prefilling an input field.
* Rounded so cm-quantization doesn't show through (e.g. 244cm shows as 8 ft,
* not 8.01): meters to the cm (2 dp), feet to 0.1 ft. */
export function displayFromCm(cm: number, unit: UnitPref): number { export function displayFromCm(cm: number, unit: UnitPref): number {
const value = unit === 'imperial' ? cm / CM_PER_INCH / INCHES_PER_FOOT : cm / CM_PER_METER if (unit === 'imperial') {
// Trim floating-point noise (e.g. 122cm → 4.0026ft → 4). const feet = cm / CM_PER_INCH / INCHES_PER_FOOT
return Math.round(value * 100) / 100 return Math.round(feet * 10) / 10
}
return Math.round((cm / CM_PER_METER) * 100) / 100
} }
/** Centimeters → a human string in the given unit (e.g. "1.22 m" or "4 0″"). */ /** Centimeters → a human string in the given unit (e.g. "1.22 m" or "4 0″"). */
+6 -6
View File
@@ -12,14 +12,14 @@ type Dialog = { kind: 'create' } | { kind: 'edit'; garden: Garden } | { kind: 'd
export function GardensPage() { export function GardensPage() {
const gardens = useGardens() const gardens = useGardens()
const [dialog, setDialog] = useState<Dialog>(null) const [dialog, setDialog] = useState<Dialog>(null)
const close = () => setDialog(null)
const hasGardens = !!gardens.data && gardens.data.length > 0
return ( return (
<section> <section>
<div className="flex items-center justify-between gap-4"> <div className="flex items-center justify-between gap-4">
Review

'New garden' visibility check duplicates grid condition with inconsistent isSuccess/data usage

maintainability · flagged by 1 model

  • web/src/pages/GardensPage.tsx:20 vs :39 — The "New garden" button visibility (gardens.data && gardens.data.length > 0) and the grid visibility (gardens.isSuccess && gardens.data.length > 0) check the same condition two different ways. Using gardens.isSuccess && in both keeps them aligned and avoids relying on data truthiness while pending. Trivial.

🪰 Gadfly · advisory

⚪ **'New garden' visibility check duplicates grid condition with inconsistent isSuccess/data usage** _maintainability · flagged by 1 model_ - **`web/src/pages/GardensPage.tsx:20` vs `:39`** — The "New garden" button visibility (`gardens.data && gardens.data.length > 0`) and the grid visibility (`gardens.isSuccess && gardens.data.length > 0`) check the same condition two different ways. Using `gardens.isSuccess &&` in both keeps them aligned and avoids relying on `data` truthiness while pending. Trivial. <sub>🪰 Gadfly · advisory</sub>
<h1 className="text-2xl font-semibold tracking-tight">Gardens</h1> <h1 className="text-2xl font-semibold tracking-tight">Gardens</h1>
{gardens.data && gardens.data.length > 0 && ( {hasGardens && <Button onClick={() => setDialog({ kind: 'create' })}>New garden</Button>}
<Button onClick={() => setDialog({ kind: 'create' })}>New garden</Button>
)}
</div> </div>
<div className="mt-6"> <div className="mt-6">
@@ -50,9 +50,9 @@ export function GardensPage() {
)} )}
</div> </div>
{dialog?.kind === 'create' && <GardenFormModal onClose={() => setDialog(null)} />} {dialog?.kind === 'create' && <GardenFormModal onClose={close} />}
Outdated
Review

🟡 Fresh lambda close handlers cause Modal effect churn

maintainability · flagged by 2 models

  • web/src/pages/GardensPage.tsx:53-55 — Three inline () => setDialog(null) lambdas recreated each render trigger the Modal useEffect churn noted above. Fix: define a stable const closeDialog = useCallback(() => setDialog(null), []) and pass it to all three modals.

🪰 Gadfly · advisory

🟡 **Fresh lambda close handlers cause Modal effect churn** _maintainability · flagged by 2 models_ - `web/src/pages/GardensPage.tsx:53-55` — Three inline `() => setDialog(null)` lambdas recreated each render trigger the `Modal` `useEffect` churn noted above. **Fix:** define a stable `const closeDialog = useCallback(() => setDialog(null), [])` and pass it to all three modals. <sub>🪰 Gadfly · advisory</sub>
{dialog?.kind === 'edit' && <GardenFormModal garden={dialog.garden} onClose={() => setDialog(null)} />} {dialog?.kind === 'edit' && <GardenFormModal garden={dialog.garden} onClose={close} />}
{dialog?.kind === 'delete' && <DeleteGardenModal garden={dialog.garden} onClose={() => setDialog(null)} />} {dialog?.kind === 'delete' && <DeleteGardenModal garden={dialog.garden} onClose={close} />}
</section> </section>
) )
} }