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 600 additions and 16 deletions
@@ -0,0 +1,42 @@
import { useState } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
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 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)
async function onConfirm() {
setError(null)
try {
await deletion.mutateAsync(garden.id)
onClose()
} catch (err) {
setError(errorMessage(err, 'Could not delete the garden.'))
}
}
return (
<Modal title="Delete garden" onClose={onClose} busy={deletion.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Delete <span className="font-medium text-fg">{garden.name}</span> and everything planned in it?
This can't be undone.
</p>
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={deletion.isPending}>
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 type="button" variant="danger" onClick={onConfirm} disabled={deletion.isPending}>
{deletion.isPending ? 'Deleting' : 'Delete'}
</Button>
</div>
</div>
</Modal>
)
}
+49
View File
@@ -0,0 +1,49 @@
import { Link } from '@tanstack/react-router'
import type { Garden } from '@/lib/gardens'
import { formatDimensions } from '@/lib/units'
/**
* One garden as a card: the body links into the editor (/gardens/:id); the
* footer has edit/delete actions (kept out of the link so they don't navigate).
*/
export function GardenCard({
garden,
onEdit,
onDelete,
}: {
garden: Garden
onEdit: () => void
onDelete: () => void
}) {
return (
<div className="flex flex-col rounded-xl border border-border bg-surface transition-colors hover:border-accent/50">
<Link
to="/gardens/$gardenId"
params={{ gardenId: String(garden.id) }}
className="flex-1 rounded-t-xl p-4 outline-none focus-visible:ring-2 focus-visible:ring-accent/40"
>
<h3 className="truncate font-semibold text-fg">{garden.name}</h3>
<p className="mt-1 text-sm text-muted">
{formatDimensions(garden.widthCm, garden.heightCm, garden.unitPref)}
</p>
{garden.notes && <p className="mt-2 line-clamp-2 text-sm text-muted">{garden.notes}</p>}
</Link>
<div className="flex justify-end gap-1 border-t border-border px-2 py-1.5">
<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"
onClick={onEdit}
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
</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"
onClick={onDelete}
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
</button>
</div>
</div>
)
}
@@ -0,0 +1,163 @@
import { useState, type FormEvent } from 'react'
import { Modal } from '@/components/ui/Modal'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { Select } from '@/components/ui/Select'
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,
isValidDimensionCm,
type UnitPref,
} from '@/lib/units'
const DEFAULT_METERS = 10 // matches the server's 10 m default
const unitOptions = [
{ value: 'metric', label: 'Metric (m)' },
{ value: 'imperial', label: 'Imperial (ft)' },
]
function dimString(cm: number | undefined, unit: UnitPref): string {
return cm === undefined ? String(DEFAULT_METERS) : String(displayFromCm(cm, unit))
}
/**
* Create (no garden) or edit (garden given) form. Dimensions are entered in the
* selected unit and converted to centimeters for the API; switching units
* converts the current values so the physical size is preserved. A 409 rebases
Review

🟡 Dummy id 0 passed to useUpdateGarden during creation is a leaky abstraction

maintainability · flagged by 2 models

  • web/src/components/gardens/GardenFormModal.tsx:32useUpdateGarden(garden?.id ?? 0) passes a dummy 0 during creation. The hook is never called, but the ?? 0 is a leaky abstraction—future readers must infer it is safe. Fix: make useUpdateGarden accept id?: number and return a no-op mutation when absent, removing the magic placeholder.

🪰 Gadfly · advisory

🟡 **Dummy id 0 passed to useUpdateGarden during creation is a leaky abstraction** _maintainability · flagged by 2 models_ - `web/src/components/gardens/GardenFormModal.tsx:32` — `useUpdateGarden(garden?.id ?? 0)` passes a dummy `0` during creation. The hook is never called, but the `?? 0` is a leaky abstraction—future readers must infer it is safe. **Fix:** make `useUpdateGarden` accept `id?: number` and return a no-op mutation when absent, removing the magic placeholder. <sub>🪰 Gadfly · advisory</sub>
* the form onto the server's fresh row.
*/
export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) {
const isEdit = !!garden
const create = useCreateGarden()
const update = useUpdateGarden()
const pending = create.isPending || update.isPending
const [name, setName] = useState(garden?.name ?? '')
const [unit, setUnit] = useState<UnitPref>(garden?.unitPref ?? 'metric')
const [width, setWidth] = useState(() => dimString(garden?.widthCm, garden?.unitPref ?? 'metric'))
const [height, setHeight] = useState(() => dimString(garden?.heightCm, garden?.unitPref ?? 'metric'))
Review

🟡 Unit-switching conversion is lossy and contradicts preserve-physical-size claim

error-handling, maintainability · flagged by 2 models

  • web/src/components/gardens/GardenFormModal.tsx:44-52 — Unit switching goes through cmFromDisplayMath.rounddisplayFromCm. This is lossy: repeated toggles can drift. The comment claims the physical size is preserved, which is only true to ±1 cm. Fix: keep a hidden cm state (or ref) for the true physical size and derive the display string from it, eliminating round-trip noise.

🪰 Gadfly · advisory

🟡 **Unit-switching conversion is lossy and contradicts preserve-physical-size claim** _error-handling, maintainability · flagged by 2 models_ - `web/src/components/gardens/GardenFormModal.tsx:44-52` — Unit switching goes through `cmFromDisplay` → `Math.round` → `displayFromCm`. This is lossy: repeated toggles can drift. The comment claims the physical size is preserved, which is only true to ±1 cm. **Fix:** keep a hidden `cm` state (or ref) for the true physical size and derive the display string from it, eliminating round-trip noise. <sub>🪰 Gadfly · advisory</sub>
const [notes, setNotes] = useState(garden?.notes ?? '')
const [version, setVersion] = useState(garden?.version ?? 0)
const [conflict, setConflict] = useState<string | null>(null)
const [formError, setFormError] = useState<string | null>(null)
function changeUnit(next: UnitPref) {
const convert = (s: string) => {
const v = parseFloat(s)
return Number.isFinite(v) ? String(displayFromCm(cmFromDisplay(v, unit), next)) : s
}
setWidth(convert(width))
setHeight(convert(height))
setUnit(next)
}
Review

🟡 Client validates dimension positivity before unit conversion, missing server's [1cm,100m] bounds after rounding

correctness · flagged by 1 model

  • web/src/components/gardens/GardenFormModal.tsx:59-64 vs. internal/service/gardens.go:16-18,146-148 — Client-side validation only checks w <= 0 / h <= 0 in the display unit before conversion, while the server enforces [minGardenCM=1, maxGardenCM=10_000] (1 cm–100 m) on the converted value via validDimensionCM. A small value (e.g. 0.001 m) passes the client check but rounds to 0 cm through cmFromMeters/cmFromFtIn (units.ts:12-19) and fails server validation; a value like `5…

🪰 Gadfly · advisory

🟡 **Client validates dimension positivity before unit conversion, missing server's [1cm,100m] bounds after rounding** _correctness · flagged by 1 model_ - `web/src/components/gardens/GardenFormModal.tsx:59-64` vs. `internal/service/gardens.go:16-18,146-148` — Client-side validation only checks `w <= 0` / `h <= 0` in the display unit before conversion, while the server enforces `[minGardenCM=1, maxGardenCM=10_000]` (1 cm–100 m) on the converted value via `validDimensionCM`. A small value (e.g. `0.001` m) passes the client check but rounds to `0` cm through `cmFromMeters`/`cmFromFtIn` (`units.ts:12-19`) and fails server validation; a value like `5… <sub>🪰 Gadfly · advisory</sub>
async function onSubmit(e: FormEvent) {
e.preventDefault()
setFormError(null)
setConflict(null)
if (!name.trim()) {
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>
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, heightCm, unitPref: unit, notes: notes.trim() }
try {
if (isEdit) {
await update.mutateAsync({ id: garden.id, ...input, version })
} else {
await create.mutateAsync(input)
}
onClose()
} catch (err) {
const current = conflictGarden(err)
Review

🟡 409 rebase duplicates dimString helper instead of reusing it

maintainability · flagged by 1 model

  • web/src/components/gardens/GardenFormModal.tsx:89-90 — The 409 rebase path calls String(displayFromCm(current.widthCm, current.unitPref)) directly, duplicating what the dimString helper (lines 19-21) already does. dimString(current.widthCm, current.unitPref) would read better and keep the "cm → input string" rule in one place (dimString accepts number | undefined, so a number arg is fine). Minor.

🪰 Gadfly · advisory

🟡 **409 rebase duplicates dimString helper instead of reusing it** _maintainability · flagged by 1 model_ - **`web/src/components/gardens/GardenFormModal.tsx:89-90`** — The 409 rebase path calls `String(displayFromCm(current.widthCm, current.unitPref))` directly, duplicating what the `dimString` helper (lines 19-21) already does. `dimString(current.widthCm, current.unitPref)` would read better and keep the "cm → input string" rule in one place (`dimString` accepts `number | undefined`, so a `number` arg is fine). Minor. <sub>🪰 Gadfly · advisory</sub>
if (current) {
// Someone else changed this garden: rebase the form onto the fresh row so
// a re-save applies against the current version.
setVersion(current.version)
setName(current.name)
setUnit(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
}
setFormError(errorMessage(err, isEdit ? 'Could not save changes.' : 'Could not create the garden.'))
}
}
const unitLabel = dimensionUnitLabel(unit)
return (
<Modal title={isEdit ? 'Edit garden' : 'New garden'} onClose={onClose} busy={pending}>
<form onSubmit={onSubmit} className="flex flex-col gap-3">
{conflict && <Alert tone="info">{conflict}</Alert>}
<TextField label="Name" name="name" required value={name} onChange={(e) => setName(e.target.value)} />
<Select
label="Units"
name="unitPref"
value={unit}
onChange={(e) => changeUnit(e.target.value as UnitPref)}
options={unitOptions}
/>
<div className="grid grid-cols-2 gap-3">
<TextField
label={`Width (${unitLabel})`}
name="width"
type="number"
inputMode="decimal"
step="any"
min="0"
required
value={width}
onChange={(e) => setWidth(e.target.value)}
/>
<TextField
label={`Height (${unitLabel})`}
name="height"
type="number"
inputMode="decimal"
step="any"
min="0"
required
value={height}
onChange={(e) => setHeight(e.target.value)}
/>
</div>
<TextArea label="Notes" name="notes" rows={2} value={notes} onChange={(e) => setNotes(e.target.value)} />
{formError && <Alert>{formError}</Alert>}
<div className="mt-1 flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={pending}>
Cancel
</Button>
<Button type="submit" disabled={pending}>
{pending ? 'Saving…' : isEdit ? 'Save' : 'Create garden'}
</Button>
</div>
</form>
</Modal>
)
}
+2 -1
View File
@@ -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,
)
}
+58
View File
@@ -0,0 +1,58 @@
import { useEffect, useRef, type ReactNode } from 'react'
/**
* 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
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
}) {
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(() => {
cardRef.current?.focus()
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' && !busyRef.current) onCloseRef.current()
}
document.addEventListener('keydown', onKey)
return () => document.removeEventListener('keydown', onKey)
}, [])
return (
<div
className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 p-4 sm:items-center"
onMouseDown={(e) => {
if (e.target === e.currentTarget && !busy) onClose()
}}
>
<div
ref={cardRef}
role="dialog"
aria-modal="true"
aria-label={title}
tabIndex={-1}
className="w-full max-w-md rounded-xl border border-border bg-surface p-6 shadow-lg outline-none"
>
<h2 className="text-lg font-semibold tracking-tight text-fg">{title}</h2>
<div className="mt-4">{children}</div>
</div>
</div>
)
}
+29
View File
@@ -0,0 +1,29 @@
import { forwardRef, type SelectHTMLAttributes } from 'react'
import { cn } from '@/lib/cn'
import { fieldControlClass, useFieldId } from './field'
interface SelectProps extends SelectHTMLAttributes<HTMLSelectElement> {
label: string
options: { value: string; label: string }[]
}
export const Select = forwardRef<HTMLSelectElement, SelectProps>(function Select(
{ label, options, id, name, className, ...props },
ref,
) {
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>
return (
<div className="flex flex-col gap-1.5">
<label htmlFor={fieldId} className="text-sm font-medium text-fg">
{label}
</label>
<select ref={ref} id={fieldId} name={name} className={cn(fieldControlClass, className)} {...props}>
{options.map((o) => (
<option key={o.value} value={o.value}>
{o.label}
</option>
))}
Review

🟡 Field chrome (styling + label/id-fallback pattern) duplicated across Select, TextArea, and TextField — extract a shared FieldShell/fieldClasses

maintainability · flagged by 1 model

  • web/src/components/ui/Select.tsx:25 (with TextArea.tsx:25 and TextField.tsx:29) — The field chrome styling string ('w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg', '...focus:border-accent focus:ring-2 focus:ring-accent/30 ... disabled:opacity-60') is copy-pasted across all three labelled-field components. The surrounding label+id-fallback pattern (generatedId = useId(); fieldId = id ?? name ?? generatedId, the wrapping `<div className="flex flex-co…

🪰 Gadfly · advisory

🟡 **Field chrome (styling + label/id-fallback pattern) duplicated across Select, TextArea, and TextField — extract a shared FieldShell/fieldClasses** _maintainability · flagged by 1 model_ - **`web/src/components/ui/Select.tsx:25`** (with `TextArea.tsx:25` and `TextField.tsx:29`) — The field chrome styling string (`'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg', '...focus:border-accent focus:ring-2 focus:ring-accent/30 ... disabled:opacity-60'`) is copy-pasted across all three labelled-field components. The surrounding label+id-fallback pattern (`generatedId = useId(); fieldId = id ?? name ?? generatedId`, the wrapping `<div className="flex flex-co… <sub>🪰 Gadfly · advisory</sub>
</select>
</div>
)
})
+22
View File
@@ -0,0 +1,22 @@
import { forwardRef, type TextareaHTMLAttributes } from 'react'
import { cn } from '@/lib/cn'
import { fieldControlClass, useFieldId } from './field'
interface TextAreaProps extends TextareaHTMLAttributes<HTMLTextAreaElement> {
label: string
}
export const TextArea = forwardRef<HTMLTextAreaElement, TextAreaProps>(function TextArea(
{ label, id, name, className, ...props },
ref,
) {
const fieldId = useFieldId(id, name)
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">
<label htmlFor={fieldId} className="text-sm font-medium text-fg">
{label}
</label>
<textarea ref={ref} id={fieldId} name={name} className={cn(fieldControlClass, className)} {...props} />
</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 { fieldControlClass, useFieldId } from './field'
interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> {
label: 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(
{ label, hint, id, name, className, ...props },
ref,
) {
const generatedId = useId()
const inputId = id ?? name ?? generatedId
const inputId = useFieldId(id, name)
const hintId = hint ? `${inputId}-hint` : undefined
return (
<div className="flex flex-col gap-1.5">
@@ -26,13 +23,7 @@ export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function T
id={inputId}
name={name}
aria-describedby={hintId}
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,
)}
className={cn(fieldControlClass, className)}
{...props}
/>
{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'
+88
View File
@@ -0,0 +1,88 @@
// Gardens data layer: zod-validated shapes for /api/v1/gardens plus the
// react-query hooks the /gardens page uses.
import { queryOptions, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
import { z } from 'zod'
import { ApiError, api } from './api'
import type { UnitPref } from './units'
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({
id: z.number(),
ownerId: z.number(),
name: z.string(),
widthCm: z.number(),
Review

🟠 Garden schema does not enforce positive dimensions, allowing zero/negative widthCm and heightCm from API

error-handling · flagged by 1 model

  • web/src/lib/gardens.ts:15-16widthCm: z.number() and heightCm: z.number() accept zero and negative values. The form guards against this on write, but the schema allows invalid server data to propagate to the UI and into formatDimensions. Defensive fix: use z.number().positive() for both.

🪰 Gadfly · advisory

🟠 **Garden schema does not enforce positive dimensions, allowing zero/negative widthCm and heightCm from API** _error-handling · flagged by 1 model_ * **web/src/lib/gardens.ts:15-16** — `widthCm: z.number()` and `heightCm: z.number()` accept zero and negative values. The form guards against this on write, but the schema allows invalid server data to propagate to the UI and into `formatDimensions`. Defensive fix: use `z.number().positive()` for both. <sub>🪰 Gadfly · advisory</sub>
heightCm: z.number(),
unitPref: unitPrefSchema,
notes: z.string(),
version: z.number(),
createdAt: z.string(),
updatedAt: z.string(),
})
export type Garden = z.infer<typeof gardenSchema>
const gardensKey = ['gardens'] as const
export const gardensQueryOptions = queryOptions({
queryKey: gardensKey,
queryFn: async (): Promise<Garden[]> => z.array(gardenSchema).parse(await api.get('/gardens')),
})
export function useGardens() {
return useQuery(gardensQueryOptions)
}
export interface GardenInput {
name: string
widthCm: number
heightCm: number
unitPref: UnitPref
notes: string
}
export function useCreateGarden() {
const qc = useQueryClient()
return useMutation({
Review

🟠 Redundant full-list refetch after create mutation

performance · flagged by 2 models

  • 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 create mutation** _performance · flagged by 2 models_ - `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 (input: GardenInput): Promise<Garden> =>
gardenSchema.parse(await api.post('/gardens', input)),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),
})
}
export interface GardenUpdate extends GardenInput {
id: number
version: number
}
export function useUpdateGarden() {
const qc = useQueryClient()
return useMutation({
// 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)),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),
})
}
export function useDeleteGarden() {
const qc = useQueryClient()
return useMutation({
mutationFn: (id: number) => api.delete(`/gardens/${id}`),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),
})
}
/**
* If err is a 409 version conflict, return the fresh server row it carries
* (under `current`), so a form can rebase onto it; otherwise null.
*/
export function conflictGarden(err: unknown): Garden | null {
if (err instanceof ApiError && err.isConflict && err.body && typeof err.body === 'object') {
const current = (err.body as { current?: unknown }).current
const parsed = gardenSchema.safeParse(current)
if (parsed.success) return parsed.data
}
return null
}
+70
View File
@@ -0,0 +1,70 @@
// Unit conversion + display for garden dimensions. The API and DB are metric
// only (centimeters); imperial is purely a display/entry concern here. A minimal
// helper set for #8 — #9's geometry lib consolidates/extends it.
export type UnitPref = 'metric' | 'imperial'
const CM_PER_INCH = 2.54
const CM_PER_METER = 100
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. */
export function cmFromFtIn(feet: number, inches = 0): number {
return Math.round((feet * INCHES_PER_FOOT + inches) * CM_PER_INCH)
}
/** Meters → whole centimeters. */
export function cmFromMeters(meters: number): number {
return Math.round(meters * CM_PER_METER)
Review

🔴 displayFromCm 2-decimal rounding causes systematic imperial round-trip errors (8 ft becomes 8.01 ft)

correctness · flagged by 1 model

  • web/src/lib/units.ts:27-31 / web/src/components/gardens/GardenFormModal.tsx:71-72displayFromCm rounds to 2 decimal places, causing systematic round-trip errors for common imperial dimensions. For example, 8 ft → cmFromFtIn(8) = 244 cm → displayFromCm(244, 'imperial') = 8.01 ft. A user who creates a garden as exactly 8 ft will later see 8.01 ft in the edit form. The same happens for 10 ft → 10.01 ft, 12 ft → 12.01 ft, etc. The PR notes that 4 ft happens to round-trip cleanl…

🪰 Gadfly · advisory

🔴 **displayFromCm 2-decimal rounding causes systematic imperial round-trip errors (8 ft becomes 8.01 ft)** _correctness · flagged by 1 model_ - **`web/src/lib/units.ts:27-31`** / **`web/src/components/gardens/GardenFormModal.tsx:71-72`** — `displayFromCm` rounds to 2 decimal places, causing systematic round-trip errors for common imperial dimensions. For example, 8 ft → `cmFromFtIn(8)` = 244 cm → `displayFromCm(244, 'imperial')` = 8.01 ft. A user who creates a garden as exactly 8 ft will later see 8.01 ft in the edit form. The same happens for 10 ft → 10.01 ft, 12 ft → 12.01 ft, etc. The PR notes that 4 ft happens to round-trip cleanl… <sub>🪰 Gadfly · advisory</sub>
}
/** A value typed in the given unit (meters, or feet) → centimeters. */
export function cmFromDisplay(value: number, unit: UnitPref): number {
return unit === 'imperial' ? cmFromFtIn(value) : cmFromMeters(value)
}
/** 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 {
if (unit === 'imperial') {
const feet = cm / CM_PER_INCH / INCHES_PER_FOOT
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″"). */
export function formatCm(cm: number, unit: UnitPref): string {
if (unit === 'imperial') {
const totalInches = cm / CM_PER_INCH
let feet = Math.floor(totalInches / INCHES_PER_FOOT)
let inches = Math.round(totalInches - feet * INCHES_PER_FOOT)
if (inches === INCHES_PER_FOOT) {
feet += 1
inches = 0
}
return `${feet} ${inches}`
}
const meters = Math.round((cm / CM_PER_METER) * 100) / 100
return `${meters} m`
}
/** Centimeters width × height → a human string in the given unit. */
export function formatDimensions(widthCm: number, heightCm: number, unit: UnitPref): string {
return `${formatCm(widthCm, unit)} × ${formatCm(heightCm, unit)}`
}
/** The input-field label for a single dimension in the given unit. */
export function dimensionUnitLabel(unit: UnitPref): string {
return unit === 'imperial' ? 'ft' : 'm'
}
+55 -2
View File
@@ -1,5 +1,58 @@
import { PageStub } from '@/components/PageStub'
import { useState } from 'react'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { DeleteGardenModal } from '@/components/gardens/DeleteGardenModal'
import { GardenCard } from '@/components/gardens/GardenCard'
import { GardenFormModal } from '@/components/gardens/GardenFormModal'
import { useGardens, type Garden } from '@/lib/gardens'
// Which modal is open, if any. `edit`/`delete` carry the target garden.
type Dialog = { kind: 'create' } | { kind: 'edit'; garden: Garden } | { kind: 'delete'; garden: Garden } | null
export function GardensPage() {
return <PageStub title="Gardens">The garden list and create flow arrive in issue #8.</PageStub>
const gardens = useGardens()
const [dialog, setDialog] = useState<Dialog>(null)
const close = () => setDialog(null)
const hasGardens = !!gardens.data && gardens.data.length > 0
return (
<section>
<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>
{hasGardens && <Button onClick={() => setDialog({ kind: 'create' })}>New garden</Button>}
</div>
<div className="mt-6">
{gardens.isPending && <p className="text-sm text-muted">Loading your gardens</p>}
{gardens.isError && <Alert>Could not load your gardens. Please refresh.</Alert>}
{gardens.isSuccess && gardens.data.length === 0 && (
<div className="rounded-xl border border-dashed border-border p-10 text-center">
<p className="text-fg">No gardens yet.</p>
<p className="mt-1 text-sm text-muted">Create your first garden to start planning.</p>
<Button className="mt-4" onClick={() => setDialog({ kind: 'create' })}>
Create your first garden
</Button>
</div>
)}
{gardens.isSuccess && gardens.data.length > 0 && (
<div className="grid grid-cols-1 gap-4 sm:grid-cols-2 lg:grid-cols-3">
{gardens.data.map((g) => (
<GardenCard
key={g.id}
garden={g}
onEdit={() => setDialog({ kind: 'edit', garden: g })}
onDelete={() => setDialog({ kind: 'delete', garden: g })}
/>
))}
</div>
)}
</div>
{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={close} />}
{dialog?.kind === 'delete' && <DeleteGardenModal garden={dialog.garden} onClose={close} />}
</section>
)
}