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
Owner

Closes #8. Phase 2, step 2 (final) of the tracking epic #20Phase 2 complete. Consumes the #7 API and the #6 auth guard/nav.

What's here

  • lib/units.ts — cm ↔ meters/feet conversion + formatCm/formatDimensions (metric m, imperial ft/in). Deliberately minimal; #9's geometry lib consolidates.
  • lib/gardens.ts — zod gardenSchema, useGardens/useCreateGarden/useUpdateGarden/useDeleteGarden (mutations invalidate the list), and conflictGarden() to pull the fresh row out of a 409.
  • GardensPage — loading/empty/error states; empty state prompts a first garden; responsive card grid (single column on phones, up to 3 wide).
  • GardenCard — name, dimensions formatted per the garden's own unit_pref, notes preview; the body links into /gardens/:id with edit/delete kept out of the link.
  • GardenFormModal — create/edit with a unit selector; dimensions are entered in the chosen unit and converted to cm, and switching units converts the current values so the physical size is preserved. A 409 rebases the form onto the server's fresh row and tells the user it changed elsewhere.
  • DeleteGardenModal — confirmation before removing.
  • ui/Modal (Escape / backdrop close), TextArea, Select.

Verification (real browser, embedded binary)

  • Create → the card appears without reload.
  • Edit → persists (version → 2); the form is prefilled from the stored cm converted back to the garden's unit (122 cm → 4).
  • Delete → confirms then removes → empty state returns.
  • An imperial 4 ft × 8 ft garden stores 122 × 244 cm and displays "4′ 0″ × 8′ 0″" (acceptance criterion).
  • Responsive grid collapses to one column on phone widths. npx tsc --noEmit clean; vite build succeeds. (Screenshot shared in-session.)

