Closes#107. Part of #96. The three tidy-ups from the audit's deferred list.
ConfirmModal primitive — a message + Cancel/Confirm that owns the shared busy lock and inline error. Folded the five hand-rolled confirm dialogs onto it: DeleteGarden, LeaveGarden, DeletePlant, DeleteSeedLot, ClearBed. Each is now just its message + mutation + labels. Bonus: ClearBed now surfaces a failure inline instead of swallowing it. (CopyGarden stays on Modal — it has a name field, so it's not a plain confirm.)
Removed components/PageStub.tsx — dead scaffolding, imported nowhere.
Full-width editor — the garden editor was squeezed into the max-w-5xl reading measure the other pages use. The canvas routes (editor + public garden) now go edge-to-edge on desktop, and the top bar matches so the brand aligns with the editor's left edge. Mobile was already full-width, so it's unchanged.
Verified live
The Delete-garden confirm renders and cancels (the refactor is behavior-preserving).
The desktop editor now uses the full viewport width (screenshot in review — the canvas spans 1400px instead of being boxed at ~1024px).
Closes #107. Part of #96. The three tidy-ups from the audit's deferred list.
- **`ConfirmModal` primitive** — a message + Cancel/Confirm that owns the shared busy lock and inline error. Folded the five hand-rolled confirm dialogs onto it: `DeleteGarden`, `LeaveGarden`, `DeletePlant`, `DeleteSeedLot`, `ClearBed`. Each is now just its message + mutation + labels. Bonus: `ClearBed` now surfaces a failure inline instead of swallowing it. (`CopyGarden` stays on `Modal` — it has a name field, so it's not a plain confirm.)
- **Removed `components/PageStub.tsx`** — dead scaffolding, imported nowhere.
- **Full-width editor** — the garden editor was squeezed into the `max-w-5xl` reading measure the other pages use. The canvas routes (editor + public garden) now go edge-to-edge on desktop, and the top bar matches so the brand aligns with the editor's left edge. Mobile was already full-width, so it's unchanged.
## Verified live
- The Delete-garden confirm renders and cancels (the refactor is behavior-preserving).
- The desktop editor now uses the full viewport width (screenshot in review — the canvas spans 1400px instead of being boxed at ~1024px).
`tsc` + `vitest` + build green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Three tidy-ups from the audit's deferred list:
- Extracted a ConfirmModal primitive (message + Cancel/Confirm, owning the
busy lock + inline error) and folded the five hand-rolled confirm dialogs
onto it: DeleteGarden, LeaveGarden, DeletePlant, DeleteSeedLot, ClearBed.
Each is now just its message + mutation + labels. Bonus: ClearBed now
shows a failure inline instead of swallowing it. (CopyGarden stays on
Modal — it has a name field, not a plain confirm.)
- Removed components/PageStub.tsx — dead scaffolding, imported nowhere.
- The garden editor was squeezed into the max-w-5xl reading measure the
other pages use; the canvas routes (editor + public garden) now go
edge-to-edge on desktop, and the top bar matches so the brand aligns with
the editor's left edge. Mobile was already full-width, so it's unchanged.
Verified live: the Delete-garden confirm renders/cancels; the desktop editor
now uses the full viewport width. tsc + vitest + build green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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-22 14:08:42Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **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** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Gadfly on #107:
- ClearBed reported a failure twice — ConfirmModal's inline Alert AND
useClearObject's own onError toast. Dropped the toast from useClearObject
(its only caller is that modal now), so the failure shows once, inline in
the dialog where the action is.
- LeaveGarden's onConfirm silently resolved (closing the dialog as if it
worked) if me.data was missing, relying on confirmDisabled to prevent it.
Throw instead, so a drift in that guard surfaces an error rather than a
fake success.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 06b887f58e into main2026-07-22 14:05:20 +00:00
steve
deleted branch feat/cleanup2026-07-22 14:05:21 +00:00
🪰Gadfly consensus review — 6 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** — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Dialog closes on no-op when me.data is absent instead of staying open
error-handling · flagged by 3 models
LeaveGardenModal closes on no-op when me.data is absent.web/src/components/gardens/LeaveGardenModal.tsx:18
🪰 Gadfly · advisory
🟡 **Dialog closes on no-op when me.data is absent instead of staying open**
_error-handling · flagged by 3 models_
- **`LeaveGardenModal` closes on no-op when `me.data` is absent.** `web/src/components/gardens/LeaveGardenModal.tsx:18`
<sub>🪰 Gadfly · advisory</sub>
⚪Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file
maintainability · flagged by 1 model
web/src/components/layout/AppShell.tsx:54 vs :98 — the nav className uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while <main> (44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identical canvasRoute condition. cn() strips falsy values, so the ternary's '' branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…
🪰 Gadfly · advisory
⚪ **Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file**
_maintainability · flagged by 1 model_
- `web/src/components/layout/AppShell.tsx:54` vs `:98` — the `nav` className uses a ternary-to-empty-string (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) while `<main>` (44 lines below) uses a short-circuit (`!canvasRoute && 'mx-auto max-w-5xl'`) for the identical `canvasRoute` condition. `cn()` strips falsy values, so the ternary's `''` branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…
<sub>🪰 Gadfly · advisory</sub>
⚪confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive
maintainability · flagged by 1 model
web/src/components/layout/AppShell.tsx:54 vs :98 — the nav className uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while <main> (44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identical canvasRoute condition. cn() strips falsy values, so the ternary's '' branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…
🪰 Gadfly · advisory
⚪ **confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive**
_maintainability · flagged by 1 model_
- `web/src/components/layout/AppShell.tsx:54` vs `:98` — the `nav` className uses a ternary-to-empty-string (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) while `<main>` (44 lines below) uses a short-circuit (`!canvasRoute && 'mx-auto max-w-5xl'`) for the identical `canvasRoute` condition. `cn()` strips falsy values, so the ternary's `''` branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…
<sub>🪰 Gadfly · advisory</sub>
web/src/editor/ClearBedModal.tsx:21 — The refactor switches ClearBedModal from clear.mutate() with an onSuccess callback to clear.mutateAsync() inside ConfirmModal. The underlying useClearObject hook (web/src/lib/objects.ts:384) still defines onError: (err) => toast.error(...). TanStack Query runs onError before mutateAsync rejects, so a failure now produces two notifications: the inline error from ConfirmModaland a toast from the mutation hook. Previously th…
🪰 Gadfly · advisory
🟠 **Duplicate error notification: ClearBedModal inline error + useClearObject toast on mutation failure**
_correctness · flagged by 2 models_
- **`web/src/editor/ClearBedModal.tsx:21`** — The refactor switches `ClearBedModal` from `clear.mutate()` with an `onSuccess` callback to `clear.mutateAsync()` inside `ConfirmModal`. The underlying `useClearObject` hook (`web/src/lib/objects.ts:384`) still defines `onError: (err) => toast.error(...)`. TanStack Query runs `onError` before `mutateAsync` rejects, so a failure now produces **two notifications**: the inline error from `ConfirmModal` *and* a toast from the mutation hook. Previously th…
<sub>🪰 Gadfly · advisory</sub>
🟡ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError)
correctness, error-handling · flagged by 3 models
Double error reporting in ClearBedModal — web/src/editor/ClearBedModal.tsx:26 (via web/src/components/ui/ConfirmModal.tsx). useClearObject still carries onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')) (web/src/lib/objects.ts:384), so on a clear failure the user now sees both the inline Alert that ConfirmModal renders and the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a glo…
🪰 Gadfly · advisory
🟡 **ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError)**
_correctness, error-handling · flagged by 3 models_
- **Double error reporting in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:26` (via `web/src/components/ui/ConfirmModal.tsx`). `useClearObject` still carries `onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.'))` (`web/src/lib/objects.ts:384`), so on a clear failure the user now sees both the inline `Alert` that `ConfirmModal` renders *and* the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a glo…
<sub>🪰 Gadfly · advisory</sub>
🟡ClearBed failure now surfaces both a toast and ConfirmModal's inline alert
error-handling · flagged by 1 model
web/src/lib/objects.ts:384 / web/src/editor/ClearBedModal.tsx — useClearObject still has its own onError: (err) => toast.error(...). Since ClearBedModal now uses ConfirmModal, whose handleConfirm also catches the same rejection from clear.mutateAsync(objectId) and renders it inline (ConfirmModal.tsx:52-54), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (useDeleteGarden, `useDele…
🪰 Gadfly · advisory
🟡 **ClearBed failure now surfaces both a toast and ConfirmModal's inline alert**
_error-handling · flagged by 1 model_
- **`web/src/lib/objects.ts:384` / `web/src/editor/ClearBedModal.tsx`** — `useClearObject` still has its own `onError: (err) => toast.error(...)`. Since `ClearBedModal` now uses `ConfirmModal`, whose `handleConfirm` also catches the same rejection from `clear.mutateAsync(objectId)` and renders it inline (`ConfirmModal.tsx:52-54`), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (`useDeleteGarden`, `useDele…
<sub>🪰 Gadfly · advisory</sub>
This PR is a frontend-only UI refactor: extracting a shared ConfirmModal primitive, deleting dead scaffolding (PageStub), and widening the editor layout. I verified the new ConfirmModal.tsx, Alert.tsx, and Modal.tsx — no dangerouslySetInnerHTML anywhere in the touched files (confirmed via repo-wide grep), all error/message content is rendered as plain React children (auto-escaped), and errorMessage() only surfaces ApiError.message or a static fallback string, unchanged behavior from before the refactor. No authn/authz logic, no new API calls, no injection-relevant input handling, and no secret handling in any of the changed files. The layout/width changes in AppShell.tsx are purely CSS class changes with no security surface.
Nothing in this diff falls within the security lens.
🎯 Correctness — Minor issues
This confirms the AppShell change is a straightforward semantics-preserving refactor (logic unchanged, just renamed/reused). No issue there, matching the draft's conclusion.
The single finding in the draft is fully verified against the actual code.
Corrected Review
VERDICT: Minor issues
web/src/editor/ClearBedModal.tsx:20-24 (via web/src/lib/objects.ts:374-386, web/src/components/ui/ConfirmModal.tsx:46-56): ClearBedModal now routes its mutation through ConfirmModal, which calls clear.mutateAsync(objectId) and catches failures to render an inline Alert. But useClearObject (lib/objects.ts:384) already has its own onError: (err) => toast.error(...) on the mutation. TanStack Query fires a mutation's onError option independently of whether the caller awaits mutateAsync and catches the rejection, so both fire. A failed "Clear bed" now shows the error twice: once as a persistent toast (error toasts don't auto-dismiss, per components/ui/toast.tsx:44-49) and once as the modal's inline Alert. Confirmed none of the other four hooks routed through ConfirmModal (useDeleteGarden, useRemoveShare, useDeletePlant, useDeleteSeedLot) has a hook-level onError, so this duplication is specific to ClearBedModal. Fix: drop the onError toast from useClearObject (let the modal own the error, consistent with the other four hooks).
🧹 Code cleanliness & maintainability — Minor issues
Confirmed both findings against the actual code — line 54 is the ternary (canvasRoute ? '' : 'mx-auto max-w-5xl'), line 98 is the && short-circuit for the identical condition; confirmVariant is only ever assigned its default, with no caller passing 'primary' anywhere in web/src.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:54 vs :98 — the nav className uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while <main> (44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identical canvasRoute condition. cn() strips falsy values, so the ternary's '' branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise cleaning up. Suggest aligning the nav line to !canvasRoute && 'mx-auto max-w-5xl' to match <main>.
web/src/components/ui/ConfirmModal.tsx:33 (type declared at line 35) — confirmVariant?: 'danger' | 'primary' is exposed on the new primitive, but no caller in web/src passes anything but the default ('danger'). Speculative API surface on a brand-new component, contrary to the project's "don't design for hypothetical future requirements" convention — low priority, fine to leave if the next confirm-style dialog is expected to need it soon.
Both are cosmetic; nothing here blocks merge. The five ConfirmModal conversions are faithful, PageStub removal is clean, and the full-width editor changes correctly centralize the canvasRoute condition.
⚡ Performance — No material issues found
VERDICT: No material issues found
This PR is a frontend-only refactor (dead code removal, layout class changes, and consolidating five duplicate confirm-dialog components into a shared ConfirmModal). I read ConfirmModal.tsx, AppShell.tsx, and Modal.tsx in full, plus all five modal call sites in the diff.
Through the performance lens specifically:
No new loops, no data-fetching changes, no added re-render triggers of consequence. sections.filter(...) in AppShell was already there and runs once per render over a 3-item array — non-issue.
Modal's focus-trap effect (mount-only, [] deps) is unchanged by this PR.
The seed-lot modal switch from del.mutate(..., { onSuccess, onError }) to del.mutateAsync() inside ConfirmModal's onConfirm is functionally equivalent — no extra network calls or renders introduced.
Removing PageStub.tsx is pure deletion of unused code, no runtime effect.
The max-w-5xl → full-width class changes are static Tailwind class strings recomputed via cn(...) per render, same cost as before (it was already a template string with conditionals).
Nothing here touches hot loops, allocation-heavy paths, network calls, or unbounded growth. Nothing to report in this lens.
🧯 Error handling & edge cases — Minor issues
Both findings verified directly against the source.
Finding 1 — objects.ts:384 confirms useClearObject still has onError: (err) => toast.error(...), while gardens.ts:136-138, plants.ts:112-115, seedLots.ts:98-104, and shares.ts:63-69 confirm none of the other four converted hooks (useDeleteGarden, useDeletePlant, useDeleteSeedLot, useRemoveShare) define an onError. ConfirmModal.tsx:46-55 confirms handleConfirm awaits onConfirm() in a try/catch and sets its own inline Alert on rejection — so for ClearBedModal specifically, a failure fires both the hook-level toast and the modal's inline alert. package.json:19 confirms @tanstack/react-query: ^5.62.0, where mutateAsync rejecting alongside a configured onError is standard behavior. Confirmed as described.
Finding 2 — LeaveGardenModal.tsx:18-20 confirms onConfirm={async () => { if (me.data) await remove.mutateAsync(me.data.id) }} — when me.data is falsy this resolves without throwing. ConfirmModal.tsx:46-51 confirms handleConfirm treats any non-throwing resolution as success and calls onClose(). Confirmed as described; also confirmed gated by confirmDisabled={!me.data} at line 16, matching the draft's "low practical severity" framing.
VERDICT: Minor issues
web/src/lib/objects.ts:384 / web/src/editor/ClearBedModal.tsx — useClearObject still has its own onError: (err) => toast.error(...). Since ClearBedModal now uses ConfirmModal, whose handleConfirm also catches the same rejection from clear.mutateAsync(objectId) and renders it inline (ConfirmModal.tsx:52-54), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (useDeleteGarden, useDeletePlant, useDeleteSeedLot, useRemoveShare) define an onError, so only this one double-surfaces. Fix: drop the onError toast from useClearObject now that the modal shows the error inline.
web/src/components/gardens/LeaveGardenModal.tsx:18-20 — onConfirm silently no-ops when me.data is falsy instead of throwing, so ConfirmModal.handleConfirm treats the no-op as success and calls onClose() as though the leave succeeded. Gated by confirmDisabled={!me.data} (line 16) so unreachable via a normal click today, but it's a latent trap for this pattern if reused elsewhere. Low severity; worth tightening (throw or otherwise signal failure instead of resolving silently).
I traced the security-relevant data flow through this refactor and found nothing materially wrong in my lane:
Error message rendering (XSS): ConfirmModal surfaces errors via errorMessage(err, errorFallback) → <Alert>{error}</Alert>. Alert (web/src/components/ui/Alert.tsx:7-19) renders children as React text nodes (no dangerouslySetInnerHTML), so the message is HTML-escaped. errorMessage (web/src/lib/api.ts:135-138) returns either the server-controlled ApiError.message or a static per-caller errorFallback literal — no untrusted client input is interpolated into the DOM. Verified by reading both files.
No new tainted input paths: The refactor only consolidates existing confirm dialogs. children, title, confirmLabel, etc. are all caller-supplied static JSX/strings (garden/plant names were already rendered the same way before). No new user-controlled string is introduced into a sink.
No credential/secret leakage: errorFallback values are static literals; nothing from ApiError.body (which can hold a server row on 409) is rendered — only err.message. No secret exposure.
Authn guard preserved: LeaveGardenModal keeps the confirmDisabled={!me.data} guard (web/src/components/gardens/LeaveGardenModal.tsx), so the action button stays disabled when there's no authenticated user, matching the prior disabled={remove.isPending || !me.data}.
ClearBed error surfacing is an improvement: ClearBed previously swallowed failures (mutate with only onSuccess); now it uses mutateAsync and shows the error inline. Better failure visibility, no security regression.
The ConfirmModal busy/disabled state is React-state-based (like the pre-existing isPending pattern it replaces), so it's not a new double-submit vector — and that's a correctness/UX concern outside this lens anyway.
🎯 Correctness — Minor issues
Verdict: Minor issues
ClearBedModal now double-reports failures (inline Alert + toast).web/src/editor/ClearBedModal.tsx:27 calls clear.mutateAsync(objectId). The useClearObject mutation (web/src/lib/objects.ts:384) has an onError that fires toast.error(...). mutateAsync rejects on error, so ConfirmModal's catch ALSO sets the inline Alert (web/src/components/ui/ConfirmModal.tsx:52-54). The original code used clear.mutate(objectId, { onSuccess: onClose }) — mutate does not reject, and the old modal had no inline error state at all, so only the toast fired. The net effect of this PR is that a failed clear now shows both an inline Alert and the existing toast. Verified against the real source: useClearObject keeps its onError toast, while the other four mutations (useDeleteGarden, useDeletePlant, useDeleteSeedLot, useRemoveShare) have no onError (grep confirmed no matches in gardens.ts, plants.ts, seedLots.ts, shares.ts), so this duplication is specific to the clear flow. Consistent fix: drop the onError toast from useClearObject so the dialog owns the inline message, matching the other four confirm flows.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed the new ConfirmModal primitive, the five folded dialogs, the PageStub deletion, and the AppShell width change. Verified:
PageStub has zero remaining imports (grep across web/src → no matches) — clean removal.
All five callers now use ConfirmModal consistently; the duplicated busy-lock/try-catch/footer boilerplate is genuinely collapsed into one place (ConfirmModal.tsx:46-56).
ConfirmModal's confirmVariant is restricted to 'danger' | 'primary', which matches Button's actual ButtonVariant (Button.tsx:4) — no leaky/invalid union.
AppShell.tsx:44,54,98 — canvasRoute is named and reused for both the nav and main width, replacing the prior inEditor && inPublicGarden-style spread; the cn(..., canvasRoute ? '' : 'mx-auto max-w-5xl') pattern matches the surrounding conditional-class style. Brand and editor both key off px-4, so alignment holds.
One trivial note (not worth blocking): ConfirmModal's confirmVariant prop (ConfirmModal.tsx:23,35) is declared and documented but none of the five callers pass it — they all rely on the 'danger' default. It's a reasonable, cheap extension point and self-documenting, so I'd leave it; flagging only for completeness as speculative API surface.
⚡ Performance — No material issues found
No material issues found.
Reviewed through the performance lens:
ConfirmModal (web/src/components/ui/ConfirmModal.tsx): The busy/error state moved from each caller's react-query isPending into local useState. This is render-equivalent — one useState pair instead of reading mutation.isPending, no extra subscriptions or re-renders. handleConfirm awaits onConfirm() and only resets busy on throw; no leaked in-flight work or unbounded growth. Verified at ConfirmModal.tsx:43-56.
AppShell (web/src/components/layout/AppShell.tsx): The two matchRoute calls (inEditor, inPublicGarden) already existed pre-PR; canvasRoute is just a boolean OR of them, so no new route matching work per render. The nav/main now go through cn() (twMerge+clsx) instead of a static string — a couple of small allocations on AppShell renders, but AppShell is not a hot path and this is immaterial. Verified at AppShell.tsx:40-54,93-102 and cn.ts:1-7.
Modal removals / PageStub deletion: pure dead-code/layout cleanup; nothing on a hot path, no allocation or query behavior change.
No N+1 queries, no missing pagination/limits, no work moved into loops, no blocking calls introduced.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/ClearBedModal.tsx:27 — double error reporting on failure (toast + inline alert).useClearObject (web/src/lib/objects.ts:384) defines a mutation-level onError that fires toast.error(objectErrorMessage(...)). The PR switches from clear.mutate(objectId, { onSuccess: onClose }) (which on failure only ran the global toast) to clear.mutateAsync(objectId), but mutateAsync does not suppress the mutation's global onError — React Query still invokes it. mutateAsyncalso rejects, so ConfirmModal.handleConfirm's catch sets the inline Alert and keeps the dialog open. Result: a failed clear now produces both a toast and an inline error in the modal. Verified that the other four mutations wrapped by ConfirmModal (useDeleteGarden, useDeletePlant, useDeleteSeedLot, useRemoveShare) have no global onError, so only ClearBed is affected. Suggested fix: drop the global onError toast from useClearObject (the inline alert is the better UX for a modal action), since callers can't inject per-call onError overrides through ConfirmModal.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
ConfirmModal: The new component centralizes confirm-dialog logic but does not introduce new attack surface. User-provided text (title, confirmLabel, busyLabel, errorFallback) is rendered as React text nodes, not HTML. The children body is JSX passed by callers, not raw HTML strings.
Error display: errorMessage only surfaces err.message for ApiError instances (server-generated messages) and falls back to a fixed string for everything else. The <Alert> component renders children via normal React interpolation ({children}), not dangerouslySetInnerHTML, so even a malicious server error payload cannot execute XSS.
Destructive action wrappers (DeleteGarden, LeaveGarden, DeletePlant, DeleteSeedLot, ClearBed): All continue to use the same underlying TanStack Query mutations with identical auth/authorization semantics. No new auth bypasses or injection vectors are introduced.
AppShell layout change: Pure CSS/layout diff (max-w-5xl removal on canvas routes); no security impact.
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/editor/ClearBedModal.tsx:21 — The refactor switches ClearBedModal from clear.mutate() with an onSuccess callback to clear.mutateAsync() inside ConfirmModal. The underlying useClearObject hook (web/src/lib/objects.ts:384) still defines onError: (err) => toast.error(...). TanStack Query runs onError before mutateAsync rejects, so a failure now produces two notifications: the inline error from ConfirmModaland a toast from the mutation hook. Previously the error was shown once (as a toast). The fix is to remove the onError toast from useClearObject since ClearBedModal is its only consumer and now handles errors inline.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The refactor is clean and well-structured:
ConfirmModal abstraction — The new primitive correctly captures the repeated pattern (busy lock, inline error, footer buttons) and all five callers supply only their unique content, labels, and mutation. The API surface is minimal and appropriately typed.
AppShell layout — The canvasRoute variable is well-named and used consistently for both the nav width and the main content width. The conditional classNames follow existing patterns in the codebase (cn from @/lib/cn).
Dead code removal — PageStub.tsx was verified as unimported before deletion.
Consistent migration — All confirm dialogs migrated uniformly from callback-style mutations (mutate with onSuccess/onError) to mutateAsync handled by the primitive.
⚡ Performance — No material issues found
No material issues found
I checked the mutation hooks (useDeleteGarden, useDeletePlant, useDeleteSeedLot, useRemoveShare, useClearObject) and the new ConfirmModal implementation. The refactor simply moves the per-modal useState for busy/error into a shared component — the number of state containers and re-render boundaries is identical. The switch from mutate with callbacks to mutateAsync awaited in ConfirmModal does not change the network or rendering behavior materially. No hot loops, no unbounded growth, no missing limits, no allocation regressions, and no new blocking patterns were introduced.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
LeaveGardenModal closes on no-op when me.data is absent.web/src/components/gardens/LeaveGardenModal.tsx:18
In the old code, the guard if (!me.data) return kept the dialog open when there was no current user. In the refactored version, onConfirm resolves with undefined instead of returning early, so ConfirmModal.handleConfirm proceeds to call onClose() and dismisses the dialog. The button is disabled via confirmDisabled={!me.data}, so this is only reachable via programmatic invocation or a race condition, but it is a behavior change in an edge case.
ClearBedModal now shows duplicate error notifications.web/src/components/ui/ConfirmModal.tsx:52 (cascading from web/src/editor/ClearBedModal.tsx:27)
useClearObject already has an onError toast handler. Refactoring ClearBedModal onto ConfirmModal means a failure now surfaces both as an inline Alert inside the modal and as a toast from the mutation hook. The PR calls the inline surfacing a "bonus," but the user sees the same error twice. Consider suppressing the hook-level toast when the caller will surface the error inline (e.g., pass an optional onError override or use mutateAsync without the hook's onError).
Through the security lens this is a behavior-preserving UI refactor: confirm dialogs consolidated onto a ConfirmModal primitive, a dead component deleted, and a CSS width change. No new inputs, no authn/authz surface touched, no injection vectors (all dynamic text — garden.name, plant.name, objectName, error messages — is rendered as JSX text content, which React escapes; no dangerouslySetInnerHTML), no secret/credential handling, no deserialization, no SSRF, no new untrusted-data paths. The onConfirm actions delegate to the same React Query mutations (useDeleteGarden, useRemoveShare, etc.) that already enforce server-side permissions; the refactor doesn't change what's called or with what arguments (verified Modal.tsx to confirm the busy/backdrop/Escape semantics the primitive relies on are unchanged). The errorMessage→Alert rendering path is identical to the prior hand-rolled dialogs. Layout change is className-only.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the diff through the correctness lens and verified the key behavior-preservation claims against the checked-out code.
ConfirmModal busy/error semantics (web/src/components/ui/ConfirmModal.tsx:46-56): On success it calls onClose() with busy still true (never reset on the success path). That's fine — the parent unmounts the modal on close, so no stuck state. On error it sets the message and resets busy=false, re-enabling the button for retry. Matches the prior per-dialog behavior.
LeaveGardenModal no-current-user guard (web/src/components/gardens/LeaveGardenModal.tsx): Old code early-returned inside onConfirm when !me.data (kept dialog open) and disabled the button. New code gates via confirmDisabled={!me.data} and the onConfirm no-ops when me.data is absent. Since the button is disabled, handleConfirm can't fire, so the dialog can't close via the empty-action path. Behavior preserved. Verified by reading the file.
ClearBedModal error surfacing (web/src/editor/ClearBedModal.tsx:27): Switched from clear.mutate(id, { onSuccess: onClose }) (errors swallowed) to clear.mutateAsync(id) caught by ConfirmModal. The plopCount === 0 guard is preserved as confirmDisabled. This is the one intentional behavior improvement and is correct.
DeleteSeedLotModal (web/src/components/plants/DeleteSeedLotModal.tsx): del.mutateAsync throws on error and is caught by ConfirmModal's try/catch — equivalent to the old onError callback path.
AppShell width logic (web/src/components/layout/AppShell.tsx:44): canvasRoute = inEditor || inPublicGarden is the same union that previously gated showBottomNav; reusing it for the width constraint is correct. The bottom-nav gating (!!user && !canvasRoute) is unchanged in effect. Verified by reading the file.
PageStub removal: grep for PageStub across web/src returns no remaining references — the deletion is safe.
No constants, conversions, or domain thresholds are introduced by this change, so there was nothing to re-derive.
🧹 Code cleanliness & maintainability — No material issues found
I've reviewed the diff through the code-cleanliness lens. Verdict: No material issues found.
Notes from verification:
ConfirmModal cleanly captures the repeated hand-rolled pattern (busy lock, inline error, Cancel/Confirm footer) across all five folded dialogs; each caller now contributes only its message, action, and labels. The primitive's doc comment is explicit about its scope and the Modal-direct escape hatch for input-bearing dialogs, which is the right boundary.
PageStub.tsx deletion is safe — grep confirms no remaining references in web/src.
The DeleteSeedLotModal lost its gap-3 for gap-4 (the new primitive uses gap-4), a deliberate normalisation, not a regression.
AppShell.tsx width logic is readable: canvasRoute is introduced once and used in both the nav and main, with comments explaining the why. The cn(...) ternary for the nav (canvasRoute ? '' : 'mx-auto max-w-5xl') is slightly less tidy than the && conditional used in main, but it's a minor stylistic inconsistency, not material.
All imports resolve; no dead imports left behind in the refactored modals.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this is a UI cleanup PR (component refactor + dead-file deletion + layout width change). No new queries, loops, allocations, or hot-path work introduced. The ConfirmModal replaces per-caller useState + try/catch with the same local state lifted into a shared component — no extra renders or allocations beyond what the originals did. The AppShell width change is a className conditional, negligible. PageStub deletion is pure dead-code removal.
🧯 Error handling & edge cases — Minor issues
I'll verify both findings against the actual code.
Let me verify the other mutation hooks to confirm ClearBed is unique in keeping an onError toast.
Both findings are confirmed by reading the actual code.
Finding 1 confirmed: useClearObject (objects.ts:384) keeps onError: (err) => toast.error(...), while useDeleteGarden, useDeletePlant, useDeleteSeedLot, and useRemoveShare have no onError toast. ClearBedModal now uses ConfirmModal, which renders an inline Alert on throw — so a clear failure produces both the toast and the inline alert.
Finding 2 confirmed: LeaveGardenModal (line 18-20) guards onConfirm with if (me.data) and otherwise resolves without throwing, so ConfirmModal.handleConfirm proceeds to onClose(). The button's disabled state and the guard both key off me.data in the same render, so it's currently unreachable but latent.
VERDICT: Minor issues
Double error reporting in ClearBedModal — web/src/editor/ClearBedModal.tsx:26 (via web/src/components/ui/ConfirmModal.tsx). useClearObject still carries onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')) (web/src/lib/objects.ts:384), so on a clear failure the user now sees both the inline Alert that ConfirmModal renders and the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a global error side effect — useDeleteGarden (gardens.ts:133), useDeletePlant (plants.ts:110), useDeleteSeedLot (seedLots.ts:97), and useRemoveShare (shares.ts:61) all have no onError. Cosmetic noise on failure, not a swallowed error; fix is to drop the onError toast from useClearObject since the inline alert is the better UX this PR introduces.
LeaveGardenModal silently no-ops when me.data is missing — web/src/components/gardens/LeaveGardenModal.tsx:18. The onConfirm guards with if (me.data) await remove.mutateAsync(...) and otherwise resolves, so ConfirmModal closes the dialog as if the leave succeeded. The confirm button is disabled by confirmDisabled={!me.data} using the same value in the same render, so the path is currently unreachable; this is a latent fragility only (a future change decoupling confirmDisabled from me.data would silently make onConfirm a no-op-that-closes). No fix required now.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This PR is a pure frontend refactor: it extracts five duplicated confirmation dialogs into a single ConfirmModal primitive, deletes a dead PageStub component, and tweaks layout CSS for full-width canvas routes.
No authn/authz changes — the same backend endpoints and permission checks remain unchanged.
No injection vectors — all interpolated data (garden.name, plant.name, objectName, lot.vendor, lot.packedForYear) is rendered through React JSX, which auto-escapes HTML. dangerouslySetInnerHTML is not used anywhere in the new or changed files.
No unsafe deserialization or secret leakage — no new network parsing, no credential handling, no env var changes.
The errorMessage helper returns server text that is rendered inside <Alert>{error}</Alert> as React children; it is not injected as raw HTML.
🎯 Correctness — No material issues found
No material issues found
ConfirmModal: Correctly manages local busy state, awaits onConfirm, catches errors to show inline, and only calls onClose on success. setBusy(false) in the catch path keeps the dialog open for retry, which is the intended behavior.
mutateAsync migration: All five refactored dialogs now pass mutateAsync (Promise-returning, throws on error) into ConfirmModal.onConfirm instead of the old mutate + onSuccess/onError callbacks. This is semantically equivalent and required for the modal's try/await to work.
LeaveGardenModal guard: confirmDisabled={!me.data} disables the button when the user isn't loaded; the onConfirm also early-returns if !me.data. Behaviorally equivalent to the old disabled={... || !me.data} plus if (!me.data) return.
AppShell canvas route detection: matchRoute({ to: '/gardens/$gardenId', fuzzy: false }) and matchRoute({ to: '/g/$token', fuzzy: false }) exactly match the route definitions in router.tsx (garden editor and public garden respectively). fuzzy: false correctly excludes the parent /gardens list.
PageStub removal: Confirmed unused across the entire web/src tree.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Wait — one of the refactored modals regresses into a duplicate error-reporting path.
useClearObject (web/src/lib/objects.ts:384) already has an onError handler that calls toast.error(...) with the message "Could not clear the bed."
ConfirmModal catches the rejected mutateAsync and also renders that same error inline inside the dialog.
Result: when clearing fails, the user gets both a toast and the inline alert — two surfaces for the same failure.
The other four refactored modals are fine because their underlying hooks (useDeleteGarden, useDeletePlant, useRemoveShare, useDeleteSeedLot) do not toast on error. Only useClearObject does.
Suggested fix: Remove the onError toast from useClearObject (or suppress it when called via mutateAsync inside a modal). Since ConfirmModal now owns the inline error for confirmations, the hook-level toast is the one that should go.
⚡ Performance — No material issues found
Verdict: No material issues found
This PR is a frontend-only refactor with no queries, no loops, no unbounded growth, and no hot-path changes. From a performance lens, it is clean.
ConfirmModal simply deduplicates the five hand-rolled confirmations into one component. It introduces no extra network requests, no new allocations on hot paths, and no render-loop hazards. Its internal busy/error states are bounded and only mutate on user interaction (confirm click), which is the same lifecycle as the per-modal states it replaces.
PageStub removal is dead-code deletion — strictly neutral.
AppShell layout changes are CSS-only (max-w-5xl toggles); the matchRoute calls existed before and are not in a render-critical path.
🧯 Error handling & edge cases — No material issues found
No material issues found
All five refactored dialogs now surface errors inline via ConfirmModal; none swallow errors anymore. ClearBedModal is explicitly improved (it previously used mutate with only onSuccess, discarding failures).
LeaveGardenModal still guards remove.mutateAsync(me.data.id) with if (me.data) inside the onConfirm callback (line 19), so it no-ops gracefully when me.data is absent rather than dereferencing null. This matches the old behavior.
ConfirmModal resets error to null on each confirm attempt (line 47), sets busy = true before awaiting (line 48), and on throw sets the error and busy = false (lines 53-54), leaving the dialog open for retry. The Modal component itself suppresses Escape/backdrop-close while busy is true, so the in-flight state is protected from accidental dismissal.
No new unhandled edge cases (nil/null, empty collections, zero/negative, boundary conditions) were introduced by the refactor. The plopCount === 0 guard on ClearBedModal (line 25) remains intact.
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** · 6 findings (3 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | Dialog closes on no-op when me.data is absent instead of staying open | `web/src/components/gardens/LeaveGardenModal.tsx:18` | 3/5 | error-handling |
| 🟡 | ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError) | `web/src/editor/ClearBedModal.tsx:26` | 3/5 | correctness, error-handling |
| 🟠 | Duplicate error notification: ClearBedModal inline error + useClearObject toast on mutation failure | `web/src/editor/ClearBedModal.tsx:20` | 2/5 | correctness |
<details><summary>3 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | ClearBed failure now surfaces both a toast and ConfirmModal's inline alert | `web/src/lib/objects.ts:384` | claude-code/sonnet | error-handling |
| ⚪ | Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file | `web/src/components/layout/AppShell.tsx:54` | claude-code/sonnet | maintainability |
| ⚪ | confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive | `web/src/components/ui/ConfirmModal.tsx:33` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
This PR is a frontend-only UI refactor: extracting a shared `ConfirmModal` primitive, deleting dead scaffolding (`PageStub`), and widening the editor layout. I verified the new `ConfirmModal.tsx`, `Alert.tsx`, and `Modal.tsx` — no `dangerouslySetInnerHTML` anywhere in the touched files (confirmed via repo-wide grep), all error/message content is rendered as plain React children (auto-escaped), and `errorMessage()` only surfaces `ApiError.message` or a static fallback string, unchanged behavior from before the refactor. No authn/authz logic, no new API calls, no injection-relevant input handling, and no secret handling in any of the changed files. The layout/width changes in `AppShell.tsx` are purely CSS class changes with no security surface.
Nothing in this diff falls within the security lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
This confirms the AppShell change is a straightforward semantics-preserving refactor (logic unchanged, just renamed/reused). No issue there, matching the draft's conclusion.
The single finding in the draft is fully verified against the actual code.
## Corrected Review
**VERDICT: Minor issues**
- `web/src/editor/ClearBedModal.tsx:20-24` (via `web/src/lib/objects.ts:374-386`, `web/src/components/ui/ConfirmModal.tsx:46-56`): `ClearBedModal` now routes its mutation through `ConfirmModal`, which calls `clear.mutateAsync(objectId)` and catches failures to render an inline `Alert`. But `useClearObject` (`lib/objects.ts:384`) already has its own `onError: (err) => toast.error(...)` on the mutation. TanStack Query fires a mutation's `onError` option independently of whether the caller awaits `mutateAsync` and catches the rejection, so both fire. A failed "Clear bed" now shows the error twice: once as a persistent toast (error toasts don't auto-dismiss, per `components/ui/toast.tsx:44-49`) and once as the modal's inline `Alert`. Confirmed none of the other four hooks routed through `ConfirmModal` (`useDeleteGarden`, `useRemoveShare`, `useDeletePlant`, `useDeleteSeedLot`) has a hook-level `onError`, so this duplication is specific to `ClearBedModal`. Fix: drop the `onError` toast from `useClearObject` (let the modal own the error, consistent with the other four hooks).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed both findings against the actual code — line 54 is the ternary (`canvasRoute ? '' : 'mx-auto max-w-5xl'`), line 98 is the `&&` short-circuit for the identical condition; `confirmVariant` is only ever assigned its default, with no caller passing `'primary'` anywhere in `web/src`.
VERDICT: Minor issues
- `web/src/components/layout/AppShell.tsx:54` vs `:98` — the `nav` className uses a ternary-to-empty-string (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) while `<main>` (44 lines below) uses a short-circuit (`!canvasRoute && 'mx-auto max-w-5xl'`) for the identical `canvasRoute` condition. `cn()` strips falsy values, so the ternary's `''` branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise cleaning up. Suggest aligning the `nav` line to `!canvasRoute && 'mx-auto max-w-5xl'` to match `<main>`.
- `web/src/components/ui/ConfirmModal.tsx:33` (type declared at line 35) — `confirmVariant?: 'danger' | 'primary'` is exposed on the new primitive, but no caller in `web/src` passes anything but the default (`'danger'`). Speculative API surface on a brand-new component, contrary to the project's "don't design for hypothetical future requirements" convention — low priority, fine to leave if the next confirm-style dialog is expected to need it soon.
Both are cosmetic; nothing here blocks merge. The five `ConfirmModal` conversions are faithful, `PageStub` removal is clean, and the full-width editor changes correctly centralize the `canvasRoute` condition.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
This PR is a frontend-only refactor (dead code removal, layout class changes, and consolidating five duplicate confirm-dialog components into a shared `ConfirmModal`). I read `ConfirmModal.tsx`, `AppShell.tsx`, and `Modal.tsx` in full, plus all five modal call sites in the diff.
Through the performance lens specifically:
- No new loops, no data-fetching changes, no added re-render triggers of consequence. `sections.filter(...)` in `AppShell` was already there and runs once per render over a 3-item array — non-issue.
- `Modal`'s focus-trap effect (mount-only, `[]` deps) is unchanged by this PR.
- The seed-lot modal switch from `del.mutate(..., { onSuccess, onError })` to `del.mutateAsync()` inside `ConfirmModal`'s `onConfirm` is functionally equivalent — no extra network calls or renders introduced.
- Removing `PageStub.tsx` is pure deletion of unused code, no runtime effect.
- The `max-w-5xl` → full-width class changes are static Tailwind class strings recomputed via `cn(...)` per render, same cost as before (it was already a template string with conditionals).
Nothing here touches hot loops, allocation-heavy paths, network calls, or unbounded growth. Nothing to report in this lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings verified directly against the source.
**Finding 1** — `objects.ts:384` confirms `useClearObject` still has `onError: (err) => toast.error(...)`, while `gardens.ts:136-138`, `plants.ts:112-115`, `seedLots.ts:98-104`, and `shares.ts:63-69` confirm none of the other four converted hooks (`useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, `useRemoveShare`) define an `onError`. `ConfirmModal.tsx:46-55` confirms `handleConfirm` awaits `onConfirm()` in a try/catch and sets its own inline `Alert` on rejection — so for `ClearBedModal` specifically, a failure fires both the hook-level toast and the modal's inline alert. `package.json:19` confirms `@tanstack/react-query: ^5.62.0`, where `mutateAsync` rejecting alongside a configured `onError` is standard behavior. Confirmed as described.
**Finding 2** — `LeaveGardenModal.tsx:18-20` confirms `onConfirm={async () => { if (me.data) await remove.mutateAsync(me.data.id) }}` — when `me.data` is falsy this resolves without throwing. `ConfirmModal.tsx:46-51` confirms `handleConfirm` treats any non-throwing resolution as success and calls `onClose()`. Confirmed as described; also confirmed gated by `confirmDisabled={!me.data}` at line 16, matching the draft's "low practical severity" framing.
## VERDICT: Minor issues
- **`web/src/lib/objects.ts:384` / `web/src/editor/ClearBedModal.tsx`** — `useClearObject` still has its own `onError: (err) => toast.error(...)`. Since `ClearBedModal` now uses `ConfirmModal`, whose `handleConfirm` also catches the same rejection from `clear.mutateAsync(objectId)` and renders it inline (`ConfirmModal.tsx:52-54`), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (`useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, `useRemoveShare`) define an `onError`, so only this one double-surfaces. Fix: drop the `onError` toast from `useClearObject` now that the modal shows the error inline.
- **`web/src/components/gardens/LeaveGardenModal.tsx:18-20`** — `onConfirm` silently no-ops when `me.data` is falsy instead of throwing, so `ConfirmModal.handleConfirm` treats the no-op as success and calls `onClose()` as though the leave succeeded. Gated by `confirmDisabled={!me.data}` (line 16) so unreachable via a normal click today, but it's a latent trap for this pattern if reused elsewhere. Low severity; worth tightening (throw or otherwise signal failure instead of resolving silently).
</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
I traced the security-relevant data flow through this refactor and found nothing materially wrong in my lane:
- **Error message rendering (XSS)**: `ConfirmModal` surfaces errors via `errorMessage(err, errorFallback)` → `<Alert>{error}</Alert>`. `Alert` (web/src/components/ui/Alert.tsx:7-19) renders children as React text nodes (no `dangerouslySetInnerHTML`), so the message is HTML-escaped. `errorMessage` (web/src/lib/api.ts:135-138) returns either the server-controlled `ApiError.message` or a static per-caller `errorFallback` literal — no untrusted client input is interpolated into the DOM. Verified by reading both files.
- **No new tainted input paths**: The refactor only consolidates existing confirm dialogs. `children`, `title`, `confirmLabel`, etc. are all caller-supplied static JSX/strings (garden/plant names were already rendered the same way before). No new user-controlled string is introduced into a sink.
- **No credential/secret leakage**: `errorFallback` values are static literals; nothing from `ApiError.body` (which can hold a server row on 409) is rendered — only `err.message`. No secret exposure.
- **Authn guard preserved**: `LeaveGardenModal` keeps the `confirmDisabled={!me.data}` guard (web/src/components/gardens/LeaveGardenModal.tsx), so the action button stays disabled when there's no authenticated user, matching the prior `disabled={remove.isPending || !me.data}`.
- **ClearBed error surfacing is an improvement**: ClearBed previously swallowed failures (`mutate` with only `onSuccess`); now it uses `mutateAsync` and shows the error inline. Better failure visibility, no security regression.
The `ConfirmModal` busy/disabled state is React-state-based (like the pre-existing `isPending` pattern it replaces), so it's not a new double-submit vector — and that's a correctness/UX concern outside this lens anyway.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`ClearBedModal` now double-reports failures (inline Alert + toast).** `web/src/editor/ClearBedModal.tsx:27` calls `clear.mutateAsync(objectId)`. The `useClearObject` mutation (`web/src/lib/objects.ts:384`) has an `onError` that fires `toast.error(...)`. `mutateAsync` rejects on error, so `ConfirmModal`'s `catch` ALSO sets the inline `Alert` (`web/src/components/ui/ConfirmModal.tsx:52-54`). The original code used `clear.mutate(objectId, { onSuccess: onClose })` — `mutate` does not reject, and the old modal had no inline error state at all, so only the toast fired. The net effect of this PR is that a failed clear now shows **both** an inline Alert and the existing toast. Verified against the real source: `useClearObject` keeps its `onError` toast, while the other four mutations (`useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, `useRemoveShare`) have no `onError` (grep confirmed no matches in `gardens.ts`, `plants.ts`, `seedLots.ts`, `shares.ts`), so this duplication is specific to the clear flow. Consistent fix: drop the `onError` toast from `useClearObject` so the dialog owns the inline message, matching the other four confirm flows.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed the new `ConfirmModal` primitive, the five folded dialogs, the `PageStub` deletion, and the `AppShell` width change. Verified:
- `PageStub` has zero remaining imports (grep across `web/src` → no matches) — clean removal.
- All five callers now use `ConfirmModal` consistently; the duplicated busy-lock/try-catch/footer boilerplate is genuinely collapsed into one place (`ConfirmModal.tsx:46-56`).
- `ConfirmModal`'s `confirmVariant` is restricted to `'danger' | 'primary'`, which matches `Button`'s actual `ButtonVariant` (`Button.tsx:4`) — no leaky/invalid union.
- `AppShell.tsx:44,54,98` — `canvasRoute` is named and reused for both the nav and main width, replacing the prior `inEditor && inPublicGarden`-style spread; the `cn(..., canvasRoute ? '' : 'mx-auto max-w-5xl')` pattern matches the surrounding conditional-class style. Brand and editor both key off `px-4`, so alignment holds.
One trivial note (not worth blocking): `ConfirmModal`'s `confirmVariant` prop (`ConfirmModal.tsx:23,35`) is declared and documented but none of the five callers pass it — they all rely on the `'danger'` default. It's a reasonable, cheap extension point and self-documenting, so I'd leave it; flagging only for completeness as speculative API surface.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
Reviewed through the performance lens:
- **ConfirmModal** (`web/src/components/ui/ConfirmModal.tsx`): The busy/error state moved from each caller's react-query `isPending` into local `useState`. This is render-equivalent — one `useState` pair instead of reading `mutation.isPending`, no extra subscriptions or re-renders. `handleConfirm` awaits `onConfirm()` and only resets `busy` on throw; no leaked in-flight work or unbounded growth. Verified at `ConfirmModal.tsx:43-56`.
- **AppShell** (`web/src/components/layout/AppShell.tsx`): The two `matchRoute` calls (`inEditor`, `inPublicGarden`) already existed pre-PR; `canvasRoute` is just a boolean OR of them, so no new route matching work per render. The `nav`/`main` now go through `cn()` (twMerge+clsx) instead of a static string — a couple of small allocations on AppShell renders, but AppShell is not a hot path and this is immaterial. Verified at `AppShell.tsx:40-54,93-102` and `cn.ts:1-7`.
- **Modal removals / PageStub deletion**: pure dead-code/layout cleanup; nothing on a hot path, no allocation or query behavior change.
- No N+1 queries, no missing pagination/limits, no work moved into loops, no blocking calls introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/editor/ClearBedModal.tsx:27` — double error reporting on failure (toast + inline alert).** `useClearObject` (`web/src/lib/objects.ts:384`) defines a mutation-level `onError` that fires `toast.error(objectErrorMessage(...))`. The PR switches from `clear.mutate(objectId, { onSuccess: onClose })` (which on failure only ran the global toast) to `clear.mutateAsync(objectId)`, but `mutateAsync` does not suppress the mutation's global `onError` — React Query still invokes it. `mutateAsync` *also* rejects, so `ConfirmModal.handleConfirm`'s `catch` sets the inline `Alert` and keeps the dialog open. Result: a failed clear now produces **both** a toast and an inline error in the modal. Verified that the other four mutations wrapped by `ConfirmModal` (`useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, `useRemoveShare`) have no global `onError`, so only ClearBed is affected. Suggested fix: drop the global `onError` toast from `useClearObject` (the inline alert is the better UX for a modal action), since callers can't inject per-call `onError` overrides through `ConfirmModal`.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
Security review of the changes:
- **ConfirmModal**: The new component centralizes confirm-dialog logic but does not introduce new attack surface. User-provided text (`title`, `confirmLabel`, `busyLabel`, `errorFallback`) is rendered as React text nodes, not HTML. The `children` body is JSX passed by callers, not raw HTML strings.
- **Error display**: `errorMessage` only surfaces `err.message` for `ApiError` instances (server-generated messages) and falls back to a fixed string for everything else. The `<Alert>` component renders children via normal React interpolation (`{children}`), not `dangerouslySetInnerHTML`, so even a malicious server error payload cannot execute XSS.
- **Destructive action wrappers** (`DeleteGarden`, `LeaveGarden`, `DeletePlant`, `DeleteSeedLot`, `ClearBed`): All continue to use the same underlying TanStack Query mutations with identical auth/authorization semantics. No new auth bypasses or injection vectors are introduced.
- **AppShell layout change**: Pure CSS/layout diff (`max-w-5xl` removal on canvas routes); no security impact.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/editor/ClearBedModal.tsx:21`** — The refactor switches `ClearBedModal` from `clear.mutate()` with an `onSuccess` callback to `clear.mutateAsync()` inside `ConfirmModal`. The underlying `useClearObject` hook (`web/src/lib/objects.ts:384`) still defines `onError: (err) => toast.error(...)`. TanStack Query runs `onError` before `mutateAsync` rejects, so a failure now produces **two notifications**: the inline error from `ConfirmModal` *and* a toast from the mutation hook. Previously the error was shown once (as a toast). The fix is to remove the `onError` toast from `useClearObject` since `ClearBedModal` is its only consumer and now handles errors inline.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
The refactor is clean and well-structured:
- **ConfirmModal abstraction** — The new primitive correctly captures the repeated pattern (busy lock, inline error, footer buttons) and all five callers supply only their unique content, labels, and mutation. The API surface is minimal and appropriately typed.
- **AppShell layout** — The `canvasRoute` variable is well-named and used consistently for both the nav width and the main content width. The conditional classNames follow existing patterns in the codebase (`cn` from `@/lib/cn`).
- **Dead code removal** — `PageStub.tsx` was verified as unimported before deletion.
- **Consistent migration** — All confirm dialogs migrated uniformly from callback-style mutations (`mutate` with `onSuccess`/`onError`) to `mutateAsync` handled by the primitive.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
I checked the mutation hooks (`useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, `useRemoveShare`, `useClearObject`) and the new `ConfirmModal` implementation. The refactor simply moves the per-modal `useState` for `busy`/`error` into a shared component — the number of state containers and re-render boundaries is identical. The switch from `mutate` with callbacks to `mutateAsync` awaited in `ConfirmModal` does not change the network or rendering behavior materially. No hot loops, no unbounded growth, no missing limits, no allocation regressions, and no new blocking patterns were introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`LeaveGardenModal` closes on no-op when `me.data` is absent.** `web/src/components/gardens/LeaveGardenModal.tsx:18`
In the old code, the guard `if (!me.data) return` kept the dialog open when there was no current user. In the refactored version, `onConfirm` resolves with `undefined` instead of returning early, so `ConfirmModal.handleConfirm` proceeds to call `onClose()` and dismisses the dialog. The button is disabled via `confirmDisabled={!me.data}`, so this is only reachable via programmatic invocation or a race condition, but it is a behavior change in an edge case.
- **`ClearBedModal` now shows duplicate error notifications.** `web/src/components/ui/ConfirmModal.tsx:52` (cascading from `web/src/editor/ClearBedModal.tsx:27`)
`useClearObject` already has an `onError` toast handler. Refactoring `ClearBedModal` onto `ConfirmModal` means a failure now surfaces both as an inline `Alert` inside the modal *and* as a toast from the mutation hook. The PR calls the inline surfacing a "bonus," but the user sees the same error twice. Consider suppressing the hook-level toast when the caller will surface the error inline (e.g., pass an optional `onError` override or use `mutateAsync` without the hook's `onError`).
</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>
No material issues found.
Through the security lens this is a behavior-preserving UI refactor: confirm dialogs consolidated onto a `ConfirmModal` primitive, a dead component deleted, and a CSS width change. No new inputs, no authn/authz surface touched, no injection vectors (all dynamic text — `garden.name`, `plant.name`, `objectName`, error messages — is rendered as JSX text content, which React escapes; no `dangerouslySetInnerHTML`), no secret/credential handling, no deserialization, no SSRF, no new untrusted-data paths. The `onConfirm` actions delegate to the same React Query mutations (`useDeleteGarden`, `useRemoveShare`, etc.) that already enforce server-side permissions; the refactor doesn't change what's called or with what arguments (verified `Modal.tsx` to confirm the busy/backdrop/Escape semantics the primitive relies on are unchanged). The `errorMessage`→`Alert` rendering path is identical to the prior hand-rolled dialogs. Layout change is className-only.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the diff through the correctness lens and verified the key behavior-preservation claims against the checked-out code.
- **`ConfirmModal` busy/error semantics** (`web/src/components/ui/ConfirmModal.tsx:46-56`): On success it calls `onClose()` with `busy` still `true` (never reset on the success path). That's fine — the parent unmounts the modal on close, so no stuck state. On error it sets the message and resets `busy=false`, re-enabling the button for retry. Matches the prior per-dialog behavior.
- **`LeaveGardenModal` no-current-user guard** (`web/src/components/gardens/LeaveGardenModal.tsx`): Old code early-returned inside `onConfirm` when `!me.data` (kept dialog open) and disabled the button. New code gates via `confirmDisabled={!me.data}` and the `onConfirm` no-ops when `me.data` is absent. Since the button is disabled, `handleConfirm` can't fire, so the dialog can't close via the empty-action path. Behavior preserved. Verified by reading the file.
- **`ClearBedModal` error surfacing** (`web/src/editor/ClearBedModal.tsx:27`): Switched from `clear.mutate(id, { onSuccess: onClose })` (errors swallowed) to `clear.mutateAsync(id)` caught by `ConfirmModal`. The `plopCount === 0` guard is preserved as `confirmDisabled`. This is the one intentional behavior improvement and is correct.
- **`DeleteSeedLotModal`** (`web/src/components/plants/DeleteSeedLotModal.tsx`): `del.mutateAsync` throws on error and is caught by `ConfirmModal`'s try/catch — equivalent to the old `onError` callback path.
- **`AppShell` width logic** (`web/src/components/layout/AppShell.tsx:44`): `canvasRoute = inEditor || inPublicGarden` is the same union that previously gated `showBottomNav`; reusing it for the width constraint is correct. The bottom-nav gating (`!!user && !canvasRoute`) is unchanged in effect. Verified by reading the file.
- **`PageStub` removal**: `grep` for `PageStub` across `web/src` returns no remaining references — the deletion is safe.
No constants, conversions, or domain thresholds are introduced by this change, so there was nothing to re-derive.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I've reviewed the diff through the code-cleanliness lens. Verdict: No material issues found.
Notes from verification:
- `ConfirmModal` cleanly captures the repeated hand-rolled pattern (busy lock, inline error, Cancel/Confirm footer) across all five folded dialogs; each caller now contributes only its message, action, and labels. The primitive's doc comment is explicit about its scope and the `Modal`-direct escape hatch for input-bearing dialogs, which is the right boundary.
- `PageStub.tsx` deletion is safe — grep confirms no remaining references in `web/src`.
- The `DeleteSeedLotModal` lost its `gap-3` for `gap-4` (the new primitive uses `gap-4`), a deliberate normalisation, not a regression.
- `AppShell.tsx` width logic is readable: `canvasRoute` is introduced once and used in both the nav and main, with comments explaining the why. The `cn(...)` ternary for the nav (`canvasRoute ? '' : 'mx-auto max-w-5xl'`) is slightly less tidy than the `&&` conditional used in `main`, but it's a minor stylistic inconsistency, not material.
- All imports resolve; no dead imports left behind in the refactored modals.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the performance lens, this is a UI cleanup PR (component refactor + dead-file deletion + layout width change). No new queries, loops, allocations, or hot-path work introduced. The `ConfirmModal` replaces per-caller `useState` + try/catch with the same local state lifted into a shared component — no extra renders or allocations beyond what the originals did. The `AppShell` width change is a className conditional, negligible. `PageStub` deletion is pure dead-code removal.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I'll verify both findings against the actual code.
Let me verify the other mutation hooks to confirm ClearBed is unique in keeping an onError toast.
Both findings are confirmed by reading the actual code.
Finding 1 confirmed: `useClearObject` (objects.ts:384) keeps `onError: (err) => toast.error(...)`, while `useDeleteGarden`, `useDeletePlant`, `useDeleteSeedLot`, and `useRemoveShare` have no `onError` toast. `ClearBedModal` now uses `ConfirmModal`, which renders an inline `Alert` on throw — so a clear failure produces both the toast and the inline alert.
Finding 2 confirmed: `LeaveGardenModal` (line 18-20) guards `onConfirm` with `if (me.data)` and otherwise resolves without throwing, so `ConfirmModal.handleConfirm` proceeds to `onClose()`. The button's disabled state and the guard both key off `me.data` in the same render, so it's currently unreachable but latent.
**VERDICT: Minor issues**
- **Double error reporting in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:26` (via `web/src/components/ui/ConfirmModal.tsx`). `useClearObject` still carries `onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.'))` (`web/src/lib/objects.ts:384`), so on a clear failure the user now sees both the inline `Alert` that `ConfirmModal` renders *and* the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a global error side effect — `useDeleteGarden` (`gardens.ts:133`), `useDeletePlant` (`plants.ts:110`), `useDeleteSeedLot` (`seedLots.ts:97`), and `useRemoveShare` (`shares.ts:61`) all have no `onError`. Cosmetic noise on failure, not a swallowed error; fix is to drop the `onError` toast from `useClearObject` since the inline alert is the better UX this PR introduces.
- **`LeaveGardenModal` silently no-ops when `me.data` is missing** — `web/src/components/gardens/LeaveGardenModal.tsx:18`. The `onConfirm` guards with `if (me.data) await remove.mutateAsync(...)` and otherwise resolves, so `ConfirmModal` closes the dialog as if the leave succeeded. The confirm button is disabled by `confirmDisabled={!me.data}` using the same value in the same render, so the path is currently unreachable; this is a latent fragility only (a future change decoupling `confirmDisabled` from `me.data` would silently make `onConfirm` a no-op-that-closes). No fix required now.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
- This PR is a pure frontend refactor: it extracts five duplicated confirmation dialogs into a single `ConfirmModal` primitive, deletes a dead `PageStub` component, and tweaks layout CSS for full-width canvas routes.
- **No authn/authz changes** — the same backend endpoints and permission checks remain unchanged.
- **No injection vectors** — all interpolated data (`garden.name`, `plant.name`, `objectName`, `lot.vendor`, `lot.packedForYear`) is rendered through React JSX, which auto-escapes HTML. `dangerouslySetInnerHTML` is not used anywhere in the new or changed files.
- **No unsafe deserialization or secret leakage** — no new network parsing, no credential handling, no env var changes.
- The `errorMessage` helper returns server text that is rendered inside `<Alert>{error}</Alert>` as React children; it is not injected as raw HTML.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
- **ConfirmModal**: Correctly manages local busy state, awaits `onConfirm`, catches errors to show inline, and only calls `onClose` on success. `setBusy(false)` in the catch path keeps the dialog open for retry, which is the intended behavior.
- **mutateAsync migration**: All five refactored dialogs now pass `mutateAsync` (Promise-returning, throws on error) into `ConfirmModal.onConfirm` instead of the old `mutate` + `onSuccess/onError` callbacks. This is semantically equivalent and required for the modal's `try/await` to work.
- **LeaveGardenModal guard**: `confirmDisabled={!me.data}` disables the button when the user isn't loaded; the `onConfirm` also early-returns if `!me.data`. Behaviorally equivalent to the old `disabled={... || !me.data}` plus `if (!me.data) return`.
- **AppShell canvas route detection**: `matchRoute({ to: '/gardens/$gardenId', fuzzy: false })` and `matchRoute({ to: '/g/$token', fuzzy: false })` exactly match the route definitions in `router.tsx` (garden editor and public garden respectively). `fuzzy: false` correctly excludes the parent `/gardens` list.
- **PageStub removal**: Confirmed unused across the entire `web/src` tree.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
Wait — one of the refactored modals regresses into a duplicate error-reporting path.
- `useClearObject` (`web/src/lib/objects.ts:384`) already has an `onError` handler that calls `toast.error(...)` with the message "Could not clear the bed."
- `ConfirmModal` catches the rejected `mutateAsync` and also renders that same error inline inside the dialog.
- Result: when clearing fails, the user gets **both** a toast **and** the inline alert — two surfaces for the same failure.
The other four refactored modals are fine because their underlying hooks (`useDeleteGarden`, `useDeletePlant`, `useRemoveShare`, `useDeleteSeedLot`) do not toast on error. Only `useClearObject` does.
**Suggested fix:** Remove the `onError` toast from `useClearObject` (or suppress it when called via `mutateAsync` inside a modal). Since `ConfirmModal` now owns the inline error for confirmations, the hook-level toast is the one that should go.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict:** No material issues found
This PR is a frontend-only refactor with no queries, no loops, no unbounded growth, and no hot-path changes. From a performance lens, it is clean.
- **ConfirmModal** simply deduplicates the five hand-rolled confirmations into one component. It introduces no extra network requests, no new allocations on hot paths, and no render-loop hazards. Its internal `busy`/`error` states are bounded and only mutate on user interaction (confirm click), which is the same lifecycle as the per-modal states it replaces.
- **PageStub removal** is dead-code deletion — strictly neutral.
- **AppShell layout** changes are CSS-only (`max-w-5xl` toggles); the `matchRoute` calls existed before and are not in a render-critical path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
- **All five refactored dialogs** now surface errors inline via `ConfirmModal`; none swallow errors anymore. `ClearBedModal` is explicitly improved (it previously used `mutate` with only `onSuccess`, discarding failures).
- **LeaveGardenModal** still guards `remove.mutateAsync(me.data.id)` with `if (me.data)` inside the `onConfirm` callback (line 19), so it no-ops gracefully when `me.data` is absent rather than dereferencing null. This matches the old behavior.
- **ConfirmModal** resets `error` to `null` on each confirm attempt (line 47), sets `busy = true` before awaiting (line 48), and on throw sets the error and `busy = false` (lines 53-54), leaving the dialog open for retry. The `Modal` component itself suppresses `Escape`/backdrop-close while `busy` is true, so the in-flight state is protected from accidental dismissal.
- No new unhandled edge cases (nil/null, empty collections, zero/negative, boundary conditions) were introduced by the refactor. The `plopCount === 0` guard on `ClearBedModal` (line 25) remains intact.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
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 #107. Part of #96. The three tidy-ups from the audit's deferred list.
ConfirmModalprimitive — a message + Cancel/Confirm that owns the shared busy lock and inline error. Folded the five hand-rolled confirm dialogs onto it:DeleteGarden,LeaveGarden,DeletePlant,DeleteSeedLot,ClearBed. Each is now just its message + mutation + labels. Bonus:ClearBednow surfaces a failure inline instead of swallowing it. (CopyGardenstays onModal— it has a name field, so it's not a plain confirm.)components/PageStub.tsx— dead scaffolding, imported nowhere.max-w-5xlreading measure the other pages use. The canvas routes (editor + public garden) now go edge-to-edge on desktop, and the top bar matches so the brand aligns with the editor's left edge. Mobile was already full-width, so it's unchanged.Verified live
tsc+vitest+ build green.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 14:08:42Z
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 — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -46,0 +15,4 @@busyLabel="Leaving…"confirmDisabled={!me.data}errorFallback="Could not leave the garden."onConfirm={async () => {🟡 Dialog closes on no-op when me.data is absent instead of staying open
error-handling · flagged by 3 models
LeaveGardenModalcloses on no-op whenme.datais absent.web/src/components/gardens/LeaveGardenModal.tsx:18🪰 Gadfly · advisory
@@ -49,1 +52,3 @@<nav className="mx-auto flex max-w-5xl items-center gap-4 px-4 py-3">{/* The bar matches the content width below: constrained on reading pages,edge-to-edge on the canvas routes so the brand aligns with the editor. */}<nav className={cn('flex items-center gap-4 px-4 py-3', canvasRoute ? '' : 'mx-auto max-w-5xl')}>⚪ Ternary-to-empty-string vs && idiom used inconsistently for the same canvasRoute condition in one file
maintainability · flagged by 1 model
web/src/components/layout/AppShell.tsx:54vs:98— thenavclassName uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while<main>(44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identicalcanvasRoutecondition.cn()strips falsy values, so the ternary's''branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…🪰 Gadfly · advisory
@@ -0,0 +30,4 @@/** The body — what's being confirmed. */children: ReactNodeconfirmLabel: string/** Label while the action is in flight (e.g. "Deleting…"). */⚪ confirmVariant 'primary' option is unused by any caller — speculative API surface on a new primitive
maintainability · flagged by 1 model
web/src/components/layout/AppShell.tsx:54vs:98— thenavclassName uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while<main>(44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identicalcanvasRoutecondition.cn()strips falsy values, so the ternary's''branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise…🪰 Gadfly · advisory
@@ -41,3 +21,1 @@</div></div></Modal><ConfirmModal🟠 Duplicate error notification: ClearBedModal inline error + useClearObject toast on mutation failure
correctness · flagged by 2 models
web/src/editor/ClearBedModal.tsx:21— The refactor switchesClearBedModalfromclear.mutate()with anonSuccesscallback toclear.mutateAsync()insideConfirmModal. The underlyinguseClearObjecthook (web/src/lib/objects.ts:384) still definesonError: (err) => toast.error(...). TanStack Query runsonErrorbeforemutateAsyncrejects, so a failure now produces two notifications: the inline error fromConfirmModaland a toast from the mutation hook. Previously th…🪰 Gadfly · advisory
@@ -44,0 +23,4 @@confirmLabel="Clear bed"busyLabel="Clearing…"confirmDisabled={plopCount === 0}errorFallback="Could not clear the bed."🟡 ClearBed now double-reports errors (inline Alert + toast from useClearObject.onError)
correctness, error-handling · flagged by 3 models
ClearBedModal—web/src/editor/ClearBedModal.tsx:26(viaweb/src/components/ui/ConfirmModal.tsx).useClearObjectstill carriesonError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.'))(web/src/lib/objects.ts:384), so on a clear failure the user now sees both the inlineAlertthatConfirmModalrenders and the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a glo…🪰 Gadfly · advisory
@@ -382,3 +382,3 @@},onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),// No toast: the only caller (ClearBedModal → ConfirmModal) shows the failure🟡 ClearBed failure now surfaces both a toast and ConfirmModal's inline alert
error-handling · flagged by 1 model
web/src/lib/objects.ts:384/web/src/editor/ClearBedModal.tsx—useClearObjectstill has its ownonError: (err) => toast.error(...). SinceClearBedModalnow usesConfirmModal, whosehandleConfirmalso catches the same rejection fromclear.mutateAsync(objectId)and renders it inline (ConfirmModal.tsx:52-54), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (useDeleteGarden, `useDele…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 6 findings (3 with multi-model agreement)
web/src/components/gardens/LeaveGardenModal.tsx:18web/src/editor/ClearBedModal.tsx:26web/src/editor/ClearBedModal.tsx:203 single-model findings (lower confidence)
web/src/lib/objects.ts:384web/src/components/layout/AppShell.tsx:54web/src/components/ui/ConfirmModal.tsx:33Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
This PR is a frontend-only UI refactor: extracting a shared
ConfirmModalprimitive, deleting dead scaffolding (PageStub), and widening the editor layout. I verified the newConfirmModal.tsx,Alert.tsx, andModal.tsx— nodangerouslySetInnerHTMLanywhere in the touched files (confirmed via repo-wide grep), all error/message content is rendered as plain React children (auto-escaped), anderrorMessage()only surfacesApiError.messageor a static fallback string, unchanged behavior from before the refactor. No authn/authz logic, no new API calls, no injection-relevant input handling, and no secret handling in any of the changed files. The layout/width changes inAppShell.tsxare purely CSS class changes with no security surface.Nothing in this diff falls within the security lens.
🎯 Correctness — Minor issues
This confirms the AppShell change is a straightforward semantics-preserving refactor (logic unchanged, just renamed/reused). No issue there, matching the draft's conclusion.
The single finding in the draft is fully verified against the actual code.
Corrected Review
VERDICT: Minor issues
web/src/editor/ClearBedModal.tsx:20-24(viaweb/src/lib/objects.ts:374-386,web/src/components/ui/ConfirmModal.tsx:46-56):ClearBedModalnow routes its mutation throughConfirmModal, which callsclear.mutateAsync(objectId)and catches failures to render an inlineAlert. ButuseClearObject(lib/objects.ts:384) already has its ownonError: (err) => toast.error(...)on the mutation. TanStack Query fires a mutation'sonErroroption independently of whether the caller awaitsmutateAsyncand catches the rejection, so both fire. A failed "Clear bed" now shows the error twice: once as a persistent toast (error toasts don't auto-dismiss, percomponents/ui/toast.tsx:44-49) and once as the modal's inlineAlert. Confirmed none of the other four hooks routed throughConfirmModal(useDeleteGarden,useRemoveShare,useDeletePlant,useDeleteSeedLot) has a hook-levelonError, so this duplication is specific toClearBedModal. Fix: drop theonErrortoast fromuseClearObject(let the modal own the error, consistent with the other four hooks).🧹 Code cleanliness & maintainability — Minor issues
Confirmed both findings against the actual code — line 54 is the ternary (
canvasRoute ? '' : 'mx-auto max-w-5xl'), line 98 is the&&short-circuit for the identical condition;confirmVariantis only ever assigned its default, with no caller passing'primary'anywhere inweb/src.VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:54vs:98— thenavclassName uses a ternary-to-empty-string (canvasRoute ? '' : 'mx-auto max-w-5xl') while<main>(44 lines below) uses a short-circuit (!canvasRoute && 'mx-auto max-w-5xl') for the identicalcanvasRoutecondition.cn()strips falsy values, so the ternary's''branch is unnecessary. Both compile and render identically, but two idioms for the same width-toggle in one file is exactly the inconsistency this PR is otherwise cleaning up. Suggest aligning thenavline to!canvasRoute && 'mx-auto max-w-5xl'to match<main>.web/src/components/ui/ConfirmModal.tsx:33(type declared at line 35) —confirmVariant?: 'danger' | 'primary'is exposed on the new primitive, but no caller inweb/srcpasses anything but the default ('danger'). Speculative API surface on a brand-new component, contrary to the project's "don't design for hypothetical future requirements" convention — low priority, fine to leave if the next confirm-style dialog is expected to need it soon.Both are cosmetic; nothing here blocks merge. The five
ConfirmModalconversions are faithful,PageStubremoval is clean, and the full-width editor changes correctly centralize thecanvasRoutecondition.⚡ Performance — No material issues found
VERDICT: No material issues found
This PR is a frontend-only refactor (dead code removal, layout class changes, and consolidating five duplicate confirm-dialog components into a shared
ConfirmModal). I readConfirmModal.tsx,AppShell.tsx, andModal.tsxin full, plus all five modal call sites in the diff.Through the performance lens specifically:
sections.filter(...)inAppShellwas already there and runs once per render over a 3-item array — non-issue.Modal's focus-trap effect (mount-only,[]deps) is unchanged by this PR.del.mutate(..., { onSuccess, onError })todel.mutateAsync()insideConfirmModal'sonConfirmis functionally equivalent — no extra network calls or renders introduced.PageStub.tsxis pure deletion of unused code, no runtime effect.max-w-5xl→ full-width class changes are static Tailwind class strings recomputed viacn(...)per render, same cost as before (it was already a template string with conditionals).Nothing here touches hot loops, allocation-heavy paths, network calls, or unbounded growth. Nothing to report in this lens.
🧯 Error handling & edge cases — Minor issues
Both findings verified directly against the source.
Finding 1 —
objects.ts:384confirmsuseClearObjectstill hasonError: (err) => toast.error(...), whilegardens.ts:136-138,plants.ts:112-115,seedLots.ts:98-104, andshares.ts:63-69confirm none of the other four converted hooks (useDeleteGarden,useDeletePlant,useDeleteSeedLot,useRemoveShare) define anonError.ConfirmModal.tsx:46-55confirmshandleConfirmawaitsonConfirm()in a try/catch and sets its own inlineAlerton rejection — so forClearBedModalspecifically, a failure fires both the hook-level toast and the modal's inline alert.package.json:19confirms@tanstack/react-query: ^5.62.0, wheremutateAsyncrejecting alongside a configuredonErroris standard behavior. Confirmed as described.Finding 2 —
LeaveGardenModal.tsx:18-20confirmsonConfirm={async () => { if (me.data) await remove.mutateAsync(me.data.id) }}— whenme.datais falsy this resolves without throwing.ConfirmModal.tsx:46-51confirmshandleConfirmtreats any non-throwing resolution as success and callsonClose(). Confirmed as described; also confirmed gated byconfirmDisabled={!me.data}at line 16, matching the draft's "low practical severity" framing.VERDICT: Minor issues
web/src/lib/objects.ts:384/web/src/editor/ClearBedModal.tsx—useClearObjectstill has its ownonError: (err) => toast.error(...). SinceClearBedModalnow usesConfirmModal, whosehandleConfirmalso catches the same rejection fromclear.mutateAsync(objectId)and renders it inline (ConfirmModal.tsx:52-54), a clear-bed failure now shows both a toast and the modal's inline alert for the same error. None of the other four converted modals' hooks (useDeleteGarden,useDeletePlant,useDeleteSeedLot,useRemoveShare) define anonError, so only this one double-surfaces. Fix: drop theonErrortoast fromuseClearObjectnow that the modal shows the error inline.web/src/components/gardens/LeaveGardenModal.tsx:18-20—onConfirmsilently no-ops whenme.datais falsy instead of throwing, soConfirmModal.handleConfirmtreats the no-op as success and callsonClose()as though the leave succeeded. Gated byconfirmDisabled={!me.data}(line 16) so unreachable via a normal click today, but it's a latent trap for this pattern if reused elsewhere. Low severity; worth tightening (throw or otherwise signal failure instead of resolving silently).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
I traced the security-relevant data flow through this refactor and found nothing materially wrong in my lane:
ConfirmModalsurfaces errors viaerrorMessage(err, errorFallback)→<Alert>{error}</Alert>.Alert(web/src/components/ui/Alert.tsx:7-19) renders children as React text nodes (nodangerouslySetInnerHTML), so the message is HTML-escaped.errorMessage(web/src/lib/api.ts:135-138) returns either the server-controlledApiError.messageor a static per-callererrorFallbackliteral — no untrusted client input is interpolated into the DOM. Verified by reading both files.children,title,confirmLabel, etc. are all caller-supplied static JSX/strings (garden/plant names were already rendered the same way before). No new user-controlled string is introduced into a sink.errorFallbackvalues are static literals; nothing fromApiError.body(which can hold a server row on 409) is rendered — onlyerr.message. No secret exposure.LeaveGardenModalkeeps theconfirmDisabled={!me.data}guard (web/src/components/gardens/LeaveGardenModal.tsx), so the action button stays disabled when there's no authenticated user, matching the priordisabled={remove.isPending || !me.data}.mutatewith onlyonSuccess); now it usesmutateAsyncand shows the error inline. Better failure visibility, no security regression.The
ConfirmModalbusy/disabled state is React-state-based (like the pre-existingisPendingpattern it replaces), so it's not a new double-submit vector — and that's a correctness/UX concern outside this lens anyway.🎯 Correctness — Minor issues
Verdict: Minor issues
ClearBedModalnow double-reports failures (inline Alert + toast).web/src/editor/ClearBedModal.tsx:27callsclear.mutateAsync(objectId). TheuseClearObjectmutation (web/src/lib/objects.ts:384) has anonErrorthat firestoast.error(...).mutateAsyncrejects on error, soConfirmModal'scatchALSO sets the inlineAlert(web/src/components/ui/ConfirmModal.tsx:52-54). The original code usedclear.mutate(objectId, { onSuccess: onClose })—mutatedoes not reject, and the old modal had no inline error state at all, so only the toast fired. The net effect of this PR is that a failed clear now shows both an inline Alert and the existing toast. Verified against the real source:useClearObjectkeeps itsonErrortoast, while the other four mutations (useDeleteGarden,useDeletePlant,useDeleteSeedLot,useRemoveShare) have noonError(grep confirmed no matches ingardens.ts,plants.ts,seedLots.ts,shares.ts), so this duplication is specific to the clear flow. Consistent fix: drop theonErrortoast fromuseClearObjectso the dialog owns the inline message, matching the other four confirm flows.🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed the new
ConfirmModalprimitive, the five folded dialogs, thePageStubdeletion, and theAppShellwidth change. Verified:PageStubhas zero remaining imports (grep acrossweb/src→ no matches) — clean removal.ConfirmModalconsistently; the duplicated busy-lock/try-catch/footer boilerplate is genuinely collapsed into one place (ConfirmModal.tsx:46-56).ConfirmModal'sconfirmVariantis restricted to'danger' | 'primary', which matchesButton's actualButtonVariant(Button.tsx:4) — no leaky/invalid union.AppShell.tsx:44,54,98—canvasRouteis named and reused for both the nav and main width, replacing the priorinEditor && inPublicGarden-style spread; thecn(..., canvasRoute ? '' : 'mx-auto max-w-5xl')pattern matches the surrounding conditional-class style. Brand and editor both key offpx-4, so alignment holds.One trivial note (not worth blocking):
ConfirmModal'sconfirmVariantprop (ConfirmModal.tsx:23,35) is declared and documented but none of the five callers pass it — they all rely on the'danger'default. It's a reasonable, cheap extension point and self-documenting, so I'd leave it; flagging only for completeness as speculative API surface.⚡ Performance — No material issues found
No material issues found.
Reviewed through the performance lens:
web/src/components/ui/ConfirmModal.tsx): The busy/error state moved from each caller's react-queryisPendinginto localuseState. This is render-equivalent — oneuseStatepair instead of readingmutation.isPending, no extra subscriptions or re-renders.handleConfirmawaitsonConfirm()and only resetsbusyon throw; no leaked in-flight work or unbounded growth. Verified atConfirmModal.tsx:43-56.web/src/components/layout/AppShell.tsx): The twomatchRoutecalls (inEditor,inPublicGarden) already existed pre-PR;canvasRouteis just a boolean OR of them, so no new route matching work per render. Thenav/mainnow go throughcn()(twMerge+clsx) instead of a static string — a couple of small allocations on AppShell renders, but AppShell is not a hot path and this is immaterial. Verified atAppShell.tsx:40-54,93-102andcn.ts:1-7.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/ClearBedModal.tsx:27— double error reporting on failure (toast + inline alert).useClearObject(web/src/lib/objects.ts:384) defines a mutation-levelonErrorthat firestoast.error(objectErrorMessage(...)). The PR switches fromclear.mutate(objectId, { onSuccess: onClose })(which on failure only ran the global toast) toclear.mutateAsync(objectId), butmutateAsyncdoes not suppress the mutation's globalonError— React Query still invokes it.mutateAsyncalso rejects, soConfirmModal.handleConfirm'scatchsets the inlineAlertand keeps the dialog open. Result: a failed clear now produces both a toast and an inline error in the modal. Verified that the other four mutations wrapped byConfirmModal(useDeleteGarden,useDeletePlant,useDeleteSeedLot,useRemoveShare) have no globalonError, so only ClearBed is affected. Suggested fix: drop the globalonErrortoast fromuseClearObject(the inline alert is the better UX for a modal action), since callers can't inject per-callonErroroverrides throughConfirmModal.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Security review of the changes:
ConfirmModal: The new component centralizes confirm-dialog logic but does not introduce new attack surface. User-provided text (
title,confirmLabel,busyLabel,errorFallback) is rendered as React text nodes, not HTML. Thechildrenbody is JSX passed by callers, not raw HTML strings.Error display:
errorMessageonly surfaceserr.messageforApiErrorinstances (server-generated messages) and falls back to a fixed string for everything else. The<Alert>component renders children via normal React interpolation ({children}), notdangerouslySetInnerHTML, so even a malicious server error payload cannot execute XSS.Destructive action wrappers (
DeleteGarden,LeaveGarden,DeletePlant,DeleteSeedLot,ClearBed): All continue to use the same underlying TanStack Query mutations with identical auth/authorization semantics. No new auth bypasses or injection vectors are introduced.AppShell layout change: Pure CSS/layout diff (
max-w-5xlremoval on canvas routes); no security impact.🎯 Correctness — Blocking issues found
Blocking issues found
web/src/editor/ClearBedModal.tsx:21— The refactor switchesClearBedModalfromclear.mutate()with anonSuccesscallback toclear.mutateAsync()insideConfirmModal. The underlyinguseClearObjecthook (web/src/lib/objects.ts:384) still definesonError: (err) => toast.error(...). TanStack Query runsonErrorbeforemutateAsyncrejects, so a failure now produces two notifications: the inline error fromConfirmModaland a toast from the mutation hook. Previously the error was shown once (as a toast). The fix is to remove theonErrortoast fromuseClearObjectsinceClearBedModalis its only consumer and now handles errors inline.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The refactor is clean and well-structured:
canvasRoutevariable is well-named and used consistently for both the nav width and the main content width. The conditional classNames follow existing patterns in the codebase (cnfrom@/lib/cn).PageStub.tsxwas verified as unimported before deletion.mutatewithonSuccess/onError) tomutateAsynchandled by the primitive.⚡ Performance — No material issues found
No material issues found
I checked the mutation hooks (
useDeleteGarden,useDeletePlant,useDeleteSeedLot,useRemoveShare,useClearObject) and the newConfirmModalimplementation. The refactor simply moves the per-modaluseStateforbusy/errorinto a shared component — the number of state containers and re-render boundaries is identical. The switch frommutatewith callbacks tomutateAsyncawaited inConfirmModaldoes not change the network or rendering behavior materially. No hot loops, no unbounded growth, no missing limits, no allocation regressions, and no new blocking patterns were introduced.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
LeaveGardenModalcloses on no-op whenme.datais absent.web/src/components/gardens/LeaveGardenModal.tsx:18In the old code, the guard
if (!me.data) returnkept the dialog open when there was no current user. In the refactored version,onConfirmresolves withundefinedinstead of returning early, soConfirmModal.handleConfirmproceeds to callonClose()and dismisses the dialog. The button is disabled viaconfirmDisabled={!me.data}, so this is only reachable via programmatic invocation or a race condition, but it is a behavior change in an edge case.ClearBedModalnow shows duplicate error notifications.web/src/components/ui/ConfirmModal.tsx:52(cascading fromweb/src/editor/ClearBedModal.tsx:27)useClearObjectalready has anonErrortoast handler. RefactoringClearBedModalontoConfirmModalmeans a failure now surfaces both as an inlineAlertinside the modal and as a toast from the mutation hook. The PR calls the inline surfacing a "bonus," but the user sees the same error twice. Consider suppressing the hook-level toast when the caller will surface the error inline (e.g., pass an optionalonErroroverride or usemutateAsyncwithout the hook'sonError).opencode/glm-5.2:cloud (opencode) — 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 this is a behavior-preserving UI refactor: confirm dialogs consolidated onto a
ConfirmModalprimitive, a dead component deleted, and a CSS width change. No new inputs, no authn/authz surface touched, no injection vectors (all dynamic text —garden.name,plant.name,objectName, error messages — is rendered as JSX text content, which React escapes; nodangerouslySetInnerHTML), no secret/credential handling, no deserialization, no SSRF, no new untrusted-data paths. TheonConfirmactions delegate to the same React Query mutations (useDeleteGarden,useRemoveShare, etc.) that already enforce server-side permissions; the refactor doesn't change what's called or with what arguments (verifiedModal.tsxto confirm the busy/backdrop/Escape semantics the primitive relies on are unchanged). TheerrorMessage→Alertrendering path is identical to the prior hand-rolled dialogs. Layout change is className-only.🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the diff through the correctness lens and verified the key behavior-preservation claims against the checked-out code.
ConfirmModalbusy/error semantics (web/src/components/ui/ConfirmModal.tsx:46-56): On success it callsonClose()withbusystilltrue(never reset on the success path). That's fine — the parent unmounts the modal on close, so no stuck state. On error it sets the message and resetsbusy=false, re-enabling the button for retry. Matches the prior per-dialog behavior.LeaveGardenModalno-current-user guard (web/src/components/gardens/LeaveGardenModal.tsx): Old code early-returned insideonConfirmwhen!me.data(kept dialog open) and disabled the button. New code gates viaconfirmDisabled={!me.data}and theonConfirmno-ops whenme.datais absent. Since the button is disabled,handleConfirmcan't fire, so the dialog can't close via the empty-action path. Behavior preserved. Verified by reading the file.ClearBedModalerror surfacing (web/src/editor/ClearBedModal.tsx:27): Switched fromclear.mutate(id, { onSuccess: onClose })(errors swallowed) toclear.mutateAsync(id)caught byConfirmModal. TheplopCount === 0guard is preserved asconfirmDisabled. This is the one intentional behavior improvement and is correct.DeleteSeedLotModal(web/src/components/plants/DeleteSeedLotModal.tsx):del.mutateAsyncthrows on error and is caught byConfirmModal's try/catch — equivalent to the oldonErrorcallback path.AppShellwidth logic (web/src/components/layout/AppShell.tsx:44):canvasRoute = inEditor || inPublicGardenis the same union that previously gatedshowBottomNav; reusing it for the width constraint is correct. The bottom-nav gating (!!user && !canvasRoute) is unchanged in effect. Verified by reading the file.PageStubremoval:grepforPageStubacrossweb/srcreturns no remaining references — the deletion is safe.No constants, conversions, or domain thresholds are introduced by this change, so there was nothing to re-derive.
🧹 Code cleanliness & maintainability — No material issues found
I've reviewed the diff through the code-cleanliness lens. Verdict: No material issues found.
Notes from verification:
ConfirmModalcleanly captures the repeated hand-rolled pattern (busy lock, inline error, Cancel/Confirm footer) across all five folded dialogs; each caller now contributes only its message, action, and labels. The primitive's doc comment is explicit about its scope and theModal-direct escape hatch for input-bearing dialogs, which is the right boundary.PageStub.tsxdeletion is safe — grep confirms no remaining references inweb/src.DeleteSeedLotModallost itsgap-3forgap-4(the new primitive usesgap-4), a deliberate normalisation, not a regression.AppShell.tsxwidth logic is readable:canvasRouteis introduced once and used in both the nav and main, with comments explaining the why. Thecn(...)ternary for the nav (canvasRoute ? '' : 'mx-auto max-w-5xl') is slightly less tidy than the&&conditional used inmain, but it's a minor stylistic inconsistency, not material.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this is a UI cleanup PR (component refactor + dead-file deletion + layout width change). No new queries, loops, allocations, or hot-path work introduced. The
ConfirmModalreplaces per-calleruseState+ try/catch with the same local state lifted into a shared component — no extra renders or allocations beyond what the originals did. TheAppShellwidth change is a className conditional, negligible.PageStubdeletion is pure dead-code removal.🧯 Error handling & edge cases — Minor issues
I'll verify both findings against the actual code.
Let me verify the other mutation hooks to confirm ClearBed is unique in keeping an onError toast.
Both findings are confirmed by reading the actual code.
Finding 1 confirmed:
useClearObject(objects.ts:384) keepsonError: (err) => toast.error(...), whileuseDeleteGarden,useDeletePlant,useDeleteSeedLot, anduseRemoveSharehave noonErrortoast.ClearBedModalnow usesConfirmModal, which renders an inlineAlerton throw — so a clear failure produces both the toast and the inline alert.Finding 2 confirmed:
LeaveGardenModal(line 18-20) guardsonConfirmwithif (me.data)and otherwise resolves without throwing, soConfirmModal.handleConfirmproceeds toonClose(). The button's disabled state and the guard both key offme.datain the same render, so it's currently unreachable but latent.VERDICT: Minor issues
Double error reporting in
ClearBedModal—web/src/editor/ClearBedModal.tsx:26(viaweb/src/components/ui/ConfirmModal.tsx).useClearObjectstill carriesonError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.'))(web/src/lib/objects.ts:384), so on a clear failure the user now sees both the inlineAlertthatConfirmModalrenders and the toast from the mutation. This is the only one of the five migrated confirmations whose underlying hook keeps a global error side effect —useDeleteGarden(gardens.ts:133),useDeletePlant(plants.ts:110),useDeleteSeedLot(seedLots.ts:97), anduseRemoveShare(shares.ts:61) all have noonError. Cosmetic noise on failure, not a swallowed error; fix is to drop theonErrortoast fromuseClearObjectsince the inline alert is the better UX this PR introduces.LeaveGardenModalsilently no-ops whenme.datais missing —web/src/components/gardens/LeaveGardenModal.tsx:18. TheonConfirmguards withif (me.data) await remove.mutateAsync(...)and otherwise resolves, soConfirmModalcloses the dialog as if the leave succeeded. The confirm button is disabled byconfirmDisabled={!me.data}using the same value in the same render, so the path is currently unreachable; this is a latent fragility only (a future change decouplingconfirmDisabledfromme.datawould silently makeonConfirma no-op-that-closes). No fix required now.opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
ConfirmModalprimitive, deletes a deadPageStubcomponent, and tweaks layout CSS for full-width canvas routes.garden.name,plant.name,objectName,lot.vendor,lot.packedForYear) is rendered through React JSX, which auto-escapes HTML.dangerouslySetInnerHTMLis not used anywhere in the new or changed files.errorMessagehelper returns server text that is rendered inside<Alert>{error}</Alert>as React children; it is not injected as raw HTML.🎯 Correctness — No material issues found
No material issues found
onConfirm, catches errors to show inline, and only callsonCloseon success.setBusy(false)in the catch path keeps the dialog open for retry, which is the intended behavior.mutateAsync(Promise-returning, throws on error) intoConfirmModal.onConfirminstead of the oldmutate+onSuccess/onErrorcallbacks. This is semantically equivalent and required for the modal'stry/awaitto work.confirmDisabled={!me.data}disables the button when the user isn't loaded; theonConfirmalso early-returns if!me.data. Behaviorally equivalent to the olddisabled={... || !me.data}plusif (!me.data) return.matchRoute({ to: '/gardens/$gardenId', fuzzy: false })andmatchRoute({ to: '/g/$token', fuzzy: false })exactly match the route definitions inrouter.tsx(garden editor and public garden respectively).fuzzy: falsecorrectly excludes the parent/gardenslist.web/srctree.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Wait — one of the refactored modals regresses into a duplicate error-reporting path.
useClearObject(web/src/lib/objects.ts:384) already has anonErrorhandler that callstoast.error(...)with the message "Could not clear the bed."ConfirmModalcatches the rejectedmutateAsyncand also renders that same error inline inside the dialog.The other four refactored modals are fine because their underlying hooks (
useDeleteGarden,useDeletePlant,useRemoveShare,useDeleteSeedLot) do not toast on error. OnlyuseClearObjectdoes.Suggested fix: Remove the
onErrortoast fromuseClearObject(or suppress it when called viamutateAsyncinside a modal). SinceConfirmModalnow owns the inline error for confirmations, the hook-level toast is the one that should go.⚡ Performance — No material issues found
Verdict: No material issues found
This PR is a frontend-only refactor with no queries, no loops, no unbounded growth, and no hot-path changes. From a performance lens, it is clean.
busy/errorstates are bounded and only mutate on user interaction (confirm click), which is the same lifecycle as the per-modal states it replaces.max-w-5xltoggles); thematchRoutecalls existed before and are not in a render-critical path.🧯 Error handling & edge cases — No material issues found
No material issues found
ConfirmModal; none swallow errors anymore.ClearBedModalis explicitly improved (it previously usedmutatewith onlyonSuccess, discarding failures).remove.mutateAsync(me.data.id)withif (me.data)inside theonConfirmcallback (line 19), so it no-ops gracefully whenme.datais absent rather than dereferencing null. This matches the old behavior.errortonullon each confirm attempt (line 47), setsbusy = truebefore awaiting (line 48), and on throw sets the error andbusy = false(lines 53-54), leaving the dialog open for retry. TheModalcomponent itself suppressesEscape/backdrop-close whilebusyis true, so the in-flight state is protected from accidental dismissal.plopCount === 0guard onClearBedModal(line 25) remains intact.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.