Polish: imperial, clear-bed, keyboard nudging, empty states (#18) #37

Merged
steve merged 2 commits from phase-7-polish into main 2026-07-19 04:44:26 +00:00
Owner

Closes #18. Part of epic #20 (Phase 7 — Polish). A grab-bag day-to-day pass on top of the core loop (#15).

What

  • Clear bed — a "Clear (N)" action on a focused plantable object → ClearBedModal confirm ("Remove all N plants from …?") → useClearObject soft-removes every active plop (a loop of PATCHes, one invalidate). Rows kept with removed_at, so it survives reload. (A bulk ClearObject endpoint arrives with #19; this uses the loop until then.)
  • Keyboard nudging (desktop) — arrows move the selected object/plop 1cm, Shift+arrows 10cm; the PATCH is debounced ~400ms on key-idle so held keys don't spam. Plops nudge in their object's local frame and clamp to its bounds; ignored while typing in a field.
  • Empty states — hint to place a first bed on an empty field, and to add a plant in an empty focused bed (non-interactive overlays). ("No gardens" / "No plants match" already existed.)
  • Paper cuts — per-route document titles (usePageTitle on every page), an emoji favicon, and a 404 catch-all route (NotFound) rendered inside the app shell.
  • Mobile — plop tap targets stay ≥~44px at low zoom via a transparent hit circle (handles already scale with 1/scale; touch-action: none was already set).
  • Hoisted the ownership check to the authoritative ownerId == me.id so the nudge handler can gate on canEdit before the loading early-returns.

Notes / choices

  • Imperial coverage verified end-to-end (inspectors, garden form, plop radius, plant spacing, card dimensions, picker) — all route through the unit-aware helpers; no metric leaks. I kept the conversion helpers in lib/units.ts (their home since #8) rather than churning them into geometry.ts as the brief loosely suggested — they're already consolidated.
  • Deep mobile-sheet work (drag-to-dismiss / snap points) is left for a real-device pass; the code-level ergonomics (hit areas, touch-action, thumb-reachable toolbar) are in.

Checks

tsc --noEmit clean · 24/24 vitest · vite build green.

Acceptance criteria

  • Clear-bed empties a bed after confirm and survives reload; nudging moves precisely (1/10cm) and persists.
  • An imperial garden never leaks a metric value; a metric garden never shows ft/in.
  • Empty states guide the first bed / first plant.

Worth a real-phone walkthrough of create → bed → plant → share-view for touch feel (not coverable by type/unit checks).

🤖 Generated with Claude Code

Closes #18. Part of epic #20 (Phase 7 — Polish). A grab-bag day-to-day pass on top of the core loop (#15). ## What - **Clear bed** — a **"Clear (N)"** action on a focused plantable object → `ClearBedModal` confirm ("Remove all N plants from …?") → `useClearObject` soft-removes every active plop (a loop of PATCHes, one invalidate). Rows kept with `removed_at`, so it survives reload. (A bulk `ClearObject` endpoint arrives with #19; this uses the loop until then.) - **Keyboard nudging (desktop)** — arrows move the selected object/plop **1cm**, Shift+arrows **10cm**; the PATCH is **debounced ~400ms** on key-idle so held keys don't spam. Plops nudge in their object's **local frame** and clamp to its bounds; ignored while typing in a field. - **Empty states** — hint to place a first bed on an empty field, and to add a plant in an empty focused bed (non-interactive overlays). ("No gardens" / "No plants match" already existed.) - **Paper cuts** — per-route **document titles** (`usePageTitle` on every page), an emoji **favicon**, and a **404 catch-all** route (`NotFound`) rendered inside the app shell. - **Mobile** — plop tap targets stay **≥~44px** at low zoom via a transparent hit circle (handles already scale with `1/scale`; `touch-action: none` was already set). - Hoisted the ownership check to the authoritative `ownerId == me.id` so the nudge handler can gate on `canEdit` before the loading early-returns. ## Notes / choices - **Imperial coverage** verified end-to-end (inspectors, garden form, plop radius, plant spacing, card dimensions, picker) — all route through the unit-aware helpers; no metric leaks. I kept the conversion helpers in **`lib/units.ts`** (their home since #8) rather than churning them into `geometry.ts` as the brief loosely suggested — they're already consolidated. - Deep mobile-sheet work (drag-to-dismiss / snap points) is left for a real-device pass; the code-level ergonomics (hit areas, touch-action, thumb-reachable toolbar) are in. ## Checks `tsc --noEmit` clean · 24/24 vitest · `vite build` green. ## Acceptance criteria - ✅ Clear-bed empties a bed after confirm and survives reload; nudging moves precisely (1/10cm) and persists. - ✅ An imperial garden never leaks a metric value; a metric garden never shows ft/in. - ✅ Empty states guide the first bed / first plant. > Worth a real-phone walkthrough of create → bed → plant → share-view for touch feel (not coverable by type/unit checks). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-19 04:22:53 +00:00
Polish: clear-bed, keyboard nudging, empty states, titles, 404 (#18)
Build image / build-and-push (push) Successful in 7s
Gadfly review (reusable) / review (pull_request) Successful in 12m58s
Adversarial Review (Gadfly) / review (pull_request) Successful in 12m58s
73fa34cb81
- Clear bed: a "Clear (N)" action on a focused plantable object → confirm modal
  (ClearBedModal) → soft-removes every active plop (useClearObject loops PATCHes,
  invalidates once); rows kept with removed_at.
- Keyboard nudging (desktop): arrows move the selected object/plop 1cm, Shift
  10cm; the PATCH is debounced ~400ms on key-idle. Plops nudge in the object's
  local frame and clamp to its bounds; ignored while typing in a field.
- Empty states: hint to place a first bed on an empty field, and to add a plant
  in an empty focused bed (both non-interactive overlays).
- Paper cuts: per-route document titles (usePageTitle on every page), an emoji
  favicon, and a 404 catch-all route (NotFound) inside the app shell.
- Mobile: plop tap targets stay ≥~44px at low zoom via a transparent hit circle.
- Ownership check hoisted to the authoritative ownerId==me so the nudge handler
  can gate on canEdit before the loading early-returns.

Imperial coverage verified across inspectors/forms/cards/picker (all via the
unit-aware helpers); conversion helpers stay in lib/units.ts (their home since #8).

tsc --noEmit clean; 24/24 vitest; production build green.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-19 04:35:51Z

claude-code/sonnet · claude-code — done

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

glm-5.2:cloud · ollama-cloud — done

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

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

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

opencode/glm-5.2:cloud · opencode — done

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

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

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

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

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

🪰 Gadfly consensus review — 15 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** — 15 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +12,4 @@
</p>
<h1 className="text-lg font-semibold text-fg">Page not found</h1>
<p className="text-sm text-muted">That page doesn't exist the link may be wrong or the page moved.</p>
<Link to="/gardens" className={buttonClasses('primary')}>

🟡 404 recovery link has no navigation-fallback error handling

error-handling · flagged by 1 model

  • buttonClasses('primary') on <Link> discards navigation semanticsweb/src/components/NotFound.tsx:15 Wrapping a TanStack Router <Link> with buttonClasses applies the visual style, but if buttonClasses ever gains type="button" or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fa…

🪰 Gadfly · advisory

🟡 **404 recovery link has no navigation-fallback error handling** _error-handling · flagged by 1 model_ - **`buttonClasses('primary')` on `<Link>` discards navigation semantics** — `web/src/components/NotFound.tsx:15` Wrapping a TanStack Router `<Link>` with `buttonClasses` applies the visual style, but if `buttonClasses` ever gains `type="button"` or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fa… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +32,4 @@
type="button"
variant="danger"
disabled={clear.isPending}
onClick={() => clear.mutate(plops, { onSuccess: onClose })}

🟠 Clear-bed button not disabled for empty plops array, causing no-op confirm

error-handling · flagged by 1 model

  • Unchecked empty-array guard in ClearBedModalweb/src/editor/ClearBedModal.tsx:35 If plops is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), clear.mutate([]) fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, useClearObject loops over the array with Promise.all; an empty array resolves immediately, making the UX misleading…

🪰 Gadfly · advisory

🟠 **Clear-bed button not disabled for empty plops array, causing no-op confirm** _error-handling · flagged by 1 model_ - **Unchecked empty-array guard in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:35` If `plops` is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), `clear.mutate([])` fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, `useClearObject` loops over the array with `Promise.all`; an empty array resolves immediately, making the UX misleading… <sub>🪰 Gadfly · advisory</sub>
@@ -266,0 +266,4 @@
/** Clear a bed: soft-remove every active plop in an object (a loop of PATCHes;
* a bulk ClearObject endpoint arrives with the agent seam, #19). Invalidates
* once at the end. Pass the object's active plops (id + current version). */
export function useClearObject(gardenId: number) {

🔴 useClearObject lacks optimistic update and partial-failure handling

correctness, error-handling, maintainability · flagged by 4 models

  • web/src/lib/objects.ts:269-281useClearObject has no optimistic update and no partial-failure handling Promise.all patches each plop individually. If one request fails, onError fires but onSuccess does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike useRemovePlanting). Fix: Add an onMutate that filters the plops from the cache…

🪰 Gadfly · advisory

🔴 **useClearObject lacks optimistic update and partial-failure handling** _correctness, error-handling, maintainability · flagged by 4 models_ - **`web/src/lib/objects.ts:269-281` — `useClearObject` has no optimistic update and no partial-failure handling** `Promise.all` patches each plop individually. If one request fails, `onError` fires but `onSuccess` does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike `useRemovePlanting`). **Fix:** Add an `onMutate` that filters the plops from the cache… <sub>🪰 Gadfly · advisory</sub>
@@ -266,0 +271,4 @@
return useMutation({
mutationFn: async (plops: { id: number; version: number }[]) => {
const today = new Date().toISOString().slice(0, 10)
await Promise.all(

🔴 Promise.all is all-or-nothing: successfully soft-removed rows are lost from the cache when one PATCH rejects, because onSuccess (the only invalidate) never runs

error-handling, performance · flagged by 4 models

  • web/src/lib/objects.ts:274-276Promise.all is all-or-nothing; partial success is silently lost. Verified: the mutation only invalidates on onSuccess (line 278), so a single rejected PATCH throws away the fact that the others already soft-removed their rows server-side. The cache remains stale for the successfully-removed plops until the next external refetch. Same location as above; the fix (per-row useRemovePlanting, or Promise.allSettled + reconcile + invalidate-on-failure) c…

🪰 Gadfly · advisory

🔴 **Promise.all is all-or-nothing: successfully soft-removed rows are lost from the cache when one PATCH rejects, because onSuccess (the only invalidate) never runs** _error-handling, performance · flagged by 4 models_ - **`web/src/lib/objects.ts:274-276` — `Promise.all` is all-or-nothing; partial success is silently lost.** Verified: the mutation only invalidates on `onSuccess` (line 278), so a single rejected PATCH throws away the fact that the others already soft-removed their rows server-side. The cache remains stale for the successfully-removed plops until the next external refetch. Same location as above; the fix (per-row `useRemovePlanting`, or `Promise.allSettled` + reconcile + invalidate-on-failure) c… <sub>🪰 Gadfly · advisory</sub>
@@ -266,0 +275,4 @@
plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version })),
)
},
onSuccess: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),

🟠 useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure

correctness, error-handling · flagged by 2 models

  • web/src/lib/objects.ts:278useClearObject fires a Promise.all of soft-remove PATCHes but has no optimistic onMutate and only invalidates on onSuccess. If one PATCH fails (e.g., version conflict) while others succeed, onError fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes. Fix: add an `onMutat…

🪰 Gadfly · advisory

🟠 **useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure** _correctness, error-handling · flagged by 2 models_ * `web/src/lib/objects.ts:278` — `useClearObject` fires a `Promise.all` of soft-remove PATCHes but has **no optimistic `onMutate`** and only invalidates on `onSuccess`. If one PATCH fails (e.g., version conflict) while others succeed, `onError` fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes. **Fix:** add an `onMutat… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +3,4 @@
/** Set the document title for the current route (suffixed with · pansy), and
* restore the previous title on unmount. */
export function usePageTitle(title: string): void {
useEffect(() => {

🟠 Page title restoration races on rapid route changes, can clobber active title

correctness, error-handling, maintainability · flagged by 3 models

  • web/src/lib/usePageTitle.ts:6-12usePageTitle restore-on-unmount can restore a stale route title across SPA navigations (minor). On mount it captures prev = document.title (the previous route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures prev = "Gardens · pansy"; when GardenEditor unmounts it restores "Gardens · pansy" instead of a neutral default, and depending on React's effect/cleanup orderi…

🪰 Gadfly · advisory

🟠 **Page title restoration races on rapid route changes, can clobber active title** _correctness, error-handling, maintainability · flagged by 3 models_ - **`web/src/lib/usePageTitle.ts:6-12` — `usePageTitle` restore-on-unmount can restore a stale route title across SPA navigations (minor).** On mount it captures `prev = document.title` (the *previous* route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures `prev = "Gardens · pansy"`; when GardenEditor unmounts it restores `"Gardens · pansy"` instead of a neutral default, and depending on React's effect/cleanup orderi… <sub>🪰 Gadfly · advisory</sub>
@@ -54,0 +60,4 @@
// Role gating, computed before the effects/returns so the nudge handler can use
// it. Ownership is the authoritative ownerId==me check.
const gd = full.data?.garden
const canEdit = gd != null && (gd.ownerId === me.data?.id || gd.myRole === 'editor')

canEdit and isOwner use inconsistent null-check idioms for the same ownerId check

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **canEdit and isOwner use inconsistent null-check idioms for the same ownerId check** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +123,4 @@
// Desktop keyboard nudging: arrows move the selected object/plop 1cm (Shift =
// 10cm); the PATCH is debounced ~400ms on key-idle so a held key doesn't spam.
// Plops nudge in their object's local frame and clamp to its bounds.
useEffect(() => {

🔴 Nudge timer cancelled by re-renders due to unstable useEffect deps

correctness, error-handling, performance · flagged by 3 models

  • web/src/pages/GardenEditorPage.tsx:126-186 — The keyboard-nudge useEffect depends on [canEdit, objects, plantings]. Both objects and plantings are fresh arrays created by useMemo on every change to the underlying server data (even identical data, because React Query’s structural sharing only preserves subtree references when the data is unchanged, and any concurrent edit or background refetch can still produce new top-level array references). The effect’s cleanup function unconditi…

🪰 Gadfly · advisory

🔴 **Nudge timer cancelled by re-renders due to unstable useEffect deps** _correctness, error-handling, performance · flagged by 3 models_ * `web/src/pages/GardenEditorPage.tsx:126-186` — The keyboard-nudge `useEffect` depends on `[canEdit, objects, plantings]`. Both `objects` and `plantings` are fresh arrays created by `useMemo` on every change to the underlying server data (even identical data, because React Query’s structural sharing only preserves subtree references when the data is unchanged, and any concurrent edit or background refetch can still produce new top-level array references). The effect’s cleanup function unconditi… <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +131,4 @@
ArrowLeft: [-1, 0],
ArrowRight: [1, 0],
}
const commitLater = (fire: () => void) => {

🔴 Keyboard nudge debounce races with drag gestures, can revert drag results

correctness · flagged by 1 model

  • Keyboard nudge debounce races with drag gesturesweb/src/pages/GardenEditorPage.tsx:134-186 The 400ms debounced PATCH from arrow-key nudging captures the nudged position in a closure (next). If the user starts and finishes a drag gesture (object move in SelectionOverlay or plop move in PlopOverlay) before that timer fires, both PATCHes fly with the same stale version. Whichever reaches the server second 409s; depending on network ordering the drag result can be silently lost an…

🪰 Gadfly · advisory

🔴 **Keyboard nudge debounce races with drag gestures, can revert drag results** _correctness · flagged by 1 model_ * **Keyboard nudge debounce races with drag gestures** — `web/src/pages/GardenEditorPage.tsx:134-186` The 400ms debounced PATCH from arrow-key nudging captures the nudged position in a closure (`next`). If the user starts and finishes a drag gesture (object move in `SelectionOverlay` or plop move in `PlopOverlay`) before that timer fires, both PATCHes fly with the same stale `version`. Whichever reaches the server second 409s; depending on network ordering the drag result can be silently lost an… <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +145,4 @@
if (el && (el.tagName === 'INPUT' || el.tagName === 'TEXTAREA' || el.tagName === 'SELECT')) return
const s = useEditorStore.getState()
const step = e.shiftKey ? 10 : 1
if (s.selectedId != null) {

🟡 Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure

maintainability · flagged by 3 models

  • web/src/pages/GardenEditorPage.tsx:148-178 — the keyboard-nudge onKey handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve base from live-state-or-list, e.preventDefault(), compute next, call the matching setLive*, then commitLater(() => { mutate(...); resetLive() }). Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook…

🪰 Gadfly · advisory

🟡 **Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure** _maintainability · flagged by 3 models_ - `web/src/pages/GardenEditorPage.tsx:148-178` — the keyboard-nudge `onKey` handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve `base` from live-state-or-list, `e.preventDefault()`, compute `next`, call the matching `setLive*`, then `commitLater(() => { mutate(...); resetLive() })`. Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook… <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +151,4 @@
e.preventDefault()
const next = { ...base, xCm: base.xCm + dir[0] * step, yCm: base.yCm + dir[1] * step }
s.setLiveObject(next)
commitLater(() => {

🔴 Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture

error-handling · flagged by 1 model

  • web/src/pages/GardenEditorPage.tsx:154 — Debounced nudge timer can clear a newer live gesture The commitLater closure calls setLiveObject(null) / setLivePlanting(null) when it fires, without checking whether liveObject/livePlanting is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position. Fix: In the timer…

🪰 Gadfly · advisory

🔴 **Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture** _error-handling · flagged by 1 model_ - **`web/src/pages/GardenEditorPage.tsx:154` — Debounced nudge timer can clear a newer live gesture** The `commitLater` closure calls `setLiveObject(null)` / `setLivePlanting(null)` when it fires, without checking whether `liveObject`/`livePlanting` is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position. **Fix:** In the timer… <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +165,4 @@
const obj = objects.find((o) => o.id === base.objectId)
let nx = base.xCm + dir[0] * step
let ny = base.yCm + dir[1] * step
if (obj) {

🔴 Nudge clamping ignores object rotation, letting plops escape rotated beds

error-handling · flagged by 1 model

  • Boundary / off-by-one in nudge clamping for non-square objectsweb/src/pages/GardenEditorPage.tsx:168-171 Plops are clamped to [-widthCm/2, widthCm/2] and [-heightCm/2, heightCm/2]. If the object has been rotated (rotationDeg != 0), the plop’s local xCm/yCm frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed. Fix: apply the inverse rotation to the plop co…

🪰 Gadfly · advisory

🔴 **Nudge clamping ignores object rotation, letting plops escape rotated beds** _error-handling · flagged by 1 model_ - **Boundary / off-by-one in nudge clamping for non-square objects** — `web/src/pages/GardenEditorPage.tsx:168-171` Plops are clamped to `[-widthCm/2, widthCm/2]` and `[-heightCm/2, heightCm/2]`. If the object has been rotated (`rotationDeg != 0`), the plop’s local `xCm/yCm` frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed. **Fix:** apply the inverse rotation to the plop co… <sub>🪰 Gadfly · advisory</sub>
@@ -111,0 +180,4 @@
window.addEventListener('keydown', onKey)
return () => {
window.removeEventListener('keydown', onKey)
if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current)

🔴 Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position

correctness, error-handling, performance · flagged by 2 models

  • web/src/pages/GardenEditorPage.tsx:186 — The keyboard nudge useEffect lists objects and plantings in its dependency array. Because patchFullCache (called by useUpdateObject / useUpdatePlanting optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global keydown listener repeatedly during normal editing. The previous effect’s cleanup clears nudgeTimer.current, so if any mutation patches the garden cache while a…

🪰 Gadfly · advisory

🔴 **Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position** _correctness, error-handling, performance · flagged by 2 models_ - **`web/src/pages/GardenEditorPage.tsx:186`** — The keyboard nudge `useEffect` lists `objects` and `plantings` in its dependency array. Because `patchFullCache` (called by `useUpdateObject` / `useUpdatePlanting` optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global `keydown` listener repeatedly during normal editing. The previous effect’s cleanup clears `nudgeTimer.current`, so if **any** mutation patches the garden cache while a… <sub>🪰 Gadfly · advisory</sub>
@@ -190,1 +272,4 @@
<GardenCanvas garden={garden} objects={objects} plantings={plantings} plantsById={plantsById} canEdit={canEdit} />
{/* Empty-state hints (non-interactive overlays). */}
{canEdit && focusedObjectId == null && objects.length === 0 && (

🟡 Empty-state overlay markup duplicated inline; extract a small EmptyHint

maintainability · flagged by 2 models

  • web/src/pages/GardenEditorPage.tsx:275-288 — empty-state overlay markup is duplicated inline. Two near-identical <div className="pointer-events-none absolute ..."><p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">…</p></div> blocks differ only in positioning class and message. A tiny <EmptyHint className=…>…</EmptyHint> (or even a shared className constant for the <p>) would dedupe.

🪰 Gadfly · advisory

🟡 **Empty-state overlay markup duplicated inline; extract a small EmptyHint** _maintainability · flagged by 2 models_ - **`web/src/pages/GardenEditorPage.tsx:275-288` — empty-state overlay markup is duplicated inline.** Two near-identical `<div className="pointer-events-none absolute ..."><p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">…</p></div>` blocks differ only in positioning class and message. A tiny `<EmptyHint className=…>…</EmptyHint>` (or even a shared `className` constant for the `<p>`) would dedupe. <sub>🪰 Gadfly · advisory</sub>
@@ -232,0 +330,4 @@
{clearing && focusedObject && (
<ClearBedModal
objectName={focusedObject.name || kindDef(focusedObject.kind)?.label || 'bed'}

🟡 Focused-object fallback-name expression duplicated with inconsistent literal ('Object' vs 'bed')

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Focused-object fallback-name expression duplicated with inconsistent literal ('Object' vs 'bed')** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

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

Finding Where Models Lens
🔴 useClearObject lacks optimistic update and partial-failure handling web/src/lib/objects.ts:269 4/5 correctness, error-handling, maintainability
🔴 Promise.all is all-or-nothing: successfully soft-removed rows are lost from the cache when one PATCH rejects, because onSuccess (the only invalidate) never runs web/src/lib/objects.ts:274 4/5 error-handling, performance
🔴 Nudge timer cancelled by re-renders due to unstable useEffect deps web/src/pages/GardenEditorPage.tsx:126 3/5 correctness, error-handling, performance
🟠 Page title restoration races on rapid route changes, can clobber active title web/src/lib/usePageTitle.ts:6 3/5 correctness, error-handling, maintainability
🟡 Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure web/src/pages/GardenEditorPage.tsx:148 3/5 maintainability
🔴 Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position web/src/pages/GardenEditorPage.tsx:183 2/5 correctness, error-handling, performance
🟠 useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure web/src/lib/objects.ts:278 2/5 correctness, error-handling
🟡 Empty-state overlay markup duplicated inline; extract a small EmptyHint web/src/pages/GardenEditorPage.tsx:275 2/5 maintainability
7 single-model findings (lower confidence)
Finding Where Model Lens
🔴 Keyboard nudge debounce races with drag gestures, can revert drag results web/src/pages/GardenEditorPage.tsx:134 kimi-k2.6:cloud correctness
🔴 Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture web/src/pages/GardenEditorPage.tsx:154 kimi-k2.6:cloud error-handling
🔴 Nudge clamping ignores object rotation, letting plops escape rotated beds web/src/pages/GardenEditorPage.tsx:168 opencode/kimi-k2.6:cloud error-handling
🟠 Clear-bed button not disabled for empty plops array, causing no-op confirm web/src/editor/ClearBedModal.tsx:35 opencode/kimi-k2.6:cloud error-handling
🟡 404 recovery link has no navigation-fallback error handling web/src/components/NotFound.tsx:15 opencode/kimi-k2.6:cloud error-handling
🟡 Focused-object fallback-name expression duplicated with inconsistent literal ('Object' vs 'bed') web/src/pages/GardenEditorPage.tsx:333 claude-code/sonnet maintainability
canEdit and isOwner use inconsistent null-check idioms for the same ownerId check web/src/pages/GardenEditorPage.tsx:63 glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found

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

🔒 Security — No material issues found

Good — GardenFull enforces role checks server-side, so plops surfaced in the client are already properly scoped. Nothing new in this PR bypasses that.

I've verified the security-relevant surface of this diff (client-side canEdit gating vs. server-side objectForRole enforcement, the clear-bed PATCH loop, document.title assignment with user-controlled garden names, and the static favicon). Nothing here weakens authorization or introduces injection/unsafe-data-handling — the client checks are pure UX, and the server independently re-validates every mutation.

VERDICT: No material issues found

This PR is almost entirely client-side UI polish (page titles, favicon, 404 route, empty-state overlays, larger tap targets, keyboard nudging, a clear-bed modal). I traced the two places that touch data/authz:

  • useClearObject (web/src/lib/objects.ts:266-280) loops per-plop PATCH /plantings/:id. Verified server-side (internal/service/plantings.go:114-160) that UpdatePlanting independently calls objectForRole(..., roleEditor) for every single request — a client bypassing/spoofing the loop (e.g. calling the endpoint directly with someone else's planting IDs) gains nothing; authorization isn't delegated to the frontend loop.
  • Keyboard-nudge canEdit gating (web/src/pages/GardenEditorPage.tsx:126-190) only controls whether the listener fires client-side; the actual PATCH still goes through the same server-enforced objectForRole check as any other edit path, so hoisting/relying on the client canEdit value doesn't create a new privilege-escalation path.
  • document.title assignment with user-controlled garden names (usePageTitle, GardenEditorPage.tsx:29).title is a plain string property, not HTML-parsed, so no XSS vector even with an arbitrary garden name.
  • Favicon is a static, hardcoded data URI in index.html, not derived from any input.
  • objectErrorMessage (web/src/lib/objects.ts:381) only surfaces ApiError.message, which is the server's own curated error string, not raw exception detail — no new leak introduced by the clear-bed error path.

No injection, SSRF, deserialization, secret-leakage, or missing-validation issues found in this diff.

🎯 Correctness — Blocking issues found

VERDICT: Blocking issues found

  • web/src/pages/GardenEditorPage.tsx:126-186 — Debounced keyboard-nudge commit is discarded on effect cleanup, silently dropping the edit while the canvas keeps showing the unsaved position. The effect's dependency array is [canEdit, objects, plantings] (line 186), and both objects (line 54) and plantings (line 56) are useMemos keyed on full.data?.objects/full.data?.plantings. Any other in-flight mutation — useUpdateObject's onMutate/onSuccess/onError (objects.ts:132-160) or useUpdatePlanting's (objects.ts:231-262) — replaces the corresponding array via patchFullCache's .map(), producing a new reference and re-running this effect. The cleanup (lines 181-184) only does clearTimeout on the pending 400ms commit; it never fires it and never resets liveObject/livePlanting. Sequence: nudge object A (setLiveObject(next) fires immediately at line 153, PATCH deferred 400ms via commitLater), then within that window perform any unrelated edit that touches the objects/plantings cache — the effect tears down, the pending PATCH for A is silently canceled, and liveObject is never cleared. Since GardenCanvas.tsx:95-98 unconditionally renders liveObject in place of the matching id, object A keeps showing the nudged position for the rest of the session even though the server was never updated. Fix: flush the pending commit in the cleanup instead of discarding it, or move the debounce state outside an effect keyed to data that changes on unrelated mutations.

  • web/src/lib/objects.ts:269-281useClearObject has no reconciliation on partial failure, leaving the UI showing already-removed plops as still active. The mutation runs Promise.all over per-plop PATCHes with no onMutate optimistic update. If one PATCH rejects, Promise.all rejects immediately even though other in-flight PATCHes may still complete server-side. onError (line 279) only shows a toast — unlike every other mutation in this file (e.g. useUpdateObject's conflict-adopt at lines 147-160, useRemovePlanting's rollback at lines 298-312), it never invalidates or reconciles the cache, so the query cache still lists all plops as active even though some were soft-removed on the server. A retry from ClearBedModal (which keeps its stale plops array since the modal only closes onSuccess) resends now-stale versions for the already-cleared rows, generating further 409s. Suggest invalidating fullKey(gardenId) in onError too, so the UI reflects whatever partially succeeded.

🧹 Code cleanliness & maintainability — Minor issues

Both findings are confirmed against the actual code. Both line numbers, quoted expressions, and structural claims match exactly.

VERDICT: Minor issues

  • web/src/pages/GardenEditorPage.tsx:247 and :333 — the focused-object display name is computed with focusedObject.name || kindDef(focusedObject.kind)?.label || 'X' twice, with two different fallback literals ('Object' in the toolbar chip vs 'bed' in the ClearBedModal prop). Confirmed by reading both lines directly — same derivation duplicated with a small, easy-to-miss inconsistency. Worth hoisting into a single const focusedObjectLabel = focusedObject.name || kindDef(focusedObject.kind)?.label || 'bed' computed once and reused.

  • web/src/pages/GardenEditorPage.tsx:148-178 — the keyboard-nudge onKey handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve base from live-state-or-list, e.preventDefault(), compute next, call the matching setLive*, then commitLater(() => { mutate(...); resetLive() }). Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook fires. A small shared helper would collapse this to one implementation.

Performance — Minor issues

This confirms each plop is created by one manual tap (radius = 1.5×spacing, not area-filling), so plop count per bed equals however many times a user tapped — not something automatically "derived" via computeDerivedCount. That formula computes the display count of plants a single plop's circle represents, not the number of plops. The draft's "~80 plops via computeDerivedCount" example misreads this — a bed reaching dozens of plops requires dozens of manual taps, not one API/derivation step. The core code claim (unbounded Promise.all fan-out, no chunking helper anywhere) is independently confirmed at objects.ts:274-276, so I'm keeping the finding but correcting the reasoning.

VERDICT: Minor issues

  • web/src/lib/objects.ts:274-276 (useClearObject) — clearing a bed fires Promise.all(plops.map(...)), i.e. N concurrent PATCH requests with no batching or concurrency cap. Confirmed no chunking/concurrency-limiting helper exists anywhere in web/src (grep -rn "Promise.all|p-limit|concurrency|chunk(" turns up only this call site). There's also no cap on how many plops an object can hold (no MAX_PLOP-style guard anywhere in the editor). Note: plop count is not derived automatically from bed area/spacing via computeDerivedCount — that function only computes the display count of plants a single already-placed circle represents (lib/plantings.ts:66-69); each plop is created by one manual tap in GardenCanvas.tsx's onPlace (editor/GardenCanvas.tsx:138-151). So reaching dozens of plops in one bed requires dozens of individual manual placements, not a single planting action — a real but less automatic path to the same fan-out. The PR's own comment acknowledges this is a stopgap pending a bulk ClearObject endpoint (#19), so it's a known, accepted tradeoff; still worth capping concurrency (e.g., batches of ~6-8) for gardens that do accumulate many hand-placed plops in one bed.
🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

  • web/src/pages/GardenEditorPage.tsx:181-184 — The keyboard-nudge debounce timer is cancelled (not flushed) on unmount/dependency change:

    return () => {
      window.removeEventListener('keydown', onKey)
      if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current)
    }
    

    A nudge shows the moved position via setLiveObject/setLivePlanting (lines 153/173) as soon as the key is released, but the PATCH that persists it is deferred ~400ms via commitLater (lines 134-140). If the component unmounts inside that window (route change, exiting focus, navigating away) the timer is cleared with no flush, so mutate() never fires and the move is silently lost — on reload the object/plop reverts to its old position with no error. The effect re-runs on every objects/plantings change too (dependency array at line 186), so this can also fire mid-session, not just on unmount. Suggest flushing the pending commit instead of dropping it.

  • web/src/lib/objects.ts:266-280 (useClearObject) — the "Clear bed" loop has no partial-failure handling:

    await Promise.all(plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version })))
    ...
    onSuccess: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
    onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),
    

    Promise.all rejects on the first failing PATCH, but any PATCHes that already resolved have already soft-removed those plops server-side — that state isn't rolled back. onError doesn't invalidate/refetch (unlike useDeleteObject, which does onSettled: () => qc.invalidateQueries(...) unconditionally — confirmed at web/src/lib/objects.ts:182), so the query cache is never reconciled: the UI keeps showing all plants as present, the toast implies nothing happened, and ClearBedModal.tsx's onSuccess: onClose means the modal stays open only on failure — enabling a retry that re-PATCHes already-removed rows with stale version, producing spurious 409s. Suggest invalidating on error too (or using Promise.allSettled and reporting which failed).

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

Security review of this grab-bag polish PR, traced tainted input → sink:

  • Clear-bed authz: useClearObject loops api.patch('/plantings/${p.id}', { removedAt, version }). The p.id/version are server-issued numbers from the authenticated garden's plops (no path/query injection — numeric interpolation only). Each PATCH is server-side authorized via service.UpdatePlantingobjectForRole(actorID, …, roleEditor) (internal/service/plantings.go:119), so the client canEdit gate is genuine defense-in-depth, not the enforcement point. No authz bypass.
  • Nudging authz: gated client-side by canEdit and dispatched through useUpdateObject/useUpdatePlanting, which hit the same server role re-check. A spoofed client canEdit/myRole cannot escalate — server rejects non-editors. Verified the hoisted canEdit (gd != null && (gd.ownerId === me.data?.id || gd.myRole === 'editor')) is logically identical to the prior post-load computation; loading state (gd null) yields canEdit=false, so the handler no-ops rather than running ungated.
  • usePageTitle: assigns document.title from full.data?.garden.name (user-controlled). document.title is a plain string property, not an HTML-parsing sink, so no XSS. React escapes objectName in ClearBedModal. No credential/secret leakage in titles.
  • Favicon / NotFound: both static — the inline SVG data-URI is a hardcoded emoji, no untrusted interpolation; NotFound renders only constant strings and a /gardens link.
  • removedAt payload: client sends new Date().toISOString().slice(0,10); even if a malicious client sent an arbitrary string, it's the actor's own garden row and the server stores it as TEXT — not a cross-user injection vector, and this PR reuses the existing useRemovePlanting pattern rather than introducing a new surface.

No findings in the security lens.

🎯 Correctness — Minor issues

Verdict: Minor issues found

Investigated the useClearObject Promise.all loop against the other planting/object mutations and the usePageTitle capture/restore implementation against actual usage.

Findings

  • web/src/lib/objects.ts:269-281useClearObject partial failure leaves the cache inconsistent with the server (medium). mutationFn runs Promise.all over per-plop PATCHes. Promise.all rejects on the first failed PATCH (e.g. a 409 on one stale version), but the remaining in-flight requests still complete — so some plops are soft-removed server-side while others are not. On rejection only onError runs (a generic toast); there is no onError/onSettled invalidation, unlike useDeleteObject which invalidates in onSettled (line 182) and useRemovePlanting which reconciles via conflictPlanting in onError (lines 298-311). The cache therefore still shows every plop, but the server has removed a subset. ClearBedModal only closes on onSuccess (clear.mutate(plops, { onSuccess: onClose })), so the modal stays open on failure, and a retry re-sends the now-stale versions of already-removed rows (they'll 409/404 again). Confirmed by reading the hook (lines 269-281) and comparing to useDeleteObject (line 182) and useRemovePlanting (lines 292-309). Suggested fix: add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) so the cache reconciles to the server's actual state regardless of partial failure, and consider Promise.allSettled so the error message can report how many succeeded.

  • web/src/lib/usePageTitle.ts:6-12usePageTitle restore-on-unmount can restore a stale route title across SPA navigations (minor). On mount it captures prev = document.title (the previous route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures prev = "Gardens · pansy"; when GardenEditor unmounts it restores "Gardens · pansy" instead of a neutral default, and depending on React's effect/cleanup ordering during a route swap the old page's cleanup can transiently clobber the new page's title. Verified the implementation at usePageTitle.ts:6-12; usePageTitle is now called from GardensPage, GardenEditorPage, PlantsPage, LoginPage, RegisterPage, and NotFound. This is a known footgun of the capture/restore pattern in SPAs. Suggested fix: don't capture/restore — set the title on mount and reset to the app default ('pansy') on unmount, or just always set (no restore) so each route owns its title.

Verified-correct items (not findings):

  • hitR = Math.max(r, 22/scale) re-derives correctly: radius 22/scale cm × scale px/cm = 22 px radius = 44 px diameter, matching the "~44px" comment. fill="transparent" is a paint and receives pointer events under the default visiblePainted, unlike fill="none".
  • Nudge debounce: the 400ms idle timer is reset on every keypress, so exactly one PATCH fires with the accumulated final position and the original committed version — correct, no self-conflict.
  • Plop nudge clamps to ±widthCm/2, ±heightCm/2 (object-local half-extents) — correct; object nudging intentionally has no garden-bounds clamp, matching drag which also doesn't clamp.
🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

  • web/src/pages/GardenEditorPage.tsx:148-178 — The keyboard-nudge onKey handler has two near-identical branches (object vs. planting) that repeat the same shape: find base from live*/array → e.preventDefault() → compute nextsetLive*commitLater(() => { mutate(...); setLive*(null) }). The only real differences are the store setter, the mutate function, and the clamping. This is copy-paste that should be factored — e.g. a small nudge(base, { setLive, commit }) helper or a discriminated helper. As written, a change to debounce/commit logic (or adding a third entity type) must be edited in two places. Verified by reading lines 141–179.

  • web/src/lib/objects.ts:269-281 vs 283-291useClearObject reimplements the "PATCH removedAt = today" soft-remove that useRemovePlanting already encodes, including the new Date().toISOString().slice(0, 10) date formatting. The "today" computation is now duplicated in two mutations. A shared todayISO() (or a shared softRemove(id, version) patch helper) would keep the two in sync; right now a date-format change must be made in both. Verified by reading both functions.

  • web/src/pages/GardenEditorPage.tsx:275-288 — The two empty-state overlay <p> elements share the identical long class string max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur. Extracting a hintOverlay class constant (or a tiny <EmptyHint> component) would remove the duplication and make the visual treatment consistent by construction. Minor.

  • web/src/pages/GardenEditorPage.tsx:60-64 — The hoisted canEdit/isOwner block leaves isOwner recomputed twice conceptually: once here (line 64) and the prior comment about authoritative ownership was removed, but the old derived isOwner/canEdit block at the former lines 201–204 is gone (good). One nit: the new canEdit uses me.data?.id (optional chaining) while isOwner requires me.data != null, so the two "owner" checks read inconsistently — worth a one-line comment or aligning the form for future readers. Minor.

No material issues found in NotFound.tsx, usePageTitle.ts, ClearBedModal.tsx, the favicon, or PlopMarker.tsx from this lens.

Performance — Minor issues

VERDICT: Minor issues

Through the performance lens, the change is mostly clean — the nudge debounce correctly collapses a held-key stream into a single PATCH (timer reset on each keydown, one fire on idle, and the cleanup path clears any pending timer), and the live store updates are cheap synchronous Zustand sets. Two notes, both in my lane:

  • web/src/lib/objects.ts:274 — unbounded concurrent PATCH fan-out in useClearObject. Promise.all(plops.map(...)) fires one HTTP request per plop simultaneously. For a bed with many plops (dozens+) this is a burst of N concurrent requests against /plantings/:id — no batching, no concurrency limit, no optimistic cache update (unlike useRemovePlanting at objects.ts:285+, which has onMutate + cancelQueries and rollback in onError). The UI stays stale until all resolve and the single invalidateQueries refetch completes. Confirmed against the actual source: useClearObject (lines 269–281) has only onSuccess/onError, while useRemovePlanting (lines 285–312) does the optimistic filter in onMutate. The PR acknowledges a bulk ClearObject endpoint is coming in #19, so this is a known temporary seam, but as shipped it's the worst-case request pattern for the largest beds. Suggested mitigation before #19: cap concurrency (e.g. a small pLimit-style loop) or sequence the patches, and optionally mirror useRemovePlanting's optimistic onMutate filter so the bed clears instantly. Not blocking — it's a one-shot user action behind a modal, not a hot path.

  • web/src/pages/GardenEditorPage.tsx:186 — nudge effect re-subscribes on every objects/plantings change. The keydown listener is torn down and re-added whenever objects or plantings get new identities (which happens on each debounced commit, since updateObject/updatePlanting patch the full-garden cache via invalidateQueries). Cost is one removeEventListener/addEventListener per commit — negligible — so this is a non-issue in practice; flagging only because the dep choice trades a tiny re-subscription cost for correctly fresh closures. No action needed.

🧯 Error handling & edge cases — Minor issues

Both findings verified against the actual code. Let me confirm the modal behavior in ClearBedModal re: the stuck-unrecoverable claim.

The ClearBedModal passes onSuccess: onClose — so on success the modal closes. On error (onError), no onClose fires, so the modal stays open with the same stale plops (the parent's focusedPlops, computed from the still-stale cache). Re-clicking "Clear bed" re-PATCHes with old versions. That confirms the stuck-unrecoverable scenario in finding 1.

Both findings survive verification.

VERDICT: Minor issues

  • web/src/lib/objects.ts:269-281useClearObject leaves the cache stale on partial failure. mutationFn runs a Promise.all of PATCHes with no onMutate optimistic update and only onSuccess invalidates. On a partial failure (one PATCH 409/500s after others already soft-removed server-side), Promise.all rejects → onError only toasts; there is no invalidateQueries and no cache reconciliation. Result: the server has removed some plops but the FullGarden cache still lists all of them, so the editor keeps showing a full bed. Worse, ClearBedModal passes { onSuccess: onClose }, so on failure the modal stays open with the same stale plops (the parent's focusedPlops, derived from the stale cache) and stale versions; re-clicking "Clear bed" re-PATCHes already-removed rows with their old versions → 409s → the user is stuck unable to recover without a manual refetch. Verified against useRemovePlanting (lines 285-314) and useUpdatePlanting (lines 226-263), both of which reconcile via onMutate/onError, which useClearObject omits. Suggested fix: add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) (or invalidate in onError), and consider an optimistic onMutate that drops the cleared plops.

  • web/src/pages/GardenEditorPage.tsx:126-186 — nudge commit can be silently dropped, leaving the object/plop moved on screen but never persisted. The keydown effect depends on [canEdit, objects, plantings], so it re-subscribes whenever the FullGarden cache changes. The cleanup clears the pending 400ms debounce timer (nudgeTimer.current). Sequence: a nudge's timer fires → updateObject.mutate(...) and setLiveObject(null) run; if the user presses an arrow again before the PATCH resolves, the handler bases next off s.liveObject/objects, calls setLiveObject(next) and schedules a new timer; the first PATCH resolves → useUpdateObject.onSuccess (objects.ts:141-145) patches the cache → objects gets a new identity → the effect re-runs → cleanup clears the new (still-pending) timer. The second nudge is never committed, and liveObject is left at the un-persisted position (the canvas prefers liveObject), so the object appears moved but won't survive a reload. Verified by reading the hook at GardenEditorPage.tsx:126-186 and objects.ts:141-146. Suggested fix: keep the timer in a ref that survives effect cleanup (it already is in nudgeTimer, but the cleanup clears it), or move the debounce scheduling out of the effect closure / skip clearing it on re-subscription, or read base from the store's live value rather than the dep-bound objects/plantings.

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 lens: the authz gating is correct, there are no injection surfaces, no XSS vectors, and no unsafe handling of untrusted data. The clear-bed flow and keyboard-nudge handler both check canEdit before mutating, and the hoisted ownership check (ownerId === me.id) is used consistently. The usePageTitle hook assigns to document.title, which is a safe text property (not HTML-parsed). All API paths use server-derived numeric IDs, not user-controlled strings.

🎯 Correctness — Blocking issues found

Blocking issues found

  • Keyboard nudge debounce races with drag gesturesweb/src/pages/GardenEditorPage.tsx:134-186
    The 400ms debounced PATCH from arrow-key nudging captures the nudged position in a closure (next). If the user starts and finishes a drag gesture (object move in SelectionOverlay or plop move in PlopOverlay) before that timer fires, both PATCHes fly with the same stale version. Whichever reaches the server second 409s; depending on network ordering the drag result can be silently lost and the object snaps back to the stale keyboard-nudged position. The timer should be cleared when a gesture starts (e.g. by watching liveObject/livePlanting in an effect, or by clearing the ref in the gesture begin path).

  • Cross-selection-type nudge cancellationweb/src/pages/GardenEditorPage.tsx:134-186
    The single nudgeTimer ref is shared between object nudges and plop nudges. Nudging an object, then selecting a plop (or vice-versa) within the 400ms window cancels the first pending PATCH entirely; the first nudge is lost and never persisted because the new selection’s commitLater clears the old timer. The fix is to maintain separate timers (or a keyed map) so object and plop debounces are independent.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

The changes are clean and maintainable:

  • usePageTitle is a small, well-scoped hook with proper cleanup, applied consistently across all top-level route pages.
  • ClearBedModal is a focused presentational component; the useClearObject hook name follows the existing convention in lib/objects.ts.
  • Keyboard nudging logic in GardenEditorPage.tsx is contained within a single useEffect with a locally scoped DIRS table and commitLater helper — acceptable length for the behavior it encapsulates.
  • Empty-state overlays are declarative and stateless. No dead code, no duplication worth extracting, no leaky abstractions.
Performance — Minor issues

Verdict: Minor issues

Findings

  • web/src/pages/GardenEditorPage.tsx:126-186 — The keyboard-nudge effect lists [canEdit, objects, plantings] as dependencies. Both objects and plantings are useMemo-ed arrays that change reference whenever the underlying useGardenFull query data updates (even on a refetch with identical content). This causes the global keydown listener to be removed and re-added frequently, but more importantly the cleanup unconditionally runs clearTimeout(nudgeTimer.current). If a query refresh or optimistic update happens during the 400 ms debounce window, the pending PATCH is silently cancelled. The user sees the nudged position (via setLiveObject) but the server never receives it, so the next refetch snaps the item back.
    Fix: Read objects/plantings through refs (or the Zustand store) inside the handler instead of closing over them in the effect, so the effect deps reduce to [canEdit] and the timer survives data changes.
🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • web/src/pages/GardenEditorPage.tsx:154 — Debounced nudge timer can clear a newer live gesture
    The commitLater closure calls setLiveObject(null) / setLivePlanting(null) when it fires, without checking whether liveObject/livePlanting is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position.
    Fix: In the timer callback, read the current store state and only call setLiveObject(null) if liveObject?.id === next.id (and similarly for planting).

  • web/src/lib/objects.ts:269-281useClearObject has no optimistic update and no partial-failure handling
    Promise.all patches each plop individually. If one request fails, onError fires but onSuccess does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike useRemovePlanting).
    Fix: Add an onMutate that filters the plops from the cache optimistically, snapshot for rollback, and handle partial failure (e.g. Promise.allSettled) or at minimum invalidate in onSettled so successes are reflected even if one PATCH errors.

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

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

🔒 Security⚠️ could not complete

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

database is locked

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run context deadline exceeded

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • web/src/lib/objects.ts:269useClearObject diverges from the established soft-remove pattern. The sibling useRemovePlanting (lines 285–314) does optimistic cache patching (onMutate), single-row invalidation, and 409-conflict recovery via conflictPlanting. The new useClearObject does none of that: Promise.all of PATCHes, one invalidateQueries, generic toast. If one of N PATCHes 409s, Promise.all rejects partway, the cache isn't rolled back, and the user sees a generic "Could not clear the bed" while some plops are gone server-side. This creates two divergent error-handling strategies for the same soft-remove operation; the next maintainer has to reconcile them. Suggest mirroring useRemovePlanting's onMutate (filter all cleared ids) + onError (restore ctx.prev, surface conflict rows), or at least a comment explaining why bulk-clear is intentionally simpler until #19.

  • web/src/pages/GardenEditorPage.tsx:148-178 — nudge handler duplicates ~30 lines of parallel structure. The selectedId and selectedPlantingId branches are near-identical: find base, preventDefault, compute next, setLive*, commitLater(() => { update*.mutate(...); setLive*(null) }). The only real differences are the clamp-to-bounds (plops only) and which setLive/update to call. A small helper like nudge(base, dir, step, { setLive, commit, clamp? }) would collapse this and make future changes one-edit. Minor.

  • web/src/pages/GardenEditorPage.tsx:275-288 — empty-state overlay markup is duplicated inline. Two near-identical <div className="pointer-events-none absolute ..."><p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">…</p></div> blocks differ only in positioning class and message. A tiny <EmptyHint className=…>…</EmptyHint> (or even a shared className constant for the <p>) would dedupe.

  • web/src/lib/usePageTitle.ts:7-11 — restore-on-unmount relies on an undocumented ordering assumption. The JSDoc notes that the previous title is restored on unmount, but the correctness of that restore depends on route transitions happening to unmount-then-mount synchronously (so the new page captures the restored default). A future maintainer could break this by wrapping pages in a keep-alive or reordering effects; a one-line note on the assumption would prevent that. Trivial.

Performance — No material issues found

VERDICT: No material issues found

Through the performance lens:

  • Nudge debounce (GardenEditorPage.tsx:134-140): The commitLater pattern correctly cancels and reschedules a single timer on every keypress, so a held arrow key produces one PATCH after ~400ms idle rather than per-repeat requests. Verified by reading the effect — the closure captures the latest next, so only the final position is committed. This is the right shape.
  • Effect re-binding (GardenEditorPage.tsx:185-186): The keydown effect depends on [canEdit, objects, plantings], both of which are useMemo'd on server data only. Live nudge updates go through setLiveObject/setLivePlanting in the editor store, not through objects/plantings, so the listener is not torn down and re-bound on each keypress — only on server refetch (after a commit's invalidation). Not a hot-path concern.
  • focusedPlops filter (GardenEditorPage.tsx:209): Runs on every render, including during nudge-induced re-renders. It's a single Array.filter over the garden's plantings; for realistic bed sizes this is trivial and not material.
  • useClearObject parallel PATCH loop (lib/objects.ts:271-277): Fires N parallel PATCHes with one invalidation. This is explicitly called out in the PR as a stopgap pending the bulk ClearObject endpoint (#19). For typical beds this is a small burst; not a regression worth flagging under this lens.
  • PlopMarker hit circle (PlopMarker.tsx:40-51): Adds one extra <circle> per plop with fill="transparent". Constant-factor, negligible render cost; no per-frame work.

No unbounded growth, N+1 patterns, missing limits, or hot-loop regressions introduced.

🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues found

Findings (verified by reading the code):

  • web/src/lib/objects.ts:269-281useClearObject has no optimistic onMutate, no 409/conflict reconciliation, and no per-row rollback. Verified: the sibling useUpdatePlanting (lines 226-263) and useRemovePlanting (285-313) both do onMutate optimistic cache patches with a prev snapshot, handle conflictPlanting(err) to adopt the server's current row on 409, and restore ctx.prev on other errors. useClearObject (269-281) does none of this — it just fires Promise.all of PATCHes and on error shows a single generic toast. Concrete impact: if one of N PATCHes 409-conflicts (e.g. another client moved a plop and bumped its version), Promise.all rejects, the whole clear is reported as failed, but every plop that did PATCH successfully is already soft-removed server-side while the cache is never invalidated on failure (onSuccess at 278 is the only invalidate) — so the UI still shows them as present (stale) until the next external refetch, and a retry uses the same stale version for the plop that 409'd, so it will keep 409-ing. There's also no snapshot to roll back the partially-cleared cache. Suggested fix: either route through useRemovePlanting per plop (it already does the right reconciliation), or give useClearObject an onMutate that filters the cleared ids with a prev snapshot, an onError that restores ctx.prev and adopts conflictPlanting(err) for the conflicting row, plus re-fetch the plops' current versions before retry. At minimum, invalidate on failure too so the UI reconciles to the partially-applied server state.

  • web/src/lib/objects.ts:274-276Promise.all is all-or-nothing; partial success is silently lost. Verified: the mutation only invalidates on onSuccess (line 278), so a single rejected PATCH throws away the fact that the others already soft-removed their rows server-side. The cache remains stale for the successfully-removed plops until the next external refetch. Same location as above; the fix (per-row useRemovePlanting, or Promise.allSettled + reconcile + invalidate-on-failure) covers this.

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

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

🔒 Security — No material issues found

No material issues found

I traced every new code path through the security lens:

  • Authz / Clear bed: The ClearBedModal button is gated on canEdit (owner or editor). The backend UpdatePlanting handler enforces roleEditor via objectForRole for each individual PATCH, so even if the hook were called directly with cross-object IDs, every request is re-authorized server-side. No bypass.
  • Authz / Keyboard nudging: The effect returns early when !canEdit, and the same backend roleEditor guard applies to updateObject / updatePlanting. Viewers cannot nudge.
  • Input validation / Nudging: Step size is hardcoded (1 or 10 cm). Object positions are validated server-side by bboxOverlapsGarden; plop positions are client-clamped to object bounds and re-validated server-side by finalizePlanting. No injection or out-of-bounds persistence path.
  • XSS / Titles: usePageTitle sets document.title through the DOM property, which is plain-text safe. The Garden name from the server is never rendered into HTML via this path.
  • SSRF / Secrets / Injection: No new external requests, SQL, file paths, or credential handling introduced.
🎯 Correctness — Blocking issues found

Verdict: Blocking issues found

  • web/src/pages/GardenEditorPage.tsx:126-186 — The keyboard-nudge useEffect depends on [canEdit, objects, plantings]. Both objects and plantings are fresh arrays created by useMemo on every change to the underlying server data (even identical data, because React Query’s structural sharing only preserves subtree references when the data is unchanged, and any concurrent edit or background refetch can still produce new top-level array references). The effect’s cleanup function unconditionally calls window.clearTimeout(nudgeTimer.current), so any re-render that occurs during the ~400 ms debounce window silently cancels the pending PATCH. The user sees the object/plop move live, but on release it snaps back to its pre-nudge position because the commit never fires.
    Fix: hoist objects and plantings into refs (useRef) and read them inside the handler; keep the effect deps limited to [canEdit] so re-renders don’t tear down the listener and cancel the timer.

  • web/src/lib/objects.ts:278useClearObject fires a Promise.all of soft-remove PATCHes but has no optimistic onMutate and only invalidates on onSuccess. If one PATCH fails (e.g., version conflict) while others succeed, onError fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes.
    Fix: add an onMutate that filters the cleared plops from the cache immediately (mirroring useRemovePlanting), and call invalidateQueries in onSettled so a partial failure is still reconciled.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

  • Minor issues:
    • web/src/pages/GardenEditorPage.tsx:209focusedPlops is computed inline on every render and used in both JSX conditions and as props for ClearBedModal. Wrapping it in useMemo avoids unnecessary array allocations and keeps the render path predictable.
    • web/src/lib/objects.ts:269useClearObject is the only mutation in the file that skips optimistic cache updates (onMutate/onError rollback). Every sibling (useUpdatePlanting, useRemovePlanting, etc.) patches the cache immediately to prevent UI flashes; this new hook breaks that local pattern and will show a lag before the plops disappear. (Verified by reading the full objects.ts file and comparing the mutation shapes.)
Performance — Blocking issues found

Blocking issues found

  • web/src/pages/GardenEditorPage.tsx:186 — The keyboard nudge useEffect lists objects and plantings in its dependency array. Because patchFullCache (called by useUpdateObject / useUpdatePlanting optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global keydown listener repeatedly during normal editing. The previous effect’s cleanup clears nudgeTimer.current, so if any mutation patches the garden cache while a nudge debounce is pending (within the 400 ms window), the timer is killed and the PATCH is silently dropped. This is easy to hit: nudge a bed, then immediately change a plant name or create a plop—the final nudge never persists.
    Fix: remove objects/plantings from the effect deps; keep fresh refs to them (or read them from the store) so the listener stays mounted and the debounce survives unrelated cache updates.

  • web/src/lib/objects.ts:274useClearObject fires every plop PATCH concurrently via Promise.all. For a densely planted bed this creates an unbounded request burst that hammers the browser’s per-host connection limit (typically 6) and can overload the server.
    Fix: add a concurrency limit or sequential loop as a stop-gap until the bulk endpoint (#19) lands; e.g. chunk the array and await each chunk.


🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • Race between rapid nudges and version incrementsweb/src/pages/GardenEditorPage.tsx:126-186
    The keyboard-nudge effect fires updateObject.mutate / updatePlanting.mutate with the stale next.version captured at key-down time. Between the nudge and the 400 ms debounced commit, an earlier optimistic patch already bumped the cached version via applyServerPatch/applyPlantingPatch (version + 1). The debounced handler does not re-read the version, so it PATCHes the old version and 409-conflicts with itself. The user sees a spurious “updated elsewhere — your change was rolled back” toast, and the nudge is lost.
    Fix: re-read the current version from the store (or query cache) inside the commitLater callback right before calling mutate, or compute the delta relative to the latest committed state rather than capturing a snapshot.

  • Missing onSettled / optimistic rollback for useClearObjectweb/src/lib/objects.ts:269-281
    useClearObject patches planting rows in a Promise.all loop and only invalidates on success. If any individual PATCH 409s or fails, onError toasts, but the successful PATCHes in the same batch have already soft-removed those plantings server-side. The local cache is not reconciled until the next refetch, so the UI temporarily shows the cleared plops as still present (or, conversely, shows them removed while the error toast implies nothing happened). More importantly, because there is no onMutate optimistic update, the user gets no visual feedback during the network round-trip; a large bed can take seconds and look frozen.
    Fix: add onMutate to optimistically drop the plops from the cache, and in onError restore the previous FullGarden snapshot so the UI matches the server state regardless of partial failure.

  • Boundary / off-by-one in nudge clamping for non-square objectsweb/src/pages/GardenEditorPage.tsx:168-171
    Plops are clamped to [-widthCm/2, widthCm/2] and [-heightCm/2, heightCm/2]. If the object has been rotated (rotationDeg != 0), the plop’s local xCm/yCm frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed.
    Fix: apply the inverse rotation to the plop coordinates before clamping, or skip clamping until the geometry math accounts for rotation.

  • Unchecked empty-array guard in ClearBedModalweb/src/editor/ClearBedModal.tsx:35
    If plops is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), clear.mutate([]) fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, useClearObject loops over the array with Promise.all; an empty array resolves immediately, making the UX misleading.
    Fix: disable the “Clear bed” button when plops.length === 0, or bail early in the mutation with a toast.

  • Potential stale closure in page-title restorationweb/src/lib/usePageTitle.ts:5-12
    On rapid mount/unmount cycles, prev is captured synchronously in the effect body. If a parent changes document.title between the mount of two usePageTitle consumers (e.g., navigating from one route to another), the unmount of the first restores the title to the previous page’s title, clobbering the second page’s title. This is a classic race with multiple competing title effects.
    Fix: use a ref or a global stack to track title history, or at least read document.title in the cleanup function rather than closing over the initial value.

  • buttonClasses('primary') on <Link> discards navigation semanticsweb/src/components/NotFound.tsx:15
    Wrapping a TanStack Router <Link> with buttonClasses applies the visual style, but if buttonClasses ever gains type="button" or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fallback onClick handler or error boundary, leaving the user stranded on the 404 page with no recovery path.
    Fix: add an onClick fallback that calls window.location.href = '/gardens' for resilience, or wrap in an error boundary. (Low severity, but confirmed.)

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** · 15 findings (8 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | useClearObject lacks optimistic update and partial-failure handling | `web/src/lib/objects.ts:269` | 4/5 | correctness, error-handling, maintainability | | 🔴 | Promise.all is all-or-nothing: successfully soft-removed rows are lost from the cache when one PATCH rejects, because onSuccess (the only invalidate) never runs | `web/src/lib/objects.ts:274` | 4/5 | error-handling, performance | | 🔴 | Nudge timer cancelled by re-renders due to unstable useEffect deps | `web/src/pages/GardenEditorPage.tsx:126` | 3/5 | correctness, error-handling, performance | | 🟠 | Page title restoration races on rapid route changes, can clobber active title | `web/src/lib/usePageTitle.ts:6` | 3/5 | correctness, error-handling, maintainability | | 🟡 | Object-nudge and plop-nudge branches duplicate the same find-base/compute-next/setLive/commitLater structure | `web/src/pages/GardenEditorPage.tsx:148` | 3/5 | maintainability | | 🔴 | Debounced keyboard-nudge commit is discarded (not flushed) on effect cleanup, silently dropping unsaved moves while the canvas keeps showing the phantom position | `web/src/pages/GardenEditorPage.tsx:183` | 2/5 | correctness, error-handling, performance | | 🟠 | useClearObject: Promise.all rejection on one PATCH leaves other successful soft-removes uncommitted to the cache — no invalidate/reconciliation on error, so UI misreports partial failures as total failure | `web/src/lib/objects.ts:278` | 2/5 | correctness, error-handling | | 🟡 | Empty-state overlay markup duplicated inline; extract a small EmptyHint | `web/src/pages/GardenEditorPage.tsx:275` | 2/5 | maintainability | <details><summary>7 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | Keyboard nudge debounce races with drag gestures, can revert drag results | `web/src/pages/GardenEditorPage.tsx:134` | kimi-k2.6:cloud | correctness | | 🔴 | Nudge debounce timer can stomp newer liveObject/livePlanting from a subsequent drag gesture | `web/src/pages/GardenEditorPage.tsx:154` | kimi-k2.6:cloud | error-handling | | 🔴 | Nudge clamping ignores object rotation, letting plops escape rotated beds | `web/src/pages/GardenEditorPage.tsx:168` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | Clear-bed button not disabled for empty plops array, causing no-op confirm | `web/src/editor/ClearBedModal.tsx:35` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | 404 recovery link has no navigation-fallback error handling | `web/src/components/NotFound.tsx:15` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | Focused-object fallback-name expression duplicated with inconsistent literal ('Object' vs 'bed') | `web/src/pages/GardenEditorPage.tsx:333` | claude-code/sonnet | maintainability | | ⚪ | canEdit and isOwner use inconsistent null-check idioms for the same ownerId check | `web/src/pages/GardenEditorPage.tsx:63` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Good — GardenFull enforces role checks server-side, so plops surfaced in the client are already properly scoped. Nothing new in this PR bypasses that. I've verified the security-relevant surface of this diff (client-side `canEdit` gating vs. server-side `objectForRole` enforcement, the clear-bed PATCH loop, document.title assignment with user-controlled garden names, and the static favicon). Nothing here weakens authorization or introduces injection/unsafe-data-handling — the client checks are pure UX, and the server independently re-validates every mutation. ## VERDICT: No material issues found This PR is almost entirely client-side UI polish (page titles, favicon, 404 route, empty-state overlays, larger tap targets, keyboard nudging, a clear-bed modal). I traced the two places that touch data/authz: - **`useClearObject` (web/src/lib/objects.ts:266-280)** loops per-plop `PATCH /plantings/:id`. Verified server-side (`internal/service/plantings.go:114-160`) that `UpdatePlanting` independently calls `objectForRole(..., roleEditor)` for every single request — a client bypassing/spoofing the loop (e.g. calling the endpoint directly with someone else's planting IDs) gains nothing; authorization isn't delegated to the frontend loop. - **Keyboard-nudge `canEdit` gating (web/src/pages/GardenEditorPage.tsx:126-190)** only controls whether the listener fires client-side; the actual PATCH still goes through the same server-enforced `objectForRole` check as any other edit path, so hoisting/relying on the client `canEdit` value doesn't create a new privilege-escalation path. - **`document.title` assignment with user-controlled garden names (`usePageTitle`, GardenEditorPage.tsx:29)** — `.title` is a plain string property, not HTML-parsed, so no XSS vector even with an arbitrary garden name. - Favicon is a static, hardcoded data URI in `index.html`, not derived from any input. - `objectErrorMessage` (web/src/lib/objects.ts:381) only surfaces `ApiError.message`, which is the server's own curated error string, not raw exception detail — no new leak introduced by the clear-bed error path. No injection, SSRF, deserialization, secret-leakage, or missing-validation issues found in this diff. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> VERDICT: Blocking issues found - **`web/src/pages/GardenEditorPage.tsx:126-186` — Debounced keyboard-nudge commit is discarded on effect cleanup, silently dropping the edit while the canvas keeps showing the unsaved position.** The effect's dependency array is `[canEdit, objects, plantings]` (line 186), and both `objects` (line 54) and `plantings` (line 56) are `useMemo`s keyed on `full.data?.objects`/`full.data?.plantings`. Any other in-flight mutation — `useUpdateObject`'s `onMutate`/`onSuccess`/`onError` (`objects.ts:132-160`) or `useUpdatePlanting`'s (`objects.ts:231-262`) — replaces the corresponding array via `patchFullCache`'s `.map()`, producing a new reference and re-running this effect. The cleanup (lines 181-184) only does `clearTimeout` on the pending 400ms commit; it never fires it and never resets `liveObject`/`livePlanting`. Sequence: nudge object A (`setLiveObject(next)` fires immediately at line 153, PATCH deferred 400ms via `commitLater`), then within that window perform any unrelated edit that touches the objects/plantings cache — the effect tears down, the pending PATCH for A is silently canceled, and `liveObject` is never cleared. Since `GardenCanvas.tsx:95-98` unconditionally renders `liveObject` in place of the matching id, object A keeps showing the nudged position for the rest of the session even though the server was never updated. Fix: flush the pending commit in the cleanup instead of discarding it, or move the debounce state outside an effect keyed to data that changes on unrelated mutations. - **`web/src/lib/objects.ts:269-281` — `useClearObject` has no reconciliation on partial failure, leaving the UI showing already-removed plops as still active.** The mutation runs `Promise.all` over per-plop PATCHes with no `onMutate` optimistic update. If one PATCH rejects, `Promise.all` rejects immediately even though other in-flight PATCHes may still complete server-side. `onError` (line 279) only shows a toast — unlike every other mutation in this file (e.g. `useUpdateObject`'s conflict-adopt at lines 147-160, `useRemovePlanting`'s rollback at lines 298-312), it never invalidates or reconciles the cache, so the query cache still lists all plops as active even though some were soft-removed on the server. A retry from `ClearBedModal` (which keeps its stale `plops` array since the modal only closes `onSuccess`) resends now-stale versions for the already-cleared rows, generating further 409s. Suggest invalidating `fullKey(gardenId)` in `onError` too, so the UI reflects whatever partially succeeded. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings are confirmed against the actual code. Both line numbers, quoted expressions, and structural claims match exactly. **VERDICT: Minor issues** - `web/src/pages/GardenEditorPage.tsx:247` and `:333` — the focused-object display name is computed with `focusedObject.name || kindDef(focusedObject.kind)?.label || 'X'` twice, with two different fallback literals (`'Object'` in the toolbar chip vs `'bed'` in the `ClearBedModal` prop). Confirmed by reading both lines directly — same derivation duplicated with a small, easy-to-miss inconsistency. Worth hoisting into a single `const focusedObjectLabel = focusedObject.name || kindDef(focusedObject.kind)?.label || 'bed'` computed once and reused. - `web/src/pages/GardenEditorPage.tsx:148-178` — the keyboard-nudge `onKey` handler has two branches (object nudge vs plop nudge) that repeat the same shape: resolve `base` from live-state-or-list, `e.preventDefault()`, compute `next`, call the matching `setLive*`, then `commitLater(() => { mutate(...); resetLive() })`. Confirmed by reading the full effect (lines 126-186) — the only real differences are the source list/live-state pair, the optional bounds clamp for plops, and which mutation hook fires. A small shared helper would collapse this to one implementation. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> This confirms each plop is created by one manual tap (radius = 1.5×spacing, not area-filling), so plop count per bed equals however many times a user tapped — not something automatically "derived" via `computeDerivedCount`. That formula computes the *display* count of plants a single plop's circle represents, not the number of plops. The draft's "~80 plops via `computeDerivedCount`" example misreads this — a bed reaching dozens of plops requires dozens of manual taps, not one API/derivation step. The core code claim (unbounded `Promise.all` fan-out, no chunking helper anywhere) is independently confirmed at `objects.ts:274-276`, so I'm keeping the finding but correcting the reasoning. ## VERDICT: Minor issues - **`web/src/lib/objects.ts:274-276`** (`useClearObject`) — clearing a bed fires `Promise.all(plops.map(...))`, i.e. **N concurrent PATCH requests with no batching or concurrency cap**. Confirmed no chunking/concurrency-limiting helper exists anywhere in `web/src` (`grep -rn "Promise.all|p-limit|concurrency|chunk("` turns up only this call site). There's also no cap on how many plops an object can hold (no `MAX_PLOP`-style guard anywhere in the editor). Note: plop count is **not** derived automatically from bed area/spacing via `computeDerivedCount` — that function only computes the *display* count of plants a single already-placed circle represents (`lib/plantings.ts:66-69`); each plop is created by one manual tap in `GardenCanvas.tsx`'s `onPlace` (`editor/GardenCanvas.tsx:138-151`). So reaching dozens of plops in one bed requires dozens of individual manual placements, not a single planting action — a real but less automatic path to the same fan-out. The PR's own comment acknowledges this is a stopgap pending a bulk `ClearObject` endpoint (#19), so it's a known, accepted tradeoff; still worth capping concurrency (e.g., batches of ~6-8) for gardens that do accumulate many hand-placed plops in one bed. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## VERDICT: Minor issues - **`web/src/pages/GardenEditorPage.tsx:181-184`** — The keyboard-nudge debounce timer is cancelled (not flushed) on unmount/dependency change: ```js return () => { window.removeEventListener('keydown', onKey) if (nudgeTimer.current != null) window.clearTimeout(nudgeTimer.current) } ``` A nudge shows the moved position via `setLiveObject`/`setLivePlanting` (lines 153/173) as soon as the key is released, but the PATCH that persists it is deferred ~400ms via `commitLater` (lines 134-140). If the component unmounts inside that window (route change, exiting focus, navigating away) the timer is cleared with no flush, so `mutate()` never fires and the move is silently lost — on reload the object/plop reverts to its old position with no error. The effect re-runs on every `objects`/`plantings` change too (dependency array at line 186), so this can also fire mid-session, not just on unmount. Suggest flushing the pending commit instead of dropping it. - **`web/src/lib/objects.ts:266-280`** (`useClearObject`) — the "Clear bed" loop has no partial-failure handling: ```js await Promise.all(plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version }))) ... onSuccess: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }), onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')), ``` `Promise.all` rejects on the first failing PATCH, but any PATCHes that already resolved have already soft-removed those plops server-side — that state isn't rolled back. `onError` doesn't invalidate/refetch (unlike `useDeleteObject`, which does `onSettled: () => qc.invalidateQueries(...)` unconditionally — confirmed at `web/src/lib/objects.ts:182`), so the query cache is never reconciled: the UI keeps showing all plants as present, the toast implies nothing happened, and `ClearBedModal.tsx`'s `onSuccess: onClose` means the modal stays open only on failure — enabling a retry that re-PATCHes already-removed rows with stale `version`, producing spurious 409s. Suggest invalidating on error too (or using `Promise.allSettled` and reporting which failed). </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** Security review of this grab-bag polish PR, traced tainted input → sink: - **Clear-bed authz**: `useClearObject` loops `api.patch('/plantings/${p.id}', { removedAt, version })`. The `p.id`/`version` are server-issued numbers from the authenticated garden's plops (no path/query injection — numeric interpolation only). Each PATCH is server-side authorized via `service.UpdatePlanting` → `objectForRole(actorID, …, roleEditor)` (`internal/service/plantings.go:119`), so the client `canEdit` gate is genuine defense-in-depth, not the enforcement point. No authz bypass. - **Nudging authz**: gated client-side by `canEdit` and dispatched through `useUpdateObject`/`useUpdatePlanting`, which hit the same server role re-check. A spoofed client `canEdit`/`myRole` cannot escalate — server rejects non-editors. Verified the hoisted `canEdit` (`gd != null && (gd.ownerId === me.data?.id || gd.myRole === 'editor')`) is logically identical to the prior post-load computation; loading state (gd null) yields `canEdit=false`, so the handler no-ops rather than running ungated. - **`usePageTitle`**: assigns `document.title` from `full.data?.garden.name` (user-controlled). `document.title` is a plain string property, not an HTML-parsing sink, so no XSS. React escapes `objectName` in `ClearBedModal`. No credential/secret leakage in titles. - **Favicon / NotFound**: both static — the inline SVG data-URI is a hardcoded emoji, no untrusted interpolation; `NotFound` renders only constant strings and a `/gardens` link. - **`removedAt` payload**: client sends `new Date().toISOString().slice(0,10)`; even if a malicious client sent an arbitrary string, it's the actor's own garden row and the server stores it as TEXT — not a cross-user injection vector, and this PR reuses the existing `useRemovePlanting` pattern rather than introducing a new surface. No findings in the security lens. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> ## Verdict: Minor issues found Investigated the `useClearObject` Promise.all loop against the other planting/object mutations and the `usePageTitle` capture/restore implementation against actual usage. **Findings** - **`web/src/lib/objects.ts:269-281` — `useClearObject` partial failure leaves the cache inconsistent with the server (medium).** `mutationFn` runs `Promise.all` over per-plop PATCHes. `Promise.all` rejects on the first failed PATCH (e.g. a 409 on one stale version), but the remaining in-flight requests still complete — so some plops are soft-removed server-side while others are not. On rejection only `onError` runs (a generic toast); there is **no `onError`/`onSettled` invalidation**, unlike `useDeleteObject` which invalidates in `onSettled` (line 182) and `useRemovePlanting` which reconciles via `conflictPlanting` in `onError` (lines 298-311). The cache therefore still shows every plop, but the server has removed a subset. `ClearBedModal` only closes on `onSuccess` (`clear.mutate(plops, { onSuccess: onClose })`), so the modal stays open on failure, and a retry re-sends the now-stale versions of already-removed rows (they'll 409/404 again). Confirmed by reading the hook (lines 269-281) and comparing to `useDeleteObject` (line 182) and `useRemovePlanting` (lines 292-309). Suggested fix: add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` so the cache reconciles to the server's actual state regardless of partial failure, and consider `Promise.allSettled` so the error message can report how many succeeded. - **`web/src/lib/usePageTitle.ts:6-12` — `usePageTitle` restore-on-unmount can restore a stale route title across SPA navigations (minor).** On mount it captures `prev = document.title` (the *previous* route's title) and restores that exact string on unmount. Navigating Gardens → GardenEditor → Plants: the GardenEditor effect captures `prev = "Gardens · pansy"`; when GardenEditor unmounts it restores `"Gardens · pansy"` instead of a neutral default, and depending on React's effect/cleanup ordering during a route swap the old page's cleanup can transiently clobber the new page's title. Verified the implementation at `usePageTitle.ts:6-12`; `usePageTitle` is now called from GardensPage, GardenEditorPage, PlantsPage, LoginPage, RegisterPage, and NotFound. This is a known footgun of the capture/restore pattern in SPAs. Suggested fix: don't capture/restore — set the title on mount and reset to the app default (`'pansy'`) on unmount, or just always set (no restore) so each route owns its title. **Verified-correct items (not findings):** - `hitR = Math.max(r, 22/scale)` re-derives correctly: radius `22/scale` cm × `scale` px/cm = 22 px radius = 44 px diameter, matching the "~44px" comment. `fill="transparent"` is a paint and receives pointer events under the default `visiblePainted`, unlike `fill="none"`. - Nudge debounce: the 400ms idle timer is reset on every keypress, so exactly one PATCH fires with the accumulated final position and the original committed version — correct, no self-conflict. - Plop nudge clamps to `±widthCm/2`, `±heightCm/2` (object-local half-extents) — correct; object nudging intentionally has no garden-bounds clamp, matching drag which also doesn't clamp. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Verdict: Minor issues - `web/src/pages/GardenEditorPage.tsx:148-178` — The keyboard-nudge `onKey` handler has two near-identical branches (object vs. planting) that repeat the same shape: find `base` from `live*`/array → `e.preventDefault()` → compute `next` → `setLive*` → `commitLater(() => { mutate(...); setLive*(null) })`. The only real differences are the store setter, the mutate function, and the clamping. This is copy-paste that should be factored — e.g. a small `nudge(base, { setLive, commit })` helper or a discriminated helper. As written, a change to debounce/commit logic (or adding a third entity type) must be edited in two places. Verified by reading lines 141–179. - `web/src/lib/objects.ts:269-281` vs `283-291` — `useClearObject` reimplements the "PATCH `removedAt` = today" soft-remove that `useRemovePlanting` already encodes, including the `new Date().toISOString().slice(0, 10)` date formatting. The "today" computation is now duplicated in two mutations. A shared `todayISO()` (or a shared `softRemove(id, version)` patch helper) would keep the two in sync; right now a date-format change must be made in both. Verified by reading both functions. - `web/src/pages/GardenEditorPage.tsx:275-288` — The two empty-state overlay `<p>` elements share the identical long class string `max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur`. Extracting a `hintOverlay` class constant (or a tiny `<EmptyHint>` component) would remove the duplication and make the visual treatment consistent by construction. Minor. - `web/src/pages/GardenEditorPage.tsx:60-64` — The hoisted `canEdit`/`isOwner` block leaves `isOwner` recomputed twice conceptually: once here (line 64) and the prior comment about authoritative ownership was removed, but the old derived `isOwner`/`canEdit` block at the former lines 201–204 is gone (good). One nit: the new `canEdit` uses `me.data?.id` (optional chaining) while `isOwner` requires `me.data != null`, so the two "owner" checks read inconsistently — worth a one-line comment or aligning the form for future readers. Minor. No material issues found in `NotFound.tsx`, `usePageTitle.ts`, `ClearBedModal.tsx`, the favicon, or `PlopMarker.tsx` from this lens. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> ## VERDICT: Minor issues Through the performance lens, the change is mostly clean — the nudge debounce correctly collapses a held-key stream into a single PATCH (timer reset on each keydown, one fire on idle, and the cleanup path clears any pending timer), and the live store updates are cheap synchronous Zustand sets. Two notes, both in my lane: - **`web/src/lib/objects.ts:274` — unbounded concurrent PATCH fan-out in `useClearObject`.** `Promise.all(plops.map(...))` fires one HTTP request per plop simultaneously. For a bed with many plops (dozens+) this is a burst of N concurrent requests against `/plantings/:id` — no batching, no concurrency limit, no optimistic cache update (unlike `useRemovePlanting` at objects.ts:285+, which has `onMutate` + `cancelQueries` and rollback in `onError`). The UI stays stale until *all* resolve and the single `invalidateQueries` refetch completes. Confirmed against the actual source: `useClearObject` (lines 269–281) has only `onSuccess`/`onError`, while `useRemovePlanting` (lines 285–312) does the optimistic filter in `onMutate`. The PR acknowledges a bulk `ClearObject` endpoint is coming in #19, so this is a known temporary seam, but as shipped it's the worst-case request pattern for the largest beds. Suggested mitigation before #19: cap concurrency (e.g. a small `pLimit`-style loop) or sequence the patches, and optionally mirror `useRemovePlanting`'s optimistic `onMutate` filter so the bed clears instantly. Not blocking — it's a one-shot user action behind a modal, not a hot path. - **`web/src/pages/GardenEditorPage.tsx:186` — nudge effect re-subscribes on every `objects`/`plantings` change.** The `keydown` listener is torn down and re-added whenever `objects` or `plantings` get new identities (which happens on each debounced commit, since `updateObject`/`updatePlanting` patch the full-garden cache via `invalidateQueries`). Cost is one `removeEventListener`/`addEventListener` per commit — negligible — so this is a non-issue in practice; flagging only because the dep choice trades a tiny re-subscription cost for correctly fresh closures. No action needed. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings verified against the actual code. Let me confirm the modal behavior in ClearBedModal re: the stuck-unrecoverable claim. The ClearBedModal passes `onSuccess: onClose` — so on success the modal closes. On error (`onError`), no `onClose` fires, so the modal stays open with the same stale `plops` (the parent's `focusedPlops`, computed from the still-stale cache). Re-clicking "Clear bed" re-PATCHes with old versions. That confirms the stuck-unrecoverable scenario in finding 1. Both findings survive verification. VERDICT: Minor issues - **`web/src/lib/objects.ts:269-281` — `useClearObject` leaves the cache stale on partial failure.** `mutationFn` runs a `Promise.all` of PATCHes with no `onMutate` optimistic update and only `onSuccess` invalidates. On a partial failure (one PATCH 409/500s after others already soft-removed server-side), `Promise.all` rejects → `onError` only toasts; there is no `invalidateQueries` and no cache reconciliation. Result: the server has removed some plops but the `FullGarden` cache still lists all of them, so the editor keeps showing a full bed. Worse, `ClearBedModal` passes `{ onSuccess: onClose }`, so on failure the modal stays open with the same stale `plops` (the parent's `focusedPlops`, derived from the stale cache) and stale `version`s; re-clicking "Clear bed" re-PATCHes already-removed rows with their old versions → 409s → the user is stuck unable to recover without a manual refetch. Verified against `useRemovePlanting` (lines 285-314) and `useUpdatePlanting` (lines 226-263), both of which reconcile via `onMutate`/`onError`, which `useClearObject` omits. Suggested fix: add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` (or invalidate in `onError`), and consider an optimistic `onMutate` that drops the cleared plops. - **`web/src/pages/GardenEditorPage.tsx:126-186` — nudge commit can be silently dropped, leaving the object/plop moved on screen but never persisted.** The keydown effect depends on `[canEdit, objects, plantings]`, so it re-subscribes whenever the `FullGarden` cache changes. The cleanup clears the pending 400ms debounce timer (`nudgeTimer.current`). Sequence: a nudge's timer fires → `updateObject.mutate(...)` and `setLiveObject(null)` run; if the user presses an arrow again before the PATCH resolves, the handler bases `next` off `s.liveObject`/`objects`, calls `setLiveObject(next)` and schedules a new timer; the first PATCH resolves → `useUpdateObject.onSuccess` (`objects.ts:141-145`) patches the cache → `objects` gets a new identity → the effect re-runs → cleanup clears the new (still-pending) timer. The second nudge is never committed, and `liveObject` is left at the un-persisted position (the canvas prefers `liveObject`), so the object appears moved but won't survive a reload. Verified by reading the hook at `GardenEditorPage.tsx:126-186` and `objects.ts:141-146`. Suggested fix: keep the timer in a ref that survives effect cleanup (it already is in `nudgeTimer`, but the cleanup clears it), or move the debounce scheduling out of the effect closure / skip clearing it on re-subscription, or read `base` from the store's live value rather than the dep-bound `objects`/`plantings`. </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 lens: the authz gating is correct, there are no injection surfaces, no XSS vectors, and no unsafe handling of untrusted data. The clear-bed flow and keyboard-nudge handler both check `canEdit` before mutating, and the hoisted ownership check (`ownerId === me.id`) is used consistently. The `usePageTitle` hook assigns to `document.title`, which is a safe text property (not HTML-parsed). All API paths use server-derived numeric IDs, not user-controlled strings. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** * **Keyboard nudge debounce races with drag gestures** — `web/src/pages/GardenEditorPage.tsx:134-186` The 400ms debounced PATCH from arrow-key nudging captures the nudged position in a closure (`next`). If the user starts and finishes a drag gesture (object move in `SelectionOverlay` or plop move in `PlopOverlay`) before that timer fires, both PATCHes fly with the same stale `version`. Whichever reaches the server second 409s; depending on network ordering the drag result can be silently lost and the object snaps back to the stale keyboard-nudged position. The timer should be cleared when a gesture starts (e.g. by watching `liveObject`/`livePlanting` in an effect, or by clearing the ref in the gesture begin path). * **Cross-selection-type nudge cancellation** — `web/src/pages/GardenEditorPage.tsx:134-186` The single `nudgeTimer` ref is shared between object nudges and plop nudges. Nudging an object, then selecting a plop (or vice-versa) within the 400ms window cancels the first pending PATCH entirely; the first nudge is lost and never persisted because the new selection’s `commitLater` clears the old timer. The fix is to maintain separate timers (or a keyed map) so object and plop debounces are independent. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found The changes are clean and maintainable: - `usePageTitle` is a small, well-scoped hook with proper cleanup, applied consistently across all top-level route pages. - `ClearBedModal` is a focused presentational component; the `useClearObject` hook name follows the existing convention in `lib/objects.ts`. - Keyboard nudging logic in `GardenEditorPage.tsx` is contained within a single `useEffect` with a locally scoped `DIRS` table and `commitLater` helper — acceptable length for the behavior it encapsulates. - Empty-state overlays are declarative and stateless. No dead code, no duplication worth extracting, no leaky abstractions. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Verdict:** Minor issues **Findings** - **`web/src/pages/GardenEditorPage.tsx:126-186`** — The keyboard-nudge effect lists `[canEdit, objects, plantings]` as dependencies. Both `objects` and `plantings` are `useMemo`-ed arrays that change reference whenever the underlying `useGardenFull` query data updates (even on a refetch with identical content). This causes the global `keydown` listener to be removed and re-added frequently, but more importantly the cleanup unconditionally runs `clearTimeout(nudgeTimer.current)`. If a query refresh or optimistic update happens during the 400 ms debounce window, the pending PATCH is silently cancelled. The user sees the nudged position (via `setLiveObject`) but the server never receives it, so the next refetch snaps the item back. **Fix:** Read `objects`/`plantings` through refs (or the Zustand store) inside the handler instead of closing over them in the effect, so the effect deps reduce to `[canEdit]` and the timer survives data changes. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **`web/src/pages/GardenEditorPage.tsx:154` — Debounced nudge timer can clear a newer live gesture** The `commitLater` closure calls `setLiveObject(null)` / `setLivePlanting(null)` when it fires, without checking whether `liveObject`/`livePlanting` is still the nudged entity. If the user starts dragging another object (or another plop) during the 400 ms debounce window, the timer will overwrite that newer live state and snap the dragged item back to its committed position. **Fix:** In the timer callback, read the current store state and only call `setLiveObject(null)` if `liveObject?.id === next.id` (and similarly for planting). - **`web/src/lib/objects.ts:269-281` — `useClearObject` has no optimistic update and no partial-failure handling** `Promise.all` patches each plop individually. If one request fails, `onError` fires but `onSuccess` does not, so the cache is never invalidated. Any plops that were successfully soft-removed on the server still appear in the UI until a manual refresh. There is also no 409 conflict recovery (unlike `useRemovePlanting`). **Fix:** Add an `onMutate` that filters the plops from the cache optimistically, snapshot for rollback, and handle partial failure (e.g. `Promise.allSettled`) or at minimum invalidate in `onSettled` so successes are reflected even if one PATCH errors. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - **`web/src/lib/objects.ts:269` — `useClearObject` diverges from the established soft-remove pattern.** The sibling `useRemovePlanting` (lines 285–314) does optimistic cache patching (`onMutate`), single-row invalidation, and 409-conflict recovery via `conflictPlanting`. The new `useClearObject` does none of that: `Promise.all` of PATCHes, one `invalidateQueries`, generic toast. If one of N PATCHes 409s, `Promise.all` rejects partway, the cache isn't rolled back, and the user sees a generic "Could not clear the bed" while some plops are gone server-side. This creates two divergent error-handling strategies for the same soft-remove operation; the next maintainer has to reconcile them. Suggest mirroring `useRemovePlanting`'s `onMutate` (filter all cleared ids) + `onError` (restore `ctx.prev`, surface conflict rows), or at least a comment explaining why bulk-clear is intentionally simpler until #19. - **`web/src/pages/GardenEditorPage.tsx:148-178` — nudge handler duplicates ~30 lines of parallel structure.** The `selectedId` and `selectedPlantingId` branches are near-identical: find base, `preventDefault`, compute `next`, `setLive*`, `commitLater(() => { update*.mutate(...); setLive*(null) })`. The only real differences are the clamp-to-bounds (plops only) and which `setLive`/`update` to call. A small helper like `nudge(base, dir, step, { setLive, commit, clamp? })` would collapse this and make future changes one-edit. Minor. - **`web/src/pages/GardenEditorPage.tsx:275-288` — empty-state overlay markup is duplicated inline.** Two near-identical `<div className="pointer-events-none absolute ..."><p className="max-w-xs rounded-lg bg-surface/85 px-4 py-3 text-sm text-muted shadow-sm backdrop-blur">…</p></div>` blocks differ only in positioning class and message. A tiny `<EmptyHint className=…>…</EmptyHint>` (or even a shared `className` constant for the `<p>`) would dedupe. - **`web/src/lib/usePageTitle.ts:7-11` — restore-on-unmount relies on an undocumented ordering assumption.** The JSDoc notes that the previous title is restored on unmount, but the correctness of that restore depends on route transitions happening to unmount-then-mount synchronously (so the new page captures the restored default). A future maintainer could break this by wrapping pages in a keep-alive or reordering effects; a one-line note on the assumption would prevent that. Trivial. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Through the performance lens: - **Nudge debounce** (`GardenEditorPage.tsx:134-140`): The `commitLater` pattern correctly cancels and reschedules a single timer on every keypress, so a held arrow key produces one PATCH after ~400ms idle rather than per-repeat requests. Verified by reading the effect — the closure captures the latest `next`, so only the final position is committed. This is the right shape. - **Effect re-binding** (`GardenEditorPage.tsx:185-186`): The keydown effect depends on `[canEdit, objects, plantings]`, both of which are `useMemo`'d on server data only. Live nudge updates go through `setLiveObject`/`setLivePlanting` in the editor store, not through `objects`/`plantings`, so the listener is not torn down and re-bound on each keypress — only on server refetch (after a commit's invalidation). Not a hot-path concern. - **`focusedPlops` filter** (`GardenEditorPage.tsx:209`): Runs on every render, including during nudge-induced re-renders. It's a single `Array.filter` over the garden's plantings; for realistic bed sizes this is trivial and not material. - **`useClearObject` parallel PATCH loop** (`lib/objects.ts:271-277`): Fires N parallel PATCHes with one invalidation. This is explicitly called out in the PR as a stopgap pending the bulk `ClearObject` endpoint (#19). For typical beds this is a small burst; not a regression worth flagging under this lens. - **PlopMarker hit circle** (`PlopMarker.tsx:40-51`): Adds one extra `<circle>` per plop with `fill="transparent"`. Constant-factor, negligible render cost; no per-frame work. No unbounded growth, N+1 patterns, missing limits, or hot-loop regressions introduced. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## VERDICT: Minor issues found **Findings (verified by reading the code):** - **`web/src/lib/objects.ts:269-281` — `useClearObject` has no optimistic `onMutate`, no 409/conflict reconciliation, and no per-row rollback.** Verified: the sibling `useUpdatePlanting` (lines 226-263) and `useRemovePlanting` (285-313) both do `onMutate` optimistic cache patches with a `prev` snapshot, handle `conflictPlanting(err)` to adopt the server's current row on 409, and restore `ctx.prev` on other errors. `useClearObject` (269-281) does none of this — it just fires `Promise.all` of PATCHes and on error shows a single generic toast. Concrete impact: if one of N PATCHes 409-conflicts (e.g. another client moved a plop and bumped its `version`), `Promise.all` rejects, the whole clear is reported as failed, but every plop that *did* PATCH successfully is already soft-removed server-side while the cache is never invalidated on failure (`onSuccess` at 278 is the only invalidate) — so the UI still shows them as present (stale) until the next external refetch, and a retry uses the same stale `version` for the plop that 409'd, so it will keep 409-ing. There's also no snapshot to roll back the partially-cleared cache. Suggested fix: either route through `useRemovePlanting` per plop (it already does the right reconciliation), or give `useClearObject` an `onMutate` that filters the cleared ids with a `prev` snapshot, an `onError` that restores `ctx.prev` and adopts `conflictPlanting(err)` for the conflicting row, plus re-fetch the plops' current versions before retry. At minimum, invalidate on failure too so the UI reconciles to the partially-applied server state. - **`web/src/lib/objects.ts:274-276` — `Promise.all` is all-or-nothing; partial success is silently lost.** Verified: the mutation only invalidates on `onSuccess` (line 278), so a single rejected PATCH throws away the fact that the others already soft-removed their rows server-side. The cache remains stale for the successfully-removed plops until the next external refetch. Same location as above; the fix (per-row `useRemovePlanting`, or `Promise.allSettled` + reconcile + invalidate-on-failure) covers this. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** I traced every new code path through the security lens: - **Authz / Clear bed**: The `ClearBedModal` button is gated on `canEdit` (owner or editor). The backend `UpdatePlanting` handler enforces `roleEditor` via `objectForRole` for each individual PATCH, so even if the hook were called directly with cross-object IDs, every request is re-authorized server-side. No bypass. - **Authz / Keyboard nudging**: The effect returns early when `!canEdit`, and the same backend `roleEditor` guard applies to `updateObject` / `updatePlanting`. Viewers cannot nudge. - **Input validation / Nudging**: Step size is hardcoded (1 or 10 cm). Object positions are validated server-side by `bboxOverlapsGarden`; plop positions are client-clamped to object bounds and re-validated server-side by `finalizePlanting`. No injection or out-of-bounds persistence path. - **XSS / Titles**: `usePageTitle` sets `document.title` through the DOM property, which is plain-text safe. The `Garden` name from the server is never rendered into HTML via this path. - **SSRF / Secrets / Injection**: No new external requests, SQL, file paths, or credential handling introduced. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Verdict: Blocking issues found** * `web/src/pages/GardenEditorPage.tsx:126-186` — The keyboard-nudge `useEffect` depends on `[canEdit, objects, plantings]`. Both `objects` and `plantings` are fresh arrays created by `useMemo` on every change to the underlying server data (even identical data, because React Query’s structural sharing only preserves subtree references when the data is unchanged, and any concurrent edit or background refetch can still produce new top-level array references). The effect’s cleanup function unconditionally calls `window.clearTimeout(nudgeTimer.current)`, so **any re-render that occurs during the ~400 ms debounce window silently cancels the pending PATCH**. The user sees the object/plop move live, but on release it snaps back to its pre-nudge position because the commit never fires. **Fix:** hoist `objects` and `plantings` into refs (`useRef`) and read them inside the handler; keep the effect deps limited to `[canEdit]` so re-renders don’t tear down the listener and cancel the timer. * `web/src/lib/objects.ts:278` — `useClearObject` fires a `Promise.all` of soft-remove PATCHes but has **no optimistic `onMutate`** and only invalidates on `onSuccess`. If one PATCH fails (e.g., version conflict) while others succeed, `onError` fires, the cache is never invalidated, and the UI continues displaying plants that were already removed on the server. A retry will then 409 on the already-removed rows because their versions were bumped by the successful PATCHes. **Fix:** add an `onMutate` that filters the cleared plops from the cache immediately (mirroring `useRemovePlanting`), and call `invalidateQueries` in `onSettled` so a partial failure is still reconciled. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found * **Minor issues:** - `web/src/pages/GardenEditorPage.tsx:209` — `focusedPlops` is computed inline on every render and used in both JSX conditions and as props for `ClearBedModal`. Wrapping it in `useMemo` avoids unnecessary array allocations and keeps the render path predictable. - `web/src/lib/objects.ts:269` — `useClearObject` is the only mutation in the file that skips optimistic cache updates (`onMutate`/`onError` rollback). Every sibling (`useUpdatePlanting`, `useRemovePlanting`, etc.) patches the cache immediately to prevent UI flashes; this new hook breaks that local pattern and will show a lag before the plops disappear. (Verified by reading the full `objects.ts` file and comparing the mutation shapes.) </details> <details><summary><b>⚡ Performance</b> — Blocking issues found</summary> **Blocking issues found** - **`web/src/pages/GardenEditorPage.tsx:186`** — The keyboard nudge `useEffect` lists `objects` and `plantings` in its dependency array. Because `patchFullCache` (called by `useUpdateObject` / `useUpdatePlanting` optimistic updates) returns new array references on every cache mutation, this effect tears down and re-adds the global `keydown` listener repeatedly during normal editing. The previous effect’s cleanup clears `nudgeTimer.current`, so if **any** mutation patches the garden cache while a nudge debounce is pending (within the 400 ms window), the timer is killed and the PATCH is silently dropped. This is easy to hit: nudge a bed, then immediately change a plant name or create a plop—the final nudge never persists. **Fix:** remove `objects`/`plantings` from the effect deps; keep fresh refs to them (or read them from the store) so the listener stays mounted and the debounce survives unrelated cache updates. - **`web/src/lib/objects.ts:274`** — `useClearObject` fires every plop PATCH concurrently via `Promise.all`. For a densely planted bed this creates an unbounded request burst that hammers the browser’s per-host connection limit (typically 6) and can overload the server. **Fix:** add a concurrency limit or sequential loop as a stop-gap until the bulk endpoint (#19) lands; e.g. chunk the array and `await` each chunk. --- </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **Race between rapid nudges and version increments** — `web/src/pages/GardenEditorPage.tsx:126-186` The keyboard-nudge effect fires `updateObject.mutate` / `updatePlanting.mutate` with the stale `next.version` captured at key-down time. Between the nudge and the 400 ms debounced commit, an earlier optimistic patch already bumped the cached version via `applyServerPatch`/`applyPlantingPatch` (version + 1). The debounced handler does not re-read the version, so it PATCHes the old version and 409-conflicts with itself. The user sees a spurious “updated elsewhere — your change was rolled back” toast, and the nudge is lost. **Fix:** re-read the current version from the store (or query cache) inside the `commitLater` callback right before calling `mutate`, or compute the delta relative to the latest committed state rather than capturing a snapshot. - **Missing `onSettled` / optimistic rollback for `useClearObject`** — `web/src/lib/objects.ts:269-281` `useClearObject` patches planting rows in a `Promise.all` loop and only invalidates on success. If any individual PATCH 409s or fails, `onError` toasts, but the successful PATCHes in the same batch have already soft-removed those plantings server-side. The local cache is not reconciled until the next refetch, so the UI temporarily shows the cleared plops as still present (or, conversely, shows them removed while the error toast implies nothing happened). More importantly, because there is no `onMutate` optimistic update, the user gets no visual feedback during the network round-trip; a large bed can take seconds and look frozen. **Fix:** add `onMutate` to optimistically drop the plops from the cache, and in `onError` restore the previous `FullGarden` snapshot so the UI matches the server state regardless of partial failure. - **Boundary / off-by-one in nudge clamping for non-square objects** — `web/src/pages/GardenEditorPage.tsx:168-171` Plops are clamped to `[-widthCm/2, widthCm/2]` and `[-heightCm/2, heightCm/2]`. If the object has been rotated (`rotationDeg != 0`), the plop’s local `xCm/yCm` frame is rotated relative to the object’s bounding box; clamping axis-aligned coordinates against the unrotated width/height allows the plop to drift outside the actual bed. **Fix:** apply the inverse rotation to the plop coordinates before clamping, or skip clamping until the geometry math accounts for rotation. - **Unchecked empty-array guard in `ClearBedModal`** — `web/src/editor/ClearBedModal.tsx:35` If `plops` is empty (possible if the caller races with a concurrent delete or a stale focused-object ID), `clear.mutate([])` fires a no-op mutation that succeeds instantly and closes the modal. It does not crash, but it silently consumes a user confirmation for nothing. More critically, `useClearObject` loops over the array with `Promise.all`; an empty array resolves immediately, making the UX misleading. **Fix:** disable the “Clear bed” button when `plops.length === 0`, or bail early in the mutation with a toast. - **Potential stale closure in page-title restoration** — `web/src/lib/usePageTitle.ts:5-12` On rapid mount/unmount cycles, `prev` is captured synchronously in the effect body. If a parent changes `document.title` between the mount of two `usePageTitle` consumers (e.g., navigating from one route to another), the unmount of the first restores the title to the *previous* page’s title, clobbering the second page’s title. This is a classic race with multiple competing title effects. **Fix:** use a ref or a global stack to track title history, or at least read `document.title` in the cleanup function rather than closing over the initial value. - **`buttonClasses('primary')` on `<Link>` discards navigation semantics** — `web/src/components/NotFound.tsx:15` Wrapping a TanStack Router `<Link>` with `buttonClasses` applies the visual style, but if `buttonClasses` ever gains `type="button"` or other button-specific attributes (it currently does not, but it is styled as a button), the component could inadvertently suppress default link behavior. More relevant to this lens: if the Link fails to resolve (e.g., router mismatch), there is no fallback `onClick` handler or error boundary, leaving the user stranded on the 404 page with no recovery path. **Fix:** add an `onClick` fallback that calls `window.location.href = '/gardens'` for resilience, or wrap in an error boundary. (Low severity, but confirmed.) </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-19 04:42:53 +00:00
Address Gadfly review on #18: clear-bed + nudge robustness
Build image / build-and-push (push) Successful in 8s
c4bc5e9f47
- useClearObject: Promise.allSettled + invalidate in onSettled (not onSuccess),
  so a partial failure still reconciles the cache with the server instead of
  losing the already-removed rows; reports how many failed.
- Keyboard nudge: the keydown effect is now mounted once and reads live values
  from a ref, so a data refetch can't re-subscribe and cancel a pending
  debounced commit. The pending move is flushed on unmount instead of dropped,
  a fire only commits if its live value is still present (a drag that cleared it
  already committed its own PATCH), and nudges are ignored while a pointer drag
  is active — closing the nudge/drag race.
- usePageTitle: dropped the restore-on-unmount, which raced and could clobber the
  incoming route's title on a fast navigation.
- ClearBedModal disables confirm on an empty plop list.
- Extracted EditorHint (empty-state overlays) and objectDisplayName (harmonized
  the name fallback across the toolbar and clear dialog).

Skipped (false positive): "nudge clamp ignores rotation" — the clamp is to the
object's LOCAL bounds (±w/2, ±h/2), exactly what the server enforces, so a plop
can't escape a rotated bed.

tsc --noEmit clean; 24/24 vitest; production build green.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit 245e0cbe71 into main 2026-07-19 04:44:26 +00:00
steve deleted branch phase-7-polish 2026-07-19 04:44:27 +00:00
Sign in to join this conversation.