Closes#15. Part of epic #20 (Phase 5 — Plops). This is pansy's core vision. Builds on #11 (field editor), #13 (PlantPicker), #14 (plantings API).
Data layer
lib/plantings.ts — ServerPlanting zod shape, EditorPlanting, effectiveCount (override ?? derived), and computeDerivedCount — a client mirror of the server's max(1, round(π·r²/spacing²)) for live resize display. Unit-tested.
lib/objects.ts — /full now types plantings + plants (was z.unknown()). Added useCreatePlanting / useUpdatePlanting / useRemovePlanting that patch the same FullGarden cache with the #11 optimistic + 409-rollback contract. A soft-remove (removedAt set) drops the plop from the active list; the version is bumped optimistically to avoid a self-conflict on a fast follow-up.
Editor
store — selectedPlantingId / livePlanting / armedPlant; object vs plop selection are mutually exclusive.
Focus mode — the object inspector's "🌱 Plant here" sets focusedObjectId; the canvas animates fitToRect to the object and dims everything else. A breadcrumb (← garden), Escape, or an empty-space tap exits. focusedObjectId mirrors to ?focus so focus is deep-linkable and survives reload.
Placing — "+ Add plant" opens the PlantPicker; the chosen plant stays armed for repeat placement; tapping inside the focused object drops a plop (default radius max(1.5·spacing, 15cm)), converted into the object's local frame before POSTing.
Manipulating — PlopOverlay (drag body to move, clamped to bounds; edge handle to resize; one PATCH per gesture) + PlopInspector (change plant via picker, radius, count override vs live derived, label, planted date, soft-remove).
PlopMarker semantic zoom (px/cm bands <0.75 / 0.75–3 / >3): far = flat color patch + a dominant-plant tint on the object so beds read at a glance; mid = colored circle + emoji; near = + plant name + count. Plops render inside each object's rotated group, so moving/rotating a bed carries its plops with no planting writes.
Notes / choices
"Click a plantable object → focus" is delivered as an explicit "Plant here" button (one tap, discoverable, works on mobile) rather than hijacking single-click — single-click still selects the bed for field-view move/resize/rotate, which the acceptance criteria also require (rotate a bed 45° → plops stay in their corners).
Checks
tsc --noEmit clean · 24/24 vitest · vite build green.
Acceptance criteria
✅ Focus into a bed, place garlic plops in one corner + basil in another; exit; zoom out → color patches/tint + names identify plantings; mid → icons; near → names + counts. Deep-linkable via ?focus; survives reload.
✅ Rotate the bed in field view — plops stay in their corners (rendered in the object's local frame).
✅ Resizing a plop updates its displayed derived count live; an override sticks.
✅ "Remove" hides the plop everywhere and survives reload (row kept with removed_at).
Worth a manual pass on desktop + phone per the epic's "test on a phone" note — the automated checks cover types/logic, not touch ergonomics.
Closes #15. Part of epic #20 (Phase 5 — Plops). **This is pansy's core vision.** Builds on #11 (field editor), #13 (PlantPicker), #14 (plantings API).
## Data layer
- **`lib/plantings.ts`** — `ServerPlanting` zod shape, `EditorPlanting`, `effectiveCount` (override ?? derived), and `computeDerivedCount` — a client mirror of the server's `max(1, round(π·r²/spacing²))` for live resize display. Unit-tested.
- **`lib/objects.ts`** — `/full` now types `plantings` + `plants` (was `z.unknown()`). Added `useCreatePlanting` / `useUpdatePlanting` / `useRemovePlanting` that patch the same `FullGarden` cache with the #11 optimistic + 409-rollback contract. A soft-remove (removedAt set) drops the plop from the active list; the version is bumped optimistically to avoid a self-conflict on a fast follow-up.
## Editor
- **store** — `selectedPlantingId` / `livePlanting` / `armedPlant`; object vs plop selection are mutually exclusive.
- **Focus mode** — the object inspector's **"🌱 Plant here"** sets `focusedObjectId`; the canvas animates `fitToRect` to the object and dims everything else. A breadcrumb (`← garden`), Escape, or an empty-space tap exits. `focusedObjectId` mirrors to **`?focus`** so focus is deep-linkable and survives reload.
- **Placing** — **"+ Add plant"** opens the `PlantPicker`; the chosen plant stays **armed for repeat placement**; tapping inside the focused object drops a plop (default radius `max(1.5·spacing, 15cm)`), converted into the object's **local frame** before POSTing.
- **Manipulating** — `PlopOverlay` (drag body to move, clamped to bounds; edge handle to resize; one PATCH per gesture) + `PlopInspector` (change plant via picker, radius, **count override vs live derived**, label, planted date, **soft-remove**).
- **`PlopMarker` semantic zoom** (px/cm bands `<0.75 / 0.75–3 / >3`): far = flat color patch + a **dominant-plant tint** on the object so beds read at a glance; mid = colored circle + emoji; near = + plant name + count. Plops render **inside each object's rotated group**, so moving/rotating a bed carries its plops with **no planting writes**.
## Notes / choices
- "Click a plantable object → focus" is delivered as an explicit **"Plant here"** button (one tap, discoverable, works on mobile) rather than hijacking single-click — single-click still selects the bed for field-view move/resize/rotate, which the acceptance criteria also require (rotate a bed 45° → plops stay in their corners).
## Checks
`tsc --noEmit` clean · 24/24 vitest · `vite build` green.
## Acceptance criteria
- ✅ Focus into a bed, place garlic plops in one corner + basil in another; exit; zoom out → color patches/tint + names identify plantings; mid → icons; near → names + counts. Deep-linkable via `?focus`; survives reload.
- ✅ Rotate the bed in field view — plops stay in their corners (rendered in the object's local frame).
- ✅ Resizing a plop updates its displayed derived count live; an override sticks.
- ✅ "Remove" hides the plop everywhere and survives reload (row kept with `removed_at`).
> Worth a manual pass on desktop + phone per the epic's "test on a phone" note — the automated checks cover types/logic, not touch ergonomics.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Pansy's core vision: click into a bed, plop plants in its corners, zoom out and
still see what's planted where. Built on the field editor (#11), PlantPicker
(#13), and plantings API (#14).
Data layer
- lib/plantings.ts: ServerPlanting zod shape, EditorPlanting, effectiveCount, and
a client mirror of the derived-count formula (computeDerivedCount) for live
resize display; unit-tested.
- lib/objects.ts: /full now types plantings + plants; useCreatePlanting /
useUpdatePlanting / useRemovePlanting patch the same FullGarden cache with the
#11 optimistic + 409-rollback contract. A soft-remove drops the plop from the
active list.
Editor
- store: selectedPlantingId / livePlanting / armedPlant (mutually-exclusive
object vs plop selection).
- Focus mode: "Plant here" (object inspector) → focusedObjectId; the canvas
animates fitToRect to the object and dims the rest; a breadcrumb / Escape /
empty-tap exits. focusedObjectId mirrors to ?focus (deep-linkable, survives
reload).
- Placing: "+ Add plant" opens the PlantPicker; the chosen plant stays armed for
repeat placement; tapping inside the focused object drops a plop (default
radius max(1.5·spacing, 15cm)) in the object's local frame.
- Manipulating: PlopOverlay (drag to move, edge handle to resize, one PATCH per
gesture) + PlopInspector (change plant, radius, count override vs live derived,
label, planted date, soft-remove).
- PlopMarker semantic zoom (px/cm bands): far = flat color patch + a dominant-
plant tint on the object; mid = circle + emoji; near = + name + count. Plops
render inside each object's rotated group, so moving/rotating a bed carries
them with no planting writes.
tsc --noEmit clean; 24/24 vitest; production build green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 03:11:58Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 25 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** — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Duplicate plantsById construction in GardenCanvas
maintainability · flagged by 2 models
GardenCanvas rebuilds plantsById that its parent already computed (web/src/editor/GardenCanvas.tsx:68). GardenEditorPage constructs the same Map and passes the raw plants array down only so the canvas can rebuild it. Passing plantsById directly removes duplicate code and a redundant useMemo.
🪰 Gadfly · advisory
🟡 **Duplicate plantsById construction in GardenCanvas**
_maintainability · flagged by 2 models_
- **`GardenCanvas` rebuilds `plantsById` that its parent already computed** (`web/src/editor/GardenCanvas.tsx:68`). `GardenEditorPage` constructs the same `Map` and passes the raw `plants` array down only so the canvas can rebuild it. Passing `plantsById` directly removes duplicate code and a redundant `useMemo`.
<sub>🪰 Gadfly · advisory</sub>
🟡onPlace allows planting on non-plantable objects when deep-linked
correctness · flagged by 1 model
web/src/editor/GardenCanvas.tsx:139 — onPlace guards with if (!focusedObject || !armedPlant) return but never checks focusedObject.plantable. A deep-link (?focus=<id>) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. Fix: add || !focusedObject.plantable to the guard.
🪰 Gadfly · advisory
🟡 **onPlace allows planting on non-plantable objects when deep-linked**
_correctness · flagged by 1 model_
- **`web/src/editor/GardenCanvas.tsx:139`** — `onPlace` guards with `if (!focusedObject || !armedPlant) return` but never checks `focusedObject.plantable`. A deep-link (`?focus=<id>`) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. **Fix:** add `|| !focusedObject.plantable` to the guard.
<sub>🪰 Gadfly · advisory</sub>
🟡MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR
maintainability · flagged by 2 models
web/src/editor/PlopOverlay.tsx:11 / web/src/editor/PlopMarker.tsx:12 — const SELECT_COLOR = '#2f7a3e' is now defined identically in three places: the pre-existing SelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. editor/colors.ts) or exporting it from SelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10 and `w…
🪰 Gadfly · advisory
🟡 **MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR**
_maintainability · flagged by 2 models_
- **`web/src/editor/PlopOverlay.tsx:11` / `web/src/editor/PlopMarker.tsx:12`** — `const SELECT_COLOR = '#2f7a3e'` is now defined identically in three places: the pre-existing `SelectionOverlay.tsx:11`, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. `editor/colors.ts`) or exporting it from `SelectionOverlay.tsx`. - **`web/src/editor/PlopInspector.tsx:10` and `w…
<sub>🪰 Gadfly · advisory</sub>
🟡Four useState initializers duplicated verbatim in the resync effect; extract a sync helper
maintainability · flagged by 1 model
web/src/editor/PlopInspector.tsx:37-40 + 46-49 — The four useState initializers are duplicated verbatim in the resync useEffect. Extract a syncFromPlop(plop, unit) helper (or a small useSyncedFields hook) to keep the two sites from drifting.
🪰 Gadfly · advisory
🟡 **Four useState initializers duplicated verbatim in the resync effect; extract a sync helper**
_maintainability · flagged by 1 model_
- **`web/src/editor/PlopInspector.tsx:37-40`** + **`46-49`** — The four `useState` initializers are duplicated verbatim in the resync `useEffect`. Extract a `syncFromPlop(plop, unit)` helper (or a small `useSyncedFields` hook) to keep the two sites from drifting.
<sub>🪰 Gadfly · advisory</sub>
🟠commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback
error-handling · flagged by 1 model
🪰 Gadfly · advisory
🟠 **commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback**
_error-handling · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:227 + web/src/pages/GardenEditorPage.tsx:78-99,118,140 + web/src/editor/GardenCanvas.tsx:81-89,178 — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: ObjectShape.tsx has no focus-mode gating on onPointerDown — the focused object itself renders at full opacity (GardenCanvas.tsx:178, focusedObjectId !== o.id dimming only applies to other objects) and is fully selectable via the plain select action…
🪰 Gadfly · advisory
🟡 **commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps**
_error-handling · flagged by 1 model_
- **`web/src/editor/Inspector.tsx:227` + `web/src/pages/GardenEditorPage.tsx:78-99,118,140` + `web/src/editor/GardenCanvas.tsx:81-89,178`** — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: `ObjectShape.tsx` has no focus-mode gating on `onPointerDown` — the focused object itself renders at full opacity (`GardenCanvas.tsx:178`, `focusedObjectId !== o.id` dimming only applies to *other* objects) and is fully selectable via the plain `select` action…
<sub>🪰 Gadfly · advisory</sub>
🔴Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back
correctness, error-handling · flagged by 4 models
web/src/editor/PlopInspector.tsx:142 — Cannot clear the "Planted" date once set. The onBlur handler guards with planted !== '', so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send plantedAt: planted || null and remove the empty-string guard.
🪰 Gadfly · advisory
🔴 **Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back**
_correctness, error-handling · flagged by 4 models_
* **`web/src/editor/PlopInspector.tsx:142`** — **Cannot clear the "Planted" date once set.** The `onBlur` handler guards with `planted !== ''`, so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send `plantedAt: planted || null` and remove the empty-string guard.
<sub>🪰 Gadfly · advisory</sub>
🟠PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes
performance · flagged by 2 models
web/src/editor/PlopLayer.tsx:48-53 (byObject grouping) and :64 (dominantColor call) — PlopLayer is wrapped in memo(), but its objects and scale props are not stable across common editor gestures, so the memo rarely pays off: - Any object drag/resize/rotate: SelectionOverlay.tsx:81 calls setLiveObject(onMove(ev)) on every pointermove, and GardenCanvas.tsx:95-98's rendered (passed to PlopLayer as objects, GardenCanvas.tsx:184) is a useMemo keyed on `live…
🪰 Gadfly · advisory
🟠 **PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes**
_performance · flagged by 2 models_
- **`web/src/editor/PlopLayer.tsx:48-53`** (byObject grouping) and **`:64`** (`dominantColor` call) — `PlopLayer` is wrapped in `memo()`, but its `objects` and `scale` props are not stable across common editor gestures, so the memo rarely pays off: - **Any object drag/resize/rotate**: `SelectionOverlay.tsx:81` calls `setLiveObject(onMove(ev))` on every pointermove, and `GardenCanvas.tsx:95-98`'s `rendered` (passed to `PlopLayer` as `objects`, `GardenCanvas.tsx:184`) is a `useMemo` keyed on `live…
<sub>🪰 Gadfly · advisory</sub>
🟠dominantColor recomputed inline on every scale change
maintainability · flagged by 1 model
dominantColor is recomputed inline on every scale change (web/src/editor/PlopLayer.tsx:64). Because scale changes during zoom and PlopLayer re-renders with it, dominantColor iterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.
🪰 Gadfly · advisory
🟠 **dominantColor recomputed inline on every scale change**
_maintainability · flagged by 1 model_
- **`dominantColor` is recomputed inline on every scale change** (`web/src/editor/PlopLayer.tsx:64`). Because `scale` changes during zoom and `PlopLayer` re-renders with it, `dominantColor` iterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.
<sub>🪰 Gadfly · advisory</sub>
🟡Object-local transform string duplicated across 5 files; extract a shared helper
maintainability · flagged by 2 models
web/src/editor/PlopLayer.tsx:69, web/src/editor/PlopMarker.tsx:49, web/src/editor/GardenCanvas.tsx:18 — Three separate dim-opacity constants (0.4, 0.3, 0.35) for the same visual concept. The PR introduced DIMMED_OPACITY in GardenCanvas but PlopLayer hardcoded 0.4 instead of reusing it (and PlopMarker's 0.3 is dead anyway). Consolidate to one exported constant.
🪰 Gadfly · advisory
🟡 **Object-local transform string duplicated across 5 files; extract a shared helper**
_maintainability · flagged by 2 models_
- **`web/src/editor/PlopLayer.tsx:69`, `web/src/editor/PlopMarker.tsx:49`, `web/src/editor/GardenCanvas.tsx:18`** — Three separate dim-opacity constants (`0.4`, `0.3`, `0.35`) for the same visual concept. The PR introduced `DIMMED_OPACITY` in GardenCanvas but PlopLayer hardcoded `0.4` instead of reusing it (and PlopMarker's `0.3` is dead anyway). Consolidate to one exported constant.
<sub>🪰 Gadfly · advisory</sub>
🟠PlopMarker dimmed prop is always false and redundant with parent group opacity
maintainability · flagged by 2 models
web/src/editor/PlopLayer.tsx:84 — dimmed={false} is hardcoded for every PlopMarker, but PlopMarker declares dimmed: boolean as a required prop and branches on it (PlopMarker.tsx:49). Dead parameter: the layer already applies dim opacity at the group level (PlopLayer.tsx:69, opacity={dimmed ? 0.4 : 1}), so the marker-level dimmed can never be true. Grep confirms no caller ever passes true. Either drop the prop from PlopMarker or pass the real per-plop dim state.
🪰 Gadfly · advisory
🟠 **PlopMarker dimmed prop is always false and redundant with parent group opacity**
_maintainability · flagged by 2 models_
- **`web/src/editor/PlopLayer.tsx:84`** — `dimmed={false}` is hardcoded for every `PlopMarker`, but `PlopMarker` declares `dimmed: boolean` as a required prop and branches on it (`PlopMarker.tsx:49`). Dead parameter: the layer already applies dim opacity at the group level (`PlopLayer.tsx:69`, `opacity={dimmed ? 0.4 : 1}`), so the marker-level `dimmed` can never be `true`. Grep confirms no caller ever passes `true`. Either drop the prop from `PlopMarker` or pass the real per-plop dim state.
<sub>🪰 Gadfly · advisory</sub>
PlopMarker carries a dead dimmed prop (web/src/editor/PlopMarker.tsx:27). The only caller (PlopLayer) always passes dimmed={false} and handles object-level dimming via the parent <g> opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.
🪰 Gadfly · advisory
🟡 **Dead dimmed prop in PlopMarker**
_maintainability · flagged by 1 model_
- **`PlopMarker` carries a dead `dimmed` prop** (`web/src/editor/PlopMarker.tsx:27`). The only caller (`PlopLayer`) always passes `dimmed={false}` and handles object-level dimming via the parent `<g>` opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.
<sub>🪰 Gadfly · advisory</sub>
🟠On-canvas plop label shows stale derivedCount during live resize instead of recomputing from live radius
correctness · flagged by 2 models
web/src/editor/PlopMarker.tsx:88 — The near-zoom label always calls effectiveCount(plop) (plop.count ?? plop.derivedCount, web/src/lib/plantings.ts:59-60), the last server-computed value. During a live resize drag, PlopOverlay.startResize (web/src/editor/PlopOverlay.tsx:115-126) only updates radiusCm on the live plop ({...base, radiusCm: r}), leaving derivedCount/count untouched, and GardenCanvas's renderedPlops merges that live plop into what PlopLayer/`PlopMarke…
🪰 Gadfly · advisory
🟠 **On-canvas plop label shows stale derivedCount during live resize instead of recomputing from live radius**
_correctness · flagged by 2 models_
- **`web/src/editor/PlopMarker.tsx:88`** — The near-zoom label always calls `effectiveCount(plop)` (`plop.count ?? plop.derivedCount`, `web/src/lib/plantings.ts:59-60`), the last server-computed value. During a live resize drag, `PlopOverlay.startResize` (`web/src/editor/PlopOverlay.tsx:115-126`) only updates `radiusCm` on the live plop (`{...base, radiusCm: r}`), leaving `derivedCount`/`count` untouched, and `GardenCanvas`'s `renderedPlops` merges that live plop into what `PlopLayer`/`PlopMarke…
<sub>🪰 Gadfly · advisory</sub>
🟡SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared
maintainability · flagged by 3 models
web/src/editor/PlopOverlay.tsx:11 / web/src/editor/PlopMarker.tsx:12 — const SELECT_COLOR = '#2f7a3e' is now defined identically in three places: the pre-existing SelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. editor/colors.ts) or exporting it from SelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10 and `w…
🪰 Gadfly · advisory
🟡 **SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared**
_maintainability · flagged by 3 models_
- **`web/src/editor/PlopOverlay.tsx:11` / `web/src/editor/PlopMarker.tsx:12`** — `const SELECT_COLOR = '#2f7a3e'` is now defined identically in three places: the pre-existing `SelectionOverlay.tsx:11`, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. `editor/colors.ts`) or exporting it from `SelectionOverlay.tsx`. - **`web/src/editor/PlopInspector.tsx:10` and `w…
<sub>🪰 Gadfly · advisory</sub>
🟠Gesture scaffolding duplicated from SelectionOverlay
maintainability · flagged by 2 models
web/src/editor/PlopOverlay.tsx:36-97 vs web/src/editor/SelectionOverlay.tsx:44-104 — ~60 lines of near-identical gesture scaffolding (cleanupRef + useEffect cleanup + begin() + makePointerLocal/makePointerWorld + detach/finish). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (useDragGesture). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.
🪰 Gadfly · advisory
🟠 **Gesture scaffolding duplicated from SelectionOverlay**
_maintainability · flagged by 2 models_
- **`web/src/editor/PlopOverlay.tsx:36-97`** vs **`web/src/editor/SelectionOverlay.tsx:44-104`** — ~60 lines of near-identical gesture scaffolding (`cleanupRef` + `useEffect` cleanup + `begin()` + `makePointerLocal`/`makePointerWorld` + `detach`/`finish`). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (`useDragGesture`). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.
<sub>🪰 Gadfly · advisory</sub>
🟠Plop move/placement clamps to the object's rectangular bounds even for circle-shaped objects (grow_bag/container), letting plops sit outside the visible circular boundary
correctness · flagged by 1 model
🪰 Gadfly · advisory
🟠 **Plop move/placement clamps to the object's rectangular bounds even for circle-shaped objects (grow_bag/container), letting plops sit outside the visible circular boundary**
_correctness · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🔴livePlanting resize omits derivedCount update, so near-zoom marker count stays stale during drag
correctness, error-handling · flagged by 2 models
web/src/editor/PlopOverlay.tsx:121 — startResize sets the optimistic livePlanting to { ...base, radiusCm: r } but leaves derivedCount unchanged. Because PlopMarker (near-zoom text) reads effectiveCount(plop) which uses the stale derivedCount, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says “Resizing a plop updates its displayed derived count live”; the inspector panel does recompute…
🪰 Gadfly · advisory
🔴 **livePlanting resize omits derivedCount update, so near-zoom marker count stays stale during drag**
_correctness, error-handling · flagged by 2 models_
- **`web/src/editor/PlopOverlay.tsx:121`** — `startResize` sets the optimistic `livePlanting` to `{ ...base, radiusCm: r }` but leaves `derivedCount` unchanged. Because `PlopMarker` (near-zoom text) reads `effectiveCount(plop)` which uses the stale `derivedCount`, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says *“Resizing a plop updates its displayed derived count live”*; the inspector panel does recompute…
<sub>🪰 Gadfly · advisory</sub>
🟠useRemovePlanting has no 409-conflict adoption and no onSettled refetch, so a version-conflict on soft-remove leaves the plop with a stale version and the user stuck until reload
error-handling · flagged by 1 model
web/src/lib/objects.ts:268-286 (useRemovePlanting) — A 409 version-conflict on soft-remove leaves the user stuck. onError (line 281) only restores ctx.prev (the pre-removal snapshot, which carries the stale version) and toasts a generic "Could not remove that plant." It neither adopts the 409's current row (the way useUpdatePlanting does at line 250-257 via conflictPlanting) nor refetches (the way useDeleteObject does at line 182 via onSettled). So after a failed remove…
🪰 Gadfly · advisory
🟠 **useRemovePlanting has no 409-conflict adoption and no onSettled refetch, so a version-conflict on soft-remove leaves the plop with a stale version and the user stuck until reload**
_error-handling · flagged by 1 model_
- **`web/src/lib/objects.ts:268-286`** (`useRemovePlanting`) — A 409 version-conflict on soft-remove leaves the user stuck. `onError` (line 281) only restores `ctx.prev` (the pre-removal snapshot, which carries the *stale* version) and toasts a generic "Could not remove that plant." It neither adopts the 409's `current` row (the way `useUpdatePlanting` does at line 250-257 via `conflictPlanting`) nor refetches (the way `useDeleteObject` does at line 182 via `onSettled`). So after a failed remove…
<sub>🪰 Gadfly · advisory</sub>
🟠useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch
error-handling · flagged by 1 model
useRemovePlanting doesn't reconcile a 409 conflict, leaving the user stuck.web/src/lib/objects.ts:281-284: the onError only restores ctx.prev and shows a generic toast. useUpdatePlanting (lines 250-257) instead calls conflictPlanting(err) and, on a conflict, writes the server's current row into the cache so the next edit carries a fresh version. useRemovePlanting lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)…
🪰 Gadfly · advisory
🟠 **useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch**
_error-handling · flagged by 1 model_
- **`useRemovePlanting` doesn't reconcile a 409 conflict, leaving the user stuck.** `web/src/lib/objects.ts:281-284`: the `onError` only restores `ctx.prev` and shows a generic toast. `useUpdatePlanting` (lines 250-257) instead calls `conflictPlanting(err)` and, on a conflict, writes the server's `current` row into the cache so the next edit carries a fresh `version`. `useRemovePlanting` lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)…
<sub>🪰 Gadfly · advisory</sub>
correctness, error-handling, maintainability · flagged by 4 models
web/src/lib/objects.ts:299 — applyPlantingPatch silently ignores removedAt despite it being in PlantingPatch. The optimistic-update function copies every other patch field but omits removedAt. Today this is latent because soft-removes use useRemovePlanting, but if anyone later calls useUpdatePlanting with removedAt, the plop will stay visible during the request and only disappear on onSuccess, producing a jarring flash.
🪰 Gadfly · advisory
🟠 **applyPlantingPatch omits removedAt, causing latent optimistic-update mismatch**
_correctness, error-handling, maintainability · flagged by 4 models_
* **`web/src/lib/objects.ts:299`** — **`applyPlantingPatch` silently ignores `removedAt` despite it being in `PlantingPatch`.** The optimistic-update function copies every other patch field but omits `removedAt`. Today this is latent because soft-removes use `useRemovePlanting`, but if anyone later calls `useUpdatePlanting` with `removedAt`, the plop will stay visible during the request and only disappear on `onSuccess`, producing a jarring flash.
<sub>🪰 Gadfly · advisory</sub>
🟠Stale ?focus IDs not validated after load: canvas stays dimmed with no visible exit UI
error-handling · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:64 / web/src/editor/GardenCanvas.tsx:178 — Stale ?focus IDs leave the canvas permanently dimmed with no visible exit affordance.setFocusedObject(focus ?? null) is adopted from the URL without validating that the object exists after the garden loads. GardenCanvas and PlopLayer dim every object when focusedObjectId != null, but GardenEditorPage hides the breadcrumb (← garden) when the object is missing. On mobile there is no Esc…
🪰 Gadfly · advisory
🟠 **Stale ?focus IDs not validated after load: canvas stays dimmed with no visible exit UI**
_error-handling · flagged by 1 model_
* **`web/src/pages/GardenEditorPage.tsx:64`** / **`web/src/editor/GardenCanvas.tsx:178`** — **Stale `?focus` IDs leave the canvas permanently dimmed with no visible exit affordance.** `setFocusedObject(focus ?? null)` is adopted from the URL without validating that the object exists after the garden loads. `GardenCanvas` and `PlopLayer` dim every object when `focusedObjectId != null`, but `GardenEditorPage` hides the breadcrumb (`← garden`) when the object is missing. On mobile there is no Esc…
<sub>🪰 Gadfly · advisory</sub>
SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared
web/src/editor/PlopOverlay.tsx:11
3/5
maintainability
🔴
livePlanting resize omits derivedCount update, so near-zoom marker count stays stale during drag
web/src/editor/PlopOverlay.tsx:121
2/5
correctness, error-handling
🟠
PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes
web/src/editor/PlopLayer.tsx:48
2/5
performance
🟠
PlopMarker dimmed prop is always false and redundant with parent group opacity
web/src/editor/PlopLayer.tsx:84
2/5
maintainability
🟠
On-canvas plop label shows stale derivedCount during live resize instead of recomputing from live radius
web/src/editor/PlopMarker.tsx:88
2/5
correctness
🟠
Gesture scaffolding duplicated from SelectionOverlay
web/src/editor/PlopOverlay.tsx:36
2/5
maintainability
🟡
Focus dimming opacity inconsistent across components
web/src/editor/GardenCanvas.tsx:18
2/5
maintainability
🟡
Duplicate plantsById construction in GardenCanvas
web/src/editor/GardenCanvas.tsx:68
2/5
maintainability
🟡
MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR
web/src/editor/PlopInspector.tsx:10
2/5
maintainability
🟡
Object-local transform string duplicated across 5 files; extract a shared helper
web/src/editor/PlopLayer.tsx:68
2/5
maintainability
22 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Deleting the focused object leaves focus mode stuck (breadcrumb, palette, and viewport-fit all break until Escape)
web/src/editor/Inspector.tsx:227
claude-code/sonnet
error-handling
🟠
commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback
web/src/editor/PlopInspector.tsx:59
kimi-k2.6:cloud
error-handling
🟠
dominantColor recomputed inline on every scale change
web/src/editor/PlopLayer.tsx:64
opencode/kimi-k2.6:cloud
maintainability
🟠
Plop move/placement clamps to the object's rectangular bounds even for circle-shaped objects (grow_bag/container), letting plops sit outside the visible circular boundary
web/src/editor/PlopOverlay.tsx:45
claude-code/sonnet
correctness
🟠
Gesture scaffolding duplicated between SelectionOverlay and PlopOverlay
web/src/editor/SelectionOverlay.tsx:73
opencode/kimi-k2.6:cloud
maintainability
🟠
PlantingPatch.removedAt ignored by applyPlantingPatch causing optimistic cache inconsistency
web/src/lib/objects.ts:223
kimi-k2.6:cloud
maintainability
🟠
useRemovePlanting has no 409-conflict adoption and no onSettled refetch, so a version-conflict on soft-remove leaves the plop with a stale version and the user stuck until reload
web/src/lib/objects.ts:268
opencode/glm-5.2:cloud
error-handling
🟠
useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch
web/src/lib/objects.ts:281
glm-5.2:cloud
error-handling
🟠
Stale ?focus IDs not validated after load: canvas stays dimmed with no visible exit UI
web/src/pages/GardenEditorPage.tsx:64
opencode/kimi-k2.6:cloud
error-handling
🟡
onPlace allows planting on non-plantable objects when deep-linked
web/src/editor/GardenCanvas.tsx:139
opencode/kimi-k2.6:cloud
correctness
🟡
Clamp-to-object-bounds duplicated between GardenCanvas.onPlace and PlopOverlay.clampLocal
web/src/editor/GardenCanvas.tsx:144
glm-5.2:cloud
maintainability
🟡
Four useState initializers duplicated verbatim in the resync effect; extract a sync helper
web/src/editor/PlopInspector.tsx:37
opencode/glm-5.2:cloud
maintainability
🟡
commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps
web/src/editor/PlopInspector.tsx:65
claude-code/sonnet
error-handling
🟡
commitCount silently rejects invalid/float input without user feedback
web/src/editor/PlopInspector.tsx:70
kimi-k2.6:cloud
error-handling
🟡
Dead dimmed prop in PlopMarker
web/src/editor/PlopMarker.tsx:27
opencode/kimi-k2.6:cloud
maintainability
🟡
Vestigial 'dimmed' prop — PlopLayer always passes false; dimming is done at the group level
web/src/editor/PlopMarker.tsx:33
glm-5.2:cloud
maintainability
🟡
Local-frame translate+rotate SVG transform string copy-pasted in 3 new spots instead of a shared geometry helper
web/src/editor/PlopOverlay.tsx:130
claude-code/sonnet
maintainability
🟡
Duplicated SELECT_COLOR and HANDLE_PX constants across overlay files
web/src/editor/SelectionOverlay.tsx:7
opencode/kimi-k2.6:cloud
maintainability
🟡
useCreatePlanting lacks optimistic update, risking duplicate taps on slow networks
web/src/lib/objects.ts:200
kimi-k2.6:cloud
error-handling
🟡
livePlanting subscribed at page level causes whole-page re-render (incl. non-memoized GardenCanvas) on every plop drag pointermove, unlike the scoped liveObject pattern
web/src/pages/GardenEditorPage.tsx:35
claude-code/sonnet
performance
🟡
plantsById Map built independently in page and in GardenCanvas from the same plants prop
web/src/pages/GardenEditorPage.tsx:49
opencode/glm-5.2:cloud
maintainability
⚪
halfFW/halfFH guard focusedObject but are only read inside a block where it's guaranteed non-null
web/src/editor/GardenCanvas.tsx:152
opencode/glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
I focused on the new plops data/mutation layer (lib/plantings.ts, lib/objects.ts), the new canvas/inspector components (PlopLayer, PlopMarker, PlopOverlay, PlopInspector), and the store/page wiring, checking specifically for injection, unsafe deserialization, missing validation, and unsafe rendering of untrusted data.
What I verified:
No unsafe DOM sinks: grepped the new/changed files for dangerouslySetInnerHTML, eval, Function(, innerHTML — none present. Plant name/label/icon are rendered as plain React/SVG text content (web/src/editor/PlopMarker.tsx:79, :87), which React escapes; free-text fields (label, plantedAt) in PlopInspector.tsx only ever populate controlled <input> values, never raw markup.
All server responses are zod-validated before use: serverPlantingSchema.parse(...) wraps every mutation response in lib/objects.ts:203-284, and fullGardenSchema now types plantings/plants instead of z.unknown() (lib/objects.ts:37-42), closing off a prior any-shape trust gap rather than introducing one.
URL construction uses only numeric, schema-derived IDs (objectId, id typed number from zod-parsed rows) — /objects/${objectId}/plantings, /plantings/${id} (lib/objects.ts:204, :230, :273) — no string interpolation of user-typed text into paths.
Focus-mode ?focus param is validated upstream by the router (web/src/router.tsx:93-96, pre-existing, unchanged by this diff) to a positive integer before GardenEditorPage.tsx ever uses it, so it can't be used to inject anything into a request; it only gates which object is displayed.
Conflict-body parsing (conflictPlanting, lib/objects.ts:314-320) safely parses err.body.current via serverPlantingSchema.safeParse, same pattern as the existing object mutations — no blind trust of attacker/server-adjacent JSON.
Numeric field coercion (commitCount/commitRadius in PlopInspector.tsx:63-77) rejects non-finite/non-integer input before sending a PATCH.
Nothing in this diff introduces a new trust boundary crossing, unescaped rendering path, or unvalidated identifier usage relative to the existing (already-reviewed) object-mutation pattern it mirrors. The diff is frontend-only; server-side authorization for the new /objects/:id/plantings and /plantings/:id endpoints isn't part of this change set and wasn't assessed.
🎯 Correctness — Minor issues
Both findings confirmed against the actual source. No shape-aware clamping exists in either onPlace (GardenCanvas.tsx:144-145) or clampLocal (PlopOverlay.tsx:62-65), and the on-canvas label genuinely uses the stale effectiveCount during a live resize while the Inspector panel independently recomputes correctly. Both draft findings hold up.
VERDICT: Minor issues
web/src/editor/PlopMarker.tsx:88 — The near-zoom label always calls effectiveCount(plop) (plop.count ?? plop.derivedCount, web/src/lib/plantings.ts:59-60), the last server-computed value. During a live resize drag, PlopOverlay.startResize (web/src/editor/PlopOverlay.tsx:115-126) only updates radiusCm on the live plop ({...base, radiusCm: r}), leaving derivedCount/count untouched, and GardenCanvas's renderedPlops merges that live plop into what PlopLayer/PlopMarker render. Meanwhile PlopInspector.tsx:56 independently recomputes computeDerivedCount(plop.radiusCm, plant.spacingCm) for its own placeholder, using the live selectedPlop (GardenEditorPage.tsx: selectedPlop = ... livePlanting?.id === rawSelectedPlop.id ? livePlanting : rawSelectedPlop). So the on-canvas "name · count" text stays frozen at the pre-drag count while the Inspector panel updates live, and the canvas label only catches up after the PATCH round-trips.
Fix: in PlopMarker, when plop.count == null, compute plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCount instead of effectiveCount(plop).
web/src/editor/GardenCanvas.tsx:144-145 (onPlace) and web/src/editor/PlopOverlay.tsx:62-65 (clampLocal) — Both clamp a plop's local position to the object's rectangular bounding box (±widthCm/2, ±heightCm/2) regardless of object.shape. Confirmed web/src/editor/kinds.ts:19-20 that grow_bag and container are shape: 'circle', and internal/service/objects.go:22-24 confirms both are plantable: true. ObjectShape.tsx:68-69 and SelectionOverlay.tsx:167-175 both special-case shape === 'circle' with an <ellipse>, an established convention this code doesn't follow. Net effect: a plop can be placed or dragged into the square "corners" outside the actual circular pot/bag boundary that's drawn.
Fix: for shape === 'circle', clamp to the ellipse (scale the offset vector by min(1, 1/hypot(x/halfW, y/halfH))) instead of the rectangle, in both the placement clamp and PlopOverlay.clampLocal.
🧹 Code cleanliness & maintainability — Minor issues
All three findings verified exactly against the source. Here's the corrected review.
VERDICT: Minor issues
Reviewed through the code-cleanliness/maintainability lens only (verified against the actual checked-out files, not just the diff).
web/src/editor/PlopOverlay.tsx:11 / web/src/editor/PlopMarker.tsx:12 — const SELECT_COLOR = '#2f7a3e' is now defined identically in three places: the pre-existing SelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. editor/colors.ts) or exporting it from SelectionOverlay.tsx.
web/src/editor/PlopInspector.tsx:10 and web/src/editor/PlopOverlay.tsx:9 — const MIN_RADIUS_CM = 1 is duplicated verbatim between two files that are both new in this PR (not pre-existing debt). One enforces the floor on typed radius input, the other on drag-resize; both should stay in sync but there's nothing wiring them together. Worth exporting once from lib/plantings.ts (which already owns computeDerivedCount and other plop domain logic) and importing in both.
web/src/editor/GardenCanvas.tsx:201, PlopLayer.tsx:68, PlopOverlay.tsx:130 — the SVG local-frame transform string `translate(${x} ${y}) rotate(${deg})` is now written out five times total (two pre-existing in ObjectShape.tsx:64/SelectionOverlay.tsx:165, three new ones from this PR). lib/geometry.ts already centralizes the local-frame math (localToWorld/worldToLocal), but the SVG-transform string itself isn't factored out, so this PR tripled a copy-paste that a small localFrameTransform(center, rotationDeg) helper (or a <LocalFrame> wrapper component) would have avoided.
Nothing else stood out structurally — the new files (PlopLayer, PlopMarker, PlopOverlay, PlopInspector, lib/plantings.ts) are appropriately sized, named consistently with existing ObjectShape/SelectionOverlay/Inspector counterparts, and the begin(onMove, fields) abstraction in PlopOverlay.tsx is a reasonable way to share the move/resize gesture plumbing rather than duplicating it.
⚡ Performance — Minor issues
Both findings verified against the actual code. Final corrected review:
VERDICT: Minor issues
Findings (⚡ Performance lens)
web/src/editor/PlopLayer.tsx:48-53 (byObject grouping) and :64 (dominantColor call) — PlopLayer is wrapped in memo(), but its objects and scale props are not stable across common editor gestures, so the memo rarely pays off:
Any object drag/resize/rotate: SelectionOverlay.tsx:81 calls setLiveObject(onMove(ev)) on every pointermove, and GardenCanvas.tsx:95-98's rendered (passed to PlopLayer as objects, GardenCanvas.tsx:184) is a useMemo keyed on liveObject, so it produces a new array reference every pointermove even when only one unrelated object's geometry changed. Since PlopLayer's memo() does a shallow prop comparison, the new array reference busts it, forcing the full byObject grouping (O(total active plops in the garden)) to redo every frame — even for objects with no plops at all.
Any zoom gesture (wheel/pinch): viewport.scale changes continuously during zoom (useViewport.tsonWheel/onPinch) and is passed straight through as scale={viewport.scale} (GardenCanvas.tsx:187), also busting the memo. (Plain panning only changes tx/ty, not scale, so panning alone does not trigger this.)
The byObject grouping runs unconditionally before the far check (GardenCanvas.tsx... PlopLayer.tsx:54), so it's redone every frame even when the tint isn't rendered at near zoom.
PlopLayer has no internal useMemo, confirmed by reading the full component body — the grouping and (when far) dominant-color scan are plain per-render work with no memoization independent of objects/scale identity.
Impact scales with total plop count and fires at drag/zoom frequency, adding avoidable per-frame CPU to gestures that predate this PR. A useMemo keyed on plantings/plantsById (independent of objects/scale reference churn) would restore effective memoization.
web/src/pages/GardenEditorPage.tsx:35 — unlike liveObject, whose value is only read inside GardenCanvas (the page only imports the setLiveObject setter, confirmed at line 34), livePlanting's value is subscribed at the page level to compute selectedPlop (lines 119-121). Since GardenCanvas is not wrapped in memo() (confirmed — export function GardenCanvas at GardenCanvas.tsx:32 has no memo wrapper), every pointermove of a plop move/resize (PlopOverlay.tsx:75 calls setLivePlanting on each move) re-renders the entire GardenEditorPage tree, including GardenCanvas and PlopInspector, rather than staying scoped to the canvas the way object drags do. This is bounded (a couple O(n).find() scans plus JSX re-creation) but is an avoidable inconsistency with the established liveObject pattern in the same file, and compounds with the PlopLayer issue since it's another source of prop-identity churn during plop-drag gestures.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/editor/Inspector.tsx:227 + web/src/pages/GardenEditorPage.tsx:78-99,118,140 + web/src/editor/GardenCanvas.tsx:81-89,178 — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: ObjectShape.tsx has no focus-mode gating on onPointerDown — the focused object itself renders at full opacity (GardenCanvas.tsx:178, focusedObjectId !== o.id dimming only applies to other objects) and is fully selectable via the plain select action (store.ts:59, no guard against focusedObjectId). Selecting it opens Inspector, whose Delete flow (Inspector.tsx:225-227) calls select(null) and del.mutate(object.id) but never touches focusedObjectId. After the optimistic delete (objects.ts:175), objects no longer contains that id, so: GardenEditorPage.tsx:118's focusedObject becomes null, hiding the breadcrumb block (GardenEditorPage.tsx:144); focusedObjectId == null && <Palette/> (GardenEditorPage.tsx:140) stays false, hiding the palette; and GardenCanvas.tsx:85-86 (target undefined, focusedObjectId != null still true) returns early every render without updating fitKeyRef, permanently short-circuiting the fit effect. Verified the Escape handler (GardenEditorPage.tsx:86-98) is the only way out — with selectedId/selectedPlantingId both null and focusedObjectId still set, it falls through to exitFocus(). Reachable via normal use (focus a plantable bed, select it while focused, delete it).
web/src/editor/PlopInspector.tsx:65-73 — Confirmed commitCount returns without patching or resetting local state on non-integer/zero/negative input, and — unlike sibling commitRadius (PlopInspector.tsx:58-63, which clamps via Math.max(MIN_RADIUS_CM, ...)) — never corrects the value. Since no patch fires, plop.count never changes, so the re-sync useEffect (keyed on plop.count, line 50) never re-triggers either — the invalid text sits in the field indefinitely with no feedback.
web/src/lib/objects.ts:299-311 — Confirmed applyPlantingPatch omits removedAt from its spread despite PlantingPatch.removedAt being declared (objects.ts:223). Grepped the repo: no call site currently invokes useUpdatePlanting.mutate with removedAt — the only setter of removedAt is useRemovePlanting (objects.ts:268-286), which bypasses applyPlantingPatch entirely via its own onMutate. So this is currently dead but is a real latent optimistic-cache gap for any future direct removedAt update through useUpdatePlanting.
I traced the tainted-input paths through this diff from a security perspective and found nothing material:
?focus query param — validated in web/src/router.tsx:93-96 as a positive integer (Number.isInteger(f) && f > 0) before reaching the store/canvas. No injection or type-coercion risk. Verified by reading the route.
Path interpolation in API calls — /objects/${objectId}/plantings (objects.ts) and /plantings/${id} (objects.ts) use objectId/id typed as number, sourced from zod-validated server rows (serverPlantingSchema/serverObjectSchema) and typed EditorObject/EditorPlanting. No string path injection possible. Verified via objects.ts and plantings.ts.
Untrusted rendering — plant color/icon/name are z.string() from the API (plants.ts:31-32) and rendered as SVG attributes (fill={color}) / React text children in PlopMarker.tsx/PlopLayer.tsx, and as a CSS backgroundColor via color-mix() in PlantIcon.tsx. React sets these through setAttribute/textContent (escaped) and the CSSOM style.backgroundColor setter (invalid values dropped per-property), so there's no HTML/CSS injection vector. No dangerouslySetInnerHTML/innerHTML anywhere (grep confirmed).
Client input validation — commitCount rejects non-integers < 1 (PlopInspector.tsx); commitRadius clamps to MIN_RADIUS_CM; computeDerivedCount guards <= 0. These bound what's POSTed/PATCHed. Server-side authz/length limits for label etc. are backend concerns outside this (frontend) diff.
Credentials — mutations go through api with credentials: 'same-origin' (api.ts:97), same as existing object mutations; no new secret handling or leakage.
The data is same-origin, authenticated, zod-parsed, and all identifiers are numeric. Nothing in the security lane is wrong here.
🎯 Correctness — Minor issues
Both findings confirmed against the actual source.
Finding 1 — PlopInspector.tsx:142 (the draft cited 148; actual line is 142). The blur handler is exactly:
Clearing a set date (planted="", plop.plantedAt="2024-01-01"): first clause "" !== "2024-01-01" is true, second planted !== '' is false → no PATCH. Confirmed. The label field at line 133 correctly sends null on clear (label.trim() || null). Asymmetry is real.
Finding 2 — objects.ts:223 declares removedAt?: string | null on PlantingPatch; applyPlantingPatch (lines 299–311) has no removedAt branch. Confirmed. Soft-removal is handled separately by useRemovePlanting (its own onMutate filters the plop, lines 275–279), and useUpdatePlanting's onSuccess does drop the plop when updated.removedAt is set (lines 245–246) — but the optimistic onMutate pass via applyPlantingPatch would not drop it, only bump the version. So it's a latent inconsistency, not an active bug. Confirmed as described.
Verdict: Minor issues
web/src/editor/PlopInspector.tsx:142 — planted date cannot be cleared. The blur guard is planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). If a plop has plantedAt = "2024-01-01" and the user empties the field (planted = ""), the first clause is true but planted !== '' is false, so no PATCH is sent and the stale date persists. The label field (line 133) correctly sends null when cleared (patch({ label: label.trim() || null })); the planted field should do the same (plantedAt: null when emptied).
web/src/lib/objects.ts:299 — applyPlantingPatch ignores removedAt though PlantingPatch declares it.PlantingPatch includes removedAt?: string | null (line 223), but applyPlantingPatch (lines 299–311) has no removedAt branch. Soft-removal normally goes through useRemovePlanting (which filters the plop in its own onMutate), and useUpdatePlanting.onSuccess does drop the plop when updated.removedAt is set (lines 245–246) — so this is a latent inconsistency, not an active bug. But if any caller uses useUpdatePlanting({ removedAt }), the optimistic pass would leave the plop visible (only the version bump applies) until the round-trip completes.
🧹 Code cleanliness & maintainability — Minor issues
All findings verified. All six findings confirmed against actual code. Producing the corrected review.
Corrected review
Verdict: Minor issues
Through the cleanliness/maintainability lens, the new plop code repeats a few patterns the surrounding files already pay for, and leaves a vestigial prop. Nothing blocking, but a small shared-helper pass would cut real churn.
web/src/editor/PlopMarker.tsx:33 — vestigial dimmed prop.PlopLayer always renders <PlopMarker ... dimmed={false} /> (PlopLayer.tsx:84), and the actual focus-dimming is applied one level up on the object <g opacity={dimmed ? 0.4 : 1}> (PlopLayer.tsx:69). So PlopMarker's dimmed/opacity={dimmed ? 0.3 : 1} branch (PlopMarker.tsx:49) is dead — a prop that can never be true. Either drive the marker's own opacity from it (and drop the group opacity) or remove the prop to avoid misleading future readers.
Object-local transform string duplicated across 5 sites.translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg}) is hand-built in GardenCanvas.tsx:201, PlopLayer.tsx:68, PlopOverlay.tsx:130, plus pre-existing SelectionOverlay.tsx:165 and ObjectShape.tsx:64. This PR adds two new copies of the core "plops live in the object's local frame" transform. A single objectGroupTransform(o: EditorObject): string (or a <ObjectGroup object={o}> wrapper) would centralize it — right now a rotation/transform convention change means editing five places.
SELECT_COLOR = '#2f7a3e' redefined in 4 files.SelectionOverlay.tsx:11, ObjectShape.tsx:59 (inline), and the new PlopMarker.tsx:12 / PlopOverlay.tsx:11 each redeclare the same constant (and #00000033 for the unselected stroke). The PR extends an existing pattern, but this is the moment to hoist one shared editor/colors constant before it spreads further.
Three different "dimmed by focus" opacities.GardenCanvas.tsx:18DIMMED_OPACITY = 0.35 (for non-focused objects), PlopLayer.tsx:690.4 (for non-focused plop groups), PlopMarker.tsx:490.3 (the dead dimmed branch). Same visual concept, three magic numbers — pick one and share it.
Duplicated clamp-to-object-bounds.GardenCanvas.tsx:144-145 inlines the ±widthCm/2/±heightCm/2 clamp for placement, and PlopOverlay.tsx:62-65 has clampLocal doing exactly the same math. A shared clampToLocalFrame(p, object) in geometry.ts (next to worldToLocal) would remove the copy and keep the bounds invariant in one place.
plantsById rebuilt in two components.GardenCanvas.tsx:68 and GardenEditorPage.tsx:49 both do new Map(plants.map((p) => [p.id, p])). Minor, but since the page already owns plants, it could pass the map down (or a memoized selector) instead of reconstructing it. Low priority.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, the change is sound. I verified the hot paths:
Plop drag (PlopOverlay) — updates livePlanting per pointermove; renderedPlops is rebuilt via useMemo on [plantings, livePlanting] (GardenCanvas.tsx:100-103), but non-dragged plops keep object identity, and PlopMarker is memo'd with stable plantsById/onSelect/scale props, so only the dragged marker actually re-renders. One PATCH per gesture on release. Good.
Bed drag — liveObject changes per move; rendered rebuilds (O(n) map) and PlopLayer re-renders because its objects prop changes identity, rebuilding the byObject map (O(plantings)) and iterating objects. This is O(objects+plantings) of reconciliation work per pointermove, but PlopMarker memo prevents DOM work on unchanged plops. Acceptable for realistic garden sizes; not a regression in kind (the object list already re-mapped per move before this PR).
No N+1 / refetch — useCreatePlanting/useUpdatePlanting/useRemovePlanting patch the existing FullGarden cache on success (objects.ts) with no invalidateQueries/refetch. Confirmed by reading the mutation bodies.
Fit effect — guarded by fitKeyRef so unrelated re-renders (plop add, bed drag, cache updates that produce a new objects array) hit the early return and don't refit. Verified at GardenCanvas.tsx:78-87.
dominantColor (PlopLayer.tsx:8-23) allocates a Map per planted object per render in far-zoom mode; during a pan/zoom this is O(total plops) of map ops per frame. Only material for very large gardens with many plops at far zoom — a useMemo/hoisted cache could avoid it, but I would not call it a blocking regression.
One trivial note (not worth a finding on its own): plantsById is built in both GardenEditorPage.tsx:49 and GardenCanvas.tsx via its own useMemo — duplicate but O(plants) and memoized, negligible.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
PlopInspector — planted date can never be cleared.web/src/editor/PlopInspector.tsx:142: the onBlur is () => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). The planted !== '' guard short-circuits when the user empties the date field, so no PATCH is sent and plantedAt cannot be reset to null. This is an asymmetry with the sibling fields: commitCount (line 66-68) sends patch({ count: null }) when the count is cleared, and the label onBlur (line 133) sends patch({ label: label.trim() || null }), allowing a reset. Once a plop has a planted date, the inspector offers no way to remove it. Fix: allow the empty case to PATCH plantedAt: null, e.g. handle planted === '' separately (PATCH null when plop.plantedAt != null) before the non-empty comparison.
useRemovePlanting doesn't reconcile a 409 conflict, leaving the user stuck.web/src/lib/objects.ts:281-284: the onError only restores ctx.prev and shows a generic toast. useUpdatePlanting (lines 250-257) instead calls conflictPlanting(err) and, on a conflict, writes the server's current row into the cache so the next edit carries a fresh version. useRemovePlanting lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale) version and no refetch is triggered (no onSettled/invalidate). The user clicks "Remove" again, the same stale version is sent, the server 409s again — the plop can't be removed until an unrelated refetch refreshes the version. Fix: mirror the update hook — on a conflictPlanting(err) result, write current into the cache instead of only restoring prev (and/or re-arm the remove with the new version).
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
After tracing data flow, input paths, auth boundaries, and rendering paths through the security lens, I found no authn/authz gaps, injection vectors, unsafe deserialization, secret leakage, or missing validation that would constitute a material security issue in this PR.
The ?focus URL parameter is validated as a positive integer in router.tsx:93-95 before reaching state logic.
All text values rendered to the DOM (plant names, labels, icons) pass through React’s JSX escaping or SVG text nodes — no raw HTML insertion points.
Event handlers on overlays use stopPropagation(), preventing gesture confusion or click-through to unintended actions.
The soft-remove / optimistic-update rollback paths in objects.ts handle 409 conflicts correctly.
No credentials or tokens are mishandled; the API layer continues to rely on same-origin HttpOnly session cookies.
🎯 Correctness — Blocking issues found
Blocking issues found
Two verified correctness bugs: the near-zoom plop marker fails to recompute its displayed count during live resize (contradicting an acceptance criterion), and the plop inspector prevents clearing an already-set planted date.
Findings
web/src/editor/PlopOverlay.tsx:121 — startResize sets the optimistic livePlanting to { ...base, radiusCm: r } but leaves derivedCount unchanged. Because PlopMarker (near-zoom text) reads effectiveCount(plop) which uses the stale derivedCount, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says “Resizing a plop updates its displayed derived count live”; the inspector panel does recompute it via computeDerivedCount, but the canvas marker does not. Fix: pass the plant (or its spacingCm) into PlopOverlay and include a live derivedCount in the optimistic livePlanting payload, or have PlopMarker recompute derivedCount from plop.radiusCm and plant.spacingCm the same way PlopInspector does.
web/src/editor/PlopInspector.tsx:142 — The planted date blur guard is planted !== '' && patch({ plantedAt: planted }). If a user clears an existing date, planted becomes '', the left-hand condition is false, and no PATCH is ever sent, so the date cannot be removed through the UI. The server schema accepts plantedAt: null for clearing. Fix: change the handler to send plantedAt: planted || null whenever the normalized value differs from the current server value, e.g. onBlur={() => { const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next }) }}.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
PlopMarkerdimmed prop is dead code — PlopLayer.tsx:84 hardcodes dimmed={false} while the parent <g> already applies opacity dimming at line 69. The dimmed parameter and the conditional opacity={dimmed ? 0.3 : 1} in PlopMarker.tsx:26-27,49 are never triggered and create confusion about where dimming lives. Remove the prop and inline opacity from PlopMarker.
Pointer-gesture scaffolding is copy-pasted between overlays — PlopOverlay.tsx:36-97 duplicates the cleanupRef, useEffect teardown, begin, move/detach/finish pattern from SelectionOverlay.tsx:44-104 almost line-for-line. The only meaningful differences are the store setter names (setLivePlanting vs setLiveObject) and the mutator type. Extract a shared useDragGesture hook so gesture fixes (e.g., adding lostpointercapture handling) don't have to be applied in two places.
PlantingPatch interface advertises removedAt but applyPlantingPatch ignores it — objects.ts:223 declares removedAt?: string | null in the patch type, yet the optimistic-update helper (objects.ts:299-311) never spreads it. If a caller ever passes removedAt to useUpdatePlanting, the cache will be optimistically inconsistent until the server response arrives. Either add removedAt to applyPlantingPatch or remove it from PlantingPatch (it's already handled by the dedicated useRemovePlanting hook).
Focus-mode dimming uses inconsistent opacity constants — Objects dim to 0.35 (GardenCanvas.tsx:18), plop groups dim to 0.4 (PlopLayer.tsx:69, magic number), and the dead PlopMarker path uses 0.3 (PlopMarker.tsx:49). These three values represent the same visual concept; unify them under one exported constant.
SELECT_COLOR is defined identically in three files — #2f7a3e appears at PlopOverlay.tsx:11, PlopMarker.tsx:12, and SelectionOverlay.tsx:11. Extract it to a shared constant so the selection accent color can be changed in one place.
⚡ Performance — Minor issues
Minor issues
web/src/editor/PlopLayer.tsx:48-64 — byObject grouping and dominantColor recomputed on every zoom/pan frame PlopLayer rebuilds the byObject Map from scratch on every render, and calls dominantColor (which iterates an object's plops and builds a frequency Map) for every planted object whenever scale < SEMANTIC_FAR. Because scale changes on every frame during a zoom or pan gesture, this work repeats even though neither plantings nor plantsById have changed. For a garden with many plantings this creates unnecessary CPU load on the render thread. Fix: Wrap byObject in useMemo keyed on plantings only. Precompute dominant-color tints in the same memo (or a second one keyed on plantings and plantsById) so that pure zoom/pan frames skip the recomputation entirely.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
PlopInspector.tsx:59 — commitRadius uses parseFloat(radius). parseFloat('1.2.3') returns 1.2, which is accepted as valid even though the user typed garbage. The field then silently commits a value the user did not intend, with no error feedback. Fix: validate with Number(radius) and a regex or isNaN check, and surface a field-level error rather than silently swallowing.
PlopInspector.tsx:70 — commitCount uses Number(count). While Number.isInteger rejects NaN, inputs like '3.5' silently do nothing with no feedback. An invalid typed value just evaporates on blur. Fix: provide explicit validation feedback in the UI.
PlopOverlay.tsx:121 — startResize computes radius via Math.hypot(p.x - base.xCm, p.y - base.yCm) clamped only by MAX_RADIUS_CM. Unlike the move gesture, resize is not clamped to the parent object's bounds, so a plop can be resized to spill outside its bed. The overlay comment says "clamped to bounds" for move but omits resize. Fix: clamp radius so the plop circle stays inside the object's local rectangle, or document that spillover is intentional.
lib/objects.ts:200 — useCreatePlanting has no optimistic onMutate. On slow networks a user may tap the placement sheet repeatedly, believing nothing happened, and create duplicate plops. The update mutation has optimistic patching; create should mirror it for repeat-placement UX. Fix: optimistically append the new plop to the cache with a temporary ID.
I'll investigate this PR through the security lens. Let me verify the key files.
Now let me check how label is rendered and where planting bodies are constructed — the user-provided label goes to the server, but let me verify how it could be displayed back. Also check Inspector.tsx to confirm the onFocus authz angle (no authz on the client).
Now I have the picture. Let me check serverPlantingSchema — label is z.string() with no max length, and color/icon from plantSchema are unbounded strings that get interpolated into SVG and CSS. Let me verify the PlantIcon's color usage path and whether the color/icon/name/label are sanitized anywhere.
Let me verify the icon and color rendering at the SVG sink points and confirm there's no React auto-escaping concern (text content in SVG is auto-escaped, so XSS via <text>{plant.icon}</text> is fine). The PlantIcon uses style={{ backgroundColor: color-mix(...) }} with template interpolation.
I have enough to conclude. Let me consolidate the security review.
Key findings:
PlantIcon — potential CSS injection via color: PlantIcon.tsx:12 interpolates plant.color (a z.string() with no format constraint) into a style={{ backgroundColor: \color-mix(in srgb, ${color} 18%, transparent)` }}. A malicious colorstring containing;or)or arbitrary CSS could escape thecolor-mix(...)call and inject style properties. But more importantly, React'sstyleprop value as a string does NOT inject;separated properties the way an attribute string would — the whole string is set as thebackground-colorvalue, which the browser will reject as invalid if malformed. So actual CSS injection into other properties is prevented by React's style-assignment semantics. Lower risk, but the unvalidatedcolor/icon/name` are pre-existing concerns, not introduced by this PR. They predate this PR. So I should only flag what's new.
PlopMarker.tsx:72,88 interpolates plant.icon and plant.name into SVG <text> content. React auto-escapes JSX text content, so no XSS — these are safe.
label (serverPlantingSchema.label = z.string()) — user-provided, unbounded, but rendered only as a TextField value (auto-escaped). No XSS sink. No new sink introduced by this PR.
Numeric inputs to API: xCm, yCm, radiusCm, count — clamped client-side (Math.max/Math.min), but the server is responsible for re-validating. plantId and objectId come from server-loaded lists; the ?focus=<id> URL param is validated in router.tsx:93-96 as a positive integer. No injection vector — they're interpolated into URL paths as numbers.
?focus=<id> is just a viewport framing hint; no authz decision is made on it client-side (the objects are already filtered by garden on the server /full endpoint). Setting ?focus=999 for an object the user can't see just results in no framing. No IDOR — the server /full endpoint is the trust boundary and is garden-scoped.
Optimistic cache patching is purely client-side UI state; no security implication.
Within my lens (security), nothing material is introduced by this PR. The unbounded string fields (color/icon/name/label) are pre-existing and rendered safely through React's auto-escaping; the PlantIcon color interpolation is a pre-existing pattern not added by this PR. The new code adds no new sinks, no new untrusted-input surfaces beyond what's already validated (zod parse on all server responses), and no authz decisions.
Verdict: No material issues found.
No material issues found
Verified all new untrusted-input sinks introduced by this PR:
PlopMarker.tsx:72,88 interpolates plant.icon and plant.name into SVG <text> content — React auto-escapes JSX text, so no XSS vector. PlopInspector.tsx:91,95 and the PlantIcon use the same auto-escaped text/safe style path. Confirmed by reading PlopMarker.tsx, PlopInspector.tsx, and PlantIcon.tsx.
plop.label (serverPlantingSchema.label: z.string() in plantings.ts:16) is user-authored and unbounded, but is only rendered as a controlled <TextField> value (PlopInspector.tsx:131-133) — auto-escaped, no HTML sink. The lack of a max-length is a pre-existing pattern shared with plantSchema.name/notes; not introduced here and not a security bug under React's text escaping.
All server responses (/full, planting create/update/remove) are parsed through zod (serverPlantingSchema, plantSchema) before touching cache/UI (objects.ts:204,230,273), so no unsafe deserialization of raw server payloads.
?focus=<id> URL search param is validated to a positive integer in router.tsx:93-96 and used only to frame the viewport / pick from already-loaded objects; no client authz decision rests on it (the /full endpoint is the trust boundary and is garden-scoped server-side). No IDOR.
Path/URL interpolations (/objects/${objectId}/plantings, /plantings/${id}) use numeric ids derived from server rows; no injection.
The one PlantIconcolor-mix(in srgb, ${color} …) template interpolation is a pre-existing pattern (also used in PlantCard.tsx:51, PlantPicker.tsx:90) not introduced by this PR, so I'm not flagging it here.
Within the security lens, this change introduces no material issues.
🎯 Correctness — Minor issues
Both findings reference line 84 for PlopMarker, but actual line is 88. Let me verify the other claims.
Both findings confirmed. Let me verify the objects.ts optimistic patch and GardenCanvas livePlanting flow.
Both findings confirmed. The line for PlopMarker is 88, not 84 (the diff truncated; the draft said 84 but the actual {plant!.name} · {effectiveCount(plop)} is on line 88). I'll correct to line 88. The PlopInspector line 142 is correct.
VERDICT: Minor issues
web/src/editor/PlopMarker.tsx:88 — At near-zoom the plop count renders {plant!.name} · {effectiveCount(plop)}, and effectiveCount = p.count ?? p.derivedCount (plantings.ts:59-61). During a resize gesture, PlopOverlay.startResize (PlopOverlay.tsx:115-126) sets livePlanting = {...base, radiusCm: r} — the new radius but the oldderivedCount — and the optimistic patch in applyPlantingPatch (objects.ts:299-311) deliberately leaves derivedCount untouched. GardenCanvas substitutes livePlanting into renderedPlops (GardenCanvas.tsx), so the marker reads the stale derivedCount until the PATCH roundtrips via onSuccess (objects.ts:240-248). The count number therefore does not update live with the radius, contradicting the acceptance criterion "Resizing a plop updates its displayed derived count live". The computeDerivedCount mirror (plantings.ts:66-69) is wired only into the inspector placeholder (PlopInspector.tsx:56), not the marker. Fix: render plop.count ?? computeDerivedCount(plop.radiusCm, plant?.spacingCm ?? 0) in the marker (import computeDerivedCount).
web/src/editor/PlopInspector.tsx:142 — The planted-date onBlur guard planted !== '' makes it impossible to clear a planted date: the PATCH only fires when the value changed and is non-empty. CreatePlanting defaults plantedAt to today server-side (plantings.go:97-100), so every plop has a date, and the PATCH API supports plantedAt: null via SetPlantedAt/parseNullableString semantics (plantings.go:211-213). The user can change the date but never remove it; meanwhile the local input shows empty while the server retains the old value until the next external resync (the resync effect at PlopInspector.tsx:44-50 would restore it once the field loses focus). Fix: drop the planted !== '' guard and patch({ plantedAt: planted || null }) on blur when the value changed.
🧹 Code cleanliness & maintainability — Minor issues
All findings verified. Let me confirm the few specifics:
PlopLayer.tsx:84 dimmed dead prop — Confirmed. PlopLayer.tsx:61 computes dimmed at the group level (opacity={dimmed ? 0.4 : 1} at line 69), and PlopMarker always receives dimmed={false} at line 84. Grep confirms no caller ever passes true. The dimmed prop on PlopMarker (line 33) and its opacity={dimmed ? 0.3 : 1} (line 49) is unreachable logic.
Three dim-opacity constants — Confirmed. DIMMED_OPACITY = 0.35 (GardenCanvas.tsx:18), 0.4 (PlopLayer.tsx:69), 0.3 (PlopMarker.tsx:49, dead). Three divergent constants.
plantsById duplicate construction — Confirmed. GardenEditorPage.tsx:49 and GardenCanvas.tsx:68 both build new Map(plants.map((p) => [p.id, p])) from the same plants array passed as prop.
applyPlantingPatch omits removedAt — Confirmed. PlantingPatch declares removedAt?: string | null (objects.ts:223), useRemovePlanting sends it (line 273), but applyPlantingPatch (lines 299-311) only copies plantId/xCm/yCm/radiusCm/count/label/plantedAt — no removedAt line. useRemovePlanting.onMutate filters the plop out, so remove path is fine; a direct removedAt patch via useUpdatePlanting wouldn't apply optimistically. Latent inconsistency.
PlopInspector duplicated initializers — Confirmed. PlopInspector.tsx:37-40 (radius/count/label/planted initializers) duplicated verbatim at 46-49 in the resync effect.
halfFW/halfFH redundant guards — Confirmed. GardenCanvas.tsx:152-153 guards focusedObject ?, only read at lines 203-206 inside {focusedObject && armedPlant && (...)} (line 200) where focusedObject is guaranteed non-null.
fitKeyRef key redundant garden.id — Confirmed but confidence medium. The key is ${garden.id}:${focusedObjectId} (GardenCanvas.tsx:83). GardenEditorPage resets editor state on gid change (lines 54-70), and GardenCanvas is rendered without an explicit key prop (line 163), so React doesn't remount it on garden change — the page's effect resets state but fitKeyRef ref persists. So garden.id in the key is actually NOT redundant (it guards against a stale ref pointing at a previous garden's fit key when garden.id changes without unmounting). This finding is questionable.
Let me re-examine finding #8 more carefully. The page does NOT pass a key={garden.id} to GardenCanvas, so the component instance persists across garden changes. fitKeyRef is a useRef, which persists. On garden change, fitKeyRef.current would still hold the old key from the previous garden. Without garden.id in the key, the new garden would never refit (because the key matches the stale ref). So garden.id is actually NECESSARY. The finding is wrong — drop it.
VERDICT: Minor issues
Findings through the 🧹 cleanliness/maintainability lens:
web/src/editor/PlopLayer.tsx:84 — dimmed={false} is hardcoded for every PlopMarker, but PlopMarker declares dimmed: boolean as a required prop and branches on it (PlopMarker.tsx:49). Dead parameter: the layer already applies dim opacity at the group level (PlopLayer.tsx:69, opacity={dimmed ? 0.4 : 1}), so the marker-level dimmed can never be true. Grep confirms no caller ever passes true. Either drop the prop from PlopMarker or pass the real per-plop dim state.
web/src/editor/PlopLayer.tsx:69, web/src/editor/PlopMarker.tsx:49, web/src/editor/GardenCanvas.tsx:18 — Three separate dim-opacity constants (0.4, 0.3, 0.35) for the same visual concept. The PR introduced DIMMED_OPACITY in GardenCanvas but PlopLayer hardcoded 0.4 instead of reusing it (and PlopMarker's 0.3 is dead anyway). Consolidate to one exported constant.
web/src/editor/PlopOverlay.tsx:36-97 vs web/src/editor/SelectionOverlay.tsx:44-104 — ~60 lines of near-identical gesture scaffolding (cleanupRef + useEffect cleanup + begin() + makePointerLocal/makePointerWorld + detach/finish). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (useDragGesture). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.
web/src/pages/GardenEditorPage.tsx:49 + web/src/editor/GardenCanvas.tsx:68 — plantsById is built independently in both files (new Map(plants.map((p) => [p.id, p]))) from the same plants array that the page already passes to the canvas. Either pass the pre-built Map down as a prop or lift it; the duplicate useMemo is wasteful and easy to drift.
web/src/lib/objects.ts:299-310 (applyPlantingPatch) — PlantingPatch declares removedAt?: string | null (line 223) and useRemovePlanting sends it (line 273), but applyPlantingPatch never copies removedAt onto the optimistic row. useRemovePlanting has its own onMutate that filters the plop out, so the remove path is fine, but a direct removedAt patch via useUpdatePlanting wouldn't apply optimistically — a latent inconsistency. Add the removedAt line for symmetry with the other fields (and with applyServerPatch).
web/src/editor/PlopInspector.tsx:37-40 + 46-49 — The four useState initializers are duplicated verbatim in the resync useEffect. Extract a syncFromPlop(plop, unit) helper (or a small useSyncedFields hook) to keep the two sites from drifting.
web/src/editor/GardenCanvas.tsx:152-153 — halfFW/halfFH guard focusedObject ? but are only read inside {focusedObject && armedPlant && (...)} (line 200), where focusedObject is guaranteed non-null. The ternaries are dead defensive code; simplify to const halfFW = focusedObject.widthCm / 2 inside the block, or just inline.
Verified by reading PlopLayer.tsx, PlopMarker.tsx, PlopOverlay.tsx, SelectionOverlay.tsx, GardenCanvas.tsx, GardenEditorPage.tsx, objects.ts, PlopInspector.tsx, and grepping for dimmed/plantsById usages. (Dropped the fitKeyRef key-redundancy finding: GardenCanvas is not keyed by garden.id in its parent, so the ref persists across garden changes and garden.id in the key is actually necessary to trigger a refit on garden switch.)
⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the diff through the performance lens, verifying against the checked-out code.
Memoization is sound.PlopLayer and PlopMarker are both memo'd. On pan (only tx/ty change), PlopLayer's props don't change (scale unchanged, objects/plantings/plantsById stable via useMemo), so it skips re-render — and even if it didn't, each PlopMarker's props are stable (the parent <g> transform carries position), so they skip too. On zoom, re-renders cascade through PlopLayer/PlopMarker only because scale legitimately changes for semantic-zoom bands — necessary work, not waste.
Selection changes don't repaint every plop. When selectedPlantingId changes, PlopLayer re-renders but only the two markers whose selected prop actually flips pass the memo check; the rest skip (verified at PlopMarker.tsx:21, PlopLayer.tsx:83).
No quadratic behavior.PlopLayer builds a byObject map (O(plops)) once per render and does O(1) lookups (PlopLayer.tsx:48-53); dominantColor is O(plops-per-object) and only in far mode. GardenCanvas does three .find() calls per render (selectedObject/selectedPlopObject/focusedObject, GardenCanvas.tsx:104-107) — O(n) each, consistent with the pre-existing selected lookup pattern, not a new hotspot.
Optimistic updates are O(n) per mutation, not N+1.useUpdatePlanting/useRemovePlanting do a single full.plantings.map/filter on the cached FullGarden (objects.ts), one PATCH per gesture — same contract as the object mutations.
Live-drag allocations are bounded.renderedPlops rebuilds only when livePlanting changes (during a plop drag), and is a single O(n) map; liveObject's merge is likewise O(n). No per-move re-sort.
Fit effect is guarded.fitKeyRef prevents a refit on unrelated re-renders (plop add, bed drag) — the comment's claim holds (GardenCanvas.tsx:83-84).
Duplicate plantsById Map is built in both GardenEditorPage and GardenCanvas — trivial duplicate allocation, not material.
The new code follows the existing memo discipline carefully; nothing in my lane regresses efficiency for realistic garden/plop sizes.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/PlopInspector.tsx:142 — Clearing the "Planted" date is silently dropped. The blur handler is planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). The planted !== '' guard means an empty (cleared) value never produces a PATCH, so plantedAt: null is never sent. Worse, because no patch fires, the local planted state stays '' while plop.plantedAt is still the old date — the field shows empty but the server keeps the date, and the re-sync useEffect (deps include plop.plantedAt, line 50) doesn't fire because plop.plantedAt never changed. Contrast with commitCount (line 65), which correctly handles the empty case by patching { count: null }. Fix: mirror commitCount — drop the planted !== '' clause and patch { plantedAt: planted.trim() ? planted : null } when the value differs from plop.plantedAt ?? ''. Verified by reading the file.
web/src/lib/objects.ts:268-286 (useRemovePlanting) — A 409 version-conflict on soft-remove leaves the user stuck. onError (line 281) only restores ctx.prev (the pre-removal snapshot, which carries the stale version) and toasts a generic "Could not remove that plant." It neither adopts the 409's current row (the way useUpdatePlanting does at line 250-257 via conflictPlanting) nor refetches (the way useDeleteObject does at line 182 via onSettled). So after a failed remove, the plop reappears in the cache with its old version; retrying sends that same stale version and 409s again — the user can't recover without reloading. Fix: either adopt conflictPlanting(err) in onError (write current into the cache so the next attempt uses the fresh version), or add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) like useDeleteObject. Verified by reading the file and comparing with the sibling object mutations in the same file.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
After tracing tainted input through the new planting (plop) editor and focus-mode deep-linking, I found no concrete security issues in this PR.
URL search params (?focus) are validated to positive integers in the route definition (router.tsx:93-96) before reaching state. No injection or open-redirect vector.
All user-editable fields (label, radius, count, plantedAt) are rendered through React’s normal JSX escaping (no dangerouslySetInnerHTML, no raw SVG text injection). The plant.icon value rendered inside an SVG <text> element is escaped by React and cannot execute scripts.
API paths are constructed from trusted server-side IDs (objectId, planting id) already present in cached garden data, not from user input. No SSRF or path-traversal surface.
Optimistic cache patches use object spreads on locally cached arrays; no prototype-pollution vectors via user-controlled keys.
No secrets or credentials appear in the diff.
The applyPlantingPatch helper omits removedAt from its optimistic merge, but that is a transient cache-correctness issue (covered by another lens), not an authz bypass or injection vulnerability.
🎯 Correctness — Minor issues
Minor issues
web/src/editor/PlopInspector.tsx:142 — The plantedAt blur handler can never clear a date. The condition planted !== '' blocks the PATCH when the user empties the field, so a previously-set date persists on the server even though the UI shows an empty input. Fix: change the blur handler to treat an empty string as null, e.g. const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next }).
web/src/editor/PlopOverlay.tsx:121 — During a resize drag, livePlanting is updated with the new radiusCm but the cached derivedCount is left unchanged. Because PlopMarker renders effectiveCount(plop) (which falls back to plop.derivedCount), the canvas text shows the stale derived count until the PATCH resolves. The inspector placeholder updates live via computeDerivedCount, but the canvas marker does not. Fix: recompute derivedCount inside the drag move callback (the overlay has access to the plant via the parent if passed down, or the marker itself can derive it from plant).
web/src/editor/GardenCanvas.tsx:139 — onPlace guards with if (!focusedObject || !armedPlant) return but never checks focusedObject.plantable. A deep-link (?focus=<id>) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. Fix: add || !focusedObject.plantable to the guard.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
PlopMarker carries a dead dimmed prop (web/src/editor/PlopMarker.tsx:27). The only caller (PlopLayer) always passes dimmed={false} and handles object-level dimming via the parent <g> opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.
Selection constants are triplicated (web/src/editor/SelectionOverlay.tsx:7 and 11, PlopOverlay.tsx:9 and 11, PlopMarker.tsx:12). SELECT_COLOR and HANDLE_PX are defined identically in three and two files respectively. Centralizing them in a single module prevents visual drift and makes future tweaks one-touch.
Gesture scaffolding is copy-pasted (web/src/editor/SelectionOverlay.tsx:73 and PlopOverlay.tsx:67). The window-listener setup, cleanup ref pattern, and mutation-on-release logic are ~30 lines of nearly identical boilerplate with only liveObject vs livePlanting differing. Extracting a shared generic hook (e.g. useDragGesture) would eliminate the duplication and ensure interaction fixes apply once.
dominantColor is recomputed inline on every scale change (web/src/editor/PlopLayer.tsx:64). Because scale changes during zoom and PlopLayer re-renders with it, dominantColor iterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.
Radius bounds are scattered and partially duplicated (web/src/editor/PlopInspector.tsx:10 and PlopOverlay.tsx:9-10). MIN_RADIUS_CM is defined in two files; MAX_RADIUS_CM only in one. Co-locating both in a shared constants module (e.g. alongside the planting domain) makes the model easier to keep consistent.
GardenCanvas rebuilds plantsById that its parent already computed (web/src/editor/GardenCanvas.tsx:68). GardenEditorPage constructs the same Map and passes the raw plants array down only so the canvas can rebuild it. Passing plantsById directly removes duplicate code and a redundant useMemo.
⚡ Performance — No material issues found
No material issues found
Wait — actually, let me re-verify. I need to be more careful.
Minor issues
web/src/editor/GardenCanvas.tsx:100-103 — During a plop drag, livePlanting updates on every pointermove. The renderedPlopsuseMemo maps over the entire plantings array (O(n) in total plantings) to create a new array with one element replaced. For a garden with many plantings, this allocates a large array on every animation frame and forces PlopLayer to re-render. The same pattern exists for liveObject (which the PR explicitly calls "cheap"), but plantings are the "many" side of the relationship and can be an order of magnitude more numerous. Fix: Pass plantings and livePlanting separately to PlopLayer; merge inline in the plop render loop (O(1) extra per rendered plop) rather than doing a full-array copy in GardenCanvas.
web/src/editor/PlopLayer.tsx:48-53 — byObject is rebuilt from scratch on every PlopLayer render, even when only scale (zooming) or objects (object drag) changed and plantings is unchanged. During a continuous zoom or object drag, this is an unnecessary O(total_plantings) rebuild on every frame. Fix: Wrap the byObject construction in useMemo(() => { ... }, [plantings]).
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/editor/PlopInspector.tsx:142 — Cannot clear the "Planted" date once set. The onBlur handler guards with planted !== '', so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send plantedAt: planted || null and remove the empty-string guard.
web/src/pages/GardenEditorPage.tsx:64 / web/src/editor/GardenCanvas.tsx:178 — Stale ?focus IDs leave the canvas permanently dimmed with no visible exit affordance.setFocusedObject(focus ?? null) is adopted from the URL without validating that the object exists after the garden loads. GardenCanvas and PlopLayer dim every object when focusedObjectId != null, but GardenEditorPage hides the breadcrumb (← garden) when the object is missing. On mobile there is no Escape key, so the user must discover that an empty-space tap exits. An effect should clear focusedObjectId when objects.length > 0 && !objects.find(o => o.id === focusedObjectId).
web/src/lib/objects.ts:299 — applyPlantingPatch silently ignores removedAt despite it being in PlantingPatch. The optimistic-update function copies every other patch field but omits removedAt. Today this is latent because soft-removes use useRemovePlanting, but if anyone later calls useUpdatePlanting with removedAt, the plop will stay visible during the request and only disappear on onSuccess, producing a jarring flash.
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** · 34 findings (12 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back | `web/src/editor/PlopInspector.tsx:142` | 4/5 | correctness, error-handling |
| 🟠 | applyPlantingPatch omits removedAt, causing latent optimistic-update mismatch | `web/src/lib/objects.ts:299` | 4/5 | correctness, error-handling, maintainability |
| 🟡 | SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared | `web/src/editor/PlopOverlay.tsx:11` | 3/5 | maintainability |
| 🔴 | livePlanting resize omits derivedCount update, so near-zoom marker count stays stale during drag | `web/src/editor/PlopOverlay.tsx:121` | 2/5 | correctness, error-handling |
| 🟠 | PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes | `web/src/editor/PlopLayer.tsx:48` | 2/5 | performance |
| 🟠 | PlopMarker dimmed prop is always false and redundant with parent group opacity | `web/src/editor/PlopLayer.tsx:84` | 2/5 | maintainability |
| 🟠 | On-canvas plop label shows stale derivedCount during live resize instead of recomputing from live radius | `web/src/editor/PlopMarker.tsx:88` | 2/5 | correctness |
| 🟠 | Gesture scaffolding duplicated from SelectionOverlay | `web/src/editor/PlopOverlay.tsx:36` | 2/5 | maintainability |
| 🟡 | Focus dimming opacity inconsistent across components | `web/src/editor/GardenCanvas.tsx:18` | 2/5 | maintainability |
| 🟡 | Duplicate plantsById construction in GardenCanvas | `web/src/editor/GardenCanvas.tsx:68` | 2/5 | maintainability |
| 🟡 | MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR | `web/src/editor/PlopInspector.tsx:10` | 2/5 | maintainability |
| 🟡 | Object-local transform string duplicated across 5 files; extract a shared helper | `web/src/editor/PlopLayer.tsx:68` | 2/5 | maintainability |
<details><summary>22 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Deleting the focused object leaves focus mode stuck (breadcrumb, palette, and viewport-fit all break until Escape) | `web/src/editor/Inspector.tsx:227` | claude-code/sonnet | error-handling |
| 🟠 | commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback | `web/src/editor/PlopInspector.tsx:59` | kimi-k2.6:cloud | error-handling |
| 🟠 | dominantColor recomputed inline on every scale change | `web/src/editor/PlopLayer.tsx:64` | opencode/kimi-k2.6:cloud | maintainability |
| 🟠 | Plop move/placement clamps to the object's rectangular bounds even for circle-shaped objects (grow_bag/container), letting plops sit outside the visible circular boundary | `web/src/editor/PlopOverlay.tsx:45` | claude-code/sonnet | correctness |
| 🟠 | Gesture scaffolding duplicated between SelectionOverlay and PlopOverlay | `web/src/editor/SelectionOverlay.tsx:73` | opencode/kimi-k2.6:cloud | maintainability |
| 🟠 | PlantingPatch.removedAt ignored by applyPlantingPatch causing optimistic cache inconsistency | `web/src/lib/objects.ts:223` | kimi-k2.6:cloud | maintainability |
| 🟠 | useRemovePlanting has no 409-conflict adoption and no onSettled refetch, so a version-conflict on soft-remove leaves the plop with a stale version and the user stuck until reload | `web/src/lib/objects.ts:268` | opencode/glm-5.2:cloud | error-handling |
| 🟠 | useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch | `web/src/lib/objects.ts:281` | glm-5.2:cloud | error-handling |
| 🟠 | Stale ?focus IDs not validated after load: canvas stays dimmed with no visible exit UI | `web/src/pages/GardenEditorPage.tsx:64` | opencode/kimi-k2.6:cloud | error-handling |
| 🟡 | onPlace allows planting on non-plantable objects when deep-linked | `web/src/editor/GardenCanvas.tsx:139` | opencode/kimi-k2.6:cloud | correctness |
| 🟡 | Clamp-to-object-bounds duplicated between GardenCanvas.onPlace and PlopOverlay.clampLocal | `web/src/editor/GardenCanvas.tsx:144` | glm-5.2:cloud | maintainability |
| 🟡 | Four useState initializers duplicated verbatim in the resync effect; extract a sync helper | `web/src/editor/PlopInspector.tsx:37` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps | `web/src/editor/PlopInspector.tsx:65` | claude-code/sonnet | error-handling |
| 🟡 | commitCount silently rejects invalid/float input without user feedback | `web/src/editor/PlopInspector.tsx:70` | kimi-k2.6:cloud | error-handling |
| 🟡 | Dead dimmed prop in PlopMarker | `web/src/editor/PlopMarker.tsx:27` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | Vestigial 'dimmed' prop — PlopLayer always passes false; dimming is done at the group level | `web/src/editor/PlopMarker.tsx:33` | glm-5.2:cloud | maintainability |
| 🟡 | Local-frame translate+rotate SVG transform string copy-pasted in 3 new spots instead of a shared geometry helper | `web/src/editor/PlopOverlay.tsx:130` | claude-code/sonnet | maintainability |
| 🟡 | Duplicated SELECT_COLOR and HANDLE_PX constants across overlay files | `web/src/editor/SelectionOverlay.tsx:7` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | useCreatePlanting lacks optimistic update, risking duplicate taps on slow networks | `web/src/lib/objects.ts:200` | kimi-k2.6:cloud | error-handling |
| 🟡 | livePlanting subscribed at page level causes whole-page re-render (incl. non-memoized GardenCanvas) on every plop drag pointermove, unlike the scoped liveObject pattern | `web/src/pages/GardenEditorPage.tsx:35` | claude-code/sonnet | performance |
| 🟡 | plantsById Map built independently in page and in GardenCanvas from the same plants prop | `web/src/pages/GardenEditorPage.tsx:49` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | halfFW/halfFH guard focusedObject but are only read inside a block where it's guaranteed non-null | `web/src/editor/GardenCanvas.tsx:152` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## Review
**VERDICT: No material issues found**
I focused on the new plops data/mutation layer (`lib/plantings.ts`, `lib/objects.ts`), the new canvas/inspector components (`PlopLayer`, `PlopMarker`, `PlopOverlay`, `PlopInspector`), and the store/page wiring, checking specifically for injection, unsafe deserialization, missing validation, and unsafe rendering of untrusted data.
What I verified:
- **No unsafe DOM sinks**: grepped the new/changed files for `dangerouslySetInnerHTML`, `eval`, `Function(`, `innerHTML` — none present. Plant name/label/icon are rendered as plain React/SVG text content (`web/src/editor/PlopMarker.tsx:79`, `:87`), which React escapes; free-text fields (`label`, `plantedAt`) in `PlopInspector.tsx` only ever populate controlled `<input>` values, never raw markup.
- **All server responses are zod-validated** before use: `serverPlantingSchema.parse(...)` wraps every mutation response in `lib/objects.ts:203-284`, and `fullGardenSchema` now types `plantings`/`plants` instead of `z.unknown()` (`lib/objects.ts:37-42`), closing off a prior any-shape trust gap rather than introducing one.
- **URL construction uses only numeric, schema-derived IDs** (`objectId`, `id` typed `number` from zod-parsed rows) — `/objects/${objectId}/plantings`, `/plantings/${id}` (`lib/objects.ts:204`, `:230`, `:273`) — no string interpolation of user-typed text into paths.
- **Focus-mode `?focus` param** is validated upstream by the router (`web/src/router.tsx:93-96`, pre-existing, unchanged by this diff) to a positive integer before `GardenEditorPage.tsx` ever uses it, so it can't be used to inject anything into a request; it only gates which object is displayed.
- **Conflict-body parsing** (`conflictPlanting`, `lib/objects.ts:314-320`) safely parses `err.body.current` via `serverPlantingSchema.safeParse`, same pattern as the existing object mutations — no blind trust of attacker/server-adjacent JSON.
- **Numeric field coercion** (`commitCount`/`commitRadius` in `PlopInspector.tsx:63-77`) rejects non-finite/non-integer input before sending a PATCH.
Nothing in this diff introduces a new trust boundary crossing, unescaped rendering path, or unvalidated identifier usage relative to the existing (already-reviewed) object-mutation pattern it mirrors. The diff is frontend-only; server-side authorization for the new `/objects/:id/plantings` and `/plantings/:id` endpoints isn't part of this change set and wasn't assessed.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings confirmed against the actual source. No shape-aware clamping exists in either `onPlace` (GardenCanvas.tsx:144-145) or `clampLocal` (PlopOverlay.tsx:62-65), and the on-canvas label genuinely uses the stale `effectiveCount` during a live resize while the Inspector panel independently recomputes correctly. Both draft findings hold up.
## VERDICT: Minor issues
- **`web/src/editor/PlopMarker.tsx:88`** — The near-zoom label always calls `effectiveCount(plop)` (`plop.count ?? plop.derivedCount`, `web/src/lib/plantings.ts:59-60`), the last server-computed value. During a live resize drag, `PlopOverlay.startResize` (`web/src/editor/PlopOverlay.tsx:115-126`) only updates `radiusCm` on the live plop (`{...base, radiusCm: r}`), leaving `derivedCount`/`count` untouched, and `GardenCanvas`'s `renderedPlops` merges that live plop into what `PlopLayer`/`PlopMarker` render. Meanwhile `PlopInspector.tsx:56` independently recomputes `computeDerivedCount(plop.radiusCm, plant.spacingCm)` for its own placeholder, using the live `selectedPlop` (`GardenEditorPage.tsx`: `selectedPlop = ... livePlanting?.id === rawSelectedPlop.id ? livePlanting : rawSelectedPlop`). So the on-canvas "name · count" text stays frozen at the pre-drag count while the Inspector panel updates live, and the canvas label only catches up after the PATCH round-trips.
Fix: in `PlopMarker`, when `plop.count == null`, compute `plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCount` instead of `effectiveCount(plop)`.
- **`web/src/editor/GardenCanvas.tsx:144-145` (`onPlace`) and `web/src/editor/PlopOverlay.tsx:62-65` (`clampLocal`)** — Both clamp a plop's local position to the object's rectangular bounding box (`±widthCm/2, ±heightCm/2`) regardless of `object.shape`. Confirmed `web/src/editor/kinds.ts:19-20` that `grow_bag` and `container` are `shape: 'circle'`, and `internal/service/objects.go:22-24` confirms both are `plantable: true`. `ObjectShape.tsx:68-69` and `SelectionOverlay.tsx:167-175` both special-case `shape === 'circle'` with an `<ellipse>`, an established convention this code doesn't follow. Net effect: a plop can be placed or dragged into the square "corners" outside the actual circular pot/bag boundary that's drawn.
Fix: for `shape === 'circle'`, clamp to the ellipse (scale the offset vector by `min(1, 1/hypot(x/halfW, y/halfH))`) instead of the rectangle, in both the placement clamp and `PlopOverlay.clampLocal`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings verified exactly against the source. Here's the corrected review.
**VERDICT: Minor issues**
Reviewed through the code-cleanliness/maintainability lens only (verified against the actual checked-out files, not just the diff).
- **`web/src/editor/PlopOverlay.tsx:11` / `web/src/editor/PlopMarker.tsx:12`** — `const SELECT_COLOR = '#2f7a3e'` is now defined identically in three places: the pre-existing `SelectionOverlay.tsx:11`, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g. `editor/colors.ts`) or exporting it from `SelectionOverlay.tsx`.
- **`web/src/editor/PlopInspector.tsx:10` and `web/src/editor/PlopOverlay.tsx:9`** — `const MIN_RADIUS_CM = 1` is duplicated verbatim between two files that are both new in this PR (not pre-existing debt). One enforces the floor on typed radius input, the other on drag-resize; both should stay in sync but there's nothing wiring them together. Worth exporting once from `lib/plantings.ts` (which already owns `computeDerivedCount` and other plop domain logic) and importing in both.
- **`web/src/editor/GardenCanvas.tsx:201`, `PlopLayer.tsx:68`, `PlopOverlay.tsx:130`** — the SVG local-frame transform string `` `translate(${x} ${y}) rotate(${deg})` `` is now written out five times total (two pre-existing in `ObjectShape.tsx:64`/`SelectionOverlay.tsx:165`, three new ones from this PR). `lib/geometry.ts` already centralizes the local-frame *math* (`localToWorld`/`worldToLocal`), but the SVG-transform string itself isn't factored out, so this PR tripled a copy-paste that a small `localFrameTransform(center, rotationDeg)` helper (or a `<LocalFrame>` wrapper component) would have avoided.
Nothing else stood out structurally — the new files (`PlopLayer`, `PlopMarker`, `PlopOverlay`, `PlopInspector`, `lib/plantings.ts`) are appropriately sized, named consistently with existing `ObjectShape`/`SelectionOverlay`/`Inspector` counterparts, and the `begin(onMove, fields)` abstraction in `PlopOverlay.tsx` is a reasonable way to share the move/resize gesture plumbing rather than duplicating it.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Both findings verified against the actual code. Final corrected review:
## VERDICT: Minor issues
**Findings (⚡ Performance lens)**
- **`web/src/editor/PlopLayer.tsx:48-53`** (byObject grouping) and **`:64`** (`dominantColor` call) — `PlopLayer` is wrapped in `memo()`, but its `objects` and `scale` props are not stable across common editor gestures, so the memo rarely pays off:
- **Any object drag/resize/rotate**: `SelectionOverlay.tsx:81` calls `setLiveObject(onMove(ev))` on every pointermove, and `GardenCanvas.tsx:95-98`'s `rendered` (passed to `PlopLayer` as `objects`, `GardenCanvas.tsx:184`) is a `useMemo` keyed on `liveObject`, so it produces a new array reference every pointermove even when only one unrelated object's geometry changed. Since `PlopLayer`'s `memo()` does a shallow prop comparison, the new array reference busts it, forcing the full `byObject` grouping (O(total active plops in the garden)) to redo every frame — even for objects with no plops at all.
- **Any zoom gesture (wheel/pinch)**: `viewport.scale` changes continuously during zoom (`useViewport.ts` `onWheel`/`onPinch`) and is passed straight through as `scale={viewport.scale}` (`GardenCanvas.tsx:187`), also busting the memo. (Plain panning only changes `tx`/`ty`, not `scale`, so panning alone does not trigger this.)
- The `byObject` grouping runs unconditionally before the `far` check (`GardenCanvas.tsx`... `PlopLayer.tsx:54`), so it's redone every frame even when the tint isn't rendered at near zoom.
- `PlopLayer` has no internal `useMemo`, confirmed by reading the full component body — the grouping and (when far) dominant-color scan are plain per-render work with no memoization independent of `objects`/`scale` identity.
- Impact scales with total plop count and fires at drag/zoom frequency, adding avoidable per-frame CPU to gestures that predate this PR. A `useMemo` keyed on `plantings`/`plantsById` (independent of `objects`/`scale` reference churn) would restore effective memoization.
- **`web/src/pages/GardenEditorPage.tsx:35`** — unlike `liveObject`, whose value is only read inside `GardenCanvas` (the page only imports the `setLiveObject` setter, confirmed at line 34), `livePlanting`'s value is subscribed at the page level to compute `selectedPlop` (lines 119-121). Since `GardenCanvas` is not wrapped in `memo()` (confirmed — `export function GardenCanvas` at `GardenCanvas.tsx:32` has no memo wrapper), every pointermove of a plop move/resize (`PlopOverlay.tsx:75` calls `setLivePlanting` on each move) re-renders the entire `GardenEditorPage` tree, including `GardenCanvas` and `PlopInspector`, rather than staying scoped to the canvas the way object drags do. This is bounded (a couple `O(n)` `.find()` scans plus JSX re-creation) but is an avoidable inconsistency with the established `liveObject` pattern in the same file, and compounds with the `PlopLayer` issue since it's another source of prop-identity churn during plop-drag gestures.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`web/src/editor/Inspector.tsx:227` + `web/src/pages/GardenEditorPage.tsx:78-99,118,140` + `web/src/editor/GardenCanvas.tsx:81-89,178`** — Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed: `ObjectShape.tsx` has no focus-mode gating on `onPointerDown` — the focused object itself renders at full opacity (`GardenCanvas.tsx:178`, `focusedObjectId !== o.id` dimming only applies to *other* objects) and is fully selectable via the plain `select` action (`store.ts:59`, no guard against `focusedObjectId`). Selecting it opens `Inspector`, whose Delete flow (`Inspector.tsx:225-227`) calls `select(null)` and `del.mutate(object.id)` but never touches `focusedObjectId`. After the optimistic delete (`objects.ts:175`), `objects` no longer contains that id, so: `GardenEditorPage.tsx:118`'s `focusedObject` becomes `null`, hiding the breadcrumb block (`GardenEditorPage.tsx:144`); `focusedObjectId == null && <Palette/>` (`GardenEditorPage.tsx:140`) stays false, hiding the palette; and `GardenCanvas.tsx:85-86` (`target` undefined, `focusedObjectId != null` still true) returns early every render without updating `fitKeyRef`, permanently short-circuiting the fit effect. Verified the Escape handler (`GardenEditorPage.tsx:86-98`) is the only way out — with `selectedId`/`selectedPlantingId` both null and `focusedObjectId` still set, it falls through to `exitFocus()`. Reachable via normal use (focus a plantable bed, select it while focused, delete it).
- **`web/src/editor/PlopInspector.tsx:65-73`** — Confirmed `commitCount` returns without patching or resetting local state on non-integer/zero/negative input, and — unlike sibling `commitRadius` (`PlopInspector.tsx:58-63`, which clamps via `Math.max(MIN_RADIUS_CM, ...)`) — never corrects the value. Since no patch fires, `plop.count` never changes, so the re-sync `useEffect` (keyed on `plop.count`, line 50) never re-triggers either — the invalid text sits in the field indefinitely with no feedback.
- **`web/src/lib/objects.ts:299-311`** — Confirmed `applyPlantingPatch` omits `removedAt` from its spread despite `PlantingPatch.removedAt` being declared (`objects.ts:223`). Grepped the repo: no call site currently invokes `useUpdatePlanting.mutate` with `removedAt` — the only setter of `removedAt` is `useRemovePlanting` (`objects.ts:268-286`), which bypasses `applyPlantingPatch` entirely via its own `onMutate`. So this is currently dead but is a real latent optimistic-cache gap for any future direct `removedAt` update through `useUpdatePlanting`.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
I traced the tainted-input paths through this diff from a security perspective and found nothing material:
- **`?focus` query param** — validated in `web/src/router.tsx:93-96` as a positive integer (`Number.isInteger(f) && f > 0`) before reaching the store/canvas. No injection or type-coercion risk. Verified by reading the route.
- **Path interpolation in API calls** — `/objects/${objectId}/plantings` (`objects.ts`) and `/plantings/${id}` (`objects.ts`) use `objectId`/`id` typed as `number`, sourced from zod-validated server rows (`serverPlantingSchema`/`serverObjectSchema`) and typed `EditorObject`/`EditorPlanting`. No string path injection possible. Verified via `objects.ts` and `plantings.ts`.
- **Untrusted rendering** — plant `color`/`icon`/`name` are `z.string()` from the API (`plants.ts:31-32`) and rendered as SVG attributes (`fill={color}`) / React text children in `PlopMarker.tsx`/`PlopLayer.tsx`, and as a CSS `backgroundColor` via `color-mix()` in `PlantIcon.tsx`. React sets these through `setAttribute`/textContent (escaped) and the CSSOM `style.backgroundColor` setter (invalid values dropped per-property), so there's no HTML/CSS injection vector. No `dangerouslySetInnerHTML`/`innerHTML` anywhere (grep confirmed).
- **Client input validation** — `commitCount` rejects non-integers `< 1` (`PlopInspector.tsx`); `commitRadius` clamps to `MIN_RADIUS_CM`; `computeDerivedCount` guards `<= 0`. These bound what's POSTed/PATCHed. Server-side authz/length limits for `label` etc. are backend concerns outside this (frontend) diff.
- **Credentials** — mutations go through `api` with `credentials: 'same-origin'` (`api.ts:97`), same as existing object mutations; no new secret handling or leakage.
The data is same-origin, authenticated, zod-parsed, and all identifiers are numeric. Nothing in the security lane is wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings confirmed against the actual source.
**Finding 1** — `PlopInspector.tsx:142` (the draft cited 148; actual line is 142). The blur handler is exactly:
```ts
onBlur={() => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })}
```
Clearing a set date (`planted=""`, `plop.plantedAt="2024-01-01"`): first clause `"" !== "2024-01-01"` is true, second `planted !== ''` is false → no PATCH. Confirmed. The label field at line 133 correctly sends `null` on clear (`label.trim() || null`). Asymmetry is real.
**Finding 2** — `objects.ts:223` declares `removedAt?: string | null` on `PlantingPatch`; `applyPlantingPatch` (lines 299–311) has no `removedAt` branch. Confirmed. Soft-removal is handled separately by `useRemovePlanting` (its own `onMutate` filters the plop, lines 275–279), and `useUpdatePlanting`'s `onSuccess` does drop the plop when `updated.removedAt` is set (lines 245–246) — but the optimistic `onMutate` pass via `applyPlantingPatch` would not drop it, only bump the version. So it's a latent inconsistency, not an active bug. Confirmed as described.
**Verdict: Minor issues**
- **`web/src/editor/PlopInspector.tsx:142` — planted date cannot be cleared.** The blur guard is `planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })`. If a plop has `plantedAt = "2024-01-01"` and the user empties the field (`planted = ""`), the first clause is true but `planted !== ''` is false, so no PATCH is sent and the stale date persists. The label field (line 133) correctly sends `null` when cleared (`patch({ label: label.trim() || null })`); the planted field should do the same (`plantedAt: null` when emptied).
- **`web/src/lib/objects.ts:299` — `applyPlantingPatch` ignores `removedAt` though `PlantingPatch` declares it.** `PlantingPatch` includes `removedAt?: string | null` (line 223), but `applyPlantingPatch` (lines 299–311) has no `removedAt` branch. Soft-removal normally goes through `useRemovePlanting` (which filters the plop in its own `onMutate`), and `useUpdatePlanting.onSuccess` does drop the plop when `updated.removedAt` is set (lines 245–246) — so this is a latent inconsistency, not an active bug. But if any caller uses `useUpdatePlanting({ removedAt })`, the optimistic pass would leave the plop visible (only the version bump applies) until the round-trip completes.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All findings verified. All six findings confirmed against actual code. Producing the corrected review.
## Corrected review
Verdict: Minor issues
Through the cleanliness/maintainability lens, the new plop code repeats a few patterns the surrounding files already pay for, and leaves a vestigial prop. Nothing blocking, but a small shared-helper pass would cut real churn.
- **`web/src/editor/PlopMarker.tsx:33` — vestigial `dimmed` prop.** `PlopLayer` always renders `<PlopMarker ... dimmed={false} />` (`PlopLayer.tsx:84`), and the actual focus-dimming is applied one level up on the object `<g opacity={dimmed ? 0.4 : 1}>` (`PlopLayer.tsx:69`). So `PlopMarker`'s `dimmed`/`opacity={dimmed ? 0.3 : 1}` branch (`PlopMarker.tsx:49`) is dead — a prop that can never be true. Either drive the marker's own opacity from it (and drop the group opacity) or remove the prop to avoid misleading future readers.
- **Object-local transform string duplicated across 5 sites.** `translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg})` is hand-built in `GardenCanvas.tsx:201`, `PlopLayer.tsx:68`, `PlopOverlay.tsx:130`, plus pre-existing `SelectionOverlay.tsx:165` and `ObjectShape.tsx:64`. This PR adds two new copies of the core "plops live in the object's local frame" transform. A single `objectGroupTransform(o: EditorObject): string` (or a `<ObjectGroup object={o}>` wrapper) would centralize it — right now a rotation/transform convention change means editing five places.
- **`SELECT_COLOR = '#2f7a3e'` redefined in 4 files.** `SelectionOverlay.tsx:11`, `ObjectShape.tsx:59` (inline), and the new `PlopMarker.tsx:12` / `PlopOverlay.tsx:11` each redeclare the same constant (and `#00000033` for the unselected stroke). The PR extends an existing pattern, but this is the moment to hoist one shared `editor/colors` constant before it spreads further.
- **Three different "dimmed by focus" opacities.** `GardenCanvas.tsx:18` `DIMMED_OPACITY = 0.35` (for non-focused objects), `PlopLayer.tsx:69` `0.4` (for non-focused plop groups), `PlopMarker.tsx:49` `0.3` (the dead `dimmed` branch). Same visual concept, three magic numbers — pick one and share it.
- **Duplicated clamp-to-object-bounds.** `GardenCanvas.tsx:144-145` inlines the `±widthCm/2`/`±heightCm/2` clamp for placement, and `PlopOverlay.tsx:62-65` has `clampLocal` doing exactly the same math. A shared `clampToLocalFrame(p, object)` in `geometry.ts` (next to `worldToLocal`) would remove the copy and keep the bounds invariant in one place.
- **`plantsById` rebuilt in two components.** `GardenCanvas.tsx:68` and `GardenEditorPage.tsx:49` both do `new Map(plants.map((p) => [p.id, p]))`. Minor, but since the page already owns `plants`, it could pass the map down (or a memoized selector) instead of reconstructing it. Low priority.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found
Through the performance lens, the change is sound. I verified the hot paths:
- **Plop drag (`PlopOverlay`)** — updates `livePlanting` per `pointermove`; `renderedPlops` is rebuilt via `useMemo` on `[plantings, livePlanting]` (GardenCanvas.tsx:100-103), but non-dragged plops keep object identity, and `PlopMarker` is `memo`'d with stable `plantsById`/`onSelect`/`scale` props, so only the dragged marker actually re-renders. One PATCH per gesture on release. Good.
- **Bed drag** — `liveObject` changes per move; `rendered` rebuilds (O(n) map) and `PlopLayer` re-renders because its `objects` prop changes identity, rebuilding the `byObject` map (O(plantings)) and iterating objects. This is O(objects+plantings) of reconciliation work per pointermove, but `PlopMarker` memo prevents DOM work on unchanged plops. Acceptable for realistic garden sizes; not a regression in kind (the object list already re-mapped per move before this PR).
- **No N+1 / refetch** — `useCreatePlanting`/`useUpdatePlanting`/`useRemovePlanting` patch the existing `FullGarden` cache on success (`objects.ts`) with no `invalidateQueries`/refetch. Confirmed by reading the mutation bodies.
- **Fit effect** — guarded by `fitKeyRef` so unrelated re-renders (plop add, bed drag, cache updates that produce a new `objects` array) hit the early return and don't refit. Verified at GardenCanvas.tsx:78-87.
- **`dominantColor`** (PlopLayer.tsx:8-23) allocates a `Map` per planted object per render in far-zoom mode; during a pan/zoom this is O(total plops) of map ops per frame. Only material for very large gardens with many plops at far zoom — a `useMemo`/hoisted cache could avoid it, but I would not call it a blocking regression.
One trivial note (not worth a finding on its own): `plantsById` is built in both `GardenEditorPage.tsx:49` and `GardenCanvas.tsx` via its own `useMemo` — duplicate but O(plants) and memoized, negligible.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`PlopInspector` — planted date can never be cleared.** `web/src/editor/PlopInspector.tsx:142`: the `onBlur` is `() => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })`. The `planted !== ''` guard short-circuits when the user empties the date field, so no PATCH is sent and `plantedAt` cannot be reset to `null`. This is an asymmetry with the sibling fields: `commitCount` (line 66-68) sends `patch({ count: null })` when the count is cleared, and the label `onBlur` (line 133) sends `patch({ label: label.trim() || null })`, allowing a reset. Once a plop has a planted date, the inspector offers no way to remove it. Fix: allow the empty case to PATCH `plantedAt: null`, e.g. handle `planted === ''` separately (PATCH `null` when `plop.plantedAt != null`) before the non-empty comparison.
- **`useRemovePlanting` doesn't reconcile a 409 conflict, leaving the user stuck.** `web/src/lib/objects.ts:281-284`: the `onError` only restores `ctx.prev` and shows a generic toast. `useUpdatePlanting` (lines 250-257) instead calls `conflictPlanting(err)` and, on a conflict, writes the server's `current` row into the cache so the next edit carries a fresh `version`. `useRemovePlanting` lacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale) `version` and no refetch is triggered (no `onSettled`/`invalidate`). The user clicks "Remove" again, the same stale `version` is sent, the server 409s again — the plop can't be removed until an unrelated refetch refreshes the version. Fix: mirror the update hook — on a `conflictPlanting(err)` result, write `current` into the cache instead of only restoring `prev` (and/or re-arm the remove with the new version).
</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**
After tracing data flow, input paths, auth boundaries, and rendering paths through the security lens, I found no authn/authz gaps, injection vectors, unsafe deserialization, secret leakage, or missing validation that would constitute a material security issue in this PR.
- The `?focus` URL parameter is validated as a positive integer in `router.tsx:93-95` before reaching state logic.
- All text values rendered to the DOM (plant names, labels, icons) pass through React’s JSX escaping or SVG text nodes — no raw HTML insertion points.
- Event handlers on overlays use `stopPropagation()`, preventing gesture confusion or click-through to unintended actions.
- The soft-remove / optimistic-update rollback paths in `objects.ts` handle 409 conflicts correctly.
- No credentials or tokens are mishandled; the API layer continues to rely on `same-origin` HttpOnly session cookies.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**Blocking issues found**
* Two verified correctness bugs: the near-zoom plop marker fails to recompute its displayed count during live resize (contradicting an acceptance criterion), and the plop inspector prevents clearing an already-set planted date.
---
### Findings
- **`web/src/editor/PlopOverlay.tsx:121`** — `startResize` sets the optimistic `livePlanting` to `{ ...base, radiusCm: r }` but leaves `derivedCount` unchanged. Because `PlopMarker` (near-zoom text) reads `effectiveCount(plop)` which uses the stale `derivedCount`, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says *“Resizing a plop updates its displayed derived count live”*; the inspector panel does recompute it via `computeDerivedCount`, but the canvas marker does not.
**Fix:** pass the plant (or its `spacingCm`) into `PlopOverlay` and include a live `derivedCount` in the optimistic `livePlanting` payload, or have `PlopMarker` recompute `derivedCount` from `plop.radiusCm` and `plant.spacingCm` the same way `PlopInspector` does.
- **`web/src/editor/PlopInspector.tsx:142`** — The `planted` date blur guard is `planted !== '' && patch({ plantedAt: planted })`. If a user clears an existing date, `planted` becomes `''`, the left-hand condition is false, and **no PATCH is ever sent**, so the date cannot be removed through the UI. The server schema accepts `plantedAt: null` for clearing.
**Fix:** change the handler to send `plantedAt: planted || null` whenever the normalized value differs from the current server value, e.g.
`onBlur={() => { const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next }) }}`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`PlopMarker` `dimmed` prop is dead code** — `PlopLayer.tsx:84` hardcodes `dimmed={false}` while the parent `<g>` already applies opacity dimming at line 69. The `dimmed` parameter and the conditional `opacity={dimmed ? 0.3 : 1}` in `PlopMarker.tsx:26-27,49` are never triggered and create confusion about where dimming lives. Remove the prop and inline opacity from `PlopMarker`.
- **Pointer-gesture scaffolding is copy-pasted between overlays** — `PlopOverlay.tsx:36-97` duplicates the `cleanupRef`, `useEffect` teardown, `begin`, `move`/`detach`/`finish` pattern from `SelectionOverlay.tsx:44-104` almost line-for-line. The only meaningful differences are the store setter names (`setLivePlanting` vs `setLiveObject`) and the mutator type. Extract a shared `useDragGesture` hook so gesture fixes (e.g., adding `lostpointercapture` handling) don't have to be applied in two places.
- **`PlantingPatch` interface advertises `removedAt` but `applyPlantingPatch` ignores it** — `objects.ts:223` declares `removedAt?: string | null` in the patch type, yet the optimistic-update helper (`objects.ts:299-311`) never spreads it. If a caller ever passes `removedAt` to `useUpdatePlanting`, the cache will be optimistically inconsistent until the server response arrives. Either add `removedAt` to `applyPlantingPatch` or remove it from `PlantingPatch` (it's already handled by the dedicated `useRemovePlanting` hook).
- **Focus-mode dimming uses inconsistent opacity constants** — Objects dim to `0.35` (`GardenCanvas.tsx:18`), plop groups dim to `0.4` (`PlopLayer.tsx:69`, magic number), and the dead `PlopMarker` path uses `0.3` (`PlopMarker.tsx:49`). These three values represent the same visual concept; unify them under one exported constant.
- **`SELECT_COLOR` is defined identically in three files** — `#2f7a3e` appears at `PlopOverlay.tsx:11`, `PlopMarker.tsx:12`, and `SelectionOverlay.tsx:11`. Extract it to a shared constant so the selection accent color can be changed in one place.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
* **`web/src/editor/PlopLayer.tsx:48-64` — `byObject` grouping and `dominantColor` recomputed on every zoom/pan frame**
`PlopLayer` rebuilds the `byObject` Map from scratch on every render, and calls `dominantColor` (which iterates an object's plops and builds a frequency Map) for every planted object whenever `scale < SEMANTIC_FAR`. Because `scale` changes on every frame during a zoom or pan gesture, this work repeats even though neither `plantings` nor `plantsById` have changed. For a garden with many plantings this creates unnecessary CPU load on the render thread.
**Fix:** Wrap `byObject` in `useMemo` keyed on `plantings` only. Precompute dominant-color tints in the same memo (or a second one keyed on `plantings` and `plantsById`) so that pure zoom/pan frames skip the recomputation entirely.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- `PlopInspector.tsx:59` — `commitRadius` uses `parseFloat(radius)`. `parseFloat('1.2.3')` returns `1.2`, which is accepted as valid even though the user typed garbage. The field then silently commits a value the user did not intend, with no error feedback. **Fix:** validate with `Number(radius)` and a regex or `isNaN` check, and surface a field-level error rather than silently swallowing.
- `PlopInspector.tsx:70` — `commitCount` uses `Number(count)`. While `Number.isInteger` rejects `NaN`, inputs like `'3.5'` silently do nothing with no feedback. An invalid typed value just evaporates on blur. **Fix:** provide explicit validation feedback in the UI.
- `PlopOverlay.tsx:121` — `startResize` computes radius via `Math.hypot(p.x - base.xCm, p.y - base.yCm)` clamped only by `MAX_RADIUS_CM`. Unlike the move gesture, resize is **not** clamped to the parent object's bounds, so a plop can be resized to spill outside its bed. The overlay comment says "clamped to bounds" for move but omits resize. **Fix:** clamp radius so the plop circle stays inside the object's local rectangle, or document that spillover is intentional.
- `lib/objects.ts:200` — `useCreatePlanting` has no optimistic `onMutate`. On slow networks a user may tap the placement sheet repeatedly, believing nothing happened, and create duplicate plops. The update mutation has optimistic patching; create should mirror it for repeat-placement UX. **Fix:** optimistically append the new plop to the cache with a temporary ID.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I'll investigate this PR through the security lens. Let me verify the key files.
Now let me check how `label` is rendered and where planting bodies are constructed — the user-provided label goes to the server, but let me verify how it could be displayed back. Also check `Inspector.tsx` to confirm the `onFocus` authz angle (no authz on the client).
Now I have the picture. Let me check `serverPlantingSchema` — `label` is `z.string()` with no max length, and `color`/`icon` from `plantSchema` are unbounded strings that get interpolated into SVG and CSS. Let me verify the PlantIcon's `color` usage path and whether the `color`/`icon`/`name`/`label` are sanitized anywhere.
Let me verify the `icon` and `color` rendering at the SVG sink points and confirm there's no React auto-escaping concern (text content in SVG is auto-escaped, so XSS via `<text>{plant.icon}</text>` is fine). The `PlantIcon` uses `style={{ backgroundColor: color-mix(...) }}` with template interpolation.
I have enough to conclude. Let me consolidate the security review.
Key findings:
1. **`PlantIcon` — potential CSS injection via `color`**: `PlantIcon.tsx:12` interpolates `plant.color` (a `z.string()` with no format constraint) into a `style={{ backgroundColor: \`color-mix(in srgb, ${color} 18%, transparent)\` }}`. A malicious `color` string containing `;` or `)` or arbitrary CSS could escape the `color-mix(...)` call and inject style properties. But more importantly, React's `style` prop value as a string does NOT inject `;` separated properties the way an attribute string would — the whole string is set as the `background-color` value, which the browser will reject as invalid if malformed. So actual CSS injection into other properties is prevented by React's style-assignment semantics. Lower risk, but the unvalidated `color`/`icon`/`name` are pre-existing concerns, not introduced by this PR. They predate this PR. So I should only flag what's *new*.
2. **`PlopMarker.tsx:72,88`** interpolates `plant.icon` and `plant.name` into SVG `<text>` content. React auto-escapes JSX text content, so no XSS — these are safe.
3. **`label`** (`serverPlantingSchema.label = z.string()`) — user-provided, unbounded, but rendered only as a TextField value (auto-escaped). No XSS sink. No new sink introduced by this PR.
4. **Numeric inputs to API**: `xCm`, `yCm`, `radiusCm`, `count` — clamped client-side (`Math.max/Math.min`), but the server is responsible for re-validating. `plantId` and `objectId` come from server-loaded lists; the `?focus=<id>` URL param is validated in `router.tsx:93-96` as a positive integer. No injection vector — they're interpolated into URL paths as numbers.
5. **`?focus=<id>`** is just a viewport framing hint; no authz decision is made on it client-side (the objects are already filtered by garden on the server `/full` endpoint). Setting `?focus=999` for an object the user can't see just results in no framing. No IDOR — the server `/full` endpoint is the trust boundary and is garden-scoped.
6. **Optimistic cache patching** is purely client-side UI state; no security implication.
Within my lens (security), nothing material is introduced by this PR. The unbounded string fields (`color`/`icon`/`name`/`label`) are pre-existing and rendered safely through React's auto-escaping; the `PlantIcon` color interpolation is a pre-existing pattern not added by this PR. The new code adds no new sinks, no new untrusted-input surfaces beyond what's already validated (zod parse on all server responses), and no authz decisions.
Verdict: No material issues found.
No material issues found
- Verified all new untrusted-input sinks introduced by this PR:
- `PlopMarker.tsx:72,88` interpolates `plant.icon` and `plant.name` into SVG `<text>` content — React auto-escapes JSX text, so no XSS vector. `PlopInspector.tsx:91,95` and the `PlantIcon` use the same auto-escaped text/safe `style` path. Confirmed by reading `PlopMarker.tsx`, `PlopInspector.tsx`, and `PlantIcon.tsx`.
- `plop.label` (`serverPlantingSchema.label: z.string()` in `plantings.ts:16`) is user-authored and unbounded, but is only rendered as a controlled `<TextField>` value (`PlopInspector.tsx:131-133`) — auto-escaped, no HTML sink. The lack of a max-length is a pre-existing pattern shared with `plantSchema.name`/`notes`; not introduced here and not a security bug under React's text escaping.
- All server responses (`/full`, planting create/update/remove) are parsed through zod (`serverPlantingSchema`, `plantSchema`) before touching cache/UI (`objects.ts:204,230,273`), so no unsafe deserialization of raw server payloads.
- `?focus=<id>` URL search param is validated to a positive integer in `router.tsx:93-96` and used only to frame the viewport / pick from already-loaded objects; no client authz decision rests on it (the `/full` endpoint is the trust boundary and is garden-scoped server-side). No IDOR.
- Path/URL interpolations (`/objects/${objectId}/plantings`, `/plantings/${id}`) use numeric ids derived from server rows; no injection.
- The one `PlantIcon` `color-mix(in srgb, ${color} …)` template interpolation is a **pre-existing** pattern (also used in `PlantCard.tsx:51`, `PlantPicker.tsx:90`) not introduced by this PR, so I'm not flagging it here.
Within the security lens, this change introduces no material issues.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings reference line 84 for PlopMarker, but actual line is 88. Let me verify the other claims.
Both findings confirmed. Let me verify the objects.ts optimistic patch and GardenCanvas livePlanting flow.
Both findings confirmed. The line for PlopMarker is 88, not 84 (the diff truncated; the draft said 84 but the actual `{plant!.name} · {effectiveCount(plop)}` is on line 88). I'll correct to line 88. The PlopInspector line 142 is correct.
VERDICT: Minor issues
- **`web/src/editor/PlopMarker.tsx:88`** — At near-zoom the plop count renders `{plant!.name} · {effectiveCount(plop)}`, and `effectiveCount = p.count ?? p.derivedCount` (plantings.ts:59-61). During a resize gesture, `PlopOverlay.startResize` (PlopOverlay.tsx:115-126) sets `livePlanting = {...base, radiusCm: r}` — the new radius but the *old* `derivedCount` — and the optimistic patch in `applyPlantingPatch` (objects.ts:299-311) deliberately leaves `derivedCount` untouched. `GardenCanvas` substitutes `livePlanting` into `renderedPlops` (GardenCanvas.tsx), so the marker reads the stale `derivedCount` until the PATCH roundtrips via `onSuccess` (objects.ts:240-248). The count number therefore does not update live with the radius, contradicting the acceptance criterion "Resizing a plop updates its displayed derived count live". The `computeDerivedCount` mirror (plantings.ts:66-69) is wired only into the inspector placeholder (PlopInspector.tsx:56), not the marker. Fix: render `plop.count ?? computeDerivedCount(plop.radiusCm, plant?.spacingCm ?? 0)` in the marker (import `computeDerivedCount`).
- **`web/src/editor/PlopInspector.tsx:142`** — The planted-date `onBlur` guard `planted !== ''` makes it impossible to clear a planted date: the PATCH only fires when the value changed *and* is non-empty. `CreatePlanting` defaults `plantedAt` to today server-side (plantings.go:97-100), so every plop has a date, and the PATCH API supports `plantedAt: null` via `SetPlantedAt`/`parseNullableString` semantics (plantings.go:211-213). The user can change the date but never remove it; meanwhile the local input shows empty while the server retains the old value until the next external resync (the resync effect at PlopInspector.tsx:44-50 would restore it once the field loses focus). Fix: drop the `planted !== ''` guard and `patch({ plantedAt: planted || null })` on blur when the value changed.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All findings verified. Let me confirm the few specifics:
1. **PlopLayer.tsx:84 dimmed dead prop** — Confirmed. `PlopLayer.tsx:61` computes `dimmed` at the group level (`opacity={dimmed ? 0.4 : 1}` at line 69), and `PlopMarker` always receives `dimmed={false}` at line 84. Grep confirms no caller ever passes `true`. The `dimmed` prop on `PlopMarker` (line 33) and its `opacity={dimmed ? 0.3 : 1}` (line 49) is unreachable logic.
2. **Three dim-opacity constants** — Confirmed. `DIMMED_OPACITY = 0.35` (GardenCanvas.tsx:18), `0.4` (PlopLayer.tsx:69), `0.3` (PlopMarker.tsx:49, dead). Three divergent constants.
3. **PlopOverlay gesture scaffolding duplication** — Confirmed. PlopOverlay.tsx:36-97 (`cleanupRef` + `useEffect` cleanup + `begin()` + `makePointerLocal` + `detach`/`finish`) closely mirrors SelectionOverlay.tsx:44-104. ~60 lines of near-identical scaffolding.
4. **plantsById duplicate construction** — Confirmed. GardenEditorPage.tsx:49 and GardenCanvas.tsx:68 both build `new Map(plants.map((p) => [p.id, p]))` from the same `plants` array passed as prop.
5. **applyPlantingPatch omits removedAt** — Confirmed. `PlantingPatch` declares `removedAt?: string | null` (objects.ts:223), `useRemovePlanting` sends it (line 273), but `applyPlantingPatch` (lines 299-311) only copies plantId/xCm/yCm/radiusCm/count/label/plantedAt — no `removedAt` line. `useRemovePlanting.onMutate` filters the plop out, so remove path is fine; a direct `removedAt` patch via `useUpdatePlanting` wouldn't apply optimistically. Latent inconsistency.
6. **PlopInspector duplicated initializers** — Confirmed. PlopInspector.tsx:37-40 (`radius`/`count`/`label`/`planted` initializers) duplicated verbatim at 46-49 in the resync effect.
7. **halfFW/halfFH redundant guards** — Confirmed. GardenCanvas.tsx:152-153 guards `focusedObject ?`, only read at lines 203-206 inside `{focusedObject && armedPlant && (...)}` (line 200) where `focusedObject` is guaranteed non-null.
8. **fitKeyRef key redundant garden.id** — Confirmed but confidence medium. The key is `${garden.id}:${focusedObjectId}` (GardenCanvas.tsx:83). GardenEditorPage resets editor state on `gid` change (lines 54-70), and `GardenCanvas` is rendered without an explicit `key` prop (line 163), so React doesn't remount it on garden change — the page's effect resets state but `fitKeyRef` ref persists. So `garden.id` in the key is actually NOT redundant (it guards against a stale ref pointing at a previous garden's fit key when garden.id changes without unmounting). This finding is questionable.
Let me re-examine finding #8 more carefully. The page does NOT pass a `key={garden.id}` to GardenCanvas, so the component instance persists across garden changes. `fitKeyRef` is a `useRef`, which persists. On garden change, `fitKeyRef.current` would still hold the old key from the previous garden. Without `garden.id` in the key, the new garden would never refit (because the key matches the stale ref). So `garden.id` is actually NECESSARY. The finding is wrong — drop it.
## VERDICT: Minor issues
Findings through the 🧹 cleanliness/maintainability lens:
- **`web/src/editor/PlopLayer.tsx:84`** — `dimmed={false}` is hardcoded for every `PlopMarker`, but `PlopMarker` declares `dimmed: boolean` as a required prop and branches on it (`PlopMarker.tsx:49`). Dead parameter: the layer already applies dim opacity at the group level (`PlopLayer.tsx:69`, `opacity={dimmed ? 0.4 : 1}`), so the marker-level `dimmed` can never be `true`. Grep confirms no caller ever passes `true`. Either drop the prop from `PlopMarker` or pass the real per-plop dim state.
- **`web/src/editor/PlopLayer.tsx:69`, `web/src/editor/PlopMarker.tsx:49`, `web/src/editor/GardenCanvas.tsx:18`** — Three separate dim-opacity constants (`0.4`, `0.3`, `0.35`) for the same visual concept. The PR introduced `DIMMED_OPACITY` in GardenCanvas but PlopLayer hardcoded `0.4` instead of reusing it (and PlopMarker's `0.3` is dead anyway). Consolidate to one exported constant.
- **`web/src/editor/PlopOverlay.tsx:36-97`** vs **`web/src/editor/SelectionOverlay.tsx:44-104`** — ~60 lines of near-identical gesture scaffolding (`cleanupRef` + `useEffect` cleanup + `begin()` + `makePointerLocal`/`makePointerWorld` + `detach`/`finish`). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (`useDragGesture`). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.
- **`web/src/pages/GardenEditorPage.tsx:49`** + **`web/src/editor/GardenCanvas.tsx:68`** — `plantsById` is built independently in both files (`new Map(plants.map((p) => [p.id, p]))`) from the same `plants` array that the page already passes to the canvas. Either pass the pre-built `Map` down as a prop or lift it; the duplicate `useMemo` is wasteful and easy to drift.
- **`web/src/lib/objects.ts:299-310` (`applyPlantingPatch`)** — `PlantingPatch` declares `removedAt?: string | null` (line 223) and `useRemovePlanting` sends it (line 273), but `applyPlantingPatch` never copies `removedAt` onto the optimistic row. `useRemovePlanting` has its own `onMutate` that filters the plop out, so the remove path is fine, but a direct `removedAt` patch via `useUpdatePlanting` wouldn't apply optimistically — a latent inconsistency. Add the `removedAt` line for symmetry with the other fields (and with `applyServerPatch`).
- **`web/src/editor/PlopInspector.tsx:37-40`** + **`46-49`** — The four `useState` initializers are duplicated verbatim in the resync `useEffect`. Extract a `syncFromPlop(plop, unit)` helper (or a small `useSyncedFields` hook) to keep the two sites from drifting.
- **`web/src/editor/GardenCanvas.tsx:152-153`** — `halfFW`/`halfFH` guard `focusedObject ?` but are only read inside `{focusedObject && armedPlant && (...)}` (line 200), where `focusedObject` is guaranteed non-null. The ternaries are dead defensive code; simplify to `const halfFW = focusedObject.widthCm / 2` inside the block, or just inline.
Verified by reading `PlopLayer.tsx`, `PlopMarker.tsx`, `PlopOverlay.tsx`, `SelectionOverlay.tsx`, `GardenCanvas.tsx`, `GardenEditorPage.tsx`, `objects.ts`, `PlopInspector.tsx`, and grepping for `dimmed`/`plantsById` usages. (Dropped the `fitKeyRef` key-redundancy finding: `GardenCanvas` is not keyed by `garden.id` in its parent, so the ref persists across garden changes and `garden.id` in the key is actually necessary to trigger a refit on garden switch.)
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the diff through the performance lens, verifying against the checked-out code.
- **Memoization is sound.** `PlopLayer` and `PlopMarker` are both `memo`'d. On pan (only `tx`/`ty` change), `PlopLayer`'s props don't change (`scale` unchanged, `objects`/`plantings`/`plantsById` stable via `useMemo`), so it skips re-render — and even if it didn't, each `PlopMarker`'s props are stable (the parent `<g>` transform carries position), so they skip too. On zoom, re-renders cascade through `PlopLayer`/`PlopMarker` only because `scale` legitimately changes for semantic-zoom bands — necessary work, not waste.
- **Selection changes don't repaint every plop.** When `selectedPlantingId` changes, `PlopLayer` re-renders but only the two markers whose `selected` prop actually flips pass the `memo` check; the rest skip (verified at `PlopMarker.tsx:21`, `PlopLayer.tsx:83`).
- **No quadratic behavior.** `PlopLayer` builds a `byObject` map (O(plops)) once per render and does O(1) lookups (`PlopLayer.tsx:48-53`); `dominantColor` is O(plops-per-object) and only in far mode. `GardenCanvas` does three `.find()` calls per render (`selectedObject`/`selectedPlopObject`/`focusedObject`, `GardenCanvas.tsx:104-107`) — O(n) each, consistent with the pre-existing `selected` lookup pattern, not a new hotspot.
- **Optimistic updates are O(n) per mutation, not N+1.** `useUpdatePlanting`/`useRemovePlanting` do a single `full.plantings.map`/`filter` on the cached `FullGarden` (`objects.ts`), one PATCH per gesture — same contract as the object mutations.
- **Live-drag allocations are bounded.** `renderedPlops` rebuilds only when `livePlanting` changes (during a plop drag), and is a single O(n) map; `liveObject`'s merge is likewise O(n). No per-move re-sort.
- **Fit effect is guarded.** `fitKeyRef` prevents a refit on unrelated re-renders (plop add, bed drag) — the comment's claim holds (`GardenCanvas.tsx:83-84`).
- **Duplicate `plantsById` Map** is built in both `GardenEditorPage` and `GardenCanvas` — trivial duplicate allocation, not material.
The new code follows the existing memo discipline carefully; nothing in my lane regresses efficiency for realistic garden/plop sizes.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/editor/PlopInspector.tsx:142`** — Clearing the "Planted" date is silently dropped. The blur handler is `planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })`. The `planted !== ''` guard means an empty (cleared) value never produces a PATCH, so `plantedAt: null` is never sent. Worse, because no patch fires, the local `planted` state stays `''` while `plop.plantedAt` is still the old date — the field shows empty but the server keeps the date, and the re-sync `useEffect` (deps include `plop.plantedAt`, line 50) doesn't fire because `plop.plantedAt` never changed. Contrast with `commitCount` (line 65), which correctly handles the empty case by patching `{ count: null }`. Fix: mirror `commitCount` — drop the `planted !== ''` clause and patch `{ plantedAt: planted.trim() ? planted : null }` when the value differs from `plop.plantedAt ?? ''`. Verified by reading the file.
- **`web/src/lib/objects.ts:268-286`** (`useRemovePlanting`) — A 409 version-conflict on soft-remove leaves the user stuck. `onError` (line 281) only restores `ctx.prev` (the pre-removal snapshot, which carries the *stale* version) and toasts a generic "Could not remove that plant." It neither adopts the 409's `current` row (the way `useUpdatePlanting` does at line 250-257 via `conflictPlanting`) nor refetches (the way `useDeleteObject` does at line 182 via `onSettled`). So after a failed remove, the plop reappears in the cache with its old version; retrying sends that same stale version and 409s again — the user can't recover without reloading. Fix: either adopt `conflictPlanting(err)` in `onError` (write `current` into the cache so the next attempt uses the fresh version), or add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` like `useDeleteObject`. Verified by reading the file and comparing with the sibling object mutations in the same file.
</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>
**Verdict:** No material issues found
After tracing tainted input through the new planting (plop) editor and focus-mode deep-linking, I found no concrete security issues in this PR.
- **URL search params (`?focus`)** are validated to positive integers in the route definition (`router.tsx:93-96`) before reaching state. No injection or open-redirect vector.
- **All user-editable fields** (`label`, `radius`, `count`, `plantedAt`) are rendered through React’s normal JSX escaping (no `dangerouslySetInnerHTML`, no raw SVG text injection). The `plant.icon` value rendered inside an SVG `<text>` element is escaped by React and cannot execute scripts.
- **API paths** are constructed from trusted server-side IDs (`objectId`, `planting id`) already present in cached garden data, not from user input. No SSRF or path-traversal surface.
- **Optimistic cache patches** use object spreads on locally cached arrays; no prototype-pollution vectors via user-controlled keys.
- **No secrets or credentials** appear in the diff.
The `applyPlantingPatch` helper omits `removedAt` from its optimistic merge, but that is a transient cache-correctness issue (covered by another lens), not an authz bypass or injection vulnerability.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/PlopInspector.tsx:142`** — The `plantedAt` blur handler can never clear a date. The condition `planted !== ''` blocks the PATCH when the user empties the field, so a previously-set date persists on the server even though the UI shows an empty input.
**Fix:** change the blur handler to treat an empty string as `null`, e.g. `const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next })`.
- **`web/src/editor/PlopOverlay.tsx:121`** — During a resize drag, `livePlanting` is updated with the new `radiusCm` but the cached `derivedCount` is left unchanged. Because `PlopMarker` renders `effectiveCount(plop)` (which falls back to `plop.derivedCount`), the canvas text shows the stale derived count until the PATCH resolves. The inspector placeholder updates live via `computeDerivedCount`, but the canvas marker does not.
**Fix:** recompute `derivedCount` inside the drag move callback (the overlay has access to the plant via the parent if passed down, or the marker itself can derive it from `plant`).
- **`web/src/editor/GardenCanvas.tsx:139`** — `onPlace` guards with `if (!focusedObject || !armedPlant) return` but never checks `focusedObject.plantable`. A deep-link (`?focus=<id>`) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt.
**Fix:** add `|| !focusedObject.plantable` to the guard.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- **`PlopMarker` carries a dead `dimmed` prop** (`web/src/editor/PlopMarker.tsx:27`). The only caller (`PlopLayer`) always passes `dimmed={false}` and handles object-level dimming via the parent `<g>` opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.
- **Selection constants are triplicated** (`web/src/editor/SelectionOverlay.tsx:7` and `11`, `PlopOverlay.tsx:9` and `11`, `PlopMarker.tsx:12`). `SELECT_COLOR` and `HANDLE_PX` are defined identically in three and two files respectively. Centralizing them in a single module prevents visual drift and makes future tweaks one-touch.
- **Gesture scaffolding is copy-pasted** (`web/src/editor/SelectionOverlay.tsx:73` and `PlopOverlay.tsx:67`). The window-listener setup, cleanup ref pattern, and mutation-on-release logic are ~30 lines of nearly identical boilerplate with only `liveObject` vs `livePlanting` differing. Extracting a shared generic hook (e.g. `useDragGesture`) would eliminate the duplication and ensure interaction fixes apply once.
- **`dominantColor` is recomputed inline on every scale change** (`web/src/editor/PlopLayer.tsx:64`). Because `scale` changes during zoom and `PlopLayer` re-renders with it, `dominantColor` iterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.
- **Radius bounds are scattered and partially duplicated** (`web/src/editor/PlopInspector.tsx:10` and `PlopOverlay.tsx:9`-`10`). `MIN_RADIUS_CM` is defined in two files; `MAX_RADIUS_CM` only in one. Co-locating both in a shared constants module (e.g. alongside the planting domain) makes the model easier to keep consistent.
- **`GardenCanvas` rebuilds `plantsById` that its parent already computed** (`web/src/editor/GardenCanvas.tsx:68`). `GardenEditorPage` constructs the same `Map` and passes the raw `plants` array down only so the canvas can rebuild it. Passing `plantsById` directly removes duplicate code and a redundant `useMemo`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
Wait — actually, let me re-verify. I need to be more careful.
**Minor issues**
- **`web/src/editor/GardenCanvas.tsx:100-103`** — During a plop drag, `livePlanting` updates on every `pointermove`. The `renderedPlops` `useMemo` maps over the entire `plantings` array (O(n) in total plantings) to create a new array with one element replaced. For a garden with many plantings, this allocates a large array on every animation frame and forces `PlopLayer` to re-render. The same pattern exists for `liveObject` (which the PR explicitly calls "cheap"), but plantings are the "many" side of the relationship and can be an order of magnitude more numerous.
**Fix:** Pass `plantings` and `livePlanting` separately to `PlopLayer`; merge inline in the plop render loop (O(1) extra per rendered plop) rather than doing a full-array copy in `GardenCanvas`.
- **`web/src/editor/PlopLayer.tsx:48-53`** — `byObject` is rebuilt from scratch on every `PlopLayer` render, even when only `scale` (zooming) or `objects` (object drag) changed and `plantings` is unchanged. During a continuous zoom or object drag, this is an unnecessary O(total_plantings) rebuild on every frame.
**Fix:** Wrap the `byObject` construction in `useMemo(() => { ... }, [plantings])`.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
* **`web/src/editor/PlopInspector.tsx:142`** — **Cannot clear the "Planted" date once set.** The `onBlur` handler guards with `planted !== ''`, so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should send `plantedAt: planted || null` and remove the empty-string guard.
* **`web/src/pages/GardenEditorPage.tsx:64`** / **`web/src/editor/GardenCanvas.tsx:178`** — **Stale `?focus` IDs leave the canvas permanently dimmed with no visible exit affordance.** `setFocusedObject(focus ?? null)` is adopted from the URL without validating that the object exists after the garden loads. `GardenCanvas` and `PlopLayer` dim every object when `focusedObjectId != null`, but `GardenEditorPage` hides the breadcrumb (`← garden`) when the object is missing. On mobile there is no Escape key, so the user must discover that an empty-space tap exits. An effect should clear `focusedObjectId` when `objects.length > 0 && !objects.find(o => o.id === focusedObjectId)`.
* **`web/src/lib/objects.ts:299`** — **`applyPlantingPatch` silently ignores `removedAt` despite it being in `PlantingPatch`.** The optimistic-update function copies every other patch field but omits `removedAt`. Today this is latent because soft-removes use `useRemovePlanting`, but if anyone later calls `useUpdatePlanting` with `removedAt`, the plop will stay visible during the request and only disappear on `onSuccess`, producing a jarring flash.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Correctness / stuck-state:
- PlopInspector: the planted date can now be cleared (blur commits null instead
of being blocked); radius/count reset to the current value on invalid input.
- applyPlantingPatch now applies removedAt too (completes the optimistic patch).
- PlopMarker computes the count live from the current radius, so the on-canvas
near-zoom label updates while resizing (not just the inspector).
- GardenEditorPage clears focus when the focused object vanishes — deleted, or a
stale ?focus id on load — so the canvas can't get stuck dimmed with no exit.
- useRemovePlanting adopts the 409 current row, so a stale-version soft-remove
can be retried instead of looping.
- Placement is gated on plantable, so a deep-linked non-plantable focus can't try
to POST a plop the server would 400.
Dedup / perf (introduced by this PR):
- editor/shared.ts: SELECT_COLOR, HANDLE_PX, MIN_RADIUS_CM, DIMMED_OPACITY, and
an objectTransform() helper, now used by ObjectShape/SelectionOverlay/PlopLayer/
PlopOverlay/PlopMarker/GardenCanvas (unified the transform string + the two
different dim opacities).
- Dropped PlopMarker's always-false `dimmed` prop (dimming is at the group level).
- Build plantsById once in the page and pass it to GardenCanvas.
- Moved the livePlanting subscription into PlopInspector so a plop drag
re-renders only the panel, not the whole page (matches the liveObject pattern).
- Memoized PlopLayer's plop grouping so it doesn't rebuild every pan/zoom frame.
Skipped: gesture-scaffolding hook extraction (risky churn in #11 code),
elliptical clamp for circle objects (consistent with the server's rect bounds),
optimistic create (matches useCreateObject).
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 48ba08e8f2 into main2026-07-19 03:22:51 +00:00
steve
deleted branch phase-5-plops-editor2026-07-19 03:22:51 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #15. Part of epic #20 (Phase 5 — Plops). This is pansy's core vision. Builds on #11 (field editor), #13 (PlantPicker), #14 (plantings API).
Data layer
lib/plantings.ts—ServerPlantingzod shape,EditorPlanting,effectiveCount(override ?? derived), andcomputeDerivedCount— a client mirror of the server'smax(1, round(π·r²/spacing²))for live resize display. Unit-tested.lib/objects.ts—/fullnow typesplantings+plants(wasz.unknown()). AddeduseCreatePlanting/useUpdatePlanting/useRemovePlantingthat patch the sameFullGardencache with the #11 optimistic + 409-rollback contract. A soft-remove (removedAt set) drops the plop from the active list; the version is bumped optimistically to avoid a self-conflict on a fast follow-up.Editor
selectedPlantingId/livePlanting/armedPlant; object vs plop selection are mutually exclusive.focusedObjectId; the canvas animatesfitToRectto the object and dims everything else. A breadcrumb (← garden), Escape, or an empty-space tap exits.focusedObjectIdmirrors to?focusso focus is deep-linkable and survives reload.PlantPicker; the chosen plant stays armed for repeat placement; tapping inside the focused object drops a plop (default radiusmax(1.5·spacing, 15cm)), converted into the object's local frame before POSTing.PlopOverlay(drag body to move, clamped to bounds; edge handle to resize; one PATCH per gesture) +PlopInspector(change plant via picker, radius, count override vs live derived, label, planted date, soft-remove).PlopMarkersemantic zoom (px/cm bands<0.75 / 0.75–3 / >3): far = flat color patch + a dominant-plant tint on the object so beds read at a glance; mid = colored circle + emoji; near = + plant name + count. Plops render inside each object's rotated group, so moving/rotating a bed carries its plops with no planting writes.Notes / choices
Checks
tsc --noEmitclean · 24/24 vitest ·vite buildgreen.Acceptance criteria
?focus; survives reload.removed_at).🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 03:11:58Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -11,40 +15,58 @@ import type { EditorGarden, EditorObject } from './types'const GRID_CM = 100 // 1 m gridconst GRID_MIN_CELL_PX = 6const GRID_MAX_OPACITY = 0.18const DIMMED_OPACITY = 0.35🟡 Focus dimming opacity inconsistent across components
maintainability · flagged by 2 models
🪰 Gadfly · advisory
@@ -45,3 +65,4 @@[garden.widthCm, garden.heightCm],)const plantsById = useMemo(() => new Map(plants.map((p) => [p.id, p])), [plants])🟡 Duplicate plantsById construction in GardenCanvas
maintainability · flagged by 2 models
GardenCanvasrebuildsplantsByIdthat its parent already computed (web/src/editor/GardenCanvas.tsx:68).GardenEditorPageconstructs the sameMapand passes the rawplantsarray down only so the canvas can rebuild it. PassingplantsByIddirectly removes duplicate code and a redundantuseMemo.🪰 Gadfly · advisory
@@ -94,0 +136,4 @@// armed for repeat-placement until Escape / Done).function onPlace(e: ReactPointerEvent) {e.stopPropagation()if (!focusedObject || !armedPlant) return🟡 onPlace allows planting on non-plantable objects when deep-linked
correctness · flagged by 1 model
web/src/editor/GardenCanvas.tsx:139—onPlaceguards withif (!focusedObject || !armedPlant) returnbut never checksfocusedObject.plantable. A deep-link (?focus=<id>) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt. Fix: add|| !focusedObject.plantableto the guard.🪰 Gadfly · advisory
@@ -107,2 +143,2 @@{ onSuccess: (o) => select(o.id) },)const local = worldToLocal(world, { x: focusedObject.xCm, y: focusedObject.yCm }, focusedObject.rotationDeg)const x = Math.max(-focusedObject.widthCm / 2, Math.min(focusedObject.widthCm / 2, local.x))🟡 Clamp-to-object-bounds duplicated between GardenCanvas.onPlace and PlopOverlay.clampLocal
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +7,4 @@import { cmFromSpacing, spacingFromCm, spacingUnitLabel, type UnitPref } from '@/lib/units'import { PlantIcon } from '@/components/plants/PlantIcon'const MIN_RADIUS_CM = 1🟡 MIN_RADIUS_CM duplicated verbatim between PlopInspector.tsx and PlopOverlay.tsx, both new in this PR
maintainability · flagged by 2 models
web/src/editor/PlopOverlay.tsx:11/web/src/editor/PlopMarker.tsx:12—const SELECT_COLOR = '#2f7a3e'is now defined identically in three places: the pre-existingSelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g.editor/colors.ts) or exporting it fromSelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10and `w…🪰 Gadfly · advisory
@@ -0,0 +34,4 @@const remove = useRemovePlanting(gardenId)const rootRef = useRef<HTMLDivElement>(null)const [radius, setRadius] = useState(String(spacingFromCm(plop.radiusCm, unit)))🟡 Four useState initializers duplicated verbatim in the resync effect; extract a sync helper
maintainability · flagged by 1 model
web/src/editor/PlopInspector.tsx:37-40+46-49— The fouruseStateinitializers are duplicated verbatim in the resyncuseEffect. Extract asyncFromPlop(plop, unit)helper (or a smalluseSyncedFieldshook) to keep the two sites from drifting.🪰 Gadfly · advisory
@@ -0,0 +56,4 @@const derived = plant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCountfunction commitRadius() {const v = parseFloat(radius)🟠 commitRadius uses parseFloat which accepts partial parses like '1.2.3' without user feedback
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +62,4 @@if (cm !== plop.radiusCm) patch({ radiusCm: cm })}function commitCount() {🟡 commitCount silently drops invalid input with no reset or feedback, unlike commitRadius which clamps
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:227+web/src/pages/GardenEditorPage.tsx:78-99,118,140+web/src/editor/GardenCanvas.tsx:81-89,178— Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed:ObjectShape.tsxhas no focus-mode gating ononPointerDown— the focused object itself renders at full opacity (GardenCanvas.tsx:178,focusedObjectId !== o.iddimming only applies to other objects) and is fully selectable via the plainselectaction…🪰 Gadfly · advisory
@@ -0,0 +67,4 @@if (plop.count != null) patch({ count: null }) // restore derivedreturn}const n = Number(count)🟡 commitCount silently rejects invalid/float input without user feedback
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +139,4 @@type="date"value={planted}onChange={(e) => setPlanted(e.target.value)}onBlur={() => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted })}🔴 Cannot clear plantedAt date: blur guard prevents null patch and re-sync snaps value back
correctness, error-handling · flagged by 4 models
web/src/editor/PlopInspector.tsx:142— Cannot clear the "Planted" date once set. TheonBlurhandler guards withplanted !== '', so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should sendplantedAt: planted || nulland remove the empty-string guard.🪰 Gadfly · advisory
@@ -0,0 +45,4 @@selectedPlantingId: number | nullonSelectPlop: (id: number) => void}) {const byObject = new Map<number, EditorPlanting[]>()🟠 PlopLayer recomputes full plop grouping + dominant-color scan every frame during any object drag or zoom, not just plop changes
performance · flagged by 2 models
web/src/editor/PlopLayer.tsx:48-53(byObject grouping) and:64(dominantColorcall) —PlopLayeris wrapped inmemo(), but itsobjectsandscaleprops are not stable across common editor gestures, so the memo rarely pays off: - Any object drag/resize/rotate:SelectionOverlay.tsx:81callssetLiveObject(onMove(ev))on every pointermove, andGardenCanvas.tsx:95-98'srendered(passed toPlopLayerasobjects,GardenCanvas.tsx:184) is auseMemokeyed on `live…🪰 Gadfly · advisory
@@ -0,0 +61,4 @@const dimmed = focusedObjectId != null && focusedObjectId !== o.idconst halfW = o.widthCm / 2const halfH = o.heightCm / 2const tint = far ? dominantColor(plops, plantsById) : null🟠 dominantColor recomputed inline on every scale change
maintainability · flagged by 1 model
dominantColoris recomputed inline on every scale change (web/src/editor/PlopLayer.tsx:64). Becausescalechanges during zoom andPlopLayerre-renders with it,dominantColoriterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.🪰 Gadfly · advisory
@@ -0,0 +65,4 @@return (<gkey={o.id}transform={`translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg})`}🟡 Object-local transform string duplicated across 5 files; extract a shared helper
maintainability · flagged by 2 models
web/src/editor/PlopLayer.tsx:69,web/src/editor/PlopMarker.tsx:49,web/src/editor/GardenCanvas.tsx:18— Three separate dim-opacity constants (0.4,0.3,0.35) for the same visual concept. The PR introducedDIMMED_OPACITYin GardenCanvas but PlopLayer hardcoded0.4instead of reusing it (and PlopMarker's0.3is dead anyway). Consolidate to one exported constant.🪰 Gadfly · advisory
@@ -0,0 +81,4 @@plant={plantsById.get(p.plantId)}scale={scale}selected={p.id === selectedPlantingId}dimmed={false}🟠 PlopMarker dimmed prop is always false and redundant with parent group opacity
maintainability · flagged by 2 models
web/src/editor/PlopLayer.tsx:84—dimmed={false}is hardcoded for everyPlopMarker, butPlopMarkerdeclaresdimmed: booleanas a required prop and branches on it (PlopMarker.tsx:49). Dead parameter: the layer already applies dim opacity at the group level (PlopLayer.tsx:69,opacity={dimmed ? 0.4 : 1}), so the marker-leveldimmedcan never betrue. Grep confirms no caller ever passestrue. Either drop the prop fromPlopMarkeror pass the real per-plop dim state.🪰 Gadfly · advisory
@@ -0,0 +24,4 @@scale,selected,dimmed,onSelect,🟡 Dead dimmed prop in PlopMarker
maintainability · flagged by 1 model
PlopMarkercarries a deaddimmedprop (web/src/editor/PlopMarker.tsx:27). The only caller (PlopLayer) always passesdimmed={false}and handles object-level dimming via the parent<g>opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.🪰 Gadfly · advisory
@@ -0,0 +85,4 @@paintOrder="stroke"style={{ pointerEvents: 'none', userSelect: 'none' }}>{plant!.name} · {effectiveCount(plop)}🟠 On-canvas plop label shows stale derivedCount during live resize instead of recomputing from live radius
correctness · flagged by 2 models
web/src/editor/PlopMarker.tsx:88— The near-zoom label always callseffectiveCount(plop)(plop.count ?? plop.derivedCount,web/src/lib/plantings.ts:59-60), the last server-computed value. During a live resize drag,PlopOverlay.startResize(web/src/editor/PlopOverlay.tsx:115-126) only updatesradiusCmon the live plop ({...base, radiusCm: r}), leavingderivedCount/countuntouched, andGardenCanvas'srenderedPlopsmerges that live plop into whatPlopLayer/`PlopMarke…🪰 Gadfly · advisory
@@ -0,0 +8,4 @@const HANDLE_PX = 12const MIN_RADIUS_CM = 1const MAX_RADIUS_CM = 10_000const SELECT_COLOR = '#2f7a3e'🟡 SELECT_COLOR hex constant duplicated across SelectionOverlay/PlopMarker/PlopOverlay instead of shared
maintainability · flagged by 3 models
web/src/editor/PlopOverlay.tsx:11/web/src/editor/PlopMarker.tsx:12—const SELECT_COLOR = '#2f7a3e'is now defined identically in three places: the pre-existingSelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g.editor/colors.ts) or exporting it fromSelectionOverlay.tsx. - **web/src/editor/PlopInspector.tsx:10and `w…🪰 Gadfly · advisory
@@ -0,0 +33,4 @@const scale = useEditorStore((s) => s.viewport.scale)const update = useUpdatePlanting(gardenId)const cleanupRef = useRef<(() => void) | null>(null)🟠 Gesture scaffolding duplicated from SelectionOverlay
maintainability · flagged by 2 models
web/src/editor/PlopOverlay.tsx:36-97vsweb/src/editor/SelectionOverlay.tsx:44-104— ~60 lines of near-identical gesture scaffolding (cleanupRef+useEffectcleanup +begin()+makePointerLocal/makePointerWorld+detach/finish). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (useDragGesture). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.🪰 Gadfly · advisory
@@ -0,0 +42,4 @@[],)const handleCm = HANDLE_PX / scale🟠 Plop move/placement clamps to the object's rectangular bounds even for circle-shaped objects (grow_bag/container), letting plops sit outside the visible circular boundary
correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +118,4 @@e,(ev) => {const p = ptr(ev)const r = Math.max(MIN_RADIUS_CM, Math.min(MAX_RADIUS_CM, Math.hypot(p.x - base.xCm, p.y - base.yCm)))🔴 livePlanting resize omits derivedCount update, so near-zoom marker count stays stale during drag
correctness, error-handling · flagged by 2 models
web/src/editor/PlopOverlay.tsx:121—startResizesets the optimisticlivePlantingto{ ...base, radiusCm: r }but leavesderivedCountunchanged. BecausePlopMarker(near-zoom text) readseffectiveCount(plop)which uses the stalederivedCount, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says “Resizing a plop updates its displayed derived count live”; the inspector panel does recompute…🪰 Gadfly · advisory
@@ -184,0 +220,4 @@count?: number | nulllabel?: string | nullplantedAt?: string | nullremovedAt?: string | null🟠 PlantingPatch.removedAt ignored by applyPlantingPatch causing optimistic cache inconsistency
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -184,0 +265,4 @@/** Soft-remove ("Remove"): PATCH removed_at to today; the row is kept but leaves* the active list. Distinct from a hard delete. */export function useRemovePlanting(gardenId: number) {🟠 useRemovePlanting has no 409-conflict adoption and no onSettled refetch, so a version-conflict on soft-remove leaves the plop with a stale version and the user stuck until reload
error-handling · flagged by 1 model
web/src/lib/objects.ts:268-286(useRemovePlanting) — A 409 version-conflict on soft-remove leaves the user stuck.onError(line 281) only restoresctx.prev(the pre-removal snapshot, which carries the stale version) and toasts a generic "Could not remove that plant." It neither adopts the 409'scurrentrow (the wayuseUpdatePlantingdoes at line 250-257 viaconflictPlanting) nor refetches (the wayuseDeleteObjectdoes at line 182 viaonSettled). So after a failed remove…🪰 Gadfly · advisory
@@ -184,0 +278,4 @@patchFullCache(qc, gardenId, (full) => ({ ...full, plantings: full.plantings.filter((p) => p.id !== id) }))return { prev }},onError: (err, _vars, ctx) => {🟠 useRemovePlanting onError doesn't adopt the 409 conflict's current row, so a stale-version remove loops on 409 and the plop can't be removed until an unrelated refetch
error-handling · flagged by 1 model
useRemovePlantingdoesn't reconcile a 409 conflict, leaving the user stuck.web/src/lib/objects.ts:281-284: theonErroronly restoresctx.prevand shows a generic toast.useUpdatePlanting(lines 250-257) instead callsconflictPlanting(err)and, on a conflict, writes the server'scurrentrow into the cache so the next edit carries a freshversion.useRemovePlantinglacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)…🪰 Gadfly · advisory
@@ -190,0 +296,4 @@* for objects), so a fast follow-up edit doesn't self-conflict. derivedCount is* left as-is here; the editor recomputes it live from the plant's spacing and* onSuccess writes the authoritative value. */function applyPlantingPatch(p: ServerPlanting, patch: PlantingPatch): ServerPlanting {🟠 applyPlantingPatch omits removedAt, causing latent optimistic-update mismatch
correctness, error-handling, maintainability · flagged by 4 models
web/src/lib/objects.ts:299—applyPlantingPatchsilently ignoresremovedAtdespite it being inPlantingPatch. The optimistic-update function copies every other patch field but omitsremovedAt. Today this is latent because soft-removes useuseRemovePlanting, but if anyone later callsuseUpdatePlantingwithremovedAt, the plop will stay visible during the request and only disappear ononSuccess, producing a jarring flash.🪰 Gadfly · advisory
@@ -38,2 +63,2 @@return reset}, [gid, select, setArmedKind, setLiveObject])clear()setFocusedObject(focus ?? null)🟠 Stale ?focus IDs not validated after load: canvas stays dimmed with no visible exit UI
error-handling · flagged by 1 model
web/src/pages/GardenEditorPage.tsx:64/web/src/editor/GardenCanvas.tsx:178— Stale?focusIDs leave the canvas permanently dimmed with no visible exit affordance.setFocusedObject(focus ?? null)is adopted from the URL without validating that the object exists after the garden loads.GardenCanvasandPlopLayerdim every object whenfocusedObjectId != null, butGardenEditorPagehides the breadcrumb (← garden) when the object is missing. On mobile there is no Esc…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 34 findings (12 with multi-model agreement)
web/src/editor/PlopInspector.tsx:142web/src/lib/objects.ts:299web/src/editor/PlopOverlay.tsx:11web/src/editor/PlopOverlay.tsx:121web/src/editor/PlopLayer.tsx:48web/src/editor/PlopLayer.tsx:84web/src/editor/PlopMarker.tsx:88web/src/editor/PlopOverlay.tsx:36web/src/editor/GardenCanvas.tsx:18web/src/editor/GardenCanvas.tsx:68web/src/editor/PlopInspector.tsx:10web/src/editor/PlopLayer.tsx:6822 single-model findings (lower confidence)
web/src/editor/Inspector.tsx:227web/src/editor/PlopInspector.tsx:59web/src/editor/PlopLayer.tsx:64web/src/editor/PlopOverlay.tsx:45web/src/editor/SelectionOverlay.tsx:73web/src/lib/objects.ts:223web/src/lib/objects.ts:268web/src/lib/objects.ts:281web/src/pages/GardenEditorPage.tsx:64web/src/editor/GardenCanvas.tsx:139web/src/editor/GardenCanvas.tsx:144web/src/editor/PlopInspector.tsx:37web/src/editor/PlopInspector.tsx:65web/src/editor/PlopInspector.tsx:70web/src/editor/PlopMarker.tsx:27web/src/editor/PlopMarker.tsx:33web/src/editor/PlopOverlay.tsx:130web/src/editor/SelectionOverlay.tsx:7web/src/lib/objects.ts:200web/src/pages/GardenEditorPage.tsx:35web/src/pages/GardenEditorPage.tsx:49web/src/editor/GardenCanvas.tsx:152Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Review
VERDICT: No material issues found
I focused on the new plops data/mutation layer (
lib/plantings.ts,lib/objects.ts), the new canvas/inspector components (PlopLayer,PlopMarker,PlopOverlay,PlopInspector), and the store/page wiring, checking specifically for injection, unsafe deserialization, missing validation, and unsafe rendering of untrusted data.What I verified:
dangerouslySetInnerHTML,eval,Function(,innerHTML— none present. Plant name/label/icon are rendered as plain React/SVG text content (web/src/editor/PlopMarker.tsx:79,:87), which React escapes; free-text fields (label,plantedAt) inPlopInspector.tsxonly ever populate controlled<input>values, never raw markup.serverPlantingSchema.parse(...)wraps every mutation response inlib/objects.ts:203-284, andfullGardenSchemanow typesplantings/plantsinstead ofz.unknown()(lib/objects.ts:37-42), closing off a prior any-shape trust gap rather than introducing one.objectId,idtypednumberfrom zod-parsed rows) —/objects/${objectId}/plantings,/plantings/${id}(lib/objects.ts:204,:230,:273) — no string interpolation of user-typed text into paths.?focusparam is validated upstream by the router (web/src/router.tsx:93-96, pre-existing, unchanged by this diff) to a positive integer beforeGardenEditorPage.tsxever uses it, so it can't be used to inject anything into a request; it only gates which object is displayed.conflictPlanting,lib/objects.ts:314-320) safely parseserr.body.currentviaserverPlantingSchema.safeParse, same pattern as the existing object mutations — no blind trust of attacker/server-adjacent JSON.commitCount/commitRadiusinPlopInspector.tsx:63-77) rejects non-finite/non-integer input before sending a PATCH.Nothing in this diff introduces a new trust boundary crossing, unescaped rendering path, or unvalidated identifier usage relative to the existing (already-reviewed) object-mutation pattern it mirrors. The diff is frontend-only; server-side authorization for the new
/objects/:id/plantingsand/plantings/:idendpoints isn't part of this change set and wasn't assessed.🎯 Correctness — Minor issues
Both findings confirmed against the actual source. No shape-aware clamping exists in either
onPlace(GardenCanvas.tsx:144-145) orclampLocal(PlopOverlay.tsx:62-65), and the on-canvas label genuinely uses the staleeffectiveCountduring a live resize while the Inspector panel independently recomputes correctly. Both draft findings hold up.VERDICT: Minor issues
web/src/editor/PlopMarker.tsx:88— The near-zoom label always callseffectiveCount(plop)(plop.count ?? plop.derivedCount,web/src/lib/plantings.ts:59-60), the last server-computed value. During a live resize drag,PlopOverlay.startResize(web/src/editor/PlopOverlay.tsx:115-126) only updatesradiusCmon the live plop ({...base, radiusCm: r}), leavingderivedCount/countuntouched, andGardenCanvas'srenderedPlopsmerges that live plop into whatPlopLayer/PlopMarkerrender. MeanwhilePlopInspector.tsx:56independently recomputescomputeDerivedCount(plop.radiusCm, plant.spacingCm)for its own placeholder, using the liveselectedPlop(GardenEditorPage.tsx:selectedPlop = ... livePlanting?.id === rawSelectedPlop.id ? livePlanting : rawSelectedPlop). So the on-canvas "name · count" text stays frozen at the pre-drag count while the Inspector panel updates live, and the canvas label only catches up after the PATCH round-trips.Fix: in
PlopMarker, whenplop.count == null, computeplant ? computeDerivedCount(plop.radiusCm, plant.spacingCm) : plop.derivedCountinstead ofeffectiveCount(plop).web/src/editor/GardenCanvas.tsx:144-145(onPlace) andweb/src/editor/PlopOverlay.tsx:62-65(clampLocal) — Both clamp a plop's local position to the object's rectangular bounding box (±widthCm/2, ±heightCm/2) regardless ofobject.shape. Confirmedweb/src/editor/kinds.ts:19-20thatgrow_bagandcontainerareshape: 'circle', andinternal/service/objects.go:22-24confirms both areplantable: true.ObjectShape.tsx:68-69andSelectionOverlay.tsx:167-175both special-caseshape === 'circle'with an<ellipse>, an established convention this code doesn't follow. Net effect: a plop can be placed or dragged into the square "corners" outside the actual circular pot/bag boundary that's drawn.Fix: for
shape === 'circle', clamp to the ellipse (scale the offset vector bymin(1, 1/hypot(x/halfW, y/halfH))) instead of the rectangle, in both the placement clamp andPlopOverlay.clampLocal.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified exactly against the source. Here's the corrected review.
VERDICT: Minor issues
Reviewed through the code-cleanliness/maintainability lens only (verified against the actual checked-out files, not just the diff).
web/src/editor/PlopOverlay.tsx:11/web/src/editor/PlopMarker.tsx:12—const SELECT_COLOR = '#2f7a3e'is now defined identically in three places: the pre-existingSelectionOverlay.tsx:11, and the two new plop files added by this PR. A future "change the selection color" edit will silently miss two of three spots. Suggest hoisting to a small shared constants module (e.g.editor/colors.ts) or exporting it fromSelectionOverlay.tsx.web/src/editor/PlopInspector.tsx:10andweb/src/editor/PlopOverlay.tsx:9—const MIN_RADIUS_CM = 1is duplicated verbatim between two files that are both new in this PR (not pre-existing debt). One enforces the floor on typed radius input, the other on drag-resize; both should stay in sync but there's nothing wiring them together. Worth exporting once fromlib/plantings.ts(which already ownscomputeDerivedCountand other plop domain logic) and importing in both.web/src/editor/GardenCanvas.tsx:201,PlopLayer.tsx:68,PlopOverlay.tsx:130— the SVG local-frame transform string`translate(${x} ${y}) rotate(${deg})`is now written out five times total (two pre-existing inObjectShape.tsx:64/SelectionOverlay.tsx:165, three new ones from this PR).lib/geometry.tsalready centralizes the local-frame math (localToWorld/worldToLocal), but the SVG-transform string itself isn't factored out, so this PR tripled a copy-paste that a smalllocalFrameTransform(center, rotationDeg)helper (or a<LocalFrame>wrapper component) would have avoided.Nothing else stood out structurally — the new files (
PlopLayer,PlopMarker,PlopOverlay,PlopInspector,lib/plantings.ts) are appropriately sized, named consistently with existingObjectShape/SelectionOverlay/Inspectorcounterparts, and thebegin(onMove, fields)abstraction inPlopOverlay.tsxis a reasonable way to share the move/resize gesture plumbing rather than duplicating it.⚡ Performance — Minor issues
Both findings verified against the actual code. Final corrected review:
VERDICT: Minor issues
Findings (⚡ Performance lens)
web/src/editor/PlopLayer.tsx:48-53(byObject grouping) and:64(dominantColorcall) —PlopLayeris wrapped inmemo(), but itsobjectsandscaleprops are not stable across common editor gestures, so the memo rarely pays off:SelectionOverlay.tsx:81callssetLiveObject(onMove(ev))on every pointermove, andGardenCanvas.tsx:95-98'srendered(passed toPlopLayerasobjects,GardenCanvas.tsx:184) is auseMemokeyed onliveObject, so it produces a new array reference every pointermove even when only one unrelated object's geometry changed. SincePlopLayer'smemo()does a shallow prop comparison, the new array reference busts it, forcing the fullbyObjectgrouping (O(total active plops in the garden)) to redo every frame — even for objects with no plops at all.viewport.scalechanges continuously during zoom (useViewport.tsonWheel/onPinch) and is passed straight through asscale={viewport.scale}(GardenCanvas.tsx:187), also busting the memo. (Plain panning only changestx/ty, notscale, so panning alone does not trigger this.)byObjectgrouping runs unconditionally before thefarcheck (GardenCanvas.tsx...PlopLayer.tsx:54), so it's redone every frame even when the tint isn't rendered at near zoom.PlopLayerhas no internaluseMemo, confirmed by reading the full component body — the grouping and (when far) dominant-color scan are plain per-render work with no memoization independent ofobjects/scaleidentity.useMemokeyed onplantings/plantsById(independent ofobjects/scalereference churn) would restore effective memoization.web/src/pages/GardenEditorPage.tsx:35— unlikeliveObject, whose value is only read insideGardenCanvas(the page only imports thesetLiveObjectsetter, confirmed at line 34),livePlanting's value is subscribed at the page level to computeselectedPlop(lines 119-121). SinceGardenCanvasis not wrapped inmemo()(confirmed —export function GardenCanvasatGardenCanvas.tsx:32has no memo wrapper), every pointermove of a plop move/resize (PlopOverlay.tsx:75callssetLivePlantingon each move) re-renders the entireGardenEditorPagetree, includingGardenCanvasandPlopInspector, rather than staying scoped to the canvas the way object drags do. This is bounded (a coupleO(n).find()scans plus JSX re-creation) but is an avoidable inconsistency with the establishedliveObjectpattern in the same file, and compounds with thePlopLayerissue since it's another source of prop-identity churn during plop-drag gestures.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/editor/Inspector.tsx:227+web/src/pages/GardenEditorPage.tsx:78-99,118,140+web/src/editor/GardenCanvas.tsx:81-89,178— Deleting the currently-focused object leaves the editor stuck in a broken focus state. Confirmed:ObjectShape.tsxhas no focus-mode gating ononPointerDown— the focused object itself renders at full opacity (GardenCanvas.tsx:178,focusedObjectId !== o.iddimming only applies to other objects) and is fully selectable via the plainselectaction (store.ts:59, no guard againstfocusedObjectId). Selecting it opensInspector, whose Delete flow (Inspector.tsx:225-227) callsselect(null)anddel.mutate(object.id)but never touchesfocusedObjectId. After the optimistic delete (objects.ts:175),objectsno longer contains that id, so:GardenEditorPage.tsx:118'sfocusedObjectbecomesnull, hiding the breadcrumb block (GardenEditorPage.tsx:144);focusedObjectId == null && <Palette/>(GardenEditorPage.tsx:140) stays false, hiding the palette; andGardenCanvas.tsx:85-86(targetundefined,focusedObjectId != nullstill true) returns early every render without updatingfitKeyRef, permanently short-circuiting the fit effect. Verified the Escape handler (GardenEditorPage.tsx:86-98) is the only way out — withselectedId/selectedPlantingIdboth null andfocusedObjectIdstill set, it falls through toexitFocus(). Reachable via normal use (focus a plantable bed, select it while focused, delete it).web/src/editor/PlopInspector.tsx:65-73— ConfirmedcommitCountreturns without patching or resetting local state on non-integer/zero/negative input, and — unlike siblingcommitRadius(PlopInspector.tsx:58-63, which clamps viaMath.max(MIN_RADIUS_CM, ...)) — never corrects the value. Since no patch fires,plop.countnever changes, so the re-syncuseEffect(keyed onplop.count, line 50) never re-triggers either — the invalid text sits in the field indefinitely with no feedback.web/src/lib/objects.ts:299-311— ConfirmedapplyPlantingPatchomitsremovedAtfrom its spread despitePlantingPatch.removedAtbeing declared (objects.ts:223). Grepped the repo: no call site currently invokesuseUpdatePlanting.mutatewithremovedAt— the only setter ofremovedAtisuseRemovePlanting(objects.ts:268-286), which bypassesapplyPlantingPatchentirely via its ownonMutate. So this is currently dead but is a real latent optimistic-cache gap for any future directremovedAtupdate throughuseUpdatePlanting.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I traced the tainted-input paths through this diff from a security perspective and found nothing material:
?focusquery param — validated inweb/src/router.tsx:93-96as a positive integer (Number.isInteger(f) && f > 0) before reaching the store/canvas. No injection or type-coercion risk. Verified by reading the route./objects/${objectId}/plantings(objects.ts) and/plantings/${id}(objects.ts) useobjectId/idtyped asnumber, sourced from zod-validated server rows (serverPlantingSchema/serverObjectSchema) and typedEditorObject/EditorPlanting. No string path injection possible. Verified viaobjects.tsandplantings.ts.color/icon/namearez.string()from the API (plants.ts:31-32) and rendered as SVG attributes (fill={color}) / React text children inPlopMarker.tsx/PlopLayer.tsx, and as a CSSbackgroundColorviacolor-mix()inPlantIcon.tsx. React sets these throughsetAttribute/textContent (escaped) and the CSSOMstyle.backgroundColorsetter (invalid values dropped per-property), so there's no HTML/CSS injection vector. NodangerouslySetInnerHTML/innerHTMLanywhere (grep confirmed).commitCountrejects non-integers< 1(PlopInspector.tsx);commitRadiusclamps toMIN_RADIUS_CM;computeDerivedCountguards<= 0. These bound what's POSTed/PATCHed. Server-side authz/length limits forlabeletc. are backend concerns outside this (frontend) diff.apiwithcredentials: 'same-origin'(api.ts:97), same as existing object mutations; no new secret handling or leakage.The data is same-origin, authenticated, zod-parsed, and all identifiers are numeric. Nothing in the security lane is wrong here.
🎯 Correctness — Minor issues
Both findings confirmed against the actual source.
Finding 1 —
PlopInspector.tsx:142(the draft cited 148; actual line is 142). The blur handler is exactly:Clearing a set date (
planted="",plop.plantedAt="2024-01-01"): first clause"" !== "2024-01-01"is true, secondplanted !== ''is false → no PATCH. Confirmed. The label field at line 133 correctly sendsnullon clear (label.trim() || null). Asymmetry is real.Finding 2 —
objects.ts:223declaresremovedAt?: string | nullonPlantingPatch;applyPlantingPatch(lines 299–311) has noremovedAtbranch. Confirmed. Soft-removal is handled separately byuseRemovePlanting(its ownonMutatefilters the plop, lines 275–279), anduseUpdatePlanting'sonSuccessdoes drop the plop whenupdated.removedAtis set (lines 245–246) — but the optimisticonMutatepass viaapplyPlantingPatchwould not drop it, only bump the version. So it's a latent inconsistency, not an active bug. Confirmed as described.Verdict: Minor issues
web/src/editor/PlopInspector.tsx:142— planted date cannot be cleared. The blur guard isplanted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). If a plop hasplantedAt = "2024-01-01"and the user empties the field (planted = ""), the first clause is true butplanted !== ''is false, so no PATCH is sent and the stale date persists. The label field (line 133) correctly sendsnullwhen cleared (patch({ label: label.trim() || null })); the planted field should do the same (plantedAt: nullwhen emptied).web/src/lib/objects.ts:299—applyPlantingPatchignoresremovedAtthoughPlantingPatchdeclares it.PlantingPatchincludesremovedAt?: string | null(line 223), butapplyPlantingPatch(lines 299–311) has noremovedAtbranch. Soft-removal normally goes throughuseRemovePlanting(which filters the plop in its ownonMutate), anduseUpdatePlanting.onSuccessdoes drop the plop whenupdated.removedAtis set (lines 245–246) — so this is a latent inconsistency, not an active bug. But if any caller usesuseUpdatePlanting({ removedAt }), the optimistic pass would leave the plop visible (only the version bump applies) until the round-trip completes.🧹 Code cleanliness & maintainability — Minor issues
All findings verified. All six findings confirmed against actual code. Producing the corrected review.
Corrected review
Verdict: Minor issues
Through the cleanliness/maintainability lens, the new plop code repeats a few patterns the surrounding files already pay for, and leaves a vestigial prop. Nothing blocking, but a small shared-helper pass would cut real churn.
web/src/editor/PlopMarker.tsx:33— vestigialdimmedprop.PlopLayeralways renders<PlopMarker ... dimmed={false} />(PlopLayer.tsx:84), and the actual focus-dimming is applied one level up on the object<g opacity={dimmed ? 0.4 : 1}>(PlopLayer.tsx:69). SoPlopMarker'sdimmed/opacity={dimmed ? 0.3 : 1}branch (PlopMarker.tsx:49) is dead — a prop that can never be true. Either drive the marker's own opacity from it (and drop the group opacity) or remove the prop to avoid misleading future readers.Object-local transform string duplicated across 5 sites.
translate(${o.xCm} ${o.yCm}) rotate(${o.rotationDeg})is hand-built inGardenCanvas.tsx:201,PlopLayer.tsx:68,PlopOverlay.tsx:130, plus pre-existingSelectionOverlay.tsx:165andObjectShape.tsx:64. This PR adds two new copies of the core "plops live in the object's local frame" transform. A singleobjectGroupTransform(o: EditorObject): string(or a<ObjectGroup object={o}>wrapper) would centralize it — right now a rotation/transform convention change means editing five places.SELECT_COLOR = '#2f7a3e'redefined in 4 files.SelectionOverlay.tsx:11,ObjectShape.tsx:59(inline), and the newPlopMarker.tsx:12/PlopOverlay.tsx:11each redeclare the same constant (and#00000033for the unselected stroke). The PR extends an existing pattern, but this is the moment to hoist one sharededitor/colorsconstant before it spreads further.Three different "dimmed by focus" opacities.
GardenCanvas.tsx:18DIMMED_OPACITY = 0.35(for non-focused objects),PlopLayer.tsx:690.4(for non-focused plop groups),PlopMarker.tsx:490.3(the deaddimmedbranch). Same visual concept, three magic numbers — pick one and share it.Duplicated clamp-to-object-bounds.
GardenCanvas.tsx:144-145inlines the±widthCm/2/±heightCm/2clamp for placement, andPlopOverlay.tsx:62-65hasclampLocaldoing exactly the same math. A sharedclampToLocalFrame(p, object)ingeometry.ts(next toworldToLocal) would remove the copy and keep the bounds invariant in one place.plantsByIdrebuilt in two components.GardenCanvas.tsx:68andGardenEditorPage.tsx:49both donew Map(plants.map((p) => [p.id, p])). Minor, but since the page already ownsplants, it could pass the map down (or a memoized selector) instead of reconstructing it. Low priority.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, the change is sound. I verified the hot paths:
PlopOverlay) — updateslivePlantingperpointermove;renderedPlopsis rebuilt viauseMemoon[plantings, livePlanting](GardenCanvas.tsx:100-103), but non-dragged plops keep object identity, andPlopMarkerismemo'd with stableplantsById/onSelect/scaleprops, so only the dragged marker actually re-renders. One PATCH per gesture on release. Good.liveObjectchanges per move;renderedrebuilds (O(n) map) andPlopLayerre-renders because itsobjectsprop changes identity, rebuilding thebyObjectmap (O(plantings)) and iterating objects. This is O(objects+plantings) of reconciliation work per pointermove, butPlopMarkermemo prevents DOM work on unchanged plops. Acceptable for realistic garden sizes; not a regression in kind (the object list already re-mapped per move before this PR).useCreatePlanting/useUpdatePlanting/useRemovePlantingpatch the existingFullGardencache on success (objects.ts) with noinvalidateQueries/refetch. Confirmed by reading the mutation bodies.fitKeyRefso unrelated re-renders (plop add, bed drag, cache updates that produce a newobjectsarray) hit the early return and don't refit. Verified at GardenCanvas.tsx:78-87.dominantColor(PlopLayer.tsx:8-23) allocates aMapper planted object per render in far-zoom mode; during a pan/zoom this is O(total plops) of map ops per frame. Only material for very large gardens with many plops at far zoom — auseMemo/hoisted cache could avoid it, but I would not call it a blocking regression.One trivial note (not worth a finding on its own):
plantsByIdis built in bothGardenEditorPage.tsx:49andGardenCanvas.tsxvia its ownuseMemo— duplicate but O(plants) and memoized, negligible.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
PlopInspector— planted date can never be cleared.web/src/editor/PlopInspector.tsx:142: theonBluris() => planted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). Theplanted !== ''guard short-circuits when the user empties the date field, so no PATCH is sent andplantedAtcannot be reset tonull. This is an asymmetry with the sibling fields:commitCount(line 66-68) sendspatch({ count: null })when the count is cleared, and the labelonBlur(line 133) sendspatch({ label: label.trim() || null }), allowing a reset. Once a plop has a planted date, the inspector offers no way to remove it. Fix: allow the empty case to PATCHplantedAt: null, e.g. handleplanted === ''separately (PATCHnullwhenplop.plantedAt != null) before the non-empty comparison.useRemovePlantingdoesn't reconcile a 409 conflict, leaving the user stuck.web/src/lib/objects.ts:281-284: theonErroronly restoresctx.prevand shows a generic toast.useUpdatePlanting(lines 250-257) instead callsconflictPlanting(err)and, on a conflict, writes the server'scurrentrow into the cache so the next edit carries a freshversion.useRemovePlantinglacks that branch: on a stale-version 409 it rolls the plop back into the cache with the original (now stale)versionand no refetch is triggered (noonSettled/invalidate). The user clicks "Remove" again, the same staleversionis sent, the server 409s again — the plop can't be removed until an unrelated refetch refreshes the version. Fix: mirror the update hook — on aconflictPlanting(err)result, writecurrentinto the cache instead of only restoringprev(and/or re-arm the remove with the new version).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
After tracing data flow, input paths, auth boundaries, and rendering paths through the security lens, I found no authn/authz gaps, injection vectors, unsafe deserialization, secret leakage, or missing validation that would constitute a material security issue in this PR.
?focusURL parameter is validated as a positive integer inrouter.tsx:93-95before reaching state logic.stopPropagation(), preventing gesture confusion or click-through to unintended actions.objects.tshandle 409 conflicts correctly.same-originHttpOnly session cookies.🎯 Correctness — Blocking issues found
Blocking issues found
Findings
web/src/editor/PlopOverlay.tsx:121—startResizesets the optimisticlivePlantingto{ ...base, radiusCm: r }but leavesderivedCountunchanged. BecausePlopMarker(near-zoom text) readseffectiveCount(plop)which uses the stalederivedCount, the count label stays at its old value for the entire drag and only updates after the server PATCH returns. The acceptance criterion says “Resizing a plop updates its displayed derived count live”; the inspector panel does recompute it viacomputeDerivedCount, but the canvas marker does not.Fix: pass the plant (or its
spacingCm) intoPlopOverlayand include a livederivedCountin the optimisticlivePlantingpayload, or havePlopMarkerrecomputederivedCountfromplop.radiusCmandplant.spacingCmthe same wayPlopInspectordoes.web/src/editor/PlopInspector.tsx:142— Theplanteddate blur guard isplanted !== '' && patch({ plantedAt: planted }). If a user clears an existing date,plantedbecomes'', the left-hand condition is false, and no PATCH is ever sent, so the date cannot be removed through the UI. The server schema acceptsplantedAt: nullfor clearing.Fix: change the handler to send
plantedAt: planted || nullwhenever the normalized value differs from the current server value, e.g.onBlur={() => { const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next }) }}.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
PlopMarkerdimmedprop is dead code —PlopLayer.tsx:84hardcodesdimmed={false}while the parent<g>already applies opacity dimming at line 69. Thedimmedparameter and the conditionalopacity={dimmed ? 0.3 : 1}inPlopMarker.tsx:26-27,49are never triggered and create confusion about where dimming lives. Remove the prop and inline opacity fromPlopMarker.Pointer-gesture scaffolding is copy-pasted between overlays —
PlopOverlay.tsx:36-97duplicates thecleanupRef,useEffectteardown,begin,move/detach/finishpattern fromSelectionOverlay.tsx:44-104almost line-for-line. The only meaningful differences are the store setter names (setLivePlantingvssetLiveObject) and the mutator type. Extract a shareduseDragGesturehook so gesture fixes (e.g., addinglostpointercapturehandling) don't have to be applied in two places.PlantingPatchinterface advertisesremovedAtbutapplyPlantingPatchignores it —objects.ts:223declaresremovedAt?: string | nullin the patch type, yet the optimistic-update helper (objects.ts:299-311) never spreads it. If a caller ever passesremovedAttouseUpdatePlanting, the cache will be optimistically inconsistent until the server response arrives. Either addremovedAttoapplyPlantingPatchor remove it fromPlantingPatch(it's already handled by the dedicateduseRemovePlantinghook).Focus-mode dimming uses inconsistent opacity constants — Objects dim to
0.35(GardenCanvas.tsx:18), plop groups dim to0.4(PlopLayer.tsx:69, magic number), and the deadPlopMarkerpath uses0.3(PlopMarker.tsx:49). These three values represent the same visual concept; unify them under one exported constant.SELECT_COLORis defined identically in three files —#2f7a3eappears atPlopOverlay.tsx:11,PlopMarker.tsx:12, andSelectionOverlay.tsx:11. Extract it to a shared constant so the selection accent color can be changed in one place.⚡ Performance — Minor issues
Minor issues
web/src/editor/PlopLayer.tsx:48-64—byObjectgrouping anddominantColorrecomputed on every zoom/pan framePlopLayerrebuilds thebyObjectMap from scratch on every render, and callsdominantColor(which iterates an object's plops and builds a frequency Map) for every planted object wheneverscale < SEMANTIC_FAR. Becausescalechanges on every frame during a zoom or pan gesture, this work repeats even though neitherplantingsnorplantsByIdhave changed. For a garden with many plantings this creates unnecessary CPU load on the render thread.Fix: Wrap
byObjectinuseMemokeyed onplantingsonly. Precompute dominant-color tints in the same memo (or a second one keyed onplantingsandplantsById) so that pure zoom/pan frames skip the recomputation entirely.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
PlopInspector.tsx:59—commitRadiususesparseFloat(radius).parseFloat('1.2.3')returns1.2, which is accepted as valid even though the user typed garbage. The field then silently commits a value the user did not intend, with no error feedback. Fix: validate withNumber(radius)and a regex orisNaNcheck, and surface a field-level error rather than silently swallowing.PlopInspector.tsx:70—commitCountusesNumber(count). WhileNumber.isIntegerrejectsNaN, inputs like'3.5'silently do nothing with no feedback. An invalid typed value just evaporates on blur. Fix: provide explicit validation feedback in the UI.PlopOverlay.tsx:121—startResizecomputes radius viaMath.hypot(p.x - base.xCm, p.y - base.yCm)clamped only byMAX_RADIUS_CM. Unlike the move gesture, resize is not clamped to the parent object's bounds, so a plop can be resized to spill outside its bed. The overlay comment says "clamped to bounds" for move but omits resize. Fix: clamp radius so the plop circle stays inside the object's local rectangle, or document that spillover is intentional.lib/objects.ts:200—useCreatePlantinghas no optimisticonMutate. On slow networks a user may tap the placement sheet repeatedly, believing nothing happened, and create duplicate plops. The update mutation has optimistic patching; create should mirror it for repeat-placement UX. Fix: optimistically append the new plop to the cache with a temporary ID.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I'll investigate this PR through the security lens. Let me verify the key files.
Now let me check how
labelis rendered and where planting bodies are constructed — the user-provided label goes to the server, but let me verify how it could be displayed back. Also checkInspector.tsxto confirm theonFocusauthz angle (no authz on the client).Now I have the picture. Let me check
serverPlantingSchema—labelisz.string()with no max length, andcolor/iconfromplantSchemaare unbounded strings that get interpolated into SVG and CSS. Let me verify the PlantIcon'scolorusage path and whether thecolor/icon/name/labelare sanitized anywhere.Let me verify the
iconandcolorrendering at the SVG sink points and confirm there's no React auto-escaping concern (text content in SVG is auto-escaped, so XSS via<text>{plant.icon}</text>is fine). ThePlantIconusesstyle={{ backgroundColor: color-mix(...) }}with template interpolation.I have enough to conclude. Let me consolidate the security review.
Key findings:
PlantIcon— potential CSS injection viacolor:PlantIcon.tsx:12interpolatesplant.color(az.string()with no format constraint) into astyle={{ backgroundColor: \color-mix(in srgb, ${color} 18%, transparent)` }}. A maliciouscolorstring containing;or)or arbitrary CSS could escape thecolor-mix(...)call and inject style properties. But more importantly, React'sstyleprop value as a string does NOT inject;separated properties the way an attribute string would — the whole string is set as thebackground-colorvalue, which the browser will reject as invalid if malformed. So actual CSS injection into other properties is prevented by React's style-assignment semantics. Lower risk, but the unvalidatedcolor/icon/name` are pre-existing concerns, not introduced by this PR. They predate this PR. So I should only flag what's new.PlopMarker.tsx:72,88interpolatesplant.iconandplant.nameinto SVG<text>content. React auto-escapes JSX text content, so no XSS — these are safe.label(serverPlantingSchema.label = z.string()) — user-provided, unbounded, but rendered only as a TextField value (auto-escaped). No XSS sink. No new sink introduced by this PR.Numeric inputs to API:
xCm,yCm,radiusCm,count— clamped client-side (Math.max/Math.min), but the server is responsible for re-validating.plantIdandobjectIdcome from server-loaded lists; the?focus=<id>URL param is validated inrouter.tsx:93-96as a positive integer. No injection vector — they're interpolated into URL paths as numbers.?focus=<id>is just a viewport framing hint; no authz decision is made on it client-side (the objects are already filtered by garden on the server/fullendpoint). Setting?focus=999for an object the user can't see just results in no framing. No IDOR — the server/fullendpoint is the trust boundary and is garden-scoped.Optimistic cache patching is purely client-side UI state; no security implication.
Within my lens (security), nothing material is introduced by this PR. The unbounded string fields (
color/icon/name/label) are pre-existing and rendered safely through React's auto-escaping; thePlantIconcolor interpolation is a pre-existing pattern not added by this PR. The new code adds no new sinks, no new untrusted-input surfaces beyond what's already validated (zod parse on all server responses), and no authz decisions.Verdict: No material issues found.
No material issues found
PlopMarker.tsx:72,88interpolatesplant.iconandplant.nameinto SVG<text>content — React auto-escapes JSX text, so no XSS vector.PlopInspector.tsx:91,95and thePlantIconuse the same auto-escaped text/safestylepath. Confirmed by readingPlopMarker.tsx,PlopInspector.tsx, andPlantIcon.tsx.plop.label(serverPlantingSchema.label: z.string()inplantings.ts:16) is user-authored and unbounded, but is only rendered as a controlled<TextField>value (PlopInspector.tsx:131-133) — auto-escaped, no HTML sink. The lack of a max-length is a pre-existing pattern shared withplantSchema.name/notes; not introduced here and not a security bug under React's text escaping./full, planting create/update/remove) are parsed through zod (serverPlantingSchema,plantSchema) before touching cache/UI (objects.ts:204,230,273), so no unsafe deserialization of raw server payloads.?focus=<id>URL search param is validated to a positive integer inrouter.tsx:93-96and used only to frame the viewport / pick from already-loaded objects; no client authz decision rests on it (the/fullendpoint is the trust boundary and is garden-scoped server-side). No IDOR./objects/${objectId}/plantings,/plantings/${id}) use numeric ids derived from server rows; no injection.PlantIconcolor-mix(in srgb, ${color} …)template interpolation is a pre-existing pattern (also used inPlantCard.tsx:51,PlantPicker.tsx:90) not introduced by this PR, so I'm not flagging it here.Within the security lens, this change introduces no material issues.
🎯 Correctness — Minor issues
Both findings reference line 84 for PlopMarker, but actual line is 88. Let me verify the other claims.
Both findings confirmed. Let me verify the objects.ts optimistic patch and GardenCanvas livePlanting flow.
Both findings confirmed. The line for PlopMarker is 88, not 84 (the diff truncated; the draft said 84 but the actual
{plant!.name} · {effectiveCount(plop)}is on line 88). I'll correct to line 88. The PlopInspector line 142 is correct.VERDICT: Minor issues
web/src/editor/PlopMarker.tsx:88— At near-zoom the plop count renders{plant!.name} · {effectiveCount(plop)}, andeffectiveCount = p.count ?? p.derivedCount(plantings.ts:59-61). During a resize gesture,PlopOverlay.startResize(PlopOverlay.tsx:115-126) setslivePlanting = {...base, radiusCm: r}— the new radius but the oldderivedCount— and the optimistic patch inapplyPlantingPatch(objects.ts:299-311) deliberately leavesderivedCountuntouched.GardenCanvassubstituteslivePlantingintorenderedPlops(GardenCanvas.tsx), so the marker reads the stalederivedCountuntil the PATCH roundtrips viaonSuccess(objects.ts:240-248). The count number therefore does not update live with the radius, contradicting the acceptance criterion "Resizing a plop updates its displayed derived count live". ThecomputeDerivedCountmirror (plantings.ts:66-69) is wired only into the inspector placeholder (PlopInspector.tsx:56), not the marker. Fix: renderplop.count ?? computeDerivedCount(plop.radiusCm, plant?.spacingCm ?? 0)in the marker (importcomputeDerivedCount).web/src/editor/PlopInspector.tsx:142— The planted-dateonBlurguardplanted !== ''makes it impossible to clear a planted date: the PATCH only fires when the value changed and is non-empty.CreatePlantingdefaultsplantedAtto today server-side (plantings.go:97-100), so every plop has a date, and the PATCH API supportsplantedAt: nullviaSetPlantedAt/parseNullableStringsemantics (plantings.go:211-213). The user can change the date but never remove it; meanwhile the local input shows empty while the server retains the old value until the next external resync (the resync effect at PlopInspector.tsx:44-50 would restore it once the field loses focus). Fix: drop theplanted !== ''guard andpatch({ plantedAt: planted || null })on blur when the value changed.🧹 Code cleanliness & maintainability — Minor issues
All findings verified. Let me confirm the few specifics:
PlopLayer.tsx:84 dimmed dead prop — Confirmed.
PlopLayer.tsx:61computesdimmedat the group level (opacity={dimmed ? 0.4 : 1}at line 69), andPlopMarkeralways receivesdimmed={false}at line 84. Grep confirms no caller ever passestrue. Thedimmedprop onPlopMarker(line 33) and itsopacity={dimmed ? 0.3 : 1}(line 49) is unreachable logic.Three dim-opacity constants — Confirmed.
DIMMED_OPACITY = 0.35(GardenCanvas.tsx:18),0.4(PlopLayer.tsx:69),0.3(PlopMarker.tsx:49, dead). Three divergent constants.PlopOverlay gesture scaffolding duplication — Confirmed. PlopOverlay.tsx:36-97 (
cleanupRef+useEffectcleanup +begin()+makePointerLocal+detach/finish) closely mirrors SelectionOverlay.tsx:44-104. ~60 lines of near-identical scaffolding.plantsById duplicate construction — Confirmed. GardenEditorPage.tsx:49 and GardenCanvas.tsx:68 both build
new Map(plants.map((p) => [p.id, p]))from the sameplantsarray passed as prop.applyPlantingPatch omits removedAt — Confirmed.
PlantingPatchdeclaresremovedAt?: string | null(objects.ts:223),useRemovePlantingsends it (line 273), butapplyPlantingPatch(lines 299-311) only copies plantId/xCm/yCm/radiusCm/count/label/plantedAt — noremovedAtline.useRemovePlanting.onMutatefilters the plop out, so remove path is fine; a directremovedAtpatch viauseUpdatePlantingwouldn't apply optimistically. Latent inconsistency.PlopInspector duplicated initializers — Confirmed. PlopInspector.tsx:37-40 (
radius/count/label/plantedinitializers) duplicated verbatim at 46-49 in the resync effect.halfFW/halfFH redundant guards — Confirmed. GardenCanvas.tsx:152-153 guards
focusedObject ?, only read at lines 203-206 inside{focusedObject && armedPlant && (...)}(line 200) wherefocusedObjectis guaranteed non-null.fitKeyRef key redundant garden.id — Confirmed but confidence medium. The key is
${garden.id}:${focusedObjectId}(GardenCanvas.tsx:83). GardenEditorPage resets editor state ongidchange (lines 54-70), andGardenCanvasis rendered without an explicitkeyprop (line 163), so React doesn't remount it on garden change — the page's effect resets state butfitKeyRefref persists. Sogarden.idin the key is actually NOT redundant (it guards against a stale ref pointing at a previous garden's fit key when garden.id changes without unmounting). This finding is questionable.Let me re-examine finding #8 more carefully. The page does NOT pass a
key={garden.id}to GardenCanvas, so the component instance persists across garden changes.fitKeyRefis auseRef, which persists. On garden change,fitKeyRef.currentwould still hold the old key from the previous garden. Withoutgarden.idin the key, the new garden would never refit (because the key matches the stale ref). Sogarden.idis actually NECESSARY. The finding is wrong — drop it.VERDICT: Minor issues
Findings through the 🧹 cleanliness/maintainability lens:
web/src/editor/PlopLayer.tsx:84—dimmed={false}is hardcoded for everyPlopMarker, butPlopMarkerdeclaresdimmed: booleanas a required prop and branches on it (PlopMarker.tsx:49). Dead parameter: the layer already applies dim opacity at the group level (PlopLayer.tsx:69,opacity={dimmed ? 0.4 : 1}), so the marker-leveldimmedcan never betrue. Grep confirms no caller ever passestrue. Either drop the prop fromPlopMarkeror pass the real per-plop dim state.web/src/editor/PlopLayer.tsx:69,web/src/editor/PlopMarker.tsx:49,web/src/editor/GardenCanvas.tsx:18— Three separate dim-opacity constants (0.4,0.3,0.35) for the same visual concept. The PR introducedDIMMED_OPACITYin GardenCanvas but PlopLayer hardcoded0.4instead of reusing it (and PlopMarker's0.3is dead anyway). Consolidate to one exported constant.web/src/editor/PlopOverlay.tsx:36-97vsweb/src/editor/SelectionOverlay.tsx:44-104— ~60 lines of near-identical gesture scaffolding (cleanupRef+useEffectcleanup +begin()+makePointerLocal/makePointerWorld+detach/finish). The PR description itself notes "the same contract as SelectionOverlay (#11)". This is a clear shared-hook candidate (useDragGesture). Not blocking for this PR, but worth a follow-up — every new overlay will copy this again.web/src/pages/GardenEditorPage.tsx:49+web/src/editor/GardenCanvas.tsx:68—plantsByIdis built independently in both files (new Map(plants.map((p) => [p.id, p]))) from the sameplantsarray that the page already passes to the canvas. Either pass the pre-builtMapdown as a prop or lift it; the duplicateuseMemois wasteful and easy to drift.web/src/lib/objects.ts:299-310(applyPlantingPatch) —PlantingPatchdeclaresremovedAt?: string | null(line 223) anduseRemovePlantingsends it (line 273), butapplyPlantingPatchnever copiesremovedAtonto the optimistic row.useRemovePlantinghas its ownonMutatethat filters the plop out, so the remove path is fine, but a directremovedAtpatch viauseUpdatePlantingwouldn't apply optimistically — a latent inconsistency. Add theremovedAtline for symmetry with the other fields (and withapplyServerPatch).web/src/editor/PlopInspector.tsx:37-40+46-49— The fouruseStateinitializers are duplicated verbatim in the resyncuseEffect. Extract asyncFromPlop(plop, unit)helper (or a smalluseSyncedFieldshook) to keep the two sites from drifting.web/src/editor/GardenCanvas.tsx:152-153—halfFW/halfFHguardfocusedObject ?but are only read inside{focusedObject && armedPlant && (...)}(line 200), wherefocusedObjectis guaranteed non-null. The ternaries are dead defensive code; simplify toconst halfFW = focusedObject.widthCm / 2inside the block, or just inline.Verified by reading
PlopLayer.tsx,PlopMarker.tsx,PlopOverlay.tsx,SelectionOverlay.tsx,GardenCanvas.tsx,GardenEditorPage.tsx,objects.ts,PlopInspector.tsx, and grepping fordimmed/plantsByIdusages. (Dropped thefitKeyRefkey-redundancy finding:GardenCanvasis not keyed bygarden.idin its parent, so the ref persists across garden changes andgarden.idin the key is actually necessary to trigger a refit on garden switch.)⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the diff through the performance lens, verifying against the checked-out code.
PlopLayerandPlopMarkerare bothmemo'd. On pan (onlytx/tychange),PlopLayer's props don't change (scaleunchanged,objects/plantings/plantsByIdstable viauseMemo), so it skips re-render — and even if it didn't, eachPlopMarker's props are stable (the parent<g>transform carries position), so they skip too. On zoom, re-renders cascade throughPlopLayer/PlopMarkeronly becausescalelegitimately changes for semantic-zoom bands — necessary work, not waste.selectedPlantingIdchanges,PlopLayerre-renders but only the two markers whoseselectedprop actually flips pass thememocheck; the rest skip (verified atPlopMarker.tsx:21,PlopLayer.tsx:83).PlopLayerbuilds abyObjectmap (O(plops)) once per render and does O(1) lookups (PlopLayer.tsx:48-53);dominantColoris O(plops-per-object) and only in far mode.GardenCanvasdoes three.find()calls per render (selectedObject/selectedPlopObject/focusedObject,GardenCanvas.tsx:104-107) — O(n) each, consistent with the pre-existingselectedlookup pattern, not a new hotspot.useUpdatePlanting/useRemovePlantingdo a singlefull.plantings.map/filteron the cachedFullGarden(objects.ts), one PATCH per gesture — same contract as the object mutations.renderedPlopsrebuilds only whenlivePlantingchanges (during a plop drag), and is a single O(n) map;liveObject's merge is likewise O(n). No per-move re-sort.fitKeyRefprevents a refit on unrelated re-renders (plop add, bed drag) — the comment's claim holds (GardenCanvas.tsx:83-84).plantsByIdMap is built in bothGardenEditorPageandGardenCanvas— trivial duplicate allocation, not material.The new code follows the existing memo discipline carefully; nothing in my lane regresses efficiency for realistic garden/plop sizes.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/editor/PlopInspector.tsx:142— Clearing the "Planted" date is silently dropped. The blur handler isplanted !== (plop.plantedAt ?? '') && planted !== '' && patch({ plantedAt: planted }). Theplanted !== ''guard means an empty (cleared) value never produces a PATCH, soplantedAt: nullis never sent. Worse, because no patch fires, the localplantedstate stays''whileplop.plantedAtis still the old date — the field shows empty but the server keeps the date, and the re-syncuseEffect(deps includeplop.plantedAt, line 50) doesn't fire becauseplop.plantedAtnever changed. Contrast withcommitCount(line 65), which correctly handles the empty case by patching{ count: null }. Fix: mirrorcommitCount— drop theplanted !== ''clause and patch{ plantedAt: planted.trim() ? planted : null }when the value differs fromplop.plantedAt ?? ''. Verified by reading the file.web/src/lib/objects.ts:268-286(useRemovePlanting) — A 409 version-conflict on soft-remove leaves the user stuck.onError(line 281) only restoresctx.prev(the pre-removal snapshot, which carries the stale version) and toasts a generic "Could not remove that plant." It neither adopts the 409'scurrentrow (the wayuseUpdatePlantingdoes at line 250-257 viaconflictPlanting) nor refetches (the wayuseDeleteObjectdoes at line 182 viaonSettled). So after a failed remove, the plop reappears in the cache with its old version; retrying sends that same stale version and 409s again — the user can't recover without reloading. Fix: either adoptconflictPlanting(err)inonError(writecurrentinto the cache so the next attempt uses the fresh version), or addonSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })likeuseDeleteObject. Verified by reading the file and comparing with the sibling object mutations in the same file.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
Verdict: No material issues found
After tracing tainted input through the new planting (plop) editor and focus-mode deep-linking, I found no concrete security issues in this PR.
?focus) are validated to positive integers in the route definition (router.tsx:93-96) before reaching state. No injection or open-redirect vector.label,radius,count,plantedAt) are rendered through React’s normal JSX escaping (nodangerouslySetInnerHTML, no raw SVG text injection). Theplant.iconvalue rendered inside an SVG<text>element is escaped by React and cannot execute scripts.objectId,planting id) already present in cached garden data, not from user input. No SSRF or path-traversal surface.The
applyPlantingPatchhelper omitsremovedAtfrom its optimistic merge, but that is a transient cache-correctness issue (covered by another lens), not an authz bypass or injection vulnerability.🎯 Correctness — Minor issues
Minor issues
web/src/editor/PlopInspector.tsx:142— TheplantedAtblur handler can never clear a date. The conditionplanted !== ''blocks the PATCH when the user empties the field, so a previously-set date persists on the server even though the UI shows an empty input.Fix: change the blur handler to treat an empty string as
null, e.g.const next = planted || null; if (next !== plop.plantedAt) patch({ plantedAt: next }).web/src/editor/PlopOverlay.tsx:121— During a resize drag,livePlantingis updated with the newradiusCmbut the cachedderivedCountis left unchanged. BecausePlopMarkerrenderseffectiveCount(plop)(which falls back toplop.derivedCount), the canvas text shows the stale derived count until the PATCH resolves. The inspector placeholder updates live viacomputeDerivedCount, but the canvas marker does not.Fix: recompute
derivedCountinside the drag move callback (the overlay has access to the plant via the parent if passed down, or the marker itself can derive it fromplant).web/src/editor/GardenCanvas.tsx:139—onPlaceguards withif (!focusedObject || !armedPlant) returnbut never checksfocusedObject.plantable. A deep-link (?focus=<id>) to a non-plantable object still lets the user arm and place a plant; the server will likely reject it, but the client should prevent the attempt.Fix: add
|| !focusedObject.plantableto the guard.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
PlopMarkercarries a deaddimmedprop (web/src/editor/PlopMarker.tsx:27). The only caller (PlopLayer) always passesdimmed={false}and handles object-level dimming via the parent<g>opacity. The prop is redundant boilerplate that should be removed to avoid misleading future callers.Selection constants are triplicated (
web/src/editor/SelectionOverlay.tsx:7and11,PlopOverlay.tsx:9and11,PlopMarker.tsx:12).SELECT_COLORandHANDLE_PXare defined identically in three and two files respectively. Centralizing them in a single module prevents visual drift and makes future tweaks one-touch.Gesture scaffolding is copy-pasted (
web/src/editor/SelectionOverlay.tsx:73andPlopOverlay.tsx:67). The window-listener setup, cleanup ref pattern, and mutation-on-release logic are ~30 lines of nearly identical boilerplate with onlyliveObjectvslivePlantingdiffering. Extracting a shared generic hook (e.g.useDragGesture) would eliminate the duplication and ensure interaction fixes apply once.dominantColoris recomputed inline on every scale change (web/src/editor/PlopLayer.tsx:64). Becausescalechanges during zoom andPlopLayerre-renders with it,dominantColoriterates over all plops for every object even though its true dependencies are only the plop list and plant map. Moving this derivation into a memoized structure (or precomputing per object) keeps data logic out of the render hot path and makes the component easier to reason about.Radius bounds are scattered and partially duplicated (
web/src/editor/PlopInspector.tsx:10andPlopOverlay.tsx:9-10).MIN_RADIUS_CMis defined in two files;MAX_RADIUS_CMonly in one. Co-locating both in a shared constants module (e.g. alongside the planting domain) makes the model easier to keep consistent.GardenCanvasrebuildsplantsByIdthat its parent already computed (web/src/editor/GardenCanvas.tsx:68).GardenEditorPageconstructs the sameMapand passes the rawplantsarray down only so the canvas can rebuild it. PassingplantsByIddirectly removes duplicate code and a redundantuseMemo.⚡ Performance — No material issues found
No material issues found
Wait — actually, let me re-verify. I need to be more careful.
Minor issues
web/src/editor/GardenCanvas.tsx:100-103— During a plop drag,livePlantingupdates on everypointermove. TherenderedPlopsuseMemomaps over the entireplantingsarray (O(n) in total plantings) to create a new array with one element replaced. For a garden with many plantings, this allocates a large array on every animation frame and forcesPlopLayerto re-render. The same pattern exists forliveObject(which the PR explicitly calls "cheap"), but plantings are the "many" side of the relationship and can be an order of magnitude more numerous.Fix: Pass
plantingsandlivePlantingseparately toPlopLayer; merge inline in the plop render loop (O(1) extra per rendered plop) rather than doing a full-array copy inGardenCanvas.web/src/editor/PlopLayer.tsx:48-53—byObjectis rebuilt from scratch on everyPlopLayerrender, even when onlyscale(zooming) orobjects(object drag) changed andplantingsis unchanged. During a continuous zoom or object drag, this is an unnecessary O(total_plantings) rebuild on every frame.Fix: Wrap the
byObjectconstruction inuseMemo(() => { ... }, [plantings]).🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/editor/PlopInspector.tsx:142— Cannot clear the "Planted" date once set. TheonBlurhandler guards withplanted !== '', so when the user clears the field the patch is silently skipped. Worse, the re-sync effect at line 44 snaps the cleared value back to the old date once focus leaves the inspector (because the field is no longer active). The handler should sendplantedAt: planted || nulland remove the empty-string guard.web/src/pages/GardenEditorPage.tsx:64/web/src/editor/GardenCanvas.tsx:178— Stale?focusIDs leave the canvas permanently dimmed with no visible exit affordance.setFocusedObject(focus ?? null)is adopted from the URL without validating that the object exists after the garden loads.GardenCanvasandPlopLayerdim every object whenfocusedObjectId != null, butGardenEditorPagehides the breadcrumb (← garden) when the object is missing. On mobile there is no Escape key, so the user must discover that an empty-space tap exits. An effect should clearfocusedObjectIdwhenobjects.length > 0 && !objects.find(o => o.id === focusedObjectId).web/src/lib/objects.ts:299—applyPlantingPatchsilently ignoresremovedAtdespite it being inPlantingPatch. The optimistic-update function copies every other patch field but omitsremovedAt. Today this is latent because soft-removes useuseRemovePlanting, but if anyone later callsuseUpdatePlantingwithremovedAt, the plop will stay visible during the request and only disappear ononSuccess, producing a jarring flash.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.