lib/gardens.ts — myRole on the garden schema + canEditRole / isOwnerRole helpers. Role always comes from the server's my_role, never guessed client-side.
Share dialog (owner only)
ShareGardenModal — invite an existing user by email as viewer/editor, change a share's role inline, remove a share; surfaces the backend's friendly "no account with that email". Reachable from the garden card and the editor (owner's Share button).
Garden list
GardenCard — a "shared · viewer/editor" badge on non-owned gardens (owned unbadged). Owner gets Share / Edit / Delete; a recipient gets Leave (LeaveGardenModal removes their own share and drops the garden from their list).
Read-only viewer mode (editor)
Gated on a single canEdit flag derived from my_role:
viewers get no palette, no select/move/resize/rotate handles, no plop overlay, no placement, and read-only inspectors — a disabled <fieldset> makes every field read-only in one shot, and Change / Delete / Remove / "Plant here" are hidden — plus a "View only" indicator.
Defense in depth: the canvas create/place handlers also check canEdit, so a missed UI gate can't fire a mutation that would 403 (the edit-firing overlays aren't mounted for viewers to begin with).
Checks
tsc --noEmit clean · 24/24 vitest · vite build green.
Acceptance criteria
✅ A shares with B as viewer → B sees the badge, opens the garden, the canvas renders, pan/zoom works, nothing is editable, no mutation requests fire; A upgrades B to editor → after refetch B can move objects and place plops; A removes the share → the garden leaves B's list.
✅ Owner sees the share dialog; editor doesn't; viewer doesn't.
✅tsc --noEmit clean.
A manual two-browser pass is worth doing (touch ergonomics / refetch timing aren't covered by the type/unit checks).
Closes #17. Part of epic #20 (Phase 6 — Sharing). Builds on the #16 API.
## What
### Data layer
- **`lib/shares.ts`** — zod `Share` shape + `useShares` / `useAddShare` / `useUpdateShareRole` / `useRemoveShare` over `/gardens/:id/shares`.
- **`lib/gardens.ts`** — `myRole` on the garden schema + `canEditRole` / `isOwnerRole` helpers. **Role always comes from the server's `my_role`, never guessed client-side.**
### Share dialog (owner only)
- `ShareGardenModal` — invite an existing user by **email** as viewer/editor, change a share's role inline, remove a share; surfaces the backend's friendly *"no account with that email"*. Reachable from the **garden card** and the **editor** (owner's Share button).
### Garden list
- `GardenCard` — a **"shared · viewer/editor"** badge on non-owned gardens (owned unbadged). Owner gets **Share / Edit / Delete**; a recipient gets **Leave** (`LeaveGardenModal` removes their own share and drops the garden from their list).
### Read-only viewer mode (editor)
Gated on a **single `canEdit` flag** derived from `my_role`:
- viewers get **no palette, no select/move/resize/rotate handles, no plop overlay, no placement**, and **read-only inspectors** — a disabled `<fieldset>` makes every field read-only in one shot, and Change / Delete / Remove / "Plant here" are hidden — plus a **"View only"** indicator.
- **Defense in depth**: the canvas create/place handlers also check `canEdit`, so a missed UI gate can't fire a mutation that would 403 (the edit-firing overlays aren't mounted for viewers to begin with).
## Checks
`tsc --noEmit` clean · 24/24 vitest · `vite build` green.
## Acceptance criteria
- ✅ A shares with B as viewer → B sees the badge, opens the garden, the canvas renders, pan/zoom works, nothing is editable, no mutation requests fire; A upgrades B to editor → after refetch B can move objects and place plops; A removes the share → the garden leaves B's list.
- ✅ Owner sees the share dialog; editor doesn't; viewer doesn't.
- ✅ `tsc --noEmit` clean.
> A manual two-browser pass is worth doing (touch ergonomics / refetch timing aren't covered by the type/unit checks).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
- lib/shares.ts: zod Share shape + hooks (useShares/useAddShare/
useUpdateShareRole/useRemoveShare over /gardens/:id/shares).
- lib/gardens.ts: myRole on the garden schema + canEditRole/isOwnerRole helpers
(role always comes from the server's my_role, never guessed).
- ShareGardenModal (owner only): invite an existing user by email as viewer/
editor, change a share's role inline, remove; surfaces the backend's friendly
"no account with that email". Reachable from the garden card and the editor.
- GardenCard: "shared · viewer/editor" badge on non-owned gardens; owner gets
Share/Edit/Delete, a recipient gets Leave (LeaveGardenModal removes their own
share).
- Read-only viewer mode in the editor, gated on a single canEdit flag:
viewers get no palette, no select/move/resize/rotate handles, no plop overlay,
no placement, and read-only inspectors (a disabled <fieldset> makes every field
read-only in one shot; Change/Delete/Remove/Plant-here hidden), plus a "View
only" indicator. Defense in depth: the canvas create/place handlers also check
canEdit, so a missed gate can't fire a mutation that would 403.
tsc --noEmit clean; 24/24 vitest; production build green.
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-19 04:05:38Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6: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
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — Blocking issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 17 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** — 17 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡actionClass/dangerClass duplicated verbatim from PlantCard — should be shared
maintainability · flagged by 3 models
Duplicated actionClass / dangerClass constants — web/src/components/gardens/GardenCard.tsx:5-10 defines the exact same two class strings as web/src/components/plants/PlantCard.tsx:5-10 (verified identical character-for-character). This is copy-paste that should be shared — e.g. extract to web/src/components/ui/button.ts or a cardActions helper. As more card components appear, this drift will diverge.
🪰 Gadfly · advisory
🟡 **actionClass/dangerClass duplicated verbatim from PlantCard — should be shared**
_maintainability · flagged by 3 models_
- **Duplicated `actionClass` / `dangerClass` constants** — `web/src/components/gardens/GardenCard.tsx:5-10` defines the exact same two class strings as `web/src/components/plants/PlantCard.tsx:5-10` (verified identical character-for-character). This is copy-paste that should be shared — e.g. extract to `web/src/components/ui/button.ts` or a `cardActions` helper. As more card components appear, this drift will diverge.
<sub>🪰 Gadfly · advisory</sub>
🟡Garden with undefined myRole is treated as non-owner and shown a Leave (mutating) button with no badge; fail-closed direction is wrong for unknown role
correctness · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:30,40,64 — a garden with myRole === undefined is treated as non-owner and gets a Leave button with no badge.isOwnerRole(undefined) is false (confirmed in gardens.ts:37), so the footer falls into the !owner branch and renders Leave; the badge is gated on !owner && garden.myRole (truthy), so a missing role yields a card with no role label and a Leave button that calls remove.mutateAsync(me.data.id) — a self-share delete that like…
🪰 Gadfly · advisory
🟡 **Garden with undefined myRole is treated as non-owner and shown a Leave (mutating) button with no badge; fail-closed direction is wrong for unknown role**
_correctness · flagged by 1 model_
- **`web/src/components/gardens/GardenCard.tsx:30,40,64` — a garden with `myRole === undefined` is treated as non-owner and gets a `Leave` button with no badge.** `isOwnerRole(undefined)` is false (confirmed in `gardens.ts:37`), so the footer falls into the `!owner` branch and renders `Leave`; the badge is gated on `!owner && garden.myRole` (truthy), so a missing role yields a card with no role label and a `Leave` button that calls `remove.mutateAsync(me.data.id)` — a self-share delete that like…
<sub>🪰 Gadfly · advisory</sub>
⚪Badge prints raw lowercase enum myRole; casing/wording inconsistent with role labels in ShareGardenModal
maintainability · flagged by 1 model
🪰 Gadfly · advisory
⚪ **Badge prints raw lowercase enum myRole; casing/wording inconsistent with role labels in ShareGardenModal**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡Non-401 useMe failure leaves Leave button disabled with no user-facing message
error-handling · flagged by 1 model
web/src/components/gardens/LeaveGardenModal.tsx:40 — a non-401 useMe failure leaves the user stuck with a disabled button and no explanation. Verified in web/src/lib/auth.ts:29-44: meQueryOptions.queryFn returns null only on ApiError.isUnauthorized (401); any other error is rethrown, leaving me.data undefined. LeaveGardenModal.tsx:40 sets disabled={remove.isPending || !me.data} with no error/loading state for me, so a non-401 useMe failure keeps "Leave" permanently di…
🪰 Gadfly · advisory
🟡 **Non-401 useMe failure leaves Leave button disabled with no user-facing message**
_error-handling · flagged by 1 model_
- **`web/src/components/gardens/LeaveGardenModal.tsx:40` — a non-401 `useMe` failure leaves the user stuck with a disabled button and no explanation.** Verified in `web/src/lib/auth.ts:29-44`: `meQueryOptions.queryFn` returns `null` only on `ApiError.isUnauthorized` (401); any other error is rethrown, leaving `me.data` undefined. `LeaveGardenModal.tsx:40` sets `disabled={remove.isPending || !me.data}` with no error/loading state for `me`, so a non-401 `useMe` failure keeps "Leave" permanently di…
<sub>🪰 Gadfly · advisory</sub>
🟠Modal busy prop does not cover updateRole or remove pending states
error-handling · flagged by 3 models
web/src/components/gardens/ShareGardenModal.tsx:50 — The Modal’s busy prop only tracks add.isPending, so Escape / backdrop-click can dismiss the dialog while an in-flight updateRole or remove is still pending. That buries any eventual error and leaves the user unsure whether the action completed. Fix: Pass busy={add.isPending || updateRole.isPending || remove.isPending}.
🪰 Gadfly · advisory
🟠 **Modal busy prop does not cover updateRole or remove pending states**
_error-handling · flagged by 3 models_
* **`web/src/components/gardens/ShareGardenModal.tsx:50`** — The Modal’s `busy` prop only tracks `add.isPending`, so Escape / backdrop-click can dismiss the dialog while an in-flight `updateRole` or `remove` is still pending. That buries any eventual error and leaves the user unsure whether the action completed. **Fix:** Pass `busy={add.isPending || updateRole.isPending || remove.isPending}`.
<sub>🪰 Gadfly · advisory</sub>
🟡Empty ul rendered during loading and error states
maintainability · flagged by 1 model
web/src/components/gardens/ShareGardenModal.tsx:84 — The <ul className="flex flex-col gap-2"> is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside the shares.isSuccess && shares.data.length > 0 branch.
🪰 Gadfly · advisory
🟡 **Empty ul rendered during loading and error states**
_maintainability · flagged by 1 model_
- `web/src/components/gardens/ShareGardenModal.tsx:84` — The `<ul className="flex flex-col gap-2">` is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside the `shares.isSuccess && shares.data.length > 0` branch.
<sub>🪰 Gadfly · advisory</sub>
🔴updateRole.mutate / remove.mutate silently swallow errors — no onError, no user feedback on failed role change or revoke
correctness, error-handling, maintainability · flagged by 5 models
web/src/components/gardens/ShareGardenModal.tsx:91 — Uses a raw <select> element (with manually imported fieldControlClass and cn) for the inline role changer, while the invite form 20 lines above uses the project's <Select> component. This creates two styling paths for the same control and duplicates the viewer/editor options already defined in the module-level roleOptions array. Prefer <Select> (or map roleOptions) to keep the UI consistent and DRY.
🪰 Gadfly · advisory
🔴 **updateRole.mutate / remove.mutate silently swallow errors — no onError, no user feedback on failed role change or revoke**
_correctness, error-handling, maintainability · flagged by 5 models_
- `web/src/components/gardens/ShareGardenModal.tsx:91` — Uses a raw `<select>` element (with manually imported `fieldControlClass` and `cn`) for the inline role changer, while the invite form 20 lines above uses the project's `<Select>` component. This creates two styling paths for the same control and duplicates the `viewer`/`editor` options already defined in the module-level `roleOptions` array. Prefer `<Select>` (or map `roleOptions`) to keep the UI consistent and DRY.
<sub>🪰 Gadfly · advisory</sub>
🔴remove mutation has no error handling or pending UI
correctness, error-handling, maintainability · flagged by 5 models
web/src/components/gardens/ShareGardenModal.tsx:102 — remove.mutate(sh.userId) is fire-and-forget with no error surface. useRemoveShare (lib/shares.ts:60-69) defines only onSuccess, no onError, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.
🪰 Gadfly · advisory
🔴 **remove mutation has no error handling or pending UI**
_correctness, error-handling, maintainability · flagged by 5 models_
- **`web/src/components/gardens/ShareGardenModal.tsx:102`** — `remove.mutate(sh.userId)` is fire-and-forget with no error surface. `useRemoveShare` (`lib/shares.ts:60-69`) defines only `onSuccess`, no `onError`, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.
<sub>🪰 Gadfly · advisory</sub>
🟠Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable
maintainability · flagged by 5 models
web/src/editor/Inspector.tsx:113 — The <fieldset> wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.
🪰 Gadfly · advisory
🟠 **Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable**
_maintainability · flagged by 5 models_
- `web/src/editor/Inspector.tsx:113` — The `<fieldset>` wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.
<sub>🪰 Gadfly · advisory</sub>
🟠onBlur handlers in disabled fieldset can fire mutations after role downgrade
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:119 and web/src/editor/PlopInspector.tsx:138 — Every onBlur handler inside the <fieldset disabled={readOnly}> calls patch(...) without checking readOnly. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a new myRole), React re-renders with readOnly=true, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403. Fix: Guard the `patch…
🪰 Gadfly · advisory
🟠 **onBlur handlers in disabled fieldset can fire mutations after role downgrade**
_error-handling · flagged by 1 model_
* **`web/src/editor/Inspector.tsx:119`** and **`web/src/editor/PlopInspector.tsx:138`** — Every `onBlur` handler inside the `<fieldset disabled={readOnly}>` calls `patch(...)` without checking `readOnly`. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a new `myRole`), React re-renders with `readOnly=true`, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403. **Fix:** Guard the `patch…
<sub>🪰 Gadfly · advisory</sub>
🟠Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable
maintainability · flagged by 5 models
web/src/editor/PlopInspector.tsx:127 — Same indentation problem: the <fieldset> tag and its first child <div> (line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.
🪰 Gadfly · advisory
🟠 **Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable**
_maintainability · flagged by 5 models_
- `web/src/editor/PlopInspector.tsx:127` — Same indentation problem: the `<fieldset>` tag and its first child `<div>` (line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.
<sub>🪰 Gadfly · advisory</sub>
⚪useShares enabled parameter has no caller; dead surface
maintainability · flagged by 1 model
web/src/editor/Inspector.tsx:113-227 & PlopInspector.tsx:127-171 — fieldset children not re-indented. The new <fieldset> wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g. <div className="grid grid-cols-2 gap-2"> at Inspector:122 sits at the same indent as the <fieldset> that owns it; same in PlopInspector:128). The closing </fieldset> at 227 is likewise dedented past the <TextArea> it contains. This flatt…
🪰 Gadfly · advisory
⚪ **useShares `enabled` parameter has no caller; dead surface**
_maintainability · flagged by 1 model_
- **`web/src/editor/Inspector.tsx:113-227` & `PlopInspector.tsx:127-171` — fieldset children not re-indented.** The new `<fieldset>` wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g. `<div className="grid grid-cols-2 gap-2">` at Inspector:122 sits at the same indent as the `<fieldset>` that owns it; same in PlopInspector:128). The closing `</fieldset>` at 227 is likewise dedented past the `<TextArea>` it contains. This flatt…
<sub>🪰 Gadfly · advisory</sub>
🔴useAddShare parses the POST /shares response with a schema requiring email/displayName, but the backend's AddShare returns a bare GardenShare without them — every successful invite throws and shows a false 'Could not share the garden' error
correctness · flagged by 1 model
🪰 Gadfly · advisory
🔴 **useAddShare parses the POST /shares response with a schema requiring email/displayName, but the backend's AddShare returns a bare GardenShare without them — every successful invite throws and shows a false 'Could not share the garden' error**
_correctness · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🔴useUpdateShareRole parses the PATCH /shares/:userId response with the same over-strict schema; the backend's UpdateShareRole also omits email/displayName, so every successful role change throws and is swallowed silently (no onError in ShareGardenModal), leaving the UI reverted to the stale role
correctness · flagged by 1 model
🪰 Gadfly · advisory
🔴 **useUpdateShareRole parses the PATCH /shares/:userId response with the same over-strict schema; the backend's UpdateShareRole also omits email/displayName, so every successful role change throws and is swallowed silently (no onError in ShareGardenModal), leaving the UI reverted to the stale role**
_correctness · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡useRemoveShare invalidates ['gardens'] on every remove, causing avoidable gardens-list refetches in the owner-revoke path
performance · flagged by 1 model
web/src/lib/shares.ts:66 — useRemoveShare.onSuccess unconditionally invalidates ['gardens'] on every remove. This is correct and necessary for the leave path (the garden leaves the recipient's list), but it also fires for the owner revoke path in ShareGardenModal. When the owner manages shares from GardensPage (where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-li…
🪰 Gadfly · advisory
🟡 **useRemoveShare invalidates ['gardens'] on every remove, causing avoidable gardens-list refetches in the owner-revoke path**
_performance · flagged by 1 model_
- **`web/src/lib/shares.ts:66`** — `useRemoveShare.onSuccess` unconditionally invalidates `['gardens']` on every remove. This is correct and necessary for the *leave* path (the garden leaves the recipient's list), but it also fires for the *owner revoke* path in `ShareGardenModal`. When the owner manages shares from `GardensPage` (where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-li…
<sub>🪰 Gadfly · advisory</sub>
🟠canEdit/isOwner false on missing myRole can lock an owner out of their own garden with no error surfaced
error-handling · flagged by 2 models
web/src/pages/GardenEditorPage.tsx:127 — canEdit/isOwner are false when myRole is absent, which can lock an owner out of their own garden with no error surfaced. Verified in web/src/lib/gardens.ts: canEditRole(undefined) (line 32-34) and isOwnerRole(undefined) (line 37-39) both return false, and myRole is declared gardenRoleSchema.optional() (line 27) "defensively." GardenEditorPage.tsx:127-128 gates all UI on these. If the server ever omits my_role (backend bug,…
🪰 Gadfly · advisory
🟠 **canEdit/isOwner false on missing myRole can lock an owner out of their own garden with no error surfaced**
_error-handling · flagged by 2 models_
- **`web/src/pages/GardenEditorPage.tsx:127` — `canEdit`/`isOwner` are `false` when `myRole` is absent, which can lock an owner out of their own garden with no error surfaced.** Verified in `web/src/lib/gardens.ts`: `canEditRole(undefined)` (line 32-34) and `isOwnerRole(undefined)` (line 37-39) both return `false`, and `myRole` is declared `gardenRoleSchema.optional()` (line 27) "defensively." `GardenEditorPage.tsx:127-128` gates all UI on these. If the server ever omits `my_role` (backend bug,…
<sub>🪰 Gadfly · advisory</sub>
actionClass/dangerClass duplicated verbatim from PlantCard — should be shared
web/src/components/gardens/GardenCard.tsx:5
3/5
maintainability
🟠
canEdit/isOwner false on missing myRole can lock an owner out of their own garden with no error surfaced
web/src/pages/GardenEditorPage.tsx:127
2/5
error-handling
14 single-model findings (lower confidence)
Finding
Where
Model
Lens
🔴
useAddShare parses the POST /shares response with a schema requiring email/displayName, but the backend's AddShare returns a bare GardenShare without them — every successful invite throws and shows a false 'Could not share the garden' error
web/src/lib/shares.ts:43
claude-code/sonnet
correctness
🔴
useUpdateShareRole parses the PATCH /shares/:userId response with the same over-strict schema; the backend's UpdateShareRole also omits email/displayName, so every successful role change throws and is swallowed silently (no onError in ShareGardenModal), leaving the UI reverted to the stale role
web/src/lib/shares.ts:52
claude-code/sonnet
correctness
🟠
patch helper does not guard against readOnly blur firing
web/src/editor/Inspector.tsx:69
opencode/kimi-k2.6:cloud
error-handling
🟠
onBlur handlers in disabled fieldset can fire mutations after role downgrade
web/src/editor/Inspector.tsx:119
kimi-k2.6:cloud
error-handling
🟠
patch helper does not guard against readOnly blur firing
web/src/editor/PlopInspector.tsx:61
opencode/kimi-k2.6:cloud
error-handling
🟠
onBlur handlers in disabled fieldset can fire mutations after role downgrade
Garden with undefined myRole is treated as non-owner and shown a Leave (mutating) button with no badge; fail-closed direction is wrong for unknown role
web/src/components/gardens/GardenCard.tsx:30
glm-5.2:cloud
correctness
🟡
Non-401 useMe failure leaves Leave button disabled with no user-facing message
Notes (all verified against the checked-out repo, not just the diff):
The client-side canEdit/readOnly gating added across GardenCanvas.tsx, Inspector.tsx, PlopInspector.tsx, and GardenEditorPage.tsx is correctly backed by server-side authorization — internal/service/objects.go and internal/service/plantings.go require roleEditor minimum via requireGardenRole/objectForRole for every mutating call, so a bypassed or stale client-side check can't produce an unauthorized write; it would just 403.
ShareGardenModal's owner-only actions (ListShares, AddShare, UpdateShareRole) map to internal/service/shares.go calls that all call requireGardenRole(..., roleOwner) — the modal being reachable in principle by a non-owner (it isn't, since GardenCard/GardenEditorPage only render the Share entry point for isOwnerRole(myRole)) would still be rejected server-side.
LeaveGardenModal calls useRemoveShare(garden.id).mutateAsync(me.data.id) — server-side RemoveShare enforces owner-or-self, so a user can only remove their own share this way; no way to remove someone else's.
myRole is a server-populated, zod-validated enum (gardenRoleSchema in lib/gardens.ts) — never client-derived, matching the PR's claim, and canEditRole/isOwnerRole treat unknown/missing role as non-editing (fail-closed default).
No XSS risk in ShareGardenModal's rendering of sh.email/sh.displayName (React auto-escapes, no dangerouslySetInnerHTML).
Checked for a bypass path via a global keydown handler in GardenEditorPage.tsx:96-108 — it only touches local selection/UI state (Escape), no mutation call, so it can't fire an edit for viewers.
One pre-existing, out-of-scope observation (not introduced by this diff, part of the already-merged #16 API): AddShare returning a distinct "no account with that email" error does allow any garden owner to enumerate whether an arbitrary email has an account. That's backend behavior from a prior PR and this diff only surfaces the message string in the UI, so I'm not flagging it as a finding against this change.
🎯 Correctness — Blocking issues found
Confirmed: updateRole.mutate at line 93 has no onError/onSuccess handler, so a thrown ZodError there is silently swallowed (unhandled mutation rejection), matching the draft's claim. errorMessage only unwraps ApiError; a ZodError falls through to the generic fallback string, matching the onInvite claim too.
Both findings hold up against the actual code — the type mismatch is real and both consumption paths behave as described.
VERDICT: Blocking issues found
web/src/lib/shares.ts:39-53 — useAddShare and useUpdateShareRole parse their mutation responses with shareSchema, which requires email and displayName as mandatory strings (shares.ts:11-22). But the backend endpoints they hit return a bare domain.GardenShare: addShare (internal/api/shares.go:44-49) calls svc.AddShare (internal/service/shares.go:30-50) which returns store.CreateShare's *domain.GardenShare (internal/store/shares.go:83), and updateShare (internal/api/shares.go:52-71) calls svc.UpdateShareRole (internal/service/shares.go:53-61) which returns store.UpdateShareRole's *domain.GardenShare (internal/store/shares.go:101). domain.GardenShare (internal/domain/domain.go:140-149) has only id, gardenId, userId, role, createdBy, version, createdAt, updatedAt — no email/displayName. Only ListSharesForGarden (internal/store/shares.go:49, used by listShares/useShares) returns the joined domain.ShareWithUser, which adds those two fields (domain.go:151-157).
Consequence: shareSchema.parse(...) throws on every successful invite and every successful role change, even though the server request succeeded (the share row was created/updated).
In ShareGardenModal.onInvite (ShareGardenModal.tsx:34-45), the thrown error is caught and errorMessage(err, 'Could not share the garden.') is shown — since a ZodError isn't an ApiError, errorMessage (web/src/lib/api.ts:130-133) falls back to that generic string. The owner sees "Could not share the garden" for an invite that actually succeeded, and onSuccess (which invalidates the shares list) never fires because the mutation promise rejected before reaching it.
In ShareGardenModal.tsx:93, updateRole.mutate({...}) has no error handler at all, so a role-change PATCH that succeeds server-side fails client-side silently — no error shown, and the <select> snaps back to the stale role since the cache was never invalidated.
Fix: either have the backend return the joined ShareWithUser row from AddShare/UpdateShareRole, or give shareSchema a variant with email/displayName optional for the add/update responses, or refetch useShares after add/update instead of parsing the mutation's own response as a Share.
🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed by direct file inspection: Inspector.tsx lines 113–227 and PlopInspector.tsx lines 127–171 both show the exact indentation mismatch described (only the first/last fieldset children reindented, the body left at the outer indentation level). Also confirmed no .prettierrc/.eslintrc/eslint.config.* in web/, and no formatting step in package.json scripts.
VERDICT: Minor issues
web/src/editor/Inspector.tsx:113-227 — The new <fieldset> wrapper isn't consistently indented. TextField "Name" (114-120) and TextArea "Notes" (219-226) are indented one level in (8 spaces, correctly nested under the fieldset), but everything between them — the width/height/x/y grid (122-163), the rotation field (165-177), the color picker row (179-207), and the "Plantable" checkbox (209-217) — is still at the fieldset's own indentation level (6 spaces), even though it's also inside the fieldset. No prettier/eslint config exists in web/ and the build/test scripts don't touch formatting, so nothing will catch or auto-fix this. Fix: reindent the full fieldset body one level so nesting is visually accurate.
web/src/editor/PlopInspector.tsx:127-171 — Same pattern: the <fieldset> opens at 127, but the radius/count grid (128-153) and the "Label" field (155-161) stay at the outer indentation while the "Planted" TextField (163-170) is correctly indented as a fieldset child. Same fix as above.
Everything else in this lens is clean: GardenCard.tsx factors the two repeated button class strings into actionClass/dangerClass rather than leaving them duplicated across five buttons; LeaveGardenModal.tsx mirrors the existing DeleteGardenModal.tsx structure closely, which is the right call for consistency; the canEdit/readOnly prop threading through GardenCanvas, Inspector, PlopInspector, and GardenEditorPage is a single, traceable flag rather than being re-derived in multiple places; and lib/shares.ts follows the existing lib/gardens.ts-style query/mutation hook pattern. The inline per-share <select> in ShareGardenModal.tsx duplicates the viewer/editor option list already in roleOptions, but the two need different label text and the shared Select UI component forces a visible <label> layout that doesn't fit the compact inline row, so hand-rolling it there is a reasonable choice — not flagging it.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens only (efficiency regressions, unnecessary allocations, hot-loop work, unbounded growth, missing pagination, blocking calls, quadratic behavior).
Findings:
web/src/editor/GardenCanvas.tsx — the new canEdit gating (canEdit && selectedObject && ..., canEdit && selectedPlop && ..., canEdit && focusedObject && armedPlant && ...) only adds O(1) boolean checks and, for viewers, skips mounting SelectionOverlay/PlopOverlay/the placement-capture <rect> entirely — a slight net win, not a regression. Verified by reading the full render path (lines 32–233); the pre-existing .find() scans for selectedObject/selectedPlop/focusedObject (lines 104–107) are unchanged by this diff and out of scope.
web/src/lib/shares.ts — useShares/useAddShare/useUpdateShareRole/useRemoveShare follow the same invalidate-on-mutate pattern already used in lib/auth.ts (verified by reading auth.ts). No pagination on GET /gardens/:id/shares, but this is a per-garden collaborator list, realistically small, and mirrors the app's existing non-paginated list endpoints — not a regression introduced here.
web/src/components/gardens/ShareGardenModal.tsx — useShares(garden.id) fires once when the modal mounts (only rendered when sharing/dialog.kind === 'share' is true), and the shares list render (shares.data?.map) is a flat, unmemoized map over a small array — no N+1, no quadratic work.
web/src/components/gardens/GardenCard.tsx — actionClass/dangerClass were hoisted to module-level string constants (previously inline JSX literals); this is perf-neutral to slightly better, not worse.
Inspector.tsx/PlopInspector.tsx — wrapping existing fields in a single <fieldset disabled={readOnly}> is a structural change only, no added render cost.
Nothing in this diff introduces N+1 queries, extra allocations in hot paths, missing limits, or blocking calls. The canvas defense-in-depth checks are cheap and, for viewers, actually reduce mounted overlay work.
🧯 Error handling & edge cases — Minor issues
Confirmed: UpdateShareRole and DeleteShare in the store genuinely return ErrNotFound on a missing row (backing the draft's failure-scenario claim), and the two mutations in ShareGardenModal.tsx (web/src/lib/shares.ts:48-69) have no onError at the hook level — so .mutate() calls at lines 93/102 with no options truly surface zero feedback. The DeleteGardenModal.tsx convention (mutateAsync + try/catch + Alert) checks out exactly as claimed. Both findings are verified.
VERDICT: Minor issues
web/src/components/gardens/ShareGardenModal.tsx:93 and :102 — updateRole.mutate(...) and remove.mutate(sh.userId) are fire-and-forget: no onError, no .isError rendering, nothing. Every other mutation in this codebase (the invite form at :42, and DeleteGardenModal.tsx:16) uses mutateAsync + try/catch + an Alert. Confirmed the underlying hooks (web/src/lib/shares.ts:48-69) define no onError either, and the server-side calls can genuinely fail: internal/store/shares.go:101-117,120-134 — UpdateShareRole/DeleteShare return ErrNotFound if the share row is already gone (e.g. the owner acting from a stale list, or the recipient just left), which internal/service/shares.go:60,68-79 propagates (plus ErrForbidden for a bad actor). When that happens here, the role <select> or the row just silently fails to change/disappear with zero feedback.
Fix: wrap both in .mutate(..., { onError: (err) => setError(errorMessage(err, '...')) }) (or mutateAsync/try-catch, matching the invite-form pattern) and render the error.
web/src/components/gardens/ShareGardenModal.tsx:93/:102 (same lines, related edge case) — clicks aren't disabled while a mutation is pending (no disabled={updateRole.isPending} / disabled={remove.isPending}), so a slow response lets a user fire duplicate PATCH/DELETE requests on the same row before the first resolves. Given the missing error handling above, a duplicate DELETE returning 404 would also go unnoticed. Low severity since the endpoints are effectively idempotent, but worth a quick disabled guard.
Through the security lens, this frontend change is well-behaved:
Fail-closed role gating.canEditRole(undefined) and isOwnerRole(undefined) both return false (verified at web/src/lib/gardens.ts). myRole is optional() on the schema, so a missing/ malformed server role degrades to read-only viewer rather than escalating — the safe default.
Role is server-derived, not guessed.canEdit/isOwner come solely from g.myRole (GardenEditorPage.tsx:127-128); no client-side inference from ownerId or share state.
Defense in depth on mutations. The canvas create/place handlers re-check canEdit (GardenCanvas.tsx:115,141) in addition to the UI gates (palette/overlays not mounted for viewers). Inspector/PlopInspector wrap all inputs in <fieldset disabled={readOnly}> and hide Change/Delete/Remove/Plant-here — disabled inputs can't focus, so the onBlur patch handlers can't fire for a viewer.
No injection surface.gardenId/userId interpolated into share URLs (shares.ts:33,48,63) are typed number and zod-validated (z.number()); no path injection. Invite email is sent as a JSON body and validated server-side; type="email" plus a non-empty trim check is adequate client-side.
No XSS.displayName/email/myRole are rendered as React text content or aria-label (auto-escaped), never dangerouslySetInnerHTML.
No credential/secret leakage. Only display names and emails (already visible to the owner via the shares endpoint) are surfaced.
One item I could not verify from this frontend-only diff (and therefore am not reporting as a finding): the DELETE /gardens/:id/shares/:userId endpoint is invoked by a recipient with me.data.id (LeaveGardenModal.tsx:21). This is correct on the client, but the safety of "leave" vs. "revoke another user's share" depends entirely on the server authorizing that :userId === authenticated user (or that the actor is the owner). That is a backend authz concern outside this PR's files; the frontend does its part correctly.
🎯 Correctness — Minor issues
Both findings confirmed against the actual code. Adjusting line numbers to the real locations.
Verdict: Minor issues
I verified the data flow and gating logic against the checked-out code. The core canEdit/isOwner derivation is sound (role comes from server myRole, helpers treat unknown as no-access), and the fieldset disabled approach correctly covers the native inputs in both inspectors. The canvas defense-in-depth checks (canEdit in onCanvasPointerDown/onPlace and the overlay-mount guards) are correctly placed. A few minor robustness gaps remain:
web/src/components/gardens/ShareGardenModal.tsx:93 — inline role-change select has no error handling.updateRole.mutate({ userId: sh.userId, role }) is fire-and-forget on the <select onChange>. useUpdateShareRole defines only an onSuccess (invalidate) with no onError, and there's no rollback/feedback — on a failed PATCH (403/409) the select keeps the chosen value in the DOM until the query refetches, so the user sees a role that wasn't applied. The Add-share path above it (onInvite) does handle errors via try/catch + Alert; this one doesn't, asymmetrically. Suggested fix: add onError surfacing via the shared error/Alert, or make the select controlled and reconcile on failure.
web/src/components/gardens/GardenCard.tsx:30,40,64 — a garden with myRole === undefined is treated as non-owner and gets a Leave button with no badge.isOwnerRole(undefined) is false (confirmed in gardens.ts:37), so the footer falls into the !owner branch and renders Leave; the badge is gated on !owner && garden.myRole (truthy), so a missing role yields a card with no role label and a Leave button that calls remove.mutateAsync(me.data.id) — a self-share delete that likely 404s/403s since there's no share row. This is a latent edge (the backend populates myRole and the schema marks it optional only defensively), but the fail-closed direction is wrong for an unknown role: Leave is a mutating action. Consider gating Leave on garden.myRole && !owner explicitly.
web/src/components/gardens/LeaveGardenModal.tsx:21 — me.data.id used as the share userId to delete. Confirmed correct against useRemoveShare (shares.ts:60-69, DELETE /gardens/:id/shares/:userId) and the comment that the recipient reuses the owner's revoke endpoint to delete their own row. No change needed; noting the backend dependency.
No blocking correctness issues. The canEdit gating is consistent across canvas, overlays, palette, inspectors, and the plant-placement flow; useRemoveShare correctly invalidates both the shares and ['gardens'] queries so the list updates after a leave.
🧹 Code cleanliness & maintainability — Minor issues
All findings confirmed against the actual source code. The only correction needed is the line number for the useShares finding (the function is at line 35, not 43).
VERDICT: Minor issues
web/src/editor/Inspector.tsx:113-227 & PlopInspector.tsx:127-171 — fieldset children not re-indented. The new <fieldset> wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g. <div className="grid grid-cols-2 gap-2"> at Inspector:122 sits at the same indent as the <fieldset> that owns it; same in PlopInspector:128). The closing </fieldset> at 227 is likewise dedented past the <TextArea> it contains. This flattens the visual scope, so a reader can't tell at a glance which elements the disabled wrapper governs — exactly the thing the comment above it says is the point of the change. Re-indent the wrapped block one level; consider extracting the shared fieldset+controls body into a small ObjectFields fragment if the file keeps growing.
web/src/components/gardens/ShareGardenModal.tsx:91-98 — inline <select> + duplicated role options. The per-row role control uses a raw <select> with fieldControlClass glued on, while the invite form above it uses the imported Select component for the same concept. Worse, the role labels now live in two places: the roleOptions constant ("Viewer (read-only)" / "Editor (can edit)") and the inline <option value="viewer">Viewer</option> / <option value="editor">Editor</option> — two sources of truth that will drift. Route the per-row control through Select (or at least map over roleOptions) so there's one list.
Duplicated "View only" indicator. The same rounded-md bg-border/40 px-2 py-1 text-xs text-muted "View only" string appears three times: Inspector.tsx:102, PlopInspector.tsx:110, and GardenEditorPage.tsx:158 (the page one adds an 👁 and mb-2, so it's not even visually consistent with the inspector ones). A single <ViewOnlyBadge> component would remove the copy-paste and let the wording/styling stay in sync.
web/src/components/gardens/ShareGardenModal.tsx:104 — remove button class string duplicated again. The ✕ remove button reuses the exact hover:bg-red-500/10 hover:text-red-600 dark:hover:text-red-400 pattern already factored into GardenCard's dangerClass and PlantCard's equivalent. This is the third inline copy of that danger-hover string; lift it to a shared constant (e.g. alongside the existing actionClass/dangerClass in a shared module) instead of pasting it once more.
web/src/lib/shares.ts:35 — enabled parameter is dead.useShares(gardenId, enabled = true) exposes an enabled flag that no caller passes (ShareGardenModal calls useShares(garden.id); grep confirms that's the only call site). Either wire it up (e.g. pass enabled={!!me}-style guards) or drop it until a caller needs it, to avoid a "why is this here" trap for the next reader. Trivial.
⚡ Performance — Minor issues
The finding is confirmed. useRemoveShare (line 66) invalidates ['gardens'] on every success, and the comment explicitly documents it's shared between owner-revoke and recipient-leave paths. ShareGardenModal calls remove.mutate(sh.userId) for the owner-revoke case, while LeaveGardenModal uses mutateAsync(me.data.id) for the leave case — both go through the same hook.
Verdict: Minor issues
I reviewed the diff through the performance lens and verified the data-layer and component code. The change is mostly clean performance-wise — no N+1 patterns, no unbounded growth, mutations invalidate narrowly scoped keys, and the shares query only fires when the modal mounts. One minor, avoidable refetch:
web/src/lib/shares.ts:66 — useRemoveShare.onSuccess unconditionally invalidates ['gardens'] on every remove. This is correct and necessary for the leave path (the garden leaves the recipient's list), but it also fires for the owner revoke path in ShareGardenModal. When the owner manages shares from GardensPage (where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-list GETs. Suggested fix: split the hook (or pass a flag) so only the leave flow invalidates ['gardens']; revoke needs only sharesKey(gardenId). Verified by reading shares.ts (the hook's own comment documents it's used by both paths) and confirming ShareGardenModal calls remove.mutate(sh.userId) for the owner-revoke case while LeaveGardenModal calls remove.mutateAsync(me.data.id) for the leave case.
No other material performance concerns: shares.data?.map renders only after load (no render of undefined), useShares is gated behind modal mounting, inspector mutation hooks are instantiated in read-only mode but issue no network requests, and there's no work inside hot loops introduced here.
🧯 Error handling & edge cases — Minor issues
All three findings verified against the actual code:
ShareGardenModal fire-and-forget — confirmed. updateRole.mutate/remove.mutate at lines 93/102 have no onError; shares.ts mutations define only onSuccess + mutationFn, no onError; queryClient.ts has no global MutationCache/mutations.onError. Contrast objects.ts uses onError: toast.error. Keep.
Missing myRole lockout — confirmed. canEditRole/isOwnerRole return false for undefined; myRole is optional(); GardenEditorPage gates everything on canEdit/isOwner. Keep.
useMe non-401 stuck button — confirmed. auth.ts:31-38 resolves to null only on 401, throws otherwise → me.data undefined → button disabled={remove.isPending || !me.data} with no error/loading UI. Keep.
Verdict: Minor issues found
web/src/components/gardens/ShareGardenModal.tsx:93 / :102 — inline role-change and remove mutations silently swallow failures.updateRole.mutate(...) (line 93) and remove.mutate(sh.userId) (line 102) are fire-and-forget. Verified in web/src/lib/shares.ts: useUpdateShareRole (lines 48-55) and useRemoveShare (lines 60-69) define only mutationFn and onSuccess, no onError. web/src/lib/queryClient.ts has no global MutationCache/mutations.onError. So a failed PATCH/DELETE (network drop, 409, 403) produces no UI feedback at all — no Alert, no toast — and the shares list only refetches via onSuccess. Contrast the invite path (:42) which uses mutateAsync + try/catch + <Alert>, and lib/objects.ts where every mutation has onError: (err) => toast.error(...). Concrete impact: an owner changes a share's role or hits ✕ to revoke, the request fails, and they're left believing it succeeded while the displayed role row still shows the old value. Fix: add onError to these two mutations (e.g. toast.error(...), matching lib/objects.ts) or switch the handlers to mutateAsync with try/catch + the existing error state.
web/src/pages/GardenEditorPage.tsx:127 — canEdit/isOwner are false when myRole is absent, which can lock an owner out of their own garden with no error surfaced. Verified in web/src/lib/gardens.ts: canEditRole(undefined) (line 32-34) and isOwnerRole(undefined) (line 37-39) both return false, and myRole is declared gardenRoleSchema.optional() (line 27) "defensively." GardenEditorPage.tsx:127-128 gates all UI on these. If the server ever omits my_role (backend bug, partial deploy, stale cache), the owner lands in read-only viewer mode on their own garden: no Palette (line 160), no Share button (line 152), disabled inspectors (readOnly={!canEdit}), all canvas mutations gated off (canEdit checks in GardenCanvas). Silent total loss of function with no error state. Worth at least a non-owner fallback or an explicit "couldn't determine your role" error rather than treating unknown as viewer.
web/src/components/gardens/LeaveGardenModal.tsx:40 — a non-401 useMe failure leaves the user stuck with a disabled button and no explanation. Verified in web/src/lib/auth.ts:29-44: meQueryOptions.queryFn returns null only on ApiError.isUnauthorized (401); any other error is rethrown, leaving me.data undefined. LeaveGardenModal.tsx:40 sets disabled={remove.isPending || !me.data} with no error/loading state for me, so a non-401 useMe failure keeps "Leave" permanently disabled with no message. Minor unhappy path with no feedback; a loading/error state on me would cover it.
Through the security lens, the PR is clean. Verified:
Role source: myRole is declared optional() in the zod schema and comes from the server response; canEditRole/isOwnerRole both safely default to false when undefined. No client-side privilege escalation.
Client gating: The editor gates UI (Palette, overlays, inspectors) on canEdit derived from server-provided myRole. Event handlers in GardenCanvas double-check canEdit before firing placement mutations — defense in depth acknowledged in code comments.
Share dialog: ShareGardenModal only renders share-management UI; actual authorization lives on the backend (/gardens/:id/shares). No injection vector — email is sent as a JSON string body, and React auto-escapes interpolated user data (displayName, email, garden name) in JSX.
Leave flow: LeaveGardenModal passes the current user’s own me.data.id to useRemoveShare; no cross-user revocation from this component.
Read-only inspectors: <fieldset disabled> is a UX convenience; the backend remains the authority, and the PR explicitly does not rely on client-side gates alone.
No authz gaps, injection vectors, secret leakage, or unsafe deserialization found.
🎯 Correctness — No material issues found
No material issues found
The role-gating logic is semantically correct throughout: canEditRole and isOwnerRole properly derive rights from the server's my_role; the canvas, inspector, palette, and share flows are all gated with the right predicates; and the defense-in-depth checks in GardenCanvas (armed && canEdit, !canEdit || ...) ensure stale store state can't bypass viewer restrictions. The read-only inspector fieldset correctly disables all nested controls in one shot, and the mutation hooks target the expected endpoints with the expected payloads. Nothing in this diff introduces a logic error or incorrect domain behavior.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/gardens/GardenCard.tsx:6-16 — actionClass and dangerClass are copy-pasted verbatim from PlantCard.tsx. The exact same 150+ character strings appear in both files. Extract them to a shared location (e.g. components/ui/actionButton.ts) so future style tweaks don't diverge.
web/src/components/gardens/ShareGardenModal.tsx:100-107 — The remove-share button uses a raw Tailwind string instead of the established dangerClass pattern used in GardenCard/PlantCard. This is inconsistent and will drift if the danger style changes.
web/src/editor/Inspector.tsx:113-227 — The <fieldset disabled={readOnly}> wrapper was added, but its children (lines 122-227) are not indented inside it; they sit at the same column as the <fieldset> tag. This makes the DOM nesting misleading and harder to maintain.
web/src/editor/PlopInspector.tsx:127-171 — Same indentation issue: the <fieldset> wrapper added in the diff has children at the same column level instead of being indented.
⚡ Performance — No material issues found
No material issues found
Through the Performance lens, the PR is clean. The read-only viewer mode actually reduces work for the majority use-case (viewers) by conditionally omitting the palette, placement overlays, selection handles, and edit-bound mutation hooks. The canEdit gating is a primitive boolean derived once at the page level, so it doesn't trigger spurious re-renders. Memoization of objects, plantings, and plantsById in GardenEditorPage is preserved. No N+1 patterns, unbounded hot-path loops, or missing pagination on lists that would be material at this scale were introduced.
🧯 Error handling & edge cases — Minor issues
Based on my verification of the actual source code, all four findings are confirmed. Here is the corrected review:
Minor issues found
web/src/components/gardens/ShareGardenModal.tsx:93 — updateRole.mutate and remove.mutate are fire-and-forget with no error handling. If the backend returns a 403, 409, or network failure, the user gets zero feedback; the role dropdown snaps back (or the removed user re-appears on next refetch) with no explanation. Fix: Wire both mutations into a local error state (or a toast) and show an <Alert> when they fail, mirroring the add mutation pattern.
web/src/components/gardens/ShareGardenModal.tsx:50 — The Modal’s busy prop only tracks add.isPending, so Escape / backdrop-click can dismiss the dialog while an in-flight updateRole or remove is still pending. That buries any eventual error and leaves the user unsure whether the action completed. Fix: Pass busy={add.isPending || updateRole.isPending || remove.isPending}.
web/src/editor/Inspector.tsx:119 and web/src/editor/PlopInspector.tsx:138 — Every onBlur handler inside the <fieldset disabled={readOnly}> calls patch(...) without checking readOnly. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a new myRole), React re-renders with readOnly=true, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403. Fix: Guard the patch helpers (or each onBlur) with an early if (readOnly) return;.
Examining the change through the security lens: I verified the backend authorization (service/shares.go, store/shares.go), the API layer (api/shares.go uses mustActor(c).ID from session, parameterized queries throughout), the data layer (lib/shares.ts uses zod parsing of server responses), and the client-side gating (lib/gardens.ts derives canEdit/isOwner solely from server-provided myRole).
VERDICT: No material issues found
Authorization is enforced server-side at every share endpoint (requireGardenRole(ctx, actorID, gardenID, roleOwner) for list/add/update; owner-or-self rule for remove in internal/service/shares.go:19,30,53,68). The client's role badges and Share button are derived from the server's myRole, not guessed.
Email invite input (ShareGardenModal.tsx) is sent as JSON to a parameterized lookup (store/shares.goGetUserByEmail-equivalent via GetUserByEmail in service), no injection surface; response displayName/email are rendered as React text (auto-escaped), no dangerouslySetInnerHTML.
Read-only viewer mode uses a disabled <fieldset> (Inspector, PlopInspector) which prevents onBlur/onChange mutations from firing, and action buttons are conditionally hidden; plus canvas-level canEdit re-checks in GardenCanvas.tsx are genuine defense in depth since they'd 403 server-side anyway.
useShares is only mounted from ShareGardenModal, which is only reachable via owner-gated UI; backend also 403s non-owners, so no authz gap if a stale client opened it.
LeaveGardenModal removes the actor's own share using me.data.id from useMe() (the authenticated session), not from URL/props — a recipient cannot supply another user's id through this path; the backend RemoveShare additionally enforces actorID == targetUserID for non-owners.
🎯 Correctness — Minor issues
Both mutations confirmed fire-and-forget with no onError. For the third finding, react-query preserves data during background refetch (only isPending on initial load with no data), so the "renders empty during refetch" claim is incorrect — dropping it.
VERDICT: Minor issues
Findings:
web/src/components/gardens/ShareGardenModal.tsx:93 — updateRole.mutate(...) fires with no error surface. The hook in lib/shares.ts:48-55 defines only onSuccess (invalidation), no onError, and the call site uses fire-and-forget mutate (no try/catch). If the PATCH fails (network error, 403 after a role-change race, version conflict), the user gets no feedback: the <select> flips to the new value, then either silently reverts on refetch or silently stays wrong if the request fails before invalidation. The add-share path correctly uses mutateAsync + try/catch, but the inline role change does not. Suggested fix: add onError to the hook (e.g. toast.error) or wrap the call in try/catch with surfaced local error state.
web/src/components/gardens/ShareGardenModal.tsx:102 — remove.mutate(sh.userId) is fire-and-forget with no error surface. useRemoveShare (lib/shares.ts:60-69) defines only onSuccess, no onError, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
Duplicated actionClass / dangerClass constants — web/src/components/gardens/GardenCard.tsx:5-10 defines the exact same two class strings as web/src/components/plants/PlantCard.tsx:5-10 (verified identical character-for-character). This is copy-paste that should be shared — e.g. extract to web/src/components/ui/button.ts or a cardActions helper. As more card components appear, this drift will diverge.
Inconsistent <select> usage within ShareGardenModal — web/src/components/gardens/ShareGardenModal.tsx:91-99 uses a raw <select> with manual fieldControlClass + cn for the inline role changer, while the same file (line 62) uses the Select component for the invite form's role picker. Within one file, picking both patterns is confusing; either use Select for both or extract a small inline-role select. The raw form also bypasses the useFieldId/label wiring the shared component provides.
<fieldset> wrapping has misleading indentation — In web/src/editor/Inspector.tsx:113-227 and web/src/editor/PlopInspector.tsx:127-171, the <fieldset> opens at one indent level but its children (the grid <div>, the rotation/label TextFields, the color row, the Plantable checkbox, the Notes TextArea) keep their pre-existing indentation rather than being re-indented one level. The result reads as if those controls are siblings of the fieldset, not children — a structural mismatch that will trip the next reader doing a git-blame or refactor. A one-time re-indent of the wrapped block (or a prettier format pass) would fix it cleanly.
Raw enum value surfaced in UI badge — GardenCard.tsx:42 renders shared · {garden.myRole}, which prints the lowercase wire value (viewer/editor). Elsewhere in the PR the same roles are presented as Viewer (read-only) / Editor (can edit) (ShareGardenModal.tsx:13-16). Minor, but a shared roleLabel(role) helper would keep the casing/wording consistent and centralize the display string.
⚡ Performance — No material issues found
VERDICT: No material issues found
I checked the data layer (web/src/lib/shares.ts) and the share/leave modals for hot-path waste, unnecessary refetches, and unbounded work. The mutations invalidate only their own sharesKey(gardenId) plus (for useRemoveShare) the ['gardens'] list key — no broad broadcast, no N+1.
useShares(garden.id) in ShareGardenModal only fires when the modal is mounted (conditional render in GardensPage.tsx:75 and GardenEditorPage.tsx:227), so the owner-only shares endpoint isn't polled on every garden view.
useRemoveShare invalidates ['gardens'] on every removal, which causes one extra list refetch even when the owner revokes a share (their garden list doesn't change). This is a single additional request gated behind a manual click, not a hot path — not material.
The role <select> and remove ✕ in ShareGardenModal.tsx:91-107 fire updateRole.mutate / remove.mutate directly with no debounce/disabled-during-pending, but each triggers a single targeted invalidation — no cascading refetches, no quadratic behavior.
No new allocations on the canvas hot loop; canEdit is a cheap boolean gate around setArmedKind/overlay mounting. The plantsById Map rebuild is pre-existing and unchanged.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Verified by reading ShareGardenModal.tsx, shares.ts, Modal.tsx, gardens.ts, and GardenEditorPage.tsx.
ShareGardenModal.tsx:93 and :102 — role change and share removal silently swallow errors. Both updateRole.mutate({ userId, role }) and remove.mutate(sh.userId) are fire-and-forget. The hooks in shares.ts:39-69 only define onSuccess (no onError), the call sites attach no onError, and .mutate (unlike .mutateAsync) does not throw — react-query stashes the error in mutation.error but surfaces nothing. So a 403/409/network failure on a role change or a revoke leaves the user with zero feedback: the <select> visually flips, then silently reverts on the next refetch (or doesn't, if the refetch also fails). Compare onInvite at :41-46, which wraps add.mutateAsync in try/catch and shows an Alert. Fix: either switch to mutateAsync with try/catch + setError, or pass { onError: (err) => setError(errorMessage(err, …)) } to each call (and/or an onError on the hooks). The role-change <select> should also be disabled while its own mutation is pending to avoid stacking a second PATCH on an in-flight one.
ShareGardenModal.tsx:50 — busy only reflects add.isPending, so update/remove mutations don't lock the modal. Confirmed Modal.tsx:32,42 suppresses Escape/backdrop-close only when busy is true. With busy={add.isPending}, a role change or revoke in flight leaves busy=false, so Escape/backdrop click can dismiss the dialog mid-request. Fix: derive busy={add.isPending || updateRole.isPending || remove.isPending}.
ShareGardenModal.tsx:100-107 — destructive revoke (✕) has no confirmation and no per-row busy state. Unlike LeaveGardenModal (which confirms via a modal) and the object delete flow in Inspector.tsx (two-step "Confirm delete"), the owner's share removal at :100-107 is one click with no undo, no busy indicator on the row, and (per the first finding) no error feedback. A mis-tap revokes someone's access with no recourse. Suggest a confirm step or at minimum a disabled state keyed to that row's pending removal.
GardenEditorPage.tsx:127-128 — safe-default on missing myRole, but misleading "View only" indicator.canEditRole/isOwnerRole (gardens.ts:32-39) return false when myRole is undefined — the right defensive default. But !canEdit at :157-159 then renders the "👁 View only" banner even when the role is simply missing rather than genuinely viewer. The schema marks myRole optional (gardens.ts:25-28), so this path is reachable in principle. Not blocking — the comment says myRole is present on every accessible response — but worth a guard so an owner with a transiently-missing role isn't told they're view-only.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
web/src/pages/GardenEditorPage.tsx:135-143 — onPickPlant fires updatePlanting.mutate for a "change" operation without checking canEdit. Every other mutation path in the editor (create object, create planting, move/resize via SelectionOverlay, delete object, remove planting) is either gated by a canEdit check or hidden behind a canEdit-conditional mount. The PlantPicker modal is the only remaining un-gated mutation path: a viewer who manipulates React state (e.g., via DevTools or by forcing the modal open) can change a plop's plant. The PR explicitly calls this out as a defense-in-depth concern for canvas handlers, but the picker callback was missed. Add an early if (!canEdit) return guard in onPickPlant.
🎯 Correctness — No material issues found
No material issues found.
I reviewed the PR thoroughly through the correctness lens, focusing on semantic/domain correctness, role gating logic, API contract consistency, and state/cache behavior.
Key areas verified:
Role schema and gating: canEditRole and isOwnerRole correctly map owner/editor/viewer to edit/owner permissions. The undefined fallback returns false — a safe defensive choice.
GardenCanvas defense in depth: canEdit gates object creation (onCanvasPointerDown), plop placement (onPlace), and overlay mounting (SelectionOverlay, PlopOverlay, placement capture). Viewers can still select objects for read-only inspection, which is intentional.
Inspector/PlopInspector: <fieldset disabled={readOnly}> correctly disables all nested inputs in one shot. Mutation buttons (Delete, Remove, Change plant, Plant here) are conditionally hidden.
Share/Leave modals: useRemoveShare(garden.id) with me.data.id correctly removes the current user's share. ShareGardenModal uses the proper API paths and zod schemas.
Cache invalidation: useRemoveShare invalidates both sharesKey and ['gardens'] (needed for the leave flow). useAddShare and useUpdateShareRole only invalidate sharesKey, which is correct for owner-local share-list UI. Cross-user list staleness is an inherent client-side caching limitation, not a correctness bug in this code.
Type safety: The Garden schema includes myRole: gardenRoleSchema.optional(), and ShareRole is correctly constrained to 'viewer' | 'editor' (no owner sharing).
The logic is sound and the defensive gating is consistent throughout.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/Inspector.tsx:113 — The <fieldset> wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.
web/src/editor/PlopInspector.tsx:127 — Same indentation problem: the <fieldset> tag and its first child <div> (line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.
web/src/components/gardens/ShareGardenModal.tsx:91 — Uses a raw <select> element (with manually imported fieldControlClass and cn) for the inline role changer, while the invite form 20 lines above uses the project's <Select> component. This creates two styling paths for the same control and duplicates the viewer/editor options already defined in the module-level roleOptions array. Prefer <Select> (or map roleOptions) to keep the UI consistent and DRY.
web/src/components/gardens/ShareGardenModal.tsx:84 — The <ul className="flex flex-col gap-2"> is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside the shares.isSuccess && shares.data.length > 0 branch.
web/src/components/gardens/GardenCard.tsx:5 — actionClass and dangerClass repeat the same long Tailwind base string; extract a shared baseActionClass (or use cn) to remove the copy-paste.
⚡ Performance — No material issues found
No material issues found
I examined the PR through the Performance lens and found no efficiency regressions. The changes add role-based conditional rendering gates (canEdit, isOwner) which are cheap boolean checks derived from stable server data. For viewers, the PR actually reduces work by not mounting interactive overlays (SelectionOverlay, PlopOverlay) and skipping placement capture — a perf win for the read-only path.
Specific areas checked:
Query invalidation in lib/shares.ts is targeted and correct; no over-invalidation or missing cache updates that would cause extra network churn.
Share dialog list (ShareGardenModal.tsx) renders without virtualization, but garden sharing lists are inherently small — no material concern.
Hot-path scans in GardenEditorPage.tsx and GardenCanvas.tsx (.find() for selectedObject, focusedObject, etc.) were pre-existing and untouched by this PR.
No new allocations inside loops or event handlers beyond the pre-existing patterns.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/components/gardens/ShareGardenModal.tsx:91-102 — updateRole.mutate and remove.mutate are fire-and-forget with no error handling, pending state, or disabled UI. If the server rejects a role change or removal (network error, 403, 404, version conflict), the user sees zero feedback; the list item stays in place as if nothing happened. Worse, the native <select> and remove button remain interactive while the mutation is in flight, so the user can trigger duplicate or conflicting requests. Fix: Wrap both in try/catch using mutateAsync, surface errors in a dedicated list error state, and disable each row’s controls while updateRole.isPending || remove.isPending. Also widen the Modal’s busy prop to cover all three pending states, or at least disable the Done button while any mutation is active.
web/src/editor/Inspector.tsx:69-70,76-83,119,131,141,151,161,173-177,191-201,225 — The patch helper and dimension commit helpers are called from every onBlur. If readOnly flips from false → true while an input has focus (e.g., the user’s role is downgraded by a background refetch), the disabled <fieldset> causes the focused input to blur, firing its onBlur handler and sending an unintended PATCH that the server will 403. Fix: Guard patch (and the inline blur commits) with if (readOnly) return; so a disabled fieldset never fires mutations.
web/src/editor/PlopInspector.tsx:61-62,67-75,77-88,90-93,138,150,160,169,180 — Same issue as Inspector: patch, commitRadius, commitCount, and commitPlanted don’t check readOnly. A role downgrade while an input is focused will blur the now-disabled fieldset and fire mutations. Fix: Add if (readOnly) return; to patch and each commit helper.
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** · 21 findings (7 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | updateRole.mutate / remove.mutate silently swallow errors — no onError, no user feedback on failed role change or revoke | `web/src/components/gardens/ShareGardenModal.tsx:91` | 5/5 | correctness, error-handling, maintainability |
| 🔴 | remove mutation has no error handling or pending UI | `web/src/components/gardens/ShareGardenModal.tsx:100` | 5/5 | correctness, error-handling, maintainability |
| 🟠 | Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable | `web/src/editor/Inspector.tsx:113` | 5/5 | maintainability |
| 🟠 | Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable | `web/src/editor/PlopInspector.tsx:127` | 5/5 | maintainability |
| 🟠 | Modal busy prop does not cover updateRole or remove pending states | `web/src/components/gardens/ShareGardenModal.tsx:50` | 3/5 | error-handling |
| 🟡 | actionClass/dangerClass duplicated verbatim from PlantCard — should be shared | `web/src/components/gardens/GardenCard.tsx:5` | 3/5 | maintainability |
| 🟠 | canEdit/isOwner false on missing myRole can lock an owner out of their own garden with no error surfaced | `web/src/pages/GardenEditorPage.tsx:127` | 2/5 | error-handling |
<details><summary>14 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | useAddShare parses the POST /shares response with a schema requiring email/displayName, but the backend's AddShare returns a bare GardenShare without them — every successful invite throws and shows a false 'Could not share the garden' error | `web/src/lib/shares.ts:43` | claude-code/sonnet | correctness |
| 🔴 | useUpdateShareRole parses the PATCH /shares/:userId response with the same over-strict schema; the backend's UpdateShareRole also omits email/displayName, so every successful role change throws and is swallowed silently (no onError in ShareGardenModal), leaving the UI reverted to the stale role | `web/src/lib/shares.ts:52` | claude-code/sonnet | correctness |
| 🟠 | patch helper does not guard against readOnly blur firing | `web/src/editor/Inspector.tsx:69` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | onBlur handlers in disabled fieldset can fire mutations after role downgrade | `web/src/editor/Inspector.tsx:119` | kimi-k2.6:cloud | error-handling |
| 🟠 | patch helper does not guard against readOnly blur firing | `web/src/editor/PlopInspector.tsx:61` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | onBlur handlers in disabled fieldset can fire mutations after role downgrade | `web/src/editor/PlopInspector.tsx:138` | kimi-k2.6:cloud | error-handling |
| 🟠 | PlantPicker onPickPlant bypasses canEdit defense-in-depth check | `web/src/pages/GardenEditorPage.tsx:135` | opencode/kimi-k2.6:cloud | security |
| 🟡 | Garden with undefined myRole is treated as non-owner and shown a Leave (mutating) button with no badge; fail-closed direction is wrong for unknown role | `web/src/components/gardens/GardenCard.tsx:30` | glm-5.2:cloud | correctness |
| 🟡 | Non-401 useMe failure leaves Leave button disabled with no user-facing message | `web/src/components/gardens/LeaveGardenModal.tsx:40` | glm-5.2:cloud | error-handling |
| 🟡 | Empty ul rendered during loading and error states | `web/src/components/gardens/ShareGardenModal.tsx:84` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | Duplicated 'View only' badge string/styling across Inspector, PlopInspector, and GardenEditorPage | `web/src/editor/Inspector.tsx:102` | glm-5.2:cloud | maintainability |
| 🟡 | useRemoveShare invalidates ['gardens'] on every remove, causing avoidable gardens-list refetches in the owner-revoke path | `web/src/lib/shares.ts:66` | glm-5.2:cloud | performance |
| ⚪ | Badge prints raw lowercase enum myRole; casing/wording inconsistent with role labels in ShareGardenModal | `web/src/components/gardens/GardenCard.tsx:42` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | useShares 'enabled' parameter has no caller; dead surface | `web/src/lib/shares.ts:35` | glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — 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>
**VERDICT: No material issues found**
Notes (all verified against the checked-out repo, not just the diff):
- The client-side `canEdit`/`readOnly` gating added across `GardenCanvas.tsx`, `Inspector.tsx`, `PlopInspector.tsx`, and `GardenEditorPage.tsx` is correctly backed by server-side authorization — `internal/service/objects.go` and `internal/service/plantings.go` require `roleEditor` minimum via `requireGardenRole`/`objectForRole` for every mutating call, so a bypassed or stale client-side check can't produce an unauthorized write; it would just 403.
- `ShareGardenModal`'s owner-only actions (`ListShares`, `AddShare`, `UpdateShareRole`) map to `internal/service/shares.go` calls that all call `requireGardenRole(..., roleOwner)` — the modal being reachable in principle by a non-owner (it isn't, since `GardenCard`/`GardenEditorPage` only render the Share entry point for `isOwnerRole(myRole)`) would still be rejected server-side.
- `LeaveGardenModal` calls `useRemoveShare(garden.id).mutateAsync(me.data.id)` — server-side `RemoveShare` enforces owner-or-self, so a user can only remove their own share this way; no way to remove someone else's.
- `myRole` is a server-populated, zod-validated enum (`gardenRoleSchema` in `lib/gardens.ts`) — never client-derived, matching the PR's claim, and `canEditRole`/`isOwnerRole` treat unknown/missing role as non-editing (fail-closed default).
- No XSS risk in `ShareGardenModal`'s rendering of `sh.email`/`sh.displayName` (React auto-escapes, no `dangerouslySetInnerHTML`).
- Checked for a bypass path via a global keydown handler in `GardenEditorPage.tsx:96-108` — it only touches local selection/UI state (Escape), no mutation call, so it can't fire an edit for viewers.
One pre-existing, out-of-scope observation (not introduced by this diff, part of the already-merged #16 API): `AddShare` returning a distinct "no account with that email" error does allow any garden owner to enumerate whether an arbitrary email has an account. That's backend behavior from a prior PR and this diff only surfaces the message string in the UI, so I'm not flagging it as a finding against this change.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
Confirmed: `updateRole.mutate` at line 93 has no `onError`/`onSuccess` handler, so a thrown `ZodError` there is silently swallowed (unhandled mutation rejection), matching the draft's claim. `errorMessage` only unwraps `ApiError`; a `ZodError` falls through to the generic fallback string, matching the `onInvite` claim too.
Both findings hold up against the actual code — the type mismatch is real and both consumption paths behave as described.
## VERDICT: Blocking issues found
- **`web/src/lib/shares.ts:39-53`** — `useAddShare` and `useUpdateShareRole` parse their mutation responses with `shareSchema`, which requires `email` and `displayName` as mandatory strings (`shares.ts:11-22`). But the backend endpoints they hit return a bare `domain.GardenShare`: `addShare` (`internal/api/shares.go:44-49`) calls `svc.AddShare` (`internal/service/shares.go:30-50`) which returns `store.CreateShare`'s `*domain.GardenShare` (`internal/store/shares.go:83`), and `updateShare` (`internal/api/shares.go:52-71`) calls `svc.UpdateShareRole` (`internal/service/shares.go:53-61`) which returns `store.UpdateShareRole`'s `*domain.GardenShare` (`internal/store/shares.go:101`). `domain.GardenShare` (`internal/domain/domain.go:140-149`) has only `id`, `gardenId`, `userId`, `role`, `createdBy`, `version`, `createdAt`, `updatedAt` — no `email`/`displayName`. Only `ListSharesForGarden` (`internal/store/shares.go:49`, used by `listShares`/`useShares`) returns the joined `domain.ShareWithUser`, which adds those two fields (`domain.go:151-157`).
Consequence: `shareSchema.parse(...)` throws on every successful invite and every successful role change, even though the server request succeeded (the share row was created/updated).
- In `ShareGardenModal.onInvite` (`ShareGardenModal.tsx:34-45`), the thrown error is caught and `errorMessage(err, 'Could not share the garden.')` is shown — since a `ZodError` isn't an `ApiError`, `errorMessage` (`web/src/lib/api.ts:130-133`) falls back to that generic string. The owner sees "Could not share the garden" for an invite that actually succeeded, and `onSuccess` (which invalidates the shares list) never fires because the mutation promise rejected before reaching it.
- In `ShareGardenModal.tsx:93`, `updateRole.mutate({...})` has no error handler at all, so a role-change PATCH that succeeds server-side fails client-side silently — no error shown, and the `<select>` snaps back to the stale role since the cache was never invalidated.
Fix: either have the backend return the joined `ShareWithUser` row from `AddShare`/`UpdateShareRole`, or give `shareSchema` a variant with `email`/`displayName` optional for the add/update responses, or refetch `useShares` after add/update instead of parsing the mutation's own response as a `Share`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings confirmed by direct file inspection: `Inspector.tsx` lines 113–227 and `PlopInspector.tsx` lines 127–171 both show the exact indentation mismatch described (only the first/last fieldset children reindented, the body left at the outer indentation level). Also confirmed no `.prettierrc`/`.eslintrc`/`eslint.config.*` in `web/`, and no formatting step in `package.json` scripts.
**VERDICT: Minor issues**
- `web/src/editor/Inspector.tsx:113-227` — The new `<fieldset>` wrapper isn't consistently indented. `TextField` "Name" (114-120) and `TextArea` "Notes" (219-226) are indented one level in (8 spaces, correctly nested under the fieldset), but everything between them — the width/height/x/y grid (122-163), the rotation field (165-177), the color picker row (179-207), and the "Plantable" checkbox (209-217) — is still at the fieldset's own indentation level (6 spaces), even though it's also inside the fieldset. No prettier/eslint config exists in `web/` and the build/test scripts don't touch formatting, so nothing will catch or auto-fix this. Fix: reindent the full fieldset body one level so nesting is visually accurate.
- `web/src/editor/PlopInspector.tsx:127-171` — Same pattern: the `<fieldset>` opens at 127, but the radius/count grid (128-153) and the "Label" field (155-161) stay at the outer indentation while the "Planted" `TextField` (163-170) is correctly indented as a fieldset child. Same fix as above.
Everything else in this lens is clean: `GardenCard.tsx` factors the two repeated button class strings into `actionClass`/`dangerClass` rather than leaving them duplicated across five buttons; `LeaveGardenModal.tsx` mirrors the existing `DeleteGardenModal.tsx` structure closely, which is the right call for consistency; the `canEdit`/`readOnly` prop threading through `GardenCanvas`, `Inspector`, `PlopInspector`, and `GardenEditorPage` is a single, traceable flag rather than being re-derived in multiple places; and `lib/shares.ts` follows the existing `lib/gardens.ts`-style query/mutation hook pattern. The inline per-share `<select>` in `ShareGardenModal.tsx` duplicates the viewer/editor option list already in `roleOptions`, but the two need different label text and the shared `Select` UI component forces a visible `<label>` layout that doesn't fit the compact inline row, so hand-rolling it there is a reasonable choice — not flagging it.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens only (efficiency regressions, unnecessary allocations, hot-loop work, unbounded growth, missing pagination, blocking calls, quadratic behavior).
Findings:
- `web/src/editor/GardenCanvas.tsx` — the new `canEdit` gating (`canEdit && selectedObject && ...`, `canEdit && selectedPlop && ...`, `canEdit && focusedObject && armedPlant && ...`) only adds O(1) boolean checks and, for viewers, *skips* mounting `SelectionOverlay`/`PlopOverlay`/the placement-capture `<rect>` entirely — a slight net win, not a regression. Verified by reading the full render path (lines 32–233); the pre-existing `.find()` scans for `selectedObject`/`selectedPlop`/`focusedObject` (lines 104–107) are unchanged by this diff and out of scope.
- `web/src/lib/shares.ts` — `useShares`/`useAddShare`/`useUpdateShareRole`/`useRemoveShare` follow the same invalidate-on-mutate pattern already used in `lib/auth.ts` (verified by reading `auth.ts`). No pagination on `GET /gardens/:id/shares`, but this is a per-garden collaborator list, realistically small, and mirrors the app's existing non-paginated list endpoints — not a regression introduced here.
- `web/src/components/gardens/ShareGardenModal.tsx` — `useShares(garden.id)` fires once when the modal mounts (only rendered when `sharing`/`dialog.kind === 'share'` is true), and the shares list render (`shares.data?.map`) is a flat, unmemoized map over a small array — no N+1, no quadratic work.
- `web/src/components/gardens/GardenCard.tsx` — `actionClass`/`dangerClass` were hoisted to module-level string constants (previously inline JSX literals); this is perf-neutral to slightly better, not worse.
- `Inspector.tsx`/`PlopInspector.tsx` — wrapping existing fields in a single `<fieldset disabled={readOnly}>` is a structural change only, no added render cost.
Nothing in this diff introduces N+1 queries, extra allocations in hot paths, missing limits, or blocking calls. The canvas defense-in-depth checks are cheap and, for viewers, actually reduce mounted overlay work.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed: `UpdateShareRole` and `DeleteShare` in the store genuinely return `ErrNotFound` on a missing row (backing the draft's failure-scenario claim), and the two mutations in `ShareGardenModal.tsx` (`web/src/lib/shares.ts:48-69`) have no `onError` at the hook level — so `.mutate()` calls at lines 93/102 with no options truly surface zero feedback. The `DeleteGardenModal.tsx` convention (`mutateAsync` + try/catch + `Alert`) checks out exactly as claimed. Both findings are verified.
## VERDICT: Minor issues
- **`web/src/components/gardens/ShareGardenModal.tsx:93` and `:102`** — `updateRole.mutate(...)` and `remove.mutate(sh.userId)` are fire-and-forget: no `onError`, no `.isError` rendering, nothing. Every other mutation in this codebase (the invite form at `:42`, and `DeleteGardenModal.tsx:16`) uses `mutateAsync` + try/catch + an `Alert`. Confirmed the underlying hooks (`web/src/lib/shares.ts:48-69`) define no `onError` either, and the server-side calls can genuinely fail: `internal/store/shares.go:101-117,120-134` — `UpdateShareRole`/`DeleteShare` return `ErrNotFound` if the share row is already gone (e.g. the owner acting from a stale list, or the recipient just left), which `internal/service/shares.go:60,68-79` propagates (plus `ErrForbidden` for a bad actor). When that happens here, the role `<select>` or the row just silently fails to change/disappear with zero feedback.
- Fix: wrap both in `.mutate(..., { onError: (err) => setError(errorMessage(err, '...')) })` (or mutateAsync/try-catch, matching the invite-form pattern) and render the error.
- **`web/src/components/gardens/ShareGardenModal.tsx:93`/`:102`** (same lines, related edge case) — clicks aren't disabled while a mutation is pending (no `disabled={updateRole.isPending}` / `disabled={remove.isPending}`), so a slow response lets a user fire duplicate PATCH/DELETE requests on the same row before the first resolves. Given the missing error handling above, a duplicate DELETE returning 404 would also go unnoticed. Low severity since the endpoints are effectively idempotent, but worth a quick `disabled` guard.
</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>
**VERDICT: No material issues found**
Through the security lens, this frontend change is well-behaved:
- **Fail-closed role gating.** `canEditRole(undefined)` and `isOwnerRole(undefined)` both return `false` (verified at `web/src/lib/gardens.ts`). `myRole` is `optional()` on the schema, so a missing/ malformed server role degrades to read-only viewer rather than escalating — the safe default.
- **Role is server-derived, not guessed.** `canEdit`/`isOwner` come solely from `g.myRole` (`GardenEditorPage.tsx:127-128`); no client-side inference from `ownerId` or share state.
- **Defense in depth on mutations.** The canvas create/place handlers re-check `canEdit` (`GardenCanvas.tsx:115,141`) in addition to the UI gates (palette/overlays not mounted for viewers). Inspector/PlopInspector wrap all inputs in `<fieldset disabled={readOnly}>` and hide Change/Delete/Remove/Plant-here — disabled inputs can't focus, so the `onBlur` patch handlers can't fire for a viewer.
- **No injection surface.** `gardenId`/`userId` interpolated into share URLs (`shares.ts:33,48,63`) are typed `number` and zod-validated (`z.number()`); no path injection. Invite `email` is sent as a JSON body and validated server-side; `type="email"` plus a non-empty trim check is adequate client-side.
- **No XSS.** `displayName`/`email`/`myRole` are rendered as React text content or `aria-label` (auto-escaped), never `dangerouslySetInnerHTML`.
- **No credential/secret leakage.** Only display names and emails (already visible to the owner via the shares endpoint) are surfaced.
One item I could *not* verify from this frontend-only diff (and therefore am not reporting as a finding): the `DELETE /gardens/:id/shares/:userId` endpoint is invoked by a recipient with `me.data.id` (`LeaveGardenModal.tsx:21`). This is correct on the client, but the safety of "leave" vs. "revoke another user's share" depends entirely on the server authorizing that `:userId === authenticated user` (or that the actor is the owner). That is a backend authz concern outside this PR's files; the frontend does its part correctly.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings confirmed against the actual code. Adjusting line numbers to the real locations.
## Verdict: Minor issues
I verified the data flow and gating logic against the checked-out code. The core `canEdit`/`isOwner` derivation is sound (role comes from server `myRole`, helpers treat unknown as no-access), and the `fieldset disabled` approach correctly covers the native inputs in both inspectors. The canvas defense-in-depth checks (`canEdit` in `onCanvasPointerDown`/`onPlace` and the overlay-mount guards) are correctly placed. A few minor robustness gaps remain:
- **`web/src/components/gardens/ShareGardenModal.tsx:93` — inline role-change `select` has no error handling.** `updateRole.mutate({ userId: sh.userId, role })` is fire-and-forget on the `<select onChange>`. `useUpdateShareRole` defines only an `onSuccess` (invalidate) with no `onError`, and there's no rollback/feedback — on a failed PATCH (403/409) the `select` keeps the chosen value in the DOM until the query refetches, so the user sees a role that wasn't applied. The Add-share path above it (`onInvite`) does handle errors via try/catch + `Alert`; this one doesn't, asymmetrically. Suggested fix: add `onError` surfacing via the shared `error`/`Alert`, or make the select controlled and reconcile on failure.
- **`web/src/components/gardens/GardenCard.tsx:30,40,64` — a garden with `myRole === undefined` is treated as non-owner and gets a `Leave` button with no badge.** `isOwnerRole(undefined)` is false (confirmed in `gardens.ts:37`), so the footer falls into the `!owner` branch and renders `Leave`; the badge is gated on `!owner && garden.myRole` (truthy), so a missing role yields a card with no role label and a `Leave` button that calls `remove.mutateAsync(me.data.id)` — a self-share delete that likely 404s/403s since there's no share row. This is a latent edge (the backend populates `myRole` and the schema marks it optional only defensively), but the fail-closed direction is wrong for an unknown role: `Leave` is a mutating action. Consider gating `Leave` on `garden.myRole && !owner` explicitly.
- **`web/src/components/gardens/LeaveGardenModal.tsx:21` — `me.data.id` used as the share userId to delete.** Confirmed correct against `useRemoveShare` (`shares.ts:60-69`, DELETE `/gardens/:id/shares/:userId`) and the comment that the recipient reuses the owner's revoke endpoint to delete their own row. No change needed; noting the backend dependency.
No blocking correctness issues. The `canEdit` gating is consistent across canvas, overlays, palette, inspectors, and the plant-placement flow; `useRemoveShare` correctly invalidates both the shares and `['gardens']` queries so the list updates after a leave.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All findings confirmed against the actual source code. The only correction needed is the line number for the `useShares` finding (the function is at line 35, not 43).
**VERDICT: Minor issues**
- **`web/src/editor/Inspector.tsx:113-227` & `PlopInspector.tsx:127-171` — fieldset children not re-indented.** The new `<fieldset>` wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g. `<div className="grid grid-cols-2 gap-2">` at Inspector:122 sits at the same indent as the `<fieldset>` that owns it; same in PlopInspector:128). The closing `</fieldset>` at 227 is likewise dedented past the `<TextArea>` it contains. This flattens the visual scope, so a reader can't tell at a glance which elements the disabled wrapper governs — exactly the thing the comment above it says is the point of the change. Re-indent the wrapped block one level; consider extracting the shared fieldset+controls body into a small `ObjectFields` fragment if the file keeps growing.
- **`web/src/components/gardens/ShareGardenModal.tsx:91-98` — inline `<select>` + duplicated role options.** The per-row role control uses a raw `<select>` with `fieldControlClass` glued on, while the invite form above it uses the imported `Select` component for the same concept. Worse, the role labels now live in two places: the `roleOptions` constant ("Viewer (read-only)" / "Editor (can edit)") and the inline `<option value="viewer">Viewer</option>` / `<option value="editor">Editor</option>` — two sources of truth that will drift. Route the per-row control through `Select` (or at least map over `roleOptions`) so there's one list.
- **Duplicated "View only" indicator.** The same `rounded-md bg-border/40 px-2 py-1 text-xs text-muted` "View only" string appears three times: `Inspector.tsx:102`, `PlopInspector.tsx:110`, and `GardenEditorPage.tsx:158` (the page one adds an `👁` and `mb-2`, so it's not even visually consistent with the inspector ones). A single `<ViewOnlyBadge>` component would remove the copy-paste and let the wording/styling stay in sync.
- **`web/src/components/gardens/ShareGardenModal.tsx:104` — remove button class string duplicated again.** The `✕` remove button reuses the exact `hover:bg-red-500/10 hover:text-red-600 dark:hover:text-red-400` pattern already factored into `GardenCard`'s `dangerClass` and `PlantCard`'s equivalent. This is the third inline copy of that danger-hover string; lift it to a shared constant (e.g. alongside the existing `actionClass`/`dangerClass` in a shared module) instead of pasting it once more.
- **`web/src/lib/shares.ts:35` — `enabled` parameter is dead.** `useShares(gardenId, enabled = true)` exposes an `enabled` flag that no caller passes (ShareGardenModal calls `useShares(garden.id)`; grep confirms that's the only call site). Either wire it up (e.g. pass `enabled={!!me}`-style guards) or drop it until a caller needs it, to avoid a "why is this here" trap for the next reader. Trivial.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The finding is confirmed. `useRemoveShare` (line 66) invalidates `['gardens']` on every success, and the comment explicitly documents it's shared between owner-revoke and recipient-leave paths. `ShareGardenModal` calls `remove.mutate(sh.userId)` for the owner-revoke case, while `LeaveGardenModal` uses `mutateAsync(me.data.id)` for the leave case — both go through the same hook.
Verdict: Minor issues
I reviewed the diff through the performance lens and verified the data-layer and component code. The change is mostly clean performance-wise — no N+1 patterns, no unbounded growth, mutations invalidate narrowly scoped keys, and the shares query only fires when the modal mounts. One minor, avoidable refetch:
- **`web/src/lib/shares.ts:66`** — `useRemoveShare.onSuccess` unconditionally invalidates `['gardens']` on every remove. This is correct and necessary for the *leave* path (the garden leaves the recipient's list), but it also fires for the *owner revoke* path in `ShareGardenModal`. When the owner manages shares from `GardensPage` (where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-list GETs. Suggested fix: split the hook (or pass a flag) so only the leave flow invalidates `['gardens']`; revoke needs only `sharesKey(gardenId)`. Verified by reading `shares.ts` (the hook's own comment documents it's used by both paths) and confirming `ShareGardenModal` calls `remove.mutate(sh.userId)` for the owner-revoke case while `LeaveGardenModal` calls `remove.mutateAsync(me.data.id)` for the leave case.
No other material performance concerns: `shares.data?.map` renders only after load (no render of undefined), `useShares` is gated behind modal mounting, inspector mutation hooks are instantiated in read-only mode but issue no network requests, and there's no work inside hot loops introduced here.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings verified against the actual code:
1. **ShareGardenModal fire-and-forget** — confirmed. `updateRole.mutate`/`remove.mutate` at lines 93/102 have no `onError`; `shares.ts` mutations define only `onSuccess` + `mutationFn`, no `onError`; `queryClient.ts` has no global `MutationCache`/`mutations.onError`. Contrast `objects.ts` uses `onError: toast.error`. Keep.
2. **Missing myRole lockout** — confirmed. `canEditRole`/`isOwnerRole` return false for undefined; `myRole` is `optional()`; GardenEditorPage gates everything on `canEdit`/`isOwner`. Keep.
3. **useMe non-401 stuck button** — confirmed. `auth.ts:31-38` resolves to `null` only on 401, throws otherwise → `me.data` undefined → button `disabled={remove.isPending || !me.data}` with no error/loading UI. Keep.
**Verdict: Minor issues found**
- **`web/src/components/gardens/ShareGardenModal.tsx:93` / `:102` — inline role-change and remove mutations silently swallow failures.** `updateRole.mutate(...)` (line 93) and `remove.mutate(sh.userId)` (line 102) are fire-and-forget. Verified in `web/src/lib/shares.ts`: `useUpdateShareRole` (lines 48-55) and `useRemoveShare` (lines 60-69) define only `mutationFn` and `onSuccess`, no `onError`. `web/src/lib/queryClient.ts` has no global `MutationCache`/`mutations.onError`. So a failed PATCH/DELETE (network drop, 409, 403) produces no UI feedback at all — no Alert, no toast — and the shares list only refetches via `onSuccess`. Contrast the invite path (`:42`) which uses `mutateAsync` + try/catch + `<Alert>`, and `lib/objects.ts` where every mutation has `onError: (err) => toast.error(...)`. Concrete impact: an owner changes a share's role or hits ✕ to revoke, the request fails, and they're left believing it succeeded while the displayed role row still shows the old value. Fix: add `onError` to these two mutations (e.g. `toast.error(...)`, matching `lib/objects.ts`) or switch the handlers to `mutateAsync` with try/catch + the existing `error` state.
- **`web/src/pages/GardenEditorPage.tsx:127` — `canEdit`/`isOwner` are `false` when `myRole` is absent, which can lock an owner out of their own garden with no error surfaced.** Verified in `web/src/lib/gardens.ts`: `canEditRole(undefined)` (line 32-34) and `isOwnerRole(undefined)` (line 37-39) both return `false`, and `myRole` is declared `gardenRoleSchema.optional()` (line 27) "defensively." `GardenEditorPage.tsx:127-128` gates all UI on these. If the server ever omits `my_role` (backend bug, partial deploy, stale cache), the owner lands in read-only viewer mode on their own garden: no Palette (line 160), no Share button (line 152), disabled inspectors (`readOnly={!canEdit}`), all canvas mutations gated off (`canEdit` checks in GardenCanvas). Silent total loss of function with no error state. Worth at least a non-owner fallback or an explicit "couldn't determine your role" error rather than treating unknown as viewer.
- **`web/src/components/gardens/LeaveGardenModal.tsx:40` — a non-401 `useMe` failure leaves the user stuck with a disabled button and no explanation.** Verified in `web/src/lib/auth.ts:29-44`: `meQueryOptions.queryFn` returns `null` only on `ApiError.isUnauthorized` (401); any other error is rethrown, leaving `me.data` undefined. `LeaveGardenModal.tsx:40` sets `disabled={remove.isPending || !me.data}` with no error/loading state for `me`, so a non-401 `useMe` failure keeps "Leave" permanently disabled with no message. Minor unhappy path with no feedback; a loading/error state on `me` would cover it.
</details>
</details>
<details><summary><b>kimi-k2.6: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, the PR is clean. Verified:
- **Role source**: `myRole` is declared `optional()` in the zod schema and comes from the server response; `canEditRole`/`isOwnerRole` both safely default to `false` when undefined. No client-side privilege escalation.
- **Client gating**: The editor gates UI (`Palette`, overlays, inspectors) on `canEdit` derived from server-provided `myRole`. Event handlers in `GardenCanvas` double-check `canEdit` before firing placement mutations — defense in depth acknowledged in code comments.
- **Share dialog**: `ShareGardenModal` only renders share-management UI; actual authorization lives on the backend (`/gardens/:id/shares`). No injection vector — email is sent as a JSON string body, and React auto-escapes interpolated user data (`displayName`, `email`, garden name) in JSX.
- **Leave flow**: `LeaveGardenModal` passes the current user’s own `me.data.id` to `useRemoveShare`; no cross-user revocation from this component.
- **Read-only inspectors**: `<fieldset disabled>` is a UX convenience; the backend remains the authority, and the PR explicitly does not rely on client-side gates alone.
No authz gaps, injection vectors, secret leakage, or unsafe deserialization found.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
The role-gating logic is semantically correct throughout: `canEditRole` and `isOwnerRole` properly derive rights from the server's `my_role`; the canvas, inspector, palette, and share flows are all gated with the right predicates; and the defense-in-depth checks in `GardenCanvas` (`armed && canEdit`, `!canEdit || ...`) ensure stale store state can't bypass viewer restrictions. The read-only inspector fieldset correctly disables all nested controls in one shot, and the mutation hooks target the expected endpoints with the expected payloads. Nothing in this diff introduces a logic error or incorrect domain behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `web/src/components/gardens/GardenCard.tsx:6-16` — `actionClass` and `dangerClass` are copy-pasted verbatim from `PlantCard.tsx`. The exact same 150+ character strings appear in both files. Extract them to a shared location (e.g. `components/ui/actionButton.ts`) so future style tweaks don't diverge.
- `web/src/components/gardens/ShareGardenModal.tsx:100-107` — The remove-share button uses a raw Tailwind string instead of the established `dangerClass` pattern used in `GardenCard`/`PlantCard`. This is inconsistent and will drift if the danger style changes.
- `web/src/editor/Inspector.tsx:113-227` — The `<fieldset disabled={readOnly}>` wrapper was added, but its children (lines 122-227) are not indented inside it; they sit at the same column as the `<fieldset>` tag. This makes the DOM nesting misleading and harder to maintain.
- `web/src/editor/PlopInspector.tsx:127-171` — Same indentation issue: the `<fieldset>` wrapper added in the diff has children at the same column level instead of being indented.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Through the Performance lens, the PR is clean. The read-only viewer mode actually *reduces* work for the majority use-case (viewers) by conditionally omitting the palette, placement overlays, selection handles, and edit-bound mutation hooks. The `canEdit` gating is a primitive boolean derived once at the page level, so it doesn't trigger spurious re-renders. Memoization of `objects`, `plantings`, and `plantsById` in `GardenEditorPage` is preserved. No N+1 patterns, unbounded hot-path loops, or missing pagination on lists that would be material at this scale were introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Based on my verification of the actual source code, all four findings are confirmed. Here is the corrected review:
**Minor issues found**
* **`web/src/components/gardens/ShareGardenModal.tsx:93`** — `updateRole.mutate` and `remove.mutate` are fire-and-forget with no error handling. If the backend returns a 403, 409, or network failure, the user gets zero feedback; the role dropdown snaps back (or the removed user re-appears on next refetch) with no explanation.
**Fix:** Wire both mutations into a local error state (or a toast) and show an `<Alert>` when they fail, mirroring the `add` mutation pattern.
* **`web/src/components/gardens/ShareGardenModal.tsx:50`** — The Modal’s `busy` prop only tracks `add.isPending`, so Escape / backdrop-click can dismiss the dialog while an in-flight `updateRole` or `remove` is still pending. That buries any eventual error and leaves the user unsure whether the action completed.
**Fix:** Pass `busy={add.isPending || updateRole.isPending || remove.isPending}`.
* **`web/src/editor/Inspector.tsx:119`** and **`web/src/editor/PlopInspector.tsx:138`** — Every `onBlur` handler inside the `<fieldset disabled={readOnly}>` calls `patch(...)` without checking `readOnly`. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a new `myRole`), React re-renders with `readOnly=true`, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403.
**Fix:** Guard the `patch` helpers (or each `onBlur`) with an early `if (readOnly) return;`.
</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>
Examining the change through the security lens: I verified the backend authorization (service/shares.go, store/shares.go), the API layer (api/shares.go uses `mustActor(c).ID` from session, parameterized queries throughout), the data layer (lib/shares.ts uses zod parsing of server responses), and the client-side gating (lib/gardens.ts derives `canEdit`/`isOwner` solely from server-provided `myRole`).
VERDICT: No material issues found
- Authorization is enforced server-side at every share endpoint (`requireGardenRole(ctx, actorID, gardenID, roleOwner)` for list/add/update; owner-or-self rule for remove in `internal/service/shares.go:19,30,53,68`). The client's role badges and Share button are derived from the server's `myRole`, not guessed.
- Email invite input (`ShareGardenModal.tsx`) is sent as JSON to a parameterized lookup (`store/shares.go` `GetUserByEmail`-equivalent via `GetUserByEmail` in service), no injection surface; response `displayName`/`email` are rendered as React text (auto-escaped), no `dangerouslySetInnerHTML`.
- Read-only viewer mode uses a disabled `<fieldset>` (Inspector, PlopInspector) which prevents `onBlur`/`onChange` mutations from firing, and action buttons are conditionally hidden; plus canvas-level `canEdit` re-checks in `GardenCanvas.tsx` are genuine defense in depth since they'd 403 server-side anyway.
- `useShares` is only mounted from `ShareGardenModal`, which is only reachable via owner-gated UI; backend also 403s non-owners, so no authz gap if a stale client opened it.
- `LeaveGardenModal` removes the actor's own share using `me.data.id` from `useMe()` (the authenticated session), not from URL/props — a recipient cannot supply another user's id through this path; the backend `RemoveShare` additionally enforces `actorID == targetUserID` for non-owners.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both mutations confirmed fire-and-forget with no `onError`. For the third finding, react-query preserves `data` during background refetch (only `isPending` on initial load with no data), so the "renders empty during refetch" claim is incorrect — dropping it.
## VERDICT: Minor issues
**Findings:**
- **`web/src/components/gardens/ShareGardenModal.tsx:93`** — `updateRole.mutate(...)` fires with no error surface. The hook in `lib/shares.ts:48-55` defines only `onSuccess` (invalidation), no `onError`, and the call site uses fire-and-forget `mutate` (no try/catch). If the PATCH fails (network error, 403 after a role-change race, version conflict), the user gets no feedback: the `<select>` flips to the new value, then either silently reverts on refetch or silently stays wrong if the request fails before invalidation. The add-share path correctly uses `mutateAsync` + try/catch, but the inline role change does not. Suggested fix: add `onError` to the hook (e.g. `toast.error`) or wrap the call in try/catch with surfaced local error state.
- **`web/src/components/gardens/ShareGardenModal.tsx:102`** — `remove.mutate(sh.userId)` is fire-and-forget with no error surface. `useRemoveShare` (`lib/shares.ts:60-69`) defines only `onSuccess`, no `onError`, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- **Duplicated `actionClass` / `dangerClass` constants** — `web/src/components/gardens/GardenCard.tsx:5-10` defines the exact same two class strings as `web/src/components/plants/PlantCard.tsx:5-10` (verified identical character-for-character). This is copy-paste that should be shared — e.g. extract to `web/src/components/ui/button.ts` or a `cardActions` helper. As more card components appear, this drift will diverge.
- **Inconsistent `<select>` usage within `ShareGardenModal`** — `web/src/components/gardens/ShareGardenModal.tsx:91-99` uses a raw `<select>` with manual `fieldControlClass` + `cn` for the inline role changer, while the same file (line 62) uses the `Select` component for the invite form's role picker. Within one file, picking both patterns is confusing; either use `Select` for both or extract a small inline-role select. The raw form also bypasses the `useFieldId`/label wiring the shared component provides.
- **`<fieldset>` wrapping has misleading indentation** — In `web/src/editor/Inspector.tsx:113-227` and `web/src/editor/PlopInspector.tsx:127-171`, the `<fieldset>` opens at one indent level but its children (the grid `<div>`, the rotation/label TextFields, the color row, the Plantable checkbox, the Notes TextArea) keep their *pre-existing* indentation rather than being re-indented one level. The result reads as if those controls are siblings of the fieldset, not children — a structural mismatch that will trip the next reader doing a git-blame or refactor. A one-time re-indent of the wrapped block (or a prettier format pass) would fix it cleanly.
- **Raw enum value surfaced in UI badge** — `GardenCard.tsx:42` renders `shared · {garden.myRole}`, which prints the lowercase wire value (`viewer`/`editor`). Elsewhere in the PR the same roles are presented as `Viewer (read-only)` / `Editor (can edit)` (`ShareGardenModal.tsx:13-16`). Minor, but a shared `roleLabel(role)` helper would keep the casing/wording consistent and centralize the display string.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
- I checked the data layer (`web/src/lib/shares.ts`) and the share/leave modals for hot-path waste, unnecessary refetches, and unbounded work. The mutations invalidate only their own `sharesKey(gardenId)` plus (for `useRemoveShare`) the `['gardens']` list key — no broad broadcast, no N+1.
- `useShares(garden.id)` in `ShareGardenModal` only fires when the modal is mounted (conditional render in `GardensPage.tsx:75` and `GardenEditorPage.tsx:227`), so the owner-only shares endpoint isn't polled on every garden view.
- `useRemoveShare` invalidates `['gardens']` on every removal, which causes one extra list refetch even when the owner revokes a share (their garden list doesn't change). This is a single additional request gated behind a manual click, not a hot path — not material.
- The role `<select>` and remove `✕` in `ShareGardenModal.tsx:91-107` fire `updateRole.mutate` / `remove.mutate` directly with no debounce/disabled-during-pending, but each triggers a single targeted invalidation — no cascading refetches, no quadratic behavior.
- No new allocations on the canvas hot loop; `canEdit` is a cheap boolean gate around `setArmedKind`/overlay mounting. The `plantsById` Map rebuild is pre-existing and unchanged.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## VERDICT: Minor issues
Verified by reading `ShareGardenModal.tsx`, `shares.ts`, `Modal.tsx`, `gardens.ts`, and `GardenEditorPage.tsx`.
- **`ShareGardenModal.tsx:93` and `:102` — role change and share removal silently swallow errors.** Both `updateRole.mutate({ userId, role })` and `remove.mutate(sh.userId)` are fire-and-forget. The hooks in `shares.ts:39-69` only define `onSuccess` (no `onError`), the call sites attach no `onError`, and `.mutate` (unlike `.mutateAsync`) does not throw — react-query stashes the error in `mutation.error` but surfaces nothing. So a 403/409/network failure on a role change or a revoke leaves the user with zero feedback: the `<select>` visually flips, then silently reverts on the next refetch (or doesn't, if the refetch also fails). Compare `onInvite` at `:41-46`, which wraps `add.mutateAsync` in try/catch and shows an `Alert`. Fix: either switch to `mutateAsync` with try/catch + `setError`, or pass `{ onError: (err) => setError(errorMessage(err, …)) }` to each call (and/or an `onError` on the hooks). The role-change `<select>` should also be disabled while its own mutation is pending to avoid stacking a second PATCH on an in-flight one.
- **`ShareGardenModal.tsx:50` — `busy` only reflects `add.isPending`, so update/remove mutations don't lock the modal.** Confirmed `Modal.tsx:32,42` suppresses Escape/backdrop-close only when `busy` is true. With `busy={add.isPending}`, a role change or revoke in flight leaves `busy=false`, so Escape/backdrop click can dismiss the dialog mid-request. Fix: derive `busy={add.isPending || updateRole.isPending || remove.isPending}`.
- **`ShareGardenModal.tsx:100-107` — destructive revoke (`✕`) has no confirmation and no per-row busy state.** Unlike `LeaveGardenModal` (which confirms via a modal) and the object delete flow in `Inspector.tsx` (two-step "Confirm delete"), the owner's share removal at `:100-107` is one click with no undo, no busy indicator on the row, and (per the first finding) no error feedback. A mis-tap revokes someone's access with no recourse. Suggest a confirm step or at minimum a disabled state keyed to that row's pending removal.
- **`GardenEditorPage.tsx:127-128` — safe-default on missing `myRole`, but misleading "View only" indicator.** `canEditRole`/`isOwnerRole` (`gardens.ts:32-39`) return false when `myRole` is undefined — the right defensive default. But `!canEdit` at `:157-159` then renders the "👁 View only" banner even when the role is simply *missing* rather than genuinely `viewer`. The schema marks `myRole` optional (`gardens.ts:25-28`), so this path is reachable in principle. Not blocking — the comment says `myRole` is present on every accessible response — but worth a guard so an owner with a transiently-missing role isn't told they're view-only.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/pages/GardenEditorPage.tsx:135-143`** — `onPickPlant` fires `updatePlanting.mutate` for a \"change\" operation without checking `canEdit`. Every other mutation path in the editor (create object, create planting, move/resize via `SelectionOverlay`, delete object, remove planting) is either gated by a `canEdit` check or hidden behind a `canEdit`-conditional mount. The `PlantPicker` modal is the only remaining un-gated mutation path: a viewer who manipulates React state (e.g., via DevTools or by forcing the modal open) can change a plop's plant. The PR explicitly calls this out as a defense-in-depth concern for canvas handlers, but the picker callback was missed. Add an early `if (!canEdit) return` guard in `onPickPlant`.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
I reviewed the PR thoroughly through the correctness lens, focusing on semantic/domain correctness, role gating logic, API contract consistency, and state/cache behavior.
Key areas verified:
- **Role schema and gating**: `canEditRole` and `isOwnerRole` correctly map `owner`/`editor`/`viewer` to edit/owner permissions. The `undefined` fallback returns `false` — a safe defensive choice.
- **GardenCanvas defense in depth**: `canEdit` gates object creation (`onCanvasPointerDown`), plop placement (`onPlace`), and overlay mounting (`SelectionOverlay`, `PlopOverlay`, placement capture). Viewers can still select objects for read-only inspection, which is intentional.
- **Inspector/PlopInspector**: `<fieldset disabled={readOnly}>` correctly disables all nested inputs in one shot. Mutation buttons (Delete, Remove, Change plant, Plant here) are conditionally hidden.
- **Share/Leave modals**: `useRemoveShare(garden.id)` with `me.data.id` correctly removes the current user's share. `ShareGardenModal` uses the proper API paths and zod schemas.
- **Cache invalidation**: `useRemoveShare` invalidates both `sharesKey` and `['gardens']` (needed for the leave flow). `useAddShare` and `useUpdateShareRole` only invalidate `sharesKey`, which is correct for owner-local share-list UI. Cross-user list staleness is an inherent client-side caching limitation, not a correctness bug in this code.
- **Type safety**: The `Garden` schema includes `myRole: gardenRoleSchema.optional()`, and `ShareRole` is correctly constrained to `'viewer' | 'editor'` (no owner sharing).
The logic is sound and the defensive gating is consistent throughout.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `web/src/editor/Inspector.tsx:113` — The `<fieldset>` wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.
- `web/src/editor/PlopInspector.tsx:127` — Same indentation problem: the `<fieldset>` tag and its first child `<div>` (line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.
- `web/src/components/gardens/ShareGardenModal.tsx:91` — Uses a raw `<select>` element (with manually imported `fieldControlClass` and `cn`) for the inline role changer, while the invite form 20 lines above uses the project's `<Select>` component. This creates two styling paths for the same control and duplicates the `viewer`/`editor` options already defined in the module-level `roleOptions` array. Prefer `<Select>` (or map `roleOptions`) to keep the UI consistent and DRY.
- `web/src/components/gardens/ShareGardenModal.tsx:84` — The `<ul className="flex flex-col gap-2">` is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside the `shares.isSuccess && shares.data.length > 0` branch.
- `web/src/components/gardens/GardenCard.tsx:5` — `actionClass` and `dangerClass` repeat the same long Tailwind base string; extract a shared `baseActionClass` (or use `cn`) to remove the copy-paste.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
I examined the PR through the Performance lens and found no efficiency regressions. The changes add role-based conditional rendering gates (`canEdit`, `isOwner`) which are cheap boolean checks derived from stable server data. For viewers, the PR actually _reduces_ work by not mounting interactive overlays (`SelectionOverlay`, `PlopOverlay`) and skipping placement capture — a perf win for the read-only path.
Specific areas checked:
- **Query invalidation** in `lib/shares.ts` is targeted and correct; no over-invalidation or missing cache updates that would cause extra network churn.
- **Share dialog list** (`ShareGardenModal.tsx`) renders without virtualization, but garden sharing lists are inherently small — no material concern.
- **Hot-path scans** in `GardenEditorPage.tsx` and `GardenCanvas.tsx` (`.find()` for `selectedObject`, `focusedObject`, etc.) were pre-existing and untouched by this PR.
- **No new allocations inside loops** or event handlers beyond the pre-existing patterns.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/components/gardens/ShareGardenModal.tsx:91-102`** — `updateRole.mutate` and `remove.mutate` are fire-and-forget with no error handling, pending state, or disabled UI. If the server rejects a role change or removal (network error, 403, 404, version conflict), the user sees zero feedback; the list item stays in place as if nothing happened. Worse, the native `<select>` and remove button remain interactive while the mutation is in flight, so the user can trigger duplicate or conflicting requests.
**Fix:** Wrap both in `try/catch` using `mutateAsync`, surface errors in a dedicated list error state, and disable each row’s controls while `updateRole.isPending || remove.isPending`. Also widen the Modal’s `busy` prop to cover all three pending states, or at least disable the Done button while any mutation is active.
- **`web/src/editor/Inspector.tsx:69-70,76-83,119,131,141,151,161,173-177,191-201,225`** — The `patch` helper and dimension commit helpers are called from every `onBlur`. If `readOnly` flips from `false` → `true` while an input has focus (e.g., the user’s role is downgraded by a background refetch), the disabled `<fieldset>` causes the focused input to blur, firing its `onBlur` handler and sending an unintended PATCH that the server will 403.
**Fix:** Guard `patch` (and the inline blur commits) with `if (readOnly) return;` so a disabled fieldset never fires mutations.
- **`web/src/editor/PlopInspector.tsx:61-62,67-75,77-88,90-93,138,150,160,169,180`** — Same issue as Inspector: `patch`, `commitRadius`, `commitCount`, and `commitPlanted` don’t check `readOnly`. A role downgrade while an input is focused will blur the now-disabled fieldset and fire mutations.
**Fix:** Add `if (readOnly) return;` to `patch` and each commit helper.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Critical:
- useAddShare/useUpdateShareRole no longer parse the response with the list's
Share schema — the backend returns a bare GardenShare (no email/displayName),
so every successful invite/role-change was throwing and showing a false error.
onSuccess refetches the list, which has the full data.
- ShareGardenModal surfaces role-change / remove failures (mutate onError → the
dialog's Alert) instead of swallowing them; Modal busy now covers all three
pending states.
Robustness / defense in depth:
- Inspector/PlopInspector patch() early-returns when readOnly, so a blur can't
fire a mutation if the role was downgraded mid-edit.
- onPickPlant guards canEdit.
- Ownership is now the authoritative ownerId==me check (GardenCard via a
currentUserId prop, GardenEditorPage via useMe), so a missing my_role can never
lock an owner out or show them a Leave button.
Cleanup:
- Shared cardActionClass/cardDangerClass (GardenCard + PlantCard).
- Normalized the read-only <fieldset> indentation; dropped the dead useShares
`enabled` param.
tsc --noEmit clean; 24/24 vitest; production build green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit c2dd93a93d into main2026-07-19 04:14:30 +00:00
steve
deleted branch phase-6-sharing-ui2026-07-19 04:14:31 +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 #17. Part of epic #20 (Phase 6 — Sharing). Builds on the #16 API.
What
Data layer
lib/shares.ts— zodShareshape +useShares/useAddShare/useUpdateShareRole/useRemoveShareover/gardens/:id/shares.lib/gardens.ts—myRoleon the garden schema +canEditRole/isOwnerRolehelpers. Role always comes from the server'smy_role, never guessed client-side.Share dialog (owner only)
ShareGardenModal— invite an existing user by email as viewer/editor, change a share's role inline, remove a share; surfaces the backend's friendly "no account with that email". Reachable from the garden card and the editor (owner's Share button).Garden list
GardenCard— a "shared · viewer/editor" badge on non-owned gardens (owned unbadged). Owner gets Share / Edit / Delete; a recipient gets Leave (LeaveGardenModalremoves their own share and drops the garden from their list).Read-only viewer mode (editor)
Gated on a single
canEditflag derived frommy_role:<fieldset>makes every field read-only in one shot, and Change / Delete / Remove / "Plant here" are hidden — plus a "View only" indicator.canEdit, so a missed UI gate can't fire a mutation that would 403 (the edit-firing overlays aren't mounted for viewers to begin with).Checks
tsc --noEmitclean · 24/24 vitest ·vite buildgreen.Acceptance criteria
tsc --noEmitclean.🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 04:05:38Z
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 — 17 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -3,2 +2,4 @@import { isOwnerRole, type Garden } from '@/lib/gardens'import { formatDimensions } from '@/lib/units'const actionClass =🟡 actionClass/dangerClass duplicated verbatim from PlantCard — should be shared
maintainability · flagged by 3 models
actionClass/dangerClassconstants —web/src/components/gardens/GardenCard.tsx:5-10defines the exact same two class strings asweb/src/components/plants/PlantCard.tsx:5-10(verified identical character-for-character). This is copy-paste that should be shared — e.g. extract toweb/src/components/ui/button.tsor acardActionshelper. As more card components appear, this drift will diverge.🪰 Gadfly · advisory
@@ -16,2 +27,4 @@onDelete: () => voidonLeave: () => void}) {const owner = isOwnerRole(garden.myRole)🟡 Garden with undefined myRole is treated as non-owner and shown a Leave (mutating) button with no badge; fail-closed direction is wrong for unknown role
correctness · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:30,40,64— a garden withmyRole === undefinedis treated as non-owner and gets aLeavebutton with no badge.isOwnerRole(undefined)is false (confirmed ingardens.ts:37), so the footer falls into the!ownerbranch and rendersLeave; the badge is gated on!owner && garden.myRole(truthy), so a missing role yields a card with no role label and aLeavebutton that callsremove.mutateAsync(me.data.id)— a self-share delete that like…🪰 Gadfly · advisory
@@ -26,0 +39,4 @@<h3 className="truncate font-semibold text-fg">{garden.name}</h3>{!owner && garden.myRole && (<span className="shrink-0 rounded bg-border/60 px-1.5 py-0.5 text-[10px] font-medium uppercase tracking-wide text-muted">shared · {garden.myRole}⚪ Badge prints raw lowercase enum myRole; casing/wording inconsistent with role labels in ShareGardenModal
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +37,4 @@<Button type="button" variant="ghost" onClick={onClose} disabled={remove.isPending}>Cancel</Button><Button type="button" variant="danger" onClick={onConfirm} disabled={remove.isPending || !me.data}>🟡 Non-401 useMe failure leaves Leave button disabled with no user-facing message
error-handling · flagged by 1 model
web/src/components/gardens/LeaveGardenModal.tsx:40— a non-401useMefailure leaves the user stuck with a disabled button and no explanation. Verified inweb/src/lib/auth.ts:29-44:meQueryOptions.queryFnreturnsnullonly onApiError.isUnauthorized(401); any other error is rethrown, leavingme.dataundefined.LeaveGardenModal.tsx:40setsdisabled={remove.isPending || !me.data}with no error/loading state forme, so a non-401useMefailure keeps "Leave" permanently di…🪰 Gadfly · advisory
@@ -0,0 +47,4 @@}return (<Modal title="Share garden" onClose={onClose} busy={add.isPending}>🟠 Modal busy prop does not cover updateRole or remove pending states
error-handling · flagged by 3 models
web/src/components/gardens/ShareGardenModal.tsx:50— The Modal’sbusyprop only tracksadd.isPending, so Escape / backdrop-click can dismiss the dialog while an in-flightupdateRoleorremoveis still pending. That buries any eventual error and leaves the user unsure whether the action completed. Fix: Passbusy={add.isPending || updateRole.isPending || remove.isPending}.🪰 Gadfly · advisory
@@ -0,0 +81,4 @@{shares.isSuccess && shares.data.length === 0 && (<p className="text-sm text-muted">Not shared with anyone yet.</p>)}<ul className="flex flex-col gap-2">🟡 Empty ul rendered during loading and error states
maintainability · flagged by 1 model
web/src/components/gardens/ShareGardenModal.tsx:84— The<ul className="flex flex-col gap-2">is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside theshares.isSuccess && shares.data.length > 0branch.🪰 Gadfly · advisory
@@ -0,0 +88,4 @@<p className="truncate text-sm font-medium text-fg">{sh.displayName}</p><p className="truncate text-xs text-muted">{sh.email}</p></div><select🔴 updateRole.mutate / remove.mutate silently swallow errors — no onError, no user feedback on failed role change or revoke
correctness, error-handling, maintainability · flagged by 5 models
web/src/components/gardens/ShareGardenModal.tsx:91— Uses a raw<select>element (with manually importedfieldControlClassandcn) for the inline role changer, while the invite form 20 lines above uses the project's<Select>component. This creates two styling paths for the same control and duplicates theviewer/editoroptions already defined in the module-levelroleOptionsarray. Prefer<Select>(or maproleOptions) to keep the UI consistent and DRY.🪰 Gadfly · advisory
@@ -0,0 +97,4 @@<option value="viewer">Viewer</option><option value="editor">Editor</option></select><button🔴 remove mutation has no error handling or pending UI
correctness, error-handling, maintainability · flagged by 5 models
web/src/components/gardens/ShareGardenModal.tsx:102—remove.mutate(sh.userId)is fire-and-forget with no error surface.useRemoveShare(lib/shares.ts:60-69) defines onlyonSuccess, noonError, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.🪰 Gadfly · advisory
@@ -98,2 +100,3 @@{object.plantable && onFocus && ({readOnly && (<p className="rounded-md bg-border/40 px-2 py-1 text-xs text-muted">View only — you can't edit this garden.</p>🟡 Duplicated 'View only' badge string/styling across Inspector, PlopInspector, and GardenEditorPage
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -111,1 +111,3 @@/>{/* A disabled fieldset makes every control below read-only for viewers inone shot (no per-input disabled). */}<fieldset disabled={readOnly} className="flex min-w-0 flex-col gap-3 border-0 p-0">🟠 Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable
maintainability · flagged by 5 models
web/src/editor/Inspector.tsx:113— The<fieldset>wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.🪰 Gadfly · advisory
@@ -112,0 +116,4 @@name="name"value={name}onChange={(e) => setName(e.target.value)}onBlur={() => name !== object.name && patch({ name })}🟠 onBlur handlers in disabled fieldset can fire mutations after role downgrade
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:119andweb/src/editor/PlopInspector.tsx:138— EveryonBlurhandler inside the<fieldset disabled={readOnly}>callspatch(...)without checkingreadOnly. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a newmyRole), React re-renders withreadOnly=true, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403. Fix: Guard the `patch…🪰 Gadfly · advisory
@@ -117,2 +124,4 @@)}</div><fieldset disabled={readOnly} className="flex min-w-0 flex-col gap-3 border-0 p-0">🟠 Inconsistent indentation inside fieldset wrapper makes JSX hierarchy unreadable
maintainability · flagged by 5 models
web/src/editor/PlopInspector.tsx:127— Same indentation problem: the<fieldset>tag and its first child<div>(line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.🪰 Gadfly · advisory
@@ -0,0 +32,4 @@}/** The shares on a garden (owner-only endpoint; pass enabled=false to skip). */export function useShares(gardenId: number, enabled = true) {⚪ useShares
enabledparameter has no caller; dead surfacemaintainability · flagged by 1 model
web/src/editor/Inspector.tsx:113-227&PlopInspector.tsx:127-171— fieldset children not re-indented. The new<fieldset>wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g.<div className="grid grid-cols-2 gap-2">at Inspector:122 sits at the same indent as the<fieldset>that owns it; same in PlopInspector:128). The closing</fieldset>at 227 is likewise dedented past the<TextArea>it contains. This flatt…🪰 Gadfly · advisory
@@ -0,0 +40,4 @@const qc = useQueryClient()return useMutation({mutationFn: async (input: { email: string; role: ShareRole }): Promise<Share> =>shareSchema.parse(await api.post(`/gardens/${gardenId}/shares`, input)),🔴 useAddShare parses the POST /shares response with a schema requiring email/displayName, but the backend's AddShare returns a bare GardenShare without them — every successful invite throws and shows a false 'Could not share the garden' error
correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +49,4 @@const qc = useQueryClient()return useMutation({mutationFn: async ({ userId, role }: { userId: number; role: ShareRole }): Promise<Share> =>shareSchema.parse(await api.patch(`/gardens/${gardenId}/shares/${userId}`, { role })),🔴 useUpdateShareRole parses the PATCH /shares/:userId response with the same over-strict schema; the backend's UpdateShareRole also omits email/displayName, so every successful role change throws and is swallowed silently (no onError in ShareGardenModal), leaving the UI reverted to the stale role
correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +63,4 @@mutationFn: (userId: number) => api.delete(`/gardens/${gardenId}/shares/${userId}`),onSuccess: () => {qc.invalidateQueries({ queryKey: sharesKey(gardenId) })qc.invalidateQueries({ queryKey: ['gardens'] })🟡 useRemoveShare invalidates ['gardens'] on every remove, causing avoidable gardens-list refetches in the owner-revoke path
performance · flagged by 1 model
web/src/lib/shares.ts:66—useRemoveShare.onSuccessunconditionally invalidates['gardens']on every remove. This is correct and necessary for the leave path (the garden leaves the recipient's list), but it also fires for the owner revoke path inShareGardenModal. When the owner manages shares fromGardensPage(where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-li…🪰 Gadfly · advisory
@@ -121,2 +124,4 @@unitPref: g.unitPref,}// Role-driven gating from the server's my_role — never guessed client-side.const canEdit = canEditRole(g.myRole)🟠 canEdit/isOwner false on missing myRole can lock an owner out of their own garden with no error surfaced
error-handling · flagged by 2 models
web/src/pages/GardenEditorPage.tsx:127—canEdit/isOwnerarefalsewhenmyRoleis absent, which can lock an owner out of their own garden with no error surfaced. Verified inweb/src/lib/gardens.ts:canEditRole(undefined)(line 32-34) andisOwnerRole(undefined)(line 37-39) both returnfalse, andmyRoleis declaredgardenRoleSchema.optional()(line 27) "defensively."GardenEditorPage.tsx:127-128gates all UI on these. If the server ever omitsmy_role(backend bug,…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 21 findings (7 with multi-model agreement)
web/src/components/gardens/ShareGardenModal.tsx:91web/src/components/gardens/ShareGardenModal.tsx:100web/src/editor/Inspector.tsx:113web/src/editor/PlopInspector.tsx:127web/src/components/gardens/ShareGardenModal.tsx:50web/src/components/gardens/GardenCard.tsx:5web/src/pages/GardenEditorPage.tsx:12714 single-model findings (lower confidence)
web/src/lib/shares.ts:43web/src/lib/shares.ts:52web/src/editor/Inspector.tsx:69web/src/editor/Inspector.tsx:119web/src/editor/PlopInspector.tsx:61web/src/editor/PlopInspector.tsx:138web/src/pages/GardenEditorPage.tsx:135web/src/components/gardens/GardenCard.tsx:30web/src/components/gardens/LeaveGardenModal.tsx:40web/src/components/gardens/ShareGardenModal.tsx:84web/src/editor/Inspector.tsx:102web/src/lib/shares.ts:66web/src/components/gardens/GardenCard.tsx:42web/src/lib/shares.ts:35Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Notes (all verified against the checked-out repo, not just the diff):
canEdit/readOnlygating added acrossGardenCanvas.tsx,Inspector.tsx,PlopInspector.tsx, andGardenEditorPage.tsxis correctly backed by server-side authorization —internal/service/objects.goandinternal/service/plantings.gorequireroleEditorminimum viarequireGardenRole/objectForRolefor every mutating call, so a bypassed or stale client-side check can't produce an unauthorized write; it would just 403.ShareGardenModal's owner-only actions (ListShares,AddShare,UpdateShareRole) map tointernal/service/shares.gocalls that all callrequireGardenRole(..., roleOwner)— the modal being reachable in principle by a non-owner (it isn't, sinceGardenCard/GardenEditorPageonly render the Share entry point forisOwnerRole(myRole)) would still be rejected server-side.LeaveGardenModalcallsuseRemoveShare(garden.id).mutateAsync(me.data.id)— server-sideRemoveShareenforces owner-or-self, so a user can only remove their own share this way; no way to remove someone else's.myRoleis a server-populated, zod-validated enum (gardenRoleSchemainlib/gardens.ts) — never client-derived, matching the PR's claim, andcanEditRole/isOwnerRoletreat unknown/missing role as non-editing (fail-closed default).ShareGardenModal's rendering ofsh.email/sh.displayName(React auto-escapes, nodangerouslySetInnerHTML).GardenEditorPage.tsx:96-108— it only touches local selection/UI state (Escape), no mutation call, so it can't fire an edit for viewers.One pre-existing, out-of-scope observation (not introduced by this diff, part of the already-merged #16 API):
AddSharereturning a distinct "no account with that email" error does allow any garden owner to enumerate whether an arbitrary email has an account. That's backend behavior from a prior PR and this diff only surfaces the message string in the UI, so I'm not flagging it as a finding against this change.🎯 Correctness — Blocking issues found
Confirmed:
updateRole.mutateat line 93 has noonError/onSuccesshandler, so a thrownZodErrorthere is silently swallowed (unhandled mutation rejection), matching the draft's claim.errorMessageonly unwrapsApiError; aZodErrorfalls through to the generic fallback string, matching theonInviteclaim too.Both findings hold up against the actual code — the type mismatch is real and both consumption paths behave as described.
VERDICT: Blocking issues found
web/src/lib/shares.ts:39-53—useAddShareanduseUpdateShareRoleparse their mutation responses withshareSchema, which requiresemailanddisplayNameas mandatory strings (shares.ts:11-22). But the backend endpoints they hit return a baredomain.GardenShare:addShare(internal/api/shares.go:44-49) callssvc.AddShare(internal/service/shares.go:30-50) which returnsstore.CreateShare's*domain.GardenShare(internal/store/shares.go:83), andupdateShare(internal/api/shares.go:52-71) callssvc.UpdateShareRole(internal/service/shares.go:53-61) which returnsstore.UpdateShareRole's*domain.GardenShare(internal/store/shares.go:101).domain.GardenShare(internal/domain/domain.go:140-149) has onlyid,gardenId,userId,role,createdBy,version,createdAt,updatedAt— noemail/displayName. OnlyListSharesForGarden(internal/store/shares.go:49, used bylistShares/useShares) returns the joineddomain.ShareWithUser, which adds those two fields (domain.go:151-157).Consequence:
shareSchema.parse(...)throws on every successful invite and every successful role change, even though the server request succeeded (the share row was created/updated).ShareGardenModal.onInvite(ShareGardenModal.tsx:34-45), the thrown error is caught anderrorMessage(err, 'Could not share the garden.')is shown — since aZodErrorisn't anApiError,errorMessage(web/src/lib/api.ts:130-133) falls back to that generic string. The owner sees "Could not share the garden" for an invite that actually succeeded, andonSuccess(which invalidates the shares list) never fires because the mutation promise rejected before reaching it.ShareGardenModal.tsx:93,updateRole.mutate({...})has no error handler at all, so a role-change PATCH that succeeds server-side fails client-side silently — no error shown, and the<select>snaps back to the stale role since the cache was never invalidated.Fix: either have the backend return the joined
ShareWithUserrow fromAddShare/UpdateShareRole, or giveshareSchemaa variant withemail/displayNameoptional for the add/update responses, or refetchuseSharesafter add/update instead of parsing the mutation's own response as aShare.🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed by direct file inspection:
Inspector.tsxlines 113–227 andPlopInspector.tsxlines 127–171 both show the exact indentation mismatch described (only the first/last fieldset children reindented, the body left at the outer indentation level). Also confirmed no.prettierrc/.eslintrc/eslint.config.*inweb/, and no formatting step inpackage.jsonscripts.VERDICT: Minor issues
web/src/editor/Inspector.tsx:113-227— The new<fieldset>wrapper isn't consistently indented.TextField"Name" (114-120) andTextArea"Notes" (219-226) are indented one level in (8 spaces, correctly nested under the fieldset), but everything between them — the width/height/x/y grid (122-163), the rotation field (165-177), the color picker row (179-207), and the "Plantable" checkbox (209-217) — is still at the fieldset's own indentation level (6 spaces), even though it's also inside the fieldset. No prettier/eslint config exists inweb/and the build/test scripts don't touch formatting, so nothing will catch or auto-fix this. Fix: reindent the full fieldset body one level so nesting is visually accurate.web/src/editor/PlopInspector.tsx:127-171— Same pattern: the<fieldset>opens at 127, but the radius/count grid (128-153) and the "Label" field (155-161) stay at the outer indentation while the "Planted"TextField(163-170) is correctly indented as a fieldset child. Same fix as above.Everything else in this lens is clean:
GardenCard.tsxfactors the two repeated button class strings intoactionClass/dangerClassrather than leaving them duplicated across five buttons;LeaveGardenModal.tsxmirrors the existingDeleteGardenModal.tsxstructure closely, which is the right call for consistency; thecanEdit/readOnlyprop threading throughGardenCanvas,Inspector,PlopInspector, andGardenEditorPageis a single, traceable flag rather than being re-derived in multiple places; andlib/shares.tsfollows the existinglib/gardens.ts-style query/mutation hook pattern. The inline per-share<select>inShareGardenModal.tsxduplicates the viewer/editor option list already inroleOptions, but the two need different label text and the sharedSelectUI component forces a visible<label>layout that doesn't fit the compact inline row, so hand-rolling it there is a reasonable choice — not flagging it.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens only (efficiency regressions, unnecessary allocations, hot-loop work, unbounded growth, missing pagination, blocking calls, quadratic behavior).
Findings:
web/src/editor/GardenCanvas.tsx— the newcanEditgating (canEdit && selectedObject && ...,canEdit && selectedPlop && ...,canEdit && focusedObject && armedPlant && ...) only adds O(1) boolean checks and, for viewers, skips mountingSelectionOverlay/PlopOverlay/the placement-capture<rect>entirely — a slight net win, not a regression. Verified by reading the full render path (lines 32–233); the pre-existing.find()scans forselectedObject/selectedPlop/focusedObject(lines 104–107) are unchanged by this diff and out of scope.web/src/lib/shares.ts—useShares/useAddShare/useUpdateShareRole/useRemoveSharefollow the same invalidate-on-mutate pattern already used inlib/auth.ts(verified by readingauth.ts). No pagination onGET /gardens/:id/shares, but this is a per-garden collaborator list, realistically small, and mirrors the app's existing non-paginated list endpoints — not a regression introduced here.web/src/components/gardens/ShareGardenModal.tsx—useShares(garden.id)fires once when the modal mounts (only rendered whensharing/dialog.kind === 'share'is true), and the shares list render (shares.data?.map) is a flat, unmemoized map over a small array — no N+1, no quadratic work.web/src/components/gardens/GardenCard.tsx—actionClass/dangerClasswere hoisted to module-level string constants (previously inline JSX literals); this is perf-neutral to slightly better, not worse.Inspector.tsx/PlopInspector.tsx— wrapping existing fields in a single<fieldset disabled={readOnly}>is a structural change only, no added render cost.Nothing in this diff introduces N+1 queries, extra allocations in hot paths, missing limits, or blocking calls. The canvas defense-in-depth checks are cheap and, for viewers, actually reduce mounted overlay work.
🧯 Error handling & edge cases — Minor issues
Confirmed:
UpdateShareRoleandDeleteSharein the store genuinely returnErrNotFoundon a missing row (backing the draft's failure-scenario claim), and the two mutations inShareGardenModal.tsx(web/src/lib/shares.ts:48-69) have noonErrorat the hook level — so.mutate()calls at lines 93/102 with no options truly surface zero feedback. TheDeleteGardenModal.tsxconvention (mutateAsync+ try/catch +Alert) checks out exactly as claimed. Both findings are verified.VERDICT: Minor issues
web/src/components/gardens/ShareGardenModal.tsx:93and:102—updateRole.mutate(...)andremove.mutate(sh.userId)are fire-and-forget: noonError, no.isErrorrendering, nothing. Every other mutation in this codebase (the invite form at:42, andDeleteGardenModal.tsx:16) usesmutateAsync+ try/catch + anAlert. Confirmed the underlying hooks (web/src/lib/shares.ts:48-69) define noonErroreither, and the server-side calls can genuinely fail:internal/store/shares.go:101-117,120-134—UpdateShareRole/DeleteSharereturnErrNotFoundif the share row is already gone (e.g. the owner acting from a stale list, or the recipient just left), whichinternal/service/shares.go:60,68-79propagates (plusErrForbiddenfor a bad actor). When that happens here, the role<select>or the row just silently fails to change/disappear with zero feedback..mutate(..., { onError: (err) => setError(errorMessage(err, '...')) })(or mutateAsync/try-catch, matching the invite-form pattern) and render the error.web/src/components/gardens/ShareGardenModal.tsx:93/:102(same lines, related edge case) — clicks aren't disabled while a mutation is pending (nodisabled={updateRole.isPending}/disabled={remove.isPending}), so a slow response lets a user fire duplicate PATCH/DELETE requests on the same row before the first resolves. Given the missing error handling above, a duplicate DELETE returning 404 would also go unnoticed. Low severity since the endpoints are effectively idempotent, but worth a quickdisabledguard.glm-5.2:cloud (ollama-cloud) — 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 frontend change is well-behaved:
canEditRole(undefined)andisOwnerRole(undefined)both returnfalse(verified atweb/src/lib/gardens.ts).myRoleisoptional()on the schema, so a missing/ malformed server role degrades to read-only viewer rather than escalating — the safe default.canEdit/isOwnercome solely fromg.myRole(GardenEditorPage.tsx:127-128); no client-side inference fromownerIdor share state.canEdit(GardenCanvas.tsx:115,141) in addition to the UI gates (palette/overlays not mounted for viewers). Inspector/PlopInspector wrap all inputs in<fieldset disabled={readOnly}>and hide Change/Delete/Remove/Plant-here — disabled inputs can't focus, so theonBlurpatch handlers can't fire for a viewer.gardenId/userIdinterpolated into share URLs (shares.ts:33,48,63) are typednumberand zod-validated (z.number()); no path injection. Inviteemailis sent as a JSON body and validated server-side;type="email"plus a non-empty trim check is adequate client-side.displayName/email/myRoleare rendered as React text content oraria-label(auto-escaped), neverdangerouslySetInnerHTML.One item I could not verify from this frontend-only diff (and therefore am not reporting as a finding): the
DELETE /gardens/:id/shares/:userIdendpoint is invoked by a recipient withme.data.id(LeaveGardenModal.tsx:21). This is correct on the client, but the safety of "leave" vs. "revoke another user's share" depends entirely on the server authorizing that:userId === authenticated user(or that the actor is the owner). That is a backend authz concern outside this PR's files; the frontend does its part correctly.🎯 Correctness — Minor issues
Both findings confirmed against the actual code. Adjusting line numbers to the real locations.
Verdict: Minor issues
I verified the data flow and gating logic against the checked-out code. The core
canEdit/isOwnerderivation is sound (role comes from servermyRole, helpers treat unknown as no-access), and thefieldset disabledapproach correctly covers the native inputs in both inspectors. The canvas defense-in-depth checks (canEditinonCanvasPointerDown/onPlaceand the overlay-mount guards) are correctly placed. A few minor robustness gaps remain:web/src/components/gardens/ShareGardenModal.tsx:93— inline role-changeselecthas no error handling.updateRole.mutate({ userId: sh.userId, role })is fire-and-forget on the<select onChange>.useUpdateShareRoledefines only anonSuccess(invalidate) with noonError, and there's no rollback/feedback — on a failed PATCH (403/409) theselectkeeps the chosen value in the DOM until the query refetches, so the user sees a role that wasn't applied. The Add-share path above it (onInvite) does handle errors via try/catch +Alert; this one doesn't, asymmetrically. Suggested fix: addonErrorsurfacing via the sharederror/Alert, or make the select controlled and reconcile on failure.web/src/components/gardens/GardenCard.tsx:30,40,64— a garden withmyRole === undefinedis treated as non-owner and gets aLeavebutton with no badge.isOwnerRole(undefined)is false (confirmed ingardens.ts:37), so the footer falls into the!ownerbranch and rendersLeave; the badge is gated on!owner && garden.myRole(truthy), so a missing role yields a card with no role label and aLeavebutton that callsremove.mutateAsync(me.data.id)— a self-share delete that likely 404s/403s since there's no share row. This is a latent edge (the backend populatesmyRoleand the schema marks it optional only defensively), but the fail-closed direction is wrong for an unknown role:Leaveis a mutating action. Consider gatingLeaveongarden.myRole && !ownerexplicitly.web/src/components/gardens/LeaveGardenModal.tsx:21—me.data.idused as the share userId to delete. Confirmed correct againstuseRemoveShare(shares.ts:60-69, DELETE/gardens/:id/shares/:userId) and the comment that the recipient reuses the owner's revoke endpoint to delete their own row. No change needed; noting the backend dependency.No blocking correctness issues. The
canEditgating is consistent across canvas, overlays, palette, inspectors, and the plant-placement flow;useRemoveSharecorrectly invalidates both the shares and['gardens']queries so the list updates after a leave.🧹 Code cleanliness & maintainability — Minor issues
All findings confirmed against the actual source code. The only correction needed is the line number for the
useSharesfinding (the function is at line 35, not 43).VERDICT: Minor issues
web/src/editor/Inspector.tsx:113-227&PlopInspector.tsx:127-171— fieldset children not re-indented. The new<fieldset>wrappers enclose a large block of pre-existing JSX, but the children inside were left at their original indentation (e.g.<div className="grid grid-cols-2 gap-2">at Inspector:122 sits at the same indent as the<fieldset>that owns it; same in PlopInspector:128). The closing</fieldset>at 227 is likewise dedented past the<TextArea>it contains. This flattens the visual scope, so a reader can't tell at a glance which elements the disabled wrapper governs — exactly the thing the comment above it says is the point of the change. Re-indent the wrapped block one level; consider extracting the shared fieldset+controls body into a smallObjectFieldsfragment if the file keeps growing.web/src/components/gardens/ShareGardenModal.tsx:91-98— inline<select>+ duplicated role options. The per-row role control uses a raw<select>withfieldControlClassglued on, while the invite form above it uses the importedSelectcomponent for the same concept. Worse, the role labels now live in two places: theroleOptionsconstant ("Viewer (read-only)" / "Editor (can edit)") and the inline<option value="viewer">Viewer</option>/<option value="editor">Editor</option>— two sources of truth that will drift. Route the per-row control throughSelect(or at least map overroleOptions) so there's one list.rounded-md bg-border/40 px-2 py-1 text-xs text-muted"View only" string appears three times:Inspector.tsx:102,PlopInspector.tsx:110, andGardenEditorPage.tsx:158(the page one adds an👁andmb-2, so it's not even visually consistent with the inspector ones). A single<ViewOnlyBadge>component would remove the copy-paste and let the wording/styling stay in sync.web/src/components/gardens/ShareGardenModal.tsx:104— remove button class string duplicated again. The✕remove button reuses the exacthover:bg-red-500/10 hover:text-red-600 dark:hover:text-red-400pattern already factored intoGardenCard'sdangerClassandPlantCard's equivalent. This is the third inline copy of that danger-hover string; lift it to a shared constant (e.g. alongside the existingactionClass/dangerClassin a shared module) instead of pasting it once more.web/src/lib/shares.ts:35—enabledparameter is dead.useShares(gardenId, enabled = true)exposes anenabledflag that no caller passes (ShareGardenModal callsuseShares(garden.id); grep confirms that's the only call site). Either wire it up (e.g. passenabled={!!me}-style guards) or drop it until a caller needs it, to avoid a "why is this here" trap for the next reader. Trivial.⚡ Performance — Minor issues
The finding is confirmed.
useRemoveShare(line 66) invalidates['gardens']on every success, and the comment explicitly documents it's shared between owner-revoke and recipient-leave paths.ShareGardenModalcallsremove.mutate(sh.userId)for the owner-revoke case, whileLeaveGardenModalusesmutateAsync(me.data.id)for the leave case — both go through the same hook.Verdict: Minor issues
I reviewed the diff through the performance lens and verified the data-layer and component code. The change is mostly clean performance-wise — no N+1 patterns, no unbounded growth, mutations invalidate narrowly scoped keys, and the shares query only fires when the modal mounts. One minor, avoidable refetch:
web/src/lib/shares.ts:66—useRemoveShare.onSuccessunconditionally invalidates['gardens']on every remove. This is correct and necessary for the leave path (the garden leaves the recipient's list), but it also fires for the owner revoke path inShareGardenModal. When the owner manages shares fromGardensPage(where the gardens query is active), each ✕ click triggers a full gardens-list refetch in addition to the shares refetch — so removing N shares means N extra gardens-list GETs. Suggested fix: split the hook (or pass a flag) so only the leave flow invalidates['gardens']; revoke needs onlysharesKey(gardenId). Verified by readingshares.ts(the hook's own comment documents it's used by both paths) and confirmingShareGardenModalcallsremove.mutate(sh.userId)for the owner-revoke case whileLeaveGardenModalcallsremove.mutateAsync(me.data.id)for the leave case.No other material performance concerns:
shares.data?.maprenders only after load (no render of undefined),useSharesis gated behind modal mounting, inspector mutation hooks are instantiated in read-only mode but issue no network requests, and there's no work inside hot loops introduced here.🧯 Error handling & edge cases — Minor issues
All three findings verified against the actual code:
ShareGardenModal fire-and-forget — confirmed.
updateRole.mutate/remove.mutateat lines 93/102 have noonError;shares.tsmutations define onlyonSuccess+mutationFn, noonError;queryClient.tshas no globalMutationCache/mutations.onError. Contrastobjects.tsusesonError: toast.error. Keep.Missing myRole lockout — confirmed.
canEditRole/isOwnerRolereturn false for undefined;myRoleisoptional(); GardenEditorPage gates everything oncanEdit/isOwner. Keep.useMe non-401 stuck button — confirmed.
auth.ts:31-38resolves tonullonly on 401, throws otherwise →me.dataundefined → buttondisabled={remove.isPending || !me.data}with no error/loading UI. Keep.Verdict: Minor issues found
web/src/components/gardens/ShareGardenModal.tsx:93/:102— inline role-change and remove mutations silently swallow failures.updateRole.mutate(...)(line 93) andremove.mutate(sh.userId)(line 102) are fire-and-forget. Verified inweb/src/lib/shares.ts:useUpdateShareRole(lines 48-55) anduseRemoveShare(lines 60-69) define onlymutationFnandonSuccess, noonError.web/src/lib/queryClient.tshas no globalMutationCache/mutations.onError. So a failed PATCH/DELETE (network drop, 409, 403) produces no UI feedback at all — no Alert, no toast — and the shares list only refetches viaonSuccess. Contrast the invite path (:42) which usesmutateAsync+ try/catch +<Alert>, andlib/objects.tswhere every mutation hasonError: (err) => toast.error(...). Concrete impact: an owner changes a share's role or hits ✕ to revoke, the request fails, and they're left believing it succeeded while the displayed role row still shows the old value. Fix: addonErrorto these two mutations (e.g.toast.error(...), matchinglib/objects.ts) or switch the handlers tomutateAsyncwith try/catch + the existingerrorstate.web/src/pages/GardenEditorPage.tsx:127—canEdit/isOwnerarefalsewhenmyRoleis absent, which can lock an owner out of their own garden with no error surfaced. Verified inweb/src/lib/gardens.ts:canEditRole(undefined)(line 32-34) andisOwnerRole(undefined)(line 37-39) both returnfalse, andmyRoleis declaredgardenRoleSchema.optional()(line 27) "defensively."GardenEditorPage.tsx:127-128gates all UI on these. If the server ever omitsmy_role(backend bug, partial deploy, stale cache), the owner lands in read-only viewer mode on their own garden: no Palette (line 160), no Share button (line 152), disabled inspectors (readOnly={!canEdit}), all canvas mutations gated off (canEditchecks in GardenCanvas). Silent total loss of function with no error state. Worth at least a non-owner fallback or an explicit "couldn't determine your role" error rather than treating unknown as viewer.web/src/components/gardens/LeaveGardenModal.tsx:40— a non-401useMefailure leaves the user stuck with a disabled button and no explanation. Verified inweb/src/lib/auth.ts:29-44:meQueryOptions.queryFnreturnsnullonly onApiError.isUnauthorized(401); any other error is rethrown, leavingme.dataundefined.LeaveGardenModal.tsx:40setsdisabled={remove.isPending || !me.data}with no error/loading state forme, so a non-401useMefailure keeps "Leave" permanently disabled with no message. Minor unhappy path with no feedback; a loading/error state onmewould cover it.kimi-k2.6: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, the PR is clean. Verified:
myRoleis declaredoptional()in the zod schema and comes from the server response;canEditRole/isOwnerRoleboth safely default tofalsewhen undefined. No client-side privilege escalation.Palette, overlays, inspectors) oncanEditderived from server-providedmyRole. Event handlers inGardenCanvasdouble-checkcanEditbefore firing placement mutations — defense in depth acknowledged in code comments.ShareGardenModalonly renders share-management UI; actual authorization lives on the backend (/gardens/:id/shares). No injection vector — email is sent as a JSON string body, and React auto-escapes interpolated user data (displayName,email, garden name) in JSX.LeaveGardenModalpasses the current user’s ownme.data.idtouseRemoveShare; no cross-user revocation from this component.<fieldset disabled>is a UX convenience; the backend remains the authority, and the PR explicitly does not rely on client-side gates alone.No authz gaps, injection vectors, secret leakage, or unsafe deserialization found.
🎯 Correctness — No material issues found
No material issues found
The role-gating logic is semantically correct throughout:
canEditRoleandisOwnerRoleproperly derive rights from the server'smy_role; the canvas, inspector, palette, and share flows are all gated with the right predicates; and the defense-in-depth checks inGardenCanvas(armed && canEdit,!canEdit || ...) ensure stale store state can't bypass viewer restrictions. The read-only inspector fieldset correctly disables all nested controls in one shot, and the mutation hooks target the expected endpoints with the expected payloads. Nothing in this diff introduces a logic error or incorrect domain behavior.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/components/gardens/GardenCard.tsx:6-16—actionClassanddangerClassare copy-pasted verbatim fromPlantCard.tsx. The exact same 150+ character strings appear in both files. Extract them to a shared location (e.g.components/ui/actionButton.ts) so future style tweaks don't diverge.web/src/components/gardens/ShareGardenModal.tsx:100-107— The remove-share button uses a raw Tailwind string instead of the establisheddangerClasspattern used inGardenCard/PlantCard. This is inconsistent and will drift if the danger style changes.web/src/editor/Inspector.tsx:113-227— The<fieldset disabled={readOnly}>wrapper was added, but its children (lines 122-227) are not indented inside it; they sit at the same column as the<fieldset>tag. This makes the DOM nesting misleading and harder to maintain.web/src/editor/PlopInspector.tsx:127-171— Same indentation issue: the<fieldset>wrapper added in the diff has children at the same column level instead of being indented.⚡ Performance — No material issues found
No material issues found
Through the Performance lens, the PR is clean. The read-only viewer mode actually reduces work for the majority use-case (viewers) by conditionally omitting the palette, placement overlays, selection handles, and edit-bound mutation hooks. The
canEditgating is a primitive boolean derived once at the page level, so it doesn't trigger spurious re-renders. Memoization ofobjects,plantings, andplantsByIdinGardenEditorPageis preserved. No N+1 patterns, unbounded hot-path loops, or missing pagination on lists that would be material at this scale were introduced.🧯 Error handling & edge cases — Minor issues
Based on my verification of the actual source code, all four findings are confirmed. Here is the corrected review:
Minor issues found
web/src/components/gardens/ShareGardenModal.tsx:93—updateRole.mutateandremove.mutateare fire-and-forget with no error handling. If the backend returns a 403, 409, or network failure, the user gets zero feedback; the role dropdown snaps back (or the removed user re-appears on next refetch) with no explanation.Fix: Wire both mutations into a local error state (or a toast) and show an
<Alert>when they fail, mirroring theaddmutation pattern.web/src/components/gardens/ShareGardenModal.tsx:50— The Modal’sbusyprop only tracksadd.isPending, so Escape / backdrop-click can dismiss the dialog while an in-flightupdateRoleorremoveis still pending. That buries any eventual error and leaves the user unsure whether the action completed.Fix: Pass
busy={add.isPending || updateRole.isPending || remove.isPending}.web/src/editor/Inspector.tsx:119andweb/src/editor/PlopInspector.tsx:138— EveryonBlurhandler inside the<fieldset disabled={readOnly}>callspatch(...)without checkingreadOnly. If the user’s role is downgraded mid-edit (owner changes them to viewer, or a concurrent refetch reveals a newmyRole), React re-renders withreadOnly=true, the fieldset disables, focused inputs blur, and the blur handlers fire unauthorized mutations that will 403.Fix: Guard the
patchhelpers (or eachonBlur) with an earlyif (readOnly) return;.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Examining the change through the security lens: I verified the backend authorization (service/shares.go, store/shares.go), the API layer (api/shares.go uses
mustActor(c).IDfrom session, parameterized queries throughout), the data layer (lib/shares.ts uses zod parsing of server responses), and the client-side gating (lib/gardens.ts derivescanEdit/isOwnersolely from server-providedmyRole).VERDICT: No material issues found
requireGardenRole(ctx, actorID, gardenID, roleOwner)for list/add/update; owner-or-self rule for remove ininternal/service/shares.go:19,30,53,68). The client's role badges and Share button are derived from the server'smyRole, not guessed.ShareGardenModal.tsx) is sent as JSON to a parameterized lookup (store/shares.goGetUserByEmail-equivalent viaGetUserByEmailin service), no injection surface; responsedisplayName/emailare rendered as React text (auto-escaped), nodangerouslySetInnerHTML.<fieldset>(Inspector, PlopInspector) which preventsonBlur/onChangemutations from firing, and action buttons are conditionally hidden; plus canvas-levelcanEditre-checks inGardenCanvas.tsxare genuine defense in depth since they'd 403 server-side anyway.useSharesis only mounted fromShareGardenModal, which is only reachable via owner-gated UI; backend also 403s non-owners, so no authz gap if a stale client opened it.LeaveGardenModalremoves the actor's own share usingme.data.idfromuseMe()(the authenticated session), not from URL/props — a recipient cannot supply another user's id through this path; the backendRemoveShareadditionally enforcesactorID == targetUserIDfor non-owners.🎯 Correctness — Minor issues
Both mutations confirmed fire-and-forget with no
onError. For the third finding, react-query preservesdataduring background refetch (onlyisPendingon initial load with no data), so the "renders empty during refetch" claim is incorrect — dropping it.VERDICT: Minor issues
Findings:
web/src/components/gardens/ShareGardenModal.tsx:93—updateRole.mutate(...)fires with no error surface. The hook inlib/shares.ts:48-55defines onlyonSuccess(invalidation), noonError, and the call site uses fire-and-forgetmutate(no try/catch). If the PATCH fails (network error, 403 after a role-change race, version conflict), the user gets no feedback: the<select>flips to the new value, then either silently reverts on refetch or silently stays wrong if the request fails before invalidation. The add-share path correctly usesmutateAsync+ try/catch, but the inline role change does not. Suggested fix: addonErrorto the hook (e.g.toast.error) or wrap the call in try/catch with surfaced local error state.web/src/components/gardens/ShareGardenModal.tsx:102—remove.mutate(sh.userId)is fire-and-forget with no error surface.useRemoveShare(lib/shares.ts:60-69) defines onlyonSuccess, noonError, and the call site has no try/catch. If the DELETE fails, the ✕ button appears to do nothing and the user gets no feedback. Same fix applies.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
Duplicated
actionClass/dangerClassconstants —web/src/components/gardens/GardenCard.tsx:5-10defines the exact same two class strings asweb/src/components/plants/PlantCard.tsx:5-10(verified identical character-for-character). This is copy-paste that should be shared — e.g. extract toweb/src/components/ui/button.tsor acardActionshelper. As more card components appear, this drift will diverge.Inconsistent
<select>usage withinShareGardenModal—web/src/components/gardens/ShareGardenModal.tsx:91-99uses a raw<select>with manualfieldControlClass+cnfor the inline role changer, while the same file (line 62) uses theSelectcomponent for the invite form's role picker. Within one file, picking both patterns is confusing; either useSelectfor both or extract a small inline-role select. The raw form also bypasses theuseFieldId/label wiring the shared component provides.<fieldset>wrapping has misleading indentation — Inweb/src/editor/Inspector.tsx:113-227andweb/src/editor/PlopInspector.tsx:127-171, the<fieldset>opens at one indent level but its children (the grid<div>, the rotation/label TextFields, the color row, the Plantable checkbox, the Notes TextArea) keep their pre-existing indentation rather than being re-indented one level. The result reads as if those controls are siblings of the fieldset, not children — a structural mismatch that will trip the next reader doing a git-blame or refactor. A one-time re-indent of the wrapped block (or a prettier format pass) would fix it cleanly.Raw enum value surfaced in UI badge —
GardenCard.tsx:42rendersshared · {garden.myRole}, which prints the lowercase wire value (viewer/editor). Elsewhere in the PR the same roles are presented asViewer (read-only)/Editor (can edit)(ShareGardenModal.tsx:13-16). Minor, but a sharedroleLabel(role)helper would keep the casing/wording consistent and centralize the display string.⚡ Performance — No material issues found
VERDICT: No material issues found
web/src/lib/shares.ts) and the share/leave modals for hot-path waste, unnecessary refetches, and unbounded work. The mutations invalidate only their ownsharesKey(gardenId)plus (foruseRemoveShare) the['gardens']list key — no broad broadcast, no N+1.useShares(garden.id)inShareGardenModalonly fires when the modal is mounted (conditional render inGardensPage.tsx:75andGardenEditorPage.tsx:227), so the owner-only shares endpoint isn't polled on every garden view.useRemoveShareinvalidates['gardens']on every removal, which causes one extra list refetch even when the owner revokes a share (their garden list doesn't change). This is a single additional request gated behind a manual click, not a hot path — not material.<select>and remove✕inShareGardenModal.tsx:91-107fireupdateRole.mutate/remove.mutatedirectly with no debounce/disabled-during-pending, but each triggers a single targeted invalidation — no cascading refetches, no quadratic behavior.canEditis a cheap boolean gate aroundsetArmedKind/overlay mounting. TheplantsByIdMap rebuild is pre-existing and unchanged.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Verified by reading
ShareGardenModal.tsx,shares.ts,Modal.tsx,gardens.ts, andGardenEditorPage.tsx.ShareGardenModal.tsx:93and:102— role change and share removal silently swallow errors. BothupdateRole.mutate({ userId, role })andremove.mutate(sh.userId)are fire-and-forget. The hooks inshares.ts:39-69only defineonSuccess(noonError), the call sites attach noonError, and.mutate(unlike.mutateAsync) does not throw — react-query stashes the error inmutation.errorbut surfaces nothing. So a 403/409/network failure on a role change or a revoke leaves the user with zero feedback: the<select>visually flips, then silently reverts on the next refetch (or doesn't, if the refetch also fails). CompareonInviteat:41-46, which wrapsadd.mutateAsyncin try/catch and shows anAlert. Fix: either switch tomutateAsyncwith try/catch +setError, or pass{ onError: (err) => setError(errorMessage(err, …)) }to each call (and/or anonErroron the hooks). The role-change<select>should also be disabled while its own mutation is pending to avoid stacking a second PATCH on an in-flight one.ShareGardenModal.tsx:50—busyonly reflectsadd.isPending, so update/remove mutations don't lock the modal. ConfirmedModal.tsx:32,42suppresses Escape/backdrop-close only whenbusyis true. Withbusy={add.isPending}, a role change or revoke in flight leavesbusy=false, so Escape/backdrop click can dismiss the dialog mid-request. Fix: derivebusy={add.isPending || updateRole.isPending || remove.isPending}.ShareGardenModal.tsx:100-107— destructive revoke (✕) has no confirmation and no per-row busy state. UnlikeLeaveGardenModal(which confirms via a modal) and the object delete flow inInspector.tsx(two-step "Confirm delete"), the owner's share removal at:100-107is one click with no undo, no busy indicator on the row, and (per the first finding) no error feedback. A mis-tap revokes someone's access with no recourse. Suggest a confirm step or at minimum a disabled state keyed to that row's pending removal.GardenEditorPage.tsx:127-128— safe-default on missingmyRole, but misleading "View only" indicator.canEditRole/isOwnerRole(gardens.ts:32-39) return false whenmyRoleis undefined — the right defensive default. But!canEditat:157-159then renders the "👁 View only" banner even when the role is simply missing rather than genuinelyviewer. The schema marksmyRoleoptional (gardens.ts:25-28), so this path is reachable in principle. Not blocking — the comment saysmyRoleis present on every accessible response — but worth a guard so an owner with a transiently-missing role isn't told they're view-only.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
Blocking issues found
web/src/pages/GardenEditorPage.tsx:135-143—onPickPlantfiresupdatePlanting.mutatefor a "change" operation without checkingcanEdit. Every other mutation path in the editor (create object, create planting, move/resize viaSelectionOverlay, delete object, remove planting) is either gated by acanEditcheck or hidden behind acanEdit-conditional mount. ThePlantPickermodal is the only remaining un-gated mutation path: a viewer who manipulates React state (e.g., via DevTools or by forcing the modal open) can change a plop's plant. The PR explicitly calls this out as a defense-in-depth concern for canvas handlers, but the picker callback was missed. Add an earlyif (!canEdit) returnguard inonPickPlant.🎯 Correctness — No material issues found
No material issues found.
I reviewed the PR thoroughly through the correctness lens, focusing on semantic/domain correctness, role gating logic, API contract consistency, and state/cache behavior.
Key areas verified:
canEditRoleandisOwnerRolecorrectly mapowner/editor/viewerto edit/owner permissions. Theundefinedfallback returnsfalse— a safe defensive choice.canEditgates object creation (onCanvasPointerDown), plop placement (onPlace), and overlay mounting (SelectionOverlay,PlopOverlay, placement capture). Viewers can still select objects for read-only inspection, which is intentional.<fieldset disabled={readOnly}>correctly disables all nested inputs in one shot. Mutation buttons (Delete, Remove, Change plant, Plant here) are conditionally hidden.useRemoveShare(garden.id)withme.data.idcorrectly removes the current user's share.ShareGardenModaluses the proper API paths and zod schemas.useRemoveShareinvalidates bothsharesKeyand['gardens'](needed for the leave flow).useAddShareanduseUpdateShareRoleonly invalidatesharesKey, which is correct for owner-local share-list UI. Cross-user list staleness is an inherent client-side caching limitation, not a correctness bug in this code.Gardenschema includesmyRole: gardenRoleSchema.optional(), andShareRoleis correctly constrained to'viewer' | 'editor'(no owner sharing).The logic is sound and the defensive gating is consistent throughout.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/Inspector.tsx:113— The<fieldset>wrapper added for read-only mode contains children at wildly inconsistent indentation (e.g. the grid div at line 122 sits at the same level as the fieldset tag, while the Name field at line 114 and Notes field at line 219 are indented +2). This makes the JSX hierarchy unreadable and invites future edits that break the intended nesting. Re-indent everything inside the fieldset uniformly.web/src/editor/PlopInspector.tsx:127— Same indentation problem: the<fieldset>tag and its first child<div>(line 128) are at the same indentation level, while the inner fields vary between +0 and +2. Uniformly indent all children.web/src/components/gardens/ShareGardenModal.tsx:91— Uses a raw<select>element (with manually importedfieldControlClassandcn) for the inline role changer, while the invite form 20 lines above uses the project's<Select>component. This creates two styling paths for the same control and duplicates theviewer/editoroptions already defined in the module-levelroleOptionsarray. Prefer<Select>(or maproleOptions) to keep the UI consistent and DRY.web/src/components/gardens/ShareGardenModal.tsx:84— The<ul className="flex flex-col gap-2">is rendered unconditionally, so it sits empty in the DOM during loading and error states alongside the "Loading…" / error messages. Move it inside theshares.isSuccess && shares.data.length > 0branch.web/src/components/gardens/GardenCard.tsx:5—actionClassanddangerClassrepeat the same long Tailwind base string; extract a sharedbaseActionClass(or usecn) to remove the copy-paste.⚡ Performance — No material issues found
No material issues found
I examined the PR through the Performance lens and found no efficiency regressions. The changes add role-based conditional rendering gates (
canEdit,isOwner) which are cheap boolean checks derived from stable server data. For viewers, the PR actually reduces work by not mounting interactive overlays (SelectionOverlay,PlopOverlay) and skipping placement capture — a perf win for the read-only path.Specific areas checked:
lib/shares.tsis targeted and correct; no over-invalidation or missing cache updates that would cause extra network churn.ShareGardenModal.tsx) renders without virtualization, but garden sharing lists are inherently small — no material concern.GardenEditorPage.tsxandGardenCanvas.tsx(.find()forselectedObject,focusedObject, etc.) were pre-existing and untouched by this PR.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/components/gardens/ShareGardenModal.tsx:91-102—updateRole.mutateandremove.mutateare fire-and-forget with no error handling, pending state, or disabled UI. If the server rejects a role change or removal (network error, 403, 404, version conflict), the user sees zero feedback; the list item stays in place as if nothing happened. Worse, the native<select>and remove button remain interactive while the mutation is in flight, so the user can trigger duplicate or conflicting requests.Fix: Wrap both in
try/catchusingmutateAsync, surface errors in a dedicated list error state, and disable each row’s controls whileupdateRole.isPending || remove.isPending. Also widen the Modal’sbusyprop to cover all three pending states, or at least disable the Done button while any mutation is active.web/src/editor/Inspector.tsx:69-70,76-83,119,131,141,151,161,173-177,191-201,225— Thepatchhelper and dimension commit helpers are called from everyonBlur. IfreadOnlyflips fromfalse→truewhile an input has focus (e.g., the user’s role is downgraded by a background refetch), the disabled<fieldset>causes the focused input to blur, firing itsonBlurhandler and sending an unintended PATCH that the server will 403.Fix: Guard
patch(and the inline blur commits) withif (readOnly) return;so a disabled fieldset never fires mutations.web/src/editor/PlopInspector.tsx:61-62,67-75,77-88,90-93,138,150,160,169,180— Same issue as Inspector:patch,commitRadius,commitCount, andcommitPlanteddon’t checkreadOnly. A role downgrade while an input is focused will blur the now-disabled fieldset and fire mutations.Fix: Add
if (readOnly) return;topatchand each commit helper.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.