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.
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)
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
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>
🪰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>
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>
🟡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>
🟠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>
🟠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>
🟡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.
🪰 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>
🟡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.
🪰 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>
🟡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>
🟠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>
🟡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>
🔴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>
🔴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>
🟡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>
🟡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>
🟡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>
⚪Redundant z.ZodType 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.
🪰 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>
🟠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.
🪰 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>
🟠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…
🪰 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>
🟠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…
🪰 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>
⚪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>
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…
🪰 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>
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>
🟡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 ModaluseEffect 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>
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 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 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.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.
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.
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 oldunit 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: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.
⚡ 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 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.
🧯 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
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-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.
🧹 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: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 ModaluseEffect 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: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:
🧯 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-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.
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.conflictGardensafeParses 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.
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 ✓
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.
🧹 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: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.
⚡ 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 (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.
🧯 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-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.
opencode/kimi-k2.6:cloud (opencode) — Blocking 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: [91m[1mError: [0mUnexpected 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: [91m[1mError: [0mUnexpected 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>
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 main2026-07-18 23:05:27 +00:00
steve
deleted branch phase-2-gardens-ui2026-07-18 23:05:27 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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(metricm, imperialft/in). Deliberately minimal; #9's geometry lib consolidates.lib/gardens.ts— zodgardenSchema,useGardens/useCreateGarden/useUpdateGarden/useDeleteGarden(mutations invalidate the list), andconflictGarden()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 ownunit_pref, notes preview; the body links into/gardens/:idwith 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)
4).npx tsc --noEmitclean;vite buildsucceeds. (Screenshot shared in-session.)Out of scope
/gardens/:id, still a stub. Sharing UI / role badges (#17).🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 22:56:23Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 22 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -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 nameddelis unnecessarily terse. Fix: rename todeleteGardenormutation.🪰 Gadfly · advisory
@@ -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 hardcodedclassName="bg-red-600 text-white hover:bg-red-700"rather than the component exposing adangervariant. SinceButtonVariantinButton.tsx:4is 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
@@ -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 thefocus-visible:ring-2 focus-visible:ring-accent/40anddisabled:cursor-not-allowed disabled:opacity-60behaviour thatButtonprovides. If these actions ever need disabled states or if the design system changes, edits must be made in bo…🪰 Gadfly · advisory
@@ -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 thefocus-visible:ring-2 focus-visible:ring-accent/40anddisabled:cursor-not-allowed disabled:opacity-60behaviour thatButtonprovides. If these actions ever need disabled states or if the design system changes, edits must be made in bo…🪰 Gadfly · advisory
@@ -0,0 +29,4 @@export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose: () => void }) {const isEdit = !!gardenconst 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:32—useUpdateGarden(garden?.id ?? 0)passes a dummy0during creation. The hook is never called, but the?? 0is a leaky abstraction—future readers must infer it is safe. Fix: makeuseUpdateGardenacceptid?: numberand return a no-op mutation when absent, removing the magic placeholder.🪰 Gadfly · advisory
@@ -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 throughcmFromDisplay→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 hiddencmstate (or ref) for the true physical size and derive the display string from it, eliminating round-trip noise.🪰 Gadfly · advisory
@@ -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-64vs.internal/service/gardens.go:16-18,146-148— Client-side validation only checksw <= 0/h <= 0in the display unit before conversion, while the server enforces[minGardenCM=1, maxGardenCM=10_000](1 cm–100 m) on the converted value viavalidDimensionCM. A small value (e.g.0.001m) passes the client check but rounds to0cm throughcmFromMeters/cmFromFtIn(units.ts:12-19) and fails server validation; a value like `5…🪰 Gadfly · advisory
@@ -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 checksw > 0on the parsed float, but the API stores whole centimeters. A value such as0.004m (or0.01ft) is accepted as “positive” yetcmFromDisplay(0.004, 'metric')=Math.round(0.4)= 0 cm. The server rejects 0 cm on update (validDimensionCMrequiresv >= 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
@@ -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 callsString(displayFromCm(current.widthCm, current.unitPref))directly, duplicating what thedimStringhelper (lines 19-21) already does.dimString(current.widthCm, current.unitPref)would read better and keep the "cm → input string" rule in one place (dimStringacceptsnumber | undefined, so anumberarg is fine). Minor.🪰 Gadfly · advisory
@@ -0,0 +14,4 @@onClose: () => voidchildren: 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
useEffectdepends ononClose, which is recreated on every parent render (e.g.() => setDialog(null)inGardensPage). Each time the parent re-renders — such as during a background React Query refetch — the effect re-runs, the oldkeydownlistener is torn down and a new one added, andcardRef.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
@@ -0,0 +28,4 @@return (<divclassName="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:31andweb/src/components/gardens/GardenFormModal.tsx:102— The modal can be dismissed while a mutation is in flight. Escape and backdrop click unconditionally invokeonClose. 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 inGardenFormModalsets error state on an unmounted component and the user never sees the failure. **Fix…🪰 Gadfly · advisory
@@ -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— TheuseId()+id ?? name ?? generatedIdfallback chain is copied verbatim from the pre-existingTextField(theTextAreacomment is even a near-duplicate ofTextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. addingaria-describedbysupport or changing fallback priority—require touching three components. Fix: extract a tinyuseFieldId(id?: string, name?: string)hook (or a sharedFormControlwrappe…🪰 Gadfly · advisory
@@ -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(withTextArea.tsx:25andTextField.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
@@ -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— TheuseId()+id ?? name ?? generatedIdfallback chain is copied verbatim from the pre-existingTextField(theTextAreacomment is even a near-duplicate ofTextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. addingaria-describedbysupport or changing fallback priority—require touching three components. Fix: extract a tinyuseFieldId(id?: string, name?: string)hook (or a sharedFormControlwrap…🪰 Gadfly · advisory
@@ -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:9—const unitPrefSchema: z.ZodType<UnitPref> = z.enum(['metric', 'imperial'])carries an explicit annotation thatauth.ts(the sibling data layer) doesn't use;z.enumalready infers the literal union. The annotation is harmless but breaks the local convention for no real benefit. Trivial.🪰 Gadfly · advisory
@@ -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
widthCm: z.number()andheightCm: 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 intoformatDimensions. Defensive fix: usez.number().positive()for both.🪰 Gadfly · advisory
@@ -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:62— Redundant full-list refetch after every create/update.useCreateGardenanduseUpdateGardenboth receive the freshGardenrow from the server (api.post/api.patchreturn the parsed object), but theironSuccesshandlers ignore it and callqc.invalidateQueries({ queryKey: gardensKey }). That forces an extraGET /gardensround-trip even though the response already contains everything needed to update the cache. On slo…🪰 Gadfly · advisory
@@ -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:62— Redundant full-list refetch after every create/update.useCreateGardenanduseUpdateGardenboth receive the freshGardenrow from the server (api.post/api.patchreturn the parsed object), but theironSuccesshandlers ignore it and callqc.invalidateQueries({ queryKey: gardensKey }). That forces an extraGET /gardensround-trip even though the response already contains everything needed to update the cache. On slo…🪰 Gadfly · advisory
@@ -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 thatcmFromFtInandcmFromMetershave no callers anywhere in the repo outsideunits.tsitself (only used internally bycmFromDisplayat line 23). Minor unnecessary public surface.🪰 Gadfly · advisory
@@ -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-72—displayFromCmrounds 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
@@ -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:20vs: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. Usinggardens.isSuccess &&in both keeps them aligned and avoids relying ondatatruthiness while pending. Trivial.🪰 Gadfly · advisory
@@ -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 theModaluseEffectchurn noted above. Fix: define a stableconst closeDialog = useCallback(() => setDialog(null), [])and pass it to all three modals.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 23 findings (8 with multi-model agreement)
web/src/components/ui/Modal.tsx:17web/src/components/gardens/GardenCard.tsx:32web/src/components/gardens/GardenFormModal.tsx:66web/src/lib/gardens.ts:46web/src/components/gardens/GardenFormModal.tsx:32web/src/components/gardens/GardenFormModal.tsx:44web/src/components/ui/TextArea.tsx:14web/src/pages/GardensPage.tsx:5315 single-model findings (lower confidence)
web/src/components/ui/Modal.tsx:31web/src/lib/units.ts:27web/src/components/gardens/GardenCard.tsx:39web/src/lib/gardens.ts:15web/src/lib/gardens.ts:62web/src/components/gardens/DeleteGardenModal.tsx:33web/src/components/gardens/GardenFormModal.tsx:59web/src/components/gardens/GardenFormModal.tsx:89web/src/components/ui/Button.tsx:4web/src/components/ui/Select.tsx:14web/src/components/ui/Select.tsx:25web/src/components/gardens/DeleteGardenModal.tsx:10web/src/lib/gardens.ts:9web/src/lib/units.ts:12web/src/pages/GardensPage.tsx:20Per-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:
web/src/lib/api.ts, unchanged) usescredentials: 'same-origin'and relies on an HttpOnly session cookie — no tokens are exposed to JS, and this file isn't touched by the diff.web/src/router.tsx:82-87confirms/gardensis behindrequireAuth, which resolves/auth/mebefore renderingGardensPage— consistent with the PR's claim of consuming the #6 auth guard.gardenSchema(zod) validates every API response before use (lib/gardens.ts:17-27,39,47,58) and the 409-conflict payload is alsosafeParse'd (lib/gardens.ts:79-84) rather than trusted blindly — no unsafe deserialization.garden.name,garden.notes, error messages) are rendered via JSX interpolation, notdangerouslySetInnerHTML— grepped the wholeweb/srctree fordangerouslySetInnerHTML/__htmland found zero matches, so no injected-HTML/XSS vector here.GardenCard.tsx:15-16,GardenFormModal.tsxPATCH URL,DeleteGardenModal.tsxdelete call) all originate from the zod-validatedGarden.id: z.number(), not from raw user input — no path/template injection surface.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).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/focususeEffectdepends on[onClose](line 26), but every caller passes a fresh inline arrow:GardensPage.tsx:53-55passes() => setDialog(null)for all three dialog kinds.queryClient.ts:6-10confirmsrefetchOnWindowFocus: falseis set butrefetchOnReconnect/background refetches are left at their TanStack Query defaults (enabled), anduseGardens()subscribes to thegardensquery used on the very page that renders the modal. A background refetch (reconnect, refocus elsewhere, or any other unrelated state change onGardensPage) re-renders the page, creates a newonClosereference, and re-fires the effect — includingcardRef.current?.focus()on line 24 — which steals focus from whatever field the user is actively editing insideGardenFormModal/DeleteGardenModal, since those inputs render as descendants ofcardRef'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 memoizeonClose.web/src/components/gardens/GardenFormModal.tsx:59-64vs.internal/service/gardens.go:16-18,146-148— Client-side validation only checksw <= 0/h <= 0in the display unit before conversion, while the server enforces[minGardenCM=1, maxGardenCM=10_000](1 cm–100 m) on the converted value viavalidDimensionCM. A small value (e.g.0.001m) passes the client check but rounds to0cm throughcmFromMeters/cmFromFtIn(units.ts:12-19) and fails server validation; a value like500m 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-46mapsErrInvalidInputto a 400 with message"invalid input", andweb/src/lib/api.ts:130-133'serrorMessage()returns that server message (not the'Could not save changes.'/'Could not create the garden.'fallback, which only applies to non-ApiErrorthrows) — 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:32and:39— the footer Edit/Delete buttons are raw<button>elements with hand-rolled classes, not the sharedButtoncomponent. Confirmed againstweb/src/components/ui/Button.tsx:4-16:buttonClassescentralizesfocus-visible:ring-2 focus-visible:ring-accent/40,disabled:opacity-60, and transition styling thatButtonVariantonly exposes as'primary' | 'ghost'— neither raw button inGardenCardpicks 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 hardcodedclassName="bg-red-600 text-white hover:bg-red-700"rather than the component exposing adangervariant. SinceButtonVariantinButton.tsx:4is 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, namedinputIdinTextField) 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 thatcmFromFtInandcmFromMetershave no callers anywhere in the repo outsideunits.tsitself (only used internally bycmFromDisplayat 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'sonSuccesscallsqc.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 theidbeing removed. This costs an extra network round trip per mutation that aqc.setQueryDatacache 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 checksw <= 0/h <= 0on the raw display-unit value, not on the centimeter value that actually gets sent. BecausecmFromDisplayrounds to the nearest whole cm (web/src/lib/units.ts:12-24), a small positive input can round down to0. E.g. entering0.004in metric mode (w = 0.004 > 0, passes the check) converts viacmFromMeters→Math.round(0.004 * 100) = 0. The server'svalidDimensionCM(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'smaxGardenCM = 10_000(100 m) bound (internal/service/gardens.go:18) — only amin="0"attribute is set on the width/heightTextFields, nomax— 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-33combined withweb/src/pages/GardensPage.tsx:53-55andDeleteGardenModal.tsx/GardenFormModal.tsx—Modal's Escape-key handler and backdroponMouseDownboth callonClose()unconditionally, with no way for the caller to signal "a mutation is in flight." InGardensPage,onCloseis() => setDialog(null), which unmounts the modal component immediately.DeleteGardenModal/GardenFormModaldisable their own Cancel/Save buttons viadisabled={pending}/disabled={del.isPending}but never guard the Modal's own dismiss paths. If a user presses Escape (or clicks the backdrop) whiledel.mutateAsync/create.mutateAsync/update.mutateAsyncis still pending, the component unmounts, and the subsequentsetError(...)/setVersion(...)/setConflict(...)/onClose()calls in theawaitcontinuation 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 acloseDisabled/pendingprop intoModalto 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:
garden.name,garden.notes) are rendered as React text children (<h3>,<p>), which auto-escape. NodangerouslySetInnerHTML, nohref/srcinterpolation from user data./gardens/${id}(gardens.ts:54,62,68) and/gardens/${gardenId}(GardenCard.tsxviaparams={{ gardenId: String(garden.id) }}).idis constrained bygardenSchema(z.number()) before it ever reaches these call sites, so it can't carry path segments or query strings.GardenFormModal'suseUpdateGarden(garden?.id ?? 0)likewise only ever passes a zod-validated number (the0is unused in the create path).API_BASE + '/gardens...'base.buildUrl(api.ts:47) only interpolates the validated numeric id, and query params are passed throughURLSearchParams.z.array(gardenSchema).parse(...)/gardenSchema.parse(...)and the 409 body viagardenSchema.safeParse(conflictGarden), so malformed/conflicting payloads are rejected rather than trusted.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.GardenFormModalvalidates name non-empty and positive finite dimensions before sending (onSubmit); this is defense-in-depth, not the trust boundary (the API re-validates).notesis 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.
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)formatCm,inchesisMath.roundof a value in[0, 12)(sincefeet = floor(totalInches/12)), so it can reach at most 12, never 13; theif (inches === 12)carry branch covers exactly that case. No off-by-one. (units.ts:38-42)displayFromCm(122, imperial) = round(4.0026 × 100)/100 = 4, so the field shows4. (units.ts:27-31)changeUnitconvertsvfrom the oldunitto cm then tonext, and all threesetStates 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)version/name/unit/width/height/notesfrom the server's fresh row, computing width/height fromcurrent.unitPref(not stale state), keeps the modal open, and leavespendingfalse (the rejectedmutateAsynchas settled) so the user can re-save against the new version. A 409 on create without acurrentGarden shape falls throughconflictGarden'ssafeParseto a normal form error — graceful. (GardenFormModal.tsx:81-96,gardens.ts:73-84)DEFAULT_METERS = 10is 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)renders0′ 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:42—Buttonexposes onlyprimary/ghostvariants (line 4), so the delete-confirm button inDeleteGardenModalinlinesbg-red-600 text-white hover:bg-red-700, whileGardenCard'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 adangervariant tobuttonClasseswould let both call sites stop hand-rolling red classes. Minor.web/src/components/gardens/GardenFormModal.tsx:89-90— The 409 rebase path callsString(displayFromCm(current.widthCm, current.unitPref))directly, duplicating what thedimStringhelper (lines 19-21) already does.dimString(current.widthCm, current.unitPref)would read better and keep the "cm → input string" rule in one place (dimStringacceptsnumber | undefined, so anumberarg is fine). Minor.web/src/pages/GardensPage.tsx:20vs: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. Usinggardens.isSuccess &&in both keeps them aligned and avoids relying ondatatruthiness while pending. Trivial.web/src/pages/GardensPage.tsx:53-55— The three modal lines each repeatonClose={() => setDialog(null)}. A singlecloseclosure (or switching the modals ondialog?.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 thatauth.ts(the sibling data layer) doesn't use;z.enumalready 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:
useGardensis a singleGET /gardenscall; mutations invalidate exactly one query key (['gardens']). Card rendering is a single.mapover the result array — no per-card fetches.gardenslist only; no global invalidations or over-broad refetches.formatCm/formatDimensionsrun once per card render (a handful of cards); conversion math is trivial. No reason to memoize at this scale.conflictGardenonly fires on the error path of a save (not hot), and does a singlesafeParse— fine.GardenFormModal.changeUnitround-trips aparseFloat→cmFromDisplay→displayFromCmon unit switch; this is a user-initiated event (not a render or list loop), so the cost is irrelevant.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 thedisabled-during-pending guard, so a modal can be dismissed mid-mutation. TheModalcloses on Escape (line 21) or a backdroponMouseDown(lines 31-33) unconditionally, whileDeleteGardenModalandGardenFormModaldeliberately setdisabled={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. ForDeleteGardenModal(DeleteGardenModal.tsx:13-21,32-38) this is the most surprising: the dialog vanishes as if cancelled, butdel.mutateAsynckeeps running and itsonSuccess(gardens.ts:68) still invalidates the list — the garden is removed with no confirmation that the user actually wanted to proceed. ForGardenFormModal(GardenFormModal.tsx:54-97,146-149), the in-flightupdate.mutateAsync/create.mutateAsyncresolves after the component has unmounted; React 18 tolerates the latersetStatecalls in theonSubmitclosure, but the success path (onClose()) and the conflict-rebase path run against an unmounted form, so a 409 that arrives after Escape would callsetVersion/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 acloseableflag (or haveonCloseearly-return when the relevant mutationisPending), or inModalaccept adisabledprop 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:
garden.name,garden.notes, error messages) are rendered via React JSX, which escapes them automatically. NodangerouslySetInnerHTMLor raw HTML insertion is introduced.numberand interpolated into hardcoded route templates (/gardens/${id}). Theidis zod-validated asz.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.apiwrapper to/api/v1/*. No user-controlled URLs are fetched.gardenSchema.parse/safeParse). The conflict-rebase path explicitlysafeParses the server-providedcurrentrow before adopting it into form state.credentials: 'same-origin'to send the HttpOnly session cookie, with standard JSON content-type headers. No tokens or credentials are exposed in the code.🎯 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. TheuseEffectdepends ononClose, which is an inline arrow (() => setDialog(null)) recreated byGardensPageon every render. When react-query background-refetches/gardens,GardensPagere-renders,onCloseidentity changes, the effect re-runs, andcardRef.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
onCloseinuseCallbackinGardensPage.web/src/lib/units.ts:27-31/web/src/components/gardens/GardenFormModal.tsx:71-72—displayFromCmrounds 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 checksw > 0on the parsed float, but the API stores whole centimeters. A value such as0.004m (or0.01ft) is accepted as “positive” yetcmFromDisplay(0.004, 'metric')=Math.round(0.4)= 0 cm. The server rejects 0 cm on update (validDimensionCMrequiresv >= 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) >= 1instead ofw > 0.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/ui/Modal.tsx:19-26— TheuseEffectdependency array is[onClose]. BecauseGardensPagepasses fresh() => setDialog(null)lambdas on every render, thekeydownlistener 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 readcloseRef.currentinside the handler) so the effect only runs once.web/src/components/gardens/GardenFormModal.tsx:32—useUpdateGarden(garden?.id ?? 0)passes a dummy0during creation. The hook is never called, but the?? 0is a leaky abstraction—future readers must infer it is safe.Fix: make
useUpdateGardenacceptid?: numberand return a no-op mutation when absent, removing the magic placeholder.web/src/components/gardens/GardenFormModal.tsx:44-52— Unit switching goes throughcmFromDisplay→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
cmstate (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 nameddelis unnecessarily terse.Fix: rename to
deleteGardenormutation.web/src/pages/GardensPage.tsx:53-55— Three inline() => setDialog(null)lambdas recreated each render trigger theModaluseEffectchurn 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:62— Redundant full-list refetch after every create/update.useCreateGardenanduseUpdateGardenboth receive the freshGardenrow from the server (api.post/api.patchreturn the parsed object), but theironSuccesshandlers ignore it and callqc.invalidateQueries({ queryKey: gardensKey }). That forces an extraGET /gardensround-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. ForuseDeleteGardeninvalidation 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
useEffectdepends ononClose, but callers pass an inline arrow (() => setDialog(null)fromGardensPage) so the reference changes every time the parent re-renders—e.g. on a backgrounduseGardensrefetch or window-focus refresh. The effect tears down and re-adds a documentkeydownlistener, and callscardRef.current?.focus(), which can pull focus away from an input the user is typing into.Fix: store
onClosein a ref so the listener is registered once and never re-binds:🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/components/ui/Modal.tsx:19-26 — The
useEffectdepends ononClose, which is recreated on every parent render (e.g.() => setDialog(null)inGardensPage). Each time the parent re-renders — such as during a background React Query refetch — the effect re-runs, the oldkeydownlistener is torn down and a new one added, andcardRef.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()andheightCm: 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 intoformatDimensions. Defensive fix: usez.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:
garden.name,garden.notes, formname/notes) are rendered as React text content (auto-escaped) — nodangerouslySetInnerHTML/innerHTML/evalanywhere in the new files (verified via grep overweb/src).String(garden.id)whereidis az.number()from the validatedgardenSchema, and API paths/gardens/${id}use the same numericid. No user string is interpolated into a URL.parseFloatresults are checked withNumber.isFiniteand> 0before conversion (GardenFormModal.tsx:54-58);cmFromDisplayonly rounds, no further trust. Server-side validation is out of scope but the client doesn't introduce unbounded sink exposure.apiFetchsends the HttpOnly session cookie withcredentials: 'same-origin'; mutations hit the versioned/api/v1/gardensendpoints whose authz is owned by the API (#7) and route guard (#6), not this PR. No new unauthenticated surface introduced.conflictGardensafeParses the server-suppliedcurrentrow throughgardenSchemabefore rebasing — malformed conflict bodies are ignored rather than trusted.errorMessageare 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:
cmFromFtIn(4, 0) = round(48 × 2.54) = 122andcmFromFtIn(8, 0) = round(96 × 2.54) = 244✓ (acceptance criterion holds)formatCm(122, 'imperial')→"4′ 0″"andformatCm(244, 'imperial')→"8′ 0″"✓ (card display){error, current}matchesconflictGarden's parsing (internal/api/errors.go:57-62) ✓gardenSchemafield names matchdomain.GardenJSON tags exactly ✓useUpdateGarden/useCreateGardencorrectly sendversiononly on update; server requiresmin=1and the form's edit-modeversionstarts atgarden.version ≥ 1✓changeUnitround-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 usesdisplayFromCm(rounds to 2 decimals), while the card usesformatCm. For an 8 ft garden stored as 244 cm (becausecmFromFtIn(8)rounds 243.84 → 244),displayFromCm(244, 'imperial')=Math.round(8.0052 × 100)/100= 8.01, so the Edit form's width field shows8.01instead of8. 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 roundingdisplayFromCmto 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(withTextArea.tsx:25andTextField.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 sharedfieldClassesconstant 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)uses0as a create-mode sentinel without explanation. It's harmless (the create branch never callsupdate), 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:
useHardens→api.get('/gardens')): single fetch, no N+1. The grid maps over the result once;formatCm/formatDimensionsdo trivial arithmetic per card. No per-item follow-up requests.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: callsuseCreateGarden()anduseUpdateGarden(...)unconditionally (required by the rules of hooks); both are lazy mutations, so the unused one does no work.changeUnitruns twoparseFloat+ two conversions on an explicit user action — not a hot path.Modal: onekeydownlistener ondocument, cleaned up on unmount. No scroll listeners, no resize observers, no rAF loops.onEdit/onDeletearrows inGardensPage's.mapare recreated per render, but the list is small andGardenCardisn't memoized anyway — not a measurable cost here, and memoizing would be a micro-optimization.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 newlib/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.ModalcallsonClose()unconditionally on Escape (line 21) and on backdropmousedown(line 32). The form/delete modals guard their Cancel buttons withdisabled={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-55renders the modal only whiledialogis set and the close path callssetDialog(null), unmounting the modal. The still-runningawait mutateAsync(...)later rejects and thesetFormError/setErrorcall 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: passpendingintoModaland no-oponClose(and skip the Escape listener / backdrop handler) whilependingis true, mirroring the Cancel-button guard.web/src/components/gardens/GardenFormModal.tsx:46-47—changeUnitpreserves non-numeric strings as-is. If a dimension field is empty ("") or contains garbage ("abc"),parseFloatreturnsNaN,Number.isFiniteis false, andconvertreturns the original string unchanged whilesetUnit(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 thew <= 0/!Number.isFinitecheck 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-fieldrequiredvalidation 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.
requireAuth. The backend service (internal/service/gardens.go) enforces ownership viarequireGardenRole/effectiveGardenRole. The frontendLinkto/gardens/$gardenIduses the garden's ownid, but the backendGetGardenre-verifies ownership, so no IDOR./gardens/${id}) with string coercion of numeric IDs, and the backend uses parameterized DB queries. No raw HTML construction.garden.nameandgarden.notesare rendered as React text nodes (JSX{…}), which React escapes automatically. NodangerouslySetInnerHTMLusage.same-originsession cookie. The backend attachesSameSite=Laxand anOrigin-checkingcsrfGuardfor mutating requests.maxGardenNameLen = 200,maxGardenNotesLen = 10_000) and a physical dimension range ([1 cm, 100 m]).gardenSchema,userSchema), which reject unexpected shapes.errorMessagesurfaces 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. TheuseEffectdepends ononClose, 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, callingcardRef.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 callfocus()only on mount (empty deps).web/src/components/ui/Modal.tsx:31andweb/src/components/gardens/GardenFormModal.tsx:102— The modal can be dismissed while a mutation is in flight. Escape and backdrop click unconditionally invokeonClose. 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 inGardenFormModalsets error state on an unmounted component and the user never sees the failure.Fix: Make the modal respect the pending state. Either have
GardenFormModalpass a guarded close handler (if (pending) return; onClose()) toModal, or add anallowClose/onCloseguard prop toModalitself.🧹 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 thefocus-visible:ring-2 focus-visible:ring-accent/40anddisabled:cursor-not-allowed disabled:opacity-60behaviour thatButtonprovides. 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 compactsizeprop toButton.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 thefocus-visible:ring-2 focus-visible:ring-accent/40anddisabled:cursor-not-allowed disabled:opacity-60behaviour thatButtonprovides. 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 compactsizeprop toButton.web/src/components/ui/TextArea.tsx:14— TheuseId()+id ?? name ?? generatedIdfallback chain is copied verbatim from the pre-existingTextField(theTextAreacomment is even a near-duplicate ofTextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. addingaria-describedbysupport or changing fallback priority—require touching three components. Fix: extract a tinyuseFieldId(id?: string, name?: string)hook (or a sharedFormControlwrapper) and use it in all three components.web/src/components/ui/Select.tsx:14— TheuseId()+id ?? name ?? generatedIdfallback chain is copied verbatim from the pre-existingTextField(theTextAreacomment is even a near-duplicate ofTextField’s). Adding two more copies of this three-line pattern makes future changes—e.g. addingaria-describedbysupport or changing fallback priority—require touching three components. Fix: extract a tinyuseFieldId(id?: string, name?: string)hook (or a sharedFormControlwrapper) and use it in all three components.⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/components/ui/Modal.tsx:21— Pressing Escape unconditionally callsonClose(), even when the caller has disabled its Cancel button because a mutation is in flight. InGardenFormModalandDeleteGardenModal, a keyboard user can therefore dismiss the modal whilecreate/update/delis 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 optionalcanCloseprop toModal.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.