Closes#11. Completes Phase 3 — the landscaping-editor half of pansy works end-to-end. Ties together #8/#9/#10.
What's here
lib/objects.ts — react-query over GET /gardens/:id/full + optimistic create/update/delete. An update applies to the cache immediately and PATCHes with the row version; a 409 adopts the server's current row and toasts "updated elsewhere". One PATCH per gesture.
editor/store.ts — adds liveObject (in-gesture geometry, rendered instantly), armedKind (palette tap-to-place), and objectDragging (so the viewport's pan gesture stands down while an object drag owns the pointer — both listen on the same svg).
editor/Palette.tsx + kinds.ts — the seven kinds with icons and the brief's default sizes; tap a kind → tap the canvas to place (desktop + touch).
editor/SelectionOverlay.tsx — move (drag body), resize (corner handles, opposite corner stays put — computed in the object-local frame so rotation is honored), rotate (knob, snaps to 15°, free with Shift). Each updates liveObject for instant feedback and fires exactly one PATCH on release.
editor/Inspector.tsx — name, dimensions + position (unit-aware), rotation, color override (+ clear, using #10's nullable PATCH), plantable, notes, two-step delete. Commits per field on blur/change; re-syncs from the object unless a field is focused (so canvas drags update it without clobbering typing).
GardenCanvas — placement, live-drag merge, z-ordered render (paths/structures under beds; explicit z_index respected), the selection overlay, and ?focus=<id> frames an object on load (groundwork for #15). GardenEditorPage loads /full and lays out palette · canvas · inspector (bottom sheet on mobile). New: components/ui/toast.tsx + Toaster.
Verification (real browser, embedded binary)
Place a bed (armed palette → tap) → move, resize, rotate → set its name in the inspector → reload: all persisted. Each gesture was exactly one PATCH (version 1→2→3→4→5); rotate snapped to 60°.
Two-client conflict: an out-of-band edit bumped the version; the next UI drag → 409 → rollback to the server's value + toast, and the final state was the other client's, not the loser's drag.
tsc --noEmit clean; npx vitest run 17 green; vite build succeeds. Screenshot shared in-session.
Touch: interactions are pointer-event based (work on touch); the inspector is a bottom sheet at small widths, and pan/zoom/pinch coexist with a selected object. Phone hardware pass is worth a final check on-device.
Closes #11. **Completes Phase 3** — the landscaping-editor half of pansy works end-to-end. Ties together #8/#9/#10.
## What's here
- **`lib/objects.ts`** — react-query over `GET /gardens/:id/full` + optimistic create/update/delete. An update applies to the cache immediately and PATCHes with the row `version`; a **409 adopts the server's `current` row and toasts** "updated elsewhere". **One PATCH per gesture.**
- **`editor/store.ts`** — adds `liveObject` (in-gesture geometry, rendered instantly), `armedKind` (palette tap-to-place), and `objectDragging` (so the viewport's pan gesture stands down while an object drag owns the pointer — both listen on the same svg).
- **`editor/Palette.tsx` + `kinds.ts`** — the seven kinds with icons and the brief's default sizes; tap a kind → tap the canvas to place (desktop + touch).
- **`editor/SelectionOverlay.tsx`** — **move** (drag body), **resize** (corner handles, opposite corner stays put — computed in the object-local frame so rotation is honored), **rotate** (knob, snaps to 15°, free with Shift). Each updates `liveObject` for instant feedback and fires exactly one PATCH on release.
- **`editor/Inspector.tsx`** — name, dimensions + position (unit-aware), rotation, color override (+ clear, using #10's nullable PATCH), plantable, notes, two-step delete. Commits per field on blur/change; re-syncs from the object unless a field is focused (so canvas drags update it without clobbering typing).
- **`GardenCanvas`** — placement, live-drag merge, z-ordered render (paths/structures under beds; explicit `z_index` respected), the selection overlay, and **`?focus=<id>`** frames an object on load (groundwork for #15). **`GardenEditorPage`** loads `/full` and lays out palette · canvas · inspector (bottom sheet on mobile). New: `components/ui/toast.tsx` + `Toaster`.
## Verification (real browser, embedded binary)
- Place a bed (armed palette → tap) → **move, resize, rotate** → set its **name** in the inspector → **reload: all persisted**. Each gesture was exactly **one PATCH** (version 1→2→3→4→5); rotate **snapped to 60°**.
- **Two-client conflict**: an out-of-band edit bumped the version; the next UI drag → **409 → rollback to the server's value + toast**, and the final state was the other client's, not the loser's drag.
- `tsc --noEmit` clean; `npx vitest run` 17 green; `vite build` succeeds. Screenshot shared in-session.
- Touch: interactions are pointer-event based (work on touch); the inspector is a bottom sheet at small widths, and pan/zoom/pinch coexist with a selected object. Phone hardware pass is worth a final check on-device.
## Out of scope
- Plops/focus interactions (#15). Read-only viewer mode (#17). Undo/redo, multi-select (post-v1).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Turns the canvas foundation into the real editor, loading a garden from the
server and editing it with optimistic sync. Completes Phase 3.
- lib/objects.ts: react-query over GET /gardens/:id/full + optimistic
create/update/delete. Update applies to the cache up front and PATCHes
with the row version; a 409 adopts the server's `current` row and toasts
"updated elsewhere". One PATCH per gesture.
- editor/store.ts: adds liveObject (in-gesture geometry, rendered instantly),
armedKind (palette tap-to-place), and objectDragging (so the pan gesture
stands down while an object drag owns the pointer).
- editor/Palette.tsx + kinds.ts: the seven kinds with icons + default sizes;
tap a kind, tap the canvas to place (works desktop + touch).
- editor/SelectionOverlay.tsx: move (drag body), resize (corner handles,
opposite corner fixed via the object-local frame so rotation is honored),
rotate (knob, snaps to 15°, free with Shift). Each updates liveObject and
fires one PATCH on release.
- editor/Inspector.tsx: name, dimensions/position (unit-aware), rotation,
color override (+ clear), plantable, notes, two-step delete. Commits per
field; re-syncs from the object unless a field is focused.
- GardenCanvas: placement, live-drag merge, z-ordered render, selection
overlay, and ?focus=<id> frames an object on load. GardenEditorPage loads
/full and lays out palette | canvas | inspector (bottom sheet on mobile).
- components/ui/toast.tsx + Toaster in AppShell.
Verified in a real browser against the embedded binary: place a bed → move,
resize, rotate (rotate snapped to 60°) → set its name in the inspector →
reload, all persisted; each gesture was exactly one PATCH (version 1→2→3→4→5);
an out-of-band edit made the next drag 409 → rollback to the server's value +
toast. tsc + 17 vitest tests 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 00:49:44Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
#### `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** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **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>
🟡ToastItem prop toast shadows the same-file exported toast helper
maintainability · flagged by 2 models
web/src/components/ui/toast.tsx:32 — ToastItem prop toast shadows the imported/exported toast helper. The component parameter { toast } (line 32) shadows the module-level export const toast = {...} (lines 27-30) within the component body. It works (the component only reads toast.id/toast.tone/toast.message), but shadowing a same-file exported symbol is a readability trap — rename the prop to data or t.
🪰 Gadfly · advisory
🟡 **ToastItem prop `toast` shadows the same-file exported `toast` helper**
_maintainability · flagged by 2 models_
- **`web/src/components/ui/toast.tsx:32` — `ToastItem` prop `toast` shadows the imported/exported `toast` helper.** The component parameter `{ toast }` (line 32) shadows the module-level `export const toast = {...}` (lines 27-30) within the component body. It works (the component only reads `toast.id`/`toast.tone`/`toast.message`), but shadowing a same-file exported symbol is a readability trap — rename the prop to `data` or `t`.
<sub>🪰 Gadfly · advisory</sub>
🟠defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts
maintainability · flagged by 3 models
web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. - web/src/lib/objects.ts:188-202 — applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function,…
🪰 Gadfly · advisory
🟠 **defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts**
_maintainability · flagged by 3 models_
- **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place. - **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function,…
<sub>🪰 Gadfly · advisory</sub>
🔴fittedForRef guard prevents re-framing when focusId changes for same garden
correctness · flagged by 2 models
web/src/editor/GardenCanvas.tsx:74-82 — The fittedForRef.current !== garden.id guard means the canvas frames the garden (or a ?focus= object) exactly once per garden mount. If the focusId search param changes client-side without remounting the page, the viewport does not re-fit to the new object because the guard only tracks garden.id, not the focus target. Track the last focused id and re-frame when it changes.
🪰 Gadfly · advisory
🔴 **fittedForRef guard prevents re-framing when focusId changes for same garden**
_correctness · flagged by 2 models_
- `web/src/editor/GardenCanvas.tsx:74-82` — The `fittedForRef.current !== garden.id` guard means the canvas frames the garden (or a `?focus=` object) exactly once per garden mount. If the `focusId` search param changes client-side without remounting the page, the viewport does **not** re-fit to the new object because the guard only tracks `garden.id`, not the focus target. Track the last focused id and re-frame when it changes.
<sub>🪰 Gadfly · advisory</sub>
🟠Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object
maintainability, performance · flagged by 5 models
web/src/editor/GardenCanvas.tsx:82-85 — rendered re-maps and re-sorts the full objects array on every liveObject change (every pointermove during a drag, per SelectionOverlay's setLiveObject calls). Confirmed the useMemo dependency array is [objects, liveObject] (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed GardenEditorPage.tsx:51 (`full.data.objects.map(toEditorO…
🪰 Gadfly · advisory
🟠 **Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object**
_maintainability, performance · flagged by 5 models_
- **`web/src/editor/GardenCanvas.tsx:82-85`** — `rendered` re-maps and re-sorts the full `objects` array on every `liveObject` change (every pointermove during a drag, per `SelectionOverlay`'s `setLiveObject` calls). Confirmed the `useMemo` dependency array is `[objects, liveObject]` (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed `GardenEditorPage.tsx:51` (`full.data.objects.map(toEditorO…
<sub>🪰 Gadfly · advisory</sub>
🟡Grid path collapsed from multi-line to single long line, hurting readability
maintainability · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. - web/src/lib/objects.ts:188-202 — applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function,…
🪰 Gadfly · advisory
🟡 **Grid path collapsed from multi-line to single long line, hurting readability**
_maintainability · flagged by 1 model_
- **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place. - **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function,…
<sub>🪰 Gadfly · advisory</sub>
🟡Hand-rolled 'Fit' button duplicates the shared Button/buttonClasses ghost styling instead of reusing it
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:149-155 — The "Fit" button was changed from <Button variant="ghost"> to a raw <button> that hand-duplicates the Button/buttonClasses ghost styling (border border-border, text-fg, hover:bg-border/50, outline-none focus-visible:ring-2 focus-visible:ring-accent/40, transition-colors — confirmed identical to web/src/components/ui/Button.tsx:9-16). The Button import was dropped from this file entirely. The prior code already used className t…
🪰 Gadfly · advisory
🟡 **Hand-rolled 'Fit' button duplicates the shared Button/buttonClasses ghost styling instead of reusing it**
_maintainability · flagged by 1 model_
- `web/src/editor/GardenCanvas.tsx:149-155` — The "Fit" button was changed from `<Button variant="ghost">` to a raw `<button>` that hand-duplicates the `Button`/`buttonClasses` ghost styling (`border border-border`, `text-fg`, `hover:bg-border/50`, `outline-none focus-visible:ring-2 focus-visible:ring-accent/40`, `transition-colors` — confirmed identical to `web/src/components/ui/Button.tsx:9-16`). The `Button` import was dropped from this file entirely. The prior code already used `className` t…
<sub>🪰 Gadfly · advisory</sub>
🟡web/src/editor/Inspector.tsx:31-37 and :44-53 — the field-init and re-sync logic duplicate the same String(displayFromCm(...)) / String(Math.round(...)) conversions.
maintainability · flagged by 1 model
web/src/editor/Inspector.tsx:31-37 and :44-53 — the field-init and re-sync logic duplicate the same String(displayFromCm(...)) / String(Math.round(...)) conversions. Seven useState initializers and the useEffect both spell out the same per-field formatting. A small fieldsFromObject(object, unit) helper returning { name, notes, width, height, x, y, rotation } would remove the duplication and make a future field a one-line add in both places. Trivial.
🪰 Gadfly · advisory
🟡 **`web/src/editor/Inspector.tsx:31-37` and `:44-53` — the field-init and re-sync logic duplicate the same `String(displayFromCm(...))` / `String(Math.round(...))` conversions.**
_maintainability · flagged by 1 model_
- **`web/src/editor/Inspector.tsx:31-37` and `:44-53` — the field-init and re-sync logic duplicate the same `String(displayFromCm(...))` / `String(Math.round(...))` conversions.** Seven `useState` initializers and the `useEffect` both spell out the same per-field formatting. A small `fieldsFromObject(object, unit)` helper returning `{ name, notes, width, height, x, y, rotation }` would remove the duplication and make a future field a one-line add in both places. Trivial.
<sub>🪰 Gadfly · advisory</sub>
🟠Re-sync skips all inspector fields when any field has focus
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:44-53 — The re-sync useEffect aborts entirely when any field inside the inspector has focus (rootRef.current?.contains(document.activeElement)). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.
🪰 Gadfly · advisory
🟠 **Re-sync skips all inspector fields when any field has focus**
_error-handling · flagged by 1 model_
- **`web/src/editor/Inspector.tsx:44-53`** — The re-sync `useEffect` aborts entirely when *any* field inside the inspector has focus (`rootRef.current?.contains(document.activeElement)`). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.
<sub>🪰 Gadfly · advisory</sub>
🔴Inspector commitDim accepts zero and negative dimensions
error-handling, maintainability · flagged by 4 models
web/src/editor/Inspector.tsx:58-63 — commitDim parses the raw input with parseFloat and sends the result without validating that it is positive or at least MIN_OBJ_CM. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (MIN_OBJ_CM = 1). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: reject v <= 0 (or cm <= 0) in commitDim and surface a brief inline war…
🪰 Gadfly · advisory
🔴 **Inspector commitDim accepts zero and negative dimensions**
_error-handling, maintainability · flagged by 4 models_
- **`web/src/editor/Inspector.tsx:58-63`** — `commitDim` parses the raw input with `parseFloat` and sends the result without validating that it is positive or at least `MIN_OBJ_CM`. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (`MIN_OBJ_CM = 1`). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: reject `v <= 0` (or `cm <= 0`) in `commitDim` and surface a brief inline war…
<sub>🪰 Gadfly · advisory</sub>
🟠Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur
correctness · flagged by 2 models
web/src/editor/Inspector.tsx:62 — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.commitDim runs on every blur and compares cmFromDisplay(v, unit) to current with no dirty guard. Because displayFromCm rounds (imperial → 0.1 ft, metric → 2 dp cm) and cmFromDisplay re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → displayFromCm gives 3.3 ft → cmFromDisplay(3.3,'imperial') = round(3.3*12*2.54) = 101 cm, so…
🪰 Gadfly · advisory
🟠 **Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur**
_correctness · flagged by 2 models_
- **`web/src/editor/Inspector.tsx:62` — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.** `commitDim` runs on every blur and compares `cmFromDisplay(v, unit)` to `current` with no dirty guard. Because `displayFromCm` rounds (imperial → 0.1 ft, metric → 2 dp cm) and `cmFromDisplay` re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → `displayFromCm` gives `3.3` ft → `cmFromDisplay(3.3,'imperial')` = `round(3.3*12*2.54)` = **101 cm**, so…
<sub>🪰 Gadfly · advisory</sub>
🟠Four dimension TextFields are copy-pasted with only label/value differing
maintainability · flagged by 2 models
web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. - web/src/lib/objects.ts:188-202 — applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function,…
🪰 Gadfly · advisory
🟠 **Four dimension TextFields are copy-pasted with only label/value differing**
_maintainability · flagged by 2 models_
- **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place. - **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function,…
<sub>🪰 Gadfly · advisory</sub>
🔴Color onChange fires a PATCH per picker drag step → version storms and spurious 409 rollbacks with a misleading 'updated elsewhere' toast
correctness, error-handling, performance · flagged by 3 models
web/src/editor/Inspector.tsx:155 — color onChange fires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks. The native <input type="color"> emits input events continuously while the user drags through the OS color picker; React's onChange maps to that input event. Each tick calls patch({ color }), sending the currentobject.version. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new versi…
🪰 Gadfly · advisory
🔴 **Color onChange fires a PATCH per picker drag step → version storms and spurious 409 rollbacks with a misleading 'updated elsewhere' toast**
_correctness, error-handling, performance · flagged by 3 models_
- **`web/src/editor/Inspector.tsx:155` — color `onChange` fires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks.** The native `<input type="color">` emits `input` events continuously while the user drags through the OS color picker; React's `onChange` maps to that `input` event. Each tick calls `patch({ color })`, sending the *current* `object.version`. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new versi…
<sub>🪰 Gadfly · advisory</sub>
🟡On delete failure the object is restored to the cache but selection was already cleared, leaving it unselected on retry
error-handling · flagged by 1 model
**web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.** select(null)runs beforedel.mutate(object.id). If the DELETE fails (network/403/500), onError` (objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the se…
🪰 Gadfly · advisory
🟡 **On delete failure the object is restored to the cache but selection was already cleared, leaving it unselected on retry**
_error-handling · flagged by 1 model_
- **`web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.** `select(null)` runs before `del.mutate(object.id)`. If the DELETE fails (network/403/500), `onError` (objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the se…
<sub>🪰 Gadfly · advisory</sub>
🔴Window pointer listeners leak when SelectionOverlay unmounts mid-drag
error-handling · flagged by 4 models
web/src/editor/SelectionOverlay.tsx:21-31 — trackPointer attaches window listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on window. On pointerup the onEnd closure fires, calling setObjectDragging(false) and update.mutate(fields(final)) ag…
🪰 Gadfly · advisory
🔴 **Window pointer listeners leak when SelectionOverlay unmounts mid-drag**
_error-handling · flagged by 4 models_
- **`web/src/editor/SelectionOverlay.tsx:21-31`** — `trackPointer` attaches `window` listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on `window`. On `pointerup` the `onEnd` closure fires, calling `setObjectDragging(false)` and `update.mutate(fields(final))` ag…
<sub>🪰 Gadfly · advisory</sub>
🟠getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)
maintainability, performance · flagged by 4 models
web/src/editor/SelectionOverlay.tsx:60-65 — getBoundingClientRect() called on every pointermove during a drag.pointerWorld reads svgRef.current?.getBoundingClientRect() and is invoked from the trackPointer move callback (line 78) on every pointer event. getBoundingClientRect forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live setLiveObject re-render is also mutating the SVG, so the layout is dirt…
🪰 Gadfly · advisory
🟠 **getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)**
_maintainability, performance · flagged by 4 models_
- **`web/src/editor/SelectionOverlay.tsx:60-65` — `getBoundingClientRect()` called on every `pointermove` during a drag.** `pointerWorld` reads `svgRef.current?.getBoundingClientRect()` and is invoked from the `trackPointer` move callback (line 78) on every pointer event. `getBoundingClientRect` forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live `setLiveObject` re-render is also mutating the SVG, so the layout is dirt…
<sub>🪰 Gadfly · advisory</sub>
🔴Stale version captured in gesture closure causes spurious 409s
correctness · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:88 — base = { ...object } snapshots the object's version at render time. The gesture-end PATCH sends base.version, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict during the drag. The fix is to read the current cached row's version when building the patch on release, or to derive it from t…
🪰 Gadfly · advisory
🔴 **Stale version captured in gesture closure causes spurious 409s**
_correctness · flagged by 1 model_
- `web/src/editor/SelectionOverlay.tsx:88` — `base = { ...object }` snapshots the object's `version` at render time. The gesture-end PATCH sends `base.version`, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict *during* the drag. The fix is to read the current cached row's `version` when building the patch on release, or to derive it from t…
<sub>🪰 Gadfly · advisory</sub>
🟡Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.
error-handling · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:108 — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.newW/newH are clamped to MIN_OBJ_CM, but newCenterLocal is still derived from the live pointer position p, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.
🪰 Gadfly · advisory
🟡 **Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.**
_error-handling · flagged by 1 model_
- **`web/src/editor/SelectionOverlay.tsx:108` — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.** `newW`/`newH` are clamped to `MIN_OBJ_CM`, but `newCenterLocal` is still derived from the live pointer position `p`, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.
<sub>🪰 Gadfly · advisory</sub>
web/src/editor/SelectionOverlay.tsx:122-134 — startRotate captures e.shiftKey from the initial ReactPointerEvent closure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: read shiftKey from the live PointerEvent passed to the move handler instead of the stale closed-over React event.
🪰 Gadfly · advisory
🟡 **Rotation shiftKey state locked at gesture start**
_error-handling · flagged by 1 model_
- **`web/src/editor/SelectionOverlay.tsx:122-134`** — `startRotate` captures `e.shiftKey` from the initial `ReactPointerEvent` closure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: read `shiftKey` from the live `PointerEvent` passed to the move handler instead of the stale closed-over React event.
<sub>🪰 Gadfly · advisory</sub>
🟠Shift key state locked at gesture start instead of evaluated live during rotate
correctness · flagged by 3 models
web/src/editor/SelectionOverlay.tsx:128 — Shift state captured at gesture start, not tracked during the drag. The onMove closure in startRotate references e.shiftKey from the original pointerdown event (if (!e.shiftKey) deg = Math.round(...)). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (ev.shiftKey) inside onMove instead of from e.
🪰 Gadfly · advisory
🟠 **Shift key state locked at gesture start instead of evaluated live during rotate**
_correctness · flagged by 3 models_
- **`web/src/editor/SelectionOverlay.tsx:128` — Shift state captured at gesture start, not tracked during the drag.** The `onMove` closure in `startRotate` references `e.shiftKey` from the original `pointerdown` event (`if (!e.shiftKey) deg = Math.round(...)`). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (`ev.shiftKey`) inside `onMove` instead of from `e`.
<sub>🪰 Gadfly · advisory</sub>
web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. - web/src/lib/objects.ts:188-202 — applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function,…
🪰 Gadfly · advisory
🟠 **Duplicate circle-vs-rect shape rendering inside SelectionOverlay**
_maintainability · flagged by 1 model_
- **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place. - **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function,…
<sub>🪰 Gadfly · advisory</sub>
🔴Optimistic PATCH never bumps cached version, causing false 409 conflicts and silent loss of the user's own concurrent edits
correctness · flagged by 2 models
web/src/lib/objects.ts:130-137 + :188-201 (onMutate / applyServerPatch) — optimistic update never advances the cached version, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit.useUpdateObject's onMutate applies the patched fields to the cache via applyServerPatch, which copies over every field except version (objects.ts:188-201). The cached object therefore still carries the pre-mutation version afte…
🪰 Gadfly · advisory
🔴 **Optimistic PATCH never bumps cached version, causing false 409 conflicts and silent loss of the user's own concurrent edits**
_correctness · flagged by 2 models_
- **`web/src/lib/objects.ts:130-137` + `:188-201` (`onMutate` / `applyServerPatch`) — optimistic update never advances the cached `version`, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit.** `useUpdateObject`'s `onMutate` applies the patched fields to the cache via `applyServerPatch`, which copies over every field except `version` (objects.ts:188-201). The cached object therefore still carries the pre-mutation `version` afte…
<sub>🪰 Gadfly · advisory</sub>
🟠Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess
performance · flagged by 1 model
web/src/lib/objects.ts:159 (and 177) — useUpdateObject and useDeleteObject both add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) even though onSuccess/onMutate already write the authoritative result into the cache (patchFullCache at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of GET /gardens/:id/full after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires o…
🪰 Gadfly · advisory
🟠 **Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess**
_performance · flagged by 1 model_
- **`web/src/lib/objects.ts:159` (and `177`)** — `useUpdateObject` and `useDeleteObject` both add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` even though `onSuccess`/`onMutate` already write the authoritative result into the cache (`patchFullCache` at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of `GET /gardens/:id/full` after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires o…
<sub>🪰 Gadfly · advisory</sub>
🔴Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits
error-handling, maintainability · flagged by 4 models
web/src/lib/objects.ts:188-201 (applyServerPatch) — Ten near-identical ...(patch.X !== undefined ? { X: patch.X } : {}) lines, one per ObjectPatch field. Every time a field is added to ObjectPatch (objects.ts:108-121), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.
🪰 Gadfly · advisory
🔴 **Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits**
_error-handling, maintainability · flagged by 4 models_
- `web/src/lib/objects.ts:188-201` (`applyServerPatch`) — Ten near-identical `...(patch.X !== undefined ? { X: patch.X } : {})` lines, one per `ObjectPatch` field. Every time a field is added to `ObjectPatch` (`objects.ts:108-121`), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.
<sub>🪰 Gadfly · advisory</sub>
🟠Unmemoized toEditorObject mapping breaks downstream memoization and causes unnecessary allocations
performance · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:51 — const objects = full.data.objects.map(toEditorObject) is unmemoized. toEditorObject creates a new EditorObject instance for every server object on every parent render. This defeats the React.memo on ObjectShape, forcing all objects to re-render whenever GardenEditorPage re-renders (e.g., on selectedId change or background refetch). Fix: wrap the mapping in useMemo(() => full.data.objects.map(toEditorObject), [full.data.objects])…
🪰 Gadfly · advisory
🟠 **Unmemoized toEditorObject mapping breaks downstream memoization and causes unnecessary allocations**
_performance · flagged by 3 models_
- **`web/src/pages/GardenEditorPage.tsx:51`** — `const objects = full.data.objects.map(toEditorObject)` is unmemoized. `toEditorObject` creates a new `EditorObject` instance for every server object on every parent render. This defeats the `React.memo` on `ObjectShape`, forcing all objects to re-render whenever `GardenEditorPage` re-renders (e.g., on `selectedId` change or background refetch). **Fix:** wrap the mapping in `useMemo(() => full.data.objects.map(toEditorObject), [full.data.objects])`…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 31 findings (14 with multi-model agreement)
Finding
Where
Models
Lens
🟠
Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object
web/src/editor/GardenCanvas.tsx:82
5/5
maintainability, performance
🔴
Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits
web/src/lib/objects.ts:188
4/5
error-handling, maintainability
🔴
Inspector commitDim accepts zero and negative dimensions
web/src/editor/Inspector.tsx:55
4/5
error-handling, maintainability
🔴
Window pointer listeners leak when SelectionOverlay unmounts mid-drag
web/src/editor/SelectionOverlay.tsx:19
4/5
error-handling
🟠
getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)
web/src/editor/SelectionOverlay.tsx:57
4/5
maintainability, performance
🔴
Color onChange fires a PATCH per picker drag step → version storms and spurious 409 rollbacks with a misleading 'updated elsewhere' toast
web/src/editor/Inspector.tsx:151
3/5
correctness, error-handling, performance
🟠
defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts
web/src/editor/GardenCanvas.tsx:15
3/5
maintainability
🟠
Shift key state locked at gesture start instead of evaluated live during rotate
web/src/editor/SelectionOverlay.tsx:128
3/5
correctness
🟠
Unmemoized toEditorObject mapping breaks downstream memoization and causes unnecessary allocations
web/src/pages/GardenEditorPage.tsx:51
3/5
performance
🔴
fittedForRef guard prevents re-framing when focusId changes for same garden
web/src/editor/GardenCanvas.tsx:71
2/5
correctness
🔴
Optimistic PATCH never bumps cached version, causing false 409 conflicts and silent loss of the user's own concurrent edits
web/src/lib/objects.ts:130
2/5
correctness
🟠
Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur
web/src/editor/Inspector.tsx:62
2/5
correctness
🟠
Four dimension TextFields are copy-pasted with only label/value differing
web/src/editor/Inspector.tsx:89
2/5
maintainability
🟡
ToastItem prop 'toast' shadows the same-file exported 'toast' helper
web/src/components/ui/toast.tsx:32
2/5
maintainability
17 single-model findings (lower confidence)
Finding
Where
Model
Lens
🔴
Stale version captured in gesture closure causes spurious 409s
web/src/editor/SelectionOverlay.tsx:88
kimi-k2.6:cloud
correctness
🔴
Wheel/pinch not blocked during object drag, causing coordinate jump
web/src/editor/useViewport.ts:58
opencode/kimi-k2.6:cloud
error-handling
🟠
Focus rect ignores rotation, clipping rotated objects
web/src/editor/GardenCanvas.tsx:78
kimi-k2.6:cloud
correctness
🟠
Re-sync skips all inspector fields when any field has focus
web/src/editor/Inspector.tsx:44
kimi-k2.6:cloud
error-handling
🟠
Checkbox commits PATCH immediately onChange, rapid toggles self-conflict with version
Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess
web/src/lib/objects.ts:159
claude-code/sonnet
performance
🟡
'web/src/editor/GardenCanvas.tsx:16-21' — per-kind default-z knowledge is split from 'kinds.ts'.
kinds.ts:5
opencode/glm-5.2:cloud
maintainability
🟡
'web/src/editor/Inspector.tsx:55-56' — 'patch' is typed against 'EditorObject', not the real 'ObjectPatch', so it accepts fields the server ignores.
objects.ts:108
opencode/glm-5.2:cloud
maintainability
🟡
'web/src/editor/Inspector.tsx:55-56' — 'patch' is typed against 'EditorObject', not the real 'ObjectPatch', so it accepts fields the server ignores.
types.ts:8
opencode/glm-5.2:cloud
maintainability
🟡
Grid path collapsed from multi-line to single long line, hurting readability
web/src/editor/GardenCanvas.tsx:129
opencode/kimi-k2.6:cloud
maintainability
🟡
Hand-rolled 'Fit' button duplicates the shared Button/buttonClasses ghost styling instead of reusing it
web/src/editor/GardenCanvas.tsx:149
claude-code/sonnet
maintainability
🟡
'web/src/editor/Inspector.tsx:31-37' and ':44-53' — the field-init and re-sync logic duplicate the same 'String(displayFromCm(...))' / 'String(Math.round(...))' conversions.
web/src/editor/Inspector.tsx:31
opencode/glm-5.2:cloud
maintainability
🟡
On delete failure the object is restored to the cache but selection was already cleared, leaving it unselected on retry
web/src/editor/Inspector.tsx:191
opencode/glm-5.2:cloud
error-handling
🟡
Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.
web/src/editor/SelectionOverlay.tsx:108
glm-5.2:cloud
error-handling
🟡
Rotation shiftKey state locked at gesture start
web/src/editor/SelectionOverlay.tsx:122
kimi-k2.6:cloud
error-handling
🟡
All resize handles use the same nwse-resize cursor regardless of corner
web/src/editor/SelectionOverlay.tsx:168
opencode/kimi-k2.6:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Blocking issues found
Good — no dangerouslySetInnerHTML/innerHTML/eval anywhere, and the backend independently validates dimensions (validDimensionCM) and color format server-side, so a malicious/buggy frontend value can't corrupt stored data. That closes out my two candidate concerns (SVG fill:url() injection via unvalidated color, and unbounded width/height from the Inspector's raw number fields) — both are enforced server-side.
I've completed my review. No security issues survived verification.
VERDICT: No material issues found
This PR is entirely frontend (React/TypeScript) editor UI wired to an existing, already-validated API. I checked the areas most likely to matter for this lens:
Untrusted data rendering: object.name/notes and toast messages are rendered as plain JSX text content (no dangerouslySetInnerHTML/innerHTML anywhere in web/src — verified via grep). React auto-escapes, so no XSS path.
Color field → SVG fill: ObjectShape.tsx renders object.color directly as an SVG fill attribute unvalidated on the client, which raised a concern about CSS/SVG url()-based injection. Verified this is a non-issue: internal/service/objects.go (validDimensionCM) and the color-format check exercised by internal/service/objects_test.go ("bad color" test case) reject non-hex colors server-side, so arbitrary paint-server values can't reach storage even via a raw API call bypassing the <input type="color"> UI.
Server response handling: all API responses go through zod schemas (serverObjectSchema, fullGardenSchema) before use — malformed/unexpected server data fails parsing rather than being trusted blindly. conflictObject() correctly uses safeParse on the 409 body.
Auth/session: unchanged — requests still use credentials: 'same-origin' (cookie-based), no tokens handled or logged client-side.
URL/search param handling: router.tsx's new validateSearch for ?focus= coerces to a positive integer or drops it entirely ({}), so it can't be used to inject anything into the DOM or downstream calls; it's only used to look up a local object by numeric id.
Optimistic mutations: id/version in PATCH bodies come from already-fetched, schema-validated objects, not raw user text — no injection surface there.
One thing outside my lens worth flagging for the correctness reviewer: Inspector.tsx's commitDim has no lower-bound clamp (unlike SelectionOverlay's MIN_OBJ_CM), so a user could type a zero/negative dimension — not a security issue since the server enforces validDimensionCM, but worth a UX-level fix.
🎯 Correctness — Blocking issues found
Both findings confirmed exactly as described, at the correct line numbers. Line numbers are 71-73 rather than 71-72 exactly, but close enough and the substance is accurate.
VERDICT: Blocking issues found
web/src/lib/objects.ts:130-137 + :188-201 (onMutate / applyServerPatch) — optimistic update never advances the cached version, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit. useUpdateObject's onMutate applies the patched fields to the cache via applyServerPatch, which copies over every field except version (objects.ts:188-201). The cached object therefore still carries the pre-mutation version after the optimistic write. Inspector.tsx builds each PATCH from object.version read off that same cache/prop, and each field (Name, Width, Height, X, Y, Rotation, Notes) has its own independent onBlur handler calling update.mutate(...) — nothing serializes these. Trace: user edits Name (blur → PATCH A, version: V), then edits Width before A returns (blur → PATCH B, also version: V, since the cache's version hasn't moved). Server processes A first, bumps to V+1 (confirmed server-side version-conflict path at internal/api/objects.go:144-149, dispatching to writeVersionConflict on domain.ErrVersionConflict). B arrives with stale version: V and is rejected as a conflict. The client's onError path (objects.ts:145-158) then overwrites the cache with the server's current row (reflecting A's change but not B's) and shows "That object was updated elsewhere — your change was rolled back." The Width edit is silently discarded, and the user is told someone else changed the object, which isn't what happened. SelectionOverlay.tsx has the identical issue: startMove/startResize/startRotate all capture base.version = object.version at gesture start (line 88), so two gestures on the same object performed back-to-back before the first gesture's PATCH resolves (e.g., move immediately followed by resize) race the same way.
Fix direction: bump the cached version optimistically in applyServerPatch (e.g. version: o.version + 1), or serialize/queue mutations per object, or debounce/coalesce rapid field commits into a single PATCH.
web/src/editor/GardenCanvas.tsx:70-73 — ?focus=<id> fit-to-rect ignores the object's rotationDeg, so a rotated object can be framed with its corners clipped outside the viewport.
The target rect is built straight from focus.widthCm/focus.heightCm as an axis-aligned box, but per DESIGN.md:30 objects are positioned by center + rotation_deg clockwise about that center (translate(x y) rotate(deg)), so a rotated rect's true screen-space extent is larger than its unrotated width/height (up to ~1.41× at 45°). zoomToFitRect (lib/geometry.ts:108-128) only adds a fixed padding margin (FIT_PADDING = 32, useViewport.ts:14) that doesn't scale with rotation, so a meaningfully-rotated focused object can end up with corners outside the fitted viewport on load. Low impact today since this is explicitly "groundwork for #15", but it is a real geometric miscalculation given the stated rotation convention.
Fix direction: compute the rotated AABB (rotate all four corners, take min/max) before calling fitToRect.
🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source.
VERDICT: Minor issues
web/src/editor/GardenCanvas.tsx:149-155 — The "Fit" button was changed from <Button variant="ghost"> to a raw <button> that hand-duplicates the Button/buttonClasses ghost styling (border border-border, text-fg, hover:bg-border/50, outline-none focus-visible:ring-2 focus-visible:ring-accent/40, transition-colors — confirmed identical to web/src/components/ui/Button.tsx:9-16). The Button import was dropped from this file entirely. The prior code already used className to add the position/backdrop overrides (absolute bottom-3 right-3 bg-surface/90 py-1.5 shadow-sm backdrop-blur), so the same result was achievable by keeping <Button variant="ghost" className="..."> — there's no functional reason for the divergence, and Inspector.tsx continues to use Button variant="ghost" for equivalent ghost buttons (Inspector.tsx:160,198,203) in this same PR.
web/src/editor/Inspector.tsx:89-130 — The four dimension fields (Width/Height/X/Y) are copy-pasted TextField blocks that differ only by label, local state variable, and which EditorObject key they patch via commitDim. This could collapse to a small loop over a [{key, label, get, set}] table with one local-state map, removing the risk of the four blocks drifting (e.g. a step/inputMode divergence slipping in during a future edit).
web/src/lib/objects.ts:188-201 (applyServerPatch) — Ten near-identical ...(patch.X !== undefined ? { X: patch.X } : {}) lines, one per ObjectPatch field. Every time a field is added to ObjectPatch (objects.ts:108-121), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.
⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/lib/objects.ts:159 (and 177) — useUpdateObject and useDeleteObject both add onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }) even though onSuccess/onMutate already write the authoritative result into the cache (patchFullCache at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of GET /gardens/:id/full after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires one PATCH per field per blur (name, width, height, x, y, rotation, color, plantable, notes) and drags fire one PATCH per gesture, this doubles network work for every one of those interactions. useCreateObject (line 99-102) deliberately avoids this pattern ("avoids a full refetch flash"), reinforcing that the invalidate in update/delete is inconsistent with the rest of the file. Fix: drop the onSettled invalidate for update/delete.
web/src/editor/SelectionOverlay.tsx:61 — pointerWorld() (lines 60-65) calls svgRef.current?.getBoundingClientRect() on every pointermove, wired through trackPointer's window.addEventListener('pointermove', ...) (lines 21-32) and invoked from begin()'s onMove (line 78). Each move also calls setLiveObject which re-renders the SVG subtree (confirmed via GardenCanvas.tsx's rendered memo keyed on liveObject), so this is a read-after-write layout query on every drag frame. By contrast, useViewport.ts's clientToCanvas (lines 37-43) does the same rect read only in onWheel/onPinch (lines 61, 67), not in onDrag (lines 48-56, which uses delta directly with no rect read) — confirming this is a hotter instance of the pattern than the codebase's existing convention. Fix: measure the rect once in begin()/gesture start and reuse it for the drag's duration.
web/src/editor/GardenCanvas.tsx:82-85 — rendered re-maps and re-sorts the full objects array on every liveObject change (every pointermove during a drag, per SelectionOverlay's setLiveObject calls). Confirmed the useMemo dependency array is [objects, liveObject] (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed GardenEditorPage.tsx:51 (full.data.objects.map(toEditorObject)) produces a new array identity on every render of the page, so the objects dependency itself never stabilizes across unrelated re-renders (e.g., a selectedId change causes GardenEditorPage to re-render, producing a new objects array, invalidating this memo too). Fix: memoize the sorted server-object list keyed only on objects content, and splice the live-dragged object's position into that stable order separately.
🧯 Error handling & edge cases — Blocking issues found
Confirms no pointerId filtering or setPointerCapture anywhere in the frontend. All three findings in the draft are verified against the actual code.
VERDICT: Blocking issues found
web/src/lib/objects.ts:130-136,188-203 — Optimistic PATCH never bumps the cached version, so two edits to the same object issued in quick succession (before the first PATCH's response lands) cause a false 409 that silently discards the second, entirely-valid edit.applyServerPatch (objects.ts:188-202) copies every patched field onto the cached row except version. Inspector.patch() (Inspector.tsx:55-56) reads object.version straight from that cache, so a second rapid mutation on the same object is sent with the stale version. Verified server-side (internal/store/objects.go:93-118): the UPDATE is WHERE id = ? AND version = ? with version = version + 1, so the first PATCH bumps the row's version and the second is genuinely rejected as ErrVersionConflict even though only one client touched it. useUpdateObject's onError (objects.ts:145-158) can't distinguish this from a real two-client conflict — it adopts the server's current row (discarding the second field's change) and shows "That object was updated elsewhere — your change was rolled back," which is false here.
Fix: bump the cached version optimistically in applyServerPatch (or serialize/queue per-object mutations while one is in flight).
web/src/editor/Inspector.tsx:58-63 — Width/Height/X/Y fields accept zero, negative, or absurdly large values with no client-side validation, unlike the drag-resize path.commitDim only checks Number.isFinite(v); it never calls isValidDimensionCm (web/src/lib/units.ts:16-18, bounds [1, 10_000] cm). Server-side, finalizeObject (internal/service/objects.go:234) rejects out-of-range width/height via validDimensionCM with a 400 (not 409). Since ObjectShape.tsx:47-48 clamps rendered half-dimensions to Math.max(0, …), entering e.g. 0 optimistically makes the object vanish from the canvas, then the 400 triggers the generic rollback path (objects.ts:154-157) restoring the old value with a "Could not save that change" toast — a confusing flash-then-revert instead of an upfront rejection/clamp. (Note: for X/Y specifically the server's actual bound is a garden-overlap check, bboxOverlapsGarden, not the isValidDimensionCm range — but the same flash-then-revert failure mode applies there too via a 400.)
Fix: validate with isValidDimensionCm for width/height (and an appropriate bound for X/Y) in commitDim before calling patch(...).
web/src/editor/SelectionOverlay.tsx:19-30 — trackPointer's window-level pointermove/pointerup listeners don't filter by event.pointerId, and no handle calls setPointerCapture. Confirmed by code inspection: no pointerId or setPointerCapture usage anywhere under web/src/editor/ (or the rest of web/src/). On a multi-touch device, a second finger touching the canvas while a move/resize/rotate gesture from the first finger is active will have its pointermove events processed by the same active gesture handler, corrupting the geometry computation.
Fix: capture the pointer on gesture start and filter the window listeners by event.pointerId.
Through the security lens I traced the client-supplied data to its server-side enforcement and found the change is adequately guarded:
Authz: every mutating endpoint the editor calls (POST /gardens/:id/objects, PATCH /objects/:id, DELETE /objects/:id, GET /gardens/:id/full) is behind requireAuth + a requireGardenRole/objectForRole(roleEditor/roleViewer) check in internal/service/objects.go (verified CreateObject/UpdateObject/DeleteObject/GardenFull). The actor is resolved from the HttpOnly session cookie (api.ts:97credentials: 'same-origin'), not from any client-supplied id. A user can only touch objects in gardens they own/edit; cross-user access returns NotFound/Unauthorized (per objects_test.go Bob-vs-Alice cases).
Input validation: kind, shape, dimensions, and color are all validated in finalizeObject (internal/service/objects.go:220+): unknown kinds rejected, isHexColor enforced, name length capped, out-of-bounds dimensions rejected. So the optimistic create.mutate({kind: def.kind, ...}) and inspector color/null-clear paths are server-validated regardless of client trust. The client's MIN_OBJ_CM clamp and #rrggbb color input are UX only; the server is the authority.
XSS / rendering: toast messages (toast.tsx) and inspector fields (name, notes) are rendered as React text children — no dangerouslySetInnerHTML anywhere in the diff or ObjectShape.tsx (verified). err.message/conflict text surfaced in toasts originates from server-controlled ApiError bodies, not user input.
?focus=<id> (router.tsx): validated as a positive integer and only used to frame the viewport — it does not authorize fetching; GardenFull returns only the objects the viewer is entitled to see, so a guessed id can't reveal anything.
Conflict adoption (conflictObject in objects.ts): the 409 body.current is server-asserted and re-parsed through serverObjectSchema; it overwrites the cache with the server row, which is the correct trust direction.
No injection (SQL/command/path), SSRF, unsafe deserialization, secret leakage, missing validation, or insecure defaults in this change.
🎯 Correctness — Minor issues
All three findings verify against the actual source. Confirming each:
Finding 1 — commitDim (Inspector.tsx:58-63) compares cmFromDisplay(v, unit) to current with no dirty guard. The width/height/x/y state is initialized from String(displayFromCm(...)) and re-synced the same way (lines 33-36, 48-51). Re-deriving the imperial round-trip: 100 cm → displayFromCm = round(100/2.54/12 * 10)/10 = 3.3 → cmFromDisplay(3.3,'imperial') = round(3.3*12*2.54) = 101, so 101 !== 100 fires patch({widthCm:101}) on a no-op blur. Metric drifts for non-integer cm (a free resize yields floats; e.g. 123.4 cm → "1.23" → 123). Rotation onBlur (line ~135) compares v !== object.rotationDeg against the raw value while the field shows Math.round(...), so 30.5° → "31" → commits 31. Confirmed.
Finding 2 — useUpdateObject.onMutate (objects.ts:130-138) calls applyServerPatch (188-202) which does not touch version; onSuccess (139-144) replaces the row with the server row (bumped version). SelectionOverlay captures const base = { ...object } (line 88) and PATCHes version: base.version on release (lines 96/118/132). No update.isPending guard exists in SelectionOverlay or Inspector. A second gesture begun before the first PATCH resolves reads the still-old cache version, sends a stale version, and 409s → onError adopts the server row and toasts "rolled back." Confirmed.
Finding 3 — startRotate's onMove closure (SelectionOverlay.tsx:125-130) references e.shiftKey from the pointerdown e, while trackPointer invokes onMove(pointerWorld(ev)) with the live ev not passed in. So Shift must be held at pointerdown; pressing/releasing mid-drag has no effect, contradicting the "free with Shift" comment. Confirmed.
VERDICT: Minor issues
web/src/editor/Inspector.tsx:62 — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.commitDim runs on every blur and compares cmFromDisplay(v, unit) to current with no dirty guard. Because displayFromCm rounds (imperial → 0.1 ft, metric → 2 dp cm) and cmFromDisplay re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → displayFromCm gives 3.3 ft → cmFromDisplay(3.3,'imperial') = round(3.3*12*2.54) = 101 cm, so on an imperial garden focusing a Width/Height/X/Y field and blurring without typing fires patch({ widthCm: 101 }), silently mutating the object by 1 cm and bumping its version. Metric drifts too for non-integer cm (a free resize produces floats, e.g. 123.4 cm → "1.23" → 123). The Rotation onBlur has the same shape: state is String(Math.round(object.rotationDeg)), so a free rotation of 30.5° displays "31" and a focus/blur commits 31. Fix: guard no-op blurs by comparing the raw string to String(displayFromCm(current, unit)) (or track a dirty flag set in onChange); for rotation compare against the rounded current rather than raw object.rotationDeg.
web/src/lib/objects.ts:130 — back-to-back gestures self-conflict on version.onMutate applies the patch via applyServerPatch but does not bump version; the version only advances on onSuccess (server row). SelectionOverlay reads object.version from the cache at gesture start (base.version) and PATCHes with it on release. If a second gesture begins before the first PATCH resolves, base.version is still the pre-edit version, so the second PATCH carries a stale version; once the first PATCH lands (server version+1), the second request 409s and onError adopts the server row + toasts "rolled back" — even though no other client touched it. There is no update.isPending guard in SelectionOverlay or Inspector. Fix: optimistically increment version in onMutate (reconcile on onSuccess/onError), or disable further gestures while a mutation for that object id is pending.
web/src/editor/SelectionOverlay.tsx:128 — Shift state captured at gesture start, not tracked during the drag. The onMove closure in startRotate references e.shiftKey from the original pointerdown event (if (!e.shiftKey) deg = Math.round(...)). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (ev.shiftKey) inside onMove instead of from e.
🧹 Code cleanliness & maintainability — Minor issues
Review
Verdict: Minor issues
Through the code-cleanliness & maintainability lens, I verified each finding against the checked-out source. All five survive independent confirmation:
web/src/editor/SelectionOverlay.tsx:57-58 — two sources of truth for scale.scale = useEditorStore((s) => s.viewport.scale) (line 52) feeds the render-time handleCm/rotateOffsetCm (lines 57-58), while pointerWorld (lines 60-65) re-reads useEditorStore.getState().viewport on every move. The split is functionally intentional (scale can change mid-gesture), but with no comment a reader can't tell that handleCm is hit-size-only. Minor readability footgun — consider a clarifying comment.
web/src/lib/objects.ts:188-201 — applyServerPatch is a verbose per-field conditional spread. Lines 189-201 repeat ...(patch.x !== undefined ? { x: ... } : {}) ten times, one per ObjectPatch field. A generic Object.entries(patch) filter-and-merge (excluding id/version) would be ~3 lines and wouldn't drift from ObjectPatch as fields are added.
web/src/editor/GardenCanvas.tsx:81-85 — liveObject merge silently no-ops on id mismatch. Line 83 objects.map((o) => (o.id === liveObject.id ? liveObject : o)) does nothing if the live object's id isn't in objects (e.g. a delete racing a drag). The merge reads as if liveObject always corresponds to a cached row; a one-line comment (or an early return) would make the intent explicit.
web/src/editor/GardenCanvas.tsx:16-21 — kind z-index defaults split from kind metadata.UNDER_KINDS/defaultZForKind encode per-kind z-ordering policy here, while kinds.ts (lines 5-22) already owns per-kind default sizes/labels/shape. The "paths/structures under beds, trees above" ordering that the render sort (a.zIndex - b.zIndex, line 84) relies on is placement-time policy split across two files. Low-impact leaky split of kind metadata.
web/src/components/ui/toast.tsx:32 — ToastItem prop toast shadows the imported/exported toast helper. The component parameter { toast } (line 32) shadows the module-level export const toast = {...} (lines 27-30) within the component body. It works (the component only reads toast.id/toast.tone/toast.message), but shadowing a same-file exported symbol is a readability trap — rename the prop to data or t.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens I checked the hot paths (per-pointermove drag updates, pan/zoom re-renders, mutation cache writes) and found no material regression:
SelectionOverlay/GardenCanvas drag loop: each pointermove does one setLiveObject → one GardenCanvas re-render. Work per frame is bounded: a single-object merge (objects.map with one replacement) + one [...merged].sort(). The sort is O(n log n) worst case but V8's Timsort is adaptive for the nearly-sorted z-order that changes by one element, so it's effectively ~O(n) per frame. No N+1, no per-element queries. Verified at GardenCanvas.tsx:82-85 and SelectionOverlay.tsx:77-84.
getBoundingClientRect() is called per pointer event (SelectionOverlay.tsx:61, GardenCanvas.tsx:99); it's a cheap layout read, not a forced reflow beyond what's needed for coordinate conversion. Fine on the hot path.
Mutations (useUpdateObject/useDeleteObject) rewrite the cache with objects.map(...) — O(n) per mutation, one per gesture end, not inside the drag loop. Verified at objects.ts:130-144.
Toaster: nextId module counter + array filtered on 4s timeout dismiss; no unbounded growth. Verified at toast.tsx:18-24.
One trivial, non-material observation (not worth blocking on): GardenCanvas.tsx:86 recomputes selected = rendered.find(...) on every render, and GardenCanvas re-renders on every viewport change (pan/zoom) and every liveObject update. That's an extra O(n) scan per frame on top of the sort. For a v1 garden (dozens of objects) this is negligible; if gardens ever hold thousands of objects it's a trivial fix (derive selected from selectedId against objects directly, or memoize). I'm noting it only because it's genuinely in the hot render path; impact is not material at current scale.
🧯 Error handling & edge cases — Minor issues
Both surviving findings are confirmed against the actual code:
Color input PATCH on every onChange — Inspector.tsx:155 calls patch({ color: e.target.value }) directly in onChange, which fires continuously during a color-slider drag. Unlike geometry fields (which commit on onBlur), this fires a PATCH per intermediate value, all with the same object.version, so concurrent edits can race and trigger 409/rollback. Confirmed.
Resize center drift past corner — SelectionOverlay.tsx:108-115: newW/newH are clamped to MIN_OBJ_CM, but newCenterLocal and thus c keep recomputing from p (the dragged pointer position) even when the pointer crosses the fixed opposite corner. So the center follows the pointer while size is pinned at minimum — a visible UX edge. Confirmed.
The other draft items (lost-update on Inspector version, click-without-move no-op, GardenCanvas armed-kind consumption, rotate Shift freeze, focus-on-deleted-object) were already self-withdrawn in the draft or are non-issues, so they are dropped.
VERDICT: Minor issues
web/src/editor/Inspector.tsx:155 — color onChange fires a PATCH per pixel of a slider drag. The <input type="color"> commits via onChange directly into patch({ color }), unlike the geometry fields (one PATCH per gesture / on blur). Each intermediate value carries the same object.version, so rapid changes race; a 409 mid-drag rolls back to the server value and toasts, discarding the user's in-progress selection. Minor — a real unhappy path but bounded.
web/src/editor/SelectionOverlay.tsx:108 — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.newW/newH are clamped to MIN_OBJ_CM, but newCenterLocal is still derived from the live pointer position p, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Through the security lens, I scrutinized the optimistic sync layer (lib/objects.ts), the gesture-based canvas interactions (editor/SelectionOverlay.tsx, editor/GardenCanvas.tsx), the inspector form (editor/Inspector.tsx), routing (router.tsx), and the toast system (components/ui/toast.tsx).
What I checked:
Authn/Authz: gardenEditorRoute gates on requireAuth; the backend is responsible for garden-scoped authorization on /gardens/:id/full and object mutations.
Injection: API paths are template-literal assembled from validated numeric IDs (object.id, gardenId). No user-controlled strings reach URL paths. No SQL/command/template injection surfaces.
XSS: object.name is rendered as a React text child in SVG <text> — automatically escaped, no dangerouslySetInnerHTML. Toast messages are hardcoded strings. No inline event handlers or HTML injection vectors.
Open redirect: The ?redirect= parameter on /login is funneled through safeRedirectPath, which rejects protocol-relative and absolute URLs. The new ?focus= parameter is validated to a positive integer and only used to look up an object by ID; it never drives navigation.
Optimistic locking: useUpdateObject requires version and correctly handles 409 conflicts by rolling back to the server's row. This is a security-positive pattern that prevents lost-update races.
Input validation: serverObjectSchema (Zod) validates all API responses. kindDef restricts created kinds to the known allow-list. Color inputs are constrained to <input type="color">.
No authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, or insecure defaults were identified.
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/editor/SelectionOverlay.tsx:88 — base = { ...object } snapshots the object's version at render time. The gesture-end PATCH sends base.version, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict during the drag. The fix is to read the current cached row's version when building the patch on release, or to derive it from the latest liveObject / query data rather than the stale closure.
web/src/editor/GardenCanvas.tsx:74-82 — The fittedForRef.current !== garden.id guard means the canvas frames the garden (or a ?focus= object) exactly once per garden mount. If the focusId search param changes client-side without remounting the page, the viewport does not re-fit to the new object because the guard only tracks garden.id, not the focus target. Track the last focused id and re-frame when it changes.
web/src/editor/GardenCanvas.tsx:78-80 — The focus rectangle for ?focus=<id> treats the object's bounds as axis-aligned: {x: focus.xCm - focus.widthCm/2, y: focus.yCm - focus.heightCm/2, w: focus.widthCm, h: focus.heightCm}. If the object has a non-zero rotationDeg, its axis-aligned bounding box is larger than widthCm × heightCm (by up to a factor of |cos|+|sin|). zoomToFitRect will clip the rotated corners. Compute the rotated AABB or at least pad the rect.
web/src/editor/SelectionOverlay.tsx:122-134 — In startRotate, e.shiftKey is evaluated from the initial ReactPointerEvent captured in the closure. Because the onMove callback only receives the world Point (not the current native PointerEvent), holding or releasing Shift mid-gesture has no effect; snap/free mode is locked at pointer-down. Pass the current event's modifier state into the move callback so Shift can be toggled live during the drag.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
I read Inspector.tsx, SelectionOverlay.tsx, ObjectShape.tsx, lib/objects.ts, useViewport.ts, editor/store.ts, and the new components/ui/toast.tsx. The diff is well-factored and follows the repo's existing patterns.
No dead or duplicated code: the new modules each have a single responsibility, and shared utilities (lib/geometry.ts, cn(), Button, TextField) are reused rather than copy-pasted.
No confusing or misleading names: store fields like liveObject, armedKind, and objectDragging are self-describing; gesture helpers in SelectionOverlay (startMove, startResize, startRotate) are concise and accurate.
No overly long or deeply nested functions: the heaviest logic (resize/rotate geometry) is delegated to localToWorld/worldToLocal in lib/geometry.ts; applyServerPatch is a flat sequence of conditional spreads, which is appropriate.
Follows existing patterns: toast.tsx uses the same cn() utility as the rest of the UI, Inspector uses the same TextField/TextArea primitives, and the react-query mutations follow the same optimistic-update structure already in the codebase.
No leaky abstractions: the object-local frame math stays in lib/geometry.ts, and the canvas doesn't leak SVG details into the store or data layer.
⚡ Performance — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:51 — const objects = full.data.objects.map(toEditorObject) is unmemoized. toEditorObject creates a new EditorObject instance for every server object on every parent render. This defeats the React.memo on ObjectShape, forcing all objects to re-render whenever GardenEditorPage re-renders (e.g., on selectedId change or background refetch). Fix: wrap the mapping in useMemo(() => full.data.objects.map(toEditorObject), [full.data.objects]).
web/src/editor/GardenCanvas.tsx:82-85 — During move/resize/rotate gestures, liveObject updates on every pointermove frame, yet the rendered memo performs an O(n) .map() copy plus O(n log n) .sort() on each frame to merge a single changed object. For gardens with many objects this creates jank and allocation pressure at interaction frequency even though z-order is unchanged during a single-object gesture. Fix: keep a stable z-sorted array and only re-sort when z-indices change, or split the liveObject merge from the sort and only sort when objects itself changes.
web/src/editor/Inspector.tsx:154-155 — The color <input> fires patch({ color: e.target.value }) on every onChange event. A native color picker emits change events continuously while dragging, generating a burst of optimistic cache patches, query cancellations, and network PATCHes. Fix: debounce the color mutation (e.g., 200 ms) or use local state for the live preview and commit on blur.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/editor/SelectionOverlay.tsx:21-31 — trackPointer attaches window listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on window. On pointerup the onEnd closure fires, calling setObjectDragging(false) and update.mutate(fields(final)) against the old garden's mutation hook, potentially PATCHing the wrong context or a deleted object. Fix: store the end cleanup in a ref and invoke it in a useEffect cleanup, or use a useEffect-based listener pattern so React tears the listeners down on unmount.
web/src/editor/Inspector.tsx:58-63 — commitDim parses the raw input with parseFloat and sends the result without validating that it is positive or at least MIN_OBJ_CM. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (MIN_OBJ_CM = 1). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: reject v <= 0 (or cm <= 0) in commitDim and surface a brief inline warning instead of calling apply.
Minor issues
web/src/editor/Inspector.tsx:44-53 — The re-sync useEffect aborts entirely when any field inside the inspector has focus (rootRef.current?.contains(document.activeElement)). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.
web/src/editor/Inspector.tsx:151-157 — The color <input> uses onChange, committing a PATCH on every mouse/touch adjustment inside the color picker. Because each PATCH carries the current version, rapid changes can launch overlapping requests; the first succeeds and bumps the version, causing subsequent in-flight requests to 409 and roll back. Fix: commit color on onBlur (or debounce with a short trailing edge) so the gesture completes before versioning is contested.
web/src/editor/SelectionOverlay.tsx:122-134 — startRotate captures e.shiftKey from the initial ReactPointerEvent closure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: read shiftKey from the live PointerEvent passed to the move handler instead of the stale closed-over React event.
I traced tainted input through every sink this PR introduces, and checked the actual checked-out code (not just the diff) for each path below:
Server → UI text sinks: object.name, object.notes, err.message, and kindDef(object.kind)?.label ?? object.kind are all rendered as React text children (<text>, <h2>, <TextField value>, toast {toast.message}). No dangerouslySetInnerHTML/innerHTML/eval exists anywhere in web/src (grep confirmed). React escapes text content, so server-controlled strings can't XSS. Verified in ObjectShape.tsx:95-107, Inspector.tsx:70, toast.tsx:42-44.
Server response parsing: serverObjectSchema.parse / fullGardenSchema.parse (zod) gate every server payload entering the cache, including the optimistic onSuccess splices and the 409 conflict body via conflictObject's safeParse (objects.ts:206-211). The 409 path reads err.body.current as unknown and only adopts it if safeParse succeeds — no trust of raw body shape.
URL-derived ?focus=<id>: router.tsx:93-96 coerces with Number(search.focus) and gates on Number.isInteger(f) && f > 0. Arrays/objects/strings like "1; ..." become NaN and are rejected (returns {}). The surviving integer is only used as objects.find(o => o.id === focusId) (GardenCanvas.tsx:70) — a numeric lookup against this garden's already-validated object list, never interpolated into a URL or attribute. If the id isn't found, target falls back to gardenRect. No injection / IDOR surface introduced here.
URL path interpolation in new mutations (objects.ts:70,98,127,166): /gardens/${gardenId}/... and /objects/${id} use gid = Number(gardenId) (route param) and object.id/patch.id (numbers off zod-validated server rows or local numeric state). Number() of a non-numeric route segment would yield NaN, which the server would reject — not an injection vector. No user-controlled string reaches a path segment.
SVG fill/stroke attributes: object.color (server-controlled, schema z.string().nullable().optional()) flows into fill={fill} (ObjectShape.tsx:74,88) and the color input's value (Inspector.tsx:154). React sets SVG attributes via setAttribute with string values — attribute-value quoting prevents breakout, so a value like " onmouseover=... can't inject a handler. The <input type="color"> itself coerces its value to a valid #rrggbb or #000000. No scripting surface.
Mutation request bodies: ObjectCreate/ObjectPatch are typed objects built from local form/geometry state, serialized via JSON.stringify in api.ts:90. No template/SQL/string-concat assembly; no injection.
Auth: All new requests go through apiFetch, which sends credentials: 'same-origin' and is gated by requireAuth in beforeLoad on gardenEditorRoute (router.tsx). No unauthenticated mutation paths were added.
CSRF: Cookie-auth with no CSRF token is a pre-existing property of api.ts, not something this PR introduces or worsens; out of this PR's scope and not a regression.
Nothing in the security lens is materially wrong here.
🎯 Correctness — Minor issues
All three findings confirmed against the actual source. Verifying line numbers and the mutation/conflict path.
All three findings are confirmed against the actual code. Correcting the line numbers: the color onChange is at line 155 (not 131), the imperial round-trip drift is in commitDim at line 62 (not 73), and e.shiftKey is at line 128 (not 142).
Corrected review
VERDICT: Minor issues
web/src/editor/Inspector.tsx:155 — color onChange fires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks. The native <input type="color"> emits input events continuously while the user drags through the OS color picker; React's onChange maps to that input event. Each tick calls patch({ color }), sending the currentobject.version. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new version still carries version N → the server returns 409 → conflictObject adopts the server row and toasts "That object was updated elsewhere — your change was rolled back." The user's color reverts with a misleading conflict toast, even though they were the only editor. Every other field commits on blur; color is the lone exception. Fix: commit color on onBlur (capture the pending value in local state via onChange, dispatch the PATCH on blur) — or debounce. Verified against the Inspector and the useUpdateObject 409 path (web/src/lib/objects.ts:145-158).
web/src/editor/Inspector.tsx:62 — imperial unit round-trip drift can fire a spurious PATCH (and 1 cm change) on blur-without-editing.displayFromCm(240, 'imperial') → 7.9 ft; if the user focuses a dimension/position field and blurs without typing, commitDim runs cmFromDisplay(7.9, 'imperial') = Math.round(7.9 * 12 * 2.54) = 241, which differs from the current 240, so it PATCHes a 1 cm change the user never made. Metric round-trips exactly (so this only bites imperial gardens). Verified against web/src/lib/units.ts:31-44. Fix: skip the PATCH when the parsed display value equals the field's current display value, or compare on the rounded display value rather than raw cm.
web/src/editor/SelectionOverlay.tsx:128 — rotate uses e.shiftKey from the pointerdown event, not the live pointermove.e is the ReactPointerEvent captured in the startRotate closure at pointerdown, so pressing/releasing Shift during the drag does not toggle free-rotate vs. snap. The spec ("free with Shift", per the component doc comment on line 38) implies live Shift state. Minor UX/correctness gap. Fix: read ev.shiftKey inside the onMove callback (the trackPointerPointerEvent carries it).
Verified-clean (no issue): resize opposite-corner math in the object-local frame (worldToLocal/localToWorld are true inverses); rotate +90° offset matches the knob's local -y placement; ?focus=<id> fit rect (center→top-left conversion); z-order merge with liveObject; conflict adoption on 409; serverObjectSchema matches the server's GardenObject JSON tags; color: null PATCH path; delete needs no version.
🧹 Code cleanliness & maintainability — Minor issues
All five findings verified against the actual source.
VERDICT: Minor issues
web/src/lib/objects.ts:188-202 — applyServerPatch repeats the same conditional-spread idiom for all 10 patchable fields. Each ObjectPatch field is hand-mirrored here, so adding a new patchable field means editing both the interface (108-121) and this helper in lockstep, with nothing enforcing they stay in sync. A generic Object.entries(patch).filter(([k]) => k !== 'id' && k !== 'version') fold would collapse the body and make it field-agnostic. Verified by reading the file.
web/src/editor/Inspector.tsx:55-56 — patch is typed against EditorObject, not the real ObjectPatch, so it accepts fields the server ignores.patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) => update.mutate({ id, version, ...fields }). EditorObject (types.ts:8-23) includes kind and shape, but ObjectPatch (objects.ts:108-121) has neither. Because the fields are spread into the mutate argument (excess-property checks don't fire on spreads), patch({ kind: 'tree' }) or patch({ shape: 'circle' }) typechecks cleanly yet is silently dropped by the server. Typing the arg as Omit<ObjectPatch, 'id' | 'version'> would let the compiler catch these. Verified across Inspector.tsx, types.ts, and objects.ts.
web/src/editor/GardenCanvas.tsx:16-21 — per-kind default-z knowledge is split from kinds.ts.UNDER_KINDS / defaultZForKind encode per-kind layering metadata that belongs in KindDef (kinds.ts:5-12), which already holds each kind's label/icon/shape/default size. Adding a new kind means editing kinds.tsand remembering to update UNDER_KINDS here. A defaultZ (or layer: 'under' | 'default' | 'over') field on KindDef would centralize it and let GardenCanvas read kindDef(kind).defaultZ. Verified by reading both files.
web/src/editor/Inspector.tsx:31-37 and :44-53 — the field-init and re-sync logic duplicate the same String(displayFromCm(...)) / String(Math.round(...)) conversions. Seven useState initializers and the useEffect both spell out the same per-field formatting. A small fieldsFromObject(object, unit) helper returning { name, notes, width, height, x, y, rotation } would remove the duplication and make a future field a one-line add in both places. Trivial.
web/src/components/ui/toast.tsx:32 — the toast prop name shadows the module's toast export.function ToastItem({ toast }) shadows the toast object exported at line 27. Not a bug (the component doesn't use the export), but a reader scanning the file sees toast.id/toast.message and has to disambiguate which toast is meant. Renaming the prop to data or item (or the export to toastApi) would remove the ambiguity. Trivial.
⚡ Performance — Minor issues
All three findings confirmed against actual code.
All three findings confirmed against the actual code. The store confirms setLiveObject is called per pointermove (line 78 of SelectionOverlay), liveObject is a dep of the rendered useMemo (line 85), and the page-level .map(toEditorObject) runs every render (line 51).
VERDICT: Minor issues
web/src/editor/SelectionOverlay.tsx:60-65 — getBoundingClientRect() called on every pointermove during a drag.pointerWorld reads svgRef.current?.getBoundingClientRect() and is invoked from the trackPointer move callback (line 78) on every pointer event. getBoundingClientRect forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live setLiveObject re-render is also mutating the SVG, so the layout is dirty. This is the textbook layout-thrash pattern and the most likely cause of jank on lower-end devices, especially with touch. Fix: capture the rect once at gesture start (in begin) and pass it into pointerWorld, or cache it on the base snapshot. The viewport tx/ty/scale are already read fresh from the store via useEditorStore.getState().viewport, so the rect (which only changes on layout/scroll) is the only thing that needs caching.
web/src/editor/GardenCanvas.tsx:82-85 — re-sort on every pointermove during a gesture.rendered depends on liveObject (line 85), which changes on every pointermove of a move/resize/rotate via setLiveObject, so [...merged].sort((a, b) => a.zIndex - b.zIndex) runs O(n log n) (plus a fresh array + an objects.map merge) on every move event. Z-order does not change during a geometry gesture, so this is wasted work. For typical gardens it's negligible, but it scales with object count and event rate. Suggested fix: split the merge from the sort, and only sort when objects changes (e.g. sort once with useMemo([objects]), then merge liveObject into the already-ordered list).
web/src/pages/GardenEditorPage.tsx:51 — objects array rebuilt with new identities every render.full.data.objects.map(toEditorObject) produces a fresh array of fresh object literals on every render of GardenEditorPage. ObjectShape is memo'd (verified at ObjectShape.tsx:37), but the memo is defeated because each render hands it a brand-new object reference. This means every parent re-render (selection change, query invalidation after a PATCH's onSettled, focus param change) re-renders every object, even unchanged ones. It's not on the pan/zoom hot path (pan only re-renders GardenCanvas, not the page), so impact is limited, but after each gesture-release PATCH the onSettled invalidation → refetch → full remap → all objects re-render. Consider memoizing the mapping (useMemo on full.data.objects) or mapping server objects to editor objects once when the query returns.
🧯 Error handling & edge cases — Minor issues
I've verified all four findings against the actual source. All confirmed.
Verdict: Minor issues
web/src/editor/Inspector.tsx:155 — color picker fires a PATCH on every onChange, storming the server with stale-version 409s.<input type="color"> emits onChange continuously while the user drags in the native picker. Each call invokes patch({ color: e.target.value }) (Inspector.tsx:55-56), which reads object.version from the prop. The optimistic applyServerPatch (objects.ts:188-202) overrides only the patched field and never touches version; onSuccess (objects.ts:139-144) only replaces the row after the network round trip. So the 2nd..Nth PATCHes in a single color drag all carry the same stale version as the first. The server accepts PATCH1 (vN→vN+1) and returns 409 for PATCH2..N. useUpdateObject.onError (objects.ts:145-157) treats every 409 as a conflict: it adopts current and toasts 'That object was updated elsewhere — your change was rolled back.'. Net effect: a single color drag spams conflict toasts and cache churn even though the user is the only one editing. Fix: debounce the color commit, or use local state + commit-on-blur like the other fields. Verified against Inspector.tsx:155, objects.ts:188 (applyServerPatch never sets version), objects.ts:145-153, and api.ts:25-27 (isConflict = status 409).
**web/src/editor/SelectionOverlay.tsx:21-32, 77-86 — window pointer listeners leak if the overlay unmounts mid-gesture; objectDraggingcan get stuck.**trackPointerregisterspointermove/pointerup/pointercancelonwindowand removes them only from insideend. There is no useEffectcleanup inSelectionOverlay. If the overlay unmounts during a drag — e.g. the selected object is deleted out-of-band so selectedbecomesnullinGardenCanvas.tsx:86and the overlay is removed at line 142 — thewindowlisteners remain registered. When the pointer is finally released,onEndrunsupdate.mutate(...)against a now-staleupdate(PATCH on a deleted/unknown object → 404 → "Could not save that change." toast), andsetObjectDragging(false)/setLiveObject(null)run on the global store (still safe, since the store is module-level). More importantly, if the gesture'spointerupnever fires (rare iframe/pointer-capture edge cases),objectDraggingstaystrueanduseViewport's pan (useViewport.ts:50) is permanently disabled. Fix: register the cleanup in a useEffectwhose cleanup also removes the listeners and resetsobjectDragging/liveObject. Verified by reading SelectionOverlay.tsx:20-86, GardenCanvas.tsx:86,142, and useViewport.ts:50`.
**web/src/editor/Inspector.tsx:58-63 — commitDimaccepts 0/negative values, briefly corrupting the optimistic cache.**commitDimonly checksNumber.isFinite(v); typing 0or-5for width/height passes the guard,cmFromDisplay(v, unit)(units.ts:31-33) rounds to0/negative cm, and onMutate(objects.ts:130-138) applies that to the cache. The render inGardenCanvasthen draws the object with a zero/negative width (thewidth={object.widthCm}flips; the overlay'shalfW = object.widthCm/2goes negative). The server rejects viavalidDimensionCM(gardens.go:170-172,v >= minGardenCMwhereminGardenCM = 1), onErrorrolls back toctx.prev, and the field re-syncs. Net effect is a brief visual glitch and a wasted network round trip — not data loss, but an avoidable unhappy path. Suggested fix: clamp the parsed value to >= MIN_DIMENSION_CM(units.ts:12) before patching, matching the gesture path'sMIN_OBJ_CMclamp inSelectionOverlay.tsx:108. Verified by reading Inspector.tsx:58-63, units.ts:31, and gardens.go:170`.
**web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.** select(null)runs beforedel.mutate(object.id). If the DELETE fails (network/403/500), onError(objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the selection in the delete mutation'sonSuccess(or in themutatecall'sonSuccessoption) rather than eagerly before the request. Verified by readingInspector.tsx:191-194andobjects.ts:163-179`.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
I reviewed the diff and the checked-out files through the security lens:
Authn/Authz: The gardenEditorRoute retains its requireAuth guard; the route search param focus is validated (positive integer only) before reaching the canvas.
Injection: No SQL/command/path/template injection vectors. API endpoints consume structured request bodies (ObjectCreate, ObjectPatch), not string concatenation. The api.patch/api.post paths are hardcoded.
SSRF: All API URLs are hardcoded /api/v1/… paths; no user-supplied URLs are fetched.
Unsafe deserialization / secret leakage: None found. The useId() output is sanitized for the SVG pattern ID; no hardcoded secrets.
Input validation / tainted data: Server responses are parsed through zod schemas (serverObjectSchema, fullGardenSchema). The notes field is rendered via React’s textarea (escaped text content), not dangerouslySetInnerHTML. The SVG <text> node renders {object.name} through React’s normal escaping.
409 conflict / optimistic sync: On a 409 the client adopts the server’s current row from the ApiError body, which is then zod-parsed before entering the cache.
Outside my lens: None.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/SelectionOverlay.tsx:138-150 — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place.
web/src/lib/objects.ts:188-202 — applyServerPatch manually spreads each of the 10 patchable fields with !== undefined guards. Adding a new patchable field requires touching this function, the interface, and every call site. Replace the 15-line wall with a loop over the known patch keys (or Object.fromEntries filtered by !== undefined) so new fields are automatically handled.
web/src/editor/Inspector.tsx:89-130 — The width/height/x/y fields are four near-copies of the same <TextField> boilerplate with only label/name/value/current-value/field-key differing. Extract a tiny <DimField label={…} value={…} currentCm={…} onCommit={…} /> component to eliminate the repetition.
web/src/editor/GardenCanvas.tsx:15-21 — defaultZForKind hard-codes kind strings ('path', 'structure', 'tree', etc.) that must be kept in sync with kinds.ts. If a kind is renamed or added there, this silently falls back to the wrong layer. Move the default z-index into the KindDef interface and the OBJECT_KINDS table so all kind metadata lives in one file.
web/src/editor/GardenCanvas.tsx:129 — The grid <path> and the garden boundary <rect> (line 136) were collapsed from readable multi-line props into single long lines. This is a readability regression; restore the multi-line formatting so diffs remain meaningful.
web/src/editor/SelectionOverlay.tsx:168 — All four resize handles are hard-coded to cursor: 'nwse-resize'. The top-right and bottom-left corners should use nesw-resize. Map the correct cursor per corner instead of using one value for all.
⚡ Performance — Minor issues
Minor issues
web/src/editor/GardenCanvas.tsx:82-85 — rendered is recomputed (shallow-copy + z-sort) on every liveObject change. During a move/resize/rotate gesture, SelectionOverlay calls setLiveObject on every pointermove event, so the entire object list is sorted O(n log n) in a hot loop even though zIndex never changes during geometry gestures. For gardens with many objects this becomes noticeable. Fix: memoize the z-sorted base array against objects alone, then merge liveObject into it without re-sorting.
web/src/editor/SelectionOverlay.tsx:60-65 — pointerWorld calls svgRef.current?.getBoundingClientRect() on every pointermove event during a drag/resize/rotate gesture. Layout reads in a sustained pointer loop can force synchronous layout work and contribute to frame drops. The SVG bounding rect is stable during object drag because viewport panning is disabled. Fix: snapshot the SVG bounding rect once at gesture start (e.g., in the begin helper or in each start* handler) and reuse it in the window-level pointermove callback.
web/src/pages/GardenEditorPage.tsx:51 — objects = full.data.objects.map(toEditorObject) creates a new array on every parent render (e.g. when selectedId changes on tap). Because GardenCanvas takes objects as a prop, its useMemo for the sorted rendered array re-runs even when the underlying server data is unchanged. Fix: wrap the map in useMemo so the array reference is stable across transient parent re-renders.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
Viewport zoom is not suppressed during object drag/resize/rotate, so wheel or pinch mid-gesture changes viewport.scale, which pointerWorld reads on every move event. The delta between the gesture-start world point (computed with the old scale) and the current world point (computed with the new scale) is wrong, causing the selected object to jump erratically. web/src/editor/useViewport.ts:58-69 Fix: Add the same guard that onDrag already has: if (useEditorStore.getState().objectDragging) return inside onWheel and onPinch.
trackPointer window listeners leak if SelectionOverlay unmounts mid-gesture (e.g., object is deleted by another client, user navigates away, or inspector deletes the object). The pointermove/pointerup/pointercancel handlers stay on window until the pointer is released, and the onEnd closure will still call update.mutate(fields(final)) after the component is gone, potentially patching the wrong object or interfering with a newly-mounted overlay. web/src/editor/SelectionOverlay.tsx:21-32 Fix: Have trackPointer return a cleanup function; store it in a ref and invoke it from a useEffect cleanup.
Inspector checkbox (and color) PATCH on every onChange without debouncing, so rapid toggles self-conflict with the optimistic-concurrency version check. Two quick clicks send two PATCHes with the same version; the first succeeds and bumps the server version, the second 409s and the cache rolls back to the server value, losing the user's second toggle. web/src/editor/Inspector.tsx:155,170 Fix: Use local state for the checkbox (like the text fields do) and commit on blur, or debounce/throttle the mutation.
Inspector dimension fields accept negative values, while the canvas resize enforces MIN_OBJ_CM = 1. Typing a negative width/height in the inspector sends a PATCH with negative cm, which corrupts the optimistic cache and can render the object inside-out until the server rejects it (if it even does). web/src/editor/Inspector.tsx:58-63 Fix: Reject or clamp negative / near-zero values in commitDim, mirroring SelectionOverlay's Math.max(MIN_OBJ_CM, …) logic.
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** · 31 findings (14 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object | `web/src/editor/GardenCanvas.tsx:82` | 5/5 | maintainability, performance |
| 🔴 | Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits | `web/src/lib/objects.ts:188` | 4/5 | error-handling, maintainability |
| 🔴 | Inspector commitDim accepts zero and negative dimensions | `web/src/editor/Inspector.tsx:55` | 4/5 | error-handling, maintainability |
| 🔴 | Window pointer listeners leak when SelectionOverlay unmounts mid-drag | `web/src/editor/SelectionOverlay.tsx:19` | 4/5 | error-handling |
| 🟠 | getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash) | `web/src/editor/SelectionOverlay.tsx:57` | 4/5 | maintainability, performance |
| 🔴 | Color onChange fires a PATCH per picker drag step → version storms and spurious 409 rollbacks with a misleading 'updated elsewhere' toast | `web/src/editor/Inspector.tsx:151` | 3/5 | correctness, error-handling, performance |
| 🟠 | defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts | `web/src/editor/GardenCanvas.tsx:15` | 3/5 | maintainability |
| 🟠 | Shift key state locked at gesture start instead of evaluated live during rotate | `web/src/editor/SelectionOverlay.tsx:128` | 3/5 | correctness |
| 🟠 | Unmemoized toEditorObject mapping breaks downstream memoization and causes unnecessary allocations | `web/src/pages/GardenEditorPage.tsx:51` | 3/5 | performance |
| 🔴 | fittedForRef guard prevents re-framing when focusId changes for same garden | `web/src/editor/GardenCanvas.tsx:71` | 2/5 | correctness |
| 🔴 | Optimistic PATCH never bumps cached version, causing false 409 conflicts and silent loss of the user's own concurrent edits | `web/src/lib/objects.ts:130` | 2/5 | correctness |
| 🟠 | Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur | `web/src/editor/Inspector.tsx:62` | 2/5 | correctness |
| 🟠 | Four dimension TextFields are copy-pasted with only label/value differing | `web/src/editor/Inspector.tsx:89` | 2/5 | maintainability |
| 🟡 | ToastItem prop 'toast' shadows the same-file exported 'toast' helper | `web/src/components/ui/toast.tsx:32` | 2/5 | maintainability |
<details><summary>17 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | Stale version captured in gesture closure causes spurious 409s | `web/src/editor/SelectionOverlay.tsx:88` | kimi-k2.6:cloud | correctness |
| 🔴 | Wheel/pinch not blocked during object drag, causing coordinate jump | `web/src/editor/useViewport.ts:58` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | Focus rect ignores rotation, clipping rotated objects | `web/src/editor/GardenCanvas.tsx:78` | kimi-k2.6:cloud | correctness |
| 🟠 | Re-sync skips all inspector fields when any field has focus | `web/src/editor/Inspector.tsx:44` | kimi-k2.6:cloud | error-handling |
| 🟠 | Checkbox commits PATCH immediately onChange, rapid toggles self-conflict with version | `web/src/editor/Inspector.tsx:170` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | Duplicate circle-vs-rect shape rendering inside SelectionOverlay | `web/src/editor/SelectionOverlay.tsx:138` | opencode/kimi-k2.6:cloud | maintainability |
| 🟠 | Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess | `web/src/lib/objects.ts:159` | claude-code/sonnet | performance |
| 🟡 | 'web/src/editor/GardenCanvas.tsx:16-21' — per-kind default-z knowledge is split from 'kinds.ts'. | `kinds.ts:5` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | 'web/src/editor/Inspector.tsx:55-56' — 'patch' is typed against 'EditorObject', not the real 'ObjectPatch', so it accepts fields the server ignores. | `objects.ts:108` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | 'web/src/editor/Inspector.tsx:55-56' — 'patch' is typed against 'EditorObject', not the real 'ObjectPatch', so it accepts fields the server ignores. | `types.ts:8` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | Grid path collapsed from multi-line to single long line, hurting readability | `web/src/editor/GardenCanvas.tsx:129` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | Hand-rolled 'Fit' button duplicates the shared Button/buttonClasses ghost styling instead of reusing it | `web/src/editor/GardenCanvas.tsx:149` | claude-code/sonnet | maintainability |
| 🟡 | 'web/src/editor/Inspector.tsx:31-37' and ':44-53' — the field-init and re-sync logic duplicate the same 'String(displayFromCm(...))' / 'String(Math.round(...))' conversions. | `web/src/editor/Inspector.tsx:31` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | On delete failure the object is restored to the cache but selection was already cleared, leaving it unselected on retry | `web/src/editor/Inspector.tsx:191` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner. | `web/src/editor/SelectionOverlay.tsx:108` | glm-5.2:cloud | error-handling |
| 🟡 | Rotation shiftKey state locked at gesture start | `web/src/editor/SelectionOverlay.tsx:122` | kimi-k2.6:cloud | error-handling |
| 🟡 | All resize handles use the same nwse-resize cursor regardless of corner | `web/src/editor/SelectionOverlay.tsx:168` | opencode/kimi-k2.6:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Good — no `dangerouslySetInnerHTML`/`innerHTML`/`eval` anywhere, and the backend independently validates dimensions (`validDimensionCM`) and color format server-side, so a malicious/buggy frontend value can't corrupt stored data. That closes out my two candidate concerns (SVG `fill:url()` injection via unvalidated color, and unbounded width/height from the Inspector's raw number fields) — both are enforced server-side.
I've completed my review. No security issues survived verification.
**VERDICT: No material issues found**
This PR is entirely frontend (React/TypeScript) editor UI wired to an existing, already-validated API. I checked the areas most likely to matter for this lens:
- **Untrusted data rendering**: `object.name`/`notes` and toast messages are rendered as plain JSX text content (no `dangerouslySetInnerHTML`/`innerHTML` anywhere in `web/src` — verified via grep). React auto-escapes, so no XSS path.
- **Color field → SVG `fill`**: `ObjectShape.tsx` renders `object.color` directly as an SVG `fill` attribute unvalidated on the client, which raised a concern about CSS/SVG `url()`-based injection. Verified this is a non-issue: `internal/service/objects.go` (`validDimensionCM`) and the color-format check exercised by `internal/service/objects_test.go` ("bad color" test case) reject non-hex colors server-side, so arbitrary paint-server values can't reach storage even via a raw API call bypassing the `<input type="color">` UI.
- **Server response handling**: all API responses go through `zod` schemas (`serverObjectSchema`, `fullGardenSchema`) before use — malformed/unexpected server data fails parsing rather than being trusted blindly. `conflictObject()` correctly uses `safeParse` on the 409 body.
- **Auth/session**: unchanged — requests still use `credentials: 'same-origin'` (cookie-based), no tokens handled or logged client-side.
- **URL/search param handling**: `router.tsx`'s new `validateSearch` for `?focus=` coerces to a positive integer or drops it entirely (`{}`), so it can't be used to inject anything into the DOM or downstream calls; it's only used to look up a local object by numeric id.
- **Optimistic mutations**: `id`/`version` in PATCH bodies come from already-fetched, schema-validated objects, not raw user text — no injection surface there.
One thing outside my lens worth flagging for the correctness reviewer: `Inspector.tsx`'s `commitDim` has no lower-bound clamp (unlike `SelectionOverlay`'s `MIN_OBJ_CM`), so a user could type a zero/negative dimension — not a security issue since the server enforces `validDimensionCM`, but worth a UX-level fix.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
Both findings confirmed exactly as described, at the correct line numbers. Line numbers are 71-73 rather than 71-72 exactly, but close enough and the substance is accurate.
## VERDICT: Blocking issues found
- **`web/src/lib/objects.ts:130-137` + `:188-201` (`onMutate` / `applyServerPatch`) — optimistic update never advances the cached `version`, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit.**
`useUpdateObject`'s `onMutate` applies the patched fields to the cache via `applyServerPatch`, which copies over every field except `version` (objects.ts:188-201). The cached object therefore still carries the pre-mutation `version` after the optimistic write. `Inspector.tsx` builds each PATCH from `object.version` read off that same cache/prop, and each field (Name, Width, Height, X, Y, Rotation, Notes) has its own independent `onBlur` handler calling `update.mutate(...)` — nothing serializes these. Trace: user edits Name (blur → PATCH A, `version: V`), then edits Width before A returns (blur → PATCH B, also `version: V`, since the cache's version hasn't moved). Server processes A first, bumps to `V+1` (confirmed server-side version-conflict path at `internal/api/objects.go:144-149`, dispatching to `writeVersionConflict` on `domain.ErrVersionConflict`). B arrives with stale `version: V` and is rejected as a conflict. The client's `onError` path (objects.ts:145-158) then overwrites the cache with the server's `current` row (reflecting A's change but not B's) and shows "That object was updated elsewhere — your change was rolled back." The Width edit is silently discarded, and the user is told someone else changed the object, which isn't what happened. `SelectionOverlay.tsx` has the identical issue: `startMove`/`startResize`/`startRotate` all capture `base.version = object.version` at gesture start (line 88), so two gestures on the same object performed back-to-back before the first gesture's PATCH resolves (e.g., move immediately followed by resize) race the same way.
Fix direction: bump the cached `version` optimistically in `applyServerPatch` (e.g. `version: o.version + 1`), or serialize/queue mutations per object, or debounce/coalesce rapid field commits into a single PATCH.
- **`web/src/editor/GardenCanvas.tsx:70-73` — `?focus=<id>` fit-to-rect ignores the object's `rotationDeg`, so a rotated object can be framed with its corners clipped outside the viewport.**
The target rect is built straight from `focus.widthCm`/`focus.heightCm` as an axis-aligned box, but per `DESIGN.md:30` objects are positioned by center + `rotation_deg` clockwise about that center (`translate(x y) rotate(deg)`), so a rotated rect's true screen-space extent is larger than its unrotated width/height (up to ~1.41× at 45°). `zoomToFitRect` (`lib/geometry.ts:108-128`) only adds a fixed padding margin (`FIT_PADDING = 32`, `useViewport.ts:14`) that doesn't scale with rotation, so a meaningfully-rotated focused object can end up with corners outside the fitted viewport on load. Low impact today since this is explicitly "groundwork for #15", but it is a real geometric miscalculation given the stated rotation convention.
Fix direction: compute the rotated AABB (rotate all four corners, take min/max) before calling `fitToRect`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings verified against the actual source.
**VERDICT: Minor issues**
- `web/src/editor/GardenCanvas.tsx:149-155` — The "Fit" button was changed from `<Button variant="ghost">` to a raw `<button>` that hand-duplicates the `Button`/`buttonClasses` ghost styling (`border border-border`, `text-fg`, `hover:bg-border/50`, `outline-none focus-visible:ring-2 focus-visible:ring-accent/40`, `transition-colors` — confirmed identical to `web/src/components/ui/Button.tsx:9-16`). The `Button` import was dropped from this file entirely. The prior code already used `className` to add the position/backdrop overrides (`absolute bottom-3 right-3 bg-surface/90 py-1.5 shadow-sm backdrop-blur`), so the same result was achievable by keeping `<Button variant="ghost" className="...">` — there's no functional reason for the divergence, and `Inspector.tsx` continues to use `Button variant="ghost"` for equivalent ghost buttons (`Inspector.tsx:160,198,203`) in this same PR.
- `web/src/editor/Inspector.tsx:89-130` — The four dimension fields (Width/Height/X/Y) are copy-pasted `TextField` blocks that differ only by label, local state variable, and which `EditorObject` key they patch via `commitDim`. This could collapse to a small loop over a `[{key, label, get, set}]` table with one local-state map, removing the risk of the four blocks drifting (e.g. a `step`/`inputMode` divergence slipping in during a future edit).
- `web/src/lib/objects.ts:188-201` (`applyServerPatch`) — Ten near-identical `...(patch.X !== undefined ? { X: patch.X } : {})` lines, one per `ObjectPatch` field. Every time a field is added to `ObjectPatch` (`objects.ts:108-121`), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`web/src/lib/objects.ts:159` (and `177`)** — `useUpdateObject` and `useDeleteObject` both add `onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })` even though `onSuccess`/`onMutate` already write the authoritative result into the cache (`patchFullCache` at lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch of `GET /gardens/:id/full` after every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires one PATCH per field per blur (name, width, height, x, y, rotation, color, plantable, notes) and drags fire one PATCH per gesture, this doubles network work for every one of those interactions. `useCreateObject` (line 99-102) deliberately avoids this pattern ("avoids a full refetch flash"), reinforcing that the invalidate in update/delete is inconsistent with the rest of the file. Fix: drop the `onSettled` invalidate for update/delete.
- **`web/src/editor/SelectionOverlay.tsx:61`** — `pointerWorld()` (lines 60-65) calls `svgRef.current?.getBoundingClientRect()` on every `pointermove`, wired through `trackPointer`'s `window.addEventListener('pointermove', ...)` (lines 21-32) and invoked from `begin()`'s `onMove` (line 78). Each move also calls `setLiveObject` which re-renders the SVG subtree (confirmed via `GardenCanvas.tsx`'s `rendered` memo keyed on `liveObject`), so this is a read-after-write layout query on every drag frame. By contrast, `useViewport.ts`'s `clientToCanvas` (lines 37-43) does the same rect read only in `onWheel`/`onPinch` (lines 61, 67), not in `onDrag` (lines 48-56, which uses `delta` directly with no rect read) — confirming this is a hotter instance of the pattern than the codebase's existing convention. Fix: measure the rect once in `begin()`/gesture start and reuse it for the drag's duration.
- **`web/src/editor/GardenCanvas.tsx:82-85`** — `rendered` re-maps and re-sorts the full `objects` array on every `liveObject` change (every pointermove during a drag, per `SelectionOverlay`'s `setLiveObject` calls). Confirmed the `useMemo` dependency array is `[objects, liveObject]` (line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmed `GardenEditorPage.tsx:51` (`full.data.objects.map(toEditorObject)`) produces a new array identity on every render of the page, so the `objects` dependency itself never stabilizes across unrelated re-renders (e.g., a `selectedId` change causes `GardenEditorPage` to re-render, producing a new `objects` array, invalidating this memo too). Fix: memoize the sorted server-object list keyed only on `objects` content, and splice the live-dragged object's position into that stable order separately.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
Confirms no `pointerId` filtering or `setPointerCapture` anywhere in the frontend. All three findings in the draft are verified against the actual code.
## VERDICT: Blocking issues found
- **`web/src/lib/objects.ts:130-136,188-203` — Optimistic PATCH never bumps the cached `version`, so two edits to the same object issued in quick succession (before the first PATCH's response lands) cause a false 409 that silently discards the second, entirely-valid edit.** `applyServerPatch` (objects.ts:188-202) copies every patched field onto the cached row except `version`. `Inspector.patch()` (Inspector.tsx:55-56) reads `object.version` straight from that cache, so a second rapid mutation on the same object is sent with the stale version. Verified server-side (`internal/store/objects.go:93-118`): the UPDATE is `WHERE id = ? AND version = ?` with `version = version + 1`, so the first PATCH bumps the row's version and the second is genuinely rejected as `ErrVersionConflict` even though only one client touched it. `useUpdateObject`'s `onError` (objects.ts:145-158) can't distinguish this from a real two-client conflict — it adopts the server's `current` row (discarding the second field's change) and shows "That object was updated elsewhere — your change was rolled back," which is false here.
Fix: bump the cached version optimistically in `applyServerPatch` (or serialize/queue per-object mutations while one is in flight).
- **`web/src/editor/Inspector.tsx:58-63` — Width/Height/X/Y fields accept zero, negative, or absurdly large values with no client-side validation, unlike the drag-resize path.** `commitDim` only checks `Number.isFinite(v)`; it never calls `isValidDimensionCm` (`web/src/lib/units.ts:16-18`, bounds `[1, 10_000]` cm). Server-side, `finalizeObject` (`internal/service/objects.go:234`) rejects out-of-range width/height via `validDimensionCM` with a 400 (not 409). Since `ObjectShape.tsx:47-48` clamps rendered half-dimensions to `Math.max(0, …)`, entering e.g. `0` optimistically makes the object vanish from the canvas, then the 400 triggers the generic rollback path (`objects.ts:154-157`) restoring the old value with a "Could not save that change" toast — a confusing flash-then-revert instead of an upfront rejection/clamp. (Note: for X/Y specifically the server's actual bound is a garden-overlap check, `bboxOverlapsGarden`, not the `isValidDimensionCm` range — but the same flash-then-revert failure mode applies there too via a 400.)
Fix: validate with `isValidDimensionCm` for width/height (and an appropriate bound for X/Y) in `commitDim` before calling `patch(...)`.
- **`web/src/editor/SelectionOverlay.tsx:19-30` — `trackPointer`'s window-level `pointermove`/`pointerup` listeners don't filter by `event.pointerId`, and no handle calls `setPointerCapture`.** Confirmed by code inspection: no `pointerId` or `setPointerCapture` usage anywhere under `web/src/editor/` (or the rest of `web/src/`). On a multi-touch device, a second finger touching the canvas while a move/resize/rotate gesture from the first finger is active will have its pointermove events processed by the same active gesture handler, corrupting the geometry computation.
Fix: capture the pointer on gesture start and filter the window listeners by `event.pointerId`.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the security lens I traced the client-supplied data to its server-side enforcement and found the change is adequately guarded:
- **Authz**: every mutating endpoint the editor calls (`POST /gardens/:id/objects`, `PATCH /objects/:id`, `DELETE /objects/:id`, `GET /gardens/:id/full`) is behind `requireAuth` + a `requireGardenRole`/`objectForRole(roleEditor/roleViewer)` check in `internal/service/objects.go` (verified `CreateObject`/`UpdateObject`/`DeleteObject`/`GardenFull`). The actor is resolved from the HttpOnly session cookie (`api.ts:97` `credentials: 'same-origin'`), not from any client-supplied id. A user can only touch objects in gardens they own/edit; cross-user access returns NotFound/Unauthorized (per `objects_test.go` Bob-vs-Alice cases).
- **Input validation**: `kind`, `shape`, dimensions, and `color` are all validated in `finalizeObject` (`internal/service/objects.go:220+`): unknown kinds rejected, `isHexColor` enforced, name length capped, out-of-bounds dimensions rejected. So the optimistic `create.mutate({kind: def.kind, ...})` and inspector color/`null`-clear paths are server-validated regardless of client trust. The client's `MIN_OBJ_CM` clamp and `#rrggbb` color input are UX only; the server is the authority.
- **XSS / rendering**: toast messages (`toast.tsx`) and inspector fields (`name`, `notes`) are rendered as React text children — no `dangerouslySetInnerHTML` anywhere in the diff or `ObjectShape.tsx` (verified). `err.message`/conflict text surfaced in toasts originates from server-controlled `ApiError` bodies, not user input.
- **`?focus=<id>`** (`router.tsx`): validated as a positive integer and only used to frame the viewport — it does not authorize fetching; `GardenFull` returns only the objects the viewer is entitled to see, so a guessed id can't reveal anything.
- **Conflict adoption** (`conflictObject` in `objects.ts`): the 409 `body.current` is server-asserted and re-parsed through `serverObjectSchema`; it overwrites the cache with the server row, which is the correct trust direction.
No injection (SQL/command/path), SSRF, unsafe deserialization, secret leakage, missing validation, or insecure defaults in this change.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
All three findings verify against the actual source. Confirming each:
**Finding 1** — `commitDim` (Inspector.tsx:58-63) compares `cmFromDisplay(v, unit)` to `current` with no dirty guard. The `width`/`height`/`x`/`y` state is initialized from `String(displayFromCm(...))` and re-synced the same way (lines 33-36, 48-51). Re-deriving the imperial round-trip: 100 cm → `displayFromCm` = `round(100/2.54/12 * 10)/10` = 3.3 → `cmFromDisplay(3.3,'imperial')` = `round(3.3*12*2.54)` = 101, so `101 !== 100` fires `patch({widthCm:101})` on a no-op blur. Metric drifts for non-integer cm (a free resize yields floats; e.g. 123.4 cm → "1.23" → 123). Rotation `onBlur` (line ~135) compares `v !== object.rotationDeg` against the raw value while the field shows `Math.round(...)`, so 30.5° → "31" → commits 31. Confirmed.
**Finding 2** — `useUpdateObject.onMutate` (objects.ts:130-138) calls `applyServerPatch` (188-202) which does not touch `version`; `onSuccess` (139-144) replaces the row with the server row (bumped version). `SelectionOverlay` captures `const base = { ...object }` (line 88) and PATCHes `version: base.version` on release (lines 96/118/132). No `update.isPending` guard exists in `SelectionOverlay` or `Inspector`. A second gesture begun before the first PATCH resolves reads the still-old cache `version`, sends a stale version, and 409s → `onError` adopts the server row and toasts "rolled back." Confirmed.
**Finding 3** — `startRotate`'s `onMove` closure (SelectionOverlay.tsx:125-130) references `e.shiftKey` from the pointerdown `e`, while `trackPointer` invokes `onMove(pointerWorld(ev))` with the live `ev` not passed in. So Shift must be held at pointerdown; pressing/releasing mid-drag has no effect, contradicting the "free with Shift" comment. Confirmed.
VERDICT: Minor issues
- **`web/src/editor/Inspector.tsx:62` — spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.** `commitDim` runs on every blur and compares `cmFromDisplay(v, unit)` to `current` with no dirty guard. Because `displayFromCm` rounds (imperial → 0.1 ft, metric → 2 dp cm) and `cmFromDisplay` re-quantizes to whole cm, the round trip is lossy. Verified: 100 cm → `displayFromCm` gives `3.3` ft → `cmFromDisplay(3.3,'imperial')` = `round(3.3*12*2.54)` = **101 cm**, so on an imperial garden focusing a Width/Height/X/Y field and blurring without typing fires `patch({ widthCm: 101 })`, silently mutating the object by 1 cm and bumping its version. Metric drifts too for non-integer cm (a free resize produces floats, e.g. 123.4 cm → `"1.23"` → `123`). The Rotation `onBlur` has the same shape: state is `String(Math.round(object.rotationDeg))`, so a free rotation of 30.5° displays `"31"` and a focus/blur commits `31`. Fix: guard no-op blurs by comparing the raw string to `String(displayFromCm(current, unit))` (or track a dirty flag set in `onChange`); for rotation compare against the rounded current rather than raw `object.rotationDeg`.
- **`web/src/lib/objects.ts:130` — back-to-back gestures self-conflict on version.** `onMutate` applies the patch via `applyServerPatch` but does **not** bump `version`; the version only advances on `onSuccess` (server row). `SelectionOverlay` reads `object.version` from the cache at gesture start (`base.version`) and PATCHes with it on release. If a second gesture begins before the first PATCH resolves, `base.version` is still the pre-edit version, so the second PATCH carries a stale version; once the first PATCH lands (server `version+1`), the second request 409s and `onError` adopts the server row + toasts "rolled back" — even though no other client touched it. There is no `update.isPending` guard in `SelectionOverlay` or `Inspector`. Fix: optimistically increment `version` in `onMutate` (reconcile on `onSuccess`/`onError`), or disable further gestures while a mutation for that object id is pending.
- **`web/src/editor/SelectionOverlay.tsx:128` — Shift state captured at gesture start, not tracked during the drag.** The `onMove` closure in `startRotate` references `e.shiftKey` from the original `pointerdown` event (`if (!e.shiftKey) deg = Math.round(...)`). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (`ev.shiftKey`) inside `onMove` instead of from `e`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Review
**Verdict: Minor issues**
Through the code-cleanliness & maintainability lens, I verified each finding against the checked-out source. All five survive independent confirmation:
- **`web/src/editor/SelectionOverlay.tsx:57-58` — two sources of truth for scale.** `scale = useEditorStore((s) => s.viewport.scale)` (line 52) feeds the render-time `handleCm`/`rotateOffsetCm` (lines 57-58), while `pointerWorld` (lines 60-65) re-reads `useEditorStore.getState().viewport` on every move. The split is functionally intentional (scale can change mid-gesture), but with no comment a reader can't tell that `handleCm` is hit-size-only. Minor readability footgun — consider a clarifying comment.
- **`web/src/lib/objects.ts:188-201` — `applyServerPatch` is a verbose per-field conditional spread.** Lines 189-201 repeat `...(patch.x !== undefined ? { x: ... } : {})` ten times, one per `ObjectPatch` field. A generic `Object.entries(patch)` filter-and-merge (excluding `id`/`version`) would be ~3 lines and wouldn't drift from `ObjectPatch` as fields are added.
- **`web/src/editor/GardenCanvas.tsx:81-85` — `liveObject` merge silently no-ops on id mismatch.** Line 83 `objects.map((o) => (o.id === liveObject.id ? liveObject : o))` does nothing if the live object's id isn't in `objects` (e.g. a delete racing a drag). The merge reads as if `liveObject` always corresponds to a cached row; a one-line comment (or an early return) would make the intent explicit.
- **`web/src/editor/GardenCanvas.tsx:16-21` — kind z-index defaults split from kind metadata.** `UNDER_KINDS`/`defaultZForKind` encode per-kind z-ordering policy here, while `kinds.ts` (lines 5-22) already owns per-kind default sizes/labels/shape. The "paths/structures under beds, trees above" ordering that the render sort (`a.zIndex - b.zIndex`, line 84) relies on is placement-time policy split across two files. Low-impact leaky split of kind metadata.
- **`web/src/components/ui/toast.tsx:32` — `ToastItem` prop `toast` shadows the imported/exported `toast` helper.** The component parameter `{ toast }` (line 32) shadows the module-level `export const toast = {...}` (lines 27-30) within the component body. It works (the component only reads `toast.id`/`toast.tone`/`toast.message`), but shadowing a same-file exported symbol is a readability trap — rename the prop to `data` or `t`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens I checked the hot paths (per-pointermove drag updates, pan/zoom re-renders, mutation cache writes) and found no material regression:
- **`SelectionOverlay`/`GardenCanvas` drag loop**: each `pointermove` does one `setLiveObject` → one `GardenCanvas` re-render. Work per frame is bounded: a single-object merge (`objects.map` with one replacement) + one `[...merged].sort()`. The sort is O(n log n) worst case but V8's Timsort is adaptive for the nearly-sorted z-order that changes by one element, so it's effectively ~O(n) per frame. No N+1, no per-element queries. Verified at `GardenCanvas.tsx:82-85` and `SelectionOverlay.tsx:77-84`.
- **`getBoundingClientRect()`** is called per pointer event (`SelectionOverlay.tsx:61`, `GardenCanvas.tsx:99`); it's a cheap layout read, not a forced reflow beyond what's needed for coordinate conversion. Fine on the hot path.
- **Mutations** (`useUpdateObject`/`useDeleteObject`) rewrite the cache with `objects.map(...)` — O(n) per mutation, one per gesture end, not inside the drag loop. Verified at `objects.ts:130-144`.
- **Toaster**: `nextId` module counter + array filtered on 4s timeout dismiss; no unbounded growth. Verified at `toast.tsx:18-24`.
One trivial, non-material observation (not worth blocking on): `GardenCanvas.tsx:86` recomputes `selected = rendered.find(...)` on every render, and `GardenCanvas` re-renders on every viewport change (pan/zoom) and every liveObject update. That's an extra O(n) scan per frame on top of the sort. For a v1 garden (dozens of objects) this is negligible; if gardens ever hold thousands of objects it's a trivial fix (derive `selected` from `selectedId` against `objects` directly, or memoize). I'm noting it only because it's genuinely in the hot render path; impact is not material at current scale.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both surviving findings are confirmed against the actual code:
1. **Color input PATCH on every onChange** — `Inspector.tsx:155` calls `patch({ color: e.target.value })` directly in `onChange`, which fires continuously during a color-slider drag. Unlike geometry fields (which commit on `onBlur`), this fires a PATCH per intermediate value, all with the same `object.version`, so concurrent edits can race and trigger 409/rollback. Confirmed.
2. **Resize center drift past corner** — `SelectionOverlay.tsx:108-115`: `newW`/`newH` are clamped to `MIN_OBJ_CM`, but `newCenterLocal` and thus `c` keep recomputing from `p` (the dragged pointer position) even when the pointer crosses the fixed opposite corner. So the center follows the pointer while size is pinned at minimum — a visible UX edge. Confirmed.
The other draft items (lost-update on Inspector version, click-without-move no-op, GardenCanvas armed-kind consumption, rotate Shift freeze, focus-on-deleted-object) were already self-withdrawn in the draft or are non-issues, so they are dropped.
VERDICT: Minor issues
- **`web/src/editor/Inspector.tsx:155` — color `onChange` fires a PATCH per pixel of a slider drag.** The `<input type="color">` commits via `onChange` directly into `patch({ color })`, unlike the geometry fields (one PATCH per gesture / on blur). Each intermediate value carries the same `object.version`, so rapid changes race; a 409 mid-drag rolls back to the server value and toasts, discarding the user's in-progress selection. Minor — a real unhappy path but bounded.
- **`web/src/editor/SelectionOverlay.tsx:108` — resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.** `newW`/`newH` are clamped to `MIN_OBJ_CM`, but `newCenterLocal` is still derived from the live pointer position `p`, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.
</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**
Through the security lens, I scrutinized the optimistic sync layer (`lib/objects.ts`), the gesture-based canvas interactions (`editor/SelectionOverlay.tsx`, `editor/GardenCanvas.tsx`), the inspector form (`editor/Inspector.tsx`), routing (`router.tsx`), and the toast system (`components/ui/toast.tsx`).
**What I checked:**
- **Authn/Authz**: `gardenEditorRoute` gates on `requireAuth`; the backend is responsible for garden-scoped authorization on `/gardens/:id/full` and object mutations.
- **Injection**: API paths are template-literal assembled from validated numeric IDs (`object.id`, `gardenId`). No user-controlled strings reach URL paths. No SQL/command/template injection surfaces.
- **XSS**: `object.name` is rendered as a React text child in SVG `<text>` — automatically escaped, no `dangerouslySetInnerHTML`. Toast messages are hardcoded strings. No inline event handlers or HTML injection vectors.
- **Open redirect**: The `?redirect=` parameter on `/login` is funneled through `safeRedirectPath`, which rejects protocol-relative and absolute URLs. The new `?focus=` parameter is validated to a positive integer and only used to look up an object by ID; it never drives navigation.
- **Optimistic locking**: `useUpdateObject` requires `version` and correctly handles 409 conflicts by rolling back to the server's row. This is a security-positive pattern that prevents lost-update races.
- **Input validation**: `serverObjectSchema` (Zod) validates all API responses. `kindDef` restricts created kinds to the known allow-list. Color inputs are constrained to `<input type="color">`.
No authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, or insecure defaults were identified.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**Blocking issues found**
- `web/src/editor/SelectionOverlay.tsx:88` — `base = { ...object }` snapshots the object's `version` at render time. The gesture-end PATCH sends `base.version`, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict *during* the drag. The fix is to read the current cached row's `version` when building the patch on release, or to derive it from the latest `liveObject` / query data rather than the stale closure.
- `web/src/editor/GardenCanvas.tsx:74-82` — The `fittedForRef.current !== garden.id` guard means the canvas frames the garden (or a `?focus=` object) exactly once per garden mount. If the `focusId` search param changes client-side without remounting the page, the viewport does **not** re-fit to the new object because the guard only tracks `garden.id`, not the focus target. Track the last focused id and re-frame when it changes.
- `web/src/editor/GardenCanvas.tsx:78-80` — The focus rectangle for `?focus=<id>` treats the object's bounds as axis-aligned: `{x: focus.xCm - focus.widthCm/2, y: focus.yCm - focus.heightCm/2, w: focus.widthCm, h: focus.heightCm}`. If the object has a non-zero `rotationDeg`, its axis-aligned bounding box is larger than `widthCm × heightCm` (by up to a factor of `|cos|+|sin|`). `zoomToFitRect` will clip the rotated corners. Compute the rotated AABB or at least pad the rect.
- `web/src/editor/SelectionOverlay.tsx:122-134` — In `startRotate`, `e.shiftKey` is evaluated from the initial `ReactPointerEvent` captured in the closure. Because the `onMove` callback only receives the world `Point` (not the current native `PointerEvent`), holding or releasing Shift mid-gesture has no effect; snap/free mode is locked at pointer-down. Pass the current event's modifier state into the move callback so Shift can be toggled live during the drag.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
- I read `Inspector.tsx`, `SelectionOverlay.tsx`, `ObjectShape.tsx`, `lib/objects.ts`, `useViewport.ts`, `editor/store.ts`, and the new `components/ui/toast.tsx`. The diff is well-factored and follows the repo's existing patterns.
- No dead or duplicated code: the new modules each have a single responsibility, and shared utilities (`lib/geometry.ts`, `cn()`, `Button`, `TextField`) are reused rather than copy-pasted.
- No confusing or misleading names: store fields like `liveObject`, `armedKind`, and `objectDragging` are self-describing; gesture helpers in `SelectionOverlay` (`startMove`, `startResize`, `startRotate`) are concise and accurate.
- No overly long or deeply nested functions: the heaviest logic (resize/rotate geometry) is delegated to `localToWorld`/`worldToLocal` in `lib/geometry.ts`; `applyServerPatch` is a flat sequence of conditional spreads, which is appropriate.
- Follows existing patterns: `toast.tsx` uses the same `cn()` utility as the rest of the UI, `Inspector` uses the same `TextField`/`TextArea` primitives, and the react-query mutations follow the same optimistic-update structure already in the codebase.
- No leaky abstractions: the object-local frame math stays in `lib/geometry.ts`, and the canvas doesn't leak SVG details into the store or data layer.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
- **`web/src/pages/GardenEditorPage.tsx:51`** — `const objects = full.data.objects.map(toEditorObject)` is unmemoized. `toEditorObject` creates a new `EditorObject` instance for every server object on every parent render. This defeats the `React.memo` on `ObjectShape`, forcing all objects to re-render whenever `GardenEditorPage` re-renders (e.g., on `selectedId` change or background refetch). **Fix:** wrap the mapping in `useMemo(() => full.data.objects.map(toEditorObject), [full.data.objects])`.
- **`web/src/editor/GardenCanvas.tsx:82-85`** — During move/resize/rotate gestures, `liveObject` updates on every `pointermove` frame, yet the `rendered` memo performs an O(n) `.map()` copy plus O(n log n) `.sort()` on each frame to merge a single changed object. For gardens with many objects this creates jank and allocation pressure at interaction frequency even though z-order is unchanged during a single-object gesture. **Fix:** keep a stable z-sorted array and only re-sort when z-indices change, or split the liveObject merge from the sort and only sort when `objects` itself changes.
- **`web/src/editor/Inspector.tsx:154-155`** — The color `<input>` fires `patch({ color: e.target.value })` on every `onChange` event. A native color picker emits change events continuously while dragging, generating a burst of optimistic cache patches, query cancellations, and network PATCHes. **Fix:** debounce the color mutation (e.g., 200 ms) or use local state for the live preview and commit on blur.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/editor/SelectionOverlay.tsx:21-31`** — `trackPointer` attaches `window` listeners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still on `window`. On `pointerup` the `onEnd` closure fires, calling `setObjectDragging(false)` and `update.mutate(fields(final))` against the *old* garden's mutation hook, potentially PATCHing the wrong context or a deleted object. Fix: store the `end` cleanup in a ref and invoke it in a `useEffect` cleanup, or use a `useEffect`-based listener pattern so React tears the listeners down on unmount.
- **`web/src/editor/Inspector.tsx:58-63`** — `commitDim` parses the raw input with `parseFloat` and sends the result without validating that it is positive or at least `MIN_OBJ_CM`. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (`MIN_OBJ_CM = 1`). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: reject `v <= 0` (or `cm <= 0`) in `commitDim` and surface a brief inline warning instead of calling `apply`.
**Minor issues**
- **`web/src/editor/Inspector.tsx:44-53`** — The re-sync `useEffect` aborts entirely when *any* field inside the inspector has focus (`rootRef.current?.contains(document.activeElement)`). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.
- **`web/src/editor/Inspector.tsx:151-157`** — The color `<input>` uses `onChange`, committing a PATCH on every mouse/touch adjustment inside the color picker. Because each PATCH carries the current `version`, rapid changes can launch overlapping requests; the first succeeds and bumps the version, causing subsequent in-flight requests to 409 and roll back. Fix: commit color on `onBlur` (or debounce with a short trailing edge) so the gesture completes before versioning is contested.
- **`web/src/editor/SelectionOverlay.tsx:122-134`** — `startRotate` captures `e.shiftKey` from the initial `ReactPointerEvent` closure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: read `shiftKey` from the live `PointerEvent` passed to the move handler instead of the stale closed-over React event.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
I traced tainted input through every sink this PR introduces, and checked the actual checked-out code (not just the diff) for each path below:
- **Server → UI text sinks**: `object.name`, `object.notes`, `err.message`, and `kindDef(object.kind)?.label ?? object.kind` are all rendered as React text children (`<text>`, `<h2>`, `<TextField value>`, toast `{toast.message}`). No `dangerouslySetInnerHTML`/`innerHTML`/`eval` exists anywhere in `web/src` (grep confirmed). React escapes text content, so server-controlled strings can't XSS. Verified in `ObjectShape.tsx:95-107`, `Inspector.tsx:70`, `toast.tsx:42-44`.
- **Server response parsing**: `serverObjectSchema.parse` / `fullGardenSchema.parse` (zod) gate every server payload entering the cache, including the optimistic `onSuccess` splices and the 409 conflict body via `conflictObject`'s `safeParse` (`objects.ts:206-211`). The 409 path reads `err.body.current` as `unknown` and only adopts it if `safeParse` succeeds — no trust of raw body shape.
- **URL-derived `?focus=<id>`**: `router.tsx:93-96` coerces with `Number(search.focus)` and gates on `Number.isInteger(f) && f > 0`. Arrays/objects/strings like `"1; ..."` become `NaN` and are rejected (returns `{}`). The surviving integer is only used as `objects.find(o => o.id === focusId)` (`GardenCanvas.tsx:70`) — a numeric lookup against this garden's already-validated object list, never interpolated into a URL or attribute. If the id isn't found, `target` falls back to `gardenRect`. No injection / IDOR surface introduced here.
- **URL path interpolation** in new mutations (`objects.ts:70,98,127,166`): `/gardens/${gardenId}/...` and `/objects/${id}` use `gid = Number(gardenId)` (route param) and `object.id`/`patch.id` (numbers off zod-validated server rows or local numeric state). `Number()` of a non-numeric route segment would yield `NaN`, which the server would reject — not an injection vector. No user-controlled string reaches a path segment.
- **SVG `fill`/`stroke` attributes**: `object.color` (server-controlled, schema `z.string().nullable().optional()`) flows into `fill={fill}` (`ObjectShape.tsx:74,88`) and the color input's `value` (`Inspector.tsx:154`). React sets SVG attributes via `setAttribute` with string values — attribute-value quoting prevents breakout, so a value like `" onmouseover=...` can't inject a handler. The `<input type="color">` itself coerces its value to a valid `#rrggbb` or `#000000`. No scripting surface.
- **Mutation request bodies**: `ObjectCreate`/`ObjectPatch` are typed objects built from local form/geometry state, serialized via `JSON.stringify` in `api.ts:90`. No template/SQL/string-concat assembly; no injection.
- **Auth**: All new requests go through `apiFetch`, which sends `credentials: 'same-origin'` and is gated by `requireAuth` in `beforeLoad` on `gardenEditorRoute` (`router.tsx`). No unauthenticated mutation paths were added.
- **CSRF**: Cookie-auth with no CSRF token is a pre-existing property of `api.ts`, not something this PR introduces or worsens; out of this PR's scope and not a regression.
Nothing in the security lens is materially wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
All three findings confirmed against the actual source. Verifying line numbers and the mutation/conflict path.
All three findings are confirmed against the actual code. Correcting the line numbers: the color `onChange` is at line 155 (not 131), the imperial round-trip drift is in `commitDim` at line 62 (not 73), and `e.shiftKey` is at line 128 (not 142).
## Corrected review
**VERDICT: Minor issues**
- **`web/src/editor/Inspector.tsx:155` — color `onChange` fires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks.** The native `<input type="color">` emits `input` events continuously while the user drags through the OS color picker; React's `onChange` maps to that `input` event. Each tick calls `patch({ color })`, sending the *current* `object.version`. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new version still carries version N → the server returns 409 → `conflictObject` adopts the server row and toasts "That object was updated elsewhere — your change was rolled back." The user's color reverts with a misleading conflict toast, even though they were the only editor. Every other field commits on `blur`; color is the lone exception. Fix: commit color on `onBlur` (capture the pending value in local state via `onChange`, dispatch the PATCH on blur) — or debounce. Verified against the Inspector and the `useUpdateObject` 409 path (`web/src/lib/objects.ts:145-158`).
- **`web/src/editor/Inspector.tsx:62` — imperial unit round-trip drift can fire a spurious PATCH (and 1 cm change) on blur-without-editing.** `displayFromCm(240, 'imperial')` → `7.9` ft; if the user focuses a dimension/position field and blurs without typing, `commitDim` runs `cmFromDisplay(7.9, 'imperial')` = `Math.round(7.9 * 12 * 2.54)` = `241`, which differs from the current `240`, so it PATCHes a 1 cm change the user never made. Metric round-trips exactly (so this only bites imperial gardens). Verified against `web/src/lib/units.ts:31-44`. Fix: skip the PATCH when the parsed display value equals the field's current display value, or compare on the rounded display value rather than raw cm.
- **`web/src/editor/SelectionOverlay.tsx:128` — rotate uses `e.shiftKey` from the pointerdown event, not the live pointermove.** `e` is the `ReactPointerEvent` captured in the `startRotate` closure at pointerdown, so pressing/releasing Shift *during* the drag does not toggle free-rotate vs. snap. The spec ("free with Shift", per the component doc comment on line 38) implies live Shift state. Minor UX/correctness gap. Fix: read `ev.shiftKey` inside the `onMove` callback (the `trackPointer` `PointerEvent` carries it).
Verified-clean (no issue): resize opposite-corner math in the object-local frame (`worldToLocal`/`localToWorld` are true inverses); rotate `+90°` offset matches the knob's local `-y` placement; `?focus=<id>` fit rect (center→top-left conversion); z-order merge with `liveObject`; conflict adoption on 409; `serverObjectSchema` matches the server's `GardenObject` JSON tags; `color: null` PATCH path; delete needs no version.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All five findings verified against the actual source.
**VERDICT: Minor issues**
- **`web/src/lib/objects.ts:188-202` — `applyServerPatch` repeats the same conditional-spread idiom for all 10 patchable fields.** Each `ObjectPatch` field is hand-mirrored here, so adding a new patchable field means editing both the interface (108-121) and this helper in lockstep, with nothing enforcing they stay in sync. A generic `Object.entries(patch).filter(([k]) => k !== 'id' && k !== 'version')` fold would collapse the body and make it field-agnostic. Verified by reading the file.
- **`web/src/editor/Inspector.tsx:55-56` — `patch` is typed against `EditorObject`, not the real `ObjectPatch`, so it accepts fields the server ignores.** `patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) => update.mutate({ id, version, ...fields })`. `EditorObject` (`types.ts:8-23`) includes `kind` and `shape`, but `ObjectPatch` (`objects.ts:108-121`) has neither. Because the fields are spread into the mutate argument (excess-property checks don't fire on spreads), `patch({ kind: 'tree' })` or `patch({ shape: 'circle' })` typechecks cleanly yet is silently dropped by the server. Typing the arg as `Omit<ObjectPatch, 'id' | 'version'>` would let the compiler catch these. Verified across `Inspector.tsx`, `types.ts`, and `objects.ts`.
- **`web/src/editor/GardenCanvas.tsx:16-21` — per-kind default-z knowledge is split from `kinds.ts`.** `UNDER_KINDS` / `defaultZForKind` encode per-kind layering metadata that belongs in `KindDef` (`kinds.ts:5-12`), which already holds each kind's label/icon/shape/default size. Adding a new kind means editing `kinds.ts` *and* remembering to update `UNDER_KINDS` here. A `defaultZ` (or `layer: 'under' | 'default' | 'over'`) field on `KindDef` would centralize it and let `GardenCanvas` read `kindDef(kind).defaultZ`. Verified by reading both files.
- **`web/src/editor/Inspector.tsx:31-37` and `:44-53` — the field-init and re-sync logic duplicate the same `String(displayFromCm(...))` / `String(Math.round(...))` conversions.** Seven `useState` initializers and the `useEffect` both spell out the same per-field formatting. A small `fieldsFromObject(object, unit)` helper returning `{ name, notes, width, height, x, y, rotation }` would remove the duplication and make a future field a one-line add in both places. Trivial.
- **`web/src/components/ui/toast.tsx:32` — the `toast` prop name shadows the module's `toast` export.** `function ToastItem({ toast })` shadows the `toast` object exported at line 27. Not a bug (the component doesn't use the export), but a reader scanning the file sees `toast.id`/`toast.message` and has to disambiguate which `toast` is meant. Renaming the prop to `data` or `item` (or the export to `toastApi`) would remove the ambiguity. Trivial.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All three findings confirmed against actual code.
All three findings confirmed against the actual code. The store confirms `setLiveObject` is called per pointermove (line 78 of SelectionOverlay), `liveObject` is a dep of the `rendered` useMemo (line 85), and the page-level `.map(toEditorObject)` runs every render (line 51).
**VERDICT: Minor issues**
- **`web/src/editor/SelectionOverlay.tsx:60-65` — `getBoundingClientRect()` called on every `pointermove` during a drag.** `pointerWorld` reads `svgRef.current?.getBoundingClientRect()` and is invoked from the `trackPointer` move callback (line 78) on every pointer event. `getBoundingClientRect` forces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the live `setLiveObject` re-render is also mutating the SVG, so the layout is dirty. This is the textbook layout-thrash pattern and the most likely cause of jank on lower-end devices, especially with touch. Fix: capture the rect once at gesture start (in `begin`) and pass it into `pointerWorld`, or cache it on the `base` snapshot. The viewport `tx/ty/scale` are already read fresh from the store via `useEditorStore.getState().viewport`, so the rect (which only changes on layout/scroll) is the only thing that needs caching.
- **`web/src/editor/GardenCanvas.tsx:82-85` — re-sort on every pointermove during a gesture.** `rendered` depends on `liveObject` (line 85), which changes on every `pointermove` of a move/resize/rotate via `setLiveObject`, so `[...merged].sort((a, b) => a.zIndex - b.zIndex)` runs O(n log n) (plus a fresh array + an `objects.map` merge) on every move event. Z-order does not change during a geometry gesture, so this is wasted work. For typical gardens it's negligible, but it scales with object count and event rate. Suggested fix: split the merge from the sort, and only sort when `objects` changes (e.g. sort once with `useMemo([objects])`, then merge `liveObject` into the already-ordered list).
- **`web/src/pages/GardenEditorPage.tsx:51` — `objects` array rebuilt with new identities every render.** `full.data.objects.map(toEditorObject)` produces a fresh array of fresh object literals on every render of `GardenEditorPage`. `ObjectShape` is `memo`'d (verified at `ObjectShape.tsx:37`), but the memo is defeated because each render hands it a brand-new `object` reference. This means every parent re-render (selection change, query invalidation after a PATCH's `onSettled`, focus param change) re-renders every object, even unchanged ones. It's not on the pan/zoom hot path (pan only re-renders `GardenCanvas`, not the page), so impact is limited, but after each gesture-release PATCH the `onSettled` invalidation → refetch → full remap → all objects re-render. Consider memoizing the mapping (`useMemo` on `full.data.objects`) or mapping server objects to editor objects once when the query returns.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified all four findings against the actual source. All confirmed.
**Verdict: Minor issues**
- **`web/src/editor/Inspector.tsx:155` — color picker fires a PATCH on every `onChange`, storming the server with stale-version 409s.** `<input type="color">` emits `onChange` continuously while the user drags in the native picker. Each call invokes `patch({ color: e.target.value })` (Inspector.tsx:55-56), which reads `object.version` from the prop. The optimistic `applyServerPatch` (objects.ts:188-202) overrides only the patched field and never touches `version`; `onSuccess` (objects.ts:139-144) only replaces the row after the network round trip. So the 2nd..Nth PATCHes in a single color drag all carry the same stale `version` as the first. The server accepts PATCH1 (vN→vN+1) and returns 409 for PATCH2..N. `useUpdateObject.onError` (objects.ts:145-157) treats every 409 as a conflict: it adopts `current` and toasts `'That object was updated elsewhere — your change was rolled back.'`. Net effect: a single color drag spams conflict toasts and cache churn even though the user is the only one editing. Fix: debounce the color commit, or use local state + commit-on-blur like the other fields. Verified against `Inspector.tsx:155`, `objects.ts:188` (`applyServerPatch` never sets `version`), `objects.ts:145-153`, and `api.ts:25-27` (`isConflict` = status 409).
- **`web/src/editor/SelectionOverlay.tsx:21-32, 77-86 — window pointer listeners leak if the overlay unmounts mid-gesture; `objectDragging` can get stuck.** `trackPointer` registers `pointermove`/`pointerup`/`pointercancel` on `window` and removes them only from inside `end`. There is no `useEffect` cleanup in `SelectionOverlay`. If the overlay unmounts during a drag — e.g. the selected object is deleted out-of-band so `selected` becomes `null` in `GardenCanvas.tsx:86` and the overlay is removed at line 142 — the `window` listeners remain registered. When the pointer is finally released, `onEnd` runs `update.mutate(...)` against a now-stale `update` (PATCH on a deleted/unknown object → 404 → "Could not save that change." toast), and `setObjectDragging(false)` / `setLiveObject(null)` run on the global store (still safe, since the store is module-level). More importantly, if the gesture's `pointerup` never fires (rare iframe/pointer-capture edge cases), `objectDragging` stays `true` and `useViewport`'s pan (useViewport.ts:50) is permanently disabled. Fix: register the cleanup in a `useEffect` whose cleanup also removes the listeners and resets `objectDragging`/`liveObject`. Verified by reading `SelectionOverlay.tsx:20-86`, `GardenCanvas.tsx:86,142`, and `useViewport.ts:50`.
- **`web/src/editor/Inspector.tsx:58-63 — `commitDim` accepts 0/negative values, briefly corrupting the optimistic cache.** `commitDim` only checks `Number.isFinite(v)`; typing `0` or `-5` for width/height passes the guard, `cmFromDisplay(v, unit)` (units.ts:31-33) rounds to `0`/negative cm, and `onMutate` (objects.ts:130-138) applies that to the cache. The render in `GardenCanvas` then draws the object with a zero/negative width (the `<rect>` `width={object.widthCm}` flips; the overlay's `halfW = object.widthCm/2` goes negative). The server rejects via `validDimensionCM` (gardens.go:170-172, `v >= minGardenCM` where `minGardenCM = 1`), `onError` rolls back to `ctx.prev`, and the field re-syncs. Net effect is a brief visual glitch and a wasted network round trip — not data loss, but an avoidable unhappy path. Suggested fix: clamp the parsed value to `>= MIN_DIMENSION_CM` (units.ts:12) before patching, matching the gesture path's `MIN_OBJ_CM` clamp in `SelectionOverlay.tsx:108`. Verified by reading `Inspector.tsx:58-63`, `units.ts:31`, and `gardens.go:170`.
- **`web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.** `select(null)` runs before `del.mutate(object.id)`. If the DELETE fails (network/403/500), `onError` (objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the selection in the delete mutation's `onSuccess` (or in the `mutate` call's `onSuccess` option) rather than eagerly before the request. Verified by reading `Inspector.tsx:191-194` and `objects.ts:163-179`.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
I reviewed the diff and the checked-out files through the security lens:
- **Authn/Authz**: The `gardenEditorRoute` retains its `requireAuth` guard; the route search param `focus` is validated (positive integer only) before reaching the canvas.
- **Injection**: No SQL/command/path/template injection vectors. API endpoints consume structured request bodies (`ObjectCreate`, `ObjectPatch`), not string concatenation. The `api.patch`/`api.post` paths are hardcoded.
- **SSRF**: All API URLs are hardcoded `/api/v1/…` paths; no user-supplied URLs are fetched.
- **Unsafe deserialization / secret leakage**: None found. The `useId()` output is sanitized for the SVG pattern ID; no hardcoded secrets.
- **Input validation / tainted data**: Server responses are parsed through `zod` schemas (`serverObjectSchema`, `fullGardenSchema`). The `notes` field is rendered via React’s textarea (escaped text content), not `dangerouslySetInnerHTML`. The SVG `<text>` node renders `{object.name}` through React’s normal escaping.
- **409 conflict / optimistic sync**: On a 409 the client adopts the server’s `current` row from the `ApiError` body, which is then `zod`-parsed before entering the cache.
Outside my lens: None.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run returned no output
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/SelectionOverlay.tsx:138-150`** — The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (`<ShapeBody shape={…} halfW={…} halfH={…} {...props} />`) so the shape fork lives in one place.
- **`web/src/lib/objects.ts:188-202`** — `applyServerPatch` manually spreads each of the 10 patchable fields with `!== undefined` guards. Adding a new patchable field requires touching this function, the interface, and every call site. Replace the 15-line wall with a loop over the known patch keys (or `Object.fromEntries` filtered by `!== undefined`) so new fields are automatically handled.
- **`web/src/editor/Inspector.tsx:89-130`** — The width/height/x/y fields are four near-copies of the same `<TextField>` boilerplate with only label/name/value/current-value/field-key differing. Extract a tiny `<DimField label={…} value={…} currentCm={…} onCommit={…} />` component to eliminate the repetition.
- **`web/src/editor/GardenCanvas.tsx:15-21`** — `defaultZForKind` hard-codes kind strings (`'path'`, `'structure'`, `'tree'`, etc.) that must be kept in sync with `kinds.ts`. If a kind is renamed or added there, this silently falls back to the wrong layer. Move the default z-index into the `KindDef` interface and the `OBJECT_KINDS` table so all kind metadata lives in one file.
- **`web/src/editor/GardenCanvas.tsx:129`** — The grid `<path>` and the garden boundary `<rect>` (line 136) were collapsed from readable multi-line props into single long lines. This is a readability regression; restore the multi-line formatting so diffs remain meaningful.
- **`web/src/editor/SelectionOverlay.tsx:168`** — All four resize handles are hard-coded to `cursor: 'nwse-resize'`. The top-right and bottom-left corners should use `nesw-resize`. Map the correct cursor per corner instead of using one value for all.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/GardenCanvas.tsx:82-85`** — `rendered` is recomputed (shallow-copy + z-sort) on every `liveObject` change. During a move/resize/rotate gesture, `SelectionOverlay` calls `setLiveObject` on every `pointermove` event, so the entire object list is sorted O(n log n) in a hot loop even though zIndex never changes during geometry gestures. For gardens with many objects this becomes noticeable.
**Fix:** memoize the z-sorted base array against `objects` alone, then merge `liveObject` into it without re-sorting.
- **`web/src/editor/SelectionOverlay.tsx:60-65`** — `pointerWorld` calls `svgRef.current?.getBoundingClientRect()` on every `pointermove` event during a drag/resize/rotate gesture. Layout reads in a sustained pointer loop can force synchronous layout work and contribute to frame drops. The SVG bounding rect is stable during object drag because viewport panning is disabled.
**Fix:** snapshot the SVG bounding rect once at gesture start (e.g., in the `begin` helper or in each `start*` handler) and reuse it in the window-level `pointermove` callback.
- **`web/src/pages/GardenEditorPage.tsx:51`** — `objects = full.data.objects.map(toEditorObject)` creates a new array on every parent render (e.g. when `selectedId` changes on tap). Because `GardenCanvas` takes `objects` as a prop, its `useMemo` for the sorted `rendered` array re-runs even when the underlying server data is unchanged.
**Fix:** wrap the `map` in `useMemo` so the array reference is stable across transient parent re-renders.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **Viewport zoom is not suppressed during object drag/resize/rotate**, so wheel or pinch mid-gesture changes `viewport.scale`, which `pointerWorld` reads on every move event. The delta between the gesture-start world point (computed with the old scale) and the current world point (computed with the new scale) is wrong, causing the selected object to jump erratically.
`web/src/editor/useViewport.ts:58-69`
*Fix:* Add the same guard that `onDrag` already has: `if (useEditorStore.getState().objectDragging) return` inside `onWheel` and `onPinch`.
- **`trackPointer` window listeners leak if `SelectionOverlay` unmounts mid-gesture** (e.g., object is deleted by another client, user navigates away, or inspector deletes the object). The `pointermove`/`pointerup`/`pointercancel` handlers stay on `window` until the pointer is released, and the `onEnd` closure will still call `update.mutate(fields(final))` after the component is gone, potentially patching the wrong object or interfering with a newly-mounted overlay.
`web/src/editor/SelectionOverlay.tsx:21-32`
*Fix:* Have `trackPointer` return a cleanup function; store it in a ref and invoke it from a `useEffect` cleanup.
- **Inspector checkbox (and color) PATCH on every `onChange` without debouncing**, so rapid toggles self-conflict with the optimistic-concurrency `version` check. Two quick clicks send two PATCHes with the same `version`; the first succeeds and bumps the server version, the second 409s and the cache rolls back to the server value, losing the user's second toggle.
`web/src/editor/Inspector.tsx:155,170`
*Fix:* Use local state for the checkbox (like the text fields do) and commit on blur, or debounce/throttle the mutation.
- **Inspector dimension fields accept negative values**, while the canvas resize enforces `MIN_OBJ_CM = 1`. Typing a negative width/height in the inspector sends a PATCH with negative cm, which corrupts the optimistic cache and can render the object inside-out until the server rejects it (if it even does).
`web/src/editor/Inspector.tsx:58-63`
*Fix:* Reject or clamp negative / near-zero values in `commitDim`, mirroring `SelectionOverlay`'s `Math.max(MIN_OBJ_CM, …)` logic.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Fix the real bugs surfaced by the PR #30 adversarial review (duplicates
across models collapsed):
- Optimistic PATCH now bumps the cached row version so a second edit
started before the first PATCH resolves sends the next version instead
of re-sending a stale one and self-conflicting (spurious 409 +
"updated elsewhere" toast + lost edit). Drop the per-PATCH onSettled
refetch: onSuccess already writes the authoritative row and onError
always rolls back, so the invalidate was a request storm during drags
and a refetch race, with no drift benefit.
- Color picker committed a PATCH on every onChange (continuous during a
native picker drag). Track the value locally and commit one PATCH on
blur.
- Rotate read shiftKey from the pointerdown event, so toggling Shift
mid-drag didn't switch snap/free. Pass the live pointermove event into
the gesture callback and read shiftKey from it.
- Window pointer listeners had no unmount cleanup: deleting/deselecting
an object mid-drag leaked listeners and could leave objectDragging
stuck true (freezing pan). Track the active gesture's cleanup and run
it on unmount.
- commitDim accepted 0/negative width/height (invalid geometry flashed
into the optimistic cache before rollback) and fired a spurious 1cm
PATCH on blur-without-edit from cm<->ft rounding drift. Guard
width/height against the server minimum and compare at display
precision first.
Perf:
- Split the z-order sort from the live-merge so a pointermove is an O(n)
map, not an O(n log n) re-sort.
- Memoize the toEditorObject map so object identities are stable across
renders and ObjectShape's memo holds.
- Snapshot the svg rect once per gesture instead of calling
getBoundingClientRect (a layout reflow) on every pointermove.
Maintainability:
- Move per-kind default z-index into kinds.ts (single source of truth).
- Rename the ToastItem `toast` param so it stops shadowing the module's
`toast` export.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
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 #11. Completes Phase 3 — the landscaping-editor half of pansy works end-to-end. Ties together #8/#9/#10.
What's here
lib/objects.ts— react-query overGET /gardens/:id/full+ optimistic create/update/delete. An update applies to the cache immediately and PATCHes with the rowversion; a 409 adopts the server'scurrentrow and toasts "updated elsewhere". One PATCH per gesture.editor/store.ts— addsliveObject(in-gesture geometry, rendered instantly),armedKind(palette tap-to-place), andobjectDragging(so the viewport's pan gesture stands down while an object drag owns the pointer — both listen on the same svg).editor/Palette.tsx+kinds.ts— the seven kinds with icons and the brief's default sizes; tap a kind → tap the canvas to place (desktop + touch).editor/SelectionOverlay.tsx— move (drag body), resize (corner handles, opposite corner stays put — computed in the object-local frame so rotation is honored), rotate (knob, snaps to 15°, free with Shift). Each updatesliveObjectfor instant feedback and fires exactly one PATCH on release.editor/Inspector.tsx— name, dimensions + position (unit-aware), rotation, color override (+ clear, using #10's nullable PATCH), plantable, notes, two-step delete. Commits per field on blur/change; re-syncs from the object unless a field is focused (so canvas drags update it without clobbering typing).GardenCanvas— placement, live-drag merge, z-ordered render (paths/structures under beds; explicitz_indexrespected), the selection overlay, and?focus=<id>frames an object on load (groundwork for #15).GardenEditorPageloads/fulland lays out palette · canvas · inspector (bottom sheet on mobile). New:components/ui/toast.tsx+Toaster.Verification (real browser, embedded binary)
tsc --noEmitclean;npx vitest run17 green;vite buildsucceeds. Screenshot shared in-session.Out of scope
🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-19 00:49:44Z
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.
@@ -0,0 +29,4 @@error: (m: string) => useToastStore.getState().push(m, 'error'),}function ToastItem({ toast }: { toast: Toast }) {🟡 ToastItem prop
toastshadows the same-file exportedtoasthelpermaintainability · flagged by 2 models
web/src/components/ui/toast.tsx:32—ToastItemproptoastshadows the imported/exportedtoasthelper. The component parameter{ toast }(line 32) shadows the module-levelexport const toast = {...}(lines 27-30) within the component body. It works (the component only readstoast.id/toast.tone/toast.message), but shadowing a same-file exported symbol is a readability trap — rename the prop todataort.🪰 Gadfly · advisory
@@ -11,2 +12,4 @@const GRID_MIN_CELL_PX = 6const GRID_MAX_OPACITY = 0.18// Kinds that render beneath beds by default; explicit z_index still wins.🟠 defaultZForKind hard-codes kind strings that must stay in sync with kinds.ts
maintainability · flagged by 3 models
web/src/editor/SelectionOverlay.tsx:138-150— The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. -web/src/lib/objects.ts:188-202—applyServerPatchmanually spreads each of the 10 patchable fields with!== undefinedguards. Adding a new patchable field requires touching this function,…🪰 Gadfly · advisory
@@ -54,0 +68,4 @@// ?focus=<id> frames that object (groundwork for #15's focus mode);// otherwise frame the whole garden.const focus = focusId != null ? objects.find((o) => o.id === focusId) : undefinedconst target: Rect = focus🔴 fittedForRef guard prevents re-framing when focusId changes for same garden
correctness · flagged by 2 models
web/src/editor/GardenCanvas.tsx:74-82— ThefittedForRef.current !== garden.idguard means the canvas frames the garden (or a?focus=object) exactly once per garden mount. If thefocusIdsearch param changes client-side without remounting the page, the viewport does not re-fit to the new object because the guard only tracksgarden.id, not the focus target. Track the last focused id and re-frame when it changes.🪰 Gadfly · advisory
@@ -60,2 +80,3 @@const ordered = useMemo(() => [...objects].sort((a, b) => a.zIndex - b.zIndex), [objects])// Merge the in-flight live geometry over the server objects, then z-order.const rendered = useMemo(() => {🟠 Full object array re-mapped and re-sorted on every drag pointermove instead of only repositioning the dragged object
maintainability, performance · flagged by 5 models
web/src/editor/GardenCanvas.tsx:82-85—renderedre-maps and re-sorts the fullobjectsarray on everyliveObjectchange (every pointermove during a drag, perSelectionOverlay'ssetLiveObjectcalls). Confirmed theuseMemodependency array is[objects, liveObject](line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmedGardenEditorPage.tsx:51(`full.data.objects.map(toEditorO…🪰 Gadfly · advisory
@@ -83,3 +129,1 @@strokeWidth={1}vectorEffect="non-scaling-stroke"/><path d={`M ${GRID_CM} 0 L 0 0 0 ${GRID_CM}`} fill="none" stroke="#808080" strokeOpacity={gridOpacity} strokeWidth={1} vectorEffect="non-scaling-stroke" />🟡 Grid path collapsed from multi-line to single long line, hurting readability
maintainability · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:138-150— The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. -web/src/lib/objects.ts:188-202—applyServerPatchmanually spreads each of the 10 patchable fields with!== undefinedguards. Adding a new patchable field requires touching this function,…🪰 Gadfly · advisory
@@ -114,3 +148,2 @@</div><Buttonvariant="ghost"<button🟡 Hand-rolled 'Fit' button duplicates the shared Button/buttonClasses ghost styling instead of reusing it
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:149-155— The "Fit" button was changed from<Button variant="ghost">to a raw<button>that hand-duplicates theButton/buttonClassesghost styling (border border-border,text-fg,hover:bg-border/50,outline-none focus-visible:ring-2 focus-visible:ring-accent/40,transition-colors— confirmed identical toweb/src/components/ui/Button.tsx:9-16). TheButtonimport was dropped from this file entirely. The prior code already usedclassNamet…🪰 Gadfly · advisory
@@ -0,0 +28,4 @@const select = useEditorStore((s) => s.select)// Local field state (initialized once; committed on blur/change).const [name, setName] = useState(object.name)🟡
web/src/editor/Inspector.tsx:31-37and:44-53— the field-init and re-sync logic duplicate the sameString(displayFromCm(...))/String(Math.round(...))conversions.maintainability · flagged by 1 model
web/src/editor/Inspector.tsx:31-37and:44-53— the field-init and re-sync logic duplicate the sameString(displayFromCm(...))/String(Math.round(...))conversions. SevenuseStateinitializers and theuseEffectboth spell out the same per-field formatting. A smallfieldsFromObject(object, unit)helper returning{ name, notes, width, height, x, y, rotation }would remove the duplication and make a future field a one-line add in both places. Trivial.🪰 Gadfly · advisory
@@ -0,0 +41,4 @@// When the object changes underneath us (e.g. a canvas move/resize/rotate, or// an optimistic PATCH result), re-sync the fields — unless the user is// actively editing one here, so we don't clobber their typing.useEffect(() => {🟠 Re-sync skips all inspector fields when any field has focus
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:44-53— The re-syncuseEffectaborts entirely when any field inside the inspector has focus (rootRef.current?.contains(document.activeElement)). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.🪰 Gadfly · advisory
@@ -0,0 +52,4 @@setRotation(String(Math.round(object.rotationDeg)))}, [object.version, object.widthCm, object.heightCm, object.xCm, object.yCm, object.rotationDeg, object.name, object.notes, unit])const patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) =>🔴 Inspector commitDim accepts zero and negative dimensions
error-handling, maintainability · flagged by 4 models
web/src/editor/Inspector.tsx:58-63—commitDimparses the raw input withparseFloatand sends the result without validating that it is positive or at leastMIN_OBJ_CM. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (MIN_OBJ_CM = 1). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: rejectv <= 0(orcm <= 0) incommitDimand surface a brief inline war…🪰 Gadfly · advisory
@@ -0,0 +59,4 @@const v = parseFloat(raw)if (!Number.isFinite(v)) returnconst cm = cmFromDisplay(v, unit)if (cm !== current) apply(cm)🟠 Blur of an unchanged dimension/position/rotation field fires a spurious PATCH due to lossy unit round-trip (e.g. 100cm -> 3.3ft -> 101cm), mutating the object and bumping version on a no-op focus/blur
correctness · flagged by 2 models
web/src/editor/Inspector.tsx:62— spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.commitDimruns on every blur and comparescmFromDisplay(v, unit)tocurrentwith no dirty guard. BecausedisplayFromCmrounds (imperial → 0.1 ft, metric → 2 dp cm) andcmFromDisplayre-quantizes to whole cm, the round trip is lossy. Verified: 100 cm →displayFromCmgives3.3ft →cmFromDisplay(3.3,'imperial')=round(3.3*12*2.54)= 101 cm, so…🪰 Gadfly · advisory
@@ -0,0 +86,4 @@onBlur={() => name !== object.name && patch({ name })}/><div className="grid grid-cols-2 gap-2">🟠 Four dimension TextFields are copy-pasted with only label/value differing
maintainability · flagged by 2 models
web/src/editor/SelectionOverlay.tsx:138-150— The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. -web/src/lib/objects.ts:188-202—applyServerPatchmanually spreads each of the 10 patchable fields with!== undefinedguards. Adding a new patchable field requires touching this function,…🪰 Gadfly · advisory
@@ -0,0 +148,4 @@<label htmlFor="obj-color" className="text-sm font-medium text-fg">Color</label><input🔴 Color onChange fires a PATCH per picker drag step → version storms and spurious 409 rollbacks with a misleading 'updated elsewhere' toast
correctness, error-handling, performance · flagged by 3 models
web/src/editor/Inspector.tsx:155— coloronChangefires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks. The native<input type="color">emitsinputevents continuously while the user drags through the OS color picker; React'sonChangemaps to thatinputevent. Each tick callspatch({ color }), sending the currentobject.version. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new versi…🪰 Gadfly · advisory
@@ -0,0 +167,4 @@<inputtype="checkbox"checked={object.plantable}onChange={(e) => patch({ plantable: e.target.checked })}🟠 Checkbox commits PATCH immediately onChange, rapid toggles self-conflict with version
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +188,4 @@variant="danger"className="flex-1"disabled={del.isPending}onClick={() => {🟡 On delete failure the object is restored to the cache but selection was already cleared, leaving it unselected on retry
error-handling · flagged by 1 model
web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.**select(null)runs beforedel.mutate(object.id). If the DELETE fails (network/403/500),onError` (objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the se…🪰 Gadfly · advisory
@@ -0,0 +16,4 @@[1, 1],[-1, 1],]🔴 Window pointer listeners leak when SelectionOverlay unmounts mid-drag
error-handling · flagged by 4 models
web/src/editor/SelectionOverlay.tsx:21-31—trackPointerattacheswindowlisteners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still onwindow. OnpointeruptheonEndclosure fires, callingsetObjectDragging(false)andupdate.mutate(fields(final))ag…🪰 Gadfly · advisory
@@ -0,0 +54,4 @@const halfW = object.widthCm / 2const halfH = object.heightCm / 2const handleCm = HANDLE_PX / scale🟠 getBoundingClientRect() called on every pointermove during drags — forces layout reflow each move event (layout thrash)
maintainability, performance · flagged by 4 models
web/src/editor/SelectionOverlay.tsx:60-65—getBoundingClientRect()called on everypointermoveduring a drag.pointerWorldreadssvgRef.current?.getBoundingClientRect()and is invoked from thetrackPointermove callback (line 78) on every pointer event.getBoundingClientRectforces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the livesetLiveObjectre-render is also mutating the SVG, so the layout is dirt…🪰 Gadfly · advisory
@@ -0,0 +85,4 @@)}const base = { ...object }🔴 Stale version captured in gesture closure causes spurious 409s
correctness · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:88—base = { ...object }snapshots the object'sversionat render time. The gesture-end PATCH sendsbase.version, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict during the drag. The fix is to read the current cached row'sversionwhen building the patch on release, or to derive it from t…🪰 Gadfly · advisory
@@ -0,0 +105,4 @@e,(world) => {const p = worldToLocal(world, center0, base.rotationDeg)const newW = Math.max(MIN_OBJ_CM, Math.abs(p.x - oppositeLocal.x))🟡 Resize clamps size to MIN_OBJ_CM but keeps recomputing/moving the center when the dragged corner crosses the fixed opposite corner.
error-handling · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:108— resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.newW/newHare clamped toMIN_OBJ_CM, butnewCenterLocalis still derived from the live pointer positionp, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.🪰 Gadfly · advisory
@@ -0,0 +119,4 @@)}const startRotate = (e: ReactPointerEvent) => {🟡 Rotation shiftKey state locked at gesture start
error-handling · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:122-134—startRotatecapturese.shiftKeyfrom the initialReactPointerEventclosure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: readshiftKeyfrom the livePointerEventpassed to the move handler instead of the stale closed-over React event.🪰 Gadfly · advisory
@@ -0,0 +125,4 @@(world) => {// +90° because the knob points up (local -y) at rotation 0.let deg = (Math.atan2(world.y - center0.y, world.x - center0.x) * 180) / Math.PI + 90if (!e.shiftKey) deg = Math.round(deg / ROTATE_SNAP_DEG) * ROTATE_SNAP_DEG🟠 Shift key state locked at gesture start instead of evaluated live during rotate
correctness · flagged by 3 models
web/src/editor/SelectionOverlay.tsx:128— Shift state captured at gesture start, not tracked during the drag. TheonMoveclosure instartRotatereferencese.shiftKeyfrom the originalpointerdownevent (if (!e.shiftKey) deg = Math.round(...)). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (ev.shiftKey) insideonMoveinstead of frome.🪰 Gadfly · advisory
@@ -0,0 +135,4 @@return (<g transform={`translate(${object.xCm} ${object.yCm}) rotate(${object.rotationDeg})`}>{/* Transparent body: drag to move. */}🟠 Duplicate circle-vs-rect shape rendering inside SelectionOverlay
maintainability · flagged by 1 model
web/src/editor/SelectionOverlay.tsx:138-150— The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place. -web/src/lib/objects.ts:188-202—applyServerPatchmanually spreads each of the 10 patchable fields with!== undefinedguards. Adding a new patchable field requires touching this function,…🪰 Gadfly · advisory
@@ -0,0 +127,4 @@serverObjectSchema.parse(await api.patch(`/objects/${id}`, body)),// Apply the change to the cache up front so the object doesn't snap back// while the request is in flight. Snapshot for rollback.onMutate: async (patch) => {🔴 Optimistic PATCH never bumps cached version, causing false 409 conflicts and silent loss of the user's own concurrent edits
correctness · flagged by 2 models
web/src/lib/objects.ts:130-137+:188-201(onMutate/applyServerPatch) — optimistic update never advances the cachedversion, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit.useUpdateObject'sonMutateapplies the patched fields to the cache viaapplyServerPatch, which copies over every field exceptversion(objects.ts:188-201). The cached object therefore still carries the pre-mutationversionafte…🪰 Gadfly · advisory
@@ -0,0 +156,4 @@toast.error(objectErrorMessage(err, 'Could not save that change.'))}},onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),🟠 Redundant full-garden refetch (invalidateQueries) after every object PATCH already synced via onMutate/onSuccess
performance · flagged by 1 model
web/src/lib/objects.ts:159(and177) —useUpdateObjectanduseDeleteObjectboth addonSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })even thoughonSuccess/onMutatealready write the authoritative result into the cache (patchFullCacheat lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch ofGET /gardens/:id/fullafter every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires o…🪰 Gadfly · advisory
@@ -0,0 +185,4 @@}/** Apply a patch onto a cached server object for the optimistic pass. */function applyServerPatch(o: ServerObject, patch: ObjectPatch): ServerObject {🔴 Optimistic PATCH doesn't bump cached version, causing false 409s that silently discard the user's own rapid sequential edits
error-handling, maintainability · flagged by 4 models
web/src/lib/objects.ts:188-201(applyServerPatch) — Ten near-identical...(patch.X !== undefined ? { X: patch.X } : {})lines, one perObjectPatchfield. Every time a field is added toObjectPatch(objects.ts:108-121), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.🪰 Gadfly · advisory
@@ -18,0 +48,4 @@heightCm: g.heightCm,unitPref: g.unitPref,}const objects = full.data.objects.map(toEditorObject)🟠 Unmemoized toEditorObject mapping breaks downstream memoization and causes unnecessary allocations
performance · flagged by 3 models
web/src/pages/GardenEditorPage.tsx:51—const objects = full.data.objects.map(toEditorObject)is unmemoized.toEditorObjectcreates a newEditorObjectinstance for every server object on every parent render. This defeats theReact.memoonObjectShape, forcing all objects to re-render wheneverGardenEditorPagere-renders (e.g., onselectedIdchange or background refetch). Fix: wrap the mapping inuseMemo(() => full.data.objects.map(toEditorObject), [full.data.objects])…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 31 findings (14 with multi-model agreement)
web/src/editor/GardenCanvas.tsx:82web/src/lib/objects.ts:188web/src/editor/Inspector.tsx:55web/src/editor/SelectionOverlay.tsx:19web/src/editor/SelectionOverlay.tsx:57web/src/editor/Inspector.tsx:151web/src/editor/GardenCanvas.tsx:15web/src/editor/SelectionOverlay.tsx:128web/src/pages/GardenEditorPage.tsx:51web/src/editor/GardenCanvas.tsx:71web/src/lib/objects.ts:130web/src/editor/Inspector.tsx:62web/src/editor/Inspector.tsx:89web/src/components/ui/toast.tsx:3217 single-model findings (lower confidence)
web/src/editor/SelectionOverlay.tsx:88web/src/editor/useViewport.ts:58web/src/editor/GardenCanvas.tsx:78web/src/editor/Inspector.tsx:44web/src/editor/Inspector.tsx:170web/src/editor/SelectionOverlay.tsx:138web/src/lib/objects.ts:159kinds.ts:5objects.ts:108types.ts:8web/src/editor/GardenCanvas.tsx:129web/src/editor/GardenCanvas.tsx:149web/src/editor/Inspector.tsx:31web/src/editor/Inspector.tsx:191web/src/editor/SelectionOverlay.tsx:108web/src/editor/SelectionOverlay.tsx:122web/src/editor/SelectionOverlay.tsx:168Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Good — no
dangerouslySetInnerHTML/innerHTML/evalanywhere, and the backend independently validates dimensions (validDimensionCM) and color format server-side, so a malicious/buggy frontend value can't corrupt stored data. That closes out my two candidate concerns (SVGfill:url()injection via unvalidated color, and unbounded width/height from the Inspector's raw number fields) — both are enforced server-side.I've completed my review. No security issues survived verification.
VERDICT: No material issues found
This PR is entirely frontend (React/TypeScript) editor UI wired to an existing, already-validated API. I checked the areas most likely to matter for this lens:
object.name/notesand toast messages are rendered as plain JSX text content (nodangerouslySetInnerHTML/innerHTMLanywhere inweb/src— verified via grep). React auto-escapes, so no XSS path.fill:ObjectShape.tsxrendersobject.colordirectly as an SVGfillattribute unvalidated on the client, which raised a concern about CSS/SVGurl()-based injection. Verified this is a non-issue:internal/service/objects.go(validDimensionCM) and the color-format check exercised byinternal/service/objects_test.go("bad color" test case) reject non-hex colors server-side, so arbitrary paint-server values can't reach storage even via a raw API call bypassing the<input type="color">UI.zodschemas (serverObjectSchema,fullGardenSchema) before use — malformed/unexpected server data fails parsing rather than being trusted blindly.conflictObject()correctly usessafeParseon the 409 body.credentials: 'same-origin'(cookie-based), no tokens handled or logged client-side.router.tsx's newvalidateSearchfor?focus=coerces to a positive integer or drops it entirely ({}), so it can't be used to inject anything into the DOM or downstream calls; it's only used to look up a local object by numeric id.id/versionin PATCH bodies come from already-fetched, schema-validated objects, not raw user text — no injection surface there.One thing outside my lens worth flagging for the correctness reviewer:
Inspector.tsx'scommitDimhas no lower-bound clamp (unlikeSelectionOverlay'sMIN_OBJ_CM), so a user could type a zero/negative dimension — not a security issue since the server enforcesvalidDimensionCM, but worth a UX-level fix.🎯 Correctness — Blocking issues found
Both findings confirmed exactly as described, at the correct line numbers. Line numbers are 71-73 rather than 71-72 exactly, but close enough and the substance is accurate.
VERDICT: Blocking issues found
web/src/lib/objects.ts:130-137+:188-201(onMutate/applyServerPatch) — optimistic update never advances the cachedversion, so any second PATCH fired before the first resolves gets a false 409 "conflict" and silently loses the user's own edit.useUpdateObject'sonMutateapplies the patched fields to the cache viaapplyServerPatch, which copies over every field exceptversion(objects.ts:188-201). The cached object therefore still carries the pre-mutationversionafter the optimistic write.Inspector.tsxbuilds each PATCH fromobject.versionread off that same cache/prop, and each field (Name, Width, Height, X, Y, Rotation, Notes) has its own independentonBlurhandler callingupdate.mutate(...)— nothing serializes these. Trace: user edits Name (blur → PATCH A,version: V), then edits Width before A returns (blur → PATCH B, alsoversion: V, since the cache's version hasn't moved). Server processes A first, bumps toV+1(confirmed server-side version-conflict path atinternal/api/objects.go:144-149, dispatching towriteVersionConflictondomain.ErrVersionConflict). B arrives with staleversion: Vand is rejected as a conflict. The client'sonErrorpath (objects.ts:145-158) then overwrites the cache with the server'scurrentrow (reflecting A's change but not B's) and shows "That object was updated elsewhere — your change was rolled back." The Width edit is silently discarded, and the user is told someone else changed the object, which isn't what happened.SelectionOverlay.tsxhas the identical issue:startMove/startResize/startRotateall capturebase.version = object.versionat gesture start (line 88), so two gestures on the same object performed back-to-back before the first gesture's PATCH resolves (e.g., move immediately followed by resize) race the same way.Fix direction: bump the cached
versionoptimistically inapplyServerPatch(e.g.version: o.version + 1), or serialize/queue mutations per object, or debounce/coalesce rapid field commits into a single PATCH.web/src/editor/GardenCanvas.tsx:70-73—?focus=<id>fit-to-rect ignores the object'srotationDeg, so a rotated object can be framed with its corners clipped outside the viewport.The target rect is built straight from
focus.widthCm/focus.heightCmas an axis-aligned box, but perDESIGN.md:30objects are positioned by center +rotation_degclockwise about that center (translate(x y) rotate(deg)), so a rotated rect's true screen-space extent is larger than its unrotated width/height (up to ~1.41× at 45°).zoomToFitRect(lib/geometry.ts:108-128) only adds a fixed padding margin (FIT_PADDING = 32,useViewport.ts:14) that doesn't scale with rotation, so a meaningfully-rotated focused object can end up with corners outside the fitted viewport on load. Low impact today since this is explicitly "groundwork for #15", but it is a real geometric miscalculation given the stated rotation convention.Fix direction: compute the rotated AABB (rotate all four corners, take min/max) before calling
fitToRect.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source.
VERDICT: Minor issues
web/src/editor/GardenCanvas.tsx:149-155— The "Fit" button was changed from<Button variant="ghost">to a raw<button>that hand-duplicates theButton/buttonClassesghost styling (border border-border,text-fg,hover:bg-border/50,outline-none focus-visible:ring-2 focus-visible:ring-accent/40,transition-colors— confirmed identical toweb/src/components/ui/Button.tsx:9-16). TheButtonimport was dropped from this file entirely. The prior code already usedclassNameto add the position/backdrop overrides (absolute bottom-3 right-3 bg-surface/90 py-1.5 shadow-sm backdrop-blur), so the same result was achievable by keeping<Button variant="ghost" className="...">— there's no functional reason for the divergence, andInspector.tsxcontinues to useButton variant="ghost"for equivalent ghost buttons (Inspector.tsx:160,198,203) in this same PR.web/src/editor/Inspector.tsx:89-130— The four dimension fields (Width/Height/X/Y) are copy-pastedTextFieldblocks that differ only by label, local state variable, and whichEditorObjectkey they patch viacommitDim. This could collapse to a small loop over a[{key, label, get, set}]table with one local-state map, removing the risk of the four blocks drifting (e.g. astep/inputModedivergence slipping in during a future edit).web/src/lib/objects.ts:188-201(applyServerPatch) — Ten near-identical...(patch.X !== undefined ? { X: patch.X } : {})lines, one perObjectPatchfield. Every time a field is added toObjectPatch(objects.ts:108-121), it must be independently added here too, and nothing enforces that — a forgotten field silently no-ops instead of failing to compile. Could be replaced with a small "merge only defined keys" helper.⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/lib/objects.ts:159(and177) —useUpdateObjectanduseDeleteObjectboth addonSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) })even thoughonSuccess/onMutatealready write the authoritative result into the cache (patchFullCacheat lines 139-144 for update, 170 for delete's optimistic removal). This triggers a full refetch ofGET /gardens/:id/fullafter every successful PATCH/DELETE, in addition to the request itself. Since the Inspector fires one PATCH per field per blur (name, width, height, x, y, rotation, color, plantable, notes) and drags fire one PATCH per gesture, this doubles network work for every one of those interactions.useCreateObject(line 99-102) deliberately avoids this pattern ("avoids a full refetch flash"), reinforcing that the invalidate in update/delete is inconsistent with the rest of the file. Fix: drop theonSettledinvalidate for update/delete.web/src/editor/SelectionOverlay.tsx:61—pointerWorld()(lines 60-65) callssvgRef.current?.getBoundingClientRect()on everypointermove, wired throughtrackPointer'swindow.addEventListener('pointermove', ...)(lines 21-32) and invoked frombegin()'sonMove(line 78). Each move also callssetLiveObjectwhich re-renders the SVG subtree (confirmed viaGardenCanvas.tsx'srenderedmemo keyed onliveObject), so this is a read-after-write layout query on every drag frame. By contrast,useViewport.ts'sclientToCanvas(lines 37-43) does the same rect read only inonWheel/onPinch(lines 61, 67), not inonDrag(lines 48-56, which usesdeltadirectly with no rect read) — confirming this is a hotter instance of the pattern than the codebase's existing convention. Fix: measure the rect once inbegin()/gesture start and reuse it for the drag's duration.web/src/editor/GardenCanvas.tsx:82-85—renderedre-maps and re-sorts the fullobjectsarray on everyliveObjectchange (every pointermove during a drag, perSelectionOverlay'ssetLiveObjectcalls). Confirmed theuseMemodependency array is[objects, liveObject](line 85), so it recomputes each frame even though only one object's position/size changes and z-order among the rest is unaffected. Additionally confirmedGardenEditorPage.tsx:51(full.data.objects.map(toEditorObject)) produces a new array identity on every render of the page, so theobjectsdependency itself never stabilizes across unrelated re-renders (e.g., aselectedIdchange causesGardenEditorPageto re-render, producing a newobjectsarray, invalidating this memo too). Fix: memoize the sorted server-object list keyed only onobjectscontent, and splice the live-dragged object's position into that stable order separately.🧯 Error handling & edge cases — Blocking issues found
Confirms no
pointerIdfiltering orsetPointerCaptureanywhere in the frontend. All three findings in the draft are verified against the actual code.VERDICT: Blocking issues found
web/src/lib/objects.ts:130-136,188-203— Optimistic PATCH never bumps the cachedversion, so two edits to the same object issued in quick succession (before the first PATCH's response lands) cause a false 409 that silently discards the second, entirely-valid edit.applyServerPatch(objects.ts:188-202) copies every patched field onto the cached row exceptversion.Inspector.patch()(Inspector.tsx:55-56) readsobject.versionstraight from that cache, so a second rapid mutation on the same object is sent with the stale version. Verified server-side (internal/store/objects.go:93-118): the UPDATE isWHERE id = ? AND version = ?withversion = version + 1, so the first PATCH bumps the row's version and the second is genuinely rejected asErrVersionConflicteven though only one client touched it.useUpdateObject'sonError(objects.ts:145-158) can't distinguish this from a real two-client conflict — it adopts the server'scurrentrow (discarding the second field's change) and shows "That object was updated elsewhere — your change was rolled back," which is false here.Fix: bump the cached version optimistically in
applyServerPatch(or serialize/queue per-object mutations while one is in flight).web/src/editor/Inspector.tsx:58-63— Width/Height/X/Y fields accept zero, negative, or absurdly large values with no client-side validation, unlike the drag-resize path.commitDimonly checksNumber.isFinite(v); it never callsisValidDimensionCm(web/src/lib/units.ts:16-18, bounds[1, 10_000]cm). Server-side,finalizeObject(internal/service/objects.go:234) rejects out-of-range width/height viavalidDimensionCMwith a 400 (not 409). SinceObjectShape.tsx:47-48clamps rendered half-dimensions toMath.max(0, …), entering e.g.0optimistically makes the object vanish from the canvas, then the 400 triggers the generic rollback path (objects.ts:154-157) restoring the old value with a "Could not save that change" toast — a confusing flash-then-revert instead of an upfront rejection/clamp. (Note: for X/Y specifically the server's actual bound is a garden-overlap check,bboxOverlapsGarden, not theisValidDimensionCmrange — but the same flash-then-revert failure mode applies there too via a 400.)Fix: validate with
isValidDimensionCmfor width/height (and an appropriate bound for X/Y) incommitDimbefore callingpatch(...).web/src/editor/SelectionOverlay.tsx:19-30—trackPointer's window-levelpointermove/pointeruplisteners don't filter byevent.pointerId, and no handle callssetPointerCapture. Confirmed by code inspection: nopointerIdorsetPointerCaptureusage anywhere underweb/src/editor/(or the rest ofweb/src/). On a multi-touch device, a second finger touching the canvas while a move/resize/rotate gesture from the first finger is active will have its pointermove events processed by the same active gesture handler, corrupting the geometry computation.Fix: capture the pointer on gesture start and filter the window listeners by
event.pointerId.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Through the security lens I traced the client-supplied data to its server-side enforcement and found the change is adequately guarded:
POST /gardens/:id/objects,PATCH /objects/:id,DELETE /objects/:id,GET /gardens/:id/full) is behindrequireAuth+ arequireGardenRole/objectForRole(roleEditor/roleViewer)check ininternal/service/objects.go(verifiedCreateObject/UpdateObject/DeleteObject/GardenFull). The actor is resolved from the HttpOnly session cookie (api.ts:97credentials: 'same-origin'), not from any client-supplied id. A user can only touch objects in gardens they own/edit; cross-user access returns NotFound/Unauthorized (perobjects_test.goBob-vs-Alice cases).kind,shape, dimensions, andcolorare all validated infinalizeObject(internal/service/objects.go:220+): unknown kinds rejected,isHexColorenforced, name length capped, out-of-bounds dimensions rejected. So the optimisticcreate.mutate({kind: def.kind, ...})and inspector color/null-clear paths are server-validated regardless of client trust. The client'sMIN_OBJ_CMclamp and#rrggbbcolor input are UX only; the server is the authority.toast.tsx) and inspector fields (name,notes) are rendered as React text children — nodangerouslySetInnerHTMLanywhere in the diff orObjectShape.tsx(verified).err.message/conflict text surfaced in toasts originates from server-controlledApiErrorbodies, not user input.?focus=<id>(router.tsx): validated as a positive integer and only used to frame the viewport — it does not authorize fetching;GardenFullreturns only the objects the viewer is entitled to see, so a guessed id can't reveal anything.conflictObjectinobjects.ts): the 409body.currentis server-asserted and re-parsed throughserverObjectSchema; it overwrites the cache with the server row, which is the correct trust direction.No injection (SQL/command/path), SSRF, unsafe deserialization, secret leakage, missing validation, or insecure defaults in this change.
🎯 Correctness — Minor issues
All three findings verify against the actual source. Confirming each:
Finding 1 —
commitDim(Inspector.tsx:58-63) comparescmFromDisplay(v, unit)tocurrentwith no dirty guard. Thewidth/height/x/ystate is initialized fromString(displayFromCm(...))and re-synced the same way (lines 33-36, 48-51). Re-deriving the imperial round-trip: 100 cm →displayFromCm=round(100/2.54/12 * 10)/10= 3.3 →cmFromDisplay(3.3,'imperial')=round(3.3*12*2.54)= 101, so101 !== 100firespatch({widthCm:101})on a no-op blur. Metric drifts for non-integer cm (a free resize yields floats; e.g. 123.4 cm → "1.23" → 123). RotationonBlur(line ~135) comparesv !== object.rotationDegagainst the raw value while the field showsMath.round(...), so 30.5° → "31" → commits 31. Confirmed.Finding 2 —
useUpdateObject.onMutate(objects.ts:130-138) callsapplyServerPatch(188-202) which does not touchversion;onSuccess(139-144) replaces the row with the server row (bumped version).SelectionOverlaycapturesconst base = { ...object }(line 88) and PATCHesversion: base.versionon release (lines 96/118/132). Noupdate.isPendingguard exists inSelectionOverlayorInspector. A second gesture begun before the first PATCH resolves reads the still-old cacheversion, sends a stale version, and 409s →onErroradopts the server row and toasts "rolled back." Confirmed.Finding 3 —
startRotate'sonMoveclosure (SelectionOverlay.tsx:125-130) referencese.shiftKeyfrom the pointerdowne, whiletrackPointerinvokesonMove(pointerWorld(ev))with the liveevnot passed in. So Shift must be held at pointerdown; pressing/releasing mid-drag has no effect, contradicting the "free with Shift" comment. Confirmed.VERDICT: Minor issues
web/src/editor/Inspector.tsx:62— spurious PATCH on blur of an unchanged dimension/position field, with unit-dependent drift.commitDimruns on every blur and comparescmFromDisplay(v, unit)tocurrentwith no dirty guard. BecausedisplayFromCmrounds (imperial → 0.1 ft, metric → 2 dp cm) andcmFromDisplayre-quantizes to whole cm, the round trip is lossy. Verified: 100 cm →displayFromCmgives3.3ft →cmFromDisplay(3.3,'imperial')=round(3.3*12*2.54)= 101 cm, so on an imperial garden focusing a Width/Height/X/Y field and blurring without typing firespatch({ widthCm: 101 }), silently mutating the object by 1 cm and bumping its version. Metric drifts too for non-integer cm (a free resize produces floats, e.g. 123.4 cm →"1.23"→123). The RotationonBlurhas the same shape: state isString(Math.round(object.rotationDeg)), so a free rotation of 30.5° displays"31"and a focus/blur commits31. Fix: guard no-op blurs by comparing the raw string toString(displayFromCm(current, unit))(or track a dirty flag set inonChange); for rotation compare against the rounded current rather than rawobject.rotationDeg.web/src/lib/objects.ts:130— back-to-back gestures self-conflict on version.onMutateapplies the patch viaapplyServerPatchbut does not bumpversion; the version only advances ononSuccess(server row).SelectionOverlayreadsobject.versionfrom the cache at gesture start (base.version) and PATCHes with it on release. If a second gesture begins before the first PATCH resolves,base.versionis still the pre-edit version, so the second PATCH carries a stale version; once the first PATCH lands (serverversion+1), the second request 409s andonErroradopts the server row + toasts "rolled back" — even though no other client touched it. There is noupdate.isPendingguard inSelectionOverlayorInspector. Fix: optimistically incrementversioninonMutate(reconcile ononSuccess/onError), or disable further gestures while a mutation for that object id is pending.web/src/editor/SelectionOverlay.tsx:128— Shift state captured at gesture start, not tracked during the drag. TheonMoveclosure instartRotatereferencese.shiftKeyfrom the originalpointerdownevent (if (!e.shiftKey) deg = Math.round(...)). Pressing or releasing Shift mid-rotate has no effect; "free with Shift" only works if Shift was held at pointerdown. Fix: read Shift state from the live pointer event (ev.shiftKey) insideonMoveinstead of frome.🧹 Code cleanliness & maintainability — Minor issues
Review
Verdict: Minor issues
Through the code-cleanliness & maintainability lens, I verified each finding against the checked-out source. All five survive independent confirmation:
web/src/editor/SelectionOverlay.tsx:57-58— two sources of truth for scale.scale = useEditorStore((s) => s.viewport.scale)(line 52) feeds the render-timehandleCm/rotateOffsetCm(lines 57-58), whilepointerWorld(lines 60-65) re-readsuseEditorStore.getState().viewporton every move. The split is functionally intentional (scale can change mid-gesture), but with no comment a reader can't tell thathandleCmis hit-size-only. Minor readability footgun — consider a clarifying comment.web/src/lib/objects.ts:188-201—applyServerPatchis a verbose per-field conditional spread. Lines 189-201 repeat...(patch.x !== undefined ? { x: ... } : {})ten times, one perObjectPatchfield. A genericObject.entries(patch)filter-and-merge (excludingid/version) would be ~3 lines and wouldn't drift fromObjectPatchas fields are added.web/src/editor/GardenCanvas.tsx:81-85—liveObjectmerge silently no-ops on id mismatch. Line 83objects.map((o) => (o.id === liveObject.id ? liveObject : o))does nothing if the live object's id isn't inobjects(e.g. a delete racing a drag). The merge reads as ifliveObjectalways corresponds to a cached row; a one-line comment (or an early return) would make the intent explicit.web/src/editor/GardenCanvas.tsx:16-21— kind z-index defaults split from kind metadata.UNDER_KINDS/defaultZForKindencode per-kind z-ordering policy here, whilekinds.ts(lines 5-22) already owns per-kind default sizes/labels/shape. The "paths/structures under beds, trees above" ordering that the render sort (a.zIndex - b.zIndex, line 84) relies on is placement-time policy split across two files. Low-impact leaky split of kind metadata.web/src/components/ui/toast.tsx:32—ToastItemproptoastshadows the imported/exportedtoasthelper. The component parameter{ toast }(line 32) shadows the module-levelexport const toast = {...}(lines 27-30) within the component body. It works (the component only readstoast.id/toast.tone/toast.message), but shadowing a same-file exported symbol is a readability trap — rename the prop todataort.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens I checked the hot paths (per-pointermove drag updates, pan/zoom re-renders, mutation cache writes) and found no material regression:
SelectionOverlay/GardenCanvasdrag loop: eachpointermovedoes onesetLiveObject→ oneGardenCanvasre-render. Work per frame is bounded: a single-object merge (objects.mapwith one replacement) + one[...merged].sort(). The sort is O(n log n) worst case but V8's Timsort is adaptive for the nearly-sorted z-order that changes by one element, so it's effectively ~O(n) per frame. No N+1, no per-element queries. Verified atGardenCanvas.tsx:82-85andSelectionOverlay.tsx:77-84.getBoundingClientRect()is called per pointer event (SelectionOverlay.tsx:61,GardenCanvas.tsx:99); it's a cheap layout read, not a forced reflow beyond what's needed for coordinate conversion. Fine on the hot path.useUpdateObject/useDeleteObject) rewrite the cache withobjects.map(...)— O(n) per mutation, one per gesture end, not inside the drag loop. Verified atobjects.ts:130-144.nextIdmodule counter + array filtered on 4s timeout dismiss; no unbounded growth. Verified attoast.tsx:18-24.One trivial, non-material observation (not worth blocking on):
GardenCanvas.tsx:86recomputesselected = rendered.find(...)on every render, andGardenCanvasre-renders on every viewport change (pan/zoom) and every liveObject update. That's an extra O(n) scan per frame on top of the sort. For a v1 garden (dozens of objects) this is negligible; if gardens ever hold thousands of objects it's a trivial fix (deriveselectedfromselectedIdagainstobjectsdirectly, or memoize). I'm noting it only because it's genuinely in the hot render path; impact is not material at current scale.🧯 Error handling & edge cases — Minor issues
Both surviving findings are confirmed against the actual code:
Color input PATCH on every onChange —
Inspector.tsx:155callspatch({ color: e.target.value })directly inonChange, which fires continuously during a color-slider drag. Unlike geometry fields (which commit ononBlur), this fires a PATCH per intermediate value, all with the sameobject.version, so concurrent edits can race and trigger 409/rollback. Confirmed.Resize center drift past corner —
SelectionOverlay.tsx:108-115:newW/newHare clamped toMIN_OBJ_CM, butnewCenterLocaland thusckeep recomputing fromp(the dragged pointer position) even when the pointer crosses the fixed opposite corner. So the center follows the pointer while size is pinned at minimum — a visible UX edge. Confirmed.The other draft items (lost-update on Inspector version, click-without-move no-op, GardenCanvas armed-kind consumption, rotate Shift freeze, focus-on-deleted-object) were already self-withdrawn in the draft or are non-issues, so they are dropped.
VERDICT: Minor issues
web/src/editor/Inspector.tsx:155— coloronChangefires a PATCH per pixel of a slider drag. The<input type="color">commits viaonChangedirectly intopatch({ color }), unlike the geometry fields (one PATCH per gesture / on blur). Each intermediate value carries the sameobject.version, so rapid changes race; a 409 mid-drag rolls back to the server value and toasts, discarding the user's in-progress selection. Minor — a real unhappy path but bounded.web/src/editor/SelectionOverlay.tsx:108— resize recomputes/moves the center even when the dragged corner crosses the fixed opposite corner.newW/newHare clamped toMIN_OBJ_CM, butnewCenterLocalis still derived from the live pointer positionp, so once the pointer passes the opposite corner the object stays at minimum size while its center keeps sliding with the pointer. Visible UX edge, non-crashing, minor.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
Through the security lens, I scrutinized the optimistic sync layer (
lib/objects.ts), the gesture-based canvas interactions (editor/SelectionOverlay.tsx,editor/GardenCanvas.tsx), the inspector form (editor/Inspector.tsx), routing (router.tsx), and the toast system (components/ui/toast.tsx).What I checked:
gardenEditorRoutegates onrequireAuth; the backend is responsible for garden-scoped authorization on/gardens/:id/fulland object mutations.object.id,gardenId). No user-controlled strings reach URL paths. No SQL/command/template injection surfaces.object.nameis rendered as a React text child in SVG<text>— automatically escaped, nodangerouslySetInnerHTML. Toast messages are hardcoded strings. No inline event handlers or HTML injection vectors.?redirect=parameter on/loginis funneled throughsafeRedirectPath, which rejects protocol-relative and absolute URLs. The new?focus=parameter is validated to a positive integer and only used to look up an object by ID; it never drives navigation.useUpdateObjectrequiresversionand correctly handles 409 conflicts by rolling back to the server's row. This is a security-positive pattern that prevents lost-update races.serverObjectSchema(Zod) validates all API responses.kindDefrestricts created kinds to the known allow-list. Color inputs are constrained to<input type="color">.No authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, or insecure defaults were identified.
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/editor/SelectionOverlay.tsx:88—base = { ...object }snapshots the object'sversionat render time. The gesture-end PATCH sendsbase.version, so if the server's version increments during the drag (another field edit, another client, or a background refetch), the mutation 409s and rolls back the user's gesture even though there was no edit conflict during the drag. The fix is to read the current cached row'sversionwhen building the patch on release, or to derive it from the latestliveObject/ query data rather than the stale closure.web/src/editor/GardenCanvas.tsx:74-82— ThefittedForRef.current !== garden.idguard means the canvas frames the garden (or a?focus=object) exactly once per garden mount. If thefocusIdsearch param changes client-side without remounting the page, the viewport does not re-fit to the new object because the guard only tracksgarden.id, not the focus target. Track the last focused id and re-frame when it changes.web/src/editor/GardenCanvas.tsx:78-80— The focus rectangle for?focus=<id>treats the object's bounds as axis-aligned:{x: focus.xCm - focus.widthCm/2, y: focus.yCm - focus.heightCm/2, w: focus.widthCm, h: focus.heightCm}. If the object has a non-zerorotationDeg, its axis-aligned bounding box is larger thanwidthCm × heightCm(by up to a factor of|cos|+|sin|).zoomToFitRectwill clip the rotated corners. Compute the rotated AABB or at least pad the rect.web/src/editor/SelectionOverlay.tsx:122-134— InstartRotate,e.shiftKeyis evaluated from the initialReactPointerEventcaptured in the closure. Because theonMovecallback only receives the worldPoint(not the current nativePointerEvent), holding or releasing Shift mid-gesture has no effect; snap/free mode is locked at pointer-down. Pass the current event's modifier state into the move callback so Shift can be toggled live during the drag.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Inspector.tsx,SelectionOverlay.tsx,ObjectShape.tsx,lib/objects.ts,useViewport.ts,editor/store.ts, and the newcomponents/ui/toast.tsx. The diff is well-factored and follows the repo's existing patterns.lib/geometry.ts,cn(),Button,TextField) are reused rather than copy-pasted.liveObject,armedKind, andobjectDraggingare self-describing; gesture helpers inSelectionOverlay(startMove,startResize,startRotate) are concise and accurate.localToWorld/worldToLocalinlib/geometry.ts;applyServerPatchis a flat sequence of conditional spreads, which is appropriate.toast.tsxuses the samecn()utility as the rest of the UI,Inspectoruses the sameTextField/TextAreaprimitives, and the react-query mutations follow the same optimistic-update structure already in the codebase.lib/geometry.ts, and the canvas doesn't leak SVG details into the store or data layer.⚡ Performance — Minor issues
Minor issues
web/src/pages/GardenEditorPage.tsx:51—const objects = full.data.objects.map(toEditorObject)is unmemoized.toEditorObjectcreates a newEditorObjectinstance for every server object on every parent render. This defeats theReact.memoonObjectShape, forcing all objects to re-render wheneverGardenEditorPagere-renders (e.g., onselectedIdchange or background refetch). Fix: wrap the mapping inuseMemo(() => full.data.objects.map(toEditorObject), [full.data.objects]).web/src/editor/GardenCanvas.tsx:82-85— During move/resize/rotate gestures,liveObjectupdates on everypointermoveframe, yet therenderedmemo performs an O(n).map()copy plus O(n log n).sort()on each frame to merge a single changed object. For gardens with many objects this creates jank and allocation pressure at interaction frequency even though z-order is unchanged during a single-object gesture. Fix: keep a stable z-sorted array and only re-sort when z-indices change, or split the liveObject merge from the sort and only sort whenobjectsitself changes.web/src/editor/Inspector.tsx:154-155— The color<input>firespatch({ color: e.target.value })on everyonChangeevent. A native color picker emits change events continuously while dragging, generating a burst of optimistic cache patches, query cancellations, and network PATCHes. Fix: debounce the color mutation (e.g., 200 ms) or use local state for the live preview and commit on blur.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/editor/SelectionOverlay.tsx:21-31—trackPointerattacheswindowlisteners for pointermove/up/cancel but never cleans them up if the component unmounts mid-gesture. If the user navigates to a different garden, the object is deleted by another client, or a background update removes the selection, the overlay unmounts while the drag listeners are still onwindow. OnpointeruptheonEndclosure fires, callingsetObjectDragging(false)andupdate.mutate(fields(final))against the old garden's mutation hook, potentially PATCHing the wrong context or a deleted object. Fix: store theendcleanup in a ref and invoke it in auseEffectcleanup, or use auseEffect-based listener pattern so React tears the listeners down on unmount.web/src/editor/Inspector.tsx:58-63—commitDimparses the raw input withparseFloatand sends the result without validating that it is positive or at leastMIN_OBJ_CM. This allows zero and negative dimensions to be PATCHed to the server, which the canvas resize logic explicitly forbids (MIN_OBJ_CM = 1). The inspector and canvas are inconsistent, and negative sizes can corrupt the SVG bounding boxes. Fix: rejectv <= 0(orcm <= 0) incommitDimand surface a brief inline warning instead of callingapply.Minor issues
web/src/editor/Inspector.tsx:44-53— The re-syncuseEffectaborts entirely when any field inside the inspector has focus (rootRef.current?.contains(document.activeElement)). If another client updates the object while the user is typing in the Notes field, the Width/Height/X/Y/Rotation fields do not refresh and remain stale even though the user is not actively editing them. Fix: track focus per-field (e.g., by field name) and only block re-sync for the focused field.web/src/editor/Inspector.tsx:151-157— The color<input>usesonChange, committing a PATCH on every mouse/touch adjustment inside the color picker. Because each PATCH carries the currentversion, rapid changes can launch overlapping requests; the first succeeds and bumps the version, causing subsequent in-flight requests to 409 and roll back. Fix: commit color ononBlur(or debounce with a short trailing edge) so the gesture completes before versioning is contested.web/src/editor/SelectionOverlay.tsx:122-134—startRotatecapturese.shiftKeyfrom the initialReactPointerEventclosure. The snap/free behavior is therefore locked to the Shift state at pointer-down; the user cannot press or release Shift mid-gesture to toggle snapping, which contradicts typical drawing-tool behavior. Fix: readshiftKeyfrom the livePointerEventpassed to the move handler instead of the stale closed-over React event.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I traced tainted input through every sink this PR introduces, and checked the actual checked-out code (not just the diff) for each path below:
Server → UI text sinks:
object.name,object.notes,err.message, andkindDef(object.kind)?.label ?? object.kindare all rendered as React text children (<text>,<h2>,<TextField value>, toast{toast.message}). NodangerouslySetInnerHTML/innerHTML/evalexists anywhere inweb/src(grep confirmed). React escapes text content, so server-controlled strings can't XSS. Verified inObjectShape.tsx:95-107,Inspector.tsx:70,toast.tsx:42-44.Server response parsing:
serverObjectSchema.parse/fullGardenSchema.parse(zod) gate every server payload entering the cache, including the optimisticonSuccesssplices and the 409 conflict body viaconflictObject'ssafeParse(objects.ts:206-211). The 409 path readserr.body.currentasunknownand only adopts it ifsafeParsesucceeds — no trust of raw body shape.URL-derived
?focus=<id>:router.tsx:93-96coerces withNumber(search.focus)and gates onNumber.isInteger(f) && f > 0. Arrays/objects/strings like"1; ..."becomeNaNand are rejected (returns{}). The surviving integer is only used asobjects.find(o => o.id === focusId)(GardenCanvas.tsx:70) — a numeric lookup against this garden's already-validated object list, never interpolated into a URL or attribute. If the id isn't found,targetfalls back togardenRect. No injection / IDOR surface introduced here.URL path interpolation in new mutations (
objects.ts:70,98,127,166):/gardens/${gardenId}/...and/objects/${id}usegid = Number(gardenId)(route param) andobject.id/patch.id(numbers off zod-validated server rows or local numeric state).Number()of a non-numeric route segment would yieldNaN, which the server would reject — not an injection vector. No user-controlled string reaches a path segment.SVG
fill/strokeattributes:object.color(server-controlled, schemaz.string().nullable().optional()) flows intofill={fill}(ObjectShape.tsx:74,88) and the color input'svalue(Inspector.tsx:154). React sets SVG attributes viasetAttributewith string values — attribute-value quoting prevents breakout, so a value like" onmouseover=...can't inject a handler. The<input type="color">itself coerces its value to a valid#rrggbbor#000000. No scripting surface.Mutation request bodies:
ObjectCreate/ObjectPatchare typed objects built from local form/geometry state, serialized viaJSON.stringifyinapi.ts:90. No template/SQL/string-concat assembly; no injection.Auth: All new requests go through
apiFetch, which sendscredentials: 'same-origin'and is gated byrequireAuthinbeforeLoadongardenEditorRoute(router.tsx). No unauthenticated mutation paths were added.CSRF: Cookie-auth with no CSRF token is a pre-existing property of
api.ts, not something this PR introduces or worsens; out of this PR's scope and not a regression.Nothing in the security lens is materially wrong here.
🎯 Correctness — Minor issues
All three findings confirmed against the actual source. Verifying line numbers and the mutation/conflict path.
All three findings are confirmed against the actual code. Correcting the line numbers: the color
onChangeis at line 155 (not 131), the imperial round-trip drift is incommitDimat line 62 (not 73), ande.shiftKeyis at line 128 (not 142).Corrected review
VERDICT: Minor issues
web/src/editor/Inspector.tsx:155— coloronChangefires a PATCH on every picker drag step, causing version storms and spurious 409 rollbacks. The native<input type="color">emitsinputevents continuously while the user drags through the OS color picker; React'sonChangemaps to thatinputevent. Each tick callspatch({ color }), sending the currentobject.version. The first PATCH succeeds (N→N+1), but a second PATCH emitted before the cache re-renders with the new version still carries version N → the server returns 409 →conflictObjectadopts the server row and toasts "That object was updated elsewhere — your change was rolled back." The user's color reverts with a misleading conflict toast, even though they were the only editor. Every other field commits onblur; color is the lone exception. Fix: commit color ononBlur(capture the pending value in local state viaonChange, dispatch the PATCH on blur) — or debounce. Verified against the Inspector and theuseUpdateObject409 path (web/src/lib/objects.ts:145-158).web/src/editor/Inspector.tsx:62— imperial unit round-trip drift can fire a spurious PATCH (and 1 cm change) on blur-without-editing.displayFromCm(240, 'imperial')→7.9ft; if the user focuses a dimension/position field and blurs without typing,commitDimrunscmFromDisplay(7.9, 'imperial')=Math.round(7.9 * 12 * 2.54)=241, which differs from the current240, so it PATCHes a 1 cm change the user never made. Metric round-trips exactly (so this only bites imperial gardens). Verified againstweb/src/lib/units.ts:31-44. Fix: skip the PATCH when the parsed display value equals the field's current display value, or compare on the rounded display value rather than raw cm.web/src/editor/SelectionOverlay.tsx:128— rotate usese.shiftKeyfrom the pointerdown event, not the live pointermove.eis theReactPointerEventcaptured in thestartRotateclosure at pointerdown, so pressing/releasing Shift during the drag does not toggle free-rotate vs. snap. The spec ("free with Shift", per the component doc comment on line 38) implies live Shift state. Minor UX/correctness gap. Fix: readev.shiftKeyinside theonMovecallback (thetrackPointerPointerEventcarries it).Verified-clean (no issue): resize opposite-corner math in the object-local frame (
worldToLocal/localToWorldare true inverses); rotate+90°offset matches the knob's local-yplacement;?focus=<id>fit rect (center→top-left conversion); z-order merge withliveObject; conflict adoption on 409;serverObjectSchemamatches the server'sGardenObjectJSON tags;color: nullPATCH path; delete needs no version.🧹 Code cleanliness & maintainability — Minor issues
All five findings verified against the actual source.
VERDICT: Minor issues
web/src/lib/objects.ts:188-202—applyServerPatchrepeats the same conditional-spread idiom for all 10 patchable fields. EachObjectPatchfield is hand-mirrored here, so adding a new patchable field means editing both the interface (108-121) and this helper in lockstep, with nothing enforcing they stay in sync. A genericObject.entries(patch).filter(([k]) => k !== 'id' && k !== 'version')fold would collapse the body and make it field-agnostic. Verified by reading the file.web/src/editor/Inspector.tsx:55-56—patchis typed againstEditorObject, not the realObjectPatch, so it accepts fields the server ignores.patch = (fields: Partial<Omit<EditorObject, 'id' | 'version'>>) => update.mutate({ id, version, ...fields }).EditorObject(types.ts:8-23) includeskindandshape, butObjectPatch(objects.ts:108-121) has neither. Because the fields are spread into the mutate argument (excess-property checks don't fire on spreads),patch({ kind: 'tree' })orpatch({ shape: 'circle' })typechecks cleanly yet is silently dropped by the server. Typing the arg asOmit<ObjectPatch, 'id' | 'version'>would let the compiler catch these. Verified acrossInspector.tsx,types.ts, andobjects.ts.web/src/editor/GardenCanvas.tsx:16-21— per-kind default-z knowledge is split fromkinds.ts.UNDER_KINDS/defaultZForKindencode per-kind layering metadata that belongs inKindDef(kinds.ts:5-12), which already holds each kind's label/icon/shape/default size. Adding a new kind means editingkinds.tsand remembering to updateUNDER_KINDShere. AdefaultZ(orlayer: 'under' | 'default' | 'over') field onKindDefwould centralize it and letGardenCanvasreadkindDef(kind).defaultZ. Verified by reading both files.web/src/editor/Inspector.tsx:31-37and:44-53— the field-init and re-sync logic duplicate the sameString(displayFromCm(...))/String(Math.round(...))conversions. SevenuseStateinitializers and theuseEffectboth spell out the same per-field formatting. A smallfieldsFromObject(object, unit)helper returning{ name, notes, width, height, x, y, rotation }would remove the duplication and make a future field a one-line add in both places. Trivial.web/src/components/ui/toast.tsx:32— thetoastprop name shadows the module'stoastexport.function ToastItem({ toast })shadows thetoastobject exported at line 27. Not a bug (the component doesn't use the export), but a reader scanning the file seestoast.id/toast.messageand has to disambiguate whichtoastis meant. Renaming the prop todataoritem(or the export totoastApi) would remove the ambiguity. Trivial.⚡ Performance — Minor issues
All three findings confirmed against actual code.
All three findings confirmed against the actual code. The store confirms
setLiveObjectis called per pointermove (line 78 of SelectionOverlay),liveObjectis a dep of therendereduseMemo (line 85), and the page-level.map(toEditorObject)runs every render (line 51).VERDICT: Minor issues
web/src/editor/SelectionOverlay.tsx:60-65—getBoundingClientRect()called on everypointermoveduring a drag.pointerWorldreadssvgRef.current?.getBoundingClientRect()and is invoked from thetrackPointermove callback (line 78) on every pointer event.getBoundingClientRectforces a synchronous layout/reflow, and during a move/resize/rotate this fires dozens of times per second — exactly when the livesetLiveObjectre-render is also mutating the SVG, so the layout is dirty. This is the textbook layout-thrash pattern and the most likely cause of jank on lower-end devices, especially with touch. Fix: capture the rect once at gesture start (inbegin) and pass it intopointerWorld, or cache it on thebasesnapshot. The viewporttx/ty/scaleare already read fresh from the store viauseEditorStore.getState().viewport, so the rect (which only changes on layout/scroll) is the only thing that needs caching.web/src/editor/GardenCanvas.tsx:82-85— re-sort on every pointermove during a gesture.rendereddepends onliveObject(line 85), which changes on everypointermoveof a move/resize/rotate viasetLiveObject, so[...merged].sort((a, b) => a.zIndex - b.zIndex)runs O(n log n) (plus a fresh array + anobjects.mapmerge) on every move event. Z-order does not change during a geometry gesture, so this is wasted work. For typical gardens it's negligible, but it scales with object count and event rate. Suggested fix: split the merge from the sort, and only sort whenobjectschanges (e.g. sort once withuseMemo([objects]), then mergeliveObjectinto the already-ordered list).web/src/pages/GardenEditorPage.tsx:51—objectsarray rebuilt with new identities every render.full.data.objects.map(toEditorObject)produces a fresh array of fresh object literals on every render ofGardenEditorPage.ObjectShapeismemo'd (verified atObjectShape.tsx:37), but the memo is defeated because each render hands it a brand-newobjectreference. This means every parent re-render (selection change, query invalidation after a PATCH'sonSettled, focus param change) re-renders every object, even unchanged ones. It's not on the pan/zoom hot path (pan only re-rendersGardenCanvas, not the page), so impact is limited, but after each gesture-release PATCH theonSettledinvalidation → refetch → full remap → all objects re-render. Consider memoizing the mapping (useMemoonfull.data.objects) or mapping server objects to editor objects once when the query returns.🧯 Error handling & edge cases — Minor issues
I've verified all four findings against the actual source. All confirmed.
Verdict: Minor issues
web/src/editor/Inspector.tsx:155— color picker fires a PATCH on everyonChange, storming the server with stale-version 409s.<input type="color">emitsonChangecontinuously while the user drags in the native picker. Each call invokespatch({ color: e.target.value })(Inspector.tsx:55-56), which readsobject.versionfrom the prop. The optimisticapplyServerPatch(objects.ts:188-202) overrides only the patched field and never touchesversion;onSuccess(objects.ts:139-144) only replaces the row after the network round trip. So the 2nd..Nth PATCHes in a single color drag all carry the same staleversionas the first. The server accepts PATCH1 (vN→vN+1) and returns 409 for PATCH2..N.useUpdateObject.onError(objects.ts:145-157) treats every 409 as a conflict: it adoptscurrentand toasts'That object was updated elsewhere — your change was rolled back.'. Net effect: a single color drag spams conflict toasts and cache churn even though the user is the only one editing. Fix: debounce the color commit, or use local state + commit-on-blur like the other fields. Verified againstInspector.tsx:155,objects.ts:188(applyServerPatchnever setsversion),objects.ts:145-153, andapi.ts:25-27(isConflict= status 409).**
web/src/editor/SelectionOverlay.tsx:21-32, 77-86 — window pointer listeners leak if the overlay unmounts mid-gesture;objectDraggingcan get stuck.**trackPointerregisterspointermove/pointerup/pointercancelonwindowand removes them only from insideend. There is nouseEffectcleanup inSelectionOverlay. If the overlay unmounts during a drag — e.g. the selected object is deleted out-of-band soselectedbecomesnullinGardenCanvas.tsx:86and the overlay is removed at line 142 — thewindowlisteners remain registered. When the pointer is finally released,onEndrunsupdate.mutate(...)against a now-staleupdate(PATCH on a deleted/unknown object → 404 → "Could not save that change." toast), andsetObjectDragging(false)/setLiveObject(null)run on the global store (still safe, since the store is module-level). More importantly, if the gesture'spointerupnever fires (rare iframe/pointer-capture edge cases),objectDraggingstaystrueanduseViewport's pan (useViewport.ts:50) is permanently disabled. Fix: register the cleanup in auseEffectwhose cleanup also removes the listeners and resetsobjectDragging/liveObject. Verified by readingSelectionOverlay.tsx:20-86,GardenCanvas.tsx:86,142, anduseViewport.ts:50`.**
web/src/editor/Inspector.tsx:58-63 —commitDimaccepts 0/negative values, briefly corrupting the optimistic cache.**commitDimonly checksNumber.isFinite(v); typing0or-5for width/height passes the guard,cmFromDisplay(v, unit)(units.ts:31-33) rounds to0/negative cm, andonMutate(objects.ts:130-138) applies that to the cache. The render inGardenCanvasthen draws the object with a zero/negative width (thewidth={object.widthCm}flips; the overlay'shalfW = object.widthCm/2goes negative). The server rejects viavalidDimensionCM(gardens.go:170-172,v >= minGardenCMwhereminGardenCM = 1),onErrorrolls back toctx.prev, and the field re-syncs. Net effect is a brief visual glitch and a wasted network round trip — not data loss, but an avoidable unhappy path. Suggested fix: clamp the parsed value to>= MIN_DIMENSION_CM(units.ts:12) before patching, matching the gesture path'sMIN_OBJ_CMclamp inSelectionOverlay.tsx:108. Verified by readingInspector.tsx:58-63,units.ts:31, andgardens.go:170`.**
web/src/editor/Inspector.tsx:191-194 — on delete error the cache rolls back but selection stays cleared.**select(null)runs beforedel.mutate(object.id). If the DELETE fails (network/403/500),onError(objects.ts:173-176) restores the object to the cache, but the inspector is already gone (selection cleared at Inspector.tsx:192). The object reappears on the canvas unselected, and the user has to re-select to retry. Minor, but an unhappy-path inconsistency. Suggested fix: clear the selection in the delete mutation'sonSuccess(or in themutatecall'sonSuccessoption) rather than eagerly before the request. Verified by readingInspector.tsx:191-194andobjects.ts:163-179`.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I reviewed the diff and the checked-out files through the security lens:
gardenEditorRouteretains itsrequireAuthguard; the route search paramfocusis validated (positive integer only) before reaching the canvas.ObjectCreate,ObjectPatch), not string concatenation. Theapi.patch/api.postpaths are hardcoded./api/v1/…paths; no user-supplied URLs are fetched.useId()output is sanitized for the SVG pattern ID; no hardcoded secrets.zodschemas (serverObjectSchema,fullGardenSchema). Thenotesfield is rendered via React’s textarea (escaped text content), notdangerouslySetInnerHTML. The SVG<text>node renders{object.name}through React’s normal escaping.currentrow from theApiErrorbody, which is thenzod-parsed before entering the cache.Outside my lens: None.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/SelectionOverlay.tsx:138-150— The circle-vs-rect conditional is repeated twice (transparent drag body + selection outline) with only a few props differing. Extract a small helper (<ShapeBody shape={…} halfW={…} halfH={…} {...props} />) so the shape fork lives in one place.web/src/lib/objects.ts:188-202—applyServerPatchmanually spreads each of the 10 patchable fields with!== undefinedguards. Adding a new patchable field requires touching this function, the interface, and every call site. Replace the 15-line wall with a loop over the known patch keys (orObject.fromEntriesfiltered by!== undefined) so new fields are automatically handled.web/src/editor/Inspector.tsx:89-130— The width/height/x/y fields are four near-copies of the same<TextField>boilerplate with only label/name/value/current-value/field-key differing. Extract a tiny<DimField label={…} value={…} currentCm={…} onCommit={…} />component to eliminate the repetition.web/src/editor/GardenCanvas.tsx:15-21—defaultZForKindhard-codes kind strings ('path','structure','tree', etc.) that must be kept in sync withkinds.ts. If a kind is renamed or added there, this silently falls back to the wrong layer. Move the default z-index into theKindDefinterface and theOBJECT_KINDStable so all kind metadata lives in one file.web/src/editor/GardenCanvas.tsx:129— The grid<path>and the garden boundary<rect>(line 136) were collapsed from readable multi-line props into single long lines. This is a readability regression; restore the multi-line formatting so diffs remain meaningful.web/src/editor/SelectionOverlay.tsx:168— All four resize handles are hard-coded tocursor: 'nwse-resize'. The top-right and bottom-left corners should usenesw-resize. Map the correct cursor per corner instead of using one value for all.⚡ Performance — Minor issues
Minor issues
web/src/editor/GardenCanvas.tsx:82-85—renderedis recomputed (shallow-copy + z-sort) on everyliveObjectchange. During a move/resize/rotate gesture,SelectionOverlaycallssetLiveObjecton everypointermoveevent, so the entire object list is sorted O(n log n) in a hot loop even though zIndex never changes during geometry gestures. For gardens with many objects this becomes noticeable.Fix: memoize the z-sorted base array against
objectsalone, then mergeliveObjectinto it without re-sorting.web/src/editor/SelectionOverlay.tsx:60-65—pointerWorldcallssvgRef.current?.getBoundingClientRect()on everypointermoveevent during a drag/resize/rotate gesture. Layout reads in a sustained pointer loop can force synchronous layout work and contribute to frame drops. The SVG bounding rect is stable during object drag because viewport panning is disabled.Fix: snapshot the SVG bounding rect once at gesture start (e.g., in the
beginhelper or in eachstart*handler) and reuse it in the window-levelpointermovecallback.web/src/pages/GardenEditorPage.tsx:51—objects = full.data.objects.map(toEditorObject)creates a new array on every parent render (e.g. whenselectedIdchanges on tap). BecauseGardenCanvastakesobjectsas a prop, itsuseMemofor the sortedrenderedarray re-runs even when the underlying server data is unchanged.Fix: wrap the
mapinuseMemoso the array reference is stable across transient parent re-renders.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
Viewport zoom is not suppressed during object drag/resize/rotate, so wheel or pinch mid-gesture changes
viewport.scale, whichpointerWorldreads on every move event. The delta between the gesture-start world point (computed with the old scale) and the current world point (computed with the new scale) is wrong, causing the selected object to jump erratically.web/src/editor/useViewport.ts:58-69Fix: Add the same guard that
onDragalready has:if (useEditorStore.getState().objectDragging) returninsideonWheelandonPinch.trackPointerwindow listeners leak ifSelectionOverlayunmounts mid-gesture (e.g., object is deleted by another client, user navigates away, or inspector deletes the object). Thepointermove/pointerup/pointercancelhandlers stay onwindowuntil the pointer is released, and theonEndclosure will still callupdate.mutate(fields(final))after the component is gone, potentially patching the wrong object or interfering with a newly-mounted overlay.web/src/editor/SelectionOverlay.tsx:21-32Fix: Have
trackPointerreturn a cleanup function; store it in a ref and invoke it from auseEffectcleanup.Inspector checkbox (and color) PATCH on every
onChangewithout debouncing, so rapid toggles self-conflict with the optimistic-concurrencyversioncheck. Two quick clicks send two PATCHes with the sameversion; the first succeeds and bumps the server version, the second 409s and the cache rolls back to the server value, losing the user's second toggle.web/src/editor/Inspector.tsx:155,170Fix: Use local state for the checkbox (like the text fields do) and commit on blur, or debounce/throttle the mutation.
Inspector dimension fields accept negative values, while the canvas resize enforces
MIN_OBJ_CM = 1. Typing a negative width/height in the inspector sends a PATCH with negative cm, which corrupts the optimistic cache and can render the object inside-out until the server rejects it (if it even does).web/src/editor/Inspector.tsx:58-63Fix: Reject or clamp negative / near-zero values in
commitDim, mirroringSelectionOverlay'sMath.max(MIN_OBJ_CM, …)logic.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.