Out of scope

  • The canvas/editor (#9, #11) — cards link to /gardens/:id, still a stub. Sharing UI / role badges (#17).

🤖 Generated with Claude Code

Closes #8. Phase 2, step 2 (final) of the tracking epic #20 — **Phase 2 complete**. Consumes the #7 API and the #6 auth guard/nav. ## What's here - **`lib/units.ts`** — cm ↔ meters/feet conversion + `formatCm`/`formatDimensions` (metric `m`, imperial `ft/in`). Deliberately minimal; #9's geometry lib consolidates. - **`lib/gardens.ts`** — zod `gardenSchema`, `useGardens`/`useCreateGarden`/`useUpdateGarden`/`useDeleteGarden` (mutations invalidate the list), and `conflictGarden()` to pull the fresh row out of a 409. - **`GardensPage`** — loading/empty/error states; empty state prompts a first garden; responsive card grid (single column on phones, up to 3 wide). - **`GardenCard`** — name, dimensions formatted per the garden's own `unit_pref`, notes preview; the body links into `/gardens/:id` with edit/delete kept out of the link. - **`GardenFormModal`** — create/edit with a unit selector; dimensions are entered in the chosen unit and converted to cm, and switching units converts the current values so the physical size is preserved. A **409 rebases the form onto the server's fresh row** and tells the user it changed elsewhere. - **`DeleteGardenModal`** — confirmation before removing. - **`ui/`** — `Modal` (Escape / backdrop close), `TextArea`, `Select`. ## Verification (real browser, embedded binary) - Create → the card **appears without reload**. - Edit → **persists** (version → 2); the form is prefilled from the stored cm converted back to the garden's unit (122 cm → `4`). - Delete → **confirms then removes** → empty state returns. - An imperial **4 ft × 8 ft** garden **stores 122 × 244 cm** and displays **"4′ 0″ × 8′ 0″"** (acceptance criterion). - Responsive grid collapses to one column on phone widths. `npx tsc --noEmit` clean; `vite build` succeeds. (Screenshot shared in-session.) ## Out of scope - The canvas/editor (#9, #11) — cards link to `/gardens/:id`, still a stub. Sharing UI / role badges (#17). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-18 22:49:03 +00:00
Add gardens UI: list page + create/edit/delete (#8)
Build image / build-and-push (push) Successful in 13s
Gadfly review (reusable) / review (pull_request) Successful in 7m21s
Adversarial Review (Gadfly) / review (pull_request) Successful in 7m21s
e0a1522608
The /gardens page is now home base, backed by the #7 API.

- lib/units.ts: cm <-> meters/feet conversion + formatCm/formatDimensions
  (metric "m", imperial "ft/in"). Minimal for #8; #9's geometry lib
  consolidates.
- lib/gardens.ts: zod gardenSchema + useGardens/useCreateGarden/
  useUpdateGarden/useDeleteGarden (mutations invalidate the list) and
  conflictGarden() to pull the fresh row out of a 409.
- GardensPage: loading/empty/error states; empty state prompts a first
  garden; responsive card grid (single column on phones).
- GardenCard: name, dimensions formatted per the garden's unit, notes
  preview; body links into /gardens/:id, edit/delete kept out of the link.
- GardenFormModal: create/edit with a unit selector; dimensions are entered
  in the chosen unit and converted to cm (switching units converts the
  current values). A 409 rebases the form onto the server's fresh row.
- DeleteGardenModal: confirmation before removing.
- ui/: Modal (Escape/backdrop close), TextArea, Select.

Verified in a real browser against the embedded binary: create appears
without reload; edit persists (version 2), form prefilled from stored cm
converted back to the garden's unit; delete confirms then removes -> empty
state; an imperial 4 ft x 8 ft garden stores 122 x 244 cm and shows
"4' 0" x 8' 0"". tsc --noEmit clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-18 22:56:23Z

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Blocking issues found

opencode/glm-5.2:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • ⚠️ performance — could not complete
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-18 22:56:23Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Blocking issues found #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ⚠️ **performance** — could not complete - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-18 22:56:24 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 22 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 22 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +7,4 @@
/** Confirmation dialog for deleting a garden (and everything in it). */
export function DeleteGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const del = useDeleteGarden()

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>
@@ -0,0 +30,4 @@
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={del.isPending}>
Cancel

🟡 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>
@@ -0,0 +29,4 @@
{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

🟠 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>
@@ -0,0 +36,4 @@
>
Edit
</button>
<button

🟠 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>
@@ -0,0 +29,4 @@
export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) {
const isEdit = !!garden
const create = useCreateGarden()
const update = useUpdateGarden(garden?.id ?? 0)

🟡 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>
@@ -0,0 +41,4 @@
const [conflict, setConflict] = useState<string | null>(null)
const [formError, setFormError] = useState<string | null>(null)
function changeUnit(next: UnitPref) {

🟡 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>
@@ -0,0 +56,4 @@
setFormError(null)
setConflict(null)
const w = parseFloat(width)

🟡 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>
@@ -0,0 +63,4 @@
return
}
const input = {

🟠 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>
@@ -0,0 +86,4 @@
setVersion(current.version)
setName(current.name)
setUnit(current.unitPref)
setWidth(String(displayFromCm(current.widthCm, current.unitPref)))

🟡 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>
@@ -0,0 +14,4 @@
onClose: () => void
children: ReactNode
}) {
const cardRef = useRef<HTMLDivElement>(null)

🔴 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>
@@ -0,0 +28,4 @@
return (
<div
className="fixed inset-0 z-50 flex items-end justify-center bg-black/40 p-4 sm:items-center"
onMouseDown={(e) => {

🔴 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>
@@ -0,0 +11,4 @@
{ label, options, id, name, className, ...props },
ref,
) {
const generatedId = useId()

🟡 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>
@@ -0,0 +22,4 @@
ref={ref}
id={fieldId}
name={name}
className={cn(

🟡 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>
@@ -0,0 +11,4 @@
{ label, id, name, className, ...props },
ref,
) {
const generatedId = useId()

🟡 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>
@@ -0,0 +6,4 @@
import { ApiError, api } from './api'
import type { UnitPref } from './units'
const unitPrefSchema: z.ZodType<UnitPref> = z.enum(['metric', 'imperial'])

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>
@@ -0,0 +12,4 @@
id: z.number(),
ownerId: z.number(),
name: z.string(),
widthCm: z.number(),

🟠 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>
@@ -0,0 +43,4 @@
export function useCreateGarden() {
const qc = useQueryClient()
return useMutation({

🟠 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>
@@ -0,0 +59,4 @@
return useMutation({
mutationFn: async (input: GardenUpdate): Promise<Garden> =>
gardenSchema.parse(await api.patch(`/gardens/${id}`, input)),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),

🟠 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>
@@ -0,0 +9,4 @@
const INCHES_PER_FOOT = 12
/** Feet (+ optional inches) → whole centimeters. */
export function cmFromFtIn(feet: number, inches = 0): number {

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>
@@ -0,0 +24,4 @@
}
/** Centimeters → a number in the given unit, for prefilling an input field. */
export function displayFromCm(cm: number, unit: UnitPref): number {

🔴 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>
@@ -5,0 +17,4 @@
<section>
<div className="flex items-center justify-between gap-4">
<h1 className="text-2xl font-semibold tracking-tight">Gardens</h1>
{gardens.data && gardens.data.length > 0 && (

'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>
@@ -5,0 +50,4 @@
)}
</div>
{dialog?.kind === 'create' && <GardenFormModal onClose={() => setDialog(null)} />}

🟡 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>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 23 findings (8 with multi-model agreement)

Finding Where Models Lens
🔴 Modal useEffect re-runs on every onClose change, repeatedly calling focus() and stealing focus from active inputs during parent re-renders web/src/components/ui/Modal.tsx:17 5/5 correctness, error-handling, maintainability, performance
🟠 Raw elements bypass shared Button component styles and accessibility web/src/components/gardens/GardenCard.tsx:32 2/5 maintainability
🟠 Form validates parsed float > 0 but small positive values round to 0 cm in API web/src/components/gardens/GardenFormModal.tsx:66 2/5 correctness, error-handling
🟠 Redundant full-list refetch after create mutation web/src/lib/gardens.ts:46 2/5 performance
🟡 Dummy id 0 passed to useUpdateGarden during creation is a leaky abstraction web/src/components/gardens/GardenFormModal.tsx:32 2/5 maintainability
🟡 Unit-switching conversion is lossy and contradicts preserve-physical-size claim web/src/components/gardens/GardenFormModal.tsx:44 2/5 error-handling, maintainability
🟡 Copied useId fallback logic from TextField instead of extracting shared hook web/src/components/ui/TextArea.tsx:14 2/5 maintainability
🟡 Fresh lambda close handlers cause Modal effect churn web/src/pages/GardensPage.tsx:53 2/5 maintainability
15 single-model findings (lower confidence)
Finding Where Model Lens
🔴 Modal dismissible during pending mutation causing lost error feedback web/src/components/ui/Modal.tsx:31 opencode/kimi-k2.6:cloud correctness
🔴 displayFromCm 2-decimal rounding causes systematic imperial round-trip errors (8 ft becomes 8.01 ft) web/src/lib/units.ts:27 kimi-k2.6:cloud correctness
🟠 Raw elements bypass shared Button component styles and accessibility web/src/components/gardens/GardenCard.tsx:39 opencode/kimi-k2.6:cloud maintainability
🟠 Garden schema does not enforce positive dimensions, allowing zero/negative widthCm and heightCm from API web/src/lib/gardens.ts:15 kimi-k2.6:cloud error-handling
🟠 Redundant full-list refetch after update mutation web/src/lib/gardens.ts:62 kimi-k2.6:cloud performance
🟡 Destructive-action button hardcodes red classes instead of adding a danger variant to buttonClasses web/src/components/gardens/DeleteGardenModal.tsx:33 claude-code/sonnet maintainability
🟡 Client validates dimension positivity before unit conversion, missing server's [1cm,100m] bounds after rounding web/src/components/gardens/GardenFormModal.tsx:59 claude-code/sonnet correctness
🟡 409 rebase duplicates dimString helper instead of reusing it web/src/components/gardens/GardenFormModal.tsx:89 glm-5.2:cloud maintainability
🟡 No destructive/danger variant; red styling hand-rolled inconsistently in DeleteGardenModal and GardenCard web/src/components/ui/Button.tsx:4 glm-5.2:cloud maintainability
🟡 Copied useId fallback logic from TextField instead of extracting shared hook web/src/components/ui/Select.tsx:14 opencode/kimi-k2.6:cloud maintainability
🟡 Field chrome (styling + label/id-fallback pattern) duplicated across Select, TextArea, and TextField — extract a shared FieldShell/fieldClasses web/src/components/ui/Select.tsx:25 opencode/glm-5.2:cloud maintainability
Variable named del is unnecessarily terse web/src/components/gardens/DeleteGardenModal.tsx:10 kimi-k2.6:cloud maintainability
Redundant z.ZodType annotation diverges from auth.ts sibling pattern web/src/lib/gardens.ts:9 glm-5.2:cloud maintainability
cmFromFtIn/cmFromMeters exported with no current external consumers web/src/lib/units.ts:12 claude-code/sonnet maintainability
'New garden' visibility check duplicates grid condition with inconsistent isSuccess/data usage web/src/pages/GardensPage.tsx:20 glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

I checked how untrusted data flows through this diff with a security lens:

  • API layer (web/src/lib/api.ts, unchanged) uses credentials: 'same-origin' and relies on an HttpOnly session cookie — no tokens are exposed to JS, and this file isn't touched by the diff.
  • Route guard: web/src/router.tsx:82-87 confirms /gardens is behind requireAuth, which resolves /auth/me before rendering GardensPage — consistent with the PR's claim of consuming the #6 auth guard.
  • Server response validation: gardenSchema (zod) validates every API response before use (lib/gardens.ts:17-27,39,47,58) and the 409-conflict payload is also safeParse'd (lib/gardens.ts:79-84) rather than trusted blindly — no unsafe deserialization.
  • Rendering: all user/server-provided strings (garden.name, garden.notes, error messages) are rendered via JSX interpolation, not dangerouslySetInnerHTML — grepped the whole web/src tree for dangerouslySetInnerHTML/__html and found zero matches, so no injected-HTML/XSS vector here.
  • IDs used in requests/links (GardenCard.tsx:15-16, GardenFormModal.tsx PATCH URL, DeleteGardenModal.tsx delete call) all originate from the zod-validated Garden.id: z.number(), not from raw user input — no path/template injection surface.
  • Client-side numeric validation (GardenFormModal.tsx:56-60) rejects non-finite/non-positive width/height before submit; this is a UX guard, not a security boundary — actual enforcement is server-side (out of scope, backend from #7).
  • Authorization for who can edit/delete a given garden is enforced by the backend (out of scope for this frontend-only diff); the frontend does not attempt and does not need to duplicate that check.

Nothing in this diff introduces an authn/authz gap, injection point, SSRF, unsafe deserialization, secret leakage, or insecure default.

🎯 Correctness — Minor issues

Both findings hold up against the actual source, with one correction needed on finding 2's error-message detail.

VERDICT: Minor issues

  • web/src/components/ui/Modal.tsx:19-26 — The Escape-key/focus useEffect depends on [onClose] (line 26), but every caller passes a fresh inline arrow: GardensPage.tsx:53-55 passes () => setDialog(null) for all three dialog kinds. queryClient.ts:6-10 confirms refetchOnWindowFocus: false is set but refetchOnReconnect/background refetches are left at their TanStack Query defaults (enabled), and useGardens() subscribes to the gardens query used on the very page that renders the modal. A background refetch (reconnect, refocus elsewhere, or any other unrelated state change on GardensPage) re-renders the page, creates a new onClose reference, and re-fires the effect — including cardRef.current?.focus() on line 24 — which steals focus from whatever field the user is actively editing inside GardenFormModal/DeleteGardenModal, since those inputs render as descendants of cardRef's div. Fix: run the focus-on-mount behavior once (empty dependency array) and keep the Escape listener in a separate effect, or have callers memoize onClose.

  • 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 500 m also passes the client check but exceeds the 100 m server cap. Correction to the original claim: the resulting error is not the component's generic fallback text — internal/api/errors.go:45-46 maps ErrInvalidInput to a 400 with message "invalid input", and web/src/lib/api.ts:130-133's errorMessage() returns that server message (not the 'Could not save changes.'/'Could not create the garden.' fallback, which only applies to non-ApiError throws) — but "invalid input" is itself just as unhelpful as a bounds message, so the substance of the finding stands. Fix: validate against the same [1, 10000] cm bounds after conversion, not just positivity before it.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • web/src/components/gardens/GardenCard.tsx:32 and :39 — the footer Edit/Delete buttons are raw <button> elements with hand-rolled classes, not the shared Button component. Confirmed against web/src/components/ui/Button.tsx:4-16: buttonClasses centralizes focus-visible:ring-2 focus-visible:ring-accent/40, disabled:opacity-60, and transition styling that ButtonVariant only exposes as 'primary' | 'ghost' — neither raw button in GardenCard picks up the focus-visible ring or disabled-state treatment other buttons in the app get.

  • 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.

  • web/src/components/ui/TextField.tsx:16-17, TextArea.tsx:14-15, Select.tsx:14-15 — confirmed the identical two-line id-fallback (const generatedId = useId(); const fieldId = id ?? name ?? generatedId, named inputId in TextField) is duplicated verbatim across all three form-field components; this PR adds two of the three copies (TextArea, Select).

  • 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.

Performance — Minor issues

All confirmed. This matches the draft's claims exactly. The finding is accurate: mutations invalidate the whole list instead of patching cache with the already-known result, the backend comment and LIMIT 1000 are accurately cited.

VERDICT: Minor issues

  • web/src/lib/gardens.ts:44-72 (useCreateGarden, useUpdateGarden, useDeleteGarden) — every mutation's onSuccess calls qc.invalidateQueries({ queryKey: gardensKey }), forcing a full list refetch after every create/edit/delete, even though the create/update responses already contain the fresh row and delete already knows the id being removed. This costs an extra network round trip per mutation that a qc.setQueryData cache patch would avoid. Low severity given the backend's own comment that this is "household scale, a handful of gardens" (internal/store/gardens.go:26-29), but it's a real, avoidable pattern.
🧯 Error handling & edge cases — Minor issues

Review — 🧯 Error handling & edge cases

VERDICT: Minor issues

  • web/src/components/gardens/GardenFormModal.tsx:66-71 — Client validation checks w <= 0 / h <= 0 on the raw display-unit value, not on the centimeter value that actually gets sent. Because cmFromDisplay rounds to the nearest whole cm (web/src/lib/units.ts:12-24), a small positive input can round down to 0. E.g. entering 0.004 in metric mode (w = 0.004 > 0, passes the check) converts via cmFromMetersMath.round(0.004 * 100) = 0. The server's validDimensionCM (internal/service/gardens.go:170-171) rejects anything < 1 (minGardenCM = 1), so the request 400s with the server's generic error even though the form just told the user their input was fine. The same gap exists on the upper end: nothing in the form mirrors the server's maxGardenCM = 10_000 (100 m) bound (internal/service/gardens.go:18) — only a min="0" attribute is set on the width/height TextFields, no max — so an oversized value also silently falls through client validation into the same unhelpful generic error. Suggest validating against the post-conversion cm value (and the known [1, 10000] range) instead of the pre-conversion display value.

  • web/src/components/ui/Modal.tsx:16-22,31-33 combined with web/src/pages/GardensPage.tsx:53-55 and DeleteGardenModal.tsx/GardenFormModal.tsxModal's Escape-key handler and backdrop onMouseDown both call onClose() unconditionally, with no way for the caller to signal "a mutation is in flight." In GardensPage, onClose is () => setDialog(null), which unmounts the modal component immediately. DeleteGardenModal/GardenFormModal disable their own Cancel/Save buttons via disabled={pending}/disabled={del.isPending} but never guard the Modal's own dismiss paths. If a user presses Escape (or clicks the backdrop) while del.mutateAsync/create.mutateAsync/update.mutateAsync is still pending, the component unmounts, and the subsequent setError(...)/setVersion(...)/setConflict(...)/onClose() calls in the await continuation become no-ops on an unmounted component — notably dropping the 409-conflict "rebase" UX on the floor if the user dismisses mid-request. Suggest passing a closeDisabled/pending prop into Modal to suppress Escape/backdrop close while a mutation is in flight.

Both are edge-case/UX-clarity gaps, not crashes or data-loss bugs.

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

Through the security lens (client-only change), I verified the taint paths and they're clean:

  • No XSS: All untrusted server fields (garden.name, garden.notes) are rendered as React text children (<h3>, <p>), which auto-escape. No dangerouslySetInnerHTML, no href/src interpolation from user data.
  • No URL/path injection: The only path interpolations are /gardens/${id} (gardens.ts:54,62,68) and /gardens/${gardenId} (GardenCard.tsx via params={{ gardenId: String(garden.id) }}). id is constrained by gardenSchema (z.number()) before it ever reaches these call sites, so it can't carry path segments or query strings. GardenFormModal's useUpdateGarden(garden?.id ?? 0) likewise only ever passes a zod-validated number (the 0 is unused in the create path).
  • No SSRF/open-redirect: No fetch targets derived from user input; all requests go to the fixed API_BASE + '/gardens...' base. buildUrl (api.ts:47) only interpolates the validated numeric id, and query params are passed through URLSearchParams.
  • No unsafe deserialization: Server responses are parsed with z.array(gardenSchema).parse(...) / gardenSchema.parse(...) and the 409 body via gardenSchema.safeParse (conflictGarden), so malformed/conflicting payloads are rejected rather than trusted.
  • Auth: Requests use credentials: 'same-origin' (the HttpOnly session cookie); this change doesn't weaken that. Authorization (ownership checks) lives in the #7 API and is correctly out of scope for this UI layer — the client doesn't attempt to enforce it.
  • Input validation: GardenFormModal validates name non-empty and positive finite dimensions before sending (onSubmit); this is defense-in-depth, not the trust boundary (the API re-validates). notes is trimmed. No injection surface in the JSON body.

Confidence note: I confirmed the above by reading web/src/lib/api.ts, web/src/lib/units.ts, and the diff'd component files. The server-side authorization/validation boundary is intentionally in #7 and not this PR, so its absence here is not a finding.

🎯 Correctness — No material issues found

VERDICT: No material issues found

I verified the conversion math, the round-trip behavior, and the 409 rebase flow against the checked-out code.

  • Imperial conversions re-derived correctly. cmFromFtIn(4) = round(48 × 2.54) = 122; cmFromFtIn(8) = round(96 × 2.54) = 244 — matches the stated "4 ft × 8 ft → 122 × 244 cm" acceptance criterion. formatCm(122, imperial): totalInches = 48.03, feet = 4, inches = round(0.03) = 0 → 4′ 0″; formatCm(244)8′ 0″. ✓ (web/src/lib/units.ts:12-47)
  • Inch carry-over is handled. In formatCm, inches is Math.round of a value in [0, 12) (since feet = floor(totalInches/12)), so it can reach at most 12, never 13; the if (inches === 12) carry branch covers exactly that case. No off-by-one. (units.ts:38-42)
  • Edit prefill round-trips as claimed: displayFromCm(122, imperial) = round(4.0026 × 100)/100 = 4, so the field shows 4. (units.ts:27-31)
  • Unit-switch conversion preserves physical size. changeUnit converts v from the old unit to cm then to next, and all three setStates are computed from current props/state, so they batch consistently. Spot-checked 10 m → 32.81 ft → 1000 cm → 10 m. (GardenFormModal.tsx:44-52)
  • 409 rebase is semantically correct. On conflict it rebases version/name/unit/width/height/notes from the server's fresh row, computing width/height from current.unitPref (not stale state), keeps the modal open, and leaves pending false (the rejected mutateAsync has settled) so the user can re-save against the new version. A 409 on create without a current Garden shape falls through conflictGarden's safeParse to a normal form error — graceful. (GardenFormModal.tsx:81-96, gardens.ts:73-84)
  • DEFAULT_METERS = 10 is consistent with the cm-only API contract (cmFromMeters(10) = 1000), used only for the create-mode prefill. (GardenFormModal.tsx:12,19-21)

Trivial (non-material) note, not flagged as a finding: formatCm(1, imperial) renders 0′ 0″ — a non-zero size shown as zero — because imperial display is rounded to whole inches. This only affects sub-half-inch sizes, which are not realistic garden dimensions and is an inherent consequence of inch-precision display, so I'm not treating it as a real defect.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • web/src/components/ui/Button.tsx:4 + DeleteGardenModal.tsx:39 + GardenCard.tsx:42Button exposes only primary/ghost variants (line 4), so the delete-confirm button in DeleteGardenModal inlines bg-red-600 text-white hover:bg-red-700, while GardenCard's delete action is a raw <button> with a different red treatment (hover:bg-red-500/10 hover:text-red-600). Two divergent "destructive" patterns, neither shared. Adding a danger variant to buttonClasses would let both call sites stop hand-rolling red classes. Minor.

  • 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.

  • 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.

  • web/src/pages/GardensPage.tsx:53-55 — The three modal lines each repeat onClose={() => setDialog(null)}. A single close closure (or switching the modals on dialog?.kind) would remove the triplication. Trivial.

  • 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.

Performance — No material issues found

VERDICT: No material issues found

Performance lens is clean for this PR:

  • No N+1 / unbounded queries. useGardens is a single GET /gardens call; mutations invalidate exactly one query key (['gardens']). Card rendering is a single .map over the result array — no per-card fetches.
  • Invalidation scope is correct. Create/update/delete invalidate the gardens list only; no global invalidations or over-broad refetches.
  • No avoidable allocations on hot paths. formatCm/formatDimensions run once per card render (a handful of cards); conversion math is trivial. No reason to memoize at this scale.
  • conflictGarden only fires on the error path of a save (not hot), and does a single safeParse — fine.
  • GardenFormModal.changeUnit round-trips a parseFloatcmFromDisplaydisplayFromCm on unit switch; this is a user-initiated event (not a render or list loop), so the cost is irrelevant.
  • No blocking calls / sync work in render. All async work goes through react-query mutations.

Nothing in the performance lane justifies a change.

🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

  • web/src/components/ui/Modal.tsx:19-26,30-34 — Escape/backdrop close bypasses the disabled-during-pending guard, so a modal can be dismissed mid-mutation. The Modal closes on Escape (line 21) or a backdrop onMouseDown (lines 31-33) unconditionally, while DeleteGardenModal and GardenFormModal deliberately set disabled={del.isPending}/disabled={pending} on their action buttons to block interaction during the in-flight request. The keyboard/backdrop paths ignore that flag, so a user can press Escape (or click the dimmer) while a create/update/delete is still awaiting the server. For DeleteGardenModal (DeleteGardenModal.tsx:13-21,32-38) this is the most surprising: the dialog vanishes as if cancelled, but del.mutateAsync keeps running and its onSuccess (gardens.ts:68) still invalidates the list — the garden is removed with no confirmation that the user actually wanted to proceed. For GardenFormModal (GardenFormModal.tsx:54-97,146-149), the in-flight update.mutateAsync/create.mutateAsync resolves after the component has unmounted; React 18 tolerates the later setState calls in the onSubmit closure, but the success path (onClose()) and the conflict-rebase path run against an unmounted form, so a 409 that arrives after Escape would call setVersion/setName/… on a gone component (no-op, but the rebased state is silently lost). Suggested fix: gate the dismiss paths to match the button-disabled semantics — e.g. pass a closeable flag (or have onClose early-return when the relevant mutation isPending), or in Modal accept a disabled prop and ignore Escape/backdrop while it's set.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

From the security lens, the PR is clean:

  • XSS: All user-provided strings (garden.name, garden.notes, error messages) are rendered via React JSX, which escapes them automatically. No dangerouslySetInnerHTML or raw HTML insertion is introduced.
  • Injection: API path parameters are typed as number and interpolated into hardcoded route templates (/gardens/${id}). The id is zod-validated as z.number() before reaching the UI, and the hooks enforce the type at compile time. No command/SQL/template injection vectors are introduced in the frontend layer.
  • SSRF: All HTTP requests go through the internal api wrapper to /api/v1/*. No user-controlled URLs are fetched.
  • Input validation: API responses are validated with zod (gardenSchema.parse / safeParse). The conflict-rebase path explicitly safeParses the server-provided current row before adopting it into form state.
  • CSRF / auth: Requests use credentials: 'same-origin' to send the HttpOnly session cookie, with standard JSON content-type headers. No tokens or credentials are exposed in the code.
  • Destructive actions: Delete is gated behind a dedicated confirmation modal; accidental triggering is mitigated.
  • Secrets: No hardcoded credentials, tokens, or env leaks.
🎯 Correctness — Blocking issues found

Blocking issues found

  • web/src/components/ui/Modal.tsx:20-24 — Focus is stolen from form inputs on every parent re-render. The useEffect depends on onClose, which is an inline arrow (() => setDialog(null)) recreated by GardensPage on every render. When react-query background-refetches /gardens, GardensPage re-renders, onClose identity changes, the effect re-runs, and cardRef.current?.focus() jumps focus from the active text field/number input back to the modal card.
    Fix: separate the one-time focus into its own effect (or track with a ref), and/or wrap onClose in useCallback in GardensPage.

  • 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 cleanly, but that is coincidental; the majority of whole-foot values drift.
    Fix: either increase precision in displayFromCm (e.g. 4 decimals) or, better, parse the original user input from the stored cm without aggressive rounding.

  • 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 unexpectedly huge garden.
    Fix: validate cmFromDisplay(w, unit) >= 1 instead of w > 0.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • web/src/components/ui/Modal.tsx:19-26 — The useEffect dependency array is [onClose]. Because GardensPage passes fresh () => setDialog(null) lambdas on every render, the keydown listener is torn down and re-attached each frame. This is wasteful and a foot-gun once stacked modals exist.
    Fix: stabilise the callback in Modal (e.g. const closeRef = useRef(onClose); closeRef.current = onClose; and read closeRef.current inside the handler) so the effect only runs once.

  • 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.

  • 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.

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

  • 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.

Performance — Minor issues

Minor issues

  • 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 slow networks the new card will appear only after this second request completes.
    Fix: use qc.setQueryData(gardensKey, old => …) to append or replace the returned row directly, then optionally invalidate in the background. For useDeleteGarden invalidation is fine because no row data is returned.

  • web/src/components/ui/Modal.tsx:17-24Effect churn and focus stealing on every parent re-render.
    The useEffect depends on onClose, but callers pass an inline arrow (() => setDialog(null) from GardensPage) so the reference changes every time the parent re-renders—e.g. on a background useGardens refetch or window-focus refresh. The effect tears down and re-adds a document keydown listener, and calls cardRef.current?.focus(), which can pull focus away from an input the user is typing into.
    Fix: store onClose in a ref so the listener is registered once and never re-binds:

    const onCloseRef = useRef(onClose)
    onCloseRef.current = onClose
    useEffect(() => {
      const handler = (e: KeyboardEvent) => { if (e.key === 'Escape') onCloseRef.current() }
      document.addEventListener('keydown', handler)
      cardRef.current?.focus()
      return () => document.removeEventListener('keydown', handler)
    }, [])
    
🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • 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 and interrupting typing. Fix: stabilize the callback with a ref so the effect only runs once on mount.

  • 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.

opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens, this change is clean:

  • No injection sinks. All user-controlled strings (garden.name, garden.notes, form name/notes) are rendered as React text content (auto-escaped) — no dangerouslySetInnerHTML/innerHTML/eval anywhere in the new files (verified via grep over web/src).
  • No path/URL injection. Route params use String(garden.id) where id is a z.number() from the validated gardenSchema, and API paths /gardens/${id} use the same numeric id. No user string is interpolated into a URL.
  • Tainted numeric input is bounded. parseFloat results are checked with Number.isFinite and > 0 before conversion (GardenFormModal.tsx:54-58); cmFromDisplay only rounds, no further trust. Server-side validation is out of scope but the client doesn't introduce unbounded sink exposure.
  • Authn/authz delegated to existing layers. apiFetch sends the HttpOnly session cookie with credentials: 'same-origin'; mutations hit the versioned /api/v1/gardens endpoints whose authz is owned by the API (#7) and route guard (#6), not this PR. No new unauthenticated surface introduced.
  • 409 handling is safe. conflictGarden safeParses the server-supplied current row through gardenSchema before rebasing — malformed conflict bodies are ignored rather than trusted.
  • No secret/credential leakage in the new code or error messages (server messages surfaced via errorMessage are status/body-derived, not headers/cookies).
🎯 Correctness — No material issues found

I've verified the conversion math, the API contract, the zod schema alignment, and the 409 rebase flow against the actual server code. Let me trace the key correctness paths.

Verification done:

  • Confirmed cmFromFtIn(4, 0) = round(48 × 2.54) = 122 and cmFromFtIn(8, 0) = round(96 × 2.54) = 244 ✓ (acceptance criterion holds)
  • Confirmed formatCm(122, 'imperial')"4′ 0″" and formatCm(244, 'imperial')"8′ 0″" ✓ (card display)
  • Confirmed server's 409 envelope shape {error, current} matches conflictGarden's parsing (internal/api/errors.go:57-62) ✓
  • Confirmed zod gardenSchema field names match domain.Garden JSON tags exactly ✓
  • Confirmed useUpdateGarden/useCreateGarden correctly send version only on update; server requires min=1 and the form's edit-mode version starts at garden.version ≥ 1
  • Confirmed changeUnit round-trips physical size (10 m → 32.81 ft) ✓

VERDICT: No material issues found

  • web/src/lib/units.ts:30Trivial (verified): the edit-form prefill uses displayFromCm (rounds to 2 decimals), while the card uses formatCm. For an 8 ft garden stored as 244 cm (because cmFromFtIn(8) rounds 243.84 → 244), displayFromCm(244, 'imperial') = Math.round(8.0052 × 100)/100 = 8.01, so the Edit form's width field shows 8.01 instead of 8. The physical size is preserved on re-save (8.01 → 244 cm), so this is display-only; the 4 ft side round-trips cleanly (4.0026 → 4.00). Root cause is double rounding (cm rounding then 2-decimal display rounding). Not a data bug; just a cosmetic inconsistency between the card and the edit form. Could be sharpened by rounding displayFromCm to fewer decimals, or by having the form prefill from a rounded-truncated value, but nothing breaks.

Everything else in the conversion library, the form/validation logic, the 409 rebase (sets version + fields onto the fresh server row), the mutation invalidation, the route params, and the modal state machines checks out correctly against the actual server contract.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • 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-col gap-1.5">, and <label className="text-sm font-medium text-fg">) is also verbatim in all three. Extracting a shared fieldClasses constant and/or a <FieldShell label> wrapper would prevent drift as more field types are added. Verified by reading all three files. Small.
  • web/src/components/gardens/GardenFormModal.tsx:32useUpdateGarden(garden?.id ?? 0) uses 0 as a create-mode sentinel without explanation. It's harmless (the create branch never calls update), but a one-line comment or a more obviously-sentinel value would clarify intent for a future maintainer. Trivial.
Performance — No material issues found

I'll investigate the PR through the performance lens. Let me verify a few things.
VERDICT: No material issues found

Through the performance lens, this change is clean:

  • List query (useHardensapi.get('/gardens')): single fetch, no N+1. The grid maps over the result once; formatCm/formatDimensions do trivial arithmetic per card. No per-item follow-up requests.
  • Mutations (useCreateGarden/useUpdateGarden/useDeleteGarden): each invalidates the single ['gardens'] key, triggering one refetch of the list — the intended react-query pattern, not a redundant cascade. No optimistic updates promised, so no double-work.
  • GardenFormModal: calls useCreateGarden() and useUpdateGarden(...) unconditionally (required by the rules of hooks); both are lazy mutations, so the unused one does no work. changeUnit runs two parseFloat + two conversions on an explicit user action — not a hot path.
  • Modal: one keydown listener on document, cleaned up on unmount. No scroll listeners, no resize observers, no rAF loops.
  • Inline onEdit/onDelete arrows in GardensPage's .map are recreated per render, but the list is small and GardenCard isn't memoized anyway — not a measurable cost here, and memoizing would be a micro-optimization.
  • Unbounded list: there's no pagination/limit on GET /gardens. For this app's scope (a user's own gardens) this is a reasonable scope decision, not a regression introduced by this PR, so I'm not flagging it as material.

I verified the above by reading web/src/lib/api.ts, web/src/pages/GardensPage.tsx, and the new lib/gardens.ts / lib/units.ts / modal components in the diff.

🧯 Error handling & edge cases — Minor issues

Both findings confirmed against the actual source.

VERDICT: Minor issues

  • web/src/components/ui/Modal.tsx:21,32 — closing the modal during an in-flight mutation swallows the error. Modal calls onClose() unconditionally on Escape (line 21) and on backdrop mousedown (line 32). The form/delete modals guard their Cancel buttons with disabled={del.isPending} / disabled={pending} (GardenFormModal.tsx:146, DeleteGardenModal.tsx), but Escape and the backdrop click are not guarded. While a save or delete is pending, the user can close the modal mid-flight: GardensPage.tsx:53-55 renders the modal only while dialog is set and the close path calls setDialog(null), unmounting the modal. The still-running await mutateAsync(...) later rejects and the setFormError / setError call lands on an unmounted component and is dropped, so the user never learns the garden wasn't saved/deleted; reopening the modal starts with a blank error state. Suggested fix: pass pending into Modal and no-op onClose (and skip the Escape listener / backdrop handler) while pending is true, mirroring the Cancel-button guard.

  • web/src/components/gardens/GardenFormModal.tsx:46-47changeUnit preserves non-numeric strings as-is. If a dimension field is empty ("") or contains garbage ("abc"), parseFloat returns NaN, Number.isFinite is false, and convert returns the original string unchanged while setUnit(next) still runs. The physical-size-preservation invariant silently breaks: the displayed value no longer reflects the previously-entered size in the new unit, and on save the field is rejected by the w <= 0/!Number.isFinite check at line 61, producing a generic "Enter a name and a positive width and height" error rather than pointing at the offending field. Minor, but the unit-switch path has no recovery for an empty/garbage input. Suggested fix: when !Number.isFinite(v), reset to the default (e.g. String(displayFromCm(DEFAULT_METERS * 100, next))) or leave the field empty and let the per-field required validation surface it.

opencode/kimi-k2.6:cloud (opencode) — Blocking issues found

Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

I reviewed this PR through the security lens, tracing tainted input from the UI through to the backend API and back.

  • AuthZ / IDOR: Every garden endpoint is behind requireAuth. The backend service (internal/service/gardens.go) enforces ownership via requireGardenRole / effectiveGardenRole. The frontend Link to /gardens/$gardenId uses the garden's own id, but the backend GetGarden re-verifies ownership, so no IDOR.
  • Injection: No SQL/command/template injection vectors. API calls use parameterized routes (/gardens/${id}) with string coercion of numeric IDs, and the backend uses parameterized DB queries. No raw HTML construction.
  • XSS: garden.name and garden.notes are rendered as React text nodes (JSX {…}), which React escapes automatically. No dangerouslySetInnerHTML usage.
  • CSRF: State-changing API requests rely on the same-origin session cookie. The backend attaches SameSite=Lax and an Origin-checking csrfGuard for mutating requests.
  • Input validation: The form validates positive finite dimensions and non-empty names. The backend additionally enforces length caps (maxGardenNameLen = 200, maxGardenNotesLen = 10_000) and a physical dimension range ([1 cm, 100 m]).
  • Unsafe deserialization: All API responses are parsed through zod schemas (gardenSchema, userSchema), which reject unexpected shapes.
  • Secret leakage: errorMessage surfaces only the server's user-facing message string; no raw stack traces or tokens are exposed.
🎯 Correctness — Blocking issues found

Blocking issues found

  • web/src/components/ui/Modal.tsx:19-26 — Focus theft on every parent re-render. The useEffect depends on onClose, which the parent (GardensPage) supplies as an inline arrow (() => setDialog(null)) that is recreated on every render. Any background re-render (e.g. React Query refetch on window focus) re-runs the effect, calling cardRef.current?.focus() and stealing focus from whichever input the user is currently typing in.
    Fix: Split the effect: register the Escape listener in one effect with a stable ref wrapper for onClose, and call focus() only on mount (empty deps).

  • 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: Make the modal respect the pending state. Either have GardenFormModal pass a guarded close handler (if (pending) return; onClose()) to Modal, or add an allowClose / onClose guard prop to Modal itself.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • 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 both places. Fix: use <Button variant="ghost" className="px-2.5 py-1 ..."> with overrides, or add a compact size prop to Button.

  • 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 both places. Fix: use <Button variant="ghost" className="px-2.5 py-1 ..."> with overrides, or add a compact size prop to Button.

  • 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 wrapper) and use it in all three components.

  • 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 wrapper) and use it in all three components.

Performance⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error

database is locked

🧯 Error handling & edge cases — Minor issues

Minor issues

  • web/src/components/ui/Modal.tsx:21 — Pressing Escape unconditionally calls onClose(), even when the caller has disabled its Cancel button because a mutation is in flight. In GardenFormModal and DeleteGardenModal, a keyboard user can therefore dismiss the modal while create/update/del is pending, discarding unsaved create/edit data or hiding a delete-confirmation while the request continues. The same applies to backdrop clicks (onMouseDown).
    Fix: Have the callers pass a guarded close handler, e.g. onClose={pending ? () => {} : () => setDialog(null)}, or add an optional canClose prop to Modal.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Blocking issues found** · 23 findings (8 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | Modal useEffect re-runs on every onClose change, repeatedly calling focus() and stealing focus from active inputs during parent re-renders | `web/src/components/ui/Modal.tsx:17` | 5/5 | correctness, error-handling, maintainability, performance | | 🟠 | Raw <button> elements bypass shared Button component styles and accessibility | `web/src/components/gardens/GardenCard.tsx:32` | 2/5 | maintainability | | 🟠 | Form validates parsed float > 0 but small positive values round to 0 cm in API | `web/src/components/gardens/GardenFormModal.tsx:66` | 2/5 | correctness, error-handling | | 🟠 | Redundant full-list refetch after create mutation | `web/src/lib/gardens.ts:46` | 2/5 | performance | | 🟡 | Dummy id 0 passed to useUpdateGarden during creation is a leaky abstraction | `web/src/components/gardens/GardenFormModal.tsx:32` | 2/5 | maintainability | | 🟡 | Unit-switching conversion is lossy and contradicts preserve-physical-size claim | `web/src/components/gardens/GardenFormModal.tsx:44` | 2/5 | error-handling, maintainability | | 🟡 | Copied useId fallback logic from TextField instead of extracting shared hook | `web/src/components/ui/TextArea.tsx:14` | 2/5 | maintainability | | 🟡 | Fresh lambda close handlers cause Modal effect churn | `web/src/pages/GardensPage.tsx:53` | 2/5 | maintainability | <details><summary>15 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | Modal dismissible during pending mutation causing lost error feedback | `web/src/components/ui/Modal.tsx:31` | opencode/kimi-k2.6:cloud | correctness | | 🔴 | displayFromCm 2-decimal rounding causes systematic imperial round-trip errors (8 ft becomes 8.01 ft) | `web/src/lib/units.ts:27` | kimi-k2.6:cloud | correctness | | 🟠 | Raw <button> elements bypass shared Button component styles and accessibility | `web/src/components/gardens/GardenCard.tsx:39` | opencode/kimi-k2.6:cloud | maintainability | | 🟠 | Garden schema does not enforce positive dimensions, allowing zero/negative widthCm and heightCm from API | `web/src/lib/gardens.ts:15` | kimi-k2.6:cloud | error-handling | | 🟠 | Redundant full-list refetch after update mutation | `web/src/lib/gardens.ts:62` | kimi-k2.6:cloud | performance | | 🟡 | Destructive-action button hardcodes red classes instead of adding a danger variant to buttonClasses | `web/src/components/gardens/DeleteGardenModal.tsx:33` | claude-code/sonnet | maintainability | | 🟡 | Client validates dimension positivity before unit conversion, missing server's [1cm,100m] bounds after rounding | `web/src/components/gardens/GardenFormModal.tsx:59` | claude-code/sonnet | correctness | | 🟡 | 409 rebase duplicates dimString helper instead of reusing it | `web/src/components/gardens/GardenFormModal.tsx:89` | glm-5.2:cloud | maintainability | | 🟡 | No destructive/danger variant; red styling hand-rolled inconsistently in DeleteGardenModal and GardenCard | `web/src/components/ui/Button.tsx:4` | glm-5.2:cloud | maintainability | | 🟡 | Copied useId fallback logic from TextField instead of extracting shared hook | `web/src/components/ui/Select.tsx:14` | opencode/kimi-k2.6:cloud | maintainability | | 🟡 | Field chrome (styling + label/id-fallback pattern) duplicated across Select, TextArea, and TextField — extract a shared FieldShell/fieldClasses | `web/src/components/ui/Select.tsx:25` | opencode/glm-5.2:cloud | maintainability | | ⚪ | Variable named del is unnecessarily terse | `web/src/components/gardens/DeleteGardenModal.tsx:10` | kimi-k2.6:cloud | maintainability | | ⚪ | Redundant z.ZodType<UnitPref> annotation diverges from auth.ts sibling pattern | `web/src/lib/gardens.ts:9` | glm-5.2:cloud | maintainability | | ⚪ | cmFromFtIn/cmFromMeters exported with no current external consumers | `web/src/lib/units.ts:12` | claude-code/sonnet | maintainability | | ⚪ | 'New garden' visibility check duplicates grid condition with inconsistent isSuccess/data usage | `web/src/pages/GardensPage.tsx:20` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** I checked how untrusted data flows through this diff with a security lens: - **API layer** (`web/src/lib/api.ts`, unchanged) uses `credentials: 'same-origin'` and relies on an HttpOnly session cookie — no tokens are exposed to JS, and this file isn't touched by the diff. - **Route guard**: `web/src/router.tsx:82-87` confirms `/gardens` is behind `requireAuth`, which resolves `/auth/me` before rendering `GardensPage` — consistent with the PR's claim of consuming the #6 auth guard. - **Server response validation**: `gardenSchema` (zod) validates every API response before use (`lib/gardens.ts:17-27,39,47,58`) and the 409-conflict payload is also `safeParse`'d (`lib/gardens.ts:79-84`) rather than trusted blindly — no unsafe deserialization. - **Rendering**: all user/server-provided strings (`garden.name`, `garden.notes`, error messages) are rendered via JSX interpolation, not `dangerouslySetInnerHTML` — grepped the whole `web/src` tree for `dangerouslySetInnerHTML`/`__html` and found zero matches, so no injected-HTML/XSS vector here. - **IDs used in requests/links** (`GardenCard.tsx:15-16`, `GardenFormModal.tsx` PATCH URL, `DeleteGardenModal.tsx` delete call) all originate from the zod-validated `Garden.id: z.number()`, not from raw user input — no path/template injection surface. - **Client-side numeric validation** (`GardenFormModal.tsx:56-60`) rejects non-finite/non-positive width/height before submit; this is a UX guard, not a security boundary — actual enforcement is server-side (out of scope, backend from #7). - Authorization for who can edit/delete a given garden is enforced by the backend (out of scope for this frontend-only diff); the frontend does not attempt and does not need to duplicate that check. Nothing in this diff introduces an authn/authz gap, injection point, SSRF, unsafe deserialization, secret leakage, or insecure default. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings hold up against the actual source, with one correction needed on finding 2's error-message detail. **VERDICT: Minor issues** - `web/src/components/ui/Modal.tsx:19-26` — The Escape-key/focus `useEffect` depends on `[onClose]` (line 26), but every caller passes a fresh inline arrow: `GardensPage.tsx:53-55` passes `() => setDialog(null)` for all three dialog kinds. `queryClient.ts:6-10` confirms `refetchOnWindowFocus: false` is set but `refetchOnReconnect`/background refetches are left at their TanStack Query defaults (enabled), and `useGardens()` subscribes to the `gardens` query used on the very page that renders the modal. A background refetch (reconnect, refocus elsewhere, or any other unrelated state change on `GardensPage`) re-renders the page, creates a new `onClose` reference, and re-fires the effect — including `cardRef.current?.focus()` on line 24 — which steals focus from whatever field the user is actively editing inside `GardenFormModal`/`DeleteGardenModal`, since those inputs render as descendants of `cardRef`'s div. Fix: run the focus-on-mount behavior once (empty dependency array) and keep the Escape listener in a separate effect, or have callers memoize `onClose`. - `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 `500` m also passes the client check but exceeds the 100 m server cap. Correction to the original claim: the resulting error is not the component's generic fallback text — `internal/api/errors.go:45-46` maps `ErrInvalidInput` to a 400 with message `"invalid input"`, and `web/src/lib/api.ts:130-133`'s `errorMessage()` returns that server message (not the `'Could not save changes.'`/`'Could not create the garden.'` fallback, which only applies to non-`ApiError` throws) — but `"invalid input"` is itself just as unhelpful as a bounds message, so the substance of the finding stands. Fix: validate against the same `[1, 10000]` cm bounds after conversion, not just positivity before it. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - `web/src/components/gardens/GardenCard.tsx:32` and `:39` — the footer Edit/Delete buttons are raw `<button>` elements with hand-rolled classes, not the shared `Button` component. Confirmed against `web/src/components/ui/Button.tsx:4-16`: `buttonClasses` centralizes `focus-visible:ring-2 focus-visible:ring-accent/40`, `disabled:opacity-60`, and transition styling that `ButtonVariant` only exposes as `'primary' | 'ghost'` — neither raw button in `GardenCard` picks up the focus-visible ring or disabled-state treatment other buttons in the app get. - `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. - `web/src/components/ui/TextField.tsx:16-17`, `TextArea.tsx:14-15`, `Select.tsx:14-15` — confirmed the identical two-line id-fallback (`const generatedId = useId(); const fieldId = id ?? name ?? generatedId`, named `inputId` in `TextField`) is duplicated verbatim across all three form-field components; this PR adds two of the three copies (`TextArea`, `Select`). - `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. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> All confirmed. This matches the draft's claims exactly. The finding is accurate: mutations invalidate the whole list instead of patching cache with the already-known result, the backend comment and LIMIT 1000 are accurately cited. VERDICT: Minor issues - `web/src/lib/gardens.ts:44-72` (`useCreateGarden`, `useUpdateGarden`, `useDeleteGarden`) — every mutation's `onSuccess` calls `qc.invalidateQueries({ queryKey: gardensKey })`, forcing a full list refetch after every create/edit/delete, even though the create/update responses already contain the fresh row and delete already knows the `id` being removed. This costs an extra network round trip per mutation that a `qc.setQueryData` cache patch would avoid. Low severity given the backend's own comment that this is "household scale, a handful of gardens" (`internal/store/gardens.go:26-29`), but it's a real, avoidable pattern. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## Review — 🧯 Error handling & edge cases **VERDICT: Minor issues** - `web/src/components/gardens/GardenFormModal.tsx:66-71` — Client validation checks `w <= 0` / `h <= 0` on the **raw display-unit value**, not on the centimeter value that actually gets sent. Because `cmFromDisplay` rounds to the nearest whole cm (`web/src/lib/units.ts:12-24`), a small positive input can round down to `0`. E.g. entering `0.004` in metric mode (`w = 0.004 > 0`, passes the check) converts via `cmFromMeters` → `Math.round(0.004 * 100) = 0`. The server's `validDimensionCM` (`internal/service/gardens.go:170-171`) rejects anything `< 1` (`minGardenCM = 1`), so the request 400s with the server's generic error even though the form just told the user their input was fine. The same gap exists on the upper end: nothing in the form mirrors the server's `maxGardenCM = 10_000` (100 m) bound (`internal/service/gardens.go:18`) — only a `min="0"` attribute is set on the width/height `TextField`s, no `max` — so an oversized value also silently falls through client validation into the same unhelpful generic error. Suggest validating against the post-conversion cm value (and the known `[1, 10000]` range) instead of the pre-conversion display value. - `web/src/components/ui/Modal.tsx:16-22,31-33` combined with `web/src/pages/GardensPage.tsx:53-55` and `DeleteGardenModal.tsx`/`GardenFormModal.tsx` — `Modal`'s Escape-key handler and backdrop `onMouseDown` both call `onClose()` unconditionally, with no way for the caller to signal "a mutation is in flight." In `GardensPage`, `onClose` is `() => setDialog(null)`, which unmounts the modal component immediately. `DeleteGardenModal`/`GardenFormModal` disable their own Cancel/Save buttons via `disabled={pending}`/`disabled={del.isPending}` but never guard the Modal's own dismiss paths. If a user presses Escape (or clicks the backdrop) while `del.mutateAsync`/`create.mutateAsync`/`update.mutateAsync` is still pending, the component unmounts, and the subsequent `setError(...)`/`setVersion(...)`/`setConflict(...)`/`onClose()` calls in the `await` continuation become no-ops on an unmounted component — notably dropping the 409-conflict "rebase" UX on the floor if the user dismisses mid-request. Suggest passing a `closeDisabled`/`pending` prop into `Modal` to suppress Escape/backdrop close while a mutation is in flight. Both are edge-case/UX-clarity gaps, not crashes or data-loss bugs. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found Through the security lens (client-only change), I verified the taint paths and they're clean: - **No XSS**: All untrusted server fields (`garden.name`, `garden.notes`) are rendered as React text children (`<h3>`, `<p>`), which auto-escape. No `dangerouslySetInnerHTML`, no `href`/`src` interpolation from user data. - **No URL/path injection**: The only path interpolations are `/gardens/${id}` (`gardens.ts:54,62,68`) and `/gardens/${gardenId}` (`GardenCard.tsx` via `params={{ gardenId: String(garden.id) }}`). `id` is constrained by `gardenSchema` (`z.number()`) before it ever reaches these call sites, so it can't carry path segments or query strings. `GardenFormModal`'s `useUpdateGarden(garden?.id ?? 0)` likewise only ever passes a zod-validated number (the `0` is unused in the create path). - **No SSRF/open-redirect**: No fetch targets derived from user input; all requests go to the fixed `API_BASE + '/gardens...'` base. `buildUrl` (`api.ts:47`) only interpolates the validated numeric id, and query params are passed through `URLSearchParams`. - **No unsafe deserialization**: Server responses are parsed with `z.array(gardenSchema).parse(...)` / `gardenSchema.parse(...)` and the 409 body via `gardenSchema.safeParse` (`conflictGarden`), so malformed/conflicting payloads are rejected rather than trusted. - **Auth**: Requests use `credentials: 'same-origin'` (the HttpOnly session cookie); this change doesn't weaken that. Authorization (ownership checks) lives in the #7 API and is correctly out of scope for this UI layer — the client doesn't attempt to enforce it. - **Input validation**: `GardenFormModal` validates name non-empty and positive finite dimensions before sending (`onSubmit`); this is defense-in-depth, not the trust boundary (the API re-validates). `notes` is trimmed. No injection surface in the JSON body. Confidence note: I confirmed the above by reading `web/src/lib/api.ts`, `web/src/lib/units.ts`, and the diff'd component files. The server-side authorization/validation boundary is intentionally in #7 and not this PR, so its absence here is not a finding. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** I verified the conversion math, the round-trip behavior, and the 409 rebase flow against the checked-out code. - **Imperial conversions re-derived correctly.** `cmFromFtIn(4) = round(48 × 2.54) = 122`; `cmFromFtIn(8) = round(96 × 2.54) = 244` — matches the stated "4 ft × 8 ft → 122 × 244 cm" acceptance criterion. `formatCm(122, imperial)`: totalInches = 48.03, feet = 4, inches = round(0.03) = 0 → `4′ 0″`; `formatCm(244)` → `8′ 0″`. ✓ (`web/src/lib/units.ts:12-47`) - **Inch carry-over is handled.** In `formatCm`, `inches` is `Math.round` of a value in `[0, 12)` (since `feet = floor(totalInches/12)`), so it can reach at most 12, never 13; the `if (inches === 12)` carry branch covers exactly that case. No off-by-one. (`units.ts:38-42`) - **Edit prefill round-trips** as claimed: `displayFromCm(122, imperial) = round(4.0026 × 100)/100 = 4`, so the field shows `4`. (`units.ts:27-31`) - **Unit-switch conversion preserves physical size.** `changeUnit` converts `v` from the *old* `unit` to cm then to `next`, and all three `setState`s are computed from current props/state, so they batch consistently. Spot-checked 10 m → 32.81 ft → 1000 cm → 10 m. (`GardenFormModal.tsx:44-52`) - **409 rebase is semantically correct.** On conflict it rebases `version/name/unit/width/height/notes` from the server's fresh row, computing width/height from `current.unitPref` (not stale state), keeps the modal open, and leaves `pending` false (the rejected `mutateAsync` has settled) so the user can re-save against the new version. A 409 on *create* without a `current` Garden shape falls through `conflictGarden`'s `safeParse` to a normal form error — graceful. (`GardenFormModal.tsx:81-96`, `gardens.ts:73-84`) - **`DEFAULT_METERS = 10`** is consistent with the cm-only API contract (`cmFromMeters(10) = 1000`), used only for the create-mode prefill. (`GardenFormModal.tsx:12,19-21`) Trivial (non-material) note, not flagged as a finding: `formatCm(1, imperial)` renders `0′ 0″` — a non-zero size shown as zero — because imperial display is rounded to whole inches. This only affects sub-half-inch sizes, which are not realistic garden dimensions and is an inherent consequence of inch-precision display, so I'm not treating it as a real defect. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - **`web/src/components/ui/Button.tsx:4` + `DeleteGardenModal.tsx:39` + `GardenCard.tsx:42`** — `Button` exposes only `primary`/`ghost` variants (line 4), so the delete-confirm button in `DeleteGardenModal` inlines `bg-red-600 text-white hover:bg-red-700`, while `GardenCard`'s delete action is a raw `<button>` with a *different* red treatment (`hover:bg-red-500/10 hover:text-red-600`). Two divergent "destructive" patterns, neither shared. Adding a `danger` variant to `buttonClasses` would let both call sites stop hand-rolling red classes. Minor. - **`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. - **`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. - **`web/src/pages/GardensPage.tsx:53-55`** — The three modal lines each repeat `onClose={() => setDialog(null)}`. A single `close` closure (or switching the modals on `dialog?.kind`) would remove the triplication. Trivial. - **`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. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Performance lens is clean for this PR: - **No N+1 / unbounded queries.** `useGardens` is a single `GET /gardens` call; mutations invalidate exactly one query key (`['gardens']`). Card rendering is a single `.map` over the result array — no per-card fetches. - **Invalidation scope is correct.** Create/update/delete invalidate the `gardens` list only; no global invalidations or over-broad refetches. - **No avoidable allocations on hot paths.** `formatCm`/`formatDimensions` run once per card render (a handful of cards); conversion math is trivial. No reason to memoize at this scale. - **`conflictGarden`** only fires on the error path of a save (not hot), and does a single `safeParse` — fine. - **`GardenFormModal.changeUnit`** round-trips a `parseFloat`→`cmFromDisplay`→`displayFromCm` on unit switch; this is a user-initiated event (not a render or list loop), so the cost is irrelevant. - **No blocking calls / sync work in render.** All async work goes through react-query mutations. Nothing in the performance lane justifies a change. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **VERDICT: Minor issues** - **`web/src/components/ui/Modal.tsx:19-26,30-34` — Escape/backdrop close bypasses the `disabled`-during-pending guard, so a modal can be dismissed mid-mutation.** The `Modal` closes on Escape (line 21) or a backdrop `onMouseDown` (lines 31-33) unconditionally, while `DeleteGardenModal` and `GardenFormModal` deliberately set `disabled={del.isPending}`/`disabled={pending}` on their action buttons to block interaction during the in-flight request. The keyboard/backdrop paths ignore that flag, so a user can press Escape (or click the dimmer) while a create/update/delete is still awaiting the server. For `DeleteGardenModal` (`DeleteGardenModal.tsx:13-21,32-38`) this is the most surprising: the dialog vanishes as if cancelled, but `del.mutateAsync` keeps running and its `onSuccess` (`gardens.ts:68`) still invalidates the list — the garden is removed with no confirmation that the user actually wanted to proceed. For `GardenFormModal` (`GardenFormModal.tsx:54-97,146-149`), the in-flight `update.mutateAsync`/`create.mutateAsync` resolves after the component has unmounted; React 18 tolerates the later `setState` calls in the `onSubmit` closure, but the success path (`onClose()`) and the conflict-rebase path run against an unmounted form, so a 409 that arrives after Escape would call `setVersion`/`setName`/… on a gone component (no-op, but the rebased state is silently lost). Suggested fix: gate the dismiss paths to match the button-disabled semantics — e.g. pass a `closeable` flag (or have `onClose` early-return when the relevant mutation `isPending`), or in `Modal` accept a `disabled` prop and ignore Escape/backdrop while it's set. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** From the security lens, the PR is clean: - **XSS:** All user-provided strings (`garden.name`, `garden.notes`, error messages) are rendered via React JSX, which escapes them automatically. No `dangerouslySetInnerHTML` or raw HTML insertion is introduced. - **Injection:** API path parameters are typed as `number` and interpolated into hardcoded route templates (`/gardens/${id}`). The `id` is zod-validated as `z.number()` before reaching the UI, and the hooks enforce the type at compile time. No command/SQL/template injection vectors are introduced in the frontend layer. - **SSRF:** All HTTP requests go through the internal `api` wrapper to `/api/v1/*`. No user-controlled URLs are fetched. - **Input validation:** API responses are validated with zod (`gardenSchema.parse` / `safeParse`). The conflict-rebase path explicitly `safeParse`s the server-provided `current` row before adopting it into form state. - **CSRF / auth:** Requests use `credentials: 'same-origin'` to send the HttpOnly session cookie, with standard JSON content-type headers. No tokens or credentials are exposed in the code. - **Destructive actions:** Delete is gated behind a dedicated confirmation modal; accidental triggering is mitigated. - **Secrets:** No hardcoded credentials, tokens, or env leaks. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** - **`web/src/components/ui/Modal.tsx:20-24`** — Focus is stolen from form inputs on every parent re-render. The `useEffect` depends on `onClose`, which is an inline arrow (`() => setDialog(null)`) recreated by `GardensPage` on every render. When react-query background-refetches `/gardens`, `GardensPage` re-renders, `onClose` identity changes, the effect re-runs, and `cardRef.current?.focus()` jumps focus from the active text field/number input back to the modal card. **Fix:** separate the one-time focus into its own effect (or track with a ref), and/or wrap `onClose` in `useCallback` in `GardensPage`. - **`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 cleanly, but that is coincidental; the majority of whole-foot values drift. **Fix:** either increase precision in `displayFromCm` (e.g. 4 decimals) or, better, parse the original user input from the stored cm without aggressive rounding. - **`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 unexpectedly huge garden. **Fix:** validate `cmFromDisplay(w, unit) >= 1` instead of `w > 0`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Minor issues - `web/src/components/ui/Modal.tsx:19-26` — The `useEffect` dependency array is `[onClose]`. Because `GardensPage` passes fresh `() => setDialog(null)` lambdas on every render, the `keydown` listener is torn down and re-attached each frame. This is wasteful and a foot-gun once stacked modals exist. **Fix:** stabilise the callback in `Modal` (e.g. `const closeRef = useRef(onClose); closeRef.current = onClose;` and read `closeRef.current` inside the handler) so the effect only runs once. - `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. - `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. - `web/src/components/gardens/DeleteGardenModal.tsx:10` — Variable named `del` is unnecessarily terse. **Fix:** rename to `deleteGarden` or `mutation`. - `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. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Minor issues** - `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 slow networks the new card will appear only after this second request completes. **Fix:** use `qc.setQueryData(gardensKey, old => …)` to append or replace the returned row directly, then optionally invalidate in the background. For `useDeleteGarden` invalidation is fine because no row data is returned. - `web/src/components/ui/Modal.tsx:17-24` — **Effect churn and focus stealing on every parent re-render.** The `useEffect` depends on `onClose`, but callers pass an inline arrow (`() => setDialog(null)` from `GardensPage`) so the reference changes every time the parent re-renders—e.g. on a background `useGardens` refetch or window-focus refresh. The effect tears down and re-adds a document `keydown` listener, and calls `cardRef.current?.focus()`, which can pull focus away from an input the user is typing into. **Fix:** store `onClose` in a ref so the listener is registered once and never re-binds: ```ts const onCloseRef = useRef(onClose) onCloseRef.current = onClose useEffect(() => { const handler = (e: KeyboardEvent) => { if (e.key === 'Escape') onCloseRef.current() } document.addEventListener('keydown', handler) cardRef.current?.focus() return () => document.removeEventListener('keydown', handler) }, []) ``` </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** * **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 and interrupting typing. Fix: stabilize the callback with a ref so the effect only runs once on mount. * **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. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found Through the security lens, this change is clean: - **No injection sinks.** All user-controlled strings (`garden.name`, `garden.notes`, form `name`/`notes`) are rendered as React text content (auto-escaped) — no `dangerouslySetInnerHTML`/`innerHTML`/`eval` anywhere in the new files (verified via grep over `web/src`). - **No path/URL injection.** Route params use `String(garden.id)` where `id` is a `z.number()` from the validated `gardenSchema`, and API paths `/gardens/${id}` use the same numeric `id`. No user string is interpolated into a URL. - **Tainted numeric input is bounded.** `parseFloat` results are checked with `Number.isFinite` and `> 0` before conversion (`GardenFormModal.tsx:54-58`); `cmFromDisplay` only rounds, no further trust. Server-side validation is out of scope but the client doesn't introduce unbounded sink exposure. - **Authn/authz delegated to existing layers.** `apiFetch` sends the HttpOnly session cookie with `credentials: 'same-origin'`; mutations hit the versioned `/api/v1/gardens` endpoints whose authz is owned by the API (#7) and route guard (#6), not this PR. No new unauthenticated surface introduced. - **409 handling is safe.** `conflictGarden` `safeParse`s the server-supplied `current` row through `gardenSchema` before rebasing — malformed conflict bodies are ignored rather than trusted. - **No secret/credential leakage** in the new code or error messages (server messages surfaced via `errorMessage` are status/body-derived, not headers/cookies). </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've verified the conversion math, the API contract, the zod schema alignment, and the 409 rebase flow against the actual server code. Let me trace the key correctness paths. **Verification done:** - Confirmed `cmFromFtIn(4, 0) = round(48 × 2.54) = 122` and `cmFromFtIn(8, 0) = round(96 × 2.54) = 244` ✓ (acceptance criterion holds) - Confirmed `formatCm(122, 'imperial')` → `"4′ 0″"` and `formatCm(244, 'imperial')` → `"8′ 0″"` ✓ (card display) - Confirmed server's 409 envelope shape `{error, current}` matches `conflictGarden`'s parsing (`internal/api/errors.go:57-62`) ✓ - Confirmed zod `gardenSchema` field names match `domain.Garden` JSON tags exactly ✓ - Confirmed `useUpdateGarden`/`useCreateGarden` correctly send `version` only on update; server requires `min=1` and the form's edit-mode `version` starts at `garden.version ≥ 1` ✓ - Confirmed `changeUnit` round-trips physical size (10 m → 32.81 ft) ✓ **VERDICT: No material issues found** - `web/src/lib/units.ts:30` — **Trivial** (verified): the edit-form prefill uses `displayFromCm` (rounds to 2 decimals), while the card uses `formatCm`. For an 8 ft garden stored as 244 cm (because `cmFromFtIn(8)` rounds 243.84 → 244), `displayFromCm(244, 'imperial')` = `Math.round(8.0052 × 100)/100` = **8.01**, so the Edit form's width field shows `8.01` instead of `8`. The physical size is preserved on re-save (8.01 → 244 cm), so this is display-only; the 4 ft side round-trips cleanly (`4.0026 → 4.00`). Root cause is double rounding (cm rounding then 2-decimal display rounding). Not a data bug; just a cosmetic inconsistency between the card and the edit form. Could be sharpened by rounding `displayFromCm` to fewer decimals, or by having the form prefill from a rounded-truncated value, but nothing breaks. Everything else in the conversion library, the form/validation logic, the 409 rebase (sets version + fields onto the fresh server row), the mutation invalidation, the route params, and the modal state machines checks out correctly against the actual server contract. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## VERDICT: Minor issues - **`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-col gap-1.5">`, and `<label className="text-sm font-medium text-fg">`) is also verbatim in all three. Extracting a shared `fieldClasses` constant and/or a `<FieldShell label>` wrapper would prevent drift as more field types are added. Verified by reading all three files. Small. - **`web/src/components/gardens/GardenFormModal.tsx:32`** — `useUpdateGarden(garden?.id ?? 0)` uses `0` as a create-mode sentinel without explanation. It's harmless (the create branch never calls `update`), but a one-line comment or a more obviously-sentinel value would clarify intent for a future maintainer. Trivial. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I'll investigate the PR through the performance lens. Let me verify a few things. VERDICT: No material issues found Through the performance lens, this change is clean: - **List query** (`useHardens` → `api.get('/gardens')`): single fetch, no N+1. The grid maps over the result once; `formatCm`/`formatDimensions` do trivial arithmetic per card. No per-item follow-up requests. - **Mutations** (`useCreateGarden`/`useUpdateGarden`/`useDeleteGarden`): each invalidates the single `['gardens']` key, triggering one refetch of the list — the intended react-query pattern, not a redundant cascade. No optimistic updates promised, so no double-work. - **`GardenFormModal`**: calls `useCreateGarden()` and `useUpdateGarden(...)` unconditionally (required by the rules of hooks); both are lazy mutations, so the unused one does no work. `changeUnit` runs two `parseFloat` + two conversions on an explicit user action — not a hot path. - **`Modal`**: one `keydown` listener on `document`, cleaned up on unmount. No scroll listeners, no resize observers, no rAF loops. - **Inline `onEdit`/`onDelete` arrows** in `GardensPage`'s `.map` are recreated per render, but the list is small and `GardenCard` isn't memoized anyway — not a measurable cost here, and memoizing would be a micro-optimization. - **Unbounded list**: there's no pagination/limit on `GET /gardens`. For this app's scope (a user's own gardens) this is a reasonable scope decision, not a regression introduced by this PR, so I'm not flagging it as material. I verified the above by reading `web/src/lib/api.ts`, `web/src/pages/GardensPage.tsx`, and the new `lib/gardens.ts` / `lib/units.ts` / modal components in the diff. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings confirmed against the actual source. **VERDICT: Minor issues** - **`web/src/components/ui/Modal.tsx:21,32` — closing the modal during an in-flight mutation swallows the error.** `Modal` calls `onClose()` unconditionally on Escape (line 21) and on backdrop `mousedown` (line 32). The form/delete modals guard their *Cancel* buttons with `disabled={del.isPending}` / `disabled={pending}` (`GardenFormModal.tsx:146`, `DeleteGardenModal.tsx`), but Escape and the backdrop click are not guarded. While a save or delete is pending, the user can close the modal mid-flight: `GardensPage.tsx:53-55` renders the modal only while `dialog` is set and the close path calls `setDialog(null)`, unmounting the modal. The still-running `await mutateAsync(...)` later rejects and the `setFormError` / `setError` call lands on an unmounted component and is dropped, so the user never learns the garden wasn't saved/deleted; reopening the modal starts with a blank error state. Suggested fix: pass `pending` into `Modal` and no-op `onClose` (and skip the Escape listener / backdrop handler) while `pending` is true, mirroring the Cancel-button guard. - **`web/src/components/gardens/GardenFormModal.tsx:46-47` — `changeUnit` preserves non-numeric strings as-is.** If a dimension field is empty (`""`) or contains garbage (`"abc"`), `parseFloat` returns `NaN`, `Number.isFinite` is false, and `convert` returns the original string unchanged while `setUnit(next)` still runs. The physical-size-preservation invariant silently breaks: the displayed value no longer reflects the previously-entered size in the new unit, and on save the field is rejected by the `w <= 0`/`!Number.isFinite` check at line 61, producing a generic "Enter a name and a positive width and height" error rather than pointing at the offending field. Minor, but the unit-switch path has no recovery for an empty/garbage input. Suggested fix: when `!Number.isFinite(v)`, reset to the default (e.g. `String(displayFromCm(DEFAULT_METERS * 100, next))`) or leave the field empty and let the per-field `required` validation surface it. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** I reviewed this PR through the **security** lens, tracing tainted input from the UI through to the backend API and back. - **AuthZ / IDOR**: Every garden endpoint is behind `requireAuth`. The backend service (`internal/service/gardens.go`) enforces ownership via `requireGardenRole` / `effectiveGardenRole`. The frontend `Link` to `/gardens/$gardenId` uses the garden's own `id`, but the backend `GetGarden` re-verifies ownership, so no IDOR. - **Injection**: No SQL/command/template injection vectors. API calls use parameterized routes (`/gardens/${id}`) with string coercion of numeric IDs, and the backend uses parameterized DB queries. No raw HTML construction. - **XSS**: `garden.name` and `garden.notes` are rendered as React text nodes (JSX `{…}`), which React escapes automatically. No `dangerouslySetInnerHTML` usage. - **CSRF**: State-changing API requests rely on the `same-origin` session cookie. The backend attaches `SameSite=Lax` and an `Origin`-checking `csrfGuard` for mutating requests. - **Input validation**: The form validates positive finite dimensions and non-empty names. The backend additionally enforces length caps (`maxGardenNameLen = 200`, `maxGardenNotesLen = 10_000`) and a physical dimension range (`[1 cm, 100 m]`). - **Unsafe deserialization**: All API responses are parsed through zod schemas (`gardenSchema`, `userSchema`), which reject unexpected shapes. - **Secret leakage**: `errorMessage` surfaces only the server's user-facing message string; no raw stack traces or tokens are exposed. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** - **`web/src/components/ui/Modal.tsx:19-26`** — Focus theft on every parent re-render. The `useEffect` depends on `onClose`, which the parent (`GardensPage`) supplies as an inline arrow (`() => setDialog(null)`) that is recreated on every render. Any background re-render (e.g. React Query refetch on window focus) re-runs the effect, calling `cardRef.current?.focus()` and stealing focus from whichever input the user is currently typing in. **Fix:** Split the effect: register the Escape listener in one effect with a stable ref wrapper for `onClose`, and call `focus()` only on mount (empty deps). - **`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:** Make the modal respect the pending state. Either have `GardenFormModal` pass a guarded close handler (`if (pending) return; onClose()`) to `Modal`, or add an `allowClose` / `onClose` guard prop to `Modal` itself. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`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 both places. Fix: use `<Button variant="ghost" className="px-2.5 py-1 ...">` with overrides, or add a compact `size` prop to `Button`. - **`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 both places. Fix: use `<Button variant="ghost" className="px-2.5 py-1 ...">` with overrides, or add a compact `size` prop to `Button`. - **`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` wrapper) and use it in all three components. - **`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` wrapper) and use it in all three components. </details> <details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`web/src/components/ui/Modal.tsx:21`** — Pressing Escape unconditionally calls `onClose()`, even when the caller has disabled its Cancel button because a mutation is in flight. In `GardenFormModal` and `DeleteGardenModal`, a keyboard user can therefore dismiss the modal while `create`/`update`/`del` is pending, discarding unsaved create/edit data or hiding a delete-confirmation while the request continues. The same applies to backdrop clicks (`onMouseDown`). **Fix:** Have the callers pass a guarded close handler, e.g. `onClose={pending ? () => {} : () => setDialog(null)}`, or add an optional `canClose` prop to `Modal`. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-18 23:03:51 +00:00
Address Gadfly review on #8: modal focus/close, dim validation, dedup
Build image / build-and-push (push) Successful in 7s
3f61f07034
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit a1294b9c69 into main 2026-07-18 23:05:27 +00:00
steve deleted branch phase-2-gardens-ui 2026-07-18 23:05:27 +00:00
Sign in to join this conversation.